-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(demo-game): sync countdown timer with socket #83
base: dev
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces real-time countdown functionality to the demo game application using Socket.IO. The changes span multiple files in the Changes
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (7)
apps/demo-game/src/pages/api/socket.ts (2)
56-57
: Avoid monkey patching the global objectAssigning the Socket.IO instance to the global object using
(global as any).io
can lead to maintenance and type safety issues. Consider managing theio
instance through module scope or a dedicated singleton pattern.
22-53
: Use a logging library instead ofconsole.log
Using
console.log
is suitable for development but not recommended in production code. Consider using a logging library that supports log levels and better log management.apps/demo-game/src/pages/play/cockpit.tsx (2)
Line range hint
99-212
:
Remove commented-out code to improve readabilityThe large block of commented-out code from lines 99 to 212 reduces code readability. Removing it can clean up the codebase since version control systems keep track of code history.
173-173
: Use appropriate heading level for nested contentUsing an
h1
element for the countdown timer may not be semantically appropriate within the sidebar. Consider using a smaller heading level likeh3
or a styleddiv
to match the surrounding content.Apply this diff to adjust the heading level:
- {timeRemaining > 0 && <h1>Time Left: {timeRemaining} seconds</h1>} + {timeRemaining > 0 && <h3>Time Left: {timeRemaining} seconds</h3>}apps/demo-game/src/pages/api/countdown.ts (1)
1-351
: Remove obsolete commented-out codeThe extensive block of commented-out code from lines 1 to 351 makes the file cluttered and hard to read. Since version control systems keep track of code history, it's better to remove this code to improve readability.
apps/demo-game/src/pages/admin/games/[id].tsx (2)
854-855
: Remove commented out code.Clean up the codebase by removing commented out code. If this code needs to be referenced later, it should be tracked in version control history instead.
Also applies to: 858-859, 864-864, 867-901
128-154
: Add handlers for additional socket events.The current implementation only handles 'connect' and 'connect_error' events. Consider adding handlers for:
- 'disconnect' event to update UI state
- 'countdown-end' event to reset UI state
- 'error' event for general error handling
useEffect(() => { const socketInstance = io(process.env.NEXT_PUBLIC_APP_URL, { reconnection: true, reconnectionAttempts: 5, reconnectionDelay: 1000, timeout: 10000, transports: ['websocket', 'polling'], }) if (!socketInstance.connected) { fetch('/api/socket') } socketInstance.on('connect', () => { console.log('Admin connected') setSocket(socketInstance) }) socketInstance.on('connect_error', async (error) => { console.error('Connection error admin:', error) await fetch('/api/socket') }) + socketInstance.on('disconnect', () => { + setSocket(null) + console.log('Disconnected from server') + }) + + socketInstance.on('countdown-end', () => { + setDuration(0) + }) + + socketInstance.on('error', (error) => { + console.error('Socket error:', error) + }) return () => { socketInstance.disconnect() } }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
apps/demo-game/package.json
(2 hunks)apps/demo-game/src/pages/admin/games/[id].tsx
(6 hunks)apps/demo-game/src/pages/api/countdown.ts
(1 hunks)apps/demo-game/src/pages/api/socket.ts
(1 hunks)apps/demo-game/src/pages/play/cockpit.tsx
(3 hunks)
🔇 Additional comments (3)
apps/demo-game/src/pages/play/cockpit.tsx (1)
87-89
: Ensure 'NEXT_PUBLIC_APP_URL' is correctly configuredVerify that
process.env.NEXT_PUBLIC_APP_URL
is set and points to the correct application URL to establish the socket connection successfully.apps/demo-game/src/pages/api/countdown.ts (1)
Line range hint
352-383
:
Confirm that the countdown functionality is implemented as intendedThe current implementation of the
handler
function seems to only maintain an SSE connection by sending ping messages, and the countdown logic is commented out. Verify whether this is intentional or if the countdown functionality needs to be included.Do you want me to help integrate the countdown logic into this handler or open a GitHub issue to track this task?
apps/demo-game/package.json (1)
20-21
: Ensure consistent Socket.IO versioning and update to latest stable version.
- Socket.IO server and client versions should match exactly to avoid compatibility issues. Currently using fixed
4.8.1
for server but^4.8.1
for client.- Consider updating to the latest stable version for security patches and bug fixes.
Run this script to check the latest stable versions and any security advisories:
Also applies to: 50-51
socket.on('start-countdown', (duration) => { | ||
console.log('Countdown started:', duration) | ||
// Clear previous interval | ||
if (countdownInterval) { | ||
clearInterval(countdownInterval) | ||
} | ||
|
||
remainingTime = duration | ||
|
||
countdownInterval = setInterval(() => { | ||
if (remainingTime > 0) { | ||
remainingTime -= 1 | ||
io.emit('countdown-update', remainingTime) | ||
} else { | ||
if (countdownInterval) { | ||
clearInterval(countdownInterval) | ||
} | ||
io.emit('countdown-finished') | ||
} | ||
}, 1000) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restrict 'start-countdown' event to authorized clients
Any connected client can emit the 'start-countdown'
event, which can lead to unauthorized users starting or resetting the countdown. Implement authentication or authorization checks to ensure only authorized users (e.g., admins) can initiate the countdown.
[security_issue]
if (res.socket.server.io) { | ||
res.end() | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Provide a consistent response when Socket.IO server is already initialized
Currently, when the Socket.IO server is already initialized, the response is ended without any feedback to the client. This may lead to confusion for clients making the request. Consider sending a JSON response indicating that the Socket server is already running.
Apply this diff to improve the response:
if (res.socket.server.io) {
- res.end()
+ res.status(200).json({ message: 'Socket server already initialized' })
return
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if (res.socket.server.io) { | |
res.end() | |
return | |
if (res.socket.server.io) { | |
res.status(200).json({ message: 'Socket server already initialized' }) | |
return |
useEffect(() => { | ||
const socketInstance = io(process.env.NEXT_PUBLIC_APP_URL, { | ||
reconnection: true, | ||
reconnectionAttempts: 5, | ||
reconnectionDelay: 1000, | ||
timeout: 10000, | ||
transports: ['websocket', 'polling'], | ||
}) | ||
|
||
if (!socketInstance.connected) { | ||
fetch('/api/socket') | ||
} | ||
|
||
socketInstance.on('connect', () => { | ||
console.log('Admin connected') | ||
setSocket(socketInstance) | ||
}) | ||
|
||
socketInstance.on('connect_error', async (error) => { | ||
console.error('Connection error admin:', error) | ||
await fetch('/api/socket') | ||
}) | ||
|
||
return () => { | ||
socketInstance.disconnect() | ||
} | ||
}, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance socket connection reliability and user feedback.
The socket initialization needs improvements in error handling and user feedback:
- Add a maximum reconnection attempts limit
- Provide user feedback for connection status
- Add loading state management
Apply these improvements:
useEffect(() => {
const socketInstance = io(process.env.NEXT_PUBLIC_APP_URL, {
reconnection: true,
- reconnectionAttempts: 5,
+ reconnectionAttempts: 3,
reconnectionDelay: 1000,
timeout: 10000,
transports: ['websocket', 'polling'],
})
+ const [isConnecting, setIsConnecting] = useState(true)
+ const [connectionError, setConnectionError] = useState<string | null>(null)
if (!socketInstance.connected) {
fetch('/api/socket')
}
socketInstance.on('connect', () => {
console.log('Admin connected')
setSocket(socketInstance)
+ setIsConnecting(false)
+ setConnectionError(null)
})
socketInstance.on('connect_error', async (error) => {
- console.error('Connection error admin:', error)
+ setConnectionError(`Connection error: ${error.message}`)
+ setIsConnecting(false)
await fetch('/api/socket')
})
return () => {
socketInstance.disconnect()
}
}, [])
Committable suggestion skipped: line range outside the PR's diff.
<Card className="flex flex-col"> | ||
<CardHeader> | ||
<CardTitle>Countdown</CardTitle> | ||
<CardDescription>Set a countdown for the segment.</CardDescription> | ||
</CardHeader> | ||
<CardContent> | ||
<input | ||
type="number" | ||
// value={countdownSeconds} | ||
// onChange={handleCountdownChange} | ||
value={duration} | ||
onChange={(e) => setDuration(Number(e.target.value))} | ||
// placeholder="Countdown in seconds" | ||
min="1" | ||
/> | ||
</CardContent> | ||
<CardFooter> | ||
<Button onClick={handleStartCountdown}>Set Countdown</Button> | ||
{/* <Button onClick={setCountdown}>Set Countdown</Button> */} | ||
</CardFooter> | ||
</Card> | ||
{/* <Formik |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve countdown implementation with validation and feedback.
The current countdown implementation needs several improvements:
- Input validation is minimal
- No error handling for socket emit
- No feedback on countdown start
- No visual indication of active countdown
Apply these improvements:
<Card className="flex flex-col">
<CardHeader>
<CardTitle>Countdown</CardTitle>
<CardDescription>Set a countdown for the segment.</CardDescription>
</CardHeader>
<CardContent>
+ <div className="flex flex-col gap-2">
<input
type="number"
- value={duration}
- onChange={(e) => setDuration(Number(e.target.value))}
+ value={duration || ''}
+ onChange={(e) => {
+ const value = Number(e.target.value)
+ if (value >= 0 && value <= 3600) {
+ setDuration(value)
+ }
+ }}
min="1"
+ max="3600"
+ className={`w-full rounded border p-2 ${
+ duration > 0 ? 'border-gray-300' : 'border-red-300'
+ }`}
/>
+ {duration <= 0 && (
+ <p className="text-sm text-red-500">
+ Please enter a duration between 1 and 3600 seconds
+ </p>
+ )}
+ </div>
</CardContent>
<CardFooter>
- <Button onClick={handleStartCountdown}>Set Countdown</Button>
+ <Button
+ onClick={handleStartCountdown}
+ disabled={!socket || duration <= 0}
+ >
+ {!socket ? 'Connecting...' : 'Start Countdown'}
+ </Button>
</CardFooter>
</Card>
Also update the handleStartCountdown
function:
const handleStartCountdown = () => {
if (socket) {
+ try {
socket.emit('start-countdown', duration)
+ toast({
+ title: 'Countdown Started',
+ description: `Countdown set for ${duration} seconds`,
+ })
+ } catch (error) {
+ console.error('Failed to start countdown:', error)
+ toast({
+ title: 'Error',
+ description: 'Failed to start countdown. Please try again.',
+ variant: 'destructive',
+ })
+ }
} else {
console.error('Socket not connected')
+ toast({
+ title: 'Error',
+ description: 'Not connected to server. Please wait or refresh the page.',
+ variant: 'destructive',
+ })
}
}
Committable suggestion skipped: line range outside the PR's diff.
@rschlaefli Here is a solution for the countdown (without front end perks) I came up with. I solved it with sockets. The admin broadcasts a new countdown time, which is count down in the back end, and the players just listen to the new value and update it in the front end. That way it is in real-time and all players have the same countdown. Not sure if this approach is over-kill or might introduce problems, so let me know what you think :)! |
Summary by CodeRabbit
New Features
Dependencies
Performance