fix(themes): persist theme into Application Options section#149
fix(themes): persist theme into Application Options section#149
Conversation
patchConfigTheme appended `theme = ...` at EOF when no existing theme line was found. If the config ended with `[color options]` (or any non-default section), the new line landed inside that section and go-flags reported `unknown option: theme` on next startup, discarding the setting. Now the insert skips past any [Application Options] headers and places the entry just before the first non-default section header (or at EOF if none). Also promoted patchConfigTheme to a method on *themeCatalog since it was called only from themeCatalog.Persist. Related to #148
Code-review followup to the #148 fix. The replace-in-place branch of patchConfigTheme was matching the first `theme = ...` line anywhere in the file and rewriting it at the same location. If a user's config was already corrupted by the pre-fix persist path (theme line stuck inside `[color options]`), the new binary kept rewriting at the wrong location and go-flags still reported `unknown option: theme` on next startup. Make the scan section-aware: track the active INI section while walking lines; replace in place only when the found line lives in the default scope ([Application Options] or the unnamed top-of-file section); otherwise delete the stray line and run the insertion path so the new entry lands before the first non-[Application Options] header. Also split the scan and insertion-position logic into two helper methods on *themeCatalog to keep cyclomatic complexity within the linter limit. Add an end-to-end regression test that round-trips patched files through flags.NewIniParser and asserts opts.Theme resolves without error, covering three testdata fixtures (good / no-theme-yet / corrupted). This is the assertion that would have caught #148 originally. Related to #148
There was a problem hiding this comment.
Pull request overview
This PR fixes #148 by ensuring the theme selector persists theme = <name> into the INI default scope / [Application Options] rather than accidentally appending into a trailing named section (e.g. [color options]), which causes go-flags to error with unknown option: theme. It also adds regression tests and fixtures that round-trip patched configs through the actual go-flags INI parser to prevent similar regressions.
Changes:
- Refactors theme persistence to track the active INI section, insert
theme = ...before the first non-[Application Options]section, and “heal” configs corrupted by the old behavior. - Adds targeted unit tests and testdata fixtures, including a parser round-trip assertion using
flags.NewIniParser. - Minor formatting/indentation cleanups in unrelated tests.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/themes.go | Moves config patching onto themeCatalog and updates insertion logic to keep theme in the default/Application Options scope. |
| app/themes_test.go | Adds regression tests and parser round-trip coverage for the fixed persistence behavior. |
| app/testdata/themes/good.ini | Fixture representing a valid config with theme in [Application Options]. |
| app/testdata/themes/no_theme.ini | Fixture representing a config missing a theme with a trailing named section. |
| app/testdata/themes/corrupted.ini | Fixture representing a previously-corrupted config with theme inside a named section. |
| app/ui/diffnav_test.go | Minor indentation/line-number alignment in an existing test. |
| app/diff/diff_test.go | Minor struct field alignment in an existing test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := os.MkdirAll(filepath.Dir(tc.configPath), 0o750); err != nil { | ||
| return fmt.Errorf("creating config dir: %w", err) | ||
| } | ||
|
|
||
| data, err := os.ReadFile(configPath) //nolint:gosec // path from user's config | ||
| data, err := os.ReadFile(tc.configPath) | ||
| if err != nil { |
There was a problem hiding this comment.
os.ReadFile(tc.configPath) will likely trigger gosec G304 (variable file path). The previous implementation had an explicit //nolint:gosec with an explanation; consider restoring an equivalent suppression (or refactoring) to keep golangci-lint green for user-provided config paths.
| key, _, ok := strings.Cut(trimmed, "=") | ||
| if !ok { | ||
| if !ok || strings.TrimSpace(key) != "theme" { | ||
| continue | ||
| } | ||
| if strings.TrimSpace(key) == "theme" { | ||
| lines[i] = "theme = " + themeName | ||
| found = true | ||
| break | ||
| } | ||
| return i, currentSection == "" || strings.EqualFold(currentSection, "Application Options") |
There was a problem hiding this comment.
scanThemeLine returns on the first theme = ... it finds. If a config contains a valid theme in the default/Application Options scope and a stray theme line later inside a named section (possible if a user manually repaired a previously-corrupted file without removing the stray line), this code will update the first occurrence and leave the later stray line behind, so go-flags can still fail with an "unknown option: theme" error. Consider scanning the entire file and removing any theme entries outside the default scope (and/or deduplicating to a single canonical entry).
| t.Run(tc.name, func(t *testing.T) { | ||
| raw, err := os.ReadFile(filepath.Join("testdata", "themes", tc.fixture)) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
This fixture read (os.ReadFile(filepath.Join("testdata", ...))) may trigger gosec G304 (variable file path). Other test fixture reads in the repo annotate these with //nolint:gosec + explanation; consider adding the same here to avoid lint failures.
Addresses Copilot review finding on #149. The previous heal logic found the first `theme = ...` occurrence and stopped. When a user hand-repaired a config corrupted by the pre-fix bug by adding a valid `theme = ...` in [Application Options] without deleting the stray inside [color options], the patch updated the valid line in place and left the stray, so go-flags kept erroring with `unknown option: theme` on next startup. scanThemeLine -> scanThemeLines now walks the full file and reports the first default-scope index separately from every stray. The patch loop deletes strays in reverse order (to keep earlier indices stable) before replacing/inserting the canonical entry. Add TestPatchConfigTheme_removesStrayDuplicates regression test and extend the testdata round-trip table with duplicate.ini, a fixture shaped like the hand-repair scenario, to assert the file parses cleanly via flags.NewIniParser after the heal. Related to #148
Deploying revdiff with
|
| Latest commit: |
c38b6e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://26e49bf2.revdiff.pages.dev |
| Branch Preview URL: | https://fix-theme-persist-ini-sectio.revdiff.pages.dev |
Fixes the #148 bug where the theme selector (Shift+T) was writing
theme = <name>at EOF of~/.config/revdiff/config. If the file ended with a named section such as[color options], the new line landed inside that section and go-flags emittedunknown option: themeon next startup, silently dropping the setting.What changed
patchConfigThemenow scans with active-section tracking. When notheme =line exists, or when a stray one sits inside a wrong section (configs already corrupted by the pre-fix persist path), the new line is inserted just before the first non-[Application Options]section header so the INI parser attributes it to the default scope.patchConfigThemeto a method on*themeCatalog(it was called only fromthemeCatalog.Persist). Scan + insert-position logic split into two small helper methods for readability.Tests
[application options]header, and blank-line collapsing.flags.NewIniParserand assertsopts.Themeresolves without error across three fixtures:good.ini,no_theme.ini,corrupted.ini. This is the assertion that would have caught theme selector saves theme into wrong INI section #148 originally.Related to #148