fix(about): say what actually failed when an update errors, in one sentence - #440
Conversation
…ntence The About tab pasted electron-updater's raw text behind a hardcoded "Update download failed:" prefix, so a failed check was reported as a failed download and users read `ENOENT ... app-update.yml` with a bundle path in it. A new pure module (update-error-copy.js) maps the error to one sentence, names the phase honestly (check / download / install), and marks whether a later success disproves it (`sticky`). The raw text stays in the debug log, where it is useful. Also fixes a stale banner sitting under a fresh "You're on the latest version", including across a remount: main now owns when a failure is settled and says so on a new update-error-cleared channel, and the About tab drops a status reply that a newer write has already overtaken. Sticky failures (full disk, permission, no update feed) survive finding a version and clear only once bytes actually arrive, because that is what disproves them.
There was a problem hiding this comment.
3 issues found across 9 files
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/update-error-copy.js">
<violation number="1" location="app/update-error-copy.js:136">
P3: About-tab errors are still rendered as two sentences in several branches, despite this PR’s one-sentence copy contract. Combine each recovery/action clause into its failure sentence so the UI has the promised single-sentence status.</violation>
</file>
<file name="app/main.js">
<violation number="1" location="app/main.js:6350">
P2: A successful download can still leave a previous failure banner visible. `update-downloaded` clears main state but does not notify the mounted About tab's error state, so emit `update-error-cleared` when this transition clears an error (or clear/invalidate it in `offDownloaded`).</violation>
</file>
<file name="app/renderer/src/lib/ipc.ts">
<violation number="1" location="app/renderer/src/lib/ipc.ts:1110">
P2: About now subscribes to this new event, but the E2E IPC mock does not expose `update-error-cleared`; mounting the About tab under the mock can therefore fail before the updater scenario runs, and the mock cannot model the new clear behavior. Adding the channel to the mock (including a no-op/emit implementation consistent with the other subscriptions) keeps the test bridge in sync with the preload contract.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // is describing a state that no longer exists — and a condition that really | ||
| // is still broken (no update feed) errors before ever reaching this event. | ||
| pendingUpdateError = null; | ||
| pendingUpdateErrorSticky = false; |
There was a problem hiding this comment.
P2: A successful download can still leave a previous failure banner visible. update-downloaded clears main state but does not notify the mounted About tab's error state, so emit update-error-cleared when this transition clears an error (or clear/invalidate it in offDownloaded).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/main.js, line 6350:
<comment>A successful download can still leave a previous failure banner visible. `update-downloaded` clears main state but does not notify the mounted About tab's error state, so emit `update-error-cleared` when this transition clears an error (or clear/invalidate it in `offDownloaded`).</comment>
<file context>
@@ -6278,32 +6290,75 @@ function setupAutoUpdater() {
+ // is describing a state that no longer exists — and a condition that really
+ // is still broken (no update feed) errors before ever reaching this event.
+ pendingUpdateError = null;
+ pendingUpdateErrorSticky = false;
+ // Tell a mounted About tab too — it keeps its own copy, and the events it
+ // already listens to (available/progress/downloaded) don't fire on a clean
</file context>
| updateDownloadProgress: Subscribe<UpdateProgressEvent>; | ||
| updateDownloaded: Subscribe<UpdateDownloadedEvent>; | ||
| updateError: Subscribe<UpdateErrorEvent>; | ||
| updateErrorCleared: Subscribe<void>; |
There was a problem hiding this comment.
P2: About now subscribes to this new event, but the E2E IPC mock does not expose update-error-cleared; mounting the About tab under the mock can therefore fail before the updater scenario runs, and the mock cannot model the new clear behavior. Adding the channel to the mock (including a no-op/emit implementation consistent with the other subscriptions) keeps the test bridge in sync with the preload contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/renderer/src/lib/ipc.ts, line 1110:
<comment>About now subscribes to this new event, but the E2E IPC mock does not expose `update-error-cleared`; mounting the About tab under the mock can therefore fail before the updater scenario runs, and the mock cannot model the new clear behavior. Adding the channel to the mock (including a no-op/emit implementation consistent with the other subscriptions) keeps the test bridge in sync with the preload contract.</comment>
<file context>
@@ -1107,6 +1107,7 @@ export interface StenoaiBridge {
updateDownloadProgress: Subscribe<UpdateProgressEvent>;
updateDownloaded: Subscribe<UpdateDownloadedEvent>;
updateError: Subscribe<UpdateErrorEvent>;
+ updateErrorCleared: Subscribe<void>;
googleAuthChanged: Subscribe<{ connected: boolean }>;
outlookAuthChanged: Subscribe<{ connected: boolean }>;
</file context>
| @@ -0,0 +1,186 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
P3: About-tab errors are still rendered as two sentences in several branches, despite this PR’s one-sentence copy contract. Combine each recovery/action clause into its failure sentence so the UI has the promised single-sentence status.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/update-error-copy.js, line 136:
<comment>About-tab errors are still rendered as two sentences in several branches, despite this PR’s one-sentence copy contract. Combine each recovery/action clause into its failure sentence so the UI has the promised single-sentence status.</comment>
<file context>
@@ -0,0 +1,186 @@
+ }
+ if (INTEGRITY_RE.test(msg)) {
+ return {
+ message: "The update couldn't be verified, so it wasn't installed. Steno will try again later.",
+ sticky: false,
+ };
</file context>
…ll phase for transport errors Review follow-up. main clears both kinds of failure at update-downloaded but only sends 'update-downloaded', so a resumed or cached transfer — where that is the first event a mounted About tab sees — left the banner next to 'Restart to Update'. And a transport error surfacing during install fell into the 'download was interrupted' arm, which is the same mislabelling this module exists to stop, one level down.
|
Thanks — three of the four are in Fixed: the banner surviving Fixed: a transport error during install labelled a download. Also right, and it is the same mislabelling this module exists to stop, one level down — applying a staged update touches no network, so promising a retry of a transfer would be wrong. Fixed, as wording: the "one sentence" claim. The module promised more than it delivers — several branches legitimately produce two, because rule 2 (say whether the user has to do anything) needs its own clause and reads better split. Rather than compress good copy to satisfy a claim, the claim is now accurate: prose, with the guarantee that no developer text survives — no errno, no Not changing: the mock and Re-verified after the changes: node:test 300, vitest 120, typecheck clean, lint 0 errors, T1 52/52. |
|
Note on ordering: #337's implementation is now up as #441, and it touches four of the same This one should go first. It is smaller, older, already reviewed, and fixes a visible bug. The reverse order would mean reworking this PR onto |
One conflict, in app/package.json: main's stenolabs#440 appended update-error-copy.test.js to test:unit while this branch appended share-temp.test.js. Both kept — the lists are additive and both files exist. The overlap flagged in the PR's merge-order note is now resolved by main itself: stenolabs#426 landed, so "Save notes as PDF" already follows the note on screen. The semantic half of that integration is the commit that follows this one.
Two conflicts. app/package.json: main's stenolabs#440 appended update-error-copy.test.js to test:unit while this branch inserted note-sections/atomic-write/note-snapshot. Both kept - the lists are additive and all four files exist. simple_recorder.py, three times: main's stenolabs#444 replaced summary_path.write_text(...) with _atomic_write_text(...), and this branch appends _write_original_snapshot(summary_path) after each note write. Resolved as the atomic write FOLLOWED BY the snapshot, at all three sites. The order is load-bearing in both directions: _write_original_snapshot reads the note back off disk to derive the editor's diff base, so it cannot run first, and taking this branch's write_text over the atomic one would have quietly undone stenolabs#444 on exactly the file it was filed for. Their combination is strictly better than either alone: the snapshot now reads back a note that was replaced by a rename, so it can no longer capture a torn or half-written file. Neither side introduces a lock, so nothing changes about concurrent writers between the write and the read-back. The write sites correspond one-to-one across the two branches, so stenolabs#444 left no note write without a snapshot and this branch left none unatomic. Tests: python -m unittest discover tests - 526 passed, 8 skipped. Renderer typecheck clean, 167 unit tests, T1 e2e suite 63 passed. ruff is 27 findings against main's 29, all pre-existing.
The bug
The About tab pasted electron-updater's raw error text behind a hardcoded
Update download failed:prefix. Two things went wrong with that.It named the wrong phase. A check that never got off the ground was reported as a failed download, and so was a failure to install. The prefix was hardcoded, so it could not be anything else.
And it was developer text. A dev build with no update feed produced:
A stack-trace fragment with a bundle path, in an interface that otherwise speaks in sentences.
There was a second, quieter problem: a failed cycle left its banner sitting under a fresh "You're on the latest version", so the tab gave two contradictory answers to the same question, and a remount brought the settled failure back.
What this changes
A new pure module,
app/update-error-copy.js, maps an updater error to one sentence. It follows three rules:updateErrorPhase()decides from live state inmain.js. A staged update on its own means nothing here: a periodic check can fail long after a download succeeded, and calling that a failed download is the original bug.sticky). A dropped connection is disproved by the next good check; a full disk, a missing update feed or a permission problem is not, and clearing those on an unrelated success would hide a condition that is still true.Sticky failures now survive finding a version and clear only once bytes actually arrive (
download-progress), because that is what disproves them.main.jsowns that decision and announces it on a newupdate-error-clearedchannel, so the About tab follows main rather than reimplementing the rule.The raw text still goes to the debug log, which is where it is worth having.
Copy is phase- and platform-aware where it has to be: a permission error while fetching says the update could not be saved, not installed, and the "move Steno to your Applications folder" hint is macOS-only, since Windows has no such folder.
Tests
app/update-error-copy.test.js— 21 cases: phase decision, branch precedence (anEACCESonapp-update.ymlis a permission problem, not a missing feed; an HTTP status is not a connection problem), sticky semantics, the platform/phase copy, and a guard that no developer-shaped string (net::, errno, a path) reaches the user.e2e/specs/settings-about.t1.spec.ts— the existing About specs plus a race regression: a slow status reply must not restore a failure the check just settled. Verified to fail without the fix. It needs a first-call-only delay inapp/e2e-mock-ipc.js, which answers with the state as of the request, asmain.jsdoes.Locally: node:test 298 pass, vitest 120 pass, renderer typecheck clean, lint 0 errors, T1 52/52.
Notes for review
v0.6.5(9b5f590), so it sits on top of both fix(update): don't auto-update onto macOS below the 14.4 launch floor (#432) #435 and fix(release): write the macOS floor into latest-mac.yml, in Darwin form (#432) #436. It deliberately did not go into 0.6.5: it touches the update error path that those two changed twice in one day.errorevent does not say which attempt it belongs to, so a periodic check that fails while a download is genuinely running is still attributed to the download. That case was already reported as a download failure before this module existed, and closing it needs per-attempt correlation the library does not expose.Summary by cubic
Make update errors on the About tab clear and accurate: show a one‑sentence message that names the failed phase (check, download, install), clear stale banners after a successful check, and clear the banner when an update finishes downloading. This replaces raw
electron-updatertext and fixes conflicting “latest version” vs. error states.app/update-error-copy.jsto convertelectron-updatererrors into one sentence; raw text stays in debug logs.main.js(check/download/install), including transport errors during install — not mislabeled as downloads.download-progressorupdate-downloaded; emitupdate-error-cleared.update-downloaded, listens forupdate-errorandupdate-error-cleared, and drops stalegetStatus()replies via a simple sequence guard so a settled failure can’t reappear.Written for commit 8223eee. Summary will update on new commits.