feat: Add Days to Liquidation column to borrowers table#405
Open
feat: Add Days to Liquidation column to borrowers table#405
Conversation
When Morpho API returns NOT_FOUND due to corrupted market records, skip that page and continue fetching the rest instead of failing entirely. This is a temporary workaround until Morpho fixes their API.
- Extract CustomTooltip components to module scope (5 files) - Move formatPercentDisplay to module scope - Fix derived state in useEffect patterns (3 files) - Add prefers-reduced-motion CSS for accessibility - Simplify Avatar component (removed unused fetch pattern) Score: 76 → 82
- Avatar: track failed address instead of boolean to reset on address change - BorrowModal: restore useEffect sync for mode state, remove unnecessary key - EditMetadata: add missing metadataError dep, reset edit state on prop change
- Avatar: rename useEffigy to effigyActive (hook naming convention) - EditMetadata: split reset effect into two (one per field)
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR addresses accessibility improvements, refactors component logic for error handling and state management, and adds a "Days to Liquidation" feature for borrowers. Changes include reduced-motion CSS support, reactive image fallback, tolerant API pagination, managed form inputs, borrower liquidation forecasting, and tooltip component extraction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
starksama
force-pushed
the
fix/skip-failed-pages
branch
from
1b78ce1 to
91ace34
Compare
coderabbitai
bot
added
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/market-detail/components/borrowers-table.tsx (1)
50-75:⚠️ Potential issue | 🟠 Major
market.lltvuses the wrong divisor—should be1e16, not1e4.
market.lltvis in WAD format (18 decimals), as shown in the codebase comment "lltv with 18 decimals" and all other display code that divides by1e16. With the divisor1e4on line 50, the result is an absurdly large number (e.g.,8e13for an 80% LLTV), whileltvon line 60 is calculated as a percentage (0–100 range). The conditionlltv > ltvon line 70 becomes meaningless—one is microscopic, the other orders of magnitude smaller.Change line 50 to:
- const lltv = Number(market.lltv) / 1e4; // lltv is in basis points (e.g., 8000 = 80%) + const lltv = Number(market.lltv) / 1e16; // lltv in WAD format (e.g., 8e17 = 80%)Then
lltvandltvwill be comparable percentages, anddaysToLiquidationwill calculate correctly only when a position is approaching liquidation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/borrowers-table.tsx` around lines 50 - 75, The lltv calculation uses the wrong divisor: update the computation of lltv (from market.lltv) in the borrowers.map block so it divides by 1e16 (WAD -> percent) instead of 1e4; this makes the lltv variable comparable to ltv (both as percentages) so the conditional using lltv > ltv and the daysToLiquidation calculation (which depends on borrowApy, ltv and lltv) behaves correctly. Ensure you only change the line that computes lltv (keep variable name lltv and its usage in the rest of the function such as the conditional and continuous compounding math intact).
🧹 Nitpick comments (10)
src/utils/generateMetadata.ts (1)
13-13: Extract the fallback URL to a named constant.
'https://monarchlend.xyz'is a bare string literal. Per coding guidelines, magic values should be extracted with a descriptive name.♻️ Proposed refactor
const deployUrl = process.env.BOAT_DEPLOY_URL ?? process.env.VERCEL_URL; +const PRODUCTION_URL = 'https://monarchlend.xyz'; -const defaultUrl = deployUrl ? `https://${deployUrl}` : 'https://monarchlend.xyz'; +const defaultUrl = deployUrl ? `https://${deployUrl}` : PRODUCTION_URL;As per coding guidelines: "Use meaningful variable names instead of magic numbers; extract constants with descriptive names."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/generateMetadata.ts` at line 13, Extract the literal 'https://monarchlend.xyz' into a descriptive constant (e.g., FALLBACK_DEPLOY_URL or DEFAULT_DEPLOY_URL) and use that constant when computing defaultUrl (currently const defaultUrl = deployUrl ? `https://${deployUrl}` : 'https://monarchlend.xyz';). Update the top of the module to declare the constant and replace the string literal in the ternary so defaultUrl uses the named constant instead of the magic string.src/features/market-detail/components/charts/concentration-chart.tsx (1)
25-53: LGTM — clean fix for the "no component-inside-component" anti-pattern.The extraction is straightforward and the guard for
position === 0correctly suppresses the sentinel origin point added byunshiftat line 86.One small a11y note: the tooltip
<div>lacksrole="tooltip", which is the semantically correct ARIA role for hover-disclosed content.♿ Suggested addition
- <div className="rounded-lg border border-border bg-background p-3 shadow-lg"> + <div role="tooltip" className="rounded-lg border border-border bg-background p-3 shadow-lg">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/charts/concentration-chart.tsx` around lines 25 - 53, Add the proper ARIA role to the exported tooltip component: update the ConcentrationTooltip function's root <div> to include role="tooltip" so the tooltip is announced correctly to assistive technologies; locate the ConcentrationTooltip component in concentration-chart.tsx and add the role attribute to the top-level container that currently renders the rounded-lg border div.src/modals/borrow/borrow-modal.tsx (1)
37-39: User-toggled mode gets silently overridden ondefaultModechange.If the parent passes a new
defaultModewhile the modal is open and the user has already toggled to the other mode (line 115), their selection is reset. This is likely intentional (e.g., reopening from a "Repay" entry point), but if there's any case where the parent's prop changes due to a re-render rather than an explicit user action, it could be surprising. No code change needed if the behavior is by design — just worth confirming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modals/borrow/borrow-modal.tsx` around lines 37 - 39, The current useEffect always calls setMode(defaultMode) whenever defaultMode changes, which silently overrides a user-toggled mode; update the sync logic in the useEffect around setMode/defaultMode so it only resets when appropriate (e.g., when the modal is opened or when an explicit "reset" signal arrives) rather than on every prop change — implement a guard using the modal open state (isOpen) or a ref that tracks whether this is the initial open (or an explicit parent-driven reset), and only call setMode(defaultMode) when isOpen transitions true (or when the parent indicates a deliberate reset); reference useEffect, setMode, defaultMode and the user toggle handler/element that flips mode so you avoid clobbering user selection.src/features/market-detail/components/charts/supplier-positions-chart.tsx (2)
36-36:blockNumbertyped asnumberbut guarded as falsy — type should benumber | undefined.The type says
blockNumber: number(always present), but the falsy guard{blockNumber && ...}(line 57) implies it can be absent. Block 0 would also be silently suppressed. Making the type optional aligns with both the guard and reality.♻️ Proposed fix
- payload: { timestamp: number; blockNumber: number } + payload: { timestamp: number; blockNumber?: number }And tighten the guard on line 57:
- {blockNumber && <p className="font-mono text-xs text-secondary/70">Block #{blockNumber.toLocaleString()}</p>} + {blockNumber != null && <p className="font-mono text-xs text-secondary/70">Block #{blockNumber.toLocaleString()}</p>}Also applies to: 44-44, 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/charts/supplier-positions-chart.tsx` at line 36, The payload inner type currently declares blockNumber as number but the rendering guard uses a falsy check, so change the type in the payload definition to blockNumber: number | undefined (i.e., payload?: { dataKey: string; value: number; color: string; payload: { timestamp: number; blockNumber: number | undefined } }[]), and update the falsy guard in the render (the {blockNumber && ...} check inside supplier-positions-chart component) to explicitly test for null/undefined (e.g., blockNumber != null or blockNumber !== undefined) so blockNumber === 0 is handled correctly; make the same type and guard adjustments for the other occurrences referenced in this file.
140-140:formatValuenot memoized — inconsistent withgetDisplayNameand creates a newcontentelement each render.
getDisplayNameusesuseCallback;formatValuedoesn't. Because both are passed to thecontentJSX element, every parent render produces a new element reference, which Recharts re-clones unnecessarily.♻️ Proposed fix
- const formatValue = (value: number) => `${formatSimple(value)} ${market.loanAsset.symbol}`; + const formatValue = useCallback( + (value: number) => `${formatSimple(value)} ${market.loanAsset.symbol}`, + [market.loanAsset.symbol], + );Also applies to: 271-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/charts/supplier-positions-chart.tsx` at line 140, formatValue is not memoized which causes a new content JSX element to be created each render (unlike getDisplayName which uses useCallback), forcing Recharts to re-clone; fix by wrapping formatValue in useCallback with the correct dependency array (include market.loanAsset.symbol and any helpers like formatSimple) so the same function reference is reused across renders, and apply the same change to the other occurrence at the second instance (around line 271) so both content props receive stable callbacks.src/features/market-detail/components/charts/debt-at-risk-chart.tsx (1)
226-226: Optional: prefer a render function over JSX element forcontent.Passing
<DebtAtRiskTooltip symbol={...} />works because Recharts clones the element internally and injectsactive/payload. A render function makes the data flow explicit and avoids relying on that internal behaviour.♻️ Render-function form
- content={<DebtAtRiskTooltip symbol={market.loanAsset.symbol} />} + content={(props) => <DebtAtRiskTooltip {...props} symbol={market.loanAsset.symbol} />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/charts/debt-at-risk-chart.tsx` at line 226, Replace the JSX element passed to the Recharts Tooltip's content prop with a render function that explicitly receives the tooltip props and returns <DebtAtRiskTooltip /> with the symbol and the injected props, i.e. change the usage of content={<DebtAtRiskTooltip symbol={market.loanAsset.symbol} />} to a function that accepts the tooltip args and returns DebtAtRiskTooltip with symbol={market.loanAsset.symbol} plus the forwarded props (so DebtAtRiskTooltip receives active/payload explicitly); update any types if needed to match the Tooltip render signature.src/features/market-detail/components/charts/borrowers-pie-chart.tsx (1)
39-42:formatPercentDisplayis duplicated insuppliers-pie-chart.tsxBoth files define byte-for-byte identical implementations. Worth moving to a shared location (e.g.,
src/utils/format.tsor a chart-specific util) so a future change only lands in one place.♻️ Suggested extraction
New shared file (e.g.,
src/utils/chartFormat.ts):+// src/utils/chartFormat.ts +export const formatPercentDisplay = (percent: number): string => { + if (percent < 0.01 && percent > 0) return '<0.01%'; + return `${percent.toFixed(2)}%`; +};Then in both chart files:
-const formatPercentDisplay = (percent: number): string => { - if (percent < 0.01 && percent > 0) return '<0.01%'; - return `${percent.toFixed(2)}%`; -}; +import { formatPercentDisplay } from '@/utils/chartFormat';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/charts/borrowers-pie-chart.tsx` around lines 39 - 42, Extract the duplicated function formatPercentDisplay into a single shared utility (e.g., create chartFormat.ts exporting formatPercentDisplay) and replace the local implementations in borrowers-pie-chart.tsx and suppliers-pie-chart.tsx with imports from that new module; ensure the exported function signature and behavior remain identical and update imports in both components to use the shared export.src/hooks/useReducedMotion.ts (1)
10-10: Initial valuefalsemisses the user's preference on first paint.Because
useState(false)is hardcoded, the hook returnsfalseon the first render (beforeuseEffectruns), causing a brief frame where animations aren't suppressed for users who do prefer reduced motion. A lazy initializer avoids the extra paint:💡 Optional improvement
- const [prefersReducedMotion, setPrefersReducedMotion] = useState(false); + const [prefersReducedMotion, setPrefersReducedMotion] = useState( + () => typeof window !== 'undefined' + ? window.matchMedia('(prefers-reduced-motion: reduce)').matches + : false, + );With this, the
setPrefersReducedMotion(mediaQuery.matches)call insideuseEffectbecomes redundant and can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useReducedMotion.ts` at line 10, The hook useReducedMotion currently initializes prefersReducedMotion with useState(false) which causes a wrong first-paint; change to a lazy initializer that reads the matchMedia value (e.g., useState(() => window.matchMedia('(prefers-reduced-motion: reduce)').matches)) so the initial render reflects the user preference, and then remove the now-redundant setPrefersReducedMotion(mediaQuery.matches) call inside the useEffect; keep references to prefersReducedMotion, setPrefersReducedMotion, useState and useEffect while ensuring mediaQuery is created the same way inside the effect for updates.src/features/market-detail/components/borrowers-table.tsx (2)
135-135:DAYS TO LIQ.abbreviation is opaque to screen readers.Consider wrapping it in an
<abbr>element or adding atitleattribute so assistive technology can expand the abbreviation. As per coding guidelines, use semantic HTML and ARIA attributes for accessibility.💡 Example
- <TableHead className="text-right">DAYS TO LIQ.</TableHead> + <TableHead className="text-right"> + <abbr title="Days to Liquidation">DAYS TO LIQ.</abbr> + </TableHead>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/borrowers-table.tsx` at line 135, The "DAYS TO LIQ." header in the TableHead is an opaque abbreviation for screen readers; update the TableHead content (in the borrowers-table.tsx TableHead element) to use a semantic <abbr> with a full-text title (e.g., title="Days to Liquidation") or add an accessible attribute like aria-label="Days to Liquidation" so assistive tech will expand the abbreviation; ensure the visible text remains "DAYS TO LIQ." while providing the full phrase via title/aria-label on the same element.
47-48: Redundant zero-check —!oraclePricealready covers0nfor bigints.
!0nistruein JavaScript, so the|| oraclePrice === 0nbranch is never reached.♻️ Proposed fix
- if (!oraclePrice || oraclePrice === 0n) return []; + if (!oraclePrice) return [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/market-detail/components/borrowers-table.tsx` around lines 47 - 48, The conditional inside the useMemo for borrowersWithMetrics redundantly checks oraclePrice twice; remove the unnecessary second check and only guard on falsy oraclePrice (i.e., change the if to only test !oraclePrice) so the bigint zero case is covered correctly; update the conditional in the borrowersWithMetrics useMemo that references oraclePrice accordingly.
🤖 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/morpho-api/market.ts`:
- Around line 87-93: The loop currently relies solely on totalCount and skips
failed first-page fetches, causing a silent empty result when totalCount is
still 0; change the pagination condition to track "hasMorePages" independently
(e.g., compute from response.data.markets.items.length === pageSize) and use
that to decide whether to continue paging when totalCount is unknown.
Specifically, inside the markets fetch loop in market.ts, when response is
present set hasMorePages = items.length === pageSize (and update totalCount if
provided), and change the loop/exit logic so you continue if hasMorePages is
true or (totalCount > 0 && skip < totalCount); when response is missing on page
1 increment skip/page pointer and continue without relying on totalCount to
break the loop. Ensure you reference response, response.data.markets.items,
pageSize, skip, totalCount and the while loop controlling pagination.
In `@src/features/autovault/components/vault-detail/settings/EditMetadata.tsx`:
- Around line 66-70: The effect currently clears metadataError immediately
because metadataError is included in the dependency array; remove metadataError
from the deps and the conditional check so the effect becomes useEffect tied
only to computedNameInput and computedSymbolInput and always calls
setMetadataError(null) (calling it with null is a no-op), ensuring errors set by
handleMetadataSubmit are not instantly cleared; update the effect referencing
useEffect, metadataError, setMetadataError, computedNameInput,
computedSymbolInput, and ensure handleMetadataSubmit continues to set
metadataError when appropriate.
In `@src/features/market-detail/components/borrowers-table.tsx`:
- Around line 157-163: The current daysDisplay logic in borrowers-table.tsx
prepends a '<' for borrower.daysToLiquidation < 30 which misleads the reader
(e.g., "<5" when the value is actually 5); update the expression that computes
daysDisplay (variable daysDisplay, using borrower.daysToLiquidation) to remove
the '<' prefix for values under 30 so it shows the actual numeric value (keep
the existing >365 cap and the null '—' case).
In `@src/features/market-detail/components/charts/borrowers-pie-chart.tsx`:
- Around line 77-82: The Collateral row in borrowers-pie-chart.tsx is missing
the asset icon; update the collateral row to mirror the borrowed row by
rendering the TokenIcon component next to the collateral amount: use
market.collateralAsset as the token prop and keep the existing formatting via
formatSimple(data.collateral) inside the same "flex items-center gap-1
tabular-nums" wrapper so the collateral line displays the TokenIcon followed by
the formatted collateral value.
In `@src/features/market-detail/components/charts/debt-at-risk-chart.tsx`:
- Around line 27-57: The wrapper div in the DebtAtRiskTooltip component lacks an
ARIA role; update the top-level returned element inside function
DebtAtRiskTooltip to include role="tooltip" (i.e., <div role="tooltip" ...>) so
screen readers identify it as a tooltip while leaving the existing early return
(if !active || !payload || !payload[0]) unchanged.
---
Outside diff comments:
In `@src/features/market-detail/components/borrowers-table.tsx`:
- Around line 50-75: The lltv calculation uses the wrong divisor: update the
computation of lltv (from market.lltv) in the borrowers.map block so it divides
by 1e16 (WAD -> percent) instead of 1e4; this makes the lltv variable comparable
to ltv (both as percentages) so the conditional using lltv > ltv and the
daysToLiquidation calculation (which depends on borrowApy, ltv and lltv) behaves
correctly. Ensure you only change the line that computes lltv (keep variable
name lltv and its usage in the rest of the function such as the conditional and
continuous compounding math intact).
---
Duplicate comments:
In `@src/features/market-detail/components/charts/suppliers-pie-chart.tsx`:
- Around line 35-39: The file defines a duplicate helper formatPercentDisplay at
module scope in suppliers-pie-chart.tsx; remove this duplication by extracting
formatPercentDisplay into a shared utility (e.g., create or add to percentUtils
or chartUtils) and export it, then replace the local function in
suppliers-pie-chart.tsx (and the identical one in the borrowers chart) with an
import of the shared formatPercentDisplay; ensure the exported symbol name
matches callers and update imports in components that used the previous local
implementations (formatPercentDisplay in suppliers-pie-chart.tsx and the
borrowers chart) so both reference the single shared function.
---
Nitpick comments:
In `@src/features/market-detail/components/borrowers-table.tsx`:
- Line 135: The "DAYS TO LIQ." header in the TableHead is an opaque abbreviation
for screen readers; update the TableHead content (in the borrowers-table.tsx
TableHead element) to use a semantic <abbr> with a full-text title (e.g.,
title="Days to Liquidation") or add an accessible attribute like
aria-label="Days to Liquidation" so assistive tech will expand the abbreviation;
ensure the visible text remains "DAYS TO LIQ." while providing the full phrase
via title/aria-label on the same element.
- Around line 47-48: The conditional inside the useMemo for borrowersWithMetrics
redundantly checks oraclePrice twice; remove the unnecessary second check and
only guard on falsy oraclePrice (i.e., change the if to only test !oraclePrice)
so the bigint zero case is covered correctly; update the conditional in the
borrowersWithMetrics useMemo that references oraclePrice accordingly.
In `@src/features/market-detail/components/charts/borrowers-pie-chart.tsx`:
- Around line 39-42: Extract the duplicated function formatPercentDisplay into a
single shared utility (e.g., create chartFormat.ts exporting
formatPercentDisplay) and replace the local implementations in
borrowers-pie-chart.tsx and suppliers-pie-chart.tsx with imports from that new
module; ensure the exported function signature and behavior remain identical and
update imports in both components to use the shared export.
In `@src/features/market-detail/components/charts/concentration-chart.tsx`:
- Around line 25-53: Add the proper ARIA role to the exported tooltip component:
update the ConcentrationTooltip function's root <div> to include role="tooltip"
so the tooltip is announced correctly to assistive technologies; locate the
ConcentrationTooltip component in concentration-chart.tsx and add the role
attribute to the top-level container that currently renders the rounded-lg
border div.
In `@src/features/market-detail/components/charts/debt-at-risk-chart.tsx`:
- Line 226: Replace the JSX element passed to the Recharts Tooltip's content
prop with a render function that explicitly receives the tooltip props and
returns <DebtAtRiskTooltip /> with the symbol and the injected props, i.e.
change the usage of content={<DebtAtRiskTooltip symbol={market.loanAsset.symbol}
/>} to a function that accepts the tooltip args and returns DebtAtRiskTooltip
with symbol={market.loanAsset.symbol} plus the forwarded props (so
DebtAtRiskTooltip receives active/payload explicitly); update any types if
needed to match the Tooltip render signature.
In `@src/features/market-detail/components/charts/supplier-positions-chart.tsx`:
- Line 36: The payload inner type currently declares blockNumber as number but
the rendering guard uses a falsy check, so change the type in the payload
definition to blockNumber: number | undefined (i.e., payload?: { dataKey:
string; value: number; color: string; payload: { timestamp: number; blockNumber:
number | undefined } }[]), and update the falsy guard in the render (the
{blockNumber && ...} check inside supplier-positions-chart component) to
explicitly test for null/undefined (e.g., blockNumber != null or blockNumber !==
undefined) so blockNumber === 0 is handled correctly; make the same type and
guard adjustments for the other occurrences referenced in this file.
- Line 140: formatValue is not memoized which causes a new content JSX element
to be created each render (unlike getDisplayName which uses useCallback),
forcing Recharts to re-clone; fix by wrapping formatValue in useCallback with
the correct dependency array (include market.loanAsset.symbol and any helpers
like formatSimple) so the same function reference is reused across renders, and
apply the same change to the other occurrence at the second instance (around
line 271) so both content props receive stable callbacks.
In `@src/hooks/useReducedMotion.ts`:
- Line 10: The hook useReducedMotion currently initializes prefersReducedMotion
with useState(false) which causes a wrong first-paint; change to a lazy
initializer that reads the matchMedia value (e.g., useState(() =>
window.matchMedia('(prefers-reduced-motion: reduce)').matches)) so the initial
render reflects the user preference, and then remove the now-redundant
setPrefersReducedMotion(mediaQuery.matches) call inside the useEffect; keep
references to prefersReducedMotion, setPrefersReducedMotion, useState and
useEffect while ensuring mediaQuery is created the same way inside the effect
for updates.
In `@src/modals/borrow/borrow-modal.tsx`:
- Around line 37-39: The current useEffect always calls setMode(defaultMode)
whenever defaultMode changes, which silently overrides a user-toggled mode;
update the sync logic in the useEffect around setMode/defaultMode so it only
resets when appropriate (e.g., when the modal is opened or when an explicit
"reset" signal arrives) rather than on every prop change — implement a guard
using the modal open state (isOpen) or a ref that tracks whether this is the
initial open (or an explicit parent-driven reset), and only call
setMode(defaultMode) when isOpen transitions true (or when the parent indicates
a deliberate reset); reference useEffect, setMode, defaultMode and the user
toggle handler/element that flips mode so you avoid clobbering user selection.
In `@src/utils/generateMetadata.ts`:
- Line 13: Extract the literal 'https://monarchlend.xyz' into a descriptive
constant (e.g., FALLBACK_DEPLOY_URL or DEFAULT_DEPLOY_URL) and use that constant
when computing defaultUrl (currently const defaultUrl = deployUrl ?
`https://${deployUrl}` : 'https://monarchlend.xyz';). Update the top of the
module to declare the constant and replace the string literal in the ternary so
defaultUrl uses the named constant instead of the magic string.
Comment on lines
+87
to
+93
| // Handle failed pages - skip to next page instead of breaking entirely | ||
| // This handles corrupted market records that cause NOT_FOUND errors | ||
| if (!response || !response.data?.markets?.items || !response.data.markets.pageInfo) { | ||
| console.warn(`[Markets] Skipping failed page at skip=${skip} for network ${network}`); | ||
| skip += pageSize; // Skip ahead to next page | ||
| if (totalCount > 0 && skip >= totalCount) break; | ||
| continue; |
There was a problem hiding this comment.
First-page failure silently returns 0 markets.
When the very first fetch fails, totalCount stays 0, skip jumps to 500, then while (500 < 0) exits the loop. The skip-and-continue logic only helps for page 2+. A network or API blip on page 1 will silently produce an empty result rather than retrying or raising.
Consider tracking "has more pages" independently of totalCount, e.g. by checking whether the page returned pageSize items, so the loop can continue even before totalCount is known.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/data-sources/morpho-api/market.ts` around lines 87 - 93, The loop
currently relies solely on totalCount and skips failed first-page fetches,
causing a silent empty result when totalCount is still 0; change the pagination
condition to track "hasMorePages" independently (e.g., compute from
response.data.markets.items.length === pageSize) and use that to decide whether
to continue paging when totalCount is unknown. Specifically, inside the markets
fetch loop in market.ts, when response is present set hasMorePages =
items.length === pageSize (and update totalCount if provided), and change the
loop/exit logic so you continue if hasMorePages is true or (totalCount > 0 &&
skip < totalCount); when response is missing on page 1 increment skip/page
pointer and continue without relying on totalCount to break the loop. Ensure you
reference response, response.data.markets.items, pageSize, skip, totalCount and
the while loop controlling pagination.
Comment on lines
66
to
+70
| useEffect(() => { | ||
| if (metadataError && metadataChanged) { | ||
| if (metadataError) { | ||
| setMetadataError(null); | ||
| } | ||
| }, [metadataChanged, metadataError]); | ||
| }, [computedNameInput, computedSymbolInput, metadataError]); |
There was a problem hiding this comment.
metadataError in effect deps self-clears the error immediately.
The useEffect fires whenever metadataError changes. So when handleMetadataSubmit sets the error, the effect runs in the same pass, sees a truthy metadataError, and sets it back to null. The error either never renders or flashes for a single frame — users won't see it.
Remove metadataError from the dependency array and drop the conditional guard (calling setMetadataError(null) when it's already null is a no-op; React skips the re-render).
🐛 Proposed fix
- // Clear error when inputs change
useEffect(() => {
- if (metadataError) {
- setMetadataError(null);
- }
- }, [computedNameInput, computedSymbolInput, metadataError]);
+ setMetadataError(null);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [computedNameInput, computedSymbolInput]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/autovault/components/vault-detail/settings/EditMetadata.tsx`
around lines 66 - 70, The effect currently clears metadataError immediately
because metadataError is included in the dependency array; remove metadataError
from the deps and the conditional check so the effect becomes useEffect tied
only to computedNameInput and computedSymbolInput and always calls
setMetadataError(null) (calling it with null is a no-op), ensuring errors set by
handleMetadataSubmit are not instantly cleared; update the effect referencing
useEffect, metadataError, setMetadataError, computedNameInput,
computedSymbolInput, and ensure handleMetadataSubmit continues to set
metadataError when appropriate.
Comment on lines
+157
to
+163
| const daysDisplay = borrower.daysToLiquidation !== null | ||
| ? borrower.daysToLiquidation < 30 | ||
| ? `<${borrower.daysToLiquidation}` | ||
| : borrower.daysToLiquidation > 365 | ||
| ? '>365' | ||
| : `${borrower.daysToLiquidation}` | ||
| : '—'; |
There was a problem hiding this comment.
<${N} display is misleading — it reads as "less than N days" but N is the actual value.
For daysToLiquidation = 5, the display is <5, implying fewer than 5 days remain. The intent seems to be capping the display at >365 and showing the exact value otherwise. Either drop the < prefix entirely or use a fixed categorical label (e.g., <30).
💡 Proposed fix
- const daysDisplay = borrower.daysToLiquidation !== null
- ? borrower.daysToLiquidation < 30
- ? `<${borrower.daysToLiquidation}`
- : borrower.daysToLiquidation > 365
- ? '>365'
- : `${borrower.daysToLiquidation}`
- : '—';
+ const DAYS_CAP = 365;
+ const daysDisplay = borrower.daysToLiquidation !== null
+ ? borrower.daysToLiquidation > DAYS_CAP
+ ? `>${DAYS_CAP}`
+ : `${borrower.daysToLiquidation}`
+ : '—';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/market-detail/components/borrowers-table.tsx` around lines 157 -
163, The current daysDisplay logic in borrowers-table.tsx prepends a '<' for
borrower.daysToLiquidation < 30 which misleads the reader (e.g., "<5" when the
value is actually 5); update the expression that computes daysDisplay (variable
daysDisplay, using borrower.daysToLiquidation) to remove the '<' prefix for
values under 30 so it shows the actual numeric value (keep the existing >365 cap
and the null '—' case).
Comment on lines
+77
to
+82
| <div className="flex items-center justify-between gap-4 text-sm"> | ||
| <span className="text-secondary">Collateral</span> | ||
| <div className="flex items-center gap-1 tabular-nums"> | ||
| <span>{formatSimple(data.collateral)}</span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Collateral row is missing the asset icon
The borrowed row (lines 64–76) shows a TokenIcon next to the amount, but the collateral row doesn't. Since market.collateralAsset is available via the market prop, adding the icon keeps the tooltip consistent.
🛠️ Suggested fix
+import { TokenIcon } from '@/components/shared/token-icon';
...
<div className="flex items-center justify-between gap-4 text-sm">
<span className="text-secondary">Collateral</span>
<div className="flex items-center gap-1 tabular-nums">
<span>{formatSimple(data.collateral)}</span>
+ <TokenIcon
+ address={market.collateralAsset.address}
+ chainId={market.morphoBlue.chain.id}
+ width={14}
+ height={14}
+ />
</div>
</div>📝 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
| <div className="flex items-center justify-between gap-4 text-sm"> | |
| <span className="text-secondary">Collateral</span> | |
| <div className="flex items-center gap-1 tabular-nums"> | |
| <span>{formatSimple(data.collateral)}</span> | |
| </div> | |
| </div> | |
| <div className="flex items-center justify-between gap-4 text-sm"> | |
| <span className="text-secondary">Collateral</span> | |
| <div className="flex items-center gap-1 tabular-nums"> | |
| <span>{formatSimple(data.collateral)}</span> | |
| <TokenIcon | |
| address={market.collateralAsset.address} | |
| chainId={market.morphoBlue.chain.id} | |
| width={14} | |
| height={14} | |
| /> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/market-detail/components/charts/borrowers-pie-chart.tsx` around
lines 77 - 82, The Collateral row in borrowers-pie-chart.tsx is missing the
asset icon; update the collateral row to mirror the borrowed row by rendering
the TokenIcon component next to the collateral amount: use
market.collateralAsset as the token prop and keep the existing formatting via
formatSimple(data.collateral) inside the same "flex items-center gap-1
tabular-nums" wrapper so the collateral line displays the TokenIcon followed by
the formatted collateral value.
Comment on lines
+27
to
+57
| // Custom tooltip at module scope | ||
| function DebtAtRiskTooltip({ | ||
| active, | ||
| payload, | ||
| symbol, | ||
| }: { | ||
| active?: boolean; | ||
| payload?: { value: number; payload: RiskDataPoint }[]; | ||
| symbol: string; | ||
| }) { | ||
| if (!active || !payload || !payload[0]) return null; | ||
| const data = payload[0].payload; | ||
|
|
||
| return ( | ||
| <div className="rounded-lg border border-border bg-background p-3 shadow-lg"> | ||
| <p className="mb-2 text-xs text-secondary">At {data.priceDrop}% price drop</p> | ||
| <div className="space-y-1"> | ||
| <div className="flex items-center justify-between gap-6 text-sm"> | ||
| <span className="text-secondary">% of Total Debt</span> | ||
| <span className="tabular-nums font-medium">{data.cumulativeDebtPercent.toFixed(1)}%</span> | ||
| </div> | ||
| <div className="flex items-center justify-between gap-6 text-sm"> | ||
| <span className="text-secondary">Debt at Risk</span> | ||
| <span className="tabular-nums"> | ||
| {formatReadable(data.cumulativeDebt)} {symbol} | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add role="tooltip" to the wrapper div.
The tooltip div has no ARIA role, which means screen readers won't identify it as a tooltip.
As per coding guidelines, semantic HTML and ARIA attributes should be used for accessibility.
🛠 Proposed fix
- <div className="rounded-lg border border-border bg-background p-3 shadow-lg">
+ <div role="tooltip" className="rounded-lg border border-border bg-background p-3 shadow-lg">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/market-detail/components/charts/debt-at-risk-chart.tsx` around
lines 27 - 57, The wrapper div in the DebtAtRiskTooltip component lacks an ARIA
role; update the top-level returned element inside function DebtAtRiskTooltip to
include role="tooltip" (i.e., <div role="tooltip" ...>) so screen readers
identify it as a tooltip while leaving the existing early return (if !active ||
!payload || !payload[0]) unchanged.
Fixes: - lltv divisor: 1e4 -> 1e16 (WAD format) - Remove redundant oraclePrice check Calculates days to liquidation using: - Current LTV from borrow/collateral ratio - Market's liquidation threshold (lltv) - Annual borrow rate (borrowApy) Uses continuous compounding formula: r = ln(1 + APY) // convert APY to continuous rate Days = ln(lltv/ltv) / r * 365 Color coded: red (<7d), orange (<30d), green (>30d)
starksama
force-pushed
the
fix/skip-failed-pages
branch
from
91ace34 to
062a2cc
Compare
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
Adds a "Days to Liq." column to the borrowers table showing estimated days until liquidation.
Calculation
Uses continuous compounding formula:
Days = ln(lltv/ltv) / borrowApy * 365Where:
Display
Related
Closes #344
Summary by CodeRabbit
New Features
Bug Fixes
Improvements