From d6f065a2b62be1ba13dae943e32530f9f240e04a Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 11:11:43 -0700 Subject: [PATCH] Fix HIGH and MEDIUM frontend defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Files pane - F16: a drag-out released back inside the app no longer re-imports its own staged copy over the container original. An in-flight flag (cleared from the drag plugin's `onEvent` channel, with a watchdog) suppresses the drop and the "Drop files into …" hint, and an exact staged-path filter is the second line of defence — the `path|size|modified` cache could otherwise write a minutes-old snapshot over a file an agent had since rewritten. - F17: a slow upload/rename no longer yanks the user back to the directory the operation started in. Every operation captures its target path and re-lists only if the user is still there; failures go to the toast host either way. - The grid keeps keyboard focus. Roving tabindex (one tab stop, not one per row) plus focus restore after navigation, rename commit/cancel and Escape. - Transient failures now surface in `ToastHost` (z-[60], persistent aria-live) instead of a `role="alert"` 300 rows down a scroller or behind a modal overlay. The inline error is kept only for the listing failure. - `navigate` is sequenced by generation; "Save to host…" sets `busy`. - Grid a11y: column headers, a text affordance for folder vs file, a live region that is mounted empty and announces completion, Label-in-Name fixed. - FileViewerModal: the blob URL is released only once its replacement exists; the preview is a focusable, named, scrollable region. Native drop routing - New `lib/dropTarget.ts`: the hit test now refuses a drop while any `[aria-modal="true"]` dialog or `[data-blocks-drop]` overlay is up, and checks z-order where the environment can answer it. Shared by FilesTab and TerminalView; App's shutdown overlay opts in. Disk - A partially failed reclaim says so in words ("… — 2 of 5 failed"), not by hue alone. - The scan/reclaim race is closed: every mutation retires an in-flight scan, so a scan can no longer repaint a pre-reclaim report plus a clickable plan of objects that are gone. Scan is disabled while working; the status is a live region; a failed destructive action keeps its dialog open and reports there. - The "unknown" layer count gets a screen-reader fallback; `--text-disabled` no longer carries live information. Terminal / OAuth - After the toast is dismissed, a truncated heuristic guess can no longer fill the slot that an exact OSC 8 or relay URL occupied — the detector remembers every exact URL and drops any candidate that is a strict prefix of one. - The prompt is reachable by keyboard: Ctrl+Shift+O jumps to the default action, Escape dismisses, focus returns to the terminal, and auto-dismiss holds off while focus is inside. It deliberately does not steal focus. - UrlToast renders through `ui/Button` and `--shadow-overlay`. Elsewhere - AuthBridgeRow: a pushed `auth-bridge-changed` status always outranks an older awaited toggle result. - The last two ad-hoc byte formatters route through `lib/formatBytes`. Contract for the backend agent: `upload_file_to_container` refusing to overwrite must satisfy `isFileExistsError` in `src/lib/uploadErrors.ts` (marker `FILE_EXISTS`) and accept an `overwrite` argument; the frontend turns that into an `ui/Modal` Replace/Skip prompt rather than a raw error string. Tests: 536 -> 627 passing. `npm run build` and `npx tsc --noEmit` green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- app/src/App.tsx | 3 + .../ClaudeCodeSettingsEditor.test.tsx | 73 ++++ .../projects/home/FileViewerModal.tsx | 58 ++- .../projects/home/FilesTab.test.tsx | 354 ++++++++++++++++- app/src/components/projects/home/FilesTab.tsx | 359 +++++++++++++++--- .../projects/home/OverwriteConfirmModal.tsx | 73 ++++ .../home/config/AuthBridgeRow.test.tsx | 74 ++++ .../projects/home/config/AuthBridgeRow.tsx | 46 ++- .../home/config/RuntimeSection.test.tsx | 52 +++ .../components/settings/DiskProjectTable.tsx | 28 +- .../components/settings/DiskSettings.test.tsx | 214 +++++++++++ app/src/components/settings/DiskSettings.tsx | 102 ++++- app/src/components/settings/UpdateDialog.tsx | 12 +- .../components/terminal/TerminalView.test.tsx | 205 +++++++++- app/src/components/terminal/TerminalView.tsx | 113 ++++-- app/src/components/terminal/UrlToast.test.tsx | 109 +++++- app/src/components/terminal/UrlToast.tsx | 156 ++++---- .../components/ui/TypedConfirmModal.test.tsx | 13 + app/src/components/ui/TypedConfirmModal.tsx | 20 +- app/src/hooks/useDiskUsage.test.tsx | 153 +++++++- app/src/hooks/useDiskUsage.ts | 142 ++++--- app/src/hooks/useFileManager.test.ts | 223 ++++++++++- app/src/hooks/useFileManager.ts | 300 +++++++++++++-- app/src/hooks/useProjectActions.ts | 10 +- app/src/lib/dropTarget.test.ts | 76 ++++ app/src/lib/dropTarget.ts | 81 ++++ app/src/lib/formatBytes.test.ts | 16 + app/src/lib/formatBytes.ts | 13 +- app/src/lib/tauri-commands.ts | 17 +- app/src/lib/uploadErrors.test.ts | 81 ++++ app/src/lib/uploadErrors.ts | 116 ++++++ app/src/lib/urlDetector.test.ts | 49 +++ app/src/lib/urlDetector.ts | 94 ++++- 33 files changed, 3120 insertions(+), 315 deletions(-) create mode 100644 app/src/components/projects/home/OverwriteConfirmModal.tsx create mode 100644 app/src/lib/dropTarget.test.ts create mode 100644 app/src/lib/dropTarget.ts create mode 100644 app/src/lib/uploadErrors.test.ts create mode 100644 app/src/lib/uploadErrors.ts diff --git a/app/src/App.tsx b/app/src/App.tsx index 7245f4f..0226df1 100644 --- a/app/src/App.tsx +++ b/app/src/App.tsx @@ -156,6 +156,9 @@ export default function App() { className="fixed inset-0 z-50 flex items-center justify-center bg-[var(--bg-primary)]/95 backdrop-blur-sm" role="status" aria-live="polite" + /* Covers the whole window, so no pane underneath may accept a + native file drop while it is up — see `lib/dropTarget.ts`. */ + data-blocks-drop="true" data-testid="shutdown-overlay" >
diff --git a/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx b/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx index e7d61a3..4993a32 100644 --- a/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx +++ b/app/src/components/projects/ClaudeCodeSettingsEditor.test.tsx @@ -119,5 +119,78 @@ describe("ClaudeCodeSettingsEditor", () => { "off", ); }); + + /** + * Auto-scroll is the second inverted field and had no project-scope test at + * all — every assertion above rides on `session_recap_disabled`, so a + * `BOOLEAN_FIELDS` entry that lost its `invert` flag would be caught for + * one of the two and pass silently for the other. It is stored as + * `auto_scroll_disabled`, so every value here reads back the other way up. + */ + describe("auto-scroll", () => { + const AUTO = "Auto-scroll"; + + it("starts on Global, which is not the same as on", () => { + // Claude Code scrolls by default, so an inheriting project *behaves* + // as on — but it has taken no position, and rendering it as "On" would + // make a later global change look like it had no effect. + renderEditor(null, "project"); + expect((screen.getByLabelText(AUTO) as HTMLSelectElement).value).toBe("global"); + }); + + it("stores the disabled sense in both directions", () => { + const onSave = renderEditor(null, "project"); + const auto = screen.getByLabelText(AUTO); + + fireEvent.change(auto, { target: { value: "off" } }); + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ auto_scroll_disabled: true }), + ); + + fireEvent.change(auto, { target: { value: "on" } }); + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ auto_scroll_disabled: false }), + ); + }); + + it("hands the setting back to the global level when Global is chosen", () => { + // Back to no opinion, and with nothing else set that collapses the + // whole object to `null` — the value that means "adds nothing over the + // global settings". + const onSave = renderEditor( + { ...CLAUDE_CODE_DEFAULTS, auto_scroll_disabled: true }, + "project", + ); + fireEvent.change(screen.getByLabelText(AUTO), { target: { value: "global" } }); + expect(onSave).toHaveBeenCalledWith(null); + }); + + it("reads a stored override back the right way up", () => { + renderEditor({ ...CLAUDE_CODE_DEFAULTS, auto_scroll_disabled: true }, "project"); + expect((screen.getByLabelText(AUTO) as HTMLSelectElement).value).toBe("off"); + }); + }); + }); + + /** + * The inverted fields store a *deviation*, so a stored `false` is the one + * value that means "the user deliberately re-enabled the default". Nothing + * asserted it: every existing test drives the `true` (turned off) direction + * or the `null` (untouched) one, and both scopes would still read correctly + * if the inversion were dropped from the `false` branch alone. + */ + describe.each([ + ["session_recap_disabled", "Session recap"] as const, + ["auto_scroll_disabled", "Auto-scroll"] as const, + ])("a stored false on %s", (key, label) => { + it("reads as On at project scope, not as Off", () => { + renderEditor({ ...CLAUDE_CODE_DEFAULTS, [key]: false }, "project"); + expect((screen.getByLabelText(label) as HTMLSelectElement).value).toBe("on"); + }); + + it("reads as on at global scope, where the control is a switch", () => { + renderEditor({ ...CLAUDE_CODE_DEFAULTS, [key]: false }); + expect(screen.getByRole("switch", { name: label })).toBeChecked(); + }); }); }); diff --git a/app/src/components/projects/home/FileViewerModal.tsx b/app/src/components/projects/home/FileViewerModal.tsx index 213bf15..672ec88 100644 --- a/app/src/components/projects/home/FileViewerModal.tsx +++ b/app/src/components/projects/home/FileViewerModal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import type { FileEntry } from "../../../lib/types"; import { readContainerFile } from "../../../lib/tauri-commands"; import Button from "../../ui/Button"; @@ -40,11 +40,28 @@ type Preview = export default function FileViewerModal({ projectId, entry, onClose, onSaveToHost }: Props) { const [preview, setPreview] = useState({ kind: "loading" }); + /** + * The object URL currently on screen. + * + * This used to be an effect-local variable revoked from the effect's own + * cleanup, which runs *before* the replacement effect body — so switching + * entries (or any re-run of the effect for the same entry) released the URL + * the `` was still pointing at, and a blank image was the result until + * the new read landed. If the new read failed, it stayed blank. So the + * hand-over is explicit instead: a URL is revoked only once its replacement + * exists, and unmount is what releases the last one. + */ + const objectUrlRef = useRef(null); + + /** Release the previous URL now that something else is on screen. */ + const replaceObjectUrl = (next: string | null) => { + const previous = objectUrlRef.current; + objectUrlRef.current = next; + if (previous && previous !== next) URL.revokeObjectURL(previous); + }; + useEffect(() => { let cancelled = false; - // Tracked separately from `preview` so cleanup can revoke it without - // depending on which state the component ended up in. - let objectUrl: string | null = null; (async () => { try { @@ -58,16 +75,20 @@ export default function FileViewerModal({ projectId, entry, onClose, onSaveToHos // A truncated image is not a smaller image, it is a broken one. if (result.truncated) { setPreview({ kind: "too-large" }); + replaceObjectUrl(null); return; } const blob = new Blob([bytes], { type: imageMimeFor(entry.name) ?? "image/png" }); - objectUrl = URL.createObjectURL(blob); - setPreview({ kind: "image", url: objectUrl }); + const url = URL.createObjectURL(blob); + // The replacement is in hand, so the previous one can go. + setPreview({ kind: "image", url }); + replaceObjectUrl(url); return; } if (looksBinary(bytes)) { setPreview({ kind: "unsupported" }); + replaceObjectUrl(null); return; } @@ -78,6 +99,7 @@ export default function FileViewerModal({ projectId, entry, onClose, onSaveToHos shownBytes: bytes.length, trueSize: result.size, }); + replaceObjectUrl(null); } catch (e) { if (!cancelled) setPreview({ kind: "error", message: String(e) }); } @@ -85,10 +107,19 @@ export default function FileViewerModal({ projectId, entry, onClose, onSaveToHos return () => { cancelled = true; - if (objectUrl) URL.revokeObjectURL(objectUrl); }; }, [projectId, entry.name, entry.path]); + // The bytes are released when the dialog goes, which is the whole reason the + // preview is a `blob:` URL rather than a `data:` one. + useEffect( + () => () => { + if (objectUrlRef.current) URL.revokeObjectURL(objectUrlRef.current); + objectUrlRef.current = null; + }, + [], + ); + const footer = ( <>
+ {/* The one failure that stays inline: it explains why the grid below is + empty, it is in context, and there are no rows for it to scroll + behind. Every *transient* failure — upload, rename, mkdir, + save-to-host, staging — goes to `ToastHost` instead, which is above + the file viewer's overlay and does not scroll away. */} {error && (
{error} @@ -379,9 +563,25 @@ export default function FilesTab({ project }: Props) {
) : ( + + + + + + + + {creatingFolder && ( - + setSelected(entry.name)} + onClick={() => { + setSelected(entry.name); + setActiveRow(entry.name); + }} onDoubleClick={() => openEntry(entry)} {...dragOutProps(entry)} onKeyDown={(e) => { @@ -440,13 +652,17 @@ export default function FilesTab({ project }: Props) { if (e.key === "Enter") { e.preventDefault(); setSelected(entry.name); + setActiveRow(entry.name); openEntry(entry); } else if (e.key === "F2") { e.preventDefault(); startRename(entry); } else if (e.key === "ArrowDown" || e.key === "ArrowUp") { e.preventDefault(); - moveFocus(e.currentTarget, e.key === "ArrowDown" ? 1 : -1); + moveActive(entry.name, e.key === "ArrowDown" ? 1 : -1); + } else if (e.key === "Home" || e.key === "End") { + e.preventDefault(); + moveActive(entry.name, e.key === "Home" ? "first" : "last"); } }} className={rowClass(isSelected)} @@ -476,6 +692,14 @@ export default function FilesTab({ project }: Props) { : "text-[var(--text-primary)]" }`} > + {/* Directory-ness was carried by hue and an + `aria-hidden` emoji, i.e. by nothing at all for a + screen reader. The emoji stays hidden — it reads + as "file folder" in some voices and as nothing in + others — and the word is what is announced. */} + + {entry.is_directory ? "Folder, " : "File, "} + {entry.is_directory && } {entry.name} {entry.is_symlink && ( @@ -498,8 +722,14 @@ export default function FilesTab({ project }: Props) { +
+ Name + + Size + + Modified + + Actions +
setActiveRow(PARENT_ROW)} + onDoubleClick={openParent} onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); - goUp(); + openParent(); } else if (e.key === "ArrowDown" || e.key === "ArrowUp") { e.preventDefault(); - moveFocus(e.currentTarget, e.key === "ArrowDown" ? 1 : -1); + moveActive(PARENT_ROW, e.key === "ArrowDown" ? 1 : -1); + } else if (e.key === "Home" || e.key === "End") { + e.preventDefault(); + moveActive(PARENT_ROW, e.key === "Home" ? "first" : "last"); } }} className="cursor-pointer hover:bg-[var(--bg-tertiary)] transition-colors" > + Folder, .. @@ -430,9 +637,14 @@ export default function FilesTab({ project }: Props) { return (
{!isRenaming && ( <> + {/* WCAG 2.5.3: the accessible name has to *contain* + the visible label, so the row context is appended + rather than substituted. "Rename notes.txt" used + to be the whole name, which left a voice-control + user saying "click Rename" at a button that had + no such name. */} {!entry.is_directory && (
)} + {conflict && ( + + )} + {viewing && ( void; +} + +/** + * "That name is taken — replace it?" + * + * This exists because the backend stopped overwriting silently, and a raw + * error string would have been a worse answer than the old silent clobber: it + * tells the user their drop failed without telling them it *can* succeed. The + * dialog names the file and the directory, because a drop is aimed with a + * mouse and "notes.txt" alone does not say which `notes.txt`. + * + * The blanket answers only appear when there is something to apply them to — a + * single-file drop with "Replace all" on it invites the reflex of clicking the + * widest button for no benefit. + * + * Dismissing (Escape, ✕, click-outside) is a **skip**, never a replace: the + * destructive answer has to be chosen explicitly. + */ +export default function OverwriteConfirmModal({ name, directory, remaining, onChoose }: Props) { + const footer = ( + <> + {remaining > 0 && ( + <> + + + + )} + + + + ); + + return ( + onChoose("skip")} + footer={footer} + widthClassName="w-[30rem]" + > +

+ {name} already exists in{" "} + {directory}. Replacing it overwrites the container's + copy, and that cannot be undone from here. +

+ {remaining > 0 && ( +

+ {remaining} more file{remaining === 1 ? "" : "s"} still to upload. +

+ )} + + ); +} diff --git a/app/src/components/projects/home/config/AuthBridgeRow.test.tsx b/app/src/components/projects/home/config/AuthBridgeRow.test.tsx index 12e0199..98f7eb5 100644 --- a/app/src/components/projects/home/config/AuthBridgeRow.test.tsx +++ b/app/src/components/projects/home/config/AuthBridgeRow.test.tsx @@ -101,6 +101,80 @@ describe("AuthBridgeRow", () => { expect(screen.queryByText(/Port 1:/)).not.toBeInTheDocument(); }); + /** + * The two halves of this row disagree about *when*, not about *what*. + * + * `set_auth_bridge_enabled` resolves with a status sampled as it returned; + * the poller's event carries one sampled afterwards. Writing the awaited + * value unconditionally therefore rolls the row back in time whenever the + * two overlap — the row says "Watching" while a port is bound, which is the + * exact silent failure the event subscription was added to end. These two + * hold the ordering down from both the resolve and the reject side. + */ + describe("a pushed event outranks an older awaited result", () => { + /** A toggle that will not settle until the test says so. */ + function deferToggle() { + let settle!: (s: AuthBridgeStatus) => void; + let fail!: (e: unknown) => void; + setAuthBridgeEnabled.mockImplementation( + () => + new Promise((resolve, reject) => { + settle = resolve; + fail = reject; + }), + ); + return { settle: (s: AuthBridgeStatus) => settle(s), fail: (e: unknown) => fail(e) }; + } + + const BRIDGING: AuthBridgeStatus = { + enabled: true, + active_ports: [{ port: 54545, family: "v4", bridged_at: "", ipv6_warning: null }], + conflicts: [], + }; + + async function startToggleThenPush() { + render(); + await waitFor(() => expect(getAuthBridgeStatus).toHaveBeenCalled()); + await waitFor(() => expect(emit).not.toBeNull()); + + fireEvent.click(screen.getByRole("switch", { name: "Auth bridge" })); + await waitFor(() => expect(setAuthBridgeEnabled).toHaveBeenCalledWith("p1", true)); + + // The poller binds a port while the command is still in flight. + emit!({ project_id: "p1", status: BRIDGING }); + expect(await screen.findByText("Bridging 1 port")).toBeInTheDocument(); + } + + it("keeps the newer state when the command settles with the older one", async () => { + const toggle = deferToggle(); + await startToggleThenPush(); + + // …and only now returns the snapshot it took *before* that port existed. + toggle.settle({ enabled: true, active_ports: [], conflicts: [] }); + + await waitFor(() => + expect(screen.getByRole("switch", { name: "Auth bridge" })).not.toBeDisabled(), + ); + expect(screen.getByText("Bridging 1 port")).toBeInTheDocument(); + expect(screen.getByText("127.0.0.1:54545")).toBeInTheDocument(); + expect(screen.queryByText("Watching")).not.toBeInTheDocument(); + }); + + it("does not let the rollback undo a status pushed while it was failing", async () => { + // The command failed, so the error belongs on screen — but the bridge + // demonstrably came up, and reverting the switch to off would contradict + // the port listed right beside it. + const toggle = deferToggle(); + await startToggleThenPush(); + + toggle.fail("bridge probe timed out"); + + expect(await screen.findByText(/probe timed out/)).toBeInTheDocument(); + expect(screen.getByText("Bridging 1 port")).toBeInTheDocument(); + expect(screen.getByRole("switch", { name: "Auth bridge" })).toBeChecked(); + }); + }); + it("puts the switch back if the command rejects", async () => { setAuthBridgeEnabled.mockRejectedValue("Project p1 not found"); render(); diff --git a/app/src/components/projects/home/config/AuthBridgeRow.tsx b/app/src/components/projects/home/config/AuthBridgeRow.tsx index ea8bc92..a8613e7 100644 --- a/app/src/components/projects/home/config/AuthBridgeRow.tsx +++ b/app/src/components/projects/home/config/AuthBridgeRow.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { listen } from "@tauri-apps/api/event"; import { getAuthBridgeStatus, @@ -77,13 +77,35 @@ export default function AuthBridgeRow({ project }: { project: Project }) { const [busy, setBusy] = useState(false); const [error, setError] = useState(null); + /** + * Which write to `status` is the newest — the same "is this still mine?" + * guard `useDiskUsage` and `useContainerMigration` use around their async + * writes, and needed here for a reason that is easy to miss. + * + * There are two sources of truth for this row and only one of them is + * ordered. `set_auth_bridge_enabled` resolves with a status *sampled at the + * moment it returned*; the poller's `auth-bridge-changed` event carries one + * sampled later. Awaiting the command therefore hands back a value that may + * already be historical, and writing it unconditionally is how the row ends + * up saying "Watching" while a port is in fact bound — the failure mode the + * event subscription exists to prevent, reintroduced one line below it. + * + * So every write claims a generation and only lands if it still holds it. + * A pushed event always claims a fresh one, which is what makes it win over + * an older awaited result no matter which order the two arrive in. + */ + const generation = useRef(0); + useEffect(() => { + const mine = ++generation.current; let cancelled = false; setStatus(null); setError(null); getAuthBridgeStatus(projectId) .then((s) => { - if (!cancelled) setStatus(s); + // The initial fetch races the poller exactly like the toggle does: an + // event can land first and describe a bridge this reply predates. + if (!cancelled && generation.current === mine) setStatus(s); }) .catch((e) => { if (!cancelled) setError(String(e)); @@ -98,6 +120,9 @@ export default function AuthBridgeRow({ project }: { project: Project }) { let unlisten: (() => void) | undefined; listen(AUTH_BRIDGE_EVENT, (event) => { if (event.payload.project_id !== projectId) return; + // A pushed status is the most recent observation that exists, so it + // claims the newest generation and invalidates anything still in flight. + generation.current += 1; setStatus(event.payload.status); }) .then((un) => { @@ -116,13 +141,24 @@ export default function AuthBridgeRow({ project }: { project: Project }) { setBusy(true); setError(null); // Optimistic, so the switch responds even though enabling has to await a - // container probe. The command's return value replaces it either way. + // container probe. It claims a generation like every other write, so a + // pushed event that lands mid-flight supersedes it rather than being + // undone by the settle below. + const mine = ++generation.current; setStatus((s) => (s ? { ...s, enabled: next } : s)); try { - setStatus(await setAuthBridgeEnabled(projectId, next)); + const settled = await setAuthBridgeEnabled(projectId, next); + // Stale by the time it arrived: the poller has already told us + // something newer, and `settled` predates it. + if (generation.current !== mine) return; + setStatus(settled); } catch (e) { - setStatus((s) => (s ? { ...s, enabled: !next } : s)); + // The error is reported either way — the command really did fail — but + // the rollback must not resurrect the pre-toggle value over a status + // the poller pushed while the command was failing. setError(String(e)); + if (generation.current !== mine) return; + setStatus((s) => (s ? { ...s, enabled: !next } : s)); } finally { setBusy(false); } diff --git a/app/src/components/projects/home/config/RuntimeSection.test.tsx b/app/src/components/projects/home/config/RuntimeSection.test.tsx index e8b7201..4733c09 100644 --- a/app/src/components/projects/home/config/RuntimeSection.test.tsx +++ b/app/src/components/projects/home/config/RuntimeSection.test.tsx @@ -107,6 +107,58 @@ describe("RuntimeSection — VPN support toggle", () => { }); }); +/** + * `scope="project"` on the settings editor is one prop with no visible owner, + * and deleting it fails silently in the worst possible direction: the editor + * falls back to `"global"`, every three-state control collapses to an on/off + * switch, and a field the project is *inheriting* as on renders flat Off. The + * user then reads a lie and, worse, flipping that switch writes a deliberate + * `false` that overrides the global On they thought they were looking at. + * + * Nothing asserted the prop was passed, so these go through what is rendered + * rather than through props — a switch where a select belongs is exactly the + * regression, and it is visible from the outside. + */ +describe("RuntimeSection — Claude Code settings are edited at project scope", () => { + beforeEach(() => vi.clearAllMocks()); + + it("gives every setting the third Global state a project can inherit through", () => { + renderSection(); + const focus = screen.getByLabelText("Focus mode") as HTMLSelectElement; + expect( + Array.from(focus.querySelectorAll("option")).map((o) => o.getAttribute("value")), + ).toEqual(["global", "off", "on"]); + }); + + it("renders an untouched setting as inheriting, not as Off", () => { + // `claude_code_settings: null` means "this project has no opinion", which + // is not the same instruction as off. At global scope the same field is a + // plain unchecked switch — indistinguishable from a user who turned it + // off, and the reason the missing prop would never be noticed. + renderSection({ claude_code_settings: null }); + expect((screen.getByLabelText("Focus mode") as HTMLSelectElement).value).toBe( + "global", + ); + expect(screen.queryByRole("switch", { name: "Focus mode" })).not.toBeInTheDocument(); + }); + + it("keeps a stored project override visible over the inherited value", () => { + renderSection({ + claude_code_settings: { + tui_mode: null, + effort: null, + auto_scroll_disabled: null, + focus_mode: true, + show_thinking_summaries: null, + session_recap_disabled: null, + env_scrub: null, + prompt_caching_1h: null, + }, + }); + expect((screen.getByLabelText("Focus mode") as HTMLSelectElement).value).toBe("on"); + }); +}); + describe("RuntimeSection — auth bridge toggle", () => { beforeEach(() => vi.clearAllMocks()); diff --git a/app/src/components/settings/DiskProjectTable.tsx b/app/src/components/settings/DiskProjectTable.tsx index 0c9b9e3..a787fbf 100644 --- a/app/src/components/settings/DiskProjectTable.tsx +++ b/app/src/components/settings/DiskProjectTable.tsx @@ -17,6 +17,10 @@ const LAYERS_HELP = const NEXT_COMMIT_HELP = "The container's writable layer. This is exactly what the next recreation will stack onto the snapshot, and it never comes back after that."; +/** Why a layer count reads "unknown" rather than as a number. */ +const layersUnknownHelp = (layers: number) => + `${layers} layers in total, but this project predates the base-image label, so there is no way to tell which of them are commits. Migrating it to the current base restores the count.`; + /** `—` for a column with nothing in it, so an empty cell never reads as zero. */ function cell(bytes: number, present: boolean) { return present ? formatBytes(bytes) : "—"; @@ -141,11 +145,25 @@ export default function DiskProjectTable({ rows, destructive, onDestroy }: Props // The base this descends from is unknown, so the count // includes the base's own layers and does not mean // "recreations". Saying so beats printing a wrong number. - - unknown - + // + // The explanation is the only thing standing between + // "unknown" and reading as a bug, so it cannot live in the + // tooltip alone: `Tooltip` portals a plain div with no + // `role` and no `aria-describedby`, and wrapped around + // children it has no focus handlers either — so on hover- + // less input it is unreachable and to a screen reader it + // does not exist. Same treatment as the column headers + // above: tooltip for the mouse, `sr-only` text for + // everything else. + <> + + unknown + + + {" "} + — {layersUnknownHelp(row.snapshot_commit_layers)} + + ) : ( {row.snapshot_commit_layers} diff --git a/app/src/components/settings/DiskSettings.test.tsx b/app/src/components/settings/DiskSettings.test.tsx index 07ec58d..f2777b3 100644 --- a/app/src/components/settings/DiskSettings.test.tsx +++ b/app/src/components/settings/DiskSettings.test.tsx @@ -6,6 +6,7 @@ import type { ProjectDiskRow, ReclaimItem, ReclaimPlan, + ReclaimResult, ReclaimTarget, } from "../../lib/types"; @@ -104,6 +105,16 @@ const item = (over: Partial = {}): ReclaimItem => ({ ...over, }); +const result = (over: Partial = {}): ReclaimResult => ({ + target: { kind: "dangling_snapshots" }, + destroyed: null, + ok: true, + freed_bytes: 0, + projected_bytes: null, + message: "Removed 3 images.", + ...over, +}); + const plan = (over: Partial = {}): ReclaimPlan => ({ items: [item()], destructive: [], @@ -620,6 +631,209 @@ describe("DiskSettings", () => { expect(within(outcome).getByText(/projected up to 7\.0 GB, actually 5\.1 GB/)).toBeInTheDocument(); }); + // ------------------------------------------------------------------------- + // Failure has to reach the words, and the place the user is looking + // ------------------------------------------------------------------------- + + it("puts a partial failure in the headline, not only in the glyph's hue", async () => { + // This panel is where the "never encode status in colour alone" rule is + // documented, and the outcome headline used to say "Reclaimed 1.2 GB" for + // a run where most of the targets threw — only the glyph and its colour + // changed, which is exactly nothing to a screen reader or to anyone who + // does not read red as bad. + reclaim.mockResolvedValue({ + results: [ + result({ freed_bytes: 1_200_000_000 }), + result({ target: { kind: "migration_pins" } }), + result({ target: { kind: "probe_containers" } }), + result({ target: { kind: "build_cache", all: true }, ok: false }), + result({ target: { kind: "orphan_volume", name: "v" }, ok: false }), + ], + total_freed_bytes: 1_200_000_000, + }); + await renderAndScan(); + await screen.findByTestId("disk-safe-bucket"); + fireEvent.click(screen.getAllByRole("checkbox")[0]); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Reclaim" })); + }); + + const outcome = await screen.findByTestId("disk-outcome"); + expect( + within(outcome).getByText("Reclaimed 1.2 GB — 2 of 5 failed"), + ).toBeInTheDocument(); + }); + + it("keeps the plain wording when every target succeeded", async () => { + reclaim.mockResolvedValue({ + results: [result({ freed_bytes: 1_200_000_000 }), result()], + total_freed_bytes: 1_200_000_000, + }); + await renderAndScan(); + await screen.findByTestId("disk-safe-bucket"); + fireEvent.click(screen.getAllByRole("checkbox")[0]); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Reclaim" })); + }); + + const outcome = await screen.findByTestId("disk-outcome"); + expect(within(outcome).getByText("Reclaimed 1.2 GB")).toBeInTheDocument(); + expect(outcome.textContent).not.toMatch(/failed/); + }); + + it("keeps the typed confirmation open, and says why, when the deletion fails", async () => { + // The dialog used to close regardless, leaving the failure in a line at + // the very top of a panel the user had scrolled past to reach the row. + listReclaimable.mockResolvedValue( + plan({ + destructive: [ + { + target: { kind: "home_volume", project_id: "p-whp" }, + project_id: "p-whp", + project_name: "whp", + label: "Home volume", + loses: "Shell history and toolchains.", + bytes: 4_860_000_000, + blocked: null, + }, + ], + }), + ); + destroyProjectDiskObject.mockRejectedValue( + "volume triple-c-home-p-whp is in use by a running container", + ); + + await renderAndScan(); + await screen.findByTestId("disk-row-p-whp"); + fireEvent.click(screen.getByRole("button", { name: "Delete whp data" })); + await act(async () => { + fireEvent.click(screen.getByRole("menuitem", { name: /Delete home volume/ })); + }); + + const dialog = screen.getByRole("dialog"); + fireEvent.change(within(dialog).getByLabelText(/Type/), { target: { value: "whp" } }); + await act(async () => { + fireEvent.click(within(dialog).getByRole("button", { name: "Delete home volume" })); + }); + + expect(screen.getByRole("dialog")).toBeInTheDocument(); + expect(within(screen.getByRole("dialog")).getByRole("alert")).toHaveTextContent( + /in use by a running container/, + ); + }); + + it("keeps the semi-safe confirmation open when the action fails", async () => { + listReclaimable.mockResolvedValue( + plan({ + items: [ + item({ + target: { kind: "compact_snapshot", project_id: "p-whp" }, + safety: "semi_safe", + label: "Compact whp's snapshot", + bytes: 5_100_000_000, + bytes_are_exact: false, + bytes_floor: 0, + }), + ], + }), + ); + reclaim.mockRejectedValue("compaction failed: no space left on device"); + + await renderAndScan(); + const semi = await screen.findByTestId("disk-semi-bucket"); + await act(async () => { + fireEvent.click(within(semi).getByRole("button", { name: "Run…" })); + }); + await act(async () => { + fireEvent.click( + within(screen.getByRole("dialog")).getByRole("button", { name: "Run it" }), + ); + }); + + const dialog = screen.getByRole("dialog"); + expect(dialog).toBeInTheDocument(); + expect(within(dialog).getByRole("alert")).toHaveTextContent(/no space left on device/); + }); + + it("closes the confirmation once the action succeeds", async () => { + listReclaimable.mockResolvedValue( + plan({ + items: [ + item({ + target: { kind: "clear_caches", project_id: "p-whp", include_rustup: false }, + safety: "semi_safe", + label: "Clear whp's caches", + }), + ], + }), + ); + await renderAndScan(); + const semi = await screen.findByTestId("disk-semi-bucket"); + await act(async () => { + fireEvent.click(within(semi).getByRole("button", { name: "Run…" })); + }); + await act(async () => { + fireEvent.click( + within(screen.getByRole("dialog")).getByRole("button", { name: "Run it" }), + ); + }); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + + // ------------------------------------------------------------------------- + // Scan status: announced, and not startable mid-mutation + // ------------------------------------------------------------------------- + + it("announces the scan status through a live region", async () => { + // The status flips between three states with no other signal; without a + // live region wrapping it the change is silent. + render(); + const live = screen.getByRole("status"); + expect(live).toHaveAttribute("aria-live", "polite"); + expect(live).toHaveTextContent("Not scanned"); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Scan" })); + }); + // The glyph is `aria-hidden` but still part of `textContent`. + expect(screen.getByRole("status")).toHaveTextContent(/Scanned \d/); + }); + + it("cannot start a scan while a reclaim is still running", async () => { + // A scan launched on top of a mutation measures a daemon that is being + // changed underneath it — the hook can only throw such a result away, so + // the seconds are better not spent. + let finish: (value: unknown) => void = () => {}; + reclaim.mockReturnValue(new Promise((r) => (finish = r))); + + await renderAndScan(); + await screen.findByTestId("disk-safe-bucket"); + fireEvent.click(screen.getAllByRole("checkbox")[0]); + fireEvent.click(screen.getByRole("button", { name: "Reclaim" })); + + await waitFor(() => + expect(screen.getByRole("button", { name: "Scan again" })).toBeDisabled(), + ); + + await act(async () => { + finish({ results: [], total_freed_bytes: 0 }); + }); + expect(screen.getByRole("button", { name: "Scan again" })).toBeEnabled(); + }); + + it("gives the unknown layer count its explanation without a hover", async () => { + // The tooltip portals a div with no role and no `aria-describedby`, and + // wrapped around children it has no focus handlers either — so without the + // sr-only copy "unknown" reads as a bug to everyone not using a mouse. + getDockerDiskUsage.mockResolvedValue( + report({ projects: [row({ base_lineage_known: false, snapshot_commit_layers: 17 })] }), + ); + await renderAndScan(); + const projectRow = await screen.findByTestId("disk-row-p-whp"); + expect(projectRow.textContent).toMatch(/predates the base-image label/); + expect(projectRow.textContent).toMatch(/Migrating it to the current base restores the count/); + }); + it("surfaces a scan failure as an alert", async () => { getDockerDiskUsage.mockRejectedValue("Could not read Docker disk usage: no such host"); render(); diff --git a/app/src/components/settings/DiskSettings.tsx b/app/src/components/settings/DiskSettings.tsx index 720fcfc..cc21ba4 100644 --- a/app/src/components/settings/DiskSettings.tsx +++ b/app/src/components/settings/DiskSettings.tsx @@ -54,6 +54,13 @@ export default function DiskSettings() { const [ticked, setTicked] = useState>(new Set()); const [confirming, setConfirming] = useState(null); const [destroying, setDestroying] = useState(null); + // A dialog whose action failed stays open and says so *inside itself*. The + // hook's `error` is rendered at the top of a panel that is metres of scroll + // long, so a user who reached a project row through the table would have + // watched the dialog vanish and seen nothing take its place. This flag is + // what distinguishes "this dialog's action just failed" from a stale scan + // error that happened to still be sitting in `error` when it opened. + const [actionFailed, setActionFailed] = useState(false); // The plan is dropped after any reclaim, so a tick can never outlive the row // it was made against and be re-fired at an object that is already gone. @@ -68,6 +75,25 @@ export default function DiskSettings() { ); const selectedBytes = selected.reduce((sum, i) => sum + i.bytes, 0); + // Opening or closing either dialog clears the in-dialog failure with it, so + // one never starts out showing the previous attempt's error. + const openConfirming = (item: ReclaimItem) => { + setConfirming(item); + setActionFailed(false); + }; + const openDestroying = (item: DestructiveItem) => { + setDestroying(item); + setActionFailed(false); + }; + const closeConfirming = () => { + setConfirming(null); + setActionFailed(false); + }; + const closeDestroying = () => { + setDestroying(null); + setActionFailed(false); + }; + const toggle = (item: ReclaimItem) => { setTicked((prev) => { const next = new Set(prev); @@ -78,6 +104,10 @@ export default function DiskSettings() { }); }; + // Counted from the per-result list rather than from a flag: a reclaim of + // five targets can come back with two failures and a real byte total. + const failedCount = outcome?.results.filter((r) => !r.ok).length ?? 0; + const tone: StatusTone = scanning ? "unknown" : report ? "ok" : "off"; const statusLabel = scanning ? "Scanning" @@ -99,10 +129,21 @@ export default function DiskSettings() { {/* --- Scan --------------------------------------------------------- */}
- - + {/* The status flips between "Scanning", "Scanned HH:MM:SS" and "Not + scanned" with no other signal. The live region is mounted here + unconditionally — wrapping it around the indicator only once there + is something to say would make the region *appear* already + populated, which is the one shape assistive tech does not announce. */} + + + Reads the whole Docker store; takes a few seconds on a large one. @@ -161,7 +202,7 @@ export default function DiskSettings() { @@ -196,7 +237,10 @@ export default function DiskSettings() {
Build cache — whole daemon, not just Triple-C{" "} - + {/* Live information about where the figure came from, not a + disabled control — `--text-disabled` is ~4.1:1 and fails AA + at this size. */} + (via {report.build_cache.source})
@@ -400,7 +444,7 @@ export default function DiskSettings() { @@ -434,9 +478,18 @@ export default function DiskSettings() { data-testid="disk-outcome" >
+ {/* The headline has to carry the failure in words. A partial + reclaim that freed something still has a byte figure worth + printing, so the count is appended to it rather than replacing + it — and the per-result lines below say *which* ones and why, + so this stops at how many. */} r.ok) ? "ok" : "error"} - label={`Reclaimed ${formatBytes(outcome.total_freed_bytes)}`} + tone={failedCount === 0 ? "ok" : "error"} + label={ + failedCount === 0 + ? `Reclaimed ${formatBytes(outcome.total_freed_bytes)}` + : `Reclaimed ${formatBytes(outcome.total_freed_bytes)} — ${failedCount} of ${outcome.results.length} failed` + } className="text-xs" /> ))} diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 166dd21..3d646d3 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -1,7 +1,24 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { render, fireEvent, cleanup } from "@testing-library/react"; +import { render, fireEvent, cleanup, act } from "@testing-library/react"; import TerminalView, { supersedes } from "./TerminalView"; import { useAppState } from "../../store/appState"; +import { uploadHostFileToTerminal } from "../../lib/tauri-commands"; + +/** + * The window-wide native drag-drop listener, captured at registration. + * + * Tauri routes *every* file drop to *every* listener, which is the whole reason + * `TerminalView` hit-tests one — so a test that wants to know what the hit test + * decides has to be able to fire the event itself. + */ +const dragDrop = vi.hoisted(() => ({ + handler: null as null | ((event: unknown) => unknown), +})); + +/** The `terminal-output-{id}` listeners, so a test can be the PTY. */ +const ptyOutput = vi.hoisted(() => ({ + listeners: new Map void>(), +})); /** * Shift+Enter has to reach the container as ESC+CR. @@ -30,7 +47,10 @@ vi.mock("../../lib/tauri-commands", () => ({ })); vi.mock("@tauri-apps/api/event", () => ({ - listen: vi.fn(async () => () => {}), + listen: async (event: string, cb: (e: { payload: number[] }) => void) => { + ptyOutput.listeners.set(event, cb); + return () => ptyOutput.listeners.delete(event); + }, })); vi.mock("@tauri-apps/plugin-opener", () => ({ @@ -38,7 +58,14 @@ vi.mock("@tauri-apps/plugin-opener", () => ({ })); vi.mock("@tauri-apps/api/webview", () => ({ - getCurrentWebview: () => ({ onDragDropEvent: vi.fn(async () => () => {}) }), + getCurrentWebview: () => ({ + onDragDropEvent: async (cb: (event: unknown) => unknown) => { + dragDrop.handler = cb; + return () => { + dragDrop.handler = null; + }; + }, + }), })); /** jsdom has no ResizeObserver, and the mount effect installs one. */ @@ -96,6 +123,11 @@ beforeEach(() => { }), ); terminalInput.mockClear(); + vi.mocked(uploadHostFileToTerminal).mockClear(); + vi.mocked(uploadHostFileToTerminal).mockResolvedValue("/workspace/api/dropped.txt"); + dragDrop.handler = null; + ptyOutput.listeners.clear(); + document.body.innerHTML = ""; useAppState.setState({ sessions: [] }); }); @@ -200,6 +232,14 @@ describe("supersedes — who owns the prompt slot", () => { expect(supersedes(relay(COMPLETE), guess(TRUNCATED))).toBe(true); }); + it("refuses to let a truncated guess replace another guess it truncates", () => { + // The same rule one rank down. Both are scrapes of the same repainting + // frame, so recency says the newer one wins and recency is wrong: a + // repaint that lands a *shorter* view of the link already on screen is + // showing less of it, not something new. + expect(supersedes(guess(TRUNCATED), guess(COMPLETE))).toBe(false); + }); + it("lets a scraped candidate grow into the complete link", () => { // A repaint can land the truncated copy first. Extending it is safe: a // longer string with the same prefix has the same origin. @@ -222,3 +262,162 @@ describe("supersedes — who owns the prompt slot", () => { ).toBe(true); }); }); + +describe("TerminalView — where a dropped file lands", () => { + /** Mount, let the async drag-drop registration settle, and give the pane a + * rect — jsdom has no layout, so every element is 0×0 and would be rejected + * as a hidden pane. */ + async function mountWithLayout() { + const view = mountSession("bash"); + await act(async () => {}); + const pane = view.container.querySelector(".xterm")?.parentElement; + if (!pane) throw new Error("terminal host element not found"); + pane.getBoundingClientRect = () => + ({ + left: 0, + top: 0, + right: 800, + bottom: 600, + width: 800, + height: 600, + x: 0, + y: 0, + toJSON: () => ({}), + }) as DOMRect; + return view; + } + + async function drop(x: number, y: number) { + if (!dragDrop.handler) throw new Error("no drag-drop listener registered"); + await act(async () => { + await dragDrop.handler!({ + payload: { type: "drop", position: { x, y }, paths: ["/host/dropped.txt"] }, + }); + }); + } + + it("uploads a file dropped onto the pane", async () => { + await mountWithLayout(); + await drop(400, 300); + expect(vi.mocked(uploadHostFileToTerminal)).toHaveBeenCalledWith( + "s1", + "/host/dropped.txt", + ); + }); + + it("ignores a drop that lands outside the pane", async () => { + await mountWithLayout(); + await drop(4000, 300); + expect(vi.mocked(uploadHostFileToTerminal)).not.toHaveBeenCalled(); + }); + + it("ignores a drop released onto an open modal", async () => { + // The hit test used to be purely geometric, and a `Modal` is a + // `fixed inset-0 z-50` portal painted *over* the whole window — so the pane + // underneath still had its rect and happily uploaded the file into the + // directory the dialog was covering. Same for the shutdown overlay, which is + // up precisely while nothing should be accepting work. + await mountWithLayout(); + const dialog = document.createElement("div"); + dialog.setAttribute("role", "dialog"); + dialog.setAttribute("aria-modal", "true"); + document.body.appendChild(dialog); + + await drop(400, 300); + + expect(vi.mocked(uploadHostFileToTerminal)).not.toHaveBeenCalled(); + + // …and it is the modal, not the mount, that is refusing: close it and the + // very same drop goes through. + dialog.remove(); + await drop(400, 300); + expect(vi.mocked(uploadHostFileToTerminal)).toHaveBeenCalledTimes(1); + }); +}); + +describe("TerminalView — reaching the URL prompt without a mouse", () => { + // This toast is the only route to completing a sign-in started in a terminal. + // It used to be mouse-only: nothing moved focus to it, nothing dismissed it + // from the keyboard, and xterm's helper textarea eats Tab, so its buttons + // could not be reached at all. + const SIGN_IN = + "https://claude.ai/oauth/authorize?code=true&client_id=abc&response_type=code"; + + /** What `container/triple-c-open` writes to its controlling terminal. */ + function relaySequence(url: string): number[] { + const payload = btoa(url); + return Array.from( + new TextEncoder().encode(`\x1b]7777;open;${payload}\x07`), + ); + } + + /** Mount, and let the container ask for a URL to be opened. */ + async function mountWithPrompt() { + const view = mountSession("claude"); + await act(async () => {}); + const emit = ptyOutput.listeners.get("terminal-output-s1"); + if (!emit) throw new Error("no terminal-output listener registered"); + await act(async () => { + emit({ payload: relaySequence(SIGN_IN) }); + // xterm parses on its own write queue. + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + }); + return view; + } + + function primaryAction(): HTMLElement { + const el = document.querySelector('[data-url-toast-primary="true"]'); + if (!el) throw new Error("toast default action not found"); + return el; + } + + it("does not take focus away from the terminal when the prompt appears", async () => { + // Deliberate: the terminal is live, and the default action opens a URL the + // *container* chose. A focused button is one stray Enter from doing it. + const { container } = await mountWithPrompt(); + expect(document.querySelector('[data-testid="url-toast"]')).not.toBeNull(); + expect(document.activeElement).toBe(helperTextarea(container)); + }); + + it("jumps to the default action on Ctrl+Shift+O", async () => { + const { container } = await mountWithPrompt(); + + fireEvent.keyDown(helperTextarea(container), { + key: "O", + ctrlKey: true, + shiftKey: true, + }); + + expect(document.activeElement).toBe(primaryAction()); + }); + + it("dismisses on Escape and hands focus back to the terminal", async () => { + // Not back to `document.body`, where the next keystroke goes nowhere. + const { container } = await mountWithPrompt(); + fireEvent.keyDown(helperTextarea(container), { + key: "O", + ctrlKey: true, + shiftKey: true, + }); + + fireEvent.keyDown(document.activeElement!, { key: "Escape" }); + + expect(document.querySelector('[data-testid="url-toast"]')).toBeNull(); + expect(document.activeElement).toBe(helperTextarea(container)); + }); + + it("leaves Ctrl+Shift+O to the terminal when there is no prompt", async () => { + const { container } = mountSession("claude"); + await act(async () => {}); + const before = document.activeElement; + + fireEvent.keyDown(helperTextarea(container), { + key: "O", + ctrlKey: true, + shiftKey: true, + }); + + expect(document.activeElement).toBe(before); + }); +}); diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 0ffc40d..149b6c3 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -21,7 +21,12 @@ import { parseUrlRelayOsc, sanitizeRelayUrl, } from "../../lib/urlRelay"; -import UrlToast from "./UrlToast"; +import { isDropTarget } from "../../lib/dropTarget"; +import UrlToast, { + URL_TOAST_PRIMARY_SELECTOR, + URL_TOAST_SELECTOR, + URL_TOAST_SHORTCUT, +} from "./UrlToast"; import { trimSelection } from "./trimSelection"; import TerminalContextMenu from "./TerminalContextMenu"; @@ -131,6 +136,26 @@ export default function TerminalView({ sessionId, active }: Props) { } | null>(null); const promptSeqRef = useRef(0); const relayLimiterRef = useRef(new RelayRateLimiter()); + // Read by the long-lived keyboard listener below, which is registered once + // and would otherwise close over the prompt as it was at mount. + const urlPromptRef = useRef<{ url: string } | null>(null); + + /** + * Empty the prompt slot, and put focus somewhere real if it was inside the + * toast. + * + * The toast never *takes* focus — see the note in `UrlToast` — but a keyboard + * user who jumped into it with {@link URL_TOAST_SHORTCUT} is standing on a + * node that is about to unmount, and React does not rehome focus: it lands on + * `document.body`, where the terminal receives nothing and the next keystroke + * goes nowhere. Every route out of the toast goes through here for that + * reason — Open, In container, ✕, Escape and the auto-dismiss alike. + */ + const dismissUrlPrompt = useCallback(() => { + const wasInside = !!document.activeElement?.closest(URL_TOAST_SELECTOR); + setUrlPrompt(null); + if (wasInside) termRef.current?.focus(); + }, []); /** * The only writer of the prompt slot. Re-validates whatever the caller @@ -158,6 +183,38 @@ export default function TerminalView({ sessionId, active }: Props) { }, [], ); + useEffect(() => { + urlPromptRef.current = urlPrompt; + }, [urlPrompt]); + + /** + * The keyboard route into the toast. + * + * Registered on `document` in the capture phase for the same reason + * `useKeyboardShortcuts` does it there: xterm would otherwise forward the + * chord to the shell. It is *not* added to that hook because the target is + * this pane's own toast — the hook has no way to name it, and only one pane + * is on screen at a time, which is what `activeRef` checks. + * + * Nothing is swallowed unless there is a prompt to jump to, so Ctrl+Shift+O + * reaches the terminal untouched the rest of the time. + */ + useEffect(() => { + const onKeyDown = (e: KeyboardEvent) => { + if (!e.ctrlKey || !e.shiftKey || e.altKey || e.metaKey) return; + if (e.key !== "o" && e.key !== "O") return; + if (!activeRef.current || !urlPromptRef.current) return; + const primary = terminalContainerRef.current?.querySelector( + `${URL_TOAST_SELECTOR} ${URL_TOAST_PRIMARY_SELECTOR}`, + ); + if (!primary) return; + e.preventDefault(); + e.stopPropagation(); + primary.focus(); + }; + document.addEventListener("keydown", onKeyDown, true); + return () => document.removeEventListener("keydown", onKeyDown, true); + }, []); const [imagePasteMsg, setImagePasteMsg] = useState(null); const [isAtBottom, setIsAtBottom] = useState(true); const [isAutoFollow, setIsAutoFollow] = useState(true); @@ -177,24 +234,21 @@ export default function TerminalView({ sessionId, active }: Props) { // in-container paths typed into the prompt so Claude Code can read them. // Tauri intercepts OS file drops at the webview level, so we use // onDragDropEvent (HTML5 ondrop on the element wouldn't expose file paths). - // The listener is window-wide, so we route purely by a hit-test against this - // terminal's bounds: the pane the drop lands on handles it. Inactive panes are - // `display:none` (zero-size rect) so they never match — this works for the - // current tabbed layout and would also do the right thing with split panes. + // + // The listener is window-wide, so every pane decides for itself whether a + // drop was meant for it. `isDropTarget` is that decision, shared with the + // Files pane: the physical-pixel position ÷ `devicePixelRatio` against this + // pane's rect (a hidden pane is `display:none`, so its zero-size rect is what + // stops two panes both claiming the drop), plus z-order — which a rect alone + // cannot see. An open `Modal` is a `fixed inset-0` portal painted *over* the + // window and the pane underneath still has its rect, so the geometric test + // that used to live here uploaded files into the directory a dialog was + // covering. Same for the shutdown overlay, which is on screen precisely while + // nothing should be accepting work at all. useEffect(() => { let unlisten: (() => void) | undefined; let cancelled = false; - const insideThisTerminal = (pos: { x: number; y: number }): boolean => { - const rect = containerRef.current?.getBoundingClientRect(); - // A hidden (display:none) pane has a zero-size rect — never a drop target. - if (!rect || rect.width === 0 || rect.height === 0) return false; - const dpr = window.devicePixelRatio || 1; - const x = pos.x / dpr; - const y = pos.y / dpr; - return x >= rect.left && x <= rect.right && y >= rect.top && y <= rect.bottom; - }; - // Always single-quote: a dropped filename can contain shell metacharacters // ($(), &&, ', spaces) even with no whitespace, and this path is typed into // a live shell. Single-quoting with '\'' escaping neutralizes all of them. @@ -203,7 +257,7 @@ export default function TerminalView({ sessionId, active }: Props) { (async () => { const un = await getCurrentWebview().onDragDropEvent(async (event) => { if (event.payload.type !== "drop") return; - if (!insideThisTerminal(event.payload.position)) return; + if (!isDropTarget(containerRef.current, event.payload.position)) return; const paths = event.payload.paths ?? []; if (paths.length === 0) return; @@ -391,6 +445,10 @@ export default function TerminalView({ sessionId, active }: Props) { console.warn("URL relay: rate-limited", url); return true; } + // Exact by construction (base64 over OSC 7777), and the detector never + // sees it — so tell it, or a truncated scrape of the same link could + // still fill the slot once this prompt is dismissed. + detectorRef.current?.noteExactUrl(url); promptUrl(url, "Container asked to open a URL", "relay"); return true; }); @@ -619,12 +677,19 @@ export default function TerminalView({ sessionId, active }: Props) { } }, [active]); - // Auto-dismiss toast after 30 seconds + // Auto-dismiss toast after 30 seconds — unless the user is standing in it. + // A keyboard user who has just jumped into the toast is mid-decision, and + // pulling it out from under them costs them the only route to finishing a + // sign-in. It goes when they act on it, which is the same thing a mouse user + // does by clicking. useEffect(() => { if (!urlPrompt) return; - const timer = setTimeout(() => setUrlPrompt(null), 30_000); + const timer = setTimeout(() => { + if (document.activeElement?.closest(URL_TOAST_SELECTOR)) return; + dismissUrlPrompt(); + }, 30_000); return () => clearTimeout(timer); - }, [urlPrompt]); + }, [urlPrompt, dismissUrlPrompt]); // Auto-dismiss image paste message after 3 seconds useEffect(() => { @@ -639,13 +704,13 @@ export default function TerminalView({ sessionId, active }: Props) { // sanitizes, so this can only fail if that invariant is broken — which is // precisely when it matters that the last thing before `openUrl` checks. const safe = sanitizeRelayUrl(urlPrompt.url); - setUrlPrompt(null); + dismissUrlPrompt(); if (!safe) { console.warn("Refusing to open a URL that failed validation"); return; } openUrl(safe).catch((e) => console.error("Failed to open URL:", e)); - }, [urlPrompt]); + }, [urlPrompt, dismissUrlPrompt]); /** * Open the prompted URL in the container's own browser instead of the host's. @@ -658,7 +723,7 @@ export default function TerminalView({ sessionId, active }: Props) { const handleOpenUrlInContainer = useCallback(() => { if (!urlPrompt) return; const safe = sanitizeRelayUrl(urlPrompt.url); - setUrlPrompt(null); + dismissUrlPrompt(); if (!safe) { console.warn("Refusing to open a URL that failed validation"); return; @@ -690,7 +755,7 @@ export default function TerminalView({ sessionId, active }: Props) { detail: String(e), }), ); - }, [urlPrompt, projectId]); + }, [urlPrompt, projectId, dismissUrlPrompt]); const handleScrollToBottom = useCallback(() => { const term = termRef.current; @@ -770,7 +835,7 @@ export default function TerminalView({ sessionId, active }: Props) { label={urlPrompt.label} onOpen={handleOpenUrl} onOpenInContainer={handleOpenUrlInContainer} - onDismiss={() => setUrlPrompt(null)} + onDismiss={dismissUrlPrompt} /> )} {imagePasteMsg && ( diff --git a/app/src/components/terminal/UrlToast.test.tsx b/app/src/components/terminal/UrlToast.test.tsx index a991ef6..ad705d0 100644 --- a/app/src/components/terminal/UrlToast.test.tsx +++ b/app/src/components/terminal/UrlToast.test.tsx @@ -1,6 +1,6 @@ import { describe, it, expect, vi } from "vitest"; -import { render, screen } from "@testing-library/react"; -import UrlToast from "./UrlToast"; +import { fireEvent, render, screen } from "@testing-library/react"; +import UrlToast, { URL_TOAST_PRIMARY_SELECTOR } from "./UrlToast"; /** * The toast is the *only* thing standing between a container-chosen URL and @@ -59,6 +59,111 @@ describe("UrlToast", () => { expect(onOpen).toHaveBeenCalledTimes(1); }); + describe("keyboard", () => { + // This toast is the only route to completing a sign-in started in a + // terminal, and xterm's helper textarea swallows Tab — so without these it + // is unreachable for a keyboard-only user. + const SIGN_IN = + "https://claude.ai/oauth/authorize?code=true&client_id=abc&response_type=code"; + + it("does not take focus from the live terminal when it appears", () => { + // Deliberate. The user may be mid-command, and the default action opens a + // URL the *container* chose — a focused button is one stray Enter away + // from doing it. The shortcut hint below is what makes that affordable. + render( + , + ); + expect(document.activeElement).toBe(document.body); + }); + + it("says how to reach it, since nothing announces a shortcut by itself", () => { + render( + , + ); + expect(screen.getByTestId("url-toast-shortcut")).toHaveTextContent( + "Ctrl+Shift+O", + ); + }); + + it("marks the default action so the shortcut has somewhere to land", () => { + // Which button that is depends on the URL, so the marker moves with the + // decision rather than the owner having to repeat it. + const { rerender } = render( + , + ); + expect( + document.querySelector(URL_TOAST_PRIMARY_SELECTOR), + ).toHaveTextContent("Open"); + + rerender( + , + ); + expect( + document.querySelector(URL_TOAST_PRIMARY_SELECTOR), + ).toHaveTextContent("In container"); + }); + + it("dismisses on Escape from anywhere inside it", () => { + const onDismiss = vi.fn(); + render( + , + ); + fireEvent.keyDown(screen.getByRole("button", { name: "Open" }), { + key: "Escape", + }); + expect(onDismiss).toHaveBeenCalledTimes(1); + }); + + it("does not answer Escape pressed outside it", () => { + // Escape belongs to whatever is running in the terminal — vim, above all. + // A document-level binding would break it for everyone who never looked + // at this toast. + const onDismiss = vi.fn(); + render( + , + ); + fireEvent.keyDown(document.body, { key: "Escape" }); + expect(onDismiss).not.toHaveBeenCalled(); + }); + + it("gives every action a real button, so Tab reaches all three", () => { + render( + , + ); + const names = screen + .getAllByRole("button") + .map((b) => b.getAttribute("aria-label") ?? b.textContent); + expect(names).toEqual(["In container", "Open", "Dismiss"]); + // Nothing is taken out of the tab order. + for (const b of screen.getAllByRole("button")) { + expect(b).not.toHaveAttribute("tabindex", "-1"); + } + }); + }); + describe("Anthropic sign-in links", () => { // The callback listener a `claude login` is waiting on is *inside* the // container. Sending the user to their host browser completes the sign-in diff --git a/app/src/components/terminal/UrlToast.tsx b/app/src/components/terminal/UrlToast.tsx index 0c17737..ddad469 100644 --- a/app/src/components/terminal/UrlToast.tsx +++ b/app/src/components/terminal/UrlToast.tsx @@ -1,5 +1,32 @@ -import type { CSSProperties, MouseEvent } from "react"; +import type { KeyboardEvent } from "react"; import { isAnthropicSignInUrl, urlOrigin } from "../../lib/urlRelay"; +import Button from "../ui/Button"; + +/** + * Marks the toast's subtree. `TerminalView` uses it to answer "is focus inside + * the thing I am about to unmount?", which is what decides whether dismissing + * has to hand focus back to the terminal. + */ +export const URL_TOAST_SELECTOR = '[data-testid="url-toast"]'; + +/** + * The chord that jumps from the terminal into this toast. + * + * Bound in `TerminalView` on `document` in the capture phase, the same way + * `useKeyboardShortcuts` binds the app's other chords, because xterm would + * otherwise forward it to the shell. Shift is what keeps it clear of the + * terminal: plain Ctrl+O is readline's `operate-and-get-next`. + */ +export const URL_TOAST_SHORTCUT = "Ctrl+Shift+O"; + +/** + * Marks the *default* action inside the toast, so the owner can put focus + * there without a ref threaded through `ui/Button` — which is a plain function + * component and not this file's to change. Which button it is depends on the + * URL (see the sign-in note below), so the attribute moves with the decision + * rather than the caller having to repeat it. + */ +export const URL_TOAST_PRIMARY_SELECTOR = '[data-url-toast-primary="true"]'; interface Props { /** Already validated by `sanitizeRelayUrl` — this component never opens it. */ @@ -41,6 +68,28 @@ interface Props { * with no host round trip and no auth bridge, so it leads — and the host button * stays, because a user who has the auth bridge on, or who wants their existing * browser session, still needs it. + * + * ## Reachable without a mouse, and it does not take focus to manage it + * + * This toast is the only route to completing a sign-in started in a terminal, + * and it used to be mouse-only: xterm's helper textarea swallows Tab, so there + * was no way to reach these buttons at all from the keyboard. + * + * The obvious fix — focus the default action when the toast appears — was + * rejected on two counts. The terminal underneath is *live*: the user may be + * mid-command, and every keystroke after the steal would go to a button instead + * of the shell. Worse, the default action opens a URL chosen by the untrusted + * side of the sandbox, and a focused button is one stray Space or Enter away + * from doing it. This prompt exists precisely to make that a deliberate act. + * + * So focus stays where the user put it and the toast is reachable on demand: + * {@link URL_TOAST_SHORTCUT} jumps to the default action (the hint is on + * screen, next to the label, because a shortcut nobody is told about is not a + * route), Tab then moves between the actions normally — this subtree is not + * inside xterm — and Escape dismisses. Escape is handled *here*, on the + * toast's own subtree, rather than globally: Escape belongs to whatever is + * running in the terminal, and a document-level binding for it would break vim + * for everyone who never looked at this toast. */ export default function UrlToast({ url, @@ -55,82 +104,56 @@ export default function UrlToast({ // host button is the only action there is, so it stays primary. const signIn = !!onOpenInContainer && isAnthropicSignInUrl(url); - // Filled uses `--accent-emphasis`, never `--accent` — the latter is the + // `Button` already owns the filled/outlined variants — including the rule + // that filled uses `--accent-emphasis` and never `--accent`, which is the // foreground/link accent and fails WCAG AA behind white text. - const primaryStyle: CSSProperties = { - padding: "4px 12px", - fontSize: 12, - fontWeight: 600, - color: "#fff", - background: "var(--accent-emphasis)", - border: "1px solid transparent", - borderRadius: 4, - cursor: "pointer", - whiteSpace: "nowrap", - flexShrink: 0, - }; - const secondaryStyle: CSSProperties = { - padding: "4px 10px", - fontSize: 12, - fontWeight: 600, - color: "var(--text-primary)", - background: "transparent", - border: "1px solid var(--border-color)", - borderRadius: 4, - cursor: "pointer", - whiteSpace: "nowrap", - flexShrink: 0, - }; - - /** Hover feedback for whichever button is currently the filled one. */ - const hover = (primary: boolean) => - primary - ? { - onMouseEnter: (e: MouseEvent) => - (e.currentTarget.style.background = "var(--accent-emphasis-hover)"), - onMouseLeave: (e: MouseEvent) => - (e.currentTarget.style.background = "var(--accent-emphasis)"), - } - : { - onMouseEnter: (e: MouseEvent) => - (e.currentTarget.style.background = "var(--bg-tertiary)"), - onMouseLeave: (e: MouseEvent) => - (e.currentTarget.style.background = "transparent"), - }; - const hostButton = ( - + ); const containerButton = onOpenInContainer && ( // A sign-in completed in the *container's* browser lands its callback on // the container's own loopback, which is where the tool waiting for it is // listening — no host round trip, no auth bridge. - + ); + const onKeyDown = (e: KeyboardEvent) => { + if (e.key !== "Escape") return; + // Scoped to this subtree, so the terminal's own Escape is untouched. + e.preventDefault(); + e.stopPropagation(); + onDismiss(); + }; + return (
@@ -157,6 +180,11 @@ export default function UrlToast({ }} > {label} + {" · "} + + {URL_TOAST_SHORTCUT} + {" "} + to reach the buttons, Esc to dismiss
)} - +
); } diff --git a/app/src/components/ui/TypedConfirmModal.test.tsx b/app/src/components/ui/TypedConfirmModal.test.tsx index bacaec3..851043d 100644 --- a/app/src/components/ui/TypedConfirmModal.test.tsx +++ b/app/src/components/ui/TypedConfirmModal.test.tsx @@ -103,6 +103,19 @@ describe("TypedConfirmModal", () => { expect(onConfirm).not.toHaveBeenCalled(); }); + it("carries a failed attempt inside the dialog, as an alert", () => { + // The caller keeps this dialog open when the deletion fails, because the + // panel behind it is several screens long and its error line sits at the + // top — nowhere near the row this was opened from. + renderModal({ error: "volume triple-c-home-p-whp is in use by a running container" }); + expect(screen.getByRole("alert")).toHaveTextContent(/in use by a running container/); + }); + + it("says nothing about failure when there has been none", () => { + renderModal(); + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + }); + it("cannot be satisfied by an empty box when there is no name to type", () => { const { confirm } = renderModal({ expected: "" }); expect(confirm).toBeDisabled(); diff --git a/app/src/components/ui/TypedConfirmModal.tsx b/app/src/components/ui/TypedConfirmModal.tsx index 8c7f036..40ed6ee 100644 --- a/app/src/components/ui/TypedConfirmModal.tsx +++ b/app/src/components/ui/TypedConfirmModal.tsx @@ -14,6 +14,13 @@ interface Props { onConfirm: (typed: string) => void; onCancel: () => void; busy?: boolean; + /** + * Why the last attempt did not happen. The caller keeps the dialog open when + * its action fails, so the failure has to be readable *here* — the panel + * behind this one is several screens long and its error line is at the top + * of it, which is not where the user is looking. + */ + error?: string | null; } /** @@ -44,6 +51,7 @@ export default function TypedConfirmModal({ onConfirm, onCancel, busy = false, + error = null, }: Props) { const [typed, setTyped] = useState(""); const inputRef = useRef(null); @@ -104,12 +112,22 @@ export default function TypedConfirmModal({ {matches ? ( Name matches. ) : ( - + // Not disabled content — the gate is live and waiting on the + // user. `--text-disabled` is ~4.1:1 and fails AA at 12px. + Waiting for the exact project name. )}

+ {error && ( + // Rendered last, next to the button that was just pressed, and as an + // `alert` so it is announced on arrival rather than waiting to be + // found. +

+ {error} +

+ )}
); diff --git a/app/src/hooks/useDiskUsage.test.tsx b/app/src/hooks/useDiskUsage.test.tsx index bda6e72..2ddfbf9 100644 --- a/app/src/hooks/useDiskUsage.test.tsx +++ b/app/src/hooks/useDiskUsage.test.tsx @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { act, renderHook, waitFor } from "@testing-library/react"; -import { useDiskUsage } from "./useDiskUsage"; +import { useDiskUsage, type DiskUsageState } from "./useDiskUsage"; import type { DiskUsageReport } from "../lib/types"; const getDockerDiskUsage = vi.fn(); @@ -226,6 +226,157 @@ describe("useDiskUsage", () => { expect(result.current.outcome?.results[0].message).toMatch(/3 were left alone/); }); + // ------------------------------------------------------------------------- + // The scan-versus-mutation race + // ------------------------------------------------------------------------- + + it("throws away a scan that a reclaim overtook", async () => { + // The live race the generation counter used to miss entirely. A scan takes + // seconds and does not set `working`, so nothing stopped the user + // reclaiming on top of one — and when the scan landed it repainted the + // pre-reclaim report *and* a fresh, clickable plan listing objects the + // reclaim had just deleted. + let resolveScan: (value: DiskUsageReport) => void = () => {}; + getDockerDiskUsage.mockReturnValueOnce( + new Promise((r) => { + resolveScan = r; + }), + ); + + const { result } = renderHook(() => useDiskUsage()); + let inFlight: Promise = Promise.resolve(); + act(() => { + inFlight = result.current.scan(); + }); + expect(result.current.scanning).toBe(true); + + await act(async () => { + await result.current.runReclaim([{ kind: "dangling_snapshots" }]); + }); + expect(result.current.plan).toBeNull(); + + // The overtaken scan finishes last, and must land nothing at all. + await act(async () => { + resolveScan(report("measured before the reclaim")); + await inFlight; + }); + expect(result.current.report).toBeNull(); + expect(result.current.plan).toBeNull(); + // It does not even get as far as re-planning: a plan built from a report + // this stale is the clickable half of the bug. + expect(listReclaimable).not.toHaveBeenCalled(); + }); + + it("does not strand `scanning` when a mutation retires the scan", async () => { + // `scanning` is cleared against the newest *scan*, not the newest + // generation — a mutation bumps the generation without starting a scan, so + // guarding on that would leave the button reading "Scanning…" forever. + let resolveScan: (value: DiskUsageReport) => void = () => {}; + getDockerDiskUsage.mockReturnValueOnce( + new Promise((r) => { + resolveScan = r; + }), + ); + + const { result } = renderHook(() => useDiskUsage()); + let inFlight: Promise = Promise.resolve(); + act(() => { + inFlight = result.current.scan(); + }); + await act(async () => { + await result.current.runReclaim([{ kind: "dangling_snapshots" }]); + }); + await act(async () => { + resolveScan(report("stale")); + await inFlight; + }); + expect(result.current.scanning).toBe(false); + }); + + it("retires an in-flight scan for a destroy and a sweep too", async () => { + // Every mutation invalidates a measurement, not just the bulk one. + destroyProjectDiskObject.mockResolvedValue({ + target: null, + destroyed: { kind: "home_volume", project_id: "p1" }, + ok: true, + freed_bytes: 1, + projected_bytes: null, + message: "gone", + }); + sweepOrphanedSnapshots.mockResolvedValue({ + removed: [], + reclaimed_bytes: 0, + in_use: 0, + failed: [], + unavailable: null, + }); + + for (const mutate of [ + (r: DiskUsageState) => r.destroy({ kind: "home_volume", project_id: "p1" }, "whp"), + (r: DiskUsageState) => r.runSweep(), + ]) { + let resolveScan: (value: DiskUsageReport) => void = () => {}; + getDockerDiskUsage.mockReturnValueOnce( + new Promise((r) => { + resolveScan = r; + }), + ); + const { result } = renderHook(() => useDiskUsage()); + let inFlight: Promise = Promise.resolve(); + act(() => { + inFlight = result.current.scan(); + }); + await act(async () => { + await mutate(result.current); + }); + await act(async () => { + resolveScan(report("stale")); + await inFlight; + }); + expect(result.current.report).toBeNull(); + expect(result.current.plan).toBeNull(); + expect(result.current.scanning).toBe(false); + } + }); + + // ------------------------------------------------------------------------- + // Reporting failure back to the caller + // ------------------------------------------------------------------------- + + it("tells the caller a reclaim failed instead of only swallowing it into `error`", async () => { + // The confirmation dialogs close on completion. Without a return value + // they closed on failure too, leaving the error at the top of a panel the + // user had scrolled well past. + reclaim.mockRejectedValueOnce("compaction failed: no space left on device"); + const { result } = renderHook(() => useDiskUsage()); + let ok: boolean | undefined; + await act(async () => { + ok = await result.current.runReclaim([{ kind: "compact_snapshot", project_id: "p1" }]); + }); + expect(ok).toBe(false); + expect(result.current.error).toMatch(/no space left on device/); + }); + + it("tells the caller a destroy failed", async () => { + destroyProjectDiskObject.mockRejectedValueOnce("volume is in use by a running container"); + const { result } = renderHook(() => useDiskUsage()); + let ok: boolean | undefined; + await act(async () => { + ok = await result.current.destroy({ kind: "home_volume", project_id: "p1" }, "whp"); + }); + expect(ok).toBe(false); + expect(result.current.error).toMatch(/in use by a running container/); + }); + + it("reports success when the call came back", async () => { + const { result } = renderHook(() => useDiskUsage()); + let ok: boolean | undefined; + await act(async () => { + ok = await result.current.runReclaim([{ kind: "dangling_snapshots" }]); + }); + expect(ok).toBe(true); + }); + it("treats an unreachable daemon in the sweep report as an error", async () => { sweepOrphanedSnapshots.mockResolvedValue({ removed: [], diff --git a/app/src/hooks/useDiskUsage.ts b/app/src/hooks/useDiskUsage.ts index f82eb19..3f00330 100644 --- a/app/src/hooks/useDiskUsage.ts +++ b/app/src/hooks/useDiskUsage.ts @@ -32,9 +32,24 @@ import type { * A user who hits Scan twice can have two `df()` calls in flight, and they can * land out of order — the second one is not necessarily slower. Every async * write in `scan` checks it is still the newest before it lands, the same - * pattern `useContainerMigration` uses. `runReclaim` and `destroy` do not need - * it: the UI disables their buttons while `working` is set, so there is never - * a second one to race. + * pattern `useContainerMigration` uses. + * + * The race that actually bites, though, is not scan-versus-scan: it is + * scan-versus-**mutation**. A scan takes seconds and does not set `working`, so + * nothing stopped a reclaim starting on top of one. The reclaim correctly drops + * the plan — and then the still-running scan landed, passed its own generation + * check, and repainted a pre-reclaim report *plus a fresh, clickable plan + * listing objects that had just been deleted*. So every mutation bumps the + * counter as well: whatever a scan is holding was measured before the mutation + * and is now a lie, and throwing it away is the only honest thing to do with + * it. (The Scan button is disabled while `working` for the mirror-image case, + * so a scan can never start *during* a mutation.) + * + * That is also why `scanning` is not cleared against the same counter: a + * mutation bumping it mid-scan would strand the flag at true and leave the + * button reading "Scanning…" forever. `latestScan` records the generation the + * newest *scan* owns — only a newer scan may take the flag away — and that is + * what the `finally` compares against. */ export interface DiskUsageState { report: DiskUsageReport | null; @@ -47,8 +62,15 @@ export interface DiskUsageState { /** The outcome of the last reclaim, kept on screen until the next scan. */ outcome: ReclaimOutcome | null; scan: () => Promise; - runReclaim: (targets: ReclaimTarget[]) => Promise; - destroy: (target: DestructiveTarget, confirmation: string) => Promise; + /** + * Resolves `true` when the call came back, `false` when it threw and the + * failure went into `error`. Callers that dismiss UI on completion — the + * confirmation dialogs — must only dismiss on `true`, or the failure is left + * with nowhere on screen the user is looking. + */ + runReclaim: (targets: ReclaimTarget[]) => Promise; + /** Same contract as `runReclaim`: `false` means it failed and `error` says how. */ + destroy: (target: DestructiveTarget, confirmation: string) => Promise; /** Run the orphaned-snapshot sweep and report what it found *and refused*. */ runSweep: () => Promise; clearOutcome: () => void; @@ -62,9 +84,21 @@ export function useDiskUsage(): DiskUsageState { const [error, setError] = useState(null); const [outcome, setOutcome] = useState(null); const generation = useRef(0); + /** The generation belonging to the most recently *started* scan. */ + const latestScan = useRef(0); + + /** + * Retire every in-flight scan. Called at the top of each mutation, because + * the moment we start deleting things, a measurement taken before that is no + * longer describing the daemon the user is looking at. + */ + const invalidateScans = useCallback(() => { + generation.current += 1; + }, []); const scan = useCallback(async () => { const mine = ++generation.current; + latestScan.current = mine; setScanning(true); setError(null); // The previous outcome describes a state that no longer holds once a new @@ -92,49 +126,66 @@ export function useDiskUsage(): DiskUsageState { // user can no longer see the totals for, but that cannot happen: the two // only ever move together. } finally { - if (generation.current === mine) setScanning(false); + // Deliberately `latestScan`, not `generation`: a mutation that retired + // this scan did not start another one, so this scan is still the last + // word on whether a scan is running. + if (latestScan.current === mine) setScanning(false); } }, []); - const runReclaim = useCallback(async (targets: ReclaimTarget[]) => { - if (targets.length === 0) return; - setWorking(true); - setError(null); - try { - const result = await commands.reclaim(targets); - setOutcome(result); - // **The plan is now stale and must not stay clickable.** Its rows - // describe objects this call just removed, so leaving them ticked lets - // the user fire the same reclaim again against nothing. Dropping the plan - // (not the report) leaves the totals on screen, marked as measured before - // the reclaim, with the tick list gone. - // - // Deliberately no automatic re-scan: it costs another `df()`, and the - // outcome already reports measured bytes for every target — a user who - // wants the new totals asks for them. - setPlan(null); - } catch (e) { - setError(String(e)); - } finally { - setWorking(false); - } - }, []); + const runReclaim = useCallback( + async (targets: ReclaimTarget[]): Promise => { + // Nothing was asked for, so nothing failed — a caller gating a dialog on + // this must not be left staring at an error that has no cause. + if (targets.length === 0) return true; + invalidateScans(); + setWorking(true); + setError(null); + try { + const result = await commands.reclaim(targets); + setOutcome(result); + // **The plan is now stale and must not stay clickable.** Its rows + // describe objects this call just removed, so leaving them ticked lets + // the user fire the same reclaim again against nothing. Dropping the plan + // (not the report) leaves the totals on screen, marked as measured before + // the reclaim, with the tick list gone. + // + // Deliberately no automatic re-scan: it costs another `df()`, and the + // outcome already reports measured bytes for every target — a user who + // wants the new totals asks for them. + setPlan(null); + return true; + } catch (e) { + setError(String(e)); + return false; + } finally { + setWorking(false); + } + }, + [invalidateScans], + ); - const destroy = useCallback(async (target: DestructiveTarget, confirmation: string) => { - setWorking(true); - setError(null); - try { - const result = await commands.destroyProjectDiskObject(target, confirmation); - setOutcome({ results: [result], total_freed_bytes: result.freed_bytes }); - // Same reasoning as `runReclaim`: the destructive list named an object - // that is now gone. - setPlan(null); - } catch (e) { - setError(String(e)); - } finally { - setWorking(false); - } - }, []); + const destroy = useCallback( + async (target: DestructiveTarget, confirmation: string): Promise => { + invalidateScans(); + setWorking(true); + setError(null); + try { + const result = await commands.destroyProjectDiskObject(target, confirmation); + setOutcome({ results: [result], total_freed_bytes: result.freed_bytes }); + // Same reasoning as `runReclaim`: the destructive list named an object + // that is now gone. + setPlan(null); + return true; + } catch (e) { + setError(String(e)); + return false; + } finally { + setWorking(false); + } + }, + [invalidateScans], + ); /** * The startup sweep, on demand. @@ -147,6 +198,7 @@ export function useDiskUsage(): DiskUsageState { * report away. */ const runSweep = useCallback(async () => { + invalidateScans(); setWorking(true); setError(null); try { @@ -178,7 +230,7 @@ export function useDiskUsage(): DiskUsageState { } finally { setWorking(false); } - }, []); + }, [invalidateScans]); const clearOutcome = useCallback(() => setOutcome(null), []); diff --git a/app/src/hooks/useFileManager.test.ts b/app/src/hooks/useFileManager.test.ts index c35e770..c91b8e5 100644 --- a/app/src/hooks/useFileManager.test.ts +++ b/app/src/hooks/useFileManager.test.ts @@ -13,7 +13,7 @@ const stageContainerFileForDrag = vi.fn(); vi.mock("../lib/tauri-commands", () => ({ listContainerFiles: (p: string, path: string) => listContainerFiles(p, path), downloadContainerFile: (p: string, c: string, h: string) => downloadContainerFile(p, c, h), - uploadFileToContainer: (p: string, h: string, d: string) => uploadFileToContainer(p, h, d), + uploadFileToContainer: (...args: unknown[]) => uploadFileToContainer(...args), renameContainerPath: (p: string, f: string, t: string) => renameContainerPath(p, f, t), createContainerDirectory: (p: string, parent: string, n: string) => createContainerDirectory(p, parent, n), @@ -21,6 +21,22 @@ vi.mock("../lib/tauri-commands", () => ({ stageContainerFileForDrag: (p: string, path: string) => stageContainerFileForDrag(p, path), })); +/** + * Transient failures go to `ToastHost` rather than an inline string — see the + * comment at the top of `useFileManager`. The store is mocked down to the one + * method the hook reaches for. + */ +const pushToast = vi.fn(); +vi.mock("../store/appState", () => ({ + useAppState: { getState: () => ({ pushToast }) }, +})); + +/** Everything the hook has said through the toast host, message and detail. */ +const toastText = () => + pushToast.mock.calls + .map(([toast]) => `${toast.kind}: ${toast.message} ${toast.detail ?? ""}`) + .join("\n"); + const save = vi.fn(); const openDialog = vi.fn(); vi.mock("@tauri-apps/plugin-dialog", () => ({ @@ -111,7 +127,10 @@ describe("useFileManager uploads", () => { await act(async () => { await result.current.uploadPaths(["/host/ok.txt", "/host/huge.bin"]); }); - expect(result.current.error).toContain("too large"); + // Inline `error` is reserved for the listing failure the user can see in + // context; a failed upload goes where it cannot scroll away. + expect(result.current.error).toBeNull(); + expect(toastText()).toContain("too large"); expect(listContainerFiles).toHaveBeenCalled(); }); @@ -152,7 +171,7 @@ describe("useFileManager rename and mkdir", () => { ok = await result.current.renameEntry(file("hosts"), "hosts.bak"); }); expect(ok).toBe(false); - expect(result.current.error).toContain("Permission denied"); + expect(toastText()).toContain("Permission denied"); }); it("treats an unchanged name as a no-op rather than a round trip", async () => { @@ -183,7 +202,7 @@ describe("useFileManager rename and mkdir", () => { ok = await result.current.createFolder("src"); }); expect(ok).toBe(false); - expect(result.current.error).toContain("File exists"); + expect(toastText()).toContain("File exists"); }); }); @@ -208,7 +227,7 @@ describe("useFileManager save to host", () => { await act(async () => { await result.current.downloadFile(file("src", { is_directory: true })); }); - expect(result.current.error).toContain("is a folder"); + expect(toastText()).toContain("is a folder"); }); }); @@ -270,8 +289,8 @@ describe("useFileManager drag-out staging", () => { }); expect(staged).toBeNull(); - expect(result.current.error).toContain("too large to drag out"); - expect(result.current.error).toContain("Save to host"); + expect(toastText()).toContain("too large to drag out"); + expect(toastText()).toContain("Save to host"); expect(result.current.busy).toBeNull(); }); @@ -290,3 +309,193 @@ describe("useFileManager drag-out staging", () => { expect(staged).toEqual({ hostPath: "/tmp/triple-c-drag-out/s1/a.txt", cached: false }); }); }); + +describe("useFileManager stays where the user is", () => { + it("does not drag the pane back when the user navigates away mid-upload", async () => { + // The closure captured `/workspace`; the user is in `/workspace/src` by the + // time the copy finishes. Re-listing the *captured* path is what used to + // yank them out of the directory they had walked into. + let failUpload: (reason: unknown) => void = () => {}; + // `Once`, deliberately: `clearAllMocks` clears calls but not + // implementations, so a never-settling one would hang every test after it. + uploadFileToContainer.mockImplementationOnce( + () => new Promise((_resolve, reject) => { failUpload = reject; }), + ); + const { result } = renderHook(() => useFileManager("p1")); + + let upload!: Promise; + await act(async () => { + upload = result.current.uploadPaths(["/host/big.bin"]); + await Promise.resolve(); + }); + + listContainerFiles.mockResolvedValue([file("index.ts", { path: "/workspace/src/index.ts" })]); + await act(async () => { + await result.current.navigate("/workspace/src"); + }); + listContainerFiles.mockClear(); + + await act(async () => { + failUpload("cp: no space left on device"); + await upload; + }); + + expect(result.current.currentPath).toBe("/workspace/src"); + expect(result.current.entries.map((e) => e.name)).toEqual(["index.ts"]); + // No re-list of the directory the upload targeted… + expect(listContainerFiles).not.toHaveBeenCalled(); + // …and no failure text painted over the listing that replaced it. + expect(result.current.error).toBeNull(); + expect(toastText()).toContain("no space left"); + }); + + it("re-lists when the user stayed put, which is the ordinary case", async () => { + const { result } = renderHook(() => useFileManager("p1")); + await act(async () => { + await result.current.navigate("/workspace"); + }); + listContainerFiles.mockClear(); + await act(async () => { + await result.current.uploadPaths(["/host/a.png"]); + }); + expect(listContainerFiles).toHaveBeenCalledWith("p1", "/workspace"); + }); + + it("lets the newest listing win when a slow one lands last", async () => { + // Two listings in flight, and the slower one is not necessarily the older + // one. Landing last used to set both the rows and the breadcrumb back. + let landSlow: (entries: FileEntry[]) => void = () => {}; + listContainerFiles.mockImplementationOnce( + () => new Promise((resolve) => { landSlow = resolve; }), + ); + listContainerFiles.mockResolvedValueOnce([file("new.txt")]); + const { result } = renderHook(() => useFileManager("p1")); + + let slow!: Promise; + await act(async () => { + slow = result.current.navigate("/workspace/slow"); + await Promise.resolve(); + }); + await act(async () => { + await result.current.navigate("/workspace/fast"); + }); + expect(result.current.currentPath).toBe("/workspace/fast"); + + await act(async () => { + landSlow([file("stale.txt")]); + await slow; + }); + + expect(result.current.currentPath).toBe("/workspace/fast"); + expect(result.current.entries.map((e) => e.name)).toEqual(["new.txt"]); + }); + + it("keeps a failed navigation from claiming the directory it never reached", async () => { + listContainerFiles.mockRejectedValueOnce("Permission denied"); + const { result } = renderHook(() => useFileManager("p1")); + await act(async () => { + await result.current.navigate("/root"); + }); + listContainerFiles.mockClear(); + // The pane never left /workspace, so an upload started now targets it. + await act(async () => { + await result.current.uploadPaths(["/host/a.png"]); + }); + expect(uploadFileToContainer).toHaveBeenCalledWith("p1", "/host/a.png", "/workspace"); + }); +}); + +describe("useFileManager overwrite prompt", () => { + const alreadyThere = "FILE_EXISTS: /workspace/a.txt already exists"; + + it("asks rather than clobbering, and replaces on demand", async () => { + uploadFileToContainer.mockRejectedValueOnce(alreadyThere); + uploadFileToContainer.mockResolvedValueOnce(undefined); + const { result } = renderHook(() => useFileManager("p1")); + + let upload!: Promise; + await act(async () => { + upload = result.current.uploadPaths(["/host/a.txt"]); + await Promise.resolve(); + }); + await waitFor(() => expect(result.current.conflict?.name).toBe("a.txt")); + expect(result.current.conflict?.directory).toBe("/workspace"); + // One file, so there is nothing for a blanket answer to apply to. + expect(result.current.conflict?.remaining).toBe(0); + + await act(async () => { + result.current.resolveConflict("replace"); + await upload; + }); + + expect(uploadFileToContainer).toHaveBeenNthCalledWith(2, "p1", "/host/a.txt", "/workspace", true); + expect(result.current.conflict).toBeNull(); + }); + + it("skips without uploading anything when the user says so", async () => { + uploadFileToContainer.mockRejectedValueOnce(alreadyThere); + const { result } = renderHook(() => useFileManager("p1")); + + let upload!: Promise; + await act(async () => { + upload = result.current.uploadPaths(["/host/a.txt"]); + await Promise.resolve(); + }); + await waitFor(() => expect(result.current.conflict).not.toBeNull()); + await act(async () => { + result.current.resolveConflict("skip"); + await upload; + }); + + expect(uploadFileToContainer).toHaveBeenCalledTimes(1); + // A skip is a choice, not a failure — nothing to report. + expect(toastText()).not.toContain("could not be uploaded"); + }); + + it("asks once for a batch when the answer is Replace all", async () => { + uploadFileToContainer.mockRejectedValueOnce(alreadyThere); + uploadFileToContainer.mockResolvedValueOnce(undefined); + uploadFileToContainer.mockRejectedValueOnce("FILE_EXISTS: /workspace/b.txt already exists"); + uploadFileToContainer.mockResolvedValueOnce(undefined); + const { result } = renderHook(() => useFileManager("p1")); + + let upload!: Promise; + await act(async () => { + upload = result.current.uploadPaths(["/host/a.txt", "/host/b.txt"]); + await Promise.resolve(); + }); + await waitFor(() => expect(result.current.conflict?.remaining).toBe(1)); + await act(async () => { + result.current.resolveConflict("replace-all"); + await upload; + }); + + expect(result.current.conflict).toBeNull(); + expect(uploadFileToContainer).toHaveBeenNthCalledWith(4, "p1", "/host/b.txt", "/workspace", true); + }); + + it("leaves an unrelated failure alone — no prompt offering a button that cannot work", async () => { + uploadFileToContainer.mockRejectedValueOnce("File too large to upload (900 MB; limit 256 MB)"); + const { result } = renderHook(() => useFileManager("p1")); + await act(async () => { + await result.current.uploadPaths(["/host/huge.bin"]); + }); + expect(result.current.conflict).toBeNull(); + expect(toastText()).toContain("too large"); + }); +}); + +describe("useFileManager staged host paths", () => { + it("recognises a path it staged, and only that path", async () => { + stageContainerFileForDrag.mockResolvedValue("/tmp/triple-c-drag-out/s1/a.txt"); + const { result } = renderHook(() => useFileManager("p1")); + + expect(result.current.isStagedHostPath("/tmp/triple-c-drag-out/s1/a.txt")).toBe(false); + await act(async () => { + await result.current.stageForDrag(file("a.txt")); + }); + expect(result.current.isStagedHostPath("/tmp/triple-c-drag-out/s1/a.txt")).toBe(true); + // Same basename, a real host file the user actually wants uploaded. + expect(result.current.isStagedHostPath("/home/me/a.txt")).toBe(false); + }); +}); diff --git a/app/src/hooks/useFileManager.ts b/app/src/hooks/useFileManager.ts index 6e7bc31..d729e05 100644 --- a/app/src/hooks/useFileManager.ts +++ b/app/src/hooks/useFileManager.ts @@ -1,8 +1,75 @@ -import { useState, useCallback, useRef } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { save, open as openDialog } from "@tauri-apps/plugin-dialog"; import type { FileEntry } from "../lib/types"; import * as commands from "../lib/tauri-commands"; +import { useAppState } from "../store/appState"; +import { + fileExistsPath, + isFileExistsError, + type OverwriteChoice, +} from "../lib/uploadErrors"; +/** + * One upload waiting on the user to say whether it may replace what is there. + * `remaining` is how many files are queued behind this one, which is what + * decides whether the blanket answers are worth offering. + */ +export interface UploadConflict { + /** Host file being uploaded. */ + hostPath: string; + /** Bare name, for the prompt. */ + name: string; + /** Container directory it is going into. */ + directory: string; + remaining: number; +} + +/** `/a/b/c.txt` and `C:\a\b\c.txt` both give `c.txt`. */ +function baseName(path: string): string { + const parts = path.split(/[\\/]/); + return parts[parts.length - 1] || path; +} + +/** + * Host paths compare on separators, not on case: the OS hands a dropped path + * back in whatever form its file dialog produced, and on Windows that is not + * reliably the form `stage_container_file_for_drag` returned. + */ +function normaliseHostPath(path: string): string { + return path.replace(/\\/g, "/").replace(/\/+$/, ""); +} + +/** + * ## Where failures are reported + * + * Two audiences, two places, and the split is deliberate. + * + * The **initial listing** failure stays in `error`, rendered inline above the + * (empty) grid. It is on screen, it is in context, it explains why there are + * no rows, and it is not transient — it stands until the directory lists. + * + * Every **transient operation** failure — upload, rename, create folder, + * save-to-host, drag staging — goes to `ToastHost` instead. Those used to land + * in the same inline `error` div, which is the first child of the *scrolling* + * list: three hundred rows down, a refused rename produced no visible change + * at all, just a rename box that stayed open for no stated reason. Worse, the + * file viewer routes its "Save to host…" through the same call, and the viewer + * is a `fixed inset-0` portal at `z-50` — so that failure reported *behind* the + * dialog that caused it. The toast host is a persistent `aria-live` region at + * `z-[60]`, i.e. the one place in the app that is above a modal and does not + * scroll away. + * + * ## Where the current directory lives + * + * `currentPath` is state (the UI renders it) *and* a ref (async work reads it + * after an await). Every long operation captures the directory it targets at + * the start and compares it against the ref at the end: a 200 MB upload into + * `/workspace` must not drag the pane back out of `src/` because that is where + * the closure happened to be created. The ref moves at the *start* of a + * navigation rather than when the listing lands, because the question being + * asked is "where is the user going", not "what is on screen right now" — and + * it is put back if that navigation fails. + */ export function useFileManager(projectId: string) { const [currentPath, setCurrentPath] = useState("/workspace"); const [entries, setEntries] = useState([]); @@ -10,33 +77,72 @@ export function useFileManager(projectId: string) { const [error, setError] = useState(null); /** Transient "uploading 3 files…" style note, shown beside the breadcrumb. */ const [busy, setBusy] = useState(null); + /** + * What just finished. A live region that only ever says "uploading…" tells a + * screen reader user when to start waiting and never when to stop. + */ + const [completed, setCompleted] = useState(null); + const [conflict, setConflict] = useState(null); + + const currentPathRef = useRef(currentPath); + + /** + * A slow listing can land after a newer one and set both the rows and the + * breadcrumb back to a directory the user already left. Same generation + * guard `useDiskUsage` and `useContainerMigration` use: every async write + * checks it is still the newest before it lands. + */ + const navGeneration = useRef(0); + + const startWork = useCallback((note: string) => { + setBusy(note); + setCompleted(null); + }, []); + + const report = useCallback((message: string, detail?: string) => { + useAppState.getState().pushToast({ kind: "error", message, detail }); + }, []); + + const confirm = useCallback((message: string) => { + useAppState.getState().pushToast({ kind: "success", message }); + }, []); const navigate = useCallback( async (path: string) => { + const mine = ++navGeneration.current; + const previous = currentPathRef.current; + currentPathRef.current = path; setLoading(true); setError(null); try { const result = await commands.listContainerFiles(projectId, path); + if (navGeneration.current !== mine) return; setEntries(result); setCurrentPath(path); } catch (e) { + if (navGeneration.current !== mine) return; + // The move did not happen, so the pane is still where it was — the ref + // has to agree with the breadcrumb or the next operation will decide + // it targeted a directory nobody is looking at. + currentPathRef.current = previous; setError(String(e)); } finally { - setLoading(false); + if (navGeneration.current === mine) setLoading(false); } }, [projectId], ); const goUp = useCallback(() => { - if (currentPath === "/") return; - const parent = currentPath.replace(/\/[^/]+$/, "") || "/"; + const here = currentPathRef.current; + if (here === "/") return; + const parent = here.replace(/\/[^/]+$/, "") || "/"; navigate(parent); - }, [currentPath, navigate]); + }, [navigate]); const refresh = useCallback(() => { - navigate(currentPath); - }, [currentPath, navigate]); + navigate(currentPathRef.current); + }, [navigate]); /** Copy an entry out to a host path the user picks. */ const downloadFile = useCallback( @@ -44,30 +150,111 @@ export function useFileManager(projectId: string) { try { const hostPath = await save({ defaultPath: entry.name }); if (!hostPath) return; - setError(null); - await commands.downloadContainerFile(projectId, entry.path, hostPath); + // Every sibling operation sets `busy`; this one did not, so a 200 MB + // copy was a click, then a frozen-looking pane, then nothing. + startWork(`Saving "${entry.name}" to the host…`); + try { + await commands.downloadContainerFile(projectId, entry.path, hostPath); + setCompleted(`Saved "${entry.name}" to ${hostPath}.`); + confirm(`Saved "${entry.name}" to the host.`); + } finally { + setBusy(null); + } } catch (e) { - setError(String(e)); + report(`Could not save "${entry.name}" to the host`, String(e)); } }, - [projectId], + [projectId, startWork, report, confirm], + ); + + /** + * The pending answer to `conflict`. Kept in a ref rather than state because + * the upload loop is `await`ing it — it needs the resolver, not a re-render. + */ + const conflictResolver = useRef<((choice: OverwriteChoice) => void) | null>(null); + + const resolveConflict = useCallback((choice: OverwriteChoice) => { + const resolve = conflictResolver.current; + conflictResolver.current = null; + setConflict(null); + resolve?.(choice); + }, []); + + // A pane unmounted mid-prompt (the tab was closed, the container stopped) + // would otherwise leave the upload loop awaiting an answer that can never + // come. Skipping is the safe reading of "the dialog went away". + useEffect( + () => () => { + conflictResolver.current?.("skip-all"); + conflictResolver.current = null; + }, + [], + ); + + const askOverwrite = useCallback( + (hostPath: string, directory: string, remaining: number, containerPath: string | null) => + new Promise((resolve) => { + conflictResolver.current = resolve; + setConflict({ + hostPath, + name: baseName(containerPath ?? hostPath), + directory, + remaining, + }); + }), + [], ); /** * Copy host files into the current directory. Shared by the Upload button and * the native drag-drop listener, so a dropped file and a picked one take the * same path — including the one refresh at the end rather than one per file. + * + * The backend refuses to overwrite unless asked to, so a name clash is not a + * failure here: it is a question, and the answer can be given once for the + * whole batch. */ const uploadPaths = useCallback( async (hostPaths: string[]) => { if (hostPaths.length === 0) return; - setError(null); - setBusy(`Uploading ${hostPaths.length} item${hostPaths.length > 1 ? "s" : ""}…`); + // The directory this upload is *for*. Compared against the live ref at + // the end, because the user is free to walk away while it copies. + const target = currentPathRef.current; + startWork(`Uploading ${hostPaths.length} item${hostPaths.length > 1 ? "s" : ""}…`); const failures: string[] = []; + let uploaded = 0; + let skipped = 0; + /** A "…all" answer, applied to every remaining clash without asking. */ + let blanket: OverwriteChoice | null = null; try { - for (const hostPath of hostPaths) { + for (let i = 0; i < hostPaths.length; i++) { + const hostPath = hostPaths[i]; try { - await commands.uploadFileToContainer(projectId, hostPath, currentPath); + await commands.uploadFileToContainer(projectId, hostPath, target); + uploaded++; + continue; + } catch (e) { + if (!isFileExistsError(e)) { + failures.push(String(e)); + continue; + } + const choice: OverwriteChoice = + blanket ?? + (await askOverwrite( + hostPath, + target, + hostPaths.length - i - 1, + fileExistsPath(e), + )); + if (choice === "replace-all" || choice === "skip-all") blanket = choice; + if (choice === "skip" || choice === "skip-all") { + skipped++; + continue; + } + } + try { + await commands.uploadFileToContainer(projectId, hostPath, target, true); + uploaded++; } catch (e) { failures.push(String(e)); } @@ -75,12 +262,25 @@ export function useFileManager(projectId: string) { } finally { setBusy(null); } - // Re-list first: `navigate` clears the error, so reporting before it - // would wipe the very message the user needs. - await navigate(currentPath); - if (failures.length > 0) setError(failures.join(" · ")); + + const summary = + `Uploaded ${uploaded} item${uploaded === 1 ? "" : "s"}` + + (skipped > 0 ? `, skipped ${skipped}` : "") + + (failures.length > 0 ? `, ${failures.length} failed` : "") + + "."; + setCompleted(summary); + + if (failures.length > 0) { + report( + failures.length === 1 ? "A file could not be uploaded" : `${failures.length} files could not be uploaded`, + failures.join("\n"), + ); + } + // Only re-list if the user is still looking at the directory this went + // into. Navigating away during a slow copy used to drag the pane back. + if (currentPathRef.current === target) await navigate(target); }, - [projectId, currentPath, navigate], + [projectId, navigate, startWork, report, askOverwrite], ); /** @@ -89,10 +289,28 @@ export function useFileManager(projectId: string) { * last listing re-stages rather than dragging a stale copy. */ const stagedRef = useRef(new Map()); + /** + * The same paths the other way round, as a set. + * + * A drag-out released back inside the app arrives as an ordinary host drop + * carrying the staged copy's path, and uploading that would write the app's + * own temp copy over the container file it came from — which is worse than a + * no-op, because the key above is built from the *last listing*, so a file an + * agent rewrote since then would be replaced by a minutes-old snapshot. This + * set is what makes the "is this ours?" test exact instead of a guess at the + * temp directory's name. + */ + const stagedHostPathsRef = useRef(new Set()); + + /** True when `path` is a copy this pane staged for a drag-out. */ + const isStagedHostPath = useCallback( + (path: string) => stagedHostPathsRef.current.has(normaliseHostPath(path)), + [], + ); /** * Copy an entry onto the host so the OS can drag it, and return the absolute - * host path — or `null`, having set `error`, if it could not be staged. + * host path — or `null`, having reported why, if it could not be staged. * * `cached` is what the caller needs to tell a gesture that will feel * instantaneous from one that has a whole-file copy in front of it: the copy @@ -105,20 +323,21 @@ export function useFileManager(projectId: string) { const cached = stagedRef.current.get(key); if (cached) return { hostPath: cached, cached: true }; - setError(null); - setBusy(`Preparing "${entry.name}"…`); + startWork(`Preparing "${entry.name}"…`); try { const hostPath = await commands.stageContainerFileForDrag(projectId, entry.path); stagedRef.current.set(key, hostPath); + stagedHostPathsRef.current.add(normaliseHostPath(hostPath)); + setCompleted(`"${entry.name}" is ready to drag.`); return { hostPath, cached: false }; } catch (e) { - setError(String(e)); + report(`Could not prepare "${entry.name}" for dragging`, String(e)); return null; } finally { setBusy(null); } }, - [projectId], + [projectId, startWork, report], ); const uploadFile = useCallback(async () => { @@ -127,9 +346,9 @@ export function useFileManager(projectId: string) { if (!selected) return; await uploadPaths(Array.isArray(selected) ? selected : [selected as string]); } catch (e) { - setError(String(e)); + report("Could not open the file picker", String(e)); } - }, [uploadPaths]); + }, [uploadPaths, report]); /** * Rename in place. `newName` is a bare name — Rust rejects anything with a @@ -140,42 +359,50 @@ export function useFileManager(projectId: string) { async (entry: FileEntry, newName: string) => { const trimmed = newName.trim(); if (!trimmed || trimmed === entry.name) return true; + const target = currentPathRef.current; try { - setError(null); await commands.renameContainerPath(projectId, entry.path, trimmed); - await navigate(currentPath); + setCompleted(`Renamed "${entry.name}" to "${trimmed}".`); + if (currentPathRef.current === target) await navigate(target); return true; } catch (e) { - setError(String(e)); + report(`Could not rename "${entry.name}"`, String(e)); return false; } }, - [projectId, currentPath, navigate], + [projectId, navigate, report], ); const createFolder = useCallback( async (name: string) => { const trimmed = name.trim(); if (!trimmed) return true; + const target = currentPathRef.current; try { - setError(null); - await commands.createContainerDirectory(projectId, currentPath, trimmed); - await navigate(currentPath); + await commands.createContainerDirectory(projectId, target, trimmed); + setCompleted(`Created "${trimmed}".`); + if (currentPathRef.current === target) await navigate(target); return true; } catch (e) { - setError(String(e)); + report(`Could not create "${trimmed}"`, String(e)); return false; } }, - [projectId, currentPath, navigate], + [projectId, navigate, report], ); return { currentPath, entries, loading, + /** Inline, in-context: why the listing on screen is empty. */ error, busy, + /** What the last operation finished doing, for the live region. */ + completed, + /** An upload waiting for a Replace / Skip answer, or `null`. */ + conflict, + resolveConflict, setError, navigate, goUp, @@ -184,6 +411,7 @@ export function useFileManager(projectId: string) { uploadFile, uploadPaths, stageForDrag, + isStagedHostPath, renameEntry, createFolder, }; diff --git a/app/src/hooks/useProjectActions.ts b/app/src/hooks/useProjectActions.ts index 9f5d7f4..07ded95 100644 --- a/app/src/hooks/useProjectActions.ts +++ b/app/src/hooks/useProjectActions.ts @@ -2,6 +2,7 @@ import { useCallback, useState } from "react"; import { save } from "@tauri-apps/plugin-dialog"; import type { Project } from "../lib/types"; import * as commands from "../lib/tauri-commands"; +import { formatBytes } from "../lib/formatBytes"; import { useAppState } from "../store/appState"; import { useProjects } from "./useProjects"; import { useTerminal } from "./useTerminal"; @@ -122,10 +123,15 @@ export function useProjectActions(project: Project) { if (!hostPath) return; setBackingUp(true); const bytes = await commands.downloadContainerBackup(project.id, hostPath); - const mb = (bytes / (1024 * 1024)).toFixed(1); + // `binary` matches what the host's file browser will say about the + // tarball this just wrote. The unit is part of the formatted string, so + // there is no separate " MB" to append — and unlike the inline + // `toFixed(1)` this replaced, a multi-gigabyte backup no longer reports + // itself as a five-digit number of megabytes. + const size = formatBytes(bytes, { binary: true }); pushToast({ kind: "success", - message: `Backup saved (${mb} MB).`, + message: `Backup saved (${size}).`, detail: "Includes Claude config — may contain API keys. Keep the archive private.", }); diff --git a/app/src/lib/dropTarget.test.ts b/app/src/lib/dropTarget.test.ts new file mode 100644 index 0000000..07a8f8b --- /dev/null +++ b/app/src/lib/dropTarget.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import { dropIsBlocked, isDropTarget } from "./dropTarget"; + +function pane(rect: Partial): HTMLElement { + const el = document.createElement("div"); + document.body.appendChild(el); + el.getBoundingClientRect = () => + ({ + left: 0, + top: 0, + right: 100, + bottom: 100, + width: 100, + height: 100, + x: 0, + y: 0, + toJSON: () => ({}), + ...rect, + }) as DOMRect; + return el; +} + +describe("dropTarget", () => { + beforeEach(() => { + document.body.innerHTML = ""; + }); + afterEach(() => { + document.body.innerHTML = ""; + }); + + it("accepts a point inside the pane", () => { + expect(isDropTarget(pane({}), { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe(true); + }); + + it("rejects a point outside the pane", () => { + expect(isDropTarget(pane({}), { x: 400, y: 50 }, { devicePixelRatio: 1 })).toBe(false); + }); + + it("converts physical pixels to CSS pixels", () => { + const el = pane({}); + expect(isDropTarget(el, { x: 150, y: 150 }, { devicePixelRatio: 2 })).toBe(true); + expect(isDropTarget(el, { x: 150, y: 150 }, { devicePixelRatio: 1 })).toBe(false); + }); + + it("rejects a hidden pane, which has a zero-size rect", () => { + const el = pane({ right: 0, bottom: 0, width: 0, height: 0 }); + expect(isDropTarget(el, { x: 0, y: 0 }, { devicePixelRatio: 1 })).toBe(false); + }); + + it("rejects every drop while a modal is open", () => { + const el = pane({}); + const dialog = document.createElement("div"); + dialog.setAttribute("role", "dialog"); + dialog.setAttribute("aria-modal", "true"); + document.body.appendChild(dialog); + + expect(dropIsBlocked()).toBe(true); + expect(isDropTarget(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe(false); + + dialog.remove(); + expect(dropIsBlocked()).toBe(false); + expect(isDropTarget(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe(true); + }); + + it("rejects every drop while a blocking overlay is up", () => { + const el = pane({}); + const overlay = document.createElement("div"); + overlay.setAttribute("data-blocks-drop", "true"); + document.body.appendChild(overlay); + expect(isDropTarget(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe(false); + }); + + it("rejects a null pane", () => { + expect(isDropTarget(null, { x: 1, y: 1 }, { devicePixelRatio: 1 })).toBe(false); + }); +}); diff --git a/app/src/lib/dropTarget.ts b/app/src/lib/dropTarget.ts new file mode 100644 index 0000000..534009e --- /dev/null +++ b/app/src/lib/dropTarget.ts @@ -0,0 +1,81 @@ +/** + * Routing for Tauri's *native* drag-drop event. + * + * The listener is window-wide — every pane that wants dropped file paths gets + * the same event — so each one decides for itself whether the drop was meant + * for it. That decision used to be purely geometric: is the payload position + * inside my rect? A rect is not what the user sees, though. An open `Modal` is + * a `fixed inset-0` portal at `z-50` painted *over* the whole window, and the + * pane underneath still had its rect, so releasing a drag onto a dialog + * uploaded the file into the directory the dialog was covering. Same for the + * shutdown overlay, which is on screen precisely while nothing should be + * accepting work at all. + * + * So the hit test is now: nothing is covering the window, **and** the point is + * inside my rect, **and** whatever is actually painted at that point is mine. + */ + +export interface DropPoint { + x: number; + y: number; +} + +/** + * Anything that swallows a drop wherever it lands. + * + * `[aria-modal="true"]` is every dialog in the app for free — `ui/Modal` is + * the only way one is built, and it sets that attribute. `data-blocks-drop` + * is for full-window overlays that are not dialogs (the shutdown overlay). + */ +const BLOCKING_SELECTOR = '[aria-modal="true"],[data-blocks-drop="true"]'; + +/** True while a modal or a blocking overlay is on screen. */ +export function dropIsBlocked(doc: Document = document): boolean { + return doc.querySelector(BLOCKING_SELECTOR) !== null; +} + +export interface DropTargetOptions { + doc?: Document; + /** Override the ratio used to convert physical pixels to CSS pixels. */ + devicePixelRatio?: number; +} + +/** + * Whether a native drop at `pos` (physical pixels) belongs to `el`. + * + * A hidden pane is `display:none` and therefore has a zero-size rect, which is + * what stops two panes both claiming the same drop. + */ +export function isDropTarget( + el: HTMLElement | null | undefined, + pos: DropPoint, + options: DropTargetOptions = {}, +): boolean { + const doc = options.doc ?? el?.ownerDocument ?? document; + if (dropIsBlocked(doc)) return false; + + const rect = el?.getBoundingClientRect(); + if (!el || !rect || rect.width === 0 || rect.height === 0) return false; + + const dpr = + options.devicePixelRatio ?? + (doc.defaultView?.devicePixelRatio || 1); + const x = pos.x / dpr; + const y = pos.y / dpr; + if (x < rect.left || x > rect.right || y < rect.top || y > rect.bottom) { + return false; + } + + // Z-order, where the environment can answer it. `elementFromPoint` skips + // `pointer-events: none`, so the pane's own decorative drop hint does not + // count as something covering it. jsdom has no layout and returns null, + // which is treated as "no opinion" rather than "not mine". + if (typeof doc.elementFromPoint === "function") { + const top = doc.elementFromPoint(x, y); + if (top && top !== doc.body && top !== doc.documentElement && !el.contains(top)) { + return false; + } + } + + return true; +} diff --git a/app/src/lib/formatBytes.test.ts b/app/src/lib/formatBytes.test.ts index aabd3d1..dc062ad 100644 --- a/app/src/lib/formatBytes.test.ts +++ b/app/src/lib/formatBytes.test.ts @@ -29,6 +29,22 @@ describe("formatBytes", () => { expect(formatBytes(1_610_612_736, { binary: true })).toBe("1.5 GB"); }); + it("absorbs the last two ad-hoc formatters, behaviour change and all", () => { + // `UpdateDialog.formatSize` and the inline `toFixed(1)` in + // `useProjectActions` both divided by 1024 and both stopped at MB. Routing + // them here is what finally makes this the *only* byte formatter, and it + // changes two things on purpose — pinned so neither reads as a regression + // to whoever meets them next. + // + // KB gains a decimal, matching every other size in the app: + expect(formatBytes(512 * 1024, { binary: true })).toBe("512.0 KB"); + // and the ladder no longer bottoms out at a five-digit megabyte count: + expect(formatBytes(2 * 1024 ** 3, { binary: true })).toBe("2.0 GB"); + // Sub-kilobyte sizes stop rendering as "0 KB", which is what the old + // `(bytes / 1024).toFixed(0)` said about every release asset under 512 B. + expect(formatBytes(400, { binary: true })).toBe("400 B"); + }); + it("reproduces the migration convention exactly by default", () => { // `migrationCopy.formatDataSize` is now a call to this, and its output is // asserted in MigrateContainerModal.test.tsx. diff --git a/app/src/lib/formatBytes.ts b/app/src/lib/formatBytes.ts index a66cb11..1ee4cab 100644 --- a/app/src/lib/formatBytes.ts +++ b/app/src/lib/formatBytes.ts @@ -4,11 +4,16 @@ * The app had four of them — `projects/home/format.ts`, * `projects/migrationCopy.ts`, `settings/UpdateDialog.tsx` and an inline * `toFixed(1)` in `useProjectActions.ts` — disagreeing about the divisor, the - * unit labels and the precision. The first two now delegate here. + * unit labels and the precision. All four now delegate here, and there are no + * remaining copies. * - * The other two deliberately do not, yet: `UpdateDialog` renders KB at - * `toFixed(0)`, so re-pointing it would change what a download size reads as, - * and neither is on the Disk panel's path. They are the remaining copies. + * The last two were held back because re-pointing them changes what they + * render, and that turned out to be the argument for doing it rather than + * against. `UpdateDialog` rendered KB at `toFixed(0)` (`512 KB` is now + * `512.0 KB`, consistent with every other size in the app) and both stopped + * the ladder at MB, so a 2 GB asset or backup read as a five-digit number of + * megabytes. Both are `{ binary: true }`: they describe files, and a host file + * browser shows the ÷1024 figure for the same bytes. * * ## Why the default is base 1000 * diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index 597b4a9..afebc9d 100644 --- a/app/src/lib/tauri-commands.ts +++ b/app/src/lib/tauri-commands.ts @@ -75,8 +75,21 @@ export const downloadContainerFile = (projectId: string, containerPath: string, invoke("download_container_file", { projectId, containerPath, hostPath }); export const downloadContainerBackup = (projectId: string, hostPath: string, containerPath?: string) => invoke("download_container_backup", { projectId, hostPath, containerPath }); -export const uploadFileToContainer = (projectId: string, hostPath: string, containerDir: string) => - invoke("upload_file_to_container", { projectId, hostPath, containerDir }); +/** + * Copy a host file into a container directory. + * + * `overwrite` is opt-in because a drop is aimed with a mouse: the backend + * refuses by default when the name is already taken (see `lib/uploadErrors.ts` + * for the marker that refusal carries), and the caller re-runs with `true` + * only once the user has said "Replace" to that specific file. Leaving it off + * is the safe default every existing caller gets. + */ +export const uploadFileToContainer = ( + projectId: string, + hostPath: string, + containerDir: string, + overwrite?: boolean, +) => invoke("upload_file_to_container", { projectId, hostPath, containerDir, overwrite }); export const readContainerFile = (projectId: string, path: string, maxBytes?: number) => invoke("read_container_file", { projectId, path, maxBytes }); /** `toPath` is the new *name*, not a destination — renames never move. */ diff --git a/app/src/lib/uploadErrors.test.ts b/app/src/lib/uploadErrors.test.ts new file mode 100644 index 0000000..a5ee0e1 --- /dev/null +++ b/app/src/lib/uploadErrors.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "vitest"; +import { + FILE_EXISTS_MARKER, + fileExistsPath, + isFileExistsError, +} from "./uploadErrors"; + +/** + * The shapes here are the point of the module. + * + * A Tauri command error crosses the IPC boundary as whatever `serde` made of + * it, and the Rust side is free to change from `Err(String)` to a serialised + * error enum without anyone thinking of this file. Every one of these has to + * keep meaning "that name is taken", or an upload that could have been + * retried with `overwrite: true` degrades into a raw string in a toast. + */ +describe("isFileExistsError", () => { + it("recognises the agreed prose form", () => { + expect(isFileExistsError("FILE_EXISTS: /workspace/notes.txt already exists")).toBe(true); + }); + + it("recognises a bare marker", () => { + expect(isFileExistsError(FILE_EXISTS_MARKER)).toBe(true); + }); + + it("recognises a serialised error enum, whatever case it is written in", () => { + expect(isFileExistsError({ kind: "FileExists", path: "/workspace/a.txt" })).toBe(true); + expect(isFileExistsError({ code: "file-exists" })).toBe(true); + expect(isFileExistsError({ type: "file_exists" })).toBe(true); + }); + + it("recognises it inside a message field", () => { + expect(isFileExistsError({ message: "upload refused: FILE_EXISTS" })).toBe(true); + expect(isFileExistsError(new Error("FILE_EXISTS: /workspace/a.txt"))).toBe(true); + }); + + it("looks one level into a wrapped error", () => { + expect(isFileExistsError({ error: { kind: "FileExists" } })).toBe(true); + }); + + it("says no to every other failure, which must not raise an overwrite prompt", () => { + expect(isFileExistsError("File too large to upload (900 MB; limit 256 MB)")).toBe(false); + expect(isFileExistsError("cp: cannot create regular file: Permission denied")).toBe(false); + expect(isFileExistsError({ kind: "NotRunning" })).toBe(false); + expect(isFileExistsError(null)).toBe(false); + expect(isFileExistsError(undefined)).toBe(false); + expect(isFileExistsError(42)).toBe(false); + expect(isFileExistsError({})).toBe(false); + }); +}); + +describe("fileExistsPath", () => { + it("reads the path out of the agreed prose form", () => { + expect(fileExistsPath("FILE_EXISTS: /workspace/notes.txt already exists")).toBe( + "/workspace/notes.txt", + ); + }); + + it("prefers a structured field", () => { + expect(fileExistsPath({ kind: "FileExists", path: "/workspace/a.txt" })).toBe( + "/workspace/a.txt", + ); + expect(fileExistsPath({ kind: "FileExists", container_path: "/workspace/b.txt" })).toBe( + "/workspace/b.txt", + ); + }); + + it("finds one in a wrapped error", () => { + expect(fileExistsPath({ error: { kind: "FileExists", path: "/workspace/c.txt" } })).toBe( + "/workspace/c.txt", + ); + }); + + it("returns null rather than guessing", () => { + // The caller falls back to the host path it was uploading, which is always + // known — so "no path" is a perfectly good answer. + expect(fileExistsPath("FILE_EXISTS")).toBeNull(); + expect(fileExistsPath({ kind: "FileExists" })).toBeNull(); + expect(fileExistsPath(null)).toBeNull(); + }); +}); diff --git a/app/src/lib/uploadErrors.ts b/app/src/lib/uploadErrors.ts new file mode 100644 index 0000000..b93a5d2 --- /dev/null +++ b/app/src/lib/uploadErrors.ts @@ -0,0 +1,116 @@ +/** + * The one place the frontend agrees with Rust about "that name is taken". + * + * `upload_file_to_container` used to clobber whatever was already at the + * destination, which is the wrong default for a drop: a drag is aimed with a + * mouse, and the file it lands on is frequently not the file the user meant to + * replace. So the backend refuses by default and the frontend asks — but only + * if it can tell *this* refusal apart from "permission denied" or "no space + * left", because an overwrite prompt raised over an unrelated failure would + * offer a button that cannot possibly work. + * + * **This module is the contract point, and the Rust half has to hold up its + * end**: `upload_file_to_container` must put `FILE_EXISTS_MARKER` in the error + * it returns when the destination already exists, ideally in the agreed shape + * + * FILE_EXISTS: /workspace/notes.txt already exists + * + * and must accept an `overwrite: bool` argument that skips the check. Nothing + * here parses a human sentence — the marker is the whole agreement, and the + * path is a bonus that is only used to name the file in the prompt. + * + * The predicate is deliberately tolerant about the *shape* of the error rather + * than its wording, because a Tauri command error crosses the IPC boundary as + * whatever `serde` made of it: a bare string from `Err(String)`, an object from + * a `#[derive(Serialize)]` error enum, or an `Error` if a JS layer wrapped it + * on the way through. All three are the same refusal, and the UI must not + * behave differently depending on which one a future refactor produces. + */ + +/** Marker the backend puts in the error for "a file with this name is already there". */ +export const FILE_EXISTS_MARKER = "FILE_EXISTS"; + +/** + * Structured error shapes carry the marker in a discriminant rather than in + * prose. These are the field names a serialised Rust error realistically uses; + * matching is case-insensitive and ignores `_`/`-` so `FileExists`, + * `file_exists` and `FILE-EXISTS` all read as the same variant. + */ +const KIND_FIELDS = ["kind", "code", "type", "error", "reason"] as const; +const MESSAGE_FIELDS = ["message", "msg", "detail", "description"] as const; +const PATH_FIELDS = ["path", "container_path", "containerPath", "target", "file"] as const; + +/** `FileExists` / `file-exists` / `FILE_EXISTS` all normalise to `fileexists`. */ +function normaliseKind(value: string): string { + return value.toLowerCase().replace(/[\s_-]/g, ""); +} + +const KIND_NEEDLE = normaliseKind(FILE_EXISTS_MARKER); + +function asRecord(e: unknown): Record | null { + return typeof e === "object" && e !== null ? (e as Record) : null; +} + +/** + * Every string an error carries, flattened: the error itself if it is one, its + * message-ish fields, and its kind-ish fields. Nesting is followed one level + * because a wrapped error (`{ error: { kind: … } }`) is the same refusal. + */ +function stringsIn(e: unknown, depth = 0): string[] { + if (typeof e === "string") return [e]; + if (e instanceof Error) return [e.message, e.name]; + const record = asRecord(e); + if (!record || depth > 1) return []; + const out: string[] = []; + for (const field of [...KIND_FIELDS, ...MESSAGE_FIELDS]) { + const value = record[field]; + if (typeof value === "string") out.push(value); + else if (value !== undefined) out.push(...stringsIn(value, depth + 1)); + } + return out; +} + +/** + * True when the backend refused an upload because the destination is taken. + * + * Accepts a bare string, an `Error`, or an object with a `kind`/`code` + * discriminant or a `message` — see the module comment for why all three have + * to work. + */ +export function isFileExistsError(e: unknown): boolean { + return stringsIn(e).some((s) => normaliseKind(s).includes(KIND_NEEDLE)); +} + +/** + * The container path the conflict is about, when the error carries one — used + * only to name the file in the prompt, so `null` is a perfectly good answer + * and the caller falls back to the host path it was uploading. + */ +export function fileExistsPath(e: unknown): string | null { + const record = asRecord(e); + if (record) { + for (const field of PATH_FIELDS) { + const value = record[field]; + if (typeof value === "string" && value.length > 0) return value; + } + // One level down, for `{ error: { path } }`. + for (const field of KIND_FIELDS) { + const nested = fileExistsPath(record[field]); + if (nested) return nested; + } + } + for (const s of stringsIn(e)) { + // The agreed prose form: `FILE_EXISTS: ` — everything up to the + // first space after the marker. + const match = new RegExp(`${FILE_EXISTS_MARKER}\\s*[:=]\\s*(\\S+)`).exec(s); + if (match) return match[1]; + } + return null; +} + +/** + * What the user answered to one conflict. The blanket answers exist because a + * ten-file drop onto a populated directory is ten prompts otherwise, which is + * the kind of dialog people dismiss without reading. + */ +export type OverwriteChoice = "replace" | "skip" | "replace-all" | "skip-all"; diff --git a/app/src/lib/urlDetector.test.ts b/app/src/lib/urlDetector.test.ts index 23ff0e5..875bb9d 100644 --- a/app/src/lib/urlDetector.test.ts +++ b/app/src/lib/urlDetector.test.ts @@ -225,6 +225,55 @@ describe("UrlDetector — OSC 8", () => { expect(seen).toEqual([[url, "heuristic"]]); }); + it("never hands back a truncated guess at a link it has already seen exactly", () => { + // The defect: the prompt slot is emptied (dismissed, or auto-dismissed + // after 30 s), the OSC 8 target is deduped for the session and cannot come + // back, and the next repaint — sliced at a different offset, so a *new* + // string — reassembles into a prefix of the real link that fills the empty + // slot. It parses, it points at claude.ai, and it authorises nothing. + // + // Nothing here knows the slot was emptied, and that is the point: the rule + // holds however many times it is. + const seen: [string, UrlSource][] = []; + const d = new UrlDetector((u, s) => seen.push([u, s]), () => COLS); + + feed(d, "Open this link to sign in:\r\n" + slicedHyperlink(SIGN_IN_URL) + "\r\ndone\r\n"); + expect(seen).toEqual([[SIGN_IN_URL, "osc8"]]); + + // …the user dismisses the toast; the TUI repaints the same link as plain + // text, cut short by the frame it was painted into. + feed(d, SIGN_IN_URL.slice(0, 150) + "\r\nWaiting for the browser…\r\n"); + + expect(seen).toHaveLength(1); + expect(seen.map(([u]) => u)).not.toContain(SIGN_IN_URL.slice(0, 150)); + }); + + it("still offers a genuinely different link after an exact one", () => { + // The suppression is a prefix rule, not "one prompt per session". + const seen: string[] = []; + const d = new UrlDetector((u) => seen.push(u), () => COLS); + const other = "https://github.com/login/device?code=" + "x".repeat(90); + + feed(d, slicedHyperlink(SIGN_IN_URL) + "\r\n"); + feed(d, other + "\r\nnext\r\n"); + + expect(seen).toEqual([SIGN_IN_URL, other]); + }); + + it("suppresses a guess at a URL the consumer reported from the relay", () => { + // The OSC 7777 relay hands `TerminalView` a base64-encoded — therefore + // exact — URL that this detector never sees. `noteExactUrl` is how it gets + // told, so a dismissed relay prompt cannot be replaced by a scrape of the + // same link either. + const seen: string[] = []; + const d = new UrlDetector((u) => seen.push(u), () => COLS); + d.noteExactUrl(SIGN_IN_URL); + + feed(d, SIGN_IN_URL.slice(0, 150) + "\r\nnext\r\n"); + + expect(seen).toEqual([]); + }); + it("ignores a short hyperlink", () => { // `ls --hyperlink` decorates every filename; none of that is a prompt. const seen: string[] = []; diff --git a/app/src/lib/urlDetector.ts b/app/src/lib/urlDetector.ts index d485583..6bab70c 100644 --- a/app/src/lib/urlDetector.ts +++ b/app/src/lib/urlDetector.ts @@ -45,8 +45,27 @@ * * So each emitted candidate is tagged with where it came from, and the consumer * refuses to let a `heuristic` candidate displace an `osc8` one. + * + * ## …and the exact copy keeps winning after the prompt is gone + * + * The consumer's precedence rule only compares a new candidate against what is + * *currently* in the prompt slot. Empty the slot — the user dismisses the + * toast, or its 30 s auto-dismiss fires — and it has nothing to compare + * against, so the next truncated guess walks straight in. Meanwhile the OSC 8 + * target is deduped for the life of the session and cannot come back to + * displace it. The user is then holding a URL that parses, points at + * claude.ai, and authorises nothing, which is the exact bug the OSC 8 branch + * was added to kill. + * + * That is fixed *here* rather than in the consumer, because this is the side + * that knows both halves: {@link UrlDetector} remembers every exact URL it has + * seen and refuses to emit a heuristic candidate that is a strict prefix of + * one — see `truncatesKnownExact`. The rule then holds however often the slot + * is emptied, and needs no cooperation from whoever owns it. */ +import { extendsUrl } from "./urlRelay"; + const ANSI_RE = /\x1b(?:\[[0-9;?]*[A-Za-z]|\][^\x07\x1b]*(?:\x07|\x1b\\)?|[()#][A-Za-z0-9]|.)/g; @@ -196,6 +215,21 @@ export class UrlDetector { /** OSC 8 targets already offered, so a hyperlink repainted every frame does * not re-prompt. Bounded by {@link MAX_REMEMBERED_LINKS}. */ private emittedLinks = new Set(); + /** + * Every *exact* URL this session has seen — OSC 8 parameters, plus whatever + * the consumer reports through {@link noteExactUrl} (the OSC 7777 relay). + * + * Kept separately from `emittedLinks` because the two answer different + * questions: that one is "have I already prompted for this?", this one is "do + * I know the full text of a link some guess might be a prefix of?". The + * second answer must survive the prompt being dismissed; the whole defect is + * that a truncated guess fills the slot the moment it is empty. + * + * Bounded the same way, and cleared wholesale rather than evicted one by one: + * a program printing a fresh hyperlink every frame is not a program whose + * older links are still on screen to be mis-scraped. + */ + private exactUrls = new Set(); constructor(callback: UrlCallback, columns: ColumnsGetter) { this.callback = callback; @@ -285,7 +319,7 @@ export class UrlDetector { // 6. URL is clearly complete (more content follows) — dedup + emit this.pendingUrl = null; - if (url !== this.lastEmitted) { + if (url !== this.lastEmitted && !this.truncatesKnownExact(url)) { this.lastEmitted = url; this.callback(url, "heuristic"); } @@ -304,10 +338,23 @@ export class UrlDetector { * `lastEmitted` is moved along with them so an identical string arriving on * the heuristic path a moment later is recognised as the same candidate * rather than fired a second time. + * + * Every target is remembered as exact whether or not it is offered — a + * hyperlink repainted a second time is the same known link, and the dedup + * that stops it re-prompting must not also stop it counting as something a + * later guess can be a truncation of. + * + * The alternative fix considered here was to make this dedup *releasable*, + * so the consumer could hand the exact URL back and have it re-offered once + * the prompt slot emptied. Rejected: it re-offers on the very next repaint, + * so dismissing the toast would put it straight back on screen — and it + * still would not establish the invariant, because a truncated guess and the + * released exact URL would simply race for the empty slot. */ private scanLinks(): void { for (const uri of osc8Targets(this.buffer)) { if (uri.length < MIN_URL_LENGTH) continue; + this.rememberExact(uri); if (this.emittedLinks.has(uri)) continue; if (this.emittedLinks.size >= MAX_REMEMBERED_LINKS) { this.emittedLinks.clear(); @@ -319,13 +366,56 @@ export class UrlDetector { } private emitPending(): void { - if (this.pendingUrl && this.pendingUrl !== this.lastEmitted) { + if ( + this.pendingUrl && + this.pendingUrl !== this.lastEmitted && + !this.truncatesKnownExact(this.pendingUrl) + ) { this.lastEmitted = this.pendingUrl; this.callback(this.pendingUrl, "heuristic"); } this.pendingUrl = null; } + /** + * Whether `url` is a strict prefix of an exact URL already seen — i.e. a + * truncated guess at a link whose full text is known. + * + * {@link extendsUrl} is the predicate, used in the direction that asks "does + * the link I already have *extend* this guess?". It is the same rule the + * prompt slot uses to let a candidate grow into its complete form, which is + * the point: the two must agree about what "the same link, only shorter" + * means, so there is one implementation of it. + * + * Deliberately *not* symmetric. A candidate that is longer than a known exact + * URL and starts with it is a different problem (text glued onto the end by a + * wrap that was not a wrap), and it is still shown in full and confirmed by + * the user before anything opens. + */ + private truncatesKnownExact(url: string): boolean { + for (const exact of this.exactUrls) { + if (extendsUrl(exact, url)) return true; + } + return false; + } + + /** + * Record a URL that arrived somewhere exact, outside this detector. + * + * The OSC 7777 relay hands `TerminalView` a base64-encoded URL — exact by + * construction, and never seen here. Without this the suppression rule above + * would cover hyperlinks and miss the relay, and a dismissed relay prompt + * could still be replaced by a truncated scrape of the same link. + */ + noteExactUrl(url: string): void { + this.rememberExact(url); + } + + private rememberExact(url: string): void { + if (this.exactUrls.size >= MAX_REMEMBERED_LINKS) this.exactUrls.clear(); + this.exactUrls.add(url); + } + dispose(): void { if (this.timer !== null) { clearTimeout(this.timer);