Drain terminal events before releasing raw mode - #985
Open
TBarregren wants to merge 1 commit into
Open
Conversation
Flyline can receive a terminal report after accepting Enter. Draining after App::run returns is too late because App's terminal has already restored echo mode. Restore terminal modes and drain while App remains alive, and cover the handoff with a deterministic PTY regression test.
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.
Fixes #984
Summary
Appstill owns the terminal and raw mode remains active.EndStatetransport type, since shutdown draining now happens insideApp::run.Root cause
The shutdown drain added in #971 runs in
get_command, afterApp::runreturns.App::runconsumesApp, so returning drops its terminal backend and restores cooked mode with echo before the drain begins. A FocusIn report arriving in that interval can be echoed as^[[I; consuming it afterward prevents it from becoming input, but cannot undo the visible output.This change sends the terminal reset sequences and performs the existing conditional drain at the end of
App::run, beforeselfis dropped. The shell still receives the sameExitStateafter the terminal handoff is complete.Regression coverage
The new test runs Flyline as a real Bash loadable builtin behind a pseudo-terminal, emulates the terminal queries used during startup, submits an empty command, and injects a FocusIn report at the focus-tracking shutdown boundary. It fails on the unmodified code with
^[[I[flyline inserted newline]and passes when the drain runs before raw mode is released.The test is part of the existing Docker Bash integration targets, so CI will exercise it across the supported Bash versions.
Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-features -- -D warningscargo test --workspace --all-features --lib -- --skip history::tests::test_pipeline_history_entry_formatting— 1,031 passed, 0 failed, 1 ignoredTZ=UTC cargo test -p flyline history::tests::test_pipeline_history_entry_formatting -- --exact— passed; this existing test expects a UTC calendar date and fails under Europe/StockholmBash 3.2 could not be built locally because the
config.guessshipped with Bash 3.2 does not recognize Docker Desktop's ARM64 platform. The existing x86 GitHub Actions job will cover that target.