🐛 Duplicate name validation for questionnaire #2536
Conversation
Signed-off-by: Shveta Sachdeva <[email protected]>
WalkthroughFetches existing questionnaires and strengthens YAML import validation: parses YAML to JSON, enforces Questionnaire shape, rejects duplicate names (trimmed, lowercased), surfaces errors via form Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImportForm
participant QuestionnairesService
participant Notification
Note over ImportForm,QuestionnairesService: Import flow (high-level)
User->>ImportForm: Select / drop YAML file
ImportForm->>QuestionnairesService: fetch existing questionnaires (useFetchQuestionnaires)
QuestionnairesService-->>ImportForm: existing questionnaires
ImportForm->>ImportForm: derive existingNames (useMemo: trim().toLowerCase())
ImportForm->>ImportForm: convertYamlToJson(file) -> JSON | null
alt YAML invalid or parse error
ImportForm->>ImportForm: setError("yamlFile", t("validation.invalidQuestionnaireYAML") / t("message.errorReadingFile"))
ImportForm-->>User: show validation error
else YAML parsed
ImportForm->>ImportForm: validate JSON is Questionnaire
alt Not a Questionnaire
ImportForm->>ImportForm: setError("yamlFile", t("validation.invalidQuestionnaireYAML"))
ImportForm-->>User: show validation error
else Valid Questionnaire
ImportForm->>ImportForm: check name ∉ existingNames
alt Duplicate name
ImportForm->>ImportForm: setError("yamlFile", t("validation.duplicateName"))
ImportForm-->>User: abort creation
else Unique name
ImportForm->>QuestionnairesService: createQuestionnaire(JSON)
QuestionnairesService-->>ImportForm: success
ImportForm->>Notification: pushNotification(success)
ImportForm-->>User: show success
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (2)
75-80: Handle 409 conflict to cover server-enforced uniqueness and client race conditionsEven with client-side checks, two users can race to create the same name. Handle a 409 Conflict with a friendly duplicate message; retain the generic fallback otherwise.
Apply this diff:
const onHandleFailedQuestionnaireCreation = (error: AxiosError) => { - pushNotification({ - title: getAxiosErrorMessage(error), - variant: "danger", - }); + if (error?.response?.status === 409) { + pushNotification({ + title: "Duplicate questionnaire", + message: "A questionnaire with this name already exists.", + variant: "danger", + timeout: 30000, + }); + return; + } + pushNotification({ + title: getAxiosErrorMessage(error), + variant: "danger", + }); };And ensure the backend enforces a unique constraint on questionnaire name to guarantee consistency.
63-73: Bug: onSaved is invoked twice on successonSaved(response) is followed by onSaved(), causing duplicate callbacks.
Apply this diff:
const onHandleSuccessfulQuestionnaireCreation = (response: Questionnaire) => { - onSaved(response); + onSaved(response); pushNotification({ title: t("toastr.success.createWhat", { type: t("terms.questionnaire"), what: response.name, }), variant: "success", }); - onSaved(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (1)
client/src/app/queries/questionnaires.ts (1)
useFetchQuestionnaires(36-54)
🔇 Additional comments (1)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (1)
21-24: LGTM: Added questionnaire query hook importImporting useFetchQuestionnaires here is appropriate.
.../src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
.../src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
sjd78
changed the title
sjd78
changed the title
.../src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
.../src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
.../src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Shveta Sachdeva <[email protected]>
sshveta
force-pushed
the
bug-fix-MTA-2783
branch
from
b47d3cc to
aebbfc8
Compare
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (1)
68-78: onSaved is called twice (duplicate callback)onSaved(response) is invoked, then onSaved() is called again. This can cause double state updates, duplicate navigation, or duplicated notifications downstream.
Apply this diff:
const onHandleSuccessfulQuestionnaireCreation = (response: Questionnaire) => { onSaved(response); pushNotification({ title: t("toastr.success.createWhat", { type: t("terms.questionnaire"), what: response.name, }), variant: "success", }); - onSaved(); };
♻️ Duplicate comments (2)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (2)
43-44: Blocking submit while questionnaires load — addressed prior feedbackDestructuring isFetching from useFetchQuestionnaires fixes the race where duplicates could slip through before data loads. Nice.
245-247: Submit disabled during fetch — nice UX safeguardAdding isFetching into the disabled condition prevents premature submission before the duplicate check can be trusted.
🧹 Nitpick comments (5)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (5)
45-47: Use a Set and remove unnecessary optional chaining
- questionnaires already defaults to [], so questionnaires? and the || [] fallback are redundant.
- Using a Set of normalized names makes membership checks O(1) and avoids duplicate entries.
- Consider Unicode normalization (NFKC) to avoid confusables.
Apply this diff:
- const existingNames = useMemo(() => { - return questionnaires?.map(({ name }) => name.trim().toLowerCase()) || []; - }, [questionnaires]); + const existingNames = useMemo(() => { + return new Set( + questionnaires + .map(({ name }) => + typeof name === "string" + ? name.normalize("NFKC").trim().toLowerCase() + : "" + ) + .filter(Boolean) + ); + }, [questionnaires]);And below (Line 127), switch includes(...) to has(...).
125-137: Harden duplicate validation: guard name type and normalize onceIf questionnaireData.name isn’t a string or is whitespace, .trim() will throw or pass a blank name through. Add a type check and normalize once. If you adopt the Set change above, also use .has(...).
Apply this diff:
- // Duplicate validation - const normalizedName = questionnaireData.name.trim().toLowerCase(); - const isDuplicate = existingNames.includes(normalizedName); - - if (isDuplicate) { + // Duplicate validation + const candidateName = questionnaireData.name; + if (typeof candidateName !== "string" || !candidateName.trim()) { + pushNotification({ + title: "Failed", + message: "Invalid questionnaire name.", + variant: "danger", + timeout: 30000, + }); + return; + } + const normalizedName = candidateName + .normalize("NFKC") + .trim() + .toLowerCase(); + const isDuplicate = + // If existingNames is a Set<string> (preferred), use has(); otherwise, fallback to includes(). + (existingNames as unknown as Set<string>).has + ? (existingNames as unknown as Set<string>).has(normalizedName) + : (existingNames as unknown as string[]).includes(normalizedName); + + if (isDuplicate) { pushNotification({ title: "Duplicate questionnaire", message: "A questionnaire with this name already exists.", variant: "danger", timeout: 30000, }); return; }Note: Replace hardcoded strings with t(...) for localization when convenient.
108-115: Tighten the Questionnaire type guardThe current guard only checks key presence. Ensure name/description are strings to avoid runtime errors when trimming/lowercasing.
Apply this diff:
- function isQuestionnaire(data: any): data is Questionnaire { - return ( - typeof data === "object" && - data !== null && - "name" in data && - "description" in data - ); - } + function isQuestionnaire(data: unknown): data is Pick<Questionnaire, "name" | "description"> { + if (!data || typeof data !== "object" || Array.isArray(data)) return false; + const maybe: any = data; + return typeof maybe.name === "string" && typeof maybe.description === "string"; + }
94-106: Don’t cast YAML parse errors as AxiosError; handle genericallyjsYaml.load throws generic Error, not AxiosError. Casting can hide useful messages. Prefer generic error handling and localize the title/message.
Apply this diff:
const convertYamlToJson = (yamlString: string) => { try { const jsonData = jsYaml.load(yamlString); return jsonData; } catch (error) { - pushNotification({ - title: "Failed", - message: getAxiosErrorMessage(error as AxiosError), - variant: "danger", - timeout: 30000, - }); + const message = + error instanceof Error ? error.message : String(error); + pushNotification({ + title: t("toastr.error.generic", { defaultValue: "Failed" }), + message, + variant: "danger", + timeout: 30000, + }); } };
100-105: Localize user-visible stringsSeveral strings are hardcoded. For consistency with the rest of the file (and i18n), wrap them with t(...). Examples: “Failed”, “Duplicate questionnaire”, “Invalid JSON data.”, “You should select a YAML file.”, and the “Upload” label.
Example diff (apply similarly to other occurrences):
- title: "Duplicate questionnaire", - message: "A questionnaire with this name already exists.", + title: t("toastr.error.duplicateQuestionnaireTitle", { defaultValue: "Duplicate questionnaire" }), + message: t("toastr.error.duplicateQuestionnaireMessage", { defaultValue: "A questionnaire with this name already exists." }),- browseButtonText="Upload" + browseButtonText={t("actions.upload", { defaultValue: "Upload" })}- You should select a YAML file. + {t("dialog.message.mustSelectYaml", { defaultValue: "You should select a YAML file." })}Also applies to: 129-135, 141-146, 214-218, 230-236, 225-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (1)
client/src/app/queries/questionnaires.ts (1)
useFetchQuestionnaires(36-54)
⏰ 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). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (2)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (2)
1-1: Importing useMemo to precompute names is appropriateGood call adding useMemo; it avoids recomputing the normalized names on every render.
21-24: Correct query importsBringing in useFetchQuestionnaires and the create mutation here makes sense and keeps the form self-contained.
.../src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Shveta Sachdeva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (3)
98-108: Fix: onSaved() is invoked twice on successonHandleSuccessfulQuestionnaireCreation calls onSaved(response) and then onSaved() again. This can double-trigger parent effects (e.g., close dialog twice, extra refetch).
Apply this diff:
const onHandleSuccessfulQuestionnaireCreation = (response: Questionnaire) => { onSaved(response); pushNotification({ title: t("toastr.success.createWhat", { type: t("terms.questionnaire"), what: response.name, }), variant: "success", }); - onSaved(); };
191-216: Reset rejection state after a valid selection/clearisFileRejected is set to true on rejection but never reset, keeping the file field in error state even after a valid file is chosen or cleared.
Apply this diff:
onFileInputChange={async (_, file) => { try { if (!file) { return; } + setIsFileRejected(false); const reader = new FileReader(); @@ onClearClick={() => { onChange(""); setFilename(""); + setIsFileRejected(false); }}Also applies to: 225-228
182-186: Fix i18n key mismatch in file import-questionnaire-form.tsxI confirmed that the only definition in client/public/locales/en/translation.json is
"dialog": { "message": { "maxfileSize": "Max file size of 1MB exceeded. Upload a smaller file." } }and the code in client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx still uses the incorrect key
dialog.message.maxFileSize. This will prevent the error message from resolving.Please apply this diff at lines 182–186:
- setError(name, { - type: "custom", - message: t("dialog.message.maxFileSize"), - }); + setError(name, { + type: "custom", + message: t("dialog.message.maxfileSize"), + });No other occurrences of either key were found in the codebase.
Let me know if any other locale files or code paths need verification.
🧹 Nitpick comments (8)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (8)
234-241: Localize hard-coded helper textReplace the hard-coded string with a translation key for consistency.
Apply this diff:
- <HelperTextItem variant="error"> - You should select a YAML file. - </HelperTextItem> + <HelperTextItem variant="error"> + {t("validation.invalidFormat")} + </HelperTextItem>
68-71: i18n: use translated entity name in duplicate messagePass the localized “questionnaire” string instead of a raw English literal.
Apply this diff:
- t("validation.duplicateName", { type: "questionnaire" }), + t("validation.duplicateName", { type: t("terms.questionnaire") }),
175-179: Accept additional YAML MIME typesSome browsers/os report YAML as application/x-yaml or text/x-yaml. Widen accept to reduce false rejections.
Apply this diff:
dropzoneProps={{ accept: { - "text/yaml": [".yml", ".yaml"], + "text/yaml": [".yml", ".yaml"], + "application/x-yaml": [".yml", ".yaml"], + "text/x-yaml": [".yml", ".yaml"], }, maxSize: 1000000,
217-223: Prefer field error over toast for file read failures hereThis catch isn’t Axios-related; getAxiosErrorMessage will likely produce “Network error”. Surface the issue inline next to the field using the same translation as elsewhere.
Apply this diff:
- pushNotification({ - title: "Failed", - message: getAxiosErrorMessage(err as AxiosError), - variant: "danger", - timeout: 30000, - }); + setError(name, { + type: "custom", + message: t("message.errorReadingFile"), + });
46-49: Minor: questionnaires is always an array; drop optional chaininguseFetchQuestionnaires returns questionnaires: data || [], so the optional chain and fallback are redundant.
Apply this diff:
const existingNames = useMemo(() => { - return questionnaires?.map(({ name }) => name.trim().toLowerCase()) || []; + return questionnaires.map(({ name }) => name.trim().toLowerCase()); }, [questionnaires]);
53-82: Reduce double YAML parsing and tailor error messages in one testThe schema parses YAML twice (once per test). You can consolidate to a single test and still keep specific messages by using createError with function() syntax.
Apply this diff:
- yamlFile: yup - .string() - .required(t("validation.invalidQuestionnaireYAML")) - .test( - "Valid Questionnaire YAML", - t("validation.invalidQuestionnaireYAML"), - (yamlFile) => { - if (!yamlFile) { - return true; - } - const jsonData = convertYamlToJson(yamlFile); - return isQuestionnaire(jsonData); - } - ) - .test( - "Duplicate name", - t("validation.duplicateName", { type: t("terms.questionnaire") }), - (yamlFile) => { - if (!yamlFile) { - return true; - } - const jsonData = convertYamlToJson(yamlFile); - if (isQuestionnaire(jsonData)) { - const normalizedName = jsonData.name.trim().toLowerCase(); - const isDuplicate = existingNames.includes(normalizedName); - return !isDuplicate; - } - return true; - } - ), + yamlFile: yup + .string() + .required(t("validation.invalidQuestionnaireYAML")) + .test("validate-questionnaire", t("validation.invalidQuestionnaireYAML"), function (yamlFile) { + if (!yamlFile) return true; + const jsonData = convertYamlToJson(yamlFile); + if (!isQuestionnaire(jsonData)) { + return this.createError({ message: t("validation.invalidQuestionnaireYAML") }); + } + const normalizedName = jsonData.name.trim().toLowerCase(); + if (existingNames.includes(normalizedName)) { + return this.createError({ + message: t("validation.duplicateName", { type: t("terms.questionnaire") }), + }); + } + return true; + }),
131-138: Optional: strengthen the Questionnaire type guardEnsure name/description are non-empty strings; prevents passing structurally invalid objects to createQuestionnaire.
Apply this diff:
function isQuestionnaire(data: unknown): data is Questionnaire { return ( - typeof data === "object" && - data !== null && - "name" in data && - "description" in data + typeof data === "object" && + data !== null && + "name" in data && + "description" in data && + typeof (data as any).name === "string" && + typeof (data as any).description === "string" && + (data as any).name.trim().length > 0 && + (data as any).description.trim().length > 0 ); }
140-159: Optional: simplify onSubmit; rely on schema guaranteesWith validation ensuring a well-formed Questionnaire, convert and submit without try/catch; mutation callbacks already handle errors. Keeps concerns separated.
Apply this diff:
- const onSubmit = ({ yamlFile }: ImportQuestionnaireFormValues) => { - if (yamlFile) { - try { - createQuestionnaire(convertYamlToJson(yamlFile) as Questionnaire); - } catch (error) { - pushNotification({ - title: "Failed", - message: - error instanceof AxiosError - ? getAxiosErrorMessage(error as AxiosError) - : error instanceof Error - ? error.message - : "Error", - variant: "danger", - timeout: 30000, - }); - } - } - }; + const onSubmit = ({ yamlFile }: ImportQuestionnaireFormValues) => { + // At this point, schema has validated yamlFile and uniqueness. + const data = convertYamlToJson(yamlFile) as Questionnaire; + createQuestionnaire(data); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
client/public/locales/en/translation.json(2 hunks)client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx (5)
client/src/app/components/NotificationsContext.tsx (1)
NotificationsContext(31-33)client/src/app/queries/questionnaires.ts (1)
useFetchQuestionnaires(36-54)client/src/app/api/models.ts (1)
Questionnaire(817-833)client/src/app/api/rest.ts (1)
createQuestionnaire(672-675)client/src/app/utils/utils.ts (1)
getAxiosErrorMessage(10-16)
⏰ 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). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (2)
client/public/locales/en/translation.json (2)
231-231: LGTM: new translation key "message.errorReadingFile"Key is clear and used by the import form to surface file-read errors.
625-625: LGTM: new translation key "validation.invalidQuestionnaireYAML"Fits the validation flow moved into the Yup schema for the YAML import.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/public/locales/en/translation.json (1)
628-628: Added validation.invalidQuestionnaireYAML: precise and aligned with the new YAML import validation.Sentence-casing and “YAML” capitalization are consistent with nearby keys (e.g., “Invalid format”). Optional: if you plan to surface parse specifics, consider a second key that accepts a reason (e.g., “Invalid questionnaire YAML: {{reason}}”) and fall back to the generic one when details aren’t available.
As a quick check, ensure the validation path shows this message (not the generic invalidFormat) when YAML parsing or schema validation fails for questionnaires. The script above already searches for usages; re-run after wiring the validation to confirm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
client/public/locales/en/translation.json(2 hunks)
🔇 Additional comments (1)
client/public/locales/en/translation.json (1)
231-231: message.errorReadingFile key verified and translations in place
- Usage confirmed in
client/src/app/pages/assessment-management/import-questionnaire-form/import-questionnaire-form.tsx
• Line 55 & 58:t("validation.invalidQuestionnaireYAML")
• Line 207:message: t("message.errorReadingFile")- All non-English
translation.jsonfiles include both
•message.errorReadingFile
•validation.invalidQuestionnaireYAMLLooks good to merge—no further changes needed. Let me know if you’d like assistance adding or updating other locales in the future.
Resolves: https://issues.redhat.com/browse/MTA-2783
Summary by CodeRabbit
New Features
Bug Fixes
Documentation