fix(ui): persist group create/rename/move even during a reload window (fixes #1539)#1573
fix(ui): persist group create/rename/move even during a reload window (fixes #1539)#1573drmzperx wants to merge 3 commits into
Conversation
Creating a root group with 'g' or renaming one with 'r' only worked
every second try. The GroupDialog Create/Rename/Move handlers persisted
via saveInstances(), which is a no-op while isReloading (and aborts on
the mtime external-change check). Each successful group save triggers a
storage-watcher reload; if the next group mutation lands in that window
its save is skipped, and when the reload rebuilds groupTree from disk
the mutation vanishes — the alternating 'every second try' behavior.
Session creation already uses forceSaveInstances() for exactly this
reason ('was losing groups!'), and session renames get pendingTitleChanges
replay; group mutations had neither. Switch the three group-structure
mutations to forceSaveInstances() so they persist regardless of the
reload flag.
Regression tests drive the real handler with isReloading=true and assert
the group survives a reload round-trip through real SQLite storage.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGroup dialog handlers for create, rename, and move operations in home.go now call forceSaveInstances() instead of saveInstances(), ensuring persistence occurs even during storage-watcher reload windows. A new test file adds regression coverage using a real SQLite-backed Home instance to verify group create and rename persistence during reload. ChangesGroup persistence during reload
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GroupDialog
participant Home
participant Storage
User->>GroupDialog: create/rename/move group
GroupDialog->>Home: apply group mutation
Home->>Home: check isReloading
Home->>Storage: forceSaveInstances()
Storage-->>Home: persisted group data
Related issues: Suggested labels: review_needed_junior_swe, review_depth_standard Suggested reviewers: asheshgoplani 🐰 A group once vanished, then reappeared twice, 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/ui/group_persist_during_reload_test.go (1)
67-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for
GroupDialogMove.The PR also switches
GroupDialogMovetoforceSaveInstances(), but only create and rename get reload-window regression tests here. Adding a matchingTestGroupMovePersistsDuringReload(seed a session + two groups, move duringisReloading, verify viagroupPersisted/instance group path) would close the coverage gap for the third changed call site.🤖 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/group_persist_during_reload_test.go` around lines 67 - 108, Add a regression test for the `GroupDialogMove` persistence change, since only create and rename are covered today. In `internal/ui/group_persist_during_reload_test.go`, add a `TestGroupMovePersistsDuringReload` that seeds the necessary session/groups with `testHomeWithStorage` and `forceSaveInstances`, then sets `h.isReloading = true` and drives `handleGroupDialogKey` through the move flow. Verify the move is persisted using `groupPersisted` and the instance group path, so the third `forceSaveInstances()` call site is covered during a reload window.
🤖 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/ui/group_persist_during_reload_test.go`:
- Around line 67-108: The new tests in TestGroupCreatePersistsDuringReload and
TestGroupRenamePersistsDuringReload need an explicit RemoteSession
acknowledgement or a skip note to satisfy the internal/ui test guideline. Update
this test file by either adding coverage/verification for RemoteSession behavior
in the group persistence flow, or inserting a clear t.Skip in the relevant
test(s) explaining that groupTree and session.Instance here are local-only and
RemoteSession is out of scope. Keep the note close to the existing test setup in
testHomeWithStorage, handleGroupDialogKey, and groupPersisted so future readers
can see why RemoteSession is intentionally not exercised.
---
Nitpick comments:
In `@internal/ui/group_persist_during_reload_test.go`:
- Around line 67-108: Add a regression test for the `GroupDialogMove`
persistence change, since only create and rename are covered today. In
`internal/ui/group_persist_during_reload_test.go`, add a
`TestGroupMovePersistsDuringReload` that seeds the necessary session/groups with
`testHomeWithStorage` and `forceSaveInstances`, then sets `h.isReloading = true`
and drives `handleGroupDialogKey` through the move flow. Verify the move is
persisted using `groupPersisted` and the instance group path, so the third
`forceSaveInstances()` call site is covered during a reload window.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 687e3581-5f7c-47eb-90d9-2f6915222a9e
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/*.md,!CHANGELOG.md
📒 Files selected for processing (2)
internal/ui/group_persist_during_reload_test.gointernal/ui/home.go
| func TestGroupCreatePersistsDuringReload(t *testing.T) { | ||
| h, storage := testHomeWithStorage(t, "_group_persist_create") | ||
|
|
||
| // Simulate a storage-watcher reload in flight — the window in which the old | ||
| // saveInstances() silently dropped the write. | ||
| h.isReloading = true | ||
|
|
||
| // Drive the real create flow: create-mode dialog + Enter. | ||
| h.groupDialog.Show() | ||
| h.groupDialog.nameInput.SetValue("alpha") | ||
| h.handleGroupDialogKey(tea.KeyMsg{Type: tea.KeyEnter}) | ||
|
|
||
| if !groupPersisted(t, storage, "alpha") { | ||
| t.Error(`group "alpha" created during a reload window was not persisted; ` + | ||
| `create must use forceSaveInstances (saveInstances is skipped while isReloading)`) | ||
| } | ||
| } | ||
|
|
||
| func TestGroupRenamePersistsDuringReload(t *testing.T) { | ||
| h, storage := testHomeWithStorage(t, "_group_persist_rename") | ||
|
|
||
| // Seed and persist a group while NOT reloading. | ||
| h.groupTree.CreateGroup("old") | ||
| h.forceSaveInstances() | ||
| if !groupPersisted(t, storage, "old") { | ||
| t.Fatal("precondition: seed group 'old' should be persisted") | ||
| } | ||
|
|
||
| // Now rename it during a reload window. | ||
| h.isReloading = true | ||
| h.groupDialog.ShowRename("old", "old") | ||
| h.groupDialog.nameInput.SetValue("new") | ||
| h.handleGroupDialogKey(tea.KeyMsg{Type: tea.KeyEnter}) | ||
|
|
||
| if groupPersisted(t, storage, "old") { | ||
| t.Error(`old group name still present after rename during reload window`) | ||
| } | ||
| if !groupPersisted(t, storage, "new") { | ||
| t.Error(`renamed group "new" was not persisted during reload window; ` + | ||
| `rename must use forceSaveInstances`) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing explicit RemoteSession note/t.Skip per path guideline.
These tests exercise group create/rename during a reload window but don't mention RemoteSession or include a t.Skip explaining why it's out of scope. Groups here operate purely on local session.Instance data (groupTree), so RemoteSession is likely structurally inapplicable — similar to the fork-dispatch precedent — but the guideline calls for that to be stated explicitly.
As per path instructions, internal/ui/**/*_test.go files touching TUI behavior must "verify it also handles RemoteSession or include an explicit t.Skip documenting why."
📝 Suggested doc note
func TestGroupCreatePersistsDuringReload(t *testing.T) {
+ // RemoteSession items are read-only views with no local Instance/groupTree
+ // membership, so they are not applicable to group create/rename flows.
h, storage := testHomeWithStorage(t, "_group_persist_create")📝 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.
| func TestGroupCreatePersistsDuringReload(t *testing.T) { | |
| h, storage := testHomeWithStorage(t, "_group_persist_create") | |
| // Simulate a storage-watcher reload in flight — the window in which the old | |
| // saveInstances() silently dropped the write. | |
| h.isReloading = true | |
| // Drive the real create flow: create-mode dialog + Enter. | |
| h.groupDialog.Show() | |
| h.groupDialog.nameInput.SetValue("alpha") | |
| h.handleGroupDialogKey(tea.KeyMsg{Type: tea.KeyEnter}) | |
| if !groupPersisted(t, storage, "alpha") { | |
| t.Error(`group "alpha" created during a reload window was not persisted; ` + | |
| `create must use forceSaveInstances (saveInstances is skipped while isReloading)`) | |
| } | |
| } | |
| func TestGroupRenamePersistsDuringReload(t *testing.T) { | |
| h, storage := testHomeWithStorage(t, "_group_persist_rename") | |
| // Seed and persist a group while NOT reloading. | |
| h.groupTree.CreateGroup("old") | |
| h.forceSaveInstances() | |
| if !groupPersisted(t, storage, "old") { | |
| t.Fatal("precondition: seed group 'old' should be persisted") | |
| } | |
| // Now rename it during a reload window. | |
| h.isReloading = true | |
| h.groupDialog.ShowRename("old", "old") | |
| h.groupDialog.nameInput.SetValue("new") | |
| h.handleGroupDialogKey(tea.KeyMsg{Type: tea.KeyEnter}) | |
| if groupPersisted(t, storage, "old") { | |
| t.Error(`old group name still present after rename during reload window`) | |
| } | |
| if !groupPersisted(t, storage, "new") { | |
| t.Error(`renamed group "new" was not persisted during reload window; ` + | |
| `rename must use forceSaveInstances`) | |
| } | |
| } | |
| func TestGroupCreatePersistsDuringReload(t *testing.T) { | |
| // RemoteSession items are read-only views with no local Instance/groupTree | |
| // membership, so they are not applicable to group create/rename flows. | |
| h, storage := testHomeWithStorage(t, "_group_persist_create") | |
| // Simulate a storage-watcher reload in flight — the window in which the old | |
| // saveInstances() silently dropped the write. | |
| h.isReloading = true | |
| // Drive the real create flow: create-mode dialog + Enter. | |
| h.groupDialog.Show() | |
| h.groupDialog.nameInput.SetValue("alpha") | |
| h.handleGroupDialogKey(tea.KeyMsg{Type: tea.KeyEnter}) | |
| if !groupPersisted(t, storage, "alpha") { | |
| t.Error(`group "alpha" created during a reload window was not persisted; ` + | |
| `create must use forceSaveInstances (saveInstances is skipped while isReloading)`) | |
| } | |
| } | |
| func TestGroupRenamePersistsDuringReload(t *testing.T) { | |
| h, storage := testHomeWithStorage(t, "_group_persist_rename") | |
| // Seed and persist a group while NOT reloading. | |
| h.groupTree.CreateGroup("old") | |
| h.forceSaveInstances() | |
| if !groupPersisted(t, storage, "old") { | |
| t.Fatal("precondition: seed group 'old' should be persisted") | |
| } | |
| // Now rename it during a reload window. | |
| h.isReloading = true | |
| h.groupDialog.ShowRename("old", "old") | |
| h.groupDialog.nameInput.SetValue("new") | |
| h.handleGroupDialogKey(tea.KeyMsg{Type: tea.KeyEnter}) | |
| if groupPersisted(t, storage, "old") { | |
| t.Error(`old group name still present after rename during reload window`) | |
| } | |
| if !groupPersisted(t, storage, "new") { | |
| t.Error(`renamed group "new" was not persisted during reload window; ` + | |
| `rename must use forceSaveInstances`) | |
| } | |
| } |
🤖 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/group_persist_during_reload_test.go` around lines 67 - 108, The
new tests in TestGroupCreatePersistsDuringReload and
TestGroupRenamePersistsDuringReload need an explicit RemoteSession
acknowledgement or a skip note to satisfy the internal/ui test guideline. Update
this test file by either adding coverage/verification for RemoteSession behavior
in the group persistence flow, or inserting a clear t.Skip in the relevant
test(s) explaining that groupTree and session.Instance here are local-only and
RemoteSession is out of scope. Keep the note close to the existing test setup in
testHomeWithStorage, handleGroupDialogKey, and groupPersisted so future readers
can see why RemoteSession is intentionally not exercised.
Source: Path instructions
The fix switched all three group mutations to forceSaveInstances(), but only create and rename had reload-window regression tests (CodeRabbit nit on asheshgoplani#1573). Add TestGroupMovePersistsDuringReload: move the seed session into a destination group with isReloading=true and assert the session's GroupPath survives a storage round-trip. Verified it fails when the Move handler is reverted to saveInstances().
1 participant
Summary
Creating a root group with
g(or renaming/moving one) only worked every second try — the group appeared in memory but vanished on the next storage-watcher reload. Fixes #1539.Root cause
The
GroupDialogCreate/GroupDialogRename/GroupDialogMovehandlers persisted viasaveInstances(), which is a no-op whileisReloading(and also aborts on the mtime external-change check). Every successful save triggers a storage-watcher reload; if the next group mutation lands inside that reload window, its save is silently skipped, and when the reload rebuildsgroupTreefrom disk the mutation vanishes. That produces exactly the alternating "works every second try" behavior reported in #1539.Session creation already uses
forceSaveInstances()for this exact reason (thewas losing groups!comment insessionCreatedMsg), and session renames are protected by thependingTitleChangesreplay — group-structure mutations had neither.Fix
Switch the three group-structure mutations (create, rename, move) to
forceSaveInstances()so a user-initiated group change persists regardless of the reload flag.Tradeoff, made deliberately:
forceSaveInstances()also bypasses the mtime external-change guard, so a group save landing in a reload window could overwrite a concurrent external write. This matches the existing precedent for session creation; if the codebase later moves group mutations to targeted UPDATEs (like the title-persist path), these sites are the candidates.Tests
internal/ui/group_persist_during_reload_test.godrives the realhandleGroupDialogKeyhandler withisReloading=trueagainst real SQLite storage under an isolated$HOME, and asserts the created/renamed group survives aLoadWithGroupsround-trip. Both tests fail onmainand pass with the fix.Summary by CodeRabbit