Fix table context menu actions and dialog-backed cell splitting#243
Fix table context menu actions and dialog-backed cell splitting#243alokwhitewolf wants to merge 1 commit intoeigenpal:mainfrom
Conversation
|
@alokwhitewolf is attempting to deploy a commit to the EigenPal Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Hey @alokwhitewolf, good stuff! Thank you for contributing, I reviewed and left a couple of comments, mostly about code duplication
| @@ -0,0 +1,352 @@ | |||
| import type { Node as PMNode } from 'prosemirror-model'; | |||
There was a problem hiding this comment.
Two things about this file:
-
Wrong package — this is pure ProseMirror logic with zero React. It should live in
packages/core(e.g. alongside TableExtension or in a table utils module), not inpackages/react/src/components/. -
~350 lines duplicated with TableToolbar.tsx — the anchor collection, column-width splitting, coordinate adjustment, and table rebuild algorithm here is nearly identical to
collectDocumentTableAnchors/splitTableColumnWidths/splitTableCellin TableToolbar.tsx. The only difference is the data types (PMNode vs Document-model Table).
Right now any change one path has to be manually mirrored in the other.
| } | ||
|
|
||
| function collectPMTableAnchors( | ||
| node: PMNode, |
There was a problem hiding this comment.
This is the third copy of the same anchor-collection algorithm in this PR (also in tableSplit.ts:collectTableAnchors and TableToolbar.tsx:collectDocumentTableAnchors). All three walk the table, build an occupied grid, and produce { row, col, rowspan, colspan } anchors.
The types differ (PMNode vs TableCell) but the grid logic is identical. Worth extracting into a generic helper that takes a row-iterator and a cell-span-getter.
| const attrs = node.attrs as TableAttrs; | ||
| const { anchors, totalCols } = collectPMTableAnchors(node, documentCounts); | ||
| const anchorByStart = new Map<string, PMTableCellAnchor>(); | ||
| const anchorByCoveredSlot = new Map<string, PMTableCellAnchor>(); |
There was a problem hiding this comment.
This anchorByStart / anchorByCoveredSlot + rebuild loop is the same pattern used in tableSplit.ts:splitActiveTableCell and TableToolbar.tsx:splitTableCell. Here it's used for the save round-trip (PM → DOCX), which means it now runs on every table save — not just split operations.
The old convertPMTableRow was simpler but didn't handle post-split merged cells correctly, so the rewrite is justified. Just flagging that this is a hot path — any bug here silently corrupts saved DOCX files. Consider adding a unit test that round-trips a table with mixed merged/unmerged cells through toProseDoc → fromProseDoc and asserts the XML is preserved.
| const handleEditorContextMenu = useCallback((e: React.MouseEvent) => { | ||
| const target = e.target as HTMLElement | null; | ||
| if (target?.closest('.paged-editor__pages')) { | ||
| return; |
There was a problem hiding this comment.
This early return means right-clicks on the visible pages now silently skip this handler and rely on PagedEditor's handleContextMenu callback instead. That works, but it's an implicit coupling — if someone removes or changes the PagedEditor handler, the context menu disappears with no error.
A short comment explaining the delegation would help:
// Visible pages handle their own context menu via PagedEditor.handleContextMenu|
|
||
| return ( | ||
| <div style={overlayStyle} onClick={onClose} onKeyDown={handleKeyDown}> | ||
| <div |
There was a problem hiding this comment.
onKeyDown is on the overlay div, which isn't focusable by default. This works because clicks inside the dialog give focus to the inputs, and key events bubble up. But if the dialog opens and the user hasn't clicked anything yet, Escape/Enter won't fire.
Minor — could add tabIndex={-1} and autoFocus on the dialog container, or move the keydown listener to a useEffect with document.addEventListener.
| } | ||
|
|
||
| export function splitActiveTableCell( | ||
| state: EditorState, |
There was a problem hiding this comment.
This is the core split algorithm. The same logic (anchor adjustment loop with deltaRows/deltaCols, band intersection checks, and full table rebuild) is duplicated in TableToolbar.tsx:splitTableCell for the document-model path.
Both are needed because the editor has two table representations (PM nodes vs Document model), but the algorithm is identical — only the data access differs. A bug fix in one needs to be mirrored in the other.
At minimum, add a comment at the top of this function pointing to its counterpart:
// Mirror: TableToolbar.splitTableCell() — same algorithm for the Document-model path| } | ||
|
|
||
| function collectDocumentTableAnchors(table: Table): { | ||
| anchors: DocumentTableAnchor[]; |
There was a problem hiding this comment.
This is the document-model counterpart of tableSplit.ts:collectTableAnchors. Same grid-walk algorithm, but reads gridSpan / vMerge (OOXML attributes) instead of PM's colspan / rowspan.
The two functions will need to stay in sync. Add a cross-reference comment so the next person editing one knows to check the other.
2 participants
Summary
Merge cellsandSplit cellto the right-click table menuSplit celldialog-backed with explicit row/column input and unify that behavior across the ProseMirror and legacy table-selection pathsFixes #242.
Before
Merge cells/Split cellwere missing from the right-click menusplitCellbehavior was closer to unmerge than true row/column splitAfter
- right-click table actions execute correctly
|Merge cellsandSplit cellare available in the right-click menuSplit cellopens a row/column dialog and performs a real structural splitVerification
bunx tsc -p packages/react/tsconfig.json --noEmitnpx playwright test e2e/tests/table-context-menu.spec.ts --project=chromium --workers=1npx playwright test e2e/tests/table-merge-split.spec.ts --project=chromium --workers=1npx playwright test e2e/tests/table-add-column-regression.spec.ts --project=chromium --workers=1