Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds an experimental Admin Stats V2: a password‑gated client dashboard, server cookie auth and an authenticated proxy to a Monarch Indexer, client GraphQL fetcher and paginated transaction fetchers, a hook to enrich transactions, multiple chart/table UI components, and extended chart palette colors. Changes
Sequence DiagramsequenceDiagram
participant User
participant Page as StatsV2 Page
participant Gate as PasswordGate
participant Store as useAdminAuth
participant AuthAPI as /api/admin/auth
participant Hook as useMonarchTransactions
participant Proxy as /api/admin/monarch-indexer
participant Indexer as Monarch Indexer
User->>Page: Navigate to /admin/stats-v2
Page->>Gate: Render children via PasswordGate
Gate->>Store: checkAuth()
Store->>AuthAPI: GET /api/admin/auth (credentials)
AuthAPI-->>Store: { authenticated: true/false }
alt not authenticated
User->>Gate: Submit password
Gate->>Store: authenticate(password)
Store->>AuthAPI: POST /api/admin/auth
AuthAPI-->>Store: set cookie / success
end
Gate-->>Page: allow render
Page->>Hook: useMonarchTransactions(timeframe)
Hook->>Proxy: POST GraphQL (includes cookie)
Proxy->>AuthAPI: validate cookie (server-side)
Proxy->>Indexer: forward GraphQL request
Indexer-->>Proxy: response JSON
Proxy-->>Hook: proxy response
Hook-->>Page: enriched transactions, chainStats, dailyVolumes, totals
Page->>User: render cards, charts, table
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@app/api/admin/auth/route.ts`:
- Around line 20-28: The current hashPassword function uses a weak 32-bit custom
hash; replace it with a secure password hashing approach (preferably
bcrypt/scrypt, otherwise at minimum SHA-256 via crypto.subtle.digest) by
updating the hashPassword function to perform a cryptographic hash and return a
hex string (note this will make hashPassword async if using
crypto.subtle.digest); update any callers of hashPassword and the stored
ADMIN_V2_PASSWORD_HASH to the new format, and ensure authentication compares the
hex-encoded secure hash (or use a bcrypt verify call) instead of the old
djb2-style output.
- Around line 49-57: The cookie is currently storing the raw password hash via
cookieStore.set(COOKIE_NAME, hash, ...) which lets anyone with
ADMIN_V2_PASSWORD_HASH forge sessions; instead stop storing the hash directly:
generate a server-side session token (secure random ID or signed value) and
store an association to the authenticated user/server-side expiry (in memory,
DB, or using a signed JWT/HMAC) and set cookieStore.set(COOKIE_NAME,
sessionToken, ...) where sessionToken is either a random UUID mapped to a server
session record or a cryptographically signed value using a separate secret
(e.g., SESSION_SECRET); update authentication checks to validate the token
against the session store or verify the signature instead of comparing cookie to
ADMIN_V2_PASSWORD_HASH.
In `@src/data-sources/monarch-indexer/transactions.ts`:
- Around line 143-148: The loop stops early because it checks "if
(supplies.length < limit && withdraws.length < limit) break;" and uses a single
offset variable, so when one collection returns fewer than limit rows the loop
breaks and remaining rows in the other collection are lost; change to continue
until both are exhausted by either (A) introducing two offsets (e.g.,
suppliesOffset and withdrawsOffset) and incrementing each only when that fetch
returned >= limit, updating the fetch calls that use offset to use the
corresponding offset variables, or (B) keep a single loop but only break when
both supplies and withdraws returned < limit and still increment offsets
independently (increment suppliesOffset when supplies.length === limit,
withdrawsOffset when withdraws.length === limit) and replace references to
offset with the correct per-collection offset variables (supplies, withdraws,
limit, offset -> suppliesOffset/withdrawsOffset).
In `@src/features/admin-v2/components/chain-volume-chart.tsx`:
- Around line 75-79: The current useMemo named visibleChainIds filters out
hidden chains (chainIds.filter(...)) which unmounts their Area components and
removes legend entries; instead, stop filtering and render all chains (use
chainIds directly or replace visibleChainIds with chainIds), and pass
hide={!visibleChains[chainId]} to each Area (and any similar graphical
components) so the Area is visually hidden but the legend entry remains
clickable; update any references to visibleChainIds to use chainIds and ensure
Area components (and related keys like the chain id variable) receive the hide
prop.
In `@src/features/admin-v2/components/stats-transactions-table.tsx`:
- Around line 66-72: The table's currentPage state in StatsTransactionsTable can
become invalid when the transactions prop changes; add a useEffect that watches
the transactions prop and resets setCurrentPage(1) (or clamps it to a valid
page) whenever transactions changes to ensure the UI shows a valid page; update
the effect to reference the existing currentPage state and the entriesPerPage
constant so you either setCurrentPage(1) or compute Math.min(currentPage,
Math.ceil(transactions.length / entriesPerPage)) to clamp the page.
In `@src/features/admin-v2/components/stats-volume-chart.tsx`:
- Around line 169-174: The label "Transactions" in the Period Summary is
misleading because it displays dailyVolumes.length (number of days); either
change the label to "Days" or display a real transaction count: locate the JSX
in stats-volume-chart.tsx where dailyVolumes is used and replace the label or
compute total transactions (e.g. const totalTransactions =
dailyVolumes.reduce((sum, day) => sum + (day.transactions ?? 0), 0)) and render
totalTransactions instead of dailyVolumes.length so the label and value match.
In `@src/stores/useAdminAuth.ts`:
- Around line 44-53: The fetch logic in the auth check (the try block that calls
fetch('/api/admin/auth')) must verify response.ok before calling response.json()
to avoid throwing on non-JSON or error responses; update the code around the
fetch/response handling in useAdminAuth (the block that defines response and
calls response.json()) to check response.ok (or response.status) first, handle
non-ok by setting isAuthenticated: false and isCheckingAuth: false (and
optionally attempt to read response.text() safely for logging), and only call
response.json() when the response is ok, keeping the catch to handle network or
parsing errors.
- Around line 59-72: The code currently calls response.json() before checking
response.ok which can throw on non-JSON error pages; in useAdminAuth (the async
block that calls fetch('/api/admin/auth')), first check response.ok, and only
call await response.json() for the success path; for the error path, call await
response.text() (or try/catch a JSON parse) to extract a usable error message
and then call set({ isLoading: false, error: ... }) and return false; ensure set
and the fetch call sites are updated so non-JSON responses do not cause an
unhandled exception.
🧹 Nitpick comments (6)
app/api/admin/monarch-indexer/route.ts (1)
32-55: Add a timeout to prevent the proxy fetch from hanging indefinitely.The current code lacks abort protection; if the indexer stalls, the route will hang. A 10-second timeout returning 504 is sensible.
For Node.js 18+ (App Router default), use the simpler approach:
Recommended: AbortSignal.timeout()
try { const body = await request.json(); const response = await fetch(INDEXER_ENDPOINT, { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify(body), + signal: AbortSignal.timeout(10_000), }); if (!response.ok) { return NextResponse.json( { error: `Indexer request failed: ${response.status}` }, { status: response.status }, ); } const data = await response.json(); return NextResponse.json(data); } catch (error) { + if ((error as any)?.name === 'TimeoutError') { + return NextResponse.json({ error: 'Indexer request timed out' }, { status: 504 }); + } console.error('Monarch indexer proxy error:', error); return NextResponse.json({ error: 'Internal server error' }, { status: 500 }); }Alternatively, if you need manual abort control, use
AbortControllerwithtry/finallyto ensure cleanup:Alternative: AbortController + clearTimeout
try { const body = await request.json(); const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 10_000); const response = await fetch(INDEXER_ENDPOINT, { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify(body), signal: controller.signal, + }); + clearTimeout(timeout); if (!response.ok) { return NextResponse.json( { error: `Indexer request failed: ${response.status}` }, { status: response.status }, ); } const data = await response.json(); return NextResponse.json(data); } catch (error) { + if ((error as any)?.name === 'AbortError') { + return NextResponse.json({ error: 'Indexer request timed out' }, { status: 504 }); + } console.error('Monarch indexer proxy error:', error); return NextResponse.json({ error: 'Internal server error' }, { status: 500 }); }Also consider adding
export const maxDuration = 30to prevent the route itself from being killed by the platform before your fetch timeout triggers.app/admin/stats-v2/page.tsx (2)
116-128:isLoadingprop is alwaysfalsehere.These components are rendered inside the
elsebranch ofisLoading ? ... : (...), soisLoadingis guaranteed to befalse. The prop is misleading and adds no value.Proposed fix
<StatsVolumeChart dailyVolumes={dailyVolumes} totalSupplyVolumeUsd={totalSupplyVolumeUsd} totalWithdrawVolumeUsd={totalWithdrawVolumeUsd} - isLoading={isLoading} /> - <ChainVolumeChart dailyVolumes={dailyVolumes} chainStats={chainStats} isLoading={isLoading} /> + <ChainVolumeChart dailyVolumes={dailyVolumes} chainStats={chainStats} /> </div> - <StatsTransactionsTable transactions={transactions} isLoading={isLoading} /> + <StatsTransactionsTable transactions={transactions} />
48-54: Move constant outside component.
timeframeOptionsdoesn't depend on props or state. Define it outsideStatsV2Contentto avoid recreating on every render.app/api/admin/auth/route.ts (1)
71-80: GET handler returns auth status without rate limiting.An attacker could probe authentication status rapidly. Not critical for an internal admin page, but worth noting for future hardening.
src/hooks/useMonarchTransactions.ts (2)
187-190: Stablecoin heuristic could false-match non-stablecoin tokens.
symbol.includes('USD')matches tokens like 'USDOGE'. Consider an explicit set of known stablecoins.Suggested approach
+const STABLECOIN_SYMBOLS = new Set(['USDC', 'USDT', 'DAI', 'FRAX', 'LUSD', 'GHO', 'crvUSD', 'PYUSD']); + // In getUsdValue: -if (symbol.includes('USD') || symbol.includes('DAI') || symbol.includes('USDT') || symbol.includes('USDC')) { +if (STABLECOIN_SYMBOLS.has(symbol)) { return formatted; }
134-151: ETH/BTC price lookup returns first match which may vary.wstETH trades at a premium over WETH. First match could be inconsistent. Consider prioritizing WETH/WBTC specifically.
Suggested approach
const ethPrice = useMemo(() => { + // Prefer canonical WETH first + for (const [key, price] of tokenPrices) { + const address = key.split('-')[0]; + const market = allMarkets.find((m) => m.loanAsset.address.toLowerCase() === address && m.loanAsset.symbol === 'WETH'); + if (market) return price; + } + // Fallback to any ETH-pegged for (const [key, price] of tokenPrices) { // ... existing code } return 0; }, [tokenPrices, allMarkets]);
| // Set httpOnly cookie - client can't read this via JS | ||
| const cookieStore = await cookies(); | ||
| cookieStore.set(COOKIE_NAME, hash, { | ||
| httpOnly: true, | ||
| secure: process.env.NODE_ENV === 'production', | ||
| sameSite: 'strict', | ||
| path: '/api/admin', | ||
| maxAge: 60 * 60 * 24 * 7, // 7 days | ||
| }); |
There was a problem hiding this comment.
Cookie stores the password hash directly.
If an attacker obtains ADMIN_V2_PASSWORD_HASH (e.g., from logs, env leak), they can forge valid session cookies. Consider using a separate session token or signing the cookie value with an additional secret.
🤖 Prompt for AI Agents
In `@app/api/admin/auth/route.ts` around lines 49 - 57, The cookie is currently
storing the raw password hash via cookieStore.set(COOKIE_NAME, hash, ...) which
lets anyone with ADMIN_V2_PASSWORD_HASH forge sessions; instead stop storing the
hash directly: generate a server-side session token (secure random ID or signed
value) and store an association to the authenticated user/server-side expiry (in
memory, DB, or using a signed JWT/HMAC) and set cookieStore.set(COOKIE_NAME,
sessionToken, ...) where sessionToken is either a random UUID mapped to a server
session record or a cryptographically signed value using a separate secret
(e.g., SESSION_SECRET); update authentication checks to validate the token
against the session store or verify the signature instead of comparing cookie to
ADMIN_V2_PASSWORD_HASH.
| export function StatsTransactionsTable({ transactions, isLoading }: StatsTransactionsTableProps) { | ||
| const [sortKey, setSortKey] = useState<SortKey>('timestamp'); | ||
| const [sortDirection, setSortDirection] = useState<SortDirection>('desc'); | ||
| const [currentPage, setCurrentPage] = useState(1); | ||
| const [selectedChains, setSelectedChains] = useState<number[]>([]); | ||
| const [selectedTypes, setSelectedTypes] = useState<('supply' | 'withdraw')[]>([]); | ||
| const entriesPerPage = 15; |
There was a problem hiding this comment.
Page doesn't reset when transaction data changes.
If the user is on page 5 and data refreshes with fewer transactions, they see an empty page. Reset currentPage when transactions changes.
Proposed fix
+import { useState, useMemo, useEffect } from 'react';
...
export function StatsTransactionsTable({ transactions, isLoading }: StatsTransactionsTableProps) {
const [sortKey, setSortKey] = useState<SortKey>('timestamp');
const [sortDirection, setSortDirection] = useState<SortDirection>('desc');
const [currentPage, setCurrentPage] = useState(1);
const [selectedChains, setSelectedChains] = useState<number[]>([]);
const [selectedTypes, setSelectedTypes] = useState<('supply' | 'withdraw')[]>([]);
+
+ // Reset to page 1 when data changes
+ useEffect(() => {
+ setCurrentPage(1);
+ }, [transactions]);🤖 Prompt for AI Agents
In `@src/features/admin-v2/components/stats-transactions-table.tsx` around lines
66 - 72, The table's currentPage state in StatsTransactionsTable can become
invalid when the transactions prop changes; add a useEffect that watches the
transactions prop and resets setCurrentPage(1) (or clamps it to a valid page)
whenever transactions changes to ensure the UI shows a valid page; update the
effect to reference the existing currentPage state and the entriesPerPage
constant so you either setCurrentPage(1) or compute Math.min(currentPage,
Math.ceil(transactions.length / entriesPerPage)) to clamp the page.
| <h4 className="mb-3 text-xs uppercase tracking-wider text-secondary">Period Summary</h4> | ||
| <div className="flex flex-wrap gap-x-8 gap-y-2"> | ||
| <div className="flex items-center gap-2"> | ||
| <span className="text-sm text-secondary">Transactions</span> | ||
| <span className="tabular-nums text-sm">{dailyVolumes.length} days</span> | ||
| </div> |
There was a problem hiding this comment.
Label says “Transactions” but value is days.
Either change the label or show an actual transaction count to avoid confusion.
Proposed tweak
- <span className="text-sm text-secondary">Transactions</span>
- <span className="tabular-nums text-sm">{dailyVolumes.length} days</span>
+ <span className="text-sm text-secondary">Days</span>
+ <span className="tabular-nums text-sm">{dailyVolumes.length} days</span>📝 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.
| <h4 className="mb-3 text-xs uppercase tracking-wider text-secondary">Period Summary</h4> | |
| <div className="flex flex-wrap gap-x-8 gap-y-2"> | |
| <div className="flex items-center gap-2"> | |
| <span className="text-sm text-secondary">Transactions</span> | |
| <span className="tabular-nums text-sm">{dailyVolumes.length} days</span> | |
| </div> | |
| <h4 className="mb-3 text-xs uppercase tracking-wider text-secondary">Period Summary</h4> | |
| <div className="flex flex-wrap gap-x-8 gap-y-2"> | |
| <div className="flex items-center gap-2"> | |
| <span className="text-sm text-secondary">Days</span> | |
| <span className="tabular-nums text-sm">{dailyVolumes.length} days</span> | |
| </div> |
🤖 Prompt for AI Agents
In `@src/features/admin-v2/components/stats-volume-chart.tsx` around lines 169 -
174, The label "Transactions" in the Period Summary is misleading because it
displays dailyVolumes.length (number of days); either change the label to "Days"
or display a real transaction count: locate the JSX in stats-volume-chart.tsx
where dailyVolumes is used and replace the label or compute total transactions
(e.g. const totalTransactions = dailyVolumes.reduce((sum, day) => sum +
(day.transactions ?? 0), 0)) and render totalTransactions instead of
dailyVolumes.length so the label and value match.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/api/admin/monarch-indexer/route.ts`:
- Around line 33-34: Wrap each call to request.json() (e.g., the const body =
await request.json() in the route handler and the other request.json() at lines
49-51) in a try/catch and, on a SyntaxError or any JSON parse error, return an
HTTP 400 response indicating "Invalid JSON body" (include error.message for
debugging if desired); ensure the handler returns a 400 Response instead of
letting the exception bubble to a 500, and keep existing logic for valid bodies
unchanged.
- Around line 35-41: The fetch call to INDEXER_ENDPOINT is unbounded and needs a
timeout: create an AbortController, pass controller.signal into the existing
fetch options (alongside method/headers/body), set a timeout (e.g. TIMEOUT_MS
constant) that calls controller.abort() after the delay, clear that timer
immediately after the fetch completes, and handle aborted requests (detect
AbortError) so the route returns an appropriate error response; update the code
around the response = await fetch(INDEXER_ENDPOINT, {...}) and reference
INDEXER_ENDPOINT, response, and body when adding the controller and timeout
logic.
- Around line 24-26: The current auth check lumps a missing EXPECTED_HASH with
an unauthorized session; change the logic in the route handler to first detect a
missing EXPECTED_HASH and return a 500 error (e.g., NextResponse.json({ error:
'Server misconfiguration' }, { status: 500 })), and only after confirming
EXPECTED_HASH exists perform the session check (session?.value && session.value
=== EXPECTED_HASH) and return 401 when the session value is present but does not
match. Update the conditional around EXPECTED_HASH and session in route.ts so
EXPECTED_HASH is validated separately from the authentication comparison.
| const response = await fetch(INDEXER_ENDPOINT, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(body), | ||
| }); |
There was a problem hiding this comment.
Add a timeout to the upstream fetch.
Unbounded external calls can hang a request thread and pile up.
Proposed fix
- const response = await fetch(INDEXER_ENDPOINT, {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- },
- body: JSON.stringify(body),
- });
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+ let response: Response;
+ try {
+ response = await fetch(INDEXER_ENDPOINT, {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ body: JSON.stringify(body),
+ signal: controller.signal,
+ });
+ } finally {
+ clearTimeout(timeout);
+ }📝 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.
| const response = await fetch(INDEXER_ENDPOINT, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify(body), | |
| }); | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 10_000); | |
| let response: Response; | |
| try { | |
| response = await fetch(INDEXER_ENDPOINT, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify(body), | |
| signal: controller.signal, | |
| }); | |
| } finally { | |
| clearTimeout(timeout); | |
| } |
🤖 Prompt for AI Agents
In `@app/api/admin/monarch-indexer/route.ts` around lines 35 - 41, The fetch call
to INDEXER_ENDPOINT is unbounded and needs a timeout: create an AbortController,
pass controller.signal into the existing fetch options (alongside
method/headers/body), set a timeout (e.g. TIMEOUT_MS constant) that calls
controller.abort() after the delay, clear that timer immediately after the fetch
completes, and handle aborted requests (detect AbortError) so the route returns
an appropriate error response; update the code around the response = await
fetch(INDEXER_ENDPOINT, {...}) and reference INDEXER_ENDPOINT, response, and
body when adding the controller and timeout logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/data-sources/monarch-indexer/transactions.ts`:
- Around line 110-121: In fetchMonarchTransactions, capture a single frozen
endTimestamp at function start (converted to the same unit the indexer expects)
and pass that fixed endTimestamp into both pagination calls (e.g.,
fetchSuppliesPage and fetchWithdrawsPage) instead of relying on timeRange.end
inside each loop; this ensures both loops use the same static upper bound during
offset-based pagination and avoids skips/duplicates when new rows arrive. Update
fetchSuppliesPage and fetchWithdrawsPage signatures/usages to accept the
endTimestamp parameter and use it in their order_by / timestamp filter logic so
both supply and withdraw loops reference the identical frozen timestamp.
- Line 46: The code silently truncates results when MAX_PAGES (const MAX_PAGES =
50) is hit; change the pagination logic so it either throws an explicit error or
returns a result object with a flag (e.g., { truncated: true }) when
pagesFetched === MAX_PAGES, and make MAX_PAGES configurable (pass as a parameter
or read from config/env) instead of a hard const; update the pagination loop(s)
that reference MAX_PAGES and all callers of the function(s) to handle the new
error/flag so callers can surface or log when results were capped.
♻️ Duplicate comments (1)
src/stores/useAdminAuth.ts (1)
60-84:response.json()called beforeresponse.okcheck.Line 71 parses JSON before checking if the response succeeded. A 502 or similar error with HTML body will throw here, bypassing your error handling.
Proposed fix
const response = await fetch('/api/admin/auth', { method: 'POST', headers: { 'Content-Type': 'application/json' }, credentials: 'same-origin', body: JSON.stringify({ password }), }); - const data = await response.json(); - if (!response.ok) { + const data = await response.json().catch(() => ({})); set({ isLoading: false, error: data.error ?? 'Authentication failed' }); return false; } + const data = await response.json(); set({ isAuthenticated: true, isLoading: false, error: null }); return true;
🧹 Nitpick comments (2)
app/admin/stats-v2/page.tsx (1)
110-150: Minor:isLoadingis alwaysfalsein this branch.The child components receive
isLoading={isLoading}but we're inside the!isLoadingbranch (line 114), so it's always false here. Not a bug - the children might use it for future refresh scenarios - but worth noting.src/stores/useAdminAuth.ts (1)
16-17: Consider using a stronger hash algorithm.The djb2-like hash in the comment produces a 32-bit value - fast to brute force. For admin auth, bcrypt/argon2 would be safer. If this is intentionally lightweight for an internal tool, document that tradeoff.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.