fix(web): keep sidebar scroll position when pinning threads - #10757
fix(web): keep sidebar scroll position when pinning threads#10757saphid wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped sidebar UI bug fix that disables browser scroll anchoring during row reordering, preserving the user’s scroll position without changing application data or APIs. The accompanying documentation-only update has no runtime impact. You can add or adjust custom eligibility rules. Learn more. |
0e4544c to
e048caf
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe sidebar disables browser scroll anchoring when reordered rows change position. Documentation describes the preserved scroll position when pinning or unpinning threads on web and desktop. ChangesSidebar scroll behavior
Priority: ⬇️ Low — Defer the sidebar scroll-position fix because it is a narrow web usability improvement with no stated broader product or external urgency. Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to Pinning or unpinning threads now preserves the sidebar’s current scroll position, with matching documentation. No current merge-readiness risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
What Changed
Pinning a visible thread partway down the sidebar can jump the entire list to the top. Exclude the sidebar content from browser scroll anchoring so pinning and unpinning preserve the scroll position. The existing row animation and explicit search navigation continue to work.
Why
The unified list introduced in #9731 retains a thread's DOM row while moving it between sections. Browser scroll anchoring can follow that row into Pinned. This is a side effect of the section move, rather than an intentional scroll request. The fix is scoped to the shared sidebar content; transcript scroll behavior is unaffected.
UI Changes
Same disposable Harbor project, 40 threads with fixed active ordering, dark theme, 1280×800 browser viewport, and the same thread/menu action. Base
7220dfe2c; candidate0e4544cc7. Menu opening uses the real DOM handler, and a normal preview click pins through the real backend.Before: sidebar scrollTop jumps from 1,000 px to 0 px.
After: sidebar scrollTop stays at 1,000 px.
GIFs crop the sidebar and sample at 20 fps. Only idle lead-in is trimmed; the action and settled result retain their original speed. They illustrate the jump, not animation smoothness. Relative age labels advance between captures.
Before recording · After recording · Annotated before · Annotated after
Before screenshot · After screenshot
Verification
Current head
e048caf6cis rebased ontob5d89038a. The documentation conflict preserves upstream file-drop instructions and this scroll-position note. The anchoring fix itself is unchanged. The recordings above retain their original revision labels; the same browser pin scenario was freshly repeated on the rebased head with the new upstream scroll-fade-padding behavior: 1,000 → 1,000 px. Focused tests (21), web typecheck, scoped lint and formatting were repeated after rebase.cd apps/web && vp test run src/components/ui/sidebar.test.tsx src/components/Sidebar.motion.test.ts: 21 tests pass.cd apps/web && vp run typecheck: passes.Math.randompurity warning in SidebarMenuSkeleton.The real scroll regression was checked in Chromium; the existing unit tests do not emulate browser scroll anchoring. Electron shares this sidebar but its native shell/context menu was not separately exercised. Grouped sidebar, React Native, and SwiftUI were not independently exercised; no native client code or wire contracts change.
Independent review was attempted with
claude -p --model claude-opus-5 --effort high --tools '' --output-format jsonagainst the frozen diff and project coding standards. It exited 1 on expired OAuth before any model ran; no independent review occurred. Local review found no actionable issues in the two-file change.Checklist
Implemented and verified with GPT-6 in the Codex harness.
Summary by CodeRabbit
Bug Fixes
Documentation