Skip to content

fix(config): migrate favorites sidebar shortcut #334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bnema wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(config): migrate favorites sidebar shortcut #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions internal/infrastructure/config/loader_actions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,36 @@ desc = "Custom stack pane"
stack := cfg.Workspace.PaneMode.Actions["stack-pane"]
assert.Equal(t, "Custom stack pane", stack.Desc)
}

func TestManagerLoad_UpgradesObsoleteFavoritesShortcutDefaultInMemory(t *testing.T) {
home := t.TempDir()
t.Setenv("HOME", home)
t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config"))
t.Setenv("XDG_DATA_HOME", filepath.Join(home, ".local", "share"))
t.Setenv("XDG_STATE_HOME", filepath.Join(home, ".local", "state"))
t.Setenv("XDG_CACHE_HOME", filepath.Join(home, ".cache"))

configFile, err := GetConfigFile()
require.NoError(t, err)
require.NoError(t, os.MkdirAll(filepath.Dir(configFile), 0o755))
require.NoError(t, os.WriteFile(configFile, []byte(`
[workspace.shortcuts.actions.toggle-favorites-systemview]
keys = []
desc = "Toggle Favorites in right split"
`), 0o644))

mgr, err := NewManager()
require.NoError(t, err)
require.NoError(t, mgr.Load())

cfg := mgr.Get()
require.NotNil(t, cfg)
favorites := cfg.Workspace.Shortcuts.Actions["toggle-favorites-systemview"]
assert.Equal(t, []string{"ctrl+b"}, favorites.Keys)
assert.Equal(t, "Toggle Favorites sidebar", favorites.Desc)
currentPage, ok := cfg.Workspace.Shortcuts.Actions["toggle-current-page-favorite"]
require.True(t, ok)
expectedCurrentPage := DefaultConfig().Workspace.Shortcuts.Actions["toggle-current-page-favorite"]
assert.Equal(t, expectedCurrentPage.Keys, currentPage.Keys)
assert.Equal(t, expectedCurrentPage.Desc, currentPage.Desc)
}
107 changes: 101 additions & 6 deletions internal/infrastructure/config/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,22 @@ func (m *Migrator) CheckMigration() (*port.MigrationResult, error) {
return nil, nil
}

// Get user-defined keys from the TOML file
userKeys, err := m.getUserConfigKeys(configFile)
userKeysWithValues, err := m.getUserConfigKeysWithValues(configFile)
if err != nil {
return nil, fmt.Errorf("failed to parse user config: %w", err)
}

userKeys := make(map[string]bool, len(userKeysWithValues))
for key := range userKeysWithValues {
userKeys[key] = true
}

// Get all default keys
defaultKeys := m.getAllDefaultKeys()

// Find missing keys (in defaults but not in user config)
missingKeys := m.findMissingKeys(defaultKeys, userKeys)
missingKeys = appendUniqueKeys(missingKeys, m.detectMissingDefaultActions(userKeysWithValues))

if len(missingKeys) == 0 {
return nil, nil
Expand Down Expand Up @@ -854,11 +859,17 @@ func (m *Migrator) detectMissingDefaultActions(userKeysWithValues map[string]any
}

for actionName := range actionMap.actions {
if _, exists := userActions[actionName]; exists {
if actionValue, exists := userActions[actionName]; exists {
if m.isObsoleteDefaultAction(actionMap.key, actionName, actionValue) {
missing = append(missing, actionMap.key+"."+actionName)
}
continue
}
legacyActionName := strings.ReplaceAll(actionName, "-", "_")
if _, exists := userActions[legacyActionName]; exists {
if actionValue, exists := userActions[legacyActionName]; exists {
if m.isObsoleteDefaultAction(actionMap.key, actionName, actionValue) {
missing = append(missing, actionMap.key+"."+actionName)
}
continue
}
missing = append(missing, actionMap.key+"."+actionName)
Expand All @@ -883,11 +894,18 @@ func (m *Migrator) mergeMissingDefaultActions(rawConfig map[string]any) {
}

for actionName, defaultAction := range actionMap.actions {
if _, exists := userActions[actionName]; exists {
if actionValue, exists := userActions[actionName]; exists {
if m.isObsoleteDefaultAction(actionMap.key, actionName, actionValue) {
userActions[actionName] = defaultAction
}
continue
}
legacyActionName := strings.ReplaceAll(actionName, "-", "_")
if _, exists := userActions[legacyActionName]; exists {
if actionValue, exists := userActions[legacyActionName]; exists {
if m.isObsoleteDefaultAction(actionMap.key, actionName, actionValue) {
delete(userActions, legacyActionName)
userActions[actionName] = defaultAction
}
continue
}
userActions[actionName] = defaultAction
Expand All @@ -897,6 +915,83 @@ func (m *Migrator) mergeMissingDefaultActions(rawConfig map[string]any) {
}
}

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"
Comment on lines +918 to +926

@coderabbitai coderabbitai Bot Jun 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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().

}

func (m *Migrator) actionBindingFromAny(value any) (entity.ActionBinding, bool) {
switch v := value.(type) {
case entity.ActionBinding:
return v, true
case map[string]any:
return actionBindingFromMap(v)
default:
mapping, ok := m.toStringAnyMap(value)
if !ok {
return entity.ActionBinding{}, false
}
return actionBindingFromMap(mapping)
}
}

func actionBindingFromMap(m map[string]any) (entity.ActionBinding, bool) {
var binding entity.ActionBinding
if keysValue, exists := m["keys"]; exists {
keys, ok := stringSliceFromAny(keysValue)
if !ok {
return entity.ActionBinding{}, false
}
binding.Keys = keys
}
if descValue, exists := m["desc"]; exists {
desc, ok := descValue.(string)
if !ok {
return entity.ActionBinding{}, false
}
binding.Desc = desc
}
return binding, true
}

func stringSliceFromAny(value any) ([]string, bool) {
switch v := value.(type) {
case []string:
return append([]string(nil), v...), true
case []any:
out := make([]string, 0, len(v))
for _, item := range v {
s, ok := item.(string)
if !ok {
return nil, false
}
out = append(out, s)
}
return out, true
default:
rv := reflect.ValueOf(value)
if rv.Kind() != reflect.Slice {
return nil, false
}
out := make([]string, 0, rv.Len())
for i := 0; i < rv.Len(); i++ {
item := rv.Index(i).Interface()
s, ok := item.(string)
if !ok {
return nil, false
}
out = append(out, s)
}
return out, true
}
}

func appendUniqueKeys(base, extra []string) []string {
if len(extra) == 0 {
return base
Expand Down
62 changes: 62 additions & 0 deletions internal/infrastructure/config/migrate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1282,3 +1282,65 @@ section:
assert.InDelta(t, 42.0, section["number"].(float64), 0.001)
})
}

func TestMigrator_ReportsAndMigratesObsoleteFavoritesShortcutDefault(t *testing.T) {
home := t.TempDir()
t.Setenv("HOME", home)
t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config"))
t.Setenv("XDG_DATA_HOME", filepath.Join(home, ".local", "share"))
t.Setenv("XDG_STATE_HOME", filepath.Join(home, ".local", "state"))
t.Setenv("XDG_CACHE_HOME", filepath.Join(home, ".cache"))

configFile, err := GetConfigFile()
require.NoError(t, err)
require.NoError(t, os.MkdirAll(filepath.Dir(configFile), 0o755))
require.NoError(t, os.WriteFile(configFile, []byte(`
[workspace.shortcuts.actions.toggle-favorites-systemview]
keys = []
desc = "Toggle Favorites in right split"
`), 0o644))

migrator := NewMigrator()
result, err := migrator.CheckMigration()
require.NoError(t, err)
require.NotNil(t, result)
assert.Contains(t, result.MissingKeys, "workspace.shortcuts.actions.toggle-favorites-systemview")
assert.Contains(t, result.MissingKeys, "workspace.shortcuts.actions.toggle-current-page-favorite")

changes, err := migrator.DetectChanges()
require.NoError(t, err)
assertContainsChange(t, changes, port.KeyChangeAdded, "", "workspace.shortcuts.actions.toggle-favorites-systemview")
assertContainsChange(t, changes, port.KeyChangeAdded, "", "workspace.shortcuts.actions.toggle-current-page-favorite")

applied, err := migrator.Migrate()
require.NoError(t, err)
assert.Contains(t, applied, "workspace.shortcuts.actions.toggle-favorites-systemview")
assert.Contains(t, applied, "workspace.shortcuts.actions.toggle-current-page-favorite")

raw, err := migrator.readRawConfig(configFile)
require.NoError(t, err)
workspace, ok := raw["workspace"].(map[string]any)
require.True(t, ok)
shortcuts, ok := workspace["shortcuts"].(map[string]any)
require.True(t, ok)
actions, ok := shortcuts["actions"].(map[string]any)
require.True(t, ok)

expectedActions := DefaultConfig().Workspace.Shortcuts.Actions
favorites, ok := actions["toggle-favorites-systemview"].(map[string]any)
require.True(t, ok)
assert.Equal(t, []any{"ctrl+b"}, favorites["keys"])
assert.Equal(t, expectedActions["toggle-favorites-systemview"].Desc, favorites["desc"])
currentPage, ok := actions["toggle-current-page-favorite"].(map[string]any)
require.True(t, ok)
assert.Equal(t, []any{"ctrl+d"}, currentPage["keys"])
assert.Equal(t, expectedActions["toggle-current-page-favorite"].Desc, currentPage["desc"])

mgr, err := NewManager()
require.NoError(t, err)
require.NoError(t, mgr.Load())
cfg := mgr.Get()
require.NotNil(t, cfg)
assert.Equal(t, []string{"ctrl+b"}, cfg.Workspace.Shortcuts.Actions["toggle-favorites-systemview"].Keys)
assert.Equal(t, []string{"ctrl+d"}, cfg.Workspace.Shortcuts.Actions["toggle-current-page-favorite"].Keys)
}
8 changes: 7 additions & 1 deletion internal/ui/browser_window_favorites_sidebar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,16 @@ func (a *App) toggleCurrentPageFavoriteAction(ctx context.Context) error {
if uri == "" {
return fmt.Errorf("favorites unavailable: active page has no URI")
}
_, err := a.deps.FavoritesUC.Toggle(ctx, uri, wv.Title())
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),
)
Comment on lines +130 to +138

@coderabbitai coderabbitai Bot Jun 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

}
a.reloadVisibleFavoritesSidebars("current-page-favorite-toggle")
return nil
}
Expand Down