From 3a931e3c810de9f02c40e51fd4258e2682154f36 Mon Sep 17 00:00:00 2001 From: Haoqing Wang <78337154+hqhq1025@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:24:01 +0800 Subject: [PATCH] fix(web): count unseen messages by rendered block, not raw message (#1255) * fix(web): count unseen messages by rendered block, not raw message The "N new messages" pill counted raw DecryptedMessages while the timeline renders folded blocks, so the two never agreed. A subagent run is dozens of sidechain messages but a single Task card; a tool_use and its tool_result are two messages and one card; consecutive tools collapse into one group. The pill could read "47 new messages" when scrolling down revealed two new rows. collectNewUnseenIds never inspected isSidechain, and it could not: the reducer's grouping is stateful (it needs the Task tool_use before it can map parentToolUseId), so a per-message predicate in the store cannot reproduce it. Adding an isSidechain check there would also invert the error for orphan sidechain messages, which tracer.ts falls back to emitting at the top level. Instead, drop the store's unseen bookkeeping entirely and count what the renderer actually produced. Watermark the visible blocks when the user scrolls away from the tail, then count the blocks past the last one they had seen. The count is anchor-based rather than timestamp-based because the blocks array is not monotonic in createdAt: messages sort by invokedAt ?? createdAt, so a queued message carries an old createdAt while sitting at the end. Anchoring also makes prepended history free, since older blocks land before the anchor. Known limit, documented at the call site: once the history window fills up, mergeIntoWindow trims incoming messages off the tail, so the pill reports 0 instead of a count. Under-reporting is preferable here, and returning to the tail force-refetches the latest page anyway. * fix(web): keep unseen watermark stable across optimistic id replacement The watermark snapshotted only block.id, but that id is not stable for the user's own messages: mergeMessages replaces an optimistic row with a stored row that keeps localId under a new server id, and the user block renders with the message id. Scrolling into history while an own message was still optimistic meant its echo anchored one block earlier and bumped the pill by one, with no new rendered row. Track localId alongside id in the watermark and match on either. Reported by HAPI Bot on #1255. * fix(web): count joined assistant cards, not pre-join blocks visibleBlocks is still not one-to-one with rendered rows: assistant-ui joins a run of adjacent assistant-role blocks into a single card, so a response made of reasoning + text + a tool call was reported as three new messages instead of one, and appending another block to an in-flight response bumped the pill without adding a row. Walk the blocks after the anchor and only start a new row where the assistant run breaks. Role assignment is the part that would drift, so rather than restating it, visibleBlockRole moves from assistant-runtime.ts to toolGroups.ts (next to the VisibleChatBlock definition it describes) and both the runtime and the counter import the one copy. Reported by HAPI Bot on #1255. --- web/e2e-fixtures/history-load-fixture.tsx | 5 +- web/src/chat/toolGroups.ts | 14 ++ web/src/chat/unseenBlocks.test.ts | 257 ++++++++++++++++++++++ web/src/chat/unseenBlocks.ts | 106 +++++++++ web/src/components/SessionChat.tsx | 10 +- web/src/hooks/queries/useMessages.ts | 5 +- web/src/hooks/useUnseenBlockCount.test.ts | 111 ++++++++++ web/src/hooks/useUnseenBlockCount.ts | 34 +++ web/src/lib/assistant-runtime.ts | 15 +- web/src/lib/message-window-store.test.ts | 8 +- web/src/lib/message-window-store.ts | 52 +---- web/src/router.tsx | 4 +- 12 files changed, 543 insertions(+), 78 deletions(-) create mode 100644 web/src/chat/unseenBlocks.test.ts create mode 100644 web/src/chat/unseenBlocks.ts create mode 100644 web/src/hooks/useUnseenBlockCount.test.ts create mode 100644 web/src/hooks/useUnseenBlockCount.ts diff --git a/web/e2e-fixtures/history-load-fixture.tsx b/web/e2e-fixtures/history-load-fixture.tsx index ec60931c..66f1bc2e 100644 --- a/web/e2e-fixtures/history-load-fixture.tsx +++ b/web/e2e-fixtures/history-load-fixture.tsx @@ -151,7 +151,6 @@ function FixtureThread() { isSyncingTail, isLoadingMore, hasMore, - unseenCount, messagesVersion, historyVersion, loadMore, @@ -206,7 +205,9 @@ function FixtureThread() { hasMoreMessages={hasMore} isLoadingMoreMessages={isLoadingMore} onLoadMore={loadMore} - unseenCount={unseenCount} + // This fixture drives HappyThread directly, bypassing the + // SessionChat block reduction that computes the real count. + unseenCount={0} rawMessagesCount={messages.length} normalizedMessagesCount={normalizedMessages.length} messagesVersion={messagesVersion} diff --git a/web/src/chat/toolGroups.ts b/web/src/chat/toolGroups.ts index 20a42c3c..905d613d 100644 --- a/web/src/chat/toolGroups.ts +++ b/web/src/chat/toolGroups.ts @@ -38,6 +38,20 @@ export type ToolGroupBlock = { export type VisibleChatBlock = ChatBlock | ToolGroupBlock +export type VisibleChatBlockRole = 'user' | 'assistant' | 'system' + +/** + * The role a block renders under in the thread. `@assistant-ui/react` joins + * adjacent assistant-role blocks into a single card, so this also determines + * how many rows a run of blocks actually produces on screen. + */ +export function visibleBlockRole(block: VisibleChatBlock): VisibleChatBlockRole { + if (block.kind === 'user-text') return 'user' + if (block.kind === 'agent-event') return 'system' + if (block.kind === 'cli-output') return block.source === 'user' ? 'user' : 'assistant' + return 'assistant' +} + type ToolGroupingOptions = { hasMoreMessages: boolean previousGroups?: ToolGroupBlock[] diff --git a/web/src/chat/unseenBlocks.test.ts b/web/src/chat/unseenBlocks.test.ts new file mode 100644 index 00000000..e0549dff --- /dev/null +++ b/web/src/chat/unseenBlocks.test.ts @@ -0,0 +1,257 @@ +/** + * Tests for the "N new messages" counter. The regression these guard against: + * the count used to be computed over raw messages, so a subagent run (dozens of + * sidechain messages folded into one Task card) and every tool_result inflated + * it far beyond what the user would actually see on screen. + */ +import { describe, expect, it } from 'vitest' +import type { NormalizedMessage } from '@/chat/types' +import { reduceChatBlocks } from '@/chat/reducer' +import { buildVisibleChatBlocks, type VisibleChatBlock } from '@/chat/toolGroups' +import { countUnseenBlocks, createUnseenWatermark } from '@/chat/unseenBlocks' + +const BASE_AT = 1_700_000_000_000 + +function userMsg(id: string, text: string, createdAt: number): NormalizedMessage { + return { + id, + localId: null, + createdAt, + role: 'user', + content: { type: 'text', text }, + isSidechain: false + } +} + +function agentText(id: string, text: string, createdAt: number): NormalizedMessage { + return { + id, + localId: null, + createdAt, + role: 'agent', + isSidechain: false, + content: [{ type: 'text', text, uuid: `uuid-${id}`, parentUUID: null }] + } as NormalizedMessage +} + +function agentReasoning(id: string, text: string, createdAt: number): NormalizedMessage { + return { + id, + localId: null, + createdAt, + role: 'agent', + isSidechain: false, + content: [{ type: 'reasoning', text, uuid: `uuid-${id}`, parentUUID: null }] + } as NormalizedMessage +} + +function toolCall(id: string, name: string, createdAt: number, input: unknown = {}): NormalizedMessage { + return { + id, + localId: null, + createdAt, + role: 'agent', + isSidechain: false, + content: [{ + type: 'tool-call', + id: `tc-${id}`, + name, + input, + description: null, + uuid: `uuid-${id}`, + parentUUID: null + }] + } as NormalizedMessage +} + +function toolResult(id: string, toolUseId: string, createdAt: number): NormalizedMessage { + return { + id, + localId: null, + createdAt, + role: 'agent', + isSidechain: false, + content: [{ + type: 'tool-result', + tool_use_id: toolUseId, + content: 'ok', + is_error: false, + uuid: `uuid-${id}`, + parentUUID: null + }] + } as NormalizedMessage +} + +/** A message produced inside a subagent run, grouped by parentToolUseId. */ +function sidechainMsg(id: string, parentToolUseId: string, createdAt: number): NormalizedMessage { + return { + id, + localId: null, + createdAt, + role: 'agent', + isSidechain: true, + parentToolUseId, + content: [{ type: 'text', text: `subagent step ${id}`, uuid: `uuid-${id}`, parentUUID: null }] + } as NormalizedMessage +} + +function visible(messages: NormalizedMessage[], hasMoreMessages = false): VisibleChatBlock[] { + const reduced = reduceChatBlocks(messages, null) + return buildVisibleChatBlocks(reduced.blocks, { hasMoreMessages }) +} + +describe('countUnseenBlocks', () => { + it('returns 0 without a watermark', () => { + const blocks = visible([userMsg('u1', 'hi', BASE_AT)]) + expect(countUnseenBlocks(blocks, null)).toBe(0) + }) + + it('returns 0 when nothing arrived after the watermark', () => { + const blocks = visible([userMsg('u1', 'hi', BASE_AT), agentText('a1', 'hello', BASE_AT + 1)]) + expect(countUnseenBlocks(blocks, createUnseenWatermark(blocks))).toBe(1 - 1) + }) + + it('counts a whole subagent run as the single Task card it renders as', () => { + const seed = [userMsg('u1', 'go', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + // One Task tool_use followed by 30 sidechain messages from the subagent. + const subagentRun: NormalizedMessage[] = [ + toolCall('task-1', 'Task', BASE_AT + 1, { prompt: 'explore', subagent_type: 'Explore' }), + ...Array.from({ length: 30 }, (_, index) => + sidechainMsg(`sc-${index}`, 'tc-task-1', BASE_AT + 2 + index)) + ] + + const blocks = visible([...seed, ...subagentRun]) + expect(countUnseenBlocks(blocks, watermark)).toBe(1) + + // Guard against a false pass: the 30 messages must actually be folded + // into the Task card, not silently dropped before reaching the reducer. + const taskCard = blocks.at(-1) + expect(taskCard?.kind).toBe('tool-call') + expect(taskCard?.kind === 'tool-call' && taskCard.children.length).toBe(30) + }) + + it('counts a run of grouped tools as one collapsed group', () => { + const seed = [userMsg('u1', 'go', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + const reads = Array.from({ length: 20 }, (_, index) => + toolCall(`read-${index}`, 'Read', BASE_AT + 1 + index, { file_path: `/tmp/${index}.ts` })) + + const blocks = visible([...seed, ...reads]) + expect(countUnseenBlocks(blocks, watermark)).toBe(1) + }) + + it('does not count a tool_result that completes an already-counted card', () => { + const seed = [userMsg('u1', 'go', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + const withCall = [...seed, toolCall('bash-1', 'Bash', BASE_AT + 1, { command: 'ls' })] + const beforeResult = countUnseenBlocks(visible(withCall), watermark) + + const withResult = [...withCall, toolResult('res-1', 'tc-bash-1', BASE_AT + 2)] + const afterResult = countUnseenBlocks(visible(withResult), watermark) + + expect(beforeResult).toBe(1) + expect(afterResult).toBe(1) + }) + + it('ignores older blocks prepended by history pagination', () => { + const seed = [userMsg('u2', 'second', BASE_AT + 10)] + const watermark = createUnseenWatermark(visible(seed)) + + const withNew = [...seed, agentText('a1', 'reply', BASE_AT + 20)] + expect(countUnseenBlocks(visible(withNew), watermark)).toBe(1) + + // loadMore prepends an older page; the count must not move. + const withOlder = [userMsg('u0', 'first', BASE_AT), ...withNew] + expect(countUnseenBlocks(visible(withOlder), watermark)).toBe(1) + }) + + it('does not jump when a lone tool card is absorbed into a group', () => { + const seed = [userMsg('u1', 'go', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + // A single eligible tool renders as a plain tool-call block (id = tool id). + const lone = [...seed, toolCall('read-0', 'Read', BASE_AT + 1, { file_path: '/a.ts' })] + expect(countUnseenBlocks(visible(lone), watermark)).toBe(1) + + // A second tool merges both into a group whose id is derived from the + // first tool — the watermark must still recognize the seed as the anchor. + const grouped = [...lone, toolCall('read-1', 'Read', BASE_AT + 2, { file_path: '/b.ts' })] + expect(countUnseenBlocks(visible(grouped), watermark)).toBe(1) + }) + + it('reports 0 when every seen block has been trimmed out of the window', () => { + const seed = [userMsg('u1', 'old', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + // The window scrolled past everything the watermark knew about. + const replaced = visible([agentText('a9', 'much later', BASE_AT + 999)]) + expect(countUnseenBlocks(replaced, watermark)).toBe(0) + }) + + it('counts alternating user and assistant turns as separate rows', () => { + const seed = [userMsg('u1', 'go', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + const blocks = visible([ + ...seed, + agentText('a1', 'first', BASE_AT + 1), + userMsg('u2', 'again', BASE_AT + 2), + agentText('a2', 'second', BASE_AT + 3) + ]) + expect(countUnseenBlocks(blocks, watermark)).toBe(3) + }) + + it('counts one response of reasoning + text + tool as a single row', () => { + // assistant-ui joins adjacent assistant-role blocks into one card, so a + // multi-part response must not read as several new messages. + const seed = [userMsg('u1', 'go', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + const blocks = visible([ + ...seed, + agentReasoning('r1', 'thinking', BASE_AT + 1), + agentText('a1', 'here is the answer', BASE_AT + 2), + toolCall('bash-1', 'Bash', BASE_AT + 3, { command: 'ls' }) + ]) + + // Four blocks in the window... + expect(blocks).toHaveLength(4) + // ...but only one new card below the anchor. + expect(countUnseenBlocks(blocks, watermark)).toBe(1) + }) + + it('does not bump the count when a response grows another assistant block', () => { + const seed = [userMsg('u1', 'go', BASE_AT)] + const watermark = createUnseenWatermark(visible(seed)) + + const started = visible([...seed, agentText('a1', 'partial', BASE_AT + 1)]) + expect(countUnseenBlocks(started, watermark)).toBe(1) + + const continued = visible([ + ...seed, + agentText('a1', 'partial', BASE_AT + 1), + toolCall('bash-1', 'Bash', BASE_AT + 2, { command: 'ls' }) + ]) + expect(countUnseenBlocks(continued, watermark)).toBe(1) + }) + + it('recognizes an optimistic block after the stored row replaces its id', () => { + // mergeMessages swaps an optimistic row for the stored one, which keeps + // localId but carries a new server id (lib/messages.ts), and the user + // block renders with the changing message id (reducerTimeline.ts). If the + // user scrolls up while their own message is still optimistic, the echo + // must not read as a brand new row. + const earlier = visible([userMsg('u0', 'earlier turn', BASE_AT)]) + const optimistic = { ...earlier[0], id: 'local-1', localId: 'local-1' } + const watermark = createUnseenWatermark([...earlier, optimistic]) + + // The stored echo keeps localId but arrives under a server id. Anchoring + // must still land on it, not fall back to the block before it. + const stored = [...earlier, { ...optimistic, id: 'srv-1' }] + expect(countUnseenBlocks(stored, watermark)).toBe(0) + }) +}) diff --git a/web/src/chat/unseenBlocks.ts b/web/src/chat/unseenBlocks.ts new file mode 100644 index 00000000..10b74373 --- /dev/null +++ b/web/src/chat/unseenBlocks.ts @@ -0,0 +1,106 @@ +import { isToolGroupBlock, visibleBlockRole, type VisibleChatBlock } from '@/chat/toolGroups' + +/** + * Snapshot of the blocks the user had already seen when they scrolled away + * from the tail. Compared against the current blocks to answer "how much new + * content is below me". + * + * Tracks localId alongside id because a block's id is not stable: an optimistic + * row is replaced by a stored row carrying the same localId under a new server + * id (mergeMessages in lib/messages.ts), and the rendered block uses the message + * id. Without localId, the user's own message would read as new content the + * moment its echo arrives. + */ +export type UnseenWatermark = { + ids: Set + localIds: Set +} + +function getLocalId(block: VisibleChatBlock): string | null { + return 'localId' in block ? block.localId : null +} + +export function createUnseenWatermark(blocks: readonly VisibleChatBlock[]): UnseenWatermark { + const ids = new Set() + const localIds = new Set() + for (const block of blocks) { + ids.add(block.id) + const localId = getLocalId(block) + if (localId) { + localIds.add(localId) + } + } + return { ids, localIds } +} + +function isKnownBlock(block: VisibleChatBlock, watermark: UnseenWatermark): boolean { + if (watermark.ids.has(block.id)) { + return true + } + const localId = getLocalId(block) + if (localId && watermark.localIds.has(localId)) { + return true + } + // A lone tool-call renders under its own tool id until a second eligible + // tool arrives and absorbs it into a group, at which point the id becomes + // `tool-group:` (see createToolGroupId in toolGroups.ts). + // Match on the member ids so that absorption doesn't look like new content. + return isToolGroupBlock(block) + && (watermark.ids.has(block.firstToolId) || watermark.ids.has(block.lastToolId)) +} + +/** + * Counts the rows that appeared after the anchor. Blocks are not rows: + * `@assistant-ui/react` joins a run of adjacent assistant-role blocks into one + * card, so a response made of reasoning + text + a tool call renders as a single + * new row. Role assignment is shared with the runtime via `visibleBlockRole` so + * the two cannot drift apart. + */ +function countRenderedRowsAfter(blocks: readonly VisibleChatBlock[], anchor: number): number { + let rows = 0 + let previousRole = visibleBlockRole(blocks[anchor]) + for (let index = anchor + 1; index < blocks.length; index += 1) { + const role = visibleBlockRole(blocks[index]) + // A new row starts unless this block joins the assistant card above it. + if (role !== 'assistant' || previousRole !== 'assistant') { + rows += 1 + } + previousRole = role + } + return rows +} + +/** + * Counts the rendered rows that appeared after the last block the watermark + * knows about — i.e. what the user would find by scrolling down. + * + * Deliberately anchor-based rather than timestamp-based: the blocks array is + * not monotonic in `createdAt` (messages sort by `invokedAt ?? createdAt`, so a + * queued message lands at the end while carrying an old `createdAt`), and + * optimistic rows change both id and `createdAt` when the server row replaces + * them. Anchoring on the last recognized block sidesteps all of that, and makes + * prepended history (loadMore) free: older blocks land before the anchor. + * + * Only counts blocks that are actually in the window. When the user scrolls far + * enough back that the history window fills up (HISTORY_WINDOW_SIZE), incoming + * messages are trimmed off the tail by mergeIntoWindow and never reach the + * reducer, so this reports 0. That is intentional: under-reporting beats the + * old behaviour of counting raw messages, and entering tail mode force-refetches + * the latest page anyway. + */ +export function countUnseenBlocks( + blocks: readonly VisibleChatBlock[], + watermark: UnseenWatermark | null +): number { + if (!watermark || watermark.ids.size === 0) { + return 0 + } + for (let index = blocks.length - 1; index >= 0; index -= 1) { + if (isKnownBlock(blocks[index], watermark)) { + return countRenderedRowsAfter(blocks, index) + } + } + // Every seen block has been trimmed out of the window. Report nothing + // rather than claiming the whole window is new. + return 0 +} diff --git a/web/src/components/SessionChat.tsx b/web/src/components/SessionChat.tsx index 6afc8d92..33dffb6e 100644 --- a/web/src/components/SessionChat.tsx +++ b/web/src/components/SessionChat.tsx @@ -20,6 +20,7 @@ import { reduceChatBlocks } from '@/chat/reducer' import { reconcileChatBlocks } from '@/chat/reconcile' import { buildConversationOutline } from '@/chat/outline' import { buildVisibleChatBlocks, isToolGroupBlock, type ToolGroupBlock } from '@/chat/toolGroups' +import { useUnseenBlockCount } from '@/hooks/useUnseenBlockCount' import { isQueuedForInvocation } from '@/lib/messages' import { inactiveSessionCanResume } from '@/lib/sessionResume' import { @@ -403,7 +404,7 @@ type SessionChatProps = { isSyncingTail: boolean isLoadingMoreMessages: boolean isSending: boolean - unseenCount: number + viewMode: 'tail' | 'history' messagesVersion: number historyVersion: number onBack: () => void @@ -986,6 +987,11 @@ function SessionChatInner(props: SessionChatProps) { visibleGroupsRef.current = visibleBlocks.filter(isToolGroupBlock) }, [visibleBlocks]) + // "N new messages" counts rendered blocks, not raw messages: a subagent run + // is dozens of sidechain messages but a single Task card, and a tool_use + + // tool_result pair is one card. + const unseenCount = useUnseenBlockCount(props.viewMode, visibleBlocks) + const outlineItems = useMemo( () => buildConversationOutline(reconciled.blocks), [reconciled.blocks] @@ -1295,7 +1301,7 @@ function SessionChatInner(props: SessionChatProps) { hasMoreMessages={props.hasMoreMessages} isLoadingMoreMessages={props.isLoadingMoreMessages} onLoadMore={props.onLoadMore} - unseenCount={props.unseenCount} + unseenCount={unseenCount} rawMessagesCount={visibleMessages.length} normalizedMessagesCount={normalizedMessages.length} messagesVersion={props.messagesVersion} diff --git a/web/src/hooks/queries/useMessages.ts b/web/src/hooks/queries/useMessages.ts index 38f6e55b..c9609011 100644 --- a/web/src/hooks/queries/useMessages.ts +++ b/web/src/hooks/queries/useMessages.ts @@ -23,7 +23,6 @@ export const EMPTY_STATE: MessageWindowState = { isLoadingMore: false, warning: null, viewMode: 'tail', - unseenCount: 0, messagesVersion: 0, historyVersion: 0, } @@ -34,7 +33,7 @@ export function useMessages(api: ApiClient | null, sessionId: string | null): { isSyncingTail: boolean isLoadingMore: boolean hasMore: boolean - unseenCount: number + viewMode: MessageViewMode messagesVersion: number historyVersion: number loadMore: () => Promise @@ -87,7 +86,7 @@ export function useMessages(api: ApiClient | null, sessionId: string | null): { isSyncingTail: state.isSyncingTail, isLoadingMore: state.isLoadingMore, hasMore: state.hasMore, - unseenCount: state.unseenCount, + viewMode: state.viewMode, messagesVersion: state.messagesVersion, historyVersion: state.historyVersion, loadMore, diff --git a/web/src/hooks/useUnseenBlockCount.test.ts b/web/src/hooks/useUnseenBlockCount.test.ts new file mode 100644 index 00000000..dcf0a2b1 --- /dev/null +++ b/web/src/hooks/useUnseenBlockCount.test.ts @@ -0,0 +1,111 @@ +/** + * Tests for the "N new messages" state machine. The pure counting rules live in + * chat/unseenBlocks.test.ts; this file covers what only shows up once React is + * driving it: when the watermark is captured, that the render-phase setState + * converges instead of looping, and that a session left in history mode does + * not capture a watermark on mount. + */ +import { describe, expect, it } from 'vitest' +import { renderHook } from '@testing-library/react' +import type { ChatBlock } from '@/chat/types' +import type { VisibleChatBlock } from '@/chat/toolGroups' +import { useUnseenBlockCount } from '@/hooks/useUnseenBlockCount' + +type Props = { + mode: 'tail' | 'history' + items: VisibleChatBlock[] +} + +// user-role blocks never join with each other, so each one is exactly one +// rendered row — keeping these cases about the state machine rather than about +// assistant-card joining (covered in chat/unseenBlocks.test.ts). +function block(id: string): ChatBlock { + return { kind: 'user-text', id, localId: null, createdAt: 1, text: id } +} + +function blocks(...ids: string[]): VisibleChatBlock[] { + return ids.map(block) +} + +function setup(initialProps: Props) { + return renderHook( + (props: Props) => useUnseenBlockCount(props.mode, props.items), + { initialProps } + ) +} + +describe('useUnseenBlockCount', () => { + it('reports 0 while the user sits at the tail', () => { + const { result, rerender } = setup({ mode: 'tail', items: blocks('a') }) + + expect(result.current).toBe(0) + rerender({ mode: 'tail', items: blocks('a', 'b', 'c') }) + expect(result.current).toBe(0) + }) + + it('counts blocks that arrive after the user scrolls into history', () => { + const { result, rerender } = setup({ mode: 'tail', items: blocks('a', 'b') }) + + // Scrolling up captures the watermark; nothing is new yet. + rerender({ mode: 'history', items: blocks('a', 'b') }) + expect(result.current).toBe(0) + + rerender({ mode: 'history', items: blocks('a', 'b', 'c') }) + expect(result.current).toBe(1) + + rerender({ mode: 'history', items: blocks('a', 'b', 'c', 'd') }) + expect(result.current).toBe(2) + }) + + it('clears the count when the user returns to the tail', () => { + const { result, rerender } = setup({ mode: 'tail', items: blocks('a') }) + + rerender({ mode: 'history', items: blocks('a') }) + rerender({ mode: 'history', items: blocks('a', 'b') }) + expect(result.current).toBe(1) + + rerender({ mode: 'tail', items: blocks('a', 'b') }) + expect(result.current).toBe(0) + + // A later arrival while at the tail still counts for nothing. + rerender({ mode: 'tail', items: blocks('a', 'b', 'c') }) + expect(result.current).toBe(0) + }) + + it('does not capture a watermark when mounting straight into history mode', () => { + // The store keeps view mode per session, so re-opening a session that was + // left scrolled up starts in history without a tail -> history flip. + const { result, rerender } = setup({ mode: 'history', items: blocks('a', 'b') }) + + expect(result.current).toBe(0) + + // Without a watermark nothing is attributed as new until the user + // actually visits the tail and scrolls away again. + rerender({ mode: 'history', items: blocks('a', 'b', 'c') }) + expect(result.current).toBe(0) + + rerender({ mode: 'tail', items: blocks('a', 'b', 'c') }) + rerender({ mode: 'history', items: blocks('a', 'b', 'c') }) + rerender({ mode: 'history', items: blocks('a', 'b', 'c', 'd') }) + expect(result.current).toBe(1) + }) + + it('settles on a stable value instead of re-rendering forever', () => { + let renders = 0 + const { result, rerender } = renderHook( + (props: Props) => { + renders += 1 + return useUnseenBlockCount(props.mode, props.items) + }, + { initialProps: { mode: 'tail', items: blocks('a') } as Props } + ) + + const afterMount = renders + rerender({ mode: 'history', items: blocks('a') }) + + // The view-mode flip costs one extra render pass to apply the watermark; + // it must not keep re-entering the setState branch after that. + expect(renders - afterMount).toBeLessThanOrEqual(3) + expect(result.current).toBe(0) + }) +}) diff --git a/web/src/hooks/useUnseenBlockCount.ts b/web/src/hooks/useUnseenBlockCount.ts new file mode 100644 index 00000000..409a139e --- /dev/null +++ b/web/src/hooks/useUnseenBlockCount.ts @@ -0,0 +1,34 @@ +import { useMemo, useState } from 'react' +import type { VisibleChatBlock } from '@/chat/toolGroups' +import { countUnseenBlocks, createUnseenWatermark, type UnseenWatermark } from '@/chat/unseenBlocks' + +/** + * Tracks how many rendered blocks appeared since the user scrolled away from + * the tail, for the "N new messages" pill. + * + * The watermark is captured during render rather than in an effect so the first + * frame after leaving the tail already reports 0 instead of briefly showing a + * stale count. This is React's "adjust state during render" pattern: the + * setState pair runs only on the frame where viewMode actually flips, and + * updating prevViewMode makes the condition false on the immediate re-render, + * so it converges instead of looping. + * + * viewMode is seeded from the caller's current value because the message window + * store keeps view mode per session in a module-level map — returning to a + * session that was left in history mode must not be mistaken for a fresh + * tail -> history transition and capture a watermark the user never saw. + */ +export function useUnseenBlockCount( + viewMode: 'tail' | 'history', + blocks: readonly VisibleChatBlock[] +): number { + const [prevViewMode, setPrevViewMode] = useState(viewMode) + const [watermark, setWatermark] = useState(null) + + if (viewMode !== prevViewMode) { + setPrevViewMode(viewMode) + setWatermark(viewMode === 'history' ? createUnseenWatermark(blocks) : null) + } + + return useMemo(() => countUnseenBlocks(blocks, watermark), [blocks, watermark]) +} diff --git a/web/src/lib/assistant-runtime.ts b/web/src/lib/assistant-runtime.ts index 7fa32fe7..e670c4a2 100644 --- a/web/src/lib/assistant-runtime.ts +++ b/web/src/lib/assistant-runtime.ts @@ -9,6 +9,7 @@ import { renderEventLabel } from '@/chat/presentation' import type { ChatBlock, CliOutputBlock, CodexReview, UsageData } from '@/chat/types' import type { AgentEvent, ToolCallBlock } from '@/chat/types' import type { ToolGroupBlock, VisibleChatBlock } from '@/chat/toolGroups' +import { visibleBlockRole } from '@/chat/toolGroups' import type { AttachmentMetadata, MessageStatus as HappyMessageStatus, Session } from '@/types/api' /** @@ -73,20 +74,6 @@ function formatCodexReviewText(review: CodexReview): string { return lines.join('\n') } -type VisibleChatBlockRole = 'user' | 'assistant' | 'system' - -/** - * Mirror the role assignment used by `toThreadMessageLike` so response - * group boundaries (the `@assistant-ui/react` converter joins adjacent - * assistant-role messages only) stay consistent with what the library - * actually flushes as one card. - */ -function visibleBlockRole(block: VisibleChatBlock): VisibleChatBlockRole { - if (block.kind === 'user-text') return 'user' - if (block.kind === 'agent-event') return 'system' - if (block.kind === 'cli-output') return block.source === 'user' ? 'user' : 'assistant' - return 'assistant' -} export function getBlockPresentationTimestamp(block: VisibleChatBlock): number { if (visibleBlockRole(block) === 'user') { diff --git a/web/src/lib/message-window-store.test.ts b/web/src/lib/message-window-store.test.ts index 63f3a745..fff52ec5 100644 --- a/web/src/lib/message-window-store.test.ts +++ b/web/src/lib/message-window-store.test.ts @@ -688,7 +688,7 @@ describe('message tail synchronization', () => { }) describe('history view and older pagination', () => { - it('appends while reading history, increments unseen state, then clears and compacts at the tail', () => { + it('appends while reading history, then compacts at the tail', () => { const id = sessionId('history-unseen') const initial = Array.from({ length: VISIBLE_WINDOW_SIZE }, (_, index) => makeAgentMessage({ id: `initial-${index}`, seq: index + 1, at: index + 1 }) @@ -701,16 +701,12 @@ describe('history view and older pagination', () => { makeAgentMessage({ id: 'new-2', seq: 402, at: 402 }) ]) - expect(getMessageWindowState(id)).toMatchObject({ - viewMode: 'history', - unseenCount: 2 - }) + expect(getMessageWindowState(id).viewMode).toBe('history') expect(getMessageWindowState(id).messages.map((message) => message.id)).toContain('new-2') setMessageViewMode(id, 'tail') const state = getMessageWindowState(id) expect(state.viewMode).toBe('tail') - expect(state.unseenCount).toBe(0) expect(state.messages).toHaveLength(VISIBLE_WINDOW_SIZE) expect(state.messages.at(-1)?.id).toBe('new-2') }) diff --git a/web/src/lib/message-window-store.ts b/web/src/lib/message-window-store.ts index 69e9ddcd..d628e7d8 100644 --- a/web/src/lib/message-window-store.ts +++ b/web/src/lib/message-window-store.ts @@ -1,5 +1,4 @@ import type { ApiClient } from '@/api/client' -import { normalizeDecryptedMessage } from '@/chat/normalize' import type { DecryptedMessage, MessageStatus, MessagesResponse } from '@/types/api' import { isQueuedForInvocation, mergeMessages } from '@/lib/messages' @@ -16,7 +15,6 @@ export type MessageWindowState = { isLoadingMore: boolean warning: string | null viewMode: MessageViewMode - unseenCount: number messagesVersion: number historyVersion: number } @@ -37,7 +35,6 @@ type InternalState = MessageWindowState & { oldestPositionSeq: number | null newestPositionAt: number | null newestPositionSeq: number | null - unseenIds: Set requiresLatestReset: boolean syncGeneration: number olderGeneration: number @@ -216,14 +213,12 @@ function createState(sessionId: string): InternalState { isLoadingMore: false, warning: null, viewMode: 'tail', - unseenCount: 0, messagesVersion: 0, historyVersion: 0, oldestPositionAt: null, oldestPositionSeq: null, newestPositionAt: null, newestPositionSeq: null, - unseenIds: new Set(), requiresLatestReset: false, syncGeneration: 0, olderGeneration: 0 @@ -373,7 +368,6 @@ function buildState( | 'oldestPositionSeq' | 'newestPositionAt' | 'newestPositionSeq' - | 'unseenIds' | 'requiresLatestReset' | 'syncGeneration' | 'olderGeneration' @@ -381,7 +375,6 @@ function buildState( >> ): InternalState { const messages = updates.messages ?? previous.messages - const unseenIds = updates.unseenIds ?? previous.unseenIds const bounds = deriveSeqBounds(messages) return { ...previous, @@ -389,8 +382,6 @@ function buildState( messages, oldestSeq: bounds.oldestSeq, newestSeq: bounds.newestSeq, - unseenIds, - unseenCount: unseenIds.size, messagesVersion: messages === previous.messages ? previous.messagesVersion : previous.messagesVersion + 1 @@ -450,39 +441,10 @@ function optimisticMessage(message: DecryptedMessage): boolean { return Boolean(message.localId && message.id === message.localId) } -function unseenIdentity(message: DecryptedMessage): string { - return message.localId ? `local:${message.localId}` : `id:${message.id}` -} - -function collectNewUnseenIds( - previous: InternalState, - incoming: DecryptedMessage[] -): Set { - if (previous.viewMode === 'tail' || incoming.length === 0) { - return previous.unseenIds - } - const representedIds = new Set(previous.messages.map((message) => message.id)) - const representedLocalIds = new Set( - previous.messages.flatMap((message) => message.localId ? [message.localId] : []) - ) - const unseenIds = new Set(previous.unseenIds) - for (const message of incoming) { - const alreadyRepresented = representedIds.has(message.id) - || Boolean(message.localId && representedLocalIds.has(message.localId)) - representedIds.add(message.id) - if (message.localId) representedLocalIds.add(message.localId) - if (alreadyRepresented || isQueuedForInvocation(message)) continue - if (normalizeDecryptedMessage(message) === null) continue - unseenIds.add(unseenIdentity(message)) - } - return unseenIds -} - function mergeIntoWindow( previous: InternalState, incoming: DecryptedMessage[], options: { - countUnseen?: boolean mode?: 'append' | 'prepend' regularLimit?: number } = {} @@ -496,8 +458,7 @@ function mergeIntoWindow( const merged = mergeMessages(previous.messages, incoming) const { kept, dropped } = trimPreservingQueued(merged, regularLimit, mode) let next = buildState(previous, { - messages: kept, - unseenIds: options.countUnseen ? collectNewUnseenIds(previous, incoming) : previous.unseenIds + messages: kept }) if (dropped.length === 0) { return next @@ -565,7 +526,6 @@ function applyLatestResponse( oldestPositionSeq: oldest?.seq ?? null, newestPositionAt: newest?.at ?? null, newestPositionSeq: newest?.seq ?? null, - unseenIds: collectNewUnseenIds(previous, response.messages), requiresLatestReset: false, isLoadingMore: options.replaceServerRows ? false : previous.isLoadingMore, olderGeneration: options.replaceServerRows @@ -663,9 +623,7 @@ async function runTailSync(api: ApiClient, sessionId: string): Promise { updateState(sessionId, (previous) => { if (previous.syncGeneration !== generation) return previous - const merged = mergeIntoWindow(previous, response.messages, { - countUnseen: previous.viewMode === 'history' - }) + const merged = mergeIntoWindow(previous, response.messages) if (merged.requiresLatestReset) { return buildState(merged, { epoch: response.page.epoch, @@ -748,7 +706,6 @@ function enterTailMode(previous: InternalState): InternalState { messages: kept, hasMore: previous.hasMore || dropped.length > 0, viewMode: 'tail', - unseenIds: new Set(), epoch: forceLatest ? null : previous.epoch, oldestPositionAt: oldest?.at ?? null, oldestPositionSeq: oldest?.seq ?? null, @@ -763,7 +720,6 @@ export function activateMessageWindow(sessionId: string): void { const forceLatest = previous.requiresLatestReset if ( previous.viewMode === 'tail' - && previous.unseenIds.size === 0 && kept.length === previous.messages.length && !forceLatest ) { @@ -875,9 +831,7 @@ export function setMessageViewMode(sessionId: string, mode: MessageViewMode): vo export function ingestIncomingMessages(sessionId: string, incoming: DecryptedMessage[]): void { if (incoming.length === 0) return updateState(sessionId, (previous) => { - let merged = mergeIntoWindow(previous, incoming, { - countUnseen: previous.viewMode === 'history' - }) + let merged = mergeIntoWindow(previous, incoming) if (merged.epoch === null || merged.requiresLatestReset) { return merged } diff --git a/web/src/router.tsx b/web/src/router.tsx index e76567bc..4a04692f 100644 --- a/web/src/router.tsx +++ b/web/src/router.tsx @@ -719,7 +719,7 @@ function SessionPage() { hasMore: messagesHasMore, loadMore: loadMoreMessages, refetch: refetchMessages, - unseenCount, + viewMode: messagesViewMode, messagesVersion, historyVersion, setViewMode, @@ -1084,7 +1084,7 @@ function SessionPage() { isSyncingTail={messagesSyncingTail} isLoadingMoreMessages={messagesLoadingMore} isSending={isSending} - unseenCount={unseenCount} + viewMode={messagesViewMode} messagesVersion={messagesVersion} historyVersion={historyVersion} onBack={goBack}