-
Notifications
You must be signed in to change notification settings - Fork 79
Implemented Team Debates with Elo Averaging, Team Channels, and Fair Turn Management #116
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
Implemented Team Debates with Elo Averaging, Team Channels, and Fair Turn Management #116
Conversation
- Introduced WebRTC signaling methods for offer, answer, and ICE candidate handling in TeamWebsocket. - Improved user data fetching and caching in useUser hook, ensuring user profile is hydrated from localStorage. - Updated TeamBuilder and TeamDebateRoom components to utilize the new user data structure. - Enhanced error handling and logging for WebSocket connections and media access. - Cleaned up unused imports and optimized component rendering logic for better performance.
|
Caution Review failedThe pull request is closed. WalkthroughThis PR introduces team-based debate functionality with WebRTC signaling, enhanced authentication middleware with database timeouts, new team data models with custom JSON marshaling, and comprehensive WebSocket messaging for real-time team interactions. It includes user profile caching, frontend media handling, and ICE candidate management, though the branch contains unresolved merge conflict markers in backend team controllers and frontend TeamBuilder. Changes
Sequence DiagramsequenceDiagram
participant Client as Frontend Client
participant WebSocket as WebSocket Server
participant DB as Database
participant Peer as RTCPeerConnection
Client->>WebSocket: Join Debate Room (debateId, token)
activate WebSocket
WebSocket->>DB: FindOne(debateId, 5s timeout)
DB-->>WebSocket: Debate loaded
WebSocket->>WebSocket: Create/load room resources<br/>(turn manager, token bucket)
WebSocket-->>Client: State Sync (phase, members)
deactivate WebSocket
Client->>Client: Acquire media (getUserMedia)
Client->>Peer: Create RTCPeerConnection
activate Peer
Client->>Peer: Add local stream tracks
Client->>WebSocket: Ready signal
activate WebSocket
WebSocket->>WebSocket: Update team ready count
WebSocket-->>Client: Broadcast ready state
deactivate WebSocket
Client->>Peer: Create Offer (SDP)
Client->>WebSocket: Send Offer (targetUserId)
activate WebSocket
WebSocket->>WebSocket: Route to target client
WebSocket-->>Client: Relay Offer
deactivate WebSocket
Client->>Peer: Create Answer (SDP)
Client->>WebSocket: Send Answer (targetUserId)
activate WebSocket
WebSocket-->>Client: Relay Answer
deactivate WebSocket
Peer->>Peer: ICE Candidate generation
Client->>WebSocket: Send Candidate (targetUserId)
activate WebSocket
WebSocket-->>Client: Relay Candidate
deactivate WebSocket
Peer-->>Peer: Connect & stream
Client->>Client: Attach remote stream to video
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Specific areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/context/authContext.tsx (1)
75-99: Normalization logic should be extracted to a shared utility.This normalization pattern (building a
Userobject with extensive fallbacks) is repeated in:
- This function (lines 75-97)
loginfunction (lines 126-148)googleLoginfunction (lines 265-287)useUser.tsfetch effect (lines 52-87)As mentioned in the
useUser.tsreview, extracting this into a sharednormalizeUserDatautility function would eliminate duplication and ensure consistency across all authentication flows.frontend/src/Pages/OnlineDebateRoom.tsx (1)
462-515: Ensure the WebSocket effect trackscurrentUserLine 487’s fallback (and the new
liveTranscriptfilter) rely oncurrentUser, but the WebSocket setup effect still depends only onroomId. When the component mounts before the user context resolves, the entire effect capturescurrentUserasundefined, sofetchRoomParticipantsnever hits the “fallback to current user” branch,opponentUserstays null, and theliveTranscripthandler treats our own broadcasts as opponent updates. Because the effect never re-runs after the user loads, those handlers stay stuck with the stale value for the rest of the session. Please gate the effect untilcurrentUserexists and include it in the dependency list so the socket/participants bootstrap happens with the real user:-useEffect(() => { - const token = getAuthToken(); - if (!token || !roomId) return; +useEffect(() => { + const token = getAuthToken(); + if (!token || !roomId || !currentUser) return; ... -}, [roomId]); +}, [roomId, currentUser]);This ensures the handlers see the correct IDs and the new live transcript flow behaves as intended.
♻️ Duplicate comments (3)
frontend/src/context/authContext.tsx (3)
14-14: Consolidate USER_CACHE_KEY with useUser.ts.This constant is defined in both
useUser.ts(line 6) and here. See the comment onuseUser.tslines 6-10 for the recommendation to extract shared constants to a common module.
126-150: Same normalization duplication as noted in lines 75-99.This is another instance of the repeated normalization logic. Please refer to the review comment on lines 75-99.
265-289: Same normalization duplication as noted in lines 75-99.This is the third instance of the repeated normalization logic. The default
isVerified: truefor Google login is appropriate, but the overall pattern should still be extracted. Please refer to the review comment on lines 75-99.
🧹 Nitpick comments (9)
frontend/src/Pages/TournamentHub.tsx (1)
53-74: Form validation relies on HTMLrequiredattributes; consider adding explicit validation.The date input is marked
required(line 232), but the handleCreate handler only validates the name field. While browser-level validation prevents empty submissions on compliant browsers, consider adding explicit checks for date validation in the handler for robustness:const handleCreate = (e: FormEvent) => { e.preventDefault(); if (!name) { setError("Tournament name is required."); return; } + if (!date) { + setError("Tournament date is required."); + return; + }This ensures consistent validation across all environments and improves error messaging clarity.
frontend/src/Pages/StartDebate.tsx (1)
45-46: Consider removing potentially redundant whitespace expression.The
{" "}between the closing</span>and "Site!" may be unnecessary, as whitespace naturally exists between the closing tag and the following text in JSX.frontend/src/hooks/useUser.ts (1)
6-10: Consider extracting these constants to a shared module.The same default values (1500, 350, 0.06) are used as inline literals in
authContext.tsx(lines 80-82, 131-133, 270-272). To prevent drift and ensure consistency, consider moving these constants to a shared location (e.g.,src/constants/user.ts) and importing them in both files.frontend/src/components/MatchmakingPool.tsx (1)
1-47: Avoid state updates after unmount in the polling loop.Because the fetch runs every 5 seconds, it’s easy to navigate away while a request is still inflight. When that promise settles,
setPoolData/setLoadingwill fire on an unmounted component, triggering React’s “state update on unmounted component” warning and leaking work. Guard the setters with anisMountedflag (or abort the request) and flip it in the cleanup so the interval can still be reused elsewhere.-import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; @@ const MatchmakingPool: React.FC = () => { const [poolData, setPoolData] = useState<PoolData | null>(null); const [loading, setLoading] = useState(false); const [error, setError] = useState<string | null>(null); + const isMountedRef = useRef(true); const fetchPoolData = async () => { setLoading(true); setError(null); try { const data = await getMatchmakingPool(); - setPoolData(data); + if (isMountedRef.current) { + setPoolData(data); + } } catch (err: any) { - setError(err.message || 'Failed to fetch pool data'); + if (isMountedRef.current) { + setError(err.message || 'Failed to fetch pool data'); + } } finally { - setLoading(false); + if (isMountedRef.current) { + setLoading(false); + } } }; useEffect(() => { fetchPoolData(); // Auto-refresh every 5 seconds const interval = setInterval(fetchPoolData, 5000); - return () => clearInterval(interval); + return () => { + isMountedRef.current = false; + clearInterval(interval); + }; }, []);backend/websocket/team_websocket.go (5)
165-186: Remove commented-out code.The commented
isTeam1variable declarations (lines 165, 175, 185) are unused and should be removed to improve code clarity.var userTeamID primitive.ObjectID - // var isTeam1 bool // Check team 1 var team1 models.Team err = teamCollection.FindOne(context.Background(), bson.M{ "_id": debate.Team1ID, "members.userId": userObjectID, }).Decode(&team1) if err == nil { userTeamID = debate.Team1ID - // isTeam1 = true } else { // Check team 2 var team2 models.Team err = teamCollection.FindOne(context.Background(), bson.M{ "_id": debate.Team2ID, "members.userId": userObjectID, }).Decode(&team2) if err == nil { userTeamID = debate.Team2ID - // isTeam1 = false }
260-260: Magic number for initial tokens.The hardcoded value
10for initial tokens should be a named constant to improve maintainability.At the package level, define:
const ( InitialTokensPerUser = 10 CountdownDurationSeconds = 3 )Then use:
- Tokens: 10, // Initial tokens + Tokens: InitialTokensPerUser,
698-888: Consider refactoring this complex function.This 190-line function handles multiple concerns: validation, state updates, broadcasting, and async phase transitions. Consider extracting smaller functions like
validateAndAssignReadyStatus,broadcastReadyUpdate, andstartCountdownIfReady.
714-821: Excessive logging in production code.The numerous log statements (lines 714, 715, 740, 745, 748, 782, 798, 802, 803, 809, 818) will create high log volume in production. Consider using debug-level logging or reducing verbosity.
104-105: Consider implementing idle room cleanup.Rooms are only deleted when all clients disconnect (line 336), but there's no mechanism to clean up rooms that become idle or abandoned. This could lead to memory leaks over time.
Implement a periodic cleanup goroutine that removes rooms with no activity:
func init() { go func() { ticker := time.NewTicker(5 * time.Minute) defer ticker.Stop() for range ticker.C { cleanupIdleRooms() } }() } func cleanupIdleRooms() { teamRoomsMutex.Lock() defer teamRoomsMutex.Unlock() now := time.Now() for key, room := range teamRooms { room.Mutex.Lock() allIdle := true for _, client := range room.Clients { if now.Sub(client.LastActivity) < 10*time.Minute { allIdle = false break } } room.Mutex.Unlock() if allIdle && len(room.Clients) == 0 { delete(teamRooms, key) log.Printf("Cleaned up idle room: %s", key) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
backend/cmd/server/main.go(1 hunks)backend/controllers/profile_controller.go(1 hunks)backend/controllers/team_controller.go(1 hunks)backend/controllers/team_debate_controller.go(1 hunks)backend/controllers/team_matchmaking.go(1 hunks)backend/db/db.go(1 hunks)backend/middlewares/auth.go(2 hunks)backend/models/team.go(1 hunks)backend/routes/team.go(1 hunks)backend/services/team_matchmaking.go(1 hunks)backend/services/team_turn_service.go(1 hunks)backend/websocket/matchmaking.go(2 hunks)backend/websocket/team_debate_handler.go(1 hunks)backend/websocket/team_websocket.go(1 hunks)backend/websocket/websocket.go(3 hunks)frontend/package.json(2 hunks)frontend/src/App.tsx(4 hunks)frontend/src/Pages/About.tsx(5 hunks)frontend/src/Pages/DebateRoom.tsx(1 hunks)frontend/src/Pages/Game.tsx(6 hunks)frontend/src/Pages/MatchLogs.tsx(5 hunks)frontend/src/Pages/OnlineDebateRoom.tsx(15 hunks)frontend/src/Pages/StartDebate.tsx(2 hunks)frontend/src/Pages/TeamBuilder.tsx(1 hunks)frontend/src/Pages/TeamDebateRoom.tsx(1 hunks)frontend/src/Pages/TournamentHub.tsx(9 hunks)frontend/src/components/ChatRoom.tsx(6 hunks)frontend/src/components/Layout.tsx(1 hunks)frontend/src/components/Matchmaking.tsx(6 hunks)frontend/src/components/MatchmakingPool.tsx(1 hunks)frontend/src/components/Sidebar.tsx(1 hunks)frontend/src/components/SpeechTranscripts.tsx(2 hunks)frontend/src/components/TeamChatSidebar.tsx(1 hunks)frontend/src/components/TeamMatchmaking.tsx(1 hunks)frontend/src/context/authContext.tsx(9 hunks)frontend/src/hooks/useUser.ts(1 hunks)frontend/src/services/teamDebateService.ts(1 hunks)frontend/src/services/teamService.ts(1 hunks)frontend/tsconfig.app.tsbuildinfo(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (25)
backend/cmd/server/main.go (2)
backend/routes/team.go (4)
SetupTeamRoutes(10-27)SetupTeamDebateRoutes(30-37)SetupTeamChatRoutes(51-53)SetupTeamMatchmakingRoutes(40-48)backend/websocket/team_websocket.go (1)
TeamWebsocketHandler(108-415)
frontend/src/components/MatchmakingPool.tsx (2)
backend/services/matchmaking.go (1)
MatchmakingPool(17-26)frontend/src/services/teamDebateService.ts (1)
getMatchmakingPool(117-130)
backend/websocket/matchmaking.go (1)
backend/services/matchmaking.go (1)
GetMatchmakingService(40-49)
backend/websocket/websocket.go (1)
backend/structs/websocket.go (2)
Room(30-37)Message(10-13)
backend/controllers/team_debate_controller.go (3)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (2)
Team(11-22)TeamDebate(61-81)backend/services/team_matchmaking.go (1)
RemoveFromMatchmaking(115-122)
backend/websocket/team_debate_handler.go (2)
backend/models/team.go (2)
TeamDebateMessage(84-95)TeamDebate(61-81)backend/db/db.go (1)
GetCollection(21-23)
frontend/src/components/TeamMatchmaking.tsx (3)
backend/models/team.go (1)
Team(11-22)frontend/src/services/teamService.ts (2)
Team(4-15)createTeamDebate(158-176)frontend/src/services/teamDebateService.ts (5)
getActiveTeamDebate(71-84)getMatchmakingStatus(132-145)joinMatchmaking(87-100)leaveMatchmaking(102-115)createTeamDebate(31-52)
frontend/src/hooks/useUser.ts (2)
frontend/src/state/userAtom.ts (1)
userAtom(4-4)frontend/src/context/authContext.tsx (1)
AuthContext(35-37)
frontend/src/components/Matchmaking.tsx (2)
backend/websocket/matchmaking.go (1)
MatchmakingMessage(49-57)backend/services/matchmaking.go (1)
MatchmakingPool(17-26)
frontend/src/services/teamDebateService.ts (3)
frontend/src/utils/auth.ts (1)
getAuthToken(5-7)backend/models/team.go (1)
TeamDebate(61-81)frontend/src/services/teamService.ts (3)
TeamDebate(26-40)createTeamDebate(158-176)getTeamDebate(179-192)
frontend/src/context/authContext.tsx (2)
frontend/src/types/user.ts (1)
User(1-21)backend/models/user.go (1)
User(10-30)
frontend/src/Pages/TeamDebateRoom.tsx (7)
backend/models/team.go (1)
TeamMember(25-32)frontend/src/services/teamService.ts (2)
TeamMember(17-24)getTeamDebate(179-192)backend/websocket/team_debate_handler.go (1)
TeamDebateRoom(32-36)frontend/src/state/userAtom.ts (1)
userAtom(4-4)frontend/src/hooks/useUser.ts (1)
useUser(12-112)frontend/src/utils/auth.ts (1)
getAuthToken(5-7)frontend/src/services/teamDebateService.ts (1)
getTeamDebate(55-68)
backend/services/team_turn_service.go (3)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (1)
Team(11-22)frontend/src/services/teamService.ts (1)
Team(4-15)
backend/models/team.go (3)
frontend/src/services/teamService.ts (3)
TeamMember(17-24)Team(4-15)TeamDebate(26-40)frontend/src/services/teamDebateService.ts (1)
TeamDebate(8-28)backend/websocket/team_debate_handler.go (1)
TeamDebateMessage(38-47)
backend/routes/team.go (4)
backend/controllers/team_controller.go (11)
CreateTeam(175-246)GetTeam(249-270)JoinTeam(273-355)LeaveTeam(358-416)RemoveMember(447-518)UpdateTeamName(49-103)UpdateTeamSize(106-172)GetTeamByCode(30-46)GetTeamMemberProfile(562-588)GetUserTeams(419-444)GetAvailableTeams(591-609)backend/controllers/team_debate_controller.go (3)
CreateTeamDebate(18-94)GetTeamDebate(97-114)GetActiveTeamDebate(117-146)backend/controllers/team_matchmaking.go (4)
JoinMatchmaking(17-69)LeaveMatchmaking(72-82)GetMatchmakingStatus(109-129)GetMatchmakingPool(85-106)backend/services/team_matchmaking.go (1)
GetMatchmakingPool(125-133)
backend/controllers/team_controller.go (3)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (2)
Team(11-22)TeamMember(25-32)frontend/src/services/teamService.ts (2)
Team(4-15)TeamMember(17-24)
frontend/src/App.tsx (1)
backend/websocket/team_debate_handler.go (1)
TeamDebateRoom(32-36)
frontend/src/components/ChatRoom.tsx (2)
backend/websocket/websocket.go (1)
Message(63-86)backend/structs/websocket.go (1)
Message(10-13)
backend/middlewares/auth.go (3)
backend/config/config.go (1)
LoadConfig(53-65)backend/models/user.go (1)
User(10-30)backend/db/db.go (1)
MongoDatabase(17-17)
backend/controllers/team_matchmaking.go (3)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (1)
Team(11-22)backend/services/team_matchmaking.go (4)
StartTeamMatchmaking(31-62)RemoveFromMatchmaking(115-122)GetMatchmakingPool(125-133)FindMatchingTeam(65-112)
frontend/src/Pages/TeamBuilder.tsx (3)
backend/models/team.go (2)
TeamMember(25-32)Team(11-22)frontend/src/services/teamService.ts (13)
TeamMember(17-24)Team(4-15)getAvailableTeams(142-155)getUserTeams(126-139)createTeam(61-77)joinTeam(96-108)getTeamMemberProfile(308-321)removeMember(276-289)deleteTeam(292-305)updateTeamName(324-339)updateTeamSize(358-373)leaveTeam(111-123)getTeamByCode(342-355)frontend/src/hooks/useUser.ts (1)
useUser(12-112)
backend/services/team_matchmaking.go (3)
backend/models/team.go (1)
Team(11-22)backend/db/db.go (1)
GetCollection(21-23)backend/controllers/team_matchmaking.go (1)
GetMatchmakingPool(85-106)
frontend/src/services/teamService.ts (3)
backend/models/team.go (3)
Team(11-22)TeamMember(25-32)TeamDebate(61-81)frontend/src/services/teamDebateService.ts (3)
TeamDebate(8-28)createTeamDebate(31-52)getTeamDebate(55-68)frontend/src/utils/auth.ts (1)
getAuthToken(5-7)
frontend/src/components/TeamChatSidebar.tsx (4)
frontend/src/components/ui/button.tsx (1)
Button(57-57)frontend/src/components/ui/scroll-area.tsx (1)
ScrollArea(46-46)frontend/src/components/ui/avatar.tsx (3)
Avatar(48-48)AvatarImage(48-48)AvatarFallback(48-48)frontend/src/components/ui/input.tsx (1)
Input(25-25)
backend/websocket/team_websocket.go (4)
backend/services/team_turn_service.go (5)
TeamTurnManager(132-136)TokenBucket(22-28)TokenBucketService(16-19)NewTeamTurnManager(139-144)NewTokenBucketService(31-35)backend/utils/auth.go (1)
ValidateTokenAndFetchEmail(131-144)backend/db/db.go (1)
MongoDatabase(17-17)backend/models/team.go (2)
TeamDebate(61-81)Team(11-22)
🪛 Biome (2.1.2)
frontend/src/Pages/TeamDebateRoom.tsx
[error] 809-810: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 810-813: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 813-814: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 814-815: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 892-893: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 916-917: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 917-917: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 918-919: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 949-950: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 950-952: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 970-970: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 970-973: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 973-976: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 976-979: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 979-983: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1001-1002: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (23)
frontend/src/Pages/About.tsx (1)
1-121: Solid presentational updates to the About page.All changes are cosmetic improvements that enhance text clarity and readability. The content rewordings make the About page more engaging and informative, and the formatting adjustments (including the semicolon on the export statement) promote consistency. No behavioral or structural issues.
frontend/src/Pages/TournamentHub.tsx (3)
95-97: New handleViewBracket utility is well-implemented.The function follows the same navigation pattern as handleJoin and correctly passes the tournament state. The addition of spectate functionality alongside join is a clean UX enhancement.
148-155: Spectate button properly integrated with accessibility support.The new Eye icon button has a descriptive aria-label and clean styling. Placement alongside tournament metadata is logical. The button calls handleViewBracket consistently.
76-93: Verify navigation route consistency between handleJoin and handleViewBracket.Both handlers navigate to
/tournament/{id}/bracketbut with different intent: one joins (increments participants), the other spectates (view only). Ensure the bracket view component correctly handles these two cases and that route parameter handling is consistent.frontend/src/Pages/StartDebate.tsx (1)
1-1: LGTM: Modern JSX runtime usage.Removing the default React import aligns with the new JSX transform introduced in React 17+, where React no longer needs to be in scope for JSX to work.
frontend/package.json (1)
26-26: Unused dependencies detected: react-dnd packages added but not yet imported or used in the codebase.The three dependencies (
react-dnd,react-dnd-html5-backend, and@types/react-dnd) were added tofrontend/package.jsonbut searches across the entire frontend source directory found zero imports or usages, including in the newTeamBuilder.tsxandTeamDebateRoom.tsxfiles. Verify this is intentional—either implement drag-and-drop functionality using these packages or remove the premature dependencies.frontend/src/Pages/MatchLogs.tsx (1)
121-126: LGTM: Formatting changes look good.The JSX restructuring and formatting updates improve code readability without altering functionality.
Also applies to: 136-139, 158-160, 174-194, 201-217, 233-235
frontend/src/hooks/useUser.ts (2)
17-30: LGTM: Clean hydration with proper error handling.The localStorage hydration logic correctly guards against missing or invalid cached data and removes corrupted entries.
107-110: Verify token presence from localStorage doesn't bypass validation.The logic checks for tokens in both
authContextandlocalStorage, which provides resilience across page refreshes. However, ensure that using a token fromlocalStoragedoesn't bypassauthContext's token validation flow, as the context may have already invalidated the token.frontend/src/context/authContext.tsx (2)
193-198: LGTM: Proper functional state update with localStorage sync.The functional update pattern correctly handles the case where
previs null and ensures the atom and localStorage remain synchronized after email verification.
299-305: LGTM: Complete cleanup on logout.The logout flow correctly clears the token, cached user profile, and userAtom state, ensuring no stale data persists.
frontend/src/Pages/Game.tsx (1)
167-179: LGTM: Functional updater pattern implemented correctly.The functional updater pattern for
setCameraOnandsetMicOnis well-implemented, allowing both direct values and functional updates. This provides flexibility for thePlayerCardcomponent.frontend/src/components/Layout.tsx (1)
2-20: LGTM! Consistent formatting applied.The changes standardize string quotes to single quotes throughout the file, improving code consistency. No functional changes.
backend/websocket/matchmaking.go (2)
192-199: LGTM! Enhanced error handling and user feedback.The improved error handling now provides explicit feedback to clients when matchmaking operations fail or succeed. The conditional flow ensures clients receive either an error or a success confirmation with pool status.
205-206: Good addition of confirmation message.Sending an explicit
matchmaking_stoppedconfirmation improves the user experience by providing clear feedback when leaving the matchmaking pool.backend/middlewares/auth.go (1)
21-74: Verify impact of increased logging on production performance.Multiple log statements have been added throughout the authentication flow. While helpful for debugging, ensure these won't impact production performance or create excessive log volume.
Consider using conditional logging based on environment or log level configuration.
frontend/src/App.tsx (2)
65-65: LGTM! Team builder route properly configured.The
/team-builderroute is correctly placed under the protected Layout, ensuring users have access to the sidebar and header navigation.
88-88: Team debate room route is outside Layout wrapper.The
/team-debate/:debateIdroute is protected but renders outside the<Layout />component, meaning it won't have the standard sidebar and header. This appears intentional for an immersive debate experience.Verify that this standalone layout is the desired UX for team debates, as users won't have quick access to navigation while in a debate room.
frontend/src/components/TeamChatSidebar.tsx (1)
83-95: LGTM! Message sending logic is solid.The send handler properly validates connection state and message content before sending. The cleanup of the input field after sending is also correct.
backend/websocket/team_websocket.go (4)
59-71: LGTM! Thread-safe write implementation.The mutex-protected write methods properly prevent concurrent writes to the WebSocket connection, which would otherwise cause panics or data corruption.
1067-1125: LGTM! WebRTC signaling handlers are well-implemented.The offer, answer, and candidate handlers properly validate required fields, construct payloads with necessary metadata, and use the safe
sendMessageToUserhelper to relay WebRTC signaling messages to target peers with appropriate error logging.
417-474: LGTM! Helper functions properly implement thread safety.The helper functions use appropriate synchronization:
snapshotTeamRecipientscorrectly locks and copies clients to avoid holding locks during I/OfindClientByUserIDproperly locks during search- Broadcast functions handle errors gracefully without stopping iteration
226-226: Remove this review comment—the concerns are unfounded.The
upgraderis properly defined inbackend/websocket/websocket.go(same package, accessible via package-scoped variable), and error handling is already in place at line 227. The code correctly validates the WebSocket upgrade, logs errors, and handles team assignment validation with proper cleanup.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
♻️ Duplicate comments (5)
backend/websocket/team_websocket.go (5)
344-365: Missing read deadline allows connection exhaustion.The read loop (line 345) has no timeout, allowing slow or malicious clients to hold connections indefinitely. This was flagged in past reviews and remains unfixed.
Set a read deadline before each read:
// Listen for messages for { + conn.SetReadDeadline(time.Now().Add(60 * time.Second)) messageType, msg, err := conn.ReadMessage() if err != nil {Also add a pong handler to detect dead connections:
conn.SetPongHandler(func(string) error { conn.SetReadDeadline(time.Now().Add(60 * time.Second)) return nil })
422-429: Default case broadcasts unvalidated messages.The default branch (lines 422-428) relays unknown message types to all clients without validation, which is a security risk. This issue was flagged in past reviews and remains unfixed.
Drop unknown messages instead of broadcasting:
default: - // Broadcast the message to all other clients in the room - for _, r := range snapshotTeamRecipients(room, conn) { - if err := r.SafeWriteMessage(messageType, msg); err != nil { - log.Printf("Team WebSocket write error in room %s: %v", roomKey, err) - } - } + log.Printf("Unknown message type received: %s from user %s", message.Type, client.UserID.Hex()) + client.SafeWriteJSON(map[string]interface{}{ + "type": "error", + "error": "Unknown message type", + }) }
511-540: Missing content length validation in chat handler.
message.Content(line 531) is not validated for length, allowing clients to send large payloads that consume memory and bandwidth. This was flagged in past reviews.Add validation before processing:
func handleTeamChatMessage(room *TeamRoom, conn *websocket.Conn, message TeamMessage, client *TeamClient, roomKey string) { + const maxContentLength = 10000 // 10KB limit + if len(message.Content) > maxContentLength { + client.SafeWriteJSON(map[string]interface{}{ + "type": "error", + "error": "Message content too large", + }) + return + } + // Add timestamp if not providedApply similar validation to
handleTeamDebateMessage,handleTeamSpeechText, andhandleTeamLiveTranscript.
865-898: Goroutine captures room pointer without verifying map entry.The countdown goroutine (lines 865-898) captures the
roompointer but doesn't verify it matches the map entry after the sleep (line 869). If the room is replaced inteamRooms[roomKey], the goroutine operates on a stale instance. This issue was flagged in past reviews.Verify the room instance matches the map entry:
go func() { time.Sleep(3 * time.Second) teamRoomsMutex.Lock() - room, stillExists := teamRooms[roomKey] + currentRoom, stillExists := teamRooms[roomKey] teamRoomsMutex.Unlock() - if !stillExists { + if !stillExists || currentRoom != room { log.Printf("[handleTeamReadyStatus] Room %s no longer exists, aborting phase change", roomKey) return } - room.Mutex.Lock() - if room.CurrentPhase == "countdown" || room.CurrentPhase == "setup" { - room.CurrentPhase = "openingFor" + currentRoom.Mutex.Lock() + if currentRoom.CurrentPhase == "countdown" || currentRoom.CurrentPhase == "setup" { + currentRoom.CurrentPhase = "openingFor"
1045-1076: Same goroutine race in handleCheckStart.The goroutine at lines 1045-1076 has the identical race condition as the one in
handleTeamReadyStatus. Apply the same fix.
🧹 Nitpick comments (3)
frontend/src/Pages/Game.tsx (1)
35-39: Consider stricter typing for WebSocket messages.The current
GameWebSocketMessagetype uses a loose index signature that accepts any property. Consider defining a discriminated union of specific message types for better type safety:type GameWebSocketMessage = | { type: "DEBATE_START" } | { type: "DEBATE_END" } | { type: "TURN_START"; content: string } | { type: "TURN_END" } | { type: "CHAT_MESSAGE"; content: string } | { type: "GENERATING_TRANSCRIPT"; content: string } | { type: "GAME_RESULT"; content: string };This would enable exhaustive type checking in the switch statement and catch missing cases at compile time.
backend/controllers/team_controller.go (2)
19-27: Consider seeding the random generator.The
generateTeamCodefunction usesmath/randwithout visible seeding. While team codes don't require cryptographic strength, ensurerand.Seed()is called during application initialization (e.g., inmain()) to avoid generating identical sequences across restarts.
338-348: Optimize the order of the team-full check.The maxSize check (lines 343-346) occurs after summing Elo (lines 339-342), wasting computation when the team is already full. Move the check immediately after fetching the team (line 332) for efficiency.
Apply this diff:
// First, get current team to calculate new average var team models.Team err = collection.FindOne(context.Background(), bson.M{"_id": objectID}).Decode(&team) if err != nil { c.JSON(http.StatusNotFound, gin.H{"error": "Team not found"}) return } + if len(team.Members) >= team.MaxSize { + c.JSON(http.StatusBadRequest, gin.H{"error": "Team is already full"}) + return + } + // Calculate new average Elo totalElo := 0.0 for _, member := range team.Members { totalElo += member.Elo } - if len(team.Members) >= team.MaxSize { - c.JSON(http.StatusBadRequest, gin.H{"error": "Team is already full"}) - return - } totalElo += newMember.Elo
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/controllers/team_controller.go(1 hunks)backend/controllers/team_matchmaking.go(1 hunks)backend/middlewares/auth.go(2 hunks)backend/models/team.go(1 hunks)backend/services/team_matchmaking.go(1 hunks)backend/services/team_turn_service.go(1 hunks)backend/websocket/team_debate_handler.go(1 hunks)backend/websocket/team_websocket.go(1 hunks)frontend/src/Pages/Game.tsx(6 hunks)frontend/src/Pages/MatchLogs.tsx(5 hunks)frontend/src/Pages/TeamBuilder.tsx(1 hunks)frontend/src/components/TeamMatchmaking.tsx(1 hunks)frontend/src/services/teamDebateService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/components/TeamMatchmaking.tsx
- frontend/src/Pages/MatchLogs.tsx
- frontend/src/services/teamDebateService.ts
- backend/websocket/team_debate_handler.go
🧰 Additional context used
🧬 Code graph analysis (9)
frontend/src/Pages/TeamBuilder.tsx (3)
backend/models/team.go (2)
TeamMember(25-32)Team(11-22)frontend/src/services/teamService.ts (13)
TeamMember(17-24)Team(4-15)getAvailableTeams(142-155)getUserTeams(126-139)createTeam(61-77)joinTeam(96-108)getTeamMemberProfile(308-321)removeMember(276-289)deleteTeam(292-305)updateTeamName(324-339)updateTeamSize(358-373)leaveTeam(111-123)getTeamByCode(342-355)frontend/src/hooks/useUser.ts (1)
useUser(12-112)
backend/services/team_turn_service.go (2)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (1)
Team(11-22)
backend/middlewares/auth.go (3)
backend/config/config.go (1)
LoadConfig(53-65)backend/models/user.go (1)
User(10-30)backend/db/db.go (1)
MongoDatabase(17-17)
backend/models/team.go (3)
frontend/src/services/teamService.ts (3)
TeamMember(17-24)Team(4-15)TeamDebate(26-40)frontend/src/services/teamDebateService.ts (1)
TeamDebate(8-28)backend/websocket/team_debate_handler.go (1)
TeamDebateMessage(39-48)
frontend/src/Pages/Game.tsx (1)
frontend/src/components/Chatbox.tsx (1)
ChatMessage(5-11)
backend/controllers/team_controller.go (3)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (2)
Team(11-22)TeamMember(25-32)frontend/src/services/teamService.ts (2)
Team(4-15)TeamMember(17-24)
backend/controllers/team_matchmaking.go (3)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (1)
Team(11-22)backend/services/team_matchmaking.go (4)
StartTeamMatchmaking(31-62)RemoveFromMatchmaking(115-122)GetMatchmakingPool(125-138)FindMatchingTeam(65-112)
backend/websocket/team_websocket.go (4)
backend/services/team_turn_service.go (5)
TeamTurnManager(132-136)TokenBucket(22-28)TokenBucketService(16-19)NewTeamTurnManager(139-144)NewTokenBucketService(31-35)backend/utils/auth.go (1)
ValidateTokenAndFetchEmail(131-144)backend/db/db.go (1)
MongoDatabase(17-17)backend/models/team.go (2)
TeamDebate(61-81)Team(11-22)
backend/services/team_matchmaking.go (3)
backend/models/team.go (1)
Team(11-22)backend/db/db.go (1)
GetCollection(21-23)backend/controllers/team_matchmaking.go (1)
GetMatchmakingPool(104-125)
🔇 Additional comments (9)
frontend/src/Pages/Game.tsx (3)
112-112: LGTM! Strict equality operator correctly used.The comparison now uses
===instead of the previously flagged==, preventing type coercion issues.
140-140: LGTM! Dependencies correctly specified.Including
handleWebSocketMessagein the dependency array is correct since it's used in the WebSocket message handler and is properly memoized.
192-203: LGTM! Functional updates properly supported.The setState calls correctly handle both direct values and updater functions, providing flexibility and preventing stale closure issues.
backend/services/team_turn_service.go (3)
91-109: GetRemainingTokens lock upgrade looks correct.The past review concern about mutating under
RLockhas been addressed. The method now acquires a full write lock (line 102) before callingrefillTokens, which mutatesTokensandLastRefill. This prevents data races.
16-62: In-memory bucket storage trades persistence for simplicity.Token buckets are stored in-memory (line 17) and will reset on server restart. This is acceptable for ephemeral debate sessions but means ongoing debates lose token state during deployments.
If persistence is needed later, consider storing token state in MongoDB or Redis with TTL expiration.
53-58: Hardcoded token parameters are reasonable defaults.Capacity (10 tokens), refill rate (1/sec), and initial tokens (10) are hardcoded. These values appear reasonable for the debate use case.
If different debate formats require varying token budgets, consider making these configurable per team or debate type.
backend/websocket/team_websocket.go (2)
121-129: Config path fix looks good.The hardcoded config path issue from past reviews has been addressed. The code now uses
os.Getenv("CONFIG_PATH")with a sensible fallback (lines 121-124).
196-239: Room initialization outside lock is correct.The room preparation (turn manager, token bucket initialization) now occurs outside the global mutex (lines 196-228), with only the map insertion synchronized (lines 231-239). This prevents blocking concurrent connections.
backend/controllers/team_controller.go (1)
596-618: GetAvailableTeams query is correctly implemented.Verification confirms:
- Team model has
MaxSizefield (bson-tagged as "maxSize")- MongoDB Go driver fully supports
$exprwith field references- Query correctly filters teams where
len(members) < maxSizeCode is approved; no changes needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/middlewares/auth.go (1)
64-79: Error handling is correct, but timeout is excessive.The code now properly distinguishes between "user not found" (401) and database failures (500), which addresses a previous major issue.
However, the 5-second timeout is too long for a request-critical path. Database queries should complete in milliseconds under normal conditions. A 5-second timeout could make failing requests very slow.
Reduce the timeout to a more reasonable value:
- dbCtx, cancel := context.WithTimeout(c.Request.Context(), 5*time.Second) + dbCtx, cancel := context.WithTimeout(c.Request.Context(), 500*time.Millisecond)
Note: The performance concern about database lookups on every authenticated request (raised in previous reviews) remains unaddressed. Consider implementing caching as previously suggested.
backend/controllers/team_matchmaking.go (1)
133-153: Add authentication to prevent unauthorized matchmaking queries.This endpoint allows any caller to query the matchmaking status of any team without authentication. Additionally, line 150 returns the full
matchingTeamobject, which may include sensitive member details (emails, etc.) not necessary for matchmaking status.Apply this diff to add authentication:
func GetMatchmakingStatus(c *gin.Context) { teamID := c.Param("teamId") objectID, err := primitive.ObjectIDFromHex(teamID) if err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid team ID"}) return } + _, exists := c.Get("userID") + if !exists { + c.JSON(http.StatusUnauthorized, gin.H{"error": "User not authenticated"}) + return + } + // Try to find a match matchingTeam, err := services.FindMatchingTeam(objectID)Consider also limiting the response to essential fields:
c.JSON(http.StatusOK, gin.H{ "matched": true, - "team": matchingTeam, + "teamId": matchingTeam.ID.Hex(), + "teamName": matchingTeam.Name, "matchId": matchingTeam.ID.Hex(), })
🧹 Nitpick comments (2)
backend/middlewares/auth.go (1)
23-23: Consider log level or conditional logging.Logging every authenticated request will generate significant log volume in production. Consider using a debug log level or making this conditional on a debug flag.
Example:
- log.Printf("AuthMiddleware called for path: %s", c.Request.URL.Path) + // Uncomment for debugging: + // log.Printf("AuthMiddleware called for path: %s", c.Request.URL.Path)backend/services/team_matchmaking.go (1)
17-28: Consider adding TTL or cleanup for stale matchmaking entries.The in-memory pool has no expiration mechanism. Teams that join matchmaking but never match (or whose clients disconnect without calling
LeaveMatchmaking) will remain in the pool indefinitely, potentially causing memory growth over time.Consider adding a TTL field and a cleanup goroutine:
type TeamMatchmakingEntry struct { TeamID primitive.ObjectID Team models.Team MaxSize int AverageElo float64 Timestamp time.Time ExpiresAt time.Time // Add expiration } // Add cleanup goroutine in init func init() { go func() { ticker := time.NewTicker(5 * time.Minute) defer ticker.Stop() for range ticker.C { cleanupExpiredEntries() } }() } func cleanupExpiredEntries() { teamMatchmakingMutex.Lock() defer teamMatchmakingMutex.Unlock() now := time.Now() for teamID, entry := range teamMatchmakingPool { if now.After(entry.ExpiresAt) { delete(teamMatchmakingPool, teamID) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/controllers/team_matchmaking.go(1 hunks)backend/middlewares/auth.go(2 hunks)backend/services/team_matchmaking.go(1 hunks)frontend/src/Pages/Game.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
backend/controllers/team_matchmaking.go (3)
backend/db/db.go (1)
GetCollection(21-23)backend/models/team.go (1)
Team(11-22)backend/services/team_matchmaking.go (4)
StartTeamMatchmaking(31-64)RemoveFromMatchmaking(117-124)GetMatchmakingPool(127-140)FindMatchingTeam(67-114)
backend/middlewares/auth.go (3)
backend/config/config.go (1)
LoadConfig(53-65)backend/models/user.go (1)
User(10-30)backend/db/db.go (1)
MongoDatabase(17-17)
backend/services/team_matchmaking.go (3)
backend/models/team.go (1)
Team(11-22)backend/db/db.go (1)
GetCollection(21-23)backend/controllers/team_matchmaking.go (1)
GetMatchmakingPool(109-130)
frontend/src/Pages/Game.tsx (2)
backend/websocket/websocket.go (1)
TypingIndicator(88-94)frontend/src/components/Chatbox.tsx (2)
TypingIndicator(13-19)ChatMessage(5-11)
🔇 Additional comments (10)
backend/middlewares/auth.go (4)
5-18: Imports look appropriate.All new imports (db, models, context, errors, log, time, bson, mongo) are necessary for the added authentication and database lookup functionality.
27-27: Error logging is well-placed.Logging configuration failures, missing authorization headers, and JWT validation errors provides a good audit trail for debugging and security monitoring.
Also applies to: 35-35, 50-50
56-62: Safe type assertion correctly implemented.The comma-ok idiom properly validates the "sub" claim type and presence, preventing the panic risk from the previous unsafe assertion. This addresses the critical issue flagged in earlier reviews.
81-86: User context propagation looks correct.The middleware properly enriches the request context with authenticated user details (email, userID, displayName, avatarUrl, rating) for downstream handlers. The field access and naming are consistent with the User model.
backend/controllers/team_matchmaking.go (2)
18-72: LGTM!The handler correctly validates the team ID, authenticates the user, verifies captain authorization, checks team readiness, and adds the team to matchmaking. The timeout for the MongoDB lookup is properly implemented.
75-106: LGTM!The handler now correctly authenticates the user, verifies captain authorization, and removes the team from matchmaking. The previous security concerns have been addressed.
backend/services/team_matchmaking.go (4)
31-64: Confirm re-joining behavior is intentional.The function correctly implements timeouts and locking. However, if a team is already in the matchmaking pool (lines 55-61), the existing entry is silently overwritten. Verify this is the intended behavior for teams re-joining matchmaking.
If re-joining should be prevented, consider adding a check:
teamMatchmakingMutex.Lock() defer teamMatchmakingMutex.Unlock() // Initialize pool if it doesn't exist if teamMatchmakingPool == nil { teamMatchmakingPool = make(map[string]*TeamMatchmakingEntry) } + if _, exists := teamMatchmakingPool[teamID.Hex()]; exists { + return fmt.Errorf("team already in matchmaking pool") + } + teamMatchmakingPool[teamID.Hex()] = &TeamMatchmakingEntry{
67-114: LGTM!The function correctly uses a read lock and returns early if the pool is nil (addressing the previous concurrent map write concern). The matchmaking logic properly filters by team size and Elo difference. Line 107 returns a pointer to the Team struct stored in the map entry, which is acceptable since entries are immutable after creation.
117-124: LGTM!The function correctly uses an exclusive lock and safely handles the nil pool case.
127-140: LGTM!The function now correctly returns a snapshot copy of the pool (lines 134-139), addressing the previous concurrent map iteration concern. The read lock ensures safe access during snapshot creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (5)
backend/websocket/team_websocket.go (5)
345-366: Read timeout still missing (previously flagged).The read loop continues to lack read deadlines, allowing clients to hold connections indefinitely without sending data. This was identified in a previous review but remains unaddressed.
As previously recommended, set a read deadline before each read:
for { + conn.SetReadDeadline(time.Now().Add(60 * time.Second)) messageType, msg, err := conn.ReadMessage()Also consider adding a pong handler to reset the deadline:
conn.SetPongHandler(func(string) error { conn.SetReadDeadline(time.Now().Add(60 * time.Second)) return nil })
423-430: Default case still broadcasts unvalidated messages (previously flagged).The default case continues to relay unknown message types without validation, which was identified as a security risk in the previous review. Clients can inject arbitrary data that will be broadcast to all participants.
As previously recommended, remove the default broadcast:
default: - // Broadcast the message to all other clients in the room - for _, r := range snapshotTeamRecipients(room, conn) { - if err := r.SafeWriteMessage(messageType, msg); err != nil { - log.Printf("Team WebSocket write error in room %s: %v", roomKey, err) - } - } + log.Printf("Unknown message type received: %s from user %s", message.Type, client.UserID.Hex()) + client.SafeWriteJSON(map[string]interface{}{ + "type": "error", + "error": "Unknown message type", + }) }
511-541: Content length validation still missing (previously flagged).Chat messages are not validated for length, allowing clients to send extremely large payloads. This issue was identified in the previous review but remains unaddressed.
As previously recommended, add validation:
func handleTeamChatMessage(room *TeamRoom, conn *websocket.Conn, message TeamMessage, client *TeamClient, roomKey string) { + const maxContentLength = 10000 // 10KB limit + if len(message.Content) > maxContentLength { + client.SafeWriteJSON(map[string]interface{}{ + "type": "error", + "error": "Message content too large", + }) + return + } + // Add timestamp if not providedApply similar validation to
handleTeamDebateMessage,handleTeamSpeechText, andhandleTeamLiveTranscript.
866-899: Countdown goroutine race condition still present (previously flagged).The goroutine captures the room pointer but after sleeping, fetches from the map without verifying it's the same instance. If the room is deleted and recreated with the same key, the goroutine may operate on a different room. This was flagged in the previous review but remains unaddressed.
As previously recommended, verify the room instance after the sleep:
go func() { time.Sleep(3 * time.Second) teamRoomsMutex.Lock() - room, stillExists := teamRooms[roomKey] + currentRoom, stillExists := teamRooms[roomKey] teamRoomsMutex.Unlock() - if !stillExists { + if !stillExists || currentRoom != room { log.Printf("[handleTeamReadyStatus] Room %s no longer exists, aborting phase change", roomKey) return } - room.Mutex.Lock() - if room.CurrentPhase == "countdown" || room.CurrentPhase == "setup" { - room.CurrentPhase = "openingFor" + currentRoom.Mutex.Lock() + if currentRoom.CurrentPhase == "countdown" || currentRoom.CurrentPhase == "setup" { + currentRoom.CurrentPhase = "openingFor"
1046-1077: Same countdown race condition in handleCheckStart (previously flagged).This goroutine has the identical race condition pattern as in
handleTeamReadyStatus. The room pointer may become stale after the sleep, but this isn't verified.Apply the same fix as suggested for
handleTeamReadyStatus:go func() { time.Sleep(3 * time.Second) teamRoomsMutex.Lock() - room, stillExists := teamRooms[roomKey] + currentRoom, stillExists := teamRooms[roomKey] teamRoomsMutex.Unlock() - if !stillExists { + if !stillExists || currentRoom != room { log.Printf("[handleCheckStart] Room %s no longer exists", roomKey) return } - room.Mutex.Lock() - if room.CurrentPhase == "countdown" || room.CurrentPhase == "setup" { - room.CurrentPhase = "openingFor" + currentRoom.Mutex.Lock() + if currentRoom.CurrentPhase == "countdown" || currentRoom.CurrentPhase == "setup" { + currentRoom.CurrentPhase = "openingFor"
🧹 Nitpick comments (1)
backend/websocket/team_websocket.go (1)
368-372: Unmarshal errors are silently ignored.When message parsing fails, the error is swallowed without logging or notifying the client. This makes debugging difficult and provides no feedback about malformed messages.
Add logging and optionally notify the client:
var message TeamMessage if err := json.Unmarshal(msg, &message); err != nil { + log.Printf("Failed to unmarshal message from user %s: %v", client.UserID.Hex(), err) + client.SafeWriteJSON(map[string]interface{}{ + "type": "error", + "error": "Invalid message format", + }) continue }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/websocket/team_websocket.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/websocket/team_websocket.go (4)
backend/services/team_turn_service.go (5)
TeamTurnManager(132-136)TokenBucket(22-28)TokenBucketService(16-19)NewTeamTurnManager(139-144)NewTokenBucketService(31-35)backend/utils/auth.go (1)
ValidateTokenAndFetchEmail(131-144)backend/db/db.go (1)
MongoDatabase(17-17)backend/models/team.go (2)
TeamDebate(61-81)Team(11-22)
🔇 Additional comments (2)
backend/websocket/team_websocket.go (2)
122-125: Config path now properly configurable.Good fix - the hardcoded config path issue from the previous review has been addressed. The code now reads from the
CONFIG_PATHenvironment variable with a sensible default fallback.
197-240: Room initialization properly moved outside critical section.Excellent improvement - the room initialization that was previously blocking all concurrent connections has been fixed. The turn manager, token bucket, and room structure are now prepared outside the mutex, and only the map lookup/insert holds the lock briefly. This significantly reduces contention for concurrent WebSocket connections.
| // SafeWriteJSON safely writes JSON data to the team client's WebSocket connection | ||
| func (tc *TeamClient) SafeWriteJSON(v any) error { | ||
| tc.writeMu.Lock() | ||
| defer tc.writeMu.Unlock() | ||
| return tc.Conn.WriteJSON(v) | ||
| } | ||
|
|
||
| // SafeWriteMessage safely writes raw WebSocket messages to the team client's connection | ||
| func (tc *TeamClient) SafeWriteMessage(messageType int, data []byte) error { | ||
| tc.writeMu.Lock() | ||
| defer tc.writeMu.Unlock() | ||
| return tc.Conn.WriteMessage(messageType, data) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add write deadlines to prevent slow client blocking.
The safe write methods don't set write deadlines before writing to the WebSocket connection. Slow or stalled clients can block these operations indefinitely, potentially exhausting server resources or causing cascading delays for other clients sharing the same room.
Set a write deadline before each write operation:
func (tc *TeamClient) SafeWriteJSON(v any) error {
tc.writeMu.Lock()
defer tc.writeMu.Unlock()
+ tc.Conn.SetWriteDeadline(time.Now().Add(10 * time.Second))
return tc.Conn.WriteJSON(v)
}
func (tc *TeamClient) SafeWriteMessage(messageType int, data []byte) error {
tc.writeMu.Lock()
defer tc.writeMu.Unlock()
+ tc.Conn.SetWriteDeadline(time.Now().Add(10 * time.Second))
return tc.Conn.WriteMessage(messageType, data)
}🤖 Prompt for AI Agents
In backend/websocket/team_websocket.go around lines 60 to 72, the SafeWriteJSON
and SafeWriteMessage methods lack write deadlines which can allow a slow/stalled
client to block writes indefinitely; before calling Conn.WriteJSON or
Conn.WriteMessage, call Conn.SetWriteDeadline(time.Now().Add(writeTimeout))
(e.g. 10s or a configurable value) and return any error from SetWriteDeadline,
then proceed to perform the write while still holding the mutex; ensure the
package imports time and that writeTimeout is defined or configurable, and keep
the existing locking/defer semantics.
| // Prepare room outside lock | ||
| roomKey := debateID | ||
| turnManager := services.NewTeamTurnManager() | ||
| tokenBucket := services.NewTokenBucketService() | ||
|
|
||
| // Initialize turn management for both teams | ||
| turnErr1 := turnManager.InitializeTeamTurns(debate.Team1ID) | ||
| turnErr2 := turnManager.InitializeTeamTurns(debate.Team2ID) | ||
|
|
||
| // Initialize token buckets for both teams | ||
| bucketErr1 := tokenBucket.InitializeTeamBuckets(debate.Team1ID) | ||
| bucketErr2 := tokenBucket.InitializeTeamBuckets(debate.Team2ID) | ||
|
|
||
| if turnErr1 != nil || turnErr2 != nil || bucketErr1 != nil || bucketErr2 != nil { | ||
| log.Printf("error initializing room resources: %v %v %v %v", turnErr1, turnErr2, bucketErr1, bucketErr2) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to initialize room resources"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential partial initialization if one team setup fails.
If initialization succeeds for team1 but fails for team2, the turnManager and tokenBucket services may retain partial state in their internal maps. While the HTTP error prevents connection upgrade, the service instances might have dangling entries.
Consider cleaning up on partial failure:
turnErr1 := turnManager.InitializeTeamTurns(debate.Team1ID)
turnErr2 := turnManager.InitializeTeamTurns(debate.Team2ID)
bucketErr1 := tokenBucket.InitializeTeamBuckets(debate.Team1ID)
bucketErr2 := tokenBucket.InitializeTeamBuckets(debate.Team2ID)
if turnErr1 != nil || turnErr2 != nil || bucketErr1 != nil || bucketErr2 != nil {
log.Printf("error initializing room resources: %v %v %v %v", turnErr1, turnErr2, bucketErr1, bucketErr2)
+ // Clean up any partially initialized state if services support cleanup
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to initialize room resources"})
return
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/websocket/team_websocket.go around lines 197 to 214, the current
logic can create partial state in turnManager and tokenBucket if initialization
succeeds for one team and fails for the other; update the code to track which
InitializeTeamTurns/InitializeTeamBuckets calls succeeded and, on any error,
roll back the successful initializations before returning (e.g., call the
services' RemoveTeam/Deinitialize methods or expose a cleanup API to delete
entries from their internal maps), ensure rollback is done under the same
locking/transactional context used for initialization to avoid races, and
surface a single combined error to the client after cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (8)
frontend/src/components/TeamMatchmaking.tsx (1)
124-152: ResetisSearchingstate to unblock UIAfter a successful join,
isSearchingremainstrueindefinitely, leaving the "Find Match" button disabled for the rest of the session. Similarly, leaving matchmaking doesn't clear the flag.Apply this diff to fix the state management:
const handleJoinMatchmaking = async () => { if (!isTeamFull) { alert('Team must be full to join matchmaking!'); return; } + setIsSearching(true); try { - setIsSearching(true); await joinMatchmaking(team.id); setHasJoined(true); } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : 'Failed to join matchmaking'; alert(errorMessage); + } finally { setIsSearching(false); } }; const handleLeaveMatchmaking = async () => { try { await leaveMatchmaking(team.id); setHasJoined(false); setMatchedTeam(null); + setIsSearching(false); } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : 'Failed to leave matchmaking'; alert(errorMessage); } };backend/controllers/team_matchmaking.go (1)
108-129: Secure the matchmaking pool endpoint.
GetMatchmakingPoolstill returns team names, captain IDs, Elo, and timestamps to any caller without validating authentication. That’s the same exposure highlighted earlier: it leaks sensitive roster data and lets anonymous clients inventory captains. Please gate this handler with the sameuserIDcheck you added elsewhere (or document that it’s intentionally public). (dota2.fandom.com)backend/websocket/team_websocket.go (6)
60-71: Add write deadlines before sending frames.
SafeWriteJSON/SafeWriteMessagestill write withoutSetWriteDeadline, so a slow client can block the goroutine forever. Prior review called this out; please set a reasonable deadline (e.g., 10 s) before each write. (dota2.fandom.com)
345-365: Enforce read timeouts / heartbeats.The read loop keeps waiting on
ReadMessage()with no deadline or pong handler. A stalled client can hold the connection (and room resources) indefinitely. Please reintroduce periodicSetReadDeadline+ pong handling (or a ping ticker) to shed dead sockets. (dota2.fandom.com)
865-899: Countdown goroutine can target the wrong room instance.After sleeping, the goroutine grabs
teamRooms[roomKey]and happily drivesroom.CurrentPhase; but if the original room was deleted and a new debate with the same key was created, you now mutate the new session’s state. Capture the original pointer and bail if the map entry differs before locking. (dota2.fandom.com)
1045-1077: Same stale-room bug inhandleCheckStart.The follow-up goroutine re-reads
teamRooms[roomKey]and mutates it without checking it’s the same instance. If the slot was recycled, you flip a fresh debate intoopeningFor. Please keep a reference to the originalroomand verify it hasn’t been replaced before applying phase changes. (dota2.fandom.com)
360-365: Broadcasting unknown message types is unsafe.The default branch still relays any payload to the whole room, so a malicious client can push arbitrary JSON to everyone. Drop the broadcast and instead log/reject unexpected message types. (dota2.fandom.com)
374-383: Validate message size before processing.User-controlled
message.Content,message.LiveTranscript, etc. go straight toSafeWriteJSONwithout a length guard. Attackers can stream MBs of data and exhaust memory/bandwidth. Please enforce a max payload (similar toconn.SetReadLimit+ manual checks). (dota2.fandom.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/controllers/team_controller.go(2 hunks)backend/controllers/team_matchmaking.go(3 hunks)backend/middlewares/auth.go(3 hunks)backend/services/team_matchmaking.go(3 hunks)backend/websocket/team_debate_handler.go(3 hunks)backend/websocket/team_websocket.go(36 hunks)frontend/src/Pages/TeamBuilder.tsx(4 hunks)frontend/src/components/TeamMatchmaking.tsx(3 hunks)frontend/src/services/teamDebateService.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/controllers/team_controller.go
- backend/websocket/team_debate_handler.go
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/Pages/TeamBuilder.tsx (1)
frontend/src/hooks/useUser.ts (1)
useUser(12-112)
backend/websocket/team_websocket.go (2)
backend/utils/auth.go (1)
ValidateTokenAndFetchEmail(128-141)backend/services/team_turn_service.go (3)
NewTeamTurnManager(139-144)NewTokenBucketService(31-35)TokenBucket(22-28)
frontend/src/components/TeamMatchmaking.tsx (2)
frontend/src/services/teamService.ts (1)
Team(5-16)frontend/src/services/teamDebateService.ts (2)
ActiveDebateSummary(42-47)getActiveTeamDebate(90-105)
frontend/src/services/teamDebateService.ts (2)
backend/models/team.go (1)
Team(11-22)frontend/src/services/teamService.ts (1)
Team(5-16)
🪛 Biome (2.1.2)
frontend/src/Pages/TeamBuilder.tsx
[error] 826-827: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
[error] 829-830: Expected a statement but instead found '>>>>>>> main'.
Expected a statement here.
(parse)
[error] 828-828: Shouldn't redeclare 'capacity'. Consider to delete it or rename it.
'capacity' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (4)
frontend/src/Pages/TeamBuilder.tsx (2)
20-20: LGTM: Clean migration to useUser hookThe replacement of Jotai's
useAtom(userAtom)with the newuseUser()hook is properly implemented.Also applies to: 83-83
634-645: LGTM: Proper user shape for TeamMatchmakingThe user object correctly provides the
BasicUsershape (id, email, displayName) that TeamMatchmaking expects, with appropriate null handling.frontend/src/components/TeamMatchmaking.tsx (1)
11-11: LGTM: Type system improvementsThe introduction of
BasicUserandActiveDebateSummarytypes creates a cleaner, lighter API surface for this component.Also applies to: 15-15, 31-31, 35-35, 75-75
frontend/src/services/teamDebateService.ts (1)
1-5: LGTM: Service layer improvementsThe changes properly address the previous review feedback:
- Environment-aware API base URL configuration
ActiveDebateSummarycorrectly represents the lightweight response shape fromGET /team-debates/team/:teamId/activegetActiveTeamDebatenow returns the accurate type instead of the fullTeamDebateTeamtype reference inMatchmakingStatusResponseeliminates duplicationAlso applies to: 42-47, 90-92, 171-171
| <<<<<<< HEAD | ||
| const capacity = team.maxSize || 4; | ||
| ======= | ||
| const capacity = | ||
| team.maxSize && team.maxSize > 0 ? team.maxSize : 4; | ||
| >>>>>>> main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve the merge conflict before merging
Unresolved Git conflict markers will cause parse errors and prevent the application from running.
Apply this diff to resolve the conflict by using the more robust capacity calculation:
-<<<<<<< HEAD
- const capacity = team.maxSize || 4;
-=======
- const capacity =
- team.maxSize && team.maxSize > 0 ? team.maxSize : 4;
->>>>>>> main
+ const capacity =
+ team.maxSize && team.maxSize > 0 ? team.maxSize : 4;📝 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.
| <<<<<<< HEAD | |
| const capacity = team.maxSize || 4; | |
| ======= | |
| const capacity = | |
| team.maxSize && team.maxSize > 0 ? team.maxSize : 4; | |
| >>>>>>> main | |
| const capacity = | |
| team.maxSize && team.maxSize > 0 ? team.maxSize : 4; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 826-827: Expected a statement but instead found '======='.
Expected a statement here.
(parse)
[error] 829-830: Expected a statement but instead found '>>>>>>> main'.
Expected a statement here.
(parse)
[error] 828-828: Shouldn't redeclare 'capacity'. Consider to delete it or rename it.
'capacity' is defined here:
(lint/suspicious/noRedeclare)
🤖 Prompt for AI Agents
In frontend/src/Pages/TeamBuilder.tsx around lines 825 to 830, remove the
unresolved Git conflict markers and replace the conflicting block with the
robust capacity calculation (use team.maxSize && team.maxSize > 0 ? team.maxSize
: 4) so capacity defaults to 4 when maxSize is absent or non-positive and no
merge markers remain.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.