From ae0b9f3fea67d3073be8c8336335ad7527ed65b1 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 4 Oct 2026 04:01:07 -0700 Subject: [PATCH] fix(notes): release a gone chat's notes, and clear sent ones in the web client too Two gaps in keeping notes out of another send while a saved chat message carries them: - The web client never installed the clearing that removes notes once a chat sends them on, so after a Retry there the notes stayed listed. It is now installed, once per renderer, by the background services both the desktop and the web client load with App. - A saved message carrying notes was thrown away only by this window's own close. A chat closed from the phone, another window or while Orca was off, or closed here without a known host, kept its notes held for good. A chat the host stops listing (affirmed, and not a launch still starting or failed) now has its queued messages thrown away once that sync lands, as does a close that can name no host, so the notes come back. --- .../src/app-shell/AppBackgroundServices.tsx | 5 + .../notes-delivered-by-chat-install.test.ts | 103 ++++++++++ .../src/lib/notes-delivered-by-chat.ts | 16 +- src/renderer/src/main.tsx | 2 - .../apply-preparation-browser.ts | 18 +- .../host-removed-structured-chats.test.ts | 189 ++++++++++++++++++ .../host-removed-structured-chats.ts | 13 ++ .../web-session-tabs-sync/store-patch.ts | 9 + .../tabs/structured-chat-close-owner.test.ts | 20 +- .../store/slices/tabs/tabs-close-actions.ts | 5 +- 10 files changed, 368 insertions(+), 12 deletions(-) create mode 100644 src/renderer/src/app-shell/notes-delivered-by-chat-install.test.ts create mode 100644 src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.test.ts create mode 100644 src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.ts diff --git a/src/renderer/src/app-shell/AppBackgroundServices.tsx b/src/renderer/src/app-shell/AppBackgroundServices.tsx index ca22678decf..caea319fd9d 100644 --- a/src/renderer/src/app-shell/AppBackgroundServices.tsx +++ b/src/renderer/src/app-shell/AppBackgroundServices.tsx @@ -8,6 +8,11 @@ import { MacosTccPromptNoticeHost } from '../hooks/MacosTccPromptNoticeHost' import { useAppStore } from '../store' import { StructuredAgentSessionAttentionBridge } from '../components/native-chat/StructuredAgentSessionAttentionBridge' import { StructuredAgentSessionStatusBridge } from '../components/native-chat/StructuredAgentSessionStatusBridge' +import { installNotesDeliveredByChat } from '../lib/notes-delivered-by-chat' + +// Why at load and not in an effect: both the desktop and the web client load this with App, before +// any chat can send its saved message. +installNotesDeliveredByChat() const DashboardPopoutBridge = lazy(() => import('../components/dashboard/DashboardPopoutBridge')) diff --git a/src/renderer/src/app-shell/notes-delivered-by-chat-install.test.ts b/src/renderer/src/app-shell/notes-delivered-by-chat-install.test.ts new file mode 100644 index 00000000000..36455b4f4ae --- /dev/null +++ b/src/renderer/src/app-shell/notes-delivered-by-chat-install.test.ts @@ -0,0 +1,103 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { DiffComment } from '../../../shared/diff-comment-types' +import type { StructuredAgentSessionOutboxEntry } from '../../../shared/structured-agent-session-outbox' + +const mocks = vi.hoisted(() => ({ clearDelivered: vi.fn() })) + +const note: DiffComment = { + id: 'note-a', + worktreeId: 'wt-1', + filePath: 'README.md', + lineNumber: 3, + body: 'tighten this', + createdAt: 1, + side: 'modified' +} + +vi.mock('../store', () => ({ + useAppStore: Object.assign(() => false, { + getState: () => ({ + getDiffComments: () => [note], + clearDeliveredDiffComments: mocks.clearDelivered, + browserAnnotationsByPageId: {}, + removeDeliveredBrowserPageAnnotations: vi.fn() + }) + }) +})) +vi.mock('../components/AgentHibernationGate', () => ({ AgentHibernationGate: () => null })) +vi.mock('../components/AiVaultTabTitleSyncGate', () => ({ AiVaultTabTitleSyncGate: () => null })) +vi.mock('../components/dashboard/RetainedAgentsSyncGate', () => ({ default: () => null })) +vi.mock('../components/ports/WorkspacePortScanner', () => ({ WorkspacePortScanner: () => null })) +vi.mock('../hooks/MacosTccPromptNoticeHost', () => ({ MacosTccPromptNoticeHost: () => null })) +vi.mock('../components/native-chat/StructuredAgentSessionAttentionBridge', () => ({ + StructuredAgentSessionAttentionBridge: () => null +})) +vi.mock('../components/native-chat/StructuredAgentSessionStatusBridge', () => ({ + StructuredAgentSessionStatusBridge: () => null +})) + +const RENDERER = join(__dirname, '..') + +function carrying(keys: string[]): StructuredAgentSessionOutboxEntry { + return { + clientMessageId: 'message-1', + sessionId: 'chat-1', + body: { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'review notes' }] }, + previewUris: [], + state: 'rejected', + queuedAt: 1, + lastAttemptAt: null, + retryAfterUnknownSubmittedAt: null, + source: 'launch', + carriedNoteKeys: keys + } +} + +describe('clearing notes a chat sends on', () => { + beforeEach(() => { + vi.resetModules() + mocks.clearDelivered.mockReset() + }) + + // The desktop and the web client both render App, which loads its background services. + it('is installed by the background services both app entries render', async () => { + expect(readFileSync(join(RENDERER, 'main.tsx'), 'utf8')).toContain("import App from './App'") + expect(readFileSync(join(RENDERER, 'web/main.tsx'), 'utf8')).toContain("import('../App')") + expect(readFileSync(join(RENDERER, 'App.tsx'), 'utf8')).toContain( + "from './app-shell/AppBackgroundServices'" + ) + + await import('./AppBackgroundServices') + const carried = + await import('../components/native-chat/structured-agent-session-outbox-carried-notes') + const { diffCommentSendKey } = await import('../lib/notes-send-in-flight') + carried.recordStructuredAgentSessionCarriedNotes( + 'chat-1', + [carrying([diffCommentSendKey(note)])], + [], + 'spent' + ) + + expect(mocks.clearDelivered).toHaveBeenCalledWith('wt-1', [note]) + }) + + it('runs once per renderer however often it is installed', async () => { + const { installNotesDeliveredByChat } = await import('../lib/notes-delivered-by-chat') + const carried = + await import('../components/native-chat/structured-agent-session-outbox-carried-notes') + const { diffCommentSendKey } = await import('../lib/notes-send-in-flight') + installNotesDeliveredByChat() + installNotesDeliveredByChat() + + carried.recordStructuredAgentSessionCarriedNotes( + 'chat-1', + [carrying([diffCommentSendKey(note)])], + [], + 'spent' + ) + + expect(mocks.clearDelivered).toHaveBeenCalledOnce() + }) +}) diff --git a/src/renderer/src/lib/notes-delivered-by-chat.ts b/src/renderer/src/lib/notes-delivered-by-chat.ts index 15872b31e19..43839295532 100644 --- a/src/renderer/src/lib/notes-delivered-by-chat.ts +++ b/src/renderer/src/lib/notes-delivered-by-chat.ts @@ -6,10 +6,15 @@ import { noteSendKeyOwner } from './notes-send-in-flight' +let uninstall: (() => void) | null = null + /** A new chat's message sent on, by its own start, a Retry or a re-check, clears the notes it - * carries from their shelf as a delivered send does, including after a reload. */ -export function installNotesDeliveredByChat(): () => void { - return subscribeToStructuredAgentSessionCarriedNotesSpent((keys) => { + * carries from their shelf as a delivered send does, including after a reload. Once per renderer. */ +export function installNotesDeliveredByChat(): void { + if (uninstall) { + return + } + uninstall = subscribeToStructuredAgentSessionCarriedNotesSpent((keys) => { const sent = new Set(keys) const state = useAppStore.getState() const worktreeIds = new Set() @@ -40,3 +45,8 @@ export function installNotesDeliveredByChat(): () => void { } }) } + +import.meta.hot?.dispose(() => { + uninstall?.() + uninstall = null +}) diff --git a/src/renderer/src/main.tsx b/src/renderer/src/main.tsx index b70b662dc8a..36239d52516 100644 --- a/src/renderer/src/main.tsx +++ b/src/renderer/src/main.tsx @@ -24,13 +24,11 @@ import { getOrCreateRendererRoot } from './lib/react-renderer-root' import { primeTerminalWebglAddon } from './lib/pane-manager/pane-webgl-renderer' import { SkillWarningPreviewLauncher } from './components/skills/SkillWarningPreviewLauncher' import { installBrowserClientPageRenderer } from './components/browser-pane/browser-client-page-renderer-installation' -import { installNotesDeliveredByChat } from './lib/notes-delivered-by-chat' recordRendererCrashBreadcrumb('renderer_bootstrap_started', { dev: import.meta.env.DEV }) installRendererCrashDiagnostics() installTypingLatencyDiagnostic() installAutomationHostDiagnostic() -installNotesDeliveredByChat() if ( import.meta.env.DEV && diff --git a/src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-browser.ts b/src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-browser.ts index 678b915d49c..c15105b80f0 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-browser.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/apply-preparation-browser.ts @@ -21,6 +21,7 @@ import { } from './state-equality-files' import { shouldRetainStructuredAgentSessionLaunchTab } from '@/lib/structured-agent-session-launch-registry' import { executionHostIdForSessionTabsOwner } from '../local-structured-session-owner' +import { noteHostRemovedStructuredChat } from './host-removed-structured-chats' export function prepareWebSessionTabsSnapshotBrowser( base: ReturnType @@ -184,11 +185,18 @@ export function prepareWebSessionTabsSnapshotBrowser( // A matching host row is authoritative; retaining the provisional tab beside its mirror // would briefly render two panes before lifecycle publication is recorded. // A host that cannot list its chats is no evidence this one closed. - return ( - !publishedAgentSessionIds.has(tab.entityId) && - (!agentSessionsAffirmed || - shouldRetainStructuredAgentSessionLaunchTab(worktreeId, tab.entityId)) - ) + if (publishedAgentSessionIds.has(tab.entityId)) { + return false + } + if ( + !agentSessionsAffirmed || + shouldRetainStructuredAgentSessionLaunchTab(worktreeId, tab.entityId) + ) { + return true + } + // Listed before, affirmed absent now: the chat itself is gone, not a launch still starting. + noteHostRemovedStructuredChat(tab.entityId) + return false } if (tab.contentType === 'browser') { return ( diff --git a/src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.test.ts b/src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.test.ts new file mode 100644 index 00000000000..f3b6ef3a1df --- /dev/null +++ b/src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.test.ts @@ -0,0 +1,189 @@ +// @vitest-environment happy-dom +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Tab } from '../../../../shared/tab-types' +import type { WebSessionTabsSyncState } from './state' + +const store = vi.hoisted(() => { + const state: Record = {} + return { state } +}) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => ({ ...store.state, scheduleAgentStatusFreshness: () => undefined }), + setState: (update: (state: Record) => Record) => { + store.state = { ...store.state, ...update(store.state) } + } + } +})) + +vi.mock('@/lib/launch-structured-agent-session', () => { + class StructuredAgentSessionCreateRefusalError extends Error {} + return { + createStructuredAgentSessionLaunchIntent: (worktreeId: string) => ({ + worktreeId, + sessionId: 'launch-1', + executionHostId: 'local', + target: { kind: 'local' }, + agent: 'codex', + params: { + envelope: { + sessionId: 'launch-1', + clientOperationId: 'operation-launch-1', + expectedRuntimeFence: null, + payloadFingerprint: 'fingerprint-launch-1' + }, + worktree: `id:${worktreeId}`, + agent: 'codex' + } + }), + retryStructuredAgentSessionLaunchIntent: (intent: unknown) => intent, + restoreStructuredAgentSessionLaunchIntent: vi.fn(), + abandonStructuredAgentSessionLaunchIntent: vi.fn(), + launchStructuredAgentSession: () => + Promise.reject(new StructuredAgentSessionCreateRefusalError('unsupported')), + StructuredAgentSessionCreateRefusalError + } +}) + +import { applyWebSessionTabsSnapshot } from '../web-session-tabs-sync' +import { applyWebSessionTabsStorePatch } from './store-patch' +import { + makeSnapshot, + makeState, + resetWebSessionTabsSyncTestState, + ENV, + NOW, + WT +} from '../web-session-tabs-sync-test-harness' +import { + appendStructuredAgentSessionOutboxMessage, + readOutbox +} from '@/components/native-chat/structured-agent-session-outbox-storage' +import { resetStructuredAgentSessionCarriedNotesForTests } from '@/components/native-chat/structured-agent-session-outbox-carried-notes' +import { isNoteInFlight } from '@/lib/notes-send-in-flight' +import { resetStructuredAgentLaunchRegistryForTests } from '@/lib/structured-agent-session-launch-registry' +import { resetStructuredAgentLaunchPersistenceForTests } from '@/lib/structured-agent-session-launch-persistence' +import { startStructuredAgentLaunch } from '@/lib/structured-agent-session-launch' +import { takeHostRemovedStructuredChats } from './host-removed-structured-chats' + +const NOTE_KEY = 'note-a' + +function chatTab(sessionId: string, id = `tab-${sessionId}`): Tab { + return { + id, + entityId: sessionId, + contentType: 'agent-session', + agentSessionAgent: 'codex', + worktreeId: WT, + groupId: 'group-1', + label: 'Codex', + customLabel: null, + color: null, + createdAt: 1, + sortOrder: 0 + } +} + +function windowWith(tabs: Tab[]): WebSessionTabsSyncState { + return makeState({ + unifiedTabsByWorktree: { [WT]: tabs }, + groupsByWorktree: { + [WT]: [ + { + id: 'group-1', + worktreeId: WT, + tabOrder: tabs.map((tab) => tab.id), + activeTabId: tabs[0]?.id ?? null + } + ] + }, + activeGroupIdByWorktree: { [WT]: 'group-1' } + }) +} + +/** A chat whose notes message the host refused: it waits on its Retry, carrying the notes. */ +function refusedNotesMessage(sessionId: string): void { + appendStructuredAgentSessionOutboxMessage(sessionId, 'review notes', [], 'launch', [NOTE_KEY]) +} + +/** The host's next list of chats, as this window's sync commits it. */ +function hostLists(sessions: string[]): void { + applyWebSessionTabsStorePatch( + (state) => + applyWebSessionTabsSnapshot( + state, + makeSnapshot( + sessions.map((sessionId) => ({ + type: 'agent-session' as const, + id: `agent-session:${sessionId}`, + sessionId, + agent: 'codex' as const, + title: 'Codex', + isActive: false + })), + { snapshotVersion: 2 } + ), + ENV, + NOW, + { contentScope: 'agent-session', preserveLocalLayout: true, terminalPtyMode: 'local' } + ), + { frames: [] } + ) +} + +describe('a chat the host stops listing', () => { + beforeEach(() => { + resetWebSessionTabsSyncTestState() + localStorage.clear() + resetStructuredAgentSessionCarriedNotesForTests() + resetStructuredAgentLaunchRegistryForTests() + resetStructuredAgentLaunchPersistenceForTests() + takeHostRemovedStructuredChats() + }) + + it('takes its queued messages with it, so the notes they carried come back', () => { + store.state = { ...windowWith([chatTab('chat-1')]) } + refusedNotesMessage('chat-1') + expect(isNoteInFlight(NOTE_KEY)).toBe(true) + + hostLists([]) + + expect(readOutbox('chat-1')).toEqual([]) + expect(isNoteInFlight(NOTE_KEY)).toBe(false) + }) + + it('keeps a chat the host still lists, under another local tab id', () => { + store.state = { ...windowWith([chatTab('chat-1', 'chat-1:history-1')]) } + refusedNotesMessage('chat-1') + + hostLists(['chat-1']) + + expect(readOutbox('chat-1')).toHaveLength(1) + expect(isNoteInFlight(NOTE_KEY)).toBe(true) + }) + + it("keeps a failed launch's messages for its Retry: the host never listed it", async () => { + const failed = startFailedLaunch() + await settle() + store.state = { ...windowWith([chatTab(failed)]) } + + hostLists([]) + + expect(readOutbox(failed)).toHaveLength(1) + expect(isNoteInFlight(NOTE_KEY)).toBe(true) + }) +}) + +function startFailedLaunch(): string { + return startStructuredAgentLaunch(WT, 'codex', { + prompt: 'review notes', + carriedNoteKeys: [NOTE_KEY] + }).sessionId +} + +async function settle(): Promise { + for (let i = 0; i < 20; i += 1) { + await Promise.resolve() + } +} diff --git a/src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.ts b/src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.ts new file mode 100644 index 00000000000..e3ae45fc06c --- /dev/null +++ b/src/renderer/src/runtime/web-session-tabs-sync/host-removed-structured-chats.ts @@ -0,0 +1,13 @@ +// Why: a chat the host stopped listing is gone, wherever it was closed (the phone, another window, +// while Orca was off). Noted while a sync patch is prepared, acted on only once that patch lands. +const removedSessionIds = new Set() + +export function noteHostRemovedStructuredChat(sessionId: string): void { + removedSessionIds.add(sessionId) +} + +export function takeHostRemovedStructuredChats(): string[] { + const sessionIds = [...removedSessionIds] + removedSessionIds.clear() + return sessionIds +} diff --git a/src/renderer/src/runtime/web-session-tabs-sync/store-patch.ts b/src/renderer/src/runtime/web-session-tabs-sync/store-patch.ts index db2758d4bbe..1f45178027c 100644 --- a/src/renderer/src/runtime/web-session-tabs-sync/store-patch.ts +++ b/src/renderer/src/runtime/web-session-tabs-sync/store-patch.ts @@ -17,6 +17,8 @@ import { type HostSessionMirrorPatchVerdict, type HostSessionMirrorSettle } from './mirror-settle' +import { takeHostRemovedStructuredChats } from './host-removed-structured-chats' +import { discardStructuredAgentSessionLaunchOutbox } from '@/components/native-chat/structured-agent-session-outbox-storage' /** Commit a reconciliation patch and return a receipt for its host evidence. */ export function applyWebSessionTabsStorePatch( @@ -149,10 +151,13 @@ export function applyWebSessionTabsStorePatch( return patch } + // Only what this patch's own preparation notes. + takeHostRemovedStructuredChats() try { useAppStore.setState(runStorePatch) } catch (error) { if (!patchCommitted) { + takeHostRemovedStructuredChats() throw error } console.warn('[web-session-tabs-sync] a store subscriber failed after the patch landed:', error) @@ -160,6 +165,10 @@ export function applyWebSessionTabsStorePatch( const settleHostMirror = createHostSessionMirrorSettle(hostMirrorVerdict) try { + // A chat gone from its host takes its queued messages, and the notes they held, with it. + for (const sessionId of takeHostRemovedStructuredChats()) { + discardStructuredAgentSessionLaunchOutbox(sessionId) + } if (mirroredAgentStatusChanged) { useAppStore.getState().scheduleAgentStatusFreshness() } diff --git a/src/renderer/src/store/slices/tabs/structured-chat-close-owner.test.ts b/src/renderer/src/store/slices/tabs/structured-chat-close-owner.test.ts index 0c9e658cac1..54df71490c5 100644 --- a/src/renderer/src/store/slices/tabs/structured-chat-close-owner.test.ts +++ b/src/renderer/src/store/slices/tabs/structured-chat-close-owner.test.ts @@ -1,8 +1,9 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { Tab } from '../../../../../shared/tab-types' import { createTestStore, makeWorktree, seedStore } from '../store-test-helpers' +import type * as OutboxStorageModule from '@/components/native-chat/structured-agent-session-outbox-storage' -const mocks = vi.hoisted(() => ({ beginClose: vi.fn() })) +const mocks = vi.hoisted(() => ({ beginClose: vi.fn(), discardOutbox: vi.fn() })) vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn(), warning: vi.fn() } @@ -10,6 +11,13 @@ vi.mock('sonner', () => ({ vi.mock('@/runtime/structured-agent-session-tab-retirement', () => ({ beginStructuredAgentSessionTabClose: mocks.beginClose })) +vi.mock( + '@/components/native-chat/structured-agent-session-outbox-storage', + async (importOriginal) => ({ + ...(await importOriginal()), + discardStructuredAgentSessionLaunchOutbox: mocks.discardOutbox + }) +) // `repoId::path` names both checkouts: this machine's and the paired server's. const WORKTREE = 'repo-1::/work/app' @@ -58,6 +66,7 @@ function storeWith(tab: Tab): ReturnType { beforeEach(() => { mocks.beginClose.mockReset() + mocks.discardOutbox.mockReset() }) describe('closing a structured chat from outside its workspace', () => { @@ -82,4 +91,13 @@ describe('closing a structured chat from outside its workspace', () => { expect(mocks.beginClose).not.toHaveBeenCalled() expect(store.getState().unifiedTabsByWorktree[WORKTREE] ?? []).toEqual([]) }) + + // Its queued messages go with it, as for any close, so notes they carried come back. + it('throws away the queued messages of a chat it can name no host for', () => { + const store = storeWith(chatTab()) + + store.getState().closeUnifiedTab('agent-session:chat-1') + + expect(mocks.discardOutbox).toHaveBeenCalledWith('chat-1') + }) }) diff --git a/src/renderer/src/store/slices/tabs/tabs-close-actions.ts b/src/renderer/src/store/slices/tabs/tabs-close-actions.ts index f4cd23a01d7..a2edefb0f95 100644 --- a/src/renderer/src/store/slices/tabs/tabs-close-actions.ts +++ b/src/renderer/src/store/slices/tabs/tabs-close-actions.ts @@ -9,6 +9,7 @@ import { } from '../tab-group-state' import { buildActiveSurfacePatch } from './tabs-surface' import { beginStructuredAgentSessionTabClose } from '@/runtime/structured-agent-session-tab-retirement' +import { discardStructuredAgentSessionLaunchOutbox } from '@/components/native-chat/structured-agent-session-outbox-storage' import { hasStructuredAgentSessionLaunchCancellationTombstone, shouldRetainStructuredAgentSessionLaunchTab @@ -69,8 +70,10 @@ export function createTabsCloseActions( provisional }) } else { - // Closing still removes the tab; no host can be named to stop its chat on. + // Closing still removes the tab; no host can be named to stop its chat on. Its queued + // messages go with it, as for any close, so notes they carried return to their shelf. console.warn('[structured-agent-session] close found no owning host', tab.entityId) + discardStructuredAgentSessionLaunchOutbox(tab.entityId) } get().clearNativeChatLaunchDraft(structuredAgentSessionTabId(tab.entityId)) }