fix(drawer): shadow clipped at drawer edge — scroll area owns the L/16 padding - #3019
Conversation
…r clip Root cause: DrawerContent's overflow-auto scroll wrapper clips painting at its own box edge. Consumers put the horizontal padding on the panel AROUND that wrapper (or on the panel via className), so a w-full button's 4px offset shadow fell outside the clip box and was cut in a straight vertical line — the avatar drawer, KYC status drawer, token selector and QR drawer all hit or worked around it locally. Fix: the scroll wrapper now carries the design.md container padding (px-4, L/16) INSIDE the overflow box, so shadows always land in the padded, unclipped region. DrawerHeader/DrawerFooter drop their own horizontal padding (py-4) to match. Every consumer's now-redundant horizontal padding is removed — content width is unchanged everywhere (16px moved inside the clip box, not added). Local workarounds retired: AvatarPicker's scrollAreaClassName px-4, QRBottomDrawer's pr-1 gutter. KycStatusDrawer's pb-12 moved into the scroll area so the retry button's bottom shadow paints too.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Code-analysis diffPainscore total: 7260.1 → 7262.35 (+2.25) 🆕 New findings (8)
✅ Resolved (7)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
🖼 Visual diff — 5 screens moved5 of 68 shots changed · 63 identical · baseline
job summary · before/after/diff images — artifact Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data. |
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The drawer padding migration is internally consistent and exact-head CI is green, but the canonical drawer recipe still teaches the old padding contract and will double-pad new callers.
Findings
- MAJOR · src/components/Global/Drawer/index.tsx:129 · Update the canonical drawer recipe with the new padding owner
The canonical design-system drawer recipe still tells callers to passclassName="p-4"toDrawerContent, and its nested-drawer example does the same. This line now addspx-4to a separate inner scroll wrapper, so a caller following that recipe gets 16px on the panel plus 16px in the scroll area (32px total horizontal inset), directly contradicting the new rule in the adjacent comment. Update the canonicaldesign.mddrawer and nested-drawer recipes as part of this rollout, including thescrollAreaClassName="px-0"escape hatch for intentionally edge-to-edge content.
Checked clean
- Detached worktree HEAD, trusted author, base ref, and base SHA match the supplied review coordinates.
- Exact-head required CI, unit tests, typecheck, lint, design-system lint, and visual snapshots completed successfully.
- Reviewed the full DrawerContent consumer census; existing horizontal padding and shadow-gutter workarounds were removed consistently.
- Checked overflow, snap-point, nested-drawer, safe-bottom, and scroll-area override behavior; no runtime defect was found in the changed consumers.
Security review: did not run — this change has no security, privacy or money surface, so it was not asked. This review is one reviewer short.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 49b5a902d658 · Context: repo, design · Took 7m
Three Kush findings, one commit because they share the drawer chrome: 1. Sheet background back to bg-background-page. e55b5d1 (#2984) deliberately went white; Kush's 2026-09-07 ruling reverts it — the sheet matches the app page background. Flagged in the PR body since it reverses a 3-day-old deliberate change. 2. Removed the Content-level onTouchMove stopPropagation. It silenced vaul's own document-level touchmove handlers (scroll containment + drag coordination), which is why dragging the sheet from its body felt broken and touches seemed to reach the page behind. The reason it existed — the document-level pull-to-refresh reacting to drawer touches — is now fixed at the root: usePullToRefresh ignores any touch starting on [data-vaul-drawer]/[data-vaul-overlay] (with a regression test). Whole-sheet drag-to-dismiss and no-P2R-during-drag verified with synthesized CDP touch swipes. 3. scrollbar-none on the drawer scroll container — Android flashes a scrollbar there while the sheet is dragged (tailwind-scrollbar utility, same overflow chain as the shadow fix).
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The drawer implementation is coherent, but the canonical drawer recipes still prescribe the old padding owner and now create double horizontal padding.
Findings
- MAJOR · src/components/Global/Drawer/index.tsx:129 · Update the canonical drawer recipe with the new padding owner — also flagged by claude-opus
This new invariant says the inner scroll wrapper owns the L/16 horizontal padding, butdesign/design.mdstill shows both ordinary and nested drawers withDrawerContent className="p-4"and explicitly says the panel owns that inset. A caller following the canonical recipe now gets the panel's 16px inset plus the scroll wrapper's new 16px inset, rendering content 32px from each edge and contradicting the rule introduced here. Update both canonical examples and the padding paragraph to document the scroll-area default and thescrollAreaClassName="px-0"edge-to-edge escape hatch.
Checked clean
- Confirmed the detached worktree head and merge base match the supplied SHAs.
- Reviewed the full Drawer consumer padding sweep; no remaining production consumer unintentionally double-pads at this head.
- Traced the vaul drawer and overlay touch targets through the pull-to-refresh guard; the focused Drawer and usePullToRefresh suites pass (17 tests).
- Required exact-head CI is green; advisory ds-shots is still in progress.
Security review: did not run — this change has no security, privacy or money surface, so it was not asked. This review is one reviewer short.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 5422facba2d8 · Context: repo, design · Took 9m
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
The drawer padding migration and Vaul touch handling are sound. The previously raised canonical-doc mismatch remains, but the author explicitly moved that accepted fix to open mono PR #139, so it is deferred there.
Answered by a human, not raised again
These are still present as far as this review can tell. You resolved the thread or deferred the fix, so Chip files them instead of repeating them.
- MAJOR · src/components/Global/Drawer/index.tsx:129 · Finish the deferred canonical drawer recipe update — task
Checked clean
- Confirmed the detached worktree HEAD and merge base exactly match the supplied head and base SHAs.
- Reviewed every DrawerContent consumer migration for preserved horizontal inset and shadow clearance, including the QR snap-point drawer and KYC bottom padding.
- Reviewed removal of the content touchmove propagation guard and the pull-to-refresh exclusion for Vaul drawer and overlay targets.
- Exact-head required CI is green; advisory ds-shots remains in progress.
- Focused local Jest execution was unavailable because the detached worktree has no node_modules; exact-head unit CI passed.
Security review: did not run — this change has no security, privacy or money surface, so it was not asked. This review is one reviewer short.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 4e809631320b · Context: repo, design · Took 8m
1 participant
Summary
Global fix for clipped button shadows inside drawers (Kush's repro: the avatar-change drawer's "Tirar los dados" / "Listo" buttons with their right shadow cut flush).
Task: TASK-21446 (DS umbrella; drawer anatomy per design.md)
Root cause
DrawerContent's scroll wrapper isoverflow-auto, and CSS clips painting at that element's own box edge. Consumers put the drawer's horizontal padding on the panel AROUND the wrapper, so aw-fullbutton spanned the whole scroll box and its 4px offset shadow fell outside it — cut in a straight vertical line. The bug class had already collected local workarounds (AvatarPicker'sscrollAreaClassName="px-4"on 2026-09-05, QRBottomDrawer'spr-1gutter) — proof it was a shared-component problem, not a per-screen one.Fix (shared component, one rule)
The scroll wrapper now owns the design.md container padding (
px-4, L/16) INSIDE the overflow box, so shadows always land in the padded, unclipped region.DrawerHeader/DrawerFootergopy-4to match. A comment inDrawer/index.tsxstates the law: never re-add horizontal padding on the panel or on drawer content.Consumer sweep (all now-redundant horizontal padding removed — content width is unchanged everywhere, the 16px moved inside the clip box, not added):
pb-12also moved into the scroll area so the retry button's bottom shadow paints), TokenSelector, QRBottomDrawerscrollAreaClassName="px-4", QRBottomDrawerpr-1gutter/dev/ds/patterns/drawerlive example + code snippet no longer teach the child-px-4patternScreenshots (375x667, mock data via fixtures)
Avatar drawer — the reported bug. Before = the pre-workaround tree (
8a1ccdbab~1, panelp-4— the state Kush reproduced); after = this branch.DS pattern-page drawer (second shadowed-button drawer): before/after pixel-identical — that example already padded inside the scroll area, confirming the migration adds no double padding.
The
pr-assets-3019branch is deleted after merge.ds-shots expectation
Drawer-bearing fixture shots should be identical or differ only by previously-clipped shadow pixels: every consumer's content width is unchanged. The avatar-picker fixture was already workaround-fixed on dev, so it should not diff (beyond its known dice-randomness flake). Anything else diffing is a bug.
Observed: 7 screens moved, all in the known flaky set at their usual magnitudes (profile 3.19%, avatar-picker 1.35% — below its usual 1.78% dice flake, rest <=0.07%) — same screens move on sibling PRs #3011-#3015 that touch none of them. No drawer-bearing screen shows a layout shift.
Risks
scrollAreaClassName="px-0"— none exists today (verified the full consumer census).QA
prettier clean · typecheck clean · jest 485 suites / 6077 tests green · ds-lint ratchet green
Round 2 — touch behavior (Kush, 2026-09-07)
1. Sheet background: WHITE stays — ruling flipped on 2026-09-07. An earlier commit on this branch (5422fac) moved the sheet to
bg-background-page, reversing Slava's deliberate white surface from #2984 (e55b5d1, 2026-09-04). Kush reversed that decision the same day: the white sheet stays, and 4e80963 reverts the background part only. No net background change ships in this PR — the shadow-clip fix, padding ownership, touch/drag fixes, and scrollbar fix all stay. Sheet on the current head:2. Touch/drag — one root cause for the pass-through blip and the unclear grab area.
DrawerContentcarriedonTouchMove={(e) => e.stopPropagation()}since the original drawer commit. That handler silenced vaul's OWN document-level touchmove handlers — the ones that do scroll containment and drag coordination — so body-drags misbehaved and touches leaked to page-level listeners. Meanwhile the app's custom pull-to-refresh also listens ondocument, so a drawer drag could pull the home screen behind the sheet (the "blip"). Fix:usePullToRefreshnow ignores any touch starting on[data-vaul-drawer]/[data-vaul-overlay](root fix + regression test)3. Android scrollbar flash:
scrollbar-noneon the drawer scroll container (tailwind-scrollbar utility; same overflow chain as the shadow fix).On-device checks for Kush (not fully verifiable in Chromium headless): the Android scrollbar flash disappearing, real-finger drag feel on iOS/Android, and the iOS rubber-band interaction with pull-to-refresh.