Skip to content

fix: stop discarding loaded settings in SettingsApp.__init__#65

Open
saya-7 wants to merge 1 commit into
TruFoundation:mainfrom
saya-7:fix/dirty-settings-init
Open

fix: stop discarding loaded settings in SettingsApp.__init__#65
saya-7 wants to merge 1 commit into
TruFoundation:mainfrom
saya-7:fix/dirty-settings-init

Conversation

@saya-7

@saya-7 saya-7 commented Jun 10, 2026

Copy link
Copy Markdown

SettingsApp.__init__ seeded self.dirty_settings from the persisted settings and then immediately re-bound the attribute to a fresh empty dict two lines later. As a result every setting the user had on disk was silently dropped before the UI rendered, and the dirty-state buffer always started blank.

The surrounding comment ("Track changes to settings without losing data when switching categories") was clearly describing the seeded copy, not the empty one. This change drops the second assignment so the dirty buffer starts equal to the loaded settings, and adds a regression test that fails on the old code path.

Closes #64

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed an issue where application settings were not properly restored from saved values on startup. User preferences, including theme selection, are now correctly initialized and properly maintained when switching between different settings categories.

SettingsApp.__init__ seeded `self.dirty_settings` from the loaded
settings and then immediately re-bound it to an empty dict, which
silently discarded any persisted values before the UI rendered.
Drop the second assignment so dirty state starts equal to the loaded
settings, as the surrounding comment intended.

Closes TruFoundation#64
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f348600f-afb2-4b7d-bdf9-7e79fac8e826

📥 Commits

Reviewing files that changed from the base of the PR and between 4c6a359 and 75d7c92.

📒 Files selected for processing (2)
  • tests/test_settings.py
  • trushell/commands/settings.py

📝 Walkthrough

Walkthrough

SettingsApp.init now initializes dirty_settings as a copy of loaded settings instead of an empty dict, fixing a bug where the dict was declared twice and the second declaration overwrote the first. Tests verify persisted settings are retained and that the "theme" value persists in dirty_settings after category switches.

Changes

Dirty Settings Initialization Fix

Layer / File(s) Summary
Fix dirty_settings initialization and add regression tests
trushell/commands/settings.py, tests/test_settings.py
dirty_settings is now initialized from dict(self.settings) instead of an empty dict. New regression test test_dirty_settings_seeded_from_loaded_settings verifies persisted defaults are retained. Existing category-switching test enhanced to assert "theme" value persists in dirty_settings.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A settings mix-up, twice declared,
One line of code, and all else spared!
Now dirty stays true to what was kept,
While tests stand guard on values slept.
The rabbit hops: initialization blessed! 🌙

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main fix: preventing the loss of loaded settings in SettingsApp.init.
Linked Issues check ✅ Passed The PR fully addresses issue #64 by removing the redundant empty dict assignment that was overwriting the initially-loaded dirty_settings.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the double declaration issue; no unrelated modifications are present in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

SettingsApp __init__ declares dirty_settings twice

1 participant