From 519e522917f0a7f4796ddd153efe61a52d0ac74a Mon Sep 17 00:00:00 2001 From: debont80 Date: Wed, 1 Jul 2026 08:53:06 -0400 Subject: [PATCH] =?UTF-8?q?feat(main):=20CC8=20=E2=80=94=20file-backed=20l?= =?UTF-8?q?eveled=20logger=20+=20renderer=20log=20sink?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnostics were invisible in a packaged build: main catches did a bare console.error (DevTools suppressed in prod, M31) and the renderer's logError (M29) wrote to an unreachable console. Add a small leveled logger (src/main/logger.ts) appending to /logs/aerofetch.log with size-capped rotation; all app/fs access is lazy+guarded so it's a safe no-op under tests. - Route the 5 main-process console.* catch sites (jsonStore write-fail, settings write-fail + plaintext-secret warn, cookie loadURL, yt-dlp auto-update) through the logger. - Add a fire-and-forget log:write IPC channel + preload logError() so the renderer's logError forwards failures to the main file sink (closes the M29 "sink" half the code comments deferred to CC8). mockApi + Api type updated. - Unit-test pure formatLine/composeMessage + no-op safety; update the R6 jsonStore test to assert against the logger. The user-facing toast half of CC8 ties to the global status surface (UI25/UX9). typecheck + 258 tests + lint + prettier green. Co-Authored-By: Claude Opus 4.8 --- src/main/constants.ts | 7 +++ src/main/cookies.ts | 5 +- src/main/index.ts | 3 +- src/main/ipc.ts | 7 +++ src/main/jsonStore.ts | 7 ++- src/main/logger.ts | 106 ++++++++++++++++++++++++++++++++ src/main/settings.ts | 5 +- src/preload/index.ts | 4 ++ src/renderer/src/mockApi.ts | 1 + src/renderer/src/reportError.ts | 18 ++++-- src/shared/ipc.ts | 3 + test/jsonStore.test.ts | 3 +- test/logger.test.ts | 31 ++++++++++ 13 files changed, 185 insertions(+), 15 deletions(-) create mode 100644 src/main/logger.ts create mode 100644 test/logger.test.ts diff --git a/src/main/constants.ts b/src/main/constants.ts index c47075b..fbc2fd9 100644 --- a/src/main/constants.ts +++ b/src/main/constants.ts @@ -46,6 +46,13 @@ export const INDEX_MAX_BUFFER = 256 * 1024 * 1024 /** How much of a failed download's stderr to retain for the error message. */ export const STDERR_TAIL_BYTES = 4000 +/** + * Size cap for the diagnostic log file (logger.ts, CC8). Once the live + * `aerofetch.log` passes this, it's rotated to `aerofetch.log.1` (one generation + * kept) so app logging can't fill the disk. + */ +export const LOG_MAX_BYTES = 1024 * 1024 + // --- JSON-store row caps ---------------------------------------------------- // Each hand-rolled JSON store trims to its cap on write so a store file can't // grow without bound. diff --git a/src/main/cookies.ts b/src/main/cookies.ts index 242040e..d97778b 100644 --- a/src/main/cookies.ts +++ b/src/main/cookies.ts @@ -2,6 +2,7 @@ import { app, safeStorage, session, BrowserWindow, type Cookie, type WebContents import { existsSync, statSync, unlinkSync, writeFileSync, readFileSync } from 'fs' import { join } from 'path' import { assertHttpUrl } from './url' +import { logger } from './logger' import type { CookiesStatus, CookiesLoginResult } from '@shared/ipc' /** @@ -228,9 +229,7 @@ export function openCookieLoginWindow( if (loginWindow && !loginWindow.isDestroyed()) { pendingResolvers.push(resolve) - loginWindow - .loadURL(validUrl) - .catch((e) => console.error('[AeroFetch] cookie login loadURL failed:', e)) + loginWindow.loadURL(validUrl).catch((e) => logger.error('cookie login loadURL failed', e)) loginWindow.focus() return } diff --git a/src/main/index.ts b/src/main/index.ts index 865d71f..21c0e3e 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -21,6 +21,7 @@ import { flushAllStores } from './jsonStore' import { extractIncomingUrl, registerSendToShortcut, focusWindow } from './deeplink' import { isSyncLaunch } from './schedule' import { createTray, markQuitting, isQuitting } from './tray' +import { logger } from './logger' // Only one instance ever runs. A second launch — e.g. the OS invoking us again // for an aerofetch:// link or a "Send to AeroFetch" file — hands its argv to @@ -271,7 +272,7 @@ if (isPrimaryInstance) { if (mainWindow && !mainWindow.isDestroyed()) { mainWindow.webContents.send(IpcChannels.ytdlpAutoUpdateStatus, status) } - }).catch((e) => console.error('[AeroFetch] yt-dlp auto-update failed:', e)) + }).catch((e) => logger.error('yt-dlp auto-update failed', e)) }) // Any real quit path (tray "Quit", OS shutdown, the updater's app.quit) must set diff --git a/src/main/ipc.ts b/src/main/ipc.ts index bc5201e..101def9 100644 --- a/src/main/ipc.ts +++ b/src/main/ipc.ts @@ -50,6 +50,7 @@ import { syncWatchedSources } from './sync' import { getScheduledSync, setScheduledSync } from './schedule' import { getActiveBadge, getErrorBadge } from './badge' import { openPoTokenWindow } from './poToken' +import { logger } from './logger' // --- Theme helpers (shared with index's system-theme bridge) ---------------- @@ -87,6 +88,12 @@ export function registerIpcHandlers(getMainWindow: () => BrowserWindow | null): app.quit() }) + // CC8: persist renderer-side failures (the M29 `logError` sites) to the main log + // file — the renderer console is unreachable in a packaged build. Fire-and-forget. + ipcMain.on(IpcChannels.logWrite, (_e, op: string, detail: string) => + logger.error(`[renderer] ${op}`, detail) + ) + ipcMain.handle(IpcChannels.appVersion, () => app.getVersion()) ipcMain.handle(IpcChannels.appUpdateCheck, () => checkForAppUpdate()) diff --git a/src/main/jsonStore.ts b/src/main/jsonStore.ts index a1853a8..51e8512 100644 --- a/src/main/jsonStore.ts +++ b/src/main/jsonStore.ts @@ -1,6 +1,7 @@ import { app } from 'electron' import { join } from 'path' import { readFileSync, writeFileSync, renameSync, existsSync, copyFileSync, unlinkSync } from 'fs' +import { logger } from './logger' /** * One shared persistence layer for the hand-rolled JSON array stores (history, @@ -69,9 +70,9 @@ export function writeJsonAtomic(path: string, value: unknown, pretty = true): bo return true } catch (e) { // R6: previously a silent catch — a failed persist (disk full, read-only - // profile) dropped the change with no trace. Log it; the user-facing warning - // is deferred to the leveled file logger (CC8). - console.error(`[AeroFetch] failed to write ${path}:`, e) + // profile) dropped the change with no trace. It now lands in the file-backed + // diagnostic log (CC8); the user-facing toast ties to the global status surface (UI25/UX9). + logger.error(`failed to write ${path}`, e) try { if (existsSync(tmp)) unlinkSync(tmp) } catch { diff --git a/src/main/logger.ts b/src/main/logger.ts new file mode 100644 index 0000000..eee55f5 --- /dev/null +++ b/src/main/logger.ts @@ -0,0 +1,106 @@ +import { app } from 'electron' +import { join } from 'path' +import { appendFileSync, mkdirSync, renameSync, statSync } from 'fs' +import { LOG_MAX_BYTES } from './constants' + +/** + * One small leveled, file-backed logger for the whole app (audit CC8). Before this, + * diagnostics were effectively invisible in a packaged build: main-process catches + * did a bare `console.error` (unreachable — the app menu/DevTools are suppressed in + * production, M31) and the renderer's `logError` (M29) wrote to a console nobody could + * open. Every catch now routes here and lands in `/logs/aerofetch.log`, so a + * field-reported failure is actually diagnosable. + * + * Scope: this is the diagnostic log. `errorlog.ts` remains separate — it's user-facing + * *download* failure data shown in Settings → Diagnostics, not app logging. The + * user-facing toast half of CC8 ties to the global status surface (UI25/UX9). + * + * All `app`/fs access is lazy and guarded so importing this module never touches + * `app` before it's ready — and so it's a safe no-op under unit tests, where + * `electron`'s `app` is undefined. + */ + +export type LogLevel = 'error' | 'warn' | 'info' | 'debug' + +const LEVELS: Record = { error: 0, warn: 1, info: 2, debug: 3 } + +// Write everything at or above this level to the file. `debug` is dev-only noise, so +// it never hits the file (but still prints to the dev console). +const FILE_THRESHOLD = LEVELS.info + +/** Format one log line: `2026-07-01T08:30:00.000Z [ERROR] message`. Pure, for tests. */ +export function formatLine(level: LogLevel, message: string, now: Date = new Date()): string { + return `${now.toISOString()} [${level.toUpperCase()}] ${message}` +} + +/** Combine an operation label with an optional error/detail into one message. Pure. */ +export function composeMessage(label: string, detail?: unknown): string { + if (detail === undefined) return label + const text = detail instanceof Error ? detail.message : String(detail) + return `${label}: ${text}` +} + +// `undefined` = not yet resolved, `null` = resolution failed (no app/userData, e.g. +// under unit tests) so file logging is disabled; a string is the resolved path. +let cachedFile: string | null | undefined +function logFile(): string | null { + if (cachedFile !== undefined) return cachedFile + try { + const dir = join(app.getPath('userData'), 'logs') + mkdirSync(dir, { recursive: true }) + cachedFile = join(dir, 'aerofetch.log') + } catch { + // No app / userData available (unit test, or app not ready) — disable the file sink. + cachedFile = null + } + return cachedFile +} + +/** Dev mode, resolved lazily and guarded (app is undefined in the test env). */ +function isDev(): boolean { + try { + return !app.isPackaged + } catch { + return false + } +} + +// Rotate to `.1` (single generation) once the live file passes the cap, so the +// log can't grow without bound. Best-effort: any fs error here is swallowed — logging +// must never throw into a catch handler. +function rotateIfNeeded(file: string): void { + try { + if (statSync(file).size > LOG_MAX_BYTES) renameSync(file, `${file}.1`) + } catch { + /* file missing (first write) or rename race — ignore */ + } +} + +function writeToFile(line: string): void { + const file = logFile() + if (!file) return + rotateIfNeeded(file) + try { + appendFileSync(file, line + '\n') + } catch { + /* disk full / read-only profile — nothing more we can safely do */ + } +} + +function emit(level: LogLevel, label: string, detail?: unknown): void { + const message = composeMessage(label, detail) + // Console in dev keeps the familiar `--inspect` workflow; the file is the sink that + // survives into a packaged build. + if (isDev()) { + const fn = level === 'error' ? console.error : level === 'warn' ? console.warn : console.log + fn(`[AeroFetch] ${message}`) + } + if (LEVELS[level] <= FILE_THRESHOLD) writeToFile(formatLine(level, message)) +} + +export const logger = { + error: (label: string, detail?: unknown): void => emit('error', label, detail), + warn: (label: string, detail?: unknown): void => emit('warn', label, detail), + info: (label: string, detail?: unknown): void => emit('info', label, detail), + debug: (label: string, detail?: unknown): void => emit('debug', label, detail) +} diff --git a/src/main/settings.ts b/src/main/settings.ts index 4a0a33b..0244b3b 100644 --- a/src/main/settings.ts +++ b/src/main/settings.ts @@ -1,6 +1,7 @@ import { app, safeStorage } from 'electron' import Store from 'electron-store' import { isSafeFilenameTemplate, isSafeOutputDir } from './validation' +import { logger } from './logger' import { AUDIO_FORMATS, VIDEO_CONTAINERS, @@ -130,7 +131,7 @@ function encryptSecret(plain: string): string { // to be written in cleartext. Warn rather than fall back silently — a leaked // settings.json would then expose the proxy password / API token. (DPAPI is // effectively always present on Windows, so this should never fire in practice.) - console.warn('[AeroFetch] OS secret encryption unavailable — storing credential as plaintext.') + logger.warn('OS secret encryption unavailable — storing credential as plaintext') return plain } @@ -238,7 +239,7 @@ export function setSettings(partial: Partial): Settings { // handler or surface as an unhandled rejection. Log it; getSettings() below // returns the store's ACTUAL persisted state, so the renderer's reconciliation // (M34) reflects what truly saved instead of the optimistic value. - console.error('[AeroFetch] settings write failed:', e) + logger.error('settings write failed', e) } cachedSettings = null return getSettings() diff --git a/src/preload/index.ts b/src/preload/index.ts index 4ed7209..f406e8b 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -83,6 +83,10 @@ const api = { readClipboard: (): Promise => ipcRenderer.invoke(IpcChannels.clipboardRead), + /** Forward a renderer-side failure to the main diagnostic log (CC8). Fire-and-forget. */ + logError: (op: string, detail: string): void => + ipcRenderer.send(IpcChannels.logWrite, op, detail), + getSettings: (): Promise => ipcRenderer.invoke(IpcChannels.settingsGet), setSettings: (partial: Partial): Promise => diff --git a/src/renderer/src/mockApi.ts b/src/renderer/src/mockApi.ts index 4440c67..eaac20d 100644 --- a/src/renderer/src/mockApi.ts +++ b/src/renderer/src/mockApi.ts @@ -137,6 +137,7 @@ export const mockApi: Api = { openUrl: async () => {}, showInFolder: async () => {}, readClipboard: async () => '', + logError: () => {}, getSettings: async () => MOCK_SETTINGS, setSettings: async (partial) => Object.assign(MOCK_SETTINGS, partial), listHistory: async () => [], diff --git a/src/renderer/src/reportError.ts b/src/renderer/src/reportError.ts index d7ba838..b960a4f 100644 --- a/src/renderer/src/reportError.ts +++ b/src/renderer/src/reportError.ts @@ -4,10 +4,18 @@ // Replaces the swallowed `.catch(() => {})` sites the audit flagged (M29) with a // single, consistent log format. `op` names the operation, e.g. 'addHistory'. // -// Note: in production the DevTools console isn't reachable (the app menu is -// suppressed), so this is primarily a development/`--inspect` diagnostic until a -// persistent renderer log sink lands (CC8). It is still strictly better than -// discarding the error. +// The failure is both printed to the renderer console (dev/`--inspect`) and +// forwarded to the main process's file-backed diagnostic log (CC8), so it survives +// into a packaged build where the renderer console is unreachable. export function logError(op: string): (e: unknown) => void { - return (e) => console.error(`[AeroFetch] ${op} failed:`, e) + return (e) => { + console.error(`[AeroFetch] ${op} failed:`, e) + try { + const detail = e instanceof Error ? e.message : String(e) + // window.api is absent in the browser UI preview and in unit tests. + if (typeof window !== 'undefined') window.api?.logError?.(op, detail) + } catch { + /* logging must never throw out of a catch handler */ + } + } } diff --git a/src/shared/ipc.ts b/src/shared/ipc.ts index 00609b7..938b16c 100644 --- a/src/shared/ipc.ts +++ b/src/shared/ipc.ts @@ -55,6 +55,9 @@ export const IpcChannels = { /** preload → main: the contextBridge failed to expose the API; main shows a * hard error instead of letting the renderer silently fall back to mock mode (M30) */ preloadBridgeFailure: 'preload:bridge-failure', + /** renderer → main fire-and-forget: forward a renderer-side failure to the main + * diagnostic log so it survives into a packaged build (CC8; the M29 sink half). */ + logWrite: 'log:write', /** main → renderer push channel: a URL handed to AeroFetch from outside the app * (the aerofetch:// protocol, or a .url file via Explorer's "Send to" menu) */ externalUrl: 'app:external-url', diff --git a/test/jsonStore.test.ts b/test/jsonStore.test.ts index 7b4b1e0..b81b550 100644 --- a/test/jsonStore.test.ts +++ b/test/jsonStore.test.ts @@ -3,6 +3,7 @@ import { mkdtempSync, rmSync, writeFileSync, readdirSync } from 'fs' import { tmpdir } from 'os' import { join } from 'path' import { readJsonArraySafe, writeJsonAtomic } from '../src/main/jsonStore' +import { logger } from '../src/main/logger' interface Row { id: string @@ -51,7 +52,7 @@ describe('jsonStore atomic write + safe read', () => { it('reports success/failure instead of swallowing a bad write (R6)', () => { const d = tmp() - const err = vi.spyOn(console, 'error').mockImplementation(() => {}) + const err = vi.spyOn(logger, 'error').mockImplementation(() => {}) try { // A normal write lands and reports true, with nothing logged. expect(writeJsonAtomic(join(d, 'data.json'), [{ id: 'a' }])).toBe(true) diff --git a/test/logger.test.ts b/test/logger.test.ts new file mode 100644 index 0000000..c10f32c --- /dev/null +++ b/test/logger.test.ts @@ -0,0 +1,31 @@ +import { describe, it, expect } from 'vitest' +import { formatLine, composeMessage, logger } from '../src/main/logger' + +// The logger's file sink needs Electron's `app` (userData path), which is absent in +// the node test env — so here we cover the pure formatting helpers and assert that +// the impure entry points degrade to a safe no-op rather than throwing (CC8). +describe('logger (CC8)', () => { + const now = new Date('2026-07-01T08:30:00.000Z') + + it('formatLine renders ISO timestamp + upper-case level + message', () => { + expect(formatLine('error', 'boom', now)).toBe('2026-07-01T08:30:00.000Z [ERROR] boom') + expect(formatLine('info', 'hi', now)).toBe('2026-07-01T08:30:00.000Z [INFO] hi') + }) + + it('composeMessage passes the label through when there is no detail', () => { + expect(composeMessage('settings write failed')).toBe('settings write failed') + }) + + it('composeMessage appends an Error message', () => { + expect(composeMessage('write failed', new Error('ENOSPC'))).toBe('write failed: ENOSPC') + }) + + it('composeMessage stringifies a non-Error detail', () => { + expect(composeMessage('op', 42)).toBe('op: 42') + expect(composeMessage('op', 'bad input')).toBe('op: bad input') + }) + + it('logger.error is a safe no-op when app/userData is unavailable', () => { + expect(() => logger.error('some op', new Error('x'))).not.toThrow() + }) +})