fix(proposals): reload-safe optimisticApply + reconciled toolbar summary (#64)
Final-review fixes: - CRITICAL: optimisticApply no longer re-captures `original` from an already-applied buffer after a window reload (in-memory `applied` set is empty post-reload). A proposal already carrying `original` is marked applied and skipped, so Reject's revert target survives save+reload (INV-51/54). Adds a reload-safety host-E2E that reproduces the prior-session state. - MINOR: the preview toolbar +N/-N summary now counts against the landed text (current minus pending proposals) via the new pure landedTextOf(), so a pending change is shown once (as a proposal), not double-counted (INV-50). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -315,6 +315,17 @@ export class ProposalController implements vscode.Disposable {
|
|||||||
const proposal = state.artifact.proposals.find((p) => p.id === proposalId);
|
const proposal = state.artifact.proposals.find((p) => p.id === proposalId);
|
||||||
const fp = proposal ? state.artifact.anchors[proposal.anchorId]?.fingerprint : undefined;
|
const fp = proposal ? state.artifact.anchors[proposal.anchorId]?.fingerprint : undefined;
|
||||||
if (!proposal || !fp) return false;
|
if (!proposal || !fp) return false;
|
||||||
|
// Reload-safety (INV-51/54): a proposal that already carries `original` was
|
||||||
|
// optimistically applied in a PRIOR session — the buffer holds the applied text
|
||||||
|
// and `fp` points at it, but this (fresh) controller's in-memory `applied` set is
|
||||||
|
// empty. Re-applying would recapture `original` from the already-applied buffer
|
||||||
|
// (= the replacement) and CLOBBER the true revert target, breaking Reject. Mark it
|
||||||
|
// applied in memory and stop — `original` is captured exactly once, on first apply.
|
||||||
|
if (proposal.original !== undefined) {
|
||||||
|
state.applied.add(proposalId);
|
||||||
|
this.renderAll(document);
|
||||||
|
return true;
|
||||||
|
}
|
||||||
const resolved = resolve(document.getText(), fp);
|
const resolved = resolve(document.getText(), fp);
|
||||||
if (resolved === "orphaned") return false;
|
if (resolved === "orphaned") return false;
|
||||||
const original = document.getText(
|
const original = document.getText(
|
||||||
|
|||||||
@@ -791,6 +791,26 @@ function renderReviewOp(
|
|||||||
* INV-34). Deterministic: proposals in the same block are ordered by anchorStart
|
* INV-34). Deterministic: proposals in the same block are ordered by anchorStart
|
||||||
* then id; trailing proposals keep input order.
|
* then id; trailing proposals keep input order.
|
||||||
*/
|
*/
|
||||||
|
/**
|
||||||
|
* F12/#64 (INV-50): `currentText` with every resolved pending proposal's applied
|
||||||
|
* span reverted to its original (`replaced`) — the "landed" text the baseline diff
|
||||||
|
* should run against, so a pending proposal renders ONCE (as a proposal), never also
|
||||||
|
* as a landed change. Reverts high→low so earlier offsets stay valid. Pure. The
|
||||||
|
* preview's summary tally diffs against this too, so the toolbar count matches the
|
||||||
|
* body (a pending change is not double-counted as both a landed add/remove and a
|
||||||
|
* proposal).
|
||||||
|
*/
|
||||||
|
export function landedTextOf(currentText: string, proposals: ProposalView[]): string {
|
||||||
|
const pendingApplied = proposals
|
||||||
|
.filter((p) => p.anchorStart !== null)
|
||||||
|
.sort((a, b) => b.anchorStart! - a.anchorStart!);
|
||||||
|
let landedText = currentText;
|
||||||
|
for (const p of pendingApplied) {
|
||||||
|
landedText = landedText.slice(0, p.anchorStart!) + p.replaced + landedText.slice(p.anchorEnd!);
|
||||||
|
}
|
||||||
|
return landedText;
|
||||||
|
}
|
||||||
|
|
||||||
export function renderReview(
|
export function renderReview(
|
||||||
baselineText: string,
|
baselineText: string,
|
||||||
currentText: string,
|
currentText: string,
|
||||||
@@ -802,16 +822,12 @@ export function renderReview(
|
|||||||
|
|
||||||
// F12/#64 (INV-50): with optimistic apply the proposed text is already in
|
// F12/#64 (INV-50): with optimistic apply the proposed text is already in
|
||||||
// `currentText`, so a naive baseline→current diff would render each proposed
|
// `currentText`, so a naive baseline→current diff would render each proposed
|
||||||
// change BOTH as a landed diff and as its proposal block. Diff against
|
// change BOTH as a landed diff and as its proposal block. Diff against the
|
||||||
// `currentText` with every resolved pending proposal reverted to its original,
|
// "landed" text (current minus pending proposals) so they render once.
|
||||||
// so the landed diff excludes pending proposals; they render once, as proposals.
|
|
||||||
const pendingApplied = proposals
|
const pendingApplied = proposals
|
||||||
.filter((p) => p.anchorStart !== null)
|
.filter((p) => p.anchorStart !== null)
|
||||||
.sort((a, b) => b.anchorStart! - a.anchorStart!);
|
.sort((a, b) => b.anchorStart! - a.anchorStart!);
|
||||||
let landedText = currentText;
|
const landedText = landedTextOf(currentText, proposals);
|
||||||
for (const p of pendingApplied) {
|
|
||||||
landedText = landedText.slice(0, p.anchorStart!) + p.replaced + landedText.slice(p.anchorEnd!);
|
|
||||||
}
|
|
||||||
|
|
||||||
const ranges = splitBlocksWithRanges(landedText);
|
const ranges = splitBlocksWithRanges(landedText);
|
||||||
const ops = diffBlocks(baselineText, landedText);
|
const ops = diffBlocks(baselineText, landedText);
|
||||||
|
|||||||
@@ -13,7 +13,7 @@ import * as vscode from "vscode";
|
|||||||
import type { DiffViewController } from "./diffViewController";
|
import type { DiffViewController } from "./diffViewController";
|
||||||
import type { AttributionController } from "./attributionController";
|
import type { AttributionController } from "./attributionController";
|
||||||
import type { ProposalController } from "./proposalController";
|
import type { ProposalController } from "./proposalController";
|
||||||
import { renderReview, renderPlain, diffBlocks, diffToBlockHunks, type BlockOp } from "./trackChangesModel";
|
import { renderReview, renderPlain, diffBlocks, diffToBlockHunks, landedTextOf, type BlockOp } from "./trackChangesModel";
|
||||||
import { buildFingerprint } from "./anchorer";
|
import { buildFingerprint } from "./anchorer";
|
||||||
import { isAuthorable } from "./workspacePath";
|
import { isAuthorable } from "./workspacePath";
|
||||||
import type { EditTurnResult, RunEditTurnOptions } from "./liveTurn";
|
import type { EditTurnResult, RunEditTurnOptions } from "./liveTurn";
|
||||||
@@ -382,9 +382,13 @@ export class TrackChangesPreviewController implements vscode.Disposable {
|
|||||||
}
|
}
|
||||||
const spans = this.attribution.spansFor(document);
|
const spans = this.attribution.spansFor(document);
|
||||||
const proposals = this.proposals.listProposals(document);
|
const proposals = this.proposals.listProposals(document);
|
||||||
|
// F12/#64 (INV-50): count added/removed against the LANDED text (current minus
|
||||||
|
// pending proposals), matching the body — a pending change shows once, as a
|
||||||
|
// proposal, and is not also tallied as a landed add/remove.
|
||||||
|
const landedOps = diffBlocks(baselineText, landedTextOf(current, proposals));
|
||||||
const summary = {
|
const summary = {
|
||||||
added: ops.filter((o) => o.kind === "added").length,
|
added: landedOps.filter((o) => o.kind === "added").length,
|
||||||
removed: ops.filter((o) => o.kind === "removed").length,
|
removed: landedOps.filter((o) => o.kind === "removed").length,
|
||||||
proposals: proposals.length,
|
proposals: proposals.length,
|
||||||
};
|
};
|
||||||
void panel.webview.postMessage({
|
void panel.webview.postMessage({
|
||||||
|
|||||||
@@ -3,6 +3,8 @@ import * as fs from "fs";
|
|||||||
import * as path from "path";
|
import * as path from "path";
|
||||||
import * as vscode from "vscode";
|
import * as vscode from "vscode";
|
||||||
import type { CowritingApi } from "../../../src/extension";
|
import type { CowritingApi } from "../../../src/extension";
|
||||||
|
import { addProposal, setProposalApplied } from "../../../src/proposalModel";
|
||||||
|
import { buildFingerprint } from "../../../src/anchorer";
|
||||||
|
|
||||||
const WS = process.env.E2E_WORKSPACE!;
|
const WS = process.env.E2E_WORKSPACE!;
|
||||||
const settle = () => new Promise((r) => setTimeout(r, 400));
|
const settle = () => new Promise((r) => setTimeout(r, 400));
|
||||||
@@ -197,3 +199,53 @@ suite("F12 inline diff — control parity (#64, INV-53)", () => {
|
|||||||
assert.ok(entry && /editorLangId == markdown/.test(entry.when ?? ""), "palette-guarded on markdown");
|
assert.ok(entry && /editorLangId == markdown/.test(entry.when ?? ""), "palette-guarded on markdown");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Reload-safety: a proposal that was optimistically applied in a PRIOR session
|
||||||
|
// persists with `original` set and its fp re-anchored to the applied text. A fresh
|
||||||
|
// controller (empty in-memory `applied` set) must NOT re-capture `original` from the
|
||||||
|
// already-applied buffer — doing so would clobber the true revert target and break
|
||||||
|
// Reject. (Regression for the final-review CRITICAL; INV-51/54.)
|
||||||
|
suite("F12 inline diff — reload-safety (#64, INV-51/54)", () => {
|
||||||
|
test("optimisticApply does not clobber a previously-persisted original", async () => {
|
||||||
|
const TRUE_ORIGINAL = "The original sentence here.";
|
||||||
|
const APPLIED = "The APPLIED sentence here.";
|
||||||
|
const { doc } = await freshDoc("docs/f12-reload.md", `# R\n\n${TRUE_ORIGINAL}\n`);
|
||||||
|
const api = await getApi();
|
||||||
|
const p = api.proposalController;
|
||||||
|
const key = p.keyFor(doc);
|
||||||
|
|
||||||
|
// 1) The buffer holds the APPLIED text (as the saved-while-pending file would).
|
||||||
|
const at = doc.getText().indexOf(TRUE_ORIGINAL);
|
||||||
|
const we = new vscode.WorkspaceEdit();
|
||||||
|
we.replace(doc.uri, new vscode.Range(doc.positionAt(at), doc.positionAt(at + TRUE_ORIGINAL.length)), APPLIED);
|
||||||
|
assert.ok(await vscode.workspace.applyEdit(we), "apply the prior-session applied text");
|
||||||
|
await settle();
|
||||||
|
|
||||||
|
// 2) Record the proposal directly in the sidecar exactly as a prior session left
|
||||||
|
// it: fp anchored to the APPLIED text + `original` = the TRUE original. We do
|
||||||
|
// NOT call optimisticApply, so this controller's in-memory `applied` stays empty.
|
||||||
|
const appliedAt = doc.getText().indexOf(APPLIED);
|
||||||
|
const appliedFp = buildFingerprint(doc.getText(), { start: appliedAt, end: appliedAt + APPLIED.length });
|
||||||
|
let id = "";
|
||||||
|
api.sidecarRouter.update(key, (a) => {
|
||||||
|
id = addProposal(a, appliedFp, APPLIED, { kind: "agent", id: "claude", agent: { sdk: "x", model: "m", sessionId: "s" } }, { granularity: "block" }).proposalId;
|
||||||
|
setProposalApplied(a, id, appliedFp, TRUE_ORIGINAL);
|
||||||
|
});
|
||||||
|
p.renderAll(doc);
|
||||||
|
await settle();
|
||||||
|
|
||||||
|
// 3) Re-entry as a reload would trigger (EditorProposalController re-applies on
|
||||||
|
// onDidChangeProposals because `applied` is empty). The guard must preserve original.
|
||||||
|
await p.optimisticApply(doc, id);
|
||||||
|
await settle();
|
||||||
|
const view = p.listProposals(doc).find((v) => v.id === id);
|
||||||
|
assert.ok(view, "proposal still present");
|
||||||
|
assert.strictEqual(view!.original, TRUE_ORIGINAL, "true original preserved, NOT clobbered with the applied text");
|
||||||
|
|
||||||
|
// 4) Reject restores the TRUE original (not the applied text).
|
||||||
|
assert.ok(await p.revertInPlace(key, id), "revert");
|
||||||
|
await settle();
|
||||||
|
assert.ok(doc.getText().includes(TRUE_ORIGINAL), "reject restored the true original");
|
||||||
|
assert.ok(!doc.getText().includes(APPLIED), "applied text removed on reject");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user