Skip to content

feat(ui): shell progress spinner, and the zsh bugs found on the way - #180

Merged
MCamner merged 4 commits into
mainfrom
feat/ui-spinner
Aug 8, 2026
Merged

feat(ui): shell progress spinner, and the zsh bugs found on the way#180
MCamner merged 4 commits into
mainfrom
feat/ui-spinner

Conversation

@MCamner

@MCamner MCamner commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Three commits. The first is the feature that was asked for; the other two are bugs the same shell split kept producing.

1. ui_spinner — the shell surface had no progress primitive

mq-agent has wrapped slow work in console.status since it grew rich panels. A grep across all 213 shell files found two hits for a spinner, both false: ASCII art containing the word SPINNER, and watch.sh hiding the cursor. So every menu that shelled out to gh (~0.75s per call, measured), ollama (tens of seconds) or a test run simply went quiet — and a quiet terminal reads as a hang.

repos="$(ui_spinner "Hämtar dina repos från GitHub" gh repo list --limit 1000 ...)"

Three design decisions worth reviewing:

It gates on /dev/tty, not on stdout being a terminal. The first version used the existing mq_wants_plain_output check, which would have silently disabled the spinner in exactly the shape shell callers use — out="$(slow_command)". Frames go to /dev/tty, so stdout is irrelevant. Locked by step 6 (GOT:captured).

The cursor is left visible on purpose. Hiding it means restoring it, and the only reliable restore is an INT/TERM trap — which gitlaunch.sh:840 and mq-git-menu.sh:137 already use to put a repo back on its base branch. Clobbering that trades a cosmetic win for a real one.

Two limits follow from backgrounding the wrapped command, both documented: it cannot wrap something that prompts on stdin, and variables it sets do not reach the caller.

Wired into the GitHub repo picker as the first call site, which also fixes a silent failure there: an empty gh result used to fall through to fzf with nothing to pick from.

2. mqobsidian menu option 3 was broken under zsh

get_mqobsidian_manifest_path:2: BASH_SOURCE[0]: parameter not set
resolve_view_relative_path:3: command not found: jq
[mqobsidian][error] Requested view key is not defined in views.json: roadmap-doc

Three error lines, two bugs, both zsh.

bin/mqlaunch is bash, so command mode (mqlaunch obsidian doctor) resolved the manifest fine while the interactive menu — terminal/launchers/mqlaunch.sh, which is #!/bin/zsh — could not. Reading BASH_SOURCE inside a function made the split invisible until someone used the menu. Now resolved at source time: inside a zsh function even $0 is no help, it holds the function name.

The jq error was not a PATH problem on the machine — jq was installed at two locations, both on PATH. Instrumenting the guard printed PATH= empty. assert_view_target_exists declared local path, and in zsh $path is a special array tied to $PATH, so PATH was blank for its whole call tree, including the jq that reads the manifest. doctor_mqobsidian_views had both traps on one line (local root key rel type path status=0) — $status is read-only in zsh — so the doctor also worked from bash and died from the menu.

Worth noting separately: neither fault mentioned itself. The operator was told the view key was undefined. It is defined. jq is now checked up front and named.

3. The last unguarded BASH_SOURCE

dev_repo_path had the same pattern in a fallback branch that only runs with BASE_DIR unset. Latent rather than live, since the launcher always sets it — a reason to guard it, not to leave it.

Verification

$ tools/scripts/test-all.sh
[PASS] Shell lint passed at warning severity (196 files)
[PASS] All selftest checks passed.

Two new test files plus a step added to the dev-menu suite, all registered in tests/manifest.tsv and test-all.sh:

  • ui-spinner-smoke.sh — 8 steps, including a pty step (a spinner that is merely safe is a spinner that never animates) and a zsh step, since background jobs plus wait are exactly the kind of construct that diverges between shells
  • mqobsidian-manifest-shell-parity-smoke.sh — 8 steps; step 6 also greps the consumer lib to stop local path / local status coming back
  • dev-menu-smoke.sh step 7 — resolves under bash and zsh with BASE_DIR unset

Driven for real, not just tested:

$ printf '9\n3\nx\n' | MQOBS_OPENER=echo zsh terminal/launchers/mqlaunch.sh
[mqobsidian] Opening roadmap-doc → /Users/mansys/mqobsidian/docs/roadmap-token-reduction.md

The spinner against live gh in a pty: 10 frames drawn, line erased, 26 repos captured, exit 0.

🤖 Generated with Claude Code

MCamner and others added 4 commits August 8, 2026 14:26
mq-agent has wrapped slow work in console.status since it grew rich panels;
the shell half of the stack had no progress primitive at all. A grep across
all 213 shell files found two hits, both false: ASCII art containing the word
SPINNER, and watch.sh hiding the cursor. So every menu that shelled out to gh
(~0.75s per call), ollama (tens of seconds) or a test run simply went quiet,
and a quiet terminal reads as a hang.

ui_spinner wraps a command and animates while it works. It gates on /dev/tty
rather than on stdout being a terminal, which is what makes the common shell
shape work: repos="$(ui_spinner 'Fetching' gh repo list)" both animates and
captures, because frames never touch stdout.

The cursor is deliberately left visible. Hiding it means restoring it, and the
only reliable restore is an INT/TERM trap — which gitlaunch and the git menu
already use to put a repo back on its base branch. Clobbering that trades a
cosmetic win for a real one.

Two limits follow from backgrounding the wrapped command, and both are
documented: it cannot wrap something that prompts on stdin, and variables it
sets do not reach the caller.

Wired into the GitHub repo picker as the first call site, which also fixes a
silent failure there: an empty gh result used to fall through to fzf with
nothing to pick from.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Menu option 3 (open the roadmap doc) failed with three errors that looked
like three problems and were two, both of them zsh:

  get_mqobsidian_manifest_path:2: BASH_SOURCE[0]: parameter not set
  resolve_view_relative_path:3: command not found: jq
  [mqobsidian][error] Requested view key is not defined in views.json: roadmap-doc

bin/mqlaunch is bash, so command mode resolved the manifest fine; the
interactive menu runs terminal/launchers/mqlaunch.sh, which is zsh, and zsh
has no BASH_SOURCE. Reading it inside a function made the split invisible
until someone used the menu. Resolved at source time now — inside a zsh
function even $0 is no help, it holds the function name.

The jq error was not a PATH problem on the machine. assert_view_target_exists
declared `local path`, and in zsh $path is a special array tied to $PATH, so
PATH was empty for its whole call tree, including the jq that reads the
manifest. Renamed to `target`. doctor_mqobsidian_views had both traps on one
line (`local root key rel type path status=0`) — $status is read-only in zsh —
so the doctor worked from bash command mode and died from the menu.

Neither fault mentioned itself: the operator was told the view key was
undefined. It is defined. jq is now checked up front and named.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dev_repo_path's fallback branch read ${BASH_SOURCE[0]} inside a function,
which is unset under zsh — the same split that broke the mqobsidian manifest
reader from the menu while command mode kept working. Latent rather than
live, because the branch only runs with BASE_DIR unset and the launcher
always sets it; that is a reason to guard it, not to leave it.

Resolved at source time like the other two call sites, and the fallback now
returns non-zero instead of silently handing back a relative path if even
that fails.

The dev-menu smoke test asserts it under both shells with BASE_DIR unset,
which is the only way the branch is reachable at all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pty step failed in CI and passed locally, because quality.yml runs the
whole suite with MQ_NO_TUI=1 and ui_spinner honours it — so the step that
exists to prove frames reach a terminal was asserting them in a run where
drawing is deliberately off. The helper was right; the test's premise was not.

Steps 5 and 6 now clear the headless switches they are not testing, and a new
step 7 pins the behaviour that caused the confusion: MQ_NO_TUI=1 suppresses
the spinner. That is a real contract — it is what lets the suite run headless
— and it had no coverage.

Verified both ways: MQ_NO_TUI=1 ./tools/scripts/test-all.sh (the CI
invocation) and a plain local run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@MCamner
MCamner merged commit a569124 into main Aug 8, 2026
3 checks passed
@MCamner
MCamner deleted the feat/ui-spinner branch August 8, 2026 13:49
MCamner added a commit that referenced this pull request Aug 8, 2026
…181)

Wrapping the gh calls in ui_spinner was the ask; measuring them first changed
what the fix should be. Reading the nine fields of the merge plan took nine
`gh pr view` calls — 3.44s measured, all of it silent — against 0.47s for one
batched read. A spinner makes a wait legible, but not waiting is better, so
the batching is the fix and the spinner covers what is left.

Also folded in: the separate `gh pr view --json number` existence check was a
tenth call saying what the plan read already says by failing, and `gh pr
checks` ran twice, once to print and once to decide.

The batched fields are joined on U+001F rather than a tab. Tab counts as IFS
whitespace, so `read` collapses a run of them and one empty field shifts every
later field left — and reviewDecision is empty on any PR nobody reviewed,
which is most of them. A tab-separated read would have printed the URL as the
review decision. Verified against live gh on #180, where reviewDecision is in
fact empty: "Review: none" and the URL both land correctly.

The script is launched as its own process by the git menu, so it sources
mq-ui.sh itself and falls back to a no-op ui_spinner when the library is not
reachable. A missing spinner must not cost you the merge tool.

First test coverage this script has had — gitmerge-safe-smoke.sh covers the
local-merge sibling, not this one. The fake gh records every invocation, so
"one call, not nine" is asserted against behaviour rather than against how the
source reads, and the no-TTY guardrail and the declined confirmation are
pinned too.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant