mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-02-27 18:22:41 -08:00
add investigation
This commit is contained in:
kyasuda
219
investigation.md
Normal file
219
investigation.md
Normal file
@@ -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<TContext>` 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.
|
||||
Reference in New Issue
Block a user