Storage load guard - #158
Conversation
📊 Coverage Report
|
🧪 E2E Test Results✅ 36 passed, 0 failed, 0 skipped
|
…ccessfully Closes the startup race from #156 where the renderer could write its blank seed list over the encrypted clip history and delete the images it referenced. Main: SecureStorage records a load error when the clips file cannot be decrypted, when encryption is unavailable, or when the background load throws; getLoadState() exposes {complete, failed, error}; saveClips() refuses to run until the load has completed without error. Exposed via the storage-get-load-state IPC channel and preload storageGetLoadState(). Renderer: useClipsStorage asks for the load state before reading clips (so a load finishing in between cannot be mistaken for an empty history), keeps isInitiallyLoading set until a successful load has been applied, and on a failed load shows a toast once and leaves saves disabled. The storage-ready event still triggers the reload. Tests: src/renderer/src/providers/clips/storage.test.tsx covers the empty-before-complete race, the failed-load and thrown-read paths, and the state-before-clips ordering; src/main/storage/index.test.ts covers the load state transitions and the saveClips guard. Notes: the OKF brain MCP server named in CLAUDE.md was not reachable from this environment, so no concepts were read or updated.
cb-jeeves
added
the
Jeeves-Review
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
This closes the startup race for real: the main process refuses saveClips until the history has loaded without error, and the renderer asks for load state before it reads clips, so the blank placeholder can no longer be written over the encrypted file. The mechanism is sound and the tests cover the paths that matter. What remains is one user-facing gap (a failed load leaves saving off for the whole session with only a 12-second toast to say so) and a few small tidy-ups.
One non-inline note: importData still calls saveClipsData directly and bypasses the new guard, so a backup import during a failed load overwrites the unread history file. That is arguably what an import means, so I'd leave it, but it is worth knowing the guard is not universal.
The correctness lens failed this round and contributed nothing.
The correctness pass did not complete this round.
…hand clips out with their load state The failed-load toast expired after 12 seconds, so a user whose keystore changed saw only an empty list while saving stayed off for the session. The list now shows a banner above the rows for as long as the load error stands, carrying the same two lines plus the error itself. storage-get-clips now returns the clips together with the load state they were read under, taken in one step in the main process, so a reader cannot mistake the placeholder served during the load for an empty history. This removes the separate storage-get-load-state channel and the ordering rule that callers had to follow. StorageLoadState carries a single nullable error instead of a failed flag that had to agree with it. The storage tests import a fresh instance per test instead of resetting private fields by name.
|
Responded to review round 1, pushed 56fffa7:
Typecheck, lint and the full test suite pass. |
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since round 1, commit 56fffa7 answers every earlier thread: the toast is now a persistent banner above the list, storage-get-clips returns the clips with the load state they were read under so the ordering rule is gone, StorageLoadState is down to a single nullable error, the storage tests get a fresh instance per test, and the README says what the UI does. The race fix is sound and none of what follows blocks merging. What is left is a few small tidy-ups on the new banner and the renamed API surface.
The correctness lens failed this round and contributed nothing.
The correctness pass did not complete this round.
…d tidy the banner and snapshot API The load state now carries `recoverable`, true only when encryption was unavailable, so the banner tells a keystore-mismatch user that the history cannot be read under this keystore instead of sending them into a restart loop. The guidance lines use the banner's normal text and only the error line is mono and muted. Clips has its own flex-column root, so the App stylesheet change is reverted. The snapshot API is named getClipsSnapshot at every layer so getClips keeps meaning just the clips. errorText moved to a shared renderer util and the storage hook uses it.
|
Responded to review round 2, pushed 227b3e0:
Typecheck, lint and the full test suite pass. |
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since round 2, commit 227b3e0 answers every open thread: the banner keeps its guidance in normal text with only the raw error in mono, the main process now says whether a failed load is recoverable and the banner picks its advice from that, Clips carries its own flex root, the snapshot API is named getClipsSnapshot at every layer, and errorText lives in a shared util. The race fix is unchanged and still sound, and nothing below blocks merging. The three notes left are all about the new recoverable flag: what it defaults to, where it lives, and what the unrecoverable banner tells the user to do next.
The correctness lens failed this round and contributed nothing.
The correctness pass did not complete this round.
…adable history at the reset Fold the message and the recoverable flag into a StorageLoadError so the two fields cannot disagree across the IPC boundary and the renderer reuses the shared type instead of mapping it. Treat an unclassified load failure as recoverable, since only a clips decrypt failure is known to repeat on every launch. When it does, the banner tells the user to clear all data in Settings and restart, which is the one way out of a permanently paused session.
|
Responded to review round 3, pushed f321226:
Typecheck, lint on the changed files and the full test suite pass. |
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since round 3, commit f321226 closes every open thread (one StorageLoadError value shared across the IPC boundary, the catch-all now marked recoverable, and the unreadable-history banner pointing at clear-all-data and restart); nothing is left to raise, though the correctness lens failed this round and contributed nothing.
The correctness pass did not complete this round.
cb-jeeves
left a comment
cb-jeeves
left a comment
There was a problem hiding this comment.
Since round 4, the only change is the v2.2.3 version bump in package.json and the lockfile refresh from the merge of main; the guard itself is untouched and still sound. One small gap in the guard is worth closing before merge, but it is a one-line change and the PR is otherwise ready. The correctness lens ran this round for the first time; the security, maintainability, and ux lenses had nothing to add.
A clips file that decrypts and parses to something other than an array used to leave loadError unset, so the save guard opened and the next save replaced the file with an empty history. Throw into the existing catch instead so it is reported as an unrecoverable load failure.
|
Responded to review round 5, pushed 0e76de4:
Typecheck, lint on the changed files and the full test suite pass. |
dantheuber
left a comment
dantheuber
left a comment
There was a problem hiding this comment.
Unit tests should be brought up to 100% coverage before merging
Cover every branch of SecureStorage, the clipboard storage-integration wrappers and the renderer useClipsStorage hook, and add a provider test for the loadError hand-off to the meta context. Two branches were unreachable and are removed rather than ignored: the saveDomain initialisation guard (every entry point initialises first) and the lock cleanup for index 0 in useClipsStorage, which can never be set because locks are only recorded from index 1.
|
Responded to review feedback (80ea561):
|
|
Freight feedback log. Last round: 1 addressed, 0 declined (pushed 80ea561). |
There was a problem hiding this comment.
🟡 Changes recommended
The implemented recoverable classification for clip-load failures conflicts with the PR’s stated behavior and should be aligned to avoid incorrect user guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents startup-time data loss by ensuring the renderer cannot write placeholder/empty clip state back to encrypted storage until the main process has completed a successful background load, and by surfacing persistent load failures to the user.
Changes:
- Main storage now tracks background load completion + error state, exposes an atomic
{ loadState, clips }snapshot, and refusessaveClips()until a successful load. - Renderer consumes the snapshot API, keeps saving disabled until a good load is applied, and shows a persistent banner on failed loads.
- Adds targeted test coverage for the race and failed-load scenarios; bumps version to 2.2.3 and updates README notes.
File summaries
| File | Description |
|---|---|
| src/shared/types.ts | Adds shared IPC-safe types for storage load errors/state and the clips snapshot payload. |
| src/renderer/src/utils/errorText.ts | Introduces a small helper for consistent thrown-error stringification in the renderer. |
| src/renderer/src/utils/errorText.test.ts | Unit tests for errorText. |
| src/renderer/src/test-setup.ts | Updates renderer test API mocks to use storageGetClipsSnapshot. |
| src/renderer/src/providers/clips/types.ts | Extends clips meta context typing to carry a storage load error. |
| src/renderer/src/providers/clips/storage.ts | Implements the renderer-side save guard using the snapshot load state + adds loadError output. |
| src/renderer/src/providers/clips/storage.test.tsx | Adds tests covering the empty-before-complete race and error/throw paths. |
| src/renderer/src/providers/clips/README.md | Documents the new “saving stays disabled until successful load” behavior. |
| src/renderer/src/providers/clips/index.tsx | Plumbs loadError from useClipsStorage into clips meta context. |
| src/renderer/src/providers/clips/index.test.tsx | Tests that the provider exposes and clears load errors via context. |
| src/renderer/src/components/settings/tools/useToolsData.ts | Switches newest-clip read to the snapshot API. |
| src/renderer/src/components/settings/tools/useToolsData.test.tsx | Updates tests for the snapshot API usage. |
| src/renderer/src/components/settings/tools/Tools.tsx | Switches errorText import to the new shared renderer util. |
| src/renderer/src/components/settings/tools/harness.tsx | Updates tools harness mocks to return a snapshot. |
| src/renderer/src/components/settings/tools/ExportImport.tsx | Switches errorText import to the new shared renderer util. |
| src/renderer/src/components/settings/general/SettingsProvider.tsx | Switches errorText import to the new shared renderer util. |
| src/renderer/src/components/settings/general/ImportPreview.tsx | Switches errorText import to the new shared renderer util. |
| src/renderer/src/components/settings/general/General.tsx | Switches errorText import to the new shared renderer util. |
| src/renderer/src/components/settings/general/ClearAll.tsx | Switches errorText import to the new shared renderer util. |
| src/renderer/src/components/clips/Clips.tsx | Adds a persistent “load failed” banner and renders it when loadError is present. |
| src/renderer/src/components/clips/Clips.test.tsx | Tests banner rendering and recoverable vs non-recoverable guidance text. |
| src/renderer/src/components/clips/Clips.module.css | Adds layout + styling for the new load-failed banner and adjusts list container flex behavior. |
| src/preload/index.ts | Replaces storageGetClips with storageGetClipsSnapshot IPC invoke. |
| src/preload/index.d.ts | Updates the global window.api typing for storageGetClipsSnapshot. |
| src/main/storage/index.ts | Adds load error/state tracking, exposes getClipsSnapshot(), and guards saveClips() until successful load. |
| src/main/storage/index.test.ts | Adds comprehensive tests for load state transitions, snapshot semantics, and the save guard. |
| src/main/clipboard/storage-integration.ts | Updates integration API to return clips snapshot and load state together. |
| src/main/clipboard/storage-integration.test.ts | Tests updated storage integration behavior for snapshot reads and error handling. |
| src/main/clipboard/ipc.ts | Wires a new IPC handler storage-get-clips-snapshot (replacing the old clips read channel). |
| README.md | Documents that nothing is written back until history load succeeds, and failures pause saving with a banner. |
| package.json | Bumps app version to 2.2.3. |
| package-lock.json | Updates lockfile version fields for 2.2.3 and reflects re-serialized package metadata. |
Review details
- Files reviewed: 28/32 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // A clips file exists but cannot be read (for example the keystore changed). | ||
| // Reporting the history as empty here would let the renderer save over it, and a | ||
| // decrypt failure repeats on every launch, so a restart will not clear it. | ||
| console.error('Failed to load clips:', error); | ||
| this.loadError = { message: errorMessage(error), recoverable: false }; |
| * Why the stored history could not be read. `recoverable` is true when the next launch may | ||
| * read it (the keystore was locked or unavailable, or something else failed on the way) and | ||
| * false when the file cannot be read under this keystore at all, so a restart would only | ||
| * repeat the failure. |
This branch assembles the listed issues for review.
Issues in this consist
Closes #156
What & why
Fixes a startup race where the renderer could write its blank seed clip list over the encrypted clip history before the initial load finished, permanently deleting the images that history referenced. Saves are now blocked until storage has confirmed a successful load, and a failed load now surfaces persistently instead of disappearing after a few seconds.
Changes
SecureStoragerecords a load error when the clips file can't be decrypted, encryption is unavailable, or the background load throws.getLoadState()exposes{complete, failed, error}, andsaveClips()refuses to run until the load has completed without error.storage-get-clipsnow returns clips together with the load state they were read under, taken in one step, so a reader can't mistake the placeholder shown during a load for an empty history — this replaces the separatestorage-get-load-statechannel and the ordering rule callers previously had to follow.StorageLoadErrorvalue carrying both the message and arecoverableflag, so the two fields can't disagree across the IPC boundary. Only a clips-decrypt failure is treated as non-recoverable; an unclassified failure is assumed to clear on restart.useClipsStoragereads load state and clips together, keepsisInitiallyLoadingset until a successful load has been applied, and on a failed load shows a persistent banner (instead of a toast) with guidance text and disables saves for the session. Thestorage-readyevent still triggers a reload. The clips list has its own snapshot API (getClipsSnapshot) sogetClipscontinues to mean just the clips.Review notes
npm run typecheck && npm run test).SecureStorage(main) anduseClipsStorage(renderer) — this is what closes the original race.src/renderer/src/providers/clips/storage.test.tsx(empty-before-complete race, failed-load/thrown-read paths, state-before-clips ordering) andsrc/main/storage/index.test.ts(load state transitions,saveClipsguard, recoverable vs. non-recoverable errors).mainat the same 2.2.2 version this branch had already claimed, so the bump was reapplied one patch level up.