Skip to content

fix(debates): claim the recordings key after reading the URLs, not before (GEO-2950) - #2476

Merged
ohohoreilly merged 1 commit into
masterfrom
ohohoreilly/geo-2950-claim-the-key-after-commit
Sep 20, 2026
Merged

ohohoreilly merged 1 commit into
masterfrom
ohohoreilly/geo-2950-claim-the-key-after-commit

Conversation

@ohohoreilly

Copy link
Copy Markdown
Contributor

Follow-up to #2449, which merged as a8b83f69. It removed a permanent Loading…; this closes a narrower version of the same bug that the fix left behind.

The gap

#2449 changed the effect to claim fetchedForRef only once URLs are committed, instead of up front. Correct, but the claim still lands one line before the values are read:

fetchedForRef.current = recordingsKey;
setUrls({ slot1: slot1Result.url, slot2: slot2Result.url });

geoChatRequest returns undefined on a 204 (core/debates/api.ts:1642), so slot1Result.url is a TypeError rather than a rejected request. It throws after the claim and lands in the .catch, which deliberately does not release — so the key stands over null URLs. The card shows its error, and every later activation takes the fetchedForRef.current === recordingsKey early return and never asks again.

That is the permanent Loading… #2449 set out to remove, reached through a narrower door. The .catch comment — "The key was never claimed" — is true for a rejected request and false for this.

The fix

Read both fields before claiming. That makes "never claimed" true for every throw that can reach the catch, and needs nothing else.

Deliberately not fixed by releasing in the .catch. That is what the code did before #2449, and because the key is identical across attempts, it let a stale cancelled attempt free a claim a newer in-flight one owned — the original defect. The catch stays exactly as it is, now with a comment saying why, so this does not get "tidied" back into the old bug.

Test

Drives a 204-shaped answer, then scrolls away and back, and asserts the card asks again rather than latching.

I checked that it bites rather than assuming: reverted to the previous ordering and the test fails; restored it and it passes. A regression test that passes without the fix is worth nothing.

Verification

105 files / 1612 tests pass across core/debates; tsc clean on the touched file.

Context

Found while reviewing #2449, reported there as a medium finding, and merged before it was addressed — so it is live on master now. Related to GEO-2950, where the permanent-grey user report sits.

@vercel

vercel Bot commented Sep 20, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
geogenesis Ready Ready Preview Sep 20, 2026 1:41am UTC

Request Review

…fore (GEO-2950)

#2449 removed a permanent "Loading…" by claiming `fetchedForRef` only once URLs are
committed, rather than up front. The claim still happens one line too early, which leaves
a narrower version of the same bug.

`geoChatRequest` returns `undefined` on a 204 (`core/debates/api.ts`), so `slot1Result.url`
is a TypeError rather than a rejected request. It throws *after* the key is claimed and
lands in the `.catch`, which deliberately does not release — so the key stands over null
URLs, the card shows the error, and every later activation takes the
`fetchedForRef.current === recordingsKey` early return and never asks again. The card is
then stuck behind "Loading…" for the rest of the session, which is exactly what #2449 set
out to remove.

Reading both fields before claiming makes "the key was never claimed" true for every throw
that can reach the catch.

Deliberately NOT fixed by releasing in the `.catch`. That is what the code did before
#2449, and because the key is identical across attempts it let a stale cancelled attempt
free a claim a newer in-flight one owned — the original defect. Ordering is the fix; the
catch stays as it is, with a comment saying why.

The regression test drives a 204-shaped answer and asserts the card asks again after
scrolling away and back. Checked that it bites: it fails against the previous ordering.

Verified: 105 files / 1612 tests pass across core/debates; tsc clean.
@ohohoreilly
ohohoreilly force-pushed the ohohoreilly/geo-2950-claim-the-key-after-commit branch from 404f68a to e3f27ae Compare September 20, 2026 01:38
@ohohoreilly
ohohoreilly merged commit 91166eb into master Sep 20, 2026
4 checks passed
@ohohoreilly
ohohoreilly deleted the ohohoreilly/geo-2950-claim-the-key-after-commit branch September 20, 2026 01:52

This branch was successfully deployed

1 active deployment
Preview — e3f27ae7 Deployed Sep 20, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant