Claim sent messages per-row and log thread validation failures#102
Claim sent messages per-row and log thread validation failures#102pmanot wants to merge 1 commit into
Conversation
When Messages.app delivers a send under a different chat GUID than the one targeted (service or handle-formatting flip for the same contact), waitForMessageSend rejected the whole batch and resolved the send as a bare boolean(true). The client then had no message to reconcile its optimistic echo against, leaving it stuck on "Still sending…" indefinitely (DESK-28166). - Validate each sent row individually instead of allSatisfy over the batch, so one foreign or unjoined row doesn't discard the rest - Accept same-recipient addresses (email casing, phone with/without country prefix) before falling back to the contacts-database comparison, which returns false when the sent handle isn't on the contact card - Log hashed target/sent thread GUIDs and report to Sentry when validation still fails, so future rageshakes show which mismatch flavor occurred Fixes DESK-28166
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChanges sent-message/thread validation in ChangesSent Thread Validation Filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlatformAPI
participant validSentMessageIDs
participant Database
Client->>PlatformAPI: waitForMessageSend(sentRows, targetThread)
PlatformAPI->>Database: resolve sentThreadIDs
PlatformAPI->>validSentMessageIDs: filter(rows, sentThreadIDs, targetThread, isSameContact)
validSentMessageIDs-->>PlatformAPI: valid subset
alt count mismatch
PlatformAPI->>PlatformAPI: log hashed thread ids, reportErrorMessage
end
alt valid subset empty
PlatformAPI-->>Client: .boolean(true)
else
PlatformAPI->>Database: fetch messages for valid subset
PlatformAPI->>PlatformAPI: validateLinkedMessageIDs
PlatformAPI-->>Client: .messages
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
src/IMessage/Sources/IMessage/PlatformAPI.swift (1)
748-755: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winError report fires on the benign cases per-message filtering is meant to tolerate.
reportErrorMessageis called whenevervalidSentMessageIDs.count < sentMessageIDs.count, which includes a dropped foreign row from a concurrent send on another device, and rows that were still unjoined whenwaitForSentThreadIDstimed out. Those are exactly the scenarios this per-message filtering was introduced to handle gracefully, so reporting them as "validation failed" may generate misleading Sentry noise even when the targeted send succeeded.If the intent is to surface only genuine misdeliveries, consider gating the report on
validSentMessageIDs.isEmpty(or otherwise distinguishing "no valid row for the target" from "some foreign/unjoined rows dropped"). If the noisy report is intentional telemetry for tracking DESK-28166 frequency, this can be left as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/IMessage/Sources/IMessage/PlatformAPI.swift` around lines 748 - 755, `PlatformAPI` is reporting an error for any partial mismatch between `sentMessageIDs` and `validSentMessageIDs`, which includes benign filtered-out foreign or unjoined rows. Update the logic around the `validSentMessageIDs.count < sentMessageIDs.count` check so `reportErrorMessage` only fires for genuine misdelivery cases, such as when `validSentMessageIDs.isEmpty` or when the target thread itself has no valid row; keep the existing `platformLog.error` if you still want telemetry, but make the `reportErrorMessage` call in `PlatformAPI.swift` distinguish benign per-message filtering from actual validation failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/IMessage/Sources/IMessageTests/SentThreadValidationTests.swift`:
- Line 4: SwiftLint is flagging the test declarations because the `@Test`
attribute is attached to the function signature on the same line. Update each
affected test in SentThreadValidationTests by moving `@Test` onto its own line
directly above the corresponding func declaration, including
addressesReferToSameRecipientMatchesFormattingVariants and the other flagged
test methods, so the attributes rule is satisfied.
---
Nitpick comments:
In `@src/IMessage/Sources/IMessage/PlatformAPI.swift`:
- Around line 748-755: `PlatformAPI` is reporting an error for any partial
mismatch between `sentMessageIDs` and `validSentMessageIDs`, which includes
benign filtered-out foreign or unjoined rows. Update the logic around the
`validSentMessageIDs.count < sentMessageIDs.count` check so `reportErrorMessage`
only fires for genuine misdelivery cases, such as when
`validSentMessageIDs.isEmpty` or when the target thread itself has no valid row;
keep the existing `platformLog.error` if you still want telemetry, but make the
`reportErrorMessage` call in `PlatformAPI.swift` distinguish benign per-message
filtering from actual validation failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2acf7f18-a273-4501-8ea5-6611786799ce
📒 Files selected for processing (3)
src/IMessage/Sources/IMessage/PlatformAPI.swiftsrc/IMessage/Sources/IMessage/Utilities/ThreadID.swiftsrc/IMessage/Sources/IMessageTests/SentThreadValidationTests.swift
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-03T17:00:19.662Z
Learnt from: KishanBagaria
Repo: beeper/platform-imessage PR: 69
File: src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift:89-93
Timestamp: 2026-05-03T17:00:19.662Z
Learning: In the beeper/platform-imessage Swift codebase, keep message IDs (`PlatformSDK.MessageID`) as raw GUIDs. When mapping from DB/event rows to `message.id`, set the ID directly from `msgRow.guid` (no GUID→public-ID hashing or transformation). For multi-part messages, append the part index as `_<part.index>` to the GUID-derived ID. During code review, if changes touch message ID creation/mapping, ensure this raw GUID + optional `_<part.index>` suffix behavior is preserved.
Applied to files:
src/IMessage/Sources/IMessageTests/SentThreadValidationTests.swiftsrc/IMessage/Sources/IMessage/Utilities/ThreadID.swiftsrc/IMessage/Sources/IMessage/PlatformAPI.swift
🪛 SwiftLint (0.65.0)
src/IMessage/Sources/IMessageTests/SentThreadValidationTests.swift
[Warning] 4-4: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 11-11: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 25-25: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 35-35: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 48-48: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 58-58: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 69-69: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Warning] 79-79: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
🔇 Additional comments (2)
src/IMessage/Sources/IMessage/Utilities/ThreadID.swift (1)
26-44: LGTM!src/IMessage/Sources/IMessage/PlatformAPI.swift (1)
766-785: LGTM!
| @testable import IMessage | ||
| import Testing | ||
|
|
||
| @Test func addressesReferToSameRecipientMatchesFormattingVariants() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move @Test onto its own line to clear the SwiftLint attributes warnings.
SwiftLint flags all eight test declarations because the attributes rule requires attributes to be on their own line for functions. Placing @Test on the line above each func resolves the warnings.
Example fix (apply to each flagged declaration)
-@Test func addressesReferToSameRecipientMatchesFormattingVariants() {
+@Test
+func addressesReferToSameRecipientMatchesFormattingVariants() {Also applies to: 11-11, 25-25, 35-35, 48-48, 58-58, 69-69, 79-79
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 4-4: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/IMessage/Sources/IMessageTests/SentThreadValidationTests.swift` at line
4, SwiftLint is flagging the test declarations because the `@Test` attribute is
attached to the function signature on the same line. Update each affected test
in SentThreadValidationTests by moving `@Test` onto its own line directly above
the corresponding func declaration, including
addressesReferToSameRecipientMatchesFormattingVariants and the other flagged
test methods, so the attributes rule is satisfied.
Source: Linters/SAST tools
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| let digitsA = a.filter(\.isNumber) | ||
| let digitsB = b.filter(\.isNumber) | ||
| guard min(digitsA.count, digitsB.count) >= minimumPhoneSuffixDigits else { return false } | ||
| return digitsA.hasSuffix(digitsB) || digitsB.hasSuffix(digitsA) |
There was a problem hiding this comment.
Latent: digit-only comparison can alias group chat GUIDs with 1-1 phone numbers
threadIDToAddress returns chat<digits> for group threads (iMessage;+;chat123…) and a phone/email for 1-1s. Because this helper strips non-digits before the suffix check, a group's chat15035550123 and the 1-1 address +15035550123 produce identical digit strings and match. The earlier sentThreadID == targetThreadID short-circuit in validSentMessageIDs won't fire because the full thread IDs differ (+ vs - type token), so we'd fall through to this helper and reconcile a foreign group row as ours (or vice versa).
Real Messages chat GUIDs are long and effectively random, so the practical collision odds are tiny, but the helper accepts any string pair and doesn't distinguish thread type. If you want to defensively gate it, consider bailing when the input looks like a group chat GUID (e.g., hasPrefix("chat")) or comparing thread types before calling into this helper.
| .map { $0.map { Hasher.thread.tokenizeRemembering(pii: $0) } ?? "<unjoined>" } | ||
| .joined(separator: " ") | ||
| platformLog.error("imsg: imessage potentially sent messages to invalid thread (target=\(hashedTarget) sent=\(hashedSent))") | ||
| reportErrorMessage("imessage sent message thread validation failed for \(sentMessageIDs.count - validSentMessageIDs.count)/\(sentMessageIDs.count) messages") |
There was a problem hiding this comment.
Nit: Sentry will fire for legitimate concurrent multi-device sends
waitForSentMessageIDs matches by text and rowID > lastRowID, so a second Apple device sending the same body around the same time will produce a foreign row that we now correctly drop — but this branch will still reportErrorMessage a validation failed count to Sentry.
Per the commit message this reporting is intentional ("so future rageshakes show which mismatch flavor occurred"). Just calling it out in case the Sentry volume from multi-device users turns out noisier than expected — you may want to keep the hashed log line but demote the Sentry report to a rarer trigger (e.g., only when no row validated, or throttled).
1 participant
When Messages.app delivers a send under a different chat GUID than the one targeted (service or handle-formatting flip for the same contact), waitForMessageSend rejected the whole batch and resolved the send as a bare boolean(true). The client then had no message to reconcile its optimistic echo against, leaving it stuck on "Still sending…" indefinitely (DESK-28166).
partial fix for DESK-28166