fix(omnibox): keep standalone launch visible on uncertainty
- commit
- 7c6795778ea0f12ef06205d7bb1e7f8cb295e295
- author
- brice · · 2026-05-23 04:28 UTC
- committer
- brice · · 2026-05-23 04:58 UTC
- parent
- 8c6c289
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
15 files · +285 -119
M cmd/dumber/main.go +24 -3
| @@ -3,6 +3,7 @@ package main | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "database/sql" | |
| 6 | + | "errors" | |
| 6 | 7 | "fmt" | |
| 7 | 8 | "os" | |
| 8 | 9 | "os/signal" | |
| @@ -98,7 +99,27 @@ func tryForwardBrowseURLToRunningInstance(ctx context.Context, relay port.Browse | |||
| 98 | 99 | return false, nil | |
| 99 | 100 | } | |
| 100 | 101 | ||
| 101 | - | return relay.DeliverOpenFreshWindow(ctx, browseURL) | |
| 102 | + | delivered, err := relay.DeliverOpenFreshWindow(ctx, browseURL) | |
| 103 | + | if err != nil { | |
| 104 | + | if delivered && errors.Is(err, desktop.ErrBrowserLaunchRelayUnconfirmed) { | |
| 105 | + | return true, nil | |
| 106 | + | } | |
| 107 | + | return false, err | |
| 108 | + | } | |
| 109 | + | return delivered, nil | |
| 110 | + | } | |
| 111 | + | ||
| 112 | + | func launchStandaloneBrowserURL(ctx context.Context, launch func(context.Context, string) error, uri string) error { | |
| 113 | + | if launch == nil { | |
| 114 | + | return fmt.Errorf("browser launcher is not configured") | |
| 115 | + | } | |
| 116 | + | if err := launch(ctx, uri); err != nil { | |
| 117 | + | if errors.Is(err, desktop.ErrBrowserLaunchRelayUnconfirmed) || errors.Is(err, desktop.ErrBrowserLaunchUnconfirmed) { | |
| 118 | + | return nil | |
| 119 | + | } | |
| 120 | + | return err | |
| 121 | + | } | |
| 122 | + | return nil | |
| 102 | 123 | } | |
| 103 | 124 | ||
| 104 | 125 | func resolveBrowserLaunchRelay(cfg *config.Config) (port.BrowserLaunchRelay, error) { | |
| @@ -871,8 +892,8 @@ func buildUIDependencies( | |||
| 871 | 892 | return config.GetManager().Watch() | |
| 872 | 893 | }, | |
| 873 | 894 | LaunchExternalURL: desktop.LaunchExternalURL, | |
| 874 | - | LaunchBrowserURL: func(uri string) { | |
| 875 | - | browserLauncher.LaunchURL(ctx, uri) | |
| 895 | + | LaunchBrowserURL: func(navCtx context.Context, uri string) error { | |
| 896 | + | return launchStandaloneBrowserURL(navCtx, browserLauncher.LaunchURL, uri) | |
| 876 | 897 | }, | |
| 877 | 898 | BrowserLaunchRelay: browserLaunchRelay, | |
| 878 | 899 | ConfigMigrator: config.NewMigrator(), | |
| @@ -3,6 +3,7 @@ package main | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "database/sql" | 5 | "database/sql" | ||
| 6 | + | "errors" | |||
| 6 | "fmt" | 7 | "fmt" | ||
| 7 | "os" | 8 | "os" | ||
| 8 | "os/signal" | 9 | "os/signal" | ||
| @@ -98,7 +99,27 @@ func tryForwardBrowseURLToRunningInstance(ctx context.Context, relay port.Browse | |||||
| 98 | return false, nil | 99 | return false, nil | ||
| 99 | } | 100 | } | ||
| 100 | 101 | ||||
| 101 | - | return relay.DeliverOpenFreshWindow(ctx, browseURL) | 102 | + | delivered, err := relay.DeliverOpenFreshWindow(ctx, browseURL) |
| 103 | + | if err != nil { | |||
| 104 | + | if delivered && errors.Is(err, desktop.ErrBrowserLaunchRelayUnconfirmed) { | |||
| 105 | + | return true, nil | |||
| 106 | + | } | |||
| 107 | + | return false, err | |||
| 108 | + | } | |||
| 109 | + | return delivered, nil | |||
| 110 | + | } | |||
| 111 | + | ||||
| 112 | + | func launchStandaloneBrowserURL(ctx context.Context, launch func(context.Context, string) error, uri string) error { | |||
| 113 | + | if launch == nil { | |||
| 114 | + | return fmt.Errorf("browser launcher is not configured") | |||
| 115 | + | } | |||
| 116 | + | if err := launch(ctx, uri); err != nil { | |||
| 117 | + | if errors.Is(err, desktop.ErrBrowserLaunchRelayUnconfirmed) || errors.Is(err, desktop.ErrBrowserLaunchUnconfirmed) { | |||
| 118 | + | return nil | |||
| 119 | + | } | |||
| 120 | + | return err | |||
| 121 | + | } | |||
| 122 | + | return nil | |||
| 102 | } | 123 | } | ||
| 103 | 124 | ||||
| 104 | func resolveBrowserLaunchRelay(cfg *config.Config) (port.BrowserLaunchRelay, error) { | 125 | func resolveBrowserLaunchRelay(cfg *config.Config) (port.BrowserLaunchRelay, error) { | ||
| @@ -871,8 +892,8 @@ func buildUIDependencies( | |||||
| 871 | return config.GetManager().Watch() | 892 | return config.GetManager().Watch() | ||
| 872 | }, | 893 | }, | ||
| 873 | LaunchExternalURL: desktop.LaunchExternalURL, | 894 | LaunchExternalURL: desktop.LaunchExternalURL, | ||
| 874 | - | LaunchBrowserURL: func(uri string) { | 895 | + | LaunchBrowserURL: func(navCtx context.Context, uri string) error { |
| 875 | - | browserLauncher.LaunchURL(ctx, uri) | 896 | + | return launchStandaloneBrowserURL(navCtx, browserLauncher.LaunchURL, uri) |
| 876 | }, | 897 | }, | ||
| 877 | BrowserLaunchRelay: browserLaunchRelay, | 898 | BrowserLaunchRelay: browserLaunchRelay, | ||
| 878 | ConfigMigrator: config.NewMigrator(), | 899 | ConfigMigrator: config.NewMigrator(), | ||
M cmd/dumber/main_test.go +37 -0
| @@ -9,6 +9,7 @@ import ( | |||
| 9 | 9 | "github.com/bnema/dumber/internal/bootstrap" | |
| 10 | 10 | "github.com/bnema/dumber/internal/infrastructure/colorscheme" | |
| 11 | 11 | "github.com/bnema/dumber/internal/infrastructure/config" | |
| 12 | + | "github.com/bnema/dumber/internal/infrastructure/desktop" | |
| 12 | 13 | ) | |
| 13 | 14 | ||
| 14 | 15 | func TestLaunchModeFromArgs_DetectsStandaloneOmnibox(t *testing.T) { | |
| @@ -238,6 +239,42 @@ func TestTryForwardBrowseURLToRunningInstance_ReturnsFalseOnRelayMiss(t *testing | |||
| 238 | 239 | } | |
| 239 | 240 | } | |
| 240 | 241 | ||
| 242 | + | func TestTryForwardBrowseURLToRunningInstance_ReturnsTrueOnUnconfirmedRelayDelivery(t *testing.T) { | |
| 243 | + | relay := mocks.NewMockBrowserLaunchRelay(t) | |
| 244 | + | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(true, desktop.ErrBrowserLaunchRelayUnconfirmed) | |
| 245 | + | ||
| 246 | + | forwarded, err := tryForwardBrowseURLToRunningInstance(context.Background(), relay, "https://example.com") | |
| 247 | + | ||
| 248 | + | if err != nil { | |
| 249 | + | t.Fatalf("expected no error, got %v", err) | |
| 250 | + | } | |
| 251 | + | if !forwarded { | |
| 252 | + | t.Fatal("expected unconfirmed relay delivery to be treated as forwarded") | |
| 253 | + | } | |
| 254 | + | } | |
| 255 | + | ||
| 256 | + | func TestLaunchStandaloneBrowserURL_TreatsUnconfirmedLaunchAsAccepted(t *testing.T) { | |
| 257 | + | err := launchStandaloneBrowserURL(context.Background(), func(context.Context, string) error { | |
| 258 | + | return desktop.ErrBrowserLaunchRelayUnconfirmed | |
| 259 | + | }, "https://example.com") | |
| 260 | + | ||
| 261 | + | if err != nil { | |
| 262 | + | t.Fatalf("expected no error, got %v", err) | |
| 263 | + | } | |
| 264 | + | } | |
| 265 | + | ||
| 266 | + | func TestLaunchStandaloneBrowserURL_PropagatesOtherErrors(t *testing.T) { | |
| 267 | + | wantErr := errors.New("boom") | |
| 268 | + | ||
| 269 | + | err := launchStandaloneBrowserURL(context.Background(), func(context.Context, string) error { | |
| 270 | + | return wantErr | |
| 271 | + | }, "https://example.com") | |
| 272 | + | ||
| 273 | + | if !errors.Is(err, wantErr) { | |
| 274 | + | t.Fatalf("expected error %v, got %v", wantErr, err) | |
| 275 | + | } | |
| 276 | + | } | |
| 277 | + | ||
| 241 | 278 | func TestTryForwardBrowseURLToRunningInstance_PropagatesError(t *testing.T) { | |
| 242 | 279 | relay := mocks.NewMockBrowserLaunchRelay(t) | |
| 243 | 280 | wantErr := errors.New("relay error") | |
| @@ -9,6 +9,7 @@ import ( | |||||
| 9 | "github.com/bnema/dumber/internal/bootstrap" | 9 | "github.com/bnema/dumber/internal/bootstrap" | ||
| 10 | "github.com/bnema/dumber/internal/infrastructure/colorscheme" | 10 | "github.com/bnema/dumber/internal/infrastructure/colorscheme" | ||
| 11 | "github.com/bnema/dumber/internal/infrastructure/config" | 11 | "github.com/bnema/dumber/internal/infrastructure/config" | ||
| 12 | + | "github.com/bnema/dumber/internal/infrastructure/desktop" | |||
| 12 | ) | 13 | ) | ||
| 13 | 14 | ||||
| 14 | func TestLaunchModeFromArgs_DetectsStandaloneOmnibox(t *testing.T) { | 15 | func TestLaunchModeFromArgs_DetectsStandaloneOmnibox(t *testing.T) { | ||
| @@ -238,6 +239,42 @@ func TestTryForwardBrowseURLToRunningInstance_ReturnsFalseOnRelayMiss(t *testing | |||||
| 238 | } | 239 | } | ||
| 239 | } | 240 | } | ||
| 240 | 241 | ||||
| 242 | + | func TestTryForwardBrowseURLToRunningInstance_ReturnsTrueOnUnconfirmedRelayDelivery(t *testing.T) { | |||
| 243 | + | relay := mocks.NewMockBrowserLaunchRelay(t) | |||
| 244 | + | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(true, desktop.ErrBrowserLaunchRelayUnconfirmed) | |||
| 245 | + | ||||
| 246 | + | forwarded, err := tryForwardBrowseURLToRunningInstance(context.Background(), relay, "https://example.com") | |||
| 247 | + | ||||
| 248 | + | if err != nil { | |||
| 249 | + | t.Fatalf("expected no error, got %v", err) | |||
| 250 | + | } | |||
| 251 | + | if !forwarded { | |||
| 252 | + | t.Fatal("expected unconfirmed relay delivery to be treated as forwarded") | |||
| 253 | + | } | |||
| 254 | + | } | |||
| 255 | + | ||||
| 256 | + | func TestLaunchStandaloneBrowserURL_TreatsUnconfirmedLaunchAsAccepted(t *testing.T) { | |||
| 257 | + | err := launchStandaloneBrowserURL(context.Background(), func(context.Context, string) error { | |||
| 258 | + | return desktop.ErrBrowserLaunchRelayUnconfirmed | |||
| 259 | + | }, "https://example.com") | |||
| 260 | + | ||||
| 261 | + | if err != nil { | |||
| 262 | + | t.Fatalf("expected no error, got %v", err) | |||
| 263 | + | } | |||
| 264 | + | } | |||
| 265 | + | ||||
| 266 | + | func TestLaunchStandaloneBrowserURL_PropagatesOtherErrors(t *testing.T) { | |||
| 267 | + | wantErr := errors.New("boom") | |||
| 268 | + | ||||
| 269 | + | err := launchStandaloneBrowserURL(context.Background(), func(context.Context, string) error { | |||
| 270 | + | return wantErr | |||
| 271 | + | }, "https://example.com") | |||
| 272 | + | ||||
| 273 | + | if !errors.Is(err, wantErr) { | |||
| 274 | + | t.Fatalf("expected error %v, got %v", wantErr, err) | |||
| 275 | + | } | |||
| 276 | + | } | |||
| 277 | + | ||||
| 241 | func TestTryForwardBrowseURLToRunningInstance_PropagatesError(t *testing.T) { | 278 | func TestTryForwardBrowseURLToRunningInstance_PropagatesError(t *testing.T) { | ||
| 242 | relay := mocks.NewMockBrowserLaunchRelay(t) | 279 | relay := mocks.NewMockBrowserLaunchRelay(t) | ||
| 243 | wantErr := errors.New("relay error") | 280 | wantErr := errors.New("relay error") | ||
M internal/application/port/browser_launch.go +2 -0
| @@ -15,6 +15,8 @@ type BrowserLaunchRelay interface { | |||
| 15 | 15 | // DeliverOpenFreshWindow attempts to deliver a request to open a fresh window. | |
| 16 | 16 | // The bool reports whether the relay accepted the request; false means the | |
| 17 | 17 | // caller should treat it as undelivered and try another path. | |
| 18 | + | // An error may still be returned with delivered=true when the relay accepted | |
| 19 | + | // the request but could not confirm completion before the caller timed out. | |
| 18 | 20 | DeliverOpenFreshWindow(ctx context.Context, url string) (bool, error) | |
| 19 | 21 | Listen(ctx context.Context, opener BrowserWindowOpener) (io.Closer, error) | |
| 20 | 22 | } | |
| @@ -15,6 +15,8 @@ type BrowserLaunchRelay interface { | |||||
| 15 | // DeliverOpenFreshWindow attempts to deliver a request to open a fresh window. | 15 | // DeliverOpenFreshWindow attempts to deliver a request to open a fresh window. | ||
| 16 | // The bool reports whether the relay accepted the request; false means the | 16 | // The bool reports whether the relay accepted the request; false means the | ||
| 17 | // caller should treat it as undelivered and try another path. | 17 | // caller should treat it as undelivered and try another path. | ||
| 18 | + | // An error may still be returned with delivered=true when the relay accepted | |||
| 19 | + | // the request but could not confirm completion before the caller timed out. | |||
| 18 | DeliverOpenFreshWindow(ctx context.Context, url string) (bool, error) | 20 | DeliverOpenFreshWindow(ctx context.Context, url string) (bool, error) | ||
| 19 | Listen(ctx context.Context, opener BrowserWindowOpener) (io.Closer, error) | 21 | Listen(ctx context.Context, opener BrowserWindowOpener) (io.Closer, error) | ||
| 20 | } | 22 | } | ||
M internal/infrastructure/desktop/adapter.go +7 -5
| @@ -407,10 +407,12 @@ func LaunchExternalURL(uri string) { | |||
| 407 | 407 | ||
| 408 | 408 | // LaunchBrowserURL opens a URL in a new dumber browser window. | |
| 409 | 409 | func LaunchBrowserURL(uri string) { | |
| 410 | - | launchBrowserBrowseURL(uri, getExecutablePath, startDetachedProcess) | |
| 410 | + | if err := launchBrowserBrowseURL(uri, getExecutablePath, startDetachedProcess); err != nil { | |
| 411 | + | fmt.Fprintf(os.Stderr, "%v\n", err) | |
| 412 | + | } | |
| 411 | 413 | } | |
| 412 | 414 | ||
| 413 | - | func launchBrowserBrowseURL(uri string, resolveExecutablePath func() (string, error), spawnDetachedProcess func(*exec.Cmd) error) { | |
| 415 | + | func launchBrowserBrowseURL(uri string, resolveExecutablePath func() (string, error), spawnDetachedProcess func(*exec.Cmd) error) error { | |
| 414 | 416 | if resolveExecutablePath == nil { | |
| 415 | 417 | resolveExecutablePath = getExecutablePath | |
| 416 | 418 | } | |
| @@ -420,15 +422,15 @@ func launchBrowserBrowseURL(uri string, resolveExecutablePath func() (string, er | |||
| 420 | 422 | ||
| 421 | 423 | execPath, err := resolveExecutablePath() | |
| 422 | 424 | if err != nil { | |
| 423 | - | fmt.Fprintf(os.Stderr, "failed to resolve dumber executable for URL %q: %v\n", uri, err) | |
| 424 | - | return | |
| 425 | + | return fmt.Errorf("failed to resolve dumber executable for URL %q: %w", uri, err) | |
| 425 | 426 | } | |
| 426 | 427 | ||
| 427 | 428 | cmd := exec.Command(execPath, "browse", uri) | |
| 428 | 429 | cmd.Env = sanitizedChildEnv(os.Environ()) | |
| 429 | 430 | if err := spawnDetachedProcess(cmd); err != nil { | |
| 430 | - | fmt.Fprintf(os.Stderr, "failed to launch dumber browse for URL %q: %v\n", uri, err) | |
| 431 | + | return fmt.Errorf("failed to launch dumber browse for URL %q: %w", uri, err) | |
| 431 | 432 | } | |
| 433 | + | return nil | |
| 432 | 434 | } | |
| 433 | 435 | ||
| 434 | 436 | func startDetachedProcess(cmd *exec.Cmd) error { | |
| @@ -407,10 +407,12 @@ func LaunchExternalURL(uri string) { | |||||
| 407 | 407 | ||||
| 408 | // LaunchBrowserURL opens a URL in a new dumber browser window. | 408 | // LaunchBrowserURL opens a URL in a new dumber browser window. | ||
| 409 | func LaunchBrowserURL(uri string) { | 409 | func LaunchBrowserURL(uri string) { | ||
| 410 | - | launchBrowserBrowseURL(uri, getExecutablePath, startDetachedProcess) | 410 | + | if err := launchBrowserBrowseURL(uri, getExecutablePath, startDetachedProcess); err != nil { |
| 411 | + | fmt.Fprintf(os.Stderr, "%v\n", err) | |||
| 412 | + | } | |||
| 411 | } | 413 | } | ||
| 412 | 414 | ||||
| 413 | - | func launchBrowserBrowseURL(uri string, resolveExecutablePath func() (string, error), spawnDetachedProcess func(*exec.Cmd) error) { | 415 | + | func launchBrowserBrowseURL(uri string, resolveExecutablePath func() (string, error), spawnDetachedProcess func(*exec.Cmd) error) error { |
| 414 | if resolveExecutablePath == nil { | 416 | if resolveExecutablePath == nil { | ||
| 415 | resolveExecutablePath = getExecutablePath | 417 | resolveExecutablePath = getExecutablePath | ||
| 416 | } | 418 | } | ||
| @@ -420,15 +422,15 @@ func launchBrowserBrowseURL(uri string, resolveExecutablePath func() (string, er | |||||
| 420 | 422 | ||||
| 421 | execPath, err := resolveExecutablePath() | 423 | execPath, err := resolveExecutablePath() | ||
| 422 | if err != nil { | 424 | if err != nil { | ||
| 423 | - | fmt.Fprintf(os.Stderr, "failed to resolve dumber executable for URL %q: %v\n", uri, err) | 425 | + | return fmt.Errorf("failed to resolve dumber executable for URL %q: %w", uri, err) |
| 424 | - | return | |||
| 425 | } | 426 | } | ||
| 426 | 427 | ||||
| 427 | cmd := exec.Command(execPath, "browse", uri) | 428 | cmd := exec.Command(execPath, "browse", uri) | ||
| 428 | cmd.Env = sanitizedChildEnv(os.Environ()) | 429 | cmd.Env = sanitizedChildEnv(os.Environ()) | ||
| 429 | if err := spawnDetachedProcess(cmd); err != nil { | 430 | if err := spawnDetachedProcess(cmd); err != nil { | ||
| 430 | - | fmt.Fprintf(os.Stderr, "failed to launch dumber browse for URL %q: %v\n", uri, err) | 431 | + | return fmt.Errorf("failed to launch dumber browse for URL %q: %w", uri, err) |
| 431 | } | 432 | } | ||
| 433 | + | return nil | |||
| 432 | } | 434 | } | ||
| 433 | 435 | ||||
| 434 | func startDetachedProcess(cmd *exec.Cmd) error { | 436 | func startDetachedProcess(cmd *exec.Cmd) error { | ||
M internal/infrastructure/desktop/browser_launch_relay.go +22 -2
| @@ -7,6 +7,7 @@ import ( | |||
| 7 | 7 | "fmt" | |
| 8 | 8 | "io" | |
| 9 | 9 | "net" | |
| 10 | + | neturl "net/url" | |
| 10 | 11 | "os" | |
| 11 | 12 | "path/filepath" | |
| 12 | 13 | "sync" | |
| @@ -20,10 +21,17 @@ import ( | |||
| 20 | 21 | ||
| 21 | 22 | const browserLaunchSocketName = "browser-launch.sock" | |
| 22 | 23 | ||
| 23 | - | const browserLaunchIOTimeout = 50 * time.Millisecond | |
| 24 | + | // Local browser-launch handoff needs enough headroom for a busy browser | |
| 25 | + | // process to accept and acknowledge the request before callers classify the | |
| 26 | + | // handoff as ambiguous. | |
| 27 | + | const browserLaunchIOTimeout = 250 * time.Millisecond | |
| 24 | 28 | ||
| 25 | 29 | const browserLaunchDirPerm = 0o700 | |
| 26 | 30 | ||
| 31 | + | // ErrBrowserLaunchRelayUnconfirmed reports that the relay accepted a launch | |
| 32 | + | // request but the caller did not receive a confirmation response in time. | |
| 33 | + | var ErrBrowserLaunchRelayUnconfirmed = errors.New("browser launch relay did not confirm delivery") | |
| 34 | + | ||
| 27 | 35 | type browserLaunchRelay struct { | |
| 28 | 36 | ipc runtimeprofile.IPCPaths | |
| 29 | 37 | } | |
| @@ -82,7 +90,11 @@ func (r *browserLaunchRelay) DeliverOpenFreshWindow(ctx context.Context, url str | |||
| 82 | 90 | return false, ctxErr | |
| 83 | 91 | } | |
| 84 | 92 | if _, ok := ctx.Deadline(); !ok { | |
| 85 | - | return true, nil | |
| 93 | + | logging.FromContext(ctx).Warn(). | |
| 94 | + | Str("url_host", safeURLHost(url)). | |
| 95 | + | Dur("timeout", browserLaunchIOTimeout). | |
| 96 | + | Msg("browser launch relay response timed out without caller deadline; delivery is unconfirmed") | |
| 97 | + | return true, ErrBrowserLaunchRelayUnconfirmed | |
| 86 | 98 | } | |
| 87 | 99 | if deadlineErr := setBrowserLaunchConnDeadline(ctx, conn); deadlineErr != nil { | |
| 88 | 100 | return false, deadlineErr | |
| @@ -104,6 +116,14 @@ func isMissingRelayListener(err error) bool { | |||
| 104 | 116 | return os.IsNotExist(err) || errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.ENOENT) | |
| 105 | 117 | } | |
| 106 | 118 | ||
| 119 | + | func safeURLHost(raw string) string { | |
| 120 | + | parsed, err := neturl.Parse(raw) | |
| 121 | + | if err != nil { | |
| 122 | + | return "" | |
| 123 | + | } | |
| 124 | + | return parsed.Host | |
| 125 | + | } | |
| 126 | + | ||
| 107 | 127 | func isBrowserLaunchReadTimeout(err error) bool { | |
| 108 | 128 | var netErr net.Error | |
| 109 | 129 | return errors.As(err, &netErr) && netErr.Timeout() | |
| @@ -7,6 +7,7 @@ import ( | |||||
| 7 | "fmt" | 7 | "fmt" | ||
| 8 | "io" | 8 | "io" | ||
| 9 | "net" | 9 | "net" | ||
| 10 | + | neturl "net/url" | |||
| 10 | "os" | 11 | "os" | ||
| 11 | "path/filepath" | 12 | "path/filepath" | ||
| 12 | "sync" | 13 | "sync" | ||
| @@ -20,10 +21,17 @@ import ( | |||||
| 20 | 21 | ||||
| 21 | const browserLaunchSocketName = "browser-launch.sock" | 22 | const browserLaunchSocketName = "browser-launch.sock" | ||
| 22 | 23 | ||||
| 23 | - | const browserLaunchIOTimeout = 50 * time.Millisecond | 24 | + | // Local browser-launch handoff needs enough headroom for a busy browser |
| 25 | + | // process to accept and acknowledge the request before callers classify the | |||
| 26 | + | // handoff as ambiguous. | |||
| 27 | + | const browserLaunchIOTimeout = 250 * time.Millisecond | |||
| 24 | 28 | ||||
| 25 | const browserLaunchDirPerm = 0o700 | 29 | const browserLaunchDirPerm = 0o700 | ||
| 26 | 30 | ||||
| 31 | + | // ErrBrowserLaunchRelayUnconfirmed reports that the relay accepted a launch | |||
| 32 | + | // request but the caller did not receive a confirmation response in time. | |||
| 33 | + | var ErrBrowserLaunchRelayUnconfirmed = errors.New("browser launch relay did not confirm delivery") | |||
| 34 | + | ||||
| 27 | type browserLaunchRelay struct { | 35 | type browserLaunchRelay struct { | ||
| 28 | ipc runtimeprofile.IPCPaths | 36 | ipc runtimeprofile.IPCPaths | ||
| 29 | } | 37 | } | ||
| @@ -82,7 +90,11 @@ func (r *browserLaunchRelay) DeliverOpenFreshWindow(ctx context.Context, url str | |||||
| 82 | return false, ctxErr | 90 | return false, ctxErr | ||
| 83 | } | 91 | } | ||
| 84 | if _, ok := ctx.Deadline(); !ok { | 92 | if _, ok := ctx.Deadline(); !ok { | ||
| 85 | - | return true, nil | 93 | + | logging.FromContext(ctx).Warn(). |
| 94 | + | Str("url_host", safeURLHost(url)). | |||
| 95 | + | Dur("timeout", browserLaunchIOTimeout). | |||
| 96 | + | Msg("browser launch relay response timed out without caller deadline; delivery is unconfirmed") | |||
| 97 | + | return true, ErrBrowserLaunchRelayUnconfirmed | |||
| 86 | } | 98 | } | ||
| 87 | if deadlineErr := setBrowserLaunchConnDeadline(ctx, conn); deadlineErr != nil { | 99 | if deadlineErr := setBrowserLaunchConnDeadline(ctx, conn); deadlineErr != nil { | ||
| 88 | return false, deadlineErr | 100 | return false, deadlineErr | ||
| @@ -104,6 +116,14 @@ func isMissingRelayListener(err error) bool { | |||||
| 104 | return os.IsNotExist(err) || errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.ENOENT) | 116 | return os.IsNotExist(err) || errors.Is(err, syscall.ECONNREFUSED) || errors.Is(err, syscall.ENOENT) | ||
| 105 | } | 117 | } | ||
| 106 | 118 | ||||
| 119 | + | func safeURLHost(raw string) string { | |||
| 120 | + | parsed, err := neturl.Parse(raw) | |||
| 121 | + | if err != nil { | |||
| 122 | + | return "" | |||
| 123 | + | } | |||
| 124 | + | return parsed.Host | |||
| 125 | + | } | |||
| 126 | + | ||||
| 107 | func isBrowserLaunchReadTimeout(err error) bool { | 127 | func isBrowserLaunchReadTimeout(err error) bool { | ||
| 108 | var netErr net.Error | 128 | var netErr net.Error | ||
| 109 | return errors.As(err, &netErr) && netErr.Timeout() | 129 | return errors.As(err, &netErr) && netErr.Timeout() | ||
M internal/infrastructure/desktop/browser_launch_relay_test.go +4 -3
| @@ -313,7 +313,7 @@ func TestBrowserLaunchRelay_DeliverOpenFreshWindow_RespectsResponseReadTimeout(t | |||
| 313 | 313 | assert.False(t, delivered) | |
| 314 | 314 | } | |
| 315 | 315 | ||
| 316 | - | func TestBrowserLaunchRelay_DeliverOpenFreshWindow_ReturnsTrueOnNoDeadlineTimeout(t *testing.T) { | |
| 316 | + | func TestBrowserLaunchRelay_DeliverOpenFreshWindow_ReturnsUnconfirmedErrorOnNoDeadlineTimeout(t *testing.T) { | |
| 317 | 317 | ipc := testIPC(shortTempDir(t)) | |
| 318 | 318 | require.NoError(t, os.MkdirAll(ipc.RuntimeDir, 0o700)) | |
| 319 | 319 | ||
| @@ -355,8 +355,9 @@ func TestBrowserLaunchRelay_DeliverOpenFreshWindow_ReturnsTrueOnNoDeadlineTimeou | |||
| 355 | 355 | select { | |
| 356 | 356 | case got := <-result: | |
| 357 | 357 | assert.True(t, got.delivered) | |
| 358 | - | require.NoError(t, got.err) | |
| 359 | - | case <-time.After(300 * time.Millisecond): | |
| 358 | + | require.Error(t, got.err) | |
| 359 | + | require.ErrorIs(t, got.err, ErrBrowserLaunchRelayUnconfirmed) | |
| 360 | + | case <-time.After(browserLaunchIOTimeout * 3): | |
| 360 | 361 | t.Fatal("DeliverOpenFreshWindow blocked without a caller deadline") | |
| 361 | 362 | } | |
| 362 | 363 | } | |
| @@ -313,7 +313,7 @@ func TestBrowserLaunchRelay_DeliverOpenFreshWindow_RespectsResponseReadTimeout(t | |||||
| 313 | assert.False(t, delivered) | 313 | assert.False(t, delivered) | ||
| 314 | } | 314 | } | ||
| 315 | 315 | ||||
| 316 | - | func TestBrowserLaunchRelay_DeliverOpenFreshWindow_ReturnsTrueOnNoDeadlineTimeout(t *testing.T) { | 316 | + | func TestBrowserLaunchRelay_DeliverOpenFreshWindow_ReturnsUnconfirmedErrorOnNoDeadlineTimeout(t *testing.T) { |
| 317 | ipc := testIPC(shortTempDir(t)) | 317 | ipc := testIPC(shortTempDir(t)) | ||
| 318 | require.NoError(t, os.MkdirAll(ipc.RuntimeDir, 0o700)) | 318 | require.NoError(t, os.MkdirAll(ipc.RuntimeDir, 0o700)) | ||
| 319 | 319 | ||||
| @@ -355,8 +355,9 @@ func TestBrowserLaunchRelay_DeliverOpenFreshWindow_ReturnsTrueOnNoDeadlineTimeou | |||||
| 355 | select { | 355 | select { | ||
| 356 | case got := <-result: | 356 | case got := <-result: | ||
| 357 | assert.True(t, got.delivered) | 357 | assert.True(t, got.delivered) | ||
| 358 | - | require.NoError(t, got.err) | 358 | + | require.Error(t, got.err) |
| 359 | - | case <-time.After(300 * time.Millisecond): | 359 | + | require.ErrorIs(t, got.err, ErrBrowserLaunchRelayUnconfirmed) |
| 360 | + | case <-time.After(browserLaunchIOTimeout * 3): | |||
| 360 | t.Fatal("DeliverOpenFreshWindow blocked without a caller deadline") | 361 | t.Fatal("DeliverOpenFreshWindow blocked without a caller deadline") | ||
| 361 | } | 362 | } | ||
| 362 | } | 363 | } | ||
M internal/infrastructure/desktop/browser_launcher.go +16 -6
| @@ -2,13 +2,17 @@ package desktop | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "errors" | |
| 5 | 6 | "fmt" | |
| 6 | - | "os" | |
| 7 | 7 | "os/exec" | |
| 8 | 8 | ||
| 9 | 9 | "github.com/bnema/dumber/internal/application/port" | |
| 10 | 10 | ) | |
| 11 | 11 | ||
| 12 | + | // ErrBrowserLaunchUnconfirmed reports that a running browser instance may have | |
| 13 | + | // received the request, but the caller could not confirm that in time. | |
| 14 | + | var ErrBrowserLaunchUnconfirmed = errors.New("browser launch could not be confirmed") | |
| 15 | + | ||
| 12 | 16 | // BrowserLauncher opens URLs in a relay-first dumber browser instance. | |
| 13 | 17 | type BrowserLauncher struct { | |
| 14 | 18 | relay port.BrowserLaunchRelay | |
| @@ -26,20 +30,26 @@ func NewBrowserLauncher(relay port.BrowserLaunchRelay) *BrowserLauncher { | |||
| 26 | 30 | } | |
| 27 | 31 | ||
| 28 | 32 | // LaunchURL forwards the URL to a running instance when possible, otherwise spawns a new one. | |
| 29 | - | func (l *BrowserLauncher) LaunchURL(ctx context.Context, url string) { | |
| 33 | + | func (l *BrowserLauncher) LaunchURL(ctx context.Context, url string) error { | |
| 30 | 34 | if l == nil { | |
| 31 | - | return | |
| 35 | + | return nil | |
| 32 | 36 | } | |
| 33 | 37 | ||
| 34 | 38 | if l.relay != nil { | |
| 35 | 39 | delivered, err := l.relay.DeliverOpenFreshWindow(ctx, url) | |
| 36 | 40 | if err != nil { | |
| 37 | - | fmt.Fprintf(os.Stderr, "failed to forward dumber browser URL %q: %v\n", url, err) | |
| 41 | + | if delivered && errors.Is(err, ErrBrowserLaunchRelayUnconfirmed) { | |
| 42 | + | return ErrBrowserLaunchUnconfirmed | |
| 43 | + | } | |
| 44 | + | return fmt.Errorf("forward dumber browser URL %q: %w", url, err) | |
| 38 | 45 | } | |
| 39 | 46 | if delivered { | |
| 40 | - | return | |
| 47 | + | return nil | |
| 41 | 48 | } | |
| 42 | 49 | } | |
| 43 | 50 | ||
| 44 | - | launchBrowserBrowseURL(url, l.resolveExecutablePath, l.startDetachedProcess) | |
| 51 | + | if err := launchBrowserBrowseURL(url, l.resolveExecutablePath, l.startDetachedProcess); err != nil { | |
| 52 | + | return fmt.Errorf("launch dumber browse for URL %q: %w", url, err) | |
| 53 | + | } | |
| 54 | + | return nil | |
| 45 | 55 | } | |
| @@ -2,13 +2,17 @@ package desktop | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "errors" | |||
| 5 | "fmt" | 6 | "fmt" | ||
| 6 | - | "os" | |||
| 7 | "os/exec" | 7 | "os/exec" | ||
| 8 | 8 | ||||
| 9 | "github.com/bnema/dumber/internal/application/port" | 9 | "github.com/bnema/dumber/internal/application/port" | ||
| 10 | ) | 10 | ) | ||
| 11 | 11 | ||||
| 12 | + | // ErrBrowserLaunchUnconfirmed reports that a running browser instance may have | |||
| 13 | + | // received the request, but the caller could not confirm that in time. | |||
| 14 | + | var ErrBrowserLaunchUnconfirmed = errors.New("browser launch could not be confirmed") | |||
| 15 | + | ||||
| 12 | // BrowserLauncher opens URLs in a relay-first dumber browser instance. | 16 | // BrowserLauncher opens URLs in a relay-first dumber browser instance. | ||
| 13 | type BrowserLauncher struct { | 17 | type BrowserLauncher struct { | ||
| 14 | relay port.BrowserLaunchRelay | 18 | relay port.BrowserLaunchRelay | ||
| @@ -26,20 +30,26 @@ func NewBrowserLauncher(relay port.BrowserLaunchRelay) *BrowserLauncher { | |||||
| 26 | } | 30 | } | ||
| 27 | 31 | ||||
| 28 | // LaunchURL forwards the URL to a running instance when possible, otherwise spawns a new one. | 32 | // LaunchURL forwards the URL to a running instance when possible, otherwise spawns a new one. | ||
| 29 | - | func (l *BrowserLauncher) LaunchURL(ctx context.Context, url string) { | 33 | + | func (l *BrowserLauncher) LaunchURL(ctx context.Context, url string) error { |
| 30 | if l == nil { | 34 | if l == nil { | ||
| 31 | - | return | 35 | + | return nil |
| 32 | } | 36 | } | ||
| 33 | 37 | ||||
| 34 | if l.relay != nil { | 38 | if l.relay != nil { | ||
| 35 | delivered, err := l.relay.DeliverOpenFreshWindow(ctx, url) | 39 | delivered, err := l.relay.DeliverOpenFreshWindow(ctx, url) | ||
| 36 | if err != nil { | 40 | if err != nil { | ||
| 37 | - | fmt.Fprintf(os.Stderr, "failed to forward dumber browser URL %q: %v\n", url, err) | 41 | + | if delivered && errors.Is(err, ErrBrowserLaunchRelayUnconfirmed) { |
| 42 | + | return ErrBrowserLaunchUnconfirmed | |||
| 43 | + | } | |||
| 44 | + | return fmt.Errorf("forward dumber browser URL %q: %w", url, err) | |||
| 38 | } | 45 | } | ||
| 39 | if delivered { | 46 | if delivered { | ||
| 40 | - | return | 47 | + | return nil |
| 41 | } | 48 | } | ||
| 42 | } | 49 | } | ||
| 43 | 50 | ||||
| 44 | - | launchBrowserBrowseURL(url, l.resolveExecutablePath, l.startDetachedProcess) | 51 | + | if err := launchBrowserBrowseURL(url, l.resolveExecutablePath, l.startDetachedProcess); err != nil { |
| 52 | + | return fmt.Errorf("launch dumber browse for URL %q: %w", url, err) | |||
| 53 | + | } | |||
| 54 | + | return nil | |||
| 45 | } | 55 | } | ||
M internal/infrastructure/desktop/browser_launcher_test.go +30 -20
| @@ -3,8 +3,6 @@ package desktop | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "errors" | |
| 6 | - | "io" | |
| 7 | - | "os" | |
| 8 | 6 | "os/exec" | |
| 9 | 7 | "testing" | |
| 10 | 8 | ||
| @@ -28,8 +26,9 @@ func TestBrowserLauncher_LaunchURL_ReturnsWithoutSpawningWhenRelayDelivers(t *te | |||
| 28 | 26 | return nil | |
| 29 | 27 | } | |
| 30 | 28 | ||
| 31 | - | launcher.LaunchURL(context.Background(), "https://example.com") | |
| 29 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") | |
| 32 | 30 | ||
| 31 | + | require.NoError(t, err) | |
| 33 | 32 | assert.False(t, spawned) | |
| 34 | 33 | } | |
| 35 | 34 | ||
| @@ -48,17 +47,17 @@ func TestBrowserLauncher_LaunchURL_FallsBackToSpawnWhenRelayMisses(t *testing.T) | |||
| 48 | 47 | return nil | |
| 49 | 48 | } | |
| 50 | 49 | ||
| 51 | - | launcher.LaunchURL(context.Background(), "https://example.com") | |
| 50 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") | |
| 52 | 51 | ||
| 52 | + | require.NoError(t, err) | |
| 53 | 53 | require.NotNil(t, gotCmd) | |
| 54 | 54 | assert.Equal(t, "/usr/bin/dumber", gotCmd.Path) | |
| 55 | 55 | assert.Equal(t, []string{"/usr/bin/dumber", "browse", "https://example.com"}, gotCmd.Args) | |
| 56 | 56 | } | |
| 57 | 57 | ||
| 58 | - | func TestBrowserLauncher_LaunchURL_ReportsRelayErrorWhenDelivered(t *testing.T) { | |
| 58 | + | func TestBrowserLauncher_LaunchURL_ReturnsUnconfirmedErrorWhenRelayDeliveryIsAmbiguous(t *testing.T) { | |
| 59 | 59 | relay := mocks.NewMockBrowserLaunchRelay(t) | |
| 60 | - | wantErr := errors.New("relay exploded") | |
| 61 | - | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(true, wantErr) | |
| 60 | + | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(true, ErrBrowserLaunchRelayUnconfirmed) | |
| 62 | 61 | ||
| 63 | 62 | launcher := NewBrowserLauncher(relay) | |
| 64 | 63 | launcher.resolveExecutablePath = func() (string, error) { | |
| @@ -66,22 +65,33 @@ func TestBrowserLauncher_LaunchURL_ReportsRelayErrorWhenDelivered(t *testing.T) | |||
| 66 | 65 | return "", nil | |
| 67 | 66 | } | |
| 68 | 67 | launcher.startDetachedProcess = func(_ *exec.Cmd) error { | |
| 69 | - | t.Fatal("expected launch to stop after relay delivery") | |
| 68 | + | t.Fatal("expected launch to stop after ambiguous relay delivery") | |
| 70 | 69 | return nil | |
| 71 | 70 | } | |
| 72 | 71 | ||
| 73 | - | oldStderr := os.Stderr | |
| 74 | - | r, w, err := os.Pipe() | |
| 75 | - | require.NoError(t, err) | |
| 76 | - | os.Stderr = w | |
| 77 | - | t.Cleanup(func() { | |
| 78 | - | os.Stderr = oldStderr | |
| 79 | - | }) | |
| 72 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") | |
| 73 | + | ||
| 74 | + | require.Error(t, err) | |
| 75 | + | assert.ErrorIs(t, err, ErrBrowserLaunchUnconfirmed) | |
| 76 | + | } | |
| 80 | 77 | ||
| 81 | - | launcher.LaunchURL(context.Background(), "https://example.com") | |
| 78 | + | func TestBrowserLauncher_LaunchURL_PropagatesRelayError(t *testing.T) { | |
| 79 | + | relay := mocks.NewMockBrowserLaunchRelay(t) | |
| 80 | + | wantErr := errors.New("relay exploded") | |
| 81 | + | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(false, wantErr) | |
| 82 | 82 | ||
| 83 | - | require.NoError(t, w.Close()) | |
| 84 | - | output, err := io.ReadAll(r) | |
| 85 | - | require.NoError(t, err) | |
| 86 | - | assert.Contains(t, string(output), wantErr.Error()) | |
| 83 | + | launcher := NewBrowserLauncher(relay) | |
| 84 | + | launcher.resolveExecutablePath = func() (string, error) { | |
| 85 | + | t.Fatal("expected executable path resolution to be skipped") | |
| 86 | + | return "", nil | |
| 87 | + | } | |
| 88 | + | launcher.startDetachedProcess = func(_ *exec.Cmd) error { | |
| 89 | + | t.Fatal("expected launch to stop after relay error") | |
| 90 | + | return nil | |
| 91 | + | } | |
| 92 | + | ||
| 93 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") | |
| 94 | + | ||
| 95 | + | require.Error(t, err) | |
| 96 | + | assert.ErrorIs(t, err, wantErr) | |
| 87 | 97 | } | |
| @@ -3,8 +3,6 @@ package desktop | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "errors" | 5 | "errors" | ||
| 6 | - | "io" | |||
| 7 | - | "os" | |||
| 8 | "os/exec" | 6 | "os/exec" | ||
| 9 | "testing" | 7 | "testing" | ||
| 10 | 8 | ||||
| @@ -28,8 +26,9 @@ func TestBrowserLauncher_LaunchURL_ReturnsWithoutSpawningWhenRelayDelivers(t *te | |||||
| 28 | return nil | 26 | return nil | ||
| 29 | } | 27 | } | ||
| 30 | 28 | ||||
| 31 | - | launcher.LaunchURL(context.Background(), "https://example.com") | 29 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") |
| 32 | 30 | ||||
| 31 | + | require.NoError(t, err) | |||
| 33 | assert.False(t, spawned) | 32 | assert.False(t, spawned) | ||
| 34 | } | 33 | } | ||
| 35 | 34 | ||||
| @@ -48,17 +47,17 @@ func TestBrowserLauncher_LaunchURL_FallsBackToSpawnWhenRelayMisses(t *testing.T) | |||||
| 48 | return nil | 47 | return nil | ||
| 49 | } | 48 | } | ||
| 50 | 49 | ||||
| 51 | - | launcher.LaunchURL(context.Background(), "https://example.com") | 50 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") |
| 52 | 51 | ||||
| 52 | + | require.NoError(t, err) | |||
| 53 | require.NotNil(t, gotCmd) | 53 | require.NotNil(t, gotCmd) | ||
| 54 | assert.Equal(t, "/usr/bin/dumber", gotCmd.Path) | 54 | assert.Equal(t, "/usr/bin/dumber", gotCmd.Path) | ||
| 55 | assert.Equal(t, []string{"/usr/bin/dumber", "browse", "https://example.com"}, gotCmd.Args) | 55 | assert.Equal(t, []string{"/usr/bin/dumber", "browse", "https://example.com"}, gotCmd.Args) | ||
| 56 | } | 56 | } | ||
| 57 | 57 | ||||
| 58 | - | func TestBrowserLauncher_LaunchURL_ReportsRelayErrorWhenDelivered(t *testing.T) { | 58 | + | func TestBrowserLauncher_LaunchURL_ReturnsUnconfirmedErrorWhenRelayDeliveryIsAmbiguous(t *testing.T) { |
| 59 | relay := mocks.NewMockBrowserLaunchRelay(t) | 59 | relay := mocks.NewMockBrowserLaunchRelay(t) | ||
| 60 | - | wantErr := errors.New("relay exploded") | 60 | + | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(true, ErrBrowserLaunchRelayUnconfirmed) |
| 61 | - | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(true, wantErr) | |||
| 62 | 61 | ||||
| 63 | launcher := NewBrowserLauncher(relay) | 62 | launcher := NewBrowserLauncher(relay) | ||
| 64 | launcher.resolveExecutablePath = func() (string, error) { | 63 | launcher.resolveExecutablePath = func() (string, error) { | ||
| @@ -66,22 +65,33 @@ func TestBrowserLauncher_LaunchURL_ReportsRelayErrorWhenDelivered(t *testing.T) | |||||
| 66 | return "", nil | 65 | return "", nil | ||
| 67 | } | 66 | } | ||
| 68 | launcher.startDetachedProcess = func(_ *exec.Cmd) error { | 67 | launcher.startDetachedProcess = func(_ *exec.Cmd) error { | ||
| 69 | - | t.Fatal("expected launch to stop after relay delivery") | 68 | + | t.Fatal("expected launch to stop after ambiguous relay delivery") |
| 70 | return nil | 69 | return nil | ||
| 71 | } | 70 | } | ||
| 72 | 71 | ||||
| 73 | - | oldStderr := os.Stderr | 72 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") |
| 74 | - | r, w, err := os.Pipe() | 73 | + | |
| 75 | - | require.NoError(t, err) | 74 | + | require.Error(t, err) |
| 76 | - | os.Stderr = w | 75 | + | assert.ErrorIs(t, err, ErrBrowserLaunchUnconfirmed) |
| 77 | - | t.Cleanup(func() { | 76 | + | } |
| 78 | - | os.Stderr = oldStderr | |||
| 79 | - | }) | |||
| 80 | 77 | ||||
| 81 | - | launcher.LaunchURL(context.Background(), "https://example.com") | 78 | + | func TestBrowserLauncher_LaunchURL_PropagatesRelayError(t *testing.T) { |
| 79 | + | relay := mocks.NewMockBrowserLaunchRelay(t) | |||
| 80 | + | wantErr := errors.New("relay exploded") | |||
| 81 | + | relay.EXPECT().DeliverOpenFreshWindow(context.Background(), "https://example.com").Return(false, wantErr) | |||
| 82 | 82 | ||||
| 83 | - | require.NoError(t, w.Close()) | 83 | + | launcher := NewBrowserLauncher(relay) |
| 84 | - | output, err := io.ReadAll(r) | 84 | + | launcher.resolveExecutablePath = func() (string, error) { |
| 85 | - | require.NoError(t, err) | 85 | + | t.Fatal("expected executable path resolution to be skipped") |
| 86 | - | assert.Contains(t, string(output), wantErr.Error()) | 86 | + | return "", nil |
| 87 | + | } | |||
| 88 | + | launcher.startDetachedProcess = func(_ *exec.Cmd) error { | |||
| 89 | + | t.Fatal("expected launch to stop after relay error") | |||
| 90 | + | return nil | |||
| 91 | + | } | |||
| 92 | + | ||||
| 93 | + | err := launcher.LaunchURL(context.Background(), "https://example.com") | |||
| 94 | + | ||||
| 95 | + | require.Error(t, err) | |||
| 96 | + | assert.ErrorIs(t, err, wantErr) | |||
| 87 | } | 97 | } | ||
M internal/ui/app.go +31 -30
| @@ -1290,7 +1290,7 @@ func (a *App) Cancel(ctx context.Context) { | |||
| 1290 | 1290 | } | |
| 1291 | 1291 | ||
| 1292 | 1292 | type omniboxCallbacks struct { | |
| 1293 | - | OnNavigate func(url string) | |
| 1293 | + | OnNavigate func(ctx context.Context, url string) error | |
| 1294 | 1294 | OnToast func(ctx context.Context, message string, level component.ToastLevel) | |
| 1295 | 1295 | OnFocusIn func(entry *gtk.SearchEntry) | |
| 1296 | 1296 | OnFocusOut func() | |
| @@ -1346,8 +1346,8 @@ func NewStandaloneOmniboxRuntime( | |||
| 1346 | 1346 | } | |
| 1347 | 1347 | ||
| 1348 | 1348 | omniboxCfg := buildOmniboxConfig(deps, faviconAdapter, omniboxCallbacks{ | |
| 1349 | - | OnNavigate: func(url string) { | |
| 1350 | - | handleStandaloneOmniboxNavigation(deps, ctx, url) | |
| 1349 | + | OnNavigate: func(navCtx context.Context, url string) error { | |
| 1350 | + | return handleStandaloneOmniboxNavigation(deps, navCtx, url) | |
| 1351 | 1351 | }, | |
| 1352 | 1352 | OnToast: func(toastCtx context.Context, message string, level component.ToastLevel) { | |
| 1353 | 1353 | logging.FromContext(toastCtx).Debug().Str("message", message).Int("level", int(level)).Msg("standalone omnibox toast") | |
| @@ -1365,18 +1365,21 @@ func NewStandaloneOmniboxRuntime( | |||
| 1365 | 1365 | }} | |
| 1366 | 1366 | } | |
| 1367 | 1367 | ||
| 1368 | - | func handleStandaloneOmniboxNavigation(deps *Dependencies, ctx context.Context, rawURL string) { | |
| 1368 | + | func handleStandaloneOmniboxNavigation(deps *Dependencies, ctx context.Context, rawURL string) error { | |
| 1369 | + | if deps == nil { | |
| 1370 | + | return fmt.Errorf("standalone omnibox dependencies are not configured") | |
| 1371 | + | } | |
| 1369 | 1372 | if urlutil.IsExternalScheme(rawURL) { | |
| 1370 | - | if deps != nil && deps.LaunchExternalURL != nil { | |
| 1371 | - | deps.LaunchExternalURL(rawURL) | |
| 1372 | - | return | |
| 1373 | + | if deps.LaunchExternalURL == nil { | |
| 1374 | + | return fmt.Errorf("external launcher is not configured") | |
| 1373 | 1375 | } | |
| 1374 | - | } else if deps != nil && deps.LaunchBrowserURL != nil { | |
| 1375 | - | deps.LaunchBrowserURL(rawURL) | |
| 1376 | - | return | |
| 1376 | + | deps.LaunchExternalURL(rawURL) | |
| 1377 | + | return nil | |
| 1377 | 1378 | } | |
| 1378 | - | ||
| 1379 | - | logging.FromContext(ctx).Info().Str("url", rawURL).Msg("standalone omnibox navigate submitted") | |
| 1379 | + | if deps.LaunchBrowserURL == nil { | |
| 1380 | + | return fmt.Errorf("browser launcher is not configured") | |
| 1381 | + | } | |
| 1382 | + | return deps.LaunchBrowserURL(ctx, rawURL) | |
| 1380 | 1383 | } | |
| 1381 | 1384 | ||
| 1382 | 1385 | func (a *App) initOmniboxConfig(ctx context.Context) { | |
| @@ -1384,12 +1387,12 @@ func (a *App) initOmniboxConfig(ctx context.Context) { | |||
| 1384 | 1387 | return | |
| 1385 | 1388 | } | |
| 1386 | 1389 | ||
| 1387 | - | log := logging.FromContext(ctx) | |
| 1388 | 1390 | a.omniboxCfg = buildOmniboxConfig(a.deps, a.faviconAdapter, omniboxCallbacks{ | |
| 1389 | - | OnNavigate: func(url string) { | |
| 1390 | - | if err := a.navigateFromOmnibox(ctx, url); err != nil { | |
| 1391 | - | log.Error().Err(err).Str("url", url).Msg("navigation failed") | |
| 1392 | - | } | |
| 1391 | + | OnNavigate: func(navCtx context.Context, url string) error { | |
| 1392 | + | return a.navigateFromOmnibox(navCtx, url) | |
| 1393 | + | }, | |
| 1394 | + | OnToast: func(toastCtx context.Context, message string, level component.ToastLevel) { | |
| 1395 | + | a.showToastOnLastFocusedBrowserWindow(toastCtx, message, level) | |
| 1393 | 1396 | }, | |
| 1394 | 1397 | OnFocusIn: func(entry *gtk.SearchEntry) { | |
| 1395 | 1398 | // Set omnibox entry as the focused input for accent picker | |
| @@ -1413,7 +1416,7 @@ func (a *App) initOmniboxConfig(ctx context.Context) { | |||
| 1413 | 1416 | }, | |
| 1414 | 1417 | }) | |
| 1415 | 1418 | a.navCoord.SetOmniboxProvider(a) | |
| 1416 | - | log.Debug().Msg("omnibox config stored, provider set") | |
| 1419 | + | logging.FromContext(ctx).Debug().Msg("omnibox config stored, provider set") | |
| 1417 | 1420 | } | |
| 1418 | 1421 | ||
| 1419 | 1422 | func (a *App) initFindBarConfig(ctx context.Context) { | |
| @@ -2128,15 +2131,12 @@ func omniboxNavigateForBrowserWindow( | |||
| 2128 | 2131 | ctx context.Context, | |
| 2129 | 2132 | bw *browserWindow, | |
| 2130 | 2133 | navigate func(context.Context, *browserWindow, string) error, | |
| 2131 | - | ) func(string) { | |
| 2132 | - | return func(url string) { | |
| 2133 | - | if err := navigate(ctx, bw, url); err != nil { | |
| 2134 | - | logging.FromContext(ctx).Error(). | |
| 2135 | - | Err(err). | |
| 2136 | - | Str("url", url). | |
| 2137 | - | Str("window_id", bw.id). | |
| 2138 | - | Msg("omnibox navigation failed") | |
| 2134 | + | ) func(context.Context, string) error { | |
| 2135 | + | return func(navCtx context.Context, url string) error { | |
| 2136 | + | if navCtx == nil { | |
| 2137 | + | navCtx = ctx | |
| 2139 | 2138 | } | |
| 2139 | + | return navigate(navCtx, bw, url) | |
| 2140 | 2140 | } | |
| 2141 | 2141 | } | |
| 2142 | 2142 | ||
| @@ -4368,13 +4368,14 @@ func (a *App) showFloatingOmnibox(ctx context.Context, session *floatingWorkspac | |||
| 4368 | 4368 | ||
| 4369 | 4369 | if session.omnibox == nil { | |
| 4370 | 4370 | cfg := a.omniboxCfg | |
| 4371 | - | cfg.OnNavigate = func(url string) { | |
| 4371 | + | cfg.OnNavigate = func(navCtx context.Context, url string) error { | |
| 4372 | 4372 | if session.pane == nil { | |
| 4373 | - | return | |
| 4373 | + | return fmt.Errorf("floating pane is not available") | |
| 4374 | 4374 | } | |
| 4375 | - | if err := session.pane.Navigate(ctx, url); err != nil { | |
| 4376 | - | logging.FromContext(ctx).Error().Err(err).Str("url", url).Msg("floating omnibox navigation failed") | |
| 4375 | + | if navCtx == nil { | |
| 4376 | + | navCtx = ctx | |
| 4377 | 4377 | } | |
| 4378 | + | return session.pane.Navigate(navCtx, url) | |
| 4378 | 4379 | } | |
| 4379 | 4380 | cfg.OnToast = func(toastCtx context.Context, message string, level component.ToastLevel) { | |
| 4380 | 4381 | a.showToastOnLastFocusedBrowserWindow(toastCtx, message, level) | |
| @@ -1290,7 +1290,7 @@ func (a *App) Cancel(ctx context.Context) { | |||||
| 1290 | } | 1290 | } | ||
| 1291 | 1291 | ||||
| 1292 | type omniboxCallbacks struct { | 1292 | type omniboxCallbacks struct { | ||
| 1293 | - | OnNavigate func(url string) | 1293 | + | OnNavigate func(ctx context.Context, url string) error |
| 1294 | OnToast func(ctx context.Context, message string, level component.ToastLevel) | 1294 | OnToast func(ctx context.Context, message string, level component.ToastLevel) | ||
| 1295 | OnFocusIn func(entry *gtk.SearchEntry) | 1295 | OnFocusIn func(entry *gtk.SearchEntry) | ||
| 1296 | OnFocusOut func() | 1296 | OnFocusOut func() | ||
| @@ -1346,8 +1346,8 @@ func NewStandaloneOmniboxRuntime( | |||||
| 1346 | } | 1346 | } | ||
| 1347 | 1347 | ||||
| 1348 | omniboxCfg := buildOmniboxConfig(deps, faviconAdapter, omniboxCallbacks{ | 1348 | omniboxCfg := buildOmniboxConfig(deps, faviconAdapter, omniboxCallbacks{ | ||
| 1349 | - | OnNavigate: func(url string) { | 1349 | + | OnNavigate: func(navCtx context.Context, url string) error { |
| 1350 | - | handleStandaloneOmniboxNavigation(deps, ctx, url) | 1350 | + | return handleStandaloneOmniboxNavigation(deps, navCtx, url) |
| 1351 | }, | 1351 | }, | ||
| 1352 | OnToast: func(toastCtx context.Context, message string, level component.ToastLevel) { | 1352 | OnToast: func(toastCtx context.Context, message string, level component.ToastLevel) { | ||
| 1353 | logging.FromContext(toastCtx).Debug().Str("message", message).Int("level", int(level)).Msg("standalone omnibox toast") | 1353 | logging.FromContext(toastCtx).Debug().Str("message", message).Int("level", int(level)).Msg("standalone omnibox toast") | ||
| @@ -1365,18 +1365,21 @@ func NewStandaloneOmniboxRuntime( | |||||
| 1365 | }} | 1365 | }} | ||
| 1366 | } | 1366 | } | ||
| 1367 | 1367 | ||||
| 1368 | - | func handleStandaloneOmniboxNavigation(deps *Dependencies, ctx context.Context, rawURL string) { | 1368 | + | func handleStandaloneOmniboxNavigation(deps *Dependencies, ctx context.Context, rawURL string) error { |
| 1369 | + | if deps == nil { | |||
| 1370 | + | return fmt.Errorf("standalone omnibox dependencies are not configured") | |||
| 1371 | + | } | |||
| 1369 | if urlutil.IsExternalScheme(rawURL) { | 1372 | if urlutil.IsExternalScheme(rawURL) { | ||
| 1370 | - | if deps != nil && deps.LaunchExternalURL != nil { | 1373 | + | if deps.LaunchExternalURL == nil { |
| 1371 | - | deps.LaunchExternalURL(rawURL) | 1374 | + | return fmt.Errorf("external launcher is not configured") |
| 1372 | - | return | |||
| 1373 | } | 1375 | } | ||
| 1374 | - | } else if deps != nil && deps.LaunchBrowserURL != nil { | 1376 | + | deps.LaunchExternalURL(rawURL) |
| 1375 | - | deps.LaunchBrowserURL(rawURL) | 1377 | + | return nil |
| 1376 | - | return | |||
| 1377 | } | 1378 | } | ||
| 1378 | - | 1379 | + | if deps.LaunchBrowserURL == nil { | |
| 1379 | - | logging.FromContext(ctx).Info().Str("url", rawURL).Msg("standalone omnibox navigate submitted") | 1380 | + | return fmt.Errorf("browser launcher is not configured") |
| 1381 | + | } | |||
| 1382 | + | return deps.LaunchBrowserURL(ctx, rawURL) | |||
| 1380 | } | 1383 | } | ||
| 1381 | 1384 | ||||
| 1382 | func (a *App) initOmniboxConfig(ctx context.Context) { | 1385 | func (a *App) initOmniboxConfig(ctx context.Context) { | ||
| @@ -1384,12 +1387,12 @@ func (a *App) initOmniboxConfig(ctx context.Context) { | |||||
| 1384 | return | 1387 | return | ||
| 1385 | } | 1388 | } | ||
| 1386 | 1389 | ||||
| 1387 | - | log := logging.FromContext(ctx) | |||
| 1388 | a.omniboxCfg = buildOmniboxConfig(a.deps, a.faviconAdapter, omniboxCallbacks{ | 1390 | a.omniboxCfg = buildOmniboxConfig(a.deps, a.faviconAdapter, omniboxCallbacks{ | ||
| 1389 | - | OnNavigate: func(url string) { | 1391 | + | OnNavigate: func(navCtx context.Context, url string) error { |
| 1390 | - | if err := a.navigateFromOmnibox(ctx, url); err != nil { | 1392 | + | return a.navigateFromOmnibox(navCtx, url) |
| 1391 | - | log.Error().Err(err).Str("url", url).Msg("navigation failed") | 1393 | + | }, |
| 1392 | - | } | 1394 | + | OnToast: func(toastCtx context.Context, message string, level component.ToastLevel) { |
| 1395 | + | a.showToastOnLastFocusedBrowserWindow(toastCtx, message, level) | |||
| 1393 | }, | 1396 | }, | ||
| 1394 | OnFocusIn: func(entry *gtk.SearchEntry) { | 1397 | OnFocusIn: func(entry *gtk.SearchEntry) { | ||
| 1395 | // Set omnibox entry as the focused input for accent picker | 1398 | // Set omnibox entry as the focused input for accent picker | ||
| @@ -1413,7 +1416,7 @@ func (a *App) initOmniboxConfig(ctx context.Context) { | |||||
| 1413 | }, | 1416 | }, | ||
| 1414 | }) | 1417 | }) | ||
| 1415 | a.navCoord.SetOmniboxProvider(a) | 1418 | a.navCoord.SetOmniboxProvider(a) | ||
| 1416 | - | log.Debug().Msg("omnibox config stored, provider set") | 1419 | + | logging.FromContext(ctx).Debug().Msg("omnibox config stored, provider set") |
| 1417 | } | 1420 | } | ||
| 1418 | 1421 | ||||
| 1419 | func (a *App) initFindBarConfig(ctx context.Context) { | 1422 | func (a *App) initFindBarConfig(ctx context.Context) { | ||
| @@ -2128,15 +2131,12 @@ func omniboxNavigateForBrowserWindow( | |||||
| 2128 | ctx context.Context, | 2131 | ctx context.Context, | ||
| 2129 | bw *browserWindow, | 2132 | bw *browserWindow, | ||
| 2130 | navigate func(context.Context, *browserWindow, string) error, | 2133 | navigate func(context.Context, *browserWindow, string) error, | ||
| 2131 | - | ) func(string) { | 2134 | + | ) func(context.Context, string) error { |
| 2132 | - | return func(url string) { | 2135 | + | return func(navCtx context.Context, url string) error { |
| 2133 | - | if err := navigate(ctx, bw, url); err != nil { | 2136 | + | if navCtx == nil { |
| 2134 | - | logging.FromContext(ctx).Error(). | 2137 | + | navCtx = ctx |
| 2135 | - | Err(err). | |||
| 2136 | - | Str("url", url). | |||
| 2137 | - | Str("window_id", bw.id). | |||
| 2138 | - | Msg("omnibox navigation failed") | |||
| 2139 | } | 2138 | } | ||
| 2139 | + | return navigate(navCtx, bw, url) | |||
| 2140 | } | 2140 | } | ||
| 2141 | } | 2141 | } | ||
| 2142 | 2142 | ||||
| @@ -4368,13 +4368,14 @@ func (a *App) showFloatingOmnibox(ctx context.Context, session *floatingWorkspac | |||||
| 4368 | 4368 | ||||
| 4369 | if session.omnibox == nil { | 4369 | if session.omnibox == nil { | ||
| 4370 | cfg := a.omniboxCfg | 4370 | cfg := a.omniboxCfg | ||
| 4371 | - | cfg.OnNavigate = func(url string) { | 4371 | + | cfg.OnNavigate = func(navCtx context.Context, url string) error { |
| 4372 | if session.pane == nil { | 4372 | if session.pane == nil { | ||
| 4373 | - | return | 4373 | + | return fmt.Errorf("floating pane is not available") |
| 4374 | } | 4374 | } | ||
| 4375 | - | if err := session.pane.Navigate(ctx, url); err != nil { | 4375 | + | if navCtx == nil { |
| 4376 | - | logging.FromContext(ctx).Error().Err(err).Str("url", url).Msg("floating omnibox navigation failed") | 4376 | + | navCtx = ctx |
| 4377 | } | 4377 | } | ||
| 4378 | + | return session.pane.Navigate(navCtx, url) | |||
| 4378 | } | 4379 | } | ||
| 4379 | cfg.OnToast = func(toastCtx context.Context, message string, level component.ToastLevel) { | 4380 | cfg.OnToast = func(toastCtx context.Context, message string, level component.ToastLevel) { | ||
| 4380 | a.showToastOnLastFocusedBrowserWindow(toastCtx, message, level) | 4381 | a.showToastOnLastFocusedBrowserWindow(toastCtx, message, level) | ||
M internal/ui/app_browser_launch_test.go +11 -5
| @@ -214,7 +214,7 @@ func setWindowTabBar(t *testing.T, mw *window.MainWindow, tabBar *component.TabB | |||
| 214 | 214 | reflect.NewAt(fv.Type(), unsafe.Pointer(fv.UnsafeAddr())).Elem().Set(reflect.ValueOf(tabBar)) | |
| 215 | 215 | } | |
| 216 | 216 | ||
| 217 | - | func omniboxNavigateCallbackForTest(t *testing.T, omnibox *component.Omnibox) func(string) { | |
| 217 | + | func omniboxNavigateCallbackForTest(t *testing.T, omnibox *component.Omnibox) func(context.Context, string) error { | |
| 218 | 218 | t.Helper() | |
| 219 | 219 | if omnibox == nil { | |
| 220 | 220 | t.Fatal("omnibox is nil") | |
| @@ -225,7 +225,7 @@ func omniboxNavigateCallbackForTest(t *testing.T, omnibox *component.Omnibox) fu | |||
| 225 | 225 | if !fv.IsValid() { | |
| 226 | 226 | t.Fatal("Omnibox missing onNavigate field") | |
| 227 | 227 | } | |
| 228 | - | cb, ok := reflect.NewAt(fv.Type(), unsafe.Pointer(fv.UnsafeAddr())).Elem().Interface().(func(string)) | |
| 228 | + | cb, ok := reflect.NewAt(fv.Type(), unsafe.Pointer(fv.UnsafeAddr())).Elem().Interface().(func(context.Context, string) error) | |
| 229 | 229 | if !ok || cb == nil { | |
| 230 | 230 | t.Fatal("omnibox onNavigate callback is nil or has unexpected type") | |
| 231 | 231 | } | |
| @@ -2117,7 +2117,9 @@ func TestApp_OmniboxNavigateCallbackCapturesBrowserWindow(t *testing.T) { | |||
| 2117 | 2117 | ||
| 2118 | 2118 | ctx := context.Background() | |
| 2119 | 2119 | cb := omniboxNavigateForBrowserWindow(ctx, second, testNavigate) | |
| 2120 | - | cb("https://google.com") | |
| 2120 | + | if err := cb(ctx, "https://google.com"); err != nil { | |
| 2121 | + | t.Fatalf("omnibox navigate callback returned error: %v", err) | |
| 2122 | + | } | |
| 2121 | 2123 | ||
| 2122 | 2124 | if capturedBW != second { | |
| 2123 | 2125 | capturedID := "<nil>" | |
| @@ -2496,7 +2498,9 @@ func TestApp_WorkspaceOmniboxNavigateUsesOwnerWindow(t *testing.T) { | |||
| 2496 | 2498 | } | |
| 2497 | 2499 | wsView.ShowOmnibox(ctx, "") | |
| 2498 | 2500 | cb := omniboxNavigateCallbackForTest(t, wsView.GetOmnibox()) | |
| 2499 | - | cb("https://example.com") | |
| 2501 | + | if err := cb(ctx, "https://example.com"); err != nil { | |
| 2502 | + | t.Fatalf("omnibox navigate callback returned error: %v", err) | |
| 2503 | + | } | |
| 2500 | 2504 | ||
| 2501 | 2505 | if fakeWv1.loadURICalled { | |
| 2502 | 2506 | t.Errorf("first window webview navigated to %q, want no navigation", fakeWv1.loadURILastURI) | |
| @@ -2530,7 +2534,9 @@ func TestApp_FloatingOmniboxNavigateUsesSessionPane(t *testing.T) { | |||
| 2530 | 2534 | ||
| 2531 | 2535 | app.showFloatingOmnibox(ctx, session) | |
| 2532 | 2536 | cb := omniboxNavigateCallbackForTest(t, session.omnibox) | |
| 2533 | - | cb("https://example.com") | |
| 2537 | + | if err := cb(ctx, "https://example.com"); err != nil { | |
| 2538 | + | t.Fatalf("floating omnibox navigate callback returned error: %v", err) | |
| 2539 | + | } | |
| 2534 | 2540 | ||
| 2535 | 2541 | if loadedURL != "https://example.com" { | |
| 2536 | 2542 | t.Fatalf("floating pane loaded URL = %q, want https://example.com", loadedURL) | |
| @@ -214,7 +214,7 @@ func setWindowTabBar(t *testing.T, mw *window.MainWindow, tabBar *component.TabB | |||||
| 214 | reflect.NewAt(fv.Type(), unsafe.Pointer(fv.UnsafeAddr())).Elem().Set(reflect.ValueOf(tabBar)) | 214 | reflect.NewAt(fv.Type(), unsafe.Pointer(fv.UnsafeAddr())).Elem().Set(reflect.ValueOf(tabBar)) | ||
| 215 | } | 215 | } | ||
| 216 | 216 | ||||
| 217 | - | func omniboxNavigateCallbackForTest(t *testing.T, omnibox *component.Omnibox) func(string) { | 217 | + | func omniboxNavigateCallbackForTest(t *testing.T, omnibox *component.Omnibox) func(context.Context, string) error { |
| 218 | t.Helper() | 218 | t.Helper() | ||
| 219 | if omnibox == nil { | 219 | if omnibox == nil { | ||
| 220 | t.Fatal("omnibox is nil") | 220 | t.Fatal("omnibox is nil") | ||
| @@ -225,7 +225,7 @@ func omniboxNavigateCallbackForTest(t *testing.T, omnibox *component.Omnibox) fu | |||||
| 225 | if !fv.IsValid() { | 225 | if !fv.IsValid() { | ||
| 226 | t.Fatal("Omnibox missing onNavigate field") | 226 | t.Fatal("Omnibox missing onNavigate field") | ||
| 227 | } | 227 | } | ||
| 228 | - | cb, ok := reflect.NewAt(fv.Type(), unsafe.Pointer(fv.UnsafeAddr())).Elem().Interface().(func(string)) | 228 | + | cb, ok := reflect.NewAt(fv.Type(), unsafe.Pointer(fv.UnsafeAddr())).Elem().Interface().(func(context.Context, string) error) |
| 229 | if !ok || cb == nil { | 229 | if !ok || cb == nil { | ||
| 230 | t.Fatal("omnibox onNavigate callback is nil or has unexpected type") | 230 | t.Fatal("omnibox onNavigate callback is nil or has unexpected type") | ||
| 231 | } | 231 | } | ||
| @@ -2117,7 +2117,9 @@ func TestApp_OmniboxNavigateCallbackCapturesBrowserWindow(t *testing.T) { | |||||
| 2117 | 2117 | ||||
| 2118 | ctx := context.Background() | 2118 | ctx := context.Background() | ||
| 2119 | cb := omniboxNavigateForBrowserWindow(ctx, second, testNavigate) | 2119 | cb := omniboxNavigateForBrowserWindow(ctx, second, testNavigate) | ||
| 2120 | - | cb("https://google.com") | 2120 | + | if err := cb(ctx, "https://google.com"); err != nil { |
| 2121 | + | t.Fatalf("omnibox navigate callback returned error: %v", err) | |||
| 2122 | + | } | |||
| 2121 | 2123 | ||||
| 2122 | if capturedBW != second { | 2124 | if capturedBW != second { | ||
| 2123 | capturedID := "<nil>" | 2125 | capturedID := "<nil>" | ||
| @@ -2496,7 +2498,9 @@ func TestApp_WorkspaceOmniboxNavigateUsesOwnerWindow(t *testing.T) { | |||||
| 2496 | } | 2498 | } | ||
| 2497 | wsView.ShowOmnibox(ctx, "") | 2499 | wsView.ShowOmnibox(ctx, "") | ||
| 2498 | cb := omniboxNavigateCallbackForTest(t, wsView.GetOmnibox()) | 2500 | cb := omniboxNavigateCallbackForTest(t, wsView.GetOmnibox()) | ||
| 2499 | - | cb("https://example.com") | 2501 | + | if err := cb(ctx, "https://example.com"); err != nil { |
| 2502 | + | t.Fatalf("omnibox navigate callback returned error: %v", err) | |||
| 2503 | + | } | |||
| 2500 | 2504 | ||||
| 2501 | if fakeWv1.loadURICalled { | 2505 | if fakeWv1.loadURICalled { | ||
| 2502 | t.Errorf("first window webview navigated to %q, want no navigation", fakeWv1.loadURILastURI) | 2506 | t.Errorf("first window webview navigated to %q, want no navigation", fakeWv1.loadURILastURI) | ||
| @@ -2530,7 +2534,9 @@ func TestApp_FloatingOmniboxNavigateUsesSessionPane(t *testing.T) { | |||||
| 2530 | 2534 | ||||
| 2531 | app.showFloatingOmnibox(ctx, session) | 2535 | app.showFloatingOmnibox(ctx, session) | ||
| 2532 | cb := omniboxNavigateCallbackForTest(t, session.omnibox) | 2536 | cb := omniboxNavigateCallbackForTest(t, session.omnibox) | ||
| 2533 | - | cb("https://example.com") | 2537 | + | if err := cb(ctx, "https://example.com"); err != nil { |
| 2538 | + | t.Fatalf("floating omnibox navigate callback returned error: %v", err) | |||
| 2539 | + | } | |||
| 2534 | 2540 | ||||
| 2535 | if loadedURL != "https://example.com" { | 2541 | if loadedURL != "https://example.com" { | ||
| 2536 | t.Fatalf("floating pane loaded URL = %q, want https://example.com", loadedURL) | 2542 | t.Fatalf("floating pane loaded URL = %q, want https://example.com", loadedURL) | ||
M internal/ui/component/omnibox.go +33 -25
| @@ -120,7 +120,7 @@ type Omnibox struct { | |||
| 120 | 120 | ctx context.Context | |
| 121 | 121 | ||
| 122 | 122 | // Callbacks | |
| 123 | - | onNavigate func(url string) | |
| 123 | + | onNavigate func(ctx context.Context, url string) error | |
| 124 | 124 | onClose func() | |
| 125 | 125 | onToast func(ctx context.Context, message string, level ToastLevel) | |
| 126 | 126 | onAccentKeyPress func(keyval uint, state gdk.ModifierType) bool | |
| @@ -164,14 +164,15 @@ type OmniboxConfig struct { | |||
| 164 | 164 | InitialBehavior entity.OmniboxInitialBehavior | |
| 165 | 165 | MostVisitedDays int | |
| 166 | 166 | SaveInitialBehavior func(ctx context.Context, behavior entity.OmniboxInitialBehavior) error | |
| 167 | - | UIScale float64 // UI scale for favicon sizing | |
| 168 | - | OnNavigate func(url string) // Callback when user navigates via omnibox | |
| 169 | - | OnToast func(ctx context.Context, message string, level ToastLevel) // Callback to show toast notification | |
| 170 | - | OnFocusIn func(entry *gtk.SearchEntry) // Callback when entry gains focus (for accent picker) | |
| 171 | - | OnFocusOut func() // Callback when entry loses focus | |
| 172 | - | OnAccentKeyPress func(keyval uint, state gdk.ModifierType) bool // Long-press accent detection | |
| 173 | - | OnAccentKeyRelease func(keyval uint) // Key release for accent cancel | |
| 174 | - | SizeConfig ModalSizeConfig // Optional geometry override for omnibox sizing | |
| 167 | + | UIScale float64 // UI scale for favicon sizing | |
| 168 | + | // OnNavigate is called when the user submits a URL; returning nil closes the omnibox. | |
| 169 | + | OnNavigate func(ctx context.Context, url string) error | |
| 170 | + | OnToast func(ctx context.Context, message string, level ToastLevel) // Callback to show toast notification | |
| 171 | + | OnFocusIn func(entry *gtk.SearchEntry) // Callback when entry gains focus (for accent picker) | |
| 172 | + | OnFocusOut func() // Callback when entry loses focus | |
| 173 | + | OnAccentKeyPress func(keyval uint, state gdk.ModifierType) bool // Long-press accent detection | |
| 174 | + | OnAccentKeyRelease func(keyval uint) // Key release for accent cancel | |
| 175 | + | SizeConfig ModalSizeConfig // Optional geometry override for omnibox sizing | |
| 175 | 176 | } | |
| 176 | 177 | ||
| 177 | 178 | // NewOmnibox creates a new native GTK4 omnibox widget. | |
| @@ -771,9 +772,8 @@ func (o *Omnibox) initList() error { | |||
| 771 | 772 | ||
| 772 | 773 | targetURL := resolveTargetURLForSelection(mode, idx, o.effectiveMaxRows(), suggestions, favorites) | |
| 773 | 774 | ||
| 774 | - | if targetURL != "" && o.onNavigate != nil { | |
| 775 | - | o.Hide(o.ctx) | |
| 776 | - | o.onNavigate(targetURL) | |
| 775 | + | if targetURL != "" { | |
| 776 | + | o.submitNavigation(targetURL) | |
| 777 | 777 | } | |
| 778 | 778 | } | |
| 779 | 779 | o.retainedCallbacks = append(o.retainedCallbacks, rowActivatedCb) | |
| @@ -2284,10 +2284,25 @@ func (o *Omnibox) selectAndNavigate(index int) { | |||
| 2284 | 2284 | return | |
| 2285 | 2285 | } | |
| 2286 | 2286 | ||
| 2287 | - | o.Hide(o.ctx) | |
| 2288 | - | if o.onNavigate != nil { | |
| 2289 | - | o.onNavigate(targetURL) | |
| 2287 | + | o.submitNavigation(targetURL) | |
| 2288 | + | } | |
| 2289 | + | ||
| 2290 | + | func (o *Omnibox) submitNavigation(targetURL string) { | |
| 2291 | + | if targetURL == "" { | |
| 2292 | + | return | |
| 2293 | + | } | |
| 2294 | + | if o.onNavigate == nil { | |
| 2295 | + | logging.FromContext(o.ctx).Warn().Msg("omnibox navigate handler is nil") | |
| 2296 | + | return | |
| 2290 | 2297 | } | |
| 2298 | + | if err := o.onNavigate(o.ctx, targetURL); err != nil { | |
| 2299 | + | logging.FromContext(o.ctx).Error().Err(err).Msg("omnibox navigation failed") | |
| 2300 | + | if o.onToast != nil { | |
| 2301 | + | o.onToast(o.ctx, "Unable to open URL", ToastError) | |
| 2302 | + | } | |
| 2303 | + | return | |
| 2304 | + | } | |
| 2305 | + | o.Hide(o.ctx) | |
| 2291 | 2306 | } | |
| 2292 | 2307 | ||
| 2293 | 2308 | // navigateToSelected navigates to the currently selected item or typed URL. | |
| @@ -2313,10 +2328,7 @@ func (o *Omnibox) navigateToSelected() { | |||
| 2313 | 2328 | if targetURL == "" { | |
| 2314 | 2329 | return | |
| 2315 | 2330 | } | |
| 2316 | - | o.Hide(o.ctx) | |
| 2317 | - | if o.onNavigate != nil { | |
| 2318 | - | o.onNavigate(targetURL) | |
| 2319 | - | } | |
| 2331 | + | o.submitNavigation(targetURL) | |
| 2320 | 2332 | return | |
| 2321 | 2333 | } | |
| 2322 | 2334 | } | |
| @@ -2342,11 +2354,7 @@ func (o *Omnibox) navigateToSelected() { | |||
| 2342 | 2354 | return | |
| 2343 | 2355 | } | |
| 2344 | 2356 | ||
| 2345 | - | o.Hide(o.ctx) | |
| 2346 | - | ||
| 2347 | - | if o.onNavigate != nil { | |
| 2348 | - | o.onNavigate(targetURL) | |
| 2349 | - | } | |
| 2357 | + | o.submitNavigation(targetURL) | |
| 2350 | 2358 | } | |
| 2351 | 2359 | ||
| 2352 | 2360 | func resolveTargetURLForSelection(mode ViewMode, idx, maxVisible int, suggestions []Suggestion, favorites []Favorite) string { | |
| @@ -2780,7 +2788,7 @@ func (o *Omnibox) IsVisible() bool { | |||
| 2780 | 2788 | } | |
| 2781 | 2789 | ||
| 2782 | 2790 | // SetOnNavigate sets the callback for URL navigation. | |
| 2783 | - | func (o *Omnibox) SetOnNavigate(fn func(url string)) { | |
| 2791 | + | func (o *Omnibox) SetOnNavigate(fn func(ctx context.Context, url string) error) { | |
| 2784 | 2792 | o.onNavigate = fn | |
| 2785 | 2793 | } | |
| 2786 | 2794 | ||
| @@ -120,7 +120,7 @@ type Omnibox struct { | |||||
| 120 | ctx context.Context | 120 | ctx context.Context | ||
| 121 | 121 | ||||
| 122 | // Callbacks | 122 | // Callbacks | ||
| 123 | - | onNavigate func(url string) | 123 | + | onNavigate func(ctx context.Context, url string) error |
| 124 | onClose func() | 124 | onClose func() | ||
| 125 | onToast func(ctx context.Context, message string, level ToastLevel) | 125 | onToast func(ctx context.Context, message string, level ToastLevel) | ||
| 126 | onAccentKeyPress func(keyval uint, state gdk.ModifierType) bool | 126 | onAccentKeyPress func(keyval uint, state gdk.ModifierType) bool | ||
| @@ -164,14 +164,15 @@ type OmniboxConfig struct { | |||||
| 164 | InitialBehavior entity.OmniboxInitialBehavior | 164 | InitialBehavior entity.OmniboxInitialBehavior | ||
| 165 | MostVisitedDays int | 165 | MostVisitedDays int | ||
| 166 | SaveInitialBehavior func(ctx context.Context, behavior entity.OmniboxInitialBehavior) error | 166 | SaveInitialBehavior func(ctx context.Context, behavior entity.OmniboxInitialBehavior) error | ||
| 167 | - | UIScale float64 // UI scale for favicon sizing | 167 | + | UIScale float64 // UI scale for favicon sizing |
| 168 | - | OnNavigate func(url string) // Callback when user navigates via omnibox | 168 | + | // OnNavigate is called when the user submits a URL; returning nil closes the omnibox. |
| 169 | - | OnToast func(ctx context.Context, message string, level ToastLevel) // Callback to show toast notification | 169 | + | OnNavigate func(ctx context.Context, url string) error |
| 170 | - | OnFocusIn func(entry *gtk.SearchEntry) // Callback when entry gains focus (for accent picker) | 170 | + | OnToast func(ctx context.Context, message string, level ToastLevel) // Callback to show toast notification |
| 171 | - | OnFocusOut func() // Callback when entry loses focus | 171 | + | OnFocusIn func(entry *gtk.SearchEntry) // Callback when entry gains focus (for accent picker) |
| 172 | - | OnAccentKeyPress func(keyval uint, state gdk.ModifierType) bool // Long-press accent detection | 172 | + | OnFocusOut func() // Callback when entry loses focus |
| 173 | - | OnAccentKeyRelease func(keyval uint) // Key release for accent cancel | 173 | + | OnAccentKeyPress func(keyval uint, state gdk.ModifierType) bool // Long-press accent detection |
| 174 | - | SizeConfig ModalSizeConfig // Optional geometry override for omnibox sizing | 174 | + | OnAccentKeyRelease func(keyval uint) // Key release for accent cancel |
| 175 | + | SizeConfig ModalSizeConfig // Optional geometry override for omnibox sizing | |||
| 175 | } | 176 | } | ||
| 176 | 177 | ||||
| 177 | // NewOmnibox creates a new native GTK4 omnibox widget. | 178 | // NewOmnibox creates a new native GTK4 omnibox widget. | ||
| @@ -771,9 +772,8 @@ func (o *Omnibox) initList() error { | |||||
| 771 | 772 | ||||
| 772 | targetURL := resolveTargetURLForSelection(mode, idx, o.effectiveMaxRows(), suggestions, favorites) | 773 | targetURL := resolveTargetURLForSelection(mode, idx, o.effectiveMaxRows(), suggestions, favorites) | ||
| 773 | 774 | ||||
| 774 | - | if targetURL != "" && o.onNavigate != nil { | 775 | + | if targetURL != "" { |
| 775 | - | o.Hide(o.ctx) | 776 | + | o.submitNavigation(targetURL) |
| 776 | - | o.onNavigate(targetURL) | |||
| 777 | } | 777 | } | ||
| 778 | } | 778 | } | ||
| 779 | o.retainedCallbacks = append(o.retainedCallbacks, rowActivatedCb) | 779 | o.retainedCallbacks = append(o.retainedCallbacks, rowActivatedCb) | ||
| @@ -2284,10 +2284,25 @@ func (o *Omnibox) selectAndNavigate(index int) { | |||||
| 2284 | return | 2284 | return | ||
| 2285 | } | 2285 | } | ||
| 2286 | 2286 | ||||
| 2287 | - | o.Hide(o.ctx) | 2287 | + | o.submitNavigation(targetURL) |
| 2288 | - | if o.onNavigate != nil { | 2288 | + | } |
| 2289 | - | o.onNavigate(targetURL) | 2289 | + | |
| 2290 | + | func (o *Omnibox) submitNavigation(targetURL string) { | |||
| 2291 | + | if targetURL == "" { | |||
| 2292 | + | return | |||
| 2293 | + | } | |||
| 2294 | + | if o.onNavigate == nil { | |||
| 2295 | + | logging.FromContext(o.ctx).Warn().Msg("omnibox navigate handler is nil") | |||
| 2296 | + | return | |||
| 2290 | } | 2297 | } | ||
| 2298 | + | if err := o.onNavigate(o.ctx, targetURL); err != nil { | |||
| 2299 | + | logging.FromContext(o.ctx).Error().Err(err).Msg("omnibox navigation failed") | |||
| 2300 | + | if o.onToast != nil { | |||
| 2301 | + | o.onToast(o.ctx, "Unable to open URL", ToastError) | |||
| 2302 | + | } | |||
| 2303 | + | return | |||
| 2304 | + | } | |||
| 2305 | + | o.Hide(o.ctx) | |||
| 2291 | } | 2306 | } | ||
| 2292 | 2307 | ||||
| 2293 | // navigateToSelected navigates to the currently selected item or typed URL. | 2308 | // navigateToSelected navigates to the currently selected item or typed URL. | ||
| @@ -2313,10 +2328,7 @@ func (o *Omnibox) navigateToSelected() { | |||||
| 2313 | if targetURL == "" { | 2328 | if targetURL == "" { | ||
| 2314 | return | 2329 | return | ||
| 2315 | } | 2330 | } | ||
| 2316 | - | o.Hide(o.ctx) | 2331 | + | o.submitNavigation(targetURL) |
| 2317 | - | if o.onNavigate != nil { | |||
| 2318 | - | o.onNavigate(targetURL) | |||
| 2319 | - | } | |||
| 2320 | return | 2332 | return | ||
| 2321 | } | 2333 | } | ||
| 2322 | } | 2334 | } | ||
| @@ -2342,11 +2354,7 @@ func (o *Omnibox) navigateToSelected() { | |||||
| 2342 | return | 2354 | return | ||
| 2343 | } | 2355 | } | ||
| 2344 | 2356 | ||||
| 2345 | - | o.Hide(o.ctx) | 2357 | + | o.submitNavigation(targetURL) |
| 2346 | - | ||||
| 2347 | - | if o.onNavigate != nil { | |||
| 2348 | - | o.onNavigate(targetURL) | |||
| 2349 | - | } | |||
| 2350 | } | 2358 | } | ||
| 2351 | 2359 | ||||
| 2352 | func resolveTargetURLForSelection(mode ViewMode, idx, maxVisible int, suggestions []Suggestion, favorites []Favorite) string { | 2360 | func resolveTargetURLForSelection(mode ViewMode, idx, maxVisible int, suggestions []Suggestion, favorites []Favorite) string { | ||
| @@ -2780,7 +2788,7 @@ func (o *Omnibox) IsVisible() bool { | |||||
| 2780 | } | 2788 | } | ||
| 2781 | 2789 | ||||
| 2782 | // SetOnNavigate sets the callback for URL navigation. | 2790 | // SetOnNavigate sets the callback for URL navigation. | ||
| 2783 | - | func (o *Omnibox) SetOnNavigate(fn func(url string)) { | 2791 | + | func (o *Omnibox) SetOnNavigate(fn func(ctx context.Context, url string) error) { |
| 2784 | o.onNavigate = fn | 2792 | o.onNavigate = fn | ||
| 2785 | } | 2793 | } | ||
| 2786 | 2794 | ||||
M internal/ui/deps.go +3 -2
| @@ -102,8 +102,9 @@ type Dependencies struct { | |||
| 102 | 102 | // Optional: if nil, external URLs are silently dropped. | |
| 103 | 103 | LaunchExternalURL func(uri string) | |
| 104 | 104 | // LaunchBrowserURL opens a URL in a new dumber browser window. | |
| 105 | - | // Optional: if nil, standalone omnibox web navigations are logged only. | |
| 106 | - | LaunchBrowserURL func(uri string) | |
| 105 | + | // Returns an error when the launcher is unavailable or the launch fails. | |
| 106 | + | // Optional: if nil, standalone omnibox navigation returns a configuration error. | |
| 107 | + | LaunchBrowserURL func(ctx context.Context, uri string) error | |
| 107 | 108 | // BrowserLaunchRelay listens for in-process browser launch requests. | |
| 108 | 109 | BrowserLaunchRelay port.BrowserLaunchRelay | |
| 109 | 110 | } | |
| @@ -102,8 +102,9 @@ type Dependencies struct { | |||||
| 102 | // Optional: if nil, external URLs are silently dropped. | 102 | // Optional: if nil, external URLs are silently dropped. | ||
| 103 | LaunchExternalURL func(uri string) | 103 | LaunchExternalURL func(uri string) | ||
| 104 | // LaunchBrowserURL opens a URL in a new dumber browser window. | 104 | // LaunchBrowserURL opens a URL in a new dumber browser window. | ||
| 105 | - | // Optional: if nil, standalone omnibox web navigations are logged only. | 105 | + | // Returns an error when the launcher is unavailable or the launch fails. |
| 106 | - | LaunchBrowserURL func(uri string) | 106 | + | // Optional: if nil, standalone omnibox navigation returns a configuration error. |
| 107 | + | LaunchBrowserURL func(ctx context.Context, uri string) error | |||
| 107 | // BrowserLaunchRelay listens for in-process browser launch requests. | 108 | // BrowserLaunchRelay listens for in-process browser launch requests. | ||
| 108 | BrowserLaunchRelay port.BrowserLaunchRelay | 109 | BrowserLaunchRelay port.BrowserLaunchRelay | ||
| 109 | } | 110 | } | ||
M internal/ui/standalone_omnibox.go +9 -4
| @@ -2,6 +2,7 @@ package ui | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "fmt" | |
| 5 | 6 | "os" | |
| 6 | 7 | "strings" | |
| 7 | 8 | ||
| @@ -154,14 +155,18 @@ func goTestFlagConsumesNextArg(arg string) bool { | |||
| 154 | 155 | } | |
| 155 | 156 | } | |
| 156 | 157 | ||
| 157 | - | func wrapStandaloneOmniboxNavigate(original func(string), quit func()) func(string) { | |
| 158 | - | return func(url string) { | |
| 159 | - | if original != nil { | |
| 160 | - | original(url) | |
| 158 | + | func wrapStandaloneOmniboxNavigate(original func(context.Context, string) error, quit func()) func(context.Context, string) error { | |
| 159 | + | return func(ctx context.Context, url string) error { | |
| 160 | + | if original == nil { | |
| 161 | + | return fmt.Errorf("standalone omnibox navigate handler is not configured") | |
| 162 | + | } | |
| 163 | + | if err := original(ctx, url); err != nil { | |
| 164 | + | return err | |
| 161 | 165 | } | |
| 162 | 166 | if quit != nil { | |
| 163 | 167 | quit() | |
| 164 | 168 | } | |
| 169 | + | return nil | |
| 165 | 170 | } | |
| 166 | 171 | } | |
| 167 | 172 | ||
| @@ -2,6 +2,7 @@ package ui | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "fmt" | |||
| 5 | "os" | 6 | "os" | ||
| 6 | "strings" | 7 | "strings" | ||
| 7 | 8 | ||||
| @@ -154,14 +155,18 @@ func goTestFlagConsumesNextArg(arg string) bool { | |||||
| 154 | } | 155 | } | ||
| 155 | } | 156 | } | ||
| 156 | 157 | ||||
| 157 | - | func wrapStandaloneOmniboxNavigate(original func(string), quit func()) func(string) { | 158 | + | func wrapStandaloneOmniboxNavigate(original func(context.Context, string) error, quit func()) func(context.Context, string) error { |
| 158 | - | return func(url string) { | 159 | + | return func(ctx context.Context, url string) error { |
| 159 | - | if original != nil { | 160 | + | if original == nil { |
| 160 | - | original(url) | 161 | + | return fmt.Errorf("standalone omnibox navigate handler is not configured") |
| 162 | + | } | |||
| 163 | + | if err := original(ctx, url); err != nil { | |||
| 164 | + | return err | |||
| 161 | } | 165 | } | ||
| 162 | if quit != nil { | 166 | if quit != nil { | ||
| 163 | quit() | 167 | quit() | ||
| 164 | } | 168 | } | ||
| 169 | + | return nil | |||
| 165 | } | 170 | } | ||
| 166 | } | 171 | } | ||
| 167 | 172 | ||||
M internal/ui/standalone_omnibox_test.go +14 -8
| @@ -12,13 +12,16 @@ import ( | |||
| 12 | 12 | ||
| 13 | 13 | func TestWrapStandaloneOmniboxNavigate_CallsOriginalThenQuit(t *testing.T) { | |
| 14 | 14 | var calls []string | |
| 15 | - | wrapped := wrapStandaloneOmniboxNavigate(func(url string) { | |
| 15 | + | wrapped := wrapStandaloneOmniboxNavigate(func(_ context.Context, url string) error { | |
| 16 | 16 | calls = append(calls, "navigate:"+url) | |
| 17 | + | return nil | |
| 17 | 18 | }, func() { | |
| 18 | 19 | calls = append(calls, "quit") | |
| 19 | 20 | }) | |
| 20 | 21 | ||
| 21 | - | wrapped("https://example.com") | |
| 22 | + | if err := wrapped(context.Background(), "https://example.com"); err != nil { | |
| 23 | + | t.Fatalf("wrapped navigate returned error: %v", err) | |
| 24 | + | } | |
| 22 | 25 | ||
| 23 | 26 | if len(calls) != 2 { | |
| 24 | 27 | t.Fatalf("expected 2 calls, got %d: %#v", len(calls), calls) | |
| @@ -31,16 +34,19 @@ func TestWrapStandaloneOmniboxNavigate_CallsOriginalThenQuit(t *testing.T) { | |||
| 31 | 34 | } | |
| 32 | 35 | } | |
| 33 | 36 | ||
| 34 | - | func TestWrapStandaloneOmniboxNavigate_QuitsWithoutOriginalNavigate(t *testing.T) { | |
| 37 | + | func TestWrapStandaloneOmniboxNavigate_DoesNotQuitWhenNavigateFails(t *testing.T) { | |
| 35 | 38 | called := false | |
| 36 | - | wrapped := wrapStandaloneOmniboxNavigate(nil, func() { | |
| 39 | + | wrapped := wrapStandaloneOmniboxNavigate(func(context.Context, string) error { | |
| 40 | + | return context.DeadlineExceeded | |
| 41 | + | }, func() { | |
| 37 | 42 | called = true | |
| 38 | 43 | }) | |
| 39 | 44 | ||
| 40 | - | wrapped("https://example.com") | |
| 41 | - | ||
| 42 | - | if !called { | |
| 43 | - | t.Fatalf("expected quit to be called") | |
| 45 | + | if err := wrapped(context.Background(), "https://example.com"); err == nil { | |
| 46 | + | t.Fatal("expected wrapped navigate to return an error") | |
| 47 | + | } | |
| 48 | + | if called { | |
| 49 | + | t.Fatalf("expected quit to be skipped when navigate fails") | |
| 44 | 50 | } | |
| 45 | 51 | } | |
| 46 | 52 | ||
| @@ -12,13 +12,16 @@ import ( | |||||
| 12 | 12 | ||||
| 13 | func TestWrapStandaloneOmniboxNavigate_CallsOriginalThenQuit(t *testing.T) { | 13 | func TestWrapStandaloneOmniboxNavigate_CallsOriginalThenQuit(t *testing.T) { | ||
| 14 | var calls []string | 14 | var calls []string | ||
| 15 | - | wrapped := wrapStandaloneOmniboxNavigate(func(url string) { | 15 | + | wrapped := wrapStandaloneOmniboxNavigate(func(_ context.Context, url string) error { |
| 16 | calls = append(calls, "navigate:"+url) | 16 | calls = append(calls, "navigate:"+url) | ||
| 17 | + | return nil | |||
| 17 | }, func() { | 18 | }, func() { | ||
| 18 | calls = append(calls, "quit") | 19 | calls = append(calls, "quit") | ||
| 19 | }) | 20 | }) | ||
| 20 | 21 | ||||
| 21 | - | wrapped("https://example.com") | 22 | + | if err := wrapped(context.Background(), "https://example.com"); err != nil { |
| 23 | + | t.Fatalf("wrapped navigate returned error: %v", err) | |||
| 24 | + | } | |||
| 22 | 25 | ||||
| 23 | if len(calls) != 2 { | 26 | if len(calls) != 2 { | ||
| 24 | t.Fatalf("expected 2 calls, got %d: %#v", len(calls), calls) | 27 | t.Fatalf("expected 2 calls, got %d: %#v", len(calls), calls) | ||
| @@ -31,16 +34,19 @@ func TestWrapStandaloneOmniboxNavigate_CallsOriginalThenQuit(t *testing.T) { | |||||
| 31 | } | 34 | } | ||
| 32 | } | 35 | } | ||
| 33 | 36 | ||||
| 34 | - | func TestWrapStandaloneOmniboxNavigate_QuitsWithoutOriginalNavigate(t *testing.T) { | 37 | + | func TestWrapStandaloneOmniboxNavigate_DoesNotQuitWhenNavigateFails(t *testing.T) { |
| 35 | called := false | 38 | called := false | ||
| 36 | - | wrapped := wrapStandaloneOmniboxNavigate(nil, func() { | 39 | + | wrapped := wrapStandaloneOmniboxNavigate(func(context.Context, string) error { |
| 40 | + | return context.DeadlineExceeded | |||
| 41 | + | }, func() { | |||
| 37 | called = true | 42 | called = true | ||
| 38 | }) | 43 | }) | ||
| 39 | 44 | ||||
| 40 | - | wrapped("https://example.com") | 45 | + | if err := wrapped(context.Background(), "https://example.com"); err == nil { |
| 41 | - | 46 | + | t.Fatal("expected wrapped navigate to return an error") | |
| 42 | - | if !called { | 47 | + | } |
| 43 | - | t.Fatalf("expected quit to be called") | 48 | + | if called { |
| 49 | + | t.Fatalf("expected quit to be skipped when navigate fails") | |||
| 44 | } | 50 | } | ||
| 45 | } | 51 | } | ||
| 46 | 52 | ||||
M internal/ui/standalone_runtime_test.go +42 -6
| @@ -2,6 +2,7 @@ package ui | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "errors" | |
| 5 | 6 | "testing" | |
| 6 | 7 | ||
| 7 | 8 | urlutil "github.com/bnema/dumber/internal/domain/url" | |
| @@ -10,13 +11,19 @@ import ( | |||
| 10 | 11 | func TestHandleStandaloneOmniboxNavigation_UsesFreshBrowserWindowForHTTP(t *testing.T) { | |
| 11 | 12 | browserURL := "" | |
| 12 | 13 | externalURL := "" | |
| 13 | - | handleStandaloneOmniboxNavigation(&Dependencies{ | |
| 14 | - | LaunchBrowserURL: func(uri string) { browserURL = uri }, | |
| 14 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ | |
| 15 | + | LaunchBrowserURL: func(_ context.Context, uri string) error { | |
| 16 | + | browserURL = uri | |
| 17 | + | return nil | |
| 18 | + | }, | |
| 15 | 19 | LaunchExternalURL: func(uri string) { | |
| 16 | 20 | externalURL = uri | |
| 17 | 21 | }, | |
| 18 | 22 | }, context.Background(), "https://example.com") | |
| 19 | 23 | ||
| 24 | + | if err != nil { | |
| 25 | + | t.Fatalf("expected no error, got %v", err) | |
| 26 | + | } | |
| 20 | 27 | if browserURL != "https://example.com" { | |
| 21 | 28 | t.Fatalf("expected browser launcher to receive URL, got %q", browserURL) | |
| 22 | 29 | } | |
| @@ -28,13 +35,19 @@ func TestHandleStandaloneOmniboxNavigation_UsesFreshBrowserWindowForHTTP(t *test | |||
| 28 | 35 | func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForCustomScheme(t *testing.T) { | |
| 29 | 36 | browserURL := "" | |
| 30 | 37 | externalURL := "" | |
| 31 | - | handleStandaloneOmniboxNavigation(&Dependencies{ | |
| 32 | - | LaunchBrowserURL: func(uri string) { browserURL = uri }, | |
| 38 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ | |
| 39 | + | LaunchBrowserURL: func(_ context.Context, uri string) error { | |
| 40 | + | browserURL = uri | |
| 41 | + | return nil | |
| 42 | + | }, | |
| 33 | 43 | LaunchExternalURL: func(uri string) { | |
| 34 | 44 | externalURL = uri | |
| 35 | 45 | }, | |
| 36 | 46 | }, context.Background(), "vscode://file/tmp/demo") | |
| 37 | 47 | ||
| 48 | + | if err != nil { | |
| 49 | + | t.Fatalf("expected no error, got %v", err) | |
| 50 | + | } | |
| 38 | 51 | if externalURL != "vscode://file/tmp/demo" { | |
| 39 | 52 | t.Fatalf("expected external launcher to receive URL, got %q", externalURL) | |
| 40 | 53 | } | |
| @@ -46,13 +59,19 @@ func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForCustomScheme(t | |||
| 46 | 59 | func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForMailto(t *testing.T) { | |
| 47 | 60 | browserURL := "" | |
| 48 | 61 | externalURL := "" | |
| 49 | - | handleStandaloneOmniboxNavigation(&Dependencies{ | |
| 50 | - | LaunchBrowserURL: func(uri string) { browserURL = uri }, | |
| 62 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ | |
| 63 | + | LaunchBrowserURL: func(_ context.Context, uri string) error { | |
| 64 | + | browserURL = uri | |
| 65 | + | return nil | |
| 66 | + | }, | |
| 51 | 67 | LaunchExternalURL: func(uri string) { | |
| 52 | 68 | externalURL = uri | |
| 53 | 69 | }, | |
| 54 | 70 | }, context.Background(), "mailto:foo@example.com") | |
| 55 | 71 | ||
| 72 | + | if err != nil { | |
| 73 | + | t.Fatalf("expected no error, got %v", err) | |
| 74 | + | } | |
| 56 | 75 | if externalURL != "mailto:foo@example.com" { | |
| 57 | 76 | t.Fatalf("expected external launcher to receive URL, got %q", externalURL) | |
| 58 | 77 | } | |
| @@ -61,6 +80,23 @@ func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForMailto(t *test | |||
| 61 | 80 | } | |
| 62 | 81 | } | |
| 63 | 82 | ||
| 83 | + | func TestHandleStandaloneOmniboxNavigation_ReturnsBrowserLaunchError(t *testing.T) { | |
| 84 | + | wantErr := context.DeadlineExceeded | |
| 85 | + | ||
| 86 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ | |
| 87 | + | LaunchBrowserURL: func(context.Context, string) error { | |
| 88 | + | return wantErr | |
| 89 | + | }, | |
| 90 | + | }, context.Background(), "https://example.com") | |
| 91 | + | ||
| 92 | + | if err == nil { | |
| 93 | + | t.Fatal("expected browser launch error") | |
| 94 | + | } | |
| 95 | + | if !errors.Is(err, wantErr) { | |
| 96 | + | t.Fatalf("expected error %v, got %v", wantErr, err) | |
| 97 | + | } | |
| 98 | + | } | |
| 99 | + | ||
| 64 | 100 | func TestStandaloneExternalSchemeRules_HTTPIsNotExternal(t *testing.T) { | |
| 65 | 101 | if urlutil.IsExternalScheme("https://example.com") { | |
| 66 | 102 | t.Fatalf("expected http url to stay in dumber") | |
| @@ -2,6 +2,7 @@ package ui | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "errors" | |||
| 5 | "testing" | 6 | "testing" | ||
| 6 | 7 | ||||
| 7 | urlutil "github.com/bnema/dumber/internal/domain/url" | 8 | urlutil "github.com/bnema/dumber/internal/domain/url" | ||
| @@ -10,13 +11,19 @@ import ( | |||||
| 10 | func TestHandleStandaloneOmniboxNavigation_UsesFreshBrowserWindowForHTTP(t *testing.T) { | 11 | func TestHandleStandaloneOmniboxNavigation_UsesFreshBrowserWindowForHTTP(t *testing.T) { | ||
| 11 | browserURL := "" | 12 | browserURL := "" | ||
| 12 | externalURL := "" | 13 | externalURL := "" | ||
| 13 | - | handleStandaloneOmniboxNavigation(&Dependencies{ | 14 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ |
| 14 | - | LaunchBrowserURL: func(uri string) { browserURL = uri }, | 15 | + | LaunchBrowserURL: func(_ context.Context, uri string) error { |
| 16 | + | browserURL = uri | |||
| 17 | + | return nil | |||
| 18 | + | }, | |||
| 15 | LaunchExternalURL: func(uri string) { | 19 | LaunchExternalURL: func(uri string) { | ||
| 16 | externalURL = uri | 20 | externalURL = uri | ||
| 17 | }, | 21 | }, | ||
| 18 | }, context.Background(), "https://example.com") | 22 | }, context.Background(), "https://example.com") | ||
| 19 | 23 | ||||
| 24 | + | if err != nil { | |||
| 25 | + | t.Fatalf("expected no error, got %v", err) | |||
| 26 | + | } | |||
| 20 | if browserURL != "https://example.com" { | 27 | if browserURL != "https://example.com" { | ||
| 21 | t.Fatalf("expected browser launcher to receive URL, got %q", browserURL) | 28 | t.Fatalf("expected browser launcher to receive URL, got %q", browserURL) | ||
| 22 | } | 29 | } | ||
| @@ -28,13 +35,19 @@ func TestHandleStandaloneOmniboxNavigation_UsesFreshBrowserWindowForHTTP(t *test | |||||
| 28 | func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForCustomScheme(t *testing.T) { | 35 | func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForCustomScheme(t *testing.T) { | ||
| 29 | browserURL := "" | 36 | browserURL := "" | ||
| 30 | externalURL := "" | 37 | externalURL := "" | ||
| 31 | - | handleStandaloneOmniboxNavigation(&Dependencies{ | 38 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ |
| 32 | - | LaunchBrowserURL: func(uri string) { browserURL = uri }, | 39 | + | LaunchBrowserURL: func(_ context.Context, uri string) error { |
| 40 | + | browserURL = uri | |||
| 41 | + | return nil | |||
| 42 | + | }, | |||
| 33 | LaunchExternalURL: func(uri string) { | 43 | LaunchExternalURL: func(uri string) { | ||
| 34 | externalURL = uri | 44 | externalURL = uri | ||
| 35 | }, | 45 | }, | ||
| 36 | }, context.Background(), "vscode://file/tmp/demo") | 46 | }, context.Background(), "vscode://file/tmp/demo") | ||
| 37 | 47 | ||||
| 48 | + | if err != nil { | |||
| 49 | + | t.Fatalf("expected no error, got %v", err) | |||
| 50 | + | } | |||
| 38 | if externalURL != "vscode://file/tmp/demo" { | 51 | if externalURL != "vscode://file/tmp/demo" { | ||
| 39 | t.Fatalf("expected external launcher to receive URL, got %q", externalURL) | 52 | t.Fatalf("expected external launcher to receive URL, got %q", externalURL) | ||
| 40 | } | 53 | } | ||
| @@ -46,13 +59,19 @@ func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForCustomScheme(t | |||||
| 46 | func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForMailto(t *testing.T) { | 59 | func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForMailto(t *testing.T) { | ||
| 47 | browserURL := "" | 60 | browserURL := "" | ||
| 48 | externalURL := "" | 61 | externalURL := "" | ||
| 49 | - | handleStandaloneOmniboxNavigation(&Dependencies{ | 62 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ |
| 50 | - | LaunchBrowserURL: func(uri string) { browserURL = uri }, | 63 | + | LaunchBrowserURL: func(_ context.Context, uri string) error { |
| 64 | + | browserURL = uri | |||
| 65 | + | return nil | |||
| 66 | + | }, | |||
| 51 | LaunchExternalURL: func(uri string) { | 67 | LaunchExternalURL: func(uri string) { | ||
| 52 | externalURL = uri | 68 | externalURL = uri | ||
| 53 | }, | 69 | }, | ||
| 54 | }, context.Background(), "mailto:foo@example.com") | 70 | }, context.Background(), "mailto:foo@example.com") | ||
| 55 | 71 | ||||
| 72 | + | if err != nil { | |||
| 73 | + | t.Fatalf("expected no error, got %v", err) | |||
| 74 | + | } | |||
| 56 | if externalURL != "mailto:foo@example.com" { | 75 | if externalURL != "mailto:foo@example.com" { | ||
| 57 | t.Fatalf("expected external launcher to receive URL, got %q", externalURL) | 76 | t.Fatalf("expected external launcher to receive URL, got %q", externalURL) | ||
| 58 | } | 77 | } | ||
| @@ -61,6 +80,23 @@ func TestHandleStandaloneOmniboxNavigation_UsesExternalLauncherForMailto(t *test | |||||
| 61 | } | 80 | } | ||
| 62 | } | 81 | } | ||
| 63 | 82 | ||||
| 83 | + | func TestHandleStandaloneOmniboxNavigation_ReturnsBrowserLaunchError(t *testing.T) { | |||
| 84 | + | wantErr := context.DeadlineExceeded | |||
| 85 | + | ||||
| 86 | + | err := handleStandaloneOmniboxNavigation(&Dependencies{ | |||
| 87 | + | LaunchBrowserURL: func(context.Context, string) error { | |||
| 88 | + | return wantErr | |||
| 89 | + | }, | |||
| 90 | + | }, context.Background(), "https://example.com") | |||
| 91 | + | ||||
| 92 | + | if err == nil { | |||
| 93 | + | t.Fatal("expected browser launch error") | |||
| 94 | + | } | |||
| 95 | + | if !errors.Is(err, wantErr) { | |||
| 96 | + | t.Fatalf("expected error %v, got %v", wantErr, err) | |||
| 97 | + | } | |||
| 98 | + | } | |||
| 99 | + | ||||
| 64 | func TestStandaloneExternalSchemeRules_HTTPIsNotExternal(t *testing.T) { | 100 | func TestStandaloneExternalSchemeRules_HTTPIsNotExternal(t *testing.T) { | ||
| 65 | if urlutil.IsExternalScheme("https://example.com") { | 101 | if urlutil.IsExternalScheme("https://example.com") { | ||
| 66 | t.Fatalf("expected http url to stay in dumber") | 102 | t.Fatalf("expected http url to stay in dumber") | ||