fix: Use unique id instead of indices as keys for DraggableList #13553

berzerkeer · 2022-05-04T09:31:58Z

Description

Reordering page list and then renaming a page won't update the targeted page's name due to key mismatch. We were using index as key for the DraggableList component. So changed the list to take unique page ids for the list items keys.

Fixes #9991

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Go to any application, create 3 pages, swap 2nd and 3rd page's order. Now rename the 3rd page, it works as expected. Previously, it was renaming the 2nd page instead of the 3rd page.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Test coverage results 🧪

⚪ Total coverage has not changed

// Code coverage diff between base branch:release and head branch: fix/pages-list-rename-bug

Status	File	% Stmts	% Branch	% Funcs	% Lines
🔴	app/client/src/pages/Editor/PagesEditor/index.tsx	46.81 (-1.02)	42.86 (-17.14)	0 (0)	47.83 (-1.06)

…th-local

welcome · 2022-05-04T09:32:00Z

Welcome to the Appsmith community! Thank you for your first pull request and making this project better. 🤗

vercel · 2022-05-04T09:32:01Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
appsmith	✅ Ready (Inspect)	Visit Preview	May 9, 2022 at 8:15AM (UTC)

github-actions · 2022-05-04T09:53:37Z

Unable to find test scripts. Please add necessary tests to the PR.

berzerkeer · 2022-05-04T10:16:09Z

/ok-to-test sha=bd269b3

github-actions · 2022-05-04T10:32:46Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2269201210.
Workflow: Appsmith External Integration Test Workflow.
Commit: bd269b3.
PR: 13553.

…ges-list-rename-bug

berzerkeer · 2022-05-05T11:38:33Z

/ok-to-test sha=102dbcf

github-actions · 2022-05-05T12:18:40Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2275567570.
Workflow: Appsmith External Integration Test Workflow.
Commit: 102dbcf.
PR: 13553.

ankitakinger · 2022-05-04T10:43:03Z

app/client/src/components/ads/DraggableList.tsx

@@ -235,8 +235,8 @@ function DraggableList(props: any) {
 {springs.map(({ scale, y, zIndex }, i) => (
 <animated.div
 {...bind(i)}
- data-rbd-draggable-id={items[i].id}
- key={i}
+ data-rbd-draggable-id={items[i].pageId}


Are you sure this change won't break the cypress tests? Just check and confirm once.

ankitakinger

This may lead to bugs at other places as pageId is not being passed at all places. Since this is a bug in just one file app/client/src/pages/Editor/PagesEditor/index.tsx you can use something similar here as file app/client/src/components/propertyControls/FieldConfigurationControl.tsx line no. 226 and have send id as well as pageId with both being equal to pageId itself:

const draggableComponentColumns = pages.map(
      ({ identifier, isCustomField, isVisible, label }, index) => ({
        id: identifier,
        index,
        isCustomField,
        isVisible,
        label,
      }),
    );

…er places

…ges-list-rename-bug

berzerkeer · 2022-05-06T07:49:50Z

This may lead to bugs at other places as pageId is not being passed at all places. Since this is a bug in just one file app/client/src/pages/Editor/PagesEditor/index.tsx you can use something similar here as file app/client/src/components/propertyControls/FieldConfigurationControl.tsx line no. 226 and have send id as well as pageId with both being equal to pageId itself:
const draggableComponentColumns = pages.map(
      ({ identifier, isCustomField, isVisible, label }, index) => ({
        id: identifier,
        index,
        isCustomField,
        isVisible,
        label,
      }),
    );

Thanks @ankitakinger , I have made those changes you suggested. Please review.

ankitakinger

LGTM

github-actions · 2022-05-06T10:45:57Z

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2269201210.
Commit: bd269b3.
Results:

Click to view performance test results

	Run 1	Run 2	Run 3	Run 4	Run 5	Median	Mean	SD.Sample	SD.Population
SELECT_WIDGET_MENU_OPEN
scripting	1604.86	1785.58	1635.67	1581.46	1600.23	1604.86	1641.56	5.05	4.51
painting	8.36	16.01	10.4	14.51	6.87	10.4	11.23	34.91	31.26
rendering	533.96	754.86	601.68	530.94	528.89	533.96	590.07	16.45	14.71
SELECT_WIDGET_SELECT_OPTION
scripting	266.71	332.58	245.15	250.22	281.74	266.71	275.28	12.76	11.41
painting	10.82	6.1	7.49	6.61	2.83	6.61	6.77	42.39	37.96
rendering	18.36	21.89	19.01	17.14	18.44	18.44	18.97	9.33	8.33

github-actions · 2022-05-06T11:02:49Z

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2269201210.
Commit: bd269b3.
Results:

Click to view performance test results

	Run 1	Run 2	Run 3	Run 4	Run 5	Median	Mean	SD.Sample	SD.Population
SELECT_WIDGET_MENU_OPEN
scripting	1604.86	1785.58	1635.67	1581.46	1600.23	1604.86	1641.56	5.05	4.51
painting	8.36	16.01	10.4	14.51	6.87	10.4	11.23	34.91	31.26
rendering	533.96	754.86	601.68	530.94	528.89	533.96	590.07	16.45	14.71
SELECT_WIDGET_SELECT_OPTION
scripting	266.71	332.58	245.15	250.22	281.74	266.71	275.28	12.76	11.41
painting	10.82	6.1	7.49	6.61	2.83	6.61	6.77	42.39	37.96
rendering	18.36	21.89	19.01	17.14	18.44	18.44	18.97	9.33	8.33

…ges-list-rename-bug

berzerkeer · 2022-05-09T09:01:51Z

/ok-to-test sha=308da3e

github-actions · 2022-05-09T09:02:27Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2293121983.
Workflow: Appsmith External Integration Test Workflow.
Commit: 308da3e.
PR: 13553.

github-actions · 2022-05-09T09:36:55Z

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2293121983.
Commit: 308da3e.
Results:

Click to view performance test results

	Run 1	Run 2	Run 3	Run 4	Run 5	Median	Mean	SD.Sample	SD.Population
SELECT_WIDGET_MENU_OPEN
scripting	1996.45	1983.42	2051.16	2036.01	2075.96	2036.01	2028.6	1.89	1.69
painting	18.39	19.67	18.56	14.78	10.8	18.39	16.44	22.20	19.83
rendering	681.78	687.05	704.06	698.11	691.03	691.03	692.41	1.28	1.14
SELECT_WIDGET_SELECT_OPTION
scripting	295.96	305.2	443.04	331.53	316.99	316.99	338.54	17.70	15.83
painting	6.94	4.71	8.39	5.17	9.02	6.94	6.85	27.74	24.82
rendering	21.42	20.68	21.68	23.48	21.35	21.42	21.72	4.83	4.33

github-actions · 2022-05-09T09:44:19Z

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2293121983.
Commit: 308da3e.
Results:

Click to view performance test results

	Run 1	Run 2	Run 3	Run 4	Run 5	Median	Mean	SD.Sample	SD.Population
SELECT_WIDGET_MENU_OPEN
scripting	1996.45	1983.42	2051.16	2036.01	2075.96	2036.01	2028.6	1.89	1.69
painting	18.39	19.67	18.56	14.78	10.8	18.39	16.44	22.20	19.83
rendering	681.78	687.05	704.06	698.11	691.03	691.03	692.41	1.28	1.14
SELECT_WIDGET_SELECT_OPTION
scripting	295.96	305.2	443.04	331.53	316.99	316.99	338.54	17.70	15.83
painting	6.94	4.71	8.39	5.17	9.02	6.94	6.85	27.74	24.82
rendering	21.42	20.68	21.68	23.48	21.35	21.42	21.72	4.83	4.33

rahulramesha · 2022-05-09T15:08:16Z

@berzerkeer @ankitakinger I actually had another PR in the pipeline for this on a different (duplicate) issue #12758.
Use of items[i].id should have been the best way to solve this but since the component DraggableList is used in a lot more places this breaks few of them, eg, Table Widget's primaryColumns in property Pane. Most of them don't seem to have reliable unique ids. here is a reference to my PR #13448.

Sangeeth Sivan added 6 commits April 25, 2022 15:23

gitignore

ff1945f

Merge branch 'release' of github.com:appsmithorg/appsmith into sangee…

4982198

…th-local

Merge branch 'release' of github.com:appsmithorg/appsmith into sangee…

ebe028a

…th-local

Merge branch 'release' of github.com:appsmithorg/appsmith into sangee…

9afdae5

…th-local

fix: Use unique id instead of indices as keys for DraggableList(#9991).

33f3e1e

removed duplicate 'stacks' from gitignore

bd269b3

berzerkeer requested a review from ankitakinger May 4, 2022 10:19

Sangeeth Sivan added 2 commits May 5, 2022 16:10

Merge branch 'release' of github.com:appsmithorg/appsmith into fix/pa…

866910c

…ges-list-rename-bug

revert change to cypress selector for draggable list

102dbcf

vercel bot deployed to Preview May 5, 2022 11:42 View deployment

ankitakinger approved these changes May 5, 2022

View reviewed changes

ankitakinger requested changes May 5, 2022

View reviewed changes

use 'id' ad key name for consistency and that it doesn't break in oth…

4378473

…er places

Merge branch 'release' of github.com:appsmithorg/appsmith into fix/pa…

36b05cc

…ges-list-rename-bug

berzerkeer requested a review from ankitakinger May 6, 2022 07:50

vercel bot deployed to Preview May 6, 2022 07:58 View deployment

ankitakinger approved these changes May 6, 2022

View reviewed changes

Sangeeth Sivan added 2 commits May 8, 2022 14:25

Merge branch 'release' of github.com:appsmithorg/appsmith into fix/pa…

2f51c3b

…ges-list-rename-bug

Merge branch 'release' of github.com:appsmithorg/appsmith into fix/pa…

308da3e

…ges-list-rename-bug

vercel bot deployed to Preview May 9, 2022 08:15 View deployment

berzerkeer merged commit d5845e9 into release May 9, 2022

berzerkeer deleted the fix/pages-list-rename-bug branch May 9, 2022 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use unique id instead of indices as keys for DraggableList #13553

fix: Use unique id instead of indices as keys for DraggableList #13553

berzerkeer commented May 4, 2022 •

edited by github-actions bot

Loading

welcome bot commented May 4, 2022

vercel bot commented May 4, 2022 •

edited

Loading

github-actions bot commented May 4, 2022

berzerkeer commented May 4, 2022

github-actions bot commented May 4, 2022

berzerkeer commented May 5, 2022

github-actions bot commented May 5, 2022

ankitakinger May 4, 2022

ankitakinger left a comment

berzerkeer commented May 6, 2022

ankitakinger left a comment

github-actions bot commented May 6, 2022

github-actions bot commented May 6, 2022

berzerkeer commented May 9, 2022

github-actions bot commented May 9, 2022

github-actions bot commented May 9, 2022

github-actions bot commented May 9, 2022

rahulramesha commented May 9, 2022 •

edited

Loading

fix: Use unique id instead of indices as keys for DraggableList #13553

fix: Use unique id instead of indices as keys for DraggableList #13553

Conversation

berzerkeer commented May 4, 2022 • edited by github-actions bot Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Test coverage results 🧪

welcome bot commented May 4, 2022

vercel bot commented May 4, 2022 • edited Loading

github-actions bot commented May 4, 2022

berzerkeer commented May 4, 2022

github-actions bot commented May 4, 2022

berzerkeer commented May 5, 2022

github-actions bot commented May 5, 2022

ankitakinger May 4, 2022

Choose a reason for hiding this comment

ankitakinger left a comment

Choose a reason for hiding this comment

berzerkeer commented May 6, 2022

ankitakinger left a comment

Choose a reason for hiding this comment

github-actions bot commented May 6, 2022

github-actions bot commented May 6, 2022

berzerkeer commented May 9, 2022

github-actions bot commented May 9, 2022

github-actions bot commented May 9, 2022

github-actions bot commented May 9, 2022

rahulramesha commented May 9, 2022 • edited Loading

berzerkeer commented May 4, 2022 •

edited by github-actions bot

Loading

vercel bot commented May 4, 2022 •

edited

Loading

rahulramesha commented May 9, 2022 •

edited

Loading