Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMakes ChangesPhone Number Required & MLH/DEV Consent Changes
SCHOOLS Constant Updates
Responsive Combo Box Search Scoring
Sequence DiagramsequenceDiagram
participant User
participant HackerForm as HackerFormPage
participant Hydration
participant API
participant DB
User->>HackerForm: load application form
HackerForm->>Hydration: mount & detect hydration
Hydration-->>HackerForm: set hasHydrated=true after mount
User->>HackerForm: fill phone (10 digits or XXX-XXX-XXXX)
User->>HackerForm: uncheck MLH+DEV consent (now optional)
HackerForm->>API: submit form (phoneNumber="", agreesToReceiveEmailsFromMLH=false)
API->>DB: createHacker/updateHacker with "" instead of null
DB-->>API: hacker record saved
API-->>HackerForm: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 1
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/routers/hackers/mutations.ts (1)
31-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce phone validation in the API instead of persisting empty strings
The API currently allows
""and explicitly stores it (Lines 127 and 210). WithNOT NULL, this still permits “missing” phone numbers and bypasses required-phone enforcement outside this one form client.Suggested server-side hardening
+const phoneNumberSchema = z + .string() + .min(1, "Phone number is required") + .regex(/^\d{10}$|^\d{3}-\d{3}-\d{4}$/, "Invalid phone number"); createHacker: protectedProcedure .input( z.object({ ...InsertHackerSchema.omit({ userId: true, age: true, discordUser: true, }).shape, + phoneNumber: phoneNumberSchema, hackathonName: z.string(), }), ) ... - phoneNumber: - hackerData.phoneNumber === "" ? "" : hackerData.phoneNumber, + phoneNumber: hackerData.phoneNumber, updateHacker: protectedProcedure .input( - InsertHackerSchema.omit({ + InsertHackerSchema.omit({ userId: true, age: true, discordUser: true, - }), + }).extend({ + phoneNumber: phoneNumberSchema, + }), ) - const normalizedPhone = phoneNumber === "" ? "" : phoneNumber; + const normalizedPhone = phoneNumber;As per coding guidelines,
packages/api/**requires robust Zod input validation across procedures; relying on frontend-only validation breaks that contract.Also applies to: 126-127, 182-188, 210-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/routers/hackers/mutations.ts` around lines 31 - 39, The input schema for this mutation is allowing empty string values for the phone field, which should be rejected at the API level. Add explicit Zod validation for the phone field in the input schema to enforce that it must be a non-empty string, using something like z.string().min(1) instead of allowing empty strings. This applies to all similar mutation procedures in the hackers router that accept phone input to ensure server-side validation enforces the phone requirement consistently across all API endpoints.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/drizzle/0009_smooth_forgotten_one.sql`:
- Line 1: The migration in the drizzle SQL file attempts to set a NOT NULL
constraint on the "phone_number" column in the "knight_hacks_hacker" table, but
this will fail if existing NULL values are present in that column. Add a data
cleanup step before the ALTER TABLE statement that either backfills NULL values
with a sensible default (such as an empty string or a placeholder value) or
deletes rows with NULL phone_number values, depending on your business
requirements. This ensures the migration is deterministic and will not fail
during deployment on databases that contain legacy NULL values in that column.
---
Outside diff comments:
In `@packages/api/src/routers/hackers/mutations.ts`:
- Around line 31-39: The input schema for this mutation is allowing empty string
values for the phone field, which should be rejected at the API level. Add
explicit Zod validation for the phone field in the input schema to enforce that
it must be a non-empty string, using something like z.string().min(1) instead of
allowing empty strings. This applies to all similar mutation procedures in the
hackers router that accept phone input to ensure server-side validation enforces
the phone requirement consistently across all API endpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f023ddd7-b4e8-41aa-9a57-55d5db139a59
📒 Files selected for processing (7)
apps/blade/src/app/_components/dashboard/hacker/hacker-application-form.tsxpackages/api/src/routers/hackers/mutations.tspackages/consts/src/forms/schools.tspackages/db/drizzle/0009_smooth_forgotten_one.sqlpackages/db/drizzle/meta/0009_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schemas/knight-hacks.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/blade/src/app/_components/settings/hacker-profile-form.tsx (1)
102-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep hacker profile edits aligned with the required phone contract.
This form updates the hacker application row via
updateHacker, but it still accepts""and labels the phone number optional. That lets existing/edited hacker applications keep a blank phone number despite this PR making phone required.🐛 Proposed fix
phoneNumber: z .string() - .regex(/^(\d{10}|\d{3}-\d{3}-\d{4})?$/, "Invalid phone number"), + .min(1, "Required") + .regex(/^\d{10}$|^\d{3}-\d{3}-\d{4}$/, "Invalid phone number"),<FormLabel> - Phone Number - <span className="text-gray-400"> - {" "} - , <i>Optional</i> - </span> + Phone Number <span className="text-destructive">*</span> </FormLabel>Also applies to: 228-228, 358-371
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blade/src/app/_components/settings/hacker-profile-form.tsx` around lines 102 - 104, The phoneNumber field in the hacker profile form currently allows empty strings due to the optional pattern in the regex, which conflicts with the requirement to make phone mandatory. Update the phoneNumber validation in the schema to enforce phone as required: remove the optional pattern markers from the regex (the trailing and internal `?` characters) and ensure the field does not accept empty strings, so that edited hacker applications must include a valid phone number in either the ten-digit or XXX-XXX-XXXX format.apps/blade/src/app/_components/dashboard/hacker/hacker-application-form.tsx (1)
2106-2119:⚠️ Potential issue | 🟡 MinorFix duplicate "administration" in MLH consent wording.
Line 2108–2109 contains redundant text: "event administration, ranking, and administration." Since the parenthetical describes "creation of linked accounts," use "account creation" as the third item for clarity and conciseness.
📝 Suggested fix
- Major League Hacking for event administration, - ranking, and administration (including the + Major League Hacking for event administration, + ranking, and account creation (including the🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/blade/src/app/_components/dashboard/hacker/hacker-application-form.tsx` around lines 2106 - 2119, The MLH consent wording in the hacker-application-form.tsx file contains a duplicate word "administration" in the phrase "event administration, ranking, and administration." Replace the second instance of "administration" with "account creation" to make the text read "event administration, ranking, and account creation" since the parenthetical immediately following describes the creation of linked accounts on MLH and DEV platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/blade/src/app/_components/dashboard/hacker/hacker-application-form.tsx`:
- Around line 1031-1034: The navigation button disabled states in the
hacker-application-form component are currently conditioned on hasHydrated being
true, which means the buttons render enabled before hydration completes,
allowing clicks that get dropped by the goToStep guard. Fix the
backButtonDisabled and forwardButtonDisabled variable assignments by inverting
the hydration check: change hasHydrated to !hasHydrated and replace the AND
operator with an OR operator so that buttons start disabled until hydration
completes. For backButtonDisabled, the condition should be !hasHydrated OR
(activeStep === 0 || navigationLocked), and for forwardButtonDisabled, it should
be !hasHydrated OR navigationLocked.
---
Outside diff comments:
In `@apps/blade/src/app/_components/dashboard/hacker/hacker-application-form.tsx`:
- Around line 2106-2119: The MLH consent wording in the
hacker-application-form.tsx file contains a duplicate word "administration" in
the phrase "event administration, ranking, and administration." Replace the
second instance of "administration" with "account creation" to make the text
read "event administration, ranking, and account creation" since the
parenthetical immediately following describes the creation of linked accounts on
MLH and DEV platforms.
In `@apps/blade/src/app/_components/settings/hacker-profile-form.tsx`:
- Around line 102-104: The phoneNumber field in the hacker profile form
currently allows empty strings due to the optional pattern in the regex, which
conflicts with the requirement to make phone mandatory. Update the phoneNumber
validation in the schema to enforce phone as required: remove the optional
pattern markers from the regex (the trailing and internal `?` characters) and
ensure the field does not accept empty strings, so that edited hacker
applications must include a valid phone number in either the ten-digit or
XXX-XXX-XXXX format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9e88ce1e-b0d4-464a-9e44-0e2e38d59691
📒 Files selected for processing (6)
apps/blade/src/app/_components/dashboard/hacker/hacker-application-form.tsxapps/blade/src/app/_components/settings/hacker-profile-form.tsxpackages/api/src/routers/hackers/mutations.tspackages/consts/src/forms/schools.tspackages/db/drizzle/0009_smooth_forgotten_one.sqlpackages/ui/src/responsive-combo-box.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/consts/src/forms/schools.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/routers/hackers/mutations.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
MLH reached out requesting the following changes as a requirement for 2027 Season Launch certification:
What
School list: Updated FORMS.SCHOOLS to match the latest CSV (let me know if we want to prioritize the Florida schools at the top).
MLH checkboxes: Updated checkbox copy to reflect current MLH + DEV language. The "agree to receive emails" checkbox is now optional per MLH guidance, while the Code of Conduct and data sharing checkboxes remain required.
Phone number (form): Added min(1, "Required") validation to the hacker application form Zod schema. The member profile form is unaffected (phone number is still optional here).
Phone number (db): Updated the knight_hacks_hacker table to set phone_number as NOT NULL. Updated createHacker and updateHacker mutations.
Migration files: Schema change was generated via pnpm db:generate.
Test Plan
Verified locally against a test hackathon by submitting applications under the following conditions:
Confirmed local DB changed. Frontend also displayed correct error/pass feedback. Migration files showed changes to schema.
Loink to testing vid: https://youtu.be/Nyqvqi-Al5M
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Improvements