🎨 app: improve tab press refresh #655
Conversation
🦋 Changeset detectedLatest commit: d4c1c5f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughIntroduces a centralized tab-press system (new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
Summary of ChangesHello @dieguezguille, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the application's tab navigation refresh mechanism to enhance modularity and maintainability. By introducing a custom Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement to the tab press handling logic. By implementing a pub/sub pattern with the new useTabPress hook, you've successfully decoupled the tab layout from the individual screen components. This removes the need for manually exporting and managing ref objects, leading to much cleaner, more maintainable, and less error-error-prone code. The refactoring has been applied consistently across all tab screens. Additionally, this change fixes a bug with stickyHeaderIndices in the Activity screen. Overall, this is an excellent refactoring.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| queryClient.refetchQueries({ queryKey }).catch(reportError); | ||
| }; | ||
| useTabPress("activity", () => { | ||
| if (data.length > 0) listRef.current?.scrollToIndex({ index: 0, animated: true }); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Auth.tsx and check if it has onScrollToIndexFailed handler
fd -e tsx -e ts "Auth.tsx" --exec cat {}
Length of output: 10028
🏁 Script executed:
#!/bin/bash
# Search for all usages of scrollToIndex in the codebase
rg -n "scrollToIndex" --type=ts --type=tsx -C3
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for onScrollToIndexFailed in the codebase
rg -n "onScrollToIndexFailed" --type=ts --type=tsx -C3
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for all usages of scrollToIndex in the codebase with correct syntax
rg -n "scrollToIndex" -C3
Length of output: 937
🏁 Script executed:
#!/bin/bash
# Search for onScrollToIndexFailed in the codebase
rg -n "onScrollToIndexFailed" -C3
Length of output: 516
@cruzdanilo, I've reviewed the finding and can confirm this is a valid issue.
Analysis:
The Sentry bot is correct on all points:
- ✅ Auth.tsx does implement the handler (line 129):
onScrollToIndexFailed={() => undefined} - ✅ Activity.tsx is missing it - the FlatList component doesn't have this prop
- ✅ The
data.length > 0check is insufficient - it only verifies that data exists, not that the item at index 0 has been rendered and measured by the FlatList
Why this matters:
FlatList renders items lazily for performance. When scrollToIndex is called, React Native expects the target item to already be rendered and measured. If a user quickly taps the Activity tab after app launch (before the list has fully laid out), the call to scrollToIndex({ index: 0, ... }) can fail and trigger a warning or error.
Recommended fix:
Add the onScrollToIndexFailed prop to the FlatList in Activity.tsx, matching the pattern already established in Auth.tsx:
<FlatList<ActivityItemType>
ref={listRef}
style={{ flex: 1 }}
contentContainerStyle={{
flexGrow: 1,
backgroundColor: data.length > 0 ? theme.backgroundMild.val : theme.backgroundSoft.val,
}}
showsVerticalScrollIndicator={false}
onScrollToIndexFailed={() => undefined}
refreshControl={<RefreshControl refreshing={isPending} onRefresh={refresh} />}
// ... rest of propsThis prevents the app from crashing or showing warnings when the scroll fails.
![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
🤖 Fix all issues with AI agents
In `@src/components/card/Card.tsx`:
- Around line 124-136: The pull-to-refresh spinner is tied only to purchases'
isPending while refresh() fires six parallel refetches (refetchCard,
refetchPurchases, refetchMarkets, refetchKYCStatus, refetchInstalledPlugins and
queryClient.refetchQueries), so the spinner can stop before all work finishes;
fix by deriving a combined refreshing state (either compute isRefreshing =
isPendingCard || isPendingPurchases || isPendingMarkets || isPendingKYC ||
isPendingPlugins || isRefetchingQueries, or manage a local isRefreshing state:
set true at start of refresh(), await Promise.allSettled([...refetchCard(),
refetchPurchases(), refetchMarkets(), refetchKYCStatus(),
refetchInstalledPlugins(), queryClient.refetchQueries({ queryKey })]) and set
false in finally) and pass that combined isRefreshing to the RefreshControl's
refreshing prop instead of using only purchases' isPending.
In `@src/utils/useTabPress.ts`:
- Around line 23-26: The emitTabPress function currently invokes all subscriber
handlers sequentially but a thrown exception from one will stop subsequent
handlers; to fix, wrap each handler invocation in its own try/catch inside
emitTabPress (referencing emitTabPress and subscribers.get(name)), so you call
every handler even if one fails, and in the catch log the error with context
(include the tab name and the handler identity) rather than letting the
exception propagate.
- Line 28: Move the module-level state declaration "const subscribers = new
Map<TabName, Set<() => void>>();" to the top of the module (above any functions
or usages like useTabPress and any references to TabName) so the state is
declared before it's referenced; ensure the TabName type/import remains
available above that line and adjust ordering so all functions (e.g.,
useTabPress) and helpers read the hoisted subscribers variable after it's
declared.
dieguezguille
force-pushed
the
refresh
branch
from
c49f7a5 to
d4c1c5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/components/activity/Activity.tsx`:
- Around line 56-64: The multi-line arrow functions should be converted to
function declarations: change the refresh arrow function (const refresh = () =>
{ ... }) into a function declaration (function refresh() { ... }) and extract
the inline multi-line handler passed to useTabPress into a named function (e.g.,
function handleTabPress() { if (data.length > 0)
listRef.current?.scrollToIndex({ index: 0, animated: true }); refresh(); }) then
pass that name to useTabPress("activity", handleTabPress); keep references to
listRef, refetch, queryClient, queryKey, data and reportError unchanged.
In `@src/components/card/Card.tsx`:
- Around line 133-148: Convert the multi-line arrow functions to function
declarations: replace the const refresh = () => { ... } with a function
refresh() { refetchCard().catch(reportError);
refetchPurchases().catch(reportError); refetchMarkets().catch(reportError);
refetchKYCStatus().catch(reportError);
refetchInstalledPlugins().catch(reportError); queryClient.refetchQueries({
queryKey }).catch(reportError); } and extract the tab handler into a named
function (e.g., function onCardTabPress() { scrollRef.current?.scrollTo({ y: 0,
animated: true }); refresh(); }) then pass onCardTabPress to useTabPress("card",
onCardTabPress); keep existing symbols isRefreshing and scrollRef unchanged and
preserve the same error handling and behavior.
In `@src/components/home/Home.tsx`:
- Around line 126-138: The multi-line arrow function assigned to refresh should
be converted to a named function declaration to follow project conventions;
replace the const refresh = () => { ... } with function refresh() {
refetchActivity().catch(reportError); refetchBytecode().catch(reportError);
refetchMarkets().catch(reportError); refetchKYCStatus().catch(reportError);
refetchLegacyKYCStatus().catch(reportError);
refetchPendingProposals().catch(reportError); } and ensure existing references
(the useTabPress callback that calls refresh and scrollRef.current?.scrollTo)
continue to work unchanged; keep the behavior identical and retain reportError
handling.
In `@src/components/pay-mode/PayMode.tsx`:
- Around line 34-43: The multi-line arrow functions should be converted to
function declarations: replace the const refresh = () => { ... } with a function
refresh() { refetch().catch(reportError); queryClient.refetchQueries({ queryKey:
["activity"] }).catch(reportError); } and extract the multi-statement tab-press
callback into a named function (e.g., function onPayModeTabPress() {
scrollRef.current?.scrollTo({ y: 0, animated: true }); refresh(); }) then pass
onPayModeTabPress to useTabPress("pay-mode", onPayModeTabPress); update
references to refresh and the new handler accordingly.
| const listRef = useRef<FlatList<ActivityItemType>>(null); | ||
| const refresh = () => { | ||
| refetch().catch(reportError); | ||
| queryClient.refetchQueries({ queryKey }).catch(reportError); | ||
| }; | ||
| useTabPress("activity", () => { | ||
| if (data.length > 0) listRef.current?.scrollToIndex({ index: 0, animated: true }); | ||
| refresh(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer function declarations for multi-line handlers.
This keeps the component aligned with the repo’s function-style rule.
♻️ Suggested refactor
- const refresh = () => {
+ function refresh() {
refetch().catch(reportError);
queryClient.refetchQueries({ queryKey }).catch(reportError);
- };
- useTabPress("activity", () => {
+ }
+
+ function handleTabPress() {
if (data.length > 0) listRef.current?.scrollToIndex({ index: 0, animated: true });
refresh();
- });
+ }
+ useTabPress("activity", handleTabPress);As per coding guidelines, prefer function declarations for multi-line functions.
🤖 Prompt for AI Agents
In `@src/components/activity/Activity.tsx` around lines 56 - 64, The multi-line
arrow functions should be converted to function declarations: change the refresh
arrow function (const refresh = () => { ... }) into a function declaration
(function refresh() { ... }) and extract the inline multi-line handler passed to
useTabPress into a named function (e.g., function handleTabPress() { if
(data.length > 0) listRef.current?.scrollToIndex({ index: 0, animated: true });
refresh(); }) then pass that name to useTabPress("activity", handleTabPress);
keep references to listRef, refetch, queryClient, queryKey, data and reportError
unchanged.
| const isRefreshing = | ||
| isFetchingCard || isFetchingPurchases || isFetchingMarkets || isFetchingKYC || isFetchingPlugins || isFetchingAsset; | ||
|
|
||
| const scrollRef = useRef<ScrollView>(null); | ||
| const refresh = () => { | ||
| refetchCard().catch(reportError); | ||
| refetchPurchases().catch(reportError); | ||
| refetchMarkets().catch(reportError); | ||
| refetchKYCStatus().catch(reportError); | ||
| refetchInstalledPlugins().catch(reportError); | ||
| queryClient.refetchQueries({ queryKey }).catch(reportError); | ||
| }; | ||
| useTabPress("card", () => { | ||
| scrollRef.current?.scrollTo({ y: 0, animated: true }); | ||
| refresh(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer function declarations for the multi-line refresh/tab handlers.
Keeps this aligned with the repo’s function-style rule.
♻️ Suggested refactor
- const refresh = () => {
+ function refresh() {
refetchCard().catch(reportError);
refetchPurchases().catch(reportError);
refetchMarkets().catch(reportError);
refetchKYCStatus().catch(reportError);
refetchInstalledPlugins().catch(reportError);
queryClient.refetchQueries({ queryKey }).catch(reportError);
- };
- useTabPress("card", () => {
+ }
+ function handleTabPress() {
scrollRef.current?.scrollTo({ y: 0, animated: true });
refresh();
- });
+ }
+ useTabPress("card", handleTabPress);As per coding guidelines, prefer function declarations for multi-line functions.
🤖 Prompt for AI Agents
In `@src/components/card/Card.tsx` around lines 133 - 148, Convert the multi-line
arrow functions to function declarations: replace the const refresh = () => {
... } with a function refresh() { refetchCard().catch(reportError);
refetchPurchases().catch(reportError); refetchMarkets().catch(reportError);
refetchKYCStatus().catch(reportError);
refetchInstalledPlugins().catch(reportError); queryClient.refetchQueries({
queryKey }).catch(reportError); } and extract the tab handler into a named
function (e.g., function onCardTabPress() { scrollRef.current?.scrollTo({ y: 0,
animated: true }); refresh(); }) then pass onCardTabPress to useTabPress("card",
onCardTabPress); keep existing symbols isRefreshing and scrollRef unchanged and
preserve the same error handling and behavior.
| const scrollRef = useRef<ScrollView>(null); | ||
| const refresh = () => { | ||
| refetchActivity().catch(reportError); | ||
| refetchBytecode().catch(reportError); | ||
| refetchMarkets().catch(reportError); | ||
| refetchKYCStatus().catch(reportError); | ||
| refetchLegacyKYCStatus().catch(reportError); | ||
| refetchPendingProposals().catch(reportError); | ||
| }; | ||
| useTabPress("index", () => { | ||
| scrollRef.current?.scrollTo({ y: 0, animated: true }); | ||
| refresh(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using function declaration for multi-line functions.
Per coding guidelines, prefer function declarations for multi-line functions. The arrow function works but a function declaration would be more consistent with project conventions.
♻️ Suggested refactor
const scrollRef = useRef<ScrollView>(null);
- const refresh = () => {
+ function refresh() {
refetchActivity().catch(reportError);
refetchBytecode().catch(reportError);
refetchMarkets().catch(reportError);
refetchKYCStatus().catch(reportError);
refetchLegacyKYCStatus().catch(reportError);
refetchPendingProposals().catch(reportError);
- };
+ }
useTabPress("index", () => {
scrollRef.current?.scrollTo({ y: 0, animated: true });
refresh();
});📝 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 scrollRef = useRef<ScrollView>(null); | |
| const refresh = () => { | |
| refetchActivity().catch(reportError); | |
| refetchBytecode().catch(reportError); | |
| refetchMarkets().catch(reportError); | |
| refetchKYCStatus().catch(reportError); | |
| refetchLegacyKYCStatus().catch(reportError); | |
| refetchPendingProposals().catch(reportError); | |
| }; | |
| useTabPress("index", () => { | |
| scrollRef.current?.scrollTo({ y: 0, animated: true }); | |
| refresh(); | |
| }); | |
| const scrollRef = useRef<ScrollView>(null); | |
| function refresh() { | |
| refetchActivity().catch(reportError); | |
| refetchBytecode().catch(reportError); | |
| refetchMarkets().catch(reportError); | |
| refetchKYCStatus().catch(reportError); | |
| refetchLegacyKYCStatus().catch(reportError); | |
| refetchPendingProposals().catch(reportError); | |
| } | |
| useTabPress("index", () => { | |
| scrollRef.current?.scrollTo({ y: 0, animated: true }); | |
| refresh(); | |
| }); |
🤖 Prompt for AI Agents
In `@src/components/home/Home.tsx` around lines 126 - 138, The multi-line arrow
function assigned to refresh should be converted to a named function declaration
to follow project conventions; replace the const refresh = () => { ... } with
function refresh() { refetchActivity().catch(reportError);
refetchBytecode().catch(reportError); refetchMarkets().catch(reportError);
refetchKYCStatus().catch(reportError);
refetchLegacyKYCStatus().catch(reportError);
refetchPendingProposals().catch(reportError); } and ensure existing references
(the useTabPress callback that calls refresh and scrollRef.current?.scrollTo)
continue to work unchanged; keep the behavior identical and retain reportError
handling.
|
|
||
| const scrollRef = useRef<ScrollView>(null); | ||
| const refresh = () => { | ||
| refetch().catch(reportError); | ||
| queryClient.refetchQueries({ queryKey: ["activity"] }).catch(reportError); | ||
| }; | ||
| useTabPress("pay-mode", () => { | ||
| scrollRef.current?.scrollTo({ y: 0, animated: true }); | ||
| refresh(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using function declaration for multi-line functions.
Same style suggestion as in Home.tsx - prefer function declarations for multi-line functions per coding guidelines.
♻️ Suggested refactor
const scrollRef = useRef<ScrollView>(null);
- const refresh = () => {
+ function refresh() {
refetch().catch(reportError);
queryClient.refetchQueries({ queryKey: ["activity"] }).catch(reportError);
- };
+ }
useTabPress("pay-mode", () => {
scrollRef.current?.scrollTo({ y: 0, animated: true });
refresh();
});📝 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 scrollRef = useRef<ScrollView>(null); | |
| const refresh = () => { | |
| refetch().catch(reportError); | |
| queryClient.refetchQueries({ queryKey: ["activity"] }).catch(reportError); | |
| }; | |
| useTabPress("pay-mode", () => { | |
| scrollRef.current?.scrollTo({ y: 0, animated: true }); | |
| refresh(); | |
| }); | |
| const scrollRef = useRef<ScrollView>(null); | |
| function refresh() { | |
| refetch().catch(reportError); | |
| queryClient.refetchQueries({ queryKey: ["activity"] }).catch(reportError); | |
| } | |
| useTabPress("pay-mode", () => { | |
| scrollRef.current?.scrollTo({ y: 0, animated: true }); | |
| refresh(); | |
| }); |
🤖 Prompt for AI Agents
In `@src/components/pay-mode/PayMode.tsx` around lines 34 - 43, The multi-line
arrow functions should be converted to function declarations: replace the const
refresh = () => { ... } with a function refresh() {
refetch().catch(reportError); queryClient.refetchQueries({ queryKey:
["activity"] }).catch(reportError); } and extract the multi-statement tab-press
callback into a named function (e.g., function onPayModeTabPress() {
scrollRef.current?.scrollTo({ y: 0, animated: true }); refresh(); }) then pass
onPayModeTabPress to useTabPress("pay-mode", onPayModeTabPress); update
references to refresh and the new handler accordingly.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.