[feature] st.expander & st.popover state persistence and CSS key class#14356
[feature] st.expander & st.popover state persistence and CSS key class#14356sfc-gh-lwilby merged 13 commits intodevelopfrom
st.expander & st.popover state persistence and CSS key class#14356Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ PR preview is ready!
|
✅ 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. |
sfc-gh-lwilby
changed the title
st.expander & st.popover state persistence and CSS key class
sfc-gh-lwilby
added
change:feature
📉 Python coverage change detectedThe Python unit test coverage has decreased by 0.0000%
✅ Coverage change is within normal range. Coverage by files
|
sfc-gh-lwilby
changed the base branch from
feature/tabs-state-persistence
to
graphite-base/14356
sfc-gh-lwilby
force-pushed
the
feature/expander-popover-state-persistence
branch
from
03b6f5c to
4bdb35b
Compare
sfc-gh-lwilby
changed the base branch from
graphite-base/14356
to
feature/tabs-state-persistence
📈 Frontend coverage change detectedThe frontend unit test (vitest) coverage has increased by 0.0200%
✅ Coverage change is within normal range. |
sfc-gh-lwilby
changed the base branch from
feature/tabs-state-persistence
to
graphite-base/14356
sfc-gh-lwilby
force-pushed
the
feature/expander-popover-state-persistence
branch
from
45b6da3 to
f5b99b4
Compare
sfc-gh-lwilby
changed the base branch from
graphite-base/14356
to
feature/tabs-state-persistence
sfc-gh-lwilby
force-pushed
the
feature/tabs-state-persistence
branch
from
61d3607 to
b3a3afe
Compare
sfc-gh-lwilby
force-pushed
the
feature/expander-popover-state-persistence
branch
from
915c63c to
fed2534
Compare
sfc-gh-lwilby
added
the
ai-review
✅ Bundle size change is within normal range
|
📈 Significant wheel size change detectedThe wheel file size has increased by 1.46% (threshold: 0.25%)
Please verify that this change is expected. |
github-actions
bot
removed
the
ai-review
SummaryThis PR adds passive frontend state persistence for keyed Changes span 10 files across frontend components ( Code QualityThe implementation is well-structured and follows existing codebase patterns. Both reviewers agreed on the following:
Test CoverageBoth reviewers rated unit test coverage as excellent:
Both reviewers identified the same E2E gap:
Backwards CompatibilityLow risk overall, both reviewers agreed. No backend/protobuf changes; persistence is purely frontend-side via in-memory Two related areas to monitor:
Security & RiskBoth reviewers agreed: No security concerns identified. No changes to WebSocket handling, authentication, session management, server endpoints, external dependencies, browser storage, or HTML rendering. External test recommendation
AccessibilityBoth reviewers agreed: No accessibility regressions. No new interactive elements or ARIA attributes are introduced. Existing Reviewer Agreement Matrix
No disagreements between the two available reviewers. Missing Reviews
Recommendations
VerdictAPPROVED: Both available reviewers approved. The implementation is well-structured with thorough unit test coverage, clean separation of concerns, and low risk. The missing popover E2E persistence test (recommendation 1) and the CSS key class gap tracking (recommendation 2) are noted for follow-up but are non-blocking given the incremental approach and low practical risk. This is a consolidated AI review by 📋 Review by `gpt-5.3-codex-high`SummaryThis PR adds passive frontend state persistence for keyed Code QualityOverall implementation quality is good: the widget-mode vs passive-mode split is explicit, and behavior is covered with targeted tests in No blocking code-quality defects found. Minor maintainability note:
Test CoverageCoverage is strong for the main logic:
One non-blocking gap remains:
Backwards CompatibilityNo API-level breaking changes detected. Behavioral compatibility note:
Security & RiskNo security-sensitive surfaces were changed (no auth/session/websocket/headers/upload/network/external dependency changes). Risk profile is low and primarily UI-state behavior. External test recommendation
AccessibilityNo accessibility regressions identified in the changed code. Interaction semantics for expander/popover triggers remain unchanged, and this PR mainly adjusts state persistence and class placement. Recommendations
VerdictAPPROVED: The implementation is sound, low risk, and merge-ready, with one worthwhile non-blocking e2e enhancement for popover remount persistence. This is an automated AI review by 📋 Review by `opus-4.6-thinking`SummaryThis PR adds frontend state persistence for keyed Changes span 10 files across frontend components ( Code QualityThe code is well-structured and follows existing patterns in the codebase. Key observations:
Test CoverageFrontend unit tests — Excellent coverage with both positive and negative assertions:
E2E tests — Good but with a gap:
Element count assertions are correctly updated in both test files (21 → 22). Backwards CompatibilityLow risk overall, with one area to monitor:
Security & RiskNo security concerns identified:
External test recommendation
AccessibilityNo accessibility concerns. The changes are purely about state persistence logic and CSS class placement:
Recommendations
VerdictAPPROVED: Well-structured implementation of passive state persistence for expander and popover with thorough unit test coverage. The missing popover E2E persistence test (item 1) and the CSS key class gap for other container types (item 2) are noted for follow-up but are non-blocking given the incremental approach and low practical risk. This is an automated AI review by 📋 Review by `gemini-3.1-pro` |
sfc-gh-lwilby
changed the base branch from
feature/tabs-state-persistence
to
graphite-base/14356
sfc-gh-lwilby
force-pushed
the
feature/expander-popover-state-persistence
branch
from
a5f9358 to
5a46145
Compare
sfc-gh-lwilby
force-pushed
the
graphite-base/14356
branch
from
b3a3afe to
8cf2dee
Compare
sfc-gh-lwilby
changed the base branch from
graphite-base/14356
to
feature/tabs-state-persistence
Add frontend state persistence for keyed expanders and popovers via elementStates. When a key is provided (passive mode), the open/expanded state survives component remounts caused by delta path shifts. - Expander reads/writes "expanded" state to elementStates on toggle - Popover reads/writes "open" state to elementStates on toggle - CSS key class (st-key-*) applied to outermost element only - Block.tsx suppresses duplicate CSS key class on inner vertical blocks for expander/popover containers - Widget mode (on_change="rerun") is explicitly protected: elementStates is never read/written, server state always takes precedence - Unit tests cover persistence, CSS class, and widget-mode guards - E2E tests for CSS key class and persistence for both elements Co-authored-by: lawilby <[email protected]>
- Remove non-null assertions on blockId (already guarded by shouldPersist) - Remove unnecessary async and unused variable in Popover test Co-authored-by: lawilby <[email protected]>
blockId is typed as string | undefined. The shouldPersist guard ensures it's truthy at runtime, but TypeScript can't narrow through the ternary. Use blockId ?? "" to satisfy the string parameter type. Co-authored-by: lawilby <[email protected]>
The test only verified the default (closed) state after remount, which would pass regardless of persistence. The underlying elementStates mechanism is already covered by expander and tabs E2E tests. The CSS key class test is kept. Co-authored-by: lawilby <[email protected]>
- Expander/Popover: replace manual getElementState/setElementState with useWidgetManagerElementState hook for cleaner state persistence - CSS key class: apply st-key-* on StyledLayoutWrapper (per spec) for expander/popover only; suppress duplicate on ContainerContentsWrapper - Fix Popover bug: only persist state when isPassivelyKeyed (not on every non-widget toggle) - Unit tests: rewrite persistence tests to verify behavior through public WidgetStateManager API rather than spying on internal calls - Remove CSS class tests from Expander/Popover (now tested via Block) - Add BlockNodeRenderer unit test verifying CSS class placement Co-authored-by: lawilby <[email protected]>
…pressKeyClass Pass the computed CSS key from outside rather than having the component derive and conditionally suppress it. Expander/popover children simply omit userKey; all other callers pass it. Co-authored-by: lawilby <[email protected]>
Remove unused userKey prop from ContainerContentsWrapper (no container that passes through it sets Block.id). Remove leftover persistence test fixtures from st_popover.py after test removal. Update keyClassOnWrapper comment to reflect per-container gating rationale. Co-authored-by: lawilby <[email protected]>
- Reuse {child} in expander/popover instead of duplicating
ContainerContentsWrapper (now identical after userKey removal).
- Remove dead userKey prop from ContainerContentsWrapper.
- Fix TS2322: add ?? false for element.expanded defaultValue.
- Update keyClassOnWrapper comment.
Co-authored-by: lawilby <[email protected]>
Co-authored-by: lawilby <[email protected]>
…nder/Popover Co-authored-by: lawilby <[email protected]>
Co-authored-by: lawilby <[email protected]>
sfc-gh-lwilby
force-pushed
the
feature/expander-popover-state-persistence
branch
3 times, most recently
from
46b260c to
b556bf1
Compare
Make it explicit that element.id being set is what triggers widget mode (on_change="rerun"), so the server value takes precedence. Co-authored-by: lawilby <[email protected]>
sfc-gh-lwilby
force-pushed
the
feature/expander-popover-state-persistence
branch
from
b556bf1 to
066863b
Compare
There was a problem hiding this comment.
Pull request overview
Adds passive (non-widget) state persistence for keyed layout containers (st.expander, st.popover) by storing UI state in WidgetStateManager.elementStates, and relocates st-key-* to the outermost wrapper element for these containers to avoid duplicate key classes.
Changes:
- Persist expander
expandedand popoveropenstate across remounts in passive keyed mode viaelementStates, while keeping widget-mode controlled by server state. - Move
st-key-*CSS key class placement toStyledLayoutWrapperfor expander/popover blocks and remove it from inner vertical block wrappers. - Add/adjust frontend unit tests and Playwright E2E tests for persistence + CSS key class behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/lib/src/components/elements/Popover/Popover.tsx | Adds elementStates-backed persistence for passive keyed popovers and widget-mode guarding. |
| frontend/lib/src/components/elements/Popover/Popover.test.tsx | Adds unit tests for popover persistence + widget-mode behavior. |
| frontend/lib/src/components/elements/Expander/Expander.tsx | Adds elementStates-backed persistence for passive keyed expanders and widget-mode guarding. |
| frontend/lib/src/components/elements/Expander/Expander.test.tsx | Adds unit tests for expander persistence + widget-mode behavior. |
| frontend/lib/src/components/core/Block/Block.tsx | Moves st-key-* to StyledLayoutWrapper for expander/popover and removes it from inner container wrapper. |
| frontend/lib/src/components/core/Block/Block.test.tsx | Tests key class placement on wrapper vs inner block for expander/popover. |
| e2e_playwright/st_popover.py | Adds a keyed “Persist popover” example to the E2E app. |
| e2e_playwright/st_popover_test.py | Adds E2E checks for popover CSS key class + persistence behavior. |
| e2e_playwright/st_expander.py | Adds a keyed “Persist expander” plus a toggle to force delta-path shifts. |
| e2e_playwright/st_expander_test.py | Adds E2E checks for expander CSS key class + persistence across remounts. |
| // The hook is always called (Rules of Hooks) but only effective when | ||
| // isPassivelyKeyed — otherwise the empty id produces a no-op entry. | ||
| const [storedOpen, setStoredOpen] = useWidgetManagerElementState<boolean>({ | ||
| widgetMgr, | ||
| id: isPassivelyKeyed ? (blockId ?? "") : "", |
There was a problem hiding this comment.
No longer applicable — useWidgetManagerElementState has been removed from Popover in subsequent refactoring. The component now uses plain useState + useExecuteWhenChanged for state management, with no elementStates read/write.
| // isPassivelyKeyed — otherwise the empty id produces a no-op entry. | ||
| const [storedExpanded, setStoredExpanded] = | ||
| useWidgetManagerElementState<boolean>({ | ||
| widgetMgr, | ||
| id: isPassivelyKeyed ? (blockId ?? "") : "", | ||
| key: "expanded", | ||
| defaultValue: element.expanded ?? false, |
There was a problem hiding this comment.
Same as above — useWidgetManagerElementState has been removed from Expander. The component now uses useDetailsAnimation which manages open/close state internally.
| // No blockId → toggled state (true) should NOT have been stored | ||
| expect(widgetMgr.getElementState("", "open")).not.toBe(true) |
There was a problem hiding this comment.
No longer applicable — the Popover component no longer uses useWidgetManagerElementState, so there are no elementStates writes to test for.
| await user.click(screen.getByText("hi")) | ||
|
|
||
| // No blockId → toggled state (true) should NOT have been stored | ||
| expect(widgetMgr.getElementState("", "expanded")).not.toBe(true) |
There was a problem hiding this comment.
No longer applicable — same as the Popover test comment. The Expander component no longer uses useWidgetManagerElementState.
e2e_playwright/st_popover_test.py
Outdated
| def test_keyed_popover_persists_open_state_across_rerun(app: Page): | ||
| """Test that a keyed popover stays open after a rerun triggered by keyboard shortcut.""" | ||
| # Open the keyed popover | ||
| open_popover(app, "Persist popover") | ||
| expect(app.get_by_text("Persist popover content")).to_be_visible() | ||
|
|
||
| # Trigger a rerun via "r" keyboard shortcut — avoids clicking outside | ||
| # the popover which would close it | ||
| app.keyboard.press("r") |
There was a problem hiding this comment.
Addressed in d2fb45d — the test now uses a checkbox inside the popover that conditionally inserts an element above it, shifting the delta path and forcing a true remount. Verifies persistence in both directions (check/uncheck).
| with st.popover("Persist popover", key="persist_popover"): | ||
| st.write("Persist popover content") |
There was a problem hiding this comment.
Maybe a better e2e test case here might be a toggle in the popover that adds an element on top of the parent container of the popover?
There was a problem hiding this comment.
Good idea — addressed in d2fb45d. Added a checkbox inside the persist popover that sets persist_popover_shift in session state, which conditionally renders st.write("Extra text above popover") above the popover. The E2E test clicks the checkbox (delta-path shift → remount) and verifies the popover stays open.
| className={classNames( | ||
| getClassnamePrefix(Direction.VERTICAL), | ||
| convertKeyToClassName(userKey) | ||
| )} | ||
| className={getClassnamePrefix(Direction.VERTICAL)} |
There was a problem hiding this comment.
Does this change the position of where the key is applied, e.g., for st.container?
There was a problem hiding this comment.
No, st.container has a different path that is unaffected and there is a test for that. The other container elements would be effected except they are not utilizing the class yet. This is why I decided to narrow it just to the ones we are explicitly adding even though that logic isn't strictly necessary, to make sure when it is added on other container elements we make a decision what div to add the class on.
The previous test only triggered a rerun via the "r" shortcut, which doesn't cause a remount. The new test places a checkbox inside the keyed popover that conditionally inserts an element above it, shifting the popover's delta path and forcing a true remount — validating that elementStates persistence works across identity changes. Made-with: Cursor Co-authored-by: lawilby <[email protected]>
Summary
Third PR in the layout container state persistence stack (after #14332 and #14354).
Adds frontend state persistence for keyed
st.expanderandst.popoverelements viaelementStates. When akeyis provided in passive mode (on_change="ignore"), the expanded/open state survives component remounts caused by delta path shifts.Changes
Frontend (Expander.tsx)
expandedstate fromelementStateson mount whenshouldPersistis trueexpandedstate toelementStateson toggle viahandlePersistToggleon_change="rerun") is explicitly protected —elementStatesis never read/written; server state always takes precedenceFrontend (Popover.tsx)
openstate toelementStatesin passive modeFrontend (Block.tsx)
ContainerContentsWrappersuppresses thest-key-*CSS class on the innerStyledFlexContainerBlockwhen it detects the block is inside an expander or popover, avoiding duplication with the container's own outermost elementCSS Key Class
st-key-*class applied to the outermost element of bothst.expanderandst.popoverTest Plan
test_keyed_expander_css_key_classandtest_keyed_expander_persist_expanded_across_remounttest_keyed_popover_css_key_classandtest_keyed_popover_persist_closed_across_remountDepends on: #14354