From 8c22e3792c75d1ddbf564949a62e249425e9262d Mon Sep 17 00:00:00 2001 From: altair823 Date: Sun, 3 May 2026 02:35:10 +0000 Subject: [PATCH] =?UTF-8?q?review(p9-fb-09):=20=ED=9A=8C=EC=B0=A8=201=20ni?= =?UTF-8?q?t=20=EB=B0=98=EC=98=81?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `App.pending_editor` / `force_redraw` 를 `pub(crate)` 로 좁힘. 외부 caller (kebab-cli/desktop) 는 enqueue 할 일 없고, 잘못 mutate 하면 \"set 후 다음 tick 에 drain\" invariant 가 깨짐. - 외부 read access 가 필요한 경우를 위해 `App::pending_editor()` 읽기 전용 accessor 추가 — integration test (`tests/search.rs`) 가 사용. - `App.force_redraw` doc comment 의 \"ratchet incremented\" 표현을 실제 type (bool flag) 에 맞게 \"when set, the next draw clears\" 로 교체. - `editor::tests::unspawnable_program_surfaces_program_name_in_error` 를 `command_status_returns_not_found_for_missing_program` 으로 rename + doc 수정 — 실제로 `with_external_program` 호출하지 않고 `Command::status()` 만 검증한다는 점을 솔직하게 명시 (helper end-to-end 는 dogfooding 으로 검증). Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/kebab-tui/src/app.rs | 31 +++++++++++++++++++++++-------- crates/kebab-tui/src/editor.rs | 32 +++++++++++++------------------- crates/kebab-tui/tests/search.rs | 7 +++---- 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/crates/kebab-tui/src/app.rs b/crates/kebab-tui/src/app.rs index 9baa769..43497fe 100644 --- a/crates/kebab-tui/src/app.rs +++ b/crates/kebab-tui/src/app.rs @@ -265,14 +265,20 @@ pub struct App { /// vim) — the actual suspend / spawn / restore happens in the /// run loop, where the `TuiTerminal` handle is in scope. /// Drained every tick after the key dispatch. - pub pending_editor: Option, - /// p9-fb-09: ratchet incremented every time the run loop should - /// force a `terminal.clear()` before the next draw. Bumped after - /// `with_external_program` so any leftover screen content from - /// the suspended TUI is wiped. Independent of pending_editor — - /// any future code path that needs a forced redraw can bump - /// this. - pub force_redraw: bool, + /// + /// `pub(crate)` because the enqueue/take invariant ("set by a + /// key handler, drained by the next run-loop tick") only holds + /// for in-crate callers; external mutation could leave a stale + /// request that never gets serviced. + pub(crate) pending_editor: Option, + /// p9-fb-09: when set, the next run-loop draw runs + /// `terminal.clear()` first so any leftover screen content from + /// a suspension (post-editor, future config-reload, …) is wiped + /// before Ratatui's diff renders the new frame. Reset back to + /// false after the clear. Independent of `pending_editor` — + /// any future code path that needs a forced redraw can flip + /// this flag. + pub(crate) force_redraw: bool, } /// p9-fb-09: external-program spawn request. Posted by a pane's key @@ -304,6 +310,15 @@ impl App { }) } + /// Read-only accessor for the in-flight external-program request. + /// Tests and future external observers (e.g. integration smokes) + /// use this to assert that a key dispatch enqueued a spawn — + /// mutating the slot stays `pub(crate)` to preserve the + /// "set-then-drained-on-next-tick" invariant. + pub fn pending_editor(&self) -> Option<&EditorRequest> { + self.pending_editor.as_ref() + } + /// Blocking event loop. Returns when the user quits or a fatal /// error escapes the loop (terminal raw-mode is restored either /// way via the `Terminal` Drop guard). diff --git a/crates/kebab-tui/src/editor.rs b/crates/kebab-tui/src/editor.rs index 778f763..b60fa63 100644 --- a/crates/kebab-tui/src/editor.rs +++ b/crates/kebab-tui/src/editor.rs @@ -101,32 +101,26 @@ fn resume_tui(terminal: &mut TuiTerminal) -> Result<()> { mod tests { use std::process::Command; - /// We can't actually spawn $EDITOR in a unit test (no terminal), - /// but we can verify that the helper rejects an unspawnable - /// command with a useful error context. The /nonexistent path - /// should fail at the OS level (`ENOENT`) and the error chain - /// should mention which program failed. + /// Sanity check on the OS layer that `with_external_program` + /// builds on top of: a missing program path makes `Command:: + /// status()` fail with `ENOENT`, which the helper wraps with + /// `with_context(|| format!("spawn child program: {:?}", ...))` + /// so the error chain points at the program name. /// - /// Skipped under MIRI / sandboxes where `Command::status` may - /// behave differently. Kept in `cfg(test)` so it runs on the - /// normal CI path. - /// - /// Note: this exercises the *spawn path* of `with_external_program` - /// without touching the TUI terminal — would require a real - /// `TuiTerminal` to fully integration-test. The terminal-side - /// suspend/restore is verified by the dogfooding loop on the spec. + /// We can't construct a `TuiTerminal` in a unit test (no real + /// terminal), so the helper end-to-end is verified by the + /// dogfooding loop in the spec rather than here. This test + /// only pins the OS behavior the helper assumes — if a future + /// libc / Rust update changes which `ErrorKind` is returned for + /// `ENOENT`, the helper's error message stays meaningful but + /// this test catches the platform regression first. #[test] - fn unspawnable_program_surfaces_program_name_in_error() { - // Guard the spawn behind a tiny no-op shim that doesn't need - // a terminal: just call `Command::status()` directly to mirror - // what the helper does internally. + fn command_status_returns_not_found_for_missing_program() { let mut cmd = Command::new("/nonexistent/kebab-test-binary-xxx"); cmd.arg("dummy-arg"); let result = cmd.status(); assert!(result.is_err(), "expected ENOENT-like failure"); let err = result.unwrap_err(); - // Verify the OS surfaced a "not found" error — the helper - // wraps this with a `with_context` adding the program name. assert!( matches!( err.kind(), diff --git a/crates/kebab-tui/tests/search.rs b/crates/kebab-tui/tests/search.rs index 7e60bfe..358dc04 100644 --- a/crates/kebab-tui/tests/search.rs +++ b/crates/kebab-tui/tests/search.rs @@ -299,15 +299,14 @@ fn g_key_enqueues_pending_editor_request() { s.hits = vec![make_hit(1, "notes/x.md", "snippet", line_citation("notes/x.md", 42))]; s.selected_hit = 0; } - assert!(app.pending_editor.is_none(), "queue starts empty"); + assert!(app.pending_editor().is_none(), "queue starts empty"); let outcome = handle_key_search( &mut app, KeyEvent::new(KeyCode::Char('g'), KeyModifiers::NONE), ); assert_eq!(outcome, KeyOutcome::Continue); let req = app - .pending_editor - .as_ref() + .pending_editor() .expect("g on a hit must enqueue an EditorRequest"); match &req.citation { Citation::Line { path, start, .. } => { @@ -330,7 +329,7 @@ fn g_key_with_no_hits_does_not_enqueue() { KeyEvent::new(KeyCode::Char('g'), KeyModifiers::NONE), ); assert!( - app.pending_editor.is_none(), + app.pending_editor().is_none(), "g with no hits must not enqueue" ); }