refactor: make tui size dynamic and make settings modular#336
refactor: make tui size dynamic and make settings modular#336SuperCoolPencil merged 6 commits intomainfrom
Conversation
…rame size helpers
…struct within Settings
Binary Size Analysis
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| type CategorySettings struct { | ||
| CategoryEnabled bool `json:"category_enabled" ui_label:"Manage Categories" ui_desc:"Sort downloads into subfolders by file type. Press Enter to open Category Manager."` | ||
| Categories []Category `json:"categories" ui_ignored:"true"` | ||
| } |
There was a problem hiding this comment.
Moving CategoryEnabled and Categories from GeneralSettings (JSON key "general") to a new top-level CategorySettings (JSON key "categories") breaks backward compatibility with every existing settings.json. On the next launch the Go JSON decoder ignores the old "general"."category_enabled" and "general"."categories" fields, and because there is no "categories" top-level key in the old file, CategorySettings falls back to its defaults: CategoryEnabled = false and Categories = DefaultCategories().
Users who had category sorting enabled and custom categories defined will silently:
- Have category routing disabled.
- Lose all custom category rules, replaced by the defaults.
Consider adding a one-time migration in LoadSettings that reads the old field locations and promotes them to the new structure before the first save, similar to what many apps do with a schema-version field.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/config/settings.go
Line: 43-46
Comment:
**Breaking backward compatibility — silent settings loss on upgrade**
Moving `CategoryEnabled` and `Categories` from `GeneralSettings` (JSON key `"general"`) to a new top-level `CategorySettings` (JSON key `"categories"`) breaks backward compatibility with every existing `settings.json`. On the next launch the Go JSON decoder ignores the old `"general"."category_enabled"` and `"general"."categories"` fields, and because there is no `"categories"` top-level key in the old file, `CategorySettings` falls back to its defaults: `CategoryEnabled = false` and `Categories = DefaultCategories()`.
Users who had category sorting enabled and custom categories defined will silently:
1. Have category routing disabled.
2. Lose all custom category rules, replaced by the defaults.
Consider adding a one-time migration in `LoadSettings` that reads the old field locations and promotes them to the new structure before the first save, similar to what many apps do with a schema-version field.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Greptile Summary
This PR refactors the TUI to replace magic numeric offsets with
lipglossframe-size helpers for dynamic layout calculations, and splits category-related settings (CategoryEnabled,Categories) out ofGeneralSettingsinto a new top-levelCategorySettingsstruct. It also adds label truncation to the settings list view to prevent layout overflow.CategoryEnabled/Categoriesfrom JSON key"general"to a new top-level key"categories"means any existingsettings.jsonthat had category sorting enabled will silently reset to defaults (CategoryEnabled = false,Categories = DefaultCategories()) on next launch — no migration path is provided.view_settings.gouseslen()(byte count) rather thanlipgloss.Width()(display columns) for prefix and label measurement, causing the selected-row label to truncate slightly earlier than necessary.Confidence Score: 4/5
Safe to merge with awareness of the settings migration break; the layout refactor and struct split are otherwise correct.
One P1 finding: moving CategorySettings to a new JSON top-level key with no migration silently drops existing users' category configuration on upgrade. All other changes (lipgloss frame-size helpers, label truncation) are functionally equivalent to what they replace and are well-tested.
internal/config/settings.go — backward-incompatible JSON key change with no migration path
Vulnerabilities
No security concerns identified. The refactor touches only UI layout, settings struct organization, and JSON serialization — no authentication, network, or input-validation paths are changed.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[settings.json on disk] -->|LoadSettings| B{Top-level key present?} B -->|Old format: general.category_enabled| C[GeneralSettings - field removed, silently ignored] B -->|New format: categories.category_enabled| D[CategorySettings.CategoryEnabled OK] C --> E[CategorySettings falls back to defaults, CategoryEnabled=false] D --> F[Category routing works correctly] E -->|User impact| G[Category sorting silently disabled, Custom categories lost] subgraph NewStruct[Refactored Settings Struct] H[Settings] H --> I[GeneralSettings - json: general] H --> J[NetworkSettings - json: network] H --> K[PerformanceSettings - json: performance] H --> L[CategorySettings NEW - json: categories] L --> M[CategoryEnabled bool] L --> N[Categories slice] endComments Outside Diff (3)
internal/config/settings.go, line 223-243 (link)Moving
category_enabledandcategoriesfrom the"general"JSON key to a new top-level"categories"key is a breaking schema change.LoadSettings()initialises fromDefaultSettings()(which resetsCategoryEnabledtofalseandCategoriestoDefaultCategories()) and then unmarshals the stored JSON on top. Any existing file that contains"general": {"category_enabled": true, "categories": [...]}will have no"categories"top-level key, so the unmarshal leaves those fields at their defaults — silently discarding the user's configuration.A one-time migration in
LoadSettings()would prevent data loss, for example:Prompt To Fix With AI
docs/SETTINGS.md, line 14-29 (link)docs/SETTINGS.mdnot updated for newcategoriessectionThe documented example JSON still shows only
general,network, andperformance. After this PR, category settings live under a top-level"categories"key and the guide omits that section entirely, leaving users with no reference for the new structure.The example should be updated to:
{ "general": { ... }, "network": { ... }, "performance": { ... }, "categories": { "category_enabled": true } }A new Category Settings table should also be added documenting
category_enabled(the only user-editable key in that section).Context Used: docs/SETTINGS.md (source)
Prompt To Fix With AI
internal/tui/view_settings.go, line 393-413 (link)renderSettingsTwoColumnstill hardcodes two offsets that were explicitly the target of this refactor:rightRows := bodyHeight - 2— therightBoxstyle isPadding(1, 2)(vertical frame = 2), so this should use the style'sGetVerticalFrameSize().rightContent := m.renderSettingsDetailBlock(..., rightWidth-4, ...)— the samePadding(1, 2)gives a horizontal frame of 4, soGetHorizontalFrameSize()would be clearer.Suggested approach:
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "style: fix some inconsistent hardcoded v..." | Re-trigger Greptile