63f0a40f0e
- L94/PERF4: add queueSummaryOf — a 1-entry items-ref memo over summarizeQueue. The store hands out a new items array per change, so App's taskbar subscription and DownloadsView's render compute the aggregate once per change (second caller hits the cache) instead of twice per tick. summarizeQueue stays pure/tested. - PERF5: hoist VirtualList estimateSize/getKey/renderItem to stable module-level functions so the virtualizer doesn't churn its measure/key cache. - L163: one useShallow selection replaces QueueItem's 10 individual useDownloads subscriptions (stable actions -> never re-renders the row). typecheck + 268 tests + eslint + prettier green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1761 lines
147 KiB
Markdown
1761 lines
147 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.
|
||
|
||
**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.
|
||
- [ ] **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.
|
||
- [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`.
|
||
- [ ] **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.
|
||
- [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`.
|
||
- [ ] **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.
|
||
- [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).
|
||
- [ ] **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.
|
||
- [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:*
|
||
|
||
- [ ] **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).
|
||
- [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`.
|
||
- [ ] **L51 — Scheduled daily sync time hardcoded to 09:00** (schedule.ts) — on/off toggle only, no time picker.
|
||
- [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.
|
||
- [ ] **L64 — `ARIA2C_ARGS` connection params hardcoded** (`-x16 -s16 -k1M`), no user control.
|
||
- [ ] **L65 — Pause uses `taskkill /F`** like cancel — a forced kill risks an unflushed `.part` tail vs a graceful stop.
|
||
- [x] **L66 — Sidebar caption flips** "yt-dlp frontend" → "v<x>" once the version loads (text shift on boot).
|
||
- [ ] **L67 — Onboarding has no Skip and can't be revisited** (no "show tips again").
|
||
- [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).
|
||
- [ ] **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).
|
||
- [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.
|
||
- [ ] **L92 — Preload names diverge from main fn names.** `markSourceItemDownloaded`↔`setMediaItemDownloaded`,
|
||
`syncSources`↔`syncWatchedSources`. Align the names across the boundary.
|
||
- [ ] **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.
|
||
- [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).*
|
||
- [ ] **L95 — "Sign in" hyphenation varies** in the Cookies card ("Sign in…", "Site to sign in to", "Sign-in window", "Signing in…").
|
||
- [ ] **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.
|
||
- [ ] **L97 — Advanced-field placeholders inconsistent.** Adjacent fields use an example (`web_safari`),
|
||
a literal `(none)`, and `Optional — Gitea access token`. Pick one placeholder style.
|
||
- [x] **L98 — "Embed chapters" has no hint** while every sibling toggle in DownloadOptionsForm does.
|
||
- [ ] **L99 — URL input has an `id` but no label.** `input={{ id: 'aerofetch-url' }}` with no
|
||
`<label for>`/`aria-label` (placeholder only) — see M28.
|
||
- [ ] **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.
|
||
- [ ] **L101 — No z-index scale.** CommandPalette and Hint both hardcode `zIndex: 1000`.
|
||
- [ ] **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.
|
||
- [ ] **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`.
|
||
- [ ] **L104 — History isn't virtualized.** It renders all filtered rows, while Downloads and Library use `VirtualList`.
|
||
- [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>`).
|
||
- [ ] **L109 — Versions auto-load with no indicator.** yt-dlp/ffmpeg versions in About just appear;
|
||
only the manual "Check" button shows a spinner.
|
||
- [ ] **L110 — One muted-text token, many class aliases.** `colorNeutralForeground3` is re-declared as
|
||
`hint`/`sub`/`meta`/`srcSub`/`stats`/`count`/`emptyHint`/`previewMeta` across components.
|
||
- [ ] **L111 — Two helper-text mechanisms** — Fluent `Field` `hint` vs manual `<Caption1 className={hint}>`; SettingsView uses both.
|
||
- [ ] **L112 — `Switch` labeling inconsistent.** Settings/DownloadOptionsForm show "On"/"Off" text;
|
||
Library switches use `aria-label` only (no visible state text).
|
||
- [ ] **L113 — Tooltip ≠ aria-label.** The Fetch button's Hint says "Fetch formats / playlist" but its
|
||
`aria-label` says "Fetch formats or playlist" (DownloadBar).
|
||
- [ ] **L114 — Row titles use different components** — `Text` (QueueItem/History/Templates) vs `span` (Library) vs `Caption1` elsewhere.
|
||
- [ ] **L115 — Equivalent row actions labeled inconsistently** — "Re-index" is icon+text; the
|
||
comparable "Retry"/"Remove"/"Re-download" are icon-only.
|
||
- [ ] **L116 — Empty-state structure varies** — Downloads (icon + line), History (colored badge + line + sub-hint), Library (icon + line, no sub-hint).
|
||
- [ ] **L117 — Terminal/error-log "empty" aren't centered blocks** like the other empty states (inline text instead).
|
||
- [ ] **L118 — Cross-component focus via hardcoded id.** App focuses the URL field with
|
||
`document.getElementById('aerofetch-url')` — brittle coupling into DownloadBar's markup.
|
||
- [x] **L119 — "No items" copy varies** — "No … yet" (most) vs "Output will appear here." (Terminal) vs "No errors logged." (Diagnostics).
|
||
- [ ] **L120 — Fluent `Card` vs hand-styled `div` cards.** Only Settings/Onboarding use `<Card>`; every
|
||
other card surface is a bespoke styled `<div>`.
|
||
- [ ] **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.
|
||
- [x] **L122 — "Checking…" indicated twice** — the Settings update button changes its label to "Checking…" *and* a separate Spinner renders beside it.
|
||
- [ ] **L123 — `Caption1` is overloaded** for hints, metadata, counts, and timestamps — one size doing four jobs.
|
||
- [ ] **L124 — `Body1` vs `Text` role overlap** — `Body1` appears only in empty states/onboarding/gate; `Text` carries titles; the split isn't principled.
|
||
- [ ] **L125 — Icon sizing has two syntaxes** — px strings (`fontSize: '20px'`) on icon *containers* vs numeric `fontSize={28}` on inline icons.
|
||
- [ ] **L126 — Uneven text truncation** — some titles/metas use `nowrap`+ellipsis, others wrap; no shared truncation utility.
|
||
- [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).*
|
||
- [ ] **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.
|
||
- [ ] **L129 — `Hint align` is silently ignored for left/right placements** yet callers still pass it.
|
||
- [ ] **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.
|
||
- [ ] **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).
|
||
- [ ] **L132 — `clearProbe` + form-reset logic is duplicated** in DownloadBar (`onUrlChange`, `download`, `addPlaylist`).
|
||
- [ ] **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).
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
|
||
*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.*
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [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).*
|
||
- [ ] **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).
|
||
- [ ] **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).
|
||
- [ ] **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).
|
||
- [ ] **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.
|
||
- [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:*
|
||
|
||
- [ ] **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.
|
||
- [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.
|
||
- [ ] **L152 — Search-field sizing differs per screen.** History `minWidth 180 / maxWidth 320`, Settings
|
||
inline `width: 100%`, DownloadBar `flexGrow`. No shared search-input width.
|
||
- [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.
|
||
- [ ] **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.
|
||
- [x] **L156 — `datetime-local` schedule has no `min`.** Past times are selectable, then silently download
|
||
now (extends L57). Add `min={now}`.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [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.
|
||
- [ ] **L160 — One-off inline margins.** `marginTop` literals (Onboarding 2px, QueueItem 4px, SettingsView
|
||
8px) instead of style classes (extends L5).
|
||
- [ ] **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.
|
||
- [ ] **L162 — Per-thumbnail store subscription.** Every `MediaThumb` calls `useResolvedDark()` (two store
|
||
subscriptions) instead of receiving a resolved theme prop — N subscriptions per list.
|
||
- [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:*
|
||
|
||
- [ ] **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).
|
||
- [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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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).
|
||
|
||
---
|
||
|
||
## 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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.)*
|
||
|
||
### Corner radius
|
||
|
||
- [ ] **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.)*
|
||
- [ ] **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.
|
||
- [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).
|
||
- [ ] **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.
|
||
|
||
### Icons
|
||
|
||
- [ ] **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).
|
||
- [ ] **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).
|
||
|
||
### 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).
|
||
- [ ] **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).
|
||
- [ ] **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.
|
||
- [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.
|
||
- [ ] **UI19 — Suggestion-banner CSS duplicated.** Identical `suggestion`/`suggestionText` styles in
|
||
DownloadBar and LibraryView. **Standard:** a shared `LinkSuggestion` component (also kills drift).
|
||
|
||
### Colors & selection
|
||
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
|
||
### Dialogs, menus, status, navigation
|
||
|
||
- [ ] **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.
|
||
- [ ] **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."
|
||
- [ ] **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).
|
||
|
||
### Animations & transitions
|
||
|
||
- [ ] **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.
|
||
|
||
### 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
|
||
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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."
|
||
- [ ] **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.
|
||
- [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).*
|
||
- [ ] **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).
|
||
- [ ] **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.
|
||
- [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).*
|
||
- [ ] **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."
|
||
|
||
### Low — polish & smaller friction
|
||
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **UX23 — Settings folder paths are read-only** (L71) — no paste; Browse-dialog only.
|
||
- [ ] **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.
|
||
- [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).*
|
||
- [ ] **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.
|
||
|
||
---
|
||
|
||
## 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).
|
||
- [ ] **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`).
|
||
- [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
|
||
|
||
- [ ] **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).
|
||
- [ ] **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).
|
||
- [ ] **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`).
|
||
- [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
|
||
|
||
- [ ] **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).
|
||
- [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`.
|
||
- [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.
|
||
- [ ] **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).
|
||
- [ ] **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).
|
||
- [ ] **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.
|
||
- [ ] **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`.
|
||
- [ ] **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.
|
||
- [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.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
- [ ] **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`.
|
||
- [ ] **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.
|
||
- [ ] **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.
|
||
|
||
### 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.*
|
||
- [ ] **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.
|
||
- [x] **CL5 — Superfluous exports.** `PROGRESS_TEMPLATE` (L167) and `getManagedBinDir` are `export`ed but
|
||
used only within their own module. **Fix:** make them module-private.
|
||
- [ ] **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.
|
||
|
||
**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.
|
||
- [ ] **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.
|
||
- [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.
|
||
- [ ] **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).
|
||
- [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).
|
||
- [ ] **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.
|
||
- [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).
|
||
- [ ] **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.
|
||
- [ ] **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).
|
||
|
||
### 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.
|
||
- [ ] **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.
|
||
- [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).*
|
||
- [ ] **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.
|
||
- [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.
|
||
- [ ] **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.
|
||
|
||
**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`).
|
||
- 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://`).
|