Skip to content

Keep channel threads in main and side workspace tabs - #905

Merged
morgmart merged 4 commits into
mainfrom
morganm/tabs-workspace-core
Oct 12, 2026
Merged

morgmart merged 4 commits into
mainfrom
morganm/tabs-workspace-core

Conversation

@morgmart

@morgmart morgmart commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What this does

Each channel keeps its open threads as tabs. Its conversation stays as the permanent first main tab; thread tabs can live in the main or side pane. Switching channels restores that channel's workspace during the session, and opening an already-open thread selects its existing tab.

This is slice 2 of the stack. Reply preservation (#904) and blank-tab removal (#850) are now merged into main and integrated here. New threads open in the side pane in this slice; the next PR adds the device preference. Pointer dragging, return highlighting and composer focus remain separate later PRs.

Why it matters

People can keep several threads open instead of repeatedly replacing one view. A full-width thread is available without losing the channel or the other work they have open.

New rule: A new thread adds a tab; opening an existing thread does not duplicate or relocate it.

How it works

One channel-owned, session-scoped store owns placement and selection. Retained thread content moves between the two existing pane hosts without recreating its editor; current reading offsets are captured before transfer. Option/Alt+Shift+Left/Right reorders and crosses panes. Back/Forward restores selection without discarding tabs. Closing the currently addressed thread returns to the channel, as explicitly approved.

Thread identity comes from the verified root. A misleading link hint cannot replace another confirmed thread or its draft, and late evidence cannot override newer selection or reopen a closed pane. There is no restart restoration, temporary rename, cross-channel main tab or general docking system.

Review fixes

  • Own-channel threads use the already-authorized current channel, including read-only public previews and readable archived channels. Other conversation tabs keep their existing restrictions; composer write restrictions are unchanged. Wes confirmed this prior fix.
  • Reading follows a retained thread's current pane rather than remembering its former tab header. Focus in the new selected header earns reading dwell; the former header no longer counts.
  • Escape in a body-portalled media viewer dismisses that viewer, preserving its thread tab and unfinished reply. Escape in the thread itself still closes the tab.
  • The prior Members/modal focus protections remain. Main's blank-tab removal and test: fix four CI flakes (avatar-loading, membership, buzz-links, WorkflowsPage) #911's profile-read test ordering are preserved through the rebase.

Verification

  • Hosted run on 7651120b5 finished with only the same stale Close Thread tab lookup failing in Chromium/WebKit shard 3; all other applicable checks passed, Windows native skipped. Test-only follow-up a351acc027b098eee8dc35ba2dc78118b5470100 updates that lookup to the existing selected-thread selector, preserving all keyboard-focus and unavailable-state assertions. The complete message-navigation file now passes all 44 Chromium/WebKit cases locally (1.6m). The push and all mandatory hooks passed. Final head a351acc02 passed all applicable hosted checks (Windows native validation conditionally skipped), and Wes’s final review on that exact head states “No changes requested.” Baxen’s approval remains.

  • On head 7651120b5fc78d370ba45aed01f2c405d5d0a018, rebased onto main be861c9a5: 109 focused owner tests and all 64 browser cases across message-management, buzz-links, Todos, navigation-thread-history, thread-unread, channel-tabs, thread-tab-moves and media-controls pass in Chromium/WebKit (2.3 minutes, local Apple Silicon macOS). Typecheck passed after rebase; final mandatory commit/push hooks passed. No source edits occurred during those runs.

  • The reading regression extends the existing real-hook ownership case, moving the retained surface without changing hook dependencies and controlling the dwell clock. It failed before the fix and passes afterward.

  • The existing browser media-confirmation journey now opens media from a thread. All previous confirmation focus/cancellation assertions remain, plus opener restoration, editor identity, draft retention and normal thread Escape. The extended case failed before the fix in Chromium and WebKit. No browser cases were added or removed in this repair; native focus and competing portal handlers justify this browser boundary.

  • Independent focused reviews found no remaining blocker in the two repairs or the Remove blank-tab creation from the side pane #850/test: fix four CI flakes (avatar-loading, membership, buzz-links, WorkflowsPage) #911 rebase integration.

  • Morgan and Baxen previously exercised the combined stack on desktop; that evidence is not new native acceptance of this rebased slice. All applicable hosted checks now pass for the test-only follow-up. No full scan, native build, hosted rerun or review-thread resolution performed. Morgan explicitly authorized merging after passing tests and Wes’s latest-head no-changes comment.

  • The previous hosted failure was the WebKit volume-knob animation test, not one of test: fix four CI flakes (avatar-loading, membership, buzz-links, WorkflowsPage) #911's four flake fixes. The complete media-controls file passes locally in both engines, with assertions unchanged. Hosted Linux WebKit now passes too.

  • Baxen's non-blocking request that Cmd+W never close the window remains a separate follow-up. The last-side-pane width/reflow jump is also deferred. Native/package acceptance and human confirmation of these latest fixes remain separate gates.

Screenshots

Synthetic identities and fixture content; these captures show the combined stack, including later slices.

Light main-pane thread tabs
Dark main-pane thread tabs

Stack

Slice 2 of 6. Based on main, including merged Reply preservation #904 and blank-tab removal #850. Next: Placement setting #906.

Later stack PRs still need restacking onto this updated head; they are being handled individually.

@morgmart
morgmart marked this pull request as ready for review October 12, 2026 19:12
@morgmart
morgmart requested a review from a team as a code owner October 12, 2026 19:12

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three changes needed, detailed inline.

Star Lord’s automated source review via Wes’s account; head ae1fa1b6776e793c0a6fdb82ce24b0bcd615b147, base a310ea71e0ce371218f1138c3c36b2c801632e55. No code or tests executed. The current-head CI snapshot has failing JavaScript and browser jobs; causes were not diagnosed here. Inspected both attached screenshots, which depict the combined stack rather than isolated-slice acceptance.

Comment on lines +583 to +585
const node = nodes.get(tab.id);
const target = channels.find((item) => item.id === tab.channelId);
if (!node || !target || !usableChannel(target)) return [];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Preserve readable primary-channel threads. Primary threads now render through this lookup, but channels comes from useChannelLabels, which excludes archived channels, and usableChannel rejects read-only public previews. ChannelsPage deliberately resolves those current channels from the full roster/exact lookup, and DiscoveryState.canAccess permits reading them. Opening their reply count or an exact reply link therefore produces ‘This conversation is no longer available’ instead of a reader. Resolve own-channel tabs from the authorized current channel and distinguish read access from write/picker eligibility; retain composer write restrictions. Cover a joined archived channel and an unjoined public preview.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Addressed by the current-channel admission fix, retained through the rebase in 7651120. Own-channel thread tabs use the already-authorized current channel, including public read-only previews and readable archived channels; unrelated tabs retain their existing restrictions and composer write guards are unchanged. Thank you for confirming this repair in your follow-up. The rebased browser link/channel-tab checks pass in Chromium and WebKit. Leaving the thread open for verification.

const saved = node.isConnected
? scrollers(node).map((element) => element.scrollTop)
: offsets.get(node);
parent.append(node);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Rebind reading focus after moving a retained thread. use-reading.ts:48–65 captures the closest tabpanel once in its effect. This reparent preserves the reader and all that effect’s dependencies, so moving a settled thread to the other pane leaves focusedTab tied to the removed source panel. The move focuses the destination header, but header-only reading no longer earns dwell/clears unread until some unrelated dependency changes or the user focuses the content. Resolve the current owning panel inside focusedTab (or explicitly update the binding on transfer), and cover destination-header dwell after a pane move with unchanged thread data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Fixed in 7651120. Reading now resolves the thread’s current owning tabpanel at each focus check, rather than retaining its former pane. Extended the existing real-hook ownership regression: reparent without changing hook dependencies, confirm the old header does not count, then confirm the new selected header earns dwell. It failed before the fix and passes afterward. All 109 focused owner tests and 64 affected Chromium/WebKit cases pass; fresh hosted CI is pending. Leaving the thread open for verification.

Comment on lines +601 to +608
if (
event.key === "Escape" &&
!event.nativeEvent.isComposing &&
event.nativeEvent.keyCode !== 229 &&
!event.defaultPrevented
) {
event.stopPropagation();
closeConversationTab(tab.id);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 — Ignore Escape from portalled media viewers. This wrapper handles React-bubbled events without the DOM-containment check used by PanelWorkspace. A thread’s MediaReviewViewer portals to document.body; its Escape dismissal runs later in the document-level useModalBoundary handler. Pressing Escape in that viewer reaches this wrapper first, closes the entire thread tab and stops the viewer’s own handler, rather than just dismissing media and restoring its opener. Require event.currentTarget.contains(event.target as Node) before closing, as the existing workspace does. Cover opening media from a thread and pressing Escape while keeping that tab/editor mounted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Fixed in 7651120. The retained-thread Escape handler now uses the same DOM-containment guard as PanelWorkspace, so body-portalled media keeps its own dismissal and focus restoration. Extended the existing media-confirmation browser journey to open from a thread and verify the same editor/draft survives viewer Escape; a subsequent Escape in the thread still closes its tab. Existing confirmation focus/cancellation assertions remain. The extension failed before the fix in both engines and now passes in both; all 64 affected browser cases pass. Leaving the thread open for verification.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two prior changes remain: moved-thread reading focus and portalled-viewer Escape. Their affected paths are unchanged; the earlier inline repair guidance still applies.

The primary-channel preview/archived-thread admission finding is addressed. No new actionable defect found in the preview-admission and Members-focus fixes.

Star Lord’s automated source review via Wes’s account; head 4298e3261e63f274cbf4a569cb02bc5b89a8f21d, base a310ea71e0ce371218f1138c3c36b2c801632e55. Source-only follow-up, including both attached screenshots; no app or tests executed. The current-head CI snapshot still had JavaScript, browser and Rust checks running. Not an approval or runtime-validation result.

@morgmart
morgmart force-pushed the morganm/tabs-reply-preservation branch from a310ea7 to 0c230d0 Compare October 12, 2026 21:29

@baxen baxen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on behalf of baxen. They tested the full stack from #909 on desktop: tab drafts surviving channel switches, moving tabs between panes (a draft moved slightly), reusing an already-open tab, the Main pane setting, resuming the cursor on tab return, the return highlight, and same-thread links.

Follow-up from baxen, not blocking: ⌘W should never close the whole window. When there's no thread tab to close, it should do nothing. That matters here because since this slice, a side-pane thread only takes ⌘W when focus is inside the side pane. With focus in the channel (main pane: only the permanent channel tab) and a thread open on the side, ⌘W falls through to closing the window.

Base automatically changed from morganm/tabs-reply-preservation to main October 12, 2026 22:33
morgmart and others added 3 commits October 12, 2026 15:36
Retain thread editors across pane moves, restore channel-local tabs, and support keyboard placement. Pointer dragging, placement preference, highlight and focus handoff ship in separate slices.

Co-authored-by: Carl <goose@block.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Use the verified current channel for read-only thread presentation without expanding access or enabling writes. Preserve modal focus during routed tab selection and exact message reveal.

Co-authored-by: Carl <goose@block.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Resolve reading focus from the current pane after reparenting. Leave body-portalled media Escape to the viewer rather than closing its parent thread. Extend existing ownership and media-confirmation regressions.

Co-authored-by: Carl <goose@block.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
@morgmart
morgmart force-pushed the morganm/tabs-workspace-core branch from 4298e32 to 7651120 Compare October 12, 2026 22:44

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No changes requested: both remaining findings from the previous review are addressed—reading follows the retained thread’s current tab panel, and Escape in a body-portalled media viewer no longer closes its parent thread. I found no new actionable regression in these repairs or the #850/#911 integration.

Star Lord’s automated source review via Wes: head 7651120b5fc78d370ba45aed01f2c405d5d0a018, base 763c023e1d94fd87c22adf70e418586c2a318862. Source-only: no tests or app execution; hosted CI was still in progress at the snapshot, and native/human acceptance remains unverified.

Replace the remaining generic Close Thread tab lookup with the existing selected-thread selector. Preserve the deletion, local-unavailable-state and native Tab focus assertions.

Co-authored-by: Carl <goose@block.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No changes requested for this test-only follow-up. The selector targets the selected thread tab’s close control despite its dynamic label; the native Tab-focus, deletion, unavailable-state, retry-control and composer assertions remain intact. Production code is unchanged from the previous reviewed head.

Star Lord’s automated source review via Wes’s account — head a351acc027b098eee8dc35ba2dc78118b5470100, base 763c023e1d94fd87c22adf70e418586c2a318862. Source-only: no tests or app execution; both attached screenshots inspected, but they depict the combined stack. Hosted CI was still running at the snapshot; Windows validation was skipped and native acceptance remains unverified. Non-blocking COMMENT, not approval.

@morgmart
morgmart merged commit af303df into main Oct 12, 2026
23 checks passed
@morgmart
morgmart deleted the morganm/tabs-workspace-core branch October 12, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants