fix: a manual sync transfers what you selected, and asks first - #31
Merged
Conversation
Selecting two folders on a server and pressing Sync to Local copied the whole
directory. Two movies became forty thousand files.
The panes have always let you select entries. `page.tsx` built its request from
`source.path` and never read `state.selected`, so the selection reached the
counter in the pane footer and nothing else. Every transfer was the whole
directory, every time.
A second bug in the same click, found on the way. The rail sets the direction
and starts the transfer in one handler:
onDirection('rtl')
onRun()
`setDirection` does not change the value the current render closed over, so
`run` built its request from the PREVIOUS direction. Pressing the arrow that
was not armed transferred the opposite way: "Sync to Local" copied local over
the server. With Mirror armed it would have deleted the wrong side. The
direction is now passed to the run rather than read back out of state.
Selections now become an rsync `--files-from` list. The trap there, confirmed
against rsync 3.4.1 rather than assumed:
rsync -a --files-from=list src/ dst/ -> cd+++++++++ movieA/
rsync -a -r --files-from=list src/ dst/ -> cd+++++++++ movieA/
>f+++++++++ movieA/a.mkv
`--files-from` switches recursion off and `--archive` does not switch it back
on, so a selected folder would have copied as an empty folder and reported
success. `buildRsyncArgs` now forces `--recursive` whenever a file list is in
play. The list is NUL-separated (`--from0`), because a newline is legal in a
filename and a line-separated list would split one name into two paths that do
not exist.
Mirror plus a selection deletes only inside what was selected. Verified, not
assumed: the other reading of `--delete` here would empty the destination.
Everything previews and is approved now, mirror or not. Both the desktop and
the CLI skipped the dry run for a plain sync, on the reasoning that it "buys no
safety, because nothing is deleted either way". Deleting is not the only way to
regret a transfer, and this bug is the proof: by the time anything was on
screen it was already copying. The dialog says which it is, Sync or Mirror, and
carries the scope ("2 selected items" / "Whole folder") next to the counts,
because a count alone never says how many of what you asked for. Confirming
starts the request that was measured, not one rebuilt from whatever the panes
say by then.
All three interfaces:
- Desktop: the pane selection is sent, validated as entry names rather than
paths. That list becomes a `--files-from` rooted at the source directory, so
a renderer able to put `../` in it would be choosing which files leave the
machine.
- CLI: `--only NAME`, repeatable, and a plain sync now previews and asks. Only
where somebody is there to answer: a pipe, `--non-interactive` or `--yes`
proceeds as before, so scheduled profile runs keep working. `--no-preview`
restores the old skip-the-scan behaviour.
- TUI: it had a cursor and no way to select at all, and `s` started an
immediate transfer of everything on screen. Space marks entries, `s` scans
and asks before it moves anything.
Two smaller things this turned up:
- `--only` did not parse. It was missing from VALUE_FLAGS, so it consumed no
value, the names fell through to the positionals, and the flag silently did
nothing. Caught by running the built binary, not by reading it.
- The preview dialog's "Trust this pair from now on" checkbox is gone. It set
renderer state that nothing read, and could not have worked if wired:
saveProfile hard-codes `trustDeletes: false` on purpose, because unattended
mirroring is the one way a delete list runs with nobody looking at it.
Verified against the real rsync and the real dev server. `--only movieA --only
movieB` over ssh: 4 files, not 7. The desktop driven in headless Chromium under
the app's real CSP: the preview request carries the ssh source, both names, and
does not start until approved. The TUI driven in a pty: marks render, the
confirm appears, `n` transfers nothing, `y` copies the two marked folders with
their contents and leaves the rest.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52
The "writes the names verbatim" test polled on a 1ms timer for a file that exists only while the transfer is being planned, so on a machine where the preview finished first it read nothing and failed. It passed here and failed in CI, which is the signature of a test that is timing rather than asserting. The mocked planTransfer now reads the list when it is handed the path. That runs while the transfer still owns the file, so there is no window to race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Selecting two folders on a server and pressing Sync to Local copied the whole directory. Two movies became forty thousand files.
The reported bug
The panes have always let you select entries.
page.tsxbuilt its request fromsource.pathand never readstate.selected, so the selection reached the counter in the pane footer and nothing else. Every transfer was the whole directory, every time.A second bug in the same click
The rail sets the direction and starts the transfer in one handler:
setDirectiondoes not change the value the current render closed over, sorunbuilt its request from the previous direction. Pressing the arrow that was not armed transferred the opposite way: "Sync to Local" copied local over the server. With Mirror armed it would have deleted the wrong side. The direction is now passed to the run rather than read back out of state.The rsync trap
Selections become an
--files-fromlist. Confirmed against rsync 3.4.1 rather than assumed:--files-fromswitches recursion off and--archivedoes not switch it back on, so a selected folder would have copied as an empty folder and reported success — the worst possible failure for a sync tool.buildRsyncArgsnow forces--recursivewhenever a file list is in play. The list is NUL-separated (--from0) because a newline is legal in a filename.Mirror plus a selection deletes only inside what was selected. Verified, not assumed: the other reading of
--deletehere would empty the destination.Everything previews and is approved now
Both the desktop and the CLI skipped the dry run for a plain sync, on the reasoning that it "buys no safety, because nothing is deleted either way". Deleting is not the only way to regret a transfer, and this bug is the proof: by the time anything was on screen it was already copying.
The dialog says which it is, Sync or Mirror, and carries the scope (2 selected items / Whole folder) next to the counts, because a count alone never says how many of what you asked for. Confirming starts the request that was measured, not one rebuilt from whatever the panes say by then.
All three interfaces
--files-fromrooted at the source directory, so a renderer able to put../in it would be choosing which files leave the machine.--only NAME, repeatable, and a plain sync now previews and asks. Only where somebody is there to answer: a pipe,--non-interactiveor--yesproceeds as before, so scheduled profile runs keep working.--no-previewrestores the old skip-the-scan behaviour.sstarted an immediate transfer of everything on screen. Space marks entries,sscans and asks before it moves anything.Two smaller things this turned up
--onlydid not parse. Missing fromVALUE_FLAGS, so it consumed no value, the names fell through to the positionals, and the flag silently did nothing. Caught by running the built binary, not by reading it.saveProfilehard-codestrustDeletes: falseon purpose, because unattended mirroring is the one way a delete list runs with nobody looking at it.Verification
Against the real rsync and the real dev server.
--only movieA --only movieBover ssh: 4 files, not 7. The desktop driven in headless Chromium under the app's real CSP — the preview request carries the ssh source, both names, and does not start until approved. The TUI driven in a pty — marks render, the confirm appears,ntransfers nothing,ycopies the two marked folders with their contents and leaves the rest.pnpm test536 passing,pnpm -r typecheckclean,pnpm smoke:desktopall three guards ok. The recursion guard was confirmed to fail when removed.🤖 Generated with Claude Code
https://claude.ai/code/session_01VScug5VRbcTuhiAoieeQ52