Register Components for Plugins using Vite#16445
Conversation
|
WalkthroughThis PR adds an automated component registration Vite plugin, wires it into vite.config, removes manual register() wrappers from several components, and renames multiple exported components (including EmptyState variants) to more specific names while updating import sites. ChangesComponent Registration Automation and Naming Standardization
Possibly related PRs
Suggested labelsneeds review, needs testing, needs peer review Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR replaces the manual
Confidence Score: 4/5Safe to merge for existing functionality; the three concerns are quality-of-life issues for developers rather than runtime defects. The plugin logic is sound and the component renames are applied consistently across all import sites. The three flagged issues do not affect production correctness — they affect how legible the transformed output is during debugging and how quickly a developer would notice a naming conflict introduced mid-session. plugins/autoRegisterComponents.ts warrants a second look on the three points raised; all other files are mechanical renames or boilerplate cleanup and look correct. Important Files Changed
Reviews (1): Last reviewed commit: "Register Components for Plugins using Vi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function uniqueName(source: string, preferred: string) { | ||
| let name = preferred; | ||
| let index = 2; | ||
|
|
||
| while (new RegExp(`\\b${name}\\b`).test(source)) { | ||
| name = `${preferred}${index}`; | ||
| index += 1; | ||
| } | ||
|
|
||
| return name; | ||
| } |
There was a problem hiding this comment.
uniqueName matches names in comments and string literals
new RegExp(\b${name}\b).test(source) searches the raw source text, so a word like LoginBase that appears only in a comment or in a string (e.g. "LoginBase") still counts as a collision. This would cause the internal base name to be unnecessarily promoted to LoginBase2, which is harmless at runtime but could confuse developers reading transformed output or stack traces.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const transformed = transformSource(source, normalizedId); | ||
| if (!transformed) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| code: transformed, | ||
| map: null, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Returning map: null from the transform means Vite cannot build a correct source-map chain for any transformed TSX file. Subsequent esbuild transforms will generate maps relative to the already-modified code (with renamed identifiers like LoginBase), so breakpoints and stack traces in production builds will point to these generated names rather than the original source. Given that vite.config.mts has sourcemap: true, this affects every component that is auto-registered. Using a library like magic-string or the TypeScript SourceMapper to emit a real source map would preserve debuggability.
| buildStart() { | ||
| assertUniqueComponentNames(srcRoot, overrideRoot); | ||
| }, |
There was a problem hiding this comment.
assertUniqueComponentNames is called from buildStart, which Vite invokes once when the server initialises. During a running dev session, if a developer creates a new file (or renames a component) that introduces a duplicate registration key, only the transform hook fires — buildStart is not re-triggered. The second register() call silently replaces the base in the registry (see registerComponent in registry.ts), and no error surfaces until the next full server restart. Adding the same uniqueness check inside the transform hook (or in a handleHotUpdate hook) would catch this immediately.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@plugins/autoRegisterComponents.ts`:
- Around line 375-382: The current transform renames the original declaration
identifier to `${exportName}Base` (baseName) which breaks same-file references
and recursive components; instead keep the original declaration name (do not
replace statement.name) and create a separately named registered alias (e.g.,
registeredName = uniqueName(source, `${exportName}Registered`)) that is appended
as an export which calls registerComponent(originalName). Concretely: stop
performing the edits that replace statement.name with baseName, keep calling
removeExportAndDefaultModifiers(sourceFile, statement, edits) to strip export
modifiers, then push an appended edit that adds `export const <registeredName> =
registerComponent(<exportName>);` (and for default exports append `export
default <registeredName>;`) so the original binding remains intact while
exposing the registered alias.
- Around line 466-504: Add fixture-based tests that exercise the main branches
of the auto-register transform: call transform (via the plugin's transform or
directly transformSource) on representative `.tsx` inputs to cover named
exports, `export default Identifier`, modules with duplicate component names (to
trigger assertUniqueComponentNames behavior), and non-component `.tsx` files
that should remain untouched; create small fixture files for each case, assert
the transformed output (or lack of transformation) and that duplicate-name cases
throw or report as expected, and wire these fixtures into the test harness so
the plugin’s transform pipeline is exercised before removing manual wrappers.
In `@src/pages/PublicAppointments/PatientRegistration.tsx`:
- Line 43: The component PublicPatientRegistration is currently a named export;
change it to be the default export for the page (either by declaring it as
"export default function PublicPatientRegistration(...)" or by keeping the
existing function and adding "export default PublicPatientRegistration" at the
end) so the page file follows the project's default-export convention for pages.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 687134f1-67de-4a52-a38b-3bc46bc87312
📒 Files selected for processing (38)
plugins/autoRegisterComponents.tssrc/Routers/PatientRouter.tsxsrc/components/Auth/AuthHero.tsxsrc/components/Auth/Login.tsxsrc/components/CareTeam/CareTeamSheet.tsxsrc/components/Common/PageHeadTitle.tsxsrc/components/Facility/ConsultationDetails/PrintAllQuestionnaireResponses.tsxsrc/components/Facility/ConsultationDetails/PrintQuestionnaireResponse.tsxsrc/components/Medicine/MedicationAdministration/AdministrationTab.tsxsrc/components/Medicine/MedicationRequestTable/index.tsxsrc/components/Patient/Common/EmptyState.tsxsrc/components/Patient/MedicationStatementList.tsxsrc/pages/Admin/organizations/components/AdminOrganizationSelector.tsxsrc/pages/Appointments/BookAppointment/BookAppointmentDetails.tsxsrc/pages/Encounters/tabs/overview/quick-actions.tsxsrc/pages/Encounters/tabs/overview/summary-panel-details-tab/account.tsxsrc/pages/Encounters/tabs/overview/summary-panel-details-tab/department-and-team.tsxsrc/pages/Encounters/tabs/overview/summary-panel-details-tab/discharge-summary.tsxsrc/pages/Encounters/tabs/overview/summary-panel-details-tab/empty-state.tsxsrc/pages/Encounters/tabs/overview/summary-panel-details-tab/encounter-tags.tsxsrc/pages/Encounters/tabs/overview/summary-panel-details-tab/locations.tsxsrc/pages/Encounters/tabs/overview/summary-panel-details-tab/manage-care-team.tsxsrc/pages/Facility/FacilityDetailsPage.tsxsrc/pages/Facility/components/UserCard.tsxsrc/pages/Facility/settings/activityDefinition/ActivityDefinitionList.tsxsrc/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsxsrc/pages/Facility/settings/healthcareService/HealthcareServiceShow.tsxsrc/pages/Facility/settings/locations/LocationSettings.tsxsrc/pages/Facility/settings/locations/LocationSheet.tsxsrc/pages/Facility/settings/locations/LocationView.tsxsrc/pages/Facility/settings/locations/components/LocationCard.tsxsrc/pages/Facility/settings/organizations/components/EditFacilityUserRoleSheet.tsxsrc/pages/Facility/settings/productKnowledge/ProductKnowledgeList.tsxsrc/pages/Facility/settings/productKnowledge/ProductKnowledgeListComponent.tsxsrc/pages/Patient/History/MedicationHistory.tsxsrc/pages/Patient/index.tsxsrc/pages/PublicAppointments/PatientRegistration.tsxvite.config.mts
| const baseName = uniqueName(source, `${exportName}Base`); | ||
|
|
||
| removeExportAndDefaultModifiers(sourceFile, statement, edits); | ||
| edits.push({ | ||
| start: statement.name.getStart(sourceFile), | ||
| end: statement.name.end, | ||
| text: baseName, | ||
| }); |
There was a problem hiding this comment.
Preserve the original component binding when generating the registered export.
Renaming Foo to FooBase changes every same-file reference to Foo. Recursive components stop referring to their own implementation, and any top-level reference that executes before the appended wrapper is initialized will hit the temporal dead zone. Keep the original declaration name and export a separately named registered alias instead.
♻️ Safer rewrite shape
- function FooBase(...) { ... }
- export const Foo = __careRegisterComponent("Foo", FooBase);
+ function Foo(...) { ... }
+ const FooRegistered = __careRegisterComponent("Foo", Foo);
+ export { FooRegistered as Foo };For default exports, use the same pattern with export default FooRegistered;.
Also applies to: 414-423
🤖 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 `@plugins/autoRegisterComponents.ts` around lines 375 - 382, The current
transform renames the original declaration identifier to `${exportName}Base`
(baseName) which breaks same-file references and recursive components; instead
keep the original declaration name (do not replace statement.name) and create a
separately named registered alias (e.g., registeredName = uniqueName(source,
`${exportName}Registered`)) that is appended as an export which calls
registerComponent(originalName). Concretely: stop performing the edits that
replace statement.name with baseName, keep calling
removeExportAndDefaultModifiers(sourceFile, statement, edits) to strip export
modifiers, then push an appended edit that adds `export const <registeredName> =
registerComponent(<exportName>);` (and for default exports append `export
default <registeredName>;`) so the original binding remains intact while
exposing the registered alias.
| export function autoRegisterComponents(): Plugin { | ||
| let srcRoot = ""; | ||
| let overrideRoot = ""; | ||
|
|
||
| return { | ||
| name: "auto-register-components", | ||
| enforce: "pre", | ||
| configResolved(config) { | ||
| srcRoot = `${normalizePath(path.resolve(config.root, "src"))}/`; | ||
| overrideRoot = `${normalizePath( | ||
| path.resolve(config.root, "src/lib/override"), | ||
| )}/`; | ||
| }, | ||
| buildStart() { | ||
| assertUniqueComponentNames(srcRoot, overrideRoot); | ||
| }, | ||
| transform(source, id) { | ||
| const normalizedId = normalizePath(id.split("?")[0]); | ||
|
|
||
| if ( | ||
| !normalizedId.endsWith(".tsx") || | ||
| !normalizedId.startsWith(srcRoot) || | ||
| normalizedId.startsWith(overrideRoot) | ||
| ) { | ||
| return null; | ||
| } | ||
|
|
||
| const transformed = transformSource(source, normalizedId); | ||
| if (!transformed) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| code: transformed, | ||
| map: null, | ||
| }; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add fixture coverage for the transform before more manual wrappers are removed.
This plugin now rewrites export semantics for every eligible .tsx module under src, but the PR does not include specs for the main branches: named exports, export default Identifier, duplicate-name failures, and untouched non-component modules. A small fixture-based suite would materially reduce regression risk here.
🤖 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 `@plugins/autoRegisterComponents.ts` around lines 466 - 504, Add fixture-based
tests that exercise the main branches of the auto-register transform: call
transform (via the plugin's transform or directly transformSource) on
representative `.tsx` inputs to cover named exports, `export default
Identifier`, modules with duplicate component names (to trigger
assertUniqueComponentNames behavior), and non-component `.tsx` files that should
remain untouched; create small fixture files for each case, assert the
transformed output (or lack of transformation) and that duplicate-name cases
throw or report as expected, and wire these fixtures into the test harness so
the plugin’s transform pipeline is exercised before removing manual wrappers.
| }; | ||
|
|
||
| export function PatientRegistration(props: PatientRegistrationProps) { | ||
| export function PublicPatientRegistration(props: PatientRegistrationProps) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use a default export for this page component.
Line 43 exports this page as a named export, but page files should default-export the page component for consistency.
Proposed fix
-export function PublicPatientRegistration(props: PatientRegistrationProps) {
+export default function PublicPatientRegistration(props: PatientRegistrationProps) {- import { PublicPatientRegistration } from "`@/pages/PublicAppointments/PatientRegistration`";
+ import PublicPatientRegistration from "`@/pages/PublicAppointments/PatientRegistration`";As per coding guidelines, src/pages/**/*.{ts,tsx} should "Export page components as default exports".
📝 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.
| export function PublicPatientRegistration(props: PatientRegistrationProps) { | |
| export default function PublicPatientRegistration(props: PatientRegistrationProps) { |
🤖 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 `@src/pages/PublicAppointments/PatientRegistration.tsx` at line 43, The
component PublicPatientRegistration is currently a named export; change it to be
the default export for the page (either by declaring it as "export default
function PublicPatientRegistration(...)" or by keeping the existing function and
adding "export default PublicPatientRegistration" at the end) so the page file
follows the project's default-export convention for pages.
Source: Coding guidelines
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #9504 |
Deploying care-preview with
|
| Latest commit: |
742189d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dbde32ce.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://auto-register-vite.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/autoRegisterComponents.ts (1)
446-454:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not rename the original component binding during transform.
Line 450 and Line 492 rename declarations to
...Base, which mutates same-file symbol references and can cause TDZ/runtime failures for references expecting the original exported name. Keep the original declaration name and export a separately named registered alias.♻️ Safer transform shape
- removeExportAndDefaultModifiers(sourceFile, statement, edits); - edits.push({ - start: statement.name.getStart(sourceFile), - end: statement.name.end, - text: baseName, - storeName: true, - }); - edits.push({ - start: statement.end, - end: statement.end, - text: `\nexport const ${exportName} = ${REGISTER_ALIAS}(${JSON.stringify(exportName)}, ${baseName});`, - }); + removeExportAndDefaultModifiers(sourceFile, statement, edits); + const registeredName = uniqueName(source, `${exportName}Registered`); + edits.push({ + start: statement.end, + end: statement.end, + text: `\nconst ${registeredName} = ${REGISTER_ALIAS}(${JSON.stringify(exportName)}, ${exportName});\nexport { ${registeredName} as ${exportName} };`, + });Also applies to: 490-496
🤖 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 `@plugins/autoRegisterComponents.ts` around lines 446 - 454, The transform currently renames the original declaration to `${exportName}Base` by replacing `statement.name` (via `baseName` from `uniqueName`) which mutates same-file symbol references and can create TDZ/runtime failures; instead, keep the original declaration name intact (do not modify `statement.name`) and create a separately named exported alias/registration using `baseName` (e.g., emit a new exported const/assignment or export binding that references the original identifier). Update the code paths that call `uniqueName`, `removeExportAndDefaultModifiers`, and push into `edits` so they remove export/default modifiers but do NOT replace `statement.name`; add a new edit that inserts an exported alias declaration or export statement for `baseName` referring to the original symbol. Apply the same change to the other occurrence (the block around lines 490-496) so both places keep the original binding and export a separately named alias.
🤖 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.
Duplicate comments:
In `@plugins/autoRegisterComponents.ts`:
- Around line 446-454: The transform currently renames the original declaration
to `${exportName}Base` by replacing `statement.name` (via `baseName` from
`uniqueName`) which mutates same-file symbol references and can create
TDZ/runtime failures; instead, keep the original declaration name intact (do not
modify `statement.name`) and create a separately named exported
alias/registration using `baseName` (e.g., emit a new exported const/assignment
or export binding that references the original identifier). Update the code
paths that call `uniqueName`, `removeExportAndDefaultModifiers`, and push into
`edits` so they remove export/default modifiers but do NOT replace
`statement.name`; add a new edit that inserts an exported alias declaration or
export statement for `baseName` referring to the original symbol. Apply the same
change to the other occurrence (the block around lines 490-496) so both places
keep the original binding and export a separately named alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fb68afa-a6bb-4e81-b071-c98627077d52
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
docs/care-apps-plugin-overrides.mdpackage.jsonplugins/autoRegisterComponents.tsvite.config.mts
There was a problem hiding this comment.
Pull request overview
This PR introduces build-time auto-registration of React components for the CARE plugin override system via a custom Vite plugin, removing the need for manual register() wrappers and enforcing unique exported component names across the codebase.
Changes:
- Added a custom Vite plugin (
autoRegisterComponents) that scans and transforms exported TSX components into override-registered components, with validation for duplicate/unknown component names. - Removed manual component registration wrappers and renamed several exported components to ensure global uniqueness.
- Updated routing/components to align with the renamed exports and clarified public vs patient-portal components.
Reviewed changes
Copilot reviewed 40 out of 41 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vite.config.mts | Wires the new auto-registration Vite plugin and adds parsing for an allowlist env var. |
| src/Routers/PatientRouter.tsx | Uses the renamed public patient registration component export. |
| src/pages/PublicAppointments/PatientRegistration.tsx | Renames exported component to PublicPatientRegistration for uniqueness/clarity. |
| src/pages/Patient/index.tsx | Renames the internal component function to a unique portal-specific name. |
| src/pages/Patient/History/MedicationHistory.tsx | Updates import to use the newly renamed medication empty state export. |
| src/pages/Facility/settings/productKnowledge/ProductKnowledgeListComponent.tsx | Renames exported component to a unique *Content name. |
| src/pages/Facility/settings/productKnowledge/ProductKnowledgeList.tsx | Updates usage/import to match renamed product knowledge component. |
| src/pages/Facility/settings/organizations/components/EditFacilityUserRoleSheet.tsx | Renames default-exported sheet component to avoid collisions. |
| src/pages/Facility/settings/locations/LocationView.tsx | Updates import alias to match renamed settings LocationCard export. |
| src/pages/Facility/settings/locations/LocationSheet.tsx | Renames default-exported sheet component for uniqueness. |
| src/pages/Facility/settings/locations/LocationSettings.tsx | Updates import alias to match renamed settings LocationCard export. |
| src/pages/Facility/settings/locations/components/LocationCard.tsx | Renames exported card component to SettingsLocationCard. |
| src/pages/Facility/settings/healthcareService/HealthcareServiceShow.tsx | Renames default export function for uniqueness. |
| src/pages/Facility/settings/activityDefinition/ActivityDefinitionListComponent.tsx | Renames exported component to a unique *Content name. |
| src/pages/Facility/settings/activityDefinition/ActivityDefinitionList.tsx | Updates usage/import to match renamed activity definition component. |
| src/pages/Facility/FacilityDetailsPage.tsx | Updates import alias to match renamed facility user card export. |
| src/pages/Facility/components/UserCard.tsx | Renames exported component to FacilityUserCard to avoid collisions. |
| src/pages/Encounters/tabs/overview/summary-panel-details-tab/manage-care-team.tsx | Switches to renamed summary-panel empty state export. |
| src/pages/Encounters/tabs/overview/summary-panel-details-tab/locations.tsx | Switches to renamed summary-panel empty state export. |
| src/pages/Encounters/tabs/overview/summary-panel-details-tab/encounter-tags.tsx | Switches to renamed summary-panel empty state export. |
| src/pages/Encounters/tabs/overview/summary-panel-details-tab/empty-state.tsx | Renames exported empty state component to SummaryPanelEmptyState. |
| src/pages/Encounters/tabs/overview/summary-panel-details-tab/discharge-summary.tsx | Switches to renamed summary-panel empty state export. |
| src/pages/Encounters/tabs/overview/summary-panel-details-tab/department-and-team.tsx | Switches to renamed summary-panel empty state export. |
| src/pages/Encounters/tabs/overview/summary-panel-details-tab/account.tsx | Switches to renamed summary-panel empty state export. |
| src/pages/Encounters/tabs/overview/quick-actions.tsx | Removes manual registration wrapper and exports QuickAction directly for auto-registration. |
| src/pages/Appointments/BookAppointment/BookAppointmentDetails.tsx | Removes manual registration wrapper and exports component directly for auto-registration. |
| src/pages/Admin/organizations/components/AdminOrganizationSelector.tsx | Renames default export function for uniqueness. |
| src/components/Patient/MedicationStatementList.tsx | Updates import to use the newly renamed medication empty state export. |
| src/components/Patient/Common/EmptyState.tsx | Renames default export function to avoid name collisions. |
| src/components/Medicine/MedicationRequestTable/index.tsx | Renames exported EmptyState to MedicationRequestEmptyState and updates props interface. |
| src/components/Medicine/MedicationAdministration/AdministrationTab.tsx | Updates import to use the newly renamed medication empty state export. |
| src/components/Facility/ConsultationDetails/PrintQuestionnaireResponse.tsx | Updates named imports to use renamed printable components. |
| src/components/Facility/ConsultationDetails/PrintAllQuestionnaireResponses.tsx | Renames exported printable components for uniqueness and updates usage. |
| src/components/Common/PageHeadTitle.tsx | Renames default export function for uniqueness/consistency. |
| src/components/CareTeam/CareTeamSheet.tsx | Renames local exported empty state component to CareTeamEmptyState. |
| src/components/Auth/Login.tsx | Removes manual registration wrapper and exports component directly for auto-registration. |
| src/components/Auth/AuthHero.tsx | Removes manual registration wrapper and exports component directly for auto-registration. |
| plugins/autoRegisterComponents.ts | Adds the Vite transform + validation logic for automatic component registration. |
| package.json | Adds magic-string dependency for source-preserving transforms and sourcemaps. |
| package-lock.json | Locks the added magic-string dependency. |
| docs/care-apps-plugin-overrides.md | Documents the new build-time registration approach, allowlist env var, and override flow. |
| export function QuickAction({ | ||
| icon, | ||
| title, | ||
| actionId, |
bodhish
left a comment
bodhish
left a comment
There was a problem hiding this comment.
Reviewed the full diff, the runtime register() wrapper, and traced the transform's AST handling against real files (including the scary case — wrapping the src/components/ui Radix/forwardRef library). Overall this is a solid, well-engineered change: enforce: "pre", MagicString hi-res sourcemaps, directive-aware import insertion, and render-time references stay TDZ-safe. The forwardRef catastrophe is avoided because shadcn primitives use export { ... } blocks, which the transform deliberately doesn't touch. Nice.
A few things worth resolving before merge. Inline comments on the specific lines; summary here.
Worth a decision
1. The default registers every component — opposite of stated goal #4. With REACT_MFE_REGISTERED_COMPONENTS unset/empty/*, include is null and every inline-exported PascalCase component (~780 of them) gets wrapped — in dev, CI, and any prod deploy that doesn't set the env var. Goal #4 was "avoid registering every exported component when deployments only need a small override surface," but the default does exactly that. Each wrapper adds a React boundary + 3 useContext reads per rendered instance, which the docs admit "compounds in dense lists or tables" — and this is an EMR with large patient/medication tables. Consider defaulting to an empty allowlist (opt-in). (plugin shouldRegisterComponent)
2. The global-uniqueness gate ignores the allowlist. assertUniqueComponentNames(targets) runs over all discovered components, not just include. So even a deploy that registers 2 components must keep every exported PascalCase component name unique across the whole repo, forever — the permanent tax behind the 38 renames in this PR, inherited by every future contributor. Registry keys only need to be unambiguous among registered components; when include is set, scope the check to include. (plugin buildStart)
Gaps
3. No tests for a 589-line build-critical transform. Zero unit tests for a plugin that rewrites every component in the app, and the "Add specs" merge-checklist item is unchecked. The project already runs vitest (jsdom). This is the highest-leverage ask — cover each branch: export function, single-declarator export const X =, export default / export default function, the multi-declarator + export {}-block skips, allowlist filtering, duplicate detection, and unknown-name rejection. Without it, the edge cases below are unguarded.
4. Export-form coverage is partial and silent. Only three forms are handled (export function, single-declarator export const X =, export default/export default function). export { Foo } blocks, export { X as Y }, multi-declarator export const A = …, B = …, and re-exports are all silently skipped. That's why src/components/ui is safe — but it also means those components can't be overridden, and an allowlisted name that resolves to one of these fails the build with no hint why. Emit a warning when an allowlisted name resolves to an untransformable export form, and document the supported forms. (plugin transformSource)
Notes (low severity)
register()isn't ref-forwarding (src/lib/override/register.ts:79—function RegisteredComponent(props), notforwardRef). Fine today because every transformed component is a plain function, but it's a latent trap: the first time someone inline-exports aforwardRef/ref-bearingmemocomponent, registration silently drops the ref. Worth skippingforwardRef/memoinitializers inisComponentInitializer, or at least a comment.Component.displayName = "X"now lands on the wrapper (e.g.MultiSelect), overwritingregister'sRegistered(MultiSelect). Cosmetic, no crash — registration is inserted before the assignment line.magic-string@^0.27.0pinned as a direct dep while Vite already ships a newer magic-string — possible duplicate in the tree. Match Vite's version or rely on the transitive one.- Doc staleness: the new
docs/care-apps-plugin-overrides.mdis good, but the JSDoc insrc/lib/override/register.tsstill teaches the obsolete manualexport default register(...)pattern.
Verdict: not a happy-path correctness blocker, but I'd want #1 (default) and #3 (tests) sorted before merge; #2 and #4 are cheap to fix while you're in here.
| name: string, | ||
| include: ReadonlySet<string> | null | undefined, | ||
| ) { | ||
| return !include || include.has(name); |
There was a problem hiding this comment.
Default registers every component. With REACT_MFE_REGISTERED_COMPONENTS unset/empty/*, include is null and this returns true for everything — so all ~780 inline-exported components get wrapped by default (dev, CI, and any prod deploy that doesn't set the env var). That's the opposite of goal #4 ("avoid registering every exported component when deployments only need a small override surface"), and each wrapper adds a component boundary + 3 useContext reads per rendered instance, which compounds in the large patient/medication tables. Consider defaulting to an empty allowlist (opt-in) so the override cost is only paid where wanted.
| }, | ||
| buildStart() { | ||
| const targets = collectRegisteredComponentTargets(srcRoot, overrideRoot); | ||
| assertUniqueComponentNames(targets); |
There was a problem hiding this comment.
Uniqueness gate ignores the allowlist. This runs over all discovered components, not just include. So a deploy that registers 2 components still must keep every exported PascalCase component name globally unique forever — the permanent tax behind the 38 renames in this PR, inherited by every future contributor (name a new component EmptyState anywhere → build fails). Registry keys only need to be unambiguous among registered components; when include is set, scope the uniqueness check to include.
| if ( | ||
| isExported && | ||
| ts.isVariableStatement(statement) && | ||
| statement.declarationList.declarations.length === 1 |
There was a problem hiding this comment.
Only 3 export forms are handled here (export function, single-declarator export const X =, export default/export default function). export { Foo } blocks, export { X as Y }, multi-declarator export const A = …, B = …, and re-exports are silently skipped — which is why src/components/ui is safe, but also means those components can't be overridden, and an allowlisted name that resolves to one of these fails the build with no hint why. Worth a warning when an allowlisted name resolves to an untransformable export form, plus a doc note on supported forms.
Proposed Changes
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Refactor
Chores