Repository navigation
refactor(rpc): one dispatcher - #27486
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
📥 Commits
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No merge-blocking behavior change was established; the refactor is mergeable after normal checks. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: consolidating RPC dispatch into one dispatcher. |
| Description check | |
The description includes all required sections and clearly explains the change, rationale, testing results, compatibility impact, and review considerations. It documents the two known baseline test fa… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
-
📝 Generate docstrings
-
-
- Commit to this branch
- Create a new PR
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Fold the streaming dispatcher, the unary invocation helper and two single-importer helpers into dispatcher.ts so request/response and streaming-unary share one unary path. No behaviour change.
a56e335 to
88baf48
Compare
ELI5
The code that answers RPC requests on an Orca host lived in five files, and the "run one ordinary request" logic was written twice: once for plain request/response connections and once for streaming connections. This PR puts it in one file with one copy of that logic. Nothing a user or a connected client sees changes.
What Changed
RpcDispatcher(dispatcher.ts) delegated streaming calls to a second class,RpcStreamingDispatcher(rpc-streaming-dispatcher.ts). Both called a shared helper (dispatcher-unary-method-invocation.ts), and each repeated the same admission and success/error reply code around it. The helper also depended on two tiny files, each used in one place (dispatcher-caller-fingerprint.ts,dispatcher-client-browser-routing.ts).RpcDispatcherhasdispatchanddispatchStreamingthat share one privateadmit(the existing admission prologue) and one privateinvokeUnary. The four files above are deleted. Their logic moved over unchanged, in the same order:mapDispatcherError, so the git/filesystem error text and the PTY error prefix that the renderer matches are unchanged;config/scripts/pr-code-change-scope.mjsno longer lists the deleted file. Itssrc/main/runtime/rpc/dispatcherprefix still coversdispatcher.ts, so the cross-version wire suites still run when the dispatcher changes.Net lines: production −101 (+157 / −258 across
src/and the CI scope script), tests −1.Why
With two copies of the unary path, every RPC change had to be made twice and could drift between the two transports. One class with one unary path removes that risk. Alternatives I considered:
dispatcher-streaming-feature-emitter.tsanddispatcher-stream-options.ts, as the plan suggested. I kept both: a native-chat payload budget test imports the emitter directly, and the stream-options type is imported by seven modules.routeClientHostedBrowserRpc, as the plan suggested. I tried a required call: 640 tests in 94 dispatcher test files failed, because their fake runtimes don't define the method. The optional call stays, now as plain?.with a one-line reason instead of a type cast.Linked Issue
Part of the Phase 3.99 cleanup (PR D1). Maintainer PR, no separate issue.
Visual Proof
N/A. Internal refactor with no UI or behaviour change.
Testing
pnpm tc,pnpm lint,oxlint,pnpm run check:code-quality:changed: all pass.pnpm run build:electron-vite,build:cli,build:orcad: all pass.Dead-code proof:
rgacrosssrc,config,scripts,tests,mobile,packagesand dotfiles (including the@ts-nocheckfiles) finds no remaining reference to the deleted files or symbols.Every test file that constructs
RpcDispatcher(165 files, including the transport-parity, orchestration legacy compatibility and takeover, emulator/computer error and browser client automation suites), plus the native-chat payload budget test and the CI scope tests: 1519 passed, 2 failed. The same 2 also fail on unmodifiedmain(structured-agent-session-subscriber-journal-retention,session-tabs-inventory-rpcpublication-waiter abort).Platforms: macOS locally. The change touches no OS-specific, SSH or path code.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
No new tests: this is a behaviour-preserving move, and the existing transport-parity and dispatcher suites cover both transports.
Review
Agent skill upstream boundary
Notes
No wire or schema change. Replies, error codes and messages, and reply ordering are unchanged for owner, mobile and paired-runtime callers, so mixed client and host versions are unaffected. SSH and folder workspaces are unaffected.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)