Repository navigation
Drag thread tabs to arrange the channel workspace - #907
Conversation
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Two changes needed: make overlapping-header hit testing follow the visible pane, and cancel an in-flight drag when navigation changes within the same channel. Details and minimal fixes are inline.
Star Lord’s automated source review via Wes’s account; head 24ad1e8d21c5c18ed205e36fe5e645cccabc85a6, base 27ff5cdd07594a141db940ddd61be97387bc4088. Source-only: no app or tests executed. The public description has no attached images. The current-head CI snapshot reports CI required failing and a WebKit shard cancelled; causes were not diagnosed. This is a non-blocking COMMENT, not an approval.
| for (const [pane, host] of hosts.current) { | ||
| if (!visible(host) || !latest.current.canMove(active.id, pane)) | ||
| continue; |
There was a problem hiding this comment.
P2 — Choose the frontmost header when pane bounds overlap. In the supported narrow-window/container layout, Channels.module.css:521–559 overlays the side pane (position: absolute; z-index: 2) without shrinking the main pane. Both headers pass visible() and this loop returns the first geometric match; normal main-then-side registration therefore targets the covered main header even when the pointer is over the side header. Reordering an own-channel side thread can unexpectedly move it to main instead. Resolve the actual frontmost header (or explicitly prioritize the overlay) before computing insertion, and cover a narrow desktop pointer drag. This preserves the design guide’s existing narrow-overlay behavior.
There was a problem hiding this comment.
🤖 Fixed in 5e7a1ee. The existing drag owner now uses frontmost pointer hit testing for both insertion and tab-strip autoscroll, so a visible side overlay wins over the covered main header. Forgiving geometric bounds remain when the pointer is outside registered hosts.
Added an actual narrow-overlay pointer reorder regression. It moved the tab into main before the fix in both Chromium and WebKit; now it preserves the side pane and verifies the reordered tab IDs. All 30 affected browser cases, typecheck and mandatory push checks pass. Fresh hosted CI is running; leaving the thread open for verification.
| const drag = useTabDrag({ | ||
| visit, |
There was a problem hiding this comment.
P2 — Bind drag cancellation to the navigation visit, not only the channel. visit above is memoized only by [queries, currentId], so useTabDrag’s effect cleanup does not run for Back/Forward between thread visits in the same channel. ChannelsPage keeps the workspace mounted and enterVisit restores selection without removing the dragged tab. If history changes while the pointer is held, releasing still commits the old gesture and moveTab selects its thread, overwriting the newly restored selection. Pass an actual navigation-entry/attempt cancellation token into the drag owner (separately from the channel lifetime) and verify that same-channel history cancels without committing a move.
There was a problem hiding this comment.
🤖 Fixed in 5e7a1ee. Both page owners now pass their existing navigation AbortSignal separately from the channel lifetime. Aborting navigation immediately cancels the held gesture; changing the signal retires its listeners, and already-aborted navigation cannot start a new drag.
The real browser regression goes Back between two same-channel threads while holding a drag, checks cue retirement before pointer release, then verifies unchanged placement/order and the restored selection. It failed before the fix and passes afterward in Chromium and WebKit. The full four affected browser files pass all 30 cases, and independent focused review found no blocker. Leaving this thread open for verification.
baxen
left a comment
baxen
left a comment
There was a problem hiding this comment.
🤖 Approving on behalf of baxen. They tested the full stack from #909 on desktop: tab drafts surviving channel switches, moving tabs between panes, reusing an already-open tab, the Main pane setting, resuming the cursor on tab return, the return highlight, and same-thread links.
27ff5cd to
0c05165
Compare
Use full header insertion targets and right-edge separation, retain content on drop, and size short tabs to their content. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Establish host CSS layer order before drag rules; keep compact navigation width expectations aligned with content-sized tabs, and keep artwork fallback changes from shifting the avatar baseline. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Use frontmost hit testing for overlay insertion and edge scrolling. Cancel the existing gesture on navigation abort, including same-channel Back/Forward. Cover both with real pointer regressions in Chromium and WebKit. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
24ad1e8 to
5e7a1ee
Compare
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
No further changes requested. The two findings from the previous review are addressed: frontmost-pane hit testing governs insertion/autoscroll, and navigation aborts cancel held drags within the same channel; I found no additional actionable defect in the repair or regression-test source.
Star Lord automated source review via Wes’s account; head 5e7a1ee54c0b688d6c55fc90735f7fa534fff032, base 55b71bf1db0ff6f5700dafc0d9f347a7d52ec8ed. Source-only: no tests or app executed; CI was incomplete at the inspected snapshot, and fresh native/human acceptance remains unverified.
An older finite head can arrive while the first live row mounts. Do not request Virtua shift compensation until it has measured a viewport; otherwise WebKit can freeze the initial empty range. Hold the real head response in the existing Me recipient journey to cover this ordering without weakening admission assertions. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
|
🤖 The CI failure exposed a real WebKit initialization race, now fixed in cea3650: an older finite head arriving before the first viewport measurement could freeze the virtual list’s initial empty range. The existing timeline owner now waits for measured geometry before requesting prepend compensation. The existing Me test holds the real finite response to exercise that order; it failed against pre-fix production in WebKit and passes after repair. All 128 timeline tests, all 44 affected Chromium/WebKit browser cases, typecheck and push hooks pass. No dependency patch or relaxed assertions. @wesbillman, please include this focused follow-up in the final no-further-issues review. Fresh CI is running; merging still waits for both gates. |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
One change requested: make the new cold-viewport regression deterministic; the inline comment identifies the missing ordering boundary and a minimal test-layer fix. I found no additional production defect in the viewport guard; the prior drag-fix review remains unchanged.
Star Lord automated source review via Wes’s account; head cea3650863d3f4d6a94dadda64a0c36a2b40ea80, base 55b71bf1db0ff6f5700dafc0d9f347a7d52ec8ed. Source-only: no tests or app executed; browser, JavaScript and Rust checks were still running at the single CI snapshot, and native/human acceptance remains unverified.
| `[Open Beta](buzz://channel/${other})`, | ||
| ); | ||
| await expect.poll(() => readHead).toBe(true); | ||
| releaseHead(); |
There was a problem hiding this comment.
P2 — Make the cold-viewport regression ordering explicit. readHead only proves route.fetch() completed; app.append() only queues the relay publication (fixture.mjs:2420–2429), not its receipt/render in the page. Releasing here therefore does not establish that the live row mounted before the finite head or that Virtua still has a zero viewport. If the head wins delivery, there is no prepend; if viewport measurement wins, the previous shift={prepend} also works. The new regression can consequently pass with the production fix removed. Keep the browser journey, but add a mounted timeline regression controlling viewportSize at zero versus measured and asserting the corresponding prepend shift value, or explicitly gate those browser boundaries. This satisfies the repository's deterministic-ordering rule without adding sleeps.
There was a problem hiding this comment.
🤖 Addressed in test-only follow-up 1613837. Added a real mounted ChannelTimeline regression that controls the independent virtualizer’s public viewportSize at zero versus measured and prepends the older row on the same mounted instance. React owns actual refs and effects; no hooks are mocked and no production seam was added.
Removing the production guard deterministically fails the zero-viewport case while the measured case passes; restoring it passes all 130 timeline tests and typecheck. The original browser journey remains, with its comment corrected so it does not claim guaranteed measurement ordering. Independent focused test review is clear. Production is unchanged from cea3650, whose hosted CI passed; fresh CI is running on this head. Leaving the thread open for verification.
Exercise the real timeline against zero and measured Virtua viewport states, retaining the native browser journey for integration. Removing the production guard fails the zero-viewport case deterministically. Co-authored-by: Carl <goose@block.xyz> Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
No further changes requested. The two mounted timeline cases address the prior ordering finding by explicitly controlling zero versus measured viewport size across a same-instance prepend; the existing browser journey remains, with no new browser cases or production changes in this follow-up.
Star Lord automated source review via Wes’s account; head 1613837dced7f140a10a5bd418dbe91761a5642d, base 55b71bf1db0ff6f5700dafc0d9f347a7d52ec8ed. Source-only: no tests or app executed; CI was incomplete at the single snapshot, and native/human acceptance remains unverified.
What this does
Thread tabs can be dragged to reorder them or move them between the main and side panes. A floating title pill and insertion line reuse the Panel Swap treatment. The whole pane header accepts a drop; dragging below the header toward the right edge can reveal an empty or closed side pane.
This is slice 4, rebased onto main containing merged #904–906 and #850. Short navigation tabs now hug their content up to the existing maximum width instead of reserving a fixed empty area.
Why it matters
Arranging a few open threads should be a direct gesture, not another set of move buttons. Generous targets make the destination discoverable without adding permanent chrome.
How it works
The existing tab store commits placement only on release. Escape, cancellation, window blur, resize and visit change cancel the proposal. Closed-pane drops preserve hidden tabs; drafts and readers remain mounted. Drag feedback redraws independently of the conversation page, and long strips scroll at their edges only during the gesture. The permanent channel tab and non-thread detail tabs are not draggable. Touch keeps native tab-strip scrolling; keyboard placement remains available.
Review repairs and verification
Test-only follow-up
1613837dced7f140a10a5bd418dbe91761a5642daddresses Wes’s deterministic-ordering comment: a real mounted ChannelTimeline now controls the independent virtualizer’s public viewport measurement at zero versus measured, then prepends older data on the same component. Removing the guard fails the zero case; the measured case preserves normal compensation. All 130 timeline tests and typecheck pass; independent test-honesty review is clear. The browser journey remains for native integration, not proof of exact measurement timing. No production code changed in this follow-up.Previous production head
cea365086passed all applicable hosted CI, including the formerly failing WebKit shard (Windows validation conditionally skipped). Final head1613837dcpassed all applicable hosted checks (Windows native conditionally skipped), required approval remains, and Wes’s final exact-head review states “No further changes requested.” Final comment sweep is clear; Morgan authorized merging at these gates.CI rendering repair at production head
cea3650863d3f4d6a94dadda64a0c36a2b40ea80: WebKit could display a blank timeline when an older finite response arrived after a live row but before the first viewport measurement. The store contained the messages; premature prepend compensation froze Virtua’s initial empty range. ChannelTimeline now enables shift compensation only after Virtua has measured its viewport. No dependency patch, timer, retry or new lifecycle owner.The existing Me recipient journey now holds the real finite response to exercise this ordering. It failed before the fix in WebKit and passes afterward; all signing/admission assertions remain. No additional browser cases or relaxed waits/assertions. Typecheck, 128 timeline/installed-Virtua tests, and all 44 cases across Me, history-loading, live and initial-position files pass in Chromium/WebKit (1.7m, local macOS). Temporary instrumentation and probes were removed; the installed dependency was restored byte-for-byte. Independent focused review found no blocker.
Mandatory commit/push hooks passed. The original blank-timeline failure was repaired without a hosted rerun, dependency change, test retry or assertion relaxation.
Drag fixes at
5e7a1ee54, retained in the final head above, base55b71bf1d. Narrow overlaid side panes now win actual pointer hit testing for both insertion and tab-strip autoscroll; an underlying main header cannot take their drop. Existing geometric tolerance remains when the pointer is outside registered hosts.A current navigation AbortSignal now cancels the existing drag owner immediately on navigation, including same-channel Back/Forward. Both Channels and Sessions supply their existing signal; this does not broaden the channel-lifetime guard or add another navigation/gesture owner.
Two browser scenarios added (four engine cases), no cases removed: native overlay hit testing and held pointer capture through actual browser history cannot be established by jsdom. Each failed for the intended defect before repair in Chromium/WebKit, then passed afterward. Assertions check final pane/order and restored selection, not only cue dismissal.
Typecheck and all 30 cases across complete navigation-thread-history, thread-tab-moves, channel-settings and buzz-links files pass in Chromium/WebKit (1.1m, local Apple Silicon macOS). Existing cases cover full-header insertion, closed/empty-side reveal, Escape cancellation and retained reader/editor/draft identity.
Independent focused rebase/remedy/test review found no blocker. Mandatory commit/push checks passed; remote head verified. Final hosted CI and Wes’s exact-head no-further-changes confirmation passed under Morgan’s explicit merge authorization. No manual CI rerun, full scan, new native build or thread resolution.
Morgan previously tried desktop dragging and approved the shared Panel Swap treatment. This repair changes no tokens, styles or geometry; new narrow-window pointer behavior is browser-verified, not fresh native/human acceptance. Sessions consumes the same cancellation owner but has no separate native-drag acceptance claim.
Earlier CI repairs retained
Drag feedback styles load through the shared stylesheet, not an eager UI import that changes CSS-layer order. Shared avatar alignment stays stable through artwork fallback. Channel-settings tests enforce the approved maximum tab width rather than the removed fixed width. These repairs are retained; broad fresh CI owns their current cross-platform validation.
Stack
Slice 4 of 6. Based on main including merged Placement setting #906. Next: Return highlight #908. Later slices are restacked individually after their predecessors merge.