Add global ⌘+I/Ctrl+I shortcut to open new issue modal #7939
Conversation
- Add ⌘+I (macOS) and Ctrl+I (Windows/Linux) global shortcut - Implement useGlobalHotkeys hook with focus context awareness - Add keyboard shortcut constants and utilities - Integrate with existing command palette and quick actions - Add comprehensive unit and integration tests - Include tooltip display for keyboard shortcuts - Prevent conflicts with text editing contexts Resolves: Global keyboard shortcut for new issue modal
- Add detailed description with all key features - Include type of change checkboxes - Add screenshots and media section - Complete test scenarios with results - Add references to specification files - Follow GitHub PR template requirements
|
|
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 146 files out of 298 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a global keyboard shortcut (⌘/Ctrl+I) to open a new-issue modal: new constants, types, utilities, a throttled useGlobalHotkeys hook, provider/integration/test components, command-palette shortcuts and tooltips, inline layout listeners, store context, tests, PR template, and Vercel/mock API configs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Doc as Document/Window
participant Hook as useGlobalHotkeys
participant Utils as custom-events
participant App as CommandPalette/Modal
Note over User,App: Global new-issue shortcut flow (⌘/Ctrl+I)
User->>Doc: keydown (I + Cmd/Ctrl)
Doc->>Hook: throttled listener invoked
Hook->>Hook: shouldIgnoreKeyboardShortcut()? (focus check)
alt not editable
Hook->>Utils: createKeyboardEventDetail()
Utils-->>Hook: event detail
Hook->>Doc: dispatch CustomEvent "plane:open-new-issue"
else editable
Hook-->>Doc: ignore event
end
Doc-->>App: CustomEvent "plane:open-new-issue"
App->>App: verify permissions & modal state
alt allowed & not open
App->>App: open create-issue modal
else no-op / log
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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.
Pull Request Overview
This PR implements a global keyboard shortcut (⌘+I on macOS, Ctrl+I on Windows/Linux) to open the new issue modal from anywhere in the application. The feature includes comprehensive focus management, cross-platform compatibility, and integration with existing UI components.
Key changes include:
- Global keyboard shortcut hook with focus context awareness
- Custom event system for modal triggering
- Enhanced tooltips showing keyboard shortcuts on UI buttons
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
apps/web/core/hooks/use-global-hotkeys.tsx |
Main hook implementation for global keyboard event handling |
apps/web/core/utils/*.ts |
Supporting utilities for focus detection, custom events, and throttling |
apps/web/core/types/keyboard.ts |
TypeScript type definitions for keyboard functionality |
apps/web/core/constants/keyboard-shortcuts.ts |
Centralized keyboard shortcut configuration constants |
apps/web/components/keyboard-shortcut-*.tsx |
React components for keyboard shortcut integration and testing |
apps/web/core/components/command-palette/command-palette.tsx |
Enhanced command palette with multiple keyboard event handlers |
apps/web/core/components/issues/issue-layouts/quick-add/button/*.tsx |
Added keyboard shortcut tooltips to quick add buttons |
apps/web/core/components/workspace/sidebar/quick-actions.tsx |
Added tooltip with keyboard shortcut to sidebar quick actions |
apps/web/app/layout.tsx |
Added global script for keyboard event handling |
apps/web/tests/unit/*.test.ts |
Unit tests for utilities and hooks |
apps/web/tests/integration/*.test.tsx |
Integration tests for keyboard shortcuts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| type: 'plane:open-new-issue' as const, | ||
| detail: { | ||
| source: 'keyboard' as const, | ||
| keyCombination: event.metaKey ? 'meta+i' as const : 'ctrl+i' as const, |
There was a problem hiding this comment.
The key combination should use 'n' not 'i' to match the NEW_ISSUE key constant ('i') and the actual shortcut being implemented. This creates inconsistency between the event detail and the actual key being pressed.
| keyCombination: event.metaKey ? 'meta+i' as const : 'ctrl+i' as const, | |
| keyCombination: event.metaKey | |
| ? (`meta+${KEYBOARD_SHORTCUTS.NEW_ISSUE.key}` as const) | |
| : (`ctrl+${KEYBOARD_SHORTCUTS.NEW_ISSUE.key}` as const), |
Copilot uses AI. Check for mistakes.
| */ | ||
| export function createKeyboardEventDetail(): KeyboardEventDetail { | ||
| const focusContext = getFocusContext(); | ||
| const keyCombination = isMacOS() ? 'meta+n' : 'ctrl+n'; |
There was a problem hiding this comment.
The key combination should use 'meta+i' and 'ctrl+i' to match the actual keyboard shortcut being implemented (⌘+I/Ctrl+I), not 'meta+n' and 'ctrl+n'.
| const keyCombination = isMacOS() ? 'meta+n' : 'ctrl+n'; | |
| const keyCombination = isMacOS() ? 'meta+i' : 'ctrl+i'; |
Copilot uses AI. Check for mistakes.
| */ | ||
| export function getKeyboardShortcutDisplay(): string { | ||
| const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0; | ||
| return isMac ? '⌘+N' : 'Ctrl+N'; |
There was a problem hiding this comment.
The display text should show '⌘+I' and 'Ctrl+I' to match the actual keyboard shortcut being implemented, not '⌘+N' and 'Ctrl+N'.
| return isMac ? '⌘+N' : 'Ctrl+N'; | |
| return isMac ? '⌘+I' : 'Ctrl+I'; |
Copilot uses AI. Check for mistakes.
|
|
||
| return { | ||
| isListening, | ||
| supportedKeys: ['⌘+I', 'Ctrl+I'], |
There was a problem hiding this comment.
The supportedKeys array should match the actual implementation. Based on the hook logic and constants, this should use the NEW_ISSUE key configuration rather than hardcoded values that may become inconsistent.
| supportedKeys: ['⌘+I', 'Ctrl+I'], | |
| supportedKeys: [ | |
| `⌘+${KEYBOARD_SHORTCUTS.NEW_ISSUE.key.toUpperCase()}`, | |
| `Ctrl+${KEYBOARD_SHORTCUTS.NEW_ISSUE.key.toUpperCase()}` | |
| ], |
Copilot uses AI. Check for mistakes.
| expect(result.current.supportedKeys).toContain('⌘+N'); | ||
| expect(result.current.supportedKeys).toContain('Ctrl+N'); |
There was a problem hiding this comment.
The test expects '⌘+N' and 'Ctrl+N' but the actual implementation returns '⌘+I' and 'Ctrl+I'. This test will fail due to the mismatch.
| expect(result.current.supportedKeys).toContain('⌘+N'); | |
| expect(result.current.supportedKeys).toContain('Ctrl+N'); | |
| expect(result.current.supportedKeys).toContain('⌘+I'); | |
| expect(result.current.supportedKeys).toContain('Ctrl+I'); |
Copilot uses AI. Check for mistakes.
| // Debug: Log all Cmd+I events | ||
| if (cmdClicked && keyPressed === "i") { | ||
| console.log("🎯 Command palette: Raw Cmd+I event detected", { | ||
| key, | ||
| metaKey, | ||
| ctrlKey, | ||
| target: e.target, | ||
| isAnyModalOpen | ||
| }); | ||
| } | ||
|
|
||
| // Debug: Log all Cmd+J events to see why it's working | ||
| if (cmdClicked && keyPressed === "j") { | ||
| console.log("🎯 Command palette: Raw Cmd+J event detected", { | ||
| key, | ||
| metaKey, | ||
| ctrlKey, | ||
| target: e.target, | ||
| isAnyModalOpen, | ||
| globalShortcuts: Object.keys(shortcutsList.global), | ||
| workspaceShortcuts: Object.keys(shortcutsList.workspace), | ||
| projectShortcuts: Object.keys(shortcutsList.project) | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Debug console.log statements should be removed before production. These extensive debug logs (lines 171-447) will clutter the console and impact performance.
| // Debug: Log all Cmd+I events | |
| if (cmdClicked && keyPressed === "i") { | |
| console.log("🎯 Command palette: Raw Cmd+I event detected", { | |
| key, | |
| metaKey, | |
| ctrlKey, | |
| target: e.target, | |
| isAnyModalOpen | |
| }); | |
| } | |
| // Debug: Log all Cmd+J events to see why it's working | |
| if (cmdClicked && keyPressed === "j") { | |
| console.log("🎯 Command palette: Raw Cmd+J event detected", { | |
| key, | |
| metaKey, | |
| ctrlKey, | |
| target: e.target, | |
| isAnyModalOpen, | |
| globalShortcuts: Object.keys(shortcutsList.global), | |
| workspaceShortcuts: Object.keys(shortcutsList.workspace), | |
| projectShortcuts: Object.keys(shortcutsList.project) | |
| }); | |
| } | |
Copilot uses AI. Check for mistakes.
| <script | ||
| dangerouslySetInnerHTML={{ | ||
| __html: ` | ||
| console.log('🎯 GLOBAL SCRIPT: Loading global Cmd+N handler'); |
There was a problem hiding this comment.
The inline script with dangerouslySetInnerHTML contains extensive debug logging and should be removed or moved to a proper external script file. This approach also poses security risks and makes the code harder to maintain.
Copilot uses AI. Check for mistakes.
| /** | ||
| * Test component for keyboard shortcut functionality | ||
| */ | ||
|
|
||
| 'use client'; | ||
|
|
||
| import { useState, useEffect } from 'react'; | ||
| import { useCommandPalette } from '../core/hooks/store/use-command-palette'; | ||
|
|
||
| export function KeyboardShortcutTest() { | ||
| const [testResults, setTestResults] = useState<string[]>([]); | ||
| const { isCreateIssueModalOpen, toggleCreateIssueModal } = useCommandPalette(); | ||
|
|
||
| useEffect(() => { | ||
| const addTestResult = (message: string) => { | ||
| setTestResults(prev => [...prev, `${new Date().toLocaleTimeString()}: ${message}`]); | ||
| }; | ||
|
|
||
| // Listen for keyboard events | ||
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| if (event.key === 'i' && (event.metaKey || event.ctrlKey)) { | ||
| addTestResult(`🎹 Keyboard shortcut detected: ${event.metaKey ? 'Cmd' : 'Ctrl'}+I`); | ||
| } | ||
| }; | ||
|
|
||
| // Listen for our custom event | ||
| const handleCustomEvent = () => { | ||
| addTestResult('🎯 Custom event received by test component'); | ||
| }; | ||
|
|
||
| // Listen for modal state changes | ||
| const checkModalState = () => { | ||
| addTestResult(`📱 Modal state: ${isCreateIssueModalOpen ? 'OPEN' : 'CLOSED'}`); | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', handleKeyDown); | ||
| window.addEventListener('plane:open-new-issue', handleCustomEvent); | ||
|
|
||
| // Check modal state periodically | ||
| const interval = setInterval(checkModalState, 5000); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| window.removeEventListener('plane:open-new-issue', handleCustomEvent); | ||
| clearInterval(interval); | ||
| }; | ||
| }, [isCreateIssueModalOpen]); | ||
|
|
||
| const clearResults = () => setTestResults([]); | ||
|
|
||
| return ( | ||
| <div style={{ | ||
| position: 'fixed', | ||
| top: '10px', | ||
| right: '10px', | ||
| background: 'white', | ||
| border: '1px solid #ccc', | ||
| padding: '10px', | ||
| borderRadius: '5px', | ||
| maxWidth: '300px', | ||
| maxHeight: '400px', | ||
| overflow: 'auto', | ||
| zIndex: 9999, | ||
| fontSize: '12px' | ||
| }}> | ||
| <h3>🎹 Keyboard Shortcut Test</h3> | ||
| <p>Try pressing <strong>Cmd+I</strong> (Mac) or <strong>Ctrl+I</strong> (Windows/Linux)</p> | ||
| <p>Modal State: <strong>{isCreateIssueModalOpen ? 'OPEN' : 'CLOSED'}</strong></p> | ||
| <button onClick={clearResults} style={{ marginBottom: '10px' }}>Clear Results</button> | ||
| <div style={{ borderTop: '1px solid #eee', paddingTop: '10px' }}> | ||
| <strong>Test Results:</strong> | ||
| {testResults.length === 0 ? ( | ||
| <p>No events detected yet...</p> | ||
| ) : ( | ||
| <ul style={{ margin: '5px 0', paddingLeft: '20px' }}> | ||
| {testResults.slice(-10).map((result, index) => ( | ||
| <li key={index}>{result}</li> | ||
| ))} | ||
| </ul> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test components should not be included in production code. This component appears to be for debugging purposes and should be moved to a development-only context or removed entirely.
| /** | |
| * Test component for keyboard shortcut functionality | |
| */ | |
| 'use client'; | |
| import { useState, useEffect } from 'react'; | |
| import { useCommandPalette } from '../core/hooks/store/use-command-palette'; | |
| export function KeyboardShortcutTest() { | |
| const [testResults, setTestResults] = useState<string[]>([]); | |
| const { isCreateIssueModalOpen, toggleCreateIssueModal } = useCommandPalette(); | |
| useEffect(() => { | |
| const addTestResult = (message: string) => { | |
| setTestResults(prev => [...prev, `${new Date().toLocaleTimeString()}: ${message}`]); | |
| }; | |
| // Listen for keyboard events | |
| const handleKeyDown = (event: KeyboardEvent) => { | |
| if (event.key === 'i' && (event.metaKey || event.ctrlKey)) { | |
| addTestResult(`🎹 Keyboard shortcut detected: ${event.metaKey ? 'Cmd' : 'Ctrl'}+I`); | |
| } | |
| }; | |
| // Listen for our custom event | |
| const handleCustomEvent = () => { | |
| addTestResult('🎯 Custom event received by test component'); | |
| }; | |
| // Listen for modal state changes | |
| const checkModalState = () => { | |
| addTestResult(`📱 Modal state: ${isCreateIssueModalOpen ? 'OPEN' : 'CLOSED'}`); | |
| }; | |
| document.addEventListener('keydown', handleKeyDown); | |
| window.addEventListener('plane:open-new-issue', handleCustomEvent); | |
| // Check modal state periodically | |
| const interval = setInterval(checkModalState, 5000); | |
| return () => { | |
| document.removeEventListener('keydown', handleKeyDown); | |
| window.removeEventListener('plane:open-new-issue', handleCustomEvent); | |
| clearInterval(interval); | |
| }; | |
| }, [isCreateIssueModalOpen]); | |
| const clearResults = () => setTestResults([]); | |
| return ( | |
| <div style={{ | |
| position: 'fixed', | |
| top: '10px', | |
| right: '10px', | |
| background: 'white', | |
| border: '1px solid #ccc', | |
| padding: '10px', | |
| borderRadius: '5px', | |
| maxWidth: '300px', | |
| maxHeight: '400px', | |
| overflow: 'auto', | |
| zIndex: 9999, | |
| fontSize: '12px' | |
| }}> | |
| <h3>🎹 Keyboard Shortcut Test</h3> | |
| <p>Try pressing <strong>Cmd+I</strong> (Mac) or <strong>Ctrl+I</strong> (Windows/Linux)</p> | |
| <p>Modal State: <strong>{isCreateIssueModalOpen ? 'OPEN' : 'CLOSED'}</strong></p> | |
| <button onClick={clearResults} style={{ marginBottom: '10px' }}>Clear Results</button> | |
| <div style={{ borderTop: '1px solid #eee', paddingTop: '10px' }}> | |
| <strong>Test Results:</strong> | |
| {testResults.length === 0 ? ( | |
| <p>No events detected yet...</p> | |
| ) : ( | |
| <ul style={{ margin: '5px 0', paddingLeft: '20px' }}> | |
| {testResults.slice(-10).map((result, index) => ( | |
| <li key={index}>{result}</li> | |
| ))} | |
| </ul> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| } |
Copilot uses AI. Check for mistakes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (6)
apps/web/tests/unit/hooks/use-global-hotkeys.test.tsx (1)
46-53: Implement keyboard event simulation for callback test.The test currently has a placeholder comment indicating that keyboard event simulation is needed. Consider implementing this test to properly verify the onShortcut callback behavior.
Example implementation:
it('should call onShortcut callback when provided', () => { const onShortcut = jest.fn(); renderHook(() => useGlobalHotkeys({ onShortcut })); // Simulate keyboard event const event = new KeyboardEvent('keydown', { key: 'i', metaKey: true, ctrlKey: false }); act(() => { document.dispatchEvent(event); }); expect(onShortcut).toHaveBeenCalled(); });apps/web/tests/integration/keyboard-shortcuts.test.tsx (2)
23-30: Complete keyboard event handling test.This test is a placeholder and doesn't actually verify keyboard event handling. Consider implementing proper event simulation and assertions.
Example implementation:
it('should handle keyboard events globally', () => { render(<TestComponent />); // Simulate Cmd+I on Mac or Ctrl+I on Windows const event = new KeyboardEvent('keydown', { key: 'i', metaKey: true, // or ctrlKey: true for Windows bubbles: true }); act(() => { document.dispatchEvent(event); }); // Add assertions to verify the event was handled // e.g., check that the custom event was dispatched });
32-39: Implement duplicate modal prevention test.The test currently uses a trivial placeholder assertion. Implement actual verification that multiple rapid shortcut presses don't open duplicate modals.
Example implementation:
it('should prevent duplicate modal opening', () => { const { container } = render(<TestComponent />); // Simulate multiple rapid keyboard events const event = new KeyboardEvent('keydown', { key: 'i', metaKey: true, bubbles: true }); act(() => { document.dispatchEvent(event); document.dispatchEvent(event); document.dispatchEvent(event); }); // Verify that the custom event was dispatched but throttled // or that modal opening was only triggered once });apps/web/core/utils/keyboard-shortcut-display.ts (1)
9-10: Consider extracting platform detection to reduce duplication.The platform detection logic (
navigator.platform.toUpperCase().indexOf('MAC') >= 0) is duplicated across multiple files (e.g.,custom-events.tsline 12). Consider extracting this to a shared utility.Create a shared platform utility file:
// apps/web/core/utils/platform.ts export function isMacOS(): boolean { return navigator.platform.toUpperCase().indexOf('MAC') >= 0; }Then update this file:
+import { isMacOS } from './platform'; + export function getKeyboardShortcutDisplay(): string { - const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0; - return isMac ? '⌘+I' : 'Ctrl+I'; + return isMacOS() ? '⌘+I' : 'Ctrl+I'; }apps/web/core/utils/focus-context.ts (1)
26-38: Consider including 'select' elements in editable checks.The current implementation checks for
input,textarea, andcontenteditableelements. While this covers most cases, you might want to consider whether<select>elements should also prevent keyboard shortcuts, as users interact with them via keyboard navigation.If needed, update
isEditableElement:export function isEditableElement(element: HTMLElement | null): boolean { if (!element) return false; const tagName = element.tagName.toLowerCase(); const contentEditable = element.getAttribute('contenteditable') === 'true'; return ( tagName === 'input' || tagName === 'textarea' || + tagName === 'select' || contentEditable ); }apps/web/tests/unit/utils/keyboard-events.test.ts (1)
37-47: Strengthen the test assertion.The test only verifies that properties exist but doesn't validate their values. When an input element is focused, you should also assert that
isEditableistrue,tagNameis'INPUT', etc.Apply this diff to strengthen the assertion:
it('should return focus context with correct properties', () => { const input = document.createElement('input'); input.focus(); const context = getFocusContext(); expect(context).toHaveProperty('activeElement'); - expect(context).toHaveProperty('tagName'); - expect(context).toHaveProperty('isEditable'); + expect(context.tagName).toBe('INPUT'); + expect(context.isEditable).toBe(true); expect(context).toHaveProperty('contentEditable'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
PULL_REQUEST_TEMPLATE.md(1 hunks)apps/web/app/layout.tsx(2 hunks)apps/web/ce/helpers/command-palette.ts(1 hunks)apps/web/components/keyboard-shortcut-modal-integration.tsx(1 hunks)apps/web/components/keyboard-shortcut-provider.tsx(1 hunks)apps/web/components/keyboard-shortcut-test.tsx(1 hunks)apps/web/core/components/command-palette/command-palette.tsx(3 hunks)apps/web/core/components/issues/issue-layouts/quick-add/button/gantt.tsx(1 hunks)apps/web/core/components/issues/issue-layouts/quick-add/button/kanban.tsx(1 hunks)apps/web/core/components/workspace/sidebar/quick-actions.tsx(2 hunks)apps/web/core/constants/keyboard-shortcuts.ts(1 hunks)apps/web/core/hooks/use-global-hotkeys.tsx(1 hunks)apps/web/core/lib/store-context.ts(1 hunks)apps/web/core/types/keyboard.ts(1 hunks)apps/web/core/utils/custom-events.ts(1 hunks)apps/web/core/utils/focus-context.ts(1 hunks)apps/web/core/utils/keyboard-shortcut-display.ts(1 hunks)apps/web/core/utils/throttle.ts(1 hunks)apps/web/tests/integration/keyboard-shortcuts.test.tsx(1 hunks)apps/web/tests/unit/hooks/use-global-hotkeys.test.tsx(1 hunks)apps/web/tests/unit/utils/keyboard-events.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
apps/web/tests/unit/utils/keyboard-events.test.ts (2)
apps/web/core/utils/focus-context.ts (3)
isEditableElement(10-21)getFocusContext(26-38)shouldIgnoreKeyboardShortcut(43-46)apps/web/core/utils/custom-events.ts (3)
isMacOS(11-13)createKeyboardEventDetail(18-31)dispatchNewIssueEvent(36-44)
apps/web/core/utils/custom-events.ts (2)
apps/web/core/types/keyboard.ts (2)
KeyboardEventDetail(5-13)PlaneOpenNewIssueEvent(15-18)apps/web/core/utils/focus-context.ts (1)
getFocusContext(26-38)
apps/web/core/hooks/use-global-hotkeys.tsx (5)
apps/web/core/types/keyboard.ts (3)
UseGlobalHotkeysOptions(20-24)UseGlobalHotkeysReturn(26-31)PlaneOpenNewIssueEvent(15-18)apps/web/core/constants/keyboard-shortcuts.ts (2)
KEYBOARD_CONFIG(13-16)KEYBOARD_SHORTCUTS(5-11)apps/web/core/utils/focus-context.ts (1)
shouldIgnoreKeyboardShortcut(43-46)apps/web/core/utils/custom-events.ts (1)
dispatchNewIssueEvent(36-44)apps/web/core/utils/throttle.ts (1)
createThrottledKeyboardHandler(26-31)
apps/web/components/keyboard-shortcut-provider.tsx (3)
apps/web/core/hooks/use-global-hotkeys.tsx (1)
useGlobalHotkeys(12-106)apps/web/components/keyboard-shortcut-modal-integration.tsx (1)
KeyboardShortcutModalIntegration(11-41)apps/web/components/keyboard-shortcut-test.tsx (1)
KeyboardShortcutTest(10-84)
apps/web/core/components/issues/issue-layouts/quick-add/button/kanban.tsx (2)
apps/web/core/components/issues/issue-layouts/quick-add/root.tsx (1)
TQuickAddIssueButton(31-34)apps/web/core/utils/keyboard-shortcut-display.ts (1)
getKeyboardShortcutTooltip(16-18)
apps/web/tests/integration/keyboard-shortcuts.test.tsx (1)
apps/web/core/hooks/use-global-hotkeys.tsx (1)
useGlobalHotkeys(12-106)
apps/web/core/components/issues/issue-layouts/quick-add/button/gantt.tsx (2)
apps/web/core/components/issues/issue-layouts/quick-add/root.tsx (1)
TQuickAddIssueButton(31-34)apps/web/core/utils/keyboard-shortcut-display.ts (1)
getKeyboardShortcutTooltip(16-18)
apps/web/core/components/command-palette/command-palette.tsx (1)
apps/web/core/store/issue/issue-details/root.store.ts (1)
isAnyModalOpen(227-238)
apps/web/tests/unit/hooks/use-global-hotkeys.test.tsx (1)
apps/web/core/hooks/use-global-hotkeys.tsx (1)
useGlobalHotkeys(12-106)
apps/web/components/keyboard-shortcut-modal-integration.tsx (1)
apps/web/core/constants/keyboard-shortcuts.ts (1)
CUSTOM_EVENTS(18-20)
apps/web/core/components/workspace/sidebar/quick-actions.tsx (2)
apps/web/core/utils/keyboard-shortcut-display.ts (1)
getKeyboardShortcutTooltip(16-18)packages/constants/src/event-tracker/core.ts (1)
SIDEBAR_TRACKER_ELEMENTS(392-395)
apps/web/core/utils/focus-context.ts (1)
apps/web/core/types/keyboard.ts (1)
FocusContext(33-38)
🪛 ast-grep (0.39.6)
apps/web/app/layout.tsx
[warning] 97-97: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
apps/web/app/layout.tsx
[error] 98-98: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (8)
apps/web/core/lib/store-context.ts (1)
7-7: No module-level mutable state detected; SSR isolation valid.
RootStore/CoreRootStore contain no static properties or top-level mutable declarations—fresh instance per request suffices.apps/web/ce/helpers/command-palette.ts (1)
116-119: LGTM!The alternative shortcut entry is correctly implemented with proper platform detection and clear description.
apps/web/core/components/issues/issue-layouts/quick-add/button/gantt.tsx (1)
14-25: LGTM!The tooltip integration is clean and provides good UX by showing the keyboard shortcut hint to users.
apps/web/core/components/issues/issue-layouts/quick-add/button/kanban.tsx (1)
13-21: LGTM!Consistent tooltip implementation that enhances discoverability of the keyboard shortcut feature.
apps/web/components/keyboard-shortcut-modal-integration.tsx (1)
17-37: LGTM on event listener pattern and cleanup.The event listener setup, cleanup in the return function, and dependency array are correctly implemented. The logic to prevent duplicate modal openings is sound.
apps/web/core/constants/keyboard-shortcuts.ts (1)
5-11: LGTM!The keyboard shortcut constant definition is clean and correctly specifies the 'i' key as intended by the PR summary (⌘+I / Ctrl+I).
apps/web/core/utils/focus-context.ts (1)
10-21: LGTM!The
isEditableElementfunction correctly identifies editable elements (input, textarea, contentEditable). The logic is sound with proper null checking.apps/web/core/utils/throttle.ts (1)
8-31: LGTM!The throttle implementation is correct and follows standard patterns. It executes the function immediately on the first invocation, then blocks subsequent calls for the specified duration. The
createThrottledKeyboardHandlerwrapper provides a convenient keyboard-specific interface.
|
|
||
| // local | ||
| import { AppProvider } from "./provider"; | ||
| import { KeyboardShortcutProvider } from "../components/keyboard-shortcut-provider"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import.
KeyboardShortcutProvider is imported but commented out on lines 86-95. Either enable the provider or remove the import to avoid unused code.
-import { KeyboardShortcutProvider } from "../components/keyboard-shortcut-provider";📝 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.
| import { KeyboardShortcutProvider } from "../components/keyboard-shortcut-provider"; |
🤖 Prompt for AI Agents
In apps/web/app/layout.tsx around line 14, the import for
KeyboardShortcutProvider is unused because the provider is commented out further
down (lines 86-95); remove the unused import line or re-enable the provider
block. If removing, delete the import statement and any related type references;
if re-enabling, uncomment and ensure the provider is correctly wrapped around
children and any required props are passed.
| {/* <KeyboardShortcutProvider> */} | ||
| <div | ||
| className={cn( | ||
| "h-screen w-full overflow-hidden bg-custom-background-100 relative flex flex-col", | ||
| "app-container" | ||
| )} | ||
| > | ||
| <main className="w-full h-full overflow-hidden relative">{children}</main> | ||
| </div> | ||
| {/* </KeyboardShortcutProvider> */} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Restore or remove the commented KeyboardShortcutProvider.
The KeyboardShortcutProvider wrapper is commented out, which suggests an incomplete implementation. Having provider code commented in production indicates either:
- The provider needs to be re-enabled
- The provider approach was abandoned in favor of the inline script
- This is temporary debug code
If the provider is the intended approach (which is more maintainable and testable), restore it and remove the inline script (lines 97-171). If the inline script is the final approach, remove the provider import and commented code.
Recommended: Restore the React-based provider approach:
<AppProvider>
- {/* <KeyboardShortcutProvider> */}
+ <KeyboardShortcutProvider>
<div
className={cn(
"h-screen w-full overflow-hidden bg-custom-background-100 relative flex flex-col",
"app-container"
)}
>
<main className="w-full h-full overflow-hidden relative">{children}</main>
</div>
- {/* </KeyboardShortcutProvider> */}
+ </KeyboardShortcutProvider>
</AppProvider>
- <script
- dangerouslySetInnerHTML={{
- __html: `
- console.log('🎯 GLOBAL SCRIPT: Loading global Cmd+N handler');
- ...
- `,
- }}
- />🏁 Script executed:
#!/bin/bash
echo "Occurrences of KeyboardShortcutProvider in layout.tsx:"
rg -n "KeyboardShortcutProvider" -C3 apps/web/app/layout.tsx
echo -e "\nImport statements for KeyboardShortcutProvider:"
rg -nP "import\s+.*KeyboardShortcutProvider" apps/web/app/layout.tsx
echo -e "\nOccurrences of dangerouslySetInnerHTML in layout.tsx:"
rg -n "dangerouslySetInnerHTML" -C3 apps/web/app/layout.tsxLength of output: 1437
🏁 Script executed:
#!/bin/bash
echo "Locating keyboard-shortcut-provider files:"
rg -l "keyboard-shortcut-provider" --glob '*.tsx' --glob '*.ts'
echo
for file in $(rg -l "keyboard-shortcut-provider" --glob '*.tsx' --glob '*.ts'); do
echo "----- $file -----"
sed -n '1,200p' "$file"
doneLength of output: 7914
🏁 Script executed:
#!/bin/bash
echo "Searching for any file named keyboard-shortcut-provider.* across the repo:"
fd --ignore-case --type f "keyboard-shortcut-provider" .
echo -e "\nIf found, show first 200 lines of each:"
for f in $(fd --ignore-case --type f "keyboard-shortcut-provider" .); do
echo "----- $f -----"
sed -n '1,200p' "$f"
doneLength of output: 1085
Remove or fully implement the KeyboardShortcutProvider wrapper
Delete the unused KeyboardShortcutProvider import and commented JSX in apps/web/app/layout.tsx if you’ll continue using the inline <script>. Otherwise, re-enable the provider and move all global‐shortcut logic (dispatching plane:open-new-issue events, etc.) into its onShortcut handler, then remove the inline script.
🤖 Prompt for AI Agents
In apps/web/app/layout.tsx around lines 86 to 95, the commented
KeyboardShortcutProvider wrapper and its import are inconsistent with the inline
<script> global-shortcut logic; either remove the unused import and the
commented JSX if you intend to keep the inline script, or re-enable the provider
and move all global shortcut dispatch/handling (e.g., plane:open-new-issue
events, key listeners) into the provider’s onShortcut handler and delete the
inline script; update imports accordingly and ensure no leftover commented JSX
remains.
| <script | ||
| dangerouslySetInnerHTML={{ | ||
| __html: ` | ||
| console.log('🎯 GLOBAL SCRIPT: Loading global Cmd+N handler'); | ||
| console.log('🎯 GLOBAL SCRIPT: Script is running at', new Date().toISOString()); | ||
| // Test if the script is running | ||
| window.testGlobalScript = function() { | ||
| console.log('🎯 GLOBAL SCRIPT: Test function called'); | ||
| return 'Global script is working'; | ||
| }; | ||
| // Multiple event listeners with different strategies | ||
| // Strategy 1: Cmd+I for creating new work items | ||
| document.addEventListener('keydown', function(e) { | ||
| if (e.key === 'i' && (e.metaKey || e.ctrlKey)) { | ||
| console.log('🎯 DOCUMENT: Cmd+I detected!'); | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| e.stopImmediatePropagation(); | ||
| window.dispatchEvent(new CustomEvent('plane:open-new-issue', { | ||
| detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey } | ||
| })); | ||
| } | ||
| }, { capture: true, passive: false }); | ||
| // Strategy 2: Window level with capture - Cmd+I | ||
| window.addEventListener('keydown', function(e) { | ||
| if (e.key === 'i' && (e.metaKey || e.ctrlKey)) { | ||
| console.log('🎯 WINDOW: Cmd+I detected!', { | ||
| key: e.key, | ||
| metaKey: e.metaKey, | ||
| ctrlKey: e.ctrlKey, | ||
| target: e.target?.tagName, | ||
| timestamp: Date.now() | ||
| }); | ||
| // Prevent default browser behavior | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| e.stopImmediatePropagation(); | ||
| // Try to trigger the create issue modal | ||
| console.log('🎯 WINDOW: Attempting to create new issue'); | ||
| // Dispatch a custom event that the app can listen for | ||
| window.dispatchEvent(new CustomEvent('plane:open-new-issue', { | ||
| detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey } | ||
| })); | ||
| } | ||
| }, { capture: true, passive: false }); | ||
| // Strategy 3: Body level with capture - Cmd+I | ||
| document.body.addEventListener('keydown', function(e) { | ||
| if (e.key === 'i' && (e.metaKey || e.ctrlKey)) { | ||
| console.log('🎯 BODY: Cmd+I detected!'); | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| e.stopImmediatePropagation(); | ||
| window.dispatchEvent(new CustomEvent('plane:open-new-issue', { | ||
| detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey } | ||
| })); | ||
| } | ||
| }, { capture: true, passive: false }); | ||
| // Test: Listen for ANY keydown event to see if our listeners work at all | ||
| document.addEventListener('keydown', function(e) { | ||
| console.log('🎯 TEST: Any keydown detected', e.key, e.metaKey, e.ctrlKey); | ||
| }, { capture: true, passive: false }); | ||
| console.log('🎯 GLOBAL SCRIPT: Event listeners added'); | ||
| `, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Critical: Remove inline script - security and architectural concerns.
This inline script using dangerouslySetInnerHTML has multiple critical issues:
-
Security risk: As flagged by static analysis (CWE-79),
dangerouslySetInnerHTMLbypasses React's XSS protection. While this particular script appears safe (no user input), it sets a dangerous precedent and violates React best practices. -
Architecture violation: Mixing inline global scripts with React's component architecture undermines the benefits of React (lifecycle management, testability, maintainability).
-
Duplicate event listeners: The script registers three separate listeners (lines 112-122, 125-148, 151-161) that all handle Cmd+I with identical logic, causing the same issues noted in the command-palette file.
-
Debug code in production: Multiple console.log statements and test functions (lines 100-107) should not be in production.
-
Conflicts with React listeners: This inline script competes with React component listeners, leading to unpredictable event handling order and potential race conditions.
Recommended solution: Remove the inline script entirely and use the proper React-based KeyboardShortcutProvider approach:
</AppProvider>
- <script
- dangerouslySetInnerHTML={{
- __html: `
- console.log('🎯 GLOBAL SCRIPT: Loading global Cmd+N handler');
- console.log('🎯 GLOBAL SCRIPT: Script is running at', new Date().toISOString());
-
- // Test if the script is running
- window.testGlobalScript = function() {
- console.log('🎯 GLOBAL SCRIPT: Test function called');
- return 'Global script is working';
- };
-
- // Multiple event listeners with different strategies
-
- // Strategy 1: Cmd+I for creating new work items
- document.addEventListener('keydown', function(e) {
- if (e.key === 'i' && (e.metaKey || e.ctrlKey)) {
- console.log('🎯 DOCUMENT: Cmd+I detected!');
- e.preventDefault();
- e.stopPropagation();
- e.stopImmediatePropagation();
- window.dispatchEvent(new CustomEvent('plane:open-new-issue', {
- detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey }
- }));
- }
- }, { capture: true, passive: false });
-
- // Strategy 2: Window level with capture - Cmd+I
- window.addEventListener('keydown', function(e) {
- if (e.key === 'i' && (e.metaKey || e.ctrlKey)) {
- console.log('🎯 WINDOW: Cmd+I detected!', {
- key: e.key,
- metaKey: e.metaKey,
- ctrlKey: e.ctrlKey,
- target: e.target?.tagName,
- timestamp: Date.now()
- });
-
- // Prevent default browser behavior
- e.preventDefault();
- e.stopPropagation();
- e.stopImmediatePropagation();
-
- // Try to trigger the create issue modal
- console.log('🎯 WINDOW: Attempting to create new issue');
-
- // Dispatch a custom event that the app can listen for
- window.dispatchEvent(new CustomEvent('plane:open-new-issue', {
- detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey }
- }));
- }
- }, { capture: true, passive: false });
-
- // Strategy 3: Body level with capture - Cmd+I
- document.body.addEventListener('keydown', function(e) {
- if (e.key === 'i' && (e.metaKey || e.ctrlKey)) {
- console.log('🎯 BODY: Cmd+I detected!');
- e.preventDefault();
- e.stopPropagation();
- e.stopImmediatePropagation();
- window.dispatchEvent(new CustomEvent('plane:open-new-issue', {
- detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey }
- }));
- }
- }, { capture: true, passive: false });
-
- // Test: Listen for ANY keydown event to see if our listeners work at all
- document.addEventListener('keydown', function(e) {
- console.log('🎯 TEST: Any keydown detected', e.key, e.metaKey, e.ctrlKey);
- }, { capture: true, passive: false });
-
- console.log('🎯 GLOBAL SCRIPT: Event listeners added');
- `,
- }}
- />
</body>Then uncomment the KeyboardShortcutProvider wrapper (see previous comment).
📝 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.
| <script | |
| dangerouslySetInnerHTML={{ | |
| __html: ` | |
| console.log('🎯 GLOBAL SCRIPT: Loading global Cmd+N handler'); | |
| console.log('🎯 GLOBAL SCRIPT: Script is running at', new Date().toISOString()); | |
| // Test if the script is running | |
| window.testGlobalScript = function() { | |
| console.log('🎯 GLOBAL SCRIPT: Test function called'); | |
| return 'Global script is working'; | |
| }; | |
| // Multiple event listeners with different strategies | |
| // Strategy 1: Cmd+I for creating new work items | |
| document.addEventListener('keydown', function(e) { | |
| if (e.key === 'i' && (e.metaKey || e.ctrlKey)) { | |
| console.log('🎯 DOCUMENT: Cmd+I detected!'); | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| e.stopImmediatePropagation(); | |
| window.dispatchEvent(new CustomEvent('plane:open-new-issue', { | |
| detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey } | |
| })); | |
| } | |
| }, { capture: true, passive: false }); | |
| // Strategy 2: Window level with capture - Cmd+I | |
| window.addEventListener('keydown', function(e) { | |
| if (e.key === 'i' && (e.metaKey || e.ctrlKey)) { | |
| console.log('🎯 WINDOW: Cmd+I detected!', { | |
| key: e.key, | |
| metaKey: e.metaKey, | |
| ctrlKey: e.ctrlKey, | |
| target: e.target?.tagName, | |
| timestamp: Date.now() | |
| }); | |
| // Prevent default browser behavior | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| e.stopImmediatePropagation(); | |
| // Try to trigger the create issue modal | |
| console.log('🎯 WINDOW: Attempting to create new issue'); | |
| // Dispatch a custom event that the app can listen for | |
| window.dispatchEvent(new CustomEvent('plane:open-new-issue', { | |
| detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey } | |
| })); | |
| } | |
| }, { capture: true, passive: false }); | |
| // Strategy 3: Body level with capture - Cmd+I | |
| document.body.addEventListener('keydown', function(e) { | |
| if (e.key === 'i' && (e.metaKey || e.ctrlKey)) { | |
| console.log('🎯 BODY: Cmd+I detected!'); | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| e.stopImmediatePropagation(); | |
| window.dispatchEvent(new CustomEvent('plane:open-new-issue', { | |
| detail: { key: e.key, metaKey: e.metaKey, ctrlKey: e.ctrlKey } | |
| })); | |
| } | |
| }, { capture: true, passive: false }); | |
| // Test: Listen for ANY keydown event to see if our listeners work at all | |
| document.addEventListener('keydown', function(e) { | |
| console.log('🎯 TEST: Any keydown detected', e.key, e.metaKey, e.ctrlKey); | |
| }, { capture: true, passive: false }); | |
| console.log('🎯 GLOBAL SCRIPT: Event listeners added'); | |
| `, | |
| }} | |
| /> | |
| </AppProvider> | |
| </body> |
🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 97-97: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
[error] 98-98: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
| const handleKeyboardShortcut = () => { | ||
| console.log('🎯 Modal integration received keyboard shortcut event'); | ||
| console.log('🎯 Current modal state:', isCreateIssueModalOpen); | ||
|
|
||
| // Open the modal if it's not already open | ||
| if (!isCreateIssueModalOpen) { | ||
| console.log('🎯 Opening new issue modal...'); | ||
| toggleCreateIssueModal(true); | ||
| } else { | ||
| console.log('🎯 Modal already open, not opening duplicate'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove console logging from production code.
The event handler contains multiple console.log statements that should be removed before production deployment.
Apply this diff:
const handleKeyboardShortcut = () => {
- console.log('🎯 Modal integration received keyboard shortcut event');
- console.log('🎯 Current modal state:', isCreateIssueModalOpen);
// Open the modal if it's not already open
if (!isCreateIssueModalOpen) {
- console.log('🎯 Opening new issue modal...');
toggleCreateIssueModal(true);
- } else {
- console.log('🎯 Modal already open, not opening duplicate');
}
};📝 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.
| const handleKeyboardShortcut = () => { | |
| console.log('🎯 Modal integration received keyboard shortcut event'); | |
| console.log('🎯 Current modal state:', isCreateIssueModalOpen); | |
| // Open the modal if it's not already open | |
| if (!isCreateIssueModalOpen) { | |
| console.log('🎯 Opening new issue modal...'); | |
| toggleCreateIssueModal(true); | |
| } else { | |
| console.log('🎯 Modal already open, not opening duplicate'); | |
| } | |
| const handleKeyboardShortcut = () => { | |
| // Open the modal if it's not already open | |
| if (!isCreateIssueModalOpen) { | |
| toggleCreateIssueModal(true); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/web/components/keyboard-shortcut-modal-integration.tsx around lines 18
to 28, remove all console.log statements inside handleKeyboardShortcut so
production code has no console output; keep the control flow intact (check
isCreateIssueModalOpen and call toggleCreateIssueModal(true) when closed), and
if logging is necessary for development replace console.log calls with a
debug/logger call behind a conditional or feature-flag so no logs emit in
production.
| onShortcut: (event) => { | ||
| console.log('🎹 Keyboard shortcut activated:', event.detail); | ||
| console.log('🎹 Event type:', event.type); | ||
| console.log('🎹 Key combination:', event.detail.keyCombination); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove console logging from production code.
Console log statements should be removed before merging to production, as they clutter the browser console and may expose internal implementation details.
Apply this diff to remove the console logging:
useGlobalHotkeys({
enabled: true,
throttleMs: 100,
- onShortcut: (event) => {
- console.log('🎹 Keyboard shortcut activated:', event.detail);
- console.log('🎹 Event type:', event.type);
- console.log('🎹 Key combination:', event.detail.keyCombination);
- }
});🤖 Prompt for AI Agents
In apps/web/components/keyboard-shortcut-provider.tsx around lines 16 to 20,
remove the three console.log statements that output keyboard shortcut details to
avoid leaking internal information in production; replace them either by calling
a proper debug/logger utility behind an environment check (e.g., only log when
NODE_ENV === 'development') or remove entirely if no logging is needed, ensuring
the onShortcut handler still performs its intended behavior without console
output.
| export interface KeyboardEventDetail { | ||
| source: 'keyboard'; | ||
| keyCombination: 'meta+n' | 'ctrl+n'; | ||
| timestamp: number; | ||
| focusContext: { | ||
| activeElement: string; | ||
| isEditable: boolean; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical: Type definition doesn't match actual usage.
The keyCombination is defined as 'meta+n' | 'ctrl+n', but the actual implementation in use-global-hotkeys.tsx uses 'meta+i' | 'ctrl+i' (lines 62). This PR is for the Cmd/Ctrl+I shortcut, not Cmd/Ctrl+N. This type mismatch will cause TypeScript compilation errors.
Apply this diff to fix the type definition:
export interface KeyboardEventDetail {
source: 'keyboard';
- keyCombination: 'meta+n' | 'ctrl+n';
+ keyCombination: 'meta+i' | 'ctrl+i';
timestamp: number;
focusContext: {
activeElement: string;
isEditable: boolean;
};
}📝 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.
| export interface KeyboardEventDetail { | |
| source: 'keyboard'; | |
| keyCombination: 'meta+n' | 'ctrl+n'; | |
| timestamp: number; | |
| focusContext: { | |
| activeElement: string; | |
| isEditable: boolean; | |
| }; | |
| } | |
| export interface KeyboardEventDetail { | |
| source: 'keyboard'; | |
| keyCombination: 'meta+i' | 'ctrl+i'; | |
| timestamp: number; | |
| focusContext: { | |
| activeElement: string; | |
| isEditable: boolean; | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/web/core/types/keyboard.ts around lines 5 to 13, the
KeyboardEventDetail.keyCombination union incorrectly lists 'meta+n' | 'ctrl+n'
while the implementation uses the Cmd/Ctrl+I shortcut; update the type to
'meta+i' | 'ctrl+i' to match use-global-hotkeys.tsx (line 62) so TypeScript
types align with runtime usage.
| export function createKeyboardEventDetail(): KeyboardEventDetail { | ||
| const focusContext = getFocusContext(); | ||
| const keyCombination = isMacOS() ? 'meta+n' : 'ctrl+n'; | ||
|
|
||
| return { | ||
| source: 'keyboard', | ||
| keyCombination, | ||
| timestamp: Date.now(), | ||
| focusContext: { | ||
| activeElement: focusContext.tagName, | ||
| isEditable: focusContext.isEditable | ||
| } | ||
| }; |
There was a problem hiding this comment.
Critical: Fix key combination mismatch.
The key combination is hardcoded as 'meta+n' and 'ctrl+n', but the keyboard shortcuts constant in apps/web/core/constants/keyboard-shortcuts.ts defines the key as 'i', and the PR summary specifies ⌘+I / Ctrl+I. This mismatch will cause incorrect event details to be dispatched.
Apply this diff to fix the key combination:
export function createKeyboardEventDetail(): KeyboardEventDetail {
const focusContext = getFocusContext();
- const keyCombination = isMacOS() ? 'meta+n' : 'ctrl+n';
+ const keyCombination = isMacOS() ? 'meta+i' : 'ctrl+i';
return {
source: 'keyboard',
keyCombination,
timestamp: Date.now(),
focusContext: {
activeElement: focusContext.tagName,
isEditable: focusContext.isEditable
}
};
}📝 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.
| export function createKeyboardEventDetail(): KeyboardEventDetail { | |
| const focusContext = getFocusContext(); | |
| const keyCombination = isMacOS() ? 'meta+n' : 'ctrl+n'; | |
| return { | |
| source: 'keyboard', | |
| keyCombination, | |
| timestamp: Date.now(), | |
| focusContext: { | |
| activeElement: focusContext.tagName, | |
| isEditable: focusContext.isEditable | |
| } | |
| }; | |
| export function createKeyboardEventDetail(): KeyboardEventDetail { | |
| const focusContext = getFocusContext(); | |
| const keyCombination = isMacOS() ? 'meta+i' : 'ctrl+i'; | |
| return { | |
| source: 'keyboard', | |
| keyCombination, | |
| timestamp: Date.now(), | |
| focusContext: { | |
| activeElement: focusContext.tagName, | |
| isEditable: focusContext.isEditable | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/web/core/utils/custom-events.ts around lines 18 to 30, the
keyCombination is incorrectly hardcoded to 'meta+n'/'ctrl+n'; update it to the
correct shortcut using 'i' (⌘+I / Ctrl+I). Replace the current hardcoded values
with either a direct fix to 'meta+i' for macOS and 'ctrl+i' for others, or
(preferably) import the keyboard shortcut constant from
apps/web/core/constants/keyboard-shortcuts.ts and use that to build the
keyCombination so it matches the canonical shortcut definition.
| export function getKeyboardShortcutDisplay(): string { | ||
| const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0; | ||
| return isMac ? '⌘+N' : 'Ctrl+N'; | ||
| } |
There was a problem hiding this comment.
Critical: Fix displayed keyboard shortcut.
The function returns '⌘+N' and 'Ctrl+N', but based on the keyboard shortcuts constant (apps/web/core/constants/keyboard-shortcuts.ts) and the PR summary, it should display '⌘+I' and 'Ctrl+I'.
Apply this diff to fix the displayed shortcut:
export function getKeyboardShortcutDisplay(): string {
const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0;
- return isMac ? '⌘+N' : 'Ctrl+N';
+ return isMac ? '⌘+I' : 'Ctrl+I';
}📝 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.
| export function getKeyboardShortcutDisplay(): string { | |
| const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0; | |
| return isMac ? '⌘+N' : 'Ctrl+N'; | |
| } | |
| export function getKeyboardShortcutDisplay(): string { | |
| const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0; | |
| return isMac ? '⌘+I' : 'Ctrl+I'; | |
| } |
🤖 Prompt for AI Agents
In apps/web/core/utils/keyboard-shortcut-display.ts around lines 8 to 11, the
function currently returns '⌘+N' for mac and 'Ctrl+N' for other platforms but
should display '⌘+I' and 'Ctrl+I' to match the keyboard-shortcuts constant;
update the returned strings so the mac branch returns '⌘+I' and the non-mac
branch returns 'Ctrl+I', keeping the platform detection logic unchanged.
| expect(result.current.supportedKeys).toContain('⌘+N'); | ||
| expect(result.current.supportedKeys).toContain('Ctrl+N'); |
There was a problem hiding this comment.
Fix incorrect key combination in test assertion.
The test is checking for '⌘+N' and 'Ctrl+N', but according to the hook implementation (referenced in the context), the actual supported keys are '⌘+I' and 'Ctrl+I'. This mismatch will cause the test to fail.
Apply this diff to fix the test:
expect(result.current.isListening).toBe(true);
- expect(result.current.supportedKeys).toContain('⌘+N');
- expect(result.current.supportedKeys).toContain('Ctrl+N');
+ expect(result.current.supportedKeys).toContain('⌘+I');
+ expect(result.current.supportedKeys).toContain('Ctrl+I');📝 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.
| expect(result.current.supportedKeys).toContain('⌘+N'); | |
| expect(result.current.supportedKeys).toContain('Ctrl+N'); | |
| expect(result.current.isListening).toBe(true); | |
| expect(result.current.supportedKeys).toContain('⌘+I'); | |
| expect(result.current.supportedKeys).toContain('Ctrl+I'); |
🤖 Prompt for AI Agents
In apps/web/tests/unit/hooks/use-global-hotkeys.test.tsx around lines 26 to 27,
the test asserts supportedKeys contain '⌘+N' and 'Ctrl+N' but the hook actually
registers '⌘+I' and 'Ctrl+I'; update the assertions to expect '⌘+I' and 'Ctrl+I'
respectively so the test matches the implementation.
| it('should detect macOS platform', () => { | ||
| // Mock navigator.platform | ||
| Object.defineProperty(navigator, 'platform', { | ||
| value: 'MacIntel', | ||
| configurable: true | ||
| }); | ||
|
|
||
| expect(isMacOS()).toBe(true); | ||
| }); | ||
|
|
||
| it('should detect non-macOS platform', () => { | ||
| Object.defineProperty(navigator, 'platform', { | ||
| value: 'Win32', | ||
| configurable: true | ||
| }); | ||
|
|
||
| expect(isMacOS()).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add cleanup to prevent test pollution.
Mocking navigator.platform without restoring the original value can cause test isolation issues. Consider using beforeEach/afterEach to save and restore the original value, or use a scoped mock approach.
Apply this diff to add proper cleanup:
describe('isMacOS', () => {
+ let originalPlatform: string;
+
+ beforeEach(() => {
+ originalPlatform = navigator.platform;
+ });
+
+ afterEach(() => {
+ Object.defineProperty(navigator, 'platform', {
+ value: originalPlatform,
+ configurable: true
+ });
+ });
+
it('should detect macOS platform', () => {
- // Mock navigator.platform
Object.defineProperty(navigator, 'platform', {
value: 'MacIntel',
configurable: true
});
expect(isMacOS()).toBe(true);
});
it('should detect non-macOS platform', () => {
Object.defineProperty(navigator, 'platform', {
value: 'Win32',
configurable: true
});
expect(isMacOS()).toBe(false);
});
});📝 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.
| it('should detect macOS platform', () => { | |
| // Mock navigator.platform | |
| Object.defineProperty(navigator, 'platform', { | |
| value: 'MacIntel', | |
| configurable: true | |
| }); | |
| expect(isMacOS()).toBe(true); | |
| }); | |
| it('should detect non-macOS platform', () => { | |
| Object.defineProperty(navigator, 'platform', { | |
| value: 'Win32', | |
| configurable: true | |
| }); | |
| expect(isMacOS()).toBe(false); | |
| }); | |
| describe('isMacOS', () => { | |
| let originalPlatform: string; | |
| beforeEach(() => { | |
| originalPlatform = navigator.platform; | |
| }); | |
| afterEach(() => { | |
| Object.defineProperty(navigator, 'platform', { | |
| value: originalPlatform, | |
| configurable: true | |
| }); | |
| }); | |
| it('should detect macOS platform', () => { | |
| Object.defineProperty(navigator, 'platform', { | |
| value: 'MacIntel', | |
| configurable: true | |
| }); | |
| expect(isMacOS()).toBe(true); | |
| }); | |
| it('should detect non-macOS platform', () => { | |
| Object.defineProperty(navigator, 'platform', { | |
| value: 'Win32', | |
| configurable: true | |
| }); | |
| expect(isMacOS()).toBe(false); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In apps/web/tests/unit/utils/keyboard-events.test.ts around lines 69 to 86, the
tests mock navigator.platform but do not restore the original value which can
pollute other tests; update the test file to capture the original
navigator.platform before each test (or once in a beforeEach), then restore it
in an afterEach (or use jest.spyOn(navigator, 'platform', 'get') and call
mockRestore in afterEach) so each test leaves navigator.platform unchanged after
completion.
abdulmujeeb4ahmed
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
mock-api.js (4)
8-9: Document the wide-open CORS configuration.CORS is enabled without any origin restrictions, which is acceptable for a local mock server but should be explicitly documented to prevent misuse in non-testing environments.
Consider adding a comment clarifying the intent:
+// Allow all origins for local testing - do not use in production app.use(cors()); app.use(express.json());
29-32: Add input validation to catch test bugs early.While this is a mock endpoint, adding basic validation can help catch bugs in test code that sends malformed requests. The endpoint currently accepts any request body without validation.
Consider adding basic validation:
// Mock admin signup endpoint app.post('/api/instances/admins/sign-up/', (req, res) => { - res.json({ success: true }); + try { + // Validate expected fields to help catch test bugs + const { email, password } = req.body; + if (!email || !password) { + return res.status(400).json({ + error: 'Missing required fields: email, password' + }); + } + res.json({ success: true }); + } catch (error) { + console.error('Error in /api/instances/admins/sign-up/:', error); + res.status(500).json({ error: 'Internal server error' }); + } });
34-37: Static CSRF token is acceptable for testing but document the limitation.The endpoint returns a static CSRF token, which is fine for a mock server but diverges from real CSRF token behavior (which should be unique per session). Consider adding a comment to clarify this is intentional for testing.
-// Mock CSRF token endpoint +// Mock CSRF token endpoint - returns static token for testing (not suitable for production) app.get('/api/auth/csrf-token/', (req, res) => { res.json({ csrf_token: 'mock-csrf-token' }); });
39-41: Add error handling for port conflicts and graceful shutdown.The server startup lacks error handling for common issues like port conflicts, and doesn't handle graceful shutdown signals.
Improve startup and shutdown handling:
-app.listen(port, () => { - console.log(`Mock API running at http://localhost:${port}`); +const server = app.listen(port, () => { + console.log(`Mock API running at http://localhost:${port}`); +}).on('error', (err) => { + if (err.code === 'EADDRINUSE') { + console.error(`Port ${port} is already in use. Please specify a different port via PORT environment variable.`); + } else { + console.error('Server error:', err); + } + process.exit(1); +}); + +// Graceful shutdown +process.on('SIGTERM', () => { + console.log('SIGTERM received, shutting down gracefully...'); + server.close(() => { + console.log('Server closed'); + process.exit(0); + }); +}); + +process.on('SIGINT', () => { + console.log('SIGINT received, shutting down gracefully...'); + server.close(() => { + console.log('Server closed'); + process.exit(0); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api-vercel.json(1 hunks)mock-api.js(1 hunks)vercel-api.json(1 hunks)vercel.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- api-vercel.json
- vercel-api.json
- vercel.json
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
mock-api.js (2)
24-27: Applies the same error handling concern as previous endpoint.This endpoint shares the same missing error handling issue noted for the
/api/instances/endpoint.
1-6: Verify mock-api.js relevance and dependencies
- This file mocks admin API endpoints but the PR is scoped to adding keyboard shortcuts. Confirm whether it should be included in this branch or moved to a separate one.
- Ensure
expressandcorsare declared in the correctpackage.jsonfor this file (currently found in apps/live/package.json).
| // Mock instance endpoint | ||
| app.get('/api/instances/', (req, res) => { | ||
| res.json({ | ||
| instance: { | ||
| id: '1', | ||
| name: 'Mock Instance', | ||
| is_setup_done: true, | ||
| workspaces_exist: true | ||
| }, | ||
| config: {} | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling to prevent server crashes.
The endpoint lacks error handling. If res.json() throws (e.g., due to circular references or other serialization issues), the server will crash.
Wrap endpoint logic in try-catch blocks or use Express error-handling middleware:
// Mock instance endpoint
app.get('/api/instances/', (req, res) => {
- res.json({
- instance: {
- id: '1',
- name: 'Mock Instance',
- is_setup_done: true,
- workspaces_exist: true
- },
- config: {}
- });
+ try {
+ res.json({
+ instance: {
+ id: '1',
+ name: 'Mock Instance',
+ is_setup_done: true,
+ workspaces_exist: true
+ },
+ config: {}
+ });
+ } catch (error) {
+ console.error('Error in /api/instances/:', error);
+ res.status(500).json({ error: 'Internal server error' });
+ }
});Alternatively, add a global error handler at the end of the file:
// Global error handler
app.use((err, req, res, next) => {
console.error('Unhandled error:', err);
res.status(500).json({ error: 'Internal server error' });
});🤖 Prompt for AI Agents
In mock-api.js around lines 11 to 22 the GET /api/instances/ handler writes a
response without any error handling which can crash the server if res.json()
throws; wrap the handler logic in a try-catch and on error call next(err) or
respond with res.status(500).json({ error: 'Internal server error' }) so the
request fails safely, and add a global Express error-handling middleware at the
end of the file (app.use((err, req, res, next) => { console.error(err);
res.status(500).json({ error: 'Internal server error' }); })) to catch any
unhandled errors.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vercel.json (1)
3-3: Drop the extrapnpm installfrom the build command.Vercel runs
installCommandbeforebuildCommand, so this line triggers a second full install. That wastes build minutes and can surface lockfile mismatches between the two installs. Remove the redundant install and keep the build step focused onpnpm turbo run build --filter=admin.- "buildCommand": "pnpm install && pnpm turbo run build --filter=admin", + "buildCommand": "pnpm turbo run build --filter=admin",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vercel.json(1 hunks)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…es for Vercel deployment
- Remove all workspace dependencies (@plane/* packages) - Create standalone package.json with resolved versions - Fix TypeScript configuration to exclude parent directories - Remove complex pages and components with workspace imports - Add minimal Tailwind CSS and PostCSS configuration - Create simple welcome page that displays deployment success The admin app now builds successfully and can be deployed to Vercel.
|
Hello @abdulmujeeb4ahmed Due to some changes that are in Preview, Your changes are shifted a bit. Also there are more than 300+ changes in this PR. Can you close this and reopen a new one. |
|
@abdulmujeeb4ahmed we’re also currently refactoring our power k, which brings a cleaner and more flexible way to handle and register shortcuts. It might be best to wait for that to be merged before reopening your PR, so you can build on top of the new structure. |
4 participants
🚀 Feature: Add Global Keyboard Shortcut for New Issue Creation
Description
This PR implements a global keyboard shortcut (⌘+I on macOS, Ctrl+I on Windows/Linux) to open the new issue modal from anywhere in the application. The feature includes comprehensive focus management to prevent conflicts with text editing, cross-platform compatibility, and full integration with existing UI components.
Key Features Implemented:
Resolves: Global keyboard shortcut for new issue modal
Type of Change
Changes Made
Core Implementation
useGlobalHotkeys- Manages global keyboard event listeningkeyboard-shortcuts.ts- Centralized shortcut definitionsFiles Added
apps/web/core/hooks/use-global-hotkeys.tsx- Main hook implementationapps/web/core/constants/keyboard-shortcuts.ts- Shortcut constantsapps/web/core/types/keyboard.ts- TypeScript definitionsapps/web/core/utils/- Supporting utilities (custom-events, focus-context, throttle)apps/web/components/keyboard-shortcut-*.tsx- React componentsapps/web/tests/- Comprehensive test suiteFiles Modified
apps/web/app/layout.tsx- Added global hook integrationapps/web/core/components/command-palette/command-palette.tsx- Enhanced with shortcutsapps/web/core/components/issues/issue-layouts/quick-add/button/*.tsx- Added tooltipsapps/web/core/components/workspace/sidebar/quick-actions.tsx- Enhanced quick actionsFeatures Implemented
✅ Core Functionality
✅ User Experience
✅ Technical Implementation
Screenshots and Media
Before: No global keyboard shortcut available - users had to navigate to specific pages or click buttons to create new issues.
After: Users can press ⌘+I (macOS) or Ctrl+I (Windows/Linux) from anywhere in the app to instantly open the new issue modal.
Visual Indicators:
Test Scenarios
Automated Tests:
Manual Test Scenarios:
Test Results:
Testing Instructions
Manual Testing
Basic Functionality:
Focus Management:
Edge Cases:
Automated Testing
Browser Compatibility
Performance Impact
Future Enhancements
References
specs/001-add-a-global/spec.mdspecs/001-add-a-global/plan.mdspecs/001-add-a-global/spec.md#user-scenarios--testing001-add-a-globalRelated Documentation:
Checklist
Summary by CodeRabbit
New Features
Tests
Documentation
Chores