feat: Preserve terminal session across recomposition (#104)

[kshivang]

Summary

• Move session ownership from EmbeddableTerminal composable to EmbeddableTerminalState
• Terminal process now survives when EmbeddableTerminal leaves the composition tree
• Add autoDispose parameter to rememberEmbeddableTerminalState() for lifecycle control

Changes

• EmbeddableTerminalState now owns session lifecycle (create/dispose)
• Add dispose() public method for explicit cleanup
• Add isDisposed property to check session state
• EmbeddableTerminal becomes a "view" that attaches to state.session
• Backward compatible: state param remains optional (defaults to auto-dispose)

Usage Patterns

1. Auto-dispose (default - current behavior):

val state = rememberEmbeddableTerminalState()
EmbeddableTerminal(state = state)
// Session disposed when composable leaves tree

2. Preserved across navigation (new capability):

val state = rememberEmbeddableTerminalState(autoDispose = false)

if (showTerminal) {
    EmbeddableTerminal(state = state)
}
// Terminal survives hide/show!

DisposableEffect(Unit) { onDispose { state.dispose() } }

3. App-level state:

val terminalState = EmbeddableTerminalState()

@Composable fun App() {
    EmbeddableTerminal(state = terminalState)
    DisposableEffect(Unit) { onDispose { terminalState.dispose() } }
}

Test plan

• Navigate away from terminal screen, navigate back → process still running
• Conditional hide/show terminal → history preserved
• Multiple terminals with separate states → independent processes
• Explicit dispose → process killed
• Auto-dispose on app exit → no orphan processes

Closes #104

🤖 Generated with Claude Code

[claude]

Code Review - PR #113: Preserve terminal session across recomposition

Thank you for this well-designed feature implementation! The architectural change to move session ownership from the composable to the state holder is exactly the right approach for solving #104. Here's my detailed review:

---

✅ Strengths

1. Excellent Architecture: Moving session lifecycle to EmbeddableTerminalState is the correct solution. This follows Compose best practices where state holders own business logic and composables are presentation-only.

2. Backward Compatibility: The autoDispose parameter with a sensible default (true) ensures existing code continues to work without changes. Nice defensive design.

3. Clear Documentation: The KDoc examples for all three usage patterns (auto-dispose, preserved, app-level) are comprehensive and helpful.

4. Clean Separation of Concerns: The initializeSession() guard with the initialized flag prevents double-initialization elegantly.

5. Proper Error Handling: The LaunchedEffect dependencies are correct, and session nullability is handled throughout.

---

🐛 Critical Issues

1. Race Condition in Session Initialization (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:114-125)

Problem: The LaunchedEffect keys include effectiveState, resolvedSettings, and effectiveCommand. If settings or command change, the effect will re-execute but the initialized flag prevents re-initialization. This could leave users with a session using old settings.

LaunchedEffect(effectiveState, resolvedSettings, effectiveCommand) {
    if (effectiveState.session == null) {
        effectiveState.initializeSession(
            // ... uses resolvedSettings and effectiveCommand
        )
    }
}

Impact: If a user changes settings or command at runtime, the session won't update.

Recommendation: Either:

• Option A: Remove resolvedSettings and effectiveCommand from keys (since sessions are meant to be long-lived)
• Option B: Add logic to recreate sessions when settings/command change (dispose old, create new)

I'd recommend Option A since the feature is about session preservation. Settings changes could be documented as requiring explicit dispose() + re-composition.

---

2. Memory Leak with Non-Auto-Dispose State (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:255-259)

Problem: When autoDispose = false, if the developer forgets to call dispose(), the terminal process and all its coroutines will leak. The coroutineScope uses SupervisorJob() + Dispatchers.Default which won't be cancelled automatically.

Current Code:

fun dispose() {
    session?.dispose()
    session = null
    initialized = false
}

Missing: No warning in logs or runtime checks to help developers catch this common mistake.

Recommendations:

1. Add a finalize() method (or Cleaner in modern Java) that logs a warning if disposed is never called
2. Add a note in the isDisposed property documentation
3. Consider adding a LeakCanary-style detection in debug builds

Example Addition:

class EmbeddableTerminalState {
    private var disposed = false
    
    fun dispose() {
        if (disposed) {
            println("WARNING: dispose() called multiple times on EmbeddableTerminalState")
            return
        }
        disposed = true
        session?.dispose()
        session = null
        initialized = false
    }
    
    protected fun finalize() {
        if (initialized && !disposed) {
            println("WARNING: EmbeddableTerminalState finalized without dispose() call - process may leak")
        }
    }
}

---

3. Callback Staleness (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:231, 246)

Problem: The onOutput and onExit callbacks are captured once during initialization. If the composable is recomposed with new callback instances (common in Compose), the old callbacks will still be invoked.

Current Code:

internal fun initializeSession(
    // ...
    onOutput: ((String) -> Unit)?,
    onExit: ((Int) -> Unit)?
) {
    if (initialized) return
    // Callbacks are captured here and never updated
}

Impact: If a parent composable updates onOutput or onExit, the changes won't take effect.

Recommendation: Store callbacks as mutable state in EmbeddableTerminalState and provide update methods:

class EmbeddableTerminalState {
    internal var onOutputCallback: ((String) -> Unit)? = null
    internal var onExitCallback: ((Int) -> Unit)? = null
    
    internal fun updateCallbacks(
        onOutput: ((String) -> Unit)?,
        onExit: ((Int) -> Unit)?
    ) {
        onOutputCallback = onOutput
        onExitCallback = onExit
    }
}

// In EmbeddableTerminal composable
LaunchedEffect(onOutput, onExit) {
    effectiveState.updateCallbacks(onOutput, onExit)
}

---

⚠️ Medium Issues

4. Thread Safety of initialized Flag (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:195, 233)

Problem: The initialized flag is not thread-safe. Multiple LaunchedEffect coroutines could potentially check and set it concurrently.

private var initialized = false  // Not @Volatile, not Atomic

Recommendation: Use AtomicBoolean:

private val initialized = AtomicBoolean(false)

internal fun initializeSession(...) {
    if (!initialized.compareAndSet(false, true)) return
    // ... rest of initialization
}

---

5. Null Safety in initializeProcess (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:240-248)

Problem: The session!! force unwrap is unsafe if dispose() is called concurrently during initialization.

session?.coroutineScope?.launch {
    initializeProcess(
        session = session!!,  // Could throw NPE
        // ...
    )
}

Recommendation: Capture session reference before launching:

val capturedSession = session ?: return
capturedSession.coroutineScope.launch {
    initializeProcess(
        session = capturedSession,
        // ...
    )
}

---

6. isDisposed Property Logic (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:212-213)

Problem: The logic session == null && initialized means isDisposed returns false before the first initialization, which is technically correct but potentially confusing.

Current Behavior:

• Before first use: isDisposed = false (not initialized yet)
• After dispose: isDisposed = true
• After dispose + re-initialization: isDisposed = false (session reused)

Recommendation: This is actually fine, but consider adding documentation to clarify this state machine. Alternatively, use a three-state enum: UNINITIALIZED, ACTIVE, DISPOSED.

---

📊 Performance Considerations

1. LaunchedEffect Re-execution (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:114): Having resolvedSettings and effectiveCommand as keys means the effect runs on every settings change even though it's a no-op after first init. Consider optimizing keys.

2. Font Loading (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:109): The font is loaded in the composable scope. If using autoDispose=false, the font could be loaded multiple times unnecessarily. Consider caching at app level.

---

🔒 Security Considerations

1. Process Lifecycle: ✅ Good - processes are properly tracked and disposed
2. Command Injection: ✅ Safe - command is passed as-is without shell expansion
3. Environment Variables: ✅ Safe - environment map is merged without interpretation

No security concerns identified.

---

🧪 Test Coverage

Missing Tests:

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

1. Unit Tests for EmbeddableTerminalState:

   • Test initialized flag prevents double initialization
   • Test dispose() clears session and resets initialized
   • Test isDisposed property states
   • Test concurrent initialization attempts

2. Integration Tests for Session Preservation:

   • Create state with autoDispose=false, remove from composition, re-add → session persists
   • Create state with autoDispose=true, remove from composition → session disposed
   • Test callback updates during session lifetime

3. Memory Leak Tests:

   • Verify no leaks with autoDispose=true
   • Verify explicit dispose() cleans up all resources

---

📝 Minor Suggestions

1. Documentation Enhancement (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:255): Add @Throws annotation if dispose() can throw exceptions

2. Naming Clarity: Consider renaming initialized to sessionCreated for clarity

3. Defensive Copy (compose-ui/src/desktopMain/kotlin/ai/rever/bossterm/compose/EmbeddableTerminal.kt:229): The environment map could be copied to prevent external mutation

---

Summary

Overall Assessment: ⭐⭐⭐⭐ (4/5) - Excellent design with some critical issues to address

Must Fix Before Merge:

1. Race condition with settings/command changes in LaunchedEffect
2. Callback staleness issue
3. Thread safety of initialized flag

Recommended Before Merge:

1. Memory leak detection/warnings
2. Null safety in initializeProcess
3. Add automated tests

Nice to Have:

1. Performance optimizations
2. Documentation improvements

The architectural approach is sound and solves the issue elegantly. With the critical fixes applied, this will be a solid addition to the codebase. Great work on the documentation and backward compatibility! 🎉

---

Reviewed with: BossTerm CLAUDE.md guidelines
Focus areas: Code quality, thread safety, Compose lifecycle, resource management

[claude]

Code Review: Preserve terminal session across recomposition (#104)

Summary

This PR implements a significant architectural improvement by moving session lifecycle ownership from the EmbeddableTerminal composable to EmbeddableTerminalState. This enables terminal processes to survive navigation/visibility changes while maintaining backward compatibility. Overall, this is a well-designed and thoughtfully implemented feature.

---

✅ Strengths

Architecture

• Clean separation of concerns: State ownership now lives in EmbeddableTerminalState, making the composable a pure "view" layer
• Excellent backward compatibility: Default behavior (auto-dispose) matches existing usage patterns
• Flexible lifecycle control: The autoDispose parameter provides both convenience and control
• Proper Compose patterns: Uses remember, LaunchedEffect, and DisposableEffect correctly

Documentation

• Comprehensive KDoc: All three usage patterns are clearly documented with code examples
• README update: Shows practical usage with conditional visibility
• Clear naming: isDisposed, autoDispose, initializeSession are self-explanatory

Safety

• Idempotent initialization: initializeSession() guards against multiple calls with the initialized flag
• Proper cleanup: dispose() resets state, allowing reuse
• Null-safe rendering: Guards against rendering before session initialization

---

🔍 Issues & Recommendations

1. CRITICAL: Race Condition in Session Initialization

Location: EmbeddableTerminal.kt:114-125

The LaunchedEffect key includes effectiveState, resolvedSettings, and effectiveCommand. If any of these change (e.g., user changes settings), the effect will restart and call initializeSession() again. While the initialized flag prevents duplicate initialization, this could lead to parameter mismatch issues where new settings are ignored.

Recommendation: Remove dependencies that should not trigger reinitialization. Only depend on the state instance itself, or add validation to warn when parameters change on an initialized state.

---

2. Memory Leak Risk: Callback Retention

Location: EmbeddableTerminal.kt:236-248

The initializeSession() method captures onOutput and onExit callbacks and stores them indirectly. If a user passes lambdas that capture the composable scope, the session will hold references to potentially stale/disposed UI components when using autoDispose=false.

Recommendations:

1. Document this behavior in KDoc for rememberEmbeddableTerminalState
2. Consider adding an updateCallbacks() method to refresh callbacks when recomposing
3. Or, accept callbacks in EmbeddableTerminal instead of initializeSession, wiring them up via LaunchedEffect

---

3. Inconsistent Lifecycle with autoDispose=false

Location: EmbeddableTerminal.kt:350-356

When autoDispose=false, the user must manually call state.dispose(). However:

• No warning/logging if the state is GC'd without disposal
• No finalizer to kill orphan processes
• No way to detect if disposal was forgotten

Recommendation: Add a finalizer to log warnings about undisposed sessions and document prominently in KDoc that forgetting dispose() creates orphan processes.

---

4. Double Coroutine Scope Potential Issue

Location: EmbeddableTerminal.kt:240

initializeSession() launches process initialization inside session.coroutineScope, but createTerminalSession() already creates a coroutine scope. Then initializeProcess() launches 3 more coroutines in the same scope.

Concern: If initializeSession() is somehow called twice (despite guards), you would have duplicate coroutines running.

Recommendation: Move the process launch outside of a nested coroutine to avoid potential duplicate launches.

---

5. Missing State Validation

Location: EmbeddableTerminalState.kt (various methods)

Methods like write(), paste(), scrollBy(), etc., silently no-op when session == null. This could hide bugs where code expects the terminal to be ready.

Recommendation: Add explicit checks or logging, or throw exceptions for programmer errors to make issues more visible.

---

6. Title Change Callback Wiring

Location: EmbeddableTerminal.kt:131-139

The title change callback is wired up via LaunchedEffect that depends on session and onTitleChange. If onTitleChange changes during recomposition, the effect restarts but the old collectLatest might still be running briefly.

Recommendation: Use rememberUpdatedState to avoid effect restarts when callbacks change.

---

7. Render-Before-Ready Risk

Location: EmbeddableTerminal.kt:150-159

The ProperTerminal is rendered as soon as session != null, but the process might not be started yet (initialization could fail asynchronously). This means UI could render an "empty" terminal before connection.

Recommendation: Consider showing a loading indicator until isConnected == true.

---

🧪 Testing Recommendations

The PR description includes a good test plan. I would add:

1. Stress test: Rapidly toggle showTerminal visibility to ensure no race conditions
2. Memory test: Create/dispose 100 terminals with autoDispose=false, verify no leaks
3. Callback test: Verify onOutput/onExit still fire after hide/show cycle
4. Parameter change test: What happens if settings change after initialization?
5. Multi-terminal test: Multiple EmbeddableTerminalState instances with independent lifecycles

---

🔒 Security Considerations

No obvious security issues. The PR maintains existing process isolation and does not introduce new user input paths.

---

⚡ Performance Considerations

• Memory: Each EmbeddableTerminalState now retains full session state when hidden (expected behavior, but document memory implications)
• Background CPU: Hidden terminals continue processing output (by design, but users should know)
• GC Pressure: No new allocations beyond the state object itself

---

📝 Suggested Improvements

1. Add an isInitialized public property for external state checking
2. Consider exposing initialized as read-only public for debugging
3. Add logging (debug level) for lifecycle events: initialize, dispose, state changes
4. Create unit tests for EmbeddableTerminalState lifecycle (init, dispose, reuse)

---

Conclusion

This is a well-architected feature that solves a real usability problem. The main concerns are:

1. Race condition in LaunchedEffect keys (needs fixing)
2. Memory leak potential with callbacks (needs documentation at minimum)
3. Missing disposal safeguards (nice-to-have improvements)

Recommendation: Fix #1, document #2, and consider improvements for #3-7 before merging.

Great work! 🎉

---

Reviewed with BossTerm CLAUDE.md conventions and Compose best practices

[claude]

PR Review: Preserve terminal session across recomposition

Overview

This PR successfully addresses issue #104 by moving session ownership from EmbeddableTerminal to EmbeddableTerminalState, allowing terminal processes to survive when the composable leaves composition.

Strengths

1. Clean API Design with autoDispose parameter
2. Well-documented with excellent kdoc and README examples
3. Backward compatible
4. Proper lifecycle initialization guard

Critical Issues

1. Race Condition in Session Initialization (EmbeddableTerminal.kt:114-121)
The session null check and initializeSession() call are not atomic. Multiple LaunchedEffect blocks could pass the null check concurrently, leading to multiple process spawns and resource leaks.
Fix: Add synchronized block or AtomicBoolean to ensure thread-safe initialization.

2. Resource Leak in Dispose (EmbeddableTerminalState.kt:255-258)
The initialization coroutine job is not explicitly cancelled in dispose(), potentially causing memory leaks if state is reused.
Fix: Store and cancel the initialization job reference.

3. Connection State Observation Issue (EmbeddableTerminal.kt:142-143)
The connectionState value is read once but changes aren't observed. onReady may not fire if state transitions after initial read.
Fix: Use proper flow collection with snapshotFlow().

4. Documentation Gap
The dispose() kdoc claims it kills the process, but implementation only cancels coroutines. Process termination relies on GC closing PTY file descriptors.
Fix: Update documentation to accurately reflect behavior.

Summary

Well-designed feature with excellent API, but needs thread-safety fixes before merging. The race condition (#1) is critical and must be addressed.

Great work overall! The architectural direction is sound, just needs thread-safety polish.