-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: setup the context for keyboard hotkeys #4493
Conversation
Warning Rate Limit Exceeded@Vikrant2520 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 51 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent update introduces a new Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- frontend/src/AppRoutes/index.tsx (2 hunks)
- frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx (1 hunks)
Additional comments: 6
frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx (5)
- 16-21: The default context value uses
noop
from lodash forregisterShortcut
andderegisterShortcut
. Ensure that this aligns with the intended behavior when the context is used outside of its provider.- 23-23: The
IGNORE_INPUTS
array is a good approach to prevent hotkey actions in input fields. Verify that all relevant input types are covered.- 26-31: The error thrown in the custom hook ensures it's used within its provider. This is a good practice for context hooks.
- 59-64: Using
useEffect
to add and remove the event listener is correct. Ensure that the dependency array is intentionally left empty to only run on mount and unmount.- 75-81: The
deregisterShortcut
function correctly removes a registered shortcut. Ensure that there's a mechanism to prevent memory leaks or dangling references.frontend/src/AppRoutes/index.tsx (1)
- 181-198: The integration of
KeyboardHotkeysProvider
aroundAppLayout
is correctly implemented to provide hotkey functionality across the application. Ensure that this does not introduce any unintended side effects, especially in components deeply nested withinAppLayout
.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
c51ac35
to
dd3c5ed
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- frontend/src/AppRoutes/index.tsx (2 hunks)
- frontend/src/container/SideNav/SideNav.tsx (3 hunks)
- frontend/src/hooks/hotkeys/tests/useKeyboardHotkeys.test.tsx (1 hunks)
- frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- frontend/src/AppRoutes/index.tsx
- frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx
Additional comments: 2
frontend/src/container/SideNav/SideNav.tsx (2)
- 12-12: The import of
useKeyboardHotkeys
is correctly placed and follows the project's import ordering conventions.- 159-173: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [102-169]
The registration and deregistration of the 'b+meta' shortcut for collapsing the sidebar are correctly implemented within a
useEffect
hook. However, the dependency array of theuseEffect
hook is empty, which is appropriate in this case sinceonCollapse
is stable and doesn't depend on any props or state. Ensure thatonCollapse
remains stable if additional dependencies are introduced in the future.
4cc7c05
to
403af83
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- frontend/src/AppRoutes/index.tsx (2 hunks)
- frontend/src/constants/shortcuts/globalShortcuts.ts (1 hunks)
- frontend/src/container/SideNav/SideNav.tsx (3 hunks)
- frontend/src/hooks/hotkeys/tests/useKeyboardHotkeys.test.tsx (1 hunks)
- frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- frontend/src/AppRoutes/index.tsx
- frontend/src/constants/shortcuts/globalShortcuts.ts
- frontend/src/container/SideNav/SideNav.tsx
- frontend/src/hooks/hotkeys/tests/useKeyboardHotkeys.test.tsx
- frontend/src/hooks/hotkeys/useKeyboardHotkeys.tsx
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Summary
PRD for the below changes :- https://www.notion.so/signoz/Keyboard-Shortcuts-Framework-309c2b67f3c04ddab316c133ef051df3
keyboard hotkeys
Usage in components ->
Case 1 -> Usage of same shortcut
l
in two components (Services Module and Logs Explorer Module)Services Trace Component
Logs Explorer Component
Since the components are not present on the screen at the same time no error is thrown and both are supported as expected
Video Reference:-
l
across two different modules not firing together and the input boxes not triggering the shortcut callbacksScreen.Recording.2024-02-05.at.3.01.50.PM.mov
Case 2 -> Usage of same shortcuts in the same component tree when both are rendered on the screen at same time
Services Root Component
Services Trace Table Component
Now since both of them are present in the screen at the same time we throw an exception and crash the UI at development stage itself so as the dev should be aware of the same.
Video Reference:-
Screen.Recording.2024-02-05.at.2.59.36.PM.mov
Added Implementation of
CMD+B
forSideNav Collapse and Open
Screen.Recording.2024-02-05.at.3.16.54.PM.mov
Summary by CodeRabbit