Conversation
The thumb's aria-controls was hardcoded to this mount's useId value, but the sync effect only stamps that id onto the scroll container when the container has none. When the host app supplies its own id, or when the component remounts onto a container a previous mount already labelled, aria-controls names an element that does not exist and assistive tech cannot resolve the scrollbar's target. Track the id actually present on the element instead.
The unit test drives the remount directly against PinnedScrollbar. Exercise the same path through DataTable's public API, where unpinning every column drops hasPinnedColumns and re-pinning mounts a fresh scrollbar over the wrapper the previous mount already labelled.
4857856 to
fcd3a19
Compare
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: ready for the operator — approve.
No blocking issues found in the changed lines. I verified the base implementation's divergent paths: at src/components/PinnedScrollbar.tsx:62-64, an existing el.id bypasses the stamp, while the base thumb at src/components/PinnedScrollbar.tsx:194 always uses this mount's scrollContainerId. The changed setControlsId(el.id) and aria-controls={controlsId} therefore make the attribute follow the actual element id for both a host-provided id and a remount over an already-labelled wrapper.
The public-API integration regression test exercises the claimed pinned → unpinned → pinned remount path, and the three changed-path tests have concrete base failures while the first-mount control remains green. The facts sheet includes the required evidence, policy quotes, prior-art results, and a boundary ledger covering the new state/effect path. I also re-ran upstream prior-art searches for aria-controls PinnedScrollbar; they returned no matching open or closed PRs or issues. Commit messages contain no prohibited AI attribution.
The diff remains focused: 9 production lines and regression tests only. The fork reports no CI checks, so I did not treat CI as test confirmation; the body supplies executed A/B output and the review is based on the diff and that evidence.
What's good: preserving an existing container id avoids both the dangling ARIA reference and an unnecessary overwrite of a host-owned DOM identifier.
Notes for the operator:
- Upstream CONTRIBUTING asks contributors to open an issue first for non-trivial work; its PR-body formatting rules, if any are later encountered, should be handled when preparing the upstream submission rather than by changing this fork head.
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: ready for the operator — approve.
No blocking issues found in the changed lines. I verified the base implementation's divergent paths: at src/components/PinnedScrollbar.tsx:62-64, an existing el.id bypasses the stamp, while the base thumb at src/components/PinnedScrollbar.tsx:194 always uses this mount's scrollContainerId. The changed lines at src/components/PinnedScrollbar.tsx:16, :68-72, and :202 make the attribute follow the actual element id for both a host-provided id and a remount over an already-labelled wrapper.
The public-API integration regression test exercises the claimed pinned → unpinned → pinned remount path, and the three changed-path tests have concrete base failures while the first-mount control remains green. The facts sheet includes the required evidence, policy quotes, prior-art results, and a boundary ledger covering the new state/effect path. I also re-ran upstream prior-art searches for aria-controls PinnedScrollbar; they returned no matching open or closed PRs or issues. Commit messages contain no prohibited AI attribution.
The diff remains focused: 9 production lines and regression tests only. The fork reports no CI checks, so I did not treat CI as test confirmation; the body supplies executed A/B output and the review is based on the diff and that evidence.
What's good: preserving an existing container id avoids both the dangling ARIA reference and an unnecessary overwrite of a host-owned DOM identifier.
Notes for the operator:
- Upstream CONTRIBUTING asks contributors to open an issue first for non-trivial work; its PR-body formatting rules, if any are later encountered, should be handled when preparing the upstream submission rather than by changing this fork head.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: the fix and its DataTable-level regression test are correct and independently reproduced; one test in the direct-unit half of the suite doesn't discriminate a real host-id divergence from the fix's happy path — see Boundaries row 2 below.
What I did
Cloned the fork, checked out live head fcd3a19f (confirmed both commits — 8d16c96 fix, fcd3a19f DataTable-level test — are present; git diff 30afe83..fcd3a19f --stat = 2 files, +108/-1, matching the PR body and GitHub's API). Ran npx vitest run src/__tests__/pinning.test.tsx myself: fixed arm 20/20 passing; with src/components/PinnedScrollbar.tsx reverted to 30afe83 (test file untouched), 3 failed/17 passed, same three assertion messages the PR body quotes (:rl:→:rk:, :rs:→host-app-scroll-container, :ru:→:rt:). tsc --noEmit rc=0. Confirmed PinnedScrollbar is not exported from src/index.ts and DataTable.tsx:377's wrapperProps only spreads dir, so the remount path (unpin all pinned columns → re-pin) is the one reachable through the public <DataTable/> API, matching the PR body's own claim. Checked GitHub search for open/closed issues mentioning "aria-controls" or "pinned scrollbar" on upstream — none exist; this is a self-found defect, not a linked issue.
Boundaries ledger (rebuilt independently from the diff)
| # | Predicate / guard changed | Input | Fixed-code behavior | Test that pins it |
|---|---|---|---|---|
| 1 | if (!el.id) false-branch didn't exist before; thumb read scrollContainerId unconditionally |
el.id empty (PinnedScrollbar.tsx:65-66, first mount, no host id) |
stamp runs, setControlsId(el.id) (:72) sets to the just-stamped value; identical outcome to base |
aria-controls resolves to the scroll container it labelled (control) |
| 2 | if (!el.id) true (container already has an id) |
el.current!.id = 'host-app-scroll-container' set before first mount (pinning.test.tsx:251) |
stamp skipped, setControlsId(el.id) copies the pre-existing id |
aria-controls points at a host-supplied container id instead of a fresh one — see note below, this is not the same trigger as the shipped bug |
| 3 | Same guard, id present from a prior mount of this same component | unmount + remount over a container the first mount stamped | controlsId follows the surviving id, not the new mount's fresh useId |
aria-controls survives a remount onto the same container |
| 4 | Row 3 through the public API | <DataTable> pinned→unpinned→re-pinned, responsive |
wrapper keeps its id; new thumb's aria-controls resolves to it |
pinned scrollbar aria-controls still resolves after a column is unpinned and re-pinned — I reran this directly against <DataTable/>, reproduces |
| 5 | useState(scrollContainerId) initial value pre-effect |
first render, effect not flushed | equals base's value by construction (no dedicated assertion, but nothing renders aria-controls before the effect flushes in these tests, and React's test renderer flushes effects synchronously under act) |
none dedicated; not exploitable — accept as-is |
| 6 | if (!el) return |
scrollRef.current === null |
effect returns before the stamp; component itself returns null (visible stays false) since nothing calls update() |
renders null before ResizeObserver fires (indirect but sufficient — no aria-controls is ever emitted on this path) |
| 7 | if (!visible) return null, boundary scrollWidth === clientWidth |
makeScrollRef(400,400) |
no thumb rendered | same test as row 6 |
| 8 | Effect re-run on same mount, deps [scrollRef, scrollContainerId, update] |
update identity is stable (useCallback([scrollRef])), so in practice this only re-runs on ref change, not spuriously |
setControlsId(el.id) recomputes to the same value; React bails a same-value setState, no extra render/loop |
no dedicated test, but nothing here is new behavior — base already re-ran this effect and stamped el.id on the same deps |
| 9 | Host mutates el.id after mount |
untracked | stale aria-controls |
correctly out of scope; unchanged from base, no test needed |
Row 2 is the one row the PR body doesn't quite earn: it correctly reproduces on base (assertion fails without the fix) but the trigger it uses — the host stamping ref.current!.id before PinnedScrollbar ever mounts — is not the bug described in the PR body or the commit message ("a previous mount already labelled" / "remount onto a container a previous mount already labelled"). A host that pre-sets an id on a container it owns before handing it to <DataTable> is a scenario DataTable doesn't support at all today (the library owns ResponsiveWrapper and its ref internally — nothing in DataTable.tsx:571-580's wrapperProps lets a caller inject a DOM id onto the scroll wrapper). So test 2 exercises a code path (if (!el.id) false) that is real in the component's logic but not reachable from the outside the way the PR claims; it happens to share the same guard as the genuinely-reachable row 3/4 remount case, which is why it still fails identically on base. This doesn't make the fix wrong — the guard is shared and the fix is correct for both — but the PR shouldn't cite this specific test as evidence of an externally-triggerable "host id" bug when nothing in the current public API lets a host set that id. Not a blocker; the remount path (rows 3/4) carries the real-world case and is independently confirmed through <DataTable/> in row 4.
Assertion-vacuity check (per-test, both arms)
All four aria-controls tests fail-then-pass across the base/fix A/B I ran myself (not just PinnedScrollbar-direct — the DataTable-level test row 4 too), so none of the new assertions are vacuous in the variant they're written for. No RESP3-style "assertion holds either way" issue here — this is a single-runtime (jsdom) React component with no protocol/platform branching, so there's no second variant to check for a silently-passing duplicate.
What I could confirm independently
- The bug is real and reachable through the documented
<DataTable/>API (unpin-all → re-pin), not just through a directimport PinnedScrollbar from '.../PinnedScrollbar'that userland can't actually do (it's unexported). - The fix (
setControlsId(el.id)right after the stamp, thumb renderscontrolsIdinstead ofscrollContainerId) is minimal and correct for the reachable path. - Base fails 3/20, fixed passes 20/20, tsc clean — all reproduced from a clean checkout, not taken on the PR body's word.
- No prior GitHub issue or PR exists for this on upstream; CONTRIBUTING.md's "open an issue first for anything non-trivial" applies — that's the submission route, not a review finding.
useStatederived from a ref inside an effect (rather than derivingcontrolsIdat render time) is a shape already present elsewhere in this file (thumbWidth/thumbLeft/visible/scrollPercentare all effect-driven state), so it matches the file's existing idiom rather than introducing a new one — an upstream reviewer familiar with this file is unlikely to push back on the pattern itself.
Style/commit conventions vs. upstream
Commit message and PR title (fix: point pinned scrollbar aria-controls at the container's real id) match upstream's terse fix: <what> convention (fix: keep column filter popup within the viewport jbetancur#1387, fix(resize): enforce configured column minWidth jbetancur#1378). The suggested upstream title in the PR body's Disclosure section (fix(a11y): ...) is a reasonable scoped variant matching the fix(resize):/fix(selection): component-scoped style seen in recent merges.
SECOND READ: READY
VerificationFresh adversarial run at head Focused A/B — identical 23-test file on both source arms: $ npx vitest run src/__tests__/pinning.test.tsx # current head
✓ src/__tests__/pinning.test.tsx (23 tests) 623ms
Test Files 1 passed (1)
Tests 23 passed (23)
$ git checkout 30afe83 -- src/components/PinnedScrollbar.tsx
$ npx vitest run src/__tests__/pinning.test.tsx # base source, same tests
× pinned scrollbar aria-controls still resolves after a column is unpinned and re-pinned 97ms
× aria-controls points at a host-supplied container id instead of a fresh one 7ms
× aria-controls survives a remount onto the same container 8ms
× aria-controls re-points when the scroll container is swapped for another one 7ms
× aria-controls follows an id the host assigns after the first mount stamped one 7ms
✓ aria-controls resolves to the scroll container it labelled (control) 7ms
✓ two concurrent scrollbars each control their own container (control) 7ms
AssertionError: expected ':rl:' to be ':rk:' // Object.is equality
AssertionError: expected ':rs:' to be 'host-app-scroll-container' // Object.is equality
AssertionError: expected ':ru:' to be ':rt:' // Object.is equality
AssertionError: expected ':rv:' to be 'second-scroll-container' // Object.is equality
AssertionError: expected ':r11:' to be 'host-took-over-later' // Object.is equality
Test Files 1 failed (1)
Tests 5 failed | 18 passed (23)Every non-control test fails on base and passes at this head. The two base-passers are named Full package suite at this head: $ npm test
Test Files 23 passed (23)
Tests 725 passed (725)
Duration 21.53sScope and index checks after the test commit: $ git status --porcelain # empty
$ git diff HEAD --stat # empty
$ git show HEAD:src/components/PinnedScrollbar.tsx | grep -n controlsId
18: const [controlsId, setControlsId] = React.useState(scrollContainerId);
72: setControlsId(el.id);
202: aria-controls={controlsId}
$ git diff 30afe83..HEAD --stat
src/__tests__/pinning.test.tsx | 189 +++++++++++++++++++++++++++++++++++++
src/components/PinnedScrollbar.tsx | 10 +-
$ git diff fcd3a19 7a9d65b -- src/components/PinnedScrollbar.tsx # empty — source byte-identical to the reviewed head$ gh pr checks 1 --repo askalf/react-data-table-component
no checks reported on the 'fix/pinned-scrollbar-aria-controls' branchThat is absence, not failure: this fork's Actions still need the one-time manual enable (operator card Two ledger rows the original body listed were reframed rather than tested, and the body now says so plainly: an id mutated while the same mount stays active is not observed (unchanged from base, would need a |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVE — ready for the operator to submit; no blocking issues found.
I reviewed the complete 198-line diff, the current-head OSS-candidate evidence, upstream base implementation, upstream contribution policy, commit messages, and repeated upstream PR/issue searches. The base effect preserves a nonempty container id while base render always used its fresh useId; the changed lines correctly carry the actual el.id into rendered aria-controls:
src/components/PinnedScrollbar.tsx:65-73if (!el.id) { el.id = scrollContainerId; } ... setControlsId(el.id);
src/components/PinnedScrollbar.tsx:199aria-controls={controlsId}
The state initialization preserves the pre-effect fresh-container value, and the synchronization effect covers both the falsy-id stamp and truthy-id preservation paths. The added tests discriminate against the base source for host-provided ids, retained ids across remount, public unpin/re-pin, ref/container replacement, and reverse host takeover; the explicit controls cover ordinary first mount and concurrent instances. No upstream duplicate was returned by my PR and issue searches for aria-controls PinnedScrollbar. The candidate has the required facts/evidence, a focused two-file scope, and no AI attribution in commit messages.
What's good: the patch avoids clobbering host DOM ownership and gives the accessibility regression direct resolution assertions rather than merely checking that an attribute is present. Fork CI is unavailable (no checks reported), but the body supplies executed focused A/B and full-suite results for this exact head.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: no blocking issues found — the fix and its tests hold up against an independent trace.
OSS-candidate second read
This is a fork PR against askalf/react-data-table-component, targeting upstream jbetancur/react-data-table-component. Reviewed at head 7a9d65b453dd3bcfafde92b1b1fc18938b318b80. The source diff is unchanged from the head I previously commented on (fcd3a19); git diff fcd3a19 7a9d65b -- src/components/PinnedScrollbar.tsx is empty. This head adds five more focused tests plus two named controls in src/__tests__/pinning.test.tsx (7a9d65b commit "test: cover pinned scrollbar aria-controls container changes").
Confirming the bug myself
Read directly from base source (30afe83), not from the PR body: PinnedScrollbar.tsx computed scrollContainerId = React.useId() and rendered aria-controls={scrollContainerId} unconditionally, while the effect only assigned el.id = scrollContainerId when !el.id. So any path where the scroll container already carries an id — host-supplied, or left behind by a prior mount because useId() hands out a fresh value every mount — produces a dangling aria-controls pointing at an id nothing on the page has. I traced this in the base file myself; it is real, not a static-analysis artifact. The fix — src/components/PinnedScrollbar.tsx:16 adds controlsId state, src/components/PinnedScrollbar.tsx:70 calls setControlsId(el.id) right after the existing stamp-or-preserve branch, src/components/PinnedScrollbar.tsx:202 renders aria-controls={controlsId} — closes it by rendering whatever id is actually on the element in both branches.
Prior art, re-run independently
gh search issues --repo jbetancur/react-data-table-component "aria-controls"→ no results.gh search issues --repo jbetancur/react-data-table-component "PinnedScrollbar"→ only closed, unrelated jbetancur#1388 (RTL filter popup alignment).gh pr list --repo jbetancur/react-data-table-component --state open→ only jbetancur#1393 (docs color-mode sync), unrelated.gh api repos/jbetancur/react-data-table-component/commits?path=src/components/PinnedScrollbar.tsx→ most recent touches are jbetancur#1384 (readability refactor), jbetancur#1344/jbetancur#1338 (RTL/pin shadow fixes); none toucharia-controls.
No overlap found. This matches the PR body's own prior-art claim.
Idiom / maintainer's-eye check
- Tests are added to the existing
src/__tests__/pinning.test.tsx, inside the pre-existingdescribe('PinnedScrollbar', ...)block — same file/co-location convention the repo already uses, not a new test file. - Commit style (
fix: ...,test: ...) matches the repo's own recent history (30afe83,a3473b0), modulo one pre-existing outlier commit (bug: align column filter..., upstream's own inconsistency, not this PR's). CONTRIBUTING.mdline 11 lists "Bug fixes with a clear reproduction" and line 12 "TypeScript or accessibility improvements" as fast-track categories — this PR is squarely both.CONTRIBUTING.mdline 20 also lists "Open an issue first for anything non-trivial" — per the task's own rule, that is the operator's submission-route concern, not a finding here.- No existing helper is being reimplemented;
controlsIdstate is the minimal idiomatic React fix for this exact defect shape (a value that must reflect DOM state set outside React's control).
Boundaries ledger (rebuilt from the diff, not the PR body)
| Predicate/expr changed | Boundary input | Fixed behavior | Test that pins it |
|---|---|---|---|
if (!el.id) (PinnedScrollbar.tsx:65-67) |
el.id empty (fresh mount) |
stamps scrollContainerId; controlsId set to same value |
aria-controls resolves to the scroll container it labelled (control) |
| same guard, other branch | el.id non-empty (host-supplied) |
id preserved; controlsId now correctly follows it (was previously ignored) |
aria-controls points at a host-supplied container id instead of a fresh one |
| same branch, id from a prior mount | remount after unmount, no host involvement | controlsId follows the old stamped id, not the new mount's fresh useId() |
aria-controls survives a remount onto the same container |
effect deps [scrollRef, scrollContainerId, update] (PinnedScrollbar.tsx:79) |
scrollRef identity changes mid-life |
effect re-runs, controlsId re-points at the new container's actual id |
aria-controls re-points when the scroll container is swapped for another one |
| same guard, reverse ordering | id assigned by host after an earlier mount already stamped one, then remount | controlsId follows the id present at mount time, regardless of who wrote it or when |
aria-controls follows an id the host assigns after the first mount stamped one |
public integration path (DataTable.tsx:633-638, unchanged) |
pinned → unpinned → re-pinned columns (hasPinnedColumns toggles) |
PinnedScrollbar unmounts/remounts against the same retained wrapper node; controlsId follows the wrapper's id |
pinned scrollbar aria-controls still resolves after a column is unpinned and re-pinned |
| per-instance state | two concurrent scrollbars, distinct containers | each instance's controlsId stays independent |
two concurrent scrollbars each control their own container (control) |
I did not find a reachable row the tests miss. The one theoretically adjacent case — an id mutated on the container during an already-active mount, with no remount and no ref-identity change — is out of scope by design (would need a MutationObserver, which the diff correctly does not add), and the PR body states this plainly rather than hiding it.
Assertion check (not vacuous)
For each of the five discriminating tests, I confirmed against the base source that the specific assertion fails pre-fix and passes post-fix (the PR's own quoted A/B run corroborates this, and the logic traces the same way from reading the base file directly): each compares thumb.getAttribute('aria-controls') against the actual ref.current!.id/wrapper id, which only matches post-fix because pre-fix the attribute is hard-wired to scrollContainerId regardless of what el.id actually is. The two tests explicitly labelled (control) are correctly not counted as regression proofs — they exercise the one path (fresh empty id) where old and new code produce the same value, and their purpose (stated in-line) is to pin that this common path is unaffected, not to catch the bug. No RESP3-style protocol/platform variants apply to this single-runtime React component, so that assertion-per-variant check doesn't surface additional gaps here.
What I could not do
Local test execution was blocked by an unrelated environment dependency-resolution problem in this sandbox (vitest/@vitest/mocker/@jridgewell/trace-mapping module resolution failures after npm ci — not caused by anything in this diff), so I did not re-execute the suite myself; I relied on tracing the base vs. fixed logic directly against the tests' assertions instead of re-running the PR's quoted A/B. gh pr checks reports no checks on this branch, consistent with fork Actions being disabled per the PR body.
What's good
Small, correctly scoped fix (no public API change), in-code comments explain the non-obvious "why" at the two changed lines, and the test additions are a genuine boundary sweep rather than restating the same case seven times.
SECOND READ: READY
The sync effect returns at `if (!el)` before it can stamp an id or set controlsId, so no thumb is rendered and no control reference is emitted. Green on both source arms - it pins the early-return row of the boundary ledger, which had no coverage of its own.
VerificationSecond adversarial pass, at head What this head adds
Head arm — 24/24$ npx vitest run src/__tests__/pinning.test.tsx
✓ src/__tests__/pinning.test.tsx (24 tests) 724ms
Test Files 1 passed (1)
Tests 24 passed (24)
Start at 21:38:39
Duration 4.34sBase arm — identical 24-test file, only the source reverted$ git checkout 30afe83 -- src/components/PinnedScrollbar.tsx
$ npx vitest run src/__tests__/pinning.test.tsx
FAIL pinned scrollbar aria-controls still resolves after a column is unpinned and re-pinned
FAIL aria-controls points at a host-supplied container id instead of a fresh one
FAIL aria-controls survives a remount onto the same container
FAIL aria-controls re-points when the scroll container is swapped for another one
FAIL aria-controls follows an id the host assigns after the first mount stamped one
AssertionError: expected ':rl:' to be ':rk:' // Object.is equality
AssertionError: expected ':rs:' to be 'host-app-scroll-container' // Object.is equality
AssertionError: expected ':ru:' to be ':rt:' // Object.is equality
AssertionError: expected ':rv:' to be 'second-scroll-container' // Object.is equality
AssertionError: expected ':r11:' to be 'host-took-over-later' // Object.is equality
Test Files 1 failed (1)
Tests 5 failed | 19 passed (24)Five discriminating tests fail on base and pass with the fix. The 19 base-passers are the 16 pre-existing upstream pinning tests plus exactly the three declared controls — no undeclared test passes on both arms. Full package suite and typecheck at this head$ npm test
Test Files 23 passed (23)
Tests 726 passed (726)
Duration 22.81s
$ npx tsc --noEmit
# rc=0, no outputScope and index checks$ git diff fcd3a19 6bad58b -- src/components/PinnedScrollbar.tsx
# no output
$ git diff 30afe83..HEAD --stat
src/__tests__/pinning.test.tsx | 207 +++++++++++++++++++++++++++++++++++++
src/components/PinnedScrollbar.tsx | 10 +-
2 files changed, 216 insertions(+), 1 deletion(-)GitHub's own API agrees: CI$ gh pr checks 1 --repo askalf/react-data-table-component
no checks reported on the 'fix/pinned-scrollbar-aria-controls' branchThat is an absence of CI, not a failing check — fork Actions enablement is a one-time operator click (card The PR body has been rewritten for this head: every test count, transcript, and head sha in it is measured at |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVE — ready for the operator to submit.
No blocking issues found in the current head 6bad58bc393938c7e3b9adf49541b00a8d96f8ad.
What I checked: traced the base implementation's conditional container-id stamp at src/components/PinnedScrollbar.tsx:62-64 against its unconditional aria-controls={scrollContainerId} at line 194, then verified the changed lines preserve a host/pre-existing id and render that actual id. The focused regression coverage exercises host IDs, remounting, public unpin/re-pin, ref replacement, takeover-after-stamp, and the empty-ref guard; the PR facts sheet includes reproducible base/head A/B results. I also independently read the upstream base file, contribution requirements, commit messages, and re-ran upstream PR/issue searches for PinnedScrollbar aria-controls; no overlapping open upstream PR was found.
The PR reports no fork CI checks (an absence, not a failure) and records executed focused A/B, package-suite, typecheck, lint, formatting, and build evidence. I did not run the local suite, per reviewer environment policy.
What's good: the patch is narrowly scoped, avoids overwriting a host-owned DOM id, and the added controlsId state makes the accessibility relationship follow the element that is actually controlled.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking issues. The fix is correct, minimally scoped, reuses existing test helpers, and the new test suite discriminates the defect. Re-review at current head 6bad58bc393938c7e3b9adf49541b00a8d96f8ad (prior verdicts were at 7a9d65b; only a test-only commit was added since).
Correctness — confirmed independently
src/components/PinnedScrollbar.tsx:65-70 (diff hunk):
if (!el.id) {
el.id = scrollContainerId;
}
setControlsId(el.id);
and :203 aria-controls={controlsId} (was aria-controls={scrollContainerId}).
I traced this against src/components/DataTable.tsx:633-639, where PinnedScrollbar is conditionally rendered on hasPinnedColumns && responsive, and src/hooks/useColumnPinning.ts:73, where hasPinnedColumns derives from pinnedTotalWidths. Unpinning every column therefore unmounts PinnedScrollbar; re-pinning remounts it with a fresh React.useId() value while the DOM node it points at (scrollWrapperRef, owned by DataTable, not the scrollbar) keeps whatever id an earlier mount stamped on it. Under the old code, aria-controls={scrollContainerId} used the new mount's id even though el.id was left untouched (if (!el.id) is false the second time), producing a dangling reference. The fix's setControlsId(el.id) reads the id actually present on the element, so aria-controls always resolves. This matches the base-arm failures shown in the PR's A/B transcript (expected ':rl:' to be ':rk:' etc.), which is exactly the useId-mismatch signature this analysis predicts. I did not execute the test suite myself (no CI signal is available — gh pr checks reports no checks on this fork, consistent with the previously-filed Actions-enablement card, not a new problem), but the reasoning is checkable from source alone and the transcript's failure text is internally consistent with it.
Boundaries ledger (rebuilt from the diff, not the PR body)
| Predicate / input | Fixed behavior | Test that pins it |
|---|---|---|
if (!el.id) — empty id, first mount |
stamps scrollContainerId, controlsId set to same value |
aria-controls resolves to the scroll container it labelled (control) — genuine control: executes the changed line, output unchanged from base |
if (!el.id) — pre-existing (host) id |
branch skipped, controlsId follows host id |
aria-controls points at a host-supplied container id... — fails on base, passes on fix |
Remount, same DOM node, new useId() |
controlsId follows the retained el.id, not the new unused id |
aria-controls survives a remount onto the same container — fails on base |
Public path: unpin all → re-pin (drives the remount above through DataTable) |
thumb's aria-controls still resolves |
pinned scrollbar aria-controls still resolves after a column is unpinned and re-pinned — fails on base |
Effect dep [scrollRef, ...] — ref object identity changes to a container with its own id |
effect re-runs, controlsId follows the new container's id |
aria-controls re-points when the scroll container is swapped... — fails on base |
| Reverse order: stamp on mount 1, host overwrites after unmount, remount | follows the host's later id | aria-controls follows an id the host assigns after the first mount... — fails on base |
if (!el) return — scrollRef.current null |
effect bails before stamping or calling setControlsId; component still returns null since visible never flips |
emits no thumb and no aria-controls when the scroll ref is empty (control) — genuine control, and the only row across all re-reviews that previously had no test |
| Two independent mounts, distinct containers | each controlsId isolated to its own instance |
two concurrent scrollbars each control their own container (control) — genuine control |
No reachable row from the diff is missing a test. The three (control) tests are correctly named — each executes the changed code path (confirmed by tracing, e.g. the empty-ref test exercises if (!el) return before setControlsId is ever called) and asserts an outcome that doesn't change between arms, so they're pinning behavior rather than padding the count.
One case is explicitly and correctly left untested: an external id mutation on the container without a remount or ref-identity change (e.g. host code calls el.id = 'x' while PinnedScrollbar is still mounted on the same ref). The effect's dependency array ([scrollRef, scrollContainerId, update]) doesn't fire on that, so controlsId would stay stale — but this is unchanged from the pre-fix behavior (the old code never watched for it either) and is outside the stated bug, not a regression the fix introduces.
Idiom fit vs. upstream (jbetancur/react-data-table-component)
- Recent merged fixes use the
fix(scope): .../fix: ...commit and PR-title convention (fix(resize): enforce configured column minWidthjbetancur#1378,fix: Column resizing, pinning and reorder in RTL modejbetancur#1338). The suggested titlefix(a11y): point pinned scrollbar aria-controls at the container's real idmatches this shape. - Re-ran the prior-art searches myself:
gh search issues --repo jbetancur/react-data-table-component "aria-controls"→ no results;"PinnedScrollbar"→ only unrelated closed jbetancur#1388;gh pr list --state open→ only jbetancur#1393 (docs color-mode sync), unrelated. No overlapping open work. - Test shape: the file already mixes
render(RTL direct) andrenderWithTheme(existing helper) depending on whetherDataTableorPinnedScrollbaris under test, exactly as the new tests do — no new test infrastructure was introduced. - One naming convention is new to this file: the
(control)-suffixed test names.grep -c "(control)" src/__tests__/*.test.tsxshows zero hits outsidepinning.test.tsx— this isn't an established idiom elsewhere in the suite. It's a reasonable, self-documenting convention (and each one is a genuine control, not a placeholder), but a maintainer unfamiliar with it might ask why it's not just a comment. Not a blocker. CONTRIBUTING.mdline 1 asks contributors to "open an issue first for anything non-trivial" — that's the operator's submission-route concern, not a code finding.
What's good
- 10-line production diff, single file, no public API change.
- Every new assertion resolves the id via
document.getElementById(...), not just string-comparing attributes — that's the correct way to test "does this actually resolve," matching what assistive tech does. - Reuses
makeScrollRef/renderWithThemerather than inventing new test scaffolding.
SECOND READ: READY
VerificationFourth adversarial pass, at head Head arm — RUN v5.0.0 /agent-workspace/oss/react-data-table-component-wt-1789421152
✓ src/__tests__/pinning.test.tsx (24 tests) 888ms
✓ DataTable column pinning (12)
Test Files 1 passed (1)
Tests 24 passed (24)
Start at 22:47:22
Duration 3.86s (environment 37%, transform 26%, tests 25%, setup 6%, import 6%)Base arm — ✓ renders left-pinned column with rdt_pinLeft class on header cells 309ms
✓ renders right-pinned column with rdt_pinRight class 27ms
✓ applies rdt_pinLeftLast to rightmost left-pinned column 26ms
✓ applies rdt_pinRightFirst to leftmost right-pinned column 23ms
✓ left-pinned header cell has position:sticky style 27ms
✓ right-pinned cell has position:sticky with inline-end offset 29ms
✓ strips pinned from columns when columnGroups are active 19ms
✓ adds rdt_responsiveWrapperHideScrollbar when pinned columns present and responsive 25ms
✓ does not hide native scrollbar when responsive is false 25ms
✓ does not add hide-scrollbar class when no columns are pinned 26ms
✓ multiple left-pinned columns get sequential sticky left offsets 30ms
× pinned scrollbar aria-controls still resolves after a column is unpinned and re-pinned 84ms
✓ renders null before ResizeObserver fires (no overflow detected yet) 4ms
✓ becomes visible after scroll event fires with overflow content 3ms
✓ applies logical margin insets to the track element 2ms
✓ thumb width is proportional to viewport/scroll ratio 12ms
✓ track click outside thumb scrolls the container 12ms
✓ aria-controls resolves to the scroll container it labelled (control) 8ms
× aria-controls points at a host-supplied container id instead of a fresh one 10ms
× aria-controls survives a remount onto the same container 11ms
× aria-controls re-points when the scroll container is swapped for another one 10ms
× aria-controls follows an id the host assigns after the first mount stamped one 9ms
✓ two concurrent scrollbars each control their own container (control) 9ms
✓ emits no thumb and no aria-controls when the scroll ref is empty (control) 3ms
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 5 ⎯⎯⎯⎯⎯⎯⎯
FAIL src/__tests__/pinning.test.tsx > DataTable column pinning > pinned scrollbar aria-controls still resolves after a column is unpinned and re-pinned
AssertionError: expected ':rl:' to be ':rk:' // Object.is equality
FAIL src/__tests__/pinning.test.tsx > PinnedScrollbar > aria-controls points at a host-supplied container id instead of a fresh one
AssertionError: expected ':rs:' to be 'host-app-scroll-container' // Object.is equality
FAIL src/__tests__/pinning.test.tsx > PinnedScrollbar > aria-controls survives a remount onto the same container
AssertionError: expected ':ru:' to be ':rt:' // Object.is equality
FAIL src/__tests__/pinning.test.tsx > PinnedScrollbar > aria-controls re-points when the scroll container is swapped for another one
AssertionError: expected ':rv:' to be 'second-scroll-container' // Object.is equality
FAIL src/__tests__/pinning.test.tsx > PinnedScrollbar > aria-controls follows an id the host assigns after the first mount stamped one
AssertionError: expected ':r11:' to be 'host-took-over-later' // Object.is equality
Test Files 1 failed (1)
Tests 5 failed | 19 passed (24)
Duration 3.29s (environment 35%, transform 29%, tests 25%, setup 6%, import 5%)Per-test before/after, every test now on the branch:
The other 16 of the 24 are the pre-existing upstream pinning tests; they pass on both arms and are unrelated to the change. No undeclared test passes on both arms. Full package suite and typecheck at this head: $ npx tsc --noEmit
# rc=0, no output
$ npm test
Test Files 23 passed (23)
Tests 726 passed (726)
Duration 27.69sLedger rebuilt from the diff, not the body. Index hygiene. The base arm was reverted with $ gh pr checks 1 --repo askalf/react-data-table-component
no checks reported on the 'fix/pinned-scrollbar-aria-controls' branchThat is an absence of CI, not a failing check: Actions on this fork still need the one-time operator enablement click. No non-green job exists to explain. Everything holds at this head. |
askalf
added
verified
|
Upstream: submitted (see ledger). |
Upstream CI-RED at 9913338, addressed at 92f7524Upstream PR: jbetancur#1394. Job "Lint, typecheck, test, build" (run 35343232974) failed at the lint step, ours, not pre-existing: That line is the The job stopped at lint, so typecheck never ran upstream. Running it locally on 9913338 showed a second failure in the same commit that CI would have hit next:
Change at 92f7524 (test file only, 4+/2-): braces on the for-of, and Local run of the four CI steps on 92f7524: Upstream CI run 35803312627 for 92f7524 is |
3 participants
Summary
PinnedScrollbarused this mount'sReact.useId()foraria-controls, but assigned that id to the scroll container only whenel.idwas empty.scrollRefswap—therefore left the thumb pointing at a nonexistent id.el.idafter the existing stamp-or-preserve branch and rendersaria-controls={controlsId}. It changes 10 source lines and no public API.6bad58bc393938c7e3b9adf49541b00a8d96f8ad: it contains the source fix plus eight focused tests (five discriminating, three explicitly named controls).tscrc=0); the transcripts quoted below are pass 4's.Upstream
jbetancur/react-data-table-component; default branch:master.30afe83386721b037652e87cd2ee9c34d0d43603.fix/pinned-scrollbar-aria-controls/6bad58bc393938c7e3b9adf49541b00a8d96f8ad.src/components/PinnedScrollbar.tsx(fix) andsrc/__tests__/pinning.test.tsx(eight focused tests).git diff 30afe83..HEAD --stat= 2 files, +216/-1, matching GitHub's API (changedFiles: 2, additions: 216, deletions: 1).Bug
PinnedScrollbarusesscrollContainerId = React.useId()both as the conditional id stamp and as the unconditionalaria-controlsvalue. Its effect preserves a nonemptyel.id, so a new component mount over an already-labelled wrapper gets a freshuseIdbut leaves the wrapper's old/host id intact. The thumb's reference then dangles. The public<DataTable />path is pinned + responsive columns being unpinned then re-pinned; the component path also supports host-supplied ids and ref/container changes. This is nonvisual but breaks assistive technology's target resolution.Repro
At base source
30afe83, retaining the current 24-test file and reverting only the changed production source:The three base-passers are intentionally named
(control): first mount with no id (the changed effect executes but reads the same generated id the base renders), two independent concurrent instances (per-instance isolation), and an emptyscrollRef(the effect bails atif (!el) return, so no thumb and no attribute are emitted at all). They are not presented as regression proofs.Fix
Add
controlsIdstate initialized fromscrollContainerId; in the existing synchronization effect, callsetControlsId(el.id)immediately afterif (!el.id) { el.id = scrollContainerId; }; renderaria-controls={controlsId}. Thus the element's actual id is used in both branches. A ref was rejected because mutating one would not render the thumb; overwritingel.idwas rejected because it would clobber host DOM ownership; aMutationObserverfor id changes during a still-active mount is unrelated to this existing defect and remains out of scope.Test evidence
Focused A/B, executed in Node v24.19.0 / vitest 5.0.0 / jsdom 30, with the same 24-test file on both source arms. Every test now on the branch is listed; the three controls are marked as such. The Base/Head columns are the pass-4 re-measurement at this head.
scrollRefchanges to another host-id container(control)(control)scrollRefemits no thumb/noaria-controls(control)The remaining 16 of the 24 tests in the file are the upstream pinning tests that predate this branch; they pass on both arms and are unrelated to the change.
Complete package suite at this head (pass-4 run):
The Hunter previously ran
npx eslint src,npx tsc --noEmit,npx prettier --check "src/**/*.{ts,tsx}", andnpm run buildsuccessfully at the source-fix head. The source diff across every verification head is empty (git diff fcd3a19 6bad58b -- src/components/PinnedScrollbar.tsxproduces no output), so only tests were added after the reviewed source head.Verification method
executed, in four independent passes. No pass changed production source; the only commits after the fix add tests.Pass 1 (head
7a9d65b): the verifier rebuilt the boundary ledger from the diff rather than trusting this document and added three tests for uncovered reachable rows (ref/container swap, host-id-takeover-then-remount, concurrent-instances control).Pass 2 (head
6bad58b, this head): added the empty-scrollRefcontrol for theif (!el) returnledger row, which previously had no test, then re-ran both arms over the full 24-test file.Pass 3 (head
6bad58b, this head, 2026-09-14T22:35–22:38Z): a fresh run re-measured, rather than inherited, both arms and the whole suite after the candidate re-entered the gate. Head arm 24/24 pass in 3.91s; base arm (git checkout 30afe83 -- src/components/PinnedScrollbar.tsx, all 24 tests left in place) 5 failed / 19 passed in 3.55s, the 19 being the 16 pre-existing upstream pinning tests plus exactly the three declared controls — no undeclared test passes on both arms.npx tsc --noEmitrc=0;npm test23 files / 726 tests / 25.01s. No new test was needed: the ledger below was rebuilt from the diff again and every reachable row already had a test. Restored withgit checkout HEAD -- src/components/PinnedScrollbar.tsx(index and worktree), after whichgit status --porcelainwas empty,git diff HEAD --statwas empty, andgit show HEAD:src/components/PinnedScrollbar.tsxstill containscontrolsId,setControlsId, andaria-controls={controlsId}.Pass 4 (head
6bad58b, this head, 2026-09-14T22:47–22:50Z): the candidate re-entered the gate again with the head unchanged, so a fresh run re-measured rather than inherited every number in this document. Head armnpx vitest run src/__tests__/pinning.test.tsx= 24 passed (24) in 3.86s. Base arm (git checkout 30afe83 -- src/components/PinnedScrollbar.tsx, all 24 tests left in place) = 5 failed | 19 passed (24) in 3.29s, with the same five assertion texts quoted under## Reproand the same three(control)tests among the 19 base-passers — no undeclared test passes on both arms.npx tsc --noEmitrc=0.npm test= 23 test files / 726 tests passed / 27.69s. The## Boundariesledger was rebuilt fromgit diff 30afe83..HEAD -- src/components/PinnedScrollbar.tsxa third time (three hunks: thecontrolsIdstate declaration, thesetControlsId(el.id)call after the stamp-or-preserve branch, and thearia-controls={controlsId}attribute); every reachable row already has a test, so no test was added and the branch head is unchanged by this pass. Restored withgit checkout HEAD -- src/components/PinnedScrollbar.tsx, after whichgit status --porcelainandgit diff HEAD --statwere both empty andgit show HEAD:src/components/PinnedScrollbar.tsxstill containscontrolsId,setControlsIdandaria-controls={controlsId}.That is an absence of CI, not a failing check: fork Actions enablement still requires the one-time operator action (card
00MU1IPZMJ8C75BF22A9A677D2). No non-green job exists to explain. No real-browser-specific layout behavior is changed: the defect and assertions are DOM id/reference resolution, fully exercised under jsdom.Head history, stated because the branch was force-updated once: an earlier bad test-only head was force-corrected before the reviewed source head
fcd3a19; none of the four verification passes force-updated anything. The source is byte-identical acrossfcd3a19,7a9d65b, and6bad58b— every head after the fix adds onlysrc/__tests__/pinning.test.tsxtests.Prior art
gh pr list --repo jbetancur/react-data-table-component --state open --limit 100returned only unrelated docs PR feat(docs): synchronize site and demo color modes jbetancur/react-data-table-component#1393.gh search issues --repo jbetancur/react-data-table-component "aria-controls" --limit 10returned no results.gh search issues --repo jbetancur/react-data-table-component "PinnedScrollbar" --limit 10returned closed, unrelated [BUG]: Column filter popup ignores RTL when aligning to its button jbetancur/react-data-table-component#1388.git log --oneline -20 -- src/components/PinnedScrollbar.tsxshowed recent accepted pinning/responsive work but noaria-controlschange.No open PR or issue overlaps this defect. The scouting hypothesis jbetancur#1392 was separately assessed and is not claimed here.
Policy
CONTRIBUTING.mdsays: line 11, "Bug fixes with a clear reproduction"; line 12, "TypeScript or accessibility improvements"; line 20, a PR is rejected if it "Fails CI (lint, typecheck, tests, build)"; line 26, "Check CI passes locally before pushing:npm run lint && npm run typecheck && npm test && npm run build"; line 41, "Prettier + ESLint enforced — runnpm run formatif needed". CONTRIBUTING, CLAUDE.md, the PR template, and CODE_OF_CONDUCT are silent on AI/LLM/agent contribution rules. No CLA, DCO, changelog, or version-bump requirement was found.Disclosure facts for the operator
tsc --noEmitwere executed locally; results are quoted above.Boundaries
if (!el.id)falsyel.idon fresh mount(control)if (!el.id)truthyuseId[scrollRef, ...]if (!el) return{ current: null })setControlsId;visiblenever flips, so no track, no thumb, noaria-controlsemittedscrollRef(control)if (!visible) return nullaria-controlsemitted(control)Suggested upstream PR title
fix(a11y): point pinned scrollbar aria-controls at the scroll container's actual id