From 6d9c2a8894b9631254c51039a8ac045839e26f37 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sat, 22 Aug 2026 22:22:29 -0700 Subject: [PATCH 1/5] feat(contracts): make the reading-session rules a shared file instead of test code Aggregate reading time is summed across every device a reader uses, so "active time" has to mean the same thing on all of them. Until now the rules existed only as Vitest calls in the web tracker's suite, which no other client can consume. Extract them into contracts/reading-sessions.json: a versioned, language-neutral table carrying the idle timeout and checkpoint interval as data rather than assuming them, and rewrite the web suite as a runner over it. What is left in the suite is the mapping from a contract event onto this tracker's API, and the browser-specific storage failures the contract deliberately leaves to each platform. The contract constrains the state machine exactly: given a case's events and timestamps, a conformant tracker produces the same sessions, kinds, durations, pages, and positions, and the same result after a crash. It does not constrain which platform occurrence produces which event, so a client with real lifecycle callbacks can be more precise about when it pauses without the arithmetic drifting. Three assertions that held for every scenario rather than one became invariants checked on every case, and asserting the span invariant everywhere caught a real bug: a session rescued from a crash ends at its last recorded activity, but checkpointNow writes accrued time that no activity followed, so a session checkpointed on pagehide could report a minute of reading inside a zero-length span. toPayload now floors the end at startedAt + activeMs. --- contracts/README.md | 73 ++ contracts/reading-sessions.json | 575 +++++++++++++++ .../lib/reading/ReadingSessionTracker.test.ts | 680 +++++++----------- web/src/lib/reading/ReadingSessionTracker.ts | 6 +- 4 files changed, 931 insertions(+), 403 deletions(-) create mode 100644 contracts/README.md create mode 100644 contracts/reading-sessions.json diff --git a/contracts/README.md b/contracts/README.md new file mode 100644 index 00000000..2831d36c --- /dev/null +++ b/contracts/README.md @@ -0,0 +1,73 @@ +# Contracts + +Behavioural contracts that more than one Codex client has to implement identically. + +Unlike `docs/api/openapi.json`, nothing here is generated from the server. These files describe +behaviour that lives in the *clients* but whose output lands in shared server data, so if two +clients implement it differently the data they produce silently stops meaning one thing. + +Each file is hand-authored, versioned by a `version` field, and consumed as data by a test suite in +every repo that implements it. A change to the definition is therefore a reviewable diff, and a +client that has not adopted it fails its own suite. + +| File | Implemented by | Consumed by | +| --- | --- | --- | +| [`reading-sessions.json`](./reading-sessions.json) | web reader, codex-reader-ios | `web/src/lib/reading/ReadingSessionTracker.test.ts` | + +## `reading-sessions.json` + +The measurement rules behind `POST /api/v1/reading-sessions`. Aggregate reading time is summed +across every device a reader uses, so "active time" has to mean the same thing on all of them. If +one client idles at two minutes and another at five, the total is a blend of two metrics and means +nothing. + +### What it constrains, and what it does not + +**Constrained, exactly:** the state machine and its arithmetic. Given the event sequence in a case, +with those timestamps, a conformant tracker produces exactly the sessions the case lists: the same +count, the same kinds, the same order, the same `activeDurationMs`, `pagesRead`, and position. The +same holds for what survives a crash. + +**Not constrained:** which platform occurrence produces which event. The web reader decides that +`visibilitychange` means `pause`; the native client decides that a `scenePhase` transition does. +That mapping is each client's business, and a native client with real lifecycle callbacks is free to +be more precise about *when* it emits `pause` than the web reader can be. Also unconstrained: how a +checkpoint is persisted, and what an id looks like beyond being unique within a case. + +That line is where the earlier open question lands. Pause and resume *are* in the contract, because +they are modelled as events rather than as platform occurrences. What a client detects, and how +promptly, is latitude. What it does once it has detected it is not. + +### Schema + +- `version` — bump on any change to a threshold, an event's meaning, or a case's expectation. +- `subject` — the book, device, and device name every case runs against, unless a case overrides + `bookId`. +- `thresholds` — `idleTimeoutMs` and `checkpointIntervalMs`, carried here rather than assumed, so a + change to the definition of "active" shows up in this file. +- `invariants` — properties asserted on every case in addition to its own expectations. +- `cases[]`: + - `name`, `group`, optional `why`. + - `bookId` — overrides `subject.bookId`. + - `persistence: "unavailable"` — run this case with a store that cannot be written. How the runner + arranges that is platform-specific. + - `seedCheckpoints[]` — `{bookId, raw}` written into the store before the case runs, to model a + checkpoint left by an older or broken write. + - `events[]` — `atMs` is absolute from the start of the case, so a runner drives a fake clock to + each timestamp in turn. Kinds: `start`, `activity` (both take optional `page` or `percentage`), + `pause`, `resume`, `stop`, `complete` (optional position), `reset`, `checkpoint` (persist + without closing), `crash` (the process dies with no chance to close), `recover` (run orphan + recovery, appending its result to `recoveries`). + - `expect`: + - `sessions[]` — emitted sessions in order, in the wire shape of the endpoint. A field set to + `null` must be **absent** from the payload. `spanMs`, where present, is + `clientEndedAt - clientStartedAt`. Fields not listed are not asserted. + - `recoveries[]` — one entry per `recover` event, each an array of recovered sessions. + - `totalActiveDurationMs` — summed across `sessions`, where the point of the case is the total. + - `tracking` — whether a session is still open at the end. + - `checkpointedBookIds` — book ids with a checkpoint waiting, sorted. + +### Adding a case + +Add it here first, then make both suites pass. A case whose events duplicate an existing case +belongs as extra expectations on that case rather than as a new row. diff --git a/contracts/reading-sessions.json b/contracts/reading-sessions.json new file mode 100644 index 00000000..59d45d37 --- /dev/null +++ b/contracts/reading-sessions.json @@ -0,0 +1,575 @@ +{ + "$comment": "The shared behavioural contract for reading-session measurement. See contracts/README.md.", + "version": 1, + "subject": { + "bookId": "book-1", + "deviceId": "device-1", + "deviceName": "Test Device" + }, + "thresholds": { + "idleTimeoutMs": 300000, + "checkpointIntervalMs": 30000 + }, + "invariants": [ + { + "name": "active time never exceeds the session span", + "rule": "For every session a case produces, activeDurationMs <= clientEndedAt - clientStartedAt." + }, + { + "name": "timestamps are ordered", + "rule": "For every session a case produces, clientEndedAt >= clientStartedAt." + }, + { + "name": "session ids are distinct", + "rule": "Within a single case, no two produced sessions share an id." + } + ], + "cases": [ + { + "name": "accumulates time between activity events", + "group": "measuring active time", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 60000, "page": 2 }, + { "kind": "activity", "atMs": 120000, "page": 3 }, + { "kind": "stop", "atMs": 120000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 120000, + "toPage": 3, + "pagesRead": 3 + } + ] + } + }, + { + "name": "a gap longer than the idle timeout contributes no active time", + "group": "measuring active time", + "why": "A book left open on the nightstand must not read as an hour of reading.", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 60000, "page": 2 }, + { "kind": "activity", "atMs": 3660000, "page": 3 }, + { "kind": "stop", "atMs": 3660000 } + ], + "expect": { + "totalActiveDurationMs": 60000, + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 2, + "pagesRead": 2 + }, + { + "kind": "progress", + "activeDurationMs": null, + "toPage": 3, + "pagesRead": 1 + } + ] + } + }, + { + "name": "an idle gap splits one sitting into separate sessions", + "group": "measuring active time", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 60000, "page": 2 }, + { "kind": "activity", "atMs": 420000, "page": 3 }, + { "kind": "activity", "atMs": 540000, "page": 4 }, + { "kind": "stop", "atMs": 540000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 2, + "pagesRead": 2 + }, + { + "kind": "progress", + "activeDurationMs": 120000, + "toPage": 4, + "pagesRead": 2 + } + ] + } + }, + { + "name": "a long but sub-timeout dwell on one page is reading", + "group": "measuring active time", + "why": "Four minutes on a dense page is reading, not idling.", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 240000, "page": 2 }, + { "kind": "stop", "atMs": 240000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 240000, + "toPage": 2, + "pagesRead": 2 + } + ] + } + }, + { + "name": "the clock stops while paused", + "group": "measuring active time", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "pause", "atMs": 60000 }, + { "kind": "resume", "atMs": 240000 }, + { "kind": "activity", "atMs": 300000, "page": 2 }, + { "kind": "stop", "atMs": 300000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 120000, + "toPage": 2, + "pagesRead": 2 + } + ] + } + }, + { + "name": "a pause outlasting the idle timeout closes the session", + "group": "measuring active time", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "pause", "atMs": 60000 }, + { "kind": "resume", "atMs": 420000 } + ], + "expect": { + "tracking": false, + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1 + } + ] + } + }, + { + "name": "a stop after the session already closed is a no-op", + "group": "measuring active time", + "why": "Pins the span invariant against the longest gap a session can carry.", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "pause", "atMs": 60000 }, + { "kind": "resume", "atMs": 660000 }, + { "kind": "stop", "atMs": 720000 } + ], + "expect": { + "tracking": false, + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1 + } + ] + } + }, + { + "name": "the last position reached is reported", + "group": "position and pages", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 60000, "page": 40 }, + { "kind": "stop", "atMs": 60000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 40, + "pagesRead": 2 + } + ] + } + }, + { + "name": "a deliberate rewind is the final position", + "group": "position and pages", + "events": [ + { "kind": "start", "atMs": 0, "page": 50 }, + { "kind": "activity", "atMs": 60000, "page": 49 }, + { "kind": "stop", "atMs": 60000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 49, + "pagesRead": 2 + } + ] + } + }, + { + "name": "distinct pages are counted, not page events", + "group": "position and pages", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 60000, "page": 2 }, + { "kind": "activity", "atMs": 60000, "page": 3 }, + { "kind": "activity", "atMs": 60000, "page": 2 }, + { "kind": "activity", "atMs": 60000, "page": 3 }, + { "kind": "stop", "atMs": 60000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 3, + "pagesRead": 3 + } + ] + } + }, + { + "name": "a reflowable position is a percentage rather than a page", + "group": "position and pages", + "events": [ + { "kind": "start", "atMs": 0, "percentage": 0.1 }, + { "kind": "activity", "atMs": 60000, "percentage": 0.42 }, + { "kind": "stop", "atMs": 60000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPercentage": 0.42, + "toPage": null, + "pagesRead": null + } + ] + } + }, + { + "name": "completion is its own session kind and closes tracking", + "group": "completion and reset", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "complete", "atMs": 60000, "page": 100 } + ], + "expect": { + "tracking": false, + "sessions": [ + { + "kind": "completed", + "activeDurationMs": 60000, + "toPage": 100, + "pagesRead": 1 + } + ] + } + }, + { + "name": "a reset is emitted as its own event after the running session", + "group": "completion and reset", + "why": "Sent as an event rather than a deletion so the server can order it against a completion made on another device.", + "events": [ + { "kind": "start", "atMs": 0, "page": 20 }, + { "kind": "reset", "atMs": 60000 } + ], + "expect": { + "tracking": false, + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 20, + "pagesRead": 1 + }, + { + "kind": "reset", + "activeDurationMs": null, + "toPage": null, + "pagesRead": null + } + ] + } + }, + { + "name": "a session with nothing to report is not emitted", + "group": "session hygiene", + "events": [ + { "kind": "start", "atMs": 0 }, + { "kind": "stop", "atMs": 0 } + ], + "expect": { + "tracking": false, + "sessions": [] + } + }, + { + "name": "a repeated start does not open a second session", + "group": "session hygiene", + "why": "A re-render must not fragment a sitting.", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "stop", "atMs": 60000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1 + } + ] + } + }, + { + "name": "a resume without a preceding pause is ignored", + "group": "session hygiene", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "resume", "atMs": 60000 }, + { "kind": "stop", "atMs": 120000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 120000, + "toPage": 1, + "pagesRead": 1 + } + ] + } + }, + { + "name": "consecutive sessions get distinct ids", + "group": "session hygiene", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "stop", "atMs": 60000 }, + { "kind": "start", "atMs": 60000, "page": 2 }, + { "kind": "stop", "atMs": 120000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1 + }, + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 2, + "pagesRead": 1 + } + ] + } + }, + { + "name": "a plain session is bracketed by its own timestamps", + "group": "session hygiene", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "stop", "atMs": 60000 } + ], + "expect": { + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1, + "spanMs": 60000 + } + ] + } + }, + { + "name": "a checkpoint exists once an interval of active time has passed", + "group": "checkpointing and crash recovery", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 31000, "page": 2 } + ], + "expect": { + "tracking": true, + "checkpointedBookIds": ["book-1"], + "sessions": [] + } + }, + { + "name": "a session left behind by a crash is recovered", + "group": "checkpointing and crash recovery", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 120000, "page": 12 }, + { "kind": "checkpoint", "atMs": 120000 }, + { "kind": "crash", "atMs": 120000 }, + { "kind": "recover", "atMs": 900000 } + ], + "expect": { + "sessions": [], + "recoveries": [ + [ + { + "kind": "progress", + "activeDurationMs": 120000, + "toPage": 12, + "pagesRead": 2 + } + ] + ] + } + }, + { + "name": "a recovered session ends at its checkpoint, not at recovery time", + "group": "checkpointing and crash recovery", + "why": "Counting the time the app was shut would inflate reading time by however long that was.", + "events": [ + { "kind": "start", "atMs": 0, "page": 5 }, + { "kind": "activity", "atMs": 60000, "page": 5 }, + { "kind": "checkpoint", "atMs": 60000 }, + { "kind": "crash", "atMs": 60000 }, + { "kind": "recover", "atMs": 86400000 } + ], + "expect": { + "sessions": [], + "recoveries": [ + [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 5, + "pagesRead": 1, + "spanMs": 60000 + } + ] + ] + } + }, + { + "name": "a crash loses at most one checkpoint interval of active time", + "group": "checkpointing and crash recovery", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "activity", "atMs": 30000, "page": 2 }, + { "kind": "crash", "atMs": 40000 }, + { "kind": "recover", "atMs": 40000 } + ], + "expect": { + "sessions": [], + "recoveries": [ + [ + { + "kind": "progress", + "activeDurationMs": 30000, + "toPage": 2, + "pagesRead": 2 + } + ] + ] + } + }, + { + "name": "a clean close clears the checkpoint so nothing is double-counted", + "group": "checkpointing and crash recovery", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "stop", "atMs": 60000 }, + { "kind": "recover", "atMs": 60000 } + ], + "expect": { + "checkpointedBookIds": [], + "recoveries": [[]], + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1 + } + ] + } + }, + { + "name": "a recovered checkpoint is consumed so it cannot be recovered twice", + "group": "checkpointing and crash recovery", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "checkpoint", "atMs": 60000 }, + { "kind": "crash", "atMs": 60000 }, + { "kind": "recover", "atMs": 60000 }, + { "kind": "recover", "atMs": 60000 } + ], + "expect": { + "sessions": [], + "checkpointedBookIds": [], + "recoveries": [ + [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1, + "spanMs": 60000 + } + ], + [] + ] + } + }, + { + "name": "an unreadable checkpoint is discarded rather than retried forever", + "group": "checkpointing and crash recovery", + "bookId": "book-9", + "seedCheckpoints": [{ "bookId": "book-9", "raw": "{not json" }], + "events": [{ "kind": "recover", "atMs": 0 }], + "expect": { + "sessions": [], + "checkpointedBookIds": [], + "recoveries": [[]] + } + }, + { + "name": "measurement is unaffected when persistence is unavailable", + "group": "degraded environments", + "why": "Losing crash recovery is acceptable. Losing the session in front of the reader is not.", + "persistence": "unavailable", + "events": [ + { "kind": "start", "atMs": 0, "page": 1 }, + { "kind": "stop", "atMs": 60000 } + ], + "expect": { + "checkpointedBookIds": [], + "sessions": [ + { + "kind": "progress", + "activeDurationMs": 60000, + "toPage": 1, + "pagesRead": 1 + } + ] + } + } + ] +} diff --git a/web/src/lib/reading/ReadingSessionTracker.test.ts b/web/src/lib/reading/ReadingSessionTracker.test.ts index f22de71e..52ef3f06 100644 --- a/web/src/lib/reading/ReadingSessionTracker.test.ts +++ b/web/src/lib/reading/ReadingSessionTracker.test.ts @@ -1,24 +1,73 @@ /** - * The shared case table for reading-session measurement. + * Drives the tracker through the shared reading-session case table. * - * The iOS client implements the same state machine and is tested against this - * same list of cases. If a case changes here it has to change there too, or - * aggregate reading time silently becomes a blend of two different metrics. + * The cases are not written here. They live in `contracts/reading-sessions.json` + * at the repository root, and the iOS client is tested against that same file. + * If a case changes it changes for both, or aggregate reading time silently + * becomes a blend of two different metrics. + * + * Only the mapping from a case's events onto this tracker's API belongs in this + * file, along with the browser-specific storage failures the contract + * deliberately leaves to each platform. */ import { beforeEach, describe, expect, it, vi } from "vitest"; +import contractDocument from "../../../../contracts/reading-sessions.json"; import { DEFAULT_CHECKPOINT_INTERVAL_MS, DEFAULT_IDLE_TIMEOUT_MS, listCheckpointedBookIds, + type ReadingPosition, type ReadingSessionPayload, ReadingSessionTracker, recoverOrphanedSessions, } from "./ReadingSessionTracker"; -const BOOK = "book-1"; -const DEVICE = "device-1"; -const MINUTE = 60 * 1000; +const CHECKPOINT_KEY_PREFIX = "codex.reading.session."; + +interface ContractEvent extends ReadingPosition { + kind: + | "start" + | "activity" + | "pause" + | "resume" + | "stop" + | "complete" + | "reset" + | "checkpoint" + | "crash" + | "recover"; + atMs: number; +} + +/** A `null` means the field must be absent; `spanMs` is derived from the timestamps. */ +type ExpectedSession = Record; + +interface ContractCase { + name: string; + group: string; + why?: string; + bookId?: string; + persistence?: "unavailable"; + seedCheckpoints?: { bookId: string; raw: string }[]; + events: ContractEvent[]; + expect: { + sessions?: ExpectedSession[]; + recoveries?: ExpectedSession[][]; + totalActiveDurationMs?: number; + tracking?: boolean; + checkpointedBookIds?: string[]; + }; +} + +interface Contract { + version: number; + subject: { bookId: string; deviceId: string; deviceName: string }; + thresholds: { idleTimeoutMs: number; checkpointIntervalMs: number }; + cases: ContractCase[]; +} + +const contract = contractDocument as unknown as Contract; /** An in-memory Storage, so tests never depend on jsdom's localStorage state. */ function memoryStorage(): Storage { @@ -39,434 +88,261 @@ function memoryStorage(): Storage { } as Storage; } -function setup(overrides: { idleTimeoutMs?: number } = {}) { +interface CaseResult { + emitted: ReadingSessionPayload[]; + recoveries: ReadingSessionPayload[][]; + tracking: boolean; + checkpointedBookIds: string[]; +} + +function runCase(testCase: ContractCase): CaseResult { + const bookId = testCase.bookId ?? contract.subject.bookId; + const storage = + testCase.persistence === "unavailable" ? null : memoryStorage(); + + for (const seed of testCase.seedCheckpoints ?? []) { + storage?.setItem(`${CHECKPOINT_KEY_PREFIX}${seed.bookId}`, seed.raw); + } + let clock = 0; let counter = 0; const emitted: ReadingSessionPayload[] = []; - const storage = memoryStorage(); + const recoveries: ReadingSessionPayload[][] = []; - const tracker = new ReadingSessionTracker({ - bookId: BOOK, - deviceId: DEVICE, - deviceName: "Test Device", + let tracker: ReadingSessionTracker | null = new ReadingSessionTracker({ + bookId, + deviceId: contract.subject.deviceId, + deviceName: contract.subject.deviceName, emit: (sessions) => emitted.push(...sessions), now: () => clock, newId: () => `session-${++counter}`, storage, - ...overrides, + idleTimeoutMs: contract.thresholds.idleTimeoutMs, + checkpointIntervalMs: contract.thresholds.checkpointIntervalMs, }); + for (const event of testCase.events) { + clock = event.atMs; + const position: ReadingPosition = {}; + if (typeof event.page === "number") position.page = event.page; + if (typeof event.percentage === "number") { + position.percentage = event.percentage; + } + + switch (event.kind) { + case "start": + tracker?.start(position); + break; + case "activity": + tracker?.recordActivity(position); + break; + case "pause": + tracker?.pause(); + break; + case "resume": + tracker?.resume(); + break; + case "stop": + tracker?.stop(); + break; + case "complete": + tracker?.markCompleted(position); + break; + case "reset": + tracker?.markReset(); + break; + case "checkpoint": + tracker?.checkpointNow(); + break; + case "crash": + // The process dies with no chance to close: drop the tracker with the + // store exactly as its last checkpoint left it. + tracker = null; + break; + case "recover": + recoveries.push(recoverOrphanedSessions([bookId], storage)); + break; + } + } + return { - tracker, emitted, - storage, - advance: (ms: number) => { - clock += ms; - }, - at: () => clock, + recoveries, + tracking: tracker?.isTracking ?? false, + checkpointedBookIds: listCheckpointedBookIds(storage).sort(), }; } -describe("ReadingSessionTracker", () => { - beforeEach(() => { - vi.restoreAllMocks(); +function assertSessions( + actual: ReadingSessionPayload[], + expected: ExpectedSession[], + label: string, +): void { + expect(actual, `${label}: session count`).toHaveLength(expected.length); + + expected.forEach((expectation, index) => { + const session = actual[index] as unknown as Record; + const where = `${label}[${index}]`; + + for (const [field, value] of Object.entries(expectation)) { + if (field === "spanMs") { + const span = + new Date(actual[index].clientEndedAt).getTime() - + new Date(actual[index].clientStartedAt).getTime(); + expect(span, `${where}.spanMs`).toBe(value); + } else if (value === null) { + expect( + session[field], + `${where}.${field} must be absent`, + ).toBeUndefined(); + } else if (typeof value === "number" && !Number.isInteger(value)) { + expect(session[field] as number, `${where}.${field}`).toBeCloseTo( + value, + ); + } else { + expect(session[field], `${where}.${field}`).toBe(value); + } + } }); +} - describe("measuring active time", () => { - it("accumulates time between activity events", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.recordActivity({ page: 2 }); - advance(MINUTE); - tracker.recordActivity({ page: 3 }); - tracker.stop(); - - expect(emitted).toHaveLength(1); - expect(emitted[0].activeDurationMs).toBe(2 * MINUTE); - }); - - it("does not count a gap longer than the idle timeout", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.recordActivity({ page: 2 }); - - // Away for an hour, then back. The gap closes the session. - advance(60 * MINUTE); - tracker.recordActivity({ page: 3 }); - tracker.stop(); - - const total = emitted.reduce( - (sum, s) => sum + (s.activeDurationMs ?? 0), - 0, - ); - expect(total).toBe(MINUTE); - }); - - it("splits a sitting into separate sessions across an idle gap", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.recordActivity({ page: 2 }); - - advance(DEFAULT_IDLE_TIMEOUT_MS + MINUTE); - tracker.recordActivity({ page: 3 }); - advance(2 * MINUTE); - tracker.recordActivity({ page: 4 }); - tracker.stop(); - - expect(emitted).toHaveLength(2); - expect(emitted[0].activeDurationMs).toBe(MINUTE); - expect(emitted[1].activeDurationMs).toBe(2 * MINUTE); - }); - - it("counts a long but sub-timeout pause on one page as reading", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - // Four minutes on a dense page is reading, not idling. - advance(4 * MINUTE); - tracker.recordActivity({ page: 2 }); - tracker.stop(); - - expect(emitted[0].activeDurationMs).toBe(4 * MINUTE); - }); - - it("stops the clock while paused", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.pause(); - - advance(3 * MINUTE); // backgrounded - tracker.resume(); - - advance(MINUTE); - tracker.recordActivity({ page: 2 }); - tracker.stop(); - - expect(emitted[0].activeDurationMs).toBe(2 * MINUTE); - }); - - it("closes the session when a pause outlasts the idle timeout", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.pause(); - - advance(DEFAULT_IDLE_TIMEOUT_MS + MINUTE); - tracker.resume(); - - expect(emitted).toHaveLength(1); - expect(emitted[0].activeDurationMs).toBe(MINUTE); - expect(tracker.isTracking).toBe(false); - }); - - it("never reports more active time than the session's own span", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.pause(); - advance(10 * MINUTE); - tracker.resume(); - advance(MINUTE); - tracker.stop(); - - const session = emitted[0]; - const span = - new Date(session.clientEndedAt).getTime() - - new Date(session.clientStartedAt).getTime(); - expect(session.activeDurationMs ?? 0).toBeLessThanOrEqual(span); - }); - }); - - describe("position and pages", () => { - it("reports the last position reached", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.recordActivity({ page: 40 }); - tracker.stop(); - - expect(emitted[0].toPage).toBe(40); - }); - - it("reports a deliberate rewind as the final position", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 50 }); - advance(MINUTE); - tracker.recordActivity({ page: 49 }); - tracker.stop(); - - expect(emitted[0].toPage).toBe(49); - }); - - it("counts distinct pages rather than page events", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.recordActivity({ page: 2 }); - tracker.recordActivity({ page: 3 }); - tracker.recordActivity({ page: 2 }); // back - tracker.recordActivity({ page: 3 }); // forward again - tracker.stop(); - - expect(emitted[0].pagesRead).toBe(3); - }); - - it("carries an EPUB percentage instead of a page", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ percentage: 0.1 }); - advance(MINUTE); - tracker.recordActivity({ percentage: 0.42 }); - tracker.stop(); +function assertInvariants(sessions: ReadingSessionPayload[]): void { + const ids = new Set(); + + for (const session of sessions) { + const startedAt = new Date(session.clientStartedAt).getTime(); + const endedAt = new Date(session.clientEndedAt).getTime(); + + expect(endedAt, "clientEndedAt >= clientStartedAt").toBeGreaterThanOrEqual( + startedAt, + ); + expect( + session.activeDurationMs ?? 0, + "activeDurationMs <= session span", + ).toBeLessThanOrEqual(endedAt - startedAt); + + expect(ids.has(session.id), `duplicate session id ${session.id}`).toBe( + false, + ); + ids.add(session.id); + } +} - expect(emitted[0].toPercentage).toBeCloseTo(0.42); - expect(emitted[0].toPage).toBeUndefined(); - }); +describe("reading-session contract", () => { + beforeEach(() => { + vi.restoreAllMocks(); }); - describe("completion and reset", () => { - it("emits a completed session", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.markCompleted({ page: 100 }); - - expect(emitted).toHaveLength(1); - expect(emitted[0].kind).toBe("completed"); - expect(emitted[0].toPage).toBe(100); - }); - - it("emits a reset as its own event", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 20 }); - advance(MINUTE); - tracker.markReset(); - - expect(emitted).toHaveLength(2); - expect(emitted[0].kind).toBe("progress"); - expect(emitted[1].kind).toBe("reset"); - }); - - it("closes tracking after completing", () => { - const { tracker, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.markCompleted({ page: 100 }); - - expect(tracker.isTracking).toBe(false); - }); + it("is the version the tracker's defaults were written for", () => { + expect(contract.version).toBe(1); + expect(contract.thresholds.idleTimeoutMs).toBe(DEFAULT_IDLE_TIMEOUT_MS); + expect(contract.thresholds.checkpointIntervalMs).toBe( + DEFAULT_CHECKPOINT_INTERVAL_MS, + ); }); - describe("session hygiene", () => { - it("does not emit a session with nothing to report", () => { - const { tracker, emitted } = setup(); - - tracker.start(); - tracker.stop(); - - expect(emitted).toHaveLength(0); - }); - - it("does not start a second session on a repeated start", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.stop(); - - expect(emitted).toHaveLength(1); - }); - - it("ignores resume without a preceding pause", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.resume(); - advance(MINUTE); - tracker.stop(); - - expect(emitted[0].activeDurationMs).toBe(2 * MINUTE); + const groups = [...new Set(contract.cases.map((c) => c.group))]; + + for (const group of groups) { + describe(group, () => { + for (const testCase of contract.cases.filter((c) => c.group === group)) { + it(testCase.name, () => { + const result = runCase(testCase); + + if (testCase.expect.sessions) { + assertSessions( + result.emitted, + testCase.expect.sessions, + "sessions", + ); + } + + if (testCase.expect.recoveries) { + expect(result.recoveries, "recovery count").toHaveLength( + testCase.expect.recoveries.length, + ); + testCase.expect.recoveries.forEach((expected, index) => { + assertSessions( + result.recoveries[index], + expected, + `recoveries[${index}]`, + ); + }); + } + + if (testCase.expect.totalActiveDurationMs !== undefined) { + const total = result.emitted.reduce( + (sum, session) => sum + (session.activeDurationMs ?? 0), + 0, + ); + expect(total, "totalActiveDurationMs").toBe( + testCase.expect.totalActiveDurationMs, + ); + } + + if (testCase.expect.tracking !== undefined) { + expect(result.tracking, "tracking").toBe(testCase.expect.tracking); + } + + if (testCase.expect.checkpointedBookIds) { + expect(result.checkpointedBookIds, "checkpointedBookIds").toEqual( + testCase.expect.checkpointedBookIds, + ); + } + + assertInvariants([...result.emitted, ...result.recoveries.flat()]); + }); + } }); + } +}); - it("gives each session a distinct id", () => { - const { tracker, emitted, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.stop(); - - tracker.start({ page: 2 }); - advance(MINUTE); - tracker.stop(); - - expect(emitted[0].id).not.toBe(emitted[1].id); +/** + * The contract says measurement must survive a store it cannot write to, and + * leaves it to each platform to say how that happens. On the web there are two + * routes into it, and both have to land on the same behaviour. + */ +describe("browser storage failures", () => { + const MINUTE = 60 * 1000; + + function trackerWith(storage: Storage | null) { + const emitted: ReadingSessionPayload[] = []; + let clock = 0; + const tracker = new ReadingSessionTracker({ + bookId: contract.subject.bookId, + deviceId: contract.subject.deviceId, + emit: (sessions) => emitted.push(...sessions), + now: () => clock, + newId: () => "session-1", + storage, }); + return { tracker, emitted, advance: (ms: number) => (clock += ms) }; + } - it("emits ISO timestamps that bracket the session", () => { - const { tracker, emitted, advance } = setup(); + it("keeps measuring when storage is unavailable", () => { + const { tracker, emitted, advance } = trackerWith(null); - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.stop(); + tracker.start({ page: 1 }); + advance(MINUTE); + tracker.stop(); - const { clientStartedAt, clientEndedAt } = emitted[0]; - expect(new Date(clientEndedAt).getTime()).toBeGreaterThanOrEqual( - new Date(clientStartedAt).getTime(), - ); - }); + expect(emitted[0].activeDurationMs).toBe(MINUTE); }); - describe("checkpointing and crash recovery", () => { - it("checkpoints after an interval of active time", () => { - const { tracker, storage, advance } = setup(); - - tracker.start({ page: 1 }); - advance(DEFAULT_CHECKPOINT_INTERVAL_MS + 1000); - tracker.recordActivity({ page: 2 }); - - expect(listCheckpointedBookIds(storage)).toEqual([BOOK]); - }); - - it("recovers a session left by a tab that died", () => { - const { tracker, storage, advance } = setup(); - - tracker.start({ page: 1 }); - advance(2 * MINUTE); - tracker.recordActivity({ page: 12 }); - tracker.checkpointNow(); - - // The tab dies here: no stop(), no pagehide. - const recovered = recoverOrphanedSessions([BOOK], storage); - - expect(recovered).toHaveLength(1); - expect(recovered[0].toPage).toBe(12); - expect(recovered[0].activeDurationMs).toBe(2 * MINUTE); - }); - - it("closes a recovered session at its checkpoint, not at recovery time", () => { - const { tracker, storage, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.recordActivity({ page: 5 }); - tracker.checkpointNow(); - - const recovered = recoverOrphanedSessions([BOOK], storage); - const span = - new Date(recovered[0].clientEndedAt).getTime() - - new Date(recovered[0].clientStartedAt).getTime(); - - expect(span).toBe(MINUTE); - }); - - it("loses at most the time since the last checkpoint", () => { - const { tracker, storage, advance } = setup(); - - tracker.start({ page: 1 }); - advance(DEFAULT_CHECKPOINT_INTERVAL_MS); - tracker.recordActivity({ page: 2 }); // triggers a checkpoint - advance(10 * 1000); // unrecorded when the tab dies - - const recovered = recoverOrphanedSessions([BOOK], storage); - expect(recovered[0].activeDurationMs).toBe( - DEFAULT_CHECKPOINT_INTERVAL_MS, - ); - }); - - it("clears the checkpoint on a clean close so nothing is double-counted", () => { - const { tracker, storage, advance } = setup(); - - tracker.start({ page: 1 }); - advance(MINUTE); - tracker.stop(); - - expect(listCheckpointedBookIds(storage)).toEqual([]); - expect(recoverOrphanedSessions([BOOK], storage)).toEqual([]); - }); - - it("consumes a recovered checkpoint so it is not recovered twice", () => { - const { tracker, storage, advance } = setup(); + it("survives a storage that throws on write", () => { + const throwing = { + ...memoryStorage(), + setItem: () => { + throw new Error("QuotaExceededError"); + }, + } as unknown as Storage; + const { tracker, emitted, advance } = trackerWith(throwing); + expect(() => { tracker.start({ page: 1 }); advance(MINUTE); - tracker.checkpointNow(); - - expect(recoverOrphanedSessions([BOOK], storage)).toHaveLength(1); - expect(recoverOrphanedSessions([BOOK], storage)).toHaveLength(0); - }); - - it("discards an unparseable checkpoint rather than retrying forever", () => { - const storage = memoryStorage(); - storage.setItem("codex.reading.session.book-9", "{not json"); - - expect(recoverOrphanedSessions(["book-9"], storage)).toEqual([]); - expect(listCheckpointedBookIds(storage)).toEqual([]); - }); - }); - - describe("degraded environments", () => { - it("keeps measuring when storage is unavailable", () => { - const emitted: ReadingSessionPayload[] = []; - let clock = 0; - const tracker = new ReadingSessionTracker({ - bookId: BOOK, - deviceId: DEVICE, - emit: (s) => emitted.push(...s), - now: () => clock, - newId: () => "session-1", - storage: null, - }); - - tracker.start({ page: 1 }); - clock += MINUTE; tracker.stop(); - - expect(emitted[0].activeDurationMs).toBe(MINUTE); - }); - - it("survives a storage that throws on write", () => { - const throwing = { - ...memoryStorage(), - setItem: () => { - throw new Error("QuotaExceededError"); - }, - } as unknown as Storage; - - const emitted: ReadingSessionPayload[] = []; - let clock = 0; - const tracker = new ReadingSessionTracker({ - bookId: BOOK, - deviceId: DEVICE, - emit: (s) => emitted.push(...s), - now: () => clock, - newId: () => "session-1", - storage: throwing, - }); - - expect(() => { - tracker.start({ page: 1 }); - clock += MINUTE; - tracker.stop(); - }).not.toThrow(); - expect(emitted).toHaveLength(1); - }); + }).not.toThrow(); + expect(emitted).toHaveLength(1); }); }); diff --git a/web/src/lib/reading/ReadingSessionTracker.ts b/web/src/lib/reading/ReadingSessionTracker.ts index a33e31ae..dbc91810 100644 --- a/web/src/lib/reading/ReadingSessionTracker.ts +++ b/web/src/lib/reading/ReadingSessionTracker.ts @@ -436,8 +436,12 @@ function toPayload( deviceId: checkpoint.deviceId, kind, clientStartedAt: new Date(checkpoint.startedAt).toISOString(), + // A session cannot end before the time it accrued. Recovery ends a session + // at its last recorded activity, and `checkpointNow` writes time that no + // activity followed, so without this floor a session rescued from a crash + // can report minutes of reading inside a zero-length span. clientEndedAt: new Date( - Math.max(endedAt, checkpoint.startedAt), + Math.max(endedAt, checkpoint.startedAt + checkpoint.activeMs), ).toISOString(), }; if (checkpoint.deviceName) payload.deviceName = checkpoint.deviceName; From c7acec1d8685b0058fbb397ba3ce2fd3dc917ab1 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sat, 22 Aug 2026 22:22:29 -0700 Subject: [PATCH 2/5] build(changelog): give API changes their own group and fix the group ordering Grouping purely by commit type scatters API changes through Features and Bug Fixes among web, config, and tooling entries. Five of 2.2.0's seven entries were API changes a client author has to read, interleaved with Docusaurus and Vitest entries they must not. The scope was already on those commits and the grouping threw it away. Pull `api`-scoped commits into their own group ahead of the type-based ones, with a separate breaking variant. The scope pattern matches `api` as a whole element of a comma-separated list, so `feat(api, auth, config, db)` counts, of which this repo has many; `feat(komga-api)` does not, being a compatibility layer rather than the native API. Restricted to the types that can move the wire, since a `test(api)` commit in the section a client reads defeats the point of the section. The header now carries the release policy that makes the changelog usable as a compatibility reference: API features ship in minor releases and are never backported into a patch. A client can then treat the release a feature first appears under as a floor it can rely on, and decide what a server supports from the changelog rather than by probing. It lives here rather than in the docs so the promise sits directly above the data it is about, and is regenerated rather than maintained. Two ordering defects fixed while in the file. Groups sort as strings, so the unpadded `` sorted between 1 and 2 and put Other third in every release; all keys are now zero-padded. And `build` commits matched no parser and fell through to Other, which is what put the Docusaurus upgrade there. CHANGELOG.md is generated and untouched. The regrouping applies retroactively, so the next regeneration will produce a large diff as past releases gain API sections. --- cliff.toml | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/cliff.toml b/cliff.toml index 1b713122..78cb53db 100644 --- a/cliff.toml +++ b/cliff.toml @@ -10,6 +10,11 @@ header = """ All notable changes to Codex will be documented in this file. +**API compatibility.** API features ship in minor releases and are never backported into a patch +release. A patch release may fix an API bug; it may not add an operation, a parameter, a response +field, or a new behaviour on an existing route. A client may therefore treat the release a feature +first appears under in the 🔌 API sections below as a floor it can rely on. + """ body = """ {% if version %}\ @@ -67,22 +72,34 @@ protect_breaking_commits = false # An array of regex based parsers for extracting data from the commit message. # Assigns commits to groups. # Optionally sets the commit's scope and can decide to exclude commits from further processing. +# Ordering keys are zero-padded because groups sort as strings: an unpadded +# "" sorts between 1 and 2, which is why "Other" used to appear third. +# +# The API parsers come first, and therefore win over the type-based groups below, +# because the audience for an API change is a client author regenerating against +# openapi.json, and they need it separated from web and config work rather than +# interleaved with it. Scoped to the types that can move the wire: a `test(api)` +# or `style(api)` commit changes nothing a client can observe. The scope pattern +# matches `api` as a whole element of a comma-separated scope list, so +# `feat(api, db)` counts and `feat(komga-api)` does not. commit_parsers = [ - { message = "^feat", group = "🚀 Features" }, - { message = "^fix", group = "🐛 Bug Fixes" }, - { message = "^doc", group = "📚 Documentation" }, - { message = "^perf", group = "⚡ Performance" }, - { message = "^refactor", group = "🚜 Refactor" }, - { message = "^style", group = "🎨 Styling" }, - { message = "^test", group = "🧪 Testing" }, + { message = "^(feat|fix|docs?|perf|refactor)\\((?:[a-z0-9_-]+, *)*api(?:, *[a-z0-9_-]+)*\\)!", group = "🔌 API (breaking)" }, + { message = "^(feat|fix|docs?|perf|refactor)\\((?:[a-z0-9_-]+, *)*api(?:, *[a-z0-9_-]+)*\\)", group = "🔌 API" }, + { message = "^feat", group = "🚀 Features" }, + { message = "^fix", group = "🐛 Bug Fixes" }, + { message = "^refactor", group = "🚜 Refactor" }, + { message = "^doc", group = "📚 Documentation" }, + { message = "^perf", group = "⚡ Performance" }, + { message = "^style", group = "🎨 Styling" }, + { message = "^test", group = "🧪 Testing" }, { message = "^chore\\(release\\): prepare for", skip = true }, { message = "^chore\\(deps.*\\)", skip = true }, { message = "^chore\\(pr\\)", skip = true }, { message = "^chore\\(pull\\)", skip = true }, - { message = "^chore|^ci", group = "⚙️ Miscellaneous Tasks" }, - { body = ".*security", group = "🛡️ Security" }, - { message = "^revert", group = "◀️ Revert" }, - { message = ".*", group = "💼 Other" }, + { message = "^chore|^ci|^build", group = "⚙️ Miscellaneous Tasks" }, + { body = ".*security", group = "🛡️ Security" }, + { message = "^revert", group = "◀️ Revert" }, + { message = ".*", group = "💼 Other" }, ] # Exclude commits that are not matched by any commit parser. filter_commits = false From 675b47fc4db6ecb7a6e82654917e70d2b3c4c513 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sat, 22 Aug 2026 22:22:29 -0700 Subject: [PATCH 3/5] docs(api): document the preference keys that exist and stop mocking ones that don't The MSW fixture served `reader.fitMode`, `reader.readingDirection`, `library.defaultView`, `library.itemsPerPage`, `notifications.enabled`, and a bare `theme`. None of those keys exists, and none ever has. The DTO examples carried `reader.zoom` alongside them. Between them they read exactly like a record of what the preference store holds, and were taken as one: a grep over web/src returns them next to the four real keys with nothing to tell them apart. Reader settings are device-local Zustand state persisted to localStorage, and per-series overrides already exist there with their own versioned schema. They are not synced on purpose: a phone and a desktop legitimately want different fit modes. The four keys that really are preferences are now in the description of GET /api/v1/user/preferences, with their value shapes and defaults, so they land in openapi.json where a client generating against it will find them. The description says explicitly that the store is open and the list is not a whitelist, so the correction does not become a new false claim, and it records why reader settings are absent so the next reader finds the answer rather than the gap. The mocks now serve exactly those four, with a test asserting the mocked key set equals PREFERENCE_DEFAULTS. That fixture is the one place an invented key can re-enter, so that is where the guard sits. --- .../src/routes/v1/dto/user_preferences.rs | 4 +-- .../routes/v1/handlers/user_preferences.rs | 28 +++++++++++++-- docs/api/openapi.json | 5 +-- web/openapi.json | 5 +-- web/src/mocks/handlers/coverage.test.ts | 19 +++++++++++ web/src/mocks/handlers/users.ts | 34 +++++++++++++------ 6 files changed, 75 insertions(+), 20 deletions(-) diff --git a/crates/codex-api/src/routes/v1/dto/user_preferences.rs b/crates/codex-api/src/routes/v1/dto/user_preferences.rs index f5ee50a1..f328943f 100644 --- a/crates/codex-api/src/routes/v1/dto/user_preferences.rs +++ b/crates/codex-api/src/routes/v1/dto/user_preferences.rs @@ -12,7 +12,7 @@ use codex_db::repositories::UserPreferencesRepository; #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] #[serde(rename_all = "camelCase")] pub struct UserPreferenceDto { - /// The preference key (e.g., "ui.theme", "reader.zoom") + /// The preference key (e.g., "ui.theme", "library.show_deleted_books") #[schema(example = "ui.theme")] pub key: String, @@ -65,7 +65,7 @@ pub struct SetPreferenceRequest { #[serde(rename_all = "camelCase")] pub struct BulkSetPreferencesRequest { /// Map of preference keys to values - #[schema(example = json!({"ui.theme": "dark", "reader.zoom": 150}))] + #[schema(example = json!({"ui.theme": "dark", "library.show_deleted_books": true}))] pub preferences: HashMap, } diff --git a/crates/codex-api/src/routes/v1/handlers/user_preferences.rs b/crates/codex-api/src/routes/v1/handlers/user_preferences.rs index 04194a3e..b45880fb 100644 --- a/crates/codex-api/src/routes/v1/handlers/user_preferences.rs +++ b/crates/codex-api/src/routes/v1/handlers/user_preferences.rs @@ -38,6 +38,28 @@ use utoipa::OpenApi; pub struct UserPreferencesApi; /// Get all preferences for the authenticated user +/// +/// The store is an open `key -> JSON` map: any syntactically valid key is +/// accepted, and this endpoint returns whatever the user has set. The list +/// below is not a whitelist, it is the set Codex's own clients read and write. +/// A client that wants a user's settings to follow them between devices has to +/// use these exact keys and value shapes. +/// +/// | Key | Value | +/// | --- | --- | +/// | `ui.theme` | `"light"`, `"dark"`, or `"system"` (default `"system"`) | +/// | `library.show_deleted_books` | boolean (default `false`) | +/// | `want_to_read.sort` | `"newest"`, `"oldest"`, or `"custom"` (default `"newest"`) | +/// | `release_tracking.muted_series_ids` | array of series id strings (default `[]`) | +/// +/// Keys are `snake_case`, matching the server settings store rather than the +/// camelCase of the JSON fields around them: a key is a value in a database +/// column, not a field name. +/// +/// Reader settings are deliberately not here. Fit mode, reading direction, +/// zoom, and per-series reader overrides are device-local state, held by each +/// client and never synced, because a phone and a desktop legitimately want +/// different ones. #[utoipa::path( get, path = "/api/v1/user/preferences", @@ -236,7 +258,7 @@ pub async fn delete_preference( } /// Validate a preference key format -/// Valid: "ui.theme", "reader.default_zoom", "library.view_mode" +/// Valid: "ui.theme", "library.show_deleted_books", "release_tracking.muted_series_ids" /// Invalid: ".theme", "ui.", "ui..theme", "ui/theme", "ui theme" fn is_valid_preference_key(key: &str) -> bool { if key.is_empty() || key.len() > 255 { @@ -270,8 +292,8 @@ mod tests { #[test] fn test_valid_preference_keys() { assert!(is_valid_preference_key("ui.theme")); - assert!(is_valid_preference_key("reader.default_zoom")); - assert!(is_valid_preference_key("library.view_mode")); + assert!(is_valid_preference_key("library.show_deleted_books")); + assert!(is_valid_preference_key("release_tracking.muted_series_ids")); assert!(is_valid_preference_key("single_key")); assert!(is_valid_preference_key("deep.nested.key.value")); assert!(is_valid_preference_key("with_underscore.another_one")); diff --git a/docs/api/openapi.json b/docs/api/openapi.json index 55aa7d19..86366514 100644 --- a/docs/api/openapi.json +++ b/docs/api/openapi.json @@ -17853,6 +17853,7 @@ "User Preferences" ], "summary": "Get all preferences for the authenticated user", + "description": "The store is an open `key -> JSON` map: any syntactically valid key is\naccepted, and this endpoint returns whatever the user has set. The list\nbelow is not a whitelist, it is the set Codex's own clients read and write.\nA client that wants a user's settings to follow them between devices has to\nuse these exact keys and value shapes.\n\n| Key | Value |\n| --- | --- |\n| `ui.theme` | `\"light\"`, `\"dark\"`, or `\"system\"` (default `\"system\"`) |\n| `library.show_deleted_books` | boolean (default `false`) |\n| `want_to_read.sort` | `\"newest\"`, `\"oldest\"`, or `\"custom\"` (default `\"newest\"`) |\n| `release_tracking.muted_series_ids` | array of series id strings (default `[]`) |\n\nKeys are `snake_case`, matching the server settings store rather than the\ncamelCase of the JSON fields around them: a key is a value in a database\ncolumn, not a field name.\n\nReader settings are deliberately not here. Fit mode, reading direction,\nzoom, and per-series reader overrides are device-local state, held by each\nclient and never synced, because a phone and a desktop legitimately want\ndifferent ones.", "operationId": "get_all_preferences", "responses": { "200": { @@ -26985,7 +26986,7 @@ "type": "string" }, "example": { - "reader.zoom": 150, + "library.show_deleted_books": true, "ui.theme": "dark" } } @@ -45777,7 +45778,7 @@ "properties": { "key": { "type": "string", - "description": "The preference key (e.g., \"ui.theme\", \"reader.zoom\")", + "description": "The preference key (e.g., \"ui.theme\", \"library.show_deleted_books\")", "example": "ui.theme" }, "updatedAt": { diff --git a/web/openapi.json b/web/openapi.json index 55aa7d19..86366514 100644 --- a/web/openapi.json +++ b/web/openapi.json @@ -17853,6 +17853,7 @@ "User Preferences" ], "summary": "Get all preferences for the authenticated user", + "description": "The store is an open `key -> JSON` map: any syntactically valid key is\naccepted, and this endpoint returns whatever the user has set. The list\nbelow is not a whitelist, it is the set Codex's own clients read and write.\nA client that wants a user's settings to follow them between devices has to\nuse these exact keys and value shapes.\n\n| Key | Value |\n| --- | --- |\n| `ui.theme` | `\"light\"`, `\"dark\"`, or `\"system\"` (default `\"system\"`) |\n| `library.show_deleted_books` | boolean (default `false`) |\n| `want_to_read.sort` | `\"newest\"`, `\"oldest\"`, or `\"custom\"` (default `\"newest\"`) |\n| `release_tracking.muted_series_ids` | array of series id strings (default `[]`) |\n\nKeys are `snake_case`, matching the server settings store rather than the\ncamelCase of the JSON fields around them: a key is a value in a database\ncolumn, not a field name.\n\nReader settings are deliberately not here. Fit mode, reading direction,\nzoom, and per-series reader overrides are device-local state, held by each\nclient and never synced, because a phone and a desktop legitimately want\ndifferent ones.", "operationId": "get_all_preferences", "responses": { "200": { @@ -26985,7 +26986,7 @@ "type": "string" }, "example": { - "reader.zoom": 150, + "library.show_deleted_books": true, "ui.theme": "dark" } } @@ -45777,7 +45778,7 @@ "properties": { "key": { "type": "string", - "description": "The preference key (e.g., \"ui.theme\", \"reader.zoom\")", + "description": "The preference key (e.g., \"ui.theme\", \"library.show_deleted_books\")", "example": "ui.theme" }, "updatedAt": { diff --git a/web/src/mocks/handlers/coverage.test.ts b/web/src/mocks/handlers/coverage.test.ts index db298a7b..695ef366 100644 --- a/web/src/mocks/handlers/coverage.test.ts +++ b/web/src/mocks/handlers/coverage.test.ts @@ -1,5 +1,7 @@ import { describe, expect, it } from "vitest"; +import { PREFERENCE_DEFAULTS } from "@/types/preferences"; import { handlers } from "./index"; +import { mockUserPreferences } from "./users"; /** * The mock handlers back `make frontend-mock`, the dev workflow that runs the @@ -43,4 +45,21 @@ describe("mock handler coverage", () => { expect(pattern).toBeGreaterThanOrEqual(0); expect(literal).toBeLessThan(pattern); }); + + /** + * The mocks used to serve `reader.fitMode`, `reader.readingDirection`, + * `library.defaultView`, `library.itemsPerPage`, `notifications.enabled`, and + * a bare `theme`. Not one of those keys exists. They read as documentation of + * what the preference store holds, and a server-side plan was written from + * them describing a reader-settings sync feature that does not exist. + * + * The preference keys are a cross-client contract: the PWA and the native + * client have to write the same ones or a user's settings stop following them + * between devices. `PREFERENCE_DEFAULTS` is where that set is declared, so the + * mocks answer to it rather than inventing their own. + */ + it("mocks exactly the preference keys the app actually uses", () => { + const mocked = mockUserPreferences.map((pref) => pref.key).sort(); + expect(mocked).toEqual(Object.keys(PREFERENCE_DEFAULTS).sort()); + }); }); diff --git a/web/src/mocks/handlers/users.ts b/web/src/mocks/handlers/users.ts index 2b33c09a..5fa60a4d 100644 --- a/web/src/mocks/handlers/users.ts +++ b/web/src/mocks/handlers/users.ts @@ -28,28 +28,40 @@ const mockUsers = [ ...createList(() => createUser(), 7), ]; -// Mock user preferences -const mockUserPreferences: Array<{ +/** + * Mock user preferences. + * + * The keys must be exactly those in `PREFERENCE_DEFAULTS`, and a test in + * `coverage.test.ts` enforces it. Preference keys are a cross-client contract: + * the PWA and the native client have to write the same ones or a user's + * settings stop following them between devices, and a fixture serving invented + * keys reads as documentation of a store that holds something it does not. + * + * Reader settings are deliberately absent. They are device-local state in + * `readerStore`, persisted to localStorage and never synced, so they are not + * preferences and never reach this endpoint. + * + * Values are chosen to exercise the UI rather than to match the defaults. + */ +export const mockUserPreferences: Array<{ key: string; value: unknown; updatedAt: string; }> = [ - { key: "theme", value: "system", updatedAt: "2024-01-01T00:00:00Z" }, + { key: "ui.theme", value: "dark", updatedAt: "2024-01-01T00:00:00Z" }, { - key: "library.defaultView", - value: "grid", + key: "library.show_deleted_books", + value: false, updatedAt: "2024-01-01T00:00:00Z", }, - { key: "library.itemsPerPage", value: 20, updatedAt: "2024-01-01T00:00:00Z" }, { - key: "reader.readingDirection", - value: "ltr", + key: "want_to_read.sort", + value: "custom", updatedAt: "2024-01-01T00:00:00Z", }, - { key: "reader.fitMode", value: "width", updatedAt: "2024-01-01T00:00:00Z" }, { - key: "notifications.enabled", - value: true, + key: "release_tracking.muted_series_ids", + value: ["series-3"], updatedAt: "2024-01-01T00:00:00Z", }, ]; From 899d6623b4edabe7f18aa321b9e280129649a512 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sat, 22 Aug 2026 22:22:30 -0700 Subject: [PATCH 4/5] ci: fail a PR that breaks the API contract, and check the spec is current first The existing openapi_spec.rs invariants check the document's shape: no unparameterised generics, no orphaned components, path parameters matching their templates. They say nothing about its evolution. An operation removed, renamed, or moved passes all of them, and the generated Swift client is the only thing that notices, at re-vendor time, in another repo. Add an oasdiff gate on pull requests, comparing against the base branch and failing on ERR. It knows 219 breaking-change rules, covering cases a short hand-written check would miss: a response property changing type, a response code disappearing, an array becoming a scalar, a format narrowing. Its base accepts a git ref, so there is no baseline file to keep current. The gate is skipped when a PR carries the `breaking-change` label, so breaking the contract stays a deliberate, visible act rather than a red check people learn to ignore. `review` is off, because the action otherwise uploads an encrypted spec comparison to a third party by default. The freshness check in front of it is what makes any of this mean anything. web/openapi.json is committed and regenerated by a pre-commit hook, and that hook was the only thing enforcing it, so a commit made with --no-verify or from a clone without hooks installed leaves it stale. oasdiff would then compare two identical stale files, pass, and let the break through. Nothing is committed by CI: it regenerates to a temp file, fails, and says to run make openapi-all. make release-prepare now also reports whether the bump matches what changed since the last tag, naming the affected endpoints when breaking changes land in a non-major release. It never fails: by then the work is merged, and a breaking change is sometimes exactly what was intended. It only makes the number on the tin a decision rather than a habit. Note that oasdiff compares the document rather than the server, so correcting a spec that described an endpoint wrongly reads as breaking even when no running client changes behaviour. Against real history, 2.1.0 to 2.2.0 reports 49 breaking changes across 9 endpoints, all of them this cycle's defect fixes, shipped as a minor bump with nothing to say so at the time. --- .github/workflows/ci.yml | 58 ++++++++++++++++++++++ Makefile | 4 ++ scripts/check-release-bump.sh | 92 +++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+) create mode 100755 scripts/check-release-bump.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 009d316c..550c460c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -90,6 +90,64 @@ jobs: - name: Clippy check run: cargo clippy --features rar -- -D warnings + # Guard the API contract: the committed spec must match the code, and the + # change must not break a client that generated against main. + # + # The freshness check is not optional decoration. `web/openapi.json` is a + # committed artifact regenerated by a pre-commit hook, so a commit made with + # `--no-verify`, or from a clone where `make setup-hooks` was never run, + # leaves it stale. oasdiff would then compare two identical stale files, pass, + # and let the breaking change through: the gate would be theatre. + api-contract: + name: API Contract + runs-on: ubuntu-latest + timeout-minutes: 30 + env: + SCCACHE_GHA_ENABLED: "true" + SCCACHE_GHA_VERSION: openapi + RUSTC_WRAPPER: sccache + steps: + - uses: actions/checkout@v4 + - name: Install mold linker + run: sudo apt-get update && sudo apt-get install -y mold + - name: Install Rust + uses: dtolnay/rust-toolchain@master + with: + toolchain: 1.95.0 + - name: Setup sccache + uses: mozilla-actions/sccache-action@v0.0.9 + + - name: Check the committed spec matches the code + run: | + cargo run -- openapi --output /tmp/openapi-fresh.json + if ! diff -q /tmp/openapi-fresh.json web/openapi.json >/dev/null; then + echo "::error::web/openapi.json is stale. Run 'make openapi-all' and commit the result." + diff -u web/openapi.json /tmp/openapi-fresh.json | head -60 || true + exit 1 + fi + if ! diff -q web/openapi.json docs/api/openapi.json >/dev/null; then + echo "::error::docs/api/openapi.json differs from web/openapi.json. Run 'make openapi'." + exit 1 + fi + echo "OpenAPI spec is in sync with the backend." + + - name: Fetch the base branch + run: git fetch --depth=1 origin ${{ github.base_ref }} + + # Skipped when the PR is labelled `breaking-change`, so breaking the API + # is a deliberate, visible act rather than a red check people learn to + # ignore. The label is the record that it was intended. + - name: Check for breaking API changes + if: ${{ !contains(github.event.pull_request.labels.*.name, 'breaking-change') }} + uses: oasdiff/oasdiff-action/breaking@v0 + with: + base: origin/${{ github.base_ref }}:web/openapi.json + revision: HEAD:web/openapi.json + fail-on: ERR + # The spec is public, but uploading it to a third party should be a + # decision rather than a default. + review: false + # Run frontend tests and build frontend: name: Frontend diff --git a/Makefile b/Makefile index d89e7659..929064ba 100644 --- a/Makefile +++ b/Makefile @@ -597,6 +597,10 @@ release-prepare: ## Prepare a release (usage: make release-prepare VERSION=1.0.0 @$(MAKE) openapi-all @echo "$(GREEN)✓$(NC) Regenerated OpenAPI spec and TypeScript types" + @# Check the bump against what actually changed in the API + @echo "$(YELLOW)Checking the version bump against the API contract...$(NC)"; + @./scripts/check-release-bump.sh $(VERSION) || true + @# Generate changelog (skip if already modified) @echo "$(YELLOW)Generating CHANGELOG.md...$(NC)"; @if git diff --quiet CHANGELOG.md 2>/dev/null && git diff --cached --quiet CHANGELOG.md 2>/dev/null; then \ diff --git a/scripts/check-release-bump.sh b/scripts/check-release-bump.sh new file mode 100755 index 00000000..ca6a969c --- /dev/null +++ b/scripts/check-release-bump.sh @@ -0,0 +1,92 @@ +#!/usr/bin/env bash +# Advisory: does this version bump match what actually changed in the API? +# +# Compares the freshly regenerated spec against the previous tag's and reports +# whether the bump is consistent with the changes found. +# +# Deliberately never fails. By the time this runs the work is merged, and a +# breaking change is sometimes exactly what was intended. The point is that the +# number on the tin is a decision rather than a habit. +# +# Note that oasdiff compares the *document*, not the server. Correcting a spec +# that described an endpoint wrongly reads as a breaking change here even though +# no running client changes behaviour, because a client generated from the old +# document really would see something different. + +set -uo pipefail + +VERSION="${1:-}" +# Optional: compare against this ref instead of the latest tag. Only needed to +# re-check a past release, or to test this script. +BASE_REF="${2:-}" +SPEC="web/openapi.json" + +if [ -z "$VERSION" ]; then + echo "usage: $0 [base-ref]" >&2 + exit 0 +fi + +if ! command -v oasdiff >/dev/null 2>&1; then + echo " oasdiff not installed, skipping the API bump check (brew install oasdiff)" + exit 0 +fi + +PREV_TAG="${BASE_REF:-$(git describe --tags --abbrev=0 2>/dev/null)}" +if [ -z "$PREV_TAG" ]; then + echo " No previous tag, skipping the API bump check" + exit 0 +fi + +if ! git cat-file -e "${PREV_TAG}:${SPEC}" 2>/dev/null; then + echo " ${PREV_TAG} has no ${SPEC}, skipping the API bump check" + exit 0 +fi + +# What kind of bump is this? +PREV_VERSION="${PREV_TAG#v}" +IFS=. read -r prev_major prev_minor prev_patch </dev/null) +ERRORS=$(printf '%s' "$REPORT" | grep -c '^error') +WARNINGS=$(printf '%s' "$REPORT" | grep -c '^warning') + +echo " ${PREV_TAG} -> v${VERSION} is a ${BUMP} bump" +echo " API contract: ${ERRORS} breaking, ${WARNINGS} warnings" + +if [ "$ERRORS" -gt 0 ] && [ "$BUMP" != "major" ]; then + echo "" + echo " ⚠ ${ERRORS} breaking changes in a ${BUMP} release." + echo " A client generated against ${PREV_TAG} may stop working." + printf '%s' "$REPORT" | grep -A1 '^error' | grep 'in API' | sed 's/^[[:space:]]*/ /' | sort -u + echo "" + echo " Intentional? Nothing to do, but say so in the release notes." + echo " Unintentional? Consider a major bump, or revert the change." +elif [ "$ERRORS" -gt 0 ]; then + echo " ✓ Breaking changes present, and this is a major bump" +elif [ "$WARNINGS" -gt 0 ] && [ "$BUMP" = "patch" ]; then + echo "" + echo " ⚠ ${WARNINGS} contract changes in a patch release." + echo " Patches should not move the API: clients decide what a server" + echo " supports from the release a feature first appeared in." +else + echo " ✓ Bump is consistent with the API changes" +fi + +echo "" +echo " Full report: oasdiff breaking ${PREV_TAG}:${SPEC} ${SPEC}" +exit 0 From 5100de6314d5263eac195166bce0baa086801b18 Mon Sep 17 00:00:00 2001 From: Sylvain Cau Date: Sat, 22 Aug 2026 22:18:55 -0700 Subject: [PATCH 5/5] ci: report API contract changes on the PR instead of failing the job A red job means "fix this before merging". A deliberate contract change is not that: the version is decided at tag time rather than per PR, so a breaking change on a feature branch is a normal event on its way to a release numbered accordingly. Failing the job would mark a routine act as a failure, and a check that is routinely red is one people learn to skim past. It would have spent its own signal within a handful of PRs, and the bypass label would have become a chore performed without reading anything. Two signals had been collapsed into one. A stale committed spec is unambiguously a mistake, since the artifact no longer matches the code and nothing downstream can be trusted until it does, so that step stays fatal. A contract change is a judgement call that needs to be visible where the work is happening, which is not the same as needing to block anything. The findings now land as a pull-request comment, edited in place so the PR shows current state rather than a thread of stale reports, and rewritten to say the contract is unchanged once the findings are gone. A lingering "3 breaking changes" on a PR that no longer has any is worse than no comment at all. The action's own commenting is disabled with an empty github-token: it posts a link to the oasdiff.com upload rather than the findings, and that upload is off. --- .github/workflows/ci.yml | 76 +++++++++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 550c460c..ee713c6f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,6 +6,8 @@ name: CI permissions: contents: read packages: write + # The API Contract job posts its report as a pull-request comment. + pull-requests: write on: pull_request: @@ -90,8 +92,8 @@ jobs: - name: Clippy check run: cargo clippy --features rar -- -D warnings - # Guard the API contract: the committed spec must match the code, and the - # change must not break a client that generated against main. + # Guard the API contract: the committed spec must match the code, and any + # change to what clients depend on is surfaced on the run. # # The freshness check is not optional decoration. `web/openapi.json` is a # committed artifact regenerated by a pre-commit hook, so a commit made with @@ -134,19 +136,77 @@ jobs: - name: Fetch the base branch run: git fetch --depth=1 origin ${{ github.base_ref }} - # Skipped when the PR is labelled `breaking-change`, so breaking the API - # is a deliberate, visible act rather than a red check people learn to - # ignore. The label is the record that it was intended. - - name: Check for breaking API changes - if: ${{ !contains(github.event.pull_request.labels.*.name, 'breaking-change') }} + # Reports rather than fails, and the distinction is deliberate. + # + # A red job means "fix this before merging". A contract change is not + # that: breaking changes are a normal mid-cycle event here, because the + # version is decided at tag time rather than per PR. Failing the job would + # mark a routine, intentional act as a failure, and a check that is + # routinely red is one people learn to skim past, which costs more than it + # catches. + # + # `continue-on-error` keeps the job green while still flagging the step, so + # the run page shows the change without claiming anything is broken. The + # decision this feeds is made by `make release-prepare`, which compares + # against the previous tag at the moment the version is actually chosen. + # + # The stale-spec check above stays fatal, because that one really is a + # mistake: the committed artifact does not match the code. + - name: Report API contract changes + id: contract + continue-on-error: true uses: oasdiff/oasdiff-action/breaking@v0 with: base: origin/${{ github.base_ref }}:web/openapi.json revision: HEAD:web/openapi.json fail-on: ERR # The spec is public, but uploading it to a third party should be a - # decision rather than a default. + # decision rather than a default. `github-token: ''` disables the + # action's own comment, which posts a link to that upload rather than + # the findings; the report is posted below instead. review: false + github-token: "" + + # One comment, edited in place, so the PR always shows the current state + # rather than a thread of stale reports. It is rewritten to say the + # contract is unchanged once the findings are gone, because a lingering + # "3 breaking changes" on a PR that no longer has any is worse than no + # comment at all. + - name: Comment the contract report + if: always() && github.event.pull_request.head.repo.full_name == github.repository + env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + REPORT: ${{ steps.contract.outputs.breaking }} + run: | + MARKER='' + + if [ -n "$REPORT" ]; then + BODY=$( + printf '%s\n### API contract changes\n\n' "$MARKER" + printf 'Compared against `%s`. These are changes a client generated from the\n' "${{ github.base_ref }}" + printf 'previous document would notice. Not a failure: breaking changes are a\n' + printf 'release-time decision, and `make release-prepare` checks the bump against\n' + printf 'them when the version is chosen.\n\n' + printf '
Report\n\n```\n' + printf '%s\n' "$REPORT" | head -c 55000 + printf '```\n\n
\n' + ) + else + BODY="${MARKER}"$'\n### API contract changes\n\nNone. Nothing a client generated from the previous document would notice.' + fi + + ID=$(gh api "repos/${REPO}/issues/${PR}/comments" --paginate \ + --jq "[.[] | select(.body | startswith(\"${MARKER}\")) | .id] | first // empty") + + if [ -n "$ID" ]; then + gh api -X PATCH "repos/${REPO}/issues/comments/${ID}" -f body="$BODY" >/dev/null + echo "Updated comment ${ID}" + else + gh api -X POST "repos/${REPO}/issues/${PR}/comments" -f body="$BODY" >/dev/null + echo "Created comment" + fi # Run frontend tests and build frontend: