@greptileai

This is a comment left during a code review.
Path: internal/tui/colors/colors.go
Line: 195-220

Comment:
Data race: currentPalette read without lock

All color accessor functions (e.g. Pink(), Gray(), Background()) dereference currentPalette without holding modeMu. LoadTheme writes currentPalette under modeMu.Lock(), so any concurrent UI render racing with a theme reload would be a data race flagged by Go's race detector. Each function should take an RLock before reading the pointer, or currentPalette should be read atomically.

The old themeColor.RGBA() was safe because it called IsDarkMode() which itself holds modeMu.RLock(), but the new code bypasses that protection entirely.

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

The cleanest fix is a single helper that takes the read lock, then have all accessor functions use it:

func palette() *Palette {
    modeMu.RLock()
    p := currentPalette
    modeMu.RUnlock()
    return p
}

Then update every accessor to call palette() instead of reading currentPalette directly:

func Background() color.Color { return lipgloss.Color(palette().Primary.Background) }
func Foreground() color.Color { return lipgloss.Color(palette().Primary.Foreground) }
func White() color.Color      { return lipgloss.Color(palette().Normal.White) }
func Gray() color.Color       { return lipgloss.Color(palette().Normal.Black) }
func Pink() color.Color       { return lipgloss.Color(palette().Bright.Red) }
// ... and so on for all accessors
func ProgressStart() color.Color { return lipgloss.Color(palette().Bright.Red) }
func ProgressEnd() color.Color   { return lipgloss.Color(palette().Bright.Magenta) }

This works because reading the pointer under RLock is safe — multiple goroutines can hold the read lock concurrently, and LoadTheme only writes under a full Lock(). The snapshot of the pointer is stable for the duration of each accessor call.

Alternatively, you could use atomic.Pointer[Palette] (Go 1.19+) to avoid the lock entirely in the hot path, but the RLock approach above is simpler given modeMu already exists.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules and .greptile/config.

@greptileai

This is a comment left during a code review.
Path: internal/tui/colors/colors.go
Line: 252-268

Comment:
SetDarkMode no longer updates currentPalette, breaking tests

SetDarkMode changes isDarkMode but does not update currentPalette. Color functions now read directly from currentPalette (initialized to &defaultDark in init()), so calling SetDarkMode(false) leaves currentPalette pointing at the dark palette — Pink() will still return #ff79c6 instead of the expected #d10074.

TestSetDarkMode_UpdatesExportedPalette exercises exactly this path and will fail. SetDarkMode needs to swap currentPalette to the appropriate default palette when no custom theme is active:

func SetDarkMode(isDark bool) {
    modeMu.Lock()
    changed := isDarkMode != isDark
    isDarkMode = isDark
    if isDark {
        currentPalette = &defaultDark
    } else {
        currentPalette = &defaultLight
    }
    modeMu.Unlock()
    // ...
}

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

The fix is to update currentPalette inside the existing modeMu.Lock() block in SetDarkMode. Here's the exact change:

func SetDarkMode(isDark bool) {
	modeMu.Lock()
	changed := isDarkMode != isDark
	isDarkMode = isDark
	if isDark {
		currentPalette = &defaultDark
	} else {
		currentPalette = &defaultLight
	}
	modeMu.Unlock()

	if !changed {
		return
	}

	hookMu.RLock()
	registeredHooks := append([]func(){}, hooks...)
	hookMu.RUnlock()
	for _, fn := range registeredHooks {
		fn()
	}
}

One caveat: this resets to the default palette even when a custom theme is active (e.g., user toggles dark/light mode while a .toml theme is loaded). If that matters, you'd need to track whether a custom path is set and re-call LoadTheme with it instead of falling back to defaultDark/defaultLight. But for fixing the test and the immediate bug, this is sufficient.

Update: CI seems to be happy now.

@SuperCoolPencil I think I have added what I want to the PR.

Some things which potentially can be improved:

• Use of FilePicker (I have no idea how to hook it up to the theme path selection), should default to $XDG_CONFIG_DIR/surge/themes$
• Expand Palette definition to include name, so that default colour schemes + custom ones can be configured/switched to easily.
• DarkGrey/Grey are using the same hex value, because there is not much of a notion of the two being separate in Base16 to Alacritty colour scheme mapping.

On an unrelated note imo there could be docs/info on installation and usage of gotestsum/golangci-lint on a local machine for development purposes. Been wanting to contribute code to this cool project for a while.

Hey @bladeacer

Thanks for the PR!

I'm out of town this weekend but i"ll try to review this asap.

Noticed some merge conflicts, would really appreciate if you could resolve them

Awesomeee

@SuperCoolPencil , I have fixed the merge conflicts.

Want your agent to iterate on Greptile's feedback? Try greploops.

Testing latest changes the theme path file picker does not have a default path, and pressing enter does not select the toml file.

Hey @bladeacer

I'm so sorry for not seeing your comments, I'm just trying to integrate some changes. Yeah, pressing enter doesnt currently select it. Thanks for reporting that. I'll fix it up.

Really good work on the PR, all the changes are well thought out. If I had a few suggestions it would be

• When you want to create a PR, create a new branch in your fork and open a PR from that. Do not use main to create PR's
• I've refactored the structs to use a more robust circular approach:
  The Dark and Light pointers are now moved inside the Palette struct itself.
  ThemeConfig now simply decodes into a single Colors *Palette field.
  This naturally maps the [colors] table to the root palette, while [colors.dark] and [colors.light] are captured as nested fields.
• I've added TestLoadTheme_SingleScheme to colors_test.go which verifies that single-scheme TOML files (like the ones currently provided) correctly override the default colors without needing explicit dark/light blocks.

When you want to create a PR, create a new branch in your fork and open a PR from that. Do not use main to create PR's

Thanks for the head's up :D

I've refactored the structs to use a more robust circular approach:
The Dark and Light pointers are now moved inside the Palette struct itself.
ThemeConfig now simply decodes into a single Colors *Palette field.
This naturally maps the [colors] table to the root palette, while [colors.dark] and [colors.light] are captured as nested fields.

The approach I took to that was assuming the user could either supply a single [colors] with an is_dark boolean for single dark/light colour scheme, while also supporting supplying both [colors.light] and [colors.dark] in the same TOML file (alternate spec).

The explicit light dark block are for the aforementioned purpose, being optional by providing a sane fallback when it is not provided.

@SuperCoolPencil lgtm.

Awesome!

Thank you so much for contributing!

OSS needs more contributors like you :)