Skip to content

fix: preserve literal plus keybindings - #4983

Merged
ogulcancelik merged 1 commit into
herdrdev:masterfrom
minatoaquaMK2:issue/4960-keybinding-roundtrip
Oct 8, 2026
Merged

ogulcancelik merged 1 commit into
herdrdev:masterfrom
minatoaquaMK2:issue/4960-keybinding-roundtrip

Conversation

@minatoaquaMK2

Copy link
Copy Markdown
Contributor

prefix+plus passes configuration validation, but the server serializes its label as prefix++. The client reparses that label when applying the command manifest and drops the binding. Plus-based prefix keys encounter the same mismatch in endpoint keybinding profiles.

Teach the shared key-combo parser to accept literal + and modifier-prefixed forms such as ctrl++, while continuing to reject missing keys, extra separators, and unknown modifiers. This restores the existing formatter/parser contract without changing labels, command IDs, or the wire protocol.

Validation on macOS arm64:

  • Reproduced the dropped command with a failing client snapshot regression before the fix.
  • Regression coverage exercises the real server command manifest, local and endpoint keybinding sources, configuration reloads, and legacy/enhanced plus-key input. Additional tests cover prefix-profile serialization and supported key/modifier label round trips.
  • All native stages of just check passed: formatting, clippy, 3,794 Rust tests, and maintenance/integration-asset tests. just docs-contract-test passed separately.
  • just check stops at Windows cross lint because this machine has no Windows SDK. The PR's native Windows CI must pass before this is ready.

No documentation change is needed: this restores already-supported bindings.

refs #4960

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9441b737-c9e9-452f-b7eb-e0d0ecb9252b
📥 Commits

Reviewing files that changed from the base of the PR and between 3d9d2b1 and f76c0ef.

📒 Files selected for processing (3)
  • src/client/shell/tests/keybindings_settings.rs
  • src/config.rs
  • src/config/keybinds.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

parse_key_combo now accepts a bare + and modified-plus forms such as ctrl++. Tests cover key-label round-tripping, malformed inputs, configuration reloads, and command invocation for multiple terminal representations.

Suggested reviewers: ogulcancelik

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to f76c0

No actionable issue remains identified for the literal-plus keybinding change. Complete the normal checks, including native Windows CI, before merging.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to f76c0

The change restores supported plus-key bindings without expanding command permissions or transfer destinations. Malformed bindings remain rejected, and command identity stays separate from key-label syntax.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The effective change reaches keybindings supplied through local configuration and endpoint profiles, including the existing remote-image-paste trigger. It adds alternate syntax for an existing key rather than a new command authority or destination-selection channel.

Security Findings and Attack Paths

  • inferred — No introduced command-injection or authority-bypass path was identified in the examined consumers. Parsed labels yield key tuples, not executable definitions; invocation still resolves an endpoint-owned command ID and rejects stale or unknown IDs.

Trust Boundaries and Controls

  • observed — Remote image paste retains its existing remote-client and active-surface checks and routes to the selected endpoint target. Accepting a literal-plus trigger does not remove these predicates; profile control over this trigger predates the PR.

Resilience and Maintainability Implications

  • observed — Snapshot transition code is unchanged. Successful application replaces the active keymap; malformed endpoint profiles retain the prior keymap and record an error while the snapshot is committed. Lower revisions for the same boot and generation are ignored, and stale command IDs remain rejected. These existing recovery semantics are not a new PR concern.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preserving literal plus keybindings.
Description check ✅ Passed The description explains the parser mismatch, the fix, and the regression coverage. It is directly related to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Fixes keybinding parser to handle literal plus characters.

The PR appears safe to merge based on the changed parser and the reviewed binding paths.

Summary

The PR restores parsing of literal-plus key labels emitted by the existing formatter, so plus bindings survive command manifests and keybinding profiles.

  • Adds client snapshot, profile round-trip, parser, and input-matching regression coverage.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Configured plus binding] --> B[Server formats key label]
  B --> C[Command manifest or keybinding profile]
  C --> D[Client parses literal-plus label]
  D --> E[Binding matches plus input]
Loading

Reviews (1) · Last reviewed commit: "fix: preserve literal plus keybindings"

@ogulcancelik
ogulcancelik merged commit 4f89611 into herdrdev:master Oct 8, 2026
8 checks passed
@ogulcancelik

Copy link
Copy Markdown
Collaborator

ty

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants