ref(tables): unify table components onto one shared Table shell - #120745
Draft
JoshuaKGoldberg wants to merge 27 commits into
Draft
ref(tables): unify table components onto one shared Table shell#120745JoshuaKGoldberg wants to merge 27 commits into
Table shell#120745JoshuaKGoldberg wants to merge 27 commits into
Conversation
Extract the duplicated column-resize drag loop (GridEditable and the explore useTableStyles hook) into a single shared useColumnResize hook under components/tables. GridEditable and useTableStyles now delegate the pointer lifecycle and template write to it; each keeps its own track-sizing rules and width store. GridResizer stays the shared DOM handle. Behavior-preserving. The --grid-editable-resizer-height write stays opt-in (GridEditable only) so the explore/logs surface is unchanged. Refs DE-1397
onResizeMouseDown now tears down any still-attached listeners before starting a new drag. Previously, if a drag never received its mouseup (e.g. the pointer was released outside the window), starting another drag orphaned the first drag's mousemove/mouseup handlers, which could apply a stale template or fire an extra commit. Refs DE-1397
Sentry had two sanctioned table components that shared no code, plus a third de-facto shell leaking out of `gridEditable/styles.tsx`, two column-resize implementations, and two competing column-track models. Adds `components/tables/table/`: a dumb shell that owns only table geometry — real `<table>/<thead>/<tbody>/<th>/<td>` on `display: grid` with `grid-template-columns: subgrid`, column tracks, and the resize drag (delegated to `useColumnResize`). Columns are declared as data because a resize needs stable identity and a known column count; cell contents stay as children. Widths are controlled or uncontrolled. Explicit `role` attributes are kept because `display: grid` drops implicit table semantics in Chrome and Safari. GridEditable, SimpleTable, the explore table kit and InfiniteTable are now facades over it: - `useTableStyles` is deleted; its callers go through the shell - the second resize drag loop and second track builder are gone - InfiniteTable no longer overrides `grid-template-columns` per row, so there is one track model - SimpleTable renders real cells instead of `div[role=cell]` Sortable headers become `<th aria-sort><button>` rather than a `div[role=columnheader]` that was itself the button, which is the standard accessible pattern. Ref DE-1392 Ref DE-1405
github-actions
Bot
added
the
Scope: Frontend
Contributor
Story previewsPreview the stories changed in this PR on the Vercel deployment:
Preview deployment: https://sentry-phd06p9g5.sentry.dev |
Contributor
📊 Type Coverage Diff
🔍 2 new type safety issues introducedNon-null assertions (
Type assertions (
This is informational only and does not block the PR. |
Follow-ups from the shell PR. `infiniteTable` and `logsInfiniteTable` now share one spacer-padding virtualization hook. Its padding formula accounts for `scrollMargin`, which fixes the offset InfiniteTable had after its scroll element moved to the table. Replays' `useVirtualizedGrid` is left alone: it measures column widths rather than spacing rows, so it shares only `useVirtualizer` boilerplate. Removes shell surface that turned out to be unreachable or unused: - `lastColumnFlexible` was only read by the default track rule, which its one caller bypasses by supplying `getColumnTrack` - `dataRows` sized a resize handle that is invisible until hover, long after the `--table-resizer-height` var is written - `stickyHeader` and `writeResizerHeightVar` had no callers Collapses the remaining duplicated styles: the two `grid-column` header-cell variants and the full-span row cell move into SimpleTable, and the two teams tables stop declaring byte-identical table and link styles. Ref DE-1392 Ref DE-1398
JoshuaKGoldberg
force-pushed
the
de-1392-unified-table
branch
from
July 28, 2026 15:57
99607c8 to
7733c19
Compare
Base automatically changed from
joshgoldberg/de-1397-consolidate-column-resize-into-usecolumnresize
to
master
July 29, 2026 11:27
The shell wrote grid-template-columns unconditionally, so a table with no `columns` cleared whatever the consumer set via an inline style. Also fixes two bugs in useVirtualRows: paddingBottom mixed scroll space with content space once scrollMargin was non-zero, and keying the re-measure effect on estimateSize's identity looped forever for callers passing an inline arrow. Deletes the second track model (`gridTemplateColumns`) now that string widths on `columns` cover it, turns `headRowHeight` into a boolean since both callers passed the same constant, and drops `withRole` along with its lint exemption.
Browser QA showed both regressed. Wiring InfiniteTable's scrollMargin from useElementOffset fed the virtualizer headerHeight-minus-scrollTop, because getOffsetRect is a viewport-relative delta, so the virtual window drifted further from the scroll position the deeper you scrolled (blank gaps of ~500px and ~1500px under the sticky header). Moving GridEditable's row border onto the shell's `divider` prop dropped dividers from every explore-kit table, since those render the same shared GridRow without the prop. Also drops the now-unused scrollMargin option from useVirtualRows. The estimateKey fix stays; it is an unrelated infinite-render-loop bug.
knip flags it as an unused exported type; nothing imports TableProps through the @sentry/scraps/table barrel.
The snapshot table now uses BuildsTableGrid, but preprodBuildsSnapshotTable.snapshots.tsx replaces preprodBuildsTableCommon wholesale to swap the router Link for a plain anchor, so BuildsTableGrid rendered as undefined and all 10 snapshots failed. Spreading jest.requireActual over the mock is not an option: that module transitively imports installAppButton -> installModal -> markedText -> marked, which is ESM and untransformed under jest.config.snapshots.ts. Moving BuildsTableGrid and FullRowLink into preprodBuildsTableStyles.tsx keeps the real styles in the snapshot and drops the heavy import chain from the test. Also stop the mocked anchor from forcing display: contents, which FullRowLink no longer sets.
JoshuaKGoldberg
marked this pull request as ready for review
July 31, 2026 15:36
JoshuaKGoldberg
marked this pull request as draft
July 31, 2026 15:36
Member
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 86a51a6. Configure here.
Member
|
should we break this up along the lines of maybe: 1) create the new core table, add the tests and stories, and mabye convert the simplest example to use it 2) more prs for each of the other callsites, expanding out to PanelTable if possible over time. |
This was referenced Aug 4, 2026
Member
Author
|
@ryan953 I set up the linked PRs as a stack (first time using the feature! 🥳):
No PanelTable yet, I'll start on that now. |
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.
Today we've got a bunch of different sometimes-overlapping-ish table components or shell-builder-patterns: mostly
GridEditable,SimpleTable, theexploretable kit, andInfiniteTable. This moves almost all of their shared-ish logic intocomponents/tables/table/as a dumb shell that owns only table geometry:<table>/<thead>/<tbody>/<th>/<td>ondisplay: gridwithgrid-template-columns: subgriduseColumnResizefrom ref(tables): consolidate column-resize handling into useColumnResize #120563)onColumnResize) or kept internallyuseTableStylesis deleted, the second resize drag loop and second track builder are gone, andInfiniteTableno longer writesgrid-template-columnsonto every row. So there is now one resize implementation. Yay!Reviewer notes
Please don't be alarmed by the seemingly horrible +2.5k/-1.9k diff 🙃. A lot of the new lines are added tests and stories, or vaguely indentation changes. And, alas, the more explicit new props-based API is more verbose than the old
styledoverrides. The main product-area-y things that previously existed are now much smaller:GridEditable: +80/-368 -> -288 🪓InfiniteTable: +79/-113 -> -34 🪓LogsInfiniteTable: +35/-85 -> -50 🪓Three intentional behaviour changes worth a look:
roleattributes are kept alongside the real elements.display: griddrops implicit table/row/cell semantics in Chrome and Safari, so real<td>alone would have been an a11y regression. Keeping the roles also means existinggetByRolequeries and[role='cell']CSS keep working.<th aria-sort><button>instead of adiv[role=columnheader]that was itself the button. That's the standard accessible pattern, but it changes the DOM contract: the button is stretched to fill the cell so the click target is unchanged.LogsInfiniteTablealready uses). This is because an absolutely positioned element is not a grid item, sosubgridon it computes tonone, which would reintroduce the per-row track model this PR removes. See the newuseVirtualRowsfor what both now use.Not in this PR
SimpleTableis still super entrenched and this is already pretty huge. I think that'll have to be a followup.Row/Cellabstraction, and its ~3 dozen consumers hand-roll<div>s. Retooling that to be actual table-y things would bloat this already-big diff a bunch.resultGrid(app + gsAdmin) and the three key/value tablesCloses DE-1405.