add amount to reach util number#609
Conversation
railway-app
Bot
temporarily deployed
to
euler-lite / euler-lite-pr-609
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthrough
ChangesVault Utilization Delta Tooltip
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. 🔧 OpenGrep (1.23.0)tests/utils/vault-display.test.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path utils/vault-display.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path 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)
tests/utils/vault-display.test.ts (1)
13-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd invalid-input coverage for the new guard paths.
Given
getVaultUtilizationDeltanow hardensunknowninputs, add explicit tests for non-object vaults and non-finite targets (NaN,Infinity) returningnull.🤖 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/utils/vault-display.test.ts` around lines 13 - 50, The test suite for getVaultUtilizationDelta needs additional coverage for invalid inputs that the function now guards against. Add new test cases within the describe block that verify getVaultUtilizationDelta returns null when passed non-object vaults (such as null, undefined, or primitive values) and when passed non-finite targets (NaN and Infinity values). These tests should validate the defensive input validation logic that was added to the function.
🤖 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 `@utils/vault-display.ts`:
- Around line 12-13: The APY getter functions use typeof checks that incorrectly
accept NaN and Infinity values since both typeof NaN and typeof Infinity return
'number'. Replace the typeof checks for both interestRates?.supplyAPY and
source.supplyApy1h with Number.isFinite() to ensure only valid finite numbers
are accepted. This prevents invalid values from propagating into UI math and
chart calculations, falling back to 0 for any non-finite or missing values.
---
Nitpick comments:
In `@tests/utils/vault-display.test.ts`:
- Around line 13-50: The test suite for getVaultUtilizationDelta needs
additional coverage for invalid inputs that the function now guards against. Add
new test cases within the describe block that verify getVaultUtilizationDelta
returns null when passed non-object vaults (such as null, undefined, or
primitive values) and when passed non-finite targets (NaN and Infinity values).
These tests should validate the defensive input validation logic that was added
to the function.
🪄 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: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a5aa914-a881-49d9-a36f-1e4eaa515f92
📒 Files selected for processing (3)
components/entities/vault/overview/VaultOverviewBlockIRM.vuetests/utils/vault-display.test.tsutils/vault-display.ts
| if (typeof interestRates?.supplyAPY === 'number') return interestRates.supplyAPY | ||
| return typeof source.supplyApy1h === 'number' ? source.supplyApy1h : 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard APY reads against NaN/Infinity values.
typeof x === 'number' still accepts non-finite numbers. Returning those can propagate invalid values into chart/UI math. Prefer Number.isFinite(...) in both APY getters.
Proposed patch
export const getVaultSupplyApy = (vault: unknown): number => {
const source = asRecord(vault)
if (!source) return 0
const interestRates = asRecord(source.interestRates)
- if (typeof interestRates?.supplyAPY === 'number') return interestRates.supplyAPY
- return typeof source.supplyApy1h === 'number' ? source.supplyApy1h : 0
+ if (Number.isFinite(interestRates?.supplyAPY)) return interestRates!.supplyAPY as number
+ return Number.isFinite(source.supplyApy1h) ? source.supplyApy1h as number : 0
}
@@
export const getVaultBorrowApy = (vault: unknown): number => {
const source = asRecord(vault)
const interestRates = asRecord(source?.interestRates)
- return typeof interestRates?.borrowAPY === 'number' ? interestRates.borrowAPY : 0
+ return Number.isFinite(interestRates?.borrowAPY) ? interestRates!.borrowAPY as number : 0
}Also applies to: 19-19
🤖 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 `@utils/vault-display.ts` around lines 12 - 13, The APY getter functions use
typeof checks that incorrectly accept NaN and Infinity values since both typeof
NaN and typeof Infinity return 'number'. Replace the typeof checks for both
interestRates?.supplyAPY and source.supplyApy1h with Number.isFinite() to ensure
only valid finite numbers are accepted. This prevents invalid values from
propagating into UI math and chart calculations, falling back to 0 for any
non-finite or missing values.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed head faf2283426e7d980e837c084cd817bcffe938168.
Verdict: Changes requested — one user-facing correctness issue in the tooltip action label/amount semantics.
What changed:
- Adds shared vault utilization delta helpers in
utils/vault-display.ts. - Adds focused utility tests.
- Displays the utilization delta in the IRM chart tooltip footer on vault overview pages.
Validation performed:
npm run test:run -- tests/utils/vault-display.test.ts— passed.npm run typecheck— passed.npm run build— passed, with existing chunk/dynamic-import style warnings only.- Browser visual smoke on a local PR build for
/lend/0xD8b27CF359b7D15710a5BE299AF6e7Bf904984C2?network=1at desktop and mobile widths. The IRM chart rendered, mobile required opening the Vault information panel, and the tooltip footer displayed the new amount. Wallet/AppKit warnings were expected for disconnected local smoke; no wallet/signing coverage was run. Screenshots were captured locally but not attached because the available GitHub CLI/gist path here does not support binary PNG uploads.
Scalability / maintainability hygiene pass:
- Good direction: the calculation lives in
utils/vault-display.tswith focused tests rather than as purely inline component math. - Sibling utilization consumers (
borrowlist sorting/cards, vault warnings, stats displays) use utilization as a percentage/readout and do not need this tooltip-specific delta. No missed sibling UI surface found for the new IRM hover behavior. - The remaining issue is semantic reuse: the shared helper currently returns one amount, but the label presents two action families whose math is not equivalent. That should be fixed in the shared utility/label contract so future consumers do not inherit an ambiguous amount.
Bot feedback checked:
- CodeRabbit’s invalid-input test suggestion is reasonable hardening but not a blocker.
- CodeRabbit’s
Number.isFinitenote on APY getters is also a reasonable adjacent guard; I did not treat it as the main blocker because the user-facing delta label issue is the material behavior change in this PR.
|
🚅 Deployed to the euler-lite-pr-609 environment in euler-lite
|
Keep IRM tooltip action labels aligned with the borrow/repay delta math.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed current head 9b7f762274f9fe10c831e02f0fa5137d5433a0c0.
Verdict: Comment — prior Leonard blocker is resolved; I did not find a remaining merge-blocking issue in this pass.
What changed:
- Adds shared utilization delta helpers in
utils/vault-display.ts. - Displays the borrow/repay amount needed to reach a hovered IRM utilization point in the vault overview chart tooltip.
- Adds focused utility coverage for borrow, repay, no-change, and clamped target cases.
Validation performed:
git diff --check origin/development...HEAD— passed.npm run test:run -- tests/utils/vault-display.test.ts— passed, 8 tests.npm run typecheck— passed.npm run build— passed; only existing Nuxt/Rollup chunk/dynamic-import style warnings observed.- Browser visual smoke on the PR preview for
/lend/0xD8b27CF359b7D15710a5BE299AF6e7Bf904984C2?network=1:- desktop: IRM chart rendered and tooltip showed
Borrow: 93.74 WETHat a higher utilization point andRepay: 452.37 WETHat a lower utilization point. - mobile: opened the Vault information modal, scrolled to the IRM chart, and verified the tooltip renders there as well (
Borrow: 86.74 WETH). - Console noise was limited to Reown/WalletConnect-style preview telemetry fetch warnings; no material page errors or route crashes were observed.
- desktop: IRM chart rendered and tooltip showed
Prior finding status:
- The previous blocker was that one amount was labelled as both borrow/withdraw and deposit/repay, even though those denominator-changing actions have different math. The current head narrows the action label to
Borrow/Repay, matching the constant-total-assets borrow-side delta calculation, so that blocker is resolved.
Scalability / maintainability hygiene pass:
- Good: the calculation lives in
utils/vault-display.tswith focused tests, rather than as inline chart-only math. - Sibling utilization consumers found in lend/borrow list cards, vault stats, warnings, and discovery tables display current utilization or use it for sorting/warnings; they do not need this hovered target-utilization delta. I did not find a missed sibling UI surface for the new tooltip behavior.
- The one reasonable follow-up is test hardening: CodeRabbit’s invalid-input test suggestion for non-object vaults and
NaN/Infinitytargets is valid defensive coverage, but the current implementation already returnsnullfor non-finite targets and non-record vaults, so I do not consider it blocking.
Bot feedback checked:
- CodeRabbit’s invalid-input test suggestion is reasonable but non-blocking.
- CodeRabbit’s
Number.isFinitesuggestion for the existing APY helper reads is a valid adjacent hardening idea, but it is not introduced by the utilization tooltip path and should not block this PR.
Smoke coverage: browser visual smoke + mobile smoke on the PR preview. Wallet/signing smoke not run — this is a read-only tooltip/display change.
Screenshots: captured locally for desktop and mobile tooltip states. I am not attaching them here because this review path does not have a safe direct binary upload mechanism to GitHub for PNG artifacts; the evidence above records the visible tooltip text from those screenshots.
3 participants
Summary by CodeRabbit
Release Notes
New Features
Tests