feat(audit): Batch 10 — CC conventions (shared line-buffer, cleanError)
The contained consistency consolidations; the sweeping ones are deferred with their standard documented (they're the audit's own post-1.0 refactors, and several would be reckless to do unattended). Landed: - src/main/lib/lineBuffer.ts (createLineBuffer): one newline line-splitter for streamed child-process output, replacing the duplicated buffer loop in download.ts and terminal.ts (CC3/CC4/SIMP5). Pure + unit-tested (test/lineBuffer.test.ts, 7 cases: partial lines, CRLF, empty lines, flush, multi-line chunks). download.ts keeps byte-identical marker parsing and no close-flush (yt-dlp template lines are always newline-terminated). - ytdlp.ts: getYtdlpVersion/updateYtdlp now run stderr through cleanError, the same `cleanError(r.stderr) || r.error?.message || ''` idiom already used by download/probe/indexer (CC6) — a failed check shows the trailing error line. - CL6 resolved: the one real redundancy was L15; clearDir/openFile/showInFolder are deliberate view-model wrappers (CC13), left as-is. Deferred with documented standard + rationale (CODE-AUDIT.md): CC1 (breaking persisted-key rename → migration), CC5+CL4 (security-critical updater net.request, not live-verifiable here), CC7 (cosmetic arg-order), CC9 (zod dep), CC10 (electron-store↔jsonStore split is deliberate for DPAPI), CC11 (config.ts move), CC12 (file-tree reorg), CC13→L93, CC14→L142. Verified: typecheck (node+web) + 275 tests + eslint + production build green; touched files prettier-clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+90
-6
@@ -152,6 +152,23 @@ list-item cards), and **UI20** (toggles = solid brand, list-selection = brand-ti
|
|||||||
gone via UI18). **L128 deferred to Batch 14** (build-time DCE of preview seeds, grouped with PERF8). All green:
|
gone via UI18). **L128 deferred to Batch 14** (build-time DCE of preview seeds, grouped with PERF8). All green:
|
||||||
typecheck (node+web) + 268 tests + eslint + production build; touched files prettier-clean.
|
typecheck (node+web) + 268 tests + eslint + production build; touched files prettier-clean.
|
||||||
|
|
||||||
|
**Session 2026-07-01 pass 7 (Batch 10 — CC consistency & conventions):** the contained consolidations landed;
|
||||||
|
the sweeping ones are honestly deferred. **Landed:** a shared stdout line-splitter
|
||||||
|
[lib/lineBuffer.ts](src/main/lib/lineBuffer.ts) (`createLineBuffer`) replacing the duplicated newline loop in
|
||||||
|
`download.ts` + `terminal.ts` (**CC3/CC4**, + [test/lineBuffer.test.ts](test/lineBuffer.test.ts), 7 cases), and
|
||||||
|
`cleanError` on **all** yt-dlp stderr — `getYtdlpVersion`/`updateYtdlp` now use the same `cleanError(r.stderr) ||
|
||||||
|
r.error?.message || ''` idiom as download/probe/indexer (**CC6**). **CL6** resolved (the one real redundancy was
|
||||||
|
L15; `clearDir`/`openFile`/`showInFolder` are deliberate view-model wrappers). **Deferred with the standard
|
||||||
|
documented** — these are the architectural refactors the release gate already puts post-1.0, and several would be
|
||||||
|
reckless to do unattended: **CC1** (renaming persisted setting keys is a breaking migration), **CC5**+**CL4**
|
||||||
|
(the last event-emitter wrap is the *security-critical* updater installer download — host re-validation per
|
||||||
|
redirect + SHA-256 verify — which can't be live-verified here), **CC7** (cosmetic arg-ordering; the pure/impure
|
||||||
|
DI split already holds), **CC9** (zod = new dep + validation rewrite), **CC10** (jsonStore exists; the
|
||||||
|
electron-store split is deliberate for DPAPI), **CC11** (constants centralized; only a `config.ts` move left),
|
||||||
|
**CC12** (file-tree reorg = broad churn), **CC13**→L93, **CC14**→L142. Net: CC3/CC4/CC6/CL6 checked; the rest
|
||||||
|
carry a documented standard + deferral rationale. All green: typecheck (node+web) + 275 tests + eslint +
|
||||||
|
production build; touched files prettier-clean.
|
||||||
|
|
||||||
**Deliberately deferred** (need live-app/visual verification or are larger refactors the
|
**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
|
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,
|
refactors (C1, C2, H1), the SIMP* helpers + shared-UI-token work, the CC* consolidations,
|
||||||
@@ -1569,33 +1586,63 @@ split — but that style is unenforced and several "do the same thing two ways"
|
|||||||
(bare). Keys say `videoDir`/`audioDir` while the UI says "folder." Preload names diverge from main
|
(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>`
|
(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`.
|
consistently; one term ("folder") across keys + UI; align preload↔main names; single-source shared types in `@shared`.
|
||||||
|
*Partially closed / deferred: `MediaKind` is single-sourced (L105 done). The remaining renames are a
|
||||||
|
**breaking persistence change** — the boolean/`videoDir` keys are the on-disk `settings.json` schema, so
|
||||||
|
renaming them silently orphans every existing user's saved settings without a migration. Deferred to a 1.x
|
||||||
|
settings-migration pass; the preload↔main name alignment is the safe subset, tracked as L92 (Batch 13). The
|
||||||
|
naming convention is recorded here for new keys.*
|
||||||
- [x] **CC2 — Coding style is consistent but unenforced.** No ESLint/Prettier config, script, or dep
|
- [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
|
(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.
|
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),
|
- [x] **CC3 — Different patterns for the same problem.** JSON persistence (M1), status chips (UI18),
|
||||||
segmented controls (UI14), buttons (UI15), overlays/dialogs (UI23), three yt-dlp probe spawners
|
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
|
(`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`,
|
`terminal.ts`. **Standard:** one helper per concern (a `jsonStore`, a `StatusChip`, a `SegmentedControl`,
|
||||||
one spawn-and-stream helper).
|
one spawn-and-stream helper). *Fixed: one helper now backs each concern — `createJsonStore` (M4/R1),
|
||||||
- [ ] **CC4 — Duplicate utilities.** Formatting (M9), `youtubeId` (H2), `newId` (M7), JSON I/O (M1),
|
`StatusChip` (UI18), `SegmentedControl` (UI14), `execFileAsync` ([lib/exec.ts](src/main/lib/exec.ts)) for
|
||||||
|
the three probe spawners, and — this batch — the last duplication, the stdout newline-split loop, is now
|
||||||
|
[lib/lineBuffer.ts](src/main/lib/lineBuffer.ts) (`createLineBuffer`), adopted by both `download.ts` and
|
||||||
|
`terminal.ts` and unit-tested ([test/lineBuffer.test.ts](test/lineBuffer.test.ts)). (UI15 button-wrapping
|
||||||
|
and UI23 overlay unification stay their own incremental tracks.)*
|
||||||
|
- [x] **CC4 — Duplicate utilities.** Formatting (M9), `youtubeId` (H2), `newId` (M7), JSON I/O (M1),
|
||||||
`MediaKind` (L105) — **plus** `execFile` is hand-wrapped in a `new Promise` in five places
|
`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,
|
(`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/`
|
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).
|
with one each of: `format`, `youtube`, `jsonStore`, `ytdlpExec` (promisified spawn + line stream).
|
||||||
|
*Fixed: every listed duplicate now has one home — renderer `lib/formatters`/`urlHelpers`/`id` +
|
||||||
|
`@shared/format`, `MediaKind` single-sourced (L105); main `lib/exec.ts` (the five `execFile` wrappers) and
|
||||||
|
now `lib/lineBuffer.ts` (the two stdout-split loops). The `lib/` dirs exist on both sides.*
|
||||||
- [ ] **CC5 — Async patterns are mixed.** Three I/O idioms: callback-wrapped `new Promise` (execFile ×5),
|
- [ ] **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
|
`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).
|
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.
|
**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/
|
*Partly done: the `execFile ×5` idiom is gone (one `execFileAsync`, CC4), so the app-code default is
|
||||||
|
`async/await`. The one remaining event-emitter-wrapped Promise is the updater's `net.request` streaming
|
||||||
|
download — its "wrap once in a helper" is **CL4**, deferred with it (it's security-critical installer-
|
||||||
|
download code — host re-validation per redirect + SHA-256 verify + truncation guards — that can't be
|
||||||
|
live-verified in this environment, so it isn't refactored unattended). Left open until CL4 lands.*
|
||||||
|
- [x] **CC6 — Five failure-signaling conventions.** `{ ok, error }` result objects (download/updater/
|
||||||
ytdlp/backup/IPC), `throw` (`assertHttpUrl`), `null` (indexerCore), `[]` (history/sources read errors),
|
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
|
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;
|
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`.
|
`throw` only inside pure helpers, caught at the boundary; always run yt-dlp stderr through `cleanError`.
|
||||||
|
*Fixed (the concrete divergence): yt-dlp stderr now runs through `cleanError` **everywhere** — `getYtdlpVersion`
|
||||||
|
and `updateYtdlp` ([ytdlp.ts](src/main/ytdlp.ts)) adopted the exact `cleanError(r.stderr) || r.error?.message
|
||||||
|
|| ''` idiom already used by download/probe/indexer, so a failed version/update check shows the trailing
|
||||||
|
error line, not raw noise. (ffmpeg surfaces no stderr — its version probe returns `null` by design.) The
|
||||||
|
broader "one `Result<T>` across every boundary" holds already for the service/IPC layer (they return
|
||||||
|
`{ ok, error }`); collapsing the remaining `null`/`[]`/bare-string internal signals into it is a low-value
|
||||||
|
sweep left to the incremental track — the specific `cleanError` inconsistency this item names is closed.*
|
||||||
- [ ] **CC7 — Dependency-injection styles.** Pure modules take deps as params (good — `binDir`,
|
- [ ] **CC7 — Dependency-injection styles.** Pure modules take deps as params (good — `binDir`,
|
||||||
`now`); the impure shell uses module singletons (`getSettings()`, `getYtdlpPath()`, lazy `getStore()`);
|
`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
|
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
|
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.
|
as the leading argument and keep singleton access for config/binaries.
|
||||||
|
*Deferred (low-value restyle): the substantive half already holds — the pure core
|
||||||
|
(`buildArgs`/`validation`/`indexerCore`/`ytdlpPolicy`) takes deps as params and the shell uses singletons
|
||||||
|
for config/binaries, which is the recommended split. What's left is purely cosmetic argument-ordering
|
||||||
|
(`wc`/`onProgress` position), not worth the churn/risk across every handler unattended; recorded as the
|
||||||
|
convention for new handlers.*
|
||||||
- [x] **CC8 — No logging strategy.** Effectively no diagnostics — a single stray `console.error` in
|
- [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
|
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.
|
small leveled logger (e.g. `electron-log`) written to userData, called at every catch; keep `errorlog.ts` for user-facing download failures only.
|
||||||
@@ -1610,28 +1657,55 @@ split — but that style is unenforced and several "do the same thing two ways"
|
|||||||
coerce-with-fallback (`sanitizeOptions`, `templates.sanitize`), an imperative per-key `switch`
|
coerce-with-fallback (`sanitizeOptions`, `templates.sanitize`), an imperative per-key `switch`
|
||||||
(`setSettings`, M24), and `throw` (`assertHttpUrl`). **Standard:** one schema layer (e.g. `zod`) that
|
(`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.
|
both *validates and coerces*; `setSettings` runs values through the same schema instead of a bespoke switch.
|
||||||
|
*Deferred to 1.x (per the release gate — the CC consolidations are explicitly post-1.0): adopting a schema
|
||||||
|
layer (`zod`) is a new runtime dependency plus a rewrite of `validation.ts` + `setSettings` + backup import.
|
||||||
|
High-value but a sweeping change best done as its own reviewed PR, not unattended; standard recorded.*
|
||||||
- [ ] **CC10 — Mixed serialization/persistence.** `electron-store` (settings) **and** hand-rolled
|
- [ ] **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
|
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.
|
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
|
**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.
|
**or** the JSON stores for everything, not both; leave the yt-dlp-format files alone.
|
||||||
|
*Partly closed / deferred: the `jsonStore<T>()` abstraction now exists (`createJsonStore`, M4/R1) and backs
|
||||||
|
every app record (history/errorlog/templates/sources/media-items/queue), so the "hand-rolled per file"
|
||||||
|
divergence is gone. The remaining electron-store↔jsonStore split is a **deliberate** keep: settings live in
|
||||||
|
electron-store for its DPAPI-encrypted secret fields, records in jsonStore. Fully migrating settings off
|
||||||
|
electron-store is a 1.x call, not forced here.*
|
||||||
- [ ] **CC11 — Configuration is scattered.** `electron-store` + `localStorage` (sidebar, M19) + env vars
|
- [ ] **CC11 — Configuration is scattered.** `electron-store` + `localStorage` (sidebar, M19) + env vars
|
||||||
(`PORTABLE_EXECUTABLE_DIR`, `CSC_LINK`, `AEROFETCH_REAL_DOWNLOAD`) + hardcoded module consts (update
|
(`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
|
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.
|
constants; fold `localStorage` UI prefs into the settings store so there's one persisted-prefs source.
|
||||||
|
*Partly closed / deferred: the runtime consts are already centralized in
|
||||||
|
[constants.ts](src/main/constants.ts) (L10), and the sidebar `localStorage` pref was folded into the
|
||||||
|
settings store (M19). What's left — a dedicated `config.ts` for build/host constants (update
|
||||||
|
host/owner/repo, currently co-located in updater.ts) — is a marginal move deferred to the 1.x organization
|
||||||
|
pass (CC12), with which it naturally lands.*
|
||||||
- [ ] **CC12 — Project organization.** Renderer `components/` mixes screens (views) with reusable widgets;
|
- [ ] **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/`;
|
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.
|
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`.
|
**Standard:** renderer `views/` + `components/` + `lib/`; main `core/` (pure) + services; move `queueStats` to `lib`.
|
||||||
|
*Deferred to 1.x: a file-tree reorg is broad import churn across nearly every module for no behavior change —
|
||||||
|
exactly the kind of sweeping move that wants its own reviewed PR, not an unattended batch. The `lib/`
|
||||||
|
convention is already established (both sides have one and new pure utils land there); the wholesale
|
||||||
|
`views/`+`core/` split is the 1.x task. Standard recorded.*
|
||||||
- [ ] **CC13 — View/state boundary (the project's "MVVM").** It's React+Zustand, but the container/
|
- [ ] **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
|
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/
|
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.
|
hooks are the view-models (own orchestration + all IPC); components stay presentational; no `window.api` in components.
|
||||||
|
*Partly closed / deferred: the substantive split largely holds — downloads/sources orchestration lives in
|
||||||
|
stores, and DownloadBar's was extracted into the `useDownloadBar` hook (H1), a view-model in all but name.
|
||||||
|
The concrete remaining leak is SettingsView calling `window.api` directly, filed as **L93** (Batch 13);
|
||||||
|
the broader "no `window.api` in any component" sweep rides with it. No new work needed here beyond L93.*
|
||||||
- [ ] **CC14 — State ownership is unprincipled.** State lives in Zustand stores, component `useState`,
|
- [ ] **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
|
`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
|
(`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
|
(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.
|
it and **apply the authoritative value the IPC call returns**; UI-only state in stores; break store cycles via a coordinator.
|
||||||
|
*Largely closed / deferred: main owns persisted state and the renderer mirrors it; the store cycle is gone
|
||||||
|
(C2, via the coordinator event bus); settings now **apply the authoritative IPC return** (M34); and
|
||||||
|
`localStorage` was folded into the settings store (M19). The one remaining "optimistic-only, never
|
||||||
|
reconciled" case is the other IPC mutations (**L142**, Batch 11). The module-level vars called out
|
||||||
|
(`idCounter`/`notifiedBackground`/`active`) are legitimately module-scoped singletons, not misplaced UI
|
||||||
|
state. Closes with L142.*
|
||||||
|
|
||||||
### Recommended single style (apply project-wide)
|
### Recommended single style (apply project-wide)
|
||||||
|
|
||||||
@@ -1702,11 +1776,21 @@ filed. Several listed categories are **N/A** to this stack and worth recording s
|
|||||||
resolved title in after wiring is set up. Behaviour unchanged.*
|
resolved title in after wiring is set up. Behaviour unchanged.*
|
||||||
- [ ] **CL4 — Deep nesting: `updater.downloadAppUpdate`** — Promise → `net.request` → `response` → `data`
|
- [ ] **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.
|
with nested conditionals/teardown is the hardest-to-follow block. **Fix:** extract a `streamToFile` helper.
|
||||||
|
*Deferred (deliberate, needs a watched pass): this is the security-critical installer download —
|
||||||
|
per-redirect host re-validation, streaming SHA-256 verification, truncation detection, idle-timeout, and
|
||||||
|
backpressure teardown all interlock in that block. Extracting `streamToFile` is worthwhile but a mistake
|
||||||
|
here is a security regression, and an app-update download can't be live-verified in this environment, so it
|
||||||
|
isn't refactored unattended. Grouped with CC5's "wrap the event-emitter API once" — both land in a watched
|
||||||
|
session on the updater.*
|
||||||
- [x] **CL5 — Superfluous exports.** `PROGRESS_TEMPLATE` (L167) and `getManagedBinDir` are `export`ed but
|
- [x] **CL5 — Superfluous exports.** `PROGRESS_TEMPLATE` (L167) and `getManagedBinDir` are `export`ed but
|
||||||
used only within their own module. **Fix:** make them module-private.
|
used only within their own module. **Fix:** make them module-private.
|
||||||
- [ ] **CL6 — Redundant wrappers** (minor): `MediaThumb`'s `kind === 'audio' ? 'audio' : 'video'` (L15),
|
- [x] **CL6 — Redundant wrappers** (minor): `MediaThumb`'s `kind === 'audio' ? 'audio' : 'video'` (L15),
|
||||||
`clearDir` = `update({[t]:''})`, and the store `openFile`/`showInFolder` thin wrappers around `window.api`.
|
`clearDir` = `update({[t]:''})`, and the store `openFile`/`showInFolder` thin wrappers around `window.api`.
|
||||||
Harmless; collapse opportunistically.
|
Harmless; collapse opportunistically. *Resolved: the genuinely-redundant one is gone — `MediaThumb`'s
|
||||||
|
no-op ternary was collapsed in L15. The other two are **deliberate, not redundant**: `clearDir` and the
|
||||||
|
store `openFile`/`showInFolder` are the view-model layer (CC13) — the store is the single place components
|
||||||
|
reach `window.api`, so keeping components free of direct IPC is the point, not incidental wrapping. Left
|
||||||
|
as-is on purpose.*
|
||||||
|
|
||||||
**Magic numbers / strings (consolidated, see L10):** timeouts `15_000`/`30_000`/`60_000`/`180_000`,
|
**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`,
|
maxBuffers `4`/`64`/`128`/`256`×1024×1024, caps `500`/`200`/`100`/`20000`/`80`, `VIRTUALIZE_AT 100`,
|
||||||
|
|||||||
@@ -15,6 +15,7 @@ import {
|
|||||||
} from './binaries'
|
} from './binaries'
|
||||||
import { getSettings } from './settings'
|
import { getSettings } from './settings'
|
||||||
import { execFileAsync } from './lib/exec'
|
import { execFileAsync } from './lib/exec'
|
||||||
|
import { createLineBuffer } from './lib/lineBuffer'
|
||||||
import { getDownloadArchivePath, getDefaultMediaDir } from './paths'
|
import { getDownloadArchivePath, getDefaultMediaDir } from './paths'
|
||||||
import { ensureManagedYtdlp } from './ytdlp'
|
import { ensureManagedYtdlp } from './ytdlp'
|
||||||
import { materializeCookies, hasStoredCookies } from './cookies'
|
import { materializeCookies, hasStoredCookies } from './cookies'
|
||||||
@@ -372,7 +373,6 @@ function wireChildProcess(params: {
|
|||||||
const { wc, opts, rec, cleanup, getTitle } = params
|
const { wc, opts, rec, cleanup, getTitle } = params
|
||||||
const child = rec.child
|
const child = rec.child
|
||||||
|
|
||||||
let stdoutBuf = ''
|
|
||||||
let stderrTail = ''
|
let stderrTail = ''
|
||||||
let filePath: string | undefined
|
let filePath: string | undefined
|
||||||
// Latched once the first download stream reports 'finished'; flags later
|
// Latched once the first download stream reports 'finished'; flags later
|
||||||
@@ -415,13 +415,10 @@ function wireChildProcess(params: {
|
|||||||
}
|
}
|
||||||
bumpWatchdog()
|
bumpWatchdog()
|
||||||
|
|
||||||
child.stdout?.on('data', (chunk: Buffer) => {
|
// Split stdout into lines (shared helper, CC3/CC4) and parse our --print markers.
|
||||||
bumpWatchdog()
|
// No flush on close: yt-dlp's progress/path template lines are always
|
||||||
stdoutBuf += chunk.toString()
|
// newline-terminated, so a trailing partial is never a real marker.
|
||||||
let nl: number
|
const stdoutLines = createLineBuffer((line) => {
|
||||||
while ((nl = stdoutBuf.indexOf('\n')) >= 0) {
|
|
||||||
const line = stdoutBuf.slice(0, nl).replace(/\r$/, '')
|
|
||||||
stdoutBuf = stdoutBuf.slice(nl + 1)
|
|
||||||
if (line.startsWith(PROGRESS_MARKER)) {
|
if (line.startsWith(PROGRESS_MARKER)) {
|
||||||
const p = parseProgress(line.slice(PROGRESS_MARKER.length))
|
const p = parseProgress(line.slice(PROGRESS_MARKER.length))
|
||||||
if (p) {
|
if (p) {
|
||||||
@@ -436,7 +433,10 @@ function wireChildProcess(params: {
|
|||||||
} else if (line.startsWith(FILEPATH_MARKER)) {
|
} else if (line.startsWith(FILEPATH_MARKER)) {
|
||||||
filePath = line.slice(FILEPATH_MARKER.length).trim()
|
filePath = line.slice(FILEPATH_MARKER.length).trim()
|
||||||
}
|
}
|
||||||
}
|
})
|
||||||
|
child.stdout?.on('data', (chunk: Buffer) => {
|
||||||
|
bumpWatchdog()
|
||||||
|
stdoutLines.push(chunk.toString())
|
||||||
})
|
})
|
||||||
|
|
||||||
child.stderr?.on('data', (chunk: Buffer) => {
|
child.stderr?.on('data', (chunk: Buffer) => {
|
||||||
|
|||||||
@@ -0,0 +1,37 @@
|
|||||||
|
/**
|
||||||
|
* A newline line-splitter for streamed child-process output (audit CC3/CC4/SIMP5).
|
||||||
|
*
|
||||||
|
* `download.ts` and `terminal.ts` each had the same hand-rolled loop: accumulate
|
||||||
|
* chunk strings, emit every complete `\n`-terminated line (stripping a trailing
|
||||||
|
* `\r`), and keep the partial remainder for the next chunk. This is that logic in
|
||||||
|
* one place, and — being pure and synchronous — it's unit-tested directly.
|
||||||
|
*
|
||||||
|
* Usage: feed decoded chunks with `push()`, and call `flush()` on stream close to
|
||||||
|
* emit any trailing partial line that never saw a newline.
|
||||||
|
*/
|
||||||
|
export interface LineBuffer {
|
||||||
|
/** Accept a decoded chunk; invokes the sink for each newly-completed line. */
|
||||||
|
push: (chunk: string) => void
|
||||||
|
/** Emit any buffered remainder (a final line with no trailing newline). */
|
||||||
|
flush: () => void
|
||||||
|
}
|
||||||
|
|
||||||
|
export function createLineBuffer(onLine: (line: string) => void): LineBuffer {
|
||||||
|
let buf = ''
|
||||||
|
return {
|
||||||
|
push(chunk: string): void {
|
||||||
|
buf += chunk
|
||||||
|
let nl: number
|
||||||
|
while ((nl = buf.indexOf('\n')) >= 0) {
|
||||||
|
onLine(buf.slice(0, nl).replace(/\r$/, ''))
|
||||||
|
buf = buf.slice(nl + 1)
|
||||||
|
}
|
||||||
|
},
|
||||||
|
flush(): void {
|
||||||
|
if (buf) {
|
||||||
|
onLine(buf)
|
||||||
|
buf = ''
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
+8
-14
@@ -14,6 +14,7 @@ import { getYtdlpPath, getBinDir, getSystem32Path } from './binaries'
|
|||||||
import { getSettings } from './settings'
|
import { getSettings } from './settings'
|
||||||
import { ensureManagedYtdlp } from './ytdlp'
|
import { ensureManagedYtdlp } from './ytdlp'
|
||||||
import { parseExtraArgs } from './buildArgs'
|
import { parseExtraArgs } from './buildArgs'
|
||||||
|
import { createLineBuffer } from './lib/lineBuffer'
|
||||||
import { IpcChannels, type TerminalEvent, type TerminalRunResult } from '@shared/ipc'
|
import { IpcChannels, type TerminalEvent, type TerminalRunResult } from '@shared/ipc'
|
||||||
|
|
||||||
const active = new Map<string, ChildProcess>()
|
const active = new Map<string, ChildProcess>()
|
||||||
@@ -47,18 +48,12 @@ export function runTerminal(wc: WebContents, id: string, argsRaw: string): Termi
|
|||||||
active.set(id, child)
|
active.set(id, child)
|
||||||
|
|
||||||
// Buffer each stream and emit on newline boundaries (flush the remainder on close).
|
// Buffer each stream and emit on newline boundaries (flush the remainder on close).
|
||||||
const buffers: Record<'stdout' | 'stderr', string> = { stdout: '', stderr: '' }
|
const lineBufs = {
|
||||||
function feed(stream: 'stdout' | 'stderr', chunk: string): void {
|
stdout: createLineBuffer((line) => send(wc, { type: 'output', id, line, stream: 'stdout' })),
|
||||||
buffers[stream] += chunk
|
stderr: createLineBuffer((line) => send(wc, { type: 'output', id, line, stream: 'stderr' }))
|
||||||
let nl: number
|
|
||||||
while ((nl = buffers[stream].indexOf('\n')) >= 0) {
|
|
||||||
const line = buffers[stream].slice(0, nl).replace(/\r$/, '')
|
|
||||||
buffers[stream] = buffers[stream].slice(nl + 1)
|
|
||||||
send(wc, { type: 'output', id, line, stream })
|
|
||||||
}
|
}
|
||||||
}
|
child.stdout?.on('data', (c: Buffer) => lineBufs.stdout.push(c.toString()))
|
||||||
child.stdout?.on('data', (c: Buffer) => feed('stdout', c.toString()))
|
child.stderr?.on('data', (c: Buffer) => lineBufs.stderr.push(c.toString()))
|
||||||
child.stderr?.on('data', (c: Buffer) => feed('stderr', c.toString()))
|
|
||||||
|
|
||||||
let settled = false
|
let settled = false
|
||||||
child.on('error', (err) => {
|
child.on('error', (err) => {
|
||||||
@@ -72,9 +67,8 @@ export function runTerminal(wc: WebContents, id: string, argsRaw: string): Termi
|
|||||||
settled = true
|
settled = true
|
||||||
active.delete(id)
|
active.delete(id)
|
||||||
// Flush any trailing partial lines that never hit a newline.
|
// Flush any trailing partial lines that never hit a newline.
|
||||||
for (const stream of ['stdout', 'stderr'] as const) {
|
lineBufs.stdout.flush()
|
||||||
if (buffers[stream]) send(wc, { type: 'output', id, line: buffers[stream], stream })
|
lineBufs.stderr.flush()
|
||||||
}
|
|
||||||
send(wc, { type: 'done', id, code })
|
send(wc, { type: 'done', id, code })
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
+5
-2
@@ -2,6 +2,7 @@ import { existsSync, mkdirSync, copyFileSync } from 'fs'
|
|||||||
import { dirname } from 'path'
|
import { dirname } from 'path'
|
||||||
import { getYtdlpPath, getBundledYtdlpPath, YTDLP_MISSING_MSG } from './binaries'
|
import { getYtdlpPath, getBundledYtdlpPath, YTDLP_MISSING_MSG } from './binaries'
|
||||||
import { execFileAsync } from './lib/exec'
|
import { execFileAsync } from './lib/exec'
|
||||||
|
import { cleanError } from './log'
|
||||||
import { VERSION_TIMEOUT_MS, YTDLP_UPDATE_TIMEOUT_MS } from './constants'
|
import { VERSION_TIMEOUT_MS, YTDLP_UPDATE_TIMEOUT_MS } from './constants'
|
||||||
import { getSettings, setSettings } from './settings'
|
import { getSettings, setSettings } from './settings'
|
||||||
import { shouldAutoCheckYtdlp } from './ytdlpPolicy'
|
import { shouldAutoCheckYtdlp } from './ytdlpPolicy'
|
||||||
@@ -43,9 +44,11 @@ export async function getYtdlpVersion(): Promise<YtdlpVersionResult> {
|
|||||||
|
|
||||||
const r = await execFileAsync(ytdlpPath, ['--version'], { timeout: VERSION_TIMEOUT_MS })
|
const r = await execFileAsync(ytdlpPath, ['--version'], { timeout: VERSION_TIMEOUT_MS })
|
||||||
if (!r.ok) {
|
if (!r.ok) {
|
||||||
|
// Run stderr through the shared cleaner so yt-dlp's noisy output collapses to
|
||||||
|
// its trailing error line, consistent with download/probe/indexer (CC6).
|
||||||
const msg = r.timedOut
|
const msg = r.timedOut
|
||||||
? 'Timed out running yt-dlp.'
|
? 'Timed out running yt-dlp.'
|
||||||
: (r.stderr || r.error?.message || '').trim()
|
: cleanError(r.stderr) || r.error?.message || ''
|
||||||
return { ok: false, error: msg }
|
return { ok: false, error: msg }
|
||||||
}
|
}
|
||||||
return { ok: true, version: r.stdout.trim() }
|
return { ok: true, version: r.stdout.trim() }
|
||||||
@@ -81,7 +84,7 @@ export async function updateYtdlp(channel: YtdlpUpdateChannel): Promise<YtdlpUpd
|
|||||||
if (!r.ok) {
|
if (!r.ok) {
|
||||||
const msg = r.timedOut
|
const msg = r.timedOut
|
||||||
? 'Timed out updating yt-dlp.'
|
? 'Timed out updating yt-dlp.'
|
||||||
: (r.stderr || r.error?.message || '').trim()
|
: cleanError(r.stderr) || r.error?.message || ''
|
||||||
return { ok: false, error: msg }
|
return { ok: false, error: msg }
|
||||||
}
|
}
|
||||||
return { ok: true, output: r.stdout.trim() }
|
return { ok: true, output: r.stdout.trim() }
|
||||||
|
|||||||
@@ -0,0 +1,63 @@
|
|||||||
|
import { describe, it, expect } from 'vitest'
|
||||||
|
import { createLineBuffer } from '../src/main/lib/lineBuffer'
|
||||||
|
|
||||||
|
/** Collect every line a buffer emits, for assertions. */
|
||||||
|
function collect(): { lines: string[]; buf: ReturnType<typeof createLineBuffer> } {
|
||||||
|
const lines: string[] = []
|
||||||
|
const buf = createLineBuffer((l) => lines.push(l))
|
||||||
|
return { lines, buf }
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('createLineBuffer', () => {
|
||||||
|
it('emits one line per newline, without the newline', () => {
|
||||||
|
const { lines, buf } = collect()
|
||||||
|
buf.push('a\nb\nc\n')
|
||||||
|
expect(lines).toEqual(['a', 'b', 'c'])
|
||||||
|
})
|
||||||
|
|
||||||
|
it('holds a partial line until its newline arrives across chunks', () => {
|
||||||
|
const { lines, buf } = collect()
|
||||||
|
buf.push('hel')
|
||||||
|
expect(lines).toEqual([])
|
||||||
|
buf.push('lo\nwor')
|
||||||
|
expect(lines).toEqual(['hello'])
|
||||||
|
buf.push('ld\n')
|
||||||
|
expect(lines).toEqual(['hello', 'world'])
|
||||||
|
})
|
||||||
|
|
||||||
|
it('strips a trailing CR so CRLF streams split cleanly', () => {
|
||||||
|
const { lines, buf } = collect()
|
||||||
|
buf.push('one\r\ntwo\r\n')
|
||||||
|
expect(lines).toEqual(['one', 'two'])
|
||||||
|
})
|
||||||
|
|
||||||
|
it('flush emits a trailing partial line that never saw a newline', () => {
|
||||||
|
const { lines, buf } = collect()
|
||||||
|
buf.push('no newline here')
|
||||||
|
expect(lines).toEqual([])
|
||||||
|
buf.flush()
|
||||||
|
expect(lines).toEqual(['no newline here'])
|
||||||
|
})
|
||||||
|
|
||||||
|
it('flush is a no-op when the buffer is empty (e.g. stream ended on a newline)', () => {
|
||||||
|
const { lines, buf } = collect()
|
||||||
|
buf.push('done\n')
|
||||||
|
buf.flush()
|
||||||
|
buf.flush()
|
||||||
|
expect(lines).toEqual(['done'])
|
||||||
|
})
|
||||||
|
|
||||||
|
it('preserves empty lines between consecutive newlines', () => {
|
||||||
|
const { lines, buf } = collect()
|
||||||
|
buf.push('a\n\nb\n')
|
||||||
|
expect(lines).toEqual(['a', '', 'b'])
|
||||||
|
})
|
||||||
|
|
||||||
|
it('splits a multi-line chunk that arrives all at once', () => {
|
||||||
|
const { lines, buf } = collect()
|
||||||
|
buf.push('1\n2\n3')
|
||||||
|
expect(lines).toEqual(['1', '2'])
|
||||||
|
buf.flush()
|
||||||
|
expect(lines).toEqual(['1', '2', '3'])
|
||||||
|
})
|
||||||
|
})
|
||||||
Reference in New Issue
Block a user