Skip to content

fix(conversation): stop scroll-up blank gaps and anchor teleports #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
RonenMars merged 1 commit into from
Aug 1, 2026
Merged

fix(conversation): stop scroll-up blank gaps and anchor teleports #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 41 additions & 8 deletions __tests__/unit/hooks/useConversations.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ describe('useConversation — { resume } delta on the tail view', () => {
// Tail first page: messages 0..2, total 3. No has_more_newer field (plain tail).
metaHandlers.srv_resume = () =>
Promise.resolve({ status: 200, etag: '"v1"', body: rawConversationPage('c_r', ['a', 'b', 'c']) })
// after_index=2 delta: two new messages 3,4 out of total 5, no more newer.
// after_index=3 delta: two new messages 3,4 out of total 5, no more newer.
handlers.srv_resume = (path) => {
paths.push(path)
return Promise.resolve(
Expand All @@ -422,13 +422,41 @@ describe('useConversation — { resume } delta on the tail view', () => {
await result.current.fetchNewerPage()
await waitFor(() => expect(result.current.data!.messages.length).toBe(5))

expect(paths.some((p) => p.includes('after_index=2'))).toBe(true)
// The server window is [after_index, …) INCLUSIVE, so resuming from held
// max index 2 must request after_index=3 — asking with 2 re-downloads the
// tail message and appends a duplicate-id row on every delta poll.
expect(paths.some((p) => p.includes('after_index=3'))).toBe(true)
expect(paths.some((p) => p.includes('after_index=2'))).toBe(false)
expect(paths.some((p) => p.includes('msg_limit=80'))).toBe(true)
// Delta path must NOT send If-None-Match (it's a plain get, not getWithMeta).
const indexes = result.current.data!.messages.map((m) => m.messageIndex)
expect(indexes).toEqual([0, 1, 2, 3, 4])
})

it('drops duplicate message indexes when a delta page overlaps already-held rows', async () => {
setActiveServers(['srv_dup'])
// Tail first page: messages 0..2.
metaHandlers.srv_dup = () =>
Promise.resolve({ status: 200, etag: '"v1"', body: rawConversationPage('c_d', ['a', 'b', 'c']) })
// Overlapping delta: the server re-sends index 2 alongside the new index 3
// (inclusive-window behavior). The flatten must keep exactly one row per
// message_index — duplicate ids break FlashList's keyExtractor and its
// maintainVisibleContentPosition anchor (phantom blank space + scroll jumps).
handlers.srv_dup = () =>
Promise.resolve(rawAnchoredPage('c_d', 2, 2, 4, { has_more_newer: false, next_after_index: null }))

const { result } = await renderHook(() => useConversation('srv_dup', 'c_d'), { wrapper: createWrapper() })
await waitFor(() => expect(result.current.data!.messages.length).toBe(3))

await result.current.fetchNewerPage()
await waitFor(() => expect(result.current.data!.messages.length).toBe(4))

const indexes = result.current.data!.messages.map((m) => m.messageIndex)
expect(indexes).toEqual([0, 1, 2, 3])
const ids = result.current.data!.messages.map((m) => m.id)
expect(new Set(ids).size).toBe(ids.length)
})

it('does not expose a resume cursor when no messages are cached (fresh install)', async () => {
setActiveServers(['srv_fresh'])
metaHandlers.srv_fresh = () =>
Expand Down Expand Up @@ -796,7 +824,9 @@ describe('useConversation — consolidated delta trigger', () => {

const { result } = await renderHook(() => useConversation('srv_mt', 'c_mt'), { wrapper })

await waitFor(() => expect(paths.filter((p) => p.includes('after_index=2'))).toHaveLength(1))
// Warm cache holds 0..2, and the server window is inclusive of after_index,
// so resuming asks for the first index we do NOT have: 3.
await waitFor(() => expect(paths.filter((p) => p.includes('after_index=3'))).toHaveLength(1))
await waitFor(() => expect(result.current.data!.messages.length).toBe(4))
// Only the delta fired, never a tail (-1) fetch.
expect(paths.every((p) => p.includes('after_index'))).toBe(true)
Expand All @@ -805,10 +835,12 @@ describe('useConversation — consolidated delta trigger', () => {
it('drains a >80-message backlog across sequential after_index pages, guard stamped once', async () => {
setActiveServers(['srv_drain'])
const paths: string[] = []
// Warm cache ends at index 2 (cursor 2). Backlog: 3 pages.
// Warm cache holds 0..2, so the first hop asks for index 3 — the server
// window is [after_index, after_index + limit), inclusive of the cursor.
// Backlog: 3 pages.
handlers.srv_drain = (path) => {
paths.push(path)
if (path.includes('after_index=2')) {
if (path.includes('after_index=3')) {
// 80 new (3..82), more newer.
return Promise.resolve(rawAnchoredPage('c_dr', 3, 80, 243, { has_more_newer: true, next_after_index: 83 }))
}
Expand All @@ -828,7 +860,7 @@ describe('useConversation — consolidated delta trigger', () => {
// Exactly three sequential after_index GETs.
const afterPaths = paths.filter((p) => p.includes('after_index'))
expect(afterPaths).toHaveLength(3)
expect(afterPaths[0]).toContain('after_index=2')
expect(afterPaths[0]).toContain('after_index=3')
expect(afterPaths[1]).toContain('after_index=83')
expect(afterPaths[2]).toContain('after_index=163')
// Full range 0..242, no gap.
Expand Down Expand Up @@ -993,10 +1025,11 @@ describe('useConversation — drain etag (item 3)', () => {
it('(e) strips the mismatched hop and stops — no resetQueries, hop-1 kept, resumable', async () => {
setActiveServers(['srv_etag'])
const paths: string[] = []
// Warm cache ends at index 2 (cursor 2). Two-hop backlog, but hop 2's etag differs.
// Warm cache holds 0..2, so hop 1 asks for index 3 (the window is inclusive
// of after_index). Two-hop backlog, but hop 2's etag differs.
handlers.srv_etag = (path) => {
paths.push(path)
if (path.includes('after_index=2')) {
if (path.includes('after_index=3')) {
// hop 1: 80 new (3..82), more newer, etag "A".
return Promise.resolve(rawAnchoredPage('c_et', 3, 80, 243, { has_more_newer: true, next_after_index: 83, etag: '"A"' }))
}
Expand Down
124 changes: 81 additions & 43 deletions components/conversation/ConversationHistoryList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,27 +107,34 @@ export const ConversationHistoryList = forwardRef<FlashListRef<Message>, Convers
const animateIdsRef = useRef<Set<string>>(new Set())
const seededRef = useRef(false)
const [animateEpoch, setAnimateEpoch] = useState(0)
// Keyed on the messages array identity so the O(messages) walk runs only
// when data actually changes, not on every unrelated re-render (FAB state,
// theme). Ids can only be added on a data change, so this is behavior-
// preserving; the epoch bump stays a render-phase update, which is how the
// first render of a new tail row already carries animateIn=true.
/* eslint-disable react-hooks/refs -- render-time seen/animate id cache; see note above */
if (!seededRef.current) {
seededRef.current = true
animateIdsRef.current = new Set() // seed nothing → history renders silently
messages.forEach((m) => animateIdsRef.current.add(`seen:${m.id}`))
} else {
const set = animateIdsRef.current
const tailStart = Math.max(0, messages.length - ANIMATE_TAIL_WINDOW)
let grew = false
messages.forEach((m, i) => {
const seenKey = `seen:${m.id}`
if (!set.has(seenKey)) {
set.add(seenKey)
if (i >= tailStart) {
set.add(m.id)
grew = true
useMemo(() => {
if (!seededRef.current) {
seededRef.current = true
animateIdsRef.current = new Set() // seed nothing → history renders silently
messages.forEach((m) => animateIdsRef.current.add(`seen:${m.id}`))
} else {
const set = animateIdsRef.current
const tailStart = Math.max(0, messages.length - ANIMATE_TAIL_WINDOW)
let grew = false
messages.forEach((m, i) => {
const seenKey = `seen:${m.id}`
if (!set.has(seenKey)) {
set.add(seenKey)
if (i >= tailStart) {
set.add(m.id)
grew = true
}
}
}
})
if (grew) setAnimateEpoch((e) => e + 1)
}
})
if (grew) setAnimateEpoch((e) => e + 1)
}
}, [messages])
const animateIds = animateIdsRef.current
/* eslint-enable react-hooks/refs */

Expand All @@ -146,17 +153,26 @@ export const ConversationHistoryList = forwardRef<FlashListRef<Message>, Convers
[lastMessageId, highlight, highlightIndex, onMatchLayout, animateEpoch],
)

// Distinguish row shapes so FlashList only recycles cells of the same kind;
// without this a recycled tool-card cell can bleed under a text row.
// Item type drives two FlashList v2 mechanisms: the recycling pool AND the
// per-type running-average height used to place rows that haven't been
// measured yet. Real conversations span ~46pt (collapsed Reasoning header)
// to ~3,100pt (markdown-table answers), so lumping every thinking/tool/diff
// row into one 'tool' pool poisons that average — measured as ±10-20k pt
// content-size swings that shove the mVCP anchor around while scrolling up
// (the blank-gap / viewport-teleport bug). Split by the row's dominant
// shape so each pool's average tracks rows that actually look alike.
const getItemType = useCallback((item: Message) => {
const hasToolOrDiff = item.content.some(
(b) =>
b.type === 'thinking' ||
b.type === 'tool_use' ||
b.type === 'tool_result' ||
b.type === 'diff',
)
if (hasToolOrDiff) return 'tool'
let hasThinking = false
let hasTool = false
let hasDiff = false
for (const b of item.content) {
if (b.type === 'thinking') hasThinking = true
else if (b.type === 'tool_use' || b.type === 'tool_result') hasTool = true
else if (b.type === 'diff') hasDiff = true
}
if (hasDiff) return 'diff'
if (hasTool) return 'tool'
if (hasThinking) return 'thinking'
return item.role === 'user' ? 'user' : 'assistant'
}, [])

Expand All @@ -170,6 +186,30 @@ export const ConversationHistoryList = forwardRef<FlashListRef<Message>, Convers
[disableAutoAnchor],
)

// FlashList re-lays-out the header/footer whenever the element identity
// changes, and an inline conditional recreates it on every parent render —
// during an onStartReached page fetch that churn feeds extra mVCP anchor
// corrections (upstream guidance: keep these stable, Shopify/flash-list#1844).
// Memoized so identity only changes when the fetching state itself flips.
const listHeader = useMemo(
() =>
isFetchingOlder ? (
<View style={styles.pageLoading}>
<ActivityIndicator color={theme.text.secondary} />
</View>
) : null,
[isFetchingOlder, styles, theme],
)
const listFooter = useMemo(
() =>
isFetchingNewer ? (
<View style={styles.pageLoading}>
<ActivityIndicator color={theme.text.secondary} />
</View>
) : null,
[isFetchingNewer, styles, theme],
)

// Only drives the two scroll FABs — no pagination or scroll logic here.
const handleScroll = useCallback((e: NativeSyntheticEvent<NativeScrollEvent>) => {
const { contentOffset, contentSize, layoutMeasurement } = e.nativeEvent
Expand All @@ -194,6 +234,16 @@ export const ConversationHistoryList = forwardRef<FlashListRef<Message>, Convers
keyExtractor={(m) => m.id}
renderItem={renderItem}
getItemType={getItemType}
// drawDistance is PIXELS of pre-rendered runway, not rows, and the
// iOS default is 250 — split 70/30 toward the scroll direction, so
// scrolling up pre-renders only ~350px above the viewport and evicts
// rows ~150px below it. A single table answer here measures ~3,100px
// (≈5 viewports), so the default buffer is outrun by any real flick
// and just-passed rows unmount into visible blanks. 2000px keeps the
// engaged window ahead of momentum scrolling and clears the known-bad
// "item taller than 2×drawDistance" mVCP-correction regime
// (Shopify/flash-list#2136) for rows up to 4,000px.
drawDistance={2000}
contentContainerStyle={contentContainerStyle}
maintainVisibleContentPosition={maintainVisibleContentPosition}
onLoad={onReady}
Expand All @@ -203,20 +253,8 @@ export const ConversationHistoryList = forwardRef<FlashListRef<Message>, Convers
onStartReachedThreshold={0.3}
onEndReached={onEndReached}
onEndReachedThreshold={0.3}
ListHeaderComponent={
isFetchingOlder ? (
<View style={styles.pageLoading}>
<ActivityIndicator color={theme.text.secondary} />
</View>
) : null
}
ListFooterComponent={
isFetchingNewer ? (
<View style={styles.pageLoading}>
<ActivityIndicator color={theme.text.secondary} />
</View>
) : null
}
ListHeaderComponent={listHeader}
ListFooterComponent={listFooter}
/>
{showScrollTop ? (
<TouchableOpacity
Expand Down
Loading
Loading