fix(frontend): make a section's wide table admit what it is hiding - #2465
Merged
dzarlax merged 6 commits intoAug 12, 2026
Merged
Conversation
added 2 commits
August 12, 2026 12:50
Three faults in the same table, all of them about a box narrower than what it holds. The grand-total cell spans every group, so its left edge sits wherever the first group is — off-screen on a table wide enough to scroll, which is the ordinary case. Left-aligned inside that cell, the one row summarising the whole table scrolled away and left its sticky label next to nothing. It now sticks past the label column. The footer scrolled out of reach vertically for the same reason a long table needs scrolling at all. It is pinned to the foot of the box, with a top edge so the block reads as a summary sitting over more rows rather than as the end of the table. Nothing said the table continued past either edge. The platform's overlay scrollbar stays hidden until a scroll begins, so at first sight there was no affordance, and a column cut through the middle reads as a rendering fault. Arrows now appear on each side that still holds something and page by just under a frame. Two earlier attempts are not here because they do not work: an edge fade suggests more by hiding what is under it, and over a pale table whose edge cells are often empty there is nothing to hide; forcing the scrollbar visible does not survive a platform that draws it as an overlay. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
The metric selector is chart-only and the plain heading needs a single metric, so a grouped multi-metric table fell through both and rendered an empty header — the largest block on the screen was the only unlabelled one, and a reader inferred what it was from the column headings. It is named by its grouping, since the metrics are already on the columns, and carries the number of groups: such a table is routinely wider than the frame, and three visible groups beside a grand total covering eleven is a reader looking at numbers that do not add up with nothing on screen to explain it. The card also held both presentations to one fixed height, which only the chart needs — it has no height of its own. A short table sat above a slab of empty card. The table now sizes to its rows, keeps the same figure as a ceiling, and offers to drop the ceiling entirely when the rows overrun it. That offer appears only when it would do something: whether the rows overflow depends on the height of a row, which depends on whether the header is one level or two, so it is measured by the table and reported up rather than guessed from a row count. It is deliberately not persisted — a card remembered at full height would greet the next visit as a wall. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe timeseries table now detects horizontal and vertical overflow, provides directional paging, and keeps totals visible. The view tracks overflow, supports table expansion, displays grouped-table metadata, and applies separate chart and table sizing. ChangesTimeseries overflow and layout
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MetricTimeseriesView
participant TimeseriesBody
participant MetricTimeseriesTable
participant ResizeObserver
MetricTimeseriesView->>TimeseriesBody: Render table with overflow callback
TimeseriesBody->>MetricTimeseriesTable: Pass onVerticalOverflow
MetricTimeseriesTable->>ResizeObserver: Observe table size
ResizeObserver->>MetricTimeseriesTable: Report overflow state
MetricTimeseriesTable->>MetricTimeseriesView: Report vertical overflow
MetricTimeseriesView->>MetricTimeseriesTable: Apply expanded table state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/frontend/src/components/widgets/metric-views/metric-timeseries-table.tsx`:
- Around line 260-279: Update the SIDES.map rendering in the metric timeseries
table so each paging Button is rendered only when more[side] is true; remove the
hidden-button opacity and pointer-events handling while preserving the existing
button properties and page(side) behavior.
- Around line 217-230: Update the ResizeObserver setup in the useEffect callback
to observe the scroll container box as well as the inner table, so measure()
runs when either the table or viewport dimensions change. Preserve the existing
conditional handling when ResizeObserver or the table is unavailable, and ensure
the observer is cleaned up through the existing effect lifecycle.
In `@src/frontend/src/components/widgets/metric-views/metric-timeseries-view.tsx`:
- Around line 358-362: Update the presentation-change handling near overflowOf
so it also resets the table’s expanded state alongside setOverflows(false).
Ensure returning to table mode starts collapsed and respects max-h-96, while
preserving existing overflow detection behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 022e907a-6621-4475-8727-c0a2d4c5a408
📒 Files selected for processing (5)
src/frontend/src/components/widgets/metric-views/metric-timeseries-chrome.tsxsrc/frontend/src/components/widgets/metric-views/metric-timeseries-presentations.test.tsxsrc/frontend/src/components/widgets/metric-views/metric-timeseries-table.tsxsrc/frontend/src/components/widgets/metric-views/metric-timeseries-view.test.tsxsrc/frontend/src/components/widgets/metric-views/metric-timeseries-view.tsx
aleksdotbar
previously requested changes
Aug 12, 2026
Four points from review. A paging arrow towards a side with nothing on it is no longer rendered at all. Hiding it with opacity left a real button in the tab order and the accessibility tree, so a keyboard reader sitting at the first column could reach "Show earlier columns" and press it to no effect. The fade goes with it; an invisible control that answers is the worse trade. The resize observer watches the scrolling box as well as the table. Both sides of the comparison move: the table when columns or rows arrive, the box when the window or the card does. A box narrowing around an unchanged table left the arrows and the full-height offer stale. Expansion is cleared with the overflow flag when the presentation changes. A table let out to full height, swapped for the chart and swapped back came up uncapped, and offered to collapse a table that already fitted. The note on `onVerticalOverflow` loses its second half, which explained why the fact travels up instead of a ref — a road not taken, and not visible from the code it sat above. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
Contributor
Author
|
All four applied in e9927f0. Arrows towards an empty side are no longer rendered. The opacity The observer watches the box as well as the table. Agreed on the Expansion is cleared with the overflow flag on a presentation The note on Verified in a browser afterwards: with no horizontal overflow and 107px Two of the four still have no unit test, for the reason already in the Full suite green (1092), typecheck and lint clean. |
Comments that restated the code or recorded how it got here, removed. The history is in the preceding commits and in git blame, and it ages badly there: a note about a render loop describes a bug that no longer exists. What is left names a constraint the code cannot hold and someone would otherwise undo — `scrollLeft` is negative in RTL, `model` must stay out of the effect deps, the reset happens during render because the lint rule forbids an effect, the arrows are rendered rather than hidden so they stay out of the tab order. Two magic numbers became names instead of prose: EDGE_SLACK_PX for the sub-pixel tolerance and NEARLY_A_FRAME for the paging step. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
Contributor
Author
|
@aleksdotbar your note generalised, in 062021c. Measured before touching anything: 105 of 352 added non-blank lines Three kinds went out:
What stayed names a constraint the code cannot hold and that someone Two numbers became names rather than prose — No behaviour change. Full suite green (1092), typecheck and lint clean. |
The new-code coverage gate failed at 71.7%: the scroll effect, the paging step and the expand toggle were all untested, because each one turns on a measurement jsdom cannot make. Supplying the geometry is enough. Defining the container's scroll and client dimensions, then dispatching a scroll, lets the real decisions run — which side still holds something, how far a page moves, which sign it takes — with only the absent layout engine stood in for. The same applies to the card: the mocked table now reports overflow, so the control it gates can be exercised. The observer stub is scoped to the two tests that need it rather than the file: charts in here measure themselves, and a no-op observer left them with nothing to draw. What stays uncovered is three `if (!box) return` guards, unreachable while the ref is attached. Signed-off-by: Alexey Panfilov <Alexey.Panfilov@constructor.tech>
dzarlax
enabled auto-merge
August 12, 2026 15:03
hello1101n
approved these changes
Aug 12, 2026
hello1101n
approved these changes
Aug 12, 2026
Merged
via the queue into
constructorfabric:main
with commit Aug 12, 2026
f076e23
55 of 56 checks passed
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
3 participants
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.
A pass over the two table blocks at the top of a person's section
screen. Everything here is about the same mismatch: the table holds far
more than the box shows, and nothing on screen admitted it.
What was wrong
What it does now
Two things deliberately not done
A metric picker on the table. The chart below has one, but that is a constraint rather than a feature: a stacked chart cannot draw two metrics at once. Several metrics side by side is what the table is for, so narrowing it to one would have made it a worse chart without touching the actual complaint.
An edge fade, and a forced-visible scrollbar. Both were built and removed. A fade suggests more by veiling what lies under it, and over a pale table whose edge cells are frequently empty there is nothing to veil. Forcing the scrollbar visible does not survive a platform that draws scrollbars as overlays — the CSS applies and the bar still takes no space.
Verification
Behaviour was checked in a browser by measurement rather than by eye: the totals' position at both ends of the horizontal scroll, the footer pinned while the box scrolls, the card shrinking to content and capping, and the arrows appearing per side against the measured overflow on each axis.
Tests cover the header and its count, the card's height per presentation, the sticky totals and the pinned footer. The arrows and the full-height control are not covered: they depend on measured overflow, and jsdom reports every dimension as zero, so both stay hidden there. Faking layout to assert them would test the fake. Full unit suite green (1091), typecheck and lint clean.
Summary by CodeRabbit
New Features
Bug Fixes