feat(core): Make ProductOption and ProductOptionGroup channel-aware #3703
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
All contributors have signed the CLA ✍️ ✅ |
WalkthroughThis PR implements channel-aware product options and option groups by adding many-to-many Channel relationships to both entities, extending the GraphQL schema with channel assignment/removal mutations and pagination support, updating services and resolvers with batch operations and channel-scoped filtering, adding comprehensive admin UI components and routes, and expanding internationalization across multiple languages. Changes
Sequence DiagramsequenceDiagram
participant Client as Admin Client
participant API as GraphQL API
participant Service as ProductOptionGroupService
participant Channel as ChannelService
participant DB as Database
rect rgba(200, 220, 255, 0.3)
Note over Client,DB: Assign Option Groups to Channel
Client->>API: assignProductOptionGroupsToChannel(groupIds, channelId)
API->>Service: assignProductOptionGroupsToChannel()
Service->>Channel: Validate permissions (UpdateCatalog)
Channel-->>Service: OK
Service->>DB: Load groups with options & channels
DB-->>Service: Groups[]
Service->>DB: Update many-to-many: groups→channels, options→channels
DB-->>Service: Success
Service-->>API: ProductOptionGroup[]
API-->>Client: Result
end
rect rgba(255, 220, 200, 0.3)
Note over Client,DB: Remove Option Groups from Channel (with In-Use Check)
Client->>API: removeProductOptionGroupsFromChannel(groupIds, channelId, force?)
API->>Service: removeProductOptionGroupsFromChannel()
Service->>Channel: Validate permissions (DeleteCatalog)
Channel-->>Service: OK
Service->>DB: Load groups with variants, check in-use
DB-->>Service: Groups[], usage counts
alt In use && not force
Service-->>API: ProductOptionGroupInUseError[]
else Not in use or force=true
Service->>DB: Remove from channel relationships
DB-->>Service: Success
Service-->>API: ProductOptionGroup[]
end
API-->>Client: Result
end
rect rgba(200, 255, 200, 0.3)
Note over Client,DB: Paginated List with Channel Scope
Client->>API: productOptionGroups(options: {take: 20, skip: 0})
API->>Service: findAll(ctx, options)
Service->>DB: Query with channel filter, pagination, relations
DB-->>Service: PaginatedList{items[], totalItems}
Service-->>API: Translated items
API-->>Client: ProductOptionGroupList
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: This PR encompasses substantial heterogeneous changes spanning entities, GraphQL schema, resolvers, services, admin UI components, E2E tests, and internationalization across 8 languages. While individual changes follow consistent patterns (channel relationships, pagination, batch operations), they require separate reasoning per component due to data model implications, error handling logic, and UI integration points. The many-to-many relationship refactoring, lifecycle cleanup mechanics, and bulk action coordination add logic density beyond simple refactoring. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 65
🔭 Outside diff range comments (3)
packages/core/src/api/schema/admin-api/product-option-group.api.graphql (1)
47-50: Incorrect translation input type used for option creation
CreateGroupOptionInput.translationsreferencesProductOptionGroupTranslationInput, but it should useProductOptionTranslationInput(group vs option). This slipped through earlier but will break localisation for options.-input CreateGroupOptionInput { - code: String! - translations: [ProductOptionGroupTranslationInput!]! -} +input CreateGroupOptionInput { + code: String! + translations: [ProductOptionTranslationInput!]! +}packages/dashboard/package.json (1)
47-133: Update react-resizable-panels to latest patch versionAll checked packages are on their latest stable releases and no known security advisories were found for
react-resizable-panels. A minor patch is available forreact-resizable-panels(v3.0.4).• packages/dashboard/package.json
- Bump
react-resizable-panelsfrom 3.0.3 to 3.0.4- "react-resizable-panels": "^3.0.3", + "react-resizable-panels": "^3.0.4",packages/dashboard/src/app/routes/_authenticated/_shipping-methods/components/shipping-eligibility-checker-selector.tsx (1)
22-25: Inconsistent props typing - missing Readonly wrapperUnlike other similar components in this PR, this component doesn't use the
Readonlywrapper for its props, which violates the coding guidelines.export function ShippingEligibilityCheckerSelector({ value, onChange, -}: ShippingEligibilityCheckerSelectorProps) { +}: Readonly<ShippingEligibilityCheckerSelectorProps>) {
🧹 Nitpick comments (26)
packages/core/src/api/schema/admin-api/product-option-group.api.graphql (3)
17-20: Document permissions & behaviour of the new channel mutationsThe schema docs don’t specify required permissions (e.g.
UpdateCatalog,DeleteCatalog) or the behaviour whenproductOptionGroupIdscontains groups already assigned / un-assigned to the given channel. This lack of spec makes it easy for downstream consumers (codegen, UI, integration tests) to make wrong assumptions.
Add a short description of required permissions and edge-case behaviour to the field descriptions.
69-73: Expose default forforceand clarify semantics
forceis nullable, but its default behaviour (assumedfalse) is not documented. Explicitly state the default in the description or make the field non-null with a default value via SDL:force: Boolean = falseto avoid surprises for API consumers.
83-83: Union name is inconsistent with mutation nameThe mutation is
removeProductOptionGroupsFromChannel(plural), but the union isRemoveProductOptionGroupFromChannelResult(singular). Aligning them avoids confusion in generated types:-union RemoveProductOptionGroupFromChannelResult = ProductOptionGroup | ProductOptionGroupInUseError +union RemoveProductOptionGroupsFromChannelResult = ProductOptionGroup | ProductOptionGroupInUseErrorpackages/core/e2e/product-option.e2e-spec.ts (1)
442-442: Consider using a more idiomatic assertion approach.Instead of
expect.fail(), consider using Vitest'sexpect().rejects.toThrow()pattern for cleaner async error assertions.- expect.fail('Should have thrown an error'); + // This line should not be reached + fail('Expected an error to be thrown');Alternatively, refactor the entire test to use:
await expect( adminClient.query(REMOVE_PRODUCT_OPTION_GROUPS_FROM_CHANNEL, { input: {...} }) ).rejects.toThrow('Items cannot be removed from the default Channel');docs/src/css/typography.css (1)
6-6: Large Google-Fonts request may impact CLS & FCPImporting four variable-axis families via CSS blocks render until they download. Consider:
• Moving the import tag to HTML
<head>withrel="preconnect"&display=swap, or
• Self-hosting subsetted fonts withfont-display: swap.This reduces layout shift and improves first paint.
docs/src/css/components.css (1)
45-52: Code-block max-height of 800 px may hide long examples without scroll cueLine 51 sets
max-height: 800pxbut no overflow rule. Add:-.limited-height-code-block pre.prism-code { - max-height: 800px; -} +.limited-height-code-block pre.prism-code { + max-height: 800px; + overflow: auto; +}docs/src/pages/index.module.css (2)
188-191: DuplicategapdeclarationLines 188-190 repeat
gap: 24px;. Remove the duplicate to avoid accidental future divergence.- flex-direction: column; - gap: 24px; - gap: 24px; + flex-direction: column; + gap: 24px;
192-199: Mixed indentation inside media queryThe
.docContainer& grid rules are indented one extra level (Lines 192-199), inconsistent with surrounding blocks and other files. Align for readability.- .docContainer { - padding: 0 1rem; - } + .docContainer { + padding: 0 1rem; + }CONTRIBUTING.md (3)
29-89: Clear getting started instructions with one consistency issue.The forking and syncing instructions are comprehensive and well-structured. However, there's an inconsistency in spelling variants.
Fix the spelling consistency by using either American or British English throughout:
-There are **three main ways** to keep your fork synchronized: +There are **three main ways** to keep your fork synchronised:Or change line 55 to use American spelling consistently with line 59.
310-398: Clear contribution standards with minor punctuation fixes needed.The guidelines effectively explain branching strategy, commit formats, and breaking changes. The use of Conventional Commits is well-documented with examples.
Fix American English punctuation for consistency:
-* **style** (formatting, missing semi colons, etc; no code change) +* **style** (formatting, missing semi colons, etc.; no code change)-* **chore** (updating build tasks etc; no production code change) +* **chore** (updating build tasks etc.; no production code change)
399-452: Thorough technical documentation with one punctuation fix.The sections on code generation, testing, and release process provide excellent technical depth for contributors.
Fix punctuation for consistency:
-e2e tests in those packages which feature e2e tests (core, elasticsearch-plugin, asset-server-plugin etc). +e2e tests in those packages which feature e2e tests (core, elasticsearch-plugin, asset-server-plugin etc.).packages/core/e2e/cache-service-redis.e2e-spec.ts (1)
99-121: Verify TTL test timing reliability.The TTL tests use hardcoded timeouts (1000ms, 1500ms) which could be flaky in slow CI environments. Consider making these timeouts configurable or adding buffer time.
- it('sets a key with ttl', async () => { - await cacheService.set('test-key1', 'test-value', { ttl: 1000 }); - const result = await cacheService.get('test-key1'); - expect(result).toBe('test-value'); - - await new Promise(resolve => setTimeout(resolve, 1500)); - - const result2 = await cacheService.get('test-key1'); - - expect(result2).toBeUndefined(); - }); + it('sets a key with ttl', async () => { + const ttl = 1000; + const waitTime = ttl + 500; // Add buffer for CI environments + await cacheService.set('test-key1', 'test-value', { ttl }); + const result = await cacheService.get('test-key1'); + expect(result).toBe('test-value'); + + await new Promise(resolve => setTimeout(resolve, waitTime)); + + const result2 = await cacheService.get('test-key1'); + + expect(result2).toBeUndefined(); + });packages/cli/e2e/cli-migrate.e2e-spec.ts (1)
102-109: Clarify expected behavior for no-arguments case.The test expects exit code 1 when no arguments are provided, but the comment suggests it "shows help". Consider adding a test comment explaining why this is the expected behavior.
it('should show help when no arguments provided', async () => { testProject = createTestProject('help-test'); + // CLI shows help in stderr but exits with error code when no command is specified // CLI shows help but exits with code 1 when no arguments provided const result = await testProject.runCliCommand([], { expectError: true }); expect(result.exitCode).toBe(1); expect(result.stderr).toContain('Usage:'); });packages/cli/src/commands/migrate/load-vendure-config-file.spec.ts (1)
96-134: Consider consolidating redundant test suites.The second
describeblock tests similar functionality to the first but focuses on parsing. Consider consolidating or clearly differentiating the test purposes.-describe('JSON parsing with comments', () => { +describe('Integration: JSON parsing with comments', () => { it('should successfully parse tsconfig.json content with /* in paths', () => { + // Integration test: verifies the complete flow from comment stripping to parsingpackages/cli/e2e/cli-test-utils.ts (1)
32-43: Consider making dependency versions configurable.The hardcoded version numbers ('3.3.7' for Vendure packages and '5.8.2' for TypeScript) may become outdated and cause test failures when new versions are released.
Consider making these configurable or reading from a shared configuration:
- dependencies: { - '@vendure/core': '3.3.7', - '@vendure/common': '3.3.7', - }, - devDependencies: { - typescript: '5.8.2', - }, + dependencies: { + '@vendure/core': process.env.VENDURE_VERSION || '3.3.7', + '@vendure/common': process.env.VENDURE_VERSION || '3.3.7', + }, + devDependencies: { + typescript: process.env.TYPESCRIPT_VERSION || '5.8.2', + },packages/dashboard/src/app/routes/_authenticated/_collections/components/collection-contents-preview-table.tsx (1)
66-70: Consider localizing the alert content.The alert title and description text are hardcoded in English. Consider using the
Transcomponent for better internationalization support.Apply this diff to add localization:
- <AlertTitle>Preview</AlertTitle> - <AlertDescription> - This is a preview of the collection contents based on the current filter settings. Once - you save the collection, the contents will be updated to reflect the new filter settings. - </AlertDescription> + <AlertTitle><Trans>Preview</Trans></AlertTitle> + <AlertDescription> + <Trans>This is a preview of the collection contents based on the current filter settings. Once + you save the collection, the contents will be updated to reflect the new filter settings.</Trans> + </AlertDescription>.github/workflows/scripts/setup-test-plugin.js (1)
14-22: Consider more robust plugin array parsing.The regex-based approach for finding and modifying the plugins array could be fragile with complex configurations or different formatting styles.
For better reliability, consider using an AST parser like
@babel/parserto parse the TypeScript file and modify the plugins array programmatically, though the current approach is sufficient for CI testing purposes.packages/dashboard/src/lib/components/data-input/select-with-options.tsx (1)
47-59: Consider safer placeholder handling for MultiSelect.The
String(placeholder)conversion might not handle complex React nodes properly. Consider providing a fallback or type check.- placeholder={placeholder ? String(placeholder) : 'Select options'} + placeholder={typeof placeholder === 'string' ? placeholder : 'Select options'}.github/workflows/build_test_dashboard.yml (1)
109-109: Fix trailing spaces.Static analysis detected trailing spaces on this line. Please remove them for cleaner code.
- - name: Copy files (Unix) + - name: Copy files (Unix)packages/dashboard/src/app/routes/_authenticated/_facets/components/add-facet-value-dialog.tsx (1)
82-86: Consider making language code configurableThe language code is hardcoded as
'en'which may limit internationalization capabilities. Consider making this configurable based on the current admin interface language or user preferences.translations: [ { - languageCode: 'en', + languageCode: i18n.locale || 'en', name: values.name, }, ],packages/dashboard/src/lib/components/shared/configurable-operation-selector.tsx (1)
69-77: Props should be typed as ReadonlyAccording to the coding guidelines, React component props objects should be typed as
Readonly<...>. The props are already destructured withReadonly<>in the function signature, but the interface definition should also reflect this pattern for consistency.-export interface ConfigurableOperationSelectorProps { +export interface ConfigurableOperationSelectorProps extends Readonly<{ /** Current selected configurable operation value */ value: ConfigurableOperationInputType | undefined; /** Callback function called when the selection changes */ onChange: (value: ConfigurableOperationInputType | undefined) => void; /** GraphQL document for querying available operations */ queryDocument: any; /** Unique key for the query cache */ queryKey: string; /** Dot-separated path to extract operations from query result (e.g., "paymentMethodHandlers") */ dataPath: string; /** Text to display on the selection button */ buttonText: string; /** Text to display when no operations are available (defaults to "No options found") */ emptyText?: string; -} +}>packages/dashboard/src/app/routes/_authenticated/_facets/components/facet-values-table.tsx (1)
30-31: Props interface should use Readonly wrapperThe component props should be wrapped with
Readonly<>at the interface level for consistency.-export interface FacetValuesTableProps { +export interface FacetValuesTableProps extends Readonly<{ facetId: string; registerRefresher?: (refresher: () => void) => void; -} +}>packages/dashboard/src/lib/components/ui/navigation-menu.tsx (1)
76-81: Remove hardcoded space characterThe space character
{" "}between{children}and theChevronDownIconcan cause inconsistent spacing. Consider using CSS for spacing instead.- {children}{" "} + {children} <ChevronDownIcon - className="relative top-[1px] ml-1 size-3 transition duration-300 group-data-[state=open]:rotate-180" + className="relative top-[1px] ml-1.5 size-3 transition duration-300 group-data-[state=open]:rotate-180" aria-hidden="true" />packages/dashboard/src/lib/components/data-input/configurable-operation-list-input.tsx (1)
143-149: Improve date handling robustnessThe DateTimeInput components receive values that might not be valid date strings, which could cause runtime errors.
Add date validation:
case 'date-form-input': + const dateValue = itemValue ? new Date(itemValue) : new Date(); + const isValidDate = !isNaN(dateValue.getTime()); return ( <DateTimeInput - value={itemValue ? new Date(itemValue) : new Date()} + value={isValidDate ? dateValue : new Date()} onChange={val => handleUpdateItem(index, val.toISOString())} disabled={readOnly} /> );Also applies to: 209-213
packages/dashboard/src/lib/components/shared/configurable-operation-arg-input.tsx (1)
171-174: Add localization for console warning messageWhile console warnings are typically not user-facing, it's good practice to have consistent messaging.
Consider using a translation function for the warning message:
- console.warn( - `Failed to load UI component ${uiComponent}, falling back to type-based input`, - ); + console.warn( + i18n.t('Failed to load UI component {component}, falling back to type-based input', + { component: uiComponent }), + );Note: This would require passing
i18nto the component or using theuseLinguihook.packages/dashboard/src/lib/components/ui/context-menu.tsx (1)
9-13: Conform to dashboard typing guideline – props should beReadonly<…>The coding guidelines for
packages/dashboard/src/**/*.{ts,tsx}mandateReadonly<…>for component props. Wrap the inferred type accordingly:-function ContextMenu({ ...props }: React.ComponentProps<typeof ContextMenuPrimitive.Root>) { +function ContextMenu({ + ...props +}: Readonly<React.ComponentProps<typeof ContextMenuPrimitive.Root>>) {Apply consistently to every wrapper in this file.
packages/dashboard/src/lib/components/shared/configurable-operation-arg-input.tsx
Show resolved
Hide resolved
packages/dashboard/src/lib/components/shared/configurable-operation-arg-input.tsx
Show resolved
Hide resolved
packages/dashboard/src/lib/components/shared/configurable-operation-arg-input.tsx
Show resolved
Hide resolved
|
Hope this getting merged soon. |
|
@michaelbromley any recommendations for a migration script? |
|
I have read the CLA Document and I hereby sign the CLA |
…tions for product option groups
…nce in creation mutation
…ers for injected dependencies
…ture for product option actions
asonnleitner
force-pushed
the
feat/channel-aware-options
branch
from
3e9ad76 to
bb005ae
Compare
… groups on variant deletion
…n Groups" for consistency
…responses and add force option
|
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
packages/dashboard/src/app/routes/_authenticated/_products/components/create-product-variants-dialog.tsx (1)
286-295: IncludecreateProductOptionMutationin disabled and loading states.The button's disabled condition and loading state checks are missing
createProductOptionMutation.isPending, even though this mutation is used inhandleCreateVariants(line 202). This could allow button clicks during option creation and fail to show the loading state.Apply this diff:
onClick={() => void handleCreateVariants()} disabled={ !variantData || createOptionGroupMutation.isPending || + createProductOptionMutation.isPending || addOptionGroupToProductMutation.isPending || createProductVariantsMutation.isPending } > {createOptionGroupMutation.isPending || + createProductOptionMutation.isPending || addOptionGroupToProductMutation.isPending || createProductVariantsMutation.isPending ? (packages/core/src/service/services/product.service.ts (1)
399-411: Harden lookup against soft-deleted groups.Guard against attaching a soft-deleted
ProductOptionGroup. AdddeletedAt: IsNull()to the lookup.- const optionGroup = await this.connection.getRepository(ctx, ProductOptionGroup).findOne({ - where: { id: optionGroupId }, - relations: ['channels'], - }); + const optionGroup = await this.connection.getRepository(ctx, ProductOptionGroup).findOne({ + where: { id: optionGroupId, deletedAt: IsNull() }, + relations: ['channels'], + });packages/dashboard/src/app/routes/_authenticated/_products/components/create-product-variants.tsx (1)
314-372: Fix variant ID generation to prevent collisions across groups.The collision risk is valid. Since
Option.idandOption.codeare optional and not globally unique across groups, two different options with the same code in different groups will produce identical IDs. For example, if Group1 and Group2 both contain an option with code "red", the current logic producesid: "red-red"regardless of which actual options are selected—causing collisions and violating React's key stability requirement.The proposed fix correctly addresses this by prefixing each option ID with its group dimension:
${currentGroup.id ?? currentIndex}:${option.id ?? option.code ?? option.name}. This ensures variant IDs encode group membership and remain stable and unique.- { name: currentGroup.name, value: option.name, id: option.id || option.code }, + { + name: currentGroup.name, + value: option.name, + id: `${currentGroup.id ?? currentIndex}:${option.id ?? option.code ?? option.name}`, + },Apply this fix at line 361 to stabilize variant IDs for both data integrity and React rendering at line 220.
packages/core/src/data-import/providers/importer/fast-importer.service.ts (1)
203-214: All five occurrences lack explicit falsy channel filtering; update all call sites to filter before unique().Found 5 call sites using the pattern without falsy channel filtering:
- Line 86:
p.channels = unique(...)- Line 119:
g.channels = unique(...)- Line 134:
po.channels = unique(...)- Line 170:
variant.channels = unique(...)- Line 203:
const assignedChannelIds = unique(...).map(...)Add
.filter()to exclude null/undefined channels across all 5 locations before theunique()call (e.g.,unique([this.defaultChannel, this.importCtx.channel].filter(Boolean), 'id')).packages/asset-server-plugin/e2e/graphql/generated-e2e-asset-server-plugin-types.ts (3)
878-883: Wrong translation input type on CreateProductOptionInputThe
translationsfield inCreateProductOptionInputuses the wrong type. Since this input is for creating aProductOption(as indicated by theproductOptionGroupIdfield), it should acceptProductOptionTranslationInputto describe the translations for the option itself—notProductOptionGroupTranslationInputwhich is for the parent group.Apply:
export type CreateProductOptionInput = { code: Scalars['String']['input']; customFields?: InputMaybe<Scalars['JSON']['input']>; productOptionGroupId: Scalars['ID']['input']; - translations: Array<ProductOptionGroupTranslationInput>; + translations: Array<ProductOptionTranslationInput>; };
848-852: Fix wrong translation input type on CreateGroupOptionInput across all generated types
CreateGroupOptionInput.translationsincorrectly usesProductOptionGroupTranslationInputbut should useProductOptionTranslationInput. This semantic mismatch breaks TypeScript type checking for clients creating product options.Apply to all affected files (asset-server-plugin, elasticsearch-plugin, payments-plugin, core, common, admin-ui, dev-server, and test plugins):
export type CreateGroupOptionInput = { code: Scalars['String']['input']; - translations: Array<ProductOptionGroupTranslationInput>; + translations: Array<ProductOptionTranslationInput>; };Also fix
CreateProductOptionInput(same issue—creates individual options, not groups):export type CreateProductOptionInput = { code: Scalars['String']['input']; productOptionGroupId: Scalars['ID']['input']; - translations: Array<ProductOptionGroupTranslationInput>; + translations: Array<ProductOptionTranslationInput>; };Regenerate all generated types after fixing the GraphQL schema to ensure consistency.
6518-6523: Fix incorrect translation input type on UpdateProductOptionInputUpdateProductOptionInput.translations is currently typed as
ProductOptionGroupTranslationInput, but should beProductOptionTranslationInput. CreateProductVariantOptionInput (line 905-908) usesProductOptionTranslationInputfor standalone options, and UpdateProductOptionInput lacks aproductOptionGroupIdfield—indicating it updates standalone options, not group options (which are handled separately by UpdateProductOptionGroupInput).export type UpdateProductOptionInput = { code?: InputMaybe<Scalars['String']['input']>; customFields?: InputMaybe<Scalars['JSON']['input']>; id: Scalars['ID']['input']; - translations?: InputMaybe<Array<ProductOptionGroupTranslationInput>>; + translations?: InputMaybe<Array<ProductOptionTranslationInput>>; };packages/core/src/service/services/product-option.service.ts (1)
169-199: Implement force behavior or remove unusedforceparameter in ProductOptionService.delete()The admin resolver passes args.force to this method (packages/core/src/api/resolvers/admin/product-option.resolver.ts:227) but the implementation in packages/core/src/service/services/product-option.service.ts (lines 169–199) never uses it. Either implement the intended force semantics (e.g., skip in-use checks and hard-delete when true) or remove the parameter and update callers (resolver and other callers).
packages/core/src/api/schema/admin-api/product-option-group.api.graphql (1)
61-70: Wrong translation input type for ProductOption translations.Option translations reference ProductOptionGroupTranslationInput. They should use ProductOptionTranslationInput to match the entity and avoid schema/DTO mismatches.
input CreateGroupOptionInput { code: String! - translations: [ProductOptionGroupTranslationInput!]! + translations: [ProductOptionTranslationInput!]! } input CreateProductOptionInput { productOptionGroupId: ID! code: String! - translations: [ProductOptionGroupTranslationInput!]! + translations: [ProductOptionTranslationInput!]! } input UpdateProductOptionInput { id: ID! code: String - translations: [ProductOptionGroupTranslationInput!] + translations: [ProductOptionTranslationInput!] }Also applies to: 72-76
packages/core/src/api/resolvers/admin/product-option.resolver.ts (1)
56-62: Pass @relations(...) to service.findOne() to honor requested fields.Currently ignores the computed relations, which can cause extra queries or missing fields.
- ): Promise<Translated<ProductOptionGroup> | undefined> { - return this.productOptionGroupService.findOne(ctx, args.id); + ): Promise<Translated<ProductOptionGroup> | undefined> { + return this.productOptionGroupService.findOne(ctx, args.id, relations);packages/common/src/generated-types.ts (2)
6869-6874: Bug: UpdateProductOptionInput uses group translation typeUpdateProductOptionInput.translations is typed as Array. It should be Array. Fix schema and regenerate.
Example schema patch:
-input UpdateProductOptionInput { - id: ID! - code: String - customFields: JSON - translations: [ProductOptionGroupTranslationInput!] -} +input UpdateProductOptionInput { + id: ID! + code: String + customFields: JSON + translations: [ProductOptionTranslationInput!] +}
889-894: Fix GraphQL schema: CreateProductOptionInput and UpdateProductOptionInput use incorrect translation typeThe bug is confirmed. Both
CreateProductOptionInputandUpdateProductOptionInputinpackages/core/src/api/schema/admin-api/product-option-group.api.graphqluseProductOptionGroupTranslationInput, but should useProductOptionTranslationInputto align withCreateProductVariantOptionInputand the ProductOption entity pattern. This error is reflected in three generated type files (packages/common/src, packages/admin-ui/src/lib/core/src/common, and packages/dev-server/test-plugins/reviews/ui). Update the GraphQL schema and regenerate types.
♻️ Duplicate comments (3)
packages/core/src/i18n/messages/de.json (1)
155-155: Fix entity/variables in PRODUCT_OPTION_IN_USE_ERRORThis key is for ProductOption, but the message references ProductOptionGroup and uses optionGroupCode. Use ProductOption and optionCode.
- "PRODUCT_OPTION_IN_USE_ERROR": "Die ProductOptionGroup \"{ optionGroupCode }\" kann nicht entfernt werden, solange sie von {variantCount, plural, one {einer Produktvariante} other {# Produktvarianten}} genutzt wird", + "PRODUCT_OPTION_IN_USE_ERROR": "Die ProductOption \"{ optionCode }\" kann nicht entfernt werden, solange sie von {variantCount, plural, one {einer Produktvariante} other {# Produktvarianten}} genutzt wird",packages/core/src/api/schema/admin-api/product-option-group.api.graphql (1)
79-81: Validate non-empty productOptionGroupIds.GraphQL can’t enforce non-empty arrays; reject empty lists in the service/DTO to avoid no-op calls.
Add validation (e.g., class-validator @ArrayMinSize(1)) or explicit check in the resolver/service.
Also applies to: 84-87
packages/core/src/service/services/product-option-group.service.ts (1)
303-309: Return query uses ctx.channelId instead of input.channelId.After assigning to input.channelId, querying with ctx.channelId can return empty results if operating from another channel. Query the target channel.
return this.connection .findByIdsInChannel( ctx, ProductOptionGroup, groupsToAssign.map(g => g.id), - ctx.channelId, + input.channelId, { relations: ['options'] }, )
🧹 Nitpick comments (53)
packages/core/src/api/config/graphql-custom-fields.ts (1)
759-763: Remove redundant type assertion.The explicit cast
(field.deprecated as string)on line 761 is unnecessary. TypeScript's control flow analysis already narrowsfield.deprecatedto typestringafter thetypeofcheck on line 759.Apply this diff to simplify the code:
if (typeof field.deprecated === 'string') { // Escape quotes in the deprecation reason - const escapedReason = (field.deprecated as string).replace(/"/g, '\\"'); + const escapedReason = field.deprecated.replace(/"/g, '\\"'); return `@deprecated(reason: "${escapedReason}")`; }packages/dashboard/src/app/routes/_authenticated/_products/components/create-product-variants-dialog.tsx (3)
126-126: Replaceanywith a proper type.The
defaultLanguageCodeparameter usesany, which bypasses type safety. Since this comes fromactiveChannel.defaultLanguageCode, use the proper type (likelystringor a LanguageCode enum type).Apply this diff:
function mapVariantToCreateInput( variant: VariantConfiguration['variants'][0], allOptionGroups: Array<{ id: string; name: string; options: Array<{ id: string; name: string }> }>, - defaultLanguageCode: any + defaultLanguageCode: string ) {
193-193: Remove redundant check.The condition
if (!group.id) continue;is redundant becauseexistingGroupswas already filtered to only include groups with IDs (line 158).Apply this diff:
// 3. Handle existing groups - create new options if needed for (const group of existingGroups) { - if (!group.id) continue; // Skip groups without ID (should not happen due to filter) const newOptions = group.options.filter(o => !o.id);
73-81: Wrap component props inReadonly<>to follow coding guidelines.As per coding guidelines for React components in the dashboard, props should be typed as
Readonly<...>.Apply this diff:
export function CreateProductVariantsDialog({ productId, productName, onSuccess, -}: { +}: Readonly<{ productId: string; productName: string; onSuccess?: () => void; -}) { +}>) {packages/dashboard/src/app/routes/_authenticated/_product-option-groups/components/product-options-table.tsx (1)
14-14: Use consistent quote style for imports.This import uses double quotes while all other imports in the file use single quotes. Maintain consistency.
Apply this diff:
-import { DeleteProductOptionsBulkAction } from "./product-option-group-bulk-actions.js"; +import { DeleteProductOptionsBulkAction } from './product-option-group-bulk-actions.js';packages/core/src/service/services/product.service.ts (3)
499-548: Micro-optimizations and resiliency (optional).
- Batch save: collect all orphan groups, update
product.optionGroupsonce, then save once.- Log more context (channelId, variant ids) to aid debugging and audit.
77-87: Minor: avoidasyncon onModuleInit and alias rxjs operator.
onModuleInitdoesn'tawait; removeasync.- Alias
filterto avoid confusion with similarly named helpers.- import { filter } from 'rxjs/operators'; + import { filter as rxFilter } from 'rxjs/operators'; ... - async onModuleInit() { + onModuleInit() { - this.eventBus - .ofType(ProductVariantEvent) - .pipe(filter(event => event.type === 'deleted')) + this.eventBus + .ofType(ProductVariantEvent) + .pipe(rxFilter(event => event.type === 'deleted'))Also applies to: 14-14
526-537: Delegate removal to ProductOptionGroupService to avoid duplicating product mutation logic.The method
ProductOptionGroupService.deleteGroupAndOptionsFromProduct()already handles removing the option group from the product (line 223 in product-option-group.service.ts). Duplicating this logic here risks inconsistency if invariants or side-effects are added later.Note:
deleteGroupAndOptionsFromProduct()emitsProductOptionGroupEvent(notProductOptionGroupChangeEvent), so the suggested event emission in the review diff would not cause double emission but would emit a different, necessary event on successful deletion.packages/dashboard/src/app/routes/_authenticated/_products/components/create-product-variants.tsx (8)
104-106: Avoid JSON.stringify in deps; rely on reference or deep-compare.JSON.stringify in deps is expensive and brittle. Depend on the array reference (OptionGroupSearchInput already returns new arrays) or use a deep-compare hook.
Apply this diff:
- const variants = useMemo( - () => generateVariants(optionGroups), - [JSON.stringify(optionGroups)], - ); + const variants = useMemo(() => generateVariants(optionGroups), [optionGroups]);
110-138: Minor: trim defensive checks.variant has static shape; the typeof/object checks add noise. Safe to remove for clarity.
- variants.forEach(variant => { - if (variant && typeof variant === 'object') { - const formVariant = formVariants[variant.id]; - if (formVariant) { - activeVariants.push({ + variants.forEach(variant => { + const formVariant = formVariants[variant.id]; + if (formVariant) { + activeVariants.push({ enabled: formVariant.enabled ?? true, sku: formVariant.sku ?? '', price: formVariant.price ?? '', stock: formVariant.stock ?? '', options: variant.options, - }); - } - } + }); + } });
141-158: Prune stale variant form entries when variant set shrinks.The effect adds missing keys but never removes obsolete ones, leaving dead state around.
useEffect(() => { // Initialize any new variants with default values const currentVariants = form.getValues().variants || {}; - const updatedVariants = { ...currentVariants }; + const updatedVariants = { ...currentVariants }; + const validIds = new Set(variants.map(v => v.id)); variants.forEach(variant => { if (!updatedVariants[variant.id]) { updatedVariants[variant.id] = { enabled: true, sku: '', price: '', stock: '', }; } }); - setValue('variants', updatedVariants); + // Drop removed variant keys + const pruned = Object.fromEntries( + Object.entries(updatedVariants).filter(([id]) => validIds.has(id)) + ); + setValue('variants', pruned, { shouldDirty: false }); }, [variants, form, setValue]);
84-88: Add staleTime to useQuery (per guidelines).Follow the app’s TanStack Query pattern to reduce refetch churn.
const { data: stockLocationsResult } = useQuery({ queryKey: ['stockLocations'], - queryFn: () => api.query(getStockLocationsDocument, { options: { take: 100 } }), + staleTime: 5 * 60 * 1000, + queryFn: () => api.query(getStockLocationsDocument, { options: { take: 100 } }), });As per coding guidelines.
59-66: Localize validation messages and placeholders.Zod messages and the “SKU” placeholder should be localized.
+import { t } from '@lingui/macro'; +import { useLingui } from '@lingui/react'; @@ -const variantSchema = z.object({ - enabled: z.boolean().default(true), - sku: z.string().min(1, { message: 'SKU is required' }), - price: z.string().refine(val => !isNaN(Number(val)) && Number(val) >= 0, { - message: 'Price must be a positive number', - }), - stock: z.string().refine(val => !isNaN(Number(val)) && parseInt(val, 10) >= 0, { - message: 'Stock must be a non-negative integer', - }), -}); +const variantSchema = z.object({ + enabled: z.boolean().default(true), + sku: z.string().min(1, { message: t`SKU is required` }), + price: z.string().refine(val => !isNaN(Number(val)) && Number(val) >= 0, { + message: t`Price must be a positive number`, + }), + stock: z.string().refine(val => !isNaN(Number(val)) && parseInt(val, 10) >= 0, { + message: t`Stock must be a non-negative integer`, + }), +}); @@ export function CreateProductVariants({ currencyCode = 'USD', onChange, }: Readonly<CreateProductVariantsProps>) { + const { i18n } = useLingui(); @@ - <Input {...field} placeholder="SKU" /> + <Input {...field} placeholder={i18n._(t`SKU`)} /> @@ - placeholder="0.00" + placeholder="0.00"As per coding guidelines.
Also applies to: 248-255, 270-274
245-256: Consider FormFieldWrapper for consistency.If the project standardizes on FormFieldWrapper, refactor these fields to match the shared pattern for labels, help text, and errors.
As per coding guidelines.
Also applies to: 259-281, 283-301
314-372: Optional: deterministic ordering.If options can be returned in arbitrary order, sort by display name or code before combination to keep row order stable between renders.
-const currentGroup = optionGroups[currentIndex]; +const currentGroup = optionGroups[currentIndex]; +// optionally enforce stable ordering +// currentGroup.options = [...currentGroup.options].sort((a, b) => (a.name || '').localeCompare(b.name || ''));
29-44: Minor: remove unused type alias.VariantForm appears unused.
-type VariantForm = z.infer<typeof variantSchema>;Also applies to: 72-74
packages/dashboard/src/app/common/duplicate-bulk-action.tsx (1)
75-85: Localize all user-facing messages (toasts and fallback strings).Several hardcoded English strings bypass i18n (Unknown error, error toast). Localize via i18n.t to meet dashboard guidelines.
As per coding guidelines.
- result.duplicateEntity.message || - result.duplicateEntity.duplicationError || - 'Unknown error'; + result.duplicateEntity.message || + result.duplicateEntity.duplicationError || + i18n.t('Unknown error'); ... - results.errors.push( - `${entityName} ${entity.name || entity.id}: ${error instanceof Error ? error.message : 'Unknown error'}`, - ); + results.errors.push( + `${entityName} ${entity.name || entity.id}: ${ + error instanceof Error ? error.message : i18n.t('Unknown error') + }`, + ); ... - toast.success( - i18n.t(`Successfully duplicated ${results.success} ${entityName.toLowerCase()}s`), - ); + toast.success( + i18n.t('Successfully duplicated {count} {entity}s', { + count: results.success, + entity: entityName.toLowerCase(), + }), + ); ... - toast.error( - `Failed to duplicate ${results.failed} ${entityName.toLowerCase()}s: ${errorMessage}`, - ); + toast.error( + i18n.t('Failed to duplicate {count} {entity}s: {message}', { + count: results.failed, + entity: entityName.toLowerCase(), + message: errorMessage, + }), + );Also applies to: 93-105
packages/dashboard/src/app/routes/_authenticated/_product-option-groups/components/edit-product-option.tsx (3)
48-51: Add staleTime and expose loading state.Follow the standard useQuery pattern and capture isLoading for correct conditional rendering.
As per coding guidelines.
- const { data: productOptions } = useQuery({ - queryKey: ['productOptions', productOptionId], - queryFn: () => api.query(productOptionsDocument, { options: { filter: { id: { eq: productOptionId } } } }), - }); + const { data: productOptions, isLoading } = useQuery({ + queryKey: ['productOptions', productOptionId], + staleTime: 30_000, + queryFn: () => + api.query(productOptionsDocument, { + options: { filter: { id: { eq: productOptionId } } }, + }), + });
97-99: Localize headings.Wrap headings/description in Trans to comply with localization rules.
As per coding guidelines.
- <h4 className="font-medium leading-none">Edit Product Option</h4> - <p className="text-sm text-muted-foreground">Update the name and code of this product option.</p> + <h4 className="font-medium leading-none"> + <Trans>Edit Product Option</Trans> + </h4> + <p className="text-sm text-muted-foreground"> + <Trans>Update the name and code of this product option.</Trans> + </p>
100-119: Prefer FormFieldWrapper for consistency with dashboard forms.Using FormFieldWrapper yields consistent labels, help text, and errors across the app.
I can provide a concrete refactor if desired. As per coding guidelines.
packages/core/src/api/schema/common/product-option-group.type.graphql (1)
8-9: Deprecate non-paginated options to steer clients to optionListAvoid large payloads and N+1s by guiding clients.
Apply:
type ProductOptionGroup implements Node { id: ID! createdAt: DateTime! updatedAt: DateTime! languageCode: LanguageCode! code: String! name: String! - options: [ProductOption!]! + options: [ProductOption!]! @deprecated(reason: "Use optionList for pagination, filtering and sorting") optionList(options: ProductOptionListOptions): ProductOptionList! translations: [ProductOptionGroupTranslation!]! }packages/core/src/i18n/messages/de.json (1)
186-186: Typo: “Produktvariantem” → “Produktvarianten”Minor German plural fix.
- "facet-force-deleted": "Die Facette wurde gelöscht und ihre FacetValues wurden von {products, plural, =0 {} one {einem Produkt} other {# Produkten}}{both, select, both { und } single {} other {}}{variants, plural, =0 {} one {einer Produktvariante} other {# Produktvariantem}} entfernt", + "facet-force-deleted": "Die Facette wurde gelöscht und ihre FacetValues wurden von {products, plural, =0 {} one {einem Produkt} other {# Produkten}}{both, select, both { und } single {} other {}}{variants, plural, =0 {} one {einer Produktvariante} other {# Produktvarianten}} entfernt",packages/dashboard/src/app/routes/_authenticated/_products/components/option-search.tsx (2)
24-31: Type props as Readonly for React componentsConform to project guideline: component props should be Readonly<...>.
-interface OptionSearchProps { +type OptionSearchProps = Readonly<{ groupId?: string; groupName: string; onSelect: (option: Option) => void; selectedOptions?: Option[]; placeholder?: string; disabled?: boolean; -} +}>;
58-70: Add staleTime and search by code OR nameFollow TanStack Query pattern (include staleTime). Also search by code as well as name using OR operator to match UX filter.
- const { data, isLoading } = useQuery({ - queryKey: ['productOptions', groupId, debouncedSearch], - queryFn: () => api.query(searchProductOptionsDocument, { - options: { - filter: { - groupId: { eq: groupId }, - name: { contains: debouncedSearch }, - }, - take: 30 - } - }), - enabled: !!groupId - }); + const { data, isLoading } = useQuery({ + queryKey: ['productOptions', groupId, debouncedSearch], + staleTime: 30_000, + queryFn: () => + api.query(searchProductOptionsDocument, { + options: { + filterOperator: 'OR', + filter: { + groupId: { eq: groupId }, + name: debouncedSearch ? { contains: debouncedSearch } : undefined, + code: debouncedSearch ? { contains: debouncedSearch } : undefined, + }, + take: 30, + }, + }), + enabled: !!groupId, + });packages/dashboard/src/app/routes/_authenticated/_product-option-groups/product-option-groups_.$id.tsx (1)
52-65: Avoid shadowing “entity” from upper scopeRename the parameter to prevent eslint no-shadow.
- setValuesForUpdate: entity => { + setValuesForUpdate: group => { return { - id: entity.id, - code: entity.code, - translations: entity.translations.map(translation => ({ + id: group.id, + code: group.code, + translations: group.translations.map(translation => ({ id: translation.id, languageCode: translation.languageCode, name: translation.name, customFields: (translation as any).customFields, })), options: [], - customFields: entity.customFields, + customFields: group.customFields, }; },packages/dashboard/src/app/routes/_authenticated/_product-option-groups/components/product-option-group-bulk-actions.tsx (1)
60-97: Optional: consolidate success/error toastsYou already surface per-item errors; consider passing a localized successMessage/errorMessage to RemoveFromChannelBulkAction for consistent UX.
packages/core/src/i18n/messages/ru.json (1)
92-99: Fill missing strings for new channel-aware flows.Keys like product-option-group-already-assigned and related channel messages are empty. These surface in the new mutations and should be localized to avoid English fallbacks.
Would you like me to propose RU translations for the empty keys in this block? (Based on learnings)
packages/dashboard/src/app/routes/_authenticated/_products/components/option-group-search-input.tsx (2)
116-124: Add accessible labels to icon-only buttons.Icon-only buttons need aria-labels for screen readers.
-<Button +<Button type="button" variant="ghost" size="sm" onClick={() => handleRemoveGroup(groupIndex)} + aria-label="Remove option group" > <X className="h-4 w-4" /> </Button> -<Button +<Button type="button" variant="ghost" size="sm" className="h-4 w-4 p-0 ml-1 hover:bg-secondary-foreground/20" onClick={() => handleRemoveOption(groupIndex, optionIndex)} + aria-label="Remove option" > <X className="h-3 w-3" /> </Button>Also applies to: 144-153
209-213: Copy tweak: button label mismatched action.When no groups exist, the button opens “Add Option Group”. Label should say “Add First Option Group”.
- <Trans>Add First Option</Trans> + <Trans>Add First Option Group</Trans>packages/core/src/i18n/messages/pt_BR.json (1)
154-156: Consistency: provide message for PRODUCT_OPTION_IN_USE_ERROR or drop placeholder.Currently empty; this error is surfaced by new channel-aware flows.
Proposed PT-BR:
-"PRODUCT_OPTION_IN_USE_ERROR": "", +"PRODUCT_OPTION_IN_USE_ERROR": "Não é possível remover a opção, pois ela é usada por {productVariantCount, plural, one {1 Variante de Produto} other {# Variantes de Produto}}",packages/core/src/api/schema/admin-api/product-option-group.api.graphql (1)
18-19: Also validate non-empty ids for bulk deletions.deleteProductOptionGroups/deleteProductOptions accept [ID!]! but may receive empty arrays. Reject early for clarity and consistent error handling.
Also applies to: 30-31
packages/dashboard/src/app/routes/_authenticated/_product-option-groups/product-option-groups.graphql.ts (1)
115-126: Surface structured error fields from union.removeProductOptionGroupsFromChannel returns ProductOptionGroup | ProductOptionGroupInUseError. Capture errorCode and the in-use details to render actionable toasts.
export const removeProductOptionGroupsFromChannelDocument = graphql(` mutation RemoveProductOptionGroupsFromChannel($input: RemoveProductOptionGroupsFromChannelInput!) { removeProductOptionGroupsFromChannel(input: $input) { ... on ProductOptionGroup { id } - ... on ErrorResult { - message - } + ... on ErrorResult { + errorCode + message + } + ... on ProductOptionGroupInUseError { + optionGroupCode + productCount + variantCount + } } } `);packages/core/e2e/graphql/generated-e2e-admin-types.ts (4)
2737-2738: Channel assignment/removal surfaces — union result and inputs look right; add __typename in union queries.Schema types are fine. For client robustness, include
__typenamewhen selecting union results (esp. removal results) to simplify narrowing.Also applies to: 2947-2948, 3114-3116, 3222-3224, 3542-3544, 5511-5517
4577-4649: Filter/sort/list types and paginated queries — LGTM; prefer optionList in queries to avoid large payloads.Types look consistent. In tests/queries returning many options, consider using
optionListwith pagination overoptionsto limit response size.Also applies to: 4673-4699, 5067-5068, 5241-5246
12154-12206: Generated TS types for new queries/mutations — OK; consider selecting __typename on unions.Types align with schema. Recommend adding
__typenameto union selections in the corresponding source .graphql and regenerating.
36890-37166: Add__typenameto union selections in e2e documents for safer narrowing.Selecting
__typenameon unions prevents fragile ad‑hoc checks in tests/clients.Apply to the source GraphQL documents (then regenerate); shown here as diff against the generated DocumentNodes for illustration:
@@ export const AssignProductOptionGroupsToChannelDocument = { @@ - selections: [ + selections: [ + { kind: 'Field', name: { kind: 'Name', value: '__typename' } }, { kind: 'Field', name: { kind: 'Name', value: 'id' } }, { kind: 'Field', name: { kind: 'Name', value: 'name' } }, ], @@ export const RemoveProductOptionGroupsFromChannelDocument = { @@ - selections: [ + selections: [ { kind: 'InlineFragment', typeCondition: { kind: 'NamedType', name: { kind: 'Name', value: 'ProductOptionGroup' }, }, selectionSet: { kind: 'SelectionSet', - selections: [ + selections: [ + { kind: 'Field', name: { kind: 'Name', value: '__typename' } }, { kind: 'Field', name: { kind: 'Name', value: 'id' } }, { kind: 'Field', name: { kind: 'Name', value: 'name' } }, ], }, }, { kind: 'InlineFragment', typeCondition: { kind: 'NamedType', name: { kind: 'Name', value: 'ProductOptionGroupInUseError' }, }, selectionSet: { kind: 'SelectionSet', - selections: [ + selections: [ + { kind: 'Field', name: { kind: 'Name', value: '__typename' } }, { kind: 'Field', name: { kind: 'Name', value: 'errorCode' } }, { kind: 'Field', name: { kind: 'Name', value: 'message' } }, { kind: 'Field', name: { kind: 'Name', value: 'optionGroupCode' } }, { kind: 'Field', name: { kind: 'Name', value: 'productCount' } }, { kind: 'Field', name: { kind: 'Name', value: 'variantCount' } }, ], }, }, ], @@ export const GetProductWithOptionGroupsDocument = { @@ - selections: [ + selections: [ + { kind: 'Field', name: { kind: 'Name', value: '__typename' } }, { kind: 'Field', name: { kind: 'Name', value: 'id' } }, { kind: 'Field', name: { kind: 'Name', value: 'name' } }, { kind: 'Field', name: { kind: 'Name', value: 'optionGroups' }, selectionSet: { kind: 'SelectionSet', - selections: [ + selections: [ + { kind: 'Field', name: { kind: 'Name', value: '__typename' } }, { kind: 'Field', name: { kind: 'Name', value: 'id' } }, { kind: 'Field', name: { kind: 'Name', value: 'code' } }, { kind: 'Field', name: { kind: 'Name', value: 'name' } }, ], }, }, ],packages/core/src/service/services/product-option-group.service.ts (1)
292-301: Minor: unnecessary generic in Promise.all.Promise.all(...) adds no type value here. Remove angle type.
- await Promise.all<any>([ + await Promise.all([ ...groupsToAssign.map(async group => {packages/core/src/api/resolvers/admin/product-option.resolver.ts (2)
112-119: Unify create() call style for clarity.You load the group, then call create() with the ID, whereas batch create uses the group object. Prefer one style.
- const productOptionGroup = await this.productOptionGroupService.findOne( + const productOptionGroup = await this.productOptionGroupService.findOne( ctx, input.productOptionGroupId, ); if (!productOptionGroup) { throw new EntityNotFoundError('ProductOptionGroup', input.productOptionGroupId); } - return this.productOptionService.create(ctx, input.productOptionGroupId, input); + return this.productOptionService.create(ctx, productOptionGroup, input);
158-164: Remove ts-ignore; typing should be valid.Avoid suppressing type checks here.
- // @ts-ignore - is ok - return Promise.all(input.map(option => this.productOptionService.update(ctx, option))); + return Promise.all(input.map(option => this.productOptionService.update(ctx, option)));packages/core/e2e/graphql/generated-e2e-shop-types.ts (2)
7219-7231: Remove duplicatetotalfield in GET_ACTIVE_ORDER_WITH_PRICE_DATA queryThe
totalfield is selected twice in the query definition. Remove the duplicate atpackages/core/e2e/graphql/shop-definitions.ts:365to clean up the generated types.
2663-2663: Resolver correctly handles channel scoping and soft-delete filtering; @deprecated suggestion not yet implementedVerification confirms the resolver at
packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.tsenforces channel scoping (viactx.channelIdin the service query builder) and filters soft-deleted options withoptions.filter(o => !o.deletedAt). However, theoptionsfield in the schema (packages/core/src/api/schema/common/product-option-group.type.graphql) is not marked@deprecated. Consider adding@deprecated(reason: "Use optionList for pagination support")to the schema to guide clients toward the new paginated field.packages/admin-ui/src/lib/core/src/common/generated-types.ts (11)
890-891: Allow creating options with group creation.Good addition. Consider wiring the Admin UI “Create group” flow to optionally include initial options.
1354-1355: Expose deprecation on CustomField interface.Suggest surfacing “Deprecated” badges/tooltips in UI where custom fields are displayed.
2851-2852: Bulk createProductOptions mutation added.Prefer this in UI when creating many options to reduce round-trips.
3093-3098: Bulk updateProductOptions mutation added.Adopt for mass edits in Admin UI forms.
4910-4920: ProductOptionFilterParameter added.Great for server-side filtering. Hook this up in list/search UIs to reduce payloads.
4930-4939: ProductOptionGroup.optionList + args introduced.Prefer optionList over the raw options array to avoid N-size payloads and enable pagination. Plan UI migration.
5012-5038: ProductOptionList + list options/sort added.All good; adopt in group detail pages for scalable option rendering.
5631-5637: Args for productOptionGroups/productOptions queries.OK; wire pagination/filtering in UI list views.
5926-5932: RemoveProductOptionGroupFromChannelResult union + input.Union enables per-item success/error; surface productCount/variantCount in UI dialogs. Based on learnings.
8167-8171: GetProductOptionGroups query uses options array.Recommend switching to optionList { items { … } totalItems } to avoid loading all options for large groups; regenerate types after updating the .graphql source.
9606-9606: GetProductOptionGroupsDocument added.Consider updating the source query to use optionList + pagination as above, then re-run codegen.
packages/common/src/generated-types.ts (1)
4835-4845: Optional: add channel-aware filtersConsider adding channelId (and possibly channelCode) to ProductOptionFilterParameter and ProductOptionGroupFilterParameter so SuperAdmins can query across channels explicitly. Current queries are channel-scoped, but explicit filters improve admin tooling and bulk ops.
Also applies to: 4866-4875, 4943-4954, 4956-4963
| /** Create one or more ProductOptions */ | ||
| createProductOptions: Array<ProductOption>; |
There was a problem hiding this comment.
🧩 Analysis chain
Bulk create/update/delete additions — check test coverage for force and cross‑channel constraints.
Please verify e2e tests cover:
- delete with/without
force - update/create blocked across channels
- soft‑deleted options excluded during assignment.
Also applies to: 2858-2863, 3019-3019, 3024-3024, 3375-3388, 3724-3726
🏁 Script executed:
#!/bin/bash
# Quick scan for force semantics and cross-channel tests.
rg -nC3 '\bforce\b|assignProductOptionGroupsToChannel|removeProductOptionGroupsFromChannel|createProductOptions|updateProductOptions|deleteProductOption(Group|s)\b' packages/core/e2eLength of output: 49805
🏁 Script executed:
#!/bin/bash
# Check for specific test cases for create/update/delete ProductOptions and soft-delete handling
rg -nA10 'describe|it\(' packages/core/e2e/product-option.e2e-spec.ts | head -150Length of output: 6698
🏁 Script executed:
#!/bin/bash
# Search for soft-delete related tests
rg -nC5 'soft.*delete|deleted|softDelete' packages/core/e2e/product-option.e2e-spec.tsLength of output: 1122
🏁 Script executed:
#!/bin/bash
# Look for createProductOptions and updateProductOptions test cases
rg -nB2 -A8 'createProductOptions|updateProductOptions' packages/core/e2e/product-option.e2e-spec.ts | grep -A8 'it\('Length of output: 23
🏁 Script executed:
#!/bin/bash
# Search for plural forms and test structure around bulk operations
rg -n 'createProductOptions|updateProductOptions|deleteProductOptions' packages/core/e2e/product-option.e2e-spec.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get the full test describe blocks to see structure
sed -n '34,700p' packages/core/e2e/product-option.e2e-spec.ts | grep -E "^\s*(describe|it)\(" | head -40Length of output: 1557
🏁 Script executed:
#!/bin/bash
# Check what mutations are actually defined in the test queries
rg -n 'CREATE_PRODUCT_OPTION|UPDATE_PRODUCT_OPTION|DELETE_PRODUCT_OPTION' packages/core/e2e/product-option.e2e-spec.ts | head -20Length of output: 833
🏁 Script executed:
#!/bin/bash
# Search entire e2e directory for plural forms of these mutations
rg -r 'createProductOptions|updateProductOptions|deleteProductOptions' packages/core/e2e/ --type tsLength of output: 732
🏁 Script executed:
#!/bin/bash
# Check if these mutations are tested anywhere in the codebase
rg -r 'createProductOptions|updateProductOptions|deleteProductOptions' packages/core/ --type ts -l | grep -i testLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the generated types to confirm these mutations exist and their signatures
sed -n '2788,2790p; 3023,3025p; 2862,2864p' packages/core/e2e/graphql/generated-e2e-admin-types.tsLength of output: 485
Bulk mutation test coverage is missing — add e2e tests for createProductOptions, updateProductOptions, and deleteProductOptions mutations before production use.
The bulk mutations are defined in the GraphQL schema but have no corresponding e2e test cases in packages/core/e2e/product-option.e2e-spec.ts. Currently tested:
removeProductOptionGroupsFromChannelwithforce: true/false(line 592, 610 of product-option.e2e-spec.ts)- Channel cross-constraint validation with error handling
- Soft-deleted options excluded from query results (line 268)
Not tested:
createProductOptionsbulk operation with cross-channel constraintsupdateProductOptionsbulk operation across channelsdeleteProductOptionswithforcesemantics- Soft-deleted options excluded during assignment/bulk operations
Recommend adding dedicated test cases for the plural mutations before merging.
| it('addOptionGroupToProduct allows sharing option groups between products', async () => { | ||
| // First verify the option group is already on product T_1 | ||
| const { product: laptop } = await adminClient.query< | ||
| Codegen.GetProductWithVariantsQuery, | ||
| Codegen.GetProductWithVariantsQueryVariables | ||
| >(GET_PRODUCT_WITH_VARIANTS, { id: 'T_1' }); | ||
|
|
||
| expect(laptop?.optionGroups.some(og => og.id === 'T_1')).toBe(true); | ||
|
|
||
| // Now add the same option group to another product | ||
| const result = await adminClient.query< | ||
| Codegen.AddOptionGroupToProductMutation, | ||
| Codegen.AddOptionGroupToProductMutationVariables | ||
| >(ADD_OPTION_GROUP_TO_PRODUCT, { | ||
| optionGroupId: 'T_1', | ||
| productId: 'T_2', | ||
| }); | ||
|
|
||
| expect(result.addOptionGroupToProduct.optionGroups.length).toBe(1); | ||
| expect(result.addOptionGroupToProduct.optionGroups[0].id).toBe('T_1'); | ||
| }); |
There was a problem hiding this comment.
Fix the test expectation: product T_2 likely has existing option groups.
The pipeline failure indicates this test expects 1 option group but receives 2. The test adds option group T_1 to product T_2, but product T_2 appears to already have one option group. The assertion at line 1297 should verify that:
- The optionGroups array includes the newly added group
T_1 - The total count matches the expected number (which is 2, not 1)
Apply this diff to fix the test:
- expect(result.addOptionGroupToProduct.optionGroups.length).toBe(1);
- expect(result.addOptionGroupToProduct.optionGroups[0].id).toBe('T_1');
+ expect(result.addOptionGroupToProduct.optionGroups.length).toBe(2);
+ expect(result.addOptionGroupToProduct.optionGroups.some(og => og.id === 'T_1')).toBe(true);Alternatively, verify what option groups product T_2 has initially and adjust the expectation accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('addOptionGroupToProduct allows sharing option groups between products', async () => { | |
| // First verify the option group is already on product T_1 | |
| const { product: laptop } = await adminClient.query< | |
| Codegen.GetProductWithVariantsQuery, | |
| Codegen.GetProductWithVariantsQueryVariables | |
| >(GET_PRODUCT_WITH_VARIANTS, { id: 'T_1' }); | |
| expect(laptop?.optionGroups.some(og => og.id === 'T_1')).toBe(true); | |
| // Now add the same option group to another product | |
| const result = await adminClient.query< | |
| Codegen.AddOptionGroupToProductMutation, | |
| Codegen.AddOptionGroupToProductMutationVariables | |
| >(ADD_OPTION_GROUP_TO_PRODUCT, { | |
| optionGroupId: 'T_1', | |
| productId: 'T_2', | |
| }); | |
| expect(result.addOptionGroupToProduct.optionGroups.length).toBe(1); | |
| expect(result.addOptionGroupToProduct.optionGroups[0].id).toBe('T_1'); | |
| }); | |
| it('addOptionGroupToProduct allows sharing option groups between products', async () => { | |
| // First verify the option group is already on product T_1 | |
| const { product: laptop } = await adminClient.query< | |
| Codegen.GetProductWithVariantsQuery, | |
| Codegen.GetProductWithVariantsQueryVariables | |
| >(GET_PRODUCT_WITH_VARIANTS, { id: 'T_1' }); | |
| expect(laptop?.optionGroups.some(og => og.id === 'T_1')).toBe(true); | |
| // Now add the same option group to another product | |
| const result = await adminClient.query< | |
| Codegen.AddOptionGroupToProductMutation, | |
| Codegen.AddOptionGroupToProductMutationVariables | |
| >(ADD_OPTION_GROUP_TO_PRODUCT, { | |
| optionGroupId: 'T_1', | |
| productId: 'T_2', | |
| }); | |
| expect(result.addOptionGroupToProduct.optionGroups.length).toBe(2); | |
| expect(result.addOptionGroupToProduct.optionGroups.some(og => og.id === 'T_1')).toBe(true); | |
| }); |
🤖 Prompt for AI Agents
In packages/core/e2e/product.e2e-spec.ts around lines 1279 to 1299, the test
currently asserts that product T_2 has exactly 1 option group after adding T_1,
but T_2 already has an existing option group so the expected length should be 2
(or, more robustly, computed as initialCount + 1). Update the assertions to (a)
confirm that the optionGroups array includes an entry with id 'T_1' and (b)
assert the total count equals the prior count plus one (or explicitly 2 if you
know T_2 starts with one), e.g., fetch product T_2's optionGroups before the
mutation to determine initialCount and then assert
result.addOptionGroupToProduct.optionGroups.length === initialCount + 1.
| @Transaction() | ||
| @Mutation() | ||
| @Allow(Permission.CreateCatalog, Permission.CreateProduct) | ||
| async createProductOptions( | ||
| @Ctx() ctx: RequestContext, | ||
| @Args() args: MutationCreateProductOptionsArgs, | ||
| ): Promise<Array<Translated<ProductOption>>> { | ||
| const { input } = args; | ||
| const productOptionGroupId = input[0]?.productOptionGroupId; | ||
| const productOptionGroup = await this.productOptionGroupService.findOne(ctx, productOptionGroupId); | ||
| if (!productOptionGroup) { | ||
| throw new EntityNotFoundError('ProductOptionGroup', productOptionGroupId); | ||
| } | ||
| const productOptions: Array<Translated<ProductOption>> = []; | ||
| for (const productOption of input) { | ||
| const res = await this.productOptionService.create(ctx, productOptionGroup, productOption); | ||
| productOptions.push(res); | ||
| } | ||
| return productOptions; | ||
| } |
There was a problem hiding this comment.
Validate batch input and enforce single target group.
This assumes all inputs share the same productOptionGroupId and doesn’t guard against an empty array.
async createProductOptions(
@Ctx() ctx: RequestContext,
@Args() args: MutationCreateProductOptionsArgs,
): Promise<Array<Translated<ProductOption>>> {
- const { input } = args;
- const productOptionGroupId = input[0]?.productOptionGroupId;
+ const { input } = args;
+ if (!input.length) {
+ throw new EntityNotFoundError('ProductOptionGroup', ''); // or a UserInputError if preferred
+ }
+ const productOptionGroupId = input[0].productOptionGroupId;
+ if (!input.every(i => i.productOptionGroupId === productOptionGroupId)) {
+ // If mixed groups should be supported, group by ID and process per group instead.
+ throw new EntityNotFoundError('ProductOptionGroup', 'mismatched-group-ids');
+ }
const productOptionGroup = await this.productOptionGroupService.findOne(ctx, productOptionGroupId);
if (!productOptionGroup) {
throw new EntityNotFoundError('ProductOptionGroup', productOptionGroupId);
}
const productOptions: Array<Translated<ProductOption>> = [];
for (const productOption of input) {
const res = await this.productOptionService.create(ctx, productOptionGroup, productOption);
productOptions.push(res);
}
return productOptions;
}If you prefer proper input errors, replace EntityNotFoundError with UserInputError and import it.
🤖 Prompt for AI Agents
In packages/core/src/api/resolvers/admin/product-option.resolver.ts around lines
122 to 141, validate the batch input before calling services: first throw a
UserInputError if args.input is empty; then collect productOptionGroupId values
from all input items and ensure they are all identical (if not, throw a
UserInputError indicating mixed target groups); use the single
productOptionGroupId to fetch the group and only then proceed (if fetch returns
null, keep throwing EntityNotFoundError or switch to UserInputError per
preference); this enforces a single target group and prevents empty/mismatched
batches before creating options.
| @ResolveField() | ||
| async optionList( | ||
| @Ctx() ctx: RequestContext, | ||
| @Parent() optionGroup: ProductOptionGroup, | ||
| @Args() args: { options: ProductOptionListOptions }, | ||
| @Relations({ entity: ProductOption }) relations: RelationPaths<ProductOption>, | ||
| ): Promise<PaginatedList<ProductOption>> { | ||
| return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, args.options, relations); | ||
| } |
There was a problem hiding this comment.
Authorize optionList and align return type
- Missing @Allow makes access inconsistent with options field.
- Return type should reflect Translated returned by the service.
- @ResolveField()
- async optionList(
+ @ResolveField()
+ @Allow(Permission.ReadCatalog, Permission.Public, Permission.ReadProduct)
+ async optionList(
@Ctx() ctx: RequestContext,
@Parent() optionGroup: ProductOptionGroup,
@Args() args: { options: ProductOptionListOptions },
@Relations({ entity: ProductOption }) relations: RelationPaths<ProductOption>,
- ): Promise<PaginatedList<ProductOption>> {
+ ): Promise<PaginatedList<Translated<ProductOption>>> {
return this.productOptionService.findByGroupIdList(ctx, optionGroup.id, args.options, relations);
}🤖 Prompt for AI Agents
In
packages/core/src/api/resolvers/entity/product-option-group-entity.resolver.ts
around lines 52–60, the optionList resolver is missing the @Allow decorator
(making access inconsistent with the options field) and its return type does not
reflect the Translated<ProductOption> returned by the service; add the same
@Allow(...) used on the options field (e.g. @Allow(Permission.ReadCatalog))
immediately above the method, change the signature return type to
Promise<PaginatedList<Translated<ProductOption>>>, and add any necessary imports
(Translated and Permission) so the code compiles.
| beforeSave: async g => { | ||
| g.channels = unique([this.defaultChannel, this.importCtx.channel], 'id'); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Possible runtime error: unique([...]) with undefined channel.
When initialize() is called without a channel, this.importCtx.channel is undefined. unique([this.defaultChannel, undefined], 'id') can throw or later .map(c => c.id) can NPE.
Apply a defensive filter:
- beforeSave: async g => {
- g.channels = unique([this.defaultChannel, this.importCtx.channel], 'id');
- },
+ beforeSave: async g => {
+ g.channels = unique(
+ [this.defaultChannel, this.importCtx.channel].filter(
+ (c): c is Channel => !!c,
+ ),
+ 'id',
+ );
+ },Also apply the same guard anywhere else this pattern occurs in this file (e.g., product/variant channel assignment).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeSave: async g => { | |
| g.channels = unique([this.defaultChannel, this.importCtx.channel], 'id'); | |
| }, | |
| }); | |
| beforeSave: async g => { | |
| g.channels = unique( | |
| [this.defaultChannel, this.importCtx.channel].filter( | |
| (c): c is Channel => !!c, | |
| ), | |
| 'id', | |
| ); | |
| }, |
| return [{ path: '/product-option-groups', label: 'Option Groups' }, isNew ? <Trans>New Option Group</Trans> : entity?.name]; | ||
| }, |
There was a problem hiding this comment.
Breadcrumb: add keys and localize “Option Groups”
Fix missing key in iterable and localize label.
- breadcrumb(isNew, entity) {
- return [{ path: '/product-option-groups', label: 'Option Groups' }, isNew ? <Trans>New Option Group</Trans> : entity?.name];
+ breadcrumb(isNew, entity) {
+ return [
+ { path: '/product-option-groups', label: <Trans>Option Groups</Trans> },
+ isNew ? <Trans key="new-crumb">New Option Group</Trans> : <span key="detail-crumb">{entity?.name}</span>,
+ ];
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return [{ path: '/product-option-groups', label: 'Option Groups' }, isNew ? <Trans>New Option Group</Trans> : entity?.name]; | |
| }, | |
| breadcrumb(isNew, entity) { | |
| return [ | |
| { path: '/product-option-groups', label: <Trans>Option Groups</Trans> }, | |
| isNew ? <Trans key="new-crumb">New Option Group</Trans> : <span key="detail-crumb">{entity?.name}</span>, | |
| ]; | |
| }, |
🧰 Tools
🪛 Biome (2.1.2)
[error] 35-35: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🤖 Prompt for AI Agents
In
packages/dashboard/src/app/routes/_authenticated/_product-option-groups/product-option-groups_.$id.tsx
around lines 35-36, the breadcrumb array returns an unlocalized label and a JSX
element without a key; change the first breadcrumb item to use a localized label
(e.g. use <Trans>Option Groups</Trans> or your i18n helper) and ensure any JSX
element in the returned array has a stable key (add key="new" to the <Trans>New
Option Group</Trans> element), so the array contains localized text and all JSX
entries include keys.
| loader: () => ({ breadcrumb: () => <Trans>Option Groups</Trans> }), | ||
| }); | ||
|
|
||
| function FacetListPage() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename function to match its purpose.
The function is named FacetListPage but implements the product option groups list page. This is misleading and will cause confusion during maintenance.
Apply this diff:
-function FacetListPage() {
+function ProductOptionGroupListPage() {
return (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function FacetListPage() { | |
| function ProductOptionGroupListPage() { |
🤖 Prompt for AI Agents
In
packages/dashboard/src/app/routes/_authenticated/_product-option-groups/product-option-groups.tsx
around line 26, the function is incorrectly named FacetListPage; rename it to
ProductOptionGroupsPage (or another name matching the file/purpose), update its
declaration and any corresponding export, and update all local references/usages
(default export, route/component imports, tests, and any JSX/props that
reference the old name) to use the new name so the symbol and file purpose are
consistent.
| } catch (error) { | ||
| console.error('Error creating variants:', error); | ||
| // Handle error (show toast notification, etc.) | ||
| } |
There was a problem hiding this comment.
Implement proper error handling.
The catch block is empty, silently swallowing all errors. Users won't be notified of failures, and partial state changes (e.g., some groups created but variant creation failed) will remain without cleanup.
At minimum, log the error and show user feedback:
} catch (error) {
- // Handle error (show toast notification, etc.)
+ console.error('Failed to create variants:', error);
+ // TODO: Show toast notification with error message
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('Error creating variants:', error); | |
| // Handle error (show toast notification, etc.) | |
| } | |
| } catch (error) { | |
| console.error('Failed to create variants:', error); | |
| // TODO: Show toast notification with error message | |
| } |
🤖 Prompt for AI Agents
In
packages/dashboard/src/app/routes/_authenticated/_products/components/create-product-variants-dialog.tsx
around lines 248-250, the catch block is empty and silently swallows errors;
update it to log the caught error (console.error or centralized logger), surface
a user-friendly error notification/toast (using the app's toast/notification
utility) so users know the operation failed, and undo or roll back any partial
state changes (revert optimistic updates, remove any groups created before the
failure or mark creation as failed) and re-enable UI controls; if appropriate
return or rethrow the error after handling so callers can react.
| interface OptionGroupSearchInputProps { | ||
| value: SelectedOptionGroup[]; | ||
| onChange: (groups: SelectedOptionGroup[]) => void; | ||
| disabled?: boolean; | ||
| } | ||
|
|
||
| export function OptionGroupSearchInput({ value, onChange, disabled }: OptionGroupSearchInputProps) { | ||
| const [expandedGroups, setExpandedGroups] = useState<Set<number>>(new Set()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Props should be Readonly per guidelines.
Type props as Readonly<...> to prevent accidental mutation and align with repo rules.
-interface OptionGroupSearchInputProps {
+interface OptionGroupSearchInputProps extends Readonly<{
value: SelectedOptionGroup[];
onChange: (groups: SelectedOptionGroup[]) => void;
disabled?: boolean;
-}
+}> {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface OptionGroupSearchInputProps { | |
| value: SelectedOptionGroup[]; | |
| onChange: (groups: SelectedOptionGroup[]) => void; | |
| disabled?: boolean; | |
| } | |
| export function OptionGroupSearchInput({ value, onChange, disabled }: OptionGroupSearchInputProps) { | |
| const [expandedGroups, setExpandedGroups] = useState<Set<number>>(new Set()); | |
| interface OptionGroupSearchInputProps extends Readonly<{ | |
| value: SelectedOptionGroup[]; | |
| onChange: (groups: SelectedOptionGroup[]) => void; | |
| disabled?: boolean; | |
| }> {} | |
| export function OptionGroupSearchInput({ value, onChange, disabled }: OptionGroupSearchInputProps) { | |
| const [expandedGroups, setExpandedGroups] = useState<Set<number>>(new Set()); |
🤖 Prompt for AI Agents
In
packages/dashboard/src/app/routes/_authenticated/_products/components/option-group-search-input.tsx
around lines 19 to 26, the component props are currently mutable; update the
component to accept props typed as Readonly<OptionGroupSearchInputProps> (or
redefine the interface fields as readonly) so the props cannot be mutated.
Modify the function signature to take a Readonly<OptionGroupSearchInputProps>
and ensure any internal state updates use local copies rather than mutating
props directly.
| const handleRemoveGroup = (index: number) => { | ||
| const newGroups = [...value]; | ||
| newGroups.splice(index, 1); | ||
| onChange(newGroups); | ||
| // Update expanded state | ||
| const newExpanded = new Set(expandedGroups); | ||
| newExpanded.delete(index); | ||
| setExpandedGroups(newExpanded); | ||
| }; |
There was a problem hiding this comment.
Expanded-group index drift after deletion.
Using indices as IDs causes expansion state to misalign when a group is removed (indices shift). Rebase the Set after splice.
const handleRemoveGroup = (index: number) => {
const newGroups = [...value];
newGroups.splice(index, 1);
onChange(newGroups);
- // Update expanded state
- const newExpanded = new Set(expandedGroups);
- newExpanded.delete(index);
- setExpandedGroups(newExpanded);
+ // Reindex expanded state to match shifted indices
+ const reindexed = new Set<number>();
+ expandedGroups.forEach(i => {
+ if (i < index) reindexed.add(i);
+ else if (i > index) reindexed.add(i - 1);
+ // i === index is removed
+ });
+ setExpandedGroups(reindexed);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleRemoveGroup = (index: number) => { | |
| const newGroups = [...value]; | |
| newGroups.splice(index, 1); | |
| onChange(newGroups); | |
| // Update expanded state | |
| const newExpanded = new Set(expandedGroups); | |
| newExpanded.delete(index); | |
| setExpandedGroups(newExpanded); | |
| }; | |
| const handleRemoveGroup = (index: number) => { | |
| const newGroups = [...value]; | |
| newGroups.splice(index, 1); | |
| onChange(newGroups); | |
| // Reindex expanded state to match shifted indices | |
| const reindexed = new Set<number>(); | |
| expandedGroups.forEach(i => { | |
| if (i < index) reindexed.add(i); | |
| else if (i > index) reindexed.add(i - 1); | |
| // i === index is removed | |
| }); | |
| setExpandedGroups(reindexed); | |
| }; |
|
What are the breaking changes? |
3 participants
Description
This PR adds channel awareness to
ProductOptionandProductOptionGroupentities, solving a critical data duplication issue where 88.7% of product options and 99.9% of option groups are duplicates across channels (see #3318).Motivation
In multi-channel Vendure installations, product options and option groups are currently global, leading to:
Changes
🏗️ Core Implementation
ProductOptionandProductOptionGroupnow implementChannelAwareinterfaceChannelentity📊 GraphQL API
Added new mutations:
🔒 Channel Isolation
✅ Testing
Future Work
productOptionGroupsquery returnPaginatedListinstead of arrayReferences
Screenshots
N/A - Backend only changes
Checklist
Summary by CodeRabbit
New Features
Improvements
Internationalization