Refactor dialog close action handling and ensure close option is last in dialog options

Author: markusressel

internal/ui/dialog/delete_file_dialog.go

@@ -86,6 +86,7 @@ func (d *DeleteFileDialog) selectAction(option *DialogOption) {
 	case DeleteFileDialogDeleteFileActionId:
 		d.DeleteFile()
 	case DialogCloseActionId:
+		d.Close()
 	default:
 		d.Close()
 	}

internal/ui/dialog/delete_snapshot_dialog.go

@@ -84,6 +84,7 @@ func (d *DeleteSnapshotDialog) selectAction(option *DialogOption) {
 	case DeleteSnapshotDialogDeleteSnapshotActionId:
 		d.DeleteSnapshot()
 	case DialogCloseActionId:
+		d.Close()
 	default:
 		d.Close()
 	}

internal/ui/dialog/dialog_close_action_test.go

@@ -0,0 +1,73 @@
+package dialog
+
+import (
+	"testing"
+	"time"
+	"zfs-file-history/internal/data"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSnapshotActionDialog_SelectCloseOption_EmitsCloseAction(t *testing.T) {
+	d := &SnapshotActionDialog{actionChannel: make(chan DialogActionId, 1)}
+
+	d.selectAction(&DialogOption{Id: DialogCloseActionId, Name: "Close"})
+
+	assertDialogActionEmitted(t, d.actionChannel, DialogCloseActionId)
+}
+
+func TestDeleteFileDialog_SelectCancelOption_EmitsCloseAction(t *testing.T) {
+	d := &DeleteFileDialog{actionChannel: make(chan DialogActionId, 1)}
+
+	d.selectAction(&DialogOption{Id: DialogCloseActionId, Name: "Cancel"})
+
+	assertDialogActionEmitted(t, d.actionChannel, DialogCloseActionId)
+}
+
+func TestDeleteSnapshotDialog_SelectCancelOption_EmitsCloseAction(t *testing.T) {
+	d := &DeleteSnapshotDialog{actionChannel: make(chan DialogActionId, 1)}
+
+	d.selectAction(&DialogOption{Id: DialogCloseActionId, Name: "Cancel"})
+
+	assertDialogActionEmitted(t, d.actionChannel, DialogCloseActionId)
+}
+
+func TestMultiSnapshotActionDialog_SelectCloseOption_EmitsCloseAction(t *testing.T) {
+	d := &MultiSnapshotActionDialog{actionChannel: make(chan DialogActionId, 1)}
+
+	d.selectAction(&DialogOption{Id: DialogCloseActionId, Name: "Close"})
+
+	assertDialogActionEmitted(t, d.actionChannel, DialogCloseActionId)
+}
+
+func TestSnapshotActionDialog_SelectCloseOption_DoesNotEmitOtherAction(t *testing.T) {
+	d := &SnapshotActionDialog{
+		actionChannel: make(chan DialogActionId, 2),
+		snapshot:      &data.SnapshotBrowserEntry{},
+	}
+
+	d.selectAction(&DialogOption{Id: DialogCloseActionId, Name: "Close"})
+
+	assertDialogActionEmitted(t, d.actionChannel, DialogCloseActionId)
+	assertNoMoreDialogActions(t, d.actionChannel)
+}
+
+func assertDialogActionEmitted(t *testing.T, ch <-chan DialogActionId, expected DialogActionId) {
+	t.Helper()
+	select {
+	case action := <-ch:
+		assert.Equal(t, expected, action)
+	case <-time.After(100 * time.Millisecond):
+		t.Fatalf("expected action %v to be emitted", expected)
+	}
+}
+
+func assertNoMoreDialogActions(t *testing.T, ch <-chan DialogActionId) {
+	t.Helper()
+	select {
+	case action := <-ch:
+		t.Fatalf("did not expect extra dialog action %v", action)
+	case <-time.After(30 * time.Millisecond):
+		// expected no more actions
+	}
+}

internal/ui/dialog/file_action_dialog.go

@@ -103,7 +103,21 @@ func buildFileDialogOptions(file *data.FileBrowserEntry, diffBinAvailable bool)
 		Name: "Create Snapshot",
 	})
 
-	return dialogOptions
+	return ensureDialogCloseIsLast(dialogOptions)
+}
+
+func ensureDialogCloseIsLast(options []*DialogOption) []*DialogOption {
+	closeIndex := slices.IndexFunc(options, func(option *DialogOption) bool {
+		return option != nil && option.Id == DialogCloseActionId
+	})
+	if closeIndex < 0 || closeIndex == len(options)-1 {
+		return options
+	}
+
+	closeOption := options[closeIndex]
+	result := slices.Delete(options, closeIndex, closeIndex+1)
+	result = append(result, closeOption)
+	return result
 }
 
 func DiffBinExists() bool {
@@ -147,6 +161,7 @@ func (d *FileActionDialog) selectAction(option *DialogOption) {
 	case FileDialogCreateSnapshotDialogActionId:
 		d.CreateSnapshot()
 	case DialogCloseActionId:
+		d.Close()
 	default:
 		d.Close()
 	}

internal/ui/dialog/file_action_dialog_test.go

@@ -2,6 +2,7 @@ package dialog
 
 import (
 	"testing"
+	"time"
 	"zfs-file-history/internal/data"
 	"zfs-file-history/internal/data/diff_state"
 
@@ -100,10 +101,45 @@ func TestBuildFileDialogOptions_OnlyRealFile(t *testing.T) {
 	)
 }
 
+func TestBuildFileDialogOptions_DeletedFile_AlwaysHasCloseLast(t *testing.T) {
+	entry := &data.FileBrowserEntry{
+		Name: "deleted-in-live.txt",
+		Type: data.File,
+		SnapshotFiles: []*data.SnapshotFile{
+			{},
+		},
+		DiffState: diff_state.Deleted,
+	}
+
+	options := buildFileDialogOptions(entry, false)
+
+	assert.Equal(t,
+		[]DialogActionId{
+			FileDialogCreateSnapshotDialogActionId,
+			FileDialogRestoreFileActionId,
+			DialogCloseActionId,
+		},
+		optionIds(options),
+	)
+}
+
 func optionIds(options []*DialogOption) []DialogActionId {
 	result := make([]DialogActionId, 0, len(options))
 	for _, option := range options {
 		result = append(result, option.Id)
 	}
 	return result
 }
+
+func TestFileActionDialog_SelectCloseOption_EmitsCloseAction(t *testing.T) {
+	d := &FileActionDialog{actionChannel: make(chan DialogActionId, 1)}
+
+	d.selectAction(&DialogOption{Id: DialogCloseActionId, Name: "Close"})
+
+	select {
+	case action := <-d.actionChannel:
+		assert.Equal(t, DialogCloseActionId, action)
+	case <-time.After(100 * time.Millisecond):
+		t.Fatal("expected close action to be emitted")
+	}
+}

internal/ui/dialog/snapshot_action_dialog.go

@@ -96,6 +96,7 @@ func (d *SnapshotActionDialog) selectAction(option *DialogOption) {
 	case SnapshotDialogDestroySnapshotRecursivelyActionId:
 		d.DestroySnapshotRecursively()
 	case DialogCloseActionId:
+		d.Close()
 	default:
 		d.Close()
 	}

internal/ui/dialog/snapshot_multi_action_dialog.go

@@ -101,6 +101,7 @@ func (d *MultiSnapshotActionDialog) selectAction(option *DialogOption) {
 	case MultiSnapshotDialogDestroySnapshotRecursivelyActionId:
 		d.DestroyAllSnapshotsRecursively()
 	case DialogCloseActionId:
+		d.Close()
 	default:
 		d.Close()
 	}