✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Looks great, thanks for the video! Approving from product side, will let the eng team give this a review.

@cursor review

Summary

This PR fixes issue #13317 where the scroll position in st.multiselect was being reset when removing an item. The fix introduces scroll position tracking via React refs (valueContainerRef and scrollTopRef) and restores the scroll position using useLayoutEffect. The implementation correctly overrides BaseWeb's ValueContainer component to attach the scroll tracking handler.

Key changes:

• Added two refs to track the scroll container and scroll position
• Created a custom ValueContainer component with scroll event handling
• Used useLayoutEffect to restore scroll position after renders
• Added comprehensive unit and E2E tests

Code Quality

The code is well-structured and follows Streamlit's established patterns:

Positive aspects:

• Refs are correctly named with Ref suffix (valueContainerRef, scrollTopRef) per TypeScript guidelines
• Event handler follows naming convention (handleValueContainerScroll)
• useCallback is used appropriately for the scroll handler with empty dependencies
• useMemo is correctly used to prevent BaseWeb from remounting the ValueContainer on every render
• ESLint disable comments include clear justifications explaining why the rules are disabled

Implementation details (Multiselect.tsx lines 227-257):

useLayoutEffect(() => {
  if (valueContainerRef.current) {
    valueContainerRef.current.scrollTop = scrollTopRef.current
  }
})

The useLayoutEffect without a dependency array is intentional - it needs to run after every render to restore scroll position after React updates the DOM (e.g., when an item is removed). This is an appropriate use of useLayoutEffect for DOM synchronization.

The custom ValueContainer component:
The nested component definition inside useMemo is unconventional but necessary here because BaseWeb requires a stable component reference to avoid remounting. The ESLint comment explains this trade-off clearly.

Test Coverage

Unit Tests (Multiselect.test.tsx)

✅ Good coverage - The test at lines 511-543:

• Creates a multiselect with 20 options (all selected by default)
• Mocks the scrollTop property to simulate a scrolled state
• Dispatches a scroll event to trigger the tracking mechanism
• Verifies scroll position is preserved after clicking a delete button
• Uses userEvent for interaction, following RTL best practices

E2E Tests (st_multiselect_test.py)

✅ Good coverage - The test at lines 481-503:

• Uses the existing multiselect 17 - show maxHeight fixture which has 28 pre-selected options
• Scrolls to the middle (avoiding edge cases at scroll boundaries)
• Verifies initial scroll position is > 0
• Removes an item and verifies scroll position is unchanged
• Uses existing test utilities (get_multiselect, del_from_multiselect)

Minor observation: The E2E test uses Python assert statements rather than Playwright's expect. This is acceptable here since scroll position values are retrieved via evaluate() and Playwright's expect doesn't support direct scroll position assertions. The test could potentially be improved with wait_until if flakiness is observed, but the current approach is reasonable.

Backwards Compatibility

✅ No breaking changes identified

• This is purely additive behavior
• The widget's API remains unchanged
• Visual appearance and interaction patterns are preserved
• Only the scroll behavior during item removal is affected

Security & Risk

✅ No security concerns identified

• No new external dependencies added
• No changes to user input handling
• DOM manipulation is limited to scroll position (read/write)
• The change is isolated to the Multiselect component

Risk assessment: Low. The change is well-scoped and only affects scroll behavior. The useLayoutEffect running on every render is a minor performance consideration but acceptable for this use case.

Recommendations

No blocking issues identified. Minor suggestions for consideration:

1. Consider adding a negative assertion to the unit test - Per the testing guidelines, it could be useful to verify that widgetMgr.setStringArrayValue is called with the expected reduced array, confirming the item was actually removed while the scroll was preserved.

2. Optional: Add tolerance to E2E scroll comparison - If flakiness is observed in CI, consider using a tolerance (e.g., abs(final_scroll - initial_scroll) < 1) instead of strict equality, as floating-point scroll positions may vary slightly across browsers.

Verdict

APPROVED: This is a well-implemented bug fix with proper test coverage. The code follows established patterns in the codebase, handles the scroll position preservation correctly using useLayoutEffect, and includes both unit and E2E tests that adequately cover the fix.

This is an automated AI review. Please verify the feedback and use your judgment.

Thanks for fixing this! Would love to send you a little swag as a thank you, feel free to just fill out this form:
https://docs.google.com/forms/d/e/1FAIpQLSe7cXh3H1DmrgNpVew9qViGIHX7vdEbTv5isA44_z5bgaKTKg/viewform