Conversation
📝 WalkthroughWalkthroughAdds a new system-tray application and supporting package 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tray/paths_darwin.go">
<violation number="1" location="tray/paths_darwin.go:25">
P2: Ignoring the error from os.UserHomeDir can yield an empty home path and cause config/logs to be written to a relative directory. Handle the error explicitly to avoid misdirected writes.</violation>
</file>
<file name="tray/paths_test.go">
<violation number="1" location="tray/paths_test.go:37">
P2: The substring check is case-sensitive. On macOS/Windows the implementation uses "Adder" with a capital A, so this assertion will fail on those platforms. Use a case-insensitive comparison (or normalize the path) before checking for the project name.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tray/paths_darwin.go
Outdated
| ) | ||
|
|
||
| func configDir() string { | ||
| home, _ := os.UserHomeDir() |
There was a problem hiding this comment.
P2: Ignoring the error from os.UserHomeDir can yield an empty home path and cause config/logs to be written to a relative directory. Handle the error explicitly to avoid misdirected writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/paths_darwin.go, line 25:
<comment>Ignoring the error from os.UserHomeDir can yield an empty home path and cause config/logs to be written to a relative directory. Handle the error explicitly to avoid misdirected writes.</comment>
<file context>
@@ -0,0 +1,32 @@
+)
+
+func configDir() string {
+ home, _ := os.UserHomeDir()
+ return filepath.Join(home, "Library", "Application Support", "Adder")
+}
</file context>
tray/paths_test.go
Outdated
| assert.Contains( | ||
| t, | ||
| dir, | ||
| "adder", |
There was a problem hiding this comment.
P2: The substring check is case-sensitive. On macOS/Windows the implementation uses "Adder" with a capital A, so this assertion will fail on those platforms. Use a case-insensitive comparison (or normalize the path) before checking for the project name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/paths_test.go, line 37:
<comment>The substring check is case-sensitive. On macOS/Windows the implementation uses "Adder" with a capital A, so this assertion will fail on those platforms. Use a case-insensitive comparison (or normalize the path) before checking for the project name.</comment>
<file context>
@@ -0,0 +1,73 @@
+ assert.Contains(
+ t,
+ dir,
+ "adder",
+ "ConfigDir should contain 'adder' in the path",
+ )
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 42-49: The default build target is picking up adder-tray from
BINARIES (auto-discovered under cmd/) and building it with incompatible flags
(CGO_ENABLED=0 and -tags nodbus), causing broken tray builds; update BINARIES to
exclude the adder-tray directory (so the default build target won't build it)
and leave the specialized build-tray target (function/target: build-tray, binary
name: adder-tray) to handle platform-specific CGO/flags, e.g., adjust the
BINARIES definition to filter out "adder-tray" so only build-tray produces that
binary with the correct CGO and tag settings.
In `@tray/config_test.go`:
- Around line 49-59: Tests like
TestLoadConfigMissing/TestSaveAndLoadConfig/TestConfigExists rely on
XDG_CONFIG_HOME which only affects Linux paths; to make them platform-agnostic
add a test-time override for the config directory (e.g., function
setConfigDirOverride(path string) and clearConfigDirOverride()) and update the
path resolution used by LoadConfig, SaveConfig and ConfigExists to consult that
override before falling back to os.UserHomeDir()/platform-specific logic; then
call setConfigDirOverride(tmpDir) at the start of each test (and clear it after)
so tests run safely on macOS/Windows/Linux without modifying platform env vars.
In `@tray/paths_linux.go`:
- Around line 24-30: The configDir() function (and its counterpart in
paths_darwin.go) currently ignores the error from os.UserHomeDir(), which can
yield an empty string and produce a relative path; update configDir to return
(string, error) instead of string, check and propagate the os.UserHomeDir()
error (or if you prefer a safe fallback use a well-known absolute path like
"/tmp/adder" explicitly), and update callers such as SaveConfig to handle the
error return and avoid creating files relative to CWD; ensure the new signature
is applied to both configDir and any internal callers to keep behavior
consistent.
In `@tray/paths_test.go`:
- Around line 34-39: The tests TestConfigDir and TestLogDir use
assert.Contains(t, dir, "adder") which is case-sensitive and fails on Windows
where the path contains "Adder"; change the assertions to perform a
case-insensitive containment check (e.g. assert.True(t,
strings.Contains(strings.ToLower(dir), "adder"), ...) and update the error
message accordingly, and add the strings import if missing.
In `@tray/paths_windows.go`:
- Around line 28-29: The code silently ignores os.UserHomeDir() errors (used
with filepath.Join) which can produce a relative path; change the helper to
check the error from os.UserHomeDir() and handle it explicitly: if
os.UserHomeDir() returns an error, try using os.Getenv("APPDATA") as a fallback
(and verify it's non-empty), otherwise return an error (i.e., change the helper
signatures to return (string, error)) or at minimum log a warning before
returning; apply the same fix to logDir(), update callers to handle the new
error return or fallback behavior, and ensure any returned path is absolute (use
filepath.Abs if needed).
In `@tray/process.go`:
- Around line 128-133: Restart() races because Stop() only signals the process
and returns before the background goroutine clears pm.cmd, so Start() often sees
pm.cmd non-nil and errors; fix by making Stop() block until the process actually
exits (call pm.cmd.Wait() while holding appropriate locks or wait on an
existing/added exit/done channel that the background goroutine closes) so pm.cmd
is cleared before Stop() returns, or alternatively change Restart() to
wait/retry until pm.cmd is nil after calling Stop() (use a short loop with
timeout). Update the implementations of Stop(), Start(), and Restart() (and the
pm.cmd/exit signaling mechanism) to coordinate via the chosen wait strategy to
avoid the race.
- Around line 120-122: The Stop() logic uses pm.cmd.Process.Signal(os.Interrupt)
which is not implemented on Windows; update Stop() to detect Windows
(runtime.GOOS == "windows") or handle the specific error from
pm.cmd.Process.Signal and call pm.cmd.Process.Kill() on Windows (or as a
fallback) instead of returning an error; modify the code paths around
pm.cmd.Process.Signal, pm.cmd.Process.Kill, and Stop() so Windows builds use
Kill() for termination while other platforms continue to use
Signal(os.Interrupt).
🧹 Nitpick comments (5)
cmd/adder-tray/main.go (1)
37-41: Consider handling a second signal for forceful exit.Currently the goroutine exits after consuming the first signal. If
application.Shutdown()orRun()hangs, there's no way for the user to force-quit with a second Ctrl+C. A common pattern is to loop andos.Exit(1)on the second signal.💡 Optional: force-exit on second signal
go func() { <-sigChan slog.Info("received shutdown signal") application.Shutdown() + <-sigChan + slog.Warn("received second signal, forcing exit") + os.Exit(1) }()tray/config_test.go (1)
61-81: Verify round-trip preserves zero-value fields.The test uses all non-zero values. Consider adding a case where
AdderConfigis""andAutoStartisfalseto ensure YAML round-tripping preserves zero values correctly (some YAML libraries omit zero-value fields by default unlessomitemptyis absent).tray/paths_test.go (1)
58-72: Platform test for Windows is weak —"Adder"also appears on Linux/macOS paths.The Windows branch only checks that the path contains
"Adder", which is true for all platforms. Consider asserting a Windows-specific marker like"AppData"to validate the platform-appropriate path.tray/app.go (1)
31-50:NewAppnever returns a non-nil error.The current implementation always returns
nilfor the error. If this is intentional scaffolding for future error paths, that's fine — but consider documenting it or simplifying the signature to just*Appuntil an error path actually exists.tray/process.go (1)
94-104: Background goroutine holds the lock while the mainStoppath also needs it.This is fine for the current flow since
Stop()returns beforeWait()completes, and the goroutine acquires the lock after. However, note that ifStop()were ever changed to wait for exit while holding the lock, this would deadlock. The current design is correct but fragile — worth a comment noting the lock ordering.
| func TestLoadConfigMissing(t *testing.T) { | ||
| // When no config file exists, LoadConfig should return defaults | ||
| // without error. We set XDG_CONFIG_HOME to a temp dir so we | ||
| // don't accidentally read a real config. | ||
| tmpDir := t.TempDir() | ||
| t.Setenv("XDG_CONFIG_HOME", tmpDir) | ||
|
|
||
| cfg, err := LoadConfig() | ||
| require.NoError(t, err) | ||
| assert.Equal(t, DefaultConfig(), cfg) | ||
| } |
There was a problem hiding this comment.
Test isolation via XDG_CONFIG_HOME only works on Linux.
t.Setenv("XDG_CONFIG_HOME", tmpDir) redirects config paths on Linux, but paths_darwin.go and paths_windows.go ignore this variable entirely — they use os.UserHomeDir() and platform-specific paths. On macOS/Windows, these tests will read from and write to the developer's real config directory, which is both fragile and potentially destructive.
Consider one of:
- Adding a
//go:build linuxconstraint to this test file (limits coverage). - Introducing an internal override/setter (e.g.,
setConfigDirOverride(path)) for test-time path redirection on all platforms. - Using
t.Setenv("HOME", tmpDir)on Darwin (andAPPDATA/LOCALAPPDATAon Windows) to redirectos.UserHomeDir()— though this is more brittle.
The same concern applies to TestSaveAndLoadConfig and TestConfigExists.
🤖 Prompt for AI Agents
In `@tray/config_test.go` around lines 49 - 59, Tests like
TestLoadConfigMissing/TestSaveAndLoadConfig/TestConfigExists rely on
XDG_CONFIG_HOME which only affects Linux paths; to make them platform-agnostic
add a test-time override for the config directory (e.g., function
setConfigDirOverride(path string) and clearConfigDirOverride()) and update the
path resolution used by LoadConfig, SaveConfig and ConfigExists to consult that
override before falling back to os.UserHomeDir()/platform-specific logic; then
call setConfigDirOverride(tmpDir) at the start of each test (and clear it after)
so tests run safely on macOS/Windows/Linux without modifying platform env vars.
wolf31o2
force-pushed
the
feature/tray-scaffolding
branch
from
76b62fd to
45a1cb0
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tray/process.go`:
- Around line 114-144: The Stop method may attempt to signal a process that
already exited; update ProcessManager.Stop to detect that case and treat it as
success: before sending a signal, check pm.cmd.ProcessState (via
pm.cmd.ProcessState != nil && pm.cmd.ProcessState.Exited()) and return nil if
the process is already finished; additionally, when Signal or Kill returns an
error, treat "process already finished" (or platform-specific equivalent) as a
no-op instead of returning an error (e.g., inspect the error string from
pm.cmd.Process.Signal / pm.cmd.Process.Kill and swallow the already-finished
error). Ensure you still unlock pm.mu in all paths and keep using pm.done to
wait when appropriate.
🧹 Nitpick comments (2)
tray/process.go (1)
114-144:Stop()has repetitive unlock-and-return paths — considerdeferwith a flag or restructuring.The manual
pm.mu.Unlock()calls on multiple error branches are fragile and easy to get wrong during future edits. Consider restructuring to reduce duplication, e.g., unlock before signaling (thedonechannel already ensures coordination), or capture the error and defer the unlock.Suggested restructuring
func (pm *ProcessManager) Stop() error { pm.mu.Lock() if pm.cmd == nil || pm.cmd.Process == nil { pm.mu.Unlock() return nil } slog.Info("stopping adder process", "pid", pm.cmd.Process.Pid) done := pm.done + proc := pm.cmd.Process + pm.mu.Unlock() + var err error if runtime.GOOS == "windows" { - // os.Interrupt is not supported on Windows - if err := pm.cmd.Process.Kill(); err != nil { - pm.mu.Unlock() - return fmt.Errorf("killing adder: %w", err) - } + err = proc.Kill() } else { - if err := pm.cmd.Process.Signal(os.Interrupt); err != nil { - pm.mu.Unlock() - return fmt.Errorf("sending interrupt to adder: %w", err) - } + err = proc.Signal(os.Interrupt) } - pm.mu.Unlock() + if err != nil && !errors.Is(err, os.ErrProcessDone) { + return fmt.Errorf("stopping adder: %w", err) + } - // Wait for the process to fully exit <-done - return nil }tray/app.go (1)
31-49:NewAppnever returns a non-nil error.
NewAppcurrently always returns(app, nil). The error return is unused since config loading failures are handled with a fallback. If this is intentional scaffolding for future validation, it's fine to keep — just noting the current state.
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tray/process.go">
<violation number="1" location="tray/process.go:142">
P2: Stop() can block forever if the adder process doesn't exit after SIGINT. Add a timeout and force-kill fallback so the tray UI can't hang indefinitely.</violation>
</file>
<file name="tray/paths_test.go">
<violation number="1" location="tray/paths_test.go:63">
P2: The linux test assumes .config/.local are always present, but the implementation respects XDG_CONFIG_HOME/XDG_STATE_HOME. This makes the test environment-dependent and can fail when those env vars are set. Consider asserting against the env-provided path when set, and falling back to the default paths otherwise.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tray/process.go
Outdated
| pm.mu.Unlock() | ||
|
|
||
| // Wait for the process to fully exit | ||
| <-done |
There was a problem hiding this comment.
P2: Stop() can block forever if the adder process doesn't exit after SIGINT. Add a timeout and force-kill fallback so the tray UI can't hang indefinitely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/process.go, line 142:
<comment>Stop() can block forever if the adder process doesn't exit after SIGINT. Add a timeout and force-kill fallback so the tray UI can't hang indefinitely.</comment>
<file context>
@@ -0,0 +1,161 @@
+ pm.mu.Unlock()
+
+ // Wait for the process to fully exit
+ <-done
+
+ return nil
</file context>
tray/paths_test.go
Outdated
|
|
||
| switch runtime.GOOS { | ||
| case "linux": | ||
| assert.Contains(t, configDir, ".config") |
There was a problem hiding this comment.
P2: The linux test assumes .config/.local are always present, but the implementation respects XDG_CONFIG_HOME/XDG_STATE_HOME. This makes the test environment-dependent and can fail when those env vars are set. Consider asserting against the env-provided path when set, and falling back to the default paths otherwise.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/paths_test.go, line 63:
<comment>The linux test assumes .config/.local are always present, but the implementation respects XDG_CONFIG_HOME/XDG_STATE_HOME. This makes the test environment-dependent and can fail when those env vars are set. Consider asserting against the env-provided path when set, and falling back to the default paths otherwise.</comment>
<file context>
@@ -0,0 +1,72 @@
+
+ switch runtime.GOOS {
+ case "linux":
+ assert.Contains(t, configDir, ".config")
+ assert.Contains(t, logDir, ".local")
+ case "darwin":
</file context>
45a1cb0 to
b007ca9
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 14 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tray/process.go">
<violation number="1" location="tray/process.go:162">
P2: Ignoring the force-kill error can leave Stop() waiting on `done` forever if the process could not be killed. Handle the error and return instead of unconditionally blocking.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tray/process.go
Outdated
| slog.Warn("adder process did not exit in time, force killing") | ||
| pm.mu.Lock() | ||
| if pm.cmd != nil && pm.cmd.Process != nil { | ||
| _ = pm.cmd.Process.Kill() |
There was a problem hiding this comment.
P2: Ignoring the force-kill error can leave Stop() waiting on done forever if the process could not be killed. Handle the error and return instead of unconditionally blocking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tray/process.go, line 162:
<comment>Ignoring the force-kill error can leave Stop() waiting on `done` forever if the process could not be killed. Handle the error and return instead of unconditionally blocking.</comment>
<file context>
@@ -0,0 +1,185 @@
+ slog.Warn("adder process did not exit in time, force killing")
+ pm.mu.Lock()
+ if pm.cmd != nil && pm.cmd.Process != nil {
+ _ = pm.cmd.Process.Kill()
+ }
+ pm.mu.Unlock()
</file context>
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feature/tray-scaffolding
branch
from
b007ca9 to
f492948
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
2 participants
Summary by cubic
Add a cross-platform system tray app (adder-tray) to start, stop, and restart the adder process. Supports auto-start and graceful shutdown, with config, platform paths, a process manager, tests, and a dedicated build target.
New Features
Migration
Written for commit f492948. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Chores