gitlore/plans/tui-prd-v2-frankentui.feedback-7.md

1. **Fix a critical contradiction in workspace/toolchain isolation**
Rationale: Section `3.2` says `crates/lore-tui` is excluded from the root workspace, but Section `9.1` currently adds it as a member. That inconsistency will cause broken CI/tooling behavior and confusion about whether stable-only workflows remain safe.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 9.1 Dependency Changes
-# Root Cargo.toml changes
-[workspace]
-members = [".", "crates/lore-tui"]
+# Root Cargo.toml changes
+[workspace]
+members = ["."]
+exclude = ["crates/lore-tui"]
@@
-# Add workspace member (no lore-tui dep, no tui feature)
+# Keep lore-tui EXCLUDED from root workspace (nightly isolation boundary)
@@ 9.3 Phase 0 — Toolchain Gate
-1. `cargo check --all-targets` passes on pinned nightly (TUI crate) and stable (core)
+1. `cargo +stable check --workspace --all-targets` passes for root workspace
+2. `cargo +nightly-2026-02-08 check --manifest-path crates/lore-tui/Cargo.toml --all-targets` passes
```

2. **Replace global loading spinner with per-screen stale-while-revalidate**
Rationale: A single `is_loading` flag causes full-screen flicker and blocked context during quick refreshes. Per-screen load states keep existing data visible while background refresh runs, improving perceived performance and usability.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 10.10 State Module — Complete
-    pub is_loading: bool,
+    pub load_state: ScreenLoadStateMap,
@@
-    pub fn set_loading(&mut self, loading: bool) {
-        self.is_loading = loading;
-    }
+    pub fn set_loading(&mut self, screen: ScreenId, state: LoadState) {
+        self.load_state.insert(screen, state);
+    }
+
+pub enum LoadState {
+    Idle,
+    LoadingInitial,
+    Refreshing,      // stale data remains visible
+    Error(String),
+}
@@ 4.4 App — Implementing the Model Trait
-        // Loading spinner overlay (while async data is fetching)
-        if self.state.is_loading {
-            crate::tui::view::common::render_loading(frame, body);
-        } else {
-            match self.navigation.current() { ... }
-        }
+        // Always render screen; show lightweight refresh indicator when needed.
+        match self.navigation.current() { ... }
+        crate::tui::view::common::render_refresh_indicator_if_needed(
+            self.navigation.current(), &self.state.load_state, frame, body
+        );
```

3. **Make `TaskSupervisor` a real scheduler (not just token registry)**
Rationale: Current design declares priority lanes but still dispatches directly with `Cmd::task`, and debounce uses `thread::sleep` per keystroke (wastes worker threads). A bounded scheduler with queued tasks and timer-driven debounce will reduce contention and tail latency.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 4.5.1 Task Supervisor (Dedup + Cancellation + Priority)
-pub struct TaskSupervisor {
-    active: HashMap<TaskKey, Arc<CancelToken>>,
-    generation: AtomicU64,
-}
+pub struct TaskSupervisor {
+    active: HashMap<TaskKey, Arc<CancelToken>>,
+    generation: AtomicU64,
+    queue: BinaryHeap<ScheduledTask>,
+    inflight: HashMap<TaskPriority, usize>,
+    limits: TaskLaneLimits, // e.g. Input=4, Navigation=2, Background=1
+}
@@
-// 200ms debounce via cancelable scheduled event (not thread::sleep).
-Cmd::task(move || {
-    std::thread::sleep(std::time::Duration::from_millis(200));
-    ...
-})
+// Debounce via runtime timer message; no sleeping worker thread.
+self.state.search.debounce_deadline = Some(now + 200ms);
+Cmd::none()
@@ 4.4 update()
+Msg::Tick => {
+    if self.state.search.debounce_expired(now) {
+        return self.dispatch_supervised(TaskKey::Search, TaskPriority::Input, ...);
+    }
+    self.task_supervisor.dispatch_ready(now)
+}
```

4. **Add a sync run ledger for exact “new since sync” navigation**
Rationale: “Since last sync” based on timestamps is ambiguous with partial failures, retries, and clock drift. A lightweight `sync_runs` + `sync_deltas` ledger makes summary-mode drill-down exact and auditable without implementing full resumable checkpoints.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 5.9 Sync
-- `i` navigates to Issue List pre-filtered to "since last sync"
-- `m` navigates to MR List pre-filtered to "since last sync"
+- `i` navigates to Issue List pre-filtered to `sync_run_id=<last_run>`
+- `m` navigates to MR List pre-filtered to `sync_run_id=<last_run>`
+- Filters are driven by persisted `sync_deltas` rows (exact entity keys changed in run)
@@ 10.1 New Files
+src/core/migrations/00xx_add_sync_run_ledger.sql
@@ New migration (appendix)
+CREATE TABLE sync_runs (
+  id INTEGER PRIMARY KEY,
+  started_at_ms INTEGER NOT NULL,
+  completed_at_ms INTEGER,
+  status TEXT NOT NULL
+);
+CREATE TABLE sync_deltas (
+  sync_run_id INTEGER NOT NULL,
+  entity_kind TEXT NOT NULL,
+  project_id INTEGER NOT NULL,
+  iid INTEGER NOT NULL,
+  change_kind TEXT NOT NULL
+);
+CREATE INDEX idx_sync_deltas_run_kind ON sync_deltas(sync_run_id, entity_kind);
@@ 11 Assumptions
-16. No new SQLite tables needed for v1
+16. Two small v1 tables are added: `sync_runs` and `sync_deltas` for deterministic post-sync UX.
```

5. **Expand the GA index set to match actual filter surface**
Rationale: Current required indexes only cover default sort paths; they do not match common filters like `author`, `assignee`, `reviewer`, `target_branch`, label-based filtering. This will likely miss p95 SLOs at M tier.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 9.3.1 Required Indexes (GA Blocker)
 CREATE INDEX IF NOT EXISTS idx_issues_list_default
     ON issues(project_id, state, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_issues_author_updated
+    ON issues(project_id, state, author_username, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_issues_assignee_updated
+    ON issues(project_id, state, assignee_username, updated_at DESC, iid DESC);
@@
 CREATE INDEX IF NOT EXISTS idx_mrs_list_default
     ON merge_requests(project_id, state, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_mrs_reviewer_updated
+    ON merge_requests(project_id, state, reviewer_username, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_mrs_target_updated
+    ON merge_requests(project_id, state, target_branch, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_mrs_source_updated
+    ON merge_requests(project_id, state, source_branch, updated_at DESC, iid DESC);
@@
+-- If labels are normalized through join table:
+CREATE INDEX IF NOT EXISTS idx_issue_labels_label_issue ON issue_labels(label, issue_id);
+CREATE INDEX IF NOT EXISTS idx_mr_labels_label_mr ON mr_labels(label, mr_id);
@@ CI enforcement
-asserts that none show `SCAN TABLE` for the primary entity tables
+asserts that none show full scans for primary tables under default filters AND top 8 user-facing filter combinations
```

6. **Add DB schema compatibility preflight (separate from binary compat)**
Rationale: Binary compat (`--compat-version`) does not protect against schema mismatches. Add explicit schema version checks before booting the TUI to avoid runtime SQL errors deep in navigation paths.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 3.2 Nightly Rust Strategy
-- **Compatibility contract:** Before spawning `lore-tui`, the `lore tui` subcommand runs `lore-tui --compat-version` ...
+- **Compatibility contract:** Before spawning `lore-tui`, `lore tui` validates:
+  1) binary compat version (`lore-tui --compat-version`)
+  2) DB schema range (`lore-tui --check-schema <db-path>`)
+If schema is out-of-range, print remediation: `lore migrate`.
@@ 9.3 Phase 0 — Toolchain Gate
+17. Schema preflight test: incompatible DB schema yields actionable error and non-zero exit before entering TUI loop.
```

7. **Refine terminal sanitization to preserve legitimate Unicode while blocking control attacks**
Rationale: Current sanitizer strips zero-width joiners and similar characters, which breaks emoji/grapheme rendering and undermines your own `text_width` goals. Keep benign Unicode, remove only dangerous controls/bidi spoof vectors, and sanitize markdown link targets too.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 10.4.1 Terminal Safety — Untrusted Text Sanitization
-- Strip bidi overrides ... and zero-width/invisible controls ...
+- Strip ANSI/OSC/control chars and bidi spoof controls.
+- Preserve legitimate grapheme-joining characters (ZWJ/ZWNJ/combining marks) for correct Unicode rendering.
+- Sanitize markdown link targets with strict URL allowlist before rendering clickable links.
@@ safety.rs
-            // Strip zero-width and invisible controls
-            '\u{200B}' | '\u{200C}' | '\u{200D}' | '\u{FEFF}' | '\u{00AD}' => {}
+            // Preserve grapheme/emoji join behavior; remove only harmful controls.
+            // (ZWJ/ZWNJ/combining marks are retained)
@@ Enforcement rule
 - Search result snippets
 - Author names and labels
+- Markdown link destinations (scheme + origin validation before render/open)
```

8. **Add key normalization layer for terminal portability**
Rationale: Collision notes are good, but you still need a canonicalization layer because terminals emit different sequences for Alt/Meta/Backspace/Enter variants. This reduces “works in iTerm, broken in tmux/SSH” bugs.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 8.2 List Screens
 **Terminal keybinding safety notes:**
@@
 - `Ctrl+M` is NOT used — it collides with `Enter` ...
+
+**Key normalization layer (new):**
+- Introduce `KeyNormalizer` before `interpret_key()`:
+  - normalize Backspace variants (`^H`, `DEL`)
+  - normalize Alt/Meta prefixes
+  - normalize Shift+Tab vs Tab where terminal supports it
+  - normalize kitty/CSI-u enhanced key protocols when present
@@ 9.2 Phases
+    Key normalization integration tests   :p5d, after p5c, 1d
+    Terminal profile replay tests         :p5e, after p5d, 1d
```

9. **Add deterministic event-trace capture for crash reproduction**
Rationale: Panic logs without recent event context are often insufficient for TUI race bugs. Persist last-N normalized events + active screen + task state snapshot on panic for one-command repro.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 3.1 Risk Matrix
 | Runtime panic leaves user blocked | High | Medium | Panic hook writes crash report, restores terminal, offers fallback CLI command |
+| Hard-to-reproduce input race bugs | Medium | Medium | Persist last 2k normalized events + state hash on panic for deterministic replay |
@@ 10.3 Entry Point / panic hook
-        // 2. Write crash dump
+        // 2. Write crash dump + event trace snapshot
+        // Includes: last 2000 normalized events, current screen, in-flight task keys/generations
@@ 10.9.1 Non-Snapshot Tests
+/// Replay captured event trace from panic artifact and assert no panic.
+#[test]
+fn replay_trace_artifact_is_stable() { ... }
```

10. **Do a plan-wide consistency pass on pseudocode contracts**
Rationale: There are internal mismatches that will create implementation churn (`search_request_id` still referenced after replacement, `items` vs `window`, keybinding mismatch `Ctrl+I` vs `Alt+o`). Tightening these now saves real engineering time later.

```diff
--- a/PRD.md
+++ b/PRD.md
@@ 4.4 LoreApp::new
-            search_request_id: 0,
+            // dedup generation handled by TaskSupervisor
@@ 8.1 Global
-| `Ctrl+O` | Jump backward in jump list (entity hops) |
-| `Alt+o` | Jump forward in jump list (entity hops) |
+| `Ctrl+O` | Jump backward in jump list (entity hops) |
+| `Alt+o`  | Jump forward in jump list (entity hops) |
@@ 10.10 IssueListState
-    pub fn selected_item(&self) -> Option<&IssueListRow> {
-        self.items.get(self.selected_index)
-    }
+    pub fn selected_item(&self) -> Option<&IssueListRow> {
+        self.window.get(self.selected_index)
+    }
```

If you want, I can now produce a single consolidated unified diff patch of the full PRD with these revisions merged end-to-end.