Repository navigation
fix(native-chat): load older messages automatically before the reader reaches the top - #22541
Conversation
… reaches the top The transcript paged older history only on an upward scroll within 80px of the top, and only when the visible row count had changed since the last request, so readers hit a "Load earlier messages" button and waited. A page that landed only hidden rows left the count unchanged and stalled paging. A sentinel above the first row is now observed by an IntersectionObserver rooted at the transcript scroller with a 600px top margin. The observer is recreated each time a page settles, so it keeps paging while the sentinel stays in range and stops once a prepend pushes it out or history runs out. The virtualizer's existing prepend anchoring keeps the reader's row in place. Both lanes' load calls now reject when a page does not land (error result, rejected read, host reset, exhausted re-anchoring). A rejection stops automatic paging and shows the manual button; a successful retry resumes it. While automatic paging works, the top shows only a polite status line.
…ands The older-history row sat in the transcript column above the window, so the final page, which takes that row away, removed its height and the column gap from above the window and every row jumped up by that much. The row now sits out of flow, absolutely positioned inside the scroller's existing top padding, so its presence never changes the content height or the window's scroll margin. The sentinel keeps the same place relative to the top of the transcript, so prefetch is unaffected, and the button stays in the padding band above the first row.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (36)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughOlder-history loading now uses an intersection-observed sentinel instead of scroll-event checks. The live-session state exposes a paging generation that changes on transcript resets and read invalidation. The transcript displays a delayed loading status during automatic paging and offers a manual retry when automatic paging is unavailable or a page fails. Tests cover paging, retries, read states, and viewport position during prepends. Loading-status translations were added for six locales. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Stale paging failures do not stop automatic loading for a replacement transcript. No actionable merge-blocking issue remains after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 31 files. (6 skipped: 6 unsupported.) Full details: Description checkExplanation The description explains the user-visible behavior, implementation, rationale, testing, visual proof, risks, and checklist. However, the required Linked Issue section contains “None,” although the template states that an issue link must always be provided.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: f050dd84-9da4-43af-b24b-cf7e81582956
📒 Files selected for processing (26)
src/renderer/src/components/native-chat/NativeChatMessageList.growth-windowing.test.tsxsrc/renderer/src/components/native-chat/NativeChatMessageList.older-history.test.tsxsrc/renderer/src/components/native-chat/NativeChatMessageList.tool-stream-cost.test.tsxsrc/renderer/src/components/native-chat/NativeChatMessageList.tsxsrc/renderer/src/components/native-chat/NativeChatOlderHistoryRow.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredSession.test-harness.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredSession.test.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredSession.tsxsrc/renderer/src/components/native-chat/native-chat-autoscroll.test.tssrc/renderer/src/components/native-chat/native-chat-autoscroll.tssrc/renderer/src/components/native-chat/native-chat-windowing-test-harness.tsxsrc/renderer/src/components/native-chat/structured-agent-session-read-owner.tssrc/renderer/src/components/native-chat/use-native-chat-live-session-visibility.test.tssrc/renderer/src/components/native-chat/use-native-chat-live-session.test.tssrc/renderer/src/components/native-chat/use-native-chat-live-session.tssrc/renderer/src/components/native-chat/use-native-chat-older-history-autoload.tssrc/renderer/src/components/native-chat/use-native-chat-transcript-scroll.test.tsxsrc/renderer/src/components/native-chat/use-native-chat-transcript-scroll.tssrc/renderer/src/components/native-chat/use-native-chat-transcript-window.tssrc/renderer/src/components/native-chat/use-structured-agent-session-read.test.tsxsrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/fr.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.json
💤 Files with no reviewable changes (3)
- src/renderer/src/components/native-chat/native-chat-autoscroll.test.ts
- src/renderer/src/components/native-chat/native-chat-autoscroll.ts
- src/renderer/src/components/native-chat/use-native-chat-transcript-scroll.test.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run read the full 26-file diff for both commits and independently checked three invariants: the out-of-flow layout/scroll-margin math, the observer lifecycle, and rejection propagation.
- Automatic paging via a sentinel: new
useNativeChatOlderHistoryAutoloadwatches a top sentinel with a 600px prefetch margin, recreating the observer each time a page settles so paging continues while in range; this replaces the removedshouldLoadEarlierscroll-offset/row-count trigger. - Retry-only button: new
NativeChatOlderHistoryRowrenders the sentinel, a delayedrole="status"line, and shows the manualButtononly after a page fails. - Failures now propagate:
loadEarlier(live lane) andloadOlder(structured read owner, newOlderPageOutcome) reject when a page does not land; the list'sloadPageis the single consumer and maps rejection to the retry state. - Layout fix: the older-history row is positioned out of flow so the final page no longer shifts the reader.
- Tests / i18n: new
older-historysuite and updated lane tests;loadingEarlierMessagesadded to all six locales.
The observer cannot stall in range, double-fire, or leak a failure across sessions, and no production call site drops the new rejections. The 12–16px shift the author documents at scrollTop 0 is the pre-existing virtualizer size-correction rule, correctly left out of scope.
Technical details
# Verification notes
## Layout / scroll margin
- `NativeChatMessageList.tsx:331-337` — the row is a sibling of the padded wrapper, so it is never an
ancestor of the sizer; `nativeChatScrollOffsetWithin` walks ancestors only, so `scrollMargin` is
unaffected when `hasMore` flips. Row `h-10` abuts the wrapper `pt-10` gutter.
## Lifecycle
- Both lanes set their loading flag before awaiting, so `shouldObserve` toggles
`true → false → true` on every real settle, recreating the observer. `inFlightRef` guards the observer
and manual paths. A `historyKey` of `${agent}:` (null sessionId) can never record a failure because no
lane loads without a session id.
## Rejections
- `use-native-chat-older-history-autoload.ts:55-62` is the only production invocation of `loadEarlier` /
`loadOlder` and attaches an onRejected handler; `loadEarlierManually` returns void.ℹ️ Nitpicks
NativeChatMessageList.windowing-test-support.tsx:26still describes the "load earlier" block as sitting in flow above the spacer. The sibling harness (native-chat-windowing-test-harness.tsx) was updated for this change, but this one was not — worth a one-line comment refresh so the two harnesses don't describe different layouts.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…doned mid-flight A reconnect snapshot, replacement, or hide ends the lane's loading while its read is still outstanding. The auto-load latch stayed held until that read settled, so the recreated observer's report was dropped and paging stalled with no button. Release the latch once the lane reports loading, and have the transcript lane resolve rather than reject when a superseded read fails, so a replaced connection cannot switch the current transcript to the retry button.
The older-history autoload kept its own "page in flight" latch beside each lane's guard. It could not tell a request the lane had not yet reported from one the lane abandoned before rendering it (a reconnect snapshot in the same batch), so a read that never settled could stall paging with no retry button. The transcript lane now dedupes synchronously on the epoch its page is in flight for, so an epoch bump abandons it with no extra reset path; the structured owner already dedupes synchronously. The autoload hook drops its latch and the effect that cleared it.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the two commits pushed since the prior pullfrog review, which move ownership of the in-flight older-history page out of the list and into each chat lane.
- Each lane now owns the in-flight page: the list-level
inFlightReflatch is gone fromuse-native-chat-older-history-autoload.ts; the transcript lane dedupes synchronously viaolderPageEpochRefand the structured owner viasnapshot.loadingOlder, so a second request beforeloadingEarlierrenders is a no-op instead of a duplicate read. - Stale rejections are distinguished from real failures:
loadEarlierinuse-native-chat-live-session.tsbecameasyncand rethrows only when!isStale(), so a read abandoned by a reconnect snapshot no longer records a failure against the new transcript; a genuine failure still rejects and stops auto-load. - Structured owner refactor:
readOlderPagereturnsOlderPageOutcome(landed/not-landed/stopped) andloadOlderrejects on a page that did not land — including a host reset — so automatic paging stops rather than looping. - Tests: new lane-level cases cover double-call dedup, stale-rejection swallowing, and continued paging after a hung read is abandoned by a snapshot.
Both lanes set their dedup flag before the first await, and each finally only clears a latch it still owns, so removing the list-level latch cannot double-request an equivalent page. I traced the observer, manual-retry, and settle paths in both lanes and found no reachable regression.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…sation Main now resolves older-page reads with an explicit result (applied, unchanged, exhausted, superseded, failed) and joins concurrent callers to the page already in flight. This takes that contract for both chat lanes in place of this branch's reject-on-failure one, and the autoload hook reads the result instead of a rejection. A failed older page in the structured lane no longer puts the whole pane in an error state: the conversation stays on screen and the list offers its retry. Each lane now publishes an older-history generation that changes whenever paging is reset (structured: open, snapshot, reset, dispose; transcript: source swap, snapshot, replacement). The list's failure belongs to one generation, so a reconnect or reset re-enables auto-loading without a click, and a new generation re-checks the range. Auto-loading also stops, offering the manual load, on a page that made no progress, so a host that keeps answering without moving the oldest row cannot drive a request loop. The older-history row and its observer exist only while the read is ready, so a pending or errored read never strands paging; recovery remounts the row and re-checks. The live-session contract types move to their own module to keep the hook under the file-size limit.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor test-contract suggestion inline.
Reviewed changes
This run re-reviewed the single commit pushed since the prior pullfrog review (8df8cf1), a merge of origin/main whose conflict resolution re-adopts main's older-page result contract across both chat lanes.
- Result contract replaces reject-on-failure:
loadEarlier/loadOldernow resolveNativeChatOlderPageResult; the autoload hook maps'failed'/'unchanged'to the retry state and stops paging. - Per-lane older-history generation: both lanes publish
olderHistoryGeneration, the list keys failures on it, so a reconnect/reset re-enables auto-load without a click. - Structured lane no longer errors the pane: a failed older page keeps the loaded conversation and offers the list's retry.
- Row gated on a ready read:
showOlderHistory = hasMore && readPhase === 'ready', so a pending/errored read never strands paging. - Contract module extracted: live-session types move to
native-chat-live-session-contract.ts, re-exported from the hook.
I traced both lanes' resolution paths (each catches to 'failed'; a superseded read maps to 'superseded') and confirmed no production caller can receive a rejection, so the autoload's .catch-less .then cannot leak an unhandled rejection. The four older-history/lane test files pass (71 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…on reaching the list The structured session test still fed a rejection into loadEarlier, a contract neither lane has any more. Assert the resolved page result and the paging generation the list needs to stop and re-arm auto-loading.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the single commit pushed since the prior pullfrog review (8df8cf1), a test-only change to the structured-session list-passthrough test.
- List-passthrough test now matches the resolved-result contract:
loadOlderresolves'failed'and the assertion usesresolves.toBe('failed')instead of the retired.rejects, and it additionally pinsolderHistoryGeneration: 3reachingmessageListProps.session, so the test now guards both the page result and the paging generation the list needs.
The prior review's inline thread is resolved: 2f87f12 changes no production code, and NativeChatStructuredSession.test.tsx passes (18 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Tested commit 8df8cf1. The running app was this worktree. The window identity reported branch brennanb2025/pr22541-grok-qa and worktree pr22541-grok-qa. The dev server returned the native chat message list as JavaScript, and that source contains olderHistoryGeneration and showOlderHistory. This worktree's package.json was served (HTTP 200) and a sibling worktree's package.json was refused (HTTP 403). A. Transcript lane, happy path — PASS A 1000-message Claude transcript was scrolled from the latest 300 messages up to the first one, both by setting the scroll position and by real wheel events. Each older page (300, then 500, 700, 900, then 1000) arrived while the reader was still well above the top. The row on screen did not move when older messages were added, including the last page (measured drift 0). The "Load earlier messages" button never appeared while those reads worked. At the true top the first line is Q0001 and the loading row is gone. The "Loading earlier messages…" line only shows after a read has been running for about a fifth of a second, so a fast local read never flashes it. One read was held for 1.5 seconds to photograph that line. The messages stayed on screen under it, and the retry button did not appear.
B. Transcript lane, failure and recovery — PASS Renaming the transcript away (and, separately, removing its read permission) left the messages already on screen in place. The chat showed "Load earlier messages" above the first row, not on top of it, and did not replace the conversation with "Conversation could not be loaded." It did not keep retrying on its own for the 1.6 seconds it was watched. Putting the file back while that button was showing resumed automatic loading by itself, so the button was often gone before a click could land. One click that did land stepped the loaded window forward, and then a fresh read of the restored file trimmed the window back to the latest 300 messages. Scrolling after that loaded further pages with no retry button. Reopening the chat with the file present also pages on its own.
C. Structured chat, failed older page — PASS A structured Claude chat with 340 history lines was opened (Chat UI, structured chat, and open-agent-tabs-in-chat were on, and the app was restarted after that). Forcing the older-page history read to fail left the conversation on screen and showed "Load earlier messages." It did not replace the chat with "Conversation could not be loaded," and it did not retry on its own. In one scroll the loaded history started at line 41; in the shot below the fault was on while the chat was still filling, so it stopped at line 141 with the same button and the same messages still visible. After the fault was removed, clicking the button loaded line 1 and the button went away.
|
Review status: ready for merge reviewHead: What changed during review
Validation
Known, not addressed here
|
The replayed stablyai#22541 contract makes loadEarlier return the page promise; awaiting it inside act() hung on reads the test resolves later, and the hang cascaded into 22 later cases. Uses upstream's own void-call form.
ELI5
In a chat, scrolling up to older messages meant hitting a "Load earlier messages" button and waiting. Now the chat starts fetching older messages while you are still scrolling toward them, so by the time you get there they are usually already on screen. The button only shows up if loading actually failed, as a retry, and your messages always stay on screen when that happens.
What Changed
Before:
After:
IntersectionObserverrooted at the transcript scroller watches it with a 600px top margin, so the next page is requested about a screen before the reader reaches the top. The observer is recreated whenever a page settles or paging is reset, and its first report re-checks the range. So paging continues while the marker is still in range and stops once a prepend pushes it out or history runs out. Row measurement and resizes can no longer trigger a load, and the row-count gate is gone.role="status"line. It reads "Loading earlier messages…" if a page takes more than 200ms.shouldLoadEarlierand its threshold) is removed rather than kept alongside the new one.native-chat-live-session-contract.tsso the hook stays under the file-size limit.Deliberate choices:
Why
Paging itself is still needed. The list is virtualized, but that only limits what is drawn; the host still has to send history in bounded pages, because a whole long transcript can't cross an SSH or paired-host connection in one message. What was wrong was the trigger (too late, and able to stall), a button shown even when nothing was needed from the user, and a failure in one lane that took the whole conversation away. Observing a marker with a generous margin loads earlier and can't fire on measurement churn. Keeping failures to the list, and clearing them when the lane resets its paging, means a failure never hides your messages and never needs a click once the connection recovers.
Linked Issue
None. Reported directly while using the native chat.
Visual Proof
Before: the "Load earlier messages" button was always at the top whenever older history existed (the same button now appears only after a failure, shown in the third image).
After, running in a hidden dev app against a 1000-message transcript (captured before the failure-containment change; updated screenshots, including the structured-lane failure path, are in the QA comment below):
Loading status while a page is in flight (the page was requested 482px before the top):
True start of history: no status row, no button, first message on top:
After a failed load, the retry button appears (clicking it loaded the page and automatic loading resumed):
Testing
Ran on macOS in a hidden dev app with a synthetic 1000-message Claude transcript in the transcript chat lane, with both
scrollTopsteps and real mouse-wheel input: every page was requested before reaching the top, the button never appeared while loading worked, the reader's visible row moved 0px across every prepend including the final page, and paging stopped at the first message. Renaming the transcript mid-scroll made the retry button appear with the messages still shown; restoring it and clicking retry resumed automatic loading.NativeChatMessageList.older-history.test.tsx(15 tests): loads when the marker is in range; keeps paging while in range, including after a page of hidden rows; stops when out of range or out of history; stops observing while a page loads; does not strand when the lane drops a page; a hidden pane doesn't observe; delayed status label; a failed page shows the retry and a retry re-enables; a page with no progress stops and offers the retry; a new paging generation resumes automatically with no click; no top row while the read isn't ready, and a re-check once it is; the reader's position holds across a prepend and when the last page removes the row.Lane tests: a structured older page that fails (for any reason) reports
failedand leaves the conversation, with no pane error. A structured snapshot starts a new paging generation, as does a transcript snapshot. The structured session hands the list the page result and the generation.Each new test was checked to fail with its fix removed.
pnpm tc, the full native-chat test folder (183 files, 1851 tests), oxlint on every changed file,check:code-quality:changedandcheck:react-doctor:changedpass.Not tested: SSH or paired remote hosts (renderer-only change; the same page reads are used unchanged), Linux, or Windows.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Known and left alone:
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
IntersectionObserveris available in every Electron renderer. If it is missing, the list falls back to the button.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)