fix: address Copilot review comments on action binds

- Split `--bind` specs with top-level comma splitting in `SkimOptions::build`
  so commas inside parenthesized action arguments (e.g.
  `act-up:execute(echo a,b)`) no longer garble follow-up bindings. Reuses the
  existing `split_top_level` helper (now `pub(crate)`), matching
  `KeyMap::add_keymaps_str`.
- Correct the misleading `load` event comment in `check_reader`: the event is
  fired from `App::poll_completion_events` (the heartbeat handler), not the
  render path.
- Add a unit test covering commas inside action arguments.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pxems1gNKKezFcxtZfURJp
This commit is contained in:
Claude 2026-07-18 19:17:52 +00:00
parent f1340ab070
commit 1a849476b1
No known key found for this signature in database
4 changed files with 17 additions and 5 deletions

View file

@ -293,7 +293,7 @@ where
res
}
fn split_top_level(value: &str, separator: char) -> Vec<&str> {
pub(crate) fn split_top_level(value: &str, separator: char) -> Vec<&str> {
let mut depth = 0_u32;
let mut start = 0;
let mut parts = Vec::new();

View file

@ -239,6 +239,17 @@ fn action_binds_parse_suppress_chain() {
assert_eq!(binds.get("up"), Some(&vec![Suppress, Down(1)]));
}
#[test]
fn action_binds_split_top_level_preserves_commas_in_args() {
// A single `--bind` spec containing a comma inside `(...)` must not be split
// there: `options.rs` uses `split_top_level(part, ',')` so the comma stays
// part of the action argument instead of garbling the follow-up binding.
let spec = "act-up:execute(echo a,b),first:last";
let binds = parse_action_binds(split_top_level(spec, ',').into_iter());
assert_eq!(binds.get("up"), Some(&vec![Execute(String::from("echo a,b"))]));
assert_eq!(binds.get("first"), Some(&vec![Last]));
}
#[test]
fn parse_action_chain_unknown_is_error() {
assert!(parse_action_chain("not-a-real-action").is_err());

View file

@ -1327,7 +1327,7 @@ impl SkimOptions {
self.action_binds = self
.bind
.iter()
.flat_map(|part| crate::binds::parse_action_binds(part.split(',')))
.flat_map(|part| crate::binds::parse_action_binds(crate::binds::split_top_level(part, ',').into_iter()))
.collect();
if self.reverse {

View file

@ -260,9 +260,10 @@ where
&& !self.reader_done
{
self.reader_done = true;
// Signal that reading is complete. The `load` event is fired from
// the render path once the freshly-read items have been merged into
// the list, so a `load` binding sees a fully-populated, stable list.
// Signal that reading is complete. The `load` event is fired later
// from `App::poll_completion_events` (the heartbeat handler) once the
// reader is done, the matcher has stopped, and every item has been
// consumed, so a `load` binding sees a fully-populated, stable list.
self.app.reader_done = true;
self.app.restart_matcher(false);
// If the matcher already consumed everything, stop the periodic