fix(reorder): make drag-and-drop land exactly where dropped - #300
fix(reorder): make drag-and-drop land exactly where dropped#300ahmet-cetinkaya wants to merge 2 commits into
Conversation
Custom-order lists (always-on reorderable tasks/habits) frequently dropped items at the wrong position: an item would snap to a seemingly arbitrary slot after the post-drop refresh. Root cause: the UI computed fragile fractional ranks that collided as midpoint gaps collapsed below minimumOrderGap (and with duplicate or near-zero orders from sync/migration), while the command handler persisted the UI value blindly. The optimistic local reorder and the persisted order disagreed, so the debounced refresh re-sorted by the wrong order and the item jumped. Redesign the reorder->persist->re-render cycle around a positional, single-source-of-truth contract: - Handlers (UpdateTaskOrderCommand, UpdateHabitOrderCommand) now take a target index plus before/after neighbor id hints instead of a pre-computed order. They load the authoritative order-sorted sibling set, compute a gap-safe rank between the real neighbors via OrderRank.neighborRank, and renormalize the whole set on any collision before placing the item exactly at its slot. - Add NormalizeTaskOrdersCommand (mirrors NormalizeHabitOrdersCommand) for batch order repair. - Fix a latent registration bug: UpdateTaskOrderCommand was registered with return type void but sent expecting UpdateTaskOrderResponse. - UI (tasks_list, habits_list) drops all client-side fractional math and sends positional commands; on-load normalization now triggers on duplicates and collapsed gaps, not only near-zero values. - Bump acore-flutter submodule for OrderRank.neighborRank / needsNormalization. Notes (batch order) and tags (settings-stored order) use different mechanisms and are unaffected. All logic is shared Dart, so the fix covers Android, Windows and Linux. Add order_rank_test.dart (13 cases) locking neighbor placement, collision throwing and collapsed-gap detection.
There was a problem hiding this comment.
Code Review
This pull request refactors the habit and task ordering system to use a robust, positional approach where the backend serves as the single source of truth for rank computation. It introduces a new NormalizeTaskOrdersCommand to batch-renormalize task orders, updates the update order commands to use target indices and neighbor hints, and integrates these changes into the UI. The feedback highlights two critical improvements: optimizing the task re-normalization handler to perform batch updates via updateMultiple instead of individual database updates in a loop, and refining the UI-side order normalization check to prevent potential infinite loops when custom ordering is inactive or when sibling sets are mixed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| Future<double> _renormalizeAndPlace(Task task, List<Task> siblings, int position) async { | ||
| final now = DateTime.now().toUtc(); | ||
| final ordered = List<Task>.from(siblings); | ||
| final clampedPosition = position.clamp(0, ordered.length); | ||
| ordered.insert(clampedPosition, task); | ||
|
|
||
| double step = OrderRank.initialStep; | ||
| double placedOrder = OrderRank.initialStep; | ||
| for (final t in ordered) { | ||
| t.order = step; | ||
| t.modifiedDate = now; | ||
| await _taskRepository.update(t); | ||
| if (t.id == task.id) placedOrder = step; | ||
| step += OrderRank.initialStep; | ||
| } | ||
| return placedOrder; | ||
| } |
There was a problem hiding this comment.
Updating each task individually in a loop (await _taskRepository.update(t)) is highly inefficient and can cause significant performance bottlenecks, database locks, or UI lag when there are many sibling tasks. Since _taskRepository supports batch updates via updateMultiple, we should perform the updates in a single batch operation after the loop.
Future<double> _renormalizeAndPlace(Task task, List<Task> siblings, int position) async {
final now = DateTime.now().toUtc();
final ordered = List<Task>.from(siblings);
final clampedPosition = position.clamp(0, ordered.length);
ordered.insert(clampedPosition, task);
double step = OrderRank.initialStep;
double placedOrder = OrderRank.initialStep;
for (final t in ordered) {
t.order = step;
t.modifiedDate = now;
if (t.id == task.id) placedOrder = step;
step += OrderRank.initialStep;
}
await _taskRepository.updateMultiple(ordered);
return placedOrder;
}| bool _shouldNormalizeOrders(List<TaskListItem> items) { | ||
| return items.any((item) => item.order.abs() < 1e-10 || (item.order > 0 && item.order < 1e-6)); | ||
| if (items.any((item) => item.order.abs() < 1e-10 || (item.order > 0 && item.order < 1e-6))) { | ||
| return true; | ||
| } | ||
| return OrderRank.needsNormalization(items.map((item) => item.order).toList()); | ||
| } |
There was a problem hiding this comment.
Automatic on-load normalization can trigger an infinite loop of refresh() -> _normalizeTaskOrders() -> refresh() if the displayed list contains mixed sibling sets (e.g., when widget.includeSubTasks is true, or when sorting by non-custom fields like date/priority where tasks from different parent tasks are mixed together). In these cases, duplicate order values are perfectly normal and expected because order is only unique within a single sibling set. To prevent infinite loops, we should only evaluate and trigger normalization when custom ordering is active and we are not mixing sibling sets.
bool _shouldNormalizeOrders(List<TaskListItem> items) {
if (widget.sortConfig?.useCustomOrder != true || widget.includeSubTasks) {
return false;
}
if (items.any((item) => item.order.abs() < 1e-10 || (item.order > 0 && item.order < 1e-6))) {
return true;
}
return OrderRank.needsNormalization(items.map((item) => item.order).toList());
}Address review findings on the drag-and-drop reorder redesign: - Extract SiblingReorderService<T> (core/application/shared) as the single owner of resolve-position -> gap-safe-rank -> renormalize-on-collision; UpdateTaskOrder/UpdateHabitOrder handlers become thin adapters, removing ~95% duplicated logic that had already diverged. - Fix non-transactional task renormalization: the handler now persists the renumbered set via a single transactional updateMultiple instead of a per-item update() loop, eliminating partial-write order corruption that could propagate via sync. - Route both normalize commands and renumbering through OrderRank.assignSequential; drop IHabitRepository.updateAll and converge all habit bulk writes on the inherited transactional updateMultiple. - UI (tasks_list, habits_list): compute reducedGroup once and reuse it for the optimistic reorder and neighbor-id resolution so they cannot desync; extract _applyOptimisticReorder; centralize near-zero detection via OrderRank.hasNearZeroOrder. - Restore consistent error logging on the habit side (add Logger import; log onReorderComplete failures, refresh failures, and normalization failures) to match the task side. - Align HabitListItem.order to non-null double (matching Task.order), removing scattered ?? 0.0 fallbacks. - Bump acore-flutter submodule for the InvalidNeighborOrderException / cannotFit / hasNearZeroOrder / assignSequential additions and dead-code removal. Extend order_rank_test.dart to 30 cases covering the new exception, cannotFit, maxOrder overflow, and assignSequential.
Problem
Custom-order lists (the always-on reorderable tasks/habits lists) frequently dropped items at the wrong position. Dragging an item and dropping it at a target slot would often make it snap to a seemingly arbitrary position after the post-drop refresh.
Root Cause
The UI computed fragile fractional ranks that collided as midpoint gaps collapsed below
minimumOrderGap(repeated inserts halve gaps: 1000 → 500 → 250 → …), and with duplicate / near-zeroordervalues from sync or migration. The command handler then persisted the UI-supplied value blindly. Because the optimistic local reorder and the persisted order disagreed, the debouncedrefresh()re-sorted by the wrong persisted order and the item visibly jumped.Fix
Redesigned the reorder → persist → re-render cycle around a positional, single-source-of-truth contract:
UpdateTaskOrderCommand,UpdateHabitOrderCommand) now take a target index + before/after neighbor id hints instead of a pre-computed order. They load the authoritative order-sorted sibling set, compute a gap-safe rank between the real neighbors viaOrderRank.neighborRank, and renormalize the whole set on any collision before placing the item exactly at its slot.NormalizeTaskOrdersCommand(mirrors the existingNormalizeHabitOrdersCommand) for batch order repair.UpdateTaskOrderCommandwas registered with return typevoidbut sent expectingUpdateTaskOrderResponse.tasks_list,habits_list) drops all client-side fractional math and sends positional commands; on-load normalization now triggers on duplicates and collapsed gaps, not only near-zero values.Scope / Consistency
OrderRankutil and identical positional pattern — both fixed together.updateNoteOrder) and tags (settings-stored custom order) use different mechanisms and are unaffected. Task board (field mutation) unaffected.Dependency
Testing
test/shared/utils/order_rank_test.dart— 13 cases locking neighbor placement, collision throwing, and the collapsed-gap regression scenario. All pass.dart analyzeon command dirs reports no issues.Follow-ups (separate)
ordervalues app-wide (current fix self-heals lazily per sibling set on load/reorder).