refactor(core): split widget ui factories by family - #276
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors the monolithic widget factory by extracting implementations into multiple focused factory modules (basic, feedback, helpers, interactive, layoutShell, media, navigation, advanced) and re-exports them from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/core/src/widgets/factories/interactive.ts (1)
18-26: Redundant function overload signatures.Lines 18 and 23 declare overload signatures identical to their implementations. These overloads serve no purpose since they don't provide additional type narrowing.
🧹 Proposed cleanup
-export function button(props: ButtonProps): VNode; export function button(props: ButtonProps): VNode { return { kind: "button", props: resolveButtonIntent(props) }; } -export function input(props: InputProps): VNode; export function input(props: InputProps): VNode { return { kind: "input", props }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/factories/interactive.ts` around lines 18 - 26, Remove the redundant identical overload declarations for the button and input functions: delete the standalone overload signatures that duplicate the implementations and leave only the concrete function implementations (the definitions of button and input that return VNode). Update references to the resolveButtonIntent call remain inside the button implementation and ensure ButtonProps/InputProps and return type VNode are preserved on the remaining function declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/widgets/factories/advanced.ts`:
- Around line 24-45: In dialog(), action IDs are generated using modalProps.id
which can be undefined causing "undefined-action-X" IDs; update the ID
generation in the actions.map callback (inside dialog) to use a safe base ID
(e.g. use props.id or modalProps.id if present, otherwise a deterministic
fallback like `dialog` or a short generated unique prefix) before appending
`-action-${index}`, so action id becomes `${baseId}-action-${index}` instead of
`${modalProps.id}-action-${index}` to avoid "undefined" IDs and collisions when
no parent id is supplied.
- Around line 79-92: Change the children parameter type of splitPane,
panelGroup, and resizablePanel from readonly VNode[] to readonly UiChild[] to
match other container factory APIs; update the function signatures for
splitPane(props: SplitPaneProps, children: readonly UiChild[] = []),
panelGroup(props: PanelGroupProps, children: readonly UiChild[] = []), and
resizablePanel(props: ResizablePanelProps = {}, children: readonly UiChild[] =
[]), and ensure any necessary UiChild import is present so these factories
accept conditional/nested children like the ones in basic.ts, layoutShell.ts,
and interactive.ts.
In `@packages/core/src/widgets/factories/layoutShell.ts`:
- Around line 120-131: The page() factory currently only forwards p when
provided, allowing calls to ui.page(...) to omit outer padding and violate the
root padding invariant; modify the returned props in page(options) so that p
defaults to 1 when not supplied (e.g., use p: p ?? 1 or spread a fallback when p
is undefined) inside the object returned by the page function so root views
always have minimum outer padding; update the code around the page function's
return (the object that currently conditionally spreads p) to apply this
default.
- Around line 253-263: The button id generation in layoutShell.ts (inside the
buttonNodes mapping using options.id and item.id) can produce duplicate
interactive IDs when options.id is undefined and the fallback "sidebar" is
reused; change the logic so each sidebar instance gets a unique prefix (e.g.,
generate a per-instance uniqueId when options.id is falsy) and use that unique
prefix in place of the static "sidebar" fallback so the constructed id
`${prefix}-${item.id}` is always unique across instances; update any relevant
initialization (where layoutShell creates the widget instance) to create/store
the per-instance prefix and reference it in the buttonNodes mapping.
---
Nitpick comments:
In `@packages/core/src/widgets/factories/interactive.ts`:
- Around line 18-26: Remove the redundant identical overload declarations for
the button and input functions: delete the standalone overload signatures that
duplicate the implementations and leave only the concrete function
implementations (the definitions of button and input that return VNode). Update
references to the resolveButtonIntent call remain inside the button
implementation and ensure ButtonProps/InputProps and return type VNode are
preserved on the remaining function declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 211c78cf-ee4a-4fec-ac75-8621bf34f30f
📒 Files selected for processing (9)
packages/core/src/widgets/factories/advanced.tspackages/core/src/widgets/factories/basic.tspackages/core/src/widgets/factories/feedback.tspackages/core/src/widgets/factories/helpers.tspackages/core/src/widgets/factories/interactive.tspackages/core/src/widgets/factories/layoutShell.tspackages/core/src/widgets/factories/media.tspackages/core/src/widgets/factories/navigation.tspackages/core/src/widgets/ui.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/widgets/factories/layoutShell.ts`:
- Around line 252-257: The new throw in sidebar() creates a breaking runtime
contract; make it backward-compatible by removing the hard throw and providing a
deterministic fallback when options.id and options.key are missing: update the
idPrefix logic inside the sidebar function to assign a stable generated id
(e.g., module-level counter like sidebarIdCounter -> `sidebar-${counter}`) when
both options.id and options.key are absent, and emit a console.warn or
logger.warn indicating a generated id was used; ensure the rest of the code
continues to use idPrefix for unique item ids (reference symbols: sidebar,
SidebarOptions, idPrefix) so existing callers won’t break.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e1cbf63-be4f-4d12-bc48-2486ce88c2e7
📒 Files selected for processing (5)
packages/core/src/widgets/__tests__/compositionWidgets.test.tspackages/core/src/widgets/factories/advanced.tspackages/core/src/widgets/factories/interactive.tspackages/core/src/widgets/factories/layoutShell.tspackages/jsx/src/__tests__/parity.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/widgets/factories/advanced.ts
- packages/core/src/widgets/factories/interactive.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/core/src/widgets/ui.tsinto internal family modules.uiAPI unchanged.Why
Validation
npm run lintnpm run typechecknpm run buildnode scripts/run-tests.mjs --filter "packages/core/dist/widgets/__tests__/"node scripts/run-tests.mjs --filter "packages/jsx/dist/__tests__/parity"Summary by CodeRabbit
New Features
Refactor
Bug Fixes