fix(catalog/ios): hide "Showing X of Y items" label when search overlay is active#1894
fix(catalog/ios): hide "Showing X of Y items" label when search overlay is active#1894
Conversation
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: 9809
✅ Fix AssessmentThe core change — gating 🔴 Critical – Potential Runtime Crash:
|
| Severity | Issue |
|---|---|
| 🔴 Critical | searchResults is not null-safe — will crash when vectorResult is undefined |
| 🟡 Medium | _isVectorFetching destructured but unused |
| 🟡 Medium | CategoriesFilter still visible during search overlay |
| 🟢 Minor | Stats computed unnecessarily while searching |
The fix is correct and targets the right layer of the problem. Addressing the searchResults ?? [] null-safety issue before merging is strongly recommended.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe CatalogItemsScreen now conditionally renders its header based on search state. When a user searches (searchValue is not empty), the header and "Showing X of Y items" label are completely hidden to prevent visual overlap with the search overlay, resolving the visual layering issue. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 1
🤖 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/screens/CatalogItemsScreen.tsx`:
- Around line 200-209: The header currently bases visibility on raw searchValue
which remounts immediately; change it to use the debounced/search-overlay flag
so it stays hidden until the debounce window closes. In CatalogItemsScreen
replace the condition that uses searchValue.length === 0 with the debounced
indicator (isSearching or the debouncedSearchValue check used by the overlay) so
the header’s render (the View containing totalItemsText and showingText) is
keyed off the same "search overlay visible" signal as the overlay, ensuring the
header remains hidden while isSearching is true and only shows after debouncing
completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e99200f1-eb60-4613-8c92-62675ca72f1e
📒 Files selected for processing (1)
apps/expo/features/catalog/screens/CatalogItemsScreen.tsx
| searchValue.length === 0 ? ( | ||
| <View className="mb-4"> | ||
| <View className="flex-row items-center justify-between"> | ||
| <Text className="text-muted-foreground">{totalItemsText}</Text> | ||
| </View> | ||
| {paginatedItems.length > 0 && ( | ||
| <Text className="mt-1 text-xs text-muted-foreground">{showingText}</Text> | ||
| )} | ||
| </View> | ||
| {paginatedItems.length > 0 && ( | ||
| <Text className="mt-1 text-xs text-muted-foreground">{showingText}</Text> | ||
| )} | ||
| </View> | ||
| ) : null |
There was a problem hiding this comment.
Keep the header hidden until the debounce window closes.
Line 200 keys the header off raw searchValue, but the overlay above still keys off debounced isSearching. When the user clears the field, this remounts the header immediately while the search overlay can remain visible for another 400ms, so the same peek-through can briefly reappear.
Minimal fix
- searchValue.length === 0 ? (
+ searchValue.length === 0 && !isSearching ? (
<View className="mb-4">
<View className="flex-row items-center justify-between">
<Text className="text-muted-foreground">{totalItemsText}</Text>
</View>
{paginatedItems.length > 0 && (
<Text className="mt-1 text-xs text-muted-foreground">{showingText}</Text>
)}
</View>
) : nullLonger-term, it would be safer to drive both the overlay content and the hidden-underlay state from the same “search overlay visible” signal.
📝 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.
| searchValue.length === 0 ? ( | |
| <View className="mb-4"> | |
| <View className="flex-row items-center justify-between"> | |
| <Text className="text-muted-foreground">{totalItemsText}</Text> | |
| </View> | |
| {paginatedItems.length > 0 && ( | |
| <Text className="mt-1 text-xs text-muted-foreground">{showingText}</Text> | |
| )} | |
| </View> | |
| {paginatedItems.length > 0 && ( | |
| <Text className="mt-1 text-xs text-muted-foreground">{showingText}</Text> | |
| )} | |
| </View> | |
| ) : null | |
| searchValue.length === 0 && !isSearching ? ( | |
| <View className="mb-4"> | |
| <View className="flex-row items-center justify-between"> | |
| <Text className="text-muted-foreground">{totalItemsText}</Text> | |
| </View> | |
| {paginatedItems.length > 0 && ( | |
| <Text className="mt-1 text-xs text-muted-foreground">{showingText}</Text> | |
| )} | |
| </View> | |
| ) : null |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/catalog/screens/CatalogItemsScreen.tsx` around lines 200 -
209, The header currently bases visibility on raw searchValue which remounts
immediately; change it to use the debounced/search-overlay flag so it stays
hidden until the debounce window closes. In CatalogItemsScreen replace the
condition that uses searchValue.length === 0 with the debounced indicator
(isSearching or the debouncedSearchValue check used by the overlay) so the
header’s render (the View containing totalItemsText and showingText) is keyed
off the same "search overlay visible" signal as the overlay, ensuring the header
remains hidden while isSearching is true and only shows after debouncing
completes.
|
@copilot Please address the CodeRabbit review feedback on this PR (1 inline comments). |
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
1 similar comment
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
|
@copilot Reminder: please address the CodeRabbit review feedback on this PR. |
…ch is cleared Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
|
@coderabbitai review |
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
1 similar comment
|
@copilot CodeRabbit has reviewed this PR. Please address the feedback. |
ListHeaderComponentwhensearchValuehas text (searchValue.length === 0check)!isSearchingto prevent brief re-appearance during 400ms debounce window when user clears searchOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit