review(p9-fb-09): 회차 1 nit 반영
- `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) <noreply@anthropic.com>
This commit is contained in:
@@ -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<EditorRequest>,
|
||||
/// 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<EditorRequest>,
|
||||
/// 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).
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user