fix: keep outer spinner alive across hook boundary#367
Merged
Conversation
4 tasks
Add pause_spinner/resume_spinner to the Output trait so CommandBridge can suspend the command-level spinner around HookExecutor::execute(). Without this, the hook's own indicatif MultiProgress fights the outer ProgressBar for stderr, and subsequent step-label updates (e.g. "Removing worktree at <path>") stay invisible during the filesystem-delete phase — leaving the user with an unexplained pause. CliOutput remembers the active spinner's message on pause and recreates a fresh spinner on resume with that message. finish_spinner deliberately does not touch paused_spinner_message: HookExecutor calls finish_spinner defensively before rendering, and clearing the saved state there would turn resume_spinner into a silent no-op. Regression coverage: - core::progress::tests — asserts run_hook brackets the executor with pause/resume entries in order - output::cli::tests — asserts the saved message survives the executor's defensive finish_spinner call Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
avihut
force-pushed
the
fix/worktree-deletion-indication
branch
from
46367d5 to
d25126b
Compare
This was referenced Apr 17, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
daft remove(and any command running a lifecycle hook while an outer spinner is active) fell silent during the filesystem-delete phase after the pre-remove hook finished, leaving an unexplained pause between the hook summary and theDeleted <branch>line.indicatif::MultiProgressshared stderr with the command-levelProgressBar, so the outer spinner's steady ticks no longer redrew once the hook renderer relinquished the terminal. Step-label updates likeRemoving worktree at <path>were being written to a spinner that had effectively stopped painting.CommandBridge::run_hooknow bracketsHookExecutor::executewithpause_spinner/resume_spinner.CliOutputstashes the active message on pause, clears the spinner cleanly, and rebuilds a freshProgressBaron resume with the preserved label.output.finish_spinner()before drawing its own UI. The initial patch clearedpaused_spinner_messageinsidefinish_spinner, silently defusing the subsequentresume_spinner.finish_spinnerno longer touches the paused-message field; ownership of that field belongs exclusively to the pause/resume pair.Test plan
cargo test -p daft --lib -- core::progress::tests::run_hook_brackets_executor_with_spinner_pause_resume— asserts pause/resume bracketsexecutor.executecargo test -p daft --lib -- output::cli::tests::finish_spinner_preserves_paused_message_across_hook_call— regression against the defensivefinish_spinnerclobbering saved statemise run clippy— cleanmise run fmt:check— cleandaft remove <branch>in a sandbox with aworktree-pre-removehook — expect a ticking spinner labeledRemoving worktree at <path>...during the previously silent pause--quietpaths still have no activity line for the fs-delete phaseNotes
--quietwere deliberately left out; they're a separate UX decision worth its own PR.OutputkeepBufferingOutput(TUI path) unchanged.🤖 Generated with Claude Code