fix(desktop/tasks): drag-to-reorder visibility, persistence, and end-of-drag cleanup#7185
fix(desktop/tasks): drag-to-reorder visibility, persistence, and end-of-drag cleanup#7185eulicesl wants to merge 6 commits intoBasedHardware:mainfrom
Conversation
…s on cursor approach The drag handle is rendered in the outer HStack as a sibling of taskRowContent, but the .onHover that drives `isHovering` (and therefore the handle's visibility) was attached to taskRowContent, not the body. Result: hovering over the handle's frame did not flip isHovering, so the handle stayed at .foregroundColor(.clear) and the user could not see or grab it. Hovering over the text region worked because that is where the hover trigger actually lived. Move .onHover to the outer body so it fires for hover anywhere within the row, including the handle area. Same handler body — no behavior change for the existing cursor / onHover? callback, just a wider hit region. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…suppress DB requery during move moveTask only wrote sortOrder updates to store.incompleteTasks. But recomputeDisplayCaches picks the displayTasks source in priority order: searchResults → filteredFromDatabase → store.incompleteTasks. With any non-status filter active (the default last7Days filter qualifies), the displayed tasks come from filteredFromDatabase, not from the store, so the writes missed entirely (firstIndex returned nil) and the row visually snapped back to its old position. Apply the new sortOrders to every source array the task could live in. Each @published reassignment fires once; recomputeDisplayCaches folds them into categorizedTasks for the next render. Even with the writes landing, recomputeAllCaches was scheduling loadFilteredTasksFromDatabase() asynchronously when filters were active. That requery clobbered filteredFromDatabase with SQLite rows that did not yet have the new sortOrder (scheduleSortOrderSync writes to SQLite +500ms after the move). Use the existing suppressDatabaseRequery flag during the debounce window — pattern matches clearTodayDeadlinesForIncompleteTasks. The flag is reset via `defer` inside syncSortOrders once SQLite is fresh, so the next requery returns valid data. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…anup The drag-to-reorder UI was 95% built but the dragged row never visually dimmed in flight: TaskRow.onDrag never invoked the parent's onDragStarted callback, and even if it had, the optional callback was silently nil at the only call site that mattered. Wire the dim end-to-end and make the callback contract explicit: - TaskRow exposes onDragStarted/onDragEnded/isBeingDragged. The two callbacks are non-optional with no-op defaults — silent-nil at the parent was the original bug we are fixing, and the type system now prevents repeats. - TaskCategorySection forwards draggedTaskId from the view model so it can compute isBeingDragged per row. - Apply .opacity(0.4) to the dragged row body with a 120ms ease-in-out. - Drop the dead onDragStarted on TaskDragDropModifier (the modifier handles drop targets only, not drag sources). End-of-drag detection is the harder half. Items 3 and 4 of the verification checklist (drop in dead space, drop outside the window) require firing onDragEnded even when no drop target accepts the drop. A first attempt with NSEvent.addLocalMonitorForEvents and addGlobalMonitorForEvents for .leftMouseUp did not fire — AppKit drains the trailing mouseUp through its modal drag loop and skips the standard event monitors, so the dragged row would stay dimmed forever on aborted drags. Replace it with an NSItemProvider subclass that fires a callback in deinit. AppKit releases the provider when the drag session ends on every path (drop, dead-space drop, off-window drop, escape cancel), so deinit is the only signal that fires uniformly for all of them. The deinit hops to main before mutating @published state since AppKit may release the provider off-main. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74cfa42d68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR addresses three independent drag-to-reorder bugs in the macOS Tasks page, all within
Confidence Score: 4/5Safe to merge; all three fixes are additive and defensive against well-described existing bugs with no behavioral regressions on callers that don't supply the new callbacks. The changes are carefully contained to the Tasks drag-reorder path. The No files require special attention beyond the noted double-callback path in Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant TR as TaskRow (.onDrag)
participant TDI as TaskDragItemProvider
participant VM as TasksViewModel
participant TDD as TaskDragDropModifier (.onDrop)
participant SQL as SQLite (syncSortOrders)
U->>TR: Grab drag handle
TR->>TDI: init(taskId, onEnd: onDragEnded)
TR-->>VM: DispatchQueue.main.async onDragStarted(id)
VM->>VM: draggedTaskId = taskId
VM-->>TR: isBeingDragged = true, opacity 0.4
U->>TDD: Drop on target row
TDD->>VM: onDragEnded() clears draggedTaskId
TDD->>VM: onMoveTask calls moveTask()
VM->>VM: applyOrder to all three backing arrays
VM->>VM: suppressDatabaseRequery = true
VM->>VM: recomputeDisplayCaches()
VM->>VM: scheduleSortOrderSync() 500ms debounce
Note over TDI: AppKit releases provider after loadItem completes
TDI->>TDI: deinit fires
TDI-->>VM: DispatchQueue.main.async onDragEnded() second call no-op
Note over VM,SQL: 500ms later
VM->>SQL: syncSortOrders() writes new sortOrders
SQL-->>VM: defer suppressDatabaseRequery = false
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
syncSortOrders() now calls recomputeAllCaches() after clearing suppressDatabaseRequery=false, matching the pattern used by clearTodayDeadlinesForIncompleteTasks(). This ensures filteredFromDatabase is refreshed when non-status filters are active, preventing stale data during the debounce window.
… NSItemProvider deinit `TaskDragItemProvider.deinit` fires `onDragEnded` after AppKit releases the provider — typically right after the synchronous drop handler in `TaskDragDropModifier.onDrop` has already called `onDragEnded` itself. The two calls land on the same closure that clears `draggedTaskId` and `dropTargetTaskId`, so the duplicate is a no-op in the common case. The subtle risk Greptile flagged in the review of BasedHardware#7185 is the cross-drag race: if a brand-new drag starts in the sub-ms window between the synchronous drop and the deinit-queued main.async hop, the deferred call would clear the new drag's `draggedTaskId` prematurely. The window is unreachable by human interaction (one main.async tick plus user mouse movement), but the two-call path was an unintentional property of the design rather than an explicit guarantee. Add `guard viewModel.draggedTaskId != nil else { return }` so the closure is strictly idempotent. Comment makes the intent explicit so future cleanups don't strip the guard as redundant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@mdmohsin7 — would you mind taking a look when you have a moment? All three bot review threads are now resolved:
CI green (Lint & Format ✅), |
1 participant
Summary
Fixes three independent bugs in the macOS Tasks page drag-to-reorder UX, all in
desktop/Desktop/Sources/MainWindow/Pages/TasksPage.swift:.onHoverwas attached totaskRowContent, so hovering on the handle's frame (a sibling oftaskRowContentinside the outerHStack) didn't flipisHovering— the handle stayed at.foregroundColor(.clear). User reported "the handle doesn't appear when the cursor is near, only when the cursor is over the text." Move.onHoverto the outer body so it fires for hover over both the handle area and the content area.moveTaskonly wrote sortOrder updates tostore.incompleteTasks. With any non-status filter active (the defaultlast7Daysfilter qualifies),recomputeDisplayCachespicks displayTasks fromfilteredFromDatabase, not from the store, so the writes missed entirely (firstIndexreturned nil). And even when they landed,recomputeAllCacheswas schedulingloadFilteredTasksFromDatabase()which clobbered local writes with stale SQLite rows before the debouncedscheduleSortOrderSynccould persist the new sortOrder. Apply the new sortOrders to every source array (incomplete + filteredFromDatabase + searchResults), and use the existingsuppressDatabaseRequeryflag during the debounce window — pattern matchesclearTodayDeadlinesForIncompleteTasks.TaskRow.onDragnever invoked the parent'sonDragStartedcallback, the optional was silently nil at the call site, and there was no consumer ofviewModel.draggedTaskIdto apply opacity. Wire the dim end-to-end with non-optional callbacks (silent-nil was the original bug — the type system now prevents repeats), and signal end-of-drag via anNSItemProvidersubclass that fires a callback indeinitso dim clears on every drag-end path (drop, dead-space drop, off-window drop, escape cancel) —NSEvent.addLocalMonitor/addGlobalMonitorfor.leftMouseUpwere tried first and don't fire from inside AppKit's drag modal loop.Changes
desktop/Desktop/Sources/MainWindow/Pages/TasksPage.swiftTaskRow.body— relocated.onHoverfromtaskRowContentso the drag handle's frame is part of the hover hit region.TasksViewModel.moveTask— replaced single-array sortOrder mutation withapplyOrder(to:)helper applied tostore.incompleteTasks,filteredFromDatabase, andsearchResults. SwappedrecomputeAllCaches()forsuppressDatabaseRequery = true; recomputeDisplayCaches()to block the async SQLite requery during the debounce window.TasksViewModel.syncSortOrders— addeddefer { suppressDatabaseRequery = false }so the flag is reset after SQLite is fresh, regardless of error/cancellation.TaskRow—onDragStarted: (String) -> VoidandonDragEnded: () -> Voidare now non-optional with no-op defaults;isBeingDragged: Booldrives.opacity(0.4)on the row body with a 120ms ease-in-out.TaskCategorySection— forwardsdraggedTaskIdfrom the view model so it can computeisBeingDragged: draggedTaskId == task.idper row; mirrored non-optional callback shape.TaskDragDropModifier— removed deadvar onDragStarteddeclaration (modifier handles drop targets only, not drag sources).TaskDragItemProvider: NSItemProvider— replaces a prior local+globalNSEventmonitor approach that didn't fire for aborted drags. Subclass fires itsonEndcallback indeinit, which AppKit triggers when the drag session ends on every path. The deinit hops to main before mutating@Publishedstate since AppKit may release the provider off-main.desktop/CHANGELOG.json— added user-facing entry underunreleased.Why
Three separate bugs that compound: the handle is invisible, so the user can't grab it; if they get past that, dropping doesn't persist; if it does persist, the dragged row stays dimmed forever after an aborted drop. All three are existing bugs against in-tree code; the changes are additive/defensive.
Test plan
xcrun swift build -c debug --package-path Desktopcompiles cleanly onupstream/mainafter cherry-pick (57.87s).git diff upstream/main..HEAD | grep '^+.*log("').TasksPage.swift+CHANGELOG.json).Live test evidence
Tested in a named bundle
omi-drag-reorder(OMI_APP_NAME="omi-drag-reorder" ./run.sh --yolo) on macOS:≡handle within the row's full frame, including the leftmost area where the handle sits. Confirmed by user after the.onHoverrelocation.REORDER: moveTask(<id>, toIndex: N, inCategory: <cat>)plus visible reorder. Capturedwrites incomplete=0 filtered=Nconfirming the write landed in the right backing array when filters are active.addGlobalMonitorForEventsfailed to catch).open /Applications/omi-drag-reorder.app; the new order was preserved on relaunch.Risk and rollback
None significant. All three changes are additive or defensive against existing in-tree code:
.onHoverrelocation is a single-modifier move with identical handler body.moveTaskchange adds writes to two more arrays (idempotent if a task isn't in them) and reuses the existingsuppressDatabaseRequeryflag with the same lifecycle pattern asclearTodayDeadlinesForIncompleteTasks.TaskDragItemProvidersubclass produces identical drop-target behavior (still registers the task id asNSString); only adds a deinit-time callback.{ _ in }/{}), so any caller that didn't supply them keeps working.Rollback: revert the three commits — they're independent and ordered by concern.
Notes for review
upstream/main; verified clean (no debug logs, no fork-only patches, scope confined to two files). Original commits authored 2026-05-04/05.defer { suppressDatabaseRequery = false }insyncSortOrdersis a no-op for the two other callers (incrementIndent,decrementIndent) since they never set the flag. Safe.cc @kodjima33
🤖 Generated with Claude Code