fix: address PR review comments

- Fix AGENTS.md table padding (MD060) and add language tag (MD040)
- Fix IsExternalScheme to handle case-insensitive schemes (RFC 3986)
- Update GoBack docstring to reflect current WebKit behavior
- Add GoBack() to OnURIChanged external scheme handling with CanGoBack guard
- Log ActionHandler errors and only claim gesture when action ran
- Add comprehensive tests for IsExternalScheme function
- Add error logging to LaunchExternalURL
- Add conceptual note to AGENTS.md about structure

Resolves all review comments from coderabbitai, greptile-apps, and copilot-pull-request-reviewer

commit: 6c92e895140e58b16430bc6c06526e6f11937eb5
author: brice · 8mo ago · 2026-01-29 04:37 UTC
parent: 2157834
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

7 files · +146 -25

M AGENTS.md +3 -2


@@ -2,6 +2,8 @@
2	2
3	3		## Layers
4	4
	5	+	> Note: This document describes conceptual architecture layers. The actual project structure uses `internal/` prefixes (e.g., `internal/application/usecase/`, `internal/application/port/`, `internal/infrastructure/`) to organize the code.
	6	+
5	7		\| Layer \| Contains \| Depends On \|
6	8		\|-------\|----------\|------------\|
7	9		\| `app/` \| Entry points, CLI/TUI, request handling \| usecase \|

@@ -20,7 +22,7 @@
20	22
21	23		## Anti-patterns strictly forbidden
22	24
23		-	```
	25	+	```text
24	26		❌ app doing business logic
25	27		❌ app calling adapters directly
26	28		❌ usecase importing adapters

@@ -29,4 +31,3 @@
29	31
30	32		## Commands
31	33
32		-

 ## Layers
+> **Note:** This document describes conceptual architecture layers. The actual project structure uses `internal/` prefixes (e.g., `internal/application/usecase/`, `internal/application/port/`, `internal/infrastructure/`) to organize the code.
 | Layer | Contains | Depends On |
 |-------|----------|------------|
 | `app/` | Entry points, CLI/TUI, request handling | usecase |
 ## Anti-patterns strictly forbidden
-```
+```text
 ❌ app doing business logic
 ❌ app calling adapters directly
 ❌ usecase importing adapters
 ## Commands

M internal/domain/url/normalize.go +7 -15


@@ -185,29 +185,21 @@ func TrimLeadingSpacesIfURL(input string) string {
185	185		// IsExternalScheme returns true if the URL uses a scheme that should be
186	186		// launched externally (e.g., vscode://, vscode-insiders://, spotify://, steam://)
187	187		// rather than handled by the browser.
	188	+	// URL schemes are case-insensitive per RFC 3986.
188	189		func IsExternalScheme(uri string) bool {
189	190		if uri == "" {
190	191		return false
191	192		}
192	193
193		-	// Internal schemes we handle
194		-	if strings.HasPrefix(uri, "http://") \|\|
195		-	strings.HasPrefix(uri, "https://") \|\|
196		-	strings.HasPrefix(uri, "file://") \|\|
197		-	strings.HasPrefix(uri, "dumb://") \|\|
198		-	strings.HasPrefix(uri, "about:") \|\|
199		-	strings.HasPrefix(uri, "data:") \|\|
200		-	strings.HasPrefix(uri, "blob:") \|\|
201		-	strings.HasPrefix(uri, "javascript:") {
	194	+	parsed, err := url.Parse(uri)
	195	+	if err != nil \|\| parsed.Scheme == "" {
202	196		return false
203	197		}
204	198
205		-	// Check if it looks like a valid URL with a scheme
206		-	parsed, err := url.Parse(uri)
207		-	if err != nil {
	199	+	switch strings.ToLower(parsed.Scheme) {
	200	+	case "http", "https", "file", "dumb", "about", "data", "blob", "javascript":
208	201		return false
	202	+	default:
	203	+	return true
209	204		}
210		-
211		-	// Must have a non-empty scheme to be considered external
212		-	return parsed.Scheme != ""
213	205		}

 // IsExternalScheme returns true if the URL uses a scheme that should be
 // launched externally (e.g., vscode://, vscode-insiders://, spotify://, steam://)
 // rather than handled by the browser.
+// URL schemes are case-insensitive per RFC 3986.
 func IsExternalScheme(uri string) bool {
 	if uri == "" {
 		return false
 	}
-	// Internal schemes we handle
+	parsed, err := url.Parse(uri)
-	if strings.HasPrefix(uri, "http://") ||
+	if err != nil || parsed.Scheme == "" {
-		strings.HasPrefix(uri, "https://") ||
-		strings.HasPrefix(uri, "file://") ||
-		strings.HasPrefix(uri, "dumb://") ||
-		strings.HasPrefix(uri, "about:") ||
-		strings.HasPrefix(uri, "data:") ||
-		strings.HasPrefix(uri, "blob:") ||
-		strings.HasPrefix(uri, "javascript:") {
 		return false
 	}
-	// Check if it looks like a valid URL with a scheme
+	switch strings.ToLower(parsed.Scheme) {
-	parsed, err := url.Parse(uri)
+	case "http", "https", "file", "dumb", "about", "data", "blob", "javascript":
-	if err != nil {
 		return false
+	default:
+		return true
 	}
-	// Must have a non-empty scheme to be considered external
-	return parsed.Scheme != ""
 }

M internal/domain/url/normalize_test.go +108 -0


@@ -401,3 +401,111 @@ func TestLooksLikeURL(t *testing.T) {
401	401		})
402	402		}
403	403		}
	404	+
	405	+	func TestIsExternalScheme(t *testing.T) {
	406	+	tests := []struct {
	407	+	name string
	408	+	input string
	409	+	want bool
	410	+	}{
	411	+	{
	412	+	name: "empty string",
	413	+	input: "",
	414	+	want: false,
	415	+	},
	416	+	{
	417	+	name: "http scheme",
	418	+	input: "http://example.com",
	419	+	want: false,
	420	+	},
	421	+	{
	422	+	name: "https scheme",
	423	+	input: "https://example.com",
	424	+	want: false,
	425	+	},
	426	+	{
	427	+	name: "file scheme",
	428	+	input: "file:///path/to/file.html",
	429	+	want: false,
	430	+	},
	431	+	{
	432	+	name: "dumb scheme",
	433	+	input: "dumb://home",
	434	+	want: false,
	435	+	},
	436	+	{
	437	+	name: "about scheme",
	438	+	input: "about:blank",
	439	+	want: false,
	440	+	},
	441	+	{
	442	+	name: "data scheme",
	443	+	input: "data:text/plain;base64,SGVsbG8gV29ybGQ==",
	444	+	want: false,
	445	+	},
	446	+	{
	447	+	name: "blob scheme",
	448	+	input: "blob:https://example.com/blob-id",
	449	+	want: false,
	450	+	},
	451	+	{
	452	+	name: "javascript scheme",
	453	+	input: "javascript:alert('hello')",
	454	+	want: false,
	455	+	},
	456	+	{
	457	+	name: "vscode scheme",
	458	+	input: "vscode://open",
	459	+	want: true,
	460	+	},
	461	+	{
	462	+	name: "spotify scheme",
	463	+	input: "spotify:track:123",
	464	+	want: true,
	465	+	},
	466	+	{
	467	+	name: "steam scheme",
	468	+	input: "steam://run/123",
	469	+	want: true,
	470	+	},
	471	+	{
	472	+	name: "uppercase HTTP scheme",
	473	+	input: "HTTP://example.com",
	474	+	want: false,
	475	+	},
	476	+	{
	477	+	name: "uppercase ABOUT scheme",
	478	+	input: "ABOUT:blank",
	479	+	want: false,
	480	+	},
	481	+	{
	482	+	name: "uppercase VSCODE scheme",
	483	+	input: "VSCODE://open",
	484	+	want: true,
	485	+	},
	486	+	{
	487	+	name: "malformed URL",
	488	+	input: "://invalid",
	489	+	want: false,
	490	+	},
	491	+	{
	492	+	name: "no scheme",
	493	+	input: "example.com",
	494	+	want: false,
	495	+	},
	496	+	{
	497	+	name: "scheme only",
	498	+	input: "custom://",
	499	+	want: true,
	500	+	},
	501	+	}
	502	+
	503	+	for _, tt := range tests {
	504	+	t.Run(tt.name, func(t *testing.T) {
	505	+	got := IsExternalScheme(tt.input)
	506	+	if got != tt.want {
	507	+	t.Errorf("IsExternalScheme(%q) = %v, want %v", tt.input, got, tt.want)
	508	+	}
	509	+	})
	510	+	}
	511	+	}


@@ -401,3 +401,111 @@ func TestLooksLikeURL(t *testing.T) {
401	})	401		})
402	}	402		}
403	}	403		}
		404	+
		405	+	func TestIsExternalScheme(t *testing.T) {
		406	+	tests := []struct {
		407	+	name string
		408	+	input string
		409	+	want bool
		410	+	}{
		411	+	{
		412	+	name: "empty string",
		413	+	input: "",
		414	+	want: false,
		415	+	},
		416	+	{
		417	+	name: "http scheme",
		418	+	input: "http://example.com",
		419	+	want: false,
		420	+	},
		421	+	{
		422	+	name: "https scheme",
		423	+	input: "https://example.com",
		424	+	want: false,
		425	+	},
		426	+	{
		427	+	name: "file scheme",
		428	+	input: "file:///path/to/file.html",
		429	+	want: false,
		430	+	},
		431	+	{
		432	+	name: "dumb scheme",
		433	+	input: "dumb://home",
		434	+	want: false,
		435	+	},
		436	+	{
		437	+	name: "about scheme",
		438	+	input: "about:blank",
		439	+	want: false,
		440	+	},
		441	+	{
		442	+	name: "data scheme",
		443	+	input: "data:text/plain;base64,SGVsbG8gV29ybGQ==",
		444	+	want: false,
		445	+	},
		446	+	{
		447	+	name: "blob scheme",
		448	+	input: "blob:https://example.com/blob-id",
		449	+	want: false,
		450	+	},
		451	+	{
		452	+	name: "javascript scheme",
		453	+	input: "javascript:alert('hello')",
		454	+	want: false,
		455	+	},
		456	+	{
		457	+	name: "vscode scheme",
		458	+	input: "vscode://open",
		459	+	want: true,
		460	+	},
		461	+	{
		462	+	name: "spotify scheme",
		463	+	input: "spotify:track:123",
		464	+	want: true,
		465	+	},
		466	+	{
		467	+	name: "steam scheme",
		468	+	input: "steam://run/123",
		469	+	want: true,
		470	+	},
		471	+	{
		472	+	name: "uppercase HTTP scheme",
		473	+	input: "HTTP://example.com",
		474	+	want: false,
		475	+	},
		476	+	{
		477	+	name: "uppercase ABOUT scheme",
		478	+	input: "ABOUT:blank",
		479	+	want: false,
		480	+	},
		481	+	{
		482	+	name: "uppercase VSCODE scheme",
		483	+	input: "VSCODE://open",
		484	+	want: true,
		485	+	},
		486	+	{
		487	+	name: "malformed URL",
		488	+	input: "://invalid",
		489	+	want: false,
		490	+	},
		491	+	{
		492	+	name: "no scheme",
		493	+	input: "example.com",
		494	+	want: false,
		495	+	},
		496	+	{
		497	+	name: "scheme only",
		498	+	input: "custom://",
		499	+	want: true,
		500	+	},
		501	+	}
		502	+
		503	+	for _, tt := range tests {
		504	+	t.Run(tt.name, func(t *testing.T) {
		505	+	got := IsExternalScheme(tt.input)
		506	+	if got != tt.want {
		507	+	t.Errorf("IsExternalScheme(%q) = %v, want %v", tt.input, got, tt.want)
		508	+	}
		509	+	})
		510	+	}
		511	+	}

M internal/infrastructure/desktop/adapter.go +7 -2


@@ -400,9 +400,14 @@ func LaunchExternalURL(uri string) {
400	400		cmd.Stdin = nil
401	401		cmd.Stdout = nil
402	402		cmd.Stderr = nil
403		-	_ = cmd.Start()
	403	+	if err := cmd.Start(); err != nil {
	404	+	fmt.Fprintf(os.Stderr, "failed to launch external URL %q with xdg-open: %v\n", uri, err)
	405	+	return
	406	+	}
404	407		if cmd.Process != nil {
405		-	_ = cmd.Process.Release()
	408	+	if err := cmd.Process.Release(); err != nil {
	409	+	fmt.Fprintf(os.Stderr, "failed to release xdg-open process for URL %q: %v\n", uri, err)
	410	+	}
406	411		}
407	412		}
408	413

 	cmd.Stdin = nil
 	cmd.Stdout = nil
 	cmd.Stderr = nil
-	_ = cmd.Start()
+	if err := cmd.Start(); err != nil {
+		fmt.Fprintf(os.Stderr, "failed to launch external URL %q with xdg-open: %v\n", uri, err)
+		return
+	}
 	if cmd.Process != nil {
-		_ = cmd.Process.Release()
+		if err := cmd.Process.Release(); err != nil {
+			fmt.Fprintf(os.Stderr, "failed to release xdg-open process for URL %q: %v\n", uri, err)
+		}
 	}
 }

M internal/infrastructure/webkit/webview.go +1 -3


@@ -945,9 +945,7 @@ func (wv *WebView) Stop(ctx context.Context) error {
945	945		}
946	946
947	947		// GoBack navigates back in history.
948		-	// After calling WebKit's GoBack, dispatches a popstate event via JS to ensure
949		-	// SPAs that use history.pushState receive the navigation notification.
950		-	// WebKit's programmatic go_back() doesn't fire popstate for pushState entries.
	948	+	// Uses WebKit's native history navigation.
951	949		func (wv *WebView) GoBack(ctx context.Context) error {
952	950		if wv.destroyed.Load() {
953	951		return fmt.Errorf("webview %d is destroyed", wv.id)

 }
 // GoBack navigates back in history.
-// After calling WebKit's GoBack, dispatches a popstate event via JS to ensure
+// Uses WebKit's native history navigation.
-// SPAs that use history.pushState receive the navigation notification.
-// WebKit's programmatic go_back() doesn't fire popstate for pushState entries.
 func (wv *WebView) GoBack(ctx context.Context) error {
 	if wv.destroyed.Load() {
 		return fmt.Errorf("webview %d is destroyed", wv.id)

M internal/ui/coordinator/content.go +5 -0


@@ -1477,6 +1477,11 @@ func (c *ContentCoordinator) setupWebViewCallbacks(ctx context.Context, paneID e
1477	1477		// Stop loading to prevent WebKit from showing an error page
1478	1478		// The page stays on the previous URL before the JS redirect
1479	1479		_ = wv.Stop(ctx)
	1480	+
	1481	+	// Navigate back to avoid stale URI in omnibox/history
	1482	+	if wv.CanGoBack() {
	1483	+	_ = wv.GoBack(ctx)
	1484	+	}
1480	1485		return
1481	1486		}
1482	1487


@@ -1477,6 +1477,11 @@ func (c *ContentCoordinator) setupWebViewCallbacks(ctx context.Context, paneID e
1477	// Stop loading to prevent WebKit from showing an error page	1477		// Stop loading to prevent WebKit from showing an error page
1478	// The page stays on the previous URL before the JS redirect	1478		// The page stays on the previous URL before the JS redirect
1479	_ = wv.Stop(ctx)	1479		_ = wv.Stop(ctx)
		1480	+
		1481	+	// Navigate back to avoid stale URI in omnibox/history
		1482	+	if wv.CanGoBack() {
		1483	+	_ = wv.GoBack(ctx)
		1484	+	}
1480	return	1485		return
1481	}	1486		}
1482		1487

M internal/ui/input/gesture.go +15 -3


@@ -122,20 +122,30 @@ func (h *GestureHandler) handlePressed(nPress int) {
122	122		handler := h.onAction
123	123		h.mu.RUnlock()
124	124
	125	+	handled := false
	126	+
125	127		switch button {
126	128		case mouseButtonBack:
127	129		if nav != nil {
128	130		log.Debug().Uint("button", button).Msg("gesture: direct go back")
129	131		nav.GoBackDirect()
	132	+	handled = true
130	133		} else if handler != nil {
131		-	_ = handler(h.ctx, ActionGoBack)
	134	+	if err := handler(h.ctx, ActionGoBack); err != nil {
	135	+	log.Warn().Err(err).Msg("gesture action failed")
	136	+	}
	137	+	handled = true
132	138		}
133	139		case mouseButtonForward:
134	140		if nav != nil {
135	141		log.Debug().Uint("button", button).Msg("gesture: direct go forward")
136	142		nav.GoForwardDirect()
	143	+	handled = true
137	144		} else if handler != nil {
138		-	_ = handler(h.ctx, ActionGoForward)
	145	+	if err := handler(h.ctx, ActionGoForward); err != nil {
	146	+	log.Warn().Err(err).Msg("gesture action failed")
	147	+	}
	148	+	handled = true
139	149		}
140	150		default:
141	151		// Not a navigation button, ignore

@@ -145,7 +155,9 @@ func (h *GestureHandler) handlePressed(nPress int) {
145	155		// Claim the gesture sequence to stop event propagation.
146	156		// This matches Epiphany's behavior: gtk_gesture_set_state(gesture, GTK_EVENT_SEQUENCE_CLAIMED)
147	157		// Without this, GTK may continue propagating the event which can interfere with WebKit.
148		-	h.clickGesture.SetState(gtkEventSequenceClaimed)
	158	+	if handled {
	159	+	h.clickGesture.SetState(gtkEventSequenceClaimed)
	160	+	}
149	161		}
150	162
151	163		// Detach removes the gesture handler.

 	handler := h.onAction
 	h.mu.RUnlock()
+	handled := false
 	switch button {
 	case mouseButtonBack:
 		if nav != nil {
 			log.Debug().Uint("button", button).Msg("gesture: direct go back")
 			nav.GoBackDirect()
+			handled = true
 		} else if handler != nil {
-			_ = handler(h.ctx, ActionGoBack)
+			if err := handler(h.ctx, ActionGoBack); err != nil {
+				log.Warn().Err(err).Msg("gesture action failed")
+			}
+			handled = true
 		}
 	case mouseButtonForward:
 		if nav != nil {
 			log.Debug().Uint("button", button).Msg("gesture: direct go forward")
 			nav.GoForwardDirect()
+			handled = true
 		} else if handler != nil {
-			_ = handler(h.ctx, ActionGoForward)
+			if err := handler(h.ctx, ActionGoForward); err != nil {
+				log.Warn().Err(err).Msg("gesture action failed")
+			}
+			handled = true
 		}
 	default:
 		// Not a navigation button, ignore
 	// Claim the gesture sequence to stop event propagation.
 	// This matches Epiphany's behavior: gtk_gesture_set_state(gesture, GTK_EVENT_SEQUENCE_CLAIMED)
 	// Without this, GTK may continue propagating the event which can interfere with WebKit.
-	h.clickGesture.SetState(gtkEventSequenceClaimed)
+	if handled {
+		h.clickGesture.SetState(gtkEventSequenceClaimed)
+	}
 }
 // Detach removes the gesture handler.