fix: Native title bar toggle not persisting after restart

[kshivang]

Summary

• Fix native title bar toggle setting not being saved before app restart
• Add encodeDefaults = true to JSON serializer to ensure all settings are written to disk
• Force immediate save before restart when toggling native title bar setting

Test plan

• Toggle "Use Native Title Bar" in Settings → Visual
• Click "Restart" in the confirmation dialog
• Verify ~/.bossterm/settings.json contains "useNativeTitleBar": false
• Verify app restarts with custom title bar (no native macOS decorations)

🤖 Generated with Claude Code

[claude]

Code Review: Fix Native Title Bar Toggle Persistence

Summary

This PR fixes a bug where the native title bar setting wasn't being persisted to disk before app restart. The fix adds two key changes:

1. Enable encodeDefaults = true in the JSON serializer
2. Force immediate save via onSettingsSave?.invoke() before restart

✅ Positive Aspects

Well-targeted fix: The changes are minimal and surgical, addressing exactly what's needed without over-engineering.

Follows existing pattern: The onSettingsSave?.invoke() pattern is already used in PerformanceSettingsSection.kt:159, making this consistent with the codebase.

Root cause addressed: The encodeDefaults = true change ensures that fields with default values (like useNativeTitleBar: Boolean = true) are always written to the JSON file, not just when they differ from defaults.

---

🔍 Observations & Questions

1. Potential Race Condition Risk (Minor)

The current sequence is:

onSettingsChange(settings.copy(useNativeTitleBar = pendingNativeTitleBarValue))
onSettingsSave?.invoke()  // Force immediate save before restart
showRestartDialog = false
onRestartApp?.invoke()

Analysis: Looking at SettingsWindow.kt:69-76, I can see that:

• onSettingsChange updates the local pendingSettings state (no I/O)
• onSettingsSave calls settingsManager.updateSettings(pendingSettings), which synchronously writes to disk

This should work correctly because:

• The settings update happens in-memory first
• The save happens synchronously before the restart
• File I/O operations are blocking in the current implementation

Potential concern: If the architecture changes to make saves asynchronous in the future, this could break. Consider adding a comment documenting that the save must complete before restart.

2. The encodeDefaults = true Impact

Positive: Solves the immediate bug and ensures completeness of the settings file.

Trade-off: This will increase the size of settings.json files since ALL fields will now be written, even when they match defaults. For BossTerm's current settings structure (~50 fields), this is negligible (<5KB additional data).

Alternative consideration (not required, just for discussion): Another approach would have been to only change the specific field's default or ensure it's explicitly set. However, encodeDefaults = true is the right choice here because:

• It makes settings files more explicit and self-documenting
• It prevents similar bugs with other boolean fields that default to true
• The performance/size impact is minimal

3. Test Coverage

Observation: No tests currently exist for SettingsManager (verified via Grep). This bug wouldn't have occurred with a test like:

@Test
fun `native title bar setting persists after toggle`() {
    val manager = SettingsManager.withCustomPath(tempFile.path)
    manager.updateSettings(TerminalSettings.DEFAULT.copy(useNativeTitleBar = false))
    
    val reloaded = SettingsManager.withCustomPath(tempFile.path)
    assertFalse(reloaded.settings.value.useNativeTitleBar)
}

Recommendation: Consider adding basic SettingsManager tests in a follow-up PR, especially for settings that trigger restarts.

4. Documentation

The inline comment at SettingsManager.kt:30 is excellent:

encodeDefaults = true  // Ensure all fields are written, not just non-default ones

Similarly good at VisualSettingsSection.kt:247. Clear inline comments like these are valuable for future maintainers.

---

🎯 Final Verdict

Approve with minor suggestions

The fix is correct and follows established patterns in the codebase. The two changes work together well:

• encodeDefaults = true ensures the setting is written to disk
• onSettingsSave?.invoke() ensures it happens before restart

Minor Suggestions (Optional):

1. Consider adding a comment at VisualSettingsSection.kt:246-249 noting that the save MUST be synchronous before restart
2. File a follow-up issue to add SettingsManager unit tests to prevent similar issues

Rating: ✅ Ready to merge (the suggestions are enhancements, not blockers)

---

Reviewed following CLAUDE.md guidelines. No runtime execution performed per dev guidelines.