fix(cloudflare): omit worker entry from wrangler.json for static builds#4255
Draft
danielroe wants to merge 1 commit into
Draft
fix(cloudflare): omit worker entry from wrangler.json for static builds#4255danielroe wants to merge 1 commit into
danielroe wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR fixes static Nuxt output deployment to Cloudflare Workers by refactoring ChangesCloudflare Static Build Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
8 tasks
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/cloudflare.utils.test.ts (1)
48-71: ⚡ Quick winSolid regression coverage; consider an additional case with a non-default
baseURL.The two cases pin down the main regression nicely. A third case with
baseURL: "/foo/"(or"/foo/bar/") would also exerciseassetsDirectoryfor non-root deployments, where the relative path differs (e.g.".."or further up) and would have caught the"..".repeatissue noted onutils.ts.🤖 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 `@test/unit/cloudflare.utils.test.ts` around lines 48 - 71, Add a third unit test that mirrors the existing two but sets the nitro stub's baseURL to a non-root path (e.g. baseURL: "/foo/" or "/foo/bar/") using createNitroStub({ static: false, baseURL: "/foo/" }) (or static:true variant as appropriate), call writeWranglerConfig(nitro, "module"), read the generated JSON from ".output/server/wrangler.json" (using readFileSync and JSON.parse), and assert that the assets directory/binding path is computed correctly for non-root deployments (verify config.assets.directory and config.assets.binding or absence of main/no_bundle/rules as per static vs non-static). This will exercise assetsDirectory logic in utils.ts and catch the repeated ".." bug.
🤖 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 `@src/presets/cloudflare/utils.ts`:
- Around line 231-237: assetsDirectory calculation uses "..".repeat(N) which
concatenates dots (e.g. "....") instead of producing repeated "../" segments,
breaking path traversal for baseURL depth >1; update the resolve call that
computes assetsDirectory (the call using wranglerConfigDir,
resolve(nitro.options.output.publicDir, "..".repeat(...))) to supply N separate
".." path segments rather than a single repeated-dot string—e.g., compute count
= nitro.options.baseURL.split("/").filter(Boolean).length and pass either an
array of ".." repeated count spread into resolve or a single
"../".repeat(count).slice(0,-1) string so pathe.resolve correctly climbs N
directories from nitro.options.output.publicDir when resolving against
wranglerConfigDir.
---
Nitpick comments:
In `@test/unit/cloudflare.utils.test.ts`:
- Around line 48-71: Add a third unit test that mirrors the existing two but
sets the nitro stub's baseURL to a non-root path (e.g. baseURL: "/foo/" or
"/foo/bar/") using createNitroStub({ static: false, baseURL: "/foo/" }) (or
static:true variant as appropriate), call writeWranglerConfig(nitro, "module"),
read the generated JSON from ".output/server/wrangler.json" (using readFileSync
and JSON.parse), and assert that the assets directory/binding path is computed
correctly for non-root deployments (verify config.assets.directory and
config.assets.binding or absence of main/no_bundle/rules as per static vs
non-static). This will exercise assetsDirectory logic in utils.ts and catch the
repeated ".." bug.
🪄 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: 736d0144-64e6-4f1f-ad21-437d6237738c
📒 Files selected for processing (2)
src/presets/cloudflare/utils.tstest/unit/cloudflare.utils.test.ts
Comment on lines
+231
to
+237
| const assetsDirectory = relative( | ||
| wranglerConfigDir, | ||
| resolve( | ||
| nitro.options.output.publicDir, | ||
| "..".repeat(nitro.options.baseURL.split("/").filter(Boolean).length) | ||
| ) | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the `"..".repeat` pattern's behavior and check if it appears elsewhere in the repo.
rg -nP '\.\.".repeat\(' --type=ts -C2
# Sanity check pathe resolve semantics
cat <<'EOF' > /tmp/check.mjs
import { resolve } from "pathe";
console.log("repeat(0):", JSON.stringify("..".repeat(0)));
console.log("repeat(2):", JSON.stringify("..".repeat(2)));
console.log("resolve depth 2 (broken):", resolve("/tmp/out/public", "..".repeat(2)));
console.log("resolve depth 2 (spread):", resolve("/tmp/out/public", "..", ".."));
EOF
# (Cannot execute in sandbox; included for reference.)
cat /tmp/check.mjsRepository: nitrojs/nitro
Length of output: 703
🏁 Script executed:
# First, check if pathe is available in the repo
cd /tmp && node --input-type=module <<'EOF'
import { resolve } from "pathe";
console.log("=== String repetition results ===");
console.log('"..".repeat(0):', JSON.stringify("..".repeat(0)));
console.log('"..".repeat(1):', JSON.stringify("..".repeat(1)));
console.log('"..".repeat(2):', JSON.stringify("..".repeat(2)));
console.log('"..".repeat(3):', JSON.stringify("..".repeat(3)));
console.log("\n=== pathe.resolve() behavior ===");
const base = "/app/dist/public";
// Test with broken input (4 consecutive dots)
const brokenPath = resolve(base, "..".repeat(2));
console.log("resolve(base, '....'):", brokenPath);
// Test with correct input (spread operator)
const correctPath1 = resolve(base, "..", "..");
console.log("resolve(base, '..', '..'):", correctPath1);
// Test with correct input (alternative: "../" repeated)
const correctPath2 = resolve(base, "../".repeat(2).slice(0, -1));
console.log("resolve(base, '../'.repeat(2).slice(0,-1)):", correctPath2);
console.log("\n=== Comparison ===");
console.log("brokenPath === correctPath1:", brokenPath === correctPath1);
console.log("correctPath1 === correctPath2:", correctPath1 === correctPath2);
EOFRepository: nitrojs/nitro
Length of output: 1045
🏁 Script executed:
# Search for tests related to cloudflare preset and assetsDirectory
fd -e test.ts -e spec.ts -e test.js | xargs grep -l "cloudflare\|assetsDirectory" 2>/dev/null || echo "No direct test files found"
# Search in test directory
fd . test --type f 2>/dev/null | head -20Repository: nitrojs/nitro
Length of output: 855
🏁 Script executed:
# Check the actual code context around the fix to understand the impact
sed -n '220,250p' src/presets/cloudflare/utils.tsRepository: nitrojs/nitro
Length of output: 1109
🏁 Script executed:
# Check the cloudflare utils test file for baseURL and assetsDirectory tests
cat -n test/unit/cloudflare.utils.test.tsRepository: nitrojs/nitro
Length of output: 3232
🏁 Script executed:
# Also check the presets test files for baseURL coverage
rg -n "baseURL" test/presets/cloudflare-module.test.ts test/presets/cloudflare-pages.test.ts --context=3Repository: nitrojs/nitro
Length of output: 39
🏁 Script executed:
# Check pathe package source or docs to understand resolve behavior
fd package.json -x grep -l pathe {} \; | head -5Repository: nitrojs/nitro
Length of output: 72
🏁 Script executed:
# Let's verify the JavaScript string behavior and reason about resolve semantics
node --eval='
console.log("JavaScript string repeat:");
console.log("\"..\" + \"..\" =", "\"..\" + \"..\"" + " -> " + (".." + ".."));
console.log(".repeat(2) =", ".repeat(2)" + " -> " + ("..".repeat(2)));
console.log("\nNode.js path.resolve semantics:");
console.log("path.resolve treats \"..\" as parent dir reference");
console.log("path.resolve treats \".....\" (or any non-.. string) as a directory name");
console.log("\nTherefore:");
console.log("resolve(base, \"..\", \"..\") -> climbs 2 levels");
console.log("resolve(base, \"....\") -> resolves to base/.... (literal directory)");
'Repository: nitrojs/nitro
Length of output: 426
🏁 Script executed:
# Check if there are any tests with baseURL other than "/"
rg "baseURL" test/ -t ts --context=2 | head -30Repository: nitrojs/nitro
Length of output: 2024
🏁 Script executed:
# Verify the proposed fix syntax is valid TypeScript
cat > /tmp/test_fix.ts << 'EOF'
import { resolve } from "pathe";
// Original broken approach
const broken = resolve("/tmp", "..".repeat(2));
// Proposed fix: spread operator
const fixed1 = resolve("/tmp", ...Array.from(
{ length: 2 },
() => ".."
));
// Alternative mentioned in the comment
const fixed2 = resolve("/tmp", "../".repeat(2).slice(0, -1));
console.log("Broken:", broken);
console.log("Fixed1:", fixed1);
console.log("Fixed2:", fixed2);
EOF
cat /tmp/test_fix.tsRepository: nitrojs/nitro
Length of output: 481
"..".repeat(N) produces broken paths for baseURL depth > 1.
"..".repeat(2) returns the string "...." (a single 4-dot segment), not "../..". pathe.resolve(publicDir, "....") then treats "...." as a literal directory name and resolves to publicDir + "/...." instead of climbing two levels up. So assetsDirectory is incorrect for any baseURL with two or more non-empty segments (e.g. "/foo/bar/"). The default baseURL = "/" (length 0 → "") and single-segment baseURL = "/foo/" (length 1 → "..") happen to work, which is why the new tests don't catch this.
This may be pre-existing (just extracted into a variable here), but the diff surface is a good place to fix it.
🐛 Proposed fix
const assetsDirectory = relative(
wranglerConfigDir,
resolve(
nitro.options.output.publicDir,
- "..".repeat(nitro.options.baseURL.split("/").filter(Boolean).length)
+ ...Array.from(
+ { length: nitro.options.baseURL.split("/").filter(Boolean).length },
+ () => ".."
+ )
)
);Note: pathe.resolve is variadic, so spreading multiple ".." segments climbs the directory tree correctly for any depth. Alternatively, "../".repeat(N).slice(0, -1) as a single argument also resolves correctly.
🤖 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 `@src/presets/cloudflare/utils.ts` around lines 231 - 237, assetsDirectory
calculation uses "..".repeat(N) which concatenates dots (e.g. "....") instead of
producing repeated "../" segments, breaking path traversal for baseURL depth >1;
update the resolve call that computes assetsDirectory (the call using
wranglerConfigDir, resolve(nitro.options.output.publicDir, "..".repeat(...))) to
supply N separate ".." path segments rather than a single repeated-dot
string—e.g., compute count =
nitro.options.baseURL.split("/").filter(Boolean).length and pass either an array
of ".." repeated count spread into resolve or a single
"../".repeat(count).slice(0,-1) string so pathe.resolve correctly climbs N
directories from nitro.options.output.publicDir when resolving against
wranglerConfigDir.
Member
|
Nitro v3 / vite has no working full staic mode yet. Pending till it does so i can test. |
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
2 participants
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
resolves nuxt/nuxt#34186
❓ Type of change
📚 Description
https://developers.cloudflare.com/workers/static-assets/
this aims to fix the generated
wrangler.jsonfor fully-static sites📝 Checklist