feat: Add initialCommand parameter to TabbedTerminal

[kshivang]

Summary

• Add initialCommand parameter to TabbedTerminal composable for programmatic use
• Add initialCommand setting in Settings > Behavior > Shell for user configuration
• Pass the initial command through to TabController.createTab()
• Send the command to PTY after shell is ready (500ms delay)

Features

1. Programmatic API (for embedding)

Enables embedding applications (like BOSS) to open a terminal and automatically execute a specific command:

TabbedTerminal(
    onExit = { /* handle exit */ },
    initialCommand = "ls -la"
)

2. User Setting (for daily use)

Users can configure a startup command in Settings > Behavior > Shell:

• Initial Command text field
• Examples: neofetch, cd ~/projects, source ~/.profile && clear
• Applied to all new tabs created via Cmd+T, tab bar, or menu

Priority

When both are set: programmatic parameter > user setting > none

Test plan

• Create terminal with initialCommand = "echo test" parameter and verify command runs
• Create terminal without initial command and verify normal behavior
• Test with long-running commands like top or htop
• Set "Initial Command" in settings to echo hello and create new tab
• Verify new tabs use the setting when no programmatic parameter is provided

🤖 Generated with Claude Code

[claude]

Code Review: Add initialCommand Parameter

Overview

This PR adds the ability to execute a command automatically when a terminal tab is created. The implementation is straightforward and follows existing patterns in the codebase.

Positives

1. Clean API Design: The optional parameter approach is non-breaking and intuitive
2. Proper Documentation: KDoc comments clearly explain the parameter purpose
3. Follows Existing Patterns: Uses similar patterns to other write operations
4. Minimal Surface Area: Only 2 files modified with 23 additions, 5 deletions

Issues and Concerns

1. Hardcoded Delay - Critical

The 500ms delay is arbitrary and may not work reliably across different shell types, system loads, or shell configurations. Command may execute before shell is ready, leading to lost or ignored commands.

Recommendation: Consider using OSC 133 integration (which already exists in the codebase) to detect prompt readiness, or make the delay configurable.

2. Error Handling Missing

The write call can throw IOException if the PTY is closed prematurely or process exits during delay. No try-catch present.

Recommendation: Add error handling and check handle.isConnected before writing.

3. Command Injection Risk - Security

No validation or sanitization of initialCommand. Malicious input could execute multiple commands.

Recommendation: Document security implications clearly in KDoc. Add WARNING that this accepts arbitrary shell syntax and should only be used with trusted input sources.

4. Limited to First Tab Only

The parameter is only passed when creating the first tab. Subsequent tabs via Ctrl+T cannot use this feature.

Question: Is this intentional? Should this be documented?

5. Documentation Style

The duplicate KDoc comment blocks should be merged.

Testing Recommendations

Expand coverage to include: commands that fail, empty string, special characters, tab closing during delay, different shells.

Performance

Minor: Every terminal startup has a 500ms coroutine overhead even when initialCommand is null.

Overall Assessment

Verdict: Good feature addition with some important concerns to address

Priority Fixes:

1. Add error handling
2. Document security implications
3. Consider OSC 133 integration

Questions:

1. Is 500ms delay based on empirical testing?
2. Should this support per-tab commands?
3. Have you considered OSC 133 shell integration?

[claude]

Code Review: Initial Command Feature

Thank you for this contribution! This is a useful feature for embedding applications. I've reviewed the changes and have several suggestions for improvement.

✅ Strengths

1. Clean API Design: The parameter flows logically from TabbedTerminal → TabController.createTab() → initializeTerminalSession()
2. Proper Documentation: KDoc comments clearly explain the new parameter's purpose
3. Non-breaking Change: Optional parameter with null default preserves backward compatibility
4. Simple Implementation: Straightforward approach that's easy to understand

---

⚠️ Issues & Recommendations

1. Hardcoded Delay is Fragile (High Priority)

Problem: The 500ms delay at line 1042 is a race condition waiting to happen:

• Fast shells (already initialized) will experience unnecessary lag
• Slow systems or complex shell configs may not be ready in 500ms
• No feedback if the shell isn't ready yet

Recommendation: Use shell integration (OSC 7) to detect when the prompt is ready. The codebase already has WorkingDirectoryOSCListener infrastructure:

// Instead of hardcoded delay, wait for first OSC 7 or prompt detection
private suspend fun waitForShellReady(tab: TerminalTab, timeoutMs: Long = 5000): Boolean {
    val startTime = System.currentTimeMillis()
    while (System.currentTimeMillis() - startTime < timeoutMs) {
        // Check if we've received OSC 7 (shell integration active)
        if (tab.workingDirectoryState.value != null) return true
        delay(100)
    }
    return false  // Timeout - shell may not have integration
}

// Then in initializeTerminalSession:
if (initialCommand != null) {
    launch(Dispatchers.IO) {
        val isReady = waitForShellReady(tab)
        if (!isReady) {
            // Fallback to shorter delay if no shell integration
            delay(500)
        }
        handle.write(initialCommand + "\n")
    }
}

Alternatively, if OSC 7 isn't available, consider making the delay configurable via TerminalSettings.

2. Missing Error Handling (High Priority)

Problem: No error handling around handle.write() at line 1044. If the PTY process dies before 500ms, this will crash.

Recommendation:

if (initialCommand != null) {
    launch(Dispatchers.IO) {
        try {
            delay(500)
            if (handle.isAlive()) {
                handle.write(initialCommand + "\n")
            } else {
                println("WARN: PTY process died before initial command could be sent")
            }
        } catch (e: Exception) {
            println("ERROR: Failed to send initial command: ${e.message}")
            e.printStackTrace()
        }
    }
}

3. Scope Constraint Issue (Medium Priority)

Problem: The launched coroutine at line 1040 may outlive the parent scope. If the tab is closed quickly, the coroutine could still try to write to a closed PTY.

Current:

launch(Dispatchers.IO) {  // This inherits from outer scope
    delay(500)
    handle.write(initialCommand + "\n")
}

Recommendation: Use tab.coroutineScope for proper lifecycle management:

tab.coroutineScope.launch(Dispatchers.IO) {
    try {
        delay(500)
        if (handle.isAlive() && isActive) {  // Check both PTY and coroutine state
            handle.write(initialCommand + "\n")
        }
    } catch (e: Exception) {
        if (e !is CancellationException) {
            println("ERROR: Failed to send initial command: ${e.message}")
        }
    }
}

4. Command Injection Risk (Medium Priority - Security)

Problem: initialCommand is passed directly from embedding apps without sanitization. Malicious apps could inject control sequences:

// Malicious example:
initialCommand = "ls\u0003rm -rf /"  // Ctrl+C followed by dangerous command

Recommendation: While the embedding app is trusted in most cases, consider:

1. Documenting this security assumption clearly in KDoc
2. Adding optional validation/escaping if the command will come from user input
3. Consider exposing a safer API like initialCommands: List<String> to avoid shell injection

Updated KDoc:

/**
 * @param initialCommand Optional command to execute after terminal is ready (sent as input with newline)
 *                       ⚠️ WARNING: This is sent directly to the shell without escaping.
 *                       Ensure the command comes from a trusted source. Do not pass
 *                       unsanitized user input.
 */

5. Missing Test Coverage (Low Priority)

The PR description includes a test plan but no automated tests. Consider adding:

• Unit test for createTab() with initialCommand parameter
• Integration test verifying command is sent to PTY mock
• Test for error handling when PTY dies early

6. Feature Discoverability (Low Priority)

Problem: The initialCommand parameter only works for the first tab created in LaunchedEffect(Unit). If a user programmatically creates additional tabs via tabController.createTab(), they can pass initialCommand there, but it's not obvious.

Recommendation: Update the KDoc for TabbedTerminal.initialCommand to clarify:

/**
 * @param initialCommand Optional command to run in the first terminal tab after startup.
 *                       For additional tabs, use TabController.createTab(initialCommand = "...").
 */

---

📋 Minor Code Style Notes

1. Line 198: Comment says "create fresh terminal with optional initial command" but the parameter name already makes this obvious. Consider removing redundant comment.

2. Consistent Naming: The parameter is initialCommand (singular) but could be misinterpreted as just the command name. Consider initialCommandLine to clarify it can include arguments.

---

🔧 Suggested Refactor (Optional)

If you want to make this more robust, consider extracting the logic:

/**
 * Sends an initial command to a newly created terminal session.
 * Waits for shell readiness before sending to avoid race conditions.
 */
private suspend fun sendInitialCommand(
    tab: TerminalTab,
    handle: ProcessHandle,
    command: String,
    settings: TerminalSettings
) {
    tab.coroutineScope.launch(Dispatchers.IO) {
        try {
            // Wait for shell readiness (with configurable timeout)
            val timeout = settings.initialCommandDelayMs ?: 500L
            delay(timeout)
            
            if (handle.isAlive() && isActive) {
                handle.write(command + "\n")
                println("INFO: Sent initial command to tab: ${tab.title.value}")
            } else {
                println("WARN: Cannot send initial command - PTY no longer alive")
            }
        } catch (e: Exception) {
            if (e !is CancellationException) {
                println("ERROR: Failed to send initial command: ${e.message}")
            }
        }
    }
}

// Then in initializeTerminalSession:
if (initialCommand != null) {
    sendInitialCommand(tab, handle, initialCommand, settings)
}

---

📊 Summary

Category	Finding	Severity
Race Condition	Hardcoded 500ms delay	High
Error Handling	Missing try-catch and liveness check	High
Lifecycle	Coroutine scope not tied to tab	Medium
Security	Potential command injection	Medium
Testing	No automated tests	Low
Documentation	Clarify first-tab-only behavior	Low

---

✅ Approval Recommendation

Conditional Approval - Please address items #1-3 before merging. This is a solid foundation, but the timing/error handling issues could cause problems in production.

Great work on a useful feature! Let me know if you'd like help implementing any of these suggestions.

🤖 Generated with Claude Code

[claude]

Code Review: Initial Command Support

Summary

This PR adds support for running commands automatically when a terminal tab starts. The implementation is clean and follows the existing codebase patterns well.

✅ Strengths

1. Good API Design: The initialCommand parameter is properly optional and flows through the call chain cleanly
2. Settings Integration: Nice addition to both the settings data class and UI panel
3. Consistent Pattern: Follows the established pattern used by OSC 7 working directory tracking
4. Documentation: Well-documented parameter descriptions and use case examples

🔴 Critical Issues

1. Race Condition with Shell Initialization (Security & Reliability)

Location: TabController.kt:1038-1045

// Current implementation
if (initialCommand != null) {
    launch(Dispatchers.IO) {
        delay(500)  // ⚠️ HARD-CODED DELAY
        handle.write(initialCommand + "\n")
    }
}

Problems:

• Hard-coded 500ms delay is fragile and may fail on slow systems or heavy load
• No verification that shell is actually ready
• Could send commands before shell initialization completes
• May break with different shells (zsh, bash, fish) that have varying startup times

Security Concern: If the shell is not ready, the command could be executed in an unpredictable state or worse, be interpreted as part of shell initialization, potentially allowing command injection through shell startup files.

Recommended Solution: Use OSC sequences for shell readiness detection (similar to OSC 7 for working directory tracking):

// Add to BossTerminal.kt or create InitialCommandListener.kt
interface ShellReadyListener {
    fun onShellReady()
}

// In shell setup (e.g., ~/.bashrc or ~/.zshrc), emit:
// precmd() { echo -ne "\\033]133;A\\007" }  # Zsh
// PROMPT_COMMAND='echo -ne "\\033]133;A\\007"'  # Bash

// In TabController, listen for OSC 133;A (prompt ready) before sending command

Alternative (if OSC approach is too complex): At minimum, increase delay to 1000ms and add a comment explaining the trade-off.

2. Priority Logic Issue with Settings

Location: TabbedTerminal.kt:198-200

// Priority: parameter > settings > none
val effectiveInitialCommand = initialCommand ?: settings.initialCommand.ifEmpty { null }
tabController.createTab(initialCommand = effectiveInitialCommand)

Problem: The priority logic doesn't match the comment or expected behavior:

• When initialCommand="" (empty string) is explicitly passed, it will fall through to settings.initialCommand
• This means you cannot override settings to "no command" via parameter

Expected Behavior:

• initialCommand=null: Use settings value
• initialCommand="": Override settings with no command
• initialCommand="foo": Override settings with "foo"

Fix:

// Three-way priority: explicit parameter > settings > none
val effectiveInitialCommand = when {
    initialCommand != null -> initialCommand.ifEmpty { null }  // Explicit override (including empty)
    else -> settings.initialCommand.ifEmpty { null }  // Fall back to settings
}

⚠️ Medium Issues

3. Inconsistent Behavior for Menu Actions

Locations: TabbedTerminal.kt:117, 234, 336

All three menu action handlers use settings.initialCommand but ignore the initialCommand parameter:

onNewTab = {
    // Uses settings only - ignores parameter
    tabController.createTab(initialCommand = settings.initialCommand.ifEmpty { null })
}

Problem: When embedding BossTerminal with initialCommand="foo", only the FIRST tab gets "foo". All subsequent tabs (Ctrl+T, etc.) use the settings value.

Question for Author: Is this intentional? Should the parameter apply to:

• A) Only the first tab (current behavior)
• B) All tabs created in this TabbedTerminal instance
• C) Only explicitly created tabs, not menu/shortcut tabs

Recommended: Make this explicit via a parameter like initialCommandScope: InitialCommandScope with enum values FIRST_TAB_ONLY, ALL_TABS.

4. Missing Input Sanitization

Location: TabController.kt:1044

handle.write(initialCommand + "\n")

Problem: No validation or sanitization of initialCommand:

• No check for null bytes or control characters
• No warning for potentially destructive commands
• Could be abused if settings file is modified by malicious software

Recommended:

// Add basic validation
if (initialCommand != null) {
    val sanitized = initialCommand.replace("\\u0000", "")  // Strip null bytes
    if (sanitized.isNotBlank()) {
        launch(Dispatchers.IO) {
            delay(500)
            handle.write(sanitized + "\n")
        }
    }
}

💡 Minor Suggestions

5. Settings UI Enhancement

Location: BehaviorSettingsSection.kt:31-37

Consider adding:

• Multi-line support: Some users may want to run multiple commands (e.g., cd ~/projects && ls)
• Validation feedback: Show a warning for potentially dangerous commands (rm -rf, sudo, etc.)
• Examples: Add more examples in the placeholder or description

6. Documentation Gaps

Missing documentation for:

• What happens if command fails or hangs
• Whether command inherits tab's working directory (it should based on PTY setup)
• Security implications for users
• Shell-specific behavior differences

Recommended: Add a section to CLAUDE.md under "Shell Integration Setup" explaining this feature.

7. Debug/Logging Support

Location: TabController.kt:1038-1045

Add logging for troubleshooting:

if (initialCommand != null) {
    println("INFO: Scheduling initial command for tab: tab_title")
    launch(Dispatchers.IO) {
        delay(500)
        println("INFO: Sending initial command: command_text")
        handle.write(initialCommand + "\n")
    }
}

🧪 Testing Recommendations

The test plan in the PR description is good, but should also cover:

1. Edge Cases:

   • Empty string vs null
   • Commands with special characters (backticks, dollar signs, quotes, etc.)
   • Very long commands (>1000 chars)
   • Commands that prompt for input (read, sudo)

2. Race Conditions:

   • Test on slow systems or with artificial delays
   • Test with shells that have long .bashrc/.zshrc files

3. Settings Interaction:

   • Parameter override behavior
   • Changing settings while terminal is open
   • Multiple windows with different settings

4. Security:

   • Commands with control characters
   • Commands that fork background processes

📊 Overall Assessment

Code Quality: 7/10 (clean but has reliability issues)
Security: 6/10 (needs input validation and better timing)
Performance: 8/10 (minimal overhead)
Maintainability: 8/10 (follows existing patterns well)

✅ Recommendation

DO NOT MERGE until critical issues #1 and #2 are addressed. These issues could cause user confusion and potential security problems.

Once fixed, this is a valuable feature that fits well with the terminal's architecture.

---

Reviewed with: BossTerm Development Guidelines (CLAUDE.md)
Review Date: 2025-12-14

[claude]

Code Review: Initial Command Support

This PR adds the ability to execute an initial command when a terminal tab is created, both programmatically and via user settings. Overall, this is a well-structured implementation that follows the existing codebase patterns. Here's my detailed feedback:

✅ Strengths

1. Clean API Design: The dual-interface approach (programmatic parameter + user setting) with clear priority is well thought out
2. Consistent Integration: Uses existing settings infrastructure and follows established patterns
3. Good Documentation: KDoc comments clearly explain the new parameters
4. Sensible Defaults: Empty string means no command, avoiding null confusion in settings
5. UI Enhancement: ScrollState addition to NavigationRail prevents overflow issues

🐛 Potential Issues

1. Timing Issue: 500ms delay is unreliable (TabController.kt:1042)

delay(500)
handle.write(initialCommand + "\n")

Problem: The 500ms delay is a hardcoded guess that may be too short or too long depending on:

• Shell startup time (zsh with plugins can take >1s)
• System load
• Slow filesystems

Impact: Commands may execute before the shell is ready, causing them to be lost or partially executed.

Recommendation: Implement shell-ready detection:

// Option 1: Wait for OSC 133;A (prompt ready) signal
// This requires shell integration but is most reliable

// Option 2: Wait for first prompt character detection
// Less reliable but doesn't require shell integration

// Option 3: Increase delay to 1500ms as a safer default
delay(1500)

2. Security: No input sanitization

Problem: The initialCommand parameter accepts arbitrary strings without validation. While this is intentional for flexibility, it creates potential security risks:

• Command injection if initialCommand comes from untrusted sources
• No escaping of special characters
• Could be exploited in embedding scenarios where the parent app doesn't validate input

Current Risk: LOW (user-controlled settings are generally trusted)
Future Risk: MEDIUM (if BOSS or other embedding apps pass user input directly)

Recommendation: Add security guidance in documentation:

/**
 * @param initialCommand Optional command to run in the first terminal tab after startup.
 *                       SECURITY: This string is sent directly to the shell without escaping.
 *                       Ensure input is validated if sourced from untrusted origins.
 */

3. Error Handling: Silent failures

Problem: If handle.write() fails, the error is silently swallowed. Users won't know their command didn't execute.

Recommendation:

launch(Dispatchers.IO) {
    delay(500)
    try {
        handle.write(initialCommand + "\n")
    } catch (e: Exception) {
        println("WARNING: Failed to send initial command: ${e.message}")
        // Consider showing notification to user
    }
}

💡 Design Considerations

4. Missing: Per-tab initial commands

Current: All new tabs created via Cmd+T use the settings-based initial command
Limitation: Users can't configure different commands for different tabs

Consideration: Is this intentional? For example, users might want:

• Tab 1: cd ~/projects && git status
• Tab 2: cd ~/logs && tail -f app.log
• Tab 3: htop

This is likely out of scope for this PR, but worth noting for future enhancement.

5. Missing: Command history integration

Observation: The initial command is sent as raw input, so it appears in shell history.

Question: Is this the desired behavior? Some users might prefer initial commands to be invisible in history (similar to .zshrc commands).

📝 Code Quality

6. Redundant code pattern (TabbedTerminal.kt)

The pattern appears 3 times:

tabController.createTab(initialCommand = settings.initialCommand.ifEmpty { null })

Suggestion: Extract to helper function:

private fun TerminalSettings.getInitialCommandOrNull() = 
    initialCommand.ifEmpty { null }

// Usage:
tabController.createTab(initialCommand = settings.getInitialCommandOrNull())

Or better, handle this logic inside createTab():

fun createTab(
    // ...
    initialCommand: String? = null
): TerminalTab {
    val effectiveCommand = initialCommand?.ifEmpty { null }
    // ...
}

7. Missing: Settings validation

The settings UI accepts any string, including:

• Multiline commands (unclear behavior)
• Commands with unbalanced quotes
• Empty strings vs whitespace-only strings

Recommendation: Add basic validation or clarify expected format in UI description.

🧪 Testing Recommendations

Beyond the test plan in the PR description, consider testing:

1. Edge cases:

   • Very long commands (>1000 chars)
   • Commands with special characters: !, $, backticks, quotes
   • Commands that prompt for input: sudo ls
   • Commands that never terminate: cat (without args)

2. Timing issues:

   • Fast shells (bash) vs slow shells (zsh with oh-my-zsh)
   • First tab vs subsequent tabs (shell might cache)

3. Race conditions:

   • Create tab → close immediately (before 500ms)
   • Create multiple tabs rapidly

📊 Performance

Impact: Negligible. The delay happens in a background coroutine and doesn't block UI or other tabs.

🔒 Security Assessment

Overall: LOW RISK for current use case (user settings)

Concerns for future embedding:

• If BOSS passes user-generated content to initialCommand, implement validation
• Consider rate limiting or command whitelisting for untrusted sources

✅ Summary

This is a solid implementation with good adherence to project conventions. The main concern is the unreliable 500ms timing which could cause issues for users with slow-starting shells. I recommend:

1. Required before merge: Increase delay to 1500ms or implement shell-ready detection
2. Recommended: Add error handling and logging for failed writes
3. Nice to have: Extract repeated code patterns, add input validation

The feature itself is well-designed and will be useful for users who want custom startup commands.

---

🤖 Review generated with Claude Code

[claude]

Code Review - PR #122: Add initialCommand Parameter

I've reviewed this PR and have the following feedback:

✅ Strengths

1. Clean API Design: The dual-mode approach (programmatic parameter + user setting) with proper priority is well-designed and intuitive.
2. Consistent Integration: The initialCommand parameter is properly threaded through all tab creation call sites (menu actions, tab bar, keyboard shortcuts).
3. Good Documentation: The KDoc comments clearly explain the feature and parameter usage.
4. Settings UI: The SettingsTextField integration is appropriate with helpful placeholder text and description.
5. Proper Scope: The coroutine launch in Dispatchers.IO for sending the command is correctly scoped to the tab lifecycle.

⚠️ Critical Issue: Race Condition with Fixed Delay

Location: TabController.kt:1038-1046

The 500ms fixed delay is a brittle race condition:

• Too short: Command may execute before shell is ready (zsh/bash initialization, .zshrc/.bashrc sourcing)
• Too long: Unnecessary latency for fast shells
• Environment-dependent: Varies by machine performance, shell config complexity, network mounts, etc.

Recommendation: Use OSC 133;A (prompt start) signal for proper synchronization. BossTerm already has OSC 133 support (see CLAUDE.md). This is the proper way to detect shell readiness.

Alternative: At minimum, make the delay configurable via TerminalSettings.

🔒 Security: Command Injection (Low Risk)

Assessment:

• ✅ Safe in current context: The command is sent as user input to the shell, not executed directly by the JVM
• ✅ User is in control: Whether from settings or programmatic API, the user owns the command source
• ❌ No escaping: Shell metacharacters will be interpreted by the shell

Recommendation: Document the security model in the KDoc for TabbedTerminal.initialCommand with a WARNING about command injection risk for embedding applications.

🎨 Minor UI Issue: Settings Panel Scrollbar

Location: SettingsPanel.kt:176-180

This change (adding scroll to navigation rail) seems unrelated to the initial command feature. Was this intentional? If so, please mention it in the PR description or split into a separate commit.

📝 Code Quality Observations

✅ Good patterns:

• Null safety: settings.initialCommand.ifEmpty { null } is idiomatic and clear
• Priority logic: initialCommand ?: settings.initialCommand.ifEmpty { null } is explicit
• LaunchedEffect dependency: Adding tabs.size ensures split actions update correctly

🧪 Testing Recommendations

Critical Test Cases:

• Test with multi-line commands
• Test with shell builtins like cd
• Test with sourcing commands
• Test with slow shells (add artificial delay in .zshrc)
• Test race condition: verify prompt appears before command executes
• Test with very long commands (>1024 chars)
• Test with special characters and shell metacharacters
• Test priority: Set both parameter and setting, verify parameter wins
• Test with 10+ rapid tab openings to verify no resource leaks

📋 Summary

Category	Status	Notes
Functionality	✅ Good	Clean API design, proper integration
Code Quality	✅ Good	Consistent patterns, well-documented
Performance	⚠️ BLOCKER	Fixed 500ms delay is a race condition
Security	⚠️ Advisory	Document command injection risk
Testing	⚠️ Needs expansion	Add race condition and edge case tests
UI/UX	⚠️ Minor	Navigation rail scroll change unexplained

🎯 Recommended Actions Before Merge

1. MUST FIX: Replace fixed delay with OSC 133 prompt detection or make delay configurable
2. SHOULD ADD: Security warning in initialCommand KDoc
3. SHOULD CLARIFY: Explain or separate the navigation rail scroll change
4. SHOULD TEST: Add test cases for race conditions and edge cases

💬 Questions for Author

1. Have you tested this with slow shell initialization (e.g., Oh My Zsh with 20+ plugins)?
2. Did you consider OSC 133;A detection for shell readiness?
3. Is the navigation rail scroll change intentional?

Overall, this is a well-implemented feature with a clear use case. The main concern is the timing race condition, which could cause subtle bugs in real-world usage. With the OSC 133 detection or configurable delay, this would be production-ready.

Great work! 🚀

[claude]

Code Review - PR #122: Add initialCommand parameter to TabbedTerminal

Thank you for this feature implementation! This is a well-thought-out addition that provides both programmatic API and user-facing configuration. Here's my detailed review:

---

✅ Strengths

1. Excellent Architecture

• Clean separation between programmatic API (initialCommand parameter) and user settings
• Priority system (parameter > settings > none) is well-documented and intuitive
• Proper use of OSC 133 shell integration with fallback mechanism

2. Smart Synchronization

The OSC 133 prompt detection is particularly clever - using CompletableDeferred with a temporary CommandStateListener avoids race conditions and ensures the shell is truly ready before sending commands.

3. Good Documentation

• Clear kdoc comments on new parameters and settings
• Helpful examples in the PR description and UI hints

---

🔍 Issues & Concerns

1. Critical: Race Condition on Terminal Disposal ⚠️

Location: TabController.kt:1069

If the user closes the tab during the delay/wait period, handle may be closed, causing an IOException when writing the initial command.

Recommendation: Add a check before writing:

if (isActive && !handle.isClosed) {
    handle.write(initialCommand + "\n")
}

2. Settings Validation Missing

Location: TerminalSettings.kt:163

The initialCommand field has no validation:

• Could contain newlines (multiple commands - which may be intended)
• Could be extremely long
• No length limits

Recommendation: Add reasonable limits:

val initialCommand: String = ""
    get() = field.trim().take(500) // Limit length

3. Delay Range Inconsistency 📝

Location: BehaviorSettingsSection.kt:43 vs TerminalSettings.kt:171

• Documentation says: "Range: 100-5000ms"
• Slider shows: 100f..2000f

Recommendation: Either update the doc to match the slider (100-2000ms) or extend the slider range to 5000ms.

4. Minor: LaunchedEffect Dependency

Location: TabbedTerminal.kt:132

Adding tabs.size as a dependency causes the effect to re-run every time a tab is added/removed. This could cause unnecessary recompositions.

Recommendation: Verify this dependency is truly needed for the menu actions wiring. Consider if this should be handled differently.

---

🎯 Performance Considerations

1. Multiple Delays

Every new tab with initialCommand waits either for OSC 133 or the timeout. For users who rapidly create tabs (Ctrl+T spam), this creates many concurrent coroutines.

Assessment: This is probably fine, but worth monitoring. The coroutines are properly scoped to tabs.

2. Code Duplication

tabController.createTab(initialCommand = settings.initialCommand.ifEmpty { null }) appears 4 times.

Recommendation: Consider extracting to a helper function to reduce duplication and make updates easier.

---

🔒 Security Considerations

1. Command Injection (Low Risk)

Since this is user-configured, the risk is to themselves. However, if BossTerm is embedded in another app, the parent app should sanitize any programmatic initialCommand values.

Recommendation: Add a warning in the kdoc for TabbedTerminal.initialCommand:

/**
 * @param initialCommand Optional command to run in the first terminal tab after startup.
 *                       WARNING: No sanitization is performed. Ensure this value is trusted
 *                       if set programmatically by a parent application.
 */

---

🧪 Test Coverage

The PR includes a test plan but no automated tests. Consider adding:

1. Unit test for priority logic (parameter vs settings)
2. Unit test for OSC 133 detection with timeout
3. Integration test for tab lifecycle with initial command

---

📝 Minor Suggestions

1. Magic Number

delay(50) on line 1063 - consider extracting as a constant:

private const val PROMPT_RENDER_DELAY_MS = 50L

2. Settings Panel Scrollability

The addition of verticalScroll(scrollState) to the navigation rail (SettingsPanel.kt:181) is good, but unrelated to this PR. Consider mentioning it in the PR description.

3. Cleanup Pattern

The try-finally pattern for listener cleanup is good, but consider adding a comment explaining it handles coroutine cancellation.

---

📊 Overall Assessment

Rating: 8/10

This is a solid feature implementation with good architectural choices. The main concerns are around edge cases (tab closure during command send) and minor validation issues. Once these are addressed, this will be a great addition to BossTerm.

Recommended Actions Before Merge:

1. ✅ Add terminal/handle state check before write
2. ✅ Fix delay range documentation or slider
3. ✅ Add security warning to kdoc
4. ⚠️ Consider input validation (nice-to-have)
5. ⚠️ Add automated tests (nice-to-have)

---

Great work overall! The OSC 133 integration is particularly well done. Let me know if you'd like me to clarify any of these points.

[claude]

Code Review: Initial Command Feature

Summary

This PR adds the ability to execute a command automatically when a terminal tab is created, both programmatically (via API parameter) and through user settings. The implementation is well-structured and follows the project's conventions.

---

✅ Strengths

1. Clean API Design

   • The initialCommand parameter in TabbedTerminal is optional and non-breaking
   • Clear priority system: programmatic parameter > user setting > none
   • Good documentation in KDoc comments

2. Smart Synchronization Strategy

   • Uses OSC 133;A (prompt started) signal for optimal timing
   • Falls back to configurable delay when shell integration isn't available
   • Cleans up temporary listener properly with finally block

3. Consistent Application

   • Initial command from settings is applied to all new tab creation points (menu, keyboard shortcuts, tab bar)
   • Code duplication is minimal and intentional for clarity

4. Good UX

   • Slider is disabled when no command is set (lines in BehaviorSettingsSection.kt:44)
   • Settings panel navigation rail now scrollable (SettingsPanel.kt:176-178)

---

🔍 Issues & Recommendations

1. Race Condition Risk (HIGH PRIORITY)

Location: TabController.kt:1039-1075

The temporary CommandStateListener is added to the terminal, but if the OSC 133;A signal was already emitted before the listener is registered, the CompletableDeferred will never complete, and the code will always wait for the full timeout.

Recommendation:

// Check if terminal is already at prompt (in case OSC 133 fired before listener added)
if (tab.terminal.isAtPrompt()) {
    promptReady.complete(Unit)
} else {
    tab.terminal.addCommandStateListener(promptListener)
}

Alternatively, restructure to add the listener before any terminal output processing begins.

---

2. No Input Validation (MEDIUM PRIORITY)

Location: TerminalSettings.kt:162

The initialCommand field accepts any string, including potentially problematic inputs:

• Commands with special shell characters that need escaping
• Multi-line commands (newlines in the middle)
• Very long commands that exceed buffer limits

Recommendation:
Add validation or sanitization:

val initialCommand: String = ""
    get() = field.trim().takeIf { it.isNotEmpty() && !it.contains('\n') } ?: ""

Or add UI-level validation in BehaviorSettingsSection.kt to prevent newlines.

---

3. Hardcoded Delay Value

Location: TabController.kt:1063

The 50ms delay after OSC 133;A is hardcoded:

delay(50)  // Small delay to ensure prompt is fully rendered

Recommendation:

• Extract to a constant: private const val POST_PROMPT_DELAY_MS = 50L
• Or make it configurable if users report timing issues

---

4. Settings Validation Missing

Location: TerminalSettings.kt:171

The initialCommandDelayMs has a documented range of 100-5000ms, but no enforcement:

/**
 * Range: 100-5000ms. Default: 500ms.
 */
val initialCommandDelayMs: Int = 500

Recommendation:
Add validation in the data class or setter:

val initialCommandDelayMs: Int = 500
    get() = field.coerceIn(100, 5000)

Also update the slider in BehaviorSettingsSection.kt:42 to match:

valueRange = 100f..5000f,  // Currently 100f..2000f

---

5. Error Handling Absent

Location: TabController.kt:1069

The handle.write() call has no error handling. If the PTY is closed or write fails, the exception will crash the coroutine.

Recommendation:

try {
    handle.write(initialCommand + "\n")
} catch (e: Exception) {
    println("WARN: Failed to send initial command: " + e.message)
}

---

6. Potential Command Injection (LOW RISK)

Location: TabController.kt:1069

While the terminal is already running arbitrary shell commands, the initial command is user-controlled input that's directly appended with \n and sent to the PTY. If this API is ever exposed to external input (e.g., deep links, IPC), it could be a security concern.

Recommendation:

• Document in TabbedTerminal KDoc that initialCommand must be validated by callers
• Consider adding a safety check for common injection patterns if programmatic use expands

---

7. Test Coverage Missing

The PR description includes a test plan, but no automated tests are added.

Recommendation:
Add unit tests for:

• TabController.createTab() with initialCommand
• Settings validation for initialCommandDelayMs
• Priority logic (parameter vs. setting)

---

🎯 Minor Suggestions

1. Typo in Comment (BehaviorSettingsSection.kt:35)

   • "applies to new tabs only" - consider "applied to new tabs only"

2. LaunchedEffect Dependency (TabbedTerminal.kt:132)

   • Adding tabController.tabs.size to dependencies might cause extra recompositions
   • Consider if tabs.lastOrNull() or a version counter would be more efficient

3. Code Comment Clarity (TabController.kt:1040-1042)

   • Comment mentions "Uses OSC 133;A (prompt started)" but doesn't explain why 50ms delay is still needed after signal
   • Clarify that the delay ensures the prompt text is fully rendered in the UI

---

📊 Performance Considerations

1. Coroutine Launch (TabController.kt:1042)

   • Launches a new coroutine for every tab with initialCommand
   • This is acceptable, but ensure CompletableDeferred doesn't leak if tab is closed during wait

   Add cleanup:

   tab.coroutineScope.launch(Dispatchers.IO) {
       // Existing code
   }.invokeOnCompletion {
       if (!promptReady.isCompleted) {
           promptReady.cancel()
       }
   }

---

🔒 Security Assessment

• No major security vulnerabilities detected
• User-controlled shell commands are already the terminal's primary function
• Risk is limited to users configuring their own settings or developers using the API

---

✅ Alignment with CLAUDE.md

The PR follows project conventions:

• ✅ Proper commit message format (no Co-Authored-By)
• ✅ Uses existing patterns (OSC 133, settings system)
• ✅ Clear KDoc comments
• ✅ Consistent with terminal action framework

---

Verdict

Status: ⚠️ Approve with Minor Changes Requested

The core implementation is solid and feature-complete. The main concern is the race condition risk in the OSC 133 listener timing. The other issues are improvements that enhance robustness but aren't blockers.

Required Before Merge:

1. Fix OSC 133;A race condition
2. Add error handling for handle.write()
3. Validate initialCommandDelayMs range

Recommended for Follow-up:

• Add automated tests
• Input validation for initialCommand
• Extract hardcoded delay constant

---

Great work on this feature! The OSC 133 integration is particularly clever. 🚀

[claude]

Code Review: Initial Command Feature

Great work on implementing this feature! The overall implementation is solid and follows the project's conventions well. Here are my detailed findings:

---

✅ Strengths

1. Excellent OSC 133 Integration: Using OSC 133;A for shell readiness detection is a major improvement over fixed delays. This is exactly the right approach per the project's CLAUDE.md documentation on shell integration.

2. Proper Listener Cleanup: The finally block ensures the temporary CommandStateListener is always removed, preventing memory leaks.

3. Graceful Fallback: The withTimeoutOrNull() approach elegantly handles shells without OSC 133 support.

4. UI/UX Polish:

   • Settings slider is properly disabled when no initial command is set
   • Clear documentation in setting descriptions about "applies to new tabs only"
   • Scrollable NavigationRail fix prevents UI overflow

5. Consistent Priority Handling: The parameter > setting > none priority is applied consistently across all tab creation points.

6. Bug Fix Excellence: The split pane focus fix (commit ea3d4fd) is elegant - calling onPaneFocus() before any other processing ensures proper focus management.

---

🐛 Potential Issues

1. Race Condition: Listener Registration Timing (High Priority)

File: TabController.kt:1042-1076

The promptListener is registered after the PTY process has already started. If the shell is extremely fast and emits OSC 133;A before the listener is registered, the signal will be missed.

Problem:

// PTY starts here (line ~974)
handle.start(tab.coroutineScope)

// ... many lines of setup code ...

// Listener registered much later (line 1053)
tab.terminal.addCommandStateListener(promptListener)

Solution: Register the listener before starting the PTY:

// Set up initial command listener BEFORE starting PTY
val promptReady = if (initialCommand \!= null) {
    val deferred = CompletableDeferred<Unit>()
    val listener = object : CommandStateListener {
        override fun onPromptStarted() {
            deferred.complete(Unit)
        }
    }
    tab.terminal.addCommandStateListener(listener)
    Pair(deferred, listener)
} else null

// NOW start the PTY
handle.start(tab.coroutineScope)

// ... later, in the existing launch block ...
if (promptReady \!= null) {
    val (deferred, listener) = promptReady
    try {
        withTimeoutOrNull(settings.initialCommandDelayMs.toLong()) {
            deferred.await()
        }?.let { delay(50) }
        handle.write(initialCommand\!\! + "\n")
    } finally {
        tab.terminal.removeCommandStateListener(listener)
    }
}

---

2. Uncaught Exception: handle.write() Can Fail (Medium Priority)

File: TabController.kt:1070

The handle.write() call can throw exceptions if the PTY is closed or encounters I/O errors. This could leave the listener registered indefinitely.

Recommendation:

try {
    // ... existing timeout logic ...
    handle.write(initialCommand + "\n")
} catch (e: Exception) {
    println("ERROR: Failed to send initial command: ${e.message}")
} finally {
    tab.terminal.removeCommandStateListener(promptListener)
}

---

3. String Encoding Issue: Newline Handling (Low Priority)

File: TabController.kt:1070

Using "\n" assumes Unix line endings. While this is correct for BossTerm (Unix-only per CLAUDE.md), consider edge cases where the shell might need "\r\n" or if the command itself contains special characters.

Recommendation: Document this assumption or add a comment:

// Send command with Unix newline (BossTerm is Unix-only)
handle.write(initialCommand + "\n")

---

4. Settings Validation Missing (Medium Priority)

File: TerminalSettings.kt:163, BehaviorSettingsSection.kt:39

The initialCommandDelayMs has a documented range of 100-5000ms in the comment, but the UI slider only allows 100-2000ms:

// TerminalSettings.kt comment says: "Range: 100-5000ms"
valueRange = 100f..2000f  // UI only allows up to 2000ms

Recommendation: Either update the comment to match the UI, or increase the slider range. Also add validation:

val initialCommandDelayMs: Int = 500,
    get() = field.coerceIn(100, 2000)  // Enforce range

---

5. Magic Number: 50ms Delay After OSC 133;A (Low Priority)

File: TabController.kt:1064

The 50ms delay after OSC 133;A is undocumented. Why is this needed if the prompt signal indicates readiness?

Recommendation: Add a comment explaining the rationale:

// Small delay to ensure prompt is fully rendered and ready for input
// (some shells emit OSC 133;A slightly before cursor is positioned)
delay(50)

---

🎯 Best Practices

1. Test Coverage Missing (High Priority)

No unit tests were added for the new functionality. Consider adding tests for:

• TabController.createTab() with initialCommand
• Settings priority logic (parameter > setting > none)
• OSC 133 listener registration/cleanup
• Timeout behavior when OSC 133 is not received

2. Documentation Gap (Medium Priority)

The feature is well-documented in the PR description but is missing from:

• CLAUDE.md: Should be added to "Major Features (Implemented)" section
• User-facing documentation: Consider adding usage examples

---

🔒 Security Considerations

1. Command Injection Risk (Low Priority)

The initialCommand is sent directly to the shell without sanitization. While this is by design (users control their own settings), consider:

• Warning users in the settings UI that the command runs in their shell
• Documenting that programmatic callers should sanitize input if it comes from untrusted sources

Recommendation: Add a note in the API documentation:

/**
 * @param initialCommand Optional command to execute after terminal is ready.
 *                       WARNING: Command is sent directly to the shell without
 *                       sanitization. Callers must ensure input is safe.
 */

---

📊 Performance Considerations

1. Listener Lifecycle: The temporary listener is correctly cleaned up, preventing memory leaks. ✅
2. Timeout Handling: Using withTimeoutOrNull() is efficient and does not block indefinitely. ✅
3. Coroutine Context: Using Dispatchers.IO for PTY writes is appropriate. ✅

---

🏗️ Architectural Feedback

1. Separation of Concerns: The feature is well-isolated with clear boundaries between settings, UI, and terminal logic. ✅
2. LaunchedEffect Fix: Adding tabController.tabs.size to the effect keys (commit 5254b15) is correct and necessary. ✅
3. Split Pane Focus: Moving focus handling to onPaneFocus callback (commit ea3d4fd) is a cleaner abstraction. ✅

---

✅ Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐	Clean, well-structured, follows project conventions
Functionality	⭐⭐⭐⭐⭐	Feature-complete with excellent OSC 133 integration
Error Handling	⭐⭐⭐	Missing try-catch around handle.write()
Documentation	⭐⭐⭐⭐	Good inline docs, needs CLAUDE.md update
Test Coverage	⭐⭐	No unit tests for new functionality
Security	⭐⭐⭐⭐	Safe design, minor documentation improvements suggested

---

🚀 Recommendation

APPROVE with minor fixes:

1. Fix the race condition by registering listener before PTY start (HIGH PRIORITY)
2. Add try-catch around handle.write() (MEDIUM PRIORITY)
3. Consider adding unit tests (MEDIUM PRIORITY)

The core implementation is excellent. The OSC 133 integration is particularly well-done and aligns perfectly with the project's shell integration standards. Once the race condition is addressed, this is ready to merge.

---

🤖 Generated with Claude Code