Update invite copy and app icon, show invite preview in composer#541
Conversation
- Detect invite URLs when pasted/typed in message input - Show invite preview in attachments area with Convos icon - Send full invite URL as separate message before any text - Replace old icon assets with convosOrangeIcon throughout
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Claude finished @yewreeka's task —— View job Code Review CompleteI've thoroughly reviewed this PR. Overall, the implementation is solid with good test coverage, but there's one critical bug that needs to be addressed along with several other improvements. 🔴 Critical IssueReply Context Lost for Invite-Only SendsLocation: When a user replies to a message and sends only an invite (no text, no attachment), the invite URL is sent as a regular message instead of as a reply. The The problem: if let inviteURL = prevInviteURL {
try await messageWriter.send(text: inviteURL, afterPhoto: photoTrackingKey)
}This code doesn't check Fix: if let inviteURL = prevInviteURL {
let inviteIsReply = replyTarget != nil && !hasAttachment && !hasText
if inviteIsReply, let replyTarget {
try await messageWriter.sendReply(text: inviteURL, afterPhoto: photoTrackingKey, toMessageWithClientId: replyTarget.base.id)
} else {
try await messageWriter.send(text: inviteURL, afterPhoto: photoTrackingKey)
}
}Impact: Users expecting to reply with an invite will see the invite appear as a regular message instead, breaking conversational context. 🟡 Code Quality & Best Practices1. Performance - Invite Detection on Every KeystrokeLocation: The Current implementation is acceptable, but monitor for performance issues. 2. Invite Code Length Validation IssueLocation: When validating invite codes containing asterisks (e.g., Current code: let potentialCode = trimmedText.replacingOccurrences(of: "*", with: "")
if isLikelyInviteCode(potentialCode), // checks potentialCode.count >= 50
(try? SignedInvite.fromURLSafeSlug(potentialCode)) != nil {Why this matters: If a user pastes However, looking at the latest commit (a6bad93), this appears to have been addressed by using 3. Test Coverage - Missing Edge CasesLocation: Great test coverage overall (305 lines!), but missing a few scenarios:
Recommendation: Add a test for invalid base64 characters to ensure graceful handling. 4. UI State ConsistencyLocation: The view receives However, consider passing the full Current approach is acceptable for this scope. 🟢 Positive Observations1. Excellent Separation of Concerns
2. Strong Test Coverage
3. Proper iOS Patterns
4. Database IntegrationLocation: The invite detection integrates seamlessly with the existing message content type system, detecting invites in both regular text messages and replies. This ensures invite URLs are properly rendered as invite cards in the message list. 📋 SwiftLint Compliance CheckReviewing against the project's SwiftLint rules:
One minor note: The poof animation dispatch (line 161) could use a comment explaining the 0.2s delay, but this is a minor style preference. 🔍 Security & Architecture Review1. Input Validation ✅
2. Cross-Platform Compatibility ✅
3. State Management ✅
📝 SummaryMust Fix:
Should Consider:
Nice to Have:
Excellent Work:
The critical bug is straightforward to fix - just add the conditional reply logic for invites. Once that's addressed, this PR is in great shape! 🚀 |
yewreeka
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
- Fix crash: range indices from trimmedText were used on original text, causing invalid index when input had leading/trailing whitespace - Add 33 unit tests covering URL detection, range accuracy, edge cases - Consolidate pendingInviteCode/URL/Range into PendingInvite struct - Cache NSDataDetector as static property instead of recreating per call - Replace silent try? with guard + Log.error - Extract magic number 50 to named constant minimumInviteCodeLength - Change view pendingInviteCode from @binding to plain value (read-only)
…ring - Run NSDataDetector on original text instead of trimmedText, eliminating error-prone range translation logic - Use potentialCode (asterisks stripped) for SignedInvite.fromURLSafeSlug instead of trimmedText, fixing decode failure for starred codes - Pass afterPhoto:photoTrackingKey when sending invite URL so photo always appears before the invite in message ordering
- Fix reply hydration dropping invite messages (returned nil for .invite content type in composeReplyMessage) - Fix XMTP reply decoding not detecting invite URLs in text replies, causing invite card to briefly appear then revert to raw text - Send invite-only replies via sendReply to preserve reply association - Run NSDataDetector on original text to simplify range handling
2 participants
Note
Add invite URL detection and attachment preview in the message composer
NSDataDetectorto find Convos invite URLs or raw invite codes in message text, returning the code, full URL, and character range.ConversationViewModel.checkForInviteURL()extracts any detected invite URL frommessageTextand stores it as pending invite state; the send button enables when a pending invite exists.messageWriter.send(text:afterPhoto:), followed by any remaining text or photo.MessagesInputViewrenders a removable chip preview for the pending invite alongside photo attachments, with a poof animation on removal.convosIcon/convosIconLargeimage assets withconvosOrangeIconacross share, QR, and invite preview views.Changes since #541 opened
InviteURLDetector.detectInviteURLto map ranges from trimmed text back to the original input string and implemented cachedNSDataDetectorinstance [ecb62c4]PendingInvitestruct inConversationViewModel[ecb62c4]pendingInviteCodefrom binding to value parameter throughout SwiftUI view hierarchy [ecb62c4]InviteURLDetectorTestscovering invite URL detection, code validation, and range mapping scenarios [ecb62c4]InviteURLDetector.detectInviteURLto operate on the full input text instead of trimmed substrings, withnsRangecreated from the full text,enumerateMatchesrunning on the full text, and match ranges taken directly from the full text, and for inferred codes without explicit URLs, validation now uses the asterisk-strippedpotentialCodeand constructsfullURLwithpotentialCode, returning the range as the entire original text rather than the trimmed range [a6bad93]ConversationViewModelmessage send flow to passafterPhotoparameter set tophotoTrackingKeyinstead ofnilwhen sending invite URLs [a6bad93]📊 Macroscope summarized a72f154. 9 files reviewed, 10 issues evaluated, 7 issues filtered, 1 comment posted
🗂️ Filtered Issues
Convos/Conversation Detail/ConversationView.swift — 0 comments posted, 2 evaluated, 2 filtered
pendingInviteCode != nilto allow sending, but the actual send logic usespendingInviteURL. IfpendingInviteCodeis set butpendingInviteURLis nil (due to sync issues between these properties), the guard will pass allowing the send to proceed, but no invite will actually be sent. This could result in a user expecting an invite to be sent when only an empty message (or nothing) is sent. [ Low confidence ].onChange(of: viewModel.messageText)modifier callsviewModel.checkForInviteURL()on every keystroke. IfcheckForInviteURL()performs expensive operations (like regex matching or URL parsing) synchronously, this could cause UI lag during typing, especially for longer messages. [ Low confidence ]Convos/Conversation Detail/ConversationViewModel.swift — 0 comments posted, 3 evaluated, 1 filtered
sendButtonEnablednow returnstruewhenpendingInviteCode != nil, enabling the send button with only a pending invite code. However, there is no visible implementation in the provided code showing how the send action handles this case. If the send logic does not check for and processpendingInviteCode, pressing send with only a pending invite (no text, no photo) may result in a no-op or unexpected behavior. [ Low confidence ]Convos/Conversation Detail/Messages/Messages View Controller/View Controller/Cells/MessageInviteContainerView.swift — 0 comments posted, 1 evaluated, 1 filtered
"convosIconLarge"to"convosOrangeIcon". If theconvosOrangeIconasset does not exist in the asset catalog, SwiftUI'sImage(_:)initializer will silently fail and render an empty view, causing the placeholder to appear blank instead of showing the intended fallback icon. [ Low confidence ]Convos/Conversation Detail/Messages/MessagesView.swift — 0 comments posted, 2 evaluated, 2 filtered
pendingInviteCode != nilto allow sending, but the actual send logic usespendingInviteURL. If these two state variables can become inconsistent (e.g.,pendingInviteCodeis set butpendingInviteURLis nil), the send operation will pass the guard but silently send nothing when the user only has an invite pending (no text or attachment). [ Low confidence ]afterPhoto: nilinstead of usingphotoTrackingKey. This could cause the invite message to appear before the photo in the conversation, rather than maintaining the expected order where messages sent together appear in sequence. [ Skipped comment generation ]Convos/Utilities & Extensions/InviteURLDetector.swift — 1 comment posted, 2 evaluated, 1 filtered
isLikelyInviteCode(line 82:text.count >= 50) is performed onpotentialCodeafter asterisks are removed (line 41), which could cause valid invite codes containing asterisks to fail the length threshold check and not be recognized as invite codes. [ Low confidence ]