From d1a6cc03e2202cd673959966fd58898e7b1d82ae Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sun, 6 Sep 2026 07:10:29 +0000 Subject: [PATCH 1/2] fix(desktop): a manual mirror or sync no longer hangs on a silent spinner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pressing Preview -- or Sync with Mirror armed -- opened the dialog on a spinner and "Scanning both sides…", and that is all it ever showed. Three separate things made that spinner the whole experience. The scan reported nothing. `previewTransfer` awaited the dry run to completion and only then returned, so for however long a full walk of both trees takes -- minutes from a laptop to a server over a WAN -- there was no count, no path, no clock. A scan that was working and one that had wedged looked exactly alike, which is why a slow preview read as a hang. The result was enormous and unused. Every itemized change was accumulated and returned. A dry run of one ordinary source tree here emitted 332,303 of them: ~71MB of `changes` crossing the IPC boundary by structured clone, for a field the renderer never read. It only ever used the summary and the delete list. Nothing was bounded or stoppable. The delete list rendered one DOM node per entry, so a first mirror into an empty destination asked Chromium for hundreds of thousands of rows. And Cancel only hid the dialog: the dry run carried on to the end, uninterruptible, then resolved into a window nobody was looking at. Now: - Progress streams while the scan runs, over a new `event:preview` channel. The dialog shows rsync's own `to-chk` counters, the changes and deletions found so far, the path being compared, and the elapsed time. `total` grows during the run because rsync builds its file list incrementally, so it is presented as an estimate rather than a deadline. - The result carries counts, not changes. `deleteTotal` and `changeTotal` are exact; the enumerated `deletes` stop at 5,000 and the list says how many more there are. The confirm button always counts the true total, so a truncated list never understates what a mirror will remove. - A preview is registered under an id the renderer chooses, so Stop scanning kills the rsync process. Starting a second preview supersedes the first, and a result whose id is stale is discarded -- otherwise a superseded scan could paint its delete list over a different pair's route. - A cancelled scan is never `ok`. It has not established that anything is safe to delete, and `ok` is what gates the confirm. Two things found by driving the real widget rather than reading it: - The delete list did not clip. `max-h-[210px]` sat on the ScrollArea root, whose viewport is `size-full`; with no definite height it grew to fit and the rows printed over the disclosure below. It is now `min-h-0 flex-1` inside a bounded flex column, like every other ScrollArea in the app. - The footer rendered throughout the scan, so a full-strength Mirror button sat under the spinner offering to run a mirror whose delete list did not exist yet. There are no confirm controls until there is something to confirm, and focus moves to Cancel when the result arrives -- `initialFocus` alone stopped being enough once the footer waits. Verified in headless Chromium against the export served with the app's real CSP, at 1360x860 and at the 960x600 minimum, in both themes. Three regression tests cover the streaming, the cap, and the cancel; each was confirmed to fail with its bug reintroduced. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52 --- apps/desktop/electron/main/ipc.ts | 9 +- .../electron/main/services/preview.test.ts | 225 ++++++++++++++++++ .../electron/main/services/transfers.ts | 170 ++++++++++++- apps/desktop/electron/preload/index.ts | 6 + apps/desktop/electron/shared/contract.ts | 21 ++ apps/desktop/src/app/page.tsx | 70 +++++- .../desktop/src/components/transfer-panel.tsx | 216 +++++++++++++---- apps/desktop/src/lib/api.ts | 28 ++- 8 files changed, 681 insertions(+), 64 deletions(-) create mode 100644 apps/desktop/electron/main/services/preview.test.ts diff --git a/apps/desktop/electron/main/ipc.ts b/apps/desktop/electron/main/ipc.ts index 6a2e12f..bfcfc5c 100644 --- a/apps/desktop/electron/main/ipc.ts +++ b/apps/desktop/electron/main/ipc.ts @@ -17,6 +17,7 @@ import { FleetRunIdSchema, IPC, JobIdSchema, + PreviewRequestSchema, PathSchema, RemotePathRequestSchema, ProfileSaveSchema, @@ -42,7 +43,7 @@ import { } from './services/fleet.js' import { browserFor, dropSession, sessionFor } from './services/sessions.js' import { store } from './services/store.js' -import { cancelTransfer, previewTransfer, saveProfile, startTransfer } from './services/transfers.js' +import { cancelPreview, cancelTransfer, previewTransfer, saveProfile, startTransfer } from './services/transfers.js' /** * Every handler validates its input with Zod before doing anything, and every @@ -299,7 +300,11 @@ export function registerIpc(): void { // --- transfers ----------------------------------------------------------- - handle(IPC.transfersPreview, TransferRequestSchema, async (request) => previewTransfer(request)) + handle(IPC.transfersPreview, PreviewRequestSchema, async (request, event) => previewTransfer(request, event.sender)) + + handle(IPC.transfersPreviewCancel, z.object({ previewId: JobIdSchema }), async ({ previewId }) => + cancelPreview(previewId), + ) handle(IPC.transfersStart, TransferRequestSchema, async (request, event) => startTransfer(request, event.sender)) diff --git a/apps/desktop/electron/main/services/preview.test.ts b/apps/desktop/electron/main/services/preview.test.ts new file mode 100644 index 0000000..7cd4bde --- /dev/null +++ b/apps/desktop/electron/main/services/preview.test.ts @@ -0,0 +1,225 @@ +import { describe, expect, it, vi } from 'vitest' +import type { RsyncEvent } from '@diskpush/schemas' + +/** + * A run whose events this test pushes by hand, so a scan can be inspected + * while it is still going -- which is the whole point of what is being tested. + */ +function fakeRun() { + const queue: RsyncEvent[] = [] + const waiters: Array<(result: IteratorResult) => void> = [] + let done = false + let killed = false + + const push = (event: RsyncEvent) => { + const waiter = waiters.shift() + if (waiter) waiter({ value: event, done: false }) + else queue.push(event) + } + const end = () => { + done = true + while (waiters.length > 0) waiters.shift()!({ value: undefined as never, done: true }) + } + + return { + push, + end, + get killed() { + return killed + }, + handle: { + cancel: () => { + killed = true + // A real SIGINT makes rsync exit, which is what closes the stream. + push({ type: 'exit', code: 20, signal: 'SIGINT', resumable: true, message: 'Interrupted.' } as RsyncEvent) + end() + }, + events: { + [Symbol.asyncIterator]() { + return { + next(): Promise> { + const queued = queue.shift() + if (queued) return Promise.resolve({ value: queued, done: false }) + if (done) return Promise.resolve({ value: undefined as never, done: true }) + return new Promise((resolve) => waiters.push(resolve)) + }, + } + }, + }, + }, + } +} + +/** A stable holder, so the mock factory reads whichever run a test installed. */ +const runs = { current: fakeRun() } + +vi.mock('./store.js', () => ({ + store: async () => ({ getSetting: async (_key: string, fallback: T) => fallback }), +})) + +vi.mock('@diskpush/rsync-core', () => ({ + planTransfer: () => ({ + binary: 'rsync', + args: [], + display: 'rsync --dry-run a/ b/', + controlDisplay: null, + warnings: [], + }), + runPlan: () => runs.current.handle, + parseRsyncCapabilities: () => ({}), + intersectCapabilities: (a: unknown) => a, + unknownCapabilities: () => ({}), +})) + +const { PREVIEW_DELETE_LIMIT, cancelPreview, previewTransfer } = await import('./transfers.js') + +const REQUEST = { + source: { type: 'local' as const, path: '/src/' }, + destination: { type: 'local' as const, path: '/dst/' }, + options: { + archive: true, + checksum: false, + compression: 'auto' as const, + deleteMode: 'delay' as const, + hardLinks: false, + acls: false, + xattrs: false, + numericIds: false, + update: false, + ignoreExisting: false, + existingOnly: false, + inplace: false, + excludes: [], + includes: [], + bwlimit: null, + maxSize: null, + minSize: null, + }, + deletesConfirmed: false, +} + +type Progress = { checked: number; total: number; changes: number; deletes: number; currentPath: string } +type Sent = { channel: string; payload: { previewId: string; progress: Progress } } + +function sender() { + const sent: Sent[] = [] + return { + sent, + webContents: { + isDestroyed: () => false, + send: (channel: string, payload: unknown) => sent.push({ channel, payload } as Sent), + } as never, + } +} + +const change = (action: string, path: string): RsyncEvent => + ({ type: 'change', change: { action, path, itemize: null, isDirectory: false, size: null } }) as RsyncEvent + +/** + * Waits for a condition rather than for a duration. + * + * The preview's first await is a real `rsync --version`, so a fixed sleep is a + * race that passes on this machine and fails on a slower one. + */ +async function until(condition: () => boolean): Promise { + for (let attempt = 0; attempt < 400; attempt += 1) { + if (condition()) return + await new Promise((resolve) => setTimeout(resolve, 5)) + } + throw new Error('condition never held') +} + +describe('previewTransfer', () => { + /* + * The bug: the dialog showed a bare spinner for the whole scan, because + * nothing was reported until the dry run finished. A scan of a remote tree + * takes minutes, so every slow preview was indistinguishable from a hang -- + * which is exactly what it was reported as. + */ + it('streams progress while the scan is still running', async () => { + runs.current = fakeRun() + const target = sender() + const pending = previewTransfer({ ...REQUEST, previewId: 'p1' }, target.webContents) + + // Reported before rsync has said anything at all. + await until(() => target.sent.length > 0) + expect(target.sent[0]!.channel).toBe('event:preview') + + runs.current.push({ + type: 'progress', + progress: { + bytesTransferred: 0, + percent: 0, + bytesPerSecond: 0, + elapsedSeconds: 0, + filesTransferred: 1, + filesRemaining: 400, + filesTotal: 1000, + }, + } as RsyncEvent) + runs.current.push(change('add', 'photos/a.jpg')) + + runs.current.push({ type: 'exit', code: 0, signal: null, resumable: false, message: 'Done.' } as RsyncEvent) + runs.current.end() + await pending + + const last = target.sent.at(-1)!.payload + expect(last.previewId).toBe('p1') + // rsync counts down, so 400 of 1000 left means 600 compared. + expect(last.progress.checked).toBe(600) + expect(last.progress.total).toBe(1000) + expect(last.progress.currentPath).toBe('photos/a.jpg') + expect(last.progress.changes).toBe(1) + }) + + /* + * The bug: every change was accumulated and returned. An ordinary source + * tree produces a few hundred thousand of them -- ~70MB across the IPC + * boundary by structured clone, for a field the renderer never read, plus a + * DOM node per delete. The counts are exact; the enumeration is not endless. + */ + it('returns counts rather than the change list, and caps the enumerated deletes', async () => { + runs.current = fakeRun() + const target = sender() + const pending = previewTransfer({ ...REQUEST, previewId: 'p2' }, target.webContents) + + const total = PREVIEW_DELETE_LIMIT + 250 + for (let index = 0; index < total; index += 1) runs.current.push(change('delete', `old/${index}.bin`)) + runs.current.push(change('add', 'new/one.bin')) + runs.current.push({ type: 'exit', code: 0, signal: null, resumable: false, message: 'Done.' } as RsyncEvent) + runs.current.end() + + const result = await pending + expect(result).not.toHaveProperty('changes') + expect(result.deleteTotal).toBe(total) + expect(result.deletes).toHaveLength(PREVIEW_DELETE_LIMIT) + expect(result.changeTotal).toBe(total + 1) + expect(result.summary.delete).toBe(total) + expect(result.summary.add).toBe(1) + expect(result.ok).toBe(true) + }) + + /* + * The bug: closing the dialog only hid it. The dry run carried on to the end + * with no way to stop it, then resolved into a window nobody was looking at. + */ + it('cancels a running scan, and never reports a stopped scan as a usable result', async () => { + runs.current = fakeRun() + const target = sender() + const pending = previewTransfer({ ...REQUEST, previewId: 'p3' }, target.webContents) + + await until(() => target.sent.length > 0) + runs.current.push(change('delete', 'old/one.bin')) + + await until(() => cancelPreview('p3')) + const result = await pending + + expect(runs.current.killed).toBe(true) + expect(result.cancelled).toBe(true) + // `ok` gates the confirm button. A scan that was stopped has not + // established that anything is safe to delete. + expect(result.ok).toBe(false) + // The registration is gone, so a late Cancel cannot kill an unrelated run. + expect(cancelPreview('p3')).toBe(false) + }) +}) diff --git a/apps/desktop/electron/main/services/transfers.ts b/apps/desktop/electron/main/services/transfers.ts index 6c1163d..bc78074 100644 --- a/apps/desktop/electron/main/services/transfers.ts +++ b/apps/desktop/electron/main/services/transfers.ts @@ -5,12 +5,11 @@ import { parseRsyncCapabilities, planTransfer, runPlan, - runToCompletion, unknownCapabilities, type ExecutionPlan, type RsyncCapabilities, } from '@diskpush/rsync-core' -import { defaultRsyncOptions, summarizeChanges, type Change, type Endpoint, type RsyncOptions } from '@diskpush/schemas' +import { defaultRsyncOptions, summarizeChanges, type Endpoint, type RsyncOptions } from '@diskpush/schemas' import { execFile } from 'node:child_process' import { promisify } from 'node:util' import { IPC, type EndpointRef, type TransferOptions, type TransferRequest } from '../../shared/contract.js' @@ -129,33 +128,182 @@ async function buildPlan(request: TransferRequest, overrides: Partial + /** Capped at PREVIEW_DELETE_LIMIT; `deleteTotal` is the real number. */ deletes: string[] + deleteTotal: number + /** Every change rsync reported, deletions included. */ + changeTotal: number command: string control: string | null warnings: string[] ok: boolean message: string + /** True when the user stopped the scan rather than it finishing. */ + cancelled: boolean } -/** The dry run behind Preview Changes and behind every mirror. */ -export async function previewTransfer(request: TransferRequest): Promise { +type RunningPreview = { cancel: () => void; cancelled: boolean } +const previews = new Map() + +/** How often scan progress is pushed to the renderer, at most. */ +const PREVIEW_TICK_MS = 120 + +/** + * The dry run behind Preview Changes and behind every mirror. + * + * Three things it deliberately does not do, each of which it used to. + * + * It does not accumulate every change. A dry run of an ordinary source tree + * emits a few hundred thousand of them; keeping the array and returning it + * meant ~70MB crossing the IPC boundary by structured clone, for a field the + * renderer never read. Only the counts and the capped delete list survive. + * + * It does not run silently. Progress is streamed as it goes, so the dialog can + * show what is being compared instead of a spinner that is indistinguishable + * from a hang. + * + * It does not run unstoppably. The handle is registered under `previewId`, so + * Cancel kills the rsync process rather than only hiding the dialog and + * leaving the scan to finish into nothing. + */ +export async function previewTransfer( + request: TransferRequest & { previewId: string }, + sender: WebContents, +): Promise { const plan = await buildPlan(request, { dryRun: true }) - const result = await runToCompletion(plan) + + const summary = summarizeChanges([]) + const deletes: string[] = [] + let deleteTotal = 0 + let changeTotal = 0 + let checked = 0 + let total = 0 + let currentPath = '' + const startedAt = Date.now() + + const handle = runPlan(plan) + const entry: RunningPreview = { cancel: handle.cancel, cancelled: false } + previews.set(request.previewId, entry) + + let lastTick = 0 + const emit = (force: boolean) => { + const now = Date.now() + if (!force && now - lastTick < PREVIEW_TICK_MS) return + lastTick = now + if (sender.isDestroyed()) return + sender.send(IPC.eventPreview, { + previewId: request.previewId, + progress: { + checked, + total, + changes: changeTotal, + deletes: deleteTotal, + currentPath, + elapsedSeconds: Math.round((now - startedAt) / 1000), + } satisfies PreviewProgress, + }) + } + + // Sent before rsync has said anything, so the dialog starts with a scan it + // can see rather than with an empty panel it has to explain. + emit(true) + + let ok = false + let message = '' + + try { + for await (const event of handle.events) { + switch (event.type) { + case 'change': { + changeTotal += 1 + summary[event.change.action] += 1 + currentPath = event.change.path + if (event.change.action === 'delete') { + deleteTotal += 1 + if (deletes.length < PREVIEW_DELETE_LIMIT) deletes.push(event.change.path) + } + emit(false) + break + } + case 'progress': { + // rsync counts down: `to-chk=remaining/total`. + if (event.progress.filesTotal !== null) { + total = event.progress.filesTotal + checked = event.progress.filesTotal - (event.progress.filesRemaining ?? 0) + } + emit(false) + break + } + case 'exit': { + ok = event.code === 0 || event.code === 24 + message = event.message + break + } + default: + break + } + } + } finally { + previews.delete(request.previewId) + } + + emit(true) + return { - changes: result.changes, - summary: summarizeChanges(result.changes), - deletes: result.changes.filter((change) => change.action === 'delete').map((change) => change.path), + summary, + deletes, + deleteTotal, + changeTotal, command: plan.display, control: plan.controlDisplay ?? null, warnings: plan.warnings, - ok: result.ok, - message: result.message, + // A scan the user stopped is not a scan that failed, and it must never be + // reported as one: `ok` gates the confirm button, and a cancelled preview + // has not established that anything is safe to delete. + ok: entry.cancelled ? false : ok, + message: entry.cancelled ? 'Scan cancelled.' : message, + cancelled: entry.cancelled, } } +/** Stops a dry run that is still scanning. False when it already finished. */ +export function cancelPreview(previewId: string): boolean { + const entry = previews.get(previewId) + if (!entry) return false + entry.cancelled = true + entry.cancel() + return true +} + export type StartedJob = { jobId: string; command: string; control: string | null; warnings: string[] } export async function startTransfer(request: TransferRequest, sender: WebContents): Promise { diff --git a/apps/desktop/electron/preload/index.ts b/apps/desktop/electron/preload/index.ts index f643397..7869ac2 100644 --- a/apps/desktop/electron/preload/index.ts +++ b/apps/desktop/electron/preload/index.ts @@ -41,6 +41,7 @@ const api = { }, transfers: { preview: (request: unknown) => call(IPC.transfersPreview, request), + cancelPreview: (previewId: string) => call(IPC.transfersPreviewCancel, { previewId }), start: (request: unknown) => call(IPC.transfersStart, request), cancel: (jobId: string) => call(IPC.transfersCancel, { jobId }), list: (limit = 50) => call(IPC.transfersList, { limit }), @@ -87,6 +88,11 @@ const api = { ipcRenderer.on(IPC.eventFleet, wrapped) return () => ipcRenderer.off(IPC.eventFleet, wrapped) }, + onPreview(listener: (payload: { previewId: string; progress: unknown }) => void): () => void { + const wrapped = (_event: unknown, payload: { previewId: string; progress: unknown }) => listener(payload) + ipcRenderer.on(IPC.eventPreview, wrapped) + return () => ipcRenderer.off(IPC.eventPreview, wrapped) + }, }, } diff --git a/apps/desktop/electron/shared/contract.ts b/apps/desktop/electron/shared/contract.ts index 1088086..78f10eb 100644 --- a/apps/desktop/electron/shared/contract.ts +++ b/apps/desktop/electron/shared/contract.ts @@ -30,6 +30,7 @@ export const IPC = { fsCreateFileLocal: 'fs:create-file-local', transfersPreview: 'transfers:preview', + transfersPreviewCancel: 'transfers:preview-cancel', transfersStart: 'transfers:start', transfersCancel: 'transfers:cancel', transfersList: 'transfers:list', @@ -59,6 +60,14 @@ export const IPC = { eventTransfer: 'event:transfer', /** Main -> renderer, one channel carrying every fleet event. */ eventFleet: 'event:fleet', + /** + * Main -> renderer, live progress for a dry run that has not finished yet. + * + * A preview is a full scan of both sides and there is no upper bound on how + * long that takes. Without this the dialog could only sit on a spinner, so + * an eight-minute scan and a wedged one looked exactly alike. + */ + eventPreview: 'event:preview', } as const /** A path the renderer asked for. Length-capped, and never joined by the renderer. */ @@ -130,6 +139,18 @@ export type TransferRequest = z.infer export const JobIdSchema = z.string().uuid() +/** + * A preview, carrying the id the renderer will cancel it by. + * + * The id is chosen by the renderer rather than returned by the call, because + * the call does not return until the scan is over -- which is exactly the + * window in which it has to be cancellable. + */ +export const PreviewRequestSchema = TransferRequestSchema.extend({ + previewId: JobIdSchema, +}) +export type PreviewRequest = z.infer + /** * Saving the current pane pair and options as a named profile. * diff --git a/apps/desktop/src/app/page.tsx b/apps/desktop/src/app/page.tsx index 4e5cc0d..61c7d41 100644 --- a/apps/desktop/src/app/page.tsx +++ b/apps/desktop/src/app/page.tsx @@ -1,6 +1,6 @@ 'use client' -import { useCallback, useEffect, useMemo, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import Image from 'next/image' import { ArrowLeftRight, @@ -24,7 +24,15 @@ import { TransferRail } from '@/components/transfer-rail' import { MirrorPreviewDialog, TransferBand, type ActiveJob } from '@/components/transfer-panel' import { Button } from '@/components/ui/button' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' -import { api, unwrap, type Connection, type PreviewResult, type SyncProfile, type TransferEvent } from '@/lib/api' +import { + api, + unwrap, + type Connection, + type PreviewProgress, + type PreviewResult, + type SyncProfile, + type TransferEvent, +} from '@/lib/api' import { withTrailingSlash } from '@/lib/format' /** A row in the header menu. Plain button, styled once. */ @@ -87,7 +95,18 @@ export default function Workspace() { const [mirror, setMirror] = useState(false) const [trust, setTrust] = useState(false) const [preview, setPreview] = useState(null) + const [previewProgress, setPreviewProgress] = useState(null) const [previewOpen, setPreviewOpen] = useState(false) + /** + * The scan the dialog is currently showing. + * + * A preview cannot be identified by "the one that is running", because + * stopping one and starting another leaves both in flight: the first call + * still resolves, and without this it would paint its delete list over the + * second one's route. Confirming that would mirror a pair the user never + * saw. Every result and every progress event is matched against this id. + */ + const previewIdRef = useRef(null) const [job, setJob] = useState(null) const [error, setError] = useState(null) const [showConnection, setShowConnection] = useState(false) @@ -183,6 +202,15 @@ export default function Workspace() { return bridge.events.onTransfer(({ jobId, event }) => setJob((current) => reduceJob(current, jobId, event))) }, []) + useEffect(() => { + const bridge = api() + if (!bridge) return + return bridge.events.onPreview(({ previewId, progress }) => { + if (previewId !== previewIdRef.current) return + setPreviewProgress(progress) + }) + }, []) + const source = direction === 'ltr' ? left : right const destination = direction === 'ltr' ? right : left const allConnections = useMemo(() => [...saved, ...sshConfig], [saved, sshConfig]) @@ -200,13 +228,43 @@ export default function Workspace() { [source, destination, mirror], ) + /** + * Closes the dialog and stops the scan behind it. + * + * Closing used to only hide the dialog. The rsync dry run carried on to the + * end -- minutes of a remote tree walk nobody could see or interrupt -- and + * then resolved into a dialog that was no longer open. + */ + const closePreview = useCallback(() => { + const previewId = previewIdRef.current + previewIdRef.current = null + setPreviewOpen(false) + setPreviewProgress(null) + if (previewId) void api()?.transfers.cancelPreview(previewId) + }, []) + const runPreview = useCallback(async () => { + const previewId = crypto.randomUUID() + // Supersedes any scan already running, so pressing Preview twice does not + // leave two rsync processes walking the same trees. + const superseded = previewIdRef.current + if (superseded) void api()?.transfers.cancelPreview(superseded) + + previewIdRef.current = previewId setError(null) setPreview(null) + setPreviewProgress(null) setPreviewOpen(true) try { - setPreview(await unwrap(api()?.transfers.preview(request))) + const result = await unwrap(api()?.transfers.preview({ ...request, previewId })) + if (previewIdRef.current !== previewId) return + // A scan the user stopped is not a result. Showing it would offer a + // confirm button for a delete list that was never finished. + if (result.cancelled) return + setPreview(result) } catch (caught) { + if (previewIdRef.current !== previewId) return + previewIdRef.current = null setPreviewOpen(false) setError(caught instanceof Error ? caught.message : String(caught)) } @@ -215,7 +273,9 @@ export default function Workspace() { const start = useCallback( async (deletesConfirmed: boolean) => { setError(null) + previewIdRef.current = null setPreviewOpen(false) + setPreviewProgress(null) try { const started = await unwrap(api()?.transfers.start({ ...request, deletesConfirmed })) setJob({ @@ -575,11 +635,13 @@ export default function Workspace() { setPreviewOpen(false)} + onCancel={closePreview} + onStopScan={closePreview} onConfirm={() => void start(true)} /> diff --git a/apps/desktop/src/components/transfer-panel.tsx b/apps/desktop/src/components/transfer-panel.tsx index 6b42459..f10fc9e 100644 --- a/apps/desktop/src/components/transfer-panel.tsx +++ b/apps/desktop/src/components/transfer-panel.tsx @@ -1,8 +1,8 @@ 'use client' -import { useRef } from 'react' +import { useEffect, useRef } from 'react' import { ArrowRight, ChevronRight, CircleCheck, Trash2, TriangleAlert } from 'lucide-react' -import type { PreviewResult } from '@/lib/api' +import type { PreviewProgress, PreviewResult } from '@/lib/api' import { formatBytes, formatDuration, formatRate } from '@/lib/format' import { Button } from '@/components/ui/button' import { Checkbox } from '@/components/ui/checkbox' @@ -24,33 +24,138 @@ export type ActiveJob = { message: string } +/** + * What the scan panel shows before rsync has said anything. + * + * A zeroed progress rather than an absent one, so the panel has the same shape + * from the first frame and does not rearrange itself once the first event + * lands. + */ +const EMPTY_PROGRESS: PreviewProgress = { + checked: 0, + total: 0, + changes: 0, + deletes: 0, + currentPath: '', + elapsedSeconds: 0, +} + +/** + * Live progress while both sides are being compared. + * + * This used to be a spinner and the words "Scanning both sides...", which is + * all a person saw for however long a full scan of two trees takes -- minutes + * over a WAN. A scan that is working and a scan that has wedged looked + * identical, so every slow preview read as a hang. It now shows rsync's own + * counters, so the panel moves whenever the scan does. + * + * `total` grows during the run, because rsync builds its file list + * incrementally. The bar is therefore explicitly an estimate, and the counts + * beside it are the honest numbers. + */ +function ScanProgress({ progress, onCancel }: { progress: PreviewProgress; onCancel: () => void }) { + const { checked, total, changes, deletes, currentPath, elapsedSeconds } = progress + const percent = total > 0 ? Math.min(100, Math.round((checked / total) * 100)) : 0 + + return ( +
+
+ + Comparing both sides + + {formatDuration(elapsedSeconds)} + +
+ + + +
+ + {checked.toLocaleString()} + {total > 0 ? <> of about {total.toLocaleString()} : null}{' '} + compared + + + {changes.toLocaleString()} to change + + {deletes > 0 ? ( + + {deletes.toLocaleString()} to delete + + ) : null} +
+ + {/* + The path currently being compared. Without it a stalled scan and a slow + one still look the same, because the counters can sit still for a long + time on one large directory. + */} +

{currentPath || 'building the file list…'}

+ + {/* + Stopping is a real stop. Closing the dialog used to hide it and leave + the dry run to finish into a window nobody was looking at, which is how + a mistyped path cost you a full scan you could not interrupt. + */} + +
+ ) +} + /** * The delete preview. * * Every proposed deletion is listed rather than summarised: "87 files" is not - * something anyone can consent to. The confirm button says what it does. + * something anyone can consent to. Past PREVIEW_DELETE_LIMIT the enumeration + * stops and says so, because a first mirror into an empty destination proposes + * hundreds of thousands and a DOM node each is what froze the window. The + * count on the confirm button is always the true one. */ export function MirrorPreviewDialog({ preview, + progress, open, route, trust, onTrustChange, onCancel, + onStopScan, onConfirm, }: { preview: PreviewResult | null + progress: PreviewProgress | null open: boolean route: string trust: boolean onTrustChange: (value: boolean) => void onCancel: () => void + onStopScan: () => void onConfirm: () => void }) { const deletes = preview?.deletes ?? [] + const deleteTotal = preview?.deleteTotal ?? 0 + const hidden = Math.max(0, deleteTotal - deletes.length) const summary = preview?.summary const cancelRef = useRef(null) + /* + * Focus lands on Cancel the moment the result does. + * + * `initialFocus` alone stopped being enough once the footer waits for the + * scan: at open time there is no Cancel button to focus, so when the delete + * list finally appeared the focus was still on the dialog and the next Enter + * could reach the confirm. This dialog's whole job is to make an + * irreversible delete deliberate. + */ + useEffect(() => { + if (preview) cancelRef.current?.focus() + }, [preview]) + return ( (next ? undefined : onCancel())}> {!preview ? ( -
- - Scanning both sides… -
+ ) : !preview.ok ? (

{preview.message}

) : ( @@ -88,7 +190,7 @@ export function MirrorPreviewDialog({ ['Add', summary?.add ?? 0, 'text-ok'], ['Update', summary?.update ?? 0, 'text-primary'], ['Unchanged', summary?.unchanged ?? 0, 'text-muted-foreground'], - ['Delete', deletes.length, 'text-destructive'], + ['Delete', deleteTotal, 'text-destructive'], ] as const ).map(([label, value, tone]) => (
@@ -99,12 +201,12 @@ export function MirrorPreviewDialog({
- {deletes.length > 0 ? ( + {deleteTotal > 0 ? (
- {deletes.length.toLocaleString()} file{deletes.length === 1 ? '' : 's'} + {deleteTotal.toLocaleString()} file{deleteTotal === 1 ? '' : 's'} {' '} at the destination will be deleted. This cannot be undone. @@ -116,22 +218,36 @@ export function MirrorPreviewDialog({ )}
- {deletes.length > 0 ? ( + {deleteTotal > 0 ? (
Files to be deleted
{/* max-, not a fixed height: two doomed files used to sit at the top of a 210px well of empty space. */} - - {deletes.map((path) => ( -
- − - {path} -
- ))} - + {/* + The scroll box needs a definite height, which `max-h` on the + ScrollArea root alone never gave it: its viewport is + `size-full`, so it grew to fit and the rows printed straight + over the disclosure below. Every other ScrollArea in the app + is `min-h-0 flex-1` inside a flex column, and so is this one. + */} +
+ + {deletes.map((path) => ( +
+ − + {path} +
+ ))} + {hidden > 0 ? ( +
+ …and {hidden.toLocaleString()} more, not listed +
+ ) : null} + +
) : null} @@ -159,30 +275,38 @@ export function MirrorPreviewDialog({ )}
- - -
- {/* - Focus opens on Cancel, not on the confirm and not on the trust - checkbox it used to land on. This dialog's whole job is to make - an irreversible delete deliberate, so a stray Enter or Space has - to hit the harmless control. - */} - - -
- + {/* + No confirm controls while there is nothing to confirm. The footer used + to render throughout the scan, so a full-strength Mirror button sat + under the spinner offering to run a mirror whose delete list did not + exist yet -- inert, which reads as a control that ignores you. + */} + {preview ? ( + + +
+ {/* + Focus opens on Cancel, not on the confirm and not on the trust + checkbox it used to land on. This dialog's whole job is to make + an irreversible delete deliberate, so a stray Enter or Space has + to hit the harmless control. + */} + + +
+ + ) : null} ) diff --git a/apps/desktop/src/lib/api.ts b/apps/desktop/src/lib/api.ts index 514d3f8..5135458 100644 --- a/apps/desktop/src/lib/api.ts +++ b/apps/desktop/src/lib/api.ts @@ -55,14 +55,38 @@ export type Change = { } export type PreviewResult = { - changes: Change[] summary: Record + /** + * Capped by the main process. `deleteTotal` is the true number and is what + * the confirm button counts, so a truncated list never understates what a + * mirror is about to remove. + */ deletes: string[] + deleteTotal: number + changeTotal: number command: string control: string | null warnings: string[] ok: boolean message: string + /** The user stopped the scan. Not a failure, and not a result to act on. */ + cancelled: boolean +} + +/** + * Live progress for a dry run that is still going. + * + * `total` comes from rsync's own `to-chk` counter and grows while the file + * list is still being built, so it is shown as a moving count rather than as a + * deadline. + */ +export type PreviewProgress = { + checked: number + total: number + changes: number + deletes: number + currentPath: string + elapsedSeconds: number } /** A saved source/destination pair with its options. Runnable from the CLI too. */ @@ -234,6 +258,7 @@ type Api = { } transfers: { preview(request: unknown): Promise> + cancelPreview(previewId: string): Promise> start(request: unknown): Promise> cancel(jobId: string): Promise> list(limit?: number): Promise> @@ -269,6 +294,7 @@ type Api = { events: { onTransfer(listener: (payload: { jobId: string; event: TransferEvent }) => void): () => void onFleet(listener: (payload: { runId: string; event: FleetEvent }) => void): () => void + onPreview(listener: (payload: { previewId: string; progress: PreviewProgress }) => void): () => void } } From 45c44ffb1101322e16a335b407a599d2b613ba9a Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Sun, 6 Sep 2026 07:12:34 +0000 Subject: [PATCH 2/2] fix(desktop): quitting stops a running scan too `cancelAll` on before-quit stopped transfers but not previews, so closing the window mid-scan left an rsync and an ssh walking a remote tree with nothing left to report to. A dry run is read-only, which is why it went unnoticed, but it is still two processes and a remote session. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52 --- .../electron/main/services/preview.test.ts | 20 ++++++++++++++++++- .../electron/main/services/transfers.ts | 7 +++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/apps/desktop/electron/main/services/preview.test.ts b/apps/desktop/electron/main/services/preview.test.ts index 7cd4bde..714ae95 100644 --- a/apps/desktop/electron/main/services/preview.test.ts +++ b/apps/desktop/electron/main/services/preview.test.ts @@ -71,7 +71,7 @@ vi.mock('@diskpush/rsync-core', () => ({ unknownCapabilities: () => ({}), })) -const { PREVIEW_DELETE_LIMIT, cancelPreview, previewTransfer } = await import('./transfers.js') +const { PREVIEW_DELETE_LIMIT, cancelAll, cancelPreview, previewTransfer } = await import('./transfers.js') const REQUEST = { source: { type: 'local' as const, path: '/src/' }, @@ -222,4 +222,22 @@ describe('previewTransfer', () => { // The registration is gone, so a late Cancel cannot kill an unrelated run. expect(cancelPreview('p3')).toBe(false) }) + + /* + * The bug: quitting cancelled transfers but not previews, so closing the + * window mid-scan left an rsync and an ssh walking a remote tree with + * nothing left to report to. + */ + it('is stopped when the app quits', async () => { + runs.current = fakeRun() + const target = sender() + const pending = previewTransfer({ ...REQUEST, previewId: 'p4' }, target.webContents) + + await until(() => target.sent.length > 0) + cancelAll() + + const result = await pending + expect(runs.current.killed).toBe(true) + expect(result.cancelled).toBe(true) + }) }) diff --git a/apps/desktop/electron/main/services/transfers.ts b/apps/desktop/electron/main/services/transfers.ts index bc78074..30f134b 100644 --- a/apps/desktop/electron/main/services/transfers.ts +++ b/apps/desktop/electron/main/services/transfers.ts @@ -423,4 +423,11 @@ export function hasActiveTransfer(): boolean { export function cancelAll(): void { for (const job of running.values()) job.cancel() + // Previews too. A dry run is read-only, but it is still an rsync and an ssh + // walking a remote tree, and quitting mid-scan used to leave both running + // with nothing left to report to. + for (const preview of previews.values()) { + preview.cancelled = true + preview.cancel() + } }