Command mode suggestions and alias resolution

[yimsk]

Summary

• Bash-style tab completion (common prefix → cycle)
• Show Enter destination with alias resolution
• Error display for unknown commands
• Dynamic input width (30→50 for long commands)
• Diff command: use ID instead of Name

Test plan

• Tests pass
• Manual testing with VHS tape

[claude]

PR Review: Command mode suggestions and alias resolution

Summary

This PR significantly improves the command mode UX by adding bash-style tab completion, alias resolution preview, error messaging, and dynamic input width. The changes are well-implemented with comprehensive test coverage.

---

✅ Strengths

Code Quality

• Excellent test coverage: Added 233 new lines of tests covering all major scenarios including bash-style completion, alias resolution, dynamic width, and edge cases
• Clean separation of concerns: resolveDestination() and resolvePrefixMatch() methods provide reusable logic
• Well-documented behavior: Test names clearly describe expected behavior (e.g., TestCommandInput_DiffTabCompletion_RealKeyInput)

UX Improvements

• Bash-style tab completion: Intuitive completion behavior that first expands to common prefix, then cycles through suggestions
• Alias resolution preview: Shows where Enter will navigate (→ highlighted destination), excellent for user confidence
• Error messaging: Unknown commands now return proper error messages instead of silently failing
• Dynamic width: Input expands from 30→50 chars for long commands like diff with instance IDs

Architecture

• Consistent API change: Renaming from GetResourceNames() to GetResourceIDs() correctly reflects that diff should use IDs (more stable than names)
• Interface-based design: DiffCompletionProvider properly abstracts completion logic

---

🔍 Code Quality & Best Practices

command_input.go:130-174 (Tab/Shift+Tab handlers)

The bash-style completion logic is well-implemented, but has code duplication:

Observation: Lines 130-151 (tab) and 153-174 (shift+tab) have nearly identical logic except for the cycling direction.

Suggestion: Extract common logic to reduce duplication:

func (c *CommandInput) handleTabCompletion(reverse bool) {
    if len(c.suggestions) == 0 {
        c.updateSuggestions()
    }
    if len(c.suggestions) > 0 {
        current := c.textInput.Value()
        prefix := commonPrefix(c.suggestions)

        if len(prefix) > len(current) {
            // Expand to common prefix
            c.textInput.Reset()
            c.textInput.SetValue(prefix)
            c.suggIdx = 0
        } else {
            // Cycle through suggestions
            if reverse {
                c.suggIdx = (c.suggIdx - 1 + len(c.suggestions)) % len(c.suggestions)
            } else {
                // Current index is already correct for forward cycling
            }
            c.textInput.Reset()
            c.textInput.SetValue(c.suggestions[c.suggIdx])
            if !reverse {
                c.suggIdx = (c.suggIdx + 1) % len(c.suggestions)
            }
        }
    }
}

// Then in Update():
case "tab":
    c.handleTabCompletion(false)
    return nil, nil
case "shift+tab":
    c.handleTabCompletion(true)
    return nil, nil

---

🐛 Potential Bugs

command_input.go:620-631 (Exact match exclusion)

for _, svc := range c.registry.ListServices() {
    // Skip if input exactly matches service (already fully typed)
    if svc != input && strings.HasPrefix(svc, input) {
        suggestions = append(suggestions, svc)
    }
}

Issue: When user types "ec2" (exact match), suggestions exclude "ec2" but might still suggest "ec2/instances", "ec2/volumes" in the resource section. This creates inconsistency - the service disappears from suggestions but resources still show.

Impact: Minor UX inconsistency. Not a critical bug, but could confuse users.

Suggestion: Consider whether this exclusion is necessary. Traditional bash completion still shows exact matches. If you keep it, ensure consistency across all suggestion types.

---

command_input.go:310-315 (First match behavior)

for _, svc := range c.registry.ListServices() {
    if strings.HasPrefix(svc, servicePart) {
        matched = svc
        break  // Takes first match
    }
}

Issue: Takes the first prefix match from an unordered list. If multiple services match the prefix (e.g., "cloud" → "cloudformation", "cloudfront", "cloudwatch"), the result is non-deterministic and depends on iteration order.

Impact: Low - services are likely from a map which has undefined order in Go. Users typing "cloud" might get different results.

Suggestion: Either:

1. Sort services before matching for consistent behavior, or
2. Document that prefix matching requires enough characters to be unique, or
3. Require exact common prefix (most bash-like)

// Option 1: Sort first
services := c.registry.ListServices()
slices.Sort(services)
for _, svc := range services {
    if strings.HasPrefix(svc, servicePart) {
        matched = svc
        break
    }
}

---

⚡ Performance Considerations

command_input.go:184-190 (Dynamic width on every input change)

// Update suggestions on input change
c.updateSuggestions()

// Dynamic width: expand when input exceeds default width
inputLen := len(c.textInput.Value())
if inputLen > commandInputWidth {
    c.textInput.SetWidth(commandInputWidthMax)
} else {
    c.textInput.SetWidth(commandInputWidth)
}

Observation: SetWidth() is called on every keystroke, even when width doesn't need to change.

Impact: Minimal - this is a TUI with low event frequency, not a performance bottleneck.

Optimization (optional):

inputLen := len(c.textInput.Value())
targetWidth := commandInputWidth
if inputLen > commandInputWidth {
    targetWidth = commandInputWidthMax
}
// Only update if changed
if c.textInput.Width != targetWidth {
    c.textInput.SetWidth(targetWidth)
}

---

command_input.go:746-764 (commonPrefix algorithm)

The algorithm is O(n*m) where n=number of suggestions, m=length of common prefix. This is fine for small suggestion lists (typically <50 items), but could be optimized.

Current implementation: Correct and readable. No action needed unless profiling shows issues.

---

🔒 Security Considerations

No security issues identified. The code:

• ✅ Properly validates profile names with config.IsValidProfileName()
• ✅ Doesn't expose sensitive information in suggestions
• ✅ Uses parameterized command building (no injection risks)

---

📝 Test Coverage

Excellent test coverage with 264 added lines:

Well-tested scenarios:

• ✅ Bash-style tab completion (common prefix → cycle)
• ✅ Diff command completion with IDs
• ✅ Alias resolution in View()
• ✅ Ctrl+C exit
• ✅ Dynamic width expansion
• ✅ Common prefix calculation with edge cases

Potential test gaps:

1. Prefix match non-determinism: No test verifying behavior when multiple services have same prefix
2. Width boundary: Test uses long input but doesn't verify width actually changes to commandInputWidthMax
3. Shift+Tab backward cycling: Tested indirectly but could use dedicated test

Suggestion: Add test case:

func TestCommandInput_PrefixMatchMultipleServices(t *testing.T) {
    // Register services with common prefix: cloud*
    // Verify consistent behavior across multiple runs
}

---

📊 Changeset Analysis

Files changed: 11 files

• Core logic: command_input.go (+224, -83)
• Tests: command_input_test.go (+264, -31)
• Interface updates: resource_browser.go (8 lines) - clean rename
• Documentation: 4 VHS tape images + 1 tape script

Code metrics:

• Net: +411 lines (540 additions, 129 deletions)
• Test/Code ratio: 264 tests / 224 code ≈ 1.18 (excellent!)

---

🎯 Recommendations

Priority: High

1. Fix prefix match non-determinism (command_input.go:310-315) - sort services before matching

Priority: Medium

2. Reduce code duplication in tab/shift+tab handlers (command_input.go:130-174)
3. Clarify exact match exclusion behavior (command_input.go:620-631) - is this intentional?

Priority: Low

4. Add test for prefix match with multiple services
5. Consider optimizing width updates (only when changed)

---

✨ Overall Assessment

Rating: ⭐⭐⭐⭐½ (4.5/5)

This is a high-quality PR that significantly improves UX with minimal risk:

• Comprehensive test coverage gives confidence
• Clean architecture with proper separation of concerns
• Thoughtful UX improvements (bash-style completion, alias preview, error messages)
• Well-documented with VHS tapes for visual verification

The issues identified are minor and mostly architectural improvements rather than bugs. The prefix match non-determinism should be addressed, but overall this is ready to merge with minor fixes.

Recommendation: ✅ Approve with minor revisions (fix prefix match sorting)

---

📚 Additional Notes

VHS tapes: Great addition for visual documentation! The test-diff.tape clearly demonstrates the feature in action.

API consistency: The rename from GetResourceNames() to GetResourceIDs() is correct - IDs are more stable than names and align with how diff command works internally (using LeftID/RightID in DiffMsg).

Follow-up opportunities:

• Consider adding fuzzy matching hints in View() when no prefix match found
• Potential to extend dynamic width based on suggestion length, not just input length
• Could add history navigation (up/down arrows) for recently used commands

Great work on this feature! 🚀