feat(editor): streamline cell editing and navigation with improved keyboard support #12770
Conversation
…handling for cell editing in both PC and virtual table hotkeys controllers fix(hotkeys): streamline selection editing logic to ensure consistent behavior when editing cells in both PC and virtual table hotkeys controllers
|
|
WalkthroughThe changes update keyboard event handling in table view controllers to refine cell editing and navigation. Enter, Tab, and Shift-Tab keys now exit editing mode before moving focus. Typing a character while a cell is selected (but not editing) immediately starts editing with that character. No exported API signatures were altered. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableHotkeysController
participant TableView
User->>TableHotkeysController: Presses character key (cell selected, not editing)
TableHotkeysController->>TableView: Set cell value to character
TableHotkeysController->>TableView: Enter editing mode
TableHotkeysController-->>User: Prevent default, cell is now editing
User->>TableHotkeysController: Presses Enter/Tab/Shift-Tab (while editing)
TableHotkeysController->>TableView: Exit editing mode
TableHotkeysController->>TableView: Move focus (down/right/left)
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
NorkzYT
changed the title
|
Tests will be added soon. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #12770 +/- ##
==========================================
- Coverage 55.91% 55.65% -0.27%
==========================================
Files 2652 2652
Lines 125440 125478 +38
Branches 19948 19896 -52
==========================================
- Hits 70142 69829 -313
- Misses 53550 53895 +345
- Partials 1748 1754 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Some tests have failed here, you might need to update them. Feel free to let me know if you need to run the test pipeline. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
blocksuite/affine/data-view/src/view-presets/table/pc-virtual/controller/hotkeys.ts (1)
192-204: 🛠️ Refactor suggestionShift-Tab has the identical race condition
Replicate the commit/flush guard introduced for
Tab/Enterto keep behaviour consistent in all horizontal-navigation paths.
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/view-presets/table/pc-virtual/controller/hotkeys.ts (1)
401-431: Edge-cases in the new type-to-edit handler
Area-selection: the guard only checks for
TableViewRowSelection, so a multi-cell area selection will still fall through and overwrite the first focused cell, which may surprise users.
– Add&& !selection.rowsSelection && !selection.columnsSelectionto restrict to a single-cell focus.Non-BMP characters:
event.key.length === 1filters out emojis and many CJK characters (length > 1 due to UTF-16). If textual columns are expected to accept them, considerevent.key.length > 0 && event.key !== 'Enter'instead.Down-stream handlers:
preventDefault()is called, but you should alsoevent.stopPropagation()to avoid duplicated handling by other listeners registered after this one.if ( selection && !TableViewRowSelection.is(selection) && + !selection.rowsSelection && + !selection.columnsSelection && !selection.isEditing && !event.metaKey && !event.ctrlKey && !event.altKey && - event.key.length === 1 + event.key.length > 0 && + event.key !== 'Enter' ) { @@ event.preventDefault(); + event.stopPropagation(); return true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/view-presets/table/pc-virtual/controller/hotkeys.ts(3 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/controller/hotkeys.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/src/view-presets/table/pc/controller/hotkeys.ts
blocksuite/affine/data-view/src/view-presets/table/pc-virtual/controller/hotkeys.ts
Show resolved
Hide resolved
blocksuite/affine/data-view/src/view-presets/table/pc-virtual/controller/hotkeys.ts
Show resolved
Hide resolved
…unit tests feat(hotkeys.unit.spec.ts): implement unit tests for TableHotkeysController and VirtualHotkeysController to ensure correct hotkey functionality chore(vitest.config.ts): create vitest configuration file to set up testing environment and coverage reporting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
blocksuite/affine/data-view/src/__tests__/hotkeys.unit.spec.ts (4)
10-46: ImprovecreateLogictyping & DRY-ness
createLogiccurrently returnsanyeverywhere and stubs out a large surface area.
This hides real compile-time regressions and forces heavy casting (as any) later in the tests.Consider:
- Declaring minimal interfaces for
View,UI,SelectionController, andLogicwith only the members exercised by the tests.- Returning those concrete types from
createLogic.- Re-using the helper across both describe blocks by exporting it (or moving to a shared
helpers.ts).-const logic: any = { +interface MockLogic { /* minimal surface */ } +const logic: MockLogic = { /* … */ }This keeps the test harness honest and makes future refactors to the real controllers safer.
48-60: Add expectation for UI refresh side-effectThe Backspace path should trigger an
ui.requestUpdate()according to the production code, yet the test only asserts this indirectly after deleting rows. Make the intent explicit:-logic.hotkeys.Backspace(); +logic.hotkeys.Backspace(); +expect(ui.requestUpdate).toHaveBeenCalled();This guards against regressions where the deletion still happens but the view no longer refreshes.
62-87: Cover modifier-key edge casesThe “starts editing on character key” test covers the happy path. It would be valuable to assert that the same character press with
metaKey | ctrlKey | altKeydoes not enter edit mode, matching typical spreadsheet behaviour.['metaKey', 'ctrlKey', 'altKey'].forEach(mod => { const evt = { key: 'A', metaKey: false, ctrlKey: false, altKey: false, preventDefault: vi.fn() } as any; evt[mod] = true; logic.keyDown({ get: () => ({ raw: evt }) }); expect(selectionController.selection.isEditing).toBe(false); });This ensures hotkey handling doesn’t collide with browser/OS shortcuts.
90-120: Deduplicate duplicate test logic with parameterised runnerThe virtual-table test repeats almost the same arrange-act-assert block as the standard table test.
Usedescribe.eachor a helper to run the same suite against both controller classes, reducing maintenance cost:const cases = [ ['Table', TableHotkeysController], ['VirtualTable', VirtualHotkeysController], ] as const; describe.each(cases)('%sHotkeysController', (_name, Ctrl) => { // shared tests … });Future behavioural parity changes will need to be updated in only one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
blocksuite/affine/data-view/package.json(1 hunks)blocksuite/affine/data-view/src/__tests__/hotkeys.unit.spec.ts(1 hunks)blocksuite/affine/data-view/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- blocksuite/affine/data-view/package.json
- blocksuite/affine/data-view/vitest.config.ts
🔇 Additional comments (1)
blocksuite/affine/data-view/src/__tests__/hotkeys.unit.spec.ts (1)
1-4: Verify file-extension correctness in ESM/TS setupThe test file is TypeScript (
*.ts) but the relative imports explicitly end with.js.
Iftsconfig.jsondoes not haveallowImportingTsExtensionsor the build pipeline rewrites extensions, this will break module resolution at compile time (and IntelliSense).Please double-check that:
- Consumers compile the sources with the same file-extension mapping.
- The Vitest runner is configured with
moduleFileExtensions/resolve.extensionsthat include.jsfor TS sources or usests-node’s--experimental-specifier-resolution=node.If none of the above is true, change the paths to omit the extension or use
.ts:-import { TableHotkeysController } from '../view-presets/table/pc/controller/hotkeys.js'; -import { TableHotkeysController as VirtualHotkeysController } from '../view-presets/table/pc-virtual/controller/hotkeys.js'; +import { TableHotkeysController } from '../view-presets/table/pc/controller/hotkeys'; +import { TableHotkeysController as VirtualHotkeysController } from '../view-presets/table/pc-virtual/controller/hotkeys';
…o ensure proper state management during editing sessions
|
When you can please re-run the test pipeline. |
|
|
… TEXTAREA to improve user experience in form fields
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
I have fixed all issues and now the CI will fully pass no problem. |
There are currently two type errors remaining, which can be found in the CI. I will review this PR in the coming days. |
|
I see the type error. I thought I had committed that part. Will fix. |
…is editing to streamline the editing process
|
There you go. Now this should be done as I have ran |
improved-cell-editing.mp4
This PR addresses #12769 and improves table editing UX by making Enter commit changes and move focus down, and Tab/Shift+Tab move focus horizontally—matching spreadsheet-like behavior.
Typing now immediately enters edit mode for selected cells without double-clicking.
These updates apply to both the standard and virtual table views.
Summary by CodeRabbit