mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-03-30 06:12:06 -07:00
fix: address latest CodeRabbit review round
This commit is contained in:
1
.gitignore
vendored
1
.gitignore
vendored
@@ -9,6 +9,7 @@ out/
|
|||||||
dist/
|
dist/
|
||||||
release/
|
release/
|
||||||
build/yomitan/
|
build/yomitan/
|
||||||
|
coverage/
|
||||||
|
|
||||||
# Launcher build artifact (produced by make build-launcher)
|
# Launcher build artifact (produced by make build-launcher)
|
||||||
/subminer
|
/subminer
|
||||||
|
|||||||
@@ -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
|
||||||
|
|
||||||
|
<!-- SECTION:DESCRIPTION:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:DESCRIPTION:END -->
|
||||||
|
|
||||||
|
## Acceptance Criteria
|
||||||
|
<!-- AC:BEGIN -->
|
||||||
|
- [ ] #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.
|
||||||
|
<!-- AC:END -->
|
||||||
|
|
||||||
|
## Final Summary
|
||||||
|
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||||
|
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.
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||||
@@ -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());
|
process.exit(runCoverageLane());
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -46,16 +46,16 @@ export function toMonthKey(timestampMs: number): number {
|
|||||||
|
|
||||||
export function pruneRawRetention(
|
export function pruneRawRetention(
|
||||||
db: DatabaseSync,
|
db: DatabaseSync,
|
||||||
nowMs: number,
|
currentMs: number,
|
||||||
policy: {
|
policy: {
|
||||||
eventsRetentionMs: number;
|
eventsRetentionMs: number;
|
||||||
telemetryRetentionMs: number;
|
telemetryRetentionMs: number;
|
||||||
sessionsRetentionMs: number;
|
sessionsRetentionMs: number;
|
||||||
},
|
},
|
||||||
): RawRetentionResult {
|
): RawRetentionResult {
|
||||||
const eventCutoff = nowMs - policy.eventsRetentionMs;
|
const eventCutoff = currentMs - policy.eventsRetentionMs;
|
||||||
const telemetryCutoff = nowMs - policy.telemetryRetentionMs;
|
const telemetryCutoff = currentMs - policy.telemetryRetentionMs;
|
||||||
const sessionsCutoff = nowMs - policy.sessionsRetentionMs;
|
const sessionsCutoff = currentMs - policy.sessionsRetentionMs;
|
||||||
|
|
||||||
const deletedSessionEvents = (
|
const deletedSessionEvents = (
|
||||||
db.prepare(`DELETE FROM imm_session_events WHERE ts_ms < ?`).run(toDbMs(eventCutoff)) as {
|
db.prepare(`DELETE FROM imm_session_events WHERE ts_ms < ?`).run(toDbMs(eventCutoff)) as {
|
||||||
@@ -82,7 +82,7 @@ export function pruneRawRetention(
|
|||||||
|
|
||||||
export function pruneRollupRetention(
|
export function pruneRollupRetention(
|
||||||
db: DatabaseSync,
|
db: DatabaseSync,
|
||||||
nowMs: number,
|
currentMs: number,
|
||||||
policy: {
|
policy: {
|
||||||
dailyRollupRetentionMs: number;
|
dailyRollupRetentionMs: number;
|
||||||
monthlyRollupRetentionMs: number;
|
monthlyRollupRetentionMs: number;
|
||||||
@@ -92,7 +92,7 @@ export function pruneRollupRetention(
|
|||||||
? (
|
? (
|
||||||
db
|
db
|
||||||
.prepare(`DELETE FROM imm_daily_rollups WHERE rollup_day < ?`)
|
.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: number;
|
||||||
}
|
}
|
||||||
).changes
|
).changes
|
||||||
@@ -101,7 +101,7 @@ export function pruneRollupRetention(
|
|||||||
? (
|
? (
|
||||||
db
|
db
|
||||||
.prepare(`DELETE FROM imm_monthly_rollups WHERE rollup_month < ?`)
|
.prepare(`DELETE FROM imm_monthly_rollups WHERE rollup_month < ?`)
|
||||||
.run(toMonthKey(nowMs - policy.monthlyRollupRetentionMs)) as {
|
.run(toMonthKey(currentMs - policy.monthlyRollupRetentionMs)) as {
|
||||||
changes: number;
|
changes: number;
|
||||||
}
|
}
|
||||||
).changes
|
).changes
|
||||||
|
|||||||
105
src/main.ts
105
src/main.ts
@@ -68,6 +68,26 @@ function getDefaultPasswordStore(): string {
|
|||||||
return 'gnome-libsecret';
|
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([
|
protocol.registerSchemesAsPrivileged([
|
||||||
{
|
{
|
||||||
scheme: 'chrome-extension',
|
scheme: 'chrome-extension',
|
||||||
@@ -399,7 +419,7 @@ import {
|
|||||||
import { handleMpvCommandFromIpcRuntime } from './main/ipc-mpv-command';
|
import { handleMpvCommandFromIpcRuntime } from './main/ipc-mpv-command';
|
||||||
import { registerIpcRuntimeServices } from './main/ipc-runtime';
|
import { registerIpcRuntimeServices } from './main/ipc-runtime';
|
||||||
import { createAnkiJimakuIpcRuntimeServiceDeps } from './main/dependencies';
|
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 { handleCliCommandRuntimeServiceWithContext } from './main/cli-runtime';
|
||||||
import { createOverlayModalRuntimeService } from './main/overlay-runtime';
|
import { createOverlayModalRuntimeService } from './main/overlay-runtime';
|
||||||
import { createOverlayModalInputState } from './main/runtime/overlay-modal-input-state';
|
import { createOverlayModalInputState } from './main/runtime/overlay-modal-input-state';
|
||||||
@@ -596,6 +616,28 @@ const getDefaultSocketPathHandler = createGetDefaultSocketPathHandler(getDefault
|
|||||||
function getDefaultSocketPath(): string {
|
function getDefaultSocketPath(): string {
|
||||||
return getDefaultSocketPathHandler();
|
return getDefaultSocketPathHandler();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
type BootServices = MainBootServicesResult<
|
||||||
|
ConfigService,
|
||||||
|
ReturnType<typeof createAnilistTokenStore>,
|
||||||
|
ReturnType<typeof createJellyfinTokenStore>,
|
||||||
|
ReturnType<typeof createAnilistUpdateQueue>,
|
||||||
|
SubtitleWebSocket,
|
||||||
|
ReturnType<typeof createLogger>,
|
||||||
|
ReturnType<typeof createMainRuntimeRegistry>,
|
||||||
|
ReturnType<typeof createOverlayManager>,
|
||||||
|
ReturnType<typeof createOverlayModalInputState>,
|
||||||
|
ReturnType<typeof createOverlayContentMeasurementStore>,
|
||||||
|
ReturnType<typeof createOverlayModalRuntimeService>,
|
||||||
|
ReturnType<typeof createAppState>,
|
||||||
|
{
|
||||||
|
requestSingleInstanceLock: () => boolean;
|
||||||
|
quit: () => void;
|
||||||
|
on: (event: string, listener: (...args: unknown[]) => void) => unknown;
|
||||||
|
whenReady: () => Promise<void>;
|
||||||
|
}
|
||||||
|
>;
|
||||||
|
|
||||||
const bootServices = createMainBootServices({
|
const bootServices = createMainBootServices({
|
||||||
platform: process.platform,
|
platform: process.platform,
|
||||||
argv: process.argv,
|
argv: process.argv,
|
||||||
@@ -675,31 +717,7 @@ const bootServices = createMainBootServices({
|
|||||||
});
|
});
|
||||||
},
|
},
|
||||||
createAppState,
|
createAppState,
|
||||||
}) as {
|
}) as BootServices;
|
||||||
configDir: string;
|
|
||||||
userDataPath: string;
|
|
||||||
defaultMpvLogPath: string;
|
|
||||||
defaultImmersionDbPath: string;
|
|
||||||
configService: ConfigService;
|
|
||||||
anilistTokenStore: ReturnType<typeof createAnilistTokenStore>;
|
|
||||||
jellyfinTokenStore: ReturnType<typeof createJellyfinTokenStore>;
|
|
||||||
anilistUpdateQueue: ReturnType<typeof createAnilistUpdateQueue>;
|
|
||||||
subtitleWsService: SubtitleWebSocket;
|
|
||||||
annotationSubtitleWsService: SubtitleWebSocket;
|
|
||||||
logger: ReturnType<typeof createLogger>;
|
|
||||||
runtimeRegistry: ReturnType<typeof createMainRuntimeRegistry>;
|
|
||||||
overlayManager: ReturnType<typeof createOverlayManager>;
|
|
||||||
overlayModalInputState: ReturnType<typeof createOverlayModalInputState>;
|
|
||||||
overlayContentMeasurementStore: ReturnType<typeof createOverlayContentMeasurementStore>;
|
|
||||||
overlayModalRuntime: ReturnType<typeof createOverlayModalRuntimeService>;
|
|
||||||
appState: ReturnType<typeof createAppState>;
|
|
||||||
appLifecycleApp: {
|
|
||||||
requestSingleInstanceLock: () => boolean;
|
|
||||||
quit: () => void;
|
|
||||||
on: (event: string, listener: (...args: unknown[]) => void) => unknown;
|
|
||||||
whenReady: () => Promise<void>;
|
|
||||||
};
|
|
||||||
};
|
|
||||||
const {
|
const {
|
||||||
configDir: CONFIG_DIR,
|
configDir: CONFIG_DIR,
|
||||||
userDataPath: USER_DATA_PATH,
|
userDataPath: USER_DATA_PATH,
|
||||||
@@ -3186,21 +3204,9 @@ const { appReadyRuntimeRunner } = composeAppReadyRuntime({
|
|||||||
shouldRunHeadlessInitialCommand: () =>
|
shouldRunHeadlessInitialCommand: () =>
|
||||||
Boolean(appState.initialArgs && isHeadlessInitialCommand(appState.initialArgs)),
|
Boolean(appState.initialArgs && isHeadlessInitialCommand(appState.initialArgs)),
|
||||||
shouldUseMinimalStartup: () =>
|
shouldUseMinimalStartup: () =>
|
||||||
Boolean(
|
getStartupModeFlags(appState.initialArgs).shouldUseMinimalStartup,
|
||||||
appState.initialArgs?.texthooker ||
|
|
||||||
(appState.initialArgs?.stats &&
|
|
||||||
(appState.initialArgs?.statsCleanup ||
|
|
||||||
appState.initialArgs?.statsBackground ||
|
|
||||||
appState.initialArgs?.statsStop)),
|
|
||||||
),
|
|
||||||
shouldSkipHeavyStartup: () =>
|
shouldSkipHeavyStartup: () =>
|
||||||
Boolean(
|
getStartupModeFlags(appState.initialArgs).shouldSkipHeavyStartup,
|
||||||
appState.initialArgs &&
|
|
||||||
(shouldRunSettingsOnlyStartup(appState.initialArgs) ||
|
|
||||||
appState.initialArgs.stats ||
|
|
||||||
appState.initialArgs.dictionary ||
|
|
||||||
appState.initialArgs.setup),
|
|
||||||
),
|
|
||||||
createImmersionTracker: () => {
|
createImmersionTracker: () => {
|
||||||
ensureImmersionTrackerStarted();
|
ensureImmersionTrackerStarted();
|
||||||
},
|
},
|
||||||
@@ -4221,6 +4227,7 @@ const { registerIpcRuntimeHandlers } = composeIpcRuntimeHandlers({
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
if (appState.activeParsedSubtitleSource === resolvedSource.sourceKey) {
|
if (appState.activeParsedSubtitleSource === resolvedSource.sourceKey) {
|
||||||
return {
|
return {
|
||||||
cues: appState.activeParsedSubtitleCues,
|
cues: appState.activeParsedSubtitleCues,
|
||||||
@@ -4230,7 +4237,6 @@ const { registerIpcRuntimeHandlers } = composeIpcRuntimeHandlers({
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
try {
|
|
||||||
const content = await loadSubtitleSourceText(resolvedSource.path);
|
const content = await loadSubtitleSourceText(resolvedSource.path);
|
||||||
const cues = parseSubtitleCues(content, resolvedSource.path);
|
const cues = parseSubtitleCues(content, resolvedSource.path);
|
||||||
appState.activeParsedSubtitleCues = cues;
|
appState.activeParsedSubtitleCues = cues;
|
||||||
@@ -4480,20 +4486,9 @@ const { runAndApplyStartupState } = composeHeadlessStartupHandlers<
|
|||||||
});
|
});
|
||||||
|
|
||||||
runAndApplyStartupState();
|
runAndApplyStartupState();
|
||||||
const shouldUseMinimalStartup = Boolean(
|
const startupModeFlags = getStartupModeFlags(appState.initialArgs);
|
||||||
appState.initialArgs?.texthooker ||
|
const shouldUseMinimalStartup = startupModeFlags.shouldUseMinimalStartup;
|
||||||
(appState.initialArgs?.stats &&
|
const shouldSkipHeavyStartup = startupModeFlags.shouldSkipHeavyStartup;
|
||||||
(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),
|
|
||||||
);
|
|
||||||
if (!appState.initialArgs || (!shouldUseMinimalStartup && !shouldSkipHeavyStartup)) {
|
if (!appState.initialArgs || (!shouldUseMinimalStartup && !shouldSkipHeavyStartup)) {
|
||||||
if (isAnilistTrackingEnabled(getResolvedConfig())) {
|
if (isAnilistTrackingEnabled(getResolvedConfig())) {
|
||||||
void refreshAnilistClientSecretStateIfEnabled({ force: true }).catch((error) => {
|
void refreshAnilistClientSecretStateIfEnabled({ force: true }).catch((error) => {
|
||||||
|
|||||||
Reference in New Issue
Block a user