fix(review): address teardown and follow-up feedback

commit: d0331c044eab609c4af27f1f3ad5a1e430ed053d
author: brice · 4mo ago · 2026-06-11 07:46 UTC
parent: 0337582
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

5 files · +91 -3

M Makefile +2 -0


@@ -158,6 +158,8 @@ fmt: ## Format Go code with gofmt
158	158		install-golangci-lint: ## Install pinned golangci-lint
159	159		@echo "Installing golangci-lint $(GOLANGCI_LINT_VERSION) to $(TOOL_BIN_DIR)..."
160	160		@mkdir -p $(TOOL_BIN_DIR)
	161	+	# Accepted trade-off: use the upstream installer entrypoint, but keep the
	162	+	# requested golangci-lint version pinned in this repo for reproducible tool use.
161	163		curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh \| sh -s -- -b $(TOOL_BIN_DIR) $(GOLANGCI_LINT_VERSION)
162	164
163	165		install-staticcheck: ## Install pinned Staticcheck with the active Go toolchain


@@ -158,6 +158,8 @@ fmt: ## Format Go code with gofmt
158	install-golangci-lint: ## Install pinned golangci-lint	158		install-golangci-lint: ## Install pinned golangci-lint
159	@echo "Installing golangci-lint $(GOLANGCI_LINT_VERSION) to $(TOOL_BIN_DIR)..."	159		@echo "Installing golangci-lint $(GOLANGCI_LINT_VERSION) to $(TOOL_BIN_DIR)..."
160	@mkdir -p $(TOOL_BIN_DIR)	160		@mkdir -p $(TOOL_BIN_DIR)
		161	+	# Accepted trade-off: use the upstream installer entrypoint, but keep the
		162	+	# requested golangci-lint version pinned in this repo for reproducible tool use.
161	curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh \| sh -s -- -b $(TOOL_BIN_DIR) $(GOLANGCI_LINT_VERSION)	163		curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh \| sh -s -- -b $(TOOL_BIN_DIR) $(GOLANGCI_LINT_VERSION)
162		164
163	install-staticcheck: ## Install pinned Staticcheck with the active Go toolchain	165		install-staticcheck: ## Install pinned Staticcheck with the active Go toolchain

M internal/application/usecase/search_history.go +9 -0


@@ -307,6 +307,10 @@ func (uc *SearchHistoryUseCase) GetDomainStats(ctx context.Context, limit int) (
307	307		return uc.historyRepo.GetDomainStats(ctx, limit)
308	308		}
309	309
	310	+	// clampOptionalLimit normalizes an optional limit where zero remains a valid
	311	+	// "no limit" value, negatives fall back to defaultLimit, and values above
	312	+	// maxLimit are capped. Use clampPositiveLimit instead when zero should be
	313	+	// treated as invalid input.
310	314		func clampOptionalLimit(limit, defaultLimit, maxLimit int) int {
311	315		if limit < 0 {
312	316		return defaultLimit

@@ -317,6 +321,11 @@ func clampOptionalLimit(limit, defaultLimit, maxLimit int) int {
317	321		return limit
318	322		}
319	323
	324	+	// clampPositiveLimit normalizes a required positive limit. Values less than or
	325	+	// equal to zero fall back to defaultLimit, values above maxLimit are capped,
	326	+	// and the result stays within the expected positive range. Use this for
	327	+	// required limits like domain stats, and clampOptionalLimit when zero has its
	328	+	// own meaning.
320	329		func clampPositiveLimit(limit, defaultLimit, maxLimit int) int {
321	330		if limit <= 0 {
322	331		return defaultLimit


@@ -307,6 +307,10 @@ func (uc *SearchHistoryUseCase) GetDomainStats(ctx context.Context, limit int) (
307	return uc.historyRepo.GetDomainStats(ctx, limit)	307		return uc.historyRepo.GetDomainStats(ctx, limit)
308	}	308		}
309		309
		310	+	// clampOptionalLimit normalizes an optional limit where zero remains a valid
		311	+	// "no limit" value, negatives fall back to defaultLimit, and values above
		312	+	// maxLimit are capped. Use clampPositiveLimit instead when zero should be
		313	+	// treated as invalid input.
310	func clampOptionalLimit(limit, defaultLimit, maxLimit int) int {	314		func clampOptionalLimit(limit, defaultLimit, maxLimit int) int {
311	if limit < 0 {	315		if limit < 0 {
312	return defaultLimit	316		return defaultLimit

@@ -317,6 +321,11 @@ func clampOptionalLimit(limit, defaultLimit, maxLimit int) int {
317	return limit	321		return limit
318	}	322		}
319		323
		324	+	// clampPositiveLimit normalizes a required positive limit. Values less than or
		325	+	// equal to zero fall back to defaultLimit, values above maxLimit are capped,
		326	+	// and the result stays within the expected positive range. Use this for
		327	+	// required limits like domain stats, and clampOptionalLimit when zero has its
		328	+	// own meaning.
320	func clampPositiveLimit(limit, defaultLimit, maxLimit int) int {	329		func clampPositiveLimit(limit, defaultLimit, maxLimit int) int {
321	if limit <= 0 {	330		if limit <= 0 {
322	return defaultLimit	331		return defaultLimit

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


@@ -2808,7 +2808,11 @@ func (a App) cleanupCreatedBrowserWindows(createdWindows []browserWindow) {
2808	2808		continue
2809	2809		}
2810	2810		mainWindow := bw.mainWindow
2811		-	a.removeBrowserWindow(bw.id)
	2811	+	if _, tracked := a.browserWindows[bw.id]; tracked {
	2812	+	a.removeBrowserWindow(bw.id)
	2813	+	} else {
	2814	+	a.cleanupTransientBrowserWindowForDestroy(bw)
	2815	+	}
2812	2816		if mainWindow != nil {
2813	2817		if a.mainWindow == mainWindow {
2814	2818		a.mainWindow = nil

 			continue
 		}
 		mainWindow := bw.mainWindow
-		a.removeBrowserWindow(bw.id)
+		if _, tracked := a.browserWindows[bw.id]; tracked {
+			a.removeBrowserWindow(bw.id)
+		} else {
+			a.cleanupTransientBrowserWindowForDestroy(bw)
+		}
 		if mainWindow != nil {
 			if a.mainWindow == mainWindow {
 				a.mainWindow = nil

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


@@ -51,6 +51,14 @@ func (bw *browserWindow) detachInputForDestroy() {
51	51		}
52	52		}
53	53
	54	+	func (bw *browserWindow) teardownForDestroy() {
	55	+	if bw == nil {
	56	+	return
	57	+	}
	58	+	bw.detachInputForDestroy()
	59	+	bw.clearShellState()
	60	+	}
	61	+
54	62		func (bw *browserWindow) clearShellState() {
55	63		if bw == nil {
56	64		return

@@ -299,8 +307,7 @@ func (a *App) removeBrowserWindow(id string) {
299	307		}
300	308		}
301	309		if removed != nil {
302		-	removed.detachInputForDestroy()
303		-	removed.clearShellState()
	310	+	removed.teardownForDestroy()
304	311		}
305	312		if a.contentCoord != nil {
306	313		a.contentCoord.ClearPopupNamedContextsForWindow(id)

@@ -330,6 +337,38 @@ func (a *App) removeBrowserWindow(id string) {
330	337		}
331	338		}
332	339
	340	+	func (a App) cleanupTransientBrowserWindowForDestroy(bw browserWindow) {
	341	+	if bw == nil {
	342	+	return
	343	+	}
	344	+	bw.teardownForDestroy()
	345	+	if a.contentCoord != nil {
	346	+	a.contentCoord.ClearPopupNamedContextsForWindow(bw.id)
	347	+	}
	348	+	fallback := a.deterministicBrowserWindowFallback()
	349	+	if a.lastFocusedWindowID == bw.id {
	350	+	if fallback != nil {
	351	+	a.lastFocusedWindowID = fallback.id
	352	+	} else {
	353	+	a.lastFocusedWindowID = ""
	354	+	}
	355	+	}
	356	+	if bw.mainWindow == a.mainWindow {
	357	+	a.clearResizeModeBorder()
	358	+	if fallback != nil {
	359	+	a.activateBrowserWindow(fallback)
	360	+	return
	361	+	}
	362	+	a.lastFocusedWindowID = ""
	363	+	a.mainWindow = nil
	364	+	a.keyboardHandler = nil
	365	+	a.globalShortcutHandler = nil
	366	+	if a.tabCoord != nil {
	367	+	a.tabCoord.SetCurrentTarget(coordinator.TabTarget{})
	368	+	}
	369	+	}
	370	+	}
	371	+
333	372		func (a App) deterministicBrowserWindowFallback() browserWindow {
334	373		if a.browserWindows == nil {
335	374		return nil


@@ -51,6 +51,14 @@ func (bw *browserWindow) detachInputForDestroy() {
51		}	51		}
52		}	52		}
53			53
			54	+	func (bw *browserWindow) teardownForDestroy() {
			55	+	if bw == nil {
			56	+	return
			57	+	}
			58	+	bw.detachInputForDestroy()
			59	+	bw.clearShellState()
			60	+	}
			61	+
54		func (bw *browserWindow) clearShellState() {	62		func (bw *browserWindow) clearShellState() {
55		if bw == nil {	63		if bw == nil {
56		return	64		return

@@ -299,8 +307,7 @@ func (a *App) removeBrowserWindow(id string) {
299		}	307		}
300		}	308		}
301		if removed != nil {	309		if removed != nil {
302	-	removed.detachInputForDestroy()	310	+	removed.teardownForDestroy()
303	-	removed.clearShellState()
304		}	311		}
305		if a.contentCoord != nil {	312		if a.contentCoord != nil {
306		a.contentCoord.ClearPopupNamedContextsForWindow(id)	313		a.contentCoord.ClearPopupNamedContextsForWindow(id)

@@ -330,6 +337,38 @@ func (a *App) removeBrowserWindow(id string) {
330		}	337		}
331		}	338		}
332			339
			340	+	func (a App) cleanupTransientBrowserWindowForDestroy(bw browserWindow) {
			341	+	if bw == nil {
			342	+	return
			343	+	}
			344	+	bw.teardownForDestroy()
			345	+	if a.contentCoord != nil {
			346	+	a.contentCoord.ClearPopupNamedContextsForWindow(bw.id)
			347	+	}
			348	+	fallback := a.deterministicBrowserWindowFallback()
			349	+	if a.lastFocusedWindowID == bw.id {
			350	+	if fallback != nil {
			351	+	a.lastFocusedWindowID = fallback.id
			352	+	} else {
			353	+	a.lastFocusedWindowID = ""
			354	+	}
			355	+	}
			356	+	if bw.mainWindow == a.mainWindow {
			357	+	a.clearResizeModeBorder()
			358	+	if fallback != nil {
			359	+	a.activateBrowserWindow(fallback)
			360	+	return
			361	+	}
			362	+	a.lastFocusedWindowID = ""
			363	+	a.mainWindow = nil
			364	+	a.keyboardHandler = nil
			365	+	a.globalShortcutHandler = nil
			366	+	if a.tabCoord != nil {
			367	+	a.tabCoord.SetCurrentTarget(coordinator.TabTarget{})
			368	+	}
			369	+	}
			370	+	}
			371	+
333		func (a App) deterministicBrowserWindowFallback() browserWindow {	372		func (a App) deterministicBrowserWindowFallback() browserWindow {
334		if a.browserWindows == nil {	373		if a.browserWindows == nil {
335		return nil	374		return nil

M internal/ui/browser_window_test.go +34 -0


@@ -67,6 +67,40 @@ func TestBrowserWindow_RemoveBrowserWindowClearsShellState(t *testing.T) {
67	67		}
68	68		}
69	69
	70	+	func TestApp_CleanupCreatedBrowserWindowsDetachesUnregisteredWindowBeforeDestroy(t *testing.T) {
	71	+	transientMainWindow := &window.MainWindow{}
	72	+	fallbackMainWindow := &window.MainWindow{}
	73	+	transient := &browserWindow{id: "window-transient", mainWindow: transientMainWindow}
	74	+	fallback := &browserWindow{id: "window-fallback", mainWindow: fallbackMainWindow}
	75	+	app := &App{
	76	+	mainWindow: transientMainWindow,
	77	+	browserWindows: map[string]*browserWindow{fallback.id: fallback},
	78	+	browserWindowOrder: []string{fallback.id},
	79	+	lastFocusedWindowID: transient.id,
	80	+	}
	81	+
	82	+	setShellField(t, transient, "keyboardHandler", &input.KeyboardHandler{})
	83	+	setShellField(t, transient, "globalShortcutHandler", &input.GlobalShortcutHandler{})
	84	+	setShellField(t, transient, "accentPicker", &component.AccentPicker{})
	85	+
	86	+	app.cleanupCreatedBrowserWindows([]*browserWindow{transient})
	87	+
	88	+	for _, name := range []string{"keyboardHandler", "globalShortcutHandler", "accentPicker"} {
	89	+	if !fieldIsZero(t, transient, name) {
	90	+	t.Fatalf("transient browserWindow.%s was not cleared", name)
	91	+	}
	92	+	}
	93	+	if app.mainWindow != fallbackMainWindow {
	94	+	t.Fatalf("mainWindow = %p, want fallback main window %p", app.mainWindow, fallbackMainWindow)
	95	+	}
	96	+	if app.lastFocusedWindowID != fallback.id {
	97	+	t.Fatalf("lastFocusedWindowID = %q, want %q", app.lastFocusedWindowID, fallback.id)
	98	+	}
	99	+	if got := app.browserWindows[fallback.id]; got != fallback {
	100	+	t.Fatalf("fallback browser window changed during transient cleanup: got %p want %p", got, fallback)
	101	+	}
	102	+	}
	103	+
70	104		func TestBrowserWindow_RemoveBrowserWindowCleansOwnedTabState(t *testing.T) {
71	105		ownedTab := entity.NewTab(entity.TabID("tab-owned"), entity.WorkspaceID("workspace-owned"), entity.NewPane(entity.PaneID("pane-owned")))
72	106		otherTab := entity.NewTab(entity.TabID("tab-other"), entity.WorkspaceID("workspace-other"), entity.NewPane(entity.PaneID("pane-other")))


@@ -67,6 +67,40 @@ func TestBrowserWindow_RemoveBrowserWindowClearsShellState(t *testing.T) {
67	}	67		}
68	}	68		}
69		69
		70	+	func TestApp_CleanupCreatedBrowserWindowsDetachesUnregisteredWindowBeforeDestroy(t *testing.T) {
		71	+	transientMainWindow := &window.MainWindow{}
		72	+	fallbackMainWindow := &window.MainWindow{}
		73	+	transient := &browserWindow{id: "window-transient", mainWindow: transientMainWindow}
		74	+	fallback := &browserWindow{id: "window-fallback", mainWindow: fallbackMainWindow}
		75	+	app := &App{
		76	+	mainWindow: transientMainWindow,
		77	+	browserWindows: map[string]*browserWindow{fallback.id: fallback},
		78	+	browserWindowOrder: []string{fallback.id},
		79	+	lastFocusedWindowID: transient.id,
		80	+	}
		81	+
		82	+	setShellField(t, transient, "keyboardHandler", &input.KeyboardHandler{})
		83	+	setShellField(t, transient, "globalShortcutHandler", &input.GlobalShortcutHandler{})
		84	+	setShellField(t, transient, "accentPicker", &component.AccentPicker{})
		85	+
		86	+	app.cleanupCreatedBrowserWindows([]*browserWindow{transient})
		87	+
		88	+	for _, name := range []string{"keyboardHandler", "globalShortcutHandler", "accentPicker"} {
		89	+	if !fieldIsZero(t, transient, name) {
		90	+	t.Fatalf("transient browserWindow.%s was not cleared", name)
		91	+	}
		92	+	}
		93	+	if app.mainWindow != fallbackMainWindow {
		94	+	t.Fatalf("mainWindow = %p, want fallback main window %p", app.mainWindow, fallbackMainWindow)
		95	+	}
		96	+	if app.lastFocusedWindowID != fallback.id {
		97	+	t.Fatalf("lastFocusedWindowID = %q, want %q", app.lastFocusedWindowID, fallback.id)
		98	+	}
		99	+	if got := app.browserWindows[fallback.id]; got != fallback {
		100	+	t.Fatalf("fallback browser window changed during transient cleanup: got %p want %p", got, fallback)
		101	+	}
		102	+	}
		103	+
70	func TestBrowserWindow_RemoveBrowserWindowCleansOwnedTabState(t *testing.T) {	104		func TestBrowserWindow_RemoveBrowserWindowCleansOwnedTabState(t *testing.T) {
71	ownedTab := entity.NewTab(entity.TabID("tab-owned"), entity.WorkspaceID("workspace-owned"), entity.NewPane(entity.PaneID("pane-owned")))	105		ownedTab := entity.NewTab(entity.TabID("tab-owned"), entity.WorkspaceID("workspace-owned"), entity.NewPane(entity.PaneID("pane-owned")))
72	otherTab := entity.NewTab(entity.TabID("tab-other"), entity.WorkspaceID("workspace-other"), entity.NewPane(entity.PaneID("pane-other")))	106		otherTab := entity.NewTab(entity.TabID("tab-other"), entity.WorkspaceID("workspace-other"), entity.NewPane(entity.PaneID("pane-other")))