tanstack query migration #457
Conversation
…gies to auth module
… dependencies for JWT authentication
…n in request headers
…ired profilePicture
… details and update SSO page for seamless sign-in experience
…tion-to-the-backend # Conflicts: # frontend/app/events/page.tsx
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds cookie-based JWT auth, GitHub and 42 OAuth backend flows; replaces header-based UserGuard with JwtAuthGuard; migrates domains from .de to .sh; introduces React Query hydration/mutations frontend-side; adds repo auto-creation with advisory locks and related DB migrations; removes client OAuth secrets/docs and a local pre-commit hook. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client Browser
participant Frontend as Frontend App
participant Backend as Backend API
participant OAuth as OAuth Provider
rect rgba(100,150,200,0.5)
note over Browser,OAuth: GitHub OAuth (backend-mediated)
Browser->>Frontend: Click "Sign in with GitHub"
Frontend->>Backend: Navigate /auth/github (redirect)
Backend->>OAuth: Redirect user to OAuth authorize endpoint
OAuth-->>Backend: Return code to callback
Backend->>Backend: Exchange code, create/update user, sign JWT
Backend-->>Browser: Set httpOnly cookie and redirect to app
Browser->>Frontend: Subsequent requests include cookie JWT
end
sequenceDiagram
participant Browser as Client Browser
participant Frontend as Frontend App
participant Backend as Backend API
participant OAuth42 as Forty-Two Provider
rect rgba(150,200,100,0.5)
note over Browser,OAuth42: 42 School linking flow
Browser->>Frontend: Request 42 auth URL
Frontend->>Backend: GET /auth/42/getUrl
Backend->>Backend: Encrypt user id, return auth URL
Backend-->>Browser: OAuth authorization URL
Browser->>OAuth42: Redirect to 42 authorization
OAuth42-->>Browser: Redirect to /auth/42/callback (with code + state)
Browser->>Backend: GET /auth/42/callback
Backend->>Backend: Decrypt state, upsert social account
Backend-->>Browser: Redirect to success URL
end
sequenceDiagram
participant Browser as Client Browser
participant Frontend as Frontend App (React Query)
participant Backend as Backend API
rect rgba(200,150,100,0.5)
note over Browser,Backend: Protected API request via cookie JWT
Browser->>Frontend: Render component needing team data
Frontend->>Frontend: Check React Query cache
Frontend->>Backend: GET /team/event/{eventId}/my (cookies sent)
Backend->>Backend: Validate JWT from cookie
Backend-->>Frontend: Return team data
Frontend->>Frontend: Update cache and render UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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
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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
frontend/app/events/[id]/dashboard/dashboard.tsx (4)
246-246: Remove empty.finally()block.This empty callback serves no purpose and should be removed.
Proposed fix
setStartingGroupPhase(false); }) - .finally(() => {}); + ;
270-270: Remove empty.finally()block.Same issue as above—dead code that should be removed.
Proposed fix
setStartingTournament(false); }) - .finally(() => {}); + ;
289-300: Add error handling for consistency.The
setEventTeamsLockDatepromise lacks a.catch()handler, unlike other action handlers in this file. If the request fails, the user won't receive any feedback.Proposed fix
onClick={() => setEventTeamsLockDate( eventId, new Date(teamAutoLockTime).getTime(), ).then(() => { // eslint-disable-next-line no-alert alert("set team auto lock date"); - })} + }).catch(() => { + // eslint-disable-next-line no-alert + alert("error occurred"); + })}
301-312: Add error handling for the Reset action.Same concern—missing
.catch()handler leaves users uninformed on failure.Proposed fix
onClick={() => { setEventTeamsLockDate(eventId, null).then(() => { // eslint-disable-next-line no-alert alert("reset team auto lock date"); setTeamAutoLockTime(""); - }); + }).catch(() => { + // eslint-disable-next-line no-alert + alert("error occurred"); + }); }}frontend/components/team/TeamInviteModal.tsx (2)
32-36: UnusedteamIdprop.The
teamIdprop is defined inTeamInviteModalProps(line 28) but is not destructured or used anywhere in the component. Either remove it from the interface or use it insendTeamInviteif the API requires it.Option 1: Remove unused prop from interface
interface TeamInviteModalProps { isOpen: boolean; onClose: () => void; - teamId: string; eventId: string; }
105-111: NestedDialogContentcomponents.There's a
DialogContentat line 105 and another nested inside at line 111. Radix/shadcn Dialog components don't support nestedDialogContent. The inner one should likely be a<div>or removed entirely.Suggested fix
<DialogContent> <DialogHeader> <DialogTitle className="text-xl font-semibold"> Invite Team Members </DialogTitle> </DialogHeader> - <DialogContent> + <div className="py-4"> <div className="grid w-full max-w-sm items-center gap-3"> ... </div> <div className="max-h-[300px] overflow-y-auto"> ... </div> - </DialogContent> + </div> <DialogFooter>frontend/components/team/TeamInfoSection.tsx (1)
64-75: Surface leave failures while the dialog stays open.
On failure you return early, but the error message is rendered outside the modal, so it’s hidden while the dialog is open. Consider closing the dialog on failure or rendering the error inside the dialog.✅ Minimal fix (close dialog on failure)
const success = await onLeaveTeam(); if (!success) { setLeaveError( "Failed to leave team. Try refreshing the page or trying again later.", ); + setIsLeaveDialogOpen(false); return; }frontend/helm/README.md (1)
87-92: Update TLS secret name to match .sh domain (doc mismatch).Line 92 still references
www-coregame-de-tls, which no longer matches the .sh domain shown in Lines 87-88. This can mislead operators copying the example.📝 Suggested doc fix
- secretName: www-coregame-de-tls + secretName: www-coregame-sh-tlsfrontend/.env.example (1)
20-35: Fix dotenv‑linter warnings (quote + key order).Line 20 has quotes and Line 35 wants ordering; minor but will keep lint clean.
✅ Suggested adjustment
-NEXT_PUBLIC_VISUALIZER_URL="https://dev.visualizer.coregame.sh" +NEXT_PUBLIC_VISUALIZER_URL=https://dev.visualizer.coregame.sh @@ -PLAUSIBLE_DOMAIN=local.coregame.sh -PLAUSIBLE_CUSTOM_DOMAIN=https://plausible.coregame.sh +PLAUSIBLE_CUSTOM_DOMAIN=https://plausible.coregame.sh +PLAUSIBLE_DOMAIN=local.coregame.shfrontend/layouts/basic-navbar.tsx (1)
191-197: Add a fallback forprofilePictureto preventnext/imageerror.If the session API call fails (line 71 in authOptions.ts),
profilePicturemay be undefined whilesession?.user?.idis still set, causing the Image component to throw an error. Either wrap the Image in a conditional render or provide a fallback URL:Suggested fix (requires creating `/public/placeholder-avatar.png` or use existing fallback)
- src={session?.user?.profilePicture} + src={session?.user?.profilePicture || "/placeholder-avatar.png"}Alternatively, conditionally render the Image only when
profilePictureexists.
🤖 Fix all issues with AI agents
In `@api/.env.example`:
- Line 18: Rename the invalid environment variable that starts with a digit:
change 42_OAUTH_SUCCESS_REDIRECT_URL to FORTYTWO_OAUTH_SUCCESS_REDIRECT_URL in
the .env.example and update the reference inside the Auth controller where
configService.getOrThrow(...) is called (search for the string
"42_OAUTH_SUCCESS_REDIRECT_URL" in auth.controller.ts) so the same key is used;
also update any other occurrences (tests, docs, or other config lookups) to the
new FORTYTWO_OAUTH_SUCCESS_REDIRECT_URL name to keep usage consistent.
In `@api/helm/values-dev.yaml`:
- Around line 43-46: The TLS secretName currently references the old domain
(secretName: dev-api-coregame-de-tls) while hosts lists dev.api.coregame.sh;
update the secretName to match the new domain (e.g., change secretName to a name
reflecting .sh) and ensure cert-manager is instructed to issue a certificate for
dev.api.coregame.sh, or alternatively confirm the existing secret contains a
certificate valid for dev.api.coregame.sh before leaving secretName unchanged;
adjust the tls block's secretName and/or recreate the TLS secret so the
certificate SANs match the hosts entry.
In `@api/src/user/social-account.service.ts`:
- Around line 52-75: The upsertSocialAccountForUser implementation is
race-prone; replace the findOne-then-save flow with an atomic upsert using
this.socialAccountRepository.upsert(...) keyed on userId and platform to avoid
unique constraint errors; after calling socialAccountRepository.upsert({ userId,
platform, username, platformUserId }, { conflictPaths: ['userId','platform'] })
fetch and return the persisted entity via this.socialAccountRepository.findOne({
where: { userId: params.userId, platform: params.platform } }) so the method
still returns a SocialAccountEntity.
In `@frontend/app/auth/sso/page.tsx`:
- Around line 9-23: The async sign-in flow currently ignores the response from
signIn and treats failures the same as successes; update the useEffect handler
(the async IIFE that calls signIn) to await and inspect the returned response
object from signIn("backend", { redirect: false }) instead of assuming it
throws: if res.error or !res.ok then, when not cancelled, call
router.replace("/auth/error"); else if res.url is present then navigate to the
provider redirect (e.g., set window.location.href = res.url) instead of
router.replace("/"), and only call router.replace("/") and router.refresh() when
res.ok is true and no external res.url exists; keep the try/catch to log real
exceptions.
In `@frontend/app/events/`[id]/my-team/components/TeamInfoDisplay.tsx:
- Around line 39-49: The query key in TeamInfoDisplay.tsx is inconsistent with
SSR prefetching; replace the inline array key ["team", team?.id, "members"] used
in the useQuery call with the shared helper teamMembersQueryKey(team?.id) from
the queries module so the client query matches server-side keys (keep other
options like queryFn, enabled, and initialData unchanged).
- Around line 27-37: Add an onSuccess handler to the leaveTeamMutation so that
after a successful API call you invalidate the cached queries for the current
event's team and pending invites (e.g., invalidateQueries(["event", eventId,
"my-team"]) and invalidateQueries(["event", eventId, "pending-invites"]) via the
react-query QueryClient) to ensure the UI updates immediately; also remove the
unused isRepoPending state and stop passing it into TeamInfoSection (delete its
declaration and the prop usage). Use the existing eventId and leaveTeamMutation
identifiers to locate where to attach the onSuccess and call
queryClient.invalidateQueries.
In `@frontend/app/events/`[id]/my-team/queries.ts:
- Around line 21-26: The teamMembersQueryFn currently accepts teamId?: string
and will call axiosInstance.get(`/team/${teamId}/members`) even when teamId is
undefined; update teamMembersQueryFn to guard against undefined teamId by either
(A) changing the parameter type to string and relying on the caller to only call
it when teamId exists (and use TanStack Query's enabled option in the consumer),
or (B) keeping the signature but early-return or throw a clear error when teamId
is undefined before calling axiosInstance.get; reference teamMembersQueryFn and
the axiosInstance.get call when making the change so the request to
`/team/undefined/members` is prevented.
In `@frontend/app/events/`[id]/queue/queueState.tsx:
- Around line 33-61: The refetchInterval on the queueState useQuery is set to
600 (milliseconds) which is too aggressive; change the refetchInterval passed to
the useQuery for queueState (the call using queueStateQueryKey/eventId and
queryFn queueStateQueryFn) to a larger value (e.g. 3000 ms) or replace the
literal with a named constant (e.g. QUEUE_POLL_INTERVAL) and document it so the
intent is clear and easily configurable; update any related comments or
constants accordingly.
- Around line 63-79: The redirect compares the live queueState to the static
props.queueState, so it never detects client-side transitions; replace the
props-based previous state with a ref: create a prevMatchStateRef via useRef,
initialize it to props.queueState.match?.state, then in the useEffect that
watches queueState (and router/eventId) compare prevMatchStateRef.current to
queueState.match?.state (checking for transition from MatchState.IN_PROGRESS to
a new match), call router.push when appropriate, and finally update
prevMatchStateRef.current = queueState.match?.state; ensure you remove
props.queueState.match?.state from the effect deps and reference the symbols
queueState, props.queueState, prevMatchStateRef, useEffect,
MatchState.IN_PROGRESS, and router.push to locate the code.
In `@frontend/app/events/page.tsx`:
- Around line 52-54: The Link component usage in events/page.tsx incorrectly
passes a non-existent color prop to next/link; remove the color="primary" prop
from the <Link> element and instead add a Tailwind class via className (for
example className="text-primary" or whatever design token you use) to style the
link; update the <Link> with className and ensure any nested children receive
the styling as needed (reference the Link element in this file).
In `@frontend/app/providers.tsx`:
- Around line 6-14: The module-level QueryClient instance (const queryClient) in
Providers leads to cross-request cache leakage; remove that top-level
instantiation and implement a getQueryClient() helper that returns a new
QueryClient when isServer() is true (per-request SSR) and a stable singleton in
the browser, then update the Providers component to call getQueryClient() and
pass its result to <QueryClientProvider client={...}> while preserving
ReactQueryDevtools and children; ensure the unique symbols referenced are
QueryClient, Providers, QueryClientProvider, getQueryClient, and isServer.
In `@frontend/app/utils/authOptions.ts`:
- Around line 58-73: On failure of the axiosInstance.get('/auth/me') call, clear
all session user fields instead of only session.user.id to avoid stale
JWT-derived data; in the catch block set session.user.id, session.user.name,
session.user.email, and session.user.profilePicture to empty values (or add/set
an explicit session.user.isAuthenticated = false and handle that at the call
site) so useSession() no longer reports an authenticated user with outdated
information.
In `@frontend/components/github.tsx`:
- Around line 7-10: The redirect builds a broken URL when
process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL is missing; update the logic around
the code that reads NEXT_PUBLIC_BACKEND_PUBLIC_URL and sets window.location.href
(the block using base = process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL and then
window.location.href = `${base?.replace(/\/$/, "")}/auth/github/callback`) to
validate base first, and if it's falsy, log a clear error (or surface a UI
error) and do not perform the redirect; only call replace(...) and assign
window.location.href when base is a non-empty string.
🟡 Minor comments (8)
frontend/components/team/TeamInfoSection.tsx-219-226 (1)
219-226: Keep a readable label when showing the spinner.
When loading, the button text disappears, which reduces clarity and screen-reader context.♿ Accessible loading label
- {isLeaving ? <Spinner /> : "Leave Team"} + {isLeaving ? ( + <> + <Spinner /> + <span className="sr-only">Leaving team</span> + </> + ) : ( + "Leave Team" + )}frontend/app/events/[id]/my-team/components/TeamCreationForm.tsx-39-43 (1)
39-43: Analytics event fires before mutation succeeds.The
plausible("create_team")call inonMutatetriggers even when the mutation subsequently fails. For accurate analytics, track successful team creations inonSuccessinstead.Proposed fix
onMutate: () => { - plausible("create_team"); setErrorMessage(null); setValidationError(null); }, onSuccess: async () => { + plausible("create_team"); await Promise.all([api/package.json-50-51 (1)
50-51: Add@types/passport-jwtto devDependencies.The codebase uses
passport-jwt(imported inapi/src/auth/jwt.strategy.ts), but@types/passport-jwtis missing from devDependencies. While@types/passport-localis already included, the corresponding type definitions for the JWT strategy are absent. Sincepassport-jwtdoes not ship with built-in TypeScript types, the types package should be added for proper TypeScript support.api/README.md-109-126 (1)
109-126: Duplicate section heading creates documentation confusion.There is already an "## Environment Variables" section at line 64. Having two sections with the same heading makes navigation difficult and breaks document structure.
Proposed fix
Merge this OAuth setup content into the existing "Environment Variables" section (around lines 64-85), perhaps as new subsections:
-## Environment Variables - -### GitHub OAuth (Required) +### GitHub OAuth (Required for Authentication)Move lines 112-126 to follow line 85, removing the duplicate
## Environment Variablesheading.api/helm/values-prod.yaml-48-51 (1)
48-51: TLS secret name still references old domain.The TLS hosts have been updated to
api.coregame.sh, butsecretNamestill referencesapi-coregame-de-tls. This inconsistency should be corrected:
- The secret name should reflect the domain it serves for clarity and consistency
- During a domain migration, this mismatch may cause confusion in operations and troubleshooting
- cert-manager will manage the secret regardless, but best practice is to keep naming conventions aligned
🔧 Proposed fix
tls: - hosts: - api.coregame.sh - secretName: api-coregame-de-tls + secretName: api-coregame-sh-tlsfrontend/app/events/[id]/my-team/components/TeamInvitesDisplay.tsx-108-124 (1)
108-124: Button disabled states don't prevent double-clicks during own action.The Accept button is disabled only when
isDeclining(line 111), not whenisAccepting. Similarly, the Decline button is disabled only whenisAccepting(line 120), not whenisDeclining. This allows users to click the same button multiple times during the action.🐛 Proposed fix
<Button size="sm" // TODO: isLoading={actionStates[invite.id]?.isAccepting} - disabled={actionStates[invite.id]?.isDeclining} + disabled={actionStates[invite.id]?.isAccepting || actionStates[invite.id]?.isDeclining} onClick={() => handleAcceptInvite(invite.id)} > Accept </Button> <Button size="sm" variant="secondary" // TODO: isLoading={actionStates[invite.id]?.isDeclining} - disabled={actionStates[invite.id]?.isAccepting} + disabled={actionStates[invite.id]?.isAccepting || actionStates[invite.id]?.isDeclining} onClick={() => handleDeclineInvite(invite.id)} > Decline </Button>frontend/app/events/[id]/queue/queries.ts-20-30 (1)
20-30: Inconsistent endpoint path format within the same file.The
/match/queue/${eventId}/endpoint uses a leading slash while the other queue endpoints in this file (team/event/${eventId}/queue/stateandteam/event/${eventId}/queue/join) use relative paths. Standardize on relative paths for consistency across the queue operations:🔧 Proposed fix
- const response = await axiosInstance.get<Match[]>( - `/match/queue/${eventId}/`, - ); + const response = await axiosInstance.get<Match[]>( + `match/queue/${eventId}/`, + );Note: This same inconsistency exists in
frontend/app/actions/team.ts(lines 42 & 46) where/match/queue/is used with a leading slash alongside relativeteam/event/paths in the same file.frontend/app/events/[id]/queue/page.tsx-51-66 (1)
51-66: Avoid mutating cached array when sorting.
match.teams.sort(...)mutates the cached array in-place. While this works in this SSR context, it violates React Query's immutability principles and can cause subtle bugs if the code is refactored. Clone before sorting to keep cached data immutable.🔧 Proposed fix
- const sortedQueueMatches = queueMatches.map(match => ({ - ...match, - teams: match.teams.sort((a, _b) => (a.id === myTeam.id ? -1 : 1)), - })); + const sortedQueueMatches = queueMatches.map(match => ({ + ...match, + teams: [...match.teams].sort((a, _b) => (a.id === myTeam.id ? -1 : 1)), + }));
🧹 Nitpick comments (29)
frontend/app/error.tsx (1)
64-68: Minor indentation inconsistency in string concatenation.The
+operators on lines 64 and 68 have extra indentation compared to the preceding lines (54-63), which slightly breaks the visual alignment of the multiline string.✨ Suggested fix for consistent indentation
+ `- OS: \n` + `- Device: \n` - + `- Time (UTC): ${new Date() - .toISOString() - .replace("T", " ") - .replace("Z", " UTC")}\n\n` - + `Additional context:\n`, + + `- Time (UTC): ${new Date() + .toISOString() + .replace("T", " ") + .replace("Z", " UTC")}\n\n` + + `Additional context:\n`,frontend/app/sitemap.ts (1)
4-4: Consider stripping trailing slashes from the base URL.The domain migration looks correct. However, if
NEXTAUTH_URLis configured with a trailing slash, the concatenated URLs would produce malformed paths likehttps://coregame.sh//wiki.Suggested defensive fix
- const baseUrl = process.env.NEXTAUTH_URL || "https://coregame.sh"; + const baseUrl = (process.env.NEXTAUTH_URL || "https://coregame.sh").replace(/\/+$/, "");frontend/app/events/loading.tsx (1)
1-6: Optional: extract a shared centered loading spinner component.These loading files duplicate the same Spinner markup. A tiny shared component would reduce repetition and keep styling consistent.
♻️ Proposed change (this file)
-import { Spinner } from "@/components/ui/spinner"; +import { CenteredSpinner } from "@/components/ui/centered-spinner"; export default function Loading() { return ( - <Spinner className="absolute top-1/2 left-1/2 -translate-x-1/2 -translate-y-1/2 size-24" /> + <CenteredSpinner /> ); }➕ Suggested new component (new file)
// frontend/components/ui/centered-spinner.tsx import { Spinner } from "@/components/ui/spinner"; export function CenteredSpinner() { return ( <Spinner className="absolute top-1/2 left-1/2 -translate-x-1/2 -translate-y-1/2 size-24" /> ); }frontend/components/team/TeamCreationSection.tsx (1)
36-41: LGTM — Loading state implementation is correct.The button correctly disables during loading and provides visual feedback with the Spinner. This properly prevents double-submission.
Optional accessibility improvement: When the button shows only the Spinner, screen readers lose context about the button's purpose. Consider adding visually hidden text:
{isLoading ? ( <> <Spinner /> <span className="sr-only">Creating team...</span> </> ) : ( "Create Team" )}frontend/app/events/[id]/dashboard/dashboard.tsx (1)
35-80: Consider migrating to TanStack Query.Given the PR objective is TanStack Query migration, this component still uses
useState+useEffectfor data fetching. Consider refactoring to useuseQueryforgetEventById,getTeamsCountForEvent,getParticipantsCountForEvent, andisEventAdmin, which would provide automatic caching, refetching, and loading/error states.frontend/components/team/TeamInviteModal.tsx (2)
171-178: Address the TODO: Button loading state.The
isInviting[user.id]state is already tracked but not used to show loading feedback on the button. Consider adding a loading spinner or disabling the button during invite.Suggested implementation
<Button size="sm" - disabled={user.isInvited} - // TODO: isLoading={isInviting[user.id]} + disabled={user.isInvited || isInviting[user.id]} onClick={() => handleInviteUser(user.id)} > - {user.isInvited ? "Invited" : "Invite"} + {isInviting[user.id] + ? "Inviting..." + : user.isInvited + ? "Invited" + : "Invite"} </Button>Do you want me to open a new issue to track this task?
46-57: Consider migrating to TanStack Query.Given the PR's TanStack Query migration scope, this search logic would benefit from
useQuerywith the debounced query as a key. This would handle caching, deduplication, and stale data automatically, while eliminating potential race conditions from rapid typing.const { data: searchResults = [], isLoading: isSearching } = useQuery({ queryKey: ['userSearch', eventId, debouncedQuery], queryFn: () => searchUsersForInvite(eventId, debouncedQuery), enabled: debouncedQuery.length >= 2, });frontend/components/team/TeamInfoSection.tsx (1)
92-105: Consider an explicit empty state instead of a perpetual skeleton.
If a repo is genuinely unset, the skeleton looks like a never-ending load. A simple placeholder text is clearer, unless you still have a loading state to gate it.💡 Possible empty-state tweak
- : ( - <Skeleton className="h-5 w-75 rounded-md m-2" /> - )} + : ( + <span className="text-muted-foreground">No repository configured</span> + )}frontend/components/social-accounts-display.tsx (1)
72-76: Prefer a non-blocking dialog/toast instead of disabling lint forconfirm/alert.
Using native dialogs blocks the UI and forces lint suppression; consider a modal/toast-based confirmation + error feedback to avoidno-alertdisables.Also applies to: 87-91
frontend/app/events/[id]/my-team/components/TeamCreationForm.tsx (1)
66-68: Unhandled promise rejection when mutation fails.Using
mutateAsync()without a try/catch means errors thrown by the mutation will result in unhandled promise rejections. WhileonErrorhandles UI state, the thrown error propagates to the caller.Consider using
mutate()instead (which doesn't throw) or wrap in try/catch if you need the async behavior.Option 1: Use mutate() instead
- async function handleCreateTeam() { - await createTeamMutation.mutateAsync(); + function handleCreateTeam() { + createTeamMutation.mutate(); }Option 2: Keep mutateAsync with try/catch
async function handleCreateTeam() { - await createTeamMutation.mutateAsync(); + try { + await createTeamMutation.mutateAsync(); + } catch { + // Error already handled in onError callback + } }frontend/helm/values-prod.yaml (1)
49-52: TLS secret name still references old domain.The host is now
coregame.shbutsecretNameremainscoregame-de-tls. While this may still work if the existing certificate covers both domains, consider updating the secret name tocoregame-sh-tlsfor consistency and to avoid confusion.The same issue exists on line 64 with
www-coregame-de-tls.Proposed fix
tls: - hosts: - coregame.sh - secretName: coregame-de-tls + secretName: coregame-sh-tlstls: enabled: true - secretName: www-coregame-de-tls + secretName: www-coregame-sh-tlsVerify whether the existing TLS secrets contain certificates valid for the new
.shdomain, or if new certificates will be provisioned by cert-manager under these secret names.frontend/app/actions/axios.ts (2)
16-28: Avoid dropping non‑token cookies in server‑side requests.Line 18–22 overwrites the Cookie header with only
token=…, which discards other cookies (e.g., refresh, CSRF, feature flags). Consider forwarding all cookies fromnext/headersto keep future auth changes safe.♻️ Suggested adjustment
- const cookieData = await require("next/headers").cookies(); - const token = cookieData.get("token"); - if (token) - config.headers.Cookie = `token=${token.value}`; + const cookieData = await require("next/headers").cookies(); + const cookieHeader = cookieData + .getAll() + .map(({ name, value }) => `${name}=${value}`) + .join("; "); + if (cookieHeader) + config.headers.Cookie = cookieHeader;
36-46: Narrow error handling to AxiosError for safer messaging.
error: anycan mask non‑Axios errors and lose useful messages. Preferaxios.isAxiosErrorand fall back toError.messagewhen not Axios.♻️ Suggested adjustment
-import axios from "axios"; +import axios from "axios"; @@ - catch (error: any) { - return { - error: error.response?.data?.message || "An unexpected error occurred", - }; - } + catch (error: unknown) { + if (axios.isAxiosError(error)) { + return { + error: + error.response?.data?.message + || error.message + || "An unexpected error occurred", + }; + } + return { + error: + error instanceof Error + ? error.message + : "An unexpected error occurred", + }; + }frontend/hooks/use42Linking.ts (1)
42-69: Clear the pending timeout to avoid setState after unmount.Line 48 schedules async work via
setTimeout; if the component unmounts before it fires, state updates can trigger warnings. Track the timeout id and clear it on unmount (or before re‑initiating).♻️ Suggested adjustment
- const processedRef = useRef<string | null>(null); + const processedRef = useRef<string | null>(null); + const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); @@ - setTimeout(async () => { + if (timeoutRef.current) + clearTimeout(timeoutRef.current); + timeoutRef.current = setTimeout(async () => { @@ - }, OAUTH_CONFIG.LOADING_DELAY); + }, OAUTH_CONFIG.LOADING_DELAY); }, []); + + useEffect(() => { + return () => { + if (timeoutRef.current) + clearTimeout(timeoutRef.current); + }; + }, []);api/.env.example (1)
23-23: Add trailing newline at end of file.Per static analysis and POSIX convention, files should end with a newline character.
Proposed fix
FORTYTWO_CLIENT_SECRET= +api/src/user/social-account.controller.ts (1)
21-25: Remove unusedLinkSocialAccountDtoclass.This DTO class is no longer used since the
linkSocialAccountendpoint was removed as part of the authentication refactor. Dead code should be cleaned up to maintain code hygiene.🧹 Proposed fix to remove unused code
-class LinkSocialAccountDto { - platform: SocialPlatform; - username: string; - platformUserId: string; -} - `@UseGuards`(JwtAuthGuard)api/src/auth/fortytwo.strategy.ts (2)
43-46: Replaceconsole.errorwith NestJS Logger for consistency.Using
console.errordirectly bypasses NestJS's logging infrastructure. Other parts of the codebase (e.g.,MatchController) useLoggerfrom@nestjs/commonfor structured logging.♻️ Proposed fix using NestJS Logger
+import { Injectable, Logger } from "@nestjs/common"; -import { Injectable } from "@nestjs/common"; ... `@Injectable`() export class FortyTwoOAuthStrategy extends PassportStrategy(Strategy, "42") { + private readonly logger = new Logger(FortyTwoOAuthStrategy.name); + constructor( ... if (!res.ok) { - console.error("42 API error:", res.status, await res.text()); + this.logger.error(`42 API error: ${res.status} - ${await res.text()}`); return done(new Error(`42 API error: ${res.status}`)); }
18-22: Remove unused injected services to reduce unnecessary coupling.
UserServiceandSocialAccountServiceare injected in the constructor but never used within this strategy. Thevalidatemethod only returns the 42 account data via thedonecallback, and user creation/linking happens elsewhere. These unused dependencies should be removed.♻️ Proposed fix
`@Injectable`() export class FortyTwoOAuthStrategy extends PassportStrategy(Strategy, "42") { constructor( config: ConfigService, - private readonly users: UserService, - private readonly socialAccounts: SocialAccountService, ) {Remove the corresponding import statements for
UserServiceandSocialAccountServiceas well.frontend/app/events/[id]/my-team/components/TeamInvitesDisplay.tsx (2)
110-111: TODO comments for loading states should be addressed.The
isLoadingprop is commented out for both buttons. Consider using the Button component's loading indicator (if supported) or wrapping with a spinner to provide visual feedback during async operations.Would you like me to open an issue to track implementing the loading states for these buttons?
Also applies to: 119-120
54-57: Inconsistent state management between accept and decline handlers.
handleAcceptInviteusesrouter.refresh()which triggers a full page data refetch, whilehandleDeclineInviteupdates local state withsetInvites. This inconsistency could cause UX issues if the server state diverges from local state. Consider using the same pattern for both, preferably leveraging React Query'sinvalidateQueriesfor consistency with the TanStack Query migration.Also applies to: 78-78
api/src/auth/auth.module.ts (1)
17-26: Consider security implications of 30-day JWT expiry.A 30-day token lifetime is quite long. If a token is compromised (e.g., cookie theft via XSS), the attacker has an extended window. Common practice is shorter-lived access tokens (e.g., 15 minutes to a few hours) with refresh tokens for session extension. Since this is stored in an httpOnly cookie, the risk is reduced but not eliminated.
If 30 days is intentional for UX reasons, consider adding token revocation capability (e.g., storing token IDs in a blocklist on logout or password change).
api/src/auth/jwt.strategy.ts (1)
37-44: Error handling swallows all errors fromgetUserById.Using
.catch(() => null)treats database/network errors the same as "user not found". A transient DB issue would result in anUnauthorizedExceptionrather than a 500 error, potentially confusing debugging. Consider logging the error or distinguishing between "not found" and other failures.♻️ Optional: Distinguish error types
async validate(payload: JwtPayload) { const userId = payload.sub; if (!userId) throw new UnauthorizedException(); // Load current user; if missing, reject token - const user = await this.users.getUserById(userId).catch(() => null); + const user = await this.users.getUserById(userId).catch((err) => { + // Log unexpected errors for debugging; still return null to trigger 401 + console.error(`Failed to load user ${userId}:`, err); + return null; + }); if (!user) throw new UnauthorizedException(); return user; // attaches to request.user }api/src/auth/github.strategy.ts (1)
40-50: Consider combining update and refetch into a single operation.After
updateUser, the code immediately fetches the same user withgetUserById. IfupdateUsercould return the updated entity, this would eliminate an extra database round-trip.frontend/app/events/[id]/my-team/components/TeamInfoDisplay.tsx (2)
51-65: Missing cache invalidation after successful leave.The mutation lacks an
onSuccesshandler to invalidate the team query cache. After leaving, the UI relies solely on the 3-second refetch interval to update, causing a brief period where stale data is shown.Proposed fix using queryClient invalidation
+import { useQueryClient } from "@tanstack/react-query"; ... +const queryClient = useQueryClient(); + const leaveTeamMutation = useMutation({ mutationFn: async () => { const result = await leaveTeam(eventId); if (isActionError(result)) { throw new Error(result.error); } }, onMutate: () => { plausible("leave_team"); setErrorMessage(null); }, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ["event", eventId, "my-team"] }); + }, onError: (error: Error) => { setErrorMessage(error.message); }, });
24-24: Dead state:isRepoPendingis always false.This state is initialized to
falseand never updated. If timer-based polling was intentionally removed, consider removing this state entirely or implementing the intended functionality.Proposed fix
-const [isRepoPending] = useState<boolean>(false); +const isRepoPending = false; // Polling removed; kept for TeamInfoSection propapi/src/auth/auth.controller.ts (3)
35-47: Redundant conditional check aftergetOrThrow.
getOrThrowthrows if the config key is missing, soredirectUrlwill always be truthy when line 38 is reached. Theifblock and the fallbackres.json({ token })on line 47 are unreachable.Proposed simplification
githubCallback(`@Req`() req: Request, `@Res`() res: Response) { const user: any = (req as any).user; const token = this.auth.signToken(user); const redirectUrl = this.configService.getOrThrow<string>( "OAUTH_SUCCESS_REDIRECT_URL", ); - if (redirectUrl) { - res.cookie("token", token, { - httpOnly: true, - secure: true, - sameSite: "none", - maxAge: 30 * 24 * 60 * 60 * 1000, - }); - return res.redirect(redirectUrl); - } - return res.json({ token }); + res.cookie("token", token, { + httpOnly: true, + secure: true, + sameSite: "none", + maxAge: 30 * 24 * 60 * 60 * 1000, + }); + return res.redirect(redirectUrl); }
104-114: Use NestJS Logger instead ofconsole.error.NestJS provides a built-in
Loggerservice that integrates with the framework's logging infrastructure and supports structured logging.Proposed fix
+import { Logger } from "@nestjs/common"; ... export class AuthController { + private readonly logger = new Logger(AuthController.name); constructor( ... } catch (e) { - console.error("Error in FortyTwo callback:", e); + this.logger.error("Error in FortyTwo callback", e instanceof Error ? e.stack : e);
32-34: Improve type safety for request user extraction.The double
anycast(req as any).userbypasses TypeScript's type checking. Consider using a typed request interface or a custom decorator.Proposed fix using typed interface
interface AuthenticatedRequest extends Request { user: { id: string; [key: string]: any }; } // Then in methods: githubCallback(`@Req`() req: AuthenticatedRequest, `@Res`() res: Response) { const user = req.user; // ... }Also applies to: 120-122
frontend/app/events/[id]/my-team/page.tsx (1)
49-54: Consider error handling forfetchQuery.
fetchQuerywill throw if the query fails. Without a try/catch, an API error will cause the page to fail entirely. Consider wrapping in try/catch or usingprefetchQuerywhich swallows errors.Option 1: Use prefetchQuery and handle null team
-const team = await queryClient.fetchQuery({ - queryKey: myTeamQueryKey(eventId), - queryFn: () => myTeamQueryFn(eventId), -}); +await queryClient.prefetchQuery({ + queryKey: myTeamQueryKey(eventId), + queryFn: () => myTeamQueryFn(eventId), +}); +const team = queryClient.getQueryData<Team | null>(myTeamQueryKey(eventId));Option 2: Wrap in try/catch with redirect on error
+let team: Team | null = null; +try { team = await queryClient.fetchQuery({ queryKey: myTeamQueryKey(eventId), queryFn: () => myTeamQueryFn(eventId), }); +} catch { + redirect(`/events/${eventId}`); +}
| GITHUB_CLIENT_SECRET= | ||
| GITHUB_CALLBACK_URL=http://localhost:4000/auth/github/callback | ||
| OAUTH_SUCCESS_REDIRECT_URL=http://localhost:3000/auth/sso | ||
| 42_OAUTH_SUCCESS_REDIRECT_URL=http://localhost:3000/profile |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and show its content
if [ -f "api/.env.example" ]; then
echo "=== Content of api/.env.example ==="
cat -n "api/.env.example"
else
echo "File api/.env.example not found"
fi
echo ""
echo "=== Searching for 42_OAUTH_SUCCESS_REDIRECT_URL ==="
rg "42_OAUTH_SUCCESS_REDIRECT_URL" || echo "Not found"
echo ""
echo "=== Searching for FORTYTWO_OAUTH_SUCCESS_REDIRECT_URL ==="
rg "FORTYTWO_OAUTH_SUCCESS_REDIRECT_URL" || echo "Not found"
echo ""
echo "=== Searching for any FORTYTWO_ prefixed variables in .env.example ==="
rg "FORTYTWO_" "api/.env.example" || echo "Not found"Repository: 42core-team/website
Length of output: 1319
🏁 Script executed:
# Check the actual usage in auth.controller.ts to see context
rg -B 2 -A 2 "42_OAUTH_SUCCESS_REDIRECT_URL" "api/src/auth/auth.controller.ts"Repository: 42core-team/website
Length of output: 179
Invalid environment variable name: starts with a digit. Fix requires changes in both .env.example and code.
42_OAUTH_SUCCESS_REDIRECT_URL is not a valid environment variable name in POSIX-compliant shells because it starts with a digit. This variable is referenced in api/src/auth/auth.controller.ts and must be renamed consistently in both locations.
The codebase already uses the FORTYTWO_* prefix for other 42 School OAuth variables (lines 21-23 in .env.example). Rename the variable to FORTYTWO_OAUTH_SUCCESS_REDIRECT_URL in both:
api/.env.example(line 18)api/src/auth/auth.controller.ts(in theconfigService.getOrThrow()call)
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 18-18: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 18-18: [UnorderedKey] The 42_OAUTH_SUCCESS_REDIRECT_URL key should go before the GITHUB_CALLBACK_URL key
(UnorderedKey)
🤖 Prompt for AI Agents
In `@api/.env.example` at line 18, Rename the invalid environment variable that
starts with a digit: change 42_OAUTH_SUCCESS_REDIRECT_URL to
FORTYTWO_OAUTH_SUCCESS_REDIRECT_URL in the .env.example and update the reference
inside the Auth controller where configService.getOrThrow(...) is called (search
for the string "42_OAUTH_SUCCESS_REDIRECT_URL" in auth.controller.ts) so the
same key is used; also update any other occurrences (tests, docs, or other
config lookups) to the new FORTYTWO_OAUTH_SUCCESS_REDIRECT_URL name to keep
usage consistent.
| tls: | ||
| - hosts: | ||
| - dev.api.coregame.de | ||
| - dev.api.coregame.sh | ||
| secretName: dev-api-coregame-de-tls |
There was a problem hiding this comment.
TLS secret name references old domain.
The TLS hosts have been updated to dev.api.coregame.sh, but secretName still references the old domain: dev-api-coregame-de-tls.
If the existing secret contains a certificate issued for *.coregame.de, it won't be valid for the new .sh domain, causing TLS errors. Either:
- Update the secret name to reflect the new domain and ensure cert-manager generates a new certificate, or
- Verify the existing secret already contains a valid certificate for the new domain.
Proposed fix (if creating new secret)
tls:
- hosts:
- dev.api.coregame.sh
- secretName: dev-api-coregame-de-tls
+ secretName: dev-api-coregame-sh-tls📝 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.
| tls: | |
| - hosts: | |
| - dev.api.coregame.de | |
| - dev.api.coregame.sh | |
| secretName: dev-api-coregame-de-tls | |
| tls: | |
| - hosts: | |
| - dev.api.coregame.sh | |
| secretName: dev-api-coregame-sh-tls |
🤖 Prompt for AI Agents
In `@api/helm/values-dev.yaml` around lines 43 - 46, The TLS secretName currently
references the old domain (secretName: dev-api-coregame-de-tls) while hosts
lists dev.api.coregame.sh; update the secretName to match the new domain (e.g.,
change secretName to a name reflecting .sh) and ensure cert-manager is
instructed to issue a certificate for dev.api.coregame.sh, or alternatively
confirm the existing secret contains a certificate valid for dev.api.coregame.sh
before leaving secretName unchanged; adjust the tls block's secretName and/or
recreate the TLS secret so the certificate SANs match the hosts entry.
| async upsertSocialAccountForUser(params: { | ||
| userId: string; | ||
| platform: SocialPlatform; | ||
| username: string; | ||
| platformUserId: string; | ||
| }): Promise<SocialAccountEntity> { | ||
| const existing = await this.socialAccountRepository.findOne({ | ||
| where: { userId: params.userId, platform: params.platform }, | ||
| }); | ||
|
|
||
| if (existing) { | ||
| existing.username = params.username; | ||
| existing.platformUserId = params.platformUserId; | ||
| return this.socialAccountRepository.save(existing); | ||
| } | ||
|
|
||
| const entity = this.socialAccountRepository.create({ | ||
| userId: params.userId, | ||
| platform: params.platform, | ||
| username: params.username, | ||
| platformUserId: params.platformUserId, | ||
| }); | ||
| return this.socialAccountRepository.save(entity); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f social-account -e tsRepository: 42core-team/website
Length of output: 230
🏁 Script executed:
find . -name "*social*account*" -type f | grep -E "\.(ts|js|sql)$"Repository: 42core-team/website
Length of output: 238
🏁 Script executed:
rg "SocialAccountEntity" --type ts -A 5 -B 5Repository: 42core-team/website
Length of output: 9550
🏁 Script executed:
rg "unique\|constraint" --type ts -i | head -20Repository: 42core-team/website
Length of output: 45
🏁 Script executed:
cat -n api/src/user/entities/social-account.entity.tsRepository: 42core-team/website
Length of output: 1269
🏁 Script executed:
rg "upsert" api/src --type ts -iRepository: 42core-team/website
Length of output: 245
🏁 Script executed:
cat api/package.json | grep -A 2 -B 2 "typeorm"Repository: 42core-team/website
Length of output: 1224
🏁 Script executed:
rg "Repository.*upsert" --type tsRepository: 42core-team/website
Length of output: 45
🏁 Script executed:
cat -n api/src/user/social-account.service.ts | head -80Repository: 42core-team/website
Length of output: 2693
Replace the find-then-save pattern with atomic upsert to prevent constraint violations under concurrent OAuth logins.
The current implementation is race-prone: if two OAuth login requests execute concurrently, both can pass the findOne check, then both attempt to insert. The second request will fail with a unique constraint violation. Although the @Unique(["userId", "platform"]) constraint exists on the entity (preventing actual duplicates), it causes avoidable errors rather than gracefully updating the record.
Use TypeORM's upsert() method keyed on (userId, platform) for atomic conflict resolution:
🛠️ Proposed fix (TypeORM upsert)
- const existing = await this.socialAccountRepository.findOne({
- where: { userId: params.userId, platform: params.platform },
- });
-
- if (existing) {
- existing.username = params.username;
- existing.platformUserId = params.platformUserId;
- return this.socialAccountRepository.save(existing);
- }
-
- const entity = this.socialAccountRepository.create({
- userId: params.userId,
- platform: params.platform,
- username: params.username,
- platformUserId: params.platformUserId,
- });
- return this.socialAccountRepository.save(entity);
+ await this.socialAccountRepository.upsert(
+ {
+ userId: params.userId,
+ platform: params.platform,
+ username: params.username,
+ platformUserId: params.platformUserId,
+ },
+ ["userId", "platform"],
+ );
+ return this.socialAccountRepository.findOne({
+ where: { userId: params.userId, platform: params.platform },
+ });📝 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.
| async upsertSocialAccountForUser(params: { | |
| userId: string; | |
| platform: SocialPlatform; | |
| username: string; | |
| platformUserId: string; | |
| }): Promise<SocialAccountEntity> { | |
| const existing = await this.socialAccountRepository.findOne({ | |
| where: { userId: params.userId, platform: params.platform }, | |
| }); | |
| if (existing) { | |
| existing.username = params.username; | |
| existing.platformUserId = params.platformUserId; | |
| return this.socialAccountRepository.save(existing); | |
| } | |
| const entity = this.socialAccountRepository.create({ | |
| userId: params.userId, | |
| platform: params.platform, | |
| username: params.username, | |
| platformUserId: params.platformUserId, | |
| }); | |
| return this.socialAccountRepository.save(entity); | |
| } | |
| async upsertSocialAccountForUser(params: { | |
| userId: string; | |
| platform: SocialPlatform; | |
| username: string; | |
| platformUserId: string; | |
| }): Promise<SocialAccountEntity> { | |
| await this.socialAccountRepository.upsert( | |
| { | |
| userId: params.userId, | |
| platform: params.platform, | |
| username: params.username, | |
| platformUserId: params.platformUserId, | |
| }, | |
| ["userId", "platform"], | |
| ); | |
| return this.socialAccountRepository.findOne({ | |
| where: { userId: params.userId, platform: params.platform }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@api/src/user/social-account.service.ts` around lines 52 - 75, The
upsertSocialAccountForUser implementation is race-prone; replace the
findOne-then-save flow with an atomic upsert using
this.socialAccountRepository.upsert(...) keyed on userId and platform to avoid
unique constraint errors; after calling socialAccountRepository.upsert({ userId,
platform, username, platformUserId }, { conflictPaths: ['userId','platform'] })
fetch and return the persisted entity via this.socialAccountRepository.findOne({
where: { userId: params.userId, platform: params.platform } }) so the method
still returns a SocialAccountEntity.
| useEffect(() => { | ||
| let cancelled = false; | ||
| (async () => { | ||
| try { | ||
| await signIn("backend", { redirect: false }); | ||
| if (!cancelled) { | ||
| router.replace("/"); | ||
| router.refresh(); | ||
| } | ||
| } | ||
| catch (e) { | ||
| console.error("Failed to finalize SSO:", e); | ||
| if (!cancelled) | ||
| router.replace("/auth/error"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
NextAuth.js signIn redirect false return value response object
💡 Result:
When you call signIn() with redirect: false (only supported for the Credentials and Email providers), it resolves to a “sign-in response” object shaped like:
type SignInResponse = {
error: string | undefined
status: number
ok: boolean
url: string | null
}So you can do:
const res = await signIn("credentials", { redirect: false, ...data })
if (res?.ok) {
// signed in
// optionally navigate to res.url (if present)
} else {
// res?.error contains the error code/message (if any)
}This behavior (and the exact fields) is documented in the NextAuth.js Client API docs. [1]
Note: there have been reports (notably around NextAuth/Auth.js v5 beta) where signIn(..., { redirect: false }) didn’t return the documented object (e.g., seemingly returning only a URL); see the related GitHub issue. [2]
🏁 Script executed:
# First, let's check the project's dependencies to see NextAuth version
fd -t f "package.json" | head -5 | xargs -I {} grep -l "next-auth" {}Repository: 42core-team/website
Length of output: 86
🏁 Script executed:
cat frontend/package.json | grep -A5 "next-auth\|auth.js"Repository: 42core-team/website
Length of output: 245
🏁 Script executed:
# Read the full file to see the context
cat -n frontend/app/auth/sso/page.tsxRepository: 42core-team/website
Length of output: 1295
Handle non-throwing signIn failures and returned redirect URL.
signIn(..., { redirect: false }) returns a response object with error, status, ok, and url fields rather than throwing on failure. The current code ignores this response, causing failed authentication to still redirect to /, and any provider-specific redirect URL from res.url is ignored.
✅ Proposed fix
- await signIn("backend", { redirect: false });
- if (!cancelled) {
- router.replace("/");
- router.refresh();
- }
+ const res = await signIn("backend", { redirect: false });
+ if (cancelled) return;
+ if (!res || res.error) {
+ router.replace("/auth/error");
+ return;
+ }
+ const target = res.url ?? "/";
+ router.replace(target);
+ router.refresh();📝 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.
| useEffect(() => { | |
| let cancelled = false; | |
| (async () => { | |
| try { | |
| await signIn("backend", { redirect: false }); | |
| if (!cancelled) { | |
| router.replace("/"); | |
| router.refresh(); | |
| } | |
| } | |
| catch (e) { | |
| console.error("Failed to finalize SSO:", e); | |
| if (!cancelled) | |
| router.replace("/auth/error"); | |
| } | |
| useEffect(() => { | |
| let cancelled = false; | |
| (async () => { | |
| try { | |
| const res = await signIn("backend", { redirect: false }); | |
| if (cancelled) return; | |
| if (!res || res.error) { | |
| router.replace("/auth/error"); | |
| return; | |
| } | |
| const target = res.url ?? "/"; | |
| router.replace(target); | |
| router.refresh(); | |
| } | |
| catch (e) { | |
| console.error("Failed to finalize SSO:", e); | |
| if (!cancelled) | |
| router.replace("/auth/error"); | |
| } | |
| })(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In `@frontend/app/auth/sso/page.tsx` around lines 9 - 23, The async sign-in flow
currently ignores the response from signIn and treats failures the same as
successes; update the useEffect handler (the async IIFE that calls signIn) to
await and inspect the returned response object from signIn("backend", {
redirect: false }) instead of assuming it throws: if res.error or !res.ok then,
when not cancelled, call router.replace("/auth/error"); else if res.url is
present then navigate to the provider redirect (e.g., set window.location.href =
res.url) instead of router.replace("/"), and only call router.replace("/") and
router.refresh() when res.ok is true and no external res.url exists; keep the
try/catch to log real exceptions.
| const { data: team } = useQuery<Team | null>({ | ||
| refetchInterval: 3000, | ||
| queryKey: ["event", eventId, "my-team"], | ||
| queryFn: async () => { | ||
| const response = await axiosInstance.get<Team | null>( | ||
| `/team/event/${eventId}/my`, | ||
| ); | ||
| return response.data; | ||
| }, | ||
| initialData: initialTeam, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the queries file and check the myTeamQueryKey definition
fd -type f -name "queries*" frontend/Repository: 42core-team/website
Length of output: 233
🏁 Script executed:
# Also check the TeamInfoDisplay.tsx file to understand context
head -100 frontend/app/events/[id]/my-team/components/TeamInfoDisplay.tsxRepository: 42core-team/website
Length of output: 2645
🏁 Script executed:
# Find and inspect page.tsx in the same directory
fd -type f -name "page.tsx" frontend/app/events/[id]/my-team/Repository: 42core-team/website
Length of output: 233
🏁 Script executed:
# Find queries file in the frontend directory
find frontend -name "*queries*" -type fRepository: 42core-team/website
Length of output: 148
🏁 Script executed:
# Find page.tsx in the my-team directory structure
find frontend/app/events/*/my-team -name "page.tsx" -type fRepository: 42core-team/website
Length of output: 104
🏁 Script executed:
# Search for myTeamQueryKey definition anywhere
rg "myTeamQueryKey" frontend/Repository: 42core-team/website
Length of output: 446
🏁 Script executed:
# Get the full definition of myTeamQueryKey and teamMembersQueryKey
cat frontend/app/events/[id]/my-team/queries.tsRepository: 42core-team/website
Length of output: 1059
🏁 Script executed:
# Check what page.tsx is doing with these keys
cat frontend/app/events/[id]/my-team/page.tsxRepository: 42core-team/website
Length of output: 2008
🏁 Script executed:
# Check leaveTeam function to understand expected behavior
rg "export.*leaveTeam" frontend/app/actions/team.ts -A 10Repository: 42core-team/website
Length of output: 364
🏁 Script executed:
# Check if there's any redirect or side effect after leaveTeam in other files
rg "leaveTeam" frontend/app/events/\[id\]/my-team/ -B 2 -A 5Repository: 42core-team/website
Length of output: 3140
🏁 Script executed:
# Check if there's any handling in teamView.tsx or elsewhere after leaveTeam
cat frontend/app/events/[id]/my-team/teamView.tsx | head -80Repository: 42core-team/website
Length of output: 2368
Add onSuccess handler to leaveTeamMutation to invalidate cached queries.
After successfully leaving the team, the cached team and pending invites data should be invalidated to ensure the UI reflects the updated state. Currently, the mutation lacks an onSuccess handler, which means stale data may persist until the 3-second refetch interval triggers.
Also, remove the unused isRepoPending state (line 24) — it's initialized to false and never modified, yet passed to TeamInfoSection.
Suggested fix
+import { myTeamQueryKey, pendingInvitesQueryKey } from "../queries";
+import { useQueryClient } from "@tanstack/react-query";
...
+const queryClient = useQueryClient();
...
const leaveTeamMutation = useMutation({
mutationFn: async () => {
const result = await leaveTeam(eventId);
if (isActionError(result)) {
throw new Error(result.error);
}
},
onMutate: () => {
plausible("leave_team");
setErrorMessage(null);
},
+ onSuccess: () => {
+ queryClient.invalidateQueries({ queryKey: myTeamQueryKey(eventId) });
+ queryClient.invalidateQueries({ queryKey: pendingInvitesQueryKey(eventId) });
+ },
onError: (error: Error) => {
setErrorMessage(error.message);
},
});-const [errorMessage, setErrorMessage] = useState<string | null>(null);
-const [isRepoPending] = useState<boolean>(false);
+const [errorMessage, setErrorMessage] = useState<string | null>(null);- isRepoPending={isRepoPending}🤖 Prompt for AI Agents
In `@frontend/app/events/`[id]/my-team/components/TeamInfoDisplay.tsx around lines
27 - 37, Add an onSuccess handler to the leaveTeamMutation so that after a
successful API call you invalidate the cached queries for the current event's
team and pending invites (e.g., invalidateQueries(["event", eventId, "my-team"])
and invalidateQueries(["event", eventId, "pending-invites"]) via the react-query
QueryClient) to ensure the UI updates immediately; also remove the unused
isRepoPending state and stop passing it into TeamInfoSection (delete its
declaration and the prop usage). Use the existing eventId and leaveTeamMutation
identifiers to locate where to attach the onSuccess and call
queryClient.invalidateQueries.
| useEffect(() => { | ||
| async function fetchQueueState() { | ||
| const newQueueState = await getQueueState(props.eventId); | ||
| if ( | ||
| queueState.match?.state === MatchState.IN_PROGRESS | ||
| && newQueueState.match?.state !== MatchState.IN_PROGRESS | ||
| ) { | ||
| if (newQueueState.match) { | ||
| router.push(`/events/${eventId}/match/${newQueueState?.match?.id}`); | ||
| } | ||
| } | ||
| setQueueState(newQueueState); | ||
| if (!queueState) { | ||
| return; | ||
| } | ||
|
|
||
| const interval = setInterval(fetchQueueState, 600); | ||
| return () => clearInterval(interval); | ||
| }); | ||
| if (queueState.match?.state === MatchState.IN_PROGRESS) { | ||
| return; | ||
| } | ||
|
|
||
| const previousMatchState = props.queueState.match?.state; | ||
| if ( | ||
| previousMatchState === MatchState.IN_PROGRESS | ||
| && queueState.match | ||
| ) { | ||
| router.push(`/events/${eventId}/match/${queueState.match.id}`); | ||
| } | ||
| }, [queueState, router, eventId, props.queueState.match?.state]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "queueState.tsx" | head -20Repository: 42core-team/website
Length of output: 110
🏁 Script executed:
cat -n ./frontend/app/events/[id]/queue/queueState.tsxRepository: 42core-team/website
Length of output: 5202
🏁 Script executed:
cat -n ./frontend/app/events/[id]/queue/queueState.tsx | sed -n '35,79p'Repository: 42core-team/website
Length of output: 1570
🏁 Script executed:
rg -l "QueueState" --type tsx --type ts | grep -v node_modules | head -20Repository: 42core-team/website
Length of output: 90
🏁 Script executed:
rg -l "QueueState" --type ts | head -20Repository: 42core-team/website
Length of output: 315
🏁 Script executed:
cat -n ./frontend/app/events/[id]/queue/page.tsxRepository: 42core-team/website
Length of output: 3589
Track previous match state with useRef instead of reading from static props.
The redirect logic has a critical flaw: props.queueState is set once during server-side rendering and never updates in the client. The queueState variable (from useQuery with refetchInterval: 600) updates regularly, but the redirect compares its state transition against a static previous value from props. This means transitions detected after the initial render won't trigger the redirect.
Use a ref to track actual previous state across all queueState updates:
🔧 Proposed fix
-import { useEffect } from "react";
+import { useEffect, useRef } from "react";
@@
const router = useRouter();
const { id } = useParams();
const eventId = id as string;
+ const previousMatchStateRef = useRef<MatchState | undefined>(
+ props.queueState.match?.state,
+ );
@@
useEffect(() => {
if (!queueState) {
return;
}
- if (queueState.match?.state === MatchState.IN_PROGRESS) {
- return;
- }
-
- const previousMatchState = props.queueState.match?.state;
- if (
- previousMatchState === MatchState.IN_PROGRESS
- && queueState.match
- ) {
- router.push(`/events/${eventId}/match/${queueState.match.id}`);
- }
- }, [queueState, router, eventId, props.queueState.match?.state]);
+ const previousMatchState = previousMatchStateRef.current;
+ if (
+ previousMatchState === MatchState.IN_PROGRESS &&
+ queueState.match &&
+ queueState.match.state !== MatchState.IN_PROGRESS
+ ) {
+ router.push(`/events/${eventId}/match/${queueState.match.id}`);
+ }
+
+ previousMatchStateRef.current = queueState.match?.state;
+ }, [queueState, router, eventId]);🤖 Prompt for AI Agents
In `@frontend/app/events/`[id]/queue/queueState.tsx around lines 63 - 79, The
redirect compares the live queueState to the static props.queueState, so it
never detects client-side transitions; replace the props-based previous state
with a ref: create a prevMatchStateRef via useRef, initialize it to
props.queueState.match?.state, then in the useEffect that watches queueState
(and router/eventId) compare prevMatchStateRef.current to
queueState.match?.state (checking for transition from MatchState.IN_PROGRESS to
a new match), call router.push when appropriate, and finally update
prevMatchStateRef.current = queueState.match?.state; ensure you remove
props.queueState.match?.state from the effect deps and reference the symbols
queueState, props.queueState, prevMatchStateRef, useEffect,
MatchState.IN_PROGRESS, and router.push to locate the code.
| <Link color="primary" href="/events/create"> | ||
| Create Event | ||
| </Link> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/app/events/page.tsx | head -60Repository: 42core-team/website
Length of output: 2201
🏁 Script executed:
# Check imports at the top of the file
head -20 frontend/app/events/page.tsxRepository: 42core-team/website
Length of output: 667
🏁 Script executed:
# Get a broader view around lines 52-54
sed -n '40,65p' frontend/app/events/page.tsxRepository: 42core-team/website
Length of output: 808
🏁 Script executed:
# Check package.json for UI libraries
cat package.json | grep -A 50 '"dependencies"'Repository: 42core-team/website
Length of output: 107
🏁 Script executed:
# Search for other Link usages to see if there's a pattern
rg '<Link' frontend/app -A 2 -B 1 | head -50Repository: 42core-team/website
Length of output: 2606
🏁 Script executed:
# Find package.json
fd package.jsonRepository: 42core-team/website
Length of output: 129
🏁 Script executed:
# Check if there are UI library imports elsewhere
rg "from.*ui|from.*nextui|from.*shadcn" frontend/app --no-heading | head -20Repository: 42core-team/website
Length of output: 1784
🏁 Script executed:
# Check frontend package.json for dependencies
cat frontend/package.json | grep -A 100 '"dependencies"'Repository: 42core-team/website
Length of output: 2559
🏁 Script executed:
# Check if there's a Link component in the UI components folder
fd Link frontend/components/uiRepository: 42core-team/website
Length of output: 45
🏁 Script executed:
# Check the components/ui directory structure
ls -la frontend/components/ui/ | head -30Repository: 42core-team/website
Length of output: 1577
🏁 Script executed:
# Check if there's a custom Link component anywhere
rg "export.*Link|export const Link" frontend/componentsRepository: 42core-team/website
Length of output: 45
🏁 Script executed:
# Check the button component to see if it wraps Link
cat frontend/components/ui/button.tsxRepository: 42core-team/website
Length of output: 2018
🏁 Script executed:
# Check the TypeScript definitions for next/link to confirm color prop doesn't exist
rg "type LinkProps|interface LinkProps" --type ts frontend/node_modules/next/dist/clientRepository: 42core-team/website
Length of output: 141
🏁 Script executed:
# Verify the pattern: check if other Link uses className instead
rg '<Link' frontend/app --no-heading -A 1 | grep -E "(className|color)"Repository: 42core-team/website
Length of output: 146
🏁 Script executed:
# Double-check: search for any TypeScript errors or compiler output related to this
rg "color.*prop|prop.*color" frontend/app/events/page.tsxRepository: 42core-team/website
Length of output: 45
Remove unsupported color prop from next/link.
The color prop is not valid for next/link. Use className with Tailwind classes instead (e.g., className="text-primary").
Suggested fix
- <Link color="primary" href="/events/create">
+ <Link href="/events/create" className="text-primary">
Create Event
</Link>📝 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.
| <Link color="primary" href="/events/create"> | |
| Create Event | |
| </Link> | |
| <Link href="/events/create" className="text-primary"> | |
| Create Event | |
| </Link> |
🤖 Prompt for AI Agents
In `@frontend/app/events/page.tsx` around lines 52 - 54, The Link component usage
in events/page.tsx incorrectly passes a non-existent color prop to next/link;
remove the color="primary" prop from the <Link> element and instead add a
Tailwind class via className (for example className="text-primary" or whatever
design token you use) to style the link; update the <Link> with className and
ensure any nested children receive the styling as needed (reference the Link
element in this file).
| const queryClient = new QueryClient(); | ||
|
|
||
| export default function Providers({ children }) { | ||
| return ( | ||
| <QueryClientProvider client={queryClient}> | ||
| {children} | ||
| <ReactQueryDevtools initialIsOpen={false} /> | ||
| </QueryClientProvider> | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TanStack React Query Next.js App Router QueryClient initialization best practices
💡 Result:
Best practices (Next.js App Router + TanStack React Query)
1) Put QueryClientProvider in a Client Component “Providers” wrapper
React context/providers aren’t supported in Server Components, so create a dedicated client wrapper (e.g. app/providers.tsx) and import it from app/layout.tsx. [3]
2) Create the QueryClient per request on the server, but as a stable singleton in the browser
TanStack’s App Router guidance is:
- Server: always make a new
QueryClient(prevents cross-request/user cache sharing). - Browser: reuse one
QueryClientinstance (prevents recreating the client on re-renders / suspense). [1]
// app/providers.tsx
'use client'
import { QueryClient, QueryClientProvider, isServer } from '@tanstack/react-query'
function makeQueryClient() {
return new QueryClient({
defaultOptions: {
queries: {
staleTime: 60 * 1000, // helps avoid immediate refetch after SSR
},
},
})
}
let browserQueryClient: QueryClient | undefined
function getQueryClient() {
if (isServer) return makeQueryClient()
if (!browserQueryClient) browserQueryClient = makeQueryClient()
return browserQueryClient
}
export function Providers({ children }: { children: React.ReactNode }) {
const queryClient = getQueryClient()
return <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
}3) For SSR/prefetch: prefetch in Server Components, then dehydrate → <HydrationBoundary>
In the App Router model, treat Server Components as a “prefetching/loading phase”: create a fresh QueryClient, prefetchQuery, then pass the dehydrated cache into a <HydrationBoundary> that wraps the Client Component(s) that call useQuery. [1][2]
4) Mind server memory: clear or tune gcTime when you create per-request clients
If you’re creating a QueryClient per request, TanStack notes you may need to queryClient.clear() after dehydration (or adjust gcTime carefully; avoid gcTime: 0 because it can cause hydration issues). [4]
Sources: TanStack Advanced SSR (Next.js App Router) [1], TanStack SSR & Hydration guide [2], Next.js App Router composition/context provider pattern [3], TanStack SSR memory/gcTime notes [4].
🌐 Web query:
React Query module scope singleton state leakage Next.js SSR
💡 Result:
In Next.js SSR, anything you create at module scope (file top-level) in a Node.js server process can be reused across requests. If you do this with React Query / TanStack Query:
// ❌ shared across all SSR requests in the same server process
export const queryClient = new QueryClient()…then the QueryClient cache becomes a singleton, so request A can populate the cache and request B can read/serialize it, causing cross-user data leakage and also unnecessary cache growth. TanStack Query explicitly warns against creating the QueryClient at the file root for SSR because it makes the cache shared “between all requests” and can leak sensitive data. [1]
Fix patterns (safe)
1) Pages Router (pages/): create it inside _app (not module scope)
Create the client in React state (or a ref) so it’s per request on the server and stable per browser session on the client. [1]
// pages/_app.tsx
import * as React from 'react'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
export default function MyApp({ Component, pageProps }) {
const [queryClient] = React.useState(() => new QueryClient())
return (
<QueryClientProvider client={queryClient}>
<Component {...pageProps} />
</QueryClientProvider>
)
}2) App Router (app/) with Server Components: use a request-scoped singleton via cache()
If you want “singleton-like” reuse within the same request (but not across requests), TanStack Query recommends using React’s cache() (scoped per request) in Next.js App Router. [2]
// app/getQueryClient.ts
import { QueryClient } from '@tanstack/react-query'
import { cache } from 'react'
// ✅ request-scoped, avoids cross-request leakage
export const getQueryClient = cache(() => new QueryClient())Then prefetch/dehydrate using that request-scoped client. [2]
Rule of thumb: in SSR, never put new QueryClient() at module scope. Create it per request (or request-scoped via cache() in App Router). [1][2]
Sources: [1] [2]
🌐 Web query:
React Query useState QueryClient initializer Next.js
💡 Result:
In Next.js, create the QueryClient inside a Client Component and keep it stable across re-renders by using a lazy initializer (or a “getQueryClient” helper). Do not create it at module scope (const queryClient = new QueryClient()), especially with SSR, because it can share cache across requests/users. [1]
App Router (app/) – recommended (TanStack Query v5)
TanStack’s SSR guide shows a getQueryClient() approach that creates a new client on the server, but reuses a singleton in the browser (and notes a Suspense-related caveat about useState). [2]
// app/providers.tsx
'use client'
import { QueryClient, QueryClientProvider, isServer } from '@tanstack/react-query'
function makeQueryClient() {
return new QueryClient({
defaultOptions: { queries: { staleTime: 60_000 } },
})
}
let browserQueryClient: QueryClient | undefined
function getQueryClient() {
if (isServer) return makeQueryClient()
if (!browserQueryClient) browserQueryClient = makeQueryClient()
return browserQueryClient
}
export function Providers({ children }: { children: React.ReactNode }) {
const queryClient = getQueryClient()
return <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
}Then wrap your layout:
// app/layout.tsx
import { Providers } from './providers'
export default function RootLayout({ children }: { children: React.ReactNode }) {
return (
<html lang="en">
<body><Providers>{children}</Providers></body>
</html>
)
}Pages Router (pages/) – common pattern
Use React state with a lazy initializer so the client is created once per app/request lifecycle (and not recreated on every render). [1]
// pages/_app.tsx
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
import * as React from 'react'
export default function MyApp({ Component, pageProps }) {
const [queryClient] = React.useState(
() => new QueryClient({ defaultOptions: { queries: { staleTime: 60_000 } } })
)
return (
<QueryClientProvider client={queryClient}>
<Component {...pageProps} />
</QueryClientProvider>
)
}Why useState(() => new QueryClient())? It lazily constructs the client once and keeps the same instance across re-renders, preventing cache resets and avoiding shared caches across SSR requests when done in the right place. [1]
Sources: TanStack Query “Server Rendering & Hydration” (v5) and “Advanced SSR” guide. [1] [2]
QueryClient instantiation at module scope causes cross-user cache leakage in Next.js App Router.
Creating QueryClient at module level is unsafe in Next.js SSR. The same instance is reused across all server requests, allowing one user's cached data to be served to another user.
TanStack Query's recommended pattern for App Router is to create a fresh QueryClient on the server per request, but maintain a stable singleton in the browser. Use the isServer helper with a getQueryClient() function rather than useState:
Proposed fix
"use client";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
+import { isServer } from "@tanstack/react-query";
import { ReactQueryDevtools } from "@tanstack/react-query-devtools";
-const queryClient = new QueryClient();
+function makeQueryClient() {
+ return new QueryClient({
+ defaultOptions: {
+ queries: {
+ staleTime: 60 * 1000,
+ },
+ },
+ });
+}
+
+let browserQueryClient: QueryClient | undefined;
+
+function getQueryClient() {
+ if (isServer) return makeQueryClient();
+ if (!browserQueryClient) browserQueryClient = makeQueryClient();
+ return browserQueryClient;
+}
-export default function Providers({ children }) {
+export default function Providers({
+ children,
+}: {
+ children: React.ReactNode;
+}) {
+ const queryClient = getQueryClient();
+
return (
<QueryClientProvider client={queryClient}>
{children}
<ReactQueryDevtools initialIsOpen={false} />
</QueryClientProvider>
);
}🤖 Prompt for AI Agents
In `@frontend/app/providers.tsx` around lines 6 - 14, The module-level QueryClient
instance (const queryClient) in Providers leads to cross-request cache leakage;
remove that top-level instantiation and implement a getQueryClient() helper that
returns a new QueryClient when isServer() is true (per-request SSR) and a stable
singleton in the browser, then update the Providers component to call
getQueryClient() and pass its result to <QueryClientProvider client={...}> while
preserving ReactQueryDevtools and children; ensure the unique symbols referenced
are QueryClient, Providers, QueryClientProvider, getQueryClient, and isServer.
| try { | ||
| const res = await axiosInstance.get<{ | ||
| id: string; | ||
| username: string; | ||
| email: string; | ||
| profilePicture: string; | ||
| }>(`/auth/me`); | ||
|
|
||
| if (dbUser) | ||
| session.user.id = dbUser.id; | ||
| session.user.id = res.data.id; | ||
| session.user.email = res.data.email; | ||
| session.user.name = res.data.username; | ||
| session.user.profilePicture = res.data.profilePicture; | ||
| } | ||
| catch { | ||
| session.user.id = ""; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/app/utils/authOptions.ts | head -100Repository: 42core-team/website
Length of output: 2572
🏁 Script executed:
rg "useSession\|NextAuthOptions\|session.*callback" frontend/app/utils/authOptions.ts -A 5 -B 5Repository: 42core-team/website
Length of output: 45
🌐 Web query:
NextAuth.js v4 session callback return null unauthenticated
💡 Result:
In NextAuth.js v4, the callbacks.session callback is expected to always return a session object (e.g. return session). The official docs only describe returning a session object, not null. [1]
If you want a request to be treated as unauthenticated, don’t try to return null from session(). There’s an (older) feature request asking for this (“Allow the session callback to return null”) and the suggested workaround is to return a session with a flag (e.g. isAuthenticated: false) and then convert it to null at the call site (e.g. after getServerSession). [2]
Common related gotcha: if you’re seeing getServerSession() (or /api/auth/session) always return null, make sure you have NEXTAUTH_SECRET (or an explicit JWT secret) set; otherwise session decryption can fail and you’ll effectively get null sessions. [3]
Sources:
[1] NextAuth.js v4 callbacks docs (session callback examples) (next-auth.js.org)
[2] GitHub issue: “Allow the session callback to return null” (#7035) (github.com)
[3] GitHub discussion: getServerSession always null; fix by setting NEXTAUTH_SECRET (github.com)
Citations:
- 1: https://next-auth.js.org/configuration/callbacks?utm_source=openai
- 2: Allow the session callback to return null nextauthjs/next-auth#7035
- 3: getServerSession Is Always Null nextauthjs/next-auth#7062
Clear all session user fields on /auth/me error to prevent stale data.
On error, only session.user.id is cleared while name/email/profilePicture remain from the JWT, causing useSession() to report authenticated with outdated information. NextAuth.js v4 session callbacks cannot return null to mark a user unauthenticated (see next-auth.js.org/configuration/callbacks). Instead, clear all affected fields on error:
catch {
- session.user.id = "";
+ session.user.id = "";
+ session.user.email = "";
+ session.user.name = "";
+ session.user.profilePicture = "";
}Alternatively, set an isAuthenticated flag to false and handle it at the call site to properly treat the session as unauthenticated.
🤖 Prompt for AI Agents
In `@frontend/app/utils/authOptions.ts` around lines 58 - 73, On failure of the
axiosInstance.get('/auth/me') call, clear all session user fields instead of
only session.user.id to avoid stale JWT-derived data; in the catch block set
session.user.id, session.user.name, session.user.email, and
session.user.profilePicture to empty values (or add/set an explicit
session.user.isAuthenticated = false and handle that at the call site) so
useSession() no longer reports an authenticated user with outdated information.
| const base | ||
| = process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL; | ||
| window.location.href = `${base?.replace(/\/$/, "")}/auth/github/callback`; | ||
| } |
There was a problem hiding this comment.
Guard against missing backend URL to avoid broken redirects.
If NEXT_PUBLIC_BACKEND_PUBLIC_URL is unset, Line 9 builds a undefined/auth/... path and silently redirects to a bad URL. Fail fast and log clearly.
✅ Suggested fix
- const base
- = process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL;
- window.location.href = `${base?.replace(/\/$/, "")}/auth/github/callback`;
+ const base = process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL?.replace(/\/$/, "");
+ if (!base) {
+ console.error("NEXT_PUBLIC_BACKEND_PUBLIC_URL is not set");
+ return;
+ }
+ window.location.href = `${base}/auth/github/callback`;📝 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.
| const base | |
| = process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL; | |
| window.location.href = `${base?.replace(/\/$/, "")}/auth/github/callback`; | |
| } | |
| const base = process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL?.replace(/\/$/, ""); | |
| if (!base) { | |
| console.error("NEXT_PUBLIC_BACKEND_PUBLIC_URL is not set"); | |
| return; | |
| } | |
| window.location.href = `${base}/auth/github/callback`; | |
| } |
🤖 Prompt for AI Agents
In `@frontend/components/github.tsx` around lines 7 - 10, The redirect builds a
broken URL when process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL is missing; update
the logic around the code that reads NEXT_PUBLIC_BACKEND_PUBLIC_URL and sets
window.location.href (the block using base =
process.env.NEXT_PUBLIC_BACKEND_PUBLIC_URL and then window.location.href =
`${base?.replace(/\/$/, "")}/auth/github/callback`) to validate base first, and
if it's falsy, log a clear error (or surface a UI error) and do not perform the
redirect; only call replace(...) and assign window.location.href when base is a
non-empty string.
# Conflicts: # frontend/pnpm-lock.yaml
…ement automated repository creation for teams when events start
…ciency, and rename autoLockEvents to autoCreateRepos
…calendar components
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/components/event-navbar.tsx (1)
35-64: Nav items won’t update after the start time passesIf a user lands before the start time and stays on the page,
hasStartedwon’t flip until another re-render happens, so “Group Phase / Tournament Tree” can remain hidden. Consider a timed state update or server-provided started flag to keep the UI accurate.🔧 Suggested fix (timer-based update)
-import { useMemo } from "react"; +import { useEffect, useMemo, useState } from "react"; @@ - const hasStarted = Date.now() >= new Date(event.startDate).getTime(); + const startTs = new Date(event.startDate).getTime(); + const [hasStarted, setHasStarted] = useState(() => Date.now() >= startTs); + + useEffect(() => { + if (!Number.isFinite(startTs)) return; + if (Date.now() >= startTs) { + setHasStarted(true); + return; + } + const id = setTimeout(() => setHasStarted(true), startTs - Date.now()); + return () => clearTimeout(id); + }, [startTs]);github-service/src/repo.utils.ts (1)
266-276: Bug: Duplicated folder path inupdateTeamName.The
repoRootparameter already includesMY_CORE_BOT_FOLDER(set at line 48 viapath.join(tempFolderPath, this.MY_CORE_BOT_FOLDER)), but line 271-273 joins it again withthis.MY_CORE_BOT_FOLDER. This results in an invalid path likebots/softcore/bots/softcore/src/main.c.🐛 Proposed fix
private async updateTeamName( repoRoot: string, teamName: string, ): Promise<void> { try { const mainCPath = path.join( repoRoot, - this.MY_CORE_BOT_FOLDER, "src", "main.c", );github-service/src/app.service.ts (2)
264-279: Critical: Error swallowed after repository deletion causes inconsistent state.When cloning/pushing fails (lines 264-274), the error is caught, logged, and the repository is deleted, but execution continues without re-throwing. This causes:
- The
repository_createdevent to be emitted (lines 276-279) even though creation failed- Subsequent collaborator operations (lines 281-302) to fail on a non-existent repository
The comment states "Error is handled by the outer catch block" but the error is not re-thrown, so the outer catch never executes.
🐛 Proposed fix
} catch (e) { this.logger.error( `Failed to clone mono repo and push to team repo for repo ${name}`, e as Error, ); await this.deleteRepository(name, githubOrg, secret); - // Error is handled by the outer catch block; do not re-throw here. + throw e; // Re-throw to prevent emitting repository_created and adding collaborators } finally { await fs.rm(tempFolderPath, { recursive: true, force: true }); this.logger.log(`Removed temp folder ${tempFolderPath}`); }
26-31: Incorrect error code check: should be "EEXIST" not "EXIST".Node.js uses
"EEXIST"for the error code when a file/directory already exists. The current conditionerror.code !== "EXIST"will always be true for this case, making the warning log fire for the wrong reason.🐛 Proposed fix
.catch((error) => { - if (error.code !== "EXIST") { + if (error.code === "EEXIST") { this.logger.warn( `Failed to create temp folder at ${this.TMP_FOLDER} because it already exists`, ); + } else { + this.logger.error( + `Failed to create temp folder at ${this.TMP_FOLDER}`, + error, + ); } });
🤖 Fix all issues with AI agents
In `@api/src/auth/auth.controller.ts`:
- Around line 110-114: The thrown BadRequestException in auth.controller.ts
currently interpolates e.message and may leak internal error details; change the
throw to use a generic message (e.g., "Invalid state parameter.") and remove
inclusion of e.message while leaving the existing debug log at the earlier line
intact; target the BadRequestException throw that references the variable e in
the relevant method of the AuthController to make this change.
- Around line 35-47: The code in AuthController where redirectUrl is assigned
via this.configService.getOrThrow<string>("OAUTH_SUCCESS_REDIRECT_URL") is
checking if (redirectUrl) even though getOrThrow will always return or throw,
making the conditional dead code; either remove the if and always set the cookie
+ res.redirect(redirectUrl) (if redirect is mandatory), or change getOrThrow to
this.configService.get<string>("OAUTH_SUCCESS_REDIRECT_URL") and keep the if to
allow fallback to return res.json({ token }) when redirectUrl is undefined —
update the block in auth.controller.ts (the redirectUrl variable and surrounding
cookie/redirect/JSON logic) accordingly.
In `@api/src/auth/github.strategy.ts`:
- Around line 51-60: The new-user creation passes profilePicture from the GitHub
profile which may be undefined; update the call to users.createUser in
github.strategy.ts (the block that constructs new user via
this.users.createUser) to provide a safe default (e.g., empty string or a
configured placeholder URL) using a nullish fallback for profilePicture so the
database always receives a string and downstream code won’t break.
In `@api/src/constants.ts`:
- Around line 1-4: Replace the hardcoded lock key literal 12346 in
match.service.ts with the enum value LockKeys.CREATE_TEAM_REPOS; locate where
lockKey is assigned (the const lockKey = ... usage) and change it to use
LockKeys.CREATE_TEAM_REPOS, and ensure LockKeys is imported from the constants
module (add or update the import for LockKeys if missing).
In `@api/src/team/team.service.ts`:
- Line 149: The repoName construction using team.event.name + "-" + team.name +
"-" + team.id must be sanitized for GitHub: create (or call) a sanitizeRepoName
utility and replace/normalize characters from team.event.name and team.name
(e.g., remove/replace non-alphanumeric characters with hyphens or underscores,
convert to lowercase, collapse consecutive hyphens, trim leading/trailing
hyphens, and enforce a sane length), then build repoName =
`${sanitize(part1)}-${sanitize(part2)}-${team.id}`; update the code around the
repoName assignment and export the sanitize function for reuse.
- Around line 175-190: The outer createTeam wraps work in dataSource.transaction
but then calls createTeamRepository which starts its own independent
transaction, risking inconsistent side-effects if the outer transaction rolls
back; update createTeamRepository to accept an optional EntityManager (or
TransactionalRepository) parameter and, inside createTeam, pass the transaction
manager (the value returned to the transaction callback) when calling
createTeamRepository (use eventService.hasEventStarted(eventId) as before, then
await createTeamRepository(newTeam.id, manager) so both DB operations share the
same transaction), or alternatively remove the createTeamRepository call from
inside the transaction and invoke it after the transaction completes; adjust
function signatures accordingly (createTeamRepository(..., manager?:
EntityManager)).
- Around line 44-49: The current use of teamRepository.findBy cannot filter on
nested relation fields or include relations; replace the findBy call with
teamRepository.find({ relations: { event: true }, where: {
startedRepoCreationAt: IsNull(), event: { startDate: LessThanOrEqual(new Date())
} } }) or implement the equivalent QueryBuilder query so the event relation is
joined and the nested condition on event.startDate is applied; update any
references to the old findBy invocation accordingly and keep the predicates
startedRepoCreationAt, event.startDate and LessThanOrEqual identical.
In `@frontend/app/events/EventTable.tsx`:
- Line 25: Remove the debug console.log call that's printing every event in the
EventTable component: delete the line console.log("try to format state for
event", event) (or replace it with a development-only guard like if
(process.env.NODE_ENV !== 'production') { /* debug log */ }) so the EventTable
rendering no longer spams the browser console; locate the statement inside the
EventTable component where the event is being formatted and remove or
conditionally guard it.
- Around line 35-45: Replace the existing status logic that checks only
event.currentRound with logic that uses both event.currentRound and
event.endDate: treat event.currentRound === 0 as the "Initial"/not-started
state, treat the event as "Completed" only when new Date() is after new
Date(event.endDate), and treat event.currentRound > 0 with endDate in the future
as "In Progress"; update the return objects (text and variant) accordingly where
the current snippet lives (EventTable.tsx) using the event.currentRound and
event.endDate fields.
In `@frontend/components/event-info-notice.tsx`:
- Around line 19-32: The refresh guard didRefreshRef is never reset when a new
event (startsAt) is loaded, so add logic to reset it whenever startsAt changes;
specifically, add a small useEffect that watches startsAt and sets
didRefreshRef.current = false (referencing didRefreshRef and startsAt) so the
countdown can trigger router.refresh() for the new event.
🧹 Nitpick comments (6)
api/src/match/match.service.ts (1)
50-52: UseLockKeys.CREATE_TEAM_REPOSinstead of hardcoded value.A new
LockKeysenum was introduced inapi/src/constants.tsspecifically to centralize advisory lock keys. This hardcoded12346should useLockKeys.CREATE_TEAM_REPOSfor consistency and maintainability.♻️ Proposed fix
+import { LockKeys } from "../constants"; + `@Cron`(CronExpression.EVERY_5_SECONDS) async processQueueMatches() { - const lockKey = 12346; + const lockKey = LockKeys.CREATE_TEAM_REPOS; const queryRunner = this.dataSource.createQueryRunner();api/src/user/user.service.ts (1)
123-128: Consider reusing the existingEventService.isEventPublicmethod.The
isEventPubliccheck duplicates logic already available inEventService.isEventPublic(as shown in the relevant code snippets). This creates maintenance burden if the visibility logic changes.♻️ Suggested refactor to reuse existing service method
Inject
EventServiceand call the existing method:- const isEventPublic = await this.userRepository.manager - .getRepository(EventEntity) - .existsBy({ - id: eventId, - isPrivate: false, - }); + const isEventPublic = await this.eventService.isEventPublic(eventId);This requires adding
EventServiceto the constructor dependencies.frontend/app/events/EventTable.tsx (1)
83-87: Avoid callingformatStatetwice per event row.
formatState(event)is invoked twice for each event, computing the same result. This is inefficient, especially with the current console.log inside.♻️ Suggested improvement
events.map((event) => ( - <TableRow + const state = formatState(event); + return <TableRow key={event.id} className="cursor-pointer transition-colors hover:bg-muted/50" onClick={() => router.push(`/events/${event.id}`)} > <TableCell className="font-medium">{event.name}</TableCell> <TableCell> {new Date(event.startDate).toLocaleDateString()} </TableCell> <TableCell> {event.minTeamSize} -{event.maxTeamSize} members </TableCell> <TableCell> - <Badge variant={formatState(event).variant}> - {formatState(event).text} + <Badge variant={state.variant}> + {state.text} </Badge> </TableCell> - </TableRow> + </TableRow>; ))Or wrap
formatStatewithuseCallbackif it needs to be memoized at the component level.api/src/auth/auth.controller.ts (1)
32-34: Improve type safety for request user access.Using
(req as any).userbypasses TypeScript checking. Consider defining a typed request interface or using the existing@UserId()decorator consistently.♻️ Suggested approach
Define a typed request interface:
interface AuthenticatedRequest extends Request { user: { id: string; /* other fields */ }; }Then use it:
- githubCallback(`@Req`() req: Request, `@Res`() res: Response) { - const user: any = (req as any).user; + githubCallback(`@Req`() req: AuthenticatedRequest, `@Res`() res: Response) { + const user = req.user;Or for
/me, simply use the existing decorator:- me(`@Req`() req: Request) { - const user: any = (req as any).user; - return this.userService.getUserById(user.id); + me(`@UserId`() userId: string) { + return this.userService.getUserById(userId);Also applies to: 120-122
github-service/src/app.service.ts (1)
281-303: Consider handling partial failures when adding collaborators.Using
Promise.allmeans if one user fails to be added, the entire operation rejects immediately. Other users may be in an inconsistent state (some added, some not).Consider
Promise.allSettledwith error aggregation if partial success should be logged and handled gracefully, or ensure retries/cleanup logic exists upstream.♻️ Alternative using Promise.allSettled
- await Promise.all( - githubUsers.map(async (user) => { + const results = await Promise.allSettled( + githubUsers.map(async (user) => { const { username, githubAccessToken } = user; this.logger.log( `Adding user ${username} to repository ${name} in org ${githubOrg}`, ); await repositoryApi.addCollaborator( githubOrg, name, username, "push", ); const decryptedGithubAccessToken = this.decryptSecret(githubAccessToken); await userApi.acceptRepositoryInvitationByRepo( githubOrg, name, decryptedGithubAccessToken, ); }), ); + + const failures = results.filter((r) => r.status === "rejected"); + if (failures.length > 0) { + this.logger.warn( + `Failed to add ${failures.length} user(s) to repository ${name}`, + ); + }frontend/components/event-info-notice.tsx (1)
16-24: Stop the interval after the event starts.Right now the interval continues to tick even when the event has started, causing a re-render every second for a banner that no longer renders. Consider stopping the interval once
hasStartedflips true.♻️ Proposed fix
- useEffect(() => { - const id = setInterval(() => setNow(new Date()), 1000); - return () => clearInterval(id); - }, []); + useEffect(() => { + if (hasStarted) return; + const id = setInterval(() => setNow(new Date()), 1000); + return () => clearInterval(id); + }, [hasStarted]);
| const redirectUrl = this.configService.getOrThrow<string>( | ||
| "OAUTH_SUCCESS_REDIRECT_URL", | ||
| ); | ||
| if (redirectUrl) { | ||
| res.cookie("token", token, { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: "none", | ||
| maxAge: 30 * 24 * 60 * 60 * 1000, | ||
| }); | ||
| return res.redirect(redirectUrl); | ||
| } | ||
| return res.json({ token }); |
There was a problem hiding this comment.
Unnecessary conditional check after getOrThrow.
getOrThrow will throw an exception if the key is missing, so redirectUrl will always be truthy if execution reaches line 38. The if (redirectUrl) check is redundant and the else branch returning JSON is dead code.
🐛 Proposed fix
If you want to support both redirect and JSON response modes, use get() instead:
- const redirectUrl = this.configService.getOrThrow<string>(
+ const redirectUrl = this.configService.get<string>(
"OAUTH_SUCCESS_REDIRECT_URL",
);Or if redirect is always required, remove the conditional:
const redirectUrl = this.configService.getOrThrow<string>(
"OAUTH_SUCCESS_REDIRECT_URL",
);
- if (redirectUrl) {
res.cookie("token", token, {
httpOnly: true,
secure: true,
sameSite: "none",
maxAge: 30 * 24 * 60 * 60 * 1000,
});
return res.redirect(redirectUrl);
- }
- return res.json({ token });📝 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.
| const redirectUrl = this.configService.getOrThrow<string>( | |
| "OAUTH_SUCCESS_REDIRECT_URL", | |
| ); | |
| if (redirectUrl) { | |
| res.cookie("token", token, { | |
| httpOnly: true, | |
| secure: true, | |
| sameSite: "none", | |
| maxAge: 30 * 24 * 60 * 60 * 1000, | |
| }); | |
| return res.redirect(redirectUrl); | |
| } | |
| return res.json({ token }); | |
| const redirectUrl = this.configService.get<string>( | |
| "OAUTH_SUCCESS_REDIRECT_URL", | |
| ); | |
| if (redirectUrl) { | |
| res.cookie("token", token, { | |
| httpOnly: true, | |
| secure: true, | |
| sameSite: "none", | |
| maxAge: 30 * 24 * 60 * 60 * 1000, | |
| }); | |
| return res.redirect(redirectUrl); | |
| } | |
| return res.json({ token }); |
| const redirectUrl = this.configService.getOrThrow<string>( | |
| "OAUTH_SUCCESS_REDIRECT_URL", | |
| ); | |
| if (redirectUrl) { | |
| res.cookie("token", token, { | |
| httpOnly: true, | |
| secure: true, | |
| sameSite: "none", | |
| maxAge: 30 * 24 * 60 * 60 * 1000, | |
| }); | |
| return res.redirect(redirectUrl); | |
| } | |
| return res.json({ token }); | |
| const redirectUrl = this.configService.getOrThrow<string>( | |
| "OAUTH_SUCCESS_REDIRECT_URL", | |
| ); | |
| res.cookie("token", token, { | |
| httpOnly: true, | |
| secure: true, | |
| sameSite: "none", | |
| maxAge: 30 * 24 * 60 * 60 * 1000, | |
| }); | |
| return res.redirect(redirectUrl); |
🤖 Prompt for AI Agents
In `@api/src/auth/auth.controller.ts` around lines 35 - 47, The code in
AuthController where redirectUrl is assigned via
this.configService.getOrThrow<string>("OAUTH_SUCCESS_REDIRECT_URL") is checking
if (redirectUrl) even though getOrThrow will always return or throw, making the
conditional dead code; either remove the if and always set the cookie +
res.redirect(redirectUrl) (if redirect is mandatory), or change getOrThrow to
this.configService.get<string>("OAUTH_SUCCESS_REDIRECT_URL") and keep the if to
allow fallback to return res.json({ token }) when redirectUrl is undefined —
update the block in auth.controller.ts (the redirectUrl variable and surrounding
cookie/redirect/JSON logic) accordingly.
| throw new BadRequestException( | ||
| e && typeof e.message === "string" | ||
| ? `Invalid state parameter: ${e.message}` | ||
| : "Invalid state parameter.", | ||
| ); |
There was a problem hiding this comment.
Avoid leaking internal error details to clients.
Including e.message in the response could expose internal implementation details (e.g., database errors, stack traces) to clients, which is a security concern.
🔒 Proposed fix
throw new BadRequestException(
- e && typeof e.message === "string"
- ? `Invalid state parameter: ${e.message}`
- : "Invalid state parameter.",
+ "Invalid state parameter.",
);The detailed error is already logged on line 106 for debugging purposes.
📝 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.
| throw new BadRequestException( | |
| e && typeof e.message === "string" | |
| ? `Invalid state parameter: ${e.message}` | |
| : "Invalid state parameter.", | |
| ); | |
| throw new BadRequestException( | |
| "Invalid state parameter.", | |
| ); |
🤖 Prompt for AI Agents
In `@api/src/auth/auth.controller.ts` around lines 110 - 114, The thrown
BadRequestException in auth.controller.ts currently interpolates e.message and
may leak internal error details; change the throw to use a generic message
(e.g., "Invalid state parameter.") and remove inclusion of e.message while
leaving the existing debug log at the earlier line intact; target the
BadRequestException throw that references the variable e in the relevant method
of the AuthController to make this change.
| } else { | ||
| user = await this.users.createUser( | ||
| email || `${githubId}@users.noreply.github.com`, | ||
| username, | ||
| name, | ||
| profilePicture, | ||
| githubId, | ||
| accessToken, | ||
| false, | ||
| ); |
There was a problem hiding this comment.
Handle potentially undefined profilePicture for new users.
When creating a new user, profilePicture is passed directly from the profile which may be undefined. If the database column has a NOT NULL constraint or downstream code expects a string, this could cause issues.
🐛 Proposed fix
user = await this.users.createUser(
email || `${githubId}@users.noreply.github.com`,
username,
name,
- profilePicture,
+ profilePicture || "",
githubId,
accessToken,
false,
);🤖 Prompt for AI Agents
In `@api/src/auth/github.strategy.ts` around lines 51 - 60, The new-user creation
passes profilePicture from the GitHub profile which may be undefined; update the
call to users.createUser in github.strategy.ts (the block that constructs new
user via this.users.createUser) to provide a safe default (e.g., empty string or
a configured placeholder URL) using a nullish fallback for profilePicture so the
database always receives a string and downstream code won’t break.
| export const enum LockKeys { | ||
| AUTO_LOCK_EVENTS = 12345, | ||
| CREATE_TEAM_REPOS = 12346, | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for hardcoded lock key values that should use the LockKeys enum
rg -n "12345|12346" --type ts -g '!**/constants.ts'Repository: 42core-team/website
Length of output: 123
Replace hardcoded lock key with LockKeys enum value in match.service.ts.
The LockKeys enum provides a centralized constant for lock key values, but match.service.ts (line 52) still uses the hardcoded value 12346 instead of LockKeys.CREATE_TEAM_REPOS. Update this line to use the enum for consistency and maintainability:
const lockKey = LockKeys.CREATE_TEAM_REPOS;🤖 Prompt for AI Agents
In `@api/src/constants.ts` around lines 1 - 4, Replace the hardcoded lock key
literal 12346 in match.service.ts with the enum value
LockKeys.CREATE_TEAM_REPOS; locate where lockKey is assigned (the const lockKey
= ... usage) and change it to use LockKeys.CREATE_TEAM_REPOS, and ensure
LockKeys is imported from the constants module (add or update the import for
LockKeys if missing).
api/src/team/team.service.ts
Outdated
| return; | ||
| } | ||
|
|
||
| const repoName = team.event.name + "-" + team.name + "-" + team.id; |
There was a problem hiding this comment.
Sanitize repository name for GitHub compatibility.
GitHub repository names can only contain alphanumeric characters, hyphens, and underscores. If event.name or team.name contain spaces or special characters, the repository creation will fail.
🐛 Proposed fix
- const repoName = team.event.name + "-" + team.name + "-" + team.id;
+ const sanitize = (str: string) => str.replace(/[^a-zA-Z0-9_-]/g, "-").replace(/-+/g, "-");
+ const repoName = `${sanitize(team.event.name)}-${sanitize(team.name)}-${team.id}`;Alternatively, move the sanitization logic to a utility function for reuse.
📝 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.
| const repoName = team.event.name + "-" + team.name + "-" + team.id; | |
| const sanitize = (str: string) => str.replace(/[^a-zA-Z0-9_-]/g, "-").replace(/-+/g, "-"); | |
| const repoName = `${sanitize(team.event.name)}-${sanitize(team.name)}-${team.id}`; |
🤖 Prompt for AI Agents
In `@api/src/team/team.service.ts` at line 149, The repoName construction using
team.event.name + "-" + team.name + "-" + team.id must be sanitized for GitHub:
create (or call) a sanitizeRepoName utility and replace/normalize characters
from team.event.name and team.name (e.g., remove/replace non-alphanumeric
characters with hyphens or underscores, convert to lowercase, collapse
consecutive hyphens, trim leading/trailing hyphens, and enforce a sane length),
then build repoName = `${sanitize(part1)}-${sanitize(part2)}-${team.id}`; update
the code around the repoName assignment and export the sanitize function for
reuse.
| async createTeam(name: string, userId: string, eventId: string) { | ||
| return await this.dataSource.transaction(async (transaction) => { | ||
| const teamRepository = transaction.getRepository(TeamEntity); | ||
|
|
||
| const newTeam = await teamRepository.save({ | ||
| name, | ||
| event: { id: eventId }, | ||
| users: [{ id: userId }], | ||
| }); | ||
|
|
||
| if (await this.eventService.hasEventStarted(eventId)) | ||
| await this.createTeamRepository(newTeam.id); | ||
|
|
||
| return team; | ||
| return newTeam; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential issue with nested independent transactions.
createTeam wraps its logic in a transaction, then calls createTeamRepository which starts its own independent transaction. If the outer transaction fails after createTeamRepository succeeds, the team record rolls back but the repo creation message may have been emitted. Consider either:
- Passing the transaction manager to
createTeamRepository - Moving the
createTeamRepositorycall outside the transaction
🔒 Suggested approach: pass transaction manager
- async createTeamRepository(teamId: string) {
- return await this.dataSource.transaction(async (transaction) => {
+ async createTeamRepository(teamId: string, transaction?: EntityManager) {
+ const execute = async (manager: EntityManager) => {
- const teamRepository = transaction.getRepository(TeamEntity);
+ const teamRepository = manager.getRepository(TeamEntity);
// ... rest of method
+ };
+
+ if (transaction) {
+ return execute(transaction);
+ }
+ return this.dataSource.transaction(execute);
+ }Then in createTeam:
if (await this.eventService.hasEventStarted(eventId))
- await this.createTeamRepository(newTeam.id);
+ await this.createTeamRepository(newTeam.id, transaction);🤖 Prompt for AI Agents
In `@api/src/team/team.service.ts` around lines 175 - 190, The outer createTeam
wraps work in dataSource.transaction but then calls createTeamRepository which
starts its own independent transaction, risking inconsistent side-effects if the
outer transaction rolls back; update createTeamRepository to accept an optional
EntityManager (or TransactionalRepository) parameter and, inside createTeam,
pass the transaction manager (the value returned to the transaction callback)
when calling createTeamRepository (use eventService.hasEventStarted(eventId) as
before, then await createTeamRepository(newTeam.id, manager) so both DB
operations share the same transaction), or alternatively remove the
createTeamRepository call from inside the transaction and invoke it after the
transaction completes; adjust function signatures accordingly
(createTeamRepository(..., manager?: EntityManager)).
| text: string; | ||
| variant: "default" | "secondary" | "destructive"; | ||
| } => { | ||
| console.log("try to format state for event", event); |
There was a problem hiding this comment.
Remove debug console.log statement.
This debug statement will log for every event on every render, polluting the browser console in production.
🐛 Proposed fix
- console.log("try to format state for event", event);🤖 Prompt for AI Agents
In `@frontend/app/events/EventTable.tsx` at line 25, Remove the debug console.log
call that's printing every event in the EventTable component: delete the line
console.log("try to format state for event", event) (or replace it with a
development-only guard like if (process.env.NODE_ENV !== 'production') { /*
debug log */ }) so the EventTable rendering no longer spams the browser console;
locate the statement inside the EventTable component where the event is being
formatted and remove or conditionally guard it.
| if (event.currentRound === 0) { | ||
| return { | ||
| text: "In Progress", | ||
| variant: "secondary", | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| text: "Completed", | ||
| variant: "destructive", | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: 42core-team/website
Length of output: 847
🏁 Script executed:
rg "currentRound" -C 3 --type ts --type tsxRepository: 42core-team/website
Length of output: 90
🏁 Script executed:
rg "currentRound" -C 3 --type ts api/Repository: 42core-team/website
Length of output: 12376
🏁 Script executed:
# Also check the event-related files directly
find api -name "*event*" -type f | head -20Repository: 42core-team/website
Length of output: 467
🏁 Script executed:
cat -n frontend/app/events/EventTable.tsx | sed -n '25,55p'Repository: 42core-team/website
Length of output: 956
🏁 Script executed:
# Also search for the function containing this logic
rg "currentRound === 0" -B 10 -A 5 frontend/app/events/EventTable.tsxRepository: 42core-team/website
Length of output: 431
🏁 Script executed:
# Check for event state or completion indicators
rg "Completed|FINISHED" frontend/app/events/EventTable.tsx -B 3 -A 3Repository: 42core-team/website
Length of output: 149
🏁 Script executed:
# Check the Event entity for other completion/state fields
cat -n api/src/event/entities/event.entity.tsRepository: 42core-team/website
Length of output: 2908
Fix the event state logic to align with backend round semantics.
The frontend logic contradicts backend behavior. According to the backend, currentRound === 0 indicates that no rounds have started yet (see match.controller.ts and match.service.ts), yet the frontend displays this as "In Progress". Additionally, the logic assumes any currentRound > 0 means "Completed", but the backend only increments the round counter after a round finishes—the event could still be active if the current time hasn't reached endDate.
Update the state logic to:
- Check
currentRound === 0to represent the initial phase (not "In Progress") - Check against
endDateto determine if the event is actually completed - Properly represent in-progress rounds when
currentRound > 0and the event hasn't ended
🤖 Prompt for AI Agents
In `@frontend/app/events/EventTable.tsx` around lines 35 - 45, Replace the
existing status logic that checks only event.currentRound with logic that uses
both event.currentRound and event.endDate: treat event.currentRound === 0 as the
"Initial"/not-started state, treat the event as "Completed" only when new Date()
is after new Date(event.endDate), and treat event.currentRound > 0 with endDate
in the future as "In Progress"; update the return objects (text and variant)
accordingly where the current snippet lives (EventTable.tsx) using the
event.currentRound and event.endDate fields.
| const didRefreshRef = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| const id = setInterval(() => setNow(new Date()), 1000); | ||
| return () => clearInterval(id); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| const timeLeftMs = startsAt.getTime() - now.getTime(); | ||
| if (!didRefreshRef.current && timeLeftMs <= 0) { | ||
| didRefreshRef.current = true; | ||
| router.refresh(); | ||
| } | ||
| }, [now, startsAt, router]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/components/event-info-notice.tsxRepository: 42core-team/website
Length of output: 2619
Reset the refresh guard when startDate changes.
If this component remains mounted across navigation to a different event, didRefreshRef will retain its true state from the previous event, preventing the refresh when the new event's countdown reaches zero. Reset it when startDate changes.
Proposed fix
const didRefreshRef = useRef(false);
+ useEffect(() => {
+ didRefreshRef.current = false;
+ }, [startDate]);🤖 Prompt for AI Agents
In `@frontend/components/event-info-notice.tsx` around lines 19 - 32, The refresh
guard didRefreshRef is never reset when a new event (startsAt) is loaded, so add
logic to reset it whenever startsAt changes; specifically, add a small useEffect
that watches startsAt and sets didRefreshRef.current = false (referencing
didRefreshRef and startsAt) so the countdown can trigger router.refresh() for
the new event.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/match/match.service.ts (1)
139-140: Error message doesn't match the actual state being checked.The code checks for
MatchState.IN_PROGRESSbut the error message says "READY state". This mismatch will cause confusion during debugging.📝 Suggested fix
if (match.state !== MatchState.IN_PROGRESS) - throw new Error(`Match with id ${matchId} is not in READY state.`); + throw new Error(`Match with id ${matchId} is not in IN_PROGRESS state.`);
changed domain from coregame.de to coregame.sh
started refactoring everything to tanstack query
includes 390-migrate-authentication-to-the-backend
Summary by CodeRabbit
New Features
Infrastructure & Configuration
Removals
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.