fix: prevent panic on Shift+Tab in shared manage editor

[avihut]

Summary

• Filter key codes upstream of edtui's handler before forwarding, so unsupported codes (BackTab, F-keys, Insert, …) no longer hit edtui's _ => unimplemented!() arm and crash the process.
• The bug surfaced as Shift+Tab "exiting" the inline editor — actually the panic hook restoring the terminal as the process died.

Root cause

edtui::events::key::input::From<crossterm::event::KeyCode> (e.g. line 146 in 0.11.3) matches a small whitelist and panics on everything else. crossterm emits Shift+Tab as KeyCode::BackTab, which is not in that whitelist, so the first Shift+Tab keypress in the inline editor panicked.

Fix

Add is_edtui_supported(code) mirroring edtui's whitelist (Char, Enter, Esc, Backspace, Delete, Tab, Left, Right, Up, Down, Home, End, PageUp, PageDown) and drop anything else before calling the handler.

Test plan

• mise run test:unit — 1698 pass (3 new regression tests)
• mise run clippy — clean
• mise run fmt:check — clean
• Manual: open daft shared manage, enter the inline editor, press Shift+Tab → no panic, editor stays open
• Manual: same with F1, F12, Insert → no panic

Fixes #345

🤖 Generated with Claude Code

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 2m 15s —— View job

---

PR Review

• Read changed files and git diff
• Analyze code for correctness, idioms, test coverage, security, and performance
• Post review feedback

---

Summary

The fix is correct and the approach is pragmatic. The root cause (edtui's unimplemented!() catch-all in its From<crossterm::event::KeyCode> impl) is accurately diagnosed, and the upstream filter is the right layer to intercept unsupported key codes. A few things worth flagging before merge:

---

Issues

1. Maintenance risk: whitelist is a copy of edtui internals ⚠️

src/output/tui/shared_picker/editor.rs:25–53

The comment correctly identifies this as a concern ("Source of truth: events/key/input.rs in the edtui crate"), but the risk is asymmetric: if edtui adds key codes in a future 0.11.x patch, users will lose access to those keys silently — no test failure, no warning. If edtui removes a code, the filter passes a key that panics anyway.

Consider adding an inline comment on the edtui dependency in Cargo.toml or a // SYNC WITH EDTUI marker, so the next person bumping the version knows to audit events/key/input.rs. The doc comment already does this well — just make the "what to do when upgrading edtui" action explicit.

2. Esc in the whitelist is dead code (minor nit)

src/output/tui/shared_picker/editor.rs:41

matches!(
    code,
    Char(_) | Enter | Esc | ...   // <-- Esc is already returned before reaching this
)

handle_key catches Esc at line 112 and always returns before the is_edtui_supported check. Including Esc in the whitelist is harmless but misleading — a reader might think Esc can reach on_key_event, when it never does. Removing it would make the whitelist a precise description of what edtui's handler actually sees.

Fix this →

3. Regression test exercises the filter function, not the call site (low priority)

src/output/tui/shared_picker/editor.rs:317–323

The three new tests verify is_edtui_supported directly, which is good. But they don't exercise handle_key itself — if the if !is_edtui_supported(...) guard were accidentally deleted from handle_key, all three tests would still pass.

Adding a test that calls handle_key with a BackTab KeyEvent directly on an EditSession would catch that class of regression. Something like:

#[test]
fn handle_key_back_tab_returns_false_and_does_not_panic() {
    let dir = tempfile::tempdir().unwrap();
    let path = dir.path().join("f.txt");
    std::fs::write(&path, "hello").unwrap();
    let mut session = EditSession::new(path, false, "test".into(), "txt".into()).unwrap();
    let event = crossterm::event::KeyEvent::new(
        crossterm::event::KeyCode::BackTab,
        crossterm::event::KeyModifiers::SHIFT,
    );
    assert!(!session.handle_key(event), "BackTab must not close the editor");
}

This would require tempfile in dev-dependencies, which might not be present. If that's a blocker, the current unit tests are sufficient — this is a nice-to-have, not a requirement.

---

Pre-existing concerns (not introduced by this PR, not blocking)

• Esc polling adds ≤20 ms latency on save: line 118 — crossterm::event::poll(Duration::from_millis(20)) runs on every Esc keypress. The 20 ms is probably imperceptible but worth knowing if users report sluggish Esc response.
• Swallowed I/O errors: line 118 — .unwrap_or(false) silently treats a crossterm::poll I/O error as "no follow-up key"; .unwrap_or(false) on event::read() at line 120 does similarly. Both are pre-existing and the behavior (treat error as no-op) is defensively correct for a TUI, but errors are invisible.

---

What's good

• Minimal blast radius — one guard, one filter function, no refactoring
• Thorough doc comment explaining the root cause and pointing to the source of truth
• Test names are descriptive and test purpose matches implementation
• matches!() macro is the right idiom for a multi-arm boolean pattern
• The PR description accurately identifies the mechanism (panic hook restoring terminal appearing as "exit")

Overall: LGTM with the Esc nit as optional cleanup. The whitelist maintenance concern is worth acknowledging but doesn't block merging.

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 1m 22s —— View job

---

PR Review

• Read changed files and git diff
• Analyze code for correctness, idioms, test coverage, security, and performance
• Post review feedback

---

Summary

The second commit (bf4c681) has addressed all three nits raised in the initial review: the SYNC WITH EDTUI Cargo.toml marker is in place, Esc is correctly omitted from the whitelist (with explanation), and the end-to-end handle_key_shift_tab_does_not_panic_or_exit test closes the call-site coverage gap. The PR is in good shape.

A few remaining observations, none blocking:

---

Issues

1. Test count in PR description is stale (cosmetic only)

The body says "3 new regression tests" but the diff now includes 4: back_tab_is_filtered_before_edtui, other_unsupported_codes_are_filtered, supported_codes_pass_the_filter, and handle_key_shift_tab_does_not_panic_or_exit. Not a code issue, but worth knowing for the squash commit message.

2. Follow-up event is consumed even on poll error (minor, pre-existing)

editor.rs:122–125

if crossterm::event::poll(Duration::from_millis(20)).unwrap_or(false) {
    let _ = crossterm::event::read();   // error silently discarded
    return false;
}

poll returning an error and unwrap_or(false) short-circuiting means the read() is only reached when poll succeeds, which is correct. The let _ on read() swallows a potential I/O error, but for a TUI that's defensively appropriate — this is pre-existing and not introduced here. Just noting it for completeness.

3. is_edtui_supported is not #[inline] (micro-nit)

The function is called on every key event in a TUI hot path. The compiler will almost certainly inline it anyway given its size, but an explicit #[inline] would make the intent clear. Worth considering; not required.

---

What's good

• The SYNC WITH EDTUI Cargo.toml comment is exactly the right maintenance marker — future version bumps now have an explicit audit checklist
• Removing Esc from the whitelist and documenting why in the doc comment avoids misleading future readers
• handle_key_shift_tab_does_not_panic_or_exit exercises the full handle_key path; a refactor that accidentally drops the guard would be caught even if is_edtui_supported itself is intact
• tempfile being in both [dependencies] and [dev-dependencies] means the integration test compiles cleanly
• matches!() with glob import is the right Rust idiom here — readable and no allocation
• Blast radius is minimal: one guard, one filter, no structural changes to the editor

Overall: LGTM. All prior nits addressed. Ready to merge.