Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds typing indicator support for DMs: introduces Nostr kind 20001, creates encrypted typing events, handles receipt/decryption in HomeModel, tracks partner typing state in DirectMessageModel, integrates UI sending/throttling in DMChatView, and adds unit tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DMChatView
participant DMTypingIndicator
participant Relay as Network/Relay
participant HomeModel
participant DirectMessageModel
rect rgba(100, 150, 200, 0.5)
Note over User,DMChatView: Sending typing indicator
User->>DMChatView: Edit message draft
DMChatView->>DMChatView: Detect draft change / throttle
DMChatView->>DMTypingIndicator: makeEvent(action: start, to: recipient, ...)
DMTypingIndicator->>DMTypingIndicator: Encrypt content via NIP04
DMTypingIndicator-->>DMChatView: NostrEvent (kind 20001)
DMChatView->>Relay: Send typing start event (throttled)
end
rect rgba(150, 100, 200, 0.5)
Note over Relay,HomeModel: Receiving typing indicator
Relay->>HomeModel: Receive typing event (kind 20001)
HomeModel->>HomeModel: handle_typing_indicator()
HomeModel->>HomeModel: Verify namespace tag & recipient, decrypt content
HomeModel->>DirectMessageModel: set_partner_typing(true)
DirectMessageModel->>DirectMessageModel: Schedule auto-clear timer
DirectMessageModel-->>DMChatView: partner_is_typing updated
DMChatView->>DMChatView: Render "Typing…" indicator
end
rect rgba(200, 100, 100, 0.5)
Note over DirectMessageModel,DMChatView: Auto-clear typing
DirectMessageModel->>DirectMessageModel: Timer expires
DirectMessageModel->>DirectMessageModel: set_partner_typing(false)
DirectMessageModel-->>DMChatView: partner_is_typing = false
DMChatView->>DMChatView: Hide "Typing…" indicator
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Welcome back @joelklabo @coderabbitai review please |
|
✅ Actions performedReview triggered.
|
|
Still working on this @alltheseas Good to 👀 you! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@damus/Features/DMs/Models/DMTypingIndicator.swift`:
- Around line 31-32: The expiration calculation in DMTypingIndicator (variables
created and expiration using ttlSeconds) can overflow when adding ttlSeconds to
created; update the logic to detect overflow (use UInt32.addingReportingOverflow
or perform a checked add) and clamp to UInt32.max or handle the error path
instead of allowing a trap—compute created =
UInt32(Date().timeIntervalSince1970) as before, then call
created.addingReportingOverflow(ttlSeconds) and if overflowed set expiration =
UInt32.max (or return/abort gracefully), otherwise set expiration to the sum.
- Around line 21-30: Add Swift doc comments for the new public-facing type and
its API: document the DMTypingIndicator struct and its static factory method
makeEvent(action:to:keypair:ttlSeconds:) by describing their purpose,
parameters, return value and any important behavior/constraints (e.g., that
makeEvent builds a NostrEvent for typing with TTL and may return nil on
failure). Place /// doc comments above the DMTypingIndicator declaration and
above the makeEvent method, mentioning the meaning of action, recipient
(Pubkey), keypair, ttlSeconds (default 30s), and when nil is returned.
In `@damus/Features/DMs/Views/DMChatView.swift`:
- Around line 16-18: Add triple-slash doc comments describing the purpose and
behavior of the outbound typing indicator state and helper: document `@State`
private var lastTypingStartSentAt: Date? = nil (tracks when the last
typing-start event was sent and why it's optional), the private let
typingStartThrottleSeconds: TimeInterval = 5.0 (explain it is the throttle
interval and units), and the send_typing helper function (describe what it
sends, when it should be called, and how it respects the throttle). Place these
/// comments immediately above the declarations for lastTypingStartSentAt,
typingStartThrottleSeconds, and the send_typing method so they follow Swift doc
comment conventions.
In `@damus/Features/Timeline/Models/HomeModel.swift`:
- Around line 957-996: In handle_typing_indicator(_ ev: NostrEvent) skip
processing if the event has an expiration tag that is already in the past:
inspect ev.tags for the expiration-style tag (e.g. a tag where t[0] ==
"expiration" or the convention used in your Nostr model and t[1] is a unix
timestamp), parse t[1] to an integer/time interval and compare against
Date().timeIntervalSince1970 (or Date()), and return early when the expiration
timestamp < now so stale typing events are ignored before decrypting or updating
model.set_partner_typing; keep the check above decryption and the other guards
in handle_typing_indicator.
In `@damusTests/DMTypingIndicatorTests.swift`:
- Around line 11-58: Add missing docstrings for the test class and its methods:
add a triple-slash comment above the DMTypingIndicatorTests declaration
describing the test suite, and triple-slash comments above
testTypingIndicatorEventCreationAndDecryption and
testDirectMessageModelTypingAutoClear describing each test’s purpose (e.g.,
event creation/decryption and typing auto-clear behavior). Keep comments short
and focused so they satisfy docstring coverage without changing any test logic
or identifiers.
|
@joelklabo can you have the agent run the code by |
Signed-off-by: Joel Klabo <joelklabo@gmail.com>
Signed-off-by: Joel Klabo <joelklabo@gmail.com>
Signed-off-by: Joel Klabo <joelklabo@gmail.com>
joelklabo
force-pushed
the
feat/dm-typing-indicators
branch
from
d14ce0d to
9904662
Compare
Signed-off-by: Joel Klabo <joelklabo@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds best-effort typing indicators to DMs using kind 20001 ephemeral Nostr events. Users see "Typing…" in the DM chat UI when their conversation partner is actively typing. Events are NIP-04 encrypted and include expiration tags with a namespace identifier ("damus-typing") to distinguish them from other clients.
Changes:
- Added
DMTypingIndicatorhelper struct andDMTypingActionenum to create encrypted kind 20001 typing indicator events - Modified
DirectMessageModelto track partner typing state with auto-clear timeout - Updated
DMChatViewto send typing start/stop events (with throttling) and display typing indicator UI - Extended
HomeModelto subscribe to and handle kind 20001 typing events - Added unit tests for event creation/decryption and auto-clear behavior
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| damus/Features/DMs/Models/DMTypingIndicator.swift | New helper for creating encrypted kind 20001 typing indicator events with expiration tags |
| damus/Features/DMs/Models/DirectMessageModel.swift | Added partner typing state tracking with auto-clear timer |
| damus/Features/DMs/Views/DMChatView.swift | Integrated typing event sending (throttled) and "Typing…" UI indicator |
| damus/Features/Timeline/Models/HomeModel.swift | Added subscription filters and handler for kind 20001 typing events |
| damus/Core/Nostr/NostrKind.swift | Added .typing = 20001 case for ephemeral typing indicators |
| damus/Features/Events/Models/LoadableNostrEventView.swift | Added .typing to unsupported kinds list |
| damusTests/DMTypingIndicatorTests.swift | Unit tests for event creation/decryption and auto-clear behavior |
| damus.xcodeproj/project.pbxproj | Added new files to project build targets |
| docs/typing-indicator-pr3551.png | Screenshot showing typing indicator UI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var dms_filter = NostrFilter(kinds: [.dm, .typing]) | ||
| dms_filter.pubkeys = [damus_state.pubkey] | ||
| dms_filter.limit = 500 | ||
|
|
||
| // DMs we sent | ||
| var our_dms_filter = NostrFilter(kinds: [.dm]) | ||
| var our_dms_filter = NostrFilter(kinds: [.dm, .typing]) | ||
| our_dms_filter.authors = [damus_state.pubkey] | ||
| our_dms_filter.limit = 500 |
There was a problem hiding this comment.
Typing indicator events (kind 20001) should not be fetched via negentropy or included in DM history. These are ephemeral events that should only be received in real-time and should not be persisted or retrieved from history.
The filters in fetchFullDMHistory() should only request kind 4 (DMs), not kind 20001 (typing indicators). Including typing indicators in historical DM fetches with limits (500 events) could cause older actual DMs to be missed, and there's no value in fetching old typing indicators since they're time-sensitive ephemeral signals.
| var dms_filter = NostrFilter(kinds: [.dm, .typing]) | ||
|
|
||
| var our_dms_filter = NostrFilter(kinds: [.dm]) | ||
| var our_dms_filter = NostrFilter(kinds: [.dm, .typing]) |
There was a problem hiding this comment.
The subscription filters for DMs should probably not include kind 20001 (typing indicators) in real-time subscriptions. Typing indicators are intentionally ephemeral and should not be stored in NostrDB or included in persistent subscriptions.
Consider either:
- Subscribing to typing indicators separately with a dedicated ephemeral-only subscription that doesn't store events, or
- Handling typing indicators through a different mechanism that explicitly marks them as ephemeral to prevent storage.
Including them in the same filter as regular DMs means they'll be treated the same way (stored in NostrDB), which defeats the purpose of them being ephemeral best-effort signals.
| senderPubkey: ev.pubkey, | ||
| content: ev.content, | ||
| encoding: .base64 | ||
| )) ?? ev.content |
There was a problem hiding this comment.
The comment says "If decryption fails, assume 'start' (clients may send plaintext)" but the code actually falls back to ev.content (the raw encrypted/plaintext content), not to "start".
This could lead to unexpected behavior:
- If decryption fails and the content is "stop" (plaintext), it would be treated as "start" because the only check is
action != "stop"and a failed decryption fallback to encrypted gibberish wouldn't equal "stop". - The comment suggests plaintext is supported, but the encryption in
DMTypingIndicator.makeEventalways encrypts, so there's no path for plaintext events in this codebase.
The fallback logic should either:
- Explicitly default to "start" as the comment suggests:
?? "start", or - Update the comment to match the actual behavior of falling back to the content value.
| )) ?? ev.content | |
| )) ?? "start" |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@damus/Features/DMs/Views/DMChatView.swift`:
- Around line 145-167: The throttle logic in send_typing(_:force:) fails to
clear lastTypingStartSentAt on a .stop, causing a subsequent .start to be
suppressed; update send_typing (the method that creates events via
DMTypingIndicator.makeEvent and posts via damus_state.nostrNetwork.postbox.send)
so that when action == .stop you reset lastTypingStartSentAt (e.g., set it to
nil) before returning, while keeping the existing behavior of updating
lastTypingStartSentAt only after successfully creating a .start event.
🧹 Nitpick comments (2)
damus/Features/DMs/Models/DMTypingIndicator.swift (1)
30-42: Consider adding- Returns:documentation.The docstring documents parameters well but omits the return value documentation. Swift conventions and the coding guidelines recommend complete docstring coverage.
📝 Suggested addition
/// - Parameters: /// - action: Whether the user started or stopped typing. /// - recipient: The pubkey of the DM recipient. /// - keypair: Sender keypair. /// - ttlSeconds: Best-effort expiration in seconds, encoded in an `expiration` tag. + /// - Returns: An encrypted `NostrEvent`, or `nil` if keypair conversion or encryption fails. static func makeEvent(damus/Features/Timeline/Models/HomeModel.swift (1)
957-1005: Move NIP04 decryption off the MainActor to avoid UI stalls.
handle_typing_indicatorruns on@MainActor, and decrypting per typing signal can be CPU work. Consider doing the decrypt in a background task, then hop back to@MainActorto updatepartner_is_typing. Also, reusingDMTypingIndicator.namespaceTagValuewill avoid hard‑coding the namespace string. As per coding guidelines, avoid expensive work on the main thread.♻️ Small maintainability tweak
- let hasNamespace = ev.tags.contains { t in - t.count >= 2 && t[0].matches_str("t") && t[1].matches_str("damus-typing") - } + let hasNamespace = ev.tags.contains { t in + t.count >= 2 && t[0].matches_str("t") && t[1].matches_str(DMTypingIndicator.namespaceTagValue) + }
Signed-off-by: Joel Klabo <joelklabo@gmail.com>
|
Addressed review feedback:
Commit: e94d2af |
|
Hi @alltheseas - I've addressed the previous feedback. The PR is now mergeable. The docstring coverage is at 66.67% which is below the 80% threshold but the code is functional. Would you be able to review and merge this PR? The typing indicators feature is complete and ready. |
|
Hey ser, with agents and beads we now have the "merge problem" and a volley of PRs. Right now trying out the Linux one branch multi commit approach, as opposed to having 70+ open PRs. Dont hold your breath for typing indicator PR merge soon. That said if the Linux branch approach works and makes it easier for Will, and Daniel's review process, it makes sense to add the typing indicator PR on the same branch as nip-17 dm PR. |
|
Thank you @joelklabo, and I apologize for the long wait. What NIP spec is this implementation based on? |
danieldaquino
left a comment
danieldaquino
left a comment
There was a problem hiding this comment.
Please let me know the spec this change is based on so I can provide a more accurate review.
Thank you!
Summary
Adds best-effort typing indicators to direct messages using kind 20001 ephemeral events.
start/stoptyping events (NIP-04 encrypted) witht=damus-typingnamespace andexpirationtag.Checklist (Standard PR)
Test report
Device: iPhone 16e (Simulator)
iOS: 26.0
Damus: PR branch
feat/dm-typing-indicatorsSteps:
Results:
damusTests)Visual evidence
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.