[FEAT] History list improvements#342
Conversation
| const year = date.getFullYear(); | ||
| const monthYear = `${month}:${year}`; | ||
|
|
||
| const lastSection = sections.length > 0 && sections[sections.length - 1]; |
There was a problem hiding this comment.
minor: do you need both checks? directly sections[sections.length -1] or !!sections.length would already identify it
There was a problem hiding this comment.
@leofelix077 what would happen when we have this case: const lastSection = sections[-1] ?
There was a problem hiding this comment.
I think javascript does not support it
There was a problem hiding this comment.
hummm it appears it would result in lastSection = undefined on javascript, which would work in this case. But I'd test it to make sure
There was a problem hiding this comment.
in JS an array is just an object under the hood at the end of day
const myArray = {
0: 'firstItem'
1: 'someitem'
}
myArray[-1] is undefined
There was a problem hiding this comment.
nice one, done!
|
|
||
| const { fetchAccountBalances, getBalances } = useBalancesStore.getState(); | ||
|
|
||
| await fetchAccountBalances({ |
There was a problem hiding this comment.
can this be a Promise.all with the getAccountHistory?
| history: historySections, | ||
| }; | ||
| }, | ||
| startPolling: (params) => { |
There was a problem hiding this comment.
general question: why do we need the polling?
There was a problem hiding this comment.
the initial idea was to keep the same pattern we use for balances where we have a central polling (on TabNavigator) which updates balances every 30 seconds. But it appears on this PR we don't have a central history polling but instead it starts/stops polling whenever users navigate to History screens which I agree does not seem very effective.
Ideally I think we should either have a central and constant polling or don't have the polling at all then just trigger a background refresh whenever user visits a history list (and take care not run concurrent history fetching).
There was a problem hiding this comment.
Yeah, the way I first did was wrong (polling only after it was fetched the first time). Now we are using a centralized polling system like the balance list
| error: shouldShowError ? error : null, | ||
| isLoading: shouldShowFullScreenLoading, | ||
| isRefreshing, | ||
| isNavigationRefresh: isNavigationRefresh || shouldRefreshAfterNavigation, |
There was a problem hiding this comment.
maybe we could have a useFocusEffect instead a param to refresh after navigation?
There was a problem hiding this comment.
I'm not against using useFocusEffect, but in case we decide on fetching history every time users navigate to history we will need to take care not to stack duplicate requests since it's a pretty heavy request
There was a problem hiding this comment.
since the idea is to prevent the fetch to happen every time we visit this screen, I prefer to stick with the param.
we have a controlled way to refresh, in this case we only force this refresh when we navigate from we finish a swap or send transaction
There was a problem hiding this comment.
I think the shouldRefreshAfterNavigation param works, but I have a suggestion to make it less specific so it's potentially easier to main:
I'm thinking maybe we could call something like fetchAccountHistory({ hasRecentTransaction }) right after the transaction submission is complete (or right before navigating to the history screen). Then on History screen we display the CustomRefreshIndicator if hasRecentTransaction && isFetching as I'm understanding that the goal of the custom shouldRefreshAfterNavigation param is to display the CustomRefreshIndicator so we make it clear for users that History is being updated after a recent transaction has been submitted.
Also, if users stay on the "Transaction Success" screen long enough for the fetchAccountHistory({ hasRecentTransaction }) fetching to complete I think it's fine to not display the CustomRefreshIndicator when they navigate to History as the recent transaction should already be appearing there.
What do you think?
There was a problem hiding this comment.
makes sense, I made the change and this helped to tone down the amount of code by a fair bit, thanks for the suggestion
| }, | ||
| "hideDust": { | ||
| "title": "Hide small payments", | ||
| "title": "Hide received payments smaller than 0.1 XLM", |
There was a problem hiding this comment.
is this intended? this is on the preferences screen. the title will break in two lines
There was a problem hiding this comment.
I think the title here should remain Hide small payments and the description should be updated to Hide received payments smaller than 0.1 XLM
There was a problem hiding this comment.
oh, my mistake, fixed!
| * This is typically called after completing a transaction (send/swap) to ensure | ||
| * the latest transaction appears in the history with a smooth UX | ||
| */ | ||
| export const markHistoryForRefreshAfterTransaction = () => { |
There was a problem hiding this comment.
curious on why you've chosen to create a helper for it in place of calling markForRefreshAfterNavigation() directly on the components
There was a problem hiding this comment.
it was actually redundant, removed
| }, | ||
| "hideDust": { | ||
| "title": "Ocultar pagamentos pequenos", | ||
| "title": "Ocultar pagamentos recebidos menores que 0.1 XLM", |
| {ListHeaderComponent} | ||
| <HistoryWrapper> | ||
| <Spinner testID="spinner" /> | ||
| <Spinner size="small" testID="spinner" /> |
There was a problem hiding this comment.
could we make it a large spinner? I think it would look better since History takes a lot of space
fix: removed unecessary code for centralized polling on balancesList
fix: removed unecessary helper function
|
@CassioMG @leofelix077 also added the improvement to the pull to refresh from #299 Beforebefore.movAfterafter.mov |
|
|
||
| // Mark history to refresh when user navigates to history screen | ||
| markHistoryForRefreshAfterTransaction(); | ||
| useHistoryStore.getState().markForRefreshAfterNavigation(); |
There was a problem hiding this comment.
why not extracting it at the beginning of the component?
const { markForRefreshAfterNavigation } = useHistoryStore();
|
|
||
| // Mark history to refresh when user navigates to history screen | ||
| markHistoryForRefreshAfterTransaction(); | ||
| useHistoryStore.getState().markForRefreshAfterNavigation(); |
| isLoading: boolean; | ||
| error: string | null; | ||
| shouldRefreshAfterNavigation: boolean; | ||
| isFetching: boolean; // Track if a fetch is currently in progress |
There was a problem hiding this comment.
could we review/update the HistoryState JSDocs above?
CassioMG
left a comment
There was a problem hiding this comment.
Lgtm - left a few suggestions
We created a new history store for managing history data
iOS
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-09-08.at.20.25.55.mp4
Android 🤖
Screen_recording_20250908_210638.mp4
closes #300
closes #299