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>, - generation: AtomicU64, -} +pub struct TaskSupervisor { + active: HashMap>, + generation: AtomicU64, + queue: BinaryHeap, + inflight: HashMap, + 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=` +- `m` navigates to MR List pre-filtered to `sync_run_id=` +- 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 `) +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.