fix: handle sparse minute kline history OK-60116 - #12909
Conversation
|
@codex review |
|
@codex security review |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![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: 5cc6ccaaa9
ℹ️ 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".
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit 00adb594c45f.
- P2 · Recovery stalls after eight empty active-day probes
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit c93ecf16543e.
- P2 · Viewport target is used as a sparse-recovery request budget
|
Addressed the remaining sparse-recovery review feedback in
All inline review threads have been replied to and resolved. |
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit b57cf1bd6ea1.
- P1 · Sparse recovery skips candles when a batched day range hits the Market page cap
Automated code review found blocking issuesReviewed commit Review summaryThe foreground TradingViewNative history state machine now keeps address-backed Market series stable across metadata changes, selects one primary or fallback source for all intervals, and recovers sparse 1-minute and 5-minute history by locating a coarse boundary and scanning older time windows. Initial loading and viewport pagination share the recovery machinery while realtime transport and the foreground-to-background Market service boundary remain unchanged. The source-consistency changes avoid mixing incompatible candles, but the sparse recovery loop still lacks a hard total request budget. What needs attention: Confirm that one sparse-history recovery batch cannot generate unbounded sequential foreground-to-background requests when occasional candles repeatedly reset the empty-window counter. Issues to address
Validation gaps
|
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit 22d94a73be79.
- P2 · Sparse recovery mixes fallback locator candles into primary Market pagination
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit 2c7b5bf9956c.
- P1 · Sparse recovery can launch an unbounded request chain
- P2 · Fallback locator candles can terminate primary Market history
|
@cursoragent review |
|
Taking a look!   |
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit 28a154061a19.
- P1 · Sparse recovery still has no request budget
- P2 · Fallback boundary can truncate primary Market history
|
@originalix Regarding P1 · Sparse recovery still has no request budget: The recovery target is intentionally based on actual viewport points, not wall-clock slots. For a sparse series, 200 one-minute points can span much more than 200 minutes. The time cursor still advances only through fully covered, contiguous time blocks ( The scan is finite and cancellable: it stops after reaching the viewport point target or the refined earliest-history boundary, and it observes the request Applying the suggested hard budget and waiting for a later viewport event would reintroduce the previous sparse-initial-load regression: if a batch finds no points, the chart has no visible-range change to trigger the next recovery round, so it can remain permanently underfilled. We therefore should not add that hard-stop behavior as suggested. Request pressure can be improved separately with adaptive time-block expansion or internally scheduled batches while preserving automatic point filling and continuous time coverage. Please consider downgrading this from a blocking P1; it does not indicate skipped data, an infinite loop, or an incorrect pagination boundary. |
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit 1aba8e862786.
- P1 · Sparse recovery still has no request budget
- P2 · Fallback boundary can truncate primary Market history
| while ( | ||
| cursorTimeTo >= boundaryTimestamp && | ||
| points.length < normalizedTargetPointCount | ||
| ) { |
There was a problem hiding this comment.
🟠 P1: Opening a 1-minute chart on a token with old but sparse data can fire hundreds of back-to-back price requests
Severity: severe
Older price data is walked backwards one fixed time window at a time with no limit on the number of requests (while loop at packages/kit/src/components/TradingView/TradingViewNative/data/useTradingViewNativeKLine.ts:794-797) until it either collects a screenful of candles or reaches the token's very first trading day, so a token whose minute data ends long before its first trading day triggers a very long chain of requests.
Impact: Charts for older, thinly traded tokens can hammer the price service with hundreds of sequential requests, leaving the chart busy for a long time and blocking other chart loading.
Why the walk is unbounded and how far it can run
recoverOlderHistoryFromBoundary first resolves the earliest available candle from the weekly/daily boundary page, then loops from timeTo downwards. Each iteration requests historyProvider.getHistoryRequestCandleCount(interval) candles worth of time (2000 for Market, i.e. ~33 hours for 1-minute candles, see MARKET_HISTORY_REQUEST_CANDLE_COUNT in packages/kit/src/components/TradingView/TradingViewNative/data/providers/market/marketDataProvider.ts:33). The only exit conditions are cursorTimeTo < boundaryTimestamp or points.length >= normalizedTargetPointCount. When the API has no minute candles for the older period (common when minute retention is much shorter than the token's history), every window returns zero points, the target is never reached, and the cursor only advances ~33 hours per request — e.g. a boundary 1 year older than the last minute candle produces ~260 sequential requests, each of which is additionally retried up to 3 times on failure via fetchRequiredHistoryPage (packages/kit/src/components/TradingView/TradingViewNative/data/useTradingViewNativeKLine.ts:549-570).
This path runs automatically right after the initial history load (packages/kit/src/components/TradingView/TradingViewNative/data/useTradingViewNativeKLine.ts:4079-4091), i.e. without any user interaction, and again on every older-page load. Note that all other scan loops in this file are explicitly bounded (HISTORY_GAP_EMPTY_SCAN_PAGE_COUNT, MAX_VIEWPORT_HISTORY_PAGE_COUNT, MAX_VIEWPORT_HISTORY_BOUNDARY_SEARCH_COUNT), and the PR description itself states each recovery round should be limited to about eight minute-history requests, which the code does not implement.
There was a problem hiding this comment.
已处理“连续全空区间可能一直请求”的情况：a17846ca28 增加了每轮最多 10 个连续空窗的停止条件，停止时保留 cursor 和 hasMoreBefore，后续 viewport load-more 可以从该 cursor 继续。bfe2ae18c4 进一步保证所有粒度共享同一个 history source，异源空页不会错误终止 primary 恢复。这里没有增加总窗口数上限；按当前产品语义，出现任意有效 K 线会重置连续空窗计数，以便继续找到真正稀疏的数据。
| const boundaryPage = await (getHistoryBoundaryPrefetchPage(seriesKey) ?? | ||
| prefetchHistoryBoundaryPage({ historyProvider, seriesKey })); | ||
| if ( | ||
| signal.aborted || | ||
| !boundaryPage || | ||
| boundaryPage.hasMoreBefore || | ||
| boundaryPage.earliestTimestamp === undefined | ||
| ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Earliest-date lookup is re-run from scratch on every sparse-history attempt when it cannot determine the first trading day
Severity: non-severe
The earliest-date lookup result is re-requested every time older data is recovered (getHistoryBoundaryPrefetchPage(seriesKey) ?? prefetchHistoryBoundaryPage(...) at packages/kit/src/components/TradingView/TradingViewNative/data/useTradingViewNativeKLine.ts:756-757) because inconclusive lookups are never remembered, so each attempt repeats up to twenty weekly data requests.
Impact: For tokens with very long histories, scrolling back repeatedly issues the same large batch of extra requests over and over, slowing chart loading.
Caching gap in prefetchHistoryBoundaryPage
prefetchHistoryBoundaryPage only writes to historyBoundaryPrefetchCache / historyBoundaryTimestampCache on the success path (packages/kit/src/components/TradingView/TradingViewNative/data/useTradingViewNativeKLine.ts:710-714). When the weekly scan exhausts MAX_VIEWPORT_HISTORY_PAGE_COUNT (20) pages with hasMoreBefore === true, it returns early at packages/kit/src/components/TradingView/TradingViewNative/data/useTradingViewNativeKLine.ts:652-660 without caching, and the in-flight entry is removed in the finally block. Previously this lookup only ran when the user opened the calendar; the new sparse-history recovery calls it after the initial minute-history load and on every subsequent older-page load, so the uncached 20-page weekly scan (plus retries) is repeated on each of those attempts even though its outcome cannot change within the cache TTL.
There was a problem hiding this comment.
这里暂不缓存 hasMoreBefore 仍为 true 的“不确定结果”，因为它不是一个可安全使用的最早历史边界，缓存后反而可能隐藏更早数据。weekly API 每页至少覆盖 200 根、合约币覆盖 299 根周 K；20 页相当于约 77–115 年历史，正常 Market 数据不会走到这个不确定分支。如果线上确实观察到该分支重复触发，可以单独增加带 TTL 的 negative/inconclusive cache，但不应把它当成真实 boundary。
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit 2f3ff51f35e0.
- P1 · Sparse recovery still has no request budget
- P2 · Fallback boundary can truncate primary Market history
| }); | ||
| consecutiveEmptyWindowCount = rangePoints.length | ||
| ? 0 | ||
| : consecutiveEmptyWindowCount + 1; |
There was a problem hiding this comment.
🟠 P1 空窗上限被任意非空窗口清零，稀疏代币仍无请求上限
问题
新增的 MAX_SPARSE_HISTORY_CONSECUTIVE_EMPTY_WINDOW_COUNT = 10 只统计连续空窗，任意一个窗口返回哪怕 1 根 K 线就把计数清零：
consecutiveEmptyWindowCount = rangePoints.length
? 0
: consecutiveEmptyWindowCount + 1;窗口宽度是 getHistoryRequestCandleCount * interval.seconds，Market 为 2000 * 60 = 120000 秒（1m 约 33.3 小时）与 2000 * 300 = 600000 秒（5m 约 6.9 天）。也就是说该上限要求连续约 13.9 天（1m）或约 69.4 天（5m）完全没有任何分钟 K 线才会生效。而本 PR 针对的正是"稀疏"而非"整段为空"的分钟历史：长尾代币典型形态是每隔数天出现一次成交爆发、每次只有几根 1m K 线，这种数据每隔几个窗口就清零一次计数，主循环退出条件重新退回到 points.length >= normalizedTargetPointCount 或 cursorTimeTo < boundaryTimestamp。
影响
在稀疏但非全空的代币上，单轮 recovery 的请求数仍无上限。初始路径目标是 198 - receivedHistoryPointCount，若每个 33.3 小时窗口平均只有 1 根 K 线，凑满目标需要约 198 个串行窗口；若先到达 boundaryTimestamp，上限就是整段跨度除以 33.3 小时（边界在两年前时约 525 次）。这期间 pagination.isLoading 恒为 true，handleVisiblePointRangeChange 全程提前返回，缺口回填与新数据加载都被冻结。另外 fetchRequiredHistoryPage 的 1s/3s 重试不计入窗口数，单个窗口失败时最多 3 次请求，因此即使命中上限，一轮的实际请求数也可达 30 次。
新增用例只覆盖全空窗口（stops a recovery batch after ten consecutive empty time windows）与"第 10、20 个窗口各返回 1 根"（resets the consecutive empty-window limit after receiving a candle，断言恰好 21 次请求），后者恰恰证明了清零语义会让请求数随稀疏程度线性增长，但没有任何用例给整轮请求数设上界。
建议
在连续空窗计数之外，再给整轮 recovery 加一个窗口总数上限，命中后同样以 hasMoreBefore: true 返回当前 cursorTimestamp，语义与现有空窗上限一致：
const MAX_SPARSE_HISTORY_RECOVERY_WINDOW_COUNT = 24;
let windowCount = 0;
while (
cursorTimeTo >= boundaryTimestamp &&
points.length < normalizedTargetPointCount &&
consecutiveEmptyWindowCount < MAX_SPARSE_HISTORY_CONSECUTIVE_EMPTY_WINDOW_COUNT &&
windowCount < MAX_SPARSE_HISTORY_RECOVERY_WINDOW_COUNT
) {
windowCount += 1;
// ...
}或对空窗采用倍增窗口宽度（每次空窗把 requestCandleCount 乘 2，非空时复位），使跨越长空档从线性降到对数级。回归用例建议用真实的 getHistoryRequestCandleCount = 2000、边界在数年前、每隔 9 个窗口返回 1 根 K 线，断言 mockFetchHistory 调用次数不超过该上限。
Generated by Claude Code
There was a problem hiding this comment.
这个行为是有意保留的产品取舍：单轮按实际 K 线根数补足下一屏，连续 10 个空窗才暂停；任意非空窗口会重置计数。增加固定总窗口上限会在仍能找到稀疏 K 线时提前停止，而且初始图表未填满时不一定会产生新的 viewport 事件来自动续跑。当前实现仍是有限且可取消的：达到目标、真实边界或 10 个连续空窗都会退出，并保留 cursor 供后续 load-more。bfe2ae18c4 没有改变这一语义。
There was a problem hiding this comment.
你对固定总窗口上限的反对成立，我接受：命中上限时若初始图尚未填满，确实可能没有新的 viewport 事件来续跑，把「请求过多」换成「图表欠填且不自愈」是更差的结果。这一点我原建议里没有考虑到。
不过这条反对只适用于固定上限，不适用于我在另一条线程提过、你自己也提到过的倍增窗口方案，这条线程里还没讨论过它：空窗时把 requestCandleCount 乘 2（非空即复位），不设任何提前停止条件。退出条件仍然只有目标点数、真实边界、10 个连续空窗三个，所以不存在提前停止导致欠填的问题；差别只是跨越空档的请求数从线性降到对数级——1m 下两年空档从约 525 次降到约 10 次，且因为窗口只在空窗后才扩大，稀疏但有数据的区间宽度不变，不会跳过任何 K 线。
代价是单个请求的时间跨度变大，若某个扩大后的窗口恰好命中密集区，返回可能被 299/200 截断；此时按返回的最早 K 线设游标并复位窗口宽度即可，不会丢数据。
不主张一定要改——按当前语义它是有限且可取消的，这是你的取舍。只是想说明「提前停止」这一条不构成对倍增方案的反对，如果你之前是基于这一点一并排除了它，可以再单独评估一次。
Generated by Claude Code
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit a17846ca28a8.
- P1 · Consecutive-empty cap still permits oversized recovery batches
- P2 · Daily fallback is cached as a primary history boundary
- P1 · A rejected fallback page ends primary sparse recovery
|
@originalix Addressed the two history-source transition findings in bfe2ae1:
Validation: focused provider and K-line suites pass 101/101; yarn agent:check --profile commit passes lint, format, and TypeScript checks. The separate total-window budget was intentionally not added. The current product rule remains: scan fixed candle-count windows and pause after 10 consecutive empty windows; any non-empty window resets that counter. This preserves discovery of genuinely sparse candles while bounding fully empty gaps. Please re-review bfe2ae1. |
originalix
left a comment
originalix
left a comment
There was a problem hiding this comment.
Automated code review found blocking issues
Reviewed commit bfe2ae18c4e4.
- P1 · Consecutive-empty cap still permits oversized recovery batches
3 participants
OK-60116
Summary
Root cause
Market history treated a short or empty response as the end of history. Tokens with gaps therefore stopped pagination even when older candles existed. The first sparse recovery implementation also scanned one active day per request, which either stopped after eight empty candidates or removed the cap and could issue hundreds of sequential requests.
Impact
Sparse 1m/5m charts now resolve the real earliest date with the shared 1W/1D boundary flow, cross more than eight empty active-day candidates automatically, and keep each recovery round bounded to eight minute-history requests. CoinGecko fallback and Hyperliquid behavior remain isolated from Market sparse-history recovery.
Validation
yarn jest packages/kit/src/components/TradingView/TradingViewNative/data/useTradingViewNativeKLine.test.ts --runInBand(85/85)yarn agent:check --profile commityarn agent:check --profile pr(all local checks passed)Issue: OK-60116