Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
270 changes: 201 additions & 69 deletions src/contexts/ProductsContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,90 +24,222 @@ interface ProductsContextType {
/** Cached products (only those that have been requested) */
products: Product[];
isLoading: boolean;
error: string | null;
fetchProducts: (ids: string[]) => Promise<void>;
getProduct: (id: string) => Product | undefined;
invalidateCache: () => void;
/** Resolves a single id (returns undefined if not cached; does NOT trigger fetch) */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: getProductById documentation is incorrect: it says no fetch is triggered, but the function queues a fetch on cache miss.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/contexts/ProductsContext.tsx, line 27:

<comment>`getProductById` documentation is incorrect: it says no fetch is triggered, but the function queues a fetch on cache miss.</comment>

<file context>
@@ -24,90 +24,222 @@ interface ProductsContextType {
-  fetchProducts: (ids: string[]) => Promise<void>;
-  getProduct: (id: string) => Product | undefined;
-  invalidateCache: () => void;
+  /** Resolves a single id (returns undefined if not cached; does NOT trigger fetch) */
+  getProductById: (id: string) => Product | undefined;
+  /**
</file context>
Suggested change
/** Resolves a single id (returns undefined if not cached; does NOT trigger fetch) */
/** Resolves a single id (returns cached value when available; queues lazy fetch on miss) */

getProductById: (id: string) => Product | undefined;
Comment on lines +27 to +28
/**
* Batch lookup — returns cached products matching the given ids (in any order).
* Missing ids are silently skipped; queueing a fetch for them is the caller's
* responsibility (or rely on the lazy queueFetch fallback).
*/
getProductsByIds: (ids: string[]) => Product[];
Comment on lines +27 to +34
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🌐 Web query:

In React 18, are scheduling timers or starting network requests during render considered side effects, and can concurrent rendering or StrictMode restart/discard renders after those side effects already ran?

💡 Result:

Yes—side effects like starting timers or network requests should be treated as “side effects” and must not run during render in React 18. React’s docs explicitly require that components and hooks be pure during render and that “side effects should not run in render” because React may render components multiple times for the best user experience [1][2]. Can concurrent rendering or StrictMode restart/discard renders after those side effects already ran? - Concurrent rendering: React can pause rendering, continue later, and even abandon an in-progress render. React guarantees UI consistency by delaying DOM mutations until commit, which implies that anything you did during render (like scheduling a timer or firing a request) would not be part of the “guaranteed” commit behavior, and you must not do it anyway [3]. - StrictMode (development): React intentionally double-invokes certain component functions in development to help you find impure code; StrictMode “calls some of your functions … twice in development” including the component function body (top-level render logic) [4]. React also double-invokes effects in development, and the React team notes that an effect firing twice in StrictMode development can lead to issues like double fetching, but it’s intended to surface bugs; they recommend making effects safe via cleanup/ignores (double fetch has no harm if you handle it correctly) [5][6]. Practical conclusion: - Do not start timers or initiate network requests in render (including top-level component body). Put them in event handlers or in Effects (useEffect/useLayoutEffect) where side effects are allowed [1]. - If you fetch in an effect, assume development/StrictMode may run it more than once; write the effect to be resilient (e.g., abort/cancel, or ignore stale responses, and clean up) [6][5].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/contexts/ProductsContext.tsx"

if [ ! -f "$FILE" ]; then
  echo "Missing file: $FILE"
  exit 1
fi

echo "=== file: $FILE (head) ==="
sed -n '1,120p' "$FILE" | nl -ba

echo "=== file: $FILE (getters + batch scheduling) ==="
sed -n '120,240p' "$FILE" | nl -ba

echo "=== file: $FILE (tail) ==="
wc -l "$FILE"
tail -n 80 "$FILE" | nl -ba

Repository: adm01-debug/promo-gifts-v4

Length of output: 186


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/contexts/ProductsContext.tsx"

if [ ! -f "$FILE" ]; then
  echo "Missing file: $FILE"
  exit 1
fi

echo "=== lines 1-220 (with numbers) ==="
awk '{printf "%d\t%s\n", NR, $0}' "$FILE" | sed -n '1,220p'

echo "=== total lines ==="
wc -l "$FILE"

Repository: adm01-debug/promo-gifts-v4

Length of output: 8036


Evitar side effects em getters e corrigir estado/contrato do cache em ProductsContext

  • getProductById (135-142) e getProductsByIds (146-165) chamam queueFetch em cache miss, que agenda setTimeout + request em scheduleBatchFetch (84-118). Se consumidores chamarem isso durante render, quebra a exigência de render “puro” no React 18/StrictMode e pode disparar fetch “fantasma” em reinícios/discards.
  • Contrato desalinhado: o type diz “does NOT trigger fetch” para getProductById (27-28), mas a implementação dispara fetch via queueFetch.
  • isLoading (49, atualizado em finally de scheduleBatchFetch 111-116) é um boolean único: batches podem sobrepor e um finally pode setar false enquanto outro fetch ainda está em andamento.
  • registerProducts (169-183) só insere se o id não existe (!next.has(p.id)), então ignora updates para IDs já presentes (cache pode ficar congelado no primeiro valor).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/contexts/ProductsContext.tsx` around lines 27 - 34, getProductById and
getProductsByIds currently cause side-effects (they call queueFetch) which
violates their contract and React render purity; change them to only read from
the cache and never call queueFetch during lookup so they match the documented
behavior. Move all queueFetch/scheduleBatchFetch invocation to event handlers or
useEffect hooks where side-effects are allowed (e.g., expose a separate
ensureProducts(ids) or fetchMissingProducts(ids) method consumers call outside
render). Make isLoading track concurrent batches (use a counter or Set of batch
ids instead of a single boolean) so one batch’s finally doesn’t clear loading
while others are active. Update registerProducts to always upsert/overwrite
existing entries instead of ignoring when next.has(p.id) so cache updates
replace stale values. Ensure references to getProductById, getProductsByIds,
queueFetch, scheduleBatchFetch, isLoading, and registerProducts are updated
accordingly.

/** Manually register products into the cache (e.g. from page-level queries) */
registerProducts: (products: Product[]) => void;
}

const ProductsContext = createContext<ProductsContextType | null>(null);
export const ProductsContext = createContext<ProductsContextType | undefined>(undefined);

interface ProductsProviderProps {
children: ReactNode;
}

export function ProductsProvider({ children }: ProductsProviderProps) {
const [products, setProducts] = useState<Product[]>([]);
/**
* Lazy-loading ProductsProvider.
* Does NOT fetch all 6000+ products on startup.
* Instead, it fetches products on-demand when requested via getProductById/getProductsByIds.
* Products from page-level queries can be registered via registerProducts.
*/
export function ProductsProvider({ children }: { children: ReactNode }) {
const [cache, setCache] = useState<Map<string, Product>>(new Map());
const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState<string | null>(null);
const fetchedIdsRef = useRef<Set<string>>(new Set());
const pendingRef = useRef<Set<string>>(new Set());

if (isDuplicateModule) {
logger.warn(
'[ProductsContext] Multiple module instances detected. ' +
'This may cause stale data or hydration mismatches in HMR.',
);
}
const [key, setKey] = useState(0); // Force re-mount key

const fetchProducts = useCallback(async (ids: string[]) => {
const newIds = ids.filter(
(id) => !fetchedIdsRef.current.has(id) && !pendingRef.current.has(id),
);
if (!newIds.length) return;

newIds.forEach((id) => pendingRef.current.add(id));
setIsLoading(true);
setError(null);

try {
const raw = await fetchPromobrindProducts(newIds);
const mapped = raw.map(mapPromobrindToProduct);

setProducts((prev) => {
const existingIds = new Set(prev.map((p) => p.id));
const next = [...prev];
for (const p of mapped) {
if (!existingIds.has(p.id)) next.push(p);
}
return next;
});

newIds.forEach((id) => {
fetchedIdsRef.current.add(id);
pendingRef.current.delete(id);
});
} catch (err) {
const msg = err instanceof Error ? err.message : 'Failed to fetch products';
setError(msg);
newIds.forEach((id) => pendingRef.current.delete(id));
} finally {
setIsLoading(false);
// HMR Recovery: If we detect a duplicate module via Global Symbol, force a re-mount
useEffect(() => {
if (isDuplicateModule) {
logger.warn('[ProductsContext] HMR duplication detected. Forcing Provider re-mount.');
setKey((prev) => prev + 1);
}
}, []);

const getProduct = useCallback(
(id: string) => products.find((p) => p.id === id),
[products],
// Refs for stable callbacks
const cacheRef = useRef<Map<string, Product>>(cache);
const fetchingRef = useRef<Set<string>>(new Set());
const batchTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const batchIdsRef = useRef<Set<string>>(new Set());
const mountedRef = useRef(true);
Comment on lines +62 to +65
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

isLoading pode ir para false com outro batch ainda em voo.

Cada batch seta true no início e false no finally. Se um segundo lote começar antes de o primeiro terminar, o finally do primeiro derruba o flag mesmo com outra busca ativa, então o contexto publica um estado de loading incorreto.

💡 Sugestão de ajuste
   const cacheRef = useRef<Map<string, Product>>(cache);
   const fetchingRef = useRef<Set<string>>(new Set());
+  const inFlightBatchesRef = useRef(0);
   const batchTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
   const batchIdsRef = useRef<Set<string>>(new Set());
   const mountedRef = useRef(true);
@@
       idsToFetch.forEach((id) => fetchingRef.current.add(id));
+      inFlightBatchesRef.current += 1;
       if (mountedRef.current) setIsLoading(true);
@@
       } finally {
         idsToFetch.forEach((id) => fetchingRef.current.delete(id));
-        if (mountedRef.current) setIsLoading(false);
+        inFlightBatchesRef.current -= 1;
+        if (mountedRef.current) {
+          setIsLoading(inFlightBatchesRef.current > 0);
+        }
       }

Also applies to: 94-115

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/contexts/ProductsContext.tsx` around lines 62 - 65, isLoading can be set
to false prematurely because each batch sets it true at start and false in
finally; change the logic to derive isLoading from the actual active fetch
set/counter instead of toggling it per-batch: use fetchingRef (or a numeric
activeFetchCount) to add batch ids before starting requests and remove them in
finally, then set isLoading = fetchingRef.current.size > 0 (or activeFetchCount
> 0) rather than blindly setting false in each finally; ensure removals target
only that batch's ids, update any places that currently toggle isLoading
(references: isLoading, fetchingRef, batchIdsRef, batchTimerRef, mountedRef) and
keep mountedRef checks before state updates.


useEffect(() => {
cacheRef.current = cache;
}, [cache]);

// Cleanup on unmount
useEffect(() => {
mountedRef.current = true;
return () => {
mountedRef.current = false;
if (batchTimerRef.current) {
clearTimeout(batchTimerRef.current);
batchTimerRef.current = null;
}
};
}, []);

// Batched fetch: collects IDs over a microtask and fetches them together
const scheduleBatchFetch = useCallback(() => {
if (batchTimerRef.current) return; // already scheduled

batchTimerRef.current = setTimeout(async () => {
const idsToFetch = [...batchIdsRef.current];
batchIdsRef.current.clear();
batchTimerRef.current = null;

if (idsToFetch.length === 0) return;

idsToFetch.forEach((id) => fetchingRef.current.add(id));
if (mountedRef.current) setIsLoading(true);

try {
const raw = await fetchPromobrindProducts({
filters: { id: idsToFetch },
limit: idsToFetch.length,
});
const mapped = raw.map(mapPromobrindToProduct);

if (mountedRef.current) {
setCache((prev) => {
const next = new Map(prev);
mapped.forEach((p) => next.set(p.id, p));
return next;
});
}
} catch (err) {
logger.warn('[ProductsContext] Failed to fetch products by IDs:', err);
} finally {
idsToFetch.forEach((id) => fetchingRef.current.delete(id));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Missing negative-cache guard causes infinite refetch loop for non-existent product IDs. When a requested ID isn't returned by fetchPromobrindProducts (e.g. deleted/inactive product), the finally block removes it from fetchingRef but it never enters cache. On the next render, getProductsByIds/getProductById will see the ID is absent from cache, fetchingRef, and batchIdsRef, and will re-queue it — repeating every 50 ms indefinitely. Track requested-but-not-returned IDs (e.g. in a notFoundRef set) and include them in the queueFetch filter to break the cycle.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/contexts/ProductsContext.tsx, line 114:

<comment>Missing negative-cache guard causes infinite refetch loop for non-existent product IDs. When a requested ID isn't returned by `fetchPromobrindProducts` (e.g. deleted/inactive product), the `finally` block removes it from `fetchingRef` but it never enters `cache`. On the next render, `getProductsByIds`/`getProductById` will see the ID is absent from `cache`, `fetchingRef`, and `batchIdsRef`, and will re-queue it — repeating every 50 ms indefinitely. Track requested-but-not-returned IDs (e.g. in a `notFoundRef` set) and include them in the `queueFetch` filter to break the cycle.</comment>

<file context>
@@ -24,90 +24,222 @@ interface ProductsContextType {
+      } catch (err) {
+        logger.warn('[ProductsContext] Failed to fetch products by IDs:', err);
+      } finally {
+        idsToFetch.forEach((id) => fetchingRef.current.delete(id));
+        if (mountedRef.current) setIsLoading(false);
+      }
</file context>

if (mountedRef.current) setIsLoading(false);
Comment on lines +114 to +115
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Track missing IDs to stop repeated fetches

When a saved favorite/compare/recently-viewed item references a product that is inactive, deleted, or otherwise not returned by fetchPromobrindProducts, the fetch completes with that id still absent from cache. This finally block then removes the id from fetchingRef and toggles isLoading, causing consumers that call getProductsByIds() during render to queue the same missing id again on the next render, creating an endless 50ms refetch loop against the products endpoint. The previous implementation marked requested ids as fetched even if no row came back; this path needs an equivalent negative-cache/attempted-id guard.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: isLoading can become false while requests are still in flight when batched fetches overlap.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/contexts/ProductsContext.tsx, line 115:

<comment>`isLoading` can become false while requests are still in flight when batched fetches overlap.</comment>

<file context>
@@ -24,90 +24,222 @@ interface ProductsContextType {
+        logger.warn('[ProductsContext] Failed to fetch products by IDs:', err);
+      } finally {
+        idsToFetch.forEach((id) => fetchingRef.current.delete(id));
+        if (mountedRef.current) setIsLoading(false);
+      }
+    }, 50); // 50ms batching window
</file context>

}
}, 50); // 50ms batching window
}, []);

// Queue IDs for lazy fetching
const queueFetch = useCallback(
(ids: string[]) => {
const missing = ids.filter(
(id) =>
!cacheRef.current.has(id) && !fetchingRef.current.has(id) && !batchIdsRef.current.has(id),
);
if (missing.length === 0) return;

missing.forEach((id) => batchIdsRef.current.add(id));
scheduleBatchFetch();
},
[scheduleBatchFetch],
);

const invalidateCache = useCallback(() => {
fetchedIdsRef.current.clear();
pendingRef.current.clear();
setProducts([]);
setError(null);
const getProductById = useCallback(
(id: string): Product | undefined => {
const cached = cacheRef.current.get(id);
if (!cached) {
queueFetch([id]);
}
return cached;
},
[queueFetch],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: getProductsByIds reads from cacheRef.current (a ref) but its identity never changes when cache state is updated because it only depends on [queueFetch]. Consumers that memoize results via useMemo(() => getProductsByIds(ids), [getProductsByIds, ids]) will keep the stale empty array returned before the lazy fetch completed. Include a cache-derived signal (e.g. cache or cache.size) in the dependency array so the callback identity changes when new products arrive, or return the lookup directly from state rather than the ref.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/contexts/ProductsContext.tsx, line 143:

<comment>`getProductsByIds` reads from `cacheRef.current` (a ref) but its identity never changes when `cache` state is updated because it only depends on `[queueFetch]`. Consumers that memoize results via `useMemo(() => getProductsByIds(ids), [getProductsByIds, ids])` will keep the stale empty array returned before the lazy fetch completed. Include a cache-derived signal (e.g. `cache` or `cache.size`) in the dependency array so the callback identity changes when new products arrive, or return the lookup directly from state rather than the ref.</comment>

<file context>
@@ -24,90 +24,222 @@ interface ProductsContextType {
+      }
+      return cached;
+    },
+    [queueFetch],
+  );
+
</file context>
Suggested change
[queueFetch],
[queueFetch, cache],

);

const getProductsByIds = useCallback(
(ids: string[]): Product[] => {
const found: Product[] = [];
const missing: string[] = [];

for (const id of ids) {
const cached = cacheRef.current.get(id);
if (cached) {
found.push(cached);
} else {
missing.push(id);
}
}

if (missing.length > 0) {
queueFetch(missing);
}

return found;
},
[queueFetch],
Comment on lines +164 to +166
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refresh lookup callbacks when cache changes

For a valid but cold product id, the first getProductsByIds() call returns [] and queues the lazy fetch, but this callback never changes identity when cache is populated because it only depends on queueFetch. Several consumers memoize results with deps like [getProductsByIds, items] (checked RecentlyViewedBar, RecentlyViewedPopover, and the trash views), so they keep rendering the empty result after the fetch succeeds until some unrelated item prop changes. Include a cache/products signal in the callback/deps or update those consumers so the lazy load can actually appear.

Useful? React with 👍 / 👎.

);

// Register products from external sources (e.g. page-level useProducts queries)
const registerProducts = useCallback((products: Product[]) => {
if (products.length === 0) return;
setCache((prev) => {
const next = new Map(prev);
let changed = false;
for (const p of products) {
if (!next.has(p.id)) {
next.set(p.id, p);
changed = true;
}
}
return changed ? next : prev;
});
}, []);
Comment on lines +169 to 183
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

registerProducts ignora refresh de IDs já cacheados.

Hoje ele só insere quando !next.has(p.id). Se uma query de página trouxer uma versão mais nova ou mais completa de um produto já cacheado, o contexto continua servindo o valor antigo e nunca reidrata esse item.

💡 Sugestão de ajuste
   const registerProducts = useCallback((products: Product[]) => {
     if (products.length === 0) return;
     setCache((prev) => {
       const next = new Map(prev);
       let changed = false;
       for (const p of products) {
-        if (!next.has(p.id)) {
+        if (next.get(p.id) !== p) {
           next.set(p.id, p);
           changed = true;
         }
       }
       return changed ? next : prev;
     });
   }, []);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/contexts/ProductsContext.tsx` around lines 169 - 183, registerProducts
currently only adds products when the id is missing, so updated or richer
versions never replace the cached item; change the logic inside setCache (in
registerProducts) to also replace an existing entry when the incoming product
differs from the cached one (e.g., compare next.get(p.id) !== p or a more
specific field like updatedAt/version) — update next.set(p.id, p) and set
changed = true whenever the new product is different from the stored product so
the cache rehydrates with newer data.


// Memoize the products array from cache
const products = useMemo(() => [...cache.values()], [cache]);

const value = useMemo(
() => ({ products, isLoading, error, fetchProducts, getProduct, invalidateCache }),
[products, isLoading, error, fetchProducts, getProduct, invalidateCache],
() => ({ products, isLoading, getProductById, getProductsByIds, registerProducts }),
[products, isLoading, getProductById, getProductsByIds, registerProducts],
);

return <ProductsContext.Provider value={value}>{children}</ProductsContext.Provider>;
return (
<ProductsContext.Provider key={key} value={value}>
{children}
</ProductsContext.Provider>
);
}

/**
* No-op fallback returned when the context is unexpectedly missing.
* This prevents the entire app from crashing under HMR race conditions or
* Suspense edge-cases where a consumer mounts before the provider re-evaluates.
* Page-level data still loads via useProducts/useExternalProducts queries.
*/
const FALLBACK_CONTEXT: ProductsContextType = {
products: [],
isLoading: false,
getProductById: () => undefined,
getProductsByIds: () => [],
registerProducts: () => {},
};

/**
* Strict consumer — returns a no-op fallback (with dev warning) if used outside ProductsProvider.
* The fallback prevents app-wide crashes during HMR module-duplication races; in production
* builds the fallback path is silent.
*/
export function useProductsContext(): ProductsContextType {
const context = useContext(ProductsContext);
if (context === undefined) {
if (import.meta.env.DEV) {
logger.warn(
'[ProductsContext] useProductsContext called outside ProductsProvider — using fallback. ' +
'This usually indicates an HMR module-duplication race; a full reload should fix it.',
);
}
return FALLBACK_CONTEXT;
}
return context;
}

export function useProducts() {
const ctx = useContext(ProductsContext);
if (!ctx) throw new Error('useProducts must be used within ProductsProvider');
return ctx;
/**
* Safe consumer — returns null when outside ProductsProvider, instead of using the fallback.
* Use this for components that may render in trees without the provider (e.g. global
* floating bars rendered above route boundaries, or modal portals) and that prefer to
* branch on null themselves rather than rely on the no-op fallback.
*
* Example:
* const ctx = useProductsContextSafe();
* const data = ctx?.getProductsByIds(ids) ?? [];
*/
export function useProductsContextSafe(): ProductsContextType | null {
return useContext(ProductsContext) ?? null;
}
Loading
Loading