Open
Conversation
📝 WalkthroughWalkthroughRefactors JIT broadcast control flow to use async/await and parallel broadcasts, tightens relay transport null checks and readiness handling, adjusts broadcast completion logic to use a ReplaySubject, and adds two NDK broadcast timing tests (offline-relay and zero-relay scenarios). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JIT as JitEngine
participant Sets as RelaySetsEngine
participant Manager as RelayManager
participant Relay as RelayTransport
participant Network as NetworkController
Client->>JIT: request broadcast(event, strategy)
JIT->>Sets: handleEventBroadcast(event, strategy)
Sets->>Manager: select & (re)connect relays (parallel)
Manager->>Relay: connect/reconnect
Relay-->>Manager: connected / failed
Manager-->>Sets: connection results
Sets->>Relay: sendToRelay (for connected transports)
Relay-->>Network: emit publish result
Network-->>Sets: publish responses aggregated
Sets->>JIT: broadcast results
JIT->>Client: return broadcast outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![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
🧹 Nitpick comments (1)
packages/ndk/test/broadcast/broadcast_test.dart (1)
17-40: Consider extracting shared setup/assert flow into a helper.Both tests repeat the same key generation, login, event creation, timing, and threshold assertion. A helper would reduce duplication and make future timeout-case additions easier.
Also applies to: 51-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ndk/test/broadcast/broadcast_test.dart` around lines 17 - 40, Tests duplicate key generation, login, event creation, timing and timeout assertion; extract that shared flow into a helper function (e.g., runBroadcastAndAssertWithin) that performs Bip340.generatePrivateKey(), calls ndk.accounts.loginPrivateKey(privkey:..., pubkey:...), constructs the Nip01Event(pubKey:..., kind:1, content:'', tags:[]), starts a Stopwatch, invokes ndk.broadcast.broadcast(nostrEvent: event), awaits broadcast.broadcastDoneFuture, stops the stopwatch and asserts stopwatch.elapsedMilliseconds is lessThan a given threshold; replace the repeated blocks in both test locations with calls to this helper and pass the threshold as a parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ndk/test/broadcast/broadcast_test.dart`:
- Around line 6-43: The test currently calls await ndk.destroy() only on the
happy path; wrap the test body so Ndk cleanup always runs by ensuring
ndk.destroy() is invoked in a finally block (or use test framework tearDown)
after creating the Ndk instance, so any exceptions from
broadcast.broadcast/broadcastDoneFuture or failed assertions still call
ndk.destroy(); locate the Ndk instantiation (Ndk(...)), the broadcast invocation
(ndk.broadcast.broadcast and broadcastDoneFuture) and ensure destroy() is called
in finally/tearDown.
---
Nitpick comments:
In `@packages/ndk/test/broadcast/broadcast_test.dart`:
- Around line 17-40: Tests duplicate key generation, login, event creation,
timing and timeout assertion; extract that shared flow into a helper function
(e.g., runBroadcastAndAssertWithin) that performs Bip340.generatePrivateKey(),
calls ndk.accounts.loginPrivateKey(privkey:..., pubkey:...), constructs the
Nip01Event(pubKey:..., kind:1, content:'', tags:[]), starts a Stopwatch, invokes
ndk.broadcast.broadcast(nostrEvent: event), awaits
broadcast.broadcastDoneFuture, stops the stopwatch and asserts
stopwatch.elapsedMilliseconds is lessThan a given threshold; replace the
repeated blocks in both test locations with calls to this helper and pass the
threshold as a parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5faa1b96-231f-4dbc-8479-47488ca37fcf
📒 Files selected for processing (1)
packages/ndk/test/broadcast/broadcast_test.dart
Comment on lines
+6
to
+43
| test( | ||
| 'broadcast should complete quickly even with one offline relay', | ||
| () async { | ||
| final ndk = Ndk( | ||
| NdkConfig( | ||
| eventVerifier: Bip340EventVerifier(), | ||
| cache: MemCacheManager(), | ||
| bootstrapRelays: ["ws://localhost:25565"], | ||
| ), | ||
| ); | ||
|
|
||
| final keyPair = Bip340.generatePrivateKey(); | ||
| ndk.accounts.loginPrivateKey( | ||
| privkey: keyPair.privateKey!, | ||
| pubkey: keyPair.publicKey, | ||
| ); | ||
|
|
||
| final event = Nip01Event( | ||
| pubKey: keyPair.publicKey, | ||
| kind: 1, | ||
| content: '', | ||
| tags: [], | ||
| ); | ||
|
|
||
| final stopwatch = Stopwatch()..start(); | ||
|
|
||
| final broadcast = ndk.broadcast.broadcast(nostrEvent: event); | ||
| await broadcast.broadcastDoneFuture; | ||
|
|
||
| stopwatch.stop(); | ||
|
|
||
| expect( | ||
| stopwatch.elapsedMilliseconds, | ||
| lessThan(5000), | ||
| ); | ||
|
|
||
| await ndk.destroy(); | ||
| }, |
There was a problem hiding this comment.
Ensure Ndk cleanup runs even on failures.
await ndk.destroy() (Line 42, Line 79) is only reached on the happy path. If broadcastDoneFuture throws or an assertion fails, cleanup is skipped and can leak state across tests.
💡 Proposed fix
test(
'broadcast should complete quickly even with one offline relay',
() async {
final ndk = Ndk(
NdkConfig(
eventVerifier: Bip340EventVerifier(),
cache: MemCacheManager(),
bootstrapRelays: ["ws://localhost:25565"],
),
);
+ addTearDown(() async => ndk.destroy());
final keyPair = Bip340.generatePrivateKey();
ndk.accounts.loginPrivateKey(
privkey: keyPair.privateKey!,
pubkey: keyPair.publicKey,
);
@@
expect(
stopwatch.elapsedMilliseconds,
lessThan(5000),
);
-
- await ndk.destroy();
},
);
@@
test(
'broadcast to 0 relay should not time out',
() async {
final ndk = Ndk.emptyBootstrapRelaysConfig();
+ addTearDown(() async => ndk.destroy());
@@
expect(
stopwatch.elapsedMilliseconds,
lessThan(5000),
);
-
- await ndk.destroy();
},
);Also applies to: 46-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ndk/test/broadcast/broadcast_test.dart` around lines 6 - 43, The
test currently calls await ndk.destroy() only on the happy path; wrap the test
body so Ndk cleanup always runs by ensuring ndk.destroy() is invoked in a
finally block (or use test framework tearDown) after creating the Ndk instance,
so any exceptions from broadcast.broadcast/broadcastDoneFuture or failed
assertions still call ndk.destroy(); locate the Ndk instantiation (Ndk(...)),
the broadcast invocation (ndk.broadcast.broadcast and broadcastDoneFuture) and
ensure destroy() is called in finally/tearDown.
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
`@packages/ndk/lib/domain_layer/usecases/jit_engine/relay_jit_broadcast_strategies/relay_jit_broadcast_other_read.dart`:
- Around line 91-94: The code uses
relayManager.connectedRelays.firstWhere((element) => element.url == relayUrl)
which can throw a StateError if no relay matches; wrap that lookup in a
try-catch (or use firstWhere with orElse) to handle the missing-relay case, and
only call sendToRelay(relay: relay) when a relay is found; ensure the catch logs
or handles the error similarly to relay_jit_broadcast_specific.dart so behavior
is consistent with other strategies.
In
`@packages/ndk/lib/domain_layer/usecases/jit_engine/relay_jit_broadcast_strategies/relay_jit_broadcast_own.dart`:
- Around line 82-85: The relay lookup using
relayManager.connectedRelays.firstWhere (the block that finds a relay by
relayUrl before calling sendToRelay) can throw StateError if no match is found;
wrap that lookup in a try-catch (matching the pattern used in
relay_jit_broadcast_specific.dart) so if the relay isn't present after
connectRelay we catch the error, skip/continue the loop and do not call
sendToRelay, and optionally log the missing-relay case; update the code around
firstWhere and sendToRelay to handle the exception gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35785250-d0da-418e-8cdd-e5cdf17196ec
📒 Files selected for processing (7)
packages/ndk/lib/domain_layer/entities/broadcast_state.dartpackages/ndk/lib/domain_layer/usecases/jit_engine/jit_engine.dartpackages/ndk/lib/domain_layer/usecases/jit_engine/relay_jit_broadcast_strategies/relay_jit_broadcast_other_read.dartpackages/ndk/lib/domain_layer/usecases/jit_engine/relay_jit_broadcast_strategies/relay_jit_broadcast_own.dartpackages/ndk/lib/domain_layer/usecases/jit_engine/relay_jit_broadcast_strategies/relay_jit_broadcast_specific.dartpackages/ndk/lib/domain_layer/usecases/relay_manager.dartpackages/ndk/lib/domain_layer/usecases/relay_sets_engine.dart
Comment on lines
+91
to
+94
| final relay = relayManager.connectedRelays | ||
| .firstWhere((element) => element.url == relayUrl); | ||
|
|
||
| sendToRelay(relay: relay); | ||
| }); | ||
| sendToRelay(relay: relay); |
There was a problem hiding this comment.
Missing error handling for relay lookup after connection.
Same issue as relay_jit_broadcast_own.dart: firstWhere can throw StateError if the relay isn't found in connectedRelays. This should be wrapped in try-catch for consistency with relay_jit_broadcast_specific.dart.
Proposed fix: wrap in try-catch for consistency
if (!success.first) {
relayManager.failBroadcast(
eventToPublish.id,
relayUrl,
"connection failed",
);
continue;
}
- final relay = relayManager.connectedRelays
- .firstWhere((element) => element.url == relayUrl);
-
- sendToRelay(relay: relay);
+ try {
+ final relay = relayManager.connectedRelays
+ .firstWhere((element) => element.url == relayUrl);
+ sendToRelay(relay: relay);
+ } catch (e) {
+ relayManager.failBroadcast(
+ eventToPublish.id,
+ relayUrl,
+ "relay not found after connection",
+ );
+ }📝 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
| final relay = relayManager.connectedRelays | |
| .firstWhere((element) => element.url == relayUrl); | |
| sendToRelay(relay: relay); | |
| }); | |
| sendToRelay(relay: relay); | |
| if (!success.first) { | |
| relayManager.failBroadcast( | |
| eventToPublish.id, | |
| relayUrl, | |
| "connection failed", | |
| ); | |
| continue; | |
| } | |
| try { | |
| final relay = relayManager.connectedRelays | |
| .firstWhere((element) => element.url == relayUrl); | |
| sendToRelay(relay: relay); | |
| } catch (e) { | |
| relayManager.failBroadcast( | |
| eventToPublish.id, | |
| relayUrl, | |
| "relay not found after connection", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/ndk/lib/domain_layer/usecases/jit_engine/relay_jit_broadcast_strategies/relay_jit_broadcast_other_read.dart`
around lines 91 - 94, The code uses
relayManager.connectedRelays.firstWhere((element) => element.url == relayUrl)
which can throw a StateError if no relay matches; wrap that lookup in a
try-catch (or use firstWhere with orElse) to handle the missing-relay case, and
only call sendToRelay(relay: relay) when a relay is found; ensure the catch logs
or handles the error similarly to relay_jit_broadcast_specific.dart so behavior
is consistent with other strategies.
Comment on lines
+82
to
+85
| final relay = relayManager.connectedRelays | ||
| .firstWhere((element) => element.url == relayUrl); | ||
|
|
||
| sendToRelay(relay: relay); | ||
| }); | ||
| sendToRelay(relay: relay); |
There was a problem hiding this comment.
Missing error handling for relay lookup after connection.
firstWhere throws StateError if no matching element is found. Unlike relay_jit_broadcast_specific.dart (lines 64-74), this code lacks a try-catch wrapper. If connectRelay succeeds but the relay isn't in connectedRelays, the loop will throw instead of continuing gracefully.
Proposed fix: wrap in try-catch for consistency
final success = await relayManager.connectRelay(
dirtyUrl: relayUrl,
connectionSource: ConnectionSource.broadcastOwn,
);
if (!success.first) {
relayManager.failBroadcast(
eventToPublish.id,
relayUrl,
"connection failed",
);
continue;
}
- final relay = relayManager.connectedRelays
- .firstWhere((element) => element.url == relayUrl);
-
- sendToRelay(relay: relay);
+ try {
+ final relay = relayManager.connectedRelays
+ .firstWhere((element) => element.url == relayUrl);
+ sendToRelay(relay: relay);
+ } catch (e) {
+ relayManager.failBroadcast(
+ eventToPublish.id,
+ relayUrl,
+ "relay not found after connection",
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/ndk/lib/domain_layer/usecases/jit_engine/relay_jit_broadcast_strategies/relay_jit_broadcast_own.dart`
around lines 82 - 85, The relay lookup using
relayManager.connectedRelays.firstWhere (the block that finds a relay by
relayUrl before calling sendToRelay) can throw StateError if no match is found;
wrap that lookup in a try-catch (matching the pattern used in
relay_jit_broadcast_specific.dart) so if the relay isn't present after
connectRelay we catch the error, skip/continue the loop and do not call
sendToRelay, and optionally log the missing-relay case; update the code around
firstWhere and sendToRelay to handle the exception gracefully.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #494 +/- ##
==========================================
+ Coverage 76.74% 76.95% +0.21%
==========================================
Files 153 153
Lines 6407 6401 -6
==========================================
+ Hits 4917 4926 +9
+ Misses 1490 1475 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1-leo
approved these changes
Mar 25, 2026
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
2 participants
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