Repository navigation
fix: align remote nostr protocol - #3
Conversation
|
|
Warning Review limit reached
More reviews will be available in 19 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR refactors remote wallet session connections to derive session identifiers deterministically from association public keys, introduces per-session runtime options storage, enforces minimum connection timeouts, and adds explicit handling of relay failure signals (subscription closure and event rejection). ChangesDeterministic Session Identifiers & Enhanced Failure Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
commit: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/browser/create-remote-wallet-session.ts (1)
374-407:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
expiresAtaligned with the enforced minimum timeout.
prepareRemoteWalletSession()clamps the live relay timeout to at least30_000ms, butexpiresAtis still computed from the rawtimeoutMs. For callers that pass a smaller timeout, the session can still be active after the advertised expiry, which is likely to desync any UI countdown or expiry-based cleanup.Compute one effective timeout up front and reuse it for both
expiresAtandprepareRemoteWalletSession().Also applies to: 461-466
🤖 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 `@src/browser/create-remote-wallet-session.ts` around lines 374 - 407, The session's expiresAt uses the raw timeoutMs while prepareRemoteWalletSession enforces a minimum (30_000ms), causing a mismatch; fix by computing an effectiveTimeout once (e.g. const effectiveTimeout = Math.max(timeoutMs, 30_000) or the appropriate MIN constant) and use effectiveTimeout when setting expiresAt and when calling prepareRemoteWalletSession({ session, timeoutMs: effectiveTimeout }); update the same pattern in the other create block referenced (lines ~461-466) so both expiresAt and prepareRemoteWalletSession share the same effective timeout.src/protocol/association-url.ts (1)
81-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
ws://for IPv6 loopback relays.
isLocalRelayHost()does not recognize bracketed IPv6 loopback hosts, so a pairing URL created fromws://[::1]:8080is parsed back aswss://[::1]:8080. That breaks the local-association flow on IPv6 even though::1is explicitly intended to be supported here.Suggested fix
function isLocalRelayHost(hostname: string) { - return hostname === '127.0.0.1' || hostname === '::1' || hostname === 'localhost' + const normalizedHostname = hostname.startsWith('[') && hostname.endsWith(']') + ? hostname.slice(1, -1) + : hostname + + return ( + normalizedHostname === '127.0.0.1' || + normalizedHostname === '::1' || + normalizedHostname === 'localhost' + ) }🤖 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 `@src/protocol/association-url.ts` around lines 81 - 87, Update isLocalRelayHost to accept bracketed IPv6 hostnames by normalizing the input: if hostname starts with '[' and ends with ']', strip the brackets before checking equality. Ensure isLocalRelayHost still compares the normalized value against '127.0.0.1', '::1', and 'localhost' so that inputs like '[::1]' or '[::1]:port' (after URL parsing) correctly return true for local relays.
🤖 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 `@src/browser/create-remote-wallet-session.ts`:
- Around line 461-466: The session identity getter used for connectToNostrRelay
(getIdentity: () => remoteWalletSessionRuntimeOptions.get(session)?.identity ??
getDefaultIdentity()) is not passed into signIn/reauthorize, so
RemoteWalletAuthorizedSession.signIn() → RemoteWalletRpcClient.signIn() →
authorizeRemoteWallet() can fall back to getDefaultIdentity() and present a
different identity; thread the same getter/value through these calls by adding a
getIdentity (or identityGetter) parameter to
RemoteWalletAuthorizedSession.signIn(), RemoteWalletRpcClient.signIn(), and
authorizeRemoteWallet(), update the callers (including the connect-time path and
the other similar site around the 682-688 range) to pass
remoteWalletSessionRuntimeOptions.get(session)?.identity ??
getDefaultIdentity(), and ensure authorizeRemoteWallet() uses the provided
getter and only falls back to getDefaultIdentity() if that getter returns
undefined.
---
Outside diff comments:
In `@src/browser/create-remote-wallet-session.ts`:
- Around line 374-407: The session's expiresAt uses the raw timeoutMs while
prepareRemoteWalletSession enforces a minimum (30_000ms), causing a mismatch;
fix by computing an effectiveTimeout once (e.g. const effectiveTimeout =
Math.max(timeoutMs, 30_000) or the appropriate MIN constant) and use
effectiveTimeout when setting expiresAt and when calling
prepareRemoteWalletSession({ session, timeoutMs: effectiveTimeout }); update the
same pattern in the other create block referenced (lines ~461-466) so both
expiresAt and prepareRemoteWalletSession share the same effective timeout.
In `@src/protocol/association-url.ts`:
- Around line 81-87: Update isLocalRelayHost to accept bracketed IPv6 hostnames
by normalizing the input: if hostname starts with '[' and ends with ']', strip
the brackets before checking equality. Ensure isLocalRelayHost still compares
the normalized value against '127.0.0.1', '::1', and 'localhost' so that inputs
like '[::1]' or '[::1]:port' (after URL parsing) correctly return true for local
relays.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c77b468a-8382-4fc9-8dab-e10c789e56ab
📒 Files selected for processing (7)
src/browser/create-remote-wallet-session.tssrc/node/connect-remote-wallet.tssrc/protocol/association-url.tssrc/protocol/types.tstest/browser/create-remote-wallet-session.test.tstest/node/connect-remote-wallet.test.tstest/protocol/association-url.test.ts
Derive Nostr session routing from the association key and emit spec-shaped pairing URLs. Require CONNECT and SESSION_END control messages, terminate on relay CLOSED/failed OK messages, and cover the protocol behavior with browser and node tests.
a7a03ff to
7b31a26
Compare
|
Addressed the review-body nits as well: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Derive Nostr session routing from the association key and emit spec-shaped pairing URLs.
Require CONNECT and SESSION_END control messages, terminate on relay CLOSED/failed OK messages, and cover the protocol behavior with browser and node tests.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements