Fix Critical Error Boundary Issues and Tailwind Configuration #120
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.
- 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.
…sues - Fix handleAsyncError to return the result from async functions - Improve TypeScript typing with generics for better type safety - Convert tailwind.config.js to tailwind.config.cjs for CommonJS compatibility - Use require() for tailwindcss-animate plugin (CommonJS) - Remove tailwind-scrollbar-hide to eliminate ES module warnings
|
Caution Review failedFailed to post review comments WalkthroughThis PR integrates Redis for debate event streaming with poll and rate-limiting infrastructure, implements comprehensive team management including creation, joining, matchmaking, and turn-based debates, adds WebSocket-based spectator support for individual and team debates, introduces global error boundaries and specialized error handling, refactors services to remove logging and add opponent debate tracking, and expands the frontend with new pages for team building and team debates alongside spectator viewing capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Spectator Client
participant WS as WebSocket<br/>(DebateHub)
participant Redis as Redis<br/>(Stream)
participant DB as MongoDB
Client->>WS: Join (spectatorId)
WS->>WS: Hash spectatorId<br/>Register in room
WS->>Redis: StartConsumerGroup()
WS->>DB: LoadPollSnapshot()
WS->>Client: Send poll_snapshot
WS->>Client: Send presence
Note over Client,Redis: Spectator Actions
Client->>WS: Vote payload
WS->>Redis: Vote(pollID, option)<br/>Track voter
WS->>Redis: PublishEvent()
WS->>Redis: XAdd to stream
loop Consumer Loop
Redis-->>WS: XReadGroup()
WS->>WS: ProcessMessage()
WS->>Client: BroadcastToDebate()
end
Client->>WS: Disconnect
WS->>WS: Unregister client
WS->>Client: BroadcastPresence()
sequenceDiagram
participant Captain as Team Captain
participant API as Matchmaking API
participant Pool as Matchmaking<br/>Pool
participant DB as MongoDB
Captain->>API: JoinMatchmaking(teamId)
API->>DB: Fetch team
API->>Pool: StartTeamMatchmaking(teamId)
Pool->>Pool: Store entry
Note over Captain,Pool: Waiting for Match...
Captain->>API: GetMatchmakingStatus(teamId)
API->>Pool: FindMatchingTeam(teamId)
Pool->>Pool: Scan pool for<br/>same size & Elo range
alt Match Found
API->>Captain: matched: true<br/>(opponent team data)
Captain->>API: CreateTeamDebate()
API->>DB: Insert TeamDebate
API->>Pool: RemoveFromMatchmaking()
API->>Captain: Redirect to debate
else No Match
API->>Captain: matched: false
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: 57
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
frontend/src/components/RoomBrowser.tsx (1)
21-52: Critical: Empty catch block silently swallows all fetch errors.The empty catch block at line 48 provides no error handling, user feedback, or recovery mechanism. Combined with the silent failures at lines 31-34 and 36-39, users cannot distinguish between legitimate empty rooms, network failures, or backend errors—they simply see "No rooms available."
Consider one of these approaches:
Option 1: Add user-visible error state
const RoomBrowser: React.FC = () => { const [rooms, setRooms] = useState<Room[]>([]); const [loading, setLoading] = useState(true); + const [error, setError] = useState<string | null>(null); const navigate = useNavigate(); const fetchRooms = async () => { const token = localStorage.getItem('token'); try { const response = await fetch('http://localhost:1313/rooms', { // ... }); if (!response.ok) { + setError('Failed to fetch rooms. Please try again.'); setLoading(false); return; } const data = await response.json(); if (!data || !Array.isArray(data)) { + setError('Invalid room data received.'); setRooms([]); return; } // ... + setError(null); setRooms(normalizedRooms); } catch (error) { + setError('Unable to connect to the server.'); } finally { setLoading(false); } };Then display the error in the UI:
{loading ? ( <p>Loading rooms...</p> ) : error ? ( <p className='text-center text-destructive'>{error}</p> ) : rooms.length === 0 ? ( <p>No rooms available at the moment.</p> ) : ( // table )}Option 2: Let error boundaries handle it
} catch (error) { + throw error; // Let error boundary catch and display } finally {frontend/src/Pages/Profile.tsx (1)
283-332: Critical: Rollback logic doesn't preserve the previous avatar URL.The optimistic update on Line 294 mutates
dashboard.profile.avatarUrlto the new value. When the API call fails, the rollback attempts to revert usingdashboard.profile.avatarUrl(lines 317 and 328), but this already contains the new URL from the optimistic update. This means the UI incorrectly displays the new avatar even when the backend update fails.Apply this diff to capture the previous value before the optimistic update:
const handleAvatarSelect = async (avatarUrl: string) => { if (!dashboard?.profile) return; const token = getAuthToken(); if (!token) { setErrorMessage("Authentication token is missing."); return; } try { + // Capture the previous avatar URL before optimistic update + const previousAvatarUrl = dashboard.profile.avatarUrl; + // Optimistically update the local state setDashboard({ ...dashboard, profile: { ...dashboard.profile, avatarUrl }, }); const response = await updateProfile( token, dashboard.profile.displayName, dashboard.profile.bio, dashboard.profile.twitter, dashboard.profile.instagram, dashboard.profile.linkedin, avatarUrl ); if (response.success) { setSuccessMessage("Avatar updated successfully!"); setErrorMessage(""); } else { setErrorMessage(response.error?.message || "Failed to update avatar."); // Revert state on failure setDashboard({ ...dashboard, profile: { ...dashboard.profile, - avatarUrl: dashboard.profile.avatarUrl, + avatarUrl: previousAvatarUrl, }, }); } } catch (err) { setErrorMessage("Failed to update avatar."); - // Optionally revert state on failure + // Revert state on failure setDashboard({ ...dashboard, profile: { ...dashboard.profile, - avatarUrl: dashboard.profile.avatarUrl, + avatarUrl: previousAvatarUrl, }, }); } };frontend/src/Pages/TournamentHub.tsx (1)
1-299: Verify PR scope: changes appear unrelated to stated objectives.The PR objectives describe fixes to error boundaries (
useErrorBoundary.ts) and Tailwind configuration (tailwind.config.cjs), but this file contains unrelated changes:
- Loading state implementation
- Skeleton loader UI
- Quote style changes
These appear to be feature additions or UX improvements rather than the critical error boundary and Tailwind fixes described in the PR summary. Please verify whether:
- These changes should be in a separate PR
- The PR description needs updating
- The intended error boundary/Tailwind config files are missing from this review
frontend/src/Pages/OnlineDebateRoom.tsx (2)
626-653: Replace hardcoded URL with environment variable.Line 631 uses hardcoded
http://localhost:1313instead ofimport.meta.env.VITE_BASE_URL.Apply this diff:
- const response = await fetch(`http://localhost:1313/rooms`, { + const response = await fetch(`${import.meta.env.VITE_BASE_URL}/rooms`, { method: "POST",
844-1047: Replace hardcoded WebSocket URL and remove debug logging.
- Line 849 uses hardcoded
ws://localhost:1313- derive from environment variable:const wsUrl = import.meta.env.VITE_BASE_URL.replace(/^http/, 'ws');- Lines 876, 939 contain
console.debugcalls that should be removed for productionApply this diff:
+ const wsUrl = import.meta.env.VITE_BASE_URL.replace(/^http/, 'ws'); const ws = new WebSocket( - `ws://localhost:1313/ws?room=${roomId}&token=${token}` + `${wsUrl}/ws?room=${roomId}&token=${token}` );backend/controllers/debatevsbot_controller.go (2)
162-163: Critical: Database error silently ignored.The error from
SaveDebateVsBotis caught but not handled. This can mask database failures and lead to data inconsistency issues where the debate state is not persisted.Handle the error appropriately:
if err := db.SaveDebateVsBot(debate); err != nil { + log.Printf("Failed to save debate: %v", err) + // Consider whether to return an error response to the client }
207-208: Critical: Database error silently ignored.The error from
UpdateDebateVsBotOutcomeis ignored, which means the debate result might not be persisted. This could lead to incorrect user statistics and rating calculations.Log or handle the error:
if err := db.UpdateDebateVsBotOutcome(email, result); err != nil { + log.Printf("Failed to update debate outcome: %v", err) }backend/websocket/matchmaking.go (1)
119-126: Clean up the WebSocket client when AddToPool fails.We upgrade to WebSocket, stash the client in
matchmakingRoom.clients, and then callAddToPool. On failure we return immediately viac.String(...), but the connection stays open, the client remains in the map, and no read/write pump ever runs. That leaves a dangling connection/channel and leaks the entry, so the next broadcast can block once the buffered channel fills. Close the socket, drop the client frommatchmakingRoom.clients, and shut downclient.send(and send the error over the socket before closing) before returning.Apply this diff:
- 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 { + errorPayload, _ := json.Marshal(MatchmakingMessage{ + Type: "error", + Error: "Failed to join matchmaking", + }) + matchmakingRoom.mutex.Lock() + delete(matchmakingRoom.clients, client) + matchmakingRoom.mutex.Unlock() + close(client.send) + _ = conn.WriteMessage(websocket.TextMessage, errorPayload) + _ = conn.Close() + return + }
🧹 Nitpick comments (32)
frontend/src/Pages/StrengthenArgument.tsx (1)
134-142: Consider preserving error logging for debugging purposes.While the user-facing error handling via
setErrorandnotifyis appropriate, completely removing console logging may hinder debugging and monitoring. Consider adding conditional logging based on environment:} catch (err: any) { + if (process.env.NODE_ENV === 'development') { + console.error('Error fetching weak statement:', err); + } const errorMessage = err.message.includes("invalid character") ? "Server returned invalid data. Please try again or contact support." : err.message; setError(errorMessage); notify("Error", errorMessage, "destructive"); } finally {Apply similar changes to the
handleSubmitcatch block:} catch (err: any) { + if (process.env.NODE_ENV === 'development') { + console.error('Error submitting argument:', err); + } setError(err.message); notify("Error", err.message, "destructive"); } finally {This approach maintains clean production logs while preserving valuable debugging information during development.
Also applies to: 178-183
frontend/src/Pages/TournamentHub.tsx (2)
16-46: Consider renaminginitialTournamentsfor clarity.The constant
initialTournamentsis no longer used to initialize state (line 46 now uses an empty array). The name is misleading since it's only referenced inside theuseEffectto simulate API data. Consider renaming tomockTournamentDataortournamentFixturesto better reflect its purpose.Apply this diff:
- const initialTournaments: Tournament[] = [ + const mockTournamentData: Tournament[] = [ { id: "1", name: "Spring Showdown", ... }, ]; const [tournaments, setTournaments] = useState<Tournament[]>([]); const [isLoading, setIsLoading] = useState(true); ... useEffect(() => { const timer = setTimeout(() => { - setTournaments(initialTournaments); + setTournaments(mockTournamentData); setIsLoading(false); }, 1000);
55-63: Consider extracting the hardcoded timeout value.The 1000ms delay is a magic number. While acceptable for a simulated API call, extracting it to a named constant would improve maintainability if you need to adjust the delay later.
+ const MOCK_API_DELAY = 1000; + useEffect(() => { // Simulate API call const timer = setTimeout(() => { setTournaments(initialTournaments); setIsLoading(false); - }, 1000); + }, MOCK_API_DELAY); return () => clearTimeout(timer); }, []);frontend/src/Pages/MatchLogs.tsx (2)
110-112: Hardcoded player names reduce maintainability.The
isFirstRoundMatch3flag checks for an exact match string including specific player names. This approach is brittle and won't work for:
- Different tournaments or matches
- Dynamic data from a backend
- Any tiebreaker scenario with different players
If this is production code, consider making the tiebreaker logic data-driven:
+interface MatchLog { + match: string; + score?: Score; + timestamp: string; + duration: string; + viewers: number; + isTiebreaker?: boolean; +}Then check the flag instead of hardcoding the match string:
-const isFirstRoundMatch3 = log.match.startsWith( - "First Round Match 3: Ayaan Khanna vs Vivaan Sharma" -); +const isTiebreaker = log.isTiebreaker || false;
227-231: Logic fix corrects broken tiebreaker display.The previous condition
stage === "First Round Match 3"would never be true sincestageis only set to "First Round", "Semifinal", or "Final" (lines 105-109). The newisFirstRoundMatch3flag correctly displays the tiebreaker note.However, this fix is limited to the hardcoded match string and won't generalize to other tiebreaker scenarios.
frontend/src/Pages/TournamentBracketPage.tsx (2)
14-32: Mock data implementation needs API integration path.The simulated API call with hard-coded data is fine for development, but consider:
- No error handling for when this becomes a real API call
- External dependency on
pravatar.ccmay fail in production- Missing loading error states
Consider adding error handling now to make the transition to real API smoother:
useEffect(() => { - // Simulate API call const timer = setTimeout(() => { - const data: Participant[] = [ - { id: 1, name: "Rishit Tiwari", avatar: `https://i.pravatar.cc/32?u=1` }, - // ... rest of mock data - ]; - setParticipants(data); - setIsLoading(false); + try { + // TODO: Replace with actual API call + const data: Participant[] = [ + { id: 1, name: "Rishit Tiwari", avatar: `https://i.pravatar.cc/32?u=1` }, + // ... rest of mock data + ]; + setParticipants(data); + } catch (error) { + console.error('Failed to load tournament data:', error); + // TODO: Set error state + } finally { + setIsLoading(false); + } }, 1000); return () => clearTimeout(timer); }, []);
96-156: Rendering structure is well-implemented.The semifinals, first round, and match labels are properly structured with:
- Correct grid layouts for each round
- Proper participant pairing logic using arithmetic
- Appropriate winner highlighting
- Well-positioned bracket connectors
Consider making match labels dynamic if match metadata (e.g., match IDs, times, or scores) becomes available:
{/* Match Labels */} <div className="w-full grid grid-cols-4 gap-2 mt-4"> - <div className="text-center text-[10px] font-medium text-primary-foreground">Match 1</div> - <div className="text-center text-[10px] font-medium text-primary-foreground">Match 2</div> - <div className="text-center text-[10px] font-medium text-primary-foreground">Match 3</div> - <div className="text-center text-[10px] font-medium text-primary-foreground">Match 4</div> + {[1, 2, 3, 4].map((matchNum) => ( + <div key={matchNum} className="text-center text-[10px] font-medium text-primary-foreground"> + Match {matchNum} + </div> + ))} </div>frontend/src/Pages/OnlineDebateRoom.tsx (2)
1174-1382: LGTM: Comprehensive speech recognition error handling.The retry logic properly handles transient network errors (lines 1291-1322) and distinguishes between recoverable and non-recoverable errors. The live transcript feature (lines 1241-1252) enhances user experience.
Consider extracting the retry logic into a separate utility function to reduce complexity of the
onerrorhandler.
1667-1671: Remove empty useEffect hooks.Lines 1667-1671 contain
useEffecthooks with empty bodies that serve no purpose and should be removed to reduce code clutter.Apply this diff:
- // Debug user state changes - useEffect(() => {}, [currentUser]); - - useEffect(() => {}, [localUser]); - - useEffect(() => {}, [opponentUser]);backend/services/gemini.go (1)
21-30: Add context timeout to GenerateContent call.The
generateModelTextfunction usesctxbut the Gemini API call might hang indefinitely if no timeout is set on the context.Consider adding a timeout if the caller doesn't provide one:
func generateModelText(ctx context.Context, modelName, prompt string) (string, error) { if geminiClient == nil { return "", errors.New("gemini client not initialized") } + // Add timeout if context doesn't have one + if _, hasDeadline := ctx.Deadline(); !hasDeadline { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, 30*time.Second) + defer cancel() + } resp, err := geminiClient.Models.GenerateContent(ctx, modelName, genai.Text(prompt), nil)frontend/src/components/JudgementPopup.tsx (3)
129-182: Simplify the complex name/avatar derivation logic.The nested ternary expressions and interleaved logic for deriving player names and avatars are difficult to follow and maintain. Consider extracting this into separate helper functions or using a lookup table approach.
Example refactor:
const getPlayerInfo = ( isUserBotFormat: boolean, localRole: DebateSide | null, side: 'for' | 'against' ) => { if (isUserBotFormat) { return side === 'for' ? { name: userName, avatar: userAvatar || localAvatar } : { name: botName || 'Bot', avatar: botAvatar || opponentAvatar }; } const isLocalSide = localRole === side; return { name: isLocalSide ? resolvedLocalName : resolvedOpponentName, avatar: isLocalSide ? derivedLocalAvatar : derivedOpponentAvatar }; }; const player1Info = getPlayerInfo(isUserBotFormat, localRole, 'for'); const player2Info = getPlayerInfo(isUserBotFormat, localRole, 'against');Additionally, add error handling for localStorage access:
- const localAvatar = - localStorage.getItem('userAvatar') || - 'https://avatar.iran.liara.run/public/40'; + const localAvatar = (() => { + try { + return localStorage.getItem('userAvatar') || 'https://avatar.iran.liara.run/public/40'; + } catch { + return 'https://avatar.iran.liara.run/public/40'; + } + })();
198-261: Consider simplifying the getScoreAndReason function.The function contains two nearly identical switch statements with duplicate case handling. This could be simplified to reduce maintenance burden.
Example refactor using a lookup approach:
const getScoreAndReason = ( section: string, player: 'player1' | 'player2' ) => { const data = judgment as any; const key = isUserBotFormat ? (player === 'player1' ? 'user' : 'bot') : (player === 'player1' ? 'for' : 'against'); // Handle special mapping for ForAgainst format const sectionKey = !isUserBotFormat && section === 'cross_examination' ? null // ForAgainst splits this into _questions and _answers : section; if (!sectionKey || !data[sectionKey]) { return { score: 0, reason: 'Data not available' }; } if (section === 'total') { return { score: data.total[key], reason: '' }; } return { score: data[sectionKey][key].score, reason: data[sectionKey][key].reason }; };Note: This refactor requires careful testing to ensure all section mappings remain correct.
1-747: Consider using automated formatting tools for style changes.The entire file has been converted from double quotes to single quotes. While consistency is valuable, style changes like this are better handled by automated tools (Prettier, ESLint with
--fix) rather than manual edits. This keeps the diff focused on functional changes and prevents review fatigue.For future PRs, consider:
- Setting up Prettier/ESLint for automatic formatting
- Separating style changes into dedicated formatting commits
- Using pre-commit hooks to enforce style consistently
frontend/src/components/TeamChatSidebar.tsx (2)
8-15: Consider renaming the interface toTeamChatMessagefor clarity.The name
ChatMessageconflicts with the existing interface infrontend/src/components/Chatbox.tsx(lines 4-10), which has different fields. While they serve different purposes, using distinct names prevents confusion and potential type errors during refactoring.Apply this diff:
-interface ChatMessage { +interface TeamChatMessage { id: string; userId: string; email: string; displayName: string; message: string; timestamp: string; }And update the type references:
- const [messages, setMessages] = useState<ChatMessage[]>([]); + const [messages, setMessages] = useState<TeamChatMessage[]>([]);- const newChatMessage: ChatMessage = { + const newChatMessage: TeamChatMessage = {
157-161: Reliance on external avatar service may cause issues.Line 159 uses an external avatar service (
https://avatar.iran.liara.run/public/${msg.userId}) that could be unavailable, blocked by corporate networks, or fail. While theAvatarFallbackprovides a fallback, consider:
- Using a self-hosted avatar generation library (e.g.,
@dicebear/core)- Generating avatar URLs from a backend endpoint you control
- Using initials-based avatars consistently without external dependencies
Example using DiceBear (if added as dependency):
import { createAvatar } from '@dicebear/core'; import { initials } from '@dicebear/collection'; // In the component const avatarSvg = createAvatar(initials, { seed: msg.userId, }).toString(); // In the JSX <AvatarImage src={`data:image/svg+xml;utf8,${encodeURIComponent(avatarSvg)}`} />backend/services/rating_service.go (1)
132-145: Consider validating fallback parameters tosanitizePlayerStats.While the function sanitizes player stats against NaN/Inf values, the
fallbackRatingandfallbackRDparameters themselves could theoretically be NaN or Inf if passed incorrectly. Consider adding validation:func sanitizePlayerStats(player *rating.Player, fallbackRating, fallbackRD float64) { + // Validate fallback values themselves + if math.IsNaN(fallbackRating) || math.IsInf(fallbackRating, 0) { + fallbackRating = 1200.0 + } + if math.IsNaN(fallbackRD) || math.IsInf(fallbackRD, 0) { + fallbackRD = 350.0 + } + if math.IsNaN(player.Rating) || math.IsInf(player.Rating, 0) { player.Rating = fallbackRating } // ... rest of function }frontend/src/components/ErrorBoundary/RouteErrorBoundary.tsx (1)
40-54: Consider removing console logging in production when error tracking is implemented.Line 52 uses
console.errorto log errors in production. Once the error tracking service is integrated (as indicated by the comment on line 53), this console logging should be removed to avoid duplicate logs and potential sensitive data exposure.private logRouteError = (error: Error, errorInfo: ErrorInfo) => { const errorData = { type: 'route_error', route: this.props.routeName || 'unknown', message: error.message, stack: error.stack, componentStack: errorInfo.componentStack, timestamp: new Date().toISOString(), url: window.location.href, pathname: window.location.pathname, }; - console.error('Route error logged:', errorData); - // In production, send to error tracking service + // Send to error tracking service (e.g., Sentry, LogRocket) + // errorTrackingService.logError(errorData); };frontend/src/components/ReactionBar.tsx (1)
57-60: Consider removing the localStorage fallback.The spectatorHash atom already handles localStorage retrieval and fallback generation (lines 33-48 in frontend/src/atoms/debateAtoms.ts). The fallback here duplicates that logic and could lead to inconsistencies if the atom's initialization logic changes.
Apply this diff to simplify:
const handleReaction = (emoji: string) => { - const storedHash = - spectatorHash || localStorage.getItem('spectatorHash') || ''; - if (!debateId || !storedHash) return; + if (!debateId || !spectatorHash) return; const payload = { reaction: emoji, - spectatorHash: storedHash, + spectatorHash: spectatorHash, timestamp: Date.now(), };frontend/src/Pages/TeamBuilder.tsx (1)
291-294: Missing error handling for clipboard API.The clipboard.writeText call can fail due to permissions or browser compatibility. Consider adding error handling to provide user feedback.
const handleCopyTeamCode = (code: string) => { - navigator.clipboard.writeText(code); - setSuccess(`Team code ${code} copied to clipboard!`); - setTimeout(() => setSuccess(""), 3000); + navigator.clipboard.writeText(code) + .then(() => { + setSuccess(`Team code ${code} copied to clipboard!`); + setTimeout(() => setSuccess(""), 3000); + }) + .catch(() => { + setError("Failed to copy to clipboard"); + setTimeout(() => setError(""), 3000); + }); };backend/internal/debate/redis_client.go (3)
10-13: Package-level background context limits cancellation control.The shared
context.Background()at line 12 means all Redis operations use the same uncancellable context. This prevents proper request-scoped cancellation and could lead to resource leaks if long-running operations aren't cancelled when requests are abandoned.Consider these options:
- Option 1: Accept context as a parameter in operations:
func GetRedisClient() *redis.Client { return rdb } // Callers pass their own context to Redis operations // rdb.Get(ctx, key)
- Option 2: Remove GetContext() and document that callers should use request contexts:
-var ( - rdb *redis.Client - ctx = context.Background() -) +var rdb *redis.Client -// GetContext returns the default context -func GetContext() context.Context { - return ctx -}The first option is preferred as it allows proper cancellation propagation from HTTP handlers.
34-37: GetRedisClient can return nil without indication.If
InitRedishasn't been called or failed,GetRedisClient()returnsnilwithout any error indication. This could cause nil pointer panics in calling code.Consider adding nil check protection:
// GetRedisClient returns the Redis client instance func GetRedisClient() *redis.Client { + if rdb == nil { + panic("Redis client not initialized. Call InitRedis first.") + } return rdb }Or change signature to return an error:
func GetRedisClient() (*redis.Client, error) { if rdb == nil { return nil, fmt.Errorf("redis client not initialized") } return rdb, nil }
15-32: LGTM with suggestion - Consider idempotent initialization.The Redis initialization logic is correct and includes proper connection testing with the Ping command.
For robustness, consider making InitRedis idempotent:
func InitRedis(redisURL string, password string, db int) error { + if rdb != nil { + return nil // Already initialized + } + opt := &redis.Options{This prevents accidental double-initialization and makes the function safe to call multiple times.
backend/controllers/team_matchmaking.go (1)
32-54: Consider refactoring to avoid duplicate database queries and logic.The team is fetched from the database and its fullness is checked here (lines 35-51), but
services.StartTeamMatchmakingperforms the same operations again (fetching the team and checking fullness). This results in duplicate database calls and redundant validation logic.Consider refactoring the service layer to return a more descriptive error that allows you to distinguish between "team not found," "team not full," and other failures, or pass the already-fetched team to the service to avoid the duplicate query.
Example refactor:
- // Get team - collection := db.GetCollection("teams") - 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 - } - - // Check if user is captain - if team.CaptainID != userID.(primitive.ObjectID) { - c.JSON(http.StatusForbidden, gin.H{"error": "Only the captain can join matchmaking"}) - return - } - - // Check if team is full - if len(team.Members) < team.MaxSize { - c.JSON(http.StatusBadRequest, gin.H{"error": "Team is not full yet"}) - return - } - - // Add to matchmaking - err = services.StartTeamMatchmaking(objectID) + // Add to matchmaking (validates captain, fullness, and fetches team) + team, err := services.StartTeamMatchmaking(objectID, userID.(primitive.ObjectID)) if err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to join matchmaking"}) + // Handle specific error types + if err == services.ErrNotCaptain { + c.JSON(http.StatusForbidden, gin.H{"error": "Only the captain can join matchmaking"}) + } else if err == services.ErrTeamNotFull { + c.JSON(http.StatusBadRequest, gin.H{"error": "Team is not full yet"}) + } else if err == mongo.ErrNoDocuments { + c.JSON(http.StatusNotFound, gin.H{"error": "Team not found"}) + } else { + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to join matchmaking"}) + } return }frontend/src/components/Matchmaking.tsx (2)
71-118: Consider logging errors in the catch block.The message handling is well-structured with proper type checking and state updates. However, the catch block at line 116-117 silently swallows all errors, making debugging difficult if message parsing fails or state updates throw errors.
} catch (error) { + console.error('Error processing matchmaking message:', error); }
143-176: Good connection state handling with retry logic.The enhanced connection handling properly checks WebSocket readyState and provides user feedback. The retry logic for CONNECTING state is a good user experience improvement.
One minor consideration: the
setTimeoutat line 162 isn't cleaned up if the component unmounts during the 1-second delay. This could lead to attempting to send messages on a closed WebSocket.If you want to make it more robust:
} else if (readyState === WebSocket.CONNECTING) { // Wait a bit and try again - setTimeout(() => { + const timeoutId = setTimeout(() => { if (wsRef.current && wsRef.current.readyState === WebSocket.OPEN) { wsRef.current.send(JSON.stringify({ type: 'join_pool' })); } else { alert( 'Connection is taking too long. Please check your internet connection and try again.' ); } }, 1000); + // Store timeoutId if you want to clean it up on unmount } else {frontend/src/Pages/ViewDebate.tsx (1)
262-271: Consider logging autoplay errors for debugging.The empty catch blocks when calling
.play()are likely intentional to handle browser autoplay restrictions, but silently swallowing these errors makes debugging difficult.if (video1Ref.current && remoteStream1) { video1Ref.current.srcObject = remoteStream1; - video1Ref.current.play().catch(() => {}); + video1Ref.current.play().catch((error) => { + console.warn('Autoplay prevented for video 1:', error); + }); } if (video2Ref.current && remoteStream2) { video2Ref.current.srcObject = remoteStream2; - video2Ref.current.play().catch(() => {}); + video2Ref.current.play().catch((error) => { + console.warn('Autoplay prevented for video 2:', error); + }); }backend/services/team_matchmaking.go (1)
44-58: Consider checking for duplicate entries before adding to the pool.The function doesn't check if the team is already in the matchmaking pool before adding it. While the map will simply overwrite the existing entry, this resets the timestamp, which could affect matchmaking fairness (longer-waiting teams might lose their priority).
teamMatchmakingMutex.Lock() defer teamMatchmakingMutex.Unlock() // Initialize pool if it doesn't exist if teamMatchmakingPool == nil { teamMatchmakingPool = make(map[string]*TeamMatchmakingEntry) } + + // Check if team is already in the pool + if _, exists := teamMatchmakingPool[teamID.Hex()]; exists { + return errors.New("team is already in matchmaking pool") + } teamMatchmakingPool[teamID.Hex()] = &TeamMatchmakingEntry{backend/controllers/team_controller.go (2)
67-71: Add type assertion safety checks.Type assertions without checks (line 83) will panic if the auth middleware doesn't set the expected type. While unlikely, defensive programming suggests using the comma-ok idiom.
- userID, exists := c.Get("userID") - if !exists { - c.JSON(http.StatusUnauthorized, gin.H{"error": "User not authenticated"}) - return - } + userID, exists := c.Get("userID") + if !exists { + c.JSON(http.StatusUnauthorized, gin.H{"error": "User not authenticated"}) + return + } + userObjectID, ok := userID.(primitive.ObjectID) + if !ok { + c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid user ID type"}) + return + } // Check if user is captain - if team.CaptainID != userID.(primitive.ObjectID) { + if team.CaptainID != userObjectID {This pattern applies throughout the file wherever type assertions are used (lines 83, 146, 199, 207-208, 282-283, 288-289, 298, 316-317, 367-368, 383, 390, 398, 421-422, 463-464, 479, 530-531, 546).
Also applies to: 83-86
383-386: Clarify captain leave/transfer captaincy flow.The error message directs captains to "Transfer captaincy first," but no
TransferCaptaincyendpoint is visible in this controller. This creates a poor UX if the captain wants to leave.Consider one of these approaches:
- Allow captain to leave if they're the only member (and automatically delete the team):
if team.CaptainID == userID.(primitive.ObjectID) { - c.JSON(http.StatusBadRequest, gin.H{"error": "Captain cannot leave team. Transfer captaincy first."}) - return + if len(team.Members) == 1 { + // Captain is the only member, delete the team + _, err := collection.DeleteOne(context.Background(), bson.M{"_id": objectID}) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to leave team"}) + return + } + c.JSON(http.StatusOK, gin.H{"message": "Successfully left team (team deleted)"}) + return + } + c.JSON(http.StatusBadRequest, gin.H{"error": "Captain cannot leave team. Transfer captaincy or delete the team."}) + return }
- Implement a
TransferCaptaincyendpoint if not already present.Do you want me to generate the transfer captaincy implementation?
backend/cmd/server/ws_debate_handler.go (3)
157-162: Silent error suppression may hide connection failures.Write errors are completely ignored in the broadcast loop. While this prevents one bad connection from blocking others, it means dead connections accumulate and are never cleaned up.
Consider logging errors and/or closing failed connections:
// Broadcast to all clients for _, client := range clients { if err := client.WriteJSON(eventData); err != nil { + // Log the error + // Optionally: close and unregister the failed connection + go hub.Unregister(debateID, client.conn) } }
286-288: Empty "join" case handler.The "join" message type is parsed but has no implementation. If this is intentional (join is implicit via connection), consider adding a comment. If not, implement the handler or remove the case.
switch clientMsg.Type { case "join": + // Join is implicit via WebSocket connection; no action needed case "vote":
299-358: Error handling is too silent throughout handlers.All error paths in
handleVote(and similarly inhandleQuestionandhandleReaction) return silently without:
- Logging the error for debugging
- Sending feedback to the client
This makes it impossible to diagnose why votes/questions/reactions fail from either the client or server side.
Add error logging at minimum:
func handleVote(client *SpectatorClient, payloadBytes []byte) { var payload debate.VotePayload if err := json.Unmarshal(payloadBytes, &payload); err != nil { + log.Printf("Failed to unmarshal vote payload: %v", err) return } // Set spectator hash and timestamp payload.SpectatorHash = client.spectatorHash if payload.ClientEventID == "" { payload.ClientEventID = uuid.New().String() } payload.Timestamp = time.Now().Unix() // Check rate limit rateLimiter := debate.NewRateLimiter() canVote, err := rateLimiter.CheckVoteRateLimit(client.debateID, payload.PollID, client.spectatorHash) if err != nil || !canVote { + if err != nil { + log.Printf("Vote rate limit check failed: %v", err) + } return }Consider also sending error responses to the client via WebSocket when operations fail.
| // Connect to Redis if configured | ||
| if cfg.Redis.URL != "" { | ||
| redisURL := cfg.Redis.URL | ||
| if redisURL == "" { | ||
| redisURL = "localhost:6379" | ||
| } | ||
| if err := debate.InitRedis(redisURL, cfg.Redis.Password, cfg.Redis.DB); err != nil { | ||
| panic("Failed to initialize Redis: " + err.Error()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant empty check in Redis URL fallback logic.
Lines 38-41 check if redisURL is empty after already assigning it from cfg.Redis.URL on line 38. If cfg.Redis.URL is empty, this condition (line 37) wouldn't be entered in the first place, making the fallback logic unreachable.
Remove the redundant check:
// Connect to Redis if configured
if cfg.Redis.URL != "" {
redisURL := cfg.Redis.URL
- if redisURL == "" {
- redisURL = "localhost:6379"
- }
if err := debate.InitRedis(redisURL, cfg.Redis.Password, cfg.Redis.DB); err != nil {
panic("Failed to initialize Redis: " + err.Error())
}
}If you want a fallback for when cfg.Redis.URL is empty, change the logic:
- if cfg.Redis.URL != "" {
- redisURL := cfg.Redis.URL
- if redisURL == "" {
- redisURL = "localhost:6379"
- }
+ redisURL := cfg.Redis.URL
+ if redisURL == "" {
+ redisURL = "localhost:6379"
+ }
+ if redisURL != "" {
if err := debate.InitRedis(redisURL, cfg.Redis.Password, cfg.Redis.DB); err != nil {
panic("Failed to initialize Redis: " + err.Error())
}
}📝 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.
| // Connect to Redis if configured | |
| if cfg.Redis.URL != "" { | |
| redisURL := cfg.Redis.URL | |
| if redisURL == "" { | |
| redisURL = "localhost:6379" | |
| } | |
| if err := debate.InitRedis(redisURL, cfg.Redis.Password, cfg.Redis.DB); err != nil { | |
| panic("Failed to initialize Redis: " + err.Error()) | |
| } | |
| } | |
| // Connect to Redis if configured | |
| if cfg.Redis.URL != "" { | |
| redisURL := cfg.Redis.URL | |
| if err := debate.InitRedis(redisURL, cfg.Redis.Password, cfg.Redis.DB); err != nil { | |
| panic("Failed to initialize Redis: " + err.Error()) | |
| } | |
| } |
🤖 Prompt for AI Agents
In backend/cmd/server/main.go around lines 36 to 45, the inner empty check for
redisURL is redundant because the outer if only runs when cfg.Redis.URL != "".
Replace the current conditional with logic that sets redisURL := cfg.Redis.URL;
if redisURL == "" set redisURL = "localhost:6379"; then unconditionally call
debate.InitRedis(redisURL, cfg.Redis.Password, cfg.Redis.DB) (or alternatively
keep the outer if and simply remove the inner check and use cfg.Redis.URL
directly) so the fallback behavior is correct and the redundant branch is
removed.
| "message": "Token is valid", | ||
| "user": gin.H{ | ||
| "id": user.ID.Hex(), | ||
| "email": user.Email, | ||
| "displayName": user.DisplayName, | ||
| "nickname": user.Nickname, | ||
| "bio": user.Bio, | ||
| "rating": user.Rating, | ||
| "rd": user.RD, | ||
| "volatility": user.Volatility, | ||
| "id": user.ID.Hex(), | ||
| "email": user.Email, | ||
| "displayName": user.DisplayName, | ||
| "nickname": user.Nickname, | ||
| "bio": user.Bio, | ||
| "rating": user.Rating, | ||
| "rd": user.RD, | ||
| "volatility": user.Volatility, | ||
| "lastRatingUpdate": user.LastRatingUpdate.Format(time.RFC3339), | ||
| "avatarUrl": user.AvatarURL, | ||
| "twitter": user.Twitter, | ||
| "instagram": user.Instagram, | ||
| "linkedin": user.LinkedIn, | ||
| "isVerified": user.IsVerified, | ||
| "createdAt": user.CreatedAt.Format(time.RFC3339), | ||
| "updatedAt": user.UpdatedAt.Format(time.RFC3339), | ||
| "avatarUrl": user.AvatarURL, | ||
| "twitter": user.Twitter, | ||
| "instagram": user.Instagram, | ||
| "linkedin": user.LinkedIn, | ||
| "isVerified": user.IsVerified, | ||
| "createdAt": user.CreatedAt.Format(time.RFC3339), | ||
| "updatedAt": user.UpdatedAt.Format(time.RFC3339), | ||
| }, |
There was a problem hiding this comment.
Sanitize stats before responding from VerifyToken
Line [514] still returns raw rating/rd/volatility values. If any of them are NaN/Inf (the exact failure case that motivated normalizeUserStats), encoding/json rejects the payload and VerifyToken blows up with a 500. The new helpers already fix this elsewhere—please reuse them here so the token-validation path can never emit unsupported floats.
@@
- // Return user details
- ctx.JSON(200, gin.H{
- "message": "Token is valid",
- "user": gin.H{
- "id": user.ID.Hex(),
- "email": user.Email,
- "displayName": user.DisplayName,
- "nickname": user.Nickname,
- "bio": user.Bio,
- "rating": user.Rating,
- "rd": user.RD,
- "volatility": user.Volatility,
- "lastRatingUpdate": user.LastRatingUpdate.Format(time.RFC3339),
- "avatarUrl": user.AvatarURL,
- "twitter": user.Twitter,
- "instagram": user.Instagram,
- "linkedin": user.LinkedIn,
- "isVerified": user.IsVerified,
- "createdAt": user.CreatedAt.Format(time.RFC3339),
- "updatedAt": user.UpdatedAt.Format(time.RFC3339),
- },
- })
+ if normalizeUserStats(&user)) {
+ if err := persistUserStats(dbCtx, &user); err != nil {
+ }
+ }
+
+ ctx.JSON(200, gin.H{
+ "message": "Token is valid",
+ "user": buildUserResponse(user),
+ })Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/controllers/auth.go around lines 512 to 530, the response body in
VerifyToken is currently returning raw user stats (rating, rd, volatility) which
can be NaN/Inf and cause encoding/json to fail; call the existing
normalizeUserStats helper before building the gin.H user map and use the
sanitized values (or the normalized user copy) for "rating", "rd", and
"volatility" so the JSON encoder never sees unsupported floats; keep the rest of
the fields the same and ensure you replace the three raw fields with the
normalized equivalents.
| // GetMatchmakingPool returns the current matchmaking pool for debugging | ||
| func GetMatchmakingPool(c *gin.Context) { | ||
| pool := services.GetMatchmakingPool() | ||
|
|
||
| // Convert to a more readable format | ||
| var poolInfo []gin.H | ||
| for teamID, entry := range pool { | ||
| poolInfo = append(poolInfo, gin.H{ | ||
| "teamId": teamID, | ||
| "teamName": entry.Team.Name, | ||
| "captainId": entry.Team.CaptainID.Hex(), | ||
| "maxSize": entry.MaxSize, | ||
| "averageElo": entry.AverageElo, | ||
| "membersCount": len(entry.Team.Members), | ||
| "timestamp": entry.Timestamp.Format("2006-01-02 15:04:05"), | ||
| }) | ||
| } | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{ | ||
| "poolSize": len(pool), | ||
| "teams": poolInfo, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add authentication or consider the privacy implications of this endpoint.
The GetMatchmakingPool endpoint has no authentication or authorization checks, allowing anyone to view all teams currently in the matchmaking pool, including their Elo ratings, captain IDs, and timestamps. This could:
- Expose sensitive matchmaking data
- Enable users to game the system by monitoring the pool
- Violate user privacy expectations
Consider either:
- Adding authentication to restrict access to logged-in users
- Rate-limiting this endpoint
- Documenting this as a public/debug endpoint if that's the intended design
- Removing sensitive information from the response if it's meant to be public
func GetMatchmakingPool(c *gin.Context) {
+ // Check authentication if this should be a protected endpoint
+ _, exists := c.Get("userID")
+ if !exists {
+ c.JSON(http.StatusUnauthorized, gin.H{"error": "Authentication required"})
+ return
+ }
+
pool := services.GetMatchmakingPool()📝 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.
| // GetMatchmakingPool returns the current matchmaking pool for debugging | |
| func GetMatchmakingPool(c *gin.Context) { | |
| pool := services.GetMatchmakingPool() | |
| // Convert to a more readable format | |
| var poolInfo []gin.H | |
| for teamID, entry := range pool { | |
| poolInfo = append(poolInfo, gin.H{ | |
| "teamId": teamID, | |
| "teamName": entry.Team.Name, | |
| "captainId": entry.Team.CaptainID.Hex(), | |
| "maxSize": entry.MaxSize, | |
| "averageElo": entry.AverageElo, | |
| "membersCount": len(entry.Team.Members), | |
| "timestamp": entry.Timestamp.Format("2006-01-02 15:04:05"), | |
| }) | |
| } | |
| c.JSON(http.StatusOK, gin.H{ | |
| "poolSize": len(pool), | |
| "teams": poolInfo, | |
| }) | |
| } | |
| // GetMatchmakingPool returns the current matchmaking pool for debugging | |
| func GetMatchmakingPool(c *gin.Context) { | |
| // Check authentication if this should be a protected endpoint | |
| _, exists := c.Get("userID") | |
| if !exists { | |
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Authentication required"}) | |
| return | |
| } | |
| pool := services.GetMatchmakingPool() | |
| // Convert to a more readable format | |
| var poolInfo []gin.H | |
| for teamID, entry := range pool { | |
| poolInfo = append(poolInfo, gin.H{ | |
| "teamId": teamID, | |
| "teamName": entry.Team.Name, | |
| "captainId": entry.Team.CaptainID.Hex(), | |
| "maxSize": entry.MaxSize, | |
| "averageElo": entry.AverageElo, | |
| "membersCount": len(entry.Team.Members), | |
| "timestamp": entry.Timestamp.Format("2006-01-02 15:04:05"), | |
| }) | |
| } | |
| c.JSON(http.StatusOK, gin.H{ | |
| "poolSize": len(pool), | |
| "teams": poolInfo, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In backend/controllers/team_matchmaking.go around lines 103 to 125, the
GetMatchmakingPool handler currently returns sensitive team data without any
authentication or authorization; update it to require an authenticated user (and
optionally an admin role) before returning the pool: apply the existing auth
middleware or check the request context for a valid user and return 401/403 when
absent/unauthorized; if this endpoint must remain public, remove or redact
sensitive fields (averageElo, captainId, exact timestamps) and/or add
rate-limiting and clear documentation that it is a debug endpoint.
| // GetMatchmakingStatus returns the matchmaking pool status | ||
| 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 | ||
| } | ||
|
|
||
| // Try to find a match | ||
| matchingTeam, err := services.FindMatchingTeam(objectID) | ||
| if err != nil { | ||
| c.JSON(http.StatusOK, gin.H{"matched": false}) | ||
| return | ||
| } | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{ | ||
| "matched": true, | ||
| "team": matchingTeam, | ||
| "matchId": matchingTeam.ID.Hex(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add authorization to prevent unauthorized status checks.
The GetMatchmakingStatus endpoint allows any user to check the matchmaking status of any team without authorization. This could:
- Leak information about when matches are found
- Allow users to monitor other teams' matchmaking activity
- Enable strategic gaming of the matchmaking system
Consider restricting this endpoint to only the team's captain or members:
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
}
+
+ // Verify the user is authorized to check this team's status
+ userID, exists := c.Get("userID")
+ if !exists {
+ c.JSON(http.StatusUnauthorized, gin.H{"error": "User not authenticated"})
+ return
+ }
+
+ // Fetch team and verify user is captain or member
+ collection := db.GetCollection("teams")
+ var team models.Team
+ if err := collection.FindOne(context.Background(), bson.M{"_id": objectID}).Decode(&team); err != nil {
+ c.JSON(http.StatusNotFound, gin.H{"error": "Team not found"})
+ return
+ }
+
+ userObjectID := userID.(primitive.ObjectID)
+ isMember := team.CaptainID == userObjectID
+ for _, member := range team.Members {
+ if member.UserID == userObjectID {
+ isMember = true
+ break
+ }
+ }
+ if !isMember {
+ c.JSON(http.StatusForbidden, gin.H{"error": "Not authorized to check this team's status"})
+ return
+ }
// Try to find a match
matchingTeam, err := services.FindMatchingTeam(objectID)📝 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.
| // GetMatchmakingStatus returns the matchmaking pool status | |
| 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 | |
| } | |
| // Try to find a match | |
| matchingTeam, err := services.FindMatchingTeam(objectID) | |
| if err != nil { | |
| c.JSON(http.StatusOK, gin.H{"matched": false}) | |
| return | |
| } | |
| c.JSON(http.StatusOK, gin.H{ | |
| "matched": true, | |
| "team": matchingTeam, | |
| "matchId": matchingTeam.ID.Hex(), | |
| }) | |
| } | |
| // GetMatchmakingStatus returns the matchmaking pool status | |
| 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 | |
| } | |
| // Verify the user is authorized to check this team's status | |
| userID, exists := c.Get("userID") | |
| if !exists { | |
| c.JSON(http.StatusUnauthorized, gin.H{"error": "User not authenticated"}) | |
| return | |
| } | |
| // Fetch team and verify user is captain or member | |
| collection := db.GetCollection("teams") | |
| var team models.Team | |
| if err := collection.FindOne(context.Background(), bson.M{"_id": objectID}).Decode(&team); err != nil { | |
| c.JSON(http.StatusNotFound, gin.H{"error": "Team not found"}) | |
| return | |
| } | |
| userObjectID := userID.(primitive.ObjectID) | |
| isMember := team.CaptainID == userObjectID | |
| for _, member := range team.Members { | |
| if member.UserID == userObjectID { | |
| isMember = true | |
| break | |
| } | |
| } | |
| if !isMember { | |
| c.JSON(http.StatusForbidden, gin.H{"error": "Not authorized to check this team's status"}) | |
| return | |
| } | |
| // Try to find a match | |
| matchingTeam, err := services.FindMatchingTeam(objectID) | |
| if err != nil { | |
| c.JSON(http.StatusOK, gin.H{"matched": false}) | |
| return | |
| } | |
| c.JSON(http.StatusOK, gin.H{ | |
| "matched": true, | |
| "team": matchingTeam, | |
| "matchId": matchingTeam.ID.Hex(), | |
| }) | |
| } |
| Role string `json:"role" binding:"required"` | ||
| Transcripts map[string]string `json:"transcripts" binding:"required"` | ||
| OpponentRole string `json:"opponentRole"` | ||
| OpponentID string `json:"opponentId"` | ||
| OpponentEmail string `json:"opponentEmail"` | ||
| OpponentTranscripts map[string]string `json:"opponentTranscripts"` |
There was a problem hiding this comment.
Restore strict role validation to prevent orphaned transcripts
Dropping oneof=for against now lets any string through. Downstream in services.SubmitTranscripts we upsert transcripts under the raw role and later only look for "for"/"against" when deciding if both sides have submitted (see backend/services/transcriptservice.go). A bad value (e.g., "FOR" or "neutral") will never match that lookup, so the debate stays stuck at “Waiting for opponent submission” even after both sides finish. Please keep the validator (or normalize the role before passing it on) so only the expected roles persist.
🤖 Prompt for AI Agents
In backend/controllers/transcript_controller.go around lines 23 to 28, the Role
field lost its strict validation which allows arbitrary strings and leads to
orphaned transcripts; restore the validator by adding the binding tag (e.g.,
`binding:"required,oneof=for against"`) or alternatively normalize incoming role
values to the canonical "for" or "against" before forwarding to services so only
the expected roles are stored and downstream lookups succeed.
| {member.userId === team.captainId && ( | ||
| <FaCrown className="text-yellow-500" /> |
There was a problem hiding this comment.
Inconsistent captain ID comparison.
Line 648 compares member.userId === team.captainId directly without the type normalization performed in the isCaptain function (lines 238-249). This could cause false negatives if ID formats are inconsistent.
Apply the isCaptain helper for consistency:
<div className="flex items-center gap-1.5">
- {member.userId === team.captainId && (
+ {isCaptain(team) && member.userId === (typeof team.captainId === 'string' ? team.captainId : (team.captainId as any)?.$oid || String(team.captainId)) && (
<FaCrown className="text-yellow-500" />
)}Or better yet, add a captain flag to the member object during mapping for simpler comparisons.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/Pages/TeamBuilder.tsx around lines 648-649, the code compares
member.userId === team.captainId directly which is inconsistent with the
normalized comparison in the isCaptain helper (lines 238-249); change the check
to use the isCaptain helper (e.g., isCaptain(member.userId, team.captainId)) so
IDs are normalized before comparison, or alternatively set a member.isCaptain
boolean during the member mapping step and use that flag here for simpler,
consistent checks.
| interface Participant { | ||
| id: number; | ||
| name: string; | ||
| avatar: string; | ||
| } |
There was a problem hiding this comment.
Type mismatch with backend Participant struct.
The frontend Participant interface doesn't align with the backend struct. The backend uses ID (string), Username, AvatarURL, Elo, and Email, while the frontend expects id (number), name, and avatar. This mismatch will cause integration issues when replacing the mock data with real API calls.
Apply this diff to align the frontend interface with the backend:
interface Participant {
- id: number;
- name: string;
- avatar: string;
+ id: string;
+ username: string;
+ elo: number;
+ avatarUrl: string;
+ email?: string;
}You'll also need to update the mock data and all references to participant.name and participant.avatar throughout the component.
📝 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.
| interface Participant { | |
| id: number; | |
| name: string; | |
| avatar: string; | |
| } | |
| interface Participant { | |
| id: string; | |
| username: string; | |
| elo: number; | |
| avatarUrl: string; | |
| email?: string; | |
| } |
🤖 Prompt for AI Agents
In frontend/src/Pages/TournamentBracketPage.tsx around lines 4 to 8, the
Participant interface doesn't match the backend struct — change the interface to
use ID: string, Username: string, AvatarURL: string, Elo: number, Email: string;
then update the mock data to use those property names/types (IDs as strings, add
Elo and Email) and replace all usages of participant.name and participant.avatar
with participant.Username and participant.AvatarURL respectively (and update any
code assuming numeric id to use string IDs).
| // Bracket logic | ||
| const round1Winners = [participants[0], participants[2], participants[4], participants[7]]; | ||
| const semiFinalWinners = [round1Winners[0], round1Winners[3]]; | ||
| const champion = semiFinalWinners[1]; | ||
| const winnerHighlight = "ring-4 ring-yellow-400 shadow-lg transition-all duration-300"; |
There was a problem hiding this comment.
Critical: Hard-coded winner logic and missing array bounds validation.
The bracket logic has several serious issues:
- Hard-coded winners: Lines 62-64 predetermine winners using fixed array indices, making the tournament results fake and non-functional.
- No bounds checking: Accessing
participants[7]without validatingparticipants.length >= 8will cause a runtime crash. - No validation: Missing check that exactly 8 participants exist before proceeding.
Apply this diff to add validation and prepare for dynamic winner selection:
+ // Validate we have exactly 8 participants for single-elimination bracket
+ if (participants.length !== 8) {
+ return (
+ <div className="flex justify-center items-center w-full py-12">
+ <p className="text-destructive">Tournament requires exactly 8 participants. Currently have {participants.length}.</p>
+ </div>
+ );
+ }
+
// Bracket logic
- const round1Winners = [participants[0], participants[2], participants[4], participants[7]];
- const semiFinalWinners = [round1Winners[0], round1Winners[3]];
- const champion = semiFinalWinners[1];
+ // TODO: Replace with actual match results from API
+ const round1Winners = [
+ participants[0], // Match 1 winner (placeholder)
+ participants[2], // Match 2 winner (placeholder)
+ participants[4], // Match 3 winner (placeholder)
+ participants[6], // Match 4 winner (placeholder - was 7, should be 6 for match 4)
+ ];
+ const semiFinalWinners = [round1Winners[0], round1Winners[2]]; // Placeholder
+ const champion = semiFinalWinners[0]; // Placeholder
const winnerHighlight = "ring-4 ring-yellow-400 shadow-lg transition-all duration-300";Note: The original code used participants[7] for the 4th winner, but Match 4 should be participants[6] vs participants[7], so the winner should come from one of those, not automatically be participants[7].
📝 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.
| // Bracket logic | |
| const round1Winners = [participants[0], participants[2], participants[4], participants[7]]; | |
| const semiFinalWinners = [round1Winners[0], round1Winners[3]]; | |
| const champion = semiFinalWinners[1]; | |
| const winnerHighlight = "ring-4 ring-yellow-400 shadow-lg transition-all duration-300"; | |
| // Validate we have exactly 8 participants for single-elimination bracket | |
| if (participants.length !== 8) { | |
| return ( | |
| <div className="flex justify-center items-center w-full py-12"> | |
| <p className="text-destructive">Tournament requires exactly 8 participants. Currently have {participants.length}.</p> | |
| </div> | |
| ); | |
| } | |
| // Bracket logic | |
| // TODO: Replace with actual match results from API | |
| const round1Winners = [ | |
| participants[0], // Match 1 winner (placeholder) | |
| participants[2], // Match 2 winner (placeholder) | |
| participants[4], // Match 3 winner (placeholder) | |
| participants[6], // Match 4 winner (placeholder - was 7, should be 6 for match 4) | |
| ]; | |
| const semiFinalWinners = [round1Winners[0], round1Winners[2]]; // Placeholder | |
| const champion = semiFinalWinners[0]; // Placeholder | |
| const winnerHighlight = "ring-4 ring-yellow-400 shadow-lg transition-all duration-300"; |
🤖 Prompt for AI Agents
In frontend/src/Pages/TournamentBracketPage.tsx around lines 61 to 65, the code
uses hard-coded winners and unsafe indexing (e.g., participants[7]) which both
fakes results and can crash; replace this with a runtime validation that
participants is an array of exactly 8 entries (throw or render a friendly
error/placeholder if not), remove the hard-coded winner assignments and instead
compute winners from match results (e.g., derive round1Winners from outcomes of
matches [0 vs1, 2 vs3, 4 vs5, 6 vs7]), ensure you never index out of bounds by
always checking length before accessing indices, and correct the 4th match to
use participants[6] vs participants[7] (pick the winner from those two rather
than auto-selecting participants[7]).
| } else if (data.type === "offer" && data.userId && data.offer) { | ||
| const debaterIndex = participantsRef.current.findIndex( | ||
| (participant) => participant.id === data.userId | ||
| ); | ||
| let pc: RTCPeerConnection | null = null; | ||
| if (debaterIndex === 0) { | ||
| pc = pc1; | ||
| } else if (debaterIndex === 1) { | ||
| pc = pc2; | ||
| } else { | ||
| pc = pc1; | ||
| } | ||
|
|
||
| if (pc && data.offer) { | ||
| try { | ||
| if (pc === pc1) { | ||
| pc1UserIdRef.current = data.userId; | ||
| pc1ConnectionIdRef.current = data.connectionId || null; | ||
| } else if (pc === pc2) { | ||
| pc2UserIdRef.current = data.userId; | ||
| pc2ConnectionIdRef.current = data.connectionId || null; | ||
| } | ||
|
|
||
| await pc.setRemoteDescription( | ||
| new RTCSessionDescription(data.offer) | ||
| ); | ||
| const answer = await pc.createAnswer(); | ||
| await pc.setLocalDescription(answer); | ||
| // Include connectionId if provided (for spectator connections) | ||
| ws.send( | ||
| JSON.stringify({ | ||
| type: "answer", | ||
| answer, | ||
| targetUserId: data.userId, | ||
| userId: data.userId, | ||
| connectionId: data.connectionId, // Include connectionId if provided | ||
| }) | ||
| ); | ||
| } catch (error) { | ||
| console.error("Error handling offer", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback logic may cause connection issues for unexpected debater indices.
At line 193, if the debaterIndex is neither 0 nor 1, the code falls back to using pc1. This could cause:
- Two debaters being assigned to the same peer connection
- Overwriting existing connections
- Incorrect video stream routing
Consider rejecting offers from unexpected participants instead of silently falling back:
const debaterIndex = participantsRef.current.findIndex(
(participant) => participant.id === data.userId
);
let pc: RTCPeerConnection | null = null;
if (debaterIndex === 0) {
pc = pc1;
} else if (debaterIndex === 1) {
pc = pc2;
- } else {
- pc = pc1;
+ } else {
+ console.error('Received offer from unexpected participant:', data.userId);
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (data.type === "offer" && data.userId && data.offer) { | |
| const debaterIndex = participantsRef.current.findIndex( | |
| (participant) => participant.id === data.userId | |
| ); | |
| let pc: RTCPeerConnection | null = null; | |
| if (debaterIndex === 0) { | |
| pc = pc1; | |
| } else if (debaterIndex === 1) { | |
| pc = pc2; | |
| } else { | |
| pc = pc1; | |
| } | |
| if (pc && data.offer) { | |
| try { | |
| if (pc === pc1) { | |
| pc1UserIdRef.current = data.userId; | |
| pc1ConnectionIdRef.current = data.connectionId || null; | |
| } else if (pc === pc2) { | |
| pc2UserIdRef.current = data.userId; | |
| pc2ConnectionIdRef.current = data.connectionId || null; | |
| } | |
| await pc.setRemoteDescription( | |
| new RTCSessionDescription(data.offer) | |
| ); | |
| const answer = await pc.createAnswer(); | |
| await pc.setLocalDescription(answer); | |
| // Include connectionId if provided (for spectator connections) | |
| ws.send( | |
| JSON.stringify({ | |
| type: "answer", | |
| answer, | |
| targetUserId: data.userId, | |
| userId: data.userId, | |
| connectionId: data.connectionId, // Include connectionId if provided | |
| }) | |
| ); | |
| } catch (error) { | |
| console.error("Error handling offer", error); | |
| } | |
| } | |
| } else if (data.type === "offer" && data.userId && data.offer) { | |
| const debaterIndex = participantsRef.current.findIndex( | |
| (participant) => participant.id === data.userId | |
| ); | |
| let pc: RTCPeerConnection | null = null; | |
| if (debaterIndex === 0) { | |
| pc = pc1; | |
| } else if (debaterIndex === 1) { | |
| pc = pc2; | |
| } else { | |
| console.error('Received offer from unexpected participant:', data.userId); | |
| return; | |
| } | |
| if (pc && data.offer) { | |
| try { | |
| if (pc === pc1) { | |
| pc1UserIdRef.current = data.userId; | |
| pc1ConnectionIdRef.current = data.connectionId || null; | |
| } else if (pc === pc2) { | |
| pc2UserIdRef.current = data.userId; | |
| pc2ConnectionIdRef.current = data.connectionId || null; | |
| } | |
| await pc.setRemoteDescription( | |
| new RTCSessionDescription(data.offer) | |
| ); | |
| const answer = await pc.createAnswer(); | |
| await pc.setLocalDescription(answer); | |
| // Include connectionId if provided (for spectator connections) | |
| ws.send( | |
| JSON.stringify({ | |
| type: "answer", | |
| answer, | |
| targetUserId: data.userId, | |
| userId: data.userId, | |
| connectionId: data.connectionId, // Include connectionId if provided | |
| }) | |
| ); | |
| } catch (error) { | |
| console.error("Error handling offer", error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In frontend/src/Pages/ViewDebate.tsx around lines 183 to 224, the current
fallback assigns pc1 when debaterIndex is neither 0 nor 1 which can cause two
debaters sharing the same RTCPeerConnection and broken streams; change the logic
to reject or ignore offers from unexpected participants: validate debaterIndex
explicitly (allow only 0 or 1), if it is out of range log a warning and send a
WS message back indicating the offer is rejected (or simply return without
assigning a peer), and do not fall back to pc1; this prevents overwriting
existing connections and ensures only known participants are handled, while
still preserving the existing flow for valid indices and sending the answer when
accepted.
| } catch (error) { | ||
| console.error("Error handling offer", error); | ||
| } |
There was a problem hiding this comment.
Error handling for WebRTC operations could mask connection failures.
The error handling for setRemoteDescription/createAnswer (lines 221-223) and addIceCandidate (lines 241-243) only logs errors without any recovery or user feedback. Failed WebRTC operations will leave users staring at blank video placeholders without understanding why.
Consider adding user-facing error feedback or recovery logic:
} catch (error) {
console.error("Error handling offer", error);
+ // Consider showing a toast notification or setting an error state
+ // that displays to the user that video connection failed
}Similarly for ICE candidate errors:
} catch (error) {
console.error("Error adding ICE candidate", error);
+ // ICE candidate failures might self-recover, but repeated failures
+ // could indicate network issues worth surfacing to the user
}Also applies to: 241-243
2 participants
Bug Fixes
Error Boundary Improvements
handleAsyncErrorreturn value issue: The async error handler was not returning the result from wrapped functions, causing callers to receiveundefinedinstead of expected dataTailwind Configuration Fix
tailwindcss-animateplugin doesn't support ES module importsrequire()syntaxtailwind-scrollbar-hideplugin that caused ES module loading warningstailwindcss-animateplugin working correctlyTechnical Changes
Testing
Related Issues
Addresses code review feedback regarding:
Impact
Summary by CodeRabbit
Release Notes