websocket functionality added#5
websocket functionality added#5PranshuLakhotia wants to merge 9 commits intoishaanxgupta:pre_prod/ishaanfrom
Conversation
Update README.md
Update README.md
…e-upsolve-ui Improve upsolve UI
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @PranshuLakhotia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Gemini here! This pull request introduces WebSocket functionality to the application, enabling real-time communication between the backend and frontend. The primary goal appears to be the implementation of a collaborative coding feature, allowing multiple users to join a 'room' and synchronize code changes in real-time. The backend is built with Go using the Gin framework and the gorilla/websocket library, while the frontend is a React component utilizing the standard WebSocket API and the Monaco Editor.
Highlights
- WebSocket Backend: A new Go package
websocketis added to the backend, implementing a WebSocket server usinggorilla/websocket. It handles client connections, room management, and message broadcasting. - Collaborative Rooms: The backend manages multiple 'rooms', each capable of holding up to 6 connected clients. Each room maintains its current code state and a list of participants.
- Real-time Code Sync: Clients connected to the same room receive real-time updates to the code as participants type. When a client joins, they receive the current code state of the room.
- Participant Tracking: The backend tracks participants in each room and broadcasts updates to the participant list whenever someone joins or leaves.
- Frontend Integration: A new React component
CodeCastis added to the frontend, providing a UI to join rooms by ID and name, a collaborative code editor (Monaco Editor), and a list of current participants.
Changelog
Click here to see the changelog
- backend/go.mod
- Added
github.com/gin-gonic/ginfor web routing. - Added
github.com/gorilla/websocketfor WebSocket handling.
- Added
- backend/go.sum
- Updated dependency checksums for new modules.
- backend/main.go
- Imported the new
websocketpackage. - Added a GET route
/wshandled bywebsocket.WebsocketHandler.
- Imported the new
- backend/websocket/handler.go
- Created a new package
websocket. - Implemented
websocket.UpgraderwithCheckOriginset to true. - Defined
Roomstruct to manage clients, participants, code, and a mutex. - Implemented global
roomsmap androomsMutexfor room management. - Defined
Messagestruct for JSON communication. - Implemented
WebsocketHandlerto handle new connections, join/create rooms, send initial code, broadcast participant lists, and process incoming messages. - Added logic to limit room size to 6 participants.
- Added logic to delete empty rooms on client disconnect.
- Implemented
broadcastParticipantshelper function.
- Created a new package
- package-lock.json
- Added
socket.io-clientand its related dependencies (engine.io-client,socket.io-parser, etc.). Note: The frontend componentCodeCast.jsxuses the standardWebSocketAPI, notsocket.io-client.
- Added
- package.json
- Added
socket.io-clientdependency. Note: The frontend componentCodeCast.jsxuses the standardWebSocketAPI, notsocket.io-client.
- Added
- src/App.js
- Imported the new
CodeCastcomponent. - Added a new route
/codecastto render theCodeCastcomponent.
- Imported the new
- src/components/CodeCast.jsx
- Created a new React component
CodeCast. - Implemented state management for room ID, user name, connection status, code, errors, and participants.
- Used
useRefto store the WebSocket connection. - Implemented
joinRoomfunction to establish a WebSocket connection to the backend/wsendpoint with room ID and name query parameters. - Added
onopen,onmessage,onclose, andonerrorhandlers for the WebSocket. - Handled incoming messages of type 'participants' and 'code'.
- Implemented
handleCodeChangeto update local state and send code changes over the WebSocket. - Used
useEffectfor WebSocket connection cleanup on component unmount. - Integrated Monaco Editor for code editing.
- Rendered UI for joining a room and the collaborative editor/participant list view.
- Created a new React component
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Code flows, byte by byte,
Through the wire, day and night.
Shared screen, thoughts align,
Building features, truly fine.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces WebSocket functionality for real-time collaboration, including a backend service in Go and a frontend React component for a code editor. The initiative is great and the foundational work is in place. However, there are several areas, particularly in error handling, security, and frontend state management, that need attention before this can be considered production-ready. The backend uses gorilla/websocket while the frontend dependencies incorrectly include socket.io-client which is not used and is incompatible; the frontend correctly uses the native WebSocket API.
Summary of Findings
- Security: WebSocket Origin Check: The WebSocket upgrader in
backend/websocket/handler.goallows connections from any origin, which is a security risk. This should be restricted to known/allowed origins. - Error Handling in Backend: Several instances of
json.Marshalandwebsocket.Conn.WriteMessageinbackend/websocket/handler.godo not check for or handle errors, which could lead to silent failures or panics. - Frontend State Management Inconsistency: In
src/components/CodeCast.jsx, the local participant state is initially updated with a different data structure than what's received from the server, potentially causing UI issues. - Hardcoded Values: The backend uses a hardcoded maximum number of clients per room. The frontend
CodeCast.jsxuses a hardcoded WebSocket URL. These should be configurable. - Styling and UX in Frontend: Some text color styles in
CodeCast.jsxare hardcoded to white, which may not work well with all themes or light mode. Theme-aware classes should be used. - React List Keys: The participant list in
CodeCast.jsxusesindexas a key, which can be problematic if the list order changes or items are non-stably inserted/deleted. - Dependency Mismatch:
socket.io-clientis added as a frontend dependency but is not used; the frontend correctly uses the nativeWebSocketAPI which is compatible with thegorilla/websocketbackend. Thesocket.io-clientdependency should likely be removed if not planned for a future Socket.IO integration. - Hardcoded Port (Low Severity): The backend in
backend/main.goruns on a hardcoded port:8080. This was not commented on directly due to review settings.
Merge Readiness
This pull request lays a good foundation for WebSocket-based collaborative functionality. However, due to the identified high and medium severity issues related to security, error handling, and frontend correctness, I recommend that these changes not be merged until these issues are addressed. Specifically, the WebSocket origin check, backend error handling, and frontend participant state management are critical. After these are resolved, another review would be beneficial. I am unable to approve pull requests myself; please ensure further review and approval from other team members once the requested changes are made.
| CheckOrigin: func(r *http.Request) bool { | ||
| return true | ||
| }, |
There was a problem hiding this comment.
Security Concern: The CheckOrigin function currently allows all origins (return true). This is a significant security risk in a production environment as it makes the WebSocket server vulnerable to Cross-Site WebSocket Hijacking (CSWSH). Could you restrict this to allowed origins, perhaps making it configurable?
| msgBytes, _ := json.Marshal(initialCodeMsg) | ||
| conn.WriteMessage(websocket.TextMessage, msgBytes) |
There was a problem hiding this comment.
Error Handling: The error returned by json.Marshal on line 81 is ignored. If marshaling fails, msgBytes could be invalid or nil, potentially causing issues when conn.WriteMessage is called. Similarly, the error from conn.WriteMessage on line 82 is also not checked. Both of these errors should be handled to prevent unexpected behavior or panics. This pattern of ignoring json.Marshal and WriteMessage errors appears нескольких times (e.g., lines 124, 127, 144). Could you ensure these errors are properly checked and handled throughout the file?
For example, for lines 81-82:
| msgBytes, _ := json.Marshal(initialCodeMsg) | |
| conn.WriteMessage(websocket.TextMessage, msgBytes) | |
| msgBytes, err := json.Marshal(initialCodeMsg) | |
| if err != nil { | |
| log.Printf("Error marshaling initial code message: %v", err) | |
| // Depending on the desired behavior, you might want to close the connection or return | |
| return | |
| } | |
| if err := conn.WriteMessage(websocket.TextMessage, msgBytes); err != nil { | |
| log.Printf("Error sending initial code message: %v", err) | |
| // Connection might be closed or problematic, consider returning to clean up this handler | |
| return | |
| } |
| outgoing, _ := json.Marshal(incoming) | ||
| for client := range room.Clients { | ||
| if client != conn { | ||
| client.WriteMessage(websocket.TextMessage, outgoing) |
There was a problem hiding this comment.
Error Handling: The error from client.WriteMessage is not checked here. If writing to a client fails (e.g., the connection is broken), this error should be logged, and potentially the problematic client should be removed from room.Clients to avoid repeated failed attempts. While the main read loop for that client would eventually handle its disconnection, explicitly handling write errors can make the system more robust.
| ws.onopen = () => { | ||
| console.log("Connected to WebSocket"); | ||
| setIsConnected(true); | ||
| setParticipants(prev => [...prev, { name: userName, id: Date.now() }]); |
There was a problem hiding this comment.
Correctness: This line optimistically adds the current user to the participants state as an object { name: userName, id: Date.now() }. However, the server broadcasts participant lists as an array of strings (as seen in data.names on line 39). This inconsistency can lead to rendering errors (e.g., [object Object] in the UI) or unexpected behavior until the server's list arrives and overwrites it.
It's generally better to rely on the server as the single source of truth for the participant list. The server will include the new user in its next broadcast. Consider removing this optimistic update. If an immediate local reflection is desired, ensure the data structure matches what the server sends and how the list is rendered.
| } | ||
|
|
||
| room.Mutex.Lock() | ||
| if len(room.Clients) >= 6 { |
There was a problem hiding this comment.
Maintainability: The maximum number of clients per room (6) is hardcoded. Would it be better to define this as a constant at the package level (e.g., const maxClientsPerRoom = 6) for better readability and easier modification in the future?
| if len(room.Clients) >= 6 { | |
| if len(room.Clients) >= maxClientsPerRoom { // Assuming maxClientsPerRoom is defined |
| err := client.WriteMessage(websocket.TextMessage, messageBytes) | ||
| if err != nil { | ||
| log.Println("Error broadcasting participants:", err) | ||
| } |
There was a problem hiding this comment.
Error Handling & Robustness: While the error from client.WriteMessage is logged here, consider if more robust handling is needed. If a client repeatedly fails to receive messages, it might indicate a stale connection. The current design relies on the ReadMessage loop to eventually detect and clean up such clients, which is generally acceptable. However, for very high-traffic rooms or long-lived stale connections, this could lead to resource consumption or repeated log noise. No immediate change is critical, but it's something to keep in mind for future enhancements.
| } | ||
|
|
||
| setError(""); | ||
| const ws = new WebSocket(`ws://localhost:8080/ws?room=${roomId}&name=${encodeURIComponent(userName)}`); |
There was a problem hiding this comment.
Maintainability/Deployment: The WebSocket URL ws://localhost:8080/ws is hardcoded. This will make it difficult to deploy to different environments (staging, production). Could this be made configurable, perhaps through an environment variable or a configuration file?
const ws = new WebSocket(`${process.env.REACT_APP_WEBSOCKET_URL || 'ws://localhost:8080/ws'}?room=${roomId}&name=${encodeURIComponent(userName)}`);
| <h3 className="text-lg font-semibold text-neutral-900 text-white"> | ||
| Participants | ||
| </h3> |
There was a problem hiding this comment.
Styling/UX: The h3 element for "Participants" has text-neutral-900 (presumably for light mode) and text-white classes applied simultaneously. The text-white will likely always take precedence, making the text white regardless of the theme. For proper dark mode support, you might want to use dark:text-white or a similar theme-aware class.
This also applies to the participant names on line 186.
<h3 className="text-lg font-semibold text-neutral-900 dark:text-white">
Participants
</h3>
| <div className="space-y-2"> | ||
| {participants.map((name, index) => ( | ||
| <div | ||
| key={index} |
There was a problem hiding this comment.
Correctness/Performance: Using index as the key for mapped list items is generally discouraged if the list items can be reordered, added, or removed in ways other than at the end. Since the participant list can change, and the order might not be stable (especially if names are not guaranteed unique by the server for keying purposes), this could lead to incorrect component state or inefficient re-renders.
If the server could provide a unique ID for each participant, that would be ideal. If names are guaranteed unique and stable in their order from the server, using name as a key might be an improvement. Otherwise, if a stable unique ID isn't available, this is a known trade-off, but worth noting.
3 participants
No description provided.