chore(ci): import-cycle ratchet + mobile viewport regression suite#808
chore(ci): import-cycle ratchet + mobile viewport regression suite#808njrini99-code wants to merge 3 commits into
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. 1 Skipped Deployment
|
|
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
WalkthroughChangesQuality 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 npm run check:cycles
checkCycles->>Madge: analyze src for circular imports
Madge-->>checkCycles: return cycle JSON
checkCycles->>Baseline: compare canonical cycle set
checkCycles-->>CI: pass or fail the import-cycle gate
sequenceDiagram
participant CI
participant Playwright
participant AuthState as setup storageState
participant App
CI->>Playwright: run mobile-public/mobile-coach/mobile-player
Playwright->>AuthState: load authenticated state for coach/player
Playwright->>App: navigate routes at mobile viewports
App-->>Playwright: render page and final URL
Playwright-->>CI: report layout and redirect assertions
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds CI guardrails for import cycles and mobile viewport regressions. The main changes are:
Confidence Score: 4/5The Playwright workflow can fail during the new mobile step when the local dev server is restarted between test runs.
.github/workflows/playwright.yml; e2e/mobile-viewports.spec.ts Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
.github/workflows/playwright.yml:171-177
**Dev Server Restarts Between Runs**
This mobile step runs as a second standalone Playwright process while `PLAYWRIGHT_BASE_URL` is unset, so the configured web server starts again with `reuseExistingServer` disabled in CI. If the first smoke run's Next dev server has not released port 3000 yet, this step can fail with a startup timeout or port-in-use error even though the mobile checks are fine.
### Issue 2 of 2
e2e/mobile-viewports.spec.ts:108-112
**Bottom Nav Can Be Missed**
The collision check only runs after it finds a fixed element that is a `NAV`, has `role="navigation"`, or contains a nested `nav`. If the app shell renders the bottom bar as another fixed wrapper shape, this returns no bottom nav and silently skips the FAB-over-bottom-nav regression this suite is meant to catch.
Reviews (1): Last reviewed commit: "chore(ci): run public mobile viewport ch..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - name: Run mobile viewport regression (blocking) | ||
| if: steps.baseball-auth-ready.outputs.ready == 'true' | ||
| run: | | ||
| npx playwright test \ | ||
| --project=mobile-public \ | ||
| --project=mobile-coach \ | ||
| --project=mobile-player |
There was a problem hiding this comment.
This mobile step runs as a second standalone Playwright process while PLAYWRIGHT_BASE_URL is unset, so the configured web server starts again with reuseExistingServer disabled in CI. If the first smoke run's Next dev server has not released port 3000 yet, this step can fail with a startup timeout or port-in-use error even though the mobile checks are fine.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/playwright.yml
Line: 171-177
Comment:
**Dev Server Restarts Between Runs**
This mobile step runs as a second standalone Playwright process while `PLAYWRIGHT_BASE_URL` is unset, so the configured web server starts again with `reuseExistingServer` disabled in CI. If the first smoke run's Next dev server has not released port 3000 yet, this step can fail with a startup timeout or port-in-use error even though the mobile checks are fine.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/helm/github/njrini99-code/helmv3/-/custom-context?memory=1f3dcb05-d969-4301-a146-119ca0d1f93e))
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed in 32a9e72 — the mobile projects now ride the mandatory-smoke npx playwright test invocation (one process, one webServer lifecycle) instead of a second standalone run. The separate "public only" fallback step was also removed as dead code: the job hard-fails earlier when baseball secrets are missing, so that branch could never execute.
Generated by Claude Code
| const bottomNav = fixedEls.find((el) => { | ||
| const isNav = el.tagName === 'NAV' || el.getAttribute('role') === 'navigation' || el.querySelector('nav') !== null; | ||
| const r = el.getBoundingClientRect(); | ||
| return isNav && r.top > window.innerHeight - 140 && r.width > window.innerWidth * 0.6; | ||
| }); |
There was a problem hiding this comment.
The collision check only runs after it finds a fixed element that is a NAV, has role="navigation", or contains a nested nav. If the app shell renders the bottom bar as another fixed wrapper shape, this returns no bottom nav and silently skips the FAB-over-bottom-nav regression this suite is meant to catch.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/mobile-viewports.spec.ts
Line: 108-112
Comment:
**Bottom Nav Can Be Missed**
The collision check only runs after it finds a fixed element that is a `NAV`, has `role="navigation"`, or contains a nested `nav`. If the app shell renders the bottom bar as another fixed wrapper shape, this returns no bottom nav and silently skips the FAB-over-bottom-nav regression this suite is meant to catch.
**Context Used:** AGENTS.md ([source](https://app.greptile.com/helm/github/njrini99-code/helmv3/-/custom-context?memory=1f3dcb05-d969-4301-a146-119ca0d1f93e))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 32a9e72, two ways: (1) detection now also accepts tab-bar-SHAPED fixed elements — bottom-pinned, near-full-width, 2+ interactive children — so a plain fixed wrapper still qualifies; (2) authenticated coach/player routes pass expectBottomNav: true, which fails loud when no bottom nav is found at phone widths (MOBILE_DOCTRINE requires one there), so the collision check can no longer be silently skipped on the routes it's meant to protect. Public routes keep skip-if-absent since login/landing legitimately have no bottom nav.
Generated by Claude Code
…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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| const containers = Array.from( | ||
| document.querySelectorAll('header, nav, [role="navigation"], [role="toolbar"]'), | ||
| ); | ||
| const seen = new Set<Element>(); | ||
| for (const container of containers) { | ||
| for (const el of Array.from(container.querySelectorAll('a, button, [role="button"]'))) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include plain fixed bottom-nav wrappers in clipping checks.
expectNoBottomNavCollision explicitly supports a bottom-nav-shaped fixed wrapper without <nav> or role="navigation" (e2e/mobile-viewports.spec.ts:112-125), but expectNoClippedControls only scans semantic containers (e2e/mobile-viewports.spec.ts:68-73). A clipped button inside that supported plain wrapper will therefore evade the regression suite.
Extract/reuse the bottom-nav detector and include the detected element when collecting controls.
Also applies to: 112-125
🤖 Prompt for 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.
In `@e2e/mobile-viewports.spec.ts` around lines 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.
| test(`${route} fits ${viewport.width}px (coach)`, async ({ page }) => { | ||
| await expectMobileFit(page, route, { expectBottomNav: true }); | ||
| expect(page.url(), `expected ${route} to render without bouncing to /login`).not.toContain('/login'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert that each requested route actually rendered.
The current check only rejects /login. The guards in src/lib/baseball/server-route-guards.ts:66-166 can redirect an authenticated but unauthorized user to another dashboard route, so this test can measure a different page and still pass. Assert the final pathname equals route, or allow only explicitly documented redirects, immediately after navigation.
Suggested assertion
await expectMobileFit(page, route, { expectBottomNav: true });
-expect(page.url(), `expected ${route} to render without bouncing to /login`).not.toContain('/login');
+expect(new URL(page.url()).pathname, `expected ${route} to render`).toBe(route);Also applies to: 188-191
🤖 Prompt for 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.
In `@e2e/mobile-viewports.spec.ts` around lines 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.
| madgeOutput = execFileSync( | ||
| 'npx', | ||
| ['madge', '--circular', '--json', 'src'], | ||
| { cwd: ROOT, encoding: 'utf-8', maxBuffer: 64 * 1024 * 1024, stdio: ['ignore', 'pipe', 'inherit'] } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Run only the installed Madge binary.
npx madge can fetch a missing package into npm’s cache, weakening reproducibility for this CI guard. Use npm exec --no -- madge or the local binary after npm ci, so a missing dependency fails instead of invoking an unpinned tool. (docs.npmjs.com)
🤖 Prompt for 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.
In `@scripts/check-cycles.mjs` around lines 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.
| madgeOutput = execFileSync( | ||
| 'npx', | ||
| ['madge', '--circular', '--json', 'src'], | ||
| { cwd: ROOT, encoding: 'utf-8', maxBuffer: 64 * 1024 * 1024, stdio: ['ignore', 'pipe', 'inherit'] } | ||
| ); | ||
| } catch (err) { | ||
| // madge exits 1 when cycles exist but still writes valid JSON to stdout. | ||
| madgeOutput = (err.stdout || '').trim(); | ||
| if (!madgeOutput.startsWith('[')) { | ||
| console.error('check-cycles: madge failed and did not produce JSON output.'); | ||
| console.error(err.message); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the script around the claimed lines and the later baseline-update logic.
sed -n '1,180p' scripts/check-cycles.mjs
# Check how Madge is invoked elsewhere, if at all.
rg -n "check-cycles|madge|warnings\\(\\)\\.skipped|--update" -S .Repository: njrini99-code/helmv3
Length of output: 9732
🌐 Web query:
Madge v8 README warnings().skipped JSON CLI --json skipped files unresolved dependencies
💡 Result:
In Madge v8, the handling of skipped files and unresolved dependencies follows established patterns for identifying and debugging missing information [1][2]. When Madge skips files or cannot resolve dependencies, it does not include them in the main output object by default. To investigate these issues, you can use the following methods: 1. Use the --warning CLI option: Running Madge with the --warning flag will list files that have been skipped due to errors or inability to be resolved [1][2]. This is the primary way to gain insight into missing dependencies via the command line. 2. Use the --debug CLI option: If more granular information is required, the --debug option provides verbose output that can help identify specific lookup or parsing errors [1][3]. 3. Programmatic access via.warnings: If you are using Madge as a library, the.warnings method returns an object containing the warnings gathered during the process [1][2]. Regarding JSON output, when using the --json CLI flag, the tool outputs the dependency graph in JSON format. If you need to include or identify skipped files within a script, you should combine the programmatic approach (calling madge and accessing.warnings) with your custom logic to handle the returned object, as the standard --json CLI output primarily focuses on the resolved graph rather than the internal warning log [1][4]. If you find that files are consistently being skipped, it is often due to issues with file extensions (ensure they are included in your configuration), absolute/relative path resolution, or complex dependency configurations (such as those involving Webpack aliases or TSConfig paths) which may require specific configuration adjustments in your.madgerc or function options [3][5].
Citations:
- 1: https://github.com/pahen/madge/blob/master/README.md
- 2: https://cdn.jsdelivr.net/npm/madge@8.0.0/README.md
- 3: 90 files skipped pahen/madge#132
- 4: https://registry.npmjs.org/madge
- 5: Madge 8.0.0 does not find any file pahen/madge#434
🌐 Web query:
site:github.com/pahen/madge warnings().skipped --json CLI skipped files madge v8 README
💡 Result:
In Madge v8, files that cannot be resolved or processed by the library are marked as skipped [1][2]. When using the Madge CLI, skipped files are reported in the output as warnings [3][4]. To view the list of skipped files, you must use the --warning flag [1][2]. If you require more detailed information about why specific files are being skipped, you can run the CLI with the --debug flag [1][5]. Regarding the warnings().skipped terminology: 1. The .warnings() method is a part of the Madge programmatic API, which returns an object containing information about warnings, including skipped files [1][2]. 2. In the CLI, the functionality previously controlled by the --show-skipped flag was renamed to --warning in v8.0.0 [3][4]. 3. By default, Madge v8 does not show warnings for skipped files; you must explicitly enable them using --warning, or disable them using --no-warning [3][4]. Common causes for files being skipped include missing dependencies, incorrect file extensions (if not specified in the configuration), or issues with resolution paths (such as tsconfig.json baseUrl settings) [1][6][7][8].
Citations:
- 1: http://github.com/pahen/madge
- 2: https://www.github.com/pahen/madge
- 3: https://github.com/pahen/madge/blob/HEAD/CHANGELOG.md
- 4: https://github.com/pahen/madge/blob/master/CHANGELOG.md
- 5: https://github.com/pahen/madge/blob/master/README.md
- 6: Typescript baseUrl "./" not recognized pahen/madge#317
- 7: 90 files skipped pahen/madge#132
- 8: Non JS index files are skipped pahen/madge#128
Fail closed on skipped Madge files
In scripts/check-cycles.mjs:40-52, madge --json can omit skipped/unresolved modules from current, so a cycle that depends on one of those files can disappear and be treated as resolved at scripts/check-cycles.mjs:108-110. Add a skipped-file check here and fail the script if any files were skipped; use Madge’s warnings data or a separate --warning pass.
🤖 Prompt for 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.
In `@scripts/check-cycles.mjs` around lines 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.
| if (UPDATE) { | ||
| writeFileSync(BASELINE_PATH, JSON.stringify(current, null, 2) + '\n', 'utf-8'); | ||
| console.log( | ||
| `check-cycles: baseline updated — ${current.length} runtime cycle${current.length !== 1 ? 's' : ''} locked in ${BASELINE_PATH}` | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the baseline update before mutating the file.
writeFileSync runs before the first update message. Emit an “updating baseline” message before Line 85, then retain the success message afterward.
As per path instructions, scripts/**/*.{ts,mjs,js,sh} scripts must be idempotent and “Log intent before mutation.”
🤖 Prompt for 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.
In `@scripts/check-cycles.mjs` around lines 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.
Source: Path instructions
2 participants
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