Enhance Category Manager UX and Implement Robust Settings Validation

[SuperCoolPencil]

This PR improves the Category Manager UX and introduces a comprehensive validation system for all application settings.

Key Changes:

Category Manager

• Added real-time validation feedback in the UI for category names, patterns, and paths.
• Enabled Up/Down arrow key navigation between input fields while editing.
• Implemented a reset confirmation modal to prevent accidental loss of custom categories.
• Standardized debug logging for category lifecycle events.

Settings & Validation

• Implemented a structured, hierarchical validation system (OOP-style) where each settings group manages its own integrity.
• Added real-time TUI feedback for invalid numeric ranges, durations, URLs, and DNS configurations.
• Introduced "partial rollback" logic: if the settings.json file on disk contains invalid values, Surge now corrects only the broken fields while preserving all other valid customizations.
• Added rigorous path verification for default download directories and category folders.

Documentation & Tests

• Updated docs/SETTINGS.md to include a section on the new self-healing configuration logic.
• Added comprehensive unit tests in internal/config/settings_test.go covering range enforcement, path rollback, and category pruning.

Greptile Summary

This PR adds a hierarchical settings validation system with per-group Validate() methods, partial rollback of invalid fields on startup, real-time TUI error feedback for settings and the category manager, Up/Down field navigation, and a reset confirmation modal for categories. The implementation is solid — the shared ValidateDNSList helper eliminates the previously duplicated DNS parsing, proxy URL validation now correctly checks for empty Scheme/Host, and the updateCategoryResetConfirm handler correctly derives defaults from config.DefaultSettings(). Only P2 findings remain: the new viewCategoryResetConfirm introduces a third verbatim copy of the button-rendering boilerplate, and the new test suite is missing coverage for the category path-rollback branch.

Confidence Score: 5/5

Safe to merge — no P0 or P1 findings; all remaining feedback is non-blocking style and test coverage.

Only P2 findings present: a style concern around duplicated button-rendering boilerplate and a missing test case for category path rollback. No runtime errors, data loss risk, or security issues were identified.

internal/tui/view.go (button rendering duplication) and internal/config/settings_test.go (missing category path rollback test).

Important Files Changed

Filename	Overview
internal/config/settings.go	Adds hierarchical Validate() methods for all settings groups with range enforcement, path checks, proxy URL guards, and a shared ValidateDNSList helper; StartupWarnings field accumulates messages for post-startup display.
internal/config/settings_test.go	Comprehensive new Validate test suite covering range resets, path rollback, and regex pruning; missing coverage for the category path-rollback code path.
internal/tui/update_settings.go	Adds validateSetting() for real-time TUI feedback with correct non-negative guards on durations; settingsError cleared on navigation/tab switches.
internal/tui/view.go	New viewCategoryResetConfirm() modal works correctly but triples the button-rendering boilerplate that already appears in viewQuitConfirm and viewRestartConfirm.
internal/tui/update_modals.go	updateCategoryResetConfirm() now correctly derives defaults from config.DefaultSettings() instead of hardcoding false.
internal/tui/update_category.go	Adds Up/Down field navigation, migrates errors to catMgrError for inline display, and standardises debug logging for category lifecycle events.
internal/tui/view_settings.go	Error line injected into layout correctly; bodyHeight and usedHeight accounting updated to include errorHeight and the trailing gap.
internal/tui/view_category.go	Inline catMgrError line rendered below infoLine with correct height accounting.
cmd/root.go	publishStartupWarnings() correctly guards on globalSettings nil and is only reachable after ensureGlobalLocalServiceAndLifecycle() succeeds (guaranteeing GlobalService is non-nil).
cmd/bugreport.go	Silences fmt.Fprint* return values with blank identifiers; cosmetic cleanup with no functional change.
internal/tui/model.go	Adds CategoryResetConfirmState, settingsError, and catMgrError fields; removes [NEW] comment tag from RestartRequested.
docs/SETTINGS.md	Adds a Configuration Validation section accurately documenting range enforcement, path verification, proxy/DNS syntax checks, category pruning, and corrupt-JSON fallback.
cmd/root_lifecycle_test.go	Pre-creates custom directories with os.MkdirAll so tests pass under the new path-verification logic in settings.Validate().
internal/tui/delete_resilience_test.go	Whitespace-only formatting cleanup; no functional change.

Comments Outside Diff (4)

1. internal/config/settings.go, line 104-109 (link)

   url.Parse won't catch invalid proxy URLs

   url.Parse in Go is extremely permissive and almost never returns an error. Strings like "badproxy", "://noscheme", or "ftp://missing-host" all parse without error, resulting in a url.URL with empty Scheme or Host. The self-healing behavior documented in SETTINGS.md — "Proxy URLs are validated for correct syntax" — will silently skip any invalid proxy URL read from disk, leaving the broken value in place.

   The TUI's validateSetting (in update_settings.go) correctly guards against this by checking u.Scheme == "" || u.Host == "". The same guard should be applied here:

   if ns.ProxyURL != "" {
       u, err := url.Parse(ns.ProxyURL)
       if err != nil || u.Scheme == "" || u.Host == "" {
           ns.ProxyURL = defaults.ProxyURL
       }
   }

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/config/settings.go
   Line: 104-109
   
   Comment:
   **`url.Parse` won't catch invalid proxy URLs**
   
   `url.Parse` in Go is extremely permissive and almost never returns an error. Strings like `"badproxy"`, `"://noscheme"`, or `"ftp://missing-host"` all parse without error, resulting in a `url.URL` with empty `Scheme` or `Host`. The self-healing behavior documented in `SETTINGS.md` — "Proxy URLs are validated for correct syntax" — will silently skip any invalid proxy URL read from disk, leaving the broken value in place.
   
   The TUI's `validateSetting` (in `update_settings.go`) correctly guards against this by checking `u.Scheme == "" || u.Host == ""`. The same guard should be applied here:
   
   ```go
   if ns.ProxyURL != "" {
       u, err := url.Parse(ns.ProxyURL)
       if err != nil || u.Scheme == "" || u.Host == "" {
           ns.ProxyURL = defaults.ProxyURL
       }
   }
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

2. internal/tui/update_modals.go, line 621-630 (link)

   Hardcoded false should be derived from defaults

   CategoryEnabled is reset to a hardcoded false rather than the value returned by config.DefaultSettings(). This means if the default ever changes (e.g., to true), this reset path diverges silently. Using the defaults struct is consistent with how every other field is rolled back in resetSettingToDefault and Validate():

   defaults := config.DefaultSettings()
   m.Settings.Categories = defaults.Categories
   m.addLogEntry(LogStyleStarted.Render("\u2714 Categories reset to defaults"))
   utils.Debug("Categories Reset to Defaults")
   m.state = SettingsState
   m.quitConfirmFocused = 0
   return m, nil

   This also avoids having to remember to update two places if a new CategorySettings field is added.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/tui/update_modals.go
   Line: 621-630
   
   Comment:
   **Hardcoded `false` should be derived from defaults**
   
   `CategoryEnabled` is reset to a hardcoded `false` rather than the value returned by `config.DefaultSettings()`. This means if the default ever changes (e.g., to `true`), this reset path diverges silently. Using the defaults struct is consistent with how every other field is rolled back in `resetSettingToDefault` and `Validate()`:
   
   ```go
   defaults := config.DefaultSettings()
   m.Settings.Categories = defaults.Categories
   m.addLogEntry(LogStyleStarted.Render("\u2714 Categories reset to defaults"))
   utils.Debug("Categories Reset to Defaults")
   m.state = SettingsState
   m.quitConfirmFocused = 0
   return m, nil
   ```
   
   This also avoids having to remember to update two places if a new `CategorySettings` field is added.
   
   How can I resolve this? If you propose a fix, please make it concise.

3. internal/tui/update_settings.go, line 692-695 (link)

   Stale error persists when input is cleared

   The error is cleared only when m.SettingsInput.Value() != "". If a user types an invalid value (error appears), then deletes all characters, the error remains shown even though the input field is now empty. The condition should just check whether m.settingsError != "" to clear it on any keystroke:

   if m.settingsError != "" {
       m.settingsError = ""
   }

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/tui/update_settings.go
   Line: 692-695
   
   Comment:
   **Stale error persists when input is cleared**
   
   The error is cleared only when `m.SettingsInput.Value() != ""`. If a user types an invalid value (error appears), then deletes all characters, the error remains shown even though the input field is now empty. The condition should just check whether `m.settingsError != ""` to clear it on any keystroke:
   
   ```go
   if m.settingsError != "" {
       m.settingsError = ""
   }
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

4. internal/config/settings.go, line 112-136 (link)

   Duplicated DNS validation logic

   The DNS-parsing loop (split on ,, net.SplitHostPort, net.ParseIP) is essentially identical to the one in validateSetting in update_settings.go. Per the project's no-duplicate-logic rule, consider extracting a shared helper (e.g., config.ValidateDNSList(s string) error) that both call, so any future changes (hostname support, IPv6 tweaks) only need to be made in one place.

   Rule Used: What: Eliminate duplicate logic, functions, or cod... (source)

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/config/settings.go
   Line: 112-136
   
   Comment:
   **Duplicated DNS validation logic**
   
   The DNS-parsing loop (split on `,`, `net.SplitHostPort`, `net.ParseIP`) is essentially identical to the one in `validateSetting` in `update_settings.go`. Per the project's no-duplicate-logic rule, consider extracting a shared helper (e.g., `config.ValidateDNSList(s string) error`) that both call, so any future changes (hostname support, IPv6 tweaks) only need to be made in one place.
   
   **Rule Used:** What: Eliminate duplicate logic, functions, or cod... ([source](https://app.greptile.com/review/custom-context?memory=f0a796a9-684f-4dfb-a5cf-8c99c16b63a1))
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: internal/tui/view.go
Line: 947-969

Comment:
**Triplicated button-rendering boilerplate**

The `activeFirst / inactiveFirst / renderBtn / quitConfirmFocused` swap pattern is now copy-pasted verbatim across three functions — `viewQuitConfirm` (line 794), `viewRestartConfirm` (line 875), and the new `viewCategoryResetConfirm` (line 947). The only differences are the accent colour and the button label suffix. Per the project's no-duplicate-logic rule, extracting a shared helper (e.g., `renderYesNoButtons(focused int, accentColor lipgloss.Color, yesLabel, noLabel string) string`) would remove ~20 duplicated lines from each modal and ensure future colour/layout tweaks only need to happen in one place.

**Rule Used:** What: Eliminate duplicate logic, functions, or cod... ([source](https://app.greptile.com/review/custom-context?memory=f0a796a9-684f-4dfb-a5cf-8c99c16b63a1))

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/config/settings_test.go
Line: 487-515

Comment:
**Category path-rollback code path is untested**

`CategorySettings.Validate` has an explicit `os.Stat` check that resets a category's path to `fallbackDir` when it is broken or inaccessible, but none of the new test cases exercise this branch. The existing tests only cover regex-invalid categories (which are pruned entirely), not the case of a valid-regex / broken-path category that should be kept but have its path replaced. A test case like the following would cover it:

```go
{
    name: "Broken Category Path Rollback",
    modify: func(s *Settings) {
        s.Categories.Categories = []Category{
            {Name: "Movies", Pattern: `\.mkv$`, Path: "/non/existent/path"},
        }
    },
    validate: func(t *testing.T, s *Settings) {
        if len(s.Categories.Categories) != 1 {
            t.Fatalf("expected category to be kept, got %d", len(s.Categories.Categories))
        }
        if s.Categories.Categories[0].Path == "/non/existent/path" {
            t.Errorf("expected broken path to be rolled back, but it was not")
        }
    },
},
```

**Rule Used:** What: All code changes must include tests for edge... ([source](https://app.greptile.com/review/custom-context?memory=2b22782d-3452-4d55-b059-e631b2540ce8))

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (5): Last reviewed commit: "refactor: replace deprecated reflect.Ptr..." | Re-trigger Greptile}

Context used:

• Rule used - What: Eliminate duplicate logic, functions, or cod... (source)
• Rule used - What: All code changes must include tests for edge... (source)

[github-actions]

Binary Size Analysis

⚠️ Size Increased

Version	Human Readable	Raw Bytes
Main	19.54 MB	20488484
PR	19.61 MB	20562212
Difference	72.00 KB	73728

[greptile-apps]

Reset intercept breaks category_enabled and is semantically mismatched

The Reset key handler for category_enabled returns early in both branches of the modal, which means the CategoryEnabled boolean can never be reset to its default through this code path. If the user confirms, all custom categories are wiped but CategoryEnabled itself is left unchanged. If the user cancels, nothing at all happens — the expected reset of the boolean is silently skipped.

category_enabled is a simple boolean toggle (CategorySettings.CategoryEnabled), so wiring its "Reset to Default" action to a full-category-wipe confirmation is semantically wrong. Consider using a dedicated key (e.g., the Edit key on the "Manage Categories" row if one exists) to open the reset modal, and letting the existing resetSettingToDefault path handle category_enabled normally.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/tui/update_settings.go
Line: 237-242

Comment:
**Reset intercept breaks `category_enabled` and is semantically mismatched**

The Reset key handler for `category_enabled` returns early in both branches of the modal, which means the `CategoryEnabled` boolean can **never** be reset to its default through this code path. If the user confirms, all custom categories are wiped but `CategoryEnabled` itself is left unchanged. If the user cancels, nothing at all happens — the expected reset of the boolean is silently skipped.

`category_enabled` is a simple boolean toggle (`CategorySettings.CategoryEnabled`), so wiring its "Reset to Default" action to a full-category-wipe confirmation is semantically wrong. Consider using a dedicated key (e.g., the `Edit` key on the "Manage Categories" row if one exists) to open the reset modal, and letting the existing `resetSettingToDefault` path handle `category_enabled` normally.

How can I resolve this? If you propose a fix, please make it concise.

[SuperCoolPencil]

I have verified the implementation of all Greptile suggestions (Proxy URL validation, shared DNS helper, category fallback path, default reset values, and TUI error clearing). Additionally, I fixed the SA1019 lint error in internal/config/settings_test.go by migrating from reflect.PtrTo to reflect.PointerTo. All tests and linting are now passing.