fix: unify keyboard handling and add q key support

[yimsk]

Summary

• Fix warning screen key handling (Enter/Space/q to dismiss)
• Add q key as quit shortcut globally
• Add :q / :quit command
• q acts as back in DetailView/DiffView (natural navigation)

Closes #60

Changes

• app.go: Add q to Quit keybinding, handle q as back for detail views
• command_input.go: Add :q / :quit command
• detail_view.go / diff_view.go: q returns to previous view
• profile_detail_view.go: q closes modal
• README.md: Update key bindings doc

[claude]

Code Review

Thank you for this PR! This is a nice UX improvement that addresses issue #60 and unifies keyboard handling across the application. Overall, the implementation is solid, but I've identified a few issues and suggestions.

Critical Issues

1. Logic Bug in app.go:206-214 - q Should Quit When No View Stack

Location: internal/app/app.go:206-214

Issue: The current logic for the q key (Quit keybinding) only quits when NOT in DetailView/DiffView, or when in those views but the view stack is empty. This creates inconsistent behavior:

case key.Matches(msg, a.keys.Quit):
    switch a.currentView.(type) {
    case *view.DetailView, *view.DiffView:
        if len(a.viewStack) > 0 {
            a.currentView = a.viewStack[len(a.viewStack)-1]
            a.viewStack = a.viewStack[:len(a.viewStack)-1]
            return a, a.currentView.SetSize(a.width, a.height-2)
        }
    }
    return a, tea.Quit

Problem: When you're in DetailView/DiffView with an empty view stack and press q, it goes to DetailView but returns nil instead of quitting. The fallthrough to tea.Quit only happens when viewStack is empty, but at that point you can't navigate back anyway, so the user is stuck.

Expected behavior: When in DetailView/DiffView:

• If view stack has items → navigate back
• If view stack is empty → quit the application

Suggested fix:

case key.Matches(msg, a.keys.Quit):
    switch a.currentView.(type) {
    case *view.DetailView, *view.DiffView:
        if len(a.viewStack) > 0 {
            a.currentView = a.viewStack[len(a.viewStack)-1]
            a.viewStack = a.viewStack[:len(a.viewStack)-1]
            return a, a.currentView.SetSize(a.width, a.height-2)
        }
        // If no view stack, fall through to quit
    }
    return a, tea.Quit

Or more explicitly:

case key.Matches(msg, a.keys.Quit):
    switch a.currentView.(type) {
    case *view.DetailView, *view.DiffView:
        if len(a.viewStack) > 0 {
            // Navigate back
            a.currentView = a.viewStack[len(a.viewStack)-1]
            a.viewStack = a.viewStack[:len(a.viewStack)-1]
            return a, a.currentView.SetSize(a.width, a.height-2)
        }
        // No view stack - quit application
        return a, tea.Quit
    }
    // For all other views, quit
    return a, tea.Quit

2. Warning Screen Dismissal Issue in app.go:117

Location: internal/app/app.go:117

Issue: The key comparison uses msg.String() == "space" but this is checking the string representation of the key, which may not match "space".

if keyMsg.Code == tea.KeyEnter || keyMsg.String() == "space" || keyMsg.String() == "q" {

Problem: In the bubbletea library, when you press the spacebar:

• msg.Code is tea.KeySpace
• msg.String() returns " " (a space character), not the string "space"

Suggested fix:

if keyMsg.Code == tea.KeyEnter || keyMsg.Code == tea.KeySpace || keyMsg.String() == "q" {

Or to be consistent with other parts of the code:

if keyMsg.Code == tea.KeyEnter || keyMsg.String() == " " || keyMsg.String() == "q" {

This is likely why the original issue #60 reported that neither Enter nor Space worked to dismiss the warning!

---

Minor Issues & Suggestions

3. Inconsistent Modal Handling in app.go:467

Location: internal/app/app.go:467

In the modal handler, you're now checking for q to close modals:

if view.IsEscKey(msg) || msg.Code == tea.KeyBackspace || msg.String() == "q" {

However, in profile_detail_view.go:35, you removed q from the case statement:

case "d":  // removed "q" from here
    return v, func() tea.Msg { return HideModalMsg{} }

Question: Is this intentional? The modal now handles q globally in app.go, so the ProfileDetailView no longer needs to handle it explicitly. However, the status line at profile_detail_view.go:55 still shows "Esc/d/q:close", which suggests q should work.

Observation: This seems correct since the modal handler in app.go now handles q globally. The status line is still accurate. But consider whether d should also be removed from the individual view and handled globally, or keep it view-specific for modal-specific shortcuts.

4. Command Input Autocomplete Missing

Location: internal/view/command_input.go:424-427

The code adds autocomplete for "quit" but only checks it with strings.HasPrefix("quit", input), which is backwards:

if strings.HasPrefix("quit", input) {
    suggestions = append(suggestions, "quit")
}

Issue: This checks if "quit" starts with the user's input, not the other way around. For example:

• Input "q" → strings.HasPrefix("quit", "q") → true ✓ (works)
• Input "qu" → strings.HasPrefix("quit", "qu") → true ✓ (works)
• Input "qui" → strings.HasPrefix("quit", "qui") → true ✓ (works)
• Input "quit" → strings.HasPrefix("quit", "quit") → true ✓ (works)
• Input "x" → strings.HasPrefix("quit", "x") → false ✓ (works)

Actually, I stand corrected - this logic is correct! It's checking if "quit" has the prefix of the user's input, which is the right way to do autocomplete. My apologies for the confusion. The logic is correct.

However, you might want to add the same for "q" alias:

if strings.HasPrefix("quit", input) {
    suggestions = append(suggestions, "quit")
}
if strings.HasPrefix("q", input) && input \!= "q" {
    suggestions = append(suggestions, "q")
}

Though suggesting "q" might be noisy since it would match many other things.

5. Unnecessary Change in app.go:166

Location: internal/app/app.go:166

-if a.showWarnings && \!a.warningsReady {
+if \!a.warningsReady {

Question: Why was the a.showWarnings condition removed? This changes the logic so that warningsReady is set to true even if warnings aren't being shown. While this might not cause bugs, it changes the semantics of the flag.

Original logic: "Mark warnings as ready only when we're showing warnings and receive the first window size"

New logic: "Mark warnings as ready when we receive the first window size, regardless of whether we're showing warnings"

Impact: Likely harmless, but the change isn't explained in the PR description. If it's intentional, it would help to document why.

---

Testing Concerns

6. Missing Test Coverage

The PR doesn't add any new tests for:

• The new q key behavior in DetailView/DiffView
• The :quit command functionality
• The warning screen dismissal with q
• Modal dismissal with q

Recommendation: Add tests to internal/app/app_test.go covering:

func TestQuitKeyInDetailView(t *testing.T) {
    // Test q navigates back when viewStack has items
    // Test q quits when viewStack is empty
}

func TestWarningScreenDismissWithQ(t *testing.T) {
    // Test q dismisses warning screen
}

And to internal/view/command_input_test.go:

func TestCommandInput_QuitCommand(t *testing.T) {
    // Test :q and :quit commands
}

---

Code Quality & Best Practices

Positive observations:

• The PR maintains consistency with existing code style
• Good use of type switches for view-specific behavior
• Documentation updates in README.md are clear and helpful
• The changes are focused and don't introduce unnecessary refactoring

Overall: This is a well-structured PR that addresses a real UX issue. The main concern is the logic bug in the quit key handling that could leave users stuck, and the warning screen dismissal bug that likely caused the original issue. Once these are fixed, this will be a solid improvement!

---

Recommendations Summary

Must fix:

1. Fix the quit key logic in app.go:206-214 to properly quit when view stack is empty
2. Fix the Space key check in app.go:117 to use tea.KeySpace or " "

Should consider:
3. Add test coverage for the new behavior
4. Clarify the intent of the warningsReady logic change

Nice to have:
5. Consider adding a comment explaining the quit key behavior for future maintainers

Let me know if you have any questions about these suggestions!

[yimsk]

Addressed review feedback in 82bf15d:

Re: items 1, 2 - Not bugs. The quit key logic correctly falls through to tea.Quit when viewStack is empty, and "space" is the correct key string in bubbletea v2 (not " ").

Changes made:

• Removed dead q key checks in DetailView/DiffView - app.go's Quit keybinding intercepts first, making these unreachable
• Added TestWarningScreenDismissal covering Enter/Space/q dismissal
• Added TestCommandInput_QuitCommand covering :q/:quit commands
• Simplified warningsReady logic with clarifying comment

All tests pass, lint clean.

[claude]

Code Review

Thanks for working on this fix for issue #60! The PR addresses important keyboard handling improvements. I've reviewed the changes and have some feedback:

Critical Issue

Bug in warning screen dismissal (app.go:117):

if keyMsg.Code == tea.KeyEnter || keyMsg.String() == "space" || keyMsg.String() == "q" {

The space key check is incorrect. In Bubbletea v2, when you press the space key, msg.String() returns (a space character), not the string "space". This means the space key won't actually dismiss the warning screen, which is the core bug reported in issue #60.

Fix: Change to:

if keyMsg.Code == tea.KeyEnter || keyMsg.Code == tea.KeySpace || keyMsg.String() == "q" {

This is confirmed by your own test at app_test.go:311, which correctly uses tea.KeySpace for the Code field, not checking String().

Code Quality

Positive aspects:

• Good test coverage with TestWarningScreenDismissal and TestCommandInput_QuitCommand
• Clean implementation of the q key for back navigation in detail/diff views
• Proper documentation updates in README
• Nice improvement to the warningsReady logic with a clear comment

Suggestions:

1. Inconsistent q key handling in ProfileDetailView (profile_detail_view.go:35):

   • The StatusLine says "Esc/d/q:close" but the code only handles "d"
   • The PR removes "esc" and "q" handling, leaving only "d", which conflicts with the status line
   • Either restore the "q" handler or update the status line to match

2. Test quality issue (app_test.go:312):

   • The test for 'q' key uses tea.KeyPressMsg{Code: 0, Text: "q"}
   • For consistency with other tests and better clarity, use: tea.KeyPressMsg{Code: 'q', Text: "q"}

Performance Considerations

No performance concerns. The changes are straightforward key event handling.

Security Concerns

None identified. This is UI navigation code with no security implications.

Test Coverage

Good test coverage overall, but:

• The test at app_test.go:311 will pass incorrectly because it tests with Code: tea.KeySpace, but the implementation checks String() == "space" which won't match when a real user presses space
• Consider adding an integration test that simulates actual key press events to catch these mismatches

Summary

The PR is well-structured and addresses the issue, but has a critical bug that will prevent the space key from working. The fix is simple (use keyMsg.Code == tea.KeySpace), and with a couple of minor consistency improvements, this will be ready to merge.

Recommended actions:

1. Fix the space key check in app.go:117
2. Fix the 'q' key handling inconsistency in ProfileDetailView
3. (Optional) Improve test consistency in app_test.go:312