feat: pagination with caching #312
Conversation
WalkthroughThe pull request introduces a new dependency, Changes
Sequence DiagramsequenceDiagram
participant UI as View Requests UI
participant QC as QueryClient
participant API as Request Network API
UI->>QC: fetchQuery(requestsQueryKey)
QC->>API: Fetch Requests
API-->>QC: Return Paginated Requests
QC-->>UI: Update Requests Data
UI->>UI: Render Requests
UI->>QC: Check hasMoreRequests
Possibly related PRs
Suggested reviewers
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: 1
🧹 Nitpick comments (2)
packages/invoice-dashboard/src/lib/view-requests.svelte (2)
209-209: Remove unnecessary console.log statementThe
console.logstatement can clutter production logs and potentially expose sensitive information. It's advisable to remove it or replace it with a proper logging mechanism if needed.Apply this diff to remove the console log:
- console.log("requestsData", requestsData);
526-526: Avoid redundant loading state assignmentThe
getRequestsfunction already manages theloadingstate by setting it tofalseupon completion. Settingloading = false;here is redundant and may lead to inconsistencies.Apply this diff to remove the redundant assignment:
- loading = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)packages/invoice-dashboard/src/lib/view-requests.svelte(9 hunks)shared/utils/queryClient.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- shared/utils/queryClient.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
package.json (1)
34-34: Verify @tanstack/svelte-query version and security.The addition of @tanstack/svelte-query aligns well with the PR objectives for implementing pagination and caching. However, let's verify the version specification.
Let's check for the latest stable version and any security advisories:
✅ Verification successful
@tanstack/svelte-query version is up-to-date and secure.
You're using the latest version (5.64.2) released on January 19, 2025, and there are no security advisories for this package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify @tanstack/svelte-query version and security # Check NPM for latest version echo "Latest version from NPM:" npm view @tanstack/svelte-query version # Check for security advisories echo -e "\nSecurity advisories:" npm audit @tanstack/svelte-query@5.64.2 # Check release date to ensure it's a stable version echo -e "\nRelease date:" npm view @tanstack/svelte-query@5.64.2 time.modifiedLength of output: 17175
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/invoice-dashboard/src/lib/view-requests.svelte (4)
115-117: Consider makingitemsPerPageconfigurable.The page size is currently hardcoded to 10. Consider making it configurable through props to allow for different use cases and screen sizes.
- const itemsPerPage = 10; + export let itemsPerPage = 10;
199-214: Enhance error handling and type safety infetchRequests.Consider these improvements:
- Add retry logic for transient failures
- Add TypeScript return type annotation
- Add specific error types for better error handling
- const fetchRequests = async (address: string, page: number, pageSize: number) => { + const fetchRequests = async (address: string, page: number, pageSize: number): Promise<Types.IRequestData | null> => { if (!address || !requestNetwork) return null; try { const requestsData = await requestNetwork.fromIdentity({ type: Types.Identity.TYPE.ETHEREUM_ADDRESS, value: address, }, undefined, { page: page, pageSize: pageSize, }); return requestsData; } catch (error) { - console.error("Failed to fetch requests:", error); - throw error; + if (error instanceof NetworkError) { + console.error("Network error while fetching requests:", error); + throw new Error(`Failed to fetch requests: ${error.message}`); + } + console.error("Unexpected error while fetching requests:", error); + throw error; } };
Line range hint
944-967: Add ARIA labels for better accessibility.The pagination buttons should have proper ARIA labels to improve accessibility for screen readers.
<button class="chevron-button" disabled={currentPage === 1} + aria-label="Previous page" on:click={() => goToPage(currentPage - 1)} > <i> <ChevronLeft /> </i> </button> <button class="chevron-button" disabled={!hasMoreRequests} + aria-label="Next page" on:click={() => goToPage(currentPage + 1)} > <i> <ChevronRight /> </i> </button>
529-531: Optimize query invalidation and refetch.The current implementation invalidates queries and immediately triggers a refetch, which could cause unnecessary network requests. Consider using the
invalidateQueriesoptions to control refetching behavior.- queryClient.invalidateQueries() - await getRequests(currentAccount, currentRequestNetwork); + await queryClient.invalidateQueries({ + queryKey: ['requestsData'], + refetchType: 'active' + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/invoice-dashboard/src/lib/view-requests.svelte(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/invoice-dashboard/src/lib/view-requests.svelte (1)
223-242: Well-implemented query caching and prefetching!The implementation includes several good practices:
- Proper query key management
- Data nullability handling
- Next page prefetching for better UX
- Appropriate error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/invoice-dashboard/package.json (1)
Line range hint
1-70: Add missing @tanstack/svelte-query dependency.The AI summary indicates that @tanstack/svelte-query is used for managing data fetching and caching, but it's not listed in the dependencies.
"dependencies": { + "@tanstack/svelte-query": "^5.0.0", "@requestnetwork/payment-detection": "0.51.0", "@requestnetwork/payment-processor": "0.54.0", "@requestnetwork/request-client.js": "0.56.0",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
packages/add-stakeholder/CHANGELOG.md(0 hunks)packages/create-invoice-form/CHANGELOG.md(0 hunks)packages/create-invoice-form/package.json(2 hunks)packages/invoice-dashboard/CHANGELOG.md(0 hunks)packages/invoice-dashboard/package.json(2 hunks)packages/payment-widget/CHANGELOG.md(0 hunks)packages/payment-widget/package.json(2 hunks)packages/single-invoice/package.json(2 hunks)
💤 Files with no reviewable changes (4)
- packages/invoice-dashboard/CHANGELOG.md
- packages/add-stakeholder/CHANGELOG.md
- packages/create-invoice-form/CHANGELOG.md
- packages/payment-widget/CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
packages/single-invoice/package.json (1)
3-3: LGTM on version bump.The version change from 0.0.0 to 0.1.1 follows semantic versioning principles, appropriate for a first minor release.
packages/payment-widget/package.json (2)
3-3: Verify if version bump aligns with changes.The minor version bump from 0.3.7 to 0.3.8 suggests backward-compatible changes. Let's verify if this aligns with the changes in this package.
✅ Verification successful
Version bump is appropriate for the changes
The version bump from 0.3.7 to 0.3.8 aligns with the scope of changes, which only includes dependency updates to @RequestNetwork packages. This follows semantic versioning principles for patch-level changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the scope of changes in this package to verify if version bump is appropriate # Check for breaking changes in this package git diff HEAD~1 packages/payment-widget/ | grep '^[-+]' | grep -v '^[-+]package.json' | grep -v '^[-+]package-lock.json' # Check commit messages for this package git log -n 10 --oneline packages/payment-widget/Length of output: 931
59-61: Verify if @tanstack/svelte-query is needed in this package.The PR objectives mention implementing query caching, and the AI summary indicates the addition of @tanstack/svelte-query. However, this dependency is not listed here. Let's verify if this package requires it.
packages/create-invoice-form/package.json (2)
3-3: Consider bumping the minor version instead of patch.The PR implements new features (pagination with caching) rather than just bug fixes. According to semantic versioning, new features should trigger a minor version bump (0.13.0) rather than a patch version bump (0.12.2).
36-37: Verify compatibility with the updated dependencies.The updates to
@requestnetwork/data-format(0.19.7) and@requestnetwork/request-client.js(0.56.0) might include breaking changes. Please ensure:
- The new request-client.js version supports the pagination features being implemented
- All features depending on these libraries still work as expected
packages/invoice-dashboard/package.json (1)
40-42: LGTM! Dependencies are consistently updated.The minor version updates across all RequestNetwork dependencies are consistent, which is good practice.
Fixes #229
Problem
Dashboard component does not work with SDK pagination and request query caching so UX for showing encrypted requests is too slow as it has to load all the wallets requests at once.
Changes
Add support to SDK pagination so it can retrieve requests on demand
Add query caching with prefetching so it enhances UX
Summary by CodeRabbit
Release Notes
New Features
Dependencies
@tanstack/svelte-querylibrary for improved data handling.Performance
Chores