fix(sdk): full-size first page for hafah transactions, no initialData seed on notifications - #1427
Merged
Conversation
hafah paginates oldest-first, so an omitted page returns the remainder bucket (total_operations mod page-size rows, as short as one row) and requesting page=total_pages explicitly returns the same short bucket. When the first page comes back short and older pages exist, chain one fetch of page total_pages-1 into the first result so the list opens at full size. If the chained fetch fails, the short page is kept and the cursor stays at total_pages so the page is retried by load-more.
…nite query An empty initialData page set counts as fresh for the whole staleTime, so a consumer that only reads data renders an empty list with no fetch, no loading state and no error. Existing web consumers force-refetch on mount and are unaffected. The spec pins the absence of initialData for the notifications options and the wallet asset transaction options, which had the same seed removed earlier.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe SDK now tops up short initial transaction pages from the next older page and preserves cursors on failure. It also removes empty ChangesSDK query behavior
Release metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant QueryOptions as getTransactionsInfiniteQueryOptions
participant PageHelper as page-fetch helper
participant Hafah as hafah REST API
QueryOptions->>PageHelper: Fetch initial transaction page
PageHelper->>Hafah: Request newest page
Hafah-->>PageHelper: Return short page and cursor
QueryOptions->>PageHelper: Fetch next older page
PageHelper->>Hafah: Request explicit older page
Hafah-->>PageHelper: Return older entries
PageHelper-->>QueryOptions: Return combined transaction entries
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/sdk/src/modules/accounts/queries/get-transactions-infinite-query-options.spec.ts (1)
51-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the initial request omits
page.The test checks
page: 145only on the chained request. Add an assertion for the first request. This guards the required omitted-page behavior.Proposed test addition
expect(page.currentPage).toBe(145); expect(mockCallREST).toHaveBeenCalledTimes(2); + expect(mockCallREST.mock.calls[0][2]).not.toHaveProperty("page"); // The chained call must ask for the page BELOW the remainder bucket —🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/modules/accounts/queries/get-transactions-infinite-query-options.spec.ts` around lines 51 - 64, The test case “chains the next older page when the newest bucket is short” must also assert that the initial mockCallREST invocation omits the page option. Add an assertion for the first call’s request options before retaining the existing page: 145 assertion for the chained call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/sdk/src/modules/accounts/queries/get-transactions-infinite-query-options.ts`:
- Around line 215-235: Update the catch block in the initial-page chaining logic
around fetchPage so it re-throws the caught error when signal?.aborted is true,
while retaining the short-page fallback only for non-abort failures.
---
Nitpick comments:
In
`@packages/sdk/src/modules/accounts/queries/get-transactions-infinite-query-options.spec.ts`:
- Around line 51-64: The test case “chains the next older page when the newest
bucket is short” must also assert that the initial mockCallREST invocation omits
the page option. Add an assertion for the first call’s request options before
retaining the existing page: 145 assertion for the chained call.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 121da9b5-0e3b-44d1-9994-55e682ebceb6
📒 Files selected for processing (4)
packages/sdk/src/modules/accounts/queries/get-transactions-infinite-query-options.spec.tspackages/sdk/src/modules/accounts/queries/get-transactions-infinite-query-options.tspackages/sdk/src/modules/notifications/queries/get-notifications-infinite-query-options.spec.tspackages/sdk/src/modules/notifications/queries/get-notifications-infinite-query-options.ts
Swallowing an abort resolved the query with a partial page after the caller had already cancelled. Non-abort failures still fall back to the short remainder page with the cursor kept at total_pages.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bundles the two open SDK issues.
#1409 - short first page on the hafah transactions query. hafah paginates oldest-first, so the first request (omitted
page) returns the remainder bucket, between 1 and page-size rows, and requestingpage=total_pagesexplicitly returns the same short bucket (verified live: good-karma, page-size 20 returns 17 rows either way). The queryFn now chains one fetch oftotal_pages - 1when the first page comes back short and older pages exist, so the list opens at full size. A failed chained fetch keeps the short page with the cursor still attotal_pages, so that page becomes the next load-more target instead of being skipped. Affects the profile transactions component, deck wallet column and transfer recent-recipient suggestions.#1405 - notifications infinite query seeds an empty page set.
initialData: { pages: [], pageParams: [] }counts as fresh for the whole staleTime, so a consumer that only readsdatarenders an empty list with no fetch. Removed; the three web consumers force-refetch on mount or only userefetch/isLoading, so nothing changes for them. New spec pins the absence ofinitialDataon the notifications options and the HIVE/HBD/HP asset transaction options.Verified: sdk vitest suite (733 tests), sdk lint, sdk + web typecheck all pass.
Closes #1409
Closes #1405
Summary by CodeRabbit