Skip to content

Commit 2e32870

Browse files
pmaxhoganclaude
andcommitted
fix(m7): harden activity store + render aggregates/coded errors (M7-P2-1..P2-6)
Frontend half of the M7 codex round-1 fixes: - P2-1: request-token + filter-snapshot guard on loadInitial/loadMore/ applyFilter so a filter change mid-flight discards the stale response. - P2-2: cap the live tail to LIVE_TAIL_CAP (1000, DESIGN s8.3) in a separate liveEntries list, evicting oldest; loaded history pages are preserved. - P2-3: track desiredSubscribed so an unsubscribe before listen() resolves tears the listeners down on arrival (no leak); subscribe both activity:new and activity:lagged. - P1-1 (frontend): on activity:lagged, reconcile page 0 + dedup-merge into the live tail; re-sync total from the page total (no over-count). - P2-4: bind the event-type dropdown to the backend distinct query. - P2-5: render the DESIGN s8.3 header (bytes today/week, throughput, files by status) via Intl.NumberFormat. - P2-6: normalize IPC errors to the stable { code } shape via a new shared ipc/errors.ts#toErrorCode (promoted from setup.ts) and render via t(`errors.${code}.long`); store exposes errorCode. Contracts kept in sync (ActivitySummaryDto/FileStatusCountDto, command wrappers, onActivityLagged). New en-US keys activity.summary.*/status.*. 8 new store tests. CODEX_NOTES updated with the M7 round-1 fix record. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CyiRqk2DVwmJjEu5gcD1m
1 parent 7930b0c commit 2e32870

10 files changed

Lines changed: 750 additions & 94 deletions

File tree

design/CODEX_NOTES.md

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -915,3 +915,79 @@ unchanged), `pnpm lint/test:unit (54 passed, 11 new activity-store)/build`
915915
(`src-tauri/src` + orchestrator.rs/types.rs): zero `todo!`/`unimplemented!`/
916916
`unreachable!` in non-test code (the orchestrator `unimplemented!()` are
917917
pre-existing `#[cfg(test)]` Fake doubles).
918+
919+
## M7 codex review round-1 fixes (1 P1 + 6 P2)
920+
921+
The codex round-1 review (`.claude/codex-reviews/M7-20260624-103442.md`, baseline
922+
f9fb164, M7 @ 9771d53; CI + Chaos GREEN on 3 OS, vue-tsc + eslint clean) raised 1
923+
P1 + 6 P2 - all verified legitimate and all fixed. No spec deviations; the fixes
924+
are additive (two new IPC commands + one new event + store hardening).
925+
926+
- **M7-P1-1 (live tail drops events on broadcast lag).** The per-account
927+
`OrchestratorEvent` broadcast is bounded (cap 256); the event bridge previously
928+
only LOGGED `RecvError::Lagged`, so an error storm permanently dropped
929+
`activity:new` rows from the live tail (violates DESIGN s8.3 last-1000 + ROADMAP
930+
M7 <500ms). Fix: on lag the bridge emits a new typed `activity:lagged` gap
931+
signal (SPEC s11.7, `events::emit_activity_lagged`); the webview store
932+
RECONCILES by re-querying `query_activity` page 0 for the current filter and
933+
dedup-merging the rows into the live tail (the durable `activity_log` is the
934+
source of truth), so no durable row is lost. The 500ms-typical path stays
935+
event-driven via `activity:new`. The bridge's per-event decision was factored
936+
into a pure `classify_bridge_event -> BridgeAction` so the Lagged->reconcile
937+
mapping is unit-testable WITHOUT a Tauri `AppHandle` (3 new assembly tests);
938+
the store side has a lag-reconcile merge test.
939+
- **M7-P2-1 (stale-response race).** `loadInitial`/`loadMore`/`applyFilter` had no
940+
generation guard. Fix: a `requestToken` (bumped per load) + a filter snapshot;
941+
a response commits ONLY if the token + filter still match (`sameFilter`). New
942+
store test: a filter change mid-flight discards the stale response.
943+
- **M7-P2-2 (unbounded live tail).** Live events grew `entries`/`seenIds` forever.
944+
Fix: the live tail is now a SEPARATE `liveEntries` list capped to
945+
`LIVE_TAIL_CAP` (1000, DESIGN s8.3) by evicting the oldest live entry on
946+
overflow; the rendered `entries` is `liveEntries` (deduped) ++ paged
947+
`historyEntries`, so an error storm is bounded while LOADED history pages are
948+
preserved. Two new store tests (cap holds; history not evicted).
949+
- **M7-P2-3 (subscribe-before-unmount listener leak).** `subscribeLive` now tracks
950+
a `desiredSubscribed` flag; if `unsubscribeLive` runs before `listen()`
951+
resolves, the resolved unlisten fns are invoked immediately on arrival. New
952+
store test drives the unsubscribe-before-resolve race.
953+
- **M7-P2-4 (event-type filter unreachable for history).** The dropdown was
954+
derived only from loaded rows. Fix: new backend `distinct_activity_event_types`
955+
IPC + `StateRepo::distinct_activity_event_types` (`SELECT DISTINCT ... ORDER
956+
BY`); the store loads it into `eventTypeOptions` and the view binds the dropdown
957+
to it. New backend repo test (sorted-unique set).
958+
- **M7-P2-5 (missing DESIGN s8.3 header aggregates).** New backend
959+
`activity_summary` IPC + `StateRepo::activity_summary` returning bytes uploaded
960+
today / this week (summed `activity_log.bytes` over caller-supplied LOCAL day /
961+
week boundaries - so "today" honours the user's timezone with NO backend
962+
timezone crate), file count by `file_state.status`, and a recent-throughput
963+
window (bytes + window-ms, the UI derives bytes/sec). The view renders the
964+
header; bytes/rate via `Intl.NumberFormat` (DESIGN s8.7). New backend repo test
965+
(boundary-correct sums + status grouping) + the `file_state_status_str` mapping
966+
test.
967+
- **M7-P2-6 (errors rendered via `String(e)`).** Activity load + diagnostic-export
968+
errors now normalize to the stable `{ code }` shape (SPEC s24) via a new shared
969+
`ui/src/ipc/errors.ts#toErrorCode` (promoted from `stores/setup.ts`, which now
970+
imports it) and render via `t(\`errors.${code}.long\`)` (the M6 pattern). The
971+
store exposes `errorCode` (was `error`); the view localizes it. New store test:
972+
a Tauri object error surfaces its code.
973+
974+
Cross-cutting: backend/frontend contracts stayed in sync (new DTOs
975+
`ActivitySummaryDto`/`FileStatusCountDto` in both `dtos.rs` + `ipc/types.ts`; new
976+
command wrappers + the `onActivityLagged` event helper). New i18n keys:
977+
`activity.summary.*`, `activity.status.*` (en-US). Two new `sqlx::query!`
978+
(DISTINCT + the summary aggregate) regenerated the `.sqlx` offline cache (0
979+
drift). The two new `StateRepo` methods have trait DEFAULT impls (empty / zeroed)
980+
so the `#[cfg(test)]` Fake doubles compile unchanged; the SQLite repo overrides
981+
both with the real SQL. All gates green: `cargo build/clippy(-D warnings)/test
982+
--workspace` (driven-core 189 incl. 2 new + driven-app 98 incl. 4 new; e2e_fake
983+
20 pass/5 gate-skip), `build -p driven-app`, `deny check`, `fmt --check`; `pnpm
984+
install` (lockfile unchanged), `pnpm lint/test:unit (62 passed, 8 new)/build`
985+
(vue-tsc clean). Anti-fake-green stub sweep on the touched surface: zero
986+
`todo!`/`unimplemented!`/`unreachable!` in non-test code.
987+
988+
### Residual / not-fixed
989+
None. All 1 P1 + 6 P2 are fully fixed with exercising tests. One acceptable known
990+
limitation (not a review finding): the M7-P2-3 fix covers unsubscribe-before-
991+
resolve; a pathological re-subscribe DURING a not-yet-resolved subscribe could
992+
orphan the second listener set, but that path does not occur in the Activity
993+
view's mount/unmount lifecycle (V1 scope).

ui/src/__tests__/activity-store.test.ts

Lines changed: 219 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,20 +14,49 @@ vi.mock("@tauri-apps/api/core", () => ({
1414
invoke: (cmd: string, args?: unknown) => invokeMock(cmd, args),
1515
}));
1616

17-
// The live-tail seam: capture the handler `onActivityNew` registers so the test
18-
// can fire `activity:new` events on demand. `listen` returns an unlisten fn.
17+
// The live-tail seam: capture the handlers `onActivityNew` / `onActivityLagged`
18+
// register so the test can fire `activity:new` + `activity:lagged` on demand.
19+
// `listen` returns an unlisten fn. M7-P2-3: each `listen` call resolves to a
20+
// DISTINCT unlisten so the unsubscribe-before-resolve test can assert teardown.
1921
let liveHandler: ((payload: unknown) => void) | null = null;
20-
const unlistenMock = vi.fn();
22+
let laggedHandler: (() => void) | null = null;
23+
const unlistenNewMock = vi.fn();
24+
const unlistenLaggedMock = vi.fn();
25+
// Allows a test to defer `listen` resolution (the leak-on-unmount race). Each
26+
// blocked `listen` call parks its own resolver; `flushListen()` releases all.
27+
let pendingResolvers: Array<() => void> = [];
28+
let blockListen = false;
29+
function flushListen(): void {
30+
const resolvers = pendingResolvers;
31+
pendingResolvers = [];
32+
for (const r of resolvers) r();
33+
}
2134
vi.mock("@tauri-apps/api/event", () => ({
22-
listen: vi.fn((event: string, cb: (e: { payload: unknown }) => void) => {
23-
if (event === "activity:new") {
24-
liveHandler = (payload: unknown) => cb({ payload });
25-
}
26-
return Promise.resolve(unlistenMock);
27-
}),
35+
listen: vi.fn(
36+
async (event: string, cb: (e: { payload: unknown }) => void) => {
37+
if (blockListen) {
38+
await new Promise<void>((res) => {
39+
pendingResolvers.push(res);
40+
});
41+
}
42+
if (event === "activity:new") {
43+
liveHandler = (payload: unknown) => cb({ payload });
44+
return unlistenNewMock;
45+
}
46+
if (event === "activity:lagged") {
47+
laggedHandler = () => cb({ payload: null });
48+
return unlistenLaggedMock;
49+
}
50+
return vi.fn();
51+
},
52+
),
2853
}));
2954

30-
import { useActivityStore, ACTIVITY_PAGE_SIZE } from "../stores/activity";
55+
import {
56+
useActivityStore,
57+
ACTIVITY_PAGE_SIZE,
58+
LIVE_TAIL_CAP,
59+
} from "../stores/activity";
3160
import type { ActivityEntry } from "../ipc/types";
3261

3362
function makeEntry(over: Partial<ActivityEntry> = {}): ActivityEntry {
@@ -69,8 +98,12 @@ function makePage(
6998
beforeEach(() => {
7099
setActivePinia(createPinia());
71100
invokeMock.mockReset();
72-
unlistenMock.mockReset();
101+
unlistenNewMock.mockReset();
102+
unlistenLaggedMock.mockReset();
73103
liveHandler = null;
104+
laggedHandler = null;
105+
pendingResolvers = [];
106+
blockListen = false;
74107
});
75108

76109
describe("activity store: pagination", () => {
@@ -182,11 +215,101 @@ describe("activity store: live tail", () => {
182215
expect(store.entries.map((e) => e.id)).toEqual([10]);
183216
});
184217

185-
it("unsubscribeLive calls the unlisten fn", async () => {
218+
it("unsubscribeLive calls BOTH unlisten fns (new + lagged)", async () => {
186219
const store = useActivityStore();
187220
await store.subscribeLive();
188221
store.unsubscribeLive();
189-
expect(unlistenMock).toHaveBeenCalledTimes(1);
222+
expect(unlistenNewMock).toHaveBeenCalledTimes(1);
223+
expect(unlistenLaggedMock).toHaveBeenCalledTimes(1);
224+
});
225+
226+
// M7-P2-3: unsubscribe-before-resolve must not leak a listener.
227+
it("tears down listeners that resolve AFTER unsubscribe (no leak)", async () => {
228+
blockListen = true;
229+
const store = useActivityStore();
230+
// Start subscribing; `listen` is blocked, so it has not resolved yet.
231+
const pending = store.subscribeLive();
232+
// The view unmounts before the listeners resolve.
233+
store.unsubscribeLive();
234+
// Now let the blocked `listen` calls resolve.
235+
blockListen = false;
236+
flushListen();
237+
await pending;
238+
// Both resolved unlisten fns were invoked immediately on arrival.
239+
expect(unlistenNewMock).toHaveBeenCalledTimes(1);
240+
expect(unlistenLaggedMock).toHaveBeenCalledTimes(1);
241+
});
242+
});
243+
244+
describe("activity store: lag reconcile (M7-P1-1)", () => {
245+
it("activity:lagged re-queries page 0 and merges dropped rows without duplicates", async () => {
246+
// Initial page 0 has rows 5 and 4.
247+
invokeMock.mockResolvedValueOnce(
248+
makePage([makeEntry({ id: 5, ts: 500 }), makeEntry({ id: 4, ts: 400 })], 0, 2),
249+
);
250+
const store = useActivityStore();
251+
await store.subscribeLive();
252+
await store.loadInitial();
253+
expect(store.entries.map((e) => e.id)).toEqual([5, 4]);
254+
255+
// A burst happened and the live broadcast lagged: the durable log now also
256+
// has rows 7 and 6 (dropped from the live tail). The reconcile re-query
257+
// returns the newest page including the already-present 5 + the new 7, 6.
258+
invokeMock.mockResolvedValueOnce(
259+
makePage(
260+
[
261+
makeEntry({ id: 7, ts: 700 }),
262+
makeEntry({ id: 6, ts: 600 }),
263+
makeEntry({ id: 5, ts: 500 }),
264+
],
265+
0,
266+
4,
267+
),
268+
);
269+
laggedHandler?.();
270+
// Let the async reconcile settle.
271+
await Promise.resolve();
272+
await Promise.resolve();
273+
274+
const ids = store.entries.map((e) => e.id);
275+
// No duplicate of id 5; the dropped 7 + 6 are recovered, newest-first.
276+
expect(ids).toEqual([7, 6, 5, 4]);
277+
expect(ids.filter((i) => i === 5)).toHaveLength(1);
278+
});
279+
});
280+
281+
describe("activity store: live-tail cap (M7-P2-2)", () => {
282+
it("caps the live tail to LIVE_TAIL_CAP, evicting oldest live events", async () => {
283+
invokeMock.mockResolvedValueOnce(makePage([], 0, 0));
284+
const store = useActivityStore();
285+
await store.subscribeLive();
286+
await store.loadInitial();
287+
288+
// Push CAP + 50 live events (ids 1..CAP+50, ascending ts).
289+
const overflow = 50;
290+
for (let i = 1; i <= LIVE_TAIL_CAP + overflow; i++) {
291+
liveHandler?.(makeEntry({ id: i, ts: i }));
292+
}
293+
// The store is bounded to the cap (oldest live entries evicted).
294+
expect(store.entries).toHaveLength(LIVE_TAIL_CAP);
295+
// Newest is the last pushed; the oldest retained is id overflow+1.
296+
expect(store.entries[0].id).toBe(LIVE_TAIL_CAP + overflow);
297+
expect(store.entries[store.entries.length - 1].id).toBe(overflow + 1);
298+
});
299+
300+
it("does NOT evict explicitly loaded history pages", async () => {
301+
// One history row (id 1). Then flood the live tail past the cap.
302+
invokeMock.mockResolvedValueOnce(makePage([makeEntry({ id: 1, ts: 1 })], 0, 1));
303+
const store = useActivityStore();
304+
await store.subscribeLive();
305+
await store.loadInitial();
306+
307+
for (let i = 2; i <= LIVE_TAIL_CAP + 100; i++) {
308+
liveHandler?.(makeEntry({ id: i, ts: i }));
309+
}
310+
// Live tail capped at CAP, but the loaded history row survives at the tail.
311+
expect(store.entries.length).toBe(LIVE_TAIL_CAP + 1);
312+
expect(store.entries[store.entries.length - 1].id).toBe(1);
190313
});
191314
});
192315

@@ -221,14 +344,95 @@ describe("activity store: empty state", () => {
221344
await store.loadInitial();
222345
expect(store.entries).toHaveLength(0);
223346
expect(store.isEmpty).toBe(true);
224-
expect(store.error).toBeNull();
347+
expect(store.errorCode).toBeNull();
225348
});
226349

227350
it("isEmpty is false when an error occurred", async () => {
228351
invokeMock.mockRejectedValueOnce(new Error("db locked"));
229352
const store = useActivityStore();
230353
await store.loadInitial();
231-
expect(store.error).toContain("db locked");
354+
// M7-P2-6: a plain Error (no `.code`) normalizes to internal.bug.
355+
expect(store.errorCode).toBe("internal.bug");
232356
expect(store.isEmpty).toBe(false);
233357
});
234358
});
359+
360+
describe("activity store: coded errors (M7-P2-6)", () => {
361+
it("surfaces the stable SPEC s24 code from a Tauri object error", async () => {
362+
invokeMock.mockRejectedValueOnce({
363+
code: "state.db_locked",
364+
message: "Driven's database is briefly locked",
365+
});
366+
const store = useActivityStore();
367+
await store.loadInitial();
368+
expect(store.errorCode).toBe("state.db_locked");
369+
});
370+
});
371+
372+
describe("activity store: request token (M7-P2-1)", () => {
373+
it("discards a stale page response after the filter changed mid-flight", async () => {
374+
const store = useActivityStore();
375+
await store.subscribeLive();
376+
377+
// First load (default filter) is slow to resolve; capture its resolver.
378+
let resolveFirst: (v: unknown) => void = () => {};
379+
invokeMock.mockImplementationOnce(
380+
() =>
381+
new Promise((res) => {
382+
resolveFirst = res;
383+
}),
384+
);
385+
const firstLoad = store.loadInitial();
386+
387+
// While in flight, the user applies an error-only filter, which re-queries.
388+
invokeMock.mockResolvedValueOnce(
389+
makePage([makeEntry({ id: 99, level: "error" })], 0, 1),
390+
);
391+
await store.applyFilter({ minLevel: "error" });
392+
expect(store.entries.map((e) => e.id)).toEqual([99]);
393+
394+
// Now the STALE first response (default-filter rows) resolves - it must be
395+
// discarded, not appended over the current filtered result.
396+
resolveFirst(makePage([makeEntry({ id: 1 }), makeEntry({ id: 2 })], 0, 250));
397+
await firstLoad;
398+
399+
expect(store.entries.map((e) => e.id)).toEqual([99]);
400+
expect(store.total).toBe(1);
401+
expect(store.loadedPage).toBe(0);
402+
});
403+
});
404+
405+
describe("activity store: backend facets + summary (M7-P2-4, P2-5)", () => {
406+
it("loadEventTypeOptions populates the dropdown source from the backend", async () => {
407+
invokeMock.mockResolvedValueOnce(["paused", "scan_done", "upload_done"]);
408+
const store = useActivityStore();
409+
await store.loadEventTypeOptions();
410+
expect(invokeMock).toHaveBeenCalledWith(
411+
"distinct_activity_event_types",
412+
undefined,
413+
);
414+
expect(store.eventTypeOptions).toEqual([
415+
"paused",
416+
"scan_done",
417+
"upload_done",
418+
]);
419+
});
420+
421+
it("loadSummary stores the header aggregates", async () => {
422+
const summary = {
423+
bytesToday: 1024,
424+
bytesWeek: 4096,
425+
fileStatusCounts: [{ status: "synced", count: 3 }],
426+
throughputWindowBytes: 512,
427+
throughputWindowMs: 60000,
428+
};
429+
invokeMock.mockResolvedValueOnce(summary);
430+
const store = useActivityStore();
431+
await store.loadSummary();
432+
expect(invokeMock).toHaveBeenCalledWith(
433+
"activity_summary",
434+
expect.objectContaining({ throughputWindowMs: 60000 }),
435+
);
436+
expect(store.summary).toEqual(summary);
437+
});
438+
});

ui/src/ipc/commands.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type {
1010
AccountDto,
1111
ActivityFilterDto,
1212
ActivityPageDto,
13+
ActivitySummaryDto,
1314
AddAccountWizardSessionId,
1415
AddSourceRequest,
1516
AddSourceResult,
@@ -188,3 +189,26 @@ export function queryActivity(
188189
export function clearActivityOlderThan(beforeTs: number): Promise<number> {
189190
return invoke("clear_activity_older_than", { beforeTs });
190191
}
192+
193+
/** The DISTINCT set of activity event types in the durable log, sorted (M7-P2-4).
194+
* Backs the event-type filter dropdown so the user can filter for a type present
195+
* in history but not in the currently-loaded rows. */
196+
export function distinctActivityEventTypes(): Promise<string[]> {
197+
return invoke("distinct_activity_event_types");
198+
}
199+
200+
/** The Activity dashboard header aggregates (M7-P2-5; DESIGN s8.3). The day /
201+
* week boundaries are computed by the caller from the LOCAL `Date` (so the day
202+
* boundary honours the user's timezone); the backend derives the throughput
203+
* window start from `now - throughputWindowMs`. */
204+
export function activitySummary(
205+
dayStartMs: number,
206+
weekStartMs: number,
207+
throughputWindowMs: number,
208+
): Promise<ActivitySummaryDto> {
209+
return invoke("activity_summary", {
210+
dayStartMs,
211+
weekStartMs,
212+
throughputWindowMs,
213+
});
214+
}

0 commit comments

Comments
 (0)