Consume NIP-19 relay hints for event fetching #3477
Conversation
danieldaquino
added
the
pr-in-queue
alltheseas
force-pushed
the
relay-hints
branch
from
e436eba to
8aa7a75
Compare
📝 WalkthroughWalkthroughAdds end‑to‑end NIP‑19/NIP‑10 relay‑hint support: parse and preserve relay hints from tags and URIs, propagate hints through mention extraction and loaders, use ephemeral relay lease acquire/ensureConnected/release for targeted lookups, and return explicit error sheets for deprecated/unsafe NIP‑19 references. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as EventView/BuilderEventView
participant Loader as EventLoaderView
participant Pool as RelayPool
participant Net as SubscriptionManager
User->>UI: open mention (noteId + relayHints)
UI->>Loader: init(noteId, relayHints)
Loader->>Pool: ensureConnected(relayHints)
Pool-->>Loader: connectedRelays
Loader->>Net: findEvent(filter, find_from: connectedRelays)
Net->>Pool: acquireEphemeralRelays(connectedRelays)
Net-->>Loader: event / not_found / EOSE
Net->>Pool: releaseEphemeralRelays(connectedRelays)
Loader-->>UI: loaded event or not_found
UI-->>User: render event or fallback
sequenceDiagram
autonumber
participant User
participant Parser as URL Parser
participant Handler as DamusURLHandler
participant Pool as RelayPool
participant Net as SubscriptionManager
participant UI as ContentView
User->>Parser: supply nevent/nprofile URI
Parser-->>Handler: ParsedURLInfo (includes relays)
Handler->>Pool: ensureConnected(relays)
Pool-->>Handler: connectedRelays
Handler->>Net: lookup/find with connectedRelays
Net->>Pool: acquireEphemeralRelays(connectedRelays)
Net-->>Handler: lookup result (event/profile)
Net->>Pool: releaseEphemeralRelays(connectedRelays)
Handler->>UI: map to ViewAction (include relayHints or error sheet)
UI-->>User: navigate / present error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (3)
damus/Core/Nostr/NostrEvent.swift (1)
812-869: Consider consolidating withfirst_eref_mentionto reduce duplication.The new
first_eref_mention_with_hintsfunction (lines 850-869) is structurally identical tofirst_eref_mention(lines 812-831), differing only in the return type and relay hint inclusion. You could deprecate the original and have callers use the hints-aware version, ignoring hints when not needed.🔎 Example consolidation
+@available(*, deprecated, renamed: "first_eref_mention_with_hints", message: "Use first_eref_mention_with_hints and ignore relayHints if not needed") func first_eref_mention(ndb: Ndb, ev: NostrEvent, keypair: Keypair) -> Mention<NoteId>? { - return try? NdbBlockGroup.borrowBlockGroup(event: ev, using: ndb, and: keypair, borrow: { blockGroup in - return blockGroup.forEachBlock({ index, block in - switch block { - case .mention(let mention): - guard let mention = MentionRef(block: mention) else { return .loopContinue } - switch mention.nip19 { - case .note(let noteId): - return .loopReturn(Mention<NoteId>.note(noteId, index: index)) - case .nevent(let nEvent): - return .loopReturn(Mention<NoteId>.note(nEvent.noteid, index: index)) - default: - return .loopContinue - } - default: - return .loopContinue - } - }) - }) + guard let result = first_eref_mention_with_hints(ndb: ndb, ev: ev, keypair: keypair) else { + return nil + } + return Mention<NoteId>.note(result.noteId, index: result.index) }damus/Features/Events/EventLoaderView.swift (1)
19-19: Remove redundant nil initialization.SwiftLint correctly flags this as redundant. Optional
@Stateproperties are implicitlynil.🔎 Proposed fix
- @State var loadingTask: Task<Void, Never>? = nil + @State var loadingTask: Task<Void, Never>?damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (1)
433-434: Remove redundantnilinitialization.Per SwiftLint: optional variables are implicitly initialized to
nil.🔎 Proposed fix
func findEvent(query: FindEvent) async -> FoundEvent? { - var filter: NostrFilter? = nil + var filter: NostrFilter? let find_from = query.find_from let query = query.type
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
damus/ContentView.swiftdamus/Core/Networking/NostrNetworkManager/SubscriptionManager.swiftdamus/Core/Nostr/Mentions.swiftdamus/Core/Nostr/NostrEvent.swiftdamus/Core/Nostr/RelayPool.swiftdamus/Features/Chat/ChatEventView.swiftdamus/Features/DMs/Views/DMView.swiftdamus/Features/Events/BuilderEventView.swiftdamus/Features/Events/EventLoaderView.swiftdamus/Features/Events/EventShell.swiftdamus/Features/Events/EventView.swiftdamus/Features/Events/MentionView.swiftdamus/Features/Events/Models/LoadableNostrEventView.swiftdamus/Features/Events/SelectedEventView.swiftdamus/Features/Search/Views/SearchResultsView.swiftdamus/Features/Search/Views/SearchingEventView.swiftdamus/Shared/Utilities/URLHandler.swiftnostrdb/NdbNote+.swiftnostrdb/NdbTagIterator.swift
🧰 Additional context used
🧬 Code graph analysis (17)
damus/Features/Events/BuilderEventView.swift (1)
damus/ContentView.swift (1)
event(985-987)
damus/Features/Events/EventLoaderView.swift (1)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (2)
lookup(359-378)lookup(397-427)
damus/Features/Search/Views/SearchingEventView.swift (4)
damus/ContentView.swift (2)
event(985-987)profile(981-983)nostrdb/Ndb.swift (2)
query(821-826)query(835-859)damus/Core/Nostr/Profiles.swift (1)
lookup(98-108)damus/Shared/Utilities/EventCache.swift (1)
lookup(220-231)
damus/Features/Events/EventShell.swift (2)
damus/Core/Nostr/NostrEvent.swift (1)
first_eref_mention_with_hints(850-869)damus/Core/Nostr/Mentions.swift (2)
note(34-36)note(193-195)
nostrdb/NdbNote+.swift (2)
damus/Features/Highlight/Models/HighlightEvent.swift (1)
tags(206-213)nostrdb/NdbTagElem.swift (2)
matches_char(94-96)id(128-131)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (1)
damus/Core/Nostr/RelayPool.swift (3)
acquireEphemeralRelays(183-190)ensureConnected(280-363)releaseEphemeralRelays(196-225)
damus/Features/Events/EventView.swift (1)
nostrdb/NdbNote+.swift (1)
repostTarget(42-56)
damus/Core/Nostr/Mentions.swift (2)
nostrdb/NdbTagElem.swift (3)
data(124-126)id(128-131)string(149-156)damus/Shared/Utilities/Bech32Object.swift (1)
pubkey(105-116)
nostrdb/NdbTagIterator.swift (1)
nostrdb/NdbTagElem.swift (1)
string(149-156)
damus/Features/Chat/ChatEventView.swift (1)
damus/Core/Nostr/NostrEvent.swift (1)
first_eref_mention_with_hints(850-869)
damus/Features/Events/MentionView.swift (1)
damus/ContentView.swift (1)
event(985-987)
damus/ContentView.swift (2)
damus/Core/Nostr/Mentions.swift (4)
pubkey(30-32)pubkey(197-199)note(34-36)note(193-195)damus/Shared/Utilities/Bech32Object.swift (1)
pubkey(105-116)
damus/Core/Nostr/NostrEvent.swift (3)
nostrdb/NdbBlock.swift (2)
borrowBlockGroup(124-139)forEachBlock(278-281)nostrdb/UnownedNdbNote.swift (1)
borrow(57-70)damus/Core/Nostr/Mentions.swift (2)
note(34-36)note(193-195)
damus/Features/Events/SelectedEventView.swift (2)
damus/Core/Nostr/NostrEvent.swift (1)
first_eref_mention_with_hints(850-869)damus/Core/Nostr/Mentions.swift (2)
note(34-36)note(193-195)
damus/Features/DMs/Views/DMView.swift (1)
damus/Core/Nostr/NostrEvent.swift (1)
first_eref_mention_with_hints(850-869)
damus/Features/Events/Models/LoadableNostrEventView.swift (2)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (4)
findEvent(433-495)query(380-386)lookup(359-378)lookup(397-427)damus/ContentView.swift (1)
event(985-987)
damus/Features/Search/Views/SearchResultsView.swift (1)
damus/ContentView.swift (2)
profile(981-983)event(985-987)
🪛 SwiftLint (0.57.0)
damus/Features/Events/EventLoaderView.swift
[Warning] 19-19: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift
[Warning] 434-434: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (41)
nostrdb/NdbTagIterator.swift (1)
45-73: LGTM - Clean relay hint extraction implementation.The extension correctly extracts relay hints from position 2 of tags per NIP-01/NIP-10 semantics. The guard conditions properly handle missing or empty URLs, and the array wrapper provides good API consistency with
NEvent.relays.damus/Core/Nostr/RelayPool.swift (5)
47-50: LGTM - Ref-counted lease tracking is a sound approach.The lease counter design prevents race conditions where multiple concurrent lookups might try to clean up the same ephemeral relay.
179-190: LGTM - Lease acquisition is straightforward.The implementation correctly increments lease counts. The release function validates ephemeral status before removal, so acquiring leases on non-ephemeral URLs is harmless.
196-225: Well-designed race condition handling.The re-check at line 212 after the
awaitcorrectly handles the case where another caller re-acquires a lease during the suspension point. This prevents premature removal when the relay is still needed.
280-363: Solid connectivity assurance implementation.The polling-based wait loop with a 300ms grace period after the first connection is a practical approach for allowing multiple hinted relays to connect. The early return optimization (line 311) and proper handling of already-connected relays improve latency.
242-250: Appropriate debug logging for ephemeral relay troubleshooting.The
#if DEBUGguards ensure no performance impact in release builds. Truncating event strings withprefix(200)prevents excessive log output.damus/Features/Chat/ChatEventView.swift (1)
160-164: LGTM - Correctly adopts relay-hints-aware mention extraction.The update properly uses
first_eref_mention_with_hintsand passes the extractedrelayHintstoMentionView, enabling relay-aware event loading for referenced notes.nostrdb/NdbNote+.swift (1)
36-56: LGTM - Correct NIP-18 repost target extraction.The implementation properly parses the
etag from repost events, extracting both the target note ID and relay hints per NIP-18 specification. Using therelayHintscomputed property from the earlierTagSequenceextension keeps the code clean.damus/Features/Events/EventView.swift (1)
36-46: LGTM - Graceful fallback for repost loading with relay hints.The three-tier approach (cache hit → async load with hints → empty fallback) correctly handles reposts. Using
EventLoaderViewwith relay hints enables fetching boosted content from hinted relays when not locally cached.damus/Features/Events/EventShell.swift (2)
38-44: LGTM - Consistent adoption of relay-hints-aware mentions.The return type and implementation updates align with the new
NoteMentionWithHintspattern used throughout the codebase.
78-80: LGTM - Relay hints correctly passed to MentionView.Both the Threaded (line 79) and Wide (line 111) layouts now pass
mention.relayHintstoMentionView, enabling relay-aware fetching for embedded note references.damus/Core/Nostr/NostrEvent.swift (2)
833-838: LGTM - Clean data structure for relay-aware mentions.The
NoteMentionWithHintsstruct effectively bundles the note ID, relay hints, and optional index for propagation through the UI layer.
840-869: LGTM - Proper NIP-19 relay hint extraction.The function correctly differentiates between plain
notereferences (empty hints) andneventreferences (with relay hints fromnEvent.relays). The implementation mirrors the existingfirst_eref_mentionpattern appropriately.damus/Core/Nostr/Mentions.swift (1)
125-170: LGTM! Clear implementation of relay hint preservation.The
from_tagmethod correctly parses tag sequences and preserves relay hints by mapping to the appropriate NIP-19 variants (nprofile,nevent,naddr) when hints are present. The docstring accurately describes the NIP-01/NIP-10 semantics.damus/Features/Events/MentionView.swift (1)
10-36: LGTM! Clean relay hints propagation.The
relayHintsproperty is correctly added and threaded through toEventLoaderViewandBuilderEventView. The default empty array maintains backward compatibility with existing call sites.damus/Features/Events/EventLoaderView.swift (1)
42-49: LGTM! Correct relay hints integration in subscribe logic.The conversion of
relayHintstonilwhen empty ensures the lookup falls back to default relay behavior, while non-empty hints are correctly passed through.damus/Features/Events/BuilderEventView.swift (2)
10-38: LGTM! Well-structured relay hints support.The two initialization paths are handled appropriately: pre-loaded events set empty hints (not needed), while the ID-based loader accepts and propagates relay hints correctly.
50-59: LGTM! Proper propagation to EventLoaderView.The
relayHintsare correctly forwarded toEventLoaderViewwhen loading events by ID.damus/ContentView.swift (2)
1117-1127: LGTM! Correct relay hints propagation for NOTE and NEVENT.The mapping correctly passes empty relays for plain
NOTEreferences and propagatesnEvent.relaysforNEVENTreferences. The TODO comment at line 1126 appropriately flags the missingnprofilerelay hint handling for future improvement.
1128-1148: LGTM! Appropriate error handling for deprecated and unsafe references.The error sheets for
nrelay(deprecated per NIP) andnsec(security risk) provide clear, actionable user guidance.damus/Shared/Utilities/URLHandler.swift (3)
93-101: Good catch: Direct parsing preserves relay hints.The comment at line 94 clearly explains why
neventandnprofileare parsed directly before falling through todecode_nostr_uri— the latter discards relay hints. This is a correct approach.
135-145: LGTM! ParsedURLInfo extended correctly.The new
profile_referencecase properly carries the pubkey and relay hints tuple, completing the URL parsing enhancement.
34-39: Document the background prefetch intent for profile_reference with relay hints.The
Task { ... }at lines 36-38 initiates a fire-and-forget network lookup from the relay hints in the nprofile URL. The result is discarded and the route is returned immediately, so the profile data may not arrive before the view loads. While this appears intentional—to prefetch from the specified relays before the user navigates to the profile—add a comment explaining this behavior. If the view cannot gracefully handle initially missing profile data, consider awaiting the result or implementing a synchronous load strategy instead.damus/Features/Events/SelectedEventView.swift (1)
83-90: LGTM! Correctly adopts relay hints for mentions.The update properly uses
first_eref_mention_with_hintsand constructs theMentionViewwith the structured.note(noteId, index:)mention and propagatesrelayHints. This is consistent with similar changes acrossChatEventView,DMView, andEventShell.damus/Features/Search/Views/SearchingEventView.swift (5)
17-22: LGTM! Enum updated to carry relay hints.The
SearchTypeenum now correctly carries associated relay URLs foreventandprofilecases, enabling relay-aware lookups downstream.
44-51: Good documentation forhandle_search.The docstring clearly describes the state transitions and possible outcomes.
87-96: Correct relay hint propagation for event lookup.The pattern of converting empty arrays to
nilfortargetRelaysis consistent with theFindEvent.event(evid:find_from:)API which expectsnilfor unrestricted lookup.
97-106: Consistent relay handling for profile lookup.Same pattern applied correctly for profile lookups.
107-116: NAddr lookup correctly usesnaddr.relays.The
naddrcase appropriately extracts relays from the NAddr struct and passes them to the lookup.damus/Features/Search/Views/SearchResultsView.swift (3)
103-115: LGTM! Relay hints correctly propagated to search views.The implementation properly:
- Passes empty relay arrays for cases without hints (
.profile,.hex,.note)- Propagates
nevent.relaysandnprofile.relaysfor bech32 objects that include hints
201-206: Good documentation forsearch_for_string.The docstring clearly describes the function's purpose and return values.
240-245: Debug logging appropriately scoped.The
#if DEBUGwrapper ensures this logging doesn't affect release builds.damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (3)
391-427: LGTM! Ephemeral relay handling for NAddr lookups.The implementation correctly:
- Acquires ephemeral relay leases before connecting
- Attempts connection and uses only successfully connected relays
- Defers release in a detached Task to avoid blocking
- Falls back to unrestricted lookup if no hints connect
The defer-based cleanup with
Task { await ... }is appropriate for async cleanup without blocking the current flow.
429-432: Good documentation forfindEvent.The docstring clearly describes the relay hint behavior and return semantics.
459-475: Consistent ephemeral relay pattern applied tofindEvent.The same acquire/connect/defer-release pattern is correctly applied here, matching
lookup(naddr:).damus/Features/DMs/Views/DMView.swift (1)
18-26: LGTM! Relay hints propagated through DM mention handling.The update correctly uses
first_eref_mention_with_hintsand passesmention.relayHintstoBuilderEventView, enabling relay-aware fetching of mentioned notes in DMs.damus/Features/Events/Models/LoadableNostrEventView.swift (5)
34-52: Consider checking cancellation withinloadTaskto ensure timeout is respected.The timeout mechanism cancels
loadTask, butexecuteLoadingLogicdoesn't check for cancellation. Since the task body runs to completion regardless of cancellation signal, if the timeout fires while loading is in progress, the loading result may still overwrite.not_foundafter the timeout sets it.Since both tasks run on
@MainActor, operations are serialized, but the sequence could be:
- Timeout fires → sets state to
.not_foundloadTaskcompletes → sets state to the loaded resultThis might actually be acceptable behavior (successful load overrides timeout), but it makes the 10-second "give up" semantics inconsistent. If strict timeout semantics are desired, check
Task.isCancelledbefore setting state inloadTask.
54-63: LGTM! Relay-aware event loading helper.Correctly converts empty relay array to
nilfor unrestricted lookup, consistent with the pattern used elsewhere.
89-92: Good: relay hints propagated for reaction lookups.Passing the same relay hints when following a
.likereaction to its referenced note is sensible—the referenced note is likely on the same relay as the reaction.
99-102: Correct relay handling for NAddr lookups.The empty-to-nil conversion is consistent with other relay hint handling in the codebase.
113-116: LGTM! NoteReference enum extended with relay hints.The enum now carries relay hints for
note_idreferences, enabling relay-aware lookups throughout the view model.
Extract relay hints from nevent, nprofile, and naddr bech32 entities and use them to fetch events from hinted relays not in the user's relay pool. Changes: - Parse relay hints directly from bech32 TLV data in URLHandler - Pass relay hints through SearchType and NoteReference enums - Add ensureConnected() to RelayPool for on-demand relay connections - Connect to hinted relays as ephemeral, return early when first connects - Use connected hinted relays for event streaming Changelog-Added: Added relay hint support for nevent, nprofile, and naddr links 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Closes damus-io#1147 Signed-off-by: alltheseas
Ephemeral relays (added for relay hint lookups) are now disconnected and removed from the pool as soon as the findEvent or lookup(naddr:) call completes. This prevents unbounded growth of the relay pool during long sessions with many hinted lookups. - Add removeEphemeralRelays() method to RelayPool - Call cleanup in defer blocks in findEvent and lookup(naddr:) - Update ensureConnected documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Closes damus-io#1147 Signed-off-by: alltheseas
When ensureConnected() is called with relay hints and at least one hinted relay is already connected, return immediately instead of waiting for additional relays to connect. The other relays are connected in the background for potential future use. This reduces latency when the same relay hint is used multiple times. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Closes damus-io#1147 Signed-off-by: alltheseas
When loading a reaction (like) event via relay hints, pass the same hints when fetching the referenced note. The referenced note is likely on the same relay as the reaction itself. Previously relay hints were dropped (relays: []) for the second hop, causing lookups to fall back to default relays. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Closes damus-io#1147 Signed-off-by: alltheseas
Add DEBUG-only logging to help diagnose relay hint issues: - Log EVENT/EOSE messages received from ephemeral relays - Log REQ messages sent to ephemeral relays - Log subscription target relays and filter IDs - Log getRelays mismatches (requested vs found) - Log parsed nevent note ID and relay hints 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Closes damus-io#1147 Signed-off-by: alltheseas
1. Grace period for additional relays: When some hinted relays are already connected, wait a short grace period (300ms) for others to connect instead of returning immediately. This prevents missing events on relays that connect quickly. 2. Ref-counted ephemeral relay cleanup: Use acquire/release leases to track ephemeral relay usage. Only remove a relay when the last lookup using it completes. This prevents race conditions where concurrent lookups could have their relay removed mid-query. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Closes damus-io#1147 Signed-off-by: alltheseas
Docstrings generation was requested by @alltheseas. * #8 (comment) The following files were modified: * `damus/ContentView.swift` * `damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift` * `damus/Core/Nostr/RelayPool.swift` * `damus/Features/Events/Models/LoadableNostrEventView.swift` * `damus/Features/Search/Views/SearchResultsView.swift` * `damus/Features/Search/Views/SearchingEventView.swift` * `damus/Shared/Utilities/URLHandler.swift`
Two doc comments had em-dashes (—) instead of triple-slashes (///), causing Swift compilation errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Per NIP-01/NIP-10, tag position 2 contains an optional relay URL hint. Add relayHint and relayHints properties to TagSequence for extracting these hints from e, p, q, and a tags. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add relayHints parameter to EventLoaderView, allowing callers to specify relay URLs where the target event may be found. When hints are provided, the lookup targets those relays specifically. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update MentionRef.from_tag to use nevent/nprofile/naddr variants when relay hints are present in the tag, rather than simple note/npub forms. This preserves relay URL information for later use in event fetching. Per NIP-01/NIP-10, position 2 in e, p, and a tags contains optional relay hints. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add repostTarget() helper to NdbNote that extracts both the event ID and relay hints from a repost's e tag. Update EventView to use EventLoaderView with relay hints when the inner reposted event is not in cache, per NIP-18. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add first_eref_mention_with_hints() that preserves relay hints from nevent bech32 entities in note content. Update all call sites (EventShell, ChatEventView, SelectedEventView, DMView) to use the new function and pass relay hints to MentionView and BuilderEventView. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Decrement lease count immediately before any suspension point to ensure atomicity. After await, only remove relay if lease is still nil (not re-acquired). This prevents the 1→2→1 cycle from incorrectly removing an active relay. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Move grace deadline check outside anyConnected condition so it's evaluated every iteration. Previously, if alreadyConnected was not empty but none of the toConnect relays connected, the grace deadline was set but never checked, causing a full 2-second timeout instead of the expected 300ms grace period. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
alltheseas
force-pushed
the
relay-hints
branch
from
82d5154 to
d3cbb8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
damus/Features/Events/Models/LoadableNostrEventView.swift (1)
37-52: Potential race condition in timeout handling.The timeout implementation has a subtle issue:
loadTask.cancel()on line 46 only sets a cancellation flag—it doesn't interrupt the running task.await loadTask.valueon line 50 waits forloadTaskto complete regardless of cancellation.- If timeout fires first,
self.state = .not_foundis set (line 47), but thenloadTaskmay complete andexecuteLoadingLogicreturns, causing line 40 to overwritestatewith the loaded result.To fix, check for cancellation inside
executeLoadingLogicor use a separate@Publishedguard to ignore late results:🔎 Suggested fix
func load() async { - // Start the loading process in a separate task to manage the timeout independently. - let loadTask = Task { @MainActor in - self.state = await executeLoadingLogic(note_reference: self.note_reference) + let loadTask = Task { @MainActor in + let result = await executeLoadingLogic(note_reference: self.note_reference) + // Only update state if not cancelled (timeout didn't fire) + if !Task.isCancelled { + self.state = result + } }
🧹 Nitpick comments (3)
damus/Core/Nostr/RelayPool.swift (1)
315-345: Grace period initialization logic may not match intent.The grace period is initialized when
alreadyConnected.isEmptyisfalse(line 317), meaning if there are already-connected relays, the grace period starts immediately. However, the comment on line 337 says "Start grace period on first connection" for relays intoConnect. This creates inconsistent behavior:
- If all hinted relays are already connected → grace period starts immediately → exits after 300ms even though no new connections were needed.
- If some relays need connecting → grace period only starts when first one connects.
If the intent is to always give newly connecting relays a 300ms window, consider initializing
graceDeadlinetonilunconditionally:🔎 Suggested fix
- var graceDeadline: ContinuousClock.Instant? = alreadyConnected.isEmpty ? nil : ContinuousClock.now + .milliseconds(300) + var graceDeadline: ContinuousClock.Instant? = nilHowever, if the current behavior is intentional (return early when all relays are already connected), then the code is correct as-is — just the comment on line 337 is slightly misleading.
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (2)
410-414:deferwithTaskmay not guarantee cleanup timing.The
deferblocks wrap the release call in aTask, which means the ephemeral relay release happens asynchronously after the function returns. While the ref-counting inRelayPoolprevents premature removal, this pattern has a subtle issue: if the caller immediately makes another lookup call before the deferredTaskexecutes, the lease count will be temporarily higher than necessary.This is unlikely to cause functional issues due to the ref-counting design, but for cleaner semantics, consider making the lookup functions fully async with cleanup at the end:
🔎 Alternative pattern
Instead of:
defer { if !ephemeralRelays.isEmpty { Task { await self.pool.releaseEphemeralRelays(ephemeralRelays) } } }You could structure as:
// At end of function, before return statements: if !ephemeralRelays.isEmpty { await self.pool.releaseEphemeralRelays(ephemeralRelays) }This requires restructuring to ensure all code paths release, but guarantees synchronous cleanup.
The current implementation is functionally correct due to the ref-counting safeguards.
Also applies to: 471-475
434-434: Redundant optional initialization.As flagged by SwiftLint, initializing an optional variable with
nilis redundant.🔎 Suggested fix
- var filter: NostrFilter? = nil + var filter: NostrFilter?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
damus/ContentView.swiftdamus/Core/Networking/NostrNetworkManager/SubscriptionManager.swiftdamus/Core/Nostr/Mentions.swiftdamus/Core/Nostr/NostrEvent.swiftdamus/Core/Nostr/RelayPool.swiftdamus/Features/Chat/ChatEventView.swiftdamus/Features/DMs/Views/DMView.swiftdamus/Features/Events/BuilderEventView.swiftdamus/Features/Events/EventLoaderView.swiftdamus/Features/Events/EventShell.swiftdamus/Features/Events/EventView.swiftdamus/Features/Events/MentionView.swiftdamus/Features/Events/Models/LoadableNostrEventView.swiftdamus/Features/Events/SelectedEventView.swiftdamus/Features/Search/Views/SearchResultsView.swiftdamus/Features/Search/Views/SearchingEventView.swiftdamus/Shared/Utilities/URLHandler.swiftnostrdb/NdbNote+.swiftnostrdb/NdbTagIterator.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- damus/Features/Chat/ChatEventView.swift
- damus/Features/Events/EventShell.swift
- damus/Features/Events/MentionView.swift
- damus/Features/Events/EventView.swift
- damus/Features/Events/EventLoaderView.swift
- damus/Core/Nostr/NostrEvent.swift
- damus/Features/Events/SelectedEventView.swift
🧰 Additional context used
🧬 Code graph analysis (9)
damus/Core/Nostr/Mentions.swift (2)
nostrdb/NdbTagElem.swift (3)
data(124-126)id(128-131)string(149-156)damus/Shared/Utilities/Bech32Object.swift (1)
pubkey(105-116)
nostrdb/NdbNote+.swift (2)
damus/Features/Highlight/Models/HighlightEvent.swift (1)
tags(206-213)nostrdb/NdbTagElem.swift (2)
matches_char(94-96)id(128-131)
damus/Shared/Utilities/URLHandler.swift (2)
damus/ContentView.swift (1)
profile(981-983)damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (2)
findEvent(433-495)query(380-386)
damus/Features/Search/Views/SearchingEventView.swift (1)
damus/ContentView.swift (2)
event(985-987)profile(981-983)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (2)
damus/Core/Nostr/RelayPool.swift (3)
acquireEphemeralRelays(183-190)ensureConnected(280-363)releaseEphemeralRelays(196-225)damus/ContentView.swift (1)
event(985-987)
damus/Features/Events/BuilderEventView.swift (1)
damus/ContentView.swift (1)
event(985-987)
damus/Features/Events/Models/LoadableNostrEventView.swift (2)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (2)
lookup(359-378)lookup(397-427)damus/ContentView.swift (2)
event(985-987)zap(37-39)
damus/Features/DMs/Views/DMView.swift (1)
damus/Core/Nostr/NostrEvent.swift (1)
first_eref_mention_with_hints(850-869)
nostrdb/NdbTagIterator.swift (1)
nostrdb/NdbTagElem.swift (1)
string(149-156)
🪛 SwiftLint (0.57.0)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift
[Warning] 434-434: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (22)
damus/Core/Nostr/Mentions.swift (1)
125-170: LGTM! Well-structured relay hint preservation.The implementation correctly parses NIP-01/NIP-10 tags and preserves relay hints by upgrading simple variants (
npub,note) to their relay-aware counterparts (nprofile,nevent) when hints are present. The docstring clearly explains the intent.damus/Core/Nostr/RelayPool.swift (2)
196-225: Good race-safety pattern for ephemeral relay cleanup.The implementation correctly handles the TOCTOU race by:
- Decrementing the lease count atomically (before any suspension point)
- Re-checking
ephemeralLeases[url] == nilafter theawait get_relay()call to ensure the lease wasn't re-acquired during the suspension
280-363: LGTM! Ephemeral relay connection logic is well-designed.The
ensureConnectedmethod properly:
- Distinguishes between already-connected, needs-connection, and new ephemeral relays
- Uses a polling loop with configurable timeout
- Implements a grace period to allow multiple relays to connect before returning
- Returns only the subset of relays that are actually connected
damus/Features/Events/BuilderEventView.swift (1)
10-66: LGTM! Clean relay hints integration.The changes properly thread relay hints through the view hierarchy while maintaining backward compatibility via default empty arrays. The docstrings clearly explain the NIP-01/NIP-10 relay hint purpose.
damus/ContentView.swift (1)
1117-1149: LGTM! Proper handling of NIP-19 variants with clear error messaging.The implementation correctly:
- Propagates relay hints from
neventto the loadable note route- Provides user-friendly error sheets for deprecated (
nrelay) and security-sensitive (nsec) references- Includes appropriate guidance in the error messages
nostrdb/NdbNote+.swift (1)
35-56: LGTM! Clean implementation of repost target extraction with relay hints.The method correctly:
- Guards for repost (boost) events before processing
- Iterates tags to find the first valid
etag with a note ID- Leverages the new
tag.relayHintsextension for relay hint extraction- Returns
nilfor non-repost events or when no valid e-tag is foundThe docstring accurately describes NIP-18 requirements.
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (1)
397-427: LGTM! Consistent ephemeral relay handling pattern.The
lookup(naddr:)method properly:
- Acquires leases before connecting
- Falls back to default pool when no hinted relays connect
- Logs relay hint usage in DEBUG builds
- Releases leases in defer block
The pattern is consistent with
findEventand aligns with theRelayPoolephemeral relay lifecycle.nostrdb/NdbTagIterator.swift (1)
45-73: LGTM! Clean relay hint extraction with proper bounds checking.The implementation correctly:
- Guards against out-of-bounds access with
count >= 3check- Validates non-empty string before creating
RelayURL- Returns an array for consistency with
NEvent.relaysand future extensibility- Includes comprehensive docstrings
RelayURLgracefully handles invalid URL strings through its failable initializer, which returnsnilfor invalid URLs, missing schemes, or non-WebSocket schemes. The code correctly chains this optional result to therelayHintproperty type.damus/Features/Search/Views/SearchResultsView.swift (3)
104-115: LGTM! Relay hints correctly propagated to SearchingEventView.The empty relay arrays for legacy cases (profile, hex, note) provide a safe fallback, while
neventandnprofilecorrectly extract and pass their embedded relay hints.
201-212: Clean implementation with helpful documentation.The early-return guard for empty strings prevents unnecessary processing and the docstring clearly documents the function's behavior.
241-244: Appropriate debug logging.The
#if DEBUGguard ensures this diagnostic output won't affect release builds.damus/Features/Search/Views/SearchingEventView.swift (4)
17-22: Clean enum extension for relay hints.The updated
SearchTypeenum properly carries relay hints as associated values, enabling relay-aware lookups throughout the search flow.
44-51: Well-documented state transitions.The docstring clearly describes the possible outcomes of
handle_search, making the async state machine easier to understand.
87-106: Consistent relay-hint handling for event and profile lookups.The
targetRelays = relays.isEmpty ? nil : relayspattern correctly maps empty arrays tonil, aligning with theFindEvent.event(evid:find_from:)andFindEvent.profile(pubkey:find_from:)signatures that expectnilfor "no hints."
107-116: Correct relay-hint propagation for naddr lookups.The naddr case properly extracts relay hints from the
NAddrobject and passes them to the lookup method.damus/Features/DMs/Views/DMView.swift (1)
18-26: Correct migration to relay-hint-aware mention extraction.The switch from
first_eref_mentiontofirst_eref_mention_with_hintsensures that DM mentions can leverage relay hints for loading referenced notes, aligning with the PR's goal of propagating hints through the UI.damus/Shared/Utilities/URLHandler.swift (3)
34-39: Verify fire-and-forget lookup is intentional.The
Task { ... }on lines 36-38 performs an asyncfindEventlookup, but the result is discarded (let _ = await ...). This appears to be a "warm the cache" pattern—the lookup fetches/caches the profile so it's available whenProfileByKeyrenders.If this is intentional, consider adding a brief comment clarifying the intent (e.g.,
// Pre-fetch profile into local cache). If the profile must be loaded before navigation, this creates a race condition where the view may render before the fetch completes.
93-101: Good approach: direct parsing preserves relay hints.The comment on line 94 explains the motivation well. Parsing
nevent/nprofiledirectly before falling through todecode_nostr_uriensures relay hints aren't discarded. This is a clean solution.
112-113: Correct fallback with empty relays.Since this path goes through
decode_nostr_uriwhich discards relay hints, returning an empty relays array is the appropriate fallback.damus/Features/Events/Models/LoadableNostrEventView.swift (3)
54-63: Clean helper for relay-aware event loading.The
loadEventhelper encapsulates the relay-hint logic consistently with the rest of the codebase.
89-92: Smart relay-hint propagation for reactions.Propagating the same relay hints when following a
.liketo its referenced note is a good heuristic—the reaction and referenced note are likely on the same relay.
113-116: Consistent enum extension for relay hints.The
NoteReferenceenum update aligns with the relay-hint propagation pattern used throughout this PR.
|
@danieldaquino working on adding relay hints for quotes (related to our discussion today) #3498, and replies to the existing commits |
Add QuoteRef struct that preserves relay hints from position 2 of q tags. Update first_eref_mention_with_hints to fall back to q tags when no inline mention is found in content, enabling quote reposts to use relay hints for fetching the quoted event. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add direct_reply_ref() to return NoteRef with relay hint. Update ReplyQuoteView to accept relayHint parameter and use EventLoaderView when reply event is not in cache, enabling replies to be fetched from hinted relays. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Reference both NIP-10 and NIP-18 for q tag documentation - Document that QuoteRef only supports hex event IDs, not event addresses 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
damus/Features/Chat/ReplyQuoteView.swift (1)
75-92: Consider handling the case when event is not cached and no relay hint is available.Currently, if
state.events.lookup(event_id)returnsnilandrelayHintis alsonil(or invalid), theGrouprenders nothing silently. This may be intentional for a compact reply preview, but could confuse users if the quoted event never appears.If this is the expected behavior, a brief inline comment would clarify intent. Otherwise, consider a placeholder or loading indicator.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
damus/Core/Nostr/Id.swiftdamus/Core/Nostr/NostrEvent.swiftdamus/Features/Chat/ChatEventView.swiftdamus/Features/Chat/ReplyQuoteView.swiftnostrdb/NdbNote.swift
🧰 Additional context used
🧬 Code graph analysis (4)
nostrdb/NdbNote.swift (1)
damus/Features/Highlight/Models/HighlightEvent.swift (1)
tags(206-213)
damus/Core/Nostr/NostrEvent.swift (3)
nostrdb/NdbBlock.swift (2)
borrowBlockGroup(124-139)forEachBlock(278-281)nostrdb/UnownedNdbNote.swift (1)
borrow(57-70)damus/Core/Nostr/Mentions.swift (2)
note(34-36)note(193-195)
damus/Features/Chat/ReplyQuoteView.swift (2)
damus/ContentView.swift (1)
event(985-987)damus/Features/Timeline/Models/HomeModel.swift (2)
should_show_event(1221-1227)should_show_event(1229-1237)
damus/Features/Chat/ChatEventView.swift (2)
nostrdb/NdbNote.swift (1)
direct_reply_ref(550-552)damus/Core/Nostr/NostrEvent.swift (1)
first_eref_mention_with_hints(853-883)
🔇 Additional comments (9)
damus/Core/Nostr/Id.swift (2)
52-67: LGTM!Minor whitespace adjustments with no semantic changes to
QuoteId.
83-127: Well-documented struct with appropriate NIP-10/NIP-18 references.The implementation correctly:
- Parses relay hints from tag position 2 via
tag.relayHints- Serializes only the first relay hint in the
tagproperty (matching the NIP spec)- Documents the limitation regarding event addresses vs hex IDs
The explicit documentation about unsupported event addresses (
<kind>:<pubkey>:<d>) is helpful for future maintainers.nostrdb/NdbNote.swift (2)
438-440: LGTM!Follows the established pattern of other
referenced_*computed properties in this extension.
546-552: LGTM!Good addition that exposes the full
NoteRef(including relay hint) rather than just theNoteId. The NIP-10 documentation is helpful context.damus/Features/Chat/ReplyQuoteView.swift (1)
10-30: LGTM!Clean API extension with a sensible default. The docstring correctly references NIP-10 for relay hints.
damus/Core/Nostr/NostrEvent.swift (2)
833-838: LGTM!Simple, focused value type. The
indexbeing optional makes sense since q-tag fallback doesn't have a content index.
840-883: Well-structured function with clear fallback logic.The implementation correctly:
- Prioritizes inline content mentions over q-tags
- Preserves relay hints from
neventreferences while returning empty hints for plainnotereferences- Falls back to q-tags for NIP-18 quote reposts
The
try?pattern is consistent with the existingfirst_eref_mentionfunction above, so the error handling approach is appropriate here.damus/Features/Chat/ChatEventView.swift (2)
146-154: LGTM!Good refactor from
direct_replies()todirect_reply_ref()to leverage the fullNoteRefincluding relay hints. The relay hint is correctly passed through toReplyQuoteView.
160-164: LGTM!Correctly updated to use
first_eref_mention_with_hintsand propagate relay hints toMentionView. The styling changes (background, clipShape) provide good visual containment for the mention.
Add DEBUG-only print statements to trace relay hint consumption: - Inline nevent mentions in content - Quote (q tag) relay hints - Reply (e tag) relay hints - Repost (e tag) relay hints - URL nevent/nprofile link hints - EventLoaderView fetch attempts Filter console by "[relay-hints]" to see all activity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
damus/Core/Nostr/NostrEvent.swift (1)
840-893: Solid implementation of relay-hint extraction.The function correctly scans inline mentions before falling back to
qtags, preserving relay hints fromnevententities as intended.A few optional refinements:
Consider using a logging framework for DEBUG logs
While
print()works for DEBUG builds, using a structured logger (e.g.,os.Loggeror the existing logging infrastructure) would provide better control and consistency:#if DEBUG -if !nEvent.relays.isEmpty { - print("[relay-hints] Inline nevent: Found \(nEvent.relays.count) hint(s) for \(nEvent.noteid.hex().prefix(8))...: \(nEvent.relays.map { $0.absoluteString })") -} +Self.logger.debug("Inline nevent: Found \(nEvent.relays.count) hint(s) for \(nEvent.noteid.hex().prefix(8))...: \(nEvent.relays.map { $0.absoluteString })") #endifSimilar pattern at lines 887-891.
Minor documentation note: Line 845 mentions "NIP-10/NIP-18" — NIP-18 is accurate for quote reposts (
qtags), but NIP-10 primarily covers replies. Consider clarifying as "NIP-18" or "NIP-18 (quotes) / NIP-10 (for broader tag context)".damus/Features/Events/EventLoaderView.swift (1)
10-57: Excellent integration of relay hints into the event loader.The changes cleanly extend
EventLoaderViewto support relay-hinted lookups while maintaining backward compatibility (empty array = no hints). The documentation is clear and the debug logging provides good visibility.Optional: Use structured logging
For consistency with production logging patterns, consider replacing
print()with a structured logger:#if DEBUG -if let targetRelays, !targetRelays.isEmpty { - print("[relay-hints] EventLoaderView: Loading event \(event_id.hex().prefix(8))... with \(targetRelays.count) relay hint(s): \(targetRelays.map { $0.absoluteString })") -} +Self.logger.debug("EventLoaderView: Loading event \(event_id.hex().prefix(8))... with \(targetRelays.count) relay hint(s): \(targetRelays.map { $0.absoluteString })") #endifSimilar for lines 53-57.
damus/Shared/Utilities/URLHandler.swift (1)
94-123: Well-designed URL parsing that preserves relay hints.The direct parsing of
neventandnprofilebefore thedecode_nostr_urifallback correctly preserves relay hints that would otherwise be lost. The comment at line 94 clearly explains the rationale, and the debug logging provides good visibility.Optional: Consolidate debug logging
The debug logging pattern is repeated. Consider extracting to a helper:
#if DEBUG private static func logRelayHints(type: String, count: Int, id: String, urls: [String]) { if count > 0 { print("[relay-hints] \(type): Found \(count) hint(s) for \(id): \(urls)") } } #endifThen use as:
logRelayHints(type: "URL nevent", count: nevent.relays.count, id: "\(nevent.noteid.hex().prefix(8))...", urls: nevent.relays.map { $0.absoluteString })Similar refactoring could apply to logging in
NostrEvent.swift.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
damus/Core/Nostr/Mentions.swiftdamus/Core/Nostr/NostrEvent.swiftdamus/Features/Events/EventLoaderView.swiftdamus/Shared/Utilities/URLHandler.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- damus/Core/Nostr/Mentions.swift
🧰 Additional context used
🧬 Code graph analysis (3)
damus/Shared/Utilities/URLHandler.swift (3)
damus/ContentView.swift (1)
profile(981-983)damus/Core/Nostr/Mentions.swift (2)
pubkey(30-32)pubkey(200-202)damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (2)
findEvent(433-495)query(380-386)
damus/Core/Nostr/NostrEvent.swift (3)
nostrdb/NdbBlock.swift (1)
borrowBlockGroup(124-139)nostrdb/UnownedNdbNote.swift (1)
borrow(57-70)damus/Core/Nostr/Mentions.swift (2)
note(34-36)note(196-198)
damus/Features/Events/EventLoaderView.swift (1)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (2)
lookup(359-378)lookup(397-427)
🔇 Additional comments (2)
damus/Core/Nostr/NostrEvent.swift (1)
833-838: LGTM! Clean data structure for relay-hinted mentions.The
NoteMentionWithHintsstruct effectively encapsulates note references with their relay hints, supporting the NIP-19 relay hints feature.damus/Shared/Utilities/URLHandler.swift (1)
34-39: LGTM! Appropriate prefetch pattern for profile relay hints.The fire-and-forget
Taskcorrectly prefetches profile metadata from hinted relays while immediately navigating to the profile view, which will display cached data and update when the fetch completes.
Add pubkey field to NoteRef struct for full NIP-10 compliance: - Parse pubkey from position 4 when marker is recognized - Include pubkey in tag generation - Update nip10_reply_tags to propagate author pubkey This enables clients to find reply targets on correct relays. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
When looking up events with relay hints: - Try hinted relays first with short timeout - Fall back to broadcast if hints don't respond - Use total deadline to prevent timeout accumulation - Skip hint phase if no relays resolve 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift:
- Around line 447-455: Remove the leftover incomplete docstring lines that
contain a standalone "/// - Parameters:" (the small block at the top of the doc
comment in SubscriptionManager.swift for the method that finds a replaceable
event by naddr) and keep only the complete docstring that follows (the full
description including the detailed - Parameters: list and - Returns:). Ensure
the method's documentation for the naddr-based replaceable-event lookup is a
single coherent docblock (remove the three-line incomplete block and leave the
full block describing the naddr, targetRelays, timeout, and return value).
🧹 Nitpick comments (1)
damus/Features/Posting/Views/PostView.swift (1)
814-833: Unusedkeypairparameter should be removed.The
keypairparameter is declared but never used within the function body. Remove it from the function signature and update all call sites.This requires changes at:
- Function definition (line 816)
- Call site in PostView.swift (line 922)
- Test call sites in NIP10Tests.swift (lines 355 and 385)
🔎 Proposed changes
Function signature:
-func nip10_reply_tags(replying_to: NostrEvent, keypair: Keypair, relayURL: RelayURL?) -> [[String]] { +func nip10_reply_tags(replying_to: NostrEvent, relayURL: RelayURL?) -> [[String]] {Call site in PostView.swift (line 922):
-tags = nip10_reply_tags(replying_to: replying_to, keypair: state.keypair, relayURL: await state.nostrNetwork.relaysForEvent(event: replying_to).first) +tags = nip10_reply_tags(replying_to: replying_to, relayURL: await state.nostrNetwork.relaysForEvent(event: replying_to).first)Call sites in NIP10Tests.swift:
-let tags = nip10_reply_tags(replying_to: parentNote, keypair: test_keypair, relayURL: relayUrl) +let tags = nip10_reply_tags(replying_to: parentNote, relayURL: relayUrl)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.beads/issues.jsonldamus/Core/Networking/NostrNetworkManager/SubscriptionManager.swiftdamus/Core/Types/Ids/Referenced.swiftdamus/Features/Posting/Views/PostView.swiftdamusTests/NIP10Tests.swift
🧰 Additional context used
📓 Path-based instructions (1)
**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
**/*.swift: Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Favor Swift-first solutions that lean on nostrdb types (Ndb, NdbNote, iterators) before introducing new storage mechanisms
Ensure docstring coverage for any code added, or modified
Ensure nevernesting: favor early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers of if statements
Files:
damus/Features/Posting/Views/PostView.swiftdamus/Core/Networking/NostrNetworkManager/SubscriptionManager.swiftdamus/Core/Types/Ids/Referenced.swiftdamusTests/NIP10Tests.swift
🧬 Code graph analysis (3)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (1)
damus/Core/Nostr/RelayPool.swift (5)
getRelays(424-437)subscribe(463-473)subscribe(488-559)acquireEphemeralRelays(183-190)releaseEphemeralRelays(196-225)
damus/Core/Types/Ids/Referenced.swift (1)
nostrdb/NdbTagElem.swift (1)
id(128-131)
damusTests/NIP10Tests.swift (3)
damus/Core/Types/Ids/Referenced.swift (1)
note_id(49-51)damus/Features/Posting/Views/PostView.swift (1)
nip10_reply_tags(816-833)nostrdb/NdbTagElem.swift (1)
id(128-131)
🔇 Additional comments (16)
.beads/issues.jsonl (1)
1-8: LGTM! Well-structured issue tracking data.The JSONL format is valid, field usage is consistent across all entries, and the content appropriately documents the relay hints implementation work described in the PR objectives. The dependency tracking, status management, and metadata are all properly structured.
damusTests/NIP10Tests.swift (9)
150-203: LGTM!The updated test correctly validates NIP-10 tag structure, verifying that the root tag lacks a pubkey (since the parent's e-tag didn't include one) while the reply tag includes the author's pubkey at position 4.
268-289: LGTM!Comprehensive test for NoteRef parsing with pubkey from position 4 of e-tags, correctly validating all NIP-10 fields.
291-310: LGTM!Good backwards compatibility test ensuring NoteRef correctly handles e-tags without the optional pubkey field.
312-327: LGTM!Validates that
NoteRef.tagcorrectly generates a 5-element array including the pubkey when present.
329-342: LGTM!Correctly validates that
NoteRef.tagomits the pubkey field when it's nil, producing a 4-element array.
344-367: LGTM!Tests the direct reply case where the parent note isn't in a thread, validating that the resulting root tag includes the parent's pubkey at position 4.
369-404: LGTM!Excellent test for pubkey propagation in threaded replies. Correctly validates that the root pubkey from the parent's e-tag is preserved while the reply tag includes the immediate parent's pubkey.
406-420: LGTM!Validates relay hint extraction from e-tag position 2.
422-435: LGTM!Important edge case test ensuring empty relay hints are preserved as empty strings rather than being converted to nil.
damus/Core/Types/Ids/Referenced.swift (3)
30-58: LGTM!Clean extension of
NoteRefwith optional pubkey support. The initializers maintain backwards compatibility with sensible defaults.
60-73: LGTM!The tag generation correctly follows NIP-10 format, only including pubkey when a marker is present (since pubkey is at position 4 and marker at position 3).
75-106: LGTM!The parsing logic correctly handles pubkey extraction with appropriate guards:
- Only parses pubkey when a valid marker is recognized
- Safely handles invalid pubkey data via
pk.id()returning nil- Aligns with the documented NIP-10 format
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (3)
419-437: Well-structured helper function.Clean implementation with good docstring coverage. The early return pattern and simple control flow align well with the coding guidelines.
469-473: Fire-and-forget Task in defer for ephemeral relay release.The
defer { Task { await ... } }pattern spawns an unstructured task that runs asynchronously after the function returns. This means:
- The release timing is non-deterministic
- Any errors from
releaseEphemeralRelaysare silently droppedThis may be intentional to avoid blocking the caller, and the
releaseEphemeralRelaysimplementation appears to handle races. However, verify this is the intended behavior, especially if ephemeral relay cleanup timing is critical for pool size management.
488-554: Good implementation of ephemeral relay support.The updated
findEventcorrectly uses theacquireEphemeralRelays+ensureConnectedpattern, with appropriate fallback to broadcast when no hinted relays connect (line 524). The docstring is clear and follows coding guidelines. The same fire-and-forget defer pattern noted elsewhere applies here (lines 530-534).
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift
Outdated
Show resolved
Hide resolved
Apply same acquireEphemeralRelays + ensureConnected pattern used by lookup(naddr:) to lookup(noteId:). Previously, noteId lookup only checked relays already in the pool, causing hint relays to be skipped entirely if not pre-connected. Also removes duplicate docstring block. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (1)
495-495: Remove redundant nil initialization.Optional variables default to
nilin Swift; explicit initialization is unnecessary.🔎 Suggested fix
- var filter: NostrFilter? = nil + var filter: NostrFilter?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift
🧰 Additional context used
📓 Path-based instructions (1)
**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
**/*.swift: Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Favor Swift-first solutions that lean on nostrdb types (Ndb, NdbNote, iterators) before introducing new storage mechanisms
Ensure docstring coverage for any code added, or modified
Ensure nevernesting: favor early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers of if statements
Files:
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift
🧬 Code graph analysis (1)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (3)
damus/Core/Nostr/RelayPool.swift (5)
acquireEphemeralRelays(183-190)releaseEphemeralRelays(196-225)ensureConnected(280-363)subscribe(463-473)subscribe(488-559)nostrdb/Ndb+.swift (1)
subscribe(21-29)nostrdb/UnownedNdbNote.swift (1)
justGetACopy(110-119)
🪛 SwiftLint (0.57.0)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift
[Warning] 495-495: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (4)
damus/Core/Networking/NostrNetworkManager/SubscriptionManager.swift (4)
358-422: Ephemeral relay handling now consistent and well-structured.The two-phase lookup approach (hint relays first with short timeout, then broadcast fallback with remaining budget) is well-implemented. The acquire/ensureConnected/release pattern is now consistent with
lookup(naddr:)andfindEvent, addressing the previous review feedback.
424-442: Clean helper extraction.Good encapsulation of the fetch logic with clear documentation of the return semantics.
452-488: LGTM!The ephemeral relay handling follows the same consistent pattern, and the fallback to broadcast (via
niltarget relays) when no hints connect is a sensible default.
520-536: Ephemeral relay handling is consistent.The acquire/ensureConnected/release pattern matches the other lookup functions, maintaining consistency across the codebase.
2 participants
User Benefit
Reduce the chance that notes are not found.
Summary
Implements relay hint consumption from NIP-10/NIP-18/NIP-19 for event fetching. When users view content that references events on other relays (quotes, reposts, mentions, replies, links), Damus extracts relay hints from tags and bech32 TLV data and uses them to fetch events from relays not in the user's relay pool.
This enables users to view notes and profiles even when the content only exists on relays they don't follow.
Changes
- Grace period: Wait 300ms after first relay connects to let others connect (improves multi-hint success)
- Ref-counted cleanup: Use acquire/release leasing to prevent race conditions during concurrent lookups
Limitations
Checklist
Standard PR Checklist
Test report
Device: iPhone 17 Simulator
iOS: iOS 26
Damus: Branch relay-hints (commit 79731e9)
Setup: Xcode iOS Simulator with console debug logging enabled. Used nak CLI tool to decode nevents and query relays directly.
Steps:
Results:
###Other notes
Used nak CLI tool for debugging:
nak decode nevent1q...
nak req -i wss://relay.example.com
Closes #1147
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.