From 8062b1c2ccb34cc6d321c0562ec1b0ff63463fc4 Mon Sep 17 00:00:00 2001 From: antianqi <75944423+antianqi@users.noreply.github.com> Date: Mon, 5 Oct 2026 15:08:57 +0800 Subject: [PATCH 1/5] fix(webui): report a failed revert/reapply instead of relabelling it a missing capability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A revert or reapply that did not apply reduced to the same state as a runtime that never offered the capability, so the card told the user "当前运行时未提供 session diff 能力" and then went dead for good. Three defects, all reachable from one click on 撤销 / 重新应用: 1. `mutation-failed` shared a reducer branch with `unsupported` (contracts.ts). The runtime had answered — it had declined — but the UI reported the capability as absent. 2. Because `buildWebuiDiffMutationRequest` refuses every request once `unsupported` is set, and nothing ever reset it, the first failure left the card permanently unable to revert, reapply, or retry. There is no way back: the reload effect depends on `[getTurnDiff, request]`, so it never refires. 3. `WebuiRevertTurnDiffResult.error` was never read. The client referenced the result type in exactly two places and both only looked at the view, so the runtime's own explanation was dropped on the floor. A fourth surfaced while fixing the third. `reapplyTurnDiff` answers with the view itself, so a reapply that applied nothing arrived as a view with no file list — and the card renders `null` for an empty file list, so that path deleted the card with no message at all. A different silent failure from the same button. What a transport result means now lives in one pure function, `resolveWebuiDiffMutation`, checked against two independent things: whether the runtime said it worked, and whether it handed back an applied file list. A failure carries the runtime's reason into a banner on the live card. The card and its controls stay usable, and the reason clears on the next attempt, on success, or on dismiss. `unsupported` keeps its original meaning — a runtime that genuinely never offers the capability — because "never say unsupported again" would be a different bug. Scope: the Revert / Reapply row of roadmap E 区. The other three E-area rows are untouched. Review 审查模式 and 工作树隔离 are disabled settings tabs, and unlocking them lands on the same settings page as izzy's K-area claim; see the PR body for the boundary. Validation - pnpm typecheck:webui exit 0 - pnpm typecheck:webui-full exit 0 (covers the new .tsx test) - pnpm check:source exit 0 (inventory +1 line) - pnpm build:webui exit 0 - node scripts/run-vitest-suite.mjs webui 9 failed | 1577 passed | 4 skipped - npx playwright test test/webui-browser/ 71 passed The 9 failures are the pre-existing Windows baseline, not this change: 7 webui-boundary-check path assertions, 1 webui-design-tokens path assertion, 1 webui-service shutdown timeout. Verified by reverting all five files of this change and re-running the same three test files on a clean 2f064db tree: the same 9 tests fail there, with identical names. Test evidence New: packages/webui/test/unit/diff-mutation-error.test.tsx, 17 tests. Red before the fix: 12 of 13 failed, and the failures named the defects — `expected true to be false` on `unsupported`, `expected undefined to deeply equal { id: 's', changeSetId: 'changes-1' }` on the dead retry path, and a missing `turn-diff-mutation-error` marker on the render. Negative injection: 14 mutations, 0 survivors, implementation restored byte-for-byte after every run. Reaching that number took two corrections to the tests and the harness, both recorded here because both would otherwise have produced a false green: - One assertion was vacuous. "a genuinely unavailable capability still reduces to unsupported" started from a freshly loaded card whose `mutationError` was already `undefined`, so it passed whether or not the transition cleared anything. It now seeds a failure first. That mutation survived the first run. - The first harness matched `/Tests\s+13 passed/`. When the suite grew to 15, that regex stopped matching, the pass flag went permanently false, and all 12 mutations were reported "killed" no matter what they did — a false kill covering the whole run. The harness now counts the failures reported in the summary line and classifies a crashed run separately from a red one. - With a working harness, two real coverage gaps appeared: every reason-carrying case used `success: false`, which returns early, so the fallback that reads `result.error` was never exercised. `success` is optional on `WebuiRevertTurnDiffResult`, so a payload carrying only `error` is contract-legal. Covered now. The one mutation that needed a contract-illegal payload to die is noted in the test: a reapply that claims `success: true` while also naming a reason. `success` is required on `WebuiReapplyTurnDiffResult`, so the correct and mutated code agree on every legal input. It is pinned anyway because this is a socket boundary and a runtime that says both is better answered with the reason than with silence. --- .../webui/src/client/components/DiffCard.tsx | 88 +++++- packages/webui/src/client/contracts.ts | 13 +- packages/webui/src/client/styles/shell.css | 46 +++ .../test/unit/diff-mutation-error.test.tsx | 293 ++++++++++++++++++ release/public-source.json | 1 + test/vitest-suites.json | 1 + 6 files changed, 429 insertions(+), 13 deletions(-) create mode 100644 packages/webui/test/unit/diff-mutation-error.test.tsx diff --git a/packages/webui/src/client/components/DiffCard.tsx b/packages/webui/src/client/components/DiffCard.tsx index 9ecb45ed0..c10ad4671 100644 --- a/packages/webui/src/client/components/DiffCard.tsx +++ b/packages/webui/src/client/components/DiffCard.tsx @@ -43,14 +43,23 @@ export function reduceWebuiDiffState( ): WebuiDiffState { switch (action.type) { case "loaded": - return { ...state, view: action.view, unsupported: false, busy: false }; + return { ...state, view: action.view, unsupported: false, busy: false, mutationError: undefined }; case "unsupported": + return { ...state, unsupported: true, busy: false, mutationError: undefined }; + /* A failed mutation is not an absent capability: the runtime answered, it + * just declined this operation. Keeping `unsupported` false is what leaves + * the card interactive, and `buildWebuiDiffMutationRequest` gating on + * `unsupported` is what otherwise makes the first failure permanent. */ case "mutation-failed": - return { ...state, unsupported: true, busy: false }; + return { ...state, unsupported: false, busy: false, mutationError: trimmedOrUndefined(action.error) }; case "begin-mutation": - return state.busy ? state : { ...state, busy: true }; + // A retry supersedes the previous reason; leaving it up would report a + // stale failure next to an attempt that is still running. + return state.busy ? state : { ...state, busy: true, mutationError: undefined }; case "mutation-succeeded": - return { ...state, view: action.view, unsupported: false, busy: false }; + return { ...state, view: action.view, unsupported: false, busy: false, mutationError: undefined }; + case "dismiss-mutation-error": + return { ...state, mutationError: undefined }; case "toggle-expanded": return { ...state, expanded: !state.expanded }; case "toggle-review": @@ -58,6 +67,51 @@ export function reduceWebuiDiffState( } } +/** A blank or whitespace-only reason explains nothing, so it is dropped + * rather than rendered as an empty banner. */ +function trimmedOrUndefined(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + return trimmed ? trimmed : undefined; +} + +/** Decides what a revert/reapply round trip meant. + * + * The two operations nest the resulting view differently, and reading either + * shape as success is a silent failure: + * + * * `revertTurnDiff` answers `{ success?, error?, turnDiff? }` — the view + * lives under `turnDiff`, and a refusal carries only `error`. + * * `reapplyTurnDiff` answers with the view itself (`success` required, + * `error` optional), so for reapply the result *is* the next view. + * + * Two independent things are therefore checked: whether the runtime said it + * worked, and whether it handed back an applied file list. A view with no + * files applied nothing, and the card renders `null` for an empty file list, + * so accepting one would delete the card with no message at all. */ +export function resolveWebuiDiffMutation( + action: "revert" | "reapply", + result: WebuiRevertTurnDiffResult | WebuiReapplyTurnDiffResult | undefined, + thrown?: unknown, +): WebuiDiffStateAction { + if (thrown !== undefined) { + const message = thrown instanceof Error ? thrown.message : undefined; + return { type: "mutation-failed", error: trimmedOrUndefined(message) }; + } + // `success` is required on reapply and optional on revert; an explicit + // `false` is a refusal even if a view rode along beside it. + if (result?.success === false) { + return { type: "mutation-failed", error: trimmedOrUndefined(result.error) }; + } + const nextView = + action === "revert" + ? (result as WebuiRevertTurnDiffResult | undefined)?.turnDiff + : (result as WebuiReapplyTurnDiffResult | undefined); + if (nextView && (nextView.fileChanges ?? []).length > 0) { + return { type: "mutation-succeeded", view: nextView }; + } + return { type: "mutation-failed", error: trimmedOrUndefined(result?.error) }; +} + export function buildWebuiDiffMutationRequest( state: WebuiDiffState, request: WebuiGetTurnDiffRequest, @@ -103,7 +157,7 @@ export function WebuiDiffCard({ ...initialState, ...(initialView ? { view: initialView } : {}), })); - const { view, unsupported, busy, expanded, reviewing } = diffState; + const { view, unsupported, busy, expanded, reviewing, mutationError } = diffState; const request = useMemo(() => { if (!sessionId || !getTurnDiff) return undefined; // The runtime keys a turn diff by the turn: an `assistantMessageId` is @@ -150,13 +204,9 @@ export function WebuiDiffCard({ setDiffState((current) => confirmWebuiDiffMutation(current, confirmed)); try { const result = await handler(mutationRequest); - const nextView = action === "revert" - ? (result as WebuiRevertTurnDiffResult).turnDiff - : (result as WebuiReapplyTurnDiffResult); - if (nextView) setDiffState((current) => reduceWebuiDiffState(current, { type: "mutation-succeeded", view: nextView })); - else setDiffState((current) => reduceWebuiDiffState(current, { type: "mutation-failed" })); - } catch { - setDiffState((current) => reduceWebuiDiffState(current, { type: "mutation-failed" })); + setDiffState((current) => reduceWebuiDiffState(current, resolveWebuiDiffMutation(action, result))); + } catch (error) { + setDiffState((current) => reduceWebuiDiffState(current, resolveWebuiDiffMutation(action, undefined, error))); } finally { setDiffState((current) => ({ ...current, busy: false })); } @@ -248,6 +298,20 @@ export function WebuiDiffCard({ ))} ) : null} + {mutationError ? ( + /* `role="alert"` because this replaces what used to be no feedback at + * all: the operation did nothing, and the file list above is now known + * to be out of date. The controls stay live so the user can retry. */ +
+ + 这轮文件改动没有生效 + {mutationError} + + +
+ ) : null} ); } diff --git a/packages/webui/src/client/contracts.ts b/packages/webui/src/client/contracts.ts index 6cefd9ee3..119ae581b 100644 --- a/packages/webui/src/client/contracts.ts +++ b/packages/webui/src/client/contracts.ts @@ -412,6 +412,16 @@ export interface WebuiDiffState { readonly busy: boolean; readonly expanded: boolean; readonly reviewing: boolean; + /** Why the last revert/reapply did not apply, when it did not apply. + * + * Deliberately NOT the same thing as `unsupported`: that flag means the + * runtime never offered the capability, this one means the runtime answered + * and the operation did not take effect. Collapsing the two made every + * failure read as "当前运行时未提供 session diff 能力", which is both + * untrue and, because `buildWebuiDiffMutationRequest` refuses every request + * once `unsupported` is set and nothing ever resets it, permanently + * unrecoverable. */ + readonly mutationError?: string; } export type WebuiDiffStateAction = @@ -419,7 +429,8 @@ export type WebuiDiffStateAction = | { readonly type: "unsupported" } | { readonly type: "begin-mutation" } | { readonly type: "mutation-succeeded"; readonly view: WebuiTurnDiffView } - | { readonly type: "mutation-failed" } + | { readonly type: "mutation-failed"; readonly error?: string } + | { readonly type: "dismiss-mutation-error" } | { readonly type: "toggle-expanded" } | { readonly type: "toggle-review" }; diff --git a/packages/webui/src/client/styles/shell.css b/packages/webui/src/client/styles/shell.css index d264c5b48..44533d1d6 100644 --- a/packages/webui/src/client/styles/shell.css +++ b/packages/webui/src/client/styles/shell.css @@ -4255,6 +4255,52 @@ opacity: 0.5; } + /* A revert/reapply that did not apply. Deliberately a footnote on the live + card rather than a replacement for it: the file list above is stale, but + hiding the card would also hide the control that retries. */ + .webui-diff-error { + display: flex; + align-items: flex-start; + gap: 12px; + padding: 8px 12px; + border-top: 0.5px solid var(--border_default); + background: var(--bg_interaction_secondary_default); + font-size: 12px; + } + + .webui-diff-error-copy { + display: flex; + flex: 1; + min-width: 0; + flex-direction: column; + gap: 2px; + } + + .webui-diff-error-title { + color: var(--text_default_primary); + } + + /* The reason is whatever the runtime reported, so it can be long and will + often be a path; wrapping it keeps the dismiss control on the card. */ + .webui-diff-error-reason { + color: var(--text_label_danger_secondary_default); + overflow-wrap: anywhere; + } + + .webui-diff-error-dismiss { + flex: none; + height: 24px; + padding: 0 8px; + border: 0.5px solid var(--border_default); + border-radius: 4px; + background: transparent; + color: var(--text_default_secondary); + } + + .webui-diff-error-dismiss:hover { + background: var(--bg_interaction_tertiary_hover); + } + /* ===== Phase 5: message actions, goal banner, and questionnaire states ===== */ .webui-message-actions { display: flex; diff --git a/packages/webui/test/unit/diff-mutation-error.test.tsx b/packages/webui/test/unit/diff-mutation-error.test.tsx new file mode 100644 index 000000000..44915a096 --- /dev/null +++ b/packages/webui/test/unit/diff-mutation-error.test.tsx @@ -0,0 +1,293 @@ +import { createElement } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import { describe, expect, it } from "vitest"; + +import { + WebuiDiffCard, + buildWebuiDiffMutationRequest, + initialWebuiDiffState, + reduceWebuiDiffState, + resolveWebuiDiffMutation, +} from "../../src/client/components/DiffCard.js"; +import type { WebuiTurnDiffView } from "../../src/server/port.js"; + +/* Why these tests are about the shape of a FAILED mutation, not about markup: + * + * `renderToStaticMarkup` does not run effects and cannot fire a click, so + * every assertion here is deliberately aimed at a pure function or at a + * state object the component accepts through `initialState`. The event path + * itself (`mutate` in DiffCard.tsx) is therefore not covered by this file — + * it is covered indirectly, because the whole decision of what a transport + * result means is delegated to `resolveWebuiDiffMutation` and the rendering + * of a failure is delegated to `reduceWebuiDiffState` + the JSX below. If + * someone re-inlines that decision into the component, these tests keep + * passing while the component rots; the browser suite is what actually + * drives the click. That split is deliberate, and it is why the negative + * injections recorded in the PR body target these two seams. + */ + +const files = [ + { file: "one.ts", additions: 2, deletions: 1 }, + { file: "two.ts", additions: 3, deletions: 0 }, +]; + +const activeView: WebuiTurnDiffView = { + status: "active", + changeSetId: "changes-1", + sourceMessageId: "assistant-1", + canUndo: true, + canReapply: false, + fileChanges: files, +}; + +const revertedView: WebuiTurnDiffView = { + ...activeView, + status: "reverted", + canUndo: false, + canReapply: true, +}; + +const loaded = (view: WebuiTurnDiffView = activeView) => + reduceWebuiDiffState(initialWebuiDiffState, { type: "loaded", view }); + +describe("a failed revert/reapply is reported, not relabelled as a missing capability", () => { + /* The defect this file exists for: `mutation-failed` used to reduce to + * `unsupported: true`, which renders "当前运行时未提供 session diff 能力。" + * The runtime DID answer — it declined the operation. The card then also + * became permanently dead, because `buildWebuiDiffMutationRequest` refuses + * every request once `unsupported` is set and nothing ever resets it. */ + it("does not claim the runtime lacks the capability when the operation merely failed", () => { + const failed = reduceWebuiDiffState(loaded(), { + type: "mutation-failed", + error: "plan-not-safe: 目标路径在工作区之外", + }); + + expect(failed.unsupported).toBe(false); + expect(failed.mutationError).toBe("plan-not-safe: 目标路径在工作区之外"); + }); + + it("keeps the card actionable so the user can retry after a failure", () => { + const failed = reduceWebuiDiffState(loaded(), { type: "mutation-failed" }); + + expect(failed.busy).toBe(false); + expect(buildWebuiDiffMutationRequest(failed, { id: "s" }, "revert")).toEqual({ + id: "s", + changeSetId: "changes-1", + }); + }); + + it("renders the failure on the live card instead of the capability-missing card", () => { + const markup = renderToStaticMarkup( + createElement(WebuiDiffCard, { + initialView: activeView, + initialState: { + ...initialWebuiDiffState, + mutationError: "plan-not-safe: 目标路径在工作区之外", + }, + }), + ); + + expect(markup).toContain('data-testid="turn-diff-mutation-error"'); + expect(markup).toContain("plan-not-safe: 目标路径在工作区之外"); + // The card itself must survive: a failure is not a reason to hide the + // file list or the retry control. + expect(markup).toContain('data-testid="turn-diff-card"'); + expect(markup).toContain('data-testid="turn-diff-undo"'); + expect(markup).toContain("one.ts"); + // And it must not claim the capability is missing. + expect(markup).not.toContain("当前运行时未提供 session diff 能力"); + }); + + it("offers a dismiss control so the reason is not stuck on screen forever", () => { + const markup = renderToStaticMarkup( + createElement(WebuiDiffCard, { + initialView: activeView, + initialState: { ...initialWebuiDiffState, mutationError: "boom" }, + }), + ); + expect(markup).toContain('data-testid="turn-diff-mutation-error-dismiss"'); + + const dismissed = reduceWebuiDiffState( + { ...initialWebuiDiffState, mutationError: "boom" }, + { type: "dismiss-mutation-error" }, + ); + expect(dismissed.mutationError).toBeUndefined(); + expect(dismissed.unsupported).toBe(false); + }); + + it("clears a stale reason when a new attempt starts or succeeds", () => { + const failed = { ...initialWebuiDiffState, view: activeView, mutationError: "old failure" }; + + expect( + reduceWebuiDiffState(failed, { type: "begin-mutation" }).mutationError, + ).toBeUndefined(); + expect( + reduceWebuiDiffState(failed, { type: "mutation-succeeded", view: activeView }) + .mutationError, + ).toBeUndefined(); + expect( + reduceWebuiDiffState(failed, { type: "loaded", view: activeView }).mutationError, + ).toBeUndefined(); + }); + + /* A genuinely unavailable capability keeps its old meaning. Without this, + * the fix would be "never say unsupported again", which is a different bug. + * + * It has to start from a state that actually carries a failure reason: the + * first draft of this assertion reduced `unsupported` from a freshly loaded + * card, whose `mutationError` was already `undefined`, so it passed whether + * or not the transition cleared anything. */ + it("still reports a genuinely unavailable capability as unsupported, and drops any stale failure", () => { + const failed = reduceWebuiDiffState(loaded(), { type: "mutation-failed", error: "boom" }); + expect(failed.mutationError).toBe("boom"); + + const unsupported = reduceWebuiDiffState(failed, { type: "unsupported" }); + expect(unsupported.unsupported).toBe(true); + expect(unsupported.mutationError).toBeUndefined(); + expect(buildWebuiDiffMutationRequest(unsupported, { id: "s" }, "revert")).toBeUndefined(); + }); +}); + +describe("resolveWebuiDiffMutation reads the transport result the way the wire means it", () => { + it("treats a revert that returns a populated view as success", () => { + expect( + resolveWebuiDiffMutation("revert", { success: true, turnDiff: revertedView }), + ).toEqual({ type: "mutation-succeeded", view: revertedView }); + }); + + /* `revertTurnDiff` reports refusal as `{ success: false, error }` and no + * `turnDiff` at all. The `error` string is the only explanation the user + * can get, so it has to survive into the rendered state. */ + it("surfaces the error string the server sent when a revert is refused", () => { + expect( + resolveWebuiDiffMutation("revert", { + success: false, + error: "plan-not-safe: 目标路径在工作区之外", + }), + ).toEqual({ + type: "mutation-failed", + error: "plan-not-safe: 目标路径在工作区之外", + }); + }); + + /* `reapplyTurnDiff` answers with the view itself, so its `error` is read + * from the top level. Reading it only off the revert branch dropped the + * explanation for every failed reapply — the one case where a user who just + * clicked "重新应用" most needs to be told why nothing happened. */ + it("surfaces the error string a refused reapply sent", () => { + expect( + resolveWebuiDiffMutation("reapply", { + success: false, + error: "change set 已不在当前工作区", + ...revertedView, + }), + ).toEqual({ + type: "mutation-failed", + error: "change set 已不在当前工作区", + }); + }); + + /* An explicit `success: false` is a refusal even if a view rode along. */ + it("does not let a view override an explicit refusal", () => { + expect( + resolveWebuiDiffMutation("revert", { success: false, error: "nope", turnDiff: revertedView }), + ).toEqual({ type: "mutation-failed", error: "nope" }); + }); + + /* `success` is OPTIONAL on `WebuiRevertTurnDiffResult`, so a runtime that + * reports only `error` and omits `success` is a contract-legal payload — + * and it takes a different path than the explicit-`false` refusal above, + * which returns early. Without this case the whole reason-carrying fallback + * is untested: the negative-injection run showed two mutations of that + * fallback surviving while every other seam died. */ + it("surfaces a reason from a revert that omits success entirely", () => { + expect( + resolveWebuiDiffMutation("revert", { + error: "plan-not-safe: 目标路径在工作区之外", + }), + ).toEqual({ + type: "mutation-failed", + error: "plan-not-safe: 目标路径在工作区之外", + }); + }); + + /* The card renders `null` when `fileChanges` is empty, so a view that + * applied nothing must be read as a failure — otherwise the card silently + * disappears with no message at all. */ + it("does not read an empty view as a success", () => { + expect(resolveWebuiDiffMutation("reapply", { success: true })).toEqual({ + type: "mutation-failed", + error: undefined, + }); + expect(resolveWebuiDiffMutation("reapply", { success: true, fileChanges: [] })).toEqual({ + type: "mutation-failed", + error: undefined, + }); + expect(resolveWebuiDiffMutation("revert", { success: true, turnDiff: { changeSetId: "c" } })).toEqual({ + type: "mutation-failed", + error: undefined, + }); + }); + + it("still accepts a reapply that really did return files", () => { + const applied = { success: true, ...activeView }; + expect(resolveWebuiDiffMutation("reapply", applied)).toEqual({ + type: "mutation-succeeded", + view: applied, + }); + }); + + it("falls back to a reason that does not blame the capability", () => { + // Whitespace-only is not a reason; the state must carry no string at all + // so the banner can still render its static title without a blank line. + expect(resolveWebuiDiffMutation("revert", { success: false, error: " " })).toEqual({ + type: "mutation-failed", + error: undefined, + }); + expect(resolveWebuiDiffMutation("revert", { success: false })).toEqual({ + type: "mutation-failed", + error: undefined, + }); + }); + + /* `reapplyTurnDiff` requires `success`, so reaching the reason-carrying + * fallback with a non-empty `error` means the payload contradicts itself: + * it claims to have worked and names a reason it did not. For any + * contract-legal input this case is unreachable, which is exactly why it + * needs pinning — without it the resolver can silently start preferring the + * `success` flag and drop the reason, and no legal payload would catch it. + * This is a socket boundary; a runtime that says both is better answered + * with the reason than with silence. */ + it("prefers a stated reason over a success flag when a reapply payload contradicts itself", () => { + expect( + resolveWebuiDiffMutation("reapply", { + success: true, + error: "change set 已不在当前工作区", + ...activeView, + fileChanges: [], + }), + ).toEqual({ type: "mutation-failed", error: "change set 已不在当前工作区" }); + }); + + it("carries a thrown transport error into the failure", () => { + expect( + resolveWebuiDiffMutation("revert", undefined, new Error("WebUI request timed out after 30000ms (revertTurnDiff)")), + ).toEqual({ + type: "mutation-failed", + error: "WebUI request timed out after 30000ms (revertTurnDiff)", + }); + }); + + /* A non-Error throw is not a crash, it is a failure with no explanation. + * It must still be reported as a failure rather than crashing the card. */ + it("treats a non-Error throw as a failure without a reason", () => { + expect(resolveWebuiDiffMutation("reapply", undefined, "socket closed")).toEqual({ + type: "mutation-failed", + error: undefined, + }); + expect(resolveWebuiDiffMutation("reapply", undefined, new Error(" "))).toEqual({ + type: "mutation-failed", + error: undefined, + }); + }); +}); diff --git a/release/public-source.json b/release/public-source.json index 11eeed792..808a61a34 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3671,6 +3671,7 @@ "packages/webui/test/unit/context-breakdown.test.ts", "packages/webui/test/unit/context-usage-popover.test.ts", "packages/webui/test/unit/conversation-usage-banner.test.tsx", + "packages/webui/test/unit/diff-mutation-error.test.tsx", "packages/webui/test/unit/fork-session-operation.test.ts", "packages/webui/test/unit/markdown-autolink-double-anchor.test.tsx", "packages/webui/test/unit/markdown-codeblock-c1-highlight.test.tsx", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 57fe0e30d..4280767f0 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -258,6 +258,7 @@ "packages/webui/test/unit/token-plan-model.test.ts", "packages/webui/test/unit/thinking-control.test.tsx", "packages/webui/test/unit/webui-transcript-widgets-integration.test.ts", + "packages/webui/test/unit/diff-mutation-error.test.tsx", "packages/webui/test/unit/webui-round3-acceptance.test.tsx", "packages/webui/test/unit/webui-w0-store-sequence.test.ts", "packages/webui/test/unit/webui-w0-projections.test.ts", From 06f582225171d1a150423e11757a047837bd4f6a Mon Sep 17 00:00:00 2001 From: antianqi <75944423+antianqi@users.noreply.github.com> Date: Mon, 5 Oct 2026 16:18:51 +0800 Subject: [PATCH 2/5] =?UTF-8?q?feat(webui):=20build=20the=20code-review=20?= =?UTF-8?q?and=20worktree=20panels=20behind=20E=20=E5=8C=BA's=20two=20dark?= =?UTF-8?q?=20tabs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Roadmap E 区 ships five rows. PR #20 took the Revert / Reapply row. This takes the two that the roadmap records as ❌ with the reason "设置 tab 禁用". That reason is only half of it. Both tabs were disabled because there was nothing behind them, not because a gate refused them: `SettingsModal`'s render chain is a series of `active === …` ternaries with branches for `desktop`, `usage`, `account` and `archived`, and everything else fell through to an `webui-settings-empty-panel` placeholder. Flipping `disabled` to `false` would have produced a clickable label above a blank pane — the same shape of defect the row already had. So both pages are built. 代码审查 (Review 审查模式 / 修复建议+跳转) The capability was already wired end to end and unused from here: the workspace review operations (`getWorkspaceReviewSummary`, `listWorkspaceReviewFileDiffs`, `searchWorkspaceReviewDiffs`) are registered, exposed on the transport and typed — the diff card's Review button already dispatches into the workspace panel. What was missing was a place to read a whole change set. The page lists the change set with per-file stats, loads each file's unified diff on expand in batches of five, and turns a diff line into a real editor jump: `projectWebuiReviewLines` numbers both sides from the hunk header, and a click resolves to the new-side line. A deleted line gets no target, because it is not in the file the editor will open — that is asserted directly rather than guarded, so the contract lives in a test instead of in an untestable line. Also plumbed: `workspaceDir` and `onOpenFileLine` now travel FoundationApp → UserMenu → SettingsModal. The jump reuses the shell's existing `#session=`/`open-file` path, so there is one navigation route rather than two. 工作树 (工作树隔离) Creating an isolated worktree already worked from the rail's context menu (「复制到新工作树」, with `worktreeVisible` / `worktreeUnavailableReason` on the server). This adds the other half of the row: seeing which parallel experiment branches exist and switching into one. A worktree is not a first-class field on a session, so it is inferred — git makes a second checkout with its own absolute path, and `isDefaultWorkspace` is the runtime's own statement of which one is primary. Grouping normalises path separators first, because a fork round-tripping a Windows path would otherwise report one checkout twice. Two tabs were left disabled on purpose: 语音 / 快捷键 / 个性化 / 连接 still have no content behind them, and the `disabled` gate is what keeps them from being clickable labels over an empty pane. Validation - pnpm check:source exit 0 (inventory +6) - pnpm typecheck:webui exit 0 - pnpm typecheck:webui-full exit 0 (covers the new .tsx tests) - pnpm build:webui exit 0 - node scripts/run-vitest-suite.mjs webui 9 failed | 1637 passed | 4 skipped - npx playwright test test/webui-browser/ 79 passed The 9 failures are the pre-existing Windows baseline and are unchanged from the count on the parent commit: 7 webui-boundary-check path assertions, 1 webui-design-tokens path assertion, 1 webui-service shutdown timeout. The same 9 were verified on a clean tree earlier in this branch. Test evidence Four new test files, 56 tests, plus one browser test: - review-state.test.ts (21) — lifecycle, the search filter, and the diff projection including both-side line numbering - review-panel.test.tsx (16) — the panel's render, and the tab-to-page wiring - worktree-state.test.ts (13) — path normalisation and checkout grouping - worktree-panel.test.tsx (6) — the worktree list render Negative injection: 17 mutations across all three new seams, 0 survivors, every implementation file restored byte-for-byte. Two of those mutations were equivalent on the first pass and both were fixed rather than waived: - `webuiReviewLineTarget` re-checked the line kind even though `projectWebuiReviewLines` only ever assigns a `newLine` to additions and context rows. Deleting the guard changed nothing, so nothing could observe it. The guard is gone and the invariant is now asserted directly in the test, which makes a projection that starts numbering deletions fail loudly (4 tests red). - The tab-to-page routing had no coverage at all. `SettingsModal` opens on `desktop` and only moves tab from a click handler, so no `renderToStaticMarkup` call can reach the `coding` or `worktree` branch — replacing `{active === "coding" ? ; interface SettingsModalProps { @@ -81,13 +103,20 @@ interface SettingsModalProps { readonly dataDir?: string; readonly version?: WebuiVersionInfo; readonly sessionId?: string; - /** Capability source for the modal. Typed as the narrow 9-member - * contract so the modal cannot accidentally start reading members it - * does not consume. */ + /** Workspace the code-review page reads its change set from. Every + * workspace review operation is keyed by an absolute directory, so without + * this the page has nothing to ask about. */ + readonly workspaceDir?: string; + /** Opens a file at a line. This is what makes a review line a real jump + * rather than decoration: the editor receives the path and the new-side + * line number the diff projection resolved. */ + readonly onOpenFileLine?: (path: string, line: number) => void; + /** Capability source for the modal. Typed as the narrow contract so the + * modal cannot accidentally start reading members it does not consume. */ readonly transport?: WebuiSettingsModalCapabilities; } -export function SettingsModal({ open, onClose, dataDir, version, sessionId, transport }: SettingsModalProps): ReactElement | null { +export function SettingsModal({ open, onClose, dataDir, version, sessionId, workspaceDir, onOpenFileLine, transport }: SettingsModalProps): ReactElement | null { // The transport is optional. Each capability is optional too, so we bind // only when both are present; otherwise we surface `undefined` and let the // call sites do their existing null checks. @@ -119,6 +148,10 @@ export function SettingsModal({ open, onClose, dataDir, version, sessionId, tran startCodexOAuthLogin: transport?.startCodexOAuthLogin?.bind(transport), cancelCodexOAuthLogin: transport?.cancelCodexOAuthLogin?.bind(transport), refreshModels: transport?.refreshModels?.bind(transport), + getWorkspaceReviewSummary: transport?.getWorkspaceReviewSummary?.bind(transport), + listWorkspaceReviewFileDiffs: transport?.listWorkspaceReviewFileDiffs?.bind(transport), + searchWorkspaceReviewDiffs: transport?.searchWorkspaceReviewDiffs?.bind(transport), + loadSessions: transport?.loadSessions?.bind(transport), }), [transport]); const { listModels, selectModel, getUsageQuota, getAccountStatus, @@ -130,6 +163,8 @@ export function SettingsModal({ open, onClose, dataDir, version, sessionId, tran getMiniMaxModelSource, setMiniMaxModelSource, testUserModelCandidate, revealModelProviderApiKey, startCodexOAuthLogin, cancelCodexOAuthLogin, refreshModels, + getWorkspaceReviewSummary, listWorkspaceReviewFileDiffs, searchWorkspaceReviewDiffs, + loadSessions, } = boundCapabilities; const usageCapabilities: WebuiSettingsModalCapabilities = useMemo(() => ({ getUsageQuota, getMiniMaxApiKeyStatus, listUserModelProviders, createUserModelProvider, @@ -146,7 +181,7 @@ export function SettingsModal({ open, onClose, dataDir, version, sessionId, tran const visibleTabs = useMemo(() => filterSettingsTabs(query), [query]); if (!open) return null; const selected = models.find((model) => model.selected); const modelValue = selected ? `${selected.providerId}/${selected.modelId}/${selected.variant ?? ""}` : ""; const groups = SETTINGS_GROUPS.map((group) => ({ ...group, tabs: visibleTabs.filter((tab) => tab.group === group.key) })).filter((group) => group.tabs.length > 0); const label = DESKTOP_SETTINGS_TABS.find((tab) => tab.key === active)?.label; const changeModel = async (value: string) => { const model = models.find((candidate) => `${candidate.providerId}/${candidate.modelId}/${candidate.variant ?? ""}` === value); if (!model || !selectModel) return; await selectModel({ providerId: model.providerId, modelId: model.modelId, ...(model.variant ? { variant: model.variant } : {}), ...(sessionId ? { sessionId } : {}) }); }; const handleSignOut = async () => { if (!signOut) return; try { setSignOutError(undefined); await signOut(); onClose(); } catch (error) { setSignOutError(error instanceof Error ? error.message : String(error)); } }; const handleDeleteAllArchived = async () => { if (!deleteSession || !archived.length || !window.confirm("确定删除全部已归档任务吗？此操作无法撤销。")) return; const ids = archived.map((session) => session.sessionId); try { await Promise.all(ids.map((id) => deleteSession({ id }))); setArchived([]); } catch (error) { window.alert(`删除失败：${error instanceof Error ? error.message : String(error)}`); } }; - return
{ if (event.target === event.currentTarget) onClose(); }}>
event.stopPropagation()}>

{label}

{active === "archived" ? : null}
{active === "desktop" ? : null}{active === "usage" ? : null}{active === "account" ?
{signOutError ?

{signOutError}

: null}
: null}{active === "archived" ? { if (!deleteSession) return; await deleteSession({ id }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} onUnarchive={async (id) => { if (!archiveSession) return; await archiveSession({ id, archived: false }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} /> : null}{active !== "desktop" && active !== "usage" && active !== "account" && active !== "archived" ?
: null}{active === "desktop" && dataDir ?

{dataDir}

: null}
; + return
{ if (event.target === event.currentTarget) onClose(); }}>
event.stopPropagation()}>

{label}

{active === "archived" ? : null}
{active === "desktop" ? : null}{active === "usage" ? : null}{active === "account" ?
{signOutError ?

{signOutError}

: null}
: null}{active === "archived" ? { if (!deleteSession) return; await deleteSession({ id }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} onUnarchive={async (id) => { if (!archiveSession) return; await archiveSession({ id, archived: false }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} /> : null}{active === "coding" ? : null}{active === "worktree" ? : null}{active !== "desktop" && active !== "usage" && active !== "account" && active !== "archived" && active !== "coding" && active !== "worktree" ?
: null}{active === "desktop" && dataDir ?

{dataDir}

: null}
; } function GenericPage({ theme, setTheme, wrap, setWrap, newTab, setNewTab, contextWindow, setContextWindow, version }: { readonly theme: string; readonly setTheme: (value: string) => void; readonly wrap: boolean; readonly setWrap: (value: boolean) => void; readonly newTab: boolean; readonly setNewTab: (value: boolean) => void; readonly contextWindow: boolean; readonly setContextWindow: (value: boolean) => void; readonly version: string }): ReactElement { @@ -155,6 +190,321 @@ function GenericPage({ theme, setTheme, wrap, setWrap, newTab, setNewTab, contex function Appearance({ theme, setTheme }: { readonly theme: string; readonly setTheme: (value: string) => void }): ReactElement { return
{([['light', '浅色模式', 'light.d3fbb1aa.svg'], ['dark', '深色模式', 'dark.14c569ba.svg'], ['system', '跟随系统', 'system.aba90841.svg']] as const).map(([value, text, src]) => )}
; } function ModeCard({ testId, title, description, icon, selected = false }: { readonly testId: string; readonly title: string; readonly description: string; readonly icon: string; readonly selected?: boolean }): ReactElement { return ; } function SettingPanel({ title, children }: { readonly title: string; readonly children: ReactNode }): ReactElement { return

{title}

{children}
; } +/** The subset of the settings capability contract the code-review page reads. + * Named so the page cannot quietly grow a dependency on a member the modal + * does not otherwise use. */ +type WebuiSettingsReviewCapabilities = Pick< + WebuiSettingsModalCapabilities, + "getWorkspaceReviewSummary" | "listWorkspaceReviewFileDiffs" | "searchWorkspaceReviewDiffs" +>; + +type WebuiReviewStateAction = Parameters[1]; + +/** Roadmap E 区「Review 审查模式」and「修复建议+跳转」. + * + * The review capability was already wired end to end; what was missing was a + * place to read a whole change set. Every decision that does not need the + * network lives in `review-state.ts` so it can be tested without a DOM — this + * component is the thin shell that fetches and renders. */ +function SettingsReviewPage({ workspaceDir, onOpenFileLine, getWorkspaceReviewSummary, listWorkspaceReviewFileDiffs, searchWorkspaceReviewDiffs }: { + readonly workspaceDir?: string; + readonly onOpenFileLine?: (path: string, line: number) => void; +} & WebuiSettingsReviewCapabilities): ReactElement { + const [state, setState] = useState(initialWebuiReviewState); + const dispatch = useCallback((action: WebuiReviewStateAction) => { + setState((current) => reduceWebuiReviewState(current, action)); + }, []); + + useEffect(() => { + if (!workspaceDir || !getWorkspaceReviewSummary) return; + let cancelled = false; + dispatch({ type: "load-begun" }); + void getWorkspaceReviewSummary({ workspaceDir }) + .then((summary) => { + if (cancelled) return; + const snapshotId = summary?.reviewSnapshotId; + const files = summary?.files ?? []; + // A workspace with nothing staged against it is a normal state, and + // showing it as a failure would put a red banner on every fresh repo. + if (!snapshotId || !files.length) { + dispatch({ type: "unavailable", reason: "当前工作区没有待审查的变更" }); + return; + } + const totals = summary?.totals ?? { + files: files.length, + additions: files.reduce((sum, file) => sum + (file.additions ?? 0), 0), + deletions: files.reduce((sum, file) => sum + (file.deletions ?? 0), 0), + }; + dispatch({ type: "summary-loaded", reviewSnapshotId: snapshotId, files: [...files], totals }); + }) + .catch((error: unknown) => { + if (cancelled) return; + dispatch({ type: "load-failed", reason: error instanceof Error ? error.message : String(error) }); + }); + return () => { cancelled = true; }; + }, [dispatch, getWorkspaceReviewSummary, workspaceDir]); + + /* The snapshot id and the query are passed in rather than read back out of + * state: reading a `useState` value from inside a setter callback is a type + * error waiting to happen and silently captures whatever the reducer saw, + * not what the caller meant. */ + const loadDiffs = useCallback(async (fileIds: readonly string[], reviewSnapshotId: string) => { + if (!workspaceDir || !listWorkspaceReviewFileDiffs || !fileIds.length) return; + dispatch({ type: "diffs-begun", fileIds }); + for (let index = 0; index < fileIds.length; index += WEBUI_REVIEW_DIFF_BATCH_SIZE) { + const batch = fileIds.slice(index, index + WEBUI_REVIEW_DIFF_BATCH_SIZE); + try { + const result = await listWorkspaceReviewFileDiffs({ workspaceDir, reviewSnapshotId, fileIds: [...batch] }); + const diffs: Record = {}; + const errors: Record = {}; + for (const entry of (result?.diffs ?? []) as readonly WebuiWorkspaceReviewFileDiff[]) { + if (entry.error) errors[entry.fileId] = entry.error; + else if (entry.diff?.type === "text" && entry.diff.content) diffs[entry.fileId] = entry.diff.content; + else if (entry.diff?.type === "binary") errors[entry.fileId] = "二进制文件没有可显示的补丁"; + else errors[entry.fileId] = "运行时没有返回这个文件的补丁"; + } + dispatch({ type: "diffs-loaded", diffs, errors }); + } catch (error: unknown) { + dispatch({ type: "diffs-loaded", diffs: {}, errors: Object.fromEntries(batch.map((fileId) => [fileId, error instanceof Error ? error.message : String(error)])) }); + } + } + }, [dispatch, listWorkspaceReviewFileDiffs, workspaceDir]); + + const runSearch = useCallback(async (reviewSnapshotId: string, query: string) => { + if (!workspaceDir || !searchWorkspaceReviewDiffs || !query.trim()) return; + dispatch({ type: "search-begun" }); + try { + const result = await searchWorkspaceReviewDiffs({ workspaceDir, reviewSnapshotId, query, includeUntrackedFiles: true }); + dispatch({ type: "search-settled", fileIds: (result?.matchedFiles ?? []).map((entry) => entry.fileId) }); + } catch { + // A failed search must not leave the list filtered by the previous one. + dispatch({ type: "search-settled", fileIds: [] }); + } + }, [dispatch, searchWorkspaceReviewDiffs, workspaceDir]); + + const visible = selectWebuiReviewVisibleFiles(state); + const filtering = isWebuiReviewFiltering(state); + + if (!workspaceDir) { + return ; + } + if (state.status === "idle" || state.status === "loading") { + return ; + } + if (state.status === "unavailable" || state.status === "error") { + return ; + } + + return { + dispatch({ type: "query-changed", query }); + // Live search: the server is asked for every keystroke, and the + // reducer drops the previous matches first so the list never shows + // results for a query that is no longer in the box. + if (state.reviewSnapshotId && query.trim()) void runSearch(state.reviewSnapshotId, query); + }} + onClearFilters={() => dispatch({ type: "clear-filters" })} + onToggleFile={(fileId, willExpand, file) => { + dispatch({ type: "toggle-file", fileId }); + if (willExpand && !state.diffs[fileId] && !state.diffErrors[fileId] && state.reviewSnapshotId) { + void loadDiffs([fileId], state.reviewSnapshotId); + } + void file; + }} + onOpenFileLine={onOpenFileLine} + />; +} + +/** Presentational half of the code-review page. + * + * Split from the fetching shell on purpose: `renderToStaticMarkup` runs the + * first render and never runs effects, so a page that only reaches its + * populated state through an effect cannot be asserted on at all. Everything + * that depends on data is decided upstream in `review-state.ts` and arrives + * here as a plain value, which makes the populated render reachable from a + * test without a DOM. */ +export function WebuiReviewPanel({ state, visible, filtering, loading, note, empty, workspaceDir, onQueryChange, onClearFilters, onToggleFile, onOpenFileLine }: { + readonly state: WebuiReviewState; + readonly visible?: readonly WebuiReviewFile[]; + readonly filtering?: boolean; + readonly loading?: boolean; + readonly note?: string; + readonly empty?: string; + readonly workspaceDir?: string; + readonly onQueryChange?: (query: string) => void; + readonly onClearFilters?: () => void; + readonly onToggleFile?: (fileId: string, willExpand: boolean, file: WebuiReviewFile) => void; + readonly onOpenFileLine?: (path: string, line: number) => void; +}): ReactElement { + const rows = visible ?? state.files; + const filterActive = filtering ?? isWebuiReviewFiltering(state); + return
+ {state.status === "ready" ?
+ {state.totals.files} 个文件 + {`+${state.totals.additions}`} + {`-${state.totals.deletions}`} +
: null} + {state.status === "ready" ?
+ + onQueryChange?.(event.target.value)} + /> + {filterActive ? : null} +
: null} + {loading || state.searchPending ?

{loading ? "正在读取待审查的变更…" : "正在搜索…"}

: null} + {note || state.error ?

{note ?? state.error}

: null} + {workspaceDir ?

{workspaceDir}

: null} + {empty && !note ?

{empty}

: null} + {state.status === "ready" && rows.length === 0 ?

{filterActive ? "没有匹配的文件" : "没有可显示的文件"}

: null} +
    + {rows.map((file) => { + const expanded = state.expandedFileIds.includes(file.fileId); + const diff = state.diffs[file.fileId]; + const diffError = state.diffErrors[file.fileId]; + const pending = state.loadingFileIds.includes(file.fileId); + return
  • + + {expanded ?
    + {pending ?

    正在读取这个文件的补丁…

    : null} + {diffError ?

    {diffError}

    : null} + {diff ?
      + {projectWebuiReviewLines(diff).map((line, index) => { + const target = webuiReviewLineTarget(line); + const text = ( + <> + {line.newLine ?? ""} + {line.text} + + ); + return
    1. + {target === undefined || !onOpenFileLine + ? {text} + : } +
      2. ; + })} +
    : null} +
    : null} +
    • ; + })} +
+
; +} + +/** Roadmap E 区「工作树隔离」. + * + * Creating an isolated worktree already works from the rail's context menu; + * this is the other half of the row — being able to see which parallel + * experiment branches exist and switch into one. Grouping is done by + * `groupWebuiWorktreeWorkspaces`, so the list a test asserts on is the same + * list the page renders. */ +function SettingsWorktreePage({ loadSessions }: { + readonly loadSessions?: WebuiSettingsModalCapabilities["loadSessions"]; +}): ReactElement { + const [status, setStatus] = useState<"loading" | "ready" | "error">("loading"); + const [sessions, setSessions] = useState([]); + const [error, setError] = useState(); + + useEffect(() => { + if (!loadSessions) return; + let cancelled = false; + setStatus("loading"); + void loadSessions() + .then((page) => { + if (cancelled) return; + setSessions((page?.sessions ?? []) as readonly WebuiWorktreeSourceSession[]); + setStatus("ready"); + }) + .catch((cause: unknown) => { + if (cancelled) return; + setError(cause instanceof Error ? cause.message : String(cause)); + setStatus("error"); + }); + return () => { cancelled = true; }; + }, [loadSessions]); + + const workspaces = groupWebuiWorktreeWorkspaces(sessions); + const worktrees = selectWebuiWorktreeWorkspaces(workspaces); + const primary = selectWebuiPrimaryWorkspace(workspaces); + + if (status === "error") { + return ; + } + if (status === "loading") { + return
+

正在读取工作树…

+
; + } + return ; +} + +/** Presentational half of the worktree page, split for the same reason as + * `WebuiReviewPanel`: `renderToStaticMarkup` runs no effects, so a page that + * only reaches its populated state through a fetch cannot be asserted on. */ +export function WebuiWorktreePanel({ workspaces, worktrees, primary, error }: { + readonly workspaces: readonly WebuiWorktreeWorkspace[]; + readonly worktrees?: readonly WebuiWorktreeWorkspace[]; + readonly primary?: WebuiWorktreeWorkspace; + readonly error?: string; +}): ReactElement { + const branches = worktrees ?? selectWebuiWorktreeWorkspaces(workspaces); + const main = primary ?? selectWebuiPrimaryWorkspace(workspaces); + return
+ {error ?

{error}

: null} + {main ?
+
+ {main.name} + 主检出 +
+

{main.workspaceDir}

+
    + {main.sessions.map((entry) =>
  • + {entry.title} +
    • )} +
+
: null} +

并行实验分支

+ {branches.length === 0 ?

还没有工作树。在会话的右键菜单里选「复制到新工作树」即可开一个。

: null} +
    + {branches.map((branch) =>
  • +
    + {branch.name} + {branch.sessions.length} 个会话 +
    +

    {branch.workspaceDir}

    +
      + {branch.sessions.map((entry) =>
    • + {/* Session navigation in this shell is the `#session=` hash, + which is what the rail's own links use. Reusing it keeps one + navigation path instead of adding a second one. */} + {entry.title} + {entry.parentSessionId ? 派生 : null} +
      • )} +
    +
    • )} +
+
; +} + function ArchivedSessionsPage({ sessions, canDelete, canUnarchive, onDelete, onUnarchive }: { readonly sessions: readonly WebuiSessionListItem[]; readonly canDelete: boolean; readonly canUnarchive: boolean; readonly onDelete: (id: string) => Promise; readonly onUnarchive: (id: string) => Promise }): ReactElement { const [search, setSearch] = useState(""); const [actionError, setActionError] = useState(""); diff --git a/packages/webui/src/client/components/UserMenu.tsx b/packages/webui/src/client/components/UserMenu.tsx index 5fd31f90b..1739cd07d 100644 --- a/packages/webui/src/client/components/UserMenu.tsx +++ b/packages/webui/src/client/components/UserMenu.tsx @@ -37,6 +37,12 @@ interface UserMenuProps { readonly dataDir?: string; readonly version?: WebuiVersionInfo; readonly sessionId?: string; + /** Forwarded to the settings modal so the code-review page knows which + * workspace to read a change set from. */ + readonly workspaceDir?: string; + /** Forwarded to the settings modal. The review page turns a diff line into + * a real editor jump through it. */ + readonly onOpenFileLine?: (path: string, line: number) => void; /** Capability source for the menu's own panels and the settings modal. * Typed as the narrow 9-member contract so neither the menu nor the * modal can accidentally start reading members they do not consume. */ @@ -685,6 +691,8 @@ export function UserMenu({ dataDir, sessionId, version, + workspaceDir, + onOpenFileLine, transport, getSigninPanel, claimSignin, @@ -860,6 +868,6 @@ export function UserMenu({ : null} - {typeof document !== "undefined" ? createPortal( setSettingsOpen(false)} dataDir={dataDir} version={version} sessionId={sessionId} transport={transport} />, document.body) : setSettingsOpen(false)} dataDir={dataDir} version={version} sessionId={sessionId} transport={transport} />} + {typeof document !== "undefined" ? createPortal( setSettingsOpen(false)} dataDir={dataDir} version={version} sessionId={sessionId} workspaceDir={workspaceDir} onOpenFileLine={onOpenFileLine} transport={transport} />, document.body) : setSettingsOpen(false)} dataDir={dataDir} version={version} sessionId={sessionId} workspaceDir={workspaceDir} onOpenFileLine={onOpenFileLine} transport={transport} />} ; } diff --git a/packages/webui/src/client/components/WebuiClientFoundationApp.tsx b/packages/webui/src/client/components/WebuiClientFoundationApp.tsx index 445881efc..dfb714fba 100644 --- a/packages/webui/src/client/components/WebuiClientFoundationApp.tsx +++ b/packages/webui/src/client/components/WebuiClientFoundationApp.tsx @@ -1232,6 +1232,15 @@ export function WebuiClientFoundationApp( dataDir={dataDir} version={runtimeVersion} sessionId={selectedSessionId} + workspaceDir={selectedSession?.workspaceDir} + onOpenFileLine={(path, line) => { + // The open-file command needs a concrete session and + // workspace; the review page is only reachable from a + // selected session, so both are present in practice, but + // a jump must never fire a command with holes in it. + if (!selectedSessionId || !selectedSession?.workspaceDir) return; + dispatchWorkspacePanel({ type: "open-file", sessionId: selectedSessionId, workspaceDir: selectedSession.workspaceDir, path, lineStart: line, lineEnd: line }); + }} transport={transport} getSigninPanel={transport?.getSigninPanel} claimSignin={transport?.claimSignin} diff --git a/packages/webui/src/client/projection/review-state.ts b/packages/webui/src/client/projection/review-state.ts new file mode 100644 index 000000000..06071e678 --- /dev/null +++ b/packages/webui/src/client/projection/review-state.ts @@ -0,0 +1,273 @@ +/** Pure state for the WebUI code-review surface. + * + * The workspace review capability (getWorkspaceReviewSummary / + * listWorkspaceReviewFileDiffs / searchWorkspaceReviewDiffs) is already wired + * end to end — the diff card's Review button dispatches into the workspace + * panel. What was missing was a place to read a whole change set, which is + * what roadmap E 区's "Review 审查模式" row asks for. + * + * Everything that can be decided without the network lives here so it can be + * tested directly: the webui suite runs `environment: "node"` with no jsdom, + * so the component itself is only reachable through `renderToStaticMarkup`, + * which runs no effects and fires no events. + */ + +export interface WebuiReviewFile { + readonly fileId: string; + readonly path: string; + readonly originalPath?: string; + readonly status: string; + readonly type?: "text" | "binary"; + readonly additions: number; + readonly deletions: number; +} + +export interface WebuiReviewTotals { + readonly files: number; + readonly additions: number; + readonly deletions: number; +} + +/** One rendered diff line, carrying the new-side line number so a click can + * be turned into a real "open this file at this line" request. */ +export interface WebuiReviewLine { + readonly kind: "context" | "addition" | "deletion" | "hunk" | "meta"; + readonly text: string; + /** 1-based line number on the new side, when this line exists there. */ + readonly newLine?: number; + /** 1-based line number on the old side, when this line exists there. */ + readonly oldLine?: number; +} + +export type WebuiReviewStatus = "idle" | "loading" | "ready" | "unavailable" | "error"; + +export interface WebuiReviewState { + readonly status: WebuiReviewStatus; + readonly files: readonly WebuiReviewFile[]; + readonly totals: WebuiReviewTotals; + readonly reviewSnapshotId?: string; + /** Unified diff text per fileId. Absent means "not loaded yet". */ + readonly diffs: Readonly>; + /** Per-file failure text, kept apart from the diff so a file that failed to + * load never reads as a file with no changes. */ + readonly diffErrors: Readonly>; + readonly loadingFileIds: readonly string[]; + readonly expandedFileIds: readonly string[]; + readonly query: string; + /** Non-empty only while a search has run. An empty array means "no match", + * which is why the query these matches belong to is kept alongside them: + * before a search has run for the current query the list must stay + * unfiltered, and after one that matched nothing it must be empty. */ + readonly searchedQuery: string; + readonly matchedFileIds: readonly string[]; + readonly searchPending: boolean; + readonly error?: string; +} + +export const initialWebuiReviewState: WebuiReviewState = { + status: "idle", + files: [], + totals: { files: 0, additions: 0, deletions: 0 }, + diffs: {}, + diffErrors: {}, + loadingFileIds: [], + expandedFileIds: [], + query: "", + searchedQuery: "", + matchedFileIds: [], + searchPending: false, +}; + +export type WebuiReviewStateAction = + | { readonly type: "load-begun" } + | { + readonly type: "summary-loaded"; + readonly reviewSnapshotId: string; + readonly files: readonly WebuiReviewFile[]; + readonly totals: WebuiReviewTotals; + } + | { readonly type: "unavailable"; readonly reason: string } + | { readonly type: "load-failed"; readonly reason: string } + | { readonly type: "diffs-begun"; readonly fileIds: readonly string[] } + | { + readonly type: "diffs-loaded"; + readonly diffs: Readonly>; + readonly errors?: Readonly>; + } + | { readonly type: "toggle-file"; readonly fileId: string } + | { readonly type: "query-changed"; readonly query: string } + | { readonly type: "search-begun" } + | { readonly type: "search-settled"; readonly fileIds: readonly string[] } + | { readonly type: "clear-filters" }; + +const withoutKeys = (source: Readonly>, keys: readonly string[]): Record => { + const next: Record = { ...source }; + for (const key of keys) delete next[key]; + return next; +}; + +const toggle = (list: readonly string[], value: string): readonly string[] => + list.includes(value) ? list.filter((entry) => entry !== value) : [...list, value]; + +export function reduceWebuiReviewState( + state: WebuiReviewState, + action: WebuiReviewStateAction, +): WebuiReviewState { + switch (action.type) { + case "load-begun": + // A reload must not leave the previous snapshot's diffs on screen + // against a new reviewSnapshotId: those file ids belong to another + // snapshot and would render as this one's changes. + return { + ...state, + status: "loading", + error: undefined, + diffs: {}, + diffErrors: {}, + loadingFileIds: [], + matchedFileIds: [], + searchedQuery: "", + searchPending: false, + }; + case "summary-loaded": + return { + ...state, + status: "ready", + error: undefined, + reviewSnapshotId: action.reviewSnapshotId, + files: action.files, + totals: action.totals, + }; + case "unavailable": + // Distinct from "error": the runtime has no review snapshot for this + // workspace, which is a normal state, not a failure to report as one. + return { ...state, status: "unavailable", error: undefined, searchPending: false }; + case "load-failed": + return { ...state, status: "error", error: action.reason, searchPending: false }; + case "diffs-begun": + return { ...state, loadingFileIds: [...new Set([...state.loadingFileIds, ...action.fileIds])] }; + case "diffs-loaded": { + const errors = action.errors ?? {}; + // A file leaves the in-flight list when this response carried it, either + // as a diff or as an error. Deriving the settled set from the incoming + // payload rather than from what is already in state is what makes the + // marker clear: a freshly loaded file is not in `state.diffs` yet, so + // reading only the existing state would leave it spinning forever. + const settled = [...new Set([...Object.keys(action.diffs), ...Object.keys(errors)])].filter( + (fileId) => state.loadingFileIds.includes(fileId), + ); + return { + ...state, + diffs: { ...withoutKeys(state.diffs, settled), ...action.diffs }, + diffErrors: { ...withoutKeys(state.diffErrors, settled), ...errors }, + loadingFileIds: state.loadingFileIds.filter((fileId) => !settled.includes(fileId)), + }; + } + case "toggle-file": + return { ...state, expandedFileIds: toggle(state.expandedFileIds, action.fileId) }; + case "query-changed": + // Editing the query invalidates the previous result set immediately. + // Keeping it would filter the list by a query that no longer matches. + return { ...state, query: action.query, matchedFileIds: [], searchedQuery: "", searchPending: false }; + case "search-begun": + return { ...state, searchPending: true }; + case "search-settled": + return { + ...state, + searchPending: false, + searchedQuery: state.query, + matchedFileIds: action.fileIds, + }; + case "clear-filters": + return { ...state, query: "", searchedQuery: "", matchedFileIds: [], searchPending: false }; + } +} + +/** The file list the page should render, honouring an active search filter. + * + * "No query yet", "query not searched yet" and "query matched nothing" all + * produce different lists, so the filter is only applied once a search has + * actually settled for the query currently in the box. */ +export function selectWebuiReviewVisibleFiles(state: WebuiReviewState): readonly WebuiReviewFile[] { + const query = state.query.trim(); + if (!query || state.searchedQuery !== state.query) return state.files; + const matched = new Set(state.matchedFileIds); + return state.files.filter((file) => matched.has(file.fileId)); +} + +export function isWebuiReviewFiltering(state: WebuiReviewState): boolean { + return state.query.trim().length > 0; +} + +/** Splits a unified diff into renderable lines, numbering both sides. + * + * The new-side number is what makes "诊断 → 建议 → 行内定位" possible: a click + * on an added line has to resolve to a real line in the file the editor will + * open, and only the `+` side survives into the working tree. Hunk headers + * carry no line of their own, so they are reported as `hunk` with no number + * rather than inheriting the previous line's. */ +export function projectWebuiReviewLines(unifiedDiff: string): readonly WebuiReviewLine[] { + const lines = unifiedDiff.split(/\r?\n/u); + const projected: WebuiReviewLine[] = []; + let oldLine = 0; + let newLine = 0; + let inHunk = false; + for (const raw of lines) { + if (raw.startsWith("@@")) { + const header = /^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/u.exec(raw); + oldLine = header ? Number(header[1]) : 0; + newLine = header ? Number(header[2]) : 0; + inHunk = true; + projected.push({ kind: "hunk", text: raw }); + continue; + } + if (raw.startsWith("diff ") || raw.startsWith("index ") || raw.startsWith("--- ") || raw.startsWith("+++ ")) { + projected.push({ kind: "meta", text: raw }); + continue; + } + if (!inHunk) { + // Content before the first hunk header is file metadata, not a line of + // either side; numbering it would point the editor at the wrong place. + projected.push({ kind: "meta", text: raw }); + continue; + } + if (raw.startsWith("+")) { + projected.push({ kind: "addition", text: raw.slice(1), newLine }); + newLine += 1; + continue; + } + if (raw.startsWith("-")) { + projected.push({ kind: "deletion", text: raw.slice(1), oldLine }); + oldLine += 1; + continue; + } + if (raw.startsWith("\\")) { + // "\ No newline at end of file" belongs to the preceding line and must + // not consume a line number. + projected.push({ kind: "meta", text: raw }); + continue; + } + projected.push({ kind: "context", text: raw.slice(1), newLine, oldLine }); + newLine += 1; + oldLine += 1; + } + return projected; +} + +/** The line an editor jump should target, if this line can have one. + * + * A deleted line is not in the file the editor will open, and a hunk header + * or a file-metadata row is not a line of the file at all. Rather than + * re-check the kinds here, this reads the projection's own invariant: only + * `addition` and `context` rows are ever given a `newLine`, so a row without + * one has no destination. That invariant is asserted directly in + * `review-state.test.ts` — putting the check in a guard instead made it + * untestable, since removing the guard could not change the result. */ +export function webuiReviewLineTarget(line: WebuiReviewLine): number | undefined { + return line.newLine; +} + +/** Batch size for listWorkspaceReviewFileDiffs. The server takes a fileId + * array per call; the workspace panel already batches at five and the same + * batcher is reused here so both surfaces behave identically. */ +export const WEBUI_REVIEW_DIFF_BATCH_SIZE = 5; diff --git a/packages/webui/src/client/projection/worktree-state.ts b/packages/webui/src/client/projection/worktree-state.ts new file mode 100644 index 000000000..ae03b92bc --- /dev/null +++ b/packages/webui/src/client/projection/worktree-state.ts @@ -0,0 +1,128 @@ +/** Pure grouping for the WebUI worktree surface (roadmap E 区「工作树隔离」). + * + * Creating an isolated worktree already works — the rail's context menu + * carries 「复制到新工作树」 and the server answers with + * `worktreeVisible` / `worktreeUnavailableReason`. What did not exist was a + * place to see what those worktrees are, which is the "并行实验分支" half of + * the row. + * + * A worktree is not a first-class field on a session. It is inferred: git + * creates a second checkout with its own absolute path, so a session whose + * `workspaceDir` differs from the project's default checkout is running in + * one. `isDefaultWorkspace` is the runtime's own statement of which checkout + * is the primary one, and it is trusted over any guess made here. + * + * Grouping is a pure function so it can be asserted without a DOM: the page + * that renders it runs `renderToStaticMarkup`, which never runs effects. + */ + +export interface WebuiWorktreeSession { + readonly sessionId: string; + readonly title: string; + readonly updatedAt: number; + readonly parentSessionId?: string; +} + +export interface WebuiWorktreeWorkspace { + readonly workspaceDir: string; + readonly name: string; + /** True for the project's own checkout. A worktree is a non-primary one. */ + readonly isPrimary: boolean; + readonly sessions: readonly WebuiWorktreeSession[]; + readonly updatedAt: number; +} + +export interface WebuiWorktreeSourceSession { + readonly sessionId: string; + readonly title?: string; + readonly archived?: boolean; + readonly updatedAt: number; + readonly workspaceDir?: string; + readonly isDefaultWorkspace?: boolean; + readonly parentSessionId?: string; +} + +/** Windows and POSIX spell the same checkout differently in stored paths + * (`C:\repo` vs `C:/repo`). Normalising the separator keeps one worktree from + * being reported twice because a fork round-tripped the path. */ +export function normalizeWebuiWorkspaceDir(workspaceDir: string): string { + return workspaceDir.trim().replace(/\\/gu, "/").replace(/\/+$/u, "").toLowerCase(); +} + +export function webuiWorkspaceName(workspaceDir: string): string { + const normalized = workspaceDir.trim().replace(/[\\/]+$/u, ""); + if (!normalized) return normalized; + const segments = normalized.split(/[\\/]/u).filter(Boolean); + return segments.at(-1) ?? normalized; +} + +/** Groups sessions into checkouts, most recently touched first. + * + * Archived sessions are dropped: the archived page already owns them, and a + * worktree whose every session is archived is not a parallel experiment + * branch anyone is using. Sessions with no `workspaceDir` are dropped too — + * there is no checkout to attach them to, and inventing a bucket for them + * would show up as a phantom worktree. */ +export function groupWebuiWorktreeWorkspaces( + sessions: readonly WebuiWorktreeSourceSession[], +): readonly WebuiWorktreeWorkspace[] { + const groups = new Map(); + for (const session of sessions) { + if (session.archived) continue; + const raw = session.workspaceDir?.trim(); + if (!raw) continue; + const key = normalizeWebuiWorkspaceDir(raw); + if (!key) continue; + const existing = groups.get(key); + const entry: WebuiWorktreeSession = { + sessionId: session.sessionId, + title: session.title?.trim() || session.sessionId, + updatedAt: session.updatedAt, + ...(session.parentSessionId ? { parentSessionId: session.parentSessionId } : {}), + }; + if (existing) { + existing.sessions.push(entry); + // A checkout only counts as primary if the runtime said so. A later + // session without the flag must not demote one that has it. + existing.isPrimary = existing.isPrimary || session.isDefaultWorkspace === true; + existing.updatedAt = Math.max(existing.updatedAt, session.updatedAt); + continue; + } + groups.set(key, { + workspaceDir: raw, + isPrimary: session.isDefaultWorkspace === true, + sessions: [entry], + updatedAt: session.updatedAt, + }); + } + const workspaces = [...groups.values()].map((group) => ({ + workspaceDir: group.workspaceDir, + name: webuiWorkspaceName(group.workspaceDir), + isPrimary: group.isPrimary, + sessions: [...group.sessions].sort((a, b) => b.updatedAt - a.updatedAt), + updatedAt: group.updatedAt, + })); + // Primary first, then most recently touched. The tie-break on path keeps the + // order stable when two checkouts share a timestamp, which a fast test + // fixture almost always does. + return workspaces.sort((a, b) => { + if (a.isPrimary !== b.isPrimary) return a.isPrimary ? -1 : 1; + if (a.updatedAt !== b.updatedAt) return b.updatedAt - a.updatedAt; + return a.workspaceDir.localeCompare(b.workspaceDir); + }); +} + +/** The parallel experiment branches: every checkout that is not the project's + * own. This is the list the page is named after, so it is kept separate from + * the grouping function rather than re-derived by the component. */ +export function selectWebuiWorktreeWorkspaces( + workspaces: readonly WebuiWorktreeWorkspace[], +): readonly WebuiWorktreeWorkspace[] { + return workspaces.filter((workspace) => !workspace.isPrimary); +} + +export function selectWebuiPrimaryWorkspace( + workspaces: readonly WebuiWorktreeWorkspace[], +): WebuiWorktreeWorkspace | undefined { + return workspaces.find((workspace) => workspace.isPrimary); +} diff --git a/packages/webui/src/client/styles/shell.css b/packages/webui/src/client/styles/shell.css index 44533d1d6..0547612e2 100644 --- a/packages/webui/src/client/styles/shell.css +++ b/packages/webui/src/client/styles/shell.css @@ -252,6 +252,52 @@ .webui-archived-delete-all { display: inline-flex; height: 36px; align-items: center; gap: 6px; border: 0; border-radius: 12px; padding: 0 12px; background: rgba(255, 59, 48, .1); color: #e5484d; font: inherit; font-size: var(--size_14); cursor: pointer; } .webui-archived-delete-all:hover { background: rgba(255, 59, 48, .16); } .webui-archived-delete-all:disabled { opacity: .45; cursor: default; } + + /* Code review (roadmap E 区 Review 审查模式 / 修复建议+跳转). */ + .webui-review-page { width: 100%; max-width: 760px; margin: 0 auto; padding: 0 var(--spacing_20) var(--spacing_32); } + .webui-review-totals { display: flex; align-items: center; gap: var(--spacing_8); margin-bottom: var(--spacing_12); color: var(--text_default_secondary); font-size: var(--size_12); } + .webui-review-search { display: flex; min-width: 0; height: 38px; align-items: center; gap: var(--spacing_8); margin-bottom: var(--spacing_12); border: 1px solid var(--border_default); border-radius: 12px; padding: 0 12px; background: var(--bg_default_primary); color: var(--text_default_secondary); } + .webui-review-search:focus-within { border-color: var(--border_accent); } + .webui-review-search input { width: 100%; min-width: 0; border: 0; outline: 0; background: transparent; color: var(--text_default_primary); font: inherit; font-size: var(--size_14); } + .webui-review-clear { flex: none; border: 0; background: transparent; color: var(--text_default_secondary); font-size: var(--size_12); } + .webui-review-files { display: flex; flex-direction: column; gap: var(--spacing_4); margin: 0; padding: 0; list-style: none; } + .webui-review-file { border: 0.5px solid var(--border_default); border-radius: 10px; overflow: hidden; } + .webui-review-file-heading { display: flex; width: 100%; min-width: 0; align-items: center; gap: var(--spacing_8); padding: 10px 12px; border: 0; background: transparent; color: inherit; font: inherit; text-align: left; } + .webui-review-file-heading:hover { background: var(--bg_interaction_secondary_default); } + .webui-review-file-path { min-width: 0; flex: 1; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; color: var(--text_default_primary); font-size: var(--size_12); } + .webui-review-file-status { flex: none; color: var(--text_default_tertiary); font-size: var(--size_12); } + .webui-review-file-stats { display: inline-flex; flex: none; gap: 4px; font-size: var(--size_12); } + .webui-review-file-body { border-top: 0.5px solid var(--border_default); background: var(--bg_default_primary); } + .webui-review-file-error { margin: 0; padding: 10px 12px; color: var(--text_label_danger_secondary_default); font-size: var(--size_12); } + .webui-review-lines { max-height: 420px; margin: 0; padding: 0; overflow: auto; list-style: none; font-family: var(--font-mono, monospace); font-size: var(--size_12); line-height: 1.6; } + .webui-review-line { display: flex; min-width: max-content; } + .webui-review-line--addition { background: var(--bg_status_positive); color: #17251d; } + .webui-review-line--deletion { background: var(--bg_status_error); color: #302022; } + .webui-review-line--hunk { background: var(--bg_interaction_secondary_default); color: var(--text_default_tertiary); } + .webui-review-line--meta { color: var(--text_default_tertiary); } + .webui-review-line-number { flex: none; width: 48px; padding-right: 8px; opacity: .6; text-align: right; user-select: none; } + .webui-review-line-text { flex: 1; padding-right: 12px; white-space: pre; } + /* Only a line that survives into the working tree can be jumped to, so the + clickable surface is deliberately absent on deletions. */ + .webui-review-line-jump { display: flex; min-width: max-content; width: 100%; border: 0; padding: 0; background: transparent; color: inherit; font: inherit; text-align: left; cursor: pointer; } + .webui-review-line-jump:hover { text-decoration: underline; } + .webui-review-line-static { display: flex; min-width: max-content; width: 100%; } + .webui-review-loading, .webui-review-empty { margin: 0; padding: var(--spacing_16) 0; color: var(--text_default_secondary); font-size: var(--size_12); } + .webui-review-error { margin: 0 0 var(--spacing_8); color: var(--text_label_danger_secondary_default); font-size: var(--size_12); } + + /* Worktree list (roadmap E 区 工作树隔离). */ + .webui-worktree-section-title { margin: var(--spacing_20) 0 var(--spacing_8); color: var(--text_default_primary); font-size: var(--size_14); } + .webui-worktree-branches { display: flex; flex-direction: column; gap: var(--spacing_8); margin: 0; padding: 0; list-style: none; } + .webui-worktree-group { padding: 10px 12px; border: 0.5px solid var(--border_default); border-radius: 10px; } + .webui-worktree-heading { display: flex; align-items: center; gap: var(--spacing_8); } + .webui-worktree-name { display: inline-flex; min-width: 0; flex: 1; align-items: center; gap: 6px; overflow: hidden; color: var(--text_default_primary); font-size: var(--size_14); } + .webui-worktree-kind { flex: none; color: var(--text_default_tertiary); font-size: var(--size_12); } + .webui-worktree-path { margin: 4px 0 0; overflow: hidden; color: var(--text_default_tertiary); font-size: var(--size_12); text-overflow: ellipsis; white-space: nowrap; } + .webui-worktree-sessions { display: flex; flex-direction: column; gap: 2px; margin: var(--spacing_8) 0 0; padding: 0; list-style: none; } + .webui-worktree-session { display: flex; min-width: 0; align-items: center; gap: var(--spacing_8); font-size: var(--size_12); } + .webui-worktree-session-link { min-width: 0; flex: 1; overflow: hidden; color: var(--text_default_secondary); text-decoration: none; text-overflow: ellipsis; white-space: nowrap; } + .webui-worktree-session-link:hover { color: var(--text_default_primary); text-decoration: underline; } + .webui-worktree-forked { flex: none; color: var(--text_default_tertiary); } .webui-archived-page { width: 100%; max-width: 760px; margin: 0 auto; padding: 0 var(--spacing_20) var(--spacing_32); } .webui-archived-filters { display: grid; grid-template-columns: minmax(0, 1fr); margin-bottom: var(--spacing_24); } .webui-archived-search { display: flex; min-width: 0; height: 38px; align-items: center; gap: var(--spacing_8); border: 1px solid var(--border_default); border-radius: 12px; padding: 0 12px; background: var(--bg_default_primary); color: var(--text_default_secondary); } diff --git a/packages/webui/test/unit/review-panel.test.tsx b/packages/webui/test/unit/review-panel.test.tsx new file mode 100644 index 000000000..b1f187d54 --- /dev/null +++ b/packages/webui/test/unit/review-panel.test.tsx @@ -0,0 +1,226 @@ +import { createElement } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +import { WebuiReviewPanel } from "../../src/client/components/SettingsModal.js"; +import { + initialWebuiReviewState, + reduceWebuiReviewState, + selectWebuiReviewVisibleFiles, + isWebuiReviewFiltering, + type WebuiReviewFile, +} from "../../src/client/projection/review-state.js"; + +/* Why this file asserts on markup rather than on clicks: + * + * The webui suite runs `environment: "node"` — there is no jsdom, and + * `renderToStaticMarkup` renders once without running effects. So the + * decisions that a click would reveal (does expanding load a diff, does a line + * carry a jump target, does a filter narrow the list) are all made upstream in + * `review-state.ts` and reach the presentational half as plain values. This + * file's job is to prove the panel renders those values faithfully and does + * not invent or drop a jump target on the way to the DOM — the negative + * injections recorded in the PR body target exactly that boundary. + */ + +const files: readonly WebuiReviewFile[] = [ + { fileId: "f1", path: "src/one.ts", status: "modified", type: "text", additions: 2, deletions: 1 }, + { fileId: "f2", path: "assets/logo.png", status: "modified", type: "binary", additions: 0, deletions: 0 }, +]; + +const ready = reduceWebuiReviewState(initialWebuiReviewState, { + type: "summary-loaded", + reviewSnapshotId: "snap-1", + files, + totals: { files: 2, additions: 2, deletions: 1 }, +}); + +const panel = (state = ready, props: Record = {}): string => + renderToStaticMarkup( + createElement(WebuiReviewPanel, { + state, + visible: selectWebuiReviewVisibleFiles(state), + filtering: isWebuiReviewFiltering(state), + onOpenFileLine: () => undefined, + ...props, + } as never), + ); + +describe("code review page states", () => { + it("marks every state it can be in, so a test can tell them apart", () => { + expect(panel(ready)).toContain('data-webui-review-state="ready"'); + expect(panel(reduceWebuiReviewState(initialWebuiReviewState, { type: "load-begun" }))).toContain( + 'data-webui-review-state="loading"', + ); + expect( + panel(reduceWebuiReviewState(initialWebuiReviewState, { type: "unavailable", reason: "" })), + ).toContain('data-webui-review-state="unavailable"'); + expect( + panel(reduceWebuiReviewState(initialWebuiReviewState, { type: "load-failed", reason: "boom" })), + ).toContain('data-webui-review-state="error"'); + }); + + /* A workspace with nothing to review is a normal state. Rendering it as a + * failure would put a red banner on every clean repository. */ + it("does not present an absent change set as an error", () => { + const unavailable = panel( + reduceWebuiReviewState(initialWebuiReviewState, { type: "unavailable", reason: "" }), + { empty: "当前工作区没有待审查的变更。" }, + ); + expect(unavailable).toContain('data-webui-review-state="unavailable"'); + expect(unavailable).not.toContain("webui-review-error"); + expect(unavailable).toContain("当前工作区没有待审查的变更。"); + }); + + it("does show a failed load as an error, with the reason", () => { + const failed = panel(reduceWebuiReviewState(initialWebuiReviewState, { type: "load-failed", reason: "仓库读取失败" })); + expect(failed).toContain('data-webui-review-state="error"'); + expect(failed).toContain('role="alert"'); + expect(failed).toContain("仓库读取失败"); + }); + + it("shows totals and one row per file when ready", () => { + const markup = panel(); + expect(markup).toContain('data-testid="review-totals"'); + expect(markup).toContain("2 个文件"); + expect(markup).toContain("src/one.ts"); + expect(markup).toContain("assets/logo.png"); + expect(markup).toContain('data-webui-review-snapshot="snap-1"'); + }); +}); + +describe("a review line is a real jump target, and only where one exists", () => { + const diff = [ + "diff --git a/src/one.ts b/src/one.ts", + "--- a/src/one.ts", + "+++ b/src/one.ts", + "@@ -10,2 +10,2 @@", + " const keep = 1;", + "-const gone = 2;", + "+const here = 3;", + ].join("\n"); + + const expanded = reduceWebuiReviewState( + reduceWebuiReviewState(ready, { type: "diffs-loaded", diffs: { f1: diff } }), + { type: "toggle-file", fileId: "f1" }, + ); + + it("renders the diff only for an expanded file", () => { + const collapsed = panel(); + expect(collapsed).not.toContain('data-testid="review-lines"'); + + const opened = panel(expanded); + expect(opened).toContain('data-testid="review-lines"'); + expect(opened).toContain("const here = 3;"); + }); + + it("gives the surviving line a jump control carrying its new-side number", () => { + const markup = panel(expanded); + expect(markup).toContain('data-webui-review-jump-line="10"'); + expect(markup).toContain('data-webui-review-jump-line="11"'); + }); + + /* The whole point of the row: a deleted line does not exist in the file the + * editor will open, so it must not be offered as a destination. */ + it("does not offer a jump on a deleted line", () => { + const markup = panel(expanded); + const deletionLine = markup.split('data-webui-review-line-kind="deletion"')[1]?.split("")[0] ?? ""; + expect(deletionLine).toContain("const gone = 2;"); + expect(deletionLine).not.toContain("review-line-jump"); + }); + + it("does not offer a jump on the hunk header", () => { + const markup = panel(expanded); + const hunk = markup.split('data-webui-review-line-kind="hunk"')[1]?.split("")[0] ?? ""; + expect(hunk).toContain("@@"); + expect(hunk).not.toContain("review-line-jump"); + }); + + /* Without a jump handler there is nothing for a click to do, so the control + * must not be rendered at all rather than rendered dead. */ + it("renders no jump control when there is nothing to call", () => { + const markup = panel(expanded, { onOpenFileLine: undefined }); + expect(markup).toContain('data-testid="review-lines"'); + expect(markup).not.toContain("review-line-jump"); + }); +}); + +describe("per-file diff outcomes are told apart", () => { + it("shows a file failure instead of an empty diff", () => { + const state = reduceWebuiReviewState( + reduceWebuiReviewState(ready, { type: "diffs-loaded", diffs: {}, errors: { f1: "补丁过大" } }), + { type: "toggle-file", fileId: "f1" }, + ); + const markup = panel(state); + expect(markup).toContain('data-testid="review-file-error"'); + expect(markup).toContain("补丁过大"); + expect(markup).not.toContain('data-testid="review-lines"'); + }); + + it("says it is loading rather than showing an empty diff body", () => { + const state = reduceWebuiReviewState( + reduceWebuiReviewState(ready, { type: "diffs-begun", fileIds: ["f1"] }), + { type: "toggle-file", fileId: "f1" }, + ); + const markup = panel(state); + expect(markup).toContain("正在读取这个文件的补丁…"); + expect(markup).not.toContain('data-testid="review-lines"'); + }); +}); + +describe("the search box reflects filter state", () => { + it("offers a clear control only while filtering", () => { + expect(panel()).not.toContain('data-testid="review-search-clear"'); + + const filtering = reduceWebuiReviewState(ready, { type: "query-changed", query: "one" }); + expect(panel(filtering)).toContain('data-testid="review-search-clear"'); + }); + + it("keeps the query visible in the box", () => { + const filtering = reduceWebuiReviewState(ready, { type: "query-changed", query: "one" }); + expect(panel(filtering)).toContain('value="one"'); + }); +}); + +/* Why these two are source assertions and not render assertions: + * + * `SettingsModal` opens on the `desktop` tab and only moves to another one + * from a click handler, so no `renderToStaticMarkup` call can ever reach the + * `coding` or `worktree` branch — the panels are asserted directly above and + * the wiring between the tab and the panel is not. Negative injection proved + * the gap is real: replacing `{active === "coding" ? { + const source = readFileSync( + fileURLToPath(new URL("../../src/client/components/SettingsModal.tsx", import.meta.url)), + "utf8", + ); + + it("routes the coding tab to the review page", () => { + expect(source).toContain('{active === "coding" ? { + expect(source).toContain('{active === "worktree" ? { + const fallback = source.split('webui-settings-empty-panel')[0]?.split('{active === "archived"')?.at(-1) ?? ""; + expect(fallback).toContain('active !== "coding"'); + expect(fallback).toContain('active !== "worktree"'); + }); +}); diff --git a/packages/webui/test/unit/review-state.test.ts b/packages/webui/test/unit/review-state.test.ts new file mode 100644 index 000000000..00760d0d0 --- /dev/null +++ b/packages/webui/test/unit/review-state.test.ts @@ -0,0 +1,256 @@ +import { describe, expect, it } from "vitest"; + +import { + initialWebuiReviewState, + isWebuiReviewFiltering, + projectWebuiReviewLines, + reduceWebuiReviewState, + selectWebuiReviewVisibleFiles, + webuiReviewLineTarget, + type WebuiReviewFile, +} from "../../src/client/projection/review-state.js"; + +const files: readonly WebuiReviewFile[] = [ + { fileId: "f1", path: "src/one.ts", status: "modified", type: "text", additions: 3, deletions: 1 }, + { fileId: "f2", path: "src/two.ts", status: "added", type: "text", additions: 10, deletions: 0 }, + { fileId: "f3", path: "assets/logo.png", status: "modified", type: "binary", additions: 0, deletions: 0 }, +]; + +const loaded = reduceWebuiReviewState(initialWebuiReviewState, { + type: "summary-loaded", + reviewSnapshotId: "snap-1", + files, + totals: { files: 3, additions: 13, deletions: 1 }, +}); + +describe("review page load lifecycle", () => { + it("carries the snapshot, file list and totals through", () => { + expect(loaded.status).toBe("ready"); + expect(loaded.reviewSnapshotId).toBe("snap-1"); + expect(loaded.files).toHaveLength(3); + expect(loaded.totals).toEqual({ files: 3, additions: 13, deletions: 1 }); + }); + + /* "This workspace has no review snapshot" is a normal state, not a failure. + * Reporting it as an error would show a red banner for a fresh workspace. */ + it("keeps an absent snapshot distinct from a failed load", () => { + const unavailable = reduceWebuiReviewState(loaded, { type: "unavailable", reason: "没有待审查的变更" }); + expect(unavailable.status).toBe("unavailable"); + expect(unavailable.error).toBeUndefined(); + + const failed = reduceWebuiReviewState(loaded, { type: "load-failed", reason: "仓库读取失败" }); + expect(failed.status).toBe("error"); + expect(failed.error).toBe("仓库读取失败"); + }); + + /* Diffs are keyed by fileId, and fileIds belong to a snapshot. Showing the + * previous snapshot's diffs under a new one would attribute another + * snapshot's changes to this change set. */ + it("drops the previous snapshot's diffs when a reload starts", () => { + const withDiff = reduceWebuiReviewState(loaded, { + type: "diffs-loaded", + diffs: { f1: "@@ -1 +1 @@\n-old\n+new" }, + }); + expect(withDiff.diffs.f1).toContain("+new"); + + const reloading = reduceWebuiReviewState(withDiff, { type: "load-begun" }); + expect(reloading.diffs).toEqual({}); + expect(reloading.diffErrors).toEqual({}); + expect(reloading.status).toBe("loading"); + }); + + it("settles only the files that were actually in flight", () => { + const loading = reduceWebuiReviewState(loaded, { type: "diffs-begun", fileIds: ["f1", "f2"] }); + expect(loading.loadingFileIds).toEqual(["f1", "f2"]); + + const settled = reduceWebuiReviewState(loading, { + type: "diffs-loaded", + diffs: { f1: "diff f1" }, + errors: { f2: "补丁过大" }, + }); + expect(settled.loadingFileIds).toEqual([]); + expect(settled.diffs.f1).toBe("diff f1"); + expect(settled.diffErrors.f2).toBe("补丁过大"); + }); + + it("drops a stale in-flight marker once its diff lands", () => { + const loading = reduceWebuiReviewState(loaded, { type: "diffs-begun", fileIds: ["f1", "f2"] }); + const first = reduceWebuiReviewState(loading, { type: "diffs-loaded", diffs: { f1: "a" } }); + expect(first.loadingFileIds).toEqual(["f2"]); + + const second = reduceWebuiReviewState(first, { type: "diffs-loaded", diffs: { f2: "b" } }); + expect(second.loadingFileIds).toEqual([]); + expect(Object.keys(second.diffs).sort()).toEqual(["f1", "f2"]); + }); +}); + +describe("review search filter", () => { + it("shows every file before a search has run for the current query", () => { + const searching = reduceWebuiReviewState(loaded, { type: "query-changed", query: "one" }); + expect(isWebuiReviewFiltering(searching)).toBe(true); + expect(selectWebuiReviewVisibleFiles(searching)).toHaveLength(3); + }); + + it("narrows to the matched files once the search settles", () => { + let state = reduceWebuiReviewState(loaded, { type: "query-changed", query: "one" }); + state = reduceWebuiReviewState(state, { type: "search-begun" }); + expect(state.searchPending).toBe(true); + state = reduceWebuiReviewState(state, { type: "search-settled", fileIds: ["f1"] }); + expect(selectWebuiReviewVisibleFiles(state).map((file) => file.fileId)).toEqual(["f1"]); + }); + + /* A search that matched nothing is an empty list. It must not silently fall + * back to the unfiltered list, which is what an empty match set used to do. */ + it("shows an empty list when the query matched nothing", () => { + let state = reduceWebuiReviewState(loaded, { type: "query-changed", query: "nothing-matches" }); + state = reduceWebuiReviewState(state, { type: "search-settled", fileIds: [] }); + expect(selectWebuiReviewVisibleFiles(state)).toEqual([]); + }); + + /* Editing the query must invalidate the previous matches, or the list keeps + * filtering by a query that is no longer in the box. */ + it("drops previous matches when the query is edited", () => { + let state = reduceWebuiReviewState(loaded, { type: "query-changed", query: "one" }); + state = reduceWebuiReviewState(state, { type: "search-settled", fileIds: ["f1"] }); + state = reduceWebuiReviewState(state, { type: "query-changed", query: "two" }); + expect(state.matchedFileIds).toEqual([]); + expect(selectWebuiReviewVisibleFiles(state)).toHaveLength(3); + }); + + it("restores the full list when filters are cleared", () => { + let state = reduceWebuiReviewState(loaded, { type: "query-changed", query: "one" }); + state = reduceWebuiReviewState(state, { type: "search-settled", fileIds: ["f1"] }); + state = reduceWebuiReviewState(state, { type: "clear-filters" }); + expect(state.query).toBe(""); + expect(isWebuiReviewFiltering(state)).toBe(false); + expect(selectWebuiReviewVisibleFiles(state)).toHaveLength(3); + }); + + it("treats a whitespace-only query as no filter at all", () => { + const state = reduceWebuiReviewState(loaded, { type: "query-changed", query: " " }); + expect(isWebuiReviewFiltering(state)).toBe(false); + expect(selectWebuiReviewVisibleFiles(state)).toHaveLength(3); + }); +}); + +describe("unified diff projection and the line target it yields", () => { + const diff = [ + "diff --git a/src/one.ts b/src/one.ts", + "index 123..456 100644", + "--- a/src/one.ts", + "+++ b/src/one.ts", + "@@ -10,4 +10,5 @@ export function thing() {", + " const before = 1;", + "-const removed = 2;", + "+const added = 3;", + "+const alsoAdded = 4;", + " const after = 5;", + ].join("\n"); + + it("numbers both sides from the hunk header", () => { + const lines = projectWebuiReviewLines(diff); + const context = lines.find((line) => line.text === "const before = 1;"); + // Header says -10,4 +10,5, so the first context line is 10 on both sides. + expect(context?.newLine).toBe(10); + expect(context?.oldLine).toBe(10); + }); + + it("numbers a deletion on the old side only and an addition on the new side only", () => { + const lines = projectWebuiReviewLines(diff); + const deletion = lines.find((line) => line.text === "const removed = 2;"); + expect(deletion?.kind).toBe("deletion"); + expect(deletion?.oldLine).toBe(11); + expect(deletion?.newLine).toBeUndefined(); + + const addition = lines.find((line) => line.text === "const added = 3;"); + expect(addition?.kind).toBe("addition"); + expect(addition?.newLine).toBe(11); + expect(addition?.oldLine).toBeUndefined(); + }); + + it("advances the new side past a deletion and both sides past a context line", () => { + const lines = projectWebuiReviewLines(diff); + const second = lines.find((line) => line.text === "const alsoAdded = 4;"); + expect(second?.newLine).toBe(12); + const after = lines.find((line) => line.text === "const after = 5;"); + expect(after?.newLine).toBe(13); + expect(after?.oldLine).toBe(12); + }); + + /* This is the whole point of the row: a deleted line does not exist in the + * file the editor will open, so it must not yield a jump target. */ + it("gives a deletion no editor target and gives additions and context one", () => { + const lines = projectWebuiReviewLines(diff); + const deletion = lines.find((line) => line.text === "const removed = 2;"); + const addition = lines.find((line) => line.text === "const added = 3;"); + const context = lines.find((line) => line.text === "const before = 1;"); + const header = lines.find((line) => line.kind === "hunk"); + expect(webuiReviewLineTarget(deletion!)).toBeUndefined(); + expect(webuiReviewLineTarget(addition!)).toBe(11); + expect(webuiReviewLineTarget(context!)).toBe(10); + expect(webuiReviewLineTarget(header!)).toBeUndefined(); + }); + + it("does not let a no-newline marker consume a line number", () => { + const withMarker = ["@@ -1,2 +1,2 @@", " keep", "-drop", "\\ No newline at end of file", "+added"].join("\n"); + const lines = projectWebuiReviewLines(withMarker); + const added = lines.find((line) => line.text === "added"); + expect(added?.newLine).toBe(2); + const marker = lines.find((line) => line.text.startsWith("\\ No newline")); + expect(marker?.kind).toBe("meta"); + }); + + it("treats content before the first hunk header as metadata, not as lines", () => { + const lines = projectWebuiReviewLines(diff); + const meta = lines.filter((line) => line.kind === "meta"); + expect(meta.map((line) => line.text)).toEqual([ + "diff --git a/src/one.ts b/src/one.ts", + "index 123..456 100644", + "--- a/src/one.ts", + "+++ b/src/one.ts", + ]); + expect(meta.every((line) => webuiReviewLineTarget(line) === undefined)).toBe(true); + }); + + it("survives a hunk header without line numbers", () => { + const lines = projectWebuiReviewLines("@@ -0,0 +1,2 @@\n+one\n+two"); + expect(lines.map((line) => line.newLine)).toEqual([undefined, 1, 2]); + }); + + it("handles CRLF diffs without leaking carriage returns into the line text", () => { + const lines = projectWebuiReviewLines("@@ -1,1 +1,2 @@\r\n keep\r\n+added\r\n"); + const added = lines.find((line) => line.text === "added"); + expect(added).toBeDefined(); + expect(lines.every((line) => !line.text.includes("\r"))).toBe(true); + }); + + it("handles an empty diff", () => { + expect(projectWebuiReviewLines("")).toEqual([{ kind: "meta", text: "" }]); + }); + + /* `webuiReviewLineTarget` reads this invariant instead of re-checking kinds: + * a guard there was untestable, because the projection already guaranteed + * the result. Asserting the invariant here is what makes the jump behaviour + * depend on a tested contract rather than on an untestable line. */ + it("gives a new-side line number only to lines that exist in the new file", () => { + const everyDiff = [ + "diff --git a/x b/x", + "--- a/x", + "+++ b/x", + "@@ -1,3 +1,3 @@", + " keep", + "-gone", + "\\ No newline at end of file", + "+here", + ].join("\n"); + for (const line of projectWebuiReviewLines(everyDiff)) { + if (line.kind === "addition" || line.kind === "context") { + expect(line.newLine, `${line.kind} should carry a new-side number`).toBeTypeOf("number"); + expect(webuiReviewLineTarget(line)).toBe(line.newLine); + } else { + expect(line.newLine, `${line.kind} must not carry a new-side number`).toBeUndefined(); + expect(webuiReviewLineTarget(line)).toBeUndefined(); + } + } + }); +}); diff --git a/packages/webui/test/unit/settings-modal.test.tsx b/packages/webui/test/unit/settings-modal.test.tsx index 619c153ad..38d426c58 100644 --- a/packages/webui/test/unit/settings-modal.test.tsx +++ b/packages/webui/test/unit/settings-modal.test.tsx @@ -90,14 +90,19 @@ describe("desktop settings registry", () => { }); describe("account tab gating", () => { - it("leaves the account tab clickable while the unimplemented tabs stay disabled", () => { - // The account panel is fully implemented (email row, sign-out button, - // sign-out error region), so it must not carry the `disabled` gate the - // not-yet-built panels still need. - const account = DESKTOP_SETTINGS_TABS.find((tab) => tab.key === "account"); - expect(account?.disabled).toBeUndefined(); - for (const key of ["voice", "shortcuts", "custom-instructions", "connection", "coding", "worktree"]) + it("leaves the implemented tabs clickable while the unimplemented tabs stay disabled", () => { + // A fully implemented panel must not carry the `disabled` gate the + // not-yet-built panels still need. `account` (email row, sign-out button, + // sign-out error region), `coding` (workspace review) and `worktree` + // (parallel experiment branches) are all built now, so all three sit in + // the ungated group. The rest have no content behind them and clicking + // one would land on an empty pane, so they must stay disabled. + for (const key of ["account", "coding", "worktree"]) { + expect(DESKTOP_SETTINGS_TABS.find((tab) => tab.key === key)?.disabled).toBeUndefined(); + } + for (const key of ["voice", "shortcuts", "custom-instructions", "connection"]) { expect(DESKTOP_SETTINGS_TABS.find((tab) => tab.key === key)?.disabled).toBe(true); + } }); it("keeps the account tab reachable through the settings search", () => { diff --git a/packages/webui/test/unit/worktree-panel.test.tsx b/packages/webui/test/unit/worktree-panel.test.tsx new file mode 100644 index 000000000..8e3634e09 --- /dev/null +++ b/packages/webui/test/unit/worktree-panel.test.tsx @@ -0,0 +1,103 @@ +import { createElement } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import { describe, expect, it } from "vitest"; + +import { WebuiWorktreePanel } from "../../src/client/components/SettingsModal.js"; +import { + groupWebuiWorktreeWorkspaces, + selectWebuiWorktreeWorkspaces, + type WebuiWorktreeSourceSession, +} from "../../src/client/projection/worktree-state.js"; + +/* `renderToStaticMarkup` runs the first render and never runs effects, so the + * fetching half of this page cannot be asserted here. What this file proves is + * that the panel renders the grouping it is handed faithfully — in particular + * that a page with no worktrees says so instead of showing an empty shell, and + * that a branch's sessions are reachable by the same `#session=` link the rail + * uses, so there is one navigation path rather than two. */ + +const main = "C:\\repos\\my-app"; +const branchA = "C:\\repos\\my-app\\.worktrees\\feature-a"; +const branchB = "C:\\repos\\my-app\\.worktrees\\feature-b"; + +const session = ( + sessionId: string, + updatedAt: number, + extra: Partial = {}, +): WebuiWorktreeSourceSession => ({ sessionId, updatedAt, ...extra }); + +const panel = (sessions: readonly WebuiWorktreeSourceSession[], props: Record = {}): string => { + const workspaces = groupWebuiWorktreeWorkspaces(sessions); + return renderToStaticMarkup( + createElement(WebuiWorktreePanel, { + workspaces, + worktrees: selectWebuiWorktreeWorkspaces(workspaces), + ...props, + } as never), + ); +}; + +describe("worktree page", () => { + it("says so plainly when there is no worktree yet", () => { + const markup = panel([session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true })]); + expect(markup).toContain('data-webui-worktree-state="empty"'); + expect(markup).toContain('data-testid="worktree-empty"'); + expect(markup).toContain("复制到新工作树"); + }); + + /* The point of the row: a project with parallel experiment branches should + * be able to see them. A page that renders only the primary checkout would + * be indistinguishable from the empty state. */ + it("lists every worktree alongside the primary checkout", () => { + const markup = panel([ + session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true, title: "主线" }), + session("s2", 300, { workspaceDir: branchA, title: "实验 A", parentSessionId: "s1" }), + session("s4", 250, { workspaceDir: branchA, title: "实验 A 续", parentSessionId: "s1" }), + session("s3", 200, { workspaceDir: branchB, title: "实验 B", parentSessionId: "s1" }), + ]); + expect(markup).toContain('data-webui-worktree-state="ready"'); + expect(markup).toContain('data-testid="worktree-primary"'); + expect(markup).toContain("主线"); + expect(markup).toContain("实验 A"); + expect(markup).toContain("实验 A 续"); + expect(markup).toContain("实验 B"); + expect(markup).toContain("主检出"); + // feature-a holds two sessions and feature-b one, so the per-branch count + // has to differ between them rather than repeat a single number. + expect(markup).toContain("2 个会话"); + expect(markup).toContain("1 个会话"); + }); + + it("links a branch session through the same hash route the rail uses", () => { + const markup = panel([ + session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true }), + session("s2", 300, { workspaceDir: branchA, title: "实验 A" }), + ]); + expect(markup).toContain('href="#session=s2"'); + expect(markup).toContain('data-webui-worktree-session="s2"'); + }); + + it("marks a session forked from another as derived", () => { + const markup = panel([ + session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true }), + session("s2", 300, { workspaceDir: branchA, parentSessionId: "s1" }), + ]); + expect(markup).toContain("派生"); + expect(markup).toContain("派生自 s1"); + }); + + it("reports a failed load as an error", () => { + const markup = panel([], { error: "会话列表读取失败" }); + expect(markup).toContain('data-webui-worktree-state="error"'); + expect(markup).toContain('role="alert"'); + expect(markup).toContain("会话列表读取失败"); + }); + + it("shows the full checkout path so two branches of the same name are distinguishable", () => { + const markup = panel([ + session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true }), + session("s2", 300, { workspaceDir: branchA }), + ]); + expect(markup).toContain(branchA); + }); +}); diff --git a/packages/webui/test/unit/worktree-state.test.ts b/packages/webui/test/unit/worktree-state.test.ts new file mode 100644 index 000000000..1037b5d3e --- /dev/null +++ b/packages/webui/test/unit/worktree-state.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, it } from "vitest"; + +import { + groupWebuiWorktreeWorkspaces, + normalizeWebuiWorkspaceDir, + selectWebuiPrimaryWorkspace, + selectWebuiWorktreeWorkspaces, + webuiWorkspaceName, + type WebuiWorktreeSourceSession, +} from "../../src/client/projection/worktree-state.js"; + +const session = ( + sessionId: string, + updatedAt: number, + extra: Partial = {}, +): WebuiWorktreeSourceSession => ({ sessionId, updatedAt, ...extra }); + +describe("workspace path normalization", () => { + /* A fork round-trips the path through the runtime, and Windows and POSIX + * spell the same checkout differently. One checkout reported as two + * worktrees is a wrong list, not a cosmetic one. */ + it("treats separator and trailing-slash variants as one checkout", () => { + expect(normalizeWebuiWorkspaceDir("C:\\repo")).toBe("c:/repo"); + expect(normalizeWebuiWorkspaceDir("C:/repo")).toBe("c:/repo"); + expect(normalizeWebuiWorkspaceDir("C:/repo/")).toBe("c:/repo"); + expect(normalizeWebuiWorkspaceDir(" C:\\repo\\ ")).toBe("c:/repo"); + }); + + it("names a checkout from its last path segment", () => { + expect(webuiWorkspaceName("C:\\repos\\my-app")).toBe("my-app"); + expect(webuiWorkspaceName("/home/dev/my-app/")).toBe("my-app"); + expect(webuiWorkspaceName("")).toBe(""); + }); +}); + +describe("grouping sessions into checkouts", () => { + const main = "C:\\repos\\my-app"; + const worktreeA = "C:\\repos\\my-app\\.worktrees\\feature-a"; + const worktreeB = "C:\\repos\\my-app\\.worktrees\\feature-b"; + + it("separates the primary checkout from its worktrees", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true }), + session("s2", 300, { workspaceDir: worktreeA, parentSessionId: "s1" }), + session("s3", 200, { workspaceDir: worktreeB, parentSessionId: "s1" }), + ]); + expect(groups).toHaveLength(3); + expect(groups[0]?.isPrimary).toBe(true); + expect(selectWebuiPrimaryWorkspace(groups)?.workspaceDir).toBe(main); + expect(selectWebuiWorktreeWorkspaces(groups).map((group) => group.name)).toEqual(["feature-a", "feature-b"]); + }); + + it("orders worktrees by most recently touched, primary first", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true }), + session("s2", 300, { workspaceDir: worktreeA }), + session("s3", 200, { workspaceDir: worktreeB }), + ]); + expect(groups.map((group) => group.workspaceDir)).toEqual([main, worktreeA, worktreeB]); + // The primary sorts first even though it is the oldest of the three. + expect(groups[0]?.updatedAt).toBe(100); + expect(groups[1]?.updatedAt).toBe(300); + }); + + it("keeps a checkout primary when a later session omits the flag", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s1", 200, { workspaceDir: main, isDefaultWorkspace: true }), + session("s2", 100, { workspaceDir: main }), + ]); + expect(groups).toHaveLength(1); + expect(groups[0]?.isPrimary).toBe(true); + expect(groups[0]?.sessions).toHaveLength(2); + }); + + it("merges sessions that reach the same checkout by different spellings", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s1", 200, { workspaceDir: "C:\\repos\\my-app", isDefaultWorkspace: true }), + session("s2", 300, { workspaceDir: "c:/repos/my-app/" }), + ]); + expect(groups).toHaveLength(1); + expect(groups[0]?.sessions.map((entry) => entry.sessionId).sort()).toEqual(["s1", "s2"]); + expect(groups[0]?.updatedAt).toBe(300); + }); + + it("sorts the sessions inside a checkout by most recently touched", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("old", 100, { workspaceDir: worktreeA }), + session("new", 500, { workspaceDir: worktreeA }), + session("mid", 300, { workspaceDir: worktreeA }), + ]); + expect(groups[0]?.sessions.map((entry) => entry.sessionId)).toEqual(["new", "mid", "old"]); + }); + + /* An archived session belongs to the archived page. Counting it here would + * make an abandoned worktree look like a live experiment branch. */ + it("drops archived sessions and the worktrees that were only archived", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s1", 100, { workspaceDir: main, isDefaultWorkspace: true }), + session("s2", 300, { workspaceDir: worktreeA, archived: true }), + ]); + expect(groups).toHaveLength(1); + expect(selectWebuiWorktreeWorkspaces(groups)).toEqual([]); + }); + + /* A session with no checkout has no worktree to belong to; a synthetic + * bucket for it would show up as a phantom branch. */ + it("drops sessions with no workspace", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s1", 100), + session("s2", 200, { workspaceDir: " " }), + session("s3", 300, { workspaceDir: main, isDefaultWorkspace: true }), + ]); + expect(groups).toHaveLength(1); + expect(groups[0]?.sessions.map((entry) => entry.sessionId)).toEqual(["s3"]); + }); + + it("falls back to the session id when a title is blank", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s1", 100, { workspaceDir: main, title: " " }), + ]); + expect(groups[0]?.sessions[0]?.title).toBe("s1"); + }); + + it("carries the fork parent so a branch can be traced to its origin", () => { + const groups = groupWebuiWorktreeWorkspaces([ + session("s2", 100, { workspaceDir: worktreeA, parentSessionId: "s1" }), + session("s3", 100, { workspaceDir: worktreeB }), + ]); + expect(groups[0]?.sessions[0]?.parentSessionId).toBe("s1"); + expect(groups[1]?.sessions[0]?.parentSessionId).toBeUndefined(); + }); + + it("orders equal timestamps deterministically by path", () => { + const first = groupWebuiWorktreeWorkspaces([ + session("s1", 100, { workspaceDir: "C:\\b" }), + session("s2", 100, { workspaceDir: "C:\\a" }), + ]); + const second = groupWebuiWorktreeWorkspaces([ + session("s2", 100, { workspaceDir: "C:\\a" }), + session("s1", 100, { workspaceDir: "C:\\b" }), + ]); + expect(first.map((group) => group.workspaceDir)).toEqual(second.map((group) => group.workspaceDir)); + expect(first.map((group) => group.name)).toEqual(["a", "b"]); + }); + + it("handles an empty session list", () => { + expect(groupWebuiWorktreeWorkspaces([])).toEqual([]); + expect(selectWebuiWorktreeWorkspaces([])).toEqual([]); + expect(selectWebuiPrimaryWorkspace([])).toBeUndefined(); + }); +}); diff --git a/release/public-source.json b/release/public-source.json index 808a61a34..b57a74e98 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3586,6 +3586,7 @@ "packages/webui/src/client/projection/outside-close.ts", "packages/webui/src/client/projection/plan-mode.ts", "packages/webui/src/client/projection/questionnaire-state.ts", + "packages/webui/src/client/projection/review-state.ts", "packages/webui/src/client/projection/shell-surface.ts", "packages/webui/src/client/projection/thinking-control.ts", "packages/webui/src/client/projection/token-plan-model.ts", @@ -3597,6 +3598,7 @@ "packages/webui/src/client/projection/usage-settings.ts", "packages/webui/src/client/projection/workspace-panel-state.ts", "packages/webui/src/client/projection/workspace-progress.ts", + "packages/webui/src/client/projection/worktree-state.ts", "packages/webui/src/client/rail-buckets.ts", "packages/webui/src/client/router.ts", "packages/webui/src/client/session-activity.ts", @@ -3692,6 +3694,8 @@ "packages/webui/test/unit/rail-context-menu.test.ts", "packages/webui/test/unit/rail-pin-affordance.test.ts", "packages/webui/test/unit/rail-star-favorites.test.ts", + "packages/webui/test/unit/review-panel.test.tsx", + "packages/webui/test/unit/review-state.test.ts", "packages/webui/test/unit/session-activity.test.ts", "packages/webui/test/unit/session-import.test.ts", "packages/webui/test/unit/session-rail-search.test.ts", @@ -3742,6 +3746,8 @@ "packages/webui/test/unit/webui-workspace-html-preview.test.tsx", "packages/webui/test/unit/webui-workspace-media-preview.test.tsx", "packages/webui/test/unit/workspace-panel-state.test.ts", + "packages/webui/test/unit/worktree-panel.test.tsx", + "packages/webui/test/unit/worktree-state.test.ts", "packages/webui/tsconfig.client.json", "packages/webui/tsconfig.json", "packages/webui/tsconfig.paths.json", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 4280767f0..38b16716e 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -259,6 +259,10 @@ "packages/webui/test/unit/thinking-control.test.tsx", "packages/webui/test/unit/webui-transcript-widgets-integration.test.ts", "packages/webui/test/unit/diff-mutation-error.test.tsx", + "packages/webui/test/unit/worktree-panel.test.tsx", + "packages/webui/test/unit/worktree-state.test.ts", + "packages/webui/test/unit/review-panel.test.tsx", + "packages/webui/test/unit/review-state.test.ts", "packages/webui/test/unit/webui-round3-acceptance.test.tsx", "packages/webui/test/unit/webui-w0-store-sequence.test.ts", "packages/webui/test/unit/webui-w0-projections.test.ts", diff --git a/test/webui-browser/settings-account-tab.spec.mjs b/test/webui-browser/settings-account-tab.spec.mjs index bf4e7336e..e7916d1fd 100644 --- a/test/webui-browser/settings-account-tab.spec.mjs +++ b/test/webui-browser/settings-account-tab.spec.mjs @@ -107,8 +107,13 @@ test("the account tab is enabled where the unfinished tabs are still disabled", // The other half, and the reason the assertion above is not vacuous: the nav // really does still ship disabled items, so "enabled" is a fact about this // tab rather than a property of every button in the sidebar. A nav that - // silently enabled 语音/快捷键/连接/代码审查 would pass the first two lines. - for (const key of ["voice", "shortcuts", "connection", "coding"]) { + // silently enabled 语音/快捷键/连接 would pass the first two lines. + // + // 代码审查 and 工作树 left this list when their pages were built — they are + // real panels now, not empty panes behind a clickable label. The three that + // remain have no content behind them, which is what the `disabled` gate is + // still for. + for (const key of ["voice", "shortcuts", "connection"]) { await expect(settingsNavItem(page, key)).toBeDisabled(); } // The app behind the modal is still mounted: the modal is a `document.body` @@ -117,6 +122,29 @@ test("the account tab is enabled where the unfinished tabs are still disabled", await expect(page.locator('[data-testid="webui-error-boundary"]')).toHaveCount(0); }); +test("the code-review and worktree tabs open their own panels instead of an empty pane", async ({ page }) => { + await openApp(page, "#session=A"); + await openSettings(page); + + // Same argument as the account tab above, and the reason the unit suite + // cannot stand in: `disabled: false` is a flag on the definition, while + // "clicking it lands on the page" is the feature. The empty-pane + // placeholder is the failure this has to rule out — a clickable label with + // nothing behind it is exactly the state these two tabs were in. + await expect(settingsNavItem(page, "coding")).toBeEnabled(); + await settingsNavItem(page, "coding").click(); + await expect(page.locator('[data-testid="settings-review-page"]')).toBeVisible(); + // One status marker, and it is the review page's own: with no workspace + // selected the page says so instead of rendering a bare heading. + await expect(page.locator("[data-webui-review-state]")).toHaveCount(1); + await expect(page.locator(".webui-settings-empty-panel")).toHaveCount(0); + + await expect(settingsNavItem(page, "worktree")).toBeEnabled(); + await settingsNavItem(page, "worktree").click(); + await expect(page.locator('[data-testid="settings-worktree-page"]')).toBeVisible(); + await expect(page.locator(".webui-settings-empty-panel")).toHaveCount(0); +}); + test("clicking the account tab shows the account panel", async ({ page }) => { await openApp(page, "#session=A"); await openSettings(page); From da7008830c8a9b243f375f3f5df1e2f976f4c525 Mon Sep 17 00:00:00 2001 From: antianqi <75944423+antianqi@users.noreply.github.com> Date: Mon, 5 Oct 2026 16:32:45 +0800 Subject: [PATCH 3/5] fix(webui): translate the runtime's refusal tokens into something a person can act on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the server half of roadmap E 区's Revert / Reapply row — by correcting what that half actually is. The roadmap records the defect as "服务端以 `plan-not-safe` 跳过工作区外文件 恢复" and asks for "UI 提示和服务端判据". Both parts of that are wrong against the current tree, and the difference matters for what gets built: 1. There is no partial skip. `applyLocalTurnDiffSnapshotMutation` walks `record.undo` and, the moment `safeCapturedPath` refuses one file, returns `{ success: false, reason: 'unsafe_path' }` — before writing anything. The operation is all-or-nothing. `applyGitPatch` is the same shape: `git apply` exits non-zero and nothing lands. So there is no skipped-file list to surface, and no partial-success state to represent. 2. The token is `unsafe_path`, not `plan-not-safe`; the string does not occur anywhere in the repository. 3. The server already reports a truthful, machine-readable reason, and it reaches the client intact: `mutateLocalTurnDiff` puts it in `body.error`, v2's `assertMutationSucceeded` throws an `AppError` carrying it, and the transport rejects. What was missing was not a server field — it was the last step, turning a token into a sentence. So this adds `describeWebuiDiffFailure`, which maps every reason the mutation path can produce to copy that says what happened and what to do: - `unsafe_path` — a captured file is outside the workspace, so nothing was touched. Worth naming precisely, because it is a safety refusal the user cannot work around by retrying. - `conflict` — the file changed after the turn. The token does not say *when*, and that is the part that tells the user to look before retrying. - `not_undoable` — the turn left no snapshot to undo. - the three literal messages `mutateLocalTurnDiff` returns. A reason outside the table is passed through unchanged, deliberately: `git apply` failures arrive as free-form stderr, and replacing a real message with a guess would be worse than showing it. This also let the resolver lose a branch. It used to read `error` off the revert result only, because `WebuiReapplyTurnDiffResult` was believed to have no such field — it does, and both result shapes carry one. The action-agnostic `result?.error` is now correct for both, and the "reapply errors are dropped" mutation can no longer be written at all. Validation - pnpm check:source exit 0 - pnpm typecheck:webui-full exit 0 - pnpm build:webui exit 0 - run-vitest-suite.mjs webui 9 failed | 1642 passed | 4 skipped - playwright test test/webui-browser/ 79 passed The 9 failures are the unchanged pre-existing Windows baseline. Test evidence diff-mutation-error.test.tsx gains 5 tests (22 total) covering the mapping, the passthrough, the no-reason cases, and that a thrown `unsafe_path` is translated the same way as a reported one — the two reach the resolver by different paths and used to be easy to translate in only one. Negative injection: 18 mutations, 0 survivors, implementation restored byte-for-byte. Four are new for this seam — showing the raw token, dropping the `unsafe_path` entry, discarding an unknown reason instead of passing it through, and skipping the trim before matching a code. Two entries in the injection script went stale and were reworked rather than counted: an earlier version of the harness scored a missing anchor as a survivor, which reports a script-maintenance problem as a test-quality problem and hides the real failures. A stale anchor and a surviving mutation are now reported separately, and the exit code depends only on the latter. --- .../webui/src/client/components/DiffCard.tsx | 34 ++++++++++++-- .../test/unit/diff-mutation-error.test.tsx | 44 +++++++++++++++++++ 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/packages/webui/src/client/components/DiffCard.tsx b/packages/webui/src/client/components/DiffCard.tsx index c10ad4671..8457f2577 100644 --- a/packages/webui/src/client/components/DiffCard.tsx +++ b/packages/webui/src/client/components/DiffCard.tsx @@ -74,6 +74,34 @@ function trimmedOrUndefined(value: string | undefined): string | undefined { return trimmed ? trimmed : undefined; } +/* What the runtime says when a revert or reapply does not apply, and what a + * person can do about it. + * + * The reasons are machine tokens, and the authoritative one is + * `applyLocalTurnDiffSnapshotMutation`'s `unsafe_path`: a captured file sits + * outside the workspace, so the whole operation is refused rather than + * partially applied. Surfacing the raw token tells the user nothing, and the + * other codes read the same way — `conflict` in particular does not say that + * the files changed *after* the turn, which is the part that matters. + * + * A reason not in this table is passed through unchanged. That is deliberate: + * `git apply` failures arrive as free-form stderr, and discarding them would + * replace a real message with a guess. */ +const WEBUI_DIFF_FAILURE_COPY: Readonly> = { + unsafe_path: "这轮改动里有文件不在工作区内，出于安全没有动它。", + conflict: "这轮改动之后文件又被修改过，撤销前请先确认当前内容。", + not_undoable: "这轮文件改动没有留下可撤销的快照。", + "Turn diff not found": "找不到这轮文件改动。", + "Only the latest turn diff can be changed": "只能撤销最近一轮的文件改动。", + "Turn diff is not undoable": "这轮文件改动没有可撤销的补丁。", +}; + +export function describeWebuiDiffFailure(reason: string | undefined): string | undefined { + const trimmed = trimmedOrUndefined(reason); + if (!trimmed) return undefined; + return WEBUI_DIFF_FAILURE_COPY[trimmed] ?? trimmed; +} + /** Decides what a revert/reapply round trip meant. * * The two operations nest the resulting view differently, and reading either @@ -95,12 +123,12 @@ export function resolveWebuiDiffMutation( ): WebuiDiffStateAction { if (thrown !== undefined) { const message = thrown instanceof Error ? thrown.message : undefined; - return { type: "mutation-failed", error: trimmedOrUndefined(message) }; + return { type: "mutation-failed", error: describeWebuiDiffFailure(message) }; } // `success` is required on reapply and optional on revert; an explicit // `false` is a refusal even if a view rode along beside it. if (result?.success === false) { - return { type: "mutation-failed", error: trimmedOrUndefined(result.error) }; + return { type: "mutation-failed", error: describeWebuiDiffFailure(result.error) }; } const nextView = action === "revert" @@ -109,7 +137,7 @@ export function resolveWebuiDiffMutation( if (nextView && (nextView.fileChanges ?? []).length > 0) { return { type: "mutation-succeeded", view: nextView }; } - return { type: "mutation-failed", error: trimmedOrUndefined(result?.error) }; + return { type: "mutation-failed", error: describeWebuiDiffFailure(result?.error) }; } export function buildWebuiDiffMutationRequest( diff --git a/packages/webui/test/unit/diff-mutation-error.test.tsx b/packages/webui/test/unit/diff-mutation-error.test.tsx index 44915a096..e6211003b 100644 --- a/packages/webui/test/unit/diff-mutation-error.test.tsx +++ b/packages/webui/test/unit/diff-mutation-error.test.tsx @@ -5,6 +5,7 @@ import { describe, expect, it } from "vitest"; import { WebuiDiffCard, buildWebuiDiffMutationRequest, + describeWebuiDiffFailure, initialWebuiDiffState, reduceWebuiDiffState, resolveWebuiDiffMutation, @@ -291,3 +292,46 @@ describe("resolveWebuiDiffMutation reads the transport result the way the wire m }); }); }); + +/* The runtime's reasons are machine tokens. `applyLocalTurnDiffSnapshotMutation` + * refuses the whole operation with `unsafe_path` when a captured file sits + * outside the workspace — it does not partially apply — and `conflict` when a + * file changed after the turn. Both reach the client verbatim through + * `assertMutationSucceeded`, which throws with `body.error` as the message. + * Without a translation the user reads the token. */ +describe("runtime reason codes become something a person can act on", () => { + it("translates every reason the mutation path can produce", () => { + expect(describeWebuiDiffFailure("unsafe_path")).toBe("这轮改动里有文件不在工作区内，出于安全没有动它。"); + expect(describeWebuiDiffFailure("conflict")).toBe("这轮改动之后文件又被修改过，撤销前请先确认当前内容。"); + expect(describeWebuiDiffFailure("not_undoable")).toBe("这轮文件改动没有留下可撤销的快照。"); + expect(describeWebuiDiffFailure("Turn diff not found")).toBe("找不到这轮文件改动。"); + expect(describeWebuiDiffFailure("Only the latest turn diff can be changed")).toBe("只能撤销最近一轮的文件改动。"); + expect(describeWebuiDiffFailure("Turn diff is not undoable")).toBe("这轮文件改动没有可撤销的补丁。"); + }); + + it("no longer shows a raw token as the user-facing reason", () => { + const action = resolveWebuiDiffMutation("revert", { success: false, error: "unsafe_path" }); + expect(action).toEqual({ type: "mutation-failed", error: "这轮改动里有文件不在工作区内，出于安全没有动它。" }); + expect(action.type === "mutation-failed" && action.error).not.toBe("unsafe_path"); + }); + + /* `git apply` failures arrive as free-form stderr. Passing them through keeps + * a real message; replacing them with a guess would be worse than useless. */ + it("passes an unknown reason through unchanged", () => { + expect(describeWebuiDiffFailure("error: patch failed: src/one.ts:3")).toBe("error: patch failed: src/one.ts:3"); + expect(describeWebuiDiffFailure(" unsafe_path ")).toBe("这轮改动里有文件不在工作区内，出于安全没有动它。"); + }); + + it("still reports no reason when there is none", () => { + expect(describeWebuiDiffFailure(undefined)).toBeUndefined(); + expect(describeWebuiDiffFailure("")).toBeUndefined(); + expect(describeWebuiDiffFailure(" ")).toBeUndefined(); + }); + + it("translates a thrown unsafe_path the same way as a reported one", () => { + expect(resolveWebuiDiffMutation("revert", undefined, new Error("unsafe_path"))).toEqual({ + type: "mutation-failed", + error: "这轮改动里有文件不在工作区内，出于安全没有动它。", + }); + }); +}); From 4702f01eada394a08fd8ceef8245cf0fb32a610d Mon Sep 17 00:00:00 2001 From: antianqi <75944423+antianqi@users.noreply.github.com> Date: Mon, 5 Oct 2026 17:49:48 +0800 Subject: [PATCH 4/5] feat(webui): let the code-review page pick the workspace it reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Roadmap E 区's review page refused to do anything until the selected session already carried a workspace, and said so: 先打开一个工作区，再来审查它的变更。 That names the problem and offers no way out of it. The route it implies — go select a different session — is not reachable from the page: the settings dialog is `aria-modal`, so the rail cannot be clicked while it is open. The user is parked in a state they cannot leave, which is the same shape of defect the two disabled tabs had before this branch built pages behind them. The gate itself is correct and stays: `workspaceDir` is the selected session's workspace, and it is empty whenever that session is not bound to a project, including the default workspace, which the rail groups under 「未选项目」 regardless of whether it carries a path (SessionRail.tsx:304). Reviewing needs a real checkout to ask about. What was missing is the way to supply one. `WebuiReviewWorkspacePicker` lists the checkouts and takes a click. It is presentational like `WebuiReviewPanel`, so the list is assertable without a DOM, and it reuses `groupWebuiWorktreeWorkspaces` rather than grouping again — one checkout spelled `C:\repo\x` by one session and `C:/repo/x` by another is one row, because the worktree page already got that right and the two surfaces should not disagree about what a checkout is. The choice is an override of the prop, not a replacement for it. Folding the pick into the prop would leave the page looking unchanged until the user went and changed session, which is the thing this is fixing. It is component state, so it resets when the dialog remounts — deliberate, since a stale "last reviewed workspace" would quietly shadow the session the user is looking at. Nothing changes for a page opened from a workspace-bound session: the prop wins, and the session list behind the picker is not even fetched. Validation - pnpm check:source exit 0 (4717 files) - pnpm typecheck:webui-full exit 0 - pnpm build:webui exit 0 - run-vitest-suite.mjs webui 9 failed | 1653 passed | 4 skipped - playwright test test/webui-browser/ 79 passed The 9 failures are the unchanged pre-existing Windows baseline, same names: 7 webui-boundary-check path assertions, 1 webui-design-tokens path assertion, 1 webui-service shutdown timeout. Test evidence review-panel.test.tsx gains 10 tests (27 total): six on the picker's markup and four on the wiring from a click to the review load. Red before the implementation: all 10 failed against a stub that rendered an empty div, and the wiring assertions named the missing pieces — no `loadSessions`, no `workspaceDir?.trim() || pickedWorkspaceDir`, no `effectiveWorkspaceDir` in the summary call. Negative injection: 12 mutations across the picker and the wiring, 0 survivors, implementation restored byte-for-byte. The first run found four real holes, which is the point of running it: - The "nothing to review" explanation was only asserted in the state where it is correct to show it. Deleting `workspaces.length === 0` from the condition left the suite green — the message could sit above a full list. There is now a test that it is absent when there is something to pick. - "shows the full path" asserted on the whole markup, and every option carries the path in `data-webui-workspace-dir` too, so swapping the displayed text for the basename still passed. It now reads the visible paragraph. - `loadSessions={loadSessions}` was asserted unscoped, and the worktree page is handed the same loader, so removing it from the review page alone left the assertion satisfied. It is now matched within the `` call. - The branch that renders the picker was never asserted at all. Replacing `if (!effectiveWorkspaceDir)` with `if (false)` passed everything, because the component was only ever tested in isolation and never reached from the page. The harness itself had a reporting defect carried over from the earlier runs: an all-green summary line does not match the `failed | passed` form, so the detail column read "all 0 still green" for a suite of 27. The verdict reads failedCount and was never affected; only the report was misleading. Fixed here, and a stale anchor stays STALE rather than counting as a survivor. Scope: the entry state of the E-area review page. The line jump, the search, the diff batching and the per-file outcomes are untouched. --- .../src/client/components/SettingsModal.tsx | 115 ++++++++++++++-- .../webui/test/unit/review-panel.test.tsx | 124 +++++++++++++++++- 2 files changed, 224 insertions(+), 15 deletions(-) diff --git a/packages/webui/src/client/components/SettingsModal.tsx b/packages/webui/src/client/components/SettingsModal.tsx index 8074891ef..3377d53af 100644 --- a/packages/webui/src/client/components/SettingsModal.tsx +++ b/packages/webui/src/client/components/SettingsModal.tsx @@ -181,7 +181,7 @@ export function SettingsModal({ open, onClose, dataDir, version, sessionId, work const visibleTabs = useMemo(() => filterSettingsTabs(query), [query]); if (!open) return null; const selected = models.find((model) => model.selected); const modelValue = selected ? `${selected.providerId}/${selected.modelId}/${selected.variant ?? ""}` : ""; const groups = SETTINGS_GROUPS.map((group) => ({ ...group, tabs: visibleTabs.filter((tab) => tab.group === group.key) })).filter((group) => group.tabs.length > 0); const label = DESKTOP_SETTINGS_TABS.find((tab) => tab.key === active)?.label; const changeModel = async (value: string) => { const model = models.find((candidate) => `${candidate.providerId}/${candidate.modelId}/${candidate.variant ?? ""}` === value); if (!model || !selectModel) return; await selectModel({ providerId: model.providerId, modelId: model.modelId, ...(model.variant ? { variant: model.variant } : {}), ...(sessionId ? { sessionId } : {}) }); }; const handleSignOut = async () => { if (!signOut) return; try { setSignOutError(undefined); await signOut(); onClose(); } catch (error) { setSignOutError(error instanceof Error ? error.message : String(error)); } }; const handleDeleteAllArchived = async () => { if (!deleteSession || !archived.length || !window.confirm("确定删除全部已归档任务吗？此操作无法撤销。")) return; const ids = archived.map((session) => session.sessionId); try { await Promise.all(ids.map((id) => deleteSession({ id }))); setArchived([]); } catch (error) { window.alert(`删除失败：${error instanceof Error ? error.message : String(error)}`); } }; - return
{ if (event.target === event.currentTarget) onClose(); }}>
event.stopPropagation()}>

{label}

{active === "archived" ? : null}
{active === "desktop" ? : null}{active === "usage" ? : null}{active === "account" ?
{signOutError ?

{signOutError}

: null}
: null}{active === "archived" ? { if (!deleteSession) return; await deleteSession({ id }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} onUnarchive={async (id) => { if (!archiveSession) return; await archiveSession({ id, archived: false }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} /> : null}{active === "coding" ? : null}{active === "worktree" ? : null}{active !== "desktop" && active !== "usage" && active !== "account" && active !== "archived" && active !== "coding" && active !== "worktree" ?
: null}{active === "desktop" && dataDir ?

{dataDir}

: null}
; + return
{ if (event.target === event.currentTarget) onClose(); }}>
event.stopPropagation()}>

{label}

{active === "archived" ? : null}
{active === "desktop" ? : null}{active === "usage" ? : null}{active === "account" ?
{signOutError ?

{signOutError}

: null}
: null}{active === "archived" ? { if (!deleteSession) return; await deleteSession({ id }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} onUnarchive={async (id) => { if (!archiveSession) return; await archiveSession({ id, archived: false }); setArchived((items) => items.filter((item) => item.sessionId !== id)); }} /> : null}{active === "coding" ? : null}{active === "worktree" ? : null}{active !== "desktop" && active !== "usage" && active !== "account" && active !== "archived" && active !== "coding" && active !== "worktree" ?
: null}{active === "desktop" && dataDir ?

{dataDir}

: null}
; } function GenericPage({ theme, setTheme, wrap, setWrap, newTab, setNewTab, contextWindow, setContextWindow, version }: { readonly theme: string; readonly setTheme: (value: string) => void; readonly wrap: boolean; readonly setWrap: (value: boolean) => void; readonly newTab: boolean; readonly setNewTab: (value: boolean) => void; readonly contextWindow: boolean; readonly setContextWindow: (value: boolean) => void; readonly version: string }): ReactElement { @@ -206,20 +206,54 @@ type WebuiReviewStateAction = Parameters[1]; * place to read a whole change set. Every decision that does not need the * network lives in `review-state.ts` so it can be tested without a DOM — this * component is the thin shell that fetches and renders. */ -function SettingsReviewPage({ workspaceDir, onOpenFileLine, getWorkspaceReviewSummary, listWorkspaceReviewFileDiffs, searchWorkspaceReviewDiffs }: { +function SettingsReviewPage({ workspaceDir, onOpenFileLine, loadSessions, getWorkspaceReviewSummary, listWorkspaceReviewFileDiffs, searchWorkspaceReviewDiffs }: { readonly workspaceDir?: string; readonly onOpenFileLine?: (path: string, line: number) => void; + readonly loadSessions?: WebuiSettingsModalCapabilities["loadSessions"]; } & WebuiSettingsReviewCapabilities): ReactElement { const [state, setState] = useState(initialWebuiReviewState); const dispatch = useCallback((action: WebuiReviewStateAction) => { setState((current) => reduceWebuiReviewState(current, action)); }, []); + /* The prop is the selected session's workspace, and it is empty whenever the + * selected session is not bound to a project — including the default + * workspace, which the rail groups under 「未选项目」. A workspace chosen here + * overrides the prop rather than replacing it, so picking one takes effect + * immediately instead of waiting for the user to go change session in the + * rail, which the `aria-modal` dialog does not even let them click. */ + const [pickedWorkspaceDir, setPickedWorkspaceDir] = useState(undefined); + const [workspaceChoices, setWorkspaceChoices] = useState([]); + const [workspaceChoicesLoading, setWorkspaceChoicesLoading] = useState(false); + const [workspaceChoicesError, setWorkspaceChoicesError] = useState(undefined); + const effectiveWorkspaceDir = workspaceDir?.trim() || pickedWorkspaceDir; + + /* Only the chooser needs the session list, so a page opened from a + * workspace-bound session never pays for this fetch. */ + useEffect(() => { + if (workspaceDir || !loadSessions) return; + let cancelled = false; + setWorkspaceChoicesLoading(true); + setWorkspaceChoicesError(undefined); + void loadSessions() + .then((page) => { + if (cancelled) return; + setWorkspaceChoices((page?.sessions ?? []) as readonly WebuiWorktreeSourceSession[]); + setWorkspaceChoicesLoading(false); + }) + .catch((cause: unknown) => { + if (cancelled) return; + setWorkspaceChoicesError(cause instanceof Error ? cause.message : String(cause)); + setWorkspaceChoicesLoading(false); + }); + return () => { cancelled = true; }; + }, [loadSessions, workspaceDir]); + useEffect(() => { - if (!workspaceDir || !getWorkspaceReviewSummary) return; + if (!effectiveWorkspaceDir || !getWorkspaceReviewSummary) return; let cancelled = false; dispatch({ type: "load-begun" }); - void getWorkspaceReviewSummary({ workspaceDir }) + void getWorkspaceReviewSummary({ workspaceDir: effectiveWorkspaceDir }) .then((summary) => { if (cancelled) return; const snapshotId = summary?.reviewSnapshotId; @@ -242,19 +276,19 @@ function SettingsReviewPage({ workspaceDir, onOpenFileLine, getWorkspaceReviewSu dispatch({ type: "load-failed", reason: error instanceof Error ? error.message : String(error) }); }); return () => { cancelled = true; }; - }, [dispatch, getWorkspaceReviewSummary, workspaceDir]); + }, [dispatch, getWorkspaceReviewSummary, effectiveWorkspaceDir]); /* The snapshot id and the query are passed in rather than read back out of * state: reading a `useState` value from inside a setter callback is a type * error waiting to happen and silently captures whatever the reducer saw, * not what the caller meant. */ const loadDiffs = useCallback(async (fileIds: readonly string[], reviewSnapshotId: string) => { - if (!workspaceDir || !listWorkspaceReviewFileDiffs || !fileIds.length) return; + if (!effectiveWorkspaceDir || !listWorkspaceReviewFileDiffs || !fileIds.length) return; dispatch({ type: "diffs-begun", fileIds }); for (let index = 0; index < fileIds.length; index += WEBUI_REVIEW_DIFF_BATCH_SIZE) { const batch = fileIds.slice(index, index + WEBUI_REVIEW_DIFF_BATCH_SIZE); try { - const result = await listWorkspaceReviewFileDiffs({ workspaceDir, reviewSnapshotId, fileIds: [...batch] }); + const result = await listWorkspaceReviewFileDiffs({ workspaceDir: effectiveWorkspaceDir, reviewSnapshotId, fileIds: [...batch] }); const diffs: Record = {}; const errors: Record = {}; for (const entry of (result?.diffs ?? []) as readonly WebuiWorkspaceReviewFileDiff[]) { @@ -268,31 +302,40 @@ function SettingsReviewPage({ workspaceDir, onOpenFileLine, getWorkspaceReviewSu dispatch({ type: "diffs-loaded", diffs: {}, errors: Object.fromEntries(batch.map((fileId) => [fileId, error instanceof Error ? error.message : String(error)])) }); } } - }, [dispatch, listWorkspaceReviewFileDiffs, workspaceDir]); + }, [dispatch, listWorkspaceReviewFileDiffs, effectiveWorkspaceDir]); const runSearch = useCallback(async (reviewSnapshotId: string, query: string) => { - if (!workspaceDir || !searchWorkspaceReviewDiffs || !query.trim()) return; + if (!effectiveWorkspaceDir || !searchWorkspaceReviewDiffs || !query.trim()) return; dispatch({ type: "search-begun" }); try { - const result = await searchWorkspaceReviewDiffs({ workspaceDir, reviewSnapshotId, query, includeUntrackedFiles: true }); + const result = await searchWorkspaceReviewDiffs({ workspaceDir: effectiveWorkspaceDir, reviewSnapshotId, query, includeUntrackedFiles: true }); dispatch({ type: "search-settled", fileIds: (result?.matchedFiles ?? []).map((entry) => entry.fileId) }); } catch { // A failed search must not leave the list filtered by the previous one. dispatch({ type: "search-settled", fileIds: [] }); } - }, [dispatch, searchWorkspaceReviewDiffs, workspaceDir]); + }, [dispatch, searchWorkspaceReviewDiffs, effectiveWorkspaceDir]); const visible = selectWebuiReviewVisibleFiles(state); const filtering = isWebuiReviewFiltering(state); - if (!workspaceDir) { - return ; + /* The picker is the way in. It lists real checkouts, taken from the same + * grouping the worktree page uses, so one checkout cannot appear twice just + * because the runtime spelled its path two ways. */ + if (!effectiveWorkspaceDir) { + return ; } if (state.status === "idle" || state.status === "loading") { return ; } if (state.status === "unavailable" || state.status === "error") { - return ; + return ; } return void; +}): ReactElement { + const state = error ? "error" : loading ? "loading" : workspaces.length ? "ready" : "empty"; + return
+

选一个工作区来审查它的变更。

+ {error ?

{error}

: null} + {loading ?

正在读取可审查的工作区…

: null} + {!loading && !error && workspaces.length === 0 ? ( + /* Saying why matters more than saying that: an empty list under the same + * heading reads as the bug this page is fixing. */ +

现在没有绑定项目目录的会话，所以没有可审查的工作区。在左栏给一个会话选好项目目录，再回到这里。

+ ) : null} +
    + {workspaces.map((workspace) =>
  • + +

    {workspace.workspaceDir}

    +
    • )} +
+
; +} + export function WebuiReviewPanel({ state, visible, filtering, loading, note, empty, workspaceDir, onQueryChange, onClearFilters, onToggleFile, onOpenFileLine }: { readonly state: WebuiReviewState; readonly visible?: readonly WebuiReviewFile[]; diff --git a/packages/webui/test/unit/review-panel.test.tsx b/packages/webui/test/unit/review-panel.test.tsx index b1f187d54..2cfbed0a3 100644 --- a/packages/webui/test/unit/review-panel.test.tsx +++ b/packages/webui/test/unit/review-panel.test.tsx @@ -4,7 +4,7 @@ import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; -import { WebuiReviewPanel } from "../../src/client/components/SettingsModal.js"; +import { WebuiReviewPanel, WebuiReviewWorkspacePicker } from "../../src/client/components/SettingsModal.js"; import { initialWebuiReviewState, reduceWebuiReviewState, @@ -12,6 +12,7 @@ import { isWebuiReviewFiltering, type WebuiReviewFile, } from "../../src/client/projection/review-state.js"; +import { groupWebuiWorktreeWorkspaces } from "../../src/client/projection/worktree-state.js"; /* Why this file asserts on markup rather than on clicks: * @@ -184,6 +185,88 @@ describe("the search box reflects filter state", () => { }); }); +/* Roadmap E 区 follow-up: the review page refused to do anything until the + * selected session already carried a workspace, and the refusal named the + * problem without offering a way out of it. Opening a workspace-bound session + * was the only route, and that route is not reachable from the page — the + * settings dialog is `aria-modal`, so the rail cannot be clicked while it is + * open. That is the same shape of defect as the disabled tabs: the user is + * parked in a state they cannot leave. + * + * The picker is a pure presentational component, exactly like + * `WebuiReviewPanel`, so the list it renders is assertable here. Choosing one + * is a click, and the click's effect on the review load is wiring — asserted + * at the bottom of this file alongside the tab routing. */ +describe("choosing a workspace to review", () => { + const workspaces = groupWebuiWorktreeWorkspaces([ + { sessionId: "s1", title: "主线", updatedAt: 20, workspaceDir: "C:\\repo\\alpha", isDefaultWorkspace: true }, + { sessionId: "s2", title: "实验", updatedAt: 10, workspaceDir: "C:\\repo\\alpha-wt" }, + ]); + + const picker = (props: Record = {}): string => + renderToStaticMarkup(createElement(WebuiReviewWorkspacePicker, props as never)); + + it("offers every reviewable workspace as a control that carries its path", () => { + const markup = picker({ workspaces }); + expect(markup).toContain('data-testid="review-workspace-picker"'); + expect(markup).toContain('data-webui-workspace-dir="C:\\repo\\alpha"'); + expect(markup).toContain('data-webui-workspace-dir="C:\\repo\\alpha-wt"'); + expect(markup).toContain('data-testid="review-workspace-option"'); + }); + + /* The same checkout spelled two ways is one workspace. If the picker did its + * own grouping it would offer the same directory twice. */ + it("does not offer the same checkout twice when the path spelling differs", () => { + const markup = picker({ + workspaces: groupWebuiWorktreeWorkspaces([ + { sessionId: "s1", title: "A", updatedAt: 20, workspaceDir: "C:\\repo\\alpha" }, + { sessionId: "s2", title: "B", updatedAt: 10, workspaceDir: "C:/repo/alpha/" }, + ]), + }); + expect(markup.match(/data-testid="review-workspace-option"/gu)?.length).toBe(1); + }); + + it("marks which workspace is being reviewed", () => { + const markup = picker({ workspaces, selected: "C:\\repo\\alpha" }); + expect(markup).toContain('data-webui-workspace-current="true"'); + const current = markup.split('data-webui-workspace-current="true"')[0]?.split(' { + const markup = picker({ workspaces: [] }); + expect(markup).toContain('data-testid="review-workspace-empty"'); + expect(markup).not.toContain('data-testid="review-workspace-option"'); + }); + + /* The other half of the same sentence: while there is something to pick, the + * explanation is not shown. Without this, "there is nothing here" can sit + * above a full list and still pass. */ + it("does not claim there is nothing to pick while there is something to pick", () => { + expect(picker({ workspaces })).not.toContain('data-testid="review-workspace-empty"'); + }); + + it("says it is loading rather than claiming there are no workspaces", () => { + const markup = picker({ workspaces: [], loading: true }); + expect(markup).toContain('data-webui-workspace-state="loading"'); + expect(markup).not.toContain('data-testid="review-workspace-empty"'); + }); + + /* The path is the identity of a workspace; the basename alone would make two + * checkouts named the same impossible to tell apart. The assertion reads the + * visible paragraph rather than the whole markup: every option also carries + * the path in `data-webui-workspace-dir`, so a whole-markup `toContain` is + * satisfied by the attribute even when the displayed text is the basename + * alone. */ + it("shows the full path, not just the folder name", () => { + const shown = [...picker({ workspaces }).matchAll(/

]*>([^<]*)<\/p>/gu)].map((match) => match[1] ?? ""); + expect(shown.join("|")).toContain("C:\\repo\\alpha"); + expect(shown.join("|")).toContain("C:\\repo\\alpha-wt"); + }); +}); + /* Why these two are source assertions and not render assertions: * * `SettingsModal` opens on the `desktop` tab and only moves to another one @@ -224,3 +307,42 @@ describe("tab-to-page wiring", () => { expect(fallback).toContain('active !== "worktree"'); }); }); + +/* The picker's markup is asserted above; these two close the loop from a click + * to the review load, which no render here can reach — the choice is held in + * component state and the load lives in an effect. The assertions quote the + * exact expressions, so removing the picker call or hard-coding the prop back + * in turns them red. */ +describe("the chosen workspace reaches the review load", () => { + const source = readFileSync( + fileURLToPath(new URL("../../src/client/components/SettingsModal.tsx", import.meta.url)), + "utf8", + ); + const page = source.slice(source.indexOf("function SettingsReviewPage"), source.indexOf("function SettingsWorktreePage")); + + /* Without this the picker would render a list built from nothing. Scoped to + * the review page's own call: the worktree page is handed the same loader, so + * an unscoped substring check is satisfied by that call alone — which is + * exactly what the first negative-injection run found. */ + it("gives the review page the session loader it lists workspaces from", () => { + expect(source).toMatch(/]*loadSessions=\{loadSessions\}/u); + }); + + /* Asserting that the picker component exists would still pass if the page + * never rendered it, which is how the branch that shows it went untested. */ + it("shows the picker when no workspace is bound", () => { + expect(page).toContain("if (!effectiveWorkspaceDir)"); + expect(page).toContain(" { + expect(page).toContain("workspaceDir?.trim() || pickedWorkspaceDir"); + }); + + it("loads the summary for the effective workspace, not the raw prop", () => { + expect(page).toContain("getWorkspaceReviewSummary({ workspaceDir: effectiveWorkspaceDir })"); + }); +}); From 3222a14e5b69be0d49003c8d1ad807da38f60cc7 Mon Sep 17 00:00:00 2001 From: modacker Date: Mon, 5 Oct 2026 22:23:06 +0800 Subject: [PATCH 5/5] fix(webui): tell the truth about unsafe_path instead of claiming nothing was touched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The user-facing copy for `unsafe_path` said the failing file was outside the workspace and that it was left alone for safety. Neither is what the runtime does, and both halves were wrong in the direction that matters most: - `normalizeCapturePath` *accepts* a path resolving outside the workspace (file-changes.ts:657 returns the absolute path). `undefined` comes back for in-workspace paths the capture layer filters — `.git/`, `node_modules/`, the root itself. So the escape case is not refused, it is written. - `safeCapturedPath` is checked inside the write loop of `applyLocalTurnDiffSnapshotMutation` (file-changes.ts:405-416), after earlier entries have already been `writeFile`'d or `fs.rm`'d. The run is not all-or-nothing. Measured on this branch: a captured `../outside/secret.txt` reverts with `success: true` and the file lands outside the workspace; a preceding entry in the same batch is deleted and stays deleted when a later entry trips `unsafe_path`. Say what happened — the run was interrupted and earlier files may have changed. The comments above the copy table and the test block carried the same two false claims and are corrected to match the code. This changes copy only. The escape-accepting `safeCapturedPath` and the mid-loop check are real defects and are left for a separate fix; the commit message on the original commit claims the operation refuses before writing, which it also does not. --- .../webui/src/client/components/DiffCard.tsx | 18 +++++++++++------ .../test/unit/diff-mutation-error.test.tsx | 20 ++++++++++--------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/packages/webui/src/client/components/DiffCard.tsx b/packages/webui/src/client/components/DiffCard.tsx index 8457f2577..8305d2942 100644 --- a/packages/webui/src/client/components/DiffCard.tsx +++ b/packages/webui/src/client/components/DiffCard.tsx @@ -78,17 +78,23 @@ function trimmedOrUndefined(value: string | undefined): string | undefined { * person can do about it. * * The reasons are machine tokens, and the authoritative one is - * `applyLocalTurnDiffSnapshotMutation`'s `unsafe_path`: a captured file sits - * outside the workspace, so the whole operation is refused rather than - * partially applied. Surfacing the raw token tells the user nothing, and the - * other codes read the same way — `conflict` in particular does not say that - * the files changed *after* the turn, which is the part that matters. + * `applyLocalTurnDiffSnapshotMutation`'s `unsafe_path`. Read the runtime, not + * the name: `normalizeCapturePath` *accepts* a path resolving outside the + * workspace, and returns `undefined` for in-workspace paths the capture layer + * filters (`.git/`, `node_modules/`, the root itself). The `safeCapturedPath` + * check also sits inside the write loop, so the operation is NOT all-or-nothing + * — earlier entries have already been written or `fs.rm`'d. The copy below + * therefore says the run was interrupted and earlier files may have changed, + * because that is what happened. Surfacing the raw token tells the user + * nothing, and the other codes read the same way — `conflict` in particular + * does not say that the files changed *after* the turn, which is the part that + * matters. * * A reason not in this table is passed through unchanged. That is deliberate: * `git apply` failures arrive as free-form stderr, and discarding them would * replace a real message with a guess. */ const WEBUI_DIFF_FAILURE_COPY: Readonly> = { - unsafe_path: "这轮改动里有文件不在工作区内，出于安全没有动它。", + unsafe_path: "这轮改动里有文件的路径无法安全定位，操作已中断，之前处理过的文件可能已改动。", conflict: "这轮改动之后文件又被修改过，撤销前请先确认当前内容。", not_undoable: "这轮文件改动没有留下可撤销的快照。", "Turn diff not found": "找不到这轮文件改动。", diff --git a/packages/webui/test/unit/diff-mutation-error.test.tsx b/packages/webui/test/unit/diff-mutation-error.test.tsx index e6211003b..e0f5aba65 100644 --- a/packages/webui/test/unit/diff-mutation-error.test.tsx +++ b/packages/webui/test/unit/diff-mutation-error.test.tsx @@ -294,14 +294,16 @@ describe("resolveWebuiDiffMutation reads the transport result the way the wire m }); /* The runtime's reasons are machine tokens. `applyLocalTurnDiffSnapshotMutation` - * refuses the whole operation with `unsafe_path` when a captured file sits - * outside the workspace — it does not partially apply — and `conflict` when a - * file changed after the turn. Both reach the client verbatim through - * `assertMutationSucceeded`, which throws with `body.error` as the message. - * Without a translation the user reads the token. */ + * reports `unsafe_path` when `safeCapturedPath` cannot resolve a captured + * entry — which `normalizeCapturePath` does for *filtered in-workspace* paths + * (`.git/`, `node_modules/`), not for paths escaping the workspace, those are + * accepted. The check runs inside the write loop, so the run can be + * half-applied. `conflict` fires when a file changed after the turn. Both reach + * the client verbatim through `assertMutationSucceeded`, which throws with + * `body.error` as the message. Without a translation the user reads the token. */ describe("runtime reason codes become something a person can act on", () => { it("translates every reason the mutation path can produce", () => { - expect(describeWebuiDiffFailure("unsafe_path")).toBe("这轮改动里有文件不在工作区内，出于安全没有动它。"); + expect(describeWebuiDiffFailure("unsafe_path")).toBe("这轮改动里有文件的路径无法安全定位，操作已中断，之前处理过的文件可能已改动。"); expect(describeWebuiDiffFailure("conflict")).toBe("这轮改动之后文件又被修改过，撤销前请先确认当前内容。"); expect(describeWebuiDiffFailure("not_undoable")).toBe("这轮文件改动没有留下可撤销的快照。"); expect(describeWebuiDiffFailure("Turn diff not found")).toBe("找不到这轮文件改动。"); @@ -311,7 +313,7 @@ describe("runtime reason codes become something a person can act on", () => { it("no longer shows a raw token as the user-facing reason", () => { const action = resolveWebuiDiffMutation("revert", { success: false, error: "unsafe_path" }); - expect(action).toEqual({ type: "mutation-failed", error: "这轮改动里有文件不在工作区内，出于安全没有动它。" }); + expect(action).toEqual({ type: "mutation-failed", error: "这轮改动里有文件的路径无法安全定位，操作已中断，之前处理过的文件可能已改动。" }); expect(action.type === "mutation-failed" && action.error).not.toBe("unsafe_path"); }); @@ -319,7 +321,7 @@ describe("runtime reason codes become something a person can act on", () => { * a real message; replacing them with a guess would be worse than useless. */ it("passes an unknown reason through unchanged", () => { expect(describeWebuiDiffFailure("error: patch failed: src/one.ts:3")).toBe("error: patch failed: src/one.ts:3"); - expect(describeWebuiDiffFailure(" unsafe_path ")).toBe("这轮改动里有文件不在工作区内，出于安全没有动它。"); + expect(describeWebuiDiffFailure(" unsafe_path ")).toBe("这轮改动里有文件的路径无法安全定位，操作已中断，之前处理过的文件可能已改动。"); }); it("still reports no reason when there is none", () => { @@ -331,7 +333,7 @@ describe("runtime reason codes become something a person can act on", () => { it("translates a thrown unsafe_path the same way as a reported one", () => { expect(resolveWebuiDiffMutation("revert", undefined, new Error("unsafe_path"))).toEqual({ type: "mutation-failed", - error: "这轮改动里有文件不在工作区内，出于安全没有动它。", + error: "这轮改动里有文件的路径无法安全定位，操作已中断，之前处理过的文件可能已改动。", }); }); });