Skip to content

feat(web): add a reset-all button to keybindings settings - #4616

Closed
m-de-graaff wants to merge 4 commits into
pingdotgg:mainfrom
m-de-graaff:feat/reset-keybindings-to-defaults
Closed

feat(web): add a reset-all button to keybindings settings#4616
m-de-graaff wants to merge 4 commits into
pingdotgg:mainfrom
m-de-graaff:feat/reset-keybindings-to-defaults

Conversation

@m-de-graaff

@m-de-graaff m-de-graaff commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closes #4576

Problem

Keybindings settings can reset a single row to its default, but there is no way to get the whole set back. A user carrying customizations from an older version can end up with conflicting bindings and no recourse short of hand-editing keybindings.json — which is exactly what someone in that state does not want to do, and which the issue calls out directly.

Change

  • server.resetKeybindings RPC → Keybindings.resetKeybindingRulesToDefaults, which rewrites the config with DEFAULT_KEYBINDINGS, refreshes the cache, and emits the change event like the existing upsert/remove paths.
  • Reset button in the Keybindings settings header, beside Add keybinding and Open keybindings.json. It confirms first, since it discards every customization at once.

Two deliberate behaviors, both covered by tests:

  • Project script bindings (script.*) are kept. They have no default to restore, so dropping them would delete shortcuts the reset cannot give back. The confirm dialog says so.
  • A config that fails to parse is replaced outright instead of erroring. A malformed keybindings.json is the state this button exists to escape, so refusing to overwrite it would defeat the feature. upsertKeybindingRule keeps its existing refuse-to-overwrite behavior.

Tests

apps/server/src/keybindings.test.ts:

  • reset restores the defaults, drops customizations, and keeps the script.* rule;
  • reset replaces a malformed config with the defaults.

vp test run src/keybindings.test.ts — 23 passed. (fails when config directory is not writable fails on this Windows host both with and without this change: fs.chmod does not restrict the directory there. Verified against a clean tree.)

Typecheck clean for packages/contracts, packages/client-runtime, apps/server, apps/web; lint and format checked on the changed files.

Verified in the app

Ran the web stack against an isolated --home-dir, planted a customized keybindings.json (remapped sidebar.toggle and terminal.toggle, plus a script.run-tests.run binding), and clicked the button:

  • confirm text: "Reset all keybindings to their defaults? / Every customization is discarded. Project script bindings are kept."
  • on disk afterwards: the non-script rules are byte-identical to DEFAULT_KEYBINDINGS, the remapped shortcuts are gone, and script.run-tests.run survives;
  • the table updates live, 42 bindings (41 defaults + the script rule).

Not included

The issue also floats migrations for defaults that change upstream. That is a separate mechanism and is not part of this change; the startup sync still backfills newly added defaults as it did before.

🤖 Generated with Claude Code


Note

Medium Risk
Bulk-writes user keybinding config and overwrites malformed files on reset; behavior is intentional but can surprise users who relied on a broken on-disk file.

Overview
Adds reset all keybindings to defaults end-to-end: a new server.resetKeybindings RPC (orchestration operate scope) wired through contracts, client-runtime, and the Keybindings settings header.

Server-side resetKeybindingRulesToDefaults rewrites keybindings.json with DEFAULT_KEYBINDINGS, keeps script.* rules, updates cache, and broadcasts changes like upsert/remove. Malformed configs are replaced on reset (unlike upsert, which still refuses to overwrite). Unreadable configs fail without writing. If script rules exceed the cap after reset, defaults are never dropped—oldest script entries are trimmed and a warning is logged.

The settings UI adds a Reset all to defaults control with confirmation (notes that script bindings stay), guards against environment changes while the dialog is open, and surfaces errors via toasts.

Tests cover happy path, malformed replacement, read failure, and max-count script truncation.

Reviewed by Cursor Bugbot for commit 1e832d8. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Add a reset-all button to keybindings settings

  • Adds a "Reset All" toolbar button in KeybindingsSettings.tsx that prompts for confirmation before resetting keybindings to defaults.
  • Implements resetKeybindingRulesToDefaults in keybindings.ts: writes defaults plus any existing script bindings, updates the in-memory cache, and broadcasts a change event.
  • Exposes a new server.resetKeybindings WebSocket RPC in ws.ts, gated by orchestration operate scope.
  • Script bindings are preserved up to MAX_KEYBINDINGS_COUNT; excess script rules are dropped (trailing rules survive) with a warning logged.
  • Risk: malformed keybinding configs are silently overwritten on reset; configs that cannot be read (e.g. a directory at the path) return a failure instead.

Macroscope summarized 1e832d8.

There is no way to get back to the stock keybindings from the GUI. A user
carrying customizations from an older version can end up with conflicting
bindings and no recourse short of hand-editing keybindings.json — which the
settings screen already links to, but which is exactly what someone hitting
this problem does not want to do.

Adds `server.resetKeybindings`, which rewrites the config with the default
rule set, and a reset button beside Add keybinding and Open keybindings.json.
The action confirms first, since it discards every customization at once.

Project script bindings (`script.*`) are kept. They have no default to
restore, so dropping them would delete shortcuts the reset cannot give back.
A config that fails to parse is replaced outright rather than erroring — that
is the state the reset exists to escape.

Closes pingdotgg#4576

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 41c0b958-619c-43ee-883f-08614f876524

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 27, 2026
Comment thread apps/server/src/keybindings.ts Outdated
Comment thread apps/web/src/components/settings/KeybindingsSettings.tsx
Comment thread apps/server/src/keybindings.ts
Comment thread apps/server/src/keybindings.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces a new user-facing feature (reset-all keybindings button) with a new RPC endpoint, backend logic, and UI components. New features introducing user-facing behavior warrant human review even when well-tested and self-contained.

You can customize Macroscope's approvability policy. Learn more.

…ax cap

Three review findings, all real:

- The reset swallowed every load failure, not just parse failures. A
  transient read error would resolve to "no rules" and the write would then
  drop the user's script bindings. It now loads through the runtime loader,
  which returns an empty rule set for a config it cannot parse but still
  fails on a filesystem error, so an unreadable config aborts the reset
  instead of overwriting it.

- Capping with slice(-MAX) kept the trailing script rules and dropped default
  rules from the front, so a reset could fail to restore the defaults it
  exists to restore. Truncate the preserved scripts instead, and log what was
  dropped.

- The web handler captured the primary environment id before awaiting the
  confirm dialog. Switching environments while the dialog was open would
  reset the wrong one. It now re-checks the current environment after the
  dialog resolves.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@m-de-graaff

Copy link
Copy Markdown
Contributor Author

All three findings were correct. Fixed in 90f7248.

Effect.result swallowed filesystem errors (flagged by both bots) — the worst of the three. loadWritableCustomKeybindingsConfig fails for fs.exists, fs.readFileString, and parse errors, and I collapsed all of them to []. A transient read error would have written defaults over a config whose script.* bindings were still on disk — the exact data loss the preserve logic exists to prevent.

The reset now goes through loadRuntimeCustomKeybindingsConfig, which already draws that line correctly: an unparseable config yields an empty rule set (with issues), while a read failure propagates. So a malformed config is still replaced — that is the point of the button — but an unreadable one aborts the reset instead of overwriting it.

The max-count cap dropped defaults, not scriptsslice(-MAX) keeps the tail, so with enough script bindings the reset would fail to restore the defaults it exists to restore. Now the preserved scripts are truncated to MAX_KEYBINDINGS_COUNT - DEFAULT_KEYBINDINGS.length and the drop is logged.

Stale environment id across the confirm dialog — the handler captured primaryEnvironment.environmentId at render and awaited a native modal before sending. It now re-checks the current primary environment after the dialog resolves and bails if it changed.

Two tests added:

  • refuses to reset a keybindings config it cannot read — a directory at the config path makes the read fail the way a transient filesystem error would; the reset fails and the path is untouched.
  • keeps every default when preserved script bindings exceed the max count — 220 script rules in, all 41 defaults still present and the file capped at 256.

vp test run src/keybindings.test.ts — 25 passed. (fails when config directory is not writable still fails on this Windows host with and without the change; fs.chmod does not restrict directories there.) Typecheck, lint, and format clean for apps/server and apps/web.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 90f7248. Configure here.

Comment thread apps/server/src/keybindings.ts Outdated
// Truncate the preserved scripts, never the defaults — a reset that
// dropped default rules to stay under the cap would defeat itself.
const scriptBudget = Math.max(0, MAX_KEYBINDINGS_COUNT - DEFAULT_KEYBINDINGS.length);
const cappedConfig = [...DEFAULT_KEYBINDINGS, ...preservedScripts.slice(0, scriptBudget)];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Script truncation drops highest-precedence rules

Low Severity

The resetKeybindingRulesToDefaults function truncates script.* keybindings by keeping the earliest entries (slice(0, budget)). This conflicts with the system's keybinding resolution, which prioritizes later rules, potentially discarding effective script shortcuts for older, overridden ones.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 90f7248. Configure here.

if (!confirmed) return;
// The primary environment can change while the dialog is open, and this
// discards every customization — target only what the user was shown.
if (primaryEnvironmentRef.current?.environmentId !== environmentId) return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Silent reset abort after confirm

Low Severity

After the user confirms reset, if primaryEnvironment changes during the async confirm dialog, the handler returns without calling the RPC and without a toast. environmentId from click time is already captured, so the confirmed reset is abandoned with no feedback.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 90f7248. Configure here.

…borted reset

Two follow-up review findings:

- Script truncation kept the earliest rules, but later rules have higher
  precedence throughout this file, so the cap could discard the script
  bindings actually in effect and keep the ones they shadow. Drop from the
  front instead, and pin the direction in the test.

- The environment guard added after the confirm dialog returned silently.
  A user who confirmed a reset that then did not run had no way to tell.
  Toast instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@m-de-graaff

Copy link
Copy Markdown
Contributor Author

Both follow-up findings were correct. Fixed in 327f00c.

Script truncation dropped the wrong end. I capped with slice(0, budget), keeping the earliest scripts — but later rules have higher precedence everywhere else in this file (mergeWithDefaultKeybindings and syncDefaultKeybindingsOnStartup both keep the tail, and say so). So the cap could discard the script bindings actually in effect and keep the ones they shadow. Now drops from the front.

The existing cap test passed either way, which is why it did not catch this. It now asserts the surviving scripts are exactly the trailing ones, so the direction is pinned.

Silent abort after confirm. The environment guard I added in the previous round returned without feedback, so a user who confirmed a reset that then did not run had no way to tell. It now toasts "Keybindings were not reset — the active environment changed while the confirmation was open."

vp test run src/keybindings.test.ts — 25 passed, same single pre-existing Windows failure (fails when config directory is not writable; fs.chmod does not restrict directories on this host, and it fails identically on a clean tree). Typecheck, lint, and format clean for apps/server and apps/web.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing as part of the open-PR backlog sweep (wave 1).

Reason: Jul conflicted keybinding reset-all; settings pages were unified later (#9281).
Related: #9281

Reopen if this is still wanted and you’re willing to rebase onto current main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: reset keybinds to default

2 participants