feat: enhance restricted address handling in App.vue and index.vue#775
Conversation
|
Caution Review failedFailed to post review comments WalkthroughReplaces NodeJS.ProcessEnv typings with a Window-based package-version declaration and expands address-restriction handling across the extension: a single-account reactive Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/extension/src/ui/action/views/restricted/index.vue (1)
101-112: Simplify primitive prop defaults.Arrow functions for primitive defaults add unnecessary overhead. Vue recommends direct values for Boolean and String types.
isAddressRestricted: { type: Boolean, - default: () => { - return false; - }, + default: false, }, restrictedAddress: { type: String, - default: () => { - return ''; - }, + default: '', },packages/extension/src/ui/action/App.vue (1)
439-453: Consider avoiding UI flash during restriction check.Resetting
isAddressRestrictedtofalse(lines 440-443) before the asyncisWalletRestrictedcall completes could briefly show unrestricted UI to a restricted user. Consider deferring the reset or adding a loading state.const checkAddress = async (activeAccount: EnkryptAccount) => { - isAddressRestricted.value = { - isRestricted: false, - address: '', - }; const isRestricted = await isWalletRestricted( currentNetwork.value.displayAddress(activeAccount.address), ); - if (isRestricted) { - isAddressRestricted.value = { - isRestricted, - address: currentNetwork.value.displayAddress(activeAccount.address), - }; - } + isAddressRestricted.value = { + isRestricted, + address: isRestricted + ? currentNetwork.value.displayAddress(activeAccount.address) + : '', + }; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extension/env.d.ts(1 hunks)packages/extension/src/ui/action/App.vue(6 hunks)packages/extension/src/ui/action/views/restricted/index.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (4)
packages/extension/env.d.ts (1)
17-20: LGTM!The Window interface extension properly exposes
__PACKAGE_VERSION__for both direct andwindow.access patterns, which is a clean TypeScript approach.packages/extension/src/ui/action/views/restricted/index.vue (2)
5-15: LGTM!The conditional rendering cleanly separates the two restriction scenarios (geo/IP-based vs address-based), and displaying the restricted address helps users understand which account is affected.
31-40: LGTM!The Switch Address block provides clear UX for users to escape a restricted address state. The event emission pattern is correctly typed.
packages/extension/src/ui/action/App.vue (1)
7-12: LGTM!The Restricted component is correctly wired with the new props and event handler.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/extension/src/ui/action/App.vue (1)
502-509:switchToUnrestrictedAddressdoesn’t verify new account is actually unrestrictedThis helper only picks the first account whose display address differs from the currently restricted one and doesn’t:
- Re-check whether that candidate is itself restricted, or
- Provide feedback when all accounts are restricted (no candidate found).
That means:
- You may switch the user from one restricted account straight into another.
- If every account is restricted, clicking the “switch” action silently does nothing.
Consider something along these lines:
-const switchToUnrestrictedAddress = () => { - const newAccount = accountHeaderData.value.activeAccounts.find( - aa => - currentNetwork.value.displayAddress(aa.address) !== - isAddressRestricted.value.address, - ); - if (newAccount) onSelectedAddressChanged(newAccount); -}; +const switchToUnrestrictedAddress = async () => { + for (const account of accountHeaderData.value.activeAccounts) { + const displayAddr = currentNetwork.value.displayAddress(account.address); + if (displayAddr === isAddressRestricted.value.address) continue; + + const restricted = await isWalletRestricted(displayAddr); + if (!restricted) { + await onSelectedAddressChanged(account); + return; + } + } + // TODO: surface UX feedback (toast / alert) when no unrestricted account exists +}This preserves the intent while guaranteeing you only auto-switch to a truly unrestricted account and giving you a hook to explain the “no available account” case to the user.
🧹 Nitpick comments (1)
packages/extension/src/ui/action/App.vue (1)
461-464: HookingcheckAddressinto address changes is good, but consider awaitingWiring
onSelectedAddressChangedtocheckAddress(newAccount)ensures the restricted banner tracks the currently selected account, which is the right behavior.Given
checkAddressis async, you might optionallyawait checkAddress(newAccount);here (and insetNetwork) or explicitly mark the call as fire‑and‑forget (void checkAddress(newAccount);) to make the intent clearer to future readers, but functionally this is fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension/src/ui/action/App.vue(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (2)
packages/extension/src/ui/action/App.vue (2)
5-12: Confirm Restricted event name matches child emitThe parent listens to
@restricted:switch-account="switchToUnrestrictedAddress". Per the PR summary, the childRestrictedview emitsrestricted:switchAccount. If the emit name is indeedrestricted:switchAccount(camelCase), this listener will never fire because the strings don’t match.Double‑check that:
- The emitted event name in
views/restricted/index.vueexactly matches the one used here, and- You stick to a single naming convention (prefer kebab-case like
restricted:switch-accountin both emit and listener for Vue templates).
353-355: Ordering ofcurrentNetworkassignment andcheckAddresslooks correctAssigning
currentNetwork.value = networkbefore invokingcheckAddress(selectedAccount)ensures the restriction check uses the newly selected network’sdisplayAddress, fixing the stale-network concern that was previously raised.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/extension/src/ui/action/views/restricted/index.vue (2)
31-40: Consider adding keyboard accessibility to clickable elements.The clickable header div lacks
role="button",tabindex="0", and keyboard event handlers, which prevents keyboard-only users from activating this control. The same pattern exists in the other content block (line 42), but since this is new code, consider improving accessibility here.- <div - class="blocked-page__content__header" - @click="emit('restricted:switchAccount')" - > + <div + class="blocked-page__content__header" + role="button" + tabindex="0" + @click="emit('restricted:switchAccount')" + @keydown.enter="emit('restricted:switchAccount')" + @keydown.space.prevent="emit('restricted:switchAccount')" + >
101-112: Simplify primitive prop defaults.For primitive types (Boolean, String), Vue 3 allows direct values without factory functions. The arrow function syntax is only required for Object/Array defaults to avoid shared references.
isAddressRestricted: { type: Boolean, - default: () => { - return false; - }, + default: false, }, restrictedAddress: { type: String, - default: () => { - return ''; - }, + default: '', },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension/src/ui/action/views/restricted/index.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (4)
packages/extension/src/ui/action/views/restricted/index.vue (4)
5-15: LGTM!The conditional rendering for address-specific vs. IP/jurisdiction restriction messages is correctly implemented. Vue's template interpolation safely escapes the
restrictedAddressvalue.
22-28: LGTM!Good security practice using
rel="noopener noreferrer"on the external link to prevent reverse tabnabbing.
57-70: LGTM!Explicit prop and event bindings improve code clarity and maintainability.
76-81: LGTM!Wiring both
@window:backand@window:closetocloseMnemonicprovides consistent behavior and future-proofs the component if:has-backchanges.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/extension/src/ui/action/views/restricted/index.vue (5)
5-13: Address/IP restriction messaging looks correct; consider empty-address fallback.The conditional copy keyed off
isAddressRestrictedvs!isAddressRestrictedis clean and mutually exclusive, and using mustache interpolation keepsrestrictedAddresssafe.If there’s any chance
isAddressRestrictedcan betruewhilerestrictedAddressis an empty string, you’d end up with[]in the sentence. If that state is possible, consider a small guard (e.g., hide the brackets or show a shortened/placeholder address).Also applies to: 16-19, 21-27
30-40: CTA gating looks good; improve accessibility of clickable headers.The split between “Switch Address” for restricted wallets and “View My Secret Recovery Phrase” for non‑restricted ones is clear and matches the described flow. Emitting
restricted:switchAccountfrom the header is also wired correctly.Since these headers are effectively buttons, consider making them keyboard- and screen‑reader friendly (e.g., use
<button>or addrole="button",tabindex="0", and a key handler for Enter/Space).
56-63: Confirm intent of broad$attrsforwarding tomodal-sign.Using
v-bind="$attrs"here will forward all attributes/listeners from<Restricted>’s parent tomodal-sign, while they’ll also still be applied to the root element unlessinheritAttrs: falseis set.If the goal is specifically to pass only certain props/events down to
modal-sign, you might want to explicitly bind those instead of$attrs, or disable root inheritance viadefineOptions({ inheritAttrs: false })to avoid duplicated/accidental forwarding. If you intentionally rely on this passthrough for multiple modal props, current code is fine—just worth double‑checking.
65-69: Minor redundancy betweenv-ifand:is-forgot.
modal-forgotis bothv-if="isOpenForgot"and receives:is-forgot="isOpenForgot". If the prop is only used to indicate visibility, one of these may be redundant. Not a problem functionally, but you could simplify later for clarity.
100-111: Props for restriction state are well-defined.
isAddressRestrictedandrestrictedAddressare correctly typed and integrated with the template logic. If you ever revisit this, you could simplify boolean/string defaults todefault: false/default: ''instead of arrow functions, but it’s purely stylistic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension/src/ui/action/views/restricted/index.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (2)
packages/extension/src/ui/action/views/restricted/index.vue (2)
75-80: Mnemonic modal wiring is consistent and safe.Binding both
window:backandwindow:closetocloseMnemonickeeps the state reset path simple and aligned with other views. No issues here.
114-116: Typed emit forrestricted:switchAccountis correct.The
defineEmitstyping matches the template usage and keeps the restricted‑address flow explicit and type‑safe. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/extension/src/ui/action/App.vue (3)
354-354: Guard against emptyactiveAccountsarray.The past review comment about stale network reference has been resolved—
currentNetwork.valueis now set before callingcheckAddress. However,selectedAccountcould beundefinedifactiveAccountsis empty (line 340), which would causecheckAddressto crash when accessingactiveAccount.address.Apply this diff to add a guard:
currentNetwork.value = network; - checkAddress(selectedAccount); + if (selectedAccount) { + checkAddress(selectedAccount); + }Or combine with the null-safety fix in
checkAddressitself (see comment on lines 439-453).
439-453: Add null-safety and prevent race conditions incheckAddress.This function lacks null-safety for
activeAccountand is vulnerable to race conditions when networks or accounts change during the asyncisWalletRestrictedcall. IfactiveAccountsis ever empty,selectedAccountwill beundefinedand accessingactiveAccount.addresswill crash.Apply the fix suggested in the past review comment: add a null-safety guard, snapshot the network and display address before the async call, and optionally verify the snapshot still matches after the async call completes.
-const checkAddress = async (activeAccount: EnkryptAccount) => { +const checkAddress = async (activeAccount?: EnkryptAccount | null) => { + // reset before running any async work isAddressRestricted.value = { isRestricted: false, address: '', }; + + if (!activeAccount) return; + + const network = currentNetwork.value; + const displayAddress = network.displayAddress(activeAccount.address); + - const isRestricted = await isWalletRestricted( - currentNetwork.value.displayAddress(activeAccount.address), - ); + const isRestricted = await isWalletRestricted(displayAddress); + + // Optional: ignore stale results if network/account changed while awaiting + // if ( + // currentNetwork.value !== network || + // accountHeaderData.value.selectedAccount?.address !== activeAccount.address + // ) { + // return; + // } + if (isRestricted) { isAddressRestricted.value = { - isRestricted, - address: currentNetwork.value.displayAddress(activeAccount.address), + isRestricted: true, + address: displayAddress, }; } };
504-511: Verify new account is actually unrestricted.The function selects the first account with a different display address but doesn't verify it's unrestricted. If multiple accounts are restricted, the user could be switched to another restricted account. Additionally, if no unrestricted account exists, the function silently does nothing.
Apply the fix suggested in the past review comment: filter accounts through
isWalletRestrictedto find a truly unrestricted one and provide user feedback when all accounts are restricted.-const switchToUnrestrictedAddress = () => { +const switchToUnrestrictedAddress = async () => { - const newAccount = accountHeaderData.value.activeAccounts.find( - aa => - currentNetwork.value.displayAddress(aa.address) !== - isAddressRestricted.value.address, - ); - if (newAccount) onSelectedAddressChanged(newAccount); + for (const account of accountHeaderData.value.activeAccounts) { + const displayAddr = currentNetwork.value.displayAddress(account.address); + if (displayAddr === isAddressRestricted.value.address) continue; + const restricted = await isWalletRestricted(displayAddr); + if (!restricted) { + onSelectedAddressChanged(account); + return; + } + } + // TODO: Show user feedback that all accounts are restricted };
🧹 Nitpick comments (1)
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
440-444: Consider i18n / consistent copy for “Restricted Address”.If you have a shared restricted-flow message elsewhere (App.vue/index.vue per PR objective), reusing a constant/i18n key would keep wording consistent across screens.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extension/src/providers/ethereum/ui/eth-connect-dapp.vue(5 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(5 hunks)packages/extension/src/ui/action/App.vue(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (6)
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (2)
178-178: Good integration point for restricted flow check.
440-444: Nice UX + safety: error messaging and send gating now honor restricted state.Also applies to: 584-590
packages/extension/src/ui/action/App.vue (3)
5-12: Template bindings look correct.The conditional rendering and props correctly map to the new
isAddressRestrictedstate structure, and the event binding properly connects toswitchToUnrestrictedAddress.
172-175: State declaration is clear and well-typed.The
isAddressRestrictedref uses an explicit type annotation and sensible initial values.
461-492: Conditional logic correctly gates on restriction status.The function appropriately checks the address restriction before propagating the address change to the background and tabs. This prevents restricted addresses from being activated. The logic is sound assuming
checkAddressis fixed to handle null-safety and race conditions as noted in the previous comment.packages/extension/src/providers/ethereum/ui/eth-connect-dapp.vue (1)
68-68: LGTM: Connect button properly disabled for restricted addresses.The button disable logic correctly prevents connection when the address is restricted. Ensure the race condition flagged in the earlier comment is addressed to make this security measure effective from the moment the component renders.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.