diff --git a/packages/ui/src/sync/use-sync.test.ts b/packages/ui/src/sync/use-sync.test.ts index bdd7dade..8f94ecc0 100644 --- a/packages/ui/src/sync/use-sync.test.ts +++ b/packages/ui/src/sync/use-sync.test.ts @@ -1,6 +1,9 @@ import { describe, expect, test } from 'bun:test' +import type { Message, Part } from '@opencode-ai/sdk/v2/client' -import { shouldFetchSessionForRenderableSync } from './use-sync' +import { shouldFetchSessionForRenderableSync, hasUserMessage } from './use-sync' +import { mergeOptimisticPage } from './optimistic' +import { materializeSessionSnapshots } from './materialization' describe('shouldFetchSessionForRenderableSync', () => { test('fetches full session detail when a lightweight list session is opened', () => { @@ -27,3 +30,215 @@ describe('shouldFetchSessionForRenderableSync', () => { })).toBe(true) }) }) + +// --- loadMessages incremental commit (#2084) --------------------------------- +// loadMessages is a React hook callback and cannot be unit-tested in isolation. +// These tests cover the three invariants the refactor relies on: +// 1. hasUserMessage gates the first store commit (assistant-only tails defer +// to the expansion loop so the skeleton stays instead of an empty chat). +// 2. Incremental materialization of superset pages preserves references for +// already-committed messages (no re-render churn, no reference breaks). +// 3. mergeOptimisticPage + clearOptimistic is idempotent across commits. + +function assistantMessage(id: string): Message { + return { id, sessionID: 'ses_1', role: 'assistant', time: { created: 1 } } as Message +} +function userMessage(id: string): Message { + return { id, sessionID: 'ses_1', role: 'user', time: { created: 1 } } as Message +} +function assistantMessageWithClientRole(id: string): Message { + // OpenCode sets clientRole on the wire; role may be absent. + return { id, sessionID: 'ses_1', clientRole: 'user', time: { created: 1 } } as unknown as Message +} +function textPart(id: string, messageID: string): Part { + return { id, messageID, sessionID: 'ses_1', type: 'text', text: id } as Part +} + +describe('hasUserMessage', () => { + test('returns true when a user message is present', () => { + expect(hasUserMessage([assistantMessage('m_1'), userMessage('m_2')])).toBe(true) + }) + + test('returns true when clientRole marks the message as user', () => { + expect(hasUserMessage([assistantMessageWithClientRole('m_1')])).toBe(true) + }) + + test('returns false when only assistant messages are present', () => { + expect(hasUserMessage([assistantMessage('m_1'), assistantMessage('m_2')])).toBe(false) + }) + + test('returns false for undefined', () => { + expect(hasUserMessage(undefined)).toBe(false) + }) + + test('returns false for an empty array', () => { + expect(hasUserMessage([])).toBe(false) + }) +}) + +describe('incremental materialization of superset pages (#2084)', () => { + const SKIP_PARTS = new Set(['patch', 'step-start', 'step-finish']) + + test('preserves message references for already-committed messages', () => { + // Simulate expansion: commit 50 (assistant-only), then 100 (with user), then 150. + // Pages are supersets: the 100-page includes all 50 from the first page, + // the 150-page includes all 100 from the second. + // Messages are sorted by id in the store (mergeMessages uses cmp by id), + // so look them up by id rather than positional index. + const a1 = assistantMessage('a_1') + const a2 = assistantMessage('a_2') + const u1 = userMessage('u_1') + const u0 = userMessage('u_0') + const a3 = assistantMessage('a_3') + const page50 = [a1, a2] + const page100 = [u1, a1, a2] + const page150 = [u0, u1, a1, a2, a3] + + const partsFor = (msgs: Message[]): { info: Message; parts: Part[] }[] => + msgs.map((info) => ({ + info, + parts: info.role === 'assistant' ? [textPart(`p_${info.id}`, info.id)] : [], + })) + + let state = { message: {} as Record, part: {} as Record } + + // Commit 1: assistant-only page (skeleton stays — no user message). + // But materialization itself is valid; we test the reference property here. + const m1 = materializeSessionSnapshots(state, 'ses_1', partsFor(page50), { skipPartTypes: SKIP_PARTS }) + state = { message: m1.message, part: m1.part } + const afterFirst = state.message.ses_1 + expect(afterFirst.find((m) => m.id === 'a_1')).toBe(a1) + expect(afterFirst.find((m) => m.id === 'a_2')).toBe(a2) + + // Commit 2: 100-message superset. + const m2 = materializeSessionSnapshots(state, 'ses_1', partsFor(page100), { skipPartTypes: SKIP_PARTS }) + state = { message: m2.message, part: m2.part } + const afterSecond = state.message.ses_1 + expect(afterSecond.find((m) => m.id === 'a_1')).toBe(a1) + expect(afterSecond.find((m) => m.id === 'a_2')).toBe(a2) + expect(afterSecond.find((m) => m.id === 'u_1')).toBe(u1) + + // Commit 3: 150-message superset. + const m3 = materializeSessionSnapshots(state, 'ses_1', partsFor(page150), { skipPartTypes: SKIP_PARTS }) + const afterThird = m3.message.ses_1 + // All previously-committed messages keep their references. + expect(afterThird.find((m) => m.id === 'a_1')).toBe(a1) + expect(afterThird.find((m) => m.id === 'a_2')).toBe(a2) + expect(afterThird.find((m) => m.id === 'u_1')).toBe(u1) + // New messages from the 150-page are present. + expect(afterThird.find((m) => m.id === 'u_0')).toBe(u0) + expect(afterThird.find((m) => m.id === 'a_3')).toBe(a3) + }) + + test('preserves part references for unchanged assistant messages', () => { + const msg = assistantMessage('a_1') + const prt = textPart('p_a_1', 'a_1') + const state = { + message: { ses_1: [msg] } as Record, + part: { a_1: [prt] } as Record, + } + + // Re-materialize with the same message + part (superset adds a new message). + const m = materializeSessionSnapshots(state, 'ses_1', [ + { info: msg, parts: [prt] }, + { info: userMessage('u_1'), parts: [] }, + ], { skipPartTypes: new Set(['patch', 'step-start', 'step-finish']) }) + + // The existing part array reference is preserved (equivalent snapshot). + expect(m.part.a_1).toBe(state.part.a_1) + expect(m.partsChanged).toBe(false) // user message has no parts → no part change + }) + + test('does not create a no-op store update when re-committing the same page', () => { + const msg = userMessage('u_1') + const state = { + message: { ses_1: [msg] } as Record, + part: {} as Record, + } + + const m = materializeSessionSnapshots(state, 'ses_1', [ + { info: msg, parts: [] }, + ], { skipPartTypes: new Set(['patch', 'step-start', 'step-finish']) }) + + expect(m.messagesChanged).toBe(false) + expect(m.partsChanged).toBe(false) + expect(m.message).toBe(state.message) + expect(m.part).toBe(state.part) + }) +}) + +describe('mergeOptimisticPage idempotency across commits (#2084)', () => { + test('second call after clearOptimistic returns the page unchanged', () => { + // Simulate: first commit confirmed an optimistic item, clearOptimistic + // removed it; second commit (expansion) finds no optimistic items. + const page = { session: [userMessage('u_1')], part: [{ id: 'u_1', part: [] }], cursor: 'c1', complete: false } + + // First merge with one optimistic item. + const optimisticItem = { message: userMessage('opt_1'), parts: [textPart('p_opt_1', 'opt_1')] } + const merged1 = mergeOptimisticPage(page, [optimisticItem]) + expect(merged1.confirmed).toEqual([]) // opt_1 is not in page.session → not confirmed + + // After clearOptimistic (simulated by passing empty items), merge is a no-op. + const merged2 = mergeOptimisticPage(page, []) + expect(merged2.confirmed).toEqual([]) + expect(merged2.session).toEqual(page.session) + expect(merged2.cursor).toBe('c1') + expect(merged2.complete).toBe(false) + }) + + test('mergeOptimisticPage with empty items is a fast no-op path', () => { + const page = { + session: [assistantMessage('a_1')], + part: [{ id: 'a_1', part: [textPart('p_a_1', 'a_1')] }], + cursor: undefined, + complete: true, + } + const merged = mergeOptimisticPage(page, []) + expect(merged.session).toEqual(page.session) + expect(merged.confirmed).toEqual([]) + expect(merged.complete).toBe(true) + }) +}) + +describe('first-commit gating invariant (#2084)', () => { + // The loadMessages refactor gates the first store commit on hasUserMessage. + // When the first page is assistant-only, it defers to the expansion loop + // and keeps the skeleton. This test verifies the gate condition itself: + // hasUserMessage(page.session) must be false for an assistant-only page. + test('assistant-only page does not trigger an early commit', () => { + const assistantOnlyPage = [assistantMessage('a_1'), assistantMessage('a_2')] + expect(hasUserMessage(assistantOnlyPage)).toBe(false) + }) + + test('page with a user message triggers an early commit', () => { + const pageWithUser = [assistantMessage('a_1'), userMessage('u_1')] + expect(hasUserMessage(pageWithUser)).toBe(true) + }) + + test('complete page triggers an early commit even without a user message', () => { + // page.complete means there are no older messages to expand into. + // Committing immediately is correct — the skeleton should disappear. + // This mirrors the `hasUserMessage(page.session) || page.complete` gate. + const completeAssistantOnlyPage = [assistantMessage('a_1')] + const isComplete = true + expect(hasUserMessage(completeAssistantOnlyPage) || isComplete).toBe(true) + }) + + test('prepend mode always commits — deferral does not apply', () => { + // The deferred init path exists to keep the skeleton visible for an + // assistant-only initial fetch. Prepend mode (loading older history) + // has no skeleton to protect and must always write to the store — + // otherwise the fetched older messages would be silently dropped. + // + // This test pins the gate condition: deferral requires `!options.before`. + // An assistant-only, incomplete page in prepend mode must NOT defer. + const assistantOnlyPage = [assistantMessage('a_1'), assistantMessage('a_2')] + const hasBefore = true // prepend mode + const isComplete = false + + // The actual gate in loadMessages is: + // const deferFirstCommit = !options?.before && !page.complete && !hasUserMessage(page.session) + const deferFirstCommit = !hasBefore && !isComplete && !hasUserMessage(assistantOnlyPage) + expect(deferFirstCommit).toBe(false) // prepend must not defer + }) +}) diff --git a/packages/ui/src/sync/use-sync.ts b/packages/ui/src/sync/use-sync.ts index e94d537d..a53ebd69 100644 --- a/packages/ui/src/sync/use-sync.ts +++ b/packages/ui/src/sync/use-sync.ts @@ -127,7 +127,7 @@ function isUserMessage(message: Message): boolean { return role === "user" } -function hasUserMessage(messages: Message[] | undefined): boolean { +export function hasUserMessage(messages: Message[] | undefined): boolean { return Boolean(messages?.some(isUserMessage)) } @@ -349,75 +349,141 @@ export function useSync() { setMetaFor(sessionID, { loading: true }) try { - // A resync (no `before`) must fetch at least as many messages as we - // already have on screen. Live events append to the store WITHOUT growing - // m.limit, so reusing the stale m.limit here would under-fetch and make - // the server hand back a spurious "older" cursor — surfacing a phantom - // "load older" button for a session whose full history is already shown - // (e.g. after a reconnect resync following a few new messages). + // Commit a fetched page to the store: merge optimistic items, run + // materialization, and write the result so the UI can render it. + // Returns the committed meta so the caller can update pagination + // state once at the end. The store write happens here (per page) so + // the hydrating skeleton disappears after the first fetch instead of + // waiting for the full expansion sequence. + const commitMessagesToStore = ( + page: Awaited>, + mode: "replace" | "prepend" | undefined, + isStale?: () => boolean, + ) => { + const items = getOptimistic(sessionID) + const merged = mergeOptimisticPage(page, items) + for (const messageID of merged.confirmed) { + clearOptimistic(sessionID, messageID) + } + + if (isStale?.()) { + return { messages: [], cursor: merged.cursor, complete: merged.complete } + } + + const current = store.getState() + const materialized = materializeSessionSnapshots( + current, + sessionID, + merged.session.map((info) => ({ + info, + parts: merged.part.find((item) => item.id === info.id)?.part ?? [], + })), + { skipPartTypes: SKIP_PARTS, mode: mode === "prepend" ? "prepend" : "merge" }, + ) + + // materializeSessionSnapshots is synchronous today, so this check + // is defense-in-depth: it guards the store write if materialization + // ever becomes async or yields between the check above and setState. + if (isStale?.()) { + return { messages: [], cursor: merged.cursor, complete: merged.complete } + } + + if (materialized.messagesChanged || materialized.partsChanged) { + store.setState({ + ...(materialized.messagesChanged ? { message: materialized.message } : {}), + ...(materialized.partsChanged ? { part: materialized.part } : {}), + }) + } + return { messages: materialized.messages, cursor: merged.cursor, complete: merged.complete } + } + + // Live events can append messages without growing m.limit. A resync + // must cover everything already rendered or it can manufacture an + // "older" cursor for history that is already on screen. const storeMessageCount = store.getState().message[sessionID]?.length ?? 0 - const limit = options?.before ? HISTORY_MESSAGE_PAGE_SIZE : Math.max(m.limit, storeMessageCount) - let page = await fetchMessages(sessionID, limit, options?.before) + const limit = options?.before + ? HISTORY_MESSAGE_PAGE_SIZE + : Math.max(m.limit, storeMessageCount) + const page = await fetchMessages(sessionID, limit, options?.before) + + // Commit the first page to the store immediately so the hydrating + // skeleton disappears after a single round-trip — but only when the + // page already contains a user message boundary. If the tail is + // assistant/tool-only (a very large final turn), committing now would + // drop the skeleton and render an empty chat (turn projection skips + // assistant messages without a user parent), which looks like a fresh + // session instead of a loading state. In that case defer the first + // commit to the expansion loop below, which fetches older records + // until a user boundary appears and commits that page instead. + // + // The deferral only applies to the initial fetch (no `before`). + // Prepend mode (loading older history) always commits — messages are + // already rendered, so there is no skeleton to protect, and skipping + // the store write would drop the fetched older messages entirely. + // + // The deferred fallback carries page.session (not []) so that if the + // expansion loop is ever a no-op (e.g. all nextLimit <= limit after a + // constant change), the final setMetaFor reflects the real fetched + // count instead of overwriting it with 0. + const deferFirstCommit = + !options?.before && !page.complete && !hasUserMessage(page.session) + let committed = deferFirstCommit + ? { messages: page.session, cursor: page.cursor, complete: page.complete } + : commitMessagesToStore(page, options?.mode, options?.isStale) + + // If the first commit detected a stale session, bail out immediately + // instead of relying on downstream guards to skip the final setMetaFor. + if (options?.isStale?.()) { + setMetaFor(sessionID, { loading: false }) + return + } // Keep the initial page small for switch performance. Some sessions // have a very large final turn, so the latest records can // contain only assistant/tool records and no user boundary. That makes // turn projection render an empty chat until the user manually loads // older messages. Expand only this initial tail fetch, with a hard cap. + // Each expanded page is committed to the store incrementally so the + // user sees content as soon as a user boundary appears, instead of + // waiting for the full expansion sequence before the first paint. if (!options?.before && !page.complete && !hasUserMessage(page.session)) { for (const nextLimit of getInitialPageExpansionLimits()) { if (nextLimit <= limit) continue - page = await fetchMessages(sessionID, nextLimit) - if (page.complete || hasUserMessage(page.session)) break + if (options?.isStale?.()) { + setMetaFor(sessionID, { loading: false }) + return + } + const expandedPage = await fetchMessages(sessionID, nextLimit) + if (options?.isStale?.()) { + setMetaFor(sessionID, { loading: false }) + return + } + committed = commitMessagesToStore(expandedPage, options?.mode, options?.isStale) + if (options?.isStale?.()) { + setMetaFor(sessionID, { loading: false }) + return + } + if (expandedPage.complete || hasUserMessage(expandedPage.session)) break } } - // Merge optimistic items - const items = getOptimistic(sessionID) - const merged = mergeOptimisticPage(page, items) - for (const messageID of merged.confirmed) { - clearOptimistic(sessionID, messageID) - } - - if (options?.isStale?.()) { - setMetaFor(sessionID, { loading: false }) - return - } - - const current = store.getState() - const materialized = materializeSessionSnapshots( - current, - sessionID, - merged.session.map((info) => ({ - info, - parts: merged.part.find((item) => item.id === info.id)?.part ?? [], - })), - { skipPartTypes: SKIP_PARTS, mode: options?.mode === "prepend" ? "prepend" : "merge" }, - ) - if (options?.isStale?.()) { setMetaFor(sessionID, { loading: false }) return } setMetaFor(sessionID, { - limit: materialized.messages.length, - cursor: merged.cursor, - complete: merged.complete, + limit: committed.messages.length, + cursor: committed.cursor, + complete: committed.complete, loading: false, }) - if (materialized.messagesChanged || materialized.partsChanged) { - store.setState({ - ...(materialized.messagesChanged ? { message: materialized.message } : {}), - ...(materialized.partsChanged ? { part: materialized.part } : {}), - }) - } setSessionPrefetch({ directory, sessionID, - limit: materialized.messages.length, - cursor: merged.cursor, - complete: merged.complete, + limit: committed.messages.length, + cursor: committed.cursor, + complete: committed.complete, }) } catch { setMetaFor(sessionID, { loading: false })