[feature] st.tabs state persistence and CSS key class#14332
[feature] st.tabs state persistence and CSS key class#14332sfc-gh-lwilby merged 7 commits intodevelopfrom
st.tabs state persistence and CSS key class#14332Conversation
✅ PR preview is ready!
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ 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
added
impact:users
sfc-gh-lwilby
force-pushed
the
feature/tabs-state-persistence
branch
2 times, most recently
from
fffd0a3 to
e1546ea
Compare
📉 Python coverage change detectedThe Python unit test coverage has decreased by 0.0000%
✅ Coverage change is within normal range. Coverage by files
|
|
|
||
| const [activeTabKey, setActiveTabKey] = useState<React.Key>(defaultTabIndex) | ||
| const [activeTabKey, setActiveTabKey] = useState<React.Key>(() => { | ||
| if (shouldPersist) { |
There was a problem hiding this comment.
Let's name this something more specific.
There was a problem hiding this comment.
Renamed stored to storedActiveTabLabel, and then went further: extracted a getPersistedTabIndex() helper that deduplicates all 4 read-and-validate sites. The local variable inside the helper is now the only place that reads from elementStates.
| const widgetId = node.deltaBlock?.tabContainer?.id | ||
| const blockId = node.deltaBlock?.id ?? "" | ||
| const isDynamic = Boolean(widgetId) | ||
| const shouldPersist = Boolean(blockId) && !isDynamic |
There was a problem hiding this comment.
Let's add a comment to explain why these conditions.
There was a problem hiding this comment.
Added an explanatory comment and also renamed shouldPersist to isPassivelyKeyed — describes the condition more precisely: tabs that have a key (stable blockId) but are not dynamic widgets, so they persist locally rather than through the backend.
sfc-gh-lwilby
added
the
ai-review
github-actions
bot
removed
the
ai-review
SummaryThis PR builds on the parent branch's block identity infrastructure to add two capabilities for
Changes span 6 files: Code QualityOverall: The code is well-structured and follows existing patterns. The changes integrate cleanly with the existing Observations:
Test CoverageFrontend unit tests (
Frontend unit tests ( E2E tests (
Minor gap: The PR description mentions "Unkeyed tabs reset to the default tab on remount" as an E2E test, but no such E2E test exists. The unit tests do cover the "no persistence for unkeyed" case ( Backwards CompatibilityNo breaking changes identified.
Security & RiskNo security concerns identified.
Low regression risk: The External test recommendation
AccessibilityNo accessibility concerns. The changes do not affect:
The added CSS class ( Recommendations
VerdictAPPROVED: Well-implemented feature with thorough test coverage, clean integration with existing patterns, no backwards compatibility or security concerns. The duplicated reconciliation pattern is a pragmatic choice given the existing architecture and can be addressed in a follow-up refactor. This is an automated AI review by |
sfc-gh-lwilby
changed the title
sfc-gh-lwilby
changed the title
st.tabs state persistence and CSS key class
sfc-gh-lwilby
changed the title
st.tabs state persistence and CSS key classst.tabs state persistence and CSS key class
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0100%
✅ Coverage change is within normal range. |
sfc-gh-lwilby
changed the base branch from
feature/passive-keyed-block-identity
to
graphite-base/14332
sfc-gh-lwilby
added
the
ai-review
github-actions
bot
removed
the
ai-review
SummaryThis PR implements state persistence and CSS key class application for
Frontend unit tests (Tabs, RenderNodeVisitor) and E2E Playwright tests are added covering the new behavior. Code QualityConsensus (3/3 reviewers agree): The code is clean, well-structured, and follows existing Streamlit frontend patterns. Specific positives noted:
Maintainability observation (2/3 reviewers agree): The reconciliation logic for passively keyed tabs is duplicated across two Test CoverageConsensus (3/3 reviewers agree): Test coverage is excellent/strong.
Minor gap (2/3 reviewers note): No E2E negative test for unkeyed tabs resetting on remount is present, though this scenario is covered at the unit test level. This is non-blocking. Backwards CompatibilityConsensus (3/3 reviewers agree): Fully backwards compatible. No breaking API changes:
Security & RiskConsensus (3/3 reviewers agree): No security concerns. State persistence uses the frontend's in-memory External test recommendation
AccessibilityConsensus (3/3 reviewers agree): No accessibility concerns. The underlying HTML structure and ARIA attributes ( Reviewer Agreement Summary
No disagreements were found across the three reviews. All reviewers unanimously approved with consistent assessments. Recommendations
VerdictAPPROVED: All three reviewers unanimously approve. The implementation is well-scoped, follows existing patterns, has comprehensive test coverage (unit + E2E), maintains full backwards compatibility, and introduces no security or accessibility concerns. The two non-blocking recommendations (deduplicating effects, adding an E2E negative test) are minor maintainability suggestions for follow-up work. Consolidated review by 📋 Review by `gemini-3.1-pro`SummaryThe PR implements state persistence and CSS key class application for Code QualityThe code is clean, well-commented, and follows existing patterns. The logic for determining Test CoverageTest coverage is excellent.
Backwards CompatibilityThere are no breaking changes. The changes enhance the behavior of keyed Security & RiskNo security concerns identified. The state persistence uses the frontend's in-memory External test recommendation
AccessibilityThe PR does not alter the underlying HTML structure or ARIA attributes of the tabs (which are handled by the RecommendationsNone. The implementation is robust and well-tested. VerdictAPPROVED: The PR successfully implements state persistence and CSS key class application for passively keyed This is an automated AI review by 📋 Review by `gpt-5.3-codex-high`SummaryThis PR adds passive state persistence for keyed Code QualityThe implementation follows existing Streamlit frontend patterns and keeps dynamic tabs ( No blocking code-quality issues found. Non-blocking maintainability note:
Test CoverageCoverage is strong for the changed behavior:
Minor gap (non-blocking): there is no explicit e2e assertion for the inverse remount case (unkeyed tabs resetting to default after remount), though this is covered at unit level. Backwards CompatibilityNo breaking API changes are introduced. Existing dynamic tab behavior remains covered and guarded ( Security & RiskNo direct security-sensitive surfaces were modified (no auth/session transport, server routes, asset handlers, cookies/XSRF/CORS, external fetches, or code-execution primitives). Risk is primarily UI state behavior and React reconciliation; regression risk appears low and is mitigated by both unit and e2e coverage. External test recommendation
AccessibilityNo new accessibility regressions identified in the changed code. Interaction remains through BaseWeb tabs/roles, and the added CSS class/state logic does not alter semantic roles, names, or keyboard interaction patterns. Recommendations
VerdictAPPROVED: The changes are well-scoped, backwards compatible, and adequately covered by unit + e2e tests, with no blocking security or regression concerns found. This is an automated AI review by 📋 Review by `opus-4.6-thinking`SummaryThis PR implements state persistence and CSS key class application for
Both frontend unit tests and E2E tests are added covering the new behavior. Code QualityThe code is well-structured and follows existing Streamlit patterns:
Minor observation: The reconciliation logic for passively keyed tabs is duplicated across two effects — the Test CoverageFrontend unit tests (
RenderNodeVisitor tests: Properly covers both keyed blocks (using E2E tests (
The existing tab count assertion was correctly updated from 13 to 14 to reflect the new tabs section in the app script. The E2E test for unkeyed tabs resetting on remount (mentioned in the PR description) is not present in the actual test code. This negative case would strengthen confidence but is already covered by the frontend unit tests. Backwards CompatibilityFully backwards compatible:
Security & RiskNo security concerns identified:
External test recommendation
AccessibilityNo accessibility concerns:
Recommendations
VerdictAPPROVED: Well-implemented feature with comprehensive test coverage, clean backwards compatibility, and no security or accessibility concerns. The state persistence mechanism correctly leverages existing This is an automated AI review by |
sfc-gh-lwilby
force-pushed
the
feature/tabs-state-persistence
branch
from
61d3607 to
b3a3afe
Compare
sfc-gh-lwilby
changed the base branch from
graphite-base/14332
to
feature/passive-keyed-block-identity
✅ Bundle size change is within normal range
|
📈 Significant wheel size change detectedThe wheel file size has increased by 1.50% (threshold: 0.25%)
Please verify that this change is expected. |
Co-authored-by: lawilby <[email protected]>
Co-authored-by: lawilby <[email protected]>
…osure from losing persisted tab Co-authored-by: lawilby <[email protected]>
- Rename generic `stored` variable to `storedActiveTabLabel` across all four elementStates read sites for clarity - Add explanatory comment on `shouldPersist` condition describing why both `blockId` and `!isDynamic` are required Co-authored-by: lawilby <[email protected]>
- Rename `shouldPersist` to `isPassivelyKeyed` for clarity: describes tabs that have a key but are not dynamic widgets - Extract `getPersistedTabIndex()` helper to deduplicate the 4 repeated read-and-validate blocks for stored tab state from elementStates - Add explanatory comment on `isPassivelyKeyed` condition - Rename local `stored` variables to `storedActiveTabLabel` within the helper for specificity Co-authored-by: lawilby <[email protected]>
Replace vi.mock/vi.spyOn-based persistence tests with behavioral tests that use real WidgetStateManager instances. Tests now pre-populate elementStates and check component behavior (selected tab, stored values) rather than asserting on internal getElementState/setElementState calls. Co-authored-by: lawilby <[email protected]>
sfc-gh-lwilby
force-pushed
the
feature/tabs-state-persistence
branch
from
b3a3afe to
8cf2dee
Compare
There was a problem hiding this comment.
Pull request overview
Adds frontend-backed state persistence and CSS key class support for st.tabs, and improves block React key stability so keyed containers keep identity across positional shifts.
Changes:
- Persist/restore active tab selection for passive keyed
st.tabsviaWidgetStateManagerelementStates; applyst-key-*CSS class to the outer tabs container. - Use
node.deltaBlock.idas the Reactkeyfor block nodes when available (fallback to positional index otherwise). - Add/extend frontend unit tests and extend the existing
st_tabsPlaywright E2E coverage for keyed-tab persistence + CSS class.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/lib/src/components/elements/Tabs/Tabs.tsx | Persists/restores active tab label for passive keyed tabs; applies st-key-* class. |
| frontend/lib/src/components/elements/Tabs/Tabs.test.tsx | Adds unit tests for CSS key class + passive persistence behaviors. |
| frontend/lib/src/components/core/Block/RenderNodeVisitor.tsx | Uses deltaBlock.id as React key for block nodes when present. |
| frontend/lib/src/components/core/Block/RenderNodeVisitor.test.tsx | Adds coverage for block keying behavior. |
| e2e_playwright/st_tabs.py | Adds a keyed-tabs scenario that shifts layout above tabs to validate persistence. |
| e2e_playwright/st_tabs_test.py | Updates snapshot count and adds E2E assertions for keyed tabs CSS key class + persistence across layout shifts. |
| const stored = widgetMgr.getElementState(blockId, "activeTabLabel") as | ||
| | string | ||
| | undefined | ||
| if (!stored) return null | ||
| const idx = allTabLabels.indexOf(stored) | ||
| return idx >= 0 ? { index: idx, label: stored } : null |
| def test_keyed_tabs_css_key_class(app: Page): | ||
| """Keyed tabs should have the st-key-* CSS class on the outermost element.""" | ||
| keyed_tabs = get_element_by_key(app, "key_only_tabs") | ||
| expect(keyed_tabs).to_have_class(re.compile(r"st-key-key_only_tabs")) | ||
|
|
||
|
|
||
| def test_keyed_tabs_persist_active_tab_across_remount(app: Page): | ||
| """Toggling a conditional element above keyed tabs shifts the delta path, | ||
| but the active tab should be preserved via elementStates. | ||
| """ | ||
| keyed_tabs = get_element_by_key(app, "persist_tabs") |
…stedTabIndex Fixes @typescript-eslint/no-unnecessary-type-assertion lint error by passing <string> to getElementState instead of casting the result. Made-with: Cursor Co-authored-by: lawilby <[email protected]>
…class (#14356) ## Summary Third PR in the layout container state persistence stack (after #14332 and #14354). Adds frontend state persistence for keyed `st.expander` and `st.popover` elements via `elementStates`. When a `key` is provided in passive mode (`on_change="ignore"`), the expanded/open state survives component remounts caused by delta path shifts. ### Changes **Frontend (Expander.tsx)** - Reads stored `expanded` state from `elementStates` on mount when `shouldPersist` is true - Writes `expanded` state to `elementStates` on toggle via `handlePersistToggle` - Widget mode (`on_change="rerun"`) is explicitly protected — `elementStates` is never read/written; server state always takes precedence **Frontend (Popover.tsx)** - Same pattern: reads/writes `open` state to `elementStates` in passive mode - Widget mode protection identical to Expander **Frontend (Block.tsx)** - `ContainerContentsWrapper` suppresses the `st-key-*` CSS class on the inner `StyledFlexContainerBlock` when it detects the block is inside an expander or popover, avoiding duplication with the container's own outermost element **CSS Key Class** - `st-key-*` class applied to the outermost element of both `st.expander` and `st.popover` ### Test Plan - [x] Unit tests: passive state persistence (read on mount, write on toggle) - [x] Unit tests: widget-mode guards (no elementStates read/write, server value wins over stale stored state) - [x] Unit tests: CSS key class presence/absence - [x] E2E tests: `test_keyed_expander_css_key_class` and `test_keyed_expander_persist_expanded_across_remount` - [x] E2E tests: `test_keyed_popover_css_key_class` and `test_keyed_popover_persist_closed_across_remount` **Depends on:** #14354 --------- Co-authored-by: lawilby <[email protected]>
Summary
Implements state persistence and CSS key class application for
st.tabscontainers, building on the block identity infrastructure from the parent PR.Changes
Tabs.tsx: When ast.tabshas akeyand is passive (non-dynamic), persists the active tab label toelementStatesviawidgetMgr.setElementState/getElementState. On mount, restores the previously active tab if it's still valid. Applies thest-key-*CSS class to the outermostStyledTabContainer.RenderNodeVisitor.tsx: Usesnode.deltaBlock.idas the Reactkeyfor block nodes when available, so keyed containers maintain component identity across positional shifts (e.g. when a conditional element above them causes a delta path change). Falls back to positional index for unkeyed blocks.RenderNodeVisitorblock keying.st_tabs_state_persistence.pyapp and test suite verifying:st-key-*CSS class is correctly appliedTest plan
Tabs.test.tsx,RenderNodeVisitor.test.tsx)st_tabs_state_persistence_test.py)make checkpasses