Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- **Creating, renaming, or moving a group no longer needs two attempts.** The `GroupDialog` create/rename/move handlers persisted via `saveInstances()`, which is skipped while a storage-watcher reload is in flight, so a group mutation landing in that window was silently dropped when the reload rebuilt the group tree from disk — the "create group has to be done twice" bug. These user-initiated group-structure mutations now use `forceSaveInstances()`, matching session creation. ([#1573](https://github.com/asheshgoplani/agent-deck/pull/1573)) (fixes [#1539](https://github.com/asheshgoplani/agent-deck/issues/1539))

## [1.10.9] - 2026-07-02

### Fixed
Expand Down
108 changes: 108 additions & 0 deletions internal/ui/group_persist_during_reload_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package ui

import (
"os"
"testing"

tea "github.com/charmbracelet/bubbletea"

"github.com/asheshgoplani/agent-deck/internal/session"
)

// These tests are the regression guard for the "new main group only works
// every second try" / "rename group only works every second try" bug.
//
// Root cause: GroupDialogCreate/Rename persisted via saveInstances(), which is
// SKIPPED while isReloading (and aborts on the mtime external-change check). A
// group mutation that landed during a storage-watcher reload window was written
// to memory but never to disk, so when the reload rebuilt groupTree from disk
// the mutation vanished — hence the alternating "every second try". The fix
// switches these paths to forceSaveInstances(), which persists regardless of
// the reload flag (mirroring session creation's "was losing groups!" forceSave).

// testHomeWithStorage builds a minimally-wired Home backed by real SQLite
// storage under an isolated temp HOME, with one seed instance so the
// empty-overwrite guard in saveInstancesWithForce does not refuse the write.
func testHomeWithStorage(t *testing.T, profile string) (*Home, *session.Storage) {
t.Helper()
origHome := os.Getenv("HOME")
os.Setenv("HOME", t.TempDir())
session.ClearUserConfigCache()
t.Cleanup(func() {
os.Setenv("HOME", origHome)
session.ClearUserConfigCache()
})

storage, err := session.NewStorageWithProfile(profile)
if err != nil {
t.Fatalf("NewStorageWithProfile: %v", err)
}
t.Cleanup(func() { storage.Close() })

seed := session.NewInstance("seed", "/tmp/seed")
h := &Home{
storage: storage,
profile: profile,
instances: []*session.Instance{seed},
groupTree: session.NewGroupTree([]*session.Instance{seed}),
groupDialog: NewGroupDialog(),
}
return h, storage
}

func groupPersisted(t *testing.T, storage *session.Storage, name string) bool {
t.Helper()
_, groups, err := storage.LoadWithGroups()
if err != nil {
t.Fatalf("LoadWithGroups: %v", err)
}
for _, g := range groups {
if g.Name == name {
return true
}
}
return false
}

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`)
}
}
Comment on lines +85 to +126

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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

13 changes: 10 additions & 3 deletions internal/ui/home.go
Original file line number Diff line number Diff line change
Expand Up @@ -9619,7 +9619,14 @@ func (h *Home) handleGroupDialogKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
}
}
h.rebuildFlatItems()
h.saveInstances() // Persist the new group
// forceSave, not saveInstances: a save is SKIPPED while
// isReloading (and aborts on the mtime external-change check),
// so a group created during a storage-watcher reload window is
// dropped when the reload rebuilds groupTree from disk — the
// "new group only works every second try" bug. Group mutations
// are user-initiated and MUST persist, like session creation
// (see the "was losing groups!" forceSave in sessionCreatedMsg).
h.forceSaveInstances() // Persist the new group
}
case GroupDialogRename:
name := h.groupDialog.GetValue()
Expand All @@ -9629,7 +9636,7 @@ func (h *Home) handleGroupDialogKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
h.instances = h.groupTree.GetAllInstances()
h.instancesMu.Unlock()
h.rebuildFlatItems()
h.saveInstances()
h.forceSaveInstances() // MUST persist even during reload (see GroupDialogCreate)
}
case GroupDialogMove:
targetGroupPath := h.groupDialog.GetSelectedGroup()
Expand All @@ -9641,7 +9648,7 @@ func (h *Home) handleGroupDialogKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
h.instances = h.groupTree.GetAllInstances()
h.instancesMu.Unlock()
h.rebuildFlatItems()
h.saveInstances()
h.forceSaveInstances() // MUST persist even during reload (see GroupDialogCreate)
}
}
case GroupDialogRenameSession:
Expand Down