feat: add vault and adapter identity display#524
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a vault account identity API and hook, surfaces vault metadata in AccountIdentity, gates positions queries for vault pages, adds vault-specific UI components (AccountVaultInfo, VaultManagedExposures), and appends validation docs. ChangesVault Identity & Positions UI Integration
Sequence DiagramsequenceDiagram
participant User
participant PositionsView
participant useVaultAccountIdentity as Hook
participant VaultRegistry
participant AccountIdentity
participant AccountVaultInfo
User->>PositionsView: open account positions
PositionsView->>Hook: useVaultAccountIdentity(account)
Hook->>VaultRegistry: getVaultAccountIdentity(address, chainId?)
VaultRegistry->>VaultRegistry: normalize & classify address
VaultRegistry->>Hook: VaultAccountIdentity | undefined
PositionsView->>AccountIdentity: render with vaultIdentity
AccountIdentity->>AccountIdentity: prefer metadata image, show tags/links
PositionsView->>AccountVaultInfo: render vault-specific info (if any)
PositionsView->>PositionsView: gate native sections by identity.kind
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labelsfeature request, ui 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive identity system for vaults and adapters, including new UI components like AccountVaultInfo and VaultManagedExposures. The VaultRegistryContext has been expanded to support detailed VaultAccountIdentity metadata, and the Positions view now conditionally renders sections based on whether the account is a V2 vault. A critical issue was identified in the Button component where the disabled prop is not correctly passed to the Radix Slot when asChild is true, which would prevent underlying elements from receiving the disabled state.
| if (asChild) { | ||
| return ( | ||
| <Slot | ||
| className={buttonClassName} | ||
| ref={ref} | ||
| {...props} | ||
| aria-disabled={isDisabled} | ||
| > | ||
| {children} | ||
| </Slot> | ||
| ); | ||
| } |
There was a problem hiding this comment.
When asChild is true, the disabled prop is not being passed to the Slot component. Since disabled was destructured from the component arguments, it is no longer part of the props object spread onto the Slot. This results in the underlying child element (like a native button) not receiving the disabled state, even if isLoading is true or the disabled prop was explicitly provided. While the new validation rule 96 in VALIDATIONS.md correctly advises against appending siblings like spinners to a slotted child, the functional disabled state must still be synchronized.
| if (asChild) { | |
| return ( | |
| <Slot | |
| className={buttonClassName} | |
| ref={ref} | |
| {...props} | |
| aria-disabled={isDisabled} | |
| > | |
| {children} | |
| </Slot> | |
| ); | |
| } | |
| if (asChild) { | |
| return ( | |
| <Slot | |
| className={buttonClassName} | |
| ref={ref} | |
| {...props} | |
| disabled={isDisabled} | |
| aria-disabled={isDisabled} | |
| > | |
| {children} | |
| </Slot> | |
| ); | |
| } |
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: 1
🧹 Nitpick comments (2)
src/features/positions/positions-view.tsx (1)
157-165: ⚡ Quick winRender managed exposures for all
vault-v2pages.
VaultManagedExposuresalready resolves adapters from vault data. Gating onadapterAddresscan hide exposures when fallback adapter info is absent.Proposed change
- {accountVaultIdentity?.kind === 'vault-v2' && accountVaultIdentity.adapterAddress && ( + {accountVaultIdentity?.kind === 'vault-v2' && ( <VaultManagedExposures vaultAddress={account as Address} fallbackAdapterAddress={accountVaultIdentity.adapterAddress} fallbackAdapterType={accountVaultIdentity.adapterType} chainId={accountVaultIdentity.chainId} period={period} /> )}🤖 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 `@src/features/positions/positions-view.tsx` around lines 157 - 165, The current render is gating VaultManagedExposures on accountVaultIdentity.adapterAddress which hides managed exposures when fallback adapter info is missing; change the condition to render VaultManagedExposures whenever accountVaultIdentity?.kind === 'vault-v2' (i.e., remove the adapterAddress truthiness check) so the component can resolve adapters from vault data itself; keep passing vaultAddress (account as Address) and the optional fallback props (fallbackAdapterAddress, fallbackAdapterType, chainId, period) so VaultManagedExposures can use fallbacks when available.src/hooks/useUserPositionsSummaryData.ts (1)
76-93: ⚡ Quick winGuard transaction cache operations behind
activeUser.When the hook is disabled, this still runs merge/reconcile with
userAddress: undefined. That can create noisy cache writes on the disabled path. Short-circuit both paths when no active user.Proposed change
const mergedTransactions = useMemo( - () => - mergeUserTransactionsWithRecentCache({ - userAddress: activeUser, - chainIds: uniqueChainIds, - apiTransactions: txData?.items ?? [], - }), + () => + activeUser + ? mergeUserTransactionsWithRecentCache({ + userAddress: activeUser, + chainIds: uniqueChainIds, + apiTransactions: txData?.items ?? [], + }) + : [], [activeUser, uniqueChainIds, txData?.items], ); useEffect(() => { + if (!activeUser) return; reconcileUserTransactionHistoryCache({ userAddress: activeUser, chainIds: uniqueChainIds, apiTransactions: txData?.items ?? [], }); }, [activeUser, uniqueChainIds, txData?.items]);🤖 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 `@src/hooks/useUserPositionsSummaryData.ts` around lines 76 - 93, The merge and reconcile calls run even when activeUser is undefined; guard both by short-circuiting when activeUser is falsy: in the useMemo that computes mergedTransactions, return an empty array (or appropriate default) immediately if activeUser is not set instead of calling mergeUserTransactionsWithRecentCache; similarly, in the useEffect that calls reconcileUserTransactionHistoryCache, early-return (do nothing) when activeUser is falsy so no cache writes occur; keep uniqueChainIds and txData?.items in the dependency arrays and reference mergeUserTransactionsWithRecentCache and reconcileUserTransactionHistoryCache to locate the two places to change.
🤖 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 `@src/components/ui/button.tsx`:
- Around line 94-105: When asChild is true, the Slot rendering uses
aria-disabled but not the disabled attribute so native button children remain
interactive; update the Slot invocation inside the asChild branch to forward the
disabled attribute (disabled={isDisabled}) so it reaches native button elements,
and ensure the resolved className (buttonClassName) also conditionally includes
pointer-events-none when isDisabled is true to cover non-button children that
rely on CSS for disabling; reference the asChild branch where Slot is rendered
and the isDisabled and buttonClassName symbols.
---
Nitpick comments:
In `@src/features/positions/positions-view.tsx`:
- Around line 157-165: The current render is gating VaultManagedExposures on
accountVaultIdentity.adapterAddress which hides managed exposures when fallback
adapter info is missing; change the condition to render VaultManagedExposures
whenever accountVaultIdentity?.kind === 'vault-v2' (i.e., remove the
adapterAddress truthiness check) so the component can resolve adapters from
vault data itself; keep passing vaultAddress (account as Address) and the
optional fallback props (fallbackAdapterAddress, fallbackAdapterType, chainId,
period) so VaultManagedExposures can use fallbacks when available.
In `@src/hooks/useUserPositionsSummaryData.ts`:
- Around line 76-93: The merge and reconcile calls run even when activeUser is
undefined; guard both by short-circuiting when activeUser is falsy: in the
useMemo that computes mergedTransactions, return an empty array (or appropriate
default) immediately if activeUser is not set instead of calling
mergeUserTransactionsWithRecentCache; similarly, in the useEffect that calls
reconcileUserTransactionHistoryCache, early-return (do nothing) when activeUser
is falsy so no cache writes occur; keep uniqueChainIds and txData?.items in the
dependency arrays and reference mergeUserTransactionsWithRecentCache and
reconcileUserTransactionHistoryCache to locate the two places to change.
🪄 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: a6c34478-a38a-45d3-98e8-1e1995780ecd
📒 Files selected for processing (11)
docs/VALIDATIONS.mdsrc/components/shared/account-identity.tsxsrc/components/ui/button.tsxsrc/contexts/VaultRegistryContext.tsxsrc/features/positions/components/account-vault-info.tsxsrc/features/positions/components/adapter-managed-exposure.tsxsrc/features/positions/positions-view.tsxsrc/hooks/useAddressLabel.tssrc/hooks/useUserPositionsSummaryData.tssrc/hooks/useVaultAccountIdentity.tssrc/utils/vaults.ts
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive identity system for vaults and adapters, enhancing the AccountIdentity component with metadata images, tooltips, and specific tags for different entity types. It adds new components like AccountVaultInfo and VaultManagedExposures to the positions view, allowing for a more detailed display of vault-related data and managed exposures. Additionally, the Button component was refactored for better Radix Slot integration, and the VaultRegistryContext was expanded to support the new identity structure. A performance improvement was suggested in the VaultRegistryContext to use .find() instead of .filter().map() when searching for aliases to avoid unnecessary array iterations.
| const adapterAlias = chainId | ||
| ? adapterAliasesByScopedAddress.get(getAddressKey(normalizedAddress, chainId)) | ||
| : adapterAliases.filter((candidate) => candidate.address.toLowerCase() === normalizedAddress).map(toAdapterAddressAlias)[0]; | ||
|
|
||
| if (adapterAlias) { | ||
| return adapterAliasToAdapterIdentity(adapterAlias, getVaultByAddress(toAddress(adapterAlias.vaultAddress), adapterAlias.chainId)); | ||
| } | ||
|
|
||
| if (!chainId) { | ||
| return undefined; | ||
| const vaultAliases = chainId | ||
| ? (adapterAliasesByVaultScopedAddress.get(getAddressKey(normalizedAddress, chainId)) ?? []) | ||
| : adapterAliases.filter((candidate) => candidate.vaultAddress.toLowerCase() === normalizedAddress).map(toAdapterAddressAlias); | ||
|
|
||
| const vaultAlias = vaultAliases[0]; | ||
| if (vaultAlias) { | ||
| return adapterAliasToVaultIdentity(vaultAlias, getVaultByAddress(address, vaultAlias.chainId)); | ||
| } |
There was a problem hiding this comment.
The current implementation uses .filter().map()[0] and .filter().map() which iterates over the entire adapterAliases array even when only the first match is needed. Using .find() is more efficient as it stops iteration once a match is found. Additionally, since only the first element of vaultAliases is used, the logic can be simplified to directly retrieve the first matching alias from the map or the array.
const adapterAlias = chainId
? adapterAliasesByScopedAddress.get(getAddressKey(normalizedAddress, chainId))
: (() => {
const match = adapterAliases.find((c) => c.address.toLowerCase() === normalizedAddress);
return match ? toAdapterAddressAlias(match) : undefined;
})();
if (adapterAlias) {
return adapterAliasToAdapterIdentity(adapterAlias, getVaultByAddress(toAddress(adapterAlias.vaultAddress), adapterAlias.chainId));
}
const vaultAlias = chainId
? adapterAliasesByVaultScopedAddress.get(getAddressKey(normalizedAddress, chainId))?.[0]
: (() => {
const match = adapterAliases.find((c) => c.vaultAddress.toLowerCase() === normalizedAddress);
return match ? toAdapterAddressAlias(match) : undefined;
})();
if (vaultAlias) {
return adapterAliasToVaultIdentity(vaultAlias, getVaultByAddress(address, vaultAlias.chainId));
}
antoncoding
changed the title
starksama
left a comment
starksama
left a comment
There was a problem hiding this comment.
Request changes. The Button/Slot issue is addressed and the remaining Gemini .find() note is non-blocking, but vault-v2 page gating currently treats unresolved vault metadata as a native account. That can render/fetch the native account sections on cold load before adapter aliases resolve, which is exactly the state this PR is trying to avoid.
| const { addVisitedAddress, toggleAddressBookmark, isAddressBookmarked } = usePortfolioBookmarks(); | ||
| const accountVaultIdentity = useVaultAccountIdentity(account); | ||
| const isV2VaultPage = accountVaultIdentity?.kind === 'vault-v2'; | ||
| const showNativeAccountSections = !isV2VaultPage; |
There was a problem hiding this comment.
This makes an unresolved identity equivalent to a non-vault account. On a direct/cold load, useVaultAccountIdentity(account) can be undefined while the vault registry/adapter aliases are still loading, so showNativeAccountSections becomes true and the page starts fetching/rendering native vault-address positions/vault tables before later flipping to the vault-v2 view. That violates the new validation rule about metadata-backed display guards not treating missing metadata as a negative match, and it can briefly show the exact native-account sections this PR is meant to suppress. The guard needs a readiness state from VaultRegistryContext/useVaultAccountIdentity and should hold the native sections in loading/neutral state until vault identity can actually be evaluated.
starksama
left a comment
starksama
left a comment
There was a problem hiding this comment.
Approved after follow-up fix. Native account sections now stay behind vault identity readiness, so vault-v2 pages no longer treat unresolved metadata as a negative match during cold load.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust identity system for vaults and adapters, adding the VaultAccountIdentity type and updating the VaultRegistryContext to support detailed metadata resolution. Key UI enhancements include new components for vault information and adapter-managed exposures, alongside a refactored AccountIdentity component. Review feedback suggests optimizing performance in the registry context by using pre-calculated maps for address lookups and simplifying the Button component's asChild logic by passing the disabled prop directly to the Radix Slot instead of using cloneElement.
| const getVaultAccountIdentity = useCallback( | ||
| (address: Address, chainId?: number): VaultAccountIdentity | undefined => { | ||
| const normalizedAddress = address.toLowerCase(); | ||
|
|
||
| const adapterAlias = chainId | ||
| ? adapterAliasesByScopedAddress.get(getAddressKey(normalizedAddress, chainId)) | ||
| : toOptionalAdapterAddressAlias(adapterAliases.find((candidate) => candidate.address.toLowerCase() === normalizedAddress)); | ||
|
|
||
| if (adapterAlias) { | ||
| return adapterAliasToAdapterIdentity(adapterAlias, getVaultByAddress(toAddress(adapterAlias.vaultAddress), adapterAlias.chainId)); | ||
| } | ||
|
|
||
| if (!chainId) { | ||
| return undefined; | ||
| const vaultAlias = chainId | ||
| ? adapterAliasesByVaultScopedAddress.get(getAddressKey(normalizedAddress, chainId))?.[0] | ||
| : toOptionalAdapterAddressAlias(adapterAliases.find((candidate) => candidate.vaultAddress.toLowerCase() === normalizedAddress)); | ||
| if (vaultAlias) { | ||
| return adapterAliasToVaultIdentity(vaultAlias, getVaultByAddress(address, vaultAlias.chainId)); | ||
| } | ||
|
|
||
| const adapterAlias = adapterAliasesByScopedAddress.get(getAddressKey(address, chainId)); | ||
| if (!adapterAlias) { | ||
| const vault = getVaultByAddress(address, chainId); | ||
| if (vault) { | ||
| return morphoVaultToIdentity(vault); | ||
| } | ||
|
|
||
| return undefined; | ||
| }, | ||
| [adapterAliases, adapterAliasesByScopedAddress, adapterAliasesByVaultScopedAddress, getVaultByAddress], | ||
| ); |
There was a problem hiding this comment.
The getVaultAccountIdentity function performs O(N) lookups using adapterAliases.find() when chainId is not provided (lines 162 and 170). Since this registry is a central context used for identity resolution across the app, these linear searches could impact performance as the number of adapters grows. Consider pre-calculating maps keyed by address and vaultAddress (ignoring chainId) within useMemo to ensure O(1) lookups in all cases.
| if (asChild) { | ||
| const slottedChildren = | ||
| isDisabled && isValidElement<{ disabled?: boolean }>(children) | ||
| ? cloneElement(children, { disabled: true }) | ||
| : children; | ||
|
|
||
| return ( | ||
| <Slot | ||
| className={buttonClassName} | ||
| ref={ref} | ||
| {...props} | ||
| aria-disabled={isDisabled} | ||
| > | ||
| {slottedChildren} | ||
| </Slot> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The asChild implementation uses cloneElement to manually inject the disabled prop. It's more idiomatic and cleaner to pass the disabled prop directly to the Slot component. Radix UI's Slot will handle the prop merging onto the child element automatically. This also simplifies the code by removing the need for manual element validation and cloning.
if (asChild) {
return (
<Slot
className={buttonClassName}
ref={ref}
{...props}
disabled={isDisabled}
aria-disabled={isDisabled}
>
{children}
</Slot>
);
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive system for identifying and displaying vault and adapter account identities. Key changes include updating the AccountIdentity component to support vault-specific metadata and images, refactoring the Button component for better asChild support, and expanding the VaultRegistryContext to manage account identities. New components like AccountVaultInfo and VaultManagedExposures were added to the positions view to provide detailed vault information and exposure data. Review feedback suggests improving type safety for asset addresses, enhancing accessibility for interactive elements by using semantic buttons, and considering performance optimizations for rendering multiple adapter exposures.
| vaultAddress: Address; | ||
| adapterAddress?: Address; | ||
| adapterType?: string; | ||
| assetAddress?: string; |
There was a problem hiding this comment.
For consistency and type safety, assetAddress should use the Address type instead of string. This ensures that downstream components, such as TokenIcon in AccountVaultInfo, receive the correctly formatted address type expected by their props.
| assetAddress?: string; | |
| assetAddress?: Address; |
| <span | ||
| className="inline-flex cursor-pointer items-center rounded-sm bg-hovered px-2 py-1 text-xs text-secondary transition-colors hover:text-primary" | ||
| aria-label={`Open actions for ${label}`} | ||
| > | ||
| {label} | ||
| </span> |
There was a problem hiding this comment.
The span element used as a trigger for the AccountActionsPopover is interactive but lacks proper accessibility attributes. It should be focusable and indicate its role to assistive technologies. Using a <button> element is more idiomatic and provides these features natively.
<button
type="button"
className="inline-flex cursor-pointer items-center rounded-sm bg-hovered px-2 py-1 text-xs text-secondary transition-colors hover:text-primary"
aria-label={`Open actions for ${label}`}
>
{label}
</button>
| {adapters.map((adapter) => ( | ||
| <AdapterExposure | ||
| key={`${chainId}:${adapter.address}`} | ||
| adapterAddress={adapter.address} | ||
| adapterType={adapter.adapterType} | ||
| chainId={chainId} | ||
| period={period} | ||
| /> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
Rendering multiple AdapterExposure components in a loop, each invoking the useUserPositionsSummaryData hook, can lead to a significant number of concurrent network requests and performance degradation if a vault has many adapters. Each hook instance triggers multiple queries (positions, blocks, transactions, snapshots). Consider if there's a way to batch these data fetches or if the UI should only load them on demand (e.g., via an accordion or lazy loading) to improve scalability.
There was a problem hiding this comment.
Not changing this in this PR. The current vault adapter list comes from the vault metadata and is expected to be small; the UX requirement here is to show each adapter exposure inline when a vault has several adapters. Adding lazy loading or an accordion now would add state and hide the relationship we are trying to make explicit. If we see vaults with many adapters in production data, batching/lazy loading should be handled as a dedicated follow-up.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust identity system for vaults and adapters, enabling the application to differentiate between native accounts, Morpho vaults, and vault adapters. Key enhancements include the definition of VaultAccountIdentity within the VaultRegistryContext, new UI components like AccountVaultInfo and VaultManagedExposures for displaying vault-specific metadata, and logic in the Positions view to conditionally render content based on the account type. Review feedback highlights opportunities to optimize identity lookups by avoiding linear searches and suggests refining the loading states to prevent layout shifts and jarring transitions when navigating to vault-specific pages.
|
|
||
| const adapterAlias = chainId | ||
| ? adapterAliasesByScopedAddress.get(getAddressKey(normalizedAddress, chainId)) | ||
| : toOptionalAdapterAddressAlias(adapterAliases.find((candidate) => candidate.address.toLowerCase() === normalizedAddress)); |
There was a problem hiding this comment.
When chainId is not provided, getVaultAccountIdentity performs a linear search over adapterAliases. While likely fine for small datasets, this could become a performance bottleneck if the registry grows or if this function is called frequently (e.g., in a list). Consider maintaining an additional Map keyed by address only to allow O(1) lookups even when chainId is missing.
| } = usePortfolioValue(marketPositions, vaults); | ||
|
|
||
| const loading = isMarketsLoading || isPositionsLoading; | ||
| const loading = !canEvaluateVaultIdentity || (showNativeAccountSections && (isMarketsLoading || isPositionsLoading)); |
There was a problem hiding this comment.
The loading state logic for V2 vault pages might lead to a jarring user experience. Currently, loading becomes false as soon as the vault registry is evaluated (line 40), which causes the global LoadingScreen to disappear. However, the VaultManagedExposures component then performs its own asynchronous data fetch (useVaultV2Data). During this time, the page will appear empty before the exposure data suddenly pops in. Consider including the loading state of the vault-specific data in this top-level loading variable to ensure a continuous loading state until the primary content is ready.
| if (adapters.length === 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Returning null when adapters.length === 0 while the data is still loading (i.e., vaultDataQuery.isLoading) contributes to the layout shift mentioned in the Positions view. It would be better to return a loading skeleton or at least preserve the section header to indicate that content is being fetched.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive vault identity system to distinguish between Morpho vaults, V2 vaults, and adapters. Key changes include the introduction of the VaultAccountIdentity type in the registry context, new components for displaying vault-specific metadata and managed exposures, and enhancements to the AccountIdentity component to support metadata images and specialized badges. The Button component was also updated to improve disabled state handling when used with Radix Slot. Feedback suggests a more idiomatic approach for the Button component by leveraging Radix Slot's automatic prop merging instead of using cloneElement to pass the disabled attribute.
| if (asChild) { | ||
| const slottedChildren = | ||
| isDisabled && isValidElement<{ disabled?: boolean }>(children) | ||
| ? cloneElement(children, { disabled: true }) | ||
| : children; | ||
|
|
||
| return ( | ||
| <Slot | ||
| className={buttonClassName} | ||
| ref={ref} | ||
| {...props} | ||
| aria-disabled={isDisabled} | ||
| > | ||
| {slottedChildren} | ||
| </Slot> | ||
| ); |
There was a problem hiding this comment.
When using Radix Slot with asChild, it is more idiomatic to pass props directly to the Slot component rather than using cloneElement. Radix Slot handles prop merging automatically, ensuring that the child element receives the disabled and aria-disabled attributes correctly. This approach is cleaner and avoids potential issues with cloneElement on complex children. Additionally, passing disabled to the Slot ensures that if the child is a native button, it correctly receives the HTML disabled attribute.
if (asChild) {
return (
<Slot
className={buttonClassName}
ref={ref}
{...props}
disabled={isDisabled}
aria-disabled={isDisabled}
>
{children}
</Slot>
);
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive identity system for accounts, specifically distinguishing between standard accounts, Morpho vaults, and vault adapters. Key updates include an expanded VaultRegistryContext for identity resolution, a refactored AccountIdentity component with support for metadata images and entity-specific badges, and new components for displaying vault-specific information and managed exposures. The Positions view was also updated to conditionally render these new sections based on the account type. Feedback was provided regarding the duplication of loading and empty state UI blocks in the VaultManagedExposures component, suggesting the use of shared components for consistency.
| chainId={chainId} | ||
| /> | ||
|
|
||
| {isPositionsLoading && <div className="rounded bg-surface px-4 py-6 text-sm text-secondary shadow-sm">Loading...</div>} |
There was a problem hiding this comment.
The loading and empty state UI blocks are repeated multiple times with identical styling in this file (lines 73, 97, 167). To adhere to the project's validation rules (Universal Validation rule 19 and UI rule 91), please consider using the established LoadingScreen and EmptyScreen components or extracting a shared component for these inline status messages to avoid duplication and ensure UI consistency.
References
- The implementation uses established UI components, util functions, and project patterns where possible. Do not declare ad hoc functions, always try to expand existing utilities or shared components first.
- Avoid repeated large UI blocks; extract or reuse only when it reduces real duplication.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive identity resolution system for vaults and adapters, enhancing the account view with specific metadata, icons, and relationship links. Key changes include the addition of VaultRegistryContext to manage vault identities, a new AccountVaultInfo component for displaying vault-specific details, and the VaultManagedExposures feature to show positions held by linked adapters. Feedback focuses on refining the Button component's handling of disabled states and asChild props to avoid redundant classes and invalid HTML. Additionally, there are concerns regarding the non-deterministic nature of address-to-identity lookups when a chain ID is missing, and the potential performance impact of parallel data fetching for multiple adapters within the exposure view.
| const buttonClassName = cn( | ||
| buttonVariants({ variant, size, radius, fullWidth, isLoading, className }), | ||
| isDisabled && 'pointer-events-none cursor-not-allowed opacity-50', | ||
| ); |
There was a problem hiding this comment.
The manual application of pointer-events-none cursor-not-allowed opacity-50 is redundant for native <button> elements as these are already handled by the disabled: pseudo-class in buttonVariants (line 9). These classes should only be applied when asChild is true. Additionally, note that pointer-events-none prevents the cursor-not-allowed from appearing, which might be confusing for users expecting a visual cue for the disabled state.
| const buttonClassName = cn( | |
| buttonVariants({ variant, size, radius, fullWidth, isLoading, className }), | |
| isDisabled && 'pointer-events-none cursor-not-allowed opacity-50', | |
| ); | |
| const buttonClassName = cn( | |
| buttonVariants({ variant, size, radius, fullWidth, isLoading, className }), | |
| asChild && isDisabled && 'pointer-events-none cursor-not-allowed opacity-50', | |
| ); |
| disabled: isDisabled, | ||
| 'aria-disabled': isDisabled, |
There was a problem hiding this comment.
When asChild is true and the child is a Link (which renders an <a> tag), passing the disabled prop results in invalid HTML. While aria-disabled is correct for accessibility on non-form elements, the disabled attribute should ideally only be passed to elements that support it (like <button>).
| const adapterAliasesByAddress = useMemo(() => { | ||
| const lookup = new Map<string, AdapterAddressAlias>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| const key = adapterAlias.address.toLowerCase(); | ||
| if (!lookup.has(key)) { | ||
| lookup.set(key, toAdapterAddressAlias(adapterAlias)); | ||
| } | ||
| } | ||
| return lookup; | ||
| }, [adapterAliases]); | ||
|
|
||
| const adapterAliasesByVaultAddress = useMemo(() => { | ||
| const lookup = new Map<string, AdapterAddressAlias>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| const key = adapterAlias.vaultAddress.toLowerCase(); | ||
| if (!lookup.has(key)) { | ||
| lookup.set(key, toAdapterAddressAlias(adapterAlias)); | ||
| } | ||
| } | ||
| return lookup; | ||
| }, [adapterAliases]); |
There was a problem hiding this comment.
Both adapterAliasesByAddress and adapterAliasesByVaultAddress maps only store the first alias encountered for a given address, ignoring potential duplicates on different chains. This makes identity resolution non-deterministic and potentially incorrect when chainId is not provided. If an address is used on multiple chains with different metadata, the registry will always return the metadata of the first one found in the list. Consider storing a list of aliases per address or ensuring that lookups without a chainId are handled more robustly.
| const { isPositionsLoading, positions, refetch, isRefetching, isEarningsLoading, actualBlockData, transactions, snapshotsByChain } = | ||
| useUserPositionsSummaryData(adapterAddress, period, chainIds); |
There was a problem hiding this comment.
Calling useUserPositionsSummaryData inside a loop for each adapter can lead to a significant number of parallel network requests and heavy client-side processing if a vault has many adapters. Consider if the underlying data fetching can be batched or if a more optimized approach for multi-account position summaries is available to improve performance and reduce API load.
There was a problem hiding this comment.
Resolved the actionable review items in e122944. For this adapter-fetching concern: there is not currently a shared multi-account positions-summary query to reuse, and changing that data contract would be larger than this PR. The component already deduplicates adapter addresses and preserves the requested inline exposure view. If production data shows vaults with many adapters, that should be handled with a dedicated batched positions query rather than adding lazy/accordion behavior here.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive "Vault Identity" system to differentiate between standard accounts, Morpho vaults, and vault adapters. Key changes include a refactored AccountIdentity component with metadata image support, new context logic for resolving account identities, and specialized UI components like AccountVaultInfo and VaultManagedExposures for detailed vault/adapter views. Additionally, the Button component was updated for better Radix Slot compatibility. Feedback was provided regarding performance optimizations in VaultRegistryContext.tsx, specifically recommending the use of .push() instead of spread syntax within loops to avoid
| const lookup = new Map<string, AdapterAddressAlias[]>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| const key = adapterAlias.address.toLowerCase(); | ||
| lookup.set(key, [...(lookup.get(key) ?? []), toAdapterAddressAlias(adapterAlias)]); |
There was a problem hiding this comment.
Avoid using spread syntax in accumulators inside loops as it leads to .push() instead, which is more efficient and consistent with the implementation on line 155.
const existing = lookup.get(key) ?? [];
existing.push(toAdapterAddressAlias(adapterAlias));
lookup.set(key, existing);
References
- Avoid spread syntax in accumulators inside loops to maintain performance and adhere to the project's validation rules.
| const lookup = new Map<string, AdapterAddressAlias[]>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| const key = adapterAlias.vaultAddress.toLowerCase(); | ||
| lookup.set(key, [...(lookup.get(key) ?? []), toAdapterAddressAlias(adapterAlias)]); |
There was a problem hiding this comment.
Avoid using spread syntax in accumulators inside loops. Using .push() is more efficient and follows the pattern established elsewhere in this file.
const existing = lookup.get(key) ?? [];
existing.push(toAdapterAddressAlias(adapterAlias));
lookup.set(key, existing);
References
- Avoid spread syntax in accumulators inside loops.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust identity system for vaults and adapters, enhancing account displays with metadata images, specialized badges, and contextual links. Key changes include the addition of the VaultAccountIdentity type, new components for vault-specific information and managed exposures, and improvements to the Button component's asChild behavior to correctly handle disabled states. Feedback was provided to optimize the VaultRegistryContext by consolidating multiple memoized lookup maps into a single iteration to improve performance and code cleanliness.
| const adapterAliasesByScopedAddress = useMemo(() => { | ||
| const lookup = new Map<string, AdapterAddressAlias>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| lookup.set(getAddressKey(adapterAlias.address, adapterAlias.chainId), { | ||
| vaultAddress: adapterAlias.vaultAddress, | ||
| vaultName: adapterAlias.vaultName, | ||
| adapterType: adapterAlias.adapterType, | ||
| }); | ||
| lookup.set(getAddressKey(adapterAlias.address, adapterAlias.chainId), toAdapterAddressAlias(adapterAlias)); | ||
| } | ||
| return lookup; | ||
| }, [adapterAliases]); | ||
|
|
||
| const adapterAliasesByAddress = useMemo(() => { | ||
| const lookup = new Map<string, AdapterAddressAlias[]>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| const key = adapterAlias.address.toLowerCase(); | ||
| const existing = lookup.get(key) ?? []; | ||
| existing.push(toAdapterAddressAlias(adapterAlias)); | ||
| lookup.set(key, existing); | ||
| } | ||
| return lookup; | ||
| }, [adapterAliases]); | ||
|
|
||
| const adapterAliasesByVaultAddress = useMemo(() => { | ||
| const lookup = new Map<string, AdapterAddressAlias[]>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| const key = adapterAlias.vaultAddress.toLowerCase(); | ||
| const existing = lookup.get(key) ?? []; | ||
| existing.push(toAdapterAddressAlias(adapterAlias)); | ||
| lookup.set(key, existing); | ||
| } | ||
| return lookup; | ||
| }, [adapterAliases]); | ||
|
|
||
| const adapterAliasesByVaultScopedAddress = useMemo(() => { | ||
| const lookup = new Map<string, AdapterAddressAlias[]>(); | ||
| for (const adapterAlias of adapterAliases) { | ||
| const key = getAddressKey(adapterAlias.vaultAddress, adapterAlias.chainId); | ||
| const existing = lookup.get(key) ?? []; | ||
| existing.push(toAdapterAddressAlias(adapterAlias)); | ||
| lookup.set(key, existing); | ||
| } | ||
| return lookup; | ||
| }, [adapterAliases]); |
There was a problem hiding this comment.
The multiple useMemo hooks for indexing adapterAliases can be consolidated into a single useMemo block. This reduces the number of iterations over the adapterAliases array from four to one, improving efficiency and making the code cleaner.
const {
adapterAliasesByScopedAddress,
adapterAliasesByAddress,
adapterAliasesByVaultAddress,
adapterAliasesByVaultScopedAddress
} = useMemo(() => {
const scoped = new Map<string, AdapterAddressAlias>();
const byAddress = new Map<string, AdapterAddressAlias[]>();
const byVault = new Map<string, AdapterAddressAlias[]>();
const byVaultScoped = new Map<string, AdapterAddressAlias[]>();
for (const adapterAlias of adapterAliases) {
const alias = toAdapterAddressAlias(adapterAlias);
const addressKey = getAddressKey(adapterAlias.address, adapterAlias.chainId);
const vaultKey = getAddressKey(adapterAlias.vaultAddress, adapterAlias.chainId);
const lowerAddress = adapterAlias.address.toLowerCase();
const lowerVault = adapterAlias.vaultAddress.toLowerCase();
scoped.set(addressKey, alias);
const existingAddr = byAddress.get(lowerAddress) ?? [];
existingAddr.push(alias);
byAddress.set(lowerAddress, existingAddr);
const existingVault = byVault.get(lowerVault) ?? [];
existingVault.push(alias);
byVault.set(lowerVault, existingVault);
const existingVaultScoped = byVaultScoped.get(vaultKey) ?? [];
existingVaultScoped.push(alias);
byVaultScoped.set(vaultKey, existingVaultScoped);
}
return {
adapterAliasesByScopedAddress: scoped,
adapterAliasesByAddress: byAddress,
adapterAliasesByVaultAddress: byVault,
adapterAliasesByVaultScopedAddress: byVaultScoped,
};
}, [adapterAliases]);
There was a problem hiding this comment.
Leaving this one unchanged. This is a cleanup/performance suggestion rather than a correctness issue, and consolidating the existing maps into one larger memo block would add churn after the review fixes. The current code is deterministic for unscoped lookups, avoids O(N^2) array spread, and keeps each lookup map explicit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/features/positions/components/adapter-managed-exposure.tsx`:
- Around line 153-157: The current early return treats failed vault fetches the
same as legitimately empty adapter lists; update the conditional in
adapter-managed-exposure.tsx to explicitly handle vaultDataQuery.isError (or
vaultDataQuery.error) before returning null: if vaultDataQuery.isError render an
error state/UI (and optionally log the error), if !showLoading and
adapters.length === 0 render an explicit “no adapters” / empty state instead of
null; keep the existing showLoading behavior for vaultDataQuery.isLoading.
Reference symbols: showLoading, vaultDataQuery, adapters.
🪄 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: a1838f2f-8d70-42e0-a9c8-6308468fddae
📒 Files selected for processing (3)
src/components/ui/button.tsxsrc/contexts/VaultRegistryContext.tsxsrc/features/positions/components/adapter-managed-exposure.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/contexts/VaultRegistryContext.tsx
| const showLoading = vaultDataQuery.isLoading && adapters.length === 0; | ||
|
|
||
| if (adapters.length === 0 && !showLoading) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Handle vault query errors explicitly instead of returning null.
At Line 155, adapters.length === 0 && !showLoading returns null for both empty data and failed fetches. That hides a broken state as if nothing exists.
Suggested fix
- const showLoading = vaultDataQuery.isLoading && adapters.length === 0;
+ const showLoading = vaultDataQuery.isLoading && adapters.length === 0;
+ const showError = vaultDataQuery.isError && adapters.length === 0;
- if (adapters.length === 0 && !showLoading) {
+ if (showError) {
+ return <ExposureStatus message="Failed to load exposures." />;
+ }
+
+ if (adapters.length === 0 && !showLoading) {
return null;
}🤖 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 `@src/features/positions/components/adapter-managed-exposure.tsx` around lines
153 - 157, The current early return treats failed vault fetches the same as
legitimately empty adapter lists; update the conditional in
adapter-managed-exposure.tsx to explicitly handle vaultDataQuery.isError (or
vaultDataQuery.error) before returning null: if vaultDataQuery.isError render an
error state/UI (and optionally log the error), if !showLoading and
adapters.length === 0 render an explicit “no adapters” / empty state instead of
null; keep the existing showLoading behavior for vaultDataQuery.isLoading.
Reference symbols: showLoading, vaultDataQuery, adapters.
2 participants
Summary by CodeRabbit
New Features
Behavior
Documentation