Repository navigation
Fix: emit one module per file so subpath imports stay small - #194
Conversation
ui/checkbox, ui/dialog and ui/tabs imported `cn` from the `ui` barrel, which formed a cycle through data-table-filter. Rollup resolved it by merging them into a shared filter-selector chunk, so importing a checkbox loaded the calendar, react-day-picker, date-fns and input-otp. - Move `cn` into ui/utils/cn and import it directly everywhere, so leaf components no longer pull zod, react-router or filter sync helpers. - Build with preserveModules (no hashed shared chunks) and externalize every declared dependency/peer instead of a hand-maintained list. Undeclared bundled code goes to dist/_vendor, since npm strips nested node_modules from tarballs. - Declare "sideEffects": false so barrel imports tree-shake. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Extract cn helper packages/components/src/ui/utils/*, packages/components/src/ui/*.tsx, packages/components/src/ui/data-table*/** |
The cn implementation is in its own module. The utilities barrel re-exports it, and component imports point directly to the module. |
Configure package build output packages/components/vite.config.ts, packages/components/package.json, .changeset/per-module-build.md |
The Vite build externalizes dependencies and preserves source modules. Package metadata sets sideEffects to false, and the changeset records a patch release and describes the build changes. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested reviewers: jaruesink
Merge Risk: 
7ab18
-
The reviewed build and module changes present no confirmed merge-blocking risk.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "Fix: emit one module per file so subpath imports stay small" matches the main change. The PR switches the Vite build to preserveModules so each source file emits its own module. The stated … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "Fix: emit one module per file so subpath imports stay small" matches the main change. The PR switches the Vite build to preserveModules so each source file emits its own module. The stated goal is that subpath imports such as ui/checkbox.js, ui/dialog.js, and ui/tabs.js stop loading unrelated shared chunks. The title is specific and conveys the primary change. The raw summary also covers supporting work (moving cn to ui/utils/cn.ts, sideEffects false, externalizing dependencies), which the title does not need to list. The word "Fix" is a minor framing choice, since the change reads more like a build restructuring than a bug fix, but this does not make the title misleading.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 38 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks each module’s trail
The class names hop without a fail
The build keeps files in rows
And marks which way each package goes
Then nibbles clover by the code
Comment @coderabbitai help to get the list of available commands.
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
ubuntu-latest now resolves to 26.04, where Playwright 1.55 refuses to install Chromium, so the test job failed before running any tests. Playwright 1.61+ supports 26.04. @storybook/test-runner accepts playwright ^1.14.0, so this is a root devDependency + lockfile bump. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@currybot-lc please review Context:
Worth a close look: |
🍛 Currybot Review — completedReviewed head Outcome: No blocking findings. Merge Confidence: 9/10; recommendation: ready. All 42 changed paths were accounted for. Focused package builds, packing, public-export/declaration compatibility, import-closure, and browser/Node consumer-bundle checks passed. Current-head CI passed 34 suites / 52 tests. An isolated installed-tarball React Router application runtime was not exercised. |
![currybot-lc[bot]](https://avatars.githubusercontent.com/in/2831021?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍛 Currybot Review
📋 Context
PR #194 publishes one module per file so small subpath imports avoid unrelated UI code. This is a full initial review of all 42 changed files at head 8bef5c7d56620da62a54ff68b4d06cb4f242a9c8, against base and merge base e719e3fc769cef2b76d6e2175e24e7f02a822a91. No generated files changed; yarn.lock was checked for dependency declaration consistency without reading its full body.
Related guidance: base AGENTS.md, base Cursor rules, constitution, and tech stack. Ticket and DevAgent plan: N/A; none was supplied or linked.
🔍 Gap Analysis
No findings. There are no prior Currybot findings or review threads. Earlier bot summaries reported no confirmed code findings at an older head. Existing feedback — Addressed by explanation: CodeRabbit’s generic 80% docstring warning is not a base-revision repository gate; the helper implementation is moved unchanged. No code defect remains from that warning.
🧪 Code Quality
No findings. The external predicate matches a declared package exactly or followed by /; a loose prefix could incorrectly treat another package with the same leading name as external. Module preservation emits source modules separately and rewrites nested node_modules paths to _vendor. The packed relative-import closure has zero missing targets, confirming those rewritten paths remain valid in the tarball.
Base comparison and package checks: The base build had 106 JavaScript files: all 102 old non-hash entry paths remain in the head pack; the four absent files are old hash chunks. All 104 base declaration paths remain; the head pack has 105 declarations. Old retained entry export-name sets were unchanged. The head pack contains 140 JavaScript files, including 36 _vendor modules and two new non-vendor files. Package metadata and export mappings resolve to packed targets; packed JavaScript and declaration relative imports have zero missing targets. The patch Changeset is present.
sideEffects: false: The flag is at package.json:36. Source inspection found no CSS imports, required bare imports, or module-level global registration. displayName assignments set local component metadata; 'use client' directives do not perform runtime registration. The built remix-hook-form/data-table-router-form.js does contain bare imports, but their inspected local targets contain no required global effect; declared external dependencies retain their own package side-effect metadata. The 36 packed _vendor modules comprise 34 libphonenumber-js and two @react-stately modules. Their inspected initialization includes local function/prototype setup, with no required bare import, CSS, or global registration found. Browser and Node esbuild bundles from the packed files retain all 34 phone vendor modules when PhoneNumberInput is used and both stately modules when Select is used, through both leaf and ui barrel imports. These checks found no current pruning regression for those exports.
Actual sandbox checks: Immutable Yarn install, focused package build, pack, relative-import and export-target checks passed. All 140 built JavaScript modules imported in Node 24. Focused browser and Node bundles resolved checkbox, dialog, tabs, form checkbox, selected barrel imports, phone input, and select; small leaf imports avoided calendar and data-table-filter inputs. The root Playwright declaration and lockfile agree on 1.64.1.
Head CI, separately: The test job passed 34 suites and 52 tests; quality checks passed lint and package type-check; the preview build passed. Storybook aliases the package to source, so its tests do not establish published-tarball behavior. Author-reported size figures remain claims; the focused consumer bundles support the import-boundary improvement.
Additional checks not performed: an isolated installed-tarball React Router SSR application runtime, feature verification, or E2E QA. Focused Node consumer bundles passed; no SSR defect was demonstrated. The sandbox runtime was Node 24, versus CI’s Node 22.9.0.
🔧 Simplicity Audit
No violations or missing justifications; numerical deduction: 0. The package-derived external predicate and _vendor filename rewrite address the observed build and packing constraints, with explanations at their use sites. The change uses existing Vite/Rollup options without an added abstraction layer.
🌟 Highlights
- Leaf
cnimports remove the UI-barrel dependency from small components;cnremains available through its utility re-export and the UI barrel. - Dependency-derived externals reduce drift from package declarations.
- Packed entry and declaration targets remain intact, and focused bundles retain the vendor code needed by phone input and select.
📊 Merge Confidence: 9/10
Recommendation: ready. Head CI and focused base, build, pack, import-closure, export, and consumer-bundle checks found no blocking issue. A successful isolated installed-tarball React Router SSR consumer check would raise confidence to 10/10 by closing the remaining runtime boundary; it is not a merge blocker on the evidence reviewed.
The dist layout changes and several dependencies move from inlined to external. In 0.x, ^0.23.0 consumers would pick up a patch automatically, so ship it as 0.24.0 to make the upgrade opt-in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
Importing
ui/checkbox.js,ui/dialog.jsorui/tabs.jsfrom the publisheddistloaded the sharedfilter-selector-<hash>.jschunk. That chunk holds the data-table filter,ui/calendar,react-day-picker,date-fnsandinput-otp, so an app that only needed a checkbox shipped the date picker and filter UI too. 360training's checkout pays about 35–40 KB gzip for this on every step.Root cause: those three components imported
cnfrom'../ui', the whole UI barrel (slider.tsxused'../ui/'). That created a cycle (barrel → data-table-filter → filter-selector → checkbox), and Rollup resolved it by merging the modules into one shared chunk. The checkbox code itself lived insidefilter-selector-<hash>.js.Changes
cnmoved tosrc/ui/utils/cn.ts. It is still re-exported fromui/utilsandui, so the public API is unchanged. Every component now imports./utils/cndirectly. Leaf components also stop pulling in theui/utilsbarrel, which broughtzod,react-router'suseSearchParamsand the data-table URL parsers.vite.config.ts):preserveModules+preserveModulesRoot: 'src': one output file per source module and no hash-named shared chunks.hoistTransitiveImports: falsekeeps each file's import list to its direct dependencies.package.jsondependencies+peerDependencies, subpaths included, instead of a hand-maintained list. Before, the list had drifted, socmdk,@radix-ui/react-select,react-tabs,react-sliderandreact-separatorwere inlined, along with a copy ofreact-dompulled in through cmdk's Radix primitives. All of these are declared dependencies, so consumers already install them.libphonenumber-jsandreact-statelyare imported by the source but not declared as dependencies, so they stay bundled as before. They're emitted underdist/_vendor/because npm strips nestednode_modulesdirectories from published tarballs.package.json:"sideEffects": false, so barrel imports likeimport { Button } from '@lambdacurry/forms/ui'can be tree-shaken. Nothing insrchas module-level side effects or CSS imports.distlayout and the set of inlined dependencies change, so^0.23.0consumers opt in rather than getting it automatically.Verification
Import closure from the built
distEvery file each entry loads, following relative imports transitively:
ui/checkbox.jscheckbox.js,utils/cn.js), 1.3 KB / 0.7 KB gzipui/dialog.jsui/tabs.jsremix-hook-form/checkbox.jsui/button.jsAfter the change, the closures of
ui/checkbox,ui/dialogandui/tabscontain no calendar, data-table-filter, command,react-day-picker,date-fns,input-otpor lodash code. Their only external imports are their own Radix primitive,lucide-react(checkbox and dialog),clsx,tailwind-mergeandreact.Consumer bundle (esbuild, minified + gzip, react / react-router / RHF / zod external)
@lambdacurry/forms/ui/checkbox@lambdacurry/forms/ui/dialog@lambdacurry/forms/ui/tabs@lambdacurry/forms/remix-hook-form/checkbox{ Checkbox } from '@lambdacurry/forms/remix-hook-form'{ Button } from '@lambdacurry/forms/ui'These numbers include the third-party code each import pulls in, such as Radix, lucide and tailwind-merge.
Total
distJSBefore: 106 files, 533 KB. After: 140 files, 390 KB. The new files are mostly the per-module
libphonenumber-jscode under_vendor/; the drop comes from no longer inlining declared dependencies.Public API unchanged
.jsentry files from the olddiststill exist at the same paths with identical export names, checked withes-module-lexer. All 104.d.tsfiles are still present. The only additions areui/utils/cn.{js,d.ts},ui/data-table-filter/locales/en.json.jsand_vendor/**.package.jsonexportsare untouched.Repo checks
yarn build: passes, with no empty-chunk warnings.yarn lint: passes.yarn test: 34 suites and 52 tests pass. Storybook resolves the package fromsrc, so this covers thecnimport refactor;distwas verified separately above.yarn workspace @lambdacurry/forms type-check: passes.@lambdacurry/forms-docstype-check fails onmaintoo, with the same 2 errors inselect-alignment.stories.tsx; this PR doesn't touch them.Notes for reviewers
remix-hook-form/data-table-router-form.jsstill has some bareimport "…"lines. They come from importing theui/data-table-filterbarrel, which that module really does depend on, so I left them.@playwright/testto^1.64.1, and with it theplaywrightthat@storybook/test-runnerresolves.ubuntu-latestis now Ubuntu 26.04, and Playwright 1.55 won't install Chromium there, so thetestjob failed before running any tests. That would happen onmaintoo. Playwright 1.61+ supports 26.04. Locally, Chromium installs without any platform override and all 52 tests pass on 1.64.1.🤖 Generated with Claude Code
Summary by CodeRabbit