fix(config): migrate favorites sidebar shortcut#334
Open
bnema wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughExtends the config migrator to detect and upgrade obsolete favorites shortcut bindings (empty ChangesObsolete favorites shortcut migration and toggle toast
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/infrastructure/config/migrate.go`:
- Around line 918-926: The sentinel in Migrator.isObsoleteDefaultAction should
not rely on an explicit empty Keys slice, because that is also a valid
user-unbound shortcut state. Update the logic so only the legacy shipped default
for "workspace.shortcuts.actions" / "toggle-favorites-systemview" is treated as
obsolete, using a more specific check than len(binding.Keys) == 0 and the legacy
Desc text; keep user-customized empty bindings from being rewritten during
Load() and Migrate().
In `@internal/ui/browser_window_favorites_sidebar.go`:
- Around line 130-138: The success toast in the Favorites sidebar is being
routed through showToastOnLastFocusedBrowserWindow, which can attach it to a
different window if focus changes during FavoritesUC.Toggle. Update the Toggle
success path in the browser window favorites sidebar flow to use the current
webview/browser window associated with the toggled page (for example via the
existing wv/browser-window context) instead of re-resolving the last focused
window, so the toast is shown on the same window that initiated the favorite
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70db8232-792b-430b-afba-17649a50be3b
📒 Files selected for processing (4)
internal/infrastructure/config/loader_actions_test.gointernal/infrastructure/config/migrate.gointernal/infrastructure/config/migrate_test.gointernal/ui/browser_window_favorites_sidebar.go
Comment on lines
+918
to
+926
| func (m *Migrator) isObsoleteDefaultAction(actionMapKey, actionName string, actionValue any) bool { | ||
| if actionMapKey != "workspace.shortcuts.actions" || actionName != "toggle-favorites-systemview" { | ||
| return false | ||
| } | ||
| binding, ok := m.actionBindingFromAny(actionValue) | ||
| if !ok { | ||
| return false | ||
| } | ||
| return len(binding.Keys) == 0 && strings.TrimSpace(binding.Desc) == "Toggle Favorites in right split" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don't use an explicit empty binding as the obsolete-default sentinel.
keys = [] is already a valid persisted shortcut state, so this heuristic cannot distinguish the old shipped default from a user intentionally unbinding ctrl+b. Any config that disables the shortcut but keeps the legacy description will now be rewritten back to ctrl+b during both Load() and Migrate(), which breaks the “preserve existing custom shortcut bindings” goal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/infrastructure/config/migrate.go` around lines 918 - 926, The
sentinel in Migrator.isObsoleteDefaultAction should not rely on an explicit
empty Keys slice, because that is also a valid user-unbound shortcut state.
Update the logic so only the legacy shipped default for
"workspace.shortcuts.actions" / "toggle-favorites-systemview" is treated as
obsolete, using a more specific check than len(binding.Keys) == 0 and the legacy
Desc text; keep user-customized empty bindings from being rewritten during
Load() and Migrate().
Comment on lines
+130
to
+138
| result, err := a.deps.FavoritesUC.Toggle(ctx, uri, wv.Title()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if result != nil && strings.TrimSpace(result.Message) != "" { | ||
| a.showToastOnLastFocusedBrowserWindow(ctx, result.Message, component.ToastSuccess, | ||
| component.WithDuration(component.ToastBriefDurationMs), | ||
| component.WithPosition(component.ToastPositionBottomRight), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Bind the toast to the same browser window as the toggled page.
This success path re-resolves the focused window via showToastOnLastFocusedBrowserWindow(). If focus changes while FavoritesUC.Toggle runs, the toast can land in a different window than the webview whose favorite state was toggled.
Suggested fix
func (a *App) toggleCurrentPageFavoriteAction(ctx context.Context) error {
if a == nil || a.deps == nil || a.deps.FavoritesUC == nil {
return fmt.Errorf("favorites unavailable: usecase not configured")
}
- _, wv := a.activeWebViewForBrowserWindow(a.lastFocusedBrowserWindow())
+ bw := a.lastFocusedBrowserWindow()
+ _, wv := a.activeWebViewForBrowserWindow(bw)
if wv == nil {
return fmt.Errorf("favorites unavailable: no active webview")
}
uri := strings.TrimSpace(wv.URI())
if uri == "" {
@@
}
if result != nil && strings.TrimSpace(result.Message) != "" {
- a.showToastOnLastFocusedBrowserWindow(ctx, result.Message, component.ToastSuccess,
+ a.showToastOnBrowserWindow(ctx, bw, result.Message, component.ToastSuccess,
component.WithDuration(component.ToastBriefDurationMs),
component.WithPosition(component.ToastPositionBottomRight),
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| result, err := a.deps.FavoritesUC.Toggle(ctx, uri, wv.Title()) | |
| if err != nil { | |
| return err | |
| } | |
| if result != nil && strings.TrimSpace(result.Message) != "" { | |
| a.showToastOnLastFocusedBrowserWindow(ctx, result.Message, component.ToastSuccess, | |
| component.WithDuration(component.ToastBriefDurationMs), | |
| component.WithPosition(component.ToastPositionBottomRight), | |
| ) | |
| bw := a.lastFocusedBrowserWindow() | |
| _, wv := a.activeWebViewForBrowserWindow(bw) | |
| if wv == nil { | |
| return fmt.Errorf("favorites unavailable: no active webview") | |
| } | |
| uri := strings.TrimSpace(wv.URI()) | |
| if uri == "" { | |
| return fmt.Errorf("favorites unavailable: current page has no URI") | |
| } | |
| result, err := a.deps.FavoritesUC.Toggle(ctx, uri, wv.Title()) | |
| if err != nil { | |
| return err | |
| } | |
| if result != nil && strings.TrimSpace(result.Message) != "" { | |
| a.showToastOnBrowserWindow(ctx, bw, result.Message, component.ToastSuccess, | |
| component.WithDuration(component.ToastBriefDurationMs), | |
| component.WithPosition(component.ToastPositionBottomRight), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/browser_window_favorites_sidebar.go` around lines 130 - 138, The
success toast in the Favorites sidebar is being routed through
showToastOnLastFocusedBrowserWindow, which can attach it to a different window
if focus changes during FavoritesUC.Toggle. Update the Toggle success path in
the browser window favorites sidebar flow to use the current webview/browser
window associated with the toggled page (for example via the existing
wv/browser-window context) instead of re-resolving the last focused window, so
the toast is shown on the same window that initiated the favorite change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
toggle-favorites-systemviewshortcut default with empty keys and migrate it toctrl+btoggle-current-page-favoritedefault shortcut asctrl+dduring migration/loadReview
Verification
go test ./internal/infrastructure/config -run 'TestManagerLoad_UpgradesObsoleteFavoritesShortcutDefaultInMemory|TestMigrator_ReportsAndMigratesObsoleteFavoritesShortcutDefault' -count=1 -v\n-go test ./internal/infrastructure/config ./internal/ui -count=1\n-make lint\n-make test\n-make check\n-git diff --checkSummary by CodeRabbit
New Features
Bug Fixes