Skip to content

Report a failure from the notification centre - #372

Merged
thcp merged 4 commits into
mainfrom
feat/report-failures
Aug 16, 2026
Merged

Report a failure from the notification centre#372
thcp merged 4 commits into
mainfrom
feat/report-failures

Conversation

@thcp

@thcp thcp commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Closes #370-adjacent gap in failure reporting. Follows #359 ("A track that
fails to load tells the user nothing") and the reporting problem #343 exposed.

Why

A failure used to live in a transient #error banner. Dismiss it, or reload,
and the evidence was gone — the position #359 complained about, where a
reporter has nothing to paste and guesses at a cause instead. #343 is the
standing proof: its author blamed a GPU and sent the investigation the wrong
way. Even developing this branch hit it — a demucs exited 1 (no stderr captured) that was really a missing ffmpeg on PATH.

Meanwhile the backend has written excellent evidence on every failure since
#277 (jobs/failed/<id>/error.txt: classified cause, device, model, timings,
40-line stderr tail) and nothing ever read it back.

What

Failures land in the notification centre, survive a reload, and open a dialog
that hands the whole thing to GitHub as a pre-filled bug report — version, OS,
install method, stage, device, model and the stderr tail already in the form.
The user adds what they were doing and ticks the two preflight boxes, which
GitHub cannot prefill and which are the point.

Covers import (foreground and background), playback, export and update
failures. A background import that failed used to say nothing whatsoever: no
banner, no queue UI, just a console.warn and a library row identical to a
healthy one. Queue three tracks, lose one, never find out.

  • Not wired into showError wholesale: it also carries benign validation
    ("Only MP3, WAV… are supported"), which must not file a bug report.
  • One failure, one card. The foreground SSE handler and the background queue
    reconciler can both notice the same dead job, and applyState can run its
    error branch on more than one frame, so records key on the job id.
  • classify_failure()'s "unknown" sentinel is dropped rather than shown: as
    a card it read "Import failed — unknown", and as an issue title it grouped
    every unclassified failure under one meaningless heading.

Privacy

Technical details only. Track title and source URL are never included —
issues are public, and the user adds them if they help. GET /api/jobs/{id}/failure enforces that server-side by parsing error.txt and
serving a whitelist, rather than trusting the client that builds the report
body. There is a test asserting neither can appear in the response.

Two Settings bugs found along the way

  • The Registry pane never loaded. loadRegistryView selected
    .settings-registry-view unscoped, but the log viewers reuse that class for
    its read-only-textarea styling and sit earlier in the markup — so the lookup
    returned the application log box. Registry JSON went into a hidden
    textarea while the Registry pane sat on its literal "Loading…" placeholder
    for ever, and the application log showed registry JSON until refreshed. Not
    web-only; it never worked anywhere.
  • backend.log had no viewer. Listed under Logs → Location and shipped in
    the logs zip, but the only views were application and setup — so the one
    log holding what killed a backend before its own logging was configured was
    the one log you could not read in the app. It gets a tab.

Testing

  • tests/js/report-url.test.mjs (45 checks) — pins the dropdown strings (an OS
    that does not match an option exactly is dropped by GitHub without
    complaint), the URL length ceiling, truncation keeping the end of the tail
    where the error is, and that no title or source URL can appear.
  • tests/e2e/report-failure.spec.mjs (5) — including the desktop path, where
    the link is intercepted and handed to open_url rather than navigating. A
    break there would do nothing in the shipped desktop app while working in
    every browser a developer tests in, which is how [Bug]: Unable to export stems again after exporting stems without restarting the program #335 hid.
  • tests/test_failure_api.py (6) — field parsing, the tail, traversal
    refusal, and the privacy assertion.
  • tests/test_logs_api.py — the backend view, its rotation ordering, and a
    test that walks _LOG_FILES against _LOG_VIEWS and fails if a file the
    Settings pane advertises has no view to read it in. That is exactly how
    backend.log stayed invisible.
  • Driven by hand against real failures (a genuine download failure and the
    quarantined jobs on disk), plus a regression sweep over every Settings pane,
    track load, playback and the benign-validation paths that must not file
    reports.

Out of scope

The phone UI (separate app, own error strings), and unhandled JS exceptions
(main.js still console.errors those) — auto-filing them risks noise and
duplicates, so that deserves its own decision.

Thales added 4 commits August 16, 2026 20:48
A failure used to live in a transient #error banner. Dismiss it, or reload,
and the evidence was gone -- which is the position #359 complained about,
where a reporter has nothing to paste and guesses at a cause instead. #343 is
the standing proof: its author blamed a GPU and sent the investigation the
wrong way. This session hit the same wall, a "demucs exited 1 (no stderr
captured)" that was really a missing ffmpeg on PATH.

Failures now land in the notification centre, survive a reload, and open a
dialog that can hand the whole thing to GitHub as a pre-filled bug report --
version, OS, install method, stage, device, model and the stderr tail already
in the form. The user adds what they were doing and ticks the two preflight
boxes, which GitHub cannot prefill and which are the point.

Covers import (foreground and background), playback, export and update
failures. A background import that failed used to say nothing whatsoever: no
banner, no queue UI, just a console warning and a library row identical to a
healthy one. Queue three tracks, lose one, never find out.

- Deliberately not wired into showError wholesale: it also carries benign
  validation ("Only MP3, WAV... are supported"), which must not file a bug.
- One failure, one card. The foreground SSE handler and the background queue
  reconciler can both notice the same dead job, and applyState can run its
  error branch on more than one frame, so records key on the job id.
- classify_failure()'s "unknown" sentinel is dropped rather than shown: as a
  card it read "Import failed - unknown", and as an issue title it grouped
  every unclassified failure under one meaningless heading.

Privacy: the report carries technical details only. Track title and source URL
are never included -- issues are public, and the user adds them if they help.
GET /api/jobs/{id}/failure enforces that server-side by parsing error.txt and
serving a whitelist, rather than trusting the client to filter the file.

That endpoint also closes a gap: the pipeline has written the quarantined
error.txt since #277 -- classified cause, device, model, timings, 40-line
stderr tail -- and nothing ever read it back, so the UI had only the one-line
error_detail. It is the difference between "demucs failed" and "CUDA out of
memory: tried to allocate 2.40 GiB".

The notification centre had no generic add-a-card path: one hardcoded release
card, and badge/empty-state toggled inline at its two call sites assuming
exactly one card. That is centralised in notifications.js now, with the
release card keeping its own per-version dismissal key.

Tests: tests/js/report-url.test.mjs pins the dropdown strings (an OS that does
not match an option exactly is dropped by GitHub without complaint), the URL
length ceiling, tail truncation keeping the end where the error is, and that
no title or source URL can appear. tests/e2e/report-failure.spec.mjs covers
the desktop path, where the link is intercepted and handed to open_url rather
than navigating -- a break there would do nothing in the shipped app while
working in every browser a developer tests in.
… log view

Two Settings defects, both found by looking at the pane rather than the code.

**Registry never loaded.** loadRegistryView selected `.settings-registry-view`
unscoped, but the two log viewers reuse that class for its read-only-textarea
styling and sit earlier in the markup. The lookup therefore returned the
*application log* box: the registry JSON was written into a hidden textarea
while the registry pane kept its literal "Loading…" placeholder for ever, and
the application log showed registry JSON until it was refreshed. Scope the
lookup to the registry pane. Not web-only -- it never worked anywhere.

**backend.log had no viewer.** It was listed under Logs → Location and shipped
in the logs zip, but the only two views were application and setup, so the one
log that holds what killed a backend before its own logging was configured was
the one log you could not read in the app. It gets a "Backend log" tab beside
the other two, reading backend.log plus its two rotations.

The sub-tab wiring is already generic (loadLogTail(overlay, name)), so the tab
needed markup and a view entry, no new JS.

Tests: the backend view's window filtering and rotation ordering, plus one that
walks _LOG_FILES against _LOG_VIEWS and fails if a file the Settings pane
advertises has no view to read it in -- which is exactly how backend.log stayed
invisible.
initNotifications assigned the stored list over whatever was already in
memory. Reading the store is async, so a failure recorded while that read was
in flight was dropped -- losing exactly the notification the user would then
go looking for. Merge by id instead, newest first.

Latent rather than observed: the current call order records nothing that
early. It is one line, and the alternative is a bug that only ever appears
when something else has already gone wrong.
…badge

CI failed two notification tests that pass on any developer machine. The
update check hits api.github.com for real; when the published release is newer
than the version under test, an update card appears and lights the same badge
failure notifications use. The tests then saw a lit badge with no failures.
Locally it never happened, because a dev build reports a version containing
"dev" and the check skips those -- the tests were passing for the wrong reason.

Answer the update check from the test instead, which also takes an external
service out of the path of every run.

The behaviour CI caught is correct and now has a test of its own: with an
update pending, dismissing the last failure card leaves the badge lit and the
empty state hidden, because the update is still there. openStudio grows an
`updateAvailable` option that forces that state (stubbing the version too --
the check skips dev builds, so a release-looking version is required for the
card to appear at all).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement]: Reorganise the transport footer into labelled control clusters

1 participant