feat: Implement Matchmaking Flow and Chess Variant Selection#676
Conversation
…ess variant support
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request introduces a chess variant selection feature to the frontend. It adds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant Selector as ChessVariantSelector
participant Context as MatchmakingContext
participant Page as page.tsx
participant Modal as AiPersonalityModal
User->>Selector: Click variant button
Selector->>Selector: Highlight selected variant
Selector->>Context: onSelect(variantId)
Context->>Context: setChessVariant(variantId)
Context->>Page: Context updates
Context->>Modal: Context updates
Page->>Page: Rerender with variant info<br/>(status panel, queue overlay)
Modal->>Modal: Rerender with variant details<br/>(label, description, avg time)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@Dantama022 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/page.tsx (1)
185-188:⚠️ Potential issue | 🟠 MajorSelected chess variant isn't forwarded to
joinMatchmaking.
chessVariantis destructured from the matchmaking context (line 42), displayed in the status panel/overlay, butjoinMatchmaking(aiPersonality)on line 187 ignores it entirely. Per the PR description ("game parameters are passed to the matchmaking context"), the variant is expected to influence queueing/time controls — otherwise a user picking "Blitz" vs "Standard" ends up in the same queue with the same parameters.The
joinMatchmakinghook currently accepts onlyaiPersonalityand sends{ ai_personality }to the API. To fix:
- Extend the hook signature to accept
chessVariant- Update the API payload to include the variant parameter
- Update the call on line 187 to pass the variant
🛠 Suggested adjustment
const handlePersonalityConfirm = () => { setIsPersonalityModalOpen(false); - joinMatchmaking(aiPersonality); + joinMatchmaking(aiPersonality, chessVariant); };…and extend
useMatchmaking'sjoinMatchmakingsignature and API payload accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/page.tsx` around lines 185 - 188, handlePersonalityConfirm currently calls joinMatchmaking(aiPersonality) but ignores the selected chessVariant from the matchmaking context; update the useMatchmaking hook so joinMatchmaking accepts a second parameter (chessVariant) and include that value in the API payload (e.g., send chess_variant or variant alongside ai_personality), update the joinMatchmaking signature/implementation in useMatchmaking to add this field and adjust any related types, and finally change handlePersonalityConfirm to call joinMatchmaking(aiPersonality, chessVariant).
🧹 Nitpick comments (6)
frontend/app/page.tsx (1)
230-239: Minor: use an en-dash or localized separator for variant/time.
{selectedVariant.label} / {selectedVariant.averageGameTime}reads fine, but an en-dash (–) or "·" renders cleaner between a name and duration and avoids confusion with literal slashes in values like10-15 min. Optional polish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/page.tsx` around lines 230 - 239, Replace the literal slash between the variant label and its average game time with a cleaner separator; update the JSX that renders {selectedVariant.label} / {selectedVariant.averageGameTime} to use an en-dash or a localized separator (e.g., " – " or " · ") by composing the display string from selectedVariant.label and selectedVariant.averageGameTime (or via a small helper if localization is needed) so the UI shows "Variant: {selectedVariant.label} – {selectedVariant.averageGameTime}" instead of using "/".frontend/components/ChessVariantSelector.tsx (1)
41-50: Consider radio-group semantics for single-select variants.Since exactly one variant can be selected at a time, a radiogroup is a closer semantic fit than
aria-pressedtoggle buttons. This would also give keyboard users arrow-key navigation expected for radio groups (assistive tech convention).♻️ Sketch of the change
- <div className="grid gap-3 sm:grid-cols-2"> + <div role="radiogroup" aria-labelledby="match-variant-heading" className="grid gap-3 sm:grid-cols-2"> {CHESS_VARIANTS.map((variant) => { const isSelected = variant.id === selectedVariant; return ( <button key={variant.id} type="button" + role="radio" + aria-checked={isSelected} onClick={() => onSelect(variant.id)} - aria-pressed={isSelected}Note: tests currently assert
aria-pressed, so they would need to be updated in tandem. Happy to leave the current pattern if you prefer to ship as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/ChessVariantSelector.tsx` around lines 41 - 50, The component ChessVariantSelector uses toggle buttons with aria-pressed for single selection; change it to a proper radio-group pattern: render the list as a container with role="radiogroup" and each item as role="radio" (or use native <input type="radio">) instead of aria-pressed, update the selection logic to use aria-checked (or checked for inputs) driven by selectedVariant, and keep using onSelect(variant.id) in the click/keyboard handlers; update tests that assert aria-pressed to assert radio semantics (role="radio" + aria-checked or checked) and ensure arrow-key navigation works for the radiogroup.frontend/app/components/matchmaking/AiPersonalityModal.tsx (1)
112-152: Missing dialog semantics / focus management on this modal.The PR advertises accessibility improvements and you've correctly added
role="dialog"+aria-modal="true"+aria-labelto the searching and reconnecting overlays infrontend/app/page.tsx(lines 252, 282). For consistency, this modal's root (line 113) should have the same, and ideally Escape-to-close plus initial focus moved into the panel (not the backdrop). Scope-wise it touches pre-existing code, so feel free to defer.♿ Suggested minimal additions
- <div className="fixed inset-0 z-50 flex items-center justify-center"> + <div + className="fixed inset-0 z-50 flex items-center justify-center" + role="dialog" + aria-modal="true" + aria-labelledby="match-setup-heading" + > {/* Backdrop */} <div className="fixed inset-0 bg-black/70 backdrop-blur-sm" onClick={onClose} + aria-hidden="true" />And add
id="match-setup-heading"to the<h2>Finalize Match Setup</h2>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/components/matchmaking/AiPersonalityModal.tsx` around lines 112 - 152, In AiPersonalityModal, add proper dialog semantics and basic focus/keyboard handling: give the modal root div (the outer fixed container) role="dialog", aria-modal="true" and an aria-label (e.g., "Match setup dialog"); add id="match-setup-heading" to the <h2>Finalize Match Setup</h2>; move initial focus into the modal panel by making the modal panel element focusable (e.g., tabIndex={-1}) and programmatically focus it on mount (useEffect), and add a keydown handler on mount that closes the modal on Escape (calling onClose) and cleans up on unmount. Ensure the backdrop click behavior remains unchanged and that focus is restored when the modal closes.frontend/vitest.config.ts (1)
4-15: Consider adding@vitejs/plugin-reactfor proper JSX transform alignment.Your
tsconfig.jsonhas"jsx": "preserve", which means Vitest's esbuild will handle JSX transformation without explicit React configuration. This works for most tests, but adding@vitejs/plugin-reactensures JSX transform semantics are fully aligned with Next.js/React 19 conventions, especially if tests render hooks or complex components.Optional addition
import path from "path"; import { defineConfig } from "vitest/config"; +import react from "@vitejs/plugin-react"; export default defineConfig({ + plugins: [react()], test: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/vitest.config.ts` around lines 4 - 15, Update the Vitest config to include the React plugin so JSX transforms match Next.js/React 19 semantics: import and invoke `@vitejs/plugin-react` and add it to the exported defineConfig plugins array (i.e., add the plugin to the config object returned by defineConfig used in frontend/vitest.config.ts) so tests use the same JSX transform as your app; ensure the plugin is installed as a dependency.frontend/package.json (2)
10-10: LGTM on thetestscript.
vitest runis the correct non-watch invocation for CI. Consider also adding companion scripts ("test:watch": "vitest","test:coverage": "vitest run --coverage") to align with common workflows, but not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/package.json` at line 10, The package.json currently defines only the CI test script ("test": "vitest run"); add complementary NPM scripts to support common workflows by adding "test:watch" that runs vitest in watch mode (e.g., "vitest") and "test:coverage" that runs vitest with coverage enabled (e.g., "vitest run --coverage") alongside the existing "test" entry so maintainers can run tests interactively and collect coverage locally.
9-9: Addignoresentry toeslint.config.mjsto exclude build artifacts.The eslint.config.mjs does not define an
ignoresentry. Without it, runningeslint .will traverse.next/,out/,coverage/, and generated files, slowing down CI and potentially surfacing errors from generated code. Add an ignores configuration:const eslintConfig = [ { ignores: ['.next/**', 'out/**', 'coverage/**', 'next-env.d.ts'], }, ...compat.extends("next/core-web-vitals", "next/typescript"), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/package.json` at line 9, Add an "ignores" entry to the eslint.config.mjs configuration so ESLint skips build and generated artifacts; update the top-level eslintConfig (the array exported from eslint.config.mjs) to include an object with ignores: ['.next/**','out/**','coverage/**','next-env.d.ts'] before spreading the compat.extends("next/core-web-vitals","next/typescript") entry so running the "lint" script (eslint .) won't traverse generated files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/app/page.tsx`:
- Around line 185-188: handlePersonalityConfirm currently calls
joinMatchmaking(aiPersonality) but ignores the selected chessVariant from the
matchmaking context; update the useMatchmaking hook so joinMatchmaking accepts a
second parameter (chessVariant) and include that value in the API payload (e.g.,
send chess_variant or variant alongside ai_personality), update the
joinMatchmaking signature/implementation in useMatchmaking to add this field and
adjust any related types, and finally change handlePersonalityConfirm to call
joinMatchmaking(aiPersonality, chessVariant).
---
Nitpick comments:
In `@frontend/app/components/matchmaking/AiPersonalityModal.tsx`:
- Around line 112-152: In AiPersonalityModal, add proper dialog semantics and
basic focus/keyboard handling: give the modal root div (the outer fixed
container) role="dialog", aria-modal="true" and an aria-label (e.g., "Match
setup dialog"); add id="match-setup-heading" to the <h2>Finalize Match
Setup</h2>; move initial focus into the modal panel by making the modal panel
element focusable (e.g., tabIndex={-1}) and programmatically focus it on mount
(useEffect), and add a keydown handler on mount that closes the modal on Escape
(calling onClose) and cleans up on unmount. Ensure the backdrop click behavior
remains unchanged and that focus is restored when the modal closes.
In `@frontend/app/page.tsx`:
- Around line 230-239: Replace the literal slash between the variant label and
its average game time with a cleaner separator; update the JSX that renders
{selectedVariant.label} / {selectedVariant.averageGameTime} to use an en-dash or
a localized separator (e.g., " – " or " · ") by composing the display string
from selectedVariant.label and selectedVariant.averageGameTime (or via a small
helper if localization is needed) so the UI shows "Variant:
{selectedVariant.label} – {selectedVariant.averageGameTime}" instead of using
"/".
In `@frontend/components/ChessVariantSelector.tsx`:
- Around line 41-50: The component ChessVariantSelector uses toggle buttons with
aria-pressed for single selection; change it to a proper radio-group pattern:
render the list as a container with role="radiogroup" and each item as
role="radio" (or use native <input type="radio">) instead of aria-pressed,
update the selection logic to use aria-checked (or checked for inputs) driven by
selectedVariant, and keep using onSelect(variant.id) in the click/keyboard
handlers; update tests that assert aria-pressed to assert radio semantics
(role="radio" + aria-checked or checked) and ensure arrow-key navigation works
for the radiogroup.
In `@frontend/package.json`:
- Line 10: The package.json currently defines only the CI test script ("test":
"vitest run"); add complementary NPM scripts to support common workflows by
adding "test:watch" that runs vitest in watch mode (e.g., "vitest") and
"test:coverage" that runs vitest with coverage enabled (e.g., "vitest run
--coverage") alongside the existing "test" entry so maintainers can run tests
interactively and collect coverage locally.
- Line 9: Add an "ignores" entry to the eslint.config.mjs configuration so
ESLint skips build and generated artifacts; update the top-level eslintConfig
(the array exported from eslint.config.mjs) to include an object with ignores:
['.next/**','out/**','coverage/**','next-env.d.ts'] before spreading the
compat.extends("next/core-web-vitals","next/typescript") entry so running the
"lint" script (eslint .) won't traverse generated files.
In `@frontend/vitest.config.ts`:
- Around line 4-15: Update the Vitest config to include the React plugin so JSX
transforms match Next.js/React 19 semantics: import and invoke
`@vitejs/plugin-react` and add it to the exported defineConfig plugins array
(i.e., add the plugin to the config object returned by defineConfig used in
frontend/vitest.config.ts) so tests use the same JSX transform as your app;
ensure the plugin is installed as a dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 610e4873-1272-4656-a18c-543ee31248c1
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
frontend/app/components/matchmaking/AiPersonalityModal.tsxfrontend/app/page.tsxfrontend/components/ChessVariantSelector.tsxfrontend/components/__tests__/ChessVariantSelector.test.tsxfrontend/context/matchmakingContext.tsxfrontend/lib/chessVariants.tsfrontend/package.jsonfrontend/vitest.config.tsfrontend/vitest.setup.ts
|
please resolve conflicts @Dantama022 |
|
@gabito1451 on it |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/context/walletContext.tsx (1)
120-336: Out-of-scope change for this PR.This wallet context refactor (Freighter typings,
getFreighterhelper,Promise<unknown>typings, Soroban placeholder) doesn't appear in the PR description, which is scoped to the chess variant selector and matchmaking flow (issue#583). Consider splitting it into a dedicated PR so reviewers and the changelog can attribute these wallet/typing improvements correctly, and so a regression here doesn't block the matchmaking feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/context/walletContext.tsx` around lines 120 - 336, The changes to the wallet context (introducing FreighterLike typings, getFreighter helper, signWithFreighter, Promise<unknown> return types, and the invokeSorobanContract placeholder) are unrelated to the chess matchmaking work and should be split out into a dedicated PR; revert or remove these walletContext.tsx changes from this branch and instead create a new branch/PR that contains the refactor (files and symbols to isolate: FreighterLike type, getFreighter(), signWithFreighter(), connectWallet(), invokeSorobanContract(), and related type changes to AppContextType), so the current PR only contains chess variant selector and matchmaking changes and reviewers can review the wallet refactor independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/context/walletContext.tsx`:
- Around line 120-336: The changes to the wallet context (introducing
FreighterLike typings, getFreighter helper, signWithFreighter, Promise<unknown>
return types, and the invokeSorobanContract placeholder) are unrelated to the
chess matchmaking work and should be split out into a dedicated PR; revert or
remove these walletContext.tsx changes from this branch and instead create a new
branch/PR that contains the refactor (files and symbols to isolate:
FreighterLike type, getFreighter(), signWithFreighter(), connectWallet(),
invokeSorobanContract(), and related type changes to AppContextType), so the
current PR only contains chess variant selector and matchmaking changes and
reviewers can review the wallet refactor independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 581d202c-d0a4-4020-9f69-7027f9beebbe
📒 Files selected for processing (5)
frontend/context/walletContext.tsxfrontend/eslint.config.mjsfrontend/hook/useCheatDetection.tsfrontend/tailwind.config.tsfrontend/types/json.d.ts
✅ Files skipped from review due to trivial changes (3)
- frontend/types/json.d.ts
- frontend/eslint.config.mjs
- frontend/tailwind.config.ts
…mprove type definitions" This reverts commit ff851b7.
|
@gabito1451 conflicts resolved |
2 participants
Closes #583
Implement Matchmaking Flow and Chess Variant Selection
This PR introduces a robust matchmaking system and a premium variant selection UI, while establishing a modern testing infrastructure for the frontend.
Key Features
Chess Variant Selection: Added a high-fidelity ChessVariantSelector component allowing users to choose between Standard, Rapid, Blitz, and Bullet formats.
Matchmaking Flow: Integrated the selection flow into the main dashboard, ensuring game parameters are correctly passed to the matchmaking context.
AI Personality Integration: Users are now prompted to select an AI personality before entering the matchmaking queue (via AiPersonalityModal).
Real-time Status: Implemented dynamic status overlays for searching, match found, and reconnection states with smooth animations.
Improved Accessibility: Conducted an accessibility audit and added ARIA roles/labels to core interactive elements.
Technical Improvements
Testing Infrastructure: Integrated Vitest and React Testing Library. Added configuration files (vitest.config.ts, vitest.setup.ts) and implemented unit tests for UI components.
Design System: Leveraged HSL-tailored gradients, glassmorphism, and Lottie animations to create a premium, state-of-the-art gaming interface.
Typed Constants: Centralized variant definitions in lib/chessVariants.ts for consistency across the application.
Verification Results
Unit Tests: Passed ChessVariantSelector.test.tsx ensuring correct state transitions and accessibility attributes.
Manual Flow: Verified the end-to-end flow from variant selection → personality selection → queue entry.
Summary by CodeRabbit