feat: reorder pinned sidebar sessions - #7509
Conversation
e68f6e5 to
ad84a70
Compare
|
CI status for current head
The fork credential cannot invoke |
ad84a70 to
5d58419
Compare
5d58419 to
9f16935
Compare
|
Maintainer workflow approval requested for current head The branch was rebased onto current main and locally passes 73 focused tests (including |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound feature, but persisting rank browser-locally while membership stays server-side spawns a reconciliation state machine disproportionate to a cosmetic preference. Watch
Suggestions
[DESIGN-REVIEWED] faef2c8 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All counts verified against the trusted base. First-Principles-Verdict: CONCERNS Exported What this change shipsIntent: let users keep their pinned sessions in a deliberate, stable order instead of the automatic sort — an ADDITION (feature request #7370).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] faef2c8 |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Fork lane: no first-time reader has seen the divider, insertion marker, or reorder gesture — the added screenshots/gif exist only in the PR, not on disk. Watch
Evidence gaps
[UX-REVIEWED] faef2c8 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've read the full diff and verified the key facts against source. Let me confirm my analysis of Candidate 2's reachability. The determining fact: Candidate 1 (committed No findings. [OPUS-REVIEWED] faef2c8 |
|
|
|
Maintainer approval needed for current head |
|
|
Maintainer approval needed for current head |
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed the full diff at d829c6a. The reconciliation is coherent after the review rounds: a single module-level batch joined by every in-flight toggle, monotonic snapshotVersion so only the newest settlement reconciles, refetch bounded at 3 then falling back to Redux, rank persisted only after the server outcome, and a rejected pin restores membership without touching survivor rank. pinnedSessionOrder.ts prunes on write and degrades malformed JSON to natural order, so the store cannot grow or corrupt. No hardcoded English; screenshots under temp-screenshots/.
Maintainer decision recorded: pin ORDER stays browser-local (localStorage, cross-tab synced) while pin MEMBERSHIP stays server-side -- accepted for this PR. Two follow-ups, non-blocking: hide PinnedSessionDivider in board view where reordering is impossible, and give Alt+ArrowUp/Down a visible hint (tooltip or shortcuts help) since aria-keyshortcuts alone makes it undiscoverable. Approving.
|
Approved, but the PR went CONFLICTING after this evening's merges (#8180 hover-hold and #8542 interrupted-session marker both landed in |
|
Resolved the conflict for you (maintainer_can_modify) by merging current Conflicts were all against #8180's hover-hold lane scopes and #8542 in Locally green: |
bolichen97
left a comment
There was a problem hiding this comment.
Re-approving on 6d20166: the only delta from the head I approved is the conflict-resolution merge commit I pushed myself (main's renderSessionRow signature at four call sites + the feature-map row union); the diff against main is the same 22 files. Auto-merge is armed and will land once PR Readiness reports green on this head. Maintainer decision on browser-local pin order stands as recorded above.
|
|
|
Maintainer approval needed for current head |
|
|
|
|
|
|
|
|
|
|
Maintainer approval needed for current head |
|
|
|
|
Maintainer rerun requested for current head |
|
Persist a manual order for pinned sessions while keeping the selected automatic sort for all other sessions. Add explicit section and insertion affordances, keyboard movement, rollback-safe pin lifecycle handling, documentation, and regression coverage.
|
Maintainer action requested: current head |
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed the full diff at faef2c8a548c801b21dca97edd47c0f414bcf922, not carried over from my dismissed approvals -- that head was 22 files, this one is 37 and the reconciliation machinery grew substantially after it (slotPinGenerations, React Query routed reconciliation, ongoing authoritative membership reconciliation, board-mode rank authority, cross-tab ping-pong guard). Re-reviewed from scratch.
Correctness of persistence. pinnedSessionOrder.ts cannot corrupt anything. Every write goes through reconcilePinnedSessionOrder, which intersects against the authoritative pinned set and dedupes, so a stale or duplicate key cannot survive a write and the store cannot grow. readPinnedSessionOrder degrades malformed JSON, a non-array, and non-string members to natural order. movePinnedSession is inert on unknown or equal keys. Rank is persisted only after the server outcome, never on the optimistic write, so a rejected pin leaves previous rank byte-identical.
No corruption of other sessions or users. Order is a separate mc-pinned-session-order localStorage key holding nothing but pinned session keys; the server is never written with order, only the existing pin-membership PATCH. Cross-user is not a vector: localStorage is per-origin-per-profile and the store is pruned against server-confirmed membership on every write. The slotPinGenerations per-key counter correctly isolates ownership so an unrelated slot update no longer suppresses a rejected pin's rollback.
Reconciliation. The three-generation model closes the races the review rounds surfaced: snapshotVersion monotonic per batch so only the newest settlement reconciles, slotsGeneration so an in-flight authoritative frame invalidates a snapshot, per-key slotPinGenerations plus matching pin value for failure-path ownership. The refetch loop is bounded at 3 then commits from the latest accepted Redux slots, so continuous live frames cannot spin. setSlotPinInOrder serializes per-key tails, so rapid toggles reach the server in invocation order.
Scope. 1,484 additions reads large for a reorder feature, but the production delta is ~560 lines across 6 files; the rest is tests. The dnd.tsx disabled pass-through is optional and backward compatible. Lowering the dead-key BASELINE 29 -> 28 by deleting the confirmed-unused show_collapsed_folders across all 13 locales is a strict improvement, not ratchet gaming. temp-screenshots/ is a sanctioned committed deliverable per .gitignore. The feature is documented in the module spec, so the spec-management rule is satisfied. No hardcoded user-facing English.
Non-blocking follow-ups, none worth another round:
activePinMutationBatchclears only once every entry settles, so a request that never settles leavespinMutationKeysInFlight()non-empty for the page lifetime, which permanently defers baseline seeding and membership reconciliation. Soft failure (stale order, reload recovers), but a settle deadline would remove the latch.- If
commitPinnedSessionSnapshotitself throws after the try block has already nulledactivePinMutationBatch, the catch path runs a second commit with fallback semantics. RequiressafeSetItemto throw, so unreachable in practice. slotPinGenerationsis never pruned whenreconcileSlotsdrops a slot. Memory-only and bounded by keys touched in one page load, so trivial, but it is a Record that only grows.
Maintainer decision, restated and unchanged: pin ORDER stays browser-local while pin MEMBERSHIP stays server-side. This is the source of the entire reconciliation surface and means order does not follow a user across browsers, profiles, or the desktop app versus the localhost tab. I am accepting it for this PR because the blast radius is confined to sidebar ordering and the worst case is a wrong order that a reload fixes -- no server state and no other user's data can be reached. If pinned order is ever expected to sync, the right change is moving rank server-side next to membership, which deletes most of this machinery rather than adding to it.
All 66 checks green on this head, PR Readiness green at 10:04, all five AI review lanes fresh and clear, zero unresolved threads. Approving as a kirocrew-ux-reviewers member, which also satisfies the UX ruleset for the website/src/** non-test files.
Problem / Motivation
Pinned sessions are promoted above other sessions, but their order is still recomputed from the selected automatic sort. Ordinary row-over-row dragging therefore snaps back; under Created: newest, creation time immediately wins again. The sidebar also has no visual boundary explaining where pin priority ends and automatic sorting begins.
Why it matters
Users pin a small working set because they want those sessions to stay in a deliberate, stable order. The existing whole-row drag affordance suggests that ordering is possible even though only folder, root, and chat-reference targets have an effect. A stable pinned section removes that mismatch while preserving useful automatic sorting for every other session.
What changed (motivation → approach → change)
Alt+ArrowUp/Alt+ArrowDownon focused pinned rows.Tests
faef2c8a548c801b21dca97edd47c0f414bcf922: 77 focused tests passed; TypeScript, zero-warning source ESLint, the production/analyze build, exact bundle-size gate, feature-map gate, and narrow exact-SHA verifier passed locally.Manual verification
Scripted Playwright ran the real baseline and feature Vite applications at 1440×900 with identical session/API fixtures. It exercised actual dnd-kit mouse movement past the activation threshold, captured the insertion state, dropped a pinned row, verified the resulting DOM order, reloaded the page, and verified the persisted order remained unchanged.
The baseline and feature screenshots use the same six sessions and Created: newest. The baseline orders pins by creation date; the feature keeps the explicit pin order while the unpinned section remains newest-first.
Screenshots / video
Before — pinned sessions still follow Created:newest; no semantic boundary
After — manual pinned order and quiet double divider
Real pointer drag, insertion feedback, drop, and persisted reload
Recorded from
faef2c8a548c801b21dca97edd47c0f414bcf922· baseline/feature Vite origins · mode: Created:newest, fixed 350px sidebar · actual dnd-kit pointer events with deterministic API fixtures, no model or personal gateway data.Insertion and reload frames
Related Issues
Fixes #7370
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)