Conversation
|
Warning Review limit reached
Next review available in: 6 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. Reproduced on the current release ( The |
There was a problem hiding this comment.
LGTM — help-text-only change with thorough tests.
What was reviewed:
- Confirmed all 17 advertised names exist in
LOADER_NAMES(src/ast/loader.rs:73) and resolve viafrom_string; the excluded names (jsonc,sqlite_embedded,base64,dataurl,sh, aliases) are justified in the description. - Checked that this is the only
Valid loaders:string insrc/— no sibling copies missed. - Tests follow harness conventions:
tempDir,bunEnv, concurrent pipe draining,await using, stderr/stdout asserted before exit code,test.concurrentfor independent subprocesses.
Extended reasoning...
Overview
The PR updates a single string literal in src/runtime/cli/Arguments.rs:137 — the --loader help text in TRANSPILER_PARAMS_ — to list seven additional loader names (yaml, json5, xml, md, css, html, sqlite) that the flag already accepts. It adds ~120 lines of tests to test/cli/bun.test.ts under a new --help describe block: one test asserts the help text matches the expected list for both bun --help and bun run --help, and a second test maps a made-up extension to each advertised loader and imports a fixture to prove the loader is actually applied.
Security risks
None. This is a documentation string in CLI help output; it does not touch parsing, validation, or any code path that handles untrusted input. The test fixtures are self-contained in a temp dir and do not touch the network.
Level of scrutiny
Low. The runtime change is a static help string with no behavioral effect — loader_resolver() already accepts every name being added (verified against LOADER_NAMES at src/ast/loader.rs:73-102). The PR description carefully justifies which accepted-but-degraded names (jsonc, sqlite_embedded, base64, dataurl, sh) and aliases are intentionally omitted, with references to the relevant tracking PRs/issues. A grep confirms this is the only Valid loaders: string in src/, so there are no sibling copies to update in this PR (docs/completions are handled in #38266 as noted).
Other factors
The tests are well above the bar for a help-text change: they tie the advertised list to actual runtime behavior by importing a fixture through each loader and asserting a loader-specific result (parsed object for data formats, HTMLBundle for html, Database instance for sqlite, rendered HTML for md, etc.). Harness conventions are followed — tempDir with using, bunEnv/bunExe, await using on spawned processes, Promise.all over stdout/stderr/exited, stderr asserted before exit code, test.concurrent for independent subprocess tests. No CODEOWNERS cover the touched files. No prior reviewer comments to address.
|
Updated 4:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 41c0527 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38270That installs a local version of the PR into your bun-38270 --bun |
d66a476 to
6fc38d5
Compare
There was a problem hiding this comment.
LGTM — one-line help-string update bringing the --loader list in sync with what LOADER_NAMES already accepts.
What was reviewed:
- Confirmed all seven added names (
yaml,json5,xml,md,css,html,sqlite) are present inLOADER_NAMESatsrc/ast/loader.rs:73and reachloader_resolverviafrom_string. - Checked the intentionally-omitted names (
jsonc,sqlite_embedded,base64,dataurl,sh, aliases) against the table — the PR description's rationale holds. - Tests follow harness conventions (concurrent pipe drain,
tempDir, stderr-before-exitCode) and the help-text assertion fails on the old string.
Extended reasoning...
Overview
The functional change is a single string literal in TRANSPILER_PARAMS_ at src/runtime/cli/Arguments.rs:137 — the --loader help text — expanding the advertised loader list from 10 to 17 names. The rest of the diff is two new test blocks in test/cli/bun.test.ts: one asserting the help text on bun --help and bun run --help, and one that maps a synthetic extension to each advertised name and imports a fixture through it, proving each name is both accepted by argument parsing and applied at runtime.
Security risks
None. This edits a static help string and adds read-only tests that spawn the bun binary in a temp dir. No parsing, validation, or resolution logic is touched.
Level of scrutiny
Low. The runtime change is documentation-only — loader_resolver() already routes through bun_ast::Loader::from_string over LOADER_NAMES, and I verified every newly-listed name appears there. The PR description explicitly justifies each inclusion and each omission (jsonc/sqlite_embedded collapse in to_api(), base64/dataurl are stubs, sh maps to file, mjs/cjs/etc. are aliases), which matches the source. The test file additions are the larger surface but follow the existing patterns in the same file (the neighboring --help preserves <placeholder> block uses identical spawn/drain/assert structure).
Other factors
The tests are well-constructed per REVIEW.md: test.concurrent for independent subprocess spawns, Promise.all on stdout/stderr/exited, using tempDir, bunEnv/bunExe(), stderr asserted before exitCode, and the help-text assertion is exact (toEqual on the split list) rather than toContain. The second test ties the advertised list to actual behavior, so future drift in either direction (a name stops working, or a new loader is added without updating help) will surface here. No prior reviews or outstanding comments; the noted clippy failure is unrelated (from main, fixed in #38272).
6fc38d5 to
d87ece3
Compare
The -l/--loader help line still advertised the loader set from before css, html, yaml, json5, xml, md and sqlite existed. The flag resolves names through bun_ast::Loader::from_string, so all of these are accepted and applied; list them, in the same order as the --loader docs.
d87ece3 to
41c0527
Compare
There was a problem hiding this comment.
LGTM — one-line help-string update bringing the --loader list in line with what LOADER_NAMES already accepts.
What was reviewed
- Confirmed every added name (
yaml,json5,xml,md,css,html,sqlite) is present inLOADER_NAMESatsrc/ast/loader.rs:73and reachable vialoader_resolver(). - Checked the new tests: pipes drained concurrently,
tempDir/bunEnv/await usingper harness convention, stderr/stdout asserted before exit code,test.concurrentfor independent subprocess spawns. - The earlier comment-cop note on
Arguments.rswas addressed in 41c0527 and the thread is resolved; the src diff is now the single string edit.
Extended reasoning...
Overview
The PR updates a single hard-coded help string in src/runtime/cli/Arguments.rs (the -l, --loader entry in TRANSPILER_PARAMS_) to list seven loader names that the flag already accepts but the help text omitted. It adds a describe("--loader help text") block to test/cli/bun.test.ts with two tests: one asserting bun --help and bun run --help print the exact 17-name list, and one that maps a synthetic extension to each advertised name, imports a fixture, and asserts the loader-specific result — tying the advertised list to actual behavior.
Security risks
None. This is a display-only string in CLI help output; no parsing, resolution, or runtime logic changed. The test additions spawn the local bunExe() in a temp dir with no network access.
Level of scrutiny
Low. The source change is a mechanical documentation fix — the flag's resolver (loader_resolver → bun_ast::Loader::from_string over LOADER_NAMES) is untouched, and I verified against src/ast/loader.rs that every newly listed name maps to a real Loader variant. The PR description carefully justifies which accepted names are excluded (aliases, unimplemented stubs, names that collapse via to_api()) and cross-references the sibling PRs updating docs/completions/types.
Other factors
The tests follow repo conventions cleanly (tempDir from harness, test.concurrent, Promise.all on stdout/stderr/exited, assertions before exit-code). The one prior review comment (comment-cop about a verbose source comment) was addressed in the current head commit and the thread is marked resolved. No outstanding reviewer feedback and no prior claude[bot] review on this PR.
Problem
bun --help,bun run --helpandbun repl --helpdescribe-l, --loaderas:Valid loaders: js, jsx, ts, tsx, json, toml, text, file, wasm, napi.css,html,yaml,json5,xml,mdandsqliteloaders. The flag resolves its value throughloader_resolver()(src/runtime/cli/Arguments.rs:38), which isbun_ast::Loader::from_stringoverLOADER_NAMES(src/ast/loader.rs:73), so all seven are accepted and applied today; the help text is the only in-binary place they are missing.TRANSPILER_PARAMS_atsrc/runtime/cli/Arguments.rs:137.Fix
js, jsx, ts, tsx, json, toml, yaml, json5, xml, text, md, css, html, wasm, napi, sqlite, file, the same list (same order) that docs, completions: update the loader name lists for bunfig [loader] and --loader #38266 puts in the--loaderdocs and shell completions. No behavior change.yaml/json5/xml, rendered HTML formd, anHTMLBundleforhtml, aDatabaseforsqlite, and the same module shape as a real.cssfile forcss. Without the mapping each fixture imports as a path (thefileloader), so the flag is what makes them work.from_stringaccepts:jsoncandsqlite_embeddedcollapse tojsonandsqliteinto_api()(src/options_types/bundle_enums.rs, being fixed in bundler: honor jsonc and sqlite_embedded in loader maps, accept every loader name in Bun.build #38247, after which both belong in this list),base64anddataurlare unimplemented stubs (bundler: implement dataurl and base64 loaders #36327, runtime + --no-bundle: implement base64/dataurl loaders #36334),shmaps tofile, and the aliases (mjs,cjs,mts,cts,node,txt,markdown) stay unlisted as before. The full accepted set is what theinvalid loadererror prints (cli: list the names --loader accepts in the invalid loader error #38283); the help line is the recommended subset.test/cli/bun.test.ts:bun --helpandbun run --help: the--loaderline'sValid loaders:list equals the 17 names above. Fails on the current release (the eight missing names), passes with this change.bunrun that maps a distinct extension to every advertised name and imports a fixture for each of the 15 that are observable at runtime, asserting the loader-specific result;wasmandnapiare only mapped (an unknown name fails argument parsing). This ties the advertised list to what the flag accepts.bun bd test test/cli/bun.test.ts: 24 pass.Related
docs/snippets/cli/run.mdx,docs/runtime/bunfig.mdx,completions/bun.zshandcompletions/bun.bash; docs, completions: update the loader name lists for bunfig [loader] and --loader #38266 updates those copies to this list. This PR carries the binary half because asrc/change needs the help-output test. TheLoadertype inbun-typesis updated separately in bun-types: add json5 and md to the Loader union, document both loaders #38267.test/internal/source-lints/) asserting the copies equalLOADER_NAMESminus an explicit exclusion list would stop the next drift; it can only pass once both are in one tree, so it is a follow-up rather than part of either PR.--loaderextension, accept it in bunfig too #36904 edits this same help line (adds an extensionless example) and will need a one-line rebase.Background
jsonparses to an object,textreturns the contents,filereturns the path, and so on).--loader .ext:nameoverrides the extension to loader mapping for the runtime and the bundler.LOADER_NAMESis the table of spellings--loader(and bunfig[loader]) accept;to_api()then converts the chosen loader into the API enum the bundler and runtime consume, which is where a few accepted names currently degrade into another loader.