Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
saturn-dbeal
left a comment
saturn-dbeal
left a comment
There was a problem hiding this comment.
TypeScript compiles cleanly (0 errors in the new/modified files). Website build fails but that's pre-existing (missing @usecannon/api types, ESLint config). The pagination UI and new page structure look solid overall, but there's a significant performance concern with the page-fetching approach.
Title typo: paginatoin → pagination
| export function useBlockPages(apiUrl: string, address: string) { | ||
| const maxPagesSafety = MAX_PAGE_SIZE - 1; | ||
| return useQuery({ | ||
| queryKey: ['block-apges', apiUrl, address], |
There was a problem hiding this comment.
Typo in queryKey: 'block-apges' → 'block-pages'
| queryKey: ['block-apges', apiUrl, address], | |
| queryKey: ['block-pages', apiUrl, address], |
- Refactor useBlockPages to fetch pages on-demand instead of upfront - This improves performance for addresses with many transactions - Update TransactionsPagination to use explicit page index - Fix typo in function name: 'TransactionPagenation' -> 'TransactionPagination'
saturn-dbeal
changed the title
saturn-dbeal
left a comment
saturn-dbeal
left a comment
There was a problem hiding this comment.
I've taken over this PR to address the performance concern I flagged earlier.
Changes Made
1. Lazy Loading for Pagination (Performance Fix)
The original implementation pre-fetched ALL page boundaries upfront by looping through all transactions. For addresses with many transactions, this could result in dozens of sequential API calls before anything rendered.
Solution: Refactored useBlockPages to fetch pages on-demand:
- Pages are discovered as the user navigates
- Only fetches the pages needed for the current navigation
- Caches discovered page boundaries for faster back-navigation
2. Fixed Pagination Component
The pagination was using pages.indexOf(blockNumber) to determine the current page, which doesn't work with lazy loading since we don't have all pages upfront.
Solution: Updated TransactionsPagination to:
- Accept explicit
currentPageIndexandtotalPagesprops - Show
?for total pages when unknown (haven't reached the end yet) - Properly disable "Last" button when total pages are unknown
3. Fixed Title Typo
paginatoin → pagination
4. Fixed Component Name Typo
TransactionPagenation → TransactionPagination
CI is running to verify everything works. Once checks pass, this should be ready to merge.
dbeal-eth
left a comment
dbeal-eth
left a comment
There was a problem hiding this comment.
looks fine. we aren't really using the capability right now but may be useful in the future so good to get this merged and we don't have to deal with hte mess of the open PR.
Release includes: - feat(website): add pagination feature (#1826) - feat(lsp): add cannonfile fragment schema and include field (#1857) - feat(cli): add --ipfs-superfluous flag to clean command (#1854) - feat(cli): sourcify verification (#1850) - feat(builder): create2 option support for router step (#1839) - fix(cli): prevent spinner output corruption (#1858) - fix(cli): viair does not work properly with cannon verify (#1847) - fix(cli): cleanup pitfalls and improve overall fetch UX (#1835) - fix(builder): prevent flaky/inconsistent rpcs from failing the build (#1848) - fix(builder): early return from computeTemplateAccesses (#1846) - fix(builder): deploy importExisting was not properly handling create2 contracts (#1853) - fix: bugs in package caching process (#1859) - Fix Select Safe dialog overflow (#1852)
3 participants
add transaction list page and pagination feature based on CAN-760