Feat/color scheme switcher#60
Merged
Merged
Conversation
AI generated btw
showing [current] is enough
Owner
Author
|
@greptile-ai |
Owner
Author
|
@greptile-ai |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
Introducing colorScheme switcher popup
Changes Made
tui/colorschemeswitcherpackagectrl+tcommandconfig.tomlTesting
filters,output, etc...Screenshots (if applicable)
Checklist
Greptile Summary
This PR adds a color scheme switcher popup (
ctrl+t) that lets users browse base24 schemes, filter by name, and persist the selection toconfig.toml. It also refactors path resolution — home-directory joining now lives exclusively inGetConfigFullPath— and propagatesUpdateThemecalls to all sub-models when a scheme is changed at runtime.internal/render/tui/colorschemeswitcher): new Bubble Tea sub-model with text-input filtering, table display with color swatches, andselectedSchemeID(slug-based) state, backed by a comprehensive test suite.CreateOrReadConfigsnow accepts a full absolute path; callers resolve it viaGetConfigFullPath(), eliminating the previous implicit home-dir prepend.repostable,repodetails, andalertsall gainUpdateThememethods called by the parent on scheme change; the previously-flagged repeated-write bug is fixed by comparing slug IDs on both sides of theschemeID != m.theme.Colors.IDguard.Confidence Score: 4/5
Safe to merge with one display correctness fix: the "[current]" marker in the scheme list compares display names instead of slugs, which can highlight the wrong row when multiple variants of a theme share a display name.
The repeated-config-write bug from the prior review is resolved, path resolution is cleaner, and the tests are thorough. One issue remains in
createRows: the active-scheme indicator usesname == t.Colors.Name(display name) rather thans.id == t.Colors.ID(slug). Base24 scheme collections commonly include dark/light variants of the same scheme name, so this can mark two rows as "[current]" simultaneously, misleading the user about which theme is actually loaded.internal/render/tui/colorschemeswitcher/ui.go — the
createRowsfunction's current-scheme detection logic.Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant TUI as tui.Model participant CSS as colorSchemeSwitcher.Model participant ThemePkg as theme package participant Config as config package User->>TUI: ctrl+t TUI->>TUI: pushFocus(FocusThemeSwitcher) User->>TUI: keypress (nav / filter / enter) TUI->>CSS: Update(msg) CSS-->>TUI: (updatedModel, cmd) alt WantsClose() TUI->>TUI: popFocus(true) else "selectedSchemeID != m.theme.Colors.ID" TUI->>ThemePkg: CreateColors(schemeID) ThemePkg-->>TUI: newColors TUI->>Config: UpdateConfigs(configs, fullPath) Config-->>TUI: err TUI->>TUI: "m.theme = Theme{newColors, CreateStyles}" TUI->>CSS: UpdateTheme(newTheme) TUI->>TUI: reposTable.UpdateTheme / repoDetails.UpdateTheme / alerts.UpdateTheme end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant TUI as tui.Model participant CSS as colorSchemeSwitcher.Model participant ThemePkg as theme package participant Config as config package User->>TUI: ctrl+t TUI->>TUI: pushFocus(FocusThemeSwitcher) User->>TUI: keypress (nav / filter / enter) TUI->>CSS: Update(msg) CSS-->>TUI: (updatedModel, cmd) alt WantsClose() TUI->>TUI: popFocus(true) else "selectedSchemeID != m.theme.Colors.ID" TUI->>ThemePkg: CreateColors(schemeID) ThemePkg-->>TUI: newColors TUI->>Config: UpdateConfigs(configs, fullPath) Config-->>TUI: err TUI->>TUI: "m.theme = Theme{newColors, CreateStyles}" TUI->>CSS: UpdateTheme(newTheme) TUI->>TUI: reposTable.UpdateTheme / repoDetails.UpdateTheme / alerts.UpdateTheme endComments Outside Diff (1)
internal/render/tui/update.go, line 1283 (link)m.colorSchemeSwitcher.SelectedSchemeName()returns the scheme slug (e.g.,"catppuccin-mocha", sourced fromtheme.Schemes), whilem.theme.Colors.Nameholds the display name parsed from the YAMLnamefield (e.g.,"Catppuccin Mocha"). These two strings are never equal, so after a user selects a theme the conditionschemeName != m.theme.Colors.Nameremains permanently true. On every subsequent keypress while the switcher is open,theme.CreateColors,config.DefaultPaths().GetConfigFullPath(), andconfig.UpdateConfigsare all called again, causing repeated config-file writes.The fix is to compare against the same domain: either update
selectedSchemeNameinsideUpdateThemeto the newnewTheme.Colors.Name, or compare againstm.configs.Output.ColorSchemeName(which is already the slug).Reviews (3): Last reviewed commit: "add ID to ColorScheme struct" | Re-trigger Greptile