fix: upgrade dart_nostr to resolve relay connection blocking bug#560
fix: upgrade dart_nostr to resolve relay connection blocking bug#560
Conversation
WalkthroughAdded architecture documentation and small runtime config changes supporting an in‑progress upgrade of Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/architecture/DART_NOSTR_UPGRADE.md (2)
376-398: Document the operation timeout reduction as an intentional change.The config change splits the 30s timeout into 5s (connection) + 20s (operation), effectively reducing the operation timeout by 5 seconds. While this is likely intentional and beneficial (operations fail faster), consider explicitly noting this behavior change in the documentation to ensure it's a deliberate decision rather than an oversight.
For example, add a note after line 386:
**Note:** This splits the original 30s into 5s connection + 20s operation. Operations (publish, fetch) now timeout 5s faster, which is acceptable given the dramatically reduced connection time (60-120s → 5s for dead relays).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/DART_NOSTR_UPGRADE.md` around lines 376 - 398, Add an explicit note documenting that the original nostrConnectionTimeout(30s) was split into Config.relayConnectionTimeout(5s) and Config.nostrOperationTimeout(20s), and that this reduces operation timeouts (publishEvent/fetchEvents) by 5s; update the docs near the Config example (after the code block showing relayConnectionTimeout and nostrOperationTimeout) to state this behavioral change and the rationale (faster failure on dead relays due to 5s handshake vs prior 60–120s connection attempts).
447-458: Consider adding a caveat about hard-coded line number references.Throughout the document (e.g., lines 189-190, 288-305), you reference specific line numbers in source files. While this provides precision for the current review, these references will become stale as the code evolves.
Consider adding a note in Appendix C or near the top of the document:
**Note:** File and line number references are accurate as of commit [commit_hash] on the `feat/upgrade-dart-nostr-parallel-relay-connections` branch. Line numbers may shift as the code evolves.This helps future readers understand that the references are a point-in-time snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/DART_NOSTR_UPGRADE.md` around lines 447 - 458, Add a brief caveat that hard-coded file and line number references (e.g., those pointing to lib/core/config.dart, lib/services/nostr_service.dart, lib/shared/providers/app_init_provider.dart, etc.) are snapshot-specific: insert a note in Appendix C (or at the top) stating that file/line references are accurate as of a specific commit and branch (e.g., commit_hash on feat/upgrade-dart-nostr-parallel-relay-connections) and that line numbers may shift as the code evolves; include guidance to update the commit hash when updating the document and prefer linking to commits or using file paths/anchors instead of fixed line numbers where possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/DART_NOSTR_UPGRADE.md`:
- Line 167: The heading "### 5.1 bip340: ^0.1.0 → ^0.3.0" is misleading because
the app currently overrides bip340 to ^0.2.0 in pubspec.yaml and the upstream
dart_nostr 9.1.1 depends on ^0.1.0; update the heading to "### 5.1 bip340:
^0.2.0 → ^0.3.0 (via dependency_override removal)" or keep the original heading
but add a one-line clarifying note immediately after it stating that the app
currently pins bip340 to ^0.2.0 via pubspec.yaml and thus the effective upgrade
is ^0.2.0 → ^0.3.0.
---
Nitpick comments:
In `@docs/architecture/DART_NOSTR_UPGRADE.md`:
- Around line 376-398: Add an explicit note documenting that the original
nostrConnectionTimeout(30s) was split into Config.relayConnectionTimeout(5s) and
Config.nostrOperationTimeout(20s), and that this reduces operation timeouts
(publishEvent/fetchEvents) by 5s; update the docs near the Config example (after
the code block showing relayConnectionTimeout and nostrOperationTimeout) to
state this behavioral change and the rationale (faster failure on dead relays
due to 5s handshake vs prior 60–120s connection attempts).
- Around line 447-458: Add a brief caveat that hard-coded file and line number
references (e.g., those pointing to lib/core/config.dart,
lib/services/nostr_service.dart, lib/shared/providers/app_init_provider.dart,
etc.) are snapshot-specific: insert a note in Appendix C (or at the top) stating
that file/line references are accurate as of a specific commit and branch (e.g.,
commit_hash on feat/upgrade-dart-nostr-parallel-relay-connections) and that line
numbers may shift as the code evolves; include guidance to update the commit
hash when updating the document and prefer linking to commits or using file
paths/anchors instead of fixed line numbers where possible.
🪄 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: 4dca15f0-b1f8-45f2-b932-86870ab36f65
📒 Files selected for processing (1)
docs/architecture/DART_NOSTR_UPGRADE.md
…er-relay timeout - Switch dart_nostr from pub.dev 9.1.1 to git ref ca07ddd - Split nostrConnectionTimeout (30s) into relayConnectionTimeout (5s) and nostrOperationTimeout (20s) - Remove bip340 dependency_override (no longer needed with bip340 ^0.3.0 from upstream)
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Four-file change: dependency pin, two config updates, and architecture documentation. All correct.
pubspec.yaml — git dependency pin
dart_nostr:
git:
url: https://github.com/anasfik/nostr.git
ref: ca07dddPinning to a specific commit (ca07ddd, merge commit of PR #19) is the right call. The fix for the blocking bug is only available on main — last pub.dev release (9.2.5) predates it. A pinned git ref gives reproducible builds without forking, and is straightforward to revert or bump later. Removing the dependency_overrides for bip340 is also correct: dart_nostr main now depends on bip340: ^0.3.0 which already includes the bigToBytes padding fix that the override was working around.
The doc accurately covers why waiting for a pub.dev release is unreasonable given the blocking impact on UX, and why forking adds unnecessary maintenance overhead.
lib/core/config.dart — split timeouts
static const Duration relayConnectionTimeout = Duration(seconds: 5);
static const Duration nostrOperationTimeout = Duration(seconds: 20);Renaming nostrConnectionTimeout to nostrOperationTimeout and adding relayConnectionTimeout is semantically correct. These are fundamentally different things:
relayConnectionTimeout: WebSocket TCP handshake per relay — 5s is reasonable (matches the upstream_connectionTimeoutdefault being activated)nostrOperationTimeout: EOSE/publish acknowledgment — 20s is appropriate for real-world relay latency
lib/services/nostr_service.dart — correct timeout routing
// relay init
connectionTimeout: Config.relayConnectionTimeout,
// publish
timeout: Config.nostrOperationTimeout,
// subscribe
timeout: Config.nostrOperationTimeout,Each call site now uses the semantically correct timeout. This is the key part: previously nostrConnectionTimeout was passed to init() but never reached the WebSocket layer (dead code in dart_nostr 9.1.1). With the upgraded library, connectionTimeout in init() now flows through to IOWebSocketChannel.connect(connectTimeout: ...). So relayConnectionTimeout = 5s will actually enforce a 5s per-relay timeout now.
docs/architecture/DART_NOSTR_UPGRADE.md
Well-written. The root cause analysis is accurate (sequential for+await loop, dead timeout code path, _connectionTimeout field existing but never passed to WebSocketChannel.connect). The before/after timing table makes the impact concrete. The dependency impact section (bip340 0.2.0→0.3.0, verify() nullability) is thorough. The rollback section is useful. One minor note: the doc header says "Status: In progress" — this should probably be updated to "Merged" or "Complete" before merge.
Summary
The blocking relay bug is real and the fix is correct — parallelizing connections with Future.wait(..., eagerError: false) and using IOWebSocketChannel.connect(connectTimeout: ...) eliminates both the sequential blocking and the dead timeout code path. The dependency pin is clean and reversible. All timeout routing in the service layer is now semantically accurate.
LGTM.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/architecture/DART_NOSTR_UPGRADE.md (2)
140-140: Optional: Add comma after year for style consistency.Similar to line 67, static analysis suggests:
-- Last pub.dev release (9.2.5) was February 9, 2026 +- Last pub.dev release (9.2.5) was February 9, 2026,This is a minor style suggestion and can be safely ignored if your style guide doesn't require it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/DART_NOSTR_UPGRADE.md` at line 140, The sentence "Last pub.dev release (9.2.5) was February 9, 2026" needs a style tweak: insert a comma after the year so it reads "February 9, 2026," to match the comma usage on line 67; update that sentence in the DART_NOSTR_UPGRADE.md content accordingly.
67-67: Optional: Add comma after year for style consistency.Static analysis suggests adding a comma after "2026" for style guide compliance:
-**Merged:** March 31, 2026 +**Merged:** March 31, 2026,This is a minor style suggestion and can be safely ignored if your style guide doesn't require it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/DART_NOSTR_UPGRADE.md` at line 67, Update the "Merged: March 31, 2026" header to include a comma after the year for style consistency by changing the string "Merged: March 31, 2026" to "Merged: March 31, 2026," wherever it appears (e.g., the document header line containing that exact text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/DART_NOSTR_UPGRADE.md`:
- Around line 168-224: In the "5.1 bip340" section update the wording that
currently says "the app overrides bip340 to `^0.2.0` in pubspec.yaml" to reflect
that the override was previously present and has been removed (or, if this PR
removes it, state that this PR removes it); also clarify the YAML snippet
showing the override is illustrating the prior/removed override (not the current
pubspec.yaml). Locate the "5.1 bip340" header and the YAML block example and
change the tense/notes accordingly so the doc matches the actual repo state (or
explicitly state "removed in an earlier commit / removed in this PR" as
appropriate).
---
Nitpick comments:
In `@docs/architecture/DART_NOSTR_UPGRADE.md`:
- Line 140: The sentence "Last pub.dev release (9.2.5) was February 9, 2026"
needs a style tweak: insert a comma after the year so it reads "February 9,
2026," to match the comma usage on line 67; update that sentence in the
DART_NOSTR_UPGRADE.md content accordingly.
- Line 67: Update the "Merged: March 31, 2026" header to include a comma after
the year for style consistency by changing the string "Merged: March 31, 2026"
to "Merged: March 31, 2026," wherever it appears (e.g., the document header line
containing that exact text).
🪄 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: 3ad65339-ec85-4f8d-95fd-8472a59b7c89
📒 Files selected for processing (1)
docs/architecture/DART_NOSTR_UPGRADE.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/DART_NOSTR_UPGRADE.md`:
- Line 67: Update month-day-year dates to include a trailing comma for style
consistency: change "Merged: March 31, 2026" to "Merged: March 31, 2026," and
similarly add commas after years for any other dates like "February 9, 2026"
(also referenced at 140-140) in the DART_NOSTR_UPGRADE.md content so all inline
dates follow the "Month D, YYYY," format.
🪄 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: 7b5fcd5d-2961-434a-ab62-c63d18da4a54
📒 Files selected for processing (1)
docs/architecture/DART_NOSTR_UPGRADE.md
There was a problem hiding this comment.
Re-approving after the doc-only commit (e2afb35a) that addressed the minor note from the previous review: "Status: In progress" updated to "Status: Complete", and the dependency_overrides description clarified. No code changes. All previous findings stand — LGTM.
3 participants
fix #548
dart_nostrfrom 9.1.1 (pub.dev) to main branch via git dependency, which includes our merged PR (fix: parallel relay connections with per-relay timeout anasfik/nostr#19) that fixes the relay connection blocking bugrelayConnectionTimeout(5s) andnostrOperationTimeout(20s)bip340: ^0.2.0dependency override (no longer needed with bip340 ^0.3.0)The problem
When any configured relay was unreachable, the app hung for 60-120 seconds per dead relay during startup and every subsequent operation. Root causes inside dart_nostr: sequential relay connections, connection timeout parameter was dead code (never reached WebSocket layer), and every send/fetch/subscribe re-triggered the blocking loop.
The fix (upstream PR anasfik/nostr#19)
WebSocketChannel.connect()→IOWebSocketChannel.connect(connectTimeout: 5s)— activates per-relay timeoutfor/awaitloop →Future.wait()— parallel relay connectionsDependency changes analyzed
Full risk analysis:
docs/architecture/DART_NOSTR_UPGRADE.mdTest plan
flutter analyze— zero issuesflutter test— 322 tests passTemporary git dependency
dart_nostrmain includes our fix but no new version has been published to pub.dev. Once anasfik publishes a new release, replace the git dependency with the pub.dev version.Summary by CodeRabbit