fix(export): stop claiming to export while the save dialog is open - #366
Merged
Conversation
save_audio_file did two things in one command: show the native picker, then stream the file. The frontend awaited the whole thing, so the button read "Exporting..." from the moment it was clicked, including the entire time the dialog sat open. Nothing was being exported during that phase, and a user who took a while choosing a folder was simply told something untrue. Split into pick_export_destination and download_to_path. The busy state is now entered from a callback the download helpers fire when bytes actually start moving, so the label describes the transfer alone. The transfer takes a token, not a path. #338 suggested download_to_path(url, path), but a path parameter would hand anything running in the WebView the ability to write an arbitrary localhost URL to an arbitrary location on disk -- the destination has until now only ever come from the native dialog. Instead the picked PathBuf stays in Rust and JS holds an opaque single-use token. The token does not need to be unguessable: every live token maps to a path the user already approved in a dialog, so a monotonic counter is enough and no new dependency is needed. Unconsumed picks are capped so an export the user abandons cannot accumulate. save_audio_file stays as a thin wrapper over both halves for the lane download links, which have no busy state to mislabel. Cancelling gets simpler rather than just better labelled: no busy state is ever entered, so there is none to unwind. Removes downloadCurrentStems, which was exported but never called. It was also the only _triggerDownload caller in a loop, which would have meant one save dialog per stem. Verified by reintroducing the defect (entering the busy state before the dialog is answered): 3 of the 4 new tests fail. The suite also covers cancellation, the guard against queueing a second export while the picker is open, and that the transfer is addressed by token rather than by path. Closes #338
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.
Closes #338.
save_audio_filedid two things in one command: show the native picker, then stream the file. The frontend awaited the whole thing, so the button read "Exporting..." from the moment it was clicked, including however long the dialog sat open. Nothing was being exported during that phase.Split into
pick_export_destinationanddownload_to_path. The busy state is now entered from a callback the download helpers fire when bytes actually start moving, so the label describes the transfer alone.One deviation from the issue, and it matters
#338 suggests
download_to_path(url, path). A path parameter would be an arbitrary write primitive for anything running in the WebView — it could stream any localhost URL to any location on disk. Until now the destination has only ever come from the native dialog, and this command has already been hardened twice for exactly this class of problem (#138 SSRF, #139 memory).So the transfer takes a token, not a path. The picked
PathBufstays in Rust; JS holds an opaque, single-use token.The token does not need to be unguessable, which is worth being explicit about: every live token maps to a path the user already approved in a dialog, so guessing one only ever yields another approved destination. A monotonic counter is sufficient and no new dependency is needed. Unconsumed picks are capped at 16 so an abandoned export cannot accumulate.
save_audio_filestays as a thin wrapper over both halves for the lane download links, which have no busy state to mislabel.Also
pickingguard stops a second export being queued while the dialog is open. The dialog is app-modal on a real desktop, but the guard should not depend on that, sincebusyis deliberately still false during this phase.downloadCurrentStems— exported but never called, and the only_triggerDownloadcaller in a loop, which would have meant one save dialog per stem.Verification
Reintroduced the defect (entering the busy state before the dialog is answered): 3 of the 4 new tests fail. That is the check that these tests are worth having.
New coverage, using the two-phase Tauri stub from #365:
download_to_pathnot yet calledpick_export_destinationcall{token, url}and no pathAll 6 pre-existing export tests were updated for the two-phase flow and still pass.
Two of my own test assumptions were wrong and I corrected them: the panel stays open during the dialog phase (only entering the busy state closes it), so re-opening it toggled it shut, and one test still asserted on the old
save_audio_filecommand name.Suites: Rust 34, e2e 13, JS 48, installer 52, Python 539 passed.
cargo fmtclean; clippy shows the same 5 findings asmain, none new.test_all_stems_zip_oggfails identically on cleanmain(local ffmpeg lackslibvorbis).Follow-up, deliberately not here
Progress reporting. The streaming loop already knows the byte count and could emit events now that the transfer is its own command, but it needs a channel and UI. Worth its own issue rather than growing this one.