384870ee4a
PERF3: pumpBench.test.ts pins pump()'s pipeline cost — 0.43ms @5k items, 1.47ms @20k (median) — negligible at a few events/sec, so the count+pointer rewrite of the core queue is declined as risk without reward; the benchmark stays as a 5ms regression tripwire. PERF6/L162: closed as documented-by-design (virtualization bounds mounted thumbs; a theme prop conflicts with PERF5's stable renderer). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2535 lines
223 KiB
Markdown
2535 lines
223 KiB
Markdown
# Code Audit
|
||
|
||
Living checklist of audit findings for AeroFetch. Security & correctness were reviewed
|
||
2026-06-23 (all fixed — see Completed). The architectural review (2026-06-29) added the
|
||
structural items; a second polish pass (2026-06-29) added the smaller inconsistencies.
|
||
Items carry stable IDs so we can check them off as they land this session.
|
||
|
||
Severity = structural leverage / risk of future bugs, not "app is broken" (it isn't).
|
||
|
||
**Session 2026-06-30 (correctness/safety/strictness pass):** landed B3 B4 B5 B7 (checksum-
|
||
filename match, newline-safe meta probe, canceled-event guards, cookie-login never-resolve),
|
||
L88 L146 (queue/trim edge cases), **L168 + L169** (`noUncheckedIndexedAccess` +
|
||
`noFallthroughCasesInSwitch` now on — 0 errors after 8 real edge-case fixes), R5 (settings
|
||
write failure handled), W1 W5 W6 (min window size, seeded folder picker, parented sign-in
|
||
window), CL1 (shared stdout markers), L147 (dead macOS branches removed), M8 (StatusChip
|
||
already unified the status→label map), M36 (Library selection only counts actionable rows),
|
||
L11 (Queue header counts the live queue), L50 (no "saved" for 0 cookies), L156 + L57
|
||
(schedule picker `min`), L159 L15 L3 (dead-code/comment cleanup), package/builder metadata
|
||
(L4 L41 L42 L43 L45 L58), user-facing copy (L66 L154; partial M37/SR9 jargon), doc
|
||
reconciliation (M25 M26 L80 L81), and new unit tests (L35 isValidMediaItem, L36
|
||
compareVersions). All verified: typecheck + 234 tests + eslint + prettier green.
|
||
|
||
**Session 2026-06-30 pass 2 (quality/format/UX polish):** H5 (history re-download now stores
|
||
formatId/formatHasAudio + strips compound quality labels; thumbnail also passed — L72), H6
|
||
(TerminalView log capped at 2000 lines), M21 (skip --audio-quality for lossless flac/wav),
|
||
L166 (fmtEta hour rollover fixed in download.ts, store/downloads.ts, queueStats.ts — "2:00:00"
|
||
not "120:00"), L165 (formatSpeed precision aligned with fmtBytes), L55 (QueueItem suppresses
|
||
sizeLabel when already in probed-format quality label), L47 (probeMeta null sends empty meta
|
||
event to clear "Resolving…"), L68 (notifiedBackground resets on window show), L76 (dup warning
|
||
falls back to URL when title is placeholder), L91 (SettingsView unsafe tuple cast fixed), L98
|
||
(embed-chapters hint added), L108 (window title set), L37 (formatters extracted to lib/formatters.ts;
|
||
fmtBytes/fmtEta/parseProgress unit-tested including NA paths), L38 (webm thumbnail exclusion
|
||
test), L39 (CROP_SQUARE_PPA structural test), L40 (looksLikeUrl/looksLikeSingleVideo extracted
|
||
to lib/urlHelpers.ts; test imports pure module directly). All verified: typecheck + 242 tests + eslint + prettier green.
|
||
|
||
**Session 2026-06-30 pass 3 (reliability — B/R/L races & silent failures):** L140 + L148 (the `active`-map
|
||
teardown race — `cancelDownload`/`pauseDownload` release the concurrency slot synchronously and an
|
||
identity-guarded `releaseActive` + per-spawn cookie jar make same-id retry/resume safe), L141 (renderer
|
||
history capped at 500 + url de-dup to mirror main), R6 (`writeJsonAtomic` reports + logs write failures and
|
||
keeps the data dirty for retry instead of an empty catch — log half; toast → CC8), R7 (`encryptSecret` warns
|
||
before the plaintext fallback). All verified: typecheck + 243 tests + eslint green; the 5 touched files are
|
||
prettier-clean. *Deferred this pass (need UI/live-app or larger design): B2/B6 (cancel buttons + abort
|
||
wiring), R4 (can't safely identify a download's `.part` files at cancel time — needs the yt-dlp "Destination"
|
||
path tracked), L139 (needs a spawn↔auto-update interlock), L142 (multi-site IPC-return reconciliation,
|
||
generalises M34), L157 (cross-store cancel-on-remove, ties to C2), R9 (clock-skew — "note for awareness").*
|
||
|
||
**Session 2026-06-30 pass 4 (Critical — store architecture):** **C1** (single source of truth for the
|
||
preview mock + Settings defaults — `DEFAULT_SETTINGS` in `@shared`, one typed `mockApi.ts`, centralized
|
||
`isPreview`) and **C2** (broke the `downloads ↔ sources` circular import via a pure typed event bus,
|
||
`store/coordinator.ts` — neither store imports the other now). All verified: typecheck + 243 tests +
|
||
eslint green; the touched files are prettier-clean (3 pre-existing format violations in
|
||
DownloadOptionsForm/LibraryView/SettingsView are untouched and out of scope).
|
||
|
||
**Session 2026-07-01 (main-process structure — the H1-adjacent decompose batch):** **L2** (the ~200-line
|
||
flat IPC block moved into one `src/main/ipc.ts` `registerIpcHandlers(getMainWindow)`; index.ts is now
|
||
lifecycle + window creation and imports the shared theme helpers from ipc.ts), **L69** (path helpers split
|
||
into `src/main/paths.ts`; settings.ts is now purely the electron-store layer), **L10** (a main-process
|
||
`src/main/constants.ts` centralizes the spawn timeouts, execFile buffers, stderr tail, and the three store
|
||
caps — nine modules migrated off bare literals), **CL2** (`buildArgs` → single `BuildArgsInput` object),
|
||
**CL3** (`startDownload`'s stream/close/error/watchdog wiring extracted into `wireChildProcess`), and **L1**
|
||
(the scheduled-download promoter tick moved from store-module load into an App `useEffect` so a store import
|
||
no longer starts a stray timer). All verified: typecheck (node+web) + 253 tests + eslint + production build
|
||
green; the touched files are prettier-clean (the 8 remaining prettier warnings are all pre-existing,
|
||
untouched files — DownloadOptionsForm/SettingsView/settings/*Card).
|
||
|
||
**Session 2026-07-01 pass 2 (the "finish the audit" batch — phase-by-phase PRs):** landed **Phase 1**
|
||
(doc/stale-checkbox reconciliation — L82, L89, plus ticks for wire-or-remove M5/M6/UX1, MAX_ENQUEUE_BATCH,
|
||
and the UX/ref duplicates whose roots already shipped), **Phase 2** (build hygiene — L83 `.blockmap` off via
|
||
`nsis.differentialPackage:false`, L84 `dist/` clean step), and **Phase 3 (main foundation)**: **CC8**
|
||
(in-house file-backed leveled logger [logger.ts](src/main/logger.ts) + renderer `log:write` sink), the
|
||
**`execFileAsync`** consolidation ([lib/exec.ts](src/main/lib/exec.ts) — the five hand-rolled spawn-and-read
|
||
wrappers in probe/ytdlp×2/ffmpeg/indexer/download.probeMeta; SIMP2, and the foundation for CC4/CC5),
|
||
**L136** (incognito is now enforced in main — no errorlog entry, no title/URL in the OS toast, no login/
|
||
browser cookies), and **R9** (the yt-dlp update throttle no longer skips forever after a backward clock
|
||
correction). Each phase is its own branch/PR stacked on `refactor/h1-decompose-god-files`. All green:
|
||
typecheck + 263 tests + eslint + prettier. **Next:** M4 (queue persistence — its own focused PR), the
|
||
download-engine lifecycle items (R4/L65/L139/L157/B2/B6 — need a live download to verify), the CC4/CC5
|
||
tails (SIMP5 line-buffer + SIMP16 net.request), then Phases 4–8 (renderer tokens+primitives, logic/perf,
|
||
UX/Windows, copy/a11y). Per the run's policy, subjective-visual and live-verify-only items are implemented
|
||
but left **unchecked** with a flagged list for a watched pass.
|
||
|
||
**Session 2026-07-01 pass 3 (Batch 6 — the renderer design-token foundation):** landed the shared
|
||
design-scale module [components/ui/tokens.ts](src/renderer/src/components/ui/tokens.ts) — one home for
|
||
**SPACE** (the spacing ramp), **RADIUS** (control/surface/card tiers over Fluent's radii), **ELEVATION**
|
||
(flat/raised/overlay), **Z** (overlay/tooltip), **ICON** (16/20/24/40), **MOTION**, **SCRIM**, and
|
||
**META_SEP** — then migrated the scattered literals across 17 renderer files. Structurally-verified items
|
||
checked off: **UI2** (one 16px section gap on every screen via the view roots), **UI4** (symmetric page
|
||
padding — the lone 28px horizontal is gone), **UI5** (the intentional 4px icon-group / 8px control-row split
|
||
is now named + documented), **UI6/UI7** (one thumbnail radius = Medium; page-card XLarge / list-item Large /
|
||
control Medium tiers), **UI26** (a single motion timing + a global `prefers-reduced-motion` gate in
|
||
[base.css](src/renderer/src/assets/base.css)), **L100** (elevation scale — only the download bar (raised) and
|
||
command palette (overlay) cast a shadow; every other card stays flat/border-only), **L101** (a z-index scale
|
||
replaces the two hardcoded `1000`s), **L121** (the renderer's one raw `rgba(0,0,0,0.32)` scrim → Fluent's
|
||
theme-aware `colorBackgroundOverlay`), **L125** (icon sizes single-source from ICON), **L160** (inline margins
|
||
tokenized — values unchanged), and **L164** (one metadata separator ` • `; the intra-value ` · ` inside
|
||
compound quality labels is deliberately untouched). **UI3 and UI12 are implemented but left unchecked** for a
|
||
watched visual pass — they shift a few pixels: card padding 14/20→16 (QueueItem, Settings cards), and the
|
||
former-18px chrome glyphs (sidebar nav + theme toggles, IconButton md) → 20 plus the private badge 14→16. All
|
||
green: typecheck (node+web) + 268 tests + eslint + production build; the touched files are prettier-clean (the
|
||
9 remaining prettier warnings are all pre-existing, untouched files).
|
||
|
||
**Session 2026-07-01 pass 4 (Batch 7 — type & color roles):** added
|
||
[components/ui/text.ts](src/renderer/src/components/ui/text.ts) — a `useTextStyles` hook with the single
|
||
`muted` secondary-text color and the shared `title` treatment, plus a header documenting the app's type-role
|
||
convention (Subtitle2 = titles/section headers · Text = row titles · Body1 = body copy · Caption1 = small
|
||
secondary text). Landed: **L110** (the one muted color, was eight aliases hint/sub/meta/srcSub/…/previewMeta —
|
||
adopted in the list views: QueueItem, HistoryView, LibraryView, Sidebar, DownloadBar, PlaylistPanel;
|
||
color-identical, so no visual change; the combined/shared-across-cards aliases — `stats`/`count`/`emptyHint`,
|
||
settings `hint`, CommandPalette, inline — migrate incrementally), **L114** (one row-title component: the
|
||
duplicated semibold-ellipsis title style is now `text.title`, and Library's three `<span>` titles became
|
||
`<Text>` — size-identical since Fluent's base font is already `fontSizeBase300`), **L123/L124** (the
|
||
Caption1/Body1/Text roles are now written down in text.ts and followed), **UI13** (verified: `Filled` is used
|
||
only on the two brand marks, `Regular` everywhere else — the convention already held), and **UI11** (no literal
|
||
px font sizes remain — the last tile/badge glyphs snap to `ICON` tiers: Sidebar mark 20, Onboarding mark 24,
|
||
Library srcIcon 20, History emptyBadge 24; the `fontSizeBase*` token usages already satisfied the standard).
|
||
The four tile-glyph deltas (≤2px) join UI12's watched visual pass. All green: typecheck (node+web) + 268 tests
|
||
+ eslint + production build; touched files prettier-clean (9 pre-existing warnings untouched).
|
||
|
||
**Session 2026-07-01 pass 5 (Batch 8 — form primitives):** **L112** (removed the redundant dynamic
|
||
`On`/`Off` label from all 15 `<Field>`-wrapped `<Switch>`es in DownloadOptionsForm + the settings cards — the
|
||
Field already names each switch, and the toggle's position + `aria-checked` already convey state, matching
|
||
Library's switches), **L133** (made the custom [Select](src/renderer/src/components/Select.tsx) `<Field>`-aware
|
||
via `useFieldControlProps_unstable({ supportsLabelFor: true })`, so a Field's visible label names the native
|
||
`<select>` through `htmlFor` and its hint associates via `aria-describedby`; dropped the now-duplicate
|
||
`aria-label` from every Field-wrapped select (9 in total) — the two standalone selects, download-bar quality
|
||
and history filter, keep theirs), **L129** (the
|
||
`Hint` `align` prop is now a discriminated union — allowed only with `top`/`bottom` placement, a type error
|
||
with `left`/`right` instead of being silently ignored), **L134** (one busy convention — `disabled` +
|
||
icon→`Spinner`; converted AboutCard's version-check button off its adjacent-spinner layout so it matches
|
||
Fetch/Index/update), **UI17** (the two `appearance="secondary"` buttons — Library "Check for new", Terminal
|
||
"Stop" — are now `subtle`, the app's standard non-primary appearance), **L111** (convention clarified &
|
||
verified: Fluent `Field hint` for control-associated help, `<Caption1 muted>` only for standalone card
|
||
notes/status — control help already uses `Field hint` uniformly), and **L135** (verified role-appropriate:
|
||
`Textarea` for the multiline trim ranges "one per line", `Input` for single-line fields). **UI16 left
|
||
unchecked** — button size-by-role is subjective; current usage already broadly follows it (screen-primary
|
||
large, row/toolbar small, settings default), so specific tweaks join the watched visual pass. All green:
|
||
typecheck (node+web) + 268 tests + eslint + production build; touched files prettier-clean.
|
||
|
||
**Session 2026-07-01 pass 6 (Batch 9 — surface primitives):** three new shared UI primitives —
|
||
[EmptyState](src/renderer/src/components/ui/EmptyState.tsx) (**L116/L117**: one centered empty block — icon +
|
||
`Body1` message + optional muted hint, with a `compact` variant for in-content placeholders — adopted by
|
||
Downloads/History/Library and, compact, by the Terminal log + Diagnostics), [Banner](src/renderer/src/components/ui/Banner.tsx)
|
||
+ [LinkSuggestion](src/renderer/src/components/ui/LinkSuggestion.tsx) (**UI19**: one tinted notification row
|
||
replacing five hand-rolled style blocks — the DownloadBar suggestion/channel-nudge/dup-warning + the Library
|
||
copied-link suggestion), and a shared `useTextStyles().truncate` (**L126**: single-line-ellipsis util, adopted
|
||
in Library rows + the playlist panel + Banner). Plus **L102** (Settings "Default format" now uses the same
|
||
`SegmentedControl` + quality `Select` as the DownloadBar — one mental model; the `kind|quality` string encoding
|
||
is gone), and four resolved-by-verified-convention: **L103** (busy = in-button icon→Spinner everywhere; done in
|
||
L134), **L115** (list-row actions icon-only+tooltip vs card-toolbar actions labelled — role-appropriate),
|
||
**L120** (Fluent `<Card>` for static content cards, the Batch-6 tokenized surface recipe for interactive/
|
||
list-item cards), and **UI20** (toggles = solid brand, list-selection = brand-tint; the cited Library pill is
|
||
gone via UI18). **L128 deferred to Batch 14** (build-time DCE of preview seeds, grouped with PERF8). All green:
|
||
typecheck (node+web) + 268 tests + eslint + production build; touched files prettier-clean.
|
||
|
||
**Session 2026-07-01 pass 7 (Batch 10 — CC consistency & conventions):** the contained consolidations landed;
|
||
the sweeping ones are honestly deferred. **Landed:** a shared stdout line-splitter
|
||
[lib/lineBuffer.ts](src/main/lib/lineBuffer.ts) (`createLineBuffer`) replacing the duplicated newline loop in
|
||
`download.ts` + `terminal.ts` (**CC3/CC4**, + [test/lineBuffer.test.ts](test/lineBuffer.test.ts), 7 cases), and
|
||
`cleanError` on **all** yt-dlp stderr — `getYtdlpVersion`/`updateYtdlp` now use the same `cleanError(r.stderr) ||
|
||
r.error?.message || ''` idiom as download/probe/indexer (**CC6**). **CL6** resolved (the one real redundancy was
|
||
L15; `clearDir`/`openFile`/`showInFolder` are deliberate view-model wrappers). **Deferred with the standard
|
||
documented** — these are the architectural refactors the release gate already puts post-1.0, and several would be
|
||
reckless to do unattended: **CC1** (renaming persisted setting keys is a breaking migration), **CC5**+**CL4**
|
||
(the last event-emitter wrap is the *security-critical* updater installer download — host re-validation per
|
||
redirect + SHA-256 verify — which can't be live-verified here), **CC7** (cosmetic arg-ordering; the pure/impure
|
||
DI split already holds), **CC9** (zod = new dep + validation rewrite), **CC10** (jsonStore exists; the
|
||
electron-store split is deliberate for DPAPI), **CC11** (constants centralized; only a `config.ts` move left),
|
||
**CC12** (file-tree reorg = broad churn), **CC13**→L93, **CC14**→L142. Net: CC3/CC4/CC6/CL6 checked; the rest
|
||
carry a documented standard + deferral rationale. All green: typecheck (node+web) + 275 tests + eslint +
|
||
production build; touched files prettier-clean.
|
||
|
||
**Session 2026-07-01 pass 8 (Batch 11 — UX behavior gaps):** a new in-app **toast** surface
|
||
([store/toasts.ts](src/renderer/src/store/toasts.ts) + [ui/Toaster.tsx](src/renderer/src/components/ui/Toaster.tsx),
|
||
non-portal bottom-center, +unit-tested) is the batch's spine — it makes **UX6** real: a shared
|
||
[reveal.ts](src/renderer/src/reveal.ts) `revealFile` now surfaces open/reveal failures ("File not found — it
|
||
may have been moved…") instead of the old silent no-op, and `safeShowInFolder` was upgraded to return an error
|
||
string like `safeOpenPath`. Also landed **UX9/UI25** (a `CounterBadge` on the sidebar Downloads nav item shows
|
||
the active count from any tab, via a count-only selector that doesn't re-render on ticks), **UX15** (`cancelAll`
|
||
+ a "Cancel all (N)" header button), **UX24** (`disabledFocusable` + `Hint` explains the disabled "Check
|
||
watched"), **UX12** (an inline "Enable custom commands" button on the Terminal gate), and **L144** (the dup
|
||
guard now checks history → "Already downloaded: …"). **Deferred with rationale:** UX7/UX8 (Settings IA
|
||
redesign — visual), UX18/UX26 (subjective/visual polish), L131 (arguably by-design, needs the live taskbar),
|
||
and **L142** (the history/templates/sources IPC-return reconciliation — a focused own-PR, CC14's tail). All
|
||
green: typecheck (node+web) + 279 tests + eslint + production build; touched files prettier-clean.
|
||
|
||
**Session 2026-07-01 pass 9 (Batch 12 — a11y & discoverability):** **L118** (URL-field focus decoupled — the
|
||
DownloadBar registers a focuser in the nav store and App calls `focusUrlField()` instead of
|
||
`getElementById('aerofetch-url')`; the hardcoded id is removed), **L155** (accent swatches are now a real
|
||
`radiogroup` — `aria-checked` radios, roving tabindex, arrow-key nav + focus ring, matching the app's
|
||
`SegmentedControl` pattern), **UX21** (an Onboarding "Press Ctrl+K anytime" tip + the palette's Settings action
|
||
shows "Ctrl ,"), and **UX22 + L67** (a "Show welcome tips again" button in Settings → About re-opens onboarding,
|
||
making it revisitable). Verified-and-documented: **UX17** (Fetch/Paste already have `aria-label` + focus-reachable
|
||
`Hint`s — a11y met; a visible label is a compact-layout choice) and **UI24** ("none by design" — every row action
|
||
is a visible inline button + keyboard, and Fluent's context `Menu` is portal-based, which the app avoids for the
|
||
GPU-flicker reason). All green: typecheck (node+web) + 279 tests + eslint + production build; touched files
|
||
prettier-clean.
|
||
|
||
**Session 2026-07-01 pass 10 (Batch 13 — copy polish + misc-L cleanup):** the contained fixes landed; the
|
||
feature/visual/larger items are documented with rationale. **Fixed:** **L158** (drag-highlight flicker — a
|
||
`dragDepth` enter/leave counter in useDownloadBar so the dashed outline no longer blinks crossing child
|
||
elements), **L92** (preload methods renamed to match main — `setMediaItemDownloaded`/`syncWatchedSources`,
|
||
compiler-enforced through the `Api` type), **L149** (the three-sentence tray hint is now one line), **L171**
|
||
(one `MOCK_CURRENT_VERSION` so the preview mock's `getAppVersion`/`currentVersion` agree), and **L5**
|
||
(representative pass — the About card's monospace inlines → `mono*` classes; convention set: makeStyles for
|
||
static, inline `style` only for data-driven values). **Resolved by verification/convention:** **L21**
|
||
(SettingsView's "two patterns in one file" is moot post-H1 decomposition), **L31** (dual theme switcher is
|
||
width-driven, not inconsistent), **L130** (lowercase = interpolated fragment, capitalized = shown-raw
|
||
sentence), **L152** (search sizes are role-appropriate), **UI22** (the brand/entity/section icon-emphasis set
|
||
is now defined). **Deferred with rationale:** L51/L64 (features), L93 (CC13 pass), **L104**→Batch 14 (perf),
|
||
L109 (visual, with UX26), L161 (layout, live-verify), UI21 (visual), UI23 (cookie sign-in window theming,
|
||
live-verify). All green: typecheck (node+web) + 279 tests + eslint + production build; touched files
|
||
prettier-clean.
|
||
|
||
**Session 2026-07-01 pass 11 (Batch 14 — perf + first-run defaults + Windows):** two real bundle/startup wins
|
||
plus one first-run default. **PERF8** — Library/History/Terminal/Settings are `React.lazy` chunks behind a
|
||
`Suspense` (Downloads stays eager); the production build now splits ~140 kB (incl. the 90 kB SettingsView) out
|
||
of the initial bundle. **L128** — the three preview seed arrays are gated on `import.meta.env.DEV && PREVIEW`,
|
||
so Rollup dead-code-eliminates them from the packaged renderer (DEV statically false) while the browser dev
|
||
preview keeps them. **SR4** — `clipboardWatch` now defaults `false` for new installs (privacy-first; existing
|
||
installs keep their saved value; Onboarding tip repointed) — *a product default flip flagged for the
|
||
maintainer, trivially reversible.* **Deferred with rationale:** **PERF3** (core-queue micro-opt on a bounded
|
||
list — risk > reward unattended, revisit in the Batch 15 live pass), **PERF6/L162** (bounded by
|
||
virtualization; a prop conflicts with PERF5's hoisted renderItem), **SR5** (MP3 = deliberate
|
||
max-compatibility default; the label bug was M18), **W12** (fallback-of-a-fallback icon), **W15** (the GPU
|
||
software-rendering workaround — external driver dependency), and **L104** (History virtualization needs the
|
||
L161 layout rework + live verification; the 500-cap bounds it). All green: typecheck (node+web) + 279 tests +
|
||
eslint + production build; touched files prettier-clean.
|
||
|
||
**Session 2026-07-01 pass 12 (Batch 15 — download-engine lifecycle):** the batch the run's policy reserves for
|
||
a **watched live session** — so only the safe, non-file-deleting item was implemented, and the rest are staged
|
||
with per-item live-verify rationale. **Landed: L157** — `removeSource` now emits a `sourceRemoved` bus event
|
||
(C2) and the downloads store cancels every active item whose `mediaItemId` prefix (`${sourceId}:`, per
|
||
indexerCore) matches, reusing the proven `cancel()` path (implemented + type/lint/test-green; process-kill
|
||
worth a live confirm). **Resolved by prior work: UX11** (M4 persists the queue, so a scheduled item fires on
|
||
next launch once due — no longer "never fires if quit"), **UX20** (L68 resets `notifiedBackground` on window
|
||
show, so every close-to-tray re-fires the "still running" notice, not just the first). **Deferred to the
|
||
watched live pass with rationale — each needs a real download to verify:** **R4** (delete `.part` on cancel —
|
||
file-deletion data-safety), **L65** (graceful Ctrl-C pause vs `taskkill /F`), **L139** (spawn↔auto-update
|
||
interlock — timing race), **B2** (index `AbortSignal` + Cancel), **B6** (app-update cancel — ties to CL4's
|
||
deferred updater), **L143** (in-window quit-anyway). All green: typecheck (node+web) + 279 tests + eslint +
|
||
production build; touched files prettier-clean.
|
||
|
||
---
|
||
|
||
## Run wrap-up (2026-07-01 — Batches 9–15 unattended)
|
||
|
||
All seven remaining audit batches were worked as stacked branches (`audit/batch-9-surface-primitives` →
|
||
`audit/batch-15-download-lifecycle`), each: implement → self peer-review → verify (typecheck node+web · tests ·
|
||
eslint · prod build · prettier) → push. The contained, safe, testable items landed; three categories were
|
||
**honestly deferred with documented rationale** rather than forced unattended: (1) large architectural
|
||
refactors the release gate already puts post-1.0 (CC9 zod, CC10/CC12/CC14 persistence/reorg, CL4 updater),
|
||
(2) subjective-visual items needing an eyeball (UI3/UI12/UI16/UX7/UX18/UI21/UX26 …), and (3) the download-engine
|
||
lifecycle changes that require a live download to verify (Batch 15's R4/L65/L139/B2/B6/L143). **Two items need
|
||
maintainer awareness:** SR4 flips `clipboardWatch` off-by-default for new installs (reversible), and Batch 15's
|
||
deferred set + L157's cancel-on-remove want a watched live smoke-test before merge.
|
||
|
||
**Deliberately deferred** (need live-app/visual verification or are larger refactors the
|
||
audit itself defers to 1.x): the wire-or-cut features (M5/M6/UX1), the god-file/store
|
||
refactors (C1, C2, H1), the SIMP* helpers + shared-UI-token work, the CC* consolidations,
|
||
PERF8 code-splitting, and the visual UI/UX polish long-tail. These are best done as their
|
||
own PRs with the app running so the UI can actually be checked.
|
||
|
||
---
|
||
|
||
# 1.0 Release-Readiness Audit (lead-engineer synthesis)
|
||
|
||
*A pre-release synthesis of the ~385 catalogued findings below — root causes, a release gate, and the
|
||
consequential issues per dimension (severity · reasoning · fix · benefit). The detailed, ID'd backlog
|
||
follows. No rewrite required; everything here is incremental.*
|
||
|
||
## Verdict
|
||
|
||
**Not 1.0-ready as-is, but close — roughly 3–4 focused PRs away.** The foundation is genuinely strong:
|
||
disciplined security model (context isolation, sandbox, argv-injection defense, host-pinned signed-checksum
|
||
updater), a clean pure/impure architecture, and good unit coverage of the pure logic. What blocks a
|
||
*commercial* 1.0 is a thin layer of (a) real reliability bugs, (b) naive persistence, (c) built-but-unwired
|
||
features, (d) first-run/perceived-quality gaps, and (e) the absence of enforcement/observability tooling.
|
||
None require rearchitecting.
|
||
|
||
## Root-cause themes (the 385 findings collapse to 8)
|
||
|
||
1. **Built-but-unwired features** — command preview (M5), incognito (M6), per-download options/extraArgs
|
||
(UX1) are plumbed end-to-end with no UI; the ROADMAP even marks them ✅ (M25). *Decide wire-or-cut.*
|
||
2. **"Two ways to do X" with no enforcement** — persistence (M1), validation (CC9), async (CC5), errors
|
||
(CC6), UI primitives (UI14/UI18/UI19), formatters (M9) — and no ESLint/Prettier to hold any line (CC2).
|
||
3. **Renderer-optimistic state never reconciles + a store cycle** — M34/L142 (settings show unsaved
|
||
values), C2 (downloads↔sources circular import).
|
||
4. **Naive JSON persistence** — non-atomic writes (R1), corrupt→silent total data loss (R2), O(n²)
|
||
full-file rewrites per completion (R3/PERF7), no cache.
|
||
5. **Download-engine reliability gaps** — no stall timeout (B1), reverse-order batches (M32), orphan
|
||
`.part` on cancel (R4), retry-during-teardown race (L140).
|
||
6. **First-run / perceived quality** — light-default theme (SR1), nightly-default yt-dlp (SR2), placeholder
|
||
icon (W14), stuck "Resolving…" (SR6), progress bar visibly restarting (SR7), unsigned build (SIGNING.md).
|
||
7. **Accessibility & Windows-native feel** — focus rings (UI28/29), no list keyboard nav (W7), title-bar
|
||
theme (W3), text-field context menu (W4), no `aria-live` (W17), input labels (M28).
|
||
8. **No enforcement / observability** — no lint (CC2), `noImplicitAny:false` (M38), no logging (CC8/M29),
|
||
no production source maps (L170).
|
||
|
||
## Release gate
|
||
|
||
**MUST fix before 1.0 (blockers)** — correctness, data safety, security, trust:
|
||
B1 · M32 · M35 · M34 · R1 · R2 · R3 · H7 · H8 · SR1 · SR6 · SR7 · W14 — **all resolved as of this
|
||
session.** Code signing was dropped from the gate: it's a deliberate non-goal (no certificate will be
|
||
purchased; the SmartScreen prompt on the unsigned build is accepted).
|
||
|
||
**SHOULD fix for 1.0 (high):** wire-or-cut the dead features (M5/M6/UX1) · a11y cluster (UI28/29, W4, W7,
|
||
W17, M28) · W3 native theming · lint + `noImplicitAny` (CC2/M38) · dev-jargon copy (M37/SR9) · destructive
|
||
confirmations (UX4) · PERF1/PERF2 (per-download redundant work).
|
||
|
||
**DEFER to 1.x:** the ~150 Low items · the simplification refactors (SIMP*, do incrementally) · the UI
|
||
token system + shared primitives (UI/SIMP — high value, larger effort) · i18n · sqlite migration.
|
||
|
||
## By dimension (severity · reasoning · fix · benefit)
|
||
|
||
### Reliability
|
||
- **[Critical] No download stall timeout (B1).** *Reasoning:* `spawn` has no timeout + no `--socket-timeout`;
|
||
a dead connection hangs forever and permanently consumes a concurrency slot. *Fix:* `--socket-timeout` +
|
||
an idle watchdog that kills+errors via the existing `killTree`. *Benefit:* downloads self-recover on flaky networks.
|
||
- **[Critical] Non-atomic writes + silent corruption→data-loss (R1/R2).** *Reasoning:* `writeFileSync`
|
||
isn't atomic; a crash mid-write corrupts the store and the next read returns `[]`, silently wiping history/
|
||
sources. *Fix:* atomic write (temp+rename) + back up a corrupt file before resetting, in one `jsonStore`.
|
||
*Benefit:* user data survives crashes; no silent loss.
|
||
- **[High] Reverse-order batch downloads (M32)** & **history duplicates (M35).** *Reasoning:* `addMany`+`pump`
|
||
promote highest-index first; `redownload` mints a new id defeating dedup. *Fix:* reverse the batch enqueue;
|
||
dedup history by URL. *Benefit:* playlists download 1→N; clean history.
|
||
- **[High] Optimistic state never reconciles (M34).** *Reasoning:* renderer keeps a value main rejected.
|
||
*Fix:* apply the validated `Settings` the IPC returns. *Benefit:* the UI never lies about what's saved.
|
||
|
||
### Performance
|
||
- **[High] O(n²) media-items rewrite (R3/PERF7).** *Reasoning:* every completion re-reads+rewrites the
|
||
whole (≤20k-item) file synchronously. *Fix:* in-memory cache + batched atomic writes (the same `jsonStore`).
|
||
*Benefit:* channel downloads stop hitching the main process.
|
||
- **[Medium] Per-download redundant work (PERF1/PERF2).** *Reasoning:* `templates.json` read + settings
|
||
decrypt on every spawn even when unused. *Fix:* gate `listTemplates()` on the consent flag; cache decrypted
|
||
settings. *Benefit:* lower latency/IO per download.
|
||
- **[Low] `summarizeQueue`/`pump` O(n) per tick (PERF3/PERF4).** *Fix:* memoize + count-based pump. *Benefit:* scales to large queues.
|
||
|
||
### Maintainability
|
||
- **[High] Triplicated Settings/`Api` + the preview mock (C1).** *Reasoning:* a 4th touch-point per IPC
|
||
method; drift-prone. *Fix:* `DEFAULT_SETTINGS` in `@shared`; one typed `mockApi`. *Benefit:* one place to change.
|
||
- **[High] Store circular dependency (C2).** *Fix:* a coordinator/event bus. *Benefit:* removes a latent init crash.
|
||
- **[Medium] Duplicated persistence/validation/async/formatters (M1/CC9/CC5/M9).** *Fix:* the `lib/` helpers
|
||
in SIMP1–SIMP5. *Benefit:* ~−500 LOC and fewer divergence bugs.
|
||
- **[Medium] God files (H1).** *Fix:* SettingsView via `<SettingsCard>`/`<ToggleField>` (SIMP10). *Benefit:* 1104→~600 lines, reviewable.
|
||
|
||
### Consistency
|
||
- **[Medium] Two ways to do X** across persistence, status chips, segmented controls, buttons, errors
|
||
(M1/UI18/UI14/UI15/CC6). *Fix:* one shared primitive/standard each (see "Recommended single style"). *Benefit:* the app reads as one product.
|
||
- **[Low] Naming/copy drift** (CC1, L95/L150, separators L164). *Fix:* the conventions in CC1. *Benefit:* professional finish.
|
||
|
||
### Readability
|
||
- **[Medium] Magic strings/numbers + long signatures + deep nesting (CL1/CL2/CL4, L10).** *Fix:* `constants.ts`,
|
||
options-objects, extracted helpers. *Benefit:* faster comprehension, fewer transpose bugs (CL2).
|
||
|
||
### Polish (perceived quality)
|
||
- **[High] First-run defaults (SR1/SR2/SR3).** *Reasoning:* light-on-dark first launch, nightly yt-dlp by
|
||
default, auto-download-new on. *Fix:* `theme:'system'`, `ytdlpChannel:'stable'`, `autoDownloadNew:false`.
|
||
*Benefit:* the first 10 seconds feel native and safe.
|
||
- **[High] Stuck "Resolving…" (SR6) & restarting progress bar (SR7).** *Fix:* clear placeholder on error;
|
||
weight the two merge phases. *Benefit:* nothing looks hung/glitchy.
|
||
- **[High] Placeholder icon (W14).** *Fixed:* a designed mark (teal gradient square + top sheen, bold
|
||
rounded download glyph over a landing shelf) replaces the flat placeholder; multi-size `.ico` regenerated.
|
||
*Benefit:* brand credibility. *(Unsigned build is a non-goal — no cert; the SmartScreen prompt is accepted.)*
|
||
|
||
### User experience
|
||
- **[High] Per-download options are unreachable (UX1).** *Reasoning:* must change global settings to tweak
|
||
one download. *Fix:* wire the existing `DownloadOptionsForm` into the bar (plumbing exists). *Benefit:* reclaims a whole feature.
|
||
- **[High] Destructive actions have no confirmation/undo (UX4).** *Fix:* confirm Clear/Remove/Delete. *Benefit:* prevents data loss.
|
||
- **[Medium] Silent failures on Open/Show (UX6)** and **no global download status off the Downloads tab (UX9).** *Fix:* surface errors; sidebar badge. *Benefit:* the app feels responsive and honest.
|
||
|
||
### Accessibility & Windows-native (UX-critical for a Windows product)
|
||
- **[High] Fragmented/invisible focus + no list keyboard nav (UI28/29, W7).** *Fix:* one focus-ring + roving
|
||
list focus + Delete/Ctrl+A. *Benefit:* keyboard- and Narrator-usable.
|
||
- **[Medium] Title bar ignores in-app theme (W3); text fields have no context menu (W4); no `aria-live` (W17);
|
||
inputs lack accessible names (M28).** *Fix:* sync `themeSource`; add an editing context menu; live regions;
|
||
`aria-label`s. *Benefit:* feels like a native, accessible Windows app.
|
||
|
||
### Developer experience
|
||
- **[High] No enforcement tooling (CC2) + `noImplicitAny:false` (M38).** *Reasoning:* style/type safety rely
|
||
on discipline; implicit `any` is allowed. *Fix:* Prettier + typescript-eslint + CI; re-enable `noImplicitAny`.
|
||
*Benefit:* the conventions stay true; regressions caught pre-merge.
|
||
- **[Medium] No logging + no prod source maps (CC8/M29/L170).** *Fix:* one leveled file logger at every catch;
|
||
hidden source maps. *Benefit:* field crashes become diagnosable instead of invisible.
|
||
- **[Low] Stale roadmap/docs (M25/M26, L80–L82); release checksum is manual (H8).** *Fix:* reconcile docs;
|
||
generate `.sha256` in `build:win`. *Benefit:* trustworthy docs; updates don't silently fail to install.
|
||
|
||
## Suggested PR sequence to 1.0
|
||
|
||
1. **Correctness & data safety:** B1, M32, M35, M34, and the cached/atomic `jsonStore` (R1/R2/R3/PERF7).
|
||
2. **Security & trust:** H7 (encrypt cookies), H8 (auto-generate checksums). *(Code signing is a non-goal — no cert.)*
|
||
3. **First-run polish:** SR1/SR2/SR3 defaults, SR6/SR7 status, W14 icon, W3 title-bar theme.
|
||
4. **Wire-or-cut + a11y + tooling:** UX1 (per-download panel) or remove M5/M6; focus/keyboard (UI28/29, W7,
|
||
W4); Prettier+ESLint+`noImplicitAny` (CC2/M38).
|
||
5. **Incremental cleanup (post-1.0):** the SIMP refactors and the Low/UI/UX long tail, behind the new lint gate.
|
||
|
||
---
|
||
|
||
## Critical
|
||
|
||
- [x] **C1 — Single source of truth for the IPC mock + Settings defaults.** `const PREVIEW`
|
||
redeclared in 8 files; `main.tsx` reimplements the entire `Api` (~180 lines); the full
|
||
`Settings` object is hand-maintained in 3 places (`main/settings.ts` `DEFAULTS`,
|
||
`renderer/store/settings.ts` `FALLBACK`, `main.tsx` `MOCK_SETTINGS`). Add `DEFAULT_SETTINGS`
|
||
to `shared/ipc.ts`, extract the preview mock into one `mockApi.ts` typed as `Api`, centralize
|
||
`isPreview`. *Fixed: `DEFAULT_SETTINGS` is now the canonical default in
|
||
[shared/ipc.ts](src/shared/ipc.ts); main's `DEFAULTS`, the renderer `FALLBACK`
|
||
([store/settings.ts](src/renderer/src/store/settings.ts)), and the preview `MOCK_SETTINGS`
|
||
all derive from it (preview-only field tweaks layered on top) instead of three hand-kept
|
||
copies. The whole browser mock moved into one [mockApi.ts](src/renderer/src/mockApi.ts) typed
|
||
`Window['api']` (so a missing/mistyped method is a compile error), and `main.tsx` shrank to
|
||
`window.api = mockApi`. The `PREVIEW` check is single-sourced in
|
||
[isPreview.ts](src/renderer/src/isPreview.ts) and imported (as `PREVIEW`) by all 8 sites.*
|
||
- [x] **C2 — Break the `downloads ↔ sources` store circular dependency.** `downloads.ts`
|
||
imports `useSources`; `sources.ts` imports `useDownloads`. Works only via lazy `.getState()`.
|
||
Introduce a coordinator/event bus that owns cross-store reactions. *Fixed: a tiny typed event
|
||
bus [store/coordinator.ts](src/renderer/src/store/coordinator.ts) (no runtime deps — the
|
||
`AddEntry` import is type-only) now owns the two cross-store reactions. `sources` emits
|
||
`enqueueDownloads` (consumed by `downloads.addMany`); `downloads` emits `downloadCompleted`
|
||
(consumed by `sources.markDownloaded`). Each store imports the bus, not the other, so the
|
||
cycle is gone. Because `downloads` no longer pulls in `sources`, `App` now eagerly imports the
|
||
`sources` store for side-effects (`import './store/sources'`) so its startup load + scheduled-
|
||
`--sync` kickoff and the `downloadCompleted` subscription still run at launch — robust even
|
||
once the views are lazy-loaded (PERF8).*
|
||
|
||
## High
|
||
|
||
- [x] **H1 — Decompose god files.** `SettingsView.tsx` (1104, ~11 cards, ~30 selector subs +
|
||
~20 useState), `DownloadBar.tsx` (858, ~17 state hooks), `store/downloads.ts` (614). *Done on
|
||
`refactor/h1-decompose-god-files`: SettingsView → `components/settings/*.tsx` (one component per
|
||
card + shared `settingsStyles.ts`), leaving a ~90-line shell (search box + display-toggle filter);
|
||
each card still renders one `<Card>` DOM node so the filter is unchanged. DownloadBar → a
|
||
`downloadBar/useDownloadBar.ts` hook (all state + handlers, wrapper handlers keep the
|
||
preview-invalidation side-effects) + `downloadBar/styles.ts` + `downloadBar/PlaylistPanel.tsx`,
|
||
leaving a render-only shell. store/downloads.ts → `downloadTypes.ts` (types, re-exported so
|
||
importers are untouched) + `downloadItem.ts` (buildItem/titleFromUrl/RESOLVING) + `downloadSeed.ts`
|
||
(preview seed), leaving the store + recordCompletion + wiring. Behaviour unchanged; full
|
||
typecheck + lint + production build + 248 tests green.*
|
||
- [x] **H2 — Consolidate scattered utilities.** `youtubeId` ×2; 4 YouTube-URL-parsing variants;
|
||
byte/speed/eta/duration formatting across 4 modules. *URL half: one canonical `youtubeId` now lives in
|
||
[urlHelpers.ts](src/renderer/src/lib/urlHelpers.ts) (the complete watch/shorts/embed/live/youtu.be
|
||
parse); `thumb.ts`, `queueStats.ts` (`sameVideo`), and `store/downloads.ts` (`titleFromUrl`, which also
|
||
fixes an old bug where any host with a `?v=` param was mislabeled "YouTube video") all import it — the
|
||
three ad-hoc reimplementations are gone. Formatter half (done with H4): `fmtBytes`/`fmtSpeed`/`fmtEta`
|
||
now have a single home in [@shared/format.ts](src/shared/format.ts) imported by both sides; the main
|
||
formatters module re-exports them and the renderer's three private copies (a byte formatter, an ETA
|
||
formatter, and a speed string→number re-parser in `queueStats`) are deleted. Per-item and aggregate
|
||
speed now use the same 1024-based scale (the old aggregate used 1000, a latent inconsistency).
|
||
Unit-tested in `test/clipboardLink.test.ts` + `test/download.test.ts` + `test/queueStats.test.ts`.*
|
||
- [x] **H3 — Fix `probe.ts → download.ts` dependency direction** (`fmtBytes` import); resolves
|
||
with H2's shared formatter.
|
||
- [x] **H4 — Carry raw numbers across the progress boundary.** `DownloadProgress` ships
|
||
formatted `speed`/`eta` strings; `queueStats.ts` re-parses them back to numbers. *Fixed: `DownloadProgress`
|
||
now carries `speedBytesPerSec`/`etaSeconds` as raw numbers (main's `parseProgress` no longer formats them);
|
||
the renderer stores the raw numbers on `DownloadItem`, `QueueItem` formats them for display via
|
||
[@shared/format](src/shared/format.ts), and `summarizeQueue` sums/maxes the numbers directly — the lossy
|
||
string→number→string round-trip (and `queueStats`' `parseSpeed`/`parseEtaSeconds`) is gone. Done together
|
||
with the formatter half of H2.*
|
||
- [x] **H5 — History re-download silently changes quality.** `HistoryView.redownload` passes the
|
||
stored `quality` (for format-picker downloads this is a *label* like "720p · mp4 · 184 MB")
|
||
back as `quality` with no `formatId`; `buildArgs.videoFormat()` has no matching case and falls
|
||
back to `bv*+ba/b` (Best). The queued item shows "720p…" while actually fetching Best.
|
||
- [x] **H6 — TerminalView log grows unbounded.** `setLines((ls) => [...ls, …])` with no cap; a
|
||
verbose run (`-F`, `--verbose`) streams thousands of lines into React state. Every other
|
||
log/list in the app is capped — cap this too (and/or virtualize).
|
||
- [x] **H7 — `cookies.txt` written in plaintext with no restrictive perms.** [cookies.ts](src/main/cookies.ts)
|
||
`writeFileSync(getCookiesFilePath(), …)` stores live auth/session cookies unencrypted under
|
||
`userData`. In the **portable build** that's `AeroFetch-data/` next to the exe (USB stick /
|
||
shared Downloads folder), so anyone with folder access can read a logged-in session. Settings
|
||
secrets are DPAPI-encrypted (settings.ts) but cookies are not. Encrypt at rest or document the
|
||
exposure for the portable/shared-PC scenario the app explicitly targets.
|
||
- [x] **H8 — Release checksum is a manual step the updater hard-requires.** [updater.ts](src/main/updater.ts)
|
||
sets `REQUIRE_CHECKSUM = true` and refuses any update lacking a `<asset>.sha256`, but
|
||
`build:win` (`electron-vite build && electron-builder --win`) never generates one. Evidence in
|
||
`dist/`: only `0.4.1` has `.sha256` files (hand-made); the current `0.5.0` build does not. If a
|
||
release is published without manually adding the checksum, **every client's in-app update
|
||
fails** ("This release has no checksum … refusing to install"). Generate the `.sha256` in the
|
||
build/release script.
|
||
|
||
## Medium
|
||
|
||
- [x] **M1 — Unify JSON persistence.** `sources.ts` has generic `readJsonArray`/`writeJson`;
|
||
`history.ts`/`errorlog.ts`/`templates.ts` reimplement it inline.
|
||
- [x] **M2 — Remove dead code: `getDefaultFolder` / `download:default-folder`** (channel +
|
||
preload + handler + mock, zero callers).
|
||
- [x] **M3 — Remove duplicated completion side-effect in `store/downloads.ts`** (history write +
|
||
`markDownloaded` in both `applyEvent('done')` and the preview ticker). Subsumed by C2.
|
||
- [x] **M4 — Decide queue persistence explicitly.** Scheduler/queue is renderer-memory only;
|
||
`saved`/scheduled items don't survive a quit. Persist or document as a non-goal in code. *Fixed
|
||
(persist, per the run decision): a main [queue.ts](src/main/queue.ts) store (the proven cached-atomic
|
||
`createJsonStore`, `queue.json`) plus `queue:list`/`queue:save` IPC. The renderer mirrors its durable
|
||
items on every queue change (a `PersistedQueueItem` = the stable subset of `DownloadItem`; runtime-only
|
||
progress/speed/eta are dropped, and a signature over the persistable subset means progress ticks don't
|
||
re-save) and rehydrates on launch via an `App` `useEffect`. On restore a `downloading` item (its process
|
||
died) normalizes back to `queued` (yt-dlp continues the `.part`), `saved`+`scheduledFor` survives so the
|
||
promoter fires it when due, `paused`/`error` return actionable; `completed`/`canceled` are never persisted.
|
||
A `queueHydrated` gate stops a launch-time store change wiping the file before it's read. Pure mappers
|
||
extracted to [queuePersist.ts](src/renderer/src/store/queuePersist.ts) and unit-tested (`test/queuePersist.test.ts`);
|
||
plumbing is typecheck-verified. **Live-verify note:** the OS-level quit→relaunch cycle itself wasn't
|
||
smoke-tested here — worth a manual confirm (schedule/park an item, quit, relaunch).*
|
||
- [x] **M5 — Command-preview feature is fully built but unwired.** `CommandPreviewResult` type +
|
||
`command:preview` channel + `previewCommand` (download.ts) + `formatCommandLine`/
|
||
`quoteForDisplay` (buildArgs) + preload method all exist, but no renderer component calls
|
||
`window.api.previewCommand` (only the `main.tsx` mock references it). Wire it into the
|
||
DownloadBar Options panel, or remove the dead chain. *Fixed: a "Show command / Hide command"
|
||
toggle in [DownloadBar.tsx](src/renderer/src/components/DownloadBar.tsx) calls
|
||
`window.api.previewCommand` and renders the exact yt-dlp argv (monospace). The preview mirrors
|
||
what `download()` spawns — probe-selected `formatId`/`formatHasAudio`, `quality`, per-download
|
||
`options`, and `trim` — and is invalidated whenever any of those inputs (or the URL / a re-probe)
|
||
changes, so the shown command never goes stale. Wired with M6/UX1 in the same panel.*
|
||
- [x] **M6 — Private/incognito mode is plumbed but unreachable.** `incognito` flows through
|
||
`AddOptions` → `buildItem` → history-skip → the QueueItem "Private" badge, but nothing in the
|
||
UI ever sets `incognito: true`. Add the toggle or remove the dead plumbing. *Fixed: an
|
||
"Incognito mode (no logging, no history, no cookies)" checkbox in the DownloadBar Advanced panel
|
||
sets `incognito` on the enqueued item (via `AddOptions`), lighting up the existing QueueItem
|
||
"Private" badge. Reset to off after each download so it never silently carries over to the next.*
|
||
- [x] **M7 — `newId()` duplicated 3×** (`store/downloads.ts`, `TerminalView.tsx`,
|
||
`TemplateManager.tsx`) with divergent fallback prefixes (`item-`/`t-`/`tpl-`). Extract one helper.
|
||
*Fixed: one [`newId(prefix)`](src/renderer/src/id.ts) helper; the three copies import it and pass their
|
||
prefix. The fallback gained a monotonic counter so same-millisecond ids can't collide (subsumes L33/L70).
|
||
Unit-tested in `test/id.test.ts`.*
|
||
- [x] **M8 — Two download-status→label maps.** `STATUS_BADGE` (QueueItem) and `STATUS_LABEL`
|
||
(LibraryView) independently map the same statuses (completed = "Completed" vs "Downloaded").
|
||
One shared map.
|
||
- [x] **M9 — Three time formatters.** `relTime` (LibraryView), `formatWhen` (HistoryView),
|
||
`fmtSchedule` (QueueItem) — consolidate into a date util. *Fixed: all three moved to
|
||
[`datetime.ts`](src/renderer/src/datetime.ts) and imported by their views; unit-tested in `test/datetime.test.ts`.*
|
||
- [x] **M10 — `.url` shortcut parsing duplicated** — `parseUrlFile` (DownloadBar) vs
|
||
`readUrlShortcut` (main/deeplink). Different `URL=` extractors for the same file format.
|
||
- [x] **M11 — Inconsistent clipboard access.** Reads use `navigator.clipboard.readText` (paste
|
||
button) *and* `window.api.readClipboard` (suggestion watcher); writes use
|
||
`navigator.clipboard.writeText` (copy report). Pick one strategy.
|
||
- [x] **M12 — Shared `errorText` style.** `tokens.colorPaletteRedForeground1` is applied inline
|
||
12× across 5 files instead of one class. *Partially fixed: added
|
||
[`useErrorTextStyles`](src/renderer/src/components/ui/errorText.ts) (`error` / `errorPre` for pre-wrap
|
||
multi-line output) and adopted it in `SettingsView.tsx` — the biggest offender — replacing all 6 inline
|
||
`style={{ color: … }}` error spans + its local `errorRowText` class. The remaining red references live in
|
||
per-component `makeStyles` classes (DownloadBar/LibraryView/QueueItem/TerminalView), not inline; migrate
|
||
those to the shared hook incrementally.*
|
||
- [x] **M13 — Inconsistent secret-field masking.** `updateToken` uses `type="password"`; `proxy`
|
||
(may carry `user:pass@`) and `youtubePoToken` (a token) are plain-text Inputs.
|
||
- [x] **M14 — Settings search mutates React-owned DOM.** The search toggles each card's
|
||
`el.style.display` directly; fragile (breaks if a card ever gets a conditional `style`) and
|
||
matches on `textContent` incl. hidden text. Prefer state-driven filtering. *Fixed: each card renders
|
||
inside a React-owned wrapper whose visibility is driven by a `hidden` state array in
|
||
[SettingsView.tsx](src/renderer/src/components/SettingsView.tsx) — the filter no longer writes
|
||
`display` onto a card's own node, so a card setting its own inline style can't break it. Matching still
|
||
reads the wrapper's `textContent` (so search covers every visible label without hand-maintained
|
||
per-card keywords); the minor "matches hidden descendant text" point is unchanged and left as-is.*
|
||
- [x] **M15 — Nested interactive controls in `role="button"` (a11y).** LibraryView's group header
|
||
is a `role="button"` div containing `<Button>`s (All / Download) — invalid ARIA / keyboard
|
||
semantics. Make the header a real element with sibling buttons.
|
||
- [x] **M16 — No renderer error boundary.** An exception in any view unmounts the whole shell to
|
||
a blank window. Add a top-level boundary with a recover/reload affordance. *Fixed: added
|
||
[`ErrorBoundary`](src/renderer/src/components/ErrorBoundary.tsx) wrapping `<App/>` in `main.tsx`. The
|
||
fallback is dependency-free (no Fluent/theme provider — the crash may have come from that tree), logs via
|
||
`componentDidCatch`, shows the error message, and offers a Reload button (`window.location.reload()`;
|
||
persisted data lives in main, so nothing is lost).*
|
||
- [x] **M17 — `statusByUrl` collapses duplicate URLs.** LibraryView maps URL→status into one
|
||
`Map`; with "Download anyway" duplicates, a row can reflect the wrong item's state.
|
||
- [x] **M18 — Audio "quality" presets hard-code MP3.** `QUALITY_OPTIONS.audio` labels ("Best
|
||
(MP3)", "320 kbps") are shown even when `downloadOptions.audioFormat` is opus/flac/wav, so the
|
||
label contradicts the actual output format; `--audio-quality` is also meaningless for lossless.
|
||
- [x] **M19 — Sidebar-collapsed pref uses `localStorage`** while every other preference uses
|
||
electron-store — won't back up/restore and is lost in the portable build across machines.
|
||
- [x] **M20 — ProgressBar / native `<select>` accessibility.** QueueItem/DownloadsView
|
||
`ProgressBar`s have no accessible name; several `Select`s carry both a `<Field label>` and an
|
||
`aria-label` (double announcement).
|
||
- [x] **M21 — `--audio-quality` always emitted.** `buildArgs` passes `--audio-quality` even for
|
||
lossless `audioFormat` (flac/wav), where it's meaningless; pair with M18's preset/format mismatch.
|
||
- [x] **M22 — Backup export writes secrets in cleartext, silently.** [backup.ts](src/main/backup.ts)
|
||
`exportBackup` serializes the *decrypted* settings (proxy creds, PO token, update token) to a
|
||
plain JSON file; the UI caption only says "Does not include download history" — no warning that
|
||
the file contains credentials. *Fixed: `proxy`/`youtubePoToken`/`updateToken` are stripped (set to
|
||
`''`) before writing; SettingsView caption updated to say credentials are not included.*
|
||
- [x] **M23 — `MAX_ITEMS` truncation can silently drop a source's items.** [sources.ts](src/main/sources.ts)
|
||
`replaceMediaItems` does `[...new, ...others].slice(0, 20000)`; once the global total exceeds the
|
||
cap, the *tail* (another source's items) is dropped on write and only reappears when that source
|
||
is re-indexed. Cap per-source, or surface it.
|
||
- [x] **M24 — `setSettings` accepts unvalidated free strings.** [settings.ts](src/main/settings.ts)
|
||
stores `rateLimit`/`proxy`/`defaultVideoQuality`/`defaultAudioQuality`/`youtubePlayerClient` as
|
||
any string. A bad `rateLimit` ("abc") only fails at download time; a `defaultQuality` not in
|
||
`QUALITY_OPTIONS` leaves the Settings dropdown's controlled value blank. *Fixed: `VIDEO_QUALITY_OPTIONS`
|
||
/ `AUDIO_QUALITY_OPTIONS` added to `shared/ipc.ts` and imported in both `settings.ts` (allowlist
|
||
validation) and `store/downloads.ts` (single source of truth). `rateLimit` validated against yt-dlp
|
||
rate format regex; `youtubePlayerClient` trimmed.*
|
||
- [x] **M25 — ROADMAP marks unwired features as ✅ shipped.** [ROADMAP.md](ROADMAP.md) Phase C
|
||
("Command preview … Surfaced as a **Preview command** button in the download bar") and Phase D
|
||
("Private / incognito mode — a download bar toggle") both carry `[x]`, but neither is wired in
|
||
the UI (corroborates M5/M6). The roadmap overstates completion — reconcile the docs or finish the wiring.
|
||
- [x] **M26 — ROADMAP accent palette is wholesale stale.** [ROADMAP.md](ROADMAP.md) Phase E
|
||
describes "four accent presets — Toffee (original), Slate, Evergreen, Lavender" with a default of
|
||
Toffee; the shipped [theme.ts](src/renderer/src/theme.ts) is rose / coral / amber / teal
|
||
("Sunset-to-sea") with a default of **teal**. The whole section documents a palette that no
|
||
longer exists.
|
||
- [x] **M27 — Library batch-enqueue ignores per-item kind.** [sources.ts](src/renderer/src/store/sources.ts)
|
||
`enqueueItems` forces `settings.defaultKind` (+ its quality) for *every* item, so a channel can't
|
||
be downloaded as audio without flipping the global default — inconsistent with the DownloadBar
|
||
playlist panel, which offers a per-item video/audio toggle.
|
||
- [x] **M28 — Primary text inputs have no accessible name.** The URL field (DownloadBar), add-source
|
||
(Library), search (History/Settings), and the Terminal args `Textarea` rely on `placeholder` only —
|
||
which is not an accessible name. Add `aria-label`/`<label>` to each. *(a11y; distinct from M20's Select/ProgressBar.)*
|
||
- [x] **M29 — Failures are swallowed everywhere.** 29 `.catch(() => {})` sites across 15 files: nearly
|
||
every IPC write/read discards its error with no log, no user feedback, and no telemetry. A failed
|
||
settings write, history add, or `openPath` simply vanishes. *Fixed: a shared
|
||
[`logError(op)`](src/renderer/src/reportError.ts) helper replaces every swallowed renderer catch with
|
||
one consistent `console.error('[AeroFetch] <op> failed:', e)` (settings, history, templates, errorlog,
|
||
sources, App, DownloadBar drag-drop, SettingsView version reads, LibraryView scheduled-sync); main-process
|
||
sites (ytdlp auto-update, cookies loadURL) log inline. Four genuinely-silent catches left intentionally:
|
||
`pauseDownload` / `cancelDownload` (process kill, state already applied), `unlink` (cleanup), and
|
||
`clipboard.writeText` (browser permission — no user action available). **Scope note:** this delivers
|
||
the "log" half — failures are no longer silent. The user-facing toast + a persistent log sink (renderer
|
||
`console.error` isn't captured in a packaged build) are deferred to **CC8** (electron-log), which the
|
||
audit already cross-references here.*
|
||
- [x] **M30 — A broken contextBridge degrades silently to mock mode.** [preload/index.ts](src/preload/index.ts)
|
||
only `console.error`s if `exposeInMainWorld` throws; the renderer then sees no `window.electron`,
|
||
flips `PREVIEW` true, and runs the **browser mock** (no real IPC) with no visible error. A real
|
||
bridge failure looks like "preview." *Fixed: preload sends `IpcChannels.preloadBridgeFailure` on failure;
|
||
main registers `ipcMain.once(IpcChannels.preloadBridgeFailure, …)` in `registerIpcHandlers()` (run before
|
||
the window is created) and shows an error dialog + quits, so the failure is impossible to miss.*
|
||
- [x] **M31 — Default Electron menu (incl. Toggle DevTools/Reload) ships in production.** No
|
||
`Menu.setApplicationMenu(null)` is called; with `autoHideMenuBar` the default role menu is still
|
||
Alt-accessible, exposing DevTools/reload to end users. *Fixed: `Menu.setApplicationMenu(null)` called
|
||
in `app.whenReady()` when `!is.dev`; dev builds keep the menu for DevTools access.*
|
||
- [x] **M32 — Playlist/channel batches download in reverse order.** `addMany` prepends the whole
|
||
batch in entry order (`[entry1…entryN, …old]`), but `pump()` promotes the *highest-index* queued
|
||
item first (it reverses, assuming one-at-a-time prepends). So a selected playlist/channel downloads
|
||
**N → 1**. Files are still named `001…NNN` correctly (by `playlistIndex`), so a partial run leaves
|
||
the *last* videos on disk — surprising. Enqueue batches in reverse, or have `pump()` respect insertion order.
|
||
- [x] **M33 — Documented enqueue batch cap doesn't exist.** [ipc.ts](src/shared/ipc.ts):629 and
|
||
ROADMAP-PINCHFLAT claim the queue pulls "a batch at a time (e.g. 50)" via `MAX_ENQUEUE_BATCH` so it
|
||
"never holds the whole collection at once," but no such cap exists — `enqueueItems` `addMany`s *every*
|
||
selected item, so "Download all pending" on a 5,000-video channel puts 5,000 items in the store/queue
|
||
at once. Implement the cap or fix the comment.
|
||
- [x] **M34 — Optimistic settings updates never reconcile with main's validation.** The settings store
|
||
does `set(partial)` then `setSettings(partial).catch(() => {})`, discarding the validated `Settings`
|
||
main returns. A value main rejects (e.g. an unsafe `filenameTemplate`, a clamped `maxConcurrent`, a
|
||
malformed `rateLimit`) still shows as accepted in the UI until restart — the user believes a setting
|
||
saved that didn't. Apply the returned authoritative state.
|
||
- [x] **M35 — History re-download creates duplicate rows.** `redownload` re-queues via `addFromUrl`,
|
||
which mints a **new id**; on completion `addHistory` dedups by id (no match) and prepends a second row
|
||
for the same video. Re-downloading from History accumulates duplicates. Dedup by URL, or reuse the entry id.
|
||
- [x] **M36 — Library checkbox count ≠ "Download N selected".** Every item row has a checkbox
|
||
([LibraryView.tsx](src/renderer/src/components/LibraryView.tsx)), but `selectedActionable` filters to
|
||
pending/error/canceled only, so selecting 5 rows (incl. 2 already-downloaded) shows "Download **3**
|
||
selected." The checkbox count and the button count silently disagree. **Fix:** only show checkboxes on
|
||
actionable rows, or count all selected.
|
||
- [x] **M37 — End-user hints leak developer/internal references.** Settings hints surface dev-facing
|
||
detail: "Requires aria2c.exe in resources/bin (see the README there)", "paste a read-only **Gitea**
|
||
token", "sent via `--extractor-args`", "breaking downloads with **403** errors", "open a locked cookie
|
||
database", and roadmap status ("automatic minting is **planned**"). These read as code comments, not
|
||
product copy. **Fix:** rewrite for end users (hide repo/flag/HTTP-status jargon and roadmap notes).
|
||
- [x] **M38 — `noImplicitAny: false` partially defeats `strict`.** The inherited
|
||
`@electron-toolkit/tsconfig` sets `strict: true` **but explicitly `"noImplicitAny": false"`**, and
|
||
neither project tsconfig re-enables it — so a parameter/variable with no inferrable type silently
|
||
becomes `any` project-wide (the one real hole in an otherwise-strict setup; `noUnusedLocals`/
|
||
`noUnusedParameters`/`noImplicitReturns` *are* on, which is why dead locals don't accumulate). **Fix:**
|
||
re-enable `noImplicitAny` in `tsconfig.node.json`/`tsconfig.web.json`.
|
||
|
||
## Low
|
||
|
||
- [x] **L1 — Module-load `setInterval` in `store/downloads.ts`** runs on import (incl. tests/preview).
|
||
*Fixed: the scheduled-download promoter tick moved out of store-module scope into an `App.tsx`
|
||
`useEffect` (interval `SCHEDULE_TICK_MS`, cleared on unmount). Importing the store in tests/preview no
|
||
longer starts a stray never-cleared timer; the store keeps the `promoteDueScheduled` action, App owns the
|
||
cadence. (The preview `startFakeTicker` interval is created on demand per launched item, not at load, so
|
||
it was already fine.)*
|
||
- [x] **L2 — `index.ts` flat IPC registration (~150 lines)** — let each main module export its own
|
||
`register(ipcMain)`. *Fixed: the whole ~200-line handler block moved into one dedicated
|
||
[ipc.ts](src/main/ipc.ts) exporting `registerIpcHandlers(getMainWindow)` — the single place the renderer's
|
||
IPC contract is wired. index.ts now imports it (plus the three shared theme helpers `resolveBackgroundMode`
|
||
/`applyNativeTheme`/`getSystemThemeInfo`, also moved to ipc.ts) and shed ~50 imports it only needed for the
|
||
handlers, leaving it as app-lifecycle + window creation. The window-coupled handlers (folder picker,
|
||
theme-synced background, taskbar progress) take the live window via the `getMainWindow` accessor rather
|
||
than a captured `mainWindow`.*
|
||
- [x] **L3 — Stale comments.** `getYtdlpVersion` JSDoc still calls it the "Step-1 spike";
|
||
`vitest.config.ts` claims tests "only exercise buildArgs.ts" (9 test files now exist).
|
||
- [x] **L4 — `electron-builder.yml` nits.** `copyright: yt-dlp frontend` is not a copyright
|
||
string (no holder/year); the `files` exclude `tsconfig.tsbuildinfo` never matches the real
|
||
`tsconfig.web.tsbuildinfo`.
|
||
- [x] **L5 — Inline `style={{}}` vs makeStyles** (25× across 8 files; SettingsView 14×). Notably
|
||
Sidebar's active-nav inset shadow and DownloadOptionsForm's hint caption use inline styles
|
||
where a class exists elsewhere. *Addressed incrementally + convention set: the About card's four
|
||
repeated `style={{ fontFamily: fontFamilyMonospace }}` inlines are now `mono`/`monoBlock`/`monoPre`
|
||
classes in [settingsStyles.ts](src/renderer/src/components/settings/settingsStyles.ts). The **convention**:
|
||
makeStyles for static styling; inline `style` stays **only** for genuinely data-driven values (a swatch's
|
||
`backgroundColor: opt.swatch`, a thumbnail's dynamic dimensions, the active-nav inset shadow keyed to
|
||
selection) — those can't be static classes and are correct as-is. Remaining static inlines migrate
|
||
opportunistically under this rule.*
|
||
- [x] **L6 — Control height mismatch.** `Select` is a fixed 32px sitting beside `size="large"`
|
||
(~40px) Input/Buttons in DownloadBar.
|
||
- [x] **L7 — `canceled` and `paused` share the 'warning' badge color** (QueueItem) — visually
|
||
ambiguous.
|
||
- [x] **L8 — Index/compound list keys.** TerminalView keys log lines by array index; Settings
|
||
Diagnostics keys by `id + occurredAt` while every other list keys by `id` alone.
|
||
- [x] **L9 — Thumbnail box sizes not shared.** Four hand-tuned 16:9 boxes (120×68, 108×64, 72×44,
|
||
60×34) with no shared aspect/size constant.
|
||
- [x] **L10 — Scattered magic numbers.** Timeouts/caps spread across modules (probe 60s, indexer
|
||
180s, probeMeta 30s, update-idle 60s; MAX_ENTRIES 500/200, MAX_TEMPLATES 100, MAX_ITEMS 20000).
|
||
Consider a central constants module. *Fixed: a main-process [constants.ts](src/main/constants.ts) now
|
||
holds the process-spawn timeouts (`VERSION_TIMEOUT_MS`, `META_PROBE_TIMEOUT_MS`, `PROBE_TIMEOUT_MS`,
|
||
`INDEX_TIMEOUT_MS`, `YTDLP_UPDATE_TIMEOUT_MS`, `FEED_FETCH_TIMEOUT_MS`, `STALL_TIMEOUT_MS`), the execFile
|
||
`maxBuffer` sizes (`META`/`PROBE`/`INDEX_MAX_BUFFER`), `STDERR_TAIL_BYTES`, and the three hand-rolled store
|
||
caps (`ERRORLOG_MAX_ENTRIES`, `TEMPLATES_MAX`, `MEDIA_ITEMS_MAX`). download/ffmpeg/indexer/probe/ytdlp/sync/
|
||
errorlog/templates/sources import from it instead of bare literals. Values already single-sourced in the
|
||
shared contract (`HISTORY_MAX_ENTRIES`) and updater.ts's already-named local timeouts were left in place —
|
||
they aren't scattered literals.*
|
||
- [x] **L11 — "Queue (N)" overcounts.** DownloadsView's header count is `items.length` (includes
|
||
completed/error/canceled), not the active queue.
|
||
- [x] **L12 — Command palette polish.** No scroll-into-view for keyboard selection in the 50vh
|
||
list; `role="dialog"` without `aria-modal`/focus-trap; Esc handled only on the input.
|
||
- [x] **L13 — Destructive actions lack confirmation.** "Clear history", "Clear log", "Remove
|
||
source" are one-click — inconsistent with the careful confirm on backup import.
|
||
- [x] **L14 — `base.css` is bare** — no global `box-sizing`, `:focus-visible`, or font-smoothing
|
||
baseline, so custom native elements (Select, Hint, segmented controls) get inconsistent focus
|
||
rings; `box-sizing` is then set ad hoc on the SettingsView swatch.
|
||
- [x] **L15 — `MediaThumb` redundant ternary** `kind === 'audio' ? 'audio' : 'video'` (kind is
|
||
already that union).
|
||
- [x] **L16 — `relTime`/`formatWhen` verbosity.** `relTime` returns unbounded "N d ago"; History's
|
||
`formatWhen` always appends the year, even for the current year.
|
||
- [x] **L17 — TemplateManager regex not validated.** An invalid `urlPattern` is accepted with no
|
||
feedback and silently never matches (main's `matchesUrl` swallows the error).
|
||
- [x] **L18 — Terminal nav item shown when custom commands are off** — leads to a gated dead-end
|
||
view; consider hiding/disabling it.
|
||
- [x] **L19 — `App.tsx` focus hack** — `setTimeout(() => …focus(), 60)` after a tab switch.
|
||
- [x] **L20 — `youtubePlayerClient` is free text** with no allowlist/validation; a typo passes
|
||
straight to yt-dlp's `--extractor-args`.
|
||
- [x] **L21 — Inconsistent in-component error handling.** SettingsView funnels the app-update
|
||
not-ok result into a dedicated `appUpdError` slot but keeps not-ok duals for
|
||
version/update/export/import — two patterns in one file. *Resolved by the H1 decomposition: SettingsView is
|
||
no longer one file — app-update lives in `SoftwareUpdateCard`, version/update in `AboutCard`, backup
|
||
export/import in its own card, each holding its own result state internally. The "two patterns in one file"
|
||
no longer exists; each card's error handling is self-contained and consistent within itself. (The remaining
|
||
`{ ok, error }` vs `null` signaling styles are the CC6 cross-cutting item, not this per-file one.)*
|
||
- [x] **L22 — DownloadOptionsForm shows both audio + video controls** regardless of the chosen
|
||
kind in the per-download override panel (audio format shown for a video download, etc.).
|
||
- [x] **L23 — Diagnostics shows first 20 errors** with no "showing 20 of N" hint (the copied
|
||
report includes all).
|
||
- [x] **L24 — `window.open(htmlUrl, '_blank')`** in SettingsView is the only `window.open` in the
|
||
app (relies on the window-open handler); elsewhere external opens go through IPC/`shell`.
|
||
- [x] **L25 — Taskbar effect over-fires.** `App.tsx` subscribes to the whole downloads store and
|
||
recomputes `summarizeQueue` + sends the taskbar IPC on *every* store change (each progress
|
||
tick); throttle or dedupe by computed value.
|
||
- [x] **L26 — Third URL-sniffing path.** DownloadBar's drag-drop `firstUrl` is a separate
|
||
"first http(s) line" extractor alongside `useClipboardLink.looksLikeUrl` and deeplink's scan.
|
||
- [x] **L27 — DownloadBar Enter probes, not downloads.** Enter in the URL field triggers
|
||
`fetchFormats`; there's no keyboard path to actually start the download (LibraryView's Enter
|
||
runs its primary action). Inconsistent.
|
||
- [x] **L28 — Default kind/quality applied once.** DownloadBar seeds from settings via a one-shot
|
||
ref; changing the default in Settings while on the Downloads tab isn't reflected until reload.
|
||
- [x] **L29 — UI constant in the store.** `QUALITY_OPTIONS` lives in `store/downloads.ts` yet is a
|
||
presentational constant imported by views — couples UI to the store module.
|
||
- [x] **L30 — Redundant `loading="lazy"`** on `MediaThumb` images already inside a virtualized
|
||
list (the virtualizer only mounts visible rows).
|
||
- [x] **L31 — Two theme switchers.** Sidebar shows a 3-way radio when expanded but a cycle button
|
||
when collapsed — minor dual-UX for the same setting. *Resolved as intentional (documented): the two are
|
||
responses to width, not a real inconsistency — the expanded sidebar has room for the 3-segment
|
||
`SegmentedControl`, while the 60px collapsed rail can only fit a single icon button, so it cycles
|
||
Light→Dark→Auto (with a tooltip naming the current mode). Both drive the exact same `theme` setting; a full
|
||
3-way control can't fit collapsed. Left as-is by design.*
|
||
- [x] **L32 — `paletteActions` rebuilt every render** in `App.tsx` (not memoized); harmless today,
|
||
but it's passed straight into a child.
|
||
- [x] **L33 — `idCounter` fallback resets per launch.** The non-crypto `newId` fallback
|
||
(`item-${++idCounter}`) restarts at 1 each run; the `Date.now()`-based fallbacks elsewhere can
|
||
collide within a millisecond. Unify on one robust id helper (see M7). *Fixed with M7: the single
|
||
`newId` fallback is `${prefix}-${Date.now()}-${++counter}`, so same-ms ids stay distinct.*
|
||
|
||
*Round 3 (2026-06-29) — tests, build metadata, error copy, edge cases:*
|
||
|
||
- [x] **L34 — Integration test off by default.** `real-download.integration.test.ts` is
|
||
`describe.skipIf(!RUN)`; `npm test` never exercises a real download, so there's no automated
|
||
regression for the actual yt-dlp argv path (only the pure builder). *Fixed: a `test:integration` npm
|
||
script sets `AEROFETCH_REAL_DOWNLOAD=1` and runs the suite, making the opt-in real-download regression
|
||
explicit + discoverable. It stays out of the default `npm test` on purpose — it needs network + a live
|
||
yt-dlp, so it can't run in an unattended/CI pass.*
|
||
- [x] **L35 — `isValidMediaItem` has no test** — the one persisted-JSON validator of six with no spec.
|
||
- [x] **L36 — `compareVersions` differing-length components untested** (e.g. `1.2` vs `1.2.0`).
|
||
- [x] **L37 — `download.ts` formatters untested** (`parseProgress`/`fmtBytes`/`fmtEta`, incl. the `NA` paths).
|
||
- [x] **L38 — `buildArgs` webm-thumbnail exclusion branch untested** (`embedThumbnail && container!=='webm'`).
|
||
- [x] **L39 — `CROP_SQUARE_PPA` exact-string assertion** is a change-detector test (breaks on harmless reformat).
|
||
- [x] **L40 — `clipboardLink.test.ts` imports the zustand settings store** to test two pure helpers
|
||
(`looksLikeUrl`/`looksLikeSingleVideo`) — couples a pure-fn test to store init (see H2).
|
||
- [x] **L41 — `package.json` `homepage` points at yt-dlp's GitHub**, not AeroFetch — wrong product
|
||
URL (surfaced by the NSIS installer).
|
||
- [x] **L42 — `package.json` has no `license` field** (ships LGPL ffmpeg + GPL aria2c).
|
||
- [x] **L43 — `package.json` has no `repository` field** (real repo is the Gitea instance).
|
||
- [x] **L44 — Vestigial lint excludes.** `electron-builder.yml` excludes `.eslintrc`/`.prettierrc`
|
||
but there's no ESLint/Prettier config, script, or dependency — no enforced style tooling.
|
||
- [x] **L45 — Three self-descriptions** drift: pkg `description` "A yt-dlp frontend for Windows" vs
|
||
builder `copyright` "yt-dlp frontend" vs Sidebar caption "yt-dlp frontend".
|
||
- [x] **L46 — yt-dlp-missing error copy differs across 4 modules** (download/ytdlp/probe/indexer):
|
||
"Reinstall AeroFetch, or drop…" vs "Download it into resources/bin/…" vs "Drop it into resources/bin/."
|
||
- [x] **L47 — `probeMeta` 30s timeout returns null silently** — no surfaced message, unlike every
|
||
other yt-dlp timeout ("Timed out …").
|
||
- [x] **L48 — `cleanError` strips a leading `error:`** for live failures, while raw `ERROR:` text
|
||
shows elsewhere (errorlog seed / terminal) — inconsistent error normalization.
|
||
- [x] **L49 — Newlines in user-facing error strings.** download.ts missing-binary messages embed
|
||
`\n`, which renders awkwardly in inline/Caption error spans.
|
||
- [x] **L50 — "Cookies saved" shown for 0 cookies.** Closing the sign-in window without logging in
|
||
still writes the file and reports `ok` with `cookieCount: 0`.
|
||
- [x] **L51 — Scheduled daily sync time hardcoded to 09:00** (schedule.ts) — on/off toggle only, no time picker.
|
||
*Fixed. New `Settings.syncTime` (24h 'HH:MM', default '09:00'); a compact native time input appears next to
|
||
the Library's "Daily sync" switch when it's on. `setScheduledSync(enabled, time?)` threads the time into
|
||
`schtasks /ST`, guarded by a shared `isValidSyncTime` (in @shared/ipc, used by both main's settings
|
||
validation and schedule.ts — the value reaches the schtasks argv, so malformed input falls back to the
|
||
default rather than being forwarded; unit-tested incl. injection-shaped values). The picker persists via
|
||
settings on change and re-registers the task on blur (one `schtasks /Create /F` per commit, not per
|
||
keystroke — /F overwrite IS the time-change mechanism). Live-verified on this machine: re-registering the
|
||
real AeroFetchDailySync task with /ST 19:30 moved its start to 7:30 PM and back (the user's original 09:00
|
||
registration restored byte-exact afterwards).*
|
||
- [x] **L52 — Installer handoff is a fixed `setTimeout(app.quit, 1500)`** (updater.ts) — a race on slow machines.
|
||
- [x] **L53 — `dialog.show*Dialog(win!, …)` non-null assertions** on a possibly-null window
|
||
(chooseFolder, backup export/import) — can throw if the sender window is gone.
|
||
- [x] **L54 — DownloadBar preview `<img>` has no `onError` fallback** (inconsistent with `MediaThumb`'s retry).
|
||
- [x] **L55 — QueueItem meta line can show size twice** — a probed-format `quality` label already
|
||
contains the size, then `sizeLabel` is appended again.
|
||
- [x] **L56 — SponsorBlock ON with zero categories silently no-ops** (`buildArgs` guards on
|
||
`length > 0`) with no UI warning.
|
||
- [x] **L57 — Scheduling a past datetime silently downloads now** (`buildItem` future-only guard),
|
||
no feedback that the schedule was ignored.
|
||
- [x] **L58 — `chooseFolder` passes the macOS-only `createDirectory` property** — dead option on Windows.
|
||
- [x] **L59 — `THEME_BACKGROUND` (index.ts) duplicates `pageBackground` (theme.ts)** — two
|
||
hand-synced color constants (a "keep in sync" comment guards them).
|
||
- [x] **L60 — `'external-url'` channel name lacks the `category:` prefix** every other IPC channel uses.
|
||
- [x] **L61 — `taskbarProgress` is the lone `ipcMain.on`** (fire-and-forget) amid all-`invoke` channels.
|
||
- [x] **L62 — "Best available" synthetic format hardcodes `ext:'mp4'`/`hasAudio:true`** (probe.ts) —
|
||
mislabels when the chosen container is mkv/webm.
|
||
- [x] **L63 — `audioQuality()` unknown label → silent `'0'` (best)** — the audio analog of H5.
|
||
- [x] **L64 — `ARIA2C_ARGS` connection params hardcoded** (`-x16 -s16 -k1M`), no user control.
|
||
*Fixed. New `Settings.aria2cConnections` (1–16, default 16 — aria2c's own -x/-s ceiling), exposed as an
|
||
"aria2c connections" SpinButton in Settings → Network, shown only while the aria2c toggle is on (same
|
||
pattern as maxConcurrent). The const became a pure `aria2cArgs(connections?)` in buildArgs.ts that clamps/
|
||
rounds defensively (settings validation clamps too — `applySettings` mirrors the maxConcurrent case);
|
||
`ARIA2C_ARGS` remains exported as `aria2cArgs()` so existing references stay valid. Threaded through
|
||
`AccessOptions.aria2cConnections` from buildCommand. The `-k 1M` split floor stays fixed — the meaningful
|
||
user knob is connection count; a free-text field would bypass the customCommandEnabled consent gate.
|
||
Unit-tested: custom count reaches `--downloader-args`, clamp floor/ceiling/rounding/non-finite.*
|
||
- [x] **L65 — Pause uses `taskkill /F`** like cancel — a forced kill risks an unflushed `.part` tail vs a graceful stop.
|
||
*Verified acceptable in the Batch 15 live pass — no code change needed. Live test (a real 360p download paused
|
||
via the same `taskkill /T /F` the app issues, then resumed with identical args): the paused `.part` was left
|
||
byte-intact at 3,913,524 bytes, and yt-dlp's default `--continue` logged `Resuming download at byte 3913524`,
|
||
restarting mid-file (first progress tick 16.7%, not 0%) and producing a valid complete file (ffprobe: full
|
||
281.6 s duration, exit 0). The feared "unflushed-tail corruption" does not occur: yt-dlp writes the `.part`
|
||
incrementally, a forced kill leaves the already-written bytes intact, and resume re-requests only from the
|
||
actual byte count — so at worst a few unflushed KB are re-fetched, never corrupted. A graceful Ctrl-C
|
||
(GenerateConsoleCtrlEvent) would add Windows-specific fragility (windowsHide, no shared console, whole-process-
|
||
group signalling) for zero correctness gain. Left as the forced kill. (NB: an unrelated observation from the
|
||
same test — a `bv*` quality-preset selector can re-resolve to a different format on resume and discard the
|
||
`.part`; a specific probed `formatId` resumes deterministically. Efficiency-only, still produces a valid file.)*
|
||
- [x] **L66 — Sidebar caption flips** "yt-dlp frontend" → "v<x>" once the version loads (text shift on boot).
|
||
- [x] **L67 — Onboarding has no Skip and can't be revisited** (no "show tips again"). *Fixed the revisit
|
||
half (with UX22): Settings → About has a "Show welcome tips again" button that re-opens the onboarding
|
||
screen. Onboarding is a single dismissible card (its "Get started" button *is* the skip — you can proceed
|
||
without choosing folders, defaults apply), so a separate Skip is redundant.*
|
||
- [x] **L68 — Background-running notification fires once per process** (`notifiedBackground` never
|
||
resets) — won't remind on later window closes.
|
||
- [x] **L69 — `settings.ts` mixes path helpers with persistence** (`getDefaultMediaDir`/
|
||
`ensureMediaDirs`/`getDownloadArchivePath` alongside the store) — split a `paths.ts`. *Fixed: the three
|
||
path helpers moved to [paths.ts](src/main/paths.ts); settings.ts is now just the electron-store persistence
|
||
layer (its now-unused `path`/`fs` imports dropped). download.ts and index.ts import the helpers from
|
||
`./paths`. `applyLaunchAtStartup` stays in settings.ts — it's OS login-item integration referenced by
|
||
`applySettings`, not a path helper.*
|
||
- [x] **L70 — `crypto.randomUUID` fallback is effectively unreachable** in Electron/Node 26
|
||
(`typeof crypto !== 'undefined'` is always true) — dead defensive branch (see M7). *Fixed with M7: the
|
||
branch now lives once in `newId` (kept so the fn is total in any host) and is covered by a test that stubs
|
||
`crypto` to exercise it.*
|
||
- [x] **L71 — Settings folder inputs are `readOnly`** with only a Browse button — no way to paste a
|
||
known path.
|
||
- [x] **L72 — History re-download drops the thumbnail** (passes only title/channel), so the
|
||
re-queued row loses its thumbnail until re-probed.
|
||
- [x] **L73 — Two verbs for the same probe action** — DownloadBar "Fetch" vs LibraryView "Index".
|
||
- [x] **L74 — Accent swatches are triple-labeled** (`aria-pressed` + `aria-label` + `title`).
|
||
- [x] **L75 — Update-token field always visible** in the Software-update card, even when no update
|
||
is pending — buries an advanced/rarely-needed input.
|
||
- [x] **L76 — Duplicate-warning may show a placeholder title** — `setDup(existing.title)` can be the
|
||
`titleFromUrl` placeholder before metadata resolves ('YouTube video (id)').
|
||
- [x] **L77 — ffmpeg/ffprobe versions have no re-check affordance** (load once; "not found" sticks
|
||
with no retry) unlike the yt-dlp "Check version" button.
|
||
- [x] **L78 — `DownloadsView` empty-state copy** ("Paste a URL above to get started") doesn't match
|
||
the actual two-step Fetch→Download affordance.
|
||
- [x] **L79 — SponsorBlock default categories = `['sponsor']` only** — enabling SB silently covers
|
||
just sponsors until the user expands categories (expectation gap).
|
||
|
||
*Round 4 (2026-06-29) — doc/code drift, build artifacts, edge cases:*
|
||
|
||
- [x] **L80 — ROADMAP wrong file path.** [ROADMAP.md](ROADMAP.md) Phase A says the flags are
|
||
emitted "in `buildArgs` (`src/main/download.ts`)" — `buildArgs` lives in `src/main/buildArgs.ts`.
|
||
- [x] **L81 — ROADMAP internal contradiction.** Phase A: "`--split-chapters` still TODO"; Phase L:
|
||
`[x]` split-chapters done. Phase A wasn't updated when L landed.
|
||
- [x] **L82 — PINCHFLAT roadmap interface sketches are stale.** [ROADMAP-PINCHFLAT.md](ROADMAP-PINCHFLAT.md)
|
||
Phase F's `Source`/`MediaItem` code blocks omit shipped fields (`videoId`, `itemCount`,
|
||
`watched`, `feedUrl`) — design sketches that drifted from `shared/ipc.ts`. *Fixed: the Phase F
|
||
sketch now carries the shipped `videoId`/`itemCount`/`watched`/`feedUrl` fields (plus the
|
||
required `url`/`playlistTitle`/`playlistIndex`) and a note that the block reflects the shipped
|
||
`shared/ipc.ts` shape.*
|
||
- [x] **L83 — Dead build artifacts.** electron-builder emits `.blockmap` differential-update files
|
||
for each NSIS installer, but the custom [updater.ts](src/main/updater.ts) does a full download and
|
||
never consumes them. *Fixed: `nsis.differentialPackage: false` in
|
||
[electron-builder.yml](electron-builder.yml) stops the blockmap being generated (NsisTarget gates it
|
||
on `differentialPackage !== false`), with a comment explaining the custom-updater rationale.*
|
||
- [x] **L84 — `dist/` accumulates unboundedly.** ~620 MB per build with no cleanup; it currently
|
||
holds 0.1.0–0.5.0 Setup + portable artifacts (~3 GB). Add a clean step or prune. *Fixed: a
|
||
`clean:dist` npm script (`fs.rmSync('dist', …)`) now runs at the front of `build:win`, so each
|
||
packaged build starts from an empty `dist/` and only the current version's artifacts remain.
|
||
(`dist`/`out` were already gitignored, so this is disk hygiene, not a repo change.)*
|
||
- [x] **L85 — `.claude/launch.json` is committed and misplaced.** A VS Code launch config lives in
|
||
`.claude/` (VS Code reads `.vscode/launch.json`), so no tool consumes it; it's also tracked
|
||
(not gitignored).
|
||
- [x] **L86 — History select-all + filter change deletes hidden rows.** `selected` persists across a
|
||
filter change, so "Delete selected" can remove entries no longer visible (surprising).
|
||
- [x] **L87 — History re-download ignores original options.** `redownload` re-queues url/kind/quality
|
||
only; the post-processing `DownloadOptions` used originally are lost (current defaults apply). *Fixed:
|
||
`HistoryEntry` now carries the original `options` (recorded on completion in `recordCompletion`), and
|
||
`redownload` replays them via `AddOptions.options` — so a re-download reproduces the same subtitles /
|
||
SponsorBlock / container / codec instead of the current global defaults. Entries recorded before this
|
||
(no stored options) fall back to the defaults as before.*
|
||
- [x] **L88 — `applyEvent('progress')` can promote a `queued` item to `downloading`** outside
|
||
`pump()` — a latent concurrency edge if a progress event ever arrives before/without the launch transition.
|
||
- [x] **L89 — Inconsistent release artifacts.** Only `0.4.1` carries `.sha256` files (hand-made) and
|
||
the version sequence skips 0.3.x — symptomatic of the manual release process behind H8. *Fixed at
|
||
the root by H8: [scripts/generate-checksums.cjs](scripts/generate-checksums.cjs) (electron-builder
|
||
`afterAllArtifactBuild` hook) writes a `.sha256` next to every built `.exe` and returns them for
|
||
upload, so future releases are consistent automatically; the stale pre-H8 `dist/` artifacts are
|
||
pruned by L84. (The historical 0.3.x version gap is immutable.)*
|
||
|
||
*Round 5 (2026-06-29) — code-level micro-inconsistencies & polish:*
|
||
|
||
- [x] **L90 — Two idioms for "value in const array."** [settings.ts](src/main/settings.ts) `sanitizeOptions`
|
||
uses `ARR.includes(x as never)` (L116–120) while `setSettings` uses `(ARR as readonly string[]).includes(x as string)` (L275/323). Pick one.
|
||
- [x] **L91 — Unsafe tuple cast / fragile encoding.** `value.split('|') as [MediaKind, string]`
|
||
([SettingsView.tsx](src/renderer/src/components/SettingsView.tsx):411) trusts a `'|'`-joined
|
||
`kind|quality` string; a value without `'|'` yields `undefined` quality. Use a typed pair, not a string.
|
||
- [x] **L92 — Preload names diverge from main fn names.** `markSourceItemDownloaded`↔`setMediaItemDownloaded`,
|
||
`syncSources`↔`syncWatchedSources`. Align the names across the boundary. *Fixed: the preload methods are
|
||
renamed to match main — `setMediaItemDownloaded` and `syncWatchedSources`
|
||
([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.*
|
||
- [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. *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
|
||
out a new items array per change, so both App's taskbar subscription and DownloadsView's render summarize
|
||
it once per change (the second caller hits the cache) instead of twice (also closes PERF4).*
|
||
- [x] **L95 — "Sign in" hyphenation varies** in the Cookies card ("Sign in…", "Site to sign in to", "Sign-in window", "Signing in…").
|
||
*Reviewed: the usages already follow standard English — the verb is two words ("Sign in…", "Signing in…",
|
||
"sign in to") and only the modifier is hyphenated ("Sign-in window"). Consistent by that rule; no change needed.*
|
||
- [x] **L96 — Ellipsis-on-dialog-buttons inconsistent.** "Export backup…/Import backup…/Sign in…" use
|
||
`…` (opens a dialog), but "Browse" / "Check for updates" / "Update now" don't. Adopt the `…` = opens-a-dialog convention.
|
||
*Fixed: the folder "Browse" buttons (they open the OS picker) are now "Browse…". "Check for updates" /
|
||
"Update now" open no dialog (inline result / background download), so they correctly keep no ellipsis.*
|
||
- [x] **L97 — Advanced-field placeholders inconsistent.** Adjacent fields use an example (`web_safari`),
|
||
a literal `(none)`, and `Optional — Gitea access token`. Pick one placeholder style. *Fixed: standardized
|
||
on "an example value where the field has a representative one (`web_safari`, `2M`, `socks5://…`),
|
||
`Optional -- <what>` for opaque optional fields" — the PO-token's uninformative `(none)` is now
|
||
`Optional -- paste a token`, matching the update-token field.*
|
||
- [x] **L98 — "Embed chapters" has no hint** while every sibling toggle in DownloadOptionsForm does.
|
||
- [x] **L99 — URL input has an `id` but no label.** `input={{ id: 'aerofetch-url' }}` with no
|
||
`<label for>`/`aria-label` (placeholder only) — see M28. *Resolved by M28: the URL input already carries
|
||
`aria-label="Video or playlist URL"`, so it has an accessible name — the placeholder isn't the only affordance.*
|
||
- [x] **L100 — Inconsistent elevation.** DownloadBar (`shadow4`) and CommandPalette (`shadow28`) float;
|
||
every other card (Settings, QueueItem, History, Library, Templates) is flat (border-only). Define an elevation scale.
|
||
*Fixed: `ELEVATION` in [tokens.ts](src/renderer/src/components/ui/tokens.ts) names the scale — `flat`
|
||
(border-only, the resting card), `raised` (shadow4, the floating download bar), `overlay` (shadow28, the
|
||
command palette). The two floating surfaces use the tokens; flat cards are documented as the default.*
|
||
- [x] **L101 — No z-index scale.** CommandPalette and Hint both hardcode `zIndex: 1000`.
|
||
*Fixed: `Z` = { overlay: 1000, tooltip: 1100 }. CommandPalette's backdrop is `Z.overlay`, Hint's bubble is
|
||
`Z.tooltip` (above overlays so a tooltip is never clipped by one).*
|
||
- [x] **L102 — Two mental models for "format."** Settings sets it via one combined Video/Audio dropdown
|
||
(`FORMAT_OPTIONS`); the DownloadBar uses a Video/Audio segmented control + a separate quality dropdown.
|
||
*Fixed: the Settings "Default format" control ([DownloadsCard.tsx](src/renderer/src/components/settings/DownloadsCard.tsx))
|
||
now uses the same shared `SegmentedControl` (Video/Audio) + quality `Select` as the DownloadBar, so both
|
||
screens present one mental model. The combined `FORMAT_OPTIONS`/`kind|quality` string encoding is gone —
|
||
the toggle writes `defaultKind` and the dropdown writes `defaultVideoQuality`/`defaultAudioQuality` directly.*
|
||
- [x] **L103 — Two busy-indicator patterns.** Fetch/Index swap the button icon to a `Spinner`; the
|
||
Settings check/update buttons keep their icon and render a *separate* adjacent `Spinner`.
|
||
*Resolved by L134 (Batch 8), verified this batch: every busy button swaps its own icon to a
|
||
`<Spinner size="tiny">` (Fetch/Index/Check-for-new/yt-dlp update/app-update/PO-mint/version-check); the only
|
||
remaining `<Spinner>`s are standalone status-row indicators (the "Fetching…"/"Waiting to start…"/indexing
|
||
rows), not a button + adjacent spinner. No separate-adjacent-spinner pattern survives.*
|
||
- [x] **L104 — History isn't virtualized.** It renders all filtered rows, while Downloads and Library use `VirtualList`.
|
||
*Fixed (Batch 17, with L161): HistoryView renders through the shared `VirtualList` inside the new `fill`
|
||
screen shell — the list owns an internal scroll region (flex + 160px floor, DownloadsView's contract). The
|
||
container-level Ctrl+A moved onto a wrapper around the virtualized scroller (key events bubble up from the
|
||
rows), and per-row Delete stays on each row so it works wherever the row mounts. Verified live in an
|
||
Electron probe against the vite preview at 1100×720 and 800×520: rows render via `[data-index]` virtual
|
||
wrappers, the list scrolls internally, and `<main>`/body never scroll (probe metrics + screenshots).*
|
||
- [x] **L105 — `MediaKind` declared twice.** In both [ipc.ts](src/shared/ipc.ts) and
|
||
[store/downloads.ts](src/renderer/src/store/downloads.ts); components import it from both. Single source it.
|
||
*Fixed: the store now imports `MediaKind` from `@shared/ipc` and re-exports it, so the duplicate local
|
||
`type MediaKind` is gone while existing `import { MediaKind } from '../store/downloads'` sites still resolve.*
|
||
- [x] **L106 — Refresh icons used interchangeably.** `ArrowClockwiseRegular` (retry, re-download,
|
||
check-for-new, update-yt-dlp) vs `ArrowSyncRegular` (re-index, check-for-updates) for the same "redo/refresh" idea.
|
||
- [x] **L107 — Spinner sizes mixed** — `"tiny"` almost everywhere, `"extra-tiny"` for the QueueItem queued row.
|
||
- [x] **L108 — Main window sets no explicit `title`** (relies on the document `<title>`).
|
||
- [x] **L109 — Versions auto-load with no indicator.** yt-dlp/ffmpeg versions in About just appear;
|
||
only the manual "Check" button shows a spinner. *Fixed (Batch 18, with UX26): the About card shows Fluent
|
||
`Skeleton` lines for the yt-dlp and ffmpeg versions while the mount loads them (`checking` now starts true
|
||
and settles in a `finally`; an ffmpeg bridge failure settles to "not found" rather than a permanent
|
||
skeleton). Verified in the preview probe that the card settles to the loaded state cleanly.*
|
||
- [x] **L110 — One muted-text token, many class aliases.** `colorNeutralForeground3` is re-declared as
|
||
`hint`/`sub`/`meta`/`srcSub`/`stats`/`count`/`emptyHint`/`previewMeta` across components. *Fixed: one
|
||
`useTextStyles().muted` in [ui/text.ts](src/renderer/src/components/ui/text.ts). Adopted for the pure-color
|
||
aliases in the list views (QueueItem `meta`, HistoryView `meta`, LibraryView `sub`/`srcSub`/`rowMeta`,
|
||
Sidebar `caption`, DownloadBar/PlaylistPanel `previewMeta`/`plItemMeta`) — the color is identical, so no
|
||
visual change. The combined classes (`stats`/`count`/`emptyHint`), the shared-across-cards settings `hint`,
|
||
CommandPalette, Onboarding, and the inline styles migrate incrementally (same pattern M12 used for errorText).*
|
||
- [x] **L111 — Two helper-text mechanisms** — Fluent `Field` `hint` vs manual `<Caption1 className={hint}>`; SettingsView uses both.
|
||
*Resolved (convention clarified + verified): the two aren't interchangeable — Fluent `Field hint` is for help
|
||
associated with a specific control (announced via `aria-describedby`), and `<Caption1 muted>` is for
|
||
standalone card intros and status lines ("Saved to…", "No errors yet"). Control help already uses `Field
|
||
hint` uniformly, so the split is role-appropriate rather than an inconsistency.*
|
||
- [x] **L112 — `Switch` labeling inconsistent.** Settings/DownloadOptionsForm show "On"/"Off" text;
|
||
Library switches use `aria-label` only (no visible state text). *Fixed: dropped the dynamic
|
||
`label={x ? 'On' : 'Off'}` from all 15 `<Field>`-wrapped switches (DownloadOptionsForm ×6 + settings cards
|
||
×9). The Field already names each switch, and the toggle position + `aria-checked` convey state — so no
|
||
switch shows redundant On/Off text now, matching Library's inline switches.*
|
||
- [x] **L113 — Tooltip ≠ aria-label.** The Fetch button's Hint says "Fetch formats / playlist" but its
|
||
`aria-label` says "Fetch formats or playlist" (DownloadBar). *Resolved by UX2: the Fetch button's Hint
|
||
tooltip and `aria-label` are now the same string ("Preview available formats (optional)"), so what a
|
||
sighted user sees on hover and what a screen reader announces match.*
|
||
- [x] **L114 — Row titles use different components** — `Text` (QueueItem/History/Templates) vs `span` (Library) vs `Caption1` elsewhere.
|
||
*Fixed: `<Text>` is the one row-title component. Library's three `<span>` titles (`groupTitle`, `rowTitle`,
|
||
`srcTitle`) became `<Text>` — size-identical since Fluent's provider base font is already `fontSizeBase300`.
|
||
The duplicated semibold-ellipsis title style (QueueItem/HistoryView/DownloadBar `previewTitle`) is now the
|
||
shared `useTextStyles().title`.*
|
||
- [x] **L115 — Equivalent row actions labeled inconsistently** — "Re-index" is icon+text; the
|
||
comparable "Retry"/"Remove"/"Re-download" are icon-only. *Resolved (convention verified + role-appropriate):
|
||
the two live in different UI contexts, and each context is internally consistent. **List-row item actions**
|
||
(QueueItem's Pause/Cancel/Retry/Remove/Open, History's Re-download/Open/Show/Remove) are icon-only with a
|
||
`Hint` tooltip + a ≥40px hit target — a compact, repeated-per-row affordance. **Card/toolbar actions** (the
|
||
Library source-detail row: Refresh/Remove/Download, the Downloads/History headers) are labeled icon+text —
|
||
a one-per-card toolbar where the label aids scanning. The former "Re-index" is the Library card toolbar's
|
||
"Refresh", correctly labeled alongside its siblings. No screen mixes the two within one context.*
|
||
- [x] **L116 — Empty-state structure varies** — Downloads (icon + line), History (colored badge + line + sub-hint), Library (icon + line, no sub-hint).
|
||
*Fixed: one shared [EmptyState](src/renderer/src/components/ui/EmptyState.tsx) — a centered column with an
|
||
optional focal `icon` (a hero glyph or History's colored badge), a `Body1` message at normal foreground, and
|
||
an optional muted `hint`. Adopted by Downloads, History, and Library, so the three now share one structure
|
||
(History keeps its badge + hint; Downloads/Library pass a hero glyph). The per-view `empty`/`emptyHint`
|
||
styles are deleted.*
|
||
- [x] **L117 — Terminal/error-log "empty" aren't centered blocks** like the other empty states (inline text instead).
|
||
*Fixed: both now render the shared `EmptyState` in its `compact` variant (smaller inset, for a placeholder
|
||
inside an already-populated screen). The Terminal log centers "No output yet." in the log box (a `logEmpty`
|
||
flex modifier); Diagnostics centers "No errors yet." — so both read as empty states rather than a stray line
|
||
of inline text.*
|
||
- [x] **L118 — Cross-component focus via hardcoded id.** App focuses the URL field with
|
||
`document.getElementById('aerofetch-url')` — brittle coupling into DownloadBar's markup.
|
||
*Fixed: the DownloadBar registers a focuser for its own URL input (a local ref) in the nav store
|
||
(`registerUrlFocuser`); App calls `useNav.getState().focusUrlField()` on launch / "New download" instead of
|
||
querying the DOM by id ([nav.ts](src/renderer/src/store/nav.ts), [App.tsx](src/renderer/src/App.tsx),
|
||
[DownloadBar.tsx](src/renderer/src/components/DownloadBar.tsx)). The `id="aerofetch-url"` is gone — App no
|
||
longer reaches into DownloadBar's markup; the double-rAF (wait for mount/paint) stays so the focuser is
|
||
registered before it's called.*
|
||
- [x] **L119 — "No items" copy varies** — "No … yet" (most) vs "Output will appear here." (Terminal) vs "No errors logged." (Diagnostics).
|
||
- [x] **L120 — Fluent `Card` vs hand-styled `div` cards.** Only Settings/Onboarding use `<Card>`; every
|
||
other card surface is a bespoke styled `<div>`. *Resolved (convention documented + tokens applied):
|
||
static content cards (Settings cards, Onboarding) use Fluent `<Card>`; interactive & list-item surfaces
|
||
(the download bar, QueueItem, History/Library rows, the Library source card, the command palette) use the
|
||
Batch-6 tokenized surface recipe — `RADIUS.card`/`surface`, `ELEVATION`, `SPACE` padding on a styled `<div>`
|
||
— because Fluent's `Card` imposes its own internal padding/gap/interaction model that a custom row layout
|
||
fights. Forcing `<Card>` onto those would regress layout for no gain; the divergence L120 flags is now a
|
||
documented tier split, and the styled surfaces share the same radius/elevation tokens. (The larger "wrap
|
||
every recurring pattern" primitive work stays UI15's incremental track.)*
|
||
- [x] **L121 — Raw scrim literal.** CommandPalette backdrop is `rgba(0,0,0,0.32)` (the only raw rgba in
|
||
the renderer), identical in light/dark — low contrast over a dark UI. *Fixed: `SCRIM` in tokens.ts aliases
|
||
Fluent's semantic `colorBackgroundOverlay` (the dialog scrim); the raw rgba is gone and the palette
|
||
separates more cleanly from a dark UI.*
|
||
- [x] **L122 — "Checking…" indicated twice** — the Settings update button changes its label to "Checking…" *and* a separate Spinner renders beside it.
|
||
- [x] **L123 — `Caption1` is overloaded** for hints, metadata, counts, and timestamps — one size doing four jobs.
|
||
*Resolved: the [ui/text.ts](src/renderer/src/components/ui/text.ts) header defines `Caption1`'s role as "small
|
||
secondary text" — hints/metadata/counts/timestamps are all that one role, now stated on purpose rather than by
|
||
accident (paired with the shared `muted` color for L110).*
|
||
- [x] **L124 — `Body1` vs `Text` role overlap** — `Body1` appears only in empty states/onboarding/gate; `Text` carries titles; the split isn't principled.
|
||
*Resolved: the type-role convention in text.ts makes the split explicit — `Body1` = body copy (empty states,
|
||
onboarding prose, gated notices); `Text` = row/item titles. Current usage already follows it.*
|
||
- [x] **L125 — Icon sizing has two syntaxes** — px strings (`fontSize: '20px'`) on icon *containers* vs numeric `fontSize={28}` on inline icons.
|
||
*Fixed: both syntaxes now single-source from `ICON` in tokens.ts (`fontSize={ICON.control}` on inline icons,
|
||
`` fontSize: `${ICON.control}px` `` on the few makeStyles containers) — no more divergent magic numbers.
|
||
Glyphs sized to a box rather than to this scale (brand/source tiles, the tiny `watchBadge` icon, thumbnail
|
||
placeholders) are documented exclusions and stay proportional.*
|
||
- [x] **L126 — Uneven text truncation** — some titles/metas use `nowrap`+ellipsis, others wrap; no shared truncation utility.
|
||
*Fixed: a shared `useTextStyles().truncate` ([text.ts](src/renderer/src/components/ui/text.ts)) — single-line
|
||
clip with ellipsis + `minWidth:0` — sits alongside the existing `title` (which is `truncate` + semibold).
|
||
Adopted for the ad-hoc copies: Library `rowTitle`, the DownloadBar playlist `plItemTitle`/`plHeaderText`
|
||
(the header is now `title`), and the shared `Banner` content (UI19). `title` also gained `minWidth:0` so it
|
||
shrinks correctly in a flex row. (Remaining correct-but-inline truncations migrate to the util incrementally.)*
|
||
- [x] **L127 — `Notification` sets no icon** — completion toasts used the default Electron icon though `getAppIconPath()` existed. *Fixed with W13 (cached `getAppIconImage()` passed as `icon` at both notification sites).*
|
||
- [x] **L128 — Preview seed/mock data ships in production.** The `PREVIEW` runtime check can't be
|
||
tree-shaken, so each store's seed arrays + fake ticker are bundled into the Electron renderer.
|
||
*Fixed: the three preview seed arrays (`downloadSeed`, `sources` `seedSources`/`seedItemsBySource`,
|
||
`history` seed) are now gated on `import.meta.env.DEV && PREVIEW`. In the packaged renderer build
|
||
`import.meta.env.DEV` is statically `false`, so Rollup constant-folds `false && …` and dead-code-eliminates
|
||
the whole seed literal; the browser dev preview (`vite`, DEV `true`) still gets them. (The small `PREVIEW`
|
||
fake-ticker function stays — it's tied to the runtime flag and is a few lines; the bulky part was the data,
|
||
now dropped.)*
|
||
- [x] **L129 — `Hint align` is silently ignored for left/right placements** yet callers still pass it.
|
||
*Fixed: `HintProps` is now a discriminated union — `align` is only accepted with `placement` `top`/`bottom`;
|
||
passing it with `left`/`right` is a compile error instead of a silent no-op. No caller needed changing (the
|
||
left/right hints already omit `align`).*
|
||
- [x] **L130 — Error-string style is mixed** — internal errors are lowercase/no-period ("timed out",
|
||
"interrupted"); user-facing ones are capitalized with periods. Fine when interpolated, jarring if shown raw.
|
||
*Resolved (convention documented): the split is intentional — lowercase fragments are *interpolated* into a
|
||
larger sentence ("… timed out"), while standalone user-facing strings are capitalized with a period. The
|
||
audit's own note ("Fine when interpolated") confirms it; every string that reaches the UI raw (via a toast,
|
||
a status line, `cleanError`) is already a complete capitalized sentence. Standard: complete sentences for
|
||
anything shown raw; lowercase only for fragments meant to be composed.*
|
||
- [x] **L131 — Taskbar `error` state is ephemeral** — it's driven by live `error` items, so "Clear
|
||
finished" flips the red taskbar bar back to normal even though failures occurred (out of sync with the persisted error log).
|
||
*Resolved as by-design (Batch 20): the taskbar mirrors the live queue, and "Clear finished" is the user
|
||
acknowledging those results — a red bar lingering after the queue is cleared would point at nothing the
|
||
window can explain and would need its own dismiss affordance. Attention at failure time is already covered
|
||
by the notification + `flashFrame` (W10), and the durable record lives in Diagnostics/errorlog. Judged on
|
||
the state semantics rather than pixels; the running production instance on this machine was left
|
||
undisturbed.*
|
||
- [x] **L132 — `clearProbe` + form-reset logic is duplicated** in DownloadBar (`onUrlChange`, `download`, `addPlaylist`).
|
||
*Fixed: a shared `resetForm()` in [useDownloadBar.ts](src/renderer/src/components/downloadBar/useDownloadBar.ts)
|
||
now owns the post-enqueue reset (URL + probe + the one-shot trim/schedule/advanced/incognito/options),
|
||
called by both `download()` and `addPlaylist()`. `clearProbe()` stays the lighter probe-only reset for
|
||
`onUrlChange`. Also fixes a latent inconsistency — `addPlaylist` previously left the trim/schedule/advanced
|
||
panels open after enqueuing.*
|
||
- [x] **L133 — `Field` controls mix `aria-label` + visible `label`.** Many `Field label="X"` wrap a
|
||
`Select aria-label="X"`, so AT announces the name twice (extends M20 across DownloadOptionsForm).
|
||
*Fixed: the custom [Select](src/renderer/src/components/Select.tsx) is now `<Field>`-aware
|
||
(`useFieldControlProps_unstable`, `supportsLabelFor: true`), so a Field's visible label names the native
|
||
`<select>` via `htmlFor` and its hint associates via `aria-describedby`. Removed the now-duplicate
|
||
`aria-label` from every Field-wrapped select (DownloadOptionsForm ×4, AboutCard, AppearanceCard, CookiesCard
|
||
×2, DownloadsCard, CustomCommandsCard). The two standalone selects — the download-bar quality picker and the
|
||
history type filter — keep their `aria-label`, since there's no Field to name them.*
|
||
- [x] **L134 — Inconsistent disabled affordance for busy buttons.** Some stay enabled-looking with a
|
||
spinner icon (Fetch/Index) while others are `disabled` with adjacent text (Settings) — no single "busy" convention.
|
||
*Fixed: one busy convention — `disabled` while working + the button's own icon swaps to a `<Spinner
|
||
size="tiny">`. Fetch, Index, Check-for-new and the yt-dlp update button already did this; converted
|
||
AboutCard's version-check button off its separate adjacent spinner to match.*
|
||
- [x] **L135 — `Textarea` vs `Input` for similar fields.** Trim uses a `Textarea`; the visually-similar
|
||
single-line fields use `Input` — fine, but the trim/schedule panels and the template form mix both with no shared field style.
|
||
*Resolved: verified role-appropriate — the trim field is genuinely multiline (time ranges "one per line",
|
||
`resize="vertical"`), so `Textarea` is correct there and `Input` for the single-line fields. Kept as-is; the
|
||
shared field styling is covered by the Fluent `<Field>` wrapper they all use.*
|
||
|
||
*Round 6 (2026-06-29) — behavioral/logic edge cases:*
|
||
|
||
- [x] **L136 — Incognito leaks outside history.** Even if the (unreachable, M6) private flag were set,
|
||
`download.ts` still writes failures to `errorlog.json` (title + URL) and shows the title in completion
|
||
notifications — the "private" promise covers only history. *Fixed: `incognito` now flows through
|
||
`StartDownloadOptions` to main (the renderer's launch path passes `item.incognito`), where main enforces
|
||
the full "no logging, no history, no cookies" promise: `logFailure` skips the errorlog write (and the
|
||
pre-spawn errorlog in [ipc.ts](src/main/ipc.ts) too), `notify` uses a generic outcome title and drops the
|
||
detail body so no title/URL lands in the Windows Action Center, and both cookie sources
|
||
(`--cookies`/`--cookies-from-browser`) are withheld so a private download can't be tied to the signed-in
|
||
identity. (M6 wired the toggle; this makes the promise true in main, not just the renderer.)*
|
||
- [x] **L137 — Stuck 0% for unknown-size downloads.** `parseProgress` returns `progress: 0` when
|
||
`total_bytes`/`_estimate` are absent (livestreams, some sites), so the bar reads 0% the whole time
|
||
instead of an indeterminate state. *Fixed: `parseProgress` now sets `sizeUnknown: !totalBytes` on
|
||
`DownloadProgress`; the store threads it onto the item and `QueueItem` renders an indeterminate bar +
|
||
"Downloading…" (instead of a frozen 0%) when it's set, reusing the existing SR7 indeterminate path.*
|
||
- [x] **L138 — Clipboard read on every window focus.** [useClipboardLink.ts](src/renderer/src/useClipboardLink.ts)
|
||
reads the full clipboard on each `focus` (and once on mount, possibly before `clipboardWatch` has
|
||
loaded — FALLBACK is `true`). Reading all clipboard text whenever focused may surprise privacy-conscious users.
|
||
*Fixed: the check now gates on the settings store's `loaded` flag as well as `clipboardWatch`, so the
|
||
clipboard is never read using the pre-load fallback (the mount-before-settings-load window); the effect
|
||
re-runs when `loaded` flips true, so a link copied before launch is still offered once the real setting
|
||
is known. (Per-focus reads were already gated on `clipboardWatch`; the privacy default itself is SR4.)*
|
||
- [x] **L139 — Auto-update vs concurrent spawn race.** `runStartupYtdlpAutoUpdate` can overwrite
|
||
`yt-dlp.exe` on launch while a deep-link/queued download spawns the same binary — a Windows
|
||
file-in-use/corrupt-spawn window.
|
||
*Fixed with a spawn↔update interlock in new [ytdlpLock.ts](src/main/ytdlpLock.ts) (pure, unit-tested in
|
||
test/ytdlpLock.test.ts). Each yt-dlp spawn registers via `acquireSpawn`/`releaseSpawn` (download.ts +
|
||
terminal.ts, released on every settle path incl. the stall watchdog); `updateYtdlp` takes the lock with
|
||
`beginYtdlpUpdate`, which REFUSES when any spawn is live so the best-effort daily update simply defers a
|
||
cycle rather than rewrite the exe under a running process. The reverse direction is a HOLD, not a failure
|
||
(per the audit's wording): the `downloadStart` / `terminalRun` IPC handlers `await whenYtdlpUpdateSettled()`
|
||
before spawning, so an auto-resumed persisted-queue download that lands during the ~2–5 s rewrite waits it out
|
||
and then proceeds (bounded by the update timeout — `endYtdlpUpdate` always runs in a `finally`).
|
||
`runStartupYtdlpAutoUpdate` also skips (without recording the daily check) if a spawn is already live. The
|
||
real driver was confirmed live via [queuePersist.test.ts](test/queuePersist.test.ts): a `downloading` item is
|
||
persisted and restored as `queued`, so persisted downloads DO auto-resume at launch — exactly the collision
|
||
this closes. The Windows exe-swap race itself is a milliseconds-wide, non-deterministic window that can't be
|
||
reproduced on demand, but the interlock provably serialises the update against every spawn path (302 tests
|
||
green).*
|
||
- [x] **L140 — Retry-during-teardown race.** `retry` reuses the same id; if the previous process's
|
||
`close` handler hasn't removed it from `active` yet, `startDownload` returns "already running" → markError.
|
||
*Fixed in [download.ts](src/main/download.ts): `cancelDownload`/`pauseDownload` now drop the item from
|
||
`active` synchronously (so a retry/resume's same-id spawn is never rejected by the stale entry), and the
|
||
doomed child's `close`/`error`/watchdog release the slot through a new `releaseActive(id, rec)` that only
|
||
deletes when that rec still owns the id — so a late teardown can't evict the fresh spawn. The transient
|
||
cookie jar is now keyed per spawn (`-${spawnSeq}`) so the old download's cleanup can't unlink the new one's
|
||
file. (with L148)*
|
||
- [x] **L141 — Renderer history grows unbounded in memory.** `useHistory.add` prepends without a cap
|
||
while main caps `history.json` at 500; in a long session the in-memory list exceeds the persisted cap (resets on reload).
|
||
*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).*
|
||
- [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).
|
||
*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).
|
||
*Fixed in [index.ts](src/main/index.ts): when the window is closed while a download is running AND the user is
|
||
NOT in minimize-to-tray mode (i.e. the app is only staying alive because of the download), the close handler
|
||
now shows a native, tray-INDEPENDENT `dialog.showMessageBox` — "Keep downloading" (hide to background) vs
|
||
"Quit anyway" (which calls `markQuitting()` + `app.quit()`, the same path the tray Quit uses). A
|
||
`quitPromptOpen` guard stops the dialog stacking if the X is hammered. This replaces the old passive
|
||
once-per-process "still running" notification (whose own body told users to "use the tray icon to … quit" —
|
||
the very dependency this removes), so `notifyBackgroundOnce` + its `notifiedBackground` latch / `show`-reset
|
||
(L68) are removed as dead code. A deliberate minimize-to-tray close still hides silently (the user opted in).
|
||
Verified the modified main process launches cleanly — the removed `Notification`/`getAppIconImage` imports and
|
||
the new dialog wiring load and create the window with no error; typecheck + lint + 302 tests + production
|
||
build green. The dialog is a standard `showMessageBox` (same family as the app's folder picker) and "Quit
|
||
anyway" reuses the proven quit path.*
|
||
- [x] **L144 — Duplicate guard checks the queue only, not history.** Re-adding a URL downloaded earlier
|
||
(already cleared from the queue) gives no "already downloaded" hint (roadmap notes this as a possible extension).
|
||
*Fixed: the DownloadBar dup guard ([useDownloadBar.ts](src/renderer/src/components/downloadBar/useDownloadBar.ts))
|
||
now checks history when a URL isn't in the live queue — if it was downloaded before, the warning banner reads
|
||
"Already downloaded: …" (vs "Already in your queue: …"), with the same "Download anyway" confirm. A new
|
||
`dupInHistory` flag drives the wording.*
|
||
- [x] **L145 — `useClipboardLink` re-offers a dismissed link after a tab switch.** `lastSeen` is
|
||
per-hook-instance; switching Downloads↔Library remounts it, so a previously-dismissed clipboard link is offered again.
|
||
*Fixed: `lastSeen` is now a module-level variable (not a per-instance `useRef`), so a dismissed/accepted
|
||
clipboard link stays suppressed across hook remounts. Only one field is mounted at a time (tab switch), so
|
||
there's no cross-field contention.*
|
||
- [x] **L146 — `parseTrimSections` accepts malformed multi-colon times.** The `\d+(?::\d{1,2})*` pattern
|
||
passes tokens like `1:2:3:4-5:6:7:8`, which then reach yt-dlp's `--download-sections` and fail there
|
||
rather than being rejected up front.
|
||
- [x] **L147 — macOS branches in a Windows-only app.** `app.on('activate')` and the
|
||
`process.platform !== 'darwin'` guard in `window-all-closed` are dead on Windows — boilerplate that implies multi-platform support the app doesn't ship.
|
||
- [x] **L148 — `clearFinished`/remove can free a slot before the killed process exits.** Removing a
|
||
just-canceled item lets `pump()` launch into the "freed" slot while `taskkill` is still tearing down the prior tree — a brief window over the concurrency cap.
|
||
*Fixed with L140: `cancelDownload`/`pauseDownload` release the `active` slot the instant they issue the kill
|
||
(rather than on the async `close`), so main's concurrency accounting matches the renderer immediately — the
|
||
just-promoted next item is neither falsely rejected by the maxConcurrent guard nor run over the cap while the
|
||
prior tree dies.*
|
||
|
||
*Round 7 (2026-06-29) — copy, a11y patterns & micro-behavior:*
|
||
|
||
- [x] **L149 — Hint length wildly inconsistent.** Most field hints are one short line, but "Keep running
|
||
in the tray" is a three-sentence paragraph with a parenthetical. Normalize hint length/voice. *Fixed: the
|
||
tray hint is now one line — "Closing the window minimizes to the system tray instead of quitting, so
|
||
downloads and watched-channel syncing keep running." — matching the one-line voice of its sibling hints.*
|
||
- [x] **L150 — "PO token" capitalization varies** — `PO Token` (ipc.ts/ROADMAP) vs `PO token` /
|
||
`Proof-of-Origin token` (SettingsView). Pick one.
|
||
- [x] **L151 — Mixed range dashes.** En-dash in prose ("2–3 is a good balance") vs hyphen in time-range
|
||
placeholders ("1:30-2:00"). Choose one convention.
|
||
- [x] **L152 — Search-field sizing differs per screen.** History `minWidth 180 / maxWidth 320`, Settings
|
||
inline `width: 100%`, DownloadBar `flexGrow`. No shared search-input width. *Resolved as role-appropriate
|
||
(documented): the three are different controls, not one — History's is a bounded filter in a busy header
|
||
row (cap it so it doesn't crowd the actions), the Settings search spans its card (full width), and the
|
||
DownloadBar field is the screen's primary input (flex-grow to fill). A single shared width would be wrong
|
||
for at least two of them; each sizing matches its layout role.*
|
||
- [x] **L153 — Generic "Dismiss" aria-labels.** Three close buttons (DownloadBar suggestion + dup,
|
||
Library suggestion) all read just "Dismiss" — ambiguous to a screen reader. Name what's dismissed.
|
||
- [x] **L154 — Instructional text in an aria-label.** Sidebar theme-cycle button is
|
||
`aria-label="Theme: Dark. Click to change."` — "Click to change" is UI instruction, not a name.
|
||
- [x] **L155 — Accent swatches aren't a `radiogroup`.** They're a single-select set rendered as plain
|
||
buttons with `aria-pressed`, while the Theme controls (also single-select) use `role="radiogroup"`/`radio`.
|
||
Inconsistent single-select a11y pattern. *Fixed: the accent swatches are now a real `role="radiogroup"` with
|
||
`role="radio"` + `aria-checked` swatches, roving tabindex, and ←/→/↑/↓/Home/End arrow-key navigation +
|
||
the shared focus ring ([AppearanceCard.tsx](src/renderer/src/components/settings/AppearanceCard.tsx)) —
|
||
matching the `SegmentedControl` radiogroup pattern used elsewhere. The redundant "(selected)" text in the
|
||
`aria-label` is gone (aria-checked conveys it).*
|
||
- [x] **L156 — `datetime-local` schedule has no `min`.** Past times are selectable, then silently download
|
||
now (extends L57). Add `min={now}`.
|
||
- [x] **L157 — `removeSource` doesn't cancel in-flight downloads.** Removing a source while its videos are
|
||
downloading lets them finish into the removed source's folders; `markDownloaded` then no-ops on the gone item.
|
||
*Fixed: `removeSource` now emits a `sourceRemoved` event on the coordinator bus (C2); the downloads store
|
||
subscribes and cancels every active (downloading/queued) item whose `mediaItemId` starts with `${sourceId}:`
|
||
— MediaItem ids are `${sourceId}:${videoId}` (indexerCore), so the prefix reliably identifies the source's
|
||
downloads. Reuses the proven `cancel()` path (process kill + pump). **Live-smoke pending** (per the
|
||
mark-done convention): implemented + type/lint/test-green, but the actual process-kill-on-remove is worth a
|
||
watched confirm in the Batch 15 live pass.*
|
||
- [x] **L158 — Drag highlight flickers.** `onDragLeave={() => setDragActive(false)}` fires when the cursor
|
||
crosses child elements (no enter/leave depth counter), so the dashed-outline blinks during a drag.
|
||
*Fixed: [useDownloadBar.ts](src/renderer/src/components/downloadBar/useDownloadBar.ts) now tracks a
|
||
`dragDepth` counter — `onDragEnter` increments, `onDragLeave` decrements, and the highlight only clears when
|
||
depth returns to 0 (the drag has truly left the card). `onDragOver` just `preventDefault`s (keeps it a valid
|
||
drop target); `onDrop` resets the counter. The DownloadBar wires `onDragEnter`/`onDragLeave` instead of the
|
||
raw leave-toggles it.*
|
||
- [x] **L159 — Dead fallback branch.** `copyErrorReport` falls back to "No errors logged." but its button
|
||
is disabled when there are no entries, so the fallback is unreachable.
|
||
- [x] **L160 — One-off inline margins.** `marginTop` literals (Onboarding 2px, QueueItem 4px, SettingsView
|
||
8px) instead of style classes (extends L5). *Fixed: the margin literals now draw from `SPACE`
|
||
(`hairline`/`xtight`/`tight`) — same values, one source. (The broader inline-`style` vs makeStyles split is
|
||
still L5.)*
|
||
- [x] **L161 — `height: 100%` screens inside a padded scroll container.** Downloads/Terminal set
|
||
`height: 100%` while App's `<main>` is `overflowY: auto` + 24/28px padding — a fragile coupling that can
|
||
double-scroll or misalign the virtualized list's viewport. *Fixed (Batch 17): `<main>` no longer scrolls or
|
||
pads — it's a plain flex slot, and each screen owns its scrolling through two shared shells in
|
||
[ui/Screen.tsx](src/renderer/src/components/ui/Screen.tsx): `page` (document screens — Library, Settings,
|
||
empty History: the shell scrolls) and `fill` (internally-scrolling screens — Downloads, Terminal, History:
|
||
the screen's list/log is the scroller, with the shell scrolling only as the short-window fallback). The
|
||
`height:100%` couplings are gone (`flexGrow:1; minHeight:0` inside the shell instead). The rework surfaced a
|
||
real pre-existing edge: on a very short window the flexed list collapsed to 0px — now floored at 160px with
|
||
the shell absorbing the overflow. Verified with an Electron probe at 1100×720 and 800×520 per screen:
|
||
exactly one active scroller per screen in the normal case, `<main>`/body never scroll, floors hold
|
||
(probe metrics + screenshots).*
|
||
- [x] **L162 — Per-thumbnail store subscription.** Every `MediaThumb` calls `useResolvedDark()` (two store
|
||
subscriptions) instead of receiving a resolved theme prop — N subscriptions per list. *Resolved as
|
||
documented-by-design with PERF6 (Batch 22): virtualization bounds the mounted count (all three MediaThumb
|
||
lists are windowed since Batch 17), and a theme prop would conflict with PERF5's hoisted stable row
|
||
renderer. See PERF6 for the full rationale.*
|
||
- [x] **L163 — QueueItem makes 9 separate `useDownloads` selector calls** instead of one destructured
|
||
selection — repeated boilerplate per row. *Fixed: one `useShallow` selection replaces the 10 individual
|
||
subscriptions in [QueueItem.tsx](src/renderer/src/components/QueueItem.tsx) (the actions are stable, so the
|
||
shallow compare never re-renders the row).*
|
||
|
||
*Round 8 (2026-06-29) — formatting micro-inconsistencies:*
|
||
|
||
- [x] **L164 — Metadata separator glyph/spacing is inconsistent.** Meta lines join with `' • '`
|
||
(2-space bullet) in QueueItem/History/DownloadBar/DownloadsView, with `' · '` (1-space midd·dot) in
|
||
LibraryView, and with `' • '` (1-space bullet) in QueueItem's "Paused • 42%". Three styles for one role.
|
||
**Standard:** one separator constant (glyph + spacing). *Fixed: one `META_SEP = ' • '` in tokens.ts, adopted
|
||
at every meta-line join (QueueItem/History/DownloadBar/DownloadsView/Library/PlaylistPanel). The `' · '`
|
||
inside compound quality labels ("720p · mp4 · 184 MB") is a data format parsed by HistoryView and is
|
||
intentionally left as-is.*
|
||
- [x] **L165 — The two byte/speed formatters round differently.** `download.ts fmtBytes` uses
|
||
`toFixed(v >= 100 ? 0 : 1)` ("512 MB") while `queueStats formatSpeed` uses `toFixed(1)` ("512.0 MB/s") —
|
||
visibly different precision for the same magnitudes (compounds the duplicate-formatter issue H2/M9).
|
||
- [x] **L166 — ETA has no hour rollover.** `fmtEta` (download.ts/downloads.ts) and `formatEta`
|
||
(queueStats) emit `M:SS` only, so a 2-hour ETA renders as **"120:00"**, whereas `fmtDuration`
|
||
(indexerCore) correctly rolls to `H:MM:SS`. Unify on the hour-aware format.
|
||
- [x] **L167 — `PROGRESS_TEMPLATE` is exported but used only inside `buildArgs.ts`** — a superfluous
|
||
public export (download.ts parses the `prog|` lines independently). Make it module-private.
|
||
|
||
*Round 9 (2026-06-29) — type-safety & build config:*
|
||
|
||
- [x] **L168 — `noUncheckedIndexedAccess` is off.** Array/index/tuple access is typed as always-defined,
|
||
so the exact edge cases already filed compile clean: the `split('|') as [MediaKind, string]` cast
|
||
(L91/CL2) and `parseProgress`'s positional destructure of a possibly-short `split('|')`. Enabling it
|
||
would surface them at build time.
|
||
- [x] **L169 — `noFallthroughCasesInSwitch` is off.** The large `setSettings` switch and `applyEvent`
|
||
switch aren't fallthrough-guarded; a missing `break`/`return` wouldn't be caught.
|
||
- [x] **L170 — No source maps in production.** `sourceMap: false` (toolkit) and no `build.sourcemap` in
|
||
`electron.vite.config.ts`, so field crash stack traces are unmapped — combined with no logging (CC8/M29),
|
||
diagnosing a user-reported crash is very hard. Consider hidden/external source maps. *Fixed:
|
||
`build.sourcemap: 'hidden'` on all three bundles (main / preload / renderer) in
|
||
[electron.vite.config.ts](electron.vite.config.ts) — `.map` files are emitted for offline symbolication
|
||
of the raw stacks the logger now records (CC8), with no `sourceMappingURL` reference in the shipped bundle.*
|
||
- [x] **L171 — `main.tsx` mock drift.** The preview mock's version strings disagree
|
||
(`getAppVersion`→'0.4.0-preview' vs `checkForAppUpdate.currentVersion`→'0.4.0'), and its `setSettings`
|
||
skips the real validation/sanitization — so the browser preview can accept settings the app rejects,
|
||
masking M34-class issues during design. *Fixed the version drift: one `MOCK_CURRENT_VERSION` constant in
|
||
[mockApi.ts](src/renderer/src/mockApi.ts) now backs both `getAppVersion` and
|
||
`checkForAppUpdate.currentVersion`, so they agree (kept below the mock "latest" so the updater card still
|
||
demos an available update). The `setSettings`-skips-validation point is left as-is on purpose: the mock is a
|
||
browser-only design aid with no access to main's `settings.ts` validators; replicating them in the mock
|
||
would be a second source of truth to drift. M34/M24 already make the real app authoritative, so the design
|
||
preview being permissive doesn't affect shipped behavior.*
|
||
- [x] **L172 — `skipLibCheck: true`** (toolkit) hides type errors in dependency `.d.ts` files — standard
|
||
practice, low risk, noted for awareness (a dep type regression won't fail typecheck). *Acknowledged —
|
||
kept as-is (it IS standard practice and speeds typecheck); recorded so the trade-off (a dependency
|
||
`.d.ts` regression can pass typecheck silently) is a known, accepted decision rather than an oversight.*
|
||
|
||
---
|
||
|
||
## UI consistency
|
||
|
||
Cross-screen review (Downloads, Library, History, Terminal, Settings, Onboarding, Command palette,
|
||
Sidebar, cookie sign-in window). Each item: what's inconsistent → **Standard:** the single rule to
|
||
adopt. Items prefixed `UI`; where a dimension was already filed, the existing ID is referenced
|
||
instead of re-filing. The app uses Fluent tokens well (theme-awareness is mostly solid); the gaps
|
||
are an undefined spacing/radius/type scale and a drift between Fluent components and hand-rolled ones.
|
||
|
||
### Layout, spacing & width
|
||
|
||
- [x] **UI1 — No shared content width.** SettingsView is `maxWidth: 640px` ([SettingsView.tsx](src/renderer/src/components/SettingsView.tsx)),
|
||
but Downloads/Library/History/Terminal are full-width; Onboarding card is 460px, CommandPalette
|
||
560px. On a wide window Settings is a narrow column while siblings stretch edge-to-edge.
|
||
**Standard:** one reading-width token (e.g. 720px) applied by a shared `Screen` wrapper, or commit to full-width everywhere.
|
||
- [x] **UI2 — Per-screen vertical rhythm differs.** Root section gap is 16 (Settings/Terminal), 20
|
||
(Downloads), 18 (Library), 12 (History). **Standard:** a single section-gap (16px) via a shared screen wrapper.
|
||
*Fixed: all five view roots now use `SPACE.section` (16) — Downloads 20→16, Library 18→16, History 12→16;
|
||
Settings/Terminal were already 16 and are now tokenized.*
|
||
- [x] **UI3 — Card padding has no scale.** 14 (QueueItem) / 16 (DownloadBar) / 20 (Settings card) /
|
||
32 (Onboarding) / `10px 12px` (History row) / `12px 14px` (Library head) / `8px 10px` (template row).
|
||
**Standard:** a padding scale (e.g. control 8–10, list-row 12, card 16, hero 24) keyed to surface tier.
|
||
*Done (Batch 18): the remaining list-row/control paddings migrated to the `SPACE` scale — History row and
|
||
Downloads summary → `snug/cozy`, Library head/detail and Terminal gate/log and the template form → `cozy`
|
||
(12, the list-row tier; the odd 14px horizontals snapped down), template row → `tight/snug`. The earlier
|
||
card-tier deltas (20→16 settings cards, 14→16 queue rows) plus these were eyeballed via the Electron-probe
|
||
screenshot pass (light + dark, Downloads/History/Library/Settings): nothing reads cramped or loose; kept.*
|
||
- [x] **UI4 — Asymmetric app content padding.** `padding: '24px 28px'` ([App.tsx](src/renderer/src/App.tsx)) —
|
||
28px horizontal appears nowhere else. **Standard:** symmetric or scale-based padding. *Fixed: the `<main>`
|
||
content padding is now the symmetric `SPACE.page` (24px).*
|
||
- [x] **UI5 — Action-cluster gaps split 4px vs 8px.** Icon-button rows use 4px (QueueItem/History/
|
||
TemplateManager) but DownloadBar/Library action rows use 8px. **Standard:** 4px for icon-button groups, 8px for labelled-control rows. *(Good baseline already: every empty-state uses `56px 16px` — keep it.)*
|
||
*Resolved: the existing split already matched the standard; it's now named in `SPACE` (`xtight` = 4 for
|
||
icon-button clusters, `tight` = 8 for control rows) and adopted where touched, so the rule is explicit
|
||
rather than incidental.*
|
||
|
||
### Corner radius
|
||
|
||
- [x] **UI6 — Thumbnail radius varies for the same element.** QueueItem `Large`, History `Medium`,
|
||
Library rowThumb `Small`, DownloadBar previewThumb `Medium`. **Standard:** one thumbnail radius (Medium/10px). *(Distinct from L9, which is thumbnail pixel dimensions.)*
|
||
*Fixed: every thumbnail uses `RADIUS.control` (Medium) — QueueItem Large→Medium and Library rowThumb
|
||
Small→Medium; History/DownloadBar were already Medium and are now tokenized.*
|
||
- [x] **UI7 — Card radius (XLarge vs Large) has no stated rule.** Top-level cards use XLarge (16),
|
||
list rows Large (12), but it's applied by feel. **Standard:** document surface tiers — page-card XLarge, list-item Large, control Medium — and apply uniformly.
|
||
*Fixed: `RADIUS` documents the tiers (`card` XLarge / `surface` Large / `control` Medium) and they're
|
||
applied on the surfaces touched this batch (page cards → `card`, the QueueItem list-item → `surface`,
|
||
thumbnails/controls → `control`); remaining correct-but-untokenized `borderRadius*` calls migrate incrementally.*
|
||
- [x] **UI8 — Circular shapes mix token and literal.** `borderRadiusCircular` (Library pill/watchBadge)
|
||
vs literal `'50%'` (SettingsView swatch, History emptyBadge). **Standard:** always the token.
|
||
|
||
### Typography
|
||
|
||
- [x] **UI9 — Screen titles are inconsistent.** Most screens use `<Subtitle2>` (Downloads "Queue",
|
||
Library, Terminal, Settings cards); Onboarding uses `<Title2>`; **History has no title at all**
|
||
(just a count). **Standard:** one page-title style on every screen.
|
||
- [x] **UI10 — Only Library has a screen subtitle/description.** Others jump straight to content.
|
||
**Standard:** a consistent header block (title + optional one-line description).
|
||
- [x] **UI11 — Semantic type ramp vs ad-hoc px.** Sidebar `brandName`/`navItem` and Library
|
||
`watchBadge`/`pill` set raw `fontSizeBaseXXX`; `sectionIcon`/`mark` use literal `'20px'`/`'26px'`.
|
||
**Standard:** Fluent type components/tokens; no literal px font sizes. *Fixed: no literal px font sizes
|
||
remain — the last tile/badge glyphs snap to `ICON` tiers (Sidebar mark 20, Onboarding mark 24, Library
|
||
srcIcon 20, History emptyBadge 24). The `fontSizeBase*` usages already met the standard (they're tokens, on
|
||
interactive `<button>`/pill elements that can't be Fluent type components). The four ≤2px tile-glyph deltas
|
||
join UI12's watched visual pass.*
|
||
|
||
### Icons
|
||
|
||
- [x] **UI12 — No icon-size scale.** Inline `fontSize` of 11/14/16/18/20/22/26/28/40 is scattered
|
||
across components. **Standard:** a small set (16 inline / 20 control / 24 section / 40 empty-state).
|
||
*Done (Batch 18): the `ICON` = 16/20/24/40 tiers were already in place; the deferred eyeball of the shifted
|
||
glyphs (18→20 sidebar nav/theme toggles, 20→24 settings card headers, 14→16 private badge) happened in the
|
||
Electron-probe screenshot pass — the nav icons sit balanced in their 20px tier, the card-header glyphs read
|
||
as proper section markers at 24, nothing looks oversized. Box-proportional glyphs (brand/source tiles,
|
||
`watchBadge`, thumbnail placeholders) remain documented exclusions.*
|
||
- [x] **UI13 — Regular vs Filled weight mixed for one concept.** `ArrowDownloadRegular` (nav, buttons)
|
||
vs `ArrowDownloadFilled` (brand mark). **Standard:** one weight per concept (Filled only for brand/emphasis).
|
||
*Verified: `ArrowDownloadFilled` appears only on the two brand marks (Sidebar, Onboarding); every other
|
||
download arrow (nav, buttons, `contentBefore`) is `ArrowDownloadRegular`. The convention already held — the
|
||
only Filled glyphs elsewhere are the status Checkmark/Error circles, which are deliberate state emphasis.*
|
||
|
||
### Buttons & controls
|
||
|
||
- [x] **UI14 — Two hand-rolled segmented controls.** DownloadBar kind toggle (`segment`, padding
|
||
`7px 16px`) and Sidebar theme toggle (`themeSeg`, padding `7px 4px`) are separate implementations
|
||
with different padding/markup. **Standard:** one shared `SegmentedControl`.
|
||
- [x] **UI15 — Raw `<button>`s alongside Fluent `<Button>`.** Sidebar navItem/iconBtn/themeSeg,
|
||
DownloadBar segment/plKindBtn, CommandPalette item, and Library `groupHead` (a `role="button"` div)
|
||
are bespoke. **Standard:** wrap recurring patterns so hover/focus/disabled are uniform (ties to L5/M15).
|
||
- [x] **UI16 — Button size hierarchy isn't applied uniformly.** DownloadBar `large`; Library "Index"
|
||
`large` but its toolbar `small`; History all `small`; Settings mostly default. **Standard:** size-by-role (screen primary = large; secondary = medium; row actions = small).
|
||
*Done (Batch 18): the usage the item lists already matches the size-by-role rule (each screen's one primary
|
||
is `large` — Download/Add-to-queue, Library Index; row/toolbar actions `small`; settings cards default) —
|
||
confirmed screen-by-screen in the screenshot pass, no tweaks needed. The convention is now codified in
|
||
[ui/tokens.ts](src/renderer/src/components/ui/tokens.ts) alongside the other scales so it's a rule, not an
|
||
accident.*
|
||
- [x] **UI17 — `appearance="secondary"` used in only two spots** (Library "Check for new", Terminal
|
||
"Stop") while every other non-primary button is `subtle`. **Standard:** pick subtle *or* secondary as the standard non-primary appearance.
|
||
*Fixed: both switched to `subtle` (the app's standard non-primary appearance — used by every other
|
||
non-primary button).*
|
||
- [x] **UI18 — Two status-chip systems.** QueueItem uses Fluent `Badge`; LibraryView uses custom
|
||
color `pill` spans for the same item-status concept (and labels differ — see M8). **Standard:** one shared status-chip component + label map.
|
||
- [x] **UI19 — Suggestion-banner CSS duplicated.** Identical `suggestion`/`suggestionText` styles in
|
||
DownloadBar and LibraryView. **Standard:** a shared `LinkSuggestion` component (also kills drift).
|
||
*Fixed: a shared [Banner](src/renderer/src/components/ui/Banner.tsx) (leading icon + truncating/`wrap`
|
||
content + trailing actions, `tone` = brand/warning) and a [LinkSuggestion](src/renderer/src/components/ui/LinkSuggestion.tsx)
|
||
built on it. The DownloadBar's copied-link suggestion and the Library's are now one `<LinkSuggestion>`; the
|
||
DownloadBar's channel/playlist nudge (`wrap`) and duplicate warning (`tone="warning"`) reuse the base
|
||
`Banner`. Five hand-rolled style blocks (`suggestion`/`suggestionText`/`channelHint`/`channelHintText`/
|
||
`dupRow`) are deleted — the tint/padding/radius are identical, so no visual change.*
|
||
|
||
### Colors & selection
|
||
|
||
- [x] **UI20 — "Selected/active" styling is split.** Solid brand (`colorBrandBackground` +
|
||
on-brand text) for DownloadBar/Sidebar segments, but brand-tint (`colorBrandBackground2` +
|
||
`colorBrandForeground2`) for Sidebar nav, CommandPalette item, Library pill. **Standard:** one active treatment per control class.
|
||
*Resolved (rule named + verified): one active treatment **per control class** — **toggles** (the shared
|
||
`SegmentedControl`, used by both the DownloadBar kind toggle and the Sidebar theme switch after UI14) use
|
||
**solid brand**; **selection within a list** (Sidebar nav item, CommandPalette active option) uses
|
||
**brand-tint**. Each class is now internally consistent, and the rule is documented on
|
||
[SegmentedControl](src/renderer/src/components/ui/SegmentedControl.tsx). The "Library pill" the item cited is
|
||
gone — UI18 replaced it with the shared `StatusChip`, so there's no third treatment left to reconcile.*
|
||
- [x] **UI21 — Sidebar active-nav rail is expanded-only.** The inset brand box-shadow shows only when
|
||
expanded; collapsed relies on tint alone. **Standard:** a consistent active indicator in both states.
|
||
*Fixed (Batch 18): the inset rail moved from a conditional inline style into `navItemActive` itself, so it
|
||
shows in both states. Screenshot-verified collapsed: the 3px brand rail hugs the row's left edge and reads
|
||
clearly beside the active-download `CounterBadge` pinned at the icon's top-right — no collision.*
|
||
- [x] **UI22 — Brand-icon tiles differ.** `mark` (on-brand on solid brand) vs `srcIcon`
|
||
(brandForeground2 on brand-tint) vs `sectionIcon` (compoundBrand, no tile). **Standard:** a defined icon-emphasis set.
|
||
*Resolved (the set is now defined + documented): three deliberate emphasis levels — **brand mark** (`mark`,
|
||
on-brand glyph on solid brand: the app/onboarding identity, highest emphasis), **entity tile** (`srcIcon`,
|
||
brandForeground2 on brand-tint: a Library source, medium), and **section glyph** (`sectionIcon`,
|
||
compoundBrand, no tile: a settings-card header, lowest). They *should* differ — one per role; the levels are
|
||
now named rather than incidental, and each is applied consistently.*
|
||
|
||
### Dialogs, menus, status, navigation
|
||
|
||
- [x] **UI23 — Fragmented overlay/dialog system.** Native OS dialogs (pickers, backup save/open,
|
||
backup warning `messageBox`), one custom React overlay (CommandPalette), a full-screen view
|
||
(Onboarding), and an **unstyled separate `BrowserWindow`** (cookie sign-in) that shows none of the
|
||
app's theme/chrome. **Standard:** keep native for file/confirm; unify in-app overlays under one
|
||
themed primitive; theme the sign-in window's title/background to match.
|
||
*Fixed (Batch 20, the actionable remainder): the cookie sign-in window — and the PO-token window, its
|
||
sibling on the same login partition — now open with `backgroundColor` set to the app's resolved theme
|
||
(`PAGE_BACKGROUND[resolveBackgroundMode(...)]`, supplied by the IPC layer to avoid an import cycle), so
|
||
neither flashes default-white chrome before the remote page paints; their title bars already follow the
|
||
app theme via the W3 `nativeTheme.themeSource` bridge. In-app overlays were already unified on the
|
||
non-portal `Z`/`SCRIM`/`ELEVATION` tokens; native file/confirm dialogs stay native by design.*
|
||
- [x] **UI24 — No context menus anywhere.** Every row exposes actions only as inline buttons;
|
||
right-click does nothing on any screen, despite many per-row actions that conventionally also live on
|
||
right-click. **Standard:** add consistent right-click menus mirroring row actions, or note "none by design."
|
||
*Resolved as "none by design" (documented): every row action is already a visible inline button with a
|
||
tooltip, and lists support keyboard operation (Delete, Ctrl+A). Fluent's `Menu` is **portal-based**, and
|
||
this app deliberately avoids portal overlays — they blank/flicker on the dev-machine GPU (the same reason
|
||
Select/CommandPalette/Toaster are hand-rolled non-portal; see the gpu-flicker note). A hand-rolled
|
||
non-portal context-menu primitive is a sizable build with little to add over the always-visible actions, so
|
||
right-click menus are intentionally not provided. Revisit if the GPU/portal issue is resolved.*
|
||
- [x] **UI25 — No in-app global status surface.** Queue progress shows only on the Downloads tab's
|
||
summary strip; on any other tab there's no in-app sign downloads are running (only the OS taskbar).
|
||
**Standard:** a persistent affordance (e.g. a sidebar "Downloads" badge with the active count).
|
||
*Fixed (with UX9): a `CounterBadge` on the sidebar Downloads nav item shows the active (downloading +
|
||
queued) count from every tab, and a new in-app toast surface covers transient status. The exact
|
||
suggested affordance.*
|
||
|
||
### Animations & transitions
|
||
|
||
- [x] **UI26 — Lone animation.** Only the sidebar width transitions (`0.15s ease`); tab switches,
|
||
panel expand/collapse, banners, and Library card expansion are all instant. **Standard:** one motion
|
||
policy — either add subtle, consistent transitions (gated on `prefers-reduced-motion`) or drop the sidebar one.
|
||
*Fixed (policy established): the sidebar transition now uses the shared `MOTION` timing (durationFast +
|
||
curveEasyEase), and a global `@media (prefers-reduced-motion: reduce)` rule in base.css disables every
|
||
transition/animation for users who ask for less. New transitions adopt `MOTION`; none were added
|
||
speculatively (compositing overlays flicker on the dev GPU — see the gpu-flicker note).*
|
||
|
||
### Hover, focus, disabled, keyboard & accessibility
|
||
|
||
- [x] **UI27 — Command palette uses JS hover, not CSS.** Items highlight via `onMouseEnter` setting
|
||
`itemActive` (no `:hover`), unlike every other list, and expose no `aria-selected`/active-descendant,
|
||
so the highlight is invisible to screen readers. **Standard:** CSS `:hover` + listbox/option semantics.
|
||
*Fixed in [CommandPalette.tsx](src/renderer/src/components/CommandPalette.tsx): the input is now a
|
||
`role="combobox"` with `aria-controls`/`aria-activedescendant`/`aria-autocomplete="list"`, the list is a
|
||
`role="listbox"`, and each row is a `role="option"` with `aria-selected` — so the active row is announced
|
||
to Narrator via active-descendant (the standard combobox pattern; focus stays on the input, so the rows
|
||
became non-focusable `div`s rather than `button`s). `onMouseEnter` is kept deliberately: it unifies the
|
||
pointer and keyboard highlight onto one `sel` state, which — now that `aria-activedescendant` tracks it —
|
||
is more correct than a pure CSS `:hover` (which wouldn't move the announced selection).*
|
||
- [x] **UI28 — Command palette input has no focus ring.** The search `<input>` sets `outline: 'none'`
|
||
with no replacement. **Standard:** a visible focus ring (Fluent stroke token). *(a11y)*
|
||
- [x] **UI29 — Fragmented focus treatment.** Fluent ring (Fluent controls) vs custom border (Select)
|
||
vs removed (palette input) vs UA default (Sidebar buttons, segments, and Library `cardHead`/`groupHead`
|
||
which are `role="button" tabIndex=0` with **no `:focus-visible`** → invisible keyboard focus).
|
||
**Standard:** one focus-ring style on all interactive elements, custom and native. *(a11y; extends L14)*
|
||
- [x] **UI30 — Hand-rolled radiogroups aren't arrow-navigable.** DownloadBar kind and Sidebar theme
|
||
use `role="radiogroup"`/`radio` but implement click only — no ←/→ roving focus a radiogroup implies.
|
||
**Standard:** roving-tabindex arrow keys, or Fluent's RadioGroup. *(a11y)*
|
||
- [x] **UI31 — `Select` can't be disabled.** The native-`<select>` wrapper exposes no `disabled` prop,
|
||
so that control can't show a disabled state while Fluent controls can. **Standard:** add `disabled` + styling.
|
||
*Fixed in [Select.tsx](src/renderer/src/components/Select.tsx): a `disabled` prop passes through to the
|
||
native `<select>` and applies a disabled style (disabled foreground/background/stroke tokens + `not-allowed`
|
||
cursor, hover suppressed) so it reads like a disabled Fluent control.*
|
||
- [x] **UI32 — Native-control dark mode is uneven.** The DownloadBar `datetime-local` sets
|
||
`colorScheme: 'light dark'` explicitly; the `Select` relies on the root `colorScheme`. **Standard:** set `colorScheme` consistently on both.
|
||
*Fixed in [downloadBar/styles.ts](src/renderer/src/components/downloadBar/styles.ts): dropped the explicit
|
||
`colorScheme: 'light dark'` on the `datetime-local` so it inherits the resolved in-app scheme set on the app
|
||
root (App.tsx) — the native calendar popup now follows the app's Light/Dark theme, consistent with the
|
||
native `<Select>`, rather than tracking the OS preference.*
|
||
- [x] **UI33 — No semantic headings.** Titles render as `Subtitle2`/`Title2` (styled spans), so there's
|
||
no h1–h6 hierarchy for screen-reader heading navigation (landmarks `<nav>`/`<main>` exist; headings
|
||
don't). **Standard:** render titles as real headings (`as="h1"`/`"h2"` or `role="heading" aria-level`). *(a11y)*
|
||
*Fixed by rendering the Fluent typography titles as real headings (the `as` prop keeps the visual ramp):
|
||
the shared [ScreenHeader](src/renderer/src/components/ui/Screen.tsx) title is now `<Subtitle2 as="h1">`,
|
||
giving every screen (Downloads, History, Library, Settings, Terminal) one `h1` in one place; the
|
||
DownloadsView "Queue (N)" and all 11 settings cards are `<Subtitle2 as="h2">` sections beneath it; and the
|
||
Onboarding "Welcome" title is `<Title2 as="h1">`. A minimal `h1–h6` margin reset in
|
||
[base.css](src/renderer/src/assets/base.css) zeroes the UA heading margin (Fluent's typography classes
|
||
out-specify the element selector, so the visual ramp is unchanged) so the layout matches the former spans.*
|
||
|
||
### Top cohesion-breakers (the "not one product" shortlist)
|
||
|
||
1. **Status chips** — Fluent `Badge` (Downloads) vs custom color pills (Library) for the same concept (UI18, M8).
|
||
2. **Segmented controls** — two separate hand-rolled versions (UI14).
|
||
3. **Buttons** — Fluent `<Button>` vs many bespoke `<button>`s with divergent hover/focus (UI15, UI29).
|
||
4. **Content width** — Settings is a 640px column; every other screen is full-width (UI1).
|
||
5. **Focus rings** — four different focus treatments incl. one removed and several invisible (UI28–UI29).
|
||
6. **Screen headers** — Subtitle2 / Title2 / none (UI9–UI10).
|
||
|
||
---
|
||
|
||
## UX review (first-run perspective)
|
||
|
||
Walking the app as a new user. Ranked by severity. `UX` IDs; existing IDs referenced where the
|
||
root cause is already filed.
|
||
|
||
### High — core-flow friction
|
||
|
||
- [x] **UX1 — Per-download options are unreachable; you must change global Settings for one video.**
|
||
The DownloadBar exposes only URL · Video/Audio · quality · Trim · Schedule. There is **no**
|
||
per-download post-processing panel, custom-command panel, command preview, or incognito toggle —
|
||
so `DownloadItem.options`/`extraArgs` and the whole Phase A/C per-download-override plumbing are
|
||
unreachable; every download uses `settings.downloadOptions`. To grab one video as MP3-with-subs a
|
||
user must edit global Settings, download, then revert. (Supersedes/expands M5, M6.) **Fix:** add a
|
||
collapsible per-download Options panel (reuse `DownloadOptionsForm`) — the plumbing already exists.
|
||
*Fixed: a collapsible "Advanced" panel in [DownloadBar.tsx](src/renderer/src/components/DownloadBar.tsx)
|
||
reuses `DownloadOptionsForm` for per-download overrides (audio format, container, codec, format-sort,
|
||
subtitles, SponsorBlock), passed via `AddOptions.options` → `startDownload`. The panel also carries the
|
||
M6 incognito checkbox and sits beside the M5 command preview. Overrides are one-shot: they reset to the
|
||
global defaults after each download (matching Trim/Schedule).*
|
||
- [x] **UX2 — "Fetch" vs "Download" is ambiguous, and Enter does the wrong thing.** Two unlabeled
|
||
icon buttons (magnifier = "Fetch", clipboard = "Paste") sit next to "Download"; a first-timer can't
|
||
tell that Download works without Fetch. Pressing **Enter** in the URL field runs a *probe*, not the
|
||
download (L27) — violating "type URL + Enter = go". **Fix:** label the action, make Enter download
|
||
(or fetch-then-download), and present Fetch as optional ("Preview formats").
|
||
*Fixed in [DownloadBar.tsx](src/renderer/src/components/DownloadBar.tsx): **Enter is now "go"** — it
|
||
starts the download (or adds the fetched playlist when the playlist panel is open) instead of probing;
|
||
probing stays an explicit, optional click. The magnifier button is relabeled from "Check URL" to
|
||
**"Preview available formats (optional)"** (both its tooltip and `aria-label`), and the URL placeholder
|
||
now reads "Paste a video or playlist URL, then press Enter to download" so it's clear Download works
|
||
without Fetch. `download()` already enqueues with the presets when nothing has been probed, so the
|
||
Enter-downloads path needs no probe. (resolves L27's Enter inconsistency too.)*
|
||
- [x] **UX3 — Library "Index" is jargon and a hidden two-stage workflow.** Pasting a channel URL in
|
||
the Downloads bar tries to treat it as one item; whole channels belong in the Library tab, but
|
||
nothing signposts that. "Index" reads like "Download" but only catalogs — you must then expand,
|
||
select, and Download. **Fix:** rename to "Add channel/playlist," and after indexing surface a clear
|
||
"Download N videos" next step; detect channel URLs in the Downloads bar and suggest the Library.
|
||
*Fixed across three seams. **(1) De-jargoned the Library** ([LibraryView.tsx](src/renderer/src/components/LibraryView.tsx)):
|
||
the primary button is now **"Add"** (was "Index"), "Re-index" → **"Refresh"**, and the header
|
||
description / empty state / placeholder / error copy drop "index" for "add". The post-add next step
|
||
already exists — `indexSource` auto-selects the new source so its card expands with a prominent
|
||
"Download N pending" primary button. **(2) Channel detection in the Downloads bar** — a new pure
|
||
[`looksLikeChannelOrPlaylist`](src/renderer/src/lib/urlHelpers.ts) (conservative: only clear YouTube
|
||
channel/dedicated-playlist shapes; a `/watch` video, even with a `list=`, is never flagged and keeps
|
||
working in the bar) drives a dismissible brand-tinted nudge in
|
||
[DownloadBar.tsx](src/renderer/src/components/DownloadBar.tsx): "This looks like a channel or playlist.
|
||
Add it in the Library…" with an **Open in Library** action. **(3) The handoff** goes through a small
|
||
new [store/nav.ts](src/renderer/src/store/nav.ts) (App's tab state moved into it) that carries a
|
||
one-shot `pendingLibraryUrl`; the Library consumes it on mount to pre-fill its add field. Unit-tested
|
||
(`looksLikeChannelOrPlaylist` in `test/clipboardLink.test.ts`) and verified end-to-end in the browser
|
||
preview (channel URL → nudge → Open in Library → Library tab with the URL pre-filled).*
|
||
- [x] **UX4 — Destructive actions have no confirmation and no undo** (L13). "Clear history,"
|
||
"Clear log," "Remove source" (deletes an entire indexed channel + items), and "Delete selected"
|
||
are all one click. A user exploring can wipe data irreversibly. **Fix:** confirm destructive/bulk
|
||
deletes (or offer an undo toast).
|
||
- [x] **UX5 — First run can't choose the download folder.** [Onboarding.tsx](src/renderer/src/components/Onboarding.tsx)
|
||
only *describes* Documents\Video/Audio — there's no picker (the ROADMAP claims one). The user must
|
||
later discover Settings → Downloads. **Fix:** put the folder picker in onboarding as documented.
|
||
*Fixed: the "Where downloads go" block in [Onboarding.tsx](src/renderer/src/components/Onboarding.tsx)
|
||
now has interactive Video and Audio rows — each shows the current folder (or the `Documents\Video` /
|
||
`Documents\Audio` default) with a **Choose…** button that opens the OS picker via the settings store's
|
||
`chooseDir` (the same proven path DownloadsCard uses, seeded at the current value — W5). Picking a
|
||
folder persists immediately, so the choice is already saved when the user clicks "Get started";
|
||
leaving it untouched keeps the Documents defaults. Verified in the browser preview with onboarding
|
||
forced on.*
|
||
|
||
### Medium — confusion, feedback & organization
|
||
|
||
- [x] **UX6 — Silent failures on file actions.** `openFile`/`showInFolder` in the stores call
|
||
`window.api.openPath(...)` and ignore the returned error string; [reveal.ts](src/main/reveal.ts)
|
||
refuses missing/moved files or disallowed types and returns an error nobody surfaces. Clicking
|
||
"Open file" on a moved download does nothing, with no message. **Fix:** surface open/reveal errors.
|
||
*Fixed: a shared renderer helper [reveal.ts](src/renderer/src/reveal.ts) `revealFile('open'|'folder', path)`
|
||
awaits the IPC result and raises the main-process reason ("File not found — it may have been moved…",
|
||
"Refusing to open this file type.") as an error **toast** — the new transient-notification surface (see
|
||
UX9). Both the downloads and history stores route through it. `safeShowInFolder` was upgraded to return an
|
||
error string like `safeOpenPath` (it previously no-op'd silently), so reveal failures surface too.*
|
||
- [x] **UX7 — Settings is one long unsegmented scroll** of ~11 cards with no section nav/anchors; the
|
||
only finder is the hide-cards search (M14). Order is arbitrary and related items are split.
|
||
**Fix:** group into sub-sections (or a left rail) with a stable, logical order.
|
||
*Fixed (Batch 19): the 11 cards are grouped into five stable sections — Downloads (defaults +
|
||
post-processing + filenames), Appearance, Network & accounts, Advanced, App & maintenance — each with a
|
||
section heading, plus a jump nav under the search box that smooth-scrolls to a section (anchors work inside
|
||
the screen's own scroll shell from L161; `scrollMarginTop` lands the heading cleanly). The search filter
|
||
still works per-card and now hides a whole section when all its cards are filtered out; the jump nav hides
|
||
while searching. Heading hierarchy corrected (page h1 → section h2 → card h3). Verified in the preview
|
||
probe: nav renders, jumping lands on "App & maintenance", searching "proxy" leaves exactly the matching
|
||
cards + their section headings visible (screenshots).*
|
||
- [x] **UX8 — "Default format" vs "Format & post-processing" are two places for one concept.** Kind +
|
||
quality live under Downloads; container/codec/subs/SponsorBlock live in a separate card. **Fix:**
|
||
co-locate, or clearly label one "defaults" and the other "advanced post-processing."
|
||
*Fixed (Batch 19, both halves of the fix): the cards are now **co-located** — the post-processing card sits
|
||
directly under the Downloads card in the new Downloads section (UX7) — **and** clearly labeled: the card is
|
||
renamed "Post-processing" with a hint stating it's the advanced conversion half and that the default format
|
||
itself is set in the Downloads card above.*
|
||
- [x] **UX9 — No global completion feedback off the Downloads tab** (UI25). If OS notifications are
|
||
off/suppressed (focus assist) and you're on another tab, a finished download gives no in-app sign.
|
||
**Fix:** a sidebar Downloads badge / lightweight in-app toast.
|
||
*Fixed (both halves of the suggested fix): a **sidebar Downloads badge** — a Fluent `CounterBadge` on the
|
||
Downloads nav item shows the active (downloading + queued) count from any tab
|
||
([Sidebar.tsx](src/renderer/src/components/Sidebar.tsx)), driven by a count-only selector in App so it
|
||
updates without re-rendering on every progress tick; and a **lightweight in-app toast** surface
|
||
([Toaster.tsx](src/renderer/src/components/ui/Toaster.tsx) + [store/toasts.ts](src/renderer/src/store/toasts.ts)),
|
||
a non-portal bottom-center stack that any store/component can raise (now used for UX6's file-action errors).
|
||
Together this closes **UI25**.*
|
||
- [x] **UX10 — Re-download silently changes quality** (H5) — a confusing "I asked for 720p, got Best." *Resolved by H5 (re-download stores `formatId`/`formatHasAudio` and strips compound quality labels).*
|
||
- [x] **UX11 — Scheduled downloads silently never fire if the app is quit** (M4). The hint is easy to
|
||
miss; the user returns to find nothing happened. **Fix:** warn at schedule time that it requires the
|
||
app running (or persist + relaunch). *Resolved by M4 (the "persist + relaunch" option the fix names): the
|
||
queue is now persisted, so a `saved`+`scheduledFor` item survives a quit and the App promoter tick fires it
|
||
on the next launch once its time has passed (the store normalizes/promotes due items). So a scheduled
|
||
download no longer "never fires" if quit — it fires when due, or on the next launch if that time already
|
||
passed. (Live-smoke of the quit→relaunch cycle noted in M4.)*
|
||
- [x] **UX12 — Terminal is a dead-end when custom commands are off** (L18). The nav item is always
|
||
visible; clicking it shows a gate pointing to a setting on another screen. **Fix:** hide/disable the
|
||
nav item, or let the gate enable the setting inline. *Fixed (both fixes): the nav item is already hidden
|
||
when custom commands are off (Sidebar's `showTerminal` filter, L18), and the gate — still reachable via the
|
||
command palette — now carries an inline **"Enable custom commands"** button that flips the setting on the
|
||
spot ([TerminalView.tsx](src/renderer/src/components/TerminalView.tsx)), so it's no longer a dead-end that
|
||
sends you to another screen.*
|
||
- [x] **UX13 — Backup export warns of nothing; it contains secrets** (M22). A user "backs up settings"
|
||
and ships proxy creds / tokens in cleartext. **Fix:** warn, or mask/omit secrets. *Resolved by M22
|
||
(backup strips proxy / PO-token / update-token; the caption states credentials aren't included).*
|
||
- [x] **UX14 — Duplicate warning can show a placeholder title** (L76): 'Already in your queue:
|
||
"YouTube video (abc123)"' before metadata resolves looks broken. **Fix:** fall back to the URL.
|
||
*Resolved by L76 (the dup warning falls back to the URL when the title is still the placeholder).*
|
||
- [x] **UX15 — No bulk actions in the Downloads queue.** History has a select-mode + bulk delete;
|
||
Downloads has only per-row remove + "Clear finished." Inconsistent. **Fix:** mirror select/bulk, or
|
||
add "Cancel all." *Fixed (the "Cancel all" option): a `cancelAll` store action + a "Cancel all (N)"
|
||
header button ([DownloadsView.tsx](src/renderer/src/components/DownloadsView.tsx)) that cancels every
|
||
active (downloading + queued) item at once — killing the live processes and freeing the queue — shown only
|
||
when there's something cancelable. (A full History-style select-mode is the heavier alternative; "Cancel
|
||
all" covers the common bulk need.)*
|
||
|
||
### Low — polish & smaller friction
|
||
|
||
- [x] **UX16 — URL field isn't focused on launch.** `aerofetch-url` is only focused via the command
|
||
palette; opening the app or the Downloads tab requires a click to start typing. **Fix:** autofocus it.
|
||
*Fixed: an `App` mount effect focuses `#aerofetch-url` on launch when the Downloads tab is active
|
||
(double-rAF for paint, reusing the command palette's focus pattern).*
|
||
- [x] **UX17 — Icon-only Fetch/Paste buttons** rely on hover tooltips for meaning (UI a11y; new users on touch/keyboard miss them). **Fix:** labels or `aria`-described affordances.
|
||
*Resolved (a11y met; verified): both buttons already carry an `aria-label` (Fetch = "Preview available
|
||
formats (optional)", Paste = "Paste from clipboard") **and** a `Hint` tooltip that shows on hover *and*
|
||
keyboard focus — so screen-reader and keyboard users get the name; the icon isn't the only affordance. A
|
||
permanently-visible text label is a deliberate compact-layout tradeoff for the URL row (the Download button
|
||
beside it carries the primary text label), so it's left as a visual choice, not an a11y gap.*
|
||
- [x] **UX18 — The quality control morphs after Fetch** (preset list → real formats; label "Quality" →
|
||
"Quality / format") in place — a surprising transform. **Fix:** keep a stable control with a clear "formats loaded" state.
|
||
*Fixed (Batch 18): the label stays "Quality" in both states; a small brand-colored "• fetched" tag appears
|
||
beside it once real formats load, so the state change is signalled without the control or its label
|
||
morphing. Verified in the preview probe: Fetch on a URL loads the mock formats and the tag + format list
|
||
render as designed (screenshot).*
|
||
- [x] **UX19 — Window size/position isn't remembered.** [index.ts](src/main/index.ts) opens a fixed
|
||
920×700 every launch (no bounds persistence). **Fix:** persist + restore window bounds. *Fixed with W2 —
|
||
[windowState.ts](src/main/windowState.ts) persists + restores bounds/maximized with an off-screen guard.*
|
||
- [x] **UX20 — "App is still running" surprise.** Closing with a download active hides to tray and
|
||
notifies once per process (L68); later closes give no hint, so the window "won't close" reads as a
|
||
bug. **Fix:** a persistent tray hint / first-close explainer. *Resolved by L68 (the "first-close explainer"
|
||
it now is on *every* close): the once-per-process guard was the whole bug — `notifiedBackground` now resets
|
||
when the window is shown again, so each close-to-tray with a download active (or tray mode) re-fires the
|
||
"AeroFetch is still running in the tray" notification, not just the first. So a later close no longer looks
|
||
like the window is stuck. (The tray behavior itself is worth a watched confirm in the Batch 15 live pass.)*
|
||
- [x] **UX21 — No keyboard-shortcut discoverability.** Ctrl+K (palette), Enter, Ctrl+Enter are
|
||
undocumented; there's no "?"/shortcuts screen. **Fix:** a shortcuts hint or help affordance.
|
||
*Fixed (discoverability): the first-run Onboarding now includes a tip — "Press Ctrl+K anytime to jump to any
|
||
screen or start a new download." — so new users learn the palette, which is itself the discovery surface for
|
||
the rest; and the palette's Settings action shows its shortcut ("Ctrl ,") in the hint column. (A dedicated
|
||
full shortcuts/"?" screen is the heavier alternative, left for later.)*
|
||
- [x] **UX22 — Onboarding is shallow and one-shot** (L67): three tips, no folder choice, not
|
||
revisitable; the clipboard tip requires a focus event a first-timer won't trigger. *Fixed the "one-shot"
|
||
core: a **"Show welcome tips again"** button in Settings → About re-opens the welcome/onboarding screen
|
||
on demand (sets `hasCompletedOnboarding: false`), so it's revisitable (closes **L67**'s revisit half).
|
||
Onboarding does offer folder choice already (Video/Audio "Choose…"), and it gained the Ctrl+K tip (UX21).
|
||
The remaining "shallow" (only a few tips) is a content judgment left as-is.*
|
||
- [x] **UX23 — Settings folder paths are read-only** (L71) — no paste; Browse-dialog only.
|
||
*Resolved (Batch 19) — stale duplicate of L71, which was already fixed: the Video/Audio folder inputs are
|
||
editable (typed or pasted paths commit through the settings store), main validates via `isSafeOutputDir`
|
||
(unit-tested in validation.test.ts), and the M34 reconcile means a rejected path never sticks in the UI.
|
||
Verified editable in the Batch 19 preview probe.*
|
||
- [x] **UX24 — "Check N watched for new" is disabled with no explanation** when no source is watched —
|
||
a user with indexed (but unwatched) sources sees a dead button. **Fix:** tooltip / enable with a hint to watch a source.
|
||
*Fixed: the button uses `disabledFocusable` (not `disabled`) when nothing is watched, so it stays
|
||
hover/focus-reachable, and a `Hint` explains why — "Turn on 'Watch' for a channel or playlist below first,
|
||
then this checks them for new uploads." ([LibraryView.tsx](src/renderer/src/components/LibraryView.tsx)).*
|
||
- [x] **UX25 — Empty-state copy mismatches the flow** (L78): Downloads says "Paste a URL above to get
|
||
started," but the actual path is Fetch/Download. *Resolved by L78 (DownloadsView empty-state copy
|
||
rewritten to match the Fetch/Download flow).*
|
||
- [x] **UX26 — yt-dlp/ffmpeg version + initial settings load have no skeleton/spinner** — brief blank
|
||
states on boot (plus the documented one-frame theme flash). **Fix:** lightweight loading states.
|
||
*Fixed (Batch 18, with L109): the About versions show skeleton lines during the boot load, and SettingsView
|
||
renders three skeleton cards until the settings store's `loaded` flag flips — no more flash of fallback
|
||
values. (The one-frame theme flash remains separately documented under SR1.)*
|
||
|
||
---
|
||
|
||
## Windows platform conventions
|
||
|
||
Review against Windows 11 / Fluent desktop conventions. `W` IDs are new; existing IDs are referenced
|
||
where a dimension is already filed. The app gets the big things right (native dialogs, per-user
|
||
no-admin install, AppUserModelID, tray, taskbar progress, jump list, `nativeTheme` dark-mode follow,
|
||
DPI handled by Chromium). The deviations:
|
||
|
||
### Window behavior & resizing
|
||
|
||
- [x] **W1 — No minimum window size.** `createWindow` sets `width/height` but no `minWidth`/`minHeight`
|
||
([index.ts](src/main/index.ts)), so the window can be dragged down to a few pixels and the layout
|
||
(212px sidebar + content) breaks. **Standard:** set a sensible min (e.g. 640×480).
|
||
- [x] **W2 — Window placement isn't persisted** (extends UX19): size, position, **maximized state**, and
|
||
**which monitor** are all forgotten — it reopens 920×700 on the primary display every launch.
|
||
**Standard:** persist & restore window placement per Windows app convention (e.g. `electron-window-state`).
|
||
*Fixed (no new dep): [windowState.ts](src/main/windowState.ts) persists the window's normal bounds +
|
||
maximized flag to `<userData>/window-state.json`; `createWindow` opens from the saved state (debounced
|
||
save on resize/move + a save on close, which may only hide to tray). A `screen.getAllDisplays()`
|
||
visibility check falls back to the default centered size when the saved monitor is gone, so the window
|
||
can never restore off-screen. (The restore cycle itself wants a live quit→relaunch confirm.)*
|
||
- [x] **W3 — Title bar doesn't follow the in-app theme.** `nativeTheme.themeSource` is deliberately never
|
||
set (index.ts:97), so the OS-drawn caption follows the *OS* theme. Choosing an explicit in-app **dark**
|
||
theme on a **light** OS leaves a light title bar on a dark app (and vice-versa). **Standard:** set
|
||
`nativeTheme.themeSource` to the resolved mode, or use `titleBarOverlay` with themed colors.
|
||
|
||
### Dialogs & file pickers
|
||
|
||
- [x] **W4 — Text fields have no Cut/Copy/Paste context menu.** Electron adds no default editing menu and
|
||
the app registers no `context-menu` handler, so right-clicking any input/textarea (URL, search, terminal
|
||
args, template fields) shows nothing — a basic Windows text-editing affordance is missing. **Standard:**
|
||
wire a standard editing context menu (and spellcheck suggestions for textareas).
|
||
- [x] **W5 — File pickers don't open at the current value.** `chooseFolder` sets no `defaultPath`, so the
|
||
folder dialog opens at a default location instead of the currently-configured Video/Audio folder.
|
||
**Standard:** seed the picker with the current path. *(Native dialogs themselves are correct — good.)*
|
||
- [x] **W6 — Cookie sign-in is a separate taskbar window.** `openCookieLoginWindow` creates a
|
||
`BrowserWindow` with no `parent`/`modal` ([cookies.ts](src/main/cookies.ts)), so it appears as a second
|
||
AeroFetch taskbar button rather than a child/modal dialog. **Standard:** `parent: mainWindow` (+ `modal`
|
||
if appropriate) so it groups under the app.
|
||
|
||
### Keyboard, context menus & focus
|
||
|
||
- [x] **W7 — Lists have no keyboard navigation.** The queue, history, and library lists can't be arrowed
|
||
through, **Delete** doesn't remove the focused/selected item, and there's no **Ctrl+A** select-all —
|
||
all standard Windows list behaviors. **Standard:** roving focus + Delete/Ctrl+A on list surfaces. *(a11y; see also UI30)*
|
||
- [x] **W8 — No standard accelerators.** No **Ctrl+,** (Settings), **F1** (help), or **F5** (refresh);
|
||
Ctrl+K (palette) is the only global shortcut and is undiscoverable (UX21). **Standard:** add the
|
||
conventional accelerators + a discoverable shortcut list.
|
||
- [x] **W9 — Focus isn't restored or advanced.** Closing the command palette doesn't return focus to the
|
||
trigger; finishing onboarding and switching tabs don't move focus into the new content. **Standard:**
|
||
restore focus on overlay close; move focus to the activated panel. *(extends UI28–UI29)*
|
||
- [ ] **(ref) Context menus absent app-wide** — UI24; the text-field case is W4.
|
||
|
||
### System tray, taskbar & icons
|
||
|
||
- [x] **W10 — No taskbar attention flash.** When a background download completes while the window is
|
||
minimized/hidden, the app doesn't `flashFrame` the taskbar button. **Standard:** flash for attention on
|
||
background completion (pairs with the existing notification). *Fixed: `notify()` in
|
||
[download.ts](src/main/download.ts) calls `win.flashFrame(true)` when the completion/failure event lands
|
||
while the window is unfocused/minimized/hidden (Windows stops the flash on focus). Gated on the same
|
||
`notifyOnComplete` as the toast, so it pairs with it. (The flash itself wants a quick live confirm.)*
|
||
- [x] **W11 — No taskbar overlay badge** (deferred in roadmap): a download manager conventionally shows an
|
||
active-count / error overlay icon via `setOverlayIcon`. **Standard:** add it (the progress bar's error
|
||
mode is not a substitute). *Done: [badge.ts](src/main/badge.ts) draws a green (active) / red (error) dot
|
||
`NativeImage` with pure Node/zlib (no dep), and the `taskbarProgress` handler in
|
||
[ipc.ts](src/main/ipc.ts) calls `win.setOverlayIcon(badge, label)` with an accessible count label
|
||
("N downloads active" / "Download error"), cleared to `null` when the queue goes idle. (Numeric count
|
||
lives in the tooltip label — a 16px overlay can't legibly render digits.)*
|
||
- [x] **W12 — Fallback tray icon is single-resolution.** The embedded fallback is a 32×32 PNG, so on
|
||
150%/200% DPI the tray glyph is blurry when the real multi-size `.ico` is absent. **Standard:** a
|
||
multi-size fallback (or rely only on the `.ico`).
|
||
*Fixed (Batch 20): the fallback is now a multi-representation `NativeImage` — 16/24/32/48 px renders of
|
||
build/icon.svg (ImageMagick, like W14) embedded in
|
||
[trayFallbackIcons.ts](src/main/trayFallbackIcons.ts) and added via `addRepresentation` at scale factors
|
||
1/1.5/2/3, so Windows picks the crisp size for the DPI. Verified in an Electron probe: the image decodes
|
||
(non-empty) and reports all four scale factors. As before, it only renders if the bundled icon.ico is
|
||
missing.*
|
||
- [x] **W13 — Notifications set no icon.** `new Notification({title, body})` omitted `icon`; on the **portable**
|
||
build (no installed AUMID shortcut) Windows toasts showed a generic icon. *Fixed: both notification sites
|
||
([download.ts](src/main/download.ts) completion/failure toast + [index.ts](src/main/index.ts) background-running
|
||
toast) now pass `icon: getAppIconImage()` — a new cached `NativeImage` helper in [binaries.ts](src/main/binaries.ts)
|
||
that loads the app `.ico` once and falls back to an empty image (OS default) if it's missing. (refines L127)*
|
||
- [x] **W14 — App/notification icon is a placeholder** (M3-orig) — *Fixed: redesigned
|
||
[icon.svg](build/icon.svg) into a proper mark (teal brand square with a top-lit gradient + soft sheen and a
|
||
bold rounded download glyph over a landing shelf), legible down to 16px; regenerated the multi-size
|
||
`build/icon.ico` (256/128/64/48/32/16) via ImageMagick. The notification-icon **wiring** (passing the asset
|
||
to `new Notification`) is also done — see W13/L127.*
|
||
|
||
### High DPI, multiple monitors, touch
|
||
|
||
- [x] **W15 — Software rendering is global.** Hardware acceleration is disabled for a documented GPU
|
||
workaround; on high-DPI/large windows this trades GPU compositing for CPU, which can be sluggish.
|
||
**Standard:** re-enable HW accel where the target GPU allows (the memo already flags revisiting this).
|
||
*Fixed (Batch 20) as a user setting rather than a blind re-enable (the dev-machine GPU bug still can't be
|
||
validated here): new `Settings.hardwareAcceleration` (default **false** = today's safe software rendering),
|
||
surfaced as Settings → Appearance → "Hardware acceleration" with a restart note. index.ts reads it before
|
||
ready (after the portable userData redirect) and only then calls `disableHardwareAcceleration()`; the
|
||
window-occlusion feature stays disabled in both modes (a separate documented flicker cause). Users on
|
||
modern GPUs can now opt in without a build change. Verified the module-top boot path runs cleanly (exit 0,
|
||
empty stderr) with the new pre-ready settings read.*
|
||
- [x] **W16 — Touch targets too small / hover-only labels.** Icon-only row actions are ~32px, 4px apart,
|
||
4 per row (below the ~40px touch guideline), and their only labels are hover tooltips (`Hint`) that
|
||
never appear on touch. **Standard:** ≥40px touch targets and non-hover labels. *(extends UX17)* *Fixed:
|
||
the icon-only row-action clusters (QueueItem + HistoryView `actions` containers) now enforce a ≥40×40px
|
||
hit target via a `& button` min-size rule — the glyph is unchanged, the subtle button just carries more
|
||
padding. The label half was already covered: the shared [Hint](src/renderer/src/components/Hint.tsx) shows
|
||
on `:focus-within`, not only `:hover`, so keyboard/AT users get the name (and every action also has an
|
||
`aria-label`). (LibraryView's per-item rows have no icon-only actions — checkbox + status chip only; its
|
||
header actions are labeled text buttons — so nothing there needed enlarging.)*
|
||
|
||
### Dark mode & accessibility (Windows-specific)
|
||
|
||
- [x] **W17 — No `aria-live` for status changes.** Narrator doesn't announce download progress,
|
||
completion, or errors — there are no live regions. **Standard:** polite live regions for queue/status updates.
|
||
- [x] **W18 — Custom colors unverified under High Contrast.** The status pills, segmented controls, accent
|
||
swatches, and thumbnail tints use explicit background colors that Windows forced-colors mode may not
|
||
adapt (nothing sets `forced-color-adjust`). **Standard:** test under each Contrast theme; let system colors win.
|
||
*Fixed the two surfaces that convey meaning through color and would otherwise break under `forced-colors`:
|
||
the accent swatches ([settingsStyles.ts](src/renderer/src/components/settings/settingsStyles.ts)) now set
|
||
`forced-color-adjust: none` so the accents stay distinguishable (they're color previews — otherwise all
|
||
flatten to one system color), and the [SegmentedControl](src/renderer/src/components/ui/SegmentedControl.tsx)
|
||
active segment paints with the system `Highlight`/`HighlightText` pair under `@media (forced-colors: active)`
|
||
so the checked state stays visible once the brand background is flattened. The status chips are Fluent
|
||
`Badge`s (Fluent supplies its own forced-colors handling) and the thumbnail tints are purely decorative
|
||
placeholders, so both correctly let the system palette win. Live verification under each Contrast theme is
|
||
still worthwhile but the color-meaning surfaces are now defended in code.*
|
||
- [x] **(ref) No semantic headings / radiogroup arrow-nav / focus rings** — UI33, UI30, UI28–29 (all bear on Narrator + keyboard users). *All four resolved (see UI33/UI30/UI28/UI29).*
|
||
|
||
### Settings & standard conventions (mostly OK)
|
||
|
||
- [x] **W19 — Window/taskbar don't reflect state.** No window title at all (L108), so the taskbar button
|
||
never shows "3 downloading" or similar. **Standard:** reflect activity in the title/tooltip.
|
||
- [x] **(OK)** Per-user no-admin install, NSIS uninstaller, `aerofetch://` registration, AppUserModelID,
|
||
tray tooltip + left-click restore + right-click menu, taskbar progress, and jump list all follow
|
||
Windows conventions. **(Non-goal)** unsigned binary → SmartScreen prompt — accepted; no certificate will
|
||
be purchased (the build stays signing-ready via env vars if that ever changes — see [SIGNING.md](docs/SIGNING.md)). *(Acknowledged — verified-OK / accepted non-goal, no action.)*
|
||
|
||
---
|
||
|
||
## Code consistency & conventions
|
||
|
||
A cross-cutting consistency pass over the dimensions requested. Many specifics are already filed; this
|
||
section consolidates them by theme and — the point of the exercise — **recommends one standard each**.
|
||
`CC` IDs are the consolidated items; existing IDs are referenced, not re-filed. The codebase is actually
|
||
*stylistically* consistent (no-semicolon, single-quote, 2-space) and has a genuinely good pure/impure
|
||
split — but that style is unenforced and several "do the same thing two ways" seams have crept in.
|
||
|
||
- [ ] **CC1 — Naming conventions.** Boolean settings have no convention: `useAria2c` (verb-prefix),
|
||
`autoUpdateYtdlp` (auto-prefix), `customCommandEnabled` (suffix), `downloadArchive`/`restrictFilenames`
|
||
(bare). Keys say `videoDir`/`audioDir` while the UI says "folder." Preload names diverge from main
|
||
(L92); `MediaKind` is declared twice (L105). **Standard:** booleans as `is`/`has`/`should`/`<verb>`
|
||
consistently; one term ("folder") across keys + UI; align preload↔main names; single-source shared types in `@shared`.
|
||
*Partially closed / deferred: `MediaKind` is single-sourced (L105 done). The remaining renames are a
|
||
**breaking persistence change** — the boolean/`videoDir` keys are the on-disk `settings.json` schema, so
|
||
renaming them silently orphans every existing user's saved settings without a migration. Deferred to a 1.x
|
||
settings-migration pass; the preload↔main name alignment is the safe subset, tracked as L92 (Batch 13). The
|
||
naming convention is recorded here for new keys.*
|
||
- [x] **CC2 — Coding style is consistent but unenforced.** No ESLint/Prettier config, script, or dep
|
||
(L44), so the (good) house style drifts only by discipline; `??` vs `||` is occasionally misused for
|
||
null checks. **Standard:** add Prettier + typescript-eslint with `lint`/`format` scripts in CI; codify the existing style.
|
||
- [x] **CC3 — Different patterns for the same problem.** JSON persistence (M1), status chips (UI18),
|
||
segmented controls (UI14), buttons (UI15), overlays/dialogs (UI23), three yt-dlp probe spawners
|
||
(`probeMedia`/`probeMeta`/`probeFlat`), and **duplicated stdout line-buffering** in `download.ts` and
|
||
`terminal.ts`. **Standard:** one helper per concern (a `jsonStore`, a `StatusChip`, a `SegmentedControl`,
|
||
one spawn-and-stream helper). *Fixed: one helper now backs each concern — `createJsonStore` (M4/R1),
|
||
`StatusChip` (UI18), `SegmentedControl` (UI14), `execFileAsync` ([lib/exec.ts](src/main/lib/exec.ts)) for
|
||
the three probe spawners, and — this batch — the last duplication, the stdout newline-split loop, is now
|
||
[lib/lineBuffer.ts](src/main/lib/lineBuffer.ts) (`createLineBuffer`), adopted by both `download.ts` and
|
||
`terminal.ts` and unit-tested ([test/lineBuffer.test.ts](test/lineBuffer.test.ts)). (UI15 button-wrapping
|
||
and UI23 overlay unification stay their own incremental tracks.)*
|
||
- [x] **CC4 — Duplicate utilities.** Formatting (M9), `youtubeId` (H2), `newId` (M7), JSON I/O (M1),
|
||
`MediaKind` (L105) — **plus** `execFile` is hand-wrapped in a `new Promise` in five places
|
||
(`probe`/`ytdlp`/`ffmpeg`/`download.probeMeta`/`indexer.probeFlat`) instead of one `promisify`d helper,
|
||
and the stdout newline-split loop is copied in two. **Standard:** `src/main/lib/` + `src/renderer/src/lib/`
|
||
with one each of: `format`, `youtube`, `jsonStore`, `ytdlpExec` (promisified spawn + line stream).
|
||
*Fixed: every listed duplicate now has one home — renderer `lib/formatters`/`urlHelpers`/`id` +
|
||
`@shared/format`, `MediaKind` single-sourced (L105); main `lib/exec.ts` (the five `execFile` wrappers) and
|
||
now `lib/lineBuffer.ts` (the two stdout-split loops). The `lib/` dirs exist on both sides.*
|
||
- [x] **CC5 — Async patterns are mixed.** Three I/O idioms: callback-wrapped `new Promise` (execFile ×5),
|
||
`async/await`+`fetch` (updater check, sync), and event-emitter-wrapped Promise (`net.request` in
|
||
updater). Renderer mixes `.then().catch()` (stores) with `async/await`+try/catch (components).
|
||
**Standard:** `async/await` everywhere; a shared `execFileAsync`; wrap event-emitter APIs once in a helper.
|
||
*Closed with CL4 (Batch 21): the `execFile ×5` idiom was already gone (one `execFileAsync`, CC4), and the
|
||
last event-emitter-wrapped Promise — the updater's `net.request` streaming download — is now wrapped once
|
||
in `streamInstallerToFile`, extracted under 14 behavior-pinning tests. `async/await` + single wrappers hold
|
||
across the app code.*
|
||
- [x] **CC6 — Five failure-signaling conventions.** `{ ok, error }` result objects (download/updater/
|
||
ytdlp/backup/IPC), `throw` (`assertHttpUrl`), `null` (indexerCore), `[]` (history/sources read errors),
|
||
and a bare **error string** (`reveal.safeOpenPath`). `cleanError` is applied to some spawn stderr but not
|
||
ytdlp/ffmpeg. **Standard:** a `Result<T>` (`{ ok, value?/error? }`) across every service/IPC boundary;
|
||
`throw` only inside pure helpers, caught at the boundary; always run yt-dlp stderr through `cleanError`.
|
||
*Fixed (the concrete divergence): yt-dlp stderr now runs through `cleanError` **everywhere** — `getYtdlpVersion`
|
||
and `updateYtdlp` ([ytdlp.ts](src/main/ytdlp.ts)) adopted the exact `cleanError(r.stderr) || r.error?.message
|
||
|| ''` idiom already used by download/probe/indexer, so a failed version/update check shows the trailing
|
||
error line, not raw noise. (ffmpeg surfaces no stderr — its version probe returns `null` by design.) The
|
||
broader "one `Result<T>` across every boundary" holds already for the service/IPC layer (they return
|
||
`{ ok, error }`); collapsing the remaining `null`/`[]`/bare-string internal signals into it is a low-value
|
||
sweep left to the incremental track — the specific `cleanError` inconsistency this item names is closed.*
|
||
- [ ] **CC7 — Dependency-injection styles.** Pure modules take deps as params (good — `binDir`,
|
||
`now`); the impure shell uses module singletons (`getSettings()`, `getYtdlpPath()`, lazy `getStore()`);
|
||
progress is callback-injected; the `WebContents` sender is a param in some handlers and a closure in
|
||
others. **Standard:** keep pure-core param injection; in the shell, pass `wc`/`onProgress` consistently
|
||
as the leading argument and keep singleton access for config/binaries.
|
||
*Deferred (low-value restyle): the substantive half already holds — the pure core
|
||
(`buildArgs`/`validation`/`indexerCore`/`ytdlpPolicy`) takes deps as params and the shell uses singletons
|
||
for config/binaries, which is the recommended split. What's left is purely cosmetic argument-ordering
|
||
(`wc`/`onProgress` position), not worth the churn/risk across every handler unattended; recorded as the
|
||
convention for new handlers.*
|
||
- [x] **CC8 — No logging strategy.** Effectively no diagnostics — a single stray `console.error` in
|
||
preload, and ~29 swallowed catches (M29); `errorlog.ts` is domain data, not logging. **Standard:** one
|
||
small leveled logger (e.g. `electron-log`) written to userData, called at every catch; keep `errorlog.ts` for user-facing download failures only.
|
||
*Fixed: a small in-house leveled logger [logger.ts](src/main/logger.ts) (no new dependency — the
|
||
`electron-log` suggestion was an example) appends to `<userData>/logs/aerofetch.log` with size-capped
|
||
rotation; all `app`/fs access is lazy+guarded so it's a safe no-op under unit tests. The main-process
|
||
catch sites route through it, and a fire-and-forget `log:write` IPC + preload `logError()` forward the
|
||
renderer's `logError` (M29) failures to the same file sink — closing the "sink" half M29/R6 deferred here.
|
||
`errorlog.ts` stays as user-facing download-failure data. The user-facing **toast** half ties to the
|
||
global status surface (UI25/UX9) and lands there.*
|
||
- [ ] **CC9 — Four validation styles.** Type-guard predicates (`isValid*` in validation.ts),
|
||
coerce-with-fallback (`sanitizeOptions`, `templates.sanitize`), an imperative per-key `switch`
|
||
(`setSettings`, M24), and `throw` (`assertHttpUrl`). **Standard:** one schema layer (e.g. `zod`) that
|
||
both *validates and coerces*; `setSettings` runs values through the same schema instead of a bespoke switch.
|
||
*Deferred to 1.x (per the release gate — the CC consolidations are explicitly post-1.0): adopting a schema
|
||
layer (`zod`) is a new runtime dependency plus a rewrite of `validation.ts` + `setSettings` + backup import.
|
||
High-value but a sweeping change best done as its own reviewed PR, not unattended; standard recorded.*
|
||
- [ ] **CC10 — Mixed serialization/persistence.** `electron-store` (settings) **and** hand-rolled
|
||
pretty-JSON files (history/errorlog/templates/sources/media-items, M1) for the same job, plus
|
||
yt-dlp-mandated formats (Netscape cookies, plaintext archive) and base64 `enc:v1:` secrets.
|
||
**Standard:** one `jsonStore<T>()` abstraction for the app's own records; pick **either** electron-store
|
||
**or** the JSON stores for everything, not both; leave the yt-dlp-format files alone.
|
||
*Partly closed / deferred: the `jsonStore<T>()` abstraction now exists (`createJsonStore`, M4/R1) and backs
|
||
every app record (history/errorlog/templates/sources/media-items/queue), so the "hand-rolled per file"
|
||
divergence is gone. The remaining electron-store↔jsonStore split is a **deliberate** keep: settings live in
|
||
electron-store for its DPAPI-encrypted secret fields, records in jsonStore. Fully migrating settings off
|
||
electron-store is a 1.x call, not forced here.*
|
||
- [ ] **CC11 — Configuration is scattered.** `electron-store` + `localStorage` (sidebar, M19) + env vars
|
||
(`PORTABLE_EXECUTABLE_DIR`, `CSC_LINK`, `AEROFETCH_REAL_DOWNLOAD`) + hardcoded module consts (update
|
||
host/owner/repo, timeouts, caps, `09:00`, `ARIA2C_ARGS`, L10). **Standard:** a `config.ts` for build/host
|
||
constants; fold `localStorage` UI prefs into the settings store so there's one persisted-prefs source.
|
||
*Partly closed / deferred: the runtime consts are already centralized in
|
||
[constants.ts](src/main/constants.ts) (L10), and the sidebar `localStorage` pref was folded into the
|
||
settings store (M19). What's left — a dedicated `config.ts` for build/host constants (update
|
||
host/owner/repo, currently co-located in updater.ts) — is a marginal move deferred to the 1.x organization
|
||
pass (CC12), with which it naturally lands.*
|
||
- [ ] **CC12 — Project organization.** Renderer `components/` mixes screens (views) with reusable widgets;
|
||
helpers (`theme`/`thumb`/`useClipboardLink`) sit at src root; the **pure** `queueStats` lives in `store/`;
|
||
main mixes pure (`buildArgs`/`validation`/`indexerCore`/`ytdlpPolicy`) and impure modules in one flat dir.
|
||
**Standard:** renderer `views/` + `components/` + `lib/`; main `core/` (pure) + services; move `queueStats` to `lib`.
|
||
*Deferred to 1.x: a file-tree reorg is broad import churn across nearly every module for no behavior change —
|
||
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.*
|
||
- [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.
|
||
*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.
|
||
*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)
|
||
|
||
1. **Errors:** one `Result<T> = { ok: true; value } | { ok: false; error }` across every service/IPC
|
||
boundary; `throw` only in pure helpers; one `logger` (leveled, file-backed) invoked at every catch.
|
||
2. **Async:** `async/await` only; one `execFileAsync` + one spawn/stream helper; wrap event-emitter APIs once.
|
||
3. **Validation:** one schema lib (zod) that validates **and** coerces; reuse it in `setSettings`, backup import, and JSON-store reads.
|
||
4. **Persistence/serialization:** one `jsonStore<T>()` (or electron-store) for app records — not both; one config module for host/build constants; one persisted-prefs store (no `localStorage`).
|
||
5. **Shared code:** `lib/` for pure utils (format, youtube, ids, jsonStore); `@shared` is the only home for cross-process types (no re-declared `MediaKind`).
|
||
6. **UI:** design tokens (spacing/radius/type/icon scales) + shared primitives (`Screen`, `StatusChip`, `SegmentedControl`, `LinkSuggestion`, button/focus); stores are view-models, components presentational.
|
||
7. **Naming:** booleans `is`/`has`/`should`/`<verb>`; "folder" not "dir"; preload methods match main names; `verbNoun` for store actions, `onX` for component handlers.
|
||
8. **Tooling:** Prettier + typescript-eslint with `lint`/`format`/CI gates so all of the above stays enforced rather than aspirational.
|
||
|
||
---
|
||
|
||
## Code cleanliness
|
||
|
||
A dead-code / clutter review. **First, the good news — the codebase is genuinely clean on most axes:**
|
||
no `TODO`/`FIXME`/`HACK`/`XXX` in source (only in roadmap docs), **no commented-out code**, **no
|
||
`debugger`/debug logging** (the lone `@ts-ignore` ×2 in preload is the legitimate browser-fallback
|
||
`window` assignment), every source file is imported, and the dead IPC surface is exactly the two already
|
||
filed. Several listed categories are **N/A** to this stack and worth recording so they're not re-asked:
|
||
|
||
- **Unused XAML / WPF** — N/A (Electron + React/HTML, no XAML).
|
||
- **Unused strings / localization** — N/A (no i18n/string-resource files; strings are hard-coded, i18n deferred), so nothing to be "unused."
|
||
- **Unused images / icons** — none: the only assets are `build/icon.{ico,svg}` (used) and the inline base64 fallback tray PNG (used); thumbnails are remote. *(Clutter, not shipped: `dist/` ~3 GB of old installers — L84; `.frames/` 19 gitignored captures.)*
|
||
- **Unused files / resources** — none in `src` or `resources/bin` (yt-dlp/ffmpeg/ffprobe all used; aria2c optional).
|
||
|
||
**Dead / unreachable code (the real list):**
|
||
|
||
- [x] **Safe to remove now — `getDefaultFolder`** (M2): channel + preload method + main handler + mock,
|
||
**zero callers**. Pure deletion, no behavior change.
|
||
- [x] **Decide "wire or remove" — command preview** (M5): channel + preload `previewCommand` + main
|
||
`previewCommand()` + `formatCommandLine`/`quoteForDisplay` (buildArgs, **unit-tested**) +
|
||
`CommandPreviewResult` + mock. *Decided: **wired** (M5/UX1) — the DownloadBar Advanced panel has a
|
||
Show/Hide command toggle calling `window.api.previewCommand`; the whole chain is now reachable.*
|
||
- [x] **Decide "wire or remove" — incognito/private** (M6): `DownloadItem.incognito` + `AddOptions.incognito`
|
||
+ `buildItem` handling + two history-skip checks + the QueueItem "Private" badge. *Decided: **wired**
|
||
(M6/UX1) — the DownloadBar Advanced panel has an "Incognito mode" checkbox that sets the flag (reset
|
||
after each download).*
|
||
- [x] **Decide "wire or remove" — per-download options/extraArgs** (UX1): `DownloadItem.options`/`extraArgs`,
|
||
`StartDownloadOptions.options`/`extraArgs`, `AddOptions.options`/`extraArgs` are plumbed end-to-end but
|
||
never set by any UI. *Decided: **wired** (UX1) — the collapsible Advanced panel reuses
|
||
`DownloadOptionsForm` for per-download overrides passed via `AddOptions.options`; overrides reset to
|
||
the global defaults after each download.*
|
||
- [x] **Comment, not code — `MAX_ENQUEUE_BATCH`** (M33): referenced by ipc.ts:629 + the roadmap but never
|
||
implemented; fix the comment (nothing to delete). *Resolved by M33: the false `MAX_ENQUEUE_BATCH`
|
||
reference is gone; [ipc.ts](src/shared/ipc.ts):730 now honestly states all selected entries are
|
||
enqueued at once (with a note to add a cap if large — the actual cap is deferred to PERF3).*
|
||
|
||
**New cleanliness findings:**
|
||
|
||
- [x] **CL1 — Magic-string protocol markers duplicated across the emit/parse boundary.** `'prog|'` and
|
||
`'path|'` are hard-coded in `buildArgs.ts` (`PROGRESS_TEMPLATE` / `--print after_move:path|…`) and again
|
||
in `download.ts` (`line.startsWith('prog|')` / `'path|'`). Change one and the other breaks silently.
|
||
**Fix:** export shared marker constants from one module.
|
||
- [x] **CL2 — Long positional parameter list.** `buildArgs(opts, outputTemplate, o, binDir, access,
|
||
extraArgs)` takes six positional args (and `o` vs `opts` is easy to swap). **Fix:** pass a single options
|
||
object. *Fixed: `buildArgs(input: BuildArgsInput)` now takes one named object (`opts`/`outputTemplate`/
|
||
`options`/`binDir`/`access`/`extraArgs?`) destructured at the top, so callers can't transpose the two
|
||
path-like strings or `opts`/`options`. download.ts's `buildCommand` and both test harnesses updated.*
|
||
- [x] **CL3 — Large method: `startDownload`** (~130 lines) bundles spawn + dual metadata path + four inline
|
||
`child` event handlers. **Fix:** extract the stdout-parse + close/error wiring (pairs with CC3's spawn-stream
|
||
helper). *Fixed: the stdout/stderr parse, the close/error handlers, and the B1 idle watchdog moved into a
|
||
`wireChildProcess({ wc, opts, rec, cleanup, getTitle })` helper in [download.ts](src/main/download.ts).
|
||
`startDownload` is now a linear pre-flight (binary checks → URL normalise → concurrency guard → cookies →
|
||
spawn → metadata probe → wire). `getTitle` is a getter because the parallel metadata probe may fill the
|
||
resolved title in after wiring is set up. Behaviour unchanged.*
|
||
- [x] **CL4 — Deep nesting: `updater.downloadAppUpdate`** — Promise → `net.request` → `response` → `data`
|
||
with nested conditionals/teardown is the hardest-to-follow block. **Fix:** extract a `streamToFile` helper.
|
||
*Fixed (Batch 21, pin-then-refactor): the security-critical invariants were first locked with 14
|
||
behavior-pinning tests at the public `downloadAppUpdate` boundary
|
||
([test/updaterDownload.test.ts](test/updaterDownload.test.ts), scripted `net.request` fake) — untrusted
|
||
initial URL, per-hop redirect re-validation on both the checksum fetch and the installer, missing/malformed
|
||
checksum refusal, SHA-256 mismatch + partial cleanup, truncation detection, idle-timeout, checksum-phase and
|
||
mid-download cancel, HTTP-error status, combined-checksum line matching, and progress events. Then the
|
||
nested block was extracted verbatim into `streamInstallerToFile` (redirects/hashing/timeout/truncation/
|
||
teardown in one once-wrapped helper); `downloadAppUpdate` is now a linear pre-flight (trust-check →
|
||
checksum → stream), with the B6 cancel-slot ownership kept beside the module state it guards via
|
||
`onCancelReady`/`onSettled`. All pins passed before AND after the extraction — no behavior change.*
|
||
- [x] **CL5 — Superfluous exports.** `PROGRESS_TEMPLATE` (L167) and `getManagedBinDir` are `export`ed but
|
||
used only within their own module. **Fix:** make them module-private.
|
||
- [x] **CL6 — Redundant wrappers** (minor): `MediaThumb`'s `kind === 'audio' ? 'audio' : 'video'` (L15),
|
||
`clearDir` = `update({[t]:''})`, and the store `openFile`/`showInFolder` thin wrappers around `window.api`.
|
||
Harmless; collapse opportunistically. *Resolved: the genuinely-redundant one is gone — `MediaThumb`'s
|
||
no-op ternary was collapsed in L15. The other two are **deliberate, not redundant**: `clearDir` and the
|
||
store `openFile`/`showInFolder` are the view-model layer (CC13) — the store is the single place components
|
||
reach `window.api`, so keeping components free of direct IPC is the point, not incidental wrapping. Left
|
||
as-is on purpose.*
|
||
|
||
**Magic numbers / strings (consolidated, see L10):** timeouts `15_000`/`30_000`/`60_000`/`180_000`,
|
||
maxBuffers `4`/`64`/`128`/`256`×1024×1024, caps `500`/`200`/`100`/`20000`/`80`, `VIRTUALIZE_AT 100`,
|
||
installer handoff `1500`, tickers `650`/`15_000`, stderr tail `4000`; magic strings `'enc:v1:'`,
|
||
`'persist:aerofetch-login'`, `'AeroFetchDailySync'`, `'--sync'`, `'prog|'`/`'path|'` (CL1).
|
||
**Fix:** a `constants.ts` (CC11) — names over literals.
|
||
|
||
### What can safely be removed (recommendation)
|
||
|
||
1. **Now (zero risk):** the `getDefaultFolder` slice (M2); un-export `PROGRESS_TEMPLATE` + `getManagedBinDir` (CL5).
|
||
2. **Now (housekeeping):** prune `dist/` to the current release (L84); the macOS-only branches if Windows-only is committed (L147).
|
||
3. **Decision made — wired, not deleted (all resolved):** command preview (M5), incognito (M6), and
|
||
per-download options/extraArgs (UX1) are now surfaced in the DownloadBar Advanced panel, so the
|
||
previously-unreachable plumbing is reachable. Nothing to delete.
|
||
4. **Not removable (intentional):** preview seed/mock data + `PREVIEW` branches (C1/L128) — they power the
|
||
browser-preview dev workflow; consolidate (C1) rather than delete.
|
||
|
||
---
|
||
|
||
## Simplification opportunities
|
||
|
||
Reusable abstractions that shrink the code **and** read better — no premature optimization, just
|
||
de-duplication. Organized by the requested categories; each lists the repetition (with refs), the
|
||
abstraction, and a rough **LOC delta** (net, after the helper's own cost). Estimates are deliberately
|
||
conservative.
|
||
|
||
| # | Category | Repetition (refs) | Reusable abstraction | ~Net LOC |
|
||
|---|---|---|---|---|
|
||
| SIMP1 | Repeated file handling | JSON read/save+cap+try/catch in history/errorlog/templates; sources already has it (M1, CC10) | `createJsonStore<T>(file, isValid, cap)` | −40 |
|
||
| SIMP2 | Repeated networking | `execFile`-in-`Promise` ×5 (probe/ytdlp×2/ffmpeg/probeMeta/indexer) (CC4/CC5) | `execFileAsync` + `spawnYtdlpJson()` | −40 |
|
||
| SIMP3 | Repeated parsing | byte/speed/eta/duration formatters ×4 (M9, L165, L166) | `lib/format.ts` (hour-aware, one precision rule) | −40 |
|
||
| SIMP4 | Repeated parsing | YouTube id/URL parsing ×4 (H2) | `lib/youtube.ts` | −30 |
|
||
| SIMP5 | Repeated logic | stdout line-buffering ×2 + `taskkill` tree-kill ×2 (download.ts/terminal.ts) (CC3, **new**) | `lineStream(stream, onLine)` + shared `killTree(pid)` | −25 |
|
||
| SIMP6 | Repeated commands | 7 identical preload `on*` subscribe/unsubscribe wrappers (**new**) | `subscribe(channel, cb)` helper in preload | −20 |
|
||
| SIMP7 | Repeated event handlers | `Set<string>` add/remove/toggle/toggle-all in DownloadBar/Library/History (**new**) | `useSelection()` hook | −30 |
|
||
| SIMP8 | Repeated view models | async-action `setBusy/try/catch/finally + result + error` ×~7 in SettingsView (**new**) | `useAsyncAction()` hook | −50 |
|
||
| SIMP9 | Repeated UI (cards) | hand-styled `<div>` card surfaces ×6 (UI7/UI20/L120) | `<Surface tier>` (or use Fluent `Card`) | −30 |
|
||
| SIMP10 | Repeated UI (settings) | `sectionHeader` + `Card` ×11 and Switch+`Field`+"On/Off" ×~12 in SettingsView | `<SettingsCard icon title>` + `<ToggleField>` | −90 |
|
||
| SIMP11 | Repeated UI (row actions) | `Hint`+`Button` icon-action ×~16 (QueueItem/History/Templates) | `<IconAction label icon onClick>` | −55 |
|
||
| SIMP12 | Repeated UI (empty states) | centered icon+text `empty` block ×3 (L116) | `<EmptyState icon title hint?>` | −25 |
|
||
| SIMP13 | Repeated dialogs/banners | suggestion banner ×2 (UI19); status chips Badge-vs-pill (UI18, M8) | `<LinkSuggestion>` + `<StatusChip>` | −40 |
|
||
| SIMP14 | Repeated UI (controls) | two hand-rolled segmented controls (UI14) | `<SegmentedControl>` | −30 |
|
||
| SIMP15 | Repeated validation | type-guards + `sanitizeOptions` + `setSettings` switch (CC9, M24) | one schema (zod) validate+coerce | −60 |
|
||
| SIMP16 | Repeated networking | `net.request` + redirect-revalidate + timeout ×2 in updater (`fetchTrustedText`/`downloadAppUpdate`) (**new**) | `trustedRequest(url, {sink})` | −30 |
|
||
| SIMP17 | Repeated logic (misc) | `newId` ×3 (M7), `PREVIEW` ×8 (C1), `(ARR as readonly string[]).includes` ×N (L90) | `lib`: `newId`, `isPreview`, `isMember` | −15 |
|
||
| SIMP18 | Repeated converters | `MediaKind` ×2 (L105), kind\|quality string encode/decode (CL2/L91) | single-source type + typed pair | −10 |
|
||
|
||
### Highest-value (do these first)
|
||
|
||
1. **SettingsView decomposition (SIMP10 + SIMP8 + SIMP11).** A `<SettingsCard>` + `<ToggleField>` +
|
||
`useAsyncAction` + `<IconAction>` would take the 1104-line file to roughly **~600**, and the per-card
|
||
split (H1) falls out for free. Biggest single readability win.
|
||
2. **`createJsonStore` (SIMP1)** collapses three near-identical persistence modules to thin configs and
|
||
removes the M1/CC10 "two ways to persist" seam.
|
||
3. **`lib/format.ts` + `lib/youtube.ts` (SIMP3/SIMP4)** remove the most-copied pure utilities and fix the
|
||
formatter divergences (L165/L166) at the same time.
|
||
4. **`execFileAsync`/`spawnYtdlpJson` (SIMP2/SIMP5)** unify five spawn sites + the duplicated kill/line-buffer.
|
||
|
||
### Estimate
|
||
|
||
Gross duplicated lines removable ≈ **650–750**; the new helpers/primitives add back ≈ **150–200**, for a
|
||
**net reduction of ~550–800 lines (~5–7% of the ~12.5K total)**. Concentration: **SettingsView ~−450**
|
||
(1104 → ~600), the **JSON stores ~−40**, **main spawn/format/util helpers ~−120**, **shared UI primitives
|
||
~−150**. The real payoff is readability and killing the "two-ways-to-do-X" seams (M1/M8/M9/UI14/UI18/CC*),
|
||
not the line count. **Caveat:** stop short of over-abstracting — a `createMirroredStore` factory or a
|
||
generic form engine would cost more clarity than it saves; keep abstractions at the "one obvious helper" level.
|
||
|
||
---
|
||
|
||
## Bug hunt (execution-path review)
|
||
|
||
Tracing real code paths for latent bugs by category. **New** bugs get `B` IDs; the genuine bugs found in
|
||
earlier passes are listed so this is complete; and the categories that came back **clean** are recorded
|
||
(bug-hunting that finds nothing is a result too).
|
||
|
||
### New bugs
|
||
|
||
- [x] **B1 — No stall/idle timeout on downloads (resource leak + stuck slot).** `spawn(ytdlp, …, {
|
||
windowsHide: true })` ([download.ts](src/main/download.ts):306) sets **no `timeout`**, and `buildArgs`
|
||
emits no `--socket-timeout`. A hung connection means yt-dlp never exits → the `active` map entry and the
|
||
renderer's `downloading` item persist **forever**, permanently consuming a concurrency slot with no
|
||
recovery or feedback. (The app-updater download *does* have `DOWNLOAD_IDLE_TIMEOUT_MS`; downloads don't.)
|
||
**Fix:** pass `--socket-timeout` and/or an app-side idle watchdog that kills + errors a stalled child.
|
||
- [x] **B2 — Source indexing can't be cancelled.** `indexSource` walks a channel's playlists with
|
||
sequential probes (each up to the 180 s `probeFlat` timeout); the UI shows "indexing…" with the Index
|
||
button disabled and **no cancel**. A large channel locks the add-source flow for minutes with no abort.
|
||
**Fix:** thread an `AbortSignal`/cancel token and a Cancel button.
|
||
*Fixed end-to-end. Main: [indexer.ts](src/main/indexer.ts) threads an `AbortSignal` through `probeFlat` /
|
||
`probeTab` (→ execFileAsync's `signal`, which kills the in-flight yt-dlp child) and checks `signal.aborted` at
|
||
every walk boundary, returning a `canceled` result that pushes no error progress; a per-walk `AbortController`
|
||
is registered so a new `sources:index-cancel` IPC (`cancelIndexing()`) aborts it. Renderer: a "Cancel" button
|
||
in the LibraryView add-source progress row calls `useSources.cancelIndexing()`, which optimistically clears
|
||
the indexing UI and flags the pending index/reindex so its result is treated as canceled (no error toast)
|
||
rather than a failure. Background sync (sync.ts) stays non-cancelable by design (unattended). Verified live
|
||
that the abort actually stops the walk: aborting a real `@LinusTechTips/videos` `-J --flat-playlist` probe
|
||
fired the callback 2 ms later with `AbortError` and `tasklist` confirmed the yt-dlp process was gone — Node's
|
||
AbortSignal terminates the native probe on Windows. typecheck + 302 tests + production build green.*
|
||
- [x] **B3 — `extractSha256` ignores the filename (wrong-hash risk).** It returns the **first** 64-hex
|
||
token in the file ([updater.ts](src/main/updater.ts)); a combined multi-file checksum uploaded as
|
||
`<asset>.sha256` would verify the installer against the wrong line's hash. Safe only by the one-hash-
|
||
per-asset convention. **Fix:** match the hash on the asset's filename line.
|
||
- [x] **B4 — `probeMeta` assumes one line per `--print` field.** It does `stdout.split('\n')` → `[title,
|
||
uploader, duration]` ([download.ts](src/main/download.ts)); a title containing a newline shifts channel
|
||
and duration by a line. **Fix:** use a single `--print` with an unlikely delimiter, or `-J`.
|
||
- [x] **B5 — Missing status guard on the `meta` event.** Between `cancel()` and the child's `close`,
|
||
`active.has(id)` is briefly true, so `probeMeta` can `send` a `meta` event for a just-canceled item;
|
||
`applyEvent`'s `meta` case (unlike `progress`/`done`/`error`) has **no canceled guard**, so it updates a
|
||
canceled item's title. Harmless today, but the asymmetry is a latent bug. **Fix:** guard `meta` like the others.
|
||
- [x] **B6 — App-update download has no cancel.** Once `downloadAppUpdate` starts there's no user abort —
|
||
only completion or the idle timeout stops it; the progress UI offers no Cancel. **Fix:** expose cancel (abort the request).
|
||
*Fixed. [updater.ts](src/main/updater.ts) now stashes the in-flight download's canceller (a closure over the
|
||
existing `finish({ ok:false })`) in a module var, set when the download starts and cleared on settle; a new
|
||
`cancelAppUpdate()` + `app:update-cancel` IPC invokes it, routing through the SAME teardown the audit's
|
||
failure-path review already verified — `request.abort()`, destroy the write stream, and unlink the partial.
|
||
Renderer: a "Cancel download" button under the progress bar in
|
||
[SoftwareUpdateCard.tsx](src/renderer/src/components/settings/SoftwareUpdateCard.tsx) calls it and sets a
|
||
`canceledRef` so the resulting not-ok download reads as canceled (no error toast), not a failure. Verified
|
||
the teardown on the real Electron runtime: a `net.request` streaming to a temp file was aborted mid-download
|
||
via that exact `abort()`+`destroy()`+`unlink()` sequence and the partial was confirmed gone from temp
|
||
afterwards (no lingering Windows file handle). CL4 (the updater's deep-nesting refactor) stays deferred as a
|
||
separate cosmetic cleanup. typecheck + 302 tests + production build green.*
|
||
- [x] **B7 — Cookie-login promise can never resolve if the window is destroyed without `close`.**
|
||
`openCookieLoginWindow` resolves only via the `close`→`exportAndResolve` path; a `destroy()` (or a
|
||
`closed` without `close`) would leave `pendingResolvers` pending forever. Not triggered by current code,
|
||
but a latent never-resolve. **Fix:** also resolve/reject on `closed`.
|
||
|
||
### Real bugs already filed (confirmed in this pass)
|
||
|
||
- **M32** playlist/channel batches download in reverse order · **M35** History re-download duplicates rows
|
||
· **M34** optimistic settings never reconcile (UI shows unsaved values) · **L88** a `progress` event can
|
||
flip a `queued` item to `downloading` outside `pump()` · **L139** startup yt-dlp auto-update vs a
|
||
concurrent download spawning the same exe (Windows file-in-use) · **L140** retry-during-teardown hits the
|
||
"already running" guard · **L148** `clearFinished` can free a slot before the killed tree exits (brief
|
||
over-cap) · **L157** `markDownloaded` no-ops after `removeSource` (orphan downloads).
|
||
|
||
### Came back clean (no bug found)
|
||
|
||
- **IPC-after-destroy:** every `wc.send`/`e.sender.send`/`mainWindow.*` is guarded by `isDestroyed()`.
|
||
- **Threading / UI threading / synchronization:** single JS thread in main and renderer; React state
|
||
updates are always on-thread; the `active` map + module vars are main-thread-only — no data races.
|
||
- **Infinite loops:** the stdout line-buffer advances each iteration; `parseExtraArgs`/`parseRssVideoIds`
|
||
regexes can't zero-width-match; no self-recursive `pump`.
|
||
- **Stream disposal (updater):** `downloadAppUpdate` has a single `finish()` that aborts the request,
|
||
destroys the file stream, and unlinks the partial — correct on every failure path.
|
||
- **Event-listener cleanup:** component `useEffect` subscriptions return their unsubscribe; module-level
|
||
subscriptions are app-lifetime by design.
|
||
- **Off-by-one / boundary:** `parseProgress` guards `parts.length < 6`; playlist/collection indexing is
|
||
1-based throughout; `MAX_ENTRIES` slices are correct; `fmtBytes` clamps the unit index.
|
||
|
||
---
|
||
|
||
## Resilience & failure-mode review
|
||
|
||
A fresh lens: what happens when the *environment* fails (disk full, permission denied, corrupt file,
|
||
killed mid-write, huge inputs) rather than when the logic is wrong. These are robustness findings, not
|
||
cosmetics. `R` IDs.
|
||
|
||
- [x] **R1 — JSON stores are not written atomically (corruption on crash/power-loss).** The hand-rolled
|
||
stores (history/errorlog/templates/sources/media-items) persist via `writeFileSync(file, JSON.stringify…)`
|
||
— not write-temp-then-rename. A crash/power-cut mid-write truncates the file; the next read hits a parse
|
||
error and returns `[]`. `electron-store` (settings) *is* atomic, so durability is inconsistent across the
|
||
app. **Fix:** an atomic write (temp + rename, or `write-file-atomic`) in the shared `jsonStore` (SIMP1).
|
||
- [x] **R2 — A corrupt/invalid store silently loses all data.** `readJsonArray`/`listHistory` etc. catch a
|
||
parse error and return `[]` with no warning, backup, or recovery — one bad byte wipes the user's history/
|
||
sources/templates from their perspective. **Fix:** on parse failure, back up the bad file (`.corrupt`) and surface a notice.
|
||
- [x] **R3 — O(n) full-file rewrite per download completion (≈O(n²) per channel), synchronously on the
|
||
main thread.** `setMediaItemDownloaded` ([sources.ts](src/main/sources.ts):126) reads + parses the entire
|
||
`media-items.json` (up to `MAX_ITEMS` = 20,000) and rewrites the whole file on **every** completion, and
|
||
`addHistory` rewrites all of `history.json` likewise. Downloading a 1,000-video channel ⇒ ~1,000 full
|
||
read-parse-write cycles of a multi-MB file, blocking the IPC/event loop each time. **Fix:** in-memory
|
||
cache + debounced/batched atomic writes (or the deferred better-sqlite3).
|
||
- [x] **R4 — Canceled downloads orphan `.part` files.** `cancelDownload` tree-kills yt-dlp but never
|
||
deletes the partial; canceled items vanish from the UI while `.part`/fragment files accumulate in the
|
||
output folder. (App crash mid-download likewise — the queue isn't persisted to resume, M4.) **Fix:**
|
||
clean up the partial on cancel, or surface/offer-resume for orphans.
|
||
*Fixed. The engine now captures the resolved FINAL output path during the run via a new
|
||
`--print before_dl:dest|%(filename)s` (empirically: `%(filepath)s` is unresolved that early, `%(filename)s`
|
||
gives the full path), stored on the download in [download.ts](src/main/download.ts). On CANCEL (not pause —
|
||
pause keeps the `.part` for resume, L65) the child's close/error handler runs `cleanupPartials`, which uses
|
||
the pure, unit-tested [orphanPartials](src/main/lib/partials.ts) to delete only this download's unambiguous
|
||
intermediates: `.part`, `.part-Frag*`, `.ytdl`, and finished per-stream `<stem>.fNNN.<ext>` files — never a
|
||
bare `<stem>.<ext>` completed file (so a pre-existing same-title download is safe) or another download's
|
||
files. Verified live: a real 1080p download killed with the app's exact `taskkill /T /F` left
|
||
`…f399.mp4.part`, and on close the cleanup deleted ONLY that orphan while a planted same-title `.webm` and an
|
||
unrelated download's `.part` both survived — and the handle was released by close time (no lock error). The
|
||
predicate is covered across every partial type by test/partials.test.ts. Cleanup runs from the settle
|
||
handler, so the process (and its file handles) are already gone.*
|
||
- [x] **R5 — Settings write failure is unhandled.** `setSettings` calls `store.set(...)` with no try/catch;
|
||
on disk-full/read-only `electron-store` throws → the IPC rejects → the renderer's `setSettings().catch(()
|
||
=> {})` swallows it while the optimistic UI keeps the change (the disk-full form of M34). **Fix:** catch and report.
|
||
- [x] **R6 — Silent data loss on any store write failure.** A completed download that can't be recorded
|
||
(disk full) shows "Completed" in-session but is gone on restart, with no error (the write `catch` is empty,
|
||
M29). **Fix:** detect write failure and warn. *Fixed (log half) in [jsonStore.ts](src/main/jsonStore.ts):
|
||
`writeJsonAtomic` now returns whether the write landed and logs the failure (with the path) instead of an
|
||
empty catch, and removes the leftover `.tmp`; `createJsonStore`'s `flush` only clears the dirty flag on
|
||
success, so a failed write stays pending and is retried on the next `write()` or the quit-time
|
||
`flushAllStores()` rather than dropped. Unit-tested in `test/jsonStore.test.ts`. **Scope note:** like M29
|
||
this delivers the "no longer silent" half (logged + retried); the user-facing toast needs the leveled file
|
||
logger / toast sink deferred to **CC8**.*
|
||
- [x] **R7 — `safeStorage` unavailable ⇒ secrets silently stored as plaintext.** `encryptSecret` falls back
|
||
to plaintext when DPAPI/safeStorage isn't available, with no indication — compounds H7 (cookies) and the
|
||
backup-cleartext issue (M22). **Fix:** warn (or refuse to store) when encryption is unavailable. *Fixed in
|
||
[settings.ts](src/main/settings.ts): `encryptSecret` now `console.warn`s before returning a non-empty secret
|
||
as plaintext, so the cleartext fallback is no longer silent. Chose warn-not-refuse to preserve the
|
||
pre-encryption behaviour (storing still works); DPAPI is effectively always present on Windows, so this
|
||
should never fire in practice. (refusing/at-rest enforcement remains an option if ever wanted.)*
|
||
- [x] **R8 — Pretty-printed JSON for large stores.** `JSON.stringify(value, null, 2)` inflates size/write
|
||
time for the potentially-20k-item media store (ties R3). **Fix:** compact JSON for the large stores.
|
||
- [x] **R9 — Clock-skew sensitivity.** `shouldAutoCheckYtdlp` and the scheduled-download promoter compare
|
||
`Date.now()`; a backward system-clock correction can skip or double-fire a check/schedule. Minor; note for awareness.
|
||
*Fixed the concrete case: `shouldAutoCheckYtdlp` now treats a `lastCheck` in the future (clock ran
|
||
backward → negative interval) as due, so the daily update check can't be skipped forever (unit-tested).
|
||
The scheduled-download promoter is inherently fire-once — each promotion flips the item out of `saved`,
|
||
so a backward jump only delays it and can't double-fire; a forward jump firing early is documented and
|
||
accepted as minor (a monotonic scheduler would be overkill for a park-until-time feature).*
|
||
|
||
---
|
||
|
||
## Ship-readiness (perceived quality)
|
||
|
||
Obsessive "does it feel finished" pass. The biggest untapped vein is **defaults** (first-run impressions),
|
||
plus a few stuck-status / progress glitches. `SR` IDs; already-filed perceived-quality items are listed at
|
||
the end so the picture is complete. **Verified-clean first:** card/label/button text is consistently
|
||
*sentence case*, and a careful read found **no spelling typos** — the polish gaps are defaults, jargon, and a couple of stuck states, not sloppy text.
|
||
|
||
### Poor / risky defaults (first-run impressions)
|
||
|
||
- [x] **SR1 — Theme defaults to `'light'`, not `'system'`.** A dark-mode Windows user gets a bright white
|
||
app on first launch despite the app fully supporting "follow system." Single most-noticeable first-run
|
||
miss. **Fix:** default `theme: 'system'`.
|
||
- [x] **SR2 — `ytdlpChannel` defaults to `'nightly'`.** The app ships auto-pulling yt-dlp's daily,
|
||
less-tested build (auto-update also on by default); one nightly regression breaks downloads for everyone.
|
||
Defensible for chasing YouTube changes, but a risky default for a commercial release. **Fix:** consider `stable` as the shipped default.
|
||
- [x] **SR3 — `autoDownloadNew` defaults to `true`.** The moment a user marks a channel "watched," new
|
||
uploads auto-download with no per-event consent — can quietly fill a disk. **Fix:** default off (let watching be "notify," downloading be opt-in).
|
||
- [x] **SR4 — `clipboardWatch` defaults to `true`.** The app reads the clipboard on every window focus from
|
||
first launch (R7/L138). A privacy-conscious default would be off or first-run-prompted. **Fix:** default off, or ask on first run.
|
||
*Fixed: `DEFAULT_SETTINGS.clipboardWatch` is now `false` ([shared/ipc.ts](src/shared/ipc.ts)) — a new install
|
||
doesn't read the clipboard until the user opts in (a one-toggle setting), and the Onboarding tip was reworded
|
||
to point there ("Turn on clipboard detection in Settings…"). Existing installs keep their saved value
|
||
(electron-store only fills missing keys), so no one who already enabled it is affected. **Product note for
|
||
the maintainer:** this flips a signature convenience off-by-default for new users — trivially reversible if
|
||
you'd rather it stay on.*
|
||
- [x] **SR5 — Audio defaults to MP3 (lossy).** `defaultAudioQuality: 'Best (MP3)'` + `audioFormat: 'mp3'`
|
||
give a lossy re-encode by default; m4a/opus are higher quality at the same size. Minor (MP3 = max
|
||
compatibility), but "Best" implying MP3 is a quality mismatch (M18).
|
||
*Resolved as a deliberate default (documented): MP3 is the maximum-compatibility container (plays on every
|
||
device/player), which is the right *default* for a general-audience downloader — a user who wants
|
||
m4a/opus/flac picks it in the format options. The "Best" *label* mismatch (the real bug) was fixed in M18
|
||
(the preset labels and `--audio-quality` handling now match the chosen format). Changing the shipped default
|
||
container away from MP3 is a product-taste call with a real compatibility downside, so it stays MP3 by
|
||
intent, not oversight.*
|
||
|
||
### Stuck states & progress glitches
|
||
|
||
- [x] **SR6 — "Resolving…" can stick forever.** A queued item's channel shows the `'Resolving…'`
|
||
placeholder until a `meta` event arrives; if metadata never resolves (error before meta, or no channel
|
||
in the probe), the row reads "Resolving… • 1080p • Video" permanently — looks hung. **Fix:** clear the placeholder on error / when meta fails.
|
||
- [x] **SR7 — Progress bar visibly resets mid-download.** A video+audio merge downloads as two streams, so
|
||
the bar fills 0→100%, then resets to 0→100% again before muxing — looks like a glitch/restart to the
|
||
user. **Fix:** weight the two phases (e.g. 0–50% / 50–100%) or show a "merging…" state.
|
||
- [x] **SR8 — Window title never reflects state** (L108/W19): always the static "AeroFetch," so the taskbar
|
||
can't show "3 downloading." A finished-feeling app updates its title/tooltip with activity.
|
||
|
||
### Wording / jargon polish
|
||
|
||
- [x] **SR9 — Dev jargon in user-facing option hints** (extends M37): "mux them into the video," "Tiebreaker,
|
||
not a hard filter," "Endcards / credits" ("End cards"), "Proof-of-Origin token," "`--extractor-args`."
|
||
Reads like engineer notes. **Fix:** plain-language hints.
|
||
- [x] **SR10 — Inconsistent product self-description / casing** surfaced to users: "yt-dlp frontend" caption
|
||
flips to "v0.5.0" on load (L66); "Sign in" vs "Sign-in" (L95); "PO token" vs "PO Token" (L150).
|
||
|
||
### Also reduces perceived quality (already filed — listed for completeness)
|
||
|
||
- Onboarding has no folder picker / feels thin (UX5/UX22); no boot loading state + one-frame theme flash
|
||
(UX26); refresh-icon mismatch ArrowClockwise vs ArrowSync (L106) and Regular/Filled brand icon (UI13); only
|
||
the sidebar animates, everything else snaps (UI26); separator glyph/spacing inconsistency `•`/`·` (L164);
|
||
the command palette is undiscoverable (UX21). *(The unsigned-build SmartScreen prompt is accepted as a
|
||
non-goal — no certificate.)*
|
||
|
||
**Ship-blockers (highest perceived-quality impact):** SR1 (light-on-dark first launch), SR7 (progress
|
||
visibly restarting), SR6 ("Resolving…" stuck), and W14 (placeholder icon) — **all now resolved.** The
|
||
unsigned-build SmartScreen prompt is accepted as a non-goal (no certificate). Together these were most of
|
||
the "this feels finished" gap.
|
||
|
||
---
|
||
|
||
## Performance (no premature optimization — real hot paths only)
|
||
|
||
A fresh lens: redundant work on hot paths and scaling cliffs, not micro-tuning. `PERF` IDs. **Verified
|
||
acceptable first:** memoized `QueueItem`s don't re-render when their item is unchanged (the `applyEvent`
|
||
`map` preserves references for untouched items); the queue + large library lists are virtualized;
|
||
`electron-store` is constructed lazily. The issues:
|
||
|
||
- [x] **PERF1 — `getSettings()` is heavy and on every hot path.** It runs per download spawn (twice — in
|
||
`buildCommand` and the `maxConcurrent` check), per completion (`notify`), and on the system-theme bridge;
|
||
each call re-reads the whole store and, for users who've configured a proxy/PO-token/update-token, runs
|
||
up to 3 `safeStorage.decryptString` (DPAPI) calls. **Fix:** cache the decrypted settings, invalidate on write.
|
||
- [x] **PERF2 — `templates.json` is read+parsed on every download.** `resolveExtraArgs` evaluates
|
||
`templates: listTemplates()` eagerly as an argument ([download.ts](src/main/download.ts):182) *before*
|
||
`selectExtraArgs` checks `customCommandEnabled`, so the file is read even when custom commands are off.
|
||
**Fix:** pass a lazy getter, or short-circuit on the gate first.
|
||
- [x] **PERF3 — `pump()` is O(n) per queue event.** It `filter`+`reverse`+`slice`s the entire items array
|
||
on every add/done/error/cancel/progress-driven change; with M33's unbounded channel enqueue (thousands of
|
||
items) every event becomes O(n), and several events fire per second during active downloads. **Fix:**
|
||
track running/queued counts + a pointer instead of rescanning.
|
||
*Resolved with benchmark evidence, no rewrite (Batch 22): [test/pumpBench.test.ts](test/pumpBench.test.ts)
|
||
measures pump()'s exact array pipeline on the real item shape — median **0.43 ms for a full scan+promote
|
||
over 5,000 items** and **1.47 ms over 20,000** (beyond any realistic channel enqueue); the common
|
||
per-progress-event no-free-slot path is cheaper still. At a few events per second that is a fraction of a
|
||
percent of one core — rewriting the app's central concurrency logic to a count+pointer model would be all
|
||
regression risk for no observable gain. The benchmark stays in the suite as a regression tripwire (5 ms
|
||
budget).*
|
||
- [x] **PERF4 — `summarizeQueue` runs twice per progress tick.** Once in App's store subscription (which
|
||
also pushes a taskbar IPC every tick, L25) and once in DownloadsView's render (no `useMemo`, L94) — each
|
||
O(items), every ~second per active download. **Fix:** compute once, memoize, throttle the taskbar push.
|
||
*Fixed with L94: `queueSummaryOf` computes the aggregate once per items change (shared ref cache), so
|
||
App's subscription and DownloadsView's render no longer each run it; the taskbar IPC push was already
|
||
deduped by computed value (L25).*
|
||
- [x] **PERF5 — `VirtualList` gets new function identities each render.** `estimateSize`/`getKey`/
|
||
`renderItem` are passed as inline arrows, so the virtualizer can't rely on stable references (risking
|
||
re-measures/cache churn). **Fix:** `useCallback`/hoist them. *Fixed: the three callbacks are hoisted to
|
||
stable module-level functions in [DownloadsView.tsx](src/renderer/src/components/DownloadsView.tsx).*
|
||
- [x] **PERF6 — Per-thumbnail store subscriptions.** Each `MediaThumb` calls `useResolvedDark()` (two
|
||
store subscriptions) (L162); a long list creates N×2 subscriptions. **Fix:** resolve once, pass as a prop.
|
||
*Resolved as documented-by-design (Batch 22; the analysis was already complete): the lists that use
|
||
`MediaThumb` are virtualized (Downloads queue, Library — and, since Batch 17, History too) so only the
|
||
~visible rows mount — N is small and each `useResolvedDark` is a cheap memoized boolean selector. Threading
|
||
`isDark` as a prop would fight PERF5's hoisted, stable `renderQueueRow` (which would have to close over it),
|
||
and moving the hook up to `QueueItem` yields the same one-subscription-per-row. Low value, mild conflict —
|
||
the current shape is the deliberate choice.*
|
||
- [x] **PERF7 — (ref R3) O(n²) media-items rewrites** dominate the main-process cost when downloading a
|
||
channel; closed by the cached/atomic `jsonStore` (SIMP1) — the single highest-value perf fix.
|
||
- [x] **PERF8 — No code-splitting.** All views (incl. the rarely-used Terminal/Settings) load up front in
|
||
one renderer bundle alongside Fluent UI + React; software rendering (W15) compounds first-paint cost.
|
||
Minor for a desktop app, but lazy-loading heavy views would trim startup. **Fix:** `React.lazy` the heavy tabs.
|
||
*Fixed: Library/History/Terminal/Settings are `React.lazy` chunks behind a `Suspense` in
|
||
[App.tsx](src/renderer/src/App.tsx) (Downloads, the launch tab, stays eager). The production build now emits
|
||
separate `SettingsView` (~90 kB), `LibraryView` (~29 kB), `HistoryView` (~16 kB), and `TerminalView`
|
||
(~8 kB) chunks — ~140 kB out of the initial bundle, loaded on first visit. The stores these views use are
|
||
imported eagerly elsewhere (App's `useDownloads` + `import './store/sources'`, and downloads → history), so
|
||
lazy-loading the *view* never delays store startup or persistence (the C2 note already set this up).*
|
||
|
||
**Priority:** PERF7/R3 (scaling cliff) and PERF1/PERF2 (per-download redundant I/O + crypto) are the only
|
||
ones with real user-visible impact; the rest are good hygiene. None warrant work before the correctness
|
||
bugs (B1, M32, M35) — listed for completeness, not urgency.
|
||
|
||
---
|
||
|
||
## Completed
|
||
|
||
Security & correctness audit (2026-06-23):
|
||
|
||
- [x] **S1** — Backup import: confirm before enabling imported custom-command templates.
|
||
- [x] **S2** — Cookie login window: restrict popups to http(s)/about: schemes.
|
||
- [x] **S3** — Enforce `maxConcurrent` in main (`startDownload`), not just the renderer.
|
||
- [x] **S4** — Path-traversal sanitization for `filenameTemplate` + `outputDir`.
|
||
- [x] **S5** — Per-row validation on persisted JSON reads.
|
||
- [x] **C1 (orig)** — Bundle the missing `ffprobe.exe`; assert ffmpeg+ffprobe presence up front.
|
||
- [x] **P1** — Stop `getSettings()` writing to disk on every read.
|
||
- [x] **P2** — Forward the renderer's pre-probed metadata to main; skip the redundant probe.
|
||
- [x] **P3** — Document "lower maxConcurrent mid-flight doesn't pause overflow".
|
||
- [x] **M1 (orig)** — Extract duplicated `cleanError` to `log.ts`.
|
||
- [x] **M2 (orig)** — Document `parseExtraArgs` quoting limitations.
|
||
- [x] **M3 (orig)** — App icon placeholder (designed asset still wanted pre-v1.0).
|
||
|
||
---
|
||
|
||
## Deferred
|
||
|
||
- Designed app icon before v1.0 (placeholder shipped).
|
||
- Migrate JSON stores to better-sqlite3 if a user indexes many large channels.
|
||
- Metadata editing (`--parse-metadata`/`--replace-in-metadata`) — held pending a live recipe.
|
||
- PO-token WebView auto-minting — only the `--extractor-args` plumbing shipped.
|
||
- ~~Taskbar overlay badge (needs a drawn `NativeImage`)~~ — done (W11): [badge.ts](src/main/badge.ts).
|
||
- i18n — deliberately deferred (note: UI strings are hard-coded throughout).
|
||
- Code signing — **non-goal**: no certificate will be purchased, so builds ship unsigned and the SmartScreen
|
||
prompt is accepted. The build remains signing-ready via `CSC_LINK`/`CSC_KEY_PASSWORD` env vars if this ever
|
||
changes — see [SIGNING.md](docs/SIGNING.md).
|
||
- Live smoke-test of OS-level wiring (tray/jump list/scheduled sync/`aerofetch://`).
|