fix: multiselect widget selected values inside list widget #41214
Conversation
saharAdem
requested review from
vivek-appsmith
and removed request for
a team
WalkthroughAdds per-list-item persistence for MultiSelectWidgetV2 via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant L as ListWidgetV2
participant M as MultiSelectWidgetV2
participant S as MetaStore
U->>L: Focus item i
L->>M: Render (currentIndex = i, listItemId)
M->>S: Read selectedValuesByItem[listItemId]
alt entry missing
M->>S: updateWidgetMetaProperty(selectedValuesByItem[listItemId] = defaultOptionValue)
end
M-->>U: Display per-item selection
U->>M: Change selection -> newValues
M->>S: updateWidgetMetaProperty(selectedValuesByItem[listItemId] = newValues)
U->>L: Focus item j or delete item k
L->>M: Render (currentIndex = j)
M->>S: syncSelectionMapOnIndexChange(...) to align keys and populate defaults
M->>S: Read selectedValuesByItem[newListItemId]
M-->>U: Display values for item j
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (2)
849-850: Redundant type assertion can be removedSince
defaultOptionValueis already typed asstring[] | OptionValue[]in the props, the explicit type assertion tostring[]on line 850 is unnecessary.- values = defaultOptionValue as string[]; + values = defaultOptionValue;
906-924: Consider edge case handling for currentIndexThe implementation assumes
currentIndexis always defined, but this might not be true for widgets outside of list contexts. Consider adding a fallback.onOptionChange = (value: DraftValueType) => { - const itemId = this.props.currentIndex; + const itemId = this.props.currentIndex ?? "default"; const updatedValue = { ...(this.props.selectedValuesByItem || {}), [itemId]: value, };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug 41210 _Spec.ts(1 hunks)app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/**/**.*
⚙️ CodeRabbit configuration file
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug 41210 _Spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js:35-37
Timestamp: 2024-11-13T09:11:36.959Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js`, when writing Cypress end-to-end tests for the Image Widget in JavaScript, use `viewWidgetsPage.imageinner` as the selector when asserting 'src' and 'alt' attributes to maintain consistency.
📚 Learning: 2024-11-13T09:11:36.959Z
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js:35-37
Timestamp: 2024-11-13T09:11:36.959Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js`, when writing Cypress end-to-end tests for the Image Widget in JavaScript, use `viewWidgetsPage.imageinner` as the selector when asserting 'src' and 'alt' attributes to maintain consistency.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug 41210 _Spec.ts
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: abhishek-bandameedi
PR: appsmithorg/appsmith#35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug 41210 _Spec.ts
🔇 Additional comments (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1)
839-856: Good implementation of per-item selection persistenceThe logic correctly tracks selections per list item using currentIndex as the key and properly initializes with default values when an item hasn't been accessed before.
| import { locators } from "../../../../support/Objects/ObjectsCore"; | ||
| import * as _ from "../../../../support/Objects/ObjectsCore"; | ||
|
|
||
| const widgetSelector = (name: string) => `[data-widgetname-cy="${name}"]`; | ||
|
|
||
| describe("Bug 41210: MultiSelectWidgetV2 inside ListWidget - selected values and labels persist per item", function () { | ||
| before(() => { | ||
| _.agHelper.AddDsl("Listv2/emptyList"); | ||
| }); | ||
|
|
||
| it("should persist selected values for each list item and initialize with default values on first render", function () { | ||
| cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget", { | ||
| x: 250, | ||
| y: 50, | ||
| }); | ||
| _.propPane.UpdatePropertyFieldValue("Default selected values", '["GREEN"]'); | ||
| _.agHelper.GetNClick(locators._enterPreviewMode); | ||
| _.agHelper.SelectFromMultiSelect(["Red"]); | ||
|
|
||
| const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`; | ||
|
|
||
| cy.get(listContainer).eq(1).click(); | ||
| cy.get(listContainer).eq(0).click(); | ||
| cy.get( | ||
| `${widgetSelector("MultiSelect1")} .rc-select-selection-item`, | ||
| ).should("contain.text", "Red"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix the filename to follow naming conventions
The filename contains a space between "Bug" and "41210" which violates standard naming conventions. Rename the file to use underscores or camelCase consistently.
-MultiselectWidget_Bug 41210 _Spec.ts
+MultiselectWidget_Bug41210_Spec.tsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug
41210 _Spec.ts lines 1-28: the filename contains a space ("Bug 41210") which
violates repo naming conventions; rename the file to remove spaces (e.g.,
MultiselectWidget_Bug_41210_Spec.ts or multiselectWidgetBug41210Spec.ts) using
git mv, update any CI/test references or import paths that point to the old
filename, and ensure the test runner configuration or any documentation is
updated accordingly so the test still runs under the new name.
| it("should persist selected values for each list item and initialize with default values on first render", function () { | ||
| cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget", { | ||
| x: 250, | ||
| y: 50, | ||
| }); | ||
| _.propPane.UpdatePropertyFieldValue("Default selected values", '["GREEN"]'); | ||
| _.agHelper.GetNClick(locators._enterPreviewMode); | ||
| _.agHelper.SelectFromMultiSelect(["Red"]); | ||
|
|
||
| const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`; | ||
|
|
||
| cy.get(listContainer).eq(1).click(); | ||
| cy.get(listContainer).eq(0).click(); | ||
| cy.get( | ||
| `${widgetSelector("MultiSelect1")} .rc-select-selection-item`, | ||
| ).should("contain.text", "Red"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test lacks comprehensive assertions
The test only verifies that "Red" is selected but doesn't verify:
- That the default value "GREEN" was initially selected
- That other list items maintain their own selections
- That the selection persists after multiple navigations
it("should persist selected values for each list item and initialize with default values on first render", function () {
cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget", {
x: 250,
y: 50,
});
_.propPane.UpdatePropertyFieldValue("Default selected values", '["GREEN"]');
_.agHelper.GetNClick(locators._enterPreviewMode);
+
+ // Verify default value is selected
+ cy.get(
+ `${widgetSelector("MultiSelect1")} .rc-select-selection-item`,
+ ).should("contain.text", "Green");
+
_.agHelper.SelectFromMultiSelect(["Red"]);
const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`;
+ // Navigate to second item and verify it has the default value
cy.get(listContainer).eq(1).click();
+ cy.get(
+ `${widgetSelector("MultiSelect1")} .rc-select-selection-item`,
+ ).should("contain.text", "Green");
+
+ // Return to first item and verify Red persists
cy.get(listContainer).eq(0).click();
cy.get(
`${widgetSelector("MultiSelect1")} .rc-select-selection-item`,
).should("contain.text", "Red");
+
+ // Navigate again to ensure persistence
+ cy.get(listContainer).eq(1).click();
+ cy.get(listContainer).eq(0).click();
+ cy.get(
+ `${widgetSelector("MultiSelect1")} .rc-select-selection-item`,
+ ).should("contain.text", "Red");
});📝 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.
| it("should persist selected values for each list item and initialize with default values on first render", function () { | |
| cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget", { | |
| x: 250, | |
| y: 50, | |
| }); | |
| _.propPane.UpdatePropertyFieldValue("Default selected values", '["GREEN"]'); | |
| _.agHelper.GetNClick(locators._enterPreviewMode); | |
| _.agHelper.SelectFromMultiSelect(["Red"]); | |
| const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`; | |
| cy.get(listContainer).eq(1).click(); | |
| cy.get(listContainer).eq(0).click(); | |
| cy.get( | |
| `${widgetSelector("MultiSelect1")} .rc-select-selection-item`, | |
| ).should("contain.text", "Red"); | |
| }); | |
| it("should persist selected values for each list item and initialize with default values on first render", function () { | |
| cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget", { | |
| x: 250, | |
| y: 50, | |
| }); | |
| _.propPane.UpdatePropertyFieldValue("Default selected values", '["GREEN"]'); | |
| _.agHelper.GetNClick(locators._enterPreviewMode); | |
| // Verify default value is selected | |
| cy.get( | |
| `${widgetSelector("MultiSelect1")} .rc-select-selection-item`, | |
| ).should("contain.text", "Green"); | |
| _.agHelper.SelectFromMultiSelect(["Red"]); | |
| const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`; | |
| // Navigate to second item and verify it has the default value | |
| cy.get(listContainer).eq(1).click(); | |
| cy.get( | |
| `${widgetSelector("MultiSelect1")} .rc-select-selection-item`, | |
| ).should("contain.text", "Green"); | |
| // Return to first item and verify Red persists | |
| cy.get(listContainer).eq(0).click(); | |
| cy.get( | |
| `${widgetSelector("MultiSelect1")} .rc-select-selection-item`, | |
| ).should("contain.text", "Red"); | |
| // Navigate again to ensure persistence | |
| cy.get(listContainer).eq(1).click(); | |
| cy.get(listContainer).eq(0).click(); | |
| cy.get( | |
| `${widgetSelector("MultiSelect1")} .rc-select-selection-item`, | |
| ).should("contain.text", "Red"); | |
| }); |
🤖 Prompt for AI Agents
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug
41210 _Spec.ts lines 11-27: the test only asserts that "Red" is selected but
misses verifying the initial default "GREEN", per-item selection independence,
and persistence after navigation; update the test to first assert that on first
render MultiSelect1 contains "GREEN" (the default), then select a different
option for a second list item and assert that each list item shows its own
selected value (e.g., item0 shows "GREEN", item1 shows "Red"), then navigate
between list items (click other indices) multiple times and re-check the
selections to confirm persistence across navigations; use the same widget
selectors already present (widgetSelector("MultiSelect1") and listContainer
indices) and add assertions after each navigation step to validate the expected
text values.
| cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget", { | ||
| x: 250, | ||
| y: 50, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using coordinate-based drag and drop
Using fixed x,y coordinates (250, 50) for drag and drop operations is brittle and may fail on different screen sizes or when the UI changes. Consider using a more robust approach.
#!/bin/bash
# Check if there are helper methods available for drag and drop operations without coordinates
rg -n --type=ts "dragAndDropToWidget.*widgetSelector" -A 3 -B 3🤖 Prompt for AI Agents
In app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug
41210 _Spec.ts around lines 12 to 15, the test uses fixed x,y coordinates for
cy.dragAndDropToWidget which is brittle; update the call to use a selector-based
target (or the helper's widgetSelector option) so the drop is done relative to
the target element instead of absolute coordinates — either pass the destination
widget selector to cy.dragAndDropToWidget (remove the x/y object) or compute the
target's center via cy.get('<target-selector>').then(el => { use its bounding
box to perform the drag/drop }) so the test works across screen sizes and layout
changes.
| _.agHelper.GetNClick(locators._enterPreviewMode); | ||
| _.agHelper.SelectFromMultiSelect(["Red"]); | ||
|
|
||
| const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`; |
There was a problem hiding this comment.
Avoid using CSS attribute selectors
The selector [type="CONTAINER_WIDGET"] violates the coding guidelines which specify avoiding attribute selectors. Use data-* attributes instead.
- const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`;
+ const listContainer = `${widgetSelector("List1")} [data-testid="list-container"]`;Note: Ensure the corresponding data-testid attribute is added to the container widget in the application code.
📝 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.
| const listContainer = `${widgetSelector("List1")} [type="CONTAINER_WIDGET"]`; | |
| const listContainer = `${widgetSelector("List1")} [data-testid="list-container"]`; |
🤖 Prompt for AI Agents
In app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiselectWidget_Bug
41210 _Spec.ts around line 20, the selector uses a CSS attribute selector
`[type="CONTAINER_WIDGET"]` which violates guidelines; replace it with a
data-testid selector and update the application to add that data-testid to the
container widget. Change the test to target the container via its data-testid
(e.g., `${widgetSelector("List1")} [data-testid="container-widget-List1"]` or a
stable name you add in the app) and add the corresponding data-testid attribute
to the container widget in the app code.
vivek-appsmith
requested review from
ankitakinger,
jacquesikot and
rahulbarwal
and removed request for
vivek-appsmith
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1)
774-780: Add selectedValuesByItem to the meta properties map.selectedValuesByItem is read/updated (componentDidUpdate, onOptionChange) but missing from getMetaPropertiesMap; add selectedValuesByItem: {} to the return object in app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (around lines 774–780).
🧹 Nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts (2)
2-2: Remove unnecessary wildcard import.The wildcard import
* as _is not following best practices and is inconsistent with the specificlocatorsimport above it.-import * as _ from "../../../../support/Objects/ObjectsCore"; +import { agHelper, propPane } from "../../../../support/Objects/ObjectsCore";
12-15: Use specific coordinates or consider alternatives to hardcoded positioning.The hardcoded coordinates (x: 250, y: 50) may be brittle across different screen sizes or viewport configurations.
- cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget", { - x: 250, - y: 50, - }); + cy.dragAndDropToWidget("multiselectwidgetv2", "containerwidget");app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1)
991-1019: Add interface documentation for new properties.The new
currentIndexandselectedValuesByItemproperties should be documented to clarify their purpose in the per-item persistence feature.export interface MultiSelectWidgetProps extends WidgetProps { // ... existing properties ... + /** Current index of the list item when widget is inside a ListWidget */ + currentIndex: number; + /** Map of selected values per list item, keyed by list item index */ + selectedValuesByItem?: Record<string, string[] | OptionValue[]>; // ... remaining properties ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts(1 hunks)app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/**/**.*
⚙️ CodeRabbit configuration file
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
🧠 Learnings (2)
📚 Learning: 2024-11-13T09:11:36.959Z
Learnt from: vhemery
PR: appsmithorg/appsmith#37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js:35-37
Timestamp: 2024-11-13T09:11:36.959Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js`, when writing Cypress end-to-end tests for the Image Widget in JavaScript, use `viewWidgetsPage.imageinner` as the selector when asserting 'src' and 'alt' attributes to maintain consistency.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: abhishek-bandameedi
PR: appsmithorg/appsmith#35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
🧬 Code graph analysis (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts (1)
app/client/cypress/support/commands.js (2)
_(1071-1071)locators(43-43)
🔇 Additional comments (4)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts (1)
16-26: LGTM! Test logic correctly validates per-item persistence.The test properly validates the bug fix by:
- Setting a default value
- Selecting a different option in the first list item
- Switching focus between list items
- Verifying the selection persists in the original item
The use of data-widgetname-cy selectors and specific container targeting aligns with coding guidelines.
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (3)
813-824: LGTM! Proper initialization of per-item state on default value changes.The implementation correctly handles initialization of per-item selections when
defaultOptionValuechanges, ensuring each list item gets its own selection state.
902-920: LGTM! Per-item selection tracking implemented correctly.The
onOptionChangemethod properly stores selections per list item usingcurrentIndexas the key, maintaining the existing trigger semantics while adding per-item persistence.
924-941: LGTM! Smart initialization of per-item defaults.The
mergeLabelAndValuemethod correctly:
- Retrieves per-item values or falls back to defaults
- Initializes missing per-item state with default values
- Returns the appropriate values for the current list item
This ensures each list item maintains its own selection state while preserving backward compatibility.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
@rahulbarwal @jacquesikot @ankitakinger Could you please review the PR? |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hey @Nikhil-Nandagopal, any updates on this? |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hey, any updates? |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
|
@saharAdem Please check these Potential Issues & Recommendations
The MultiSelectWidgetProps interface at the bottom of the file doesn't include the new props: These should be added to app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx:197-225
getMetaPropertiesMap() at line 774 should include selectedValuesByItem:
Recommendations
The approach is sound and well-designed for fixing the bug. The per-item state tracking is the right solution. However, there are some TypeScript interface completeness issues that should be addressed before merging to ensure type safety. |
|
Thank you for your review @jacquesikot . I’ll be resolving your comments within the next two days. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts(1 hunks)app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
🔇 Additional comments (4)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (4)
779-779: Interface and meta properties look good.The additions properly address the reviewer's concerns about missing properties. The types are appropriate for the per-item selection tracking functionality.
Also applies to: 1071-1072
961-979: Per-item selection storage looks correct.The method properly stores selections keyed by
currentIndex, preserves existing entries via spread, and maintains theisDirtyflag. The fallback to0whencurrentIndexis undefined provides reasonable default behavior.
982-993: Value retrieval logic is clean and correct.The fallback chain
selectedValuesByItem[itemId] ?? defaultOptionValueensures values are always available, and the destructuring with defaults prevents undefined errors.
833-884: <!-- [scratchpad_start]
[task verify_review_assumption 1/1]
[observations]
- Review comment flags potential memory leak in MultiSelectWidgetV2
- Claims selectedValuesByItem map accumulates entries indefinitely
- Assumes List widget provides listSize property
- Proposed fix adds cleanup logic in componentDidUpdate
- Shell script output did not return the target file (tsx parsing error)
- No verification of actual List widget API or accumulation pattern
[analysis]- Review makes assumptions without confirming List widget exposes item count
- Cleanup logic placement assumes componentDidUpdate executes on list resize
- Without seeing actual usage patterns, cannot confirm if accumulation is real
- The method shown (syncSelectionMapOnIndexChange) handles index shifts during navigation, not removal cleanup
[pending]- Verify if List widget actually provides item count property
- Confirm selectedValuesByItem actually grows unbounded in practice
- Check if this is preventive or addressing observed issue
[actions]- Request manual verification of List widget API
- Ask developer to confirm observed memory growth
[done]
Unable to verify assumptions in review comment due to script failure and missing API visibility
[scratchpad_end]Verify assumptions about List widget API and actual memory accumulation.
The review assumes the List widget exposes an
listSizeproperty and thatselectedValuesByItemgrows unbounded in production. Without access to the List widget props interface or evidence of actual memory growth, these assumptions need confirmation.Specifically verify:
- Does the List widget provide
listSize,itemCount, or similar property?- Is memory accumulation observed in practice, or is this preventive?
- Is
componentDidUpdatealways called when the list resizes?If these assumptions don't hold, the proposed cleanup logic may be unnecessary or placed incorrectly.
|
This PR has been closed because of inactivity. |
saharAdem
changed the title
|
I’ve resolved the AI comments and added the list item ID for the list widget, but I can’t commit the changes because the PR is closed. Could you please reopen it? @jacquesikot |
|
@jacquesikot Would it be possible to re-open this PR so we can submit the requested changes? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1)
820-835: User selections may still be overwritten whendefaultOptionValuechanges.Lines 820-823 reset
isDirtyto false when changes are detected, but lines 824-835 still unconditionally updateselectedValuesByItem[listItemId]with the newdefaultOptionValue. This overwrites user selections made for that item.Consider guarding the update:
- if (hasChanges) { + if (hasChanges && !this.props.isDirty) { const listItemId = this.getListItemId(this.props.currentIndex);Or move the
isDirtyreset after the condition to preserve user selections when dirty.
🧹 Nitpick comments (3)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts (3)
8-9: Preferdata-*attribute selectors over type attributes.
[type="CONTAINER_WIDGET"]is an attribute selector. Per coding guidelines, preferdata-*attributes for stability. If adata-testidor similar attribute exists on containers, use that instead.
53-55: CSS class selector from third-party component.
.rc-select-selection-itemis a library-internal class. If adata-*attribute is available on these elements, prefer that for resilience against library updates. Otherwise, document this as a known fragile selector.
40-56: Consider adding baseline assertion before interaction.Before selecting "Red", assert that the default "GREEN" is shown. This confirms the default value is applied correctly and makes test failures more diagnostic.
_.agHelper.GetNClick(locators._enterPreviewMode); + cy.get( + `${locators._widgetByName("MultiSelect1")} .rc-select-selection-item`, + ).should("contain.text", "Green"); _.agHelper.SelectFromMultiSelect(["Red"]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts(1 hunks)app/client/src/widgets/ListWidgetV2/widget/index.tsx(2 hunks)app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/**/**.*
⚙️ CodeRabbit configuration file
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
🧠 Learnings (10)
📚 Learning: 2025-05-21T04:51:50.934Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 40711
File: app/client/src/widgets/ListWidgetV2/widget/index.tsx:514-532
Timestamp: 2025-05-21T04:51:50.934Z
Learning: In the ListWidgetV2 component, `klonaRegularWithTelemetry` is used to perform deep cloning of objects before modifying them, which is a valid pattern for preserving the original reference while making changes to a copy.
Applied to files:
app/client/src/widgets/ListWidgetV2/widget/index.tsx
📚 Learning: 2025-06-13T07:49:21.417Z
Learnt from: rahulbarwal
Repo: appsmithorg/appsmith PR: 40886
File: app/client/src/widgets/ListWidgetV2/MetaWidgetGenerator.ts:1115-1134
Timestamp: 2025-06-13T07:49:21.417Z
Learning: For ListWidgetV2, an empty array supplied by users is internally converted to `[{}]` (a placeholder row) when `pageNo === 1`; this sentinel is never produced by user data, so a singleton empty object on page 1 can be safely treated as “empty list” in MetaWidgetGenerator.
Applied to files:
app/client/src/widgets/ListWidgetV2/widget/index.tsx
📚 Learning: 2024-11-13T09:11:36.959Z
Learnt from: vhemery
Repo: appsmithorg/appsmith PR: 37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js:35-37
Timestamp: 2024-11-13T09:11:36.959Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_base64_spec.js`, when writing Cypress end-to-end tests for the Image Widget in JavaScript, use `viewWidgetsPage.imageinner` as the selector when asserting 'src' and 'alt' attributes to maintain consistency.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2025-11-05T12:56:49.249Z
Learnt from: vsvamsi1
Repo: appsmithorg/appsmith PR: 41363
File: app/client/cypress/e2e/Regression/ClientSide/StaticUrl/PagePersistance_Spec.ts:0-0
Timestamp: 2025-11-05T12:56:49.249Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/StaticUrl/PagePersistance_Spec.ts`, the test suite intentionally uses sequential tests where Tests 4-6 depend on state (`page1Slug`, `page2Slug`) set in Tests 2-3. The author (vsvamsi1) prioritizes readability by breaking the flow into different segments over strict test isolation.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2025-11-05T12:58:32.486Z
Learnt from: vsvamsi1
Repo: appsmithorg/appsmith PR: 41363
File: app/client/cypress/e2e/Regression/ClientSide/StaticUrl/SlugPersistance_Spec.ts:32-225
Timestamp: 2025-11-05T12:58:32.486Z
Learning: In the file `app/client/cypress/e2e/Regression/ClientSide/StaticUrl/SlugPersistance_Spec.ts`, the test suite intentionally uses sequential tests where Tests 2-5 depend on state from previous tests (e.g., slug values modified in earlier tests). The author (vsvamsi1) prioritizes readability by breaking the flow into different segments over strict test isolation.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: abhishek-bandameedi
Repo: appsmithorg/appsmith PR: 35133
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Checkbox/CheckBox2_spec.js:0-0
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In Cypress tests for the Appsmith project, ensure the use of locator variables for selectors and include multiple assertions for comprehensive testing.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: sagar-qa007
Repo: appsmithorg/appsmith PR: 34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-10-08T15:32:34.115Z
Learning: Learnt from: sagar-qa007
PR: appsmithorg/appsmith#34955
File: app/client/cypress/e2e/Regression/ClientSide/ActionExecution/General_settings_Spec.ts:14-56
Timestamp: 2024-07-16T06:44:55.118Z
Learning: Avoid using sleep functions like `agHelper.Sleep`, `this.Sleep`, and other related sleep functions in Cypress tests within the `app/client/cypress` directory to prevent non-deterministic behaviors and ensure tests are more reliable and maintainable.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2024-07-26T21:12:57.228Z
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Radio/Radio2_spec.ts:404-407
Timestamp: 2024-07-26T21:12:57.228Z
Learning: The `Radio2_spec.ts` file is not part of the initial phase for the removal of static waits in the pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1".
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2024-11-13T09:07:54.931Z
Learnt from: vhemery
Repo: appsmithorg/appsmith PR: 37371
File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Image/Image_spec.js:44-47
Timestamp: 2024-11-13T09:07:54.931Z
Learning: When reviewing code in `app/client/cypress/**` paths, ensure that suggestions are appropriate for test code and not for `src` code. Avoid applying source code checks to test code unless relevant.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
📚 Learning: 2024-10-08T15:32:39.374Z
Learnt from: Aishwarya-U-R
Repo: appsmithorg/appsmith PR: 29405
File: app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js:37-41
Timestamp: 2024-10-08T15:32:39.374Z
Learning: The pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1" is part of a phased approach where only certain test specifications are targeted for static wait removal in the initial phase. Future phases will address additional specs.
Applied to files:
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts
🧬 Code graph analysis (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts (1)
app/client/cypress/support/commands.js (2)
_(1071-1071)locators(43-43)
🔇 Additional comments (9)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (5)
783-791: Solid implementation for stable item identification.Using
listItemId(stable primary key from List widget) over positional index prevents the mapping issues flagged in prior reviews. The fallback toString(index ?? 0)correctly handles standalone usage outside a List widget.
843-871: Clean per-item initialization logic.This addresses the prior review feedback—no index-shifting, just on-demand initialization for missing entries. The reference equality check at line 868 avoids unnecessary meta property updates.
948-966: LGTM.Correctly updates per-item state and marks the widget dirty on user interaction.
1058-1061: Interface additions look correct.Optional properties maintain backward compatibility. Types align with their usage throughout the widget.
969-980: Verify type compatibility and function intent inmergeLabelAndValue.The concern about type mismatch between the return type
LabelInValueType[]anddefaultOptionValue(potentiallystring[] | OptionValue[]) may be valid based on known patterns in Appsmith's MultiSelect handling, but this requires inspection of:
- The actual type definition of
defaultOptionValueat line 1040- How
mergeLabelAndValue()is used across the codebase- Whether this is intentional fallback behavior or a latent bug
Additionally, verify if the function name
mergeLabelAndValuestill accurately describes its behavior as a simple getter.app/client/src/widgets/ListWidgetV2/widget/index.tsx (2)
80-85: Clean type extension.Adding
listItemIdtoBaseMetaWidgetenables per-item identity propagation to child widgets without breaking existing consumers.
728-736: Properly wires stable item identity.Using
getPrimaryKeyforlistItemIdensures child widgets (like MultiSelect) can key their per-item state to stable identifiers rather than volatile indices.app/client/cypress/e2e/Regression/ClientSide/BugTests/MultiSelectWidget_Bug41210_Spec.ts (2)
11-38: Test setup is well-structured.The JSObject provides controlled test data and a deletion mechanism. Using
toRun: falseavoids unnecessary execution during setup.
58-83: Good coverage of the deletion edge case.This test verifies that per-item state maps correctly after item removal—a critical scenario for the bug fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1)
779-779: Consider cleanup logic for removed list items.
selectedValuesByItemaccumulates entries for every item visited, but no cleanup occurs when list items are deleted or the list shrinks. For dynamic lists with frequent changes, this map grows unbounded.Consider adding cleanup logic to prune stale entries, either:
- When list size decreases (compare previous/current list length in
componentDidUpdate)- Periodically cap the map size by removing oldest entries
- Expose a method to reset per-item state
Not a blocker for this PR given the Chill review mode, but worth tracking for future optimization.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(5 hunks)
🔇 Additional comments (3)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (3)
783-791: LGTM - Clean stable ID fallback.The helper correctly prioritizes stable
listItemIdwhen available and falls back to index-based keys, ensuring consistent string keys for the selection map.
843-871: LGTM - Clean on-demand initialization.
syncSelectionMapOnIndexChangecorrectly initializes missing per-item entries without attempting to reorganize or shift existing entries. The stable key approach viagetListItemIdprevents the index-mapping issues flagged in prior reviews.Minor note:
previousRowIndexparameter is unused but doesn't hurt.
824-835: Type mismatch:defaultOptionValueassigned toLabelInValueType[]without conversion.
defaultOptionValueis typed asstring[] | OptionValue[](line 1040), but at lines 828 and 863 it's assigned/cast toLabelInValueType[]without conversion. WhendefaultOptionValueis a string array like["GREEN", "RED"], it doesn't match the{ label, value }shape expected byLabelInValueType[].Verify that string arrays are properly transformed before storage, or update the type of
selectedValuesByItemto accept both formats.
saharAdem
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (1)
839-867: Good: Simplified initialization logic, but consider cleanup for large lists.The removal of shifting logic addresses the critical data-loss issue from previous reviews. The current implementation only initializes missing entries, which is correct.
However,
selectedValuesByItemgrows unbounded as users navigate items. For lists with thousands of items, this could accumulate significant state over time.Consider adding cleanup logic to remove entries for items that no longer exist in the list, possibly triggered when the list size decreases or on a periodic basis.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(5 hunks)
🔇 Additional comments (5)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (5)
774-781: LGTM: Meta property correctly added.The
selectedValuesByItemmeta property is properly initialized as an empty object to support per-item selection tracking.
783-791: Good: Stable ID with defensive fallback.The helper correctly prioritizes stable
listItemIdover volatilecurrentIndex, addressing the critical mapping issue from previous reviews. The fallback to"0"when both are undefined ensures keys are always defined.
944-962: LGTM: Per-item selection update is correctly implemented.The method properly:
- Uses stable item IDs via
getListItemId- Preserves selections for other items
- Triggers the appropriate action
- Sets the dirty flag
965-976: LGTM: Per-item value retrieval with proper fallback.The method correctly retrieves per-item values using the stable ID and gracefully falls back to
defaultOptionValuefor uninitialized items.
1054-1056: LGTM: Interface extensions are properly typed.The new properties are correctly typed and appropriately marked as optional since they're only relevant when the MultiSelect is inside a List widget.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (2)
842-870: Good refactor: removed problematic index-shifting logic.The on-demand initialization approach is much simpler and avoids the data loss issues from the previous implementation.
Consider: Reviewer feedback mentioned adding cleanup logic for removed list items to prevent memory growth. Currently,
selectedValuesByItementries are never pruned. For large dynamic lists with frequent item changes, this could accumulate stale data over time.
968-979: Function name is now misleading.After this change,
mergeLabelAndValueno longer merges anything—it simply retrieves the selected values for the current item. Consider renaming togetSelectedValuesor similar to reflect actual behavior.- mergeLabelAndValue = (): LabelInValueType[] => { + getSelectedValues = (): LabelInValueType[] => { const { currentIndex = 0, defaultOptionValue = [], selectedValuesByItem = {}, } = this.props; const itemId = this.getListItemId(currentIndex); const values = selectedValuesByItem[itemId] ?? defaultOptionValue; return values; };Also update the call site at line 902.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-30T07:17:49.646Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883
Timestamp: 2025-10-30T07:17:49.646Z
Learning: In app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx, the isErrorPersistingAppSlug flag is unused and should be removed. There is no corresponding selector to expose it, and no handlers set it to true.
Applied to files:
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx
🔇 Additional comments (6)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (6)
774-781: LGTM!Meta property correctly added for per-item selection tracking.
783-791: LGTM!Helper properly prioritizes stable
listItemIdover volatilecurrentIndex, with safe fallback handling.
820-840: LGTM!The per-item existence check correctly preserves user selections while allowing defaults for unvisited items. Addresses the previous review concerns about global
isDirtyblocking unvisited items.
947-965: LGTM!Per-item selection storage correctly implemented. Key derivation via
getListItemIdensures consistency across the widget.
1057-1059: LGTM!Interface correctly extended with the new props. Addresses reviewer feedback about missing interface properties.
976-978: Verify type compatibility betweendefaultOptionValueand return type.
defaultOptionValueis typed asstring[] | OptionValue[], but this function returnsLabelInValueType[]. When falling back todefaultOptionValue, there's a potential type mismatch.Ensure
MultiSelectComponenthandles both formats correctly, or normalize the value before returning.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (3)
836-839: Optimize: Only sync when currentIndex actually changes.
syncSelectionMapOnIndexChangeis called on everycomponentDidUpdate, even whencurrentIndexhasn't changed. This is inefficient—the method will perform a no-op check each time. Gate the call with a comparison:+ if (prevProps.currentIndex !== this.props.currentIndex) { this.syncSelectionMapOnIndexChange( prevProps.currentIndex, this.props.currentIndex, ); + }This preserves correctness while avoiding unnecessary work.
842-845: Minor: Remove unusedpreviousRowIndexparameter.The
previousRowIndexparameter is never used in the method body—it's a vestigial artifact from the removed shifting logic. Remove it to improve clarity:private syncSelectionMapOnIndexChange( - previousRowIndex?: number, currentRowIndex?: number, ) {And update the call site accordingly (line 836).
820-834: Consider consolidating initialization paths.Two code paths can initialize
selectedValuesByItementries in the same update cycle:
- Lines 820–834 when
defaultOptionValuechanges for an unvisited item- Lines 857–865 in
syncSelectionMapOnIndexChangewhen navigating to an unvisited itemBoth read
this.props.selectedValuesByItem, compute new state, and callupdateWidgetMetaProperty. If they fire simultaneously (e.g., navigating to a new item whiledefaultOptionValuechanges), they could issue redundant or conflicting updates depending on framework batching behavior.While both produce the same end result in practice, the overlapping responsibility is a maintenance risk. Consider refactoring so only one path owns item initialization—for example, remove lines 820–834 and rely solely on
syncSelectionMapOnIndexChange, or vice versa.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-30T07:17:49.646Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883
Timestamp: 2025-10-30T07:17:49.646Z
Learning: In app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx, the isErrorPersistingAppSlug flag is unused and should be removed. There is no corresponding selector to expose it, and no handlers set it to true.
Applied to files:
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx
🔇 Additional comments (3)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (3)
783-791: LGTM: Clean item identification with appropriate fallbacks.The helper correctly prioritizes
listItemIdfor stable identification and gracefully handles undefined indices by defaulting to "0". The null checks are sound and the string conversion is consistent with the map key type.
947-965: LGTM: Per-item state management is correct.The
onOptionChangeandgetSelectedValuesmethods correctly usegetListItemIdfor consistent keying, properly handle the per-item map, and maintain appropriate fallback behavior. The empty array case is intentionally handled as a valid selection state.Also applies to: 968-979
1057-1059: LGTM: Props interface additions align with implementation.The new properties
currentIndex,selectedValuesByItem, andlistItemIdare correctly typed and match their usage throughout the component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (2)
836-837: Optimize: Sync is called on every update, not just index changes.
syncSelectionMapOnIndexChangeis invoked on everycomponentDidUpdatewithout checking ifcurrentIndexactually changed. While the method has internal guards, this creates unnecessary overhead.Apply this diff to only sync when the index changes:
componentDidUpdate(prevProps: MultiSelectWidgetProps): void { // Check if defaultOptionValue is string let isStringArray = false; // ... existing hasChanges logic ... if (hasChanges) { const listItemId = this.getListItemId(this.props.currentIndex); if (!(listItemId in (this.props.selectedValuesByItem || {}))) { const updatedSelectedValuesByItem = { ...(this.props.selectedValuesByItem || {}), [listItemId]: this.props.defaultOptionValue, }; this.props.updateWidgetMetaProperty( "selectedValuesByItem", updatedSelectedValuesByItem, ); } } - this.syncSelectionMapOnIndexChange(this.props.currentIndex); + if (prevProps.currentIndex !== this.props.currentIndex) { + this.syncSelectionMapOnIndexChange(this.props.currentIndex); + } }Also applies to: 839-864
779-779: Consider implementing cleanup for removed list items.The
selectedValuesByItemmap will accumulate entries over time without cleanup when list items are deleted. For dynamic lists with frequent additions/removals, this could lead to gradual memory growth.Consider adding cleanup logic that:
- Tracks the current set of valid list item IDs
- Periodically or on update, removes stale entries not in the current set
- Balances cleanup frequency against performance
This is more critical for large dynamic lists than static ones.
Also applies to: 839-864
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-30T07:17:49.646Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883
Timestamp: 2025-10-30T07:17:49.646Z
Learning: In app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx, the isErrorPersistingAppSlug flag is unused and should be removed. There is no corresponding selector to expose it, and no handlers set it to true.
Applied to files:
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx
🔇 Additional comments (7)
app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (7)
779-779: LGTM: Meta property registration is correct.The
selectedValuesByIteminitialization as an empty object is appropriate for per-item state tracking.
820-834: LGTM: Default value initialization respects user selections.The logic correctly initializes defaults only for unvisited items, preserving existing user selections. Previous concerns about
isDirtyhave been properly addressed.
896-896: LGTM: View correctly uses per-item values.The widget view now retrieves values from
getSelectedValues(), which properly implements per-item state.
941-959: LGTM: Option change correctly updates per-item state.The handler properly isolates changes to the current list item while preserving selections for other items.
962-973: LGTM: Value retrieval logic is correct.The method properly retrieves per-item selections with an appropriate fallback to defaults using nullish coalescing.
1051-1053: LGTM: Interface additions are well-typed.The new props are appropriately optional and correctly typed for per-item state management.
783-791: Verify handling when bothlistItemIdandcurrentIndexare undefined.When both props are undefined, the method returns
"0", which could cause multiple items to share the same key. Confirm thatcurrentIndexis always defined when this widget is used inside a List widget, and verify the behavior of the guard at line 846 to ensure this scenario doesn't occur in practice.
|
@vivek-appsmith @jacquesikot PR Comments resolved, Could you please review the PR? |
4 participants
Description
This PR resolves MultiSelect widgets issue, where MultiSelect widgets inside a ListWidget lose their selected values when switching between list items. The fix ensures that selected values are persisted per list item, maintaining user selections as they navigate through the list
Fixes #41210
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.