docs(readme): behavior accuracy, dry-run note, config vs registry locking#5
docs(readme): behavior accuracy, dry-run note, config vs registry locking#5bschellenberger2600 wants to merge 14 commits intomainfrom
Conversation
Replace the previous default mainline-only remote fetch with git fetch --all --prune per repository (honoring sync_tags). Introduce --mainline-fetch to opt into the old MainlineFetchRemotes behavior. Remove the redundant --fetch-only flag and update docs, UAT, and tests. Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
README and CLAUDE describe default full fetch, --fetch-mainline, --sync with branch_mode (including all-branches), and --risky on sync. Rename CLI flag from --mainline-fetch to --fetch-mainline and fix variable naming; update UAT labels and test messages. Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
Default fetch is git fetch --all without --prune. Add global fetch_prune, registry fetch_prune, local rain.fetchprune, and --prune/--no-prune flags (CLI wins). Wire RainOptions.FetchPrune through fetchOnly, MainlineFetchRemotes, and RainRepository. Document precedence and pruning risks in README with a warning callout. Add unit tests for resolution order and ReadRainFetchPrune. Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
Dry-run now prints resolved Fetch --prune for --sync as well as fetch paths. Registry Upsert for-loop indentation matches other methods after gofmt.
…guard - applyRepoFetchPrune: skip git config read when CLI sets prune; warn and fall back on ReadRainFetchPrune errors - Reject --fetch-mainline with full-sync triggers; add test - runDryRun: show fetch --prune resolution for --sync path too - Fix Upsert for-loop indentation (gofmt) - UAT scenario 7 echo matches prune-opt-in default - CLAUDE/README: prune precedence and --fetch-mainline note - Replace misleading frozen label (fetch --all) Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
- Resolve conflicts in cmd/root.go and cmd/root_test.go (keep fetch-mainline note and dry-run prune test). - SaveConfig and WriteExampleConfig: exclusive flock on config.toml.lock, PID-scoped temp file, atomic rename (avoids corruption / stale .tmp). - Add github.com/gofrs/flock; concurrent SaveConfig test. Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
Match full-branch-hydration triggers to computeFullSync, fix default_mode example, document prune precedence and backup ref shape. Add a dry-run pun and a durability note contrasting repos.toml locking with config.toml atomic writes and remaining race scenarios.
📝 WalkthroughWalkthroughDocumentation clarified fetch/sync/prune/risky semantics and TUI behavior; config defaults and validation for Changes
Sequence Diagram(s)sequenceDiagram
participant TUI as "TUI (picker)"
participant Scanner as "Scanner (filesystem/git)"
participant TUIChan as "tuiRepoChan (buffered)"
participant CancelCtx as "Scan cancel context"
Note over TUI,Scanner: Start streaming scan
TUI->>Scanner: Start ScanRepositoriesStream(ctx)
Scanner-->>TUIChan: send Repo items (async)
TUIChan-->>TUI: receive Repo items (render)
alt User selects repo / exits
TUI->>CancelCtx: cancelScan() /* cancel before draining */
CancelCtx-->>Scanner: context cancelled
Scanner--xTUIChan: stop sending / close channel
TUI->>TUIChan: drain remaining items
TUI->>TUI: shutdown UI and return
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
- Example config and TUI config screen use sync-default/sync-all/ sync-current-branch/leave-untouched (matches ParseMode). - Validate global.default_mode strictly; empty normalizes to sync-default. - Trim git-fire mirroring from CLI flag text and README; tighten CLAUDE.md.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
244-244:⚠️ Potential issue | 🟡 MinorResolve the
--branch-modetrigger contradiction.Line 244 says
--branch-modeapplies “With--sync”, but Line 28 and Line 305 say any CLI--branch-modetriggers full branch hydration even without--sync. These statements conflict and will confuse users.Suggested doc fix
-| `--branch-mode` | With `--sync`: `mainline`, `checked-out`, `all-local`, or `all-branches` (overrides config for this run) | +| `--branch-mode` | Branch selection mode: `mainline`, `checked-out`, `all-local`, or `all-branches`. On CLI, providing this flag enters the full branch-hydration path for the run (overrides config). |Based on learnings: Use
RainRepositoryfor local branch hydration when--sync, non-mainlinebranch_mode, or risky-only config forces full sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 244, Update the README to remove the contradictory wording that `--branch-mode` only applies "With `--sync`" and clearly document the actual behavior: state that supplying the CLI flag `--branch-mode` triggers full branch hydration even when `--sync` is not provided, and additionally explain that when `--sync` is used the system uses `RainRepository` for local branch hydration and that non-mainline `branch_mode` or a risky-only config also forces full sync; reference the flags `--branch-mode` and `--sync`, the config key `branch_mode`, and the class `RainRepository` so readers can map the behavior to the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 240: Update the README entry for `--dry-run` to clarify its behavior when
combined with `--no-scan`: mention that `--dry-run` normally skips git
fetch/branch updates but still scans the disk to list repos, and explicitly add
a caveat that if `--no-scan` is also provided, the disk scan is skipped and
`--dry-run` will not list repos from disk; reference the `--dry-run` and
`--no-scan` flags so readers understand the interaction.
---
Outside diff comments:
In `@README.md`:
- Line 244: Update the README to remove the contradictory wording that
`--branch-mode` only applies "With `--sync`" and clearly document the actual
behavior: state that supplying the CLI flag `--branch-mode` triggers full branch
hydration even when `--sync` is not provided, and additionally explain that when
`--sync` is used the system uses `RainRepository` for local branch hydration and
that non-mainline `branch_mode` or a risky-only config also forces full sync;
reference the flags `--branch-mode` and `--sync`, the config key `branch_mode`,
and the class `RainRepository` so readers can map the behavior to the
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| | Flag | Description | | ||
| |---|---| | ||
| | `--dry-run` | Show what would run without making changes | | ||
| | `--dry-run` | No `git fetch` / branch updates — still scans disk to list repos. The name is weather-themed irony: no “wet” git work, but not a no-op. | |
There was a problem hiding this comment.
Clarify --dry-run behavior with --no-scan.
The current text says dry-run “still scans disk,” which is not universally true if users also pass --no-scan. A small caveat would make this precise.
Suggested doc fix
-| `--dry-run` | No `git fetch` / branch updates — still scans disk to list repos. The name is weather-themed irony: no “wet” git work, but not a no-op. |
+| `--dry-run` | No `git fetch` / branch updates — still scans disk to list repos (unless `--no-scan` is set). The name is weather-themed irony: no “wet” git work, but not a no-op. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `--dry-run` | No `git fetch` / branch updates — still scans disk to list repos. The name is weather-themed irony: no “wet” git work, but not a no-op. | | |
| | `--dry-run` | No `git fetch` / branch updates — still scans disk to list repos (unless `--no-scan` is set). The name is weather-themed irony: no "wet" git work, but not a no-op. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 240, Update the README entry for `--dry-run` to clarify
its behavior when combined with `--no-scan`: mention that `--dry-run` normally
skips git fetch/branch updates but still scans the disk to list repos, and
explicitly add a caveat that if `--no-scan` is also provided, the disk scan is
skipped and `--dry-run` will not list repos from disk; reference the `--dry-run`
and `--no-scan` flags so readers understand the interaction.
…into docs/readme-dry-run-durability-locks
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/loader.go (1)
118-125:⚠️ Potential issue | 🟡 MinorCleanup stale tmp on
os.WriteFilefailure too.If
os.WriteFilefails mid-write (e.g. ENOSPC, permissions), a partialpath.<pid>.tmpcan be left behind — cleanup only happens onRenamefailure. Mirror the sameos.Remove(tmp)on the WriteFile error path for symmetry and to avoid littered temp files across runs.Proposed fix
tmp := fmt.Sprintf("%s.%d.tmp", path, os.Getpid()) if err := os.WriteFile(tmp, data, 0o600); err != nil { + _ = os.Remove(tmp) return fmt.Errorf("writing temp config: %w", err) }Apply the same to
WriteExampleConfigat line 228.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/loader.go` around lines 118 - 125, The temp file created as tmp := fmt.Sprintf("%s.%d.tmp", path, os.Getpid()) is not removed if os.WriteFile(tmp, data, 0o600) fails, leaving stale tmp files; update the error path in the block that writes and renames the temp file to call os.Remove(tmp) before returning when os.WriteFile fails, and apply the same cleanup change to the analogous WriteExampleConfig function so both write-failure and rename-failure paths remove the temporary file (reference tmp, path, os.WriteFile, os.Rename, and WriteExampleConfig to locate the spots to change).
🧹 Nitpick comments (1)
internal/config/loader.go (1)
215-235: Duplication withSaveConfig— consider extracting.
WriteExampleConfigis now structurally identical toSaveConfig(lock → write tmp → rename), differing only in payload source and error-message wording. Worth extracting a smallatomicWrite(path string, data []byte) errorhelper to keep the lock/tmp/rename invariants in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/loader.go` around lines 215 - 235, WriteExampleConfig duplicates SaveConfig's lock/tmp/rename logic; extract that shared behavior into a new helper atomicWrite(path string, data []byte) error that acquires the same file lock (flock.New(path+".lock")), writes to a temp file (fmt.Sprintf("%s.%d.tmp", path, os.Getpid())), renames it into place, and cleans up on error; then update SaveConfig and WriteExampleConfig to call atomicWrite with their respective payloads (e.g., ExampleConfigTOML() output) and preserve existing error wrapping and permissions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/loader.go`:
- Around line 108-112: Replace the blocking flock.Lock() call in SaveConfig with
a non-blocking TryLockContext: create a context with a short timeout (e.g.,
500ms) and call lock.TryLockContext(ctx, retryDelay) instead of lock.Lock(); on
timeout or error propagate the error through the existing configSaveErr
mechanism so the TUI can surface the failure; ensure you still defer unlocking
(lock.Unlock()) when TryLockContext succeeds and keep using flock.New(path +
".lock") as before.
---
Outside diff comments:
In `@internal/config/loader.go`:
- Around line 118-125: The temp file created as tmp := fmt.Sprintf("%s.%d.tmp",
path, os.Getpid()) is not removed if os.WriteFile(tmp, data, 0o600) fails,
leaving stale tmp files; update the error path in the block that writes and
renames the temp file to call os.Remove(tmp) before returning when os.WriteFile
fails, and apply the same cleanup change to the analogous WriteExampleConfig
function so both write-failure and rename-failure paths remove the temporary
file (reference tmp, path, os.WriteFile, os.Rename, and WriteExampleConfig to
locate the spots to change).
---
Nitpick comments:
In `@internal/config/loader.go`:
- Around line 215-235: WriteExampleConfig duplicates SaveConfig's
lock/tmp/rename logic; extract that shared behavior into a new helper
atomicWrite(path string, data []byte) error that acquires the same file lock
(flock.New(path+".lock")), writes to a temp file (fmt.Sprintf("%s.%d.tmp", path,
os.Getpid())), renames it into place, and cleans up on error; then update
SaveConfig and WriteExampleConfig to call atomicWrite with their respective
payloads (e.g., ExampleConfigTOML() output) and preserve existing error wrapping
and permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95376c1a-20e6-4f86-b9f1-7add88bf9878
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
CLAUDE.mdREADME.mdcmd/root.gocmd/root_test.gogo.modinternal/config/config_test.gointernal/config/defaults.gointernal/config/loader.gointernal/registry/types.gointernal/ui/config_view.go
✅ Files skipped from review due to trivial changes (6)
- go.mod
- internal/registry/types.go
- CLAUDE.md
- cmd/root.go
- internal/config/defaults.go
- internal/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| lock := flock.New(path + ".lock") | ||
| if err := lock.Lock(); err != nil { | ||
| return fmt.Errorf("config file lock: %w", err) | ||
| } | ||
| defer func() { _ = lock.Unlock() }() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
gofrs/flock TryLockContext usage and default retry interval
💡 Result:
TryLockContext in gofrs/flock is a method on Flock that attempts to acquire an exclusive file lock with retries. It takes a context.Context for cancellation/timeout and a time.Duration for the retryDelay between attempts. Signature: func (f Flock) TryLockContext(ctx context.Context, retryDelay time.Duration) (bool, error) Behavior: It repeatedly calls f.TryLock in a loop until: - TryLock succeeds (returns true, locked) - TryLock fails with an error - The context is done (cancelled or timed out) Between attempts, it waits using time.After(retryDelay), or returns immediately if ctx.Err != nil at start. There is no fixed "default retry interval". The retryDelay must be provided by the caller; it has no predefined default value. Examples use 678time.Millisecond, but any time.Duration can be passed (including 0 for no delay). Usage example from official example: f := flock.New(filepath.Join(os.TempDir, "go-lock.lock")) lockCtx, cancel := context.WithTimeout(context.Background, 30time.Second) defer cancel locked, err := f.TryLockContext(lockCtx, 678*time.Millisecond) if err != nil { // handle error } if locked { fmt.Printf("path: %s; locked: %v\n", f.Path, f.Locked) f.Unlock } Source code confirms the implementation uses the provided retryDelay directly in time.After(retryDelay). This is from the latest version v0.13.0 (2025-10-09), actively maintained as of 2026.
Citations:
- 1: https://github.com/gofrs/flock/blob/main/flock_example_test.go
- 2: https://pkg.go.dev/github.com/gofrs/flock
- 3: https://pkg.go.dev/github.com/gofrs/flock@v0.13.0
- 4: https://github.com/gofrs/flock
- 5: https://pkg.go.dev/github.com/deepin-community/golang-github-gofrs-flock
- 6: https://github.com/gofrs/flock/blob/main/flock.go
- 7: https://github.com/gofrs/flock/blob/v0.13.0/flock.go
- 8: https://godoc.org/github.com/gofrs/flock
🏁 Script executed:
# Check if SaveConfig is invoked on keypresses in config_view.go
fd "config_view.go" --type fRepository: git-fire/git-rain
Length of output: 87
🏁 Script executed:
# Check the SaveConfig implementation in loader.go
cat -n internal/config/loader.go | sed -n '100,120p'Repository: git-fire/git-rain
Length of output: 1060
🏁 Script executed:
# Search for SaveConfig calls with context
rg "SaveConfig" --type go -B 2 -A 2Repository: git-fire/git-rain
Length of output: 2488
🏁 Script executed:
# Check lines around 218, 226, 237 in config_view.go to verify keypress handling
wc -l internal/ui/config_view.goRepository: git-fire/git-rain
Length of output: 91
🏁 Script executed:
# Read the relevant section from config_view.go
cat -n internal/ui/config_view.go | sed -n '210,245p'Repository: git-fire/git-rain
Length of output: 1071
Use TryLockContext with timeout in SaveConfig to prevent UI freezes on contended or stale locks.
SaveConfig is invoked synchronously from the TUI event loop on space/→/← keypresses (lines 218, 226). flock.Lock() blocks indefinitely, so a stale config.toml.lock, an NFS/remote-FS lock holder, or a concurrently-running git-rain CLI invocation will hang the UI with no feedback. Replace lock.Lock() with lock.TryLockContext(ctx, retryDelay) using a context with short timeout (e.g., 500ms) and surface errors via the existing configSaveErr mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/loader.go` around lines 108 - 112, Replace the blocking
flock.Lock() call in SaveConfig with a non-blocking TryLockContext: create a
context with a short timeout (e.g., 500ms) and call lock.TryLockContext(ctx,
retryDelay) instead of lock.Lock(); on timeout or error propagate the error
through the existing configSaveErr mechanism so the TUI can surface the failure;
ensure you still defer unlocking (lock.Unlock()) when TryLockContext succeeds
and keep using flock.New(path + ".lock") as before.
Add PanelBlockWidth/PanelTextWidth/RainDisplayWidth so list, settings, rain, and PathWidthFor share one layout budget (outer margin 6 + box padding 4). Clamp list/settings rows with lipgloss; align lipgloss padding in tests. Treat tea.InterruptMsg like quit; map tea.ErrInterrupted to ErrCancelled. Co-authored-by: bschellenberger2600 <bschellenberger2600@gmail.com>
…lysis Cancel scan context immediately after the TUI returns so filepath walk and git subprocesses unwind. Buffer tuiRepoChan to reduce bridge backpressure. Use exec.CommandContext in ScanRepositoriesStream analysis; add test that a pre-cancelled scan still closes the output channel. Co-authored-by: bschellenberger2600 <bschellenberger2600@gmail.com>
Document q/ctrl+c and scan cancellation in README; point maintainers at panel_layout and runRainTUIStream shutdown in CLAUDE.md. Co-authored-by: bschellenberger2600 <bschellenberger2600@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/root.go (1)
552-564:⚠️ Potential issue | 🟡 MinorBridge goroutine's
saveRegistryisn't awaited.
<-scanDoneonly waits for the scanner goroutine, not for the bridge goroutine at lines 526–536 that doessaveRegistry(reg, regPath)afterscanChancloses. On theui.ErrCancelledpath a few lines below,os.Exit(0)fires as soon as the drain returns, which can happen before the bridge finalizes and persists registry changes (e.g., newly-discovered repos,LastSeenbumps).♻️ Suggested fix
var scanErr error scanDone := make(chan struct{}) go func() { defer close(scanDone) scanErr = git.ScanRepositoriesStream(opts, scanChan) }() now := time.Now() defaultMode := git.ParseMode(cfg.Global.DefaultMode) + bridgeDone := make(chan struct{}) go func() { + defer close(bridgeDone) defer close(tuiRepoChan) for repo := range scanChan { repo, include := upsertRepoIntoRegistry(reg, repo, now, defaultMode) if include { repo.Selected = true tuiRepoChan <- repo } } saveRegistry(reg, regPath) }() ... <-scanDone + <-bridgeDone🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 552 - 564, The bridge goroutine that reads scanChan and calls saveRegistry(reg, regPath) is not awaited: current code only waits on <-scanDone (scanner) so the program can exit before the bridge finishes persisting registry changes. Fix by adding an explicit synchronization for the bridge (e.g., a done channel or sync.WaitGroup) that the bridge signals after saveRegistry returns, then wait for that signal before returning/exiting (on the ui.ErrCancelled path and the normal shutdown path); reference the bridge goroutine that closes over scanChan and calls saveRegistry(reg, regPath), the existing scanDone waiter, and the cancelScan() call to ensure the new wait happens after cancelScan() and draining.internal/ui/repo_selector.go (1)
1226-1231:⚠️ Potential issue | 🔴 CriticalRemove unused
min— CI lint is failing.golangci-lint reports
func min is unused. After switching toresolveRainBackgroundWidth/RainDisplayWidth, nothing calls the local helper anymore, and Go 1.21+ provides a built-inminanyway.🧹 Proposed fix
-func min(a, b int) int { - if a < b { - return a - } - return b -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/repo_selector.go` around lines 1226 - 1231, Remove the unused local helper function min in internal/ui/repo_selector.go: delete the min(a, b int) int definition since golangci-lint flags it as unused and Go 1.21+ provides a built-in min; also search for any residual callers (e.g., inside resolveRainBackgroundWidth or RainDisplayWidth) and replace them with the language built-in min if any remain, ensuring no references to the min function are left behind.
🧹 Nitpick comments (2)
internal/ui/path_display.go (1)
84-89: Document whatreserve = 34accounts for.The measured
prefixW/suffixWexclude the dirty indicator (💧, ~3 cells) and the scroll hint (<< SCROLL PATH >>, ~19 cells) which only appears on the focused row. A single opaque34makes this easy to drift if either string is tweaked elsewhere. Consider either breaking it out or at least commenting the derivation.♻️ Example
- suffixW := lipgloss.Width(fmt.Sprintf("›) [%s] %s", repo.Mode.String(), remotesInfo)) - const reserve = 34 + suffixW := lipgloss.Width(fmt.Sprintf("›) [%s] %s", repo.Mode.String(), remotesInfo)) + // reserve = dirty indicator (" 💧" ≈ 3) + scroll hint (" << SCROLL PATH >>" ≈ 19) + safety margin. + const reserve = 34🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/path_display.go` around lines 84 - 89, The hard-coded reserve = 34 is undocumented and hides the width contributions from the per-row dirty indicator and the focused-row scroll hint; update the pw calculation in path_display.go to replace the magic number with a named constant (e.g., scrollHintWidth or reservedWidth) or add a clear inline comment explaining the derivation (dirty indicator ~3 cells + scroll hint ~19 cells + any padding = 34) and use that constant when computing pw alongside prefixW and suffixW so future changes to the dirty indicator or scroll hint are obvious; reference the existing variables prefixW, suffixW, reserve (or new constant) and pw to locate where to apply the change.internal/git/scanner.go (1)
163-179: Remove theerrors.Is(err, context.Canceled)checks—they never match for killed subprocesses.When
exec.CommandContextcancels a child process,cmd.Output()returns an*exec.ExitErrorwith signal information, not an error satisfyingerrors.Is(err, context.Canceled). The subsequentctx.Err()guards already detect cancellation correctly, making these branches unreachable dead code.♻️ Suggested simplification
remotes, err := getRemotes(ctx, repoPath) if err == nil { repo.Remotes = remotes - } else if errors.Is(err, context.Canceled) { - return Repository{}, err } if err := ctx.Err(); err != nil { return Repository{}, err } dirty, err := isDirty(ctx, repoPath) if err == nil { repo.IsDirty = dirty - } else if errors.Is(err, context.Canceled) { - return Repository{}, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/git/scanner.go` around lines 163 - 179, The checks for errors.Is(err, context.Canceled) after calling getRemotes and isDirty are dead/unreliable for killed subprocesses; remove those branches and rely on the existing ctx.Err() guard instead. Specifically, in internal/git/scanner.go update the getRemotes and isDirty call sites (the variables remotes, err := getRemotes(ctx, repoPath) and dirty, err := isDirty(ctx, repoPath)) to drop the else if errors.Is(err, context.Canceled) return Repository{}, err blocks and simply handle success by assigning repo.Remotes / repo.IsDirty and otherwise let the subsequent ctx.Err() check and the generic error return handle cancellation and other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/root.go`:
- Around line 552-564: The bridge goroutine that reads scanChan and calls
saveRegistry(reg, regPath) is not awaited: current code only waits on <-scanDone
(scanner) so the program can exit before the bridge finishes persisting registry
changes. Fix by adding an explicit synchronization for the bridge (e.g., a done
channel or sync.WaitGroup) that the bridge signals after saveRegistry returns,
then wait for that signal before returning/exiting (on the ui.ErrCancelled path
and the normal shutdown path); reference the bridge goroutine that closes over
scanChan and calls saveRegistry(reg, regPath), the existing scanDone waiter, and
the cancelScan() call to ensure the new wait happens after cancelScan() and
draining.
In `@internal/ui/repo_selector.go`:
- Around line 1226-1231: Remove the unused local helper function min in
internal/ui/repo_selector.go: delete the min(a, b int) int definition since
golangci-lint flags it as unused and Go 1.21+ provides a built-in min; also
search for any residual callers (e.g., inside resolveRainBackgroundWidth or
RainDisplayWidth) and replace them with the language built-in min if any remain,
ensuring no references to the min function are left behind.
---
Nitpick comments:
In `@internal/git/scanner.go`:
- Around line 163-179: The checks for errors.Is(err, context.Canceled) after
calling getRemotes and isDirty are dead/unreliable for killed subprocesses;
remove those branches and rely on the existing ctx.Err() guard instead.
Specifically, in internal/git/scanner.go update the getRemotes and isDirty call
sites (the variables remotes, err := getRemotes(ctx, repoPath) and dirty, err :=
isDirty(ctx, repoPath)) to drop the else if errors.Is(err, context.Canceled)
return Repository{}, err blocks and simply handle success by assigning
repo.Remotes / repo.IsDirty and otherwise let the subsequent ctx.Err() check and
the generic error return handle cancellation and other errors.
In `@internal/ui/path_display.go`:
- Around line 84-89: The hard-coded reserve = 34 is undocumented and hides the
width contributions from the per-row dirty indicator and the focused-row scroll
hint; update the pw calculation in path_display.go to replace the magic number
with a named constant (e.g., scrollHintWidth or reservedWidth) or add a clear
inline comment explaining the derivation (dirty indicator ~3 cells + scroll hint
~19 cells + any padding = 34) and use that constant when computing pw alongside
prefixW and suffixW so future changes to the dirty indicator or scroll hint are
obvious; reference the existing variables prefixW, suffixW, reserve (or new
constant) and pw to locate where to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 139853fb-5261-473b-8d30-5b4d28edcc8d
📒 Files selected for processing (10)
CLAUDE.mdREADME.mdcmd/root.gointernal/git/scanner.gointernal/git/scanner_test.gointernal/ui/config_view.gointernal/ui/panel_layout.gointernal/ui/panel_layout_test.gointernal/ui/path_display.gointernal/ui/repo_selector.go
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/ui/config_view.go
- README.md
2 participants
Summary
default_modenote.config.tomland TUI Default mode row now use canonicalglobal.default_modestrings (sync-default,sync-all,sync-current-branch,leave-untouched) matchinggit.ParseMode. Removed mistakenpush-*labels that never matched the parser.global.default_modeis validated on load; empty normalizes tosync-default; invalid values error.--rainflag, README); shortened CLAUDE.md maintenance notes.Testing
make test-racemake lintNote
Medium Risk
Medium risk because it changes config persistence semantics (adds cross-process file locking and stricter
global.default_modevalidation) and adjusts concurrent scan/TUI cancellation and buffering, which could affect interactive behavior and shutdown paths.Overview
Updates docs to more precisely describe fetch vs full-hydration triggers,
--pruneprecedence,--dry-runbehavior, and registry/config durability.Makes
global.default_modecanonical and strict: defaults tosync-default, validates allowed values on load, updates TUI settings/options and registry entry comments to match, and adds tests for invalid/empty cases.Hardens interactive
--rainflow and scanning:ScanRepositoriesStreamnow passes a context into per-repogitcalls so cancel aborts in-flight subprocesses,runRainTUIStreamcancels before draining channels and increases buffering to avoid goroutine stalls, and the TUI layout is refactored to shared panel width helpers with clamped rendering plus explicitctrl+c/interrupt-to-cancel handling.Reviewed by Cursor Bugbot for commit cf8e229. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Bug Fixes
New Features
Tests