feat: added basic functioning event templates for defining multiple b…#538
feat: added basic functioning event templates for defining multiple b…#538
Conversation
…asePaths and mycorebotdocker images
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds event starter templates end-to-end: DB migration and entity/DTOs, API endpoints and service logic for template CRUD, team creation integration using templates, match payloads selecting per-team images, frontend UI/actions for managing and selecting templates, and plumbing through GitHub/repo cloning utilities. Changes
Sequence DiagramsequenceDiagram
participant Admin as Event Admin
participant FE as Frontend
participant API as Event API
participant DB as Database
participant TeamSvc as Team Service
participant GHSvc as GitHub Service
participant RepoUtils as Repo Utils
participant Queue as Match Queue
rect rgba(100,150,200,0.5)
Admin->>FE: submit create starter template
FE->>API: POST /event/:id/templates
API->>DB: INSERT event_starter_templates
DB-->>API: created template
API-->>FE: created template
end
rect rgba(100,150,200,0.5)
FE->>API: POST /team (name, starterTemplateId)
API->>DB: SELECT template WHERE id=starterTemplateId
DB-->>API: template (basePath, myCoreBotDockerImage)
API->>TeamSvc: createTeam(..., starterTemplateId)
TeamSvc->>GHSvc: createTeamRepository(..., starterTemplateId)
GHSvc->>RepoUtils: cloneMonoRepo(..., starterTemplateId)
RepoUtils->>RepoUtils: updateConfigsUrls(..., starterTemplateId)
RepoUtils-->>GHSvc: repo ready
GHSvc-->>TeamSvc: repository created
TeamSvc->>DB: INSERT team with starterTemplateId
DB-->>TeamSvc: team created
TeamSvc-->>API: Team
API-->>FE: Team
end
rect rgba(100,150,200,0.5)
FE->>API: enqueue match
API->>DB: SELECT match with teams and starterTemplate
DB-->>API: match + team.starterTemplate
API->>API: image = team.starterTemplate?.myCoreBotDockerImage ?? event.myCoreBotDockerImage
API->>Queue: enqueue match with per-team image
Queue-->>API: ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/src/team/dtos/createTeamDto.ts (1)
8-11:⚠️ Potential issue | 🟡 MinorRegex allows 4 characters but the error message says "between 5 and 30".
The pattern
{4,30}permits a 4-character name, while the message states the minimum is 5. One of the two is wrong — align the regex with the intended rule.api/src/team/team.service.ts (1)
199-222:⚠️ Potential issue | 🟠 MajorNo validation that
starterTemplateIdbelongs to the given event.A user could pass a
starterTemplateIdfrom a different event. The FK constraint only ensures the template exists, not that it's associated witheventId. This could lead to a team using a mismatched basePath/docker image from another event's template.Add a check before saving:
🛡️ Proposed fix
async createTeam( name: string, userId: string, eventId: string, starterTemplateId?: string, ) { return await this.dataSource.transaction(async (entityManager) => { const teamRepository = entityManager.getRepository(TeamEntity); + if (starterTemplateId) { + const templateBelongsToEvent = await this.eventService.getStarterTemplates(eventId) + .then(templates => templates.some(t => t.id === starterTemplateId)); + if (!templateBelongsToEvent) { + throw new BadRequestException('Starter template does not belong to this event.'); + } + } + const newTeam = await teamRepository.save({ name, event: { id: eventId }, users: [{ id: userId }], starterTemplate: starterTemplateId ? { id: starterTemplateId } : undefined, });
🤖 Fix all issues with AI agents
In `@api/src/event/event.service.ts`:
- Around line 299-302: The update logic in event.service.ts currently uses falsy
checks (if (data.name), if (data.basePath), if (data.myCoreBotDockerImage))
which prevents clearing fields to an empty string; change those conditions to
explicit undefined checks (e.g., data.name !== undefined) so that provided empty
strings are applied to template.name, template.basePath, and
template.myCoreBotDockerImage while still ignoring truly omitted properties on
the data object.
In `@api/src/team/dtos/createTeamDto.ts`:
- Around line 14-17: The Swagger decorator on the CreateTeamDto property
starterTemplateId is inconsistent with its optional validation and lacks UUID
validation; replace `@ApiProperty`() with `@ApiPropertyOptional`() for
starterTemplateId, add the `@IsUUID`() decorator to the starterTemplateId
property, and update the class-validator import to include IsUUID so the DTO
both documents the field as optional in OpenAPI and enforces UUID format.
In `@api/src/team/team.controller.ts`:
- Around line 87-92: The CreateTeamDto currently marks starterTemplateId as
optional and string-only, allowing invalid template IDs to pass; update the DTO
by adding the `@IsUUID`() decorator to the starterTemplateId property (the
property named starterTemplateId on class CreateTeamDto) so validation enforces
a UUID format before calling teamService.createTeam; ensure the decorators order
includes `@IsUUID`() alongside `@IsOptional`() and `@IsString`() on that property.
In
`@frontend/app/events/`[id]/dashboard/components/StarterTemplatesManagement.tsx:
- Around line 80-93: The form currently validates via formSchema on submit but
never shows field-level errors to the user; update the component that renders
each input (those bound to useForm created by useForm) to read and render each
field's state.meta.errors (e.g., via form.field("name").state.meta.errors or the
equivalent accessor) directly under the corresponding input, add an
"invalid"/error styling when errors exist, and ensure the submit button shows
validation failure (e.g., disabled or shows error state) until errors are
cleared; keep the existing onSubmit (createMutation.mutateAsync and form.reset)
but surface field.state.meta.errors next to the "name", "basePath", and
"myCoreBotDockerImage" inputs so users see why submission failed.
In `@frontend/app/events/`[id]/dashboard/dashboard.tsx:
- Around line 1073-1085: The GitHub token input updates
pendingSettings.githubOrgSecret but that key is missing from the fields array
used by handleSaveSettings, so changes are never sent; add "githubOrgSecret" to
the fields array (the same array referenced when building the payload in
handleSaveSettings) so pendingSettings.githubOrgSecret is included in the update
payload and reaches the backend (which expects updateEventSettingsDto and
handles encryption in event.service.ts).
In `@frontend/app/events/`[id]/my-team/components/TeamCreationForm.tsx:
- Around line 41-48: The templates query can be still loading and default to []
causing the template selector to be hidden and validation to be bypassed; update
the useQuery call in TeamCreationForm to destructure isLoading (e.g., const {
data: templates = [], isLoading } = useQuery(...)) and use that flag to disable
the form/submit button (include isLoading in the disabled condition alongside
the existing selectedTemplateId and templates.length checks) so users cannot
submit until templates have finished loading.
- Around line 263-267: The button currently renders only Loader2 when
createTeamMutation.isPending, causing the label to disappear; update the JSX in
TeamCreationForm so that when createTeamMutation.isPending you render the
Loader2 icon followed by a text label (e.g., "Creating…" or the original "Create
My Team") instead of replacing it, keep the button in a disabled state while
pending, and ensure accessibility by preserving an appropriate
aria-label/aria-live text for the loading state.
In `@frontend/package.json`:
- Line 33: The package.json currently lists "@tanstack/zod-form-adapter":
"^0.42.1", which only supports zod@^3.x and is incompatible with our installed
"zod" (v4.x); fix by either upgrading "@tanstack/zod-form-adapter" to a release
that declares support for zod v4 (replace the version string with the compatible
release), or revert "zod" to a v3.x version in package.json, or remove/replace
the adapter dependency entirely and update any import/usages of
"@tanstack/zod-form-adapter" accordingly (search for the package name in code to
update call sites).
🧹 Nitpick comments (9)
frontend/package.json (2)
30-33: Two competing form libraries:react-hook-formand@tanstack/react-form.The project already uses
react-hook-form(line 50) with@hookform/resolvers(line 15). Adding@tanstack/react-formand@tanstack/zod-form-adapterintroduces a second form management library, increasing bundle size and creating inconsistency in form handling patterns across the codebase.Consider standardizing on one form library. If
@tanstack/react-formis preferred going forward, plan to migrate existingreact-hook-formusage; otherwise, implement the new forms withreact-hook-formfor consistency.
85-85:eslint-plugin-tailwindcssis pinned to a beta version.Version
4.0.0-beta.0is a pre-release. This is fine for development tooling, but be aware that beta APIs may change, potentially breaking lint runs on upgrades.api/src/event/dtos/createEventStarterTemplateDto.ts (1)
1-15: LGTM — consider adding@MaxLength()constraints.The DTO is clean and correct. Optionally, consider adding
@MaxLength()validators to prevent excessively long inputs, especially forbasePathandmyCoreBotDockerImage, which are used in Docker/filesystem contexts.frontend/app/events/[id]/my-team/components/TeamCreationForm.tsx (1)
88-111: Error handling relies on fragile string prefix matching.Line 104 checks
!error.message.startsWith("An unexpected")to decide whether to show the raw error message. This couples the conditional to a specific wording and is easy to break if the message text ever changes. Consider using a more structured approach (e.g., a custom error class or error code) rather than string-prefix matching.frontend/components/ui/field.tsx (2)
5-10: Consider droppingforwardRef— React 19 passesrefas a regular prop.Since the project uses React 19,
forwardRefis no longer necessary.refcan be accepted as a standard prop. The current code works, but simplifying would align with the new React 19 idiom and reduce boilerplate.Also applies to: 13-18, 21-30, 33-49, 52-57, 60-65, 68-73, 76-86
33-49:errorsprop typed asany[]— consider a narrower type.
any[]provides no compile-time safety. A type likestring[](orArray<string | undefined>) would better communicate the expected shape and prevent accidental misuse.Suggested change
- React.HTMLAttributes<HTMLParagraphElement> & { errors?: any[] } + React.HTMLAttributes<HTMLParagraphElement> & { errors?: (string | undefined)[] }frontend/app/events/[id]/dashboard/components/StarterTemplatesManagement.tsx (2)
281-329: Use JSX children instead of thechildrenprop forform.Field.Biome flags
childrenas a prop (lint/correctness/noChildrenProp). While TanStack Form supports both patterns, using JSX children is more idiomatic and avoids the lint warning.♻️ Example refactor for one field (apply to all three)
<form.Field name="name" - children={(field) => ( +> + {(field) => ( <div className="space-y-1"> <Input id={field.name} name={field.name} value={field.state.value} onBlur={field.handleBlur} onChange={(e) => field.handleChange(e.target.value)} placeholder="New Template Name..." className="h-8 bg-background" /> </div> )} - /> + </form.Field>
262-270: No delete confirmation — users can accidentally delete templates in use.Especially since the backend will reject deletion of templates referenced by teams (ConflictException), a confirmation dialog would improve UX and reduce unnecessary error toasts.
api/src/event/event.service.ts (1)
274-288:getEventByIdfetches and sanitizes the full event entity unnecessarily.
createStarterTemplatecallsthis.getEventById(eventId)which loads the entire event (including configs) and runssanitizeEventConfigson it. Since you only need to establish a relation, you can avoid the extra query and config mutation by using a reference object instead:Proposed refactor
- const event = await this.getEventById(eventId); const template = this.templateRepository.create({ name, basePath, myCoreBotDockerImage, - event, + event: { id: eventId } as EventEntity, });Note: this skips the existence check on the event. If you want to retain that validation, a lightweight
this.eventRepository.findOneByOrFail({ id: eventId })with aselect: { id: true }would suffice.
frontend/app/events/[id]/dashboard/components/StarterTemplatesManagement.tsx
Show resolved
Hide resolved
frontend/app/events/[id]/my-team/components/TeamCreationForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/events/`[id]/dashboard/dashboard.tsx:
- Around line 78-87: Wrap the DashboardPage component in a React.Suspense
boundary in its parent page component (the async Page function) because
DashboardPage uses useSearchParams(); import Suspense from react, render
<Suspense fallback={...}> around <DashboardPage eventId={eventId} /> and keep
eventId resolution from params as before so the client component is isolated
inside the suspense boundary; ensure the fallback is a minimal loading element
(e.g., "Loading dashboard...") and export the Page as async that awaits params
then returns the Suspense-wrapped DashboardPage.
🧹 Nitpick comments (1)
frontend/app/events/[id]/dashboard/dashboard.tsx (1)
81-87: URL-synced tabs are a nice UX improvement for deep linking.Minor edge case: if the URL contains an invalid
tabvalue (e.g.,?tab=foo), noTabsContentwill match and the panel will be blank. Consider validating against the known tab values:Suggested guard
+ const VALID_TABS = ["overview", "operation", "settings", "admins"] as const; + const tabParam = searchParams.get("tab"); - const currentTab = searchParams.get("tab") || "overview"; + const currentTab = tabParam && VALID_TABS.includes(tabParam as any) ? tabParam : "overview";Also applies to: 398-408
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
github-service/src/app.service.ts (1)
341-356:⚠️ Potential issue | 🟠 MajorPre-existing:
repository_createdemitted after failed repo setup.Not introduced by this PR, but worth noting: when the inner
try(Line 302) fails, thecatchat Line 341 deletes the repository but does not re-throw. Execution then falls through to Line 353 and emits"repository_created"and attempts to add collaborators to a deleted/nonexistent repo. Consider re-throwing after cleanup to let the outer catch handle it.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; } finally {api/src/team/team.service.ts (1)
200-222:⚠️ Potential issue | 🟠 MajorNo validation that
starterTemplateIdbelongs to the given event.A caller can pass a
starterTemplateIdfrom a different event. TheteamRepository.savewill succeed as long as the template UUID exists (the FK references the template table, not the event+template pair). This creates a cross-event data integrity issue.Validate the template belongs to the event before persisting:
Proposed fix
async createTeam( name: string, userId: string, eventId: string, starterTemplateId?: string, ) { return await this.dataSource.transaction(async (entityManager) => { const teamRepository = entityManager.getRepository(TeamEntity); + if (starterTemplateId) { + const templateExists = await entityManager.exists( + EventStarterTemplateEntity, + { where: { id: starterTemplateId, event: { id: eventId } } }, + ); + if (!templateExists) { + throw new BadRequestException( + "Starter template not found for this event.", + ); + } + } + const newTeam = await teamRepository.save({ name, event: { id: eventId }, users: [{ id: userId }], starterTemplate: starterTemplateId ? { id: starterTemplateId } : undefined, });
🧹 Nitpick comments (4)
frontend/app/events/[id]/dashboard/page.tsx (1)
17-21: Consider a more polished loading fallback.The Suspense boundary is a good pattern here for the Next.js App Router client component boundary. However, the plain
<div>Loading dashboard...</div>is quite bare compared to what users likely see elsewhere in the app. Consider using a spinner or skeleton component consistent with the rest of the UI for a better user experience.api/src/event/event.service.ts (1)
198-223: Two separate queries where one join would suffice.
getTemplateVersionissues two independent DB round-trips — one for the event and one for the template. Consider using a single query with a join (or querying the template with the event relation) to reduce latency.frontend/app/events/[id]/dashboard/components/StarterTemplatesManagement.tsx (1)
289-293: Use JSX children instead ofchildrenprop to satisfy Biome lint.Biome flags
childrenas a prop (lint/correctness/noChildrenProp). TanStack Form supports both patterns — switching to JSX children silences the lint without changing behavior:Example for one field (apply same pattern to all)
<form.Field name="name" - children={(field) => ( +> + {(field) => ( <div className="space-y-1"> ... </div> )} - /> + </form.Field>Also applies to: 322-326, 355-359, 388-392
frontend/app/events/[id]/dashboard/dashboard.tsx (1)
150-158: Starter templates query is duplicated withStarterTemplatesManagement.Both this component and
StarterTemplatesManagementdeclare the sameuseQuerywith key["event", eventId, "templates"]. React Query deduplicates the network request, so this works correctly. However, if the query logic changes in the future, it'll need updating in two places. Consider extracting a shared custom hook (e.g.,useStarterTemplates(eventId)) to keep the query definition DRY.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
github-service/src/repo.utils.ts (1)
55-59:⚠️ Potential issue | 🔴 CriticalBug:
starterTemplateIdis accepted but never forwarded toupdateConfigsUrls.
cloneMonoReporeceivesstarterTemplateId(line 26) but the call toupdateConfigsUrlsomits it, so the parameter will always beundefinedinsideupdateConfigsUrlsand the template-specific event URL will never be used.🐛 Proposed fix
this.updateConfigsUrls( path.join(tempFolderPath, basePath), eventId, apiBaseUrl, + starterTemplateId, ),
🤖 Fix all issues with AI agents
In `@github-service/src/repo.utils.ts`:
- Line 333: The code currently hardcodes "my-core-bot" when building mainCPath,
causing team-name replacement to be skipped for other templates; update
updateTeamName() to use the extracted folder name instead: either add a
folderName parameter to updateTeamName() and use that to build mainCPath
(replace the hardcoded "my-core-bot"), or call extractFolderNameFromRepo()
inside updateTeamName() to compute folderName the same way README handling does
(see the README path usage at lines ~255–256) and then construct mainCPath using
path.join(repoRoot, folderName, "src", "main.c"); ensure the change is applied
consistently wherever mainCPath is used so replacements run for all templates.
In `@k8s-service/internal/kube/game.go`:
- Around line 108-125: The current code injects bot.RepoURL via fmt.Sprintf into
an sh -c string (in game.go) which allows shell-injection if the URL contains a
single quote; stop interpolating bot.RepoURL directly—set the repo URL as a
container environment variable (e.g., REPO_URL from bot.RepoURL) in the Pod spec
and then reference it inside the shell script as "$REPO_URL" (or assign once to
a local shell var) so the fmt.Sprintf call only injects safe static values,
remove the duplicate %s placeholder and replace both uses with the env var
reference; additionally, add a simple validation/sanitization step on
bot.RepoURL before setting the env var (ensure it matches an expected URL
pattern) to harden against malformed inputs.
… `repo.utils.ts`.
PaulicStudios
deleted the
537-ability-to-add-different-repo-templates
branch
2 participants
…asePaths and mycorebotdocker images
Summary by CodeRabbit
New Features
Improvements
Chores