Fixed the flow of user vs user debate, implemented voice to text in user vs user debate, added judging system after debate ends, with user elo rating change according to outcome #111
Conversation
…ents - Updated Go version to 1.24 and added toolchain specification. - Updated dependencies in go.mod and go.sum, including cloud.google.com/go to v0.116.0. - Added Redis configuration to the backend config. - Removed unnecessary log statements and improved code readability across various backend controllers and services. - Enhanced frontend components by adding new routes for viewing debates and integrating new libraries like fuse.js and zustand. - Cleaned up unused console logs and improved error handling in frontend components.
WalkthroughThis PR introduces Redis-backed team debate infrastructure with spectator support, rate-limited interactions, and streaming event persistence. It adds comprehensive team management (creation, matchmaking, member handling), extends the rating system for opponent tracking, and adds multiple WebSocket handlers for real-time debate collaboration across frontend and backend. Changes
Sequence Diagram(s)sequenceDiagram
participant Spectator as Spectator Client
participant WebSocket as WebSocket Handler
participant Hub as DebateHub
participant PollStore as PollStore
participant Redis as Redis
participant Consumer as StreamConsumer
Spectator->>WebSocket: Connect to /ws/debate/:debateID
WebSocket->>Hub: Register(debateID, conn, spectatorHash)
Hub->>PollStore: Load current poll state
PollStore->>Redis: Get poll data & voter counts
Hub->>Spectator: Send poll_snapshot + presence
Spectator->>WebSocket: Send vote message
WebSocket->>PollStore: Vote(debateID, pollID, option, hash)
PollStore->>Redis: SAdd voters, HIncrBy counts
WebSocket->>Hub: BroadcastToDebate(event)
Hub->>Spectator: Broadcast vote event to all clients
Hub->>Consumer: PublishEvent (via StreamConsumer)
Consumer->>Redis: XAdd to stream
Spectator->>WebSocket: Send question message
WebSocket->>WebSocket: checkQuestionRateLimit
WebSocket->>Redis: Check question count
WebSocket->>PollStore: recordQuestion
WebSocket->>Hub: BroadcastToDebate(question)
Hub->>Spectator: Broadcast question to all
Hub->>Consumer: PublishEvent
Consumer->>Redis: XAdd event to stream
sequenceDiagram
participant User as Team Captain
participant Frontend as Frontend App
participant Matchmaking as Matchmaking Service
participant Backend as Backend API
participant MongoDB as MongoDB
participant Pool as Matchmaking Pool
User->>Frontend: Click "Join Matchmaking"
Frontend->>Backend: POST /matchmaking/:teamId/join
Backend->>MongoDB: Verify team is full
Backend->>Matchmaking: StartTeamMatchmaking(teamID)
Matchmaking->>Pool: Add entry with MaxSize & Elo
Backend->>Frontend: 200 OK
par User1 Polling
Frontend->>Backend: GET /matchmaking/:teamId/status (every 3s)
Backend->>Matchmaking: FindMatchingTeam(teamID)
Matchmaking->>Pool: Scan for matching Elo & size
end
alt Match Found
Matchmaking->>Backend: Return matched team
Backend->>Frontend: { matched: true, opponent: Team }
Frontend->>User: "Match found! Opponent: ..."
User->>Frontend: Click "Create Debate"
Frontend->>Backend: POST /team-debates
Backend->>MongoDB: Create debate, assign stances
Backend->>Matchmaking: RemoveFromPool(team1), RemoveFromPool(team2)
Backend->>Frontend: { debateId, ... }
Frontend->>User: Redirect to /team-debate/:debateId
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
frontend/src/components/RoomBrowser.tsx (1)
21-52: Add error handling and user feedback infetchRooms.The empty catch block at lines 48-51 silently swallows all errors, making debugging difficult and providing no feedback when room fetching fails. This is inconsistent with
handleJoinMatch(lines 60-81), which properly alerts users on errors.Apply this diff to improve error handling:
} catch (error) { + console.error('Failed to fetch rooms:', error); + setRooms([]); } finally { setLoading(false); }Additionally, consider providing user feedback for the error cases at lines 31-34:
if (!response.ok) { + console.error('Failed to fetch rooms: HTTP', response.status); setLoading(false); return; }backend/utils/auth.go (1)
48-54: Major: Error information loss and no observability for security-critical operations.Removing logging and returning generic error messages ("failed to hash password", "failed to generate token", "failed to generate random token") makes production debugging extremely difficult. When authentication fails in production, operators have no visibility into the root cause.
Consider:
- Wrap the actual error instead of replacing it with a generic message.
- Add structured logging for these security-critical operations (even if just at ERROR level).
Apply these changes to preserve error context:
func HashPassword(password string) (string, error) { bytes, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) if err != nil { - return "", fmt.Errorf("failed to hash password") + return "", fmt.Errorf("failed to hash password: %w", err) } return string(bytes), nil }func GenerateJWTToken(userID, email string) (string, error) { // ... claims setup ... signedToken, err := token.SignedString(jwtSecret) if err != nil { - return "", fmt.Errorf("failed to generate token") + return "", fmt.Errorf("failed to generate token: %w", err) } return signedToken, nil }func GenerateRandomToken(length int) (string, error) { b := make([]byte, length) if _, err := rand.Read(b); err != nil { - return "", fmt.Errorf("failed to generate random token") + return "", fmt.Errorf("failed to generate random token: %w", err) } return base64.URLEncoding.EncodeToString(b), nil }Additionally, consider adding structured logging (using a logger like
zaporlogrus) for audit trails of authentication operations.Also applies to: 69-91, 119-125
frontend/src/components/ChatRoom.tsx (1)
146-160: Stop double-counting votes.
handleVoteinstantly incrementsvoteResults, but the WebSocketvoteevent also increments the same counter. When the server echoes a vote back to every client (including the voter), totals end up +2 for the same ballot. Drop the local increment and rely on the broadcast so that everyone—including the voter—sees consistent counts.Apply this diff:
if (wsRef.current) { wsRef.current.send(JSON.stringify({ type: 'vote', extra: { option } })); } - setVoteResults((prev) => ({ - ...prev, - [option]: (prev[option] || 0) + 1, - })); setUserVoted(true);frontend/src/hooks/useUser.ts (1)
45-46: Add error logging for debugging and observability.The empty catch block silently swallows all errors during user data fetching, making it difficult to diagnose authentication or API failures. This degrades both developer experience and system observability.
Apply this diff to restore error logging:
} catch (error) { + console.error('Failed to fetch user profile:', error); }backend/routes/rooms.go (2)
37-41: Fix concurrency issues and add collision detection.Multiple critical issues with room ID generation:
rand.Seedandrand.Intnuse the global rand source, which is not safe for concurrent use and can cause data races.- Seeding on every call is inefficient and can produce duplicate IDs if called in rapid succession.
- No uniqueness check means room ID collisions will overwrite existing rooms in the database.
Apply this diff to fix the concurrency issue:
+var rng = rand.New(rand.NewSource(time.Now().UnixNano())) +var rngMutex sync.Mutex + func generateRoomID() string { - rand.Seed(time.Now().UnixNano()) - return strconv.Itoa(rand.Intn(900000) + 100000) + rngMutex.Lock() + defer rngMutex.Unlock() + return strconv.Itoa(rng.Intn(900000) + 100000) }Additionally, implement collision detection in
CreateRoomHandlerby checking if the generated ID already exists before insertion, and retry with a new ID if necessary.
219-219: Add type assertion check to prevent panic.Same issue as in
JoinRoomHandler: the type assertionemail.(string)will panic if the email value is not a string.Apply this diff:
- err = userCollection.FindOne(ctx, bson.M{"email": email.(string)}).Decode(&user) + emailStr, ok := email.(string) + if !ok { + c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid email format"}) + return + } + err = userCollection.FindOne(ctx, bson.M{"email": emailStr}).Decode(&user)backend/websocket/matchmaking.go (1)
120-126: Ensure pool-join failures close the upgraded socket
Once the HTTP upgrade succeeds, responding withc.Stringis no longer valid, and we leave the upgraded connection registered inmatchmakingRoom.clientseven thoughAddToPoolfailed. The client never receives a proper WebSocket close/error frame, and the room map keeps a zombie entry. Please send an error over the WebSocket (or at least write a close frame), remove the client frommatchmakingRoom.clients, and close the socket before returning. A minimal fix looks like:- err = matchmakingService.AddToPool(user.ID.Hex(), user.DisplayName, userRating) - if err != nil { - c.String(http.StatusInternalServerError, "Failed to join matchmaking") - return - } + err = matchmakingService.AddToPool(user.ID.Hex(), user.DisplayName, userRating) + if err != nil { + matchmakingRoom.mutex.Lock() + delete(matchmakingRoom.clients, client) + matchmakingRoom.mutex.Unlock() + _ = conn.WriteMessage(websocket.CloseMessage, websocket.FormatCloseMessage(websocket.CloseInternalServerErr, "Failed to join matchmaking")) + conn.Close() + return + }
🧹 Nitpick comments (14)
backend/routes/leaderboard.go (1)
3-7: Minor: import grouping convention.The blank line separating local (
arguehub/controllers) and external (github.com/gin-gonic/gin) imports was removed. Go convention typically groups these separately. Consider restoring the blank line for clarity, though this is a minor style point.frontend/src/components/RoomBrowser.tsx (1)
24-24: Consider extracting the API base URL to configuration.The hardcoded localhost URL should be moved to an environment configuration file to support different environments (development, staging, production).
Example configuration approach:
// config.ts export const API_BASE_URL = import.meta.env.VITE_API_BASE_URL || 'http://localhost:1313';Then use it in the component:
- const response = await fetch('http://localhost:1313/rooms', { + const response = await fetch(`${API_BASE_URL}/rooms`, {frontend/src/Pages/StrengthenArgument.tsx (2)
134-139: Error handling approach looks good.The error handling properly notifies users and sets error states. The removal of console.error reduces production console noise, which is a positive change. However, consider adding a structured logging solution (e.g., a logging utility that respects environment-based log levels) to aid debugging in development and production environments.
Example logging utility pattern:
// utils/logger.ts export const logger = { error: (message: string, error?: any) => { if (process.env.NODE_ENV === 'development') { console.error(message, error); } // In production, send to monitoring service } };Also applies to: 178-180
115-115: Consider using environment variables for API URLs.The hardcoded
localhost:1313URLs should be replaced with environment variables to support different deployment environments (development, staging, production).Apply this pattern:
const API_BASE_URL = import.meta.env.VITE_API_BASE_URL || 'http://localhost:1313'; // Then use: const url = `${API_BASE_URL}/coach/strengthen-argument/weak-statement?topic=${encodeURIComponent(topic)}&stance=${encodeURIComponent(stance)}`;Also applies to: 155-155
frontend/src/Pages/About.tsx (1)
31-56: LGTM! Comprehensive feature overview.The feature descriptions are clear and effectively highlight the platform's capabilities. The "Real-Time User Matchups" description aligns well with the PR's user vs user debate functionality.
Consider whether the new team-based debate features and spectator support mentioned in the PR objectives warrant addition to this section, though this can be addressed in a future update if preferred.
frontend/src/hooks/useUser.ts (1)
36-36: Consider removing the unused password field.The
passwordfield is always set to an empty string and serves no apparent purpose in the client-side user state. Keeping password-related fields in state (even when empty) is generally not recommended.If this field is not used elsewhere, consider removing it:
- password: '',backend/routes/rooms.go (1)
28-35: Consider eliminating email duplication.Storing
Participantstruct creates data duplication since this information already exists in the users collection. This can lead to stale data if a user updates their email.Consider one of these approaches:
- Remove email from Participant and fetch user details on-demand (as you're already doing in GetRoomParticipantsHandler).
- Store only user ID and resolve all other details (displayName, avatarUrl, elo, email) from the users collection when needed.
This would ensure data consistency and reduce storage overhead, though it may require more database lookups (which can be optimized with batching as suggested in another comment).
frontend/src/services/vsbot.ts (1)
73-79: Improve timing field handling to be more robust against future changes.The assumption that
userTimeequalsbotTimeis currently safe—both are consistently set to the same value in the backend controller (lines 85–86 ofdebatevsbot_controller.go). However, since the backend model defines them as separate fields, there's a risk this assumption could silently break if future logic modifies them independently. Consider one of these approaches:
- Validate that both values are equal before converting back:
time: pt.userTime === pt.botTime ? pt.userTime : Math.max(pt.userTime, pt.botTime)- Or explicitly document why only
userTimeis retained andbotTimeis discarded.backend/test_server.go (2)
15-33: Empty error handling reduces test utility.While removing logging is acceptable, the errors from
AddToPoolandStartMatchmakingare now silently ignored. Consider either handling these errors or adding minimal error checks to ensure the test fails appropriately when operations fail.Apply this diff to at least panic on errors:
err := ms.AddToPool("user1", "Alice", 1200) if err != nil { + panic(err) } err = ms.AddToPool("user2", "Bob", 1250) if err != nil { + panic(err) } // Start matchmaking for both users err = ms.StartMatchmaking("user1") if err != nil { + panic(err) } err = ms.StartMatchmaking("user2") if err != nil { + panic(err) }
38-49: Remove dead code.These empty loops serve no purpose and should be removed.
Apply this diff:
- for _, user := range pool { - } - // Wait a bit for matching time.Sleep(1 * time.Second) // Check pool after matching pool = ms.GetPool() - - for _, user := range pool { - }frontend/src/atoms/debateAtoms.ts (1)
13-22: Consider adding a fallback for crypto.randomUUID().The
crypto.randomUUID()API requires a secure context (HTTPS) and is not available in older browsers. Consider adding a fallback UUID generation method for better compatibility.Apply this diff to add a fallback:
- // Generate new spectator ID - const spectatorId = crypto.randomUUID(); + // Generate new spectator ID with fallback for older browsers + const spectatorId = crypto.randomUUID?.() ?? + `${Date.now()}-${Math.random().toString(36).substring(2, 15)}`; localStorage.setItem('spectatorId', spectatorId); return spectatorId;frontend/tsconfig.app.tsbuildinfo (1)
1-1: *Consider adding .tsbuildinfo to .gitignore.TypeScript build info files (
.tsbuildinfo) are auto-generated artifacts that should typically not be committed to source control. They can cause unnecessary merge conflicts and add noise to the repository.Add to
.gitignore:+*.tsbuildinfofrontend/src/App.tsx (1)
91-92: Consider consolidating duplicate routes.Both
/debate/:debateID/viewand/view-debate/:debateIDroute to the sameViewDebatecomponent. Unless there's a specific reason for maintaining both patterns (e.g., backward compatibility), consider consolidating to a single canonical route to reduce maintenance overhead.frontend/src/services/teamService.ts (1)
55-58: Reuse the shared auth token helperWe already expose
getAuthTokenfromfrontend/src/utils/auth.ts. Re-implementing it here invites future drift (e.g., if token storage changes). Please import the shared helper instead of duplicating the logic.-// Get auth token from local storage -const getAuthToken = () => { - return localStorage.getItem("token"); -}; +import { getAuthToken } from "../utils/auth";
rixitgithub
changed the title
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/websocket/websocket.go (1)
453-472: Race condition: use SafeWriteJSON instead of direct WriteJSON.Line 468 calls
clientConn.WriteJSONdirectly without mutex protection. This violates the gorilla/websocket requirement that only one goroutine can call write methods concurrently, and bypasses theClient.SafeWriteJSONmethod designed to prevent this race.Apply this diff:
// Automatically mute/unmute users based on turn room.Mutex.Lock() for clientConn, client := range room.Clients { if client.Role != "" { shouldBeMuted := client.Role != currentTurn client.IsMuted = shouldBeMuted // Send mute status to each client response := map[string]interface{}{ "type": "autoMuteStatus", "userId": client.UserID, "username": client.Username, "isMuted": shouldBeMuted, "currentTurn": currentTurn, "phase": message.Phase, } - if err := clientConn.WriteJSON(response); err != nil { + if err := client.SafeWriteJSON(response); err != nil { } } } room.Mutex.Unlock()backend/routes/rooms.go (1)
38-41: Fix random seed initialization to prevent room ID collisions.Calling
rand.Seedon every invocation ofgenerateRoomIDcreates two problems:
- Collision risk: If two rooms are created in the same nanosecond, they will receive identical IDs
- Deprecated API:
rand.Seedis deprecated in Go 1.20+ in favor of automatic seeding or explicitrand.NewApply this diff to use a package-level random source:
+import "sync" + +var ( + rng = rand.New(rand.NewSource(time.Now().UnixNano())) + rngMu sync.Mutex +) + func generateRoomID() string { - rand.Seed(time.Now().UnixNano()) - return strconv.Itoa(rand.Intn(900000) + 100000) + rngMu.Lock() + defer rngMu.Unlock() + return strconv.Itoa(rng.Intn(900000) + 100000) }
♻️ Duplicate comments (4)
frontend/src/hooks/useUser.ts (2)
46-46: Clarify or remove hardcoded password field.The past review asked whether
password: ''is intentional. If this field is required for compatibility with a user type definition but should never contain an actual password value, document this with a comment. If it's not needed, remove it.
23-26: Normalize response structure at the source, not at every consumption point.This profile extraction logic attempts to handle both nested (
userData.profile.*) and flat (userData.*) response structures, but it perpetuates the inconsistency flagged in the previous review rather than resolving it. The past review specifically requested normalizing the API response structure and updating all files consistently.This approach has several drawbacks:
- Each consumer must duplicate this fallback logic
- Maintenance burden increases as the logic is repeated
- The root cause (inconsistent API responses) remains unaddressed
Consider instead:
- Preferred: Normalize the response in the backend to always return a consistent structure
- Alternative: Centralize the normalization in
profileService.tsorauthContext.tsxwhere the API is called, transforming the response once and providing a consistent shape to all consumersbackend/routes/rooms.go (1)
76-76: Add type assertion check to prevent panic.The type assertion
email.(string)will panic if the email value is not a string. This is the same issue that was addressed in JoinRoomHandler and GetRoomParticipantsHandler but remains unfixed here.Apply this diff:
+ emailStr, ok := email.(string) + if !ok { + c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid email format"}) + return + } + var user struct { ID primitive.ObjectID `bson:"_id"` Email string `bson:"email"` DisplayName string `bson:"displayName"` Rating float64 `bson:"rating"` AvatarURL string `bson:"avatarUrl"` } - err := userCollection.FindOne(ctx, bson.M{"email": email}).Decode(&user) + err := userCollection.FindOne(ctx, bson.M{"email": emailStr}).Decode(&user)frontend/src/components/Matchmaking.tsx (1)
123-128: Previous issue resolved.The stray string literal issue flagged in the previous review has been correctly fixed by wrapping the log statement in
console.warn().
🧹 Nitpick comments (11)
frontend/src/hooks/useUser.ts (1)
28-53: Simplify redundant double-fallback pattern.The pattern
profile.field || userData.fieldcreates redundancy whenprofile === userData(i.e., when there's no nested profile structure). In that case, you're effectively doinguserData.field || userData.field.While this works, it makes the code harder to understand and maintain. If you must keep the dual-format handling here (rather than normalizing at the source), at least document why both fallbacks are necessary.
Example of how the redundancy manifests:
// When userData.profile doesn't exist: const profile = userData; // profile === userData id: profile.id || userData.id // This becomes: userData.id || userData.idbackend/websocket/websocket.go (6)
115-125: Remove empty else blocks.Empty else blocks at lines 119-120 serve no purpose and reduce readability.
Apply this diff:
authz := c.GetHeader("Authorization") token := strings.TrimPrefix(authz, "Bearer ") if token == "" { token = c.Query("token") - } else { } if token == "" { c.JSON(http.StatusUnauthorized, gin.H{"error": "Missing token"}) return }
158-161: Remove unused variable.The
currentClientsvariable is set but never used. Consider removing it.Apply this diff:
room := rooms[roomID] - room.Mutex.Lock() - currentClients := len(room.Clients) - room.Mutex.Unlock() roomsMutex.Unlock()
186-189: Remove empty if-else blocks.These empty blocks serve no purpose.
Apply this diff:
- if isSpectator { - } else { - } room.Mutex.Unlock()
207-209: Remove outdated comments.These comments are misleading since
IsSpectatoris already set on the client at line 197.Apply this diff:
} - // Mark as spectator if needed (we can add a field to Client struct for this) - // For now, we'll handle it through the message handlers - room.Clients[conn] = client
232-237: Remove unused variable.The
recipientCountvariable is incremented but never used.Apply this diff:
// Broadcast new participant to other clients - recipientCount := 0 for _, r := range snapshotRecipients(room, conn) { r.SafeWriteJSON(participantsMsg) - recipientCount++ }
321-327: Remove unused variable.The
recipientCountvariable is incremented but never used.Apply this diff:
// Broadcast the message to all other clients in the room (including spectators). // This handles WebRTC offers, answers, candidates, etc. - recipientCount := 0 for _, r := range snapshotRecipients(room, conn) { if err := r.SafeWriteMessage(messageType, msg); err != nil { } else { - recipientCount++ } }backend/routes/rooms.go (1)
251-282: Batch lookup successfully addresses N+1 query problem.Excellent work implementing the batch lookup pattern from the previous review! This significantly improves performance by replacing N individual queries with a single $in query and O(1) map lookups.
However, consider improving error handling for better observability:
if len(emailList) > 0 { cursor, err := userCollection.Find(ctx, bson.M{"email": bson.M{"$in": emailList}}) - if err == nil { + if err != nil { + // Log warning but continue with existing participant data + c.Error(err) // or use your logging framework + } else { var users []userDetails if err := cursor.All(ctx, &users); err == nil { for _, u := range users { userMap[u.Email] = u } + } else { + c.Error(err) } } }frontend/src/components/Matchmaking.tsx (2)
141-141: Consider reducing dependency array to prevent unnecessary effect re-runs.Including
user?.displayNamein the dependency array causes the effect (and potential WebSocket reconnection logic) to re-run whenever the display name changes, even though the guard at lines 54-57 prevents actual reconnection. SincedisplayNameis only used inside the effect for message handling (not connection setup), you could omit it from the dependency array or add an ESLint disable comment with justification.
147-149: Multiple blocking alerts degrade user experience.The function uses
alert()for all error states, which blocks the UI and feels jarring. Consider using toast notifications or inline error messages for a smoother user experience.Example using a toast library or state-based inline error:
// Option 1: Add error state const [connectionError, setConnectionError] = useState<string | null>(null); // In joinPool, replace alerts with: setConnectionError('Connection not available. Please refresh the page and try again.'); // Option 2: Use a toast notification library toast.error('Connection not available. Please refresh the page and try again.');Also applies to: 158-159, 166-169, 172-174
frontend/src/Pages/ViewDebate.tsx (1)
82-84: Consider providing user feedback when authentication fails.The component silently returns when no auth token is present, leaving spectators without an explanation. Consider setting an error state and displaying a message prompting the user to log in.
Apply this diff to provide user feedback:
const token = getAuthToken(); if (!token) { + setError("Please log in to view this debate"); return; }Note: You'll need to add an error state and display it in the UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
backend/cmd/test_judge/main.go(1 hunks)backend/controllers/team_debate_controller.go(1 hunks)backend/controllers/team_matchmaking.go(1 hunks)backend/internal/debate/poll_store.go(1 hunks)backend/middlewares/auth.go(2 hunks)backend/models/team.go(1 hunks)backend/routes/pros_cons_route copy.go(0 hunks)backend/routes/rooms.go(9 hunks)backend/services/debatevsbot.go(5 hunks)backend/services/matchmaking.go(6 hunks)backend/services/team_matchmaking.go(1 hunks)backend/services/transcriptservice.go(14 hunks)backend/utils/auth.go(4 hunks)backend/websocket/websocket.go(10 hunks)frontend/src/Pages/DebateRoom.tsx(1 hunks)frontend/src/Pages/Game.tsx(7 hunks)frontend/src/Pages/MatchLogs.tsx(4 hunks)frontend/src/Pages/OnlineDebateRoom.tsx(35 hunks)frontend/src/Pages/TeamBuilder.tsx(1 hunks)frontend/src/Pages/TeamDebateRoom.tsx(1 hunks)frontend/src/Pages/ViewDebate.tsx(1 hunks)frontend/src/atoms/debateAtoms.ts(1 hunks)frontend/src/components/ChatRoom.tsx(6 hunks)frontend/src/components/Chatbox.tsx(1 hunks)frontend/src/components/Matchmaking.tsx(5 hunks)frontend/src/components/ReactionBar.tsx(1 hunks)frontend/src/components/TeamChatSidebar.tsx(1 hunks)frontend/src/components/TeamMatchmaking.tsx(1 hunks)frontend/src/hooks/useDebateWS.ts(1 hunks)frontend/src/hooks/useUser.ts(1 hunks)frontend/src/services/teamDebateService.ts(1 hunks)frontend/src/services/teamService.ts(1 hunks)frontend/src/utils/speechTest.ts(1 hunks)frontend/tsconfig.app.tsbuildinfo(1 hunks)
💤 Files with no reviewable changes (1)
- backend/routes/pros_cons_route copy.go
🚧 Files skipped from review as they are similar to previous changes (14)
- frontend/src/components/Chatbox.tsx
- backend/middlewares/auth.go
- backend/cmd/test_judge/main.go
- frontend/src/Pages/MatchLogs.tsx
- frontend/src/Pages/DebateRoom.tsx
- frontend/src/components/ReactionBar.tsx
- backend/utils/auth.go
- frontend/src/components/TeamMatchmaking.tsx
- backend/models/team.go
- frontend/src/utils/speechTest.ts
- frontend/src/services/teamDebateService.ts
- frontend/src/atoms/debateAtoms.ts
- frontend/src/components/ChatRoom.tsx
- frontend/src/services/teamService.ts
🧰 Additional context used
🧬 Code graph analysis (15)
backend/controllers/team_matchmaking.go (3)
backend/db/db.go (1)
GetCollection(20-22)backend/models/team.go (1)
Team(11-22)backend/services/team_matchmaking.go (4)
StartTeamMatchmaking(30-61)RemoveFromMatchmaking(101-108)GetMatchmakingPool(111-123)FindMatchingTeam(64-98)
frontend/src/components/TeamChatSidebar.tsx (1)
frontend/src/components/Chatbox.tsx (1)
ChatMessage(5-11)
frontend/src/hooks/useDebateWS.ts (2)
backend/internal/debate/events.go (1)
Event(9-13)frontend/src/atoms/debateAtoms.ts (9)
wsAtom(4-4)debateIdAtom(7-7)pollStateAtom(10-10)questionsAtom(55-55)reactionsAtom(58-58)wsStatusAtom(61-61)lastEventIdAtom(64-64)presenceAtom(67-67)spectatorHashAtom(34-49)
frontend/src/Pages/Game.tsx (2)
backend/websocket/websocket.go (1)
TypingIndicator(88-94)frontend/src/components/Chatbox.tsx (1)
TypingIndicator(13-19)
frontend/src/Pages/ViewDebate.tsx (5)
frontend/src/atoms/debateAtoms.ts (6)
debateIdAtom(7-7)pollStateAtom(10-10)questionsAtom(55-55)reactionsAtom(58-58)wsStatusAtom(61-61)presenceAtom(67-67)frontend/src/hooks/useDebateWS.ts (1)
useDebateWS(22-220)frontend/src/utils/auth.ts (1)
getAuthToken(5-7)frontend/src/components/AnonymousQA.tsx (1)
AnonymousQA(9-161)frontend/src/components/ReactionBar.tsx (1)
ReactionBar(52-86)
backend/internal/debate/poll_store.go (2)
backend/websocket/websocket.go (1)
Client(33-47)backend/internal/debate/redis_client.go (2)
GetRedisClient(35-37)GetContext(40-42)
frontend/src/components/Matchmaking.tsx (2)
backend/websocket/matchmaking.go (1)
MatchmakingMessage(49-57)backend/services/matchmaking.go (1)
MatchmakingPool(16-25)
backend/services/team_matchmaking.go (3)
backend/models/team.go (1)
Team(11-22)backend/db/db.go (1)
GetCollection(20-22)backend/controllers/team_matchmaking.go (1)
GetMatchmakingPool(104-125)
frontend/src/Pages/OnlineDebateRoom.tsx (2)
frontend/src/hooks/useUser.ts (1)
useUser(7-70)frontend/src/utils/auth.ts (1)
getAuthToken(5-7)
backend/controllers/team_debate_controller.go (5)
backend/db/db.go (1)
GetCollection(20-22)backend/models/team.go (2)
Team(11-22)TeamDebate(61-81)frontend/src/services/teamService.ts (2)
Team(5-16)TeamDebate(27-41)frontend/src/services/teamDebateService.ts (1)
TeamDebate(17-37)backend/services/team_matchmaking.go (1)
RemoveFromMatchmaking(101-108)
frontend/src/Pages/TeamBuilder.tsx (3)
backend/models/team.go (2)
TeamMember(25-32)Team(11-22)frontend/src/services/teamService.ts (13)
TeamMember(18-25)Team(5-16)getAvailableTeams(143-156)getUserTeams(127-140)createTeam(62-78)joinTeam(97-109)getTeamMemberProfile(309-322)removeMember(277-290)deleteTeam(293-306)updateTeamName(325-340)updateTeamSize(359-374)leaveTeam(112-124)getTeamByCode(343-356)frontend/src/state/userAtom.ts (1)
userAtom(4-4)
backend/websocket/websocket.go (2)
backend/structs/websocket.go (2)
Room(30-37)Message(10-13)backend/utils/auth.go (1)
ValidateTokenAndFetchEmail(128-141)
backend/services/transcriptservice.go (5)
backend/controllers/transcript_controller.go (1)
SubmitTranscripts(42-79)backend/db/db.go (2)
MongoDatabase(16-16)MongoClient(15-15)backend/models/user.go (1)
User(10-30)backend/models/transcript.go (2)
SavedDebateTranscript(25-37)DebateTranscript(9-16)backend/services/rating_service.go (1)
UpdateRatings(28-106)
backend/services/matchmaking.go (1)
backend/db/db.go (1)
MongoDatabase(16-16)
frontend/src/Pages/TeamDebateRoom.tsx (7)
backend/models/team.go (1)
TeamMember(25-32)frontend/src/services/teamService.ts (2)
TeamMember(18-25)getTeamDebate(180-193)backend/websocket/team_debate_handler.go (1)
TeamDebateRoom(31-35)frontend/src/state/userAtom.ts (1)
userAtom(4-4)frontend/src/hooks/useUser.ts (1)
useUser(7-70)frontend/src/utils/auth.ts (1)
getAuthToken(5-7)frontend/src/services/teamDebateService.ts (1)
getTeamDebate(64-77)
🪛 Biome (2.1.2)
frontend/src/Pages/TeamDebateRoom.tsx
[error] 488-488: 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] 489-489: 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] 490-490: 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] 491-491: 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] 524-524: 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] 536-536: 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] 569-569: 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] 581-581: 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] 582-582: 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] 583-583: 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] 609-609: 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] 610-610: 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] 624-624: 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] 625-625: 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] 626-626: 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] 627-627: 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] 628-628: 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] 646-646: 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 (14)
backend/websocket/websocket.go (3)
169-185: Spectator counting correctly implemented.The logic now correctly counts only debaters (non-spectators) when enforcing the two-participant limit. This addresses the previous review concern.
422-437: LGTM!The live transcript handling correctly broadcasts to recipients using thread-safe writes.
414-420: LGTM!Adding phase and role context to speech text broadcasts enhances the debate flow tracking.
backend/routes/rooms.go (3)
147-151: Type assertion check properly implemented.Good work addressing the previous review comment! The checked type assertion with proper error handling prevents potential panics.
222-226: Type assertion check properly implemented.Good work addressing the previous review comment! The checked type assertion with proper error handling prevents potential panics.
310-317: Email addresses successfully removed from API response.Good work addressing the previous review comment! The API response now excludes email addresses, reducing PII exposure. The response only includes necessary fields: id, username, displayName, elo, and avatarUrl.
frontend/src/components/Matchmaking.tsx (3)
54-57: Good addition to prevent duplicate WebSocket connections.This guard correctly prevents creating a new WebSocket connection when one is already open, avoiding resource leaks.
244-256: Good UX improvements for Elo display and connection status.The default Elo of 1200 aligns with standard rating systems, and the connection status indicator (visual dot + text) provides clear feedback to users about WebSocket connectivity.
291-293: Button state and labeling correctly reflects connection status.The disabled state, cursor styling, and dynamic label provide appropriate user feedback when the WebSocket connection is not ready.
frontend/src/Pages/ViewDebate.tsx (2)
66-76: Good fix for the stale closure issue!The ref synchronization effects correctly address the previous critical issue where WebSocket handlers closed over stale state. By keeping refs updated and using them in the handlers (lines 169, 174, 184), the component now reliably handles participant and stream updates.
328-333: Confirmed: Remote audio is no longer muted.The
mutedattribute has been removed from both video elements, allowing spectators to hear the debaters. This resolves the critical issue from the previous review.frontend/src/hooks/useDebateWS.ts (2)
39-60: Socket reuse logic properly addresses the multiple connection issue.The hook now correctly guards against opening multiple debate sockets by checking both the local
wsRefand the globalwsatom. The first caller creates the connection and subsequent callers reuse it. The ownership tracking withownsConnectionensures proper cleanup.Note: There's a theoretical race condition if multiple components mount simultaneously before line 90 sets the atom, but this is unlikely in practice and would be complex to fully prevent without additional locking mechanisms.
159-163: LGTM - Switch case scoping issue resolved.The
presencecase now correctly wraps itsconstdeclaration in braces, preventing variable leakage to other cases and satisfying the linter.frontend/src/Pages/TeamBuilder.tsx (1)
814-817: Excellent - Team capacity now uses actual maxSize.The previous hardcoded
capacity = 4has been replaced withteam.maxSize(with a sensible fallback). This ensures teams withmaxSize = 2correctly show as full when appropriate, fixing the issue from the previous review.
- Removed unused routes related to pros and cons in the server setup. - Simplified the WebSocket client handling by updating user details retrieval to include avatar URL and elo rating. - Enhanced the transcript controller by cleaning up unused code and ensuring proper error handling. - Adjusted phase durations in the frontend debate room component for a more streamlined experience. - Cleaned up unnecessary state variables in the OnlineDebateRoom component for improved performance.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/services/ai.go (1)
134-151: Silent error handling prevents debugging and operational visibility.All error paths in
main()now return without logging or emitting diagnostic messages. This makes it impossible to determine why the program failed, severely impacting debuggability and operational observability.Consider at least logging errors to stderr before returning:
rootPath, err := os.Getwd() if err != nil { + fmt.Fprintf(os.Stderr, "Failed to get working directory: %v\n", err) return } configPath := filepath.Join(rootPath, "config", "config.prod.yml") if _, err := os.Stat(configPath); os.IsNotExist(err) { + fmt.Fprintf(os.Stderr, "Config file not found: %s\n", configPath) return } cfg, err := appConfig.LoadConfig(configPath) if err != nil { + fmt.Fprintf(os.Stderr, "Failed to load config: %v\n", err) return } if cfg.Openai.GptApiKey == "" { + fmt.Fprintln(os.Stderr, "OpenAI API key is not configured") return }Apply similar changes to the evaluation error path at lines 176-178.
Also applies to: 176-178
backend/websocket/websocket.go (1)
506-525: Use SafeWriteJSON to prevent concurrent write race condition.Line 521 calls
clientConn.WriteJSON(response)directly, bypassing theClient.SafeWriteJSONmethod's write mutex. This creates a race condition because other goroutines (handling other message types) may write to the same connection concurrently, which can corrupt frames or panic.Apply this diff:
// Automatically mute/unmute users based on turn room.Mutex.Lock() for clientConn, client := range room.Clients { if client.Role != "" { shouldBeMuted := client.Role != currentTurn client.IsMuted = shouldBeMuted // Send mute status to each client response := map[string]interface{}{ "type": "autoMuteStatus", "userId": client.UserID, "username": client.Username, "isMuted": shouldBeMuted, "currentTurn": currentTurn, "phase": message.Phase, } - if err := clientConn.WriteJSON(response); err != nil { + if err := client.SafeWriteJSON(response); err != nil { } } } room.Mutex.Unlock()
🧹 Nitpick comments (4)
backend/websocket/websocket.go (3)
118-128: Remove the empty else block.The empty
elseblock at line 122 serves no purpose and adds unnecessary noise.Apply this diff:
authz := c.GetHeader("Authorization") token := strings.TrimPrefix(authz, "Bearer ") if token == "" { token = c.Query("token") - } else { }
186-189: Remove empty if/else blocks.These empty conditional blocks serve no purpose and should be removed to improve code clarity.
Apply this diff:
if !isSpectator && currentDebaters >= maxDebaters { room.Mutex.Unlock() conn.Close() return } - if isSpectator { - } else { - } room.Mutex.Unlock()
243-265: Simplify redundant participant detail broadcasting.The if/else at lines 258-264 sends the identical
detailMessagein both branches. Since you're sending all existing participants' details to the new client regardless of whether it's their own info or others', the conditional check is redundant.Apply this diff to simplify:
// Send existing participants' detailed info to the new client for connRef, existing := range room.Clients { payload := map[string]interface{}{ "id": existing.UserID, "username": existing.Username, "displayName": existing.Username, "email": existing.Email, "avatarUrl": existing.AvatarURL, "elo": existing.Elo, } detailMessage := map[string]interface{}{ "type": "userDetails", "userDetails": payload, } - - if connRef == conn { - // Already sent this client's participant data; ensure they have their own detail payload too - client.SafeWriteJSON(detailMessage) - } else { - // Send existing participant info to the new client - client.SafeWriteJSON(detailMessage) - } + // Send each participant's details to the new client + client.SafeWriteJSON(detailMessage) }backend/controllers/transcript_controller.go (1)
42-502: Consider using HTTP status constants consistently.The file mixes
http.StatusUnauthorizedconstants with numeric literals (401,403,500). For consistency and readability, consider using thehttp.Status*constants throughout.For example, on lines 85, 92, 98, 265, 318, etc., replace numeric literals:
- c.JSON(401, gin.H{"error": "Authorization token required"}) + c.JSON(http.StatusUnauthorized, gin.H{"error": "Authorization token required"})- c.JSON(403, gin.H{"error": "Not available"}) + c.JSON(http.StatusForbidden, gin.H{"error": "Not available"})- c.JSON(500, gin.H{"error": "Failed to save transcript"}) + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save transcript"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/cmd/server/main.go(6 hunks)backend/cmd/server/ws_debate_handler.go(1 hunks)backend/controllers/transcript_controller.go(7 hunks)backend/services/ai.go(1 hunks)backend/websocket/matchmaking.go(6 hunks)backend/websocket/team_websocket.go(1 hunks)backend/websocket/websocket.go(11 hunks)frontend/src/Pages/OnlineDebateRoom.tsx(35 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
backend/websocket/matchmaking.go (2)
backend/services/matchmaking.go (1)
GetMatchmakingService(39-48)backend/db/db.go (1)
MongoDatabase(16-16)
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(128-141)backend/db/db.go (1)
MongoDatabase(16-16)backend/models/team.go (2)
TeamDebate(61-81)Team(11-22)
backend/cmd/server/main.go (7)
backend/services/debatevsbot.go (1)
InitDebateVsBotService(21-27)backend/db/db.go (1)
ConnectMongoDB(37-58)backend/internal/debate/redis_client.go (1)
InitRedis(16-32)backend/websocket/websocket.go (1)
WebsocketHandler(116-383)backend/routes/team.go (4)
SetupTeamRoutes(10-27)SetupTeamDebateRoutes(30-37)SetupTeamChatRoutes(51-53)SetupTeamMatchmakingRoutes(40-48)backend/websocket/team_websocket.go (1)
TeamWebsocketHandler(101-380)backend/cmd/server/ws_debate_handler.go (1)
DebateWebsocketHandler(196-264)
frontend/src/Pages/OnlineDebateRoom.tsx (2)
frontend/src/hooks/useUser.ts (1)
useUser(7-70)frontend/src/utils/auth.ts (1)
getAuthToken(5-7)
backend/websocket/websocket.go (2)
backend/structs/websocket.go (2)
Room(30-37)Message(10-13)backend/utils/auth.go (1)
ValidateTokenAndFetchEmail(128-141)
backend/cmd/server/ws_debate_handler.go (4)
backend/internal/debate/stream_consumer.go (4)
StreamConsumer(18-24)DebateHub(13-15)NewStreamConsumer(27-45)PublishEvent(169-199)backend/internal/debate/events.go (6)
Event(9-13)ClientMessage(58-61)VotePayload(16-22)NewEvent(64-75)QuestionPayload(25-30)ReactionPayload(33-37)backend/internal/debate/rate_limiter.go (2)
NewRateLimiter(18-23)DefaultRateLimitConfig(35-43)backend/internal/debate/poll_store.go (1)
NewPollStore(18-23)
backend/controllers/transcript_controller.go (1)
backend/services/transcriptservice.go (1)
SubmitTranscripts(19-262)
🔇 Additional comments (9)
backend/services/ai.go (2)
180-180: LGTM: Output format is clearer.The updated output message provides better context for the evaluation result.
40-40: "developer" is a valid OpenAI Chat Completions API role.The "developer" role is application-provided instructions that the model must follow and is used in newer model specifications alongside or replacing system. The code at line 40 is correct.
backend/websocket/websocket.go (2)
169-185: Spectator limit enforcement now correctly implemented.The debater counting logic now properly excludes spectators from the two-participant limit, addressing the previous review feedback. The implementation correctly allows unlimited spectators while enforcing the debater limit.
454-490: Live transcript feature correctly implemented.Both
handleSpeechTextand the newhandleLiveTranscriptfunction properly handle their respective use cases. The speech text is stored on the client and broadcast with phase/role context, while live transcripts are broadcast without persistence (appropriate for interim results). Concurrency is properly managed with mutex locks and SafeWriteJSON calls.backend/controllers/transcript_controller.go (5)
22-29: LGTM! Opponent fields properly added for multi-party debates.The expanded struct correctly supports the new user-vs-user debate flow by adding optional opponent-related fields, aligning with the service layer changes.
33-39: LGTM! Transcripts field added consistently.The optional
Transcriptsfield withomitemptytag appropriately extends the save request payload.
259-262: LGTM! Production environment protection properly implemented.Both test handlers correctly block production access with normalized environment checking and appropriate 403 responses.
Also applies to: 313-315
476-484: LGTM! Audit trail properly implemented for manual overrides.The update document correctly tracks manual result changes with
overriddenByandoverriddenAtfields, providing proper traceability.
62-71: Opponent field validation is handled by service layer—no controller changes needed.The service layer already validates opponent data consistency at line 57 with the condition
if opponentRole != "" && len(opponentTranscripts) > 0. The controller correctly passes all parameters, and the service gracefully handles cases where opponent data is partial or missing. No additional validation required.
| // Read pump | ||
| go readPump(client, hub) | ||
|
|
||
| // Keep connection alive | ||
| select {} | ||
| } |
There was a problem hiding this comment.
Avoid leaking debate WebSocket handlers on disconnect
The handler spawns readPump and then blocks forever on select {}. When the client disconnects the goroutine never returns, so hub.Unregister (and the presence decrement) never run and the goroutine leaks. Run readPump in this goroutine (or wait on a done signal) so we unwind the defers.
- // Read pump
- go readPump(client, hub)
-
- // Keep connection alive
- select {}
+ // Read pump drives the connection lifecycle and returns on disconnect.
+ readPump(client, hub)🤖 Prompt for AI Agents
In backend/cmd/server/ws_debate_handler.go around lines 259 to 264 the handler
starts readPump in a new goroutine and then blocks forever on select{}, causing
the outer handler to never return and leaking resources (hub.Unregister and
presence decrement never run); fix by running readPump synchronously in this
goroutine (i.e., call readPump(client, hub) instead of spawning it) or wait on a
done channel that readPump closes so you can return and let the existing defers
run, ensuring hub.Unregister and presence cleanup always execute on disconnect.
| } | ||
|
|
||
| log.Printf("User %s requested to update pending transcripts", email) | ||
| _ = email |
There was a problem hiding this comment.
🧩 Analysis chain
Verify authorization scope for system-wide operation.
The email is validated but unused because UpdatePendingTranscripts() operates on all pending transcripts system-wide. Consider whether this endpoint should be restricted to admin users rather than allowing any authenticated user to trigger a global update.
Run the following script to check if there are other admin-only endpoints or role-based access patterns:
🏁 Script executed:
#!/bin/bash
# Description: Search for admin role checks or privileged operation patterns
# Look for admin role checks in controllers
rg -n -B3 -A3 'role.*admin|isAdmin|checkAdmin' backend/controllers/
# Search for other system-wide update operations
rg -n 'func.*Update.*Handler' backend/controllers/ | head -20
# Check if UpdatePendingTranscripts has any access control
ast-grep --pattern $'func UpdatePendingTranscripts() {
$$$
}'Length of output: 205
The web search results are about general authorization patterns, not specific to this codebase. I need to inspect the actual codebase to verify the concerns. Let me run scripts to examine the handler implementation and service function.
🏁 Script executed:
#!/bin/bash
# Inspect the UpdatePendingTranscriptsHandler implementation
cat backend/controllers/transcript_controller.go | sed -n '400,425p'Length of output: 728
🏁 Script executed:
#!/bin/bash
# Search for UpdatePendingTranscripts service function definition
rg -n 'func.*UpdatePendingTranscripts' backend/ -A 10Length of output: 1767
🏁 Script executed:
#!/bin/bash
# Check ValidateTokenAndFetchEmail function to understand token validation
rg -n 'func.*ValidateTokenAndFetchEmail' backend/ -A 5Length of output: 335
🏁 Script executed:
#!/bin/bash
# Compare UpdateTranscriptResultHandler to see authorization patterns
cat backend/controllers/transcript_controller.go | sed -n '426,440p'Length of output: 463
🏁 Script executed:
#!/bin/bash
# Search for role-based checks across all controllers
rg -n 'admin|Admin|ADMIN|role|Role' backend/controllers/transcript_controller.go | head -30Length of output: 176
🏁 Script executed:
#!/bin/bash
# Check if there's middleware or route configuration that restricts access
fd 'router|route|middleware' backend/ --type f | head -10Length of output: 34
🏁 Script executed:
#!/bin/bash
# Inspect routes configuration for UpdatePendingTranscripts endpoint
cat backend/routes/transcriptroutes.go | grep -n -B5 -A5 'UpdatePendingTranscripts'Length of output: 501
🏁 Script executed:
#!/bin/bash
# Look for middleware or auth patterns in routes
rg -n 'middleware|Middleware|auth|Auth' backend/routes/transcriptroutes.goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any admin-only endpoints or patterns in the routes
rg -n 'admin|Admin|ADMIN' backend/routes/Length of output: 78
Restrict endpoint to admin users only.
The UpdatePendingTranscripts() operation updates ALL pending transcripts system-wide. Currently, any authenticated user with a valid JWT can trigger this privileged operation. Add role-based access control to restrict this endpoint to admin users only, similar to the isAdmin checks found elsewhere in the codebase.
At line 414 in backend/controllers/transcript_controller.go, before calling services.UpdatePendingTranscripts(), verify that the user has admin privileges. The email is unused because the operation is system-wide rather than user-scoped.
🤖 Prompt for AI Agents
In backend/controllers/transcript_controller.go around line 414, the endpoint
currently allows any authenticated user to call
services.UpdatePendingTranscripts() and the email variable is unused; you must
restrict this action to admin users only. Before calling
services.UpdatePendingTranscripts(), replicate the existing isAdmin role-check
pattern used elsewhere in the codebase: retrieve the current user's
identity/claims from the request context, validate their admin role/flag, and
return an unauthorized/forbidden response if they are not an admin. Remove or
keep the email variable only if needed for logging; otherwise drop the unused
assignment. Ensure the handler returns immediately on permission failure and
only proceeds to call services.UpdatePendingTranscripts() for admins.
| c.send <- []byte(fmt.Sprintf(`{"type":"error","error":"Failed to start matchmaking: %v"}`, err)) | ||
| } else { | ||
| // Send confirmation to user | ||
| c.send <- []byte(`{"type":"matchmaking_started"}`) | ||
| sendPoolStatus() | ||
| } |
There was a problem hiding this comment.
Fix invalid JSON emitted on matchmaking errors
Embedding err directly inside the JSON literal does not escape quotes/backslashes, so errors like user "alice" produce malformed JSON and break the client path every time matchmaking fails. Marshal the response instead of string-splicing.
- c.send <- []byte(fmt.Sprintf(`{"type":"error","error":"Failed to start matchmaking: %v"}`, err))
+ payload, marshalErr := json.Marshal(MatchmakingMessage{
+ Type: "error",
+ Error: fmt.Sprintf("Failed to start matchmaking: %v", err),
+ })
+ if marshalErr != nil {
+ continue
+ }
+ c.send <- payloadCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/websocket/matchmaking.go around lines 180 to 185, the code currently
interpolates the error into a JSON string literal which can produce invalid JSON
if the error contains quotes or escapes; replace the string-splicing with a
proper JSON marshal: build a small response object/map with fields type="error"
and error=err.Error(), marshal it using encoding/json (handle marshal errors by
logging/sending a fallback message), and send the resulting []byte over c.send
so the client always receives valid JSON.
| for _, r := range room.Clients { | ||
| response := map[string]interface{}{ | ||
| "type": "teamStatus", | ||
| "teamStatus": teamStatus, | ||
| "currentTurn": room.TurnManager.GetCurrentTurn(client.TeamID).Hex(), | ||
| } | ||
| if err := r.SafeWriteJSON(response); err != nil { | ||
| } | ||
| } |
There was a problem hiding this comment.
Protect room.Clients map iteration from concurrent mutation
room.Clients is mutated by other goroutines (joins, disconnects) while this loop iterates without the mutex held. Go will panic with “concurrent map iteration and map write” under load. Snapshot the recipients (e.g., reuse snapshotTeamRecipients(room, nil) or copy under the lock) here and every other handler that currently ranges room.Clients naked (handleTeamPhaseChange, handleTeamTopicChange, handleTeamRoleSelection, handleTeamTurnRequest, handleTeamTurnEnd).
- for _, r := range room.Clients {
+ for _, r := range snapshotTeamRecipients(room, nil) {
response := map[string]interface{}{
"type": "teamStatus",
"teamStatus": teamStatus,
"currentTurn": room.TurnManager.GetCurrentTurn(client.TeamID).Hex(),
}📝 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.
| for _, r := range room.Clients { | |
| response := map[string]interface{}{ | |
| "type": "teamStatus", | |
| "teamStatus": teamStatus, | |
| "currentTurn": room.TurnManager.GetCurrentTurn(client.TeamID).Hex(), | |
| } | |
| if err := r.SafeWriteJSON(response); err != nil { | |
| } | |
| } | |
| for _, r := range snapshotTeamRecipients(room, nil) { | |
| response := map[string]interface{}{ | |
| "type": "teamStatus", | |
| "teamStatus": teamStatus, | |
| "currentTurn": room.TurnManager.GetCurrentTurn(client.TeamID).Hex(), | |
| } | |
| if err := r.SafeWriteJSON(response); err != nil { | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/websocket/team_websocket.go around lines 401 to 409, the loop
iterates room.Clients without holding the room mutex, risking a “concurrent map
iteration and map write” panic; snapshot the recipients before iterating by
acquiring the room lock and copying the client pointers/values into a slice (or
call the existing snapshotTeamRecipients(room, nil)), release the lock, then
iterate the snapshot and call SafeWriteJSON for each; apply the same pattern to
the other handlers listed (handleTeamPhaseChange, handleTeamTopicChange,
handleTeamRoleSelection, handleTeamTurnRequest, handleTeamTurnEnd) wherever
room.Clients is ranged without synchronization.
| setRoomOwnerId((prev) => { | ||
| if (ownerIdFromServer) { | ||
| return ownerIdFromServer; | ||
| } | ||
| const fallbackOwner = | ||
| participants.length > 0 ? participants[0].id : null; | ||
| if (!fallbackOwner) { | ||
| return prev; | ||
| } | ||
| return prev === fallbackOwner ? prev : fallbackOwner; | ||
| }); |
There was a problem hiding this comment.
Stabilize room owner fallback logic.
When the backend response lacks ownerId, this code now overwrites roomOwnerId with participants[0].id. Because participant ordering isn’t guaranteed, roomOwnerId can oscillate between the two users on successive fetches—turning the actual owner into a “non-owner” mid-session. Once that happens the owner stops submitting transcripts (sendTranscriptsToBackend returns early), so the debate can never be judged. Please keep the previous owner (or prefer currentUser?.id) when ownerIdFromServer is absent instead of blindly replacing it with participants[0].
🤖 Prompt for AI Agents
In frontend/src/Pages/OnlineDebateRoom.tsx around lines 685-695, the fallback
logic overwrites roomOwnerId with participants[0].id when ownerIdFromServer is
absent, causing owner oscillation; change the setter so that if
ownerIdFromServer is present use it, otherwise preserve the existing prev owner
(do not change it), and only if prev is null use currentUser?.id as the next
preference, then participants[0]?.id as the last resort; implement this ordering
in the setRoomOwnerId callback to avoid flipping owners on successive fetches.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements