SDK: auth.broadcast is not deprecated, and a guard so it is not misread again - #1378
Conversation
…ad again Checked whether AuthContext.broadcast could be deleted now that #1376 migrated the four mutations off it. It cannot, and the reason matters more than the answer. Mobile is already fully V2: useAuthContext returns { adapter, enableFallback } and never sets broadcast. But the web app has two live callers, and neither is legacy: - use-login-by-key grants posting permission DURING login, before the user exists to the adapter, so the key comes from a ref. - wallet-operations-sign dispatches on a signing method the user picks mid-flow, which the adapter has no way to know about. Both are the custom-broadcaster case, which useBroadcastMutation supports as case 'custom', the last link of the default fallback chain. It is a feature. So the @deprecated tag was wrong, and plausibly the cause. It said to use broadcastWithKeychain instead, which reads as "this is the old way to broadcast", and four mutations duly treated it as the keychain path: they checked auth?.broadcast and threw when it was absent. Every field on AuthContext is optional, so AuthContextV2 satisfies it structurally and the checks compiled everywhere; each failed only when a real user arrived. The doc now says what the field is for, names both legitimate callers, and says plainly never to use it to detect Keychain. The guard is a syntax scan, since the type system cannot express this. It reads the AST rather than the text because auth.broadcast now appears in prose in these files and a text search reports the comments explaining why not to use it. Sanctioned readers are listed with reasons; anything else fails. Confirmed by reintroducing the exact bug in use-account-revoke-posting: the scan names the file.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe SDK documents ChangesCustom broadcast controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 2
🧹 Nitpick comments (1)
packages/sdk/src/modules/core/custom-broadcast-usage.spec.ts (1)
74-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the repository-size threshold with stable assertions.
files.length > 50depends on the current module count. Normal cleanup can fail the test, and an incorrect directory with more than 50 files can still pass. Assert that both sanctioned files are discovered instead.Proposed test change
- expect(files.length).toBeGreaterThan(50); + expect(files).toContain( + join(MODULES, "core/mutations/use-broadcast-mutation.ts") + ); + expect(files).toContain( + join(MODULES, "core/mutations/broadcast-json.ts") + );🤖 Prompt for 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. In `@packages/sdk/src/modules/core/custom-broadcast-usage.spec.ts` around lines 74 - 77, Replace the files.length threshold assertion in the “finds the modules to scan” test with stable assertions that verify both sanctioned files are present in files, using their established filenames or symbols. Preserve the test’s purpose of confirming the intended directory is scanned without relying on the repository’s total module count.
🤖 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 `@packages/sdk/src/modules/core/custom-broadcast-usage.spec.ts`:
- Around line 57-65: Extend the AST detector in the visit function to flag every
read of AuthContext.broadcast, including string-literal element access such as
auth["broadcast"] and broadcast property bindings in object destructuring.
Normalize parenthesized and type-asserted expressions before checking the
receiver so (auth as AuthContext).broadcast and (<AuthContext>auth).broadcast
are covered, while preserving the existing direct and optional property-access
detection.
In `@packages/sdk/src/modules/core/types/auth.ts`:
- Around line 30-31: Update the contract documentation near the caller-supplied
broadcaster declaration to use the complete wording “A caller-supplied
broadcaster for signing operations that the platform adapter cannot perform.”
---
Nitpick comments:
In `@packages/sdk/src/modules/core/custom-broadcast-usage.spec.ts`:
- Around line 74-77: Replace the files.length threshold assertion in the “finds
the modules to scan” test with stable assertions that verify both sanctioned
files are present in files, using their established filenames or symbols.
Preserve the test’s purpose of confirming the intended directory is scanned
without relying on the repository’s total module count.
🪄 Autofix
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 Plus
Run ID: 4d7d1b7c-b9e4-4a63-99a1-ed55dce8ae17
📒 Files selected for processing (2)
packages/sdk/src/modules/core/custom-broadcast-usage.spec.tspackages/sdk/src/modules/core/types/auth.ts
1 participant
You asked whether we could delete
AuthContext.broadcastnow that #1376 migrated the four mutations off it. We can't, and the reason is more useful than the answer.What I checked
Mobile is already fully V2.
useAuthContext()returns{ adapter, enableFallback }and never setsbroadcast. Nothing invision-mobilereads it.The web app has two live callers, and neither is legacy:
use-login-by-key.ts:34grants posting permission during login, before the user exists to the adapter, so the key comes from a ref.wallet-operations-sign.tsx:57dispatches on a signing method the user picks mid-flow, which the adapter has no way to know about.Both are the custom-broadcaster case, which
useBroadcastMutationsupports ascase 'custom'— the last link of the default fallback chain['key', 'hiveauth', 'hivesigner', 'keychain', 'custom']. It is a feature, not a leftover.So the
@deprecatedtag was wrong, and it plausibly caused the bugIt read:
Which parses as "this is the old way to broadcast". Four mutations duly treated it as the keychain path: they checked
auth?.broadcastand threw when it was absent. Every field onAuthContextis optional, soAuthContextV2satisfies it structurally, the checks compiled everywhere, and each site failed only when a real user arrived. That is how four accumulated before one became a Sentry issue.The doc now says what the field is actually for, names both legitimate callers so the next reader can tell their case apart, and says plainly never to use it to detect Keychain.
The guard
A syntax scan, because the type system cannot express "optional, but not the thing you want". It reads the AST, not the text, because
auth.broadcastnow appears in prose throughout these files and a text search reports the comments explaining why not to use it.Sanctioned readers are listed with reasons:
use-broadcast-mutation.tsownscase 'custom'broadcast-json.tskeeps it as the first branch of its own chain so V1 callers still workAnything else fails, with the offending file named. Mutation-checked by reintroducing the exact bug in
use-account-revoke-posting.ts, which the scan reports by name.When it can actually be deleted
When those two web call sites have another way to supply signing. Until then, deleting the field would break login-by-key and the wallet signer. Worth revisiting if the adapter ever grows a "sign with this key I am handing you" method.
681 tests pass, 48 files. Typecheck clean.
Summary by CodeRabbit
Documentation
Bug Fixes
Chores