fix(dashboard): fix validation issues and improve option group management UX#4360
Open
latifniz wants to merge 7 commits intovendurehq:masterfrom
Open
fix(dashboard): fix validation issues and improve option group management UX#4360latifniz wants to merge 7 commits intovendurehq:masterfrom
latifniz wants to merge 7 commits intovendurehq:masterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR adds trimmed-input validation, required-field and duplicate-name checks to AddOptionGroupDialog and AddOptionValueDialog, shows localized error toasts for validation failures, resets dialog form state when opened and after successful creation, threads existing group/value name lists into the dialogs for duplicate detection, triggers creation mutations and appends new groups/values to products on success, and adds UI for removing option groups with confirmation and error handling. Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/dashboard/src/app/routes/_authenticated/_products/components/add-option-group-dialog.tsx (1)
61-95:⚠️ Potential issue | 🟠 MajorUntrimmed name used in mutation payload.
trimmedNameis computed for validation (Lines 63–73), but the mutation on Lines 78, 82, and 86 still uses the originalformValue.name(which may contain leading/trailing whitespace). Thecodeandnamefields sent to the backend will retain that whitespace.Proposed fix
try { const createResult = await createOptionGroupMutation.mutateAsync({ input: { - code: formValue.name.toLowerCase().replace(/\s+/g, '-'), + code: trimmedName.toLowerCase().replace(/\s+/g, '-'), translations: [ { languageCode: 'en', - name: formValue.name, + name: trimmedName, }, ], options: formValue.values.map(value => ({packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx (1)
117-140:⚠️ Potential issue | 🟠 MajorSame untrimmed-name bug as in
AddOptionGroupDialog.
trimmedValueis used for the duplicate check (Line 124) but the mutation payload on Lines 132 and 136 still sends the originalvalues.namewith potential whitespace.Also, the duplicate check here uses case-sensitive
includes(Line 124), whileAddOptionGroupDialoguses case-insensitive comparison. This inconsistency means "Red" and "red" would be treated as duplicates for groups but allowed for values.Proposed fix
const onSubmit = (values: AddOptionValueFormValues) => { const trimmedValue = values.name.trim(); - // Prevent duplicates const currentValues = existingValues ?? []; - - if (currentValues.includes(trimmedValue)) { + if (currentValues.some(v => v.toLowerCase() === trimmedValue.toLowerCase())) { toast.error(t`Option value already exists`); return; } createOptionMutation.mutate({ input: { productOptionGroupId: groupId, - code: values.name.toLowerCase().replace(/\s+/g, '-'), + code: trimmedValue.toLowerCase().replace(/\s+/g, '-'), translations: [ { languageCode: 'en', - name: values.name, + name: trimmedValue, }, ], }, }); };
🤖 Fix all issues with AI agents
In
`@packages/dashboard/src/app/routes/_authenticated/_orders/components/edit-order-table.tsx`:
- Around line 120-132: The onKeyDown handler currently uses Number(input) and
defaultValue so invalid (NaN) or fractional quantities can be sent and the input
won't reflect server-side updates; modify the Enter handler in the input tied to
row.original.quantity to validate and sanitize before calling onAdjustLine
(e.g., trim, ensure it's a finite integer >= 0 using parseInt or rounding and
reject non-numeric or fractional values), and prevent calling onAdjustLine when
validation fails; also keep the input in sync after mutations by either making
it controlled (bind value to row.original.quantity) or add a key prop (e.g.,
key={`qty-${row.original.id}-${row.original.quantity}`}) to force remount when
quantity changes.
In
`@packages/dashboard/src/app/routes/_authenticated/_products/products_`.$id_.variants.tsx:
- Line 337: The line rendering AddOptionGroupDialog is longer than the
110-character Prettier limit; split the JSX props onto multiple lines so each
prop is on its own line (or wrapped sensibly) to shorten the line length. Locate
the AddOptionGroupDialog usage and break out productId={id},
existingGroupNames={productData.product.optionGroups.map(g => g.name)}, and
onSuccess={() => refetch()} onto separate lines (or wrap the map expression
across lines) so the full JSX element conforms to the 110-character rule.
🧹 Nitpick comments (3)
packages/dashboard/src/app/routes/_authenticated/_products/components/add-option-group-dialog.tsx (1)
43-51:formin the dependency array may cause unnecessary resets.
useFormreturns a new object reference on every render in some React Hook Form versions, which would re-trigger this effect and reset the form while the user is typing. Consider usingform.resetdirectly (stable ref) or moving the reset into theonOpenChangecallback instead.Safer alternative
- useEffect(() => { - if (open) { - form.reset({ - name: '', - values: [], - }); - } - }, [open, form]); + useEffect(() => { + if (open) { + form.reset({ + name: '', + values: [], + }); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [open]);packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx (2)
96-100: Sameformdependency concern as the other dialog.Same recommendation as in
AddOptionGroupDialog: consider droppingformfrom the dep array to avoid potential re-reset on every render.
311-332: Delete handler: consider disabling button while mutation is pending.The delete button doesn't guard against double-clicks while
removeOptionGroupMutationis in progress. A rapid second click would fire anotherconfirm+ mutation call.Proposed fix
<Button size="icon" variant="ghost" + disabled={removeOptionGroupMutation.isPending} onClick={() => {
Comment on lines
120
to
132
| defaultValue={row.original.quantity} | ||
| placeholder="1" | ||
| onKeyDown={e => { | ||
| if (e.key !== 'Enter') return; | ||
| const input = e.currentTarget.value.trim(); | ||
| const quantity = Number(input); | ||
|
|
||
| onAdjustLine({ | ||
| lineId: row.original.id, | ||
| quantity: value, | ||
| quantity, | ||
| customFields: row.original.customFields, | ||
| }); | ||
| }} |
Contributor
There was a problem hiding this comment.
Missing input validation — NaN and fractional quantities can be submitted.
Number(input) can produce NaN (e.g. non-numeric text) or fractional values (e.g. 1.5), both of which are sent straight to onAdjustLine. Guard against invalid values before dispatching.
Additionally, switching from controlled value to defaultValue means the input won't reflect server-side quantity changes after a successful mutation (e.g. if the backend clamps or rejects the value). Consider using a key prop tied to the quantity to force re-render, or reverting to a controlled input.
Proposed fix for input validation
onKeyDown={e => {
if (e.key !== 'Enter') return;
const input = e.currentTarget.value.trim();
- const quantity = Number(input);
+ const quantity = parseInt(input, 10);
+ if (isNaN(quantity) || quantity < 0) return;
onAdjustLine({
lineId: row.original.id,
quantity,
customFields: row.original.customFields,
});
}}To keep defaultValue in sync after mutations, add a key to force remount:
<Input
+ key={`${row.original.id}-${row.original.quantity}`}
type="number"
min={0}
defaultValue={row.original.quantity}🤖 Prompt for AI Agents
In
`@packages/dashboard/src/app/routes/_authenticated/_orders/components/edit-order-table.tsx`
around lines 120 - 132, The onKeyDown handler currently uses Number(input) and
defaultValue so invalid (NaN) or fractional quantities can be sent and the input
won't reflect server-side updates; modify the Enter handler in the input tied to
row.original.quantity to validate and sanitize before calling onAdjustLine
(e.g., trim, ensure it's a finite integer >= 0 using parseInt or rounding and
reject non-numeric or fractional values), and prevent calling onAdjustLine when
validation fails; also keep the input in sync after mutations by either making
it controlled (bind value to row.original.quantity) or add a key prop (e.g.,
key={`qty-${row.original.id}-${row.original.quantity}`}) to force remount when
quantity changes.
packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx
Outdated
Show resolved
Hide resolved
latifniz
force-pushed
the
fix/dashboard-add-option-value-ux
branch
from
c987306 to
ae1f843
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx (1)
117-141:⚠️ Potential issue | 🟠 MajorBug: trimmed value is not used in the mutation payload.
trimmedValueis computed on line 118 and used for the duplicate check, but the mutation on lines 129–140 still sends the rawvalues.name(potentially with leading/trailing whitespace) for bothcodeandtranslations[].name. This means:
- A user can submit
" Red "— it passes the duplicate check against"Red", but the server receives the untrimmed string.- The
codeis derived from the untrimmed value, producing a leading/trailing-hyphen code.Also, the duplicate check is case-sensitive, so
"Red"and"red"are not detected as duplicates.Proposed fix
const onSubmit = (values: AddOptionValueFormValues) => { const trimmedValue = values.name.trim(); + if (!trimmedValue) { + toast.error(t`Option value name is required`); + return; + } // Prevent duplicates - const currentValues = existingValues ?? []; - - if (currentValues.includes(trimmedValue)) { + if (existingValues.some(v => v.toLowerCase() === trimmedValue.toLowerCase())) { toast.error(t`Option value already exists`); return; } createOptionMutation.mutate({ input: { productOptionGroupId: groupId, - code: values.name.toLowerCase().replace(/\s+/g, '-'), + code: trimmedValue.toLowerCase().replace(/\s+/g, '-'), translations: [ { languageCode: 'en', - name: values.name, + name: trimmedValue, }, ], }, }); };
🧹 Nitpick comments (1)
packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx (1)
311-332: Multiple lines exceed the 110-character Prettier limit.Lines 316, 321–322 are well beyond the configured line length. Consider extracting the click handler into a named function to improve readability and comply with the formatting guideline.
Proposed refactor
+ <div className="col-span-1 flex justify-end"> + <Button + size="icon" + variant="ghost" + onClick={() => { + const msg = t`Are you sure you want to remove this option group? This will affect all variants using this option.`; + if (confirm(msg)) { + removeOptionGroupMutation.mutate( + { + productId: id, + optionGroupId: group.id, + }, + { + onError: (error) => { + toast.error( + t`Failed to remove option group`, + { + description: + error instanceof Error + ? error.message + : t`Unknown error`, + }, + ); + }, + }, + ); + } + }} + > + <Trash2 className="h-4 w-4 text-destructive" /> + </Button> + </div>As per coding guidelines, "Enforce line length of 110 characters (Prettier)".
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx (1)
111-133:⚠️ Potential issue | 🟠 MajorBug: trimmed value used for duplicate check but untrimmed value sent in mutation.
trimmedValueis computed on line 112 and used for the duplicate check, but the mutation on lines 125 and 129 still uses the rawvalues.name. A user typing" Red "would pass the duplicate check against"Red"but submit" Red "as the name.Additionally, the duplicate comparison is case-sensitive, so
"Red"and"red"won't be caught.Proposed fix
const onSubmit = (values: AddOptionValueFormValues) => { const trimmedValue = values.name.trim(); // Prevent duplicates const currentValues = existingValues ?? []; - if (currentValues.includes(trimmedValue)) { + if (currentValues.some(v => v.toLowerCase() === trimmedValue.toLowerCase())) { toast.error(t`Option value already exists`); return; } createOptionMutation.mutate({ input: { productOptionGroupId: groupId, - code: values.name.toLowerCase().replace(/\s+/g, '-'), + code: trimmedValue.toLowerCase().replace(/\s+/g, '-'), translations: [ { languageCode: 'en', - name: values.name, + name: trimmedValue, }, ], }, }); };
🧹 Nitpick comments (3)
packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx (3)
304-325: Extract the inline handler and fix line-length violations.Several lines in this block (notably lines 309 and 314–315) exceed the 110-character Prettier limit. The dense inline
onClickhandler with a nestedconfirm,mutate, andonErrorcallback is hard to scan. Consider extracting it into a named function.Proposed refactor
+ const removeOptionGroup = (groupId: string) => { + if ( + confirm( + t`Are you sure you want to remove this option group? This will affect all variants using this option.`, + ) + ) { + removeOptionGroupMutation.mutate( + { productId: id, optionGroupId: groupId }, + { + onError: (error) => { + toast.error(t`Failed to remove option group`, { + description: + error instanceof Error + ? error.message + : t`Unknown error`, + }); + }, + }, + ); + } + };Then in the JSX:
<div className="col-span-1 flex justify-end"> <Button size="icon" variant="ghost" - onClick={() => { - if (confirm(t`Are you sure you want to remove this option group? This will affect all variants using this option.`)) { - removeOptionGroupMutation.mutate( - { productId: id, optionGroupId: group.id }, - { - onError: (error) => { - toast.error(t`Failed to remove option group`, { - description: error instanceof Error ? error.message : t`Unknown error`, - }); - } - } - ); - } - }} + onClick={() => removeOptionGroup(group.id)} >As per coding guidelines, "Enforce line length of 110 characters (Prettier)".
136-138: Nit: trailing whitespace in Dialog opening tag.Line 137 has a trailing space before
>and line 138 is blank — both appear to be accidental.- <Dialog open={open} onOpenChange={setOpen} > - + <Dialog open={open} onOpenChange={setOpen}>
114-115: Unnecessary fallback —existingValuesis always an array.
existingValuesis typed asstring[](line 83) and always passed asgroup.options.map(o => o.name)(line 299), so the?? []fallback on line 115 is dead code. Removing it keeps things clear.- const currentValues = existingValues ?? []; - - if (currentValues.includes(trimmedValue)) { + if (existingValues.includes(trimmedValue)) {
latifniz
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/dashboard/src/app/routes/_authenticated/_products/components/add-option-group-dialog.tsx (1)
76-78:⚠️ Potential issue | 🟡 MinorBug:
codefield uses untrimmed name.Line 78 generates the
codefromformValue.name(untrimmed), but the displaynameon line 82 correctly usestrimmedName. A name like" Size "would produce code"-size-"with leading/trailing dashes.Proposed fix
- code: formValue.name.toLowerCase().replace(/\s+/g, '-'), + code: trimmedName.toLowerCase().replace(/\s+/g, '-'),packages/dashboard/src/app/routes/_authenticated/_products/products_.$id_.variants.tsx (1)
111-131:⚠️ Potential issue | 🟡 MinorInconsistency: duplicate check is case-sensitive here but case-insensitive in
AddOptionGroupDialog.Line 115 uses
existingValues.includes(trimmedValue)(exact match), while the group-name duplicate check inadd-option-group-dialog.tsxline 70 uses.toLowerCase()comparison. A user could add "Red" and "red" as separate option values.Also,
values.nameon lines 123 and 127 should usetrimmedValuefor consistency with the trimming on line 112.Proposed fix
const onSubmit = (values: AddOptionValueFormValues) => { const trimmedValue = values.name.trim(); // Prevent duplicates - if (existingValues.includes(trimmedValue)) { + if (existingValues.some(v => v.toLowerCase() === trimmedValue.toLowerCase())) { toast.error(t`Option value already exists`); return; } createOptionMutation.mutate({ input: { productOptionGroupId: groupId, - code: values.name.toLowerCase().replace(/\s+/g, '-'), + code: trimmedValue.toLowerCase().replace(/\s+/g, '-'), translations: [ { languageCode: 'en', - name: values.name, + name: trimmedValue, }, ], }, }); };
🤖 Fix all issues with AI agents
In
`@packages/dashboard/src/app/routes/_authenticated/_products/products_`.$id_.variants.tsx:
- Around line 301-322: The click handler passed to the Button violates the
110-character line length rule; extract it into a named function (e.g.,
handleRemoveOptionGroup) inside the component and call that function from
onClick to reduce line length and improve readability. In the new
handleRemoveOptionGroup function reference id, group.id and
removeOptionGroupMutation.mutate, perform the confirm check and the mutate call
with the same onError toast logic (using error instanceof Error ? error.message
: t`Unknown error`) so behavior remains identical, then replace the inline arrow
function in the Button's onClick with a call to handleRemoveOptionGroup.
🧹 Nitpick comments (1)
packages/dashboard/src/app/routes/_authenticated/_products/components/add-option-group-dialog.tsx (1)
85-93: Option value names are not trimmed before sending to the API.The option value
code(line 86) andname(line 90) both usevalue.valueas-is. If users enter values with leading/trailing whitespace, those will be persisted untrimmed — and the duplicate check inAddOptionValueDialog(which trims before comparing) won't catch them on a future attempt.Consider trimming consistently:
Proposed fix
options: formValue.values.map(value => ({ - code: value.value.toLowerCase().replace(/\s+/g, '-'), + code: value.value.trim().toLowerCase().replace(/\s+/g, '-'), translations: [ { languageCode: 'en', - name: value.value, + name: value.value.trim(), }, ], })),
Comment on lines
+301
to
+322
| <div className="col-span-1 flex justify-end"> | ||
| <Button | ||
| size="icon" | ||
| variant="ghost" | ||
| onClick={() => { | ||
| if (confirm(t`Are you sure you want to remove this option group? This will affect all variants using this option.`)) { | ||
| removeOptionGroupMutation.mutate( | ||
| { productId: id, optionGroupId: group.id }, | ||
| { | ||
| onError: (error) => { | ||
| toast.error(t`Failed to remove option group`, { | ||
| description: error instanceof Error ? error.message : t`Unknown error`, | ||
| }); | ||
| } | ||
| } | ||
| ); | ||
| } | ||
| }} | ||
| > | ||
| <Trash2 className="h-4 w-4 text-destructive" /> | ||
| </Button> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Several lines exceed the 110-character Prettier limit.
Lines 306, 311–312 are well over the configured line length. Extract the click handler to a named function to improve readability and comply with the line-length guideline.
Proposed fix
<div className="col-span-1 flex justify-end">
<Button
size="icon"
variant="ghost"
- onClick={() => {
- if (confirm(t`Are you sure you want to remove this option group? This will affect all variants using this option.`)) {
- removeOptionGroupMutation.mutate(
- { productId: id, optionGroupId: group.id },
- {
- onError: (error) => {
- toast.error(t`Failed to remove option group`, {
- description: error instanceof Error ? error.message : t`Unknown error`,
- });
- }
- }
- );
- }
- }}
+ onClick={() => {
+ const msg = t`Are you sure you want to remove this option group? This will affect all variants using this option.`;
+ if (confirm(msg)) {
+ removeOptionGroupMutation.mutate(
+ {
+ productId: id,
+ optionGroupId: group.id,
+ },
+ {
+ onError: (error) => {
+ toast.error(
+ t`Failed to remove option group`,
+ {
+ description:
+ error instanceof Error
+ ? error.message
+ : t`Unknown error`,
+ },
+ );
+ },
+ },
+ );
+ }
+ }}
>As per coding guidelines, "Enforce line length of 110 characters (Prettier)".
🤖 Prompt for AI Agents
In
`@packages/dashboard/src/app/routes/_authenticated/_products/products_`.$id_.variants.tsx
around lines 301 - 322, The click handler passed to the Button violates the
110-character line length rule; extract it into a named function (e.g.,
handleRemoveOptionGroup) inside the component and call that function from
onClick to reduce line length and improve readability. In the new
handleRemoveOptionGroup function reference id, group.id and
removeOptionGroupMutation.mutate, perform the confirm check and the mutate call
with the same onError toast logic (using error instanceof Error ? error.message
: t`Unknown error`) so behavior remains identical, then replace the inline arrow
function in the Button's onClick with a call to handleRemoveOptionGroup.
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
1 participant
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.
** Description**
While testing variant option group management in the Dashboard, several UX and validation issues were observed that could lead to invalid or confusing product data. This PR fixes these issues in the Dashboard UI.
Affected area: Product Variant Option Groups in the Dashboard React/Admin UI.
Observed Issues
Option groups cannot be deleted
Previous option group data persists when adding a new group
Duplicate option values are allowed
Empty option group names are allowed
Fixes Implemented
Steps to Test
Open the Dashboard → navigate to a product with variants.
Add a new option group with a name and multiple values.
Test:
Verify all behave as expected in the UI.
Additional Context
dashboard_group_options.mp4