Merge pull request #317 from bnema/backlog/correctness-move-targets

fix(usecase): preserve pane state on failed moves

commit: c0d056e6a8cd61faa37a11daaee2483524b4e0d0
author: bnema · 4mo ago · 2026-06-20 13:01 UTC
committer: GitHub · 4mo ago · 2026-06-20 13:01 UTC
merge of: cfbaa81 + b7d0e21 · diff against the first parent
signature: GPG UnverifiedB5690EEEBB952194 The key is unknown

2 files · +147 -7

M internal/application/usecase/move_pane_to_tab.go +29 -7


@@ -46,12 +46,9 @@ func (uc MovePaneToTabUseCase) Execute(input MovePaneToTabInput) (MovePaneToTa
46	46		return nil, err
47	47		}
48	48
49		-	if detachErr := detachPaneFromWorkspace(sourceTab.Workspace, sourceNode); detachErr != nil {
50		-	return nil, detachErr
51		-	}
52		-
53		-	sourceTabClosed := closeSourceTabIfEmpty(input.TabList, sourceTab)
54		-
	49	+	// CORRECTNESS-02: Resolve/prepare the target BEFORE mutating source.
	50	+	// This ensures that target resolution (idGenerator check for new tabs)
	51	+	// and insertion validation happen while source state is still intact.
55	52		targetTab, newTabCreated, err := uc.resolveTargetTab(input.TabList, input.TargetTabID, movedPane)
56	53		if err != nil {
57	54		return nil, err

@@ -60,7 +57,29 @@ func (uc MovePaneToTabUseCase) Execute(input MovePaneToTabInput) (MovePaneToTa
60	57		return nil, fmt.Errorf("target tab/workspace is nil")
61	58		}
62	59
	60	+	// Pre-validate insertion prerequisites before mutating source.
	61	+	// Insertion into an existing non-empty workspace requires idGenerator
	62	+	// for creating split parent nodes (for non-empty targets with Root != nil),
	63	+	// and requires a valid active pane. Fail early if either is unavailable.
	64	+	if !newTabCreated && targetTab.Workspace.Root != nil {
	65	+	if uc.idGenerator == nil {
	66	+	return nil, fmt.Errorf("id generator is required to insert pane into existing workspace")
	67	+	}
	68	+	activePane := targetTab.Workspace.ActivePane()
	69	+	if activePane == nil \|\| activePane.Pane == nil {
	70	+	return nil, fmt.Errorf("target tab has no active pane")
	71	+	}
	72	+	}
	73	+
	74	+	// Now mutate source — pane is detached from original location.
	75	+	if detachErr := detachPaneFromWorkspace(sourceTab.Workspace, sourceNode); detachErr != nil {
	76	+	return nil, detachErr
	77	+	}
	78	+
	79	+	sourceTabClosed := closeSourceTabIfEmpty(input.TabList, sourceTab)
	80	+
63	81		if newTabCreated {
	82	+	input.TabList.Add(targetTab)
64	83		return &MovePaneToTabOutput{
65	84		TargetTab: targetTab,
66	85		MovedPaneNode: targetTab.Workspace.Root,

@@ -158,7 +177,10 @@ func (uc *MovePaneToTabUseCase) resolveTargetTab(
158	177		tabID := entity.TabID(uc.idGenerator())
159	178		wsID := entity.WorkspaceID(uc.idGenerator())
160	179		targetTab := entity.NewTab(tabID, wsID, movedPane)
161		-	tl.Add(targetTab)
	180	+	// NOTE: tab is NOT added to TabList here. Addition is deferred until
	181	+	// after successful detach in Execute() (see newTabCreated path).
	182	+	// This prevents leaving a stale/duplicate tab in the list if detach fails
	183	+	// (CORRECTNESS-02).
162	184		return targetTab, true, nil
163	185		}
164	186

 		return nil, err
 	}
-	if detachErr := detachPaneFromWorkspace(sourceTab.Workspace, sourceNode); detachErr != nil {
+	// CORRECTNESS-02: Resolve/prepare the target BEFORE mutating source.
-		return nil, detachErr
+	// This ensures that target resolution (idGenerator check for new tabs)
-	}
+	// and insertion validation happen while source state is still intact.
-	sourceTabClosed := closeSourceTabIfEmpty(input.TabList, sourceTab)
 	targetTab, newTabCreated, err := uc.resolveTargetTab(input.TabList, input.TargetTabID, movedPane)
 	if err != nil {
 		return nil, err
 		return nil, fmt.Errorf("target tab/workspace is nil")
 	}
+	// Pre-validate insertion prerequisites before mutating source.
+	// Insertion into an existing non-empty workspace requires idGenerator
+	// for creating split parent nodes (for non-empty targets with Root != nil),
+	// and requires a valid active pane. Fail early if either is unavailable.
+	if !newTabCreated && targetTab.Workspace.Root != nil {
+		if uc.idGenerator == nil {
+			return nil, fmt.Errorf("id generator is required to insert pane into existing workspace")
+		}
+		activePane := targetTab.Workspace.ActivePane()
+		if activePane == nil || activePane.Pane == nil {
+			return nil, fmt.Errorf("target tab has no active pane")
+		}
+	}
+	// Now mutate source — pane is detached from original location.
+	if detachErr := detachPaneFromWorkspace(sourceTab.Workspace, sourceNode); detachErr != nil {
+		return nil, detachErr
+	}
+	sourceTabClosed := closeSourceTabIfEmpty(input.TabList, sourceTab)
 	if newTabCreated {
+		input.TabList.Add(targetTab)
 		return &MovePaneToTabOutput{
 			TargetTab:       targetTab,
 			MovedPaneNode:   targetTab.Workspace.Root,
 	tabID := entity.TabID(uc.idGenerator())
 	wsID := entity.WorkspaceID(uc.idGenerator())
 	targetTab := entity.NewTab(tabID, wsID, movedPane)
-	tl.Add(targetTab)
+	// NOTE: tab is NOT added to TabList here. Addition is deferred until
+	// after successful detach in Execute() (see newTabCreated path).
+	// This prevents leaving a stale/duplicate tab in the list if detach fails
+	// (CORRECTNESS-02).
 	return targetTab, true, nil
 }

M internal/application/usecase/move_pane_to_tab_test.go +118 -0


@@ -191,6 +191,124 @@ func TestMovePaneToTab_InsertsRightOfActiveEvenWhenActiveInStack(t *testing.T) {
191	191		require.Equal(t, paneA.ID, out.TargetTab.Workspace.Root.Right().Pane.ID)
192	192		}
193	193
	194	+	// CORRECTNESS-02: Resolve move targets before detaching panes.
	195	+	//
	196	+	// When a pane move fails after source detach (e.g., nil idGenerator prevents
	197	+	// insertion into an existing workspace), the source tab must retain its pane.
	198	+	// Previously, detachPaneFromWorkspace was called before resolveTargetTab and
	199	+	// insertIntoTargetWorkspace, so a failure in either would leave the pane
	200	+	// detached from its original location while returning an error.
	201	+	func TestMovePaneToTab_FailedMovePreservesSourcePane(t *testing.T) {
	202	+	// Source tab with a single pane.
	203	+	tabs := entity.NewTabList()
	204	+	paneA := entity.NewPane(entity.PaneID("pA"))
	205	+	tabA := entity.NewTab(entity.TabID("tA"), entity.WorkspaceID("wA"), paneA)
	206	+	tabs.Add(tabA)
	207	+
	208	+	// Target tab with an existing pane (non-nil Root), so insertion needs idGenerator.
	209	+	paneB := entity.NewPane(entity.PaneID("pB"))
	210	+	tabB := entity.NewTab(entity.TabID("tB"), entity.WorkspaceID("wB"), paneB)
	211	+	tabs.Add(tabB)
	212	+	tabB.Workspace.ActivePaneID = paneB.ID
	213	+
	214	+	// Use nil idGenerator — insertion into existing workspace will fail.
	215	+	uc := NewMovePaneToTabUseCase(nil)
	216	+
	217	+	_, err := uc.Execute(MovePaneToTabInput{
	218	+	TabList: tabs,
	219	+	SourceTabID: tabA.ID,
	220	+	SourcePaneID: paneA.ID,
	221	+	TargetTabID: tabB.ID,
	222	+	})
	223	+	require.Error(t, err)
	224	+	require.Contains(t, err.Error(), "id generator is required")
	225	+
	226	+	// Source must be preserved: tab still exists and pane is still inside it.
	227	+	require.NotNil(t, tabs.Find(tabA.ID), "source tab must not be removed on failed move")
	228	+	require.NotNil(t, tabA.Workspace.FindPane(paneA.ID), "source pane must not be detached on failed move")
	229	+	}
	230	+
	231	+	// CORRECTNESS-02: Pre-validate that target workspace has a valid active pane
	232	+	// before detaching source. When a target workspace has Root != nil but
	233	+	// ActivePane() returns nil (e.g., ActivePaneID points to a non-existent pane),
	234	+	// insertIntoTargetWorkspace would fail after detach. Pre-validation catches
	235	+	// this early.
	236	+	func TestMovePaneToTab_FailsOnTargetWithNoActivePane(t *testing.T) {
	237	+	id := newTestIDGen()
	238	+	uc := NewMovePaneToTabUseCase(id)
	239	+
	240	+	tabs := entity.NewTabList()
	241	+
	242	+	// Source tab.
	243	+	paneA := entity.NewPane(entity.PaneID("pA"))
	244	+	tabA := entity.NewTab(entity.TabID("tA"), entity.WorkspaceID("wA"), paneA)
	245	+	tabs.Add(tabA)
	246	+
	247	+	// Target tab with Root != nil but ActivePaneID pointing to a non-existent pane.
	248	+	paneB := entity.NewPane(entity.PaneID("pB"))
	249	+	tabB := entity.NewTab(entity.TabID("tB"), entity.WorkspaceID("wB"), paneB)
	250	+	tabB.Workspace.ActivePaneID = "nonexistent"
	251	+	tabs.Add(tabB)
	252	+
	253	+	_, err := uc.Execute(MovePaneToTabInput{
	254	+	TabList: tabs,
	255	+	SourceTabID: tabA.ID,
	256	+	SourcePaneID: paneA.ID,
	257	+	TargetTabID: tabB.ID,
	258	+	})
	259	+	require.Error(t, err)
	260	+	require.Contains(t, err.Error(), "no active pane")
	261	+
	262	+	// Source must be preserved.
	263	+	require.NotNil(t, tabs.Find(tabA.ID), "source tab must not be removed on failed move")
	264	+	require.NotNil(t, tabA.Workspace.FindPane(paneA.ID), "source pane must not be detached on failed move")
	265	+	}
	266	+
	267	+	// CORRECTNESS-02: New tab must not be added to TabList when detach fails.
	268	+	// When create-new-tab is requested (empty TargetTabID), resolveTargetTab
	269	+	// generates IDs and creates the tab but must NOT add it to TabList until
	270	+	// after successful detach from source. This test verifies that a detach
	271	+	// failure (e.g., pane's parent is not a valid split) does not leave a
	272	+	// stale/duplicate tab in the list.
	273	+	func TestMovePaneToTab_NewTabNotAddedOnDetachFailure(t *testing.T) {
	274	+	id := newTestIDGen()
	275	+	uc := NewMovePaneToTabUseCase(id)
	276	+
	277	+	tabs := entity.NewTabList()
	278	+
	279	+	paneA := entity.NewPane(entity.PaneID("pA"))
	280	+	tabA := entity.NewTab(entity.TabID("tA"), entity.WorkspaceID("wA"), paneA)
	281	+	tabs.Add(tabA)
	282	+
	283	+	// Construct an invalid parent structure so detach fails:
	284	+	// Make paneA's PaneNode a child of a non-split container node.
	285	+	// detachLeafFromWorkspace will reject because parent.IsSplit() is false.
	286	+	invalidParent := &entity.PaneNode{
	287	+	ID: "invalid-parent",
	288	+	Children: []*entity.PaneNode{tabA.Workspace.Root},
	289	+	// SplitDir is deliberately SplitNone (zero value), so IsSplit() returns false.
	290	+	}
	291	+	tabA.Workspace.Root.Parent = invalidParent
	292	+	tabA.Workspace.Root = invalidParent
	293	+
	294	+	initialCount := tabs.Count()
	295	+
	296	+	_, err := uc.Execute(MovePaneToTabInput{
	297	+	TabList: tabs,
	298	+	SourceTabID: tabA.ID,
	299	+	SourcePaneID: paneA.ID,
	300	+	TargetTabID: "", // creates new tab
	301	+	})
	302	+	require.Error(t, err)
	303	+	require.Contains(t, err.Error(), "pane parent is not a split")
	304	+
	305	+	// No new tab should have been added to the list.
	306	+	require.Equal(t, initialCount, tabs.Count(), "no new tab should be added on failed move")
	307	+	// Source tab and pane should still be in their original positions.
	308	+	require.NotNil(t, tabs.Find(tabA.ID), "source tab must not be removed on failed move")
	309	+	require.NotNil(t, tabA.Workspace.FindPane(paneA.ID), "source pane must not be detached on failed move")
	310	+	}
	311	+
194	312		func newTestIDGen() func() string {
195	313		counter := 0
196	314		return func() string {


@@ -191,6 +191,124 @@ func TestMovePaneToTab_InsertsRightOfActiveEvenWhenActiveInStack(t *testing.T) {
191	require.Equal(t, paneA.ID, out.TargetTab.Workspace.Root.Right().Pane.ID)	191		require.Equal(t, paneA.ID, out.TargetTab.Workspace.Root.Right().Pane.ID)
192	}	192		}
193		193
		194	+	// CORRECTNESS-02: Resolve move targets before detaching panes.
		195	+	//
		196	+	// When a pane move fails after source detach (e.g., nil idGenerator prevents
		197	+	// insertion into an existing workspace), the source tab must retain its pane.
		198	+	// Previously, detachPaneFromWorkspace was called before resolveTargetTab and
		199	+	// insertIntoTargetWorkspace, so a failure in either would leave the pane
		200	+	// detached from its original location while returning an error.
		201	+	func TestMovePaneToTab_FailedMovePreservesSourcePane(t *testing.T) {
		202	+	// Source tab with a single pane.
		203	+	tabs := entity.NewTabList()
		204	+	paneA := entity.NewPane(entity.PaneID("pA"))
		205	+	tabA := entity.NewTab(entity.TabID("tA"), entity.WorkspaceID("wA"), paneA)
		206	+	tabs.Add(tabA)
		207	+
		208	+	// Target tab with an existing pane (non-nil Root), so insertion needs idGenerator.
		209	+	paneB := entity.NewPane(entity.PaneID("pB"))
		210	+	tabB := entity.NewTab(entity.TabID("tB"), entity.WorkspaceID("wB"), paneB)
		211	+	tabs.Add(tabB)
		212	+	tabB.Workspace.ActivePaneID = paneB.ID
		213	+
		214	+	// Use nil idGenerator — insertion into existing workspace will fail.
		215	+	uc := NewMovePaneToTabUseCase(nil)
		216	+
		217	+	_, err := uc.Execute(MovePaneToTabInput{
		218	+	TabList: tabs,
		219	+	SourceTabID: tabA.ID,
		220	+	SourcePaneID: paneA.ID,
		221	+	TargetTabID: tabB.ID,
		222	+	})
		223	+	require.Error(t, err)
		224	+	require.Contains(t, err.Error(), "id generator is required")
		225	+
		226	+	// Source must be preserved: tab still exists and pane is still inside it.
		227	+	require.NotNil(t, tabs.Find(tabA.ID), "source tab must not be removed on failed move")
		228	+	require.NotNil(t, tabA.Workspace.FindPane(paneA.ID), "source pane must not be detached on failed move")
		229	+	}
		230	+
		231	+	// CORRECTNESS-02: Pre-validate that target workspace has a valid active pane
		232	+	// before detaching source. When a target workspace has Root != nil but
		233	+	// ActivePane() returns nil (e.g., ActivePaneID points to a non-existent pane),
		234	+	// insertIntoTargetWorkspace would fail after detach. Pre-validation catches
		235	+	// this early.
		236	+	func TestMovePaneToTab_FailsOnTargetWithNoActivePane(t *testing.T) {
		237	+	id := newTestIDGen()
		238	+	uc := NewMovePaneToTabUseCase(id)
		239	+
		240	+	tabs := entity.NewTabList()
		241	+
		242	+	// Source tab.
		243	+	paneA := entity.NewPane(entity.PaneID("pA"))
		244	+	tabA := entity.NewTab(entity.TabID("tA"), entity.WorkspaceID("wA"), paneA)
		245	+	tabs.Add(tabA)
		246	+
		247	+	// Target tab with Root != nil but ActivePaneID pointing to a non-existent pane.
		248	+	paneB := entity.NewPane(entity.PaneID("pB"))
		249	+	tabB := entity.NewTab(entity.TabID("tB"), entity.WorkspaceID("wB"), paneB)
		250	+	tabB.Workspace.ActivePaneID = "nonexistent"
		251	+	tabs.Add(tabB)
		252	+
		253	+	_, err := uc.Execute(MovePaneToTabInput{
		254	+	TabList: tabs,
		255	+	SourceTabID: tabA.ID,
		256	+	SourcePaneID: paneA.ID,
		257	+	TargetTabID: tabB.ID,
		258	+	})
		259	+	require.Error(t, err)
		260	+	require.Contains(t, err.Error(), "no active pane")
		261	+
		262	+	// Source must be preserved.
		263	+	require.NotNil(t, tabs.Find(tabA.ID), "source tab must not be removed on failed move")
		264	+	require.NotNil(t, tabA.Workspace.FindPane(paneA.ID), "source pane must not be detached on failed move")
		265	+	}
		266	+
		267	+	// CORRECTNESS-02: New tab must not be added to TabList when detach fails.
		268	+	// When create-new-tab is requested (empty TargetTabID), resolveTargetTab
		269	+	// generates IDs and creates the tab but must NOT add it to TabList until
		270	+	// after successful detach from source. This test verifies that a detach
		271	+	// failure (e.g., pane's parent is not a valid split) does not leave a
		272	+	// stale/duplicate tab in the list.
		273	+	func TestMovePaneToTab_NewTabNotAddedOnDetachFailure(t *testing.T) {
		274	+	id := newTestIDGen()
		275	+	uc := NewMovePaneToTabUseCase(id)
		276	+
		277	+	tabs := entity.NewTabList()
		278	+
		279	+	paneA := entity.NewPane(entity.PaneID("pA"))
		280	+	tabA := entity.NewTab(entity.TabID("tA"), entity.WorkspaceID("wA"), paneA)
		281	+	tabs.Add(tabA)
		282	+
		283	+	// Construct an invalid parent structure so detach fails:
		284	+	// Make paneA's PaneNode a child of a non-split container node.
		285	+	// detachLeafFromWorkspace will reject because parent.IsSplit() is false.
		286	+	invalidParent := &entity.PaneNode{
		287	+	ID: "invalid-parent",
		288	+	Children: []*entity.PaneNode{tabA.Workspace.Root},
		289	+	// SplitDir is deliberately SplitNone (zero value), so IsSplit() returns false.
		290	+	}
		291	+	tabA.Workspace.Root.Parent = invalidParent
		292	+	tabA.Workspace.Root = invalidParent
		293	+
		294	+	initialCount := tabs.Count()
		295	+
		296	+	_, err := uc.Execute(MovePaneToTabInput{
		297	+	TabList: tabs,
		298	+	SourceTabID: tabA.ID,
		299	+	SourcePaneID: paneA.ID,
		300	+	TargetTabID: "", // creates new tab
		301	+	})
		302	+	require.Error(t, err)
		303	+	require.Contains(t, err.Error(), "pane parent is not a split")
		304	+
		305	+	// No new tab should have been added to the list.
		306	+	require.Equal(t, initialCount, tabs.Count(), "no new tab should be added on failed move")
		307	+	// Source tab and pane should still be in their original positions.
		308	+	require.NotNil(t, tabs.Find(tabA.ID), "source tab must not be removed on failed move")
		309	+	require.NotNil(t, tabA.Workspace.FindPane(paneA.ID), "source pane must not be detached on failed move")
		310	+	}
		311	+
194	func newTestIDGen() func() string {	312		func newTestIDGen() func() string {
195	counter := 0	313		counter := 0
196	return func() string {	314		return func() string {