Skip to content
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

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

jajakob
Copy link
Collaborator

@jajakob jajakob commented Jan 9, 2025

Summary by CodeRabbit

  • New Features

    • Added WebSocket (Socket.IO) support for real-time countdown functionality
    • Implemented server-side countdown management with Server-Sent Events
    • Enhanced game cockpit with live countdown updates
  • Dependencies

    • Added Socket.IO client and server type definitions
    • Installed Socket.IO client and server packages
  • Performance

    • Replaced static countdown calculation with dynamic, real-time updates

@jajakob jajakob requested a review from rschlaefli January 9, 2025 09:52
@jajakob jajakob self-assigned this Jan 9, 2025
Copy link

vercel bot commented Jan 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gbl-uzh ❌ Failed (Inspect) Jan 9, 2025 9:53am

Copy link
Contributor

coderabbitai bot commented Jan 9, 2025

📝 Walkthrough

Walkthrough

This pull request introduces real-time countdown functionality to the demo game application using Socket.IO. The changes span multiple files in the apps/demo-game directory, implementing WebSocket-based communication for managing and displaying a countdown timer. The implementation includes both server-side (Socket.IO server) and client-side (React components) modifications to support real-time countdown updates across different game interfaces.

Changes

File Change Summary
apps/demo-game/package.json Added Socket.IO related dependencies: @types/socket.io, @types/socket.io-client, socket.io, and socket.io-client
apps/demo-game/src/pages/admin/games/[id].tsx Implemented socket-based countdown management, replacing Formik form with direct socket interaction. Added socket and duration state variables.
apps/demo-game/src/pages/api/countdown.ts Created Server-Sent Events (SSE) countdown API with real-time update mechanism and client connection management
apps/demo-game/src/pages/api/socket.ts Implemented WebSocket server using Socket.IO, managing countdown timer and broadcasting updates to connected clients
apps/demo-game/src/pages/play/cockpit.tsx Updated countdown display to use WebSocket-driven real-time countdown, replacing previous static timestamp-based approach

Possibly related PRs

Finishing Touches

  • 📝 Generate Docstrings

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 object

Assigning the Socket.IO instance to the global object using (global as any).io can lead to maintenance and type safety issues. Consider managing the io instance through module scope or a dedicated singleton pattern.


22-53: Use a logging library instead of console.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 readability

The 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 content

Using an h1 element for the countdown timer may not be semantically appropriate within the sidebar. Consider using a smaller heading level like h3 or a styled div 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 code

The 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:

  1. 'disconnect' event to update UI state
  2. 'countdown-end' event to reset UI state
  3. '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

📥 Commits

Reviewing files that changed from the base of the PR and between df119f1 and 3cd12ff.

⛔ 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 configured

Verify 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 intended

The 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.

  1. 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.
  2. 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

Comment on lines +28 to +48
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)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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]

Comment on lines +6 to +8
if (res.socket.server.io) {
res.end()
return
Copy link
Contributor

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.

Suggested change
if (res.socket.server.io) {
res.end()
return
if (res.socket.server.io) {
res.status(200).json({ message: 'Socket server already initialized' })
return

Comment on lines +128 to +154
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()
}
}, [])
Copy link
Contributor

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:

  1. Add a maximum reconnection attempts limit
  2. Provide user feedback for connection status
  3. 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.

Comment on lines +846 to +867
<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
Copy link
Contributor

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:

  1. Input validation is minimal
  2. No error handling for socket emit
  3. No feedback on countdown start
  4. 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.

@jajakob
Copy link
Collaborator Author

jajakob commented Jan 9, 2025

@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 :)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant