feat: Add async context menu callback (onContextMenuOpenAsync)

[kshivang]

Summary

• Add new onContextMenuOpenAsync suspend callback parameter that waits for completion before displaying the context menu
• Enables use cases like refreshing dynamic menu items (e.g., AI assistant detection) before the menu is shown
• Add comprehensive documentation for onInitialCommandComplete and onContextMenuOpen callbacks
• Update examples to demonstrate async callback usage

Changes

• ProperTerminal.kt: Refactor right-click handler to support async callback
• TabbedTerminal.kt: Add parameter threading
• EmbeddableTerminal.kt: Add parameter threading
• SplitContainer.kt: Thread parameter through all composable functions
• docs/: Added documentation and release notes

Test plan

• Verify context menu opens after async callback completes
• Verify sync callback (onContextMenuOpen) still works
• Test with simulated delay in examples
• Verify no regression in context menu behavior

Closes #220

🤖 Generated with Claude Code

[claude]

Code Review: PR #222 - Async Context Menu Callback

Overview

This PR adds a new onContextMenuOpenAsync callback parameter that enables async operations before displaying context menus. The implementation is clean, well-documented, and maintains backward compatibility.

---

✅ Strengths

1. Clean Architecture

   • The refactoring in ProperTerminal.kt:810-934 uses a local function doShowContextMenu() to avoid code duplication
   • Proper value capture (currentHoveredHyperlink) prevents race conditions
   • Clear separation between sync and async code paths

2. Excellent Documentation

   • Comprehensive KDoc with clear use cases
   • Release notes (v1.0.73.md) follow project conventions
   • Examples in both embedded-example and tabbed-example demonstrate realistic usage
   • API reference updated in docs/wiki/API-Reference.md

3. Backward Compatibility

   • Optional parameter with default null - no breaking changes
   • Existing onContextMenuOpen still works as before
   • Clear precedence: async callback takes priority when provided

4. Thorough Parameter Threading

   • All 7 functions in SplitContainer.kt properly pass the parameter through
   • Consistent API across TabbedTerminal, EmbeddableTerminal, and ProperTerminal

---

🔍 Potential Issues & Suggestions

1. Race Condition with Hover State (Minor)

Location: ProperTerminal.kt:808, 813-814

While you correctly capture hoveredHyperlink early, the hover state could theoretically change during the async callback execution (e.g., user moves mouse away). This is likely acceptable UX, but consider if you want to:

• Option A: Keep current behavior (menu shows based on hover state at click time) ✅ Recommended
• Option B: Re-check hover state after async callback completes

Current behavior seems correct - the menu should be based on what was hovered when clicked, not what's hovered after async work.

2. Error Handling Missing (Moderate)

Location: ProperTerminal.kt:926-928

Problem: If onContextMenuOpenAsync throws an exception, the menu never appears and the user is left confused.

Suggested Fix:
Add try-catch around the async callback invocation to ensure the menu always appears, even if the callback fails. The menu should show regardless of callback success.

This ensures the menu always appears, even if the async operation fails.

3. No Timeout Protection (Low Priority)

If onContextMenuOpenAsync hangs indefinitely (e.g., network timeout, deadlock), the menu never appears. Consider adding optional timeout protection (e.g., 5 seconds) with withTimeout.

Note: This is low priority since apps should handle their own timeouts, but it provides a safety net for user experience.

4. Documentation: Behavior When Both Callbacks Provided (Minor)

Location: Release notes and KDoc

The release notes state: "If onContextMenuOpenAsync is provided, it takes precedence over onContextMenuOpen"

Looking at the code, only ONE callback is invoked, not both - they are mutually exclusive.

Suggestion: Clarify in KDoc that if both are provided, only the async one is called. Add a note like: "NOTE: If provided, onContextMenuOpen is ignored."

---

🧪 Testing Recommendations

The PR description mentions test plan items but they are unchecked. Consider testing:

1. ✅ Happy path: Async callback with delay - menu appears after delay
2. ✅ Exception handling: Callback throws exception - menu should still appear (after fix 🔍 Implement text search/find functionality (Ctrl+F) #2)
3. ✅ Null checks: Both callbacks null - menu appears immediately
4. ✅ Hover state: Right-click on link, move mouse during async work - menu should show for original link
5. ✅ Fast repeated clicks: Right-click twice quickly - should not show duplicate menus
6. ✅ Thread safety: Async callback modifies shared state - no crashes

---

🔒 Security Considerations

No security concerns identified. The callback is user-provided and runs in the same coroutine scope as the UI.

---

📊 Performance Considerations

• Positive: Using coroutines for async work is appropriate
• Concern: Long-running async operations block menu appearance - but this is by design
• Recommendation: Document expected callback duration (< 500ms recommended for good UX)

---

✨ Code Quality

• +1 for avoiding code duplication with doShowContextMenu() helper
• +1 for consistent parameter naming across all layers
• +1 for comprehensive examples showing realistic usage (AI detection)

---

📝 Summary

Recommendation: ✅ Approve with minor fixes

Required before merge:

1. ❗ Add try-catch error handling around async callback invocation (Issue 🔍 Implement text search/find functionality (Ctrl+F) #2)

Nice to have:
2. Clarify mutual exclusivity in KDoc (Issue #4)
3. Consider timeout protection for robustness (Issue #3)

Can merge as-is:

• Hover state capture behavior is correct (Issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1)

---

Great work on this feature! The use case (dynamic AI assistant detection) is compelling, and the implementation is solid. Once error handling is added, this will be a valuable addition to the API.

Closes #220

[claude]

PR Review: Async Context Menu Callback

This PR adds support for async callbacks before context menu display. Overall, this is a solid implementation with good documentation.

Strengths

1. Clean API Design - Clear separation between sync (onContextMenuOpen) and async (onContextMenuOpenAsync) callbacks with proper backward compatibility
2. Consistent Implementation - Parameter threading is consistent across all layers (TabbedTerminal, SplitContainer, ProperTerminal)
3. Good Refactoring - The doShowContextMenu() helper function eliminates code duplication and properly captures values before async execution
4. Excellent Documentation - Comprehensive release notes and updated API reference with real-world examples

Potential Issues

Coroutine Scope Lifecycle (ProperTerminal.kt:926-929)

• Verify the 'scope' variable is properly cancelled when terminal/tab closes
• Consider adding timeout to prevent indefinite delays (e.g., withTimeout(5000L))
• Document expected callback duration in API docs

UX Concern: No Visual Feedback

• If async callback takes 500ms-2s, users might right-click multiple times or think app is frozen
• Consider: loading indicator, timeout with fallback, or document duration limits (<200ms recommended)

Test Coverage

Missing unit tests for:

• Async callback cancellation when terminal closes
• Behavior when async callback throws exception
• Precedence when both callbacks provided

Suggestions for Follow-up

1. Add exception handling around onContextMenuOpenAsync.invoke() to show menu even if callback fails
2. Add timeout protection with withTimeout(5000L) to prevent indefinite delays
3. Document callback duration expectations in API docs (recommend <200ms for good UX)

Final Verdict

✅ Approve with minor recommendations

The code quality is good, backward compatibility is maintained, and documentation is comprehensive. The concerns above are medium/low priority and can be addressed in follow-up PRs. Current implementation is production-ready for callbacks under 200ms.

Great work!

---

Review conducted following BossTerm development guidelines from CLAUDE.md