✅ 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.

✅ PR preview is ready!

Summary

This PR fixes an issue where the sidebar would incorrectly collapse on mobile when users click on portaled elements (dropdowns, modals, popovers, etc.) that render outside the normal DOM hierarchy. The fix modifies the click-outside handler in Sidebar.tsx to check if the click target is within the main app container ([data-testid="stApp"]), effectively excluding portaled elements since they render outside this container.

Changes:

• Sidebar.tsx: Added logic to verify clicks are inside the main app container before collapsing (lines 189-201)
• Sidebar.test.tsx: Added 4 unit tests for click-outside behavior and mocked window dimensions

Code Quality

The implementation is clean and follows TypeScript best practices:

Strengths:

• Uses optional chaining (mainAppContainer?.contains(target)) as recommended by the TypeScript guidelines
• Clear, explanatory comments describing the portal detection logic
• Added null check for target before checking containment
• The logic is straightforward and easy to understand

Minor observation (not from this PR):

• Line 216 in Sidebar.tsx includes lastInnerWidth in the useEffect dependency array, but it's not used in the effect body. This is pre-existing code and not introduced by this PR.

Test Coverage

The PR adds comprehensive unit tests for the click-outside behavior, but there's a gap:

Covered scenarios (negative cases):

1. Desktop viewport - sidebar should NOT collapse on click outside
2. Mobile viewport - clicking inside sidebar should NOT collapse it
3. Mobile viewport - clicking on portaled elements (outside main app) should NOT collapse
4. Mobile viewport - clicking when already collapsed should NOT collapse

Missing test (positive case):
The tests only verify scenarios where the sidebar should NOT collapse. There's no test verifying that the sidebar DOES collapse when clicking outside the sidebar but inside the main app container on mobile. Per the TypeScript testing guidelines:

"Positive + negative assertions (anti-regression): When you assert that something appears/changes, also add at least one complementary assertion when practical that something else does not appear/change"

The inverse also applies - testing the positive case ensures the collapse behavior isn't accidentally broken by future changes.

Recommendation: Add a test like:

it("collapses sidebar when clicking outside sidebar but inside main app on mobile", () => {
  mockWindowDimensions.innerWidth = 500  // Mobile
  
  const mockOnToggleCollapse = vi.fn()
  renderSidebar({
    isCollapsed: false,
    onToggleCollapse: mockOnToggleCollapse,
  })
  
  // Click outside sidebar but inside main app area
  const mainApp = screen.getByTestId("stApp")
  fireEvent.mouseDown(mainApp)
  
  expect(mockOnToggleCollapse).toHaveBeenCalledWith(true)
})

Other test quality notes:

• The ESLint disable comment for fireEvent.mouseDown is appropriate and well-explained
• The wrapper component correctly includes data-testid="stApp" for the fix to work
• Proper cleanup in afterEach for portal elements

Backwards Compatibility

The change is backwards compatible:

• The data-testid="stApp" selector already exists in App.tsx (line 2462)
• The behavior change is conservative - it prevents unintended collapses rather than adding new collapse triggers
• Only affects mobile viewport behavior
• Existing e2e test test_sidebar_no_collapse_on_text_input_mobile continues to pass

Security & Risk

No security concerns identified.

Low regression risk:

• The fix is additive (additional check before collapse)
• If mainAppContainer is null for some reason, the optional chaining prevents errors and simply won't trigger collapse
• The existing test for text input on mobile provides some coverage for the behavior

Recommendations

1. Add a positive test case that verifies the sidebar DOES collapse when clicking outside the sidebar but inside the main app container on mobile. This ensures the collapse behavior works as expected and prevents accidental regressions.

2. Consider adding an e2e test for the specific scenario of clicking on portaled elements (like a selectbox dropdown) on mobile to verify the fix works end-to-end. This would provide more confidence since the actual portal behavior is complex.

Verdict

CHANGES REQUESTED: The fix is well-implemented and addresses the reported issue correctly. However, adding a positive test case that verifies the collapse behavior still works when expected would strengthen the test coverage and align with the testing best practices. This is a minor addition that should be quick to implement.

This is an automated AI review using opus-4.5-thinking. Please verify the feedback and use your judgment.

📈 Frontend coverage change detected

The frontend unit test (vitest) coverage has increased by 0.0100%

• Current PR: 86.2600% (13555 lines, 1862 missed)
• Latest develop: 86.2500% (13540 lines, 1861 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

Summary

This PR fixes issue #13647 where clicking on portaled elements (dropdowns, modals, etc.) on mobile would incorrectly collapse the sidebar. The solution adds a ref (appRootRef) to the main app container and passes it through the context to the Sidebar component. The click-outside handler now checks that the click target is inside the main app container (not just outside the sidebar), effectively excluding clicks on portaled elements since they render outside the app container DOM hierarchy.

Files Changed:

• frontend/app/src/App.tsx - Adds ref to root app container and passes it through context
• frontend/app/src/components/Sidebar/Sidebar.tsx - Modifies click-outside logic to exclude portaled elements
• frontend/app/src/components/StreamlitContextProvider.tsx - Adds appRootRef prop propagation
• frontend/lib/src/components/core/SidebarConfigContext.tsx - Adds appRootRef to context interface
• frontend/app/src/components/Sidebar/Sidebar.test.tsx - Adds unit tests for click-outside behavior
• e2e_playwright/st_sidebar.py - Adds selectbox for portal testing

Code Quality

Strengths:

• The fix is elegant and uses a clean pattern of ref containment to detect portal clicks
• Good JSDoc documentation added to SidebarConfigContext.tsx (lines 77-86) explaining the purpose of appRootRef
• Proper null safety with optional chaining: appRootRef?.current?.contains(target) (line 197 in Sidebar.tsx)
• The useEffect dependency array correctly includes appRootRef (line 217 in Sidebar.tsx)
• Test refactoring in Sidebar.test.tsx improves the test setup by manually constructing context providers instead of using renderWithContexts, giving more control over the test environment

Minor Observations:

• The comment in Sidebar.tsx (lines 190-192) clearly explains the logic, which is helpful for future maintainers
• The eslint disable comment for testing-library/prefer-user-event (line 640 in test file) is appropriately scoped and justified since fireEvent.mouseDown is specifically needed to test the mousedown event handler

Test Coverage

Unit Tests (Good Coverage):
The PR adds 4 comprehensive unit tests for click-outside behavior in Sidebar.test.tsx:

1. "does not collapse sidebar when clicking outside on desktop viewport" - Validates desktop behavior
2. "does not collapse sidebar when clicking inside sidebar" - Validates sidebar click behavior
3. "does not collapse sidebar when clicking outside the main app container (portaled elements)" - Key test for the fix
4. "does not collapse when sidebar is already collapsed" - Edge case coverage

The test setup was refactored to properly create an appRootRef that wraps the Sidebar in a div with data-testid="stApp", which accurately simulates the production DOM structure.

E2E Test Gap:
A selectbox was added to e2e_playwright/st_sidebar.py (line 40) with the comment "Selectbox for testing portal behavior on mobile", but no corresponding e2e test was added to st_sidebar_test.py. The existing test_sidebar_no_collapse_on_text_input_mobile tests clicking on an element inside the sidebar, but there's no test that:

1. Opens a selectbox dropdown on mobile (which renders via portal)
2. Clicks on a dropdown option (outside the main app container)
3. Verifies the sidebar remains expanded

As noted in the unit test comments (lines 688-691), the positive case (sidebar DOES collapse when clicking outside on mobile) is meant to be tested via e2e, but the portal-specific scenario also deserves e2e coverage.

Backwards Compatibility

No breaking changes identified:

• The appRootRef property is optional in SidebarConfigContextProps (line 86: appRootRef?: RefObject<HTMLDivElement> | null)
• The default context value in SidebarConfigContext.tsx doesn't include appRootRef, which is correct since it's optional
• Existing code that doesn't provide appRootRef will continue to work - the sidebar collapse behavior would simply fall back to not checking the container (though in practice appRootRef is always provided by App.tsx)

Security & Risk

No security concerns identified.

Low regression risk:

• The fix is additive - it adds an additional condition (appRootRef?.current?.contains(target)) to the existing click-outside logic
• If appRootRef is not available (undefined), the condition appRootRef?.current?.contains(target) evaluates to undefined, which is falsy, so the sidebar won't collapse - this is safe behavior
• The change only affects mobile viewports (innerWidth <= mediumBreakpointPx)

Recommendations

1. Add E2E test for portal behavior on mobile (Recommended):
   Add a test to e2e_playwright/st_sidebar_test.py that specifically tests the fixed scenario:

   def test_sidebar_no_collapse_on_selectbox_dropdown_mobile(app: Page):
       app.set_viewport_size({"width": 400, "height": 800})
       
       # Expand the sidebar on mobile
       app.get_by_test_id("stExpandSidebarButton").click()
       
       # Click on the selectbox to open dropdown
       selectbox = app.get_by_test_id("stSidebar").get_by_test_id("stSelectbox")
       selectbox.click()
       
       # Click on a dropdown option (which is portaled outside the app container)
       dropdown_option = app.get_by_text("Option 2")
       dropdown_option.click()
       
       # Verify sidebar is still expanded
       sidebar = app.get_by_test_id("stSidebar")
       expect(sidebar).to_have_attribute("aria-expanded", "true")

2. Minor - Consider removing unused selectbox if no e2e test is added (Optional):
   If the e2e test is not added, the selectbox added to st_sidebar.py serves no testing purpose and could be removed to keep the test app minimal.

Verdict

APPROVED: This is a well-implemented fix for a real user-facing bug. The solution is elegant, properly documented, and has good unit test coverage. The code follows Streamlit's existing patterns and maintains backwards compatibility. The only gap is the missing e2e test for the portal scenario, but this is a minor issue that can be addressed separately, and the unit tests adequately cover the critical behavior. The fix is low-risk and ready for merge.

This is an automated AI review using opus-4.5-thinking. Please verify the feedback and use your judgment.