Conversation
commit: |
huang-julien
force-pushed
the
refactor/migrateRPC
branch
from
2acc3b3 to
868aca2
Compare
📝 WalkthroughWalkthroughReplaces SSE-based hint delivery with a devtools RPC approach. Adds client RPC plugin (registers hints namespace and forwards RPC callbacks to nuxtApp hooks), introduces src/rpc-types.ts for HintsClientRpc/HintsServerRpc, and adds a server rpc-bridge that broadcasts Nitro hook payloads via a global RPC group. Removes SSE server handler, SSE client plugin, SSE-related types/routes, and associated tests. Updates client plugins (hydration, lazy-load, html-validate) to consume nuxtApp.hook('hints:rpc:*') events instead of parsing SSE MessageEvent data. Module setup now registers the RPC server plugin when devtools are enabled. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/app/plugins/html-validate.ts (1)
9-12:⚠️ Potential issue | 🟠 MajorDon't let the bootstrap fetch overwrite live RPC updates.
htmlValidateReportsis still owned byuseLazyFetch, so a report/delete received here before the first response resolves can be replaced by that response and disappear from the panel. Please switch this file to the same standaloneref+ merge-bootstrap pattern used inclient/app/plugins/hydration.ts.Possible fix
-import { defineNuxtPlugin } from '#imports' +import { defineNuxtPlugin, ref } from '#imports' @@ - const { data: htmlValidateReports } = useLazyFetch<HtmlValidateReport[]>(new URL(HTML_VALIDATE_ROUTE, window.location.origin).href, { - default: () => [], - deep: true, - }) + const htmlValidateReports = ref<HtmlValidateReport[]>([]) + $fetch<HtmlValidateReport[]>(new URL(HTML_VALIDATE_ROUTE, window.location.origin).href).then((reports) => { + htmlValidateReports.value = [ + ...htmlValidateReports.value, + ...reports.filter(report => !htmlValidateReports.value.some(existing => existing.id === report.id)), + ] + })Also applies to: 14-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/plugins/html-validate.ts` around lines 9 - 12, The current useLazyFetch-owned data (htmlValidateReports) can be overwritten by the bootstrap response; change to a standalone ref and implement the merge-bootstrap pattern used in hydration.ts: create a ref (e.g., htmlValidateReportsRef) to hold the live reports, call useLazyFetch only to get the initial bootstrap payload, and on its resolution merge its items into htmlValidateReportsRef without replacing existing entries; keep RPC handlers (report/delete) updating htmlValidateReportsRef directly so live updates are preserved when the bootstrap resolves.src/module.ts (1)
1-1:⚠️ Potential issue | 🟠 MajorRemove the stale
addServerHandlerimport.This migration replaces the old SSE handler path, so
addServerHandleris now unused at Line 1. CI is already failing on@typescript-eslint/no-unused-vars, so the PR will not pass lint until that import is dropped.Also applies to: 105-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.ts` at line 1, Remove the unused named import addServerHandler from the Nuxt kit import lines: edit the import statement that currently lists defineNuxtModule, addPlugin, createResolver, addBuildPlugin, addComponent, addServerPlugin, addServerHandler, addTemplate and delete addServerHandler; also remove addServerHandler from any other import lists (the other occurrence around the later import block) so no unused addServerHandler symbol remains.
🧹 Nitpick comments (2)
client/app/plugins/0.rpc.ts (1)
4-5: Extract the RPC literals into a shared contract module.The namespace is duplicated here and in
src/devtools.ts, and these hook names now have to stay in sync with the consumer plugins. Please move them into a tiny side-effect-free shared module next tosrc/rpc-types.tsinstead of importingsrc/devtools.tsinto the iframe bundle.Also applies to: 13-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/plugins/0.rpc.ts` around lines 4 - 5, Move the duplicated RPC string literals (e.g., RPC_NAMESPACE = 'hints' and the hook names used across files) into a new tiny, side-effect-free shared module (e.g., next to src/rpc-types.ts) that exports named constants; then update client/app/plugins/0.rpc.ts and src/devtools.ts to import those constants instead of hardcoding the literals or importing src/devtools.ts into the iframe bundle. Make sure the shared module only exports constants (no runtime side effects), preserves the exact symbol names used by consumers, and replace all literal occurrences (RPC_NAMESPACE and the hook name constants referenced in lines ~13-29) with imports from the new module.src/runtime/core/server/rpc-bridge.ts (1)
5-7: Race condition concern is theoretical; hooks fire during request handling after devtools initialization.The code registers hook handlers that will fire during request handling (POST/DELETE routes and response hooks), not during server startup. By the time clients make HTTP requests, the devtools initialization via
onDevToolsInitialized()will have already completed. The optional chaining (getRpc()?.broadcast...) gracefully handles the undefined case without runtime errors.If this is intentional by design (devtools as a debug tool), the current implementation is appropriate. If documentation about this expectation would be helpful, consider adding a comment noting that hooks rely on devtools initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/core/server/rpc-bridge.ts` around lines 5 - 7, The race-condition concern is theoretical; document the intended behavior by adding a clear comment near getRpc() (and/or where getRpc()?.broadcast is called) stating that hooks (POST/DELETE routes and response hooks) execute during request handling after onDevToolsInitialized() has completed, and that getRpc() may be undefined until devtools init but optional chaining intentionally prevents runtime errors; reference the getRpc function and onDevToolsInitialized to make the expectation explicit for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/app/plugins/html-validate.ts`:
- Around line 9-12: The current useLazyFetch-owned data (htmlValidateReports)
can be overwritten by the bootstrap response; change to a standalone ref and
implement the merge-bootstrap pattern used in hydration.ts: create a ref (e.g.,
htmlValidateReportsRef) to hold the live reports, call useLazyFetch only to get
the initial bootstrap payload, and on its resolution merge its items into
htmlValidateReportsRef without replacing existing entries; keep RPC handlers
(report/delete) updating htmlValidateReportsRef directly so live updates are
preserved when the bootstrap resolves.
In `@src/module.ts`:
- Line 1: Remove the unused named import addServerHandler from the Nuxt kit
import lines: edit the import statement that currently lists defineNuxtModule,
addPlugin, createResolver, addBuildPlugin, addComponent, addServerPlugin,
addServerHandler, addTemplate and delete addServerHandler; also remove
addServerHandler from any other import lists (the other occurrence around the
later import block) so no unused addServerHandler symbol remains.
---
Nitpick comments:
In `@client/app/plugins/0.rpc.ts`:
- Around line 4-5: Move the duplicated RPC string literals (e.g., RPC_NAMESPACE
= 'hints' and the hook names used across files) into a new tiny,
side-effect-free shared module (e.g., next to src/rpc-types.ts) that exports
named constants; then update client/app/plugins/0.rpc.ts and src/devtools.ts to
import those constants instead of hardcoding the literals or importing
src/devtools.ts into the iframe bundle. Make sure the shared module only exports
constants (no runtime side effects), preserves the exact symbol names used by
consumers, and replace all literal occurrences (RPC_NAMESPACE and the hook name
constants referenced in lines ~13-29) with imports from the new module.
In `@src/runtime/core/server/rpc-bridge.ts`:
- Around line 5-7: The race-condition concern is theoretical; document the
intended behavior by adding a clear comment near getRpc() (and/or where
getRpc()?.broadcast is called) stating that hooks (POST/DELETE routes and
response hooks) execute during request handling after onDevToolsInitialized()
has completed, and that getRpc() may be undefined until devtools init but
optional chaining intentionally prevents runtime errors; reference the getRpc
function and onDevToolsInitialized to make the expectation explicit for future
readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a06e005c-ec1b-425b-9ad1-33d4c44846d2
📒 Files selected for processing (18)
client/app/plugins/0.rpc.tsclient/app/plugins/0.sse.tsclient/app/plugins/html-validate.tsclient/app/plugins/hydration.tsclient/app/plugins/lazy-load.tsclient/app/utils/routes.tssrc/devtools.tssrc/module.tssrc/rpc-types.tssrc/runtime/core/server/rpc-bridge.tssrc/runtime/core/server/sse.tssrc/runtime/core/server/types.tssrc/runtime/html-validate/handlers/nitro-handlers.plugin.tssrc/runtime/hydration/nitro.plugin.tssrc/runtime/hydration/utils.tssrc/runtime/lazy-load/nitro.plugin.tssrc/runtime/types.d.tstest/unit/core/sse.test.ts
💤 Files with no reviewable changes (8)
- src/runtime/types.d.ts
- client/app/utils/routes.ts
- client/app/plugins/0.sse.ts
- src/runtime/lazy-load/nitro.plugin.ts
- src/runtime/core/server/sse.ts
- src/runtime/html-validate/handlers/nitro-handlers.plugin.ts
- src/runtime/hydration/nitro.plugin.ts
- test/unit/core/sse.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/devtools.ts (1)
8-8: Avoid duplicated RPC namespace literals across client/server wiring.
RPC_NAMESPACEis hardcoded here and also inclient/app/plugins/0.rpc.ts; this can silently break channel alignment during future renames. Centralize it in one shared export (e.g.,src/rpc-types.ts) and import from both sides.♻️ Proposed refactor (server side)
-import type { HintsClientRpc, HintsServerRpc } from './rpc-types' +import { RPC_NAMESPACE, type HintsClientRpc, type HintsServerRpc } from './rpc-types' -export const RPC_NAMESPACE = 'hints'Also apply the same shared import in
client/app/plugins/0.rpc.tsto remove the second literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/devtools.ts` at line 8, RPC_NAMESPACE is duplicated as a hardcoded literal; create a single shared export (e.g., export const RPC_NAMESPACE = 'hints' in a new module named rpc-types) and update both places that currently hardcode the string to import RPC_NAMESPACE from that module. Replace the literal in the existing export of RPC_NAMESPACE with an import from the new shared module and update the client plugin file that contains the second literal (the 0.rpc plugin) to import the same RPC_NAMESPACE instead of redefining it, ensuring both sides use the single shared symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/devtools.ts`:
- Line 8: RPC_NAMESPACE is duplicated as a hardcoded literal; create a single
shared export (e.g., export const RPC_NAMESPACE = 'hints' in a new module named
rpc-types) and update both places that currently hardcode the string to import
RPC_NAMESPACE from that module. Replace the literal in the existing export of
RPC_NAMESPACE with an import from the new shared module and update the client
plugin file that contains the second literal (the 0.rpc plugin) to import the
same RPC_NAMESPACE instead of redefining it, ensuring both sides use the single
shared symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f74034e9-f611-4342-9dbf-22d4205ae311
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
client/app/plugins/0.rpc.tsclient/app/plugins/0.sse.tsclient/app/plugins/html-validate.tsclient/app/plugins/hydration.tsclient/app/plugins/lazy-load.tsclient/app/utils/routes.tssrc/devtools.tssrc/module.tssrc/rpc-types.tssrc/runtime/core/server/rpc-bridge.tssrc/runtime/core/server/sse.tssrc/runtime/core/server/types.tssrc/runtime/html-validate/handlers/nitro-handlers.plugin.tssrc/runtime/hydration/nitro.plugin.tssrc/runtime/hydration/utils.tssrc/runtime/lazy-load/nitro.plugin.tssrc/runtime/types.d.tstest/unit/core/sse.test.ts
💤 Files with no reviewable changes (7)
- client/app/utils/routes.ts
- src/runtime/html-validate/handlers/nitro-handlers.plugin.ts
- src/runtime/core/server/sse.ts
- client/app/plugins/0.sse.ts
- test/unit/core/sse.test.ts
- src/runtime/lazy-load/nitro.plugin.ts
- src/runtime/hydration/nitro.plugin.ts
✅ Files skipped from review due to trivial changes (2)
- client/app/plugins/0.rpc.ts
- client/app/plugins/hydration.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/module.ts
- src/runtime/hydration/utils.ts
- client/app/plugins/lazy-load.ts
- src/runtime/core/server/types.ts
- src/runtime/core/server/rpc-bridge.ts
- client/app/plugins/html-validate.ts
- src/rpc-types.ts
1 participant
This PR moves away from SSE to use devtools RPC.