fix(ui): handle keyboard text selection in canvas #1933
Conversation
Signed-off-by: Petr Kadlec <petr@puradesign.cz>
dosubot
bot
added
the
size:S
Summary of ChangesHello @kapetr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where text selection within the canvas UI was not responsive to keyboard input. By introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue with keyboard text selection in the canvas. The approach is sound, but I've identified an opportunity to make the implementation more robust and performant. My main feedback is on the handleKeyUp logic, which is currently too broad and could lead to unnecessary function calls. I've suggested a more precise implementation that also handles more selection keys and the 'Select All' shortcut.
| const handleKeyUp = (event: KeyboardEvent) => { | ||
| if (event.shiftKey || isArrowKey(event.key)) { | ||
| processSelection(); | ||
| } | ||
| }; | ||
|
|
||
| container.addEventListener('mouseup', handleMouseUp); | ||
| document.addEventListener('keyup', handleKeyUp); | ||
|
|
||
| return () => { | ||
| container.removeEventListener('mouseup', handleMouseUp); | ||
| document.removeEventListener('keyup', handleKeyUp); | ||
| }; | ||
| }, [containerRef, onSelectionChange]); | ||
| } | ||
|
|
||
| const ARROW_KEYS = ['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown']; | ||
|
|
||
| export function isArrowKey(key: string): boolean { | ||
| return ARROW_KEYS.includes(key); | ||
| } |
There was a problem hiding this comment.
The current logic for handling keyboard selection can be improved for robustness and performance.
- The condition
event.shiftKeyis too broad. It will triggerprocessSelectionfor any key pressed with Shift (e.g., typing uppercase letters), causing unnecessary processing. isArrowKeyis too specific and doesn't handle other common selection keys likeHome,End,PageUp, andPageDown. Pressing these keys should also update the selection state (e.g., to clear it when pressed without Shift).
I suggest expanding the list of keys to include all navigation keys and refining the handleKeyUp logic to be more precise. This will make the code more performant, readable, and will handle more edge cases correctly. I've also included handling for the 'Select All' keyboard shortcut (Ctrl+A or Cmd+A).
const handleKeyUp = (event: KeyboardEvent) => {
const isSelectAll = (event.ctrlKey || event.metaKey) && event.key.toLowerCase() === 'a';
if (isSelectionKey(event.key) || isSelectAll) {
processSelection();
}
};
container.addEventListener('mouseup', handleMouseUp);
document.addEventListener('keyup', handleKeyUp);
return () => {
container.removeEventListener('mouseup', handleMouseUp);
document.removeEventListener('keyup', handleKeyUp);
};
}, [containerRef, onSelectionChange]);
}
const SELECTION_KEYS = ['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown', 'Home', 'End', 'PageUp', 'PageDown'];
export function isSelectionKey(key: string): boolean {
return SELECTION_KEYS.includes(key);
}
|
I wonder if you could simplify this through https://developer.mozilla.org/en-US/docs/Web/API/Document/selectionchange_event ? Gemini is right that by this approach you would still miss non-arrow keyboard navigation. |
@JanPokorny That's where I started :) I abandoned the |
| }, [containerRef, onSelectionChange]); | ||
| } | ||
|
|
||
| const ARROW_KEYS = ['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown']; |
There was a problem hiding this comment.
Not any issue, but you can use keycode-js that we have as a dependency (see apps/agentstack-ui/src/utils/form-utils.ts). :-)
|
@kapetr Did you try debouncing it, with e.g. https://lodash.com/docs/#debounce ? |
|
@JanPokorny I considered that, but that doesn't change the fact, that the tooltip with controls would pop up even if you're not done selecting (unless you're super fast). But I might have unnecessarily locked myself into the idea of showing the tooltip only after selection is finished. |
|
@kapetr Maybe combine the approaches and avoid showing the tooltip when left click is held down, that avoids accidentally getting in the way of the user doing a mouse selection, and with keyboard selection it would just pop up when you're not pressing the keys fast enough 😁 |
Summary
The PR fixes issue where text selection in canvas didn’t work with keyboard.
Linked Issues
Refs #1874
Documentation