From 9a0b789f4afe8ecee189cf47eed65344ab8405c8 Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Tue, 15 Sep 2026 04:32:50 -0700 Subject: [PATCH] fix(notifications): read the tray with scope=all so read notifications stay in it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scope=tray` returns unread rows only and ignores `limit`, so the macOS tray emptied as the user read it while the web bell retained recent history. Same feature, different behaviour — and the macOS one was the surprising one: a notification you glanced at vanished, with no way back to it from the tray. The issue asked for a probe before concluding anything, and the probe found the cheap answer. A read-inclusive scope already exists and honours the limit: ?scope=tray&limit=5 -> 1 item (the single unread one) ?scope=all&limit=3 -> 3 items ?scope=all&limit=10 -> 10 items ?scope=all -> 20 items (the account's notificationTrayLimit) `unreadCount` is returned under both scopes, so the badge is untouched. No backend ask is needed. The client-side `prefix(limit)` goes with it. It existed only because the tray scope ignored the parameter, and it made `notificationTrayLimit` appear to work while the rows it trimmed were unread-only — so the preference could only ever shrink an already-wrong list. It is deliberately not re-added as a belt-and- braces cap: a client-side trim over a correctly-limited response is invisible when it agrees with the server and wrong when it does not. Nothing here fetches a wider page and filters locally, which is the workaround the issue ruled out and the failure mode PR #69 fixed for the DM inbox. Refs #80 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016gSWb3scYobtxLJioV1qF9 --- .../Services/NotificationsService.swift | 58 ++++++++++++------- .../NotificationsServiceTests.swift | 6 +- .../ViewPreferencesWiringTests.swift | 53 ++++++++++++++--- .../Endpoints/NotificationsEndpoint.swift | 34 +++++++---- .../NotificationsEndpointTests.swift | 48 ++++++++++++++- 5 files changed, 152 insertions(+), 47 deletions(-) diff --git a/Packages/InterlinedDomain/Sources/InterlinedDomain/Services/NotificationsService.swift b/Packages/InterlinedDomain/Sources/InterlinedDomain/Services/NotificationsService.swift index 3629552..5e05ffa 100644 --- a/Packages/InterlinedDomain/Sources/InterlinedDomain/Services/NotificationsService.swift +++ b/Packages/InterlinedDomain/Sources/InterlinedDomain/Services/NotificationsService.swift @@ -10,14 +10,28 @@ import InterlinedKit /// encodes it for us). The user-side `notificationTrayLimit` (10–40, default /// 20) controls the page size. /// -/// **Tray limit — corrected 2026-09-09 (G35 / issue #43).** The previous note -/// here claimed the server ignores a client `limit` outright. A live probe -/// disproves that: unscoped, `?limit=5` returns 5 rows and `?limit=40` returns -/// 36, and omitting it returns exactly the account's stored limit. Only the -/// `scope=tray` form ignores it. So `tray(limit:)` both sends the value *and* -/// caps the decoded rows client-side — the send is what will start working the -/// moment the tray route honours it, and the cap is what makes the preference -/// observable today. +/// **Scope — corrected 2026-09-15 (GitHub #80).** The tray reads `scope=all`, +/// not `scope=tray`. +/// +/// `scope=tray` returns **unread rows only** and ignores `limit`, so the macOS +/// tray emptied as the user read it while the web bell retained recent history. +/// A glanced-at notification simply vanished, with no way back to it. `scope=all` +/// is what the web bell uses — the last N regardless of read state — and it +/// honours `limit`, falling back to the account's stored `notificationTrayLimit` +/// when the parameter is absent. Probed live: +/// +/// ``` +/// ?scope=tray&limit=5 → 1 item (the single unread one) +/// ?scope=all&limit=3 → 3 items +/// ?scope=all&limit=10 → 10 items +/// ?scope=all → 20 items (the stored preference) +/// ``` +/// +/// `unreadCount` is returned under both scopes, so the badge is unaffected. +/// +/// The client-side `prefix` that used to cap the rows is gone with it: it existed +/// only because `scope=tray` ignored the parameter, and it made the preference +/// *appear* to work while the rows being trimmed were the wrong rows. /// /// Follows the same DI shape as the other domain services — takes its /// `APIClientProtocol` as a parameter so unit tests run against a stub. @@ -28,10 +42,10 @@ public protocol NotificationsServicing: Sendable { /// `unreadCount` + the items page). /// /// - Parameter limit: the account's `notificationTrayLimit`. `nil` (the - /// default, via the `tray()` convenience) leaves the page size entirely - /// to the server. `unreadCount` is never capped — it is the server's own - /// total and stays authoritative for the badge even when the rendered - /// rows are trimmed. + /// default, via the `tray()` convenience) lets the server apply the stored + /// preference itself. `unreadCount` is always the server's own total and + /// stays authoritative for the badge regardless of how many rows come + /// back — a limit that shrinks the list must not shrink the badge. func tray(limit: Int?) async throws -> NotificationTray /// Marks a single notification read by id. The service does not return @@ -69,17 +83,17 @@ public final class NotificationsService: NotificationsServicing { } public func tray(limit: Int?) async throws -> NotificationTray { + // `scope=all` honours `limit` server-side, so the client-side `prefix` + // that used to sit here is gone (GitHub #80). That trim existed because + // `scope=tray` ignored the parameter — it made `notificationTrayLimit` + // *appear* to work while the rows being trimmed were unread-only, so the + // preference could only ever shrink an already-wrong list. + // + // Deliberately not re-added as a belt-and-braces cap: a client-side trim + // over a correctly-limited response is invisible when it agrees with the + // server and wrong when it does not. let dto = try await api.send(Notifications.tray(limit: limit)) - let tray = NotificationTray(from: dto) - // Client-side cap: `scope=tray` ignores the query parameter today, so - // without this the user's limit would have no visible effect. The - // server-authoritative `unreadCount` is preserved as-is — trimming the - // rendered rows must not understate the badge. - guard let limit, limit > 0, tray.items.count > limit else { return tray } - return NotificationTray( - unreadCount: tray.unreadCount, - items: Array(tray.items.prefix(limit)) - ) + return NotificationTray(from: dto) } public func markRead(id: String) async throws { diff --git a/Packages/InterlinedDomain/Tests/InterlinedDomainTests/NotificationsServiceTests.swift b/Packages/InterlinedDomain/Tests/InterlinedDomainTests/NotificationsServiceTests.swift index 42dd827..c0131b0 100644 --- a/Packages/InterlinedDomain/Tests/InterlinedDomainTests/NotificationsServiceTests.swift +++ b/Packages/InterlinedDomain/Tests/InterlinedDomainTests/NotificationsServiceTests.swift @@ -43,10 +43,12 @@ final class NotificationsServiceTests: XCTestCase { // The second row is marked read (readAt present). XCTAssertTrue(tray.items.last?.isRead ?? false) - // And — the request shape carries `scope=tray`. + // And — the request asks for `scope=all`, the read-inclusive scope the + // web bell uses. `scope=tray` is unread-only, which made the macOS tray + // empty itself as the user read it (GitHub #80). let recorded = await api.recorded XCTAssertEqual(recorded.first?.path, "/api/notifications") - XCTAssertEqual(recorded.first?.query["scope"], "tray") + XCTAssertEqual(recorded.first?.query["scope"], "all") } func test_givenMalformedTray_whenLoadingTray_thenThrowsDecoding() async throws { diff --git a/Packages/InterlinedDomain/Tests/InterlinedDomainTests/ViewPreferencesWiringTests.swift b/Packages/InterlinedDomain/Tests/InterlinedDomainTests/ViewPreferencesWiringTests.swift index 378ca9b..07478af 100644 --- a/Packages/InterlinedDomain/Tests/InterlinedDomainTests/ViewPreferencesWiringTests.swift +++ b/Packages/InterlinedDomain/Tests/InterlinedDomainTests/ViewPreferencesWiringTests.swift @@ -61,30 +61,67 @@ final class ViewPreferencesWiringTests: XCTestCase { // MARK: - Notification tray limit - func test_givenTrayLimit_whenLoadingTray_thenSendsItAndTrimsTheRenderedRows() async throws { - // Given — the server returns more rows than the account's limit. Under - // `scope=tray` it ignores the query parameter (probed 2026-09-09), so - // the client-side cap is what makes the preference observable. + func test_givenTrayLimit_whenLoadingTray_thenSendsItUnderTheReadInclusiveScope() async throws { + // Given — the account's `notificationTrayLimit`. Under `scope=all` the + // server honours it, so the limit travels and the rows come back already + // sized (probed 2026-09-15: `?scope=all&limit=3` → 3, `&limit=10` → 10). let api = StubAPIClient() - let items = (1...5).map { Fixtures.notificationObject(id: "n-\($0)") } + let items = (1...2).map { Fixtures.notificationObject(id: "n-\($0)") } await api.enqueue(json: Fixtures.notificationTrayEnvelope(unreadCount: 4, items: items)) let service = NotificationsService(api: api) // When let tray = try await service.tray(limit: 2) - // Then — trimmed rows, untouched unread count, and the limit on the wire. + // Then — the server's page is rendered as-is. XCTAssertEqual(tray.items.map(\.id), ["n-1", "n-2"]) XCTAssertEqual( tray.unreadCount, 4, - "The server-authoritative unread badge must not be understated by trimming rows" + "The server-authoritative unread badge is independent of how many rows are shown" ) let recorded = await api.recorded - XCTAssertEqual(recorded.first?.query["scope"], "tray") + XCTAssertEqual(recorded.first?.query["scope"], "all") XCTAssertEqual(recorded.first?.query["limit"], "2") } + func test_givenTheServerOverAnswers_whenLoadingTray_thenRowsAreNotTrimmedClientSide() async throws { + // Upstream-behaviour case, and a deliberate choice worth pinning. + // + // The old implementation capped the decoded rows with `prefix(limit)`. + // That existed only because `scope=tray` ignored the parameter, and it + // made the preference *appear* to work while the rows being trimmed were + // unread-only — so it could only ever shrink an already-wrong list. + // + // With `scope=all` the server does the limiting. A belt-and-braces trim + // on top would be invisible when it agrees with the server and wrong + // when it does not, so the client renders what it is given. + let api = StubAPIClient() + let items = (1...5).map { Fixtures.notificationObject(id: "n-\($0)") } + await api.enqueue(json: Fixtures.notificationTrayEnvelope(unreadCount: 4, items: items)) + let service = NotificationsService(api: api) + + let tray = try await service.tray(limit: 2) + + XCTAssertEqual(tray.items.count, 5, "the server's answer is rendered, not second-guessed") + } + + func test_givenAllRowsRead_whenLoadingTray_thenTheTrayStillShowsThem() async throws { + // The behaviour this whole change is about. On `scope=tray` a fully-read + // account got an empty tray — a notification you glanced at vanished, + // with no way back to it. The web bell keeps the last N. + let api = StubAPIClient() + let items = (1...3).map { Fixtures.notificationObject(id: "n-\($0)", readAt: "2026-09-14T04:00:00.000Z") } + await api.enqueue(json: Fixtures.notificationTrayEnvelope(unreadCount: 0, items: items)) + let service = NotificationsService(api: api) + + let tray = try await service.tray(limit: 20) + + XCTAssertEqual(tray.items.count, 3, "read notifications stay in the tray") + XCTAssertTrue(tray.items.allSatisfy(\.isRead)) + XCTAssertEqual(tray.unreadCount, 0, "and the badge correctly shows nothing unread") + } + func test_givenNoTrayLimit_whenLoadingTray_thenOmitsTheParameterAndKeepsEveryRow() async throws { // Boundary: `nil` leaves the page size entirely to the server, which // applies the account's own `notificationTrayLimit`. diff --git a/Packages/InterlinedKit/Sources/InterlinedKit/Endpoints/NotificationsEndpoint.swift b/Packages/InterlinedKit/Sources/InterlinedKit/Endpoints/NotificationsEndpoint.swift index 1aab7e7..fec72b3 100644 --- a/Packages/InterlinedKit/Sources/InterlinedKit/Endpoints/NotificationsEndpoint.swift +++ b/Packages/InterlinedKit/Sources/InterlinedKit/Endpoints/NotificationsEndpoint.swift @@ -13,21 +13,31 @@ import Foundation /// `GET /api/notifications` does **not** use the standard /// `{ data, pagination }` envelope; it returns /// `{ unreadCount, items: [...] }`, so it decodes into `NotificationTrayDTO` -/// rather than `Paginated`. The `scope=tray` query parameter is required by -/// the API and defaulted here. +/// rather than `Paginated`. public enum Notifications { - /// `GET /api/notifications` (with `scope=tray`). + /// `GET /api/notifications` — the tray, read **and** unread. /// - /// `limit` is optional and omitted by default. VERIFIED live 2026-09-09 - /// (G35 / issue #43): the route honours `limit` in its *unscoped* form - /// (`?limit=5` → 5 rows, `?limit=40` → 36 rows on the test account) and - /// falls back to the account's `notificationTrayLimit` when absent - /// (no params → exactly 20 rows, matching the stored preference). Under - /// `scope=tray` the server currently ignores `limit` and returns unread - /// rows only, so the preference is *also* enforced client-side by - /// `NotificationsService.tray(limit:)` — see the note there. - public static func tray(scope: String = "tray", limit: Int? = nil) -> Request { + /// The scope defaults to **`all`**, not `tray`. VERIFIED live 2026-09-15 + /// (GitHub #80) on the test account: + /// + /// ``` + /// ?scope=tray → 1 item (the single unread one), `limit` ignored + /// ?scope=tray&limit=5 → 1 item (still just the unread one) + /// ?scope=all&limit=3 → 3 items (read and unread, newest first) + /// ?scope=all&limit=10 → 10 items + /// ?scope=all → 20 items (the account's `notificationTrayLimit`) + /// ``` + /// + /// `scope=tray` is **unread-only** and ignores `limit`, so on macOS the tray + /// emptied as you read it while the web bell retained recent history. Same + /// feature, different behaviour — and the macOS one was the surprising one. + /// + /// `scope=all` is what the web bell uses: the last N regardless of read + /// state, honouring `limit`, and falling back to the account preference when + /// it is absent. `unreadCount` is returned under both scopes, so the badge + /// still comes from the same call. + public static func tray(scope: String = "all", limit: Int? = nil) -> Request { Request( method: .get, path: "/api/notifications", diff --git a/Packages/InterlinedKit/Tests/InterlinedKitTests/NotificationsEndpointTests.swift b/Packages/InterlinedKit/Tests/InterlinedKitTests/NotificationsEndpointTests.swift index a0c395e..b85a0ec 100644 --- a/Packages/InterlinedKit/Tests/InterlinedKitTests/NotificationsEndpointTests.swift +++ b/Packages/InterlinedKit/Tests/InterlinedKitTests/NotificationsEndpointTests.swift @@ -25,7 +25,9 @@ final class NotificationsEndpointTests: XCTestCase { XCTAssertEqual(Notifications.tray().path, "/api/notifications") XCTAssertEqual(Notifications.tray().method, .get) XCTAssertEqual(Notifications.tray().auth, .bearer) - XCTAssertEqual(Notifications.tray().query.first(where: { $0.name == "scope" })?.value, "tray") + // `all`, not `tray`: `scope=tray` is unread-only and ignores `limit`, + // so the macOS tray emptied as the user read it (GitHub #80). + XCTAssertEqual(Notifications.tray().query.first(where: { $0.name == "scope" })?.value, "all") XCTAssertEqual(Notifications.markRead(id: "n1").method, .patch) XCTAssertEqual(Notifications.markRead(id: "n1").path, "/api/notifications/n1/read") @@ -40,7 +42,7 @@ final class NotificationsEndpointTests: XCTestCase { // Happy path: the account's `notificationTrayLimit` reaches the wire. let request = Notifications.tray(limit: 25) - XCTAssertEqual(request.query.first(where: { $0.name == "scope" })?.value, "tray") + XCTAssertEqual(request.query.first(where: { $0.name == "scope" })?.value, "all") XCTAssertEqual(request.query.first(where: { $0.name == "limit" })?.value, "25") } @@ -73,7 +75,7 @@ final class NotificationsEndpointTests: XCTestCase { let received = await transport.received let comps = URLComponents(url: try XCTUnwrap(received[0].url), resolvingAgainstBaseURL: false) - XCTAssertTrue(comps?.queryItems?.contains(URLQueryItem(name: "scope", value: "tray")) ?? false) + XCTAssertTrue(comps?.queryItems?.contains(URLQueryItem(name: "scope", value: "all")) ?? false) } func test_givenOkBody_whenMarkReadSent_thenDecodesOk() async throws { @@ -137,3 +139,43 @@ final class NotificationsEndpointTests: XCTestCase { } } } + +// MARK: - Scope (GitHub #80) + +extension NotificationsEndpointTests { + + /// The read-inclusive scope is the default, and the unread-only one is still + /// reachable for anything that genuinely wants it. + /// + /// Probed live 2026-09-15 on the test account: + /// + /// ?scope=tray&limit=5 -> 1 item (the single unread one) + /// ?scope=all&limit=3 -> 3 items + /// ?scope=all&limit=10 -> 10 items + /// ?scope=all -> 20 items (the account's notificationTrayLimit) + func test_givenTheDefaultScope_whenBuildingTheTrayRequest_thenItIsReadInclusive() { + XCTAssertEqual( + Notifications.tray().query.first(where: { $0.name == "scope" })?.value, + "all" + ) + XCTAssertEqual( + Notifications.tray(scope: "tray").query.first(where: { $0.name == "scope" })?.value, + "tray", + "the unread-only scope stays available; it is just not the tray's default" + ) + } + + func test_givenALimit_whenBuildingTheTrayRequest_thenItTravelsAlongsideTheScope() { + // Boundary: the limit is only meaningful under `scope=all` — `scope=tray` + // ignores it server-side — so the two have to go out together. + let request = Notifications.tray(limit: 40) + XCTAssertEqual(request.query.first(where: { $0.name == "scope" })?.value, "all") + XCTAssertEqual(request.query.first(where: { $0.name == "limit" })?.value, "40") + } + + func test_givenNoLimit_whenBuildingTheTrayRequest_thenTheParameterIsOmitted() { + // Omitting it is not the same as sending zero: an absent `limit` makes + // the server apply the account's own stored preference. + XCTAssertNil(Notifications.tray().query.first(where: { $0.name == "limit" })?.value) + } +}