Repository navigation
GEO-2680: Add drag handles to reorder content blocks - #2239
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds drag-and-drop reordering for top-level editor blocks while preserving relation identities.
Changes:
- Adds floating drag handles, drop indicators, and block movement.
- Persists reordered relation positions using
updateRelation. - Adds unit tests for movement, drop zones, and relation updates.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
partials/editor/editor.tsx |
Integrates and persists block reordering. |
partials/editor/block-reorder.tsx |
Implements drag-and-drop behavior. |
partials/editor/block-reorder.test.ts |
Tests reorder utilities and handle rendering. |
core/utils/publish/publish.ts |
Emits relation update operations. |
core/utils/publish/publish.test.ts |
Tests publishing reordered relations. |
core/types.ts |
Adds relation-update metadata. |
core/sync/use-mutate.tsx |
Classifies relation mutations for publishing. |
core/sync/relation-update.ts |
Determines update-operation eligibility. |
core/sync/relation-update.test.ts |
Tests relation update classification. |
core/state/editor/use-editor.tsx |
Updates existing block relation positions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const persistReorderedBlocks = React.useCallback(() => { | ||
| if (!editableRef.current || !editorRef.current) return; | ||
| const json = editorRef.current.getJSON(); | ||
| trackEditorDocument(json); | ||
| upsertEditorStateRef.current(json); |
There was a problem hiding this comment.
Addressed in f7b7a20. Drag start now calls the same ensureUniqueNodeIds helper used by blur, assigning IDs to missing nodes and deduplicating repeated IDs before the reordered document is persisted. I also added regression coverage for null and duplicate IDs.
| const sensors = useSensors( | ||
| useSensor(PointerSensor, { | ||
| activationConstraint: { distance: 4 }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Addressed in f7b7a20. The block handles now register a KeyboardSensor with custom Up/Down coordinates for the vertical drop boundaries. Each handle remains keyboard-focusable while visually hidden, reveals itself on focus, and supports Space/Enter to pick up/drop plus ArrowUp/ArrowDown to move. Added focus and boundary-navigation tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
apps/web/core/sync/relation-update.ts:13
- This classifies clearing
toSpaceIdas an update, but the publish path only includestoSpacewhen it is truthy (core/utils/publish/publish.ts:101). Existing chip editors can settoSpaceIdtoundefinedwhen selecting the top-ranked/default space (design-system/select-space.tsx:107-113), so the resultingupdateRelationcontains no field that clears the remote space and the UI change disappears after refresh. Handle this transition with an explicit unset operation if the SDK supports it, or route it through a delete/recreate representation that can removetoSpace.
const onlyUpdatesSupportedFields =
base.id === changed.id &&
base.entityId === changed.entityId &&
base.type.id === changed.type.id &&
base.fromEntity.id === changed.fromEntity.id &&
base.toEntity.id === changed.toEntity.id &&
base.spaceId === changed.spaceId;
| const currentIndex = currentBoundary === null ? sourceIndex : toFinalBlockIndex(sourceIndex, currentBoundary); | ||
| const targetIndex = currentIndex + direction; | ||
| const targetBoundary = targetIndex > sourceIndex ? targetIndex + 1 : targetIndex; | ||
|
|
||
| return boundaries.includes(targetBoundary) ? targetBoundary : null; |
There was a problem hiding this comment.
Addressed in 7191731. Keyboard movement now converts the source and current boundary to their ranks in the measured boundary list, computes the next draggable rank, and maps that rank back to the real ProseMirror boundary. This skips excluded empty/tail nodes correctly; regression coverage includes draggable child indexes [0, 2].
| const resizeObserver = new ResizeObserver(measureBlocks); | ||
| const mutationObserver = new MutationObserver(measureBlocks); | ||
| resizeObserver.observe(editorElement); | ||
| mutationObserver.observe(editorElement, { childList: true }); |
There was a problem hiding this comment.
Addressed in a8154a6. The editor mutation observer now watches descendant class, child-list, and character-data changes, so an existing top-level block is remeasured when it transitions between empty and non-empty without being replaced or resized. Layout equality avoids redundant React state updates, and I added regression coverage for the descendant is-empty class transition.
There was a problem hiding this comment.
Follow-up in 9583ae6 after visual testing exposed post-drop flicker: the broad DOM mutation observer is now replaced with an editor-transaction subscription plus frame-coalesced measurement. This still remeasures empty/non-empty transitions, but only after ProseMirror has settled the DOM, and the drag listeners stay mounted across start/drop.
|
@ohohoreilly Thanks for the thorough review. Addressed in 1758e6f:\n\n1. Flicker blast radius: the ProseMirror dispatch, JSON read, position calculation, and in-memory relation update all run synchronously within the same drag-end event. I removed the misleading async marker from makeBlocksRelations as part of making that invariant explicit. There is no paint/input boundary where the UI can show the old order while the store has the new one. The visual/scroll defect is now prominent at the top of the PR and tracked in GEO-2693.\n\n2. First/last positions: extracted the position calculation into a pure helper, added direct first/last reorder coverage, and exclude the moved relation from the position-ordered fallback set so correctness no longer relies on its stale position.\n\n3. Classifier naming: renamed canPublishRelationUpdate to isExistingRelationWithUnchangedIdentity and updated its comments/tests. It now describes the identity/existence check it actually performs without implying a changed-field whitelist. The pre-existing verified payload gap remains called out in the PR description.\n\n4. PR description: replaced the bisect/debug log with the known issue, blast-radius assessment, implementation, review seams, and current validation.\n\nFocused result after rebasing onto the latest master merge: 80 tests and ESLint pass. |
| const nextLayout = Array.from(editorElement.children).flatMap((element, childIndex) => { | ||
| if (!(element instanceof HTMLElement) || !isDraggableBlock(element)) return []; |
There was a problem hiding this comment.
Addressed in ed5b1fb. Block measurement now enumerates top-level document children by ProseMirror positions and resolves each node with view.nodeDOM. The hover lookup uses the same measured element-to-child-index mapping, so direct gap-cursor widgets cannot shift drag indexes. Added regression coverage with a direct .ProseMirror-gapcursor between two blocks.
| const resizeObserver = new ResizeObserver(measureBlocks); | ||
| const mutationObserver = new MutationObserver(measureBlocks); | ||
| resizeObserver.observe(editorElement); | ||
| mutationObserver.observe(editorElement, { childList: true }); |
There was a problem hiding this comment.
Addressed in ed5b1fb. Editor update events now schedule one frame-coalesced measurement after ProseMirror settles the DOM, catching empty/non-empty transitions even when the top-level element is retained. Scheduling is disabled while a drag is active, so the drop transaction does not add another post-drop measurement cycle.
| const identityIsUnchanged = | ||
| base.id === changed.id && | ||
| base.entityId === changed.entityId && | ||
| base.type.id === changed.type.id && | ||
| base.fromEntity.id === changed.fromEntity.id && | ||
| base.toEntity.id === changed.toEntity.id && | ||
| base.spaceId === changed.spaceId; |
There was a problem hiding this comment.
Addressed in 1b32b4f. Existing relations whose endpoint or other identity field changes are now staged as deleteRelation(old ID) plus createRelation(fresh ID), preventing the backend no-op caused by reusing a committed relation ID. Unpublished local relations can still change identity before their first create, while position-only edits continue through updateRelation. Added regression coverage for all three cases.
| const handleDragEnd = (event: DragEndEvent) => { | ||
| const sourceIndex = event.active.data.current?.childIndex; | ||
| const dropBoundary = event.over?.data.current?.boundary; | ||
|
|
||
| if ( | ||
| typeof sourceIndex === 'number' && | ||
| typeof dropBoundary === 'number' && | ||
| moveTopLevelBlock(editor, sourceIndex, dropBoundary) | ||
| ) { | ||
| onReorder(); | ||
| } |
There was a problem hiding this comment.
Addressed in f9b494a. Pointer drops now map the source and target boundary to their ranks in the measured draggable layout before deciding whether to persist. This treats both sides of the source visible slot as a no-op even when excluded empty nodes make ProseMirror child indexes sparse, matching the keyboard path. Added regression coverage for contiguous and sparse boundary sets.
| opacity: isDragging ? 0 : visible || isFocused ? 1 : 0, | ||
| pointerEvents: visible || isFocused ? 'auto' : 'none', |
There was a problem hiding this comment.
Addressed in cc59046. Block handles now detect coarse or hoverless primary pointers and remain visible/pointer-enabled on those devices, while preserving hover-only visibility for mouse input. The activator also uses touch-action: none so dnd-kit can own the gesture, and handles stop intercepting events during an active drag. Added regression coverage for the hoverless/coarse-pointer state.
Known issue\n\nDropping a reordered block can visibly flicker or scroll surrounding blocks. This is tracked in GEO-2693.\n\nThe ProseMirror move transaction and relation-position store update execute synchronously in the same drag-end event. There is no await or event-loop boundary between them, so the browser cannot paint or accept a second drag while the editor and store disagree about order. The known blast radius is visual/scroll behavior, not an old-UI/new-store ordering window.\n\n## Summary\n\n- show a six-dot drag handle when hovering a top-level content block or its left gutter in edit mode\n- support pointer and keyboard reordering with a visible drop indicator and edge auto-scroll\n- persist the moved block by updating its existing Blocks relation position\n- preserve relation identity rather than deleting and recreating the relation\n- assign and deduplicate missing block IDs before drag persistence\n\n## Implementation\n\n- uses classic dnd-kit with a floating drag overlay and measured top-level drop boundaries\n- maps DOM elements through ProseMirror positions so decoration widgets cannot shift document indexes\n- remeasures empty/non-empty editor transitions on a frame after ProseMirror updates, while suppressing that schedule during an active drag\n- handles sparse draggable indexes for keyboard movement\n- generates the moved relation position between its neighbours, excluding the moved relation stale position\n- uses the existing first relation as the upper bound for multiple adjacent insertions at the beginning\n- publishes existing relation changes through updateRelation, including explicit optional toSpace unsets\n- names the update classifier for what it verifies: an existing relation with unchanged identity\n\n## Review seams\n\n- first-position, last-position, and adjacent beginning-insertion fractional generation are covered directly\n- gap-cursor decorations are covered without relying on raw DOM child indexes\n- existing published relations use updateRelation; unpublished local relations remain createRelation operations\n- verified remains outside the SDK payload in both the existing create path and this update path; this PR does not change that pre-existing behavior\n\n## Testing\n\n- 82 focused tests pass across block positioning, drag behavior, node IDs, relation mutation, and publishing\n- focused ESLint passes\n- TypeScript reaches only the existing unrelated test typing errors in governance and entity-response tests\n\nLinear: GEO-2680