fix(groups): reject rename when target path collides#1556
Conversation
Renaming a group to a name matching an existing sibling silently overwrote the existing entry in GroupTree.Groups. The orphaned group's sessions were then dropped from GetAllInstances(), and the following SaveWithGroups swept them from state.db via the full-table DELETE ... WHERE id NOT IN rewrite — permanent data loss. RenameGroup now returns ErrGroupAlreadyExists for both top-level and subtree collisions; TUI and web mutator callers surface the error before persisting, so the tree stays untouched. Related: asheshgoplani#1550 — full-table sweep is the amplifier, tracked separately.
📝 WalkthroughWalkthrough
ChangesRenameGroup error handling
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant HomeUI
participant WebMutator
participant GroupTree
HomeUI->>GroupTree: RenameGroup(oldPath, newName)
alt validation fails
GroupTree-->>HomeUI: ErrGroupNotFound or ErrGroupAlreadyExists
HomeUI->>HomeUI: setError and stop processing
else success
GroupTree-->>HomeUI: nil
HomeUI->>HomeUI: rebuild and save rename state
end
WebMutator->>GroupTree: RenameGroup(groupPath, newName)
alt validation fails
GroupTree-->>WebMutator: error
WebMutator-->>WebMutator: return error before persistence
else success
GroupTree-->>WebMutator: nil
WebMutator->>WebMutator: persist updated state
end
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/session/groups.go (1)
1142-1147: 🚀 Performance & Scalability | 🔵 TrivialSilent no-op when
oldPathis missing.Returning
nilwhen the source group doesn't exist means a caller (e.g. if the UI holds a stale group path due to a concurrent modification) gets a "success" signal even though nothing happened. Given this PR's goal is to surface rename failures rather than fail silently, consider returning a distinct sentinel (e.g.ErrGroupNotFound) here too for consistency, though this is a pre-existing behavior and a narrow edge case.🤖 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/session/groups.go` around lines 1142 - 1147, RenameGroup currently returns nil when the source group path is missing, which makes a failed rename look successful. Update GroupTree.RenameGroup to return a distinct not-found error (such as ErrGroupNotFound) from the missing-oldPath branch, and ensure callers can distinguish this from a successful rename; keep the existing ErrGroupAlreadyExists behavior for destination collisions.
🤖 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.
Nitpick comments:
In `@internal/session/groups.go`:
- Around line 1142-1147: RenameGroup currently returns nil when the source group
path is missing, which makes a failed rename look successful. Update
GroupTree.RenameGroup to return a distinct not-found error (such as
ErrGroupNotFound) from the missing-oldPath branch, and ensure callers can
distinguish this from a successful rename; keep the existing
ErrGroupAlreadyExists behavior for destination collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 285ff1fa-30e5-4e64-bfef-685ec34560fc
📒 Files selected for processing (4)
internal/session/groups.gointernal/session/groups_test.gointernal/ui/home.gointernal/ui/web_mutator.go
Address CodeRabbit review on asheshgoplani#1556. Consistent with this PR's "surface rename failures" goal — the missing-oldPath branch used to return nil, making a rename of a stale/gone group look successful to the caller. TUI and web mutator already surface whatever RenameGroup returns, so they now show a clear error.
…f RenameTargetPath and pendingGroupOps reapply Committed by Ashesh Goplani
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/ui/web_mutator.go (1)
545-551: 🩺 Stability & Availability | 🔵 TrivialNon-atomic delete-then-save pattern risks data loss on crash.
DeleteGroupSubtreeandSaveWithGroupsare separate SQLite operations. A process crash between them leaves the old group rows deleted but the new paths unsaved — the group (and its sessions) silently disappears fromstate.dbon the next reload. This mirrors the existingDeleteGrouppattern, but applying it toRenameGroupwidens the blast radius since a rename is a non-destructive user action that shouldn't risk data loss.Consider wrapping both operations in a single SQLite transaction so the delete and re-add are atomic. As per path instructions, SQLite write atomicity is a key concern for
internal/session/**code.🤖 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/ui/web_mutator.go` around lines 545 - 551, Make the RenameGroup flow encompassing DeleteGroupSubtree and SaveWithGroups execute within one SQLite transaction, committing only after both operations succeed and rolling back on any error or crash. Update the relevant storage API or transaction-aware methods rather than leaving the existing separate calls in RenameGroup, preserving no-op rename behavior and returning contextual errors.Source: Path instructions
🤖 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.
Nitpick comments:
In `@internal/ui/web_mutator.go`:
- Around line 545-551: Make the RenameGroup flow encompassing DeleteGroupSubtree
and SaveWithGroups execute within one SQLite transaction, committing only after
both operations succeed and rolling back on any error or crash. Update the
relevant storage API or transaction-aware methods rather than leaving the
existing separate calls in RenameGroup, preserving no-op rename behavior and
returning contextual errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bdfc387e-4fb2-4818-aa96-6712dae94143
📒 Files selected for processing (4)
internal/session/groups.gointernal/session/groups_test.gointernal/ui/home.gointernal/ui/web_mutator.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ui/home.go
2 participants
Summary
GroupTree.Groups. The orphaned group's sessions dropped out ofGetAllInstances(), and the followingSaveWithGroupsswept them fromstate.dbvia the full-tableDELETE ... WHERE id NOT IN (...)rewrite — permanent data loss, no error to the user, and no way to retry the rename to recover.RenameGroupnow returns a newErrGroupAlreadyExistsfor both top-level and subtree collisions (a rename that would rewrite a descendant onto an occupied path is rejected too).internal/ui/home.go) and web mutator (internal/ui/web_mutator.go) callers surface the error before the save, so the tree stays untouched on collision.Repro (before this PR)
MySessions(the default one) andinner, with sessions in each.MySessions→inner.innerand its sessions are gone from the UI and fromstate.db. Trying to rename again silently no-ops because the original name no longer resolves.Related
DELETE ... NOT INsweep is what turned this collision from a UI merge into permanent data loss. That amplifier is orthogonal and tracked separately; this PR fixes the rename API so the collision can't fire in the first place.Test plan
TestRenameGroup_RejectsCollision— sibling collision returnsErrGroupAlreadyExists; both groups and their sessions remain intact.TestRenameGroup_RejectsSubtreeCollision— collision at a descendant path (Alpha→BetawhenBeta/Childalready exists) is rejected before any mutation lands.TestRenameGroup,TestRenameGroupWithSubgroups,TestRenameSubgroupstill pass unchanged.TestHomeRenameGroupWithR(TUI hotkey path) still passes.go build ./...clean;go test ./internal/session/... ./internal/web/...green.Summary by CodeRabbit