Open
Conversation
📝 WalkthroughWalkthroughIntroduces a direct challenge system allowing teams to initiate matches against other teams and toggle challenge availability. Replaces random queue matchmaking with a score-based algorithm considering Elo similarity, recency penalties, and activity boosts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant Controller as Team Controller
participant TeamSvc as Team Service
participant MatchSvc as Match Service
participant DB as Database
Client->>Controller: POST /challenge/{targetTeamId}
Controller->>TeamSvc: challengeTeam(eventId, challengerTeamId, targetTeamId)
TeamSvc->>DB: Fetch target team
TeamSvc->>TeamSvc: Validate event containment
TeamSvc->>TeamSvc: Validate allowChallenges & self-challenge
TeamSvc->>MatchSvc: Create match (QUEUE phase)
MatchSvc->>DB: Insert match record
MatchSvc->>DB: Save match entity
TeamSvc->>MatchSvc: Start match
MatchSvc->>DB: Update match state
TeamSvc-->>Controller: Return created match
Controller-->>Client: 200 OK / Navigate to queue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/team/TeamInfoSection.tsx (1)
1-27:⚠️ Potential issue | 🔴 CriticalMissing
"use client"directive.This component uses React hooks (
useState,useParams) which require the"use client"directive at the top of the file in Next.js App Router. Without it, the component will fail at runtime.🐛 Proposed fix
+"use client"; + import type { Team, TeamMember } from "@/app/actions/team"; import { DialogTrigger } from "@radix-ui/react-dialog";Based on learnings: "Use App Router structure (app/) in Next.js and mark Client Components with 'use client' directive at the top"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/team/TeamInfoSection.tsx` around lines 1 - 27, This file is missing the "use client" directive required for client components using hooks (useState, useParams); add a "use client" directive as the very first line of TeamInfoSection.tsx so the component (imports/uses like useState, useParams and related UI components such as TeamInviteModal, DialogTrigger, Switch, etc.) runs as a client component and avoids runtime errors.
🧹 Nitpick comments (3)
frontend/components/team/TeamInfoSection.tsx (1)
56-63: Consider displaying error feedback to the user.The toggle handler correctly uses optimistic updates and reverts on error, but silently fails without informing the user. Consider showing a toast or error message.
♻️ Suggested improvement
const handleToggleAllowChallenges = async () => { const previous = allowChallenges; setAllowChallenges(!previous); const result = await toggleAllowChallenges(eventId); if (result && "error" in result) { setAllowChallenges(previous); + // Consider adding toast notification here + // toast.error("Failed to update challenge settings"); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/team/TeamInfoSection.tsx` around lines 56 - 63, The handler handleToggleAllowChallenges currently performs an optimistic update and reverts on error but gives no user feedback; update it to display an error notification when toggleAllowChallenges(eventId) returns an error (and optionally a success message on success). Locate handleToggleAllowChallenges and, after detecting "error" in result, call the project's UI notification utility (e.g., showToast, notify, enqueueSnackbar or similar) to show a concise error message and then revert with setAllowChallenges(previous); likewise consider showing a brief success toast when the call succeeds to confirm the change to the user.api/src/team/team.service.ts (1)
662-667: Consider using atomic update for toggle.The current read-then-write pattern is susceptible to race conditions if multiple requests toggle simultaneously. While the impact is low (worst case: toggle direction is inconsistent), an atomic approach is cleaner.
♻️ Atomic toggle alternative
async toggleAllowChallenges(teamId: string) { - const team = await this.getTeamById(teamId); - return this.teamRepository.update(teamId, { - allowChallenges: !team.allowChallenges, - }); + return this.teamRepository + .createQueryBuilder() + .update() + .set({ allowChallenges: () => "NOT allowChallenges" }) + .where("id = :teamId", { teamId }) + .execute(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/team/team.service.ts` around lines 662 - 667, toggleAllowChallenges currently does a read-then-write using getTeamById then teamRepository.update, which can race; change it to an atomic update that flips the DB value server-side (remove the initial getTeamById). Use the repository's atomic operator/SQL expression to invert the boolean (e.g., DB-level NOT/inversion) in the update call for teamRepository.update (or use an updateOne with a payload that toggles allowChallenges), and return the updated record; keep the method name toggleAllowChallenges and ensure no separate read is performed before the update.frontend/app/events/[id]/teams/[teamId]/ChallengeButton.tsx (1)
37-38: Narrow the caught value instead of usingany.Use
unknownhere and unwrapErrorexplicitly; the current annotation sidesteps the repo's strict typing rule.🔍 Proposed fix
- } catch (e: any) { - setError(e.message || "An error occurred"); + } catch (e: unknown) { + setError(e instanceof Error ? e.message : "An error occurred"); }As per coding guidelines, "Use strict TypeScript typing and avoid any type where possible, using interfaces/types for DTOs and props instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/teams/[teamId]/ChallengeButton.tsx around lines 37 - 38, Change the catch clause in ChallengeButton.tsx from catching `any` to `unknown` and unwrap the error safely before passing to `setError`: use `catch (e: unknown)` and call `setError(e instanceof Error ? e.message : String(e))` (optionally add `console.error(e)` for debugging). Locate the catch block that currently reads `catch (e: any) { setError(e.message || "An error occurred"); }` and replace with the safe `unknown` check using `instanceof Error`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/match/match.service.ts`:
- Around line 90-108: recentMatches.forEach loop assumes each match has two
teams; add a defensive check at the start of the iteration to skip or handle
matches where m.teams is missing, not an array, or has fewer than 2 entries so
we don't access m.teams[0] / m.teams[1] and corrupt lastPlayedWith; update the
loop in match.service.ts (the recentMatches.forEach block that writes to
lastPlayedWith using t1, t2 and m.createdAt) to validate m.teams length and
return/continue early for invalid entries, optionally logging or counting
skipped matches.
In `@api/src/team/team.service.ts`:
- Around line 669-698: challengeTeam currently skips eligibility checks: fetch
the challenger team via this.getTeamById(challengerTeamId, { event: true }) and
verify challengerTeam.repo is not null; then ensure neither team is already in
an active match by using the matchService to look up any match involving
challengerTeamId or targetTeamId with phase === MatchPhase.IN_PROGRESS (or an
equivalent "active" lookup) and throw BadRequestException if found; only after
these checks call this.matchService.createMatch(...) and
this.matchService.startMatch(match.id).
In `@frontend/app/actions/team.ts`:
- Line 84: The mapping that assigns allowChallenges currently takes
team.allowChallenges directly and can produce undefined; update each occurrence
(the mapping lines setting allowChallenges in frontend/app/actions/team.ts at
the three spots) to use a safe fallback (e.g., team.allowChallenges ?? false) so
the resulting value always matches the non-optional Team interface; apply this
change to all three mappings that set allowChallenges.
In `@frontend/app/events/`[id]/teams/[teamId]/ChallengeButton.tsx:
- Line 55: The error message rendered in ChallengeButton.tsx using the plain
span for {error} is not reliably announced by screen readers; change the error
node to be a live region (e.g., use a container with aria-live="assertive" or
"polite" and/or role="alert") so failed challenges are announced. Locate the JSX
that renders {error} in the ChallengeButton component and replace the plain
<span> with an accessible live region element (keeping the same styling classes
like "text-xs text-destructive") using aria-live="assertive" or role="alert" to
ensure screen readers announce the error.
- Around line 31-40: The current finally block clears isPending which allows the
button to be re-clicked before navigation completes; update the flow so
setIsPending(false) is only called on failure paths: when isActionError(result)
is true (inside that branch where you call setError(result.error)) and in the
catch block (where you call setError(e.message || "...")), but do NOT call
setIsPending(false) on the success branch that calls
router.push(`/events/${eventId}/queue`) — leave isPending true during navigation
to prevent duplicate challengeTeam() calls. Ensure this change touches the async
handler that calls challengeTeam, isActionError, setError, setIsPending and
router.push in ChallengeButton.tsx.
In `@frontend/app/events/`[id]/teams/[teamId]/page.tsx:
- Around line 52-53: The canChallenge calculation should ensure the user
actually has a team before allowing the challenge button; update the logic
around myTeam/isMyTeam/canChallenge so you first derive a hasTeam boolean (e.g.,
hasTeam = Boolean(myTeam)), compute isMyTeam using hasTeam (isMyTeam = hasTeam
&& myTeam.id === teamId), and then set canChallenge = hasTeam && !isMyTeam &&
teamInfo.allowChallenges so the button is hidden for users without a team.
---
Outside diff comments:
In `@frontend/components/team/TeamInfoSection.tsx`:
- Around line 1-27: This file is missing the "use client" directive required for
client components using hooks (useState, useParams); add a "use client"
directive as the very first line of TeamInfoSection.tsx so the component
(imports/uses like useState, useParams and related UI components such as
TeamInviteModal, DialogTrigger, Switch, etc.) runs as a client component and
avoids runtime errors.
---
Nitpick comments:
In `@api/src/team/team.service.ts`:
- Around line 662-667: toggleAllowChallenges currently does a read-then-write
using getTeamById then teamRepository.update, which can race; change it to an
atomic update that flips the DB value server-side (remove the initial
getTeamById). Use the repository's atomic operator/SQL expression to invert the
boolean (e.g., DB-level NOT/inversion) in the update call for
teamRepository.update (or use an updateOne with a payload that toggles
allowChallenges), and return the updated record; keep the method name
toggleAllowChallenges and ensure no separate read is performed before the
update.
In `@frontend/app/events/`[id]/teams/[teamId]/ChallengeButton.tsx:
- Around line 37-38: Change the catch clause in ChallengeButton.tsx from
catching `any` to `unknown` and unwrap the error safely before passing to
`setError`: use `catch (e: unknown)` and call `setError(e instanceof Error ?
e.message : String(e))` (optionally add `console.error(e)` for debugging).
Locate the catch block that currently reads `catch (e: any) { setError(e.message
|| "An error occurred"); }` and replace with the safe `unknown` check using
`instanceof Error`.
In `@frontend/components/team/TeamInfoSection.tsx`:
- Around line 56-63: The handler handleToggleAllowChallenges currently performs
an optimistic update and reverts on error but gives no user feedback; update it
to display an error notification when toggleAllowChallenges(eventId) returns an
error (and optionally a success message on success). Locate
handleToggleAllowChallenges and, after detecting "error" in result, call the
project's UI notification utility (e.g., showToast, notify, enqueueSnackbar or
similar) to show a concise error message and then revert with
setAllowChallenges(previous); likewise consider showing a brief success toast
when the call succeeds to confirm the change to the user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce4a02bb-e169-4795-a101-637b571c4317
📒 Files selected for processing (9)
api/db/migrations/1773501626133-direct_matches.tsapi/src/match/match.service.tsapi/src/team/entities/team.entity.tsapi/src/team/team.controller.tsapi/src/team/team.service.tsfrontend/app/actions/team.tsfrontend/app/events/[id]/teams/[teamId]/ChallengeButton.tsxfrontend/app/events/[id]/teams/[teamId]/page.tsxfrontend/components/team/TeamInfoSection.tsx
Comment on lines
+31
to
+40
| const result = await challengeTeam(eventId, targetTeamId); | ||
| if (isActionError(result)) { | ||
| setError(result.error); | ||
| } else { | ||
| router.push(`/events/${eventId}/queue`); | ||
| } | ||
| } catch (e: any) { | ||
| setError(e.message || "An error occurred"); | ||
| } finally { | ||
| setIsPending(false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "frontend/app/events/[id]/teams/[teamId]/ChallengeButton.tsx"Repository: 42core-team/website
Length of output: 2045
🏁 Script executed:
# Check for any other similar patterns in the codebase that might inform the correctness of the suggestion
find frontend -name "*.tsx" -type f | head -10 | xargs grep -l "finally.*setIsPending" 2>/dev/null | head -5Repository: 42core-team/website
Length of output: 45
🏁 Script executed:
# Check what challengeTeam action does and whether it's a server action
fd -t f "challengeTeam" frontend --exec grep -l "challengeTeam" {} \; 2>/dev/null | head -5Repository: 42core-team/website
Length of output: 45
🏁 Script executed:
# Find the challengeTeam action to understand its behavior
find frontend -name "*.ts" -o -name "*.tsx" | xargs grep -l "export.*challengeTeam" 2>/dev/nullRepository: 42core-team/website
Length of output: 91
🏁 Script executed:
# Check the app/actions directory structure
ls -la "frontend/app/actions/" 2>/dev/null || echo "actions dir check"Repository: 42core-team/website
Length of output: 861
🏁 Script executed:
# Search for the actual challengeTeam implementation
rg "export.*challengeTeam|function challengeTeam" frontend/app/actions/ -A 5 2>/dev/nullRepository: 42core-team/website
Length of output: 431
Don't clear isPending on the success path.
finally makes the button clickable again before the route transition finishes, so a slow navigation can send challengeTeam() twice. For this write flow, only clear the pending state when the challenge fails.
💡 Proposed fix
try {
const result = await challengeTeam(eventId, targetTeamId);
if (isActionError(result)) {
setError(result.error);
+ setIsPending(false);
} else {
router.push(`/events/${eventId}/queue`);
}
} catch (e: any) {
setError(e.message || "An error occurred");
- } finally {
setIsPending(false);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/events/`[id]/teams/[teamId]/ChallengeButton.tsx around lines 31
- 40, The current finally block clears isPending which allows the button to be
re-clicked before navigation completes; update the flow so setIsPending(false)
is only called on failure paths: when isActionError(result) is true (inside that
branch where you call setError(result.error)) and in the catch block (where you
call setError(e.message || "...")), but do NOT call setIsPending(false) on the
success branch that calls router.push(`/events/${eventId}/queue`) — leave
isPending true during navigation to prevent duplicate challengeTeam() calls.
Ensure this change touches the async handler that calls challengeTeam,
isActionError, setError, setIsPending and router.push in ChallengeButton.tsx.
| <Swords className="size-4" /> | ||
| {isPending ? "Challenging..." : `Challenge ${targetTeamName}`} | ||
| </Button> | ||
| {error && <span className="text-xs text-destructive">{error}</span>} |
There was a problem hiding this comment.
Make challenge failures announce themselves.
This error node is inserted after the click, but a plain span is not reliably announced by screen readers. Add a live region so failed challenges are not silent.
♿ Proposed fix
- {error && <span className="text-xs text-destructive">{error}</span>}
+ {error && (
+ <span role="alert" className="text-xs text-destructive">
+ {error}
+ </span>
+ )}📝 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.
Suggested change
| {error && <span className="text-xs text-destructive">{error}</span>} | |
| {error && ( | |
| <span role="alert" className="text-xs text-destructive"> | |
| {error} | |
| </span> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/events/`[id]/teams/[teamId]/ChallengeButton.tsx at line 55, The
error message rendered in ChallengeButton.tsx using the plain span for {error}
is not reliably announced by screen readers; change the error node to be a live
region (e.g., use a container with aria-live="assertive" or "polite" and/or
role="alert") so failed challenges are announced. Locate the JSX that renders
{error} in the ChallengeButton component and replace the plain <span> with an
accessible live region element (keeping the same styling classes like "text-xs
text-destructive") using aria-live="assertive" or role="alert" to ensure screen
readers announce the error.
Comment on lines
+52
to
+53
| const isMyTeam = myTeam?.id === teamId; | ||
| const canChallenge = !isMyTeam && teamInfo.allowChallenges; |
There was a problem hiding this comment.
canChallenge should also verify the user has a team.
If myTeam is null (user not in any team for this event), isMyTeam will be false, potentially making canChallenge true. The button would render but the backend MyTeamGuards would reject the action. Better UX would be to hide the button entirely.
🛠️ Proposed fix
const isMyTeam = myTeam?.id === teamId;
-const canChallenge = !isMyTeam && teamInfo.allowChallenges;
+const canChallenge = myTeam && !isMyTeam && teamInfo.allowChallenges;📝 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.
Suggested change
| const isMyTeam = myTeam?.id === teamId; | |
| const canChallenge = !isMyTeam && teamInfo.allowChallenges; | |
| const isMyTeam = myTeam?.id === teamId; | |
| const canChallenge = myTeam && !isMyTeam && teamInfo.allowChallenges; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/events/`[id]/teams/[teamId]/page.tsx around lines 52 - 53, The
canChallenge calculation should ensure the user actually has a team before
allowing the challenge button; update the logic around
myTeam/isMyTeam/canChallenge so you first derive a hasTeam boolean (e.g.,
hasTeam = Boolean(myTeam)), compute isMyTeam using hasTeam (isMyTeam = hasTeam
&& myTeam.id === teamId), and then set canChallenge = hasTeam && !isMyTeam &&
teamInfo.allowChallenges so the button is hidden for users without a team.
PaulicStudios
requested changes
Mar 14, 2026
Comment on lines
+141
to
+153
| // 3. Activity Boost (Prioritize active players) | ||
| // Use updatedAt as a proxy for recent queue activity/challenges toggle | ||
| const now = Date.now(); | ||
| const t1Activity = | ||
| (now - t1.updatedAt.getTime()) / (1000 * 60); // minutes | ||
| const t2Activity = | ||
| (now - t2.updatedAt.getTime()) / (1000 * 60); // minutes | ||
|
|
||
| // Boost if they were active in the last 10 minutes | ||
| if (t1Activity < 10) score += 50; | ||
| if (t2Activity < 10) score += 50; | ||
|
|
||
| pairings.push({ t1, t2, score }); |
Member
There was a problem hiding this comment.
it does not make any sense to use if the teams information was updated in the last 10 minutes that they should be chosen more over others. It should be if the team themselfs challenged someone else or if they went in the queue. And the scoring should dynamic not fixed based on 10 minutes
Comment on lines
+23
to
+43
| const [isPending, setIsPending] = useState(false); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const router = useRouter(); | ||
|
|
||
| const handleChallenge = async () => { | ||
| setIsPending(true); | ||
| setError(null); | ||
| try { | ||
| const result = await challengeTeam(eventId, targetTeamId); | ||
| if (isActionError(result)) { | ||
| setError(result.error); | ||
| } else { | ||
| router.push(`/events/${eventId}/queue`); | ||
| } | ||
| } catch (e: any) { | ||
| setError(e.message || "An error occurred"); | ||
| } finally { | ||
| setIsPending(false); | ||
| } | ||
| }; | ||
|
|
Member
There was a problem hiding this comment.
use TanStack Query here 😛
| const [isOpen, setIsOpen] = useState(false); | ||
| const [leaveError, setLeaveError] = useState<string | null>(null); | ||
| const [isLeaveDialogOpen, setIsLeaveDialogOpen] = useState(false); | ||
| const [allowChallenges, setAllowChallenges] = useState(myTeam.allowChallenges); |
Member
|
@Peu77 also run pnpm lint so that the lint pipeline does not fail |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores