diff --git a/investigation.md b/investigation.md new file mode 100644 index 0000000..4266383 --- /dev/null +++ b/investigation.md @@ -0,0 +1,219 @@ +# Refactoring Investigation Report + +## Overview + +This report evaluates the SubMiner refactoring effort on the `refactor` branch against the plan defined in `plan.md` and the safety checklist in `docs/refactor-main-checklist.md`. The refactoring aimed to eliminate unnecessary abstraction layers, consolidate related services, fix known bugs, and add test coverage. + +**Result**: 65 commits, 104 files changed, main.ts reduced from ~5,800 to 1,398 lines, all 67 tests passing, build succeeds. + +--- + +## Phase 1: Delete Thin Wrappers — COMPLETE + +All 9 target wrapper services and 7 associated test files have been deleted and their logic inlined into callers (mostly main.ts or the services they fed). + +| Target File | Status | +|-------------|--------| +| `config-warning-runtime-service.ts` + test | Deleted, inlined | +| `overlay-modal-restore-service.ts` + test | Deleted, inlined | +| `runtime-options-manager-runtime-service.ts` + test | Deleted, inlined | +| `app-logging-runtime-service.ts` + test | Deleted, inlined | +| `overlay-send-service.ts` (no test) | Deleted, inlined | +| `startup-resource-runtime-service.ts` + test | Deleted, inlined | +| `config-generation-runtime-service.ts` + test | Deleted, inlined | +| `app-shutdown-runtime-service.ts` + test | Deleted, inlined | +| `shortcut-ui-deps-runtime-service.ts` + test | Deleted, inlined | + +**Files removed**: 16 (9 services + 7 tests) +**No issues found.** + +--- + +## Phase 2: Consolidate DI Adapter Services — COMPLETE + +All 4 dependency-injection adapter services have been merged into the services they fed, and their test files removed. + +| Adapter | Merged Into | Status | +|---------|------------|--------| +| `cli-command-deps-runtime-service.ts` + test | `cli-command-service.ts` | Done | +| `ipc-deps-runtime-service.ts` + test | `ipc-service.ts` | Done | +| `tokenizer-deps-runtime-service.ts` + test | `tokenizer-service.ts` | Done | +| `app-lifecycle-deps-runtime-service.ts` + test | `app-lifecycle-service.ts` | Done | + +**Files removed**: 8 (4 services + 4 tests) +**No issues found.** + +--- + +## Phase 3: Consolidate Related Services — COMPLETE + +All planned service merges have been executed. + +| Consolidation | Source Files | Result File | Status | +|--------------|-------------|-------------|--------| +| Overlay visibility | `overlay-visibility-runtime-service.ts` | `overlay-visibility-service.ts` | Done | +| Overlay broadcast | `overlay-broadcast-runtime-service.ts` + test | `overlay-manager-service.ts` | Done | +| Overlay shortcuts | `overlay-shortcut-lifecycle-service.ts`, `overlay-shortcut-fallback-runner.ts` | `overlay-shortcut-service.ts` + `overlay-shortcut-handler.ts` | Done | +| Numeric shortcuts | `numeric-shortcut-runtime-service.ts` + test, `numeric-shortcut-session-service.ts` | `numeric-shortcut-service.ts` | Done | +| Startup/bootstrap | `startup-bootstrap-runtime-service.ts` + test, `app-ready-runtime-service.ts` + test | `startup-service.ts` | Done | + +**Files removed**: ~10 +**No issues found.** + +--- + +## Phase 4: Fix Bugs and Code Quality — COMPLETE + +### 4.1 Debug console.log statements +**Status**: RESOLVED — No debug `console.log` or `console.warn` calls remain in `overlay-visibility-service.ts`. + +### 4.2 `as never` type cast +**Status**: RESOLVED — No `as never` cast remains in `tokenizer-service.ts`. The type mismatch was fixed properly. + +### 4.3 fieldGroupingResolver race condition +**Status**: RESOLVED — Fixed in `main.ts` (lines 305–332) with a sequence counter mechanism. Each new resolver increments `fieldGroupingResolverSequence`, and wrapped resolvers check if their sequence matches the current value before executing. Stale resolutions are correctly ignored. + +### 4.4 Async callback safety +**Status**: RESOLVED +- **CLI commands** (`cli-command-service.ts`): Async commands use `runAsyncWithOsd` helper (lines 177–187) which catches errors, logs them, and displays via MPV OSD. +- **IPC handlers** (`ipc-service.ts`): Async handlers use `ipcMain.handle` (not `.on`), which properly awaits and propagates promise results. Synchronous handlers correctly use `ipcMain.on`. + +### 4.5 `-runtime-service` naming convention +**Status**: RESOLVED — No files with `-runtime-service` in the name exist under `src/core/services/`. All have been renamed to `*-service.ts`. + +--- + +## Phase 5: Add Tests for Critical Untested Services — COMPLETE + +Tests added per plan: + +| Service | Test File | Tests | +|---------|-----------|-------| +| `mpv-service.ts` (761 lines) | `mpv-service.test.ts` | Socket protocol, property changes, reconnection | +| `subsync-service.ts` (427 lines) | `subsync-service.test.ts` | Config resolution, command construction, error handling | +| `tokenizer-service.ts` (305 lines) | `tokenizer-service.test.ts` | Parser init, token extraction, edge cases | +| `cli-command-service.ts` (204 lines) | `cli-command-service.test.ts` (290 lines) | Expanded: all dispatch paths, error propagation | + +**Total**: 67 tests passing across all test suites. + +--- + +## Phase 6: Directory Restructure — NOT STARTED (Optional) + +Services remain in the flat `src/core/services/` directory. This phase was explicitly marked optional and low-priority. The current flat structure with ~25 files is manageable. + +--- + +## Checklist Compliance (docs/refactor-main-checklist.md) + +### Invariants + +| Invariant | Status | +|-----------|--------| +| CLI flags and aliases working | Verified via tests | +| IPC channel names backward-compatible | No channel names changed | +| Overlay toggle behavior preserved | Logic moved verbatim | +| MPV integration behavior preserved | Logic moved verbatim, tested | +| Texthooker mode preserved | Not altered | +| Mining/runtime options paths preserved | Logic moved verbatim | + +### Automated Checks + +| Check | Status | +|-------|--------| +| `pnpm run build` | Passes | +| `pnpm run test:core` | 67/67 passing | + +### Manual Smoke Checks + +**Status**: NOT YET PERFORMED — Visual overlay rendering, card mining flow, and field-grouping interaction require a real desktop session with MPV. Automated tests validate logic correctness but cannot catch rendering regressions. + +--- + +## Loose Ends and Concerns + +### 1. Unused Architectural Scaffolding (~500 lines, dead code) + +The following files exist in the repository but are **not imported or used anywhere**: + +**Core abstractions:** +| File | Lines | Purpose | +|------|-------|---------| +| `src/core/action-bus.ts` | 22 | Generic action dispatcher (`register`/`dispatch`) | +| `src/core/actions.ts` | 17 | Union type `AppAction` with 16 action variants | +| `src/core/app-context.ts` | 46 | Module context interfaces | +| `src/core/module-registry.ts` | 37 | Module lifecycle manager (init/start/stop) | +| `src/core/module.ts` | 7 | `SubminerModule` interface | + +**Module implementations:** +| File | Lines | Purpose | +|------|-------|---------| +| `src/modules/runtime-options/module.ts` | 62 | Runtime options module | +| `src/modules/subsync/module.ts` | 79 | Subsync module | +| `src/modules/jimaku/module.ts` | 73 | Jimaku module | + +**IPC abstraction layer:** +| File | Lines | Purpose | +|------|-------|---------| +| `src/ipc/contract.ts` | 62 | IPC channel name constants | +| `src/ipc/main-api.ts` | 20 | Main process IPC helpers | +| `src/ipc/renderer-api.ts` | 28 | Renderer process IPC helpers | + +These represent scaffolding for a module-based architecture that was prototyped but never wired into main.ts. The current codebase uses the service-oriented approach throughout. + +**Recommendation**: Remove if abandoned, or document with clear intent if planned for a future phase. + +### 2. New Consolidated Services Without Test Coverage + +Seven service files created or consolidated during Phase 3 lack dedicated tests: + +| File | Lines | Risk Level | Reason | +|------|-------|------------|--------| +| `overlay-shortcut-handler.ts` | 216 | Higher | Complex runtime handlers with fallback logic | +| `mining-service.ts` | 179 | Higher | 6 public functions orchestrating mining workflows | +| `anki-jimaku-service.ts` | 173 | Higher | Complex IPC registration with multiple handlers | +| `startup-service.ts` | ~150 | Medium | Bootstrap and app-ready orchestration | +| `numeric-shortcut-service.ts` | 133 | Medium | Session state management | +| `subsync-runner-service.ts` | 86 | Lower | Thin runtime wrapper | +| `jimaku-service.ts` | 81 | Lower | Config accessor functions | + +Phase 5 targeted the 4 highest-risk *previously existing* untested services. The Phase 3 consolidation created new files that inherited logic from multiple sources — those were not explicitly called out in the plan for testing. + +### 3. Null Safety in Consolidated Services + +All checked consolidated services handle null/undefined correctly: + +- **`mpv-control-service.ts`**: Accepts `MpvRuntimeClientLike | null`, uses null checks and optional chaining before all access. +- **`overlay-bridge-service.ts`**: Returns `false` early if window is null or destroyed (`line 17: if (!options.mainWindow || options.mainWindow.isDestroyed()) return false`). +- **`startup-service.ts`**: Uses dependency injection with all deps provided upfront, async operations awaited in sequence, config access uses optional chaining. + +**No null safety issues found.** + +### 4. Import Consistency + +All 68 imports in `main.ts` from `./core/services` resolve to existing exports. No references to deleted files remain. The barrel export in `index.ts` has 79 entries with no dead exports (one minor case: `isGlobalShortcutRegisteredSafe` is only used within the service layer itself, not by main.ts). + +--- + +## Risk Assessment + +| Area | Risk | Mitigation | +|------|------|------------| +| Logic regression from inlining | Low | Code moved verbatim, 67 tests pass | +| Overlay rendering regression | Medium | Requires manual smoke test with MPV | +| Unused scaffolding becoming stale | Low | Remove or document with clear intent | +| Missing test coverage on new files | Medium | Add tests for the 3 higher-risk services | +| Race conditions | Low | fieldGroupingResolver race fixed with sequence counter | +| Async error swallowing | Low | All async paths have error boundaries | + +--- + +## Recommendations + +1. **Remove unused scaffolding** (`src/core/action-bus.ts`, `src/core/actions.ts`, `src/core/app-context.ts`, `src/core/module-registry.ts`, `src/core/module.ts`, `src/modules/`, `src/ipc/`) — ~500 lines of dead code that contradicts the refactoring goal of reducing unnecessary abstraction. + +2. **Add tests for higher-risk consolidated services** — `overlay-shortcut-handler.ts`, `mining-service.ts`, and `anki-jimaku-service.ts` have the most complex logic among the untested new files. + +3. **Perform desktop smoke test** — Verify overlay rendering, card mining, and field-grouping interaction in a real session with MPV running. + +4. **Consider removing `isGlobalShortcutRegisteredSafe` from barrel export** — It's only used internally by the service layer, not by main.ts.