feat(data-modeling): add keyboard controls for DM experience COMPASS-9779 #7474

addaleax · 2025-10-18T22:04:08Z

Add support for keyboard shortcuts for deleting items, blurring items (i.e. removing focus), confirming edits in inputs with Enter, and undo/redo.

Description

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
If this change updates the UI, screenshots/videos are added and a design review is requested
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

…9779 Add support for keyboard shortcuts for deleting items, blurring items (i.e. removing focus), confirming edits in inputs with Enter, and undo/redo.

Copilot

Pull Request Overview

Adds keyboard interaction support to the data modeling diagram (delete via Backspace/Delete, Escape to clear selection, Enter to confirm edits, Undo/Redo shortcuts) plus related tests and selection behavior refinements.

Introduces keepFieldSelection logic to avoid clearing field selection on certain background interactions.
Adds global and local key handling for delete, escape, and undo/redo; updates blur behavior to commit on Enter.
Expands tests to cover Enter submission and undo/redo via keyboard.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
packages/compass-e2e-tests/tests/data-modeling-tab.test.ts	Adds undo/redo keyboard shortcut test sequences.
packages/compass-data-modeling/src/store/diagram.ts	Adds keepFieldSelection flag to background selection action and its handling.
packages/compass-data-modeling/src/components/drawer/use-change-on-blur.tsx	Adds Enter key handling to trigger blur and commit changes.
packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx	Adds tests for Enter-based editing; new helper for submitting with Enter.
packages/compass-data-modeling/src/components/diagram-editor.tsx	Implements keyboard shortcut logic, selection handling, global listeners, and toolbar element tagging.
packages/compass-components/src/index.ts	Exports DiagramProps type.
packages/compass-components/src/hooks/use-throttled-props.tsx	Broadens generic constraint for throttling hook.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

packages/compass-data-modeling/src/components/drawer/diagram-editor-side-panel.spec.tsx

packages/compass-data-modeling/src/components/diagram-editor.tsx

addaleax · 2025-10-18T22:07:36Z

packages/compass-data-modeling/src/components/diagram-editor.tsx

+      // Escape key clears selection
+      else if (!isUnfocusedEvent && event.key === 'Escape') {
+        markHandled();
+        onDiagramBackgroundClicked(true);


When using Esc to blur away from a field, the focus moves to the surrounding ReactFlow node (i.e. the collection node), which marks that node as selected.

I don't think that's the behavior we're going for here, but I think it is an acceptable one (esp. since another Esc will just fully blur the view), and honestly I don't quite know how we could prevent that behavior here

gribnoysup · 2025-10-20T09:12:53Z

packages/compass-data-modeling/src/components/diagram-editor.tsx

+    [onDiagramBackgroundClicked, onNodeClick, onEdgeClick]
+  );
+
+  const onKeyDown = useCallback(


We use useHotkeys elsewhere in compass for hotkey handling, for consistency we should continue using it

Yeah, seems to work just fine – switched in b44e0b5!

gribnoysup · 2025-10-20T09:15:45Z

packages/compass-data-modeling/src/components/diagram-editor.tsx

+    onUndoClick: undoEdit,
+    onRedoClick: redoEdit,


For some cases having the hotkeys defined here makes sense, but I think for some of them you can keep closer to the UI that they are "attached" to, for example instead of adding undo / redo here, you can use useHotkey hook to define these hotkeys near the button elements that do undo / redo

Done in 41e2ff9 👍

packages/compass-data-modeling/src/components/diagram-editor.tsx

gribnoysup · 2025-10-20T12:27:14Z

packages/compass-data-modeling/src/components/diagram-editor.tsx

+  const onSelectionChange = useCallback(
+    ({ nodes, edges }: { nodes: NodeProps[]; edges: EdgeProps[] }) => {
+      // Select the first selected item (if any) in the diagram
+      // since unlike react-flow, we only support single item selections
+      if (nodes.length > 0) {
+        onNodeClick(null, nodes[0]);
+        return;
+      }
+      if (edges.length > 0) {
+        onEdgeClick(null, edges[0]);
+        return;
+      }
+      // If nothing is selected, clear the selection
+      onDiagramBackgroundClicked(true);
+    },
+    [onDiagramBackgroundClicked, onNodeClick, onEdgeClick]
+  );


I'm not sure it's a very good idea to replace previous separate onNodeClick / onFieldClick / onEdgeClick handlers with this one while onSelectionChange doesn't support field selection, this unexpectedly changes the meaning of the background selected action: you're now dispatching it when background is not actually being clicked / selected which introduces faulty assumptions into the redux actions / reduder logic

Yeah no, that's absolutely fair – I think this is honestly a leftover from when I started working on this, right now the Escape hotkey catch seems to work pretty well

gribnoysup · 2025-10-20T12:33:52Z

packages/compass-data-modeling/src/components/diagram-editor.tsx

+  useHotkeys('Backspace', deleteItem, [deleteItem]);
+  useHotkeys('Delete', deleteItem, [deleteItem]);
+  useHotkeys('Escape', () => {
+    onDiagramBackgroundClicked(true);


It might be slightly convenient in the moment to reuse this handler here, but this does break some redux rules around how exactly to define actions, especially as this one has some special logic, different from actual background click, that just unconditionally deselects everything, making it harder to understand the actual logic flow and make changes to the logic without affecting other parts of the app. As escape seems to require some special handling depending on the current state of the app, I would suggest to introduce a dedicated action for it and handle it specially in the reducer

@gribnoysup Does your concern still apply with the current version of this patch? If yes, I don't think I fully understood it 🙂

I'd say it still applies, but to a way lesser extent 😆 I think in the current state after changes you just made it looks good and will be fine to keep as is but I'd like to clarify what I was talking about here, maybe it will be a helpful example to keep in mind (maybe it's already clear to you, then sorry in advance for this wall of text below).

Say we actually notice that because "Esc" is a bit more ambiguous compared to clicking the background, we decide that it should actually have its own special logic based on your current selection.

Someone looking at the existing code might be tempted to do something similar to what you did before, modify the existing "background clicked" action in a way that would make it configurable enough to apply the special logic in the reducer, but this "muddies the waters" in terms of what's actually going on here by mixing unrelated actions into one (over)configured one, now every time a background clicked action needs some changes the "Esc" case should be taken into account and the other way around.

If instead you create a dedicated action like "escape pressed", this issue can be avoided: currently it will have exactly the same behavior in the reducer as the background click, but if needs be, there is a very clear splitting poing for the logic that already exists in the code.

As always, the question how far to push applying these patterns is really not easy to answer in advance and a certain balance needs to be kept, not everything deserves its own new action type. That's why in your current implementation the argument definitely can be made that this is just an extra way to trigger a "background clicked", similar to how undo / redo also just got hotkey listeners without getting a dedicated actions for those.

Ah, I see – yeah, I'd mildly argue that the problem here is that the actions should already be somewhat translated from "specific UI interaction" to "semantic action the user took" – in this case the reducer shouldn't receive the information that the background was clicked, it should receive the information that all items were deselected.

If you like, I'll update this by renaming DIAGRAM_BACKGROUND_SELECTED to ALL_ITEMS_DESELECTED or similar (and likewise for the action constructor/props). Or I can also add a separate action, but that honestly doesn't feel quite right to me.

Yeah, super valid. I see your point and the line here is a bit hard to draw super clearly I think. From my perspective renaming these to "selected" / "deselected" moves them a bit too close to look like these are "setter actions", prone to be misused and bulk dispatched in the future if some new action is added that is supposed to select / deselect as part of its logic. I'm fine with keeping it as "background click" and using in Esc handler 👍

packages/compass-data-modeling/src/components/diagram-editor.tsx

gribnoysup

Two very small, non blocking comments, otherwise LGTM!

gribnoysup · 2025-10-20T16:13:27Z

packages/compass-data-modeling/src/components/diagram-editor-toolbar.tsx

+  // macOS: Cmd+Shift+Z = Redo, Cmd+Z = Undo
+  // Windows/Linux: Ctrl+Z = Undo, Ctrl+Y = Redo
+  useHotkeys('mod+z', onUndoClick, { enabled: step === 'EDITING' }, [
+    onUndoClick,
+  ]);
+  useHotkeys('mod+shift+z', onRedoClick, { enabled: step === 'EDITING' }, [
+    onRedoClick,
+  ]);


Very small nit, not sure if it's worth dealing with right now: we show the default app Edit menu (at least on macOS), which means that you can select Undo / Redo there, but because it's not really just triggering the hotkey, it doesn't do anything, a bit confusing, but not the end of the world, can do a follow-up

Yeah, definitely makes sense – I've created COMPASS-9976 and added a TODO about this, will address that but ideally as a separate PR since it won't have much overlap with the work in this one

packages/compass-data-modeling/src/components/diagram-editor.tsx

feat(data-modeling): add keyboard controls for DM experience COMPASS-…

7fe61c0

…9779 Add support for keyboard shortcuts for deleting items, blurring items (i.e. removing focus), confirming edits in inputs with Enter, and undo/redo.

Copilot AI review requested due to automatic review settings October 18, 2025 22:04

addaleax requested a review from a team as a code owner October 18, 2025 22:04

addaleax requested a review from ivandevp October 18, 2025 22:04

github-actions bot added the feat label Oct 18, 2025

Copilot AI reviewed Oct 18, 2025

View reviewed changes

addaleax commented Oct 18, 2025

View reviewed changes

fixup: copilot cr

d885374

addaleax added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label Oct 18, 2025

gribnoysup reviewed Oct 20, 2025

View reviewed changes

packages/compass-data-modeling/src/components/diagram-editor.tsx Outdated Show resolved Hide resolved

addaleax added 2 commits October 20, 2025 13:42

fixup: use useHotkeys

b44e0b5

fixup: move undo/redo hotkeys to toolbar

41e2ff9

addaleax requested a review from gribnoysup October 20, 2025 11:47

gribnoysup reviewed Oct 20, 2025

View reviewed changes

addaleax added 3 commits October 20, 2025 15:10

fixup: cr

c738187

Merge remote-tracking branch 'origin/main' into 9779-dev

9a1a464

fixup: remove onSelectionChange, keepFieldSelection

adbea50

gribnoysup approved these changes Oct 20, 2025

View reviewed changes

addaleax added 2 commits October 20, 2025 18:32

fixup: cr

72d2d9d

fixup: add TODO re: application menu integration

ce5971c

gribnoysup approved these changes Oct 20, 2025

View reviewed changes

addaleax merged commit 54bc9cc into main Oct 21, 2025
81 of 84 checks passed

addaleax deleted the 9779-dev branch October 21, 2025 09:08

feat(data-modeling): add keyboard controls for DM experience COMPASS-9779 #7474

feat(data-modeling): add keyboard controls for DM experience COMPASS-9779 #7474

Uh oh!

Conversation

addaleax commented Oct 18, 2025

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gribnoysup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gribnoysup Oct 20, 2025 •

edited

Loading

gribnoysup Oct 20, 2025 •

edited

Loading

gribnoysup Oct 20, 2025 •

edited

Loading