chore: Anvil only: Automatically delete redundant zones #34879
Conversation
znamenskii-ilia
added
skip-changelog
WalkthroughThe changes introduce new functions and tests related to widget handling in the Appsmith platform. These modifications aim to enhance the deletion logic by removing redundant zones when widgets are moved or deleted. Key updates include adding new utility functions to check widget properties and integrating these functions into existing sagas and utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Saga
participant Utils
participant Store
User->>Saga: Move widget
Saga->>Utils: Call handleDeleteRedundantZones
Utils->>Store: Check widget properties and determine redundant zones
Store-->>Utils: Return redundant zone information
Utils-->>Saga: Delete redundant zones
Saga-->>User: Update UI to reflect changes
Assessment against linked issues
Possibly related issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
Anvil Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9891175547. |
znamenskii-ilia
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts (5 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.test.ts (1 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (2 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/widgetUtils.test.ts (1 hunks)
- app/client/src/layoutSystems/anvil/utils/layouts/widgetUtils.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.test.ts
Additional comments not posted (11)
app/client/src/layoutSystems/anvil/utils/layouts/widgetUtils.ts (3)
32-33: LGTM!The
isEmptyWidgetfunction correctly checks if a widget has no children.
35-37: LGTM!The
hasWidgetJsPropertiesEnabledfunction correctly checks for dynamic properties.
39-40: LGTM!The
widgetChildrenfunction correctly returns the children of a widget.app/client/src/layoutSystems/anvil/utils/layouts/widgetUtils.test.ts (3)
8-34: LGTM!The test cases for
isEmptyWidgetare thorough and correctly implemented.
37-54: LGTM!The test cases for
widgetChildrenare thorough and correctly implemented.
57-81: LGTM!The test cases for
hasWidgetJsPropertiesEnabledare thorough and correctly implemented.app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (2)
251-252: LGTM!The
isZoneWidgetfunction correctly checks if a widget is a zone widget.
254-261: LGTM!The
isRedundantZoneWidgetfunction correctly checks if a zone widget is redundant based on the specified conditions.app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts (3)
396-424: LGTM!The
handleDeleteRedundantZonesfunction correctly handles the deletion of redundant zones based on the specified conditions.
50-50: LGTM!The
widgetChildrenfunction correctly returns the children of a widget.
337-341: LGTM!The updates to
moveWidgetsSagacorrectly incorporate thehandleDeleteRedundantZonesfunction.
|
Deploy-Preview-URL: https://ce-34879.dp.appsmith.com |
github-actions
bot
added
Enhancement
KelvinOm
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts
| ): boolean => | ||
| isZoneWidget(widget) && | ||
| isEmptyWidget(widget) && | ||
| widgetChildren(parentSection).length === 1 && |
There was a problem hiding this comment.
What are we checking by this line?
There was a problem hiding this comment.
That the zone is the only child in the parent. I will add a comment
| widget: FlattenedWidgetProps, | ||
| ): boolean => (widget.dynamicPropertyPathList || []).length > 0; | ||
|
|
||
| export const widgetChildren = (widget: FlattenedWidgetProps): string[] => |
There was a problem hiding this comment.
I don't really understand the meaning of this method. It seems easier and clearer to just get children through a dot(widget.children). When using the method, it is not obvious what is happening inside, whether we somehow modify the children or maybe filter them, etc. what operations are performed in this function.
There was a problem hiding this comment.
You are right, it is easier to use widget.children directly. However, right now children is optional field and we always have to check it is array or not.
Here is one example found in code:
return (a.children || []).length - (b.children || []).length;
widgetChildren increases readability a bit imo
return widgetChildren(a).length - widgetChildren(b).length;
Once we define clear entities and split widgets by types we will not need to have children optional and we will remove this utility.
There was a problem hiding this comment.
I would still write something more specific for section, something like sectionHasOnlyOneZone. In this method you can incapsulate all necessary child checks, etc., Also you won't need to add the comment, and the code will be more readable.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/layoutSystems/anvil/utils/layouts/update/zoneUtils.ts
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9906867344. |
|
Deploy-Preview-URL: https://ce-34879.dp.appsmith.com |
app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts
Outdated
Show resolved
Hide resolved
| @@ -440,6 +452,37 @@ function* deleteAllSelectedWidgetsSaga( | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Find a way to reuse identical code from anvilDraggingSagas/index.ts | |||
| export function handleDeleteRedundantZones( | |||
There was a problem hiding this comment.
Good point. This can be a utility function that only applies to Anvil. We'll refactor widget delete to be Anvil specific.
There was a problem hiding this comment.
@znamenskii-ilia Would you mind creating an issue on Github for this? You could assign it to both of us, and the Anvil team, so that it shows up on our Zenhub board.
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/layoutSystems/anvil/integrations/sagas/anvilDraggingSagas/index.ts
riodeuno
added
ok-to-test
riodeuno
changed the title
github-actions
bot
added
skip-changelog
riodeuno
added
ok-to-test
znamenskii-ilia
dismissed stale reviews from riodeuno and coderabbitai[bot]
via
d2a5392
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/WidgetDeletionSagas.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/WidgetDeletionSagas.ts
znamenskii-ilia
added
ok-to-test
Description
Delete redundant zones when drag last widget out or remove it.
https://www.notion.so/appsmith/Removing-redundant-zones-8a2cf907c97246adb618664940e298b0
Fixes #34854
Screen.Recording.2024-07-12.at.13.32.02.mov
Screen.Recording.2024-07-12.at.13.28.31.mov
Automation
/ok-to-test tags="@tag.Anvil"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9953698717
Commit: d2a5392
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Tue, 16 Jul 2024 09:12:41 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests