Repository navigation
Conversation
When the layout flips from a window resize event, the container size from ResizeObserver and the tooltip height measured in the previous pane are still stale for one render. Sizing from them could report a zero main pane, which made the chart host unmount and recreate the chart instead of reflowing it. Re-measure the container synchronously when the controlled layout changes, and until the tooltip is measured in the new layout size the main pane as if the tooltip were empty without notifying consumers.
…kip the measurement gate in vertical layout The vertical layout does not depend on the tooltip height, so a width change there no longer renders the content twice. The resize callback reads the layout and width from a ref, so the ResizeObserver is not recreated on every container resize. Both dimension writers share one updater that bails out on equal sizes. The component test no longer imports the viewport layout hook from the gravity-charts plugin.
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
|
🎭 Tests Report is ready. |
Contributor
|
Preview is ready. |
Addresses: 4494056490 Addresses: 4494064643 Addresses: 4494069336
korvin89
marked this pull request as ready for review
November 6, 2026 15:30
kuzmadom
reviewed
Nov 6, 2026
| onMainPaneSizeChangeRef.current = onMainPaneSizeChange; | ||
|
|
||
| const tooltipPaneRef = React.useRef({layout, containerWidth}); | ||
| tooltipPaneRef.current = {layout, containerWidth}; |
Contributor
There was a problem hiding this comment.
AI generated
Updating this ref during render exposes an uncommitted layout to ResizeObserver when a layout transition suspends. A subsequent tooltip resize leaves tooltipMeasured false and the main pane stuck at 476px instead of 96px. Reproduced on this PR; main passes. Moving the assignment into a layout effect fixes it.
| const [measuredLayout, setMeasuredLayout] = React.useState(props.layout); | ||
| if (props.layout !== measuredLayout) { | ||
| setMeasuredLayout(props.layout); | ||
| const rect = containerRef.current?.getBoundingClientRect(); |
Contributor
There was a problem hiding this comment.
AI generated
This measurement still reads the previous DOM size when a parent changes its dimensions and layout in the same React update. With the wrapping tooltip, a 600×300 vertical → 320×500 horizontal update reports 0 before 76. This also happens on main; the new test does not cover this remaining case.
…e measured after commit Reading the container during render still saw the previous DOM when a parent changed the size and the layout in one update, and tagging the tooltip measurement from a ref written during render could expose a layout that never committed. The layout is now applied in a layout effect together with a fresh container measurement, and the ref is updated after commit. Addresses: 4494644344 Addresses: 4494644362
… layout change ResizeObserver reports changes against the size it delivered last. When the synchronous measurement in the layout effect sees a size that reverts before the next frame, the observer never reports it and the component stays with the stale size. Measure once more in the next frame. CI-fix: Visual Tests
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the gravity-charts split tooltip recreating the chart instead of reflowing it when the viewport orientation changes, and the resulting flake of the visual test «preserves tooltip content height and reflows the existing chart across orientation changes» (from #951).
Why
The layout prop flips from the
window.resizeevent (or a parent update), while two inputs ofSplitTooltipContentare still stale for that render:ResizeObserverdelivers later in the same frame;Sizing from stale inputs could give a zero main pane (e.g.
300 − 24 − 400or500 − 24 − 600).onMainPaneSizeChange(0)requested areflow(), and@gravity-ui/chartsChart, on its forced measurement, saw a zero host and unmountedChartInner, so.gcharts-chartwas created anew.How
SplitTooltipapplies a newlayoutprop in a layout effect together with a fresh container measurement, so the content never renders a new layout with the dimensions of the previous one. This covers both the window-resize path and a parent changing the size and the layout in one update. The container is measured once more on the next frame:ResizeObserverreports changes against the size it delivered last, so a size that reverts within the same frame would otherwise never be reported (seen as a CI-only failure of «preserves the mounted main pane…»).SplitTooltipContentkeeps the tooltip measurement together with the layout and container width it was taken in (tagged after commit). Until the tooltip is re-measured in the current horizontal layout, the main pane is sized as if the tooltip were empty (containerHeight − 24, never zero) andonMainPaneSizeChangeis held back. This also covers a reflow that was already pending before the layout change.enabled={false}and the vertical layout do not depend on the tooltip height and are not gated.Behavior note: on mount in horizontal layout
onMainPaneSizeChangeis now called once with the measured size instead of twice (containerHeight − 24first), which is onereflow()less forwithSplitPane.Small choices made alone: when the container
ResizeObserverfires before the resize event (seen in the test environment), one notification with the previous layout and the new container size can still happen; it matches the DOM and is non-zero, so it is accepted and documented in the test. The test defines a local viewport-layout hook instead of importing it from the gravity-charts plugin.Testing
npm run typecheck,npx eslint src --quiet,npm test(138 unit) — green, run before every push.SplitTooltip behavior, both with a wrapping tooltip (12 × 100×100 blocks, 600px tall in a 240px pane, 400px in a 320px pane):main([0, 'horizontal']).Both assert no zero size and that the new layout is only ever reported with its final size; green 5/5 locally.
src/plugins/gravity-charts/__tests__/SplitTooltip.visual.test.tsx10/10 locally (was failing ~2 of 3 runs before).npm run test:visualgreen locally except the known Indicator «default state» flake, which passes on rerun.SplitTooltipvisual tests in the Playwright Linux container (scripts/playwright-docker.sh) 6/6 after the next-frame re-measure (1 of 2 before).npm run test:visualon Linux/Docker (npm run test:docker) to confirm the new tests need no screenshot baseline (they have none) and the gravity-charts test is stable there.Plan
One unit:
remeasure-on-layout-change(this PR). Change log: v1 container re-measure only → v2 added the tooltip-height gate and a regression test → v3 gate replaced by a provisional non-zero size,enabled=falsebypass, deterministic test content → v4 (review): render-phase container read replaced by applying the layout in a layout effect with a fresh measurement; tooltip measurement tag updated after commit; parent-update test added.