feat(web): drag sidebar threads between categories - #8001

💭 I should check the current state of the PR at its latest commit d4975c7, since earlier reviews were based on older commits, to see if past feedback was addressed and spot any new issues.

1. git_tools — d7ea7c2..d4975c7

2. git_tools — b1670ac..d4975c7

💭 The prior issues seem addressed with recent commits fixing the tooltip, sidebar boundaries, and snooze drag behavior. Now I want to check the two commits after d7ea7c2, ebe62fe and d4975c7, to see if they introduced anything new.

3. git_tools — ebe62fe

4. git_tools — d4975c7

💭 With both prior findings resolved, I want to verify the newest fixes don't introduce regressions, and check the remaining open items — the grab cursor issue, missing tests, the empty snoozed rail, and the shelf header's aria-expanded handling. Looking at the aria-expanded conflation in SidebarThreadShelfHeader, the code now correctly separates the actual expanded state from the presentational label/chevron state, so that one's fixed. The grab cursor styling has also been added via the data-thread-row selector, resolving that finding too. But the empty Snoozed rail issue still looks unaddressed — when the snoozed count is zero but the rail is visible, it's still rendering the shelf header with a count of zero instead of falling back to the empty section rail component, matching the same problem flagged before, so I won't repost that since it's already a known comment. Now I'm checking the regular droppable case when both pinned and regular sections are empty. The tests concern is resolved too, since a dedicated logic test file for the sidebar drag-and-drop now exists, and the boundary list-item and tooltip fixes both landed in separate commits. Now I'm double-checking whether marking the boundary list item as aria-hidden is actually safe — for the regular section it's just decorative content so that's fine, and for snoozed/settled sections the toggle button stays accessible since aria-hidden isn't applied there, though I want to verify the pinned-empty rail case doesn't accidentally hide something interactive too. For the tooltip restoration fix, I want to confirm the Tooltip components are properly imported and used consistently with how other icon buttons in the sidebar implement their tooltips, checking the render pattern and trigger structure against neighboring code.

5. git_tools — TooltipTrigger

💭 I'm noticing the "Wake thread now" button lacks a tooltip, and I want to check whether that's preexisting behavior from before this diff or something introduced by these changes, so I should compare against the merge base version.

6. git_tools — Wake thread now

💭 The tooltip matches main now, and the missing tooltips on the "Wake thread now" and "Settle thread" buttons are preexisting in main, not a new finding. Since the prior issues seem resolved, I want to re-scan the current head versions of the key new files for any unflagged UI consistency issues before reporting. Checking through EmptySectionRail, SidebarThreadShelfHeader, the "Show more" button, and the ScrollArea viewportRef addition — these all look fine at head, and the snoozed empty rail issue is already flagged so I won't repeat it. I'm also noting the contain-intrinsic-size changes (96px→78px, 34px→36px) as a minor possible layout jitter, and confirming that inert prop handling and data-dnd-source attributes look correct. I'm revisiting the overlay's pointer-events-none/cursor-grabbing interaction on the drag view item, since a prior comment noted cursor-grabbing never actually applies because the overlay blocks pointer events — the fix added a cursor-grab utility scoped to thread rows in the shell, though I still need to check how grabbing behaves during an active drag. I want to verify the aria-hidden logic doesn't accidentally hide the pinned EmptySectionRail — since rail content is non-null and the section is pinned, aria-hidden should stay undefined there. For the regular section, aria-hidden is now always true on the list item, which is fine since that content is decorative, and this doesn't affect shelf header buttons in the snoozed/settled sections. I'm also confirming that when settledThreadCount is zero and the rail is visible, the EmptySectionRail renders with non-null content so it isn't aria-hidden and "Settled" gets announced properly. Now I want to pull up SidebarThreadBoard.tsx at head to check for any remaining issues in the full component.

7. browse_code — apps/web/src/components/sidebar/SidebarThreadBoard.tsx

💭 Checking the shelf header button change — adding rounded-md and transition-colors doesn't introduce any regression since there was no focus ring before either, and it's purely cosmetic. The hidden count and showMoreCount logic look fine, so I'm moving on to look at the "settled" boolean next. Removing the per-variant key from the row and dropping the FLIP crossfade comment is intentional for supporting the flat drag-and-drop board, since dnd requires stable keys — this is documented and acceptable. The autoAnimate removal means non-drag mutations like a thread settling or a new thread appearing lose their transition animation, though dnd-kit sortable rows still animate layout changes on their own during drags. This feels more like an intentional design tradeoff than a concrete regression worth flagging, since it's within the scope of the rewrite described in the docs. Checking whether removing that comment matters, since it explained a FLIP cross-slide artifact avoided by keying per variant—but since dnd-kit's sortable transitions only trigger during active sorting, non-drag list changes won't animate across the list, so that artifact can't occur anymore. That means the comment's rationale no longer applies, so no new finding here. Moving on to check other CSS concerns like overflow-anchor settings. I'm checking where the overflow-anchor:none class actually lands in the SidebarContent component tree—it seems to attach to the outer wrapper div rather than directly on the ScrollArea viewport, so I need to verify whether that ancestor placement still suppresses scroll anchoring inside the actual scrolling viewport per spec behavior. Actually the property also matters if applied directly to the scroll container itself, disabling anchoring there. So I need to check whether the class is actually being applied to the scroller element in sidebar.tsx's current implementation.

8. browse_code — apps/web/src/components/ui/sidebar.tsx

9. browse_code — apps/web/src/components/ui/scroll-area.tsx

💭 I realize the className actually lands on the inner content div, not the viewport itself, and since overflow-anchor:none excludes both that element and its descendants from anchor selection, this effectively disables scroll anchoring for all the sidebar content since everything lives inside that div. I recall the spec's algorithm skips excluded elements and their whole descendant subtree, so applying overflow-anchor:none on the SidebarContent wrapper effectively disables anchoring for everything inside it — matching the documented intent, even though it's placed on the inner content div rather than the outer scroll container. Checking the SidebarThreadDnd shell, I verify the new cursor-grab descendant selector correctly outranks the existing cursor-pointer class by specificity, so the grab cursor wins as intended. I also confirm the pinned-divider test id is preserved, then move to checking whether renderBoardThread still sets its key prop correctly. The key is now set inside SidebarThreadDndRow instead, which seems fine, and the snoozed shelf header logic still gates rows by count and expansion state as expected. I also note the board list disables pointer events during drag, matching the documented behavior, and I want to make sure there's no dead code like an unused autoAnimate import lingering around, then check orderItemsByPreferredIds for consistency. I confirm orderItemsByPreferredIds and planPinnedReorder are still actively used, and firstValidTimestampMs moved to being exported from Sidebar.logic rather than imported directly, which is fine. Now I want to check whether any CSS still references data-thread-item or data-thread-row selectors that might break if those attributes were removed from list items during the refactor.

10. git_tools — data-thread-item|data-thread-selection-safe|data-thread-row|sidebar-pinned-divider|sidebar-snoozed-shelf-toggle

💭 I'm confirming the selection-safe attribute moved correctly onto the wrapping div while staying functionally equivalent, and checking that the "Show more" list item still sits within the same list structure as before. I want to double-check SidebarThreadDnd.tsx's recent changes around the grab cursor fix, plus verify the pinned rail stays visible (not aria-hidden) during drag. The empty Snoozed section header issue was already flagged earlier, so I shouldn't repeat it — instead I should pull the existing PR review comments to see what's already been raised and avoid duplicating findings.

11. github_api_read_only — repos/{owner}/{repo}/pulls/{pull_number}/comments?per_page=100

💭 Rather than trying to filter fields on the fetch, I'll just review the current head code directly for issues instead—let me pull up SidebarThreadDnd.tsx in full at head.

12. browse_code — apps/web/src/components/sidebar/SidebarThreadDnd.tsx

💭 I'm noting that the touch-pan-y setting combined with pointer-down capture disables dragging on touch pointers by design, since mobile relies on menus instead, and that's fine for touch devices with pan gestures. I'm also checking whether the spread listeners on the list item include onKeyDown, which only appears if a keyboard sensor is registered. Since only PointerSensor is configured, listeners just include onPointerDown, so there's no risk of the row swallowing keyboard events. I confirm accessibility is acceptable since desktop has a context menu as a keyboard-reachable alternative to dragging, and the list role/li structure is intact. I'm now reviewing the Sidebar row surface class change that applies overlay styling with important overrides when dndDragView is active, which seems appropriate for a drag overlay state. I want to trace how rowSurfaceClassName is composed to make sure this override is applied correctly. Checking the semantic tokens used for overlay styling, they look consistent with the design system and don't conflict with active row states. Overall I don't see any new unflagged violations in the current head — the only outstanding issue is the previously flagged empty Snoozed rail inconsistency, and since that code hasn't changed I shouldn't post a duplicate comment about it. But this raises a conflict: the instructions say to respond with exactly "All clear" when there are no new findings, yet there's an unresolved prior issue still present in the code, and my final output needs to fit the required JSON schema rather than being a bare string. Tracing through the commit history, it seems the author addressed several other flagged items across multiple commits but never touched the empty Snoozed section rendering — it still shows a header with count 0 instead of a proper drop rail, unlike the Pinned and Settled sections. I'm checking whether the header's dropActive state might have been intended as a substitute fix, but that only affects the label and chevron styling, not an actual highlight, so this still reads as unresolved. I'm now cross-referencing this against the PR's own documentation, which explicitly states that empty Pinned, Snoozed, and Settled sections should appear as drop targets while dragging. Since the header itself is technically the droppable node, it arguably does satisfy that documentation even without the dashed rail visual, so I'm treating this as a minor, already-flagged inconsistency rather than a new blocking issue. Given everything else I found has either been fixed or previously reported, I'm leaning toward a neutral or success verdict, since the check should only fail when there are genuinely new findings. I want to do one more pass checking the Sidebar.tsx changes around renderBoardThread and row key handling before finalizing that conclusion. I'm also verifying that the tooltip nesting pattern in the latest commit matches what main already did, so that's not a regression. Then I check the aria-hidden logic on the boundary list items for the snoozed/settled sections — confirming it correctly toggles based on whether content is null, and considering whether hiding a list item within a role="list" container causes any accessibility concerns. Finally, I look at the DragOverlay's wrapper element, which renders a ul/li structure outside the main list — since it's marked inert, it's properly excluded from the accessibility tree, so no issue there either. With no new findings, I'll conclude the review as clean and prepare a minimal success response with "All clear" as the summary.