fix(exec): clear View before releasing terminal for subprocess - #1762
Open
zhaoxinyi02 wants to merge 1 commit into
Open
fix(exec): clear View before releasing terminal for subprocess#1762zhaoxinyi02 wants to merge 1 commit into
zhaoxinyi02 wants to merge 1 commit into
Conversation
When ExecProcess (or Exec) runs a subprocess, the renderer's final flush in releaseTerminal wrote the last View() output to stdout. In inline mode this content persisted in the terminal after the subprocess exited, forcing users to work around it by returning an empty string from View() before issuing ExecProcess (see the workaround linked in charmbracelet#431). Root cause: exec() calls releaseTerminal(false), which calls stopRenderer(false) -> renderer.flush(true). That closing flush emits the current view to the output, so the View() rendered for the message that triggered the exec is written to the terminal right before the subprocess takes over. Fix: blank the current view in exec() before releasing the terminal, so the closing flush has no content to write. The new renderer.clearView clears Content and Cursor while preserving terminal-mode flags such as AltScreen. Preserving AltScreen matters: close() and start() key screen restoration off lastView.AltScreen, so a plain empty View{} (AltScreen false) would skip re-entering the alt screen on resume and cause a flicker. Preserving the flag keeps alt screen exit/re-enter symmetric with the non-exec path. Regression tests: - TestExecProcessDoesNotLeakViewOutput: inline program whose View() returns a marker until the subprocess finishes; asserts the marker never reaches stdout (and the post-exec view still does). - TestExecProcessAltscreenNoLeak: same in alt screen mode; asserts no leak and that alt screen enter/leave sequences stay balanced. Fixes charmbracelet#431 Co-Authored-By: Claude <noreply@anthropic.com>
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
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
Fixes #431.
When
ExecProcess(orExec) runs a subprocess, the renderer's final flush wrote the currentView()output to stdout before handing the terminal to the subprocess. In inline (non-alt-screen) mode this content persisted in the terminal after the subprocess exited, which is why the documented workaround is to return an empty string fromView()right before issuingExecProcess.Root cause
exec()callsreleaseTerminal(false), which callsstopRenderer(false)->renderer.flush(true). That closing flush emits the current view to the output. The view rendered for the message that triggered the exec is therefore written to the terminal immediately before the subprocess takes over.Concretely, with a program whose
View()returnsEXEC_MARKER, the output captured around the exec hand-off is:The
EXEC_MARKERcontent is written to stdout and (for multi-line views, whereclose()only erases from the last line down) persists after the subprocess exits.The fix
Blank the current view in
exec()before releasing the terminal, so the closing flush has no content to write:clearViewis a new renderer method that clearsContentandCursorwhile preserving terminal-mode flags such asAltScreen.Why preserve
AltScreenclose()andstart()key alt-screen exit/re-enter offlastView.AltScreen:close()only writes the exit-alt-screen sequence whenlastView.AltScreenis true.start()only re-enters the alt screen on resume whenlastView.AltScreenis true.A plain
render(View{})flipsAltScreento false, so on resumestart()skips re-entering the alt screen and it is only re-entered on the next render flush - a visible flicker, and the screen-mode bookkeeping diverges from the non-exec path. Preserving the flag keeps the alt-screen exit/re-enter symmetric with normal suspend/resume and avoids the flicker.Tests
TestExecProcessDoesNotLeakViewOutput- inline program whoseView()returns a marker until the subprocess finishes and a different marker afterwards; asserts the pre-exec marker never reaches stdout while the post-exec view still renders. UsesWithFPS(1)so the only flushes during the short test are the exec hand-off and shutdown, keeping the assertion deterministic.TestExecProcessAltscreenNoLeak- same in alt-screen mode; asserts no content leak and that alt-screen enter/leave sequences stay balanced (the terminal is left in a clean state).Both tests fail on
mainand pass with this change.go test ./...,go vet,gofumpt, andgoimportsare clean.Notes
There is an older open PR (#1687) that addresses the same symptom by rendering an empty
View{}. This change takes a different approach - clearing content viaclearViewwhile preservingAltScreen- to avoid the alt-screen resume flicker described above, and adds the regression tests that #1687 lacks. Happy to coordinate/close in favor of whichever the maintainers prefer.Checklist
go test ./...passesgo vet,gofumpt,goimportsclean