fix(baseball): mobile shell header/nav controls clipped at 320/390px on every dashboard route - #927
fix(baseball): mobile shell header/nav controls clipped at 320/390px on every dashboard route#927njrini99-code wants to merge 2 commits into
Conversation
…on every dashboard route Fixes #905. CI run 29608511857 (workflow_dispatch, main @ 6d25e44) shows e2e/mobile-viewports.spec.ts's `assertNoClippedControls` failing for every baseball dashboard route, both coach and player projects, at 320/390px: "Home" [left -2, right 66] vs viewport 320 "Home" [left -2, right 80] vs viewport 390 "Operations" [left 223, right 351] vs viewport 320 "Postgame Review" [left 251, right 424] vs viewport 320/390 Two root causes, same bug class as #899's FairwayBottomNav min-w-0 fix (flex children with the default `min-width: auto` floor refusing to shrink, pushing a control past the viewport edge): 1. FairwayBottomNav's row still used `justify-around`. #899 already zeroed each column's shrink floor (`min-w-0`), but #899's own root-cause writeup names the OTHER half of the mechanism: `justify-content: space-around` falls back to `center` per the CSS Box Alignment spec whenever the line's free space goes negative, and centering an overflowing row shifts its start point negative — the exact "Home" left-edge overhang reproduced here despite #899 already being merged. Dropping `justify-around` (a no-op in the steady state, since all 5 columns are `flex-1` and flex-grow already consumes 100% of the row) removes the negative-shift mechanism entirely: any residual overflow now degrades to the LAST column bleeding off the right edge instead of shifting the FIRST column negative. 2. HubSubNav's tabs (`_components/hub-sub-nav.tsx`) used `shrink-0` on every `<li>`, giving each tab the flex item's default content-based floor — its full `whitespace-nowrap` label width. A hub is capped at ≤3 tabs (Ruling 2), but even 3 tabs with icon+padding+a longer label ("Postgame Review", "Operations") already exceed 320/390px unshrunk. `overflow-x-auto` visually clips the strip, but getBoundingClientRect reflects LAYOUT position, not ancestor clipping, so the trailing tab's own rect still bled past the viewport. Fix: `min-w-0` on the `<li>` + `min-w-0 truncate` on the label span (the same pattern #899 applied to FairwayBottomNav) — tabs now shrink-to-fit first, falling back to the strip's horizontal scroll only once even truncated tabs don't fit. Both routes checked against the settings/player-today failures in the same CI run confirm those are unrelated (auth/login redirect in that run, not a clipped-controls failure) — out of scope here. Gates: tsc --noEmit clean; eslint clean on all 4 changed files; vitest green (FairwayBottomNav.test.tsx + hub-sub-nav.test.tsx new, plus the existing FairwayTopBar/more-nav/nav-manifest/resolve-active-hub suites re-run to confirm no regression — 612+112 tests passing). No local browser/Playwright run was possible (sandboxed laptop) — the new unit tests pin the class-level fix (jsdom doesn't do real CSS layout, so they can't reproduce the geometry itself); CI's blocking `Run BaseballHelm mandatory smoke + mobile viewport regression` step re-running e2e/mobile-viewports.spec.ts against this PR is the actual verification gate for the pixel-level fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✨ 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 review |
✅ Action performedReview finished.
|
|
🤖 Mission Control — PR summary What it changes: Fixes #905. Risk / areas: baseball mobile shell (header + bottom nav) across all dashboard routes. Watch: CI: ✅ green so far — 34 checks passing, 4 pending, 0 failing; mergeable state BLOCKED on required review (no CI failure). Awaiting review. |
…n leak Root cause was NOT the flex/justify-content class list the previous commit (f6ea925) touched — that commit's fix left the measured CI geometry byte-for-byte identical ("Home" [left -2, right 66] vs viewport 320, on run 29616625581, testing f6ea925 itself), which is the tell that the change never reached the actual defect. The real cause: globals.css's "Inline link touch targets on mobile" rule @media (max-width: 1023px) { .prose a, p a, li a, span a { margin: -0.375rem -0.125rem; ... } } is a BLANKET `li a` selector — it matches every `<a>` that's a descendant of an `<li>`, anywhere in the app, not just inline links inside rendered prose (its actual intent). FairwayBottomNav's tabs and HubSubNav's tabs are both `<li><Link>` (a plain `<a>`) with no margin utility class of their own, so this rule's `margin: -0.375rem -0.125rem` (-6px/-2px) applied completely unchallenged — no competing Tailwind class means no specificity contest to win. The arithmetic: FairwayBottomNav's 5 columns are each an honest `flex: 1 1 0%` share of the row — 320px / 5 = 64px, with real headroom (icon 22px + padding way under that). No overflow should be possible. But the negative margin on the `<a>` inside each 64px `<li>` adds 2px on each side (+4px width) and shifts the box 2px left of its li's true position, independently per column. For column 0 ("Home"), li[0] spans x=[0,64], so its `<a>` renders at x=[-2, 66] — width 68, exactly matching the CI failure. Columns 1-3 shift the same way but only bleed into their neighbors (not a viewport edge), so they never tripped the assertion; the 5th column is a `<button>` (not `<a>`), immune to `li a` entirely — which is exactly why only "Home" ever showed up in the failure list. Same mechanism explains HubSubNav's "Operations"/"Postgame Review" overflow reported in the original #905 bug (also `<li><Link>`). Fix, two layers: 1. globals.css: scope the touch-target rule with `:not(nav a, [role="navigation"] a, [role="tablist"] a, [role="toolbar"] a)` so it stays exactly what its name says (inline prose links) and can never again silently margin a structural nav/tab/toolbar anchor. Verified via `npx tailwindcss -i globals.css -o ...` that the scoped selector compiles through the real PostCSS/Tailwind pipeline intact. 2. FairwayBottomNav.tsx + hub-sub-nav.tsx: `m-0` directly on each tab's `<Link>` — a class selector beats `li a`'s two-type-selector specificity, so this independently neutralizes the leak at the component level regardless of the global rule. Also corrected the now-known-wrong `justify-around`/space-around root-cause comment left by f6ea925 (kept the class removal itself — harmless, arguably more predictable — but the writeup no longer claims it was the fix, since CI proved it wasn't). Gates: tsc --noEmit clean; eslint clean on all 3 changed files; vitest green (FairwayBottomNav.test.tsx + hub-sub-nav.test.tsx + the full `nav` matcher across the unit project — 186 tests passing, no regressions). globals.css itself has no test harness; validated by running it through the actual Tailwind CLI build (not just visual inspection) to confirm the `:not()` selector is syntactically valid and survives the pipeline. No local Playwright run (per standing policy — laptop browser automation is banned; verify via CI). CI's blocking mobile-viewport e2e re-run against this commit is the actual pixel-level verification gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
Real root cause found — it wasn't the flex/justify-content classesThe previous commit on this branch (f6ea925) removed The real mechanism
@media (max-width: 1023px) {
.prose a, p a, li a, span a {
display: inline-block;
padding: 0.375rem 0.125rem;
margin: -0.375rem -0.125rem;
}
}
The arithmetic
But the negative margin on the
That's exactly why only Same mechanism explains the original Fix (two layers)
Also corrected the now-known-wrong Gates run
No local Playwright run (standing policy — no browser automation on this laptop). CI's blocking mobile-viewport e2e re-run against Co-Authored-By: Claude Fable 5 noreply@anthropic.com |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Problem
Fixes #905.
e2e/mobile-viewports.spec.ts's blocking mobile-viewport regression project failsassertNoClippedControls(header/nav controls clipped at viewport edge) for every baseball dashboard route, both coach and player projects, at 320/390px. Pulled the real failure data from the referenced CI run (gh run view 29608511857 --log-failed, workflow_dispatch onmain@6d25e44f, the same SHA this branch forked from):Two distinct root causes, both the same bug class as #899's
FairwayBottomNavmin-w-0fix (a flex child keeping the defaultmin-width: autofloor, refusing to shrink, so a control gets pushed past the viewport edge):1.
FairwayBottomNav's row still usedjustify-around. #899 already zeroed every column's shrink floor (min-w-0on the<li>/<Link>), but #899's own root-cause writeup names the other half of the mechanism it was fighting:justify-content: space-aroundfalls back tocenterper the CSS Box Alignment spec whenever the line's free space goes negative — and centering an overflowing row shifts its start point negative. That's exactly the "Home" left-edge overhang still reproducing in this run despite #899 already being merged (verified:e7900b05is an ancestor of the SHA this CI run built). Droppingjustify-aroundis a no-op in the steady state (all 5 columns areflex-1, so flex-grow already consumes 100% of the row —justify-contentnever had anything to distribute) but removes the negative-shift failure mode entirely: any stray sub-pixel/font-metric overflow now degrades to the last column bleeding off the right edge, never the first column shifting negative.2.
HubSubNav's tabs (_components/hub-sub-nav.tsx) usedshrink-0. Every<li>carried the flex item's default content-based floor — its fullwhitespace-nowraplabel width. Hubs are capped at ≤3 tabs (Ruling 2), but even 3 tabs with icon + padding + a longer label ("Postgame Review", "Operations") already exceed 320/390px unshrunk.overflow-x-autovisually clips the strip, butgetBoundingClientRect()reflects layout position, not ancestor clipping, so the trailing tab's own rect still bled past the viewport and tripped the check. Fix mirrors #899 exactly:min-w-0on the<li>+min-w-0 truncateon the label span — tabs now shrink-to-fit first, falling back to the strip's horizontal scroll only once even truncated tabs don't fit.Both shells (coach and player) render through the same
BaseballFairwayShell→AppShell→FairwayBottomNav/HubSubNav, so one fix covers both roles.I also checked the
settings/player-todayfailures in the same CI run — those are a/baseball/loginredirect (auth/session issue in that run), notassertNoClippedControls. Unrelated, out of scope here.Fix
src/components/fairway/app-shell/FairwayBottomNav.tsx: removejustify-aroundfrom the tab row.src/app/baseball/(dashboard)/_components/hub-sub-nav.tsx:shrink-0→min-w-0on each tab<li>; addmin-w-0 truncateto the label<span>.Gates
npx tsc --noEmit -p tsconfig.json— clean.npx eslinton all 4 changed files — clean.npx vitest run— newFairwayBottomNav.test.tsx+hub-sub-nav.test.tsx(class-level lock-in: nojustify-around/shrink-0,min-w-0/truncatepresent) plus the existingFairwayTopBar,more-nav,nav-manifest,resolve-active-hubsuites re-run for regression — 612 + 112 tests passing.Verification caveat: no local browser/Playwright run was possible (sandboxed laptop, per house rules). jsdom doesn't do real CSS layout, so the new unit tests can only pin the class-level fix, not reproduce the actual geometry/pixel bug. CI's blocking
Run BaseballHelm mandatory smoke + mobile viewport regressionstep re-runninge2e/mobile-viewports.spec.tsagainst this PR is the real verification gate — it should go green where it's currently red onmain.🤖 Generated with Claude Code