Fix/socket collaboration migration - #1342
Conversation
… to Socket.io WebSockets
…restore security rules, and room creation fallback
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
quality:exceptional
|
Hi @vijaypatil477, thanks for contributing to Debugra! 🎉 I have automatically:
Our workflows will now analyze your changes to classify:
Tip Ensure your PR description references the issue it resolves (e.g. Happy coding! 🚀 |
github-actions
Bot
added
gssoc:approved
📝 WalkthroughWalkthroughThe change adds Socket.IO-based room collaboration, modular Firebase Admin access, broader Firestore rules, Judge0-to-Wandbox execution fallback, updated webhook and rate-limit behavior, and ESLint, Vitest, and Vite configuration. ChangesRealtime collaboration
Execution and platform behavior
Development tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Editor as EditorPage
participant Room as useRoom
participant Socket as Socket.IO server
participant Store as Firestore
Editor->>Room: connect to room
Room->>Socket: join room and send editor events
Socket->>Store: seed or persist room state
Socket-->>Room: state, presence, cursor, and chat events
Room-->>Editor: update editor and collaboration state
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonParsing error: Unexpected token : server/package.jsonParsing error: Unexpected token : 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Editor/EditorPage.jsx (1)
1943-1961: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a
keysoChatPanelresets when the room changes.
ChatPanelholdsmessagesin local state and no longer loads them from Firestore. A room switch changes theroomIdandsocketprops but does not remount the component, so React keeps the previous room'smessagesarray. The user then sees messages from the previous room in the new room.Set
key={room.roomId}on both instances to force a fresh mount per room.🐛 Proposed fix
<ChatPanel + key={room.roomId} roomId={room.roomId} user={user} isOpen={true} onToggle={() => setMobileTab(MOBILE_TABS.CODE)} socket={room.socket} /><ChatPanel + key={room.roomId} roomId={room.roomId} user={user} isOpen={chatOpen} onToggle={() => setChatOpen(!chatOpen)} socket={room.socket} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Editor/EditorPage.jsx` around lines 1943 - 1961, Update both ChatPanel instances in EditorPage so each receives key={room.roomId}, ensuring switching rooms remounts ChatPanel and resets its local messages state.
🟠 Major comments (15)
src/hooks/useWebRTC.js-100-116 (1)
100-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent replay of processed signal documents.
Firestore sends existing matching documents as
addedwhen this listener starts. The handler only checkspeersRef.current[data.senderUid], so an unprocessed signal from before a disconnect can be applied to the new peer. Restore one-time consumption or add a call-session/document ID and consume/deduplicate signals once before callingpeer.signal().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useWebRTC.js` around lines 100 - 116, Update the signal handling in useWebRTC’s onSnapshot callback to consume or deduplicate each signal document before creating a peer or calling peer.signal(). Ensure documents replayed as added on listener startup, including stale pre-disconnect signals, are ignored after prior processing while allowing each current call-session signal to be applied once.eslint.config.js-24-30 (1)
24-30: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister Vitest globals for
src/hooks/__tests__files.Vitest includes
src/**/__tests__/**/*.{test,spec}.{js,jsx}and enables globals, but these imports usedescribe,it,expect, andvidirectly. Add a later flat-config entry forsrc/hooks/__tests__that setslanguageOptions.globals.vitest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.js` around lines 24 - 30, Update the flat ESLint configuration after the existing config entry with a later rule targeting src/hooks/__tests__ test files, and set languageOptions.globals.vitest for that pattern so direct describe, it, expect, and vi usage is recognized.vite.config.js-20-26 (1)
20-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not externalize Monaco in the browser build.
externalleavesmonaco-editor/esm/vs/editor/editor.apias a bare browser import.@monaco-editor/reactand monaco-vim need Monaco resolved by the bundler; with this config, the Editor route can fail in production. Remove therolldownOptions.externalentry and addmonaco-editordirectly if peer resolution fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vite.config.js` around lines 20 - 26, Update the build configuration’s rolldownOptions by removing the external entry for monaco-editor/esm/vs/editor/editor.api so Monaco is bundled for browser production builds; if peer dependency resolution still fails, add monaco-editor directly to the build inputs or dependencies using the existing project convention.server/services/judge0Service.js-3-29 (1)
3-29: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHonor custom
JUDGE0_API_URLvalues.
getJudge0Configreturns either the RapidAPI endpoint orhttps://ce.judge0.com/submissions?wait=true; it never usesenvUrlfor non-RapidAPI values. A configured self-hosted Judge0 URL is discarded, and code execution then posts source code to the public Judge0 CE endpoint. Add a branch that usesenvUrlfor custom, non-RapidAPI URLs unless that configuration option is no longer supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/judge0Service.js` around lines 3 - 29, Update getJudge0Config so configured non-RapidAPI envUrl values are preserved instead of falling back to the public Judge0 endpoint. Add a custom-URL branch after the RapidAPI handling, returning envUrl with the required submissions?wait=true path and JSON headers, while retaining the existing RapidAPI and default public endpoint behavior.server/middleware/rateLimiter.js-4-5 (1)
4-5: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReturn the actual client IP from
getIpKey.
ipKeyGenerator(req)does not create an IP-based limiter key.getIpKeyshould fall back to a client IP, with IPv6 normalized viaipKeyGenerator(req.ip), otherwise the rate limiter can get one bucket per request object and allow clients to bypassmax: 5.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/middleware/rateLimiter.js` around lines 4 - 5, Update getIpKey to derive the key from the client IP rather than passing the request object to ipKeyGenerator: use req.ip as the fallback input and normalize it through ipKeyGenerator(req.ip), preserving the actual client-IP-based limiter bucket.src/hooks/useRoom.js-136-147 (1)
136-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
join-roomalways reports the stale initial role.Line 145 sends
role: myRole.myRolederives fromroomData(Lines 107-109), androomDatais loaded by the separate effect at Lines 202-216, which resolves after the socket connects.At connect time
roomDataisnull, somyRoleevaluates to'viewer'for every user, including the room host. The effect deps at Line 185 omitmyRole, andjoin-roomis emitted only inside theconnecthandler, so the role is never corrected. The presence list built atsocketService.jsLines 68-73 therefore shows every collaborator as a viewer for the whole session.Emit a role update after
roomDataresolves, or wait forroomDatabefore joining.🐛 Proposed fix
Send the role in a separate effect that reacts to
myRole:+ // Re-announce the role whenever it resolves or changes. + useEffect(() => { + if (!roomId || !user || !socket || !isConnected) return; + socket.emit('join-room', { + roomId, + user: { + uid: user.uid, + displayName: user.displayName || user.email?.split('@')[0] || 'Guest', + email: user.email, + }, + role: myRole, + }); + }, [roomId, user, socket, isConnected, myRole]);The server treats
join-roomas idempotent (socketService.jsLines 67-79), so the repeat call updates the existing entry. The server must still resolve the authoritative role itself, as noted onserver/services/socketService.js.Also applies to: 185-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRoom.js` around lines 136 - 147, Update the socket join flow around the connect handler and the effect at line 185 so room membership is emitted again when myRole changes after roomData loads. Keep the initial connection behavior, but add a role-reactive join-room emission using the existing payload and socket state, ensuring the server receives the authoritative resolved role instead of retaining the initial viewer value.firestore.rules-56-58 (1)
56-58: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAny signed-in user can delete other users' subcollection documents.
votes,signals, anddrawingsnow allowdeletefor any authenticated user, with no participant check and no owner check.createis also unconstrained, so a document is not tied to its author.A signed-in user who is not a participant can delete another room's whiteboard drawings and WebRTC signal documents, which breaks the call setup and the whiteboard for that room.
Restrict these rules to room participants, and restrict
deleteto the author or the intended recipient.🔒 Proposed direction
+ function isParticipant() { + let room = get(/databases/$(database)/documents/rooms/$(roomId)).data; + return request.auth != null + && (request.auth.uid == room.createdBy || request.auth.uid in room.participantIds); + } + match /signals/{signalId} { - allow read, create, delete: if request.auth != null; + allow read: if isParticipant(); + allow create: if isParticipant() && request.resource.data.senderId == request.auth.uid; + allow delete: if isParticipant() + && (resource.data.senderId == request.auth.uid + || resource.data.recipientId == request.auth.uid); } match /drawings/{drawingId} { - allow read, create, delete: if request.auth != null; + allow read, create: if isParticipant(); + allow delete: if isParticipant() && resource.data.uid == request.auth.uid; }Adjust the field names to match the documents that
src/hooks/useWebRTC.jsand the whiteboard component write.Also applies to: 84-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@firestore.rules` around lines 56 - 58, Update the Firestore rules for the affected subcollections around the `allow read/create/update/delete` statements so access requires room participation, creation binds the document to its author using the field names written by `useWebRTC.js` and the whiteboard component, and deletion is limited to that author or the intended recipient. Apply the same restrictions to the additional affected rule block.src/hooks/useRoom.js-356-361 (1)
356-361: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe execution vote no longer reaches a real quorum.
Presence moved to Socket.IO, so
activeUsersis no longer written to the room document. This join path updatesparticipantIdsandrolesonly.
castVotestill reads presence from Firestore:const totalUsersCount = (data.activeUsers || []).length;at Line 504. That value is now always0. The initiator pre-approves at Line 459, soapprovals.length > 0 / 2is true immediately and the vote status becomesapprovedon creation. The consensus gate on code execution is bypassed. The deadlock-resolution effect at Line 607 has the same problem.Make the active-user count available to the vote logic. Two options:
- Have the server write the current
activeUserscount into the room document on eachpresence-update, so the transaction reads a fresh value.- Pass the socket-derived
activeUsers.lengthintocastVoteand store it on the vote when it is created.The first option is safer, because a client-supplied count lets any client force a quorum.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRoom.js` around lines 356 - 361, Update the Socket.IO presence-update handling so the server persists the current active-user count in the room document, preserving the existing activeUsers field shape consumed by castVote and the deadlock-resolution effect. Ensure the write occurs whenever presence changes and that vote creation and quorum checks read this fresh server-derived value instead of defaulting to an empty list.src/components/Chat/ChatPanel.jsx-46-66 (1)
46-66: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftChat messages are no longer persisted, so history is lost.
This component now receives messages only from the live socket.
server/services/socketService.jsLines 142-146 relayschat-messagewithout storing it, and nothing else writes themessagessubcollection.Consequences:
- A user who joins after a message was sent never sees it.
- A page reload clears the entire conversation.
- Messages sent while a user's socket is down are lost permanently, because the server relays with
socket.to(roomId)and keeps no buffer.
firestore.rulesLines 68-70 still grantread, createonrooms/{roomId}/messages, which indicates the collection is still expected to hold the history.Write each message to Firestore from the server when it relays the event, and load the recent history when the panel mounts.
Do you want me to open an issue for restoring chat persistence?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Chat/ChatPanel.jsx` around lines 46 - 66, Restore chat persistence by updating the server’s chat-message relay in socketService to write each message to the room’s Firestore messages subcollection before or alongside broadcasting it. In ChatPanel’s mount effect, load recent messages for roomId from Firestore, initialize messages with that history, and retain the existing socket listener for new-message updates and deduplication.server/services/socketService.js-5-7 (1)
5-7: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRoom state survives only in process memory and is flushed only when the room empties.
activeRoomsis module-level state in a single Node process, and Firestore receives a write only whenactiveUsers.length === 0(Line 165).Two failure modes follow:
- A crash, a deploy, or a restart loses every unsaved edit for every active room. The previous Firestore-backed sync wrote continuously, so this migration removes durability that users relied on.
- With more than one server instance, two users in the same room can connect to different instances. Each instance keeps its own
activeRoomsentry, so the users never see each other's edits. The instance that empties last overwrites the other instance's saved code.Recommended changes:
- Flush
room.code,room.language,room.stdin, andupdatedAton a debounced interval, for example every 10 seconds whenroom.code !== room.lastSavedCode.lastSavedCodeis already tracked at Line 60 but never read.- Add the Socket.IO Redis adapter and move
activeRoomsto shared storage before running more than one instance.- Flush all rooms on
SIGTERMbefore the process exits.Do you want me to implement the debounced flush and the
SIGTERMhandler?Also applies to: 164-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/socketService.js` around lines 5 - 7, Make room persistence durable by periodically flushing each activeRooms entry when room.code differs from lastSavedCode, including code, language, stdin, and updatedAt, then update lastSavedCode after a successful write. Add a SIGTERM shutdown handler that flushes all rooms before exit, and configure the Socket.IO Redis adapter with shared room-state storage so users connected to different instances observe the same edits.server/services/socketService.js-149-182 (1)
149-182: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftOne disconnected tab removes a user who is still connected elsewhere.
currentRoomIdandcurrentUserare per-socket closures, butactiveUsersis deduplicated byuidat Line 67. If one user opens two tabs, the room holds a single entry for that uid.When the first tab disconnects, Line 159 filters the uid out of
activeUsers. The second tab is still joined, but presence reports the user as gone. If that user was the only member, Line 165 treats the room as empty, flushes it, and callsactiveRooms.delete(roomId)at Line 179. The surviving socket keeps emittingcode-update, but Line 95 returns early becauseactiveRoomsno longer holds the room. That tab silently stops syncing until the user reloads.Track connections per uid, and remove the user only when the last socket for that uid disconnects.
A second problem in the same handler: a socket can call
join-roomagain with a differentroomId. Line 28 overwritescurrentRoomId, but the socket never leaves the previous Socket.IO room, so it keeps receiving events from it and is never removed from itsactiveUserslist.🐛 Proposed direction
socket.on('join-room', async ({ roomId, user, role }) => { if (!roomId || !user) return; + if (currentRoomId && currentRoomId !== roomId) { + socket.leave(currentRoomId); + removeSocketFromRoom(currentRoomId, currentUser, socket.id); + }Store
sockets: Set<socketId>on eachactiveUsersentry. Ondisconnect, deletesocket.idfrom that set and remove the user only when the set is empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/socketService.js` around lines 149 - 182, Update the room membership flow around join-room and the disconnect handler to track socket IDs per active user, adding and removing socket.id from each user’s sockets Set and removing the user only when that Set becomes empty. Before switching rooms, remove the socket from its previous room’s membership and Socket.IO room so rejoining cannot leave stale presence or subscriptions; preserve the existing empty-room persistence and cleanup only after the last member is gone.server/services/socketService.js-94-114 (1)
94-114: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCap the size of
codeandstdinpayloads.Neither handler limits the payload size.
room.codeandroom.stdinaccept unbounded strings from a client, retain them in theactiveRoomsmap, and broadcast them to every room member.A single client can push megabytes per event. That grows the process heap without bound and multiplies outbound bandwidth by the number of room members.
src/hooks/useRoom.jsLines 540-548 already cap execution output at 10 000 characters, so the codebase has a precedent for this limit.A payload above the 1 MiB Firestore document limit also makes the persistence write at Line 169 fail permanently, so the room can never be saved again.
Reject or truncate oversized payloads, and set
maxHttpBufferSizeon the Socket.IO server.🛡️ Proposed fix
+const MAX_CODE_BYTES = 500 * 1024; // stays under the 1 MiB Firestore document limit +const MAX_STDIN_BYTES = 64 * 1024; + function initSocketServer(server, allowedOrigins) { const io = new Server(server, { + maxHttpBufferSize: 1e6, cors: { ... }, });socket.on('code-update', ({ roomId, code, language }) => { if (!roomId || !activeRooms.has(roomId)) return; + if (typeof code !== 'string' || Buffer.byteLength(code, 'utf8') > MAX_CODE_BYTES) { + logger.warn(`[Socket] Rejected oversized code-update for room ${roomId}`); + return; + }Apply the equivalent guard with
MAX_STDIN_BYTESinstdin-update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/socketService.js` around lines 94 - 114, Cap incoming code and stdin payloads before storing or broadcasting them: define and reuse byte-size limits, including the proposed MAX_STDIN_BYTES guard, and reject or truncate values exceeding the limits. Update both the code-update and stdin-update handlers in the socket service, and configure the Socket.IO server’s maxHttpBufferSize to enforce the transport-level limit while preserving normal room updates.firestore.rules-11-14 (1)
11-14: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the public read allow on
/users/{userId}.The broader rule permits unauthenticated reads on every user document because it matches above the owner-only rule. User documents store
uid,displayName, anddisplayNames/{normalizedName}collection or a server-side Admin SDK check instead of public reads of user documents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@firestore.rules` around lines 11 - 14, Remove the `allow read: if true` rule from the `/users/{userId}` match block, preserving the existing owner-only access rule. Update signup display-name uniqueness checks to use the `displayNames/{normalizedName}` collection or a server-side Admin SDK lookup rather than reading user documents publicly.src/hooks/useRoom.js-246-296 (1)
246-296: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftStore password materials when creating password-protected rooms.
createRoomsetspasswordProtected: true, but neither the Firestore write nor/api/rooms/createstorespasswordHashorpasswordSalt; Firestore rules only block client writes to those fields. Joined users then reachPOST /api/rooms/verify-password, which returns500 Room configuration error.Derive the hash with a fresh salt over HTTPS and writepasswordHashandpasswordSaltusing the Admin SDK.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRoom.js` around lines 246 - 296, Update createRoom to derive a password hash and fresh salt when trimmedPassword is present, then include passwordHash and passwordSalt in the server room-creation payload over HTTPS; do not rely on the client-side setDoc for these protected fields, since they must be written through the Admin SDK. Ensure password-protected rooms persist both materials and unprotected rooms retain the existing behavior.src/components/Auth/AuthModal.jsx-75-86 (1)
75-86: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFail closed when display-name lookup fails.
If the duplicate lookup throws, this code logs and then runs
createUserWithEmailAndPassword, so signup can bypass the read. Client-side Firestore reads are not atomic undermatch /users/{userId} { allow read: if true; }, and the server has no displayed uniqueness reservation fordisplayNameLower. Block signup when the lookup cannot complete, or use a trusted atomic reservation.Proposed fail-closed handling
} catch (err) { - console.warn('[AuthModal] Username uniqueness check failed:', err.message); + toast.error('Could not verify the display name. Please try again.'); + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Auth/AuthModal.jsx` around lines 75 - 86, Update the username uniqueness-check catch block in AuthModal’s signup flow to fail closed: after logging the lookup error, stop signup, clear the loading state, and surface an appropriate error instead of continuing to create the account. Preserve the existing duplicate-name rejection behavior and ensure createUserWithEmailAndPassword runs only when the lookup completes successfully.
🟡 Minor comments (6)
server/tests/cors.test.js-15-18 (1)
15-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winImport the Vitest globals in
server/tests/cors.test.js.The lint config only defines browser/ES globals and
server/tests/cors.test.jsdoes not explicitly importit,expect, ordescribe. Add the import above the describe block to avoid these undefined-globals errors.Proposed fix
+import { expect, it } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/cors.test.js` around lines 15 - 18, Import the Vitest globals used by server/tests/cors.test.js—describe, it, and expect—before the existing describe block so the test file passes lint without relying on implicit globals.Source: Linters/SAST tools
server/services/judge0Service.js-6-7 (1)
6-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix
no-undefforprocessin ESLint’s server rules.
eslint.config.jsdeclaresglobals.browserandglobals.es2020for**/*.{js,jsx}, but notglobals.node. Addglobals.nodeor an equivalent Node environment for theserver/files soprocessis not flagged byno-undef.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/judge0Service.js` around lines 6 - 7, Update the ESLint configuration for server-side JavaScript files to include the Node globals environment, ensuring process in judge0Service.js and other server files is recognized and no longer triggers no-undef. Preserve the existing browser and ES2020 globals configuration for non-server files.Source: Linters/SAST tools
src/components/Chat/ChatPanel.jsx-80-81 (1)
80-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
crypto.randomUUID()for the message id.
Math.random().toString(36).substring(2, 9)produces at most 7 base-36 characters, and fewer whenevertoString(36)drops trailing zeros. That is roughly 36^7 values at best, generated independently by every client.The id serves as the React
keyat Line 387 and as the deduplication key at Line 52. A collision between two users silently discards a legitimate message.
src/hooks/useRoom.jsLine 252 already usescrypto.randomUUID(). The comment on Line 80 also calls this a "UUID prefix", which does not match the implementation.♻️ Proposed fix
- // Generate standard client-side UUID prefix - const msgId = Math.random().toString(36).substring(2, 9); + const msgId = + typeof crypto !== 'undefined' && typeof crypto.randomUUID === 'function' + ? crypto.randomUUID() + : `${Date.now()}-${user.uid}-${Math.random().toString(36).slice(2)}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Chat/ChatPanel.jsx` around lines 80 - 81, Update the message-id assignment in ChatPanel to use crypto.randomUUID() instead of the Math.random-based value, preserving the resulting id’s use as the React key and deduplication key.src/hooks/useRoom.js-173-178 (1)
173-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemote cursors are never removed when a user leaves.
The
cursor-movehandler only adds and updates entries. No handler deletes an entry, andpresence-update(Line 160) does not pruneremoteCursors.
src/components/Editor/EditorPage.jsxLines 592-638 renders one decoration and one injected<style>element per entry. Its cleanup at Lines 645-654 removes style elements only for uids that are absent fromremoteCursors, and that object never shrinks. A user who leaves the room keeps a visible ghost cursor and a<style>element in the document head until the page reloads.Prune
remoteCursorsagainst the presence list.🐛 Proposed fix
newSocket.on('presence-update', (users) => { setActiveUsers(users); + const liveUids = new Set(users.map((u) => u.uid)); + setRemoteCursors((prev) => { + const next = {}; + for (const [uid, cursor] of Object.entries(prev)) { + if (liveUids.has(uid)) next[uid] = cursor; + } + return next; + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRoom.js` around lines 173 - 178, Update the presence-update handling in useRoom so remoteCursors is pruned against the current presence list, removing entries for users who are no longer present while preserving active cursor entries. Keep the existing cursor-move updates in the newSocket handler unchanged.package.json-11-11 (1)
11-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExtend
lintto coverserver/with a CommonJS Node config.
lintonly runseslint src, so the new server files never get lint coverage. Ifserver/**.jsfiles are added, ESLint will still apply the existingfiles: ['**/*.{js,jsx}']config, which setssourceType: 'module', browser globals, and React rules. Add a later config block withfiles: ['server/**/*.js'],sourceType: 'commonjs', andglobals: { node: true }, then update the script to includeserverwhile excluding the React JSX/client pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 11, Extend the lint script to lint server files alongside src while excluding the React JSX/client pattern, and add a later ESLint configuration block targeting server/**/*.js with CommonJS source type and Node globals. Preserve the existing client/React configuration for src and ensure the server override takes precedence.Source: Linters/SAST tools
src/components/Auth/AuthModal.jsx-77-79 (1)
77-79: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse one canonical display name for lookup and storage.
Line 78 trims
namefor the query, but Line 89 passes the rawnametoupdateProfile.saveUserthen derivesdisplayNameLowerfrom that value at Lines 36-38. An input such as" Alice "can store" alice "while a later lookup searches"alice".Trim the value once. Use it for the query,
updateProfile, anddisplayNameLower.Proposed normalization
+ const normalizedName = name.trim(); try { const { collection, query, where, getDocs } = await import('firebase/firestore'); const q = await getDocs( - query(collection(db, 'users'), where('displayNameLower', '==', name.trim().toLowerCase())) + query(collection(db, 'users'), where('displayNameLower', '==', normalizedName.toLowerCase())) ); ... - await updateProfile(result.user, { displayName: name }); + await updateProfile(result.user, { displayName: normalizedName });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Auth/AuthModal.jsx` around lines 77 - 79, Normalize the display name once in the AuthModal submission flow, then reuse that trimmed value for the users query, updateProfile call, and saveUser/displayNameLower derivation. Update the relevant variable and references near getDocs and updateProfile so inputs such as surrounding whitespace produce one canonical stored and queried name.
🧹 Nitpick comments (5)
server/services/judge0Service.js (2)
86-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the fallback when Judge0 returns an unexpected response shape.
If
data.status.idis undefined, execution falls through to the Wandbox fallback without any log message. Thecatchblock below logs a warning, so this specific fallback path is comparatively silent. Add a log statement before falling through, so operators can distinguish "Judge0 unreachable" from "Judge0 returned an unexpected payload."🔍 Proposed fix to log malformed Judge0 responses
const data = response.data; if (data && data.status && data.status.id !== undefined) { const stdout = (data.stdout || '').slice(0, MAX_OUTPUT_LENGTH); const stderr = (data.stderr || data.compile_output || data.message || '').slice(0, MAX_OUTPUT_LENGTH); return { stdout: stdout || null, stderr: stderr || null, compile_output: data.compile_output || null, status: data.status, time: data.time || null, memory: data.memory || null, }; } + console.warn('[Execution] Judge0 CE returned an unexpected response shape. Falling back to Wandbox...'); } catch (judge0Error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/judge0Service.js` around lines 86 - 102, Add a warning immediately before the Judge0 response-handling block falls through when data.status.id is undefined, clearly indicating that Judge0 returned an unexpected response payload and execution is falling back to Wandbox. Keep the existing catch warning for request failures unchanged.
125-130: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider marking the maintenance error as retryable for callers.
The thrown maintenance error is a plain
Errorwith only a message string.server/routes/execute.jspasses it tonext(err)with no special handling, so callers can only detect "retryable" failures by matching a substring in the message. Attach a machine-readable marker (e.g.,err.statusCode = 503orerr.retryable = true) so the route and downstream error handling can respond with the correct HTTP status and support automated retry.♻️ Proposed fix to mark the error as retryable
if (isWandboxRuntimeFailure) { - throw new Error('Remote execution engine is currently undergoing maintenance. Please retry in a few moments.'); + const maintenanceError = new Error('Remote execution engine is currently undergoing maintenance. Please retry in a few moments.'); + maintenanceError.statusCode = 503; + maintenanceError.retryable = true; + throw maintenanceError; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/services/judge0Service.js` around lines 125 - 130, Update the maintenance error thrown in the isWandboxRuntimeFailure branch to include a machine-readable retryable marker, such as statusCode 503 or retryable true, while preserving the existing message and next(err) propagation through execute.js.server/routes/webhooks.js (1)
186-189: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the changed no-destination response contract.
Line 186 changes
/api/webhooks/room-eventfrom HTTP 503 to HTTP 200 without dispatching an event. Add a regression test for the status and response body. Verify that callers do not rely on HTTP 503 to detect missing integration configuration. Keep a separate test for configured destination failures returning HTTP 502.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/routes/webhooks.js` around lines 186 - 189, Add regression coverage for the no-destination branch in the room-event webhook handler, asserting HTTP 200 and the complete response body including success, empty dispatched results, and the configuration message. Verify callers do not depend on HTTP 503 for missing integrations, while preserving a separate test that configured destination failures return HTTP 502.src/hooks/useRoom.js (1)
119-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant early-return branch that calls setState in the effect body.
When
roomIdoruserbecomes falsy, React runs the previous effect's cleanup (Lines 180-184) before this body executes. That cleanup already callsdisconnect(),setSocket(null), andsetIsConnected(false).The branch is therefore redundant. It also reads
socket, which is not in the dependency list at Line 185, so it can act on a stale value. The synchronoussetSocket(null)call triggers the ESLintreact-hooks/set-state-in-effecterror reported for Line 123 and forces an extra render.Delete the branch.
♻️ Proposed refactor
useEffect(() => { - if (!roomId || !user) { - if (socket) { - socket.disconnect(); - setSocket(null); - } - setIsConnected(false); - return; - } + if (!roomId || !user) return; const apiUrl = import.meta.env.VITE_API_URL || 'http://localhost:3001';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useRoom.js` around lines 119 - 127, Remove the early-return branch in the useRoom effect that checks !roomId or !user, including its socket disconnect and state updates, so the effect no longer performs redundant setState calls or reads stale socket state; rely on the existing cleanup for disconnect and reset behavior.Source: Linters/SAST tools
server/package.json (1)
30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused Yjs dependencies. Both manifests add
yjsandy-websocket, and the client also addsy-monaco, but the repository has no code that consumes them. Keep the CRDT/editor binding only if this PR lands it.
server/package.json#L30-L31: removey-websocketandyjs.package.json#L50-L54: removey-monaco,y-websocket, or add the Yjs binding that uses them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/package.json` around lines 30 - 31, Remove the unused Yjs dependencies: in server/package.json lines 30-31, remove y-websocket and yjs; in package.json lines 50-54, remove y-monaco and y-websocket unless this PR adds the Yjs binding that consumes them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@firestore.rules`:
- Around line 19-20: Replace the broad authenticated-user read rule for room
documents with a participant-only condition, and ensure private room credentials
and server-only hash fields are never exposed through that document. Update the
room-creation flow to write the join-time metadata (`name` and
`passwordProtected`) from the server into the existing `rooms/{roomId}/meta`
subcollection, preserving the narrower metadata read contract used for
inspection before joining.
In `@server/routes/rooms.js`:
- Around line 171-191: Update the POST /create handler to authenticate the
Firebase ID token using the existing verification pattern, derive the creator
UID from the verified token instead of req.body.createdBy, and apply the same
rate limiter used by /verify-password. Replace the rooms document set with merge
behavior by using create() so existing room IDs are never modified, and map the
ALREADY_EXISTS failure to HTTP 409.
- Around line 178-193: Update the room creation handler around the db guard to
return an HTTP 503 error response immediately when db is unavailable. Only
execute the rooms document write and return the existing 200 success response
when db is configured, preventing clients from treating an unpersisted room as
successfully created.
In `@server/services/socketService.js`:
- Around line 18-32: Secure the Socket.IO flow around the connection handler and
its join-room, code-update, stdin-update, roles-update, and chat-message
handlers: add io.use() Firebase ID-token verification, store the verified uid
and identity on socket.data, and require the client token from
src/hooks/useRoom.js with connect_error refresh handling. In join-room, load the
room document, require the verified uid in participantIds, and derive the role
from roles[uid] instead of payload values; reject unauthorized joins. Enforce
that resolved role for code, stdin, and role mutations/broadcasts, and construct
chat identity from the verified token rather than client-supplied
uid/displayName.
- Around line 167-178: Update the Firestore persistence block in the room-empty
handling flow to use a merge-enabled set operation so missing room documents are
created while preserving existing fields. Include a fresh Firestore Timestamp in
updatedAt, importing Timestamp from firebase-admin/firestore, and refresh this
field periodically for active rooms—not only when they become empty—so
roomCleanupService does not remove live sessions.
In `@src/hooks/useRoom.js`:
- Around line 219-234: Update the synchronization logic in useRoom’s
code-update, room-state, stdin-update handlers, and push useEffect to track when
code or stdin values were applied remotely; consume that marker in the effect to
skip emitting the corresponding remote-applied value while preserving local
edits and existing debounce behavior.
---
Outside diff comments:
In `@src/components/Editor/EditorPage.jsx`:
- Around line 1943-1961: Update both ChatPanel instances in EditorPage so each
receives key={room.roomId}, ensuring switching rooms remounts ChatPanel and
resets its local messages state.
---
Major comments:
In `@eslint.config.js`:
- Around line 24-30: Update the flat ESLint configuration after the existing
config entry with a later rule targeting src/hooks/__tests__ test files, and set
languageOptions.globals.vitest for that pattern so direct describe, it, expect,
and vi usage is recognized.
In `@firestore.rules`:
- Around line 56-58: Update the Firestore rules for the affected subcollections
around the `allow read/create/update/delete` statements so access requires room
participation, creation binds the document to its author using the field names
written by `useWebRTC.js` and the whiteboard component, and deletion is limited
to that author or the intended recipient. Apply the same restrictions to the
additional affected rule block.
- Around line 11-14: Remove the `allow read: if true` rule from the
`/users/{userId}` match block, preserving the existing owner-only access rule.
Update signup display-name uniqueness checks to use the
`displayNames/{normalizedName}` collection or a server-side Admin SDK lookup
rather than reading user documents publicly.
In `@server/middleware/rateLimiter.js`:
- Around line 4-5: Update getIpKey to derive the key from the client IP rather
than passing the request object to ipKeyGenerator: use req.ip as the fallback
input and normalize it through ipKeyGenerator(req.ip), preserving the actual
client-IP-based limiter bucket.
In `@server/services/judge0Service.js`:
- Around line 3-29: Update getJudge0Config so configured non-RapidAPI envUrl
values are preserved instead of falling back to the public Judge0 endpoint. Add
a custom-URL branch after the RapidAPI handling, returning envUrl with the
required submissions?wait=true path and JSON headers, while retaining the
existing RapidAPI and default public endpoint behavior.
In `@server/services/socketService.js`:
- Around line 5-7: Make room persistence durable by periodically flushing each
activeRooms entry when room.code differs from lastSavedCode, including code,
language, stdin, and updatedAt, then update lastSavedCode after a successful
write. Add a SIGTERM shutdown handler that flushes all rooms before exit, and
configure the Socket.IO Redis adapter with shared room-state storage so users
connected to different instances observe the same edits.
- Around line 149-182: Update the room membership flow around join-room and the
disconnect handler to track socket IDs per active user, adding and removing
socket.id from each user’s sockets Set and removing the user only when that Set
becomes empty. Before switching rooms, remove the socket from its previous
room’s membership and Socket.IO room so rejoining cannot leave stale presence or
subscriptions; preserve the existing empty-room persistence and cleanup only
after the last member is gone.
- Around line 94-114: Cap incoming code and stdin payloads before storing or
broadcasting them: define and reuse byte-size limits, including the proposed
MAX_STDIN_BYTES guard, and reject or truncate values exceeding the limits.
Update both the code-update and stdin-update handlers in the socket service, and
configure the Socket.IO server’s maxHttpBufferSize to enforce the
transport-level limit while preserving normal room updates.
In `@src/components/Auth/AuthModal.jsx`:
- Around line 75-86: Update the username uniqueness-check catch block in
AuthModal’s signup flow to fail closed: after logging the lookup error, stop
signup, clear the loading state, and surface an appropriate error instead of
continuing to create the account. Preserve the existing duplicate-name rejection
behavior and ensure createUserWithEmailAndPassword runs only when the lookup
completes successfully.
In `@src/components/Chat/ChatPanel.jsx`:
- Around line 46-66: Restore chat persistence by updating the server’s
chat-message relay in socketService to write each message to the room’s
Firestore messages subcollection before or alongside broadcasting it. In
ChatPanel’s mount effect, load recent messages for roomId from Firestore,
initialize messages with that history, and retain the existing socket listener
for new-message updates and deduplication.
In `@src/hooks/useRoom.js`:
- Around line 136-147: Update the socket join flow around the connect handler
and the effect at line 185 so room membership is emitted again when myRole
changes after roomData loads. Keep the initial connection behavior, but add a
role-reactive join-room emission using the existing payload and socket state,
ensuring the server receives the authoritative resolved role instead of
retaining the initial viewer value.
- Around line 356-361: Update the Socket.IO presence-update handling so the
server persists the current active-user count in the room document, preserving
the existing activeUsers field shape consumed by castVote and the
deadlock-resolution effect. Ensure the write occurs whenever presence changes
and that vote creation and quorum checks read this fresh server-derived value
instead of defaulting to an empty list.
- Around line 246-296: Update createRoom to derive a password hash and fresh
salt when trimmedPassword is present, then include passwordHash and passwordSalt
in the server room-creation payload over HTTPS; do not rely on the client-side
setDoc for these protected fields, since they must be written through the Admin
SDK. Ensure password-protected rooms persist both materials and unprotected
rooms retain the existing behavior.
In `@src/hooks/useWebRTC.js`:
- Around line 100-116: Update the signal handling in useWebRTC’s onSnapshot
callback to consume or deduplicate each signal document before creating a peer
or calling peer.signal(). Ensure documents replayed as added on listener
startup, including stale pre-disconnect signals, are ignored after prior
processing while allowing each current call-session signal to be applied once.
In `@vite.config.js`:
- Around line 20-26: Update the build configuration’s rolldownOptions by
removing the external entry for monaco-editor/esm/vs/editor/editor.api so Monaco
is bundled for browser production builds; if peer dependency resolution still
fails, add monaco-editor directly to the build inputs or dependencies using the
existing project convention.
---
Minor comments:
In `@package.json`:
- Line 11: Extend the lint script to lint server files alongside src while
excluding the React JSX/client pattern, and add a later ESLint configuration
block targeting server/**/*.js with CommonJS source type and Node globals.
Preserve the existing client/React configuration for src and ensure the server
override takes precedence.
In `@server/services/judge0Service.js`:
- Around line 6-7: Update the ESLint configuration for server-side JavaScript
files to include the Node globals environment, ensuring process in
judge0Service.js and other server files is recognized and no longer triggers
no-undef. Preserve the existing browser and ES2020 globals configuration for
non-server files.
In `@server/tests/cors.test.js`:
- Around line 15-18: Import the Vitest globals used by
server/tests/cors.test.js—describe, it, and expect—before the existing describe
block so the test file passes lint without relying on implicit globals.
In `@src/components/Auth/AuthModal.jsx`:
- Around line 77-79: Normalize the display name once in the AuthModal submission
flow, then reuse that trimmed value for the users query, updateProfile call, and
saveUser/displayNameLower derivation. Update the relevant variable and
references near getDocs and updateProfile so inputs such as surrounding
whitespace produce one canonical stored and queried name.
In `@src/components/Chat/ChatPanel.jsx`:
- Around line 80-81: Update the message-id assignment in ChatPanel to use
crypto.randomUUID() instead of the Math.random-based value, preserving the
resulting id’s use as the React key and deduplication key.
In `@src/hooks/useRoom.js`:
- Around line 173-178: Update the presence-update handling in useRoom so
remoteCursors is pruned against the current presence list, removing entries for
users who are no longer present while preserving active cursor entries. Keep the
existing cursor-move updates in the newSocket handler unchanged.
---
Nitpick comments:
In `@server/package.json`:
- Around line 30-31: Remove the unused Yjs dependencies: in server/package.json
lines 30-31, remove y-websocket and yjs; in package.json lines 50-54, remove
y-monaco and y-websocket unless this PR adds the Yjs binding that consumes them.
In `@server/routes/webhooks.js`:
- Around line 186-189: Add regression coverage for the no-destination branch in
the room-event webhook handler, asserting HTTP 200 and the complete response
body including success, empty dispatched results, and the configuration message.
Verify callers do not depend on HTTP 503 for missing integrations, while
preserving a separate test that configured destination failures return HTTP 502.
In `@server/services/judge0Service.js`:
- Around line 86-102: Add a warning immediately before the Judge0
response-handling block falls through when data.status.id is undefined, clearly
indicating that Judge0 returned an unexpected response payload and execution is
falling back to Wandbox. Keep the existing catch warning for request failures
unchanged.
- Around line 125-130: Update the maintenance error thrown in the
isWandboxRuntimeFailure branch to include a machine-readable retryable marker,
such as statusCode 503 or retryable true, while preserving the existing message
and next(err) propagation through execute.js.
In `@src/hooks/useRoom.js`:
- Around line 119-127: Remove the early-return branch in the useRoom effect that
checks !roomId or !user, including its socket disconnect and state updates, so
the effect no longer performs redundant setState calls or reads stale socket
state; rely on the existing cleanup for disconnect and reset behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f139569-5efd-4b1a-9c2a-b854363b3ebd
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonserver/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.gitignoreeslint.config.jsfirestore.rulespackage.jsonserver/middleware/rateLimiter.jsserver/package.jsonserver/routes/execute.jsserver/routes/rooms.jsserver/routes/webhooks.jsserver/server.jsserver/services/firebaseAdmin.jsserver/services/judge0Service.jsserver/services/roomCleanupService.jsserver/services/socketService.jsserver/tests/cors.test.jssrc/components/Auth/AuthModal.jsxsrc/components/Chat/ChatPanel.jsxsrc/components/Editor/EditorPage.jsxsrc/hooks/useRoom.jssrc/hooks/useWebRTC.jsvite.config.jsvitest.config.js
| // Allow any authenticated user to read room documents (needed to inspect/join rooms) | ||
| allow read: if request.auth != null; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Room reads now expose passwordHash, passwordSalt, and private room code.
Firestore rules are not filters. allow read: if request.auth != null grants every signed-in user the full room document, including passwordHash and passwordSalt.
The write rules on Lines 24-25, 30, 34, and 38 block clients from writing those fields, and server/routes/rooms.js Line 75 calls them "server-only hash fields". This read rule contradicts that design. Any signed-in user can now export the hash and salt of every room and run an offline attack against the scrypt hash.
The same rule also exposes code for every private room, so a user who does not know the password can read the room contents and bypass the password gate.
Restrict document reads to participants, and move the fields needed for "inspect before join" into the existing rooms/{roomId}/meta subcollection (Lines 95-98), which already has a narrower contract.
🔒 Proposed direction
- // Allow any authenticated user to read room documents (needed to inspect/join rooms)
- allow read: if request.auth != null;
+ // Participants only. Non-participants read rooms/{roomId}/meta to inspect a room
+ // before joining. Password hash/salt must never reach a client.
+ allow read: if request.auth != null
+ && (
+ request.auth.uid == resource.data.createdBy
+ || request.auth.uid in resource.data.participantIds
+ );Write the join-time metadata (name, passwordProtected) to rooms/{roomId}/meta from the server when the room is created.
📝 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.
| // Allow any authenticated user to read room documents (needed to inspect/join rooms) | |
| allow read: if request.auth != null; | |
| // Participants only. Non-participants read rooms/{roomId}/meta to inspect a room | |
| // before joining. Password hash/salt must never reach a client. | |
| allow read: if request.auth != null | |
| && ( | |
| request.auth.uid == resource.data.createdBy | |
| || request.auth.uid in resource.data.participantIds | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@firestore.rules` around lines 19 - 20, Replace the broad authenticated-user
read rule for room documents with a participant-only condition, and ensure
private room credentials and server-only hash fields are never exposed through
that document. Update the room-creation flow to write the join-time metadata
(`name` and `passwordProtected`) from the server into the existing
`rooms/{roomId}/meta` subcollection, preserving the narrower metadata read
contract used for inspection before joining.
| router.post('/create', async (req, res) => { | ||
| const { roomId, name, createdBy, isPrivate, passwordProtected, code, language } = req.body; | ||
| if (!roomId || !createdBy) { | ||
| return res.status(400).json({ error: 'roomId and createdBy are required.' }); | ||
| } | ||
|
|
||
| try { | ||
| if (db) { | ||
| const now = new Date(); | ||
| await db.collection('rooms').doc(roomId.trim()).set({ | ||
| name: name || `Room ${roomId}`, | ||
| createdBy, | ||
| isPrivate: Boolean(isPrivate), | ||
| passwordProtected: Boolean(passwordProtected), | ||
| code: code || '', | ||
| language: language || 'python', | ||
| participantIds: [createdBy], | ||
| roles: { [createdBy]: 'host' }, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }, { merge: true }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Authenticate POST /create and stop merging into existing rooms.
This endpoint has no authentication and takes createdBy from the request body. It writes with the Admin SDK, so firestore.rules does not apply. The comment on Line 169 states this explicitly.
An attacker can send an existing roomId with their own createdBy. Because the write uses { merge: true }, the request replaces createdBy, roles, and participantIds, and it resets isPrivate and passwordProtected. The attacker becomes host of any room and can remove password protection. It also overwrites code and language, which destroys the collaborators' work.
Required changes:
- Verify a Firebase ID token and derive
createdByfrom the verified uid. - Use
create()instead ofset(..., { merge: true })so an existing room is never modified. - Apply a rate limiter, as
/verify-passworddoes.
🔒 Proposed fix
-router.post('/create', async (req, res) => {
- const { roomId, name, createdBy, isPrivate, passwordProtected, code, language } = req.body;
- if (!roomId || !createdBy) {
- return res.status(400).json({ error: 'roomId and createdBy are required.' });
- }
+router.post('/create', roomCreateLimiter, async (req, res) => {
+ const { roomId, name, isPrivate, passwordProtected, code, language } = req.body;
+ if (!roomId || typeof roomId !== 'string' || !roomId.trim()) {
+ return res.status(400).json({ error: 'roomId is required.' });
+ }
+
+ // Derive the owner from a verified ID token, never from the request body.
+ const idToken = (req.headers.authorization || '').replace(/^Bearer\s+/i, '');
+ if (!idToken) {
+ return res.status(401).json({ error: 'Authentication required.' });
+ }
+ let createdBy;
+ try {
+ const decoded = await getAuth(getApp()).verifyIdToken(idToken);
+ createdBy = decoded.uid;
+ } catch {
+ return res.status(401).json({ error: 'Invalid credentials.' });
+ }
try {
- if (db) {
- const now = new Date();
- await db.collection('rooms').doc(roomId.trim()).set({
+ const now = new Date();
+ await db.collection('rooms').doc(roomId.trim()).create({
name: name || `Room ${roomId}`,
createdBy,
isPrivate: Boolean(isPrivate),
passwordProtected: Boolean(passwordProtected),
code: code || '',
language: language || 'python',
participantIds: [createdBy],
roles: { [createdBy]: 'host' },
createdAt: now,
updatedAt: now,
- }, { merge: true });
- }
+ });create() rejects with ALREADY_EXISTS. Map that error to a 409 response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/routes/rooms.js` around lines 171 - 191, Update the POST /create
handler to authenticate the Firebase ID token using the existing verification
pattern, derive the creator UID from the verified token instead of
req.body.createdBy, and apply the same rate limiter used by /verify-password.
Replace the rooms document set with merge behavior by using create() so existing
room IDs are never modified, and map the ALREADY_EXISTS failure to HTTP 409.
| if (db) { | ||
| const now = new Date(); | ||
| await db.collection('rooms').doc(roomId.trim()).set({ | ||
| name: name || `Room ${roomId}`, | ||
| createdBy, | ||
| isPrivate: Boolean(isPrivate), | ||
| passwordProtected: Boolean(passwordProtected), | ||
| code: code || '', | ||
| language: language || 'python', | ||
| participantIds: [createdBy], | ||
| roles: { [createdBy]: 'host' }, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| }, { merge: true }); | ||
| } | ||
| return res.status(200).json({ success: true, roomId }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Return an error when db is not configured.
If Firebase Admin has no credentials, db is null, the if (db) block is skipped, and the handler still returns 200 { success: true }. The client fallback in src/hooks/useRoom.js (Lines 288-299) treats that response as success, calls setRoomId(id), and stores the id in localStorage. The user then joins a room that does not exist, and the subsequent Firestore snapshot never resolves.
Return 503 when db is unavailable.
🐛 Proposed fix
try {
- if (db) {
+ if (!db) {
+ console.error('[rooms] create room requested but Firestore is not configured');
+ return res.status(503).json({ error: 'Room service unavailable.' });
+ }
const now = new Date();
- await db.collection('rooms').doc(roomId.trim()).set({
+ await db.collection('rooms').doc(roomId.trim()).set({
...
- }, { merge: true });
- }
+ }, { merge: true });
return res.status(200).json({ success: true, roomId });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/routes/rooms.js` around lines 178 - 193, Update the room creation
handler around the db guard to return an HTTP 503 error response immediately
when db is unavailable. Only execute the rooms document write and return the
existing 200 success response when db is configured, preventing clients from
treating an unpersisted room as successfully created.
| io.on('connection', (socket) => { | ||
| let currentRoomId = null; | ||
| let currentUser = null; | ||
|
|
||
| logger.info(`[Socket] Client connected: ${socket.id}`); | ||
|
|
||
| // ─── Join Room ──────────────────────────────────────────────────────────── | ||
| socket.on('join-room', async ({ roomId, user, role }) => { | ||
| if (!roomId || !user) return; | ||
|
|
||
| currentRoomId = roomId; | ||
| currentUser = user; | ||
| socket.join(roomId); | ||
|
|
||
| logger.info(`[Socket] User ${user.displayName} (${user.uid}) joined room ${roomId}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Add authentication and authorization to the Socket.IO server.
There is no io.use() middleware and no token verification. Every handler trusts the client payload. This defeats the room password flow and the Firestore role model.
Concrete consequences:
- Line 26 accepts any
roomIdand anyuserobject. A client that knows a room id joins the room and receivesroom-statewith the fullcode(Lines 85-90) plus every latercode-update,stdin-update, andchat-message. The password check inserver/routes/rooms.jsis never consulted, so it is client-enforced only. - Line 71 takes
rolefrom the client. A viewer declares itselfhost. - Lines 94 and 106 apply
codeandstdinwith no role check.firestore.rulesLine 38 forbids viewers from writingcode, but this path bypasses the rules and then persists the result with Admin credentials at Line 169. The read-only guarantee is void. - Line 142 relays the client-supplied
uidanddisplayNameverbatim. Any client can impersonate another user in chat. - Line 136 broadcasts an arbitrary
rolesmap with no ownership check.
Verify a Firebase ID token in a connection middleware, derive uid from the token, and resolve the role from the Firestore room document instead of the payload.
🔒 Proposed direction
+const { getAuth } = require('firebase-admin/auth');
+
function initSocketServer(server, allowedOrigins) {
const io = new Server(server, { cors: { ... } });
+ // Reject unauthenticated connections before any handler runs.
+ io.use(async (socket, next) => {
+ try {
+ const token = socket.handshake.auth?.token;
+ if (!token) return next(new Error('unauthorized'));
+ const decoded = await getAuth().verifyIdToken(token);
+ socket.data.uid = decoded.uid;
+ socket.data.email = decoded.email;
+ return next();
+ } catch {
+ return next(new Error('unauthorized'));
+ }
+ });
+
io.on('connection', (socket) => {Then in join-room, load the room document, confirm socket.data.uid is in participantIds, and read the role from roles[socket.data.uid]. Reject the join otherwise. In code-update, stdin-update, and roles-update, check the resolved role before mutating or broadcasting. In chat-message, replace message.uid and message.displayName with the verified identity.
The client must pass the token. In src/hooks/useRoom.js Line 130, add auth: { token: await auth.currentUser.getIdToken() } and refresh it on connect_error.
Also applies to: 71-71, 94-114, 136-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/services/socketService.js` around lines 18 - 32, Secure the Socket.IO
flow around the connection handler and its join-room, code-update, stdin-update,
roles-update, and chat-message handlers: add io.use() Firebase ID-token
verification, store the verified uid and identity on socket.data, and require
the client token from src/hooks/useRoom.js with connect_error refresh handling.
In join-room, load the room document, require the verified uid in
participantIds, and derive the role from roles[uid] instead of payload values;
reject unauthorized joins. Enforce that resolved role for code, stdin, and role
mutations/broadcasts, and construct chat identity from the verified token rather
than client-supplied uid/displayName.
| try { | ||
| if (firestoreDb) { | ||
| await firestoreDb.collection('rooms').doc(roomId).update({ | ||
| code: room.code, | ||
| language: room.language, | ||
| stdinValue: room.stdin, | ||
| }); | ||
| logger.info(`[Socket] Room ${roomId} successfully saved to Firestore.`); | ||
| } | ||
| } catch (err) { | ||
| logger.error(`[Socket] Failed to save empty room ${roomId} to Firestore: ${err.message}`); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
update() rejects when the room document is missing, and updatedAt is never written.
Two defects in this persistence block:
-
Line 169 calls
update(). The Admin SDK rejectsupdate()withNOT_FOUNDif the document does not exist.join-roomaccepts anyroomId, and Lines 43-49 tolerate a missing document by seeding empty state. When such a room empties, the write rejects, the catch at Line 176 only logs, and the session code is lost. Useset(..., { merge: true }). -
The write omits
updatedAt.server/services/roomCleanupService.jsLine 94 queriesroomswhereupdatedAt < cutoffand then callsrecursiveDeleteon each match. A room that is actively used through Socket.IO never refreshesupdatedAt, so the cleanup job deletes live rooms and their subcollections afterROOM_STALE_HOURS. That is silent data loss.
🐛 Proposed fix
if (firestoreDb) {
- await firestoreDb.collection('rooms').doc(roomId).update({
- code: room.code,
- language: room.language,
- stdinValue: room.stdin,
- });
+ await firestoreDb.collection('rooms').doc(roomId).set(
+ {
+ code: room.code,
+ language: room.language,
+ stdinValue: room.stdin,
+ updatedAt: Timestamp.now(),
+ },
+ { merge: true }
+ );
logger.info(`[Socket] Room ${roomId} successfully saved to Firestore.`);
}Add const { Timestamp } = require('firebase-admin/firestore'); at the top. Also refresh updatedAt periodically while the room is active, not only when it empties.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/services/socketService.js` around lines 167 - 178, Update the
Firestore persistence block in the room-empty handling flow to use a
merge-enabled set operation so missing room documents are created while
preserving existing fields. Include a fresh Firestore Timestamp in updatedAt,
importing Timestamp from firebase-admin/firestore, and refresh this field
periodically for active rooms—not only when they become empty—so
roomCleanupService does not remove live sessions.
| useEffect(() => { | ||
| if (!roomId || !user || !roomData) return; | ||
| if (!roomId || !user || !socket || !isConnected) return; | ||
| if (!isEditor) return; | ||
| const timer = setTimeout(() => { | ||
| updateDoc(doc(db, 'rooms', roomId), { | ||
| socket.emit('code-update', { | ||
| roomId, | ||
| code, | ||
| language, | ||
| }); | ||
| socket.emit('stdin-update', { | ||
| roomId, | ||
| stdin: stdinValue, | ||
| _lastEditor: user.uid, | ||
| updatedAt: serverTimestamp(), | ||
| }).catch(() => {}); | ||
| }); | ||
| }, 300); | ||
| return () => clearTimeout(timer); | ||
| }, [code, language, stdinValue, roomId, user, isEditor, roomData]); | ||
| }, [code, language, stdinValue, roomId, user, isEditor, socket, isConnected]); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔴 Critical | 🏗️ Heavy lift
Remote code updates re-emit themselves, creating an echo loop.
The code-update handler at Line 164 calls setCode(rCode). code is a dependency of this push effect (Line 234). So every remote update schedules a local emit of the same content 300 ms later.
The server stores that emit at socketService.js Line 98 and re-broadcasts it to all other clients, whose handlers call setCode and emit again. One keystroke by one user produces one emit from every other member of the room. The amplification grows with the room size and does not settle while anyone keeps typing.
The room-state handler at Line 153 causes the same effect on join: a user who joins immediately echoes the whole room code back to the server.
Track whether the current value came from a remote apply, and skip the emit in that case.
🐛 Proposed fix
+ const applyingRemoteRef = useRef(false); newSocket.on('room-state', ({ code: rCode, language: rLang, stdin: rStdin, activeUsers: rUsers }) => {
+ applyingRemoteRef.current = true;
if (rCode !== undefined) setCode(rCode);
...
});
newSocket.on('code-update', ({ code: rCode, language: rLang }) => {
+ applyingRemoteRef.current = true;
if (rCode !== undefined) setCode(rCode);
if (rLang) setLanguage(rLang);
}); useEffect(() => {
if (!roomId || !user || !socket || !isConnected) return;
if (!isEditor) return;
+ if (applyingRemoteRef.current) {
+ // This render came from a remote update. Do not send it back.
+ applyingRemoteRef.current = false;
+ return;
+ }
const timer = setTimeout(() => {Apply the same guard in the stdin-update handler at Line 169.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useRoom.js` around lines 219 - 234, Update the synchronization
logic in useRoom’s code-update, room-state, stdin-update handlers, and push
useEffect to track when code or stdin values were applied remotely; consume that
marker in the effect to skip emitting the corresponding remote-applied value
while preserving local edits and existing debounce behavior.
1 participant
✦ Description
Please include a clear and concise summary of the changes made and the problem solved.
Fixes # (issue number)
⟡ Type of Change
Please delete options that are not relevant:
✦ Checklist
npm run lintandnpm run formatlocally before pushing.⟡ Screenshots / Screen Recordings (Required for UI changes)
Please provide Before and After screenshots or a short GIF showcasing the visual changes.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
Checklist:
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests