chore(ci): import-cycle ratchet + mobile viewport regression suite - #808
Conversation
Two guardrails from the UI audit's Stage-1 'protect trust and stop regressions' list, each targeting a bug class that has repeatedly reached production: 1. Import-cycle ratchet (scripts/check-cycles.mjs + .cycles-baseline.json + CI job in the 'all' hard gate). Value-level import cycles typecheck and build cleanly but crash at cold runtime with TDZ errors — the #803/#804 prod-crash class. madge (config in .madgerc) skips 'import type' edges since those are erased at compile time; the 33 pre-existing runtime cycles are baselined and only NEW cycles fail, mirroring the lint-ratchet pattern. Verified: synthetic two-file cycle is detected, baseline run is green. 2. Mobile viewport regression suite (e2e/mobile-viewports.spec.ts + mobile-* Playwright projects + blocking step in playwright.yml). Functional checks at 320/390/430px on public + seeded baseball coach/player routes: no horizontal page pan, no partially-clipped header/nav controls (the #806 'Today pill' class), no fixed-element/ bottom-nav collisions (the #799 FAB class). Plain viewports rather than isMobile device emulation — mobile emulation zooms out on overflow, which defeats the geometry checks. Helpers validated against known-good and known-bad fixture pages (all three defect classes detected; clean page passes). playwright.config.ts also skips webServer when PLAYWRIGHT_BASE_URL points at an external deployment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV
|
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. |
The e2e job in playwright.yml (where the full mobile suite runs) only executes on main pushes and manual dispatch — PRs get pr-smoke. Add the public-route mobile project there as a blocking step so phone-width layout regressions are caught before merge, not after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds baseline-ratcheted Madge runtime import-cycle checks and makes them a required CI gate. It also adds Playwright mobile viewport regression coverage for public, coach, and player routes, with dedicated projects and shared dev-server handling in CI. ChangesQuality gates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant checkCycles as check-cycles.mjs
participant Madge
participant Baseline as cycles-baseline.json
CI->>checkCycles: run check:cycles
checkCycles->>Madge: analyze src imports
Madge-->>checkCycles: return cycles and skipped files
checkCycles->>Baseline: compare current results
checkCycles-->>CI: pass or fail import-cycle gate
sequenceDiagram
participant CI
participant Playwright
participant AuthState as setup storageState
participant App
CI->>Playwright: run mobile projects
Playwright->>AuthState: load authenticated coach/player state
Playwright->>App: navigate routes at mobile viewports
App-->>Playwright: render page and final URL
Playwright-->>CI: report layout and route assertions
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 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 |
Greptile SummaryThis PR adds CI guardrails for import cycles and mobile viewport regressions. The main changes are:
Confidence Score: 4/5This is close, but the PR smoke workflow should be fixed before merging.
.github/workflows/pr-smoke.yml Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
.github/workflows/pr-smoke.yml:101-103
**Second Server Lifecycle**
This step starts another standalone Playwright run in the same job right after the accessibility run. In this workflow `PLAYWRIGHT_BASE_URL` is not set, so Playwright starts `npm run dev`; in CI, `reuseExistingServer` is false, so the first server is torn down and this step starts a new one on port 3000. If the port has not been released yet, the mobile viewport check can fail before it tests any layout behavior. Run this project in the existing Playwright invocation or point both steps at one prestarted server.
Reviews (2): Last reviewed commit: "fix(ci): madge API over CLI, skipped-fil..." | Re-trigger Greptile |
…tom-nav detection Two Greptile findings: 1. The standalone mobile step booted a second Playwright webServer in the same job, risking port-3000 contention with the smoke run's server. The mobile projects now ride the mandatory-smoke invocation (one process, one server). The 'public routes only' fallback step was dead code — the job hard-fails earlier when baseball secrets are missing. 2. The bottom-nav collision check silently passed when it couldn't identify the nav. Detection now also accepts tab-bar-SHAPED fixed elements (bottom-pinned, near-full-width, 2+ interactive children), and authenticated dashboard routes assert the bottom nav exists at phone widths per MOBILE_DOCTRINE — so a missed nav fails loud instead of skipping the check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@e2e/mobile-viewports.spec.ts`:
- Around line 68-73: Update expectNoClippedControls to reuse the bottom-nav
detector already used by expectNoBottomNavCollision, and include any detected
plain fixed bottom-nav wrapper when collecting controls alongside semantic
containers. Preserve the existing control de-duplication and clipping checks.
- Around line 178-181: Update the coach viewport tests around the “fits” cases
to assert immediately after navigation that the final pathname matches the
requested route, rather than only checking that it does not contain “/login”.
Apply the same route assertion to both affected test blocks, preserving any
explicitly documented redirect exceptions if the test suite defines them.
In `@scripts/check-cycles.mjs`:
- Around line 40-43: Update the Madge invocation in the cycle-check script to
run only the installed project binary, replacing the npx command with npm exec
--no -- madge or the equivalent local binary invocation. Preserve the existing
arguments and execution options so missing dependencies fail rather than being
downloaded.
- Around line 84-88: In the UPDATE branch of the baseline-writing flow, add an
“updating baseline” log immediately before writeFileSync mutates BASELINE_PATH.
Keep the existing success message after the write completes, and limit the
change to preserving intent-before-mutation ordering.
- Around line 40-52: Update the Madge execution and validation around
madgeOutput to detect any skipped or unresolved files using Madge warnings data
or a separate --warning pass. If skipped files are reported, print the relevant
error details and exit nonzero before cycle results are treated as resolved;
preserve the existing JSON parsing and failure handling for valid output without
skipped files.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c152fd4-d718-4ad4-a2f6-6d9a3eb2f3ad
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (9)
.cycles-baseline.json.github/workflows/ci.yml.github/workflows/playwright.yml.github/workflows/pr-smoke.yml.madgerce2e/mobile-viewports.spec.tspackage.jsonplaywright.config.tsscripts/check-cycles.mjs
…detector
check-cycles.mjs now calls madge's Node API in-process instead of shelling
out to `npx madge` — no unpinned-package fetch is possible, and we get
res.warnings().skipped, which the CLI can never expose in --json mode (see
bin/cli.js: it only prints --warning output when NOT combined with --json).
Skipped files (e.g. this repo's permanent `server-only` bare-specifier,
aliased by Next.js at build time without ever being an installed package)
are now baseline-ratcheted the same way cycles are, so a genuinely new
unresolved import fails closed instead of silently hiding a cycle, without
permanently red-ing the gate on the known benign case. .cycles-baseline.json
moves from a flat cycle array to { skipped, cycles } (same 33 cycles,
verified byte-identical to the prior baseline). Baseline-file logging now
happens before the write, not after.
mobile-viewports.spec.ts: the bottom-nav detector (semantic nav/role OR
tab-bar-shaped fixed wrapper) is now computed once and reused by both the
clipped-controls scan and the collision scan, so a plain fixed bottom-nav
wrapper with no <nav>/role can no longer evade the clipped-controls check.
Coach/player route tests now assert the exact final pathname instead of
only rejecting /login, since a server route guard can redirect an
authed-but-unauthorized user to a different dashboard route and still pass
the old check.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
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. |
Same class of bug already fixed in playwright.yml, in a workflow that fix didn't touch: pr-smoke.yml ran the accessibility smoke and the public mobile viewport check as two separate `npx playwright test` invocations back to back, each managing its own webServer since neither set PLAYWRIGHT_BASE_URL. In CI reuseExistingServer is false, so the mobile step's server boot could race the accessibility step's server teardown on port 3000. Unlike playwright.yml's mandatory-smoke step, these two can't just be merged into one `npx playwright test` invocation — the accessibility step is intentionally advisory (continue-on-error, due to known pre-existing WCAG-AA contrast debt) while the mobile step is intentionally blocking, and merging would collapse that distinction into one exit code. Instead: start the dev server once in its own step, wait for it to respond, and point both test steps at it via PLAYWRIGHT_BASE_URL (which playwright.config.ts's webServer block treats as "skip booting your own server"). Stops the background server at the end regardless of outcome. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…811) Root cause of the intermittently-failing engine-run-helm-lifting test (and a latent correctness gap that would eventually hit every rolling- window metric the same way): loadLiftMetrics, loadReadinessMetrics, loadPlayerMetrics' workload calc, and loadCatchingMetrics all computed their cutoff from Date.now() — the real wall clock — instead of the nowIso already threaded through runBaseballEngineCore for deterministic runs. A test fixing "now" at a past date and seeding data just inside a 14-day window would pass only as long as real time hadn't yet carried that window past the seeded date; it then fails with zero code changes once it did (confirmed: passed on this exact branch 3 days before it started failing, purely from the calendar advancing). Threaded nowIso through the full call chain instead of just the one function the failing test touches: mergeV10PlayerMetrics -> { loadReadinessMetrics, loadLiftMetrics }, loadAllPlayerMetrics -> loadPlayerMetrics, mergeEventPlayerMetrics -> loadCatchingMetrics, with engine-run.ts's three call sites now passing its own nowIso through. Each function's new nowIso param defaults to new Date().toISOString() (matching the existing input.now ?? new Date() pattern in effectiveness/engine.ts) so the three OTHER callers of loadPlayerMetrics (practice-effectiveness.ts, action-baseline.ts, outcome-sweep.ts) keep their current real-time behavior unchanged — only the deterministic engine-run path needed fixing. Left out of scope (same class, but a bigger architectural change and not the cause of any current failure): generators/v10.ts's importQualityGenerator also filters recent import runs via Date.now(), but BaseballV10EngineInputs has no now/nowIso field for generators at all today — adding one is a separate, larger PR. Verified: full `npm run test:run` (unit project, 494 files/4921 tests) and `npm run test:business` (business project, 505 files/4996 tests) both pass clean — these are the exact commands CI's "Unit tests" and "Business contracts" jobs run, and the ones that were failing on PR #808 before this fix. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/pr-smoke.yml:
- Around line 106-112: Update the startup polling loop in the workflow around
the curl readiness check to track the dev server process and verify it remains
alive on each iteration. If the process has exited, print or expose its startup
logs and fail immediately; otherwise preserve the existing readiness polling and
timeout behavior.
- Line 94: Add a kill -0 "$DEV_PID" process-aliveness check inside the
development server readiness loop, alongside the existing readiness polling, and
immediately fail with the appropriate error when the server process has exited.
Preserve the current timeout and successful readiness behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7dcd0315-2454-4384-9baa-f3c37555d9af
📒 Files selected for processing (1)
.github/workflows/pr-smoke.yml
CodeRabbit round on #808: poll loop hung the full 120s when npm run dev crashed immediately; now checks the pid each iteration and surfaces the log at once. (Its second comment — missing permissions block — is stale: pr-smoke.yml:15 already has one.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 1 minute. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@e2e/mobile-viewports.spec.ts`:
- Around line 199-202: Update the PUBLIC_ROUTES test using expectMobileFit to
also assert that navigation ends on the requested route pathname, matching the
existing coach and player route checks. Ensure redirects to login, error, or
fallback pages fail the test while preserving the viewport-fit assertion.
- Line 107: Update the clipping-detection selector in the viewport test loop to
include elements matching [role="tab"], alongside the existing link, button, and
button-role selectors. Keep the bottom-navigation handling and other clipping
checks unchanged.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f3051eb-5173-4ef6-ad8e-6d1d5ae817c3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (9)
.cycles-baseline.json.github/workflows/ci.yml.github/workflows/playwright.yml.github/workflows/pr-smoke.yml.madgerce2e/mobile-viewports.spec.tspackage.jsonplaywright.config.tsscripts/check-cycles.mjs
…hname CodeRabbit full-review round: [role=tab] counted for bottom-nav shape but was invisible to clipping detection; public-route tests could measure a redirect target and pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…de changes) (#853) Syncs 5 living docs + 1 memory file to actual tonight's reality on batch/bbh-finish-0714 @ 0056bc0, independently re-verified (not copied from PR claims) via grep/gh API/local test runs: - PRODUCTION_READINESS_MISSION_2026-07-09.md: dated addendum (history kept intact) covering #792-#807 merged, discover-privacy P0 fixed+tested, 29-surface Living-Annual migration done, tonight's batch merge state (#808 merged not "green-pending", #810 still open, #812-#841 + #851 on batch branch, #842-#850 still open), and the batch HEAD's 3 currently-red CI checks (Business contracts/Unit tests/Import-cycle ratchet). - ui-migration-map.md + ui-migration-execution-plan.md: code-verified status headers — all 29 surfaces executed, Batch H (PR #820) done, zero isRedesignEnabled() forks remain under src/app/baseball or src/components/baseball. - BASEBALLHELM_FEATURE_READINESS_MATRIX.md: ran check-readiness-matrix.ts (green before and after); upgraded Documents, Travel, Practice, Staff/Roles to ready and Practice Effectiveness to partial on real new test-coverage PRs (#822-#825); updated Player Today/Signals/Videos with tonight's #377 contract tests (#826) and #379 Phase 4a progress (#851); rollup 10->14 ready. Re-ran the checker (route resolution + live owner-issue validation) clean after edits. - BASEBALLHELM_PRODUCTION_VERDICT.md: reissued (old 2026-06-25 verdict kept as history below a new 2026-07-15 section) — honest "batch branch pending integration merge + CI" verdict, deferred-minors list, and the journey/pipeline vocabulary decision, #379 legacy-backfill scope, marketing-root (helm-website-ui/ vs src/app/page.tsx), and dual-wizard (ImportWizardClient vs EventImportWizard) open decisions, each grounded in a specific file/PR. - memory/context/baseballhelm-features.md: corrected narrative lines now verifiably false (stale 2026-06-30 rollup counts, decision-room "unapplied migration"/#405-406 "open", pipeline "7 columns vs 5-stage enum", journey "UNVERIFIED source table", discover.ts profile_visibility omission, documents #393) — no AUTOGEN blocks in this file, none touched. Gates: check:readiness-matrix exit 0 (route resolution + live GITHUB_TOKEN owner-issue validation); readiness-matrix-routes.test.ts 204/204 passing; no markdownlint config present in repo (skipped per task instructions). Docs-only change; no product code touched. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Adds a baseball-auth-smoke job to ci.yml that runs the already-built e2e/baseball-smoke.spec.ts + e2e/baseball-onboarding-smoke.spec.ts (with their existing fail-loud auth setup) as a required PR gate, folded into the `all` aggregate — the same promotion mechanism used for Supabase RLS tests (#517) and the import-cycle ratchet (#808). Previously this authenticated coach/player smoke only ran post-merge via playwright.yml's push/manual-only `e2e` job, which stayed untouched. Skips (not fails) on fork/Dependabot PRs, which get no repo secrets; same-repo pushes/PRs must have the required secrets or the job fails loudly. Docs updated to reflect the new hard-gate status and its added CI-minutes cost. Also unmasks the `|| echo "Playwright suite has failures..."` exit-code shim in playwright.yml's advisory "Playwright (chromium)" job so a real failure there turns the job red instead of silently passing (reviewer- flagged on #812 as blocking trust in the e2e gate). That job is advisory, not a required check, so this cannot newly block a merge — it only makes the status honest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) * ci: promote BaseballHelm authed smoke to a required PR gate (#372) Adds a baseball-auth-smoke job to ci.yml that runs the already-built e2e/baseball-smoke.spec.ts + e2e/baseball-onboarding-smoke.spec.ts (with their existing fail-loud auth setup) as a required PR gate, folded into the `all` aggregate — the same promotion mechanism used for Supabase RLS tests (#517) and the import-cycle ratchet (#808). Previously this authenticated coach/player smoke only ran post-merge via playwright.yml's push/manual-only `e2e` job, which stayed untouched. Skips (not fails) on fork/Dependabot PRs, which get no repo secrets; same-repo pushes/PRs must have the required secrets or the job fails loudly. Docs updated to reflect the new hard-gate status and its added CI-minutes cost. Also unmasks the `|| echo "Playwright suite has failures..."` exit-code shim in playwright.yml's advisory "Playwright (chromium)" job so a real failure there turns the job red instead of silently passing (reviewer- flagged on #812 as blocking trust in the e2e gate). That job is advisory, not a required check, so this cannot newly block a merge — it only makes the status honest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ci): skip baseball-auth-smoke on Dependabot PRs, not just forks Dependabot PRs are opened against this repo (not a fork), so the existing head.repo.full_name == github.repository check alone did not exclude them. The job's own docs already claimed a Dependabot skip, but without secrets it would hard-fail on the pull_request branch, blocking the required `all` aggregate on every Dependabot PR. Add an explicit github.actor check and sync the "skips on fork/Dependabot PRs" prose in the job comment, branch-protection.md, and CI_RUNBOOK.md to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
First PR from the UI-audit remediation Nick approved — the two Stage-1 guardrails, each targeting a defect class that has repeatedly shipped to production:
1. Import-cycle ratchet (
scripts/check-cycles.mjs,.cycles-baseline.json,.madgerc, newimport-cyclesCI job wired into theallhard gate). Value-level import cycles typecheck and build cleanly but crash at cold runtime with "Cannot access X before initialization" — the exact class behind the #803 golf CoachHelm and #804 baseball roster prod crashes. madge is configured to skipimport typeedges (erased at compile time, TDZ-safe); the 33 pre-existing runtime cycles are baselined so only NEW cycles fail, mirroring the existing lint-ratchet pattern.npm run check:cycles -- --updatelocks in reductions.2. Mobile viewport regression suite (
e2e/mobile-viewports.spec.ts, threemobile-*Playwright projects, blocking step inplaywright.yml). Functional assertions at 320/390/430px on public routes plus CI-seeded baseball coach/player routes:Plain viewports rather than
isMobiledevice emulation — mobile emulation zooms out on overflow, growinginnerWidthwith the content, which silently defeats the pan/clip geometry checks (found while validating the helpers).Verification
npm run typecheckclean.Type of change
Area
shared · CI · mobile
Risk level
Git Activity Timeline note
Adds CI guardrails that block new runtime import cycles and phone-width layout regressions — the two bug classes behind several recent production fixes.
Checklist
npm run typecheckpassesnpm run check:cyclesgreen on baseline; detection verified with a synthetic cycle🤖 Generated with Claude Code
https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV
Generated by Claude Code