refactor: Pre-phase-6 review cleanup.
Some checks failed
CI / Lint (push) Has been cancelled
CI / Test (push) Has been cancelled

- Propagate serialization errors in output path
instead of unwrap_or_default() silently writing
blank lines (main.rs, handler.rs).
- Cap selection undo stack at 50 entries to bound
memory on heavy multi-select usage (menu.rs).
- Break TUI event loop on dead action channel
instead of silently dropping keypresses (lib.rs).
- Log dropped handler hook events when channel is
full (handler.rs).
- Reject menu and headless DSL tests that have no
assertions at compile time (codegen.rs).
- Document filter_text ownership design in TUI and
exec/handler error asymmetry in CompositeHandler.
This commit is contained in:
2026-03-14 15:26:31 -04:00
parent 8077469d19
commit cd1927fa9f
5 changed files with 57 additions and 12 deletions

View File

@@ -35,6 +35,10 @@ pub enum ActionOutcome {
NoOp, NoOp,
} }
/// Maximum number of undo snapshots to retain. Keeps memory
/// bounded when users toggle hundreds of selections.
const UNDO_STACK_LIMIT: usize = 50;
/// Tracks multi-select state: which items are selected (by /// Tracks multi-select state: which items are selected (by
/// original index), with undo/redo support. /// original index), with undo/redo support.
pub struct SelectionState { pub struct SelectionState {
@@ -57,6 +61,9 @@ impl SelectionState {
} }
fn push_undo(&mut self) { fn push_undo(&mut self) {
if self.undo_stack.len() >= UNDO_STACK_LIMIT {
self.undo_stack.remove(0);
}
self.undo_stack.push(self.selected.clone()); self.undo_stack.push(self.selected.clone());
self.redo_stack.clear(); self.redo_stack.clear();
} }

View File

@@ -141,6 +141,16 @@ fn gen_headless(case: &TestCase, fixtures: &Fixtures) -> syn::Result<TokenStream
}); });
} }
if asserts.is_empty() {
return Err(syn::Error::new(
case.name.span(),
format!(
"headless test '{}' has no assertions (expected stdout, stderr contains, or exit)",
case.name
),
));
}
Ok(quote! { Ok(quote! {
#[test] #[test]
fn #test_name() { fn #test_name() {
@@ -364,8 +374,13 @@ fn gen_menu(case: &TestCase, fixtures: &Fixtures) -> syn::Result<TokenStream> {
} }
} }
} else { } else {
// No assertion on result. Probably an error, but let it compile. return Err(syn::Error::new(
quote! {} case.name.span(),
format!(
"menu test '{}' has no result assertion (expected selected, selected_items, or cancelled)",
case.name
),
));
}; };
// If test expects cancellation via sender drop (no cancel action, no confirm), // If test expects cancellation via sender drop (no cancel action, no confirm),

View File

@@ -71,12 +71,21 @@ async fn run_inner(
// Send initial resize // Send initial resize
let size = terminal.size()?; let size = terminal.size()?;
let list_height = size.height.saturating_sub(1); let list_height = size.height.saturating_sub(1);
let _ = action_tx if action_tx
.send(Action::Resize { .send(Action::Resize {
height: list_height, height: list_height,
}) })
.await; .await
.is_err()
{
tracing::warn!("core channel closed before TUI started");
return Ok(());
}
// Local copy of filter text for immediate cursor positioning.
// Synced from core on every StateChanged. Core is the source
// of truth: if IPC or a hook changes the filter, the next
// broadcast overwrites this.
let mut filter_text = String::new(); let mut filter_text = String::new();
let mut view_state: Option<ViewState> = None; let mut view_state: Option<ViewState> = None;
let mut event_stream = EventStream::new(); let mut event_stream = EventStream::new();
@@ -124,12 +133,16 @@ async fn run_inner(
mode = *m; mode = *m;
pending = PendingKey::None; pending = PendingKey::None;
} }
let _ = action_tx.send(action).await; if action_tx.send(action).await.is_err() {
break; // core is gone
}
} }
} }
Event::Resize(_, h) => { Event::Resize(_, h) => {
let list_height = h.saturating_sub(1); let list_height = h.saturating_sub(1);
let _ = action_tx.send(Action::Resize { height: list_height }).await; if action_tx.send(Action::Resize { height: list_height }).await.is_err() {
break;
}
} }
_ => {} _ => {}
} }

View File

@@ -76,7 +76,9 @@ impl HookHandler for ShellHandlerHook {
let kind = event.kind(); let kind = event.kind();
if let Some(tx) = self.event_txs.get(&kind) { if let Some(tx) = self.event_txs.get(&kind) {
// Non-blocking send. If the channel is full, drop the event. // Non-blocking send. If the channel is full, drop the event.
let _ = tx.try_send(event); if let Err(e) = tx.try_send(event) {
tracing::debug!(error = %e, "handler event channel full, dropping event");
}
} }
Ok(()) Ok(())
} }
@@ -128,7 +130,13 @@ async fn run_handler_process(
// Writer loop: reads events from channel, writes to stdin // Writer loop: reads events from channel, writes to stdin
while let Some(event) = event_rx.recv().await { while let Some(event) = event_rx.recv().await {
let json = serde_json::to_string(&event).unwrap_or_default(); let json = match serde_json::to_string(&event) {
Ok(j) => j,
Err(e) => {
tracing::warn!(error = %e, "failed to serialize hook event, skipping");
continue;
}
};
if stdin_writer.write_all(json.as_bytes()).await.is_err() { if stdin_writer.write_all(json.as_bytes()).await.is_err() {
break; break;
} }

View File

@@ -353,8 +353,9 @@ struct CompositeHookHandler {
impl HookHandler for CompositeHookHandler { impl HookHandler for CompositeHookHandler {
fn handle(&self, event: pikl_core::hook::HookEvent) -> Result<(), PiklError> { fn handle(&self, event: pikl_core::hook::HookEvent) -> Result<(), PiklError> {
// Both fire. Exec is fire-and-forget, handler may // Exec hooks are fire-and-forget: errors are logged
// send responses through action_tx. // internally by ShellExecHandler, not propagated here.
// Handler hooks are bidirectional, so errors propagate.
let _ = self.exec.handle(event.clone()); let _ = self.exec.handle(event.clone());
if let Some(ref h) = self.handler { if let Some(ref h) = self.handler {
h.handle(event)?; h.handle(event)?;
@@ -507,7 +508,7 @@ fn handle_result(result: Result<MenuResult, PiklError>, cli: &Cli) {
/// Serialize an OutputItem as JSON and write it to the given writer. /// Serialize an OutputItem as JSON and write it to the given writer.
fn write_output_json(writer: &mut impl Write, output: &OutputItem) -> Result<(), std::io::Error> { fn write_output_json(writer: &mut impl Write, output: &OutputItem) -> Result<(), std::io::Error> {
let json = serde_json::to_string(output).unwrap_or_default(); let json = serde_json::to_string(output).map_err(|e| std::io::Error::other(e.to_string()))?;
writeln!(writer, "{json}") writeln!(writer, "{json}")
} }
@@ -517,7 +518,8 @@ fn write_plain_value(writer: &mut impl Write, value: &Value) -> Result<(), std::
match value { match value {
Value::String(s) => writeln!(writer, "{s}"), Value::String(s) => writeln!(writer, "{s}"),
_ => { _ => {
let json = serde_json::to_string(value).unwrap_or_default(); let json =
serde_json::to_string(value).map_err(|e| std::io::Error::other(e.to_string()))?;
writeln!(writer, "{json}") writeln!(writer, "{json}")
} }
} }