Skip to content

refactor(chat): one centering module, one closed loop - #29606

Draft
chrisnojima wants to merge 3 commits into
nojima/HOTPOT-arch-01-thread-windowfrom
nojima/HOTPOT-arch-02-thread-centering
Draft

refactor(chat): one centering module, one closed loop#29606
chrisnojima wants to merge 3 commits into
nojima/HOTPOT-arch-01-thread-windowfrom
nojima/HOTPOT-arch-02-thread-centering

Conversation

@chrisnojima

@chrisnojima chrisnojima commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Problem

"Center the thread on message X" was a five-module conversation with three representations of the target and three independent closed-loop scroll correctors.

  • center-context.tsx derived an ordinal from a messageID by identity cast — not the same as the real resolver in thread-message-state.tsx, which neither it nor thread-context called.
  • list-area/index.tsx ran a desktop corrector, a native corrector, and a second native loop driven by onViewableItemsChanged on a [50,250,500,900] timer ladder — each with its own cancellation token, retry budget and deadband.
  • Nothing outside list-area could observe whether centering succeeded, so search.tsx advanced its n of m counter on !!message?.id — for a hit the list might be unable to reach. The clamp was silently absorbed by a pinnedChecks >= 3 check.

Change

The interface returns the outcome:

centerOn(messageID, highlightMode): Promise<'centered' | 'clamped' | 'not-found'>
useConversationCenter() // → {centeredOrdinal, centeredHighlightOrdinal, hasCenter}

Three correctors become one loop behind a single adapter — {measureTarget, scrollToIndex, scrollToOffset} — with exactly two implementations, desktop (LegendList) and native (FlatList). measureTarget returns measured | offscreen | pending; pending is native's answer while its viewable range is stale, so the shared loop waits rather than correcting off a stale reading.

The identity cast is deleted; the target resolves through the real getOrdinalForMessageID, and the row must also be present in messageOrdinals. pinnedToEndRef and the end-anchor loop become the module's own ScrollOwner state.

Ported verbatim

8px desktop deadband; native's half-row deadband and 0.9 damping; 3 settle checks; 3 pinned checks → clamp; 3s budget; native's 12-correction cap; the onScrollToIndexFailed retry (cap 5, reset per target); abort on wheel / onScrollBeginDrag; the end anchor's 2000ms/50ms/2-correction budget.

Bug found and fixed

Re-centering on the message you are already on left the promise hanging until the not-found watchdog fired — the effect was keyed on the ordinal alone, which does not change. Covered by a test that fails on either half of the fix.

⚠️ Needs a device check

On native the [50,250] and [50,250,500,900] ladders and the onViewableItemsChanged corrector are replaced by the shared 50ms poll loop. It is a superset of the old firing schedule under the same caps, but this has not been verified on a simulator or device. The desktop path is covered by the corrector tests.

Validation

lint:all clean — 0 bailed out, 0 whole-props deps, tsc clean both projects.
jest --runInBand230 suites / 2258 tests.

The file had no tests. Added 17 (deadband, settle count, clamp, offscreen, stale/pending, abort, correction budget, timeout, end anchor, outcomes through the provider, the ordinal resolution the identity cast got wrong) plus 3 in search.test.ts. Every one mutation-checked.

This comment was marked as outdated.

chrisnojima and others added 3 commits September 11, 2026 09:57
"Center the thread on message X" was a five-module conversation with three
representations of the target and three independent closed-loop scroll
correctors. center-context derived an ordinal from a messageID by identity
cast, which is not what the real resolver answers - a message you sent keeps
the fractional ordinal it had in the outbox, so the cast highlighted a row
that does not exist. thread-message-state held the real resolver and nobody
called it. list-area held a desktop pixel-measuring loop, a native
index-measuring loop, a second native corrector on a [50,250,500,900] timer
ladder, and the pinnedToEnd/end-anchor pair, all as free refs.

centering.tsx now owns it. centerOn(messageID, highlightMode) returns the
outcome - 'centered' | 'clamped' | 'not-found' - and behind it sits one
messageID -> ordinal resolution through getOrdinalForMessageID, the
clear-and-reload through requestWindow({anchor: {centeredOn}}), and one
corrector loop driven through one adapter interface: measureTarget,
scrollToIndex, scrollToOffset. Exactly two adapters, LegendList and FlatList,
both in list-area. The deadbands, the settle and clamp counts, the correction
budget, the retry ladder and abort-on-user-scroll are ported, per adapter
where they differ: desktop measures pixels against the DOM, native measures
in index space against the viewable range and reports a stale range as
pending rather than correcting off it. The end anchor and the who-owns-the-
scroll state move in with them.

search.tsx steps on the outcome. It advanced the "n of m" counter on
!!message.id alone, so a hit the thread could not produce left the counter
claiming a row that never rendered; a clamp - a hit within half a viewport of
either end - was absorbed silently. The counter is now taken optimistically
and handed back only on 'not-found'.

The file had no tests. The corrector is now addressable without a DOM, so it
has them, along with the end anchor, the outcomes through the provider, and
the ordinal resolution that the identity cast used to get wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rccpV5nLxxC5opF5xzrz7
…e clock

`not-found` was reported after three seconds of wall clock, whatever the thread
was doing. It is the one outcome a caller reads as "this hit is unreachable" -
search hands its counter back on it - so a slow RPC retracted a hit that was
moments from arriving, which is the same lie the counter used to tell, arrived at
from the other direction. It is now judged on the reload this request asked for
having finished: a new generation, loaded, and the message not in the window. If
the window never settles at all the honest answer is not `not-found`, so that
stays `clamped` and the caller's optimistic answer stands.

The retreat in search was half a retreat. centerOn had already cleared and
reloaded the thread around a message it turned out not to hold, so handing back
only the counter left the reader on a window centered on nothing while `n of m`
named a row somewhere else - and for the first hit of a fresh search, which comes
in as select(0, 0), the rollback did nothing at all. It now goes back to the hit
it came from, centre included, or gives the centre up when there is nowhere to go.
One step, not a walk: the retreat passes itself as its own previous, so a second
miss lands on the give-up branch instead of unwinding the list.

onScrollToIndexFailed scheduled six retries against a cap of five.

The ordinal-in-window test both the centered selector and the watcher need is one
function now, so the question "is this message actually in the thread" cannot be
answered two ways.

Also binds a correction to the request that started it. Waiting for the adapter
can park a correction for the whole timeout, and the existing signal check passes
for a stale resumption that overwrites the newer signal on its way through. I
could not build an interleaving that reaches it - abortCorrection covers the
orderings I could construct - so this is hardening of an invariant the module
already relies on elsewhere, not a fix for a demonstrated failure, and it carries
no test for that reason.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S
Moved here from the row-identity branch, where it was written: it is this
module's code, and this is the PR that should carry it.

The native adapter measures in index space and converts to pixels with the
average row height, so every value it reports scales by that average. Fed a
content height the list has not reported yet, the average is zero - and `offBy`
and `tolerance` are then both zero, which is indistinguishable from "already in
the middle". The corrector counted its three settled readings, returned
'centered' without ever issuing a scroll, and search recorded the hit as reached:
`n of m` parked on a row the reader was never taken to.

The viewable range had the same shape of problem from the other side. It is
indices into one window, and dropping the window renumbers all of them, but
nothing cleared it. Before this branch that was survivable because the coarse
scrollToItem went out first and again at 50/250ms; it stopped going out, because
`measureTarget` asked for the anchor only when the target was missing from the
window and the corrector only runs once the target is confirmed present - so
scrollToIndex and the onScrollToIndexFailed retry behind it had become
unreachable.

The range and the height it is scaled by are now dropped with the generation they
describe - value compared against a ref, not a dependency, because a
react-native-screens freeze/thaw re-mounts effects while the refs survive and
zeroing good geometry on a thaw would leave nothing to restore it. The three ways
the measurement can be fed nothing useful ask for the anchor instead of reporting
a measurement, which is what makes the coarse anchor reachable again on first
mount and after every window swap.

A briefly absent range is not one of those three. The list reports an empty
viewable set for a frame whenever a scroll lands where its cells have not
rendered yet, and re-anchoring there throws away a fine correction that may be
one reading from settling. With a known scale that is a `pending` poll.

`offscreen` becomes `needs-anchor`: both adapters used it to mean "no trustworthy
frame of reference yet", and the corrector's answer to it has always been to
coarse-anchor and re-poll.

The arithmetic moves to measureNativeCenter, pure and apart from the refs that
feed it, because that is the half worth testing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S
@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-arch-02-thread-centering branch from 238c146 to b77569c Compare September 11, 2026 13:57
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