[fix] Programmatically closed popovers/expanders reopening on interaction with another#14945
Conversation
#14943) When a popover or expander was closed programmatically via st.session_state.key = False, the frontend updated its local React state but did not update the WidgetStateManager. On the next rerun (triggered by another widget), the stale "open=true" value was sent back to the backend, causing the closed popover/expander to reopen. Fix: call widgetMgr.setBoolValue with fromUi=false when the backend overrides the open/expanded state, keeping the widget manager in sync without triggering an additional rerun. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Orca <help@stably.ai>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
sfc-gh-kmcgrady
added
change:bugfix
|
| Filename | Overview |
|---|---|
| frontend/lib/src/components/elements/Popover/Popover.tsx | Adds widgetMgr?.setBoolValue(…, { fromUi: false }) inside the existing useExecuteWhenChanged callback to sync the widget manager on backend-driven open-state changes; logic is guarded by widgetId and notNullOrUndefined checks. |
| frontend/lib/src/components/elements/Expander/Expander.tsx | Adds a new useExecuteWhenChanged block (mirroring the Popover fix) that syncs the widget manager when element.expanded changes programmatically; properly guarded and uses fromUi: false to avoid triggering a rerun. |
| frontend/lib/src/components/elements/Popover/Popover.test.tsx | Updates existing 'syncs open state' test to assert the new setBoolValue call, adds a new 'programmatic close' test; the previously-passing not.toHaveBeenCalled assertion is correctly replaced to reflect the new intended behaviour. |
| frontend/lib/src/components/elements/Expander/Expander.test.tsx | Adds two new unit tests ('syncs widget manager state on programmatic expand/collapse') that directly assert the setBoolValue call arguments for both expand and collapse scenarios. |
| e2e_playwright/st_expander.py | Adds two stateful expanders with programmatic-close buttons for the regression E2E test; straightforward fixture addition. |
| e2e_playwright/st_expander_test.py | Adds test_programmatic_close_does_not_reopen_other_expander E2E test and bumps NUMBER_OF_EXPANDERS from 22 to 24 to account for the two new fixture expanders. |
| e2e_playwright/st_popover.py | Adds two stateful popovers with programmatic-close buttons for the regression E2E test. |
| e2e_playwright/st_popover_test.py | Adds test_programmatic_close_does_not_reopen_other_popover E2E test and bumps expected popover count from 25 to 27. |
Sequence Diagram
sequenceDiagram
participant User
participant Frontend
participant WidgetMgr as WidgetStateManager
participant Backend
User->>Frontend: Click "Close A" button
Frontend->>WidgetMgr: setBoolValue(popoverA, false, fromUi:true)
WidgetMgr->>Backend: sendRerunBackMsg (open=false)
Backend->>Frontend: Re-render with element.open=false
Frontend->>WidgetMgr: setBoolValue(popoverA, false, fromUi:false) NEW
Note over WidgetMgr: Widget manager now has correct false value
User->>Frontend: Click to open Popover B
Frontend->>WidgetMgr: setBoolValue(popoverB, true, fromUi:true)
WidgetMgr->>Backend: sendRerunBackMsg (popoverB=true, popoverA=false)
Backend->>Frontend: Re-render (A stays closed, B opens)
Reviews (1): Last reviewed commit: "[fix] Sync widget manager state on progr..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
Fixes a regression where Popover/Expander components that are programmatically closed (via st.session_state.<key> = False) could reopen on a later rerun because the frontend WidgetStateManager retained a stale true state. This aligns the frontend widget manager state with backend-driven open/expanded updates so subsequent reruns send the correct value back.
Changes:
- Sync
WidgetStateManagerboolean state when backend programmatically updates Popover open state and Expander expanded state (using{ fromUi: false }to avoid triggering reruns). - Add/adjust frontend unit tests to assert widget manager syncing for programmatic open/close and expand/collapse.
- Add E2E regression tests covering multi-popover and multi-expander programmatic close scenarios; update expected element counts in existing E2E tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/lib/src/components/elements/Popover/Popover.tsx | Sync widget manager bool state when backend changes element.open. |
| frontend/lib/src/components/elements/Popover/Popover.test.tsx | Update/add tests to verify widget manager sync on programmatic open/close. |
| frontend/lib/src/components/elements/Expander/Expander.tsx | Sync widget manager bool state when backend changes element.expanded. |
| frontend/lib/src/components/elements/Expander/Expander.test.tsx | Add tests to verify widget manager sync on programmatic expand/collapse. |
| e2e_playwright/st_popover_test.py | Add regression E2E test; update expected popover count. |
| e2e_playwright/st_popover.py | Add multi-popover app scenario exercising programmatic close via session state. |
| e2e_playwright/st_expander_test.py | Add regression E2E test; update expected expander count. |
| e2e_playwright/st_expander.py | Add multi-expander app scenario exercising programmatic close via session state. |
github-actions
Bot
removed
the
ai-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR fixes a state-sync regression (#14943) where programmatically closing a widget-mode popover or expander (e.g. st.session_state.key = False) would leave stale frontend widget state, causing the element to reopen when another stateful widget triggered a rerun. The fix updates the WidgetStateManager when the backend-controlled open/expanded value changes, using { fromUi: false } to avoid triggering extra reruns. Both unit tests and E2E regression tests are added for the popover and expander flows.
Reviewer agreement: Both available reviewers (gemini-3.1-pro and gpt-5.3-codex-high) agree the fix is logically correct, well-scoped, and backed by strong test coverage. They agree on backwards compatibility, security, accessibility, and external-test assessment (no risk). The sole disagreement is on implementation approach — see Code Quality.
Missing review: claude-4.6-opus-high-thinking failed to complete review.
Code Quality
Both reviewers confirm the root cause is correctly identified and the fix is sound: syncing widgetMgr state when the backend overrides open/expanded prevents stale values from being sent on subsequent reruns.
Point of disagreement: gemini-3.1-pro flags that calling widgetMgr.setBoolValue() inside useExecuteWhenChanged violates React's render purity rules, since WidgetStateManager is an external store. It recommends moving the sync to a useEffect. gpt-5.3-codex-high found no issues and considers the implementation clean and consistent with existing patterns.
Consolidation assessment: After reviewing the codebase, useExecuteWhenChanged is an established render-time hook used in 8+ components (Slider, Selectbox, BaseColorPicker, DeckGlJsonChart, Sidebar, AppView, etc.). Notably, Sidebar.tsx already calls an external callback (onToggleCollapse) during render via this same hook — so the pattern of triggering external side effects in useExecuteWhenChanged has existing precedent. The widgetMgr.setBoolValue() call with { fromUi: false } is effectively idempotent and does not trigger reruns, making the practical risk from React concurrent mode negligible. While gemini's observation about render purity is technically valid, it does not represent a novel anti-pattern in this codebase. Moving the sync to useEffect could also introduce a one-frame timing gap that might re-expose the stale-state bug in edge cases. This is noted as a non-blocking suggestion in the inline comments.
Test Coverage
Test coverage is excellent and directly targets the regression:
- Unit tests: New tests in
Expander.test.tsxandPopover.test.tsxverify that programmatic state changes (both open→close and close→open) updatewidgetMgrwith{ fromUi: false }. Tests include both positive and negative assertions (stale reopen prevention). - E2E tests: New Playwright regression tests in
st_expander_test.pyandst_popover_test.pyreproduce the exact scenario from #14943 — programmatically closing one element, then interacting with another, and verifying the first stays closed. - Existing element count assertions are correctly updated to account for newly added test widgets (22→24 expanders, 25→27 popovers).
Backwards Compatibility
No breaking changes. The fix only corrects state synchronization behavior and does not alter any public APIs or expected component behaviors. Users will only observe corrected behavior in the previously broken scenario.
Security & Risk
No security-sensitive surfaces were modified. No changes to auth, cookies, routing, WebSocket transport, CSP/headers, file serving, external networking, eval/exec paths, or dependencies. Regression risk is low and mitigated by targeted unit and E2E tests.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
frontend/lib/src/components/elements/Popover/Popover.tsx: local frontend widget-state sync fix only; no cross-origin, routing, auth, transport, storage, or embedding-boundary changes.frontend/lib/src/components/elements/Expander/Expander.tsx: same class of frontend state-sync fix with{ fromUi: false }; no external-host/runtime integration points changed.
- Suggested external_test focus areas: N/A
- Confidence: High
- Assumptions and gaps: Assessment is based on this PR diff only; changes are strictly confined to frontend widget state management.
Accessibility
No accessibility impact. The changes only affect internal state synchronization and do not modify the DOM structure, ARIA attributes, focus behavior, or keyboard interactions.
Recommendations
- Non-blocking: Consider moving the
widgetMgr.setBoolValue()call to auseEffectin bothExpander.tsxandPopover.tsxto align with strict React render purity. This is a codebase-wide pattern question (otheruseExecuteWhenChangedcallsites also trigger external side effects) and not specific to this PR. See inline comments for details.
Verdict
APPROVED: The fix correctly addresses the root cause of #14943 with a well-scoped, idempotent state sync and comprehensive test coverage. The render-purity concern is noted but non-blocking given established codebase patterns and negligible practical risk.
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from gemini-3.1-pro (CHANGES REQUESTED) and gpt-5.3-codex-high (APPROVED). claude-4.6-opus-high-thinking failed to complete individual review.
This review also includes 2 inline comment(s) on specific code lines.
| // expanded value (e.g. st.session_state.key = False). Without this, the | ||
| // widget manager retains the stale value and sends it back on the next | ||
| // rerun, causing the expander to revert to its old state. | ||
| useExecuteWhenChanged(() => { |
There was a problem hiding this comment.
thought: Calling widgetMgr.setBoolValue() during render (via useExecuteWhenChanged) technically violates React's render purity rules, since WidgetStateManager is an external store. The project's frontend/AGENTS.md guidelines state: "Use an Effect only to synchronize the component with an external system" — which implies external-system sync belongs in a useEffect. That said, this pattern has existing precedent in the codebase (e.g. Sidebar.tsx calls onToggleCollapse via useExecuteWhenChanged), and the call here is idempotent with { fromUi: false }, so the practical risk is negligible. Non-blocking, but worth considering as a broader codebase cleanup.
There was a problem hiding this comment.
Acknowledged — intentionally keeping this in useExecuteWhenChanged rather than moving to useEffect for two reasons:
- Timing: splitting
setOpen(render-time) andsetBoolValue(useEffect) would create a window where the local state is updated but the widget manager is not. If another widget triggers a flush in that gap, the stale value gets sent to the backend — exactly the bug this PR fixes. - Precedent:
Sidebar.tsxalready callsonToggleCollapse(an external side effect) through this same hook. The call here is idempotent and uses{ fromUi: false }so it cannot trigger reruns or cascading renders.
Agree this is worth revisiting as a broader codebase cleanup if the project moves toward stricter render purity, but not in scope for this bug fix.
| // on subsequent reruns. Without this, a programmatic close (e.g. | ||
| // st.session_state.key = False) would leave a stale "true" in the widget | ||
| // state, causing the popover to reopen when another widget triggers a rerun. | ||
| widgetMgr?.setBoolValue( |
There was a problem hiding this comment.
thought: Same pattern as Expander — widgetMgr.setBoolValue() is called during render via useExecuteWhenChanged. The setOpen(element.open) part is appropriate for render-time state adjustment, but the widget manager mutation is an external side effect. If this is ever refactored, consider splitting: keep setOpen in useExecuteWhenChanged and move widgetMgr.setBoolValue() to a useEffect. Non-blocking given established codebase conventions.
There was a problem hiding this comment.
Same rationale as the Expander comment — keeping both calls together in useExecuteWhenChanged avoids a timing gap where a concurrent flush could read stale widget state. The { fromUi: false } source ensures no rerun is triggered.
There was a problem hiding this comment.
Summary
This PR fixes a state-sync bug (#14943) where programmatically closing a widget-mode popover or expander (e.g. st.session_state.key = False) would leave stale frontend widget state, causing the element to reopen when another stateful widget triggered a rerun. The fix adds a widgetMgr.setBoolValue(..., { fromUi: false }) call inside the existing useExecuteWhenChanged hook in both Popover.tsx and Expander.tsx, ensuring the widget manager stays in sync with backend-driven state changes without triggering additional reruns. Both unit tests and E2E regression tests are added.
Reviewer consensus: All three reviewers (claude-4.6-opus-high-thinking, gemini-3.1-pro, gpt-5.3-codex-high) agree the root-cause analysis is correct, the fix is well-scoped, test coverage is thorough, and there are no backwards compatibility, security, or accessibility concerns. The sole disagreement is on implementation approach — see Code Quality.
Code Quality
All reviewers confirm the root cause is correctly identified: the WidgetStateManager retained stale state when the backend overrode open/expanded, and syncing it with { fromUi: false } prevents stale values from being sent on subsequent reruns.
Point of disagreement — render purity: gemini-3.1-pro flags that calling widgetMgr.setBoolValue() inside useExecuteWhenChanged violates React's render purity rules and requests changes to move the sync to a useEffect. Both claude-4.6-opus-high-thinking and gpt-5.3-codex-high find the implementation clean and consistent with existing patterns, noting no issues.
Consolidation assessment: After verifying the codebase, useExecuteWhenChanged is an established render-time hook used in 8+ components (Slider, Selectbox, BaseColorPicker, DeckGlJsonChart, Sidebar, AppView, etc.). Notably, Sidebar.tsx already calls an external callback (onToggleCollapse) during render via this same hook — so triggering external side effects in useExecuteWhenChanged has existing precedent. The setBoolValue call with { fromUi: false } is effectively idempotent and does not trigger reruns, making the practical risk from React concurrent mode negligible. Additionally, moving the sync to useEffect could introduce a one-frame timing gap that might re-expose the stale-state bug in edge cases. While gemini's observation about render purity is technically valid, it does not represent a novel anti-pattern in this codebase and is assessed as non-blocking. See inline comments for details.
Test Coverage
Test coverage is excellent and directly targets the regression:
- Frontend unit tests: New tests in
Expander.test.tsxverify programmatic expand and collapse updatewidgetMgrwith{ fromUi: false }. Updated and new tests inPopover.test.tsxcover programmatic open and close with the same assertions. Tests include both positive and negative assertions (stale reopen prevention). - E2E tests: New Playwright regression tests in
st_expander_test.pyandst_popover_test.pyreproduce the exact bug scenario from #14943 — programmatically close one element, interact with another, verify the first stays closed. - Existing element count assertions are correctly updated (22→24 expanders, 25→27 popovers).
Backwards Compatibility
No breaking changes. The fix only corrects state synchronization behavior for widget-mode popovers and expanders. Non-widget-mode and passively-keyed components are unaffected due to the !widgetId guard. Users will only observe corrected behavior in the previously broken scenario.
Security & Risk
No security-sensitive surfaces were modified. No changes to auth, cookies, routing, WebSocket transport, CSP/headers, file serving, external networking, eval/exec paths, or dependencies. Regression risk is low and mitigated by targeted unit and E2E tests.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
frontend/lib/src/components/elements/Popover/Popover.tsx: Pure frontend widget state sync change — no routing, auth, embedding, cross-origin, or transport impact.frontend/lib/src/components/elements/Expander/Expander.tsx: Same scope — widget manager sync during render with{ fromUi: false }.
- Suggested external_test focus areas: N/A
- Confidence: High
- Assumptions and gaps: None. Changes are strictly confined to React component render logic and widget state management, which behave identically in local and externally hosted contexts.
Accessibility
No accessibility impact. The changes are entirely in widget state management logic — no DOM structure, ARIA attributes, focus management, or keyboard interaction changes.
Recommendations
- Non-blocking: The render-purity concern about calling
widgetMgr.setBoolValue()insideuseExecuteWhenChangedis noted but assessed as non-blocking given established codebase patterns and negligible practical risk. This could be addressed as a broader codebase cleanup in a future PR. See inline comments for details. - Non-blocking: The two new Expander widget-sync tests are placed inside the
describe("passive state persistence", ...)block but test widget-mode behavior. They would be more logically grouped under the existingdescribe("widget mode (widgetMgr + element.id)", ...)block for consistency withPopover.test.tsx.
Verdict
APPROVED: Well-targeted bug fix with correct root-cause analysis, consistent implementation across Popover and Expander, thorough unit and E2E test coverage, and no backwards compatibility or security concerns. The render-purity observation is non-blocking given established codebase conventions.
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from claude-4.6-opus-high-thinking (APPROVED), gemini-3.1-pro (CHANGES REQUESTED), and gpt-5.3-codex-high (APPROVED).
This review also includes 3 inline comment(s) on specific code lines.
Address review feedback: the two new Expander unit tests for programmatic state sync belong in "widget mode" not "passive state persistence". Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Orca <help@stably.ai>
kmcgrady
changed the title
sfc-gh-lwilby-1
deleted the
fix/popover-expander-programmatic-close-stale-state
branch
…tion with another (streamlit#14945) ## Summary - Fixes streamlit#14943 — programmatically closed popovers reopen when interacting with another stateful popover - When `st.session_state.key = False` closes a popover/expander, the frontend's `WidgetStateManager` was not updated, leaving a stale `true` value that got sent back to the backend on the next rerun - Added `widgetMgr.setBoolValue(..., { fromUi: false })` to sync the widget manager when the backend overrides the open/expanded state, without triggering an additional rerun - Applied the same fix to `st.expander` which had the identical latent bug ## Test plan - [x] Updated Popover unit test ("syncs open state when element.open changes programmatically") to verify widget manager sync - [x] Added Popover unit test ("syncs widget manager state on programmatic close to prevent stale reopens") - [x] Added Expander unit test ("syncs widget manager state on programmatic expand change") - [x] Added Expander unit test ("syncs widget manager state on programmatic collapse to prevent stale reopens") - [x] Added Popover E2E test (`test_programmatic_close_does_not_reopen_other_popover`) - [x] Added Expander E2E test (`test_programmatic_close_does_not_reopen_other_expander`) - [x] All existing Popover/Expander unit tests pass (49/49) - [x] Both new E2E tests pass - [x] Verified manually via Playwright against `make debug` that the fix resolves the reported issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Made with [Orca](https://github.com/stablyai/orca) 🐋 --------- Co-authored-by: Orca <help@stably.ai>
Summary
st.session_state.key = Falsecloses a popover/expander, the frontend'sWidgetStateManagerwas not updated, leaving a staletruevalue that got sent back to the backend on the next rerunwidgetMgr.setBoolValue(..., { fromUi: false })to sync the widget manager when the backend overrides the open/expanded state, without triggering an additional rerunst.expanderwhich had the identical latent bugTest plan
test_programmatic_close_does_not_reopen_other_popover)test_programmatic_close_does_not_reopen_other_expander)make debugthat the fix resolves the reported issue🤖 Generated with Claude Code
Made with Orca 🐋