D14z client prerelease updates

[cameronvoell]

Introduction 📟

Purpose ℹ️

Scope 🔭

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue 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.

[claude]

Claude finished @cameronvoell's task —— View job

---

Code Review Complete ✅

Summary

This PR updates the XMTP iOS SDK with D14Z client prerelease changes, primarily focusing on API client caching improvements, a new message visibility options API, and a test for D14N staging testnet compatibility. Overall, the changes are well-structured and follow good practices.

---

🟢 Strengths

1. API Client Caching Enhancement (Client.swift:96-133, 390-442)

• Excellent cache key design: The new ApiClientCache.makeCacheKey() method properly incorporates all configuration parameters (v3Host, gatewayHost, isSecure, appVersion) to prevent cache collisions
• Good documentation: Clear comments explain the caching strategy
• Proper actor isolation: Thread-safe implementation using Swift actors

2. MessageVisibilityOptions API (MessageVisibilityOptions.swift)

• Clean abstraction: New MessageVisibilityOptions struct provides a clear API for controlling message push notifications
• Good naming: The shouldPush parameter is self-explanatory
• Proper integration: Consistently integrated across Conversation, Dm, and Group types

3. Test Coverage (ClientTests.swift:345-372)

• Good addition: New test testCreatesAD14NStagingClient validates integration with D14N staging environment
• Uses production-like settings: Tests with isSecure: true and custom gateway URL

---

🟡 Issues & Concerns

1. Missing PR Description ⚠️

Location: PR body (Introduction, Purpose, Scope sections are empty)

Issue: The PR lacks any description of:

• What changes were made and why
• What the D14Z prerelease entails
• Breaking changes or migration notes
• Testing performed

Recommendation: Add a comprehensive PR description explaining:

## Introduction
D14Z prerelease updates for XMTP iOS SDK

## Purpose  
- Improve API client caching to support gateway URLs
- Add MessageVisibilityOptions API for finer control over push notifications
- Validate compatibility with D14N staging testnet

## Scope
- API client caching refactor
- New MessageVisibilityOptions type
- Test coverage for D14N staging

2. Potential Breaking Change - Not Documented ⚠️

Location: Conversation.swift:140-154, 225-238

Issue: New visibilityOptions parameter added to prepareMessage() and send() methods. While it's optional (defaults to nil), this changes method signatures.

Example:

public func prepareMessage(
    encodedContent: EncodedContent, 
    visibilityOptions: MessageVisibilityOptions? = nil  // NEW parameter
) async throws -> String

Impact:

• Existing code will continue to work (backward compatible)
• However, developers should be informed about this new capability

Recommendation: Document this in release notes as a new feature, clarifying it's backward compatible.

3. Inconsistent Error Handling Pattern ⚠️

Location: Group.swift:334-343

Issue: Redundant try-catch that just rethrows:

do {
    let opts = visibilityOptions?.toFfi() ?? FfiSendMessageOpts(shouldPush: true)
    let messageId = try await ffiGroup.send(
        contentBytes: encodedContent.serializedData(),
        opts: opts
    )
    return messageId.toHex
} catch {
    throw error  // Just rethrows without adding value
}

Comparison: The equivalent code in Dm.swift:156-165 doesn't have this unnecessary wrapper.

Recommendation: Remove the do-catch block in Group.swift:334-343:

let opts = visibilityOptions?.toFfi() ?? FfiSendMessageOpts(shouldPush: true)
let messageId = try await ffiGroup.send(
    contentBytes: encodedContent.serializedData(),
    opts: opts
)
return messageId.toHex

4. Test Hardcodes Specific Gateway URL ℹ️

Location: ClientTests.swift:349

Issue: The test hardcodes a specific staging URL:

gatewayUrl: "https://payer.testnet-staging.xmtp.network:443"

Concern:

• If this URL changes, the test will break
• Other developers may not have access to this specific environment

Recommendation: Consider:

• Adding a comment explaining this is a specific test environment
• OR making this configurable via environment variables
• OR documenting required test infrastructure in CLAUDE.md or test documentation

5. libxmtp Version Discrepancy ℹ️

Location: Package.swift:27, XMTP.podspec:3

Observation:

• Package.swift still references: swift-bindings-1.6.0-dev.c046238
• XMTP.podspec references: 4.6.0-dev

Question: Is this intentional? The version schemes differ:

• Package.swift: 1.6.0-dev.c046238 (seems older)
• Podspec: 4.6.0-dev (seems newer)

Recommendation: Verify version alignment. If intentional, add a comment explaining the different versioning schemes for Swift bindings vs. the SDK itself.

---

🔍 Code Quality Observations

Positive Patterns:

1. Consistent API design: The MessageVisibilityOptions parameter is consistently added across all conversation types
2. Good defaults: Default of shouldPush: true maintains expected behavior
3. Actor-based concurrency: Proper use of Swift actors for thread-safe caching
4. Comprehensive logging: Good use of XMTPLogger for debugging (Client.swift:202-211)

Minor Style Issues:

1. Inconsistent whitespace: Some methods have inconsistent indentation (acceptable for generated code like xmtpv3.swift, but manual code should be consistent)
2. Long parameter lists: Some methods could benefit from parameter objects (e.g., ApiClientCache.makeCacheKey with 4 parameters)

---

🔒 Security Considerations

✅ No security concerns identified

• Encryption keys properly handled
• No secrets or credentials in code
• Gateway URLs are configuration, not secrets
• Actor isolation prevents race conditions in caching

---

⚡ Performance Considerations

✅ Performance improvements present

• Caching optimization: The improved cache key generation prevents unnecessary API client creation
• Potential issue: Cache grows unbounded - consider adding cache eviction policy if many different gateway/version combinations are used in practice

Recommendation: Consider adding cache size limits or LRU eviction if cache growth becomes an issue:

actor ApiClientCache {
    private var apiClientCache: [String: (client: XmtpApiClient, lastUsed: Date)] = [:]
    private let maxCacheSize = 10
    
    // Add eviction logic when cache exceeds maxCacheSize
}

---

✅ Test Coverage Assessment

Good:

• New test for D14N staging environment
• Existing comprehensive client tests maintained

Could be improved:

1. Missing tests for MessageVisibilityOptions: No tests verify that shouldPush: false actually prevents push notifications
2. Missing tests for cache key generation: No unit tests for ApiClientCache.makeCacheKey() edge cases (nil handling, special characters in URLs, etc.)
3. No integration tests: The new gateway URL support isn't tested end-to-end beyond the staging test

Recommendation: Add tests:

func testMessageVisibilityOptions() async throws {
    let opts = MessageVisibilityOptions(shouldPush: false)
    XCTAssertFalse(opts.shouldPush)
    XCTAssertEqual(opts.toFfi().shouldPush, false)
}

func testApiClientCacheKeyGeneration() {
    let key1 = ApiClientCache.makeCacheKey(
        v3Host: "host1", gatewayHost: nil, 
        isSecure: true, appVersion: nil
    )
    let key2 = ApiClientCache.makeCacheKey(
        v3Host: "host1", gatewayHost: "gateway1",
        isSecure: true, appVersion: nil
    )
    XCTAssertNotEqual(key1, key2)
}

---

📋 Action Items

High Priority:

1. ⚠️ Fill in PR description with Introduction, Purpose, and Scope
2. ⚠️ Remove unnecessary try-catch block in Group.swift:334-343
3. ⚠️ Verify and document version alignment between Package.swift and XMTP.podspec

Medium Priority:

4. 📝 Add tests for MessageVisibilityOptions functionality
5. 📝 Add tests for ApiClientCache.makeCacheKey() edge cases
6. 📝 Document the new feature in release notes or changelog

Low Priority:

7. 💡 Consider cache eviction policy for unbounded cache growth
8. 💡 Document or make configurable the hardcoded staging gateway URL in tests

---

✅ Verdict

Recommendation: APPROVE with minor changes

This PR introduces solid improvements to API client caching and adds a useful MessageVisibilityOptions API. The code quality is good, follows established patterns, and maintains backward compatibility. The main issues are:

• Missing PR description
• One unnecessary try-catch block
• Some missing test coverage

These are easily addressable and don't block the PR from being merged, but should be resolved before or shortly after merge.

---

[macroscopeapp]

Add prerelease gateway routing and message visibility controls across iOS client messaging and update binary target to swift-bindings-1.6.0-dev.c046238 with checksum c011e963334de61a3bf7d81164a127443c68486b3722912203a76ad2d0771d5f

This change introduces an optional ClientOptions.Api.gatewayUrl used in API client construction and caching, adds explicit FfiSendMessageOpts usage for message send paths with new MessageVisibilityOptions, regenerates FFI bindings to include updated send signatures and backend connection parameters, and bumps the CocoaPods spec for a prerelease. The package manifest is updated to consume a new prebuilt artifact with a specific version and checksum.

• Add ClientOptions.Api.gatewayUrl and cache keys that include v3Host, gatewayHost, isSecure, and appVersion via ApiClientCache.makeCacheKey, and update Client.connectToApiBackend/Client.connectToSyncApiBackend to pass v3Host and gatewayHost (Client.swift).
• Route MessageVisibilityOptions through Conversation.send/prepareMessage into DM and Group paths (Conversation.swift).
• Add visibility-aware send and prepare flows in Dm and Group, deriving shouldPush via codec and passing FfiSendMessageOpts to FFI send calls (Dm.swift, Group.swift).
• Regenerate xmtpv3 bindings to use FfiSendMessageOpts, updated connect signature (v3Host,gatewayHost,isSecure,appVersion), add FfiCursor, non-optional sequenceId, originatorId, and related changes (xmtpv3.swift).
• Introduce MessageVisibilityOptions with shouldPush and FFI conversion (MessageVisibilityOptions.swift).
• Update binary target URL and checksum in Package.swift and bump XMTP.podspec to 4.6.0-dev (XMTP.podspec).
• Add a test covering gateway URL configuration in client creation (ClientTests.swift).

📍Where to Start

Start with the updated client connection and caching flow in Client.connectToApiBackend and ApiClientCache in Client.swift, then review the FFI signature changes in xmtpv3.swift to see how FfiSendMessageOpts and (v3Host,gatewayHost,isSecure,appVersion) propagate through send and connect paths.

---

📊 Macroscope summarized 4a8ee5d. 6 files reviewed, 8 issues evaluated, 6 issues filtered, 0 comments posted

🗂️ Filtered Issues

Sources/XMTPiOS/Client.swift — 0 comments posted, 4 evaluated, 2 filtered

• line 113: Cache key construction in ApiClientCache.makeCacheKey can collide for legitimate inputs. It uses a '|' delimiter and substitutes "none" for missing optionals. If any of the inputs contain a '|' character (e.g., a host or version string) or if a real gateway host/app version is literally "none", different configurations can yield identical cache keys, causing incorrect reuse of an XmtpApiClient across distinct backends. Use an unambiguous encoding (e.g., JSON serialization of a struct, or escaping the delimiter) and avoid a sentinel like "none"—encode nil explicitly. [ Low confidence ]
• line 406: Ambiguous/misaligned connection configuration: both v3Host (which in this code is a full URL including scheme and port via api.env.url) and a separate isSecure boolean are passed into connectToBackend. If v3Host indicates http://… but isSecure is true (or vice versa), the two sources of truth can contradict, leading to failed connections or confusing behavior depending on which parameter the backend honors. Enforce consistency by deriving isSecure from the URL or validating them together before calling connectToBackend and surfacing a clear error if they mismatch. [ Low confidence ]

Sources/XMTPiOS/Dm.swift — 0 comments posted, 2 evaluated, 2 filtered

• line 159: Defaulting shouldPush to true when visibilityOptions is omitted in Dm.send(encodedContent:visibilityOptions:) can cause unintended push notifications and breaks contract parity with the generic send<T>(content:options:) path, which derives shouldPush from the codec via encodeContent. If a caller sends a pre-encoded EncodedContent without specifying visibilityOptions, the message will always push, even if the codec's shouldPush for that content type would be false. This creates divergent behavior between the encoded and generic content paths and can lead to externally visible side effects (push notifications) that the caller did not intend. [ Low confidence ]
• line 224: Defaulting shouldPush to true when visibilityOptions is omitted in Dm.prepareMessage(encodedContent:visibilityOptions:) can cause unintended push notifications and breaks parity with prepareMessage<T>(content:options:), which computes shouldPush from the codec via encodeContent. Sending a pre-encoded EncodedContent without visibilityOptions will always push, even if codec-derived shouldPush would be false. [ Low confidence ]

Sources/XMTPiOS/Group.swift — 0 comments posted, 2 evaluated, 2 filtered

• line 335: Defaulting shouldPush to true when visibilityOptions is omitted in Group.send(encodedContent:visibilityOptions:) can cause unintended push notifications and breaks parity with send<T>(content:options:), which computes shouldPush via encodeContent. A pre-encoded EncodedContent sent without visibilityOptions will always push, diverging from codec-derived behavior. [ Low confidence ]
• line 403: Defaulting shouldPush to true when visibilityOptions is omitted in Group.prepareMessage(encodedContent:visibilityOptions:) can cause unintended push notifications and breaks parity with prepareMessage<T>(content:options:), which computes shouldPush via encodeContent. Pre-encoded messages without visibilityOptions will always push, potentially surprising callers and diverging from codec-derived behavior. [ Low confidence ]