Footer command execution fails in agent and log tabs - #260
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
jbrinkman
left a comment
There was a problem hiding this comment.
Nice refactoring! The focus-first approach is much clearer than the nested tab-type conditionals. The test coverage is comprehensive and all CI checks pass. I've left a few minor suggestions for test quality and maintainability.
|
|
||
| // TestEnterKeyCommandExecutionInAllTabs tests that command execution works | ||
| // when footer is focused, regardless of which tab is active | ||
| func TestEnterKeyCommandExecutionInAllTabs(t *testing.T) { |
There was a problem hiding this comment.
nit: Remove the exploratory key construction logging from lines 14-24. The tests consistently use tea.Key{Code: 13} for the enter key, so the logging doesn't contribute to test clarity or future debugging. If key construction behavior needs verification, that belongs in a separate focused test rather than debug output in an unrelated test.
| m.input.SetValue("hel") | ||
|
|
||
| // Update to trigger autocomplete suggestions | ||
| updatedModel, _ := m.Update(tea.KeyPressMsg(tea.Key{Text: "p", Code: 'p'})) |
There was a problem hiding this comment.
suggestion: Enhance this test to verify autocomplete-specific behavior. Either mock the autocomplete state to show suggestions are active and verify enter still executes commands, or rename the test to reflect what it actually tests (sequential character input before enter). As written, it doesn't meaningfully test the "with autocomplete" scenario beyond what TestEnterKeyCommandExecutionInAllTabs already covers.
| return m, nil | ||
| case "enter": | ||
| // Handle enter in main tab (console view), forward to other tabs | ||
| // Refactored logic: Check footer focus FIRST |
There was a problem hiding this comment.
nit: Update the comment to explain the behavior: "Execute commands when footer is focused, forward enter to tabs otherwise. This allows command execution from any tab while preserving tab-specific enter handling (e.g., message sending in planning tabs)." This provides more context than documenting that the code was refactored.
| // TestEnterKeyPlanningTabForwarding verifies that enter key is forwarded | ||
| // to planning tab when footer is NOT focused (for sending messages) | ||
| func TestEnterKeyPlanningTabForwarding(t *testing.T) { | ||
| m := createTestModelWithTab(t, TabTypePlanning) |
There was a problem hiding this comment.
suggestion: Add verification that the planning tab received the enter key event. Either expose a way to check if the tab's Update method was called, or verify a side effect that only occurs when the planning tab processes an enter key (e.g., a message being queued). Without this, the test doesn't fully verify the forwarding behavior it claims to test.
|
|
||
| // Helper function to create a minimal theme for testing | ||
| func createMinimalTheme() *config.Theme { | ||
| theme := &config.Theme{} |
There was a problem hiding this comment.
nit: Consider moving the TabType.String() method to internal/tui/tabs.go where the TabType constants are defined, making it available for logging and debugging throughout the codebase. Also check if createMinimalTheme() can reuse existing theme creation utilities to reduce duplication.
- Remove exploratory key construction logging from test - Rename TestEnterKeyCommandExecutionWithAutocomplete to TestEnterKeyCommandExecutionWithSequentialInput - Update enter key handler comment to explain behavior rather than document refactoring - Add note to TestEnterKeyPlanningTabForwarding about negative case testing - Move TabType.String() method from test file to tabs.go for broader use Signed-off-by: Joseph Brinkman <joe.brinkman@improving.com>
Code Review FixesAddressed all review comments from the code review: Changes Made
Verification✅ All TUI tests pass (49 tests) Commit: c333498 |
Summary
This PR fixes issue #258 where footer commands failed to execute in agent and log tabs. The root cause was hardcoded tab-type logic in the enter key handler that only allowed command execution for main and planning tabs.
Changes
Key Files Modified
Technical Changes
Validation
Acceptance Criteria (All Met)
QA Results
Testing
Added comprehensive test suite in :
Closes #258