Repository navigation
feat(mobile): load earlier chat messages on scroll-up, keeping the reader's place - #31339
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 59.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 37 functions across 27 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main change: automatic loading of earlier mobile chat messages while preserving the reader’s position during upward scrolling. Description check The description includes all required sections, explains the user-facing change and implementation, links issue #31177, provides visual proof, documents testing and performance results, and addresses …Linked Issues check Issue #31177requires automatic loading of older mobile chat messages while scrolling upward and a fallback after load failure.MobileNativeChatPageListrequests older pages before the drawn history…Out of Scope Changes check The changes stay within issue #31177. The reversed page list, automatic paging, retry state, reader-position preservation, deferred below-reader updates, and related tests support mobile history revie…Full details: Description check
-
Explanation
The description includes all required sections, explains the user-facing change and implementation, links issue
#31177, provides visual proof, documents testing and performance results, and addresses platform scope and compatibility. The unchecked small-and-focused checklist item is non-critical.
Full details: Linked Issues check
-
Explanation
Issue
#31177requires automatic loading of older mobile chat messages while scrolling upward and a fallback after load failure.MobileNativeChatPageListrequests older pages before the drawn history ends throughonNearTop.MobileNativeChatViewconnects this callback to guarded loading.mobile-native-chat-list-headerrenders retry UI after failure. Updated history-paging tests cover near-top loading, short initial content, and retry behavior.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#31177. The reversed page list, automatic paging, retry state, reader-position preservation, deferred below-reader updates, and related tests support mobile history review. The changes also remove obsolete windowing and measurement code used by the replaced behavior. The nativeFlatListpath, desktop behavior, wire protocol, and unrelated contextual issues remain outside the change.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e334f23c-aa2f-449c-8e43-b7f15a6c5697
📒 Files selected for processing: 33
mobile/src/session/MobileNativeChatMessage.tsxmobile/src/session/MobileNativeChatPageList.tsxmobile/src/session/MobileNativeChatPageScroll.tsxmobile/src/session/MobileNativeChatReasoningRow.tsxmobile/src/session/MobileNativeChatRowList.tsxmobile/src/session/MobileNativeChatScrollHold.tsxmobile/src/session/MobileNativeChatScrollRootsBoundary.tsxmobile/src/session/MobileNativeChatToolLine.tsxmobile/src/session/MobileNativeChatToolRun.tsxmobile/src/session/MobileNativeChatView.harness.test-fixture.tsmobile/src/session/MobileNativeChatView.history-paging.test.tsmobile/src/session/MobileNativeChatView.render-stubs.test-fixture.tsmobile/src/session/MobileNativeChatView.tsxmobile/src/session/mobile-native-chat-list-header.tsxmobile/src/session/mobile-native-chat-page-heights.test.tsmobile/src/session/mobile-native-chat-page-heights.tsmobile/src/session/mobile-native-chat-page-rows.tsmobile/src/session/mobile-native-chat-page-window-coverage.tsmobile/src/session/mobile-native-chat-page-window-state.tsmobile/src/session/mobile-native-chat-reading-place.test.tsxmobile/src/session/mobile-native-chat-reading-place.tsmobile/src/session/mobile-native-chat-scroll-anchoring.tsmobile/src/session/mobile-native-chat-scroll-motion.tsmobile/src/session/mobile-native-chat-scroll-ownership.test.tsxmobile/src/session/mobile-native-chat-scroll-root-handle.tsmobile/src/session/mobile-native-chat-scroll-roots.tsmobile/src/session/mobile-native-chat-view-styles.tsmobile/src/session/use-mobile-native-chat-page-render.tsmobile/src/session/use-mobile-native-chat-page-window.tsmobile/src/session/use-mobile-native-chat-session.test.tsmobile/src/session/use-mobile-native-chat-session.tsmobile/src/session/use-mobile-native-chat-tail-follow.tsmobile/src/session/use-mobile-structured-agent-state.ts
💤 Files with no reviewable changes: 9
- mobile/src/session/mobile-native-chat-page-heights.test.ts
- mobile/src/session/use-mobile-native-chat-page-window.ts
- mobile/src/session/use-mobile-native-chat-page-render.ts
- mobile/src/session/use-mobile-native-chat-session.test.ts
- mobile/src/session/mobile-native-chat-page-window-coverage.ts
- mobile/src/session/mobile-native-chat-page-heights.ts
- mobile/src/session/MobileNativeChatScrollHold.tsx
- mobile/src/session/mobile-native-chat-page-rows.ts
- mobile/src/session/mobile-native-chat-page-window-state.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| const drawAndFetch = useCallback( | ||
| (s: Scroller) => { | ||
| const { first: at, keys: drawnKeys, n: total, rowKeys: current } = latest.current | ||
| const screens = (s.scrollHeight - s.clientHeight + s.scrollTop) / s.clientHeight | ||
| if (at > 0 && screens < DRAW_AHEAD_SCREENS) { | ||
| const next = Math.max(0, at - DRAW_BATCH) | ||
| setDrawn({ scope: current.scope, firstKey: drawnKeys[next]!, count: total - next }) | ||
| } else if (isFollowing() && at < total - FIRST_ROWS && screens > 3 * DRAW_AHEAD_SCREENS) { | ||
| // Back at the end, rows far above go: taking them away moves nothing. | ||
| setDrawn({ scope: current.scope, firstKey: null, count: 0 }) | ||
| } | ||
| if (at < FETCH_AHEAD_ROWS && screens < FETCH_REACH_SCREENS) { | ||
| ask.current?.() | ||
| } | ||
| }, | ||
| [isFollowing] | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The draw/trim rule in drawAndFetch can loop with no end and crash the page list.
drawAndFetch runs in the useLayoutEffect after every commit (Line 257-263), and each setDrawn starts a new commit. The draw branch runs when screens < DRAW_AHEAD_SCREENS (3). The trim branch runs when the reader is following and screens > 3 * DRAW_AHEAD_SCREENS (9). One DRAW_BATCH of 12 rows can add more than 6 screens of content, for example with Tools expanded and several Edit diff cards. Under that condition the cycle is:
- The trim keeps 24 rows, and
screensdrops below 3. - The next commit draws 12 more rows, and
screensrises above 9. - The next commit trims again.
Every step is a setState inside a layout effect. React stops after its nested-update limit with "Maximum update depth exceeded", so the chat view fails. The ResizeObserver callback (Line 272-276) can also drive the same cycle.
Do not trim on commit or resize. Trim only when the reader returns to the end in a scroll event. The draw-ahead rule then stops at 3 screens, and a commit cannot restart the trim.
🐛 Proposed fix: trim only from a scroll event
const drawAndFetch = useCallback(
- (s: Scroller) => {
+ (s: Scroller, mayTrim = false) => {
const { first: at, keys: drawnKeys, n: total, rowKeys: current } = latest.current
const screens = (s.scrollHeight - s.clientHeight + s.scrollTop) / s.clientHeight
if (at > 0 && screens < DRAW_AHEAD_SCREENS) {
const next = Math.max(0, at - DRAW_BATCH)
setDrawn({ scope: current.scope, firstKey: drawnKeys[next]!, count: total - next })
- } else if (isFollowing() && at < total - FIRST_ROWS && screens > 3 * DRAW_AHEAD_SCREENS) {
+ } else if (
+ mayTrim &&
+ isFollowing() &&
+ at < total - FIRST_ROWS &&
+ screens > 3 * DRAW_AHEAD_SCREENS
+ ) {Then, in scrolled:
- drawAndFetch(s)
+ drawAndFetch(s, true)| // Older runtimes ignore the cursor and return the growing tail; rows that streamed in while | ||
| // it waited stay. | ||
| const live = mergerRef.current.list | ||
| const after = live.findIndex((message) => message.id === result.messages.at(-1)?.id) | ||
| setList(after === -1 ? result.messages : [...result.messages, ...live.slice(after + 1)]) | ||
| more = result.messages.length >= nextLimit |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
On older hosts, a cursorless page keeps its own stale copy of the joined row.
The page from an older runtime can end at a row that is still streaming. At Line 302, the page's copy of that row (result.messages.at(-1)) replaces the live copy. Only rows after it come from live. Live frames can update that row while the page read is in flight. The newer text is then replaced by the older snapshot. If the turn has already ended, no later frame corrects it, and the message stays truncated until the chat reloads. The removed mergeNativeChatMessages reconciliation prevented this.
There is a second problem on the same line. If result.messages is empty, after is -1 and setList([]) clears every row the reader holds.
From the joined row on, keep the live copies. If the page is empty, keep the live list.
🐛 Proposed fix
const live = mergerRef.current.list
const after = live.findIndex((message) => message.id === result.messages.at(-1)?.id)
- setList(after === -1 ? result.messages : [...result.messages, ...live.slice(after + 1)])
+ if (result.messages.length > 0) {
+ setList(
+ after === -1
+ ? result.messages
+ : [...result.messages.slice(0, -1), ...live.slice(after)]
+ )
+ }
more = result.messages.length >= nextLimit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Older runtimes ignore the cursor and return the growing tail; rows that streamed in while | |
| // it waited stay. | |
| const live = mergerRef.current.list | |
| const after = live.findIndex((message) => message.id === result.messages.at(-1)?.id) | |
| setList(after === -1 ? result.messages : [...result.messages, ...live.slice(after + 1)]) | |
| more = result.messages.length >= nextLimit | |
| // Older runtimes ignore the cursor and return the growing tail; rows that streamed in while | |
| // it waited stay. | |
| const live = mergerRef.current.list | |
| const after = live.findIndex((message) => message.id === result.messages.at(-1)?.id) | |
| if (result.messages.length > 0) { | |
| setList( | |
| after === -1 | |
| ? result.messages | |
| : [...result.messages.slice(0, -1), ...live.slice(after)] | |
| ) | |
| } | |
| more = result.messages.length >= nextLimit |
9bb0866 to
8c299fc
Compare
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:
47da29da-2d5e-47e9-b904-5d12cf54cf03
📒 Files selected for processing: 8
mobile/src/session/MobileNativeChatPageList.tsxmobile/src/session/MobileNativeChatToolLine.tsxmobile/src/session/MobileNativeChatToolRun.tsxmobile/src/session/MobileNativeChatView.tsxmobile/src/session/mobile-native-chat-list-header.tsxmobile/src/session/mobile-native-chat-reading-place.test.tsxmobile/src/session/mobile-native-chat-scroll-ownership.test.tsxmobile/src/session/mobile-native-chat-view-styles.ts
🚧 Files skipped from review as they are similar to previous changes: 2
- mobile/src/session/MobileNativeChatToolLine.tsx
- mobile/src/session/MobileNativeChatToolRun.tsx
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| return ( | ||
| <Pressable style={styles.loadEarlier} onPress={loadEarlier} disabled={loadingEarlier}> | ||
| {loadingEarlier ? ( | ||
| <ActivityIndicator size="small" color={colors.textMuted} /> | ||
| ) : ( | ||
| <Text style={styles.loadEarlierText}>Load earlier messages</Text> | ||
| )} | ||
| {olderPages === 'failed' ? ( | ||
| <Text style={styles.loadEarlierFailedText}>{LOAD_EARLIER_FAILED}</Text> | ||
| ) : null} | ||
| </Pressable> | ||
| <View style={styles.loadEarlier}> | ||
| <FadeIn key="loading" delay={SPINNER_DELAY_MS}> | ||
| <ActivityIndicator | ||
| size="small" | ||
| color={colors.textMuted} | ||
| accessibilityLabel={LOADING_EARLIER} | ||
| /> | ||
| </FadeIn> | ||
| </View> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'scrollMayLoadEarlier|onLoadEarlier|loadEarlier\b' mobile/src/session --type=ts --type=tsx -g '!**/*.test.*' 2>/dev/null || rg -nP -C4 'scrollMayLoadEarlier|onLoadEarlier|loadEarlier\b' mobile/src/session -g '!**/*.test.*'
rg -nP -C3 'onContentSizeChange|pinToTailAfterContentResize' mobile/src/session/use-mobile-native-chat-tail-follow.tsRepository: stablyai/orca
Length of output: 22841
Add a native fallback for short lists.
When hasMore is true and the native list content does not fill the viewport, the idle header only renders an ActivityIndicator. The native branch starts loading only from onScroll when contentOffset.y < 60 and scrollMayLoadEarlier() is true. A short Android list may emit no scroll event, so the reader cannot load earlier messages or retry from the idle state.
Suggested fix: add a native idle fallback
+ if (Platform.OS !== 'web' && olderPages === 'idle') {
+ return (
+ <View style={styles.loadEarlier}>
+ <Pressable accessibilityRole="button" onPress={loadEarlier}>
+ <Text style={styles.loadEarlierRetryLabel}>Load earlier messages</Text>
+ </Pressable>
+ </View>
+ )
+ }
return (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <Pressable style={styles.loadEarlier} onPress={loadEarlier} disabled={loadingEarlier}> | |
| {loadingEarlier ? ( | |
| <ActivityIndicator size="small" color={colors.textMuted} /> | |
| ) : ( | |
| <Text style={styles.loadEarlierText}>Load earlier messages</Text> | |
| )} | |
| {olderPages === 'failed' ? ( | |
| <Text style={styles.loadEarlierFailedText}>{LOAD_EARLIER_FAILED}</Text> | |
| ) : null} | |
| </Pressable> | |
| <View style={styles.loadEarlier}> | |
| <FadeIn key="loading" delay={SPINNER_DELAY_MS}> | |
| <ActivityIndicator | |
| size="small" | |
| color={colors.textMuted} | |
| accessibilityLabel={LOADING_EARLIER} | |
| /> | |
| </FadeIn> | |
| </View> | |
| if (Platform.OS !== 'web' && olderPages === 'idle') { | |
| return ( | |
| <View style={styles.loadEarlier}> | |
| <Pressable accessibilityRole="button" onPress={loadEarlier}> | |
| <Text style={styles.loadEarlierRetryLabel}>Load earlier messages</Text> | |
| </Pressable> | |
| </View> | |
| ) | |
| } | |
| return ( | |
| <View style={styles.loadEarlier}> | |
| <FadeIn key="loading" delay={SPINNER_DELAY_MS}> | |
| <ActivityIndicator | |
| size="small" | |
| color={colors.textMuted} | |
| accessibilityLabel={LOADING_EARLIER} | |
| /> | |
| </FadeIn> | |
| </View> |
The phone page's chat list is anchored at the newest message, so older history lands above the reader without moving them, mid-fling included. Fixes #31177
…der moves The reveal grows its row on its own frames, outside the list's hold, so each new line pushed the reader mid-fling and through the long iOS coast after it.
…n rows A held row drawn through the live renderer still took a run's new members, and trimming back to the newest rows could undo a draw on every commit.
…art mid-scroll The held rows began inside a run whose head the older page drew live, so the held head drew the run's members a second time until the reader rested.
In RN's column flexbox every streamed token re-laid each drawn row; with 500 rows drawn deep in history that was a 20 ms layout per frame on a throttled phone-class CPU. Block flow draws the same geometry for a fraction of it.
8c299fc to
dd10762
Compare
ELI5
On the phone, reading back through a long chat meant scrolling to the top, tapping "Load earlier messages", then scrolling and tapping again. Now older messages load by themselves while you scroll up, even in the middle of a flick, and the text you are reading stays still.
What Changed
Before, older history on the phone page loaded only from the "Load earlier messages" tap. Even after a tap, scrolling up stopped at a spinner every few rows, because the page added rows only while the scroll was completely still.
Now:
flex-direction: column-reverse). Rows added at the top move nothing, so landing older history needs no scroll correction, and following a streaming reply needs no pinning.This removes the page list's windowing, height cache and measuring rounds, the place-hold component, and the step where both chat lanes waited for the reader to stop before landing an older page. The scroll boxes inside rows (tool output, diffs, reasoning) keep their behaviour behind a small commit boundary.
Why
On iPhone the page cannot correct the scroll position mid-flick. iOS 26 and earlier stop the flick on the write, and iOS 27 overrides it on the next frame; I measured both in the simulators. Safari 27's own scroll anchoring has the same problem mid-flick. That is why the page waited for rest before adding older rows, and why loading needed a tap.
Chat list libraries hit the same limit. Legend List's web build corrects with
scrollBy, and its open issue #488 reports that this kills flicks in WKWebView. TanStack Virtual and virtua hold their corrections until the flick ends, which brings back the stop at the top. Anchoring the list at its end removes the correction for the common case, reading back through history, on every engine. What remains is a change below a parked reader, which the at-rest rule handles.Linked Issue
Fixes #31177
Visual Proof
A fake 400-message chat in iOS 27 simulator Safari, with the same scripted flicks in both.
Before (main): the flicks reach the top and nothing loads until the tap. After the tap, one page lands and the next flicks stop at the top again.
before-main-small.mp4
After: the same flicks never reach the top while older pages load mid-flick.
after-branch-small.mp4
Testing
I tested in the iOS 26 and iOS 27 simulators (Safari) and Android Chrome on the emulator, with a harness that renders the real chat view over fake paging. The cases were non-stop flicks, flicks with reading pauses, a reply streaming while scrolled up, opening a tool row and its output box, jump to latest, and a failed load with retry. Frame-by-frame recordings showed no jumps when pages landed and no snap-backs while a reply streamed.
I also compared main with this branch on a fake 2,000-message agent chat, with the same harness bundled from each tree, in Chromium at 4× CPU throttling (median of three runs):
The rows are laid out in block flow, not React Native's column flexbox, which cut that last case's layout time from 2.3 s to 0.3 s. Unthrottled WebKit is smooth on both.
The page list tests are rewritten for the new behaviour, and the reading-place tests now run against the new list.
AI Disclosure
Review
MobileNativeChatPageList.tsxholds the new rules: drawing upward, asking for older pages, keeping the reader's line at rest, and holding back changes below a moving reader. The native FlatList branch is unchanged; since #29969 only tests reach it. The scroll boxes inside rows still decide by browser name whether to correct themselves, and their lists are short.Agent skill upstream boundary
Notes
Phone page only. No wire change, so older hosts and the desktop are unaffected. The same page code runs for SSH and local hosts.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)