fix(transforms): strip browser server hooks before compile - #3956
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
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 |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0918d941f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e0918d9 to
196d010
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 196d010eee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2c62d7b to
0a223e5
Compare
|
@codex review |
1 similar comment
|
@codex review |
0a223e5 to
1f76379
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
1 similar comment
|
@codex review |
The browser artifact should never feed server-only hook bodies into custom pre-compile plugins, esbuild keepNames output, or development sourcemaps. Moving the browser strip pass ahead of compile makes the ordering explicit and removes the compile-stage metadata handoff. The strip pass now classifies only value references while preserving authored JSX pragmas. Direct helper callers keep the legacy behavior for unrelated unused imports by demoting them to side-effect imports. The browser pipeline passes an explicit TypeScript-only option so TS and TSX inputs leave unrelated named imports authored for the following compile stage to erase. Hook-owned project imports are still deleted when no surviving browser code reads any binding from that statement, so their transitive server graph is not kept alive. JS, JSX, and generated MDX browser artifacts retain only a bare side-effect import for unrelated unused project imports. Precompile parsing now accepts the compiler-supported decorator placements before and after export. Authored parser source diagnostics are surfaced with the existing compilation-error tenant-build-failure shape, while missing parser and parser-internal failures remain framework owned. Constraint: Browser pipeline custom plugins are sorted by numeric stage, so the plugin stage and pipeline array order must both place stripping between parse and compile. Constraint: Re-exported and import-equals server hooks have no local body that the strip pass can safely empty. Rejected: Cherry-pick PR #3855 wholesale | it carries unrelated broad predicate/corpus work beyond the independently landable strip-before-compile fix. Rejected: Keep compile sourcemap metadata restoration | stripping before compile makes the browser compile map naturally originate from stripped input. Confidence: medium Scope-risk: moderate Directive: Keep browser-server-exports-strip before compile unless unused-import, parse-diagnostic ownership, and source-map behavior are redesigned together. Tested: deno task test:file extensions/ext-parser-babel/src/index.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts src/transforms/pipeline/stages/compile.test.ts src/transforms/pipeline/index.test.ts src/build/bundler/code-splitter/esbuild-plugin.test.ts Tested: deno fmt --check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno lint extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno check extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.ts src/transforms/esm/transform-utils.ts src/transforms/pipeline/stages/compile.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: git diff --check Tested: rg -n Unicode dash scan across touched files, no matches Tested: rg -n console.log, empty catch, and hardcoded apiKey patterns across touched files, no matches Not-tested: Full repository test suite Related: #3855
The browser server-hook strip stage now runs before compile, so it must preserve the old parse behavior and diagnostics that downstream compile used to provide. The parser-only path retries the legacy decorator plugin only for Babel's parameter-decorator rejection, matching the full parser's accepted TypeScript syntax without widening ordinary decorator parsing. Authored source parse diagnostics are also recorded in the compile-error collector before being rethrown as tenant compilation errors, keeping dev dashboard and MCP error reporting intact after the stage reorder. Constraint: Browser server-hook stripping must parse source before esbuild compiles TypeScript. Rejected: Return to post-compile stripping | that would reintroduce server-hook leakage into browser transforms. Rejected: Use decorators-legacy by default | it would lose the current decorators-after-export syntax accepted by the branch. Confidence: high Scope-risk: narrow Directive: Keep parser-only syntax parity with the full Babel parser when this stage parses browser source before compile. Tested: deno task test:file extensions/ext-parser-babel/src/index.test.ts extensions/ext-parser-babel/src/parser-only.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno task fmt:check && deno task lint && deno task typecheck && git diff --check
Babel parser no longer exposes the File type through its namespace, so the parse-only parser keeps the existing runtime parse behavior and lets parser.parse infer its current return type. Constraint: CI compiles integration and binary e2e shards against the current Babel parser type surface Rejected: Add a type-only @babel/types import | unnecessary for the local AST handoff and widens the parse-only module surface Confidence: high Scope-risk: narrow Directive: Keep parser-only independent from traversal, generation, and full Babel extension imports Tested: clean-cache deno check extensions/ext-parser-babel/src/parser-only.ts Tested: deno task test:file extensions/ext-parser-babel/src/parser-only.test.ts extensions/ext-parser-babel/src/index.test.ts Tested: VERYFRONT_BINARY_FRESH=1 deno task test:e2e:binary (65 steps) Tested: deno task typecheck; deno fmt --check; deno lint; git diff --check
|
@codex review |
e93bbb9 to
2045fae
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2045fae908
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The browser strip pass parses authored TypeScript before esbuild, so it must accept the union of modern and legacy decorator syntax that the compiler accepts. Try the modern Babel dialect first, retry any Babel syntax rejection with the legacy dialect, and preserve the original diagnostic when neither parser accepts the source. Constraint: Babel modern and legacy decorator plugins accept disjoint syntax that pinned esbuild supports Rejected: Whitelist individual Babel reason codes | the stable parameter-decorator code already missed generic decorator expressions reported as UnexpectedToken Confidence: high Scope-risk: narrow Directive: Keep the primary modern parse first so decorators-after-export remain supported, and rethrow its diagnostic when both dialects reject the source Tested: red-green exact generic-decorator browser pipeline regression Tested: deno task test:file combined parser and browser-strip suite (7 files, 252 steps) Tested: deno task typecheck; deno fmt --check; deno lint; deno check; git diff --check
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Why
Browser server hooks were stripped after compile. That let server-only bodies reach custom pre-compile plugins and development source maps, and forced the strip stage to repair compiler metadata after the fact.
PR #3855 identified the ordering problem, but its branch grew to thousands of lines and mixed the fix with broad predicate corpora. This is the bounded extraction.
What changed
browser-server-exports-stripafter parse and before compile.export.compilation-errorclassification for authored syntax diagnostics while keeping generated MD/MDX and parser-internal failures framework owned.Proof
The regression test observes the real browser pipeline. Before the ordering change, a custom plugin at
PARSE + 0.75seesSERVER_SECRET; after the change it does not, and the final artifact contains neither the binding nor its value.Final-artifact tests also cover:
The independently extracted #3955 commit composes with this branch without conflict. Its 16-step leak corpus and this branch's focused suites pass together.
Validation
deno fmt --check,deno lint,deno check, andgit diff --checkSupersedes #3855.
Refs veryfront/veryfront-issue-inbox#605.