From 8a64f6bc81a0f9709576ed23ea06a547b2a86017 Mon Sep 17 00:00:00 2001 From: variablefate Date: Mon, 11 May 2026 20:54:31 -0700 Subject: [PATCH 1/2] refactor(sdk): drop unused incrementedUnread payload from ChatMessageAppendOutcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The .inserted associated value was added to support a haptic-decision use case that turned out not to need it. ChatCoordinator.handleChatEvent only ever pattern-matches on `case .inserted` for the haptic — it never reads the payload. Tests that exercised the unread side effect also assert store.unreadCount directly (the canonical source). Per the project's YAGNI guidance ("Don't design for hypothetical future requirements"), drop the payload. The enum becomes: case duplicate case inserted If a future consumer genuinely needs a fresh-unread signal it can be added back as an associated value at that time. Updated ADR-0020 rationale and alternatives-considered section to reflect the actual motivation for the enum shape (readable named cases at call sites vs Bool) and the reason for dropping the payload. Tests rewritten: every `.inserted(incrementedUnread: ...)` simplified to `.inserted`, with explicit `store.unreadCount` assertions added where the payload was the sole carrier of the unread-side-effect check (specifically `unreadCutoffSuppressesReplayedHistory`). All 21 ChatMessageStore tests pass; full xcodebuild RoadFlareTests scheme passes. --- .../RoadFlare/ChatMessageStore.swift | 15 ++++--- .../RoadFlare/ChatMessageStoreTests.swift | 15 +++---- decisions/0020-chat-message-store.md | 41 ++++++++++++++----- 3 files changed, 45 insertions(+), 26 deletions(-) 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..e976ba0 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. Per the project's YAGNI guidance the +payload 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 From b36fd8b1b5271304a994135c8a86f4b60de18c9b Mon Sep 17 00:00:00 2001 From: variablefate Date: Mon, 11 May 2026 20:56:04 -0700 Subject: [PATCH 2/2] review: drop misleading "project YAGNI guidance" citation from ADR-0020 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The phrase implied .claude/CLAUDE.md contains a YAGNI principle, but it does not — its Project Conventions section covers SDK/app split, build verification, and concurrency only. Reword the justification to stand on its own ("the payload was dead public surface") so a future reader chasing the citation does not come up empty. --- decisions/0020-chat-message-store.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/decisions/0020-chat-message-store.md b/decisions/0020-chat-message-store.md index e976ba0..b1d4e52 100644 --- a/decisions/0020-chat-message-store.md +++ b/decisions/0020-chat-message-store.md @@ -125,9 +125,9 @@ 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. Per the project's YAGNI guidance the -payload was dropped; if a future consumer genuinely needs a fresh-unread -signal it can be added back as an associated value at that time. +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