fix(modules): skip the __esModule marker when extracting a component - #4148
Conversation
extractComponent fell back to the first key of the module namespace when
there was no default export:
const firstKey = Object.keys(moduleObj)[0];
const component = moduleObj.default ?? moduleObj[firstKey];
For a transpiled CommonJS namespace shaped { __esModule: true, Named },
moduleObj.default is undefined and firstKey is "__esModule", so the
extracted "component" was the boolean true. That shape is what many
transpilers emit for a module with only named exports, so it is not an
exotic input. The caller then received true where it expected a
component, and rendering failed later and further from the cause than it
would have if extraction had reported no component at all.
Select the first export that can actually be rendered instead: a function
(function or class component) or an object, which is what React.memo,
React.forwardRef and React.lazy produce. The __esModule marker is skipped
explicitly, and a default export that is not renderable no longer shadows
a usable named export. A namespace carrying only the marker now throws
the existing "No component exported" error at the point of extraction.
Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesComponent export resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to Component extraction may select a standalone React memo marker instead of a valid component, causing rendering to fail when that export appears first. The PR is otherwise mergeable with explicit owner awareness and a targeted allowlist fix. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Out of Scope Changes checkExplanation The changes remain within the component-extraction objective. Additional React type recognition, getter handling, export-order scanning, and tests support correct named-export selection and do not introduce unrelated functionality. ✨ 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 |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
Validating the default export as well as the named fallback went beyond the reported defect and cost a better diagnostic. A layout whose default export is not a component, as in "export default 42", previously reached build-app-route-renderer's own check and failed with "Invalid layout component", which names the slot that is wrong. Rejecting it inside extractComponent replaced that with the vaguer "No component exported". Restore the original default handling and keep the change to the named fallback, which is what the issue is about. Callers that need a stricter contract already enforce it and can report it better than this function can. Also prefer a function over an object when scanning named exports, so a module pairing data with a component, such as an App Router page exporting metadata, resolves to the component rather than the metadata. Objects are still accepted, since React.memo, React.forwardRef and React.lazy all produce one. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Pushed What broke: Why the test was right and I was wrong: that file does export a default. The failure is "your default export is not a component", and only the caller knows the export is a layout. Moving the rejection earlier traded a specific message for a vaguer one. Fix: default-export handling is now byte-for-byte the original One deliberate addition while narrowing: the fallback now prefers a function export over an object one, so a module exporting App Router Verified locally across the surface the shard covers: |
![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: e61e1d9ec7
ℹ️ 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".
Preferring every function over every object was wrong for a module
exporting an object component ahead of a helper, such as
{ Page: React.memo(...), loader() {} }: the helper won and the memo page
was discarded.
React.memo, React.forwardRef and React.lazy all tag their result with a
well-known symbol on $$typeof, which distinguishes a component object
from an ordinary data export. Functions and tagged objects are therefore
both treated as components and declaration order decides between them,
which restores the original contract for the memo case while still
skipping an App Router metadata object.
An untagged object is neither obviously a component nor obviously not
one, so it stays as a last resort. A module whose only candidate is an
unrecognised component shape still resolves rather than reporting no
component at all.
Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09adc73a11
ℹ️ 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".
A React element carries a symbol-valued $$typeof too, so accepting any tagged object let an exported element win over the module's actual component. An element is a rendered node rather than a component type, so the caller then handed React something it cannot instantiate. Match the tags memo, forwardRef and lazy produce instead. An object carrying any other tag falls to the untagged last-resort tier rather than being selected outright, so an unfamiliar shape still degrades safely. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5357c2cb6f
ℹ️ 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".
Context and provider objects are renderable React types, so leaving them out of the tag set let a later helper function win against one. Added to the whitelist. Object.entries also materialised every export value before the loop ran. A module namespace exposes its exports as getters, and one can throw while a usable component sits further along, as happens with a circular import. Read one key at a time and skip a value that throws. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
@codex review |
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review\n\nPlease review the exact current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbe30c4721
ℹ️ 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".
Fragment, Suspense, StrictMode and Profiler are registered symbols rather
than functions or tagged objects, so a layout that is one lost to a
helper declared after it. Verified against react@19.2.4: all four report
typeof "symbol" with react.* registry keys.
Match on the registry key rather than an enumerated list, so types React
adds later are covered without the list silently falling behind. A bare
symbol cannot be an element, so this does not reopen the element case
REACT_COMPONENT_TAGS exists to exclude, and an unrelated registered
symbol such as Symbol.for("app.marker") is still skipped.
Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
@codex review Please review the exact current head . All existing review threads are resolved. Please report any remaining findings against this SHA. |
1 similar comment
|
@codex review Please review the exact current head . All existing review threads are resolved. Please report any remaining findings against this SHA. |
extractComponent returns React.ComponentType, so comparing its result against an object-literal fixture such as a memo, provider, consumer or client reference fails assertEquals' parameter inference. The suite runs with --no-check, so this only surfaced in lint:test-typecheck. Widen the call result to unknown at the seven affected assertions, which is what the symbol and non-renderable-default cases already do. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bc0b7808b
ℹ️ 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".
Accepting any react.* symbol was wrong. react-is re-exports Memo, ForwardRef, Lazy, ContextProvider and ContextConsumer as the bare symbols React uses for $$typeof, and React rejects every one of them as an element type. A module re-exporting one would have had it selected over its actual component. Verified rather than reasoned: react-is 19.2.4's own isValidElementType returns true for react.fragment, react.suspense, react.strict_mode and react.profiler standing alone, and false for react.memo, react.forward_ref, react.lazy, react.context, react.provider, react.consumer, react.client.reference, react.element and react.portal. Enumerating react@19.2.4's symbol-valued exports gives Fragment, Suspense, StrictMode, Profiler and Activity. Activity is included on React's export rather than react-is, which reports false for it and has evidently not caught up with the 19.2 feature. The bare-symbol check is therefore a whitelist while the object check stays an exclusion list. Component wrappers are open-ended, so excluding rendered nodes is what stays true there; bare element types are a closed set, and everything else is a marker. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review Please review the exact current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962a5cae05
ℹ️ 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".
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
tests (bun) failed on "keeps a React 18 Flight module reference declared before a helper". The failing assertion was isValidElementType18(Page) for a react.module.reference tag, which is gated behind a build-time feature flag in react-is, so Bun resolves a build that answers differently from the one Deno and Node resolve. The assertion characterised react-is rather than this module, and it could not hold across the runtimes CI covers. Remove both react-is imports and the three assertions that only restated library behaviour. What each test exists to check, that extractComponent selects the component and keeps declaration order, is unchanged, and SuspenseList is now the bare symbol it always was. The lockfile is back to matching main. That also closes the standing review request to keep these fixtures dependency-free. Also replace two `value === null` guards with truthiness checks. CodeQL flags the comparison after the typeof narrowing; the behaviour for null, undefined and every non-object primitive is unchanged. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eef848a76
ℹ️ 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".
|
@codex review Please re-review the exact current head |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
3 participants
Description
extractComponentfell back to the first key of the module namespace when there was no default export:For a transpiled CommonJS namespace shaped
{ __esModule: true, Named },moduleObj.defaultisundefinedandfirstKeyis"__esModule", so the extracted "component" was the booleantrue. That is what many transpilers emit for a module with only named exports, so it is not an exotic input.trueis truthy, so the existing guard let it through. The caller receivedtruewhere it expected a component and rendering failed later and further from the cause.extractComponentis reached from bothloadComponentFromSourceand the SSR module loader, so this is on the core render path.Fix
Select the first export that can actually be rendered: a function (function or class component) or an object, which is what
React.memo,React.forwardRefandReact.lazyproduce. The__esModulemarker is skipped explicitly.Two consequences worth calling out:
{ default: true, Named }returnedtrue; it now returnsNamed.No component exported from <file>error at the point of extraction, which is where the diagnosis belongs.Object exports are deliberately accepted rather than restricted to functions, because rejecting them would break
memo/forwardRef/lazycomponents.Related Issue(s)
Fixes #4087
Type of Change
Checklist
Testing
Five cases added to
extract-component.test.ts, all verified to fail before the fix and pass after: the__esModulefallback, a marker-only namespace, primitive exports declared ahead of the component, amemo-shaped object component, and an unrenderable default alongside a usable named export. The four pre-existing tests are unchanged and still pass.Summary by CodeRabbit