fix(devtools): correct options cache guard and stop mutating defaults - #1018
Conversation
getOptions(tab) had an inverted cache guard (re-reading from disk on every call once a tab was populated) and aliased the shared defaultTabOptions constant, letting per-project settings permanently overwrite it. Clone the defaults on first use and only read a tab from disk once, tracked via a Set. Removes plans/004-fix-options-rpc-cache.md now that it's implemented.
📝 WalkthroughWalkthroughThe options system now creates fresh default option graphs through a factory, supports configurable server-task defaults, and updates module and client consumers. The options RPC tracks per-tab reads with a Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/devtools/client/composables/storage-options.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/devtools/src/constant.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/devtools/src/module-main.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/devtools/src/server-rpc/options.ts`:
- Around line 23-28: Update the read function to pass the already-deeply-cloned
options![tab] value as the defaults argument to readLocalOptions instead of
defaultTabOptions[tab]. Preserve the existing root, key, assignment, and
hasReadOnce behavior while ensuring nested option data remains isolated from the
shared defaults.
In `@packages/devtools/test/default-tab-options.test.ts`:
- Around line 1-13: Rewrite the test around setupOptionsRPC so it invokes the
actual options RPC method rather than directly calling structuredClone on
defaultTabOptions. Obtain the returned options, mutate a nested reference such
as an array, and assert that defaultTabOptions remains unchanged; replace the
scale mutation and update the test setup to use the RPC’s required inputs and
dependencies.
🪄 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: b27707b1-c132-497d-bc39-19e2f81f480b
📒 Files selected for processing (4)
packages/devtools/src/server-rpc/options.tspackages/devtools/test/default-tab-options.test.tsplans/004-fix-options-rpc-cache.mdplans/README.md
💤 Files with no reviewable changes (1)
- plans/004-fix-options-rpc-cache.md
Passing defaultTabOptions[tab] into readLocalOptions defeated the structuredClone in getOptions, since readLocalOptions only shallow-copies its defaults argument — nested objects/arrays (e.g. ui.hiddenTabs) stayed aliased to the shared constant. Source the defaults from the already-cloned options![tab] instead. Also rewrite the regression test to exercise setupOptionsRPC directly and assert isolation on a nested array, per review feedback.
…ions() factory Replace the shared defaultTabOptions object with a factory function that returns a brand-new object graph on every call, so no caller can alias or leak mutations into shared state — the underlying class of bug fixed earlier in this branch by cloning. This also fixes a latent regression from that clone-based fix: the module mutated defaultTabOptions.serverTasks.enabled directly from a nitro:config hook to auto-enable server tasks when Nitro's experimental tasks feature is on, but once the options cache stopped aliasing the shared object, that mutation silently stopped propagating. serverTasks.enabled is now tracked via an explicit setServerTasksEnabledByDefault() flag that the factory reads fresh on every call (including inside options.ts's read(), so the first real read of a tab always sees the latest dynamic defaults, not a frozen initial snapshot). Updates all call sites (options RPC, module-main settings template, client storage-options composable) and the regression test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/devtools/src/module-main.ts`:
- Around line 125-129: Update the nitro:config hook to assign
serverTasksEnabledByDefault from the current config.experimental?.tasks value on
every pass, rather than only calling setServerTasksEnabledByDefault(true) when
enabled. Ensure later Nuxt instances reset the process-wide default when tasks
are not opted in.
🪄 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: bf6072a1-eb89-4f67-9d0b-21b5971f53ff
📒 Files selected for processing (5)
packages/devtools/client/composables/storage-options.tspackages/devtools/src/constant.tspackages/devtools/src/module-main.tspackages/devtools/src/server-rpc/options.tspackages/devtools/test/default-tab-options.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/devtools/src/server-rpc/options.ts
| nuxt.hook('nitro:config', (config) => { | ||
| // Check user opted-in for tasks | ||
| if (config.experimental?.tasks) | ||
| defaultTabOptions.serverTasks.enabled = true | ||
| setServerTasksEnabledByDefault(true) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== module-main outline ==\n'
ast-grep outline packages/devtools/src/module-main.ts --view expanded || true
printf '\n== constant outline ==\n'
ast-grep outline packages/devtools/src/constant.ts --view expanded || true
printf '\n== search setter/getter usages ==\n'
rg -n "setServerTasksEnabledByDefault|serverTasksEnabledByDefault" packages/devtools/src packages/devtools/test packages/devtools -g '!**/dist/**' || true
printf '\n== relevant slice of module-main.ts ==\n'
sed -n '100,155p' packages/devtools/src/module-main.ts
printf '\n== relevant slice of constant.ts ==\n'
sed -n '1,220p' packages/devtools/src/constant.tsRepository: nuxt/devtools
Length of output: 7760
Reset serverTasksEnabledByDefault on every nitro:config pass.
This is process-wide module state, but the hook only ever sets it to true. A later Nuxt instance in the same process can inherit true from an earlier one and default serverTasks.enabled incorrectly. Set it from the current config instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/devtools/src/module-main.ts` around lines 125 - 129, Update the
nitro:config hook to assign serverTasksEnabledByDefault from the current
config.experimental?.tasks value on every pass, rather than only calling
setServerTasksEnabledByDefault(true) when enabled. Ensure later Nuxt instances
reset the process-wide default when tasks are not opted in.
2 participants
Summary
getOptions(tab)in the options RPC had two bugs:if (!options || options[tab])re-ran the disk read (existsSync+readFile+JSON.parse) on every call once a tab was already populated, instead of only when it wasn't. The in-memory cache was effectively never used, and every settings read/write paid the disk-read cost.options = defaultTabOptionsaliased the imported module-level constant rather than cloning it, soread()mutateddefaultTabOptionsin place. Per-project user settings then permanently overwrote the process-global defaults, andclearOptions()could no longer restore pristine values.Changes
defaultTabOptionsviastructuredCloneon first use instead of aliasing it.Setso each tab is loaded from disk at most once, fixing the inverted guard without breaking tabs whose persisted value is falsy.packages/devtools/test/default-tab-options.test.tsto pin the "clone before mutate" contract ondefaultTabOptions.Verification
pnpm test:unit— exit 0, all tests pass (including new test).pnpm lint— exit 0.vue-tscerrors remain in other files).packages/devtools/src/server-rpc/options.tsand the new test file were modified;packages/devtools/src/constant.tsuntouched.This PR was created with the help of an agent.