feat: unified Sign-with-Extension picker across all signing surfaces - #987
Conversation
Extend the existing Sign-with-Extension detection and picker (already used by login and the auth-upgrade dialog) to the remaining single-button signing surfaces, so a "Keychain" choice is never silently routed to Hive Keeper, which aliases window.hive_keychain. - Make the saved extension preference per-username instead of one global value, with a fallback to the legacy global key so existing users are not reset. A user with one account on Keychain and another on Keeper now keeps the right wallet for each. - KeyOrHot and the wallet operations signer now detect all installed extensions (Keeper, Keychain, Peak Vault), show the shared ExtensionChooser when more than one is present, persist the choice for the active account, then sign with it. A single extension signs directly; private key, Hivesigner, MetaMask and mobile/in-app paths are unchanged. - Route the legacy per-op keychain helpers (utils/keychain.ts) through the chosen instance via resolveKeychainInstance instead of raw window.hive_keychain, so the preference is honored there too rather than resolving Keeper-first. - Tests: per-username preference and legacy fallback, resolveKeychainInstance routing, and the KeyOrHot picker persisting the chosen extension before signing.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 512e1f0cb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <ExtensionChooser | ||
| extensions={detectedExtensions} | ||
| onSelect={handleChooseExtension} |
There was a problem hiding this comment.
When KeyOrHot is rendered for owner-authority flows such as the key-rotation confirmation (authority="owner"), this chooser still passes every detected extension through. If a user has Keychain plus Peak Vault and selects Peak Vault, the handler persists peakvault and calls onKc, but broadcastWithExtension rejects owner operations for Peak Vault, so the flow fails even though a compatible extension was available. Filter or disable unsupported extensions when authority === "owner".
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 46d471c. KeyOrHot now excludes Peak Vault from the chooser and the single-extension direct path when authority === "owner", applied at both render time and click time. I also labeled account-recovery's signer authority="owner" (it broadcasts change_recovery_account, which the SDK authority map marks owner-only), so the same filter covers that flow. manage-keys-dialog is conditionally-owner (depends on whether owner keys are touched), so it stays dynamic; Peak Vault there still surfaces the clear "owner not supported" rejection if mis-picked rather than signing wrong. Added owner-exclusion tests (chooser and single-extension).
|
Warning Review limit reached
More reviews will be available in 15 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughExtension preference storage migrates from a single global ChangesPer-user Hive extension preference and multi-extension chooser UI
Sequence Diagram(s)sequenceDiagram
participant User
participant KeyOrHot
participant hive_extensions
participant localStorage
User->>KeyOrHot: Click "Sign with Keychain"
KeyOrHot->>hive_extensions: getDetectedExtensions()
alt Multiple extensions detected
hive_extensions-->>KeyOrHot: [keychain, hive-keeper]
KeyOrHot->>KeyOrHot: setChoosing(true) → render ExtensionChooser
User->>KeyOrHot: Select "Keychain"
KeyOrHot->>hive_extensions: setPreferredExtensionId(username, "keychain")
hive_extensions->>localStorage: write PREFERRED_EXTENSION_MAP_KEY[username]
KeyOrHot->>KeyOrHot: onKc()
else Single extension detected
hive_extensions-->>KeyOrHot: [hive-keeper]
KeyOrHot->>hive_extensions: setPreferredExtensionId(username, "hive-keeper")
hive_extensions->>localStorage: write PREFERRED_EXTENSION_MAP_KEY[username]
KeyOrHot->>KeyOrHot: onKc()
end
sequenceDiagram
participant signBufferWithExtension
participant getPreferredExtensionId
participant localStorage
participant resolveKeychainInstance
participant window_hive_keychain
signBufferWithExtension->>getPreferredExtensionId: getPreferredExtensionId(account)
getPreferredExtensionId->>localStorage: read PREFERRED_EXTENSION_MAP_KEY[account]
alt Per-user entry found
localStorage-->>getPreferredExtensionId: "keychain"
else Fallback
getPreferredExtensionId->>localStorage: read legacy PREFERRED_EXTENSION_KEY
end
getPreferredExtensionId-->>signBufferWithExtension: preferredId
signBufferWithExtension->>resolveKeychainInstance: resolveKeychainInstance(account)
resolveKeychainInstance->>localStorage: getPreferredExtensionId(account)
resolveKeychainInstance-->>signBufferWithExtension: KeyChainImpl instance
alt Stale preference (extension gone)
signBufferWithExtension->>getPreferredExtensionId: setPreferredExtensionId(account, null)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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)
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.
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 `@apps/web/src/specs/features/shared/key-or-hot-metamask.spec.tsx`:
- Around line 116-128: The test in the "with one extension, persists it for the
active user BEFORE signing" spec currently verifies that setPreferredExtensionId
was called with correct arguments and that onKc was called once, but does not
explicitly assert the order in which these calls occurred. To properly catch the
regression described in the comment, add explicit invocation-order assertions
between the setPreferredExtensionId mock and the props.onKc mock calls to verify
that the extension persistence happens before the onKc callback is triggered.
This ensures that downstream broadcast receives the persisted extension before
any auto-detection logic in onKc can hijack the user's selection.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7223d394-8c5c-4b73-ab13-04c6d6973732
📒 Files selected for processing (9)
apps/web/src/features/shared/auth-upgrade/auth-upgrade-dialog.tsxapps/web/src/features/shared/key-or-hot/index.tsxapps/web/src/features/shared/login/login.tsxapps/web/src/features/wallet/operations/wallet-operations-sign.tsxapps/web/src/specs/features/shared/auth-upgrade-dialog.spec.tsxapps/web/src/specs/features/shared/key-or-hot-metamask.spec.tsxapps/web/src/specs/utils/hive-extensions.spec.tsapps/web/src/utils/hive-extensions.tsapps/web/src/utils/keychain.ts
Greptile SummaryThis PR unifies extension detection and per-account preference persistence across all signing surfaces. Previously a single "Sign with Keychain" button could be silently hijacked by Hive Keeper (which aliases
Confidence Score: 5/5Safe to merge. The change is additive and well-scoped: per-user localStorage preference is isolated from the existing auto-detect fallback, every signing surface now persists the chosen extension before calling the broadcast path, and all routing changes are covered by new or updated tests. All three signing surfaces (KeyOrHot, WalletOperationSign, AuthUpgradeDialog) correctly persist the extension choice per username before triggering the broadcast. The previous review finding about the single-extension path in AuthUpgradeDialog not persisting is addressed. The resolveKeychainInstance routing is deterministic (keychain → real Keychain, never Keeper alias), and the stale-preference self-healing logic carries over to the per-user map. No data-loss or wrong-wallet paths were found. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks Sign with Extension] --> B{Re-read getDetectedExtensions at click time}
B -->|0 extensions| C[Sign via Keychain Mobile / in-app path]
B -->|1 extension| D[Persist ext for username\nsetPreferredExtensionId]
D --> E[Call onKc / doExtensionSign]
B -->|2+ extensions| F[setChoosing true\nShow ExtensionChooser]
F --> G{authority === owner?}
G -->|Yes| H[Filter out Peak Vault\nfrom chooser list]
G -->|No| I[Show all detected extensions]
H --> J[User picks extension]
I --> J
J --> K[setChoosing false\nPersist ext for username]
K --> E
E --> L{Broadcast adapter resolves extension}
L --> M[getPreferredExtensionId username]
M -->|keychain| N[getKeychainInstance\nnever Keeper alias]
M -->|hive-keeper| O[getHiveKeeperInstance\nwindow.hive]
M -->|peakvault| P[getPeakVaultInstance]
M -->|null| Q[Keeper-first auto-detect fallback]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User clicks Sign with Extension] --> B{Re-read getDetectedExtensions at click time}
B -->|0 extensions| C[Sign via Keychain Mobile / in-app path]
B -->|1 extension| D[Persist ext for username\nsetPreferredExtensionId]
D --> E[Call onKc / doExtensionSign]
B -->|2+ extensions| F[setChoosing true\nShow ExtensionChooser]
F --> G{authority === owner?}
G -->|Yes| H[Filter out Peak Vault\nfrom chooser list]
G -->|No| I[Show all detected extensions]
H --> J[User picks extension]
I --> J
J --> K[setChoosing false\nPersist ext for username]
K --> E
E --> L{Broadcast adapter resolves extension}
L --> M[getPreferredExtensionId username]
M -->|keychain| N[getKeychainInstance\nnever Keeper alias]
M -->|hive-keeper| O[getHiveKeeperInstance\nwindow.hive]
M -->|peakvault| P[getPeakVaultInstance]
M -->|null| Q[Keeper-first auto-detect fallback]
Reviews (3): Last reviewed commit: "fix: persist the lone extension on the a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…der test) - KeyOrHot excludes Peak Vault from the chooser and the single-extension path for owner-authority flows (Peak Vault can't sign owner ops; broadcastWithExtension rejects them), at both render and click time. Also label account-recovery's signer authority="owner" (change_recovery_account is owner-only per the SDK authority map) so the same filter applies there. - Validate stored preference values against the known extension-id union on read (per-user map and legacy global) so a corrupted or hand-edited localStorage entry never routes signing; it falls through to auto-detect. - Regression test now asserts the chosen extension is persisted BEFORE onKc via invocationCallOrder; add owner Peak-Vault-exclusion and corrupted-value tests.
…path The auth-upgrade dialog persisted the choice only when the user went through the chooser; its single-extension direct path resolved without saving, so a Keychain-only user who later installed Keeper could be hijacked on the next active-authority sign. Persist the lone extension there too, matching KeyOrHot and WalletOperationSign, and exclude Peak Vault for owner-authority requests (the dialog can be invoked for owner ops such as change_recovery_account).
|
Good catch, closed in this push. The auth-upgrade dialog's single-extension path now persists the lone extension before resolving ( |
1 participant
Summary
When a Keychain user signs (transfers, wallet ops, permissions, etc.), the single "Sign with Keychain" button could be silently hijacked by Hive Keeper, because Keeper aliases
window.hive_keychainand the broadcast auto-detects Keeper-first. With one account on Keychain and another on Keeper, the wrong wallet would answer.This brings the unified "Sign with Extension" detection and picker that login and the auth-upgrade dialog already use to the remaining single-button signing surfaces, and makes the routing respect the user's explicit choice per account.
Changes
{username: extId}map) instead of one global value, falling back to the legacy global key so existing users are not reset. A user keeps Keychain for one account and Keeper for another.KeyOrHotand the wallet operations signer (wallet-operations-sign.tsx, the transfer dialog in the screenshot) now callgetDetectedExtensions(), show the sharedExtensionChooserwhen more than one is installed, persist the choice for the active account, then sign with it. A single extension signs directly. Private key, Hivesigner, MetaMask and mobile/in-app paths are unchanged.utils/keychain.tsper-op helpers now resolve the chosen instance viaresolveKeychainInstance(account)instead of reading rawwindow.hive_keychain, so a Keychain choice routes to the real Keychain (never Keeper) and a Keeper choice uses the livewindow.hive.Detection is hybrid by necessity: only Keeper implements the unified
window.hiveregistry; Keychain and Peak Vault are detected via their legacy globals, and Keeper'swindow.hive_keychainself-alias is filtered out so it is never offered as a phantom Keychain.Tests
resolveKeychainInstanceroutes to the account's chosen extension, never Keeper-first when a Keychain choice exists.KeyOrHotpicker persists the chosen extension (per username) beforeonKc, and opens the chooser without auto-signing when more than one extension is present.Typecheck adds no new errors over
develop; all affected specs pass.Notes
The funds-transfer flow already used the auth-upgrade picker; this closes the remaining single-button surfaces so the behavior is uniform everywhere.
Summary by CodeRabbit
New Features
Improvements