Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors forms into shared Form* primitives with per-field validation and Apollo client name checks; replaces project input with debounced Autocomplete ProjectSelector; changes default menteesLimit 5→0; post-mutation navigation uses mutation-returned keys; updates unit tests to mock useApolloClient/query and use userEvent; updates CSS tokens and grids. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/__tests__/unit/pages/CreateModule.test.tsx (1)
88-116: Consider simplifying the project selection logic.The multiple fallback strategies for selecting a project option (role, text, data-key, keyboard) suggest the test may be brittle. Consider:
- Standardizing the ProjectSelector's test attributes (e.g.,
data-testid)- Moving detailed interaction testing to a component-level test for ProjectSelector
- Using a single, reliable selector in integration tests
Based on learnings, page-level tests should focus on submission handling and navigation rather than detailed form interaction patterns.
frontend/src/app/globals.css (1)
20-39: Form container styles use!importantoverrides.The styles for
.module-form-containerand.program-form-containeruse!importantto override Hero UI's default slot styles. While this ensures proper error message wrapping and width behavior, it creates tight coupling to Hero UI's internal structure.Consider:
- Checking if Hero UI provides configuration props to avoid
!important- Documenting these overrides for future Hero UI version upgrades
- Adding a comment explaining why these overrides are necessary
frontend/src/components/ProgramForm.tsx (3)
133-134: Consider renaming the function to reflect its async nature.The function is named
checkNameUniquenessSyncbut it's anasyncfunction returning aPromise. A clearer name would becheckNameUniquenessorcheckNameUniquenessAsync.
254-266: Consider addingonBlurhandler for consistent validation feedback.Unlike the
namefield which setstouchedon change, the textarea doesn't provide blur-based feedback. Users won't see validation errors until form submission.🔎 Suggested enhancement
<textarea id="program-description" placeholder="Enter program description" value={formData.description} onChange={(e) => handleInputChange('description', e.target.value)} + onBlur={() => setTouched((prev) => ({ ...prev, description: true }))} rows={4}
419-426: Consider usingrouter.back()for consistency withModuleForm.
ModuleFormusesrouter.back()from Next.js navigation, whileProgramFormuseshistory.back(). Using the Next.js router provides better integration with the framework and avoids potential SSR issues.🔎 Suggested change
+'use client' + +import { useRouter } from 'next/navigation' // ... other imports const ProgramForm = ({ ... }: ProgramFormProps) => { + const router = useRouter() // ... <Button type="button" variant="bordered" - onPress={() => history.back()} + onPress={() => router.back()} className="font-medium" >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/__tests__/unit/pages/EditModule.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsxfrontend/src/app/globals.cssfrontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/create/page.tsxfrontend/src/components/ModuleForm.tsxfrontend/src/components/ProgramForm.tsx
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/EditModule.test.tsxfrontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/src/components/ProgramForm.tsxfrontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/src/components/ProgramForm.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/src/components/ProgramForm.tsxfrontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsxfrontend/src/components/ModuleForm.tsxfrontend/src/app/my/mentorship/programs/create/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/__tests__/unit/pages/CreateModule.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsxfrontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/__tests__/unit/pages/EditProgram.test.tsx
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/ProgramForm.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/ModuleForm.tsxfrontend/src/app/my/mentorship/programs/create/page.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: Use React's useId() hook rather than manually generating random strings when creating accessibility identifiers for UI components. This creates stable, unique IDs without causing hydration mismatches.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/app/globals.css
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx
🧬 Code graph analysis (2)
frontend/src/components/ProgramForm.tsx (2)
frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetMyProgramsDocument(35-35)frontend/src/types/button.ts (1)
Button(4-9)
frontend/src/components/ModuleForm.tsx (2)
frontend/src/types/button.ts (1)
Button(4-9)frontend/src/types/__generated__/projectQueries.generated.ts (1)
SearchProjectNamesDocument(39-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (24)
frontend/__tests__/unit/pages/EditModule.test.tsx (2)
94-94: LGTM!The test selector update correctly reflects the removal of the asterisk from the "Name" label, aligning with the Hero UI Forms migration.
106-109: LGTM!The selector update correctly targets the new autocomplete-based ProjectSelector using its placeholder text, aligning with the component refactor to Hero UI Forms.
frontend/__tests__/unit/pages/EditProgram.test.tsx (2)
1-1: LGTM!The Apollo Client mock setup enables testing of client-side GraphQL queries (e.g., program name uniqueness checks) and follows the consistent testing pattern established across the test suite.
Also applies to: 21-21, 31-44
102-102: LGTM!The label selector update aligns with the Hero UI Forms migration, removing the asterisk from required field labels.
frontend/__tests__/unit/pages/CreateModule.test.tsx (2)
2-3: LGTM!Switching to
userEventprovides more realistic user interaction simulation, and moving timer cleanup toafterEachimproves test isolation.Also applies to: 48-50
120-123: Verify the navigation target after module creation.The test now expects navigation to the program page (
/my/mentorship/programs/test-program) instead of the newly created module's detail page. Confirm this UX change is intentional, as users might expect to see the module they just created.frontend/__tests__/unit/pages/CreateProgram.test.tsx (3)
1-1: LGTM!The Apollo Client mock setup follows the consistent pattern established across the test suite, enabling proper testing of client-side GraphQL queries.
Also applies to: 12-12, 33-48
74-74: LGTM!Label selector updates consistently reflect the Hero UI Forms migration, removing asterisks from required field labels.
Also applies to: 117-117, 141-150, 200-209
168-168: Verify the semantics ofmenteesLimit: 0.The default mentees limit changed from 5 to 0. Clarify whether
0means:
- No mentees allowed in the program
- Unlimited mentees (no limit)
- To be specified later by the admin
Ensure this default aligns with your business logic and that validation/documentation reflects the intended behavior.
frontend/src/app/my/mentorship/programs/create/page.tsx (1)
26-26: Verify the defaultmenteesLimitvalue of 0.The default changed from 5 to 0. Confirm this aligns with your business requirements:
- Does
0mean no mentees allowed, unlimited mentees, or a placeholder value?- Should there be validation to prevent programs with
menteesLimit: 0if it means "no mentees"?- Consider adding a comment or using a more explicit constant (e.g.,
UNLIMITED_MENTEESorNO_LIMIT)frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx (1)
113-123: LGTM!Using the server-returned
updatedModuleKeyfor navigation correctly handles cases where the server might regenerate the module key (e.g., when the name changes). The fallback to the originalmoduleKeyensures navigation works even if the response structure changes.frontend/src/app/globals.css (2)
41-62: LGTM!The responsive grid implementation provides a clean, mobile-first layout that scales appropriately across breakpoints (1/2/3 columns), with proper overflow handling for child elements.
79-80: LGTM!The primary color update to
210 76% 48%(approximately #1D7BD7) appears consistent with previous design system changes. Based on learnings, this color has been used elsewhere in the codebase for visual consistency.Also applies to: 112-113
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx (3)
34-34: Verify the defaultmenteesLimitvalue of 0.The default changed from 5 to 0 in both initial state and when loading existing program data. This should align with the same business logic as the create flow. Confirm whether
0represents no limit, no mentees allowed, or a placeholder requiring explicit configuration.Also applies to: 76-76
106-117: LGTM!Using the server-returned
updatedProgramKeyfor navigation is the correct approach, ensuring the redirect works even if the server regenerates the program key based on updated properties (e.g., name changes).
150-150: LGTM!Passing
currentProgramKeyto the ProgramForm enables proper validation in edit mode (e.g., excluding the current program when checking name uniqueness), which aligns with the enhanced client-side validation mentioned in the PR summary.frontend/src/components/ModuleForm.tsx (8)
58-65: LGTM!The select change handler correctly handles the various key formats from HeroUI's Select component and ensures only valid single selections update the form state.
67-113: LGTM!The validation functions provide clear, consistent validation logic for all required fields with appropriate error messages.
139-176: LGTM!The submit handler correctly validates all required fields synchronously and prevents submission when any validation errors exist.
472-479: Potential edge case with controlled value synchronization.The effect syncs
inputValuewhendefaultNamechanges, but ifvalue(projectId) changes whiledefaultNameremains the same, the input won't update. This could happen if a parent resets projectId without clearing projectName.Consider whether this edge case can occur in your usage patterns. If the parent always updates both
valueanddefaultNametogether, this is fine.
482-508: LGTM!The debounced fetch correctly handles the search flow with minimum character requirements, excludes the currently selected project, and limits results for better UX. The 300ms debounce is appropriate for reducing API calls.
539-542: LGTM!Good UX pattern to suppress validation errors while the user is actively typing a search query. This prevents premature error messages during the normal search-and-select flow.
546-580: LGTM!The Autocomplete is well-configured with appropriate settings for a required project selector: triggered on input, disallows custom values, shows loading state, and applies consistent styling.
427-441: LGTM!The button section correctly uses explicit
typeattributes, proper disabled state handling during loading, and Next.js router for navigation. Based on learnings, explicit button types prevent unintended form submission behavior.
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/ProgramForm.tsx (1)
177-197: Race condition still present: uniqueness error not blocking submission.The async
setNameUniquenessErrorat line 180 won't update state beforevalidateNameruns at line 185. SincevalidateNamereadsnameUniquenessErrorfrom state (which is stillundefined), the form will submit despite finding a duplicate.The local
uniquenessErrorvariable must be included in the validation check:🔎 Proposed fix
if (formData.name.trim()) { const uniquenessError = await checkNameUniquenessSync(formData.name) if (uniquenessError) { setNameUniquenessError(uniquenessError) + return } }Or alternatively, include it in the final check:
+ let uniquenessError: string | undefined if (formData.name.trim()) { - const uniquenessError = await checkNameUniquenessSync(formData.name) + uniquenessError = await checkNameUniquenessSync(formData.name) if (uniquenessError) { setNameUniquenessError(uniquenessError) } } // ...validation code... // Prevent submission if any validation errors exist - if (nameError || descriptionError || startDateError || endDateError || menteesLimitError) { + if (nameError || uniquenessError || descriptionError || startDateError || endDateError || menteesLimitError) { return }
🧹 Nitpick comments (4)
frontend/src/components/ProgramForm.tsx (4)
281-299: Date inputs missing touched state update.The date inputs don't update touched state on value change, unlike the name field. Consider adding
onBlurhandlers or updating touched on change for consistent validation feedback.🔎 Proposed fix for Start Date
<Input id="program-start-date" type="date" label="Start Date" labelPlacement="outside" value={formData.startedAt} - onValueChange={(value) => handleInputChange('startedAt', value)} + onValueChange={(value) => { + handleInputChange('startedAt', value) + setTouched((prev) => ({ ...prev, startedAt: true })) + }} isRequired
419-426: Consider usingwindow.history.back()for clarity.While
history.back()works in browser environments, explicitly usingwindow.history.back()or importing a router's navigation function would be clearer and more consistent with Next.js patterns.🔎 Proposed fix
<Button type="button" variant="bordered" - onPress={() => history.back()} + onPress={() => window.history.back()} className="font-medium" >Alternatively, use Next.js router:
import { useRouter } from 'next/navigation' // ... const router = useRouter() // ... onPress={() => router.back()}
232-239: Consider extracting repeatedclassNamesconfiguration.The same
classNamesobject is duplicated across all Input components. Extracting it to a constant would reduce repetition and make future styling changes easier.🔎 Proposed fix
// Add near the top of the component const inputClassNames = { base: 'w-full min-w-0', label: 'text-sm font-semibold text-gray-600 dark:text-gray-300', input: 'text-gray-800 dark:text-gray-200', inputWrapper: 'bg-gray-50 dark:bg-gray-800', helperWrapper: 'min-w-0 max-w-full w-full', errorMessage: 'break-words whitespace-normal max-w-full w-full', } // Then use in each Input: <Input // ...other props classNames={inputClassNames} />
112-131: Validation functions could be memoized to satisfy exhaustive-deps.The
eslint-disableforreact-hooks/exhaustive-depsmasks that the validation functions are recreated on each render. While the current code works correctly because the functions only use their parameters (andnameUniquenessErroris in deps forvalidateName), wrapping them inuseCallbackwould make the dependency explicit and remove the need for the disable comment.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ProgramForm.tsx
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleForm.tsx:112-134
Timestamp: 2025-07-14T16:18:07.287Z
Learning: In the OWASP Nest mentorship system, date validation for modules (ensuring start date precedes end date) is handled on the backend in the module GraphQL mutations via the `_validate_module_dates` helper function in backend/apps/mentorship/graphql/mutations/module.py, which prevents invalid date ranges from being stored in the database.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/ProgramForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/src/components/ProgramForm.tsx (2)
193-231: LGTM!The handleSubmit function correctly addresses the previously flagged race condition by capturing the async uniqueness check result in a local variable (line 209) before validation. The comprehensive validation flow ensures all fields are checked before form submission.
257-266: Textarea missing touched state update on change.The description textarea (line 262) only calls
handleInputChangeon change, unlike the name input (lines 247-250) which also sets the touched state. This inconsistency means validation errors won't appear for the description field until form submission, creating an inconsistent UX.🔎 Proposed fix
<FormTextarea id="program-description" label="Description" placeholder="Enter program description" value={formData.description} - onChange={(e) => handleInputChange('description', e.target.value)} + onChange={(e) => { + handleInputChange('description', e.target.value) + setTouched((prev) => ({ ...prev, description: true })) + }} error={errors.description} touched={touched.description} required />frontend/src/components/ModuleForm.tsx (1)
410-424: Silent failure when selected project is not found in items.If a user selects a project but the
itemsarray has been updated (e.g., due to a debounced fetch completing), theselectedProjectat line 414 will beundefinedand no action occurs. The user receives no feedback about this failure state.🔎 Proposed defensive handling
const selectedKey = Array.from(keySet as Set<string>)[0] if (selectedKey) { const selectedProject = items.find((item) => item.id === selectedKey) if (selectedProject) { setInputValue(selectedProject.name) onProjectChange(selectedProject.id, selectedProject.name) + } else { + // Project not found - clear selection + setInputValue('') + onProjectChange(null, '') } } else {
🧹 Nitpick comments (2)
frontend/src/components/forms/shared/FormDateInput.tsx (1)
5-12: Extract duplicated COMMON_INPUT_CLASS_NAMES constant.The
COMMON_INPUT_CLASS_NAMESobject is duplicated betweenFormDateInput.tsxandFormTextInput.tsx. This duplication makes it harder to maintain consistent styling across form inputs.🔎 Proposed refactor
Create a new shared constants file:
// frontend/src/components/forms/shared/formConstants.ts export const COMMON_INPUT_CLASS_NAMES = { base: 'w-full min-w-0', label: 'text-sm font-semibold text-gray-600 dark:text-gray-300', input: 'text-gray-800 dark:text-gray-200', inputWrapper: 'bg-gray-50 dark:bg-gray-800', helperWrapper: 'min-w-0 max-w-full w-full', errorMessage: 'break-words whitespace-normal max-w-full w-full', }Then import in both files:
'use client' import { Input } from '@heroui/react' +import { COMMON_INPUT_CLASS_NAMES } from './formConstants' -const COMMON_INPUT_CLASS_NAMES = { - base: 'w-full min-w-0', - label: 'text-sm font-semibold text-gray-600 dark:text-gray-300', - input: 'text-gray-800 dark:text-gray-200', - inputWrapper: 'bg-gray-50 dark:bg-gray-800', - helperWrapper: 'min-w-0 max-w-full w-full', - errorMessage: 'break-words whitespace-normal max-w-full w-full', -}Apply the same change to
FormTextInput.tsx.frontend/src/components/ProgramForm.tsx (1)
74-91: Consider consolidating validation functions.The
validateName(lines 74-80) andvalidateNameWithUniqueness(lines 82-91) functions are nearly identical, differing only in how they access the uniqueness error (state vs. parameter). This duplication can be reduced.🔎 Proposed consolidation
-const validateName = (value: string): string | undefined => { - const requiredError = validateRequired(value, 'Name') - if (requiredError) return requiredError - if (value.length > 200) return 'Name must be 200 characters or less' - if (nameUniquenessError) return nameUniquenessError - return undefined -} - -const validateNameWithUniqueness = ( +const validateName = ( value: string, - uniquenessError?: string | undefined + uniquenessError?: string ): string | undefined => { const requiredError = validateRequired(value, 'Name') if (requiredError) return requiredError if (value.length > 200) return 'Name must be 200 characters or less' - if (uniquenessError) return uniquenessError + if (uniquenessError !== undefined ? uniquenessError : nameUniquenessError) return uniquenessError !== undefined ? uniquenessError : nameUniquenessError return undefined }Then update usages:
- Line 129:
validator: () => validateName(formData.name, nameUniquenessError)- Line 216:
const nameError = validateName(formData.name, uniquenessError)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/src/components/ModuleForm.tsxfrontend/src/components/ProgramForm.tsxfrontend/src/components/forms/shared/FormButtons.tsxfrontend/src/components/forms/shared/FormContainer.tsxfrontend/src/components/forms/shared/FormDateInput.tsxfrontend/src/components/forms/shared/FormTextInput.tsxfrontend/src/components/forms/shared/FormTextarea.tsx
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/forms/shared/FormTextInput.tsxfrontend/src/components/forms/shared/FormButtons.tsxfrontend/src/components/ProgramForm.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-12T17:12:25.807Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/ProgramDetails.test.tsx:35-0
Timestamp: 2025-07-12T17:12:25.807Z
Learning: In React applications, button elements should always have an explicit type attribute (type="button", type="submit", or type="reset") to prevent unintended form submission behavior, even when the button appears to be outside of a form context. The default type is "submit" which can cause unexpected behavior.
Applied to files:
frontend/src/components/forms/shared/FormTextarea.tsxfrontend/src/components/forms/shared/FormContainer.tsxfrontend/src/components/forms/shared/FormButtons.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/src/components/forms/shared/FormContainer.tsxfrontend/src/components/ProgramForm.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/forms/shared/FormButtons.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ModuleForm.tsx
🧬 Code graph analysis (2)
frontend/src/components/ProgramForm.tsx (8)
backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetMyProgramsDocument(35-35)frontend/src/components/forms/shared/FormContainer.tsx (1)
FormContainer(12-33)backend/apps/github/models/common.py (1)
title(40-44)frontend/src/components/forms/shared/FormTextInput.tsx (1)
FormTextInput(29-62)frontend/src/components/forms/shared/FormTextarea.tsx (1)
FormTextarea(17-51)frontend/src/components/forms/shared/FormDateInput.tsx (1)
FormDateInput(25-52)frontend/src/components/forms/shared/FormButtons.tsx (1)
FormButtons(12-35)
frontend/src/components/ModuleForm.tsx (5)
frontend/src/components/forms/shared/FormTextInput.tsx (1)
FormTextInput(29-62)frontend/src/components/forms/shared/FormTextarea.tsx (1)
FormTextarea(17-51)frontend/src/components/forms/shared/FormDateInput.tsx (1)
FormDateInput(25-52)frontend/src/components/forms/shared/FormButtons.tsx (1)
FormButtons(12-35)frontend/src/types/__generated__/projectQueries.generated.ts (1)
SearchProjectNamesDocument(39-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (7)
frontend/src/components/forms/shared/FormTextarea.tsx (1)
1-51: LGTM!The FormTextarea component is well-implemented with proper error handling, required field indicators, and responsive dark mode styling. The touched-state pattern for error display provides good UX.
frontend/src/components/forms/shared/FormContainer.tsx (1)
1-33: LGTM!The FormContainer provides a clean abstraction for form layout with consistent styling. The
noValidateattribute on line 27 correctly delegates validation to the custom per-field validation logic implemented in ProgramForm and ModuleForm.Based on learnings, HTML validation is being replaced with JavaScript validation in this refactor.
frontend/src/components/forms/shared/FormButtons.tsx (1)
1-35: LGTM!The FormButtons component correctly implements explicit button types (
type="button"for Cancel,type="submit"for Save) and provides flexible cancellation handling. The loading state properly disables the submit button to prevent duplicate submissions.Based on learnings, explicit button types are essential to prevent unintended form submission behavior.
frontend/src/components/ProgramForm.tsx (1)
233-338: Well-structured form composition.The refactored form effectively uses the new shared Form* components with consistent validation patterns. The addition of
currentProgramKey(lines 43, 54) properly enables name uniqueness checks that exclude the current program during edits.frontend/src/components/ModuleForm.tsx (3)
59-66: LGTM!The
handleSelectChangefunction correctly handles Hero UI's complex key types (single key, Set, or 'all') and properly updates the touched state when a selection is made.
155-192: LGTM!The handleSubmit function implements comprehensive client-side validation, marking all required fields as touched and preventing submission when validation errors exist. This provides immediate feedback to users about incomplete or invalid fields.
Based on learnings, this client-side validation complements the form component architecture and replaces the previous HTML-only validation approach.
431-434: Excellent UX pattern for validation display.The logic to suppress validation errors while the user is actively typing (lines 432-434) provides a better user experience by avoiding premature error messages. Errors only display when the field is empty or the user has finished their input.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/src/components/ProgramForm.tsx (1)
245-254: Textarea missing touched state update on change.The description
FormTextareaonly callshandleInputChangewithout marking the field as touched. Unlike the name input (lines 235-238) which updates touched state on change, validation errors for description won't display until form submission.🔎 Proposed fix
<FormTextarea id="program-description" label="Description" placeholder="Enter program description" value={formData.description} - onChange={(e) => handleInputChange('description', e.target.value)} + onChange={(e) => { + handleInputChange('description', e.target.value) + setTouched((prev) => ({ ...prev, description: true })) + }} error={errors.description} touched={touched.description} required />frontend/src/components/ModuleForm.tsx (1)
401-415: Silent failure when selected project is not found in items.If
selectedKeyexists but the corresponding project isn't found initems(e.g., items were cleared by a concurrent debounced fetch), no action is taken and the user receives no feedback.🔎 Suggested defensive handling
const handleSelectionChange = (keys: React.Key | Set<React.Key> | 'all') => { const keySet = keys instanceof Set ? keys : keys === 'all' ? new Set() : new Set([keys]) const selectedKey = Array.from(keySet as Set<string>)[0] if (selectedKey) { const selectedProject = items.find((item) => item.id === selectedKey) if (selectedProject) { setInputValue(selectedProject.name) onProjectChange(selectedProject.id, selectedProject.name) + } else { + // Project not found - may have been filtered out; keep current input + console.warn('Selected project not found in current items list') } } else { // Selection cleared setInputValue('') onProjectChange(null, '') } }
🧹 Nitpick comments (1)
frontend/src/components/forms/shared/useFormValidation.ts (1)
16-31: Consider exportingValidationRuletype for type-safe consumer usage.The
ValidationRuletype is defined internally but consumers ofuseFormValidationneed to construct objects matching this shape. Exporting the type would provide better TypeScript support and IDE autocompletion for callers.🔎 Proposed fix
-type ValidationRule = { +export type ValidationRule = { field: string shouldValidate: boolean validator: () => string | undefined }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/ModuleForm.tsxfrontend/src/components/ProgramForm.tsxfrontend/src/components/forms/shared/FormTextInput.tsxfrontend/src/components/forms/shared/useFormValidation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/forms/shared/FormTextInput.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/forms/shared/useFormValidation.tsfrontend/src/components/ProgramForm.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-07-08T17:24:36.501Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Applied to files:
frontend/src/components/ProgramForm.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/src/components/ProgramForm.tsxfrontend/src/components/ModuleForm.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ModuleForm.tsx
🧬 Code graph analysis (1)
frontend/src/components/ProgramForm.tsx (7)
frontend/src/components/forms/shared/useFormValidation.ts (1)
useFormValidation(16-32)frontend/src/types/__generated__/programsQueries.generated.ts (1)
GetMyProgramsDocument(35-35)frontend/src/components/forms/shared/FormContainer.tsx (1)
FormContainer(12-33)frontend/src/components/forms/shared/FormTextInput.tsx (1)
FormTextInput(29-62)frontend/src/components/forms/shared/FormTextarea.tsx (1)
FormTextarea(17-51)frontend/src/components/forms/shared/FormDateInput.tsx (1)
FormDateInput(25-52)frontend/src/components/forms/shared/FormButtons.tsx (1)
FormButtons(12-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (10)
frontend/src/components/forms/shared/useFormValidation.ts (1)
20-31: Acknowledge the intentional design but note the stale closure risk.The pattern of passing
dependenciesas a parameter with the eslint-disable is a known approach for flexible memoization. However, thevalidationsarray is reconstructed on each render (in ProgramForm/ModuleForm), so if a caller inadvertently omits a dependency that affects a validator's closure, errors may become stale.The current usage in ProgramForm and ModuleForm correctly includes
formDataandtouchedin dependencies, so this works as intended. Just be mindful when adding new validators that capture external state.frontend/src/components/ProgramForm.tsx (3)
150-179: LGTM!The name uniqueness check implementation is well-designed:
- Correctly excludes the current program when editing
- Uses case-insensitive comparison
- Silently fails gracefully, relying on backend validation as fallback
181-219: Submit validation flow is well-structured.The race condition from the previous review has been properly addressed by capturing
uniquenessErrorin a local variable and usingvalidateNameWithUniquenessinstead of relying on state. The validation chain is comprehensive.
221-325: Clean modular form structure with shared components.The refactored form properly leverages the new shared components (
FormContainer,FormTextInput,FormTextarea,FormDateInput,FormButtons) with consistent styling and responsive layout. The conditional rendering of admin fields for edit mode is correctly implemented.frontend/src/components/ModuleForm.tsx (6)
206-218: LGTM - Textarea touched state properly implemented.The description
FormTextareanow correctly updates touched state on change (line 213), addressing the previous review comment.
60-67: LGTM!The experience level select handler correctly extracts the value from the key set and properly updates both
formDataandtouchedstate.
143-180: LGTM!The submit validation flow is comprehensive, properly marking all fields as touched and validating each required field before allowing submission.
355-362: Sync logic looks reasonable but has intentional eslint-disable.The
useEffecthandles initial population and external resets. The eslint-disable is necessary to avoid the dependency loop withinputValue. This pattern works for the current use case.
422-425: Good UX pattern for error display suppression.Suppressing validation errors while the user is actively typing prevents premature "Project is required" messages during search. Errors only display when appropriate.
429-463: LGTM!The Autocomplete configuration is well-suited for project selection:
allowsCustomValue={false}ensures only valid projects can be selected,menuTrigger="input"provides intuitive UX, and loading state is properly communicated.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/ModuleForm.tsx (1)
397-407: Silent failure when selected project is not found in items.This issue was flagged in a previous review: if the user selects an item but
itemshas been updated (e.g., due to a debounced fetch completing),selectedProjectwill beundefinedand no action is taken. The user receives no feedback. Consider adding defensive handling.
🧹 Nitpick comments (2)
frontend/src/components/ModuleForm.tsx (2)
68-82: Consider simplifyinghandleSelectChangefor single-select usage.The
keys === 'all'branch creates an empty set which results invaluebeingundefined, causing no state update. For a single-selectSelectcomponent, the'all'case is typically unreachable. You could simplify this by removing the'all'branch or adding a comment explaining why it's handled this way.🔎 Optional simplification
const handleSelectChange = (keys: React.Key | Set<React.Key> | 'all') => { - let keySet: Set<React.Key> - if (keys instanceof Set) { - keySet = keys - } else if (keys === 'all') { - keySet = new Set() - } else { - keySet = new Set([keys]) - } - const [value] = Array.from(keySet as Set<string>) + // Single-select: normalize to Set and extract first value + const keySet = keys instanceof Set ? keys : new Set([keys]) + const [value] = Array.from(keySet as Set<string>) if (value) { setFormData((prev) => ({ ...prev, experienceLevel: value })) setTouched((prev) => ({ ...prev, experienceLevel: true })) } }
341-348: Acknowledge the eslint suppression for sync effect.The
react-hooks/exhaustive-depssuppression is intentional here to prevent an infinite loop when syncinginputValuewith external props. The conditiondefaultName !== inputValueuses the currentinputValueat call time, which is acceptable for this controlled/uncontrolled synchronization pattern. Consider adding a brief comment explaining whyinputValueis excluded.🔎 Optional documentation
useEffect(() => { + // Sync inputValue with external props when they change. + // inputValue is intentionally excluded from deps to prevent infinite loops. if (value && defaultName && defaultName !== inputValue) { setInputValue(defaultName) } else if (!value && !defaultName && inputValue) { setInputValue('') } - // eslint-disable-next-line react-hooks/exhaustive-deps }, [defaultName, value])Note: The eslint comment can be kept if you prefer explicit suppression.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ModuleForm.tsxfrontend/src/components/forms/shared/useFormValidation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/forms/shared/useFormValidation.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/src/components/ModuleForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (5)
frontend/src/components/ModuleForm.tsx (5)
1-51: LGTM!The imports are well-organized, and the
ModuleFormPropsinterface clearly defines the expected form data structure. TheEXPERIENCE_LEVELSconstant provides a clean mapping from enum values to display labels.
84-118: LGTM!The validation functions are well-organized.
validateEndDateLocalcorrectly capturesformData.startedAtfor date comparison, andvalidateProjectproperly validates both the ID and trimmed name. TheuseFormValidationhook integration with per-field touched tracking provides good UX.
120-157: LGTM!The
handleSubmitfunction correctly marks all required fields as touched before validation, ensuring error messages display for untouched fields. The synchronous validation followed by conditional submission is a clean pattern.
415-418: Good UX: validation errors suppressed while typing.The
isTypinglogic appropriately suppresses validation errors while the user is actively searching for a project. This prevents premature error messages during the search flow. Once the user clears the input or submits without selecting, the error will display as expected.
420-458: LGTM!The
Autocompletecomponent is well-configured with controlledinputValueandselectedKey, proper loading state, and styled class names for overflow handling. TheallowsCustomValue={false}ensures users must select from the fetched results, maintaining data integrity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/ModuleForm.tsx (1)
388-409: Existing minor issue: Silent handling when selected project not found in items.This issue was previously flagged and remains unaddressed. When a user selects a key but the corresponding project is not found in
items(e.g., due to a race condition with the debounced fetch), the code treats it as a cleared selection (lines 404-408). While this prevents errors, it provides no user feedback.The previous review suggested adding defensive handling such as logging or user feedback when this edge case occurs.
🧹 Nitpick comments (2)
frontend/src/components/ModuleForm.tsx (2)
230-255: Consider replacing inline styles with Tailwind classes.Lines 230, 283, and 422 use inline
style={{ maxWidth: '100%', overflow: 'hidden' }}. Since the codebase uses Tailwind CSS extensively, consider using utility classes instead for consistency.🔎 Proposed refactor
- <div className="w-full min-w-0" style={{ maxWidth: '100%', overflow: 'hidden' }}> + <div className="w-full min-w-0 max-w-full overflow-hidden">Apply the same change to lines 283 and 422.
Also applies to: 283-283, 422-422
352-378: Consider memoizing the debounced function separately.The debounced function is recreated whenever
clientorvaluechanges (line 377). While this works in practice (value changes only on selection), a more robust pattern would memoize the debounced function separately usinguseMemooruseRefto prevent losing pending timers.🔎 Alternative pattern
// Create stable debounced function const debouncedQuery = useMemo( () => debounce(async (query: string, currentValue: string) => { const trimmedQuery = query.trim() if (trimmedQuery.length < 2) { setItems([]) setIsLoading(false) return } setIsLoading(true) try { const { data } = await client.query({ query: SearchProjectNamesDocument, variables: { query: trimmedQuery }, }) const projects = data.searchProjects || [] const filtered = projects.filter((proj) => proj.id !== currentValue) setItems(filtered.slice(0, 5)) } catch { setItems([]) } finally { setIsLoading(false) } }, 300), [client] ) // Then call with current values const fetchSuggestions = useCallback( (query: string) => { debouncedQuery(query, value) }, [debouncedQuery, value] )This ensures the debounce timer persists across renders.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ModuleForm.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/components/ModuleForm.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/src/components/ModuleForm.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModuleForm.tsx (6)
frontend/src/components/forms/shared/formValidationUtils.ts (3)
validateName(14-20)validateEndDate(30-37)getCommonValidationRules(53-81)frontend/src/components/forms/shared/useFormValidation.ts (1)
useFormValidation(9-25)frontend/src/components/forms/shared/FormTextInput.tsx (1)
FormTextInput(29-62)frontend/src/components/forms/shared/FormTextarea.tsx (1)
FormTextarea(17-51)frontend/src/components/forms/shared/FormDateInput.tsx (1)
FormDateInput(25-52)frontend/src/components/forms/shared/FormButtons.tsx (1)
FormButtons(12-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
frontend/src/components/ModuleForm.tsx (7)
120-157: LGTM—Standard form submission pattern.The submit handler correctly marks all fields as touched and runs validation synchronously before calling
onSubmit. The manual validation is necessary becausesetTouchedis asynchronous, and we need immediate validation results to prevent submission.
171-184: LGTM—Name input now updates touched state correctly.The name input now properly marks the field as touched when the value changes (line 178), ensuring validation errors display immediately during user interaction.
✅ This addresses a previous review comment about missing touched state updates.
186-198: LGTM—Description textarea now updates touched state correctly.The description textarea now properly marks the field as touched when the value changes (line 193), ensuring validation errors display immediately during user interaction.
✅ This addresses a previous review comment about missing touched state updates.
205-229: LGTM—Date inputs now update touched state correctly.Both start date (line 211) and end date (line 223) inputs now properly mark their fields as touched when values change, ensuring validation errors display immediately during user interaction.
✅ This addresses a previous review comment about missing touched state updates.
287-294: LGTM—ProjectId now updates touched state correctly.The
onProjectChangecallback now properly marks the projectId field as touched when the user interacts with the ProjectSelector (line 293), ensuring validation errors display appropriately.✅ This addresses a previous review comment about missing touched state updates.
416-419: LGTM—Smart validation suppression while typing.The
isTypinglogic (lines 416-419) suppresses validation errors while the user is actively typing but hasn't selected a project yet. This prevents showing "required" errors prematurely and improves the user experience.
342-349: Verify the useEffect synchronization logic.The effect synchronizes
inputValuewith externaldefaultName/valuechanges, intentionally omittinginputValuefrom dependencies to avoid loops. The logic handles two cases:
- Line 343: When both
valueanddefaultNameexist and differ from currentinputValue- Line 345: When both are empty but
inputValuehas contentThis should work correctly for edit mode initialization and clearing, but the conditions are complex.
Consider testing edge cases such as:
- Initial render with
defaultNameset (edit mode)- Parent clearing the selection (
value='',defaultName='') while user has typed something- Rapid parent updates to
defaultNameThese scenarios should maintain correct input synchronization without loops or stale values.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/forms/shared/formValidationUtils.ts (2)
3-8: Optional: Remove redundant type check.Since
valueis typed asstring, thetypeof value === 'string'check on Line 4 is redundant and will always be true. The condition can be simplified to:if (!value || !value.trim()) {This maintains the same behavior while being more concise.
🔎 Proposed simplification
export const validateRequired = (value: string, fieldName: string): string | undefined => { - if (!value || (typeof value === 'string' && !value.trim())) { + if (!value || !value.trim()) { return `${fieldName} is required` } return undefined }
35-47: Recommended: Export type definitions for consumer use.While TypeScript can infer these types from the
getCommonValidationRulesfunction signature, explicitly exportingCommonFormDataandCommonTouchedwould improve the developer experience by:
- Enabling explicit typing of form state in consumer components
- Providing better IDE autocomplete and type checking
- Making the API more self-documenting
🔎 Suggested export
-type CommonFormData = { +export type CommonFormData = { name: string description: string startedAt: string endedAt: string } -type CommonTouched = { +export type CommonTouched = { name?: boolean description?: boolean startedAt?: boolean endedAt?: boolean }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/pages/CreateProgram.test.tsxfrontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsxfrontend/src/components/forms/shared/formValidationUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/pages/CreateProgram.test.tsx
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/components/forms/shared/formValidationUtils.tsfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx
📚 Learning: 2025-09-29T06:02:35.566Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/components/SingleModuleCard.tsx:54-54
Timestamp: 2025-09-29T06:02:35.566Z
Learning: In the Module type from types/mentorship.ts, the experienceLevel field is required (experienceLevel: ExperienceLevelEnum), not optional, so null/undefined checks are not needed when accessing this field.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx
📚 Learning: 2025-07-13T11:34:31.823Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/page.tsx:14-14
Timestamp: 2025-07-13T11:34:31.823Z
Learning: In the Next.js frontend mentorship application, there are two distinct types for authentication-related data: ExtendedSession for useSession hook (containing accessToken and user.login properties) and UserRolesData for useUserRoles hook (containing currentUserRoles.roles array). The correct access pattern for GitHub username is `(session as ExtendedSession)?.user?.login`.
Applied to files:
frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/src/components/forms/shared/formValidationUtils.ts
🧬 Code graph analysis (1)
frontend/src/components/forms/shared/formValidationUtils.ts (1)
frontend/src/components/forms/shared/useFormValidation.ts (1)
ValidationRule(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (8)
frontend/src/components/forms/shared/formValidationUtils.ts (3)
10-24: LGTM!The validation logic for name, description, and start date is clean and correct. The 200-character limit for names is reasonable, and the wrapper functions provide a clear, maintainable API.
26-33: Verify: Does the business logic permit same-day events?Line 29 uses
<=comparison, which means events cannot start and end on the same date/time. If your application needs to support single-day events or events wherestartDateequalsendDate(e.g., all-day events on the same day), consider changing the comparison to<:if (startDate && new Date(value) < new Date(startDate)) { return 'End date must be after start date' }If multi-day events are strictly required, the current logic is correct.
49-77: LGTM! Well-designed validation rule builder.The
getCommonValidationRulesfunction demonstrates good design:
- Accepts custom validators via parameters for extensibility (e.g., uniqueness checks)
- Line 73 intelligently re-validates
endedAtwhenstartedAtis touched and an end date exists, ensuring the date relationship validation stays current- Clean separation of concerns between generic and custom validation logic
This provides a solid foundation for the new form validation infrastructure.
frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/edit/page.tsx (1)
114-115: Good defensive pattern for capturing mutation result and navigating with updated key.The code correctly captures the mutation result and extracts the module key with a sensible fallback to the original
moduleKey. TheUpdateModuleDocumentmutation includes thekeyfield in its GraphQL selection set, ensuring the value is always available. The subsequent navigation usingupdatedModuleKeyensures users land on the correct module page after the update.frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx (4)
107-108: LGTM! Good practice to use mutation-returned key.Capturing the updated program key from the mutation result and using it for navigation is more reliable than client-side slug generation. The optional chaining with a fallback to the original
programKeyprovides good defensive coding.
118-118: LGTM! Navigation uses server-returned key.Using
updatedProgramKeyfrom the mutation result ensures the navigation path is consistent with the server state, avoiding potential slug mismatches.
151-151: LGTM! Edit context provided to form.Passing
currentProgramKeytoProgramFormenables the form to properly scope name-uniqueness validation during edits, preventing false conflicts with the program's own current name.
35-35: Confirm the menteesLimit default change from 5 to 0 represents intended behavior.The backend model defines
mentees_limitas aPositiveIntegerFieldthat allows 0 as a valid value. Combined with the field's help text ("Maximum number of mentees allowed in this program"), a default of 0 means programs will allow zero new mentees by default — effectively closed to new mentees at creation/edit time.This differs significantly from the previous default of 5. While the change is technically valid, confirm this behavioral shift is intentional and aligned with product requirements.
* Update forms to use Hero UI components * Update code * Update Program form required fields * Potential fix for pull request finding 'Useless conditional' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * Address code duplication * Address comments * Update code to fix duplication * Update code * Address Sonar issues * Clean up * Update touched state for project selector * Update code --------- Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
2 participants
Switched to using Hero UI Forms instead of custom coded forms.
Checklist
make check-testlocally and all tests passed