Feat infinite scroll transactions #16760
Conversation
WalkthroughThe pull request introduces a variety of changes centered on transaction management and user interface enhancements. A new optional property, 🪧 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 (
|
packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts
Outdated
Show resolved
Hide resolved
| pagesFetched, | ||
| }) | ||
| ) { | ||
| fetchPage(pagesFetched + 1); |
There was a problem hiding this comment.
ensure that the fetching isn't happening like 10000x
There was a problem hiding this comment.
Best would be to move the logic in the if statement above right to the fetching - making it imperative
| const numberOfHidden = allTransactions.length - visibleTransactions.length; | ||
| const totalPossiblyVisible = allAccountTransactions - numberOfHidden; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This is obviously not idea, better solution would be to move this "fetching with condition" right to place where fetching is occuring - in the thunks, but that would require a lot of changes IMHO, this is simpler for this particular case.
I didn't want to be stuck in the complex thunks. Event hough I prefer it.
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
from
0f1719d to
ff188b3
Compare
| numberOfPagesRequested, | ||
| }: { | ||
| account: Account; | ||
| numberOfPagesRequested: number; |
There was a problem hiding this comment.
this number should be the number of the page in the UI.
When, for some reason, the FE filtering displays different number / paging of the transactions, we should treat the number in the UI as the one which should detemine how many "pages" of the transactions we should fetch.
As the "totals" and from that derive paging, doesn't correspond with the UI.
| import { TransactionList } from './TransactionList'; | ||
| import { useVisibleTransactionsRetriever } from './useFetchTransactions'; | ||
|
|
||
| const NoResults = styled.div` |
There was a problem hiding this comment.
@jvaclavik pls advice, taht's the clsoes want and most similar I found, but it obviously violates the design system doctrine.
| ${typography.hint} | ||
| `; | ||
|
|
||
| export const NoVisibleTransactions = () => ( |
There was a problem hiding this comment.
TBD: just placeholder for something better https://github.com/trezor/trezor-suite/pull/16760/files#r1941045584
packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts
Show resolved
Hide resolved
| visibleTransactions, | ||
| visibleTotal: totalPossiblyVisible, | ||
| isFetching: isFetching || transactionsIsLoading, | ||
| isLoading: discovery && discovery?.status !== DiscoveryStatus.COMPLETED, // NOTE: loading indicates that the state of the data is unknown and we are "loading for the first time" |
There was a problem hiding this comment.
I am not entirely sure about this.
isLoading should indicate a state, when the wallet is loading the transactions for the first time. This is useful when displaying the empty state.
| isExportable = true, | ||
| }: TransactionListProps) => { | ||
| // NOTE: The number of the displayed pages may be different from the number of the pages for all transactions | ||
| const [visiblePages, setVisiblePages] = useState(1); |
There was a problem hiding this comment.
This tracks the paging according to the visible transactions, it is not the same as if you derived paging from the total.
|
Please remove the "stats" element from accounts on the following networks. After we'll filter out hidden and scam transactions entirely, the numbers might not add up.
|
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
from
ff188b3 to
3b13d9e
Compare
|
🚀 Expo preview is ready!
|
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
4 times, most recently
from
6cad49f to
0765c01
Compare
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
from
0765c01 to
ca2bfe6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (20)
packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts (3)
23-32: Refine the XRP fallback approach ingetPaging.The logic of adding 1 to
txFetchedfor Ripple can lead to inaccurate page calculations if there are more than one additional transaction. This might prematurely consider the last page reached, possibly missing some items.
72-93: Consider error handling forfetchCommon.The code dispatches async thunks but does not handle potential errors, such as network failures or partial fetches. Adding a try/catch or separate error state (e.g., a Redux slice or local hook state) can help ensure robust error handling and user feedback in case of fetch failure.
220-220: UnifyisFetchingandisLoadingstates for clarity.Currently,
isFetchingindicates ongoing fetch operations, whileisLoadingdepends ondiscovery. Merging them or using clearer naming can prevent confusion and better reflect the UI’s loading state.packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (3)
31-41: Add documentation for new props.Properties like
allTransactions,limitPaging,customPageFetching, andonPageRequestedshape important parts of the pagination flow. Consider adding JSDoc or inline comments explaining their roles and expected behavior for better maintainability.
59-80: Evaluate the performance impact offetchAllwhenanchoris present.Fetching the entire transaction history for an anchor can be expensive. A partial fetch strategy—loading only up to the anchor—might improve responsiveness for large accounts.
199-199: Use consistent naming for the pagination limit prop.
limitedPageList={Boolean(limitPaging)}is descriptive, but renaming the prop to a more standardized pattern such asisPagingLimitedmay better align with conventional boolean naming.packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts (1)
17-18: Fix spelling of “Visible” in the variable name.The variable
requestedNumberOfVisibileItemscontains a minor typo. Correcting it torequestedNumberOfVisibleItemsimproves readability.- const requestedNumberOfVisibileItems = numberOfPagesRequested * perPage; + const requestedNumberOfVisibleItems = numberOfPagesRequested * perPage;packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (2)
17-21: Consider using semantic HTML for better accessibility.The
NoResultsstyled component could benefit from using a more semantic HTML element likesectionorarticleinstead ofdiv.-const NoResults = styled.div` +const NoResults = styled.section` text-align: center; color: ${({ theme }) => theme.legacy.TYPE_DARK_GREY}; ${typography.hint} `;
73-75: Add loading state feedback for better UX.When there are no visible transactions and the data is still loading, consider showing a loading indicator instead of the NoVisibleTransactions component.
- if (!result.isLoading && result.visibleTransactions.length === 0) { + if (result.isLoading) { + return <LoadingTransactions />; + } + + if (result.visibleTransactions.length === 0) { return <NoVisibleTransactions />; }packages/suite/src/views/wallet/transactions/Transactions.tsx (1)
61-61: Consider adding error boundaries around WalletTransactionList.Since WalletTransactionList is a new component handling complex state and data fetching, it should be wrapped in an error boundary to gracefully handle potential runtime errors.
+import { ErrorBoundary } from 'src/components/ErrorBoundary'; // ... in both instances: -<WalletTransactionList account={account} symbol={account.symbol} /> +<ErrorBoundary> + <WalletTransactionList account={account} symbol={account.symbol} /> +</ErrorBoundary>Also applies to: 74-74
packages/suite/src/views/wallet/transactions/TransactionList/__tests__/transaction-fetch-utils.test.ts (2)
88-88: Fix typo in test description.There's a typo in the test description: "enought" should be "enough".
- it('should NOT request fetching any more pages since there is enought of visible transactions already', () => { + it('should NOT request fetching any more pages since there is enough of visible transactions already', () => {
3-98: Add edge case tests for better coverage.The test suite should include additional edge cases:
- Zero total transactions
- Negative numbers
- Maximum safe integer values
it('should handle zero total transactions', () => { const result = shouldAttemptToLoadNextPageForVisibleTransactions({ totalNumberOfTransactions: 0, currentNumberOfTransactions: 0, currentNumberOfVisibleTransactions: 0, perPage: 25, numberOfPagesRequested: 1, }); expect(result).toBe(false); }); it('should handle maximum safe integer values', () => { const result = shouldAttemptToLoadNextPageForVisibleTransactions({ totalNumberOfTransactions: Number.MAX_SAFE_INTEGER, currentNumberOfTransactions: Number.MAX_SAFE_INTEGER - 25, currentNumberOfVisibleTransactions: Number.MAX_SAFE_INTEGER - 50, perPage: 25, numberOfPagesRequested: 1, }); expect(result).toBe(true); });packages/suite/src/components/wallet/Pagination.tsx (1)
138-140: Consider adding a tooltip for pagination controls.When limitedPageList is true and some pages are hidden, add a tooltip to inform users about the limited view.
{totalPages && totalPages > 2 && !limitedPageList && ( - <PageItem onClick={() => onPageSelected(totalPages)}>»</PageItem> + <PageItem + onClick={() => onPageSelected(totalPages)} + title={limitedPageList ? "Limited view enabled" : "Go to last page"} + >»</PageItem> )}packages/suite/src/support/messages.ts (7)
1-3: Consider using TypeScript for better type safetyThe file could benefit from TypeScript type definitions for the message objects to ensure type safety and better IDE support.
- import { defineMessages } from 'react-intl'; + import { defineMessages, MessageDescriptor } from 'react-intl'; + + type Messages = { [key: string]: MessageDescriptor }; + + export default defineMessages<Messages>({
1-9499: Consider splitting messages into smaller feature-specific filesThe file is quite large (9000+ lines) which can make maintenance difficult. Consider splitting messages into smaller feature-specific files (e.g. staking-messages.ts, wallet-messages.ts etc.) and combining them in an index file.
91-95: Avoid exposing sensitive information in error messagesThe TR_NO_VISIBLE_TRANSACTIONS message includes a reference to "Hidden tokens tab" which could potentially expose sensitive UI information. Consider making error messages more generic.
- defaultMessage: "No transactions to show. If you're looking for a specific transaction, check the <tabLink>Hidden tokens tab</tabLink>—but be cautious, as hidden tokens may carry risks.", + defaultMessage: "No transactions to show. Check your settings for additional transaction visibility options.",
6167-6172: Use consistent message formatting for placeholdersSome messages use different styles for placeholders ({value} vs {0}). Stick to a consistent format throughout the file.
- defaultMessage: '{value} {value, plural, one {transaction} other {transactions}}', + defaultMessage: '{count} {count, plural, one {transaction} other {transactions}}',
1-9499: Add message grouping comments for better organizationConsider adding comment headers to group related messages together (e.g. // Wallet messages, // Transaction messages etc.) to improve code organization and maintainability.
1-9499: Consider adding message validationAdd runtime validation to ensure all required message properties are present and formatted correctly. This could help catch issues early in development.
Example validation:
const validateMessages = (messages: Record<string, any>) => { Object.entries(messages).forEach(([key, msg]) => { if (!msg.id || !msg.defaultMessage) { throw new Error(`Invalid message format for key: ${key}`); } }); };
1-9499: Add JSDoc comments for message groupsConsider adding JSDoc comments to document message groups, their usage context and any special formatting requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/suite/src/components/wallet/Pagination.tsx(4 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx(4 hunks)packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/__tests__/transaction-fetch-utils.test.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts(1 hunks)packages/suite/src/views/wallet/transactions/Transactions.tsx(3 hunks)packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx(2 hunks)suite-common/wallet-config/src/types.ts(1 hunks)suite-common/wallet-config/src/utils.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (6)
packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx (1)
141-150: LGTM! Implementation aligns with requirements.The conditional rendering of SummaryCards based on transaction reliability aligns with the PR objectives and the request to hide stats for unreliable coins.
packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts (2)
61-68: Clarify thefetchedAlllogic.Setting
fetchedAllto true whenisLastPageis reached might be correct for networks with a known total transaction count. However, for Ripple, the artificially incremented total can cause a prematurefetchedAllvalue if there are actually more transactions beyond that single fallback page. Consider adjusting the logic or providing a separate condition for Ripple to ensure completeness.
190-214: Prevent potential excessive fetching calls.The effect that triggers
fetchPage(allTransactionsPagesFetched + 1)may fire repeatedly if theshouldAttemptToLoadNextPageForVisibleTransactionsconditions are met in quick succession. Consider debouncing or adding a guard condition to prevent excessive requests, especially when there are many hidden or filtered transactions.packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts (1)
20-26: Verify infinite fetching possibility with hidden transactions.Repeatedly returning
truehere when no visible transactions are found could lead to continuous fetch attempts. Confirm whether this is the intended behavior or if a safeguard is needed.packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
67-71: Consider adding error handling for the useVisibleTransactions hook.The hook result should be checked for potential errors to provide appropriate feedback to users.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if the hook handles errors ast-grep --pattern 'useVisibleTransactions($$$).error'Length of output: 56
Error Handling Missing for useVisibleTransactions Hook
The search confirms that there’s no explicit error handling (e.g. checking for a
result.errorproperty) where the hook is used inWalletTransactionList.tsx. This indicates that potential errors from the hook aren’t being caught, which may result in inadequate user feedback during failure cases.
- The current code only calls the hook without any conditional check or fallback for error scenarios.
packages/suite/src/support/messages.ts (1)
1-9499: LGTM for message content and formattingThe actual message content and formatting looks good:
- Messages use proper ICU MessageFormat syntax
- Placeholders are properly named
- Pluralization rules are correctly implemented
- Messages provide good context through descriptions
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx
Show resolved
Hide resolved
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx
Show resolved
Hide resolved
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
from
ca2bfe6 to
fb731b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
60-91: Consider adding loading state handling.The component handles the empty state well but could benefit from explicit loading state handling to prevent potential flicker during state transitions.
export const WalletTransactionList = ({ account, symbol, customTotalItems, isExportable = true, }: TransactionListProps) => { const [visiblePages, setVisiblePages] = useState(1); const result = useVisibleTransactions({ account, numberOfPagesRequested: visiblePages, }); + if (result.isLoading) { + return <TransactionList.Skeleton />; + } if (!result.isLoading && result.visibleTransactions.length === 0) { return <NoVisibleTransactions />; }packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (1)
84-89: Consider adding a comment explaining the page calculation logic.The page calculation logic is non-trivial and would benefit from a more detailed comment explaining why
startPagedefaults to 1 when there's no anchor.- // NOTE: if there is no anchor, we can keep the page 1 + // When there's no anchor transaction, we start from page 1. + // With an anchor, we calculate the page containing that transaction + // to ensure the user lands on the correct page. const startPage = useMemo( () => (anchor ? findAnchorTransactionPage(transactions, perPage, anchor) : null), [anchor, perPage, transactions], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/suite/src/components/wallet/Pagination.tsx(4 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/staking/components/StakingDashboard/components/Transactions.tsx(3 hunks)packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx(4 hunks)packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/__tests__/transaction-fetch-utils.test.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts(1 hunks)packages/suite/src/views/wallet/transactions/Transactions.tsx(3 hunks)packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx(2 hunks)suite-common/wallet-config/src/types.ts(1 hunks)suite-common/wallet-config/src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/suite/src/components/wallet/Pagination.tsx
- packages/suite/src/views/wallet/transactions/Transactions.tsx
- suite-common/wallet-config/src/types.ts
- suite-common/wallet-config/src/utils.ts
- packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts
- packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🧠 Learnings (2)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (1)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
🔇 Additional comments (6)
packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx (2)
5-5: LGTM!The import of
areNetworkTransactionsUnreliablealigns with the PR objective to hide transaction statistics for unreliable coins.
141-150: Verify that all requested networks are covered.The implementation looks good and aligns with the requirement to hide transaction statistics for unreliable coins. Let's verify that all requested networks (Ethereum, Polygon, Solana, BNB, Arbitrum, Base, Optimism, Cardano, and XRP) are properly configured in the
UNRELIABLE_TRANSACTION_COINSconstant.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all requested networks are included in UNRELIABLE_TRANSACTION_COINS # Search for the constant definition echo "Searching for UNRELIABLE_TRANSACTION_COINS definition:" rg -A 10 "UNRELIABLE_TRANSACTION_COINS.*=" suite-common/wallet-config/src/Length of output: 925
Verification Complete: All Requested Networks Are Covered
The constant
UNRELIABLE_TRANSACTION_COINSinsuite-common/wallet-config/src/types.tsincludes the following entries:
'eth'(Ethereum)'xrp'(XRP)'sol'(Solana)'bsc'(Binance Smart Chain, which corresponds to the BNB network)'arb'(Arbitrum)'pol'(Polygon)'base'(Base)'op'(Optimism)'ada'(Cardano)This precisely covers all the requested networks. No further modifications are needed.
packages/suite/src/views/wallet/staking/components/StakingDashboard/components/Transactions.tsx (1)
21-23: LGTM!The addition of
allTransactionsselector and prop is well-integrated and aligns with the infinite scroll feature requirements.Also applies to: 34-34
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
23-51: LGTM! Well-structured empty state component.The
NoVisibleTransactionscomponent provides clear user feedback and navigation options.packages/suite/src/views/wallet/transactions/TransactionList/__tests__/transaction-fetch-utils.test.ts (1)
3-110: LGTM! Comprehensive test coverage.The test suite thoroughly covers various scenarios including edge cases, ensuring robust pagination behavior.
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (1)
101-112: Verify search and pagination interaction.The interaction between search and custom page fetching needs verification to ensure it doesn't cause issues with infinite scroll.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for other components using similar pagination with search rg -A 5 'onPageSelected.*isSearching.*customPageFetching'Length of output: 59
Action Required: Verify Search and Infinite Scroll Interaction Manually
The current implementation correctly guards calling
fetchPage(page)by checking if neitherisSearchingnorcustomPageFetchingare true. However, our initial repository search for similar patterns returned no additional references, so we don’t have extra context confirming that similar logic is applied elsewhere in the codebase.Please verify manually that:
- When a search is active, pagination does not inadvertently trigger infinite scroll behavior.
- Custom page fetching and regular pagination remain isolated without causing duplicate or continuous page fetches.
vojtatranta
changed the title
| </PageItem> | ||
| )) | ||
| calculatedPages | ||
| .slice(0, limitedPageList ? currentPage + 2 : calculatedPages.length) |
There was a problem hiding this comment.
2 more pages visible, aswe agreed. (1, 2, 3) not just (1, 2)
There was a problem hiding this comment.
maybe we could add it to some const to make it more clear, but nitpick
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx
Outdated
Show resolved
Hide resolved
| import { TransactionList } from './TransactionList'; | ||
| import { useVisibleTransactionsRetriever } from './useFetchTransactions'; | ||
|
|
||
| const NoResults = styled.div` |
packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts
Show resolved
Hide resolved
packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts
Outdated
Show resolved
Hide resolved
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
from
fb731b0 to
6ff4ca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (3)
4-16: Consider grouping imports logically.
Your import statements fromstyled-components,@trezor/components, etc., are fine. However, to improve maintainability, you could optionally group imports by domain (e.g., external libraries vs. local modules) or by functionality.
17-22: Naming consistency for styled components.
WhileNoResultsis succinct, consider renaming it toNoResultsWrapperor similar to clarify its role as a styled wrapper/container.-const NoResults = styled.div` +const NoResultsWrapper = styled.div`
53-58: Interface naming clarity.
WhileTransactionListPropsis acceptable, consider a name likeWalletTransactionListPropsto stay aligned with the component’s name. This helps unify naming across the codebase.packages/suite/src/components/wallet/Pagination.tsx (2)
56-67: Boolean prop naming for clarity.
limitedPageListmight be clearer asisPageListLimitedor similar. This conveys the boolean nature immediately.- limitedPageList?: boolean; + isPageListLimited?: boolean;
138-138: Consistent usage oflimitedPageListcheck.
Hiding the "»" button is logical when limiting pages, but ensure a consistent approach for other boundary controls, e.g. "«" if it applies.packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (3)
77-80: Conditional fetch approach for anchor-based loading.
Anchors trigger a full fetch if not all items are loaded. This keeps anchor-based navigation consistent. Just ensure performance remains adequate if repeated anchor visits occur.
93-93: Early return can cause confusion.
Exiting silently ifstartPage === nullmight hide unexpected states. Consider logging or providing fallback to page 1 with a warning.- if (startPage === null) return; + if (startPage === null) { + console.warn('startPage is null, defaulting to page 1'); + return; + }
199-199:limitedPageListusage in<Pagination>
TheBoolean(limitPaging)cast is straightforward, but watch out for potential naming mismatches between the two props.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/suite/src/components/wallet/Pagination.tsx(4 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/staking/components/StakingDashboard/components/Transactions.tsx(3 hunks)packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx(4 hunks)packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/__tests__/transaction-fetch-utils.test.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts(1 hunks)packages/suite/src/views/wallet/transactions/Transactions.tsx(3 hunks)packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx(2 hunks)suite-common/wallet-config/src/types.ts(1 hunks)suite-common/wallet-config/src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/suite/src/views/wallet/transactions/Transactions.tsx
- packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx
- packages/suite/src/views/wallet/staking/components/StakingDashboard/components/Transactions.tsx
- suite-common/wallet-config/src/types.ts
- packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts
- suite-common/wallet-config/src/utils.ts
- packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts
- packages/suite/src/views/wallet/transactions/TransactionList/tests/transaction-fetch-utils.test.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🧠 Learnings (2)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (1)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (12)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (3)
1-2: Imports look good.
No issues found in the import statements for React and Redux hooks.
60-91: Solid usage of custom hook and conditional rendering.
The component:
- Uses
useVisibleTransactionsto fetch and store transaction data.- Returns an empty-state component if no visible transactions.
- Passes data correctly to
<TransactionList />.Implementation looks consistent with best practices.
23-51: Link event handling looks good, but verify router integration.
YouronClickhandler dispatches thegotoaction, preserving parameters. This is appropriate. Just ensure that external references to'wallet-tokens-hidden'remain valid in the router definitions.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for "wallet-tokens-hidden" references in the codebase to confirm route validity rg "wallet-tokens-hidden"Length of output: 994
Router Integration Verified
The search results confirm that the reference to
'wallet-tokens-hidden'is present in the router definitions (notably insuite-common/suite-config/src/routes.ts) and is used consistently across other related components. No issues were identified.packages/suite/src/components/wallet/Pagination.tsx (1)
103-115: Slice condition might cause off-by-one confusion.
The expressioncurrentPage + 2could omit or include more pages than desired for certain edge cases. Validate that your UI reflects exactly the intended near-page windows.packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (8)
28-28: Hook import is appropriate.
Custom hooks help isolate and reuse fetching logic, aligning with React best practices.
31-40: Expanded interface for transaction props.
New props (allTransactions,limitPaging,customPageFetching,onPageRequested) provide more granular control and reflect a well-considered design.
44-53: Destructuring of new props is clear.
Ensure thecustomPageFetchinglogic does not conflict with any existing thunk-based fetching flows.
59-59: Destructured fetch handlers are well-organized.
fetchedAllandfetchAllare succinct property names. Confirm in usage thatfetchAlltriggers no redundant calls if data is already loaded.
85-89:startPagelogic is concise.
UsingfindAnchorTransactionPageis helpful. Check for scenarios where the anchor transaction might be near the last page.
100-100: Synchronized totalItems.
defaultTotalItemsandsearchedTransactions.lengthlogic is clear. Ensure edge cases (like 0 total) are correctly handled.
103-103:onPageRequestedcallback usage.
Calls the prop callback only if defined, with safe optional chaining. This is a good approach for optional callbacks.
105-105: Custom fetch logic complements the existing approach.
Your conditional ensures that search mode does not trigger a new page fetch unnecessarily. This is a well-balanced approach.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
17-21: 🛠️ Refactor suggestionRemove unused styled component.
The
NoResultsstyled component is defined but never used in the code.-const NoResults = styled.div` - text-align: center; - color: ${({ theme }) => theme.legacy.TYPE_DARK_GREY}; - ${typography.hint} -`;
🧹 Nitpick comments (1)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
67-67: Move inline comment to JSDoc.The note about page count differences should be moved to JSDoc for better documentation.
+ /** The number of the displayed pages may be different from the number of the pages for all transactions */ const [visiblePages, setVisiblePages] = useState(1); - // NOTE: The number of the displayed pages may be different from the number of the pages for all transactions🧰 Tools
🪛 Biome (1.9.4)
[error] 67-67: expected
}but instead foundconstRemove const
(parse)
[error] 67-69: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
🪛 Biome (1.9.4)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx
[error] 47-47: Expected corresponding JSX closing tag for 'Text'.
Opening tag
closing tag
(parse)
[error] 29-29: Expected corresponding JSX closing tag for 'Text'.
Opening tag
closing tag
(parse)
[error] 50-51: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 54-54: expected } but instead found :
Remove :
(parse)
[error] 57-58: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 64-65: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 65-65: Unexpected token. Did you mean {'>'} or >?
(parse)
[error] 67-67: expected } but instead found const
Remove const
(parse)
[error] 70-70: expected } but instead found :
Remove :
(parse)
[error] 71-71: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 74-74: expected } but instead found return
Remove return
(parse)
[error] 75-75: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 90-91: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 91-91: expected < but instead the file ends
the file ends here
(parse)
[error] 67-69: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 74-78: There is a suspicious semicolon in the JSX element.
This is usually the result of a typo or some refactor gone wrong.
Remove the semicolon, or move it inside a JSX element.
(lint/suspicious/noSuspiciousSemicolonInJsx)
🪛 GitHub Actions: [Check]: Commit messages
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx
[error] 1-1: Conventional Commits validation failed for commit 49efd42.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
🔇 Additional comments (3)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (3)
53-58: LGTM!The interface is well-defined with appropriate types for all props.
🧰 Tools
🪛 Biome (1.9.4)
[error] 54-54: expected
}but instead found:Remove :
(parse)
[error] 57-58: Unexpected token. Did you mean
{'}'}or}?(parse)
60-91: LGTM! Implementation aligns with PR objectives.The component successfully implements:
- Infinite scrolling through page-based transaction loading
- Handling of unreliable networks
- Conditional rendering of empty state
🧰 Tools
🪛 Biome (1.9.4)
[error] 64-65: Expected an expression but instead found '}'.
Expected an expression here.
(parse)
[error] 65-65: Unexpected token. Did you mean
{'>'}or>?(parse)
[error] 67-67: expected
}but instead foundconstRemove const
(parse)
[error] 70-70: expected
}but instead found:Remove :
(parse)
[error] 71-71: Unexpected token. Did you mean
{'}'}or}?(parse)
[error] 74-74: expected
}but instead foundreturnRemove return
(parse)
[error] 75-75: Unexpected token. Did you mean
{'}'}or}?(parse)
[error] 90-91: Unexpected token. Did you mean
{'}'}or}?(parse)
[error] 91-91: expected
<but instead the file endsthe file ends here
(parse)
[error] 67-69: Wrap comments inside children within braces.
Unsafe fix: Wrap the comments with braces
(lint/suspicious/noCommentText)
[error] 74-78: There is a suspicious semicolon in the JSX element.
This is usually the result of a typo or some refactor gone wrong.
Remove the semicolon, or move it inside a JSX element.(lint/suspicious/noSuspiciousSemicolonInJsx)
1-1: Fix commit message format.The commit message doesn't follow the Conventional Commits format. Please update it to follow the pattern:
type(scope): description.Example:
feat(transactions): implement infinite scroll for transaction list🧰 Tools
🪛 GitHub Actions: [Check]: Commit messages
[error] 1-1: Conventional Commits validation failed for commit 49efd42.
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx
Outdated
Show resolved
Hide resolved
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx
Outdated
Show resolved
Hide resolved
…some transactions are hidden locally
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
from
49efd42 to
e83f089
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (1)
197-207: 💡 Verification agent🧩 Analysis chain
Verify limitPaging implementation against requirements.
The PR objectives mention limiting access to only two pages around the current one for unreliable coins. Please verify that
limitedPageListprop correctly implements this requirement.
🏁 Script executed:
#!/bin/bash # Search for Pagination component implementation rg -A 10 "interface PaginationProps"Length of output: 865
I'll now search in the repository how the
limitedPageListprop is used inside the Pagination component to verify that it indeed limits the displayed page links to only two pages around the current page. Let's inspect the actual implementation in the Pagination component.
🏁 Script executed:
#!/bin/bash # Grep for lines where limitedPageList is referenced in the Pagination component, including nearby context rg -A 30 -B 10 "limitedPageList" packages/suite/src/components/wallet/Pagination.tsxLength of output: 3729
The current implementation of the limited page list in the Pagination component merely slices the generated array of page numbers from the beginning to
currentPage + 2. This means that withlimitedPageListenabled, if a user is on page 5, they’ll see pages 1 through 7. This does not strictly enforce a two-page window around the current page (e.g. showing only two pages before and after the current page) as the PR objectives indicate.Actionable point:
- The pagination logic should be adjusted to display a centered window (e.g. two pages before and after the current page) rather than slicing from the beginning.
🧹 Nitpick comments (2)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
14-42: Fix component structure for better readability.The component structure can be improved by:
- Moving Column outside of Text component
- Ensuring proper component nesting
export const NoVisibleTransactions = () => { const dispatch = useDispatch(); return ( <Card> - <Text typographyStyle="hint" variant="tertiary"> - <Column alignItems="center"> + <Column alignItems="center"> + <Text typographyStyle="hint" variant="tertiary"> <Translation id="TR_NO_VISIBLE_TRANSACTIONS" values={{ tabLink: (chunks: ReactNode[]) => ( <Link onClick={event => { event.preventDefault(); dispatch( goto('wallet-tokens-hidden', { preserveParams: true }), ); }} > {chunks} </Link> ), }} /> + </Text> </Column> - </Text> </Card> ); };packages/suite/src/views/wallet/transactions/TransactionList/__tests__/transaction-fetch-utils.test.ts (1)
3-110: LGTM! Comprehensive test coverage with clear test cases.The test suite effectively covers:
- Edge cases (empty state, filtered transactions)
- Pagination scenarios (single page, multiple pages)
- Boundary conditions (no more pages, sufficient transactions)
The test descriptions are clear and the test data is well-structured.
However, consider adding these test cases for completeness:
- Invalid input validation (negative numbers)
- Edge case with perPage = 0
- Edge case with numberOfPagesRequested > total pages possible
it('should handle invalid input gracefully', () => { const result = shouldAttemptToLoadNextPageForVisibleTransactions({ totalNumberOfTransactions: 100, currentNumberOfTransactions: -1, // Invalid input currentNumberOfVisibleTransactions: 0, perPage: 25, numberOfPagesRequested: 1, }); expect(result).toBe(false); }); it('should handle zero perPage gracefully', () => { const result = shouldAttemptToLoadNextPageForVisibleTransactions({ totalNumberOfTransactions: 100, currentNumberOfTransactions: 25, currentNumberOfVisibleTransactions: 0, perPage: 0, // Invalid input numberOfPagesRequested: 1, }); expect(result).toBe(false); }); it('should handle numberOfPagesRequested > total pages possible', () => { const result = shouldAttemptToLoadNextPageForVisibleTransactions({ totalNumberOfTransactions: 100, currentNumberOfTransactions: 25, currentNumberOfVisibleTransactions: 0, perPage: 25, numberOfPagesRequested: 5, // More than possible (100/25 = 4 pages total) }); expect(result).toBe(false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/suite/src/components/wallet/Pagination.tsx(4 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/staking/components/StakingDashboard/components/Transactions.tsx(3 hunks)packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx(4 hunks)packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/__tests__/transaction-fetch-utils.test.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts(1 hunks)packages/suite/src/views/wallet/transactions/Transactions.tsx(3 hunks)packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx(2 hunks)suite-common/wallet-config/src/types.ts(1 hunks)suite-common/wallet-config/src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx
- packages/suite/src/views/wallet/transactions/Transactions.tsx
- suite-common/wallet-config/src/types.ts
- packages/suite/src/views/wallet/staking/components/StakingDashboard/components/Transactions.tsx
- packages/suite/src/components/wallet/Pagination.tsx
- packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts
- packages/suite/src/views/wallet/transactions/TransactionList/transaction-fetch-utils.ts
- suite-common/wallet-config/src/utils.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🧠 Learnings (2)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (1)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (5)
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (3)
30-41: LGTM! Interface changes align with PR objectives.The new props provide necessary flexibility for handling unreliable coins and custom pagination behavior:
allTransactionsfor complete transaction datalimitPagingfor restricting page accesscustomPageFetchingfor custom fetch behavioronPageRequestedcallback for pagination events
84-90: LGTM! Improved startPage calculation logic.The new startPage calculation is more robust, with a clear fallback to page 1 when no anchor is present.
144-209: LGTM! Rendering logic remains well-structured.The component maintains clean separation of concerns:
- Handles loading states with skeletons
- Separates pending and confirmed transactions
- Preserves search functionality
- Implements pagination controls appropriately
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (2)
44-49: LGTM! Well-defined interface with proper types.The interface is clear, concise, and uses proper TypeScript types for props.
51-82: Verify pagination behavior for unreliable networks.The implementation looks good, but ensure that the
limitPagingprop (line 71) correctly restricts pagination for unreliable networks as mentioned in the PR objectives.Let's verify the implementation of
areNetworkTransactionsUnreliable:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of areNetworkTransactionsUnreliable function # to ensure it covers all networks mentioned in PR comments ast-grep --pattern 'export const areNetworkTransactionsUnreliable = (symbol: string): boolean => { $$$ }'Length of output: 109
I'll execute a new search across the repository to locate the definition (or usage) of
areNetworkTransactionsUnreliableso that we can verify its implementation and behavior for unreliable networks.
🏁 Script executed:
#!/bin/bash # Searching for any occurrence of "areNetworkTransactionsUnreliable" rg -n "areNetworkTransactionsUnreliable" .Length of output: 809
I'll gather more context on the implementation of
areNetworkTransactionsUnreliableby inspecting its definition in the repository. Let's check the relevant lines in the filesuite-common/wallet-config/src/utils.ts.
🏁 Script executed:
#!/bin/bash # Displaying the implementation of areNetworkTransactionsUnreliable from suite-common/wallet-config/src/utils.ts sed -n '110,130p' suite-common/wallet-config/src/utils.tsLength of output: 408
I'll gather more context by locating how
UNRELIABLE_TRANSACTION_COINSis declared in the repository.
🏁 Script executed:
#!/bin/bash # Search for the declaration of UNRELIABLE_TRANSACTION_COINS in the repository rg -n "const UNRELIABLE_TRANSACTION_COINS" .Length of output: 137
I'll inspect the content of the
UNRELIABLE_TRANSACTION_COINSarray by viewing its declaration in the filesuite-common/wallet-config/src/types.tsto ensure that all the networks that are intended to restrict pagination are correctly included.
🏁 Script executed:
#!/bin/bash # Displaying the declaration of UNRELIABLE_TRANSACTION_COINS from suite-common/wallet-config/src/types.ts sed -n '40,60p' suite-common/wallet-config/src/types.tsLength of output: 465
Pagination Behavior Correctly Restricted for Unreliable Networks
The function
areNetworkTransactionsUnreliableis implemented as a simple check to see if the provided network symbol exists in theUNRELIABLE_TRANSACTION_COINSarray. The constant is defined insuite-common/wallet-config/src/types.tswith the appropriate networks. TheWalletTransactionListcomponent correctly passes the result ofareNetworkTransactionsUnreliable(symbol)to thelimitPagingprop, ensuring that pagination is restricted for unreliable networks as intended.
- Location Checked:
- Function definition in
suite-common/wallet-config/src/utils.ts(lines 110-130)- Constant declaration in
suite-common/wallet-config/src/types.ts
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx
Show resolved
Hide resolved
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx
Outdated
Show resolved
Hide resolved
…d go to end for networks with unrealiable transactions
vojtatranta
force-pushed
the
feat-infinite-scroll-transactions
branch
from
e83f089 to
8d5c67f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (1)
51-82: Consider adding a constant for the page limit.The number of visible pages (currentPage + 2) is hardcoded. Consider extracting this value into a named constant for better maintainability and clarity.
+const VISIBLE_PAGES_LIMIT = 2; // Number of additional pages to show around current page + export const WalletTransactionList = ({ account, symbol, customTotalItems, isExportable = true, }: TransactionListProps) => { const [visiblePages, setVisiblePages] = useState(1); const result = useVisibleTransactions({ account, - numberOfPagesRequested: visiblePages, + numberOfPagesRequested: visiblePages + VISIBLE_PAGES_LIMIT, });packages/suite/src/components/wallet/Pagination.tsx (1)
102-105: Add safety check for page slicing.The slice operation should handle edge cases where currentPage is negative or very large.
- .slice(0, isPageListLimited ? currentPage + 2 : calculatedPages.length) + .slice(0, isPageListLimited ? Math.min(currentPage + 2, calculatedPages.length) : calculatedPages.length)Also applies to: 138-139
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (2)
59-60: Consider adding loading state for fetchAll operation.The fetchAll operation might take time to complete, but there's no loading state to indicate this to users.
+ const [isFetchingAll, setIsFetchingAll] = useState(false); + useEffect(() => { if (anchor && !fetchedAll) { - fetchAll(); + setIsFetchingAll(true); + fetchAll().finally(() => { + setIsFetchingAll(false); + }); } }, [anchor, account, dispatch, fetchedAll, fetchAll]);Also applies to: 76-80
84-90: Add comment explaining the startPage logic.The startPage calculation logic is complex and would benefit from a more detailed comment explaining the behavior.
- // NOTE: if there is no anchor, we can keep the page 1 + // NOTE: When an anchor (transaction ID) is provided, find the page containing that transaction. + // If no anchor is provided or the transaction is not found, default to page 1. + // This allows deep-linking to specific transactions while maintaining a sensible default. const startPage = useMemo( () => (anchor ? findAnchorTransactionPage(transactions, perPage, anchor) : null), [anchor, perPage, transactions], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/suite/src/components/wallet/Pagination.tsx(4 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx(4 hunks)packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx(1 hunks)packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts(1 hunks)packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx(2 hunks)suite-common/wallet-config/src/types.ts(1 hunks)suite-common/wallet-config/src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/suite/src/views/wallet/transactions/components/TransactionSummary.tsx
- suite-common/wallet-config/src/types.ts
- suite-common/wallet-config/src/utils.ts
- packages/suite/src/views/wallet/transactions/TransactionList/useFetchTransactions.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🧠 Learnings (2)
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (2)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:59-60
Timestamp: 2025-02-11T14:02:43.417Z
Learning: In the Trezor Suite codebase, error handling for transaction fetching is managed in the thunks themselves, even when wrapped by custom hooks like `useFetchTransactions`.
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (2)
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:105-107
Timestamp: 2025-02-11T09:53:11.461Z
Learning: Error handling for transaction fetching operations in the Trezor Suite is managed at the thunk layer rather than in the component layer.
Learnt from: vojtatranta
PR: trezor/trezor-suite#16760
File: packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx:59-60
Timestamp: 2025-02-11T14:02:43.417Z
Learning: In the Trezor Suite codebase, error handling for transaction fetching is managed in the thunks themselves, even when wrapped by custom hooks like `useFetchTransactions`.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
packages/suite/src/views/wallet/transactions/TransactionList/WalletTransactionList.tsx (2)
14-42: LGTM!The NoVisibleTransactions component is well-structured, follows the design system, and provides clear feedback to users when no transactions are visible.
44-49: LGTM!The TransactionListProps interface is well-defined with clear types and appropriate optional properties.
packages/suite/src/components/wallet/Pagination.tsx (1)
50-58: LGTM!The PaginationProps interface is well-defined with the new isPageListLimited property.
packages/suite/src/views/wallet/transactions/TransactionList/TransactionList.tsx (1)
30-41: LGTM!The TransactionListProps interface is well-defined with new props for enhanced configurability.
What to always check
Description
5.2.2025 from call todo:
Related Issue
Resolve
Test url: https://dev.suite.sldev.cz/suite-web/feat-infinite-scroll-transactions/web/
Screenshots:
UX stuff
The current state: coz there is an unknown number of "hidden" transactions, the total number of pages is unreliable. Here is, how it works with Polygon. It is far worse for Solana though, coz for Solana, we only load 4 transactions per page.
the.pages.gone.mov
Provisional empty state - when there are no visible transactions to be displayed