fix(open-interest): return OI history in ascending order for charts#171
fix(open-interest): return OI history in ascending order for charts#1716figpsolseeker wants to merge 1 commit intodcccrypto:mainfrom
Conversation
The /open-interest/:slab endpoint returned oi_history rows in descending order (newest first). Chart consumers using lightweight-charts require strictly ascending timestamps for setData() — the same issue that was fixed in prices.ts (where DESC caused aggregateCandles to produce DESC-ordered candles and lightweight-charts silently failed). Switch to ascending: true so the history array can be fed directly into the chart component without a client-side reversal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughA Supabase query for historical open interest records in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
src/routes/open-interest.ts (1)
83-83: ReplaceGH#xxxwith a real issue/PR reference.Line 83 still has a placeholder, which makes traceability brittle.
Proposed comment cleanup
- // (newest first) — same issue that was fixed in prices.ts (see GH#xxx). + // (newest first) — same class of issue previously fixed in prices.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/open-interest.ts` at line 83, Replace the placeholder "GH#xxx" in the comment inside open-interest.ts with the real GitHub issue or PR reference (e.g., "GH#1234" or full URL) to restore traceability; search for the placeholder string "GH#xxx" in open-interest.ts (near the comment referencing prices.ts) and update it to the actual issue/PR ID used when the same fix was applied in prices.ts, then run a quick grep to ensure no other files still contain the placeholder and include a brief commit message like "chore: replace GH#xxx with real PR reference".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/open-interest.ts`:
- Around line 88-89: The query currently uses .order("timestamp", { ascending:
true }).limit(100) which returns the oldest 100 rows; change the order to
.order("timestamp", { ascending: false }).limit(100) to fetch the latest 100
rows, then reverse the returned array client-side before passing it to the chart
so the data is in ascending timestamp order; update the code around the query
call (the chain containing .order("timestamp", { ascending: ... }).limit(100))
and ensure any variable that holds the fetched rows is reversed (e.g.,
rows.reverse()) before rendering.
---
Nitpick comments:
In `@src/routes/open-interest.ts`:
- Line 83: Replace the placeholder "GH#xxx" in the comment inside
open-interest.ts with the real GitHub issue or PR reference (e.g., "GH#1234" or
full URL) to restore traceability; search for the placeholder string "GH#xxx" in
open-interest.ts (near the comment referencing prices.ts) and update it to the
actual issue/PR ID used when the same fix was applied in prices.ts, then run a
quick grep to ensure no other files still contain the placeholder and include a
brief commit message like "chore: replace GH#xxx with real PR reference".
🪄 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: fc8537ad-442f-41f8-98d7-f82b07c7196c
📒 Files selected for processing (1)
src/routes/open-interest.ts
| .order("timestamp", { ascending: true }) | ||
| .limit(100); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PostgREST/Supabase, does order("timestamp", { ascending: true }).limit(100) return the oldest 100 rows? What is the recommended pattern to return the latest 100 rows in ascending timestamp order?
💡 Result:
Yes. In PostgREST (and therefore Supabase), order('timestamp', { ascending: true }).limit(100) is equivalent to ORDER BY timestamp ASC LIMIT 100, so it returns the oldest 100 rows by timestamp (smallest/earliest timestamps first). (postgrest.org)
Recommended pattern to get “latest 100, but displayed oldest→newest”
Because you can’t apply two different sort orders (DESC to pick the last 100, then ASC to present them) in a single flat PostgREST query, the common patterns are:
A) Fetch latest 100 in descending order, then reverse client-side
const { data } = await supabase
.from('events')
.select('*')
.order('timestamp', { ascending: false })
.order('id', { ascending: false }) // tie-breaker for identical timestamps
.limit(100)
const latest100Ascending = (data ?? []).slice().reverse()PostgREST supports ordering by multiple columns (comma-separated), and Supabase’s .order() can be called multiple times—use a unique tie-breaker like id for deterministic results when timestamps collide. (postgrest.org)
B) Do it server-side via a view (or an rpc function)
create view latest_events_100 as
select *
from (
select *
from events
order by "timestamp" desc, id desc
limit 100
) t
order by "timestamp" asc, id asc;Then query latest_events_100 normally (already in ascending order). This “subquery then reorder” approach is generally required when you need “last N rows, but returned ascending.” (stackoverflow.com)
Citations:
- 1: https://postgrest.org/en/v10/api.html
- 2: https://postgrest.org/en/v10/api.html
- 3: https://stackoverflow.com/questions/67408293/get-last-n-rows-ordered-ascending-without-using-nested-selects?utm_source=openai
ascending: true with limit(100) returns the oldest 100 records, not the latest.
This changes the data window semantics. If the table has >100 rows, the query serves the oldest records instead of the most recent data. Fetch with ascending: false and then reverse client-side to get the latest 100 in ascending order for the chart.
Proposed fix
- .order("timestamp", { ascending: true })
+ .order("timestamp", { ascending: false })
.limit(100);
@@
const filteredHistory = (history ?? [])
.filter((h) => !isPhantomOiRecord(h))
+ .reverse()
.map((h) => ({
timestamp: h.timestamp,
totalOi: h.total_oi,
netLpPos: h.net_lp_pos,
}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/open-interest.ts` around lines 88 - 89, The query currently uses
.order("timestamp", { ascending: true }).limit(100) which returns the oldest 100
rows; change the order to .order("timestamp", { ascending: false }).limit(100)
to fetch the latest 100 rows, then reverse the returned array client-side before
passing it to the chart so the data is in ascending timestamp order; update the
code around the query call (the chain containing .order("timestamp", {
ascending: ... }).limit(100)) and ensure any variable that holds the fetched
rows is reversed (e.g., rows.reverse()) before rendering.
1 participant
Summary
/open-interest/:slabendpoint returnedoi_historyrows in descending order (newest first). Chart consumers usinglightweight-chartsrequire strictly ascending timestamps forsetData().src/routes/prices.ts(see comment at lines 30-37), where DESC ordering causedaggregateCandlesto produce DESC-ordered candles andlightweight-chartssilently failed..order("timestamp", { ascending: false })→{ ascending: true }so the history array can be fed directly into the chart component without a client-side reversal.Test plan
npx vitest run tests/routes/open-interest.test.ts— 16/16 passingtests/routes/prices.test.tsissue onmain, unrelated)npx tsc --noEmit— cleanSummary by CodeRabbit