refactor(asv3): ChatThreadsRepository port + domain VO tests (WP-14)#408
refactor(asv3): ChatThreadsRepository port + domain VO tests (WP-14)#408Luis85 wants to merge 5 commits into
Conversation
Lift chat-threads persistence out of the plugin-layer side-channel
(`SpecoratorPlugin.scheduleChatThreadsPersistence`) into a narrow
`ChatThreadsRepositoryPort` (ADR-008). Three runtime concerns split:
- `src/domain/ports/ChatThreadsRepositoryPort.ts` — pure two-method
interface (`load` / `save`); no Obsidian or plugin types leak.
- `src/infrastructure/chat/chatThreadsCodec.ts` — codec relocated from
`src/plugin/chatThreadsPersistence.ts` (deleted); pure functions only.
- `src/infrastructure/obsidian/ObsidianChatThreadsRepository.ts` —
production adapter. Owns the 1 s trailing-edge debounce, the
tail-chained flush queue (Codex P1, PR #350), and `flushPending()` for
the Obsidian `onunload()` drain (Codex P1, PR #346).
- `src/infrastructure/mock/MockChatThreadsRepository.ts` — in-memory
adapter for tests + standalone UI; no debounce (test determinism).
Views consume the port directly: `SpecoratorView.onOpen` and
`AgentSidepanelView.onOpen` now `await repo.load()` to seed the threads
store, then forward Pinia mutations to `repo.save()`. The port is
provided under a new `CHAT_THREADS_REPO` InjectionKey; consumers reach
it via the per-port composable `useChatThreadsRepo()`. The Pinia
threads store stays decoupled from persistence (Arch review #4).
main.ts:
- removes `scheduleChatThreadsPersistence`, `_flushChatThreads`,
`_chatThreadsFlushTimer`, `_pendingChatThreadsSnapshot`,
`_chatThreadsFlushQueue`, `_CHAT_THREADS_FLUSH_DEBOUNCE_MS`,
`_initialChatThreads`, and `getInitialChatThreads()`;
- constructs `ObsidianChatThreadsRepository` in `loadSettings()`
(passes `loadData`/`saveData` and `activeWindow.{set,clear}Timeout`);
- `onunload()` drains via `repo.flushPending()`.
Net main.ts drops ~85 raw LOC (662 → 577); effective code 387 → 359.
Wave 2's WP-16 will continue splitting main.ts; this WP's touch is
mechanical so WP-16 can rebase cleanly.
Domain VO coverage lift (audit gap):
- `tests/domain/chat/SessionId.test.ts` — brand semantics, JSON
round-trip, type-level invariants via `expectTypeOf`.
- `tests/domain/chat/ChatThreadRecord.test.ts` — type-level shape
pinning for the seven SPEC §2.2 fields; transport literal pinned
`'api-key' | 'subscription'` (degraded not persisted).
Test moves:
- `tests/plugin/chatThreadsPersistence.test.ts` →
`tests/infrastructure/chat/chatThreadsCodec.test.ts` (import paths
updated; assertions preserved line-for-line).
- `tests/plugin/main.chat-threads-flush.test.ts` deleted; the covered
behaviours moved into the new adapter tests under
`tests/infrastructure/obsidian/ObsidianChatThreadsRepository.test.ts`.
- New `tests/infrastructure/mock/MockChatThreadsRepository.test.ts`
covers load/save round-trip, initial seeds (Map and Array), and
defensive copies on `load()` / `snapshot()`.
- `tests/__fakes__/fake-ports.ts` now exposes
`chatThreadsRepo: MockChatThreadsRepository`.
Coverage post-change on the relocated codec: 98.33% statements /
96.15% branches / 100% functions / 98.14% lines (above the brief's
90% requirement). Overall project thresholds intact at
93.53 / 87.6 / 92.3 / 94.62 (gate 80/70/80/80).
Satisfies REQ-ASM-037, SPEC-ASM-001 §9.3, ADR-0031.
https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ff1f12f15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loadData: () => this.loadData(), | ||
| saveData: (data) => this.saveData(data), | ||
| setActiveTimeout: (cb, ms) => activeWindow.setTimeout(cb, ms), |
There was a problem hiding this comment.
The repository host wiring saves chat-thread updates straight to disk but never updates SpecoratorPlugin._storedData, while updateSettings/updateModuleSettings still persist from that in-memory cache. After any chat activity, a later settings save can therefore write an older specorator.chatThreads snapshot back to data.json, silently discarding newer threads. This regression is introduced by moving persistence out of main.ts without mirroring repo writes into _storedData.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2e8473f.
Confirmed the data-loss race: adapter flushed specorator.chatThreads to disk but _storedData was untouched, so a subsequent updateSettings/updateModuleSettings would saveData(this._storedData) and clobber the just-persisted threads.
Fix. ObsidianChatThreadsRepository's constructor now accepts an optional write-through hook:
onChatThreadsPersisted?: (chatThreads: Record<string, unknown>) => void_flushChatThreads invokes the hook after host.saveData() resolves (so a rejected disk write can never poison the cache; verified by a dedicated test that asserts the hook is not called on rejection). In main.ts:420-, the closure mirrors the encoded blob into this._storedData.specorator.chatThreads, preserving sibling keys (locale, claudeCliPath, etc.).
The port surface stays at load()/save() — the closure is private to the production wiring, so ADR-008 and ADR-001 are intact.
Tests. New describe block onChatThreadsPersisted hook (Codex P1) in tests/infrastructure/obsidian/ObsidianChatThreadsRepository.test.ts with four cases: a no-hook reproducer that asserts the data-loss precondition still holds today, the success path mirroring the snapshot into a host cache, the rejection guard, and a save→reopen→flush end-to-end sequence covering both this fix and the P1.2 fix on the sibling thread.
Suite count 12→19; project total 1882→1889. Full pre-PR gate green.
Generated by Claude Code
| async load(): Promise<ReadonlyMap<string, ChatThreadRecord>> { | ||
| const stored = (await this.host.loadData()) as Record<string, unknown> | null | ||
| if (stored === null || typeof stored !== 'object') return new Map() | ||
| const specoratorBlob = (stored.specorator ?? {}) as Record<string, unknown> | ||
| const chatThreadsBlob = specoratorBlob.chatThreads |
There was a problem hiding this comment.
load() always reads from persisted plugin data and ignores _pendingSnapshot, so reopening a chat view within the debounce window rehydrates stale threads and drops the just-created ones from the UI state. If the user sends another message before the old debounce flush is reflected in the store, the next save() can persist the stale map and lose the recent thread history. The previous implementation avoided this by exposing the latest scheduled snapshot for rehydrate-on-reopen.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2e8473f.
Confirmed: load() always read from disk, so reopening the chat view inside the 1 s debounce window rehydrated the pre-save threads and the store's next save() would persist that stale map on top of the in-flight new thread.
Fix. load() now follows an explicit precedence:
async load(): Promise<ReadonlyMap<string, ChatThreadRecord>> {
if (this._pendingSnapshot !== null) {
return new Map(this._pendingSnapshot) // in-flight wins
}
// ...fall through to decoding the disk blob
}The defensive copy prevents caller-side mutation from leaking into the snapshot that the queued debounce will write. _pendingSnapshot is cleared inside the debounce callback before _flushChatThreads(snapshot) runs, so once the disk write begins load() falls through to disk as before — no stale-snapshot lingering.
Precedence is documented in the load() JSDoc.
Tests. New describe block load — pending-snapshot precedence (Codex P1) in tests/infrastructure/obsidian/ObsidianChatThreadsRepository.test.ts:
- Returns the in-flight snapshot while a debounced write is pending (the bug reproducer — fails before the fix).
- Reverts to disk after the pending snapshot has been flushed.
- Returns a defensive copy (caller-side
.delete()does not corrupt the queued write).
Plus the cross-cutting end-to-end: pending-snapshot precedence + cache sync test under the P1.1 describe block that walks save→reopen→flush→settings-save and asserts the just-created thread survives every step.
Suite count 12→19; project total 1882→1889. Full pre-PR gate green.
Generated by Claude Code
Two real data-loss bugs surfaced by Codex on PR #408 are addressed in this commit. P1.1 — `_storedData` cache desync (`src/plugin/main.ts:424`) ============================================================= Reproduction: user sends a chat message; the adapter flushes `specorator.chatThreads` to disk after the 1 s debounce. The plugin's in-memory `_storedData` cache is NEVER updated. Later, the user toggles any setting -> `updateSettings` / `updateModuleSettings` call `saveData(this._storedData)`, which re-emits the stale (pre-chat) `specorator.chatThreads` snapshot and silently destroys the recent thread. Fix: `ObsidianChatThreadsRepository` constructor now accepts an `onChatThreadsPersisted: (chatThreads) => void` hook. The adapter invokes it AFTER the disk write resolves (so a failed `saveData` cannot poison the cache). In `main.ts`, the closure mirrors the encoded blob into `this._storedData.specorator.chatThreads`, preserving sibling keys. The port surface is unchanged; the closure is private to the production wiring and stays within ADR-008 / ADR-001. P1.2 — stale `load()` inside the debounce (`ObsidianChatThreadsRepository.ts:74`) =================================================================================== Reproduction: user sends a message -> `save()` schedules a debounced flush; before the timer fires, the user reopens the chat view -> `load()` reads the stale disk blob and the store rehydrates without the just-created thread. If the user sends another message before the old debounce flushes, the next `save()` persists the stale map on top of the in-flight new thread, destroying it. Fix: `load()` now returns `new Map(this._pendingSnapshot)` when a debounced write is in flight; only falls through to the disk decode when no snapshot is pending. Returns a defensive copy so caller-side mutation cannot leak into the queued write. Precedence documented in the JSDoc and re-asserted by an end-to-end test. Tests ===== `tests/infrastructure/obsidian/ObsidianChatThreadsRepository.test.ts`: new describe blocks `load — pending-snapshot precedence (Codex P1)` (3 cases) and `onChatThreadsPersisted hook (Codex P1)` (4 cases, including a regression reproducer and a save-reopen-flush sequence). Suite count 12 -> 19; project total 1882 -> 1889. Pre-PR gate: `npm audit`, typecheck, lint (0 errors, 24 pre-existing warnings), test, build, build:web, docs:api all green. https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e8473f1b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const stored = ((await this.host.loadData()) as Record<string, unknown> | null) ?? {} | ||
| const currentSpecorator = (stored.specorator ?? {}) as Record<string, unknown> | ||
| const nextSpecorator = { ...currentSpecorator, chatThreads: encoded } | ||
| const nextStored: Record<string, unknown> = { ...stored, specorator: nextSpecorator } | ||
| await this.host.saveData(nextStored) |
There was a problem hiding this comment.
_flushChatThreads rebuilds specorator from await host.loadData() and immediately writes it back, but settings/module saves in src/plugin/main.ts (updateSettings / updateModuleSettings) also call saveData independently and are not serialized with this queue. If a settings save is in flight when the debounce fires, loadData() can return the pre-settings blob and this flush overwrites the newer settings on disk (or vice versa), causing silent rollback of either settings or chat state depending on write order. This regression is introduced by moving persistence to the repository without coordinating cross-writer ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right — this is the symmetric race to the P1.1 fix and equally real. Round-1 closed the write side (mirror every successful chat-threads flush into _storedData so later updateSettings / updateModuleSettings can't re-emit the stale snapshot). But _flushChatThreads still pulled sibling keys from await host.loadData(). If a settings save had mutated _storedData but its own saveData was still in flight, loadData() returned the pre-settings disk blob → the chat-threads flush merged its new chatThreads into that stale settings snapshot → wrote back → silently rolled back the in-flight settings change (or vice versa, depending on write order).
Fixed in cf66087 with a read-through closure symmetric to the existing write-through one. ObsidianChatThreadsRepository now accepts a new optional constructor option readHostData?: () => Record<string, unknown> | null | undefined. _flushChatThreads prefers readHostData() over host.loadData() when the closure is provided and returns a non-null object. main.ts wires readHostData: () => this._storedData, so both writers (settings updates and the chat-threads flush) now share _storedData as the single source of truth. The worst-case race degrades to "two writes hit disk in unpredictable order with the same up-to-date payload" — convergent, not destructive. The closure falls back to loadData() on null/undefined/non-object so the existing bare-host test paths and uninitialised-cache scenarios still work.
New tests under readHostData hook (Codex P1 round-2) cover (a) the race reproducer — mutate storedData.specorator.someSetting = 'new-value' directly mid-flight, trigger the chat-threads debounce, assert the writeback preserves someSetting === 'new-value' and that loadData was never called; (b) the no-closure fallback regression guard; (c) the null-return defensive fallback. Pre-PR gate green: 1892/1892 tests (was 1889; +3), 0 audit findings, typecheck/lint/build/build:web/docs:api all clean.
Generated by Claude Code
…ex P1 round-2) The symmetric race to the P1.1 fix: round-1 mirrored every successful chat-threads disk write into `SpecoratorPlugin._storedData` so settings saves could no longer roll back chat history. But the *read* side of `_flushChatThreads` still pulled sibling keys from `await host.loadData()`. If an `updateSettings` / `updateModuleSettings` had mutated `_storedData` but its `saveData` was still in flight, `loadData()` returned the pre-settings disk blob — the chat-threads flush merged its new `chatThreads` into that stale snapshot, wrote back, and silently rolled back the in-flight settings change (or vice versa, depending on write order). Fix: read-through closure symmetric to the existing write-through one. `ObsidianChatThreadsRepository` accepts a new optional `readHostData?: () => Record<string, unknown> | null | undefined` and prefers it over `host.loadData()` at flush time. `main.ts` wires `readHostData: () => this._storedData`, so both writers share one source of truth. The worst-case race degrades to "two writes hit disk in unpredictable order with the same up-to-date payload" — convergent, not destructive. The closure falls back to `loadData()` when it returns null/undefined/non-object, keeping the existing bare-host test paths working. Tests (×3 new, 19 → 22 in the file, 1889 → 1892 overall): - in-flight settings mutation survives a chat-threads flush, and the closure short-circuits `loadData` entirely - no-closure fallback regression guard - null-return defensive fallback https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf6608729d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this._flushTimer = null | ||
| this._pendingSnapshot = null | ||
| // Serialise via the tail-chained queue so older snapshots can never | ||
| // resolve after newer ones (Codex P1, PR #350). `.catch(() => undefined)` | ||
| // keeps the chain alive past a transient saveData failure. | ||
| this._flushQueue = this._flushQueue | ||
| .catch(() => undefined) | ||
| .then(() => this._flushChatThreads(snapshot)) |
There was a problem hiding this comment.
save() clears _pendingSnapshot as soon as the debounce fires, before the queued _flushChatThreads(snapshot) has actually written to disk. When saveData is slow (or an older flush is still in _flushQueue), load() falls back to stale disk state during that gap; reopening a chat view in that window rehydrates stale threads, and the next mutation can persist that stale map and overwrite newer thread history. This is a data-loss regression in the in-flight-flush path even though the debounce-window case is covered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged — this is the fourth race the WP-14 refactor introduced and your timeline reproduces it exactly. save() cleared _pendingSnapshot synchronously when the debounce timer fired, but the queued _flushChatThreads(snapshot) could still be sitting behind an older in-flight flush. During that gap load() saw _pendingSnapshot === null and fell through to disk, which was still pre-A. The view rehydrated the stale map and the next mutation persisted it on top of the just-created thread — silently clobbering newer chat history. Fixed in f2ba90f.
The fix moves the _pendingSnapshot = null assignment out of the setActiveTimeout callback and into the queued flush's .then(async () => { ... }) handler, so it runs after _flushChatThreads(snapshot) resolves — not just after the debounce fires. The clear uses identity equality (if (this._pendingSnapshot === snapshot)) so a newer save() that replaced the pending slot mid-flight is left alone; its own debounce schedules the next flush. On saveData rejection the snapshot is preserved, so flushPending()/the next save() can retry. flushPending() follows the same pattern and composes correctly when called during an in-flight flush — both flushes are awaited before resolution.
Regression coverage in tests/infrastructure/obsidian/ObsidianChatThreadsRepository.test.ts under pending snapshot held until queued flush completes (Codex P1 round-3): the race reproducer (parked saveData → load() mid-flight returns in-flight snapshot, not stale disk), the identity-equality guard against an older flush erasing a newer pending snapshot, the flushPending() two-flush drain composition, and the rejection-preservation guard. All four fail on cf66087 and pass after the fix. Pre-PR gate green (audit / typecheck / lint / 1896/1896 tests / build / build:web / docs:api).
Generated by Claude Code
…Codex P1 round-3)
`save()` previously cleared `_pendingSnapshot` as soon as the debounce
timer fired, even though the queued `_flushChatThreads(snapshot)` had
not yet won the `_flushQueue` and committed to disk. When an older
flush was still in-flight, flush(A) waited; during that window `load()`
fell through to the stale disk copy. Reopening the chat view in that
gap rehydrated the pre-save threads, and the next mutation persisted
that stale map on top of the just-created thread — silently
clobbering newer chat history. This is the fourth race in the WP-14
refactor; rounds 1-3 closed the _storedData write-through, the
pending-snapshot read precedence, and the readHostData read-through.
Fix: the `_pendingSnapshot = null` assignment now lives inside the
queued flush's `.then(async () => { ... })` handler and runs AFTER
`_flushChatThreads(snapshot)` resolves. The clear uses identity
equality (`if (this._pendingSnapshot === snapshot)`) so a newer
`save()` that replaced the pending slot mid-flight is left alone —
its own debounce schedules the next flush. On `saveData` rejection
the snapshot is preserved so the next `save()` or `flushPending()`
can retry. `flushPending()` follows the same pattern and composes
correctly with an in-flight flush.
Tests under `pending snapshot held until queued flush completes (Codex
P1 round-3)` cover the race reproducer (load returns the in-flight
snapshot, not stale disk), the identity-equality guard against an
older flush erasing a newer pending snapshot, `flushPending()`
draining both in-flight and queued flushes, and rejection-preservation.
All four fail on `cf66087` and pass after the fix (1896/1896, +4).
https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
2 participants
Summary
Lifts chat-threads persistence out of
SpecoratorPlugin.scheduleChatThreadsPersistenceinto a narrowChatThreadsRepositoryPort(ADR-008), with production / mock adapters and the codec relocated tosrc/infrastructure/chat/. Closes WP-3's carry-out (SpecoratorView/AgentSidepanelViewno longer shovel snapshots into the plugin layer) and the audit's domain-VO coverage gap onSessionId/ChatThreadRecord.src/domain/ports/ChatThreadsRepositoryPort.ts— two methods (load,save), no Obsidian / plugin types leak across. NewCHAT_THREADS_REPOInjectionKey anduseChatThreadsRepo()composable mirror the existing per-port pattern.src/plugin/chatThreadsPersistence.ts→src/infrastructure/chat/chatThreadsCodec.ts. Pure functions preserved verbatim.src/infrastructure/obsidian/ObsidianChatThreadsRepository.ts— owns the 1 s trailing-edge debounce, the tail-chained flush queue (Codex P1, PR Promote develop → demo: Agent Sidepanel MVP (Increment 1) #350), andflushPending()for theonunload()drain (Codex P1, PR feat(asm): PR-ASM-3 — session persistence + audit log (T-ASM-044..058) #346).src/infrastructure/mock/MockChatThreadsRepository.ts— in-memory, synchronous (test determinism). Exposed onfakeModulePorts().SpecoratorView/AgentSidepanelViewnowawait repo.load()to seed the threads store, then forward Pinia mutations torepo.save(). Neither view referencesplugin.scheduleChatThreadsPersistence(verified by grep).main.ts662 → 577 raw LOC (~85 LOC drop). Removed:scheduleChatThreadsPersistence,_flushChatThreads,_chatThreadsFlushTimer,_pendingChatThreadsSnapshot,_chatThreadsFlushQueue,_CHAT_THREADS_FLUSH_DEBOUNCE_MS,_initialChatThreads,getInitialChatThreads().loadSettings()constructs the repo;onunload()callsrepo.flushPending(). Coordinates with WP-16 (plugin-core split, Wave 2): the touch is mechanical so WP-16 can rebase cleanly.tests/domain/chat/SessionId.test.ts(brand semantics, JSON round-trip,expectTypeOfinvariants) andtests/domain/chat/ChatThreadRecord.test.ts(SPEC §2.2 field types, transport-literal pin, sessionId nullability).tests/plugin/chatThreadsPersistence.test.ts→tests/infrastructure/chat/chatThreadsCodec.test.ts(import paths updated; assertions preserved).tests/plugin/main.chat-threads-flush.test.tsdeleted; coverage moved into the adapter tests.tests/infrastructure/obsidian/ObsidianChatThreadsRepository.test.tscovers load, debounced save, coalescing, sibling-key preservation, degraded filtering, customdebounceMs,flushPending, and the serialised flush-queue invariant.tests/infrastructure/mock/MockChatThreadsRepository.test.tscovers load/save round-trip, Map/Array seeds, defensive copies.Coverage on the relocated codec: 98.33% statements / 96.15% branches / 100% functions / 98.14% lines (brief target: ≥90%). Overall project thresholds intact at 93.53 / 87.6 / 92.3 / 94.62 (gate 80 / 70 / 80 / 80).
Satisfies REQ-ASM-037, SPEC-ASM-001 §9.3, ADR-0031, ADR-008.
Test plan
npm audit --audit-level=high --omit=dev— 0 vulnerabilitiesnpm run typecheck— cleannpm run lint— 0 errors (24 pre-existing warnings)npm run test— 1882 / 1882 passing (was 1851 pre-WP; +31 new tests)npm run build— OKnpm run build:web— OKnpm run docs:api— 0 errors (1 pre-existing typedoc link warning)grep -r 'scheduleChatThreadsPersistence' src/confirms only doc comments mention the removed methodnpm run test:coverage—chatThreadsCodec.ts≥ 90% branches; total thresholds methttps://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
Generated by Claude Code