Skip to content

feat: Add horizontal and vertical split pane support#95

Merged
kshivang merged 8 commits intomasterfrom
dev
Dec 8, 2025
Merged

feat: Add horizontal and vertical split pane support#95
kshivang merged 8 commits intomasterfrom
dev

Conversation

@kshivang
Copy link
Copy Markdown
Owner

@kshivang kshivang commented Dec 8, 2025

Summary

  • Implement iTerm2-style split views for terminal panes within tabs
  • Add tree-based split architecture (SplitNode sealed class hierarchy)
  • Enable vertical splits (Cmd+D) and horizontal splits (Cmd+Shift+H)
  • Support pane navigation with Cmd+Option+arrows and Cmd+]/[
  • Add draggable dividers with 1dp visible line + 16dp invisible drag area
  • Integrate split options into context menu and Shell menu

Features

Split Operations

  • Split Vertically: Creates left/right panes (Cmd+D)
  • Split Horizontally: Creates top/bottom panes (Cmd+Shift+H)
  • Close Pane: Close current split pane

Navigation

  • Directional: Cmd+Option+Arrow keys to navigate UP/DOWN/LEFT/RIGHT
  • Cycle: Cmd+] for next pane, Cmd+[ for previous pane

Visual Feedback

  • Blue border (2dp) on focused pane
  • Gray border (1dp) on unfocused panes (only shown when splits exist)

Resize

  • Drag dividers to resize (10%-90% ratio constraints)
  • Thin visible divider (1dp) with larger invisible drag area (16dp)

New Files

File Description
splits/SplitNode.kt Sealed class: Pane, VerticalSplit, HorizontalSplit
splits/SplitOrientation.kt Enum: HORIZONTAL, VERTICAL
splits/NavigationDirection.kt Enum: UP, DOWN, LEFT, RIGHT
splits/SplitViewState.kt State management for split tree
splits/SplitContainer.kt Recursive composable rendering split tree
splits/SplitDivider.kt Draggable divider with overlay pattern

Test Plan

  • Split pane vertically with Cmd+D
  • Split pane horizontally with Cmd+Shift+H
  • Navigate between panes with Cmd+Option+arrows
  • Navigate with Cmd+] and Cmd+[
  • Resize splits by dragging dividers
  • Close pane when shell exits (type exit)
  • Context menu shows split options
  • Shell menu shows split options
  • New splits inherit working directory from parent

🤖 Generated with Claude Code

Implement iTerm2-style split views for terminal panes within tabs:

Architecture:
- SplitNode: Sealed class hierarchy (Pane, VerticalSplit, HorizontalSplit)
- SplitViewState: Manages tree structure, focus tracking, spatial navigation
- SplitContainer: Recursive composable rendering split tree
- SplitDivider: 1dp visible line + 16dp invisible drag overlay

Features:
- Split panes vertically (Cmd+D) or horizontally (Cmd+Shift+H)
- Navigate between panes with Cmd+Option+arrows or Cmd+]/[
- Resize splits by dragging dividers (10%-90% ratio constraints)
- Focus indication with blue border on active pane
- Auto-close panes when shell exits
- Working directory inheritance for new splits
- Context menu integration for split actions
- Shell menu with split options

Keyboard shortcuts:
- Cmd+D: Split vertically (left/right)
- Cmd+Shift+H: Split horizontally (top/bottom)
- Cmd+Option+arrows: Navigate between panes directionally
- Cmd+]: Navigate to next pane
- Cmd+[: Navigate to previous pane

New files:
- splits/SplitNode.kt: Tree node sealed class
- splits/SplitOrientation.kt: HORIZONTAL/VERTICAL enum
- splits/NavigationDirection.kt: UP/DOWN/LEFT/RIGHT enum
- splits/SplitViewState.kt: State management
- splits/SplitContainer.kt: UI rendering
- splits/SplitDivider.kt: Draggable divider components

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Support

This is a solid implementation of iTerm2-style split panes! The architecture is well-designed with a clean tree-based approach.


✅ Strengths

Architecture & Design

  • Clean tree structure: The SplitNode sealed class hierarchy is elegant and type-safe
  • Immutable operations: Tree transformations use replaceNode, removePane, etc. - excellent functional approach
  • Spatial navigation: Bounds-based directional navigation is well-implemented with overlap detection
  • Proper separation of concerns: State management (SplitViewState), rendering (SplitContainer), and UI (SplitDivider) are cleanly separated

Implementation Quality

  • Ratio constraints: 10%-90% limits prevent unusable panes (SplitNode.kt:35, 49)
  • Cumulative drag tracking: Solves recomposition issues during resize (SplitDivider.kt:83-120)
  • Comprehensive keyboard shortcuts: All major iTerm2 shortcuts implemented
  • Context menu integration: Split options properly added to right-click menu

🐛 Critical Issues

1. Session ID Closure Capture Bug (HIGH PRIORITY)

Location: TabbedTerminal.kt:125-133, 143-151, 230-246

Problem: The sessionId capture pattern is prone to race conditions. If the shell exits immediately, onProcessExit might fire before sessionId is assigned, causing zombie panes.

Solution: Use reference equality instead of ID lookup:

val newSession = tabController.createSessionForSplit(
    workingDir = workingDir,
    onProcessExit = {
        splitState.getAllPanes()
            .find { it.session === newSession }  // Reference equality
            ?.let { pane -> splitState.closePane(pane.id) }
    }
)

Occurrences: This pattern appears 3 times and all need fixing.


2. Memory Leak Verification Needed (HIGH PRIORITY)

Location: SplitViewState.kt:142

When a pane is closed manually, session.dispose() is called. Need to verify TerminalTab.dispose() properly cleans up PTY, coroutines, listeners, and buffers. If not, this is a critical memory leak.


3. Unsafe Type Casting (MEDIUM PRIORITY)

Location: Multiple (e.g., TabbedTerminal.kt:133)

Add validation:

val newSession = tabController.createSessionForSplit(...)
require(newSession is TerminalTab) { "Session must be TerminalTab" }

⚠️ High-Priority Concerns

4. Unused Helper Function

Location: TabbedTerminal.kt:81-88

The createSessionForSplit(splitState, paneId) helper is never called. Remove it.


5. Divider Drag Performance

Location: SplitDivider.kt:109-120

Every mouse move triggers recomposition. Consider debouncing to 60fps max.


6. Focus Management

Location: SplitViewState.kt:117-120

Focus switches immediately on split. If user is typing, input might briefly go to wrong pane. Document expected behavior.


🎨 Code Quality

7. Magic Numbers

Location: SplitDivider.kt:28, 33

Document why DIVIDER_THICKNESS=1dp and DRAG_AREA_SIZE=16dp.


8. Error Handling

Location: SplitViewState.kt:269

Use proper logger instead of println.


9. Navigation Edge Cases

Location: SplitViewState.kt:176-182

Add comment that no-op on boundary navigation is intentional.


🧪 Testing Recommendations

Critical Test Cases

  1. Split → type exit immediately → verify pane closes (tests race condition)
  2. Open 20 splits → close all → check memory
  3. Close last pane → should close tab
  4. Resize to 9% → should clamp to 10%

🔒 Security

Resource Exhaustion

No limit on split count. User could create 100+ panes and exhaust memory. Consider max limit (e.g., 20 panes).


📝 Documentation

Missing CLAUDE.md update per project conventions.


🎯 Recommended Actions

Must Fix Before Merge 🔴

  1. Fix session ID closure capture bug (issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1)
  2. Verify TerminalTab.dispose() (issue 🔍 Implement text search/find functionality (Ctrl+F) #2)
  3. Remove unused helper (issue ⚙️ Implement user settings and configuration system #4)
  4. Add type validation (issue 🔗 Implement hyperlink detection and click handling #3)

Should Fix Before Merge 🟡

  1. Update CLAUDE.md
  2. Add drag debouncing (issue 🌏 Implement IME (Input Method Editor) support for CJK languages #5)

Consider for Future 🟢

  1. Keyboard pane resizing (accessibility)
  2. Resource limits
  3. Automated tests

🎉 Overall Assessment

Rating: 8.5/10 - Excellent architecture, nearly production-ready

Strengths: Clean abstraction, functional tree ops, comprehensive shortcuts, spatial navigation

Main Concerns: Lifecycle management (session disposal, closure captures)

With critical fixes, this is a stellar feature that matches iTerm2 quality! 🚀

- Add onMoveToNewTab callback throughout split component hierarchy
- Add extractFocusedPaneSession() to SplitViewState for non-destructive pane extraction
- Add createTabFromExistingSession() to TabController for session reuse
- Wire up context menu option in TabbedTerminal (only shown when splits exist)

The feature allows moving a split pane to a new tab while preserving
the running shell session and terminal history.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Support

This is a substantial feature addition with excellent architectural design.

✅ Strengths

1. Excellent Architecture

  • Tree-based data structure using sealed classes is clean and type-safe
  • Immutable tree operations follow functional best practices
  • Clear separation of concerns between state, rendering, and structure

2. Comprehensive Keyboard Shortcuts

  • All shortcuts properly registered with platform-aware modifiers
  • Cmd+D / Cmd+Shift+H for splitting
  • Cmd+Option+Arrows for directional navigation
  • Cmd+]/[ for cycling panes

3. Spatial Navigation

  • Well-implemented with proper overlap detection
  • Distance-based sorting for choosing closest pane
  • Edge-case handling with isInDirection checks

4. Resource Management

  • Proper disposal of sessions when panes close
  • Cleanup of split states when tabs close
  • Session extraction for Move to New Tab avoids double-disposal

5. Visual Feedback

  • Good UX with borders, draggable dividers, and cursor icons
  • 2dp blue border on focused, 1dp gray on unfocused

⚠️ Issues & Concerns

1. Duplicate Split Logic (Medium Priority)

Lines 126-169 and 227-269 in TabbedTerminal.kt contain nearly identical code for split handlers. Recommendation: Extract to shared lambda that can be reused.

2. Potential Memory Leak in Session ID Closure (High Priority)

Lines 133, 148, 237, 252 in TabbedTerminal.kt use pattern: var sessionId: String? = null with mutable capture in onProcessExit lambda. If splits are created rapidly, the variable might be overwritten before closure captures it. Recommendation: Extract session ID synchronously before creating exit handler.

3. Missing Null Safety (Medium Priority)

Lines 138-142, 255-259: The onProcessExit logic silently fails if pane is already removed. Recommendation: Add defensive logging for debugging.

4. SplitDragHandle Recomposition (Low Priority)

Lines 83-84 in SplitDivider.kt: startRatio and cumulativeDelta trigger recomposition on every drag event. Use plain var inside remember to avoid snapshot system overhead.

5. Unsafe Cast (Low Priority)

TabController.createSessionForSplit returns TerminalSession but callers cast to TerminalTab to get ID. Return TerminalTab directly.

🔍 Security & Safety

No Critical Issues Found

  • ✅ No XSS/injection vulnerabilities
  • ✅ Input validation present (ratio clamping 0.1-0.9)
  • ✅ No hardcoded credentials
  • ✅ Proper resource disposal

🧪 Test Coverage

Missing tests for:

  • Unit tests for SplitNode tree operations (removePane, replaceNode, updateRatio)
  • Edge cases: closing last pane, rapid split/close sequences
  • Spatial navigation edge cases

🚀 Performance

Generally Good

  • ✅ Tree operations efficient (O(n))
  • ✅ Bounds tracking appropriate
  • ✅ Ratio updates localized

Minor Concern: Deep split trees (10+ levels) may need depth limit warning.

📝 Documentation

Good:

  • ✅ KDoc comments on all public APIs
  • ✅ Clear parameter descriptions

Missing:

  • ❌ No inline comments explaining sessionId holder pattern
  • ❌ No documentation on Move to New Tab feature

🎯 Recommendations Summary

Priority Issue Action
HIGH Mutable sessionId closure Refactor to immutable capture
MEDIUM Code duplication Extract to shared lambda
MEDIUM Missing null safety Add defensive checks
LOW SplitDragHandle recomposition Use plain var
LOW Unsafe cast Change return type
LOW Missing unit tests Add tree operation tests

✅ Overall Assessment

Status: Approve with Minor Changes

Well-architected feature with excellent design patterns. The identified issues are mostly minor and can be addressed in follow-up commits. Core functionality is sound.

Recommended merge strategy:

  1. Fix HIGH priority mutable closure issue
  2. Address MEDIUM priority items (DRY violations, null safety)
  3. Merge to dev branch
  4. Address LOW priority items in follow-up PRs

Great work! The split pane architecture follows best practices from iTerm2/tmux, and the implementation is production-ready with minor polish.


Review generated by Claude Code
Reviewed commit: a85e2f4

- Add Cmd+Shift+M (macOS) / Ctrl+Shift+M (Windows/Linux) shortcut
- Only registered when splits exist (same as context menu behavior)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Implementation

This is an excellent addition that brings iTerm2-style split panes to BossTerm! The implementation is well-structured with a clean tree-based architecture. Here's my detailed feedback:


Strengths

  1. Solid Architecture

    • Clean sealed class hierarchy for SplitNode (Pane, VerticalSplit, HorizontalSplit)
    • Immutable tree operations with functional transformations
    • Proper separation of concerns (state management, UI rendering, tree operations)
  2. Comprehensive Feature Set

    • Directional navigation with spatial awareness (Cmd+Option+arrows)
    • Cycle navigation (Cmd+] / Cmd+[)
    • Draggable dividers with 16dp hit area for better UX
    • Focus indication with blue borders
    • Working directory inheritance from parent pane
  3. Good Resource Management

    • Proper disposal of sessions on pane close
    • Extract pattern for moving panes to new tabs (doesn't dispose session)
    • Cleanup of split states when tabs close
    • DisposableEffect for tab lifecycle
  4. UI Polish

    • Thin 1dp visible divider with larger invisible drag area
    • Cumulative drag tracking avoids recomposition issues
    • 10%-90% ratio constraints prevent too-small panes
    • Context menu integration for split operations

🔍 Issues & Concerns

Critical

  1. Memory Leak Risk in TabbedTerminal.kt:122-168

    The onProcessExit lambda captures splitState, creating a circular reference:

    • Session holds reference to onProcessExit closure
    • Closure captures splitState
    • splitState holds reference to session via rootNode

    Impact: Sessions may not be GC'd even after pane closes, causing memory leaks in long-running sessions with many splits.

    Solution: Use WeakReference to break the cycle and prevent memory leaks.

  2. Duplicate Code in TabbedTerminal.kt

    • Lines 122-168 (menu actions LaunchedEffect)
    • Lines 224-248 (split operation handlers)

    Both create sessions with nearly identical onProcessExit logic. This violates DRY and makes maintenance harder.

    Solution: Extract to helper function to reduce duplication and centralize the logic.

High Priority

  1. Unchecked Type Cast in Multiple Locations

    sessionId = (newSession as? TerminalTab)?.id

    If createSessionForSplit returns a TerminalSession that isn't a TerminalTab, sessionId becomes null, and the onProcessExit cleanup won't work.

    Solution: Either make createSessionForSplit return TerminalTab directly (type safety) or add assertion.

  2. Spatial Navigation Edge Cases in SplitViewState.kt:266-272

    Uses <= and >= for direction checks, allowing adjacent panes with no gap to be considered "in direction". This works for split layouts but might behave unexpectedly if dividers have thickness or there's visual spacing.

    Recommendation: Consider < / > for stricter adjacency, or document the assumption that dividers are zero-width for navigation purposes.

  3. Missing Null Safety in SplitViewState.kt:142

    If session.dispose() throws an exception, the pane won't be removed from the tree and paneBounds won't be cleaned up, leaving inconsistent state.

    Solution: Add try-catch like in dispose() method to ensure cleanup happens even on errors.

  4. SplitContainer.kt Focus Click Handler

    Uses pointerInput(Unit), which means the handler will not update if splitState or pane.id changes. This could lead to stale closures in complex split scenarios.

    Solution: Use pointerInput(pane.id) to recreate handler when pane changes.

Medium Priority

  1. Performance: Bounds Tracking in SplitViewState

    • paneBounds is a mutableStateMapOf, triggering recomposition on every update
    • Bounds are updated on every layout pass via onGloballyPositioned

    Suggestion: Consider using a regular MutableMap instead of mutableStateMapOf since bounds are only read during navigation.

  2. Drag Performance in SplitDivider.kt

    • Uses remember { mutableFloatStateOf(...) } for drag tracking
    • These trigger recomposition on every drag pixel

    Solution: Use regular var with remember instead of state objects to avoid unnecessary recompositions.

  3. Missing Error Handling in createSessionForSplit

    • PTY creation can fail but no way for caller to detect or handle this

    Recommendation: Either return a Result<TerminalSession> or throw an exception that can be caught by the UI layer.

Low Priority

  1. Magic Numbers - The 0.1/0.9 ratio bounds are hardcoded in multiple places. Extract to constants.

  2. Inconsistent Documentation - Add KDoc to all composables for consistency.

  3. Test Coverage - No unit tests for tree operations or spatial navigation logic.


🎨 Code Quality Notes

  1. Good Use of Sealed Classes: The SplitNode hierarchy is a textbook example.
  2. Immutability: Tree operations return new nodes, excellent for Compose.
  3. Clear Naming: Self-documenting function names.
  4. Proper Kotlin Idioms: Good use of when, extension functions, and data classes.

🏁 Recommendations

Before Merge:

  1. Fix the memory leak with WeakReference (critical)
  2. Extract duplicate session creation logic (critical)
  3. Add try-catch around session.dispose() (high)
  4. Fix pointerInput(Unit) to use pointerInput(pane.id) (high)

Nice to Have:
5. Add basic unit tests for tree operations
6. Extract magic numbers to constants
7. Consider non-state variables for drag tracking


Overall Assessment

This is a high-quality implementation with a solid foundation. The architecture is clean, the feature set is comprehensive, and the code is well-organized. The main concerns are around memory management and some minor performance optimizations.

Rating: 8/10 (would be 9.5/10 after addressing the critical issues)

Great work! This brings BossTerm much closer to feature parity with iTerm2 and tmux. 🚀

New settings in ~/.bossterm/settings.json:
- splitDefaultRatio: Initial ratio for new splits (default 0.5)
- splitMinimumSize: Minimum pane size when resizing (default 0.1 = 10%)
- splitFocusBorderEnabled: Show border on focused pane (default true)
- splitFocusBorderColor: Color of focus indicator (default #4A90E2)
- splitInheritWorkingDirectory: New splits inherit CWD (default true)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Support

This is an impressive feature implementation that adds iTerm2-style split panes to BossTerm! The code is well-architected with a clean tree-based approach. Here's my detailed feedback:


Strengths

  1. Excellent Architecture: The sealed class hierarchy (SplitNode) is a textbook example of modeling a binary tree in Kotlin. Clean separation between Pane, VerticalSplit, and HorizontalSplit.

  2. Immutable Tree Operations: Tree manipulation functions like replaceNode, removePane, and updateRatio follow functional programming principles with immutable updates. This prevents subtle state bugs.

  3. Spatial Navigation: The bounds-based directional navigation (isInDirection, hasHorizontalOverlap, etc.) is sophisticated and properly handles overlap detection for intuitive pane switching.

  4. Resource Management: Proper session lifecycle management with extractFocusedPaneSession() for move-to-tab and dispose() for cleanup. The distinction between "close and dispose" vs "extract and reuse" is important.

  5. Good Keyboard Shortcuts: Platform-aware shortcuts (Cmd on macOS, Ctrl elsewhere) with intuitive bindings matching iTerm2 conventions.


🐛 Potential Bugs

1. Session ID Capture Race Condition (TabbedTerminal.kt:122-158)

var sessionId: String? = null
val newSession = tabController.createSessionForSplit(
    workingDir = workingDir,
    onProcessExit = {
        sessionId?.let { id ->  // ⚠️ sessionId may still be null here
            splitState.getAllPanes()
                .find { (it.session as? TerminalTab)?.id == id }
                ?.let { pane -> splitState.closePane(pane.id) }
        }
    }
)
sessionId = (newSession as? TerminalTab)?.id

Problem: If the process exits immediately before sessionId is assigned (line 152), the callback will find sessionId == null and won't close the pane, leaving a zombie pane in the UI.

Fix: Capture the session directly instead of relying on ID:

val newSession = tabController.createSessionForSplit(
    workingDir = workingDir,
    onProcessExit = {
        splitState.getAllPanes()
            .find { it.session === newSession }  // Direct reference comparison
            ?.let { pane -> splitState.closePane(pane.id) }
    }
)

This pattern repeats in 3 places in TabbedTerminal.kt (lines 122-158, 206-242, and 249-266).


2. Missing Validation in SplitNode Init Blocks (SplitNode.kt:35, 49)

init {
    require(ratio in 0.1f..0.9f) { "Split ratio must be between 0.1 and 0.9" }
}

Problem: This hardcodes the min/max ratio (0.1-0.9) but the settings allow user-configurable splitMinimumSize (default 0.1). If a user changes the setting, the hardcoded validation won't respect it, causing potential crashes or unexpected clamps.

Fix: Make ratio validation use the configurable setting or remove the hardcoded validation since clamping happens in updateRatio() anyway.


3. Redundant LaunchedEffect Re-triggering (TabbedTerminal.kt:122)

LaunchedEffect(menuActions, tabController.activeTabIndex, tabController.tabs.size) {
    // ... sets onSplitVertical, onSplitHorizontal, onClosePane ...
}

Problem: This re-runs whenever tabs.size changes (e.g., every tab creation/closure), causing menu action lambdas to be recreated unnecessarily. The lambdas capture activeTab which already changes when activeTabIndex changes.

Fix: Remove tabs.size dependency:

LaunchedEffect(menuActions, tabController.activeTabIndex) {
    // ... (tabs.size is redundant - activeTabIndex already triggers update)
}

⚠️ Performance Concerns

1. N² Complexity in Pane Lookup (TabbedTerminal.kt:134-137, 160-163)

onProcessExit = {
    sessionId?.let { id ->
        splitState.getAllPanes()  // O(N) tree traversal
            .find { (it.session as? TerminalTab)?.id == id }  // O(N) linear search
            ?.let { pane -> splitState.closePane(pane.id) }  // O(N) tree traversal again
    }
}

Impact: For a user with 10 splits, every process exit does ~30 tree traversals. This is acceptable for <20 panes but could lag with heavy split usage.

Mitigation: The session reference fix (bug #1) also solves this - direct reference comparison is O(N) instead of O(N²).


2. Potential Memory Leak in paneBounds (SplitViewState.kt:44)

val paneBounds = mutableStateMapOf<String, Rect>()

The paneBounds map is cleaned up in closePane() but might accumulate stale entries if panes are removed through tree manipulation without going through closePane().

Fix: Add a cleanup pass in dispose():

fun dispose() {
    getAllSessions().forEach { ... }
    paneBounds.clear()  // ✅ Already present!
}

Actually, this is already handled correctly. Good!


🔒 Security Concerns

1. Unbounded Split Recursion

There's no maximum split depth limit. A malicious script or user could recursively split until OOM.

Fix: Add a hard limit (e.g., 50 panes max) in SplitViewState.splitFocusedPane():

fun splitFocusedPane(...): String? {
    if (getAllPanes().size >= MAX_PANES) {
        println("Maximum split limit reached")
        return null
    }
    // ... existing logic
}

📝 Code Quality Suggestions

1. Magic Numbers in SplitDivider.kt

The divider code likely uses hardcoded values for thickness and drag area.

Suggestion: Extract to TerminalSettings to allow customization.


2. Inconsistent Error Handling

SplitViewState.dispose() silently catches exceptions:

try {
    session.dispose()
} catch (e: Exception) {
    println("Error disposing session: ${e.message}")  // ⚠️ Lost exception
}

Better: Use structured logging or at least print stack trace for debugging.


3. Type Safety in Menu Action Wiring

The menu actions use nullable lambdas but are conditionally invoked safely with ?.invoke(). This is actually fine! Good defensive programming.


Feature Completeness

1. Missing: Split Ratio Persistence

When a user resizes splits, the ratios aren't saved. On app restart, all splits reset to 50/50.

Enhancement: Consider serializing SplitNode tree to JSON in future iteration.


2. Missing: Zoom Focused Pane

iTerm2 has Cmd+Shift+Enter to temporarily "zoom" the focused pane to fullscreen.

Suggestion: Add a zoomedPaneId: String? state for future enhancement.


🧪 Test Coverage Recommendations

Missing test cases for:

  1. Edge Case: Split, then immediately close parent tab before process starts
  2. Edge Case: Dragging divider to 0% or 100% (should clamp to min/max)
  3. Regression: Rapidly switching focus while splits are created/destroyed
  4. Performance: Rendering 10+ splits with streaming output

📋 Summary

Category Count
Critical Bugs 1 (race condition)
Medium Issues 2 (validation, unbounded splits)
Minor Issues 2 (error handling, magic numbers)
Enhancements 2 (persistence, zoom)

Recommendation:

  • Approve with minor changes - The core architecture is excellent
  • 🔧 Fix the session ID race condition before merge (critical)
  • 📝 Consider adding max pane limit for safety

🎉 Overall Assessment

This is high-quality work! The tree-based split architecture is elegant and extensible. The spatial navigation algorithm is particularly impressive. Great job following Kotlin idioms (sealed classes, data classes, immutability).

The main concern is the session lifecycle race condition - please address that before merging. Everything else is polish for future iterations.


Testing Notes:

  1. Verify the race condition: Test rapid exit in new panes
  2. Test with 10+ splits and streaming output
  3. Verify working directory inheritance with nested splits

🤖 Generated with Claude Code

- Add SPLITS category to SettingsCategory enum
- Create SplitsSettingsSection.kt with:
  - Default Split Ratio slider (30%-70%)
  - Minimum Pane Size slider (5%-40%)
  - Inherit Working Directory toggle
  - Show Focus Border toggle
  - Focus Border Color picker (enabled when border is on)
- Wire up SPLITS case in SettingsPanel

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Support

This is an excellent implementation of iTerm2-style split panes! The architecture is well-designed with clean separation of concerns.

Strengths

Architecture & Design

  • Excellent tree-based split architecture with sealed classes
  • Clean state management in SplitViewState
  • Immutable tree operations (replaceNode, removePane, updateRatio)
  • Proper lifecycle management with DisposableEffect

Features

  • Comprehensive iTerm2-style keyboard shortcuts
  • Sophisticated spatial navigation with overlap detection
  • Draggable dividers (1dp visible + 16dp drag area)
  • Good settings integration
  • Clever Move to New Tab implementation

Potential Issues

1. Race Condition in Session Lifecycle (High Priority)

Location: TabbedTerminal.kt lines 227-257, 130-164

The sessionId capture pattern has a race condition. If the process exits immediately after creation, sessionId is still null when onProcessExit fires.

Fix: Capture the pane ID directly and update SplitViewState.splitFocusedPane() to accept optional newPaneId parameter.

2. Duplicate Session Cleanup (Medium Priority)

Location: SplitViewState.kt lines 141-169

When a process exits, onProcessExit calls closePane() which calls session.dispose(), but the session might already be disposed.

Fix: Add try-catch guard in closePane() for IllegalStateException.

3. Inconsistent Working Directory Inheritance (Medium Priority)

Location: TabbedTerminal.kt lines 127-164 vs lines 227-257

Menu-triggered splits always inherit working directory, but keyboard shortcuts respect settings.splitInheritWorkingDirectory.

Fix: Apply the setting consistently in menu actions.

4. Missing Null Safety in Navigation (Low Priority)

Location: SplitViewState.kt lines 233-252

If focusedPaneId becomes invalid, indexOfFirst returns -1 which is not handled.

Fix: Add fallback for currentIndex == -1.

Code Quality

Good Practices:

  • Proper use of remember and mutableStateMapOf
  • Exhaustive when expressions
  • Immutable data classes
  • Bounds clamping (0.1-0.9)

Minor Improvements:

  • Extract magic numbers for overlap/distance thresholds
  • Consider caching Map of sessionId to paneId to avoid O(n) lookups
  • Multiple as? TerminalTab casts could fail silently

Test Coverage Suggestions

  1. Race condition test: Simulate immediate process exit
  2. Tree manipulation tests with complex nested splits
  3. Navigation edge cases: L-shaped splits, no overlap scenarios
  4. Memory leak test: Verify disposal after repeated split/close cycles

Performance

  1. Bounds tracking overhead: onGloballyPositioned fires frequently
  2. Tree traversal: O(n) lookups are fine for typical usage (<20 panes)
  3. Recomposition efficiency: Using Modifier.weight() is efficient

Final Verdict

Recommend: Merge after addressing race condition (issue 1)

This is production-quality code with only one high-priority fix needed. The architecture is solid and follows best practices.

Priority fixes before merge:

  1. Fix session lifecycle race condition (High)
  2. Make working directory inheritance consistent (Medium)

Post-merge improvements:
3. Add disposal guard
4. Add navigation fallback
5. Performance profiling with many splits

Great work! This maintains the codebase high quality standards.


Review generated by Claude Code following BossTerm development guidelines

- Add materialIconsExtended dependency for better icon selection
- Change Splits icon from Add to Splitscreen (shows split screen layout)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Support

This is an impressive and well-architected feature! The split pane implementation follows solid design principles and integrates cleanly with the existing codebase. Here's my detailed review:


🎯 Overall Assessment

Grade: A- (Excellent work with minor improvement opportunities)

The implementation demonstrates:

  • ✅ Clean separation of concerns (tree structure, state management, rendering)
  • ✅ Proper resource lifecycle management
  • ✅ Robust spatial navigation logic
  • ✅ Comprehensive keyboard shortcuts following iTerm2 patterns
  • ✅ Good defensive programming (null checks, bounds validation)

🌟 Strengths

1. Architecture & Design

  • Binary tree structure (SplitNode sealed class): Clean, functional approach that makes tree transformations safe and predictable
  • Immutability where it matters: Tree nodes use data class with copy(), preventing accidental mutations
  • Separation of concerns: State management (SplitViewState), rendering (SplitContainer), and tree operations (SplitNode) are properly decoupled

2. Resource Management

  • Proper disposal: SplitViewState.dispose() correctly iterates through all sessions and disposes them (lines 310-319 in SplitViewState.kt)
  • Cleanup on tab close: TabbedTerminal.kt cleanup logic (lines 177-181) properly disposes split states
  • Extract vs Close: Smart distinction between extractFocusedPaneSession() (no disposal for tab migration) and closePane() (disposal for pane removal)

3. Navigation Logic

  • Spatial navigation: The bounds-based directional navigation (findPanesInDirection, isInDirection) is mathematically sound
  • Overlap detection: hasHorizontalOverlap and hasVerticalOverlap correctly handle edge cases
  • Distance sorting: Closest panes are prioritized (line 266 in SplitViewState.kt)

4. Session Lifecycle

  • Process exit handling: The sessionId holder pattern (TabbedTerminal.kt lines 131-143) correctly handles async process exits
  • Auto-close on exit: Panes automatically close when shell exits, with proper lookup by session ID

5. Settings Integration

  • Configurable behavior: Split ratio, minimum size, border colors, and working directory inheritance are all configurable
  • New settings section: Clean UI for split-specific settings (SplitsSettingsSection.kt)

🔧 Areas for Improvement

1. Session ID Lifecycle Race Condition ⚠️

Location: TabbedTerminal.kt lines 131-143, 147-159, 230-244

Issue: There's a subtle race condition in the onProcessExit closure:

var sessionId: String? = null
val newSession = tabController.createSessionForSplit(
    onProcessExit = {
        sessionId?.let { id ->  // sessionId might still be null if process exits quickly
            // ... lookup and close pane
        }
    }
)
sessionId = (newSession as? TerminalTab)?.id  // Set AFTER creation

Problem: If the shell process exits immediately (e.g., exit in .zshrc), the onProcessExit callback may fire before sessionId is assigned, causing the pane to not auto-close.

Recommended Fix:

val newSession = tabController.createSessionForSplit(
    onProcessExit = {
        // Capture session in closure directly
        val paneToClose = splitState.getAllPanes()
            .find { it.session === newSession }  // Use identity comparison
        paneToClose?.let { splitState.closePane(it.id) }
    }
)

Instances to fix: Lines 131-143, 147-159, 230-244 in TabbedTerminal.kt


2. Redundant Helper Function 🧹

Location: TabbedTerminal.kt lines 91-101

The createSessionForSplit helper function (lines 91-101) is never used and duplicates logic from lines 131-159. Consider removing it to reduce code duplication.


3. Menu Actions LaunchedEffect Efficiency 🔄

Location: TabbedTerminal.kt lines 122-169

Issue: The LaunchedEffect re-runs whenever tabController.tabs.size changes, even for unrelated tab operations (e.g., tab 3 closing when viewing tab 1).

Current dependencies:

LaunchedEffect(menuActions, tabController.activeTabIndex, tabController.tabs.size)

Recommendation: Remove tabs.size dependency since activeTabIndex already triggers updates when switching tabs:

LaunchedEffect(menuActions, tabController.activeTabIndex)

This reduces unnecessary recompositions when background tabs are added/removed.


4. Missing Error Handling in Tree Operations 🛡️

Location: SplitNode.kt lines 100-113, 119-158

Issue: replaceNode and removePane silently return the original tree if the target isn't found. No logging or error indication.

Recommendation: Add debug logging to help troubleshoot issues:

fun SplitNode.replaceNode(targetId: String, transform: (SplitNode) -> SplitNode): SplitNode {
    if (this.id == targetId) return transform(this)
    
    val result = when (this) { /* ... existing logic ... */ }
    
    if (result === this && this.findNode(targetId) == null) {
        println("WARN: replaceNode called with unknown targetId: $targetId")
    }
    return result
}

5. Keyboard Shortcut Conflicts ⚠️

Location: BuiltinActions.kt lines 434-447

Issue: Cmd+D (macOS) / Ctrl+D (Linux) is a standard shell EOF signal. Using it for vertical split may confuse users and could conflict with terminal applications.

Recommendation:

  • Consider Cmd+Shift+D instead (follows VSCode pattern)
  • Document the conflict in CLAUDE.md and settings UI
  • Add a user warning if they try to change the default binding

6. Spatial Navigation Edge Cases 🧮

Location: SplitViewState.kt lines 272-279

Potential Issue: The current isInDirection logic works well for rectangular splits, but may behave unexpectedly with complex nested splits (e.g., 3-way splits where overlap detection is ambiguous).

Example:

┌─────┬─────┐
│  A  │  B  │
├─────┴──┬──┤
│   C    │ D│
└────────┴──┘

Navigating RIGHT from pane A: Does it go to B (directly right) or C (below but also has right component)?

Recommendation: Add unit tests for complex split layouts and document the expected navigation behavior.


7. Minor: Unused sessionFactory Parameter 🧹

Location: SplitViewState.kt line 22

The sessionFactory: (() -> TerminalSession)? parameter is declared but never used. Consider removing it to reduce API surface.


🧪 Testing Recommendations

  1. Race Condition Test:

  2. Memory Leak Test:

    • Create 20 splits, close them all, run GC
    • Monitor for leaked PTY processes or terminal buffers
  3. Navigation Stress Test:

    • Create 6+ splits in complex patterns
    • Test all navigation directions (UP/DOWN/LEFT/RIGHT)
    • Verify Cmd+] and Cmd+[ cycle correctly
  4. Settings Persistence:

    • Change split ratio, border color, inherit CWD settings
    • Restart app and verify settings are restored
  5. Edge Cases:

    • Try to close the last pane (should close tab)
    • Move last pane to new tab (should keep original tab open)
    • Resize splits to minimum bounds (10%)

🔒 Security Considerations

No security issues identified:

  • No command injection risks
  • No XSS vulnerabilities (Compose native rendering)
  • Proper disposal prevents resource leaks
  • Settings validation (ratio clamped to 0.1-0.9)

📊 Code Quality Metrics

Metric Score Notes
Architecture 9/10 Clean tree-based design
Resource Management 8/10 Good disposal, minor race condition
Error Handling 7/10 Missing debug logging
Documentation 9/10 Excellent KDoc comments
Test Coverage 5/10 No unit tests included (expected for PR)

Recommendation

APPROVE with minor changes

The code is production-ready with the session lifecycle race condition fix (#1). Other improvements are optional enhancements that can be addressed in follow-up PRs.

Required Before Merge:

Optional Follow-ups:


Great work on this feature! The architecture is solid and the implementation is thorough. 🚀

kshivang and others added 2 commits December 7, 2025 18:48
Bug fixes:
- Remove hardcoded ratio validation (0.1-0.9) from SplitNode init blocks
- Parameterize updateRatio() with minRatio to respect user settings
- splitMinimumSize setting now properly enforced during resize

Code cleanup:
- Use direct reference comparison in session lookup callbacks
- Remove redundant tabs.size from LaunchedEffect dependencies

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@kshivang kshivang merged commit 7477f5e into master Dec 8, 2025
7 checks passed
@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Support

This is a well-structured implementation of iTerm2-style split views! The architecture is clean, the code quality is high, and the feature set is comprehensive. Here's my detailed review:


Strengths

Architecture & Design

  • Excellent tree-based structure: The sealed class hierarchy (SplitNode) is elegant and type-safe
  • Clean separation of concerns: SplitNode (data), SplitViewState (logic), SplitContainer (UI) are well-separated
  • Immutable tree operations: Using functional tree transformations (replaceNode, removePane) prevents state bugs
  • Spatial navigation: The bounds-based directional navigation is sophisticated and intuitive

Code Quality

  • Clear naming: Functions like findPanesInDirection, hasHorizontalOverlap are self-documenting
  • Comprehensive utilities: 15+ extension functions on SplitNode cover all operations
  • Proper resource cleanup: dispose() methods handle session lifecycle correctly
  • Settings integration: All behaviors are configurable through settings panel

User Experience

  • Keyboard shortcuts: Complete set of shortcuts (split, navigate, close) with platform-aware modifiers
  • Visual feedback: Focus borders, draggable dividers with invisible hit areas (smart UX!)
  • Context menus: Split options integrated into right-click menus
  • Working directory inheritance: New splits inherit CWD when enabled

⚠️ Issues & Concerns

1. Session Lifecycle Bug (High Priority)

Location: TabbedTerminal.kt lines 120-168

The onProcessExit callback has a race condition:

var newSessionRef: TerminalSession? = null
val newSession = tabController.createSessionForSplit(
    workingDir = workingDir,
    onProcessExit = {
        newSessionRef?.let { session ->  // ❌ May be null if shell exits immediately
            splitState.getAllPanes()
                .find { it.session === session }
                ?.let { pane -> splitState.closePane(pane.id) }
        }
    }
)
newSessionRef = newSession  // Race: session might exit before this executes

Problem: If the shell exits immediately (e.g., exit command in .zshrc), the callback fires before newSessionRef is assigned, leaving a zombie pane.

Solution: Capture the pane ID instead:

val newPaneId = splitState.splitFocusedPane(orientation, newSession, ratio)
if (newPaneId != null) {
    // Register cleanup for this specific pane ID
    newSession.onProcessExit = { splitState.closePane(newPaneId) }
}

Impact: This same pattern appears 5+ times in TabbedTerminal.kt (lines 122-140, 143-161, 213-242, 244-273).


2. Unconstrained Ratio Updates (Medium Priority)

Location: SplitNode.kt lines 156-181

fun SplitNode.updateRatio(targetId: String, newRatio: Float, minRatio: Float = 0.1f): SplitNode {
    val clampedRatio = newRatio.coerceIn(minRatio, 1f - minRatio)  // ✅ Good
    // ... but no validation that minRatio itself is valid
}

Problem: If splitMinimumSize setting is set to an invalid value (e.g., 0.6), the constraint 1f - minRatio becomes less than minRatio, causing stuck dividers.

Solution:

fun SplitNode.updateRatio(targetId: String, newRatio: Float, minRatio: Float = 0.1f): SplitNode {
    require(minRatio in 0.0f..0.5f) { "minRatio must be between 0.0 and 0.5" }
    val clampedRatio = newRatio.coerceIn(minRatio, 1f - minRatio)
    // ...
}

Also add validation in TerminalSettings.kt:

val splitMinimumSize: Float = 0.1f,
init {
    require(splitMinimumSize in 0.0f..0.5f) { "splitMinimumSize must be 0.0-0.5" }
}

3. Resource Leak in SplitViewState (Medium Priority)

Location: SplitViewState.kt line 148

fun closePane(paneId: String): Boolean {
    // ...
    val paneToClose = rootNode.findPane(paneId) ?: return false
    paneToClose.session.dispose()  // ✅ Session disposed
    paneBounds.remove(paneId)       // ✅ Bounds cleaned up
    // ❌ But what if removePane() fails? Session already disposed!
    val newRoot = rootNode.removePane(paneId)
    if (newRoot != null) {
        rootNode = newRoot
        return true
    }
    return false  // Session disposed but tree unchanged - inconsistent state
}

Solution: Dispose session AFTER successful tree update:

fun closePane(paneId: String): Boolean {
    if (isSinglePane) return false
    val paneToClose = rootNode.findPane(paneId) ?: return false
    
    val newRoot = rootNode.removePane(paneId) ?: return false
    
    // Only dispose after successful removal
    paneToClose.session.dispose()
    paneBounds.remove(paneId)
    rootNode = newRoot
    
    // Update focus...
    return true
}

4. Missing Null Safety in Navigation (Low Priority)

Location: SplitViewState.kt lines 233-253

fun navigateToNextPane() {
    val allPanes = getAllPanes()
    if (allPanes.size <= 1) return
    
    val currentIndex = allPanes.indexOfFirst { it.id == focusedPaneId }  // ❌ Returns -1 if not found
    val nextIndex = (currentIndex + 1) % allPanes.size  // With -1: nextIndex = 0 (lucky!)
    focusedPaneId = allPanes[nextIndex].id
}

Problem: If focusedPaneId is stale (shouldn't happen, but defensive coding), indexOfFirst returns -1, and math still works by accident.

Solution: Explicit check:

fun navigateToNextPane() {
    val allPanes = getAllPanes()
    if (allPanes.size <= 1) return
    
    val currentIndex = allPanes.indexOfFirst { it.id == focusedPaneId }
    if (currentIndex == -1) {
        // Focused pane not found - reset to first pane
        focusedPaneId = allPanes.first().id
        return
    }
    val nextIndex = (currentIndex + 1) % allPanes.size
    focusedPaneId = allPanes[nextIndex].id
}

5. Focus Handling Edge Case (Low Priority)

Location: SplitContainer.kt lines 261-269

.pointerInput(pane.id) {
    detectTapGestures(
        onPress = {
            splitState.setFocusedPane(pane.id)
            tryAwaitRelease()  // ❌ Blocks gesture handling
        }
    )
}

Problem: tryAwaitRelease() blocks until pointer is released, which may interfere with ProperTerminal's gesture handling (drag selection, etc.).

Solution: Use onTap instead:

.pointerInput(pane.id) {
    detectTapGestures(
        onTap = { splitState.setFocusedPane(pane.id) }
    )
}

6. Divider Hit Area Calculation (Low Priority)

Location: SplitContainer.kt lines 424, 544

.offset { IntOffset(leftWidth.toInt() - with(density) { 8.dp.toPx() }.toInt(), 0) }

Issue: 8.dp is hardcoded (should be DRAG_HANDLE_PADDING / 2 from SplitDivider.kt).

Solution: Extract constant:

private val DRAG_HANDLE_HALF_SIZE = 8.dp

.offset { IntOffset(leftWidth.toInt() - with(density) { DRAG_HANDLE_HALF_SIZE.toPx() }.toInt(), 0) }

🧪 Testing Recommendations

Critical Tests

  1. Immediate shell exit: Run bash -c exit in split pane - verify pane auto-closes
  2. Rapid split/close: Create 10 splits, close them all rapidly - check for memory leaks
  3. Divider edge cases: Drag divider to min/max bounds - ensure it stays within 10%-90%
  4. Focus persistence: Close middle pane of 3-way split - verify focus moves correctly

Stress Tests

  • Create 20+ nested splits, monitor memory usage
  • Navigate directionally in complex split layouts (5+ panes)
  • Resize window while splits exist - verify layout recalculation

📝 Minor Suggestions

Code Style

  1. SplitNode.kt:203 - Missing kdoc for findParentId()
  2. SplitViewState.kt:44 - paneBounds could be private with a public getter
  3. BuiltinActions.kt:447 - Comment says "No shortcut" but it's just a comment discrepancy

Performance

  • SplitViewState.findPanesInDirection(): Consider caching pane bounds in a spatial index for large split counts (though 20+ splits is unlikely)

Accessibility

  • Add ARIA labels or accessibility descriptions for split dividers (for screen readers)

🎯 Recommendations

Before Merge:

  1. ✅ Fix session lifecycle race condition (Issue feat: Terminal rendering improvements + 4 high-priority features (#2, #3, #4, #5) #1) - HIGH PRIORITY
  2. ✅ Add ratio validation (Issue 🔍 Implement text search/find functionality (Ctrl+F) #2)
  3. ✅ Fix dispose ordering (Issue 🔗 Implement hyperlink detection and click handling #3)

After Merge:

  • Add unit tests for SplitNode tree operations
  • Add integration tests for split lifecycle
  • Consider performance profiling with 10+ splits

📊 Summary

Overall Assessment: ⭐⭐⭐⭐ (4/5)

This is a production-ready feature with excellent architecture. The issues identified are mostly edge cases that are unlikely to occur in normal usage, but fixing them will make the code more robust.

Code Quality: Excellent
Architecture: Excellent
Testing: Needs improvement (no tests included)
Documentation: Good (inline comments are clear)

Verdict: Approve with requested changes - Fix Issue #1 (session lifecycle), then merge.

Great work! 🚀

@claude
Copy link
Copy Markdown

claude Bot commented Dec 8, 2025

Code Review: Split Pane Feature

This is a comprehensive and well-implemented feature that adds iTerm2-style split views to BossTerm. The implementation demonstrates strong architectural design and attention to detail.


Strengths

Architecture & Design

  • Clean tree-based architecture: The sealed class hierarchy (SplitNode) with Pane, VerticalSplit, and HorizontalSplit is elegant and composable
  • Immutable tree operations: Using functional tree transformations (replaceNode, removePane, updateRatio) prevents mutation bugs
  • Separation of concerns: Clear split between state (SplitViewState), UI (SplitContainer), and data models (SplitNode)
  • Composable rendering: Recursive rendering with RenderSplitNode follows Compose patterns perfectly

Feature Completeness

  • Comprehensive keyboard shortcuts (Cmd+D, Cmd+Shift+H, Cmd+Option+arrows, Cmd+[/])
  • Smart spatial navigation with overlap detection and distance calculation
  • Working directory inheritance from parent pane (configurable)
  • Pane borders with focus indication (configurable)
  • Drag-to-resize with minimum size constraints (10%-90%)
  • Context menu and Shell menu integration
  • Settings panel with all split-related options
  • Auto-close on process exit

Code Quality

  • Excellent documentation with clear KDoc comments
  • Consistent naming conventions throughout
  • Proper resource cleanup with dispose() methods
  • Settings properly serialized to JSON

⚠️ Potential Issues

1. Memory Leak Risk in Session Lifecycle (HIGH PRIORITY)

Location: TabbedTerminal.kt:88-121

The session creation closures capture newSessionRef in a way that could cause memory leaks:

var newSessionRef: TerminalSession? = null
val newSession = tabController.createSessionForSplit(
    workingDir = workingDir,
    onProcessExit = {
        newSessionRef?.let { session ->
            splitState.getAllPanes()
                .find { it.session === session }
                ?.let { pane -> splitState.closePane(pane.id) }
        }
    }
)
newSessionRef = newSession

Problem: The onProcessExit closure captures newSessionRef and splitState, creating a reference cycle that may prevent GC.

Recommendation:

  • Store pane ID instead of session reference
  • Use WeakReference if session reference is needed
  • Or simply pass paneId to createSessionForSplit callback
// Better approach
val newPaneId = UUID.randomUUID().toString()
val newSession = tabController.createSessionForSplit(
    workingDir = workingDir,
    onProcessExit = {
        splitState.closePane(newPaneId)
    }
)

2. Race Condition in Split State Cleanup (MEDIUM PRIORITY)

Location: TabbedTerminal.kt:178-181

LaunchedEffect(tabController.tabs.size) {
    val currentTabIds = tabController.tabs.map { it.id }.toSet()
    splitStates.keys.removeAll { it !in currentTabIds }
}

Problem: This cleanup happens asynchronously, so there's a window where:

  1. Tab is removed from tabController.tabs
  2. Cleanup hasn't run yet
  3. User switches to a different tab
  4. Stale splitState might be accessed

Recommendation: Move cleanup logic into TabController.closeTab() for immediate execution.

3. Missing Null Safety in Navigation (LOW PRIORITY)

Location: SplitViewState.kt:233-241

fun navigateToNextPane() {
    val allPanes = getAllPanes()
    if (allPanes.size <= 1) return
    
    val currentIndex = allPanes.indexOfFirst { it.id == focusedPaneId }
    val nextIndex = (currentIndex + 1) % allPanes.size
    focusedPaneId = allPanes[nextIndex].id
}

Problem: If focusedPaneId doesn't exist in allPanes, currentIndex will be -1, causing incorrect navigation.

Recommendation: Add guard clause:

if (currentIndex == -1) {
    focusedPaneId = allPanes.firstOrNull()?.id ?: return
    return
}

4. Settings Validation Missing (LOW PRIORITY)

Location: TerminalSettings.kt:366-382

val splitDefaultRatio: Float = 0.5f,
val splitMinimumSize: Float = 0.1f,

Problem: No validation for settings ranges. Invalid values in JSON could crash the app.

Recommendation: Add validation in settings initialization:

val splitDefaultRatio: Float = 0.5f,
    get() = field.coerceIn(0.3f, 0.7f)
val splitMinimumSize: Float = 0.1f,
    get() = field.coerceIn(0.05f, 0.5f)

🎯 Performance Considerations

Spatial Navigation Efficiency (POSITIVE)

The spatial navigation algorithm (SplitViewState.kt:259-306) is well-optimized:

  • Uses early filtering before distance calculations
  • Sorts only filtered candidates
  • O(n) time complexity where n = number of panes

Tree Traversal Performance (POSITIVE)

Immutable tree operations are efficient for typical split counts (<10 panes). For extreme cases (>20 panes), consider:

  • Caching getAllPanes() results
  • Maintaining a flat pane map alongside the tree

🔒 Security Concerns

No Critical Issues Found

  • No command injection risks (working directory is properly handled)
  • No XSS vulnerabilities (all text is rendered safely)
  • Resource disposal properly implemented

🧪 Test Coverage

Missing Test Scenarios

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

  1. Unit tests for SplitNode tree operations:

    • removePane with nested splits
    • updateRatio boundary conditions
    • findPane with deep trees
  2. Unit tests for SplitViewState:

    • Navigation edge cases (single pane, no overlap)
    • Focus tracking after pane removal
    • Session lifecycle management
  3. Integration tests:

    • Split → close → split sequence
    • Working directory inheritance toggle
    • Move pane to new tab

📋 Minor Suggestions

1. Extract Magic Numbers

Location: SplitDivider.kt (not fully shown in diff)

Consider extracting drag area size (16dp) and divider thickness (1dp) to constants:

private val DIVIDER_DRAG_AREA = 16.dp
private val DIVIDER_THICKNESS = 1.dp

2. Add Logging for Debugging

Add structured logging for key operations:

fun closePane(paneId: String): Boolean {
    if (isSinglePane) {
        logger.debug("Cannot close last pane")
        return false
    }
    logger.debug("Closing pane: $paneId")
    // ... rest of implementation
}

3. Document Settings Constraints

Add KDoc to clarify settings ranges:

/**
 * Default ratio for new splits.
 * Valid range: 0.3 to 0.7 (enforced in UI, not in data class)
 */
val splitDefaultRatio: Float = 0.5f,

🎉 Overall Assessment

This is excellent work. The implementation is:

  • ✅ Architecturally sound
  • ✅ Well-documented
  • ✅ Feature-complete
  • ✅ Following Compose best practices
  • ✅ Consistent with project conventions

The identified issues are minor and mostly edge cases. The memory leak risk should be addressed before merging, but otherwise this is production-ready code.

Recommendation: Approve with minor changes


📝 Action Items

Before Merge (Critical):

  1. Fix session lifecycle memory leak (TabbedTerminal.kt:88-121)
  2. Add null safety guard in navigateToNextPane()

Post-Merge (Nice to Have):

  1. Add unit tests for tree operations
  2. Move split state cleanup to TabController.closeTab()
  3. Add settings validation
  4. Extract magic numbers to constants

Great work on this feature! The split pane implementation is one of the most requested terminal features, and this delivers it with style. 🚀

kshivang added a commit that referenced this pull request Dec 12, 2025
feat: Add horizontal and vertical split pane support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant