refactor(server): use nitropack/runtime in proxy handlers - #707
Merged
Conversation
… handlers Switches server-side proxy handlers from the Nuxt `#imports` virtual module to direct `nitropack/runtime` imports. This lets unit tests import the handler files (and their colocated utility exports like `rewriteUrl`, `scopeCss`) without pulling Nuxt's virtual module graph. Adds a vitest alias to a lightweight `nitropack/runtime` mock for the unit project.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
📝 WalkthroughWalkthroughThis PR extracts Instagram embed utilities into a new module Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vitest.config.ts`:
- Around line 24-27: The alias currently uses new URL(...).pathname which yields
a POSIX-style path on Windows and breaks resolution; replace the pathname usage
by importing and calling fileURLToPath on the same URL and use that result for
the alias value (update the resolve.alias entry where 'nitropack/runtime' is
mapped and the URL is constructed to call fileURLToPath(new URL(...)) instead of
accessing .pathname).
🪄 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: e142d1a3-bab3-40b6-b566-fac11c861bf1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/script/src/runtime/server/google-maps-geocode-proxy.tspackages/script/src/runtime/server/google-static-maps-proxy.tspackages/script/src/runtime/server/gravatar-proxy.tspackages/script/src/runtime/server/proxy-handler.tspackages/script/src/runtime/server/utils/withSigning.tspnpm-workspace.yamltest/unit/__mocks__/nitropack-runtime.tsvitest.config.ts
…e handler Drop the nitropack/runtime vitest alias + stub by moving the pure URL rewriting + CSS scoping helpers out of the handler file. The handler now re-imports them; the unit test imports directly from the utils module and no longer pulls the `withSigning` → `nitropack/runtime` virtual chain at load time.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/script/src/runtime/server/utils/instagram-embed.ts`:
- Around line 93-110: The current use of selector.split(',') in scopeCss (around
the selector -> selectors mapping) incorrectly splits commas inside functional
pseudo-classes like :is(), :where(), :not(), and :has(), producing invalid
scoped CSS; replace that naive split with a nesting-aware selector splitter that
only splits on top-level commas (respecting parentheses, brackets, and quotes)
and use it where selectors is computed (instead of selector.split(',')) so
subsequent normalization (MULTI_SPACE_RE), filtering, mapping to scopedSelectors
(`scopeSelector ${s}`), and the output.push remain correct; ensure the new
splitter preserves whitespace and original selector fragments so
filteredSelectors and scopeSelector scoping logic works unchanged.
- Around line 65-69: The code assumes atRule.content always has a block by using
braceStart = atRule.content.indexOf('{') and slicing, which fails when
extractAtRule() returns a statement-form like "@layer foo;" (braceStart === -1)
and emits a stray "}". Fix in the branch handling atName === 'media' ||
'supports' || 'layer' by first checking if braceStart === -1 and, if so, push
the original atRule.content unchanged to output (preserving the statement-form);
otherwise proceed with extracting innerCss, calling processRules(innerCss,
scopeSelector), and pushing the reconstructed block as it currently does (using
atRule.content.slice(0, braceStart + 1) + scopedInner + '}').
🪄 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: 2cdebe19-7d5b-415d-8f14-82e4f8433f65
📒 Files selected for processing (3)
packages/script/src/runtime/server/instagram-embed.tspackages/script/src/runtime/server/utils/instagram-embed.tstest/unit/instagram-embed.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/unit/instagram-embed.test.ts
…lector split Addresses CodeRabbit findings: - scopeCss now preserves statement-form `@layer foo;` rules instead of emitting a stray `}`. - Top-level selector splitting respects parentheses, brackets, and quoted strings so commas inside `:is(...)`, `:not(...)`, and attribute selectors no longer fragment scoped rules.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/instagram-embed.test.ts (1)
292-297: Test name mentions:where()but fixture doesn’t include it.Optional: include a
:where(...)selector in the same test so title and assertion coverage match exactly.Suggested test tweak
- it('does not split commas inside :is() / :where() / :not() selectors', () => { - const css = ':is(.Embed, .EmbedMedia) > :not(span, em) { color: red; }' + it('does not split commas inside :is() / :where() / :not() selectors', () => { + const css = ':is(.Embed, .EmbedMedia) > :not(span, em), :where(.EmbedCard, .EmbedFooter) { color: red; }' const result = scopeCss(css, scope) expect(result).toContain(`${scope} :is(.Embed, .EmbedMedia) > :not(span, em)`) + expect(result).toContain(`${scope} :where(.EmbedCard, .EmbedFooter)`) expect(result).not.toMatch(/\.EmbedMedia\) > :not\(span$/m) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/instagram-embed.test.ts` around lines 292 - 297, The test titled "does not split commas inside :is() / :where() / :not() selectors" currently only exercises :is() and :not(); update the fixture in the same test (the css string passed to scopeCss) to also include a :where(...) selector (e.g., add :where(.SomeClass, .OtherClass) alongside :is/.not) so the test name matches coverage and verifies scopeCss handles :where() comma-containing lists correctly; keep assertions using scopeCss and the same scope variable (referencing scopeCss and the test case name) and assert the resulting string contains the scoped :where(...) and does not incorrectly split its commas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/instagram-embed.test.ts`:
- Around line 292-297: The test titled "does not split commas inside :is() /
:where() / :not() selectors" currently only exercises :is() and :not(); update
the fixture in the same test (the css string passed to scopeCss) to also include
a :where(...) selector (e.g., add :where(.SomeClass, .OtherClass) alongside
:is/.not) so the test name matches coverage and verifies scopeCss handles
:where() comma-containing lists correctly; keep assertions using scopeCss and
the same scope variable (referencing scopeCss and the test case name) and assert
the resulting string contains the scoped :where(...) and does not incorrectly
split its commas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 407491dc-5f2a-4b84-9eb3-b9a6984787b2
📒 Files selected for processing (2)
packages/script/src/runtime/server/utils/instagram-embed.tstest/unit/instagram-embed.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/script/src/runtime/server/utils/instagram-embed.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
N/A
❓ Type of change
📚 Description
Swaps
useRuntimeConfig/useNitroAppimports in the server-side proxy handlers (proxy-handler.ts,google-maps-geocode-proxy.ts,google-static-maps-proxy.ts,gravatar-proxy.ts,utils/withSigning.ts) from the Nuxt#importsvirtual module to directnitropack/runtimeimports. This removes the lazy dynamicimport('#imports')workaround inwithSigningand lets unit tests import handler files (and their colocated utilities likerewriteUrl/scopeCss) without dragging in Nuxt's virtual module graph.Adds a vitest alias on the
unitproject pointingnitropack/runtimeto a tiny local mock so handler files resolve cleanly in Node tests. Also includes a routine catalog bump (@antfu/eslint-config,happy-dom,oxc-parser,posthog-js).