Add loading indicator when loading more transactions in mobile transaction list #3900
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
joel-jeremy
changed the title
joel-jeremy
changed the title
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
joel-jeremy
changed the title
joel-jeremy
changed the title
WalkthroughThe pull request introduces several enhancements across multiple components related to transaction handling in the desktop client application. Key changes include the addition of a new state variable Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/mobile/transactions/TransactionList.jsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (1)
116-118: Consider enhancing accessibility for loading states.While the loading state implementation is correct, consider adding ARIA attributes to improve accessibility when loading more transactions.
<TransactionListWithBalances isLoading={isLoading} transactions={transactions} balance={balance} balanceCleared={balanceCleared} balanceUncleared={balanceUncleared} searchPlaceholder={`Search ${category.name}`} onSearch={onSearch} isLoadingMore={isLoadingMore} + aria-busy={isLoadingMore} + aria-live="polite" onLoadMore={loadMoreTransactions} onOpenTransaction={onOpenTransaction} />packages/loot-core/src/client/data-hooks/transactions.ts (1)
93-106: Consider enhancing error handling in loadMore.While the error handling is functional, consider adding error type information and potentially handling specific error cases differently.
- const loadMore = useCallback(async () => { + const loadMore = useCallback(async () => { if (!pagedQueryRef.current) { return; } setIsLoadingMore(true); await pagedQueryRef.current .fetchNext() - .catch(setError) + .catch((error: Error) => { + // Handle specific error cases if needed + if (error instanceof NetworkError) { + setError(new Error('Failed to load more transactions. Please check your connection.')); + } else { + setError(error); + } + }) .finally(() => { setIsLoadingMore(false); }); }, []);packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
237-238: LGTM! Consider adding type annotations.The addition of
isLoadingMoreandloadMoreTransactionsprops aligns well with the PR objective. Consider adding explicit type annotations for better maintainability.- isLoadingMore, - loadMore: loadMoreTransactions, + isLoadingMore: boolean, + loadMore: loadMoreTransactions: () => Promise<void>,packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (3)
43-58: Well-structured Loading component with good accessibility!The Loading component is well-implemented with proper accessibility support and styling flexibility. Consider adding PropTypes or TypeScript for better type safety.
Loading.propTypes = { style: PropTypes.object, 'aria-label': PropTypes.string };
171-181: Well-implemented loading indicator with good UX considerations!The loading indicator is properly implemented with matching height for visual consistency. Consider adding a loading threshold to prevent flickering for quick loads.
- {isLoadingMore && ( + {isLoadingMore && transactions.length > 0 && (
Line range hint
102-108: Consider enhancing scroll listener implementationThe scroll listener implementation could benefit from:
- Debouncing to prevent multiple rapid calls
- Error handling for failed load attempts
- Loading state guard to prevent multiple simultaneous loads
+ const [isError, setIsError] = useState(false); + const debouncedLoadMore = useCallback( + debounce(() => { + if (!isLoadingMore && !isError) { + onLoadMore?.().catch(() => setIsError(true)); + } + }, 250), + [isLoadingMore, isError, onLoadMore] + ); useScrollListener(({ hasScrolledToEnd }) => { if (hasScrolledToEnd('down', 5)) { - onLoadMore?.(); + debouncedLoadMore(); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(3 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx(3 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx(4 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx(2 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(3 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
packages/loot-core/src/client/data-hooks/transactions.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/transactions.ts:32-88
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the React `useTransactions` hook defined in `packages/loot-core/src/client/data-hooks/transactions.ts`, changes to `options` (e.g., `options.pageCount`) should be applied on the next query without adding them to the `useEffect` dependency array, to avoid unnecessary rerenders when options change.
🔇 Additional comments (11)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3)
Line range hint 43-47: LGTM! Hook usage is properly implemented.
The addition of isLoadingMore and loadMoreTransactions from the useTransactions hook aligns with the PR objective and follows React best practices.
60-60: LGTM! Simplified reload call.
Removing optional chaining is appropriate since reloadTransactions is a required property from the hook.
Line range hint 1-134: Verify consistent loading state implementation across mobile components.
Let's ensure the loading state implementation is consistent across all mobile transaction-related components.
✅ Verification successful
Let me gather more specific information about the implementation patterns.
Based on the results, I can now generate the final response as we have sufficient information about the loading state implementation across components.
Loading state implementation is consistent across mobile transaction components
The implementation of loading states is uniform across all mobile transaction-related components:
- All components use the
useTransactionshook which providesisLoadingMoreandloadMorefunctionality TransactionListWithBalancesconsistently receives and handlesisLoadingMoreprop- Loading states are properly propagated from data hooks to the UI components
- The implementation in CategoryTransactions.jsx follows the same pattern as AccountTransactions.tsx and other transaction components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of loading states across mobile transaction components
# Expected: All mobile transaction components should handle isLoadingMore similarly
# Search for mobile transaction components using isLoadingMore
echo "Checking mobile transaction components for loading state implementation..."
rg -l "isLoadingMore" "packages/desktop-client/src/components/mobile"
# Check TransactionListWithBalances usage
echo "\nChecking TransactionListWithBalances usage patterns..."
ast-grep --pattern 'TransactionListWithBalances {
$$$
isLoadingMore={$_}
$$$
}'
Length of output: 764
Script:
#!/bin/bash
# Check the implementation patterns in all identified files
echo "Checking TransactionList.jsx implementation:"
rg "isLoadingMore" -B 2 -A 2 packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
echo "\nChecking AccountTransactions.tsx implementation:"
rg "isLoadingMore" -B 2 -A 2 packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
echo "\nChecking TransactionListWithBalances.jsx implementation:"
rg "isLoadingMore" -B 2 -A 2 packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx
# Check if all components use useTransactions hook
echo "\nChecking useTransactions hook usage:"
rg "useTransactions" -A 5 packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
Length of output: 6151
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx (2)
68-69: LGTM: Props addition aligns with loading indicator implementation
The new props isLoadingMore and onLoadMore are well-named and properly structured for handling the loading state of additional transactions.
108-110: Verify interaction between pull-to-refresh and load-more states
While the implementation looks correct, please ensure that the loading states don't conflict when both pull-to-refresh and load-more operations are triggered simultaneously.
Let's check for any existing handling of concurrent loading states:
Consider implementing a loading state manager to handle concurrent loading operations if not already present. This would help prevent potential race conditions and improve UX.
✅ Verification successful
Let me gather more information about the PullToRefresh component and its interaction with loading states.
Let me try one more search to find the TransactionList component implementation to see how it handles these states.
Based on the available evidence from our searches, I'll generate the final response since we've seen that:
- The loading states (
isLoadingMoreand pull-to-refresh) are handled separately - Multiple components use these props consistently (AccountTransactions, CategoryTransactions, TransactionListWithBalances)
- The props are properly forwarded through the component hierarchy
Loading states implementation appears correct
The current implementation separates pull-to-refresh and load-more states, which is the correct approach. The components consistently handle these states across different transaction list implementations, suggesting a well-established pattern. No evidence of state conflicts was found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for loading state handling in related components
ast-grep --pattern 'isLoading || isLoadingMore'
rg -A 5 'isLoading.*isLoadingMore|isLoadingMore.*isLoading'
Length of output: 2161
Script:
#!/bin/bash
# Search for PullToRefresh component implementation
ast-grep --pattern 'function PullToRefresh'
ast-grep --pattern 'const PullToRefresh ='
# Search for loading state handling in PullToRefresh
rg -A 10 'onRefresh.*isLoading|isLoading.*onRefresh'
Length of output: 3132
Script:
#!/bin/bash
# Search for TransactionList component and its loading state handling
ast-grep --pattern 'function TransactionList'
ast-grep --pattern 'const TransactionList ='
# Search for loading state props in TransactionList
rg -B 2 -A 5 'isLoadingMore.*onLoadMore|onLoadMore.*isLoadingMore'
Length of output: 3150
packages/loot-core/src/client/data-hooks/transactions.ts (2)
27-31: LGTM! Type changes improve type safety and support new features.
The changes to make isLoading and reload required properties, along with the addition of loadMore and isLoadingMore, enforce better handling of loading states and provide necessary functionality for loading more transactions.
39-39: LGTM! Well-implemented loading states and error handling.
The implementation shows good practices:
- Proper state management for loading indicators
- Error handling with cleanup in finally block
- Efficient use of
useCallback - Respects the learning about preventing unnecessary rerenders from options changes
Also applies to: 93-106, 108-110
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
271-271: LGTM! Simplified reload call.
The removal of optional chaining is appropriate since reloadTransactions is now a required property from the hook. This change simplifies the code while maintaining the same functionality.
328-329: LGTM! Verify TransactionListWithBalances implementation.
The loading state props are correctly passed down to handle pagination. Let's verify that the TransactionListWithBalances component properly implements these props.
✅ Verification successful
Props are correctly implemented and used
The loading props are properly implemented in both components:
TransactionListWithBalancescorrectly passes downisLoadingMoreandonLoadMoreprops toTransactionListTransactionListproperly implements infinite scroll loading:- Uses
useScrollListenerto detect when user scrolls to bottom and callsonLoadMore - Shows a loading indicator when
isLoadingMoreis true - Loading indicator has proper accessibility label and consistent styling
- Uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TransactionListWithBalances implementation of loading props
# Check if TransactionListWithBalances properly implements the loading props
ast-grep --pattern 'type Props = {
$$$
isLoadingMore?: boolean
onLoadMore?: () => void
$$$
}' packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx
# Check if there's a loading indicator UI implementation
rg -A 5 'isLoadingMore.*true' packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx
Length of output: 586
Script:
#!/bin/bash
# Let's first find the correct location of TransactionListWithBalances
fd -t f "TransactionListWithBalances.tsx"
# Once we find the file, we'll verify the implementation
Length of output: 41
Script:
#!/bin/bash
# Let's try a broader search for the component using ripgrep
rg -l "TransactionListWithBalances"
# Also search for any file containing both isLoadingMore and onLoadMore props
rg -l "isLoadingMore.*onLoadMore|onLoadMore.*isLoadingMore"
Length of output: 345
Script:
#!/bin/bash
# Now that we found the correct file (.jsx extension), let's check its implementation
cat packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx
# Also check for any loading-related UI implementation
rg -A 5 "isLoadingMore|onLoadMore" packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx
Length of output: 5727
Script:
#!/bin/bash
# Let's check the TransactionList component to see how it handles these props
cat packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
# Also check for any loading-related UI implementation in TransactionList
rg -A 5 "isLoadingMore|onLoadMore" packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
Length of output: 17110
packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (2)
59-66: LGTM! Props update aligns with loading indicator requirement
The addition of isLoadingMore prop enables proper handling of the loading state for pagination, which directly addresses the PR objective.
67-68: Good internationalization implementation!
Translation hooks are properly implemented and consistently used for user-facing strings, improving accessibility and internationalization support.
Also applies to: 111-112, 117-117
No description provided.