Repository navigation
fix(export): re-enable Export All Stems after an export - #337
Merged
Merged
Conversation
resetBusy() cleared aria-disabled from the mix row and re-derived the region row via applyFormatState(), but never cleared the stems row. flashBusy() sets the attribute on all three, so after any export the "Export All Stems" item stayed at opacity 0.4 with pointer-events: none for the rest of the session, across every song, until the app restarted. Only reproducible in the desktop build. In a browser _triggerDownload() appends an <a> and clicks it; that synthetic click bubbles to the document handler and closes the chip panel before flashBusy() runs, so actionItems() -- which filters on offsetParent -- returns empty and nothing is disabled. Under Tauri invoke() returns without that click, the panel is still open, and all three rows get disabled. Clear all three unconditionally rather than through the visibility-filtered actionItems(), so the set and clear paths stay symmetric. applyFormatState() still runs last and re-derives the region row's genuine "no loop selected" state. Fixes #335
downloadAllStemsZip() returns early when there is no current job or no stems loaded, but the click handler called flashBusy() regardless, so the button showed "Exporting..." for 1200 ms for a download that never started. Return false on both early exits, matching the contract downloadCurrentMix and downloadCurrentVideo already use, and surface the same kind of showError() the mix and region rows do.
flashBusy() reset on a 1200 ms timer regardless of how long the export actually took, so on a large stems zip the button reported done while ffmpeg was still working, and a failed save reported success. _triggerDownload() now returns the invoke() promise on desktop, where save_audio_file resolves only after the body is streamed to a temp file and renamed, and `true` in a browser, where an <a download> is owned by the download manager and reports nothing back. flashBusy() waits on the promise when there is one and falls back to the timer only when there is genuinely no signal. A rejected invoke now surfaces the backend message instead of silently looking like a success. A 15-minute backstop and a generation token keep a promise that never settles from leaving the menu disabled for the session, which is the failure mode #335 was about.
showError() always rendered a "Try again" button that hid the error and
moved focus to the URL import field. That is the right action for an
import failure and the wrong one for an export failure, which has nothing
to do with importing a new track.
It went unnoticed because export failures never reached the error box
before: the busy state was a fixed timer that could not tell success from
failure, so a failed save looked exactly like a successful one.
showError() now takes { retry }, defaulting to the existing behaviour.
The export call sites pass retry:false and get a plain Dismiss that clears
the box without stealing focus.
Same defect as the audio export rows: a failed log export is not fixed by being sent to the URL import field.
This was referenced Aug 9, 2026
thcp
marked this pull request as ready for review
August 9, 2026 07:01
This was referenced Aug 9, 2026
Closed
thcp
added a commit
that referenced
this pull request
Aug 12, 2026
Implements #339. CI ran one check on static/js -- a syntax parse -- so any behavioural regression shipped unnoticed until a user hit it. #335 is the case in point: "Export All Stems" became permanently unclickable after a single export, for every track, until the app restarted. It shipped in alpha 15 and a user found it. Setup is Playwright against the real backend. tests/e2e/serve.sh seeds a throwaway jobs directory with one finished track and execs uvicorn against it, with every data path redirected, so a run cannot read or touch a developer's library. Only the separation pipeline is skipped; the endpoints, the registry and the Range requests for stems are real. Two details in the fixtures are load-bearing, both learned by getting them wrong first: - The sidebar renders from the library store, not /api/jobs. A job on disk but absent from the store is invisible in the UI, and a test that clicks nothing passes for the wrong reason. - stubTauri installs a controllable window.__TAURI__ so the desktop code path runs. This is the point of the exercise: #335 was invisible in a browser, because there the synthetic <a>.click() closes the chip panel before the busy state is applied and the bug hides. The stub also leaves save_audio_file pending until the test settles it, so the busy state machine is driven rather than raced. Nine tests cover all four defects from #335 and #337: rows re-enabled after an export in both host modes, a second export still working, the busy state waiting on the save rather than a timer, failures surfacing and recovering, and export errors not offering a "Try again" that sends the user to the URL import field. Verified by reintroducing each defect and watching the suite fail: clearing only the visible rows on reset fails 3 tests (the panel is closed by then, so a visibility-filtered clear clears nothing), the fixed-timer reset fails 1, and the retry button fails 1. MP4 format switching is only partly covered. The video format is hidden unless the track has one, and a video fixture is its own piece of work, so what is here pins that MP4 is not offered for audio-only tracks. Beat grid and transport coverage remain open on #339. Closes #339
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.
Fixes #335, plus three adjacent defects in the same export state machine found while tracing it.
1. Export All Stems stuck disabled (#335)
Symptom
After exporting once, "Export All Stems" is greyed out and unclickable for every song until the app is restarted.
Wider than the report: any export triggers it, not just the stems zip. Exporting a mix once kills the stems row for the rest of the session.
Cause
resetBusy()clearedaria-disabledfrom the mix row and re-derived the region row viaapplyFormatState(), but never cleared the stems row.flashBusy()sets the attribute on all three..export-item[aria-disabled="true"]isopacity: 0.4; pointer-events: none, and the DOM node is global, so it survives track switches and only a page reload clears it.itemStemshad four references in the codebase: set at one, read at one, never cleared.Why it never showed up in a browser
_triggerDownload()branches on the host:<a>and clicks it. That synthetic click bubbles to the document handler and closes the chip panel beforeflashBusy()runs.actionItems()filters onoffsetParent !== null, so with the panel closed it returns[]and nothing is disabled.invoke("save_audio_file")returns without that click, the panel is still open, all three rows get disabled.Desktop-only, which matches the Linux tar.gz report.
Fix
Clear all three rows unconditionally instead of through the visibility-filtered
actionItems(), keeping the set and clear paths symmetric.applyFormatState()still runs last so the region row's genuine "no loop selected" state is re-derived, not clobbered.2. "Exporting..." shown when there is nothing to zip
downloadAllStemsZip()returns early when there is no current job or no stems loaded, but the click handler calledflashBusy()regardless, so the button announced a 1200 ms export that never started. The mix and region rows both check their return value; this one did not.Returns
falseon both early exits now, matching the contractdownloadCurrentMixanddownloadCurrentVideoalready use.3. Busy state was a fixed timer, not real completion
flashBusy()reset after 1200 ms no matter how long the export took, so a large stems zip reported done while ffmpeg was still working, and a failed save looked identical to a successful one._triggerDownload()now returns what the host can actually tell us:invoke()promise.save_audio_fileisasyncand resolves only after the response is streamed to a temp file,sync_all'd and renamed, so it covers the save dialog and the copy.true. An<a download>hands the fetch to the download manager, which reports nothing back to the page, so the timed guess is kept there and only there.flashBusy()waits on the promise when there is one. A rejection now surfaces the backend's message instead of passing for success.Because this makes the busy state open-ended, two guards keep it from becoming a new instance of bug 1: a 15-minute backstop, and a generation token so a stale timer from a finished export cannot reset a later one.
4. Export errors offered "Try again", which went to the import field
showError()always rendered a "Try again" button that hid the error and moved focus to the URL import input. Correct for an import failure, wrong for an export failure, which has nothing to do with importing a track.This was invisible until bug 3 was fixed: export failures never reached the error box, because the timer could not tell success from failure.
showError()now takes{ retry }, defaulting to today's behaviour; the export call sites passretry: falseand get a plain Dismiss that clears the box without stealing focus.Verification
Playwright against a local server with real jobs, driving the actual export menu, with
window.__TAURI__stubbed to reach the desktop branch.Bug 1, before:
Bug 1, after:
Bug 3, stubbing
invoketo resolve after 4 s, to reject, and removing__TAURI__entirely:The slow case held the busy state for the real 4 s rather than the old 1200 ms; the browser case still uses the flash, since there is nothing to wait on.
Bug 4, both paths:
Also checked:
display: block, in viewport, 65 px tall).{"busy":false,"stems":"null","mix":"null"}.display: nonethere, so the unconditional clear cannot surface a wrongly-enabled control, and leaving MP4 restores the region row's real state (region: "true"with no loop selected).main.jsis the only caller.node --checkpasses on every file instatic/js/.Settings had the same wart at the log-export call site, fixed here too.
Behaviour changes worth knowing
Consequences of tracking real completion, not defects:
save_audio_fileshows the picker before fetching. Splitting those phases needs a Rust change, filed as Desktop export shows "Exporting..." while the save dialog is still open #338.Follow-ups filed
Scope
Frontend only:
static/js/main.js,static/js/player.js,static/js/job.js,static/js/catalog.js. No backend or Rust changes; the desktop command already resolved at the right moment, nothing was listening for it.