|
| 1 | +--- |
| 2 | +status: Accepted |
| 3 | +date: 2026-05-18 |
| 4 | +deciders: |
| 5 | + - aaronsb |
| 6 | + - claude |
| 7 | +related: [] |
| 8 | +--- |
| 9 | + |
| 10 | +# ADR-202: Split semantic router monolith into per-operation modules |
| 11 | + |
| 12 | +## Context |
| 13 | + |
| 14 | +`src/semantic/router.ts` had grown to **2,228 lines** — ~2.8× the project's |
| 15 | +800-line "split before adding more" quality threshold (#199). It was already |
| 16 | +oversized before the May 2026 contributor-PR work; every new operation or |
| 17 | +action grew it further (the #139 per-file-lock change had to be threaded into |
| 18 | +an already-2,200-line file because there was no smaller home). |
| 19 | + |
| 20 | +`SemanticRouter.executeOperation()` is a clean dispatcher to per-operation |
| 21 | +handlers (`executeVaultOperation`, `executeEditOperation`, …). These are |
| 22 | +natural module seams and already match the `src/semantic/operations/` layout |
| 23 | +CLAUDE.md prescribes. `executeVaultOperation` plus its vault-private helpers |
| 24 | +is by far the largest single unit (~the bulk of the file). |
| 25 | + |
| 26 | +CLAUDE.md calls the `IObsidianAPI`/router abstraction the architectural |
| 27 | +cornerstone, so this is a deliberate, mechanical-but-large refactor that must |
| 28 | +be staged and behaviour-preserving, not bundled into a feature PR. |
| 29 | + |
| 30 | +## Decision |
| 31 | + |
| 32 | +Introduce a **`RouterContext` interface** (the dependency surface a router |
| 33 | +exposes to extracted handlers) and extract per-operation handlers into |
| 34 | +`src/semantic/operations/*.ts` as free functions |
| 35 | +`executeXOperation(ctx: RouterContext, action, params)`. `SemanticRouter |
| 36 | +implements RouterContext` and passes **itself** as the context, so shared |
| 37 | +state and mutations propagate without getter/setter indirection. |
| 38 | +`executeOperation` becomes a thin dispatch (`return |
| 39 | +executeVaultOperation(this, action, params)`). |
| 40 | + |
| 41 | +**This PR is the first, focused stage** (the issue's own recommendation — |
| 42 | +"`executeVaultOperation` is the biggest single win and could be extracted |
| 43 | +first in isolation"): |
| 44 | + |
| 45 | +- New `operations/router-context.ts` — minimal `RouterContext` (the four |
| 46 | + members the vault path touches: `api`, `app?`, `fragmentRetriever`, |
| 47 | + `validator`); grows as further handlers are extracted. |
| 48 | +- New `operations/shared.ts` — `Params`, `SearchResultItem`, and the |
| 49 | + `paramStr/paramNum/paramBool` helpers, previously router-private and needed |
| 50 | + by every handler. |
| 51 | +- New `operations/vault.ts` — `executeVaultOperation` + its live helpers |
| 52 | + (`splitContent`, `sortFiles`, `copyFile`, `copyDirectoryRecursive`), |
| 53 | + mechanically transformed (`this.` → `ctx.`; the TypeScript compiler is the |
| 54 | + safety net — a missed rewrite is a compile error in a free function). |
| 55 | +- The router's relevant fields (`api`, `app`, `fragmentRetriever`, |
| 56 | + `validator`) are made `public readonly` to satisfy `RouterContext`. This |
| 57 | + widens visibility on the cornerstone class; the interface is the explicit |
| 58 | + boundary and the cost is accepted. |
| 59 | +- **Pre-existing dead code removed.** Extraction surfaced a fully |
| 60 | + unreachable file-search subtree — `combineSearchResults`, `isDirectory`, |
| 61 | + `performFileBasedSearch`, `indexVaultFiles`, and transitively |
| 62 | + `getSearchWorkflowHints`, `extractContext`, `getFileType` — private methods |
| 63 | + with **zero call sites** in the original `router.ts` or anywhere in |
| 64 | + `src/`/`tests/` (eslint's `no-unused-vars` does not flag unused class |
| 65 | + methods, which is why they survived). Removing unreachable code is |
| 66 | + behaviour-preserving by definition; ~290 lines eliminated outright. |
| 67 | + |
| 68 | +Result: `router.ts` **2,228 → 988 lines**; `vault.ts` 948; `make check` |
| 69 | +green with the **exact** pre-existing warning baseline (0 errors, 5 |
| 70 | +unrelated `prefer-active-doc` warnings) and **235/235** tests unchanged. |
| 71 | + |
| 72 | +### Scope and staging |
| 73 | + |
| 74 | +`router.ts` at 988 lines is **not yet under the 800-line acceptance**; |
| 75 | +`vault.ts` at 948 is itself over 800. Both are accepted for this stage: |
| 76 | + |
| 77 | +- Faithful to the issue's "extract vault first in isolation" guidance and |
| 78 | + to keeping one PR mechanically reviewable. |
| 79 | +- `vault.ts` maps exactly to the `operations/vault.ts` seam CLAUDE.md |
| 80 | + prescribes; `executeVaultOperation` is inherently the largest operation. |
| 81 | + Sub-splitting vault *by action* is a possible later refinement, not this |
| 82 | + ADR's seam. |
| 83 | +- #199 stays open with a follow-up: extract `edit`/`view`/`graph`/ |
| 84 | + `system`/`bases` the same way (router → thin dispatcher, < ~300 lines; |
| 85 | + each `operations/*.ts` independently testable). The acceptance closes when |
| 86 | + that lands. |
| 87 | + |
| 88 | +## Consequences |
| 89 | + |
| 90 | +### Positive |
| 91 | + |
| 92 | +- Removes the single worst quality-threshold violation in the codebase and |
| 93 | + ~290 lines of provably dead code. |
| 94 | +- Establishes the reusable `RouterContext` + free-function pattern; each |
| 95 | + subsequent handler extraction is now mechanical and low-risk. |
| 96 | +- `vault.ts` is independently testable without constructing the whole |
| 97 | + router. |
| 98 | +- No behaviour change: `tsc` proves the mechanical rewrite; the full suite |
| 99 | + is unchanged at 235/235. |
| 100 | + |
| 101 | +### Negative |
| 102 | + |
| 103 | +- Widens visibility of four router fields from `private` to `public |
| 104 | + readonly` (mitigated: `RouterContext` is the declared, narrow boundary; |
| 105 | + `readonly` prevents external mutation). |
| 106 | +- Two-stage (or more) resolution of #199 — the acceptance is not met by |
| 107 | + this PR alone. Accepted in exchange for reviewability and the issue's |
| 108 | + explicit "not bundled / extract vault first" guidance. |
| 109 | + |
| 110 | +### Neutral |
| 111 | + |
| 112 | +- `operations/shared.ts` now owns param helpers; `router.ts` and future |
| 113 | + operation modules import them rather than each redefining. |
| 114 | +- Follow-up PRs extend `RouterContext` with the members the remaining |
| 115 | + handlers touch (e.g. `context`, `tokenManager`, graph tools). |
| 116 | + |
| 117 | +## Alternatives Considered |
| 118 | + |
| 119 | +- **One PR extracting all seven handlers.** Rejected: same volume of change |
| 120 | + but far worse reviewability ("trust me" vs. a readable mechanical diff); |
| 121 | + contradicts the issue's "not bundled / first in isolation" guidance. |
| 122 | +- **Mixin / prototype assignment to split the class across files.** |
| 123 | + Rejected: fights TypeScript's type system, loses the compile-time safety |
| 124 | + net that makes the `this.`→`ctx.` rewrite verifiable. |
| 125 | +- **Thin delegating wrapper methods that call module functions.** Rejected: |
| 126 | + adds lines back to `router.ts`, working against the threshold goal for no |
| 127 | + structural benefit. |
| 128 | +- **Carry the dead code into `vault.ts` unchanged.** Rejected: it would add |
| 129 | + ~290 lines and 13 lint warnings to a fresh module; the code is provably |
| 130 | + unreachable so deletion is behaviour-preserving and the honest result of |
| 131 | + the move. |
0 commit comments