test(web): deterministic seek-acceptance and auth-geometry browser tests - #79
Merged
Merged
Conversation
…'s clock The creator learns its seek was accepted only on LobbyController's 10 s setInterval refresh, but the test gave it a 10 s waitForURL after the accept, so under four-worker load the poll phase plus navigation lost the race. Install Playwright's clock before navigation and fast-forward exactly one refresh interval: on the acceptor to show the seek, and on the creator only after the acceptor's game URL commits. Both must reach the same game. The product's 10 s interval is unchanged.
At tablet width the lobby sits above the sign-in card, and when /v1/seeks answers its empty state pushes the card 163px down shortly after #auth is visible. Separate boundingBox() round trips straddled that shift and read Register and the passkey button a row apart. Read all related boxes in one page.evaluate so they come from the same layout; assertions are unchanged.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates two browser tests. The auth responsive test batches layout measurements. The seek acceptance test uses Playwright clocks to control lobby refresh timing. The project state records the test changes and validation results. ChangesBrowser test reliability
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes two browser tests more deterministic and records the work in project documentation. It does not alter product behavior, and no actionable merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
PR Summary by QodoStabilize seek-acceptance and auth-layout browser tests
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
 |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can route each severity your way: inline, summary, both, or drop |
sayed710
added a commit
that referenced
this pull request
Sep 30, 2026
sayed710
added a commit
that referenced
this pull request
Sep 30, 2026
…ls (P0) (#76) * feat: durable, exactly-once ratings with explicit variant x speed pools (P0) Ratings were keyed by (user, variant) and never written at runtime. Rated results now update both players exactly once in their variant x speed pool, applied from the committed event log. - Migration 0044: speed joins the ratings key with finite-value checks, a leaderboard index, the rating checkpoint, the rating_applications ledger (primary key game_id) and rating_blocked_games; it fails closed with operator instructions if legacy variant-only rows exist. 0045 builds the ordered GameEnded index online. - PgRatingsApplier walks GameEnded rows in (xact_id, server_ts, game_id) order below pg_snapshot_xmin (the ADR-0147 committed prefix), so live processing and replay apply one order. Per game: both pool rows are locked in player-id order, both new ratings come from the pre-game rows, and ledger, updates and checkpoint commit together. - Eligibility is folded from the stream only; bot, casual, aborted and every no-show ending are never rated; unprovable streams are blocked. - Consumers: GET /v1/leaderboard/:variant/:speed (no default speed), profiles list every pool, seek ranges read the seek's own pool, and the web leaderboard asks for a time control before loading. ADR-0150 records the owner decisions and design. * fix: block impossible endings and malformed creations; update load scenario Review findings on PR #76: - Qodo: decideRating now takes the committed stream and blocks any ending the authority could not have written: a result that termination cannot have (e.g. a decisive stalemate), a winner contradicting the result, a non-boolean rated flag, or an unknown time-control kind. - Qodo: the k6 API baseline and load README use the pooled leaderboard route; FEATURE_PARITY_AUDIT describes it. - Greptile: test that endings sharing one transaction id (the migration 0040 upgrade case) apply in ending-time order across pages. * fix: harden durable ratings decisions and operations * test(web): choose a rating pool in the RTL leaderboard fixture PR #76 loads no leaderboard until a time control is chosen, so #78's RTL containment fixture waited for rows that never mounted. The fixture now selects blitz after navigation and serves only /v1/leaderboard/standard/blitz with speed-bearing entries, so rows prove the real pool load. Falsified: removing the selection, or choosing bullet, fails all four leaderboard viewports; the fixed spec passes 4/4 and the RTL suite 36/36. * fix(persistence): guard rating_ineligible_games only after the 0046 backfill The decision-exclusion trigger takes one advisory lock per inserted row. With it installed before the pre_upgrade backfill, a database with many historical unrated endings could exhaust max_locks_per_transaction during the upgrade. 0046 is unpublished, so it now backfills first and installs the trigger after. The backfill needs no guard: its NOT EXISTS clauses exclude every other decision and the held checkpoint row lock keeps appliers out. The new migration test backfills 40 earlier skips inside a rolled-back run of 0046 and requires zero advisory locks (the old order held 40), then migrates for real and requires every skip frozen, the pending ending untouched, and the guard installed. Removing the trigger fails it. * fix(observability): alert on failing ratings batches and give them a runbook A ratings batch that fails on anything other than stream data rolls back without advancing the single checkpoint and retries with backoff, so a repeating failure stops every pool. Only the backlog-age alert noticed, and no runbook said what to do. GambitRatingsBatchFailing fires when ratings_batch_failures_total keeps increasing for ten minutes. The new "Ratings batch failures" runbook section separates transient database errors from a repeating one, gives a read-only query for the ending being retried, forbids hand edits and forced decisions, and escalates to the owner. No skip or mark-ineligible command is added. scripts/test/alert-runbooks.test.mjs requires every alert's runbook link to resolve to a RUNBOOKS.md heading and the failure counter to have an alert. Renaming the heading or repointing the expression fails it; promtool check passes, and a scratch promtool unit test confirmed a failure every 30 s fires by 16 min while a single failure never fires. * docs: record the PR #76 integration fixes and validation in Increment 77 * docs: record the validation of the PR #79 integration in Increment 78 * fix(persistence): keep ratings retries retryable and blocks deterministic Four review findings on the ratings applier (Qodo 1 and 3, CodeRabbit 1 and 2), each with a regression test that fails on the old code: - Judge a checkpoint rewind by a horizon read after the checkpoint lock. A checkpoint another applier committed while this one waited looked like a logical restore and replayed history from the origin. - Report a block found from before as already_blocked, so a replay is not counted, listed or logged as a new block. - Load the stream outside the block decision and block only on the new CorruptGameStreamError. An unreadable event version, or any loader, database or runtime failure, now aborts the batch for retry instead of permanently blocking a valid game. - Block a game whose variant is not in the variants catalog before any rating write, instead of failing ratings_variant_fkey on every retry. * fix(observability): alert when a rating game is newly blocked GambitRatingsGameBlocked fires on increase(ratings_games_total {outcome="blocked"}[15m]), so a new block fires it and an old counter value or a restart reset does not. A promtool rule test proves both and now runs in CI; the runbook guard pins the metric, label and anchor. * docs: record the ratings review fixes in ADR-0150, the runbook and Increment 78 * fix(persistence): block a non-string variant instead of coercing it requireCatalogVariant looked up String(game.variant), so ['standard'] passed while the array itself reached ratings.variant, failed ratings_variant_fkey outside the block path and stalled every pool. A non-string variant is now a CorruptGameStreamError, and the lookup uses the string itself. * fix(observability): alert on a block recorded before a gateway's first scrape increase() needs two samples, so a block written during a gateway's startup backfill, before Prometheus first scraped it, never fired GambitRatingsGameBlocked. A second term fires for a blocked series first seen non-zero within 15 minutes; promtool cases cover it, a new series at zero, a reset of an existing series and a flat counter. The drift guard now strips offset durations as it does range selectors. * docs: record the exact-head ratings fixes in ADR-0150, the runbook and Increment 78
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two backend Playwright failures reproduced on untouched
main(edacc07) under four-worker load. Both are test defects, not product regressions. This PR changes only the two specs and appends PROJECT_STATE Increment 77. No production code changes.1.
seek-acceptance.spec.ts: deterministic lobby refreshCause: the creator learns its seek was accepted only on
LobbyController's 10 ssetIntervalrefresh (lobby-controller.ts:118-122,166). The acceptor is routed by the accept response (:279). The oldpage1.waitForURL(/\/game\/.+/, { timeout: 10_000 })gave the creator one poll interval to poll and navigate.Fix: install Playwright's clock (1.61.1) before navigation, then fast-forward exactly one refresh interval:
/game/:id. The product's 10 s interval is unchanged.Falsification: three disposable variants each first aligned the creator's poll to its worst phase (a tick had just answered "unmatched" before accept).
fastForwardremoved, 5 s probe2.
auth-responsive.spec.ts: atomic geometrylayout-shiftentries show that at 768px#lobbysits above#auth. When/v1/seeksanswers, the seek-list empty state (164px) pushes the whole card down 163px about 100 ms after#authis visible. This happened in 4/5 backend loads and never in static mode, where the request fails.boundingBox()round trips straddled the shift and read them a row apart.boxesOf()reads every related rect in onepage.evaluate, so all of them come from the same layout. It throws on missing or hidden elements, just asboundingBox() === nullwas handled. No assertion was changed or removed.sameRow(register, passkey). The new spec passed 20/20 alongside it..auth-actionsmutation tominmax(20rem, 1fr)made the new spec failsameRow(register, passkey)at 1440, 1024 and 768px. The stylesheet was restored from a hashed backup and rebuilt.Validation
npm run build,npm run lint: pass.test:scripts: 308/308.check:test-topology,check:ci-parity.ci:local --quick: pass. Its Postgres, gateway/Redis, Nginx and POSIX jobs were not run locally.Note
PR #76 has its own unmerged PROJECT_STATE increment. This PR follows current
main, so whichever merges second must renumber.Test plan
Summary by CodeRabbit