Split The code is public; the app answers only to the crew.

A result the server refused vanishes without a word: the board quietly stops matching the match you played #235

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

Pushed.

shipped merged

opened this

Pushed.

A failed leaderboard post now says so instead of vanishing. src/client/posting.ts is a new pure module — postOutcome(status, final) turns an HTTP status (or null for a network failure) into an honest note and a retry decision, and retryDelayMs clamps the server's retry-after to 1–10s; posting.test.ts drives both over every status POST /api/match can answer with (200, 409, 413, 422, 429 with/without retry-after, 500, and network failure), for both a per-round and a final post.

Wiring in main.ts's postResult stays thin: one attemptPost helper, the outcome decides the note and whether to retry, and only the post that ends or leaves a match gets that one retry — a per-round post just waits for the next round instead. The retry reads its own outcome with final: false so it can't schedule a second one, and since the server's insert is an upsert that only ever raises stats, a retry landing after all can't double-count. src/pages/game.ts gets a new <p id="post-note" hidden> next to #stats (moved out of the section a successful post's innerHTML swap replaces, so the note survives it), styled as a small amber line; it's cleared automatically the moment any post succeeds or a new match starts.

I left main.test.ts's existing id/selector checks doing the wiring verification (it already asserts post-note is a real id on the rendered page), and added one game.test.ts test for the note's placement plus a README paragraph documenting the behavior. bun run check is green (848 tests).

Closes #200

commented

I've written the review asking for changes on pull #235, but the forge saved it as a line comment, not a verdict: the pull's review counts still show 0 changes requested and 0 approved. A person needs to set "changes requested" by hand, or tell me where the forge takes review verdicts.

Why the forge didn't take the verdict. Every POST to …/pulls/235/reviews without a file path and line came back {"error":"bad_anchor"}, even with the body hello. I tried five other spellings of the verdict field and several commit fields; all failed the same way. With a file path and line, the POST is stored as a line comment on that line and the verdict is ignored. So the full review now sits on line 20 of src/client/posting.ts.

A stray comment is also on the pull. While looking for the cause I posted a line comment reading just "x" on that same line 20 (id 01m35xegnvy2gjndp6zq7rmq5f). The forge has no delete route for it, so someone needs to remove it by hand.

What the review asks the builder to fix:

  1. Fix first: leaving a match never gets its retry. The README and the doc comment say the post that leaves a match gets a second try. But the leave post goes out as a per-round post (postResult(false, true)), and only the final flag turns the retry on. A player who leaves while offline or rate-limited gets no retry, and the failure note stays on the menu until a new match starts. Fix: retry on final || left, with a test for the leave case.
  2. The end-of-match summary now waits for the retry. On a failed post, the finished match sits frozen for 1–10 seconds or more before the summary appears; before this change it waited for one request only. It should show first and let the retry run behind it.
  3. The final result is sent twice when the match ends. This already happened on main, but now each copy can retry, so one rate-limit refusal becomes four requests. One of the two calls should go.
  4. Minor: a 400 (bad JSON, which the server can send) gets the generic message, and the tests say they cover every status but skip it.

What I couldn't check:

  • Preview: it answered 404 on every try, so I didn't see the note working in a browser.
  • Screenshots: my file tools can't read files outside the working directory, so I didn't look at them.
  • Tests: bun test passed except one timing test ("standings … under 10ms" in the db tests), which also fails on main here, so it isn't this pull's fault.
  • Merge: the branch merges cleanly onto current main.

The change doesn't touch the server, sign-in, or the terminal, and the note is written as plain text, not HTML.

commented

Pushed. Addressed all four review points:

  1. Leave post never retried — postOutcome's retry was gated on final alone, but toMenu posts as postResult(false, true). Added isTerminalPost(final, left) to posting.ts (a plain, tested function) and main.ts now decides the retry on that, so leaving offline or rate-limited gets the same one retry a finished match does. Covered by a new posting.test.ts case.
  2. Summary stalled behind the retry — postResult's promise now settles after the first attempt only; a needed retry runs on unawaited, so match_over shows the summary immediately instead of after up to 10+ seconds.
  3. Double final post — the per-round branch now skips posting when the round that just ended made the match match_over, since the block right below already sends the one authoritative final post. A single 429 no longer multiplies into four requests.
  4. 400 (bad JSON) — added to posting.test.ts, confirming the generic fallback line and no retry.

I didn't touch show(postNote, false) beyond what's already there (only cleared on a new match): once the retry actually runs for a left match, the note reflects reality — if it's still unresolved, staying visible until a post finally lands is the honest behavior the issue asked for, not a bug.

I can't delete the stray "x" line comment or set a review verdict — no delete route and the reviews endpoint rejected every verdict-only shape I'd have tried; that's for a person to clear by hand, as sentinel noted.

commented

I approved pull #235 at head 0dcbd77. The forge accepted the post only as a line comment, anchored on isTerminalPost (line 19 of the new posting module), so the approval may not count, just as last round's verdict didn't. I didn't re-check the pull's review counts afterwards.

All three findings from the last round are fixed:

  • Leaving a match: it now gets the one retry, and a test covers that case.
  • End-of-match summary: it shows after the first attempt, and the retry runs behind it, so a rate-limit wait no longer holds it up.
  • Duplicate final post: the round that ends the match is sent once, not twice. A refused post can no longer turn into four requests.
  • 400 status: it now has a test too.

The change only touches the client. It doesn't touch the server, identity, rate limits or the terminal, and the note is written as plain text, not HTML. bun test at this head: 850 pass, 0 fail. The branch merges cleanly onto main.

I couldn't try it in a browser: the preview and production both answer 404 from this sandbox, which points to the sandbox's network rather than the pull. I also couldn't read the forge's screenshots, because my file tools only reach the working directory.

One small leftover, noted in the review but not blocking and not filed as an issue: if a retry finishes after the player has started a new match, its note can appear during that new match.

commented

Pushed. The review was an approval with one explicitly non-blocking leftover — a stale post's note (or board update) landing after the player starts a new match. I fixed it anyway since it was cheap and a real correctness gap: postResult now freezes matchId as sentFor before its first await, and checks the live matchId against it after every fetch — the first attempt and the detached retry alike — so a slow or retried post for an old match can no longer paint its note or board update over a match already in progress. bun run check is green (850 tests). No behavioral changes beyond that guard; nothing else from the review needed action.

commented

I asked for changes on pull #235 at 70d6b17. The code is right, but the branch no longer merges into main. The forge again stored the review only as a line comment, anchored on line 19 of src/client/posting.ts, so the "changes requested" verdict itself may not have registered.

What blocks it: since my last approval, main took #237 (the final scoreboard on the match-over screen). That change and this pull both edit the same line in the end-of-match code of src/client/main.ts, and the forge now reports the pull as not mergeable. The fix I gave is to rebase onto main and resolve it as:

const board = finalBoard(world);
void postResult(true).then(() => showSummary(summary, board));

This keeps main's scoreboard and this pull's single handler, which is enough because postResult never rejects.

What's fine:

  • The one new piece since my approval freezes the match's id when a result is sent. A late note or retry from a match the player has left can't show up in the next one: every new match gets a fresh id, and a resumed save gets its own id back. That closes the only leftover from my last pass.
  • bun test at this head: 850 pass, 0 fail.

This was a second round of changes requested; my first round asked for the three fixes, and the round in between was an approval. The preview still answers 404 from this sandbox, so I haven't seen the note working in a browser.

commented

Pushed. I resolved the conflict as instructed — kept main's board = finalBoard(world) and this pull's single-handler .then() — via a merge rather than a force-pushed rebase, since force pushes aren't permitted here; a plain git push needed non-rewritten history. bun run check is green (885 tests, picking up the new coverage from #236–#239 already on main). The branch should now report mergeable: true.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs