docs(phase9): plan corrections from critical review
Pre-execution review against the actual codebase (Codex unavailable this session) surfaced three mismatches: kon-llm uses LlmEngine with a synchronous generate(prompt, config), AppState exposes llm_engine as a direct Arc not behind RwLock, and llmTags persistence requires a real SQLite migration plus Tauri command extension because saveHistory() is a no-op stub today (which also means manualTags edits weren't persisting). Plan now adds Task 8.5 for migration v14 + storage and Tauri-command extension. Spec data-model section corrected to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -12,6 +12,56 @@
|
||||
|
||||
---
|
||||
|
||||
## Plan corrections (added during execution 2026-04-24)
|
||||
|
||||
Critical-review pass against the actual codebase (after Codex was unavailable for a cross-model read) surfaced three mismatches between the plan's prescribed code and what the repo actually contains. Implementations adapt as follows.
|
||||
|
||||
**1. `kon-llm` API.** Plan referenced `LlamaEngine::generate_chat(messages, config).await`. Actual: `LlmEngine::generate(prompt: &str, config: &GenerationConfig) -> Result<String, EngineError>` — *synchronous*, prompt is a single rendered string, no chat-message vec. The engine struct is `LlmEngine` not `LlamaEngine`. Loaded via `engine.load(model_path)` (sync, &self). The closest in-tree analogue to the new `extract_content_tags` is `LlmEngine::cleanup_text` — same crate, same generate-with-prompt shape; copy that pattern. Tasks 7 + 8 code samples are guides, not literals.
|
||||
|
||||
**2. Tauri `AppState`.** Plan referenced `state.llm.read().await.engine.as_ref()`. Actual: `AppState.llm_engine: Arc<LlmEngine>` — direct, no RwLock, accessed as `state.llm_engine.as_ref()`. Existing LLM commands file is `src-tauri/src/commands/llm.rs` (already there — do not create a new one).
|
||||
|
||||
**3. STRUCTURAL — tag persistence path.**
|
||||
Plan assumed `llmTags` could mirror `manualTags` as a comma-joined string in a JS-only persistence path. Actual:
|
||||
- `transcripts.manual_tags` is a real SQLite column (added by an earlier migration). The Rust storage fn `db_update_transcript` accepts `manual_tags`.
|
||||
- The Tauri `update_transcript` command currently exposes only `text` + `title`, NOT `manual_tags` — so existing manualTags edits aren't persisting today either (latent bug).
|
||||
- The frontend `saveHistory()` is a no-op stub.
|
||||
- `transcripts.llm_tags` does not exist.
|
||||
|
||||
Fix in scope (decided 2026-04-24, "do it properly so we don't need to revisit"): Phase 9 picks up the persistence work as new **Task 8.5**, then Tasks 9 + 10 are rewired to call the proper update command instead of the no-op `saveHistory`.
|
||||
|
||||
### NEW Task 8.5: Migration v14 + storage extension + Tauri command extension for tags
|
||||
|
||||
**Why here:** Tasks 9 + 10 cannot persist `llmTags` (or even `manualTags`) without this. Bundling fixes the latent manualTags persistence bug for free.
|
||||
|
||||
**Files:**
|
||||
- Modify: `crates/storage/src/migrations.rs` — append migration v14 entry; update the FTS5 / table-recreate migrations if any of them rebuild the schema with explicit columns (verify with grep on `manual_tags` first).
|
||||
- Modify: `crates/storage/src/database.rs` — add `llm_tags` to every SELECT / INSERT / UPDATE that touches transcripts; extend `db_update_transcript` to accept an `Option<&str>` for `llm_tags`.
|
||||
- Modify: `src-tauri/src/commands/transcripts.rs` — `TranscriptDto` gets `llm_tags: String` (camelCase via serde). `add_transcript` accepts and writes `manualTags` + `llmTags` on insert. `update_transcript` now accepts `manualTags: Option<Vec<String>>` + `llmTags: Option<Vec<String>>` — joins to comma-strings before calling storage.
|
||||
- Modify: `src/lib/types/app.ts` — `TranscriptRow.llmTags: string` (storage shape) + `TranscriptEntry.llmTags: string[]` (in-memory).
|
||||
- Modify: `src/lib/stores/page.svelte.ts` — hydrate `llmTags` in `mapTranscriptRow`; rewrite `saveHistory()` from no-op to call `update_transcript` with the current `manualTags` + `llmTags` of a target entry. New helper `persistTags(item)` that handles the actual write.
|
||||
|
||||
**TDD steps:**
|
||||
1. Storage test: migration v14 adds `llm_tags TEXT NOT NULL DEFAULT ''` column; pre-existing rows default to empty string.
|
||||
2. Storage test: `db_update_transcript(.., None, Some("topic:x,intent:y"))` writes only `llm_tags` and leaves `manual_tags` untouched.
|
||||
3. Tauri command compile-check via `cargo build -p kon`.
|
||||
4. Frontend `npm run check` clean.
|
||||
|
||||
Commit message: `feat(phase9): migration v14 + tag persistence wiring`.
|
||||
|
||||
### Adjusted Task 9 — Frontend types + persistence rewire
|
||||
|
||||
The original Task 9 body remains a guide for the data-shape changes, but the persistence path replaces no-op `saveHistory()` with calls to a new `persistTags(item)` helper that wraps `invoke("update_transcript", { id, manualTags, llmTags })`. All call sites in `HistoryPage.svelte` that currently call `saveHistory()` after touching `manualTags` or `llmTags` swap to `persistTags(item)`.
|
||||
|
||||
### Adjusted Task 10 — HistoryPage Tag UI
|
||||
|
||||
Same code as written; replace every `saveHistory()` call site in the Tag handlers with `await persistTags(item)`. Errors from the persist call surface via the existing `toasts.error` pattern.
|
||||
|
||||
### Renumbering
|
||||
|
||||
Tasks 8.5 inserted between 8 and 9. Subsequent task numbers (11..16) unchanged. Total task count: **16 + 1 = 17**.
|
||||
|
||||
---
|
||||
|
||||
## Conventions
|
||||
|
||||
- Branch: `main` (Jake's standing rule — phases ship on main, not feature branches).
|
||||
|
||||
Reference in New Issue
Block a user