🤖 refactor: reintroduce ORPC migration for type-safe RPC #784
+15,861
−13,392
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Member
Author
|
@codex review |
ThomasK33
force-pushed
the
orpc
branch
2 times, most recently
from
2fc3426 to
c512fb7
Compare
This commit reintroduces the ORPC refactoring that was originally merged in #763 and subsequently reverted in #777 due to regressions. ## Original PR: #763 Replaces the custom IPC layer with oRPC for type-safe RPC between browser/renderer and backend processes. ## Why it was reverted (#777) The original migration caused regressions including: - Streaming content delay from ORPC schema validation - Field stripping issues in sendMessage output - Auto-compaction trigger deletion ## What's different this time - Rebased onto latest main which includes fixes that were developed post-revert (model favorites, auto-compaction, etc.) - Conflict resolution preserves upstream features added after revert: - Workspace name collision retry with hash suffix - Mux Gateway coupon code handling with default models - AWS Bedrock credential nested structure ## Key Changes ### Architecture - New ORPC router (src/node/orpc/router.ts) - Central router with Zod schemas - Schema definitions (src/common/orpc/schemas.ts) - Shared validation - ServiceContainer (src/node/services/serviceContainer.ts) - DI container - React integration (src/browser/orpc/react.tsx) - ORPCProvider and useORPC() ### Transport - Desktop (Electron): MessagePort-based RPC via @orpc/server/message-port - Server mode: HTTP + WebSocket via @orpc/server/node and @orpc/server/ws - Auth middleware with timing-safe token comparison ### Removed - src/browser/api.ts (old HTTP/WS client) - src/node/services/ipcMain.ts (old IPC handler registration) - Old IPC method definitions in preload.ts --- _Generated with mux_
The useEffect cleanup captured state at mount time ('connecting'),
so even after transitioning to 'connected', cleanup never ran.
Now stores cleanup function in a ref that's always current.
Extend listBranches mock response with a feature branch example to better represent realistic branch listings during component development and testing.
The oRPC async generator was yielding messages one at a time with async boundaries, causing premature React renders during history replay. This led to scroll-to-bottom firing before all messages loaded. Extract createAsyncMessageQueue utility that yields all queued messages synchronously (no async pauses within a batch). Used by both the real router and storybook mocks to match original IPC callback behavior.
The MarkdownTables story was emitting messages without the required `type: "message"` field. The `as WorkspaceChatMessage` type casts hid this from TypeScript, causing messages to not be recognized by the chat processor and rendering "No Messages Yet" instead. Also fix ChatEventProcessor tests that had invalid event shapes: - Remove invalid `role`/`timestamp` from stream-start events - Add missing `workspaceId`/`tokens` to delta events
The scroll-to-bottom when workspace loads was using useEffect with requestAnimationFrame, which runs asynchronously after browser paint. Chromatic could capture the snapshot before scroll completed. Switch to useLayoutEffect which runs synchronously after DOM mutations but before paint, ensuring scroll happens before Chromatic captures. This also removes the need for the RAF wrapper since useLayoutEffect already guarantees DOM is ready.
- Add waitForChatScroll play function that waits for messages to load and scroll to complete before Chromatic takes screenshots - Add data-testid="message-window" to scroll container in AIView - Add data-testid="chat-message" to message wrappers for test queries - Apply play function to ActiveWorkspaceWithChat and MarkdownTables stories - Fix ProviderIcon lint error: remove redundant "mux-gateway" union type (already included in ProviderName)
- Add missing 'error' event handler in normalizeChatEvent.ts Converts error events to stream-error for mobile display - Fix result.metadata access in WorkspaceScreen.tsx ResultSchema wraps data, so access result.data.metadata not result.metadata - Add RequestInitWithDispatcher type in aiService.ts Extends RequestInit with undici-specific dispatcher property for Node.js
Replace setTimeout(..., 100) with queueMicrotask() for message delivery
in Storybook mocks. This ensures messages arrive synchronously (in the
microtask queue) rather than 100ms later, eliminating timing races with
Chromatic's screenshot capture.
- All onChat mock callbacks now use queueMicrotask for message delivery
- Add chromatic: { delay: 500 } as backup for ActiveWorkspaceWithChat
and MarkdownTables stories
- Fix import: use storybook/test instead of @storybook/test (Storybook 10)
In Storybook 10, interaction testing is built into the core and the addon-interactions package was deprecated. Remove it from the addons list to fix build failures caused by version mismatch.
Storybook 10 imports semver/functions/sort.js which only exists in semver 7.x. The root-level semver was 6.x (needed by babel), causing ESM resolution in Node 20.x to fail in CI. Adding semver ^7.x as a direct dependency forces the root-level version to 7.x, which is backwards compatible with 6.x consumers.
The ProviderConfigInfoSchema was missing couponCodeSet (for Mux Gateway) and aws (for Bedrock). oRPC strips fields not in the schema, so these values were never reaching the frontend UI. Also fixes workspace.fork to use FrontendWorkspaceMetadataSchema instead of WorkspaceMetadataSchema, ensuring namedWorkspacePath is not stripped.
Eliminates triple-definition of ProviderConfigInfo/AWSCredentialStatus types that existed in providerService.ts, Settings/types.ts, and the oRPC schema. Now the Zod schema is the single source of truth, with TypeScript types derived via z.infer. Adds conformance tests that validate oRPC schemas preserve all fields when parsing - this would have caught the missing couponCodeSet/aws fields bug.
1. Refactor MuxToolPartSchema using extend pattern for DRY code: - Base schema shares common fields (type, toolCallId, toolName, input, timestamp) - Pending variant: state="input-available", no output field - Available variant: state="output-available", output required 2. Remove dead ReasoningStartEventSchema: - Schema was defined but never emitted by backend - Not in WorkspaceChatMessageSchema union - No type guard existed for it 3. Update MuxToolPart type to use Zod inference: - Type now properly discriminates based on state - Accessing output requires narrowing to output-available state
Remove explicit DynamicToolPart cast in favor of TypeScript's built-in type narrowing after the discriminant check. The type guard `part.type !== "dynamic-tool"` already narrows the type, making the cast redundant. Also eliminate intermediate variables (toolPart, redacted) by returning the new object directly, reducing visual noise.
Capture event count before interrupt to establish baseline, then poll for new events rather than waiting for next event. The clear event may arrive before or simultaneously with stream-abort, causing the previous waitForQueuedMessageEvent call to miss it or timeout.
kylecarbs
requested changes
Dec 1, 2025
| return <ORPCContext.Provider value={state.client}>{props.children}</ORPCContext.Provider>; | ||
| }; | ||
|
|
||
| export const useORPC = (): RouterClient<AppRouter> => { |
Member
There was a problem hiding this comment.
To me, this should just be in hooks.
Maybe just useAPI? ORPC is an implementation detail and as a dev, I wouldn't go to this to access the client <-> server via it's current naming.
| } | ||
|
|
||
| // Show loading while connecting | ||
| if (state.status === "connecting") { |
Member
There was a problem hiding this comment.
I think it'd be better if we did:
const { api } = useAPI();
And had a set of fields like:
const { api, error, status } = useAPI();
So that other consumers can perform actions based on the state. We do not want to block the entire UI on a loading state, as this will be extremely difficult for us to rip out later on. It's much better if we have skeleton loaders for async actions imo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Reintroduces the ORPC refactoring originally merged in #763 and reverted in #777.
This replaces the custom IPC layer with oRPC for type-safe RPC between browser/renderer and backend processes.
Why it was reverted (#777)
The original migration caused regressions:
What's different this time
Key Changes
Architecture
src/node/orpc/router.ts) - Central router with Zod schemassrc/common/orpc/schemas.ts) - Shared validationsrc/node/services/serviceContainer.ts) - DI containersrc/browser/orpc/react.tsx) -ORPCProvideranduseORPC()Transport
@orpc/server/message-port@orpc/server/nodeand@orpc/server/wsRemoved
src/browser/api.ts(old HTTP/WS client)src/node/services/ipcMain.ts(old IPC handler registration)Test plan
make typecheck- passes locallymake test- verify existing tests passGenerated with mux