Validate app.bundlerOptions types in Bun.serve#30423
Closed
robobun wants to merge 2 commits into
Closed
Conversation
Passing a non-object value for app.bundlerOptions, or for its server/ client/ssr sub-objects, or a non-boolean non-object for their minify property, would hit a debug assertion in JSValue.get(). Additionally, minify: false would fall through to the object path and hit the same assertion. Throw proper TypeErrors for these cases instead.
Collaborator
Author
Contributor
WalkthroughThe PR enhances bundler options validation error messages by introducing a ChangesBundler Options Validation Error Messages
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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: 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 `@test/bake/bundler-options-validation.test.ts`:
- Around line 13-42: Replace the manual for-loops with Jest's parameterized
helpers: use describe.each([...]) for the outer matrix over
["server","client","ssr"] to create a describe block per key, and inside each
describe use either test.each([...]) or another describe.each for the minify
variants ([true, false, { whitespace: true }]) to produce individual tests for
non-object cases and accepted values; update the test titles to interpolate the
current key/minify (matching existing strings like `non-object
bundlerOptions.${key} throws`, `non-object non-boolean
bundlerOptions.${key}.minify throws`, and `bundlerOptions.${key}.minify =
${JSON.stringify(minify)} is accepted`) and remove the loop variables, keeping
the same Bun.serve assertions.
- Around line 33-40: The test currently uses .not.toThrow(/must be/) which can
pass for unrelated errors; instead assert for the specific downstream error that
occurs only when the bundlerOptions shape is accepted. Update the assertion in
the test around Bun.serve(...) (the block creating app: { bundlerOptions: {
[key]: { minify } } }) to expect the known runtime/startup error message
produced when the bundlerOptions branch is accepted (e.g., replace
.not.toThrow(/must be/) with .toThrow(/<actual downstream error message>/)); run
the test once to capture the real downstream message and use that exact regex so
the test proves this validation path was exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 958e89b7-7c61-4a21-9d6c-a73c6b0d1b7d
📒 Files selected for processing (2)
src/bake/bake.zigtest/bake/bundler-options-validation.test.ts
Comment on lines
+13
to
+42
| for (const key of ["server", "client", "ssr"] as const) { | ||
| test(`non-object bundlerOptions.${key} throws`, () => { | ||
| expect(() => | ||
| Bun.serve({ | ||
| // @ts-expect-error | ||
| app: { bundlerOptions: { [key]: 1225 } }, | ||
| }), | ||
| ).toThrow(`'bundlerOptions.${key}' must be an object`); | ||
| }); | ||
|
|
||
| test(`non-object non-boolean bundlerOptions.${key}.minify throws`, () => { | ||
| expect(() => | ||
| Bun.serve({ | ||
| // @ts-expect-error | ||
| app: { bundlerOptions: { [key]: { minify: 1225 } } }, | ||
| }), | ||
| ).toThrow(`'bundlerOptions.${key}.minify' must be a boolean or an object`); | ||
| }); | ||
|
|
||
| for (const minify of [true, false, { whitespace: true }]) { | ||
| test(`bundlerOptions.${key}.minify = ${JSON.stringify(minify)} is accepted`, () => { | ||
| expect(() => | ||
| Bun.serve({ | ||
| // @ts-expect-error | ||
| app: { bundlerOptions: { [key]: { minify } } }, | ||
| }), | ||
| ).not.toThrow(/must be/); | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use describe.each() for parameterized cases.
The loop-based matrix works, but this suite should use describe.each() per repo test conventions for parameterized coverage and reporting clarity.
Refactor sketch
- for (const key of ["server", "client", "ssr"] as const) {
- test(`non-object bundlerOptions.${key} throws`, () => {
+ describe.each(["server", "client", "ssr"] as const)("bundlerOptions.%s", key => {
+ test("non-object throws", () => {
...
- test(`non-object non-boolean bundlerOptions.${key}.minify throws`, () => {
+ test("non-object non-boolean minify throws", () => {
...
- for (const minify of [true, false, { whitespace: true }]) {
- test(`bundlerOptions.${key}.minify = ${JSON.stringify(minify)} is accepted`, () => {
+ describe.each([true, false, { whitespace: true }] as const)("minify=%j", minify => {
+ test("is accepted", () => {
...
});
- }
- }
+ });
+ });As per coding guidelines: "Use describe.each() for parameterized tests".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const key of ["server", "client", "ssr"] as const) { | |
| test(`non-object bundlerOptions.${key} throws`, () => { | |
| expect(() => | |
| Bun.serve({ | |
| // @ts-expect-error | |
| app: { bundlerOptions: { [key]: 1225 } }, | |
| }), | |
| ).toThrow(`'bundlerOptions.${key}' must be an object`); | |
| }); | |
| test(`non-object non-boolean bundlerOptions.${key}.minify throws`, () => { | |
| expect(() => | |
| Bun.serve({ | |
| // @ts-expect-error | |
| app: { bundlerOptions: { [key]: { minify: 1225 } } }, | |
| }), | |
| ).toThrow(`'bundlerOptions.${key}.minify' must be a boolean or an object`); | |
| }); | |
| for (const minify of [true, false, { whitespace: true }]) { | |
| test(`bundlerOptions.${key}.minify = ${JSON.stringify(minify)} is accepted`, () => { | |
| expect(() => | |
| Bun.serve({ | |
| // @ts-expect-error | |
| app: { bundlerOptions: { [key]: { minify } } }, | |
| }), | |
| ).not.toThrow(/must be/); | |
| }); | |
| } | |
| } | |
| describe.each(["server", "client", "ssr"] as const)("bundlerOptions.%s", key => { | |
| test("non-object throws", () => { | |
| expect(() => | |
| Bun.serve({ | |
| // `@ts-expect-error` | |
| app: { bundlerOptions: { [key]: 1225 } }, | |
| }), | |
| ).toThrow(`'bundlerOptions.${key}' must be an object`); | |
| }); | |
| test("non-object non-boolean minify throws", () => { | |
| expect(() => | |
| Bun.serve({ | |
| // `@ts-expect-error` | |
| app: { bundlerOptions: { [key]: { minify: 1225 } } }, | |
| }), | |
| ).toThrow(`'bundlerOptions.${key}.minify' must be a boolean or an object`); | |
| }); | |
| describe.each([true, false, { whitespace: true }] as const)("minify=%j", minify => { | |
| test("is accepted", () => { | |
| expect(() => | |
| Bun.serve({ | |
| // `@ts-expect-error` | |
| app: { bundlerOptions: { [key]: { minify } } }, | |
| }), | |
| ).not.toThrow(/must be/); | |
| }); | |
| }); | |
| }); |
🤖 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/bake/bundler-options-validation.test.ts` around lines 13 - 42, Replace
the manual for-loops with Jest's parameterized helpers: use describe.each([...])
for the outer matrix over ["server","client","ssr"] to create a describe block
per key, and inside each describe use either test.each([...]) or another
describe.each for the minify variants ([true, false, { whitespace: true }]) to
produce individual tests for non-object cases and accepted values; update the
test titles to interpolate the current key/minify (matching existing strings
like `non-object bundlerOptions.${key} throws`, `non-object non-boolean
bundlerOptions.${key}.minify throws`, and `bundlerOptions.${key}.minify =
${JSON.stringify(minify)} is accepted`) and remove the loop variables, keeping
the same Bun.serve assertions.
Comment on lines
+33
to
+40
| test(`bundlerOptions.${key}.minify = ${JSON.stringify(minify)} is accepted`, () => { | ||
| expect(() => | ||
| Bun.serve({ | ||
| // @ts-expect-error | ||
| app: { bundlerOptions: { [key]: { minify } } }, | ||
| }), | ||
| ).not.toThrow(/must be/); | ||
| }); |
Contributor
There was a problem hiding this comment.
Strengthen accepted-shape assertions to avoid false positives.
At Line 39, .not.toThrow(/must be/) still passes if Bun.serve() throws a different error for an unrelated reason. Assert a specific downstream error to prove this validation branch was actually accepted.
Proposed fix
- ).not.toThrow(/must be/);
+ ).toThrow("'app' is missing 'framework'");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test(`bundlerOptions.${key}.minify = ${JSON.stringify(minify)} is accepted`, () => { | |
| expect(() => | |
| Bun.serve({ | |
| // @ts-expect-error | |
| app: { bundlerOptions: { [key]: { minify } } }, | |
| }), | |
| ).not.toThrow(/must be/); | |
| }); | |
| test(`bundlerOptions.${key}.minify = ${JSON.stringify(minify)} is accepted`, () => { | |
| expect(() => | |
| Bun.serve({ | |
| // `@ts-expect-error` | |
| app: { bundlerOptions: { [key]: { minify } } }, | |
| }), | |
| ).toThrow("'app' is missing 'framework'"); | |
| }); |
🤖 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/bake/bundler-options-validation.test.ts` around lines 33 - 40, The test
currently uses .not.toThrow(/must be/) which can pass for unrelated errors;
instead assert for the specific downstream error that occurs only when the
bundlerOptions shape is accepted. Update the assertion in the test around
Bun.serve(...) (the block creating app: { bundlerOptions: { [key]: { minify } }
}) to expect the known runtime/startup error message produced when the
bundlerOptions branch is accepted (e.g., replace .not.toThrow(/must be/) with
.toThrow(/<actual downstream error message>/)); run the test once to capture the
real downstream message and use that exact regex so the test proves this
validation path was exercised.
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Collaborator
Author
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.
Passing a non-object value for
app.bundlerOptions, or for itsserver/client/ssrsub-objects, or a non-boolean non-object for theirminifyproperty, would hit a debug assertion inJSValue.get()(which requires its receiver to be an object).Additionally,
minify: falsewould fall through theisBoolean() and asBoolean()check to the object code path and hit the same assertion.Throw proper
TypeErrors for these cases instead.Found by Fuzzilli.