From 3eb24f2c63ce9ac72cc65e47dd5f0cb4eacffd97 Mon Sep 17 00:00:00 2001 From: Jake Date: Fri, 24 Apr 2026 23:46:06 +0100 Subject: [PATCH] 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) --- .../plans/2026-04-24-phase9-polish-debt.md | 50 +++++++++++++++++++ .../2026-04-24-phase9-polish-debt-design.md | 2 +- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-04-24-phase9-polish-debt.md b/docs/superpowers/plans/2026-04-24-phase9-polish-debt.md index 5bc05b0..01098ba 100644 --- a/docs/superpowers/plans/2026-04-24-phase9-polish-debt.md +++ b/docs/superpowers/plans/2026-04-24-phase9-polish-debt.md @@ -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` — *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` — 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>` + `llmTags: Option>` — 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). diff --git a/docs/superpowers/specs/2026-04-24-phase9-polish-debt-design.md b/docs/superpowers/specs/2026-04-24-phase9-polish-debt-design.md index cb2d4ee..2a5e71f 100644 --- a/docs/superpowers/specs/2026-04-24-phase9-polish-debt-design.md +++ b/docs/superpowers/specs/2026-04-24-phase9-polish-debt-design.md @@ -96,7 +96,7 @@ A new utility `src/lib/utils/saveMarkdown.ts` centralises `suggestedFilename` + ### Item 3 — LLM content tags -**Data model.** Add `llmTags: string[]` to history entries alongside `manualTags`. Persisted via the existing `saveHistory` path; columnar storage already serialises arrays as comma-joined strings (see `page.svelte.ts:290`). +**Data model.** Add `llmTags: string[]` to history entries alongside `manualTags`. Persisted via a real SQLite column `transcripts.llm_tags TEXT NOT NULL DEFAULT ''` added in migration v14, exposed through an extended `update_transcript` Tauri command. (Earlier draft of this spec assumed the existing `saveHistory()` path was a real persist; closer review showed it's a no-op stub. The migration + command extension is in-scope as Task 8.5 of the plan, and additionally fixes a latent bug whereby existing `manualTags` edits weren't persisting either.) **Tag schema.** - `topic:<1-3 noun phrase>` — free-form, lowercase, hyphen-joined. Examples: `topic:interview-prep`, `topic:grant-application`, `topic:personal-finance`. Limit one per transcript (the dominant subject).