feat: Add onInitialCommandComplete callback (#216)

[kshivang]

Summary

• Add onInitialCommandComplete: ((success: Boolean, exitCode: Int?) -> Unit)? callback to TabbedTerminal and EmbeddableTerminal
• Fires when the initialCommand finishes executing
• success: true if exit code is 0, false otherwise
• exitCode: command exit code (requires OSC 133 shell integration to detect)

Implementation

Uses one-shot CommandStateListener pattern:

1. After sending initialCommand, register a command state listener
2. On first onCommandFinished(exitCode) event, fire the callback
3. Immediately unregister the listener to ensure callback only fires once

Files Changed

• EmbeddableTerminal.kt: Added parameter and one-shot listener in initializeProcess()
• TabController.kt: Added parameter and one-shot listener in initializeTerminalSession()
• TabbedTerminal.kt: Added parameter, passed to first tab creation
• embedded-example/Main.kt: Demo usage showing status message update
• tabbed-example/Main.kt: Demo usage showing window title update

Test plan

• Verify callback fires when initial command completes (with OSC 133 shell integration)
• Verify callback receives correct exit code
• Verify callback only fires once per terminal
• Test with EmbeddableTerminal example
• Test with TabbedTerminal example

Note

Callback requires OSC 133 shell integration to detect command completion. See CLAUDE.md for shell setup instructions.

Closes #216

🤖 Generated with Claude Code

[claude]

Code Review - PR #218: Add onInitialCommandComplete callback

Overall, this is a well-implemented feature with clean code and good documentation. The implementation follows existing patterns in the codebase and properly handles the one-shot listener pattern. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Pattern Consistency: The one-shot listener pattern is cleanly implemented and mirrors the existing CommandNotificationHandler approach
2. API Design: The callback signature (success: Boolean, exitCode: Int?) -> Unit is intuitive and provides both convenience (success flag) and detail (exit code)
3. Self-Cleaning: The listener properly unregisters itself after firing, preventing memory leaks
4. Documentation: PR description and example usage are clear and comprehensive
5. Thread Safety: Uses CopyOnWriteArrayList for listener management (inherited from BossTerminal)
6. Non-Breaking: The parameter is optional with default null, maintaining backward compatibility

---

⚠️ Potential Issues & Suggestions

1. Race Condition: Multiple Commands Scenario (Medium Priority)

Location: EmbeddableTerminal.kt:686-697, TabController.kt:1190-1200

Issue: If the user types a command before the initialCommand finishes, the callback will fire for the user's command instead of the initialCommand.

Scenario:

1. initialCommand = "npm install" (takes 10 seconds)
2. User types "echo done" at 5 seconds
3. "echo done" completes first
4. Callback fires with exitCode=0 for "echo done", NOT "npm install"

Root Cause: OSC 133;D doesn't identify which command finished - just that a command finished.

Suggested Fix: Track command execution count to ensure we capture the first command:

if (onInitialCommandComplete \!= null) {
    var hasFirstCommandFired = false  // Capture once per terminal instance
    val completionListener = object : CommandStateListener {
        override fun onCommandFinished(exitCode: Int) {
            if (\!hasFirstCommandFired) {
                hasFirstCommandFired = true
                onInitialCommandComplete(exitCode == 0, exitCode)
                session.terminal.removeCommandStateListener(this)
            }
        }
    }
    session.terminal.addCommandStateListener(completionListener)
}

Alternative: Accept this behavior and document it clearly ("fires for first completed command, which is usually the initialCommand").

---

2. Missing Null Exit Code Handling (Low Priority)

Location: EmbeddableTerminal.kt:691, TabController.kt:1194

Issue: The callback signature uses exitCode: Int? (nullable), but the implementation always passes a non-null Int from onCommandFinished(exitCode: Int).

Current Code:

override fun onCommandFinished(exitCode: Int) {
    onInitialCommandComplete(exitCode == 0, exitCode)  // Always non-null
    //                                      ^^^^^^^^
}

Question: When would exitCode be null?

Options:

1. If it's never null, change signature to exitCode: Int (breaking change, but clearer)
2. If it could be null (e.g., timeout before OSC 133;D received), add timeout logic
3. Keep as-is for future-proofing (document why it's nullable)

Recommendation: Keep nullable for defensive programming, but add a comment explaining when it would be null (if applicable).

---

3. OSC 133 Dependency Documentation (Low Priority)

Location: Example files, PR description

Suggestion: The examples show the callback usage but don't explicitly warn developers about the OSC 133 requirement in the code itself. Consider adding a KDoc comment:

/**
 * Callback invoked when the [initialCommand] completes execution.
 * 
 * @param success true if exit code is 0, false otherwise
 * @param exitCode the command's exit code (requires OSC 133 shell integration)
 * 
 * **Important**: This callback requires OSC 133 shell integration to be configured
 * in the user's shell. Without it, the callback will never fire. See CLAUDE.md
 * for shell configuration instructions.
 * 
 * **Note**: If the user types commands before [initialCommand] finishes, the callback
 * may fire for the first *completed* command instead of [initialCommand].
 */
onInitialCommandComplete: ((success: Boolean, exitCode: Int?) -> Unit)? = null,

---

4. Edge Case: Shell Exits Before Command Completes (Low Priority)

Scenario: What if the shell/process exits before OSC 133;D is received?

Current Behavior:

• onExit callback fires (EmbeddableTerminal.kt:709, TabController.kt:1335)
• onInitialCommandComplete never fires (listener still registered but terminal destroyed)

Suggested Enhancement: Consider firing the callback with success=false, exitCode=null if the process exits before command completion is detected. This requires refactoring to track listener state across process lifecycle.

Alternative: Document this as expected behavior ("callback only fires if OSC 133;D is received before process exit").

---

🧪 Testing Recommendations

Based on the PR test plan, I'd add:

1. Race condition test: Type a command immediately after terminal opens (while initialCommand is running)
2. Shell exit test: Run exit as initialCommand and verify callback behavior
3. No OSC 133 test: Test with a shell that doesn't have OSC 133 configured (callback should never fire)
4. Long-running command test: sleep 10 to verify callback fires after completion
5. Failed command test: exit 1 to verify success=false, exitCode=1

---

📊 Code Quality Assessment

Criteria	Rating	Notes
Correctness	🟡 Medium	Works for happy path, race condition needs consideration
Maintainability	🟢 Good	Clean, follows existing patterns
Performance	🟢 Excellent	Minimal overhead, self-cleaning listener
Security	🟢 Good	No security concerns
Documentation	🟢 Good	PR description is thorough, inline docs could be enhanced
Testing	🟡 Medium	Examples provided, but unit tests would be ideal

---

🎯 Verdict

Recommendation: ✅ Approve with minor suggestions

This PR is functionally sound and ready to merge. The race condition is edge-case and could be addressed in a follow-up if it becomes a real-world issue. The implementation is clean and follows best practices.

Priority Actions (optional, can be done in follow-up PR):

1. Add KDoc comments explaining OSC 133 requirement and edge cases
2. Consider race condition mitigation if users report issues
3. Add unit tests for listener registration/unregistration logic

---

Great work on this feature! The one-shot listener pattern is elegantly implemented. 🎉

[claude]

Code Review: onInitialCommandComplete Callback

Summary: This PR adds a callback mechanism to notify when the initial command completes, which is a useful feature for workflow automation. The implementation is clean and follows the existing patterns in the codebase.

✅ Strengths

1. Clean API Design: The callback signature (success: Boolean, exitCode: Int?) -> Unit is intuitive and follows Kotlin conventions
2. Consistent Pattern: Uses the existing CommandStateListener infrastructure correctly
3. Thread-Safe: Uses CopyOnWriteArrayList for listener storage (already in place)
4. One-Shot Pattern: Self-unregistering listener prevents accidental double-fires
5. Good Documentation: Examples in both demo apps show practical usage
6. Non-Breaking: Optional parameter preserves backward compatibility

🔍 Potential Issues & Suggestions

1. Thread Safety Concern - Race Condition Risk ⚠️

Issue: The listener registration and command sending are not atomic. There's a window where the command could complete before the listener is registered.

// Current code in EmbeddableTerminal.kt:682-696
processHandle.write(initialCommand + "\n")  // Command sent

// Small time gap here - command could finish instantly

if (onInitialCommandComplete \!= null) {
    val completionListener = object : CommandStateListener { ... }
    session.terminal.addCommandStateListener(completionListener)  // Listener registered
}

For fast-completing commands (like echo "hello"), the OSC 133;D sequence could arrive before the listener is registered, causing the callback to never fire.

Recommended Fix:

// Register listener BEFORE sending command
if (onInitialCommandComplete \!= null) {
    val completionListener = object : CommandStateListener {
        override fun onCommandFinished(exitCode: Int) {
            onInitialCommandComplete(exitCode == 0, exitCode)
            session.terminal.removeCommandStateListener(this)
        }
    }
    session.terminal.addCommandStateListener(completionListener)
}

// Send command AFTER listener is registered
processHandle.write(initialCommand + "\n")

Apply to both files:

• EmbeddableTerminal.kt:682-696
• TabController.kt:1187-1201

---

2. Listener Cleanup on Early Exit 🧹

Issue: If the process crashes or the tab is closed before the command completes, the listener is never unregistered.

Impact: Minor memory leak (one orphaned listener object per terminal session).

Suggested Fix: Store a reference to the listener and clean it up in the exit handler:

// In initializeProcess() / initializeTerminalSession()
var initialCommandListener: CommandStateListener? = null

if (initialCommand \!= null && onInitialCommandComplete \!= null) {
    initialCommandListener = object : CommandStateListener {
        override fun onCommandFinished(exitCode: Int) {
            onInitialCommandComplete(exitCode == 0, exitCode)
            session.terminal.removeCommandStateListener(this)
            initialCommandListener = null  // Clear reference
        }
    }
    session.terminal.addCommandStateListener(initialCommandListener)
    processHandle.write(initialCommand + "\n")
}

// In process exit handler (around line 704 in EmbeddableTerminal.kt)
initialCommandListener?.let {
    session.terminal.removeCommandStateListener(it)
}

---

3. Exit Code Nullability Semantic ❓

Observation: The callback has exitCode: Int? but the implementation always passes a non-null Int:

override fun onCommandFinished(exitCode: Int) {
    onInitialCommandComplete(exitCode == 0, exitCode)  // exitCode is always non-null
}

Question: When would exitCode be null?

Options:

1. Keep as-is: Future-proof for cases where exit code can't be determined (no OSC 133 support)
2. Change to non-null: exitCode: Int to match reality
3. Document the null case: Add KDoc explaining when it's null

Recommendation: Option 1 (keep as-is) is fine since it's forward-compatible, but add a KDoc comment explaining the nullability contract.

---

4. Missing KDoc on Public API 📝

The new parameters lack documentation. Add KDoc for discoverability:

/**
 * Callback invoked when the initial command completes execution.
 * 
 * **Requires OSC 133 shell integration** - see CLAUDE.md for setup.
 * 
 * @param success true if exit code is 0, false otherwise
 * @param exitCode The command's exit code (0 = success, non-zero = failure).
 *                 Currently always non-null, but nullable for future compatibility
 *                 if exit code cannot be determined.
 */
onInitialCommandComplete: ((success: Boolean, exitCode: Int?) -> Unit)? = null

Add to:

• EmbeddableTerminal.kt:158
• TabbedTerminal.kt:100

---

5. Test Coverage 🧪

Missing Tests: No unit tests for the new callback logic.

Suggested Test Cases:

1. ✅ Callback fires with correct exit code for successful command
2. ✅ Callback fires with correct exit code for failed command
3. ✅ Callback only fires once (not on subsequent commands)
4. ✅ Callback doesn't fire if OSC 133 not configured
5. ✅ Listener is cleaned up after callback fires
6. ⚠️ Race condition: Fast-completing commands don't miss callback

Note: Testing #6 requires the ordering fix from Issue #1.

---

📊 Code Quality Assessment

Aspect	Rating	Notes
Correctness	⚠️ 7/10	Race condition risk, cleanup gaps
API Design	✅ 9/10	Clean, idiomatic Kotlin
Documentation	⚠️ 6/10	Missing KDoc, good examples
Thread Safety	✅ 8/10	Uses thread-safe collections
Performance	✅ 10/10	Zero overhead when unused
Security	✅ 10/10	No security concerns

---

🎯 Priority Recommendations

1. HIGH: Fix race condition (Issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1) - register listener before sending command
2. MEDIUM: Add KDoc to public API parameters (Issue ⚙️ Implement user settings and configuration system #4)
3. LOW: Add cleanup for orphaned listeners (Issue 🔍 Implement text search/find functionality (Ctrl+F) #2)
4. LOW: Document exitCode nullability (Issue 🔗 Implement hyperlink detection and click handling #3)
5. OPTIONAL: Add unit tests

---

🚀 Final Verdict

Approval Status: ✅ Approve with suggestions

The core implementation is solid and follows best practices. The race condition is the only blocking issue - it's a simple fix (swap two lines) but important for reliability.

Once the ordering is fixed, this is production-ready! Nice work implementing this feature cleanly. 👍

---

References:

• CommandStateListener.kt: Interface definition
• BossTerminal.kt:368-409: Thread-safe listener implementation
• EmbeddableTerminal.kt:682-696: EmbeddableTerminal implementation
• TabController.kt:1187-1201: TabbedTerminal implementation

[claude]

Code Review: Add onInitialCommandComplete callback (#216)

Summary

This PR adds a callback to notify when the initial command completes execution. The implementation is clean and follows the existing OSC 133 shell integration pattern. Overall, this is a well-implemented feature with good code quality.

---

✅ Strengths

1. Excellent Design Pattern

• Uses one-shot listener pattern that self-unregisters after firing
• Registers listener BEFORE sending command (critical for fast commands)
• Consistent implementation across both EmbeddableTerminal and TabController
• Proper use of CopyOnWriteArrayList for thread-safe listener management

2. Good Documentation

• Clear inline comments explaining the timing requirements
• PR description includes test plan and setup requirements
• Demo implementations in both example apps

3. Thread Safety

• Listener registration/removal uses thread-safe CopyOnWriteArrayList
• No race conditions identified in the listener lifecycle

4. Backward Compatibility

• Optional parameter with null default - no breaking changes
• Gracefully handles shells without OSC 133 support (callback simply won't fire)

[claude]

🔍 Issues & Concerns

1. Potential Race Condition with Listener Removal ⚠️

Location: EmbeddableTerminal.kt:691, TabController.kt:1196

The listener removes itself from within the callback. While CopyOnWriteArrayList makes this safe from ConcurrentModificationException, there's a subtle timing issue:

• If the callback throws an exception, the listener is never removed
• This could cause the callback to fire multiple times if subsequent commands complete

Fix: Wrap in try-finally to ensure cleanup even on exception.

2. Callback Exception Handling ⚠️

Impact: Medium

If onInitialCommandComplete throws an exception:

1. It could crash the listener dispatch loop in BossTerminal.kt:404
2. Other listeners might not get notified
3. The one-shot listener might not unregister (see issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1)

Recommendation: Add try-catch around the callback invocation for robustness.

3. Missing Tests ⚠️

Impact: Medium

The PR has no unit tests. While the feature is demonstrated in example apps, automated tests would help ensure:

• Callback fires exactly once
• Callback receives correct exit codes
• Callback doesn't fire if no initial command provided
• Callback handles exceptions gracefully
• Listener is properly cleaned up

Recommendation: Add tests in compose-ui/src/desktopTest/kotlin/

[claude]

🎯 Performance & Security

Performance ✅

• Minimal overhead: Single listener registration/removal per initial command
• No polling or timers
• Listener removed immediately after firing
• No memory leaks: CopyOnWriteArrayList handles concurrent modifications safely

Security ✅

• No security issues identified
• No user input validation needed (exit code is from shell)
• No privilege escalation vectors
• No injection vulnerabilities

---

📋 Recommendations

Priority: High

1. Add try-finally block around listener removal to prevent callback firing multiple times on exception
2. Add exception handling around callback invocation to prevent crashes

Priority: Medium

3. Add unit tests for the one-shot listener behavior
4. Document OSC 133 requirement more prominently (maybe in KDoc)

Priority: Low

5. Consider extracting shared listener logic to reduce duplication between EmbeddableTerminal and TabController

[claude]

🎉 Verdict

Approve with minor fixes recommended

This is a solid implementation that follows the project's existing patterns. The core logic is sound, but adding exception handling and tests would make it production-ready. The issues identified are edge cases that are unlikely to occur in normal usage, but should be addressed for robustness.

Suggested Code Fix

Apply this change to both EmbeddableTerminal.kt:686-694 and TabController.kt:1191-1199:

if (onInitialCommandComplete != null) {
    val completionListener = object : CommandStateListener {
        override fun onCommandFinished(exitCode: Int) {
            try {
                onInitialCommandComplete(exitCode == 0, exitCode)
            } catch (e: Exception) {
                println("ERROR: onInitialCommandComplete threw exception")
                e.printStackTrace()
            } finally {
                // Always unregister to ensure callback only fires once
                session.terminal.removeCommandStateListener(this)
                // Use tab.terminal for TabController
            }
        }
    }
    session.terminal.addCommandStateListener(completionListener)
}

---

Great work overall! The feature is well-designed and the implementation is clean. Just needs those small robustness improvements. 👍

[claude]

Code Review - PR #218: Add onInitialCommandComplete callback

Overall Assessment

LGTM with minor suggestions. This is a well-implemented feature that adds useful functionality for tracking initial command completion.

Strengths

1. Clean one-shot listener pattern - registers listener BEFORE sending command and immediately unregisters after firing
2. Consistent API design - callback signature provides both success check and detailed exit code
3. Good documentation - PR description clearly explains OSC 133 dependency
4. Proper threading - callback invoked from terminal event thread
5. Nice demo examples in both example apps

Code Quality Observations

• EmbeddableTerminal.kt lines 593-607: Shell integration injection is correctly added after building environment map
• Listener registration timing is correct: registered before command sent, crucial for fast commands
• mutableMapOf change (line 594) correctly allows mutation for shell integration injection

Minor Suggestions

1. Callback exception handling: If onInitialCommandComplete throws, listener won't unregister. Consider try-finally wrapper (low priority).

2. exitCode nullability: Callback uses Int? but CommandStateListener.onCommandFinished uses Int (non-nullable). Consider making it non-nullable for clarity since it's always present.

3. Debug logging: Add LOG.debug when callback fires to help users troubleshoot shell integration issues.

Security & Performance

• No security concerns: callback is user-provided, exit code is public info, no resource leaks
• Minimal performance impact: listener added/removed once, callback fires once

Test Coverage

PR has manual test plan but no automated tests. Consider adding tests for callback firing once, correct exit codes, null initial command handling.

Final Recommendation

Approve with minor improvements. Implementation is solid and ready to merge. Suggestions are non-blocking enhancements for follow-up PR if desired.

Priority: Consider making exitCode non-nullable
Nice-to-haves: try-finally wrapper, debug logging, KDoc

Great work maintaining consistency with existing patterns!

[claude]

Code Review: onInitialCommandComplete Callback (#216)

Summary

This PR successfully implements the onInitialCommandComplete callback feature for both TabbedTerminal and EmbeddableTerminal. The implementation is well-structured and follows the existing patterns in the codebase.

---

✅ Strengths

1. Consistent API Design: The callback signature (success: Boolean, exitCode: Int?) -> Unit is clean and intuitive, matching the requirements from issue Add onInitialCommandComplete callback to TabbedTerminal and EmbeddedTerminal #216.

2. One-Shot Pattern: Excellent use of self-unregistering listener pattern to ensure the callback fires exactly once:

   val completionListener = object : CommandStateListener {
       override fun onCommandFinished(exitCode: Int) {
           onInitialCommandComplete(exitCode == 0, exitCode)
           terminal.removeCommandStateListener(this)
       }
   }

3. Proper OSC 133 Integration: Correctly leverages the existing CommandStateListener infrastructure and shell integration auto-injection via ShellIntegrationInjector.

4. Thread-Safe Listener Management: The use of CopyOnWriteArrayList for myCommandStateListeners in BossTerminal.kt ensures safe concurrent modifications.

5. Good Documentation: KDoc comments clearly explain the OSC 133 requirement and callback semantics.

6. Example Usage: Both example apps demonstrate proper usage, showing practical patterns for UI updates.

---

⚠️ Potential Issues

1. Listener Registration Race Condition (Critical)

Location: EmbeddableTerminal.kt:699-708, TabController.kt:1188-1199

Issue: The listener is registered AFTER sending the command. If the command completes very quickly (e.g., echo "done"), the OSC 133;D sequence could arrive before the listener is registered.

Current Code:

// Send the command followed by newline
processHandle.write(initialCommand + "\n")

// Register listener AFTER command sent
if (onInitialCommandComplete != null) {
    val completionListener = object : CommandStateListener { ... }
    session.terminal.addCommandStateListener(completionListener)
}

Fix: Move listener registration BEFORE the write() call:

// Register one-shot listener BEFORE sending command
if (onInitialCommandComplete != null) {
    val completionListener = object : CommandStateListener {
        override fun onCommandFinished(exitCode: Int) {
            onInitialCommandComplete(exitCode == 0, exitCode)
            session.terminal.removeCommandStateListener(this)
        }
    }
    session.terminal.addCommandStateListener(completionListener)
}

// Send the command followed by newline
processHandle.write(initialCommand + "\n")

Probability: HIGH for fast-completing commands (echo, true, false, exit, etc.)

---

2. Callback Never Fires Without Shell Integration (Medium Priority)

Issue: If the user doesn't have OSC 133 shell integration configured (or has autoInjectShellIntegration: false), the callback will never fire, leaving the caller waiting indefinitely.

Current Behavior: Silent failure - no callback, no error, no logging.

Recommendations:

• Option A: Add timeout-based fallback (e.g., 30s) that calls onInitialCommandComplete(false, null) if no OSC 133;D received
• Option B: Log a warning when onInitialCommandComplete is provided but shell integration is disabled
• Option C: Document prominently in KDoc that callback requires OSC 133 (already done, but consider adding runtime warning)

Suggested Addition (low-impact logging):

if (onInitialCommandComplete != null && !settings.autoInjectShellIntegration) {
    LOG.warn("onInitialCommandComplete callback provided but autoInjectShellIntegration is disabled. " +
             "Callback will not fire unless shell is manually configured with OSC 133 support.")
}

---

3. Listener Leak on Terminal Disposal (Low Priority)

Issue: If the terminal is disposed before the initial command completes (e.g., user closes tab quickly), the listener remains registered but never unregistered.

Impact: Memory leak (one listener per tab closed before command completes). Since listeners are small objects and this scenario is rare, impact is minimal.

Mitigation: Consider weak references or disposal cleanup (not critical for this PR).

---

📋 Code Quality & Best Practices

✅ Good

• Follows existing code patterns (matches CommandNotificationHandler structure)
• Proper null safety handling
• Clear variable naming
• Consistent with project style (no emojis, concise comments)

🔧 Minor Suggestions

1. Redundant Comment (TabController.kt:1188, EmbeddableTerminal.kt:699):

   // Register one-shot listener BEFORE sending command
   // (must be registered before command executes to catch fast commands)

   The second parenthetical comment is redundant - "BEFORE sending command" already implies this.

2. Explicit Coroutine Context (EmbeddableTerminal.kt:699):
   The callback fires in the same coroutine context as the OSC 133 processing (likely IO dispatcher). Consider documenting this behavior if UI updates are expected:

   /**
    * @param onInitialCommandComplete Callback invoked on a background thread when command completes.
    *                                  Use appropriate dispatchers for UI updates.
    */

---

🧪 Test Coverage

Missing Tests: No unit/integration tests for the new callback. Recommended test cases:

1. Callback fires with correct exit code (0 for success)
2. Callback fires with correct exit code (non-zero for failure)
3. Callback fires exactly once (not multiple times)
4. Callback doesn't fire if initialCommand is null
5. Fast-completing commands trigger callback correctly

Manual Testing Checklist (from PR description):

• Verify callback fires when initial command completes (with OSC 133 shell integration)
• Verify callback receives correct exit code
• Verify callback only fires once per terminal
• Test with EmbeddableTerminal example
• Test with TabbedTerminal example

---

🔒 Security Considerations

No concerns identified. The implementation:

• Doesn't execute shell commands directly
• Doesn't expose internal state
• Properly sanitizes exit codes (uses toIntOrNull() in BossTerminal.kt:403)

---

⚡ Performance Considerations

Negligible impact. The implementation:

• Uses thread-safe CopyOnWriteArrayList (already used for other listeners)
• Listener self-unregisters immediately (no long-lived overhead)
• No blocking operations in callback path

---

📝 Documentation

Well-documented with:

• Clear KDoc comments explaining OSC 133 requirement
• Example usage in both demo apps
• Reference to CLAUDE.md for shell configuration

Suggestion: Add to CLAUDE.md under "Major Features" once merged.

---

🎯 Verdict

APPROVE with Minor Fix Required

The implementation is solid and well-designed, with one critical fix needed:

Required Before Merge:

• ✅ Fix race condition by moving listener registration before write() call

Recommended Enhancements (can be addressed in follow-up PRs):

• Add timeout-based fallback for missing shell integration
• Add logging when callback won't fire due to disabled shell integration
• Add unit tests for callback behavior

---

🔧 Suggested Fix

Apply this change to both files:

EmbeddableTerminal.kt (line ~694-708):

// If result is null, timeout occurred - proceed with fallback delay
// (already waited initialCommandDelayMs)

// Register one-shot listener BEFORE sending command
// (must be registered before command executes to catch fast commands)
if (onInitialCommandComplete != null) {
    val completionListener = object : CommandStateListener {
        override fun onCommandFinished(exitCode: Int) {
            // Fire callback once with success status and exit code
            onInitialCommandComplete(exitCode == 0, exitCode)
            // Unregister self to ensure callback only fires once
            session.terminal.removeCommandStateListener(this)
        }
    }
    session.terminal.addCommandStateListener(completionListener)
}

// Send the command followed by newline
processHandle.write(initialCommand + "\n")

TabController.kt (apply same pattern around line ~1180-1199)

---

Great work on this feature! The one-shot listener pattern is elegant and the OSC 133 integration is well-utilized. Once the race condition is fixed, this will be a solid addition to the API.

🤖 Generated with Claude Code

[claude]

Code Review: PR #218 - Add onInitialCommandComplete callback

Summary

This PR adds a callback mechanism to notify when an initialCommand finishes executing in both TabbedTerminal and EmbeddableTerminal. The implementation follows established patterns in the codebase and is well-designed overall.

---

✅ Strengths

1. Excellent Pattern Consistency: Uses the existing CommandStateListener interface and follows the same one-shot listener pattern already established in the codebase for OSC 133;A (prompt detection).

2. Proper Listener Cleanup: The try-finally block in onCommandFinished() ensures the listener is always unregistered, even if the callback throws an exception. This prevents memory leaks.

3. Thread Safety: Uses CopyOnWriteArrayList for listener storage (verified in BossTerminal.kt), which is appropriate for the add-iterate-remove pattern used here.

4. Correct Timing: Registers the listener BEFORE sending the command (line 696 in EmbeddableTerminal, line 1188 in TabController), which is critical for catching fast-completing commands.

5. Good Documentation: KDoc clearly explains the callback parameters and OSC 133 requirement.

6. Consistent API: Both TabbedTerminal and EmbeddableTerminal get the same callback signature.

7. Shell Integration: Properly adds ShellIntegrationInjector to EmbeddableTerminal (was missing before), ensuring OSC 133 support is configured.

---

🔍 Issues & Recommendations

🐛 Issue 1: DEBUG println Left in Production Code

Location: EmbeddableTerminal.kt:702, TabController.kt:1194

println("DEBUG: Initial command completed with exit code: $exitCode")

Impact: Debug output will pollute production console logs.

Recommendation: Remove or replace with proper logging:

// Option 1: Remove entirely (preferred for production)
onInitialCommandComplete(exitCode == 0, exitCode)

// Option 2: Use proper logging if debugging is needed
if (settings.debugModeEnabled) {
    println("Initial command completed with exit code: $exitCode")
}

---

⚠️ Issue 2: Potential Race Condition in TabController

Location: TabController.kt:1188-1207

The listener is registered inside the initialCommand != null coroutine, which means:

1. If the prompt listener times out and falls back to delay, there's a ~50ms window (line 1183 delay)
2. During this window, the command hasn't been sent yet, but a fast shell could emit multiple OSC 133 sequences
3. If a previous command (or shell startup script) emits OSC 133;D, the one-shot listener might fire prematurely

Recommendation: Consider checking if the callback has already fired or add a sequence counter to ensure it only fires for the FIRST command after registration:

val completionListener = object : CommandStateListener {
    private val fired = AtomicBoolean(false)
    
    override fun onCommandFinished(exitCode: Int) {
        if (fired.compareAndSet(false, true)) {
            try {
                onInitialCommandComplete(exitCode == 0, exitCode)
            } finally {
                tab.terminal.removeCommandStateListener(this)
            }
        }
    }
}

Severity: Low - This is an edge case that would only occur if:

• Shell has OSC 133 integration
• Shell startup scripts run commands that emit OSC 133;D
• Timing aligns perfectly

But given the one-shot nature, it's worth defending against.

---

📝 Issue 3: Parameter Type Could Be More Flexible

Location: All callback signatures

Current signature:

onInitialCommandComplete: ((success: Boolean, exitCode: Int) -> Unit)?

The exitCode: Int assumes we always have an exit code, but the OSC 133 protocol could theoretically have issues where no exit code is received.

Recommendation: Consider exitCode: Int? for better error handling:

onInitialCommandComplete: ((success: Boolean, exitCode: Int?) -> Unit)?

Then in the callback:

override fun onCommandFinished(exitCode: Int) {
    onInitialCommandComplete(exitCode == 0, exitCode)
}

This matches the documentation which already mentions "requires OSC 133" (implying it might not always work).

Severity: Low - Current implementation is fine if we guarantee OSC 133 is working, but nullable would be more defensive.

---

💡 Enhancement: Consider Callback Context

The callback doesn't provide any context about which command completed. If a developer runs multiple terminals with initialCommand, they might want to know which one finished.

Optional Enhancement:

onInitialCommandComplete: ((command: String, success: Boolean, exitCode: Int) -> Unit)?

// Then call it:
onInitialCommandComplete(initialCommand, exitCode == 0, exitCode)

This is not critical, but improves API ergonomics.

---

🧪 Testing Notes

The test plan in the PR description is good, but consider adding:

1. Fast Command Test: initialCommand = "true" (exits immediately) to ensure listener registration happens fast enough
2. Slow Command Test: initialCommand = "sleep 5" to verify callback waits for completion
3. Failing Command Test: initialCommand = "false" (exit code 1) to verify success=false
4. No OSC 133 Test: Test with shell that doesn't have OSC 133 configured (callback should never fire)
5. Multiple Terminals Test: Open 2 terminals with different initialCommand values, verify callbacks don't interfere

---

📊 Performance Impact

✅ No concerns - The implementation:

• Registers a single lightweight listener
• Uses CopyOnWriteArrayList (optimized for read-heavy workloads)
• Cleans up immediately after callback fires
• No polling or timers

---

🔒 Security Review

✅ No security concerns - The callback:

• Doesn't expose raw shell output (only exit code)
• Properly sanitizes input (no command injection risk)
• Uses existing terminal security boundaries

---

📝 Documentation Quality

✅ Good - KDoc explains:

• OSC 133 requirement
• Parameter meanings
• When callback fires

Suggestion: Add a note in the KDoc about when the callback will NOT fire (e.g., shell without OSC 133).

---

✅ Overall Assessment

Quality: High
Readiness: Nearly ready to merge after addressing Issue #1 (DEBUG println)

Recommendation:

1. ✅ Must Fix: Remove DEBUG println statements
2. 🔍 Should Consider: Add AtomicBoolean guard against premature firing
3. 💡 Nice to Have: Make exitCode nullable and add command context

This is well-designed code that follows established patterns. The core logic is sound. Address the DEBUG println before merging, and consider the race condition guard for robustness.

---

Great work on this feature! The one-shot listener pattern is exactly right, and the timing (register before send) is spot-on. 🎯