refactor(workstation): extract modal overlay handlers into overlayInput.ts#1803
Conversation
…ut.ts Pulls the theme picker, gitignore picker, command palette, help, and view-keys handlers out of getLogInkInputEvents's monolithic router into a new handleOverlayInput function, following the same (state, inputValue, key, context) contract as surfaces/bisect/input.ts.
![gfargo-horizon-agent[bot]](https://avatars.githubusercontent.com/in/4083186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — LGTM
REVIEW: LGTM
RESOLVES: full
Faithful verbatim extraction of the five contiguous modal-overlay handlers into overlayInput.ts, delegated at the exact precedence spot (below splitPlan, above the g-chord Esc/q handler) with all moved imports and no stale references left in inkInput.ts. The one substantive finding is a latent circular-import nit; behavior is unchanged and the acceptance criteria are met.
1 nit — 1 inline on the diff
| import { filterThemePresets, getThemePickerSelection } from './themePicker' | ||
| import type { LogInkAction, LogInkState } from './inkViewModel' | ||
| import { | ||
| getLogInkPaletteExecuteEvents, |
There was a problem hiding this comment.
🧹 Runtime circular import overlayInput.ts <-> inkInput.ts
overlayInput.ts imports getLogInkPaletteExecuteEvents as a runtime (non-type) value from inkInput.ts, which in turn imports handleOverlayInput at module top level — a genuine require cycle. This diverges from the cited surfaces/bisect/input.ts contract, which imports only types from inkInput.ts and so has no runtime cycle. It is harmless today because the reference resolves inside a function body after both modules finish loading (and the full suite passes), but it is a latent footgun under stricter bundler/module-order configs; extracting or injecting getLogInkPaletteExecuteEvents would keep the module purely leaf-like.
Addresses review nit on #1803: overlayInput.ts's runtime import of getLogInkPaletteExecuteEvents forms a require cycle with inkInput.ts importing handleOverlayInput. It's safe (both bindings are only read inside function bodies invoked after module init) and confirmed by the full suite + a clean `npm run build`. Extracting the 261-line, multi-helper-dependent palette-execute mapping to fully avoid it is out of scope for this pass, so leave a comment explaining why.
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — LGTM
REVIEW: LGTM
RESOLVES: full
Verified the five overlay handlers were lifted byte-for-byte (whitespace-normalized diff of removed vs. new blocks is identical) into overlayInput.ts with the exact precedence order and delegated from the correct spot below splitPlan handling and above the g-chord Esc/q rung; imports migrated with zero leftover references and tsc/jest (431 pass)/eslint all clean. The one runtime import cycle (getLogInkPaletteExecuteEvents) is intentional, documented, and safe since both bindings are read only inside post-init function bodies.
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — LGTM
REVIEW: LGTM
RESOLVES: full
The five modal-overlay handlers are lifted verbatim into overlayInput.ts with identical conditions, branch order, and precedence (theme → gitignore → palette → help → view-keys), the delegating call is correctly placed between the splitPlan catch-all and the g-chord handler with context in scope, all stale imports are removed while the needed exports (getLogInkPaletteExecuteEvents, LogInkInput* types) exist and are used, and the full 431-test inkInput suite passes. The documented overlayInput↔inkInput require cycle is safe (both cross-references resolve inside function bodies invoked after m
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — LGTM
REVIEW: LGTM
RESOLVES: full
Verified verbatim extraction of the five modal-overlay handlers into overlayInput.ts preserving exact conditions, order, and returned events, placed at the correct precedence spot (below splitPlan, above the g-chord handler) with the (state, inputValue, key, context) contract. The three moved imports have no remaining references in inkInput.ts, the required types/functions are properly exported, and the single documented require cycle is confined to function-body use and therefore safe.
There was a problem hiding this comment.
🔎 Agent review (sonnet→opus) — LGTM
REVIEW: LGTM
RESOLVES: full
Verified: the five overlay handlers are lifted verbatim into overlayInput.ts, delegated from the identical precedence spot (between splitPlan's terminal return and the g-chord handler) with the correct null/empty-array fall-through semantics preserved, and the five now-unused imports cleanly removed with zero leftover references. All cross-module bindings (getLogInkPaletteExecuteEvents plus the three types) are exported from inkInput.ts, the require-cycle is real but correctly analyzed as safe, and out-of-scope handlers (input-prompt, pendingChoice) were untouched.
…ct-modal-overlay-handlers-into-runt # Conflicts: # src/workstation/runtime/inkInput.ts
1 participant
What
Extracts the five contiguous modal-overlay key handlers (theme picker, gitignore picker, command palette, help, view-keys) out of
getLogInkInputEvents's monolithic router ininkInput.tsinto a newhandleOverlayInputfunction inruntime/overlayInput.ts.Why
Plane: OSS-1061
Sub-item 1/4 of OSS-950 — trims
inkInput.tsby lifting a self-contained, mutually-exclusive block of overlay handlers into their own module, following the same extraction pattern already used forsurfaces/bisect/input.ts.How
src/workstation/runtime/overlayInput.tsexportshandleOverlayInput(state, inputValue, key, context): LogInkInputEvent[] | null, matching thehandleBisectInputcontract exactly.inkInput.ts:1712-1940replaced with a single delegating call at the same precedence spot — below splitPlan handling, above the g-chord Esc/q handler.filterThemePresets/getThemePickerSelection(./themePicker),getLogInkPaletteCommands/filterLogInkPaletteCommands(./inkKeymap), andderiveGitignoreOptions(../chrome/gitignore) moved to the new module; confirmed via grep no other references remain ininkInput.ts.getLogInkPaletteExecuteEventsstays ininkInput.ts(out of scope to move) and is imported back intooverlayInput.ts— this creates an intentional circular import, which is safe since it's a hoistedfunctiondeclaration (confirmed via a full build + test run).Testing
npm run build)inkInput.test.tsoverlay assertions unmodified, full suite green (5346 passed; 4 pre-existing tree-sitter WASM failures unrelated to this change, reproduced identically on unmodifiedmain)npm run lint, 0 errors)🤖 Generated by the harbor agent loop. Reviewed by a human before merge.