feat(audit): Batch 16 — store discipline (L142 reconciliation, L93 view-model hooks)

L142 (+CC14): keyed latest-wins reconciler (lib/reconcile.ts, unit-tested)
applies main's authoritative IPC returns in the history/templates/sources
stores, extending the M34 settings pattern; loadSources shares the key so
a slow list read can't clobber a newer mutation result.

L93 (+CC13): settings cards, TerminalView, and LibraryView now reach IPC
only through colocated view-model hooks (useAboutCard/useBackupCard/
useCookiesCard/useSoftwareUpdateCard/useNetworkCard/useTerminalRun) or
store actions (openHighContrastSettings, scheduled-sync on sources) —
no window.api left in components.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-02 11:10:49 -04:00
parent 96800d5a17
commit 2ef19ead71
21 changed files with 816 additions and 366 deletions
+35 -23
View File
@@ -935,12 +935,22 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
([preload/index.ts](src/preload/index.ts)); the renderer callers ([store/sources.ts](src/renderer/src/store/sources.ts))
and the preview mock ([mockApi.ts](src/renderer/src/mockApi.ts)) updated to suit. The `Api` type derives from
the preload, so the rename is enforced end-to-end by the compiler.*
- [ ] **L93 — SettingsView bypasses the store layer.** It calls `window.api` directly for
- [x] **L93 — SettingsView bypasses the store layer.** It calls `window.api` directly for
cookies/ffmpeg/yt-dlp/app-update/backup while using stores for settings/templates/errorlog — mixed
data-access in one component. *Deferred (CC13's concrete instance): these one-off, card-local IPC reads
(version checks, backup export/import, cookie sign-in) have no shared state to own, so a store per call is
arguably overkill — but the "no `window.api` in components" convention argues for thin hooks. Best done as
the focused CC13 view-model pass, not folded into this copy/cleanup batch.*
data-access in one component. *Fixed (Batch 16, the CC13 view-model pass): each card's IPC + state machine
moved into a colocated view-model hook on the `useDownloadBar` pattern —
[useAboutCard](src/renderer/src/components/settings/useAboutCard.ts) /
[useBackupCard](src/renderer/src/components/settings/useBackupCard.ts) (reloads the settings/templates
stores via new store `reload()` actions after a restore) /
[useCookiesCard](src/renderer/src/components/settings/useCookiesCard.ts) /
[useSoftwareUpdateCard](src/renderer/src/components/settings/useSoftwareUpdateCard.ts) /
[useNetworkCard](src/renderer/src/components/settings/useNetworkCard.ts) (PO-token mint); AppearanceCard's
one-liner became a settings-store action (`openHighContrastSettings`). The sweep also cleared the two
non-settings components: TerminalView's run/stream/stop →
[useTerminalRun](src/renderer/src/components/useTerminalRun.ts), and LibraryView's scheduled-sync IPC →
sources-store state/actions (`scheduledSyncEnabled`/`loadScheduledSync`/`setScheduledSync`/
`commitSyncTime`). No `window.api` remains in any component (`useDownloadBar` is itself the view-model
layer, per CC13).*
- [x] **L94 — `summarizeQueue` recomputed every render** in [DownloadsView.tsx](src/renderer/src/components/DownloadsView.tsx)
(no `useMemo`) — and again on every store change in App's taskbar effect. Memoize/share. *Fixed: a 1-entry
items-ref memo `queueSummaryOf` in [queueStats.ts](src/renderer/src/store/queueStats.ts) — the store hands
@@ -1191,14 +1201,18 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
*Fixed in [store/history.ts](src/renderer/src/store/history.ts): `add` now `.slice(0, MAX_ENTRIES)` (500, the
same cap as main's `history.ts`) and de-dupes by `url` as well as `id`, so the optimistic in-memory list
matches what a reload would show (also aligns with main's M35 url de-dup).*
- [ ] **L142 — IPC mutation return values are discarded everywhere.** history/templates/sources/settings
- [x] **L142 — IPC mutation return values are discarded everywhere.** history/templates/sources/settings
IPC calls return the authoritative (validated/capped/sanitized) state, but every renderer caller does
optimistic-only updates and ignores it — client and persisted state can silently diverge until reload (generalizes M34).
*Deferred (targeted, own PR): settings already reconcile (M34). Extending "apply the returned authoritative
state" to history/templates/sources touches each store's mutation path and each corresponding IPC return
shape, and the divergence it guards against is rare (only when main caps/sanitizes differently than the
optimistic update) and self-heals on reload. Best done as one focused reconciliation PR with each store
verified, not folded into this UX batch. Standard already recorded under CC14.*
*Fixed (Batch 16, the focused reconciliation PR): a keyed latest-wins reconciler
([lib/reconcile.ts](src/renderer/src/lib/reconcile.ts), unit-tested in
[test/reconcile.test.ts](test/reconcile.test.ts)) applies main's authoritative return on every mutation —
history add/remove/removeMany/clear, template save/remove, and sources setWatched/removeSource/
setMediaItemDownloaded (keyed per source so overlapping completions don't suppress each other) — extending
the M34 settings pattern to all four stores. The seq guard drops a stale response when a newer call was
issued on the same collection, so an overlapped pair can't transiently roll back the newer optimistic
update; `loadSources` shares the mutations' key so a slow list read can't clobber a newer authoritative
result either.*
- [x] **L143 — No in-window "quit anyway."** With a download active (or tray mode), closing only hides;
quitting requires the tray menu. If the tray ever fails to create, the only exit is Task Manager
(mitigated today by the embedded fallback tray icon).
@@ -2001,25 +2015,23 @@ split — but that style is unenforced and several "do the same thing two ways"
exactly the kind of sweeping move that wants its own reviewed PR, not an unattended batch. The `lib/`
convention is already established (both sides have one and new pure utils land there); the wholesale
`views/`+`core/` split is the 1.x task. Standard recorded.*
- [ ] **CC13 — View/state boundary (the project's "MVVM").** It's React+Zustand, but the container/
- [x] **CC13 — View/state boundary (the project's "MVVM").** It's React+Zustand, but the container/
presentational split is inconsistent: orchestration lives in stores for downloads/sources yet inside the
component for DownloadBar, and SettingsView calls `window.api` directly (L93, UX1). **Standard:** stores/
hooks are the view-models (own orchestration + all IPC); components stay presentational; no `window.api` in components.
*Partly closed / deferred: the substantive split largely holds — downloads/sources orchestration lives in
stores, and DownloadBar's was extracted into the `useDownloadBar` hook (H1), a view-model in all but name.
The concrete remaining leak is SettingsView calling `window.api` directly, filed as **L93** (Batch 13);
the broader "no `window.api` in any component" sweep rides with it. No new work needed here beyond L93.*
- [ ] **CC14 — State ownership is unprincipled.** State lives in Zustand stores, component `useState`,
*Closed with L93 (Batch 16): the settings cards, TerminalView, and LibraryView now reach IPC only through
colocated view-model hooks or store actions; `grep window.api src/renderer/src/components` matches only
hooks (`use*.ts`). The standard holds codebase-wide: stores/hooks own orchestration + IPC, components render.*
- [x] **CC14 — State ownership is unprincipled.** State lives in Zustand stores, component `useState`,
`localStorage`, the main `electron-store` (mirrored into a renderer store), and module-level vars
(`idCounter`, `notifiedBackground`, `active`). Persisted state is optimistic-only and never reconciled
(M34/L142); two stores form a cycle (C2). **Standard:** main owns persisted state; renderer stores mirror
it and **apply the authoritative value the IPC call returns**; UI-only state in stores; break store cycles via a coordinator.
*Largely closed / deferred: main owns persisted state and the renderer mirrors it; the store cycle is gone
(C2, via the coordinator event bus); settings now **apply the authoritative IPC return** (M34); and
`localStorage` was folded into the settings store (M19). The one remaining "optimistic-only, never
reconciled" case is the other IPC mutations (**L142**, Batch 11). The module-level vars called out
(`idCounter`/`notifiedBackground`/`active`) are legitimately module-scoped singletons, not misplaced UI
state. Closes with L142.*
*Closed with L142 (Batch 16): every persisted-state mutation now applies main's authoritative return
(history/templates/sources via the keyed reconciler; settings via M34). Main owns persisted state, the
renderer mirrors + reconciles, the store cycle is gone (C2), `localStorage` is folded in (M19). The
module-level vars called out (`idCounter`/`notifiedBackground`/`active`) are legitimately module-scoped
singletons, not misplaced UI state.*
### Recommended single style (apply project-wide)