From 76e9ac3915e5bea2a7920a73a2d3c5d52aa546a9 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sun, 26 Jul 2026 05:00:18 -0500 Subject: [PATCH] fix(desktop): preserve correction order in session tabs --- .../e2e/correction-session-switch.spec.ts | 19 ++- .../e2e/image-attachment-resume.spec.ts | 7 +- apps/desktop/e2e/warm-resume-jitter.spec.ts | 158 ++++++++---------- .../src/app/chat/session-tile-actions.ts | 81 ++++++--- 4 files changed, 142 insertions(+), 123 deletions(-) diff --git a/apps/desktop/e2e/correction-session-switch.spec.ts b/apps/desktop/e2e/correction-session-switch.spec.ts index 0b908380760..dc435b1d538 100644 --- a/apps/desktop/e2e/correction-session-switch.spec.ts +++ b/apps/desktop/e2e/correction-session-switch.spec.ts @@ -21,11 +21,9 @@ const INFERENCE_SWITCH_TRIGGER = 'E2E_INFERENCE_SWITCH_TRIGGER' const INFERENCE_PROMPT = `${INFERENCE_SWITCH_TRIGGER}: original inference prompt must remain singular.` const INFERENCE_CORRECTION = `${INFERENCE_SWITCH_TRIGGER}: correction sent while inference is live.` -// A ⌘T-style tab can put several chat surfaces on the page at once, so every -// locator here is scoped to the ACTIVE one (the most recently mounted surface) -// rather than `.first()` / a bare document query, which would silently target -// the wrong session's composer or transcript. -const SURFACE = '[data-composer-target]' +// Inactive tabs stay mounted under a data-pane-hidden ancestor. Match the +// renderer's keep-alive visibility policy instead of relying on DOM order. +const SURFACE = '[data-composer-target]:not([data-pane-hidden] [data-composer-target])' function activeSurface(page: Page) { return page.locator(SURFACE).last() @@ -102,7 +100,9 @@ async function transcriptMessageOrder(page: Page): Promise { const viewport = surfaces[surfaces.length - 1]?.querySelector('[data-slot="aui_thread-viewport"]') if (!viewport) return [] - return Array.from(viewport.querySelectorAll('[data-role="user"], [data-role="assistant"]')) + return Array.from( + viewport.querySelectorAll('[data-role="user"], [data-role="assistant"], [data-role="system"]'), + ) .map(message => message.textContent?.trim() ?? '') .filter(Boolean) }, SURFACE) @@ -150,7 +150,12 @@ async function reopenInferenceSession(page: Page): Promise { } function relevantOrder(messages: string[]): string[] { - return messages.filter(message => message.includes(ORIGINAL_PROMPT) || message.includes(CORRECTION)) + return messages.flatMap(message => { + if (message.includes(ORIGINAL_PROMPT)) return [ORIGINAL_PROMPT] + if (message.includes(CORRECTION)) return [CORRECTION] + + return [] + }) } function steerTurnOrder(messages: string[]): string[] { diff --git a/apps/desktop/e2e/image-attachment-resume.spec.ts b/apps/desktop/e2e/image-attachment-resume.spec.ts index 3c653f2e2c4..a4f8da68e39 100644 --- a/apps/desktop/e2e/image-attachment-resume.spec.ts +++ b/apps/desktop/e2e/image-attachment-resume.spec.ts @@ -93,10 +93,9 @@ function sessionRow(page: Page) { return page.locator('[data-slot="sidebar"] button').filter({ hasText: CAPTION }).first() } -// A ⌘T-style tab can put several chat surfaces on the page at once, so -// transcript queries target the ACTIVE (most recently mounted) surface rather -// than the first match, which would read the wrong session. -const SURFACE = '[data-composer-target]' +// Inactive tabs stay mounted under a data-pane-hidden ancestor. Match the +// renderer's keep-alive visibility policy instead of relying on DOM order. +const SURFACE = '[data-composer-target]:not([data-pane-hidden] [data-composer-target])' function activeViewportText(surfaceSelector: string): string { const surfaces = document.querySelectorAll(surfaceSelector) diff --git a/apps/desktop/e2e/warm-resume-jitter.spec.ts b/apps/desktop/e2e/warm-resume-jitter.spec.ts index 657c503bd9c..2469ac72e4c 100644 --- a/apps/desktop/e2e/warm-resume-jitter.spec.ts +++ b/apps/desktop/e2e/warm-resume-jitter.spec.ts @@ -24,6 +24,8 @@ * MutationObserver burst), but `$messages` was still set twice. * * The test passes when bursts === 1 AND reconciles === 0. + * The sidebar "+" keeps the session warm in another tab. Its reactivation + * follows the same contract: one additive paint and zero reconciles. * * Prerequisite: `npm run build` must have been run so dist/ exists. */ @@ -43,6 +45,10 @@ import { startMockServer } from './mock-server' import { RealSessionBuilder } from './real-session-builder' const SESSION_TITLE = 'E2E Warm Resume Jitter Test' + +// Inactive tabs stay mounted under a data-pane-hidden ancestor. Match the +// renderer's keep-alive visibility policy instead of relying on DOM order. +const SURFACE = '[data-composer-target]:not([data-pane-hidden] [data-composer-target])' /** 32 messages (16 user/assistant pairs) — enough DOM churn for detection. */ const MESSAGE_COUNT = 32 /** Seeded PRNG so the generated content is deterministic across runs. */ @@ -155,8 +161,9 @@ test.afterAll(async () => { * add/remove nodes. */ async function installRenderCounter(page: import('@playwright/test').Page): Promise { - await page.evaluate(() => { - const viewport = document.querySelector('[data-slot="aui_thread-viewport"]') + await page.evaluate((surfaceSelector: string) => { + const surfaces = document.querySelectorAll(surfaceSelector) + const viewport = surfaces[surfaces.length - 1]?.querySelector('[data-slot="aui_thread-viewport"]') if (!viewport) { throw new Error('Thread viewport not found before warm resume') } @@ -224,7 +231,53 @@ async function installRenderCounter(page: import('@playwright/test').Page): Prom hasMessages = true } }, 2) - }) + }, SURFACE) +} + +/** Wait until the ACTIVE chat surface's transcript contains `text`. */ +async function waitForActiveTranscriptText( + page: import('@playwright/test').Page, + text: string, + timeout = 30_000, +): Promise { + await page.waitForFunction( + ([expected, surfaceSelector]: [string, string]) => { + const surfaces = document.querySelectorAll(surfaceSelector) + const active = surfaces[surfaces.length - 1] + + return (active?.querySelector('[data-slot="aui_thread-viewport"]')?.textContent ?? '').includes(expected) + }, + [text, SURFACE] as [string, string], + { timeout }, + ) +} + +async function waitForActiveTranscriptWithoutText( + page: import('@playwright/test').Page, + text: string, +): Promise { + await page.waitForFunction( + ([expected, surfaceSelector]: [string, string]) => { + const surfaces = document.querySelectorAll(surfaceSelector) + const active = surfaces[surfaces.length - 1] + + return !(active?.querySelector('[data-slot="aui_thread-viewport"]')?.textContent ?? '').includes(expected) + }, + [text, SURFACE] as [string, string], + { timeout: 15_000 }, + ) +} + +/** Replace the primary surface with a draft while retaining its warm cache. */ +async function openFreshDraft(page: import('@playwright/test').Page, priorText: string): Promise { + await page.keyboard.press(process.platform === 'darwin' ? 'Meta+N' : 'Control+N') + await waitForActiveTranscriptWithoutText(page, priorText) +} + +/** Stack an empty tab while leaving the current transcript mounted and warm. */ +async function openNewSessionTab(page: import('@playwright/test').Page, priorText: string): Promise { + await page.locator('[data-slot="sidebar"] button[aria-label="New session"]').first().click() + await waitForActiveTranscriptWithoutText(page, priorText) } /** Stop the render counter and return the recorded burst/reconcile counts. */ @@ -261,7 +314,7 @@ function assertNoJitter(result: { bursts: number; mutations: number; timeline: n ).toBe(0) } -test('warm-route resume paints transcript exactly once (no jitter)', async ({}, testInfo) => { +test('tab reactivation paints the transcript exactly once (no jitter)', async ({}, testInfo) => { const page = fixture!.page // Wait for the sidebar to populate with our seeded session. @@ -277,61 +330,22 @@ test('warm-route resume paints transcript exactly once (no jitter)', async ({}, // Wait for the transcript to appear — the first user message text confirms // the cold-path prefetch painted. - await page.waitForFunction( - (text: string) => - document.querySelector('[data-slot="aui_thread-viewport"]')?.textContent?.includes(text) ?? - false, - FIRST_USER_MSG, - { timeout: 30_000 }, - ) + await waitForActiveTranscriptText(page, FIRST_USER_MSG) // Wait for the session to fully settle (cold-path RPC + reconciliation). await page.waitForTimeout(2_000) - // Step 2: Navigate away to a new chat — this does NOT evict the warm cache. - const newSessionButton = page - .locator('[data-slot="sidebar"] button[aria-label="New session"]') - .first() - await newSessionButton.click() - - // Wait for the new-chat empty state. The "+" opens a NEW TAB beside the - // resumed session rather than replacing it, so the old transcript stays - // mounted — assert the newly-added surface is the empty one. - await page.waitForFunction( - (firstMsg: string) => { - const surfaces = document.querySelectorAll('[data-composer-target]') - const active = surfaces[surfaces.length - 1] - if (!active) return false - const text = active.querySelector('[data-slot="aui_thread-viewport"]')?.textContent ?? '' - return !text.includes(firstMsg) - }, - FIRST_USER_MSG, - { timeout: 15_000 }, - ) + // Observe the loaded transcript before "+" hides it. The old test attached + // after the switch and watched the empty draft instead of this session. + await installRenderCounter(page) + await openNewSessionTab(page, FIRST_USER_MSG) await page.waitForTimeout(500) - // Step 3: Install render counter, click back (warm resume), wait, assert. - await installRenderCounter(page) + // Step 3: Click back, settle, and assert one paint with no second reconcile. await sessionRow.click() - await page.waitForFunction( - (text: string) => - document.querySelector('[data-slot="aui_thread-viewport"]')?.textContent?.includes(text) ?? - false, - FIRST_USER_MSG, - { timeout: 30_000 }, - ) - - // Wait for at least 1 burst, then settle. - await page.waitForFunction( - () => { - const w = window as unknown as { __RENDER_COUNT__?: { bursts: number } } - return Boolean(w.__RENDER_COUNT__ && w.__RENDER_COUNT__.bursts > 0) - }, - undefined, - { timeout: 10_000 }, - ) + await waitForActiveTranscriptText(page, FIRST_USER_MSG) await page.waitForTimeout(2_000) const result = await readRenderCount(page) @@ -357,13 +371,7 @@ test('warm-route resume after background inference completes (no jitter)', async // Step 1: Cold resume — populate the warm cache. await sessionRow.click() - await page.waitForFunction( - (text: string) => - document.querySelector('[data-slot="aui_thread-viewport"]')?.textContent?.includes(text) ?? - false, - FIRST_USER_MSG, - { timeout: 30_000 }, - ) + await waitForActiveTranscriptText(page, FIRST_USER_MSG) await page.waitForTimeout(2_000) // Step 2: Send a message — triggers inference via the mock server. @@ -376,37 +384,15 @@ test('warm-route resume after background inference completes (no jitter)', async // Wait for the mock response to appear in the transcript, confirming // the turn completed and message.complete fired (which updates the warm // cache via updateSessionState). - await page.waitForFunction( - () => { - const viewport = document.querySelector('[data-slot="aui_thread-viewport"]') - return viewport?.textContent?.includes('mock inference server') ?? false - }, - undefined, - { timeout: 60_000 }, - ) + await waitForActiveTranscriptText(page, 'mock inference server', 60_000) // Extra settle for message.complete → updateSessionState → cache write. await page.waitForTimeout(2_000) // Verify the prompt was received by the mock server. expect(mock.receivedPrompts).toContain(PROMPT) - // Step 3: Navigate away — the warm cache retains the updated messages. - const newSessionButton = page - .locator('[data-slot="sidebar"] button[aria-label="New session"]') - .first() - await newSessionButton.click() - // "+" stacks a new tab, so the prior transcript stays mounted in its own - // surface — check the newly-added surface rather than the whole page. - await page.waitForFunction( - (prompt: string) => { - const surfaces = document.querySelectorAll('[data-composer-target]') - const active = surfaces[surfaces.length - 1] - if (!active) return false - return !(active.querySelector('[data-slot="aui_thread-viewport"]')?.textContent ?? '').includes(prompt) - }, - PROMPT, - { timeout: 15_000 }, - ) + // Step 3: Replace the primary chat; the warm cache retains the updated messages. + await openFreshDraft(page, PROMPT) await page.waitForTimeout(500) // Step 4: Install render counter, click back (warm resume), wait, assert. @@ -415,13 +401,7 @@ test('warm-route resume after background inference completes (no jitter)', async // Wait for the transcript to reappear — the warm cache should already // have the completed turn (updated by message.complete events). - await page.waitForFunction( - (text: string) => - document.querySelector('[data-slot="aui_thread-viewport"]')?.textContent?.includes(text) ?? - false, - FIRST_USER_MSG, - { timeout: 30_000 }, - ) + await waitForActiveTranscriptText(page, FIRST_USER_MSG) // Wait for at least 1 burst, then settle. await page.waitForFunction( diff --git a/apps/desktop/src/app/chat/session-tile-actions.ts b/apps/desktop/src/app/chat/session-tile-actions.ts index 0a6db973350..a189af6f647 100644 --- a/apps/desktop/src/app/chat/session-tile-actions.ts +++ b/apps/desktop/src/app/chat/session-tile-actions.ts @@ -236,23 +236,6 @@ export function useSessionTileActions({ runtimeId, scope, storedSessionId }: Ses [listTileSession, scope.attachments.$attachments, submitPromptText] ) - const appendSystemNote = useCallback( - (text: string) => { - update(state => ({ - ...state, - messages: [ - ...state.messages, - { - id: `system-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, - role: 'system', - parts: [textPart(text)] - } - ] - })) - }, - [update] - ) - const cancelRun = useCallback(async () => { const sessionId = runtimeIdRef.current @@ -283,30 +266,82 @@ export function useSessionTileActions({ runtimeId, scope, storedSessionId }: Ses const steerPrompt = useCallback( async (rawText: string): Promise => { const text = rawText.trim() + const sessionId = runtimeIdRef.current - if (!text) { + if (!text || !sessionId) { return false } + const messageId = `user-${Date.now()}-${Math.random().toString(36).slice(2, 8)}` + const mutate = (updater: (state: ClientSessionState) => ClientSessionState) => + sessionTileDelegate()?.updateSession(sessionId, updater) + + // Match the primary composer: insert the correction before the active + // reply before awaiting the redirect RPC, whose completion can race us. + mutate(state => { + const message = { + id: messageId, + role: 'user' as const, + parts: [textPart(text)] + } + const streamIndex = state.streamId + ? state.messages.findIndex(candidate => candidate.id === state.streamId) + : -1 + const lastAssistantIndex = state.messages.map(candidate => candidate.role).lastIndexOf('assistant') + const insertionIndex = streamIndex >= 0 ? streamIndex : lastAssistantIndex + const messages = + insertionIndex >= 0 + ? [...state.messages.slice(0, insertionIndex), message, ...state.messages.slice(insertionIndex)] + : [...state.messages, message] + + return { ...state, messages } + }) + + const discardOptimisticMessage = () => + mutate(state => ({ + ...state, + messages: state.messages.filter(message => message.id !== messageId) + })) + + const moveOptimisticMessageToEnd = () => + mutate(state => { + const message = state.messages.find(candidate => candidate.id === messageId) + + return message + ? { ...state, messages: [...state.messages.filter(candidate => candidate.id !== messageId), message] } + : state + }) + try { - const result = await requestGateway<{ status?: string }>('session.steer', { - session_id: runtimeIdRef.current, + const result = await requestGateway<{ status?: string }>('session.redirect', { + session_id: sessionId, text }) - if (result?.status === 'queued') { + if (result?.status === 'redirected') { + triggerHaptic('submit') + + return true + } + + if (result?.status === 'queued') { + moveOptimisticMessageToEnd() triggerHaptic('submit') - appendSystemNote(`steer:${text}`) return true } } catch { + discardOptimisticMessage() // Swallow — the caller queues the text so nothing is lost. + + return false } + discardOptimisticMessage() + return false }, - [appendSystemNote, requestGateway] + [requestGateway] ) // Rewind primitive (interrupt-first for live turns, busy-retry) — shared with