-
Notifications
You must be signed in to change notification settings - Fork 30
Added Mac support to trigger extended keyboard shortcuts #9161
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.
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
Conversation
📝 WalkthroughWalkthroughThe changes add macOS support for extended command key shortcuts by exporting a platform detection constant ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/libs/input.tsfrontend/javascripts/viewer/constants.tsunreleased_changes/9161.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-11T15:25:53.526Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 9117
File: frontend/javascripts/admin/statistic/time_tracking_overview.tsx:261-279
Timestamp: 2025-12-11T15:25:53.526Z
Learning: Ant Design v6 Select: when using the Select component, consider supplying a prefix prop to render an icon or element before the input for better visual context. Apply this guideline to TS and TSX files across the codebase where Ant Design Select is used; ensure prefix usage is accessible (e.g., provide meaningful aria-label if needed) and avoid unnecessary prefixes on simple inputs.
Applied to files:
frontend/javascripts/viewer/constants.tsfrontend/javascripts/libs/input.ts
📚 Learning: 2025-09-04T10:01:56.727Z
Learnt from: knollengewaechs
Repo: scalableminds/webknossos PR: 8895
File: frontend/javascripts/viewer/view/action_bar_view.tsx:0-0
Timestamp: 2025-09-04T10:01:56.727Z
Learning: In frontend/javascripts/viewer/view/action_bar_view.tsx, knollengewaechs prefers not to use onKeyUpCapture for preventing modal close on modifier keys because: 1) holding ctrl while opening modal will still close on keyup, 2) normal case of opening modal then pressing ctrl doesn't cause issues with just onKeyDownCapture.
Applied to files:
frontend/javascripts/libs/input.ts
🧬 Code graph analysis (1)
frontend/javascripts/libs/input.ts (1)
frontend/javascripts/viewer/constants.ts (1)
isMac(395-407)
🪛 LanguageTool
unreleased_changes/9161.md
[uncategorized] ~2-~2: The operating system from Apple is written “macOS”.
Context: ### Added - Added MacOS supported trigger extended keyboard sho...
(MAC_OS)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
- GitHub Check: frontend-tests
🔇 Additional comments (2)
frontend/javascripts/libs/input.ts (1)
10-10: LGTM! Platform-specific keyboard binding implemented correctly.The import and dynamic binding properly enable CMD+K on macOS while maintaining CTRL+K on other platforms. The existing
preventBrowserSearchbarShortcutmethod (lines 132-137) already handles bothctrlKeyandmetaKey, which complements this change nicely.Also applies to: 72-72
frontend/javascripts/viewer/constants.ts (1)
395-407: LGTM! Platform detection constant properly exported.Exporting
isMacenables external modules to implement platform-specific logic. The implementation is defensive with proper error handling, and the comments appropriately document the deprecated status ofnavigator.platformwhile explaining why it's still used.
| @@ -0,0 +1,2 @@ | |||
| ### Added | |||
| - Added MacOS supported trigger extended keyboard shortcuts with CMD + K | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix capitalization and grammar.
The operating system should be "macOS" (official Apple spelling), and the grammar needs adjustment.
🔎 Suggested fix
-### Added
-- Added MacOS supported trigger extended keyboard shortcuts with CMD + K
+### Added
+- Added macOS support to trigger extended keyboard shortcuts with CMD + K📝 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.
| - Added MacOS supported trigger extended keyboard shortcuts with CMD + K | |
| - Added macOS support to trigger extended keyboard shortcuts with CMD + K |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~2-~2: The operating system from Apple is written “macOS”.
Context: ### Added - Added MacOS supported trigger extended keyboard sho...
(MAC_OS)
🤖 Prompt for AI Agents
In unreleased_changes/9161.md around line 2, fix capitalization and grammar:
change "Added MacOS supported trigger extended keyboard shortcuts with CMD + K"
to proper macOS styling and clearer phrasing—e.g., "Added macOS-supported
trigger for extended keyboard shortcuts using ⌘+K (Command+K)". Ensure "macOS"
is lowercase 'm' and include either the ⌘ symbol or "Command" for clarity.
A small PR to add Mac support to trigger extended keyboard shortcut. In other words, CMD + K instead of CTRL + K. The Mac shortcut is already documented in our docs but was not implemented until now.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)