Skip to content

feat(claims): put debates and their extracted claims in the claim's thread (GEO-3008) - #2550

Merged
jwalkingjew merged 38 commits into
masterfrom
preston/geo-3008-claim-activity-feed
Sep 26, 2026
Merged

jwalkingjew merged 38 commits into
masterfrom
preston/geo-3008-claim-activity-feed

Conversation

@jwalkingjew

@jwalkingjew jwalkingjew commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Linear: GEO-3008 · Design spec + mockups

The problem

A claim page ended in a comment thread listing only what people typed, with the debates held on that claim parked in a gallery above it. Two places showing the same argument — and neither showing the claims that came out of those debates. The thing a reader most wants to follow (what was actually said, by whom, and when) was the one thing the page never surfaced.

What this does

The comment thread becomes Activity. Debates are ordered in among the comments by when they happened; expanding one lists the claims extracted from its transcript — attributed to the debater who said them, in the order they were said, each votable in place and each linking back to the moment on the video.

Debate row — published keyframe still, then debaters + Debate + date on the header line, the claim it argued as the body, and upvote/downvote, claim count, comment count, Watch debate beneath. The debaters are the link to the debate. No separate Reply: the comment count opens the composer in place, and two controls for one box was one too many.

Extracted claim row — speaker · timecode, the sentence, then one line of controls: EntityVoteButtons (thumbs + agree share, responder faces trailing) and EntityCommentsButton. Deliberately not the full explore card: that card is right when the claim is the thing being read and far too much when six hang off one debate two levels into a thread. The full treatment is one click away.

Debates are rows in the thread now. The gallery above it stays, because it is the route to the Debates tab — the complete, filterable index — where the thread shows the most recent few in the order they happened, in among the comments. Two jobs, not two copies.

Decisions worth a look

Ordering comes from claim-timing.ts and nowhere else. The graph's relation order looks chronological and is not: those relations are published with Position.generate(), which is random rather than monotonic. Reading them as a transcript would give a stable, arbitrary, entirely plausible-looking sequence — the kind of bug that passes review. Claims nothing could place sit in a tail after the timed ones rather than being sorted among themselves; there is nothing to sort them by.

A timing good enough to sort by is not always good enough to state. The timecode chip and the jump link appear only above isAssertableMoment's 0.40 bar. A block fallback lands at the start of the turn — sometimes most of a minute early — so it orders the row correctly and claims nothing about when it happened.

Keyframe, not a mounted player. Three debate rows would otherwise be three DebateFeedPlayers fetching and decoding at once for a reader scrolling past all of them. useDebateKeyframes batches the two-hop lookup; play is one click.

The seek is applied once, latched by debate id. These recordings are MediaRecorder WebM with one cluster and no Cues, so every seek is a parse walk (GEO-2828). Re-applying on any later render would fight the viewer's own scrubbing.

Response kind is passed down, not derived per row. EntityVoteButtons does an unscoped useQueryEntity per instance unless told; twenty extracted claims would be twenty reads. One batched useDebateClaims call answers for all of them.

Extracted claims render read-only when TranscriptClaim.spaceId is null. Pointing the vote at this page's space instead would record it where the claim does not live, which reads back as a claim nobody answered.

A restated claim gets no speaker and no jump. One claim entity linked from two turns cannot honestly answer "who" or "when" — the data model already declines, so the UI does too rather than guessing from the first block.

Corrected along the way

Two comments claimed published timecodes are null for "all of them bar the test debate". Measured against testnet on 2026-09-23: 921 of 1,072 statements carry one, and 23 of 25 debates are timed all the way through — which is what lets useClaimTimings skip the Whisper transcript fetch for almost every debate. Both comments are fixed.

Not in this PR

  • A fifth level of nesting. The feed draws four, counted from the claim: a debate, the claims it
    produced, the comments on one of those, and the replies to those (ACTIVITY_MAX_DEPTH). Deeper
    replies exist, are counted, and are reached through "Continue this thread", which opens that
    comment's own page with a fresh depth budget. This bullet used to say that comments left on a
    debate were deferred to phase 4; they are in, because the review asked for a comment box under
    every row in the feed rather than a side panel.
  • Server-side ranking for the thread — Best and Top ship here (Best is the default, and the sort reads Best / Top / New / Old), but both are a client-side score over the fetched window: debates have a server ranking and comments do not, and EntitiesOrderBy has no vote or score member. Ordering over complete data is part of GEO-3030.
  • Backfilling the 151 statements still missing offsets. The short-circuit is all-or-nothing, so one missing offset in a 13-claim debate still costs the whole transcript fetch.
  • Retiring WinnerShare in ClaimDebates now that winner voting is being sunset — the Crown badge in comments-section.tsx still reads tallyDebateVotes, so that wants its own change.

Testing

  • New tests across debate-timecode, claim-activity-order, batched-counts, comment-time,
    extracted-claim-row, debate-activity-row, the seek latch in debate-feed-player, and the
    comment-thread merge. Every fix from the four review rounds was proved by breaking it and
    confirming the test failed.
  • Full apps/web suite: 682 files / 8186 tests passing, clean. It has a pre-existing flaky tail under parallel load — two earlier runs of identical code failed different files, all passing in isolation and none touched here (GEO-3034) — so treat a single red run as suspect and re-run before chasing it., no regressions. (Run locally with the localStorage shim; without it ~10 debates suites fail to collect on main too.)
  • tsc --noEmit clean, eslint clean on changed paths, next build succeeds.

🤖 Generated with Claude Code

@vercel

vercel Bot commented Sep 23, 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 26, 2026 11:31pm UTC

Request Review

…hread (GEO-3008)

A claim page ended in a comment thread that listed only what people typed, with
the debates held on that claim parked in a gallery above it. Two places showing
the same argument, and neither of them showing the claims that came out of it —
so the thing a reader most wants to follow, what was actually said and by whom,
was the one thing the page never surfaced.

The comment thread becomes Activity: debates are ordered in among the comments
by when they happened, and expanding one lists the claims extracted from its
transcript — attributed to the debater who said them, in the order they were
said, each votable in place and each linking back to the moment on the video.

Ordering under a debate comes from `claim-timing.ts` and nowhere else. The
graph's own relation order looks chronological and is not: those relations are
published with `Position.generate()`, which is random rather than monotonic, so
reading them as a transcript would produce a stable, arbitrary, plausible-looking
sequence. Claims nothing could place sit in a tail after the timed ones rather
than being sorted among themselves.

A timing good enough to sort by is not always good enough to state. The timecode
chip and the jump link appear only above `isAssertableMoment`'s bar, so a block
fallback — which lands at the start of the turn, sometimes most of a minute early
— orders the row correctly and claims nothing about when it happened.

The debate row shows the ordinary entity upvote/downvote rather than a who-won
tally, and its thumbnail is the published keyframe rather than a mounted player:
three debate rows would otherwise be three players fetching and decoding for a
reader scrolling past all of them.

Also corrects two comments claiming published timecodes exist for "all of them
bar the test debate". Measured against testnet on 2026-09-23: 921 of 1,072
statements carry one, and 23 of 25 debates are timed all the way through — which
is what lets `useClaimTimings` skip the transcript fetch for almost every debate.
… Best

Six things from reviewing the feed against a real claim page.

Claims now load with the debate row instead of behind "Show claims from this
debate". What a debate produced is the reason it is in this feed at all, and a
row that only says a debate happened is the state the feed was built to replace.
The cost is smaller than it looked: the traversal is one request per debate, and
the timing resolver only reaches for the transcript on the two debates in the
corpus with an unpublished offset.

The speaker avatar was rendering at full column width. `Avatar` fills its
container whenever it has a real `avatarUrl` — `size` only sizes the generated
fallback — so the frame is the caller's job, as it is at the other 56 call sites.
It now takes the comment row's own `density.avatarPx`, and the rest of the row
takes the same density's type ramp, so a claim row and a comment row in one
thread sit on one ramp rather than two.

Extracted claims carry the side their speaker argued, through the same tag a
commenter's stance uses — extracted from `ClaimCommentPositionBadge` as
`ResponsePositionTag` so the two cannot drift. The facts differ (a commenter's
tag tracks where they stand now; a debater's is fixed by the debate) but to a
reader scanning the thread it is one question, and the answer should not change
shape per row.

The debate row shows what was argued, and both the text and the still open the
video. Without a body it was a byline and a control strip, which beside comments
that each say something read as a row that failed to load.

Debate dates now age like comments do — `getRelativeTime`, moved to its own
module so both use one implementation. "Sep 17, 2026" beside "1d ago" read as
two lists that happened to be adjacent.

And the thread can be ranked. Sort offers Best, Top, New, Old, defaulting to
Best on the claim page: this list is the record of an argument rather than a
running conversation. Best takes the net score, so a divisive row falls; Top
takes upvotes alone, so a row many people backed holds its place regardless of
pushback. Scores are read from the cache each row's own vote control already
fills rather than fetched — ordering a list by score should not cost a round
trip per row — so an unranked row scores zero and recency breaks every tie.
…hread connectors

Three from the second pass over the feed.

The comment count on a debate read 0 on a debate that had comments. The button
seeds itself from a count its host supplies and then only ever corrects that seed
*downward*, from a list somebody has actually opened — so a host with nothing to
give passes zero and the number stays wrong until the panel is opened and the
comments are found. Wrong in the one direction that tells a reader not to look.
`useEntityCommentCounts` asks the server for the counts of many entities at once,
through the same backlink aggregate the explore cards already use for their own
pill, so a debate row and every claim under it get a real number for one request.

Extracted claims are no longer paged. "Show 17 more claims" asked a reader who
had scrolled to a debate to ask a second time for the thing the debate produced,
which is the reason the debate is in this feed at all. The whole branch collapses
in one press instead, which is the control that actually shortens the page.

And a debate connects to its claims the way a comment connects to its replies.
The spine, the elbow into the last row and the arm into the rest are now shared
components over the geometry `comment-density.ts` already owned; `CommentList`
draws its spine from the same one, and the segment classes moved beside the
geometry so a hover cannot light one branch and not the other. Two drawings of
"this row belongs to that one" would have said they were different relationships.
…s replies

The connectors were a half-copy: arms floating at the wrong height, no spine
between them, and none of the ⊖ controls that make a comment thread collapsible.
They now come from one implementation rather than two that resemble each other.

`thread-branch.tsx` owns the parent spine, the list spine, the arm, the elbow and
the collapse toggle, and `CommentList` and `CommentItem` draw theirs from it —
so a change to how a branch looks cannot land on one surface and miss the other.
Each piece takes pixels rather than a `CommentDensity`, because the two ends of a
connector can belong to rows of different sizes: a claim hanging off a debate
reaches back to a 44px keyframe column and lands on a 32px avatar, where a reply
does both at 32. A single density would have quietly assumed those were one
number, which is the class of bug the geometry helpers exist to prevent.

A debate row is now laid out as what it is — a row with a branch of rows that
belong to it. The spine descends from the thumbnail, the ⊖ threaded onto it in
the action row closes the whole branch, and ⊕ in the thumbnail's place reopens it.

And a debate's own comments load into that branch, after the claims. They never
appeared before because they reply to the debate, not to the claim, so the claim's
backlinks do not know about them: the reader saw a debate, the claims it produced,
and no sign of the conversation happening on it one click away. Claims first,
because they are what the debate is and the comments are a reaction to them. The
fetch is gated on the count the feed already has, so debates with no comments —
most of them — cost nothing to rule out.
Someone who comments under a debate and holds a position on the claim it argued
is making the same commitment as someone who comments on the claim directly, so
the thread now says so in both places rather than only where the comment happened
to be filed. Same badge, same provider — the context already wrapped the whole
thread, the row just was not asking for it.

The debate row leads with the claim and follows with the debaters, the other way
round from the first cut. A comment row's strong first line is what was said and
its byline is who said it; a debate row reads as a peer of those rows only if it
is built the same way. The byline no longer falls back to `debate.name` when no
debater profile has resolved: published debates are named "<claim> | <A> vs. <B>"
since #2554, so that fallback would have printed the claim underneath itself.

And the page composer says "Join the conversation..." like the panel's, rather
than "Start the discussion..." — which is the wrong offer on a thread that already
holds debates and the claims pulled out of them.
…e agree on

Depth goes to four: a debate, the claims it produced, the comments on one of
those, and the replies to those. Past that the row shows its count and a way
into the debate's own thread, which is where a conversation that deep already
lives. The plain comment thread is unchanged — it has always recursed as far as
it goes.

The Activity heading now counts everything the feed draws rather than only the
rows it can see: debates, the claims extracted from them, and every comment
anywhere in that tree. The claim's Explore card shows the same number, from the
same definition, so opening a card can no longer contradict what it promised.

Two measurements made that affordable rather than a per-card traversal:

Comment depth is free. A reply carries `Reply to` to every ancestor, not just
its parent, so one aggregate counts an entity's whole comment tree at any depth.

Extracted claims can be counted from the debate — they are published with
`Sources` pointing back at it, so it is one aggregate rather than the
Debate → Transcripts → Blocks → Claims walk used to read them. Identical totals
against testnet (177 across 30 debates), 0.52s against 3.48s. Added to the shared
explore card selection the whole thing costs +0.02s and +1 KB over a 20-card
page, because for anything that is not a claim those lookups return empty
immediately — so there is no claim-only selection to keep in step.

What the count means lives in `claim-activity-fields.ts`, deliberately not a
client module: the card's selection is assembled on the server, and a
`'use client'` import there becomes a client-reference proxy at build time, which
interpolated the fields away and broke the query. The build caught it; a parse
test now covers the assembled selection so the next one is caught in a suite.

Debates keep their Activity gallery as well as their place in the thread. The
gallery is the way through to the Debates tab — the complete, filterable index —
where the thread carries the recent ones in the order they happened.
Every arm in a debate's branch was drawn 18px below its row's avatar. The cause
is the distinction the module's own doc comment describes and its API then made
impossible to honour: a connector has two ends, and they belong to rows of
different sizes. A claim hanging off a debate reaches back across a 44px keyframe
column but lands on a 32px avatar. `ThreadBranch` took one `density` for both, so
the arm's height came out as half the *parent's* 67px keyframe — 33.5px — instead
of half the row's, 16px.

The prop is now `rowDensity`, which can only mean the rows in the branch, with
`reachPx` required from the parent for the horizontal end. Nothing can supply one
and silently get the other.

Measured in a browser rather than reasoned about, because reasoning about it is
what produced the bug: 17 branch rows, 0 misaligned, at 1280px and 480px. Each
arm's 1px line now sits at the avatar's vertical centre and stops at its left
edge, and each elbow's curve ends there.
…root

Two things a thread can say when it has more than it is drawing, and until now
we said both with one control: "N replies", which opened the global comments
panel. Reddit keeps them apart and it is worth copying, because they answer
different questions.

More of the same, here. Replies held back for length are already in the tree
`useComments` built, so revealing them costs nothing and happens in place —
`ThreadShowMore`, capped at three, a cap about legibility rather than fetching.

Deeper than this page goes. `ThreadContinue` navigates to the entity whose own
page roots that thread, rather than opening a panel. We are better placed for
this than Reddit is: Reddit has to synthesise a permalink for a comment, where
every node in our tree is already an entity with a page that renders its own
thread. A link rather than a button, so middle-click and open-in-new-tab work.

Half right for one case, and worth saying so: an extracted claim's page renders
its full thread, so continuing lands exactly where it should. A published
debate's page is the video feed, with the thread behind the comments button —
better than the old flat panel, because the debate's context survives, but not
the clean re-rooting. The fix is a deep link that opens the panel on arrival and
there is no such link today; GEO-3033 is now that piece and nothing else.

Depth is counted once, from the root. `claim-activity-depth.ts` owns the numbers
and `canNestBelow(depth)` asks the only question. The arithmetic was previously
spread across three files — `ACTIVITY_MAX_DEPTH - 2` twice, `maxDepth - 1`, and
a `depth < maxDepth` comparison — which was correct but had to be hand-verified
to stay that way, and a row now knows only its own depth.

`DebateCommentRow`'s `debateId` becomes `targetEntityId`, because under an
extracted claim it was being handed a claim id.
@jwalkingjew
jwalkingjew force-pushed the preston/geo-3008-claim-activity-feed branch from f5c2dad to 2e5d4e0 Compare September 24, 2026 22:59
…oth exist

GEO-3030 landed a single-entity comment count over entitiesConnection while this
branch was open. The two now look like duplicates and are not: one answers for a
page with one count, the other for a screen of rows in a single round trip, which
is what lets the activity feed put a count on every debate. Left unmarked, the
next person deletes one of them.
A nested backlinksList inside entities(filter: {id: {in: …}}) stops at ten rows
without erroring, and that is how both the batched count and the explore card
selection read. So the claim whose feed draws 52 rows printed 33, and every debate
with more than ten extracted claims was cut off at ten — the number was wrong
exactly on the claims busy enough for anyone to notice.

Take the extracted total from an aggregate instead of a list length, give every
nested list an explicit first, and dedupe debates by id: testnet has a debate
carrying two Claims relations to the same claim, which counted its subtree twice
while the feed drew it once. The two errors were cancelling unevenly.

The hook now interpolates the shared selection rather than restating it with
variables, which is how the two shapes could have drifted again.

Verified against the API on the 20 busiest claims: every total now matches what
the same fields read one claim at a time, in 0.57s and 28 KB.
Three things in the activity feed sent the reader somewhere else to finish a
thought, and the somewhere else was always the side panel — which replaces the
thread with a flat list of one entity's comments, losing the debate, the claim and
the position badges at exactly the moment the reader wanted to respond to them.

Comment and Reply on a debate row, an extracted claim and a comment now open a
composer under that row. The write is unchanged: it still lands on the entity the
row belongs to, with the full ancestor chain, so a reply made here and one made in
the panel are the same reply. A row that gated its thread on a server count also
lifts that gate once something has been posted into it, or the first comment on a
silent claim would be written and never drawn.

Names and faces open the person's profile beside the thread instead of navigating
to their space. They stay anchors, so middle-click and copy-link still work.

The responder faces open the list of who responded and which way. They were the
only thing in a vote row that looks like people and answered nothing, while the
claim cards in the debate side panel have opened that list from this same stack all
along. Gated on a non-zero total: the avatars component renders nothing when nobody
has responded but is still an element, so a truthiness check would have put an empty
clickable button on every unanswered claim in the app.

Rows say what they are. A claim with no assertable moment and a comment from someone
holding no position were indistinguishable — same face, name, sentence, vote and
count — because every difference between them is conditional. And the debate row
counts its claims beside its comments, claims first, in the order the branch draws
them.
Wrapping the avatar in a profile link made the link the row's flex item and left
the frame inside it display:inline, which ignores the inline width and height that
size it. The image inside is h-full w-full, so it resolved against nothing and drew
at its natural size — measured on the preview at 987px across, in a 32px row, on
all 30 rows of a claim page.

The link is a flex container now, so the frame is a flex item again and its
dimensions apply. The test asserts that structurally, since jsdom computes no
layout: it fails against the previous class list.
Making the profile link a flex container fixed the size but left it a stretched
flex item of the row, so the 32px frame centred against the row's full height and
the face dropped down beside the claim text instead of sitting on the name line.

Alignment comes from the caller now: the face asks for self-start, the name sits in
an already-centred header and asks for nothing. One wrapper cannot know which of
its two uses it is.
…irst

Two problems with one badge. It sat after the row's Claim/Comment tag, which put a
label about the row between a name and a fact about that name — the side belongs to
the person, so it goes first.

And it always reported the page's claim, wherever the comment was. A comment on an
extracted claim sits directly under that claim's sentence, so a bare "Agree" beside
the author reads as agreement with it. Observed on testnet: a comment arguing
against an extracted claim, badged Agree, because its author agrees with the page
claim the debate was about. Both facts were true and the badge showed the one the
reader could not see.

The rule is now one sentence: the nearest claim above the comment. Nesting the
existing provider under an extracted claim is the whole change — the badge already
reads whichever is closest, so a comment under a debate, or at the top of the
thread, still reports the page claim. Nothing is borrowed from further away when
the author holds no position on the nearer claim; an absent badge says less than a
wrong one.

The badge also names what it is about on hover, since the claim it reports can be
several screens up by the time the reader reaches it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Activity totals, optimistic comment ordering, and repeated same-debate timecode navigation have unresolved correctness issues.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (3)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Sort optimistic debate comments newest-first

apps/​web/​core/​claims/​browse/​debate-activity-row.tsx:440

A newly posted debate comment is appended to the shared comments cache by useCreateComment, while fetched rows arrive newest-first. Mapping the array directly therefore renders the newest optimistic comment after all existing comments until the indexing poll replaces the list, contradicting this branch's newest-first ordering. Sort these root rows by createdAt before rendering them.

Medium severity Sort optimistic comments newest-first

apps/​web/​core/​claims/​browse/​extracted-claim-row.tsx:341

Inline publishing appends the optimistic comment to the end of the cache, so this direct mapping places a new comment below every existing root comment until the indexer responds. The fetched list is newest-first, so preserve that ordering for optimistic rows by sorting on createdAt before rendering.

Medium severity Latch debate seek requests by ID and timecode

apps/​web/​core/​debates/​browse/​debate-feed-player.tsx:162

The latch only records the debate ID. A client-side navigation or browser history change from ?t=120 to ?t=500 on the same debate updates initialSeekSeconds without remounting the keyed feed item, but this guard suppresses the newly requested seek. Latch the (debate.id, initialSeekSeconds) request instead, so rerenders do not re-seek while a distinct timecode navigation still works.

Comment thread apps/web/core/claims/browse/claim-page-view.tsx
…ounted

Four findings. Three are one class nobody had named yet: the cache appends, the
query orders newest-first, and every branch in this feed rendered the cache order
straight — so the one row that is certainly the newest, the comment the reader had
just written, was drawn last, under comments from weeks earlier, until the indexer
replaced the list. `orderNewestFirst` is a no-op on a settled list and a correction
for exactly that row, applied at all three sites: a debate's comments, a claim's
replies, and the replies under a debate comment — the third being one the review did
not flag but which is the same list from the same cache, sorted before the slice so
"show more" reveals the next-newest.

The fourth is last round's hook, one level out. `useAdjustActivityTotal` corrects a
cached answer, so it does nothing when that answer was never fetched — and the hero
that holds it renders on every tab while the heading, the aggregate's only other
caller, lives inside the Overview. An explanation published from the Debates tab
therefore moved nothing. The hook subscribes to the aggregate itself now, so a caller
cannot hold the adjuster without the baseline being on its way. It costs one batched
request on tabs that do not display the number; the Overview is the default tab, so
nearly every visit was fetching it anyway.

And the timecode latch recorded only the debate id, so following a second extracted
claim's timecode in the same debate — a client-side navigation that does not remount
the card — was swallowed. It latches the debate and the moment.

The test for the aggregate mount was vacuous when written: it set a tab mock the page
does not read, so the Overview rendered anyway and the assertion held with the fix
removed. The active tab comes from the route, and it now sets that.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jwalkingjew

Copy link
Copy Markdown
Collaborator Author

Round 11 — three suppressed findings, and a class nobody had named

Two of the three were the same thing, and looking for the rest of it found a third site the review did not flag.

Sort optimistic debate comments newest-first · Sort optimistic comments newest-first

Both correct, and I checked the premise rather than taking it: comment-entities-connection asks for [CREATED_AT_DESC, ID_ASC], and useCreateComment does setQueryData(old => [...old, optimistic]). So the list is newest-first and the reader's own comment is appended to the end of it — the one row that is certainly the newest was the one drawn last, beneath comments from weeks earlier, until the indexer replaced the list.

Enumerated, and there is a third site. The same cache feeds the replies under a debate comment: buildCommentTree groups them out of that same newest-first list, and an optimistic reply is appended there too. debate-comment-row rendered them straight, and it also slices them for its "show more" — so the paging was over the cache's order as well. All three now go through orderNewestFirst, and the slice happens after the sort.

It is one exported helper rather than three inline sorts, in claim-activity-order.ts beside the feed's other ordering, and it reuses compareByTime so an undated row goes to the tail rather than to 1970 — the rule the merged feed already follows. On a settled list it is a no-op; the only row it moves is the optimistic one.

Deliberately not the top-level thread's ordering, which has a sort control and pins the reader's own comments through sortWithSessionPinned. A branch has neither and newest-first is what it says it draws.

Tests: five on the helper in claim-activity-order.test.ts — the appended row lifts to the front, a settled list is untouched, ties keep their id order, the input is not mutated, an undated row goes last — plus a DOM test in debate-activity-row.test.tsx that renders a three-week-old comment and an appended new one and asserts the new one is drawn first. Making the helper the identity fails two of the five; rendering the cache order fails the DOM one.

Latch debate seek requests by ID and timecode

Correct, and it is a gap in my own round-6 tests: I covered "fires once across re-renders" and "fires again for a different debate" and never the case in between. Two claims extracted from the same debate carry two timecodes, and following one after the other is a client-side navigation — initialSeekSeconds changes, the keyed card does not remount, and the id-only latch swallowed the second seek entirely.

It latches ${debate.id}@${initialSeekSeconds} now. The three guarantees that latch exists for all still hold, and the suite says so: it waits for ready, it does not re-seek on a re-render or on scrolling away and back, and it fires for a different debate. Two new tests cover the new one — a different moment in the same debate seeks again, the same moment asked twice does not. Reverting to the id alone fails the first.

State of the PR

tsc clean · eslint clean · 675 files / 8102 tests passing, clean run · on current master, no conflicts · 97c8009b4.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Comment-count cache synchronization, failure balancing, and top-level comment error handling can currently produce hidden comments or incorrect activity totals.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity CommentSection silently ignores comment query failures

apps/​web/​core/​claims/​browse/​claim-page-view.tsx:609

The new Activity view handles failures for debates and the aggregate, but CommentSection still discards the top-level useComments error. If that request fails, the heading can count claim comments while the list silently renders none and offers no retry. Surface the comment query error/refetch alongside the other Activity failures.

Medium severity Activity branch omits comments due to stale aggregate count

apps/​web/​core/​comments/​use-entity-comment-counts.ts:89

This aggregate can stay at 0 for 60 seconds after a comment is created on another surface, because useCreateComment updates ['comments', entityId] but never this query. The activity rows use this value to decide whether to mount the debate/extracted-claim branch, while EntityCommentsButton independently observes the live comments cache. After posting on an entity and navigating back, the button can show 1 while the branch remains absent, making the existing comment unreachable in Activity. Synchronize these count entries on publish, or include the live comments cache in the branch gate.

Medium severity Failed comment creation decrements count without optimistic insertion

apps/​web/​core/​hooks/​use-publish-comment.ts:79

createComment can return null before inserting an optimistic row (for example when readCachedSmartAccount finds no account), but this unconditionally invokes onFailed. Callers use onOptimistic/onFailed as a balanced +1/-1, so this path decrements the Activity total even though it was never incremented. Track whether the optimistic callback actually ran and only report a failure for a counted row.

#2581 (the sticky entity header) and this branch had both rebuilt the responder
faces beside a claim's tally into a trigger for the responder list, in
`EntityVoteButtons`, and both had closed the invisible-tab-stop case where the
faces render nothing. Five hunks, one component written two ways.

Took master's throughout, and it supersedes this branch's version rather than
merely winning the conflict:

- It gates the faces' trigger on the served `totalResponders`, not the
  optimistic `effectiveTotal`. The list it opens reads the served responders,
  so on the optimistic count a viewer's first vote made their own face open a
  popover saying nobody had responded. This branch gated on the optimistic count.
- Its `RespondersPopover` owns its open state and reads the slide-up container
  itself, where this branch threaded both in from the caller.
- It covers "the count says five but the list has not answered" with
  `empty:hidden`, which this branch had rejected on the grounds that the button
  would stay in the tab order. It would not: `display: none` removes an element
  from both the tab order and the accessibility tree. That objection was wrong.

So this branch's `wrap` prop on `ClaimResponderAvatars`, which handed the trigger
inward to solve the same problem, has no caller left and goes, with its tests.
Both files are master's.
…ry rollback

Three findings.

A comment published on a debate or an extracted claim was out of reach after the
reader navigated away and back. The branch gates read the batched comment-count
aggregate, which is held for a minute and never written on publish, plus a flag the
row's own composer set — and that flag reset on the remount while the aggregate
still said none. The comment button beside the row reads the live list, so it
reported the comment while the branch that would draw it was absent.

All three gates — the debate's branch, the debate's comments query, the extracted
claim's replies — read `useCommentCount` now, the hook the button already uses,
with the same arguments. The button and the gate cannot disagree because they are
one derivation. It survives a remount because its seeds are module-level by design,
sees a comment from any surface because every composer writes the same cache, and
drops a rejected comment on its own because `useCreateComment` removes the row. So
the composer's posted-count state had no reader left, and it goes; what remains is
whether the box is open.

A publish that failed before inserting anything — `createComment` giving up with no
cached account — still reported a failure, so callers treating the callbacks as a
balanced +1/-1 took back a count never taken. Last round's flag covered only the
queued retries; it covers the whole lifecycle now, first attempt included.

And the claim's own comments could fail silently under an Activity heading that is
an independent aggregate and kept counting them. `CommentSection` keeps that error
to itself, which is right where its heading counts the same list. The page reads it
too — same key, so no request — and says so beside the other two Activity reads.
@jwalkingjew

Copy link
Copy Markdown
Collaborator Author

Round 12 — three suppressed findings, a merge, and a correction

A correction first

In round 6 I rejected an :empty rule for the empty responder-faces trigger, saying it "hides the symptom in CSS while the button stays in the accessibility tree and the tab order." That was wrong. Tailwind's empty:hidden is display: none, and display: none removes an element from both the tab order and the accessibility tree. #2581 uses exactly that, and it is correct. I am saying so here because this PR's history would otherwise record a false claim about code that is now on master.

The merge

#2581 (the sticky entity header) and this branch had each rebuilt the responder faces beside a claim's tally into a trigger for the responder list, and each had closed the empty-trigger case. Five hunks in EntityVoteButtons, one component written two ways. I took master's throughout, and it supersedes this branch's version rather than merely winning the conflict: it gates the trigger on the served totalResponders where this branch used the optimistic effectiveTotal — so here a viewer's first vote made their own face open a popover saying nobody had responded — and its RespondersPopover owns its open state and the slide-up container rather than having them threaded in. This branch's wrap prop on ClaimResponderAvatars, which solved the same problem another way, has no caller left and is gone with its tests. Both files are master's.

Activity branch omits comments due to stale aggregate count

Correct, and the fix is to stop having two answers to one question. Every branch gate read the batched comment-count aggregate — held for a minute, never written on publish — plus a flag the row's own composer set. The flag reset on remount; the aggregate did not. So after posting and navigating back, the comment button beside the row (reading the live list) said 1 while the branch that would draw that comment was absent.

Of your two options I took the second, and went further than the gate: all three gates now read useCommentCount — the hook the button already uses, with the same arguments. The debate's branch, the debate's comments query, and the extracted claim's replies. The button and the gate cannot disagree now because they are one derivation, not two kept in step.

I checked that this actually survives the remount before relying on it, because useCommentCount returns the server count and ignores the cache whenever the cache predates its seed. Its seeds are deliberately module-level — "a component that remounts would otherwise mint a fresh timestamp for a count it has had all along" — so on navigate-back the seed is the original one and the comment written after it counts.

That left the composer's posted-count state with no reader, so it is gone. It could only ever see comments written from that row since that mount; the live list sees every surface, survives remounts, and drops a rejected comment without being told.

I did not synchronise the aggregate on publish, the other option. It would be a second correction on a second cache for a fact the comment list already holds — the shape that produced three defects earlier in this series.

Tests: a debate row and an extracted claim row each rendered with a stale aggregate of zero, a pending comment written to the cache, then unmounted and remounted — the branch is reachable. Gating on the aggregate alone, which is what the reset flag left, fails both.

Failed comment creation decrements count without optimistic insertion

Correct, and it is last round's fix applied to half the lifecycle. The countedByCaller flag guarded only the queued retries; the first attempt still reported a failure unconditionally, so a publish that gave up before inserting anything — no cached account — took back a count never taken. One flag now for the whole lifecycle, and onFailed fires only for a row onOptimistic actually reported.

Two existing tests encoded the bug: one asserted a failure was reported for a publish whose stand-in never called onOptimistic. The stand-in now behaves like useCreateComment — it reports the row it inserts — and the suite has both halves: a failure after counting reports, a failure before counting does not.

CommentSection silently ignores comment query failures

Correct for this page. CommentSection keeps its list's error to itself, which is reasonable where its heading counts that same list — there a failure reads as "Comments (0)" over nothing, consistent if unhelpful. On the claim page the heading is an independent aggregate that goes on counting the comments the list could not load. The page reads the same query — same key, so a second subscription rather than a request — and says so beside the other two Activity reads.

I did not change CommentSection itself, which would add a retry to every comment surface in the app. Worth doing, but a separate PR.

State of the PR

tsc clean · eslint clean · 681 files / 8177 tests passing, clean run · merged with current master · 7d4dcd056.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Retry recovery can desynchronize totals from visible comments, debate ordering can use mutable timestamps, and activity counts are mislabeled for assistive technology.

Review effort: Balanced
Findings: 2 Medium severity · 1 Low severity

Open (3)

Comment thread apps/web/core/claims/browse/use-claim-activity-rows.tsx Outdated
Comment thread apps/web/core/hooks/use-publish-comment.ts Outdated
Comment thread apps/web/partials/comments/entity-comments-button.tsx Outdated
…t succeeds

Three findings.

Round 10 re-counted a comment when a retry succeeded, on the assumption that the
retry had put its row back. It had not: with an existing id, `useCreateComment` only
mapped over the cache, so a row an earlier failure filtered out stayed gone and the
Activity heading counted a comment the thread was not drawing. Not re-counting was
checked and rejected — it converges to the opposite disagreement once the indexer
catches up. The counting was never wrong; its assumption about the row was.

So the assumption is made true. `useCreateComment` inserts the optimistic row
whenever it is not in the cache, and reports it through `onOptimistic` as a first
attempt does; the retry passes the same counter. Every insertion is counted and
every removal given back, on the row's own events. A retry whose row survived
finds it cached and reports nothing. This touches a hook every comment surface
shares, but only on the retry-after-failure path: the queued publish is the only
caller that passes an id.

The feed ordered debates by `debateDate`, which falls back to `updatedAt` — under a
comment saying it ordered by `createdAt` precisely to avoid that. Ordering reads
`debateCreatedDate` now, `createdAt` alone; display keeps the fallback.

And the comments button announced an activity total as "Comments (N)". It reads
its label off `commentsInCount`, the prop that already says the count is broader
than comments, rather than a second prop a caller would have to keep in step.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Eager per-author and per-speaker space lookups introduce avoidable N+1 requests across expanded activity branches.

Review effort: Balanced
Findings: None

Resolved since last review (3)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Avoid per-comment space queries when rendering debate threads

apps/​web/​core/​claims/​browse/​debate-comment-row.tsx:81

Each rendered debate comment calls useOpenDebaterProfile, which eagerly calls useSpace and starts a separate getSpace query. Because expanded debate branches render all visible comments and replies, a thread with N distinct authors now adds N profile-space requests before anyone clicks an author link. Batch those author space IDs at the branch level (the existing useSpacesByIds hook already does this) or make profile resolution lazy and pass the resolved target into each row.

Medium severity Batch debater space queries for extracted-claim rows

apps/​web/​core/​claims/​browse/​extracted-claim-row.tsx:114

This hook eagerly resolves the speaker's space via useSpace; mounting the extracted-claim rows therefore issues one getSpace query per distinct debater, despite the parent already batching the same participant IDs for profiles. With ten debates this can add roughly twenty independent requests on initial expansion. Batch the spaces alongside useProfilesBySpaceIds, or make useOpenDebaterProfile resolve only after activation.

Low severity Remove obsolete veracity control behavior from claim row contract

apps/​web/​core/​claims/​browse/​extracted-claim-row.tsx:52

This description still says claim rows can render the retired veracity/chevron control, but ResponseKind is now only curation | stance, and this caller always passes stance. Keeping the obsolete behavior in the new component's contract makes future maintenance misleading.

…ists

#2541 answered every claim with Agree/Disagree and removed the `veracity`
response kind. Three comments this branch added still described the two
vocabularies as current: the extracted claim row's contract said the vote control
picks chevrons for veracity, and two were orphaned by the `responseVocabulary`
removal during the merge that brought #2541 in — one a pair of lines left above an
unrelated call in the claim page, the other a doc comment attached to no prop at
all.

The historical notes that say the kind was removed, and why, are accurate and stay.
Two more stale references are master's own lines rather than this branch's, and
are reported rather than changed.
@jwalkingjew

Copy link
Copy Markdown
Collaborator Author

Round 14 — three suppressed findings, no open ones

The first round since round 4 with nothing on the comment-count or publish path. Two findings are about request fan-out and one is a stale doc.

Remove obsolete veracity control behavior from claim row contract

Correct. Fixed in 9f8e4ddee, together with the rest of the same class in this branch's own lines. #2541 removed the veracity kind, and three comments added here still described it as current: the flagged contract, which said the vote control picks chevrons for veracity; two lines left orphaned above an unrelated call in claim-page-view.tsx when the merge removed responseVocabulary; and a doc comment in extracted-claim-row.tsx left attached to no prop at all.

The historical notes saying the kind was removed, and why, are accurate and stay. There are two more stale references, in claim-comment-position.tsx:37 and claim-explore-feed-card.tsx:37, but blame puts both on master, so they are master's cleanup rather than this branch's and I have not touched them.

This is a comments-only change, so there is no behaviour to break-test. The proof is the sweep: every remaining veracity reference in this branch's added lines is a historical note.

Avoid per-comment space queries · Batch debater space queries

Both are right about the problem, and I measured it rather than assuming. On the deployed preview, Space requests are 6 of 116 on the rich claim and 9 of 130 on the Voter ID claim. So it is real, and it is around 5–7% of the page. useSpace is one uncoalesced ['space', id] query per id, and useSpacesByIds caches under a different key, so batching through it does not satisfy those lookups.

I am not fixing this in this PR. Each of the fixes available here is worse than the 6–9 requests it saves:

  • Batching at the branch, your first suggestion, works for speakers, whose ids are known up front. Comment authors are discovered as each nested list loads. Collecting them needs one of two things: a batch that waits before any row fetches, which is a fragile "wait for the batch or fetch your own" rule inside a shared hook; or a space map threaded through the recursive comment rows. Both would land in exactly the components that have produced a defect in almost every round of this review.
  • Lazy resolution, your second suggestion, delays the side panel by a round trip on the first click of each person's name. That is a change to a flow that was tested on a device.
  • Profile.id from the batch the feed already runs cannot stand in for the lookup. It falls back to the space id, which is the system entity the hook refuses to open.

The right fix is in the hook, not in this feed. Give useSpace the coalescing loadProfileBySpaceId already has: concurrent lookups in one tick go out as a single getSpaces request, and each id keeps its own cache entry. That is one change in one place, it needs no threading and no waiting rule, and it fixes the same N+1 on every surface that looks up spaces per row, not just this one. It is app-wide, so it should be its own PR.

A larger instance of the same class, not flagged

The same measurement turned up something ten times bigger. EntityResponseCounts plus EntityResponders account for 66 of the rich claim's 116 requests and 82 of the Voter ID claim's 130. Every vote control on every extracted claim fetches its own counts and its own responders. The repo already has the fix for this: useClaimResponseSummaryBatch with ClaimResponseBatchBoundary, which is how the matchmaking claim cards load a list of claims' responses in one batch. Wrapping each debate branch's extracted claims in it would replace roughly 70 requests with a few.

This PR's rows cause it, so it is in scope, but it changes how the vote controls and responder list on extracted claims load. That was tested on a device, so I have raised it with the author rather than changing it in this round.

State of the PR

tsc clean · eslint clean · touched suites passing · current with master, no conflicts · 9f8e4ddee.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Revisiting the same timed debate URL after removing its time parameter fails to reapply the requested seek.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Clear timed URL latch when no timecode is requested

apps/​web/​core/​debates/​browse/​debate-feed-player.tsx:169

A timed URL is ignored when it is revisited after the parameter was removed. The ref retains debate@seconds while initialSeekSeconds is null, so a client-side ?t=124 → no t → ?t=124 navigation (including browser Back) matches the old request and does not seek, even though this is a new navigation asking for that moment. Clear the latch when no timecode is requested; ordinary rerenders with the same non-null value will still remain latched.

The seek latch kept the last request after the timecode was removed, because a
missing timecode returned early without clearing it. So `?t=124`, then no `t`, then
`?t=124` again — which browser Back produces — matched the stale latch and never
seeked, though it was a new navigation asking for that moment.

A missing timecode clears the latch now. A render that is merely not ready keeps
it, so the same request still cannot fire twice across a buffering stall.
@jwalkingjew

Copy link
Copy Markdown
Collaborator Author

Round 15 — one suppressed finding, no open ones

Clear timed URL latch when no timecode is requested

Correct, and it is the remaining gap in the latch I widened in round 11. That round covered a different moment in the same debate and the same moment asked twice; it did not cover a moment, then none, then the same moment again. A missing timecode returned early without clearing the latch, so the latch outlived the request that set it, and ?t=124 → no t → ?t=124 — what browser Back produces — matched it and never seeked.

Fixed in the way you suggested, with one distinction kept deliberately: only a missing timecode clears the latch. A render where the media is merely not ready still returns without touching it. Otherwise the same request would fire twice across a buffering stall, which is the thing the latch exists to prevent.

Tests in debate-feed-player.test.tsx, and I broke the fix both ways. Never clearing, the old behaviour, fails the new "gone and come back" test. Clearing on not-ready as well, the over-eager version of this fix, fails a second new test that stalls and recovers readiness without re-seeking. The harness used to treat a missing and a cleared timecode the same, so it now tells them apart.

State of the PR

tsc clean · eslint clean · 682 files / 8186 tests passing · next build exit 0 · current with master, no conflicts.

This branch was successfully deployed

1 active deployment
Preview — c8f17da9 Deployed Sep 26, 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.

2 participants