fix: stabilize wallet home asset refresh(OK-61576) - #13103
Conversation
|
@codex review |
|
@codex security review |
|
@cursoragent review |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
PR #13103 代码审查报告审查概要 [REQUIRED]
评分 [REQUIRED — NEVER SKIP THIS SECTION]
PR 评论分析 [REQUIRED if comments exist, OMIT if none]
评论误报分析
发现的问题 [REQUIRED][🟡 中] [🔵 High] Accepted refresh 失败后会丢掉 last-good 结果文件: 后果：
if (onStartedTask) {
await onStartedTask;
if (onStartedError) {
if (onStartedError instanceof Error) {
// oxlint-disable-next-line no-throw-literal
throw onStartedError;
}
const err = new Error('onStarted failed');
throw err;
}
}修复建议: 只在成功 resolve 后丢弃上一份 published result；失败时回滚。不要靠 Auto-fix: lastRunSignatureRef.current = currentRunSignature;
+ const previousPublishedResult = lastPublishedResultRef.current;
if (undefinedResultIfReRun) {
// Keep a queued refresh from restoring the prior published result
// after the new run has already invalidated it.
clearResultRef.current?.();
lastPublishedResultRef.current = undefined;
}
+ const restorePublishedResultAfterFailure = () => {
+ if (!undefinedResultIfReRun) {
+ return;
+ }
+ lastPublishedResultRef.current = previousPublishedResult;
+ if (
+ previousPublishedResult?.runSignature === currentRunSignature
+ ) {
+ clearResultRef.current = undefined;
+ }
+ };更干净的写法是把 fan-out 包进独立 推荐实现： lastRunSignatureRef.current = currentRunSignature;
+ const previousPublishedResult = lastPublishedResultRef.current;
if (undefinedResultIfReRun) {
clearResultRef.current?.();
lastPublishedResultRef.current = undefined;
}
runCountRef.current += 1;
// ...
try {
// existing fan-out
} finally {
isFetching.current = false;
hasQueuedRerun = scheduleQueuedRerun();
}
+ } catch (error) {
+ if (undefinedResultIfReRun) {
+ lastPublishedResultRef.current = previousPublishedResult;
+ if (
+ previousPublishedResult?.runSignature === currentRunSignature
+ ) {
+ setResult(previousPublishedResult.result);
+ }
+ }
+ throw error;
+ }注意：现有 [🟡 中] [🟠 Medium] 新增队列语义没有行为测试文件:
仓库里对同类逻辑已经抽过 修复建议: 抽出纯函数（例如
analytics 的源码顺序测试可以留作锁栏，但不能代替这些。 [🟢 低] [🟠 Medium]
|
| 优先级 | 置信度 | 文件 | 类型 | 描述 | Auto-fix |
|---|---|---|---|---|---|
| 🟡 中 | 🔵 High | useAllNetwork.ts:610 |
运行时 | accepted fan-out 失败后 last-good 结果无法恢复 | ✅ |
| 🟡 中 | 🟠 Medium | useAllNetwork.ts:506 / TokenListBlock.portfolioSync.test.ts:47 |
规范 | 队列/失败恢复缺少行为测试 | — |
| 🟢 低 | 🟠 Medium | useAllNetwork.ts:344 |
规范 | 旗标与 usePromiseResult 同名不同义 |
— |
测试建议 [REQUIRED]
- All-networks Home：连续下拉刷新，第二次落在 1s debounce 内，第二次必须真正发请求，且第一次成功结果不能被依赖重入吃掉。
- 刷新开始后让
getAllNetworkAccounts/onStarted失败：列表应保留上一份权威快照，随后的依赖重入不应把allNetworksResult钉死在undefined。 - 后台/前台、账户切换、HW
AddDBAccountsToWallet：must-run flag 合并后仍能穿过 redundant-run gate。 - 回归：
yarn jest packages/kit/src/hooks/allNetworkRunResultUtils.test.ts packages/kit/src/hooks/shouldSkipRedundantAllNetworkRun.test.ts packages/kit/src/views/Home/components/TokenListBlock --runInBand - 手工：iOS 挂起仍可能拖住 JS，PR 已说明；重点看 resume 后下拉刷新是否还能提交，而不是只看 spinner。
- NFT / DeFi 未开启该旗标，抽查一次确认调度行为与
release/v6.5.2一致。
GH 评论操作 [REQUIRED if qualifying findings exist, OMIT if none]
以下问题（🔵 High 置信度 + 🟡 中及以上）建议直接评论到 PR：
- Accepted refresh 失败后丢掉 last-good 结果 —
packages/kit/src/hooks/useAllNetwork.ts:610
确认后将通过
ghCLI 发送 inline comments。
主修复是对的：getAllHdHwQrWallets 那条慢路径不再挡住 commitAuthoritativeIngest / updateTokenListState，Home 的手动刷新也不会在 debounce 里再开一条 runner 冲掉 nonce。NFT/DeFi 默认关旗标，影响面可控。
合入前建议先补上失败回滚，并把队列决策抽成可测的纯函数。需要的话我可以直接改并补测试。
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2678b492a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Automated code review found blocking issuesReviewed commit Review summaryThe change adds freshness metadata to wallet asset snapshots across the background fetch, persistence, and foreground account-overview paths. Before, each settled response could replace shared worth/cache state without a durable ordering marker; after, background fetches mint request-order metadata, HTTP Date is retained as a tie-breaker, and the active atom plus SimpleDB admit or reject writes by snapshot freshness. Full all-network replacements additionally require a complete marker and preserve newer omitted networks. Cache migration tolerance, no-op writes, token-list propagation, and existing token rendering/filter behavior remain otherwise unchanged. The foreground and background runtimes each hold their own JavaScript copies while the background runtime persists through shared native storage, so marker propagation is the synchronization boundary. What needs attention: Verify that a versioned merge:false single-network refresh replaces the scoped worth map while retaining only a stale-safe value for the requested key. Issues to address
Validation gaps
|
| const preserveCurrentValues = | ||
| sameAccount && (!isCompleteSnapshot || !canReplaceFullSnapshot); | ||
| const nextWorth: Record<string, string> = preserveCurrentValues | ||
| ? { ...currentValue } | ||
| : {}; | ||
| const nextMetaByKey: Record<string, IAssetSnapshotMeta> = | ||
| preserveCurrentValues ? { ...currentMetaByKey } : {}; |
There was a problem hiding this comment.
🟠 P1: Single-network switches retain prior balances
Severity: severe
In the main-runtime Jotai copy, preserveCurrentValues retains the previous network when a versioned account switches networks. Merge-derived views sum both values and inflate the balance; no shared native resource or background copy participates.
There was a problem hiding this comment.
Analyzed with full codebase context — keeping this behavior as designed, no code change for now. Reasoning:
- The retention is the atom's intended cross-network-map semantics (codified by the new tests, e.g.
retains omitted networks when an unversioned all-network cache is partial).worthis keyed by(accountId, networkId), so a retained entry is the other network's real balance under its own key — never a second copy of the current network's value, so per-key consumers cannot double count. - The displayed Home total does not sum this map. It reads
lastConfirmedOverviewBalance.byOwner[ownerKey](keyed per owner+network,HomeOverviewContainer.tsx), so the visible balance is unaffected by retained sibling-network keys. - The only consumers that sum the whole map are the walletStatus
hasValuethreshold (HomeOverviewContainer.tsxallWorth) anduseHomeBalanceState's funded-positivity bucket. Both are wallet/account-level signals where cross-network aggregation is semantically acceptable, and both already saw the same values whenever the all-network path had run. - HD accounts switching across impls (e.g. ETH -> BTC) change the payload
accountId, sosameAccountis false and a full replacement clears the map. The retention window is limited to same-account-id families (EVM networks, Others accounts) — exactly where the values remain valid balances.
If we later want single-network views to drop sibling-network keys, that should be a deliberate change on the reset path in HomeOverviewContainer, not in the freshness admission logic. Leaving this thread open for your confirmation — happy to follow up if there is a concrete surface where the summed value is user-visible.
…shot before asset analytics OK-61576
…sisting as network totals OK-61576
…annot pin a stale total OK-61576
|
Status update after re-analysing the customer's iOS log (6.5.2, 2026-09-01): The header/list mismatch in the screenshot is the To keep the 6.5.2 hotfix small, the evidence-backed parts of this PR (refresh queue race, analytics-after-commit, This PR keeps the full |
Stacked on fix/wallet-home-header-hold-v6.5.2 (#13124). This commit carries only the IAssetSnapshotMeta freshness system so it can soak separately: - shared: assetSnapshotFreshness (localSeq primary, HTTP Date tie-break, watermark self-heal), IAssetSnapshotMeta types, resp.assetSnapshotMeta - bg: ServiceToken mints one marker per fetch; ServiceAccountProfile admits active-account-value writes under a mutex; SimpleDb accountValue / localTokens admit same-or-newer supplied keys, strictly newer for omitted keys, tolerate pre-migration legacy buckets, skip no-op writes - main: accountOverview.updateAccountWorth admission (equal markers admitted for full replacements, retained currency when every value is rejected, createAtNetworkWorth recomputed from absolute values, reset flag), buildMergedAllNetworkSnapshot / TokenListBlock / HomeOverviewContainer / TokenSelector thread the markers through to persistence
7a96275 to
6049014
Compare
|
Restacked: this branch was rewritten on top of The previous head (7a96275, full history incl. review-round commits) is preserved at |
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit 604901406225.
- P1 · Versioned single-network updates retain previous-network balances
3 participants
OK-61576
Summary
localSeq+ HTTP Date marker) and apply last-writer-wins admission at every persistence/state layer, so an out-of-order stale response can no longer overwrite newer data.Intent & Context
Users reported that wallet balances and prices could remain stale or inconsistent after repeated refreshes, app background/foreground transitions, and switching between mobile and desktop. The investigation showed successful token-list API responses followed by delayed or missing UI publication.
Root Cause
getAllHdHwQrWalletscall took about 93 seconds, and iOS suspension amplified the delay.useAllNetworkRequestsallowed manual/event refreshes to overlap its internal debounce and in-flight queue; queued must-run intent could be lost or filtered as a redundant run while the previous result remained visible.Design Decisions
usePromiseResultbehavior or unrelated consumers. The Home-only flag is namedclearRetainedResultOnAcceptedRunto avoid confusion withusePromiseResult's same-named-but-differentundefinedResultIfReRunoption.localSeqper token fetch in the background runtime (the single sequencer) and attach it to responses asassetSnapshotMeta. Every write layer (SimpleDbEntityLocalTokens,SimpleDbEntityAccountValue,activeAccountValueAtomviaServiceAccountProfile,accountWorthAtomvia accountOverview actions) admits a write only when it is newer than the stored marker; unversioned legacy writes may initialize a key but never clobber versioned data.@onekeyhq/shared/src/utils/assetSnapshotFreshnessand are shared by all layers, so the comparison semantics cannot drift.Changes Detail
Refresh scheduling and publication:
packages/kit/src/views/Home/components/TokenListBlock/TokenListBlock.tsx: opt Home into retained-result handling, publish the authoritative snapshot before asset-status analytics, and drop the duplicateAddDBAccountsToWalletrefresh (the hook-level wallet-scoped listener already forces one fan-out with an accounts-cache bypass; only the LP token list keeps a dedicated trigger).packages/kit/src/hooks/useAllNetwork.ts: reserve debounce windows, merge queued must-run configuration, distinguish accepted refreshes from redundant runs; restore the last-good published result when an accepted fan-out fails before publication; bail out stale runners whose closure belongs to a previous owner (they could otherwise clear the new owner's result and run a ghost fan-out).packages/kit/src/hooks/allNetworkRunResultUtils.ts: pureresolveAllNetworkFailedRunRestoredecision + tests.Snapshot freshness admission:
packages/shared/src/utils/assetSnapshotFreshness.ts(+packages/shared/types/assetSnapshot.ts): sequence minting, header parsing, tolerant comparison, shared admission helpers (canApplyAssetSnapshotMeta,getNewestAssetSnapshotMeta, …), clock-rollback watermark self-heal.packages/kit-bg/src/services/ServiceToken.ts: mint the marker before each fetch, thread it through responses and the local token cache; failed debounced cache flushes merge back and reschedule persistence.packages/kit-bg/src/dbs/simple/entity/SimpleDbEntityLocalTokens.ts/SimpleDbEntityAccountValue.ts: versioned admission inside the entity mutex for token-list slices and account values (full-replace vs per-network merge).packages/kit-bg/src/services/ServiceAccountProfile.ts: mutex-serialized active-account-value atom updates with owner-aware freshness admission; the old "value can only go up" heuristic is replaced by the freshness gate (legitimate balance decreases now propagate).packages/kit/src/states/jotai/contexts/accountOverview/actions.ts/atoms.ts: worth-map admission with per-key and aggregate markers; create-at-network scalar recomputed from materialized absolute values.packages/kit/src/views/Home/pages/HomeOverviewContainer.tsx: persist per-network values by compound key with their markers; filtered TokenSelector responses no longer persist as canonical network totals (packages/kit/src/views/AssetSelector/pages/TokenSelector.tsx).Risk Assessment
Test plan
yarn jest packages/kit/src/views/Home/components/TokenListBlock packages/kit-bg/src/dbs/simple/entity/SimpleDbEntityAssetSnapshot.test.ts packages/kit-bg/src/services/ServiceAccountProfile.updateAllNetworkAccountValue.test.ts packages/kit/src/states/jotai/contexts/accountOverview/actions.test.tsx packages/shared/src/utils/assetSnapshotFreshness.test.ts packages/kit/src/hooks/allNetworkRunResultUtils.test.ts --runInBand(12 suites, 96 tests passed)yarn agent:check --profile commit(lint-worktree-ts, lint-staged, tsc-staged all passed)getAllNetworkAccounts/onStartedto fail mid-refresh; the list must keep the last authoritative snapshot and later pull-to-refresh must still commit