fix(history): tighten final sidebar review issues
- commit
- a3edf9178a83d500decb44ec790f575107206234
- author
- brice · · 2026-06-13 08:02 UTC
- parent
- 0acad67
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +9 -7
M internal/ui/browser_window_history_sidebar.go +4 -0
| @@ -146,6 +146,10 @@ func (a *App) toggleHistorySidebarAction(ctx context.Context) error { | |||
| 146 | 146 | if bw.historySidebar == nil { | |
| 147 | 147 | return fmt.Errorf("history sidebar unavailable: native sidebar not initialized") | |
| 148 | 148 | } | |
| 149 | + | if bw.sidebarVisible { | |
| 150 | + | a.hideAndRestoreFocusForBrowserWindow(bw) | |
| 151 | + | return nil | |
| 152 | + | } | |
| 149 | 153 | bw.toggleHistorySidebar() | |
| 150 | 154 | return nil | |
| 151 | 155 | } | |
| @@ -146,6 +146,10 @@ func (a *App) toggleHistorySidebarAction(ctx context.Context) error { | |||||
| 146 | if bw.historySidebar == nil { | 146 | if bw.historySidebar == nil { | ||
| 147 | return fmt.Errorf("history sidebar unavailable: native sidebar not initialized") | 147 | return fmt.Errorf("history sidebar unavailable: native sidebar not initialized") | ||
| 148 | } | 148 | } | ||
| 149 | + | if bw.sidebarVisible { | |||
| 150 | + | a.hideAndRestoreFocusForBrowserWindow(bw) | |||
| 151 | + | return nil | |||
| 152 | + | } | |||
| 149 | bw.toggleHistorySidebar() | 153 | bw.toggleHistorySidebar() | ||
| 150 | return nil | 154 | return nil | ||
| 151 | } | 155 | } | ||
M internal/ui/component/history_sidebar_keyboard.go +2 -2
| @@ -196,7 +196,7 @@ func (hs *HistorySidebar) applyDeletedEntryLocked(url string, entryID int64, nex | |||
| 196 | 196 | hs.loadGen++ | |
| 197 | 197 | hs.isLoading = false | |
| 198 | 198 | hs.loadStarted = false | |
| 199 | - | hs.removeFromAllEntries(url, entryID) | |
| 199 | + | hs.removeFromAllEntries(entryID) | |
| 200 | 200 | hs.totalLoaded = len(hs.allEntries) | |
| 201 | 201 | hs.removeFromSearchResults(entryID) | |
| 202 | 202 | hs.rebuildLocalGroups() | |
| @@ -229,7 +229,7 @@ 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(_ string, id int64) { | |
| 232 | + | func (hs *HistorySidebar) removeFromAllEntries(id int64) { | |
| 233 | 233 | filtered := make([]*entity.HistoryEntry, 0, len(hs.allEntries)) | |
| 234 | 234 | for _, e := range hs.allEntries { | |
| 235 | 235 | if e != nil && e.ID == id { | |
| @@ -196,7 +196,7 @@ func (hs *HistorySidebar) applyDeletedEntryLocked(url string, entryID int64, nex | |||||
| 196 | hs.loadGen++ | 196 | hs.loadGen++ | ||
| 197 | hs.isLoading = false | 197 | hs.isLoading = false | ||
| 198 | hs.loadStarted = false | 198 | hs.loadStarted = false | ||
| 199 | - | hs.removeFromAllEntries(url, entryID) | 199 | + | hs.removeFromAllEntries(entryID) |
| 200 | hs.totalLoaded = len(hs.allEntries) | 200 | hs.totalLoaded = len(hs.allEntries) | ||
| 201 | hs.removeFromSearchResults(entryID) | 201 | hs.removeFromSearchResults(entryID) | ||
| 202 | hs.rebuildLocalGroups() | 202 | hs.rebuildLocalGroups() | ||
| @@ -229,7 +229,7 @@ func (hs *HistorySidebar) rebuildLocalGroups() { | |||||
| 229 | 229 | ||||
| 230 | // removeFromAllEntries removes all history entries matching the given URL or ID | 230 | // removeFromAllEntries removes all history entries matching the given URL or ID | ||
| 231 | // from hs.allEntries. Must be called with hs.mu write lock held. | 231 | // from hs.allEntries. Must be called with hs.mu write lock held. | ||
| 232 | - | func (hs *HistorySidebar) removeFromAllEntries(_ string, id int64) { | 232 | + | func (hs *HistorySidebar) removeFromAllEntries(id int64) { |
| 233 | filtered := make([]*entity.HistoryEntry, 0, len(hs.allEntries)) | 233 | filtered := make([]*entity.HistoryEntry, 0, len(hs.allEntries)) | ||
| 234 | for _, e := range hs.allEntries { | 234 | for _, e := range hs.allEntries { | ||
| 235 | if e != nil && e.ID == id { | 235 | if e != nil && e.ID == id { | ||
M internal/ui/component/history_sidebar_loading_search.go +1 -3
| @@ -197,9 +197,7 @@ func (hs *HistorySidebar) restoreScrollAndSelection() { | |||
| 197 | 197 | if hs.prevScrollValue > maxVal { | |
| 198 | 198 | hs.prevScrollValue = maxVal | |
| 199 | 199 | } | |
| 200 | - | if hs.prevScrollValue >= 0 { | |
| 201 | - | vadj.SetValue(hs.prevScrollValue) | |
| 202 | - | } | |
| 200 | + | vadj.SetValue(hs.prevScrollValue) | |
| 203 | 201 | } | |
| 204 | 202 | } | |
| 205 | 203 | ||
| @@ -197,9 +197,7 @@ func (hs *HistorySidebar) restoreScrollAndSelection() { | |||||
| 197 | if hs.prevScrollValue > maxVal { | 197 | if hs.prevScrollValue > maxVal { | ||
| 198 | hs.prevScrollValue = maxVal | 198 | hs.prevScrollValue = maxVal | ||
| 199 | } | 199 | } | ||
| 200 | - | if hs.prevScrollValue >= 0 { | 200 | + | vadj.SetValue(hs.prevScrollValue) |
| 201 | - | vadj.SetValue(hs.prevScrollValue) | |||
| 202 | - | } | |||
| 203 | } | 201 | } | ||
| 204 | } | 202 | } | ||
| 205 | 203 | ||||
M internal/ui/component/history_sidebar_search_test.go +2 -2
| @@ -3,6 +3,7 @@ package component | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "errors" | |
| 6 | + | "fmt" | |
| 6 | 7 | "sync" | |
| 7 | 8 | "testing" | |
| 8 | 9 | "time" | |
| @@ -168,8 +169,7 @@ func (f *fakeHistorySidebarHistory) GetRecent(ctx context.Context, limit, offset | |||
| 168 | 169 | if f.getRecentFn != nil { | |
| 169 | 170 | return f.getRecentFn(ctx, limit, offset) | |
| 170 | 171 | } | |
| 171 | - | <-make(chan struct{}) | |
| 172 | - | return nil, nil | |
| 172 | + | return nil, fmt.Errorf("unexpected GetRecent call in fakeHistorySidebarHistory") | |
| 173 | 173 | } | |
| 174 | 174 | ||
| 175 | 175 | func (f *fakeHistorySidebarHistory) Search(ctx context.Context, input dto.HistorySearchInput) (*dto.HistorySearchOutput, error) { | |
| @@ -3,6 +3,7 @@ package component | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "errors" | 5 | "errors" | ||
| 6 | + | "fmt" | |||
| 6 | "sync" | 7 | "sync" | ||
| 7 | "testing" | 8 | "testing" | ||
| 8 | "time" | 9 | "time" | ||
| @@ -168,8 +169,7 @@ func (f *fakeHistorySidebarHistory) GetRecent(ctx context.Context, limit, offset | |||||
| 168 | if f.getRecentFn != nil { | 169 | if f.getRecentFn != nil { | ||
| 169 | return f.getRecentFn(ctx, limit, offset) | 170 | return f.getRecentFn(ctx, limit, offset) | ||
| 170 | } | 171 | } | ||
| 171 | - | <-make(chan struct{}) | 172 | + | return nil, fmt.Errorf("unexpected GetRecent call in fakeHistorySidebarHistory") |
| 172 | - | return nil, nil | |||
| 173 | } | 173 | } | ||
| 174 | 174 | ||||
| 175 | func (f *fakeHistorySidebarHistory) Search(ctx context.Context, input dto.HistorySearchInput) (*dto.HistorySearchOutput, error) { | 175 | func (f *fakeHistorySidebarHistory) Search(ctx context.Context, input dto.HistorySearchInput) (*dto.HistorySearchOutput, error) { | ||