feat: make CodeEditor toggle-comment shortcut configurable#7896
feat: make CodeEditor toggle-comment shortcut configurable#7896jespermeller wants to merge 2 commits intousebruno:mainfrom
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jespermeller
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe CodeEditor comment-toggle shortcut is made dynamic: a new utility resolves platform-aware CodeMirror key strings from Redux hotkey preferences, and CodeEditor is connected to Redux to receive and apply the computed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CodeMirror
participant CodeEditor
participant Redux
User->>CodeMirror: press shortcut
CodeMirror->>CodeEditor: invoke bound key handler
CodeEditor->>Redux: read commentToggleKey (injected prop)
Redux-->>CodeEditor: returns resolved CodeMirror key string
CodeEditor-->>CodeMirror: toggleComment using JSON/LD+JSON tokens
CodeMirror-->>User: comments toggled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
Adds a configurable keybinding for “Toggle Comment” in the CodeEditor, so users (e.g. with Nordic/ISO layouts) can customize the shortcut via Preferences instead of relying on a hard-coded Cmd-/ / Ctrl-/.
Changes:
- Added a new Editor keybinding section with a
toggleCommentaction. - Introduced
getCodeMirrorKeyForActionto translate stored keybinding format into a CodeMirrorextraKeyskey string. - Wired
CodeEditorto Redux preferences to apply the configured toggle-comment shortcut.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/bruno-app/src/providers/Hotkeys/keyMappings.js | Adds toggleComment default binding and a helper to convert stored keybindings to CodeMirror key strings. |
| packages/bruno-app/src/components/CodeEditor/index.js | Switches CodeEditor export to a connected component and uses the configured key for toggleComment in CodeMirror extraKeys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/components/CodeEditor/index.js (1)
92-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComment-toggle hotkey does not react to preference changes after mount.
extraKeysis initialized once (Line 92 onward), butcommentToggleKeyupdates are never reconciled incomponentDidUpdate(Line 223 onward). If the user changes keybindings while this editor instance is mounted, the old shortcut remains active.Proposed fix
class CodeEditor extends React.Component { + toggleComment = () => { + if (['application/ld+json', 'application/json'].includes(this.props.mode)) { + this.editor.toggleComment({ lineComment: '//', blockComment: '/*' }); + } else { + this.editor.toggleComment(); + } + }; componentDidMount() { @@ - const toggleComment = () => { - if (['application/ld+json', 'application/json'].includes(this.props.mode)) { - this.editor.toggleComment({ lineComment: '//', blockComment: '/*' }); - } else { - this.editor.toggleComment(); - } - }; @@ - ...(this.props.commentToggleKey ? { [this.props.commentToggleKey]: toggleComment } : {}), + ...(this.props.commentToggleKey ? { [this.props.commentToggleKey]: this.toggleComment } : {}), @@ componentDidUpdate(prevProps) { @@ + if (this.props.commentToggleKey !== prevProps.commentToggleKey && this.editor) { + const extraKeys = { ...(this.editor.getOption('extraKeys') || {}) }; + if (prevProps.commentToggleKey) { + delete extraKeys[prevProps.commentToggleKey]; + } + if (this.props.commentToggleKey) { + extraKeys[this.props.commentToggleKey] = this.toggleComment; + } + this.editor.setOption('extraKeys', extraKeys); + } + this.ignoreChangeEvent = false; }Also applies to: 223-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/CodeEditor/index.js` around lines 92 - 124, The CodeEditor mounts extraKeys once (extraKeys) so changes to props.commentToggleKey aren’t applied—update componentDidUpdate to detect when this.props.commentToggleKey != prevProps.commentToggleKey and reconcile the editor keymap by removing the old mapping and adding the new one (or rebuild and call editor.setOption('extraKeys', updatedExtraKeys)); specifically update the CodeEditor lifecycle method componentDidUpdate to modify the CodeMirror instance (editor) to point the new key to toggleComment (and remove the previous key) so the shortcut reflects preference changes while mounted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/providers/Hotkeys/keyMappings.js`:
- Around line 184-189: getCodeMirrorKeyForAction currently assumes (isMac ?
binding.mac : binding.windows) is always a string and calls .split(), which can
throw if binding.mac/windows is undefined; update the function to guard this
value before splitting (e.g. compute const raw = isMac ? binding.mac :
binding.windows and if (!raw) return null or use a safe default like '') then
call raw.split('+bind+').filter(Boolean); refer to getCodeMirrorKeyForAction,
getMergedKeyBindings, binding and isMac to locate the change and ensure
consumers don't break on malformed persisted preferences.
---
Outside diff comments:
In `@packages/bruno-app/src/components/CodeEditor/index.js`:
- Around line 92-124: The CodeEditor mounts extraKeys once (extraKeys) so
changes to props.commentToggleKey aren’t applied—update componentDidUpdate to
detect when this.props.commentToggleKey != prevProps.commentToggleKey and
reconcile the editor keymap by removing the old mapping and adding the new one
(or rebuild and call editor.setOption('extraKeys', updatedExtraKeys));
specifically update the CodeEditor lifecycle method componentDidUpdate to modify
the CodeMirror instance (editor) to point the new key to toggleComment (and
remove the previous key) so the shortcut reflects preference changes while
mounted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 292f7028-d442-499b-a9a3-2c4c05918e26
📒 Files selected for processing (2)
packages/bruno-app/src/components/CodeEditor/index.jspackages/bruno-app/src/providers/Hotkeys/keyMappings.js
- Reconcile editor's extraKeys when commentToggleKey changes after mount - Guard getCodeMirrorKeyForAction against missing platform binding - Normalize special-key tokens (enter, esc, arrow*, etc.) to CodeMirror names Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const getCodeMirrorKeyForAction = (action, userKeyBindings) => { | ||
| const binding = getMergedKeyBindings(userKeyBindings)[action]; | ||
| if (!binding) return null; | ||
| const isMac = navigator.platform.toLowerCase().includes('mac'); |
There was a problem hiding this comment.
getCodeMirrorKeyForAction uses navigator.platform directly. This duplicates OS detection logic used elsewhere (utils/common/platform.isMacOS) and will throw if navigator is unavailable (e.g., certain test/runtime contexts). Consider switching to isMacOS() (or at least guarding with typeof navigator !== 'undefined') so this helper is safe and consistent across the app.
| const isMac = navigator.platform.toLowerCase().includes('mac'); | |
| const platform = typeof navigator !== 'undefined' && typeof navigator.platform === 'string' ? navigator.platform : ''; | |
| const isMac = platform.toLowerCase().includes('mac'); |
| const mapStateToProps = (state) => ({ | ||
| commentToggleKey: getCodeMirrorKeyForAction('toggleComment', state.app.preferences?.keyBindings) | ||
| }); | ||
|
|
||
| export default connect(mapStateToProps, null, null, { forwardRef: true })(CodeEditor); |
There was a problem hiding this comment.
CodeEditor is now wrapped with react-redux connect, which means it cannot be rendered outside a Redux <Provider>. There is an existing Jest test that renders <CodeEditor /> without a store; this change will break that test (and any other standalone usage). Consider exporting the unconnected class (named export) for tests, or updating tests/consumers to wrap in a Provider with a minimal store.
| const mapStateToProps = (state) => ({ | ||
| commentToggleKey: getCodeMirrorKeyForAction('toggleComment', state.app.preferences?.keyBindings) | ||
| }); | ||
|
|
There was a problem hiding this comment.
The app has a preferences.keybindingsEnabled toggle (used by HotkeysProvider/useKeybinding to disable shortcuts), but CodeEditor will still bind the comment-toggle shortcut even when keybindings are disabled. Since toggleComment is now part of KEY_BINDING_SECTIONS, consider also selecting keybindingsEnabled here and returning null for commentToggleKey when disabled so the preference toggle consistently disables this shortcut too.
| const mapStateToProps = (state) => ({ | |
| commentToggleKey: getCodeMirrorKeyForAction('toggleComment', state.app.preferences?.keyBindings) | |
| }); | |
| const mapStateToProps = (state) => { | |
| const keybindingsEnabled = state.app.preferences?.keybindingsEnabled; | |
| return { | |
| commentToggleKey: keybindingsEnabled === false | |
| ? null | |
| : getCodeMirrorKeyForAction('toggleComment', state.app.preferences?.keyBindings) | |
| }; | |
| }; |
Fixes: #7874
Heavily build with AI, but i ran it and it worked. I could change keybindings and use it in the json editor.
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit