0acad67 fix(history): address final review feedback · dumber

M internal/ui/browser_window_history_sidebar_test.go +8 -3


@@ -94,7 +94,13 @@ func TestHistorySidebarConfig_OnNavigateKeepOpenNavigatesWithoutClosing(t *testi
94	94		bwTabs.Add(tab)
95	95		bwTabs.SetActive(tab.ID)
96	96
97		-	bw := &browserWindow{id: "window-1", tabs: bwTabs}
	97	+	bw := &browserWindow{
	98	+	id: "window-1",
	99	+	tabs: bwTabs,
	100	+	mainWindow: &window.MainWindow{},
	101	+	historySidebar: &component.HistorySidebar{},
	102	+	sidebarVisible: true,
	103	+	}
98	104
99	105		contentCoord := contentcoord.NewCoordinator(ctx, nil, nil, nil, nil, nil, nil, nil)
100	106		fakeWv := &fakeRecordingWebView{id: 1}

@@ -118,6 +124,7 @@ func TestHistorySidebarConfig_OnNavigateKeepOpenNavigatesWithoutClosing(t *testi
118	124		require.NoError(t, err)
119	125		assert.True(t, fakeWv.loadURICalled)
120	126		assert.Equal(t, navigateURL, fakeWv.loadURILastURI, "Ctrl+Enter navigation should go to the URL")
	127	+	assert.True(t, bw.sidebarVisible, "Ctrl+Enter navigation should keep the sidebar visible")
121	128		}
122	129
123	130		// TestHistorySidebar_OwnershipOnMultiWindowNavigation verifies that when

@@ -844,8 +851,6 @@ func TestApp_HideAndRestoreFocusForBrowserWindow_HidesSidebar(t *testing.T) {
844	851
845	852		// Sidebar must be hidden.
846	853		assert.False(t, bw.sidebarVisible, "sidebar must be hidden")
847		-
848		-	_ = wsView
849	854		}
850	855
851	856		// TestApp_HideAndRestoreFocusForBrowserWindow_NilBWIsSafe verifies that

 	bwTabs.Add(tab)
 	bwTabs.SetActive(tab.ID)
-	bw := &browserWindow{id: "window-1", tabs: bwTabs}
+	bw := &browserWindow{
+		id:             "window-1",
+		tabs:           bwTabs,
+		mainWindow:     &window.MainWindow{},
+		historySidebar: &component.HistorySidebar{},
+		sidebarVisible: true,
+	}
 	contentCoord := contentcoord.NewCoordinator(ctx, nil, nil, nil, nil, nil, nil, nil)
 	fakeWv := &fakeRecordingWebView{id: 1}
 	require.NoError(t, err)
 	assert.True(t, fakeWv.loadURICalled)
 	assert.Equal(t, navigateURL, fakeWv.loadURILastURI, "Ctrl+Enter navigation should go to the URL")
+	assert.True(t, bw.sidebarVisible, "Ctrl+Enter navigation should keep the sidebar visible")
 }
 // TestHistorySidebar_OwnershipOnMultiWindowNavigation verifies that when
 	// Sidebar must be hidden.
 	assert.False(t, bw.sidebarVisible, "sidebar must be hidden")
-	_ = wsView
 }
 // TestApp_HideAndRestoreFocusForBrowserWindow_NilBWIsSafe verifies that

M internal/ui/component/history_sidebar.go +2 -2


@@ -255,7 +255,7 @@ func (hs *HistorySidebar) Show() {
255	255		hs.Reload()
256	256		return false
257	257		})
258		-	glib.IdleAdd(&reloadCb, 0)
	258	+	hs.scheduleIdle(reloadCb)
259	259
260	260		// Focus search entry via idle callback to ensure layout is stable.
261	261		cb := glib.SourceFunc(func(uintptr) bool {

@@ -269,7 +269,7 @@ func (hs *HistorySidebar) Show() {
269	269		entry.GrabFocus()
270	270		return false
271	271		})
272		-	glib.IdleAdd(&cb, 0)
	272	+	hs.scheduleIdle(cb)
273	273		}
274	274
275	275		// Hide hides the sidebar.


@@ -255,7 +255,7 @@ func (hs *HistorySidebar) Show() {
255		hs.Reload()	255		hs.Reload()
256		return false	256		return false
257		})	257		})
258	-	glib.IdleAdd(&reloadCb, 0)	258	+	hs.scheduleIdle(reloadCb)
259			259
260		// Focus search entry via idle callback to ensure layout is stable.	260		// Focus search entry via idle callback to ensure layout is stable.
261		cb := glib.SourceFunc(func(uintptr) bool {	261		cb := glib.SourceFunc(func(uintptr) bool {

@@ -269,7 +269,7 @@ func (hs *HistorySidebar) Show() {
269		entry.GrabFocus()	269		entry.GrabFocus()
270		return false	270		return false
271		})	271		})
272	-	glib.IdleAdd(&cb, 0)	272	+	hs.scheduleIdle(cb)
273		}	273		}
274			274
275		// Hide hides the sidebar.	275		// Hide hides the sidebar.

M internal/ui/component/history_sidebar_keyboard.go +4 -2


@@ -229,10 +229,10 @@ func (hs *HistorySidebar) rebuildLocalGroups() {
229	229
230	230		// removeFromAllEntries removes all history entries matching the given URL or ID
231	231		// from hs.allEntries. Must be called with hs.mu write lock held.
232		-	func (hs *HistorySidebar) removeFromAllEntries(url string, id int64) {
	232	+	func (hs *HistorySidebar) removeFromAllEntries(_ string, id int64) {
233	233		filtered := make([]*entity.HistoryEntry, 0, len(hs.allEntries))
234	234		for _, e := range hs.allEntries {
235		-	if e != nil && (e.URL == url \|\| e.ID == id) {
	235	+	if e != nil && e.ID == id {
236	236		continue
237	237		}
238	238		filtered = append(filtered, e)

@@ -377,6 +377,8 @@ func (hs *HistorySidebar) jumpToLastSelectable() {
377	377		lastRow = row
378	378		}
379	379		}
	380	+	// GetRowAtIndex returns nil once the index is past the end of the list,
	381	+	// so this scan terminates naturally at the first missing row.
380	382		// If we found a selectable row, try it. Otherwise fall back to last row.
381	383		if lastRow != nil {
382	384		hs.listBox.SelectRow(lastRow)

 // removeFromAllEntries removes all history entries matching the given URL or ID
 // from hs.allEntries. Must be called with hs.mu write lock held.
-func (hs *HistorySidebar) removeFromAllEntries(url string, id int64) {
+func (hs *HistorySidebar) removeFromAllEntries(_ string, id int64) {
 	filtered := make([]*entity.HistoryEntry, 0, len(hs.allEntries))
 	for _, e := range hs.allEntries {
-		if e != nil && (e.URL == url || e.ID == id) {
+		if e != nil && e.ID == id {
 			continue
 		}
 		filtered = append(filtered, e)
 			lastRow = row
 		}
 	}
+	// GetRowAtIndex returns nil once the index is past the end of the list,
+	// so this scan terminates naturally at the first missing row.
 	// If we found a selectable row, try it. Otherwise fall back to last row.
 	if lastRow != nil {
 		hs.listBox.SelectRow(lastRow)

M internal/ui/component/history_sidebar_loading_search.go +4 -5


@@ -353,14 +353,15 @@ func (hs *HistorySidebar) applyFilter() {
353	353		func (hs *HistorySidebar) doFTSearch(query string, gen uint64) {
354	354		hs.mu.RLock()
355	355		uc := hs.historyUC
	356	+	ctx := hs.ctx
356	357		hs.mu.RUnlock()
357	358
358		-	if uc == nil {
	359	+	if uc == nil \|\| ctx == nil {
359	360		return
360	361		}
361	362
362	363		go func() {
363		-	out, err := uc.Search(hs.ctx, dto.HistorySearchInput{Query: query, Limit: sidebarSearchLimit})
	364	+	out, err := uc.Search(ctx, dto.HistorySearchInput{Query: query, Limit: sidebarSearchLimit})
364	365		var entries []*entity.HistoryEntry
365	366		if out != nil {
366	367		entries = make([]*entity.HistoryEntry, len(out.Matches))

@@ -397,9 +398,7 @@ func (hs HistorySidebar) applySearchResults(entries []entity.HistoryEntry, gen
397	398		}
398	399		hs.searchResults = entries
399	400		hs.searchDone = true
400		-	if err != nil {
401		-	hs.searchErr = err
402		-	}
	401	+	hs.searchErr = err
403	402		hs.setDisplayGroupsLocked(groupHistoryByDay(entries))
404	403		return true
405	404		}

 func (hs *HistorySidebar) doFTSearch(query string, gen uint64) {
 	hs.mu.RLock()
 	uc := hs.historyUC
+	ctx := hs.ctx
 	hs.mu.RUnlock()
-	if uc == nil {
+	if uc == nil || ctx == nil {
 		return
 	}
 	go func() {
-		out, err := uc.Search(hs.ctx, dto.HistorySearchInput{Query: query, Limit: sidebarSearchLimit})
+		out, err := uc.Search(ctx, dto.HistorySearchInput{Query: query, Limit: sidebarSearchLimit})
 		var entries []*entity.HistoryEntry
 		if out != nil {
 			entries = make([]*entity.HistoryEntry, len(out.Matches))
 	}
 	hs.searchResults = entries
 	hs.searchDone = true
-	if err != nil {
+	hs.searchErr = err
-		hs.searchErr = err
-	}
 	hs.setDisplayGroupsLocked(groupHistoryByDay(entries))
 	return true
 }

M internal/ui/component/history_sidebar_rendering.go +5 -3


@@ -24,6 +24,7 @@ func (hs *HistorySidebar) rebuildList() {
24	24		rows := hs.displayRows
25	25		query := hs.currentQuery
26	26		hasSearchResults := hs.searchResults != nil
	27	+	searchDone := hs.searchDone
27	28		totalLoaded := hs.totalLoaded
28	29		hs.mu.RUnlock()
29	30

@@ -33,7 +34,7 @@ func (hs *HistorySidebar) rebuildList() {
33	34		if len(rows) == 0 {
34	35		if !hasSearchResults && totalLoaded == 0 {
35	36		// Browse has not loaded yet AND no search has completed.
36		-	hs.showLoadingOrEmpty()
	37	+	hs.showLoadingOrEmpty(query, searchDone)
37	38		return
38	39		}
39	40		// Search completed with 0 results, or browse loaded but empty (no history).

@@ -60,7 +61,7 @@ func (hs *HistorySidebar) rebuildList() {
60	61		hs.ensureAtLeastOneSelection()
61	62		}
62	63
63		-	func (hs *HistorySidebar) showLoadingOrEmpty() {
	64	+	func (hs *HistorySidebar) showLoadingOrEmpty(query string, searchDone bool) {
64	65		label := gtk.NewLabel(nil)
65	66		if label == nil {
66	67		return

@@ -69,12 +70,13 @@ func (hs *HistorySidebar) showLoadingOrEmpty() {
69	70
70	71		hs.mu.RLock()
71	72		isLoading := hs.isLoading
72		-	query := hs.currentQuery
73	73		hs.mu.RUnlock()
74	74
75	75		switch {
76	76		case isLoading && query == "":
77	77		label.SetText("Loading history...")
	78	+	case query != "" && !searchDone:
	79	+	label.SetText("Searching...")
78	80		case query != "":
79	81		label.SetText(fmt.Sprintf("No results for \"%s\"", query))
80	82		default:

 	rows := hs.displayRows
 	query := hs.currentQuery
 	hasSearchResults := hs.searchResults != nil
+	searchDone := hs.searchDone
 	totalLoaded := hs.totalLoaded
 	hs.mu.RUnlock()
 	if len(rows) == 0 {
 		if !hasSearchResults && totalLoaded == 0 {
 			// Browse has not loaded yet AND no search has completed.
-			hs.showLoadingOrEmpty()
+			hs.showLoadingOrEmpty(query, searchDone)
 			return
 		}
 		// Search completed with 0 results, or browse loaded but empty (no history).
 	hs.ensureAtLeastOneSelection()
 }
-func (hs *HistorySidebar) showLoadingOrEmpty() {
+func (hs *HistorySidebar) showLoadingOrEmpty(query string, searchDone bool) {
 	label := gtk.NewLabel(nil)
 	if label == nil {
 		return
 	hs.mu.RLock()
 	isLoading := hs.isLoading
-	query := hs.currentQuery
 	hs.mu.RUnlock()
 	switch {
 	case isLoading && query == "":
 		label.SetText("Loading history...")
+	case query != "" && !searchDone:
+		label.SetText("Searching...")
 	case query != "":
 		label.SetText(fmt.Sprintf("No results for \"%s\"", query))
 	default:

M internal/ui/component/history_sidebar_search_test.go +1 -1


@@ -271,7 +271,7 @@ func TestApplySearchResults_CurrentGenerationAppliedAfterSearch(t *testing.T) {
271	271		}
272	272
273	273		hs.mu.RLock()
274		-	assert.NotNil(t, hs.searchResults)
	274	+	require.NotNil(t, hs.searchResults)
275	275		assert.True(t, hs.searchDone)
276	276		assert.Len(t, hs.searchResults, 1)
277	277		assert.Equal(t, "https://live.com", hs.searchResults[0].URL)


@@ -271,7 +271,7 @@ func TestApplySearchResults_CurrentGenerationAppliedAfterSearch(t *testing.T) {
271		}	271		}
272			272
273		hs.mu.RLock()	273		hs.mu.RLock()
274	-	assert.NotNil(t, hs.searchResults)	274	+	require.NotNil(t, hs.searchResults)
275		assert.True(t, hs.searchDone)	275		assert.True(t, hs.searchDone)
276		assert.Len(t, hs.searchResults, 1)	276		assert.Len(t, hs.searchResults, 1)
277		assert.Equal(t, "https://live.com", hs.searchResults[0].URL)	277		assert.Equal(t, "https://live.com", hs.searchResults[0].URL)

M internal/ui/window/main_window.go +0 -1


@@ -114,7 +114,6 @@ func New(ctx context.Context, app gtk.Application, tabBarPosition string) (Mai
114	114		mw.mainContentBox.SetHexpand(true)
115	115		mw.mainContentBox.SetVexpand(true)
116	116		mw.mainContentBox.SetVisible(true)
117		-	mw.mainContentBox.AddCssClass("content-area")
118	117
119	118		// Sidebar box (hidden by default).
120	119		mw.sidebarBox = gtk.NewBox(gtk.OrientationVerticalValue, 0)


@@ -114,7 +114,6 @@ func New(ctx context.Context, app gtk.Application, tabBarPosition string) (Mai
114		mw.mainContentBox.SetHexpand(true)	114	mw.mainContentBox.SetHexpand(true)
115		mw.mainContentBox.SetVexpand(true)	115	mw.mainContentBox.SetVexpand(true)
116		mw.mainContentBox.SetVisible(true)	116	mw.mainContentBox.SetVisible(true)
117	-	mw.mainContentBox.AddCssClass("content-area")
118			117
119		// Sidebar box (hidden by default).	118	// Sidebar box (hidden by default).
120		mw.sidebarBox = gtk.NewBox(gtk.OrientationVerticalValue, 0)	119	mw.sidebarBox = gtk.NewBox(gtk.OrientationVerticalValue, 0)