Self-review fixes: revert ineffective/regression-risk changes from audit batch
Addressing issues found reviewing this session's audit commits:
REGRESSIONS FIXED
- updater.ts (L52): reverted the spawn(detached) installer launch. Raw
CreateProcess/spawn fails with ERROR_ELEVATION_REQUIRED if the NSIS build
flips to perMachine (the config comment invites this), and it silently
dropped shell.openPath's launch-error detection. Restored shell.openPath
(ShellExecute honors the elevation manifest) and kept the L52 goal by
replacing the 1500ms timer with setImmediate(app.quit).
- LibraryView.tsx (M15): native <button> doesn't inherit color (UA sets
ButtonText), so the source-card chevron (currentColor) would render wrong
in dark mode. Added color: colorNeutralForeground1 to cardHead.
INEFFECTIVE CHANGE REVERTED
- base.css (W18): forced-color-adjust:auto on * is the CSS default (no-op),
and [class*="backdrop"] never matches Fluent/Griffel's hashed atomic class
names. Reverted; W18 unmarked (needs real High-Contrast testing).
WEAK FIX REVERTED
- Onboarding.tsx (L67): "Skip" and "Get started" called the identical handler
on a single-screen onboarding. Removed the duplicate button; L67 unmarked
(real ask is a "show tips again" revisit affordance).
STYLE / CORRECTNESS
- Stripped UTF-8 BOMs accidentally added to Select/CommandPalette/App/
Onboarding by the PowerShell sanitizer; left pre-existing BOMs untouched.
- DownloadBar.tsx: merged the duplicate @shared/ipc import; removed a stray
double blank line.
- qualityOptions.ts (L29): restored the original `satisfies Record<MediaKind,
readonly string[]>` constraint + the noUncheckedIndexedAccess rationale
comment (the extraction had dropped both for a weaker `as const`).
- downloads.ts: removed double blank line left by the QUALITY_OPTIONS move.
- binaries.ts: corrected YTDLP_MISSING_MSG doc comment ("at startup" -> the
actual call sites).
typecheck + 242 tests + eslint all green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+2
-2
@@ -501,7 +501,7 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n
|
||||
- [ ] **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).
|
||||
- [x] **L67 — Onboarding has no Skip and can't be revisited** (no "show tips again").
|
||||
- [ ] **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.
|
||||
- [ ] **L69 — `settings.ts` mixes path helpers with persistence** (`getDefaultMediaDir`/
|
||||
@@ -1042,7 +1042,7 @@ DPI handled by Chromium). The deviations:
|
||||
|
||||
- [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
|
||||
- [ ] **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.
|
||||
- [ ] **(ref) No semantic headings / radiogroup arrow-nav / focus rings** — UI33, UI30, UI28–29 (all bear on Narrator + keyboard users).
|
||||
|
||||
@@ -80,7 +80,7 @@ export function getFfprobePath(): string {
|
||||
return join(getBinDir(), 'ffprobe.exe')
|
||||
}
|
||||
|
||||
/** User-facing error shown whenever yt-dlp.exe is not found at startup. */
|
||||
/** User-facing error shown by download/probe/index/update when yt-dlp.exe is missing. */
|
||||
export const YTDLP_MISSING_MSG =
|
||||
'yt-dlp.exe is missing. Open Settings → Software update to re-download it.'
|
||||
|
||||
|
||||
+9
-8
@@ -1,5 +1,4 @@
|
||||
import { app, net, type WebContents } from 'electron'
|
||||
import { spawn } from 'child_process'
|
||||
import { app, net, shell, type WebContents } from 'electron'
|
||||
import { createWriteStream, type WriteStream } from 'fs'
|
||||
import { stat, unlink } from 'fs/promises'
|
||||
import { join, normalize, dirname } from 'path'
|
||||
@@ -426,12 +425,14 @@ export async function runAppUpdate(filePath: string): Promise<{ ok: boolean; err
|
||||
return { ok: false, error: 'Refused to run an unexpected file.' }
|
||||
}
|
||||
await stat(target) // throws if the file is missing
|
||||
// Spawn the installer detached + unref'd so it outlives our process even on a
|
||||
// slow machine. shell.openPath goes through ShellExecute but gives us no child
|
||||
// PID to wait on; spawning directly lets us call unref() before we quit.
|
||||
const child = spawn(target, [], { detached: true, stdio: 'ignore', shell: false })
|
||||
child.unref()
|
||||
// Let the IPC response reach the renderer before we exit.
|
||||
// Hand the installer to the OS via ShellExecute, which (unlike a raw
|
||||
// CreateProcess/spawn) honors the NSIS elevation manifest — needed if the
|
||||
// build ever flips to perMachine. openPath resolves once the launch is
|
||||
// initiated, so the installer process already exists when we quit.
|
||||
const err = await shell.openPath(target)
|
||||
if (err) return { ok: false, error: err }
|
||||
// Quit on the next tick so this IPC response reaches the renderer first; the
|
||||
// launched installer is independent of our process and survives the quit.
|
||||
setImmediate(() => app.quit())
|
||||
return { ok: true }
|
||||
} catch (e) {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useState, useEffect, useMemo, useRef } from 'react'
|
||||
import { useState, useEffect, useMemo, useRef } from 'react'
|
||||
import { FluentProvider, makeStyles, tokens } from '@fluentui/react-components'
|
||||
import { Sidebar, type TabValue } from './components/Sidebar'
|
||||
import { DownloadsView } from './components/DownloadsView'
|
||||
|
||||
@@ -2,17 +2,6 @@
|
||||
*::before,
|
||||
*::after {
|
||||
box-sizing: border-box;
|
||||
/* Let Windows High Contrast mode override all colors by default (W18). */
|
||||
forced-color-adjust: auto;
|
||||
}
|
||||
|
||||
/* CommandPalette backdrop uses a hardcoded rgba — replace it with the standard HC
|
||||
overlay so the backdrop remains perceptible under forced-colors. */
|
||||
@media (forced-colors: active) {
|
||||
[class*="backdrop"] {
|
||||
background-color: Canvas;
|
||||
border: 2px solid ButtonText;
|
||||
}
|
||||
}
|
||||
|
||||
:focus-visible {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useEffect, useRef, useState } from 'react'
|
||||
import { useEffect, useRef, useState } from 'react'
|
||||
import { makeStyles, mergeClasses, tokens, shorthands } from '@fluentui/react-components'
|
||||
import { useFocusStyles } from './ui/focusRing'
|
||||
|
||||
|
||||
@@ -27,7 +27,12 @@ import {
|
||||
CalendarClockRegular,
|
||||
WarningRegular
|
||||
} from '@fluentui/react-icons'
|
||||
import type { MediaInfo, FormatOption, PlaylistInfo } from '@shared/ipc'
|
||||
import {
|
||||
parseUrlShortcutContent,
|
||||
type MediaInfo,
|
||||
type FormatOption,
|
||||
type PlaylistInfo
|
||||
} from '@shared/ipc'
|
||||
import { useDownloads, type MediaKind } from '../store/downloads'
|
||||
import { QUALITY_OPTIONS } from '../qualityOptions'
|
||||
import { sameVideo } from '../store/queueStats'
|
||||
@@ -36,12 +41,10 @@ import { Select } from './Select'
|
||||
import { Hint } from './Hint'
|
||||
import { SegmentedControl } from './ui/SegmentedControl'
|
||||
import { IconButton } from './ui/IconButton'
|
||||
import { parseUrlShortcutContent } from '@shared/ipc'
|
||||
import { useClipboardLink, looksLikeUrl, firstUrlInText } from '../useClipboardLink'
|
||||
import { logError } from '../reportError'
|
||||
import { THUMB_LG } from '../thumbSizes'
|
||||
|
||||
|
||||
/** Pull the URL= target out of a dropped Windows .url Internet Shortcut file. */
|
||||
function parseUrlFile(content: string): string | null {
|
||||
const url = parseUrlShortcutContent(content)
|
||||
|
||||
@@ -127,6 +127,9 @@ const useStyles = makeStyles({
|
||||
width: '100%',
|
||||
border: 'none',
|
||||
backgroundColor: 'transparent',
|
||||
// A native <button> doesn't inherit color; set it so the chevron (currentColor)
|
||||
// matches the page foreground in dark mode instead of UA ButtonText.
|
||||
color: tokens.colorNeutralForeground1,
|
||||
fontFamily: tokens.fontFamilyBase,
|
||||
textAlign: 'left',
|
||||
cursor: 'pointer',
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import {
|
||||
import {
|
||||
Card,
|
||||
Title2,
|
||||
Body1,
|
||||
@@ -72,11 +72,6 @@ const useStyles = makeStyles({
|
||||
flexShrink: 0,
|
||||
marginTop: '2px',
|
||||
color: tokens.colorCompoundBrandForeground1
|
||||
},
|
||||
btnRow: {
|
||||
display: 'flex',
|
||||
justifyContent: 'flex-end',
|
||||
gap: '8px'
|
||||
}
|
||||
})
|
||||
|
||||
@@ -131,13 +126,6 @@ export function Onboarding(): React.JSX.Element {
|
||||
))}
|
||||
</div>
|
||||
|
||||
<div className={styles.btnRow}>
|
||||
<Button
|
||||
appearance="subtle"
|
||||
onClick={() => update({ hasCompletedOnboarding: true })}
|
||||
>
|
||||
Skip
|
||||
</Button>
|
||||
<Button
|
||||
appearance="primary"
|
||||
icon={<RocketRegular />}
|
||||
@@ -145,7 +133,6 @@ export function Onboarding(): React.JSX.Element {
|
||||
>
|
||||
Get started
|
||||
</Button>
|
||||
</div>
|
||||
</Card>
|
||||
</div>
|
||||
)
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { makeStyles, mergeClasses, tokens, shorthands } from '@fluentui/react-components'
|
||||
import { makeStyles, mergeClasses, tokens, shorthands } from '@fluentui/react-components'
|
||||
|
||||
// A native <select> styled to match Fluent inputs. We use this instead of
|
||||
// Fluent's <Dropdown> because Fluent's portal/popover menu is a composited
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
import { VIDEO_QUALITY_OPTIONS, AUDIO_QUALITY_OPTIONS } from '@shared/ipc'
|
||||
import { VIDEO_QUALITY_OPTIONS, AUDIO_QUALITY_OPTIONS, type MediaKind } from '@shared/ipc'
|
||||
|
||||
// `satisfies` (not an annotation) so the tuple element types survive: under
|
||||
// noUncheckedIndexedAccess a `readonly string[]` index yields `string | undefined`,
|
||||
// but the const tuples keep `QUALITY_OPTIONS[kind][0]` known-defined (L168).
|
||||
export const QUALITY_OPTIONS = {
|
||||
video: VIDEO_QUALITY_OPTIONS,
|
||||
audio: AUDIO_QUALITY_OPTIONS
|
||||
} as const
|
||||
} satisfies Record<MediaKind, readonly string[]>
|
||||
|
||||
@@ -40,10 +40,10 @@ export interface DownloadItem {
|
||||
speed?: string
|
||||
eta?: string
|
||||
sizeLabel?: string
|
||||
/** yt-dlp reported no total size — render the bar indeterminate, not 0% (L137) */
|
||||
/** yt-dlp reported no total size -- render the bar indeterminate, not 0% (L137) */
|
||||
sizeUnknown?: boolean
|
||||
/** true once the main download stream finished and yt-dlp is fetching the
|
||||
* remaining stream / merging / post-processing — the bar goes indeterminate
|
||||
* remaining stream / merging / post-processing -- the bar goes indeterminate
|
||||
* instead of visibly restarting 0→100% for the second stream (SR7) */
|
||||
finishing?: boolean
|
||||
filePath?: string
|
||||
@@ -55,21 +55,20 @@ export interface DownloadItem {
|
||||
options?: DownloadOptions
|
||||
/** per-download custom-command override (omitted = use the persisted default template) */
|
||||
extraArgs?: string
|
||||
/** raw trim spec — time ranges to keep (parsed to --download-sections in main) */
|
||||
/** raw trim spec -- time ranges to keep (parsed to --download-sections in main) */
|
||||
trim?: string
|
||||
/** epoch ms to auto-start a 'saved' item; undefined = parked indefinitely (Phase M) */
|
||||
scheduledFor?: number
|
||||
/** private download — completion is never recorded to history */
|
||||
/** private download -- completion is never recorded to history */
|
||||
incognito?: boolean
|
||||
/** metadata already fetched at probe time; forwarded to main so it skips a redundant probe */
|
||||
probedMeta?: DownloadMeta
|
||||
/** media-manager folder context — files this into <channel>/<playlist>/<NNN> - <title> */
|
||||
/** media-manager folder context -- files this into <channel>/<playlist>/<NNN> - <title> */
|
||||
collection?: CollectionContext
|
||||
/** when set, the source MediaItem id this download came from — marked downloaded on completion */
|
||||
/** when set, the source MediaItem id this download came from -- marked downloaded on completion */
|
||||
mediaItemId?: string
|
||||
}
|
||||
|
||||
|
||||
// Placeholder channel shown while metadata is still being fetched. Tracked as a
|
||||
// constant so the meta/error/done handlers can recognise and clear it instead of
|
||||
// letting it stick forever when a probe returns no channel (SR6).
|
||||
@@ -204,7 +203,7 @@ const seed: DownloadItem[] = PREVIEW
|
||||
{
|
||||
id: newId('item'),
|
||||
url: 'https://youtube.com/watch?v=dQw4w9WgXcQ',
|
||||
title: 'Building a Desktop App with Electron — Full Course',
|
||||
title: 'Building a Desktop App with Electron -- Full Course',
|
||||
channel: 'DevChannel',
|
||||
durationLabel: '1:42:08',
|
||||
kind: 'video',
|
||||
@@ -306,7 +305,7 @@ function startFakeTicker(
|
||||
if (finished?.mediaItemId) {
|
||||
useSources.getState().markDownloaded(finished.mediaItemId, finished.filePath)
|
||||
}
|
||||
get().pump() // a slot just freed — promote the next queued item
|
||||
get().pump() // a slot just freed -- promote the next queued item
|
||||
}
|
||||
}, 650)
|
||||
}
|
||||
@@ -350,9 +349,9 @@ export const useDownloads = create<DownloadState>((set, get) => {
|
||||
// Promote queued items (oldest first) into free slots, then launch them.
|
||||
//
|
||||
// Note: this only gates *future* promotions. Lowering maxConcurrent while N
|
||||
// downloads are already active does NOT pause the overflow — the running
|
||||
// downloads are already active does NOT pause the overflow -- the running
|
||||
// processes finish on their own, and the lower cap takes effect only as slots
|
||||
// free up. (audit P3 — documented behaviour, not a bug.)
|
||||
// free up. (audit P3 -- documented behaviour, not a bug.)
|
||||
function pump(): void {
|
||||
const items = get().items
|
||||
const running = items.filter((i) => i.status === 'downloading').length
|
||||
@@ -385,7 +384,7 @@ export const useDownloads = create<DownloadState>((set, get) => {
|
||||
// The queue is a newest-first array and pump() promotes the highest-index
|
||||
// (oldest) queued item first, so a batch must be stored last-entry-first for
|
||||
// the playlist/channel to download 1→N instead of N→1 (M32). Entry 1 then
|
||||
// sits at the bottom of the list — the same way repeated single adds stack.
|
||||
// sits at the bottom of the list -- the same way repeated single adds stack.
|
||||
const built = entries.map((e) => buildItem(e.url, e.kind, e.quality, e.opts)).reverse()
|
||||
set((s) => ({ items: [...built, ...s.items] }))
|
||||
pump()
|
||||
@@ -455,8 +454,8 @@ export const useDownloads = create<DownloadState>((set, get) => {
|
||||
},
|
||||
|
||||
prioritize: (id) => {
|
||||
// Reposition the item so pump() — which promotes the highest-index queued
|
||||
// item first (oldest-first over a newest-first array) — picks it next.
|
||||
// Reposition the item so pump() -- which promotes the highest-index queued
|
||||
// item first (oldest-first over a newest-first array) -- picks it next.
|
||||
set((s) => {
|
||||
const idx = s.items.findIndex((i) => i.id === id)
|
||||
if (idx < 0 || s.items[idx]?.status !== 'queued') return {}
|
||||
@@ -552,7 +551,7 @@ export const useDownloads = create<DownloadState>((set, get) => {
|
||||
switch (ev.type) {
|
||||
case 'meta':
|
||||
// A late meta event can arrive between cancel() and the child's
|
||||
// close — don't overwrite a canceled item's fields (B5; mirrors the
|
||||
// close -- don't overwrite a canceled item's fields (B5; mirrors the
|
||||
// canceled guard on the done/error cases below).
|
||||
if (i.status === 'canceled') return i
|
||||
return {
|
||||
@@ -566,7 +565,7 @@ export const useDownloads = create<DownloadState>((set, get) => {
|
||||
}
|
||||
case 'progress':
|
||||
// Only a launched (downloading) item should take progress. Never
|
||||
// promote a 'queued' item here — pump() owns the queued→downloading
|
||||
// promote a 'queued' item here -- pump() owns the queued→downloading
|
||||
// transition and the concurrency cap (L88); a stray progress event
|
||||
// for a non-downloading item is ignored.
|
||||
if (i.status !== 'downloading') return i
|
||||
@@ -634,7 +633,7 @@ export const useDownloads = create<DownloadState>((set, get) => {
|
||||
useSources.getState().markDownloaded(item.mediaItemId, item.filePath)
|
||||
}
|
||||
}
|
||||
// A finished item frees a slot — promote whatever is queued next.
|
||||
// A finished item frees a slot -- promote whatever is queued next.
|
||||
if (ev.type === 'done' || ev.type === 'error') pump()
|
||||
}
|
||||
}
|
||||
@@ -658,7 +657,7 @@ if (!PREVIEW) {
|
||||
window.api.onDownloadEvent((ev) => useDownloads.getState().applyEvent(ev))
|
||||
} else {
|
||||
// Browser preview: animate any seed item that starts out 'downloading' so the
|
||||
// queue is live (and its completion promotes the queued seed item — a real
|
||||
// queue is live (and its completion promotes the queued seed item -- a real
|
||||
// demo of the concurrency cap).
|
||||
useDownloads
|
||||
.getState()
|
||||
|
||||
Reference in New Issue
Block a user