Repository navigation
feat(explore): head a debate card with the claim it argued - #2458
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code review pass — two findings, both addressedRan a high-effort review over the full diff. The core change held up; two real issues came out of it, both about navigation rather than correctness. 1. The card re-headed itself mid-scroll (fixed)A debate card painted headed by the claim, then swapped to the generic fallback — headed by the debate's own name, linked to the debate — once the viewport-gated geo-chat lookups came back saying the video was never processed. That's the exact swap this PR's rationale argues against; it had just moved from before the lookup to after it. Root cause was DRY: two spellings of one decision — Falls out of it: the heading's 2. The Debate entity page is no longer reachable from its own card — your callNow that the title points at the Claim, nothing on a watchable debate card resolves to Mitigated, not eliminated: Share → Copy link on the card yields exactly I've left it as-is because adding a new affordance is a design decision, not a review fix. Say the word if you want e.g. the timestamp or a "Watch full screen" chip pointing at the debate. Verified and not problems
Repo patterns / DRY
Verification
|
There was a problem hiding this comment.
🟡 Changes recommended
Claim extraction can mislabel non-debate or cross-space cards, and fallback behavior conflicts with the stated scope.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates Explore debate cards to lead with the debated claim and open that claim in the side panel.
Changes:
- Extracts debate claims from graph relations.
- Introduces a shared claim-aware card title.
- Adds title, navigation, fallback, and extraction tests.
File summaries
| File | Description |
|---|---|
| apps/web/partials/feed/entity-feed.test.tsx | Updates the feed fixture. |
| apps/web/partials/explore/explore-ranking-card-body.test.tsx | Updates the ranking fixture. |
| apps/web/partials/explore/explore-feed-card.tsx | Uses the shared card title. |
| apps/web/partials/explore/explore-feed-card.test.tsx | Tests fallback claim headings. |
| apps/web/partials/explore/explore-card-title.tsx | Implements claim-aware headings and targets. |
| apps/web/partials/explore/explore-card-title.test.tsx | Tests heading and navigation behavior. |
| apps/web/partials/explore/explore-card-entity-link.tsx | Clarifies debate panel exceptions. |
| apps/web/partials/explore/debate-explore-feed-card.tsx | Uses the shared title for debates. |
| apps/web/partials/explore/debate-explore-feed-card.test.tsx | Tests debate claim headings. |
| apps/web/partials/explore/claim-explore-feed-card.test.tsx | Updates the claim fixture. |
| apps/web/partials/blocks/table/use-block-explore-feed-item.ts | Extracts claims for block rows. |
| apps/web/core/explore/explore-card-selection.ts | Selects debate claim relations. |
| apps/web/core/explore/explore-card-item.ts | Adds claim extraction to feed items. |
| apps/web/core/explore/explore-card-item.test.ts | Tests relation extraction and scoping. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const claim = item.debateClaim; | ||
| if (!claim) return { text: item.title, target: item }; |
There was a problem hiding this comment.
Good catch — this was a real bug, and the guard is now in place (08e487c).
You're right that the relation is not debate-only: debate-publish-draft writes it from debateRef for the argued motion (:210) and from blockRef for every claim extracted out of a transcript block (:437). My comment on the extractor claimed "null on every non-debate" as an invariant and then keyed purely on the relation type, so it was a hope, not an invariant. A text block reaching a card — a data block's explore view renders whatever its query returns — would have been re-headed and re-linked to an extracted claim.
Fix: debateClaimFromRelations → debateClaimFromEntity(types, relations), gating on isDebateEntity(types). Taking the types as a parameter rather than gating at the reader is the point: a caller can't forget it, so the invariant holds at both producers by construction instead of at whichever consumer remembered.
Enumerated the class rather than patching the one site, as asked:
- Both producers —
buildExploreFeedRowsanduseBlockExploreFeedItem— now pass types. Worth notingbuildExploreFeedRowsderives a row'stypesfrom the Types relation in the display space, which is the same sourceExploreFeedCardroutes on, so what can carry a claim and what reaches the debate card can't disagree. debateVideoUrls, the neighbouring relation-type-keyed field — checked, needs no gate:DEBATE_VIDEOS_PROPERTY_IDis written only from the debate entity (debate-publish-draft.ts:277); the other references are property definitions in the space template, not data.getRecordingUrls— community-call recordings, unrelated property, unaffected.- Every other reader of
Claimsin the repo —claim-provenance(types: TEXT_BLOCK_TYPE_ID),claim-debatesandtopic-debates(bothtypes: DEBATE_TYPE_ID) already constrain the entity type in the query. So this reader was the odd one out, and the fix brings it in line with the established pattern rather than inventing one.
I did not add a second gate in exploreCardHeading. The rule now lives once, at the only construction site, enforced by the signature — a consumer-side copy would recreate the "two spellings of one decision" problem the previous commit existed to remove.
Tests added at both levels: debateClaimFromEntity returns null for a text block, for an untyped entity, and for undefined types, while still reading a claim off an entity typed debate and something else; and buildExploreFeedRows leaves debateClaim null on a block row that carries the relation.
`Claims` is not a debate-only relation. `debate-publish-draft` writes it twice: once from the debate, for the motion that was argued, and again from every transcript text block, for the claims extracted out of that block's speech. Keyed on the relation alone, any text block drawn as a card — a data block's explore view renders whatever its query returns — would be re-headed and re-linked to one of those extracted claims. `debateClaim` documented "null on every non-debate" as an invariant and then never enforced it. Gate on `isDebateEntity`, and take the types as a parameter so the gate cannot be skipped by a caller that forgets it — hence the rename to `debateClaimFromEntity`. This is what every other reader of the relation in the repo already does (`claim-provenance`, `claim-debates`, `topic-debates` all constrain the entity type in the query); this reader was the odd one out. Note the row's `types` come from the Types relation in the display space, the same source `ExploreFeedCard` routes on — so what can carry a claim and what reaches the debate card cannot disagree. The neighbouring `debateVideoUrls` needs no such gate: `Debate videos` is written only from the debate entity. Reported by Copilot on #2458.
Copilot review — all 3 comments triagedChecked for suppressed/low-confidence comments too: the review reports Comments generated: 3, all surfaced inline, no suppressed section. Replied on each thread.
The one real bug (#2)
Enumerated the class rather than patching the one site, per the ask:
Deliberately did not add a second gate in Verification152 files / 1991 tests green across |
There was a problem hiding this comment.
🟡 Changes recommended
Cold block renders can still change headings after hydration, and block type detection is not display-space scoped.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
`Claims` is not a debate-only relation. `debate-publish-draft` writes it twice: once from the debate, for the motion that was argued, and again from every transcript text block, for the claims extracted out of that block's speech. Keyed on the relation alone, any text block drawn as a card — a data block's explore view renders whatever its query returns — would be re-headed and re-linked to one of those extracted claims. `debateClaim` documented "null on every non-debate" as an invariant and then never enforced it. Gate on `isDebateEntity`, and take the types as a parameter so the gate cannot be skipped by a caller that forgets it — hence the rename to `debateClaimFromEntity`. This is what every other reader of the relation in the repo already does (`claim-provenance`, `claim-debates`, `topic-debates` all constrain the entity type in the query); this reader was the odd one out. Note the row's `types` come from the Types relation in the display space, the same source `ExploreFeedCard` routes on — so what can carry a claim and what reaches the debate card cannot disagree. The neighbouring `debateVideoUrls` needs no such gate: `Debate videos` is written only from the debate entity. Reported by Copilot on #2458.
…from `useEntityTypes` is unscoped in the data-block producer while the relations beside it are filtered to the row's space, so the guard added in 08e487c was answering about a different space than the relations it was guarding. An entity typed Debate in some other space could take a `Claims` relation written in this one — on a transcript text block that is an extracted claim, not a motion — and re-head the card with it. `buildExploreFeedRows` has the property for free, since it derives a row's types from the Types relation in the display space; this makes the second producer agree. Only the guard is scoped. `item.types` is what routes the card and has always been unscoped on this surface, so narrowing it would change which rows render as debate, claim or ranking cards — a data-block change, not this one. The two disagreeing is harmless: the card renders as a debate and falls back to the debate's own name, which is the documented behaviour. Reported by Copilot on #2458.
08e487c to
cf7f09d
Compare
Round 2: 2 new Copilot comments + the CI failureCI — pre-existing flake, fixed
So: flaky on master too, roughly 1 run in 4, and this branch drew a bad one. The speaker list is populated from Awaited those queries (0794a5c) → 5/5 clean runs of the full 181-test file. Also hardened the two other places that query a device radio synchronously right after opening the settings — same latent race, not yet observed. Flagging clearly because it is an unrelated drive-by fix in a file this PR otherwise doesn't touch. Also rebased onto current master, which had gained #2453. Comment 4 — guard scope mismatch: real, fixed (cf7f09d)
Narrowed one part of the suggestion deliberately: I scoped only the guard's types, not Comment 5 — cold-render re-heading: investigated, premise doesn't hold
Removing even that transition needs a tri-state The explore feed itself has zero exposure: Verification2360 tests / 144 files green across |
A Debate entity is named "<debater> vs. <debater> on <claim>", so the explore card led with the matchup and buried the motion at the end of a long line — the one thing a reader might have an opinion about. The full-screen /debates feed has always headed a debate with the claim instead; this brings the card into line with it, and points the title at the Claim entity rather than at the debate. Explore titles open the side panel, and a claim is exactly what the panel is for — unlike a debate, which is a full-screen video experience and so is exempted from the panel by GEO-2794. Typing the title's target as a Claim is what keeps that exemption off it. The claim comes off the entity's own Claims relation rather than out of geo-chat's `debate.claim`, which the card also loads: those lookups are viewport-gated and land long after the card paints, so a title read from them would show the debate's name and then swap under someone mid-scroll. Reading the relation costs one more relation type in the shared card selection and gives a title that is correct on first paint. A debate missing the relation still falls back to its own name, and to navigation, exactly as before.
Review found a debate card re-heading itself mid-scroll: it painted with the claim, then swapped to the generic fallback — headed by the debate's own name and linked to the debate — once the viewport-gated geo-chat lookups came back saying the video was never processed. That is exactly the swap reading the claim off the graph was meant to avoid; it just moved from "before the lookup" to "after it". The cause was two spellings of one decision: the debate card had its own heading logic, and the generic card had `CardTitle`. Extract `ExploreCardTitle` so both renditions share it and cannot disagree. Its own module because `explore-feed-card` is what draws the debate card, so importing the other way would close a cycle. Falls out of the extraction: the heading's `h2` class list stops being duplicated between the two files, and `DebateExploreFeedCard` loses a useMemo and an ontology import it no longer needs. Also refreshes the debate exception's comment in `ExploreCardEntityLink`, which described a rule keyed on the card rather than on what the heading opens.
`Claims` is not a debate-only relation. `debate-publish-draft` writes it twice: once from the debate, for the motion that was argued, and again from every transcript text block, for the claims extracted out of that block's speech. Keyed on the relation alone, any text block drawn as a card — a data block's explore view renders whatever its query returns — would be re-headed and re-linked to one of those extracted claims. `debateClaim` documented "null on every non-debate" as an invariant and then never enforced it. Gate on `isDebateEntity`, and take the types as a parameter so the gate cannot be skipped by a caller that forgets it — hence the rename to `debateClaimFromEntity`. This is what every other reader of the relation in the repo already does (`claim-provenance`, `claim-debates`, `topic-debates` all constrain the entity type in the query); this reader was the odd one out. Note the row's `types` come from the Types relation in the display space, the same source `ExploreFeedCard` routes on — so what can carry a claim and what reaches the debate card cannot disagree. The neighbouring `debateVideoUrls` needs no such gate: `Debate videos` is written only from the debate entity. Reported by Copilot on #2458.
…list Unrelated to this PR's change, but it is what turned CI red here. Measured at the merge-base as well: it fails roughly one run in four on master too, so it is a pre-existing flake this branch happened to draw. The speaker list is populated from `enumerateDevices`, and choosing a speaker re-renders it while that selection is still pending — so a device name can be briefly absent between two clicks. The test clicked two speaker radios back to back through synchronous `getByRole`, which is the only place in the file that does that, and it is the one that flakes: it found "Studio Speakers" and then failed to find "Display Speakers" in the same list. Awaiting the queries fixes it — 5/5 clean runs of the full file afterwards. Hardened the two other places that query a device radio synchronously straight after opening the settings, which is the same latent race, not yet observed.
…from `useEntityTypes` is unscoped in the data-block producer while the relations beside it are filtered to the row's space, so the guard added in 08e487c was answering about a different space than the relations it was guarding. An entity typed Debate in some other space could take a `Claims` relation written in this one — on a transcript text block that is an extracted claim, not a motion — and re-head the card with it. `buildExploreFeedRows` has the property for free, since it derives a row's types from the Types relation in the display space; this makes the second producer agree. Only the guard is scoped. `item.types` is what routes the card and has always been unscoped on this surface, so narrowing it would change which rows render as debate, claim or ranking cards — a data-block change, not this one. The two disagreeing is harmless: the card renders as a debate and falls back to the debate's own name, which is the documented behaviour. Reported by Copilot on #2458.
cf7f09d to
ed7bd5d
Compare
…wn tests (#2477) `debate-room-page-client.test.tsx` failed intermittently on CI — on master as well as on branches — with `Unable to find role="radio"`, naming a different device each time. It had cost several PRs a red Test job. The intro screen opens its own LiveKit connection, and `devicesLocked` — a participant's `ready_at`, or `roomState` being 'connecting'/'reconnecting' — force-closes any open settings menu. That is deliberate and has its own passing test. The bug was the race around it: these tests rendered and opened the menu immediately, so a connection landing mid-interaction shut the menu underneath them and every device radio disappeared. Measured rather than reasoned about, because two earlier readings of this failure were wrong: holding `roomConnect` pending across the interaction reproduces the exact error deterministically, and under CPU contention the file failed 1 run in 6 locally — the rate CI showed, and why it passed 12 out of 12 on an idle machine. An earlier attempt (#2458) read it as a re-render between two clicks and moved to `findByRole`. Waiting longer cannot help: the menu is closed and stays closed, which is why its own comment recorded it still failing "about one run in four". Every site that opens a device menu now goes through a helper that waits for the connection to have been attempted and for the trigger to be enabled — exactly `devicesLocked === false`. All 19, not the two that happened to fail. Verified under the contention that reproduced it: 10 runs, 10 passes. Tests only; no product code changed.
Master moved 60+ commits under this branch, four files conflicted, and one of those commits had already shipped half of what this branch was doing. **The claim title is master's now, not this branch's.** #2458 landed GEO-2879 as `ExploreCardTitle`, and its version is better: it reads the claim from the Debate's own `Claims` relation, which the explore feed resolves before the card paints, where this branch read geo-chat's `debate.claim` behind a viewport gate that lands after. So this branch's title swapped from the entity name to the claim under a reader mid-scroll -- the exact failure master's version exists to avoid -- and it only headed the video card, where master's also heads the non-watchable fallback. `DebateCardTitle` is deleted; the card calls `ExploreCardTitle`. What it keeps from here is the two-line clamp, because this card's height budget is calculated against a two-line title and a third line is 23px the viewport was not promised. That went to `ExploreCardTitle` as an optional `className` -- only the debate card passes it, since only the debate card sizes fixed-aspect media from what the title leaves over. **Claims and Share now stand down with the player, not just with the debate.** Master made the media window non-sticky so leaving it unmounts the videos and releases them (GEO-2963), and its test asserts the Claims control goes with them. Hoisting the interaction state to the card root for the shared bar had quietly undone that: the transcript-claims query would have stayed subscribed for every off-screen row. Both now gate on `mediaMounted` -- resolved *and* in the window -- while votes and the comment count, which ask geo-chat nothing, stay for the whole row's life. The rest of the resolution: - The bar takes master's `Warning` claims icon (#2462 unified it) with this branch's optional Claims/Share and `commentsPanelOpen`. - `DebateCardVideos` keeps master's `{debate, active}` signature and its `playbackAllowed` veto, wrapped in the `React.memo` the review added. - The feed takes master's `useLineClampOverflow` refactor alongside `JoinDebateButton`. - `DebateCardExtras` is gone -- the shared bar replaced the grey icon buttons it held, which is also why the `ExploreShareIcon` master still imported can stay deleted. - The test file is master's, with this branch's join-button and shared-bar cases re-applied on top; master's own `claim heading` block covers what this branch's title tests did. `@geogenesis/auth` needed rebuilding for master's new exports. Verified: full build green, 6786 tests across 561 files pass, `tsc --noEmit` now completely clean -- master's #2488 fixed the three test files that used to fail it.
What
On the explore feed, a debate card was titled with the Debate entity's name — which
debate-publish-draftgenerates as"<debater> vs. <debater> on <claim>". That leads with the matchup and buries the motion at the end of a long line.Now the card is headed by the claim, and the title opens that Claim in the side panel — the same thing the full-screen
/debatesinfinite-scroll feed does.Ada vs. Blaise on Fast fashion should be discouraged with higher taxationFast fashion should be discouraged with higher taxationHow
Explore titles already open the side panel, but
ExploreCardEntityLinkdeliberately exempts debates from it (GEO-2794) — a debate is a full-screen video the panel would serve badly. Typing the title's target as a Claim is what keeps that exemption off it, so the claim gets the panel and the exemption still holds wherever a title genuinely points at a debate.The claim is read from the Debate entity's own
Claimsrelation (ExploreFeedItem.debateClaim), not from geo-chat'sdebate.claimthat the card also loads. Those geo-chat lookups are viewport-gated and land long after the card paints, so a title read from them would render the debate's name and then swap under someone mid-scroll. The relation costs one extra relation type in the shared card selection and gives a title that is correct on first paint.Claimsis not a debate-only relation —debate-publish-draftalso writes it from each transcript text block, for the claims extracted out of that block's speech — sodebateClaimFromEntitytakes the entity's types and gates onisDebateEntity. That matches every other reader of the relation in the repo (claim-provenance,claim-debates,topic-debatesall constrain the entity type), and makesdebateClaim's "null on every non-debate" an invariant rather than a comment.Modified-click,
href, "copy link address" and the opener data attribute all behave as they do on every other explore title.One decision in one place
Both renditions of a debate card share
ExploreCardTitle:DebateExploreFeedCardwhile the debate can be watched, and the generic card it falls back to when it cannot.This matters beyond tidiness. The renditions swap after the card has painted — the geo-chat lookups behind the decision are viewport-gated — so per-rendition headings meant a card that painted with the claim and then re-headed itself to the debate's name mid-scroll, which is the exact failure reading the claim off the graph exists to prevent. Sharing the heading is what makes the two agree. It also stops the heading's
h2class list being duplicated between the two files.Scope note: this does mean the non-watchable fallback is now headed by the claim and opens the claim, where before it was headed by the debate and navigated to it. That is a deliberate change from the original plan, for the reason above.
Known trade-off — the Debate entity page
With the title pointing at the Claim, nothing on a watchable debate card resolves to
NavUtils.toEntity(spaceId, debateEntityId)any more: "View all" goes to the space's debates index, and vote / comments / claims are panel buttons. So cmd-click and "copy link address" on the title now get you the claim, not the debate.Mitigated rather than eliminated: Share → Copy link yields exactly the debate's URL, and the card already plays the debate inline with voting, claims and share. Left as-is because adding a new affordance is a design decision — say the word if you want something like a "Watch full screen" chip.
Testing
apps/web/core/explore/explore-card-item.test.ts(new) — relation extraction: id-spelling normalization, deleted relations, unnamed claims, display-space scoping, and the non-debate guard (a transcript text block carrying the relation gets no claim, at both function and row level).apps/web/partials/explore/explore-card-title.test.tsx(new) — the heading decision and every click path: claim vs. debate name, target, panel open, opener attribute, modified clicks, non-opted-in surfaces, and the debate-name fallback.apps/web/partials/explore/explore-feed-card.test.tsx— regression for the mid-scroll re-heading: the fallback is headed by the claim too.apps/web/partials/explore/debate-explore-feed-card.test.tsx— the card is wired to the shared heading, and stays headed correctly before any geo-chat request resolves.core/explore partials/explore partials/blocks partials/feed core/debates core/topics core/claims→ 152 files, 1991 tests.tsc --noEmitclean;bun run buildcompiles.