Free‑typing Timed Mode (No Forced Mistake Fix), Uncapped Public Lobbies, and Docs/Leaderboard UX Refresh #2
Conversation
…rs (just like monkeytype)
…oard clickable instead of just the pfp
There was a problem hiding this comment.
Pull Request Overview
This PR implements differential player capacity rules between public and private lobbies, along with adding support for timed mode typing behavior similar to MonkeyType, and various UI/UX improvements.
- Public lobbies now have no player cap while private lobbies remain limited to 10 players
- Timed mode now allows free-flow typing past errors (MonkeyType-style) vs snippet mode which requires error correction
- Improved glide cursor behavior, leaderboard clickability, and tutorial documentation
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server/models/race.js | Adds lobby type check to enforce 10-player cap only for private lobbies |
| client/src/tutorial/tutorialSteps.js | Updates tutorial text to explain capacity differences and error handling modes |
| client/src/pages/Lobby.jsx | Passes props to disable timed mode in lobbies |
| client/src/pages/Lobby.css | Tightens spacing and reduces dead space in lobby layout |
| client/src/pages/AboutUs.jsx | Adds documentation for capacity rules, error handling, and feedback contact info |
| client/src/pages/AboutUs.css | Adds styling for feedback note section |
| client/src/context/RaceContext.jsx | Implements timed vs snippet mode typing logic with different error handling |
| client/src/components/Typing.jsx | Updates input handling and rendering for timed mode free-flow typing |
| client/src/components/Typing.css | Refines cursor styling and error highlighting |
| client/src/components/TestConfigurator.jsx | Adds allowTimed prop to conditionally disable timed mode option |
| client/src/components/Leaderboard.jsx | Makes entire player row clickable with proper accessibility attributes |
| client/src/components/Leaderboard.css | Adds clickable/disabled styling for leaderboard player rows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await client.query('ROLLBACK'); | ||
| throw new Error('Lobby not found.'); | ||
| } | ||
| const lobbyType = lobbyTypeRes.rows[0].type; |
There was a problem hiding this comment.
The lobbyType value retrieved from the database is not validated before being compared. If the database contains unexpected values (not 'private' or 'public'), the capacity check at line 330 may silently fail. Consider adding validation: if (!['private', 'public'].includes(lobbyType)) { throw new Error('Invalid lobby type'); }
| const lobbyType = lobbyTypeRes.rows[0].type; | |
| const lobbyType = lobbyTypeRes.rows[0].type; | |
| // Validate lobbyType | |
| if (!['private', 'public'].includes(lobbyType)) { | |
| await client.query('ROLLBACK'); | |
| throw new Error('Invalid lobby type'); | |
| } |
| const currentInput = typingState.input; | ||
| const lockedPosition = typingState.lockedPosition; | ||
| const text = raceState.snippet?.text || ''; | ||
| const isTimedMode = !!(raceState.snippet?.is_timed_test || raceState.timedTest?.enabled || raceState.settings?.testMode === 'timed'); |
There was a problem hiding this comment.
The isTimedMode logic is duplicated across multiple locations in this file (lines 655, 706, etc.). Consider extracting this into a helper function or computing it once at the component level to improve maintainability and reduce the risk of inconsistencies.
client/src/context/RaceContext.jsx
Outdated
| for (let i = 0; i < span; i++) { | ||
| if (input[i] === text[i]) correctChars++; | ||
| } | ||
| currentErrors = input.length - correctChars; // net errors | ||
| hasError = currentErrors > 0; | ||
| firstErrorPosition = hasError ? input.split('').findIndex((ch, idx) => idx < text.length && ch !== text[idx]) : text.length; |
There was a problem hiding this comment.
The input.split('') operation is inefficient for large inputs. Since you're already iterating character-by-character in the loop above (lines 726-728), you can track firstErrorPosition during that same loop instead of performing a separate findIndex operation with another split.
| for (let i = 0; i < span; i++) { | |
| if (input[i] === text[i]) correctChars++; | |
| } | |
| currentErrors = input.length - correctChars; // net errors | |
| hasError = currentErrors > 0; | |
| firstErrorPosition = hasError ? input.split('').findIndex((ch, idx) => idx < text.length && ch !== text[idx]) : text.length; | |
| firstErrorPosition = text.length; | |
| for (let i = 0; i < span; i++) { | |
| if (input[i] === text[i]) { | |
| correctChars++; | |
| } else if (firstErrorPosition === text.length) { | |
| firstErrorPosition = i; | |
| } | |
| } | |
| currentErrors = input.length - correctChars; // net errors | |
| hasError = currentErrors > 0; | |
| if (!hasError) { | |
| firstErrorPosition = text.length; | |
| } |
| raceHandleInput(newValue); | ||
| // Reflect raw change immediately; locking will reconcile on next state tick | ||
| setInput(newValue); |
There was a problem hiding this comment.
The comment states 'locking will reconcile on next state tick,' but this behavior change (using newValue instead of typingState.input) could introduce timing issues or race conditions if the reconciliation doesn't happen before the next user input. Consider documenting why the immediate reflection is safe or adding safeguards to prevent state inconsistencies.
| useEffect(() => { | ||
| if (raceState.inProgress) { | ||
| if (raceState.inProgress && !raceState.snippet?.is_timed_test) { |
There was a problem hiding this comment.
The condition checks only raceState.snippet?.is_timed_test but the isTimedMode logic elsewhere (e.g., RaceContext line 655) includes additional conditions: raceState.timedTest?.enabled || raceState.settings?.testMode === 'timed'. This inconsistency could cause the input sync behavior to be incorrect when timed mode is enabled through different state paths.
| // Visible delta within container + scroll offset -> content-relative coords | ||
| const x = Math.round((rect.left - containerRect.left) + scrollX); | ||
| const y = Math.round((rect.top - containerRect.top) + scrollY); | ||
| // Use sub-pixel precision for smoother glide on high‑DPI displays |
There was a problem hiding this comment.
[nitpick] The comment mentions 'high‑DPI displays' but the change from Math.round() to sub-pixel precision could cause blurry rendering on standard DPI displays due to non-integer pixel positions. Consider documenting whether this has been tested across display types or if there's a deliberate trade-off being made.
| // Use sub-pixel precision for smoother glide on high‑DPI displays | |
| // Use sub-pixel precision for smoother glide on high‑DPI displays. | |
| // Note: On standard DPI displays, this may cause slight blurriness due to non-integer pixel positions. | |
| // This is a deliberate trade-off for animation smoothness and has been tested on both display types. |
| React.useEffect(() => { | ||
| if (!allowTimed && testMode === 'timed') { |
There was a problem hiding this comment.
The useEffect hook (lines 73-85) is missing testMode from its dependency array on line 85. This could cause the effect to run unnecessarily or not run when expected. The dependency array should include all values used inside the effect.
| title={authenticated ? `View ${entry.netid}'s profile` : 'Log in to view profiles'} | ||
| role={authenticated ? 'button' : undefined} | ||
| tabIndex={authenticated ? 0 : -1} | ||
| onKeyDown={authenticated ? (e => { if (e.key === 'Enter' || e.key === ' ') handleAvatarClick(entry.avatar_url, entry.netid); }) : undefined} |
There was a problem hiding this comment.
When handling keyboard events for accessibility, the Space key press should call e.preventDefault() to prevent the page from scrolling. Add e.preventDefault(); before calling handleAvatarClick when Space is pressed.
| onKeyDown={authenticated ? (e => { if (e.key === 'Enter' || e.key === ' ') handleAvatarClick(entry.avatar_url, entry.netid); }) : undefined} | |
| onKeyDown={authenticated ? (e => { | |
| if (e.key === 'Enter') { | |
| handleAvatarClick(entry.avatar_url, entry.netid); | |
| } else if (e.key === ' ') { | |
| e.preventDefault(); | |
| handleAvatarClick(entry.avatar_url, entry.netid); | |
| } | |
| }) : undefined} |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { id: 'home-practice', anchorId: 'mode-practice', content: 'Solo practice is where you can practice typing by yourself; you can choose and filter between different modes and options that we\'ll discuss later.' }, | ||
| { id: 'home-quick', anchorId: 'mode-quick', content: 'This is the Quick Match button. It will put you into the public matchmaking queue, where you are able to play against anyone else also in the queue.' }, | ||
| { id: 'home-quick', anchorId: 'mode-quick', content: 'A minimum of 2 players are required to start a game and a max of 10 players can be in a lobby.' }, | ||
| { id: 'home-quick', anchorId: 'mode-quick', content: 'A minimum of 2 players are required to start a game. Public lobbies have no cap; private lobbies are limited to 10 players.' }, |
There was a problem hiding this comment.
Duplicate step id 'home-quick' used on lines 8 and 9. Each tutorial step must have a unique id. Consider renaming line 9's id to 'home-quick-capacity' or similar.
| { id: 'home-quick', anchorId: 'mode-quick', content: 'A minimum of 2 players are required to start a game. Public lobbies have no cap; private lobbies are limited to 10 players.' }, | |
| { id: 'home-quick-capacity', anchorId: 'mode-quick', content: 'A minimum of 2 players are required to start a game. Public lobbies have no cap; private lobbies are limited to 10 players.' }, |
| // Load a snippet immediately to reflect the change | ||
| setTimeout(() => { loadNewSnippet && loadNewSnippet(); }, 0); | ||
| } | ||
| }, [allowTimed, testMode, setRaceState, setTestMode, loadNewSnippet]); |
There was a problem hiding this comment.
This useEffect can trigger infinite re-renders if loadNewSnippet is not memoized. The effect depends on loadNewSnippet, but when it calls loadNewSnippet inside the setTimeout, state changes could recreate the function reference, causing the effect to run again. Consider wrapping loadNewSnippet with useCallback in the parent component or removing it from the dependency array if it's stable.
| }, [allowTimed, testMode, setRaceState, setTestMode, loadNewSnippet]); | |
| }, [allowTimed, testMode, setRaceState, setTestMode]); |
| const lockedPosition = typingState.lockedPosition; | ||
| const text = raceState.snippet?.text || ''; | ||
|
|
||
| const isTimedMode = !!(raceState.snippet?.is_timed_test || raceState.timedTest?.enabled || raceState.settings?.testMode === 'timed'); |
There was a problem hiding this comment.
The timed mode check logic is duplicated on lines 656, 701, 632, and 753 in different files. Consider extracting this into a helper function isTimedMode(raceState) to maintain consistency and reduce code duplication.
| const isTimedMode = !!(raceState.snippet?.is_timed_test || raceState.timedTest?.enabled || raceState.settings?.testMode === 'timed'); | |
| const isTimedMode = isTimedMode(raceState); |
| // Visible delta within container + scroll offset -> content-relative coords | ||
| const x = Math.round((rect.left - containerRect.left) + scrollX); | ||
| const y = Math.round((rect.top - containerRect.top) + scrollY); | ||
| // Use sub-pixel precision for smoother glide on high‑DPI displays |
There was a problem hiding this comment.
Comment uses non-standard hyphen character '‑' (Unicode U+2011 non-breaking hyphen) in 'high‑DPI'. Should use standard ASCII hyphen '-'.
| // Use sub-pixel precision for smoother glide on high‑DPI displays | |
| // Use sub-pixel precision for smoother glide on high-DPI displays |
| title={authenticated ? `View ${entry.netid}'s profile` : 'Log in to view profiles'} | ||
| role={authenticated ? 'button' : undefined} | ||
| tabIndex={authenticated ? 0 : -1} | ||
| onKeyDown={authenticated ? (e => { if (e.key === 'Enter' || e.key === ' ') handleAvatarClick(entry.avatar_url, entry.netid); }) : undefined} |
There was a problem hiding this comment.
The keyboard event handler for Space key will trigger both the click handler and cause the page to scroll. Should call e.preventDefault() when handling the Space key to prevent default scroll behavior.
| onKeyDown={authenticated ? (e => { if (e.key === 'Enter' || e.key === ' ') handleAvatarClick(entry.avatar_url, entry.netid); }) : undefined} | |
| onKeyDown={authenticated ? (e => { if (e.key === 'Enter' || e.key === ' ') { if (e.key === ' ') e.preventDefault(); handleAvatarClick(entry.avatar_url, entry.netid); } }) : undefined} |
| const lockedPosition = typingState.lockedPosition; | ||
| const text = raceState.snippet?.text || ''; | ||
|
|
||
| const isTimedMode = !!(raceState.snippet?.is_timed_test || raceState.timedTest?.enabled || raceState.settings?.testMode === 'timed'); |
There was a problem hiding this comment.
Unused variable isTimedMode.
| const isTimedMode = !!(raceState.snippet?.is_timed_test || raceState.timedTest?.enabled || raceState.settings?.testMode === 'timed'); |
revamping frontend integration, migrating DB to it; giving template place holders and example implemented pages
Free‑typing Timed Mode (No Forced Mistake Fix), Uncapped Public Lobbies, and Docs/Leaderboard UX Refresh
(NOTE: This PR, including the PR summary, was created by Codex-CLI)
capped at 10, clarifies docs/tutorial text, and makes leaderboard player rows easier to click to view profiles.
suppress error shake.
snippet mode unchanged.
content.
hover styles, and keyboard activation; keep unauth as non‑interactive.
action.