From dac2cac7c884f1f7278158bc37d709c3a56ca16e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 4 Oct 2026 05:07:41 -0700 Subject: [PATCH] fix(notes): a cleared send stops waiting for notes that can no longer appear Keys whose notes were not in the store yet stayed pending forever when the note never showed up: a second "delivered" ending for notes already cleared, a page closed or a workspace removed, or a note edited while its send was on the way. Each kept a store listener that scanned on every store write for the rest of the run. A key now waits only while its workspace has not loaded; once it has, a key with no note is dropped, and the listener detaches as soon as nothing is waiting. --- .../src/lib/notes-sent-by-chat.test.ts | 128 ++++++++++++++++++ src/renderer/src/lib/notes-sent-by-chat.ts | 59 +++++--- 2 files changed, 165 insertions(+), 22 deletions(-) create mode 100644 src/renderer/src/lib/notes-sent-by-chat.test.ts diff --git a/src/renderer/src/lib/notes-sent-by-chat.test.ts b/src/renderer/src/lib/notes-sent-by-chat.test.ts new file mode 100644 index 00000000000..5beecf63ab6 --- /dev/null +++ b/src/renderer/src/lib/notes-sent-by-chat.test.ts @@ -0,0 +1,128 @@ +// A used message's notes are cleared from their shelf, waiting only while their workspace has not +// loaded yet; nothing waits on a note that can no longer appear, and nothing keeps watching the +// store once nothing is waiting. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + diffComments: new Map(), + workspaceSessionReady: false, + storeListeners: new Set<() => void>(), + getDiffCommentsCalls: 0, + clearDeliveredDiffComments: vi.fn() +})) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => ({ + workspaceSessionReady: mocks.workspaceSessionReady, + getDiffComments: (worktreeId: string) => { + mocks.getDiffCommentsCalls += 1 + return mocks.diffComments.get(worktreeId) ?? [] + }, + clearDeliveredDiffComments: mocks.clearDeliveredDiffComments, + browserAnnotationsByPageId: {}, + removeDeliveredBrowserPageAnnotations: vi.fn() + }), + subscribe: (listener: () => void) => { + mocks.storeListeners.add(listener) + return () => mocks.storeListeners.delete(listener) + } + } +})) + +import { endStructuredAgentSessionEntry } from '@/components/native-chat/structured-agent-session-entry-endings' +import { browserAnnotationSendKey, diffCommentSendKey } from './notes-send-in-flight' +import { installNotesSentByChat } from './notes-sent-by-chat' + +const NOTE = { id: 'note-a', body: 'fix this', filePath: 'a.ts', lineNumber: 1 } +const KEY = diffCommentSendKey('wt', NOTE) + +function storeWrite(): void { + for (const listener of mocks.storeListeners) { + listener() + } +} + +function used(keys: string[], clientMessageId = 'm'): void { + endStructuredAgentSessionEntry( + { sessionId: 's', clientMessageId, carriedNoteKeys: keys }, + 'delivered' + ) +} + +let uninstall = (): void => {} + +beforeEach(() => { + mocks.diffComments = new Map([['wt', [NOTE]]]) + mocks.workspaceSessionReady = true + mocks.storeListeners.clear() + mocks.getDiffCommentsCalls = 0 + mocks.clearDeliveredDiffComments.mockReset() + mocks.clearDeliveredDiffComments.mockImplementation( + async (worktreeId: string, notes: unknown[]) => { + mocks.diffComments.set( + worktreeId, + (mocks.diffComments.get(worktreeId) ?? []).filter((note) => !notes.includes(note)) + ) + return true + } + ) + uninstall = installNotesSentByChat() +}) + +afterEach(() => uninstall()) + +describe('notes a used message carried', () => { + // A pending answer, then the journal's row: the same message ends delivered twice. + it('are cleared once, and a second ending for them adds nothing', () => { + used([KEY]) + used([KEY]) + expect(mocks.clearDeliveredDiffComments).toHaveBeenCalledExactlyOnceWith('wt', [NOTE]) + expect(mocks.storeListeners.size).toBe(0) + }) + + it('wait for a workspace that has not loaded, and are cleared once it does', () => { + mocks.workspaceSessionReady = false + mocks.diffComments = new Map() + used([KEY]) + expect(mocks.clearDeliveredDiffComments).not.toHaveBeenCalled() + expect(mocks.storeListeners.size).toBe(1) + + mocks.diffComments = new Map([['wt', [NOTE]]]) + storeWrite() + expect(mocks.clearDeliveredDiffComments).toHaveBeenCalledExactlyOnceWith('wt', [NOTE]) + expect(mocks.storeListeners.size).toBe(0) + }) + + it('are dropped when their note can no longer appear, and leave nothing watching', () => { + const edited = diffCommentSendKey('wt', { ...NOTE, body: 'what it said when sent' }) + const removedWorktree = diffCommentSendKey('removed-wt', NOTE) + const closedPage = browserAnnotationSendKey({ + browserPageId: 'closed-page', + id: 'a', + comment: 'c', + intent: 'fix' + }) + used([edited, removedWorktree, closedPage]) + expect(mocks.clearDeliveredDiffComments).not.toHaveBeenCalled() + expect(mocks.storeListeners.size).toBe(0) + }) + + it('stop being looked for once the session loads without them, so no write scans again', () => { + mocks.workspaceSessionReady = false + mocks.diffComments = new Map() + used([diffCommentSendKey('removed-wt', NOTE)]) + expect(mocks.storeListeners.size).toBe(1) + + mocks.workspaceSessionReady = true + storeWrite() + expect(mocks.storeListeners.size).toBe(0) + mocks.getDiffCommentsCalls = 0 + for (let write = 0; write < 100; write += 1) { + storeWrite() + } + expect(mocks.getDiffCommentsCalls).toBe(0) + expect(mocks.clearDeliveredDiffComments).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/lib/notes-sent-by-chat.ts b/src/renderer/src/lib/notes-sent-by-chat.ts index 2493902c8d3..0fa1a18c2a4 100644 --- a/src/renderer/src/lib/notes-sent-by-chat.ts +++ b/src/renderer/src/lib/notes-sent-by-chat.ts @@ -8,8 +8,10 @@ import { // Why: notes follow their text, so they have one owner. Once a chat message carrying them reaches // the host, or its text goes back to the composer, they are used and leave their shelf; only a -// message thrown away with nothing handed back leaves them there. A note not loaded yet when that -// happens (its workspace or page hydrates later) is cleared when it loads, never forgotten. +// message thrown away with nothing handed back leaves them there. A note whose workspace has not +// loaded yet when that happens is cleared when it loads. Once its owner is loaded, a key with no +// note (already cleared, edited since, or its workspace or page gone) is dropped: nothing waits on +// a note that can no longer appear. type PendingNotes = { kind: 'diff-comment' | 'browser-annotation' owner: string @@ -25,41 +27,54 @@ function ownerKey(kind: 'diff-comment' | 'browser-annotation', owner: string): s return JSON.stringify([kind, owner]) } -/** Clears the pending notes the store now holds, and forgets each one it cleared. */ +type StoreState = ReturnType + +/** Whether the owner's notes are in the store as they will be: a workspace's arrive with the + * session's hydration; a page's annotations live only in memory, so they are there or gone. */ +function ownerLoaded(state: StoreState, pending: PendingNotes): boolean { + return ( + pending.kind === 'browser-annotation' || + state.workspaceSessionReady || + state.getDiffComments(pending.owner).length > 0 + ) +} + +/** Clears the pending notes the store now holds; drops what a loaded owner no longer has. */ function clearLoadedNotes(): void { const state = useAppStore.getState() - for (const [ownerId, parsed] of pendingByOwner) { - const { keys } = parsed + for (const [ownerId, pending] of pendingByOwner) { + const { keys } = pending + const loaded = ownerLoaded(state, pending) const current = - parsed.kind === 'diff-comment' - ? state.getDiffComments(parsed.owner) - : state.browserAnnotationsByPageId[parsed.owner] - if (current === parsed.seen) { + pending.kind === 'diff-comment' + ? state.getDiffComments(pending.owner) + : state.browserAnnotationsByPageId[pending.owner] + if (!loaded && current === pending.seen) { continue } - parsed.seen = current - if (parsed.kind === 'diff-comment') { + pending.seen = current + if (pending.kind === 'diff-comment') { const notes = state - .getDiffComments(parsed.owner) - .filter((note) => keys.has(diffCommentSendKey(parsed.owner, note))) - for (const note of notes) { - keys.delete(diffCommentSendKey(parsed.owner, note)) - } + .getDiffComments(pending.owner) + .filter((note) => keys.has(diffCommentSendKey(pending.owner, note))) if (notes.length > 0) { - void state.clearDeliveredDiffComments(parsed.owner, notes) + void state.clearDeliveredDiffComments(pending.owner, notes) + } + for (const note of notes) { + keys.delete(diffCommentSendKey(pending.owner, note)) } } else { - const annotations = (state.browserAnnotationsByPageId[parsed.owner] ?? []).filter( + const annotations = (state.browserAnnotationsByPageId[pending.owner] ?? []).filter( (annotation) => keys.has(browserAnnotationSendKey(annotation)) ) + if (annotations.length > 0) { + state.removeDeliveredBrowserPageAnnotations(pending.owner, annotations) + } for (const annotation of annotations) { keys.delete(browserAnnotationSendKey(annotation)) } - if (annotations.length > 0) { - state.removeDeliveredBrowserPageAnnotations(parsed.owner, annotations) - } } - if (keys.size === 0) { + if (loaded || keys.size === 0) { pendingByOwner.delete(ownerId) } }