Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo components refactored to optimize re-computation logic. EditCaps.tsx now prevents redundant state updates using memoization and deep equality checks for market cap maps. rebalance-modal.tsx adds deterministic signature helpers and refs to control smart plan recalculation dependencies instead of relying directly on eligibleMarkets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/autovault/components/vault-detail/settings/EditCaps.tsx (1)
151-158:⚠️ Potential issue | 🟠 MajorReplace
Number(bigint)division withformatUnits().
Number()conversion loses precision for large cap values, which corrupts display and later comparisons (inhasChanges). Both the relative and absolute cap conversions are affected. UseformatUnits()from viem instead.Fix
-import { type Address, parseUnits, maxUint128 } from 'viem'; +import { type Address, formatUnits, parseUnits, maxUint128 } from 'viem'; ... - const relativeCap = (Number(relativeCapBigInt) / 1e16).toString(); + const relativeCap = formatUnits(relativeCapBigInt, 16); ... - : (Number(absoluteCapBigInt) / 10 ** vaultAssetDecimals).toString(); + : formatUnits(absoluteCapBigInt, vaultAssetDecimals);🤖 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/EditCaps.tsx` around lines 151 - 158, The current conversions for relativeCap and absoluteCap use Number(bigint) / divisor which loses precision; replace them with viem's formatUnits to preserve full precision: import formatUnits from viem, convert relativeCapBigInt via formatUnits(relativeCapBigInt, 16) (since code used 1e16) and convert absoluteCapBigInt via formatUnits(absoluteCapBigInt, vaultAssetDecimals); keep the existing sentinel handling for absoluteCapBigInt (0n or >= maxUint128 => ''), and ensure parseBigIntOrFallback, relativeCapBigInt, absoluteCapBigInt, maxUint128, and vaultAssetDecimals are used as before so hasChanges comparisons still work.
🧹 Nitpick comments (2)
src/features/positions/components/rebalance/rebalance-modal.tsx (2)
94-104: IncludechainIdin these signatures.They only stay collision-free because this modal already filters to one chain. Adding chain scope here keeps the identity rule local to the helper and avoids quiet cross-chain collisions if either helper gets reused.
Possible tweak
function getSmartPlannerMarketSignature(market: Market): string { return [ + market.morphoBlue.chain.id, market.uniqueKey, market.loanAsset.address.toLowerCase(), market.collateralAsset.address.toLowerCase(), market.oracleAddress?.toLowerCase() ?? '', market.irmAddress?.toLowerCase() ?? '', market.lltv ?? '', market.state.rateAtTarget, ].join(':'); } function getSmartPlannerGroupedPositionSignature(groupedPosition: GroupedPosition): string { - return groupedPosition.markets - .map((position) => `${position.market.uniqueKey}:${position.state.supplyAssets}:${position.state.supplyShares}`) - .sort() - .join('|'); + return `${groupedPosition.chainId}|${groupedPosition.markets + .map((position) => `${position.market.uniqueKey}:${position.state.supplyAssets}:${position.state.supplyShares}`) + .sort() + .join('|')}`; }As per coding guidelines: "All market/token/route identity checks must be chain-scoped using canonical identifiers (chainId + market.uniqueKey or chainId + address)".
Also applies to: 113-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/positions/components/rebalance/rebalance-modal.tsx` around lines 94 - 104, The market signature currently built in getSmartPlannerMarketSignature lacks chain scoping which can cause cross-chain collisions; include the chainId as a canonical identifier (e.g., prepend or include market.chainId as a string) when constructing the signature, normalizing it (toString()) alongside existing fields and keeping existing lowercasing for addresses; also apply the same change to the analogous token/route helper referenced around lines 113-118 (e.g., getSmartPlannerTokenSignature or related helper) so all market/token identity checks are chain-scoped using chainId + uniqueKey/address.
205-214: These signatures are still bypassed by raw deps.
smartPlannerSelectedMarketsSignatureandsmartPlannerConstraintSignatureonly change on content changes, but the effect still also depends onsmartSelectedMarketKeysanddebouncedSmartMaxAllocationBps. BecauseupdateMaxAllocationalways creates a fresh object, a blur that normalizes back to the same value still reruns the planner. If the goal is content-based invalidation, mirror those values behind refs too, or make the setters returnprevon no-op updates.Worth rerunning the hooks exhaustive-deps check if you move either value behind a ref.
Also applies to: 313-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/positions/components/rebalance/rebalance-modal.tsx` around lines 205 - 214, The memoized signatures smartPlannerSelectedMarketsSignature and smartPlannerConstraintSignature are being bypassed by their raw dependencies (smartSelectedMarketKeys and debouncedSmartMaxAllocationBps) because updater functions like updateMaxAllocation create new objects on every call; fix by either (A) mirror smartSelectedMarketKeys and debouncedSmartMaxAllocationBps behind refs and use those refs as the actual dependencies for the useMemo hooks (eg. derive smartPlannerSelectedMarketsSignature from a ref copy instead of the raw Set/number), or (B) change the setter/updateMaxAllocation to perform a no-op when the new value equals the previous value and return the previous state (so the reference doesn't change), and then re-run the hooks exhaustive-deps check to ensure no raw deps remain; apply the same approach to the other affected signatures (see smartPlannerGroupedPosition and eligibleMarkets usages in the file).
🤖 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/components/shared/token-icon.tsx`:
- Around line 41-53: The component is still using token.symbol instead of the
explicit symbol prop; destructure symbol from TokenIcon's props and replace any
uses of token.symbol (notably in the Image alt and any tooltip/title rendering
such as the renderImage/triggerImage usage and the element at the other
occurrence) with the passed-in symbol so an override is respected; keep tokenImg
= token.img as-is but ensure all user-visible labels use the symbol prop (update
TokenIcon's props destructuring and the Image alt and tooltip/title references).
In `@src/features/autovault/components/vault-detail/settings/EditCaps.tsx`:
- Around line 143-149: The current code in EditCaps.tsx calls continue when
hasCompleteEditableMarketMetadata(market) is false, which hides a persisted
on-chain cap; instead, keep the cap in the edit model but mark it as
invalid/read-only and attach an error so it remains visible and non-editable.
Replace the continue branch in the loop that builds caps (referencing market,
cap, cap.capId and hasCompleteEditableMarketMetadata) with logic that pushes a
placeholder entry into the edits array (e.g., { ...cap, readOnly: true,
validationError: 'missing token metadata' }) or toggles a flag on the existing
edit object, ensure the UI rendering for EditCaps shows readOnly rows with the
validationError, and make the save/submit path (the form submit handler) fail
the save if any edit entries have validationError/readOnly so users cannot
accidentally drop or modify that cap.
- Around line 305-310: The code is silently falling back to 0n when
parseBigIntOrFallback fails for adapterCap
(currentRelativeCap/currentAbsoluteCap) — change this so parse failures abort
the save and surface an error: replace the silent-default pattern around
existingCaps?.adapterCap parsing with explicit validation (using the parse
result or an exception from parseBigIntOrFallback) and, if parsing fails, throw
or return an error from the save handler instead of fabricating old* values;
apply the same change for collateralCap and marketCap parsing paths so malformed
persisted cap values stop the transaction flow and show a user-facing/throwable
error rather than defaulting to 0n.
---
Outside diff comments:
In `@src/features/autovault/components/vault-detail/settings/EditCaps.tsx`:
- Around line 151-158: The current conversions for relativeCap and absoluteCap
use Number(bigint) / divisor which loses precision; replace them with viem's
formatUnits to preserve full precision: import formatUnits from viem, convert
relativeCapBigInt via formatUnits(relativeCapBigInt, 16) (since code used 1e16)
and convert absoluteCapBigInt via formatUnits(absoluteCapBigInt,
vaultAssetDecimals); keep the existing sentinel handling for absoluteCapBigInt
(0n or >= maxUint128 => ''), and ensure parseBigIntOrFallback,
relativeCapBigInt, absoluteCapBigInt, maxUint128, and vaultAssetDecimals are
used as before so hasChanges comparisons still work.
---
Nitpick comments:
In `@src/features/positions/components/rebalance/rebalance-modal.tsx`:
- Around line 94-104: The market signature currently built in
getSmartPlannerMarketSignature lacks chain scoping which can cause cross-chain
collisions; include the chainId as a canonical identifier (e.g., prepend or
include market.chainId as a string) when constructing the signature, normalizing
it (toString()) alongside existing fields and keeping existing lowercasing for
addresses; also apply the same change to the analogous token/route helper
referenced around lines 113-118 (e.g., getSmartPlannerTokenSignature or related
helper) so all market/token identity checks are chain-scoped using chainId +
uniqueKey/address.
- Around line 205-214: The memoized signatures
smartPlannerSelectedMarketsSignature and smartPlannerConstraintSignature are
being bypassed by their raw dependencies (smartSelectedMarketKeys and
debouncedSmartMaxAllocationBps) because updater functions like
updateMaxAllocation create new objects on every call; fix by either (A) mirror
smartSelectedMarketKeys and debouncedSmartMaxAllocationBps behind refs and use
those refs as the actual dependencies for the useMemo hooks (eg. derive
smartPlannerSelectedMarketsSignature from a ref copy instead of the raw
Set/number), or (B) change the setter/updateMaxAllocation to perform a no-op
when the new value equals the previous value and return the previous state (so
the reference doesn't change), and then re-run the hooks exhaustive-deps check
to ensure no raw deps remain; apply the same approach to the other affected
signatures (see smartPlannerGroupedPosition and eligibleMarkets usages in the
file).
🪄 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: 46b170d9-e57b-4e30-adc4-dec21bd2fb2d
📒 Files selected for processing (3)
src/components/shared/token-icon.tsxsrc/features/autovault/components/vault-detail/settings/EditCaps.tsxsrc/features/positions/components/rebalance/rebalance-modal.tsx
Comment on lines
+41
to
+53
| const tokenImg = token.img; | ||
| const renderImage = () => ( | ||
| <Image | ||
| className="rounded-full" | ||
| src={token.img} | ||
| src={tokenImg} | ||
| alt={token.symbol} | ||
| width={width} | ||
| height={height} | ||
| style={{ opacity }} | ||
| unoptimized | ||
| /> | ||
| ); | ||
| const triggerImage = renderImage(); |
There was a problem hiding this comment.
Use the passed symbol here too.
This helper still reads token.symbol, so callers with an intentional symbol override will keep seeing the registry symbol in the image alt text and tooltip title.
✏️ Suggested change
if (token?.img) {
const tokenImg = token.img;
+ const displaySymbol = symbol ?? token.symbol;
const renderImage = () => (
<Image
className="rounded-full"
src={tokenImg}
- alt={token.symbol}
+ alt={displaySymbol}
width={width}
height={height}
style={{ opacity }}
unoptimized
/>
);
const triggerImage = renderImage();
- const title = customTooltipTitle ?? token.symbol;
+ const title = customTooltipTitle ?? displaySymbol;You’ll also need to destructure symbol from TokenIcon’s props.
Based on learnings: The TokenIcon component accepts a symbol prop, and intentional display-symbol differences should be captured as explicit overrides instead of silent drift in token metadata.
Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/shared/token-icon.tsx` around lines 41 - 53, The component is
still using token.symbol instead of the explicit symbol prop; destructure symbol
from TokenIcon's props and replace any uses of token.symbol (notably in the
Image alt and any tooltip/title rendering such as the renderImage/triggerImage
usage and the element at the other occurrence) with the passed-in symbol so an
override is respected; keep tokenImg = token.img as-is but ensure all
user-visible labels use the symbol prop (update TokenIcon's props destructuring
and the Image alt and tooltip/title references).
Comment on lines
+143
to
+149
| if (!hasCompleteEditableMarketMetadata(market)) { | ||
| console.error('[EditCaps] skipping market cap with incomplete market metadata', { | ||
| marketUniqueKey: market.uniqueKey, | ||
| capId: cap.capId, | ||
| }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Don't hide persisted caps here.
This continue drops a live on-chain market cap from the edit model whenever metadata is incomplete. The table then shows fewer caps than actually exist, and a later save can never remove or repair that hidden cap. Better to block editing with a visible error, or keep a read-only placeholder row for the bad market.
As per coding guidelines: Fail closed for any market whose required token metadata cannot be resolved safely.
🤖 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/EditCaps.tsx` around
lines 143 - 149, The current code in EditCaps.tsx calls continue when
hasCompleteEditableMarketMetadata(market) is false, which hides a persisted
on-chain cap; instead, keep the cap in the edit model but mark it as
invalid/read-only and attach an error so it remains visible and non-editable.
Replace the continue branch in the loop that builds caps (referencing market,
cap, cap.capId and hasCompleteEditableMarketMetadata) with logic that pushes a
placeholder entry into the edits array (e.g., { ...cap, readOnly: true,
validationError: 'missing token metadata' }) or toggles a flag on the existing
edit object, ensure the UI rendering for EditCaps shows readOnly rows with the
validationError, and make the save/submit path (the form submit handler) fail
the save if any edit entries have validationError/readOnly so users cannot
accidentally drop or modify that cap.
Comment on lines
+305
to
+310
| const currentRelativeCap = existingCaps?.adapterCap | ||
| ? parseBigIntOrFallback(existingCaps.adapterCap.relativeCap, 0n, `adapter:${existingCaps.adapterCap.capId}:relativeCap`) | ||
| : 0n; | ||
| const currentAbsoluteCap = existingCaps?.adapterCap | ||
| ? parseBigIntOrFallback(existingCaps.adapterCap.absoluteCap, 0n, `adapter:${existingCaps.adapterCap.capId}:absoluteCap`) | ||
| : 0n; |
There was a problem hiding this comment.
Abort save on malformed persisted cap values.
Defaulting a bad persisted adapter cap to 0n here means we still build a write payload with synthetic old* values. The same pattern repeats for collateral and market caps below. In this path, parse failure should stop the save and surface an error instead of fabricating fallback state.
As per coding guidelines: Guard null/undefined/stale API and contract fields in all tx-critical paths so malformed data degrades safely.
🤖 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/EditCaps.tsx` around
lines 305 - 310, The code is silently falling back to 0n when
parseBigIntOrFallback fails for adapterCap
(currentRelativeCap/currentAbsoluteCap) — change this so parse failures abort
the save and surface an error: replace the silent-default pattern around
existingCaps?.adapterCap parsing with explicit validation (using the parse
result or an exception from parseBigIntOrFallback) and, if parsing fails, throw
or return an error from the save handler instead of fabricating old* values;
apply the same change for collateralCap and marketCap parsing paths so malformed
persisted cap values stop the transaction flow and show a user-facing/throwable
error rather than defaulting to 0n.
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 by CodeRabbit
Bug Fixes
Refactor