Skip to content

Update invite copy and app icon, show invite preview in composer#541

Open
yewreeka wants to merge 1 commit intodevfrom
invite-url-detection
Open

Update invite copy and app icon, show invite preview in composer#541
yewreeka wants to merge 1 commit intodevfrom
invite-url-detection

Conversation

@yewreeka
Copy link
Contributor

@yewreeka yewreeka commented Feb 27, 2026

  • 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

Note

Add invite URL detection and attachment preview in the message composer

  • Adds InviteURLDetector.swift, a utility that uses NSDataDetector to find Convos invite URLs or raw invite codes in message text, returning the code, full URL, and character range.
  • As the user types, ConversationViewModel.checkForInviteURL() extracts any detected invite URL from messageText and stores it as pending invite state; the send button enables when a pending invite exists.
  • On send, the invite URL is dispatched first via messageWriter.send(text:afterPhoto:), followed by any remaining text or photo.
  • MessagesInputView renders a removable chip preview for the pending invite alongside photo attachments, with a poof animation on removal.
  • Replaces several references to removed convosIcon/convosIconLarge image assets with convosOrangeIcon across share, QR, and invite preview views.
📊 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
  • [line 51](https://github.com/xmtplabs/convos-ios/blob/a72f154ce3379895a59fbc9ce85c95abe6f04aeb/Convos/Conversation Detail/ConversationView.swift#L51): The guard condition checks pendingInviteCode != nil to allow sending, but the actual send logic uses pendingInviteURL. If pendingInviteCode is set but pendingInviteURL is 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 ]
  • [line 103](https://github.com/xmtplabs/convos-ios/blob/a72f154ce3379895a59fbc9ce85c95abe6f04aeb/Convos/Conversation Detail/ConversationView.swift#L103): The .onChange(of: viewModel.messageText) modifier calls viewModel.checkForInviteURL() on every keystroke. If checkForInviteURL() 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
  • [line 214](https://github.com/xmtplabs/convos-ios/blob/a72f154ce3379895a59fbc9ce85c95abe6f04aeb/Convos/Conversation Detail/ConversationViewModel.swift#L214): sendButtonEnabled now returns true when pendingInviteCode != 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 process pendingInviteCode, 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
  • [line 91](https://github.com/xmtplabs/convos-ios/blob/a72f154ce3379895a59fbc9ce85c95abe6f04aeb/Convos/Conversation Detail/Messages/Messages View Controller/View Controller/Cells/MessageInviteContainerView.swift#L91): The image asset name was changed from "convosIconLarge" to "convosOrangeIcon". If the convosOrangeIcon asset does not exist in the asset catalog, SwiftUI's Image(_:) 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
  • [line 88](https://github.com/xmtplabs/convos-ios/blob/a72f154ce3379895a59fbc9ce85c95abe6f04aeb/Convos/Conversation Detail/Messages/MessagesView.swift#L88): The guard condition checks pendingInviteCode != nil to allow sending, but the actual send logic uses pendingInviteURL. If these two state variables can become inconsistent (e.g., pendingInviteCode is set but pendingInviteURL is 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 ]
  • [line 714](https://github.com/xmtplabs/convos-ios/blob/a72f154ce3379895a59fbc9ce85c95abe6f04aeb/Convos/Conversation Detail/Messages/MessagesView.swift#L714): When sending a message with both a photo attachment and a pending invite, the invite URL is sent with afterPhoto: nil instead of using photoTrackingKey. 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

- 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
Copy link
Contributor Author


How to use the Graphite Merge Queue

Add 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
Copy link

claude bot commented Feb 27, 2026

Claude finished @yewreeka's task —— View job


Pull Request Review

Comprehensive review of the invite URL detection feature.


Overview

This PR successfully implements invite URL detection in the message composer. The implementation is clean and follows project conventions well. The feature detects invite URLs when pasted/typed, shows a preview in the attachments area, and sends the full URL as a separate message.


Code Quality & Best Practices

✅ Strengths

  1. Clean Architecture: InviteURLDetector is well-structured as a stateless enum with clear separation of concerns
  2. SwiftUI Conventions: Proper use of @Observable, @State, and bindings throughout
  3. Error Handling: Graceful fallbacks when URLs aren't detected
  4. Accessibility: Good use of accessibility labels and identifiers
  5. Code Style: Follows project conventions (K&R braces, guard preference, naming)
  6. Asset Cleanup: Consolidation to convosOrangeIcon improves consistency

⚠️ Issues & Improvements

1. Missing Test CoverageHIGH PRIORITY

InviteURLDetector.swift:1 has no unit tests. This is critical functionality that should be thoroughly tested.

Recommended tests:

  • Valid HTTPS invite URLs (with all associated domains)
  • Valid custom URL scheme invites
  • Raw invite codes (base64url format)
  • Invalid URLs (wrong domain, wrong scheme)
  • Edge cases (empty strings, whitespace, multiple URLs)
  • Range detection accuracy
  • URL removal logic

2. Force Try in Detection Logic ⚠️

InviteURLDetector.swift:19 uses try? to silently suppress NSDataDetector initialization errors:

let detector = try? NSDataDetector(types: NSTextCheckingResult.CheckingType.link.rawValue)

While NSDataDetector initialization rarely fails, this could silently fail without logging. Consider:

guard let detector = try? NSDataDetector(types: NSTextCheckingResult.CheckingType.link.rawValue) else {
    Log.error("Failed to create NSDataDetector for invite URL detection")
    return nil
}

3. Magic Number - Minimum Code Length 📝

InviteURLDetector.swift:82 has a hardcoded magic number:

return text.count >= 50

Recommendation: Extract to a named constant with documentation:

private static let minimumInviteCodeLength: Int = 50  // Compressed protobuf invite data

4. Performance - Multiple String Operations

ConversationViewModel.swift:104 calls checkForInviteURL() on every text change, which:

  • Creates a new NSDataDetector each time
  • Scans the entire text for URLs
  • Performs string manipulation

Recommendations:

  • Consider debouncing or only checking when text contains common URL patterns (http, ://, .)
  • Reuse NSDataDetector instance if possible
  • Only check when text length crosses thresholds

5. State Management - Pending Invite Properties

ConversationViewModel.swift:209-211 adds three separate properties:

var pendingInviteCode: String?
var pendingInviteURL: String?
private var pendingInviteRange: Range<String.Index>?

Recommendation: Encapsulate in a struct for better cohesion:

struct PendingInvite {
    let code: String
    let fullURL: String
    let range: Range<String.Index>
}
var pendingInvite: PendingInvite?

This would simplify clearing (pendingInvite = nil) and reduce the chance of inconsistent state.

6. Message Sending Order 🔍

ConversationViewModel.swift:714-717 sends invite URL first, then text:

// Send invite URL FIRST (renders as invite preview)
if let inviteURL = prevInviteURL {
    try await messageWriter.send(text: inviteURL, afterPhoto: nil)
}

This is intentional (per the comment), but consider:

  • What happens if the invite send succeeds but the text send fails?
  • Should they be sent atomically or with error recovery?

Security Concerns

✅ Good Security Practices

  1. Domain Validation: InviteURLDetector.swift:60-61 validates against ConfigManager.shared.associatedDomains
  2. Scheme Validation: Checks for both HTTPS and custom app URL scheme
  3. Invite Validation: InviteURLDetector.swift:44 verifies invite codes with SignedInvite.fromURLSafeSlug()

🔒 Recommendations

1. URL Scheme Validation

InviteURLDetector.swift:59-66 allows custom schemes but doesn't log suspicious activity:

if url.scheme == "https" {
    guard let host = url.host,
          ConfigManager.shared.associatedDomains.contains(host) else {
        return nil
    }
} else if url.scheme != ConfigManager.shared.appUrlScheme {
    return nil
}

Consider logging rejected URLs for security monitoring (without PII).

2. Invite Code Validation Edge Case

InviteURLDetector.swift:41 removes asterisks before validation:

let potentialCode = trimmedText.replacingOccurrences(of: "*", with: "")

Why are asterisks allowed? If they're separators, ensure this is documented and tested. This could be a vector for obfuscated malicious input.


Performance Considerations

⚡ Areas of Concern

  1. Real-time Detection: Running NSDataDetector on every keystroke could impact typing performance with long messages
  2. String Manipulation: removeInviteURL performs substring removal and trimming on each detection
  3. No Caching: Detection results aren't cached even if text hasn't changed significantly

💡 Optimization Ideas

  1. Only check when paste occurs or text contains URL patterns
  2. Cache the last detected result and invalidate only when relevant text changes
  3. Consider background queue for detection if text is very long

Potential Bugs

1. Range Mutation After Detection 🐛

ConversationViewModel.swift:1219 removes the URL from messageText:

messageText = InviteURLDetector.removeInviteURL(from: messageText, range: result.range)

But pendingInviteRange is stored for later use. If messageText is mutated again (e.g., user edits), the stored range becomes invalid but is never cleared. This could cause crashes or incorrect behavior.

Recommendation: Clear pendingInviteRange when messageText changes, or only store it for immediate use.

2. Multiple Invites in Text

InviteURLDetector.swift:34 stops after finding the first invite:

stop.pointee = true

This is likely intentional, but what if a user pastes multiple invite URLs? Should there be a limit or UX feedback?

3. Empty Message with Invite

ConversationViewModel.swift:663 allows sending with only an invite:

guard hasText || hasAttachment || hasInvite else { return }

Ensure this is the desired UX - sending a message with just an invite and no text.


Test Coverage

❌ Missing Tests

  • No unit tests for InviteURLDetector (critical gap)
  • No integration tests for the invite detection flow in ConversationViewModel
  • No UI tests for the invite preview attachment

✅ Recommended Test Suite

// InviteURLDetectorTests.swift
- testDetectsHTTPSInviteURL()
- testDetectsCustomSchemeURL()
- testDetectsRawInviteCode()
- testRejectsInvalidDomain()
- testRejectsInvalidScheme()
- testRejectsShortCode()
- testRemovesURLFromText()
- testHandlesEmptyString()
- testHandlesWhitespaceOnly()
- testRangeAccuracy()
- testMultipleURLsReturnsFirst()

Additional Observations

Icon Asset Changes

The PR removes old icon assets (convosIcon, convosIconLarge, convosIconRounded) and consolidates to convosOrangeIcon. This is good for consistency, but:

  1. Ensure all references are updated (grep shows 7 files - verify all are in this PR)
  2. Consider asset catalog cleanup - verify no orphaned assets remain
  3. Check App Clip target - ensure it has access to the new assets

Accessibility

Good use of accessibility labels throughout (MessagesInputView.swift:209, 233). Consider adding:

  • VoiceOver hint for what happens when invite is detected
  • Announcement when invite is auto-extracted from text

Summary & Recommendations

Must Fix Before Merge

  1. Add unit tests for InviteURLDetector
  2. 🐛 Fix range mutation bug - clear pendingInviteRange when text changes

Should Fix

  1. Extract magic number (minimum code length) to constant
  2. Encapsulate pending invite state in a struct
  3. Add logging for detector initialization failures
  4. Consider performance optimizations (debouncing, caching)

Nice to Have

  1. Document asterisk handling in invite codes
  2. Add integration tests for the full flow
  3. Consider UX for multiple invite URLs in one message
  4. Add VoiceOver announcements for invite detection

Overall Assessment: This is solid work that follows project conventions well. The implementation is clean and the UX improvement is valuable. The main concern is lack of test coverage for critical URL detection logic. With tests added and the range bug fixed, this would be ready to merge.

Recommendation: Request changes for test coverage and bug fix, then approve.


@yewreeka yewreeka marked this pull request as ready for review February 27, 2026 18:28
@yewreeka yewreeka changed the title Add invite URL detection in message composer Update invite copy and app icon, show invite preview in composer Feb 27, 2026
detector?.enumerateMatches(in: trimmedText, options: [], range: range) { match, _, stop in
guard let match = match,
let url = match.url,
let matchRange = Range(match.range, in: trimmedText) else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High Utilities & Extensions/InviteURLDetector.swift:27

matchRange is computed from trimmedText (line 27) but returned for use on the original text in removeInviteURL. When leading/trailing whitespace exists, these indices won't match, causing a crash. Consider computing the range relative to the original text instead.

Suggested change
let matchRange = Range(match.range, in: trimmedText) else {
let matchRange = Range(match.range, in: text) else {
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file Convos/Utilities & Extensions/InviteURLDetector.swift around line 27:

`matchRange` is computed from `trimmedText` (line 27) but returned for use on the original `text` in `removeInviteURL`. When leading/trailing whitespace exists, these indices won't match, causing a crash. Consider computing the range relative to the original `text` instead.

Evidence trail:
Convos/Utilities & Extensions/InviteURLDetector.swift lines 15, 20, 27, 32 (at REVIEWED_COMMIT) - shows trimmedText creation and matchRange computed from trimmedText.
Convos/Conversation Detail/ConversationViewModel.swift lines 1215, 1219 (at REVIEWED_COMMIT) - shows detectInviteURL called with messageText, then removeInviteURL called with same messageText and the returned range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant