fix(catalog): deduplicate variants in browser modal + increase page density#2582
Merged
Merged
Conversation
…ensity Apply groupCatalogItems deduplication (already used in CatalogItemsScreen since PR #2572) to CatalogBrowserModal — paginated list, popular items, and recently used sections were all showing variants as separate repeated entries. Bump paginated limit from 20 → 80 on both the catalog tab and browser modal. With ~6–7 variants per unique product on average, 20 raw items was collapsing to ~3 visible groups per page and immediately re-triggering the infinite scroll loader. 80 items yields ~12+ unique groups per page without adding any extra DB sort complexity (list projection, no embedding/fat JSONB columns).
Contributor
|
Warning Review limit reached
More reviews will be available in 42 minutes and 32 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 (4)
WalkthroughCatalogBrowserModal and CatalogItemsScreen are updated to apply item grouping logic to catalog display sections. ChangesCatalog item grouping and pagination
🎯 2 (Simple) | ⏱️ ~10 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/units (./packages/units)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/mcp (./packages/mcp)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/analytics (./packages/analytics)
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: 1
🤖 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/features/catalog/components/CatalogBrowserModal.tsx`:
- Around line 390-394: The flattened paginatedRawItems is recreated every render
so groupedPaginatedItems' useMemo always sees a new dependency; fix by memoizing
the flattened array (paginatedRawItems) or combining the flatten+group into one
useMemo. Concretely, move the flatMap call into a useMemo that depends on
paginatedData?.pages (e.g. create paginatedRawItems via useMemo(() =>
paginatedData?.pages.flatMap(p => p.items) ?? [], [paginatedData?.pages])) and
then pass that memoized paginatedRawItems into the existing useMemo that calls
groupCatalogItems, or collapse both steps into a single useMemo that computes
and returns groupCatalogItems(paginatedData?.pages.flatMap(...)). Ensure you
reference paginatedRawItems, groupedPaginatedItems, paginatedData?.pages, and
groupCatalogItems when making the change.
🪄 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: cb598b8b-567d-497c-8b1c-3fd647f46952
📒 Files selected for processing (2)
apps/expo/features/catalog/components/CatalogBrowserModal.tsxapps/expo/features/catalog/screens/CatalogItemsScreen.tsx
Comment on lines
+390
to
+394
| const paginatedRawItems = paginatedData?.pages.flatMap((page) => page.items) ?? []; | ||
| const groupedPaginatedItems = useMemo( | ||
| () => groupCatalogItems(paginatedRawItems), | ||
| [paginatedRawItems], | ||
| ); |
Contributor
There was a problem hiding this comment.
Unstable useMemo dependency causes unnecessary recomputation.
paginatedRawItems is computed inline on every render, creating a new array reference each time. This defeats useMemo for groupedPaginatedItems since the dependency always appears changed.
Memoize the flattened array or compute everything in a single useMemo:
Proposed fix
- const paginatedRawItems = paginatedData?.pages.flatMap((page) => page.items) ?? [];
- const groupedPaginatedItems = useMemo(
- () => groupCatalogItems(paginatedRawItems),
- [paginatedRawItems],
- );
+ const groupedPaginatedItems = useMemo(
+ () => groupCatalogItems(paginatedData?.pages.flatMap((page) => page.items) ?? []),
+ [paginatedData],
+ );📝 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
| const paginatedRawItems = paginatedData?.pages.flatMap((page) => page.items) ?? []; | |
| const groupedPaginatedItems = useMemo( | |
| () => groupCatalogItems(paginatedRawItems), | |
| [paginatedRawItems], | |
| ); | |
| const groupedPaginatedItems = useMemo( | |
| () => groupCatalogItems(paginatedData?.pages.flatMap((page) => page.items) ?? []), | |
| [paginatedData], | |
| ); |
🤖 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/components/CatalogBrowserModal.tsx` around lines
390 - 394, The flattened paginatedRawItems is recreated every render so
groupedPaginatedItems' useMemo always sees a new dependency; fix by memoizing
the flattened array (paginatedRawItems) or combining the flatten+group into one
useMemo. Concretely, move the flatMap call into a useMemo that depends on
paginatedData?.pages (e.g. create paginatedRawItems via useMemo(() =>
paginatedData?.pages.flatMap(p => p.items) ?? [], [paginatedData?.pages])) and
then pass that memoized paginatedRawItems into the existing useMemo that calls
groupCatalogItems, or collapse both steps into a single useMemo that computes
and returns groupCatalogItems(paginatedData?.pages.flatMap(...)). Ensure you
reference paginatedRawItems, groupedPaginatedItems, paginatedData?.pages, and
groupCatalogItems when making the change.
…thumbnails in detail screen - Delete neon/ directory (monitor-query-performance.md, pgstatementsextensiondoc.md) - Add CatalogItemImage thumbnail to each VariantRow in CatalogItemDetailScreen
github-actions
Bot
added
the
documentation
- Add ImageViewerModal: full-screen swipeable gallery with horizontal FlatList pagination, native pinch-to-zoom on iOS (ScrollView maximumZoomScale), dot indicator, and close button - Tappable hero image on detail screen opens viewer at index 0; expand icon hint in corner when images are present - Horizontal thumbnail strip below hero when item has multiple images; tapping any thumbnail opens viewer at that index
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
groupCatalogItemsdeduplication toCatalogBrowserModal— the paginated list, popular items, and recently used sections were all showing variants as separate repeated entries (same fix as PR fix(catalog): stable pagination + variant grouping #2572 which only coveredCatalogItemsScreen)useCatalogItemsInfinitelimit from 20 → 80 on both the catalog tab and browser modal — at ~6–7 variants per unique product, 20 raw items was collapsing to ~3 visible groups per page and immediately re-triggering the infinite scroll loaderTest plan
Summary by CodeRabbit