Add ghostel-spinner-progress for animated indeterminate progress#214
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an optional spinner-based progress indicator for Ghostel’s OSC 9;4 progress integration, using spinner.el when available while preserving the existing plain-text indicator as a fallback.
Changes:
- Introduces
ghostel-spinner-progress, buffer-local spinner state, and sentinel cleanup to prevent spinner timers from outliving the shell process. - Updates
ghostel-progress-functiondefault selection to prefer the spinner handler whenspinner.elis onload-path, and addsghostel-spinner-typecustomization. - Adds ERT coverage for spinner behavior and updates docs (README + changelog) to describe the built-in handler.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
lisp/ghostel.el |
Adds spinner-backed progress handler, buffer-local spinner state, cleanup helper, and default selection logic. |
test/ghostel-test.el |
Adds unit tests for missing-spinner error path, idempotent indeterminate behavior, stop-on-set/remove behavior, and spinner-stop helper. |
README.md |
Replaces the previous spinner example with documentation for the new built-in handler and configuration. |
CHANGELOG.md |
Documents the newly added spinner-based progress support and customization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (should (eq 'fake-spinner ghostel--spinner)))))) | ||
|
|
||
| (ert-deftest ghostel-test-spinner-progress-set-stops-and-shows-percent () | ||
| "On `'set, the spinner is stopped and `mode-line-process' is the percent text. |
There was a problem hiding this comment.
Docstring quoting looks malformed: "'set" has an extra leading backtick before the quoted symbol. Consider changing this to set' (help-style) or 'set for readability.
| "On `'set, the spinner is stopped and `mode-line-process' is the percent text. | |
| "On `set', the spinner is stopped and `mode-line-process' is the percent text. |
| (should (equal " [50%]" mode-line-process)))))) | ||
|
|
||
| (ert-deftest ghostel-test-spinner-progress-remove-clears-modeline () | ||
| "On `'remove, the spinner stops and `mode-line-process' is nil." |
There was a problem hiding this comment.
Docstring quoting looks malformed: "'remove" has an extra leading backtick before the quoted symbol. Consider changing this to remove' (help-style) or 'remove for readability.
| "On `'remove, the spinner stops and `mode-line-process' is nil." | |
| "On `remove', the spinner stops and `mode-line-process' is nil." |
| [spinner.el](https://github.com/Malabarba/spinner.el) during | ||
| indeterminate progress (e.g. while Claude Code is working) and | ||
| shows percentage text for determinate states. spinner.el is a | ||
| soft dependency: when it is on the `load-path' at ghostel load |
There was a problem hiding this comment.
Markdown inline-code formatting is broken here: load-path' starts a code span with a backtick but never closes it (it closes with an apostrophe instead). This will render incorrectly and can swallow the rest of the paragraph in some Markdown renderers; use load-path` (backticks on both sides) or plain text instead.
| soft dependency: when it is on the `load-path' at ghostel load | |
| soft dependency: when it is on the `load-path` at ghostel load |
There was a problem hiding this comment.
Fixed in 9b975a3 — changed to backtick-on-both-sides for the markdown context.
|
|
||
| (ert-deftest ghostel-test-spinner-progress-indeterminate-starts-once () | ||
| "`ghostel-spinner-progress' creates the spinner once across repeat events. | ||
| Multiple `'indeterminate events during one working phase must not stack |
There was a problem hiding this comment.
Docstring quoting looks malformed: "'indeterminate" includes both a backtick and an apostrophe before the symbol name. If you meant to reference the symbol in a docstring, prefer either indeterminate' (Emacs help-style quoting) or just 'indeterminate.
| Multiple `'indeterminate events during one working phase must not stack | |
| Multiple `indeterminate' events during one working phase must not stack |
There was a problem hiding this comment.
Fixed in 9b975a3 — changed to `indeterminate'.
dakra
force-pushed
the
feat/spinner-progress
branch
3 times, most recently
from
d781471 to
3676ba9
Compare
Built-in handler for `ghostel-progress-function' that animates `mode-line-process' via spinner.el during indeterminate progress (typical for Claude Code, which only emits the indeterminate state) and falls back to the existing text indicator for set/error/pause/ remove. Stopping the spinner on any non-indeterminate state avoids the failure mode where spinner.el's timer overwrites a percentage display on its next tick. spinner.el is a soft dependency. When it is on the load-path at ghostel load time, `ghostel-progress-function' defaults to the new handler; otherwise it stays on `ghostel-default-progress'. The function itself defers `(require 'spinner nil t)' to first call and signals `user-error' with an install hint if missing, so users who pin the function explicitly get a clear message rather than a void function error. Buffer-local `ghostel--spinner' state is torn down by `ghostel--sentinel' alongside the existing redraw/input/link timers, so the spinner timer does not outlive the shell process. New `ghostel-spinner-type' defcustom (default `progress-bar') selects the spinner style; see `spinner-types' in spinner.el for choices. README's previous custom example had two latent bugs the built-in avoids: percentages were stomped by the next spinner tick, and the timer was never cleaned up on buffer kill. Replaced with a brief section pointing at the built-in plus the soft-dependency contract.
dakra
force-pushed
the
feat/spinner-progress
branch
2 times, most recently
from
f5d661e to
20356bf
Compare
Summary
ghostel-spinner-progress, a built-in handler forghostel-progress-functionthat animatesmode-line-processvia spinner.el during indeterminate progress (the only state Claude Code emits) and falls back to the existing text indicator forset/error/pause/remove.load-pathat ghostel load time,ghostel-progress-functiondefaults to the new handler; otherwise it stays onghostel-default-progress. The handler itself defers(require 'spinner nil t)to first call and signalsuser-errorwith an install hint if missing.ghostel-spinner-typedefcustom (defaultprogress-bar) selects the spinner style. Spinner state is buffer-local and torn down byghostel--sentinelalongside the existing redraw / input / link timers, so the timer can't outlive the shell process.Why not the README example
The previous README snippet had two latent bugs the built-in avoids:
mode-line-processevery ~100 ms, so on a'setevent the percentage display was stomped almost immediately. The built-in stops the spinner on any non-indeterminatestate before deferring to the text indicator.README updated to point at the built-in instead.
Test plan
make -j4 allclean (build, lint, 224/224 tests pass — 5 new spinner tests under the existing elisp test group, no native dep).'set, modeline cleared on'remove,ghostel--spinner-stophelper.M-x ghostel, runclaudeon a multi-step task, confirm modeline animates during work and clears on completion. (Reviewer please sanity-check on your own setup; reporter saw[...]text in the prior fix and asked for animation.)load-path, confirmghostel-progress-functiondefaults toghostel-default-progressand the existing text indicator behavior is preserved.