Mobile, Desktop: Fixes #13102: Keep list type when reordering items via drag and drop#15038
Open
Ashutoshx7 wants to merge 2 commits intolaurent22:devfrom
Open
Mobile, Desktop: Fixes #13102: Keep list type when reordering items via drag and drop#15038Ashutoshx7 wants to merge 2 commits intolaurent22:devfrom
Ashutoshx7 wants to merge 2 commits intolaurent22:devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughIntercepts ProseMirror drop events for moved NodeSelections; computes a candidate insertion via Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (drag/drop)
participant View as EditorView
participant DragState as DraggingState
participant PosCalc as PositionCalc (posAtCoords/dropPoint)
participant Adjust as adjustListItemDropInsertPos
participant Doc as Document/State
participant Dispatcher as TransactionDispatcher
participant Browser as Browser
User->>View: drop event (coords, dataTransfer)
View->>DragState: read view.dragging (moved? NodeSelection?)
alt moved + node present
View->>PosCalc: posAtCoords -> dropPoint(slice)
PosCalc->>Adjust: provide doc, insertPos, slice
Adjust->>View: return adjustedInsertPos
View->>Doc: create transaction (replaceRangeWith / replaceRange)
Doc->>Dispatcher: dispatch(tr with uiEvent:'drop')
Dispatcher->>View: set selection near insertion, focus
View->>Browser: preventDefault()
else not a moved NodeSelection
View-->>Browser: return false (allow default)
end
Suggested labels: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts (1)
77-98: Parameterise the list-type variants.These two cases exercise the same behaviour with different fixtures, so a
test.eachtable would remove the repetition and make thetask_list_itempath cheap to add as another row later.As per coding guidelines: "Avoid duplicating code in tests; use test.each or shared helpers instead of repeating similar test blocks."♻️ Suggested refactor
- test('keeps dragged bullet list items in a bullet list at list boundary drop positions', () => { - const state = makeState('bullet_list'); - const unadjusted = moveSecondItemAboveFirst(state, false); - const adjusted = moveSecondItemAboveFirst(state, true); - - expect(unadjusted.firstChild.type).toBe(schema.nodes.paragraph); - expect(unadjusted.child(1).type).toBe(schema.nodes.ordered_list); - - expect(adjusted.firstChild.type).toBe(schema.nodes.paragraph); - expect(adjusted.child(1).type).toBe(schema.nodes.bullet_list); - expect(listItemTexts(adjusted.child(1))).toStrictEqual(['two', 'one']); - }); - - test('keeps dragged ordered list items in an ordered list at list boundary drop positions', () => { - const state = makeState('ordered_list'); - const adjusted = moveSecondItemAboveFirst(state, true); - - expect(adjusted.firstChild.type).toBe(schema.nodes.paragraph); - expect(adjusted.child(1).type).toBe(schema.nodes.ordered_list); - expect(adjusted.childCount).toBe(2); - expect(listItemTexts(adjusted.child(1))).toStrictEqual(['two', 'one']); - }); + test.each([ + { + listType: 'bullet_list' as const, + expectedAdjustedType: schema.nodes.bullet_list, + expectedUnadjustedType: schema.nodes.ordered_list, + }, + { + listType: 'ordered_list' as const, + expectedAdjustedType: schema.nodes.ordered_list, + expectedUnadjustedType: undefined, + }, + ])( + 'keeps dragged $listType items in their original list type at list boundary drop positions', + ({ listType, expectedAdjustedType, expectedUnadjustedType }) => { + const state = makeState(listType); + const unadjusted = moveSecondItemAboveFirst(state, false); + const adjusted = moveSecondItemAboveFirst(state, true); + + expect(adjusted.firstChild.type).toBe(schema.nodes.paragraph); + expect(adjusted.child(1).type).toBe(expectedAdjustedType); + expect(adjusted.childCount).toBe(2); + expect(listItemTexts(adjusted.child(1))).toStrictEqual(['two', 'one']); + + if (expectedUnadjustedType) { + expect(unadjusted.firstChild.type).toBe(schema.nodes.paragraph); + expect(unadjusted.child(1).type).toBe(expectedUnadjustedType); + } + }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts` around lines 77 - 98, Replace the two near-duplicate tests with a single parameterized test using test.each: iterate over rows like ['bullet_list', schema.nodes.bullet_list] and ['ordered_list', schema.nodes.ordered_list], call makeState(listType) and moveSecondItemAboveFirst(state, true) for each row, and assert the same expectations (firstChild is paragraph, child(1) is the expected list node, childCount where needed, and listItemTexts matches ['two','one']); reuse moveSecondItemAboveFirst, makeState, listItemTexts and schema node references so adding task_list_item is just adding another row to the table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/editor/ProseMirror/createEditor.ts`:
- Around line 198-199: Replace the direct call to view.focus() inside the custom
drop branch with the module's existing focus helper (instead of calling
view.focus() directly) so focus bookkeeping is consistent; after dispatching
tr.setMeta('uiEvent', 'drop') call the existing focus handler used elsewhere in
this file (the same helper the rest of the module uses) rather than
view.focus().
In `@packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.ts`:
- Around line 28-34: The current logic in adjustListItemDropInsertPos uses two
separate checks on $pos.nodeAfter and $pos.nodeBefore so when both sides are
compatible the nodeAfter branch wins and flips append/prepend semantics; change
this to compute booleans (e.g. afterOk =
$pos.nodeAfter?.type.contentMatch.matchType(draggedListItemType), beforeOk =
$pos.nodeBefore?.type.contentMatch.matchType(draggedListItemType)) and only
adjust insertPos when exactly one side is compatible: if afterOk && !beforeOk
return insertPos + 1; else if beforeOk && !afterOk return insertPos - 1;
otherwise return insertPos unchanged.
---
Nitpick comments:
In `@packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts`:
- Around line 77-98: Replace the two near-duplicate tests with a single
parameterized test using test.each: iterate over rows like ['bullet_list',
schema.nodes.bullet_list] and ['ordered_list', schema.nodes.ordered_list], call
makeState(listType) and moveSecondItemAboveFirst(state, true) for each row, and
assert the same expectations (firstChild is paragraph, child(1) is the expected
list node, childCount where needed, and listItemTexts matches ['two','one']);
reuse moveSecondItemAboveFirst, makeState, listItemTexts and schema node
references so adding task_list_item is just adding another row to the table.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3427f51-e4ce-4f2f-b5da-4287ebe6ef67
📒 Files selected for processing (3)
packages/editor/ProseMirror/createEditor.tspackages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.tspackages/editor/ProseMirror/utils/adjustListItemDropInsertPos.ts
Ashutoshx7
force-pushed
the
fix/keep-list-type-on-drag-drop
branch
from
cd01256 to
deecd5d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts (1)
77-98: Reduce duplicated adjusted-list assertions across these tests.The adjusted-result assertions are repeated; extracting a shared helper will keep intent clearer and future edits safer.
Suggested refactor
+const expectAdjustedListResult = ( + doc: ProseMirrorNode, + listType: 'bullet_list' | 'ordered_list', +) => { + expect(doc.firstChild.type).toBe(schema.nodes.paragraph); + expect(doc.child(1).type).toBe(schema.nodes[listType]); + expect(listItemTexts(doc.child(1))).toStrictEqual(['two', 'one']); +}; + describe('adjustListItemDropInsertPos', () => { test('keeps dragged bullet list items in a bullet list at list boundary drop positions', () => { const state = makeState('bullet_list'); const unadjusted = moveSecondItemAboveFirst(state, false); const adjusted = moveSecondItemAboveFirst(state, true); expect(unadjusted.firstChild.type).toBe(schema.nodes.paragraph); expect(unadjusted.child(1).type).toBe(schema.nodes.ordered_list); - expect(adjusted.firstChild.type).toBe(schema.nodes.paragraph); - expect(adjusted.child(1).type).toBe(schema.nodes.bullet_list); - expect(listItemTexts(adjusted.child(1))).toStrictEqual(['two', 'one']); + expectAdjustedListResult(adjusted, 'bullet_list'); }); test('keeps dragged ordered list items in an ordered list at list boundary drop positions', () => { const state = makeState('ordered_list'); const adjusted = moveSecondItemAboveFirst(state, true); - expect(adjusted.firstChild.type).toBe(schema.nodes.paragraph); - expect(adjusted.child(1).type).toBe(schema.nodes.ordered_list); + expectAdjustedListResult(adjusted, 'ordered_list'); expect(adjusted.childCount).toBe(2); - expect(listItemTexts(adjusted.child(1))).toStrictEqual(['two', 'one']); });As per coding guidelines, "Avoid duplicating code in tests; use
test.eachor shared helpers instead of repeating similar test blocks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts` around lines 77 - 98, Two tests duplicate the same "adjusted" assertions; extract a small shared helper (e.g., assertAdjustedList) and call it from both tests (or convert to test.each) to remove the repeated checks. Implement the helper to accept the state or adjusted doc produced by moveSecondItemAboveFirst(state, true) and assert the firstChild is schema.nodes.paragraph, the second child is the expected list type (schema.nodes.bullet_list or schema.nodes.ordered_list), that childCount (if needed) equals 2, and that listItemTexts(...) equals ['two','one']; update the tests to call makeState and moveSecondItemAboveFirst then delegate the duplicated expect calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts`:
- Around line 77-98: Two tests duplicate the same "adjusted" assertions; extract
a small shared helper (e.g., assertAdjustedList) and call it from both tests (or
convert to test.each) to remove the repeated checks. Implement the helper to
accept the state or adjusted doc produced by moveSecondItemAboveFirst(state,
true) and assert the firstChild is schema.nodes.paragraph, the second child is
the expected list type (schema.nodes.bullet_list or schema.nodes.ordered_list),
that childCount (if needed) equals 2, and that listItemTexts(...) equals
['two','one']; update the tests to call makeState and moveSecondItemAboveFirst
then delegate the duplicated expect calls to this helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aec3b227-2625-4919-b767-861f4c3c68be
📒 Files selected for processing (5)
.eslintignore.gitignorepackages/editor/ProseMirror/createEditor.tspackages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.tspackages/editor/ProseMirror/utils/adjustListItemDropInsertPos.ts
✅ Files skipped from review due to trivial changes (2)
- .eslintignore
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.ts
- packages/editor/ProseMirror/createEditor.ts
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Ashutoshx7
force-pushed
the
fix/keep-list-type-on-drag-drop
branch
from
deecd5d to
4586ff7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/editor/ProseMirror/createEditor.ts`:
- Around line 194-203: The current drop handler always calls
event.preventDefault() and returns true even when no transaction was applied
(when tr.doc.eq(beforeInsert) is true), preventing ProseMirror’s fallback drop
handling; update the logic so that event.preventDefault() and the true return
happen only inside the branch where a change was made (the block guarded by if
(!tr.doc.eq(beforeInsert)) that computes mappedPos/clampedPos, calls
tr.setSelection, focus('createEditor', view), and
view.dispatch(tr.setMeta('uiEvent','drop'))), and let the function fall through
(no preventDefault/return true) when the transaction did not change the
document.
In `@packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts`:
- Around line 68-72: The test uses Array.prototype.forEach on listNode which
violates the github/array-foreach lint rule; replace the forEach call with a
classic for loop that iterates over listNode (e.g., for (let i = 0; i <
listNode.length; i++)) and inside the loop check if listNode[i].type ===
schema.nodes.list_item and then push listNode[i].textContent into result
(keeping the existing logic around result and schema.nodes.list_item and
avoiding use of .forEach).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 746975e8-6830-47e2-9a6c-6a394deb07ea
📒 Files selected for processing (5)
.eslintignore.gitignorepackages/editor/ProseMirror/createEditor.tspackages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.tspackages/editor/ProseMirror/utils/adjustListItemDropInsertPos.ts
✅ Files skipped from review due to trivial changes (3)
- .eslintignore
- .gitignore
- packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.ts
coderabbitai
bot
added
mobile
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/editor/ProseMirror/createEditor.ts (1)
187-192: Add non-null assertion for type safety.When
childCount === 1,firstChildis guaranteed to exist at runtime, but TypeScript types it asNode | null. Adding a non-null assertion makes the intent explicit.♻️ Suggested fix
const isNodeSlice = slice.openStart === 0 && slice.openEnd === 0 && slice.content.childCount === 1; if (isNodeSlice) { - tr.replaceRangeWith(insertPos, insertPos, slice.content.firstChild); + tr.replaceRangeWith(insertPos, insertPos, slice.content.firstChild!); } else { tr.replaceRange(insertPos, insertPos, slice); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editor/ProseMirror/createEditor.ts` around lines 187 - 192, The code branches on isNodeSlice but uses slice.content.firstChild which TypeScript types as Node | null; update the node access to assert non-null when childCount === 1 by replacing slice.content.firstChild with slice.content.firstChild! inside the isNodeSlice branch (the block that calls tr.replaceRangeWith(insertPos, insertPos, slice.content.firstChild)) so the intent is explicit and the compiler knows the node is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/editor/ProseMirror/createEditor.ts`:
- Around line 187-192: The code branches on isNodeSlice but uses
slice.content.firstChild which TypeScript types as Node | null; update the node
access to assert non-null when childCount === 1 by replacing
slice.content.firstChild with slice.content.firstChild! inside the isNodeSlice
branch (the block that calls tr.replaceRangeWith(insertPos, insertPos,
slice.content.firstChild)) so the intent is explicit and the compiler knows the
node is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cab96e81-e0e8-47bd-a805-6ad8d4f5bd72
📒 Files selected for processing (2)
packages/editor/ProseMirror/createEditor.tspackages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/ProseMirror/utils/adjustListItemDropInsertPos.test.ts
Collaborator
|
Thank you for the pull request!
Be aware that the above screen recording shows the desktop TinyMCE-based editor, while this pull request adjusts the mobile ProseMirror-based editor. Please ensure that testing has been done with the correct editor. |
Owner
|
Closing stale PR |
Contributor
Author
|
@laurent22 can you reopen this pr i am working on this |
Contributor
Author
Screencast.From.2026-04-16.22-51-18.mp4sorry for the late reply |
Collaborator
Note: the attached screen recording shows the CodeMirror-based mobile Markdown editor. To switch to the Rich Text Editor, open the three dots menu at the top right of the screen, then click "edit as rich text". |
Contributor
Author
Screencast.From.2026-04-16.23-47-38.mp4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13102
Problem
when dragging list items to reorder them in the rich text editor , the list type would change example bullet list would become numbered list after a frag
Solution
Fixed by adjusting the drop position so the item lands inside the existing list instead of outside it. Falls back to default behavior for non-list drags. Added unit tests.
Screencast.From.2026-04-07.03-24-02.mp4