Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughAdds a unified ManageKeysDialog to handle add and revoke flows, moves revoke handling to parent components, extends the review step for revoke-mode and initial selection, removes legacy revoke/add components, adds SDK helpers to build revoke operations, and updates i18n, tests, and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ManageKeysDialog (UI)
participant Query as AccountQuery
participant SDK as useAccountRevokeKey / buildRevokeKeysOp
participant Keychain as Keychain/MetaMask
participant Cache as QueryCache
User->>UI: open dialog (show / initialRevokeKey?)
UI->>Query: fetch accountData
Query-->>UI: accountData
alt Add Flow
User->>UI: select "Add New Keys"
UI->>UI: Generate master password -> Review -> Confirm
UI-->>User: provide keys file / master password
else Revoke Flow
User->>UI: select keys to revoke (pre-selections may apply)
User->>UI: confirm revoke & choose signing method
alt Private Key
UI->>SDK: call mutateAsync with revokingKey(s)
SDK-->>UI: revoke result
else Keychain/MetaMask
UI->>SDK: buildRevokeKeysOp(accountData, revokingKeys)
UI->>Keychain: broadcast built account_update op
Keychain-->>UI: tx result
end
UI->>Cache: invalidate account query
UI-->>User: show success / close dialog
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx:
- Around line 405-424: The broadcast currently always requests "owner" signing
in handleSignByKeychain, which can be incorrect when buildRevokeKeysOp omitted
owner; inspect the op payload returned from buildRevokeKeysOp(account,
allRevokingKeys) and derive the correct keyType to pass to
broadcastWithKeychain: if opPayload.owner is present use "owner", else if
opPayload.active is present use "active", else if opPayload.posting is present
use "posting" (fallback to a sensible default if none); then call
adapter.broadcastWithKeychain!(username, [op], keyType) instead of hardcoding
"owner".
- Around line 471-477: The authority prop is hardcoded to "owner" but must
reflect the selected keyType; compute the authority (e.g., if keyType ===
"posting" or "active" use that value, otherwise "owner") and pass that
computedAuthority to the KeyOrHot component (replace authority="owner") and also
use the same computedAuthority when calling broadcastWithKeychain inside
handleSignByKeychain so both the UI prompt and the broadcast use the same
dynamic authority tied to keyType/isRevoking/selected keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 811b7938-aebc-4853-ae1e-3f8baa178e16
⛔ Files ignored due to path filters (7)
packages/sdk/dist/browser/index.d.tsis excluded by!**/dist/**packages/sdk/dist/browser/index.jsis excluded by!**/dist/**packages/sdk/dist/browser/index.js.mapis excluded by!**/dist/**,!**/*.mappackages/sdk/dist/node/index.cjsis excluded by!**/dist/**packages/sdk/dist/node/index.cjs.mapis excluded by!**/dist/**,!**/*.mappackages/sdk/dist/node/index.mjsis excluded by!**/dist/**packages/sdk/dist/node/index.mjs.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (6)
apps/web/src/app/(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsxapps/web/src/specs/features/permissions/add-keys-flow.spec.tsxapps/web/src/specs/setup-any-spec.tspackages/sdk/src/modules/accounts/mutations/build-revoke-keys-op.tspackages/sdk/src/modules/accounts/mutations/index.tspackages/sdk/src/modules/accounts/mutations/use-account-revoke-key.ts
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/src/modules/accounts/mutations/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/specs/features/permissions/add-keys-flow.spec.tsx
- packages/sdk/src/modules/accounts/mutations/use-account-revoke-key.ts
Comment on lines
+405
to
+424
| // Keychain/MetaMask signing - uses shared SDK op builder, broadcasts via adapter | ||
| const handleSignByKeychain = async () => { | ||
| onSignStart(); | ||
| try { | ||
| const account = await queryClient.fetchQuery( | ||
| getAccountFullQueryOptions(username) | ||
| ); | ||
| if (!account) throw new Error("Account data not loaded"); | ||
|
|
||
| const opPayload = buildRevokeKeysOp(account, allRevokingKeys); | ||
| const op = ["account_update", opPayload] as unknown as Operation; | ||
|
|
||
| const adapter = getWebBroadcastAdapter(); | ||
| await adapter.broadcastWithKeychain!(username, [op], "owner"); | ||
| onSuccess(); | ||
| } catch (err: any) { | ||
| onSignError(); | ||
| error(...formatError(err)); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
keyType should match whether owner authority is being modified.
buildRevokeKeysOp conditionally sets owner: undefined when no revoking keys exist in the owner authority (see lines 55-60 in build-revoke-keys-op.ts). However, line 418 always passes "owner" to broadcastWithKeychain, which requests owner-level authentication from the user even when only active/posting keys are being revoked.
This unnecessarily elevates the required signing authority and may confuse users or fail if they only have active keys available.
Proposed fix
const opPayload = buildRevokeKeysOp(account, allRevokingKeys);
const op = ["account_update", opPayload] as unknown as Operation;
+ const requiredKeyType = opPayload.owner ? "owner" : "active";
const adapter = getWebBroadcastAdapter();
- await adapter.broadcastWithKeychain!(username, [op], "owner");
+ await adapter.broadcastWithKeychain!(username, [op], requiredKeyType);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Keychain/MetaMask signing - uses shared SDK op builder, broadcasts via adapter | |
| const handleSignByKeychain = async () => { | |
| onSignStart(); | |
| try { | |
| const account = await queryClient.fetchQuery( | |
| getAccountFullQueryOptions(username) | |
| ); | |
| if (!account) throw new Error("Account data not loaded"); | |
| const opPayload = buildRevokeKeysOp(account, allRevokingKeys); | |
| const op = ["account_update", opPayload] as unknown as Operation; | |
| const adapter = getWebBroadcastAdapter(); | |
| await adapter.broadcastWithKeychain!(username, [op], "owner"); | |
| onSuccess(); | |
| } catch (err: any) { | |
| onSignError(); | |
| error(...formatError(err)); | |
| } | |
| }; | |
| // Keychain/MetaMask signing - uses shared SDK op builder, broadcasts via adapter | |
| const handleSignByKeychain = async () => { | |
| onSignStart(); | |
| try { | |
| const account = await queryClient.fetchQuery( | |
| getAccountFullQueryOptions(username) | |
| ); | |
| if (!account) throw new Error("Account data not loaded"); | |
| const opPayload = buildRevokeKeysOp(account, allRevokingKeys); | |
| const op = ["account_update", opPayload] as unknown as Operation; | |
| const requiredKeyType = opPayload.owner ? "owner" : "active"; | |
| const adapter = getWebBroadcastAdapter(); | |
| await adapter.broadcastWithKeychain!(username, [op], requiredKeyType); | |
| onSuccess(); | |
| } catch (err: any) { | |
| onSignError(); | |
| error(...formatError(err)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx
around lines 405 - 424, The broadcast currently always requests "owner" signing
in handleSignByKeychain, which can be incorrect when buildRevokeKeysOp omitted
owner; inspect the op payload returned from buildRevokeKeysOp(account,
allRevokingKeys) and derive the correct keyType to pass to
broadcastWithKeychain: if opPayload.owner is present use "owner", else if
opPayload.active is present use "active", else if opPayload.posting is present
use "posting" (fallback to a sensible default if none); then call
adapter.broadcastWithKeychain!(username, [op], keyType) instead of hardcoding
"owner".
Comment on lines
+471
to
+477
| <KeyOrHot | ||
| inProgress={isRevoking} | ||
| onKey={handleSignByKey} | ||
| onKc={handleSignByKeychain} | ||
| onMetaMask={handleSignByKeychain} | ||
| authority="owner" | ||
| /> |
Contributor
There was a problem hiding this comment.
KeyOrHot authority should also be dynamic.
The authority="owner" prop is hardcoded, but when only revoking active/posting keys, the user should be prompted for an active key instead. This should be consistent with the keyType passed to broadcastWithKeychain.
Proposed fix
Compute the required authority based on selected keys and pass it to both KeyOrHot and broadcastWithKeychain:
+ // Determine minimum required authority level
+ const requiredAuthority = keysToRevoke.owner.length > 0 ? "owner" : "active";
+
// ...existing code...
<KeyOrHot
inProgress={isRevoking}
onKey={handleSignByKey}
onKc={handleSignByKeychain}
onMetaMask={handleSignByKeychain}
- authority="owner"
+ authority={requiredAuthority}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <KeyOrHot | |
| inProgress={isRevoking} | |
| onKey={handleSignByKey} | |
| onKc={handleSignByKeychain} | |
| onMetaMask={handleSignByKeychain} | |
| authority="owner" | |
| /> | |
| // Determine minimum required authority level | |
| const requiredAuthority = keysToRevoke.owner.length > 0 ? "owner" : "active"; | |
| // ...existing code... | |
| <KeyOrHot | |
| inProgress={isRevoking} | |
| onKey={handleSignByKey} | |
| onKc={handleSignByKeychain} | |
| onMetaMask={handleSignByKeychain} | |
| authority={requiredAuthority} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx
around lines 471 - 477, The authority prop is hardcoded to "owner" but must
reflect the selected keyType; compute the authority (e.g., if keyType ===
"posting" or "active" use that value, otherwise "owner") and pass that
computedAuthority to the KeyOrHot component (replace authority="owner") and also
use the same computedAuthority when calling broadcastWithKeychain inside
handleSignByKeychain so both the UI prompt and the broadcast use the same
dynamic authority tied to keyType/isRevoking/selected keys.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/app/(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx (2)
420-421: Consider extracting the operation type construction.The double type assertion
as unknown as Operationis a code smell, though sometimes necessary with library type mismatches. If this pattern is used elsewhere, consider a helper that provides proper typing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx around lines 420 - 421, The double type assertion on the constructed operation should be removed by introducing a small helper (e.g., toOperation or buildOperation) that accepts the operation type string and payload and returns a properly typed Operation; replace the inline `const op = ["account_update", opPayload] as unknown as Operation` with a call to that helper and update usages around buildRevokeKeysOp and the local `op` variable so the Operation type is produced without `as unknown as Operation`.
52-60: Consideraccount_authswhen gating revoke availability.The
auth.key_auths.length > 1check excludes authorities whereaccount_authscontribute sufficient weight. Per the SDK'scanRevokeFromAuthority(context snippet 3), account auths are valid weight contributors. An authority with 1key_auth+ sufficientaccount_authswould incorrectly hide the revoke option.If this conservative approach is intentional to avoid edge cases, consider adding a comment explaining the reasoning.
♻️ Proposed fix to include account_auths in the gate
const canRevoke = accountData ? [accountData.owner, accountData.active, accountData.posting].some((auth) => - auth.key_auths.length > 1 && + (auth.key_auths.length + (auth.account_auths?.length ?? 0)) > 1 && auth.key_auths.some(([key]) => { const without = new Set([String(key)]); return canRevokeFromAuthority(auth, without); }) ) : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx around lines 52 - 60, The current canRevoke gating uses auth.key_auths.length > 1 which ignores account_auths weight; update the predicate used to compute canRevoke to consider account_auths so authorities that rely on account_auths for sufficient weight aren't incorrectly hidden: for each authority in accountData.owner/active/posting, call canRevokeFromAuthority(auth, without) for candidate removals derived from both key_auths and account_auths (or compute total remaining weight including account_auths when removing a key) and allow revoke if any removal returns true; keep references to canRevoke, accountData, auth.key_auths, auth.account_auths and canRevokeFromAuthority to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx:
- Around line 129-131: The Add/Edit/Remove flow components are being passed
username using non-null assertions (username!) which can throw if activeUser is
undefined; update manage-keys-dialog.tsx to guard before rendering these flows:
either check that username is truthy and return/disable the action UI early
(e.g., don't render AddFlow/EditFlow/RemoveFlow when username is falsy) or
disable the action buttons inside ActionChooser so flows never open without a
defined username; ensure the code paths that reference username use the guarded
value (refer to AddFlow, EditFlow, RemoveFlow and ActionChooser) to avoid
relying on non-null assertions.
---
Nitpick comments:
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx:
- Around line 420-421: The double type assertion on the constructed operation
should be removed by introducing a small helper (e.g., toOperation or
buildOperation) that accepts the operation type string and payload and returns a
properly typed Operation; replace the inline `const op = ["account_update",
opPayload] as unknown as Operation` with a call to that helper and update usages
around buildRevokeKeysOp and the local `op` variable so the Operation type is
produced without `as unknown as Operation`.
- Around line 52-60: The current canRevoke gating uses auth.key_auths.length > 1
which ignores account_auths weight; update the predicate used to compute
canRevoke to consider account_auths so authorities that rely on account_auths
for sufficient weight aren't incorrectly hidden: for each authority in
accountData.owner/active/posting, call canRevokeFromAuthority(auth, without) for
candidate removals derived from both key_auths and account_auths (or compute
total remaining weight including account_auths when removing a key) and allow
revoke if any removal returns true; keep references to canRevoke, accountData,
auth.key_auths, auth.account_auths and canRevokeFromAuthority to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 990fff06-8a0e-43af-9a61-6e54efb80465
📒 Files selected for processing (1)
apps/web/src/app/(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx
Comment on lines
+129
to
+131
| {mode === "add" && ( | ||
| <AddFlow | ||
| username={username!} |
Contributor
There was a problem hiding this comment.
Guard against undefined username before entering flows.
The non-null assertions username! on lines 131, 154, and 181 will throw if activeUser is undefined. While the permissions page likely requires authentication, adding an explicit guard improves robustness.
🛡️ Proposed fix
{mode === "add" && (
+ username ? (
<AddFlow
username={username}
// ... rest of props
/>
+ ) : null
)}Or disable the action buttons when username is falsy in ActionChooser.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/permissions/_components/manage-keys-dialog.tsx
around lines 129 - 131, The Add/Edit/Remove flow components are being passed
username using non-null assertions (username!) which can throw if activeUser is
undefined; update manage-keys-dialog.tsx to guard before rendering these flows:
either check that username is truthy and return/disable the action UI early
(e.g., don't render AddFlow/EditFlow/RemoveFlow when username is falsy) or
disable the action buttons inside ActionChooser so flows never open without a
defined username; ensure the code paths that reference username use the guarded
value (refer to AddFlow, EditFlow, RemoveFlow and ActionChooser) to avoid
relying on non-null assertions.
feruzm
added
the
patch
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