fix(ui): reduce orderbook API bursts and improve trade load - #192
Open
Siddharth2207 wants to merge 1 commit into
Open
Siddharth2207 wants to merge 1 commit into
Siddharth2207 wants to merge 1 commit into
Conversation
Limit parallel orders/token fetches to 4, remove global prefetch on home and trade, slow dashboard poll to 30s, scope deploy invalidation to the deployed asset, and defer secondary trade-page queries after first paint. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR optimizes query performance and data freshness by introducing concurrency limiting for order fetches, standardizing query builder parameters across the application, refining query invalidation semantics, and refactoring the trade page to defer secondary queries and centralize token resolution via Pyth token data. ChangesQuery concurrency and freshness optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: dependency version conflict. Check your lock file or package.json. 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/lib/utils/mapWithConcurrency.test.ts (1)
5-20: ⚡ Quick winAdd a test for the max in-flight limit.
These cases prove ordering and rejection handling, but they never assert that only
concurrencytasks run at once. A regression could still reintroduce burst traffic while keeping both tests green.Suggested test shape
+ it('never exceeds the requested concurrency', async () => { + let inFlight = 0; + let maxInFlight = 0; + + await mapWithConcurrency([1, 2, 3, 4, 5], 2, async (n) => { + inFlight++; + maxInFlight = Math.max(maxInFlight, inFlight); + await new Promise((resolve) => setTimeout(resolve, 5)); + inFlight--; + return n; + }); + + expect(maxInFlight).toBeLessThanOrEqual(2); + });🤖 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 `@tests/lib/utils/mapWithConcurrency.test.ts` around lines 5 - 20, Add a new test in mapWithConcurrency.test.ts that verifies the max in-flight tasks never exceeds the concurrency limit: instrument mapWithConcurrency by creating async task factories that increment a shared inFlight counter on start and decrement on finish (and await a Promise that resolves after a short timeout), capture the peak inFlight value while running mapWithConcurrency with concurrency set (e.g., 2 or 3), and assert the peak inFlight <= concurrency; use the existing mapWithConcurrency symbol so the test explicitly fails if more than concurrency tasks run simultaneously.src/lib/queries/orderbook.ts (1)
117-121: ⚡ Quick winUse these constants for the local query defaults too.
This file still hardcodes
15_000and30_000in the query builders, so these exports can drift from the behavior they are supposed to document. ReusingGLOBAL_ORDERBOOK_POLL_MSandTOKEN_ORDERBOOK_POLL_MShere would keep the module self-consistent.🤖 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 `@src/lib/queries/orderbook.ts` around lines 117 - 121, The module exports GLOBAL_ORDERBOOK_POLL_MS and TOKEN_ORDERBOOK_POLL_MS but the query builders still use hardcoded 30_000 and 15_000; update the query-builder code in this file (the functions that construct the orderbook query options / defaults — e.g., the global orderbook and per-token orderbook query builders) to replace the literal 30_000 and 15_000 values with the exported constants GLOBAL_ORDERBOOK_POLL_MS and TOKEN_ORDERBOOK_POLL_MS respectively, and ensure both staleTime and refetchInterval (or similar local query default fields) reference those constants so the exports and query defaults stay in sync.
🤖 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 `@src/lib/api/orders.ts`:
- Around line 207-234: The current per-token fetch inside mapWithConcurrency
discards tokenQuotes if apiGetOrdersByToken throws on a later page; modify the
page loop in the async callback (the block that builds tokenQuotes, tokenSeen,
page, hasMore) to catch errors from apiGetOrdersByToken (and any per-page
processing) so that on a page-level failure you log/warn the error and break out
of the while loop and return the already-collected tokenQuotes instead of
letting the callback reject; keep using convertApiOrderToProcessedQuote,
tokenSeen, and hasMore logic unchanged and preserve the existing MAX_ORDER_PAGES
pagination cap behavior.
In `@src/lib/stores/deployTransactionStore.ts`:
- Around line 297-299: The current checks only call
invalidateOrderQueries(network.id, assetTokenInfo.address) when
assetTokenInfo?.address exists, causing sell-limit/DCA/DSF/folio flows to skip
invalidation; update each occurrence that references assetTokenInfo (the blocks
calling invalidateOrderQueries) to call invalidateOrderQueries(network.id,
assetTokenInfo.address) when address exists and otherwise call a global fallback
invalidateOrderQueries(network.id) so orders are always invalidated even when
assetTokenInfo is absent.
In `@src/routes/`(main)/trade/[id]/+page.svelte:
- Around line 1515-1517: The conditional rendering currently shows a permanent
LoadingSpinner when currentToken is falsy; instead check the subgraph query
error state by branching on $singleTokenQuery.isError (in addition to
currentToken) and render an error state/message (or an ErrorBanner component)
when isError is true so users see a failure instead of an infinite spinner;
update the three occurrences that use currentToken (around the LoadingSpinner at
the shown block and the similar blocks at lines ~1537 and ~1585) to first check
$singleTokenQuery.isError, then show the spinner only when not errored and still
loading.
- Around line 1263-1270: The vault filter currently compares vault.token address
to tradeTokenAddress directly, which misses legacy/subgraph aliases; update the
predicate in the allVaultData.map(...).filter(...) for raindexVault to use the
same alias-aware check as the rest of the page by constructing or using
assetAddressSet for tradeTokenAddress (e.g., const addrSet =
assetAddressSet(tradeTokenAddress) or equivalent) and replace the equality check
with addrSet.has(vaultTokenAddr) while keeping the hasBalance check via
vaultBalanceToBigInt(v) > 0n; reference tradeTokenAddress, allVaultData,
raindexVault, vaultBalanceToBigInt, and assetAddressSet to locate and change the
code.
- Around line 603-605: The page currently starts a single one-shot timer in
onMount that sets loadSecondaryQueries = true (using secondaryTimer and
SECONDARY_QUERIES_DELAY_MS), so when the /trade/[id] component is reused across
in-app navigations the deferred queries remain enabled; fix by resetting
loadSecondaryQueries to false and restarting (and clearing) the secondaryTimer
whenever the route token changes: listen for navigation or $page.params.id
changes (e.g., using afterNavigate or a reactive $: on $page.params.id), on each
change clearTimeout(secondaryTimer) (and clear in onDestroy), set
loadSecondaryQueries = false, then create a new timeout with
SECONDARY_QUERIES_DELAY_MS to set loadSecondaryQueries = true.
---
Nitpick comments:
In `@src/lib/queries/orderbook.ts`:
- Around line 117-121: The module exports GLOBAL_ORDERBOOK_POLL_MS and
TOKEN_ORDERBOOK_POLL_MS but the query builders still use hardcoded 30_000 and
15_000; update the query-builder code in this file (the functions that construct
the orderbook query options / defaults — e.g., the global orderbook and
per-token orderbook query builders) to replace the literal 30_000 and 15_000
values with the exported constants GLOBAL_ORDERBOOK_POLL_MS and
TOKEN_ORDERBOOK_POLL_MS respectively, and ensure both staleTime and
refetchInterval (or similar local query default fields) reference those
constants so the exports and query defaults stay in sync.
In `@tests/lib/utils/mapWithConcurrency.test.ts`:
- Around line 5-20: Add a new test in mapWithConcurrency.test.ts that verifies
the max in-flight tasks never exceeds the concurrency limit: instrument
mapWithConcurrency by creating async task factories that increment a shared
inFlight counter on start and decrement on finish (and await a Promise that
resolves after a short timeout), capture the peak inFlight value while running
mapWithConcurrency with concurrency set (e.g., 2 or 3), and assert the peak
inFlight <= concurrency; use the existing mapWithConcurrency symbol so the test
explicitly fails if more than concurrency tasks run simultaneously.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e670e25-9c2e-4c1a-99e4-0a7a60964009
📒 Files selected for processing (10)
src/lib/api/orders.tssrc/lib/components/QuickTrade.sveltesrc/lib/queries/oracleQuotes.tssrc/lib/queries/orderbook.tssrc/lib/queries/tradeActivity.tssrc/lib/stores/deployTransactionStore.tssrc/lib/utils/mapWithConcurrency.tssrc/routes/(main)/dashboard/+page.sveltesrc/routes/(main)/trade/[id]/+page.sveltetests/lib/utils/mapWithConcurrency.test.ts
Comment on lines
+207
to
+234
| const results = await mapWithConcurrency(stockTokens, TOKEN_ORDER_FETCH_CONCURRENCY, async (token) => { | ||
| const tokenQuotes: ProcessedQuote[] = []; | ||
| const tokenSeen = new Set<string>(); | ||
| let page = 1; | ||
| let hasMore = true; | ||
| while (hasMore && page <= MAX_ORDER_PAGES) { | ||
| const response = await apiGetOrdersByToken(token.address, { page, pageSize: 50 }); | ||
| for (const order of response.orders) { | ||
| if (tokenSeen.has(order.orderHash)) continue; | ||
| tokenSeen.add(order.orderHash); | ||
| const quote = convertApiOrderToProcessedQuote( | ||
| order, | ||
| paymentToken.address, | ||
| allTokens, | ||
| networkId | ||
| ); | ||
| if (quote) tokenQuotes.push(quote); | ||
| } | ||
| }) | ||
| ); | ||
| hasMore = response.pagination.hasMore; | ||
| page++; | ||
| } | ||
| if (hasMore) { | ||
| console.warn( | ||
| `[orders] Hit pagination cap (${MAX_ORDER_PAGES} pages) for token ${token.address}` | ||
| ); | ||
| } | ||
| return tokenQuotes; | ||
| }); |
There was a problem hiding this comment.
Keep partial quotes when a later page fetch fails.
If apiGetOrdersByToken() fails on page 2+, this callback rejects and the already collected quotes for that token are discarded. The single-token path below keeps partial data on page failures, so the global path now regresses to an all-or-nothing fetch per token.
Suggested fix
const results = await mapWithConcurrency(stockTokens, TOKEN_ORDER_FETCH_CONCURRENCY, async (token) => {
const tokenQuotes: ProcessedQuote[] = [];
const tokenSeen = new Set<string>();
let page = 1;
let hasMore = true;
while (hasMore && page <= MAX_ORDER_PAGES) {
- const response = await apiGetOrdersByToken(token.address, { page, pageSize: 50 });
+ let response;
+ try {
+ response = await apiGetOrdersByToken(token.address, { page, pageSize: 50 });
+ } catch (error) {
+ console.warn(`[orders] Page ${page} fetch failed for token ${token.address}:`, error);
+ break;
+ }
for (const order of response.orders) {
if (tokenSeen.has(order.orderHash)) continue;
tokenSeen.add(order.orderHash);
const quote = convertApiOrderToProcessedQuote(
order,🤖 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 `@src/lib/api/orders.ts` around lines 207 - 234, The current per-token fetch
inside mapWithConcurrency discards tokenQuotes if apiGetOrdersByToken throws on
a later page; modify the page loop in the async callback (the block that builds
tokenQuotes, tokenSeen, page, hasMore) to catch errors from apiGetOrdersByToken
(and any per-page processing) so that on a page-level failure you log/warn the
error and break out of the while loop and return the already-collected
tokenQuotes instead of letting the callback reject; keep using
convertApiOrderToProcessedQuote, tokenSeen, and hasMore logic unchanged and
preserve the existing MAX_ORDER_PAGES pagination cap behavior.
Comment on lines
+297
to
+299
| if (assetTokenInfo?.address) { | ||
| invalidateOrderQueries(network.id, assetTokenInfo.address); | ||
| } |
There was a problem hiding this comment.
Keep a global invalidation fallback when assetTokenInfo is absent.
assetTokenInfo is only set for the buy-side deploy flows in this file. Sell limit/DCA deploys, plus DSF/folio deploys, now skip orderbook invalidation completely, so their new orders will not show up until polling catches up.
Suggested fix
- if (assetTokenInfo?.address) {
- invalidateOrderQueries(network.id, assetTokenInfo.address);
- }
+ if (assetTokenInfo?.address) {
+ invalidateOrderQueries(network.id, assetTokenInfo.address);
+ } else {
+ invalidateOrderQueries(network.id);
+ }Also applies to: 319-321, 330-332
🤖 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 `@src/lib/stores/deployTransactionStore.ts` around lines 297 - 299, The current
checks only call invalidateOrderQueries(network.id, assetTokenInfo.address) when
assetTokenInfo?.address exists, causing sell-limit/DCA/DSF/folio flows to skip
invalidation; update each occurrence that references assetTokenInfo (the blocks
calling invalidateOrderQueries) to call invalidateOrderQueries(network.id,
assetTokenInfo.address) when address exists and otherwise call a global fallback
invalidateOrderQueries(network.id) so orders are always invalidated even when
assetTokenInfo is absent.
Comment on lines
+603
to
+605
| const secondaryTimer = setTimeout(() => { | ||
| loadSecondaryQueries = true; | ||
| }, SECONDARY_QUERIES_DELAY_MS); |
There was a problem hiding this comment.
Reset the secondary-query delay on token navigation.
This timer only runs in onMount, so a reused /trade/[id] page instance keeps loadSecondaryQueries = true after the first visit. Subsequent in-app token navigations will fire the “deferred” queries immediately, which defeats the burst-reduction change.
Suggested fix
- let loadSecondaryQueries = false;
+ let loadSecondaryQueries = false;
+ let secondaryDelayFor: string | null = null;
+ let secondaryTimer: ReturnType<typeof setTimeout> | null = null;
...
- onMount(() => {
+ onMount(() => {
cleanupScrollTracking = initScrollTracking('trade_page');
-
- const secondaryTimer = setTimeout(() => {
- loadSecondaryQueries = true;
- }, SECONDARY_QUERIES_DELAY_MS);
return () => {
- clearTimeout(secondaryTimer);
+ if (secondaryTimer) clearTimeout(secondaryTimer);
if (cleanupScrollTracking) {
cleanupScrollTracking();
}
};
});
+
+ $: if (browser && tradeTokenAddress && secondaryDelayFor !== tradeTokenAddress) {
+ secondaryDelayFor = tradeTokenAddress;
+ loadSecondaryQueries = false;
+ if (secondaryTimer) clearTimeout(secondaryTimer);
+ secondaryTimer = setTimeout(() => {
+ loadSecondaryQueries = true;
+ }, SECONDARY_QUERIES_DELAY_MS);
+ }🤖 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 `@src/routes/`(main)/trade/[id]/+page.svelte around lines 603 - 605, The page
currently starts a single one-shot timer in onMount that sets
loadSecondaryQueries = true (using secondaryTimer and
SECONDARY_QUERIES_DELAY_MS), so when the /trade/[id] component is reused across
in-app navigations the deferred queries remain enabled; fix by resetting
loadSecondaryQueries to false and restarting (and clearing) the secondaryTimer
whenever the route token changes: listen for navigation or $page.params.id
changes (e.g., using afterNavigate or a reactive $: on $page.params.id), on each
change clearTimeout(secondaryTimer) (and clear in onDestroy), set
loadSecondaryQueries = false, then create a new timeout with
SECONDARY_QUERIES_DELAY_MS to set loadSecondaryQueries = true.
Comment on lines
+1263
to
1270
| {@const vaults = tradeTokenAddress | ||
| ? allVaultData | ||
| .map((vd) => vd.raindexVault) | ||
| .filter((v) => { | ||
| const vaultTokenAddr = (v.token?.address ?? v.token?.id)?.toLowerCase(); | ||
| const isCorrectToken = | ||
| vaultTokenAddr === currentToken.address.toLowerCase(); | ||
| vaultTokenAddr === tradeTokenAddress.toLowerCase(); | ||
| const hasBalance = vaultBalanceToBigInt(v) > 0n; |
There was a problem hiding this comment.
Match vaults with the same address set used everywhere else on this page.
This exact tradeTokenAddress comparison will miss vault balances when the vault token resolves to the legacy or subgraph address variant. The rest of the page already uses assetAddressSet to handle those aliases, so this tab can silently show “No position found” for tokens the user actually holds.
Suggested fix
{`@const` vaults = tradeTokenAddress
? allVaultData
.map((vd) => vd.raindexVault)
.filter((v) => {
const vaultTokenAddr = (v.token?.address ?? v.token?.id)?.toLowerCase();
- const isCorrectToken =
- vaultTokenAddr === tradeTokenAddress.toLowerCase();
+ const isCorrectToken = vaultTokenAddr
+ ? assetAddressSet.has(vaultTokenAddr)
+ : false;
const hasBalance = vaultBalanceToBigInt(v) > 0n;
return isCorrectToken && hasBalance;
})
: []}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {@const vaults = tradeTokenAddress | |
| ? allVaultData | |
| .map((vd) => vd.raindexVault) | |
| .filter((v) => { | |
| const vaultTokenAddr = (v.token?.address ?? v.token?.id)?.toLowerCase(); | |
| const isCorrectToken = | |
| vaultTokenAddr === currentToken.address.toLowerCase(); | |
| vaultTokenAddr === tradeTokenAddress.toLowerCase(); | |
| const hasBalance = vaultBalanceToBigInt(v) > 0n; | |
| {`@const` vaults = tradeTokenAddress | |
| ? allVaultData | |
| .map((vd) => vd.raindexVault) | |
| .filter((v) => { | |
| const vaultTokenAddr = (v.token?.address ?? v.token?.id)?.toLowerCase(); | |
| const isCorrectToken = vaultTokenAddr | |
| ? assetAddressSet.has(vaultTokenAddr) | |
| : false; | |
| const hasBalance = vaultBalanceToBigInt(v) > 0n; |
🤖 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 `@src/routes/`(main)/trade/[id]/+page.svelte around lines 1263 - 1270, The
vault filter currently compares vault.token address to tradeTokenAddress
directly, which misses legacy/subgraph aliases; update the predicate in the
allVaultData.map(...).filter(...) for raindexVault to use the same alias-aware
check as the rest of the page by constructing or using assetAddressSet for
tradeTokenAddress (e.g., const addrSet = assetAddressSet(tradeTokenAddress) or
equivalent) and replace the equality check with addrSet.has(vaultTokenAddr)
while keeping the hasBalance check via vaultBalanceToBigInt(v) > 0n; reference
tradeTokenAddress, allVaultData, raindexVault, vaultBalanceToBigInt, and
assetAddressSet to locate and change the code.
Comment on lines
+1515
to
+1517
| {#if !currentToken} | ||
| <LoadingSpinner variant="inline" size="md" text="Loading on-chain supply data…" /> | ||
| {:else} |
There was a problem hiding this comment.
Don’t render a permanent loading spinner when singleTokenQuery fails.
If the config token exists but the subgraph request errors, currentToken stays falsy and these tabs remain stuck on “Loading…”. Please branch on $singleTokenQuery.isError here so users get an error state instead of an infinite spinner.
Also applies to: 1537-1539, 1585-1587
🤖 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 `@src/routes/`(main)/trade/[id]/+page.svelte around lines 1515 - 1517, The
conditional rendering currently shows a permanent LoadingSpinner when
currentToken is falsy; instead check the subgraph query error state by branching
on $singleTokenQuery.isError (in addition to currentToken) and render an error
state/message (or an ErrorBanner component) when isError is true so users see a
failure instead of an infinite spinner; update the three occurrences that use
currentToken (around the LoadingSpinner at the shown block and the similar
blocks at lines ~1537 and ~1585) to first check $singleTokenQuery.isError, then
show the spinner only when not errored and still loading.
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.
Summary
Addresses backend load from burst
GET /api/st0x/v1/orders/token/*traffic (complements server-side caching).Request shaping
mapWithConcurrency— global orderbook loads at most 4orders/tokenrequests in parallel (was ~14 at once) viafetchAndQuotePaymentTokenOrders.prefetchGlobalOrders()on mount (was triggering a full multi-token sweep).GLOBAL_ORDERBOOK_POLL_MS).invalidateOrderQueries(networkId, assetAddress)only (no full-network refetch).Trade page
tokens.ts(currentPythToken) immediately; subgraph only for supply/mints/burns tabs.createTokenOrderbookQuotesQuery(no reassignment loop);refetchOnMount: true(respects stale cache).Test plan
/dashboard: at most ~4 concurrentorders/tokenrequests, then rest; repeats ~every 30s not 15s./: no wall oforders/tokenon load (Quick Trade uses selected token only)./trade/<addr>: page paints without waiting on subgraph; one primaryorders/tokenflow; secondary calls after ~1.5s.npm run checknpm test -- tests/lib/utils/mapWithConcurrency.test.ts --runMade with Cursor
Summary by CodeRabbit
Bug Fixes
New Features
Performance