refactor(chat): give a row one render identity - #29607
Draft
chrisnojima wants to merge 3 commits into
Draft
Conversation
This was referenced Sep 9, 2026
chrisnojima
added this pull request to stack #29616
September 9, 2026 19:40
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Reactive username updates can again make row rendering and recycling-pool identities disagree, and the ordering regression test does not exercise its claimed scenario.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Centralizes chat-row header and recycling-pool identity to keep rendering decisions consistent.
Changes:
- Adds snapshot-memoized row identity derivation.
- Removes the shared mutable username-header context.
- Adds/refactors row identity tests.
File summaries
| File | Description |
|---|---|
thread-context.tsx |
Removes header-cache context. |
wrapper/wrapper.tsx |
Uses centralized row identity. |
separator.tsx |
Uses shared identity derivation. |
row-metadata.tsx |
Makes header metadata derivation pure. |
row-metadata.test.ts |
Tests provisional metadata behavior. |
row-identity.tsx |
Implements identity caching and hooks. |
row-identity.test.tsx |
Tests sticky headers and pool keys. |
list-area/index.tsx |
Uses centralized recycling-pool keys. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| scrollDirection: 'none', | ||
| }) | ||
| }) | ||
| expect(result.current.poolKeyFor(ord(702))).toBe('text:hdr') |
| snapshot: ConversationThreadState, | ||
| ordinal: T.Chat.Ordinal | ||
| ): RowIdentity => { | ||
| const you = useCurrentUserState.getState().username |
| <ConversationThreadStoreContext value={p.store}> | ||
| <ShownUsernameCacheContext value={p.shownUsernameCache}>{p.children}</ShownUsernameCacheContext> | ||
| </ConversationThreadStoreContext> | ||
| <ConversationThreadStoreContext value={p.store}>{p.children}</ConversationThreadStoreContext> |
"Does this row paint an author header, and therefore which recycling pool does it join" was computed twice per row per render, in two modules, from a mutable Map passed through a context - and a third time in the separator, which wrote to that same Map on the way past. getMessageShowUsername had a pure signature and a hidden write, so whichever caller ran first decided what the others saw: getItemType read the live store, the row read its own selector's snapshot, and a row typed headerless while it renders a header lands in the headerless pool and poisons that pool's height average. The code documented the hazard rather than removing it. row-identity.tsx owns it. useRowIdentity(ordinal) for a row, useRowPoolKey() for getItemType, and one derivation behind both, memoized per store snapshot - so the two callers cannot be told different things about the same row, whoever asks first. The sticky header record is private to the module, per conversation, and reset with the window generation rather than by a hand- placed clear inside messagesClear. getMessageShowUsername keeps its pure signature and loses the write: it now answers what the row shows and whether that answer is firm enough to remember (`provisional`), which is the half that was always pure. list-area no longer reaches past the thread-context interface to the store, the shown-username cache, the current user and getMessageRowType just to answer getItemType, and ShownUsernameCacheContext is gone from thread-context. row-metadata.test.ts keeps the pure half, now asserting `provisional` directly. The sticky half moves to row-identity.test.tsx, along with the failure mode that was unreachable while the cache was shared: the row and the list must be told the same thing, whichever asks first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rccpV5nLxxC5opF5xzrz7
The architecture doc still routed thread loads through `loadMoreMessages` -> `loadConversationThreadMessages` in thread-load.tsx and described `ShownUsernameCacheContext` as a sibling of the thread store. Both are gone: the loads go through requestWindow/runThreadWindowLoad in thread-window.tsx, and the sticky header record is a module-level cache in row-identity.tsx keyed on the store and thrown away with the window's generation. thread-load.tsx is still there and still worth naming, but it no longer holds the RPC - what is left is what both the window and the row derivation draw on: meta and current-user lookups, exploding-mode-from-gregor, snapshot accessors, pagination sizing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S
`you` was read off the store inside the derivation, which put it outside every subscription that depends on it. The wrapper re-renders through its own username subscription, but the separator's thread selector does not re-run and getItemType keeps the same callback - so a change while the thread stayed mounted put the painted header, the separator and the recycling pool back into exactly the three-way disagreement this module exists to make impossible. It is passed in now, and all three entry points subscribe to it. The cache was thrown away wholesale on that change, which is the other half of the same mistake: it reads as "this is handled" while collapsing every reserved header in the thread at once. `you` reaches almost nothing - one row type suppresses its header when the invitee is you - so only the rows that answer differently under the new name give up their reserved height. Which ones those are is asked of the derivation rather than spelled out against the one case that reads `you` today, so a second one cannot quietly go stale there. They do give it up rather than keeping the gap: unlike the scroll-back load this record exists to smooth over, a different signed-in user is a real change in what the row is, and holding space for a header that is never coming back is not a kindness. The ordering test did not test ordering. `act` flushes renders, so a single hook holding both callers has always already computed the row by the time the pool is asked. The real shape - and the one getItemType actually runs in - is a row the list types while nothing renders it, so the pool consumer and the row consumer mount separately, with the sticky record written by the list's question and the row arriving afterwards to agree with it. RowIdentity goes file-local; nothing outside the module names it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S
chrisnojima
force-pushed
the
nojima/HOTPOT-arch-03-row-identity
branch
from
September 11, 2026 14:19
938c259 to
76e9376
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Whether a row paints an author header — and therefore which recycling pool it joins — was computed twice per row per render, in two modules, from a mutable
Mapthreaded through a context.getMessageShowUsernamehad a pure signature but a hidden write:shownCache.set(ordinal, showUsername). Whichever caller ran first decided what the other saw.list-area/index.tsxreached past the thread-context interface touseConversationThreadStore().getState()to call it forgetItemType.wrapper/wrapper.tsxcalled it again inside a shallow-compared selector — different subscription semantics over the same mutating cache.separator.tsx, also wrote to the shared Map.The code documented the hazard itself:
Change
messages/row-identity.tsxowns the derivation:All entry points go through one derivation memoized per store snapshot in a
WeakMap, so the callers cannot be told different things.getMessageShowUsernamekeeps its pure signature and loses the write — it returns{provisional, showUsername}, and the sticky record lives module-private, keyed per conversation and reset on the windowgenerationrather than by a hand-placedclear().ShownUsernameCacheContextis gone fromthread-context.list-areano longer importsuseConversationThreadStore,useCurrentUserStateorgetMessageRowType.Validation
lint:allclean —0 bailed out, 0 whole-props deps, tsc clean both projects.jest --runInBand— 231 suites / 2262 tests.row-metadata.test.tskeeps the pure half.row-identity.test.tsxcovers the sticky half plus the previously-unreachable failure mode: the row and the list are told the same thing whichever asks first. All six mutation-checked.