review(p9-fb-08): 회차 1 nit 반영
- `SearchState.worker_thread` 필드 제거 — `JoinHandle` 을 저장만 하고
어디서도 join 안 했음. fire_search 가 spawn 후 handle 을 즉시 drop
하면 OS 가 thread 를 detach (search 는 pure read 라 cleanup 의무
없음). YAGNI — ask.rs 의 thread 와 달리 cancel/observe 수요가 없는
fire-and-forget. doc 으로 의도 명시.
- `debounce_due` 가 `pub` 으로 노출 — 새 skip 분기 (`searching && 같은
query`) 회귀 테스트 추가:
- `debounce_due_skips_when_in_flight_for_same_query`: 같은 input/mode
재입력 시 spawn 안 함 (worker 누적 방지)
- `debounce_due_fires_when_in_flight_for_different_query`: 사용자가
in-flight 보다 빠르게 새 query 입력하면 정상 spawn (poll_worker 의
stale guard 가 이전 결과 처리)
- `search_state_with` 헬퍼: `SearchState::default()` + field 재할당
패턴이 clippy `field_reassign_with_default` 위반 → `#[allow(...)]`
로 lint 무시 (테스트 helper 의 가독성 우선).
23 tests/search.rs + 35 lib + 18 ask + 12 inspect + 10 library = 98
통과. clippy clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -83,12 +83,17 @@ pub struct SearchState {
|
||||
/// (the user kept typing and a newer query is already in
|
||||
/// flight). Wraps at u64::MAX which is unreachable in practice.
|
||||
pub generation: u64,
|
||||
/// p9-fb-08: in-flight worker thread. Held so the worker can be
|
||||
/// joined / observed; results stream back via `worker_rx`.
|
||||
pub worker_thread: Option<std::thread::JoinHandle<()>>,
|
||||
/// p9-fb-08: receiver for `(generation, Result<Vec<SearchHit>>)`
|
||||
/// messages from the in-flight worker. Drained every tick by
|
||||
/// p9-fb-08: receiver for the in-flight worker's
|
||||
/// `SearchWorkerMessage::Done`. Drained every tick by
|
||||
/// `crate::search::poll_worker`. `None` between runs.
|
||||
///
|
||||
/// Workers are fire-and-forget (no join) — search is a pure
|
||||
/// read with no cleanup obligation, and dropping the receiver
|
||||
/// makes the worker's `tx.send` no-op (worker exits after).
|
||||
/// We don't store the `JoinHandle` because nothing observes it
|
||||
/// (cf. `AskState.thread` which is `take().join()`'d on
|
||||
/// `Ctrl-L`); the previous draft kept one for "symmetry" but
|
||||
/// it was dead code.
|
||||
pub worker_rx: Option<std::sync::mpsc::Receiver<SearchWorkerMessage>>,
|
||||
}
|
||||
|
||||
@@ -115,7 +120,6 @@ impl Default for SearchState {
|
||||
searching: false,
|
||||
preview: None,
|
||||
generation: 0,
|
||||
worker_thread: None,
|
||||
worker_rx: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -43,8 +43,10 @@ pub use library::{handle_key_library, render_library};
|
||||
// only safe constructor path for raw mode + alt-screen). External
|
||||
// callers stage editor spawns via `App.pending_editor` instead.
|
||||
pub use search::{build_jump_command, handle_key_search, render_search};
|
||||
// p9-fb-08: expose `poll_worker` so integration tests can drive the
|
||||
// stale-result drop / fresh-result apply paths without spawning the
|
||||
// real thread (they inject a `SearchWorkerMessage` directly via a
|
||||
// channel they construct in the test).
|
||||
// p9-fb-08: expose `poll_worker` + `debounce_due` so integration
|
||||
// tests can drive the stale-result drop / fresh-result apply paths
|
||||
// without spawning the real thread (they inject a
|
||||
// `SearchWorkerMessage` directly via a channel they construct in
|
||||
// the test) and can pin the in-flight-skip invariant of debounce.
|
||||
pub use search::poll_worker as poll_search_worker;
|
||||
pub use search::debounce_due as search_debounce_due;
|
||||
|
||||
@@ -431,7 +431,7 @@ fn parse_editor_env(env: &str) -> (String, Vec<String>) {
|
||||
/// - if a worker is already in flight for the *same* `(input, mode)`
|
||||
/// the spawn is redundant — wait for the result.
|
||||
/// - dedupe against `last_query` (was already there pre-fb-08, kept).
|
||||
pub(crate) fn debounce_due(s: &SearchState) -> bool {
|
||||
pub fn debounce_due(s: &SearchState) -> bool {
|
||||
let Some(at) = s.input_dirty_at else { return false };
|
||||
let elapsed = (time::OffsetDateTime::now_utc() - at)
|
||||
.try_into()
|
||||
@@ -488,7 +488,12 @@ pub(crate) fn fire_search(state: &mut App) -> anyhow::Result<()> {
|
||||
};
|
||||
|
||||
let (tx, rx) = std::sync::mpsc::channel();
|
||||
let handle = std::thread::Builder::new()
|
||||
// Fire-and-forget — `JoinHandle` is dropped immediately so the
|
||||
// OS detaches the thread. Search is a pure read with no
|
||||
// cleanup obligation; if the receiver is replaced (next
|
||||
// keystroke spawns a fresh worker), the old worker's
|
||||
// `tx.send` no-ops and it exits silently.
|
||||
std::thread::Builder::new()
|
||||
.name(format!("kebab-tui-search-gen{generation}"))
|
||||
.spawn(move || {
|
||||
let query = SearchQuery {
|
||||
@@ -498,9 +503,6 @@ pub(crate) fn fire_search(state: &mut App) -> anyhow::Result<()> {
|
||||
filters: kebab_core::SearchFilters::default(),
|
||||
};
|
||||
let result = kebab_app::search_with_config(cfg, query);
|
||||
// Send result back. If the receiver dropped (UI closed,
|
||||
// or — in tests — state torn down), the result is
|
||||
// discarded; that's fine since search is a pure read.
|
||||
let _ = tx.send(crate::app::SearchWorkerMessage::Done {
|
||||
generation,
|
||||
result,
|
||||
@@ -509,7 +511,6 @@ pub(crate) fn fire_search(state: &mut App) -> anyhow::Result<()> {
|
||||
.map_err(|e| anyhow::anyhow!("spawn search worker: {e}"))?;
|
||||
|
||||
let s = state.search.as_mut().expect("Search slot must exist");
|
||||
s.worker_thread = Some(handle);
|
||||
s.worker_rx = Some(rx);
|
||||
Ok(())
|
||||
}
|
||||
@@ -526,15 +527,13 @@ pub fn poll_worker(state: &mut App) {
|
||||
Err(std::sync::mpsc::TryRecvError::Empty) => return,
|
||||
Err(std::sync::mpsc::TryRecvError::Disconnected) => {
|
||||
// Worker panicked or dropped tx without sending. Clear
|
||||
// the worker handles + searching flag so the next debounce
|
||||
// tick can re-fire if needed.
|
||||
s.worker_thread = None;
|
||||
// the rx + searching flag so the next debounce tick can
|
||||
// re-fire if needed.
|
||||
s.worker_rx = None;
|
||||
s.searching = false;
|
||||
return;
|
||||
}
|
||||
};
|
||||
s.worker_thread = None;
|
||||
s.worker_rx = None;
|
||||
match msg {
|
||||
crate::app::SearchWorkerMessage::Done { generation, result } => {
|
||||
|
||||
@@ -8,7 +8,7 @@ use kebab_core::{
|
||||
};
|
||||
use kebab_tui::{
|
||||
App, KeyOutcome, Pane, SearchState, SearchWorkerMessage, build_jump_command,
|
||||
handle_key_search, poll_search_worker, render_search,
|
||||
handle_key_search, poll_search_worker, render_search, search_debounce_due,
|
||||
};
|
||||
use ratatui::Terminal;
|
||||
use ratatui::backend::TestBackend;
|
||||
@@ -410,6 +410,56 @@ fn poll_worker_noop_when_no_rx() {
|
||||
assert!(s.worker_rx.is_none());
|
||||
}
|
||||
|
||||
/// Helper for the debounce_due tests — build a state with the four
|
||||
/// fields the test cares about set, others default.
|
||||
#[allow(clippy::field_reassign_with_default)]
|
||||
fn search_state_with(input: &str, mode: SearchMode, searching: bool, last_query: Option<(String, SearchMode)>) -> SearchState {
|
||||
let mut s = SearchState::default();
|
||||
s.input = input.into();
|
||||
s.mode = mode;
|
||||
s.searching = searching;
|
||||
s.last_query = last_query;
|
||||
s.input_dirty_at = Some(
|
||||
time::OffsetDateTime::now_utc() - time::Duration::seconds(1),
|
||||
);
|
||||
s
|
||||
}
|
||||
|
||||
/// p9-fb-08 — `debounce_due` skips when an in-flight worker is
|
||||
/// already running for the same `(input, mode)` pair. Without this
|
||||
/// guard, a "phantom keystroke" (re-typing the same chars) would
|
||||
/// pile up workers and burn CPU.
|
||||
#[test]
|
||||
fn debounce_due_skips_when_in_flight_for_same_query() {
|
||||
let s = search_state_with(
|
||||
"hello",
|
||||
SearchMode::Hybrid,
|
||||
true,
|
||||
Some(("hello".into(), SearchMode::Hybrid)),
|
||||
);
|
||||
assert!(
|
||||
!search_debounce_due(&s),
|
||||
"in-flight worker for same query → debounce must skip"
|
||||
);
|
||||
}
|
||||
|
||||
/// p9-fb-08 — `debounce_due` still fires when a different query is
|
||||
/// in flight (user typed past the in-flight one). The new spawn
|
||||
/// makes the prior result stale (handled by `poll_worker`).
|
||||
#[test]
|
||||
fn debounce_due_fires_when_in_flight_for_different_query() {
|
||||
let s = search_state_with(
|
||||
"hello world",
|
||||
SearchMode::Hybrid,
|
||||
true,
|
||||
Some(("hello".into(), SearchMode::Hybrid)),
|
||||
);
|
||||
assert!(
|
||||
search_debounce_due(&s),
|
||||
"in-flight worker for old query → new query still spawns"
|
||||
);
|
||||
}
|
||||
|
||||
/// p9-fb-08 — disconnected channel (worker panicked) clears the rx
|
||||
/// + searching flag so the next debounce tick can re-fire cleanly.
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user