From 43d008ebfe71cbf659b7f8c2e7f2efd1331fae52 Mon Sep 17 00:00:00 2001 From: sudacode Date: Sun, 29 Mar 2026 01:02:09 -0700 Subject: [PATCH] fix: address latest CodeRabbit review round --- .gitignore | 1 + ...ss-PR-36-latest-CodeRabbit-review-round.md | 35 +++++ scripts/run-coverage-lane.ts | 3 +- .../services/immersion-tracker/maintenance.ts | 14 +- src/main.ts | 121 +++++++++--------- 5 files changed, 103 insertions(+), 71 deletions(-) create mode 100644 backlog/completed/task-243 - Assess-and-address-PR-36-latest-CodeRabbit-review-round.md diff --git a/.gitignore b/.gitignore index d42112f..396421c 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ out/ dist/ release/ build/yomitan/ +coverage/ # Launcher build artifact (produced by make build-launcher) /subminer diff --git a/backlog/completed/task-243 - Assess-and-address-PR-36-latest-CodeRabbit-review-round.md b/backlog/completed/task-243 - Assess-and-address-PR-36-latest-CodeRabbit-review-round.md new file mode 100644 index 0000000..cdd612a --- /dev/null +++ b/backlog/completed/task-243 - Assess-and-address-PR-36-latest-CodeRabbit-review-round.md @@ -0,0 +1,35 @@ +--- +id: TASK-243 +title: 'Assess and address PR #36 latest CodeRabbit review round' +status: Done +assignee: [] +created_date: '2026-03-29 07:39' +updated_date: '2026-03-29 07:41' +labels: + - code-review + - pr-36 +dependencies: [] +references: + - 'https://github.com/ksyasuda/SubMiner/pull/36' +priority: high +ordinal: 3600 +--- + +## Description + + +Inspect the latest CodeRabbit review round on PR #36, verify each actionable comment against the current branch, implement the confirmed fixes, and verify the touched paths. + + +## Acceptance Criteria + +- [ ] #1 Confirmed review comments are implemented or explicitly deferred with rationale. +- [ ] #2 Touched paths are verified with the smallest sufficient test/build lane. +- [ ] #3 Current PR feedback is reduced to resolved or intentionally deferred suggestions. + + +## Final Summary + + +Addressed the confirmed latest CodeRabbit review items on PR #36. `scripts/run-coverage-lane.ts` now uses the Bun-style `import.meta.main` entrypoint check with a local ts-ignore to preserve the repo's CommonJS typecheck settings. `src/core/services/immersion-tracker/maintenance.ts` no longer shadows the imported `nowMs` helper in retention functions. `src/main.ts` now centralizes the startup-mode predicates behind a shared helper and releases `resolvedSource.cleanup` on the cached-subtitle fast path so materialized sources do not leak. + diff --git a/scripts/run-coverage-lane.ts b/scripts/run-coverage-lane.ts index 98b747f..964962e 100644 --- a/scripts/run-coverage-lane.ts +++ b/scripts/run-coverage-lane.ts @@ -305,6 +305,7 @@ function runCoverageLane(): number { } } -if (require.main === module) { +// @ts-ignore Bun entrypoint detection; TS config for scripts still targets CommonJS. +if (import.meta.main) { process.exit(runCoverageLane()); } diff --git a/src/core/services/immersion-tracker/maintenance.ts b/src/core/services/immersion-tracker/maintenance.ts index 6e6db32..b2cda9b 100644 --- a/src/core/services/immersion-tracker/maintenance.ts +++ b/src/core/services/immersion-tracker/maintenance.ts @@ -46,16 +46,16 @@ export function toMonthKey(timestampMs: number): number { export function pruneRawRetention( db: DatabaseSync, - nowMs: number, + currentMs: number, policy: { eventsRetentionMs: number; telemetryRetentionMs: number; sessionsRetentionMs: number; }, ): RawRetentionResult { - const eventCutoff = nowMs - policy.eventsRetentionMs; - const telemetryCutoff = nowMs - policy.telemetryRetentionMs; - const sessionsCutoff = nowMs - policy.sessionsRetentionMs; + const eventCutoff = currentMs - policy.eventsRetentionMs; + const telemetryCutoff = currentMs - policy.telemetryRetentionMs; + const sessionsCutoff = currentMs - policy.sessionsRetentionMs; const deletedSessionEvents = ( db.prepare(`DELETE FROM imm_session_events WHERE ts_ms < ?`).run(toDbMs(eventCutoff)) as { @@ -82,7 +82,7 @@ export function pruneRawRetention( export function pruneRollupRetention( db: DatabaseSync, - nowMs: number, + currentMs: number, policy: { dailyRollupRetentionMs: number; monthlyRollupRetentionMs: number; @@ -92,7 +92,7 @@ export function pruneRollupRetention( ? ( db .prepare(`DELETE FROM imm_daily_rollups WHERE rollup_day < ?`) - .run(Math.floor((nowMs - policy.dailyRollupRetentionMs) / DAILY_MS)) as { + .run(Math.floor((currentMs - policy.dailyRollupRetentionMs) / DAILY_MS)) as { changes: number; } ).changes @@ -101,7 +101,7 @@ export function pruneRollupRetention( ? ( db .prepare(`DELETE FROM imm_monthly_rollups WHERE rollup_month < ?`) - .run(toMonthKey(nowMs - policy.monthlyRollupRetentionMs)) as { + .run(toMonthKey(currentMs - policy.monthlyRollupRetentionMs)) as { changes: number; } ).changes diff --git a/src/main.ts b/src/main.ts index db6a403..5dd1931 100644 --- a/src/main.ts +++ b/src/main.ts @@ -68,6 +68,26 @@ function getDefaultPasswordStore(): string { return 'gnome-libsecret'; } +function getStartupModeFlags(initialArgs: CliArgs | null | undefined): { + shouldUseMinimalStartup: boolean; + shouldSkipHeavyStartup: boolean; +} { + return { + shouldUseMinimalStartup: Boolean( + initialArgs?.texthooker || + (initialArgs?.stats && + (initialArgs.statsCleanup || initialArgs.statsBackground || initialArgs.statsStop)), + ), + shouldSkipHeavyStartup: Boolean( + initialArgs && + (shouldRunSettingsOnlyStartup(initialArgs) || + initialArgs.stats || + initialArgs.dictionary || + initialArgs.setup), + ), + }; +} + protocol.registerSchemesAsPrivileged([ { scheme: 'chrome-extension', @@ -399,7 +419,7 @@ import { import { handleMpvCommandFromIpcRuntime } from './main/ipc-mpv-command'; import { registerIpcRuntimeServices } from './main/ipc-runtime'; import { createAnkiJimakuIpcRuntimeServiceDeps } from './main/dependencies'; -import { createMainBootServices } from './main/boot/services'; +import { createMainBootServices, type MainBootServicesResult } from './main/boot/services'; import { handleCliCommandRuntimeServiceWithContext } from './main/cli-runtime'; import { createOverlayModalRuntimeService } from './main/overlay-runtime'; import { createOverlayModalInputState } from './main/runtime/overlay-modal-input-state'; @@ -596,6 +616,28 @@ const getDefaultSocketPathHandler = createGetDefaultSocketPathHandler(getDefault function getDefaultSocketPath(): string { return getDefaultSocketPathHandler(); } + +type BootServices = MainBootServicesResult< + ConfigService, + ReturnType, + ReturnType, + ReturnType, + SubtitleWebSocket, + ReturnType, + ReturnType, + ReturnType, + ReturnType, + ReturnType, + ReturnType, + ReturnType, + { + requestSingleInstanceLock: () => boolean; + quit: () => void; + on: (event: string, listener: (...args: unknown[]) => void) => unknown; + whenReady: () => Promise; + } +>; + const bootServices = createMainBootServices({ platform: process.platform, argv: process.argv, @@ -675,31 +717,7 @@ const bootServices = createMainBootServices({ }); }, createAppState, -}) as { - configDir: string; - userDataPath: string; - defaultMpvLogPath: string; - defaultImmersionDbPath: string; - configService: ConfigService; - anilistTokenStore: ReturnType; - jellyfinTokenStore: ReturnType; - anilistUpdateQueue: ReturnType; - subtitleWsService: SubtitleWebSocket; - annotationSubtitleWsService: SubtitleWebSocket; - logger: ReturnType; - runtimeRegistry: ReturnType; - overlayManager: ReturnType; - overlayModalInputState: ReturnType; - overlayContentMeasurementStore: ReturnType; - overlayModalRuntime: ReturnType; - appState: ReturnType; - appLifecycleApp: { - requestSingleInstanceLock: () => boolean; - quit: () => void; - on: (event: string, listener: (...args: unknown[]) => void) => unknown; - whenReady: () => Promise; - }; -}; +}) as BootServices; const { configDir: CONFIG_DIR, userDataPath: USER_DATA_PATH, @@ -3186,21 +3204,9 @@ const { appReadyRuntimeRunner } = composeAppReadyRuntime({ shouldRunHeadlessInitialCommand: () => Boolean(appState.initialArgs && isHeadlessInitialCommand(appState.initialArgs)), shouldUseMinimalStartup: () => - Boolean( - appState.initialArgs?.texthooker || - (appState.initialArgs?.stats && - (appState.initialArgs?.statsCleanup || - appState.initialArgs?.statsBackground || - appState.initialArgs?.statsStop)), - ), + getStartupModeFlags(appState.initialArgs).shouldUseMinimalStartup, shouldSkipHeavyStartup: () => - Boolean( - appState.initialArgs && - (shouldRunSettingsOnlyStartup(appState.initialArgs) || - appState.initialArgs.stats || - appState.initialArgs.dictionary || - appState.initialArgs.setup), - ), + getStartupModeFlags(appState.initialArgs).shouldSkipHeavyStartup, createImmersionTracker: () => { ensureImmersionTrackerStarted(); }, @@ -4221,16 +4227,16 @@ const { registerIpcRuntimeHandlers } = composeIpcRuntimeHandlers({ }; } - if (appState.activeParsedSubtitleSource === resolvedSource.sourceKey) { - return { - cues: appState.activeParsedSubtitleCues, - currentTimeSec, - currentSubtitle, - config, - }; - } - try { + if (appState.activeParsedSubtitleSource === resolvedSource.sourceKey) { + return { + cues: appState.activeParsedSubtitleCues, + currentTimeSec, + currentSubtitle, + config, + }; + } + const content = await loadSubtitleSourceText(resolvedSource.path); const cues = parseSubtitleCues(content, resolvedSource.path); appState.activeParsedSubtitleCues = cues; @@ -4480,20 +4486,9 @@ const { runAndApplyStartupState } = composeHeadlessStartupHandlers< }); runAndApplyStartupState(); -const shouldUseMinimalStartup = Boolean( - appState.initialArgs?.texthooker || - (appState.initialArgs?.stats && - (appState.initialArgs?.statsCleanup || - appState.initialArgs?.statsBackground || - appState.initialArgs?.statsStop)), -); -const shouldSkipHeavyStartup = Boolean( - appState.initialArgs && - (shouldRunSettingsOnlyStartup(appState.initialArgs) || - appState.initialArgs.stats || - appState.initialArgs.dictionary || - appState.initialArgs.setup), -); +const startupModeFlags = getStartupModeFlags(appState.initialArgs); +const shouldUseMinimalStartup = startupModeFlags.shouldUseMinimalStartup; +const shouldSkipHeavyStartup = startupModeFlags.shouldSkipHeavyStartup; if (!appState.initialArgs || (!shouldUseMinimalStartup && !shouldSkipHeavyStartup)) { if (isAnilistTrackingEnabled(getResolvedConfig())) { void refreshAnilistClientSecretStateIfEnabled({ force: true }).catch((error) => {