From d8c078ad5a3084836c3b32732d215d99d8275ba0 Mon Sep 17 00:00:00 2001 From: sdarbinyan Date: Thu, 13 Aug 2026 17:20:00 +0400 Subject: [PATCH] fix: review findings from full-diff audit (7 fixed) - popularSearches sent translated display text as the actual search query instead of the canonical term - useSuggestion() now prefers target.query.q when present. - CartService.addItem() dedup guard resolved immediately instead of awaiting the real in-flight add; now tracks the pending Promise per itemID so concurrent callers await the actual result. - addItem()'s Promise never rejected on failure (resolve() in both next/error branches) - now rejects on error; buyNow() catches and shows an error toast instead of navigating on a failed add. - Quick View had no stale-response guard - a slower earlier request could overwrite a faster later one. Added a request-generation counter. - cart autoSubmitPurchase() set paymentStatus to null synchronously right after firing the async submit call, blanking the success screen while the request was still in flight. Removed the redundant/harmful line. - Order terminal-status guard (cancelled/refunded can't be reopened) lived only in the page component. Moved enforcement into the gateway (single write path) via a shared TERMINAL_ORDER_STATUSES const, so no future caller can bypass it. - TranslatePipe's per-instance memoization cache had no eviction, so bindings with volatile params (pagination counts) grew it unbounded for the component's lifetime. Capped at 50 entries. Not changed: the dark-mode color override was flagged as clobbering admin branding, but it's the exact palette explicitly requested this session for the global dark default - not a bug. Co-Authored-By: Claude Sonnet 5 --- .../orders/facade/admin-orders.facade.ts | 2 +- .../admin/orders/models/admin-order.model.ts | 8 +++++++ .../admin-order-detail-page.component.ts | 9 ++++---- .../pages/admin-orders-list-page.component.ts | 5 +++-- .../services/admin-orders-local.gateway.ts | 22 +++++++++++++------ .../containers/catalog-container.component.ts | 19 ++++++++++++++-- .../product-details-container.component.ts | 7 +++++- src/app/i18n/translate.pipe.ts | 6 +++++ src/app/pages/cart/cart.component.ts | 1 - src/app/services/cart.service.ts | 20 +++++++++++------ 10 files changed, 73 insertions(+), 26 deletions(-) diff --git a/src/app/features/admin/orders/facade/admin-orders.facade.ts b/src/app/features/admin/orders/facade/admin-orders.facade.ts index 6ea263f..14a1d39 100644 --- a/src/app/features/admin/orders/facade/admin-orders.facade.ts +++ b/src/app/features/admin/orders/facade/admin-orders.facade.ts @@ -1,6 +1,6 @@ import { Injectable, computed, inject, signal } from '@angular/core'; import { take } from 'rxjs/operators'; -import { AdminOrder, AdminOrderListFilters, AdminOrderStatus } from '../models/admin-order.model'; +import { AdminOrder, AdminOrderListFilters, AdminOrderStatus, TERMINAL_ORDER_STATUSES } from '../models/admin-order.model'; import { AdminOrdersLocalGateway } from '../services/admin-orders-local.gateway'; import { LocalStorageService } from '../../../../core/storage/local-storage.service'; diff --git a/src/app/features/admin/orders/models/admin-order.model.ts b/src/app/features/admin/orders/models/admin-order.model.ts index a0d50e3..081c0f0 100644 --- a/src/app/features/admin/orders/models/admin-order.model.ts +++ b/src/app/features/admin/orders/models/admin-order.model.ts @@ -3,6 +3,14 @@ import { UUID } from '../../../../shared/types/primitive.types'; export type AdminOrderStatus = 'pending' | 'processing' | 'shipped' | 'delivered' | 'cancelled' | 'refunded'; export type AdminOrderPaymentStatus = 'unpaid' | 'paid' | 'refund_requested' | 'refunded'; +/** + * Single source of truth for which statuses are terminal. Enforced at the + * facade layer (not just the page component's dropdown filtering) so any + * caller - a different admin surface, a future API route - can't move a + * cancelled/refunded order back to an active status. + */ +export const TERMINAL_ORDER_STATUSES: readonly AdminOrderStatus[] = ['cancelled', 'refunded']; + export interface AdminOrderCustomer { name: string; email: string; diff --git a/src/app/features/admin/orders/pages/admin-order-detail-page.component.ts b/src/app/features/admin/orders/pages/admin-order-detail-page.component.ts index db4050b..04b8d19 100644 --- a/src/app/features/admin/orders/pages/admin-order-detail-page.component.ts +++ b/src/app/features/admin/orders/pages/admin-order-detail-page.component.ts @@ -3,7 +3,7 @@ import { CommonModule } from '@angular/common'; import { FormsModule } from '@angular/forms'; import { ActivatedRoute, Router } from '@angular/router'; import { AdminOrdersFacade } from '../facade/admin-orders.facade'; -import { AdminOrderStatus } from '../models/admin-order.model'; +import { AdminOrderStatus, TERMINAL_ORDER_STATUSES } from '../models/admin-order.model'; import { TranslatePipe } from '../../../../i18n/translate.pipe'; import { TranslateService } from '../../../../i18n/translate.service'; import { LanguageService } from '../../../../services/language.service'; @@ -13,7 +13,6 @@ import { OrderTimelineComponent, OrderTimelineEntry } from '../components/order- import { ConfirmDialogComponent } from '../../../../shared/ui/confirm-dialog/confirm-dialog.component'; const WORKFLOW_STEPS: AdminOrderStatus[] = ['pending', 'processing', 'shipped', 'delivered']; -const TERMINAL_STATUSES: AdminOrderStatus[] = ['cancelled', 'refunded']; @Component({ selector: 'app-admin-order-detail-page', @@ -32,7 +31,7 @@ export class AdminOrderDetailPageComponent { readonly statuses: AdminOrderStatus[] = ['pending', 'processing', 'shipped', 'delivered', 'cancelled', 'refunded']; /** Terminal statuses are only reachable via the confirm-gated cancel()/requestRefund(), never the raw dropdown. */ - readonly selectableStatuses: AdminOrderStatus[] = this.statuses.filter(status => !TERMINAL_STATUSES.includes(status)); + readonly selectableStatuses: AdminOrderStatus[] = this.statuses.filter(status => !TERMINAL_ORDER_STATUSES.includes(status)); readonly workflowSteps = WORKFLOW_STEPS; readonly noteDraft = signal(''); readonly pendingCancelId = signal(null); @@ -41,7 +40,7 @@ export class AdminOrderDetailPageComponent { readonly isTerminal = computed(() => { const order = this.facade.selected(); - return !!order && TERMINAL_STATUSES.includes(order.status); + return !!order && TERMINAL_ORDER_STATUSES.includes(order.status); }); readonly currentStepIndex = computed(() => { @@ -79,7 +78,7 @@ export class AdminOrderDetailPageComponent { } setStatus(id: string, status: AdminOrderStatus): void { - if (TERMINAL_STATUSES.includes(status)) { + if (TERMINAL_ORDER_STATUSES.includes(status)) { // Unreachable from the dropdown (options are filtered), but guard anyway // since terminal transitions must always go through the confirm dialog. return; diff --git a/src/app/features/admin/orders/pages/admin-orders-list-page.component.ts b/src/app/features/admin/orders/pages/admin-orders-list-page.component.ts index cdd8cb5..ae6502d 100644 --- a/src/app/features/admin/orders/pages/admin-orders-list-page.component.ts +++ b/src/app/features/admin/orders/pages/admin-orders-list-page.component.ts @@ -3,7 +3,7 @@ import { CommonModule } from '@angular/common'; import { FormsModule } from '@angular/forms'; import { Router } from '@angular/router'; import { AdminOrdersFacade, AdminOrderColumn, ALL_ORDER_COLUMNS } from '../facade/admin-orders.facade'; -import { AdminOrderStatus } from '../models/admin-order.model'; +import { AdminOrderStatus, TERMINAL_ORDER_STATUSES } from '../models/admin-order.model'; import { TranslatePipe } from '../../../../i18n/translate.pipe'; import { LanguageService } from '../../../../services/language.service'; import { ButtonComponent } from '../../../../shared/ui/button/button.component'; @@ -32,7 +32,8 @@ export class AdminOrdersListPageComponent { readonly statuses = ['all', 'pending', 'processing', 'shipped', 'delivered', 'cancelled', 'refunded'] as const; /** Bulk status change excludes terminal statuses - cancel/refund must go through the confirm-gated single-order flow. */ - readonly bulkSelectableStatuses: AdminOrderStatus[] = ['pending', 'processing', 'shipped', 'delivered']; + readonly bulkSelectableStatuses: AdminOrderStatus[] = (['pending', 'processing', 'shipped', 'delivered'] as const) + .filter(status => !TERMINAL_ORDER_STATUSES.includes(status)); readonly allColumns = ALL_ORDER_COLUMNS; protected readonly columnsPanelOpen = signal(false); protected readonly bulkStatusValue = signal('pending'); diff --git a/src/app/features/admin/orders/services/admin-orders-local.gateway.ts b/src/app/features/admin/orders/services/admin-orders-local.gateway.ts index b3f7969..cd7cd54 100644 --- a/src/app/features/admin/orders/services/admin-orders-local.gateway.ts +++ b/src/app/features/admin/orders/services/admin-orders-local.gateway.ts @@ -1,7 +1,7 @@ import { Injectable, inject } from '@angular/core'; import { Observable, of } from 'rxjs'; import { delay } from 'rxjs/operators'; -import { AdminOrder, AdminOrderListFilters, AdminOrdersListResult, AdminOrderStatus } from '../models/admin-order.model'; +import { AdminOrder, AdminOrderListFilters, AdminOrdersListResult, AdminOrderStatus, TERMINAL_ORDER_STATUSES } from '../models/admin-order.model'; import { AdminOrdersGateway } from './admin-orders-gateway.interface'; import { AdminAuthService } from '../../../../core/admin-auth/admin-auth.service'; @@ -38,12 +38,20 @@ export class AdminOrdersLocalGateway implements AdminOrdersGateway { } updateStatus(id: string, status: AdminOrderStatus): Observable { - return this.mutate(id, order => ({ - ...order, - status, - updatedAt: new Date().toISOString(), - timeline: [...order.timeline, { status, timestamp: new Date().toISOString(), eventKey: 'statusChanged' as const, actor: this.currentActor }], - })); + return this.mutate(id, order => { + // Once terminal (cancelled/refunded), an order never moves to any + // other status again - enforced here, not just in the admin UI, so + // no caller (this gateway is the single write path) can reopen one. + if (TERMINAL_ORDER_STATUSES.includes(order.status)) { + return order; + } + return { + ...order, + status, + updatedAt: new Date().toISOString(), + timeline: [...order.timeline, { status, timestamp: new Date().toISOString(), eventKey: 'statusChanged' as const, actor: this.currentActor }], + }; + }); } requestRefund(id: string): Observable { diff --git a/src/app/features/website/catalog/containers/catalog-container.component.ts b/src/app/features/website/catalog/containers/catalog-container.component.ts index c56ced6..1da3e9f 100644 --- a/src/app/features/website/catalog/containers/catalog-container.component.ts +++ b/src/app/features/website/catalog/containers/catalog-container.component.ts @@ -294,15 +294,25 @@ export class CatalogContainerComponent { readonly quickViewProduct = signal(null); readonly quickViewLoading = signal(false); + private quickViewRequestId = 0; + openQuickView(productId: number): void { this.quickViewProduct.set(null); this.quickViewLoading.set(true); + const requestId = ++this.quickViewRequestId; this.productFacade.getProduct(productId).pipe(takeUntilDestroyed(this.destroyRef)).subscribe({ next: product => { + // A faster later click may have already resolved and changed + // quickViewRequestId - discard this response if so, rather than + // showing the wrong product in the still-open dialog. + if (requestId !== this.quickViewRequestId) return; this.quickViewProduct.set(product); this.quickViewLoading.set(false); }, - error: () => this.quickViewLoading.set(false) + error: () => { + if (requestId !== this.quickViewRequestId) return; + this.quickViewLoading.set(false); + } }); } @@ -358,7 +368,12 @@ export class CatalogContainerComponent { } useSuggestion(suggestion: SearchSuggestionItem): void { - const value = suggestion.title; + // Prefer the canonical query text (e.g. popularSearches' target.query.q, + // always the stable English term the backend index matches against) over + // the display title, which may be translated for locales other than the + // one the backend expects. + const canonicalQuery = suggestion.target?.query?.['q']; + const value = typeof canonicalQuery === 'string' ? canonicalQuery : suggestion.title; this.onSearchQueryChange(value); this.submitSearch(value); } diff --git a/src/app/features/website/product/containers/product-details-container.component.ts b/src/app/features/website/product/containers/product-details-container.component.ts index 539bc9a..c65abb5 100644 --- a/src/app/features/website/product/containers/product-details-container.component.ts +++ b/src/app/features/website/product/containers/product-details-container.component.ts @@ -330,7 +330,12 @@ export class ProductDetailsContainerComponent { } async buyNow(): Promise { - await this.addToCart(); + try { + await this.addToCart(); + } catch { + this.notifications.show(this.translate.t('common.errorDescription'), 'warning'); + return; + } void this.router.navigate([`/${this.languageService.currentLanguage()}/cart`]); } diff --git a/src/app/i18n/translate.pipe.ts b/src/app/i18n/translate.pipe.ts index a35360e..3e0ef4d 100644 --- a/src/app/i18n/translate.pipe.ts +++ b/src/app/i18n/translate.pipe.ts @@ -11,6 +11,9 @@ import { LanguageService } from '../services/language.service'; * nothing actually changed - hit a Map lookup instead of re-walking the * translation tree and re-running the interpolation regex. */ +/** Bindings with volatile params (pagination counts, etc.) would otherwise grow this cache for the component's whole lifetime. */ +const MAX_CACHE_ENTRIES = 50; + @Pipe({ name: 'translate', pure: false, @@ -36,6 +39,9 @@ export class TranslatePipe implements PipeTransform { } const result = this.translateService.t(key, params); + if (this.cache.size >= MAX_CACHE_ENTRIES) { + this.cache.clear(); + } this.cache.set(cacheKey, result); return result; } diff --git a/src/app/pages/cart/cart.component.ts b/src/app/pages/cart/cart.component.ts index b030843..8cc9110 100644 --- a/src/app/pages/cart/cart.component.ts +++ b/src/app/pages/cart/cart.component.ts @@ -499,7 +499,6 @@ export class CartComponent implements OnDestroy { this.router.navigate([`/${lang}`]); } }); - this.paymentStatus.set(null); } copyPaymentLink(): void { diff --git a/src/app/services/cart.service.ts b/src/app/services/cart.service.ts index 7a07de6..0f5f671 100644 --- a/src/app/services/cart.service.ts +++ b/src/app/services/cart.service.ts @@ -14,7 +14,7 @@ export class CartService { private readonly STORAGE_KEY = 'marketplace_cart'; private cartItems = signal([]); private isTelegram = typeof window !== 'undefined' && !!window.Telegram?.WebApp; - private addingItems = new Set(); + private addingItems = new Map>(); private initialized = false; items = this.cartItems.asReadonly(); @@ -211,8 +211,11 @@ export class CartService { * synchronously. */ addItem(itemID: number, quantity: number = 1, variant?: CartVariant): Promise { - // Prevent duplicate API calls for same item - if (this.addingItems.has(itemID)) return Promise.resolve(); + // A concurrent call for the same not-yet-in-cart item awaits the same + // in-flight promise instead of resolving immediately - otherwise a + // rapid double-click could see the item "added" before it actually is. + const pending = this.addingItems.get(itemID); + if (pending) return pending; const currentItems = this.cartItems(); const existingItem = currentItems.find(i => this.isSameCartLine(i, itemID, variant)); @@ -224,9 +227,8 @@ export class CartService { } // Get item details from API and add to cart - this.addingItems.add(itemID); - return import('./api.service').then(({ ApiService }) => - new Promise((resolve) => { + const promise = import('./api.service').then(({ ApiService }) => + new Promise((resolve, reject) => { this.injector.get(ApiService).getItem(itemID).subscribe({ next: (item) => { const cartItem = this.normalizeCartItem({ @@ -244,14 +246,18 @@ export class CartService { error: (err) => { console.error('Error adding to cart:', err); this.addingItems.delete(itemID); - resolve(); + reject(err); } }); }) ).catch((err) => { console.error('Error loading API service:', err); this.addingItems.delete(itemID); + throw err; }); + + this.addingItems.set(itemID, promise); + return promise; } updateQuantity(itemID: number, quantity: number, variant?: CartVariant): void {