fix(favorites): address sidebar review requests
- commit
- 284f18b57df7b77f781a6ac3ed7b308c2b4cc46a
- author
- brice · · 2026-06-28 16:51 UTC
- parent
- 91c19f7
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +55 -18
M internal/ui/browser_window_favorites_sidebar.go +13 -3
| @@ -131,8 +131,18 @@ func (a *App) toggleCurrentPageFavoriteAction(ctx context.Context) error { | |||
| 131 | 131 | if err != nil { | |
| 132 | 132 | return err | |
| 133 | 133 | } | |
| 134 | - | if bw := a.lastFocusedBrowserWindow(); bw != nil && bw.favoritesSidebar != nil { | |
| 135 | - | bw.favoritesSidebar.RequestReloadIfVisible("current-page-favorite-toggle") | |
| 136 | - | } | |
| 134 | + | a.reloadVisibleFavoritesSidebars("current-page-favorite-toggle") | |
| 137 | 135 | return nil | |
| 138 | 136 | } | |
| 137 | + | ||
| 138 | + | func (a *App) reloadVisibleFavoritesSidebars(reason string) { | |
| 139 | + | if a == nil { | |
| 140 | + | return | |
| 141 | + | } | |
| 142 | + | for _, bw := range a.browserWindows { | |
| 143 | + | if bw == nil || bw.favoritesSidebar == nil { | |
| 144 | + | continue | |
| 145 | + | } | |
| 146 | + | bw.favoritesSidebar.RequestReloadIfVisible(reason) | |
| 147 | + | } | |
| 148 | + | } | |
| @@ -131,8 +131,18 @@ func (a *App) toggleCurrentPageFavoriteAction(ctx context.Context) error { | |||||
| 131 | if err != nil { | 131 | if err != nil { | ||
| 132 | return err | 132 | return err | ||
| 133 | } | 133 | } | ||
| 134 | - | if bw := a.lastFocusedBrowserWindow(); bw != nil && bw.favoritesSidebar != nil { | 134 | + | a.reloadVisibleFavoritesSidebars("current-page-favorite-toggle") |
| 135 | - | bw.favoritesSidebar.RequestReloadIfVisible("current-page-favorite-toggle") | |||
| 136 | - | } | |||
| 137 | return nil | 135 | return nil | ||
| 138 | } | 136 | } | ||
| 137 | + | ||||
| 138 | + | func (a *App) reloadVisibleFavoritesSidebars(reason string) { | |||
| 139 | + | if a == nil { | |||
| 140 | + | return | |||
| 141 | + | } | |||
| 142 | + | for _, bw := range a.browserWindows { | |||
| 143 | + | if bw == nil || bw.favoritesSidebar == nil { | |||
| 144 | + | continue | |||
| 145 | + | } | |||
| 146 | + | bw.favoritesSidebar.RequestReloadIfVisible(reason) | |||
| 147 | + | } | |||
| 148 | + | } | |||
M internal/ui/component/favorites_sidebar.go +14 -13
| @@ -49,19 +49,20 @@ type FavoritesSidebar struct { | |||
| 49 | 49 | onOpenInNewPane func(ctx context.Context, url string) error | |
| 50 | 50 | onClose func() | |
| 51 | 51 | ||
| 52 | - | allFavorites []*entity.Favorite | |
| 53 | - | allTags []*entity.Tag | |
| 54 | - | selectedTagIDs map[entity.TagID]struct{} | |
| 55 | - | displayRows []favoriteSidebarDisplayRow | |
| 56 | - | currentQuery string | |
| 57 | - | notice string | |
| 58 | - | loadGen uint64 | |
| 59 | - | destroyed bool | |
| 60 | - | visible bool | |
| 61 | - | focusZone favoritesSidebarFocusZone | |
| 62 | - | mode favoritesSidebarMode | |
| 63 | - | editingID entity.FavoriteID | |
| 64 | - | confirmDelete bool | |
| 52 | + | allFavorites []*entity.Favorite | |
| 53 | + | allTags []*entity.Tag | |
| 54 | + | selectedTagIDs map[entity.TagID]struct{} | |
| 55 | + | displayRows []favoriteSidebarDisplayRow | |
| 56 | + | currentQuery string | |
| 57 | + | notice string | |
| 58 | + | loadGen uint64 | |
| 59 | + | destroyed bool | |
| 60 | + | visible bool | |
| 61 | + | focusZone favoritesSidebarFocusZone | |
| 62 | + | mode favoritesSidebarMode | |
| 63 | + | editingID entity.FavoriteID | |
| 64 | + | confirmDelete bool | |
| 65 | + | confirmDeleteID entity.FavoriteID | |
| 65 | 66 | ||
| 66 | 67 | retainedCallbacks []interface{} | |
| 67 | 68 | tagCallbacks []interface{} | |
| @@ -49,19 +49,20 @@ type FavoritesSidebar struct { | |||||
| 49 | onOpenInNewPane func(ctx context.Context, url string) error | 49 | onOpenInNewPane func(ctx context.Context, url string) error | ||
| 50 | onClose func() | 50 | onClose func() | ||
| 51 | 51 | ||||
| 52 | - | allFavorites []*entity.Favorite | 52 | + | allFavorites []*entity.Favorite |
| 53 | - | allTags []*entity.Tag | 53 | + | allTags []*entity.Tag |
| 54 | - | selectedTagIDs map[entity.TagID]struct{} | 54 | + | selectedTagIDs map[entity.TagID]struct{} |
| 55 | - | displayRows []favoriteSidebarDisplayRow | 55 | + | displayRows []favoriteSidebarDisplayRow |
| 56 | - | currentQuery string | 56 | + | currentQuery string |
| 57 | - | notice string | 57 | + | notice string |
| 58 | - | loadGen uint64 | 58 | + | loadGen uint64 |
| 59 | - | destroyed bool | 59 | + | destroyed bool |
| 60 | - | visible bool | 60 | + | visible bool |
| 61 | - | focusZone favoritesSidebarFocusZone | 61 | + | focusZone favoritesSidebarFocusZone |
| 62 | - | mode favoritesSidebarMode | 62 | + | mode favoritesSidebarMode |
| 63 | - | editingID entity.FavoriteID | 63 | + | editingID entity.FavoriteID |
| 64 | - | confirmDelete bool | 64 | + | confirmDelete bool |
| 65 | + | confirmDeleteID entity.FavoriteID | |||
| 65 | 66 | ||||
| 66 | retainedCallbacks []interface{} | 67 | retainedCallbacks []interface{} | ||
| 67 | tagCallbacks []interface{} | 68 | tagCallbacks []interface{} | ||
M internal/ui/component/favorites_sidebar_forms.go +9 -2
| @@ -41,6 +41,7 @@ func (fs *FavoritesSidebar) beginAddForm() { | |||
| 41 | 41 | fs.mode = favoritesSidebarModeAdd | |
| 42 | 42 | fs.editingID = 0 | |
| 43 | 43 | fs.confirmDelete = false | |
| 44 | + | fs.confirmDeleteID = 0 | |
| 44 | 45 | fs.notice = "Add favorite: URL, title, comma-separated tag IDs, shortcut 1-9. Press Ctrl+Enter or Save." | |
| 45 | 46 | fs.mu.Unlock() | |
| 46 | 47 | fs.renderForm(nil) | |
| @@ -61,6 +62,7 @@ func (fs *FavoritesSidebar) beginEditForm() { | |||
| 61 | 62 | fs.mode = favoritesSidebarModeEdit | |
| 62 | 63 | fs.editingID = fav.ID | |
| 63 | 64 | fs.confirmDelete = false | |
| 65 | + | fs.confirmDeleteID = 0 | |
| 64 | 66 | fs.notice = "Edit favorite: URL is read-only; use tag mode for tags. Press Ctrl+Enter or Save." | |
| 65 | 67 | fs.mu.Unlock() | |
| 66 | 68 | fs.renderForm(fav) | |
| @@ -166,6 +168,7 @@ func (fs *FavoritesSidebar) cancelManagement() bool { | |||
| 166 | 168 | fs.mode = favoritesSidebarModeNone | |
| 167 | 169 | fs.editingID = 0 | |
| 168 | 170 | fs.confirmDelete = false | |
| 171 | + | fs.confirmDeleteID = 0 | |
| 169 | 172 | if active { | |
| 170 | 173 | fs.notice = "" | |
| 171 | 174 | } | |
| @@ -246,6 +249,7 @@ func (fs *FavoritesSidebar) setModeNotice(mode favoritesSidebarMode, notice stri | |||
| 246 | 249 | if !fs.destroyed { | |
| 247 | 250 | fs.mode = mode | |
| 248 | 251 | fs.confirmDelete = false | |
| 252 | + | fs.confirmDeleteID = 0 | |
| 249 | 253 | fs.notice = notice | |
| 250 | 254 | } | |
| 251 | 255 | fs.mu.Unlock() | |
| @@ -311,13 +315,15 @@ func (fs *FavoritesSidebar) setShortcutKey(key *int) bool { | |||
| 311 | 315 | } | |
| 312 | 316 | ||
| 313 | 317 | func (fs *FavoritesSidebar) requestDeleteConfirmation() bool { | |
| 314 | - | if fs.selectedFavorite() == nil { | |
| 318 | + | fav := fs.selectedFavorite() | |
| 319 | + | if fav == nil { | |
| 315 | 320 | fs.setNotice("Select a favorite to delete") | |
| 316 | 321 | return true | |
| 317 | 322 | } | |
| 318 | 323 | fs.mu.Lock() | |
| 319 | 324 | if !fs.destroyed { | |
| 320 | 325 | fs.confirmDelete = true | |
| 326 | + | fs.confirmDeleteID = fav.ID | |
| 321 | 327 | fs.notice = "Delete selected favorite? Press Delete again to confirm, Esc to cancel." | |
| 322 | 328 | } | |
| 323 | 329 | fs.mu.Unlock() | |
| @@ -350,10 +356,11 @@ func (fs *FavoritesSidebar) confirmDeleteFavorite() bool { | |||
| 350 | 356 | } | |
| 351 | 357 | fs.mu.RLock() | |
| 352 | 358 | confirmed := fs.confirmDelete | |
| 359 | + | confirmedID := fs.confirmDeleteID | |
| 353 | 360 | uc := fs.favoritesUC | |
| 354 | 361 | ctx := fs.ctx | |
| 355 | 362 | fs.mu.RUnlock() | |
| 356 | - | if !confirmed { | |
| 363 | + | if !confirmed || confirmedID != fav.ID { | |
| 357 | 364 | return fs.requestDeleteConfirmation() | |
| 358 | 365 | } | |
| 359 | 366 | if uc == nil { | |
| @@ -41,6 +41,7 @@ func (fs *FavoritesSidebar) beginAddForm() { | |||||
| 41 | fs.mode = favoritesSidebarModeAdd | 41 | fs.mode = favoritesSidebarModeAdd | ||
| 42 | fs.editingID = 0 | 42 | fs.editingID = 0 | ||
| 43 | fs.confirmDelete = false | 43 | fs.confirmDelete = false | ||
| 44 | + | fs.confirmDeleteID = 0 | |||
| 44 | fs.notice = "Add favorite: URL, title, comma-separated tag IDs, shortcut 1-9. Press Ctrl+Enter or Save." | 45 | fs.notice = "Add favorite: URL, title, comma-separated tag IDs, shortcut 1-9. Press Ctrl+Enter or Save." | ||
| 45 | fs.mu.Unlock() | 46 | fs.mu.Unlock() | ||
| 46 | fs.renderForm(nil) | 47 | fs.renderForm(nil) | ||
| @@ -61,6 +62,7 @@ func (fs *FavoritesSidebar) beginEditForm() { | |||||
| 61 | fs.mode = favoritesSidebarModeEdit | 62 | fs.mode = favoritesSidebarModeEdit | ||
| 62 | fs.editingID = fav.ID | 63 | fs.editingID = fav.ID | ||
| 63 | fs.confirmDelete = false | 64 | fs.confirmDelete = false | ||
| 65 | + | fs.confirmDeleteID = 0 | |||
| 64 | fs.notice = "Edit favorite: URL is read-only; use tag mode for tags. Press Ctrl+Enter or Save." | 66 | fs.notice = "Edit favorite: URL is read-only; use tag mode for tags. Press Ctrl+Enter or Save." | ||
| 65 | fs.mu.Unlock() | 67 | fs.mu.Unlock() | ||
| 66 | fs.renderForm(fav) | 68 | fs.renderForm(fav) | ||
| @@ -166,6 +168,7 @@ func (fs *FavoritesSidebar) cancelManagement() bool { | |||||
| 166 | fs.mode = favoritesSidebarModeNone | 168 | fs.mode = favoritesSidebarModeNone | ||
| 167 | fs.editingID = 0 | 169 | fs.editingID = 0 | ||
| 168 | fs.confirmDelete = false | 170 | fs.confirmDelete = false | ||
| 171 | + | fs.confirmDeleteID = 0 | |||
| 169 | if active { | 172 | if active { | ||
| 170 | fs.notice = "" | 173 | fs.notice = "" | ||
| 171 | } | 174 | } | ||
| @@ -246,6 +249,7 @@ func (fs *FavoritesSidebar) setModeNotice(mode favoritesSidebarMode, notice stri | |||||
| 246 | if !fs.destroyed { | 249 | if !fs.destroyed { | ||
| 247 | fs.mode = mode | 250 | fs.mode = mode | ||
| 248 | fs.confirmDelete = false | 251 | fs.confirmDelete = false | ||
| 252 | + | fs.confirmDeleteID = 0 | |||
| 249 | fs.notice = notice | 253 | fs.notice = notice | ||
| 250 | } | 254 | } | ||
| 251 | fs.mu.Unlock() | 255 | fs.mu.Unlock() | ||
| @@ -311,13 +315,15 @@ func (fs *FavoritesSidebar) setShortcutKey(key *int) bool { | |||||
| 311 | } | 315 | } | ||
| 312 | 316 | ||||
| 313 | func (fs *FavoritesSidebar) requestDeleteConfirmation() bool { | 317 | func (fs *FavoritesSidebar) requestDeleteConfirmation() bool { | ||
| 314 | - | if fs.selectedFavorite() == nil { | 318 | + | fav := fs.selectedFavorite() |
| 319 | + | if fav == nil { | |||
| 315 | fs.setNotice("Select a favorite to delete") | 320 | fs.setNotice("Select a favorite to delete") | ||
| 316 | return true | 321 | return true | ||
| 317 | } | 322 | } | ||
| 318 | fs.mu.Lock() | 323 | fs.mu.Lock() | ||
| 319 | if !fs.destroyed { | 324 | if !fs.destroyed { | ||
| 320 | fs.confirmDelete = true | 325 | fs.confirmDelete = true | ||
| 326 | + | fs.confirmDeleteID = fav.ID | |||
| 321 | fs.notice = "Delete selected favorite? Press Delete again to confirm, Esc to cancel." | 327 | fs.notice = "Delete selected favorite? Press Delete again to confirm, Esc to cancel." | ||
| 322 | } | 328 | } | ||
| 323 | fs.mu.Unlock() | 329 | fs.mu.Unlock() | ||
| @@ -350,10 +356,11 @@ func (fs *FavoritesSidebar) confirmDeleteFavorite() bool { | |||||
| 350 | } | 356 | } | ||
| 351 | fs.mu.RLock() | 357 | fs.mu.RLock() | ||
| 352 | confirmed := fs.confirmDelete | 358 | confirmed := fs.confirmDelete | ||
| 359 | + | confirmedID := fs.confirmDeleteID | |||
| 353 | uc := fs.favoritesUC | 360 | uc := fs.favoritesUC | ||
| 354 | ctx := fs.ctx | 361 | ctx := fs.ctx | ||
| 355 | fs.mu.RUnlock() | 362 | fs.mu.RUnlock() | ||
| 356 | - | if !confirmed { | 363 | + | if !confirmed || confirmedID != fav.ID { |
| 357 | return fs.requestDeleteConfirmation() | 364 | return fs.requestDeleteConfirmation() | ||
| 358 | } | 365 | } | ||
| 359 | if uc == nil { | 366 | if uc == nil { | ||
M internal/ui/component/favorites_sidebar_test.go +19 -0
| @@ -402,6 +402,7 @@ func TestFavoritesSidebarEnterConfirmsDelete(t *testing.T) { | |||
| 402 | 402 | uc := fs.favoritesUC.(*fakeFavoritesSidebarUC) | |
| 403 | 403 | fs.displayRows = []favoriteSidebarDisplayRow{{FavoriteID: fav.ID, Favorite: fav, URL: fav.URL, Selectable: true}} | |
| 404 | 404 | fs.confirmDelete = true | |
| 405 | + | fs.confirmDeleteID = fav.ID | |
| 405 | 406 | ||
| 406 | 407 | assert.True(t, fs.confirmDeleteActive()) | |
| 407 | 408 | assert.True(t, fs.confirmDeleteFavorite()) | |
| @@ -463,6 +464,24 @@ func TestFavoritesSidebarConfirmDeleteRequiresConfirmationState(t *testing.T) { | |||
| 463 | 464 | assert.True(t, fs.confirmDelete) | |
| 464 | 465 | } | |
| 465 | 466 | ||
| 467 | + | func TestFavoritesSidebarConfirmDeleteRepromptsWhenSelectionChanges(t *testing.T) { | |
| 468 | + | first := &entity.Favorite{ID: 5, URL: "https://first.test"} | |
| 469 | + | second := &entity.Favorite{ID: 6, URL: "https://second.test"} | |
| 470 | + | fs := newFavoritesSidebarHarness([]*entity.Favorite{first, second}, nil) | |
| 471 | + | uc := fs.favoritesUC.(*fakeFavoritesSidebarUC) | |
| 472 | + | fs.displayRows = []favoriteSidebarDisplayRow{ | |
| 473 | + | {FavoriteID: second.ID, Favorite: second, URL: second.URL, Selectable: true}, | |
| 474 | + | {FavoriteID: first.ID, Favorite: first, URL: first.URL, Selectable: true}, | |
| 475 | + | } | |
| 476 | + | fs.confirmDelete = true | |
| 477 | + | fs.confirmDeleteID = first.ID | |
| 478 | + | ||
| 479 | + | assert.True(t, fs.confirmDeleteFavorite()) | |
| 480 | + | assert.Empty(t, uc.deletedIDs) | |
| 481 | + | assert.True(t, fs.confirmDelete) | |
| 482 | + | assert.Equal(t, second.ID, fs.confirmDeleteID) | |
| 483 | + | } | |
| 484 | + | ||
| 466 | 485 | func TestFavoritesSidebarAddShortcutFailureClosesFormAfterCreate(t *testing.T) { | |
| 467 | 486 | uc := &fakeFavoritesSidebarUC{errOnSetShortcut: errors.New("shortcut failed")} | |
| 468 | 487 | fs := newFavoritesSidebarHarness(nil, nil) | |
| @@ -402,6 +402,7 @@ func TestFavoritesSidebarEnterConfirmsDelete(t *testing.T) { | |||||
| 402 | uc := fs.favoritesUC.(*fakeFavoritesSidebarUC) | 402 | uc := fs.favoritesUC.(*fakeFavoritesSidebarUC) | ||
| 403 | fs.displayRows = []favoriteSidebarDisplayRow{{FavoriteID: fav.ID, Favorite: fav, URL: fav.URL, Selectable: true}} | 403 | fs.displayRows = []favoriteSidebarDisplayRow{{FavoriteID: fav.ID, Favorite: fav, URL: fav.URL, Selectable: true}} | ||
| 404 | fs.confirmDelete = true | 404 | fs.confirmDelete = true | ||
| 405 | + | fs.confirmDeleteID = fav.ID | |||
| 405 | 406 | ||||
| 406 | assert.True(t, fs.confirmDeleteActive()) | 407 | assert.True(t, fs.confirmDeleteActive()) | ||
| 407 | assert.True(t, fs.confirmDeleteFavorite()) | 408 | assert.True(t, fs.confirmDeleteFavorite()) | ||
| @@ -463,6 +464,24 @@ func TestFavoritesSidebarConfirmDeleteRequiresConfirmationState(t *testing.T) { | |||||
| 463 | assert.True(t, fs.confirmDelete) | 464 | assert.True(t, fs.confirmDelete) | ||
| 464 | } | 465 | } | ||
| 465 | 466 | ||||
| 467 | + | func TestFavoritesSidebarConfirmDeleteRepromptsWhenSelectionChanges(t *testing.T) { | |||
| 468 | + | first := &entity.Favorite{ID: 5, URL: "https://first.test"} | |||
| 469 | + | second := &entity.Favorite{ID: 6, URL: "https://second.test"} | |||
| 470 | + | fs := newFavoritesSidebarHarness([]*entity.Favorite{first, second}, nil) | |||
| 471 | + | uc := fs.favoritesUC.(*fakeFavoritesSidebarUC) | |||
| 472 | + | fs.displayRows = []favoriteSidebarDisplayRow{ | |||
| 473 | + | {FavoriteID: second.ID, Favorite: second, URL: second.URL, Selectable: true}, | |||
| 474 | + | {FavoriteID: first.ID, Favorite: first, URL: first.URL, Selectable: true}, | |||
| 475 | + | } | |||
| 476 | + | fs.confirmDelete = true | |||
| 477 | + | fs.confirmDeleteID = first.ID | |||
| 478 | + | ||||
| 479 | + | assert.True(t, fs.confirmDeleteFavorite()) | |||
| 480 | + | assert.Empty(t, uc.deletedIDs) | |||
| 481 | + | assert.True(t, fs.confirmDelete) | |||
| 482 | + | assert.Equal(t, second.ID, fs.confirmDeleteID) | |||
| 483 | + | } | |||
| 484 | + | ||||
| 466 | func TestFavoritesSidebarAddShortcutFailureClosesFormAfterCreate(t *testing.T) { | 485 | func TestFavoritesSidebarAddShortcutFailureClosesFormAfterCreate(t *testing.T) { | ||
| 467 | uc := &fakeFavoritesSidebarUC{errOnSetShortcut: errors.New("shortcut failed")} | 486 | uc := &fakeFavoritesSidebarUC{errOnSetShortcut: errors.New("shortcut failed")} | ||
| 468 | fs := newFavoritesSidebarHarness(nil, nil) | 487 | fs := newFavoritesSidebarHarness(nil, nil) | ||