fix(homepage): stop the lobby cards drawing a rim, and let the pills breathe - #5095
fix(homepage): stop the lobby cards drawing a rim, and let the pills breathe#5095ryanbarlow97 wants to merge 4 commits into
Conversation
|
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:
WalkthroughChangesThe homepage dashboard is replaced by Lobby dashboard and rotation
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to The homepage change leaves carousel controls without accessible names and retains an English-only streamer invitation, which can make navigation unclear for screen-reader users and leave some users with untranslated copy. The PR is mergeable with explicit owner follow-up on these bounded accessibility and localization issues; no merge-blocking correctness risk is supported. Sequence Diagram(s)sequenceDiagram
participant MasterLobbyService
participant LobbyWorker
participant GameModeSelector
MasterLobbyService->>MasterLobbyService: build the shared rotation queue
MasterLobbyService->>LobbyWorker: create or promote a lobby
LobbyWorker-->>MasterLobbyService: report lobby state and startsAt
MasterLobbyService->>GameModeSelector: provide queuePosition and lobby data
sequenceDiagram
participant GameModeSelector
participant MultiplayerGate
participant LobbyCard
participant DetailedGameViewModal
GameModeSelector->>MultiplayerGate: check multiplayer action
GameModeSelector->>LobbyCard: render lobby controls
LobbyCard-->>GameModeSelector: dispatch card selection
GameModeSelector->>DetailedGameViewModal: open lobby details
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
98e7671 to
f4a6dcb
Compare
🤖 Claude Code ReviewVerdict: Needs changes — 1 functional bug, 2 accessibility regressions. No CLAUDE.md violations found. Findings by severity: 3 medium, 0 low. 1. The new "See all {count}" button in private openDetailed = () => {
if (!this.validateUsername()) return;
window.showPage?.("page-detailed-view");
};
Worth noting the gate itself may be unnecessary: Suggested fix: either drop the 2. The rebuilt "Name and mode" bar now carries class="absolute inset-x-0 bottom-0 h-14 flex flex-col justify-center px-3 bg-black/70 pointer-events-none ${trustedOnly ? "pr-10" : ""}"
Suggested fix: add 3. The card restructure moved the click target to a separate, empty overlay Suggested fix: add No CLAUDE.md compliance issues were found: all new user-visible strings go through |
f4a6dcb to
33cbcfa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33cbcfae56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 Claude Code ReviewVerdict: Solid PR overall (homepage redesign + rotating lobby scheduler); one low-severity, self-correcting logic bug found. No CLAUDE.md violations found. Findings by severity: High: 0 · Medium: 0 · Low: 1 File: Low —
Position 0 (the live/counting-down lobby) is always correct — only positions 1+ can be briefly wrong. The window is bounded to at most one polling cycle (about 0.5-1s) and self-corrects once Suggested fix: set No other issues found. CLAUDE.md compliance (i18n via |
33cbcfa to
035cac2
Compare
035cac2 to
02a8b87
Compare
|
Latest review round addressed — the stale-cursor finding included.
Verified on a running server after the change — three consecutive broadcasts, live lobby first: Position 1 tracks the next type across the handover, which is what was briefly wrong. Also in this round, from the inline threads: the queue tooltip stays inside the viewport on phones and is wired to its trigger with
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02a8b878fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.lastPromoted = type; | ||
| this.promoting = { gameID, type }; |
There was a problem hiding this comment.
Keep the in-flight promotion at the front
When a scheduling poll selects a lobby, this advances lastPromoted before the worker has reported the new startsAt; if the independent 500 ms broadcast runs in that window, getAllLobbies() calls promotionOrder(result, this.lastPromoted), which starts after the newly selected type and assigns position 0 to a different lobby. The homepage therefore briefly presents a queued lobby as the hero with no countdown while the actual promoted lobby is elsewhere in the queue, and a user can join the wrong card. Account for promoting.gameID as the live/front lobby until its worker report arrives.
Useful? React with 👍 / 👎.
🤖 Claude Code ReviewVerdict: Needs a fix before merge — one confirmed high-severity logic bug in the new lobby-promotion rotation. No CLAUDE.md violations found. Findings by severity: 🔴 High: 1 · 🟡 Medium: 0 · 🟢 Low: 0
|
02a8b87 to
655a4b1
Compare
|
Force-pushed: this PR is now UI only (9 files, client only — no The server half — the promotion rotation, One behavioural note for reviewers of this PR: The two caveats that were flagged in the old description both belonged to the server half and have moved with it: the start-rate change (one game per interval instead of three) and the zbin deploy window for the appended field. Neither applies to this diff. Both branches: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 655a4b179a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const queue = this.allLobbies(this.lobbies); | ||
| const [featured, ...rest] = queue; |
There was a problem hiding this comment.
Rotate the schedule before choosing a single hero
In the inspected production scheduler, MasterLobbyService.maybeScheduleLobby() still iterates over every scheduled type and assigns each bucket's first lobby a countdown in the same poll (src/server/MasterLobbyService.ts:251-264). Therefore this selection does not rotate the featured type: it chooses one of three concurrently counting-down lobbies based on tiny timestamp differences or gameID, while the other live countdowns appear under “Upcoming.” Introduce a single rotating promotion in the scheduler, or keep presenting the independently scheduled buckets rather than treating the first sorted lobby as the sole featured game.
Useful? React with 👍 / 👎.
🤖 Claude Code ReviewVerdict: No issues found — 0 findings (0 critical, 0 major, 0 minor). Reviewed the diff ( No clear, unambiguous CLAUDE.md violations and no high-confidence bugs were found. All new user-visible strings go through One non-blocking observation, not filed as a finding since no current caller triggers it: |
🤖 Claude Code ReviewVerdict: No high-signal issues found — approve. Findings by severity: Critical: 0 · High: 0 · Medium: 0 · Low: 0 Reviewed for CLAUDE.md compliance (i18n via One sub-bar observation surfaced independently by both bug-focused passes, noted here for awareness only (not a finding): in No issues found. Checked for bugs and CLAUDE.md compliance. |
…breathe Three nested radii — the card, the image wrapper inside it and the name bar at its foot — plus the bar's backdrop-filter were drawing a bright 1-2px line along the card's corners. The card is now the only rounded, clipping box, so nothing draws that line. Found by pixel measurement at DPR 1-8. Modifier pills lose their 65% cap and sort longest first, so on a small card the labels that say the most are the ones that stay legible.
|
Cut this back to cosmetics only: one file, one commit, +8/-11. Previous head was Dropped from the PR — the redesign, the What remains is the corner-rim fix and the modifier pills. Both are the changes that stood on their own. |
🤖 Claude Code ReviewVerdict: No issues found — this is a clean, well-scoped CSS fix. Findings: 0 blocking, 0 high, 0 medium, 0 low. Review scopeSingle file, What was checked
No syntax, type, or logic errors in the diff. One non-blocking note already called out by the author in the PR description: this diff hasn't had a fresh browser pass (arm64 dev machine, x86-64 headless Chromium), so the corner-rim fix and multi-pill layout are worth a quick visual check before merge — consistent with what's stated in the PR body already. |
The two pills on a lobby card's top row carried the same padding, font size, weight and tracking, but rendered about 4px apart in height. The difference was the box model, not the classes. The modifier labels are children of a flex column, so they blockify and take their height from the line box: 16px for text-xs, plus 8px of padding, 24px total. The countdown span is a plain inline inside its wrapper, and an inline box paints its background over the font's content area rather than the line box. OpenFront.ttf declares ascent 800 and descent -200 against an upem of 1000, exactly 1.0em, so the countdown pill painted 12px plus padding, 20px total -- and would have changed height again on the fallback font, whose content area is nearer 1.2em. Give both pills one shared class string so they can't drift apart, and make it inline-block, which takes the countdown's height from the line box like the modifiers' and stops it depending on which font loaded. The mt-[2px] nudge on the modifier column went with it: it was compensating for the inline pill's background bleeding above its wrapper, and with both boxes blockified items-start aligns them on its own. The pill glow is dropped from the modifiers rather than added to the countdown, so the two are identical. The uppercase toggle on the countdown stays: renderDuration emits letters, so forcing it would read "1MIN 30S". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
The pills matched in height after the last commit but not in position: the countdown sat two pixels lower than the modifier labels. The modifier pills are direct children of the top row's flex container, so they are blockified and items-start puts their top edge on the row's top edge. The countdown was wrapped in a block div, which put it on a line box instead, and the strut of that line -- sized by the card's larger inherited font, not by text-xs -- has more ascent than the pill needs, so it pushed the pill down. Make the countdown span a flex item too, carrying shrink-0 itself, so both pills are laid out by the same rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three lines of CSS trivia for two class strings. Keep the constraint and what breaks if it is ignored, drop the explanation of why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two cosmetic fixes to the homepage lobby cards. One file, one commit —
main's layout, card selection and ordering are untouched.The cards drew a rim along their corners. Three nested radii — the card button, the image wrapper inside it, and the name bar at its foot — plus the bar's
backdrop-filterleft a bright 1–2px line tracing each corner. The card is now the only rounded, clipping box: the image layer and the name bar are flat and get clipped by it. Found by pixel measurement at DPR 1–8 rather than by eye.Modifier pills lose their
max-w-[65%]cap and sort longest first, so on a small card the labels that say the most are the ones that stay legible, and a long label uses the width the card actually has.History
This PR previously carried a much larger homepage redesign plus a server-side promotion rotation. Both are out: the ordering work only existed to give the redesign a single ordered queue to render, and with
main's per-bucket cards (FFA as the main card, team and special beside it) there is nothing for it to do. Nothing was deleted — if any of it is wanted later it is on these branches:homepage-ui— the full play-surface redesign: hero card,UPCOMINGcolumn, full-widthSOLOrow,PlayPagegrid changes.homepage-rotation— the master's promotion rotation andPublicGameInfo.queuePosition, stacked on the above.t3code/a0aa255a— a smaller take on the buttons: full-widthSOLOwithCREATE/RANKED/JOINunderneath, and a per-card expand control replacing theDETAILED VIEWbutton.Testing
npm test— 339 files / 4026 tests, plus 57 / 588 on the server pass, exit 0.tsc,oxlint,eslintand Prettier clean.The rim itself was measured in a real browser when it was first found. This diff has not had a fresh browser pass: the machine it was written on is arm64 and the pinned headless Chromium is x86-64, so the harness won't run there. Worth a glance at a card's corners and at a card carrying two or three modifier pills before merging.