Skip to content

Choose the default pane for new threads in Messages - #906

Merged
morgmart merged 3 commits into
mainfrom
morganm/tabs-placement-preference
Oct 12, 2026
Merged

morgmart merged 3 commits into
mainfrom
morganm/tabs-placement-preference

Conversation

@morgmart

@morgmart morgmart commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What this does

Adds Messages → Open threads in to Settings, with Side pane and Main pane choices. Side pane is the default. The preference is saved on this device and applies only to newly opened threads; existing tabs keep their placement.

This is slice 3, rebased onto main including merged workspace tabs #905, reply preservation #904 and blank-tab removal #850. The Messages plugin contributes its own settings destination through the existing settings mechanism—there is no core Settings-framework change. The contribution belongs to Messages rather than Agents; Messages is currently a required plugin and cannot be disabled through Settings.

Why it matters

Someone who prefers the larger thread view can make it the default without moving every new thread. Deliberately moving a tab never changes where the next thread opens.

How it works

Ordinary and link-opened threads read the same device preference. Stored values are validated, and storage failure keeps the chosen value effective while showing a retry. The shared preference row retains the explanation that already-open threads stay put. Flat sessions keep their existing side-only host; legacy “Remember mentioned agents” remains unchanged.

Review repair and verification

  • Head 0c0516524fc9bb8060e86c69e710c2f7dedb92eb, base af303dffe. Failed saves keep their warning and Retry saving action when leaving and returning to Messages settings. Recovery derives from the preference owner's existing unsaved choice, not component-local error state. Retrying the same active choice clears the warning after storage succeeds.
  • Extended the existing real React failure test through destination unmount/remount and successful retry. It fails against pre-fix production for the missing recovery notice and passes after repair. The app's toast host remains mounted, matching actual Settings lifetime. No test-only production seam, new retry loop, storage format change, or browser case added/removed.
  • Typecheck and 21 focused Settings/tab-state/tab-hook/plugin-wiring tests pass; the final formatting-only test cleanup was rechecked with both Settings cases. All 24 cases in the complete Settings and navigation-thread-history files pass in Chromium and WebKit (36.4s, local Apple Silicon macOS). Existing controls and layout are unchanged; prior complete-stack visual/native acceptance remains separately scoped.
  • Independent focused remedy/rebase review found no blocker. Existing flat-session side-only behavior, archived/read-only presentation, previously reviewed focus fixes and main's blank-tab removal are preserved.
  • Settings keyboard traversal includes the Messages destination at both existing barriers without weakening downstream focus assertions. No original CI jobs rerun. All applicable hosted checks passed on 0c0516524 (Windows native conditionally skipped), Wes’s final exact-head comment states “No changes requested,” required approval remains and final new-comment sweep is clear. Morgan explicitly authorized merging at these gates.
  • Mandatory commit/push checks passed; remote head verified. Review thread remains open for reviewer confirmation. No native build, full scan or new native acceptance claimed.

Screenshots

Messages placement preference, light
Messages placement preference, dark

Stack

Slice 3 of 6. Based on main containing merged Workspace tabs #905. Next: Dragging #907. Later slices still need restacking as each is handled.

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

@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.

One change needed: retain failed-save recovery when leaving and reopening Messages settings. The active in-memory choice survives that navigation, but its error and Retry action do not (inline).

Star Lord automated source review via Wes’s account. Head 494eabe891a83180bb3c83e9e651f8d6b6c58888; base ae1fa1b6776e793c0a6fdb82ce24b0bcd615b147. Source-only; both attached screenshots inspected. No tests, app execution or CI validation; interactive/native behavior remains unverified.

/** Registered by Messages; its settings presentation follows plugin lifetime. */
export function MessageSettings({ active }: { active(): boolean }) {
const placement = useThreadPlacementPreference();
const [error, setError] = useState<string | null>(null);

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] Keep failed-save recovery across settings remounts

error lives only in MessageSettings, while the unsaved choice lives in the module-level unsavedPreference. If saving Main pane fails, then the user leaves this Settings destination to try a thread and returns, Settings.tsx:179–199 has unmounted and recreated this component. The selector still shows Main pane, but error starts as null, so the warning and Retry saving action are gone even though storage still has the old value. Restarting then silently restores the old placement. Keep the persistence failure alongside the unsaved preference (and derive the recovery UI from that state), so returning to Settings can retry the active choice. Add a failed-save → unmount/remount → successful-retry test; the current failure test never leaves the mounted page.

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 0c05165, rebased onto main with merged #905. The warning and Retry saving now derive from the preference owner’s existing unsaved state rather than component-local error. A successful retry notifies recovery even when the chosen pane is unchanged.

Extended the existing mounted failure test through Settings unmount/remount with the app toast host retained, then successful retry. It fails against pre-fix production for the missing warning and passes after the repair. Typecheck, 21 focused owner/wiring tests, all 24 Settings/navigation-thread-history cases in Chromium/WebKit, and mandatory push checks pass. Fresh hosted CI is running; leaving the thread open for your 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.

One change remains: the failed-save recovery finding is unchanged—leaving and reopening Messages settings still loses the warning and Retry action. The keyboard-traversal additions preserve the downstream focus assertions; no new finding in that repair.

Star Lord automated source review via Wes’s account. Head 27ff5cdd07594a141db940ddd61be97387bc4088; base 4298e3261e63f274cbf4a569cb02bc5b89a8f21d. Both attached screenshots inspected. Source-only: no tests, app execution or CI validation; browser/native behavior remains unverified.

@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, reusing an already-open tab, the Main pane setting, resuming the cursor on tab return, the return highlight, and same-thread links.

@morgmart
morgmart force-pushed the morganm/tabs-workspace-core branch from 4298e32 to 7651120 Compare October 12, 2026 22:44
Base automatically changed from morganm/tabs-workspace-core to main October 12, 2026 23:17
morgmart and others added 3 commits October 12, 2026 16:17
Register plugin-owned Messages settings and save the placement choice on this device; existing tabs keep their placement.

Co-authored-by: Carl <goose@block.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Co-authored-by: Carl <goose@block.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
Derive the warning and Retry saving action from the preference owner's unsaved state, so leaving and reopening Messages preserves recovery. Extend the existing mounted failure test through unmount/remount and successful retry.

Co-authored-by: Carl <goose@block.xyz>
Signed-off-by: morgmart <98432065+morgmart@users.noreply.github.com>
@morgmart
morgmart force-pushed the morganm/tabs-placement-preference branch from 27ff5cd to 0c05165 Compare October 12, 2026 23:22

@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 in this follow-up. The failed-save recovery finding is addressed: unsaved state now owns the warning across Settings remounts, and same-choice retry has regression coverage.

Star Lord automated source review via Wes’s account; head 0c0516524fc9bb8060e86c69e710c2f7dedb92eb, base af303dffead56c7e30765d1d55c0b776d25c6c14. Source-only; PR description and both screenshots inspected. No tests, app execution or CI validation; browser/native behavior remains unverified.

@morgmart
morgmart merged commit 55b71bf into main Oct 12, 2026
23 checks passed
@morgmart
morgmart deleted the morganm/tabs-placement-preference branch October 12, 2026 23:38

@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.

🤖 One change needed: a diverted thread link ignores its host's pane. With Main pane selected, it vanishes in Sessions (inline). The save-recovery repair and Settings keyboard traversal look right.

Validation: unit probe of useChannelTabState at this head; source review of the Settings card and preference store. CI is green. Not checked: the native app, and the in-app flow with Main selected.

Merge criteria: the divert path keeps the diverted tab's pane, plus a regression test.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

requested,
"select",
"side",
threadPlacementPreference(),

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] Keep a diverted thread in the pane it was diverted from

A diversion is not a new user-placed thread. It replaces what the user just asked to see in an existing tab. Using the global preference here breaks flat hosts. SessionsPage uses useConversationTabs without mainPane, so mainTabs is always empty there (useConversationTabs.tsx:253), but onDifferentThread → divertThread is still wired.

Repro (unit probe at 0c051652): setThreadPlacementPreference("main"), openThreadTab("alpha", "reply-y", "x", "select", "side") (what a flat host does), then divertThread("thread:alpha:x", "reply-y", "x"). The new tab is { id: "thread:alpha:reply-y", pane: "main" } with mainSelected set, while selected stays thread:alpha:x. In Sessions the requested thread never renders, and the side pane keeps showing the old thread. In Channels the link opened from the side pane jumps to Main.

Fix: pass tabPane(tab) instead of threadPlacementPreference(), and add a test with the preference set to Main.

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 in draft follow-up #927, head f30a4db. The provisional correction now uses the originating tab’s pane rather than the new-thread default; an already-open verified destination still keeps its own placement.

Extended the existing mounted regression for both panes with the opposite default. Both cases failed before the fix and pass afterward, preserving the original thread, selection, destination deduplication and saved preference. Typecheck, 65 focused owner tests, all 16 navigation-thread-history Chromium/WebKit cases, independent focused review and push hooks pass. Fresh CI and native Sessions acceptance remain separate; leaving this thread open for verification.

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