fix: stay on current view after profile/region change

[yimsk]

Summary

• Profile/region 変更時に viewStack を pop せず現在のビューに留まるよう修正
• viewStack 操作を popView() / navigateBack() / pushOrClearStack() に集約し重複排除
• 戻る操作で Init() を呼び出すことでビューが正しく初期化されるよう修正

Changes

• refreshCurrentView(): 現在ビューが Refreshable なら RefreshMsg を送信、そうでなければ SetSize のみ
• navigateBack(): pop した view に対して Init() + SetSize() を実行
• nil guard 追加（refreshCurrentView で currentView が nil の場合の安全対策）

Testing

• 4 new test cases for profile/region change behavior
• All 47 tests passing

[claude]

Pull Request Review: Fix Profile/Region Stay on Current View

Summary

This PR improves the user experience by keeping users on their current view when switching profiles or regions, rather than popping the view stack to find a refreshable view. The changes also consolidate view stack manipulation logic into well-named helper methods.

---

✅ Code Quality & Best Practices

Strengths:

1. Excellent refactoring - The consolidation of view stack operations into popView(), navigateBack(), and pushOrClearStack() eliminates code duplication and improves maintainability
2. Clear naming - Method names accurately describe their purpose
3. Good documentation - Comments explain the "why" behind changes, especially for refreshCurrentView() and navigateBack()
4. Consistent patterns - All navigation operations now use the same helper methods
5. Logging - Appropriate debug logging in navigateBack() and refreshCurrentView() aids debugging

Suggestions:

1. Minor: Type assertion safety - In refreshCurrentView() at line 613, the type assertion is safe, but consider documenting why canRefresh is checked separately from r.CanRefresh():

   r, canRefresh := a.currentView.(view.Refreshable)
   if canRefresh && r.CanRefresh() {

   This is correct (prevents nil pointer if assertion fails), but a comment might help future readers understand the double-check pattern.

---

✅ Potential Bugs & Issues

Good catches:

1. Nil guard in refreshCurrentView() (line 609) - Prevents potential panic if currentView is nil
2. Empty stack check in navigateBack() (line 537-539) - Returns nil instead of panicking
3. Calling Init() on navigation back (line 544) - Critical for stateful views to reinitialize properly

No bugs found - The implementation is solid and handles edge cases appropriately.

---

✅ Performance Considerations

Positive:

1. No performance regressions introduced
2. The refactoring reduces code size without adding overhead
3. Stack operations remain O(1) amortized

No concerns - Performance is unchanged or slightly improved due to reduced code paths.

---

✅ Security Concerns

No security issues identified - This is purely a UI/UX refactoring with no security implications.

---

✅ Test Coverage

Excellent test coverage:

1. 4 new comprehensive tests covering the main behavior changes:

   • TestProfileChangeStaysOnCurrentRefreshableView - Verifies staying on refreshable views
   • TestRegionChangeStaysOnCurrentRefreshableView - Same for region changes
   • TestProfileChangeFromNonRefreshableViewStaysOnCurrentView - Tests non-refreshable view behavior
   • TestRegionChangeFromNonRefreshableViewStaysOnCurrentView - Same for region changes

2. Edge case tests:

   • TestNavigateBackWithEmptyStack - Validates nil return for empty stack
   • TestRefreshCurrentViewWithNilView - Tests nil view guard
   • TestRefreshCurrentViewSendsRefreshMsgForRefreshableView - Verifies refresh message
   • TestRefreshCurrentViewKeepsNonRefreshableViewUnchanged - Tests non-refreshable handling

3. All 47 tests passing - No regressions introduced

Minor suggestion:
The test TestRefreshCurrentViewSendsRefreshMsgForRefreshableView at line 759 checks for a non-nil cmd but doesn't verify it's actually a RefreshMsg. Consider adding:

if cmd == nil {
    t.Fatal("Expected non-nil cmd for refreshable view")
}
// Optional: Execute cmd and verify it returns RefreshMsg
msg := cmd()
if _, ok := msg.(view.RefreshMsg); !ok {
    t.Errorf("Expected RefreshMsg, got %T", msg)
}

---

📊 Architecture Alignment

This change aligns well with the architecture documented in docs/architecture.md:

• Maintains the view stack pattern
• Properly respects the Refreshable interface
• Follows Bubbletea's Init() + SetSize() message pattern
• Uses structured logging appropriately

---

🎯 Overall Assessment

Rating: Excellent ⭐⭐⭐⭐⭐

This is a well-crafted PR that:

• Solves a real UX issue (users staying in context)
• Improves code quality through consolidation
• Has comprehensive test coverage
• Introduces no bugs or regressions
• Follows Go best practices

Recommendation: Approve and merge ✅

The minor suggestions above are optional improvements, not blockers. The PR is ready to merge as-is.

---

📝 Nitpicks (Optional)

1. Line 617: The log message could include whether a RefreshMsg was sent:

   log.Debug("refreshing current view", 
       "view", a.currentView.StatusLine(), 
       "refreshable", canRefresh && r.CanRefresh(),
       "sentRefreshMsg", canRefresh && r.CanRefresh())

   (Though admittedly the last two fields would be identical)

2. Test naming: The test names are descriptive but quite long. Consider abbreviating "FromNonRefreshableView" to "OnNonRefreshableView" for brevity, though this is purely stylistic.

---

Great work! This is exactly the kind of refactoring that improves both UX and code maintainability. 🚀