fix: remap text/both input_mode from config file in TUI to avoid stdin conflict#193
Open
octo-patch wants to merge 1 commit intodnhkng:mainfrom
Open
Conversation
…n conflict (fixes dnhkng#188) When input_mode is set to "text" or "both" directly in the config YAML, the TUI would start a TextListener that reads from sys.stdin, competing with Textual's own stdin handling. This caused keyboard input to require multiple keystrokes before registering. The TUI's instantiate_glados() already handled this for the CLI --input-mode override but not for the config file value. This commit extends the remapping to also apply when the config file specifies input_mode: "text" (to "none") or "both" (to "audio"), so the TUI's Input widget handles text submission via submit_text_input() without stdin contention.
Contributor
📝 WalkthroughWalkthroughUpdated input-mode resolution logic in the Textual TUI's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/glados/tui.py (1)
1417-1427: Please lock the YAML path down with a regression test.The original bug only reproduced when
input_modecame from config rather than the CLI override. A focused test for the"text" -> "none"and"both" -> "audio"remaps here would keep the two paths from drifting again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/glados/tui.py` around lines 1417 - 1427, Add a regression test that verifies the remapping logic that converts glados_config.input_mode == "text" to "none" and "both" to "audio" when the TUI is running: simulate a config-only input (no CLI override) and assert that the function which mutates the updates dict (the code handling glados_config and updates in tui.py) sets updates["input_mode"] to "none" for "text" and to "audio" for "both"; also add negative tests ensuring CLI overrides prevent these remaps (i.e., when a CLI input_mode is present, updates should not be rewritten), and wire the tests to run under the same code-path used by the TUI startup so the regression is locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/glados/tui.py`:
- Around line 1406-1427: The code writes "none" into updates["input_mode"],
which violates GladosConfig.input_mode's Literal["audio","text","both"]; instead
avoid injecting "none" and add a TUI-only flag to suppress the TextListener:
leave updates["input_mode"] set to one of "audio"/"text"/"both" (use the same
mapping logic in the branches around _input_mode_override and
glados_config.input_mode) and add a separate boolean key such as
updates["tui_suppress_text_listener"] (or self._tui_suppress_text_listener)
where you currently set "none" to indicate the TUI should not start
TextListener; update Glados.from_config()/engine code to read that flag to
conditionally skip TextListener (affecting speech_listener/text_listener
startup) rather than changing input_mode's value.
---
Nitpick comments:
In `@src/glados/tui.py`:
- Around line 1417-1427: Add a regression test that verifies the remapping logic
that converts glados_config.input_mode == "text" to "none" and "both" to "audio"
when the TUI is running: simulate a config-only input (no CLI override) and
assert that the function which mutates the updates dict (the code handling
glados_config and updates in tui.py) sets updates["input_mode"] to "none" for
"text" and to "audio" for "both"; also add negative tests ensuring CLI overrides
prevent these remaps (i.e., when a CLI input_mode is present, updates should not
be rewritten), and wire the tests to run under the same code-path used by the
TUI startup so the regression is locked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1406
to
+1427
| if self._input_mode_override == "text": | ||
| # In TUI text mode, use "none" so neither audio stream nor TextListener | ||
| # is started. The TUI Input widget handles all text input, avoiding | ||
| # both high CPU from audio and stdin contention with TextListener. | ||
| updates["input_mode"] = "none" | ||
| elif self._input_mode_override == "both": | ||
| # In TUI+both mode, keep audio ASR active; the TUI Input widget handles | ||
| # text input, so TextListener is not needed (avoids stdin contention). | ||
| updates["input_mode"] = "audio" | ||
| else: | ||
| updates["input_mode"] = self._input_mode_override | ||
| elif glados_config.input_mode == "text": | ||
| # Config file sets input_mode: "text" but TUI is running — remap to "none" | ||
| # to prevent TextListener from competing with Textual for stdin, which causes | ||
| # keyboard input to require multiple keystrokes. The TUI Input widget submits | ||
| # text via submit_text_input() instead. | ||
| updates["input_mode"] = "none" | ||
| elif glados_config.input_mode == "both": | ||
| # Config file sets input_mode: "both" but TUI is running — remap to "audio" | ||
| # so audio ASR stays active while TextListener is suppressed to avoid stdin | ||
| # contention with Textual. Text input is handled by the TUI Input widget. | ||
| updates["input_mode"] = "audio" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "GladosConfig.input_mode declaration:"
sed -n '104,112p' src/glados/core/engine.py
echo
echo "Engine input_mode branches:"
rg -nP -C2 'self\.input_mode\s*(==|in)' src/glados/core/engine.py
echo
echo 'Current TUI remap block:'
sed -n '1403,1434p' src/glados/tui.py
echo
echo 'Any existing engine-side support for "none":'
rg -nP '"none"' src/glados/core/engine.py src/glados/tui.pyRepository: dnhkng/GLaDOS
Length of output: 4007
Don't inject an undeclared input_mode value.
Lines 1410 and 1422 write "none" to input_mode, but GladosConfig.input_mode is declared as Literal["audio", "text", "both"] and the engine only branches on those three values. When this config is passed to Glados.from_config(), the "none" value will violate the type contract and cause both speech_listener and text_listener to be silently skipped without explicit intent.
Either extend the input_mode Literal type to include "none" across config and engine layers, or suppress TextListener using a separate TUI-only flag while keeping input_mode within its declared contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/glados/tui.py` around lines 1406 - 1427, The code writes "none" into
updates["input_mode"], which violates GladosConfig.input_mode's
Literal["audio","text","both"]; instead avoid injecting "none" and add a
TUI-only flag to suppress the TextListener: leave updates["input_mode"] set to
one of "audio"/"text"/"both" (use the same mapping logic in the branches around
_input_mode_override and glados_config.input_mode) and add a separate boolean
key such as updates["tui_suppress_text_listener"] (or
self._tui_suppress_text_listener) where you currently set "none" to indicate the
TUI should not start TextListener; update Glados.from_config()/engine code to
read that flag to conditionally skip TextListener (affecting
speech_listener/text_listener startup) rather than changing input_mode's value.
Owner
|
@octo-patch can you comment on the issues raised by coderabbit? |
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
2 participants
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 #188
Problem
When
input_mode: "text"(or"both") is set directly in the config YAML file, the TUI starts aTextListenerthread that reads fromsys.stdin. This conflicts with Textual's own stdin handling, causing keyboard input to require multiple keystrokes before registering.The
instantiate_glados()method intui.pyalready correctly remaps"text"→"none"and"both"→"audio"wheninput_modeis passed as a CLI override (--input-mode text), but did not apply the same remapping when the value came from the config file itself.Solution
Added two
elifbranches after the existing CLI-override block to also remapinput_modewhen the config file specifies"text"or"both":"text"→"none": preventsTextListenerfrom competing with Textual for stdin; the TUIInputwidget submits text viasubmit_text_input()."both"→"audio": keeps audio ASR active while suppressingTextListenerto avoid stdin contention.Testing
Reproduced by setting
input_mode: "text"inglados_config.yamland runninguv run glados tui. Before the fix, each keypress required multiple attempts. After the fix, keyboard input registers correctly on the first keypress.Summary by CodeRabbit