Every kill since the season board shipped was thrown away - #228
Merged
Conversation
OWNER, looking at the live Season 2 board: *"aku test login pakai email, tapi di
leaderboard tidak tercatat jumlah kills nya??"* — his row read 1,095 XP and 0
kills. So did every other row.
One line in the wrong place. Firestore rejects a transaction that reads after it
writes:
"Firestore transactions require all reads to be executed before all writes."
recordRunKills gained a season mirror and the mirror's `tx.get` went in BELOW
the career `tx.set`. Every call threw, the catch turned it into a console line
nobody was watching, and because Firestore buffers a transaction's writes until
it commits, NOTHING landed — not the season kills the change was for, and not
the career kills that had worked for months.
XP was fine on the same rows, which is exactly why nobody caught it:
recordRunProgress does one read and two writes and was never affected. A column
that reads 0 looks like a column nobody has earned yet.
The fix is to hoist both reads above both writes. The value is in the test.
scripts/test-leaderboard-writes.js runs the real functions against a stub that
enforces Firestore's constraint AND buffers writes the way a real transaction
does — a stub that applied writes as they were made would have shown the career
column surviving and understated the damage. Reverted against the old code it
fails four assertions, including both totals reading empty.
It also asserts the RULE rather than this instance: no transaction anywhere in
the module may read after it writes. The next person to add a mirror gets a red
test instead of a silently empty column.
The delta logic the throw was hiding is now covered too — a Revive keeps
p.kills climbing, so 23 then 30 must add 7; a fresh run reports 5 and must add
5, not subtract 25.
Verified: 8/8 test:kills, tsc clean, lint unchanged.
NOT included, because they need the owner's decision rather than a guess: the
food drop rate and the props he reported near a doorway. Measurements for both
are in the PR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017A764RdnwpyWnG7uCNhMiQ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The owner's Season 2 row read 1,095 XP and 0 kills. So did every other row on the board.
One line in the wrong place. Firestore rejects a transaction that reads after it writes:
recordRunKillsgained a season mirror, and the mirror'stx.getwent in below the careertx.set. Every call threw, thecatchturned it into a console line nobody was watching, and because Firestore buffers a transaction's writes until it commits, nothing landed — not the season kills the change was for, and not the career kills that had worked for months.XP was fine on the same rows, which is exactly why it went unnoticed:
recordRunProgressdoes one read and two writes and was never affected. A column that reads 0 looks like a column nobody has earned yet.The fix is two lines. The value is in the test.
scripts/test-leaderboard-writes.jsruns the real functions against a stub that enforces Firestore's constraint and buffers writes the way a real transaction does. That second part matters: a stub applying writes as they were made would have shown the career column surviving and understated the damage.Reverted against the shipped code it fails four assertions:
It also asserts the rule, not this instance: no transaction anywhere in the module may read after it writes. The next person to add a mirror gets a red test instead of a silently empty column.
The delta logic the throw was hiding is covered too — a Revive keeps
p.killsclimbing, so 23 then 30 must add 7; a fresh run reports 5 and must add 5, not subtract 25.What this does not recover
Kills earned while this was broken are gone — they were never written anywhere. The counter starts working from the next run after deploy. Season 2's kill column will be honest from that point but understated for anyone who played before it.
Since ranking is by XP, not kills, no prize was misallocated.
Testing
Two other reports, measured but not changed here
Food drops. Not removed — never common. Food is item ids 321–515 of a 1,244-icon library, so 15.6% of item drops are food, confirmed over 200k simulated rolls. The scarcity is upstream: only 18 of 52 breakables can drop an item at all, and the best is
chestat 47% item → 7.3% food. The props you actually smash dozens of per floor —hay_pile1.25%,vase/pot/bones/rubble0% — are the reason it feels absent. This is a tuning decision, not a bug, so it is left for the owner.Props near a doorway. Could not reproduce. Across 60 generated floors and 642 doorways, zero had a prop inside them, and no prop stood within 1.6 tiles of any door centre. The engine already guarantees the lift is reachable and that unbreakables never seal a room or a container. Awaiting more detail before changing placement rules that are currently measuring clean.
Generated by Claude Code