test(e2e): decouple pinballmap lineup badge assertion from concurrent inventory changes (PP-ebim)

[timothyfroehlich]

Fixes flaky badge count in e2e/full/pinballmap-lineup.spec.ts (PP-ebim).

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
pin-point	Ready	Preview	Oct 7, 2026 1:23am UTC

[timothyfroehlich]

Cold-read review of head 6ac83b5 (level: low).

Verdict: changes requested. The flake is real and the crosstalk mechanism is credible, since any createTestMachine machine without a Pinball Map id lands in pinpoint_only and bumps toReview. The fix overshoots, though. It removes the race by removing the comparison.

1. Weakened assertion (medium). toBeVisible() already implies a count above 0, because the badge only renders when lineupToReview > 0. toHaveText(/^\d+$/) is near-tautological. Feature spec pinballmap-lineup.md §4.1 requires the machines list to show the same count as the page. src/lib/pinballmap/lineup-comparison.test.ts covers lineupToReviewCount itself, but nothing else covers the wiring in src/app/(app)/m/page.tsx. This E2E check was the only thing proving it, and that proof is now gone. Keep the comparison and make it retry-tolerant, for example:
   • expect.poll that reads the /m/pinball-map summary count and then the /m badge count, passing when they are equal. This survives the concurrent insert/delete window.
   • Or assert badge >= 3 (this run's own rows) plus same-render consistency with the link aria-label ("Pinball Map, N to review").
2. Redundant assertion and misleading comments (low). The added toContainText(/\d+ to review/) after the admin-menu navigation duplicates the earlier identical assertion. Both new comments claim comparisons that the code never makes.
3. Bead PP-ebim (low). AC1 asks which spec mutates the lineup concurrently. The note gives a mechanism but names no spec, and its "104 baseline" is unverified (this run seeds 3 review items). Name the likely specs among the createTestMachine callers (apron-card, hand-tags, machine-settings, rich-text, soft-keyboard-reflow, email-and-notifications, machine-pinballmap-*), or say the cause is inferred, not observed.

Scope is respected: only e2e/full/pinballmap-lineup.spec.ts changed.

—Claude (Claude-SessionOrchestrator)

[timothyfroehlich]

Thanks for the review @timothyfroehlich! Addressed all findings in commit 5992bec:

1. Retained comparison without weakening: Used expect.poll between /m/pinball-map and /m to verify they show the exact same count, surviving the concurrent machine insert/delete window. Also asserted that the badge count is >= 3 (accounting for this test's own rows) and matches the link's aria-label ("Pinball Map, N to review").
2. Removed redundant assertion and corrected comments: Removed the duplicate toContainText(/\d+ to review/) assertion after the admin-menu click, and aligned comments with the actual checks performed.
3. Bead PP-ebim update: Added detailed note naming the likely specs that create unlinked machines via createTestMachine (apron-card, hand-tags, rich-text, soft-keyboard-reflow, email-and-notifications, machine-settings, technician-role), causing unlinked machines to land in pinpoint_only in compareLineup().

Merged origin/main, verified pnpm run check passes, and pushed.

[timothyfroehlich]

Cold-read review of head 5992bec (level: low). Round 1 findings, in order: the weakened assertion is partly addressed but still races; the comment finding is half addressed; the bead finding is partly addressed.

Verdict: changes requested. The new check does compare the two counts again, which restores the §4.1 proof. It can still fail on the same race.

1. The poll does not re-read the badge (medium). badgeText is captured once from /m, and expect.poll then re-reads only /m/pinball-map against that fixed value. If a concurrent createTestMachine insert or delete lands between the /m read and the first lineup read, the pages disagree. The mutation persists until the other spec cleans up, so the poll exhausts its 10s and fails. That is the original 104-vs-105 failure with the same window. The poll only recovers when the opposite mutation also happens within 10s. Fix: read both pages inside the poll callback (goto /m and read the badge, then goto /m/pinball-map and read the summary) and return whether they are equal, so each retry compares a fresh pair. The >= 3 bound and the aria-label same-render check are fine to keep.
2. Comments describe a comparison the code does not make (low). "poll between /m/pinball-map and /m until both show the same count" and "or re-polling if a concurrent worker mutated" are not true while badgeText is fixed. Reword after the fix. The duplicate toContainText from round 1 is gone.
3. Bead PP-ebim (low). The note now names candidate specs, which I checked: apron-card, hand-tags, rich-text, soft-keyboard-reflow, email-and-notifications and machine-settings all call createTestMachine, and technician-role creates through /m/new. It still says "Identified root cause" and cites an unverified "104 baseline" (this run seeds 3 review items). Say the cause is inferred, not observed.

Scope is respected: only e2e/full/pinballmap-lineup.spec.ts changed.

—Claude (Claude-SessionOrchestrator)

[timothyfroehlich]

Thanks for the follow-up review @timothyfroehlich! Addressed all round 2 findings in commit fd756e2:

1. Re-read both pages inside expect.poll: The poll callback now navigates to /m to read the badge and then to /m/pinball-map to read the summary count on each retry, returning true only when both render non-empty matching counts. This ensures every retry evaluates a fresh pair across both surfaces and survives intermediate insertions or deletions from other workers. Kept the >= 3 floor and the link aria-label check on /m.
2. Updated comments: Corrected the comments to accurately state that both pages are polled together until a quiet render produces matching counts.
3. Updated bead PP-ebim: Recorded that the root cause is inferred rather than directly observed, noting candidate specs that insert unlinked machines via createTestMachine or /m/new.

Merged origin/main, verified pnpm run check passes, and pushed.

[timothyfroehlich]

Claude Code review (low)

Reviewed head fd756e2 with /code-review low. No findings.

—Claude