mirror of
https://github.com/stablyai/orca.git
synced 2026-10-07 16:02:29 +00:00
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.
This commit is contained in:
@@ -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<string, unknown[]>(),
|
||||
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()
|
||||
})
|
||||
})
|
||||
@@ -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<typeof useAppStore.getState>
|
||||
|
||||
/** 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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user