feat: allow hiding account modal quick actions#600
feat: allow hiding account modal quick actions#600Agilulfo1820 merged 3 commits intovechain:mainfrom
Conversation
👋 Thanks for your contribution!Since this PR comes from a forked repository, the lint and build will only run for internal PRs for security reasons. Thank you for your patience! 🙏 |
📝 WalkthroughWalkthroughThe PR adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as VeChainKitProvider
participant Context as KitContext
participant UI as QuickActionsSection
participant Button as QuickActionButton
Provider->>Context: initialize value (hiddenQuickActions: [])
Note right of Context: consumer reads config
UI->>Context: read hiddenQuickActions
UI->>UI: filter QUICK_ACTIONS -> visibleQuickActions
alt visibleQuickActions not empty
UI->>UI: compute grid columns based on count
UI->>Button: render each visible action (id,label,showRedDot)
else none visible
UI-->>UI: return null (no rendering)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vechain-kit/src/providers/VeChainKitProvider.tsx (1)
160-183:⚠️ Potential issue | 🟠 MajorKeep the exported config type backward-compatible.
VeChainKitConfigis re-exported from the package, so makinghiddenQuickActionsrequired is a compile-time breaking change for downstream mocks/builders that already satisfy this public type. Please keep the public field optional, or split out an internal “resolved config” type for the context value instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vechain-kit/src/providers/VeChainKitProvider.tsx` around lines 160 - 183, The exported VeChainKitConfig currently makes hiddenQuickActions required, which is a breaking change; change hiddenQuickActions to optional on the public VeChainKitConfig (i.e., hiddenQuickActions?: AccountQuickAction[]), or alternatively introduce an internal ResolvedVeChainKitConfig (or VeChainKitInternalConfig) used by the provider/context that includes the required hiddenQuickActions while keeping the exported VeChainKitConfig unchanged; update usages in VeChainKitProvider (where you read/normalize config) to accept the optional field and populate a resolved/internal config before use.
🧹 Nitpick comments (1)
packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsx (1)
124-142: Optimize data fetching by making hooks conditional on quick action visibility.The
useTotalBalanceanduseUpgradeRequiredhooks execute before checking if any quick actions are visible (line 136–138), so configurations that hide all actions still pay the cost of fetching token balances and upgrade status on every render.Since
useTotalBalancedoesn't currently support anenabledparameter, enhance it to accept one (following the pattern already present inuseUpgradeRequiredat line 80). Then wire both hooks to skip execution whenvisibleQuickActionsis empty:const hasVisibleActions = visibleQuickActions.length > 0; const { hasAnyBalance } = useTotalBalance({ address: account?.address ?? '', enabled: hasVisibleActions, }); const { data: upgradeRequired } = useUpgradeRequired( smartAccount?.address ?? '', connectedWallet?.address ?? '', 3, hasVisibleActions, );This aligns with the coding guidelines for conditional query enabling and ensures the hidden section carries negligible cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsx` around lines 124 - 142, The code currently calls useTotalBalance and useUpgradeRequired unconditionally which triggers unnecessary fetches even when all quick actions are hidden; add a local boolean (e.g., hasVisibleActions = visibleQuickActions.length > 0) and update calls so useTotalBalance accepts and uses an enabled flag (mirror useUpgradeRequired's pattern) and pass enabled: hasVisibleActions when invoking useTotalBalance, and pass hasVisibleActions as the last parameter to useUpgradeRequired (instead of calling it unconditionally); update the useTotalBalance hook implementation to accept and forward an enabled option to its underlying query so it skips fetching when hasVisibleActions is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/vechain-kit/src/providers/VeChainKitProvider.tsx`:
- Around line 160-183: The exported VeChainKitConfig currently makes
hiddenQuickActions required, which is a breaking change; change
hiddenQuickActions to optional on the public VeChainKitConfig (i.e.,
hiddenQuickActions?: AccountQuickAction[]), or alternatively introduce an
internal ResolvedVeChainKitConfig (or VeChainKitInternalConfig) used by the
provider/context that includes the required hiddenQuickActions while keeping the
exported VeChainKitConfig unchanged; update usages in VeChainKitProvider (where
you read/normalize config) to accept the optional field and populate a
resolved/internal config before use.
---
Nitpick comments:
In
`@packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsx`:
- Around line 124-142: The code currently calls useTotalBalance and
useUpgradeRequired unconditionally which triggers unnecessary fetches even when
all quick actions are hidden; add a local boolean (e.g., hasVisibleActions =
visibleQuickActions.length > 0) and update calls so useTotalBalance accepts and
uses an enabled flag (mirror useUpgradeRequired's pattern) and pass enabled:
hasVisibleActions when invoking useTotalBalance, and pass hasVisibleActions as
the last parameter to useUpgradeRequired (instead of calling it
unconditionally); update the useTotalBalance hook implementation to accept and
forward an enabled option to its underlying query so it skips fetching when
hasVisibleActions is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0062bbc-3a52-4301-a621-33311a1d72b3
📒 Files selected for processing (2)
packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsxpackages/vechain-kit/src/providers/VeChainKitProvider.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsx (1)
158-158:⚠️ Potential issue | 🟠 MajorBug:
showRedDotcondition references non-existent action label.The condition
action.label === 'Settings'will always befalsebecause none of the quick actions have the label'Settings'. The available labels are'Send','Swap', and'Receive'.This means the red dot indicator for upgrade notifications will never appear on any quick action button.
If the red dot should not appear on any quick action, simplify by removing the condition entirely. Otherwise, update the condition to reference the correct action label.
🐛 Proposed fix (if red dot should not appear on quick actions)
onClick={() => action.onClick(setCurrentContent)} isDisabled={action.isDisabled?.(hasAnyBalance)} - showRedDot={showRedDot && action.label === 'Settings'} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsx` at line 158, The showRedDot check uses a non-existent label 'Settings', so update the expression in QuickActionsSection.tsx where the quick action button passes showRedDot: either remove the label comparison so the prop is simply showRedDot (if the dot should appear on all quick actions) or change the comparison to a valid label from the actions list (e.g., replace action.label === 'Settings' with action.label === 'Receive') depending on the intended UX.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsx`:
- Line 158: The showRedDot check uses a non-existent label 'Settings', so update
the expression in QuickActionsSection.tsx where the quick action button passes
showRedDot: either remove the label comparison so the prop is simply showRedDot
(if the dot should appear on all quick actions) or change the comparison to a
valid label from the actions list (e.g., replace action.label === 'Settings'
with action.label === 'Receive') depending on the intended UX.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b71403d-8051-4b5c-a3f6-3b167f0ce335
📒 Files selected for processing (2)
packages/vechain-kit/src/components/AccountModal/Components/QuickActionsSection.tsxpackages/vechain-kit/src/providers/VeChainKitProvider.tsx
This is a valid pre-existing issue, but not introduced by this PR. |
Agilulfo1820
added
safe-to-deploy
2 participants
Summary
hiddenQuickActionsprovider config to control which account modal quick actions are shownWhy
swap/send/receiveSummary by CodeRabbit