diff --git a/app/src/App.tsx b/app/src/App.tsx index 0226df1..c345a8c 100644 --- a/app/src/App.tsx +++ b/app/src/App.tsx @@ -9,6 +9,7 @@ import DockerInstallDialog from "./components/DockerInstallDialog"; import ProjectHome from "./components/projects/home/ProjectHome"; import AddProjectDialog from "./components/projects/AddProjectDialog"; import ToastHost from "./components/ui/ToastHost"; +import { PaneVisibilityProvider } from "./components/ui/PaneVisibility"; import StatusIndicator from "./components/ui/StatusIndicator"; import Button from "./components/ui/Button"; import { useDocker } from "./hooks/useDocker"; @@ -128,19 +129,34 @@ export default function App() { ) : (
+ {/* Every tab stays mounted and the inactive ones are merely + `hidden`, which a dialog's portal to `document.body` does not + inherit: a confirmation opened in one project stayed painted + over whatever tab the user switched to, kept its focus trap, + and — being a blocking overlay — refused every native file + drop in the window. `PaneVisibilityProvider` is how a `Modal` + inside a pane finds out the pane stepped aside. */} {homeProjectIds.map((projectId) => ( - + visible={activeTabKey === homeTabKey(projectId)} + > + + ))} {sessions.map((session) => ( - + visible={session.id === activeSessionId} + > + + ))}
)} diff --git a/app/src/components/projects/home/FilesTab.test.tsx b/app/src/components/projects/home/FilesTab.test.tsx index 38ff127..250ad27 100644 --- a/app/src/components/projects/home/FilesTab.test.tsx +++ b/app/src/components/projects/home/FilesTab.test.tsx @@ -394,15 +394,83 @@ describe("FilesTab host drag-and-drop", () => { expect(uploadFileToContainer).not.toHaveBeenCalled(); }); - it("divides the payload position by devicePixelRatio", async () => { - // The native payload is in physical pixels; the rect is in CSS pixels. - // At dpr 2 a physical (900, 900) is a CSS (450, 450) — inside an 800x600 pane. - const original = window.devicePixelRatio; + it("divides the payload position by devicePixelRatio on Windows only", async () => { + // Only wry's WebView2 backend hands over *physical* pixels; the macOS and + // GTK ones deliver logical points and `tauri-runtime-wry` does not rescale + // them. At dpr 2 a physical (900, 900) is a CSS (450, 450) — inside the + // 800x600 pane — but the same payload on a HiDPI Mac or Linux box really + // is (900, 900) and belongs to nobody. + const originalDpr = window.devicePixelRatio; + const originalUa = window.navigator.userAgent; Object.defineProperty(window, "devicePixelRatio", { value: 2, configurable: true }); + Object.defineProperty(window.navigator, "userAgent", { + value: "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36", + configurable: true, + }); await renderTab(); await drop(["/host/a.png"], { x: 900, y: 900 }); expect(uploadFileToContainer).toHaveBeenCalled(); - Object.defineProperty(window, "devicePixelRatio", { value: original, configurable: true }); + + Object.defineProperty(window.navigator, "userAgent", { + value: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15", + configurable: true, + }); + vi.mocked(uploadFileToContainer).mockClear(); + await drop(["/host/a.png"], { x: 900, y: 900 }); + expect(uploadFileToContainer).not.toHaveBeenCalled(); + // …and the *unhalved* point still lands, which is the half a HiDPI Mac + // user was losing. + await drop(["/host/a.png"], { x: 400, y: 300 }); + expect(uploadFileToContainer).toHaveBeenCalled(); + + Object.defineProperty(window, "devicePixelRatio", { + value: originalDpr, + configurable: true, + }); + Object.defineProperty(window.navigator, "userAgent", { + value: originalUa, + configurable: true, + }); + }); + + it("accepts a drop that lands on a toast floating over the pane", async () => { + // `ToastHost` is `fixed bottom-4 right-4 z-[60]` and 24rem wide, and its + // error cards stay until dismissed — so a z-order gate asking "is what is + // painted here part of my pane?" made the bottom-right corner of this pane + // refuse drops for as long as one error was on screen. jsdom has no + // `elementFromPoint`, so that branch only runs when a test supplies one. + await renderTab(); + const toastCard = document.createElement("div"); + document.body.appendChild(toastCard); + Object.defineProperty(document, "elementFromPoint", { + configurable: true, + writable: true, + value: () => toastCard, + }); + + await drop(["/host/a.png"], { x: 700, y: 550 }); + expect(uploadFileToContainer).toHaveBeenCalled(); + + delete (document as Partial).elementFromPoint; + toastCard.remove(); + }); + + it("refuses a drop that lands on a dialog painted over the pane", async () => { + await renderTab(); + const backdrop = document.createElement("div"); + backdrop.setAttribute("data-blocks-drop", "true"); + document.body.appendChild(backdrop); + Object.defineProperty(document, "elementFromPoint", { + configurable: true, + writable: true, + value: () => backdrop, + }); + + await drop(["/host/a.png"], { x: 400, y: 300 }); + expect(uploadFileToContainer).not.toHaveBeenCalled(); + + delete (document as Partial).elementFromPoint; + backdrop.remove(); }); it("highlights the pane while a drag hovers it, and drops the highlight on leave", async () => { diff --git a/app/src/components/projects/home/FilesTab.tsx b/app/src/components/projects/home/FilesTab.tsx index 261764e..a917277 100644 --- a/app/src/components/projects/home/FilesTab.tsx +++ b/app/src/components/projects/home/FilesTab.tsx @@ -3,7 +3,7 @@ import { getCurrentWebview } from "@tauri-apps/api/webview"; import { startDrag } from "@crabnebula/tauri-plugin-drag"; import type { FileEntry, Project } from "../../../lib/types"; import { useFileManager } from "../../../hooks/useFileManager"; -import { isDropTarget } from "../../../lib/dropTarget"; +import { classifyDrop, isDropTarget } from "../../../lib/dropTarget"; import { useAppState } from "../../../store/appState"; import Button from "../../ui/Button"; import FileViewerModal from "./FileViewerModal"; @@ -413,8 +413,11 @@ export default function FilesTab({ project }: Props) { // reason `TerminalView` uses it: `dragDropEnabled` is on (the terminal needs // it), which blocks HTML5 drag inside the webview on Windows, and only the // native payload carries real file *paths*. The listener is window-wide, so - // routing is `isDropTarget` — the rect hit test, in CSS pixels, *plus* the - // z-order and "is anything modal on screen" questions a rect cannot answer. + // routing is `classifyDrop` — the rect hit test *plus* the z-order question + // a rect cannot answer: is a modal or a blocking overlay painted at that + // point? (Not "is that element mine": chrome painted over a pane — a toast, + // a button — is not something that swallows a drop, and treating it as such + // made permanent dead zones.) // // Two further filters sit in front of it, both about our own drag-out: // `dragOutInFlight`, and the staged-path check, which is exact because @@ -440,7 +443,23 @@ export default function FilesTab({ project }: Props) { if (payload.type !== "drop") return; setDragOver(false); if (dragOutInFlight.current) return; - if (!isDropTarget(paneRef.current, payload.position)) return; + const verdict = classifyDrop(paneRef.current, payload.position); + // Aimed at this pane and refused anyway: say so. Nothing else would — + // the file just never appears in the listing. + if (verdict === "blocked") { + console.warn( + "[drop] refused: an overlay is covering the drop point", + payload.position, + ); + useAppState.getState().pushToast({ + kind: "info", + message: "File drop ignored", + detail: + "A dialog or full-window overlay is covering that point. Close it and drop the file again.", + }); + return; + } + if (verdict !== "accept") return; // Anything we staged for a drag-out is our own copy of a file that is // already in the container; re-importing it would overwrite the // original with a snapshot. diff --git a/app/src/components/settings/DiskSettings.test.tsx b/app/src/components/settings/DiskSettings.test.tsx index a76d358..595f64f 100644 --- a/app/src/components/settings/DiskSettings.test.tsx +++ b/app/src/components/settings/DiskSettings.test.tsx @@ -943,6 +943,65 @@ describe("DiskSettings", () => { expect(within(dialog).getByRole("alert")).toHaveTextContent(/no space left on device/); }); + it("keeps the semi-safe confirmation open, in the backend's words, when it is refused", async () => { + // A refusal comes back inside `Ok` — the command succeeded at declining — + // so it never reaches `error`, and reading only "did it throw" closed the + // dialog, dropped the tick list, and left the explanation in the outcome + // panel several screens above the row that was clicked. The sentence shown + // is the backend's own: it is the only side that knows which blocker is + // actually holding the project. + 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.mockResolvedValue({ + results: [ + { + target: { kind: "compact_snapshot", project_id: "p-whp" }, + destroyed: null, + ok: false, + freed_bytes: 0, + projected_bytes: null, + message: "Cannot compact whp: a terminal session is still attached.", + } as ReclaimResult, + ], + total_freed_bytes: 0, + }); + + 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( + /a terminal session is still attached/, + ); + // Nothing was removed, so the row that offered the action is still there. + expect( + within(await screen.findByTestId("disk-semi-bucket")).getByRole("button", { + name: "Run…", + }), + ).toBeInTheDocument(); + }); + it("closes the confirmation once the action succeeds", async () => { listReclaimable.mockResolvedValue( plan({ diff --git a/app/src/components/settings/DiskSettings.tsx b/app/src/components/settings/DiskSettings.tsx index 5c37b76..e88206f 100644 --- a/app/src/components/settings/DiskSettings.tsx +++ b/app/src/components/settings/DiskSettings.tsx @@ -158,6 +158,18 @@ export default function DiskSettings() { // five targets can come back with two failures and a real byte total. const failedCount = outcome?.results.filter((r) => !r.ok).length ?? 0; + // What the backend said about the parts it refused, rendered **verbatim**. + // A refusal arrives inside `Ok` — the command succeeded at declining — so it + // never reaches `error`, and the dialog that asked for the work has nothing + // else to show. Not a sentence of our own: the backend is the only side that + // knows which blocker is actually holding the project, and one written here + // would go stale the day that answer improves. + const refusalText = + outcome?.results + .filter((r) => !r.ok) + .map((r) => r.message) + .join(" ") ?? ""; + const tone: StatusTone = scanning ? "unknown" : report ? "ok" : "off"; const statusLabel = scanning ? "Scanning" @@ -706,6 +718,9 @@ export default function DiskSettings() { // minutes, and the dialog reporting it beats it vanishing — // and if it fails, the dialog is the only place the user is // still looking, so it stays open and reports it here. + // `false` covers both a throw and a refusal that came back + // inside `Ok`; either way the work did not happen, so the + // dialog stays put and reports it where the user is looking. const ok = await runReclaim([confirming.target]); setActionFailed(!ok); if (ok) setConfirming(null); @@ -721,7 +736,7 @@ export default function DiskSettings() { line, which this dialog is covering. */} {actionFailed && (

- {error ?? "That did not run. Nothing was changed."} + {error ?? (refusalText || "That did not run. Nothing was changed.")}

)}

{confirming.detail}

@@ -787,7 +802,11 @@ export default function DiskSettings() { // A failure here has to land inside the dialog. The panel's own // error line is at the top of several screens of scroll, and this // dialog was reached from a project row far below it. - error={actionFailed ? (error ?? "That did not run. Nothing was deleted.") : null} + error={ + actionFailed + ? (error ?? (refusalText || "That did not run. Nothing was deleted.")) + : null + } onCancel={closeDestroying} onConfirm={async (typed) => { // The modal stays mounted until the call settles, so its `busy` diff --git a/app/src/components/terminal/TerminalView.test.tsx b/app/src/components/terminal/TerminalView.test.tsx index 3d646d3..819d4b7 100644 --- a/app/src/components/terminal/TerminalView.test.tsx +++ b/app/src/components/terminal/TerminalView.test.tsx @@ -266,12 +266,17 @@ describe("supersedes — who owns the prompt slot", () => { 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. */ + * as a hidden pane. + * + * The rect goes on the *pane wrapper*, which is what the hit test asks + * about: it is what the user sees as the terminal (gutter included), and + * the chrome painted over it — the Following toggle, the URL toast — are + * its children rather than the xterm host's. */ 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"); + const pane = view.container.firstElementChild as HTMLElement | null; + if (!pane) throw new Error("terminal pane not found"); pane.getBoundingClientRect = () => ({ left: 0, @@ -287,6 +292,17 @@ describe("TerminalView — where a dropped file lands", () => { return view; } + /** jsdom has no `elementFromPoint`, so the z-order branch is unreachable + * unless a test supplies one — which is exactly how a gate that refused + * every drop under the Following toggle shipped through this file green. */ + function stubElementFromPoint(top: Element | null) { + Object.defineProperty(document, "elementFromPoint", { + configurable: true, + writable: true, + value: () => top, + }); + } + async function drop(x: number, y: number) { if (!dragDrop.handler) throw new Error("no drag-drop listener registered"); await act(async () => { @@ -333,6 +349,47 @@ describe("TerminalView — where a dropped file lands", () => { await drop(400, 300); expect(vi.mocked(uploadHostFileToTerminal)).toHaveBeenCalledTimes(1); }); + + it("uploads a file dropped onto the always-present Following toggle", async () => { + // The regression this file could not see. The toggle is `absolute top-2 + // right-4 z-50` and is rendered unconditionally, so `elementFromPoint` + // returns *it* for the terminal's top-right corner — and a gate asking + // "is what is painted here inside the xterm host?" answered no, forever, + // with no message and no log line. jsdom never ran that branch. + const view = await mountWithLayout(); + const toggle = view.getByTitle(/Auto-scroll/i); + stubElementFromPoint(toggle); + + await drop(780, 10); + + expect(vi.mocked(uploadHostFileToTerminal)).toHaveBeenCalledWith( + "s1", + "/host/dropped.txt", + ); + delete (document as Partial).elementFromPoint; + }); + + it("refuses — and says so — when a dialog is painted over the drop point", async () => { + await mountWithLayout(); + const backdrop = document.createElement("div"); + backdrop.setAttribute("data-blocks-drop", "true"); + const panel = document.createElement("div"); + panel.setAttribute("aria-modal", "true"); + backdrop.appendChild(panel); + document.body.appendChild(backdrop); + stubElementFromPoint(backdrop); + + await drop(400, 300); + + expect(vi.mocked(uploadHostFileToTerminal)).not.toHaveBeenCalled(); + // A refused drop is otherwise indistinguishable from a broken one. + expect( + useAppState.getState().toasts.some((t) => t.message === "File drop ignored"), + ).toBe(true); + + backdrop.remove(); + delete (document as Partial).elementFromPoint; + }); }); describe("TerminalView — reaching the URL prompt without a mouse", () => { diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 149b6c3..04ec48a 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -21,7 +21,7 @@ import { parseUrlRelayOsc, sanitizeRelayUrl, } from "../../lib/urlRelay"; -import { isDropTarget } from "../../lib/dropTarget"; +import { classifyDrop } from "../../lib/dropTarget"; import UrlToast, { URL_TOAST_PRIMARY_SELECTOR, URL_TOAST_SELECTOR, @@ -237,14 +237,22 @@ export default function TerminalView({ sessionId, active }: Props) { // // 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. + // Files pane: the payload position 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. + // + // The rect asked about is the **pane wrapper**, not the xterm host inside it: + // the pane is what the user sees as "the terminal", gutter included, and the + // chrome painted over it (the Following toggle, the URL toast) is a sibling + // of the host rather than a child. `classifyDrop` answers "is a *blocking + // overlay* here?" rather than "is this element mine?" for the same reason — + // asking the second question turned every pixel under that chrome into a + // permanent dead zone. useEffect(() => { let unlisten: (() => void) | undefined; let cancelled = false; @@ -257,7 +265,29 @@ export default function TerminalView({ sessionId, active }: Props) { (async () => { const un = await getCurrentWebview().onDragDropEvent(async (event) => { if (event.payload.type !== "drop") return; - if (!isDropTarget(containerRef.current, event.payload.position)) return; + const verdict = classifyDrop( + terminalContainerRef.current, + event.payload.position, + ); + // A refused drop is invisible — the file simply does not arrive — so + // the one case where the user aimed at us and we said no gets both a + // log line and something on screen. The toast, not `imagePasteMsg`: + // whatever refused this is painted over the terminal, and `ToastHost` + // sits above it. + if (verdict === "blocked") { + console.warn( + "[drop] refused: an overlay is covering the drop point", + event.payload.position, + ); + useAppState.getState().pushToast({ + kind: "info", + message: "File drop ignored", + detail: + "A dialog or full-window overlay is covering that point. Close it and drop the file again.", + }); + return; + } + if (verdict !== "accept") return; const paths = event.payload.paths ?? []; if (paths.length === 0) return; diff --git a/app/src/components/ui/Modal.test.tsx b/app/src/components/ui/Modal.test.tsx index 0f33602..2a85143 100644 --- a/app/src/components/ui/Modal.test.tsx +++ b/app/src/components/ui/Modal.test.tsx @@ -1,6 +1,8 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, screen, fireEvent, act } from "@testing-library/react"; import Modal from "./Modal"; +import { PaneVisibilityProvider } from "./PaneVisibility"; +import { dropIsBlocked } from "../../lib/dropTarget"; /** * Modal focuses asynchronously via rAF so the panel is laid out first; jsdom @@ -102,6 +104,64 @@ describe("Modal", () => { expect(container).toBeTruthy(); }); + // ------------------------------------------------------------------------- + // Stepping aside with the pane that owns it + // ------------------------------------------------------------------------- + + it("marks its backdrop as swallowing native file drops", async () => { + // The backdrop, not the panel, is what `elementFromPoint` returns for a + // drop released beside the dialog — so it is the element that has to carry + // the marker `lib/dropTarget` looks for. + render( + +

body

+
, + ); + const backdrop = document.querySelector(".fixed.inset-0"); + expect(backdrop).toHaveAttribute("data-blocks-drop", "true"); + expect(dropIsBlocked()).toBe(true); + }); + + it("paints nothing, traps nothing and blocks no drop while its pane is hidden", async () => { + // A dialog portals to `document.body`, where the `hidden` class its pane + // uses to step aside for another tab cannot reach it. Left to itself it + // stayed on screen over the tab the user switched to, kept its Escape + // binding, and refused every native file drop in the window. + const onClose = vi.fn(); + const { rerender } = render( + + + + + , + ); + await flushFocus(); + + const backdrop = document.querySelector(".fixed.inset-0") as HTMLElement; + expect(backdrop.hidden).toBe(true); + expect(backdrop.style.display).toBe("none"); + expect(dropIsBlocked()).toBe(false); + expect(backdrop.contains(document.activeElement)).toBe(false); + + fireEvent.keyDown(document, { key: "Escape" }); + expect(onClose).not.toHaveBeenCalled(); + + // Back on screen: the same dialog, still mounted, resumes everything. + rerender( + + + + + , + ); + await flushFocus(); + expect(backdrop.hidden).toBe(false); + expect(dropIsBlocked()).toBe(true); + expect(screen.getByRole("dialog").contains(document.activeElement)).toBe(true); + fireEvent.keyDown(document, { key: "Escape" }); + expect(onClose).toHaveBeenCalledTimes(1); + }); + it("ignores Escape and overlay clicks when not dismissible", async () => { const onClose = vi.fn(); render( diff --git a/app/src/components/ui/Modal.tsx b/app/src/components/ui/Modal.tsx index cbe48b3..d373f84 100644 --- a/app/src/components/ui/Modal.tsx +++ b/app/src/components/ui/Modal.tsx @@ -1,5 +1,6 @@ import { useCallback, useEffect, useId, useRef, type ReactNode } from "react"; import { createPortal } from "react-dom"; +import { usePaneVisible } from "./PaneVisibility"; const FOCUSABLE_SELECTOR = [ "a[href]", @@ -66,26 +67,43 @@ export default function Modal({ const restoreFocusRef = useRef(null); const titleId = useId(); const descId = useId(); + // A dialog portals to `document.body`, so the `hidden` class its pane uses to + // step aside for another tab cannot reach it. `PaneVisibility` is how it + // finds out, and while it is false this dialog paints nothing, traps + // nothing, and — via `[hidden]` — blocks no native file drop. + const paneVisible = usePaneVisible(); + const paneVisibleRef = useRef(paneVisible); + paneVisibleRef.current = paneVisible; - // Remember what had focus, move focus inside, restore on unmount. + // Remember what had focus, and restore it on unmount — but not if the pane + // is hidden by then: a dialog closed while the user is on another tab would + // otherwise yank focus back to a control they cannot see. useEffect(() => { restoreFocusRef.current = document.activeElement as HTMLElement | null; - const panel = panelRef.current; - if (panel) { - const target = - initialFocusRef?.current ?? focusableWithin(panel)[0] ?? panel; - // Defer so the panel is laid out (offsetParent) before we query it. - requestAnimationFrame(() => target.focus?.()); - } return () => { - restoreFocusRef.current?.focus?.(); + if (paneVisibleRef.current) restoreFocusRef.current?.focus?.(); }; - // Mount/unmount only — re-running would steal focus mid-interaction. - // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - // Escape closes; Tab is trapped inside the panel. + // Move focus inside — on mount, and again whenever the pane comes back. useEffect(() => { + if (!paneVisible) return; + const panel = panelRef.current; + if (!panel) return; + const target = initialFocusRef?.current ?? focusableWithin(panel)[0] ?? panel; + // Defer so the panel is laid out (offsetParent) before we query it. + const frame = requestAnimationFrame(() => target.focus?.()); + return () => cancelAnimationFrame(frame); + // `initialFocusRef` is a ref object; re-running on its identity would steal + // focus mid-interaction. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [paneVisible]); + + // Escape closes; Tab is trapped inside the panel. Neither applies while the + // pane is hidden — those keystrokes belong to whatever the user is looking + // at instead. + useEffect(() => { + if (!paneVisible) return; const onKeyDown = (e: KeyboardEvent) => { if (e.key === "Escape" && dismissible) { e.stopPropagation(); @@ -119,7 +137,7 @@ export default function Modal({ }; document.addEventListener("keydown", onKeyDown, true); return () => document.removeEventListener("keydown", onKeyDown, true); - }, [dismissible, onClose]); + }, [dismissible, onClose, paneVisible]); const handleOverlayClick = useCallback( (e: React.MouseEvent) => { @@ -133,6 +151,15 @@ export default function Modal({ ref={overlayRef} onClick={handleOverlayClick} className="fixed inset-0 bg-black/60 flex items-center justify-center z-50 p-4" + /* The backdrop, not the panel, is what `elementFromPoint` returns for a + drop released beside the dialog — so it is the element that has to say + "I swallow drops". See `lib/dropTarget.ts`. */ + data-blocks-drop="true" + hidden={!paneVisible} + aria-hidden={paneVisible ? undefined : true} + /* `hidden` is a base-layer rule and `flex` is a utility-layer one, so the + attribute alone loses. Inline wins over both. */ + style={paneVisible ? undefined : { display: "none" }} >
+ {children} + + ); +} + +/** True unless an ancestor pane says it is currently hidden. */ +export function usePaneVisible(): boolean { + return useContext(PaneVisibilityContext); +} diff --git a/app/src/hooks/useDiskUsage.test.tsx b/app/src/hooks/useDiskUsage.test.tsx index 2ddfbf9..0c74a54 100644 --- a/app/src/hooks/useDiskUsage.test.tsx +++ b/app/src/hooks/useDiskUsage.test.tsx @@ -368,6 +368,84 @@ describe("useDiskUsage", () => { expect(result.current.error).toMatch(/in use by a running container/); }); + it("calls a refusal that came back inside `Ok` a failure, and keeps the plan", async () => { + // `reclaim` reports per-target results, and a compaction the backend + // declined is `ok: false` with a sentence saying why — not a thrown error. + // Treating that as success closed the dialog that asked for it and took + // the tick list away, even though every object it listed is still there. + getDockerDiskUsage.mockResolvedValue(report("first")); + reclaim.mockResolvedValue({ + results: [ + { + target: { kind: "compact_snapshot", project_id: "p1" }, + destroyed: null, + ok: false, + freed_bytes: 0, + projected_bytes: null, + message: "Cannot compact p1: a terminal session is still attached.", + }, + ], + total_freed_bytes: 0, + }); + const { result } = renderHook(() => useDiskUsage()); + await act(async () => { + await result.current.scan(); + }); + 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.plan).toEqual(plan); + expect(result.current.outcome?.results[0].message).toMatch(/still attached/); + }); + + it("drops the plan when part of a batch did happen", async () => { + getDockerDiskUsage.mockResolvedValue(report("first")); + reclaim.mockResolvedValue({ + results: [ + { target: { kind: "dangling_snapshots" }, destroyed: null, ok: true, freed_bytes: 12, projected_bytes: null, message: "Removed 3 images" }, + { target: { kind: "compact_snapshot", project_id: "p1" }, destroyed: null, ok: false, freed_bytes: 0, projected_bytes: null, message: "Refused" }, + ], + total_freed_bytes: 12, + }); + const { result } = renderHook(() => useDiskUsage()); + await act(async () => { + await result.current.scan(); + }); + let ok: boolean | undefined; + await act(async () => { + ok = await result.current.runReclaim([ + { kind: "dangling_snapshots" }, + { kind: "compact_snapshot", project_id: "p1" }, + ]); + }); + expect(ok).toBe(false); + expect(result.current.plan).toBeNull(); + }); + + it("calls a refused destroy a failure and leaves its row in the plan", async () => { + getDockerDiskUsage.mockResolvedValue(report("first")); + destroyProjectDiskObject.mockResolvedValue({ + target: null, + destroyed: { kind: "home_volume", project_id: "p1" }, + ok: false, + freed_bytes: 0, + projected_bytes: null, + message: "The volume is still attached to a running container.", + }); + const { result } = renderHook(() => useDiskUsage()); + await act(async () => { + await result.current.scan(); + }); + 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.plan).toEqual(plan); + }); + it("reports success when the call came back", async () => { const { result } = renderHook(() => useDiskUsage()); let ok: boolean | undefined; diff --git a/app/src/hooks/useDiskUsage.ts b/app/src/hooks/useDiskUsage.ts index 3f00330..b3f2769 100644 --- a/app/src/hooks/useDiskUsage.ts +++ b/app/src/hooks/useDiskUsage.ts @@ -63,13 +63,25 @@ export interface DiskUsageState { outcome: ReclaimOutcome | null; scan: () => 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. + * Resolves `true` only when the work actually happened. + * + * Two different failures reach here and both have to answer `false`. One is + * the call throwing, which lands in `error`. The other is the backend coming + * back inside `Ok` with a *refusal* — `reclaim` reports per-target results, + * and a compaction declined because the project is busy is a `ReclaimResult` + * with `ok: false` and a sentence saying why. Reading only "did it throw" + * treated that refusal as a success: the confirmation dialog closed, the plan + * was dropped, and the explanation appeared in the outcome panel several + * screens above the row the user had clicked. + * + * Callers that dismiss UI on completion — the confirmation dialogs — must + * only dismiss on `true`, and take the wording from `outcome`'s per-result + * `message` rather than writing their own: the backend's sentence is the one + * that names the real blocker. */ runReclaim: (targets: ReclaimTarget[]) => Promise; - /** Same contract as `runReclaim`: `false` means it failed and `error` says how. */ + /** Same contract as `runReclaim`: `false` means it did not happen, and either + * `error` or the outcome's `message` says why. */ destroy: (target: DestructiveTarget, confirmation: string) => Promise; /** Run the orphaned-snapshot sweep and report what it found *and refused*. */ runSweep: () => Promise; @@ -153,8 +165,15 @@ export function useDiskUsage(): DiskUsageState { // 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; + // + // The exception is a call that removed *nothing at all* because every + // target was refused: those objects are all still there, so the plan + // still describes the daemon accurately and taking it away would leave + // the user re-scanning to get back a list that never went stale. + const everythingRefused = + result.results.length > 0 && result.results.every((r) => !r.ok); + if (!everythingRefused) setPlan(null); + return result.results.every((r) => r.ok); } catch (e) { setError(String(e)); return false; @@ -173,10 +192,11 @@ export function useDiskUsage(): DiskUsageState { 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; + // Same reasoning as `runReclaim`, refusal included: the destructive + // list named an object that is now gone — unless the backend declined, + // in which case it is still there and so is the row for it. + if (result.ok) setPlan(null); + return result.ok; } catch (e) { setError(String(e)); return false; diff --git a/app/src/lib/dropTarget.test.ts b/app/src/lib/dropTarget.test.ts index 07a8f8b..876913c 100644 --- a/app/src/lib/dropTarget.test.ts +++ b/app/src/lib/dropTarget.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, beforeEach, afterEach } from "vitest"; -import { dropIsBlocked, isDropTarget } from "./dropTarget"; +import { classifyDrop, dropIsBlocked, isDropTarget } from "./dropTarget"; function pane(rect: Partial): HTMLElement { const el = document.createElement("div"); @@ -20,12 +20,62 @@ function pane(rect: Partial): HTMLElement { return el; } +/** + * **jsdom has no `elementFromPoint`.** That single fact is how a z-order gate + * that refused every drop under a button, a toast and a dialog shipped green + * through 81 drop tests: the branch was never entered, in any of them. So the + * tests below install one. A test that passes because the environment lacks + * the API under test is not a test. + */ +function stubElementFromPoint(top: Element | null): void { + Object.defineProperty(document, "elementFromPoint", { + configurable: true, + writable: true, + value: () => top, + }); +} + +function removeElementFromPoint(): void { + delete (document as Partial).elementFromPoint; +} + +/** The DOM `ui/Modal` really renders: a marked backdrop around the dialog. */ +function openModal(): { backdrop: HTMLElement; panel: HTMLElement; button: HTMLElement } { + const backdrop = document.createElement("div"); + backdrop.setAttribute("data-blocks-drop", "true"); + const panel = document.createElement("div"); + panel.setAttribute("role", "dialog"); + panel.setAttribute("aria-modal", "true"); + const button = document.createElement("button"); + panel.appendChild(button); + backdrop.appendChild(panel); + document.body.appendChild(backdrop); + return { backdrop, panel, button }; +} + +function withUserAgent(ua: string): void { + Object.defineProperty(window.navigator, "userAgent", { + configurable: true, + value: ua, + }); +} + +const REAL_UA = window.navigator.userAgent; +const REAL_DPR = window.devicePixelRatio; + +function withDevicePixelRatio(value: number): void { + Object.defineProperty(window, "devicePixelRatio", { configurable: true, value }); +} + describe("dropTarget", () => { beforeEach(() => { document.body.innerHTML = ""; }); afterEach(() => { document.body.innerHTML = ""; + removeElementFromPoint(); + withUserAgent(REAL_UA); + withDevicePixelRatio(REAL_DPR); }); it("accepts a point inside the pane", () => { @@ -73,4 +123,188 @@ describe("dropTarget", () => { it("rejects a null pane", () => { expect(isDropTarget(null, { x: 1, y: 1 }, { devicePixelRatio: 1 })).toBe(false); }); + + // --------------------------------------------------------------------- + // Z-order — the branch jsdom cannot reach on its own + // --------------------------------------------------------------------- + + describe("z-order, with elementFromPoint actually present", () => { + it("confirms the environment gap this whole block exists for", () => { + // If jsdom ever grows layout, this fails and the stubs below can be + // reconsidered — but until then, *nothing* reaches the z-order branch + // unless a test puts the API there itself. + expect(typeof document.elementFromPoint).toBe("undefined"); + }); + + it("accepts a drop onto chrome the pane paints over itself", () => { + // The regression. `TerminalView`'s "▼ Following / ▽ Paused" toggle is + // `absolute top-2 right-4 z-50` and is rendered *unconditionally*, as a + // sibling of the xterm host rather than a child — so a gate asking "does + // the pane contain what is painted here?" turned the terminal's top-right + // corner into a dead zone that no user action could clear. + const el = pane({}); + const following = document.createElement("button"); + document.body.appendChild(following); // sibling, not a child of the pane + stubElementFromPoint(following); + + expect(classifyDrop(el, { x: 90, y: 5 }, { devicePixelRatio: 1 })).toBe("accept"); + }); + + it("accepts a drop onto a toast floating above every pane", () => { + // `ToastHost` is `fixed bottom-4 right-4 z-[60]`, 24rem wide, and its + // error cards never time out — so under the containment rule the + // bottom-right corner of both the terminal and the Files pane stopped + // accepting drops for as long as one error stayed on screen. + const el = pane({}); + const toastCard = document.createElement("div"); + document.body.appendChild(toastCard); + stubElementFromPoint(toastCard); + + expect(classifyDrop(el, { x: 95, y: 95 }, { devicePixelRatio: 1 })).toBe("accept"); + }); + + it("accepts a drop onto the pane's own content", () => { + const el = pane({}); + const child = document.createElement("span"); + el.appendChild(child); + stubElementFromPoint(child); + + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); + }); + + it("refuses a drop released onto a dialog's backdrop", () => { + const el = pane({}); + const { backdrop } = openModal(); + stubElementFromPoint(backdrop); + + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + }); + + it("refuses a drop released onto the dialog panel or anything inside it", () => { + const el = pane({}); + const { panel, button } = openModal(); + + stubElementFromPoint(panel); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + + stubElementFromPoint(button); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + }); + + it("refuses a drop under an unmarked overlay that wraps a dialog", () => { + // Belt to the braces: a dialog built without `ui/Modal`'s marked + // backdrop is still refused, because the element painted at the point + // *contains* something modal. + const el = pane({}); + const backdrop = document.createElement("div"); + const panel = document.createElement("div"); + panel.setAttribute("aria-modal", "true"); + backdrop.appendChild(panel); + document.body.appendChild(backdrop); + stubElementFromPoint(backdrop); + + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + }); + + it("refuses a drop under the shutdown overlay", () => { + const el = pane({}); + const overlay = document.createElement("div"); + overlay.setAttribute("data-blocks-drop", "true"); + document.body.appendChild(overlay); + stubElementFromPoint(overlay); + + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + }); + + it("scopes a dialog's refusal to the points it actually covers", () => { + // `dropIsBlocked` is document-wide and `ui/Modal` portals to + // `document.body`, so an open dialog used to refuse every drop in the + // window. Asked per point, a dialog only swallows what lands on it. + const el = pane({}); + openModal(); + const paneContent = document.createElement("span"); + el.appendChild(paneContent); + stubElementFromPoint(paneContent); + + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); + }); + + it("ignores a blocker whose pane has stepped aside", () => { + // `ui/Modal` marks itself `hidden` when the tab that owns it is not the + // one on screen. A dialog left open in project A must not keep refusing + // drops in project B. + const el = pane({}); + const { backdrop } = openModal(); + backdrop.setAttribute("hidden", ""); + + expect(dropIsBlocked()).toBe(false); + stubElementFromPoint(backdrop); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("accept"); + }); + + it("falls back to the document-wide question when the point cannot be resolved", () => { + // `elementFromPoint` answers `null` for a point outside the viewport, and + // `` for a point over nothing in particular. Neither is evidence + // that the pane is clear, so the conservative answer is the old one. + const el = pane({}); + openModal(); + + stubElementFromPoint(null); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + + stubElementFromPoint(document.body); + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + }); + + it("tells a refused drop apart from someone else's drop", () => { + // The caller reports one and stays silent about the other: a drop that + // was aimed at this pane and swallowed by an overlay is invisible unless + // something says so, while a drop into another pane is not ours to + // narrate. + const el = pane({}); + const { backdrop } = openModal(); + stubElementFromPoint(backdrop); + + expect(classifyDrop(el, { x: 50, y: 50 }, { devicePixelRatio: 1 })).toBe("blocked"); + expect(classifyDrop(el, { x: 900, y: 900 }, { devicePixelRatio: 1 })).toBe("elsewhere"); + }); + }); + + // --------------------------------------------------------------------- + // HiDPI + // --------------------------------------------------------------------- + + describe("physical vs logical payload coordinates", () => { + it("divides by devicePixelRatio on Windows", () => { + // wry's WebView2 drag-drop handler passes the OS point through in device + // pixels, so a physical (150,150) at dpr 2 is a CSS (75,75). + withUserAgent("Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36"); + withDevicePixelRatio(2); + expect(isDropTarget(pane({}), { x: 150, y: 150 })).toBe(true); + }); + + it("does not divide on macOS or Linux, where the payload is already logical", () => { + // The macOS and GTK backends deliver logical points and + // `tauri-runtime-wry` does not rescale them. Halving one there aimed the + // hit test at a point the user never touched — harmless while the test + // was a bare rect, and a refused drop once z-order joined in. + withDevicePixelRatio(2); + + withUserAgent("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15"); + expect(isDropTarget(pane({}), { x: 150, y: 150 })).toBe(false); + expect(isDropTarget(pane({}), { x: 50, y: 50 })).toBe(true); + + withUserAgent("Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36"); + expect(isDropTarget(pane({}), { x: 150, y: 150 })).toBe(false); + expect(isDropTarget(pane({}), { x: 50, y: 50 })).toBe(true); + }); + + it("takes an explicit override over the platform guess", () => { + withUserAgent("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15"); + withDevicePixelRatio(2); + expect( + isDropTarget(pane({}), { x: 150, y: 150 }, { physicalPixelPayload: true }), + ).toBe(true); + }); + }); }); diff --git a/app/src/lib/dropTarget.ts b/app/src/lib/dropTarget.ts index 534009e..e4a3266 100644 --- a/app/src/lib/dropTarget.ts +++ b/app/src/lib/dropTarget.ts @@ -11,8 +11,38 @@ * 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. + * So the hit test is: the point is inside my rect, **and** nothing that + * *swallows* drops is painted at that point. + * + * ## The question the z-order test asks — and the one it must not ask + * + * The first version of this asked `el.contains(document.elementFromPoint(x,y))` + * — "is the thing painted here mine?" That is the wrong question, and it + * created permanent dead zones. Panes have chrome painted *over* them that is + * not part of the element handed to this function and does not swallow + * anything: `TerminalView`'s always-present "▼ Following" button, the URL + * toast, and `ToastHost`'s bottom-right stack — which is `fixed` at `z-[60]` + * over *every* pane and whose error cards stay until dismissed. Under the + * containment rule a drop onto any of them was silently refused, forever. + * + * The question that matches the intent is "is a *blocking overlay* painted + * here?". A button, a toast or a tooltip over the pane is not one; a modal + * backdrop is. Anything else painted at the point belongs to the pane's own + * subtree or is chrome that is happy for the drop to fall through to it. + * + * That rule is also what *scopes* the blocking question. `dropIsBlocked` is + * document-wide, and `ui/Modal` portals to `document.body`, so any dialog + * anywhere used to refuse every drop in the window. Asking per-point means a + * dialog only refuses the points it actually covers. + * + * ## jsdom + * + * jsdom implements no layout and has no `elementFromPoint`, so the z-order + * branch cannot be exercised by simply rendering — which is exactly how the + * containment bug shipped green through 81 drop tests. Every test that cares + * about z-order therefore stubs `elementFromPoint` (see `dropTarget.test.ts`), + * and the fallback below — when there is no such API, or it cannot resolve the + * point — is the conservative document-wide question this used to ask. */ export interface DropPoint { @@ -25,57 +55,146 @@ export interface DropPoint { * * `[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). + * is for full-window overlays that are not dialogs (the shutdown overlay), and + * `ui/Modal` puts it on its backdrop as well: the backdrop is what + * `elementFromPoint` returns for a point outside the dialog panel, and it is + * the element that is really covering the pane. */ const BLOCKING_SELECTOR = '[aria-modal="true"],[data-blocks-drop="true"]'; -/** True while a modal or a blocking overlay is on screen. */ +/** + * A blocker inside one of these is in the DOM but not on screen — `ui/Modal` + * marks itself this way when the pane that owns it is not the visible one, so + * a dialog left open in project A stops covering project B the moment the tab + * changes. + */ +const OFFSCREEN_SELECTOR = '[hidden],[aria-hidden="true"]'; + +/** A blocker that is actually painted, rather than merely mounted. */ +function isOnScreen(el: Element): boolean { + return el.closest(OFFSCREEN_SELECTOR) === null; +} + +/** True while a modal or a blocking overlay is on screen *anywhere*. */ export function dropIsBlocked(doc: Document = document): boolean { - return doc.querySelector(BLOCKING_SELECTOR) !== null; + return Array.from(doc.querySelectorAll(BLOCKING_SELECTOR)).some(isOnScreen); } export interface DropTargetOptions { doc?: Document; /** Override the ratio used to convert physical pixels to CSS pixels. */ devicePixelRatio?: number; + /** + * Override the platform question "does this payload arrive in physical + * pixels?". Tests use it; nothing in the app passes it. + */ + physicalPixelPayload?: boolean; } /** - * Whether a native drop at `pos` (physical pixels) belongs to `el`. + * Whether the drop payload's coordinates are physical device pixels. + * + * **Only Windows delivers physical pixels.** `wry`'s WebView2 drag-drop + * handler reads the point from the OS in device pixels and passes it through + * (`wry/src/webview2/drag_drop.rs`), while the macOS and GTK backends deliver + * logical points and `tauri-runtime-wry`'s forwarding does not rescale them. + * Dividing by `devicePixelRatio` unconditionally therefore halved every drop + * position on a HiDPI Mac or Linux box — which used to be a silent + * mis-aimed-but-usually-still-inside-the-pane error and, with a z-order test + * in place, becomes a drop refused because the *halved* point lands on + * something else. + * + * Verified by reading the wry/tauri sources named above. **Not** verified on a + * real HiDPI macOS or GTK machine — neither is available here — which is why + * the divisor is platform-conditional rather than simply deleted: the Windows + * behaviour is the one covered by tests and by the shipped code path. + */ +function payloadIsPhysical( + view: (Window & typeof globalThis) | null, + options: DropTargetOptions, +): boolean { + if (options.physicalPixelPayload !== undefined) return options.physicalPixelPayload; + return /windows/i.test(view?.navigator?.userAgent ?? ""); +} + +/** + * Why a native drop at `pos` did or did not belong to `el`. + * + * - `accept` — it is ours. + * - `blocked` — it landed on our rect, but a modal or a blocking overlay is + * painted there and swallowed it. Worth *saying* to the user: the drop + * visibly did nothing. + * - `elsewhere` — not our drop. Silence is the right response; some other + * pane's listener is about to accept it. * * 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 type DropVerdict = "accept" | "blocked" | "elsewhere"; + +export function classifyDrop( + el: HTMLElement | null | undefined, + pos: DropPoint, + options: DropTargetOptions = {}, +): DropVerdict { + const doc = options.doc ?? el?.ownerDocument ?? document; + + const rect = el?.getBoundingClientRect(); + if (!el || !rect || rect.width === 0 || rect.height === 0) return "elsewhere"; + + const view = doc.defaultView; + const dpr = + options.devicePixelRatio ?? + (payloadIsPhysical(view, options) ? view?.devicePixelRatio || 1 : 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 "elsewhere"; + } + + // 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. + const top = + typeof doc.elementFromPoint === "function" ? doc.elementFromPoint(x, y) : null; + if (top && top !== doc.body && top !== doc.documentElement) { + return coveredByBlocker(top, doc) ? "blocked" : "accept"; + } + + // No layout information — jsdom, or a point the view could not resolve. Fall + // back to the document-wide question, which is the conservative answer: a + // dialog somewhere refuses everything rather than risking a drop landing + // underneath one. + return dropIsBlocked(doc) ? "blocked" : "accept"; +} + +/** + * Is the element painted at the drop point part of something that swallows + * drops? + * + * Two directions, because a dialog is two elements: the panel carries + * `aria-modal`, and the backdrop around it is what is painted over the pane. + * `ui/Modal` marks its own backdrop, so `closest` covers both; the `contains` + * half is the safety net for any overlay that wraps a dialog without marking + * itself, and is deliberately not asked of ``/`` — those contain + * every portal in the app and would make the answer "blocked" always. + */ +function coveredByBlocker(top: Element, doc: Document): boolean { + const nearest = top.closest(BLOCKING_SELECTOR); + if (nearest && isOnScreen(nearest)) return true; + if (top === doc.body || top === doc.documentElement) return false; + const inside = top.querySelector(BLOCKING_SELECTOR); + return inside !== null && isOnScreen(inside); +} + +/** + * Whether a native drop at `pos` (physical pixels on Windows, logical + * elsewhere) belongs to `el`. + */ 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; + return classifyDrop(el, pos, options) === "accept"; } diff --git a/app/src/lib/uploadErrors.test.ts b/app/src/lib/uploadErrors.test.ts index f5c9c6e..e701694 100644 --- a/app/src/lib/uploadErrors.test.ts +++ b/app/src/lib/uploadErrors.test.ts @@ -49,6 +49,42 @@ describe("isFileExistsError", () => { expect(isFileExistsError(42)).toBe(false); expect(isFileExistsError({})).toBe(false); }); + + it("cannot be forged by the name of the file being uploaded", () => { + // The one that mattered. Matching `fileexists` anywhere in a normalised + // error meant a host file called `file-exists.txt` turned *every* failure + // into a collision: the overwrite prompt appeared over a permission error, + // and Replace re-invoked the upload with `overwrite: true`, clobbering + // whatever shared that name in the container. + expect( + isFileExistsError("Failed to upload /host/file-exists.txt: Permission denied"), + ).toBe(false); + expect( + isFileExistsError({ + message: "cp: cannot create regular file '/workspace/FILE_EXISTS.txt'", + }), + ).toBe(false); + expect(isFileExistsError("no space left on device: /host/File Exists.png")).toBe( + false, + ); + // A path that merely ends in the marker is a path, not the marker. + expect(isFileExistsError("cannot stat /workspace/FILE_EXISTS: no such file")).toBe( + false, + ); + // …while the contract's own shape still reads as the refusal it is. + expect( + isFileExistsError("FILE_EXISTS: /workspace/file-exists.txt already exists"), + ).toBe(true); + }); + + it("still reads a wrapped error whose `error` field is a whole sentence", () => { + // `error` is listed as a discriminant field but routinely carries prose, + // so it is held to both standards. + expect( + isFileExistsError({ error: "FILE_EXISTS: /workspace/a.txt already exists" }), + ).toBe(true); + expect(isFileExistsError({ error: "upload of file-exists.txt failed" })).toBe(false); + }); }); describe("fileExistsPath", () => { diff --git a/app/src/lib/uploadErrors.ts b/app/src/lib/uploadErrors.ts index 87fa3b5..d329332 100644 --- a/app/src/lib/uploadErrors.ts +++ b/app/src/lib/uploadErrors.ts @@ -25,6 +25,26 @@ * 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. + * + * **Tolerant about shape is not the same as tolerant about content.** This + * used to normalise the whole error (lower-case, `_`/`-` stripped) and ask + * whether `fileexists` appeared *anywhere* in it — which a host file named + * `file-exists.txt` satisfies on its way through any error at all. Uploading + * that file and hitting "permission denied" therefore raised the overwrite + * prompt, and answering Replace re-invoked the upload with `overwrite: true`: + * an unrelated failure silently promoted into an overwrite of whatever shared + * the name in the container. So the marker now has to appear in a form a + * *filename* cannot produce: + * + * - in prose, the canonical `FILE_EXISTS` (or `FILE-EXISTS`) in upper case, + * standing alone — end of string, or followed by the `:`/`=` of the agreed + * `FILE_EXISTS: ` form. `file-exists.txt`, `FILE_EXISTS.txt` and + * `/workspace/FILE_EXISTS` all fail that, because a filename brings its own + * extension, quote or path separator along with it. + * - in a discriminant field, the *whole* value, case- and separator-insensitive + * (`FileExists`, `file_exists`, `file-exists`, `FileExistsError`) — a + * discriminant is a variant name, not a sentence, so equality is the right + * test and a filename never gets to be one. */ /** Marker the backend puts in the error for "a file with this name is already there". */ @@ -47,6 +67,26 @@ function normaliseKind(value: string): string { const KIND_NEEDLE = normaliseKind(FILE_EXISTS_MARKER); +/** + * The marker standing on its own inside a sentence. + * + * Derived from `FILE_EXISTS_MARKER` so the two cannot drift. Upper case is + * load-bearing (a lower-case `file-exists` is a plausible filename, the + * upper-case token is not), and so is the lookahead: the marker must end the + * string or be followed by the `:`/`=` that introduces the path. That is what + * a path or a filename cannot forge — `FILE_EXISTS.txt`, `"FILE_EXISTS"` and + * `/workspace/FILE_EXISTS` are each rejected by one end or the other. + */ +const PROSE_MARKER = new RegExp( + `(?:^|[\\s:;(\\[{"'\`])${FILE_EXISTS_MARKER.replace(/_/g, "[_-]")}(?=$|[\\s:=])`, +); + +/** A discriminant *is* the refusal, rather than mentioning it. */ +function isFileExistsDiscriminant(value: string): boolean { + const normalised = normaliseKind(value); + return normalised === KIND_NEEDLE || normalised === `${KIND_NEEDLE}error`; +} + function asRecord(e: unknown): Record | null { return typeof e === "object" && e !== null ? (e as Record) : null; } @@ -57,17 +97,40 @@ function asRecord(e: unknown): Record | null { * 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 { prose, kinds } = partitionStrings(e, depth); + return [...prose, ...kinds]; +} + +/** + * The same flattening, but keeping track of *where* each string came from. + * + * A discriminant field and a message field are held to different standards + * (see the module comment), so they cannot be pooled. `error` is listed as a + * discriminant field and yet routinely carries a whole sentence, which is why + * a kind string is tested against both rules and a prose string only against + * the prose one. + */ +function partitionStrings( + e: unknown, + depth = 0, +): { prose: string[]; kinds: string[] } { + if (typeof e === "string") return { prose: [e], kinds: [] }; + if (e instanceof Error) return { prose: [e.message], kinds: [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; + if (!record || depth > 1) return { prose: [], kinds: [] }; + const prose: string[] = []; + const kinds: string[] = []; + const walk = (value: unknown, into: string[]) => { + if (typeof value === "string") into.push(value); + else if (value !== undefined) { + const nested = partitionStrings(value, depth + 1); + prose.push(...nested.prose); + kinds.push(...nested.kinds); + } + }; + for (const field of KIND_FIELDS) walk(record[field], kinds); + for (const field of MESSAGE_FIELDS) walk(record[field], prose); + return { prose, kinds }; } /** @@ -78,7 +141,11 @@ function stringsIn(e: unknown, depth = 0): string[] { * to work. */ export function isFileExistsError(e: unknown): boolean { - return stringsIn(e).some((s) => normaliseKind(s).includes(KIND_NEEDLE)); + const { prose, kinds } = partitionStrings(e); + return ( + kinds.some((s) => isFileExistsDiscriminant(s) || PROSE_MARKER.test(s)) || + prose.some((s) => PROSE_MARKER.test(s)) + ); } /**