379048f191
- loadConfig now checks isPlainObject(parsed) before spreading over defaults. Non-object JSON (array, primitive, null) is logged and the app falls back to defaults instead of silently polluting the config with array indices or dropping values. - loadQueue runs every entry through sanitizeQueueItem which validates the status enum, clamps progress to [0, 100], validates customClip and mergeGroup shapes (with sanitizeCustomClip / sanitizeMergeGroup helpers), and demotes stale status="downloading" entries to "pending" with progress=0 on cold start. The previous filter only checked typeof id/url/status === "string" and let through whatever shape customClip / mergeGroup happened to have. - The stale-downloading normalisation fixes a real user trap: after a hard kill mid-download, the queue persisted status="downloading", but no download was running on next launch and start-download only resumed paused items, leaving "downloading" entries stuck. - Bonus: CustomClip and MergeGroupItem imports now have call sites (previously unused-import warnings). docs/IMPROVEMENT_LOG.md gains a Cycle 2 dated section. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
77 lines
9.0 KiB
Markdown
77 lines
9.0 KiB
Markdown
# Improvement Log
|
|
|
|
Dated entries from improvement cycles. Newest at top.
|
|
|
|
## 2026-05-03 — Cycle 2: release pipeline + defensive parsing
|
|
|
|
Three independent improvements landed this cycle.
|
|
|
|
### 1. `scripts/release_gitea.mjs` skips rebuild when artifacts exist (release pipeline)
|
|
|
|
- **File**: `scripts/release_gitea.mjs`.
|
|
- **Problem**: The script unconditionally ran `npm run dist:win` (full test suite + electron-builder) even when the version's artifacts were already on disk under `release/`. When `npm run test:e2e` was broken (cycle 1 follow-up), the release path was unusable — the previous cycle had to bypass the script with direct API uploads via PowerShell. Every future agent would hit the same wall.
|
|
- **Fix**: New `--skip-build` flag. The script now also auto-detects whether all 3 required artifacts (`Setup-<v>.exe`, `Setup-<v>.exe.blockmap`, `latest.yml`) exist for the requested version and skips `dist:win` accordingly. The auto-skip is the safe default — explicit `--skip-build` documents intent. Help text updated to describe the new flag and the auto-skip behaviour.
|
|
|
|
### 2. `playwright` in `devDependencies` + simplified test scripts (release pipeline)
|
|
|
|
- **Files**: `package.json` (+ `package-lock.json`).
|
|
- **Problem**: `npm exec --yes --package=playwright -- node scripts/smoke-test*.js` failed with `MODULE_NOT_FOUND` in environments where `npm exec` couldn't resolve playwright on the fly (clean caches, locked CI runners). Cycle 1 worked around it with `npm install --no-save playwright`. Result: the documented test path was unreliable.
|
|
- **Fix**: `playwright ^1.59.1` added to `devDependencies`. `test:e2e`, `test:e2e:guide`, `test:e2e:full` now invoke `node scripts/smoke-test*.js` directly — `require('playwright')` resolves locally. No browser binary install needed because the smoke tests drive Electron via `_electron`, not a browser.
|
|
|
|
### 3. Defensive parsing in `loadConfig` and `loadQueue` (server-side correctness)
|
|
|
|
- **File**: `src/main.ts` — new `isPlainObject` / `isValidQueueStatus` / `sanitizeCustomClip` / `sanitizeMergeGroup` / `sanitizeQueueItem` helpers; rewritten `loadConfig` and `loadQueue`.
|
|
- **Problem**: `loadConfig` blindly spread `JSON.parse(data)` over the defaults. If the config file ever held a non-object (corrupt, manually edited to an array, partial write before Cycle 1's fsync landed), the spread either dropped values silently (primitives) or polluted the config object (arrays became numeric keys). `loadQueue` only validated `id`, `url`, `status` are strings — it accepted `customClip` / `mergeGroup` of any shape, never validated `progress` was a finite number, and notably never normalized stale `status: 'downloading'` items. After a hard kill mid-download, those items came back marked as still downloading with no actual download running, and `start-download` only resurrected `paused` items, leaving them stuck.
|
|
- **Fix**: `loadConfig` checks `isPlainObject(parsed)` before spread; non-objects are logged and ignored, defaults used. `loadQueue` runs every entry through `sanitizeQueueItem` which validates the `status` enum, normalizes `progress` to `[0, 100]`, validates and normalizes `customClip` / `mergeGroup` shapes, and demotes stale `status: 'downloading'` to `pending` with `progress = 0` so the user can actually resume the queue. Invalid items are dropped with a count logged. As a bonus, the previously-unused `CustomClip` and `MergeGroupItem` type imports now have call sites.
|
|
|
|
### Regression
|
|
|
|
- `npm run build` — clean (TypeScript strict, 0 errors).
|
|
- `npm run test:e2e:update-logic` — passed.
|
|
- `npm run test:e2e` — passed via the new direct script path (no `npm exec` workaround), `issues: []`.
|
|
- `npm run test:e2e:guide` — passed.
|
|
- `npm run test:merge-split` — passed.
|
|
- `npm run test:e2e:full` — passed (`failures: []`, `runtimeIssues: []`; flows: language switch, queue, duplicate prevention, runtime metrics, clip queue, pause/resume, retry, reorder, media cut/merge, update check).
|
|
|
|
## 2026-05-03 — Cycle 1: stability & UX polish
|
|
|
|
Three independent improvements landed this cycle.
|
|
|
|
### 1. Atomic file writes survive power loss / crash mid-write (correctness)
|
|
|
|
- **Files**: `src/main.ts` — new `writeFileAtomicSync` helper, `saveConfig`, `writeQueueToDisk`.
|
|
- **Problem**: `saveConfig` and `writeQueueToDisk` used `writeFileSync` + `renameSync`. Node's `writeFileSync` does NOT call `fsync` — the OS may report the rename complete while the file content still sits in the write cache. A power loss / kernel panic between `writeFileSync` and `renameSync` could leave the renamed file empty or truncated. On next launch, `JSON.parse` throws and the app silently falls back to defaults (config) or `[]` (queue). Users would see "settings reset" / "queue lost" with no diagnostic in the debug log beyond a `console.error`.
|
|
- **Fix**: `openSync(tmp, 'w')` → `writeSync(fd, buffer, 0, len, 0)` → `fsyncSync(fd)` → `closeSync(fd)` → `renameSync`. The `fsyncSync` is wrapped in an inner try (some filesystems reject it, e.g. network shares); failure there is non-fatal but the close + rename order is always preserved. The Windows copy/unlink fallback for "rename failed because target locked" is kept.
|
|
|
|
### 2. Per-item filename claims fix parallel-download race (race condition + dead-code cleanup)
|
|
|
|
- **Files**: `src/main.ts` — `ensureUniqueFilename`, new `releaseClaimedFilenamesForItem`, every download call site, `splitMergedFile` signature.
|
|
- **Problem**: `claimedFilenames` was a global `Set<string>` and `processOneQueueItem` did `claimedFilenames.clear()` in its `finally`. With parallel downloads enabled (max 2), when item A finished, the `clear()` wiped item B's reservations too. In the narrow window between B claiming a filename via `ensureUniqueFilename` and streamlink actually writing the first bytes to disk, a third item entering the freed slot could compute the SAME filename (claim set empty, file not yet on disk) → both downloads would race writing the same path. The dead `releaseClaimedFilename(filePath)` function was defined at line 722 but never called from anywhere.
|
|
- **Fix**: New `Map<itemId, Set<filename>>` tracks which item claimed which filenames. `ensureUniqueFilename(filePath, itemId)` registers per-item; `releaseClaimedFilenamesForItem(itemId)` removes only that item's claims. `splitMergedFile` gained an `itemId` parameter so split-phase claims register correctly. The dead `releaseClaimedFilename` is gone, replaced by the per-item variant.
|
|
|
|
### 3. Renderer UX polish — robust progress lookup, persisted active tab, keyboard shortcuts (client-side feature)
|
|
|
|
- **Files**: `src/renderer-queue.ts`, `src/renderer.ts`.
|
|
- **Problem(s)** (small wins bundled as one coherent UX improvement):
|
|
- `updateQueueItemProgress` indexed `byId('queueList').children[idx]` by array position — fragile if the queue array and DOM ever diverged for a frame (queue mutated after render-fingerprint shortcut, or during the throttled queue-sync window).
|
|
- The active tab always reset to `vods` on app launch — annoying for users who live in `settings`, `cutter`, or `merge`.
|
|
- No way to dismiss any of the three modals (`clipModal`, `templateGuideModal`, `updateModal`) without clicking the close button.
|
|
- No keyboard navigation between tabs (only `Del` and `S` were wired).
|
|
- The page title used to show the streamer name even when the user was on Settings or Cutter, because `showTab` always preferred `currentStreamer` over the tab title.
|
|
- **Fix**:
|
|
- Look up queue items by `[data-id="..."]` selector instead of array index. Resilient to mutation between renders. Determinate / indeterminate progress class logic tightened (`isDeterminate = progress > 0 && progress <= 100`).
|
|
- Active tab persisted to `localStorage` on every `showTab`; restored on init via `loadPersistedActiveTab`, whitelisted to known tab IDs (`vods | clips | cutter | merge | settings`) so a future rename can't strand users on a missing tab. Title logic fixed: streamer name only appears in the page title when the VODs tab is active.
|
|
- `Escape` closes the topmost open modal regardless of focus (priority order: clip dialog → template guide → update modal). Works while typing in a modal input.
|
|
- `Ctrl+1..5` (or `Cmd+1..5` on macOS) jumps directly to a tab. Existing `Del` (delete selected) and `S` (start/pause) shortcuts continue to work and remain blocked while typing in inputs.
|
|
|
|
### Regression
|
|
|
|
- `npm run build` — clean (TypeScript strict, 0 errors, 0 new warnings).
|
|
- `node scripts/smoke-test-update-version-logic.js` — passed.
|
|
- `node scripts/smoke-test-merge-split-logic.js` — passed.
|
|
- `node scripts/smoke-test.js` — passed (37 VODs listed, queue add OK, preflight green, `issues: []`).
|
|
- `node scripts/smoke-test-template-guide.js` — passed (17 variable rows, live preview reactive, `failures: []`).
|
|
- `node scripts/smoke-test-full.js` — passed (`failures: []`, `runtimeIssues: []`; flows verified: language switch, queue add, duplicate prevention, runtime metrics, clip queue, pause/resume, retry, reorder, media cut/merge, update check).
|
|
|
|
ESLint reports 36 pre-existing warnings and 1 pre-existing error (control-character regex in `sanitizeFilenamePart`); none new from this cycle.
|