fix(ui): address PR review follow-ups

commit: ff584d18e50f0359a9793ca653cc5ffd575410d7
author: brice · 8mo ago · 2026-02-10 05:45 UTC
parent: 941c1f6
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

7 files · +61 -2

M internal/ui/app.go +1 -0


@@ -2454,6 +2454,7 @@ func (a *App) releaseFloatingSession(ctx context.Context, key floatingSessionKey
2454	2454		session.overlay = nil
2455	2455		session.webView = nil
2456	2456		session.pane = nil
	2457	+	// Session is fully released/zeroed; caller must drop this reference and not reuse it.
2457	2458		}
2458	2459
2459	2460		func (a *App) updateFloatingSessionURI(paneID entity.PaneID, url string) {


@@ -2454,6 +2454,7 @@ func (a *App) releaseFloatingSession(ctx context.Context, key floatingSessionKey
2454	session.overlay = nil	2454		session.overlay = nil
2455	session.webView = nil	2455		session.webView = nil
2456	session.pane = nil	2456		session.pane = nil
		2457	+	// Session is fully released/zeroed; caller must drop this reference and not reuse it.
2457	}	2458		}
2458		2459
2459	func (a *App) updateFloatingSessionURI(paneID entity.PaneID, url string) {	2460		func (a *App) updateFloatingSessionURI(paneID entity.PaneID, url string) {

M internal/ui/component/floating_pane.go +6 -1


@@ -125,7 +125,12 @@ func (fp *FloatingPane) SessionStarted() bool {
125	125		func (fp *FloatingPane) ShowURL(ctx context.Context, url string) error {
126	126		url = strings.TrimSpace(url)
127	127		if url == "" {
128		-	return fp.ShowToggle(ctx)
	128	+	fp.mu.Lock()
	129	+	fp.visible = true
	130	+	fp.omniboxVisible = true
	131	+	fp.stateVersion++
	132	+	fp.mu.Unlock()
	133	+	return nil
129	134		}
130	135
131	136		fp.mu.Lock()

 func (fp *FloatingPane) ShowURL(ctx context.Context, url string) error {
 	url = strings.TrimSpace(url)
 	if url == "" {
-		return fp.ShowToggle(ctx)
+		fp.mu.Lock()
+		fp.visible = true
+		fp.omniboxVisible = true
+		fp.stateVersion++
+		fp.mu.Unlock()
+		return nil
 	}
 	fp.mu.Lock()

M internal/ui/component/workspace_view.go +6 -0


@@ -557,6 +557,12 @@ func (wv *WorkspaceView) AddWorkspaceOverlayWidget(widget layout.Widget) {
557	557		if parent == wv.overlay {
558	558		return
559	559		}
	560	+
	561	+	parentGTK := parent.GtkWidget()
	562	+	overlayGTK := wv.overlay.GtkWidget()
	563	+	if parentGTK != nil && overlayGTK != nil && parentGTK.GoPointer() == overlayGTK.GoPointer() {
	564	+	return
	565	+	}
560	566		widget.Unparent()
561	567		}
562	568


@@ -557,6 +557,12 @@ func (wv *WorkspaceView) AddWorkspaceOverlayWidget(widget layout.Widget) {
557	if parent == wv.overlay {	557		if parent == wv.overlay {
558	return	558		return
559	}	559		}
		560	+
		561	+	parentGTK := parent.GtkWidget()
		562	+	overlayGTK := wv.overlay.GtkWidget()
		563	+	if parentGTK != nil && overlayGTK != nil && parentGTK.GoPointer() == overlayGTK.GoPointer() {
		564	+	return
		565	+	}
560	widget.Unparent()	566		widget.Unparent()
561	}	567		}
562		568

M internal/ui/component/workspace_view_test.go +32 -0


@@ -4,6 +4,7 @@ import (
4	4		"context"
5	5		"testing"
6	6
	7	+	"github.com/jwijenbergh/puregotk/v4/gtk"
7	8		"github.com/stretchr/testify/assert"
8	9		"github.com/stretchr/testify/mock"
9	10		"github.com/stretchr/testify/require"

@@ -316,6 +317,8 @@ func TestAddWorkspaceOverlayWidget_UnparentsBeforeAdding(t *testing.T) {
316	317		widget := mocks.NewMockWidget(t)
317	318		otherParent := mocks.NewMockWidget(t)
318	319		widget.EXPECT().GetParent().Return(otherParent).Once()
	320	+	otherParent.EXPECT().GtkWidget().Return(nil).Once()
	321	+	mockOverlay.EXPECT().GtkWidget().Return(nil).Once()
319	322		widget.EXPECT().Unparent().Once()
320	323		mockOverlay.EXPECT().AddOverlay(widget).Once()
321	324		mockOverlay.EXPECT().SetClipOverlay(widget, false).Once()

@@ -324,6 +327,35 @@ func TestAddWorkspaceOverlayWidget_UnparentsBeforeAdding(t *testing.T) {
324	327		wv.AddWorkspaceOverlayWidget(widget)
325	328		}
326	329
	330	+	func TestAddWorkspaceOverlayWidget_DoesNotReaddWhenParentGTKMatchesOverlay(t *testing.T) {
	331	+	mockFactory := mocks.NewMockWidgetFactory(t)
	332	+	ctx := context.Background()
	333	+
	334	+	mockContainer := mocks.NewMockBoxWidget(t)
	335	+	mockOverlay := mocks.NewMockOverlayWidget(t)
	336	+	mockFactory.EXPECT().NewBox(layout.OrientationVertical, 0).Return(mockContainer).Once()
	337	+	mockContainer.EXPECT().SetHexpand(true).Once()
	338	+	mockContainer.EXPECT().SetVexpand(true).Once()
	339	+	mockContainer.EXPECT().SetVisible(true).Once()
	340	+	mockFactory.EXPECT().NewOverlay().Return(mockOverlay).Once()
	341	+	mockOverlay.EXPECT().SetHexpand(true).Once()
	342	+	mockOverlay.EXPECT().SetVexpand(true).Once()
	343	+	mockOverlay.EXPECT().SetChild(mockContainer).Once()
	344	+	mockOverlay.EXPECT().SetVisible(true).Once()
	345	+
	346	+	wv := component.NewWorkspaceView(ctx, mockFactory)
	347	+
	348	+	widget := mocks.NewMockWidget(t)
	349	+	otherParent := mocks.NewMockWidget(t)
	350	+	gtkWidget := &gtk.Widget{}
	351	+
	352	+	widget.EXPECT().GetParent().Return(otherParent).Once()
	353	+	otherParent.EXPECT().GtkWidget().Return(gtkWidget).Once()
	354	+	mockOverlay.EXPECT().GtkWidget().Return(gtkWidget).Once()
	355	+
	356	+	wv.AddWorkspaceOverlayWidget(widget)
	357	+	}
	358	+
327	359		func TestSetActivePaneID_UpdatesStyling(t *testing.T) {
328	360		// Arrange
329	361		mockFactory := mocks.NewMockWidgetFactory(t)


@@ -4,6 +4,7 @@ import (
4	"context"	4		"context"
5	"testing"	5		"testing"
6		6
		7	+	"github.com/jwijenbergh/puregotk/v4/gtk"
7	"github.com/stretchr/testify/assert"	8		"github.com/stretchr/testify/assert"
8	"github.com/stretchr/testify/mock"	9		"github.com/stretchr/testify/mock"
9	"github.com/stretchr/testify/require"	10		"github.com/stretchr/testify/require"

@@ -316,6 +317,8 @@ func TestAddWorkspaceOverlayWidget_UnparentsBeforeAdding(t *testing.T) {
316	widget := mocks.NewMockWidget(t)	317		widget := mocks.NewMockWidget(t)
317	otherParent := mocks.NewMockWidget(t)	318		otherParent := mocks.NewMockWidget(t)
318	widget.EXPECT().GetParent().Return(otherParent).Once()	319		widget.EXPECT().GetParent().Return(otherParent).Once()
		320	+	otherParent.EXPECT().GtkWidget().Return(nil).Once()
		321	+	mockOverlay.EXPECT().GtkWidget().Return(nil).Once()
319	widget.EXPECT().Unparent().Once()	322		widget.EXPECT().Unparent().Once()
320	mockOverlay.EXPECT().AddOverlay(widget).Once()	323		mockOverlay.EXPECT().AddOverlay(widget).Once()
321	mockOverlay.EXPECT().SetClipOverlay(widget, false).Once()	324		mockOverlay.EXPECT().SetClipOverlay(widget, false).Once()

@@ -324,6 +327,35 @@ func TestAddWorkspaceOverlayWidget_UnparentsBeforeAdding(t *testing.T) {
324	wv.AddWorkspaceOverlayWidget(widget)	327		wv.AddWorkspaceOverlayWidget(widget)
325	}	328		}
326		329
		330	+	func TestAddWorkspaceOverlayWidget_DoesNotReaddWhenParentGTKMatchesOverlay(t *testing.T) {
		331	+	mockFactory := mocks.NewMockWidgetFactory(t)
		332	+	ctx := context.Background()
		333	+
		334	+	mockContainer := mocks.NewMockBoxWidget(t)
		335	+	mockOverlay := mocks.NewMockOverlayWidget(t)
		336	+	mockFactory.EXPECT().NewBox(layout.OrientationVertical, 0).Return(mockContainer).Once()
		337	+	mockContainer.EXPECT().SetHexpand(true).Once()
		338	+	mockContainer.EXPECT().SetVexpand(true).Once()
		339	+	mockContainer.EXPECT().SetVisible(true).Once()
		340	+	mockFactory.EXPECT().NewOverlay().Return(mockOverlay).Once()
		341	+	mockOverlay.EXPECT().SetHexpand(true).Once()
		342	+	mockOverlay.EXPECT().SetVexpand(true).Once()
		343	+	mockOverlay.EXPECT().SetChild(mockContainer).Once()
		344	+	mockOverlay.EXPECT().SetVisible(true).Once()
		345	+
		346	+	wv := component.NewWorkspaceView(ctx, mockFactory)
		347	+
		348	+	widget := mocks.NewMockWidget(t)
		349	+	otherParent := mocks.NewMockWidget(t)
		350	+	gtkWidget := &gtk.Widget{}
		351	+
		352	+	widget.EXPECT().GetParent().Return(otherParent).Once()
		353	+	otherParent.EXPECT().GtkWidget().Return(gtkWidget).Once()
		354	+	mockOverlay.EXPECT().GtkWidget().Return(gtkWidget).Once()
		355	+
		356	+	wv.AddWorkspaceOverlayWidget(widget)
		357	+	}
		358	+
327	func TestSetActivePaneID_UpdatesStyling(t *testing.T) {	359		func TestSetActivePaneID_UpdatesStyling(t *testing.T) {
328	// Arrange	360		// Arrange
329	mockFactory := mocks.NewMockWidgetFactory(t)	361		mockFactory := mocks.NewMockWidgetFactory(t)

M internal/ui/coordinator/content_active_override_test.go +6 -0


@@ -12,6 +12,8 @@ import (
12	12		)
13	13
14	14		func TestContentCoordinator_ActiveWebView_UsesOverride(t *testing.T) {
	15	+	t.Parallel()
	16	+
15	17		mainWV := &webkit.WebView{}
16	18		floatingWV := &webkit.WebView{}
17	19

@@ -29,6 +31,8 @@ func TestContentCoordinator_ActiveWebView_UsesOverride(t *testing.T) {
29	31		}
30	32
31	33		func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBack(t *testing.T) {
	34	+	t.Parallel()
	35	+
32	36		mainWV := &webkit.WebView{}
33	37
34	38		c := &ContentCoordinator{

@@ -45,6 +49,8 @@ func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBack(t *testing.T) {
45	49		}
46	50
47	51		func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBackToWorkspace(t *testing.T) {
	52	+	t.Parallel()
	53	+
48	54		ctx := context.Background()
49	55		mainWV := &webkit.WebView{}
50	56		mainPane := entity.NewPane(entity.PaneID("main-pane"))


@@ -12,6 +12,8 @@ import (
12	)	12		)
13		13
14	func TestContentCoordinator_ActiveWebView_UsesOverride(t *testing.T) {	14		func TestContentCoordinator_ActiveWebView_UsesOverride(t *testing.T) {
		15	+	t.Parallel()
		16	+
15	mainWV := &webkit.WebView{}	17		mainWV := &webkit.WebView{}
16	floatingWV := &webkit.WebView{}	18		floatingWV := &webkit.WebView{}
17		19

@@ -29,6 +31,8 @@ func TestContentCoordinator_ActiveWebView_UsesOverride(t *testing.T) {
29	}	31		}
30		32
31	func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBack(t *testing.T) {	33		func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBack(t *testing.T) {
		34	+	t.Parallel()
		35	+
32	mainWV := &webkit.WebView{}	36		mainWV := &webkit.WebView{}
33		37
34	c := &ContentCoordinator{	38		c := &ContentCoordinator{

@@ -45,6 +49,8 @@ func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBack(t *testing.T) {
45	}	49		}
46		50
47	func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBackToWorkspace(t *testing.T) {	51		func TestContentCoordinator_ActiveWebView_ClearOverrideFallsBackToWorkspace(t *testing.T) {
		52	+	t.Parallel()
		53	+
48	ctx := context.Background()	54		ctx := context.Background()
49	mainWV := &webkit.WebView{}	55		mainWV := &webkit.WebView{}
50	mainPane := entity.NewPane(entity.PaneID("main-pane"))	56		mainPane := entity.NewPane(entity.PaneID("main-pane"))

M internal/ui/input/floating_shortcuts_test.go +9 -0


@@ -192,3 +192,12 @@ func TestShortcutSet_FloatingProfiles_SupportModifierCombos(t *testing.T) {
192	192		assert.Equal(t, "search", searchTarget.SessionID)
193	193		assert.Equal(t, "https://search.example.com", searchTarget.URL)
194	194		}
	195	+
	196	+	func TestParseFloatingProfileTarget_AllowsPipeInURL(t *testing.T) {
	197	+	action := NewFloatingProfileAction("pipe-test", "https://example.com/a\|b")
	198	+
	199	+	target, ok := ParseFloatingProfileTarget(action)
	200	+	require.True(t, ok)
	201	+	assert.Equal(t, "pipe-test", target.SessionID)
	202	+	assert.Equal(t, "https://example.com/a\|b", target.URL)
	203	+	}


@@ -192,3 +192,12 @@ func TestShortcutSet_FloatingProfiles_SupportModifierCombos(t *testing.T) {
192	assert.Equal(t, "search", searchTarget.SessionID)	192		assert.Equal(t, "search", searchTarget.SessionID)
193	assert.Equal(t, "https://search.example.com", searchTarget.URL)	193		assert.Equal(t, "https://search.example.com", searchTarget.URL)
194	}	194		}
		195	+
		196	+	func TestParseFloatingProfileTarget_AllowsPipeInURL(t *testing.T) {
		197	+	action := NewFloatingProfileAction("pipe-test", "https://example.com/a\|b")
		198	+
		199	+	target, ok := ParseFloatingProfileTarget(action)
		200	+	require.True(t, ok)
		201	+	assert.Equal(t, "pipe-test", target.SessionID)
		202	+	assert.Equal(t, "https://example.com/a\|b", target.URL)
		203	+	}

M internal/ui/input/shortcuts.go +1 -1


@@ -199,7 +199,7 @@ const (
199	199		)
200	200
201	201		const floatingProfileActionPrefix = "open_floating_profile:"
202		-	const floatingProfileActionSeparator = "\|"
	202	+	const floatingProfileActionSeparator = "\u241F"
203	203
204	204		// ShortcutTable maps KeyBinding to Action.
205	205		type ShortcutTable map[KeyBinding]Action


@@ -199,7 +199,7 @@ const (
199		)	199		)
200			200
201		const floatingProfileActionPrefix = "open_floating_profile:"	201		const floatingProfileActionPrefix = "open_floating_profile:"
202	-	const floatingProfileActionSeparator = "\|"	202	+	const floatingProfileActionSeparator = "\u241F"
203			203
204		// ShortcutTable maps KeyBinding to Action.	204		// ShortcutTable maps KeyBinding to Action.
205		type ShortcutTable map[KeyBinding]Action	205		type ShortcutTable map[KeyBinding]Action