diff --git a/RidestrSDK/Sources/RidestrSDK/RoadFlare/ChatMessageStore.swift b/RidestrSDK/Sources/RidestrSDK/RoadFlare/ChatMessageStore.swift index d989ae6..7bf957c 100644 --- a/RidestrSDK/Sources/RidestrSDK/RoadFlare/ChatMessageStore.swift +++ b/RidestrSDK/Sources/RidestrSDK/RoadFlare/ChatMessageStore.swift @@ -22,13 +22,13 @@ public struct ChatMessage: Hashable, Sendable { /// Result of appending a message to a `ChatMessageStore`. /// -/// `.duplicate` is returned when a message with the same `id` is already -/// present; the store is unchanged. `.inserted` is returned when the message -/// is accepted; `incrementedUnread` is `true` iff the message was remote -/// (`!isMine`) and its timestamp was at or after the current unread cutoff. +/// `.duplicate` — a message with the same `id` is already present and the +/// store is unchanged. `.inserted` — the message was accepted. Callers +/// that need to know whether the append also incremented `unreadCount` +/// observe `unreadCount` directly. public enum ChatMessageAppendOutcome: Equatable, Sendable { case duplicate - case inserted(incrementedUnread: Bool) + case inserted } /// Platform-neutral in-memory store for an in-ride chat thread. @@ -97,11 +97,10 @@ public final class ChatMessageStore: @unchecked Sendable { let removed = messages.removeFirst() messageIds.remove(removed.id) } - let incrementedUnread = !message.isMine && message.timestamp >= unreadCutoff - if incrementedUnread { + if !message.isMine && message.timestamp >= unreadCutoff { unreadCount += 1 } - return .inserted(incrementedUnread: incrementedUnread) + return .inserted } } diff --git a/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ChatMessageStoreTests.swift b/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ChatMessageStoreTests.swift index 9e8f439..7fab0f6 100644 --- a/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ChatMessageStoreTests.swift +++ b/RidestrSDK/Tests/RidestrSDKTests/RoadFlare/ChatMessageStoreTests.swift @@ -35,7 +35,7 @@ struct ChatMessageStoreTests { @Test func appendInsertsNewMessage() { let store = makeStore() let outcome = store.append(message(id: "a", timestamp: 100)) - #expect(outcome == .inserted(incrementedUnread: true)) + #expect(outcome == .inserted) #expect(store.messages.count == 1) #expect(store.messages[0].id == "a") } @@ -120,7 +120,7 @@ struct ChatMessageStoreTests { _ = store.append(message(id: "b", isMine: true, timestamp: 200)) _ = store.append(message(id: "c", isMine: true, timestamp: 300)) let outcome = store.append(message(id: "a", isMine: false, timestamp: 100)) - #expect(outcome == .inserted(incrementedUnread: true)) + #expect(outcome == .inserted) #expect(store.messages.map(\.id) == ["b", "c"]) #expect(store.unreadCount == 1) } @@ -130,7 +130,7 @@ struct ChatMessageStoreTests { @Test func unreadDoesNotIncrementForOwnMessages() { let store = makeStore() let outcome = store.append(message(id: "a", isMine: true, timestamp: 100)) - #expect(outcome == .inserted(incrementedUnread: false)) + #expect(outcome == .inserted) #expect(store.unreadCount == 0) } @@ -145,9 +145,10 @@ struct ChatMessageStoreTests { let store = makeStore() store.setUnreadCutoff(500) let stale = store.append(message(id: "old", isMine: false, timestamp: 400)) + #expect(stale == .inserted) + #expect(store.unreadCount == 0) let live = store.append(message(id: "new", isMine: false, timestamp: 600)) - #expect(stale == .inserted(incrementedUnread: false)) - #expect(live == .inserted(incrementedUnread: true)) + #expect(live == .inserted) #expect(store.unreadCount == 1) } @@ -156,7 +157,7 @@ struct ChatMessageStoreTests { let store = makeStore() store.setUnreadCutoff(500) let atBoundary = store.append(message(id: "boundary", isMine: false, timestamp: 500)) - #expect(atBoundary == .inserted(incrementedUnread: true)) + #expect(atBoundary == .inserted) #expect(store.unreadCount == 1) } @@ -196,7 +197,7 @@ struct ChatMessageStoreTests { #expect(store.unreadCount == 0) // Cutoff preserved: a message at timestamp 400 should NOT increment unread. let outcome = store.append(message(id: "b", isMine: false, timestamp: 400)) - #expect(outcome == .inserted(incrementedUnread: false)) + #expect(outcome == .inserted) #expect(store.unreadCount == 0) } diff --git a/decisions/0020-chat-message-store.md b/decisions/0020-chat-message-store.md index f2e7553..b1d4e52 100644 --- a/decisions/0020-chat-message-store.md +++ b/decisions/0020-chat-message-store.md @@ -43,7 +43,7 @@ public struct ChatMessage: Hashable, Sendable { public enum ChatMessageAppendOutcome: Equatable, Sendable { case duplicate - case inserted(incrementedUnread: Bool) + case inserted } @Observable @@ -68,7 +68,9 @@ Specific decisions captured: 3. **Sort:** ascending `timestamp`, tiebreak ascending `id` — deterministic. 4. **Capacity:** init parameter with default `500`. FIFO eviction (drop `removeFirst()` after sort). -5. **Append API:** returns `enum AppendOutcome { duplicate, inserted(incrementedUnread:) }`. +5. **Append API:** returns `enum AppendOutcome { duplicate, inserted }`. Callers + that need to know whether the append also bumped `unreadCount` observe the + counter directly rather than receiving it as a return-value side channel. 6. **Unread cutoff:** caller-supplied via `setUnreadCutoff(_:)`, defaults to `0`, inclusive check (`timestamp >= cutoff`). 7. **`reset()` semantics:** clears messages, message-id set, and @@ -114,11 +116,18 @@ costs essentially nothing, lets tests exercise the boundary with small capacities (`capacity: 2` and `capacity: 5` in the test suite), and gives future consumers a config knob without forcing a refactor. -**Append outcome enum.** `Bool` would have told callers *whether* a message -was inserted but not whether it bumped the unread counter — and the -coordinator's haptic decision needs to know both. The enum bundles the two -signals atomically without polling the counter or recomputing the cutoff -check externally. +**Append outcome enum.** The enum's named cases (`.duplicate`, `.inserted`) +read more clearly at call sites than a `Bool` would — the haptic decision +in `ChatCoordinator.handleChatEvent` becomes a self-describing +`if case .inserted = store.append(message), !message.isMine`. An earlier +revision attached an `incrementedUnread: Bool` payload to `.inserted`, +motivated by a hypothetical caller that needed to know whether the badge +just bumped (e.g. for a "fresh-unread pulse" UI). The actual coordinator +never read the payload — it only checks the case — and every test that +exercised the unread side effect also asserts `store.unreadCount` directly, +which is the canonical source. The payload was therefore dead public surface +and was dropped; if a future consumer genuinely needs a fresh-unread signal +it can be added back as an associated value at that time. **Default cutoff of 0.** This makes "no cutoff set" equivalent to "every remote message is unread," which is the safe default for a fresh store @@ -178,10 +187,20 @@ gain and the driver coordinator will not call this type at all. test-seam value (running boundary tests with `capacity: 2` instead of pushing 501 messages) was outsized. -- **`@discardableResult func append(_:) -> Bool`.** Simpler. Rejected - because the coordinator's haptic decision needs to know whether the - message also bumped unread; `Bool` would have forced callers to poll - `unreadCount` deltas, which is brittle. +- **`@discardableResult func append(_:) -> Bool`.** Strictly equivalent + to the chosen `enum { duplicate, inserted }`. Rejected on readability — + `if case .inserted = store.append(msg)` at the call site documents what + the value means; `if store.append(msg)` requires the reader to remember + which boolean polarity means "inserted." + +- **Bundle unread-bump info into the return value** (e.g. `.inserted(incrementedUnread:)`). + An earlier revision did this. Dropped because the only production + caller — `ChatCoordinator.handleChatEvent` — never read the payload; + it branches solely on `.inserted` vs `.duplicate` for haptics. Tests + that check the unread side effect assert `store.unreadCount` directly, + which is the canonical source. If a future consumer genuinely needs + a "fresh-unread" signal it can be added back as an associated value + at that point. - **Construct with cutoff in init, no `setUnreadCutoff(_:)`.** Removes the default-0 footgun. Rejected because the subscription-managing caller