fix: cancel pending uploads when composer chips are removed

This commit is contained in:
2026-10-05 20:55:17 +09:00
parent 9812e68b4d
commit c847c2ffad
7 changed files with 186 additions and 34 deletions
+1 -1
View File
@@ -59,7 +59,7 @@ Completion requests carry a kind, fresh `request_id`, and source-qualified argum
- TUI may select a path visible to the TUI process, stage an upload, and replace the invocation intent with an `UploadedFile` chip. - TUI may select a path visible to the TUI process, stage an upload, and replace the invocation intent with an `UploadedFile` chip.
- Web opens the existing browser picker; it does not enumerate the browser host filesystem. - Web opens the existing browser picker; it does not enumerate the browser host filesystem.
- The local path is never sent to or resolved by the Worker. - The local path is never sent to or resolved by the Worker.
- Uploading is cancellable draft staging, not invocation execution. Removing an unsent uploaded-file chip cancels or deletes staged state, and a late upload completion must not recreate a removed chip. - Uploading is cancellable draft staging, not invocation execution. Removing an in-progress upload chip by Backspace/Delete, range replacement, or draft abandonment cancels its reservation and adapter upload; the removed upload must no longer block a text-only Submit. Late callbacks and Undo cannot restore that cancelled reservation as sendable input. A completed unsent uploaded-file chip may retain its staged resource while editor Undo can restore it, releasing abandoned resources at the draft boundary; a late completion must never turn a deleted pending reservation into such an Undo-owned resource.
- Authoritative `SubmissionAccepted` (not socket-send success) transfers uploaded references to the durable input lifecycle. Draft removal cannot delete an in-flight reference whose admission may already have succeeded, or an accepted attachment. Rejection restores the exact typed intent for retry; retry retains request/invocation IDs. Upload completion alone is not submission acceptance. - Authoritative `SubmissionAccepted` (not socket-send success) transfers uploaded references to the durable input lifecycle. Draft removal cannot delete an in-flight reference whose admission may already have succeeded, or an accepted attachment. Rejection restores the exact typed intent for retry; retry retains request/invocation IDs. Upload completion alone is not submission acceptance.
- TUI standalone currently has no client-local upload staging transport. It reports the unsupported adapter and preserves the draft; it does not reinterpret a local path as a Worker host path. - TUI standalone currently has no client-local upload staging transport. It reports the unsupported adapter and preserves the draft; it does not reinterpret a local path as a Worker host path.
@@ -0,0 +1,13 @@
# Review verdict validation and single-call policy
During T-696's independent review, `ReviewRequested` captured MR `01a10bc8-59f4-7630-8166-d8cd71943e15`, source `9812e68b4d4021ea778a40d9ead4fe545d09e8bf`, and Ticket revision `00001M4517HE0:5`. The Reviewer reproduced a pending-upload deletion defect and attempted a structured `request_changes` verdict. The request used finding severity `medium`; the Backend returned HTTP 422, listing the accepted values as `blocker`, `major`, `minor`, and `note`. No review ID or verdict event was returned.
The Reviewer obeyed the role prompt's instruction to call `ReviewMergeRequest` exactly once and did not retry. Its prose recommendation and artifacts are evidence of investigation, not recorded review authority. The parent preserves the finding, fixes the defect, and requests a new independent review bound to the updated source; it does not invent a verdict or bypass the rejected operation.
Potential improvements:
- Publish the finding severity vocabulary as a closed tool-schema enum, matching Backend validation, rather than relying on a free-form string.
- Clarify the single-call policy for an unambiguously rejected validation request that committed no event. Keep transport ambiguity, consumed capabilities, and source movement distinct from input validation; any retry permission must come from the authority contract, not the caller's inference.
- Return explicit committed-event/capability-consumption evidence on rejection, so a caller can distinguish an absent verdict from an ambiguous write without inspecting storage.
These are observed workflow barriers and proposals, not changes to the review authority contract or implementation requirements of T-696. The original rejected request and reproduction are preserved in the parent-observed Reviewer session and `target/review/T-696/review-submission-blocker.md`.
@@ -306,7 +306,7 @@ Deno.test("production attachment upload failure and in-flight cancellation never
); );
}, true)); }, true));
Deno.test("production upload retry retains upload ID and late completion cannot resurrect deleted reservation", () => Deno.test("production upload retry retains upload ID and deleted pending upload cancels without blocking Submit", () =>
fixture(async (page, base, errors, caseOutput) => { fixture(async (page, base, errors, caseOutput) => {
await control(base, { upload: "fail" }); await control(base, { upload: "fail" });
await attach(page, "retry.md"); await attach(page, "retry.md");
@@ -335,41 +335,35 @@ Deno.test("production upload retry retains upload ID and late completion cannot
const held = await eventually(base, (value) => value.held_uploads.length === 1); const held = await eventually(base, (value) => value.held_uploads.length === 1);
const lateId = held.held_uploads[0]; const lateId = held.held_uploads[0];
await page.locator(".composer-typed-chip").filter({ hasText: "Uploading late.md" }).waitFor(); await page.locator(".composer-typed-chip").filter({ hasText: "Uploading late.md" }).waitFor();
// Delete only the reservation, without aborting its real pending upload. // Keyboard deletion cancels staging; a separate Remove action is not needed.
await page.locator(".cm-content").focus(); await page.locator(".cm-content").focus();
await page.keyboard.press("Control+End"); await page.keyboard.press("Control+End");
await page.keyboard.press("Backspace"); await page.keyboard.press("Backspace");
assertEquals(await page.locator(".composer-typed-chip").count(), 0); assertEquals(await page.locator(".composer-typed-chip").count(), 0);
await page.screenshot({ path: join(caseOutput, "late-upload-deleted.png") }); await page.screenshot({ path: join(caseOutput, "late-upload-deleted.png") });
const response = page.waitForResponse((response) => // Submit immediately, without waiting for upload completion or cancellation HTTP.
response.request().method() === "PUT" && response.url().endsWith(lateId) await page.keyboard.type("finish late deletion");
);
await control(base, { release_uploads: true });
assertEquals((await response).status(), 200);
const late = await eventually(
base,
(value) => value.uploads.includes(lateId) && value.held_uploads.length === 0,
);
assertEquals(
await page.locator(".composer-typed-chip").count(),
0,
"late success cannot resurrect deleted atom",
);
assertEquals(
late.methods.filter((method) => method.method === "submit").length,
1,
"upload/retry/late completion never submits",
);
// Deleted resources remain undo-owned until the next accepted draft boundary.
await page.screenshot({ path: join(caseOutput, "late-upload-completed.png") });
await draft(page, "finish late deletion");
await page.keyboard.press("Control+Enter"); await page.keyboard.press("Control+Enter");
const completed = await eventually(base, (value) => value.deletes.includes(lateId)); const completed = await eventually(
assertEquals(completed.methods.filter((method) => method.method === "submit").length, 2); base,
(value) =>
value.deletes.includes(lateId) && value.held_uploads.length === 0 &&
value.methods.filter((method) => method.method === "submit").length === 2,
);
assertEquals(await page.locator(".composer-attachments").count(), 0);
assertEquals( assertEquals(
completed.methods.filter((method) => method.method === "submit").at(-1)?.params?.input, completed.methods.filter((method) => method.method === "submit").at(-1)?.params?.input,
[{ kind: "text", content: "finish late deletion" }], [{ kind: "text", content: "finish late deletion" }],
); );
// A delayed fixture release after cancellation cannot revive a resource or intent.
// Component tests separately inject a late completion callback against the tombstone.
await page.waitForFunction(() => document.querySelector(".cm-content")?.textContent === "");
await control(base, { release_uploads: true });
const late = await state(base);
assert(!late.uploads.includes(lateId));
assertEquals(late.methods.filter((method) => method.method === "submit").length, 2);
assertEquals(await page.locator(".composer-typed-chip").count(), 0);
await page.screenshot({ path: join(caseOutput, "late-upload-completed.png") });
assert(errors.every((error) => /503/.test(error)), JSON.stringify(errors)); assert(errors.every((error) => /503/.test(error)), JSON.stringify(errors));
await Deno.writeTextFile( await Deno.writeTextFile(
join(caseOutput, "retry-late-result.json"), join(caseOutput, "retry-late-result.json"),
@@ -381,6 +375,57 @@ Deno.test("production upload retry retains upload ID and late completion cannot
); );
}, true)); }, true));
Deno.test("production pending-upload Delete, range, cut and replacement cancel staging and allow immediate text Submit", () =>
fixture(async (page, base, errors, caseOutput) => {
await control(base, { upload: "hold" });
const results = [];
for (const [index, operation] of ["Delete", "range", "cut", "replace"].entries()) {
await attach(page, `keyboard-${operation}.md`);
const held = await eventually(base, (value) => value.held_uploads.length === 1);
const uploadId = held.held_uploads[0];
await page.locator(".composer-typed-chip").filter({
hasText: `Uploading keyboard-${operation}.md`,
}).waitFor();
await page.screenshot({ path: join(caseOutput, `keyboard-${operation}-pending.png`) });
await page.locator(".cm-content").focus();
if (operation === "Delete") {
await page.keyboard.press("Control+Home");
await page.keyboard.press("Delete");
} else {
await page.keyboard.press("Control+a");
if (operation === "range") await page.keyboard.press("Backspace");
else if (operation === "cut") await page.keyboard.press("Control+x");
else await page.keyboard.type("replacement ");
}
assertEquals(await page.locator(".composer-typed-chip").count(), 0);
await page.screenshot({ path: join(caseOutput, `keyboard-${operation}-deleted.png`) });
const text = `${operation === "replace" ? "replacement " : ""}text after ${operation}`;
await page.keyboard.type(`text after ${operation}`);
await page.keyboard.press("Control+Enter");
const accepted = await eventually(
base,
(value) =>
value.deletes.includes(uploadId) && value.held_uploads.length === 0 &&
value.methods.filter((method) => method.method === "submit").length === index + 1,
);
assertEquals(
accepted.methods.filter((method) => method.method === "submit").at(-1)?.params?.input,
[{ kind: "text", content: text }],
);
assertEquals(await page.locator(".composer-attachments").count(), 0);
await page.waitForFunction(() => document.querySelector(".cm-content")?.textContent === "");
await control(base, { release_uploads: true });
assert(!(await state(base)).uploads.includes(uploadId));
await page.screenshot({ path: join(caseOutput, `keyboard-${operation}-accepted.png`) });
results.push({ operation, held, accepted });
}
assertEquals(errors, []);
await Deno.writeTextFile(
join(caseOutput, "keyboard-cancellation-result.json"),
JSON.stringify({ status: "pass", results, errors }, null, 2),
);
}, true));
Deno.test("production selected invocation preserves multiple positional typed arguments", () => Deno.test("production selected invocation preserves multiple positional typed arguments", () =>
fixture(async (page, base, errors, caseOutput) => { fixture(async (page, base, errors, caseOutput) => {
await draft(page, "/rev"); await draft(page, "/rev");
@@ -3,7 +3,7 @@ import { cleanup, fireEvent, render, waitFor } from "@testing-library/svelte";
import { afterEach, expect, test, vi } from "vitest"; import { afterEach, expect, test, vi } from "vitest";
import { EditorView } from "@codemirror/view"; import { EditorView } from "@codemirror/view";
import type { FeatureInvocationDescriptor, Segment } from "#lib/generated/protocol.ts"; import type { FeatureInvocationDescriptor, Segment } from "#lib/generated/protocol.ts";
import { undo } from "@codemirror/commands"; import { redo, undo } from "@codemirror/commands";
import ComposerInput from "./ComposerInput.svelte"; import ComposerInput from "./ComposerInput.svelte";
afterEach(cleanup); afterEach(cleanup);
@@ -188,6 +188,52 @@ test("restored chip descriptor edits reject same-draft cursor ABA", async () =>
expect(ui.component.snapshot().segments).toEqual([invocationSegment]); expect(ui.component.snapshot().segments).toEqual([invocationSegment]);
}); });
test.each(["Backspace", "Delete", "range", "cut", "replace"])("pending upload %s cancels its reservation before late completion and Undo", async (operation) => {
const oncancelupload = vi.fn();
const ui = render(ComposerInput, { historyScope: `upload-delete-${operation}`, oncancelupload });
const cm = editor(ui.container);
type(cm, "before after", 7);
const reservation = ui.component.reserveUpload("report.md")!;
const from = 7, to = cm.state.selection.main.head;
if (operation === "Backspace" || operation === "Delete") {
cm.dispatch({ selection: { anchor: operation === "Backspace" ? to : from } });
await fireEvent.keyDown(cm.contentDOM, { key: operation });
} else if (operation === "replace") {
cm.dispatch({ changes: { from, to, insert: "replacement" }, userEvent: "input" });
} else {
// happy-dom emits selectionchange synchronously while CodeMirror updates a
// focused DOM range. Exercise the transaction/cut handler unfocused here;
// the Chromium suite covers focused keyboard range deletion.
cm.contentDOM.blur();
cm.dispatch({ selection: { anchor: from, head: to } });
if (operation === "cut") await fireEvent.cut(cm.contentDOM, { clipboardData: { setData: vi.fn() } });
else await fireEvent.keyDown(cm.contentDOM, { key: "Backspace" });
}
expect(oncancelupload).toHaveBeenCalledExactlyOnceWith(reservation);
expect(ui.component.completeUpload(reservation, uploadedSegment)).toBe(false);
expect(ui.component.snapshot().segments.some((segment) => segment.kind === "unknown" || segment.kind === "uploaded_file")).toBe(false);
undo(cm);
expect(ui.component.completeUpload(reservation, uploadedSegment)).toBe(false);
expect(ui.component.snapshot().segments.some((segment) => segment.kind === "unknown" || segment.kind === "uploaded_file")).toBe(false);
redo(cm);
ui.component.clear();
expect(oncancelupload).toHaveBeenCalledOnce();
});
test.each(["clear", "restore", "unmount"])("pending upload %s boundary cancels once without acquiring late staged resources", (boundary) => {
const oncancelupload = vi.fn();
const onremoveatom = vi.fn();
const ui = render(ComposerInput, { historyScope: `upload-boundary-${boundary}`, oncancelupload, onremoveatom });
const reservation = ui.component.reserveUpload("report.md")!;
const complete = ui.component.completeUpload;
if (boundary === "clear") ui.component.clear();
else if (boundary === "restore") ui.component.restoreSegments([invocationSegment]);
else ui.unmount();
expect(oncancelupload).toHaveBeenCalledExactlyOnceWith(reservation);
expect(complete(reservation, uploadedSegment)).toBe(false);
expect(onremoveatom).not.toHaveBeenCalled();
});
test("cancelled upload cannot become sendable via late completion or Undo", async () => { test("cancelled upload cannot become sendable via late completion or Undo", async () => {
const onremoveatom = vi.fn(); const onremoveatom = vi.fn();
const ui = render(ComposerInput, { historyScope: "upload-cancel-late", onremoveatom }); const ui = render(ComposerInput, { historyScope: "upload-cancel-late", onremoveatom });
@@ -80,6 +80,7 @@
signal: AbortSignal, signal: AbortSignal,
) => Promise<ComposerCompletionEntry[]>; ) => Promise<ComposerCompletionEntry[]>;
onremoveatom?: (segment: Segment) => void; onremoveatom?: (segment: Segment) => void;
oncancelupload?: (reservation: number) => void;
onclientadapter?: (descriptor: FeatureInvocationDescriptor) => void; onclientadapter?: (descriptor: FeatureInvocationDescriptor) => void;
} }
@@ -98,6 +99,7 @@
resolveFeatureCompletions, resolveFeatureCompletions,
resolveFeatureArgumentCompletions, resolveFeatureArgumentCompletions,
onremoveatom, onremoveatom,
oncancelupload,
onclientadapter, onclientadapter,
}: Props = $props(); }: Props = $props();
@@ -105,6 +107,7 @@
let view: EditorView | null = null; let view: EditorView | null = null;
let editorExtensions: Extension[] = []; let editorExtensions: Extension[] = [];
const stagedTypedAtoms = new Map<number, Segment>(); const stagedTypedAtoms = new Map<number, Segment>();
const pendingUploads = new Set<number>();
let composerHistory = new ComposerHistory(); let composerHistory = new ComposerHistory();
let restoringHistory = false; let restoringHistory = false;
let nextPasteId = 1; let nextPasteId = 1;
@@ -411,9 +414,19 @@
create: () => new Map(), create: () => new Map(),
update(registry, transaction) { update(registry, transaction) {
const additions = transaction.effects.filter((effect) => effect.is(registerTypedAtom)); const additions = transaction.effects.filter((effect) => effect.is(registerTypedAtom));
if (!additions.length) return registry; if (!additions.length && !transaction.docChanged) return registry;
const next = new Map(registry); const next = new Map(registry);
for (const addition of additions) next.set(addition.value.key, addition.value.atom); for (const addition of additions) next.set(addition.value.key, addition.value.atom);
if (transaction.docChanged) {
const live = new Set(composerTypedAtoms(transaction.newDoc.toString(), next).map((atom) => atom.key));
for (const atom of composerTypedAtoms(transaction.startState.doc.toString(), registry)) {
if (atom.uploadReservation && !live.has(atom.key)) {
// Cancellation is part of the editor transaction, not a later callback.
// Undo restores text only; it cannot regain this upload reservation.
next.set(atom.key, { segment: { kind: "text", content: "" }, label: "Removed upload" });
}
}
}
return next; return next;
}, },
}); });
@@ -457,7 +470,14 @@
EditorView.decorations.of((currentView) => typedDecorations(currentView.state)), EditorView.decorations.of((currentView) => typedDecorations(currentView.state)),
EditorView.atomicRanges.of((currentView) => typedDecorations(currentView.state)), EditorView.atomicRanges.of((currentView) => typedDecorations(currentView.state)),
]; ];
// Deleted uploads remain staged while Undo can still restore their atoms. function releasePendingUploads(): void {
const cancelled = [...pendingUploads];
pendingUploads.clear();
for (const key of cancelled) oncancelupload?.(key);
}
// Completed uploads remain staged while Undo can still restore their atoms.
// Pending reservations are cancelled immediately, not retained as Undo leases.
// Release abandoned resources at a draft boundary; accepted live atoms belong to the host. // Release abandoned resources at a draft boundary; accepted live atoms belong to the host.
function releaseStagedAtoms(accepted = false): void { function releaseStagedAtoms(accepted = false): void {
const live = accepted && view ? new Set(composerTypedAtoms( const live = accepted && view ? new Set(composerTypedAtoms(
@@ -481,7 +501,7 @@
function insertTypedSegment( function insertTypedSegment(
segment: Segment, segment: Segment,
options: { from?: number; to?: number; cleanup?: boolean; label?: string } = {}, options: { from?: number; to?: number; cleanup?: boolean; label?: string; uploadReservation?: boolean } = {},
): number | null { ): number | null {
if (!view || view.state.readOnly) return null; if (!view || view.state.readOnly) return null;
const selection = view.state.selection.main; const selection = view.state.selection.main;
@@ -489,13 +509,14 @@
const to = options.to ?? selection.to; const to = options.to ?? selection.to;
const key = nextTypedKey++; const key = nextTypedKey++;
if (options.cleanup) stagedTypedAtoms.set(key, segment); if (options.cleanup) stagedTypedAtoms.set(key, segment);
if (options.uploadReservation) pendingUploads.add(key);
const token = composerTypedToken(key); const token = composerTypedToken(key);
view.dispatch({ view.dispatch({
changes: { from, to, insert: token }, changes: { from, to, insert: token },
selection: EditorSelection.cursor(from + token.length), selection: EditorSelection.cursor(from + token.length),
effects: registerTypedAtom.of({ effects: registerTypedAtom.of({
key, key,
atom: { segment, label: options.label ?? typedAtomLabel(segment), cleanup: options.cleanup }, atom: { segment, label: options.label ?? typedAtomLabel(segment), cleanup: options.cleanup, uploadReservation: options.uploadReservation },
}), }),
annotations: isolateHistory.of("full"), annotations: isolateHistory.of("full"),
userEvent: "input.complete", userEvent: "input.complete",
@@ -784,6 +805,14 @@
spellcheck: "true", spellcheck: "true",
}), }),
EditorView.updateListener.of((update) => { EditorView.updateListener.of((update) => {
// StateField tombstones deleted reservations before notifying the
// adapter. The callback must not dispatch a nested editor update.
for (const key of pendingUploads) {
if (!update.state.field(typedRegistry).get(key)?.uploadReservation) {
pendingUploads.delete(key);
oncancelupload?.(key);
}
}
if (update.docChanged || update.selectionSet) { if (update.docChanged || update.selectionSet) {
editorGeneration++; editorGeneration++;
// Invalidate synchronously: an A→B→A edit/cursor cycle in one // Invalidate synchronously: an A→B→A edit/cursor cycle in one
@@ -881,6 +910,7 @@
return () => { return () => {
closeCompletion(); closeCompletion();
releasePendingUploads();
releaseStagedAtoms(); releaseStagedAtoms();
editVersion++; editVersion++;
view?.destroy(); view?.destroy();
@@ -940,11 +970,14 @@
} }
export function reserveUpload(fileName: string): number | null { export function reserveUpload(fileName: string): number | null {
return insertTypedSegment({ kind: "unknown" }, { label: `Uploading ${fileName}` }); return insertTypedSegment({ kind: "unknown" }, { label: `Uploading ${fileName}`, uploadReservation: true });
} }
export function completeUpload(key: number, segment: Segment): boolean { export function completeUpload(key: number, segment: Segment): boolean {
if (!view || view.state.field(typedRegistry).get(key)?.segment.kind !== "unknown") return false; if (!view || !pendingUploads.has(key) ||
!view.state.field(typedRegistry).get(key)?.uploadReservation ||
!composerTypedAtoms(view.state.doc.toString(), view.state.field(typedRegistry)).some((atom) => atom.key === key)) return false;
pendingUploads.delete(key);
stagedTypedAtoms.set(key, segment); stagedTypedAtoms.set(key, segment);
view.dispatch({ effects: registerTypedAtom.of({ view.dispatch({ effects: registerTypedAtom.of({
key, atom: { segment, label: typedAtomLabel(segment), cleanup: true }, key, atom: { segment, label: typedAtomLabel(segment), cleanup: true },
@@ -968,6 +1001,7 @@
export function clear(acceptedTypedResources = false, retainStagedResources = false): void { export function clear(acceptedTypedResources = false, retainStagedResources = false): void {
if (!view) return; if (!view) return;
releasePendingUploads();
if (!retainStagedResources) releaseStagedAtoms(acceptedTypedResources); if (!retainStagedResources) releaseStagedAtoms(acceptedTypedResources);
editVersion++; editVersion++;
closeCompletion(); closeCompletion();
@@ -32,6 +32,8 @@ export interface ComposerTypedSegment {
label: string; label: string;
/** Whether deleting this unsent atom must release client-staged state. */ /** Whether deleting this unsent atom must release client-staged state. */
cleanup?: boolean; cleanup?: boolean;
/** Client-only reservation whose removal cancels an in-progress upload. */
uploadReservation?: boolean;
} }
export interface ComposerTypedAtom extends ComposerTypedSegment { export interface ComposerTypedAtom extends ComposerTypedSegment {
@@ -1091,6 +1091,17 @@
); );
} }
function cancelComposerUpload(reservation: number): void {
const attachment = attachments.find((candidate) =>
uploadReservations.get(candidate.id) === reservation);
if (!attachment) return;
// The editor already cancelled/tombstoned its reservation. Do not call
// cancelUpload here: this may run inside a CodeMirror update listener.
uploadReservations.delete(attachment.id);
attachments = attachments.filter((candidate) => candidate.id !== attachment.id);
attachment.request?.abort();
}
function completeAttachmentUpload( function completeAttachmentUpload(
attachment: ComposerAttachment, attachment: ComposerAttachment,
reference: NonNullable<ComposerAttachment["reference"]>, reference: NonNullable<ComposerAttachment["reference"]>,
@@ -2177,6 +2188,7 @@
resolveFeatureArgumentCompletions={(context, prefix, signal) => resolveFeatureArgumentCompletions={(context, prefix, signal) =>
featureArgumentCompletionLane(context).request(prefix, signal)} featureArgumentCompletionLane(context).request(prefix, signal)}
onremoveatom={(segment) => void releaseComposerAtom(segment)} onremoveatom={(segment) => void releaseComposerAtom(segment)}
oncancelupload={cancelComposerUpload}
onclientadapter={(descriptor) => { onclientadapter={(descriptor) => {
if (descriptor.client_adapter === "attachment") fileInput?.click(); if (descriptor.client_adapter === "attachment") fileInput?.click();
}} }}