diff --git a/docs/refactor/FINAL_SUMMARY.md b/docs/refactor/FINAL_SUMMARY.md new file mode 100644 index 0000000..1081664 --- /dev/null +++ b/docs/refactor/FINAL_SUMMARY.md @@ -0,0 +1,97 @@ +# Final Cleanup Summary + +## Scope + +Executed the final global cleanup pass using `docs/refactor` as the source of truth. + +Source documents reviewed before code changes: + +- `docs/refactor/ARCHITECTURE_AUDIT.md` +- `docs/refactor/ARCHITECTURE_RULES.md` +- `docs/refactor/TARGET_ARCHITECTURE.md` +- `docs/refactor/MIGRATION_PLAN.md` +- `docs/refactor/SESSION_HANDOFF.md` +- `docs/refactor/PHASE_1_SUMMARY.md` +- `docs/refactor/PHASE_1_FIX_SUMMARY.md` +- `docs/refactor/PHASE_2_SUMMARY.md` +- `docs/refactor/PHASE_3_SUMMARY.md` +- `docs/refactor/PHASE_4_SUMMARY.md` + +## Cleanup Completed + +- Removed the unused `parseEnvInt` helper and its tests. +- Consolidated duplicated `unknown` parsing helpers into `src/main/utils/unknown-values.ts`. +- Narrowed `ApplicationController` path typing to reuse `ApplicationPaths`. +- Narrowed update config module boundaries so updater settings only accept the runtime config fields they need. +- Narrowed the NodeCG process manager child-process contract to the actual process surface it uses. +- Removed `as unknown as ChildProcess` casts from process-manager tests. +- Fixed update dialog Spanish text encoding. +- Added an explicit return type to the update dialog message-box helper. +- Renamed legacy updater test files: + - `update-settings.test.ts` -> `update-config.test.ts` + - `update-utils.test.ts` -> `update-schema.test.ts` + +## Architecture Preserved + +- No UX changes. +- No new features. +- No renderer, preload, or IPC layer. +- No NodeCG runtime model changes. +- No Electron permission expansion. +- No broad framework or new lifecycle layer. +- BrowserWindow security posture remains explicit. +- Update validation and download boundaries remain separated. +- Managed runtime preservation of `cfg`, `db`, and `logs` remains unchanged. + +## Verification + +Passed: + +```text +npm.cmd run typecheck +npm.cmd exec -- tsc --noEmit --noUnusedLocals --noUnusedParameters +npm.cmd test +npm.cmd run lint +npm.cmd run build +npm.cmd run doctor +``` + +Test result: + +```text +63 tests passing +``` + +Sanity searches passed for production/test source: + +```text +rg -n "parseEnvInt\(|ActualizaciÃ|estÃ|versiÃ|cerrarÃ|update-utils|update-settings|\bany\b|unknown as|as unknown|@ts-ignore|@ts-expect-error|eslint-disable|nodeIntegration:\s*true|webSecurity:\s*false" src scripts +``` + +Result: + +- No `any` in `src` or `scripts`. +- No `as unknown` casts remain. +- No legacy updater module names remain in `src`. +- No Spanish mojibake remains in update dialog source. +- No unsafe Electron settings were introduced. + +IPC/preload sanity: + +```text +rg -n "ipcMain|ipcRenderer|contextBridge|preload" src scripts +``` + +Result: + +- Matches are limited to the regression test that guards the no-IPC/no-preload policy. + +## Build Notes + +The first non-escalated `npm.cmd run build` attempt was blocked by sandbox permissions while creating generated parent-repo output at: + +```text +C:\Users\pcantos\Documents\scoreko-dev\shared\dist +``` + +The escalated rerun passed. The build emitted existing dependency/deprecation warnings during runtime dependency installation, but completed successfully and `npm.cmd run doctor` validated the prepared runtime. diff --git a/src/main/app/application-controller.ts b/src/main/app/application-controller.ts index ec60c74..fae32a5 100644 --- a/src/main/app/application-controller.ts +++ b/src/main/app/application-controller.ts @@ -2,6 +2,7 @@ import { AppRuntimeConfig } from "../config/runtime-config"; import { NodecgProcessManager } from "../nodecg/process-manager"; import { PreparedNodecgRuntime } from "../nodecg/runtime-provisioner"; import { getRemainingDelayMs } from "../utils/timing"; +import { ApplicationPaths } from "./paths"; import { createShutdownService, ShutdownService } from "./shutdown-service"; export type ApplicationState = "idle" | "preparing" | "starting" | "ready" | "stopping" | "stopped" | "failed"; @@ -21,14 +22,7 @@ export type ApplicationControllerConfig = { appVersion: string; isPackaged: boolean; isWindows: boolean; - paths: { - rootPath: string; - sourceNodecgRuntimePath: string; - userDataPath: string; - nodecgBaseUrl: string; - mainDashboardUrl: string; - loadingDashboardUrl: string; - }; + paths: ApplicationPaths; deps: { createLoadingWindow: () => ApplicationWindow; createMainWindow: () => ApplicationWindow; diff --git a/src/main/config/runtime-config.ts b/src/main/config/runtime-config.ts index 05ce75f..6ec2409 100644 --- a/src/main/config/runtime-config.ts +++ b/src/main/config/runtime-config.ts @@ -53,16 +53,6 @@ export function getEnv(name: string, fallback: string): string { return getOptionalEnv(name) ?? fallback; } -export function parseEnvInt(name: string, fallback: number): number { - const rawValue = process.env[name]; - if (!rawValue) { - return fallback; - } - - const parsedValue = Number.parseInt(rawValue, 10); - return Number.isFinite(parsedValue) ? parsedValue : fallback; -} - export function parseEnvIntInRange(name: string, fallback: number, min: number, max: number): number { // We throw here instead of silently coercing to avoid hidden misconfiguration in production. const rawValue = process.env[name]; diff --git a/src/main/nodecg/platform-process-killer.ts b/src/main/nodecg/platform-process-killer.ts index 5edeb4e..1af2d90 100644 --- a/src/main/nodecg/platform-process-killer.ts +++ b/src/main/nodecg/platform-process-killer.ts @@ -1,12 +1,16 @@ -import { ChildProcess, SpawnOptions } from "node:child_process"; +import { SpawnOptions } from "node:child_process"; export type PlatformProcessKillerDeps = { platform: NodeJS.Platform; - spawnProcess: (command: string, args: string[], options: SpawnOptions) => ChildProcess; + spawnProcess: (command: string, args: string[], options: SpawnOptions) => SpawnedKillerProcess; killProcess: (pid: number, signal: NodeJS.Signals) => void; log: (...args: unknown[]) => void; }; +type SpawnedKillerProcess = { + on: (event: "error", listener: (error: Error) => void) => unknown; +}; + export function killProcessTree(pid: number, signal: NodeJS.Signals, deps: PlatformProcessKillerDeps): boolean { if (!Number.isSafeInteger(pid) || pid <= 0) { deps.log(`Invalid pid for process tree termination: ${pid}`); diff --git a/src/main/nodecg/process-manager.ts b/src/main/nodecg/process-manager.ts index 5f6b879..69a12a4 100644 --- a/src/main/nodecg/process-manager.ts +++ b/src/main/nodecg/process-manager.ts @@ -1,4 +1,4 @@ -import { ChildProcess, spawn, SpawnOptions } from "node:child_process"; +import { spawn, SpawnOptions } from "node:child_process"; import fs from "node:fs"; import net from "node:net"; import path from "node:path"; @@ -17,7 +17,7 @@ type NodecgProcessManagerConfig = { }; type NodecgProcessManagerDeps = { - spawnProcess: (command: string, args: string[], options: SpawnOptions) => ChildProcess; + spawnProcess: (command: string, args: string[], options: SpawnOptions) => NodecgChildProcess; pathExists: (candidatePath: string) => boolean; fetchUrl: typeof fetch; platform: NodeJS.Platform; @@ -31,6 +31,22 @@ type NodecgProcessManagerDeps = { hasReadWriteAccess: (candidatePath: string) => boolean; }; +type NodecgChildProcess = { + pid?: number; + killed: boolean; + exitCode: number | null; + signalCode: NodeJS.Signals | null; + stdout?: ProcessOutputStream | null; + stderr?: ProcessOutputStream | null; + on(event: "exit", listener: (code: number | null, signal: NodeJS.Signals | null) => void): unknown; + on(event: "error", listener: (error: Error) => void): unknown; + once(event: "exit", listener: () => void): unknown; +}; + +type ProcessOutputStream = { + on(event: "data", listener: (chunk: unknown) => void): unknown; +}; + export type NodecgProcessManager = { startNodecgProcess: () => Promise; waitForNodecgReady: (startTime: number) => Promise; @@ -50,7 +66,7 @@ export function createNodecgProcessManager({ }: NodecgProcessManagerConfig): NodecgProcessManager { const resolvedDeps = resolveDeps(deps); - let nodecgProcess: ChildProcess | null = null; + let nodecgProcess: NodecgChildProcess | null = null; let nodecgState: NodecgProcessState = "idle"; let startNodecgPromise: Promise | null = null; let stopNodecgPromise: Promise | null = null; diff --git a/src/main/updates/update-config.ts b/src/main/updates/update-config.ts index f43e721..2852327 100644 --- a/src/main/updates/update-config.ts +++ b/src/main/updates/update-config.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import { getDefaultUpdateConfigPath } from "../app/paths"; import { AppRuntimeConfig } from "../config/runtime-config"; +import { isRecord, readNonEmptyString } from "../utils/unknown-values"; import { UpdateFileConfig, validateHttpUrl } from "./update-schema"; const DEFAULT_UPDATE_ASSET_PATTERN = "Scoreko-setup-.*\\.exe$"; @@ -17,8 +18,17 @@ type UpdateConfigOptions = { allowInsecureHttp: boolean; }; +type UpdateRuntimeConfig = Pick< + AppRuntimeConfig, + | "updateApiUrl" + | "updateAssetPattern" + | "updateConfigPathOverride" + | "updateReleasePageUrl" + | "updatesEnabled" +>; + export function loadUpdateSettings( - appConfig: AppRuntimeConfig, + appConfig: UpdateRuntimeConfig, rootPath: string, log: (...args: unknown[]) => void, options: UpdateConfigOptions = { allowInsecureHttp: true }, @@ -32,12 +42,12 @@ export function loadUpdateSettings( ...(apiUrl ? { apiUrl } : {}), ...(releasePageUrl ? { releasePageUrl } : {}), assetPattern: - appConfig.updateAssetPattern || readOptionalString(fileConfig.assetPattern) || DEFAULT_UPDATE_ASSET_PATTERN, + appConfig.updateAssetPattern || readNonEmptyString(fileConfig.assetPattern) || DEFAULT_UPDATE_ASSET_PATTERN, }; } export function readUpdateFileConfig( - appConfig: AppRuntimeConfig, + appConfig: Pick, rootPath: string, log: (...args: unknown[]) => void, ): UpdateFileConfig { @@ -70,18 +80,10 @@ function normalizeUpdateFileConfig(value: unknown): UpdateFileConfig { } function readOptionalHttpUrl(value: unknown, options: UpdateConfigOptions): string | undefined { - const rawValue = readOptionalString(value); + const rawValue = readNonEmptyString(value); if (!rawValue) { return undefined; } return validateHttpUrl(rawValue, options) ?? undefined; } - -function readOptionalString(value: unknown): string | undefined { - return typeof value === "string" && value.trim().length > 0 ? value.trim() : undefined; -} - -function isRecord(value: unknown): value is Record { - return typeof value === "object" && value !== null && !Array.isArray(value); -} diff --git a/src/main/updates/update-dialogs.ts b/src/main/updates/update-dialogs.ts index 0934296..d448fae 100644 --- a/src/main/updates/update-dialogs.ts +++ b/src/main/updates/update-dialogs.ts @@ -1,5 +1,5 @@ import { BrowserWindow, dialog } from "electron"; -import type { MessageBoxOptions } from "electron"; +import type { MessageBoxOptions, MessageBoxReturnValue } from "electron"; import { ReleaseUpdate } from "./update-schema"; @@ -41,6 +41,9 @@ export async function askToInstallUpdate(update: ReleaseUpdate, parentWindow: Br return result.response === 0; } -function showMessageBox(parentWindow: BrowserWindow | null, options: MessageBoxOptions) { +function showMessageBox( + parentWindow: BrowserWindow | null, + options: MessageBoxOptions, +): Promise { return parentWindow ? dialog.showMessageBox(parentWindow, options) : dialog.showMessageBox(options); } diff --git a/src/main/updates/update-schema.ts b/src/main/updates/update-schema.ts index 8e6334b..10e3172 100644 --- a/src/main/updates/update-schema.ts +++ b/src/main/updates/update-schema.ts @@ -1,3 +1,5 @@ +import { isRecord, readNonEmptyString } from "../utils/unknown-values"; + export type GiteaReleaseAsset = { name: string; browserDownloadUrl: string; @@ -47,11 +49,14 @@ export function parseGiteaRelease(value: unknown): GiteaRelease | null { return null; } + const title = readNonEmptyString(value.name); + const pageUrl = readOptionalUrlString(value.html_url); + return { tagName, assets, - ...(readOptionalString(value.name) ? { title: readOptionalString(value.name) } : {}), - ...(readOptionalUrlString(value.html_url) ? { pageUrl: readOptionalUrlString(value.html_url) } : {}), + ...(title ? { title } : {}), + ...(pageUrl ? { pageUrl } : {}), }; } @@ -177,16 +182,12 @@ function normalizeVersion(version: string): number[] { } function readRequiredString(value: unknown): string | null { - const text = readOptionalString(value); + const text = readNonEmptyString(value); return text && text.length > 0 ? text : null; } -function readOptionalString(value: unknown): string | undefined { - return typeof value === "string" && value.trim().length > 0 ? value.trim() : undefined; -} - function readOptionalUrlString(value: unknown): string | undefined { - const rawValue = readOptionalString(value); + const rawValue = readNonEmptyString(value); if (!rawValue) { return undefined; } @@ -194,10 +195,6 @@ function readOptionalUrlString(value: unknown): string | undefined { return validateHttpUrl(rawValue, { allowInsecureHttp: true }) ?? undefined; } -function isRecord(value: unknown): value is Record { - return typeof value === "object" && value !== null && !Array.isArray(value); -} - function isPresent(value: T | null): value is T { return value !== null; } diff --git a/src/main/utils/unknown-values.ts b/src/main/utils/unknown-values.ts new file mode 100644 index 0000000..475a430 --- /dev/null +++ b/src/main/utils/unknown-values.ts @@ -0,0 +1,7 @@ +export function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +export function readNonEmptyString(value: unknown): string | undefined { + return typeof value === "string" && value.trim().length > 0 ? value.trim() : undefined; +} diff --git a/src/tests/process-manager.test.ts b/src/tests/process-manager.test.ts index e279633..bbb50d7 100644 --- a/src/tests/process-manager.test.ts +++ b/src/tests/process-manager.test.ts @@ -85,7 +85,7 @@ test("waitForNodeCGReady resolves when endpoint returns 404", async () => { deps: { platform: "linux", pathExists: () => true, - spawnProcess: () => child as unknown as import("node:child_process").ChildProcess, + spawnProcess: () => child, fetchUrl: async () => ({ ok: false, status: 404 }) as Response, setTimer: (handler: (...args: unknown[]) => void, _timeoutMs: number) => { handler(); @@ -118,7 +118,7 @@ test("stopNodeCG sends SIGTERM and then SIGKILL if the process does not exit", a deps: { platform: "linux", pathExists: () => true, - spawnProcess: () => child as unknown as import("node:child_process").ChildProcess, + spawnProcess: () => child, fetchUrl: async () => ({ ok: false, status: 404 }) as Response, killProcess: (pid, signal) => { killSignals.push({ pid, signal }); @@ -163,7 +163,7 @@ test("stopNodeCG reuses the same promise when invoked in parallel", async () => log: () => undefined, deps: { pathExists: () => true, - spawnProcess: () => child as unknown as import("node:child_process").ChildProcess, + spawnProcess: () => child, fetchUrl: async () => ({ ok: false, status: 404 }) as Response, killProcess: () => undefined, setTimer: () => 0, @@ -206,7 +206,7 @@ test("startNodeCG reuses the same promise while startup is in progress", async ( }), spawnProcess: () => { spawnCalls += 1; - return child as unknown as import("node:child_process").ChildProcess; + return child; }, stdoutWrite: () => undefined, stderrWrite: () => undefined, @@ -241,7 +241,7 @@ test("stopNodeCG normalizes negative timeout to zero", async () => { log: () => undefined, deps: { pathExists: () => true, - spawnProcess: () => child as unknown as import("node:child_process").ChildProcess, + spawnProcess: () => child, fetchUrl: async () => ({ ok: false, status: 404 }) as Response, killProcess: () => undefined, setTimer: (handler, timeoutMs) => { @@ -306,7 +306,7 @@ test("startNodeCG spawns Electron directly on Windows", async () => { capturedCommand = command; capturedArgs = args; capturedOptions.push(options); - return child as unknown as import("node:child_process").ChildProcess; + return child; }, stdoutWrite: () => undefined, stderrWrite: () => undefined, @@ -333,7 +333,7 @@ test("waitForNodeCGReady exposes diagnostics when NodeCG exits before readiness" deps: { pathExists: () => true, platform: "linux", - spawnProcess: () => child as unknown as import("node:child_process").ChildProcess, + spawnProcess: () => child, fetchUrl: async () => { child.emit("exit", 1, null); throw new Error("still starting"); diff --git a/src/tests/runtime-config.test.ts b/src/tests/runtime-config.test.ts index 87fa6ff..a283f96 100644 --- a/src/tests/runtime-config.test.ts +++ b/src/tests/runtime-config.test.ts @@ -5,7 +5,6 @@ import { getEnv, getOptionalEnv, parseEnvBool, - parseEnvInt, parseEnvIntInRange, parseEnvPort, parseOptionalHttpUrl, @@ -56,18 +55,6 @@ test("getEnv returns the value when present", () => { }); }); -test("parseEnvInt returns fallback for invalid values", () => { - withEnv("TEST_ENV_INT", "abc", () => { - assert.equal(parseEnvInt("TEST_ENV_INT", 100), 100); - }); -}); - -test("parseEnvInt parses valid integers", () => { - withEnv("TEST_ENV_INT", "4500", () => { - assert.equal(parseEnvInt("TEST_ENV_INT", 100), 4500); - }); -}); - test("parseEnvIntInRange hard-fails for out-of-range values", () => { withEnv("TEST_ENV_INT_RANGE", "999", () => { assert.throws(() => parseEnvIntInRange("TEST_ENV_INT_RANGE", 100, 0, 100), /must be an integer/); diff --git a/src/tests/update-settings.test.ts b/src/tests/update-config.test.ts similarity index 98% rename from src/tests/update-settings.test.ts rename to src/tests/update-config.test.ts index 270602a..8462936 100644 --- a/src/tests/update-settings.test.ts +++ b/src/tests/update-config.test.ts @@ -80,7 +80,7 @@ test("readUpdateFileConfig normalizes malformed config into an empty file config }); test("readUpdateFileConfig logs invalid JSON and returns an empty file config", () => { - const rootPath = fs.mkdtempSync(path.join(os.tmpdir(), "scoreko-update-settings-")); + const rootPath = fs.mkdtempSync(path.join(os.tmpdir(), "scoreko-update-config-")); const staticPath = path.join(rootPath, "static"); fs.mkdirSync(staticPath, { recursive: true }); fs.writeFileSync(path.join(staticPath, "updates.json"), "{ invalid", "utf8"); @@ -95,7 +95,7 @@ test("readUpdateFileConfig logs invalid JSON and returns an empty file config", }); function makeTempRoot(config: unknown): string { - const rootPath = fs.mkdtempSync(path.join(os.tmpdir(), "scoreko-update-settings-")); + const rootPath = fs.mkdtempSync(path.join(os.tmpdir(), "scoreko-update-config-")); const staticPath = path.join(rootPath, "static"); fs.mkdirSync(staticPath, { recursive: true }); diff --git a/src/tests/update-utils.test.ts b/src/tests/update-schema.test.ts similarity index 100% rename from src/tests/update-utils.test.ts rename to src/tests/update-schema.test.ts