Uh oh!
There was an error while loading. Please reload this page.
fix(Tasks): implement hotkeysEnabled state to control keyboard shortcuts - #426
fix(Tasks): implement hotkeysEnabled state to control keyboard shortcuts#426ShivaGupta-14 wants to merge 1 commit into
Conversation
Thank you for opening this PR! Before a maintainer takes a look, it would be really helpful if you could walk through your changes using GitHub's review tools. Please take a moment to:
More information on how to conduct a self review: This helps make the review process smoother and gives us a clearer understanding of your thought process. Once you've added your self-review, we'll continue from our side. Thank you! |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ShivaGupta-14
left a comment
There was a problem hiding this comment.
self review done, ready for review
its-me-abhishek
commented
Jan 23, 2026
@ShivaGupta-14 this looks good but the mouse control check not work in scenarios though. Like for example a tablet with some keyboard. Probably need a better check, instead of this. |
ShivaGupta-14
commented
Jan 24, 2026
thanks @its-me-abhishek! you are right it needs a better check. I researched this and I am planning to use the Active Context approach combined with the current implementation, so it will work on both touch/keyboard devices and with mouse hover. will push the update soon! |
7d0605a to
d82cb88CompareShivaGupta-14
commented
Jan 24, 2026
pushed the updated changes! now hotkeys work for mouse hover, clicks, taps, and stylus input |
its-me-abhishek
commented
Jan 26, 2026
I think just having an active context should suffice for this, not sure though |
ShivaGupta-14
commented
Jan 26, 2026
for task management apps, hover is the standard UX, users don't context-switch like in VS Code, so i think it's better to keep it. |
3113ea5 to
5a80ce2Compare- Add `enabled` parameter to useHotkeys hook with default true value - Pass hotkeysEnabled state to all useHotkeys calls in Tasks component - Add hotkeysEnabled check in manual keyboard handler (ArrowUp/Down/Enter) - Add data-testid to tasks table container for testing - Add tests for useHotkeys enabled parameter - Add tests for hotkeys enable/disable on mouse hover - Add active context pattern with `onPointerDown` for tablet/touch support - Add global pointerdown listener to disable hotkeys on outside click - Add tests for active context scenarios - Add helper.ts to reduce redundant hotkey setup in tests
5a80ce2 to
57351eaCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
This looks fine but since we already have too many states and useEffects already, I guess we should avoid adding more of them to maintain.
The Intersection Observer API is a better fix (either globally or locally) for this issue, and this package provides that. This solution might be a bit better, and well tested, as compared to this, as the current approach requires some testing and might break on some devices. Can close the PR for now if required.
Hi @its-me-abhishek, Thanks for the suggestion about using Intersection Observer and the package! I did some research on it and found that it detects visibility (whether an element is on screen) rather than user interaction (hover/click). while this might work fro our current setup with mainly the Tasks table, it could create conflicts when we add the planned features like Kanban, Calendar, and Graph views, since multiple views could be visible simultaneously, causing hotkey conflicts. regarding device compatibility concerns you mentioned, the current implementation uses pointer events which are well-supported across desktop, mobile, tablets, and stylus devices. I completely understand your concern about too many states and useEffects in Tasks.tsx, as a solution, I can refactor the current implementation into a reusable useHotkeyScope custom hook. This would:
would that work better? happy to discuss alternatives if you have other ideas! Thanks! |
ShivaGupta-14
commented
Feb 19, 2026
@its-me-abhishek PTAL :) |
Description
Add
enabledparameter to useHotkeys hook with default true valuePass hotkeysEnabled state to all useHotkeys calls in Tasks component
Add hotkeysEnabled check in manual keyboard handler (ArrowUp/Down/Enter)
Add data-testid to tasks table container for testing
Add tests for useHotkeys enabled parameter
Add tests for hotkeys enable/disable on mouse hover
Add active context pattern with
onPointerDownfor tablet/touch supportAdd global pointerdown listener to disable hotkeys on outside click
Add tests for active context scenarios
Fixes: bug: hotkeysEnabled state is set but never checked before executing hotkeys #425
Checklist
npx prettier --write .(for formatting)gofmt -w .(for Go backend)npm test(for JS/TS testing)Additional Notes
Video:
Screen.Recording.2026-01-22.at.1.17.29.AM.mov