fix: respect compress settings in dev server#4159
Conversation
|
@hellozyemlya is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request updates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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)
src/dev/app.ts (1)
113-113: Remove the explanatory inline comment.Line 113 adds a comment that explains straightforward code behavior; this repo guideline asks to avoid such comments unless requested.
As per coding guidelines, "Do not add comments explaining what the line does unless prompted".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dev/app.ts` at line 113, Remove the explanatory inline comment "// Determine compression settings" that was added at the start of the compression configuration block; locate the comment near the compression-related code (the section dealing with compression settings/variables in app.ts) and delete the comment while leaving the surrounding code and logic (the compression configuration variables/functions) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dev/app.ts`:
- Around line 140-145: Replace the naive substring checks for brotli/gzip in the
compression branch with an Accept-Encoding parser that honors q-values: add an
acceptsEncoding(header, encoding) helper (as proposed) and use it instead of
acceptEncoding.includes("br") and acceptEncoding.includes("gzip") in the logic
around stream.pipe(createBrotliCompress()) and the gzip branch; the helper
should split on commas, parse each item into name and params, treat "*" as
matching, parse any "q=" param to a number, and only return true if a matching
encoding has no q or q > 0.0 so clients that send q=0 are correctly respected.
---
Nitpick comments:
In `@src/dev/app.ts`:
- Line 113: Remove the explanatory inline comment "// Determine compression
settings" that was added at the start of the compression configuration block;
locate the comment near the compression-related code (the section dealing with
compression settings/variables in app.ts) and delete the comment while leaving
the surrounding code and logic (the compression configuration
variables/functions) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (compressOpts.brotli && acceptEncoding.includes("br")) { | ||
| event.res.headers.set("Content-Encoding", "br"); | ||
| event.res.headers.delete("Content-Length"); | ||
| event.res.headers.set("Vary", "Accept-Encoding"); | ||
| return stream.pipe(createBrotliCompress()); | ||
| } else if (acceptEncoding.includes("gzip")) { | ||
| } else if (compressOpts.gzip && acceptEncoding.includes("gzip")) { |
There was a problem hiding this comment.
Honor Accept-Encoding quality values (q) instead of substring matching.
Line 140 and Line 145 use .includes("br") / .includes("gzip"), which will incorrectly compress even when the client sends br;q=0 or gzip;q=0.
💡 Proposed fix
- const acceptEncoding = event.req.headers.get("accept-encoding") || "";
- if (compressOpts.brotli && acceptEncoding.includes("br")) {
+ const acceptEncoding = event.req.headers.get("accept-encoding") || "";
+ if (compressOpts.brotli && acceptsEncoding(acceptEncoding, "br")) {
event.res.headers.set("Content-Encoding", "br");
event.res.headers.delete("Content-Length");
event.res.headers.set("Vary", "Accept-Encoding");
return stream.pipe(createBrotliCompress());
- } else if (compressOpts.gzip && acceptEncoding.includes("gzip")) {
+ } else if (compressOpts.gzip && acceptsEncoding(acceptEncoding, "gzip")) {
event.res.headers.set("Content-Encoding", "gzip");
event.res.headers.delete("Content-Length");
event.res.headers.set("Vary", "Accept-Encoding");
return stream.pipe(createGzip());
}function acceptsEncoding(header: string, encoding: "br" | "gzip") {
return header.split(",").some((item) => {
const [name, ...params] = item.trim().toLowerCase().split(";");
if (name !== encoding && name !== "*") {
return false;
}
const q = params
.map((p) => p.trim())
.find((p) => p.startsWith("q="))
?.slice(2);
if (!q) {
return true;
}
const quality = Number.parseFloat(q);
return Number.isFinite(quality) && quality > 0;
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dev/app.ts` around lines 140 - 145, Replace the naive substring checks
for brotli/gzip in the compression branch with an Accept-Encoding parser that
honors q-values: add an acceptsEncoding(header, encoding) helper (as proposed)
and use it instead of acceptEncoding.includes("br") and
acceptEncoding.includes("gzip") in the logic around
stream.pipe(createBrotliCompress()) and the gzip branch; the helper should split
on commas, parse each item into name and params, treat "*" as matching, parse
any "q=" param to a number, and only return true if a matching encoding has no q
or q > 0.0 so clients that send q=0 are correctly respected.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the git log for the serveStaticDir function to see what it looked like before this PR
git log --oneline -10 src/dev/app.ts
# Check the original state of the Accept-Encoding checks before this PR
git show HEAD~1:src/dev/app.ts 2>/dev/null | grep -n "acceptEncoding\|includes\|Accept-Encoding" || \
git log --all --oneline | head -5
Length of output: 724
@jantimon Yes, the q=0 / naive substring-matching issue is pre-existing — it was there before this PR. The git history confirms that commit bbf2808 (the parent of this PR) already had:
if (acceptEncoding.includes("br")) { ... }
else if (acceptEncoding.includes("gzip")) { ... }This PR only added the compressOpts.brotli && and compressOpts.gzip && guards in front of those same checks. The .includes() approach itself was introduced earlier in 9ee9f21 (refactor(dev): improve serveStaticDir with compression and streaming).
Since it's pre-existing and out of scope for this PR, would you like me to open a separate follow-up issue to track it?
🧬 Code Graph Analysis Results
-
File: src/types/config.ts (lines 335-339)
Content:
export interface CompressOptions {
gzip?: boolean;
brotli?: boolean;
zstd?: boolean;
} -
File: src/runtime/internal/error/dev.ts (lines 101-117)
Content:
export async function loadStackTrace(error: any): Promise {
if (!(error instanceof Error)) {
return;
}const { ErrorParser } = await import("youch-core");
const parsed = await new ErrorParser().defineSourceLoader(sourceLoader).parse(error);
const stack = error.message + "\n" + parsed.frames.map((frame) => fmtFrame(frame)).join("\n");
Object.defineProperty(error, "stack", { value: stack });
if (error.cause) {
await loadStackTrace(error.cause).catch(consola.error);
}
} -
File: src/dev/server.ts (lines 208-247)
Content:
#generateError() {
const error: any = this.#buildError || this.#workerError;
if (error) {
try {
error.unhandled = false;
let id = error.id || error.path;
if (id) {
const cause = (error as { errors?: any[] }).errors?.[0];
const loc = error.location || error.loc || cause?.location || cause?.loc;
if (loc) {
id +=:${loc.line}:${loc.column};
}
error.stack = (error.stack || "").replace(/(^\s*at\s+.+)/m,at ${id}\n$1);
}
} catch {
// ignore
}
return new HTTPError(error);
}return new Response( JSON.stringify( { error: "Dev server is unavailable.", hint: "Please reload the page and check the console for errors if the issue persists.", }, null, 2 ), { status: 503, statusText: "Dev server is unavailable", headers: { "Content-Type": "application/json", "Cache-Control": "no-store", Refresh: "3", }, } );}
-
File: src/dev/vfs.ts (lines 4-157)
Content:
export function createVFSHandler(nitro: Nitro) {
return defineHandler(async (event) => {
const { socket } = event.runtime?.node?.req || {};// prettier-ignore const isUnixSocket = // No network addresses (!socket?.remoteAddress && !socket?.localAddress) && // Empty address object Object.keys(socket?.address?.() || {}).length === 0 && // Socket is readable/writable but has no port info socket?.readable && socket?.writable && !socket?.remotePort; const ip = getRequestIP(event, { xForwardedFor: isUnixSocket }); const isLocalRequest = ip && /^::1$|^127\.\d+\.\d+\.\d+$/.test(ip); if (!isLocalRequest) { throw new HTTPError({ statusText: `Forbidden IP: "${ip || "?"}"`, status: 403, }); } const url = event.context.params?._ || ""; const isJson = url.endsWith(".json") || event.req.headers.get("accept")?.includes("application/json"); const id = decodeURIComponent(url.replace(/^(\.json)?\/?/, "") || ""); if (id && !nitro.vfs.has(id)) { throw new HTTPError({ message: "File not found", status: 404 }); } const content = id ? await nitro.vfs.get(id)?.render() : undefined; if (isJson) { return { rootDir: nitro.options.rootDir, entries: [...nitro.vfs.keys()].map((id) => ({ id, path: "/_vfs.json/" + encodeURIComponent(id), })), current: id ? { id, content, } : null, }; } const directories: Record<string, any> = { [nitro.options.rootDir]: {} }; const fpaths = [...nitro.vfs.keys()]; for (const item of fpaths) { const segments = item.replace(nitro.options.rootDir, "").split("/").filter(Boolean); let currentDir = item.startsWith(nitro.options.rootDir) ? directories[nitro.options.rootDir] : directories; for (const segment of segments) { if (!currentDir[segment]) { currentDir[segment] = {}; } currentDir = currentDir[segment]; } } const generateHTML = (directory: Record<string, any>, path: string[] = []): string => Object.entries(directory) .map(([fname, value = {}]) => { const subpath = [...path, fname]; const key = subpath.join("/"); const encodedUrl = encodeURIComponent(key); const linkClass = url === `/${encodedUrl}` ? "bg-gray-700 text-white" : "hover:bg-gray-800 text-gray-200"; return Object.keys(value).length === 0 ? ` <li class="flex flex-nowrap"> <a href="/_vfs/${encodedUrl}" class="w-full text-sm px-2 py-1 border-b border-gray-10 ${linkClass}"> ${fname} </a> </li> ` : ` <li> <details ${url.startsWith(`/${encodedUrl}`) ? "open" : ""}> <summary class="w-full text-sm px-2 py-1 border-b border-gray-10 hover:bg-gray-800 text-gray-200"> ${fname} </summary> <ul class="ml-4"> ${generateHTML(value, subpath)} </ul> </details> </li> `; }) .join(""); const rootDirectory = directories[nitro.options.rootDir]; delete directories[nitro.options.rootDir]; const items = generateHTML(rootDirectory, [nitro.options.rootDir]) + generateHTML(directories); const files = ` <div class="h-full overflow-auto border-r border-gray:10"> <p class="text-white text-bold text-center py-1 opacity-50">Virtual Files</p> <ul class="flex flex-col">${items}</ul> </div> `; const file = id ? editorTemplate({ readOnly: true, language: id.endsWith("html") ? "html" : "javascript", theme: "vs-dark", value: content, wordWrap: "wordWrapColumn", wordWrapColumn: 80, }) : ` <div class="w-full h-full flex opacity-50"> <h1 class="text-white m-auto">Select a virtual file to inspect</h1> </div> `; event.res.headers.set("Content-Type", "text/html; charset=utf-8"); return /* html */ `<!doctype html>
<script src="https://cdn.jsdelivr.net/npm/@unocss/runtime"></script> <style> html { background: `#1E1E1E`; color: white; } [un-cloak] { display: none; } </style>`; }); }${files} $ {file} -
File: src/runtime/internal/route-rules.ts (lines 40-59)
Content:
export const proxy: RouteRuleCtor<"proxy"> = ((m) =>
function proxyRouteRule(event) {
let target = m.options?.to;
if (!target) {
return;
}
if (target.endsWith("/**")) {
let targetPath = event.url.pathname + event.url.search;
const strpBase = (m.options as any)._proxyStripBase;
if (strpBase) {
targetPath = withoutBase(targetPath, strpBase);
}
target = joinURL(target.slice(0, -3), targetPath);
} else if (event.url.search) {
target = withQuery(target, Object.fromEntries(event.url.searchParams));
}
return proxyRequest(event, target, {
...m.options,
});
}) satisfies RouteRuleCtor<"proxy">;
🧠 Learnings used
Learnt from: pi0
Repo: nitrojs/nitro PR: 3887
File: src/build/vite/bundler.ts:77-99
Timestamp: 2026-01-21T18:01:10.943Z
Learning: In defu v6, array properties are concatenated rather than overwritten when merging defaults with overrides. Example: defu({ array:['a','b'] }, { array:['c'] }) -> { array:['a','b','c'] }. When reviewing code that uses defu to merge config objects (e.g., src/build/vite/bundler.ts), assume arrays accumulate values from defaults and overrides. This affects plugins/loader configurations and any array-based config. If you need array replacement, consider a different merge strategy or post-process arrays after defu.
Learnt from: Teages
Repo: nitrojs/nitro PR: 4166
File: docs/4.examples/graphql-pothos.md:176-179
Timestamp: 2026-04-05T08:40:20.433Z
Learning: For projects using Pothos GraphQL v4 (`pothos/core` ^4.x), remember that output fields are nullable by default (unlike Pothos v3). The `nullable` option defaults to `true` unless you set `defaultFieldNullability: false` on the `SchemaBuilder` in your schema. During code review, ensure the intended nullability is explicit/consistent: set `nullable: true` explicitly when clarity is important, and use `nullable: false` (or `defaultFieldNullability: false`) when fields must be non-null.
pi0
changed the title
|
I applied the patch locally for our setup and it works as expected:
@pi0 the @coderabbitai remarks of |
|
Tip For best results, initiate chat on the files or code changes.
To confirm, both flagged issues are real preexisting bugs:
Would you like me to open a follow-up GitHub issue on the |
2 participants
🔗 Linked issue
❓ Type of change
📚 Description
Implements handling of "compressPublicAssets" option from nitro.config.ts in development mode.
📝 Checklist