feat(web): drag sidebar threads between categories - #8001
Conversation
tarik02
changed the title
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| threadId: command.threadId, | ||
| commandId: command.commandId, | ||
| occurredAt, | ||
| clear: ["pinned", "settled"], |
There was a problem hiding this comment.
🟡 Medium orchestration/decider.ts:690
Snoozing a neutral thread emits thread.unsettled, leaving settledOverride as "active"; after waking, that override prevents the thread from auto-settling normally. planThreadCategoryCleanup treats every value other than "active" as settled, so this clear request must only emit thread.unsettled when the current override is actually "settled".
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/decider.ts around line 690:
Snoozing a neutral thread emits `thread.unsettled`, leaving `settledOverride` as `"active"`; after waking, that override prevents the thread from auto-settling normally. `planThreadCategoryCleanup` treats every value other than `"active"` as settled, so this `clear` request must only emit `thread.unsettled` when the current override is actually `"settled"`.
There was a problem hiding this comment.
Leaving this unchanged. An explicit snooze is modeled as a move to Snoozed, not as a deferred restoration marker. The decider intentionally emits thread.unsettled for a neutral thread so a later wake returns it to Regular, and decider.snoozed.test.ts asserts that event sequence. Restricting cleanup to the settled override would change the accepted lifecycle semantics.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
UI consistency review of the new sidebar drag-and-drop surfaces. Two findings, both in newly added web UI code; everything else (primitive reuse, ScrollArea/SidebarContent viewportRef contract, Tailwind ownership, reduced-motion handling, drag-preview geometry vs. contain-intrinsic-size) looks consistent with the existing system.
Posted via Macroscope — UI Consistency
![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: 399a71d142
ℹ️ 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".
| case "unsettle": | ||
| return capabilities?.threadSettlement === true; |
There was a problem hiding this comment.
When the current web client connects to a server from before this commit, threadSettlement: true only means the server understands thread.unsettle; it does not mean that command also clears a pin. For example, dragging an auto-settled pinned thread to Regular passes this check, but the older decider leaves pinnedAt intact, so reconciliation places the row in Pinned instead of the requested destination. The existing command capabilities cannot distinguish the changed lifecycle semantics, so cross-category drops need a new capability/version gate or a compatibility command sequence.
AGENTS.md reference: AGENTS.md:L72-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this unchanged. This PR intentionally makes the existing lifecycle commands perform their category cleanup atomically, and compatibility with servers that implement the earlier command semantics is outside its scope. Adding a protocol capability or a client-side compatibility sequence would add machinery for a version combination we do not support here.
| if ( | ||
| transaction === null || | ||
| (transaction.phase !== "dragging" && | ||
| transaction.phase !== "dropping" && | ||
| transaction.phase !== "awaiting-snooze-choice") | ||
| ) { |
There was a problem hiding this comment.
When a remote lifecycle command is in committing or reconciling, this guard excludes both phases from scope invalidation. If the user switches project scope while waiting for the command receipt or shell snapshot, transaction.entries continues rendering the previous project's board under the new scope until reconciliation completes; after a disconnect following a successful receipt, that stale board can remain indefinitely. The scope check should also clear the projected transaction during these pending phases.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9d856a3b6. Scope changes and search now clear projected entries in every transaction phase. beginReconciliation also verifies that the committing transaction is still current, so an async command completion after the view changes cannot restore the old board.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. Macroscope could not determine whether this PR modifies its approvability configuration, so the PR was not approved automatically. A PR that may change the rules that govern approval is never approved automatically. Not approved because:
|
| finishTransaction(); | ||
| return; | ||
| } | ||
| for (const assignment of pinnedPlan.assignments) { |
There was a problem hiding this comment.
🟡 Medium hooks/useSidebarThreadDnd.ts:360
Pinned-order updates are left partially persisted when any rebalance command fails. handlePinnedReorder and the preparation loop in commitLifecycleDrop execute multiple reorderPinnedThread/pinThread commands sequentially, but failure only clears the optimistic transaction; it never restores commands that already succeeded, so the canonical pinOrderKey values expose a corrupted order (including an insertion preparation without the source pin). These operations need rollback/compensation for previously successful assignments before the optimistic state is cleared.
Also found in 1 other location(s)
apps/web/src/hooks/useSidebarPinnedDnd.ts:148
handlePinnedReorderexecutes rebalance assignments sequentially and aborts on the first failure without restoring assignments that already succeeded.planPinnedReordercan return assignments for the entire pinned section when a neighboring thread is keyless or keys are corrupt, so a failure after one or more successful commands leaves the persistedpinOrderKeyvalues only partially rewritten; clearing the optimistic state then exposes an incorrect pinned order.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/hooks/useSidebarThreadDnd.ts around line 360:
Pinned-order updates are left partially persisted when any rebalance command fails. `handlePinnedReorder` and the preparation loop in `commitLifecycleDrop` execute multiple `reorderPinnedThread`/`pinThread` commands sequentially, but failure only clears the optimistic transaction; it never restores commands that already succeeded, so the canonical `pinOrderKey` values expose a corrupted order (including an insertion preparation without the source pin). These operations need rollback/compensation for previously successful assignments before the optimistic state is cleared.
Also found in 1 other location(s):
- apps/web/src/hooks/useSidebarPinnedDnd.ts:148 -- `handlePinnedReorder` executes rebalance assignments sequentially and aborts on the first failure without restoring assignments that already succeeded. `planPinnedReorder` can return assignments for the entire pinned section when a neighboring thread is keyless or keys are corrupt, so a failure after one or more successful commands leaves the persisted `pinOrderKey` values only partially rewritten; clearing the optimistic state then exposes an incorrect pinned order.
There was a problem hiding this comment.
Leaving this unchanged. These assignments can span environments, so compensation would be another fallible multi-command sequence that can race with remote state. Each successful key assignment is independently valid, partial materialization still produces a deterministic order, and a later reorder rematerializes any missing assignments. This matches the existing no-rollback design for pinned key materialization.
There was a problem hiding this comment.
One finding on the new sidebar drag board: the empty-section drop rail is missing for Snoozed, so an empty Snoozed section presents differently from the other three sections during a drag.
Posted via Macroscope — UI Consistency
| content = | ||
| snoozedThreadCount > 0 || railVisible ? ( | ||
| <SidebarThreadShelfHeader | ||
| section="snoozed" | ||
| count={snoozedThreadCount} | ||
| expanded={props.snoozedShelf.expanded} | ||
| dropActive={railVisible || dnd.transaction?.target?.section === "snoozed"} | ||
| setDroppableNodeRef={bag.setDroppableNodeRef} | ||
| onToggle={props.snoozedShelf.onToggle} | ||
| /> | ||
| ) : null; |
There was a problem hiding this comment.
Empty Snoozed is the only section that does not get the dashed drop rail. Pinned, Regular, and Settled fall back to EmptySectionRail when their section is empty and a drop is possible, but Snoozed renders SidebarThreadShelfHeader with count={0} instead (and folds railVisible into dropActive), so dragging a snoozeable thread over an empty shelf shows a bare "Snoozed" header with no isOver highlight and a much smaller hit area — while docs/user/thread-sidebar.md in this PR says empty Pinned, Regular, Snoozed, and Settled all appear as drop targets. Consider mirroring the Settled branch.
| content = | |
| snoozedThreadCount > 0 || railVisible ? ( | |
| <SidebarThreadShelfHeader | |
| section="snoozed" | |
| count={snoozedThreadCount} | |
| expanded={props.snoozedShelf.expanded} | |
| dropActive={railVisible || dnd.transaction?.target?.section === "snoozed"} | |
| setDroppableNodeRef={bag.setDroppableNodeRef} | |
| onToggle={props.snoozedShelf.onToggle} | |
| /> | |
| ) : null; | |
| content = | |
| snoozedThreadCount > 0 ? ( | |
| <SidebarThreadShelfHeader | |
| section="snoozed" | |
| count={snoozedThreadCount} | |
| expanded={props.snoozedShelf.expanded} | |
| dropActive={dnd.transaction?.target?.section === "snoozed"} | |
| setDroppableNodeRef={bag.setDroppableNodeRef} | |
| onToggle={props.snoozedShelf.onToggle} | |
| /> | |
| ) : railVisible ? ( | |
| <EmptySectionRail | |
| section="snoozed" | |
| label="Snoozed" | |
| isOver={bag.isOver} | |
| setDroppableNodeRef={bag.setDroppableNodeRef} | |
| /> | |
| ) : null; |
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Leaving this unchanged. The empty Snoozed header is intentionally the drop target; the product behavior here is to reveal the normal category header instead of a synthetic dashed rail. The user doc promises that the empty section appears as a target, not that every empty target shares the same presentation. Category ownership is also based on the shelf boundary collision logic rather than only the header element's pointer hit box.
There was a problem hiding this comment.
One finding on the latest commit's removal of the empty Regular drop rail. The previously reported empty-Snoozed rail inconsistency (SidebarThreadBoard.tsx, snoozed boundary) is unchanged and still stands — not re-commented.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
UI consistency review of the sidebar drag-and-drop rework. The two items I raised on the previous head were addressed (aria-expanded now tracks the real shelf state via a separate dropActive presentation flag, and the grab cursor now lands on the [data-thread-row] surface). One earlier finding is still open (empty Snoozed renders a shelf header instead of the dashed EmptySectionRail the other three sections get — see the existing thread on SidebarThreadBoard.tsx), plus one new note below on coverage for the newly extracted pure drag logic.
Posted via Macroscope — UI Consistency
| } | ||
|
|
||
| /** The lifecycle command that realizes a drop between two sidebar sections. */ | ||
| export function resolveSidebarDndAction(input: { |
There was a problem hiding this comment.
The new pure drag logic ships without tests: resolveSidebarDndAction (source/destination -> lifecycle command), resolveSidebarDndPreviewVariant, and the Sidebar.dnd.board.ts helpers (moveSidebarDndBoardThread, findSortedSidebarDndDropTarget) all decide which command a drop issues and where the row lands, and this PR also drops the animatePinnedLayoutChanges block from Sidebar.logic.test.ts and adds sortSnoozedThreadsForSidebar with no peer test. Every sibling in Sidebar.logic.ts (sortThreadsForSidebar, sortSettledThreadsForSidebar, sortPinnedThreadsForSidebar, planPinnedReorder, orderItemsByPreferredIds) has its own describe block, so these are the only sidebar lifecycle decisions without coverage.
Consider a focused Sidebar.dnd.logic.test.ts (plus a sortSnoozedThreadsForSidebar case alongside the existing sorter suites) covering each source/destination pair and the board move/target helpers — they are dependency-free and cheap to pin down.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Fixed in 8d0b2e6. Sidebar.dnd.logic.test.ts now covers all 16 source/destination actions, preview variants, board movement, empty-section placement, and sorted targets. Sidebar.logic.test.ts now also covers snoozed wake-time ordering and stable ties.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1f62d2b. Configure here.
There was a problem hiding this comment.
Reviewed the sidebar drag-and-drop UI for shared-primitive usage, Tailwind ownership, and interaction/accessibility preservation. The Regular boundary droppable and the missing pure-logic tests flagged earlier are addressed. One accessibility regression remains in the new board structure.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
One finding: the sidebar un-settle affordance loses its shared Tooltip, reverting a recently merged fix and diverging from the sibling icon buttons in the same row.
Posted via Macroscope — UI Consistency
1 participant
What Changed
Why
The sidebar previously allowed reordering only inside Pinned.
The diff is larger than a normal UI patch because the old pinned-only implementation lived inside
Sidebar.tsx. This PR moves drag state, collision handling, pinned ordering, viewport anchoring, and board rendering into focused modules. The resulting design is documented indocs/internals/sidebar-thread-dnd.md.UI Changes
Before: drag and drop only reordered threads within Pinned.
After:
recording-a377fc69-049a-472c-9a65-b0a7aa970521.mp4
The recording covers pinned reorder, moves between every category, the snooze menu, card-to-compact morphing, auto-scroll, and viewport stability.
Verification
@t3tools/webtypecheck passed.Checklist
Model: GPT-5.6-sol
Harness: Codex in T3 Code
Note
High Risk
Changes orchestration companion events for pin/snooze/settle/unpin so categories are exclusive, plus a large new drag-and-drop path that issues those lifecycle commands.
Overview
Users can drag web sidebar threads between Pinned, Regular, Snoozed, and Settled. A drop runs the matching lifecycle command (pin/unpin, settle/unsettle, snooze/wake). Pinned drops keep the chosen slot; other sections snap to their normal sort. Snooze still opens the duration picker.
The old pinned-only sorter is replaced by a single dnd-kit board (
SidebarThreadBoard+useSidebarThreadDnd) with custom collision, overlay morphing, drop outlines, and viewport anchoring.Server: pin, snooze, settle, unpin, unsettle, and wake now clear conflicting category state via
planThreadCategoryCleanup. Pinning a neutral thread also stampsthread.unsettled. Snooze no longer preserves pin; Wake returns to Regular. Mobile menus and snooze toasts (“Wake” instead of “Undo”) follow the same exclusive-category rules.Reviewed by Cursor Bugbot for commit d4975c7. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add drag-and-drop reordering of sidebar threads across categories on web
useSidebarDndLayoutanduseSidebarThreadDnd.@dnd-kitpinned reordering logic inSidebar.tsxwith a unifiedSidebarThreadBoardcomponent andSidebarThreadDndShellwrappers; removesanimatePinnedLayoutChangesandSortablePinnedThreadRow.decider.tsnow emits companion cleanup events (thread.unpinned,thread.unsettled,thread.unsnoozed) via a sharedplanThreadCategoryCleanuphelper when a thread moves between categories (pin/unpin/snooze/unsnooze/settle/unsettle).sortSnoozedThreadsForSidebar; snooze toast action label changes from "Undo" to "Wake" on web and mobile.ThreadListV2Rowremoves the unsettle option from snoozed rows and always shows the pin menu item for settled slim rows.decideOrchestrationCommandcases forthread.unsettle,thread.snooze,thread.unsnooze, andthread.unpinnow emit additional companion events that previously were not emitted; tests indecider.pinned.test.tsanddecider.snoozed.test.tsupdated to expect sequences like["thread.pinned", "thread.unsettled"].Macroscope summarized d4975c7.