[#549] Add configurable challenge groups for project judging - #550
Conversation
farisAwheel
added
Feature
af7c8fa to
53d5730
Compare
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds organizer-managed judging groups with hierarchical challenge membership, import matching, schedule locks, and parent-aware judging behavior. Updates project imports, room and rubric editing, project filters, evaluation badges, and guest access. Adds database migration, API mutations, validation schemas, UI controls, tests, and feature documentation. Priority: ➖ Normal Merge Risk: 🔵 Low · up to Challenge groups now support hierarchical judging, preserved imports, schedule locks, and visible child tags. The remaining risk is bounded to regression coverage and filter metadata handling, which could cause incorrect challenge filtering or less-clear locked-state behavior in edge cases but does not establish a broad production failure. 🚥 Pre-merge checks | ✅ 5 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 3
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/projects-import.server.ts (1)
205-210: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle group-label collisions during import.
When an imported label matches a group label,
retainedLabelsexcludes the group andchallengeLabelsToCreateinserts a non-groupProjectChallengewith the same label. The database constraint allows this because it includesisGroup, andchallengeIdsthen maps the imported label to the duplicate non-group row. Exclude group labels before insertion or reuse the existing group row.
🟡 Other comments (5)
apps/blade/src/app/_components/judging/evaluation-dialog.tsx-327-335 (1)
327-335: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not hardcode white text over an organizer-chosen color.
tagColorcomes from an unconstrained<input type="color">in the challenge configuration panel. If an organizer picks a light color,color: "white"makes the challenge label unreadable. The badge is the only place the child challenge label appears in this dialog.Pick the foreground from the color's relative luminance.
Proposed fix
+function readableForeground(hex: string) { + const value = hex.replace("#", ""); + if (value.length !== 6) return "white"; + const [r, g, b] = [0, 2, 4].map((offset) => { + const channel = Number.parseInt(value.slice(offset, offset + 2), 16) / 255; + return channel <= 0.03928 + ? channel / 12.92 + : ((channel + 0.055) / 1.055) ** 2.4; + }) as [number, number, number]; + return 0.2126 * r + 0.7152 * g + 0.0722 * b > 0.4 ? "black" : "white"; +}style={ challenge.tagColor ? { backgroundColor: challenge.tagColor, borderColor: challenge.tagColor, - color: "white", + color: readableForeground(challenge.tagColor), } : undefined }As per path instructions for
apps/blade/**: "Accessibility (alt text, ARIA, semantic HTML)".Source: Path instructions
packages/api/src/routers/projects.ts-837-841 (1)
837-841: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate ids in
input.challengeIdstrigger the challenge-setup lock spuriously.
projectUpdateInputSchema.challengeIdsdoes not enforce uniqueness, and line 804 compares againstnew Set(input.challengeIds).size, so duplicates reach this point.explicitIdsholds distinct membership rows, so the length comparison fails andassertChallengeSetupEditablethrows "Challenge setup is locked" for a save that changes nothing. Compare sets instead.🐛 Proposed fix
- if ( - explicitIds.length !== input.challengeIds.length || - explicitIds.some((id) => !input.challengeIds.includes(id)) - ) + const selectedIds = new Set(input.challengeIds); + if ( + explicitIds.length !== selectedIds.size || + explicitIds.some((id) => !selectedIds.has(id)) + ) await assertChallengeSetupEditable(tx, existing.hackathonId);apps/blade/src/app/_components/judging/judging-configuration-panel.tsx-61-61 (1)
61-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the lock reason text for the saved schedule.
When
data.setupLockedis true anddata.configuration.stateis"draft", an existingJudgingSchedulelocks the rubric. Tell the officer to drop the schedule. Keep the current message for non-draft states.Proposed fix
- const rubricLocked = data.setupLocked || data.configuration.state !== "draft"; + const scheduleLocked = data.setupLocked; + const rubricLocked = scheduleLocked || data.configuration.state !== "draft";{rubricLocked ? ( <Alert> <AlertTitle>Rubric locked</AlertTitle> <AlertDescription> - The rubric cannot change after judging opens. Close and reopen - judging without changing the questions. + {scheduleLocked + ? "A saved schedule locks the rubric. Drop the schedule to change the questions." + : "The rubric cannot change after judging opens. Close and reopen judging without changing the questions."} </AlertDescription> </Alert> ) : null}apps/blade/src/app/_components/judging/judging-control-panel.tsx-727-727 (1)
727-727: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExplain the room lock when
data.setupLockedis true.A saved schedule sets
data.setupLockedand disables room creation, saving, reordering, editing, and archiving. The schedule controls are on a separate tab, so officers can see disabled room controls without a cause or remediation. Render a lock alert in this panel and state that officers must drop the eligible schedule before changing rooms.Proposed fix
+ {data.setupLocked ? ( + <Alert> + <QrCode className="size-4" /> + <AlertTitle>Room setup locked</AlertTitle> + <AlertDescription> + A saved schedule locks room changes. Open the Schedule tab and drop + the schedule while it is eligible before changing rooms. + </AlertDescription> + </Alert> + ) : null} {!data.challenges.length ? (packages/api/src/routers/project-challenges.ts-159-159 (1)
159-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd specific messages to all three
NOT_FOUNDerrors.Stale group requests, or a challenge deleted between validation and update, can reach these branches.
ChallengeConfigurationPanel.changedisplayserror.messagedirectly, so organizers can see only tRPC's genericNOT_FOUNDmessage. Use entity-specific messages for the two groups and the challenge.
🧹 Nitpick comments (3)
packages/validators/src/audit.ts (1)
535-542: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeclare change fields for the judging update actions.
validateActionPayloadrejectschangesfields that are not declared in each policy. Add change fields for the group properties and challengeparentIdandisScheduled. Update both emitters to provide the corresponding before and after values.apps/blade/src/tests/projects/challenge-configuration-panel.test.tsx (1)
165-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the locked state of existing group names.
When
data.challengeSetupLockedis true, the group-nameInputreceivesdisabled={locked || pending}. Addexpect(screen.getByRole("textbox", { name: "Group name: General" })).toBeDisabled()so this behavior cannot regress unnoticed.packages/validators/src/projects.ts (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit
tagColorregex message to both group schemas.When malformed
tagColorreaches either mutation,ChallengeConfigurationPaneldisplays Zod's generic regex error in a toast. Use the same message in both.regex(...)calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: QUIET
Plan: Advanced
Run ID: a41b4834-ebba-45a8-bd6f-a417868af412
📒 Files selected for processing (56)
.forge/features/collapsed-challenges/spec.md.forge/features/collapsed-challenges/srd.md.forge/features/collapsed-challenges/status.md.forge/features/collapsed-challenges/test-cases.mdapps/blade/src/app/_components/judging/challenge-configuration-panel.tsxapps/blade/src/app/_components/judging/evaluation-dialog.tsxapps/blade/src/app/_components/judging/judge-submissions.tsxapps/blade/src/app/_components/judging/judging-configuration-panel.tsxapps/blade/src/app/_components/judging/judging-control-panel.tsxapps/blade/src/app/_components/judging/judging-schedule-panel.tsxapps/blade/src/app/_components/projects/admin-project-workspace.tsxapps/blade/src/app/_components/projects/drop-all-projects-dialog.tsxapps/blade/src/app/_components/projects/judge-project-workspace.tsxapps/blade/src/app/_components/projects/project-detail-dialog.tsxapps/blade/src/app/_components/projects/project-directory.tsxapps/blade/src/app/_components/projects/project-import-dialog.tsxapps/blade/src/app/judge/projects/page.tsxapps/blade/src/tests/projects/challenge-configuration-panel.test.tsxapps/blade/src/tests/projects/judge-deliberation.test.tsxapps/blade/src/tests/projects/judging-announcement-editor.test.tsxapps/blade/src/tests/projects/project-judge-privacy.test.tsxdocs/DATABASE-USAGE.mdpackages/api/src/projects-import.server.tspackages/api/src/routers/hackathon.tspackages/api/src/routers/judging-schedule-view.tspackages/api/src/routers/judging-scores.tspackages/api/src/routers/judging.tspackages/api/src/routers/project-challenges.tspackages/api/src/routers/projects.tspackages/api/src/tests/integration/judging-access.test.tspackages/api/src/tests/integration/judging-schedule.test.tspackages/api/src/tests/integration/project-room-filter.test.tspackages/api/src/tests/projects/devpost-import.test.tspackages/api/src/tests/projects/drop-all.test.tspackages/api/src/utils/audit/coverage.tspackages/api/src/utils/judging-schedule/appointments.tspackages/api/src/utils/judging-schedule/evaluation-access.tspackages/api/src/utils/judging-schedule/source.tspackages/api/src/utils/judging/scope.tspackages/api/src/utils/projects/challenge-configuration.tspackages/api/src/utils/projects/challenge-labels.tspackages/api/src/utils/projects/devpost-import.tspackages/api/src/utils/projects/initialize-judging-groups.tspackages/db/drizzle/0053_naive_wasp.sqlpackages/db/drizzle/0054_giant_black_cat.sqlpackages/db/drizzle/0055_initialize_judging_groups.sqlpackages/db/drizzle/0056_premium_mesmero.sqlpackages/db/drizzle/meta/0053_snapshot.jsonpackages/db/drizzle/meta/0054_snapshot.jsonpackages/db/drizzle/meta/0055_snapshot.jsonpackages/db/drizzle/meta/0056_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schemas/knight-hacks.tspackages/db/src/tests/projects-schema.test.tspackages/validators/src/audit.tspackages/validators/src/projects.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
|
Addressed the review findings:
Two findings retain the existing behavior with added regression coverage:
Also reproduced and fixed the failed CI run's API contract snapshot: only the four new group/challenge procedures were missing. Full-suite validation exposed a separate test timeout caused by importing the shared API barrel inside the webhook test's timer; that import now runs during collection with all assertions preserved. Validation: |
Co-authored-by: Codex <codex@openai.com>
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
apps/blade/src/app/_components/judging/judging-control-panel.tsx-303-306 (1)
303-306: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard
RoomEditor.submitbeforecreateBuilding.If
data.setupLockedbecomes true while the dialog is open,submitcan create a globalJudgingBuildingbeforecreateRoomrejects the locked setup. Return immediately whendata.setupLockedis true.packages/api/src/tests/integration/project-room-filter.test.ts-506-514 (1)
506-514: 🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟡 Minor | ⚡ Quick winAssert
FORBIDDENfor both guest authorization failures.
rejects.toThrow()accepts any error. Match the authorization code so unrelated failures cannot satisfy these checks.💚 Proposed fix
await expect( guest.projects.listJudge({ ...guestListInput, challengeIds: [mlhOptInId], }), - ).rejects.toThrow(); + ).rejects.toMatchObject({ code: "FORBIDDEN" }); await expect( guest.projects.listJudge({ ...guestListInput, challengeIds: [mlhId] }), - ).rejects.toThrow(); + ).rejects.toMatchObject({ code: "FORBIDDEN" });apps/blade/src/app/_components/judging/evaluation-dialog.tsx-316-319 (1)
316-319: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive the labelled container an ARIA role.
A plain
divcannot usearia-labelas its accessible name. Addrole="group"so assistive technology can expose “Challenge opt-ins” before its child labels.♿ Proposed fix
<div className="mt-2 flex max-h-28 flex-wrap gap-2 overflow-y-auto" + role="group" aria-label="Challenge opt-ins" >
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: QUIET
Plan: Advanced
Run ID: 4adeb0fd-926f-4b8c-a708-63c674ed87cf
⛔ Files ignored due to path filters (7)
.forge/features/collapsed-challenges/screenshots/add-only-import.jpgis excluded by!**/*.jpg.forge/features/collapsed-challenges/screenshots/challenge-groups.jpgis excluded by!**/*.jpg.forge/features/collapsed-challenges/screenshots/imported-challenges.jpgis excluded by!**/*.jpg.forge/features/collapsed-challenges/screenshots/project-detail.jpgis excluded by!**/*.jpg.forge/features/collapsed-challenges/screenshots/project-tags.jpgis excluded by!**/*.jpg.forge/features/collapsed-challenges/screenshots/room-setup-lock.jpgis excluded by!**/*.jpgpackages/api/src/tests/root/__snapshots__/api-surface.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (29)
.forge/features/collapsed-challenges/spec.md.forge/features/collapsed-challenges/srd.md.forge/features/collapsed-challenges/status.md.forge/features/collapsed-challenges/test-cases.mdapps/blade/src/app/_components/judging/evaluation-dialog.tsxapps/blade/src/app/_components/judging/judging-configuration-panel.tsxapps/blade/src/app/_components/judging/judging-control-panel.tsxapps/blade/src/app/_components/projects/admin-project-workspace.tsxapps/blade/src/app/_components/projects/challenge-tag-style.tsapps/blade/src/app/_components/projects/project-detail-dialog.tsxapps/blade/src/app/_components/projects/project-directory.tsxapps/blade/src/app/_components/projects/project-import-dialog.tsxapps/blade/src/tests/member/member-dues-webhook.test.tsapps/blade/src/tests/projects/challenge-configuration-panel.test.tsxapps/blade/src/tests/projects/judging-announcement-editor.test.tsxapps/blade/src/tests/projects/project-import-dialog.test.tsxapps/blade/src/tests/projects/project-judge-privacy.test.tsxdocs/DATABASE-USAGE.mdpackages/api/src/projects-import.server.tspackages/api/src/routers/project-challenges.tspackages/api/src/routers/projects.tspackages/api/src/tests/integration/project-room-filter.test.tspackages/db/drizzle/0053_challenge_groups.sqlpackages/db/drizzle/meta/0053_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/tests/migration-lineage.test.tspackages/validators/src/audit.tspackages/validators/src/projects.tspackages/validators/src/tests/projects.test.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/blade/src/app/_components/projects/project-directory.tsx (1)
48-49: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
isGeneralandparentIdrequired on this contract.Both fields are optional. Line 129 excludes general groups from the filter list only when
isGeneralistrue, so any caller that omits the field silently reintroduces all-project groups into the challenge filter. The API challenge shape supplies both fields; drop the?so a caller cannot skip them.♻️ Proposed change
challenges: { id: string; label: string; - isGeneral?: boolean; - parentId?: string | null; + isGeneral: boolean; + parentId: string | null; }[];
🟡 Other comments (1)
.forge/features/collapsed-challenges/test-cases.md-20-20 (1)
20-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign behavioral case 6 with this regression requirement.
Case 6 says search excludes child challenges. Line 20 requires child challenges to remain selectable in filters. Change case 6 to include child challenges and exclude only every-project groups.
🧹 Nitpick comments (1)
packages/api/src/tests/integration/project-room-filter.test.ts (1)
735-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the filtered audit events are not empty.
If the filter matches zero events, the loop body never runs and the test passes without checking anything. A regression that stops recording
judging.group.createdorjudging.group.updatedwould not fail this test. Add a length assertion before the loop.🧪 Proposed fix
- for (const event of auditEvents.filter( - (event) => - event.actionKey === "judging.group.created" || - event.actionKey === "judging.group.updated", - )) { + const groupAuditEvents = auditEvents.filter( + (event) => + event.actionKey === "judging.group.created" || + event.actionKey === "judging.group.updated", + ); + expect(groupAuditEvents.length).toBeGreaterThan(0); + for (const event of groupAuditEvents) { expect(event.metadata).not.toHaveProperty("tagColor"); expect(event.changes.some((change) => change.field === "tagColor")).toBe( false, ); }As per path instructions for
**/*.test.*: "Check for meaningful descriptions, proper assertions, and no skipped tests without explanation."Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: QUIET
Plan: Advanced
Run ID: ee86e1d7-2f9e-43ef-854e-12e65a60147c
⛔ Files ignored due to path filters (1)
.forge/features/collapsed-challenges/screenshots/room-editor.jpgis excluded by!**/*.jpg
📒 Files selected for processing (27)
.forge/features/collapsed-challenges/spec.md.forge/features/collapsed-challenges/srd.md.forge/features/collapsed-challenges/status.md.forge/features/collapsed-challenges/test-cases.mdapps/blade/src/app/_components/judging/challenge-configuration-panel.tsxapps/blade/src/app/_components/judging/evaluation-dialog.tsxapps/blade/src/app/_components/judging/judging-control-panel.tsxapps/blade/src/app/_components/projects/project-detail-dialog.tsxapps/blade/src/app/_components/projects/project-directory.tsxapps/blade/src/tests/projects/challenge-configuration-panel.test.tsxapps/blade/src/tests/projects/judging-announcement-editor.test.tsxapps/blade/src/tests/projects/project-judge-privacy.test.tsxpackages/api/src/routers/judging-schedule-view.tspackages/api/src/routers/judging-scores.tspackages/api/src/routers/project-challenges.tspackages/api/src/routers/projects.tspackages/api/src/tests/integration/project-room-filter.test.tspackages/api/src/utils/judging-schedule/evaluation-access.tspackages/api/src/utils/projects/challenge-configuration.tspackages/api/src/utils/projects/challenge-labels.tspackages/db/drizzle/0053_challenge_groups.sqlpackages/db/drizzle/meta/0053_snapshot.jsonpackages/db/src/schemas/knight-hacks.tspackages/db/src/tests/event-management-migration.test.tspackages/validators/src/audit.tspackages/validators/src/projects.tspackages/validators/src/tests/projects.test.ts
💤 Files with no reviewable changes (6)
- packages/validators/src/projects.ts
- packages/api/src/utils/projects/challenge-labels.ts
- packages/api/src/utils/projects/challenge-configuration.ts
- packages/db/src/schemas/knight-hacks.ts
- packages/db/drizzle/0053_challenge_groups.sql
- packages/api/src/routers/projects.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
3 participants
Why
First Time Hacker projects are judged with General, while their eligibility must remain obvious to General judges. MLH tracks use the same room model. Both need organizer-controlled challenge groups.
What
Closes #549.
Prize challenges continue to come from Devpost. No shared or production database was migrated by this PR.
Test Plan
Screenshots
Fresh captures from the current Blade build:
Group controls
Imported challenge assignments
Project tags
Room editor
Add-only import after scheduling (old-screenshot)
Room setup lock (old-screenshot)
Projects search filter
Judging modal
for the judging modal, it filters based on what your search filter is, this works because if you're judging for general, you don't need to worry about mlh, and if mlh does use our system, they only see their challenges under their filter too!