fix: avoid Exec restore panic with WithInput(nil)#1680
fix: avoid Exec restore panic with WithInput(nil)#1680aymanbagabas merged 1 commit intocharmbracelet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a panic when running ExecProcess in a program configured with WithInput(nil) by ensuring terminal restoration does not attempt to reinitialize the input reader when input is explicitly disabled.
Changes:
- Update
Program.RestoreTerminal()to only callinitInputReader(false)whenp.input != nil. - Add a regression test covering
ExecProcesswithWithInput(nil)to prevent reintroducing the panic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tea.go |
Prevents RestoreTerminal() from reinitializing the cancel reader when input is disabled (p.input == nil). |
exec_test.go |
Adds a regression test that runs ExecProcess under WithInput(nil) to ensure no panic and no error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return execFinishedMsg{err} | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
testExecNoInputModel embeds testExecModel, but it relies on the promoted Update method whose receiver is *testExecModel. That Update returns m (a *testExecModel), so after the first update the program’s model type changes from *testExecNoInputModel to *testExecModel. This is a bit brittle/confusing for a regression test; consider giving testExecNoInputModel its own Update/View (or avoid embedding) so it consistently returns the same concrete model type.
| func (m *testExecNoInputModel) Update(msg Msg) (Model, Cmd) { | |
| switch msg := msg.(type) { | |
| case execFinishedMsg: | |
| if msg.err != nil { | |
| m.err = msg.err | |
| } | |
| return m, Quit | |
| } | |
| return m, nil | |
| } | |
| func (m *testExecNoInputModel) View() View { | |
| return m.testExecModel.View() | |
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1680 +/- ##
==========================================
+ Coverage 55.38% 55.64% +0.26%
==========================================
Files 25 25
Lines 1309 1310 +1
==========================================
+ Hits 725 729 +4
+ Misses 493 491 -2
+ Partials 91 90 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@lawrence3699 Thank you so much! |
CONTRIBUTING.md.Summary
Fix the
WithInput(nil)+ExecProcesspanic reported in the#761discussion.Problem
#761recommendstea.WithInput(nil)as the workaround when there is no TTY available. That works until a program executestea.ExecProcess(...)and returns to Bubble Tea.At that point
RestoreTerminal()always calledinitInputReader(false), even when input had been explicitly disabled. That rebuilds the cancel reader withp.input == niland panics incancelreader.Minimal repro:
Fix
Make
RestoreTerminal()match the initial startup path inRun()and only reinitialize the input reader whenp.input != nil.Added
TestTeaExecWithNilInputto cover the regression.Validation
go test -run 'TestTeaExec|TestTeaExecWithNilInput' .go test ./...