Pressing Enter confirms prompts with an uppercase default#93
Merged
carole-lavillonniere merged 3 commits intomainfrom Mar 10, 2026
Merged
Pressing Enter confirms prompts with an uppercase default#93carole-lavillonniere merged 3 commits intomainfrom
carole-lavillonniere merged 3 commits intomainfrom
Conversation
carole-lavillonniere
changed the title
carole-lavillonniere
requested review from
anisaoshafi and
silv-io
as code owners
📝 WalkthroughWalkthroughInput handling was changed to resolve pending selections via a new prioritized match: an "any" option, explicit "enter" key, an uppercase-label default, then case-insensitive key match. A new resolveOption helper and hasLetters (unicode-based) were added; tests expanded to cover these behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/ui/app_test.go (1)
242-310: Add a test for uppercase shortcut input too.The production change on Line 130 of
internal/ui/app.goalso acceptsYfor an option keyed asy, but the new coverage only exercises the Enter/default path. A smalltea.KeyRunes{'Y'}case would lock in the other advertised behavior from this PR.🧪 Suggested test
+func TestAppAcceptsUppercaseShortcutKey(t *testing.T) { + t.Parallel() + + app := NewApp("dev", "", "", nil) + responseCh := make(chan output.InputResponse, 1) + + model, _ := app.Update(output.UserInputRequestEvent{ + Prompt: "Open browser now?", + Options: []output.InputOption{ + {Key: "y", Label: "Y"}, + {Key: "n", Label: "n"}, + }, + ResponseCh: responseCh, + }) + app = model.(App) + + model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Y'}}) + app = model.(App) + if cmd == nil { + t.Fatal("expected response command for uppercase shortcut") + } + cmd() + + select { + case resp := <-responseCh: + if resp.SelectedKey != "y" { + t.Fatalf("expected y key, got %q", resp.SelectedKey) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for response on channel") + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app_test.go` around lines 242 - 310, Add a test exercising the uppercase shortcut input (pressing 'Y') for an option keyed "y": after creating the App and sending the UserInputRequestEvent (same pattern as TestAppEnterSelectsUppercaseLabelDefault), send a tea.KeyMsg with Type: tea.KeyRunes and Runes: []rune{'Y'} to app.Update, execute the returned command if non-nil, then assert the responseCh receives SelectedKey "y" and that app.inputPrompt.Visible() is false; you can add this as a new test function (e.g., TestAppUppercaseShortcutSelectsLowercaseKey) or extend the existing TestAppEnterSelectsUppercaseLabelDefault to include the tea.KeyRunes{'Y'} case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/app.go`:
- Around line 117-124: The current default-selection check in the loop over
a.pendingInput.Options (where it uses opt.Label == strings.ToUpper(opt.Label))
mistakenly treats non-letter labels like "1" or "?" as uppercase; update that
condition to require the label actually contains at least one letter before
treating it as the conventional uppercase default — e.g., add a guard that the
label has at least one letter (use unicode.IsLetter on runes or an ASCII A-Z
check) in the same if that checks opt.Label == strings.ToUpper(opt.Label), then
proceed to appendLine/formatResolvedInput and sendInputResponseCmd only when
both conditions pass.
---
Nitpick comments:
In `@internal/ui/app_test.go`:
- Around line 242-310: Add a test exercising the uppercase shortcut input
(pressing 'Y') for an option keyed "y": after creating the App and sending the
UserInputRequestEvent (same pattern as
TestAppEnterSelectsUppercaseLabelDefault), send a tea.KeyMsg with Type:
tea.KeyRunes and Runes: []rune{'Y'} to app.Update, execute the returned command
if non-nil, then assert the responseCh receives SelectedKey "y" and that
app.inputPrompt.Visible() is false; you can add this as a new test function
(e.g., TestAppUppercaseShortcutSelectsLowercaseKey) or extend the existing
TestAppEnterSelectsUppercaseLabelDefault to include the tea.KeyRunes{'Y'} case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0ff23b91-89db-46de-8963-766f6dd32495
📒 Files selected for processing (2)
internal/ui/app.gointernal/ui/app_test.go
anisaoshafi
reviewed
Mar 10, 2026
internal/ui/app.go
Outdated
| } | ||
| } | ||
| // fall back to the option with an uppercase label (conventional default) | ||
| for _, opt := range a.pendingInput.Options { |
Contributor
There was a problem hiding this comment.
Do we need to iterate twice on the pendingInput.Options (line 107), or could we move the new if block up there?
Seems like most of the things done in these two blocks are similar.
Collaborator
Author
There was a problem hiding this comment.
Good catch, there were 4 identical loops in there 🙈
Extracted to a helper func with unit tests: 6d36242
anisaoshafi
approved these changes
Mar 10, 2026
Contributor
anisaoshafi
left a comment
anisaoshafi
left a comment
There was a problem hiding this comment.
LGTM 🙌🏼 just added one question
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/app_test.go (1)
242-343:⚠️ Potential issue | 🟡 MinorAdd a regression test for uppercase rune matching.
These new tests cover the Enter-default branches well, but they never exercise the other behavior introduced by this PR: case-insensitive key matching. A regression where
tea.KeyRunes{'Y'}no longer resolves{Key: "y"}would still pass this suite.Proposed test
+func TestAppMatchesInputOptionsCaseInsensitively(t *testing.T) { + t.Parallel() + + app := NewApp("dev", "", "", nil) + responseCh := make(chan output.InputResponse, 1) + + model, _ := app.Update(output.UserInputRequestEvent{ + Prompt: "Open browser now?", + Options: []output.InputOption{ + {Key: "y", Label: "Y"}, + {Key: "n", Label: "n"}, + }, + ResponseCh: responseCh, + }) + app = model.(App) + + model, cmd := app.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Y'}}) + app = model.(App) + if cmd == nil { + t.Fatal("expected response command for uppercase keypress") + } + cmd() + + select { + case resp := <-responseCh: + if resp.SelectedKey != "y" { + t.Fatalf("expected y key, got %q", resp.SelectedKey) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for response on channel") + } + + if app.inputPrompt.Visible() { + t.Fatal("expected input prompt to be hidden after response") + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app_test.go` around lines 242 - 343, Add a regression test to exercise case-insensitive rune matching: when an input prompt contains an option with Key "y" and Label "y", sending a tea.KeyMsg whose Runes contain 'Y' (e.g., tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Y'}}) should produce the same response command and select "y"; update or add a test (e.g., TestAppEnterSelectsCaseInsensitiveRune) that constructs the App, sends output.UserInputRequestEvent with Options including {Key: "y", Label: "y"}, captures the responseCh, calls app.Update with the KeyRunes message, executes the returned cmd if non-nil, and asserts the responseCh yields SelectedKey "y" and that app.inputPrompt is hidden.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/ui/app_test.go`:
- Around line 242-343: Add a regression test to exercise case-insensitive rune
matching: when an input prompt contains an option with Key "y" and Label "y",
sending a tea.KeyMsg whose Runes contain 'Y' (e.g., tea.KeyMsg{Type:
tea.KeyRunes, Runes: []rune{'Y'}}) should produce the same response command and
select "y"; update or add a test (e.g., TestAppEnterSelectsCaseInsensitiveRune)
that constructs the App, sends output.UserInputRequestEvent with Options
including {Key: "y", Label: "y"}, captures the responseCh, calls app.Update with
the KeyRunes message, executes the returned cmd if non-nil, and asserts the
responseCh yields SelectedKey "y" and that app.inputPrompt is hidden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cc386c94-b482-4979-8e6f-696561a43952
📒 Files selected for processing (2)
internal/ui/app.gointernal/ui/app_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ui/app.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/app.go`:
- Around line 264-279: In resolveOption, the current early returns make
precedence depend on option order; instead do a single scan to capture pointers
for anyOption (opt.Key == "any"), enterOption (opt.Key == "enter"),
uppercaseDefault (opt.Label uppercase when msg.Type == tea.KeyEnter), and a
caseInsensitiveMatch (strings.EqualFold(msg.String(), opt.Key) when msg.Type !=
tea.KeyEnter), then apply the fixed precedence: if anyOption != nil return it;
else if msg.Type == tea.KeyEnter and enterOption != nil return enterOption; else
if msg.Type == tea.KeyEnter and uppercaseDefault != nil return uppercaseDefault;
else return caseInsensitiveMatch; update resolveOption to use those local
variables (referencing resolveOption, opt.Key, opt.Label, hasLetters,
strings.EqualFold, tea.KeyMsg) rather than returning mid-loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e199a86b-1ba4-4b4e-a116-60c75fa5f944
📒 Files selected for processing (2)
internal/ui/app.gointernal/ui/app_test.go
Comment on lines
+264
to
+279
| func resolveOption(options []output.InputOption, msg tea.KeyMsg) *output.InputOption { | ||
| var uppercaseDefault *output.InputOption | ||
| for i, opt := range options { | ||
| switch { | ||
| case opt.Key == "any": | ||
| return &options[i] | ||
| case msg.Type == tea.KeyEnter && opt.Key == "enter": | ||
| return &options[i] | ||
| case msg.Type == tea.KeyEnter && uppercaseDefault == nil && | ||
| opt.Label != "" && hasLetters(opt.Label) && opt.Label == strings.ToUpper(opt.Label): | ||
| uppercaseDefault = &options[i] | ||
| case msg.Type != tea.KeyEnter && strings.EqualFold(msg.String(), opt.Key): | ||
| return &options[i] | ||
| } | ||
| } | ||
| return uppercaseDefault |
There was a problem hiding this comment.
Honor the documented option precedence regardless of slice order.
Line 275 can return a case-insensitive key match before the loop has seen a later "any" option, and Line 270 does the same for a later "any" when Enter is pressed. That makes the result depend on option ordering even though this helper advertises a fixed priority. For example, [{Key: "y"}, {Key: "any"}] + y currently resolves to "y", not "any".
💡 One way to make the precedence order-independent
func resolveOption(options []output.InputOption, msg tea.KeyMsg) *output.InputOption {
+ var anyOption *output.InputOption
+ var enterOption *output.InputOption
var uppercaseDefault *output.InputOption
+ var keyMatch *output.InputOption
for i, opt := range options {
switch {
- case opt.Key == "any":
- return &options[i]
- case msg.Type == tea.KeyEnter && opt.Key == "enter":
- return &options[i]
+ case opt.Key == "any" && anyOption == nil:
+ anyOption = &options[i]
+ case msg.Type == tea.KeyEnter && opt.Key == "enter" && enterOption == nil:
+ enterOption = &options[i]
case msg.Type == tea.KeyEnter && uppercaseDefault == nil &&
opt.Label != "" && hasLetters(opt.Label) && opt.Label == strings.ToUpper(opt.Label):
uppercaseDefault = &options[i]
- case msg.Type != tea.KeyEnter && strings.EqualFold(msg.String(), opt.Key):
- return &options[i]
+ case msg.Type != tea.KeyEnter && keyMatch == nil && strings.EqualFold(msg.String(), opt.Key):
+ keyMatch = &options[i]
}
}
- return uppercaseDefault
+ if anyOption != nil {
+ return anyOption
+ }
+ if enterOption != nil {
+ return enterOption
+ }
+ if uppercaseDefault != nil {
+ return uppercaseDefault
+ }
+ return keyMatch
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func resolveOption(options []output.InputOption, msg tea.KeyMsg) *output.InputOption { | |
| var uppercaseDefault *output.InputOption | |
| for i, opt := range options { | |
| switch { | |
| case opt.Key == "any": | |
| return &options[i] | |
| case msg.Type == tea.KeyEnter && opt.Key == "enter": | |
| return &options[i] | |
| case msg.Type == tea.KeyEnter && uppercaseDefault == nil && | |
| opt.Label != "" && hasLetters(opt.Label) && opt.Label == strings.ToUpper(opt.Label): | |
| uppercaseDefault = &options[i] | |
| case msg.Type != tea.KeyEnter && strings.EqualFold(msg.String(), opt.Key): | |
| return &options[i] | |
| } | |
| } | |
| return uppercaseDefault | |
| func resolveOption(options []output.InputOption, msg tea.KeyMsg) *output.InputOption { | |
| var anyOption *output.InputOption | |
| var enterOption *output.InputOption | |
| var uppercaseDefault *output.InputOption | |
| var keyMatch *output.InputOption | |
| for i, opt := range options { | |
| switch { | |
| case opt.Key == "any" && anyOption == nil: | |
| anyOption = &options[i] | |
| case msg.Type == tea.KeyEnter && opt.Key == "enter" && enterOption == nil: | |
| enterOption = &options[i] | |
| case msg.Type == tea.KeyEnter && uppercaseDefault == nil && | |
| opt.Label != "" && hasLetters(opt.Label) && opt.Label == strings.ToUpper(opt.Label): | |
| uppercaseDefault = &options[i] | |
| case msg.Type != tea.KeyEnter && keyMatch == nil && strings.EqualFold(msg.String(), opt.Key): | |
| keyMatch = &options[i] | |
| } | |
| } | |
| if anyOption != nil { | |
| return anyOption | |
| } | |
| if enterOption != nil { | |
| return enterOption | |
| } | |
| if uppercaseDefault != nil { | |
| return uppercaseDefault | |
| } | |
| return keyMatch | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ui/app.go` around lines 264 - 279, In resolveOption, the current
early returns make precedence depend on option order; instead do a single scan
to capture pointers for anyOption (opt.Key == "any"), enterOption (opt.Key ==
"enter"), uppercaseDefault (opt.Label uppercase when msg.Type == tea.KeyEnter),
and a caseInsensitiveMatch (strings.EqualFold(msg.String(), opt.Key) when
msg.Type != tea.KeyEnter), then apply the fixed precedence: if anyOption != nil
return it; else if msg.Type == tea.KeyEnter and enterOption != nil return
enterOption; else if msg.Type == tea.KeyEnter and uppercaseDefault != nil return
uppercaseDefault; else return caseInsensitiveMatch; update resolveOption to use
those local variables (referencing resolveOption, opt.Key, opt.Label,
hasLetters, strings.EqualFold, tea.KeyMsg) rather than returning mid-loop.
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.
Example: Pressing Enter or y here will confirm as well
