From 7409d3e1fff45aee7b4bba8efd0f6c8368934ce9 Mon Sep 17 00:00:00 2001 From: Thales Pereira <31625914+thcp@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:35:03 +0100 Subject: [PATCH] fix(export): stop claiming to export while the save dialog is open 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 --- desktop/src-tauri/src/main.rs | 198 +++++++++++++++++++++++++++++++-- static/js/main.js | 80 ++++++++----- static/js/player.js | 62 +++++------ tests/e2e/export-menu.spec.mjs | 97 +++++++++++++++- tests/e2e/helpers.mjs | 33 +++++- 5 files changed, 390 insertions(+), 80 deletions(-) diff --git a/desktop/src-tauri/src/main.rs b/desktop/src-tauri/src/main.rs index e8af4f4..36b8037 100644 --- a/desktop/src-tauri/src/main.rs +++ b/desktop/src-tauri/src/main.rs @@ -2,6 +2,7 @@ use flate2::read::GzDecoder; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::{ + collections::HashMap, env, fs, io::{Read, Write}, net::{TcpListener, TcpStream}, @@ -67,7 +68,24 @@ struct BackendStateInner { starting: bool, /// PID of an in-progress pip subprocess; killed by stop_backend on window close (#140). pip_pid: Option, -} + /// Save destinations the user has picked but not yet downloaded to (#338). + /// + /// The export is two commands so the UI can tell "choosing a folder" apart + /// from "writing the file", but the second half must not take a path from + /// JS: that would hand a compromised WebView the ability to write any URL + /// to any location on disk. The path stays here and JS only ever holds an + /// opaque token. + pending_saves: HashMap, + /// Source of those tokens. A counter is enough -- the token is not a + /// secret. Every live token maps to a path the user chose in a native + /// dialog, so guessing one only ever yields another approved destination. + next_save_token: u64, +} + +/// Cap on unconsumed destinations. A pick whose download never runs (the user +/// closes the window mid-export) would otherwise sit here for the life of the +/// process. +const MAX_PENDING_SAVES: usize = 16; impl Default for BackendStateInner { fn default() -> Self { @@ -75,6 +93,8 @@ impl Default for BackendStateInner { handles: None, starting: false, pip_pid: None, + pending_saves: HashMap::new(), + next_save_token: 0, } } } @@ -269,6 +289,8 @@ fn main() { build_target, open_url, save_audio_file, + pick_export_destination, + download_to_path, pick_stems_folder, store_get, store_set, @@ -1641,23 +1663,71 @@ fn open_url(url: String) -> Result<(), String> { Ok(()) } -/// Prompts the user for a save path, then streams a localhost audio URL to disk. -#[tauri::command] -async fn save_audio_file( - app: tauri::AppHandle, - url: String, - filename: String, -) -> Result<(), String> { +/// Only localhost URLs, and only http(s). Guards against a compromised WebView +/// using the desktop shell as an SSRF proxy (#138). +fn validate_download_url(url: &str) -> Result<(), String> { if !url.starts_with("http://") && !url.starts_with("https://") { return Err("only http/https URLs are permitted".to_string()); } - // Restrict to localhost to prevent SSRF from a compromised WebView (#138). - let parsed_url = reqwest::Url::parse(&url).map_err(|_| "invalid URL".to_string())?; + let parsed_url = reqwest::Url::parse(url).map_err(|_| "invalid URL".to_string())?; let host = parsed_url.host_str().unwrap_or(""); if host != "127.0.0.1" && host != "localhost" { return Err("only localhost URLs are permitted".to_string()); } + Ok(()) +} +/// Records a picked destination and returns the token JS will hand back. +fn store_pending_save(state: &BackendState, dest: PathBuf) -> Result { + let mut guard = state + .inner + .lock() + .map_err(|_| "state poisoned".to_string())?; + if guard.pending_saves.len() >= MAX_PENDING_SAVES { + // Drop the oldest by token order; tokens are monotonic, so the smallest + // numeric key is the stalest pick. + if let Some(oldest) = guard + .pending_saves + .keys() + .min_by_key(|k| k.parse::().unwrap_or(u64::MAX)) + .cloned() + { + guard.pending_saves.remove(&oldest); + } + } + guard.next_save_token += 1; + let token = guard.next_save_token.to_string(); + guard.pending_saves.insert(token.clone(), dest); + Ok(token) +} + +/// Consumes a token. Single use: a failed transfer needs a fresh destination +/// rather than silently reusing one the user picked for an earlier attempt. +fn take_pending_save(state: &BackendState, token: &str) -> Result { + let mut guard = state + .inner + .lock() + .map_err(|_| "state poisoned".to_string())?; + guard + .pending_saves + .remove(token) + .ok_or_else(|| "no destination is pending for this export".to_string()) +} + +/// Shows the native save dialog and remembers where the user pointed it. +/// +/// Split from the transfer (#338) so the UI can show "Exporting..." for the +/// writing only. Awaiting one combined command meant the button claimed to be +/// exporting for however long the picker sat open, when nothing was happening. +/// +/// Returns None when the user cancels, which the caller treats as "do nothing" +/// -- no busy state is ever entered, so there is none to unwind. +#[tauri::command] +async fn pick_export_destination( + app: tauri::AppHandle, + state: tauri::State<'_, BackendState>, + filename: String, +) -> Result, String> { use tauri_plugin_dialog::DialogExt; let dest = app .dialog() @@ -1665,9 +1735,25 @@ async fn save_audio_file( .set_file_name(&filename) .blocking_save_file(); let Some(file_path) = dest else { - return Ok(()); // user cancelled + return Ok(None); // user cancelled }; let dest = file_path.into_path().map_err(|e| e.to_string())?; + store_pending_save(&state, dest).map(Some) +} + +/// Streams a localhost URL to the destination a previous pick recorded. +/// +/// Takes a token rather than a path on purpose: a path parameter would let +/// anything running in the WebView write an arbitrary URL to an arbitrary +/// location. The destination never leaves Rust. +#[tauri::command] +async fn download_to_path( + state: tauri::State<'_, BackendState>, + token: String, + url: String, +) -> Result<(), String> { + validate_download_url(&url)?; + let dest = take_pending_save(&state, &token)?; // Stream response to disk to avoid buffering a large audio file in memory (#139). // 5-minute timeout covers large WAV exports over a slow loopback. @@ -1700,6 +1786,25 @@ async fn save_audio_file( Ok(()) } +/// Pick-then-transfer in one call, for the lane download links. +/// +/// Those have no busy state to mislabel, so they want the convenience. The +/// export menu drives the two halves separately. +#[tauri::command] +async fn save_audio_file( + app: tauri::AppHandle, + state: tauri::State<'_, BackendState>, + url: String, + filename: String, +) -> Result<(), String> { + // Validate before showing a dialog the request could never satisfy. + validate_download_url(&url)?; + let Some(token) = pick_export_destination(app, state.clone(), filename).await? else { + return Ok(()); // user cancelled + }; + download_to_path(state, token, url).await +} + fn stop_backend(state: &BackendState) { let (handles, pip_pid) = match state.inner.lock() { Ok(mut guard) => (guard.handles.take(), guard.pip_pid.take()), @@ -3031,6 +3136,7 @@ fn hide_console_window(_command: &mut Command) {} #[cfg(test)] mod tests { use std::fs; + use std::path::PathBuf; use tempfile::TempDir; fn make_tmp() -> TempDir { @@ -3561,4 +3667,74 @@ b6052160df96b31c9b1e33854a4dcda3d4b57641b880270f31736fb9f445d384 ffmpeg-n7.1-la // Present but empty: also None. assert_eq!(super::find_driver_store_nvidia_smi(repo.path()), None); } + + // ── export destinations (#338) ─────────────────────────────────────────── + + #[test] + fn a_token_yields_the_path_that_was_picked() { + let state = super::BackendState::default(); + let token = super::store_pending_save(&state, PathBuf::from("/tmp/song.wav")).unwrap(); + assert_eq!( + super::take_pending_save(&state, &token).unwrap(), + PathBuf::from("/tmp/song.wav") + ); + } + + #[test] + fn a_token_works_only_once() { + // A failed transfer has to go back through the dialog rather than + // quietly reusing a destination the user chose for an earlier attempt. + let state = super::BackendState::default(); + let token = super::store_pending_save(&state, PathBuf::from("/tmp/song.wav")).unwrap(); + assert!(super::take_pending_save(&state, &token).is_ok()); + assert!(super::take_pending_save(&state, &token).is_err()); + } + + #[test] + fn an_unknown_token_is_refused() { + // This is the security property: without a matching pick there is no + // destination, so the WebView cannot name one of its own. + let state = super::BackendState::default(); + assert!(super::take_pending_save(&state, "nope").is_err()); + assert!(super::take_pending_save(&state, "1").is_err()); + } + + #[test] + fn tokens_are_distinct_per_pick() { + let state = super::BackendState::default(); + let a = super::store_pending_save(&state, PathBuf::from("/tmp/a.wav")).unwrap(); + let b = super::store_pending_save(&state, PathBuf::from("/tmp/b.wav")).unwrap(); + assert_ne!(a, b); + assert_eq!( + super::take_pending_save(&state, &b).unwrap(), + PathBuf::from("/tmp/b.wav") + ); + assert_eq!( + super::take_pending_save(&state, &a).unwrap(), + PathBuf::from("/tmp/a.wav") + ); + } + + #[test] + fn unconsumed_picks_do_not_accumulate_forever() { + let state = super::BackendState::default(); + let first = super::store_pending_save(&state, PathBuf::from("/tmp/first.wav")).unwrap(); + for i in 0..super::MAX_PENDING_SAVES { + super::store_pending_save(&state, PathBuf::from(format!("/tmp/{i}.wav"))).unwrap(); + } + let held = state.inner.lock().unwrap().pending_saves.len(); + assert!(held <= super::MAX_PENDING_SAVES, "held {held}"); + // The stalest pick is the one dropped. + assert!(super::take_pending_save(&state, &first).is_err()); + } + + #[test] + fn only_localhost_urls_are_downloadable() { + assert!(super::validate_download_url("http://127.0.0.1:8000/api/x.wav").is_ok()); + assert!(super::validate_download_url("http://localhost:8000/api/x.wav").is_ok()); + // The SSRF boundary from #138, still enforced after the split. + assert!(super::validate_download_url("http://example.com/x.wav").is_err()); + assert!(super::validate_download_url("file:///etc/passwd").is_err()); + assert!(super::validate_download_url("not a url").is_err()); + } } diff --git a/static/js/main.js b/static/js/main.js index 6a730e6..b9d493d 100644 --- a/static/js/main.js +++ b/static/js/main.js @@ -150,6 +150,8 @@ function wireFooterControls() { let format = "wav"; let busy = false; + // True from the click until the transfer starts or the dialog is cancelled. + let picking = false; const panelOpen = () => exportPanel && !exportPanel.classList.contains("hidden"); function openPanel() { @@ -194,12 +196,11 @@ function wireFooterControls() { busy = false; exportBtn?.classList.remove("is-busy"); if (exportLabel) exportLabel.textContent = "Export Mix"; - // Clear every row, not just the ones flashBusy could see: it disables via - // actionItems(), which filters on visibility, so a row hidden at reset time - // would keep the attribute forever. Under Tauri the panel is still open when - // flashBusy runs (invoke() returns without the synthetic click that - // closes it in a browser), so all three get disabled and only a symmetric - // clear brings them back. + // Clear every row, not just the ones enterBusy could see: it disables via + // actionItems(), which filters on visibility, and it closes the panel, so + // by the time this runs every row is hidden and a visibility-filtered clear + // would clear nothing at all -- leaving the menu dead for the rest of the + // session (#335). for (const it of [itemMix, itemStems, itemRegion]) it?.removeAttribute("aria-disabled"); applyFormatState(); // re-derives the region row's genuine disabled state } @@ -212,18 +213,32 @@ function wireFooterControls() { // Guards against a stale timer from a finished export resetting a later one. let busyToken = 0; - // `pending` is whatever the download helper returned: a promise on desktop, - // where save_audio_file resolves once the file is written, or `true` in a - // browser, where an is fire-and-forget and there is nothing to - // wait on. Only the guess needs a fixed duration. - function flashBusy(pending) { + // Show the busy state. Called when bytes actually start moving, which on + // desktop is after the user has picked a destination -- not on click, or the + // label would claim to be exporting for as long as the save dialog sat open + // (#338). + function enterBusy() { busy = true; - const token = ++busyToken; - const finish = () => { if (token === busyToken) resetBusy(); }; exportBtn?.classList.add("is-busy"); if (exportLabel) exportLabel.textContent = "Exporting…"; actionItems().forEach((it) => it?.setAttribute("aria-disabled", "true")); closePanel(); + } + + // `pending` is whatever the download helper returned: a promise on desktop, + // resolving once the file is written, or `true` in a browser, where an + // is fire-and-forget and there is nothing to wait on. Only the + // guess needs a fixed duration. + // + // `picking` covers the gap between the click and the transfer: the dialog is + // app-modal so the menu is unreachable anyway, but the flag keeps a second + // export from being queued behind it without lying about the label. + function settleBusy(pending) { + const token = ++busyToken; + const finish = () => { + picking = false; + if (token === busyToken) resetBusy(); + }; if (!pending || typeof pending.then !== "function") { window.setTimeout(finish, EXPORT_FLASH_MS); @@ -232,7 +247,8 @@ function wireFooterControls() { const backstop = window.setTimeout(finish, EXPORT_BUSY_MAX_MS); pending .catch((err) => { - // A cancelled save dialog resolves, so anything here is a real failure. + // A cancelled dialog resolves false without ever entering the busy + // state, so anything here is a real failure. showError(typeof err === "string" && err ? err : "Export failed.", null, { retry: false }); }) .finally(() => { @@ -241,37 +257,47 @@ function wireFooterControls() { }); } + // Kick off an export: hand the helper a callback that flips the UI into its + // busy state, then wait on the result. + function runExport(start, emptyMessage) { + if (busy || picking) return; + picking = true; + const pending = start(enterBusy); + if (!pending) { + picking = false; + showError(emptyMessage, null, { retry: false }); + return; + } + settleBusy(pending); + } + exportBtn?.addEventListener("click", (e) => { e.stopPropagation(); - if (busy) return; + if (busy || picking) return; panelOpen() ? closePanel() : openPanel(); }); // Export Mix: MP4 produces the video; any other format an audio mix. itemMix?.addEventListener("click", (e) => { e.stopPropagation(); - if (busy) return; - const ok = format === "mp4" ? downloadCurrentVideo() : downloadCurrentMix(format); - if (!ok) { showError("All stems are muted - nothing to export.", null, { retry: false }); return; } - flashBusy(ok); + runExport( + (onStart) => (format === "mp4" ? downloadCurrentVideo(onStart) : downloadCurrentMix(format, onStart)), + "All stems are muted - nothing to export.", + ); }); itemRegion?.addEventListener("click", (e) => { e.stopPropagation(); - if (busy || itemRegion.getAttribute("aria-disabled") === "true") return; - const ok = downloadRegionMix(format); - if (!ok) { showError("All stems are muted - nothing to export.", null, { retry: false }); return; } - flashBusy(ok); + if (itemRegion.getAttribute("aria-disabled") === "true") return; + runExport((onStart) => downloadRegionMix(format, onStart), "All stems are muted - nothing to export."); }); // All Stems = a single backend-built ZIP, named after the song. Audio-only, // so it's disabled (and inert) while MP4 is the selected format. itemStems?.addEventListener("click", (e) => { e.stopPropagation(); - if (busy || itemStems.getAttribute("aria-disabled") === "true") return; - const ok = downloadAllStemsZip(format); - if (!ok) { showError("No stems to export.", null, { retry: false }); return; } - flashBusy(ok); + if (itemStems.getAttribute("aria-disabled") === "true") return; + runExport((onStart) => downloadAllStemsZip(format, onStart), "No stems to export."); }); // Keyboard: ↓ opens/moves into the menu, ↑/↓ cycle rows, Esc closes + restores focus. diff --git a/static/js/player.js b/static/js/player.js index d8b7bb0..9846f74 100644 --- a/static/js/player.js +++ b/static/js/player.js @@ -1558,15 +1558,29 @@ export function updateFooterTrack({ title, thumbnail, key, bpm, stemCount } = {} // Returns a promise that settles when the file is actually on disk, or `true` // when the host gives no completion signal. Callers use the difference to show // a real "Exporting…" state instead of a fixed-length guess. -function _triggerDownload(url, filename) { +// +// `onTransferStart` fires when bytes actually begin moving, which on desktop is +// after the user has chosen a destination. Awaiting one combined command made +// the button read "Exporting…" for however long the save dialog sat open, when +// nothing was being exported yet (#338). Callers enter their busy state here +// rather than on click. +function _triggerDownload(url, filename, onTransferStart) { const fullUrl = url.startsWith("http") ? url : `${location.origin}${url}`; - if (window.__TAURI__?.core?.invoke) { - // save_audio_file streams to a temp file and renames, resolving only once - // the whole body is written -- so this covers the save dialog and the copy. - return window.__TAURI__.core.invoke("save_audio_file", { url: fullUrl, filename }); + const invoke = window.__TAURI__?.core?.invoke; + if (invoke) { + // Two commands: the dialog, then the transfer. A cancelled dialog resolves + // to null and never starts a transfer, so no busy state is entered and + // there is none to unwind. + return invoke("pick_export_destination", { filename }).then((token) => { + if (!token) return false; + onTransferStart?.(); + return invoke("download_to_path", { token, url: fullUrl }); + }); } // A browser is fire-and-forget: the fetch is owned by the - // download manager and reports nothing back to the page. + // download manager and reports nothing back to the page. There is no dialog + // to wait on, so the transfer is under way as soon as the click lands. + onTransferStart?.(); const a = document.createElement("a"); a.href = fullUrl; a.download = filename; @@ -1659,16 +1673,16 @@ function _exportFilename(ext) { // The download functions return true when a download was triggered and false // when there is nothing audible to export (every lane muted), so the caller can // surface a message. -export function downloadCurrentMix(ext = "wav") { +export function downloadCurrentMix(ext = "wav", onTransferStart) { const url = _mixdownUrl(ext, false); if (!url) return false; - return _triggerDownload(url, _exportFilename(ext)); + return _triggerDownload(url, _exportFilename(ext), onTransferStart); } // MP4 export: the preserved source video muxed with the current audio mix. // Only meaningful for mp4-sourced jobs (currentJobHasVideo()); returns false when // there's no video track or every lane is muted. -export function downloadCurrentVideo() { +export function downloadCurrentVideo(onTransferStart) { if (!currentJobId || !_currentHasVideo) return false; const { names, gains } = _effectiveMixGains(); if (!names.length) return false; @@ -1683,34 +1697,14 @@ export function downloadCurrentVideo() { .slice(0, 80) .replace(/^_+|_+$/g, ""); const name = safe ? `${safe}_video.mp4` : "video.mp4"; - return _triggerDownload(`/api/jobs/${currentJobId}/video.mp4?${q}`, name); + return _triggerDownload(`/api/jobs/${currentJobId}/video.mp4?${q}`, name, onTransferStart); } -export function downloadCurrentStems(format = "wav", onProgress) { - const stems = _currentStems.filter((s) => s.name !== "original"); - const total = stems.length; - if (!total) { onProgress?.(0, 0); return; } - // Name each file "_." using the same title - // sanitization as the mix/region exports. - const safe = _currentTitle - .replace(/[^a-zA-Z0-9]+/g, "_") - .replace(/_{2,}/g, "_") - .slice(0, 80) - .replace(/^_+|_+$/g, ""); - stems.forEach((s, i) => { - window.setTimeout(() => { - const url = format === "mp3" ? s.url.replace(/\.wav(\?|$)/, ".mp3$1") : s.url; - const fname = safe ? `${safe}_${s.name}.${format}` : `${s.name}.${format}`; - _triggerDownload(url, fname); - onProgress?.(i + 1, total); - }, i * 150); - }); -} // Returns false when there is nothing to zip, matching downloadCurrentMix and // downloadCurrentVideo, so the caller can skip the "Exporting…" state instead of // showing it for a download that never starts. -export function downloadAllStemsZip(format = "wav") { +export function downloadAllStemsZip(format = "wav", onTransferStart) { if (!currentJobId) return false; // Only the active (selected) stems loaded in the DAW — not all 6. const names = _currentStems.filter((s) => s.name !== "original").map((s) => s.name); @@ -1722,7 +1716,7 @@ export function downloadAllStemsZip(format = "wav") { .replace(/^_+|_+$/g, ""); const name = safe ? `${safe}_stems.zip` : "stems.zip"; const q = new URLSearchParams({ format, stems: names.join(",") }); - return _triggerDownload(`/api/jobs/${currentJobId}/stems/all.zip?${q}`, name); + return _triggerDownload(`/api/jobs/${currentJobId}/stems/all.zip?${q}`, name, onTransferStart); } function _regionFilename(ext) { @@ -1734,9 +1728,9 @@ function _regionFilename(ext) { return `${safe || "region"}_region.${ext}`; } -export function downloadRegionMix(ext = "wav") { +export function downloadRegionMix(ext = "wav", onTransferStart) { if (!loopEnabled || loopStart >= loopEnd) return false; const url = _mixdownUrl(ext, true); if (!url) return false; - return _triggerDownload(url, _regionFilename(ext)); + return _triggerDownload(url, _regionFilename(ext), onTransferStart); } diff --git a/tests/e2e/export-menu.spec.mjs b/tests/e2e/export-menu.spec.mjs index 9056afc..d59a43d 100644 --- a/tests/e2e/export-menu.spec.mjs +++ b/tests/e2e/export-menu.spec.mjs @@ -12,6 +12,11 @@ import { test, expect } from "@playwright/test"; import { openStudio, exportUi } from "./helpers.mjs"; +// Desktop exports go through a save dialog first. Picking a destination is what +// starts the transfer, so most tests have to answer the dialog before there is +// any busy state to assert on (#338). +const choosePath = (page) => page.evaluate(() => window.__e2e.choosePath()); + test.describe("export menu, desktop (Tauri) mode", () => { test("every row is usable again after an export completes", async ({ page }) => { await openStudio(page, { tauri: true }); @@ -19,6 +24,7 @@ test.describe("export menu, desktop (Tauri) mode", () => { await ui.open(); await ui.stems.click(); + await choosePath(page); // Mid-export: the menu is busy and says so. await expect(ui.label).toHaveText(/Exporting/); @@ -40,12 +46,13 @@ test.describe("export menu, desktop (Tauri) mode", () => { for (const pass of [1, 2]) { await ui.open(); await ui.stems.click(); + await choosePath(page); await expect(ui.label).toHaveText(/Exporting/, { timeout: 5000 }); await page.evaluate(() => window.__e2e.finishSave()); await expect(ui.label).toHaveText("Export Mix"); expect( - await page.evaluate(() => window.__e2e.callsFor("save_audio_file").length), - `save_audio_file should have fired on pass ${pass}`, + await page.evaluate(() => window.__e2e.callsFor("download_to_path").length), + `the transfer should have fired on pass ${pass}`, ).toBe(pass); } }); @@ -58,6 +65,7 @@ test.describe("export menu, desktop (Tauri) mode", () => { await ui.open(); await ui.mix.click(); + await choosePath(page); await expect(ui.label).toHaveText(/Exporting/); await page.waitForTimeout(3000); // comfortably past the 1200 ms guess @@ -74,6 +82,7 @@ test.describe("export menu, desktop (Tauri) mode", () => { await ui.open(); await ui.mix.click(); + await choosePath(page); await expect(ui.label).toHaveText(/Exporting/); await page.evaluate(() => window.__e2e.failSave("disk full")); @@ -94,6 +103,7 @@ test.describe("export menu, desktop (Tauri) mode", () => { await ui.open(); await ui.mix.click(); + await choosePath(page); await page.evaluate(() => window.__e2e.failSave("nope")); await expect(ui.error).toBeVisible(); @@ -102,6 +112,88 @@ test.describe("export menu, desktop (Tauri) mode", () => { }); }); +test.describe("the save dialog phase (#338)", () => { + test("the label does not claim to be exporting while the picker is open", async ({ page }) => { + // The whole point of splitting the command. Awaiting one combined + // save_audio_file meant the button read "Exporting..." from the moment it + // was clicked, including however long the user spent choosing a folder, + // when nothing was being exported yet. + await openStudio(page, { tauri: true }); + const ui = exportUi(page); + + await ui.open(); + await ui.mix.click(); + + await expect.poll(() => page.evaluate(() => window.__e2e.pickPending())).toBe(true); + await expect(ui.label).toHaveText("Export Mix"); + await expect(ui.button).not.toHaveClass(/is-busy/); + // Nothing has been transferred, so no transfer command has been issued. + expect(await page.evaluate(() => window.__e2e.callsFor("download_to_path").length)).toBe(0); + + await choosePath(page); + await expect(ui.label).toHaveText(/Exporting/); + expect(await page.evaluate(() => window.__e2e.callsFor("download_to_path").length)).toBe(1); + + await page.evaluate(() => window.__e2e.finishSave()); + await expect(ui.label).toHaveText("Export Mix"); + }); + + test("cancelling the dialog leaves the menu exactly as it was", async ({ page }) => { + // No busy state is ever entered, so there is none to unwind. + await openStudio(page, { tauri: true }); + const ui = exportUi(page); + + await ui.open(); + await ui.mix.click(); + await expect.poll(() => page.evaluate(() => window.__e2e.pickPending())).toBe(true); + + await page.evaluate(() => window.__e2e.cancelPick()); + + await expect(ui.label).toHaveText("Export Mix"); + await expect(ui.button).not.toHaveClass(/is-busy/); + expect(await page.evaluate(() => window.__e2e.callsFor("download_to_path").length)).toBe(0); + await expect(ui.error).toHaveCount(0); + + // The panel was never closed: only entering the busy state does that, and + // a cancelled pick never gets there. So the menu is still open and usable. + await expect(ui.panel).not.toHaveClass(/hidden/); + await ui.mix.click(); + await choosePath(page); + await expect(ui.label).toHaveText(/Exporting/); + }); + + test("a second export cannot be queued while the picker is open", async ({ page }) => { + // The dialog is app-modal on a real desktop, but the guard must not depend + // on that: `busy` is deliberately still false during this phase. + await openStudio(page, { tauri: true }); + const ui = exportUi(page); + + await ui.open(); + await ui.mix.click(); + await expect.poll(() => page.evaluate(() => window.__e2e.pickPending())).toBe(true); + + await ui.mix.click(); + await ui.button.click(); + expect(await page.evaluate(() => window.__e2e.callsFor("pick_export_destination").length)).toBe(1); + expect(await page.evaluate(() => window.__e2e.callsFor("download_to_path").length)).toBe(0); + }); + + test("the transfer is told where to write by token, never by path", async ({ page }) => { + // The destination stays in Rust. A path argument here would be an arbitrary + // write primitive for anything running in the WebView. + await openStudio(page, { tauri: true }); + const ui = exportUi(page); + + await ui.open(); + await ui.mix.click(); + await choosePath(page); + + const args = await page.evaluate(() => window.__e2e.callsFor("download_to_path")[0].args); + expect(Object.keys(args).sort()).toEqual(["token", "url"]); + expect(args.token).toBeTruthy(); + }); +}); + test.describe("export menu, browser mode", () => { test("rows recover after the fire-and-forget download path", async ({ page }) => { // No Tauri bridge: _triggerDownload falls back to a synthetic , @@ -156,6 +248,7 @@ test.describe("busy state", () => { await ui.open(); await ui.mix.click(); + await choosePath(page); await expect(ui.label).toHaveText(/Exporting/); await expect(ui.panel).toHaveClass(/hidden/); diff --git a/tests/e2e/helpers.mjs b/tests/e2e/helpers.mjs index f7620dd..fc03798 100644 --- a/tests/e2e/helpers.mjs +++ b/tests/e2e/helpers.mjs @@ -47,19 +47,37 @@ export async function seedLibrary(page) { /** * Install a fake Tauri bridge so the app takes its desktop code path. * - * `save_audio_file` is left pending until the test resolves or rejects it, - * which is the whole point: the export busy state is a promise state machine, - * and #335 was a stuck one. Tests drive it through window.__e2e. + * The export is two commands, and the test controls each independently: + * + * pick_export_destination the native save dialog + * download_to_path the transfer + * + * Holding the dialog open is what makes #338 testable -- the label must still + * read "Export Mix" while the user is choosing a folder, because nothing is + * being exported yet. Holding the transfer open is what makes #335 testable. + * Tests drive both through window.__e2e. */ export async function stubTauri(page) { await page.addInitScript(() => { const calls = []; let pendingResolve = null; let pendingReject = null; + let pickResolve = null; + + const settle = (fn) => (v) => { + pendingResolve = null; + pendingReject = null; + fn(v); + }; window.__e2e = { calls, - // Settle the export that is currently in flight. + // Choose a destination, as if the user hit Save in the dialog. + choosePath: () => pickResolve && (pickResolve("token-1"), (pickResolve = null)), + // Dismiss the dialog. No transfer follows. + cancelPick: () => pickResolve && (pickResolve(null), (pickResolve = null)), + pickPending: () => Boolean(pickResolve), + // Settle the transfer that is currently in flight. finishSave: (value) => pendingResolve && pendingResolve(value ?? null), failSave: (message) => pendingReject && pendingReject(message ?? "save failed"), savePending: () => Boolean(pendingResolve), @@ -71,10 +89,13 @@ export async function stubTauri(page) { invoke: (cmd, args) => { calls.push({ cmd, args }); switch (cmd) { + case "pick_export_destination": + return new Promise((resolve) => { pickResolve = resolve; }); + case "download_to_path": case "save_audio_file": return new Promise((resolve, reject) => { - pendingResolve = (v) => { pendingResolve = null; pendingReject = null; resolve(v); }; - pendingReject = (e) => { pendingResolve = null; pendingReject = null; reject(e); }; + pendingResolve = settle(resolve); + pendingReject = settle(reject); }); // The library store lives in the Tauri store on desktop. Back it // with localStorage so seedLibrary works in this mode too.