fix: Improve ProgramForm validation to avoid duplicate backend requests#4216
Open
mrkeshav-05 wants to merge 11 commits intoOWASP:mainfrom
Open
fix: Improve ProgramForm validation to avoid duplicate backend requests#4216mrkeshav-05 wants to merge 11 commits intoOWASP:mainfrom
mrkeshav-05 wants to merge 11 commits intoOWASP:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTranslate DB unique-constraint IntegrityErrors into GraphQL VALIDATION_ERROR for program name; remove client-side uniqueness queries; surface backend field validation in ProgramForm via extractGraphQLErrors; adjust page submit handlers to suppress generic toasts for validation errors; update tests to mock GraphQL validation responses. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
Confidence score: 3/5
- There is moderate merge risk because
frontend/src/components/ProgramForm.tsxhas a concrete validation gap (severity 6/10, high confidence) where persisted backend name errors are ignored on submit, which can let users repeatedly submit already-invalid input. frontend/src/utils/helpers/handleGraphQLError.tshas a medium issue where broaderrorsarray detection may misclassify non-GraphQL failures, potentially hiding fallback messages and making error feedback less reliable.- Given one user-facing validation regression risk plus one error-handling robustness issue, this looks mergeable with caution after targeted verification.
- Pay close attention to
frontend/src/components/ProgramForm.tsxandfrontend/src/utils/helpers/handleGraphQLError.ts- submit validation and error classification behavior may produce incorrect user feedback.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ProgramForm.tsx">
<violation number="1" location="frontend/src/components/ProgramForm.tsx:120">
P2: Submit validation ignores persisted backend name errors, allowing repeated submissions for already-invalid input.</violation>
</file>
<file name="frontend/src/utils/helpers/handleGraphQLError.ts">
<violation number="1" location="frontend/src/utils/helpers/handleGraphQLError.ts:34">
P2: Broad `errors` array detection can misclassify non-GraphQL errors and suppress fallback error messages.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/mentorship/api/internal/mutations/program.py`:
- Around line 53-59: The _handle_program_save_integrity_error function currently
assumes every IntegrityError is due to the "name" unique constraint; change it
to inspect the IntegrityError (e.g., exc.orig or str(exc)) to detect which
constraint was violated (name vs key or fallback) and set the GraphQLError
message and extensions["field"] accordingly (e.g., "name" or "key" or a generic
validation field) before re-raising the GraphQLError from the original
exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0a2d9a8-ec9e-4c8a-b24e-538d384445c8
📒 Files selected for processing (7)
backend/apps/mentorship/api/internal/mutations/program.pyfrontend/__tests__/unit/components/ProgramForm.test.tsxfrontend/__tests__/unit/utils/handleGraphQLError.test.tsfrontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsxfrontend/src/app/my/mentorship/programs/create/page.tsxfrontend/src/components/ProgramForm.tsxfrontend/src/utils/helpers/handleGraphQLError.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ProgramForm.tsx`:
- Around line 133-140: The current try/catch in the ProgramForm submission
swallows all errors so parent pages never receive non-validation errors; modify
the submit error handling in the function that calls await onSubmit(e) to use
extractGraphQLErrors(error) and, after calling
setBackendErrors(validationErrors) when hasValidationErrors is true, rethrow the
original error when hasValidationErrors is false (or when validationErrors are
absent) so upstream handlers in create/page.tsx and edit/page.tsx can run;
reference the onSubmit call site, extractGraphQLErrors, setBackendErrors and
ensure non-validation errors are not suppressed but propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7157db7d-be9a-4496-91b6-78f53485f5fd
📒 Files selected for processing (2)
frontend/src/components/ProgramForm.tsxfrontend/src/utils/helpers/handleGraphQLError.ts
mrkeshav-05
force-pushed
the
fix/issue-3977
branch
from
c1810e1 to
250d5b7
Compare
mrkeshav-05
force-pushed
the
fix/issue-3977
branch
from
250d5b7 to
7908117
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
frontend/src/components/ProgramForm.tsx (2)
133-140:⚠️ Potential issue | 🟠 MajorRethrow non-validation failures.
This catch still swallows network/server/unmapped GraphQL errors. When
extractGraphQLErrorsdoes not return field validation errors, the original exception needs to bubble so the page-level handlers can show their generic error state/toast.Suggested fix
try { await onSubmit(e) } catch (error) { const { validationErrors, hasValidationErrors } = extractGraphQLErrors(error) if (hasValidationErrors) { setBackendErrors(validationErrors) + } else { + throw error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ProgramForm.tsx` around lines 133 - 140, The catch block around awaiting onSubmit(e) currently swallows all errors; after calling extractGraphQLErrors(error) you must rethrow the original error when hasValidationErrors is false so non-validation failures bubble to page-level handlers. Update the try/catch that calls onSubmit(e) to only call setBackendErrors(validationErrors) when hasValidationErrors is true and otherwise throw the received error (keep references to onSubmit, extractGraphQLErrors, setBackendErrors to locate the change).
62-76:⚠️ Potential issue | 🟠 MajorBackend field errors only render for
name.
setBackendErrors(validationErrors)stores a generic field-error map, but the validation pipeline only readsbackendErrors.name. If the backend returns a field-level error fordescription,startedAt,endedAt, ormenteesLimit, this component drops it instead of surfacing it on the form. Please mergebackendErrors[field]into each field’s displayed error, not justname.Suggested direction
- const errors = useFormValidation( + const localErrors = useFormValidation( [ ...getCommonValidationRules(formData, touched, validateNameLocal, validateEndDateLocal), { field: 'menteesLimit', shouldValidate: touched.menteesLimit ?? false, validator: () => validateMenteesLimit(formData.menteesLimit), }, ], [formData, touched, backendErrors] ) + + const errors = { + ...localErrors, + name: backendErrors.name ?? localErrors.name, + description: backendErrors.description ?? localErrors.description, + startedAt: backendErrors.startedAt ?? localErrors.startedAt, + endedAt: backendErrors.endedAt ?? localErrors.endedAt, + menteesLimit: backendErrors.menteesLimit ?? localErrors.menteesLimit, + }Also applies to: 94-103, 136-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ProgramForm.tsx` around lines 62 - 76, The component only reads backendErrors.name when computing field errors, so backend field errors for other fields (e.g., description, startedAt, endedAt, menteesLimit) are never surfaced; update each field validator and the UI error resolution to merge backendErrors[field] into the displayed error (e.g., in validateNameLocal and analogous validateDescriptionLocal/validateStartedAtLocal, etc.) and when rendering field error props use combinedError = localValidationError || backendErrors[field]; also ensure handleInputChange clears only backendErrors[name] as it does now and that setBackendErrors stores the full Record<string,string> returned from the server so these backendErrors[field] merges work everywhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/ProgramForm.tsx`:
- Around line 133-140: The catch block around awaiting onSubmit(e) currently
swallows all errors; after calling extractGraphQLErrors(error) you must rethrow
the original error when hasValidationErrors is false so non-validation failures
bubble to page-level handlers. Update the try/catch that calls onSubmit(e) to
only call setBackendErrors(validationErrors) when hasValidationErrors is true
and otherwise throw the received error (keep references to onSubmit,
extractGraphQLErrors, setBackendErrors to locate the change).
- Around line 62-76: The component only reads backendErrors.name when computing
field errors, so backend field errors for other fields (e.g., description,
startedAt, endedAt, menteesLimit) are never surfaced; update each field
validator and the UI error resolution to merge backendErrors[field] into the
displayed error (e.g., in validateNameLocal and analogous
validateDescriptionLocal/validateStartedAtLocal, etc.) and when rendering field
error props use combinedError = localValidationError || backendErrors[field];
also ensure handleInputChange clears only backendErrors[name] as it does now and
that setBackendErrors stores the full Record<string,string> returned from the
server so these backendErrors[field] merges work everywhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d7d3a3b-580f-4674-bb0b-c3c1bad6e608
📒 Files selected for processing (1)
frontend/src/components/ProgramForm.tsx
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/ProgramForm.tsx">
<violation number="1" location="frontend/src/components/ProgramForm.tsx:140">
P1: Non-validation submit errors are swallowed after removing the rethrow branch, preventing upstream error handling for real backend/network failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
There was a problem hiding this comment.
Pull request overview
This PR removes ProgramForm’s client-side “duplicate program name” lookup (which triggered an extra GetMyPrograms request) and instead relies on backend uniqueness enforcement, with frontend support for mapping GraphQL validation errors (via extensions) to field-level form errors.
Changes:
- Frontend: remove query-based name uniqueness validation in
ProgramFormand surface backend validation errors on the form. - Frontend: expand GraphQL error extraction to support Apollo Client v4
CombinedGraphQLErrors(errorsarray). - Backend: translate
IntegrityErrorduring program create/update into aGraphQLErrorwithextensionsfor field-level validation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/utils/helpers/handleGraphQLError.ts | Adds support for extracting GraphQL errors from either errors or graphQLErrors. |
| frontend/src/components/ProgramForm.tsx | Removes frontend uniqueness query; captures backend validation errors and wires them into name validation. |
| frontend/src/app/my/mentorship/programs/create/page.tsx | Suppresses failure toast for backend validation errors and rethrows so the form can render field errors. |
| frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx | Same as create page: suppress toast for validation errors; rethrow for form-level handling. |
| frontend/tests/unit/utils/handleGraphQLError.test.ts | Adds tests for Apollo v4 errors-based error shape. |
| frontend/tests/unit/components/ProgramForm.test.tsx | Replaces uniqueness-query tests with backend validation-error rendering tests. |
| backend/apps/mentorship/api/internal/mutations/program.py | Catches IntegrityError on create/save and raises a GraphQLError with validation extensions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+55
to
+58
| msg = "A program with this name already exists." | ||
| raise GraphQLError( | ||
| msg, | ||
| extensions={"code": "VALIDATION_ERROR", "field": "name"}, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
This PR removes the frontend duplicate name check in
ProgramFormthat previously made an extra backend request usingGetMyPrograms. Instead, name uniqueness is now handled solely by backend validation.Resolves: #3977
Changes:
checkNameUniquenessSyncquery-based validation.Checklist
make check-testlocally: all warnings addressed, tests passed