[fix] st.metric with chart_data wrapping in horizontal containers#14110
[fix] st.metric with chart_data wrapping in horizontal containers#14110sfc-gh-lwilby merged 3 commits intodevelopfrom
Conversation
Metrics with sparkline charts in `st.container(horizontal=True)` could non-deterministically wrap to multiple rows due to a sizing race between Vega-Lite async rendering and flex layout recalculation. Conditionally apply `LARGE_ELEMENT` config (14rem min-width) when the metric has `chart_data`, matching the behavior of other chart elements. Metrics without charts retain `DEFAULT` config for zero behavior change. Co-authored-by: Cursor <[email protected]> Co-authored-by: lawilby <[email protected]>
✅ 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-lwilby
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes a frontend layout instability where st.metric with chart_data (sparkline) could wrap unpredictably inside st.container(horizontal=True) by ensuring metrics with charts use a larger minimum flex width.
Changes:
- Detects when a metric has non-empty
chartData. - Uses
ElementContainerConfig.LARGE_ELEMENTfor metrics with charts (keepsDEFAULTotherwise). - Refactors the metric case to avoid repeated proto casts.
| case "metric": { | ||
| const metricProto = node.element.metric as MetricProto | ||
| const hasChart = | ||
| metricProto.chartData && metricProto.chartData.length > 0 | ||
| return ( | ||
| <ElementContainer | ||
| node={node} | ||
| config={ElementContainerConfig.DEFAULT} | ||
| config={ | ||
| hasChart | ||
| ? ElementContainerConfig.LARGE_ELEMENT | ||
| : ElementContainerConfig.DEFAULT | ||
| } |
There was a problem hiding this comment.
This change adds new layout behavior (switching to LARGE_ELEMENT for metrics with chartData) but there doesn’t appear to be unit/integration coverage asserting the container styles for metric-with-chart in a horizontal layout. Please add a test (similar to the Markdown auto-width integration tests) that renders a metric node with non-empty chartData under a horizontal FlexContext and asserts the resulting min-width / flex-basis behavior stays stable.
There was a problem hiding this comment.
Added three tests in ElementNodeRenderer.test.tsx verifying the conditional config: metric with chartData → LARGE_ELEMENT, empty chartData → DEFAULT, no chartData → DEFAULT. Uses vi.mock to wrap ElementContainer and assert the config prop directly.
Verify that metrics with chartData use LARGE_ELEMENT config and metrics without chartData use DEFAULT config. Co-authored-by: Cursor <[email protected]> Co-authored-by: lawilby <[email protected]>
Replace `any` types with proper `ElementContainerProps` and `Element` types, and replace non-null assertions with throw guards. Co-authored-by: Cursor <[email protected]> Co-authored-by: lawilby <[email protected]>
lukasmasuch
added
the
ai-review
github-actions
bot
removed
the
ai-review
Consolidated Code ReviewPR: [fix] st.metric with chart_data wrapping in horizontal containers (#14110)SummaryThis PR fixes a non-deterministic layout regression (#13785) where The fix is minimal and surgical: in Files changed: Code QualityBoth reviewers agree the implementation is clean, minimal, and follows established patterns. The conditional config selection mirrors how other chart-containing elements are sized with Minor observation (non-blocking): The expression Test CoverageBoth reviewers agree the three unit tests provide solid coverage of the decision logic:
Both reviewers note no new E2E test was added. They agree this is reasonable given the non-deterministic nature of the race condition bug (which would make E2E tests inherently flaky). One reviewer (gpt-5.3-codex-high) recommends adding a targeted E2E regression test as a non-blocking follow-up; the other (opus-4.6-thinking) accepts the unit-test-only approach as pragmatic. Minor observation (non-blocking): The tests use Backwards CompatibilityBoth reviewers agree there are no backwards-compatibility concerns:
The Security & RiskBoth reviewers agree there are no security concerns. The change is purely presentational (CSS layout configuration) with no new data handling, network calls, or user input processing. Regression risk is minimal, scoped to a single AccessibilityBoth reviewers agree there is no accessibility impact. The change modifies only the CSS container configuration ( Reviewer Agreement Summary
No disagreements between reviewers. Both approved with only minor non-blocking observations. Recommendations
VerdictAPPROVED: Both reviewers approve unanimously. The fix is well-scoped, low-risk, consistent with existing codebase patterns, and adequately tested. No critical or blocking issues were raised. Ready to merge. Missing ReviewsAll expected models submitted reviews:
Consolidated review by 📋 Review by `gpt-5.3-codex-high`SummaryThis PR fixes a layout regression where Code QualityNo blocking code-quality issues found.
Test CoverageCoverage is good at the unit level, with a small residual e2e gap.
Backwards CompatibilityNo API or protocol changes were introduced, and no breaking changes are apparent.
Security & RiskNo security concerns identified.
AccessibilityNo accessibility regressions identified in this diff.
Recommendations
VerdictAPPROVED: The fix is well-scoped, low risk, and unit-tested; it is ready to merge, with a non-blocking recommendation to add a focused e2e regression test. This is an automated AI review by 📋 Review by `opus-4.6-thinking`SummaryThis PR fixes a non-deterministic wrapping bug (#13785) where The fix is minimal and surgical: in Only two files are changed: Code QualityThe implementation is clean and follows established patterns in the codebase. The conditional config selection mirrors how other chart-containing elements (e.g., Minor observations:
Test CoverageThe three new unit tests provide solid coverage of the decision logic:
Each test also validates the expected No new E2E test was added. The PR description notes that existing E2E tests ( Backwards CompatibilityThe change is fully backwards compatible:
The Security & RiskNo security concerns. The change is purely presentational (CSS layout configuration) with no new data handling, network calls, or user input processing. Regression risk is minimal — the change is scoped to a single AccessibilityNo accessibility impact. The change modifies only the CSS container configuration (min-width), not the rendered DOM structure, ARIA attributes, or interactive elements. Recommendations
VerdictAPPROVED: Clean, minimal, and well-tested fix for a non-deterministic layout race condition. The approach is consistent with existing codebase patterns and carries no backwards-compatibility or security risk. This is an automated AI review by |
|
On the e2e test: I did consider this as well. The layouts area has many possible configurations that could be tested with snapshots. When the feature shipped, I added tests that cover some common patterns, but it's not everything. I think a good next step would be to align the e2e tests with what we are guiding agents to do in the skills. I will follow up on this separately. |
Summary
Fixes #13785.
Test App.
st.metricelements withchart_data(sparkline charts) placed insidest.container(horizontal=True)could non-deterministically wrap to multiple rows. This was caused by a sizing race condition: the Vega-Lite chart renders asynchronously viauseEffect+ResizeObserver, which changes the element's intrinsic size after the initial flex layout, causing a positive feedback loop of reflows and widening.The fix conditionally applies
LARGE_ELEMENTconfig (14remmin-width, matching other chart elements) when the metric haschart_data. Metrics without charts retainDEFAULTconfig — zero behavior change for the common case.Behavior matrix:
DEFAULT(no min-width → race condition)LARGE_ELEMENT(14rem min-width → stable)DEFAULTDEFAULT(unchanged)st.columnsTest plan
ElementNodeRendererunit tests pass (4/4)Metriccomponent unit tests pass (58/58)st_metrictests (metrics with charts in columns — unchanged behavior)st_layouts_container_various_elements(metrics without charts in horizontal — unchanged behavior)