fix(chat): stop stranding ordinals outside the loaded thread window by chrisnojima · Pull Request #29589 · keybase/client

chrisnojima · 2026-09-02T14:04:58Z

Fixes a chat thread that stops scrolling back after a DB nuke, sitting on
"Digging ancient messages..." forever with an ancient "set the channel name"
row pinned above the loaded window.

Root cause

messageOrdinals is the data array for LegendList, so a stranded item at
index 0 means back-paging stops being a pure prepend and onStartReached fires
against that row instead of the real top of loaded content.

The chain, confirmed live rather than inferred:

After a nuke the inbox localizer caches the ancient METADATA message
(setChannelname, msg ID 1) into thread storage, unboxed in quick mode.
Every thread load runs a post-send ResolveSkippedUnboxeds, which
re-unboxes those quick-mode messages, marks them changed, and emits a
MessagesUpdated notification.
The client routed that to applyMessagesUpdatedToThread -> addMessages,
which had no notion of the loaded window, so ordinal 1 landed at index 0.

Client

addMessages may not strand an ordinal below the window
(dropNewBelowWindow). A notification may update a message already in the
window and may append above it, but may not introduce a new ordinal below
the window floor. Thread loads are unaffected — they remain the only thing
allowed to extend the window downward. Nothing is dropped: the message loads
normally when paged back to.
validatedRange prune narrowed to whole windows. In INCREMENTAL mode the
service filters the full response down to changed messages once a cached
thread has been sent, so neither response is a complete window. The old
reconciled latch fired on whichever arrived first — the cached one — and was
observed deleting 6 real messages on a focused refresh (96 -> 89 -> 90
ordinals).
jumpToRecent clears before reloading, matching loadMessagesCentered;
it used to merge a disjoint newest window into the old one.
Phantom-ordinal fix: the placeholder early-continue ran before the
ordinal remap, stranding _m.ordinal with nothing in messageMap when a sent
message kept its fractional outbox ordinal.
Orange line: loadOrangeLine now bails on a non-positive read position.
emptyConversationMeta reads readMsgID: -1, and the old code did
readMsgID < 0 ? 0 : readMsgID, turning "unknown" into "nothing read" so the
service answered with the oldest message. State is set once and only refreshed
while the conversation is inactive, so it stuck.
Dead loadNextAttachment deleted — zero callers.

Service

Split out into #29590, which is based on this branch: patchPaginationLast
(a page reaching message ID 1 or the nukepoint is the last page, so the
spinner stops) and topicNameMemCache (one 100-message page in a 28-channel
team cost ~2,800 METADATA fetches). Their tests and verification live there.

Verification

The client fix was validated against a real keybase db nuke plus a
keybasefriends#help scrollback, using a temporary probe (removed in the last
commit):

before:  cached dir=back [7141..7151] -> range=[1..7421]  >>> BAD unexplained gap 1->7141(7140)
after:   full   dir=back [7152..7276] -> ordinals=180 range=[7152..7421] maxgap=19 gaps=40 >>> OK

The floor is the result: it used to read range=[1..7421]; the back page now
extends the window 7277 -> 7152 as a pure prepend, with no setChannelname row
at the top of the thread.

yarn lint:all is clean (0 bailouts, 0 whole-props deps).

Second stall: a back page of pure tombstones

A back page can be composed entirely of messages the thread will never render.
A message superseded by a DELETE arrives as a hidden placeholder
(CreateHiddenPlaceholder), becomes deleted on the client
(message.tsx:1132), and addMessages drops it — deleted is the client's
only ordinal-dropping filter, since every other hidden type is already
filtered server-side. messageOrdinals is then identical to what it was, the
list never fires onStartReached again, and scrollback stops even though the
pager says there is more. Observed as
incoming=882 [12599..13585] -> ordinals unchanged.

The fix notices that case and asks for the next page directly.

What bounds it is message IDs, not a retry budget. The tombstones still
carry IDs, IDs only increase, and each request is one page, so requiring every
reload to reach strictly further back walks a finite space one page at a time.
A service that keeps handing back the same window stops us on the first repeat.

Deliberately narrow: backwards scrollback only, and the authoritative pass only
— once a cached thread has been sent the service filters the full response down
to what changed, so a cached pass adding nothing is expected.

Note the service is arguably the one at fault here: it knows it is handing up a
page of tombstones, having produced them in TransformSupersedes. This is the
client half.

Review pass

Four reviewers went over this. The substantive findings, all fixed in
c4c36ece86:

The back-page reload fired on the normal warm-cache sequence. Judging the
full pass on ordinal count alone treats an INCREMENTAL response — only the
changed messages, all already in the window — as "added nothing", so one
scroll gesture walked the client back through the whole conversation. Now
gated on whether a cached pass actually delivered content.
That same flag revives validatedRange pruning, which was dead code
because the flag was set even for an empty cached pass.
Three fixes had no effective coverage. Deleting dropNewBelowWindow from
its callsite, the !sawCachedPass narrowing, or jumpToRecent's clear each
left the full 2150-test suite green. All three are now pinned.
Two findings against the cache and one against the pager moved to fix(chat): mark a page last when it reaches the beginning, cache topic names #29590
with the code.

Also: the write-only authoritative prop removed.

Second review pass

jumpToRecent kept anything above the floor it dropped, on the theory
that the reload lands newer than that. It reloads the newest page, which for
a reader parked far back starts thousands of ordinals above where they were,
so an update arriving in the gap for a message just above the old floor
installed itself as the whole window and stranded — the bug this pr exists to
close. Both callers now clear unconditionally, which leaves
ClearedWindow.floor with no reader and collapses the struct to a boolean.
The drop set was keyed on the raw ordinal but tested with the remapped
one, so a push whose own ordinal sat inside the window could still be
written outside it. Both sides now use the ordinal the message will occupy.
sawCachedPass was set from a non-empty response rather than from
messages. A cold cache still sends a cached pass carrying nothing, which
suppressed the stale-ordinal prune on exactly the first load after a nuke —
where INCREMENTAL filters nothing out and the full pass is a whole window.

Every fix in this pr is mutation checked — removing any one of them fails a
test.

Copilot

🟡 Changes recommended

Empty cached passes and stale in-flight loads can still reopen or repopulate a cleared thread window.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes chat pagination stalls and unread-divider behavior after database resets while strengthening regression coverage and validation guidance.

Changes:

Enforces contiguous thread windows and safely retries tombstone-only back pages.
Corrects orange-line initialization and mark-read timing.
Removes dead attachment code and updates developer tooling documentation.

File summaries

File	Description
`skill/playwright-cli/SKILL.md`	Documents Electron tab identification.
`shared/chat/conversation/thread-message-state.tsx`	Prevents out-of-window ordinal insertion.
`shared/chat/conversation/thread-message-state.test.tsx`	Tests message-window invariants.
`shared/chat/conversation/thread-load.tsx`	Reconciles loads and retries tombstone pages.
`shared/chat/conversation/thread-load.test.tsx`	Tests paging, pruning, and gate behavior.
`shared/chat/conversation/thread-context.tsx`	Manages window clearing and mark-read timing.
`shared/chat/conversation/thread-context.test.tsx`	Adds integration-level regressions.
`shared/chat/conversation/normal/container.tsx`	Defers orange-line loading until metadata exists.
`shared/chat/conversation/normal/container.test.tsx`	Tests unread-position handling.
`shared/chat/conversation/attachment-actions.tsx`	Removes an unused attachment loader.
`CLAUDE.md`	Expands validation requirements.
`.claude/hooks/pre-commit-check.sh`	Adds React Compiler bailout checks.

Review details

Suppressed comments (1)

skill/playwright-cli/SKILL.md:296

This command annotation repeats the wrong URL fragment; the renderer URL used by this repository is main.dev.html.

playwright-cli tab-list                 # find the row whose URL has main.html

Files reviewed: 12/12 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.

    }) => {
      updateThreadState(s => {
        s.loaded = true
+        s.windowCleared = false


+        // A clear under us - jump to recent, a centered jump - means this chain is walking back
+        // from a window that no longer exists, and would prepend pages the reader never asked for.
+        after.clearVersion === clearVersionAtLoadStart


+  // There is no valid message ID 0, so a non-positive read position means the conversation's meta
+  // has not landed yet (emptyConversationMeta reads -1), which a DB nuke makes the norm. Asking the
+  // service with 0 answers "everything is unread" and pins the line above the oldest message, and
+  // the state is set once, so that answer used to stick for the life of the mount.


-### Identifying the main app tab
+### Identifying the main app tab — do this EVERY time
+
+Never reuse a remembered index. Re-run `tab-list` and pick the row whose URL contains `main.html`


A DB nuke left an ancient "set the channel name" message pinned at the top of threads, thousands of ordinals below the loaded window. messageOrdinals is the data array for LegendList, so a stranded item at index 0 makes back-paging a mid-list splice instead of a prepend, and onStartReached fires against that row rather than the real top of loaded content -- scrollback stops firing and the thread sits on "Digging ancient messages..." forever. It arrives by notification, not by thread load: the localizer caches the ancient METADATA message into thread storage unboxed in quick mode, then every load's post-send ResolveSkippedUnboxeds re-unboxes it, marks it changed, and pushes a MessagesUpdated notification into addMessages, which had no notion of the window. - addMessages may update a message already in the window and may append above it, but may not introduce a new ordinal below the window floor. Thread loads are unaffected; they are the only thing allowed to extend the window down. Nothing is deleted -- the message loads normally when paged back to. - Narrow the validatedRange prune to responses that are provably whole windows. In INCREMENTAL cb mode the service filters the full response down to changed messages once a cached thread has been sent, so neither response is complete; the old latch fired on the cached one and was observed deleting 6 real messages on an ordinary focused refresh. - jumpToRecent clears before reloading, the way loadMessagesCentered does, instead of merging a disjoint newest window into the old one. - Do not resolve the orange line from a non-positive read position. emptyConversationMeta reads -1, and clamping that to 0 asked the service "what is unread when nothing has been read", pinning the line above the oldest message; the state is set once and only refreshed while the conversation is inactive, so the wrong answer stuck. - patchPaginationLast: a page holding message ID 1 has reached the beginning, checked before the expunge record, which reads Upto:0 for a nuked conversation whose history was deleted and so never fired. - Drop the unused loadNextAttachment, and stop a superseded placeholder from leaving its own ordinal in the list with nothing in messageMap.

ParseChannelNameMentions runs on every message body holding a `#token`, and the regex matches ordinary text like "#1", so a busy channel hits it constantly. Each call went to GetChannelsTopicName, which read the inbox and then fetched the METADATA message of every channel in the team, uncached. Unboxing one 100-message page in a 28-channel team cost thousands of single-message fetches: one measured burst turned 13 requests into 42,461 GetMessages calls. Memoize per (tlfID, topicType, uid). The window is short and deliberately needs no invalidation on rename or channel create/delete: a page's resolutions all land within milliseconds of each other, so a few seconds collapses them into one, while keeping a renamed channel from lingering long enough to notice. The cache check sits before the Trace defer. Tracing a hit costs two log lines, and hits ran to ~8,500 per burst, which is its own drag on a debug build.

Tab order is not stable across app restarts, and a DevTools window can take slot 0. A wrong tab fails silently: DevTools answers every eval plausibly and its console is nearly empty, so bad readings look like findings rather than mistakes. Require re-running tab-list and asserting the URL each time, and note that console capture only starts when playwright attaches, so earlier output lives in the user's own DevTools and cannot be pulled through playwright.

The probe did its job: on a post-nuke `keybasefriends#help` scrollback the back page now reads `[7152..7276] -> ordinals=180 range=[7152..7421] >>> OK`, where it used to strand ordinal 1 and report `range=[1..7421]`. Drops `GAPPROBE`, `describeOrdinalGaps`, the `gapProbeAccounted` ref and the two `logger.info` blocks in the thread context, plus their tests. The `dropNewBelowWindow` invariant they were validating stays.

Drops the PullLocalOnly local-max guard. It was never proven: in every nuke reproduction both fetches failed and returned a clean miss before reaching it, so the branch has no evidence behind it and no test could reach it either. Adds unit tests for the two service changes that are reachable without kbweb. Neither calls setupTest, so neither needs localhost:3000 - patchPaginationLast only needs a bare GlobalContext for its logger, and the topic name cache is pure. - patchPaginationLast: message ID 1 ends the thread with expunge nil, with expunge reading back Upto:0 (the post-nuke case that stuck the spinner), and regardless of page order; the pre-existing nukepoint behaviour still holds; Last is never turned back off; a nil page does not panic. - topicNameMemCache: round trip, key separation across TLF/topic type/uid, copies in both directions so neither side can reach into the shared slice, TTL expiry either side of the boundary, and clear via clearCache, OnLogout and OnDbNuke. Mutation checked: reverting the ID-1 check fails 3 subtests, and returning the cached slice by reference fails the copy test.

A back page can arrive composed entirely of messages the thread will never render. The service filters EDIT/DELETE/REACTION/ATTACHMENTUPLOADED/UNFURL/ TLFNAME out before they reach us, but messages superseded by a DELETE still arrive as `deleted`, and addMessages drops those from the ordinal list. The list is then identical to what it was, so LegendList never fires onStartReached again and scrollback stops for good even though the pager still says there is more to come. Observed as `incoming=882 [12599..13585] -> ordinals unchanged`. The real fix belongs in the service, which knows it is handing up a page of tombstones. This is the liveness belt: the thread must not be one unlucky response away from a permanent stall. `shouldRetryEmptyBackPage` is deliberately narrow — backwards scrollback only, the authoritative pass only (once a cached thread has been sent the service switches the full response to INCREMENTAL, so a cached pass adding nothing is normal), the page must have contained something, the pager must still say there is more, and it is bounded at 3 re-issues. Mutation checked: every clause of the predicate fails a test when inverted, and removing the wiring turns the bounded re-issue from 4 calls into 1.

A back page can be composed entirely of messages the thread will never render. A message superseded by a DELETE arrives as a hidden placeholder, becomes `deleted` on this side, and addMessages drops it. `messageOrdinals` is then identical to what it was, so the list never fires onStartReached again and scrollback stops even though the pager says there is more to come. Observed as `incoming=882 [12599..13585] -> ordinals unchanged`. Notice that case and ask for the next page directly. The tombstones still carry message IDs, and message IDs only increase, so requiring each reload to reach strictly further back is what bounds this: the ID space is finite and we walk it one page at a time. There is no retry budget to outrun, and a service that keeps handing back the same window stops us on the first repeat. Deliberately narrow: backwards scrollback only, and the authoritative pass only - once a cached thread has been sent the service filters the full response down to what changed, so a cached pass adding nothing is expected. Mutation checked: each of the five clauses fails a test when removed.

Reviewers found that the empty-back-page reload fired on the normal warm-cache sequence, and that three of the behaviour changes here could be deleted with a green suite. Reload cascade. The gate judged the full pass on ordinal count alone. On a warm cache the cached pass carries the whole page, `ordinalsBefore` is sampled after it lands, and the INCREMENTAL full pass that follows carries only changed messages - all already in the window. That is indistinguishable from a page of tombstones, so one scroll gesture walked the client back through the entire conversation, bypassing both throttles. Gate on `sawCachedPass`, and set that flag only when a cached pass actually delivered content. `why === 'full'` is now implied by it, so it goes. The same flag fix revives `validatedRange` pruning, which had been dead since the flag was being set even for an empty cached pass. Tests that could not fail. Removing `dropNewBelowWindow` from its callsite, the `!sawCachedPass` narrowing, or `jumpToRecent`'s clear all left 2150 tests green - the four `dropNewBelowWindow` unit tests pass the flag themselves, and the `validatedRange` test used a degenerate one-message range with nothing to prune. Added a callsite test for the window invariant, made the incremental pass span the gap it is supposed to protect, and covered the jumpToRecent clear. The back-page fake now models addMessages instead of a frozen array, so a page of tombstones and a page of real messages no longer look alike to it. Service. GetChannelsTopicName cached whatever survived its error paths, pinning a degraded answer for the window - worst right after a db nuke, when local storage has no METADATA messages. Only cache a complete result. The claim that this needs no invalidation was wrong: a resolution becomes MessageUnboxedValid.ChannelNameMentions and is stored with the message, so expiry heals later resolutions, not committed ones. Comment corrected to say so. `oldest <= 1` tightened to `== 1` with a boundary case, and the TTL constant pinned. Also removes the write-only `authoritative` prop left over from the diagnostic probe (5 writes, 0 reads). Every fix above is mutation checked: removing any one of the six fails a test.

…channel read Two review findings. dropNewBelowWindow dropped the ordinal but had already written the message to messageMap, messageIDToOrdinal and messageTypeMap - the skip sat in the third loop, after the store. So messageIDToOrdinal.get(1) resolved to an ordinal with no row, and getOrdinalForMessageID handed that to callers in thread-engine, who then acted on a message the thread was not rendering. Decide the drop up front instead, before anything is written, and skip those messages whole. GetChannelsTopicName treated an empty conversation list as a complete result and cached it. A chat TLF always has at least #general, so zero ACTIVE channels only happens on a degraded inbox read - exactly the case the completeness gate was added for, surviving in the one shape the flag did not cover. It would pin "this team has no channels" for the window, and GetChannelTopicName would return "no convs found" to its callers. Also notes in the comment that an incomplete result is still returned to the caller even though it is no longer cached, so a resolution committed during a degraded read is missing channels regardless. That is pre-existing. Mutation checked: restoring the store-then-drop shape fails two of the three new map-cleanliness tests.

Round-2 review found the reload was inert in exactly the case it exists for, and that reviving validatedRange pruning had turned a latent bug live. The reload gate keyed on sawCachedPass, which was set by any non-empty JSON string. A warm cache delivers a back page on the CACHED pass, so for a conversation already in local storage - the ordinary case - the flag was always set and the reload never fired. It only ever worked on a cold-cache miss. Judge the whole load instead of one pass of it: capture the window floor before either pass, accumulate the oldest message ID seen across both, and decide after the full pass, which is the last. A page of real messages moves the floor wherever it arrived; a page of tombstones does not. That covers the warm case and the cascade the previous round fixed, with one mechanism. Compare the floor rather than the ordinal count, so a page that adds real messages while its `deleted` entries remove more from the window is not misread as no progress. Stop the chain when clearVersion moves: jump to recent and a centered jump both clear and reload, and a chain still walking backwards would prepend pages into a window the reader has left. The placeholder branch dropped its ordinal from incomingOrdinals without re-adding the remapped one, so once pruning was no longer dead code the validatedRange prune deleted the real message underneath. Verified as new: the probe passes on the base branch and fails on the previous head. dropNewBelowWindow was inert with an empty window, and jumpToRecent now empties it - so the branch's own bug was reproducible by tapping jump to recent, giving ordinals [1, 9001]. The floor now survives messagesClear. Orange line: readMsgID <= 0 also suppressed 0, which ReaderInfo reports for a conversation never read - every first open of a new channel, where "everything is unread" is the right answer. Only negative means unknown. The mount-time freeze also pinned -1 forever, so the latch now waits for a real value. All 13 fixes are mutation checked; three needed new tests to become so. Not covered: the orange line recovering after localization lands.

The pre-commit hook ran eslint and tsc but not lint:bailouts, which is the only thing that catches react-compiler bailouts - no compiler rule is wired into eslint.config.mjs, so eslint passing says nothing about them. A bailout could reach a commit with the hook green. Also requires running lint:all and /code-review high against your own diff before reporting a TS change complete, rather than handing unvalidated work over for review.

A centered window - a search result - has more to load above and below it, so a notification newer than its ceiling strands against a hole exactly as one older than its floor does. Guard both edges, with the ceiling counting only while moreToLoadForward, so a live message still appends once the window reaches the newest message. clearedWindowFloor becomes clearedWindow, because the two callers that clear then reload land in different places: jumpToRecent reloads newer than the old floor, so a push above it belongs in what is coming, while a centered jump reloads an arbitrary region where nothing arriving first can be placed at all. The gate a clear puts up could also stick. applyThreadLoad only dropped it when a load actually applied, so an offline load, a kicked-from-team load, a response carrying no thread, or a bail before the RPC left it up for the life of the provider. Release it when the load ends however it ends, keyed on clearVersion: the thread load generation only moves when the conversation changes, so a load that started before a clear would otherwise pull down the gate belonging to the load that started after it. Mark-read reads the newest message out of the thread window and needs no meta, so on an unlocalized conversation - the norm right after a db nuke - it always beat localization and overwrote the read position before useOrangeLine could ask for the unreadline against it, leaving a channel with genuine unread messages showing no divider at all. Defer it until the read position is known, and run it again when localization lands. The no-new-ordinals back page reload now goes through the throttled action rather than calling the loader directly, and stops after ten pages. Strict progress in message ID alone let one scroll gesture walk an expunged history for minutes; scrolling away and back starts a fresh chain from where it stopped.

Three window-gate holes found reviewing this branch. jumpToRecent kept anything above the floor it dropped, on the theory that the reload lands newer than that. It does not: it reloads the newest page, which for a reader parked far back starts thousands of ordinals above where they were. An update arriving in the gap for a message just above the old floor installed itself as the whole window and stranded once the page landed - the bug this branch exists to close. Both callers now clear unconditionally, which leaves ClearedWindow.floor with no reader, so the struct collapses to a boolean. The gate's drop set was keyed on the raw ordinal but tested with the one an outbox or messageID match remaps the message to, so a push whose own ordinal sat inside the window could still be written outside it. Both sides now use the ordinal the message will actually occupy. sawCachedPass was set from a non-empty response rather than from messages. A cold cache still sends a cached pass - PullLocalOnly's collector suppresses the miss - carrying nothing, and that suppressed the stale-ordinal prune on exactly the first load after a db nuke, where INCREMENTAL filters nothing out and the full pass is a whole window after all.

Refusing to cache an incomplete result meant never caching one. The inbox read asks for every member status, so a team carries channels the user has left or never joined, and those fail to resolve on every pass - "incomplete" is the steady state, and under that rule the cache stayed empty and the per-#token fan-out it exists to collapse stayed with it. An incomplete result is now cached under a much shorter TTL, long enough to collapse the burst one page of messages fires and short enough that a channel which becomes resolvable is picked up on the next page.

patchPaginationLast and the topic-name cache are service-side and reviewable on their own; they move to a branch stacked on this one so this pr is the client window work alone.

chrisnojima force-pushed the nojima/chat-ordinal-gap-invariant branch from 45e09dd to fcbcb7b Compare September 4, 2026 13:46

Base automatically changed from nojima/HOTPOT-deps-9-1 to master September 4, 2026 13:53

chrisnojima force-pushed the nojima/chat-ordinal-gap-invariant branch from fcbcb7b to 9810193 Compare September 4, 2026 13:53

chrisnojima requested a balanced review from Copilot September 4, 2026 14:06

chrisnojima force-pushed the nojima/chat-ordinal-gap-invariant branch from 9810193 to 1342bf8 Compare September 4, 2026 14:06

Copilot started reviewing on behalf of chrisnojima September 4, 2026 14:07 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

chrisnojima added 15 commits September 4, 2026 16:27

revert(chat): take the go changes out, they land in their own pr

f8a5464

patchPaginationLast and the topic-name cache are service-side and reviewable on their own; they move to a branch stacked on this one so this pr is the client window work alone.

chrisnojima force-pushed the nojima/chat-ordinal-gap-invariant branch from 1342bf8 to f8a5464 Compare September 4, 2026 20:28

Navigation Menu

Uh oh!

fix(chat): stop stranding ordinals outside the loaded thread window - #29589

fix(chat): stop stranding ordinals outside the loaded thread window#29589
chrisnojima wants to merge 15 commits into
masterfrom
nojima/chat-ordinal-gap-invariant

chrisnojima commented Sep 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

chrisnojima commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Client

Service

Verification

Second stall: a back page of pure tombstones

Review pass

Second review pass

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisnojima commented Sep 2, 2026 •

edited

Loading