646bc9f fix(ui): address external link review findings · dumber

M cmd/dumber/main.go +1 -1


@@ -298,7 +298,7 @@ func runGUI(cfg *config.Config, timing startupTiming) int {
298	298		if relaunchSetter, ok := engine.(port.AlreadyRunningAppRelaunchHandlerSetter); ok {
299	299		relaunchSetter.SetAlreadyRunningAppRelaunchHandler(func(url string) {
300	300		if err := app.OpenExternalURL(ctx, url); err != nil {
301		-	log.Warn().Err(err).Str("url", url).Msg("failed to open relaunch browser window")
	301	+	log.Warn().Err(err).Str("url", url).Msg("failed to open relaunch external URL")
302	302		}
303	303		})
304	304		}


@@ -298,7 +298,7 @@ func runGUI(cfg *config.Config, timing startupTiming) int {
298		if relaunchSetter, ok := engine.(port.AlreadyRunningAppRelaunchHandlerSetter); ok {	298		if relaunchSetter, ok := engine.(port.AlreadyRunningAppRelaunchHandlerSetter); ok {
299		relaunchSetter.SetAlreadyRunningAppRelaunchHandler(func(url string) {	299		relaunchSetter.SetAlreadyRunningAppRelaunchHandler(func(url string) {
300		if err := app.OpenExternalURL(ctx, url); err != nil {	300		if err := app.OpenExternalURL(ctx, url); err != nil {
301	-	log.Warn().Err(err).Str("url", url).Msg("failed to open relaunch browser window")	301	+	log.Warn().Err(err).Str("url", url).Msg("failed to open relaunch external URL")
302		}	302		}
303		})	303		})
304		}	304		}

M docs/reference/configuration.md +2 -3

@@ -1,5 +1,7 @@
1	1		# Configuration Reference
2	2
	3	+	`workspace.external_links` controls URLs opened from another application. Non-`windowed` modes target the last-focused window in the current Dumber profile and engine; if that destination is unavailable, Dumber falls back to opening a new window. `placement` applies only to `split`.
	4	+
3	5		\| Key \| Type \| Default \| Valid Values \|
4	6		\|-----\|------\|---------\|--------------\|
5	7		\| `database.path` \| string \| `~/.local/share/dumber/dumber.db` \| \|

@@ -123,9 +125,6 @@
123	125		\| `workspace.browsing_contexts.blank_target_behavior` \| string \| `stacked` \| `split`, `stacked`, `tabbed` \|
124	126		\| `workspace.browsing_contexts.enable_smart_detection` \| bool \| `true` \| \|
125	127		\| `workspace.browsing_contexts.oauth_auto_close` \| bool \| `true` \| \|
126		-
127		-	`workspace.external_links` controls URLs opened from another application. Non-`windowed` modes target the last-focused window in the current Dumber profile and engine; if that destination is unavailable, Dumber falls back to opening a new window. `placement` applies only to `split`.
128		-
129	128		\| `workspace.styling.border_width` \| int \| `1` \| \|
130	129		\| `workspace.styling.border_color` \| string \| `@theme_selected_bg_color` \| \|
131	130		\| `workspace.styling.mode_border_width` \| int \| `4` \| \|

 # Configuration Reference
+`workspace.external_links` controls URLs opened from another application. Non-`windowed` modes target the last-focused window in the current Dumber profile and engine; if that destination is unavailable, Dumber falls back to opening a new window. `placement` applies only to `split`.
 | Key | Type | Default | Valid Values |
 |-----|------|---------|--------------|
 | `database.path` | string | `~/.local/share/dumber/dumber.db` | |
 | `workspace.browsing_contexts.blank_target_behavior` | string | `stacked` | `split`, `stacked`, `tabbed` |
 | `workspace.browsing_contexts.enable_smart_detection` | bool | `true` | |
 | `workspace.browsing_contexts.oauth_auto_close` | bool | `true` | |
-`workspace.external_links` controls URLs opened from another application. Non-`windowed` modes target the last-focused window in the current Dumber profile and engine; if that destination is unavailable, Dumber falls back to opening a new window. `placement` applies only to `split`.
 | `workspace.styling.border_width` | int | `1` | |
 | `workspace.styling.border_color` | string | `@theme_selected_bg_color` | |
 | `workspace.styling.mode_border_width` | int | `4` | |

M internal/ui/browser_window.go +5 -2


@@ -634,7 +634,6 @@ func (a *App) openExternalURLInFocusedWindow(ctx context.Context, url string, cf
634	634		return err
635	635		}
636	636
637		-	a.activateBrowserWindow(target)
638	637		switch cfg.Behavior {
639	638		case entity.ExternalLinkBehaviorTabbed:
640	639		if err := a.createExternalURLTab(ctx, target, url); err != nil {

@@ -710,7 +709,11 @@ func (a App) placeExternalURLInPane(ctx context.Context, ws entity.Workspace,
710	709		}
711	710
712	711		func (a App) rollbackExternalPaneCreation(ctx context.Context, ws entity.Workspace, previous entity.PaneID) {
713		-	if a.panesUC == nil \|\| ws == nil \|\| ws.ActivePaneID == "" \|\| ws.ActivePaneID == previous {
	712	+	if a.panesUC == nil {
	713	+	logging.FromContext(ctx).Warn().Msg("ui: cannot roll back external URL pane; panes use case is unavailable")
	714	+	return
	715	+	}
	716	+	if ws == nil \|\| ws.ActivePaneID == "" \|\| ws.ActivePaneID == previous {
714	717		return
715	718		}
716	719		created := ws.ActivePane()

 		return err
 	}
-	a.activateBrowserWindow(target)
 	switch cfg.Behavior {
 	case entity.ExternalLinkBehaviorTabbed:
 		if err := a.createExternalURLTab(ctx, target, url); err != nil {
 }
 func (a *App) rollbackExternalPaneCreation(ctx context.Context, ws *entity.Workspace, previous entity.PaneID) {
-	if a.panesUC == nil || ws == nil || ws.ActivePaneID == "" || ws.ActivePaneID == previous {
+	if a.panesUC == nil {
+		logging.FromContext(ctx).Warn().Msg("ui: cannot roll back external URL pane; panes use case is unavailable")
+		return
+	}
+	if ws == nil || ws.ActivePaneID == "" || ws.ActivePaneID == previous {
 		return
 	}
 	created := ws.ActivePane()

M internal/ui/coordinator/workspace.go +16 -3


@@ -1490,6 +1490,7 @@ func (c *WorkspaceCoordinator) StackPaneWithURL(ctx context.Context, initialURL
1490	1490		addErr := c.stackedPaneMgr.AddPaneToStack(ctx, stackCtx.wsView, stackCtx.activePaneID, newPaneView, defaultPaneTitle)
1491	1491		if addErr != nil {
1492	1492		log.Error().Err(addErr).Msg("failed to add pane to stack")
	1493	+	c.rollbackStackPane(ctx, stackCtx, newPaneID)
1493	1494		return addErr
1494	1495		}
1495	1496

@@ -1506,21 +1507,24 @@ func (c *WorkspaceCoordinator) StackPaneWithURL(ctx context.Context, initialURL
1506	1507		}
1507	1508		widget := c.contentCoord.WrapWidget(ctx, wv)
1508	1509		if widget == nil {
1509		-	return fmt.Errorf("wrap webview for stacked pane")
	1510	+	err := fmt.Errorf("wrap webview for stacked pane")
	1511	+	c.rollbackStackPane(ctx, stackCtx, newPaneID)
	1512	+	return err
1510	1513		}
1511	1514		if err := stackCtx.wsView.SetWebViewWidget(newPaneID, widget); err != nil {
1512	1515		log.Warn().Err(err).Str("pane_id", string(newPaneID)).Msg("failed to attach webview widget for stacked pane")
	1516	+	c.rollbackStackPane(ctx, stackCtx, newPaneID)
1513	1517		return err
1514	1518		}
1515		-	// Load initial page for the new pane.
	1519	+	// Loading failures leave the attached pane available for a later navigation.
1516	1520		if err := wv.LoadURI(ctx, newPane.URI); err != nil {
1517	1521		log.Warn().Err(err).Str("uri_host", logging.SafeURLHost(newPane.URI)).Msg("failed to load initial page")
1518		-	return err
1519	1522		}
1520	1523
1521	1524		// Update workspace view
1522	1525		if err := stackCtx.wsView.SetActivePaneID(newPaneID); err != nil {
1523	1526		log.Warn().Err(err).Msg("failed to set active pane")
	1527	+	c.rollbackStackPane(ctx, stackCtx, newPaneID)
1524	1528		return err
1525	1529		}
1526	1530		stackCtx.wsView.FocusPane(newPaneID)

@@ -1554,6 +1558,15 @@ func (c *WorkspaceCoordinator) StackPaneWithURL(ctx context.Context, initialURL
1554	1558		return nil
1555	1559		}
1556	1560
	1561	+	func (c WorkspaceCoordinator) rollbackStackPane(ctx context.Context, stackCtx stackPaneContext, paneID entity.PaneID) {
	1562	+	if err := c.ClosePaneByID(ctx, paneID); err != nil {
	1563	+	logging.FromContext(ctx).Warn().Err(err).Str("pane_id", string(paneID)).
	1564	+	Msg("failed to roll back stacked pane")
	1565	+	}
	1566	+	stackCtx.ws.ActivePaneID = stackCtx.activePaneID
	1567	+	c.notifyStateChanged()
	1568	+	}
	1569	+
1557	1570		func (c *WorkspaceCoordinator) createOrAddStackPane(
1558	1571		ctx context.Context,
1559	1572		stackCtx *stackPaneContext,

 	addErr := c.stackedPaneMgr.AddPaneToStack(ctx, stackCtx.wsView, stackCtx.activePaneID, newPaneView, defaultPaneTitle)
 	if addErr != nil {
 		log.Error().Err(addErr).Msg("failed to add pane to stack")
+		c.rollbackStackPane(ctx, stackCtx, newPaneID)
 		return addErr
 	}
 	}
 	widget := c.contentCoord.WrapWidget(ctx, wv)
 	if widget == nil {
-		return fmt.Errorf("wrap webview for stacked pane")
+		err := fmt.Errorf("wrap webview for stacked pane")
+		c.rollbackStackPane(ctx, stackCtx, newPaneID)
+		return err
 	}
 	if err := stackCtx.wsView.SetWebViewWidget(newPaneID, widget); err != nil {
 		log.Warn().Err(err).Str("pane_id", string(newPaneID)).Msg("failed to attach webview widget for stacked pane")
+		c.rollbackStackPane(ctx, stackCtx, newPaneID)
 		return err
 	}
-	// Load initial page for the new pane.
+	// Loading failures leave the attached pane available for a later navigation.
 	if err := wv.LoadURI(ctx, newPane.URI); err != nil {
 		log.Warn().Err(err).Str("uri_host", logging.SafeURLHost(newPane.URI)).Msg("failed to load initial page")
-		return err
 	}
 	// Update workspace view
 	if err := stackCtx.wsView.SetActivePaneID(newPaneID); err != nil {
 		log.Warn().Err(err).Msg("failed to set active pane")
+		c.rollbackStackPane(ctx, stackCtx, newPaneID)
 		return err
 	}
 	stackCtx.wsView.FocusPane(newPaneID)
 	return nil
 }
+func (c *WorkspaceCoordinator) rollbackStackPane(ctx context.Context, stackCtx *stackPaneContext, paneID entity.PaneID) {
+	if err := c.ClosePaneByID(ctx, paneID); err != nil {
+		logging.FromContext(ctx).Warn().Err(err).Str("pane_id", string(paneID)).
+			Msg("failed to roll back stacked pane")
+	}
+	stackCtx.ws.ActivePaneID = stackCtx.activePaneID
+	c.notifyStateChanged()
+}
 func (c *WorkspaceCoordinator) createOrAddStackPane(
 	ctx context.Context,
 	stackCtx *stackPaneContext,

M internal/ui/coordinator/workspace_external_url_test.go +2 -2


@@ -55,7 +55,7 @@ func TestWorkspaceCoordinator_StackPaneUsesConfiguredURLThroughLegacyEntryPoint(
55	55
56	56		// The test coordinator intentionally has no WebView pool. Creation reaches
57	57		// that detectable UI failure only after the requested URL enters the model.
58		-	require.ErrorContains(t, coord.StackPane(context.Background()), "webview pool not configured")
	58	+	require.Error(t, coord.StackPane(context.Background()))
59	59		assert.Equal(t, legacyURL, ws.ActivePane().Pane.URI)
60	60		}
61	61

@@ -63,7 +63,7 @@ func TestWorkspaceCoordinator_StackPaneWithURLUsesExplicitURL(t *testing.T) {
63	63		const explicitURL = "https://explicit.example/path?token=secret"
64	64		coord, ws := stackURLTestCoordinator(t, "https://legacy.example/")
65	65
66		-	require.ErrorContains(t, coord.StackPaneWithURL(context.Background(), explicitURL), "webview pool not configured")
	66	+	require.Error(t, coord.StackPaneWithURL(context.Background(), explicitURL))
67	67		assert.Equal(t, explicitURL, ws.ActivePane().Pane.URI)
68	68		}
69	69


@@ -55,7 +55,7 @@ func TestWorkspaceCoordinator_StackPaneUsesConfiguredURLThroughLegacyEntryPoint(
55			55
56		// The test coordinator intentionally has no WebView pool. Creation reaches	56		// The test coordinator intentionally has no WebView pool. Creation reaches
57		// that detectable UI failure only after the requested URL enters the model.	57		// that detectable UI failure only after the requested URL enters the model.
58	-	require.ErrorContains(t, coord.StackPane(context.Background()), "webview pool not configured")	58	+	require.Error(t, coord.StackPane(context.Background()))
59		assert.Equal(t, legacyURL, ws.ActivePane().Pane.URI)	59		assert.Equal(t, legacyURL, ws.ActivePane().Pane.URI)
60		}	60		}
61			61

@@ -63,7 +63,7 @@ func TestWorkspaceCoordinator_StackPaneWithURLUsesExplicitURL(t *testing.T) {
63		const explicitURL = "https://explicit.example/path?token=secret"	63		const explicitURL = "https://explicit.example/path?token=secret"
64		coord, ws := stackURLTestCoordinator(t, "https://legacy.example/")	64		coord, ws := stackURLTestCoordinator(t, "https://legacy.example/")
65			65
66	-	require.ErrorContains(t, coord.StackPaneWithURL(context.Background(), explicitURL), "webview pool not configured")	66	+	require.Error(t, coord.StackPaneWithURL(context.Background(), explicitURL))
67		assert.Equal(t, explicitURL, ws.ActivePane().Pane.URI)	67		assert.Equal(t, explicitURL, ws.ActivePane().Pane.URI)
68		}	68		}
69			69

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


@@ -17,6 +17,7 @@ import (
17	17
18	18		func TestExternalLinkSplitDirectionPlacements(t *testing.T) {
19	19		tests := map[entity.ExternalLinkPlacement]usecase.SplitDirection{
	20	+	"": usecase.SplitRight,
20	21		entity.ExternalLinkPlacementLeft: usecase.SplitLeft,
21	22		entity.ExternalLinkPlacementRight: usecase.SplitRight,
22	23		entity.ExternalLinkPlacementTop: usecase.SplitUp,


@@ -17,6 +17,7 @@ import (
17		17
18	func TestExternalLinkSplitDirectionPlacements(t *testing.T) {	18		func TestExternalLinkSplitDirectionPlacements(t *testing.T) {
19	tests := map[entity.ExternalLinkPlacement]usecase.SplitDirection{	19		tests := map[entity.ExternalLinkPlacement]usecase.SplitDirection{
		20	+	"": usecase.SplitRight,
20	entity.ExternalLinkPlacementLeft: usecase.SplitLeft,	21		entity.ExternalLinkPlacementLeft: usecase.SplitLeft,
21	entity.ExternalLinkPlacementRight: usecase.SplitRight,	22		entity.ExternalLinkPlacementRight: usecase.SplitRight,
22	entity.ExternalLinkPlacementTop: usecase.SplitUp,	23		entity.ExternalLinkPlacementTop: usecase.SplitUp,