Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRemoved the proxied Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
starksama
left a comment
starksama
left a comment
There was a problem hiding this comment.
Reviewed on latest head.
I spot-checked the actual refactor changes (Envio -> Monarch API naming/data-source consolidation), fallback provider labels, and admin-v2 data-source wiring.
Validation:
- pnpm lint:check ✅
- pnpm typecheck ✅ (after clearing stale local .next generated route types from deleted API file)
LGTM from my side.
coderabbitai
Bot
added
the
feature request
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/data-sources/monarch-api/market-detail.ts (2)
209-215:⚠️ Potential issue | 🟠 MajorBound these Monarch requests.
monarchGraphqlFetcheraccepts asignal, but none of these new calls pass one. If Monarch stalls,runMarketDetailFallbacknever reaches Morpho API or subgraph, so the hook can hang instead of failing over. Please add a per-request timeout and pass its abort signal into each fetch.As per coding guidelines: Network fetches should use bounded request timeouts that account for fallback coverage; any fallback path used for first paint must preserve market completeness (no truncated first: 1000 fallback).
Also applies to: 237-243, 262-268, 282-288, 302-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data-sources/monarch-api/market-detail.ts` around lines 209 - 215, The monarchGraphqlFetcher calls (e.g., monarchGraphqlFetcher<MonarchSuppliersPageResponse>(envioSuppliersPageQuery, {...})) are missing an AbortSignal, so stalled Monarch requests can block the fallback; wrap each call with a per-request AbortController using a bounded timeout (use a shared constant like MONARCH_REQUEST_TIMEOUT_MS), pass controller.signal into the monarchGraphqlFetcher options, and clear the timeout/abort on completion; apply the same pattern to the other similar calls referenced (the calls around the other ranges) so the runMarketDetailFallback path can reliably fail-over when Monarch is slow.
137-150:⚠️ Potential issue | 🟠 MajorStop scanning the full participant set before slicing.
fetchMonarchMarketSuppliersandfetchMonarchMarketBorrowersstill walk every 1000-row page viascanAllPages()and only thenslice(). On large markets, page 1 now pays for the whole participant list on the shared endpoint, which makes latency and API cost scale with total participant count. This path needs provider-boundary pagination, or an explicit fail-closed/unknown-total mode, instead of full-history scans here.Based on learnings: Provider fallbacks for market details must page at the provider boundary or fail closed with typed source/network errors instead of fetching full history and slicing client-side or returning empty success on missing subgraph configuration.
Also applies to: 199-224, 227-254, 320-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data-sources/monarch-api/market-detail.ts` around lines 137 - 150, The current scanAllPages implementation (used by fetchMonarchMarketSuppliers and fetchMonarchMarketBorrowers) fetches the entire participant set page-by-page before any slicing, causing provider-side full scans; change the flow to paginate at the provider boundary by adding a maxResults or limit parameter to scanAllPages and to the provider fetchPage calls (and/or add a stop condition inside scanAllPages to return as soon as collectedItems.length >= requestedLimit) so you never request beyond what the caller needs; additionally add a fail-closed/unknown-total mode where, if the provider cannot honor bounded pagination, the functions (fetchMonarchMarketSuppliers/fetchMonarchMarketBorrowers) return a typed error indicating the provider/network limitation instead of silently returning truncated/empty results, and keep MONARCH_SCAN_BATCH_SIZE only as the internal page size while honoring the upstream max limit to avoid full-history scans.src/data-sources/monarch-api/transactions.ts (1)
102-133:⚠️ Potential issue | 🟠 MajorDo not silently truncate the
ALLrange at 50 pages.Both loops stop after
MAX_PAGES, but the dashboard still offersALL. Once either collection grows past50 * limit, the charts and totals undercount with no signal. Either page until exhaustion or throw when the cap is hit so this fails closed instead of showing partial stats.Possible fail-closed guard
for (let page = 0; page < MAX_PAGES; page++) { const supplies = await fetchSuppliesPage(frozenTimeRange, limit, suppliesOffset); allSupplies.push(...supplies); if (supplies.length < limit) break; + if (page === MAX_PAGES - 1) { + throw new Error(`Monarch supplies pagination exceeded ${MAX_PAGES} pages`); + } suppliesOffset += limit; } @@ for (let page = 0; page < MAX_PAGES; page++) { const withdraws = await fetchWithdrawsPage(frozenTimeRange, limit, withdrawsOffset); allWithdraws.push(...withdraws); if (withdraws.length < limit) break; + if (page === MAX_PAGES - 1) { + throw new Error(`Monarch withdraw pagination exceeded ${MAX_PAGES} pages`); + } withdrawsOffset += limit; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/data-sources/monarch-api/transactions.ts` around lines 102 - 133, The loops in fetchMonarchTransactions currently stop silently after MAX_PAGES which undercounts "ALL" ranges; modify the paging logic in fetchMonarchTransactions so it fails closed instead of truncating: when iterating with fetchSuppliesPage and fetchWithdrawsPage, detect if you reached MAX_PAGES and the last page returned length === limit (meaning more pages likely exist) and throw a descriptive error (e.g., "Monarch transactions page cap reached") rather than returning partial results; apply the same check for both the supplies loop and the withdraws loop (referencing suppliesOffset/withdrawsOffset, MAX_PAGES, fetchSuppliesPage, fetchWithdrawsPage, and fetchMonarchTransactions) so callers can handle or surface the failure.
🧹 Nitpick comments (2)
src/hooks/useMarketBorrows.ts (1)
102-102: Incomplete dependency array in useEffect.Same issue as
useMarketSupplies.ts- missingmarketId,loanAssetId,network,minAssets,pageSizefrom deps.Suggested fix
- }, [page, data, queryClient, queryFn]); + }, [page, data, queryClient, queryFn, marketId, loanAssetId, network, minAssets, pageSize]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useMarketBorrows.ts` at line 102, The useEffect dependency array in the hook useMarketBorrows is missing several props (marketId, loanAssetId, network, minAssets, pageSize) which can cause stale closures; update the dependency array used in the effect inside useMarketBorrows (the effect that currently lists [page, data, queryClient, queryFn]) to include marketId, loanAssetId, network, minAssets and pageSize so the effect reruns when any of those inputs change, keeping behavior consistent with useMarketSupplies; ensure you keep existing dependencies (page, data, queryClient, queryFn) and only add the listed identifiers.src/hooks/useMarketSupplies.ts (1)
102-102: Incomplete dependency array in useEffect.The
useEffectdependency array is missingmarketId,loanAssetId,network,minAssets, andpageSize. Compare withuseMarketBorrowers.tsline 145 which includes all deps. Works via closure but may cause stale prefetches if those values change mid-render.Suggested fix
- }, [page, data, queryClient, queryFn]); + }, [page, data, queryClient, queryFn, marketId, loanAssetId, network, minAssets, pageSize]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useMarketSupplies.ts` at line 102, The useEffect in useMarketSupplies.ts has an incomplete dependency array (currently [page, data, queryClient, queryFn]) which can lead to stale prefetches; update the dependency array used by the useEffect that performs prefetching to include marketId, loanAssetId, network, minAssets, and pageSize in addition to page, data, queryClient, and queryFn so the effect re-runs when any of those values change (mirror the dependencies used in useMarketBorrowers.ts line ~145); ensure you reference the same variables used inside the effect and avoid adding non-stable references unless memoized.
🤖 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/data-sources/monarch-api/transactions.ts`:
- Around line 11-12: The admin stats read path currently calls
monarchGraphqlFetcher from client pages (e.g., used by PasswordGate) — move that
logic to a server-side API/route or a server action (create a server-only
handler such as getAdminStatsServer or /api/admin/stats) that calls
monarchGraphqlFetcher using a server-only env var (do NOT use NEXT_PUBLIC_*),
applies a bounded AbortController timeout on the upstream fetch, and returns
only the necessary data to the client; update all referenced client calls (the
blocks around lines 72-94 and 102-133) to call this server endpoint instead of
invoking monarchGraphqlFetcher directly.
---
Outside diff comments:
In `@src/data-sources/monarch-api/market-detail.ts`:
- Around line 209-215: The monarchGraphqlFetcher calls (e.g.,
monarchGraphqlFetcher<MonarchSuppliersPageResponse>(envioSuppliersPageQuery,
{...})) are missing an AbortSignal, so stalled Monarch requests can block the
fallback; wrap each call with a per-request AbortController using a bounded
timeout (use a shared constant like MONARCH_REQUEST_TIMEOUT_MS), pass
controller.signal into the monarchGraphqlFetcher options, and clear the
timeout/abort on completion; apply the same pattern to the other similar calls
referenced (the calls around the other ranges) so the runMarketDetailFallback
path can reliably fail-over when Monarch is slow.
- Around line 137-150: The current scanAllPages implementation (used by
fetchMonarchMarketSuppliers and fetchMonarchMarketBorrowers) fetches the entire
participant set page-by-page before any slicing, causing provider-side full
scans; change the flow to paginate at the provider boundary by adding a
maxResults or limit parameter to scanAllPages and to the provider fetchPage
calls (and/or add a stop condition inside scanAllPages to return as soon as
collectedItems.length >= requestedLimit) so you never request beyond what the
caller needs; additionally add a fail-closed/unknown-total mode where, if the
provider cannot honor bounded pagination, the functions
(fetchMonarchMarketSuppliers/fetchMonarchMarketBorrowers) return a typed error
indicating the provider/network limitation instead of silently returning
truncated/empty results, and keep MONARCH_SCAN_BATCH_SIZE only as the internal
page size while honoring the upstream max limit to avoid full-history scans.
In `@src/data-sources/monarch-api/transactions.ts`:
- Around line 102-133: The loops in fetchMonarchTransactions currently stop
silently after MAX_PAGES which undercounts "ALL" ranges; modify the paging logic
in fetchMonarchTransactions so it fails closed instead of truncating: when
iterating with fetchSuppliesPage and fetchWithdrawsPage, detect if you reached
MAX_PAGES and the last page returned length === limit (meaning more pages likely
exist) and throw a descriptive error (e.g., "Monarch transactions page cap
reached") rather than returning partial results; apply the same check for both
the supplies loop and the withdraws loop (referencing
suppliesOffset/withdrawsOffset, MAX_PAGES, fetchSuppliesPage,
fetchWithdrawsPage, and fetchMonarchTransactions) so callers can handle or
surface the failure.
---
Nitpick comments:
In `@src/hooks/useMarketBorrows.ts`:
- Line 102: The useEffect dependency array in the hook useMarketBorrows is
missing several props (marketId, loanAssetId, network, minAssets, pageSize)
which can cause stale closures; update the dependency array used in the effect
inside useMarketBorrows (the effect that currently lists [page, data,
queryClient, queryFn]) to include marketId, loanAssetId, network, minAssets and
pageSize so the effect reruns when any of those inputs change, keeping behavior
consistent with useMarketSupplies; ensure you keep existing dependencies (page,
data, queryClient, queryFn) and only add the listed identifiers.
In `@src/hooks/useMarketSupplies.ts`:
- Line 102: The useEffect in useMarketSupplies.ts has an incomplete dependency
array (currently [page, data, queryClient, queryFn]) which can lead to stale
prefetches; update the dependency array used by the useEffect that performs
prefetching to include marketId, loanAssetId, network, minAssets, and pageSize
in addition to page, data, queryClient, and queryFn so the effect re-runs when
any of those values change (mirror the dependencies used in
useMarketBorrowers.ts line ~145); ensure you reference the same variables used
inside the effect and avoid adding non-stable references unless memoized.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 823281d6-6326-495e-b45d-37433845a850
📒 Files selected for processing (19)
app/admin/stats-v2/page.tsxapp/api/admin/monarch-indexer/route.tsdocs/TECHNICAL_OVERVIEW.mdsrc/data-sources/envio/fetchers.tssrc/data-sources/monarch-api/index.tssrc/data-sources/monarch-api/market-detail.tssrc/data-sources/monarch-api/transactions.tssrc/data-sources/monarch-indexer/fetchers.tssrc/data-sources/monarch-indexer/index.tssrc/features/admin-v2/components/password-gate.tsxsrc/features/admin-v2/index.tssrc/hooks/queries/market-detail-fallback.tssrc/hooks/useMarketBorrowers.tssrc/hooks/useMarketBorrows.tssrc/hooks/useMarketLiquidations.tssrc/hooks/useMarketSuppliers.tssrc/hooks/useMarketSupplies.tssrc/hooks/useMonarchTransactions.tssrc/stores/useAdminAuth.ts
💤 Files with no reviewable changes (4)
- src/data-sources/envio/fetchers.ts
- src/data-sources/monarch-indexer/fetchers.ts
- src/data-sources/monarch-indexer/index.ts
- app/api/admin/monarch-indexer/route.ts
| import { monarchGraphqlFetcher } from './fetchers'; | ||
|
|
There was a problem hiding this comment.
Keep the admin stats read path behind a server-side reader.
app/admin/stats-v2/page.tsx is still a client page, and monarchGraphqlFetcher sends this GraphQL call straight from the browser. That makes PasswordGate UI-only: the underlying admin query and shared API spend are no longer protected by the old server proxy. Please route this through a server action or route that uses a server-only key and a bounded timeout.
Based on learnings: Server-side Monarch proxy routes must use server-only API key env vars and bounded AbortController timeouts on upstream fetches; do not let Monarch GraphQL or metrics calls hang indefinitely; do not reference NEXT_PUBLIC_* secrets in server authorization headers.
Also applies to: 72-94, 102-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/data-sources/monarch-api/transactions.ts` around lines 11 - 12, The admin
stats read path currently calls monarchGraphqlFetcher from client pages (e.g.,
used by PasswordGate) — move that logic to a server-side API/route or a server
action (create a server-only handler such as getAdminStatsServer or
/api/admin/stats) that calls monarchGraphqlFetcher using a server-only env var
(do NOT use NEXT_PUBLIC_*), applies a bounded AbortController timeout on the
upstream fetch, and returns only the necessary data to the client; update all
referenced client calls (the blocks around lines 72-94 and 102-133) to call this
server endpoint instead of invoking monarchGraphqlFetcher directly.
2 participants
Summary by CodeRabbit
Release Notes
Refactor
Documentation