feat(chat): Comments and review in VS Code, like the OpenChamber desktop app (#1724)
* feat(chat): render code comments as cards instead of fenced text * fix(vscode): route Add Comment to the active session editor panel * fix(chat): persist queued inline comments and tighten file-chip path matching * feat(vscode): comment on code from the editor * fix(chat): keep attached context in the message and broadcast comment removal * fix(vscode): hold every pending comment and gate both entry points on the workspace * fix(vscode): let only the owning surface decide its comment threads * fix(vscode): drop a comment removed while its delivery was still in flight * test(vscode): cover the in-flight comment removal guard * test(vscode): cover comment removal reaching every chat surface * fix(vscode): give up on a comment the chat never confirmed holding * fix(vscode): retract a comment everywhere before reporting it discarded * fix(chat): preserve queued comment cards * fix: preserve inline comment context across send paths * fix(chat): preserve command routing with context * fix(chat): keep unavailable actions on normal send path --------- Co-authored-by: Bohdan Triapitsyn <artmore@protonmail.com>
This commit is contained in:
committed by
GitHub
co-authored by
Bohdan Triapitsyn
parent
d323b51a0a
commit
a12b9be443
@@ -84,6 +84,18 @@ export function sendBridgeMessage<T = unknown>(type: string, payload?: unknown):
|
||||
return sendBridgeMessageWithOptions<T>(type, payload);
|
||||
}
|
||||
|
||||
/**
|
||||
* Tells the extension something without waiting for an answer.
|
||||
*
|
||||
* Requests are tracked until a response arrives, so a message the extension
|
||||
* never replies to would leak a pending entry on every call. State the webview
|
||||
* pushes outward (editor comment threads following the composer's drafts) has
|
||||
* no answer to wait for, so it does not go through the request path at all.
|
||||
*/
|
||||
export function postBridgeNotification<Payload extends object>(type: string, payload: Payload): void {
|
||||
getVSCodeAPI().postMessage({ type, payload });
|
||||
}
|
||||
|
||||
export function sendBridgeMessageWithOptions<T = unknown>(
|
||||
type: string,
|
||||
payload?: unknown,
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
import { describe, test } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import { createRemovalTombstones } from './inlineCommentRemovals';
|
||||
|
||||
describe('inline comment removal tombstones', () => {
|
||||
test('a delivery arriving after its removal is dropped', () => {
|
||||
// The window this closes: the extension holds a payload for a booting
|
||||
// panel, or the handler waits for a directory, and the user removes the
|
||||
// thread meanwhile. Without this the draft lands as a chip they dropped.
|
||||
const tombstones = createRemovalTombstones();
|
||||
tombstones.remember('icd-1');
|
||||
assert.equal(tombstones.consume('icd-1'), true);
|
||||
});
|
||||
|
||||
test('an unrelated delivery is untouched', () => {
|
||||
const tombstones = createRemovalTombstones();
|
||||
tombstones.remember('icd-1');
|
||||
assert.equal(tombstones.consume('icd-2'), false);
|
||||
});
|
||||
|
||||
test('a delivery with no id is never dropped', () => {
|
||||
// Comments from other entry points carry no draft id.
|
||||
const tombstones = createRemovalTombstones();
|
||||
tombstones.remember('icd-1');
|
||||
assert.equal(tombstones.consume(undefined), false);
|
||||
});
|
||||
|
||||
test('the record is consumed, so only the delayed delivery is refused', () => {
|
||||
const tombstones = createRemovalTombstones();
|
||||
tombstones.remember('icd-1');
|
||||
tombstones.consume('icd-1');
|
||||
assert.equal(tombstones.consume('icd-1'), false);
|
||||
assert.equal(tombstones.size(), 0);
|
||||
});
|
||||
|
||||
test('remembering the same removal twice keeps one record', () => {
|
||||
const tombstones = createRemovalTombstones();
|
||||
tombstones.remember('icd-1');
|
||||
tombstones.remember('icd-1');
|
||||
assert.equal(tombstones.size(), 1);
|
||||
});
|
||||
|
||||
test('an empty id is not recorded', () => {
|
||||
const tombstones = createRemovalTombstones();
|
||||
tombstones.remember('');
|
||||
assert.equal(tombstones.size(), 0);
|
||||
});
|
||||
|
||||
test('the record is bounded, evicting the oldest first', () => {
|
||||
const tombstones = createRemovalTombstones(3);
|
||||
for (const id of ['a', 'b', 'c', 'd']) tombstones.remember(id);
|
||||
|
||||
assert.equal(tombstones.size(), 3);
|
||||
// 'a' aged out; the three most recent still refuse their deliveries.
|
||||
assert.equal(tombstones.consume('a'), false);
|
||||
assert.equal(tombstones.consume('d'), true);
|
||||
assert.equal(tombstones.consume('c'), true);
|
||||
assert.equal(tombstones.consume('b'), true);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,49 @@
|
||||
/**
|
||||
* Comments the user dropped before their draft reached this webview's store.
|
||||
*
|
||||
* Delivery is asynchronous on both sides: the extension holds a payload for a
|
||||
* panel that has not booted, and the handler that files the draft can wait
|
||||
* seconds for a directory to resolve. A removal can arrive anywhere in that
|
||||
* window, when there is no draft yet to remove. Recording it here lets the
|
||||
* delayed delivery recognise a comment that is no longer wanted, instead of
|
||||
* filing it as a chip the user already dropped and sending it with the next
|
||||
* message.
|
||||
*
|
||||
* Bounded because it is a tombstone list, not state: ids are unique per comment,
|
||||
* so entries are never revisited once their delivery window has passed.
|
||||
*/
|
||||
|
||||
const REMEMBERED_REMOVALS = 50;
|
||||
|
||||
export function createRemovalTombstones(limit: number = REMEMBERED_REMOVALS) {
|
||||
const ids = new Set<string>();
|
||||
|
||||
return {
|
||||
/** Records a removal, evicting the oldest once the bound is reached. */
|
||||
remember(draftId: string): void {
|
||||
if (!draftId) return;
|
||||
ids.add(draftId);
|
||||
if (ids.size > limit) {
|
||||
const oldest = ids.values().next();
|
||||
if (!oldest.done) ids.delete(oldest.value);
|
||||
}
|
||||
},
|
||||
|
||||
/**
|
||||
* Whether this delivery should be dropped.
|
||||
*
|
||||
* Consumes the record: the window closes once the delayed delivery has
|
||||
* been refused, and a later comment reusing the id would be unrelated.
|
||||
*/
|
||||
consume(draftId: string | undefined): boolean {
|
||||
if (!draftId || !ids.has(draftId)) return false;
|
||||
ids.delete(draftId);
|
||||
return true;
|
||||
},
|
||||
|
||||
/** Number of removals currently remembered. Exposed for tests. */
|
||||
size(): number {
|
||||
return ids.size;
|
||||
},
|
||||
};
|
||||
}
|
||||
@@ -1,5 +1,6 @@
|
||||
import { createVSCodeAPIs } from './api';
|
||||
import { onCommand, onThemeChange, proxyApiRequest, proxySessionMessageRequest, sendBridgeMessage, startSseProxy, stopSseProxy } from './api/bridge';
|
||||
import { createRemovalTombstones } from './inlineCommentRemovals';
|
||||
import { onCommand, onThemeChange, postBridgeNotification, proxyApiRequest, proxySessionMessageRequest, sendBridgeMessage, startSseProxy, stopSseProxy } from './api/bridge';
|
||||
import { vscodeStreamPerfCount, vscodeStreamPerfMeasure, vscodeStreamPerfObserve } from './api/streamPerf';
|
||||
import { extractBodyBase64, extractBodyText, extractJsonBody, hasInitBody } from './requestBodyTransport';
|
||||
import type { RuntimeAPIs } from '@openchamber/ui/lib/api/types';
|
||||
@@ -1329,6 +1330,159 @@ onCommand('addContextSelection', (payload) => {
|
||||
});
|
||||
});
|
||||
|
||||
// Comments dropped from their editor thread before the draft reached this
|
||||
// store. See the module for why the window exists.
|
||||
const removedComments = createRemovalTombstones();
|
||||
|
||||
onCommand('addLineComment', (payload) => {
|
||||
// SAFETY: the payload crossed the extension boundary as JSON; every field is
|
||||
// read as unknown here and trusted only after the checks below.
|
||||
const record = payload as {
|
||||
draftId?: unknown;
|
||||
filePath?: unknown;
|
||||
relativePath?: unknown;
|
||||
source?: unknown;
|
||||
side?: unknown;
|
||||
startLine?: unknown;
|
||||
endLine?: unknown;
|
||||
code?: unknown;
|
||||
language?: unknown;
|
||||
comment?: unknown;
|
||||
};
|
||||
|
||||
// The editor thread mints the id so it can track its own draft without a
|
||||
// round trip. Absent when the comment came from anywhere else.
|
||||
const draftId = typeof record.draftId === 'string' && record.draftId ? record.draftId : undefined;
|
||||
const relativePath = typeof record.relativePath === 'string' ? record.relativePath : '';
|
||||
const source = record.source === 'diff' ? 'diff' : 'file';
|
||||
const side = record.side === 'original' || record.side === 'modified' ? record.side : undefined;
|
||||
const startLine = typeof record.startLine === 'number' ? record.startLine : 1;
|
||||
const endLine = typeof record.endLine === 'number' ? record.endLine : startLine;
|
||||
const code = typeof record.code === 'string' ? record.code : '';
|
||||
const language = typeof record.language === 'string' ? record.language : 'text';
|
||||
const comment = typeof record.comment === 'string' ? record.comment.trim() : '';
|
||||
|
||||
if (!relativePath) {
|
||||
console.warn('[openchamber] inline comment arrived without a path; dropping', record);
|
||||
return;
|
||||
}
|
||||
|
||||
void Promise.all([
|
||||
import('@/sync/session-ui-store'),
|
||||
import('@/stores/useDirectoryStore'),
|
||||
import('@/stores/useInlineCommentDraftStore'),
|
||||
]).then(async ([{ useSessionUIStore }, { useDirectoryStore }, { useInlineCommentDraftStore }]) => {
|
||||
// Inline drafts are owned by runtime + directory + session. Both halves are
|
||||
// read together, from one store snapshot: read apart, a session that
|
||||
// finished loading between them would pair its key with the previous
|
||||
// session's directory, and the draft would land under a key ChatInput never
|
||||
// reads. Directory precedence matches the composer's own.
|
||||
const resolveTarget = () => {
|
||||
const sessionState = useSessionUIStore.getState();
|
||||
const currentSessionId = sessionState.currentSessionId;
|
||||
const sessionDirectory = currentSessionId ? sessionState.getDirectoryForSession(currentSessionId) : null;
|
||||
const draftDirectory = sessionState.newSessionDraft?.open
|
||||
? sessionState.newSessionDraft.bootstrapPendingDirectory ?? sessionState.newSessionDraft.directoryOverride ?? null
|
||||
: null;
|
||||
const directory = sessionDirectory ?? draftDirectory ?? useDirectoryStore.getState().currentDirectory;
|
||||
return directory ? { directory, sessionKey: currentSessionId ?? 'draft' } : null;
|
||||
};
|
||||
|
||||
// A comment can arrive before the chat surface has finished booting: the
|
||||
// extension opens the sidebar and posts after a fixed delay, which a cold
|
||||
// webview can outlast. Dropping the draft here loses a comment the user
|
||||
// already wrote and already saw accepted in the editor, so wait for the
|
||||
// target to land instead.
|
||||
let target = resolveTarget();
|
||||
for (let attempt = 0; !target && attempt < 40; attempt += 1) {
|
||||
await new Promise((resolve) => setTimeout(resolve, 250));
|
||||
target = resolveTarget();
|
||||
}
|
||||
if (!target) {
|
||||
console.warn('[openchamber] no directory resolved; dropping inline comment', { relativePath, startLine });
|
||||
return;
|
||||
}
|
||||
|
||||
// Checked after the wait, which is the window the removal can land in.
|
||||
if (removedComments.consume(draftId)) {
|
||||
return;
|
||||
}
|
||||
|
||||
useInlineCommentDraftStore.getState().addDraft(target, {
|
||||
id: draftId,
|
||||
source,
|
||||
fileLabel: relativePath,
|
||||
startLine,
|
||||
endLine,
|
||||
side,
|
||||
code,
|
||||
language,
|
||||
text: comment,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// The editor's comment threads mirror the composer's drafts, so every change to
|
||||
// the draft store is reported as a whole snapshot. Sending the full list rather
|
||||
// than add/remove events means a dropped notification cannot leave a thread
|
||||
// anchored to a comment that is no longer attached; sending the message empties
|
||||
// the list, which clears the threads through the same path.
|
||||
void import('@/stores/useInlineCommentDraftStore').then(({ useInlineCommentDraftStore }) => {
|
||||
let lastSignature = '';
|
||||
|
||||
const publish = (drafts: Record<string, Array<{ id: string; text: string }>>) => {
|
||||
const flat = Object.values(drafts)
|
||||
.flat()
|
||||
.map((draft) => ({ id: draft.id, text: draft.text }));
|
||||
const signature = JSON.stringify(flat);
|
||||
if (signature === lastSignature) return;
|
||||
lastSignature = signature;
|
||||
postBridgeNotification('inlineComments:sync', { drafts: flat });
|
||||
};
|
||||
|
||||
publish(useInlineCommentDraftStore.getState().drafts);
|
||||
useInlineCommentDraftStore.subscribe((state) => publish(state.drafts));
|
||||
});
|
||||
|
||||
onCommand('removeLineComment', (payload) => {
|
||||
if (typeof payload !== 'object' || payload === null || !('draftId' in payload)) return;
|
||||
const { draftId } = payload;
|
||||
if (typeof draftId !== 'string' || !draftId) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Recorded even when the draft is already here: the store removal below is
|
||||
// the normal path, and this only matters when the draft has not landed yet.
|
||||
removedComments.remember(draftId);
|
||||
|
||||
void Promise.all([
|
||||
import('@/stores/useInlineCommentDraftStore'),
|
||||
import('@/lib/runtime-switch'),
|
||||
]).then(([{ useInlineCommentDraftStore }, { getRuntimeKey }]) => {
|
||||
const state = useInlineCommentDraftStore.getState();
|
||||
const runtimeKey = getRuntimeKey();
|
||||
|
||||
// The thread knows its draft id but not which target holds it. Search for
|
||||
// the owning key, and only within the current runtime: `removeDraft`
|
||||
// recomputes the key from the live runtime, so a target rebuilt from
|
||||
// another runtime's key would delete from the wrong place.
|
||||
for (const [key, drafts] of Object.entries(state.drafts)) {
|
||||
if (!drafts.some((draft) => draft.id === draftId)) continue;
|
||||
let parsed: unknown;
|
||||
try {
|
||||
parsed = JSON.parse(key);
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
if (!Array.isArray(parsed) || parsed.length !== 3 || !parsed.every((segment) => typeof segment === 'string')) continue;
|
||||
const [keyRuntime, directory, sessionKey] = parsed;
|
||||
if (keyRuntime !== runtimeKey) continue;
|
||||
state.removeDraft({ directory, sessionKey }, draftId);
|
||||
return;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
onCommand('addFileMentions', (payload) => {
|
||||
const rawPaths = Array.isArray((payload as { paths?: unknown[] })?.paths)
|
||||
? (payload as { paths: unknown[] }).paths
|
||||
|
||||
Reference in New Issue
Block a user