fix(devtools): rework timeline function wrapping to run after key injection - #1028
cernymatej wants to merge 8 commits into
Conversation
095b589 to
d10cb86
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe timeline integration now wraps eligible imports in a post-build Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change moves timeline wrapping into a post-transform and adds keyed-composable coverage. No concrete merge-blocking failure was established; mergeability remains subject to normal build and test checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change remains development-only and preserves configured import filtering. Recording is initially disabled, but enabling it can now capture arguments from additional explicitly imported functions. No introduced security vulnerability was established; build-order guarantees and broader deployment exposure remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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/vitest.config.ts (1)
1-7: 🧹 Nitpick | 🔵 TrivialWire the new test suite into CI before merge.
Per the PR summary, tests were added but aren't yet configured in CI. This config also runs the 60s-timeout e2e suite (
timeline.e2e.test.ts, which boots a real dev server) together with fast unit tests under the samevitest runinvocation — consider splitting them (e.g. separateincludeglobs/projects or a dedicatedtest:e2escript) once CI is wired up, so unit-test feedback isn't gated on booting a Nuxt dev server.🤖 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/vitest.config.ts` around lines 1 - 7, Wire the new Vitest suite into the repository’s CI workflow so it runs before merges. In the test configuration around defineConfig, separate the fast unit tests from timeline.e2e.test.ts into distinct globs, projects, or scripts, and ensure CI invokes the unit suite independently so it is not gated by the real dev-server e2e test.
🤖 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/integrations/timeline.ts`:
- Around line 87-104: Update the resolveId handling in the bySource processing
block to invoke ctx.resolveId(from) inside a promise-safe boundary so
synchronous throws become rejected resolutions handled as undefined. Ensure
resolvedFrom caches that safely handled promise and Promise.all cannot reject
from a resolver throw, preserving normal mapping behavior for successful
resolutions.
---
Nitpick comments:
In `@packages/devtools/vitest.config.ts`:
- Around line 1-7: Wire the new Vitest suite into the repository’s CI workflow
so it runs before merges. In the test configuration around defineConfig,
separate the fast unit tests from timeline.e2e.test.ts into distinct globs,
projects, or scripts, and ensure CI invokes the unit suite independently so it
is not gated by the real dev-server e2e test.
🪄 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: 04b140c7-e9b9-4e43-a2cc-3271b7c282b2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/devtools/package.jsonpackages/devtools/src/integrations/timeline-wrap.tspackages/devtools/src/integrations/timeline.tspackages/devtools/test/fixtures/timeline/app.vuepackages/devtools/test/fixtures/timeline/nuxt.config.tspackages/devtools/test/timeline-wrap.test.tspackages/devtools/test/timeline.e2e.test.tspackages/devtools/vitest.config.tsplaygrounds/tab-timeline/components/GlobalNav.vueplaygrounds/tab-timeline/pages/keyed.vuepnpm-workspace.yaml
…acros Drop the stale `keyedComposableFactories` cast (the schema types now declare it) and the unimport `resolveId` re-keying, which Nuxt never configures. Move the plugin definition next to the wrapper so its id and code filters are exercised through unplugin in unit tests instead of a re-implementation, and keep a single re-wrap guard. Remove the per-package vitest config, script and devDependency: the root config already runs `packages/**/test`. Playground: add a `createUseFetch` composable (nuxt/nuxt#34934) and assert it stays unwrapped while every keyed call still receives a key. Fix the `/api/data` route, which relied on Nitro auto-imports that no longer apply.
Removing the per-import-list cache and the `exclude` option leaves behaviour unchanged under the unit and e2e suites: recomputing the wrappable set is a filter over the import list, and the only exclusion ever passed was our own runtime dir, which the plugin now owns. Kept after ablation because the suites fail without them: registering from `modules:done`, `enforce: 'post'`, and the keyed-function-factory exclusion. Kept although unobservable here: the runtime-dir exclude (otherwise DevTools' own `useState`/`useRouter` calls land in the user's timeline) and the id/code filters mirroring Nuxt's compiler plugins.
…-wrapping # Conflicts: # pnpm-lock.yaml # pnpm-workspace.yaml
This branch has not been deployed
2 participants
🔗 Linked issue
fix #941
📚 Description
this aims to fix the issues with wrapping functions with
__nuxtTimelineWrapafter the changes in Nuxt that made function key injection stricter (and more accurate): nuxt/nuxt#33446the main issue was that we wrapped functions before the key injection plugin transformed the code... as a result, the plugin only saw local variable declarations instead of imports and bailed out without injecting function keys
another issue, reported in nuxt/nuxt#34934, was that it attempted to wrap new compiler macros, even though they're not meant to be called that way
this implementation fixes both of these issues and improves on the original implementation by supporting explicitly imported functions too, where the old one only wrapped auto-imported functions