test(e2e): authenticated Baseball route crawler, replace broken script (#373) - #850
Conversation
|
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. 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Greptile SummaryReplaces the broken
Confidence Score: 3/5The new spec is well-designed but won't actually run in CI as-is — The core implementation logic (DOM discovery, health checks, anonymous re-verification) is correct and the deleted code is genuinely dead. However, the
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI: baseball-auth-ready?] -->|yes| B[Run route crawler step\n--project=baseball-coach\n--project=baseball-player\nbaseball-route-crawler.spec.ts]
B -->|testMatch mismatch\nonly baseball-smoke matches| C[0 tests collected\nfor role projects]
A -->|later in job| D[Run Playwright tests\n--project=chromium]
D -->|testIgnore does NOT\nexclude crawler| E[baseball-route-crawler.spec.ts\npicked up unauthenticated]
E --> F[Entry route redirect to login\ndiscoverVisibleNavLinks 0 links\nresults empty array]
F --> G[expect 0 toBe 0\nsilent false-pass]
subgraph INTENDED[Intended flow after config fix]
H[baseball-coach project\ntestMatch updated] --> I[crawlAuthenticatedRole\nentry command-center]
I --> J[discoverVisibleNavLinks\nfrontier visible nav hrefs]
J --> K{For each route}
K --> L[gotoAndAssessRouteHealth]
L -->|ok| M[discover nested subnav links\nexpand frontier]
L -->|fail| N[record failure reason]
M --> K
K -->|done| O[verifyPublicSamplesAnonymously]
O --> P[writeReport coach-report.json]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[CI: baseball-auth-ready?] -->|yes| B[Run route crawler step\n--project=baseball-coach\n--project=baseball-player\nbaseball-route-crawler.spec.ts]
B -->|testMatch mismatch\nonly baseball-smoke matches| C[0 tests collected\nfor role projects]
A -->|later in job| D[Run Playwright tests\n--project=chromium]
D -->|testIgnore does NOT\nexclude crawler| E[baseball-route-crawler.spec.ts\npicked up unauthenticated]
E --> F[Entry route redirect to login\ndiscoverVisibleNavLinks 0 links\nresults empty array]
F --> G[expect 0 toBe 0\nsilent false-pass]
subgraph INTENDED[Intended flow after config fix]
H[baseball-coach project\ntestMatch updated] --> I[crawlAuthenticatedRole\nentry command-center]
I --> J[discoverVisibleNavLinks\nfrontier visible nav hrefs]
J --> K{For each route}
K --> L[gotoAndAssessRouteHealth]
L -->|ok| M[discover nested subnav links\nexpand frontier]
L -->|fail| N[record failure reason]
M --> K
K -->|done| O[verifyPublicSamplesAnonymously]
O --> P[writeReport coach-report.json]
end
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
e2e/baseball-route-crawler.spec.ts:120-135
**`frontier.includes(nested)` makes the crawl loop quadratic**
Each time a hub page is visited and new subnav links are revealed, `!frontier.includes(nested)` performs a linear scan of the entire frontier array. As the frontier grows (e.g., 12 dashboard tabs × their subnav items), this scan runs for every nested link on every page, making the inner work O(frontier_size) per discovered link. A `Set<string>` tracking enqueued routes (alongside `visited`) would make this O(1) per check and is idiomatic here.
### Issue 2 of 3
e2e/helpers/route-health.ts:95-97
**HTTP 3xx redirects can be silently swallowed before status check**
`page.goto` with `waitUntil: 'domcontentloaded'` follows redirects automatically — by the time `response` is captured, it reflects the final settled URL's response, not the intermediate redirect. A `302 → /login` yields a final 200 on the login page body, so `status >= 400` never fires. The `guard-bounce` check on `finalUrl.includes('/login')` then catches this correctly, but the `http-error` branch misleadingly reports `status: 200` for what was actually a guard redirect. This is a diagnostic / report-accuracy concern rather than a test-coverage gap (the guard-bounce check fires), but it means the `status` field in the emitted JSON report is unreliable for redirect chains. Adding a note in the JSDoc that `status` reflects the terminal response and guard detection is handled separately would prevent confusion when reading the artifact.
### Issue 3 of 3
e2e/helpers/route-health.ts:22
**`NEAR_BLANK_TEXT_THRESHOLD` of 24 chars may be too low to catch thin but non-empty route renders**
`page.locator('body').innerText()` returns all visible text — including the sidebar nav labels (`Command Center`, `Practice`, `Stats Center`, …) which are mounted on every authenticated page. Even a route that rendered only the nav shell and zero content body would score well above 24 characters through nav text alone, so the near-blank check would never fire for a page where the main content area silently failed to render while the nav remained healthy. The threshold is arguably correct for a pure-no-content case (absolute navigation failure), but it won't catch partial content failures (nav present, content slot empty). This is a known heuristic limitation, but worth a comment so future maintainers understand why the threshold is set where it is and don't lower it further thinking it's being conservative.
Reviews (1): Last reviewed commit: "test(e2e): authenticated Baseball route ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (let i = 0; i < frontier.length; i++) { | ||
| const route = frontier[i]; | ||
| if (!route || visited.has(route)) continue; | ||
| visited.add(route); | ||
|
|
||
| const result = await gotoAndAssessRouteHealth(page, route, { | ||
| expectedSportPrefix: '/baseball/', | ||
| }); | ||
| results.push(result); | ||
|
|
||
| if (result.ok) { | ||
| for (const nested of await discoverVisibleNavLinks(page)) { | ||
| if (!visited.has(nested) && !frontier.includes(nested)) frontier.push(nested); | ||
| } | ||
| for (const link of await discoverPublicSampleLinks(page)) publicSamples.add(link); | ||
| } |
There was a problem hiding this comment.
frontier.includes(nested) makes the crawl loop quadratic
Each time a hub page is visited and new subnav links are revealed, !frontier.includes(nested) performs a linear scan of the entire frontier array. As the frontier grows (e.g., 12 dashboard tabs × their subnav items), this scan runs for every nested link on every page, making the inner work O(frontier_size) per discovered link. A Set<string> tracking enqueued routes (alongside visited) would make this O(1) per check and is idiomatic here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/baseball-route-crawler.spec.ts
Line: 120-135
Comment:
**`frontier.includes(nested)` makes the crawl loop quadratic**
Each time a hub page is visited and new subnav links are revealed, `!frontier.includes(nested)` performs a linear scan of the entire frontier array. As the frontier grows (e.g., 12 dashboard tabs × their subnav items), this scan runs for every nested link on every page, making the inner work O(frontier_size) per discovered link. A `Set<string>` tracking enqueued routes (alongside `visited`) would make this O(1) per check and is idiomatic here.
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!
| if (status !== null && status >= 400) { | ||
| return { route, finalUrl, status, ok: false, failureReason: 'http-error', detail: `HTTP ${status}` }; | ||
| } |
There was a problem hiding this comment.
page.goto with waitUntil: 'domcontentloaded' follows redirects automatically — by the time response is captured, it reflects the final settled URL's response, not the intermediate redirect. A 302 → /login yields a final 200 on the login page body, so status >= 400 never fires. The guard-bounce check on finalUrl.includes('/login') then catches this correctly, but the http-error branch misleadingly reports status: 200 for what was actually a guard redirect. This is a diagnostic / report-accuracy concern rather than a test-coverage gap (the guard-bounce check fires), but it means the status field in the emitted JSON report is unreliable for redirect chains. Adding a note in the JSDoc that status reflects the terminal response and guard detection is handled separately would prevent confusion when reading the artifact.
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/helpers/route-health.ts
Line: 95-97
Comment:
**HTTP 3xx redirects can be silently swallowed before status check**
`page.goto` with `waitUntil: 'domcontentloaded'` follows redirects automatically — by the time `response` is captured, it reflects the final settled URL's response, not the intermediate redirect. A `302 → /login` yields a final 200 on the login page body, so `status >= 400` never fires. The `guard-bounce` check on `finalUrl.includes('/login')` then catches this correctly, but the `http-error` branch misleadingly reports `status: 200` for what was actually a guard redirect. This is a diagnostic / report-accuracy concern rather than a test-coverage gap (the guard-bounce check fires), but it means the `status` field in the emitted JSON report is unreliable for redirect chains. Adding a note in the JSDoc that `status` reflects the terminal response and guard detection is handled separately would prevent confusion when reading the artifact.
How can I resolve this? If you propose a fix, please make it concise.| * "near-blank" — rendered, but with no meaningful content, as opposed to a | ||
| * real empty state (which still renders a heading + explanatory copy). | ||
| */ | ||
| const NEAR_BLANK_TEXT_THRESHOLD = 24; |
There was a problem hiding this comment.
NEAR_BLANK_TEXT_THRESHOLD of 24 chars may be too low to catch thin but non-empty route renders
page.locator('body').innerText() returns all visible text — including the sidebar nav labels (Command Center, Practice, Stats Center, …) which are mounted on every authenticated page. Even a route that rendered only the nav shell and zero content body would score well above 24 characters through nav text alone, so the near-blank check would never fire for a page where the main content area silently failed to render while the nav remained healthy. The threshold is arguably correct for a pure-no-content case (absolute navigation failure), but it won't catch partial content failures (nav present, content slot empty). This is a known heuristic limitation, but worth a comment so future maintainers understand why the threshold is set where it is and don't lower it further thinking it's being conservative.
Prompt To Fix With AI
This is a comment left during a code review.
Path: e2e/helpers/route-health.ts
Line: 22
Comment:
**`NEAR_BLANK_TEXT_THRESHOLD` of 24 chars may be too low to catch thin but non-empty route renders**
`page.locator('body').innerText()` returns all visible text — including the sidebar nav labels (`Command Center`, `Practice`, `Stats Center`, …) which are mounted on every authenticated page. Even a route that rendered only the nav shell and zero content body would score well above 24 characters through nav text alone, so the near-blank check would never fire for a page where the main content area silently failed to render while the nav remained healthy. The threshold is arguably correct for a pure-no-content case (absolute navigation failure), but it won't catch partial content failures (nav present, content slot empty). This is a known heuristic limitation, but worth a comment so future maintainers understand why the threshold is set where it is and don't lower it further thinking it's being conservative.
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!
#373) scripts/route-crawler-baseball.mjs POSTed credentials to a /api/auth/login REST endpoint that never existed in this repo (BaseballHelm auth is a client-side Supabase form, not a JSON login API), so its sign-in always failed and it exited 0 ("no credentials — skipping") regardless of whether CI secrets were configured — and it was never wired into any workflow. Its "contract test" only regex-checked the source text, so it could never catch this. Replaces both with e2e/baseball-route-crawler.spec.ts, which runs under the existing baseball-coach/baseball-player Playwright projects (reusing the storageState auth baseball-smoke.spec.ts already established — no new login code) and discovers routes from the live rendered DOM (visible <nav> links, both the main sidebar and any hub-subnav strip) instead of statically parsing nav-registry.ts source. e2e/helpers/route-health.ts (shared with baseball-smoke.spec.ts, which now imports its ERROR_BOUNDARY_TEXT_RE instead of duplicating the regex) asserts each discovered route isn't a 4xx/5xx, doesn't bounce to /login, doesn't redirect into /golf/, doesn't render an error boundary, doesn't get stuck on a loading spinner, and isn't near-blank. Best-effort discovers public player/team/program/packet links surfaced on an authenticated page (capped at 3, only real linked routes, never guessed IDs) and re-verifies each in a fresh unauthenticated context. Wired into playwright.yml's e2e job as its own step + artifact upload. Deliberately NOT folded into #372's new required ci.yml gate: DOM-driven discovery and the stuck-spinner/near-blank heuristics are new, unproven surface area, so it runs advisory-only until it's demonstrated stable across several main-branch runs. 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>
Review flagged three CI-wiring gaps that would make the new advisory
crawler step deterministically fail on every main-branch run:
- playwright.config.ts: baseball-coach/baseball-player projects only
matched baseball-smoke.spec.ts, so the crawler step's --project
filters resolved zero tests ("No tests found").
- playwright.config.ts: the bare chromium project's testIgnore didn't
exclude the new spec, so it would also run unauthenticated there.
- .github/workflows/playwright.yml: the crawler step lacked
continue-on-error, so any future genuine failure/flake would cascade
into skipping the subsequent required "Run Playwright tests" step,
contradicting the step's own "advisory, doesn't block" comment.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
10e320c to
2faab4f
Compare
|
Rebased onto
Gate evidence:
Co-Authored-By: Claude Fable 5 noreply@anthropic.com |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
#373 — route crawler auth extension
Prior state: the file the issue's own evidence section cited (
e2e/route-crawler/route-crawler.spec.ts) never existed. The actual artifact wasscripts/route-crawler-baseball.mjs, a standalone Node script that POSTed JSON credentials to${BASE_URL}/api/auth/login— an endpoint that does not exist anywhere in this repo (onlysrc/app/api/golf/auth/login/route.tsexists, and it's Golf-specific; BaseballHelm auth is a client-side Supabase form flow, not a JSON login API). SosignIn()always failed, andmain()exited 0 ("No baseball coach credentials — skipping authenticated crawl") regardless of whetherE2E_BASEBALL_COACH_EMAIL/PASSWORDwere actually configured in CI. Its "contract test" (scripts/__tests__/route-crawler-baseball.test.mjs) only regex-checked the source text for strings likeannouncements— it could never have caught any of this. Neither file was wired into any GitHub Actions workflow.What this PR does: deletes both and replaces them with a real Playwright-based crawler.
e2e/baseball-route-crawler.spec.ts(new)Runs under the existing
baseball-coach/baseball-playerPlaywright projects (seeplaywright.config.ts) — same persisted storageState authbaseball-smoke.spec.tsalready established viaplaywright/baseball-auth.setup.ts. No new login code.<nav> a[href]links (covers both the main sidebar —FairwaySidebar,aria-label="Main navigation"— and any hub-subnav strip, e.g.src/app/baseball/(dashboard)/_components/hub-sub-nav.tsx, since both are real Next<Link>s inside a<nav>). This catches a link that's registered innav-registry.tsbut silently fails to render — something a static source-parse (the old script's approach) structurally cannot catch./baseball/(player|team|program)/[id]or/baseball/packet/[token]links surfaced anywhere on an authenticated page (not just<nav>— these are typically "share"/"view public profile" affordances), caps at 3, and re-verifies each in a fresh, unauthenticated browser context to confirm it renders without requiring login. Only real linked routes are probed — never guessed IDs — so this stays safe against seed-data drift and is naturally a no-op if nothing is linked.test-results/baseball-route-crawler-{coach,player}-report.json.e2e/helpers/route-health.ts(new, shared withbaseball-smoke.spec.ts)One
gotoAndAssessRouteHealth()covering every failure mode the AC calls for: HTTP 4xx/5xx, guard-bounce (redirect to/login), wrong-sport redirect (leaving/baseball/, most notably into/golf/), a rendered React/Next error boundary, a stuck loading spinner (checked after an explicit settle), and near-blank page (very little visible body text after settling).baseball-smoke.spec.tsnow importsERROR_BOUNDARY_TEXT_REfrom here instead of duplicating the regex inline — small refactor, no behavior change, single source of truth going forward.CI wiring
Added as its own step inside
playwright.yml'se2ejob (Run BaseballHelm authenticated route crawler (advisory)), gated on the samebaseball-auth-readyreadiness check, plus anUpload route crawler reportartifact step (if: always(), tolerant of no files).Deliberately NOT folded into #372's new required
ci.ymlgate. DOM-driven visible-link discovery and the stuck-spinner/near-blank heuristics are genuinely new, unproven surface area in this codebase — no existing helper to lean on, and inherently more flake-prone than a fixed route list or a plain HTTP status check. Bundling it into an already-required gate before it's demonstrated stable would compound #372's own flake risk (noted explicitly in the original ticket research). It runs as its own isolated step in the already-advisorye2ejob for now, with its own artifact — a failure here is visible and diagnosable without blocking any merge. Promote to a hard gate as a deliberate follow-up once it's proven clean across severalmainruns.Docs
e2e/README.mdgets a new "BaseballHelm authenticated route crawler (#373)" section mirroring the existing mandatory-smoke section's style/detail level.Deferred / out of scope for this PR
docs/operations/generated/route-coverage-report.json/scripts/baseball/generate-route-coverage-report.ts— these are normally regenerated against a live running app; I can't run a dev server or drive a browser locally (no browser automation on this box), so I didn't touch the generated report. Worth a follow-up once this crawler has run in CI.Gate evidence
actionlint .github/workflows/playwright.yml— clean.npm run typecheck(fulltsc --noEmit) — clean (caught and fixed two realnoUncheckedIndexedAccessissues in the new spec during review — an unguarded.split('#')[0].split('?')[0]chain and an unguarded indexed-array read in the frontier loop; both fixed before this evidence run).npx eslint --max-warnings 0on all touched TS files — clean.vitest.config.tsthat the deletedscripts/__tests__/route-crawler-baseball.test.mjswas never wired into vitest, any npm script, or any CI workflow (grepped.github/workflows/*.yml+package.json) — deletion has zero blast radius.vitest.config.ts'sunitproject globs).Do not merge — for review. Independent of #849 (#372 + masking fix): touches a different job's steps in the same
playwright.ymlfile (this PR's new step sits between the existing mandatory-smoke step and the "Run Playwright tests" step that #849 edits), so either can merge first without conflict.Co-Authored-By: Claude Fable 5 noreply@anthropic.com