Enhance catalog browser with Popular Items, Recently Used, and per-item quantity adjustment#1900
Enhance catalog browser with Popular Items, Recently Used, and per-item quantity adjustment#1900
Conversation
…tem quantity adjustment Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1452 🏁 Script executed: Length of output: 472 🏁 Script executed: Length of output: 16159 🏁 Script executed: Length of output: 50805 🏁 Script executed: Length of output: 26969
Code Review SummaryThe core feature implementation is solid — the atom, hooks, and quantity panel all work together coherently. Several issues deserve attention before merge, ranging from a UX bug to unrelated regressions in the landing app. 🐛 Bug — Search not cleared after "Add Items"File:
// handleAddSelected — missing setSearchValue('')
const handleAddSelected = () => {
const selectedCatalogItems = Array.from(selectedItems)
.map((id) => allAvailableItems.get(id))
.filter((item): item is CatalogItem => item !== undefined)
.map((item) => ({ ...item, quantity: itemQuantities.get(item.id) ?? 1 }));
onItemsSelected(selectedCatalogItems);
resetSelection();
setSearchValue(''); // ← add this
onClose();
};🐛 Bug / Inconsistency — Missing empty-array guard in
|
|
@copilot CodeRabbit has reviewed this PR and provided feedback. Please address the review comments and push updates. Once you've made the changes, the monitor will ask CodeRabbit to re-review. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change adds catalog item tracking and quick-access features by introducing a recently-used items atom with async storage, hooks for fetching popular and recent items, and enhancing the catalog browser modal with per-item quantity controls. Two pack-related components now track when users select catalog items. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Modal as CatalogBrowserModal
participant Hook as useRecentlyUsedCatalogItems
participant Atom as recentlyUsedCatalogItemsAtom
participant Storage as AsyncStorage
User->>Modal: Opens catalog browser
Modal->>Hook: Calls useRecentlyUsedCatalogItems()
Hook->>Atom: Reads current recently used items
Atom->>Storage: Fetches from async storage
Storage-->>Atom: Returns stored items (max 10)
Atom-->>Hook: Provides recentItems
Hook-->>Modal: Returns recentItems + trackRecentlyUsed callback
Modal->>Modal: Renders QuickAccessSection with recent items
User->>Modal: Selects catalog item(s) and adjusts quantities
Modal->>Modal: Updates itemQuantities state
User->>Modal: Taps "Add" button
Modal->>Hook: Calls trackRecentlyUsed(selectedItems)
Hook->>Hook: Calls buildUpdatedRecentlyUsed(current, added)
Hook->>Hook: Deduplicates, prepends new items, truncates to 10
Hook->>Atom: Updates atom with new list
Atom->>Storage: Persists updated items to async storage
Storage-->>Atom: Acknowledged
Modal->>Modal: Resets selection and quantities
Modal->>User: Returns selected items with quantities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/expo/features/pack-templates/components/AddPackTemplateItemActions.tsx (1)
117-126:⚠️ Potential issue | 🟠 MajorAdd guard for empty selection and error handling.
Unlike
AddPackItemActions, this handler callstrackRecentlyUsedunconditionally, causing unnecessary AsyncStorage writes whencatalogItemsis empty. Additionally,addItemsToPackTemplatelacks error handling—if the API fails, the success toast will still show.🛡️ Proposed fix with guard and error handling
const handleCatalogItemsSelected = async (catalogItems: CatalogItem[]) => { - trackRecentlyUsed(catalogItems); - await addItemsToPackTemplate(packTemplateId, catalogItems as CatalogItemWithPackItemFields[]); - const itemWord = - catalogItems.length === 1 ? t('packTemplates.item') : t('packTemplates.items'); - Toast.show({ - type: 'success', - text1: t('packTemplates.addedItems', { count: catalogItems.length, itemWord }), - }); + if (catalogItems.length === 0) return; + trackRecentlyUsed(catalogItems); + try { + await addItemsToPackTemplate(packTemplateId, catalogItems as CatalogItemWithPackItemFields[]); + const itemWord = + catalogItems.length === 1 ? t('packTemplates.item') : t('packTemplates.items'); + Toast.show({ + type: 'success', + text1: t('packTemplates.addedItems', { count: catalogItems.length, itemWord }), + }); + } catch (err) { + console.error('Failed to add items to template:', err); + Toast.show({ + type: 'error', + text1: t('packTemplates.failedToSaveItem'), + }); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/pack-templates/components/AddPackTemplateItemActions.tsx` around lines 117 - 126, In handleCatalogItemsSelected, add a guard so nothing runs if catalogItems is empty (do not call trackRecentlyUsed or proceed) and wrap the call to addItemsToPackTemplate(...) in a try/catch; only call trackRecentlyUsed when catalogItems.length > 0, await addItemsToPackTemplate inside try, show the success Toast.show(...) on success, and on catch log/report the error and show a failure toast instead of the success message so failures don't incorrectly notify success.
🧹 Nitpick comments (2)
apps/expo/features/catalog/components/CatalogBrowserModal.tsx (2)
153-160: Consider using local state for search instead of global atom.Using the global
searchValueAtomfor modal-local search can leak state to other screens that consume this atom. If the modal is closed without clearing (e.g., via gesture), the search value persists and may cause unexpected behavior when reopening the modal or on other screens.♻️ Proposed refactor to use local state
-import { searchValueAtom } from 'expo-app/atoms/itemListAtoms'; ... - const [searchValue, setSearchValue] = useAtom(searchValueAtom); + const [searchValue, setSearchValue] = useState('');This isolates the search state to the modal lifecycle and eliminates the need to manually clear it on close.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/catalog/components/CatalogBrowserModal.tsx` around lines 153 - 160, The modal currently uses the global searchValueAtom (searchValue, setSearchValue) which can leak state across screens; change CatalogBrowserModal to use a local state (e.g., const [localSearch, setLocalSearch] = useState('')) in place of searchValueAtom, wire debouncedSearchValue = useDebounce(localSearch, 400) and replace references to searchValue and setSearchValue with localSearch and setLocalSearch, remove the atom usage/import, and ensure isSearching/isDefaultView derive from debouncedSearchValue and activeFilter so the modal search state is fully isolated and no external clearing is required when the modal closes.
204-221: Consider functional update pattern to avoid stale closure.
handleItemTogglereadsselectedItemsdirectly and includes it in dependencies, causing the callback to be recreated on every selection change. This triggers unnecessary re-renders inListHeaderComponent. Using functional updates would allow an empty dependency array.♻️ Proposed refactor using functional updates
const handleItemToggle = useCallback( (item: CatalogItem) => { - const newSelected = new Set(selectedItems); - if (newSelected.has(item.id)) { - newSelected.delete(item.id); + setSelectedItems((prev) => { + const newSelected = new Set(prev); + const wasSelected = newSelected.has(item.id); + if (wasSelected) { + newSelected.delete(item.id); + } else { + newSelected.add(item.id); + } + return newSelected; + }); + setItemQuantities((prev) => { + const next = new Map(prev); + if (prev.has(item.id)) { + next.delete(item.id); + } else { + next.set(item.id, 1); + } + return next; + }); - setItemQuantities((prev) => { - const next = new Map(prev); - next.delete(item.id); - return next; - }); - } else { - newSelected.add(item.id); - setItemQuantities((prev) => new Map(prev).set(item.id, 1)); - } - setSelectedItems(newSelected); }, - [selectedItems], + [], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/catalog/components/CatalogBrowserModal.tsx` around lines 204 - 221, handleItemToggle closes over selectedItems causing it to be recreated on every change; refactor to use functional updates so the callback doesn't depend on external selectedItems. In CatalogBrowserModal.tsx, update handleItemToggle to call setSelectedItems(prev => { const next = new Set(prev); ... return next; }) and setItemQuantities(prev => { const next = new Map(prev); ... return next; }) instead of reading selectedItems directly, then set the useCallback dependency array to [] (or only stable refs) so ListHeaderComponent won't re-render unnecessarily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/features/catalog/components/CatalogBrowserModal.tsx`:
- Around line 231-239: In handleAddSelected, after calling resetSelection() and
before onClose(), also clear the search state (reset the searchValue used by
this component) so reopening the modal shows the default item list; update the
function that currently references selectedItems, allAvailableItems,
itemQuantities, resetSelection, and onClose to call the search clearing handler
or setSearchValue('') (or equivalent) to reset the search input when items are
added.
In `@apps/expo/features/packs/components/AddPackItemActions.tsx`:
- Around line 103-108: The handler handleCatalogItemsSelected is awaiting
addItemsToPack without error handling and uses an unnecessary cast on packId;
wrap the addItemsToPack call in a try/catch inside handleCatalogItemsSelected,
remove the redundant "packId as string" cast (use packId directly), and in the
catch block log the error and surface user feedback (e.g., set an error state or
call the existing toast/notification helper) so failures are visible to users;
keep trackRecentlyUsed(catalogItems) as-is but ensure any UI state updates
(loading/disabled) are handled around the try/catch.
In `@apps/expo/lib/i18n/locales/en.json`:
- Around line 520-524: The JSON contains unused i18n keys "quickAdd",
"adjustQuantity", and "tapToAdjust"; remove these three entries from
apps/expo/lib/i18n/locales/en.json and also delete the same keys from any other
locale files (or remove them from your translations source) and run a
project-wide search for "quickAdd", "adjustQuantity", and "tapToAdjust" to
confirm they are not referenced before committing.
---
Outside diff comments:
In `@apps/expo/features/pack-templates/components/AddPackTemplateItemActions.tsx`:
- Around line 117-126: In handleCatalogItemsSelected, add a guard so nothing
runs if catalogItems is empty (do not call trackRecentlyUsed or proceed) and
wrap the call to addItemsToPackTemplate(...) in a try/catch; only call
trackRecentlyUsed when catalogItems.length > 0, await addItemsToPackTemplate
inside try, show the success Toast.show(...) on success, and on catch log/report
the error and show a failure toast instead of the success message so failures
don't incorrectly notify success.
---
Nitpick comments:
In `@apps/expo/features/catalog/components/CatalogBrowserModal.tsx`:
- Around line 153-160: The modal currently uses the global searchValueAtom
(searchValue, setSearchValue) which can leak state across screens; change
CatalogBrowserModal to use a local state (e.g., const [localSearch,
setLocalSearch] = useState('')) in place of searchValueAtom, wire
debouncedSearchValue = useDebounce(localSearch, 400) and replace references to
searchValue and setSearchValue with localSearch and setLocalSearch, remove the
atom usage/import, and ensure isSearching/isDefaultView derive from
debouncedSearchValue and activeFilter so the modal search state is fully
isolated and no external clearing is required when the modal closes.
- Around line 204-221: handleItemToggle closes over selectedItems causing it to
be recreated on every change; refactor to use functional updates so the callback
doesn't depend on external selectedItems. In CatalogBrowserModal.tsx, update
handleItemToggle to call setSelectedItems(prev => { const next = new Set(prev);
... return next; }) and setItemQuantities(prev => { const next = new Map(prev);
... return next; }) instead of reading selectedItems directly, then set the
useCallback dependency array to [] (or only stable refs) so ListHeaderComponent
won't re-render unnecessarily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29ed9152-c575-426c-a4e1-2631573ace51
📒 Files selected for processing (8)
apps/expo/atoms/recentlyUsedCatalogItemsAtom.tsapps/expo/features/catalog/components/CatalogBrowserModal.tsxapps/expo/features/catalog/hooks/index.tsapps/expo/features/catalog/hooks/usePopularCatalogItems.tsapps/expo/features/catalog/hooks/useRecentlyUsedCatalogItems.tsapps/expo/features/pack-templates/components/AddPackTemplateItemActions.tsxapps/expo/features/packs/components/AddPackItemActions.tsxapps/expo/lib/i18n/locales/en.json
| const handleAddSelected = () => { | ||
| const selectedCatalogItems = items.filter((item: CatalogItem) => selectedItems.has(item.id)); | ||
| const selectedCatalogItems = Array.from(selectedItems) | ||
| .map((id) => allAvailableItems.get(id)) | ||
| .filter((item): item is CatalogItem => item !== undefined) | ||
| .map((item) => ({ ...item, quantity: itemQuantities.get(item.id) ?? 1 })); | ||
| onItemsSelected(selectedCatalogItems); | ||
| setSelectedItems(new Set()); | ||
| resetSelection(); | ||
| onClose(); | ||
| }; |
There was a problem hiding this comment.
Search value not cleared after adding items.
handleAddSelected resets selection and closes the modal but doesn't clear searchValue. If the modal is reopened, it will show the previous search results. Consider clearing the search value here as well.
🔧 Proposed fix
const handleAddSelected = () => {
const selectedCatalogItems = Array.from(selectedItems)
.map((id) => allAvailableItems.get(id))
.filter((item): item is CatalogItem => item !== undefined)
.map((item) => ({ ...item, quantity: itemQuantities.get(item.id) ?? 1 }));
onItemsSelected(selectedCatalogItems);
resetSelection();
+ setSearchValue('');
onClose();
};📝 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.
| const handleAddSelected = () => { | |
| const selectedCatalogItems = items.filter((item: CatalogItem) => selectedItems.has(item.id)); | |
| const selectedCatalogItems = Array.from(selectedItems) | |
| .map((id) => allAvailableItems.get(id)) | |
| .filter((item): item is CatalogItem => item !== undefined) | |
| .map((item) => ({ ...item, quantity: itemQuantities.get(item.id) ?? 1 })); | |
| onItemsSelected(selectedCatalogItems); | |
| setSelectedItems(new Set()); | |
| resetSelection(); | |
| onClose(); | |
| }; | |
| const handleAddSelected = () => { | |
| const selectedCatalogItems = Array.from(selectedItems) | |
| .map((id) => allAvailableItems.get(id)) | |
| .filter((item): item is CatalogItem => item !== undefined) | |
| .map((item) => ({ ...item, quantity: itemQuantities.get(item.id) ?? 1 })); | |
| onItemsSelected(selectedCatalogItems); | |
| resetSelection(); | |
| setSearchValue(''); | |
| onClose(); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/catalog/components/CatalogBrowserModal.tsx` around lines
231 - 239, In handleAddSelected, after calling resetSelection() and before
onClose(), also clear the search state (reset the searchValue used by this
component) so reopening the modal shows the default item list; update the
function that currently references selectedItems, allAvailableItems,
itemQuantities, resetSelection, and onClose to call the search clearing handler
or setSearchValue('') (or equivalent) to reset the search input when items are
added.
| const handleCatalogItemsSelected = async (catalogItems: CatalogItem[]) => { | ||
| if (catalogItems.length > 0) { | ||
| trackRecentlyUsed(catalogItems); | ||
| await addItemsToPack(packId as string, catalogItems as CatalogItemWithPackItemFields[]); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for the network call.
The addItemsToPack call is awaited but not wrapped in try/catch. If the API fails, users won't receive feedback. Also, the packId as string cast on line 106 is redundant since packId is already typed as string in the props interface.
🛡️ Proposed fix with error handling
const handleCatalogItemsSelected = async (catalogItems: CatalogItem[]) => {
if (catalogItems.length > 0) {
trackRecentlyUsed(catalogItems);
- await addItemsToPack(packId as string, catalogItems as CatalogItemWithPackItemFields[]);
+ try {
+ await addItemsToPack(packId, catalogItems as CatalogItemWithPackItemFields[]);
+ } catch (err) {
+ console.error('Failed to add items to pack:', err);
+ // Consider showing a toast or alert to the user
+ }
}
};📝 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.
| const handleCatalogItemsSelected = async (catalogItems: CatalogItem[]) => { | |
| if (catalogItems.length > 0) { | |
| trackRecentlyUsed(catalogItems); | |
| await addItemsToPack(packId as string, catalogItems as CatalogItemWithPackItemFields[]); | |
| } | |
| }; | |
| const handleCatalogItemsSelected = async (catalogItems: CatalogItem[]) => { | |
| if (catalogItems.length > 0) { | |
| trackRecentlyUsed(catalogItems); | |
| try { | |
| await addItemsToPack(packId, catalogItems as CatalogItemWithPackItemFields[]); | |
| } catch (err) { | |
| console.error('Failed to add items to pack:', err); | |
| // Consider showing a toast or alert to the user | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/packs/components/AddPackItemActions.tsx` around lines 103
- 108, The handler handleCatalogItemsSelected is awaiting addItemsToPack without
error handling and uses an unnecessary cast on packId; wrap the addItemsToPack
call in a try/catch inside handleCatalogItemsSelected, remove the redundant
"packId as string" cast (use packId directly), and in the catch block log the
error and surface user feedback (e.g., set an error state or call the existing
toast/notification helper) so failures are visible to users; keep
trackRecentlyUsed(catalogItems) as-is but ensure any UI state updates
(loading/disabled) are handled around the try/catch.
| "quickAdd": "Quick Add", | ||
| "adjustQuantity": "Adjust Quantity", | ||
| "selectedItemsQuantity": "Selected Items ({{count}})", | ||
| "noRecentlyUsedItems": "No recently used items yet", | ||
| "tapToAdjust": "Tap items to select, adjust quantities below", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of these i18n keys in the codebase
rg -n "catalog\.quickAdd|catalog\.adjustQuantity|catalog\.tapToAdjust" --type ts --type tsxRepository: PackRat-AI/PackRat
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for usage of these i18n keys in TypeScript/JavaScript files
rg -n "quickAdd|adjustQuantity|tapToAdjust" --type ts -A 2 -B 2Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for these strings in all files
rg -n "quickAdd|adjustQuantity|tapToAdjust" .
# Also check how i18n keys are typically accessed
echo "---"
echo "Checking how i18n is accessed in the codebase:"
rg -n "catalog\[" --max-count=5
# Check for the CatalogBrowserModal file
echo "---"
echo "Checking CatalogBrowserModal file:"
fd -n "CatalogBrowserModal" -t fRepository: PackRat-AI/PackRat
Length of output: 659
🏁 Script executed:
#!/bin/bash
# Search for CatalogBrowserModal or related files
fd "Catalog" -t f
# Check other locale files to see the pattern
echo "---"
echo "Checking all locale files:"
ls -la ./apps/expo/lib/i18n/locales/
# Search broadly for references to these strings without quotes
echo "---"
echo "Searching for any reference in all files:"
rg "quickAdd|adjustQuantity|tapToAdjust" --max-count=10 .Repository: PackRat-AI/PackRat
Length of output: 2461
🏁 Script executed:
#!/bin/bash
# Check the CatalogBrowserModal file
cat -n ./apps/expo/features/catalog/components/CatalogBrowserModal.tsx | head -100Repository: PackRat-AI/PackRat
Length of output: 3789
🏁 Script executed:
#!/bin/bash
# Search for t( calls in CatalogBrowserModal.tsx
rg "t\(" ./apps/expo/features/catalog/components/CatalogBrowserModal.tsx -A 1 -B 1Repository: PackRat-AI/PackRat
Length of output: 3388
Remove unused i18n keys.
The keys quickAdd, adjustQuantity, and tapToAdjust are defined but not referenced anywhere in the codebase. Remove them to reduce translation maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/lib/i18n/locales/en.json` around lines 520 - 524, The JSON contains
unused i18n keys "quickAdd", "adjustQuantity", and "tapToAdjust"; remove these
three entries from apps/expo/lib/i18n/locales/en.json and also delete the same
keys from any other locale files (or remove them from your translations source)
and run a project-wide search for "quickAdd", "adjustQuantity", and
"tapToAdjust" to confirm they are not referenced before committing.
|
@copilot Please address the CodeRabbit review feedback on this PR (3 inline comments). |
… through guard Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Addressed the three CodeRabbit inline comments (commit
|
|
@copilot Reminder: please address the CodeRabbit review feedback on this PR. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
recentlyUsedCatalogItemsAtomin atoms/ with AsyncStorage persistenceuseRecentlyUsedCatalogItemshook in catalog/hooks/usePopularCatalogItemshook in catalog/hooks/canSave(trimmedFirst/trimmedLast)handleSaveguard so return key on last field and both Save buttons share samecanSavecheckOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
New Features
Documentation