Skip to content

tui refactor#461

Merged
SuperCoolPencil merged 10 commits into
mainfrom
tui-refactor
May 21, 2026
Merged

tui refactor#461
SuperCoolPencil merged 10 commits into
mainfrom
tui-refactor

Conversation

@SuperCoolPencil
Copy link
Copy Markdown
Member

@SuperCoolPencil SuperCoolPencil commented May 21, 2026

  • remove redundant test
  • refactor: rename buildPoolIsNameActive to buildActiveDownloadChecker for better clarity
  • refactor: migrate settings to a schema-driven structure using wrapper types for enhanced validation and restart tracking
  • refactor: improve type resilience in settings formatting to support various numeric input types
  • refactor: replace TUI settings conversion tests with comprehensive metadata validation and basic format tests
  • chore: replace em dashes with hyphens in comments and documentation strings

Greptile Summary

This PR is a substantial TUI refactor that migrates all settings fields from plain Go primitives (int, bool, string, time.Duration) to *Setting wrapper types carrying metadata (label, description, type tag, NeedsRestart, validation function). The migration touches 66 files and replaces reflection-on-reflect.Kind dispatch with reflection-on-Setting.Type string dispatch throughout the settings read/write/format paths.

  • internal/config/settings.go + settings_schema.go: introduces the Setting type, Resolve[T](), Clone() via JSON round-trip, and a schema-driven CategoriesList used for metadata export and validation.
  • internal/tui/view_settings.go + helpers.go: setSettingValue, formatSettingValue, resetSettingToDefault, and checkRestartRequirement are all rewritten to operate on *Setting pointers; snapshotSettings is upgraded from a shallow struct copy to Clone(), which is critical because the old shallow copy would now share *Setting pointers between live and baseline.
  • Tests: TestSettingsUnitConversion is replaced by TestSetSettingValueConversions (targeted conversion checks) and TestSettingsMetadataValidation (schema completeness).

Confidence Score: 5/5

Safe to merge; the refactor is mechanically correct and the critical upgrade from shallow-copy to deep-Clone in snapshotSettings prevents a shared-pointer bug that would have broken restart detection.

The new type-dispatch paths in setSettingValue, formatSettingValue, and checkRestartRequirement all handle the float64-after-JSON-deserialization coercion correctly. Clone() and ResolveT are sound. No correctness regressions were found in the production paths.

internal/config/settings_test.go is missing assertions that duration values survive the JSON round-trip correctly.

Important Files Changed

Filename Overview
internal/config/settings.go Core migration from plain Go types to *Setting wrapper types; introduces Clone(), ResolveT, schema-driven validation, and JSON custom marshaling. Logic is sound, though Clone() still returns partially-default data on unmarshal failure (noted in previous thread).
internal/config/settings_schema.go New file defining the Setting struct and SettingsCategory type; clean, well-structured schema definition with appropriate json tags.
internal/tui/helpers.go snapshotSettings upgraded from shallow copy to Clone(); new settingsEqual helper correctly handles float64/int/duration coercion for post-JSON comparison; checkRestartRequirement uses NeedsRestart metadata instead of ui_restart struct tags.
internal/tui/view_settings.go getSettingsValues now extracts raw setting.Value (could be float64 post-JSON); formatSettingValue/formatSettingValueForEdit updated to handle all numeric input types; setSettingValue dispatches on setting.Type string instead of reflect.Kind; resetSettingToDefault replaced large switch with reflection loop using json tags.
internal/tui/settings_unit_test.go Old conversion round-trip tests replaced; TestSetSettingValueConversions directly tests the 4 special-cased conversions via Resolve[T]; TestSettingsMetadataValidation adds schema completeness checks.
internal/config/settings_test.go Tests migrated to use Resolve[T]; filepath.IsAbs check removed from TestGetSettingsPath; time.Duration round-trip not explicitly verified in TestSaveAndLoadSettings despite being a critical JSON conversion path.
internal/tui/settings_reset_test.go setNonDefaultValue updated to work with *Setting.Value type-switch; correctly handles bool/string/int/int64/float64/time.Duration.
cmd/root.go Rename of buildPoolIsNameActive to buildActiveDownloadChecker with doc comment added; Resolve[int] call for MaxConcurrentDownloads in pool construction.
internal/tui/update_settings.go Theme cycling correctly updated to use Resolve[int]/Resolve[string]; all .Value accesses on Setting pointers are correct.
tests/test_analyze.py 90 lines of redundant Python test removed as stated in PR description.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
internal/config/settings_test.go:181-184
The duration round-trip is a critical path: `time.Duration` values serialize to bare nanosecond integers in JSON, which unmarshal back as `float64`, then require `Resolve[time.Duration]` to recover. `TestSaveAndLoadSettings` already sets `SlowWorkerGracePeriod` and `StallTimeout` but never reads them back, leaving the float64→Duration conversion untested by this test.

```suggestion
	if Resolve[int](loaded.Network.MaxConcurrentDownloads) != Resolve[int](original.Network.MaxConcurrentDownloads) {
		t.Error("MaxConcurrentDownloads mismatch")
	}
	if Resolve[time.Duration](loaded.Performance.SlowWorkerGracePeriod) != Resolve[time.Duration](original.Performance.SlowWorkerGracePeriod) {
		t.Errorf("SlowWorkerGracePeriod mismatch: got %v, want %v",
			Resolve[time.Duration](loaded.Performance.SlowWorkerGracePeriod),
			Resolve[time.Duration](original.Performance.SlowWorkerGracePeriod))
	}
	if Resolve[time.Duration](loaded.Performance.StallTimeout) != Resolve[time.Duration](original.Performance.StallTimeout) {
		t.Errorf("StallTimeout mismatch: got %v, want %v",
			Resolve[time.Duration](loaded.Performance.StallTimeout),
			Resolve[time.Duration](original.Performance.StallTimeout))
	}
}
```

### Issue 2 of 2
internal/tui/helpers.go:243-255
**`checkRestartRequirement` misses non-`*Setting` struct fields silently**

`CategorySettings` contains `Categories []Category` alongside `CategoryEnabled *Setting`. When iterating its fields, the type assertion to `*config.Setting` fails for `Categories`, so the field is silently skipped. This is harmless today because `Categories` has no restart semantics, but any future struct field that is NOT a `*Setting` in a category struct will be invisibly excluded from restart-change detection with no compiler warning or runtime error to surface it.

Reviews (4): Last reviewed commit: "refactor: remove deprecated MaxConnectio..." | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

Binary Size Analysis

⚠️ Size Increased

Version Human Readable Raw Bytes
Main 19.93 MB 20902180
PR 19.95 MB 20918564
Difference 16.00 KB 16384

Comment thread internal/config/settings.go
Comment thread internal/tui/settings_unit_test.go
Comment thread cmd/root.go Outdated
@SuperCoolPencil
Copy link
Copy Markdown
Member Author

@greptileai

@SuperCoolPencil SuperCoolPencil merged commit ce294dc into main May 21, 2026
21 of 22 checks passed
@SuperCoolPencil SuperCoolPencil deleted the tui-refactor branch May 21, 2026 07:38
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.

1 participant