fix: review findings from full-diff audit (7 fixed)
Some checks failed
Architecture Governance / architecture (push) Has been cancelled

- 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 <noreply@anthropic.com>
This commit is contained in:
sdarbinyan
2026-08-13 17:20:00 +04:00
parent 357d346787
commit d8c078ad5a
10 changed files with 73 additions and 26 deletions

View File

@@ -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';

View File

@@ -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;

View File

@@ -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<string | null>(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;

View File

@@ -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<AdminOrderStatus>('pending');

View File

@@ -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<AdminOrder | null> {
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<AdminOrder | null> {

View File

@@ -294,15 +294,25 @@ export class CatalogContainerComponent {
readonly quickViewProduct = signal<Product | null>(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);
}

View File

@@ -330,7 +330,12 @@ export class ProductDetailsContainerComponent {
}
async buyNow(): Promise<void> {
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`]);
}

View File

@@ -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;
}

View File

@@ -499,7 +499,6 @@ export class CartComponent implements OnDestroy {
this.router.navigate([`/${lang}`]);
}
});
this.paymentStatus.set(null);
}
copyPaymentLink(): void {

View File

@@ -14,7 +14,7 @@ export class CartService {
private readonly STORAGE_KEY = 'marketplace_cart';
private cartItems = signal<CartItem[]>([]);
private isTelegram = typeof window !== 'undefined' && !!window.Telegram?.WebApp;
private addingItems = new Set<number>();
private addingItems = new Map<number, Promise<void>>();
private initialized = false;
items = this.cartItems.asReadonly();
@@ -211,8 +211,11 @@ export class CartService {
* synchronously.
*/
addItem(itemID: number, quantity: number = 1, variant?: CartVariant): Promise<void> {
// 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<void>((resolve) => {
const promise = import('./api.service').then(({ ApiService }) =>
new Promise<void>((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 {