Broadcast mode in SDK#764
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds Hive Unified Wallet Protocol support and user-preferred extension selection to web login; threads an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant LoginUI as Login Component
participant ExtDetect as Extension Detector
participant Modal as Extension Modal
participant Storage as localStorage
participant Extension as Wallet Extension
User->>LoginUI: Click "extension" login
LoginUI->>ExtDetect: getDetectedExtensions()
ExtDetect-->>LoginUI: detectedExtensions[]
alt none or multiple without preferred
LoginUI->>Modal: open extensions info / selection
Modal-->>User: show options or links
User->>Modal: select extId
Modal->>Storage: setPreferredExtensionId(extId)
Storage-->>Modal: saved
Modal->>LoginUI: handleSelectExtension(extId)
end
LoginUI->>Extension: signBufferWithExtension(preferredId)
Extension-->>LoginUI: signed response
LoginUI-->>User: Complete login
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/utils/hive-extensions.ts (1)
153-235:⚠️ Potential issue | 🟡 MinorSilent fallback when the preferred extension is unavailable.
In both
signBufferWithExtensionandbroadcastWithExtension, ifpreferredId(or the persisted value) resolves but the corresponding instance is currentlynull(e.g., user disabled/uninstalled the extension after saving the preference), the code silently falls through togetKeychainLikeInstance()/getPeakVaultInstance()auto-detection. The user who explicitly picked Peak Vault may suddenly be prompted by Keychain instead, with no indication why.Consider either rejecting explicitly with a clear "preferred extension no longer available" error, or at minimum clearing the stored preference (
setPreferredExtensionId(null)) when the preferred instance can't be resolved, so the selector reopens on the next attempt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/utils/hive-extensions.ts` around lines 153 - 235, When a user-selected extension (extId from getPreferredExtensionId() or preferredId) resolves to null, clear the persisted preference instead of silently falling back: inside signBufferWithExtension and broadcastWithExtension, after determining extId and calling getPeakVaultInstance(), getHiveKeeperInstance(), or getKeychainInstance() and finding the instance is null, call setPreferredExtensionId(null) (and then continue with the existing fallback/autodetect logic or return a clear rejection if you prefer). Update both functions (referencing signBufferWithExtension, broadcastWithExtension, getPeakVaultInstance, getHiveKeeperInstance, getKeychainInstance, getKeychainLikeInstance, setPreferredExtensionId, preferredId, getPreferredExtensionId) so the stored preference is cleared when the desired extension is unavailable to force the selector to reopen on next attempt.
🧹 Nitpick comments (2)
apps/web/src/utils/hive-extensions.ts (1)
203-234: Minor duplication of the Peak Vault owner-key guard.The owner-key rejection lives both in the preferred-branch (line 207) and in the legacy auto-detect path (lines 226–228). Consider hoisting it to a single guard at the top of
broadcastWithExtensionwhen the resolved extension ends up being Peak Vault, or extracting a helper. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/utils/hive-extensions.ts` around lines 203 - 234, The duplicate Peak Vault owner-key check should be consolidated: in broadcastWithExtension resolve the target extension once (use extId/preferredId logic and getPeakVaultInstance()) and then perform a single guard that rejects if the resolved extension is Peak Vault and keyType === "owner"; after that dispatch to broadcastViaPeakVault or broadcastViaKeychain as appropriate. Alternatively extract a small helper (e.g., isPeakVault(instance) or rejectIfPeakVaultOwner(instance, keyType)) and replace both checks in the branches (references: extId, preferredId, getPeakVaultInstance, keyType, broadcastViaPeakVault, broadcastViaKeychain).apps/web/src/features/shared/login/login.tsx (1)
62-67: SimplifyextensionLabel.The outer and inner
elsebranches both returni18next.t("login.extensions"), so the ternary collapses to:Proposed change
- const extensionLabel = detectedExtensions.length > 0 - ? i18next.t("login.extensions") - : hasExtensions - ? i18next.t("login.keychain-mobile") - : i18next.t("login.extensions"); + const extensionLabel = + detectedExtensions.length === 0 && useKeychainMobile + ? i18next.t("login.keychain-mobile") + : i18next.t("login.extensions");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/login/login.tsx` around lines 62 - 67, The nested ternary assigning extensionLabel is redundant because both the outer and inner else return i18next.t("login.extensions"); simplify by computing extensionLabel from detectedExtensions.length and useKeychainMobile directly: check detectedExtensions.length first (return i18next.t("login.extensions") if >0), otherwise return i18next.t("login.keychain-mobile") only when useKeychainMobile is true, else return i18next.t("login.extensions"); update the expression around the symbols extensionLabel, detectedExtensions, useKeychainMobile and the i18next.t calls accordingly to remove the duplicate branch.
🤖 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/features/shared/login/login.tsx`:
- Around line 189-223: The extension Button is incorrectly gated by username
which prevents opening the install-info modal when no username is entered;
update the Button so its disabled prop only depends on isLoginByKeychainPending
(remove !username) and change its onClick handler to call handleExtensionLogin()
when username is present but otherwise open the install/info modal (e.g., call a
showInstallInfoModal or handleShowInstallInfo function). Locate the Button, its
onClick, disabled and icon usage as well as the handleExtensionLogin,
detectedExtensions and extensionLabel symbols and implement the conditional
click routing so the info modal is accessible even without a username while
preserving the login flow when a username exists.
---
Outside diff comments:
In `@apps/web/src/utils/hive-extensions.ts`:
- Around line 153-235: When a user-selected extension (extId from
getPreferredExtensionId() or preferredId) resolves to null, clear the persisted
preference instead of silently falling back: inside signBufferWithExtension and
broadcastWithExtension, after determining extId and calling
getPeakVaultInstance(), getHiveKeeperInstance(), or getKeychainInstance() and
finding the instance is null, call setPreferredExtensionId(null) (and then
continue with the existing fallback/autodetect logic or return a clear rejection
if you prefer). Update both functions (referencing signBufferWithExtension,
broadcastWithExtension, getPeakVaultInstance, getHiveKeeperInstance,
getKeychainInstance, getKeychainLikeInstance, setPreferredExtensionId,
preferredId, getPreferredExtensionId) so the stored preference is cleared when
the desired extension is unavailable to force the selector to reopen on next
attempt.
---
Nitpick comments:
In `@apps/web/src/features/shared/login/login.tsx`:
- Around line 62-67: The nested ternary assigning extensionLabel is redundant
because both the outer and inner else return i18next.t("login.extensions");
simplify by computing extensionLabel from detectedExtensions.length and
useKeychainMobile directly: check detectedExtensions.length first (return
i18next.t("login.extensions") if >0), otherwise return
i18next.t("login.keychain-mobile") only when useKeychainMobile is true, else
return i18next.t("login.extensions"); update the expression around the symbols
extensionLabel, detectedExtensions, useKeychainMobile and the i18next.t calls
accordingly to remove the duplicate branch.
In `@apps/web/src/utils/hive-extensions.ts`:
- Around line 203-234: The duplicate Peak Vault owner-key check should be
consolidated: in broadcastWithExtension resolve the target extension once (use
extId/preferredId logic and getPeakVaultInstance()) and then perform a single
guard that rejects if the resolved extension is Peak Vault and keyType ===
"owner"; after that dispatch to broadcastViaPeakVault or broadcastViaKeychain as
appropriate. Alternatively extract a small helper (e.g., isPeakVault(instance)
or rejectIfPeakVaultOwner(instance, keyType)) and replace both checks in the
branches (references: extId, preferredId, getPeakVaultInstance, keyType,
broadcastViaPeakVault, broadcastViaKeychain).
🪄 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: 9df0ee65-2d1e-4694-a6a4-0e40ec6e9aae
📒 Files selected for processing (9)
apps/web/src/features/shared/login/login.tsxapps/web/src/utils/hive-extensions.tspackages/sdk/src/modules/accounts/mutations/use-claim-account.tspackages/sdk/src/modules/posts/mutations/use-comment.tspackages/sdk/src/modules/posts/mutations/use-reblog.tspackages/sdk/src/modules/posts/mutations/use-update-reply.tspackages/sdk/src/modules/posts/mutations/use-vote.tspackages/sdk/src/modules/wallet/mutations/use-claim-engine-rewards.tspackages/sdk/src/modules/wallet/mutations/use-transfer-to-savings.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/sdk/src/modules/posts/mutations/use-comment.ts
- packages/sdk/src/modules/wallet/mutations/use-transfer-to-savings.ts
- packages/sdk/src/modules/posts/mutations/use-update-reply.ts
- packages/sdk/src/modules/posts/mutations/use-vote.ts
- packages/sdk/src/modules/wallet/mutations/use-claim-engine-rewards.ts
- packages/sdk/src/modules/accounts/mutations/use-claim-account.ts
- packages/sdk/src/modules/posts/mutations/use-reblog.ts
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