Skip to content

feat: sticky entity header with name, avatar and vote controls - #2581

Merged
jwalkingjew merged 20 commits into
masterfrom
preston/sticky-entity-header
Sep 26, 2026
Merged

jwalkingjew merged 20 commits into
masterfrom
preston/sticky-entity-header

Conversation

@jwalkingjew

@jwalkingjew jwalkingjew commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

What

A LinkedIn-style sticky bar for entity pages. Scroll past the entity's name and a bar docks under the navbar carrying:

  • the avatar or cover in a circle, if the entity has one
  • the name, clamped to one line
  • the entity's main interaction on the right — upvote/downvote for ordinary entities, agree/disagree for every claim (feat(claims): answer every claim with Agree/Disagree #2541 retired the separate verify/dispute kind for factual claims while this was open)

Entities with no name set get nothing.

How

  • EntityStickyHeader is mounted once in the (entity) layout, above every branch that draws an entity page — the generic page, claim, topic, profile, and all the type-owned record tabs (/claims, /sources, /debates, /comments, /activity…).
  • It portals into EntityStickyHeaderHost, a zero-height sticky top-11 slot registered by the app shell. The shell is the only place that can express "full width of the content column, docked under the navbar": the route renders inside main, which is width-capped and transform-animated, so neither position: fixed nor a full-bleed sticky behaves there. Zero height means appearing costs no layout shift. z-40 keeps it under the browse sidebar, whose collapse toggle overhangs this column.
  • The trigger is useScrolledPastElement. It watches for the route's title by selector rather than taking a ref, because four unrelated components draw that title and only one is mounted at a time. isIntersecting alone can't tell "scrolled off the top" from "not reached yet" — the sign of boundingClientRect.bottom separates them. A MutationObserver (coalesced to one lookup per frame) handles the title arriving after hydration and being swapped between tabs.
  • The row's width is measured, not hardcoded: useMirroredContentColumn reads the content box of whichever column the tracked title sits in. The pages this covers are 900 / 840 / 1142 wide with two different gutters, so a constant here would be one more opinion that drifts — and it would have drifted: feat(topics): bring the topic page up to entity-page parity #2580 moved the topic page from 720 to 900 while this was open, and nothing here needed changing. A new view with its own width is matched by carrying data-entity-page-content; anything untagged falls back to the generic 900.
  • Both anchors and the selector built from them live in entity-page-anchors.ts, following space-tabs-anchor.ts — one contract with two halves, named once so they can't drift. The title selector is scoped to <main> so side-panel headings, which are drawn by the same components but portal to document.body, can't be mistaken for the route's.

Also in this PR: responder avatars became a second popover trigger

This modifies EntityVoteButtons, not just the new bar. The responder faces beside a claim's tally were a plain <span> — they are pictures of the people the list names, so readers reach for them, but only the tally was wired to the popover. Both halves now open it, matching ClaimSideResponders, which already opens the same list from the same faces on the claim hero.

Because the component is shared, the faces are now clickable everywhere they appear — claim cards, Explore, the claim page header — not only in the sticky bar. A Popover.Root per trigger rather than one root with two, which Radix doesn't support. The faces stay a plain span until there is somebody served to list, gated on the same counts that disable the tally.

Also in this PR: compact on EntityVoteButtons

Another change to the shared control. Three of its states are sentences rather than controls, and each overflowed the 48px bar on a phone — measured at 390px, the confirmation notice alone pushed the row 94px past its width and gave the document a horizontal scrollbar. compact (used only by the bar):

  • drops "Publish changes before responding" and "Response unavailable" — they describe state, which the page also explains;
  • keeps the "Response submitted. Waiting for confirmation." live region, as sr-only. It announces an event the reader just caused from a control in the bar, and on a claim page it is the only announcement there is: ClaimPositionCommentControl exposes the same sentence only as a title.

Known and accepted: on generic, topic and profile pages the page's own EntityVoteButtons also announces, so the confirmation can be spoken twice. Deduplicating needs a cross-surface registry in a component that renders once per claim on a list; a duplicate announcement is the better failure than a missing one. That review thread is left open deliberately for your call.

Also in this PR: the thumbs behave like the claim pills while a vote confirms

A behaviour change to voting, wherever the thumbs appear — the sticky bar, entity and profile headers, and every other inline use. The claim pills learned this in #2587 / #2598 and the thumbs never did. For the confirming window the thumbs now ignore presses (a second press used to publish a retraction mid-confirmation), are aria-disabled without greying out, show a progress cursor, drop the hover step, and put the confirming copy in the tooltip.

The debate overlay draws the same control as a pill and is not changed here; it and the remaining differences between the thumbs and the pills are tracked in follow-up tickets.

Not covered

The entity side panel has its own scroll container and is out of scope — this is the full-page entity routes only.

Verification

Driven in a real browser on testnet data, not only in jsdom:

  • Alignment — row edges vs. the page's content edges, to the pixel: claim [420, 1220], topic [390, 1250] (900 column, after feat(topics): bring the topic page up to entity-page parity #2580), person [370, 1270] at 1440px; claim [237, 963], person [217, 983] at 1000px; claim [33, 357], person [17, 373] at 390px.
  • Responsive to width changes — mutating the column's max-width and padding at runtime, resizing the window across the column's cap (it moves without resizing — the case a ResizeObserver on the column alone would miss), and collapsing/expanding the sidebar: 9/9 still aligned.
  • Sidebar toggle — elementFromPoint returns the toggle and an un-forced click flips aria-expanded.
  • Responder faces — clicking the cluster in the bar opens the Agree/Disagree list.
  • Clamping — with the sidebar collapsed at 900px the row stays inside the bar: [24, 883] within [24, 900].

Tests

New: use-scrolled-past-element, use-mirrored-content-column, entity-page-anchors, entity-sticky-header, entity-sticky-header-host, entity-vote-buttons.responders. Full vitest run clean; tsc --noEmit and eslint clean.

🤖 Generated with Claude Code

Long entity pages lose their subject: by the time you are in the property
sheet, the comments or a claim's sources, nothing on screen says which
entity you are reading, and voting means scrolling back to the top.

Docks a bar under the navbar once the title scrolls away, carrying the
name, the avatar or cover if there is one, and the entity's own response
control — `EntityVoteButtons` unmodified, so a claim keeps agree/disagree
and everything else keeps upvote/downvote without a second place deciding
which is which.

Mounted once in the `(entity)` layout, above every branch that draws a
page, and portalled into a zero-height host in the app shell. The shell is
the only place that can express "full width of the content column, under
the navbar" — the route renders inside a width-capped, transform-animated
`main` where neither `fixed` nor a full-bleed `sticky` behaves — and the
host takes no height so appearing costs no layout shift.

The title is found by `data-entity-page-title`, not by a ref: four
unrelated components draw it (generic header, claim hero, topic hero,
profile layout) and only one is mounted at a time.
@vercel

vercel Bot commented Sep 25, 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 8:41pm UTC

Request Review

A topic draws its own `<h1>` in browse mode rather than going through
`EntityPageTitle`, so the bar had nothing to watch and never appeared —
verified in a browser against a real topic page, alongside the claim and
person routes, which were already right.
Two things the sticky bar sat on top of, or left inert.

The browse sidebar's collapse toggle hangs off the sidebar's right edge at
52px from the top, squarely inside the bar's band, and the `z-[60]` on the
button cannot lift it out of the sidebar's own `z-50` stacking context —
so the bar covered it and cut the circle in half. The bar drops to `z-40`:
matching 50 would not have been enough, since the host comes later in the
DOM and an equal layer still wins.

The responder faces beside a claim's tally were a plain span. They are
pictures of the people the list names, so they are what a reader reaches
for, but only the tally was ever wired to the popover — the cluster looked
like a control and did nothing. Both halves now open it, the way
`ClaimSideResponders` already opens the same list from the same faces on
the claim hero. A `Popover.Root` each rather than one root with two
triggers, which Radix does not support; two roots also behave when one is
already open, since the outside pointerdown dismisses it and the trigger
that was clicked opens its own. The faces stay a plain span until there is
somebody to list, so there is no invisible tab stop around nothing.
The responder faces move after the thumbs. Everywhere else they lead,
because they sit inside a card with the claim's text above them; in the bar
the name runs right up to the control, so faces between the two read as
part of the name rather than as part of the tally they belong to.

The collapsed sidebar keeps a vertical rail 24px into this column with
nothing holding the space, so a full-width bar ran its background and
bottom border out past the rail and cut the one line still saying where the
sidebar is. The bar now starts on the rail, which draws over it from z-50 —
so it reads as beginning the pixel after. Full width again below the mobile
breakpoint, where there is no sidebar and so no rail; an expanded sidebar
needs nothing either, having real width and its own border-r.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bar was 900 wide with a gutter of its own, which is the generic entity
page and nothing else: a claim's column is 840 at px-4/px-5, a topic's 720
at the same, and a page with a rail 1142. So on every page but one the name
and the controls sat a little inside or outside the text they belong to.

No number here would have fixed that — it would have been a fourth opinion
that drifts as views are added. The bar is portalled into the app shell, so
no CSS reaches it from the page either. It now measures the column the
title it is already tracking sits in, marked with one shared attribute, and
copies its content box: padding taken off, so the bar's text starts exactly
where the page's does rather than a gutter outside it. A view with a width
of its own is matched by carrying the attribute, with nothing to keep in
sync here; one that does not falls back to the generic width as before.

Measured in a browser at three widths: claim, topic and person pages line
up to the pixel on both edges, including a phone, where the bar follows the
page's own 17px inset.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The observer is what makes the bar follow a width rather than record one,
and nothing was asserting that it fires. Both halves are covered: the
column changing size, and the host moving under a column that has not — a
column at its max width does not resize when the window does, it only
moves, which an observer on the column alone would miss.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five defects and one piece of duplication, from a review of the branch.

The row's mirrored offset was unclamped. A collapsed sidebar insets the
host by the rail's 24px while the page's column still starts at the content
edge, so a viewport narrow enough for the column to reach that edge put the
offset below zero and painted the avatar and the name outside the bar's own
background, over the rail. The left edge is clamped and the right held, so
what gives is the one edge that cannot be honoured.

`useScrolledPastElement` did not reset when its selector changed. The next
entity's title is not in the document at that moment and `next === watched`
is true when both are null, so the early return left the previous entity's
answer standing and kept handing out its detached title to be measured.

`useMirroredContentColumn` passed a detached or hidden column on as
`{ width: 0 }` rather than as the absence it is, so the bar drew a blank
strip instead of falling back to its own width.

The responder faces became a trigger on the optimistic count while the list
they open reads the served responders, so a viewer's first vote made their
own face open a popover saying nobody had responded — beside a tally that
stayed disabled. Both now read the served counts.

The body `MutationObserver` re-queried the document on every batch on a
route that draws no title. The debates feed mutates continuously; lookups
are now coalesced to one a frame.

And the title attribute was spelled out by hand in four files while the
content one was a constant. Both now live in `entity-page-anchors`, beside
the selector built from them — `space-tabs-anchor`'s pattern, for its
reason: they are one contract with two halves and must not drift.

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

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

Side-panel headings can incorrectly trigger the global header, and the existing profile-layout test lacks the new component’s required providers or mock.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity · 1 Low severity

Open (5)
What changed in this PR

Adds a sticky entity identity and voting bar beneath the navbar.

Changes:

  • Adds title tracking, content-column measurement, and app-shell portal hosting.
  • Marks entity-page titles and content containers for alignment.
  • Makes responder avatars open the responder popover and adds tests.
File Description
apps/​web/​partials/​entity-page/​entity-vote-buttons.tsx Adds responder-avatar popover triggers.
apps/​web/​partials/​entity-page/​entity-vote-buttons.selected-state.test.tsx Makes vote-button selection robust.
apps/​web/​partials/​entity-page/​entity-vote-buttons.responders.test.tsx Tests responder triggers.
apps/​web/​partials/​entity-page/​entity-sticky-header.tsx Implements the sticky header.
apps/​web/​partials/​entity-page/​entity-sticky-header.test.tsx Tests header rendering and alignment.
apps/​web/​partials/​entity-page/​entity-sticky-header-host.tsx Adds the app-shell portal host.
apps/​web/​partials/​entity-page/​entity-sticky-header-host.test.tsx Tests host registration and layout.
apps/​web/​partials/​entity-page/​entity-page-title.tsx Adds title anchors.
apps/​web/​partials/​entity-page/​entity-page-content-container.tsx Marks measurable content columns.
apps/​web/​partials/​entity-page/​entity-page-anchors.ts Defines anchor attributes and selectors.
apps/​web/​partials/​entity-page/​editable-entity-header.tsx Marks editable entity headings.
apps/​web/​core/​topics/​browse/​topic-page-view.tsx Anchors topic titles and columns.
apps/​web/​core/​hooks/​use-scrolled-past-element.ts Tracks titles scrolled above the navbar.
apps/​web/​core/​hooks/​use-scrolled-past-element.test.tsx Tests scroll tracking.
apps/​web/​core/​hooks/​use-mirrored-content-column.ts Measures portalled-row alignment.
apps/​web/​core/​hooks/​use-mirrored-content-column.test.tsx Tests column measurements.
apps/​web/​core/​claims/​browse/​claim-page-view.tsx Anchors claim titles and columns.
apps/​web/​atoms/​index.ts Stores the sticky-header host.
apps/​web/​app/​space/​(entity)/​[id]/​[entityId]/​layout.tsx Mounts one header per entity route.
apps/​web/​app/​entry.tsx Registers the host in the app shell.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apps/web/app/space/(entity)/[id]/[entityId]/layout.tsx
Comment thread apps/web/core/claims/browse/claim-page-view.tsx
Comment thread apps/web/core/topics/browse/topic-page-view.tsx
Comment thread apps/web/partials/entity-page/editable-entity-header.tsx
Comment thread apps/web/partials/entity-page/entity-vote-buttons.tsx
… test

Two findings from the PR review.

The layout test was broken and I had not run it — `profile-layout.test.tsx`
renders the layout with no SyncEngine provider, so the header's
`useQueryEntity` threw before any assertion ran. Stubbed like every other
child of that layout, and its props are captured rather than discarded,
because that suite's subject is that the route's id reaches everything the
layout draws. The branch that draws no profile shell now also asserts the
bar is still there: it belongs to the route, not to the shell.

The title selector matched side-panel headings. It is value-scoped by
entity, and document order covers the case where the route draws a title of
its own — but a Debate route draws the live feed instead, so with the panel
open on that same entity its heading was the only match, and the panel
scrolls in a container of its own. Scoped to `<main>`, which holds the
routed page and nothing else; both of the panel's branches portal to
`document.body`. An allowlist rather than naming the panel, so the next
surface portalled out of the page is excluded by construction.

Not the fix the review proposed, which was a route-only flag threaded
through `EditableHeading`, `ClaimPageView` and `TopicPageView`: three props
to remember at every future call site, and no help for a view nobody has
written yet. It was also wrong that `PowerToolsScreen` is out of scope —
it routes under this layout, so its heading is that route's title.

The assertion that pinned the selector as a literal went stale against the
change, which is the same lesson as the anchors module: it now derives from
`entityPageTitleSelector`, and what that selector matches is pinned in
`entity-page-anchors.test.tsx` instead.

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

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

The extracted responder popover can retain a stale portal container and become unscrollable inside slide-up sheets.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity · 1 Low severity

Open (5)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Refresh popover container when opening from a sheet

apps/​web/​partials/​entity-page/​entity-vote-buttons.tsx:463

Moving the open state into this child breaks the non-subscribing container lookup above. EntityVoteButtons often renders in a SlideUp before that sheet registers its popover host; previously opening the tally updated parent state, reran store.get(...), and picked up the host. Now opening rerenders only RespondersPopover, so the captured container can remain null and the popover portals to body, outside the sheet’s RemoveScroll shard. Read the atom again in this child’s opening render (or subscribe to it) so responder lists inside sheets remain scrollable.

Splitting the tally and the responder faces into a `Popover.Root` each
moved the open state down into `RespondersPopover` and left the container
read behind in `EntityVoteButtons`.

That read is deliberately non-subscribing — a subscription would re-render
every claim on a list whenever any sheet opened — and what kept a bare read
current was stated in the comment beside it: `Popover.Portal` only mounts
when the popover opens, and opening renders the component holding `open`.
Moving that state moved the render with it, so the parent stopped
re-rendering on open and kept whatever container it captured when the row
first drew. A sheet registers its host after the rows inside it mount, so
that capture was null: the responder list portalled to `body`, outside the
sheet's `RemoveScroll` shard, where it can be seen and not scrolled.

The read now lives with the state it depends on. Not a subscription, which
would cost more than the original did — there are two of these per claim
now, not one.

Enumerated across the branch: this is the only non-reactive store read in
it, and the only state that moved. The three other `useState`s added are in
new hooks with no prior consumer whose render timing could have shifted.

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

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

Route transitions can briefly paint stale header state, and delayed-response messaging can overflow the mobile sticky bar.

Review effort: Balanced
Findings: 1 High severity · 3 Medium severity

Open (4)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Prevent stale scroll state from painting after selector changes

apps/​web/​core/​hooks/​use-scrolled-past-element.ts:46

This reset runs in a passive effect, so when the selector changes after client-side navigation the hook still returns the previous entity's scrolledPast: true and detached target for the intervening paint. If the next entity is already hydrated, its sticky bar can flash immediately even though its title has not been observed yet. Run the observer/reset in a layout effect (or key the returned state by selector) so stale state cannot be painted.

Medium severity Prevent submitted status from overflowing the mobile sticky bar

apps/​web/​partials/​entity-page/​entity-sticky-header.tsx:101

The reused control can render the long Response submitted. Waiting for confirmation. status, but this wrapper is shrink-0 inside a narrow fixed-height row. On mobile, the status plus buttons/avatars exceeds the measured column and makes the bar overflow (or grow beyond 48px), unlike the ordinary state covered by the alignment checks. Add a compact sticky-header presentation that omits or separately positions this status.

Two findings from the review, both in code it had already passed once.

The reset for a changed selector ran in a passive effect, which is a paint
too late: the render that first saw the new entity still handed back the
previous one's `scrolledPast` and its detached title, so navigating to an
already-hydrated entity painted its bar for a frame over a page whose title
had never been observed. Adjusted during render instead — React re-runs the
component and discards that pass without committing it, so the wrong value
is never emitted at all. `useLayoutEffect`, the suggested fix, would beat
the paint only by committing once and correcting itself.

The control's three prose states do not fit the bar. Measured at 390px with
the flag forced: the indexing notice pushed the row 94px past its own width
and ran 62px past the bar, giving the document a horizontal scrollbar. The
other two — "Publish changes before responding" and "Response unavailable"
— stand in for the control entirely and are the same class; the finding
named only the first. A `compact` flag drops all three.

Dropped rather than truncated, since a sentence cut to "Response s…" tells
nobody anything, and nothing is lost either way: the page's own copy of the
control is still mounted below, merely scrolled out of view, so it keeps the
text and the `aria-live` announcement. Confirmed in the browser — with the
notice forced on, one lives on the page and none in the bar.

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

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.

Comment thread apps/web/core/hooks/use-scrolled-past-element.ts Outdated
Comment thread apps/web/partials/entity-page/entity-vote-buttons.tsx
Comment thread apps/web/partials/entity-page/entity-vote-buttons.responders.test.tsx Outdated
Comment thread apps/web/partials/entity-page/entity-vote-buttons.tsx Outdated
`unobserve` stops future records; it does not purge ones already queued. So
a notification about the title a tab just replaced can land after the swap,
and taking the batch's last entry without asking what it describes let that
stale record overwrite the measurement taken for the new title — undoing,
for a frame, the very thing measuring at the swap was added to fix two
rounds ago. Entries are filtered by target first.

The `ResizeObserver` alongside it needed nothing: its callback re-reads the
current geometry and never touches entry data, so a stale record cannot
mislead it.

Also rewrites two comments that had gone actively dangerous. Both still
said `compact` drops all three prose states and that the page's copy
carries the announcement — the claim that turned out to be false for claim
pages, and the reason the indexing notice is now `sr-only` rather than
gone. Left as they were, they invited the next reader to remove the fix.
The contract now says which two are dropped and why, which one is not, and
what would happen if somebody "finished the job".

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

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

A queued callback from a retired intersection observer can restore stale sticky-header state after navigation.

Review effort: Balanced
Findings: 1 Medium severity

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

In code that hasn't changed since last review

Medium severity Ignore queued callbacks from retired IntersectionObservers

apps/​web/​core/​hooks/​use-scrolled-past-element.ts:91

Filtering by watched only rejects stale targets from the current observer. When selector, topOffset, or enabled changes, cleanup disconnects this observer, but already queued IntersectionObserver records can still invoke its callback; that retired closure still has the old watched, so the record passes this filter and can overwrite the render-time reset or the new observer’s measurement. Guard the callback with an effect-local disposed flag set by both cleanup paths, and add a regression test that invokes the first observer after rerendering with a new selector.

Low severity Update contract to document actual response controls

apps/​web/​partials/​entity-page/​entity-sticky-header.tsx:39

This contract is stale: the component is configured below with compact and trailing avatars, and the response model no longer has a factual/veracity kind. ResponseKind is only curation | stance, and resolveEntityResponseKind returns stance for every claim (core/responses/entity-response.ts:8,141-165), so factual claims show Agree/Disagree rather than Verify/Dispute. Document the behavior the delegated control can actually provide.

Low severity Remove the contradictory stale docblock

apps/​web/​partials/​entity-page/​entity-vote-buttons.selected-state.test.tsx:82

The old one-line docblock immediately above now falsely says the inline order is up, score, down, while this new explanation correctly notes that a leading responder trigger can precede the up button. Remove the stale docblock so the helper has one consistent contract.

A disconnected `IntersectionObserver` can still call back. `disconnect()`
empties `[[ObservationTargets]]`; the spec does not empty
`[[QueuedEntries]]`, and "notify intersection observers" invokes the
callback of any observer whose queue is non-empty. That closure keeps its
own `watched`, so last round's target filter waved the record through and
it landed on top of the render-time reset the new selector had just
performed. An effect-local `disposed` flag, set on both cleanup paths.

The other two observers need no equivalent, and the difference is in their
specs rather than in luck: `ResizeObserver.disconnect` clears
`activeTargets`, `MutationObserver.disconnect` empties the record queue.
This one is alone in leaving work behind.

The sticky header's contract promised verify/dispute for a factual claim.
That kind no longer exists — #2541 reduced `ResponseKind` to
`curation | stance` and every claim now answers Agree/Disagree. Restating
the delegate's behaviour is what let this rot, so the contract now says so
about itself, and documents the two things it does ask for: `compact`, and
the faces after the thumbs.

And `inlineButtons` carried two docblocks, the older still claiming the row
draws up, score, down — untrue since the responder faces became a trigger
ahead of the up arrow, which is what the newer one exists to explain.

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

Copy link
Copy Markdown
Collaborator Author

Three "Previously missed" findings from the latest review. These arrived in the review body rather than as inline threads, so there is nothing to resolve for them — replying here instead. All three were right; fixed in 912c674.

1. A retired IntersectionObserver can still call back

Last round's fix filtered entries by entry.target === watched. That only guards stale targets from the current observer. When the selector changes, cleanup disconnects the old one — but disconnect() empties [[ObservationTargets]] and the spec does not empty [[QueuedEntries]], and "notify intersection observers" invokes the callback of any observer whose queue is non-empty. The retired closure keeps its own watched, so the record sailed through the filter and landed on top of the render-time reset the new selector had just performed.

That is three consecutive rounds finding a distinct way for this hook to answer about the wrong element. An effect-local disposed flag, set on both cleanup paths, closes the remaining one.

Class check, on specs rather than luck. ResizeObserver.disconnect clears activeTargets; MutationObserver.disconnect empties the record queue. Both are explicitly drained; IntersectionObserver alone is not, so it alone needs the flag.

Proved by removing the guard: the new test fails; with it, it passes.

2. The sticky header's contract promised a response kind that no longer exists

It said "verify/dispute for a factual one". #2541 reduced ResponseKind to curation | stance and resolveEntityResponseKind now returns stance for every claim, so factual claims answer Agree/Disagree like any other. Restating a delegate's behaviour is exactly what let this rot, so the contract now says that about itself — and documents the two things it genuinely asks of the control (compact, and faces after the thumbs) rather than paraphrasing what the control decides.

3. Two stacked docblocks on inlineButtons

Mine. I added an explanation above the helper in an earlier round without deleting the one-liner already there, which still claimed the row draws up, score, down — untrue since the responder faces became a trigger ahead of the up arrow, which is the whole reason the newer comment exists. Removed.


Unrelated flake, reported not fixed. The full suite exited non-zero once on an unhandled setTimeout from use-debounced-value escaping teardown in core/hooks/use-search.analytics.test.tsx — all 7928 tests passed, but the stray timer set the exit code. It is not mine: the branch touches nothing search- or debounce-related, the file passes 3/3 in isolation, and two earlier full runs on this same branch were clean. Worth knowing it can redden CI at random; the fix is a clearTimeout on unmount in that hook, which belongs in its own change.

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

The documented factual-claim interaction conflicts with the implemented two-kind response model.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Align PR description with agree/disagree claim interactions

apps/​web/​partials/​entity-page/​entity-sticky-header.tsx:40

The PR description promises verify/dispute controls for factual claims, but this implementation explicitly delegates to the current two-kind model where every claim uses stance (agree/disagree) and the factual flag no longer selects a third interaction. Update the description to reflect agree/disagree for all claims, or restore verify/dispute if that remains a requirement.

@jwalkingjew

Copy link
Copy Markdown
Collaborator Author

"Previously missed" — Align PR description with agree/disagree claim interactions. Correct. Fixed in the description; no code change.

Last round I fixed this exact stale claim in the sticky header's code contract, and said the veracity promise was "not in the PR description" — I had grepped the tree and never read the body. It was on line 7.

Not restoring verify/dispute, as the finding offers as the alternative. #2541 retired that kind deliberately on master, and bringing it back is neither this PR's job nor consistent with master.

The same class, enumerated across the description rather than just line 7. The body had not been revised across eight rounds, and three more things in it had gone false the same way — behaviour changed underneath it by upstream or by later rounds:

  • It said topic pages are 720 wide. feat(topics): bring the topic page up to entity-page parity #2580 moved them to 900 in the merge. Now stated as history, which is also the best evidence for measuring rather than hard-coding a width.
  • It cited the topic alignment as [480, 1160] — the pre-merge 720 column. Replaced with the post-merge measurement, [390, 1250].
  • It never mentioned compact, a real behaviour change to the shared EntityVoteButtons — the same omission the very first review round caught for the responder faces. It now has its own section: which two prose states are dropped and why, why the confirmation stays as an sr-only live region, and the accepted double-announcement trade-off with a pointer to the thread left open for that call.

The two remaining mentions of "veracity" in files this PR touches are #2541's own past-tense notes, accurate and not mine — left alone.

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

Browser-specific sticky positioning, portal layering, observer behavior, and shared voting changes warrant final human verification.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

@jwalkingjew
jwalkingjew marked this pull request as ready for review September 26, 2026 19:54
The claim pills gained a progress cursor for the confirming window in
#2598 — tens of seconds in which the side is already drawn as taken and
nothing else says the press registered. The thumbs `EntityVoteButtons`
draws, which is what the sticky header votes with, had nothing: the sentence
that used to say so is `sr-only` in the bar, and #2595 removed its visible
equivalent from the pills for reading as an unsettled side.

Driven by the hook's own `isProcessingResponse` rather than a re-derived
predicate, and applied through one class value both thumbs share, so the two
cannot drift apart the way this control drifted from the pills.

Every surface that draws these thumbs gets it, not only the bar: "busy" is as
true on an entity header. `progress` rather than `wait`, because the thumbs
are still usable — a second press still goes through, which is a separate
difference from the pills and not changed here.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The pills learned how to behave in the confirming window in #2587 and
#2598, and nothing tied the thumbs to them, so the two drifted. The thumbs —
what the sticky header votes with, and every entity header besides — now
match all of it:

- presses are ignored. The held thumb is this client's guess until the write
  lands, and pressing a held thumb means "remove", so a second press or a
  double-click published a retraction mid-confirmation;
- `aria-disabled`, and a progress cursor on the pointer;
- no hover step, since nothing under the pointer is going to happen;
- the tooltip reads the confirming copy, as the pills' `actionTitle` does;
- full strength — `aria-disabled` rather than `disabled`, so the held side
  still reads as taken.

All of it lives in one `voteButtonProps` both thumbs spread, so the two
thumbs cannot drift from each other. Keeping the thumbs and the pills from
drifting again needs them to be one control, which is tracked separately.

Inline only. `DebateVotePill` shares the handlers, so the guard sits on the
inline buttons rather than in them; the debate overlay keeps its current
behaviour and goes with the unification.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jwalkingjew
jwalkingjew merged commit 0eb769c into master Sep 26, 2026
5 checks passed
@jwalkingjew
jwalkingjew deleted the preston/sticky-entity-header branch September 26, 2026 21:00
jwalkingjew added a commit that referenced this pull request Sep 26, 2026
#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.

This branch was successfully deployed

1 active deployment
Preview — 719c79cd 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