fix(client): prevent unintended procedure calls when awaiting client - #887
Conversation
Previously, awaiting the client directly (e.g., `await client`) would trigger a procedure call with the name "rpc", causing unexpected behavior. This fix ensures that awaiting the client itself does nothing, while maintaining the ability to await actual procedure calls like `await client.myProcedure()`. The solution uses `preventNativeAwait` to disable the default Promise-like behavior of the client proxy when used with the `await` keyword.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses an issue where directly awaiting the client object (e.g., await client) in the ORPC client library would inadvertently trigger an RPC procedure call named "rpc". The fix ensures that awaiting the client itself performs no action, while preserving the intended functionality of awaiting specific procedure calls like await client.myProcedure(). This is achieved by introducing and utilizing a new utility function, preventNativeAwait, which disables the default Promise-like behavior of the client proxy when used with the await keyword.
Highlights
- Resolved Unintended Behavior: Fixed an issue where await client would erroneously initiate an RPC call, ensuring it now correctly performs no operation.
- Introduced preventNativeAwait Utility: A new utility function has been added to the @orpc/shared package, designed to prevent objects from being treated as Promises by the native await mechanism.
- Client Proxy Enhancement: The ORPC client's internal proxy now utilizes preventNativeAwait to control its awaitable behavior, allowing explicit procedure calls to be awaited while the client object itself is not.
- Comprehensive Testing: New test cases have been added in both packages/client and packages/shared to validate the correct behavior of the preventNativeAwait utility and the client's updated await semantics.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where awaiting an ORPC client directly (e.g., await client) would unintentionally trigger a procedure call with the name "rpc". The fix introduces a preventNativeAwait utility that intercepts the Promise-like behavior of the client proxy when used with the await keyword, while preserving the ability to await actual procedure calls.
- Adds a
preventNativeAwaitutility function that prevents objects from being awaitable by intercepting nativeawaitmechanism - Applies the utility to the ORPC client to prevent unintended procedure calls when awaiting the client directly
- Includes comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/proxy.ts | Implements the preventNativeAwait utility function that intercepts Promise-like behavior |
| packages/shared/src/proxy.test.ts | Comprehensive test suite for the preventNativeAwait function |
| packages/shared/src/index.ts | Exports the new proxy module |
| packages/client/src/client.ts | Applies preventNativeAwait to the ORPC client |
| packages/client/src/client.test.ts | Adds test to verify the fix prevents unintended procedure calls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| */ | ||
| if ( | ||
| args.length !== 2 | ||
| || args.some(arg => typeof arg !== 'function' || !NATIVE_FUNCTION_REGEX.test(arg.toString())) |
There was a problem hiding this comment.
[nitpick] The nested condition combining type checking and regex testing in a single line reduces readability. Consider breaking this into separate conditions or extracting the logic into a helper function.
There was a problem hiding this comment.
Code Review
This pull request introduces a clever fix to prevent the oRPC client from being accidentally awaited, which previously caused unintended procedure calls. The solution uses a preventNativeAwait utility that wraps the client in a Proxy to intercept calls to .then() made by the await keyword. The implementation is well-contained in the @orpc/shared package and is accompanied by a comprehensive set of unit tests. My main feedback is regarding the technique used to detect native await calls, which relies on Function.prototype.toString() and could be fragile. Overall, this is a solid improvement that addresses a potentially confusing behavior for users of the client.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-event-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/react
@orpc/react-query
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/shared/src/proxy.ts (2)
1-1: Harden await-detection: current native-function regex is brittle across enginesRelying on an exact
function () { [native code] }shape is fragile and may vary across Node/browser engines or toolchains. A more permissive check that only asserts the presence of “[native code]” is safer and still specific enough to distinguish user callbacks.Proposed change:
-const NATIVE_FUNCTION_REGEX = /^\s*function\s*\(\)\s*\{\s*\[native code\]\s*\}\s*$/ +const isNativeFunction = (fn: unknown): fn is Function => + typeof fn === 'function' && /\{\s*\[native code\]\s*\}/.test(Function.prototype.toString.call(fn))And update the guard:
- if ( - args.length !== 2 - || args.some(arg => typeof arg !== 'function' || !NATIVE_FUNCTION_REGEX.test(arg.toString())) - ) { + if (args.length !== 2 || args.some(arg => !isNativeFunction(arg))) { return Reflect.apply(targetFn, thisArg, args) }Optional: run the test suite across multiple Node versions (LTS/min/max you support) or a headless browser runner to ensure detection remains solid.
Also applies to: 22-26
29-43: Nit: return explicitly from the apply trap in the native-await branchWhile the return value of
thenis ignored by the native await path, an explicitreturnkeeps semantics clear and avoids accidental leaks if this code evolves.let shouldOmit = true - args[0].call(thisArg, preventNativeAwait(new Proxy(target, { + args[0].call(thisArg, preventNativeAwait(new Proxy(target, { get: (target, prop, receiver) => { /** * Only omit `then` once, in `await` resolution, afterwards it should become normal */ if (shouldOmit && prop === 'then') { shouldOmit = false return undefined } return Reflect.get(target, prop, receiver) }, }))) + returnpackages/client/src/client.ts (1)
42-42: Be aware of identity change; consider tightening the castWrapping
recursivewithpreventNativeAwaitchanges the object identity returned to consumers. This is likely fine, but if any downstream relies on identity orinstanceofchecks, it could be observable. Theas anycast remains from prior code; if feasible, narrow toas unknown as Tto preserve type intent.- return preventNativeAwait(recursive) as any + return preventNativeAwait(recursive) as unknown as TIf you have any known consumers performing identity checks, please verify no regressions.
packages/shared/src/proxy.test.ts (2)
9-11: Silence Biome’s noThenProperty rule in tests where it’s intentionalThe linter warnings are correct in application code, but here you intentionally create objects with
thento simulate thenables. Either disable the rule for tests via Biome overrides or add inline ignores above the offending lines.Option A: Inline ignores (minimal diff, precise scope)
- const obj2 = { then: 323, catch: 123 } + /* biome-ignore lint/suspicious/noThenProperty: intentional in tests */ + const obj2 = { then: 323, catch: 123 } - const obj3 = { then: (...args: any[]) => ({ args }), catch: (...args: any[]) => ({ args }) } + /* biome-ignore lint/suspicious/noThenProperty: intentional in tests */ + const obj3 = { then: (...args: any[]) => ({ args }), catch: (...args: any[]) => ({ args }) }Repeat the inline ignore above each object literal that defines a
thenproperty (lines 24, 29, 44, 51, 70, 77).Option B: Biome config override (cleaner if many tests rely on thenables)
Add an override to your Biome config (biome.json):
{ "overrides": [ { "files": ["**/*.test.{ts,tsx,js,jsx}"], "linter": { "rules": { "suspicious/noThenProperty": "off" } } } ] }Also applies to: 13-17, 24-28, 29-34, 44-50, 51-56, 70-76, 77-86
19-34: Consider adding coverage for Promise.resolve assimilationNative
awaitis covered; optionally add tests ensuringPromise.resolve(preventNativeAwait(obj))also returns the original object (since engines also use thenable assimilation here). This guards against regressions if the detection logic changes.Example test additions:
it('resolves via Promise.resolve without triggering thenable assimilation', async () => { const obj = { value: 42 } const proxy = preventNativeAwait(obj) await expect(Promise.resolve(proxy)).resolves.toEqual(obj) /* biome-ignore lint/suspicious/noThenProperty: intentional in tests */ const obj2 = { then: 123 } const proxy2 = preventNativeAwait(obj2) await expect(Promise.resolve(proxy2)).resolves.toEqual(obj2) /* biome-ignore lint/suspicious/noThenProperty: intentional in tests */ const obj3 = { then: (...args: any[]) => ({ args }) } const proxy3 = preventNativeAwait(obj3) const resolved = await Promise.resolve(proxy3 as any) expect(resolved.then(1, 2, 3)).toEqual({ args: [1, 2, 3] }) })Also applies to: 36-60, 62-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/client/src/client.test.ts(1 hunks)packages/client/src/client.ts(2 hunks)packages/shared/src/index.ts(1 hunks)packages/shared/src/proxy.test.ts(1 hunks)packages/shared/src/proxy.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/client/src/client.test.ts (1)
packages/client/src/client.ts (1)
createORPCClient(17-43)
packages/client/src/client.ts (1)
packages/shared/src/proxy.ts (1)
preventNativeAwait(8-47)
packages/shared/src/proxy.test.ts (1)
packages/shared/src/proxy.ts (1)
preventNativeAwait(8-47)
🪛 Biome (2.1.2)
packages/shared/src/proxy.test.ts
[error] 9-9: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 13-13: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 24-24: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 44-44: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 51-51: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 70-70: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 77-77: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: publish-commit
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/shared/src/proxy.ts (1)
17-45: Behavior is well-isolated to native-await; non-await.then(...)calls still flow throughGood separation of concerns: native-await invocations are intercepted while intentional
client.then(...)calls are forwarded. The single-omit strategy forthenavoids assimilation loops elegantly.packages/shared/src/index.ts (1)
15-15: Re-export looks goodPublicly exposing
preventNativeAwaitvia the shared index aligns with the intended consumption from the client package.packages/client/src/client.ts (1)
2-2: Correct integration pointImporting
preventNativeAwaitfrom@orpc/sharedis consistent with the new re-export and keeps client decoupled from internal file paths.packages/client/src/client.test.ts (1)
49-63: Great coverage of the native-await path and chained tokensThis test effectively validates that awaiting the client and then accessing path tokens like
then/somethingcontinue to route into the link with the correct path accumulation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/shared/src/proxy.test.ts (2)
19-34: Clarify identity vs. equality in “returns itself if awaited”Awaiting resolves to an object equal in shape but not necessarily the same reference (it’s typically a proxy). Consider asserting non-identity for clarity.
Apply this diff to make the intent explicit:
const proxy = preventNativeAwait(obj) - expect(await proxy).toEqual(obj) + expect(await proxy).toEqual(obj) + expect(await proxy).not.toBe(obj) @@ const proxy2 = preventNativeAwait(obj2) - expect(await proxy2).toEqual(obj2) - expect(await proxy2).toEqual(obj2) + expect(await proxy2).toEqual(obj2) + expect(await proxy2).toEqual(obj2) + expect(await proxy2).not.toBe(obj2)
41-41: Reduce flakiness: prefer microtasks over timersUsing setTimeout(1) ties tests to macrotask timing and can flake under load. If you only need the event loop to tick, use a microtask.
Apply this diff:
- await new Promise(resolve => setTimeout(resolve, 1)) + await Promise.resolve() @@ - await new Promise(resolve => setTimeout(resolve, 1)) + await Promise.resolve() @@ - await new Promise(resolve => setTimeout(resolve, 1)) + await Promise.resolve() @@ - await new Promise(resolve => setTimeout(resolve, 1)) + await Promise.resolve() @@ - await new Promise(resolve => setTimeout(resolve, 1)) + await Promise.resolve() @@ - await new Promise(resolve => setTimeout(resolve, 1)) + await Promise.resolve()Also applies to: 48-48, 56-56, 67-67, 74-74, 82-82
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/shared/src/proxy.test.ts(1 hunks)packages/shared/src/proxy.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/proxy.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/shared/src/proxy.test.ts (1)
packages/shared/src/proxy.ts (1)
preventNativeAwait(8-44)
🪛 Biome (2.1.2)
packages/shared/src/proxy.test.ts
[error] 9-9: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 13-13: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 24-24: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 44-44: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 51-51: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 70-70: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 77-77: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 93-93: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 97-97: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (1)
packages/shared/src/proxy.test.ts (1)
1-3: LGTM: tests align with PR goalThe suite thoroughly verifies that awaiting a proxy does not trigger thenable assimilation beyond the first internal resolution and preserves normal behavior for property access and methods. This directly supports preventing unintended procedure calls when awaiting the client object.
Previously, awaiting the client directly (e.g.,
await client) would trigger a procedure call with the name "rpc", causing unexpected behavior.This fix ensures that awaiting the client itself does nothing, while maintaining the ability to await actual procedure calls like
await client.myProcedure().The solution uses
preventNativeAwaitto disable the default Promise-like behavior of the client proxy when used with theawaitkeyword.Summary by CodeRabbit
New Features
Bug Fixes
Tests