fix(core): Optimize collection variants N+1 and persist catalog filters#4636
Open
RubenDarioGuerreroNeira wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds request-scoped caching and prefetching for collection product variants: the admin CollectionResolver precomputes collection IDs and, when Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/core/src/config/config.module.ts (1)
18-18: Type the optional dependency to match runtime behavior.At line 18,
@Optional()allowsModuleRefto beundefinedat runtime, but the type annotation is non-nullable. Change toprivate moduleRef?: ModuleRef(orModuleRef | undefined) to keep type-safety aligned with actual injection behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/config/config.module.ts` at line 18, The parameter decorated with `@Optional`() (private moduleRef: ModuleRef) is typed as non-nullable but can be undefined at runtime; update the constructor/property declaration for moduleRef to be optional (e.g., private moduleRef?: ModuleRef or private moduleRef: ModuleRef | undefined) so the type matches runtime injection behavior and adjust any usages of moduleRef in ConfigModule to handle possible undefined values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/api/resolvers/admin/collection.resolver.ts`:
- Around line 74-80: The preload branch is checking the wrong field name: change
the isFieldInSelection call from isFieldInSelection(info, 'variants') to
isFieldInSelection(info, 'productVariants') so the cache gets primed when the
GraphQL selection requests productVariants; update the branch that creates
variantsPromise via this.collectionService.getProductVariantsForCollections and
the subsequent this.requestContextCache.set(ctx, CacheKey.CollectionVariants) to
run under the corrected field check so the new preload path is exercised.
In `@packages/core/src/api/resolvers/entity/collection-entity.resolver.ts`:
- Around line 68-78: The fast-path uses
requestContextCache.get(CacheKey.CollectionVariants) and reuses a cached
Promise<Map<string, ProductVariant[]>> regardless of the requested relations,
causing incorrect/partially-hydrated variants; modify the logic so the cached
shortcut only applies when the requested relation set is the minimal/default set
(e.g., no extra variant relations) or change the cache key to include a stable
representation of the requested relations (e.g., serialize relations into the
key), and ensure productVariantService.getVariantsByCollectionId(...) is called
with the same relations when populating the cache so that
productVariantService.applyPricesAndTranslateVariants(...) always receives
fully-hydrated variants consistent with the query.
- Around line 66-82: The shortcut branch for cached collection variants bypasses
the normal pagination: it returns the full cached array and sets totalItems =
items.length, breaking the pagination contract. Change the branch (the
isDefaultOptions && apiType === 'admin' block that uses requestContextCache.get
and productVariantService.applyPricesAndTranslateVariants) to mirror the normal
getVariantsByCollectionId behaviour: compute the configured default list options
(the same defaults used elsewhere), determine the totalItems as the full cached
variants.length, slice the cached variants to the default limit before calling
applyPricesAndTranslateVariants, and return items (the sliced/transformed
variants) and totalItems equal to the full cached array length rather than
items.length. Ensure you use the same default options logic as
getVariantsByCollectionId to keep behaviour consistent.
In `@packages/core/src/config/config.module.ts`:
- Around line 40-42: The two early-return checks that currently use "if
(!this.moduleRef) { return; }" should emit a warning before returning so
operators know initialization was skipped; update both occurrences in
config.module.ts to log a clear message (e.g., "ConfigModule: moduleRef missing
— skipping initialization of strategies/operations") using the module's existing
logger (this.logger / this.log / similar) or console.warn if no logger is
available, and include which init branch was skipped to aid debugging.
In `@packages/core/src/service/helpers/list-query-builder/list-query-builder.ts`:
- Around line 404-409: The current branch logs a warning and returns when
building the EXISTS subquery for condition.isExistsCondition.customPropertyKey,
which silently drops the filter; change this to fail explicitly instead of
returning silently: replace the Logger.warn(...) + return with throwing a
descriptive Error (including condition.isExistsCondition.customPropertyKey and
any error details) so the caller knows the filter couldn't be applied, or
alternatively preserve the original predicate/JOIN path by invoking the existing
code path that applies the original predicate/JOIN (rather than skipping) —
adjust the code around the EXISTS handling (the block referencing
condition.isExistsCondition.customPropertyKey and Logger.warn) to either
re-apply the original condition/JOIN or throw a clear exception.
In `@packages/core/src/service/services/collection.service.ts`:
- Around line 1061-1079: The listQueryBuilder.build call in this method is
unintentionally limited by the global default pagination, causing the
multi-collection variant fetch to only include the first page; fix it by calling
listQueryBuilder.build with a copy of the incoming options that explicitly
disables pagination (e.g., set take and skip to undefined/null) before building
the ProductVariant query used by qb/getRawAndEntities so the subsequent grouping
into variantsByCollectionId sees the full result set; update the call site where
options is passed into listQueryBuilder.build (referencing
listQueryBuilder.build, ProductVariant, qb, and getRawAndEntities) to use the
adjusted options.
---
Nitpick comments:
In `@packages/core/src/config/config.module.ts`:
- Line 18: The parameter decorated with `@Optional`() (private moduleRef:
ModuleRef) is typed as non-nullable but can be undefined at runtime; update the
constructor/property declaration for moduleRef to be optional (e.g., private
moduleRef?: ModuleRef or private moduleRef: ModuleRef | undefined) so the type
matches runtime injection behavior and adjust any usages of moduleRef in
ConfigModule to handle possible undefined values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67391ccc-026c-4cbc-a549-5408129b43ea
📒 Files selected for processing (9)
e2e-common/benchmarks/benchmark-list-query.e2e-spec.tspackages/core/src/api/resolvers/admin/collection.resolver.tspackages/core/src/api/resolvers/entity/collection-entity.resolver.tspackages/core/src/common/constants.tspackages/core/src/config/config.module.tspackages/core/src/service/helpers/list-query-builder/list-query-builder.tspackages/core/src/service/helpers/list-query-builder/parse-filter-params.tspackages/core/src/service/services/collection.service.tspackages/core/src/service/services/product-variant.service.ts
RubenDarioGuerreroNeira
force-pushed
the
fix/collection-variants-n-plus-1-v3
branch
from
5e39a8d to
fac3fb7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/api/resolvers/entity/collection-entity.resolver.ts (1)
80-81: Consider extractingdefaultRelationsForCacheas a shared constant.The cache is populated in
collection.resolver.tswith['taxCategory']relations, and this resolver has a separate hardcoded copy. If either side changes independently, the compatibility check may incorrectly use stale cache data or unnecessarily bypass a valid cache.🔧 Example: Define a shared constant
In a shared constants file (e.g.,
common/constants.ts):export const COLLECTION_VARIANTS_CACHE_RELATIONS = ['taxCategory'] as const;Then import and use in both:
collection.resolver.tswhen populating the cachecollection-entity.resolver.tsfor the compatibility check🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/api/resolvers/entity/collection-entity.resolver.ts` around lines 80 - 81, Extract the hardcoded defaultRelationsForCache used in the isCacheCompatible check into a shared exported constant (e.g., COLLECTION_VARIANTS_CACHE_RELATIONS) and import that constant here and in the place that populates the collection cache; replace the inline array in collection-entity.resolver.ts with the imported constant and update isCacheCompatible to use it so both cache population and compatibility checks reference the same source of truth (ensure the constant is typed as readonly/const tuple if desired).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/api/resolvers/entity/collection-entity.resolver.ts`:
- Around line 80-81: Extract the hardcoded defaultRelationsForCache used in the
isCacheCompatible check into a shared exported constant (e.g.,
COLLECTION_VARIANTS_CACHE_RELATIONS) and import that constant here and in the
place that populates the collection cache; replace the inline array in
collection-entity.resolver.ts with the imported constant and update
isCacheCompatible to use it so both cache population and compatibility checks
reference the same source of truth (ensure the constant is typed as
readonly/const tuple if desired).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd49a918-f7ec-4247-887a-8f9fe51df216
📒 Files selected for processing (11)
packages/core/src/api/middleware/auth-guard.tspackages/core/src/api/resolvers/admin/collection.resolver.tspackages/core/src/api/resolvers/entity/collection-entity.resolver.tspackages/core/src/config/auth/default-verification-token-strategy.tspackages/core/src/config/config.module.tspackages/core/src/config/order/order-by-code-access-strategy.tspackages/core/src/plugin/default-scheduler-plugin/default-scheduler-strategy.tspackages/core/src/service/helpers/list-query-builder/list-query-builder.tspackages/core/src/service/helpers/settings-store/settings-store.service.tspackages/core/src/service/services/collection.service.tspackages/core/src/service/services/session.service.ts
✅ Files skipped from review due to trivial changes (4)
- packages/core/src/config/auth/default-verification-token-strategy.ts
- packages/core/src/api/middleware/auth-guard.ts
- packages/core/src/service/helpers/settings-store/settings-store.service.ts
- packages/core/src/api/resolvers/admin/collection.resolver.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/service/helpers/list-query-builder/list-query-builder.ts
- packages/core/src/config/config.module.ts
- packages/core/src/service/services/collection.service.ts
RubenDarioGuerreroNeira
force-pushed
the
fix/collection-variants-n-plus-1-v3
branch
from
c248af5 to
ba2ac94
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/stellate-plugin/lib/src/service/stellate.service.d.ts (2)
55-55: Keeppurge()consistently async.
Line 55 returningPromise<void> | undefinedforces unnecessary branching in consumers; prefer always returningPromise<void>(usePromise.resolve()for no-op paths).Proposed declaration update
- purge(type: CachedType, keys?: ID[], keyName?: string): Promise<void> | undefined; + purge(type: CachedType, keys?: ID[], keyName?: string): Promise<void>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stellate-plugin/lib/src/service/stellate.service.d.ts` at line 55, The purge declaration currently returns Promise<void> | undefined which forces callers to handle a non-async branch; change the signature of purge(type: CachedType, keys?: ID[], keyName?: string) to always return Promise<void>, and update the implementation of StellateService.purge (and any overrides) to return Promise.resolve() for no-op branches or otherwise return a Promise so callers can uniformly await it; reference the purge method and the CachedType/ID/keyName parameters when making the change.
3-10:CachedTypeis over-widened by| string.
On Lines 3-10, the trailing| stringeffectively collapses literal-type safety.Proposed type-shape refinement
+type KnownCachedType = + | 'Product' + | 'ProductVariant' + | 'Collection' + | 'SearchResponse' + | 'SearchResult' + | 'SearchResponseCacheIdentifier'; -type CachedType = - | 'Product' - | 'ProductVariant' - | 'Collection' - | 'SearchResponse' - | 'SearchResult' - | 'SearchResponseCacheIdentifier' - | string; +type CachedType = KnownCachedType | (string & {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stellate-plugin/lib/src/service/stellate.service.d.ts` around lines 3 - 10, The union type CachedType is over-widened by the trailing "| string", which defeats literal-type safety; remove the "| string" from the CachedType definition (or alternatively split into two distinct types such as a strict literal union named CachedType and a separate alias for arbitrary strings) so callers get the intended literal types ('Product', 'ProductVariant', 'Collection', 'SearchResponse', 'SearchResult', 'SearchResponseCacheIdentifier') and maintain type-safety in the code paths that depend on CachedType.packages/stellate-plugin/lib/src/api/search-response.resolver.d.ts (1)
3-5: TightencacheIdentifierreturn typing (avoidany).
Line 4 exposescollectionSlug: any, which removes type-safety from this public API.Proposed declaration update
export declare class SearchResponseFieldResolver { cacheIdentifier(info: GraphQLResolveInfo): { - collectionSlug: any; + collectionSlug: string | null; }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stellate-plugin/lib/src/api/search-response.resolver.d.ts` around lines 3 - 5, The public method cacheIdentifier currently returns { collectionSlug: any } which removes type-safety; update its return type to a concrete string-based type (e.g. { collectionSlug: string } or { collectionSlug?: string | null } depending on whether collectionSlug can be absent) in the cacheIdentifier declaration so callers get proper typings; adjust any internal code in the cacheIdentifier implementation to guarantee/convert to that string type rather than returning any.packages/payments-plugin/package/mollie/mollie.plugin.d.ts (1)
1-3: Remove deep dependency import;ListParametersis not publicly exported by Mollie.Line 1 imports from
@mollie/api-client/dist/types/binders/methods/parameters, which is a non-public implementation path not exposed in Mollie's official public API. The type does not exist in any stable public entrypoint (e.g., package root), so importing from the root won't work as an alternative.Consider defining
ListParameterslocally within this module to avoid relying on Mollie's internal implementation details, which may change across versions. Alternatively, document the risk explicitly if the deep import must be retained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/payments-plugin/package/mollie/mollie.plugin.d.ts` around lines 1 - 3, Remove the deep import of ListParameters from `@mollie/api-client` and replace it by defining a local type that captures only the fields you actually need; specifically delete the import of ListParameters and update the AdditionalEnabledPaymentMethodsParams alias to use a locally declared interface (e.g., declare interface MollieListParameters { /* include resource?: string and other needed props */ }) or a minimal Partial/Omit-based local type, then use Partial<Omit<MollieListParameters, 'resource'>> for AdditionalEnabledPaymentMethodsParams so the module no longer depends on Mollie’s internal path.packages/elasticsearch-plugin/lib/src/indexing/indexer.controller.d.ts (1)
16-27: Reuse the sharedReindexMessageResponsetype.The same public shape is already exported from
packages/elasticsearch-plugin/lib/src/types.d.ts(Lines 200-204). Keeping a second copy here makes the declaration surface drift-prone for no real benefit.♻️ Proposed cleanup
import { BulkOperation, BulkOperationDoc, ProductChannelMessageData, + ReindexMessageResponse, ReindexMessageData, UpdateAssetMessageData, UpdateProductMessageData, UpdateVariantMessageData, UpdateVariantsByIdMessageData, VariantChannelMessageData, VariantIndexItem, } from '../types'; export declare const defaultProductRelations: Array<EntityRelationPaths<Product>>; export declare const defaultVariantRelations: Array<EntityRelationPaths<ProductVariant>>; -export interface ReindexMessageResponse { - total: number; - completed: number; - duration: number; -} type BulkVariantOperation = {Also applies to: 30-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/elasticsearch-plugin/lib/src/indexing/indexer.controller.d.ts` around lines 16 - 27, Replace the duplicated local ReindexMessageResponse shape with the shared exported type from the package types module: import and use ReindexMessageResponse from '../types' instead of redefining it in indexer.controller.d.ts (apply the same replacement for the other duplicated declarations referenced around the file, e.g., the block at the second occurrence). Update any references in this file to point to the imported ReindexMessageResponse symbol so the file reuses the single canonical type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/elasticsearch-plugin/lib/src/options.d.ts`:
- Around line 484-485: Fix the typo in the JSDoc for the mapQuery option: update
the comment describing mapQuery (the doc above the mapQuery symbol) to remove
the duplicated word "the" so the sentence reads "This config option allows the
modification of the whole (already built) search query..." and keep the rest of
the description unchanged.
In `@packages/payments-plugin/package/mollie/mollie.plugin.d.ts`:
- Around line 104-105: Fix the documentation typos: correct the malformed inline
code punctuation around redirectUrl in the PaymentMethod description (replace
the stray double backticks so it reads `redirectUrl`) and fix the misspelling
`automaticallreleased` to `automatically released` (search for the same phrase
in the mollie plugin docs where that typo appears, e.g., near the
PaymentMethod/redirectUrl text). Ensure the inline code formatting uses single
backticks and the spelling reads "automatically released".
In `@packages/payments-plugin/package/stripe/metadata-sanitize.d.ts`:
- Around line 4-5: Fix the JSDoc typo in metadata-sanitize.d.ts: change
"Santitize metadata to ensure it follow Stripe's instructions" to a
grammatically correct sentence (e.g., "Sanitize metadata to ensure it follows
Stripe's instructions") in the JSDoc comment above the exported function
(sanitizeMetadata) so public API docs read correctly; also scan the file for the
same typo and update any other occurrences in the JSDoc.
In `@packages/payments-plugin/package/stripe/stripe-client.d.ts`:
- Around line 3-7: The class docstring is misleading about secret exposure:
update the comment for VendureStripeClient to state that the API key is stored
privately (apiKey is a private field) and only the webhook secret
(webhookSecret) is exposed/accessible, so it doesn't imply the API key is
exposed; reference VendureStripeClient, apiKey, and webhookSecret when making
this wording change.
---
Nitpick comments:
In `@packages/elasticsearch-plugin/lib/src/indexing/indexer.controller.d.ts`:
- Around line 16-27: Replace the duplicated local ReindexMessageResponse shape
with the shared exported type from the package types module: import and use
ReindexMessageResponse from '../types' instead of redefining it in
indexer.controller.d.ts (apply the same replacement for the other duplicated
declarations referenced around the file, e.g., the block at the second
occurrence). Update any references in this file to point to the imported
ReindexMessageResponse symbol so the file reuses the single canonical type.
In `@packages/payments-plugin/package/mollie/mollie.plugin.d.ts`:
- Around line 1-3: Remove the deep import of ListParameters from
`@mollie/api-client` and replace it by defining a local type that captures only
the fields you actually need; specifically delete the import of ListParameters
and update the AdditionalEnabledPaymentMethodsParams alias to use a locally
declared interface (e.g., declare interface MollieListParameters { /* include
resource?: string and other needed props */ }) or a minimal Partial/Omit-based
local type, then use Partial<Omit<MollieListParameters, 'resource'>> for
AdditionalEnabledPaymentMethodsParams so the module no longer depends on
Mollie’s internal path.
In `@packages/stellate-plugin/lib/src/api/search-response.resolver.d.ts`:
- Around line 3-5: The public method cacheIdentifier currently returns {
collectionSlug: any } which removes type-safety; update its return type to a
concrete string-based type (e.g. { collectionSlug: string } or {
collectionSlug?: string | null } depending on whether collectionSlug can be
absent) in the cacheIdentifier declaration so callers get proper typings; adjust
any internal code in the cacheIdentifier implementation to guarantee/convert to
that string type rather than returning any.
In `@packages/stellate-plugin/lib/src/service/stellate.service.d.ts`:
- Line 55: The purge declaration currently returns Promise<void> | undefined
which forces callers to handle a non-async branch; change the signature of
purge(type: CachedType, keys?: ID[], keyName?: string) to always return
Promise<void>, and update the implementation of StellateService.purge (and any
overrides) to return Promise.resolve() for no-op branches or otherwise return a
Promise so callers can uniformly await it; reference the purge method and the
CachedType/ID/keyName parameters when making the change.
- Around line 3-10: The union type CachedType is over-widened by the trailing "|
string", which defeats literal-type safety; remove the "| string" from the
CachedType definition (or alternatively split into two distinct types such as a
strict literal union named CachedType and a separate alias for arbitrary
strings) so callers get the intended literal types ('Product', 'ProductVariant',
'Collection', 'SearchResponse', 'SearchResult', 'SearchResponseCacheIdentifier')
and maintain type-safety in the code paths that depend on CachedType.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36be10c2-9088-4849-922c-926ff0678d7a
📒 Files selected for processing (61)
.gemini/vendure_gemini_backup/settings.jsonpackages/admin-ui/src/karma.conf.jspackages/core/src/api/resolvers/admin/collection.resolver.tspackages/core/src/api/resolvers/entity/collection-entity.resolver.tspackages/core/src/common/constants.tspackages/elasticsearch-plugin/lib/index.d.tspackages/elasticsearch-plugin/lib/src/api/api-extensions.d.tspackages/elasticsearch-plugin/lib/src/api/custom-mappings.resolver.d.tspackages/elasticsearch-plugin/lib/src/api/custom-script-fields.resolver.d.tspackages/elasticsearch-plugin/lib/src/api/elasticsearch-resolver.d.tspackages/elasticsearch-plugin/lib/src/build-elastic-body.d.tspackages/elasticsearch-plugin/lib/src/constants.d.tspackages/elasticsearch-plugin/lib/src/elasticsearch.health.d.tspackages/elasticsearch-plugin/lib/src/elasticsearch.service.d.tspackages/elasticsearch-plugin/lib/src/indexing/elasticsearch-index.service.d.tspackages/elasticsearch-plugin/lib/src/indexing/indexer.controller.d.tspackages/elasticsearch-plugin/lib/src/indexing/indexing-utils.d.tspackages/elasticsearch-plugin/lib/src/options.d.tspackages/elasticsearch-plugin/lib/src/plugin.d.tspackages/elasticsearch-plugin/lib/src/types.d.tspackages/payments-plugin/package/braintree/braintree-common.d.tspackages/payments-plugin/package/braintree/braintree.handler.d.tspackages/payments-plugin/package/braintree/braintree.plugin.d.tspackages/payments-plugin/package/braintree/braintree.resolver.d.tspackages/payments-plugin/package/braintree/constants.d.tspackages/payments-plugin/package/braintree/index.d.tspackages/payments-plugin/package/braintree/types.d.tspackages/payments-plugin/package/index.d.tspackages/payments-plugin/package/mollie/api-extensions.d.tspackages/payments-plugin/package/mollie/constants.d.tspackages/payments-plugin/package/mollie/graphql/generated-shop-types.d.tspackages/payments-plugin/package/mollie/index.d.tspackages/payments-plugin/package/mollie/mollie.common-resolver.d.tspackages/payments-plugin/package/mollie/mollie.controller.d.tspackages/payments-plugin/package/mollie/mollie.handler.d.tspackages/payments-plugin/package/mollie/mollie.helpers.d.tspackages/payments-plugin/package/mollie/mollie.plugin.d.tspackages/payments-plugin/package/mollie/mollie.service.d.tspackages/payments-plugin/package/mollie/mollie.shop-resolver.d.tspackages/payments-plugin/package/mollie/types.d.tspackages/payments-plugin/package/stripe/constants.d.tspackages/payments-plugin/package/stripe/index.d.tspackages/payments-plugin/package/stripe/metadata-sanitize.d.tspackages/payments-plugin/package/stripe/raw-body.middleware.d.tspackages/payments-plugin/package/stripe/stripe-client.d.tspackages/payments-plugin/package/stripe/stripe-utils.d.tspackages/payments-plugin/package/stripe/stripe.controller.d.tspackages/payments-plugin/package/stripe/stripe.handler.d.tspackages/payments-plugin/package/stripe/stripe.plugin.d.tspackages/payments-plugin/package/stripe/stripe.resolver.d.tspackages/payments-plugin/package/stripe/stripe.service.d.tspackages/payments-plugin/package/stripe/types.d.tspackages/stellate-plugin/lib/index.d.tspackages/stellate-plugin/lib/src/api/api-extensions.d.tspackages/stellate-plugin/lib/src/api/search-response.resolver.d.tspackages/stellate-plugin/lib/src/constants.d.tspackages/stellate-plugin/lib/src/default-purge-rules.d.tspackages/stellate-plugin/lib/src/purge-rule.d.tspackages/stellate-plugin/lib/src/service/stellate.service.d.tspackages/stellate-plugin/lib/src/stellate-plugin.d.tspackages/stellate-plugin/lib/src/types.d.ts
💤 Files with no reviewable changes (1)
- packages/admin-ui/src/karma.conf.js
✅ Files skipped from review due to trivial changes (15)
- packages/payments-plugin/package/index.d.ts
- packages/elasticsearch-plugin/lib/index.d.ts
- packages/payments-plugin/package/braintree/constants.d.ts
- packages/payments-plugin/package/stripe/raw-body.middleware.d.ts
- packages/payments-plugin/package/stripe/index.d.ts
- packages/payments-plugin/package/braintree/index.d.ts
- .gemini/vendure_gemini_backup/settings.json
- packages/payments-plugin/package/mollie/index.d.ts
- packages/elasticsearch-plugin/lib/src/constants.d.ts
- packages/elasticsearch-plugin/lib/src/build-elastic-body.d.ts
- packages/payments-plugin/package/mollie/types.d.ts
- packages/payments-plugin/package/braintree/braintree.plugin.d.ts
- packages/payments-plugin/package/braintree/braintree-common.d.ts
- packages/stellate-plugin/lib/src/types.d.ts
- packages/elasticsearch-plugin/lib/src/plugin.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/api/resolvers/admin/collection.resolver.ts
- packages/core/src/api/resolvers/entity/collection-entity.resolver.ts
Comment on lines
+484
to
+485
| * This config option allows the the modification of the whole (already built) search query. This allows | ||
| * for e.g. wildcard / fuzzy searches on the index. |
Contributor
There was a problem hiding this comment.
Fix minor doc typo in mapQuery description.
Line 484 currently says “allows the the modification”; remove the duplicated “the”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/elasticsearch-plugin/lib/src/options.d.ts` around lines 484 - 485,
Fix the typo in the JSDoc for the mapQuery option: update the comment describing
mapQuery (the doc above the mapQuery symbol) to remove the duplicated word "the"
so the sentence reads "This config option allows the modification of the whole
(already built) search query..." and keep the rest of the description unchanged.
Comment on lines
+104
to
+105
| * PaymentMethod was given the code "mollie-payment-method". The `redirectUrl``is the url that is used to redirect the end-user | ||
| * back to your storefront after completing the payment. |
Contributor
There was a problem hiding this comment.
Fix documentation typos in public plugin docs.
Line 104 has malformed inline code punctuation, and Line 168 has a typo (automaticallreleased).
Suggested doc fix
- * PaymentMethod was given the code "mollie-payment-method". The `redirectUrl``is the url that is used to redirect the end-user
+ * PaymentMethod was given the code "mollie-payment-method". The `redirectUrl` is the URL that is used to redirect the end-user
...
- * Make sure to capture a payment within 28 days, after that the payment will be automaticallreleased.
+ * Make sure to capture a payment within 28 days, after that the payment will be automatically released.Also applies to: 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/payments-plugin/package/mollie/mollie.plugin.d.ts` around lines 104
- 105, Fix the documentation typos: correct the malformed inline code
punctuation around redirectUrl in the PaymentMethod description (replace the
stray double backticks so it reads `redirectUrl`) and fix the misspelling
`automaticallreleased` to `automatically released` (search for the same phrase
in the mollie plugin docs where that typo appears, e.g., near the
PaymentMethod/redirectUrl text). Ensure the inline code formatting uses single
backticks and the spelling reads "automatically released".
Comment on lines
+4
to
+5
| * Santitize metadata to ensure it follow Stripe's instructions | ||
| * |
Contributor
There was a problem hiding this comment.
Fix JSDoc typo/grammar in description.
Line 4 says “Santitize … it follow”; please correct wording for public API docs quality.
Suggested doc fix
- * Santitize metadata to ensure it follow Stripe's instructions
+ * Sanitize metadata to ensure it follows Stripe's instructions📝 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.
Suggested change
| * Santitize metadata to ensure it follow Stripe's instructions | |
| * | |
| * Sanitize metadata to ensure it follows Stripe's instructions | |
| * |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/payments-plugin/package/stripe/metadata-sanitize.d.ts` around lines
4 - 5, Fix the JSDoc typo in metadata-sanitize.d.ts: change "Santitize metadata
to ensure it follow Stripe's instructions" to a grammatically correct sentence
(e.g., "Sanitize metadata to ensure it follows Stripe's instructions") in the
JSDoc comment above the exported function (sanitizeMetadata) so public API docs
read correctly; also scan the file for the same typo and update any other
occurrences in the JSDoc.
Comment on lines
+3
to
+7
| * Wrapper around the Stripe client that exposes ApiKey and WebhookSecret | ||
| */ | ||
| export declare class VendureStripeClient extends Stripe { | ||
| private apiKey; | ||
| webhookSecret: string; |
Contributor
There was a problem hiding this comment.
Docstring currently overstates API key exposure.
Line 3 says the wrapper exposes the API key, but Line 6 declares apiKey as private. Please align the comment to avoid confusion about secret exposure.
✏️ Suggested wording fix
- * Wrapper around the Stripe client that exposes ApiKey and WebhookSecret
+ * Wrapper around the Stripe client that stores the API key internally
+ * and exposes the webhook secret for webhook verification.📝 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.
Suggested change
| * Wrapper around the Stripe client that exposes ApiKey and WebhookSecret | |
| */ | |
| export declare class VendureStripeClient extends Stripe { | |
| private apiKey; | |
| webhookSecret: string; | |
| * Wrapper around the Stripe client that stores the API key internally | |
| * and exposes the webhook secret for webhook verification. | |
| */ | |
| export declare class VendureStripeClient extends Stripe { | |
| private apiKey; | |
| webhookSecret: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/payments-plugin/package/stripe/stripe-client.d.ts` around lines 3 -
7, The class docstring is misleading about secret exposure: update the comment
for VendureStripeClient to state that the API key is stored privately (apiKey is
a private field) and only the webhook secret (webhookSecret) is
exposed/accessible, so it doesn't imply the API key is exposed; reference
VendureStripeClient, apiKey, and webhookSecret when making this wording change.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/service/services/collection.service.ts (1)
1077-1086: Consider usinginnerJoininstead ofleftJoinfor the product relation.The comment states "We explicitly join with the product to ensure we filter out soft-deleted products", but using
leftJoincould include orphaned variants whereproductisNULL. Since you're filtering withproduct.deletedAt IS NULL(which would excludeNULLproducts anyway), aninnerJoinwould be more semantically correct and slightly more efficient.♻️ Suggested change
- qb.leftJoin('productvariant.product', 'product') + qb.innerJoin('productvariant.product', 'product') .innerJoin('productvariant.collections', 'collection', 'collection.id IN (:...collectionIds)', { collectionIds, }) .andWhere('product.deletedAt IS NULL')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/collection.service.ts` around lines 1077 - 1086, The current query uses qb.leftJoin('productvariant.product', 'product') which can allow null products; change this to an inner join so orphaned variants are excluded at the join level: replace the leftJoin on productvariant.product with an innerJoin and keep the existing innerJoin('productvariant.collections','collection',...) and the andWhere('product.deletedAt IS NULL') and addSelect lines unchanged; update references to qb.leftJoin -> qb.innerJoin for the product relation (function/variable: qb, relation: productvariant.product, predicate: product.deletedAt IS NULL, aliases: product, collectionId, variantId).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/service/services/collection.service.ts`:
- Around line 1077-1086: The current query uses
qb.leftJoin('productvariant.product', 'product') which can allow null products;
change this to an inner join so orphaned variants are excluded at the join
level: replace the leftJoin on productvariant.product with an innerJoin and keep
the existing innerJoin('productvariant.collections','collection',...) and the
andWhere('product.deletedAt IS NULL') and addSelect lines unchanged; update
references to qb.leftJoin -> qb.innerJoin for the product relation
(function/variable: qb, relation: productvariant.product, predicate:
product.deletedAt IS NULL, aliases: product, collectionId, variantId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 711e3433-9ce5-4076-acd2-2ed6d2f600ff
📒 Files selected for processing (14)
packages/core/src/api/middleware/auth-guard.tspackages/core/src/api/resolvers/admin/collection.resolver.tspackages/core/src/api/resolvers/entity/collection-entity.resolver.tspackages/core/src/common/constants.tspackages/core/src/config/auth/default-verification-token-strategy.tspackages/core/src/config/config.module.tspackages/core/src/config/order/order-by-code-access-strategy.tspackages/core/src/plugin/default-scheduler-plugin/default-scheduler-strategy.tspackages/core/src/service/helpers/list-query-builder/list-query-builder.tspackages/core/src/service/helpers/list-query-builder/parse-filter-params.tspackages/core/src/service/helpers/settings-store/settings-store.service.tspackages/core/src/service/services/collection.service.tspackages/core/src/service/services/product-variant.service.tspackages/core/src/service/services/session.service.ts
✅ Files skipped from review due to trivial changes (6)
- packages/core/src/config/auth/default-verification-token-strategy.ts
- packages/core/src/config/order/order-by-code-access-strategy.ts
- packages/core/src/service/helpers/settings-store/settings-store.service.ts
- packages/core/src/plugin/default-scheduler-plugin/default-scheduler-strategy.ts
- packages/core/src/service/services/session.service.ts
- packages/core/src/common/constants.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/api/middleware/auth-guard.ts
- packages/core/src/api/resolvers/admin/collection.resolver.ts
- packages/core/src/service/helpers/list-query-builder/parse-filter-params.ts
- packages/core/src/service/services/product-variant.service.ts
- packages/core/src/config/config.module.ts
RubenDarioGuerreroNeira
force-pushed
the
fix/collection-variants-n-plus-1-v3
branch
from
ba2ac94 to
a79acfa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
e2e-common/benchmarks/benchmark-list-query.e2e-spec.ts (2)
40-45: Query capture predicate is too loose and mutation of sharedcapturedQueriesis racy across tests.
query.includes('SELECT') && query.includes('"product"')will match unrelated statements (e.g.product_variant,product_translation,product_option_group— all contain the substringproduct, and with MySQL backticks`product_variant`also contains`product`as a substring is false, but the double-quoted case"product_variant"does not contain"product"; however"product_translation"likewise does not — so this is actually narrower than it looks). More importantly, the check allows any capitalization/casing mismatch to slip through and, sincecapturedQueriesis module-scoped and only reset inside eachitbefore issuing the query, any background queries fromserver.initor auth (asSuperAdmin) prior to the first test can persist; and if tests ever run concurrently, captures from one test can bleed into another.Consider (a) scoping the filter to exact table identifiers (
"product"/`product`as full quoted identifiers with word boundaries), and (b) ensuring the reset is tied to the test lifecycle (e.g.beforeEach) rather than ad-hoc before eachadminClient.query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-common/benchmarks/benchmark-list-query.e2e-spec.ts` around lines 40 - 45, The current predicate that pushes SQL into capturedQueries (the check using query.includes('SELECT') && (query.includes('"product"') || query.includes('`product`'))) is too loose and capturedQueries is module-scoped causing racy cross-test leakage; change the filter in the capture handler to match full quoted table identifiers using a stricter regex/word-boundary match for exact identifiers like "product" or `product` (and ensure SELECT is matched case-insensitively), and move the capturedQueries reset into the test lifecycle (use beforeEach to reinitialize capturedQueries before each test) instead of ad-hoc before calling adminClient.query or relying on server.init/asSuperAdmin timing.
67-67: Hardcoded fixture path couplese2e-commontopackages/coreinternal structure.The relative path
path.join(__dirname, '../../packages/core/e2e/fixtures/e2e-products-minimal.csv')breaks the established pattern where each package maintains its own fixtures directory. This approach is fragile: reorganizingpackages/core/e2e/fixtures/or running this spec from a different working directory silently fails during the 240s timeout.Either add a fixture under
e2e-common/fixtures/to match the pattern used by other packages (asset-server-plugin, cli, core, dashboard, job-queue-plugin), or export a fixtures-path helper from@vendure/testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-common/benchmarks/benchmark-list-query.e2e-spec.ts` at line 67, The hardcoded reference to a fixture inside packages/core ties e2e-common to core; update the productsCsvPath usage in benchmark-list-query.e2e-spec.ts (the productsCsvPath variable) to follow the shared-fixtures pattern: either add e2e-products-minimal.csv into e2e-common/fixtures and point productsCsvPath to that fixtures directory (e.g. use path.join(__dirname, '../fixtures/...')), or instead add and use a fixtures-path helper exported from `@vendure/testing` and replace the direct path with that helper so the test no longer depends on packages/core internal layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-common/benchmarks/benchmark-list-query.e2e-spec.ts`:
- Around line 93-100: The test uses _and with two eq conditions on the same
field (testManyToManyId) which can never match; update the filter in
benchmark-list-query.e2e-spec.ts to use two distinct FacetValue IDs that are
known to be assigned to the same Product in the fixtures (e.g., replace the
duplicate eq '1' and '2' with the actual pair that co-occur on a product), and
add assertions after the query to verify totalItems and items content (assert
the expected product id(s) and count) so the test fails if the query collapses
to a single EXISTS or otherwise returns semantically incorrect results while
still passing structural SQL checks.
- Around line 102-174: The tests name the variable lastQuery but use
capturedQueries.find which returns the first match; change each test's lookup
(the three blocks referencing lastQuery) to select the final matching SQL
statement—either by using Array.prototype.findLast (or reverse-iterating
capturedQueries) or by tightening the predicate to only match the items SELECT
(e.g., exclude "SELECT COUNT" or require "SELECT" with columns) so the assertion
on capturedQueries/lastQuery in the filter and sort tests (references:
capturedQueries, lastQuery) targets the actual items query and not the
COUNT/earlier statements.
---
Nitpick comments:
In `@e2e-common/benchmarks/benchmark-list-query.e2e-spec.ts`:
- Around line 40-45: The current predicate that pushes SQL into capturedQueries
(the check using query.includes('SELECT') && (query.includes('"product"') ||
query.includes('`product`'))) is too loose and capturedQueries is module-scoped
causing racy cross-test leakage; change the filter in the capture handler to
match full quoted table identifiers using a stricter regex/word-boundary match
for exact identifiers like "product" or `product` (and ensure SELECT is matched
case-insensitively), and move the capturedQueries reset into the test lifecycle
(use beforeEach to reinitialize capturedQueries before each test) instead of
ad-hoc before calling adminClient.query or relying on server.init/asSuperAdmin
timing.
- Line 67: The hardcoded reference to a fixture inside packages/core ties
e2e-common to core; update the productsCsvPath usage in
benchmark-list-query.e2e-spec.ts (the productsCsvPath variable) to follow the
shared-fixtures pattern: either add e2e-products-minimal.csv into
e2e-common/fixtures and point productsCsvPath to that fixtures directory (e.g.
use path.join(__dirname, '../fixtures/...')), or instead add and use a
fixtures-path helper exported from `@vendure/testing` and replace the direct path
with that helper so the test no longer depends on packages/core internal layout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b0957a4-0a70-45c4-b973-6c0f8dd0c85d
📒 Files selected for processing (5)
.gemini/settings.json.gemini/vendure_gemini_backup/settings.json.gitignoree2e-common/benchmarks/benchmark-list-query.e2e-spec.tspackages/core/src/api/middleware/auth-guard.ts
✅ Files skipped from review due to trivial changes (4)
- .gemini/vendure_gemini_backup/settings.json
- packages/core/src/api/middleware/auth-guard.ts
- .gemini/settings.json
- .gitignore
RubenDarioGuerreroNeira
force-pushed
the
fix/collection-variants-n-plus-1-v3
branch
from
f443c3a to
65f831a
Compare
Member
|
Hi, before I can properly review this, it needs to be cleaned up to include only the changes related to the fix. |
RubenDarioGuerreroNeira
force-pushed
the
fix/collection-variants-n-plus-1-v3
branch
from
65f831a to
f36a9be
Compare
Author
|
Hi @michaelbromley, I have cleaned up the PR as requested. I removed all unrelated changes, including minor |
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
2 participants
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.
This Pull Request introduces significant performance optimizations and stability improvements to the core Vendure
engine, specifically addressing the Collection variants N+1 problem and catalog filtering logic. This version (v3) has
been rewritten to resolve previous technical debt, ensuring O(n) complexity and better type safety.
Key Changes
fetching all variants for multiple collections in a single database query.
linearly with the number of variants.
preloading variants, significantly speeding up the Collection List view in the Admin UI.
integrity.
they are only used for filtering (and not sorting), drastically reducing SQL complexity for stores with many custom
fields.
potential "catastrophic" startup crashes in environments where the NestJS container might not be fully available
(e.g., certain CLI tools or migrations).
batching optimizations within resolvers.
Why these changes?
Previous attempts at these optimizations lacked rigorous validation and had performance bottlenecks in the mapping logic. This PR provides a "production-ready" implementation that follows Vendure's architectural standards while providing a massive performance boost for catalog-heavy installations.