chore: fix blueprint operation inconsistency for zone#36980
chore: fix blueprint operation inconsistency for zone#36980
Conversation
WalkthroughThe changes in this pull request involve renaming parameters for clarity in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/layoutSystems/anvil/utils/layouts/update/sectionUtils.ts (1)
Line range hint
62-62: Consider renaming parameter in splitWidgets for consistencyTo maintain consistency with the recent changes, consider renaming the
widgetsparameter in thesplitWidgetsfunction todraggedWidgets.Here's the suggested change:
-export function splitWidgets(widgets: WidgetLayoutProps[]): WidgetLayoutProps[][] { +export function splitWidgets(draggedWidgets: WidgetLayoutProps[]): WidgetLayoutProps[][] {Don't forget to update the function body to use the new parameter name.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/layoutSystems/anvil/utils/layouts/update/sectionUtils.ts (2 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (4 hunks)
- app/client/src/sagas/WidgetBlueprintSagas.ts (1 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts
[error] 87-87: children is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
🔇 Additional comments (9)
app/client/src/layoutSystems/anvil/utils/layouts/update/sectionUtils.ts (3)
22-22: Improved parameter naming in createSectionAndAddWidgetThe renaming of
widgetstodraggedWidgetsenhances code clarity. It now explicitly indicates that the function deals with dragged widgets.
51-51: Consistent parameter naming in addWidgetsToSectionThe parameter renaming from
widgetstodraggedWidgetsin this function maintains consistency with the changes made increateSectionAndAddWidget. This improves overall code readability.
Line range hint
1-240: Overall assessment: Improved naming conventionsThe changes in this file enhance code clarity by consistently renaming parameters to better reflect their purpose. The modifications are straightforward and do not alter the logic of the functions. With the suggested update to the
splitWidgetsfunction, the file will maintain a high level of consistency in its naming conventions.app/client/src/sagas/WidgetBlueprintSagas.ts (1)
137-140: Improved immutability in state updates.The change enhances the immutability of state updates by using the spread operator. This aligns with Redux best practices and functional programming principles.
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (5)
61-61: Function call updated correctlyPassing
zoneProps.widgetIdtoaddWidgetsToZonealigns with the updated function signature.
71-71: Function signature modified appropriately
addWidgetsToZonenow takeszoneWidgetIdinstead of the entirezoneobject, simplifying the function parameters.
74-74: Accessing layout viazoneWidgetIdUsing
updatedWidgets[zoneWidgetId].layoutis consistent with the parameter change.
128-128: Updating zone layout correctlyAssigning the updated
zoneLayoutback toupdatedWidgets[zoneWidgetId].layoutlooks good.
131-132: Return statement updated appropriatelyReturning
updatedWidgets[zoneWidgetId]as thezonesimplifies the data handling.
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts
Outdated
Show resolved
Hide resolved
| draggedWidgets, | ||
| ); | ||
| zoneProps.children = updatedWidgets[zoneWidgetId].children; | ||
| updatedWidgets[zoneWidgetId].children = updatedWidgets[zoneWidgetId].children; |
There was a problem hiding this comment.
the updateDraggedWidgets can update the zone in the blueprint operation as well, but we didn't use updatedWidgets's zone. We just worked on the zone that we got in params.
This is the source of inconsistency.
| updatePropertyPayloads.forEach((params: UpdatePropertyArgs) => { | ||
| widgets[params.widgetId][params.propertyName] = | ||
| params.propertyValue; | ||
| widgets[params.widgetId] = { |
There was a problem hiding this comment.
For cases where widgets are already present in redux, the objects are frozen and the fields are readonly. So we couldn't update them. used the spread operator to bypass that. Hoping this won't cause any issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (1)
Line range hint
112-112: UseupdatedWidgetsinstead ofallWidgetswhen adding new widgets.In the
updateDraggedWidgetsfunction, you're passingallWidgetstoaddNewAnvilWidgetToDSL, butupdatedWidgetsmay have been modified earlier in the loop. To ensure consistency and include all prior updates, consider passingupdatedWidgetsinstead.Apply this diff to fix the issue:
- updatedWidgets = yield addNewAnvilWidgetToDSL(allWidgets, { + updatedWidgets = yield addNewAnvilWidgetToDSL(updatedWidgets, { widgetId, type: widgetType, parentId: zoneWidgetId, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (5 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts
[error] 87-87: children is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
🔇 Additional comments (4)
app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (4)
61-61: Change is appropriate: updated parameter reflects the new function signature.Passing
zoneProps.widgetIdtoaddWidgetsToZonealigns with the updated function signature that now acceptszoneWidgetIdinstead of the entirezoneobject.
71-71: Function signature updated correctly.Changing the parameter from
zone: WidgetPropstozoneWidgetId: stringsimplifies the function and reduces unnecessary data passing.
87-87: Remove redundant self-assignment.Line 87 assigns
childrento itself, which is unnecessary and can be safely removed.🧰 Tools
🪛 Biome
[error] 87-87: children is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
217-217: Confirm thatcanvasIdcorrectly references the zone widget.In the
moveWidgetsToNewLayoutfunction, you're usingwidgets[canvasId]to obtainzone, and then passingzone.widgetIdtoaddWidgetsToZone. Ensure thatcanvasIdcorrectly references the intended zone widget to avoid potential issues with incorrect widget references.
app/client/packages/design-system/widgets/src/components/Markdown/src/styles.module.css
Show resolved
Hide resolved
| allWidgets: CanvasWidgetsReduxState, | ||
| highlight: AnvilHighlightInfo, | ||
| widgets: WidgetLayoutProps[], | ||
| draggedWidgets: WidgetLayoutProps[], |
There was a problem hiding this comment.
used better contextual name to avoid confusion.
widgets felt like it is all the widgets but it is only the dragged widgets.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts (1)
113-114: LGTM: Improved state handling with deep cloning.The renaming and use of
klonafor deep cloning enhance state management. Consider using a more descriptive name for the cloned state, such asmutableWidgets, to further clarify its purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/client/src/layoutSystems/anvil/integrations/sagas/anvilWidgetAdditionSagas/index.ts (1)
30-30: LGTM: Good choice for deep cloning.The addition of
klonais a solid choice for creating deep copies of objects, which aligns well with immutable state management practices in Redux.
jsartisan
added
ok-to-test
| const { alignment, canvasId } = highlight; | ||
| const allWidgets: CanvasWidgetsReduxState = yield select(getWidgets); | ||
| const allWidgetsFromRedux: CanvasWidgetsReduxState = yield select(getWidgets); | ||
| const allWidgets = klona(allWidgetsFromRedux) as CanvasWidgetsReduxState; |
There was a problem hiding this comment.
@riodeuno Is this safe? The use case is aichatwidget wants to modify the parent zone's elevatedBackground to false. ( that code is written in aichatbot blueprint operation ).
So the problem I was facing is if there is an existing zone that is empty and I drop aichatwidget into it, I was not able to modify the zone elevated background, as the redux state was coming from immer reducer and it was immutable, due to which the property was readonly.
There was a problem hiding this comment.
@jsartisan This is a problem because we will quickly lose track of where the original values were updated. Ideally, we should let the functions return what needs to be updated, then in this function we should use something like produce from immer to update the appropriate paths.
I'm not a 100% sure of what is happening in this PR, so I can't provide an alternative. Could you elaborate in the description on "how" we're going about changing the zone's elevatedBackground property?
There was a problem hiding this comment.
@riodeuno Updated the description. Let me know if you need more info.
jsartisan
added
ok-to-test
|
Note:
I'd suggest we unblock AI Chat with the changes in this PR, and revisit the system to understand how to modularise this particular service. |
) In anvil, when widgets are dropped on canvas, we create a zone which has `visual separation` marked as true by default. Now, There is a usecase for the chat widget in which we want to modify the zone`s `Visual separation` to false on creation of zone. The code for bleuprint operation for chat widget would like this: ```js blueprint: { operations: [ { type: BlueprintOperationTypes.MODIFY_PROPS, fn: ( widget: FlattenedWidgetProps, widgets: CanvasWidgetsReduxState, parent: FlattenedWidgetProps, layoutSystemType: LayoutSystemTypes, ) => { if (layoutSystemType !== LayoutSystemTypes.ANVIL) return []; const updates: UpdatePropertyArgs[] = []; const parentId = widget.parentId; if (!parentId) return updates; const parentWidget = widgets[parentId]; // we want to proceed only if the parent is a zone widget and has no children if ( parentWidget.children?.length === 0 && parentWidget.type === "ZONE_WIDGET" ) { updates.push({ widgetId: parentId, propertyName: "elevatedBackground", propertyValue: false, }); } return updates; }, }, ], }, ``` This should work fine, but in the code where we create zone and attaching widgets to it, after running the blueprint operations, we were not using the correct updated zone that returned from blueprint operations of the child widgets. This PR uses the correct zone. Also there is a case where blueprint operation is not able to update the existing widgets because all properties of existing widgets were readonly. So cloned the widgets from redux before passing to widget addition saga functions. /ok-to-test tags="@tag.All" ## Summary by CodeRabbit - **New Features** - Improved clarity in widget handling by renaming parameters related to dragged widgets. - Streamlined the process of adding widgets to zones by simplifying parameter structures. - **Bug Fixes** - Enhanced immutability in widget property updates within the state management process. - **Style** - Updated CSS styles for the `.markdown` class, removing unnecessary pseudo-elements for improved formatting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11474751370> > Commit: 8a385ee > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11474751370&attempt=3" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 23 Oct 2024 09:47:24 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved clarity in widget handling by renaming parameters related to dragged widgets. - Enhanced functionality for adding widgets to zones by simplifying the data structure used. - Implemented safer state manipulation for widget addition using a deep copy approach. - **Bug Fixes** - Addressed potential issues with direct state mutation during widget addition. - **Style** - Updated CSS styles for the Markdown component by removing unnecessary pseudo-elements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
In anvil, when widgets are dropped on canvas, we create a zone which has
visual separationmarked as true by default.Now, There is a usecase for the chat widget in which we want to modify the zone
sVisual separation` to false on creation of zone.The code for bleuprint operation for chat widget would like this:
This should work fine, but in the code where we create zone and attaching widgets to it, after running the blueprint operations, we were not using the correct updated zone that returned from blueprint operations of the child widgets. This PR uses the correct zone.
Also there is a case where blueprint operation is not able to update the existing widgets because all properties of existing widgets were readonly. So cloned the widgets from redux before passing to widget addition saga functions.
/ok-to-test tags="@tag.All"
Summary by CodeRabbit
New Features
Bug Fixes
Style
.markdownclass, removing unnecessary pseudo-elements for improved formatting.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11474751370
Commit: 8a385ee
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 23 Oct 2024 09:47:24 UTC
Summary by CodeRabbit
New Features
Bug Fixes
Style