From de78e4d8bc40035314bf50651f9ccd6e4c1eec0d Mon Sep 17 00:00:00 2001 From: Matt White Date: Mon, 4 May 2026 14:46:48 -0600 Subject: [PATCH 1/2] fix(sync): hint SPACE/ENTER when interactive picker returns empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gum choose --no-limit` exits 0 with empty stdout when the user presses ENTER without first toggling items with SPACE. The single-select source picker right before it accepts ENTER on its own, so muscle memory makes this trivially easy to do — and the wizard previously returned 1 silently in the targets case and printed only a bare "No types selected." in the types case, leaving users to think the picker was broken. Both empty-selection paths now print the same SPACE/ENTER hint to stderr before returning 1. Adds two regression tests exercising the fallback path (CKIPPER_NO_GUM=1) which mirrors the gum empty-stdout branch. --- lib/account/sync/dispatcher.zsh | 28 +++++++++++++++++++++++++-- lib/account/sync/dispatcher_test.bats | 23 ++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/lib/account/sync/dispatcher.zsh b/lib/account/sync/dispatcher.zsh index 1ee9ef7..c032d97 100644 --- a/lib/account/sync/dispatcher.zsh +++ b/lib/account/sync/dispatcher.zsh @@ -95,15 +95,39 @@ _ckipper_account_sync_run() { fi if (( ${#_CKIPPER_SYNC_TARGETS} == 0 )); then _CKIPPER_SYNC_TARGETS=( ${(f)"$(_ckipper_account_sync_pick_targets "$_CKIPPER_SYNC_FROM")"} ) - (( ${#_CKIPPER_SYNC_TARGETS} == 0 )) && return 1 + if (( ${#_CKIPPER_SYNC_TARGETS} == 0 )); then + _ckipper_account_sync_empty_select_hint "target accounts" + return 1 + fi fi _ckipper_account_sync_validate_accounts || return 1 local -a types types=( ${(f)"$(_ckipper_account_sync_resolve_types)"} ) - (( ${#types} == 0 )) && { echo "No types selected." >&2; return 1; } + if (( ${#types} == 0 )); then + _ckipper_account_sync_empty_select_hint "sync types" + return 1 + fi _ckipper_account_sync_run_targets types } +# Print a friendly hint when a multi-select picker returns nothing. +# +# `gum choose --no-limit` exits 0 with empty stdout when the user presses +# ENTER without first toggling items with SPACE — easy to do because the +# single-select source picker right before it accepts ENTER on its own. +# Without this hint, the wizard exits silently and users believe the picker +# is broken (see PR adding this for the reproduction). +# +# Args: $1 — what was being selected ("target accounts" | "sync types"). +# Returns: 0 always. +# Errors (stderr): "No selected." plus a SPACE/ENTER hint. +_ckipper_account_sync_empty_select_hint() { + { + echo "No $1 selected." + echo "Hint: in the picker, press SPACE to mark items, then ENTER to confirm." + } >&2 +} + # Walk every target and apply the resolved type list. # # Args: $1 — name of array variable holding type ids. diff --git a/lib/account/sync/dispatcher_test.bats b/lib/account/sync/dispatcher_test.bats index 735df2e..31c8afd 100644 --- a/lib/account/sync/dispatcher_test.bats +++ b/lib/account/sync/dispatcher_test.bats @@ -180,6 +180,29 @@ run_full() { [ "$status" -eq 0 ] } +# Regression: with no positional targets and an empty multi-select reply, +# the wizard previously returned 1 silently — users perceived the picker +# as broken. Both pickers now print a SPACE/ENTER hint before exiting. +@test "interactive sync prints a SPACE/ENTER hint when no targets are picked" { + setup_two_accounts + # Pipe enough blank lines to satisfy every prompt the fallback path asks + # for (source = "1", targets = "", types = ""). The empty targets line + # is what we're exercising. + run_full 'printf "1\n\n\n" | ckipper account sync' + [ "$status" -ne 0 ] + [[ "$output" == *"No target accounts selected"* ]] + [[ "$output" == *"press SPACE to mark items"* ]] +} + +@test "interactive sync prints a SPACE/ENTER hint when no types are picked" { + setup_two_accounts + # Source = "1", targets = "dst", types = "" (empty -> hint). + run_full 'printf "1\ndst\n\n" | ckipper account sync' + [ "$status" -ne 0 ] + [[ "$output" == *"No sync types selected"* ]] + [[ "$output" == *"press SPACE to mark items"* ]] +} + # Regression: when the user picks "View changes" then "Apply", the diff # output written by drill_down_loop must NOT pollute the captured action, # else the [[ "$action" == "apply" ]] check downstream silently skips apply. From 903ce2a71d9da03fbd0586d3a5e579b284b60b9c Mon Sep 17 00:00:00 2001 From: Matt White Date: Mon, 4 May 2026 15:35:08 -0600 Subject: [PATCH 2/2] =?UTF-8?q?fix(worktree):=20speed=20up=20`worktree=20l?= =?UTF-8?q?ist`=20and=20stop=20leaking=20branch=3D=E2=80=A6=20lines?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs surface together in `ckipper worktree list`: 1. Slow scan. `find` recursed unbounded under $CKIPPER_WORKTREES_DIR with only `*/node_modules/*` excluded, so the heavy build/cache trees (`dist`, `.next`, `target`, `__pycache__`, etc.) were walked in full. On a 6 GB / 380k-file worktrees tree the scan took ~700 ms. Pruning the known-heavy directory names brings it to ~30 ms (≈25× faster); bounding by depth was not viable because branches contain slashes (e.g. `feature/OGD-320-…`). 2. Spurious `branch='…'` lines under each bullet. `local branch` (no `=value`) inside the per-iteration loop body behaves like `typeset -p branch` once the variable already carries a value from a prior iteration. Hoist `branch` (and the other loop locals) above the `while` and assign without `local` inside, matching the existing precedent in lib/account/sync/preview.zsh. Adds three regression tests: - pruning skips `.git` files inside node_modules / dist / __pycache__ - branches with slashes still resolve (no depth bound) - output never contains literal `branch=` lines --- lib/worktree/worktree.zsh | 47 +++++++++++++++++++++++---- lib/worktree/worktree_test.bats | 57 +++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/lib/worktree/worktree.zsh b/lib/worktree/worktree.zsh index 09beac9..adc3ae3 100644 --- a/lib/worktree/worktree.zsh +++ b/lib/worktree/worktree.zsh @@ -3,6 +3,18 @@ readonly _CKIPPER_WT_FIND_MAX_DEPTH=3 +# Directory names pruned during the worktree-list scan. Branches can contain +# slashes (e.g. `feature/foo`), so we cannot bound the search by depth — but +# we CAN skip the heavy build/cache trees that inflate scan time without +# containing real worktrees. Empirically takes a 6 GB / 380k-file worktrees +# tree from ~700 ms to ~30 ms. Also prunes `.git` directories (the parent +# repo's, not the worktree's `.git` *file*) so we don't descend into git's +# internal storage. Add new entries as the worktrees tree grows. +readonly _CKIPPER_WT_PRUNE_DIRS=( + .git node_modules .next .nuxt dist build target + .venv venv __pycache__ vendor .turbo .cache +) + # Print all worktrees under $CKIPPER_WORKTREES_DIR, grouped by project. # # Reads CKIPPER_PROJECTS_DIR and CKIPPER_WORKTREES_DIR globals. @@ -11,16 +23,19 @@ _ckipper_worktree_list_worktrees() { [[ ! -d "$CKIPPER_WORKTREES_DIR" ]] && return 0 local previous_project_for_grouping="" - find "$CKIPPER_WORKTREES_DIR" -name ".git" -type f -not -path "*/node_modules/*" 2>/dev/null \ + # Hoist loop locals out of the body: re-declaring `local var` (no =value) + # on a subsequent iteration causes zsh to print `var='prior_value'`, + # leaking lines onto the worktree list. + local wt_dir="" rel="" project="" after_first="" branch="" + _ckipper_worktree_find_worktree_git_files \ | sort \ | while IFS= read -r git_metadata_file; do - local wt_dir="${git_metadata_file:h}" - local rel="${wt_dir#$CKIPPER_WORKTREES_DIR/}" - local project="${rel%%/*}" - local after_first="${rel#*/}" + wt_dir="${git_metadata_file:h}" + rel="${wt_dir#$CKIPPER_WORKTREES_DIR/}" + project="${rel%%/*}" + after_first="${rel#*/}" [[ "$after_first" == "$rel" ]] && continue - local branch IFS=$'\t' read -r project branch < <(_ckipper_worktree_get_project_and_branch "$project" "$after_first") if [[ "$project" != "$previous_project_for_grouping" ]]; then @@ -32,6 +47,26 @@ _ckipper_worktree_list_worktrees() { done } +# Emit the `.git` files of every worktree under $CKIPPER_WORKTREES_DIR, +# pruning known-heavy directories (see _CKIPPER_WT_PRUNE_DIRS) so the scan +# does not descend into node_modules / dist / build / etc. Worktrees mark +# their root with a `.git` *file* (a gitdir reference), not a directory — +# we test for `-type f` to filter accordingly. +# +# Args: none. +# Returns: 0 always; prints absolute paths to `.git` files, one per line. +_ckipper_worktree_find_worktree_git_files() { + local -a prune_args=() + local d + for d in "${_CKIPPER_WT_PRUNE_DIRS[@]}"; do + (( ${#prune_args} > 0 )) && prune_args+=(-o) + prune_args+=(-name "$d") + done + find "$CKIPPER_WORKTREES_DIR" \ + \( -type d \( "${prune_args[@]}" \) -prune \) \ + -o \( -type f -name .git -print \) 2>/dev/null +} + # Resolve project and branch from path components for nested-project worktrees. # # Args: diff --git a/lib/worktree/worktree_test.bats b/lib/worktree/worktree_test.bats index af01dd6..ba3cdc0 100644 --- a/lib/worktree/worktree_test.bats +++ b/lib/worktree/worktree_test.bats @@ -46,6 +46,63 @@ _run_worktree() { [[ "$output" =~ "Worktrees" ]] } +# Regression: scanning was unbounded and pruned only node_modules. With a +# 6 GB worktrees tree the scan took ~700 ms. Pruning the heavy build dirs +# (`dist`, `.next`, `target`, `__pycache__`, etc.) brings it to ~30 ms and +# avoids reporting any phantom `.git` files nested inside those trees. +@test "_ckipper_worktree_list_worktrees skips .git files inside pruned dirs" { + # Real worktree (should appear in output). + mkdir -p "$CKIPPER_WORKTREES_DIR/myapp/feature-x" + touch "$CKIPPER_WORKTREES_DIR/myapp/feature-x/.git" + + # Phantom .git files buried inside dirs we should prune. Each filename + # is unique so we can assert it does NOT show up by name. + mkdir -p "$CKIPPER_WORKTREES_DIR/myapp/feature-x/node_modules/pkg-a" + touch "$CKIPPER_WORKTREES_DIR/myapp/feature-x/node_modules/pkg-a/.git" + mkdir -p "$CKIPPER_WORKTREES_DIR/myapp/feature-x/dist/pkg-b" + touch "$CKIPPER_WORKTREES_DIR/myapp/feature-x/dist/pkg-b/.git" + mkdir -p "$CKIPPER_WORKTREES_DIR/myapp/feature-x/__pycache__/pkg-c" + touch "$CKIPPER_WORKTREES_DIR/myapp/feature-x/__pycache__/pkg-c/.git" + + _run_worktree "_ckipper_worktree_list_worktrees" + + [ "$status" -eq 0 ] + [[ "$output" == *"feature-x"* ]] + [[ "$output" != *"pkg-a"* ]] + [[ "$output" != *"pkg-b"* ]] + [[ "$output" != *"pkg-c"* ]] +} + +# Regression: branches contain slashes (`feature/foo`, `fix/bar`) so the +# scan must not be depth-bounded — the pruned find still has to surface a +# worktree whose `.git` lives several levels deep. +@test "_ckipper_worktree_list_worktrees finds worktrees whose branch name contains slashes" { + mkdir -p "$CKIPPER_WORKTREES_DIR/myapp/feature/OGD-320-deep-branch" + touch "$CKIPPER_WORKTREES_DIR/myapp/feature/OGD-320-deep-branch/.git" + + _run_worktree "_ckipper_worktree_list_worktrees" + + [ "$status" -eq 0 ] + [[ "$output" == *"feature/OGD-320-deep-branch"* ]] +} + +# Regression: `local branch` (no assignment) inside the per-worktree loop +# behaves like `typeset -p branch` once `branch` carries a value from a +# prior iteration, leaking literal `branch='…'` lines onto stdout. The fix +# is `local branch=""`. Two worktrees are needed to trigger iteration N>1 +# on the same loop scope. +@test "_ckipper_worktree_list_worktrees does not leak local-redeclare echoes" { + mkdir -p "$CKIPPER_WORKTREES_DIR/myapp/feature/one" + touch "$CKIPPER_WORKTREES_DIR/myapp/feature/one/.git" + mkdir -p "$CKIPPER_WORKTREES_DIR/myapp/feature/two" + touch "$CKIPPER_WORKTREES_DIR/myapp/feature/two/.git" + + _run_worktree "_ckipper_worktree_list_worktrees" + + [ "$status" -eq 0 ] + [[ "$output" != *"branch="* ]] +} + @test "_ckipper_worktree_remove_worktree fails when worktree path does not exist" { _run_worktree "_ckipper_worktree_remove_worktree myapp nonexistent-branch"