chore: Enable meta modal widgets to render in canvas#39652
chore: Enable meta modal widgets to render in canvas#39652ashit-rath merged 4 commits intoreleasefrom
Conversation
WalkthroughThis pull request updates both rendering and widget retrieval logic. In the fixed layout canvas components, the dependency arrays for useMemo hooks now include the Changes
Sequence Diagram(s)sequenceDiagram
participant S as Saga
participant SEL as Selector (getWidgetByName)
participant ST as State (widgets & metaWidgets)
S->>SEL: Request widget by name
SEL->>ST: Retrieve widget data
ST-->>SEL: Return matching widget (or null)
SEL-->>S: Return widget
S->>S: Proceed with show/close modal logic
sequenceDiagram
participant C as Canvas Component
participant M as useMemo Hook
participant R as Render Process
C->>M: Receive new props (including positioning)
M->>M: Re-compute canvasChildren based on updated dependencies
M-->>C: Provide updated children
C->>R: Render updated canvas with new children
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
github-actions
bot
added
Frontend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx (1)
49-51: Consider a fallback fortopRowin sorting.
If any child widget lackstopRow, this sorting could cause unexpected outcomes. A default or fallback might prevent edge cases.app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx (1)
75-78: Check for missingtopRowin sorting.
Like in the viewer canvas, ensure that each child has a validtopRowor consider a fallback to avoid unexpected results.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx(1 hunks)app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx(1 hunks)app/client/src/sagas/ModalSagas.ts(3 hunks)app/client/src/sagas/selectors.tsx(2 hunks)app/client/src/widgets/ModalWidget/widget/index.tsx(3 hunks)app/client/src/widgets/withWidgetProps.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / ci-test / ci-test (48)
- GitHub Check: perform-test / ci-test / ci-test (43)
- GitHub Check: perform-test / ci-test / ci-test (24)
🔇 Additional comments (10)
app/client/src/widgets/withWidgetProps.tsx (1)
97-109: Good implementation to enable modal support in UI modulesThe change correctly extends the
getMetaWidgetChildrenStructureselector call to allow the main container widget to access meta widget children (like modals) even when it doesn't havehasMetaWidgetsexplicitly set to true. The added comments clearly explain the rationale behind this change.app/client/src/widgets/ModalWidget/widget/index.tsx (3)
528-533: Good logic for handling both regular and meta widget childrenThe implementation properly prioritizes meta widget children when available, falling back to regular children. This enables modals to render correctly within UI modules.
543-543: Correctly using the new children referenceThe change to use the dynamically determined
childrenvariable instead of directly accessingthis.props.childrenensures the modal properly renders the correct children structure.
580-580: Interface extension to support new functionalityAdding the
isMetaWidgetflag to the interface is essential for identifying modals that are meta widgets. This property aligns with similar flags in other widget types.app/client/src/sagas/selectors.tsx (2)
45-64: Well-implemented modal widget selectorThis selector efficiently finds modal widgets by name across both regular and meta widget stores. The early return pattern is clean and efficient.
97-112: Good enhancement to widget type selectorThe updated selector now properly searches for widgets of a specific type in both standard and meta widget stores, returning a combined array of widget IDs. This change is essential for the modal widget functionality to work across all contexts.
app/client/src/sagas/ModalSagas.ts (2)
138-141: Improved modal retrieval with specialized selectorUsing the new
getModalWidgetByNameselector instead of the generic widget selector improves code clarity and ensures only modal widgets are returned. The change fromundefinedtonullcheck simplifies the conditional logic that follows.
203-206: Consistent use of specialized modal selectorThe change here mirrors the one in the
showModalByNameSagafunction, ensuring consistent modal widget handling throughout the codebase.app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx (1)
36-46: Good approach for combining metaWidget children.
Mergingprops.metaWidgetChildrenStructurewithprops.childrenis a clean solution to ensure modals from UI modules are rendered.app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx (1)
63-73: Nice addition to combine metaWidget children.
This helps accommodate UI modules that inject modals, keeping the main canvas logic straightforward.
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx
Show resolved
Hide resolved
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
Show resolved
Hide resolved
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
Show resolved
Hide resolved
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
Show resolved
Hide resolved
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/client/src/sagas/selectors.tsx (2)
76-91:⚠️ Potential issueRemove duplicate getWidgetIdsByType implementation
There's a duplication of the
getWidgetIdsByTypefunction in the file. The same function appears twice, once at lines 76-91 and again at lines 97-112. Remove one of these duplicate implementations to avoid confusion and potential issues.- export const getWidgetIdsByType = createSelector( - getWidgets, - getMetaWidgets, - (_state: AppState, widgetType: WidgetType) => widgetType, - (canvasWidgets, metaWidgets, widgetType) => { - const canvasWidgetIds = Object.values(canvasWidgets) - .filter((widget: FlattenedWidgetProps) => widget.type === widgetType) - .map((widget: FlattenedWidgetProps) => widget.widgetId); - - const metaWidgetIds = Object.values(metaWidgets) - .filter((widget: FlattenedWidgetProps) => widget.type === widgetType) - .map((widget: FlattenedWidgetProps) => widget.widgetId); - - return [...canvasWidgetIds, ...metaWidgetIds]; - }, - );Also applies to: 97-112
66-70:⚠️ Potential issueFix incomplete getCanvasAndMetaWidgets implementation
The
getCanvasAndMetaWidgetsselector appears to have a truncated/incomplete implementation. It callscreateSelectorwith input selectors but doesn't provide the transformation function. Also, it has an unmatched closing brace at line 70.export const getCanvasAndMetaWidgets = createSelector( getWidgets, getMetaWidgets, + (widgets, metaWidgets) => { + return { ...widgets, ...metaWidgets }; + } - ); - }; + );
🧹 Nitpick comments (2)
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx (1)
36-46: Consider adding type definition for metaWidgetChildrenStructureWhile TypeScript can infer types from usage, it would be better to explicitly define the type for
props.metaWidgetChildrenStructurein the component props interface for better code documentation and type safety.app/client/src/sagas/selectors.tsx (1)
45-64: Consider refactoring to reuse getWidgetByNameThe implementation works correctly for finding modal widgets, but it duplicates logic from the existing
getWidgetByNamefunction (lines 202-221). Consider refactoring to leverage the existing selector:export const getModalWidgetByName = createSelector( - getWidgets, - getMetaWidgets, + getWidgetByName, (state: AppState, widgetName: string) => widgetName, - (widgets, metaWidgets, widgetName) => { - for (const widget of Object.values(widgets)) { - if (widget.widgetName === widgetName && widget.type === "MODAL_WIDGET") { - return widget; - } - } - - for (const widget of Object.values(metaWidgets)) { - if (widget.widgetName === widgetName && widget.type === "MODAL_WIDGET") { - return widget; - } - } - - return null; + (widget, widgetName) => { + return widget && widget.type === "MODAL_WIDGET" ? widget : null; }, );This would make the code more maintainable and reduce duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx(1 hunks)app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx(1 hunks)app/client/src/sagas/ModalSagas.ts(3 hunks)app/client/src/sagas/selectors.tsx(2 hunks)app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx(1 hunks)app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx(1 hunks)app/client/src/sagas/ModalSagas.ts(3 hunks)app/client/src/sagas/selectors.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
- app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx
- app/client/src/sagas/ModalSagas.ts
- app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutEditorCanvas.tsx
- app/client/src/sagas/selectors.tsx
- app/client/src/sagas/ModalSagas.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (2)
app/client/src/layoutSystems/fixedlayout/canvas/FixedLayoutViewerCanvas.tsx (2)
36-66: Includeprops.positioningin the dependency arrayYou're using
props.positioningin theuseMemocallback but it's not included in the dependencies array. If positioning changes, the memoized value won't update correctly.}, [ props.children, props.shouldScrollContents, props.widgetId, props.componentHeight, props.componentWidth, snapColumnSpace, props.metaWidgetChildrenStructure, + props.positioning, ]);
36-57: Good implementation of meta widget children supportThe changes correctly implement the inclusion of meta widget children structures for modals in UI modules. The approach of combining
props.childrenwithprops.metaWidgetChildrenStructureis clean and maintains the existing rendering logic.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres2_Spec.ts (1)
1-731: 🛠️ Refactor suggestionTest file contains usage of cy.wait() and agHelper.Sleep() which should be avoided.
The test file contains several instances of
cy.wait()(lines 309, 321, 346, 546) andagHelper.Sleep()(lines 181, 183, 184, 358, 380, 389, 444, 458, 536, 563, 572, 584, 591). According to the coding guidelines, these should be avoided in favor of more deterministic waiting strategies.Consider refactoring to use Cypress built-in retry-ability instead:
- Replace
cy.wait('@postExecute')withcy.intercept(...).as('postExecute')followed by assertions that wait for elements to reflect the expected state- Replace
agHelper.Sleep()withcy.get(),cy.find(), or custom commands that wait for specific UI statesFor example:
- agHelper.Sleep(2000); - table.SelectTableRow(0, 0, false, "v2"); + cy.get('[data-cy="table-widget-v2"]').should('be.visible') + .find('tbody tr:first').click();
🛑 Comments failed to post (1)
app/client/src/ce/sagas/moduleInterfaceSagas.ts (1)
17-21:
⚠️ Potential issueGenerator function without yield statement
This function is declared as a generator function (
function*) but doesn't contain anyyieldstatements, which is flagged by static analysis. Since it simply returnsprops.widgets, consider changing it to a regular function or properly implementing it as a generator with yield.-export function* handleModuleWidgetCreationSaga( +export function handleModuleWidgetCreationSaga( props: HandleModuleWidgetCreationSagaPayload, ) { return props.widgets; }📝 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 handleModuleWidgetCreationSaga( props: HandleModuleWidgetCreationSagaPayload, ) { return props.widgets; }🧰 Tools
🪛 Biome (1.9.4)
[error] 17-21: This generator function doesn't contain yield.
(lint/correctness/useYield)
## Description
This PR adds support for modals within UI modules. Modals within UI
modules are created as meta widgets and modal meta widgets were not
supported.
### Changes
1. Canvas Rendering Enhancement:
- Modified FixedLayoutEditorCanvas and FixedLayoutViewerCanvas to
include meta widget children in the canvas rendering
2. Modal Widget Improvements:
- Updated ModalWidget to handle meta widget children structures
3. Selectors:
- Created new selector getModalWidgetByName to find modals in both
regular widgets and meta widgets
- Enhanced getWidgetIdsByType to include meta widgets in the search
- Updated modal-related sagas to use the new selectors
4. Main Container Widget Support:
- Modified withWidgetProps to allow the main container widget to access
meta widget children even when hasMetaWidgets is false
Fixes appsmithorg#39287
## Automation
/ok-to-test tags="@tag.All"
### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/13829067577>
> Commit: 15cde00
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13829067577&attempt=2"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.All`
> Spec:
> <hr>Thu, 13 Mar 2025 11:09:59 UTC
<!-- end of auto-generated comment: Cypress test results -->
## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Improved canvas rendering responsiveness, ensuring interface elements
update correctly with layout changes.
- Enhanced modal display behavior by refining widget retrieval,
resulting in more consistent interactions.
- **New Features**
- Introduced an optional modal configuration to better support
alternative widget structures.
- Updated child component handling to accommodate new meta widget
conditions within the main interface.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
This PR adds support for modals within UI modules. Modals within UI modules are created as meta widgets and modal meta widgets were not supported.
Changes
Canvas Rendering Enhancement:
Modal Widget Improvements:
Selectors:
Main Container Widget Support:
Fixes #39287
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13829067577
Commit: 15cde00
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 13 Mar 2025 11:09:59 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
New Features