feat: Add configurable Shift+Enter behavior (#196)

[kshivang]

Summary

• Add shiftEnterBehavior setting with two options:
  • "newline" (default): Send LF (0x0A) for multi-line input (iTerm2 style)
  • "same-as-enter": Send CR (0x0D) like regular Enter
• Add dropdown in Settings UI under Behavior → Keyboard
• Configurable via ~/.bossterm/settings.json

Changes

File	Change
TerminalKeyEncoder.kt	Add setShiftEnterSendsNewline() method
TerminalSettings.kt	Add shiftEnterBehavior setting
TerminalSettingsOverride.kt	Add override support
BehaviorSettingsSection.kt	Add UI dropdown
ProperTerminal.kt	Configure encoder from settings

Test Plan

• Run app and press Shift+Enter - should insert newline (not execute)
• Change setting to "Same as Enter" - Shift+Enter should execute like Enter
• Verify setting persists in ~/.bossterm/settings.json

Closes #196

🤖 Generated with Claude Code

[claude]

Code Review for PR #198: Add configurable Shift+Enter behavior

Summary

This PR adds a configurable Shift+Enter behavior setting that allows users to choose between sending LF (newline) for multi-line input or CR (same as Enter). The implementation is clean and follows existing patterns in the codebase.

---

Strengths

1. Consistent with existing patterns: The implementation mirrors the setAltSendsEscape() pattern already used in the codebase, making it maintainable and predictable.

2. Good defaults: Defaulting to newline (iTerm2 style) is a sensible choice that matches user expectations from other modern terminals.

3. Complete implementation: All necessary layers are covered - Core encoder logic, Settings persistence, Override support, UI controls, Runtime configuration

4. Clear documentation: Comments and descriptions effectively explain the behavior differences.

---

Issues & Recommendations

Critical: Missing Terminal Interface Method

The current implementation only configures the keyEncoder at the UI layer but does not follow the established pattern of going through the Terminal interface. This creates an architectural inconsistency.

Current pattern for altSendsEscape:

• Terminal.kt (interface) has: fun setAltSendsEscape(enabled: Boolean)
• BossTerminal.kt implements it by calling myTerminalKeyEncoder.setAltSendsEscape(enabled)

Recommended fix:

1. Add fun setShiftEnterSendsNewline(enabled: Boolean) to Terminal.kt interface (around line 185)
2. Implement in BossTerminal.kt (around line 1510) by calling myTerminalKeyEncoder.setShiftEnterSendsNewline(enabled)
3. Update ProperTerminal.kt to call terminal.setShiftEnterSendsNewline() instead of keyEncoder directly

Why this matters: The Terminal interface serves as the contract between the UI layer and the terminal emulation logic. All settings that affect terminal behavior should go through this interface for consistency and future extensibility.

---

Minor: String constant validation

The shiftEnterBehavior setting uses string values without validation or constants. Consider adding an enum for type safety, though the current implementation handles unexpected values gracefully with the else branch.

---

Testing Recommendations

Additional test cases to consider:

1. Settings persistence - verify the setting survives app restart
2. Override behavior - test TerminalSettingsOverride properly overrides the base setting
3. Edge case - test behavior in alternate buffer mode (vim, less, etc.)
4. Multiple tabs - verify each tab respects its own settings if overrides are used

---

Code Quality Assessment

• Security: No security concerns
• Performance: Negligible impact (one-time configuration)
• Maintainability: Good, but see architectural consistency issue above
• Test Coverage: No automated tests (consistent with rest of codebase)
• Documentation: Clear inline comments and UI descriptions

---

Recommendation

Approval with minor architectural fix requested

The PR is well-implemented and follows most existing patterns. However, I recommend addressing the architectural inconsistency by adding the Terminal interface method before merging. This ensures the codebase maintains a consistent architecture pattern.

Great work overall! The feature is useful and the implementation is clean.

[claude]

Code Review for PR #198: Configurable Shift+Enter Behavior

Overall, this is a well-implemented feature that adds useful functionality matching iTerm2 behavior. The code follows existing patterns, is properly documented, and includes UI integration. Here's my detailed feedback:

---

✅ Strengths

1. Clean Architecture: The implementation follows the existing codebase patterns correctly:

   • Settings added to TerminalSettings.kt
   • Override support added to TerminalSettingsOverride.kt
   • UI integrated into BehaviorSettingsSection.kt
   • Encoder configuration in ProperTerminal.kt

2. Good Documentation: Clear inline comments explaining the behavior (e.g., // Default: Shift+Enter sends LF (newline) - matches iTerm2 behavior)

3. User-Friendly Default: Defaulting to "newline" matches iTerm2 behavior, which is what users expect (per issue Shift + Return - Does not take me to new line. #196)

4. Proper UI Integration: The dropdown implementation follows existing patterns and includes helpful descriptions

---

🔍 Issues & Recommendations

1. Potential State Synchronization Bug (Medium Priority)

In ProperTerminal.kt:262-275, you're configuring both the local keyEncoder and the terminal's internal encoder. However, there's a subtle issue:

LaunchedEffect(settings.shiftEnterBehavior, settings.altSendsEscape) {
    // Configure local key encoder
    keyEncoder.setShiftEnterSendsNewline(shiftEnterNewline)
    keyEncoder.setAltSendsEscape(settings.altSendsEscape)
    // Configure terminal's internal encoder
    terminal.setShiftEnterSendsNewline(shiftEnterNewline)
    terminal.setAltSendsEscape(settings.altSendsEscape)
}

Problem: The keyEncoder is created with remember { TerminalKeyEncoder() }, which means it's initialized BEFORE the LaunchedEffect runs. During initialization, TerminalKeyEncoder sets the default mapping to Ascii.LF (line 53). Then the LaunchedEffect runs and overwrites it.

Recommendation: This should work correctly, but for clarity and consistency with existing code patterns (like altSendsEscape), consider checking if there's already a pattern for initial configuration. The current approach is functional but might cause a brief moment where the encoder has the default config before the effect runs.

---

2. Missing Null Safety Check (Low Priority)

In BehaviorSettingsSection.kt:120-132, the dropdown uses a when expression with an else branch:

selectedOption = when (settings.shiftEnterBehavior) {
    "newline" -> "Newline (iTerm2 style)"
    else -> "Same as Enter"
}

Problem: If a user manually edits settings.json and adds an invalid value (e.g., "shiftEnterBehavior": "invalid"), it will default to "Same as Enter" in the UI but still send "invalid" to the backend.

Recommendation: Add validation in the onOptionSelected callback or use an enum instead of strings. For consistency with the existing codebase (which uses strings for settings), add a comment noting the valid values.

---

3. Code Duplication in LaunchedEffect (Low Priority)

The LaunchedEffect block configures both keyEncoder and terminal with identical calls:

keyEncoder.setShiftEnterSendsNewline(shiftEnterNewline)
keyEncoder.setAltSendsEscape(settings.altSendsEscape)
terminal.setShiftEnterSendsNewline(shiftEnterNewline)
terminal.setAltSendsEscape(settings.altSendsEscape)

Recommendation: This is fine for now, but consider adding a comment explaining why both need to be configured (the comment at line 265-266 is good but could be more specific about when each encoder is used).

---

4. Consistency with Existing Code (Minor Observation)

Looking at the existing code, I notice that altSendsEscape is handled similarly (configured in a LaunchedEffect). However, for altSendsEscape, there's no initial configuration in TerminalKeyEncoder's init block—it defaults to true (line 18).

For Shift+Enter, you're setting the default in the init block (line 53) AND in the LaunchedEffect. This is fine, but note that if you change the default in TerminalSettings.kt from "newline" to "same-as-enter", you'd also need to update line 53 in TerminalKeyEncoder.kt.

Recommendation: Add a comment linking the default in TerminalKeyEncoder.kt:53 to the default in TerminalSettings.kt:232 to prevent them from getting out of sync.

---

🧪 Testing Suggestions

1. Test with Multiple Tabs: Verify that the setting applies correctly when:

   • Creating a new tab with the setting already changed
   • Changing the setting while multiple tabs are open
   • Closing and reopening tabs

2. Test Settings Persistence:

   • Change the setting, restart the app, verify it persists
   • Manually edit ~/.bossterm/settings.json with invalid values

3. Test with Override:

   • Verify that TerminalSettingsOverride works correctly for embedded terminals

---

🔒 Security & Performance

• ✅ No security concerns
• ✅ No performance concerns (the LaunchedEffect only runs when settings change)
• ✅ No memory leaks or resource issues

---

📝 Minor Suggestions

1. Documentation: Consider adding this new setting to the project's CLAUDE.md file under the "Settings System" section

2. Default Value: The PR description mentions "iTerm2 style" as the default—this is great! Consider adding a note in the settings UI that this matches iTerm2 behavior for users familiar with that terminal

---

✅ Verdict

This PR is ready to merge with minor suggestions for future improvement. The implementation is solid, follows existing patterns, and solves the reported issue (#196). The concerns raised above are mostly about edge cases and documentation—none are blocking.

Recommended Action: Approve and merge after author reviews the suggestions (especially #1 about state synchronization, which is more of a clarification than a bug).

---

Great work! 🎉

[claude]

Code Review: Shift+Enter Behavior Configuration

Summary

This PR implements configurable Shift+Enter behavior, allowing users to choose between sending a newline (LF, iTerm2 style) or behaving like regular Enter (CR). The implementation is clean, well-documented, and follows the project's established patterns.

✅ Strengths

1. Excellent Documentation

   • Clear comments in TerminalKeyEncoder.kt explaining the default behavior and sync requirement
   • Comprehensive KDoc for setShiftEnterSendsNewline() explaining both modes
   • Well-documented settings field with valid values and default justification

2. Consistent Architecture

   • Follows existing altSendsEscape pattern (both in API design and UI placement)
   • Properly updates both local keyEncoder and terminal's internal encoder
   • Uses LaunchedEffect correctly for settings reactivity

3. Good Default Choice

   • Defaulting to "newline" (iTerm2 style) is the right choice for a modern terminal
   • Default is consistently applied in both TerminalKeyEncoder.init() and TerminalSettings

4. Override Support

   • Correctly added to TerminalSettingsOverride for per-instance customization

🔍 Code Quality Observations

Minor: Validation Handling

In BehaviorSettingsSection.kt (lines 121-125), invalid values default to "newline":

selectedOption = when (settings.shiftEnterBehavior) {
    "newline" -> "Newline (iTerm2 style)"
    "same-as-enter" -> "Same as Enter"
    else -> "Newline (iTerm2 style)" // Invalid value defaults to newline
}

Consideration: Since settings are persisted to JSON, a corrupted/manually-edited settings file could contain invalid values. The current approach (silent default) is reasonable, but you could optionally log a warning or normalize the value on load. This is not blocking - the fallback is safe and sensible.

Architecture: Dual Encoder Configuration

The implementation configures both keyEncoder (local) and terminal (internal) in ProperTerminal.kt:

// Configure local key encoder (used for keyboard input at line ~1358)
keyEncoder.setShiftEnterSendsNewline(shiftEnterNewline)
keyEncoder.setAltSendsEscape(settings.altSendsEscape)
// Configure terminal's internal encoder (via Terminal interface)
terminal.setShiftEnterSendsNewline(shiftEnterNewline)
terminal.setAltSendsEscape(settings.altSendsEscape)

The comments explain this is for "architectural consistency", but based on the code analysis:

• Only keyEncoder (local) is actually used at line 1367: keyEncoder.getCode(vkCode, modifiers)
• The terminal's encoder may be for future use or internal processing

This is fine - the redundant configuration is harmless and ensures consistency if terminal emulation code later uses its own encoder. The comment explains the reasoning well.

🧪 Test Coverage

Missing: No unit tests for the new functionality.

Recommendation: Consider adding tests similar to ActionRegistryTest.kt:

// In TerminalKeyEncoderTest.kt (new file)
@Test
fun testShiftEnterDefaultSendsNewline() {
    val encoder = TerminalKeyEncoder()
    val bytes = encoder.getCode(VK_ENTER, InputEvent.SHIFT_MASK)
    assertNotNull(bytes)
    assertEquals(1, bytes.size)
    assertEquals(Ascii.LF.code.toByte(), bytes[0])
}

@Test
fun testShiftEnterConfigurableBehavior() {
    val encoder = TerminalKeyEncoder()
    
    // Test newline mode (default)
    encoder.setShiftEnterSendsNewline(true)
    val lfBytes = encoder.getCode(VK_ENTER, InputEvent.SHIFT_MASK)
    assertEquals(Ascii.LF.code.toByte(), lfBytes?.get(0))
    
    // Test same-as-enter mode
    encoder.setShiftEnterSendsNewline(false)
    val crBytes = encoder.getCode(VK_ENTER, InputEvent.SHIFT_MASK)
    assertEquals(Ascii.CR.code.toByte(), crBytes?.get(0))
}

Not blocking - manual testing is documented in the PR, but automated tests would improve confidence and prevent regressions.

🔒 Security & Performance

• ✅ No security concerns: Simple key-to-byte mapping
• ✅ No performance impact: Single map lookup, same as existing keys
• ✅ No memory leaks: Settings properly scoped to composable lifecycle

📝 Suggestions for Future Work (Optional)

1. Settings Validation: Consider normalizing invalid values on load (not in UI code)
2. Unit Tests: Add TerminalKeyEncoderTest.kt for regression prevention
3. User Documentation: If you have user-facing docs, document this feature

✅ Verdict

LGTM with minor suggestions. The implementation is solid, follows project conventions, and solves the stated problem (#196). The missing tests are not blocking given the manual test plan in the PR description.

Recommendation: Approve and merge. Consider adding unit tests in a follow-up PR if you want to improve coverage.

---

Test Plan Verification:

• ✅ Default behavior matches iTerm2 (newline)
• ✅ Setting persists in JSON
• ✅ UI dropdown follows existing patterns
• ✅ Override support included

Great work! 🚀