fix(devtools): let birpc be singlesource of turth for server<->client comms#978
fix(devtools): let birpc be singlesource of turth for server<->client comms#978huang-julien wants to merge 3 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe diff removes the centralized Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/devtools/client/composables/client.ts (1)
63-73: MakeextendClientRpc()re-registration idempotent.
registerClientFunctions()inpackages/devtools/client/composables/rpc.tsalready usesupdate()with aregister()fallback. Mirroring that here would keep${namespace}:${name}safe across HMR/re-init instead of assumingregister()replaces an existing handler.♻️ Proposed change
const register = (client: DevToolsRpcClient) => { for (const [name, handler] of Object.entries(functions)) { if (typeof handler === 'function') { - client.client.register({ - name: `${namespace}:${name}`, - type: 'event', - handler: handler as any, - }) + try { + client.client.update({ + name: `${namespace}:${name}`, + type: 'event', + handler: handler as any, + }) + } + catch { + client.client.register({ + name: `${namespace}:${name}`, + type: 'event', + handler: handler as any, + }) + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/client/composables/client.ts` around lines 63 - 73, The current register loop in the register function unconditionally calls client.client.register for each `${namespace}:${name}`, which can clash on HMR/re-init; make re-registration idempotent by attempting client.client.update({ name: `${namespace}:${name}`, type: 'event', handler }) first and only calling client.client.register(...) as a fallback if update fails/not available. Update the register function (and extendClientRpc usage if present) to use update() with register() fallback for each entry in functions to keep handlers safe across re-inits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/devtools/client/composables/client.ts`:
- Around line 75-78: The branch that calls connectPromise.then(register) drops
the returned promise and can cause an unhandled rejection if the RPC connection
fails; update the code around rpcClient/register/connectPromise to consume
rejections (for example by chaining .catch and forwarding the error to the same
wsError/console handler or invoking the existing error handler) so the promise
returned by connectPromise.then(register) is handled and its errors are
processed consistently with the other failure path.
---
Nitpick comments:
In `@packages/devtools/client/composables/client.ts`:
- Around line 63-73: The current register loop in the register function
unconditionally calls client.client.register for each `${namespace}:${name}`,
which can clash on HMR/re-init; make re-registration idempotent by attempting
client.client.update({ name: `${namespace}:${name}`, type: 'event', handler })
first and only calling client.client.register(...) as a fallback if update
fails/not available. Update the register function (and extendClientRpc usage if
present) to use update() with register() fallback for each entry in functions to
keep handlers safe across re-inits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da97b456-c8bd-437c-9f81-ec0f4995d125
📒 Files selected for processing (2)
packages/devtools/client/composables/client.tspackages/devtools/client/composables/rpc.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Deploying nuxt-devtools with
|
| Latest commit: |
1f87b76
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ddc3b255.nuxt-devtools.pages.dev |
| Branch Preview URL: | https://fix-extendsrpc.nuxt-devtools.pages.dev |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/devtools/client/composables/client.ts (1)
63-78: Consider applying the update/register fallback pattern toextendClientRpc.The
registerhelper uses onlyclient.client.register()without the update/register fallback pattern used inregisterClientFunctions(seepackages/devtools/client/composables/rpc.tslines 67-85). SinceextendClientRpcis a public API that external consumers can call, they may inadvertently call it multiple times with the same namespace and function name. To be defensive, apply the same pattern:Suggested change
for (const [name, handler] of Object.entries(functions)) { if (typeof handler === 'function') { - client.client.register({ - name: `${namespace}:${name}`, - type: 'event', - handler: handler as any, - }) + try { + client.client.update({ + name: `${namespace}:${name}`, + type: 'event', + handler: handler as any, + }) + } + catch { + client.client.register({ + name: `${namespace}:${name}`, + type: 'event', + handler: handler as any, + }) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/devtools/client/composables/client.ts` around lines 63 - 78, The current register helper in extendClientRpc calls only client.client.register(...) which can fail or duplicate when consumers call extendClientRpc multiple times; mirror the update/register fallback used in registerClientFunctions by attempting client.client.update({...}) first and, if that fails (catch), call client.client.register({...}) as a fallback. Update the register function (the one defined inside extendClientRpc) to use client.client.update and fallback to client.client.register for each { name: `${namespace}:${name}`, type: 'event', handler } to make the API idempotent and defensive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/devtools/client/composables/client.ts`:
- Around line 63-78: The current register helper in extendClientRpc calls only
client.client.register(...) which can fail or duplicate when consumers call
extendClientRpc multiple times; mirror the update/register fallback used in
registerClientFunctions by attempting client.client.update({...}) first and, if
that fails (catch), call client.client.register({...}) as a fallback. Update the
register function (the one defined inside extendClientRpc) to use
client.client.update and fallback to client.client.register for each { name:
`${namespace}:${name}`, type: 'event', handler } to make the API idempotent and
defensive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b458595f-d13c-4e07-bd8a-586610fac1e3
📒 Files selected for processing (1)
packages/devtools/client/composables/client.ts
1 participant
… comms
🔗 Linked issue
📚 Description
Found bug when testing out nuxt/hints#318 with Vite 8 + devtools 4.0.0-alpha.3
This solves a race condiftion and remove
extendedRpcMapto make the code a bit simpler. ifextendClientRpcis called whileconnectDevToolsRpc(is mid-flight (after it already iteratedextendedRpcMapbut before it setsrpcClient), neither path registers the handlers.Need also #977 to fix the issue with RPC in nuxt hints.