feat: T6 Split Pane & T7 Reactive State plugin APIs

[kshivang]

Summary

• T6 Split Pane API: Public methods on TabbedTerminalState for programmatic split pane management — splitVertical(), splitHorizontal(), closeFocusedPane(), navigatePaneFocus(direction), navigateToNextPane(), navigateToPreviousPane(), getPaneCount(), hasSplitPanes(), getSplitSessionIds(), writeToFocusedPane()
• T7 Reactive State API: Observable tabsFlow and activeTabIndexFlow StateFlows on TabbedTerminalState, plus TerminalTabInfo data class with tab id, title, connection state, working directory, and pane count
• tabbed-example demo: New "API Demo" view with interactive split pane controls, live pane state display, directional navigation buttons, tab controls, and a reactive status bar that updates in real-time from T7 flows

Test plan

• Run ./gradlew :tabbed-example:run and switch to "API Demo" view
• Click Split Vertical / Split Horizontal and verify panes are created
• Observe reactive status bar updates live as tabs and panes change
• Test pane navigation buttons (Next, Previous, directional)
• Test "Write to Focused Pane" button sends text to correct pane
• Test "Create Tab" / "Close Active Tab" buttons
• Verify View menu "Split Vertical (API)" / "Split Horizontal (API)" / "Close Pane (API)" items work
• Verify pane count badge appears in ViewSwitcherBar when splits exist
• Switch between Terminal and API Demo views — terminal state persists

🤖 Generated with Claude Code

[claude]

Code Review: T6 Split Pane & T7 Reactive State Plugin APIs

Overall this is a well-structured addition with good documentation. The reactive state bridging pattern (snapshot state → StateFlow) is the right approach for exposing Compose state to non-Compose consumers. A few issues worth addressing before merge:

---

Issues

1. newSessionRef circular capture — fragile pattern

In performSplit():

var newSessionRef: TerminalSession? = null
val newSession = controller.createSessionForSplit(
    onProcessExit = {
        newSessionRef?.let { session -> ... }  // reads newSessionRef
    }
)
newSessionRef = newSession  // assigned AFTER the lambda is passed

This relies on onProcessExit never being called synchronously during createSessionForSplit. That's a safe assumption for a PTY process exit, but the code is unnecessarily fragile. A cleaner approach:

val newSession = controller.createSessionForSplit(workingDir = workingDir, onProcessExit = {})
newSession.onProcessExit = {
    if (splitState.isSinglePane) { ... }
    else {
        splitState.getAllPanes().find { it.session === newSession }
            ?.let { pane -> splitState.closePane(pane.id) }
    }
}

This captures newSession directly (already assigned by the time the lambda runs) and removes the mutable var.

---

2. paneCount in toTabInfo() may not be reactive

paneCount = splitStates[this.id]?.getAllPanes()?.size ?: 1

splitStates is a SnapshotStateMap, so changes to the map itself (adding/removing tabs) will be tracked. However, changes to a tab's pane tree (splitting or closing panes within an existing tab) will only trigger tabsFlow updates if SplitViewState.getAllPanes() reads from snapshot-backed state internally.

If SplitViewState uses a regular data structure for its pane tree, tabsFlow will have stale paneCount values after a split operation—which would silently break any T7 consumer relying on pane counts. This should be verified and documented either way.

---

3. Logic duplication in getOrCreateSplitState()

The comment acknowledges this: "Mirrors the logic from TabbedTerminal composable". This creates a maintenance liability—two code paths setting up onProcessExit for the initial tab session with no shared abstraction between them. If the composable's initialization logic changes, this copy may silently drift.

Consider extracting the session-exit wiring into a shared private function, or documenting an explicit test/assertion that these two paths must remain equivalent.

---

4. ViewSwitcherBar parameter over-coupling

The signature changed from:

terminalTabCount: Int

to:

terminalState: TabbedTerminalState

The composable only actually uses terminalState.tabsFlow and terminalState.activeTabIndexFlow. Passing the entire state object increases the API surface unnecessarily and makes the component harder to test or reuse. Prefer passing the minimal required values:

fun ViewSwitcherBar(
    currentView: AppView,
    onViewChange: (AppView) -> Unit,
    tabsFlow: StateFlow<List<TerminalTabInfo>>,
    activeTabIndexFlow: StateFlow<Int>
)

---

5. TerminalTabInfo missing @Immutable

data class TerminalTabInfo(
    val id: String,
    val title: String,
    val isConnected: Boolean,
    val workingDirectory: String?,
    val paneCount: Int
)

All fields are immutable primitives, but Compose's compiler plugin can't infer stability for classes in non-Compose modules. Adding @Immutable (or @Stable) gives Compose stronger guarantees for skip-based recomposition optimization—especially relevant since this type will appear in List<TerminalTabInfo> StateFlows that drive multiple composables.

---

6. ReactiveStatusBar is always visible

ReactiveStatusBar is added unconditionally to the window layout, so it appears on every view (Terminal, Settings, API Demo). If the intent is to demonstrate the T7 API specifically, it should probably be scoped to the API_DEMO view. If it's intended as a permanent feature of the app, the current placement is fine—but the "Reactive" badge reads as a debug artifact rather than a production UI element.

---

Minor / Nits

• settingsFlow exposes a global singleton through an instance property: val settingsFlow get() = SettingsManager.instance.settings. This is fine as a convenience but could surprise consumers who expect settingsFlow to be per-instance state like tabsFlow and activeTabIndexFlow. A comment noting it's process-global would help.

• PluginApiDemoPanel reads getPaneCount()/hasSplitPanes()/getSplitSessionIds() directly in the composable body: These will be reactive only if the underlying splitStates/SplitViewState internals are snapshot-backed—same concern as issue 🔍 Implement text search/find functionality (Ctrl+F) #2. If they work correctly, a comment explaining the reactivity contract would be useful.

• Box layout change from fillMaxSize() to weight(1f).fillMaxWidth(): This is a correct fix to accommodate the status bar, but it's a subtle behavioral change for any app that embeds TabbedTerminalWindow. Worth noting in the PR description for downstream awareness.

---

Summary

The API design is clean and the snapshotFlow bridging approach is idiomatic. The main items to address before merge are the newSessionRef fragility (#1), verifying pane count reactivity in tabsFlow (#2), and the getOrCreateSplitState() duplication risk (#3). The others are lower-priority improvements.

[claude]

PR Review: T6 Split Pane & T7 Reactive State plugin APIs

Overall this is a solid addition — the API surface is clean, documentation is thorough, and the demo panel does a good job exercising the new APIs. A few issues worth addressing before merging.

---

Bug (High): Coroutine scope leak in initialize()

File: TabbedTerminalState.kt (the initialize() method, T7 section)

flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { scope ->
    scope.launch { ... }
    scope.launch { ... }
}

If initialize() is called more than once (which can happen in Compose when TabbedTerminal recomposes with the same TabbedTerminalState), the previous flowScope is silently overwritten without being cancelled. The two launched coroutines from the old scope continue running indefinitely, accumulating with each re-initialization.

Fix: cancel before reassigning:

flowScope?.cancel()
flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { scope ->
    scope.launch { ... }
    scope.launch { ... }
}

---

Bug (Medium): closeFocusedPane() fails on never-split tabs

File: TabbedTerminalState.kt, closeFocusedPane()

val splitState = splitStates[resolvedTabId] ?: return false

A tab that has never been split has no entry in splitStates. So calling closeFocusedPane() on a single-pane tab that was never split returns false and does nothing, even though the intended contract is "if it's the last pane, close the entire tab." The splitStates entry only exists after getOrCreateSplitState() has been called (i.e., after a split has occurred). Any caller that hasn't first called splitVertical() / splitHorizontal() will silently fail.

Compare to getPaneCount() which correctly handles the no-split-state case by returning 1.

Fix: fall through to tab close when splitState is absent (treat it as the single-pane case).

---

Concern (Medium): Silent onProcessExit skip in performSplit()

File: TabbedTerminalState.kt, performSplit()

(newSession as? TerminalTab)?.onProcessExit = { ... }

If createSessionForSplit() ever returns a non-TerminalTab implementation, onProcessExit is silently not set. The pane would then become a zombie — it stays open after the process exits. A non-nullable cast with a clear error message, or an explicit check before proceeding, would surface this at development time rather than silently producing a stuck pane at runtime.

---

Design (Low): settingsFlow exposes a singleton directly

val settingsFlow: StateFlow<TerminalSettings> get() = SettingsManager.instance.settings

This getter re-exposes a global singleton's internals. Callers get a StateFlow that's backed by SettingsManager.instance, so any change to how SettingsManager works (e.g. lazy initialization, scoped instances) silently affects this contract. Consistent with the other two flows, this could be bridged through the same flowScope infrastructure, or at minimum the getter dependency on the singleton should be documented.

---

Design (Low): resolveTabId() is too trivial to justify its own function

private fun resolveTabId(tabId: String?): String? {
    return tabId ?: activeTabId
}

This function body is a single expression that's no more readable than its inline form. Every call site reads resolveTabId(tabId) where tabId ?: activeTabId would be equally clear and avoid the indirection. It's not reused enough to warrant being a named function.

---

Minor: paneCount = 0 vs paneCount = 1 inconsistency in TerminalTabInfo

In toTabInfo():

paneCount = splitStates[this.id]?.getAllPanes()?.size ?: 1

A tab always has at least 1 pane, so TerminalTabInfo.paneCount is always ≥ 1. But in ViewSwitcherBar:

val paneCount = activeTab?.paneCount ?: 0

The fallback of 0 is used when activeTab is null (no tabs at all), not when the pane count is unknown. The TerminalTabInfo docs should clarify that paneCount is always ≥ 1 (never 0), and downstream consumers shouldn't conflate "no tab" (null) with "zero panes" (0).

---

Non-reactive reads in PluginApiDemoPanel

val paneCount = terminalState.getPaneCount()
val hasSplits = terminalState.hasSplitPanes()
val sessionIds = terminalState.getSplitSessionIds()

These read from splitStates (a SnapshotStateMap), so Compose will recompose when splitStates changes — that part is correct. However, these calls also internally read from SplitViewState.getAllPanes(). If that method returns a plain List (not snapshot-backed), changes to pane structure within an existing split state won't trigger recomposition. Worth verifying that getAllPanes() reads from observable snapshot state, or switching to the reactive tabsFlow path for consistency with the status bar.

---

Summary

Severity	Issue
High	Coroutine scope leak when initialize() called multiple times
Medium	closeFocusedPane() does nothing on never-split tabs
Medium	Silent onProcessExit skip if session isn't a TerminalTab
Low	settingsFlow couples directly to singleton internals
Low	resolveTabId() is an unnecessary helper
Minor	paneCount = 0 vs ≥ 1 semantic inconsistency

The coroutine leak and closeFocusedPane() behavior are the ones most likely to surface as runtime bugs. The rest are polish/robustness items.

🤖 Generated with Claude Code

[claude]

PR Review: T6 Split Pane & T7 Reactive State Plugin APIs

Overall this is a solid addition — the public API surface is clean, KDoc is thorough, and the demo panel gives a good visual walkthrough. A few issues worth addressing before merge.

---

Bugs

1. flowScope leak on re-initialization

TabbedTerminalState.kt, initialize() block (~line 55):

flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { scope -> ... }

If initialize() is called more than once without an intervening dispose() (e.g., if the composable reattaches), the old flowScope is silently orphaned — two coroutines now emit into the same MutableStateFlow and neither is ever cancelled. Fix:

flowScope?.cancel()
flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { ... }

2. Non-reactive state reads in PluginApiDemoPanel

Main.kt, PluginApiDemoPanel composable:

val paneCount = terminalState.getPaneCount()
val hasSplits = terminalState.hasSplitPanes()
val sessionIds = terminalState.getSplitSessionIds()

These are plain method calls — they are not tracked by the Compose snapshot system, so the "Live Pane State" card will not recompose when pane state changes. The T7 flow already carries paneCount per tab; use it:

val activeTab = tabs.getOrNull(activeTabIndex)
val paneCount = activeTab?.paneCount ?: 1
val hasSplits = paneCount > 1
// sessionIds still needs a reactive source or a snapshotFlow bridge

---

Code Quality

3. Duplicated split state creation in getOrCreateSplitState()

The method replicates the onProcessExit wiring that already exists inside the TabbedTerminal composable. The comment acknowledges this ("Mirrors the logic from TabbedTerminal composable"), but that's the risk — they can drift independently. Consider extracting the wiring into a shared internal helper so there is one canonical location.

4. Hard crash via error() in performSplit()

val newTab = newSession as? TerminalTab
    ?: error("createSessionForSplit returned ${newSession::class.simpleName}, expected TerminalTab")

error() throws IllegalStateException and will crash the process if the contract is violated. Since this is defensive programming against an internal API, an error log + returning null might be preferable for a library:

val newTab = newSession as? TerminalTab ?: run {
    // log error
    return null
}

5. settingsFlow leaks a singleton reference

val settingsFlow: StateFlow<TerminalSettings> get() = SettingsManager.instance.settings

This ties the public API directly to SettingsManager.instance. If SettingsManager is ever made injectable or testable, callers will depend on the singleton. At minimum, document that this returns the global settings flow.

---

Minor / Polish

6. Working directory overflow in ReactiveStatusBar

The CWD is rendered in a Row with no width constraint or overflow handling. A deep path (e.g., /home/user/projects/very/deep/path) will overflow or push the "Reactive" badge off-screen. Consider:

Text(
    text = cwd,
    maxLines = 1,
    overflow = TextOverflow.Ellipsis,
    modifier = Modifier.weight(1f, fill = false)
)

7. closeFocusedPane() closes entire tab — caller surprise

The docstring is clear about this, but it might still be unexpected for callers who only want pane-level control. Worth adding a @return note distinguishing "pane closed" vs "tab closed" in the return value, or exposing a separate closeActiveTab(tabId) at the call site so callers can distinguish intent.

---

Positive Highlights

• SupervisorJob + Dispatchers.Main is the right scope for bridging snapshot state → StateFlow.
• Private _tabsFlow/_activeTabIndexFlow with public StateFlow facade is the correct pattern.
• @Immutable on TerminalTabInfo is accurate (all vals, no mutable-by-reference fields) and will help Compose skip unnecessary recompositions.
• Default tabId: String? = null consistently falling back to the active tab is ergonomic and makes the API pleasant to use.
• KDoc on all public methods is thorough and accurate.

---

Priority summary:

• Must fix before merge: feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1 (scope leak), 🔍 Implement text search/find functionality (Ctrl+F) #2 (stale UI reads)
• Should fix: 🔗 Implement hyperlink detection and click handling #3 (duplicated wiring), ⚙️ Implement user settings and configuration system #4 (crash vs graceful failure)
• Nice to have: 🌏 Implement IME (Input Method Editor) support for CJK languages #5, 📝 Add context menu / right-click menu #6, 📂 Implement multiple terminal sessions/tabs #7

🤖 Generated with Claude Code

[claude]

PR Review: T6 Split Pane & T7 Reactive State Plugin APIs

Overall this is a well-structured addition — the API surface is clean, the KDoc is thorough, and the demo panel does a good job exercising the new functionality. A few issues worth addressing before merge:

---

Critical: Coroutine scope leak in initialize()

TabbedTerminalState.kt lines ~55-64:

flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { scope ->
    scope.launch { snapshotFlow { ... }.collect { ... } }
    scope.launch { snapshotFlow { ... }.collect { ... } }
}

If initialize() is called more than once without an intervening dispose() (which Compose can do during re-composition or configuration changes), the previous flowScope is silently orphaned — its coroutines keep running and hold references to tabController. Add a guard:

flowScope?.cancel()  // cancel any previous scope before creating a new one
flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { ... }

---

Bug: error() in a public API path (performSplit)

TabbedTerminalState.kt lines ~164-165:

val newTab = newSession as? TerminalTab
    ?: error("createSessionForSplit returned ${newSession::class.simpleName}, expected TerminalTab")

error() throws IllegalStateException and will crash the caller. Since splitVertical() / splitHorizontal() already return String? to signal failure, this should return null instead of throwing:

val newTab = newSession as? TerminalTab ?: return null

---

Bug: Non-reactive reads in PluginApiDemoPanel

Main.kt lines ~952-954:

val paneCount = terminalState.getPaneCount()
val hasSplits = terminalState.hasSplitPanes()
val sessionIds = terminalState.getSplitSessionIds()

These are called directly in a @Composable context. Whether they trigger recomposition depends on whether SplitViewState.getAllPanes() internally reads snapshot state — that's an implementation detail that isn't guaranteed here. Worse, this is inconsistent with the T7 demo being presented: paneCount is already exposed via tabs.getOrNull(activeTabIndex)?.paneCount. Use the flow data already collected above, or wrap in remember { derivedStateOf { ... } }:

val activeTab = tabs.getOrNull(activeTabIndex)
val paneCount = activeTab?.paneCount ?: 0
val hasSplits = paneCount > 1
val sessionIds = terminalState.getSplitSessionIds()  // still needs a reactive solution

---

Design: getOrCreateSplitState duplicates composable logic

TabbedTerminalState.kt lines ~91-110 — the method mirrors split state initialisation that already lives inside the TabbedTerminal composable. This creates two code paths that must be kept in sync, including the onProcessExit wiring. If getOrCreateSplitState runs first (before the composable renders), the state is set up one way; if the composable runs first, it may overwrite things differently. A comment noting the risk and an integration test (or at least a // TODO: consolidate with composable) would help signal the fragility.

---

Design: settingsFlow leaks internal singleton

val settingsFlow: StateFlow<TerminalSettings> get() = SettingsManager.instance.settings

This directly exposes SettingsManager.instance, coupling consumers to the global singleton. Minor, but it breaks the abstraction boundary that the rest of TabbedTerminalState maintains. Consider wrapping: SettingsManager.instance.settings.asStateFlow() or just documenting the dependency clearly.

---

API consistency: getSplitSessionIds vs hasSplitPanes

When no split state exists:

• hasSplitPanes() returns false
• getSplitSessionIds() returns listOf(resolvedTabId) (non-empty)

Callers iterating over getSplitSessionIds() when hasSplitPanes() == false will see one entry. This is defensible but surprising. Either document this explicitly in the KDoc or return emptyList() when isSinglePane is true.

---

Documentation gaps

1. getFocusedSplitSession(tabId?) is a public method but is absent from both docs/wiki/API-Reference.md and the Split Pane method tables in Tabbed-Terminal-Guide.md.
2. navigateToNextPane and navigateToPreviousPane are missing from the method table in Tabbed-Terminal-Guide.md (present in tabbed-terminal.md and API-Reference.md but not the guide).

---

Minor: ViewSwitcherBar receiving StateFlow parameters

Passing StateFlow directly to a composable works but is unusual in Compose; the more common pattern is passing observable state values or the parent state object. The comment explains the rationale (decoupling for testability), so this is acceptable — just worth a note that callers must ensure the flows are live before the composable is first drawn.

---

Summary

Severity	Issue
Critical	flowScope not cancelled before re-initialisation
Bug	error() crash in performSplit() — should return null
Bug	Non-reactive pane state reads in PluginApiDemoPanel
Design	getOrCreateSplitState duplicates composable logic
Design	settingsFlow exposes internal singleton
API	getSplitSessionIds / hasSplitPanes inconsistency
Docs	getFocusedSplitSession undocumented; nav methods missing from guide table

The critical and bug items should be fixed before merge. The rest are good-to-address but lower priority.

[claude]

PR Review: T6 Split Pane & T7 Reactive State Plugin APIs

Overall this is a solid, well-documented addition. The API surface is clean, the snapshotFlow -> StateFlow bridging pattern is idiomatic, and the demo panel does a good job exercising the new APIs interactively. A few issues worth addressing before merge:

---

Bugs / Correctness

1. flowScope leaks on re-initialization

In TabbedTerminalState.kt, the initialization block unconditionally overwrites flowScope without cancelling the previous one:

// Current code - old scope is leaked if initialize() is called twice
flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { scope -> ... }

If TabbedTerminal composable is ever re-called on the same state instance (e.g., after an error recovery), the previous coroutines keep running and observing stale snapshot state. Fix:

flowScope?.cancel()
flowScope = CoroutineScope(SupervisorJob() + Dispatchers.Main).also { scope -> ... }

---

2. error() in performSplit() can crash the app

val newTab = newSession as? TerminalTab
    ?: error("createSessionForSplit returned <type>, expected TerminalTab")

error() throws IllegalStateException which is unhandled at the call site. For a public API method, callers expect a null return on failure rather than an uncaught exception. Suggest returning null with a log instead, consistent with the rest of performSplit().

---

3. Pane state reads in PluginApiDemoPanel are not fully reactive

val paneCount = terminalState.getPaneCount()
val hasSplits = terminalState.hasSplitPanes()
val sessionIds = terminalState.getSplitSessionIds()

These read splitStates (a SnapshotStateMap) so recomposition is triggered by top-level map changes. However, changes within a SplitViewState (e.g., a pane closing internally) won't recompose this block unless SplitViewState itself uses snapshot-observable fields. The T7 tabs from tabsFlow already carries paneCount -- reading from it is more reliable and consistent:

val activeTab = tabs.getOrNull(activeTabIndex)
val paneCount = activeTab?.paneCount ?: 0

---

Design / Maintenance

4. Duplicated SplitViewState creation logic

getOrCreateSplitState() mirrors the SplitViewState creation logic in the TabbedTerminal composable. The comment acknowledges this. This is a real maintenance risk -- if the composable's logic evolves (e.g., new onProcessExit behaviour, initialization parameters), getOrCreateSplitState() can silently diverge. Consider adding a // Keep in sync with TabbedTerminal.kt:L<line> marker at both sites.

---

5. settingsFlow exposes an internal singleton

val settingsFlow: StateFlow<TerminalSettings> get() = SettingsManager.instance.settings

This directly forwards an internal singleton through the public API. Consumers cannot substitute a different SettingsManager in tests, and changes to the singleton's type signature are an implicit breaking change. Worth at least a comment noting it delegates to SettingsManager.instance.

---

Thread Safety

6. Split API methods have no thread-safety documentation

splitVertical(), splitHorizontal(), closeFocusedPane(), etc. mutate splitStates (a SnapshotStateMap) and session objects that expect main-thread access. The KDoc says nothing about threading requirements. At minimum, add a // Must be called on the main thread note or a @MainThread annotation to each public split API method. Unexpected background-thread calls (e.g., from a coroutine on Dispatchers.IO) could cause snapshot state corruption.

---

Minor

• ViewSwitcherBar now accepting StateFlow parameters instead of a raw Int is a good decoupling decision -- makes the composable easier to test.
• Documentation across tabbed-terminal.md, API-Reference.md, and Tabbed-Terminal-Guide.md is thorough and consistent.
• The TerminalTabInfo DTO KDoc is clear; the "always >= 1" pane count contract is respected consistently across getPaneCount() and getSplitSessionIds().

---

Summary

Category	Finding
Bug	flowScope leak on re-initialization (no cancel before overwrite)
Bug	error() crash risk in performSplit() -- public API should return null
Correctness	Non-fully-reactive pane reads in PluginApiDemoPanel
Maintenance	Duplicated SplitViewState creation logic risks silent divergence
API design	Missing thread-safety docs/annotations on all split API methods
Nit	settingsFlow should note it delegates to SettingsManager.instance

The T6/T7 feature set is well-thought-out and the bridging of Compose snapshot state to StateFlow is the right pattern. Addressing the flowScope leak and the error() crash risk before merge would be the priority.

Generated with Claude Code