refactor: dissolve single-file folders (21 → 14)#51
Merged
Conversation
Prepare branchprompt for folding into internal/tui where Model is already an interface. Renames the exported symbols to avoid the collision; call sites in internal/add and internal/cli adjusted. Model -> BranchPromptModel NewModel -> NewBranchPromptModel PickedMsg -> BranchPromptPickedMsg CancelledMsg -> BranchPromptCancelledMsg No behavior change. Folding into tui happens in the next commit.
Branchprompt is a TUI primitive (default-branch picker model). Moving it inside the internal/tui package eliminates the standalone folder without crossing the seam — branchprompt already imported tui for its KeyMsg/Cmd/Model/Style primitives. Renamed symbols (from prior commit) keep BranchPrompt-prefixed names to avoid collision with the existing tui.Model interface. Net: -1 folder, file moves only.
The conflict store is daemon-owned: the reconciler writes, ws sync
resolve reads. cli and doctor already import daemon for unrelated
reasons; the import direction stays the same after the fold.
Renamed to disambiguate inside package daemon:
conflict.Store -> daemon.ConflictStore
conflict.Open -> daemon.OpenConflictStore
conflict.Path -> daemon.ConflictPath
conflict.Kind -> daemon.ConflictKind
conflict.Conflict, Notify, NotifyNew, Kind* constants keep their names
Call sites updated: cli/{bootstrap,migrate,sync}.go,
daemon/reconciler.go, doctor/doctor.go, doctor/system_test.go,
daemon/conflict_bench_test.go.
auth is GitHub auth — device flow + PAT exist only to feed github.Client. Folding makes github.NewProvider() self-contained instead of forcing every caller to wire auth separately. No symbol collisions; names stay (Token, LoadToken, SaveToken, etc.) and become github.Token, github.LoadToken, etc. at call sites. Call sites updated: cli/root.go, daemon/daemon.go, github/github.go (self-references drop the auth. prefix).
Both packages transition a repo into the bare+worktree layout: migrate
upgrades an existing plain clone; bootstrap materializes from
workspace.toml. Same operation, different starting state.
Combined into internal/repo/ with two files (bootstrap.go, migrate.go).
Collision symbols renamed with Bootstrap/Migrate prefix to keep each
flow's state and lifecycle separate (no premature shared abstraction):
bootstrap: Sidecar -> BootstrapSidecar, DoneEntry -> BootstrapDoneEntry,
New/Load/Save/Delete -> NewBootstrapSidecar/etc.,
IsAlive -> BootstrapSidecarIsAlive
migrate: Sidecar -> MigrateSidecar, DoneEntry -> MigrateDoneEntry,
Options/Result -> MigrateOptions/MigrateResult,
New/Load/Save/Delete -> NewMigrateSidecar/etc.,
IsAlive -> MigrateSidecarIsAlive
Non-colliding symbols keep their names (Plan, PlanItem, State, ScanPlan,
Check, CheckResult, MigrateProject, ErrAlreadyMigrated).
Call sites updated in cli/bootstrap.go and cli/migrate.go. Tests
repackaged to repo_test.
aliasmgr is the interactive manager for the same alias domain that internal/alias's generator/installer/resolver operates on. Same domain, same workspace.toml surface, no reason for separate packages. Renamed to disambiguate inside the merged package: aliasmgr.Model -> alias.ManagerModel aliasmgr.New -> alias.NewManagerModel aliasmgr.Result -> alias.ManagerResult manager.go drops the alias. prefix on internal references (no longer cross-package). Call sites in cli/root.go updated. The package now reads as alias.go (generator + resolver + installer) + manager.go (interactive picker).
Doctor is consumed only by cli; the folder added an import path for zero structural benefit. Folded as cli/doctor.go + three test files (doctor_test, doctor_project_test, doctor_system_test). No symbol collisions on exported names. One private helper renamed to avoid duplicate: contains() in doctor_system_test.go conflicts with contains() in worktree_test.go (different signatures) — doctor's becomes containsSubstr.
Setup is the first-run wizard, consumed only by cli. Folded as cli/setup.go. Renamed to disambiguate inside cli: setup.Model -> SetupModel setup.NewModel -> NewSetupModel setup.Result -> SetupResult Private helper humanizeTime renamed to humanizeTimeShort — collides with cli's existing humanizeTime which uses different formatting and bucket boundaries; the two utilities serve different display contexts.
The exclusion globs in .golangci.yml were keyed to pre-dissolution paths (internal/aliasmgr, internal/setup, internal/migrate). After folding those into alias/, cli/, and repo/, the exclusions no longer matched and 8 long-standing complex funcs (MigrateProject, buildTree, etc.) started failing the gate. Paths updated to match new homes; same functions, same waivers, no new debt.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR consolidates multiple internal Go packages into larger namespaces and systematically renames public APIs with clearer purpose-driven prefixes. ChangesPackage and API consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/tui/branchprompt.go (1)
1-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh module comments to match renamed package and symbols.
The header still references
branchprompt,Model,PickedMsg, andCancelledMsg, but the file now exposestui,BranchPromptModel,BranchPromptPickedMsg, andBranchPromptCancelledMsg.Suggested doc-only update
-// Package branchprompt provides a standalone bubbletea model for picking +// Package tui provides a standalone bubbletea model for picking // a default branch when clone.CloneIntoLayout cannot auto-resolve one. @@ -// Callers embed Model inside their own tea.Model and delegate Update/View +// Callers embed BranchPromptModel inside their own tea.Model and delegate Update/View // when the parent step is "branch-prompt". When the user picks a branch -// or cancels, the model emits PickedMsg / CancelledMsg; the parent is +// or cancels, the model emits BranchPromptPickedMsg / BranchPromptCancelledMsg; the parent is🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/branchprompt.go` around lines 1 - 13, Update the package header comment to reflect the renamed package and exported symbols: replace references to "branchprompt", "Model", "PickedMsg", and "CancelledMsg" with "tui", "BranchPromptModel", "BranchPromptPickedMsg", and "BranchPromptCancelledMsg" respectively, and ensure the comment text explains that callers embed BranchPromptModel and expect BranchPromptPickedMsg / BranchPromptCancelledMsg; keep the existing intent and phrasing but update identifiers to match the current code.internal/cli/bootstrap.go (1)
134-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways delete the bootstrap sidecar after a completed run.
The sidecar is saved before cloning starts, but this cleanup only runs when at least one project reaches
Done. If every clone fails, the nextws bootstraprun will still find a dead sidecar and prompt for resume/discard even though there is nothing resumable.Suggested cleanup
- if final.sidecar != nil && len(final.sidecar.Done) > 0 { - if err := commitBootstrap(final.sidecar); err != nil { - return fmt.Errorf("commit bootstrap: %w", err) - } - - if err := repo.DeleteBootstrapSidecar(wsRoot); err != nil { - fmt.Fprintf(os.Stderr, "warning: could not remove sidecar: %v\n", err) - } - } + if final.sidecar != nil { + if len(final.sidecar.Done) > 0 { + if err := commitBootstrap(final.sidecar); err != nil { + return fmt.Errorf("commit bootstrap: %w", err) + } + } + if err := repo.DeleteBootstrapSidecar(wsRoot); err != nil { + fmt.Fprintf(os.Stderr, "warning: could not remove sidecar: %v\n", err) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/bootstrap.go` around lines 134 - 141, The code only calls repo.DeleteBootstrapSidecar(wsRoot) when final.sidecar != nil && len(final.sidecar.Done) > 0, leaving stale sidecars if all clones fail; change the logic so DeleteBootstrapSidecar(wsRoot) is invoked whenever final.sidecar != nil (regardless of final.sidecar.Done), i.e., move or add the repo.DeleteBootstrapSidecar(wsRoot) call outside/after the Done check so the sidecar is always removed after the run; preserve existing error handling behavior (print a non-fatal warning to stderr) and keep commitBootstrap(final.sidecar) as-is when at least one project reached Done.internal/cli/migrate.go (1)
611-618:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDelete the migrate sidecar even when nothing completed.
repo.SaveMigrateSidecarruns before processing starts, but the final cleanup only removes it when at least one project was marked done. A run with only skips or failures will therefore leave a dead sidecar behind and trigger a bogus resume/discard prompt next time.Suggested cleanup
- if final.sidecar != nil && len(final.sidecar.Done) > 0 { - if err := commitMigrate(final.sidecar); err != nil { - return fmt.Errorf("commit migrate: %w", err) - } - if err := repo.DeleteMigrateSidecar(wsRoot); err != nil { - fmt.Fprintf(os.Stderr, "warning: could not remove sidecar: %v\n", err) - } - } + if final.sidecar != nil { + if len(final.sidecar.Done) > 0 { + if err := commitMigrate(final.sidecar); err != nil { + return fmt.Errorf("commit migrate: %w", err) + } + } + if err := repo.DeleteMigrateSidecar(wsRoot); err != nil { + fmt.Fprintf(os.Stderr, "warning: could not remove sidecar: %v\n", err) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/migrate.go` around lines 611 - 618, The current cleanup only calls repo.DeleteMigrateSidecar(wsRoot) when final.sidecar != nil && len(final.sidecar.Done) > 0, leaving a sidecar when nothing completed; change the logic so repo.DeleteMigrateSidecar(wsRoot) is always attempted if final.sidecar != nil (regardless of len(final.sidecar.Done)). Keep the commitMigrate(final.sidecar) call only when len(final.sidecar.Done) > 0, but ensure DeleteMigrateSidecar(wsRoot) runs even if commitMigrate returns an error (e.g., call DeleteMigrateSidecar in a defer or after handling commitMigrate error and still return the commit error), referencing final.sidecar, commitMigrate, repo.DeleteMigrateSidecar, repo.SaveMigrateSidecar, and wsRoot to locate the code.
🧹 Nitpick comments (1)
.golangci.yml (1)
189-193: ⚡ Quick winNarrow the TUI waiver to the moved TUI files.
This regex now disables
gocyclo/gocognit/funlenfor every Go file ininternal/aliasandinternal/repo, while the comment only justifiesalias/manager.goand repo orchestration files. That broadens the lint blind spot beyond the code this PR actually moved.♻️ Suggested scope tightening
- - path: internal/(agent|add|alias|create|repo|tui)/.*\.go + - path: internal/(agent|add|create|tui)/.*\.go + - path: internal/alias/manager\.go + - path: internal/repo/bootstrap\.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.golangci.yml around lines 189 - 193, The current .golangci.yml path glob disables gocyclo/gocognit/funlen for all files under internal/alias and internal/repo; narrow it to only the moved TUI-related files by replacing the broad pattern (internal/(agent|add|alias|create|repo|tui)/.*\.go) with a pattern that continues to match the full packages that actually need waivers (e.g., internal/agent, internal/add, internal/create, internal/tui) but explicitly lists the moved files in alias and repo (for example internal/alias/manager\.go and the specific orchestration file in internal/repo such as bootstrap\.go) so only those exact files are exempted; update the path entry in .golangci.yml accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/bootstrap.go`:
- Line 55: Replace the misleading empty-state text in internal/cli/bootstrap.go:
locate the fmt.Println call that prints "No active projects to repo." and change
the message to clearly reflect the command behavior (e.g. "No active projects to
bootstrap." or "No active projects to clone."). Update only the literal string
in that fmt.Println so the CLI shows the correct action word (“bootstrap” or
“clone”) instead of "repo."
In `@internal/cli/migrate.go`:
- Line 167: The user-facing empty-state string currently prints fmt.Println("No
active projects to repo.") which exposes an internal namespace; update both
occurrences in internal/cli/migrate.go (the print statements in the migrate
command handler) to use "migrate" in the message, e.g. replace with
fmt.Println("No active projects to migrate.") so the CLI describes the command
rather than the internal package name.
In `@internal/cli/sync.go`:
- Around line 162-175: The resolver dispatch in handleConflict currently covers
only a subset of conflict kinds; add explicit case branches for
daemon.KindNeedsBootstrap, daemon.KindPathBlocked, and daemon.KindCloneFailed in
the switch inside handleConflict(c daemon.Conflict) so those conflicts are
routed to appropriate handlers (e.g., resolveNeedsBootstrap, resolvePathBlocked,
resolveCloneFailed) or to a common resolver function as appropriate; ensure you
reference these new handler function names and update the switch to return their
results so ws sync resolve can handle those conflict kinds instead of falling
through to the unknown path.
---
Outside diff comments:
In `@internal/cli/bootstrap.go`:
- Around line 134-141: The code only calls repo.DeleteBootstrapSidecar(wsRoot)
when final.sidecar != nil && len(final.sidecar.Done) > 0, leaving stale sidecars
if all clones fail; change the logic so DeleteBootstrapSidecar(wsRoot) is
invoked whenever final.sidecar != nil (regardless of final.sidecar.Done), i.e.,
move or add the repo.DeleteBootstrapSidecar(wsRoot) call outside/after the Done
check so the sidecar is always removed after the run; preserve existing error
handling behavior (print a non-fatal warning to stderr) and keep
commitBootstrap(final.sidecar) as-is when at least one project reached Done.
In `@internal/cli/migrate.go`:
- Around line 611-618: The current cleanup only calls
repo.DeleteMigrateSidecar(wsRoot) when final.sidecar != nil &&
len(final.sidecar.Done) > 0, leaving a sidecar when nothing completed; change
the logic so repo.DeleteMigrateSidecar(wsRoot) is always attempted if
final.sidecar != nil (regardless of len(final.sidecar.Done)). Keep the
commitMigrate(final.sidecar) call only when len(final.sidecar.Done) > 0, but
ensure DeleteMigrateSidecar(wsRoot) runs even if commitMigrate returns an error
(e.g., call DeleteMigrateSidecar in a defer or after handling commitMigrate
error and still return the commit error), referencing final.sidecar,
commitMigrate, repo.DeleteMigrateSidecar, repo.SaveMigrateSidecar, and wsRoot to
locate the code.
In `@internal/tui/branchprompt.go`:
- Around line 1-13: Update the package header comment to reflect the renamed
package and exported symbols: replace references to "branchprompt", "Model",
"PickedMsg", and "CancelledMsg" with "tui", "BranchPromptModel",
"BranchPromptPickedMsg", and "BranchPromptCancelledMsg" respectively, and ensure
the comment text explains that callers embed BranchPromptModel and expect
BranchPromptPickedMsg / BranchPromptCancelledMsg; keep the existing intent and
phrasing but update identifiers to match the current code.
---
Nitpick comments:
In @.golangci.yml:
- Around line 189-193: The current .golangci.yml path glob disables
gocyclo/gocognit/funlen for all files under internal/alias and internal/repo;
narrow it to only the moved TUI-related files by replacing the broad pattern
(internal/(agent|add|alias|create|repo|tui)/.*\.go) with a pattern that
continues to match the full packages that actually need waivers (e.g.,
internal/agent, internal/add, internal/create, internal/tui) but explicitly
lists the moved files in alias and repo (for example internal/alias/manager\.go
and the specific orchestration file in internal/repo such as bootstrap\.go) so
only those exact files are exempted; update the path entry in .golangci.yml
accordingly.
🪄 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: 46e03fd6-e485-4879-b343-b389eba2ed2c
📒 Files selected for processing (25)
.golangci.ymlinternal/add/sources.gointernal/add/tui.gointernal/alias/manager.gointernal/cli/bootstrap.gointernal/cli/doctor.gointernal/cli/doctor_project_test.gointernal/cli/doctor_system_test.gointernal/cli/doctor_test.gointernal/cli/migrate.gointernal/cli/root.gointernal/cli/setup.gointernal/cli/sync.gointernal/daemon/conflict.gointernal/daemon/conflict_bench_test.gointernal/daemon/daemon.gointernal/daemon/reconciler.gointernal/github/auth.gointernal/github/github.gointernal/repo/bootstrap.gointernal/repo/bootstrap_test.gointernal/repo/migrate.gointernal/repo/migrate_test.gointernal/tui/branchprompt.gointernal/tui/branchprompt_test.go
Three empty-state messages printed "No active projects to repo." after the package rename sweep — restore "bootstrap" / "migrate" verbs. handleConflict switch only covered 5 of 8 daemon.Kind* values. Add cases for KindNeedsBootstrap, KindPathBlocked, KindCloneFailed with guidance handlers matching the resolveNeedsMigration pattern (print the next step, return false so the daemon re-evaluates on next sync).
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
1 participant
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.
Criterion: a single-file folder (
internal/auth/auth.go,internal/doctor/doctor.go, …) buys a directory + import path + mental boundary for zero structural benefit. Fold it into the consumer unless the package is foundational (multi-consumer leaf) or a Go convention (testutil).By that yardstick nine packages were wrong:
alias+aliasmgr,auth,bootstrap,branchprompt,conflict,doctor,migrate,setup. Each fold preserves the consumer relationship that already existed and is gated by [[top-to-bottom-readability]] — every merged file still walks as one narrative.21 folders → 14. Production file count holds at 45 (folds consolidate but
repo/adds back one). Binary 14 MB unchanged. TUI seam intact: 0charmbraceletimports outsideinternal/tui/. All tests +go vet+golangci-lint+gofmtgreen at every commit.Where things went:
alias + aliasmgr → alias/(same domain, manager joins generator);auth → github/(auth is github auth — feedsgithub.Client);bootstrap + migrate → repo/(both transition a repo into the bare+worktree layout — new package);branchprompt → tui/(TUI primitive belongs with the others);conflict → daemon/(daemon owns the conflict store, cli + doctor already imported daemon);doctor → cli/andsetup → cli/(cli-only consumers, each gets its owncli/doctor.go/cli/setup.goto keeproot.gofrom growing past readability).Collision renames (made explicit so future readers can grep):
branchprompt.Model→tui.BranchPromptModel(the existingtui.Modelis an interface);conflict.Store/Open/Path/Kind→daemon.ConflictStore/OpenConflictStore/ConflictPath/ConflictKind; bootstrap & migrate both exportedSidecar/DoneEntry/New/Load/Save/Delete/IsAlive— each gets aBootstrap/Migrateprefix inrepo/;aliasmgr.Model/New/Result→alias.ManagerModel/NewManagerModel/ManagerResult;setup.Model/NewModel/Result→cli.SetupModel/NewSetupModel/SetupResult; one private-helper collisionhumanizeTime(different formatting) →humanizeTimeShortin setup,contains(different signature) →containsSubstrin doctor's system test.The lint config commit at the end updates path-keyed gocognit waivers to point at the new file homes (same functions, same justified-complex waivers, just new paths).
go build,go test ./...,go vet,golangci-lint run,gofmt -l .all green. TUI seam check returns 0. Smoke remaining:ws agent,ws alias,ws add,ws status,ws syncon a real workspace.Summary by CodeRabbit