Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/UX and consistency issues to address (notably initial load/cleanup ordering in WalletPage and missing localized keys in other satellite .resx files).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds explicit NFT “load more” pagination to the wallet so enumeration can continue past the first 100 items, while ensuring RPC iterator sessions are terminated on completion, failure, cancellation, or when leaving the wallet page.
Changes:
- Introduces
NftPager+NftPageSessionto page NFT enumeration (100 per UI page; bounded 25-item iterator/metadata batches) and reliably dispose/terminate iterator sessions. - Updates
RpcClientandTokenManagerto expose NFT paging as an async stream and to propagate cancellation through RPC calls. - Updates
WalletPageUI/state to support loading, failure messaging, and a “Load more NFTs” footer; adds localization keys and adds a focused test project validating paging + cleanup behaviors.
File summaries
| File | Description |
|---|---|
| tests/p2-06/NftPagerTests.cs | Adds unit tests covering multi-collection paging, cancellation/disposal, metadata failure, oversized replies, and cleanup behavior. |
| tests/p2-06/NftPager.Tests.csproj | New test project compiling pager/session sources via linked files and referencing app project without building it. |
| OneGateApp/Services/TokenManager.cs | Switches NFT loading from single-shot list to async page enumeration. |
| OneGateApp/Services/RPC/RpcClient.cs | Adds cancellation-aware RPC send and replaces GetNFTs with iterator-based GetNFTPages plus session termination. |
| OneGateApp/Services/RPC/NftPager.cs | New pager that stitches per-collection iterator traversals into 100-item UI pages with bounded batches. |
| OneGateApp/Services/NftPageSession.cs | New UI session wrapper ensuring safe cancellation + async-enumerator disposal sequencing. |
| OneGateApp/Pages/WalletPage.xaml.cs | Moves wallet NFT loading to paged reads, adds “load more”/loading/failure state, and closes sessions on page exit. |
| OneGateApp/Pages/WalletPage.xaml | Adds footer UI for failure text, spinner, and “Load more NFTs” button; adjusts empty-state visibility. |
| OneGateApp/Properties/Strings.resx | Adds new resource keys for “Load more NFTs” and partial-load failure message (EN). |
| OneGateApp/Properties/Strings.zh-Hans.resx | Adds Simplified Chinese translations for the new NFT paging UI strings. |
| OneGateApp/Properties/Strings.Designer.cs | Adds strongly-typed accessors for the new resource keys. |
| OneGateApp.slnx | Registers the new test project in the solution. |
Review details
Files not reviewed (1)
- OneGateApp/Properties/Strings.Designer.cs: Generated file
- Files reviewed: 11/12 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+89
to
+93
| NftLoadFailed = false; | ||
| HasMoreNFTs = true; | ||
| IsLoadingNFTs = false; | ||
| if (previous is not null) await CloseNftSessionAsync(previous); | ||
| await LoadNFTPageAsync(session); |
Comment on lines
79
to
+84
| var tokens = await LoadNep11TokensAsync(includeHiddens); | ||
| NFT[] nfts = await rpcClient.GetNFTs(account.ScriptHash, tokens.Select(p => p.Hash).ToArray()); | ||
| foreach (NFT nft in nfts) | ||
| nft.TokenInfo = tokens.First(p => p.Hash == nft.CollectionId); | ||
| return nfts; | ||
| await foreach (NFT[] nfts in rpcClient.GetNFTPages(account.ScriptHash, tokens.Select(p => p.Hash).ToArray(), cancellationToken)) | ||
| { | ||
| foreach (NFT nft in nfts) nft.TokenInfo = tokens.First(p => p.Hash == nft.CollectionId); | ||
| yield return nfts; | ||
| } |
Comment on lines
+1291
to
+1296
| <data name="LoadMoreNFTs" xml:space="preserve"> | ||
| <value>Load more NFTs</value> | ||
| </data> | ||
| <data name="NFTLoadFailed" xml:space="preserve"> | ||
| <value>Some NFTs could not be loaded. Pull down to retry.</value> | ||
| </data> |
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
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.
Summary
The wallet currently stops NFT enumeration after 100 items across all curated collections. This change adds explicit load-more pagination so later items and collections remain reachable, while releasing the node iterator session when enumeration finishes, fails, or the wallet page is left.
Validation
dotnet test tests/p2-06/NftPager.Tests.csproj --no-restore --nologo: 22 passed. Covers 137 NFTs across two collections with a seven-item server limit, disposal/cancellation, failure, oversized replies, serialized cleanup, two production-WalletPage handover regressions and 13 satellite locales.WalletPage,TokenManager, andRpcClientusing controlled HTTP responses: first 100 items, load-more to all 137 including the second collection, iterator cleanup on completion and leaving, fresh first-page loading on return, and bounded requests. No live assets were signed or broadcast. This is synthetic-inventory integration evidence, not live-chain wallet validation.terminatesessionHTTP request. During that wait, two actual load-more handler calls could not start reading the replacement session. After release only its first 100 items loaded; extra clicks after all 137 items completed did not set failure. Each of 13 actual satellite resource sets supplied both keys without parent/English fallback.c159c9ahas SHA-256b9fc1d71adf60ad3e62c30603a46c0d30df4c19ff0455104520343904af1d520; full patch against623603dis3d5e097d003edba687aee579b1420034b0f85256a623244fe9bbb169b4633cb5. Both native follow-up runs match these hashes; older 8-check runs are not used as substitute evidence. Screenshots and QA artifacts remain outside the repository; no uploaded screenshot or hosted CI result is claimed here.Limits and integration notes
masterat623603d634f07eaead14745da87920a356159be0, not a combined validation of other audit branches. When integrating other acceptedRpcClientfixes, preserve their transfer/signer checks alongside this cancellation and pagination code; linked-source test projects that includeRpcClientwill also need the new pager source.