fix(catalog): stable pagination + variant grouping#2572
Merged
Conversation
…ation ETL-imported items share identical created_at timestamps (PostgreSQL NOW() is transaction-scoped). ORDER BY created_at DESC with no secondary key produces non-deterministic ordering within a tie group, causing the same items to appear on multiple LIMIT/OFFSET pages and others to be skipped. Measured: 10 pages × 20 items → 21 duplicates (10.5%) before; 0 after. The default sort already had desc(id) as tiebreaker; add it to the category, usage, and generic field branches for the same guarantee.
…creen ETL-imported gear items are stored as one row per size/color variant with a unique SKU. The catalog list was showing "Fineline Stretch Full-Zip Pant - Men's" three times (sizes L, L/Short, M/Short) as separate cards. Group the paginated items by name+brand before rendering: - One CatalogItemCard per group (representative = first variant) - "N options" badge on the card when the group has >1 variant - Tapping the card stores all variant items in catalogGroupVariantsAtom and navigates to the representative item's detail screen Detail screen reads the atom and shows an "Available Variants" section listing each other variant's size, color, price, weight, availability, and a retailer link. No picker or selection UI — just a plain list. Search results (vector search) still show individual items unchanged.
…-bleed categories
- Fix "3 {{count}} options" badge: i18n key is now plain "options" so
the manually-prepended count renders as "3 options" correctly
- Remove "197,717 items / Showing X of Y" from the catalog list header —
that count reflects raw variants, not grouped products
- Reduce top gap: FlatList contentContainerStyle loses top/horizontal
padding; horizontal padding moves to a px-4 wrapper in renderItem so
the categories row can be full-bleed
- CategoriesFilter gains contentPaddingX prop: ScrollView uses it as
paddingHorizontal in contentContainerStyle, letting chips scroll to the
actual screen edge while starting at the same 16 px indent as other
content
- Move variants section to directly below the action buttons in the
detail screen; shorten title from "Available Variants" → "Variants"
Contributor
|
Warning Review limit reached
More reviews will be available in 41 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughCatalog items are now grouped by normalized name and brand. The list screen renders grouped items as representative cards, stores the full variant list in a Jotai atom before navigation, and the detail screen retrieves and displays those variants without additional API calls. ChangesCatalog Item Variants Grouping and Detail Display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
Coverage Report for packages/analytics (./packages/analytics)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/mcp (./packages/mcp)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/units (./packages/units)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/api (./packages/api)
File CoverageNo changed files found. |
Contributor
Coverage Report for apps/expo (./apps/expo)
File CoverageNo changed files found. |
![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
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/expo/atoms/catalogGroupAtom.ts`:
- Around line 1-7: Move the catalogGroupVariantsAtom export out of this shared
atoms file into the catalog feature module’s state/atoms file so
catalog-specific state lives with the feature; create or add to the catalog
feature’s atoms/state file a exported atom named catalogGroupVariantsAtom (same
type atom<CatalogItem[]>() and initial value) and update all imports across the
app to point to the new location, then remove the original export from the
current file to avoid duplicate exports.
In `@apps/expo/components/CategoriesFilter.tsx`:
- Around line 62-67: The ScrollView's contentContainerStyle currently treats
contentPaddingX as falsy (so 0 is ignored); update the conditional in the
component (where ScrollView is rendered) to check for undefined (or the
property's type) instead of a truthy check—e.g., use contentPaddingX !==
undefined or typeof contentPaddingX === 'number' to return { paddingHorizontal:
contentPaddingX } when 0 is intended, otherwise undefined; this change involves
the ScrollView rendering block referencing contentPaddingX in
CategoriesFilter.tsx.
In `@apps/expo/features/catalog/screens/CatalogItemDetailScreen.tsx`:
- Around line 65-73: Remove the redundant type assertion on variant.productUrl
inside the Pressable onPress handler: update the onPress={() =>
Linking.openURL(variant.productUrl as string)} usage to call Linking.openURL
with variant.productUrl directly (no "as string" cast) in
CatalogItemDetailScreen so the Pressable and its onPress handler use the
already-typed variant.productUrl value.
- Line 287: In CatalogItemDetailScreen locate the View with className
"rounded-lg p-3 py-0 gap-4" and simplify the redundant padding by replacing "p-3
py-0" with "px-3" (so the class becomes "rounded-lg px-3 gap-4"); if vertical
padding was intended elsewhere, instead explicitly add the needed "py-*" value
rather than overriding p-3.
- Around line 26-76: VariantRow currently uses hardcoded hex colors for
availability icons; update it to use theme-based colors (via useColorScheme() or
NativeWind semantic tokens) so color follows the app theme. Import and call the
theme hook at the top of VariantRow (e.g., const { colors } = useColorScheme()
or retrieve semantic tokens), then replace '`#22c55e`' and '`#ef4444`' passed to
Ionicons with the corresponding theme values (e.g., colors.green / colors.red or
semantic tokens like 'text-success' / 'text-danger') and ensure the Icon/Text
state logic still uses the same availability checks (variant.availability ===
'in_stock').
In `@apps/expo/features/catalog/screens/CatalogItemsScreen.tsx`:
- Around line 80-87: The atom catalogGroupVariantsAtom is set in
CatalogItemsScreen via handleGroupPress before routing but isn't explicitly
cleared on back/deep-link navigation; update the code by adding a clear comment
near the catalogGroupVariantsAtom definition (and/or a short inline comment
above handleGroupPress) that documents the intended behavior: the atom is
populated only when a user navigates via the group press and may be empty on
direct/deep-link entry to the detail screen, and if desired implement explicit
cleanup on detail-screen unmount (e.g., reset atom in the detail screen's
useEffect cleanup) to guarantee clearing on back navigation.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0d7cdf5a-8f21-448d-92ef-e1f7d56efaec
📒 Files selected for processing (7)
apps/expo/atoms/catalogGroupAtom.tsapps/expo/components/CategoriesFilter.tsxapps/expo/features/catalog/lib/groupCatalogItems.tsapps/expo/features/catalog/screens/CatalogItemDetailScreen.tsxapps/expo/features/catalog/screens/CatalogItemsScreen.tsxapps/expo/lib/i18n/locales/en.jsonpackages/api/src/services/catalogService.ts
Comment on lines
+1
to
+7
| import type { CatalogItem } from 'expo-app/features/catalog/types'; | ||
| import { atom } from 'jotai'; | ||
|
|
||
| // Holds all variants of the catalog group the user tapped in the list. | ||
| // Set immediately before pushing to /catalog/[id] so the detail screen can | ||
| // show a variants list without any extra API calls. | ||
| export const catalogGroupVariantsAtom = atom<CatalogItem[]>([]); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider relocating this atom into the catalog feature module.
The atom is tightly coupled to the catalog feature and is only consumed by catalog screens. Per the feature module structure guidelines, catalog-specific state should live within apps/expo/features/catalog/ (e.g., features/catalog/atoms.ts or features/catalog/state.ts).
🤖 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 `@apps/expo/atoms/catalogGroupAtom.ts` around lines 1 - 7, Move the
catalogGroupVariantsAtom export out of this shared atoms file into the catalog
feature module’s state/atoms file so catalog-specific state lives with the
feature; create or add to the catalog feature’s atoms/state file a exported atom
named catalogGroupVariantsAtom (same type atom<CatalogItem[]>() and initial
value) and update all imports across the app to point to the new location, then
remove the original export from the current file to avoid duplicate exports.
Source: Coding guidelines
Comment on lines
+62
to
+67
| <ScrollView | ||
| horizontal | ||
| showsHorizontalScrollIndicator={false} | ||
| contentContainerStyle={ | ||
| contentPaddingX ? { paddingHorizontal: contentPaddingX } : undefined | ||
| } |
Contributor
There was a problem hiding this comment.
Fix falsy check for contentPaddingX.
Line 66 treats contentPaddingX as falsy when it's 0, which is a valid padding value. The current logic contentPaddingX ? { paddingHorizontal: contentPaddingX } : undefined will skip applying the style when contentPaddingX={0}.
🐛 Proposed fix
- contentContainerStyle={
- contentPaddingX ? { paddingHorizontal: contentPaddingX } : undefined
- }
+ contentContainerStyle={
+ contentPaddingX > 0 ? { paddingHorizontal: contentPaddingX } : undefined
+ }Or more explicitly:
- contentContainerStyle={
- contentPaddingX ? { paddingHorizontal: contentPaddingX } : undefined
- }
+ contentContainerStyle={
+ typeof contentPaddingX === 'number' && contentPaddingX > 0
+ ? { paddingHorizontal: contentPaddingX }
+ : undefined
+ }🤖 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 `@apps/expo/components/CategoriesFilter.tsx` around lines 62 - 67, The
ScrollView's contentContainerStyle currently treats contentPaddingX as falsy (so
0 is ignored); update the conditional in the component (where ScrollView is
rendered) to check for undefined (or the property's type) instead of a truthy
check—e.g., use contentPaddingX !== undefined or typeof contentPaddingX ===
'number' to return { paddingHorizontal: contentPaddingX } when 0 is intended,
otherwise undefined; this change involves the ScrollView rendering block
referencing contentPaddingX in CategoriesFilter.tsx.
Comment on lines
+65
to
+73
| {variant.productUrl ? ( | ||
| <Pressable | ||
| onPress={() => Linking.openURL(variant.productUrl as string)} | ||
| className="ml-3 flex-row items-center gap-1 rounded-md border border-border px-3 py-1.5" | ||
| > | ||
| <Text className="text-xs text-foreground">{t('catalog.viewOnRetailerSite')}</Text> | ||
| <Icon name="open-in-new" size={11} color="text-foreground" /> | ||
| </Pressable> | ||
| ) : null} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove unnecessary type cast.
Line 67 casts variant.productUrl as string, but according to the CatalogItemSchema, productUrl is already typed as string (non-nullable). The type cast is redundant.
♻️ Proposed fix
- onPress={() => Linking.openURL(variant.productUrl as string)}
+ onPress={() => Linking.openURL(variant.productUrl)}📝 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
| {variant.productUrl ? ( | |
| <Pressable | |
| onPress={() => Linking.openURL(variant.productUrl as string)} | |
| className="ml-3 flex-row items-center gap-1 rounded-md border border-border px-3 py-1.5" | |
| > | |
| <Text className="text-xs text-foreground">{t('catalog.viewOnRetailerSite')}</Text> | |
| <Icon name="open-in-new" size={11} color="text-foreground" /> | |
| </Pressable> | |
| ) : null} | |
| {variant.productUrl ? ( | |
| <Pressable | |
| onPress={() => Linking.openURL(variant.productUrl)} | |
| className="ml-3 flex-row items-center gap-1 rounded-md border border-border px-3 py-1.5" | |
| > | |
| <Text className="text-xs text-foreground">{t('catalog.viewOnRetailerSite')}</Text> | |
| <Icon name="open-in-new" size={11} color="text-foreground" /> | |
| </Pressable> | |
| ) : null} |
🤖 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 `@apps/expo/features/catalog/screens/CatalogItemDetailScreen.tsx` around lines
65 - 73, Remove the redundant type assertion on variant.productUrl inside the
Pressable onPress handler: update the onPress={() =>
Linking.openURL(variant.productUrl as string)} usage to call Linking.openURL
with variant.productUrl directly (no "as string" cast) in
CatalogItemDetailScreen so the Pressable and its onPress handler use the
already-typed variant.productUrl value.
| {t('catalog.specifications')} | ||
| </Text> | ||
| <View className="rounded-lg p-3 gap-4"> | ||
| <View className="rounded-lg p-3 py-0 gap-4"> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Simplify redundant padding class.
The class string 'p-3 py-0' is redundant because py-0 overrides the vertical padding from p-3, leaving only the horizontal padding. This is effectively px-3.
Consider using 'px-3' for clarity, or if vertical padding is needed elsewhere, restructure the classes.
♻️ Proposed fix
- <View className="rounded-lg p-3 py-0 gap-4">
+ <View className="rounded-lg px-3 gap-4">📝 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
| <View className="rounded-lg p-3 py-0 gap-4"> | |
| <View className="rounded-lg px-3 gap-4"> |
🤖 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 `@apps/expo/features/catalog/screens/CatalogItemDetailScreen.tsx` at line 287,
In CatalogItemDetailScreen locate the View with className "rounded-lg p-3 py-0
gap-4" and simplify the redundant padding by replacing "p-3 py-0" with "px-3"
(so the class becomes "rounded-lg px-3 gap-4"); if vertical padding was intended
elsewhere, instead explicitly add the needed "py-*" value rather than overriding
p-3.
Comment on lines
+80
to
+87
| const groupedItems = useMemo(() => groupCatalogItems(paginatedItems), [paginatedItems]); | ||
|
|
||
| const totalItemsText = `${Number(totalItems).toLocaleString()} ${ | ||
| totalItems === 1 ? t('catalog.item') : t('catalog.items') | ||
| }`; | ||
| const showingText = t('catalog.showingItems', { | ||
| current: paginatedItems.length, | ||
| total: Number(totalItems).toLocaleString(), | ||
| }); | ||
| const setGroupVariants = useSetAtom(catalogGroupVariantsAtom); | ||
|
|
||
| const handleGroupPress = (group: CatalogItemGroup) => { | ||
| setGroupVariants(group.variants); | ||
| router.push({ pathname: '/catalog/[id]', params: { id: group.representative.id } }); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Verify atom cleanup on back navigation.
The atom is set before navigation (line 85) but never explicitly cleared. If the user navigates back from the detail screen and then selects a different group, the atom will be overwritten correctly. However, if the user navigates to a detail screen via deep link or direct URL entry, the atom will be empty, which is the intended behavior per the design.
Consider documenting this behavior in a comment near the atom definition or in the handleGroupPress function.
🤖 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 `@apps/expo/features/catalog/screens/CatalogItemsScreen.tsx` around lines 80 -
87, The atom catalogGroupVariantsAtom is set in CatalogItemsScreen via
handleGroupPress before routing but isn't explicitly cleared on back/deep-link
navigation; update the code by adding a clear comment near the
catalogGroupVariantsAtom definition (and/or a short inline comment above
handleGroupPress) that documents the intended behavior: the atom is populated
only when a user navigates via the group press and may be empty on
direct/deep-link entry to the detail screen, and if desired implement explicit
cleanup on detail-screen unmount (e.g., reset atom in the detail screen's
useEffect cleanup) to guarantee clearing on back navigation.
- Replace hardcoded hex availability colors with colors.green/destructive - Fix open-in-new icon invisible in dark mode (was passing NativeWind class string as color value; now uses colors.foreground) - Move SimilarItems outside padded container for full-bleed scroll - Add px-4 to SimilarItems title to maintain heading alignment - Remove unused 'options' i18n key
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
1 participant
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.
Summary
ORDER BY created_at DESCwith no secondary sort key produced non-deterministic ordering for ETL-imported items (all sharing identical timestamps). Addeddesc(id)tiebreaker to every sort branch ingetCatalogItems— measured 21 duplicates per 10 pages before, 0 after.name + brandso size/color variants (e.g. three rows of "Fineline Stretch Full-Zip Pant - Men's") show as one card. Tapping sets a Jotai atom with all variant items and navigates to the representative item's detail screen.contentPaddingXprop.open-in-newicon on both the main button and each variant row.Test plan
createdAt(price, ratingValue) — confirm no cross-page duplicatesSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements