feat(common): add a source/zone drag abstraction on sortablejs - #15086
feat(common): add a source/zone drag abstraction on sortablejs#15086habibayman wants to merge 10 commits into
Conversation
github-actions
Bot
added
DEV: renderers
|
👋 Hi @habibayman, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
habibayman
changed the title
Build Artifacts
Smoke test screenshot |
habibayman
changed the title
🔵 Review postedLast updated: 2026-07-27 03:24 UTC |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15086 — the sortable → draggable migration lands cleanly: the drag logic is real composables instead of the renderless class-poking Draggable/DragHandle pair, the universe/region/item/handle vocabulary matches #14891, and all five call sites are migrated with equivalent data flow. CI passing. No blocking findings.
Manual QA exercised the four reachable call sites (lesson resources table, quiz section order, questions accordion, QTI order interaction) by pointer, touch-type pointer and keyboard, in LTR and RTL, plus axe-core on every affected route: behaviour matches develop everywhere, the @dragend fix reproduces as described, and no new a11y violation or console error is attributable to this PR. Rearrange-channels reorder could not be exercised (one channel installed) — it renders correctly and reports 0 axe violations.
Findings, all inline:
- suggestions — DOM revert runs on no-op drags;
sortable/clone/delayare snapshotted at mount whilecapacity/disabled/acceptsare live;disabledonly blocks incoming drops; clone mode shares the item object reference; keyboard-move announcement reports the pre-move order; new specs use@vue/test-utils+wrapper.vm._provided; two focus-out branches lost coverage vs the deletedDragContainer.spec.js; the affordance stylesheet is now global where it used to be scoped. - nitpicks — handle
cursorcollision inRearrangeChannelsPage, dropped stable-key rationale, stalekey, deadDRAG_CLASS,acceptsgets a mutator.
One scope note for the summary rather than a line: nothing outside components/draggable/ uses <DraggableUniverse>, so the source→zone half of the abstraction (capacity, accepts, clone, getRegion, itemMovedToRegion$) is exercised only by Jest — #14891's first four acceptance criteria rest entirely on the unit tests until a real consumer lands. That looks like the intended scope split, but a dev-only sandbox route (à la /qti_sandbox) would make the zone path browser-testable before the feature PR depends on it.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| const { item, clone, from, to, oldIndex, oldDraggableIndex, newDraggableIndex, pullMode } = evt; | ||
|
|
||
| // 1. Undo SortableJS's DOM mutation | ||
| removeNode(item); |
There was a problem hiding this comment.
suggestion: The revert runs before the no-op check below. SortableJS fires onEnd for any completed drag, including press-and-hold-then-release where oldIndex === newIndex and the DOM was never mutated — so this detaches and re-inserts the item subtree for nothing, discarding focus inside it and reloading any nested browsing context (an <iframe> from ContentViewer in QuestionsAccordion, say). DragContainer left the DOM alone on a no-op, and vuedraggable does this pair inside onDragUpdate/onDragAdd/onDragRemove rather than unconditionally in onEnd. Moving the to === from && oldDraggableIndex === newDraggableIndex guard above step 1 gets the same result with no DOM work.
| universe.registerRegion(el, regionApi); | ||
| el.addEventListener('focusout', handleFocusOut); | ||
|
|
||
| sortable = new Sortable(el, { |
There was a problem hiding this comment.
suggestion: Half of the region props are live, half are frozen here. put: canAccept is a closure, so capacity, disabled and accepts are re-read on every drop check — good. But sort and pull are read as plain values at mount, as is universe.sortableDefaults.delay, so :sortable="isEditing" / :clone="mode === 'copy'" / DraggableUniverse's delay silently keep their mount-time value forever. No current call site flips them, so nothing is broken — but it's a trap baked into a new shared API. Either watch and call sortable.option('sort', …) / sortable.option('group', …) (SortableJS supports runtime updates), or document them as initial-value-only in the prop comments.
| default: true, | ||
| }, | ||
| // for the region | ||
| disabled: { |
There was a problem hiding this comment.
suggestion: disabled reads as "this region is inert", but its only consumer is canAccept, which is wired to group.put — and SortableJS consults put only for cross-list drops (_onDragOver branches on isOwner before checkPut; same-list moves use options.sort). So a disabled region still lets its own items be reordered and dragged out. Either set SortableJS's own disabled option alongside it, or rename to something that says what it does (acceptsDrops, locked).
| return; | ||
| } | ||
| const movedItem = props.items[oldDraggableIndex]; | ||
| target.insertAt(movedItem, newDraggableIndex); |
There was a problem hiding this comment.
suggestion: In clone mode the source keeps movedItem and the target receives the identical object, not a copy. Dropping one source item into two regions then puts one object in two arrays (mutating it in one place mutates both), and removing it from a target by identity can't distinguish copy from original. vuedraggable exposes a clone function prop (clone: original => ({...original, id: uuid()})) so the consumer decides what a copy means. Nothing exercises clone yet, but as written value-copy semantics are impossible without patching update:items after the fact — worth either a clone-transform prop or an explicit note that clone yields a shared reference.
| return props.accepts(universe.draggedItem.value, universe.activeRegion.value); | ||
| } | ||
|
|
||
| function handleFocusOut(event) { |
There was a problem hiding this comment.
suggestion: Not a regression (moved verbatim from DragContainer), but this PR re-homes the code so it's the natural place to fix: a keyboard reorder announces the pre-move order instead of the move. Vue's re-render detaches and re-inserts the moved row, blurring the focused move button and firing focusout with relatedTarget === null, so the guard on line 124 doesn't suppress it — and the announcement clobbers DragSortWidget's itemMovedToPosition$. Observed on the lesson resources table, one click of row 1's "move down":
announced : "Current order: 1. Adding to 10, 2. 1. Introduction to Studio, …"
resulting DOM order: ["1. Introduction to Studio", "Adding to 10", …]
Expected "Adding to 10 moved to position 2 of 4". A cheap guard: defer the check to requestAnimationFrame and skip it when document.activeElement is still inside the region, or suppress while a keyboard move is in flight.
| return next; | ||
| } | ||
|
|
||
| function addBounce(node) { |
There was a problem hiding this comment.
nitpick: DragContainer carried the rationale for this hand-rolled bounce, including the non-obvious half: the moved DOM node survives Vue's re-render because call sites key their items stably, which is an unwritten requirement on every DraggableItem consumer. Only the "SortableJS has no equivalent" half survives, over in classDefinitions.js:15. Worth keeping the constraint next to the code that depends on it.
|
|
||
| <DragContainer | ||
| <DraggableRegion | ||
| key="drag-container" |
There was a problem hiding this comment.
nitpick: Stale name — this is a DraggableRegion now. It's a static key on a non-v-for, non-v-if element, so dropping it entirely works too.
| // The item being dragged, still in its source list (chosenClass). | ||
| export const CHOSEN_CLASS = 'draggable-item--chosen'; | ||
| // The copy under the cursor in native-drag mode (dragClass). | ||
| export const DRAG_CLASS = 'draggable-item--drag'; |
There was a problem hiding this comment.
nitpick: dragClass only applies in native HTML5 drag mode, and sortableDefaults sets forceFallback: true, so .draggable-item--drag can never be applied; CHOSEN_CLASS is applied but has no rule in draggable.scss. Harmless, just two constants suggesting styling hooks that don't exist. While here: these names are re-typed as literals in draggable.scss and DraggableHandle.vue, so a rename silently orphans the styles — a one-line comment saying so would help.
| get label() { | ||
| return props.label; | ||
| }, | ||
| insertAt(item, index) { |
There was a problem hiding this comment.
nitpick: insertAt is the right internal contract for a source region handing an item to a target, but the same regionApi object is passed to consumer code as the second argument of accepts (line 115), so any consumer predicate can mutate the source region mid-drag. Passing a narrowed { items, label } view to accepts keeps the mutator private to the universe ("small interfaces").
| }, | ||
| handleQuestionOrderChange({ newArray }) { | ||
| this.$emit('sort', { newArray }); | ||
| handleDragEnd() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Splitting this out of the sort handler fixes a real bug — QA confirms user-select on the accordion headers goes text → none → text across a drag that reorders nothing, where on develop it stayed none until the next successful sort.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15086 — no new commits since the last review (HEAD is still 980253fb), so 0 of 14 prior findings are resolved; 13 still open. CI passing. Manual QA re-ran across all five migrated call sites: no behavioural regressions, no new a11y violations, and the @dragend user-select fix is confirmed in the browser. Seven new findings this round, all from the frontend/a11y lens plus one latent cross-bundle issue — none blocking.
The one thing worth repeating at the summary level: the source→zone half of this API (capacity, clone, accepts, disabled, label, DraggableUniverse itself) has no consumer in the running app, so the linked issue's first four ACs have unit-test evidence only. Not a blocker — the tests do cover them — but the first real consumer will be the first browser exercise of that path.
Prior-finding status
UNADDRESSED — packages/kolibri-common/components/draggable/useDraggableRegion.js:74 — DOM revert runs on no-op drags
UNADDRESSED — packages/kolibri-common/components/draggable/useDraggableRegion.js:151 — sort/pull/delay frozen at mount while capacity/disabled/accepts are live
UNADDRESSED — packages/kolibri-common/components/draggable/DraggableRegion.vue:50 — disabled only blocks incoming drops
UNADDRESSED — packages/kolibri-common/components/draggable/useDraggableRegion.js:96 — clone mode shares the item object reference
UNADDRESSED — packages/kolibri-common/components/draggable/useDraggableRegion.js:118 — keyboard-move announcement reports the pre-move order
UNADDRESSED — packages/kolibri-common/components/draggable/tests/DraggableRegion.spec.js:1 — new specs use @vue/test-utils and wrapper.vm._provided
UNADDRESSED — packages/kolibri-common/components/draggable/tests/DraggableRegion.spec.js:254 — two focus-out branches lost coverage vs the deleted DragContainer.spec.js
UNADDRESSED — packages/kolibri-common/components/draggable/DraggableRegion.vue:71 — affordance stylesheet now global; bounce-in keyframe name
UNADDRESSED — packages/kolibri-common/components/draggable/DraggableHandle.vue:37 — handle cursor specificity collision in RearrangeChannelsPage
UNADDRESSED — packages/kolibri-common/components/draggable/useDraggableRegion.js:51 — dropped stable-key rationale for the bounce
UNADDRESSED — kolibri/plugins/coach/frontend/views/common/QuestionsAccordion.vue:38 — stale key="drag-container"
UNADDRESSED — packages/kolibri-common/components/draggable/classDefinitions.js:14 — dead DRAG_CLASS, unstyled CHOSEN_CLASS
UNADDRESSED — packages/kolibri-common/components/draggable/useDraggableRegion.js:37 — accepts receives a region mutator
ACKNOWLEDGED — kolibri/plugins/coach/frontend/views/common/QuestionsAccordion.vue:318 — praise for the @dragend fix
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| const DraggableUniverseSymbol = Symbol('draggableUniverse'); | ||
|
|
||
| // Backs the generated group name so distinct universes never share a group | ||
| let universeCounter = 0; |
There was a problem hiding this comment.
suggestion: Auto-generated universe names can collide across plugin bundles. kolibri's exposes list is only kolibri-logging, vue, vuex, so kolibri-common modules are bundled per plugin — each bundle gets its own copy of this module and its own counter starting at 0. Two standalone regions that happen to be first in their respective bundles both get draggable-universe-1.
SortableJS matches groups globally by name. A function put goes through toFn, which calls it and treats true as "accept" with no group-name check, and canAccept returns true for a foreign-universe drag once the names match. The drop is accepted, handleEnd reverts the DOM, universe.getRegion(to) misses, and it falls into the "dropped outside this universe" branch — no data corruption, but the item visibly lands in an unrelated list and snaps back.
Not reachable today (the only cross-bundle pairing is QuestionsAccordion embedding a non-interactive OrderInteraction, which renders no region), so this is a trap baked into a new shared API rather than a live bug. Cheapest fix is to stop trusting the name and check membership instead — SortableJS already hands you the source instance:
function canAccept(to, from) {
// `from` is the source Sortable instance; reject drags from another universe
if (!universe.getRegion(from.el)) {
return false;
}
...
}That also makes the getRegion(to) miss in handleEnd unreachable rather than a silent no-op.
| newDraggableIndex: 0, | ||
| pullMode: true, | ||
| }); | ||
| expect(sendPoliteMessage).toHaveBeenCalledWith('Moved to Gap 1'); |
There was a problem hiding this comment.
suggestion: These assertions hard-code translated English. Both this and 'Current order: 1. First, 2. Second, 3. Third' at :268 are createTranslator output (itemMovedToRegion, currentOrder), so a copy edit to 'Moved into {region}' breaks a unit test that has nothing to do with the change, with a failure message pointing at the drag logic.
The sibling spec this PR touches already does it right — OrderInteraction.spec.js imports dragSortStrings and builds expectations from it:
const { itemMovedToRegion$, currentOrder$ } = dragSortStrings;
expect(sendPoliteMessage).toHaveBeenCalledWith(itemMovedToRegion$({ region: GAP_LABEL }));with GAP_LABEL shared between the mountUniverse({ label: … }) setup at :225 and the assertion. Keep '1. First, 2. Second, 3. Third' literal — that fragment is assembled in useDraggableRegion.js:131-134, not translated, and it's the part the test is actually about.
| type: Function, | ||
| default: () => true, | ||
| }, | ||
| label: { |
There was a problem hiding this comment.
suggestion: label is announcement-only — it feeds itemMovedToRegion$({ region: target.label }) in useDraggableRegion.js:103 and never reaches the DOM. So a region that wants both the drop announcement and an accessible name has to pass the same string twice, and the only call site that names its region today does it the other way, via attrs: { 'aria-label': orderListLabel$() } (OrderInteraction.vue:239) — meaning it announces nothing on a cross-region drop.
Two sources of truth for one label will drift. Binding label as the fallback accessible name collapses them:
<component :is="tag" ref="root" :aria-label="$attrs['aria-label'] || label">Then OrderInteraction passes label: orderListLabel$() and drops the attrs entry.
| context: | ||
| 'Live region announcement of the full list order after focus leaves the reorderable list', | ||
| }, | ||
| itemMovedToRegion: { |
There was a problem hiding this comment.
suggestion: itemMovedToRegion is the one new string here, and it only fires when a target region has a non-null label (useDraggableRegion.js:103). None of the five migrated call sites sets label, and nothing outside components/draggable/__tests__/ mounts a <DraggableUniverse>, so the cross-region drop that reaches this branch can't happen in the shipped app.
It'll still go to Crowdin and be translated into every supported language for a path no user can hit. Either land the first drop-zone consumer here, or hold this one message back until it arrives — the rest of the API ships fine without it. Not blocking; flagging it because translator effort is the one cost you don't get back by deleting the code later.
| visibility: hidden; | ||
| } | ||
|
|
||
| .draggable-item--placed { |
There was a problem hiding this comment.
suggestion: The drop bounce ignores prefers-reduced-motion, as does animation: 150 in useDraggableUniverse.js:49. Both carried over verbatim from DragContainer.vue, so not a regression — but the rule has moved from one component's scoped block into a shared unscoped stylesheet every future DraggableRegion consumer inherits, which makes this the moment to fix it once.
Kolibri already honours the preference in both CSS and JS (LearningActivityBar.vue:664, AuthBase.vue:640, PicturePasswordGrid.vue:522; PicturePasswordConfirmModal.vue:131 branches on window.matchMedia).
@media (prefers-reduced-motion: reduce) {
.draggable-item--placed {
animation: none;
}
}and gate the SortableJS animation default the same way PicturePasswordConfirmModal gates its duration.
|
|
||
| <style lang="scss" scoped> | ||
|
|
||
| .draggable-handle { |
There was a problem hiding this comment.
nitpick: Separate from the specificity point above: pointer reads as "this activates something on click", but the affordance for a drag handle is grab — and the rest of this feature already agrees (draggable.scss:7 sets cursor: grabbing on the mirror, RearrangeChannelsPage.vue's .item sets cursor: grab on what is now also the handle). Inherited verbatim from the deleted DragHandle.vue, but grab is the right default for the shared component — and setting it here makes the specificity collision harmless rather than load-order-dependent.
| return { root }; | ||
| }, | ||
| props: { | ||
| /* eslint-disable vue/no-unused-properties */ |
There was a problem hiding this comment.
nitpick: The suppression is legitimate — the props are consumed through the object handed to useDraggableRegion(props, emit, root), so the rule can't see them. But wrapping all nine also disables the check for props that later go genuinely dead. tag doesn't need it at all: it's referenced by name in the template. Narrowing to the props the composable reads, with a one-line comment saying why, keeps the rule working for the rest.
3 participants
Summary
This PR adds a
universe/region/item/handledrag-and-drop abstraction on SortableJS, supporting source→zone dragging (items moved from a pool into fixed-capacity slots) in addition to list reordering.And we migrate the 5 existing list-sorting callsites onto it as the first consumer behaviour unchanged.
Manual verification performed on all 5 migrated callsites (Device channel reorder, Coach lesson resource table, section order, questions accordion, QTI order interaction), by pointer and touch, per the checklist in the linked plan doc.
One intentional behaviour change:
QuestionsAccordionnow clears its drag-active state on@dragendinstead of only in the sort handler, fixing a bug where a drag that ends without a reorder left text selection permanently disabled.References
Reviewer guidance
QuestionsAccordion.vueLessonResourcesTable.vueSectionOrder.vueRearrangeChannelsPage.vueOrderInteraction.vueAI usage
I used claude to write all the unit tests and for some of the direct replacement for the existing consumers