refactor(input): consolidate sim-hid + pointer-service + flutter-vm backends (#707 b)#733
Conversation
Move sim-hid, pointer-service, and flutter-vm backends from src/tools/ into src/input/ for consistency with the #707a split. The old paths are kept as thin re-export shims so all existing callers (tests, tools, integration suites) continue to work without modification. Internal callers that were already inside src/ are updated to import directly from the new canonical paths: - src/input/backend-resolver.ts → ./sim-hid-backend, ./pointer-service-backend, ./flutter-vm-backend - src/tools/diagnose.ts → ../input/sim-hid-backend - src/tools/pasteboard-input.ts → ../input/sim-hid-backend src/metrics/input-telemetry.ts and src/metrics/input-telemetry-rollup.ts already imported from ../input/backend directly (no change needed). Behavior is strictly unchanged: no fallback semantics, no public API, no tool schemas were altered. Decision on pointer-service and flutter-vm: both moves are mechanical (relocation + import-path update, zero logic change), so they are included here per the task spec. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase by consolidating input backend tools into the src/input/ directory while maintaining backward compatibility via re-export shims in src/tools/. The reviewer suggests optimizing runTapPs by moving dynamic imports to the top level for better performance and updating tryCreateSimulatorKitHIDBackend to return null instead of throwing an error when the bridge is missing, ensuring consistency with its documentation and other factory methods.
| const { execFile } = await import('child_process'); | ||
| const { promisify } = await import('util'); | ||
| const execFileAsync = promisify(execFile); |
There was a problem hiding this comment.
The dynamic imports for child_process and util inside runTapPs are executed on every tap call. Since these are standard Node.js modules and are already used by the delegate backend, they should be imported at the top level of the file to avoid unnecessary overhead and improve performance.
| const searched = candidates.map((c) => ` - ${c}`).join('\n'); | ||
| throw new InputBackendError( | ||
| `sim-hid-bridge not found. Searched:\n${searched}\n` + | ||
| 'Run npm run build or set OPENSAFARI_ALLOW_SWIFT_INTERPRETER=1 for dev mode.', | ||
| 'HID_BRIDGE_MISSING', | ||
| ); |
There was a problem hiding this comment.
The implementation of tryCreateSimulatorKitHIDBackend throws an InputBackendError when the bridge is not found, which contradicts its docstring (line 430) stating it returns null. It is also inconsistent with tryCreatePointerServiceBackend in pointer-service-backend.ts. To align with the documentation and provide a consistent API for the resolver, this should return null instead of throwing.
| const searched = candidates.map((c) => ` - ${c}`).join('\n'); | |
| throw new InputBackendError( | |
| `sim-hid-bridge not found. Searched:\n${searched}\n` + | |
| 'Run npm run build or set OPENSAFARI_ALLOW_SWIFT_INTERPRETER=1 for dev mode.', | |
| 'HID_BRIDGE_MISSING', | |
| ); | |
| return null; |
- Hoist runTapPs's child_process / util imports to module scope so each tap call no longer re-resolves them. Matches sim-hid-backend.ts which already imports execFile/promisify at the top. - Document tryCreateSimulatorKitHIDBackend's actual contract: returns a backend or throws InputBackendError(HID_BRIDGE_MISSING). The previous docstring said it returns null on missing bridge, which has never been the case (see tests/unit/sim-hid-input-backend.test.ts). Aligning the contract to null is a behavior change and is intentionally deferred so this PR remains a behavior-preserving file relocation.
|
Re: gemini's suggestion to change This PR is intentionally a behavior-preserving file relocation (per the PR description). The throw vs null decision is unchanged from before #707b — and the throw is currently asserted by What I did instead in 40e6c04: fixed the docstring to match actual behavior (returns a backend or throws |
|
/gemini review |
|
@codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Stacked on PR #731 (base =
refactor/707a-input-resolver). Will retarget todevelopafter #731 merges.Implements the #707 follow-up deferred from PR #731. Behavior-preserving moves + import path updates only. Closes #707 when combined with #707a (PR #731).
What changed
Three backend files are relocated from
src/tools/intosrc/input/to be consistent with the modules already split there by #707a:src/tools/sim-hid-input-backend.tssrc/input/sim-hid-backend.tssrc/tools/pointer-service-input-backend.tssrc/input/pointer-service-backend.tssrc/tools/flutter-vm-input-backend.tssrc/input/flutter-vm-backend.tsThe old
src/tools/paths become thin re-export shims so all existing external callers (tests, integration suites, any downstream consumers) continue to work without modification.Internal callers that live inside
src/are updated to import from the new canonical paths directly:src/input/backend-resolver.ts— previously crossed thesrc/input/→src/tools/boundary; now imports withinsrc/input/src/tools/diagnose.tsandsrc/tools/pasteboard-input.ts— both imported from./sim-hid-input-backend; updated to../input/sim-hid-backendsrc/metrics/input-telemetry.tsandsrc/metrics/input-telemetry-rollup.tsalready imported from../input/backenddirectly — no change needed.Decision: pointer-service and flutter-vm
Both moves are purely mechanical (file relocation + import-path updates, zero logic change), so they are included here per the task spec. No behaviour changes, no promotion of experimental status, no schema changes.
Validation
npx tsc --noEmit— 0 errorsnpm run build— cleannpx jest(5 input unit test suites, 175 tests) — all passnpx jest(2 telemetry suites, 28 tests) — all passnpm run lint -- --quiet— cleangit diff --check— cleanDiff is ~60 net new lines (shims + updated imports) on top of ~1200 lines moved.