Repository navigation
base red: Cmd-N row slot tests (a placement's own rows pass drew the new row at the end) - #31069
Conversation
… rows pass runs CmdNRowSlotTests, CmdNInGroupRowSlotTests (3 of 4 cases) and ColdCmdNRowSlotTests (intermittent, 3 of 5) were red on feat-cmux-next. Austin's report: a new workspace row draws at the end of the list, then moves to its slot. Event order in the failing runs (SLOTDBG trace, job c8b7e3ff4e954c8e515b260c): 1. The press draws the pending row at its slot. 2. Reconcile: the daemon's workspace is live in the store; applyPendingPlacements takes its entry and clears it. 3. The placement's own refresh (placementSections, sameTurn.syncNow) runs a rows pass before the move is sent. The sidebar document does not place the item yet and does not lag (version == shownVersion), so PendingWorkspaceRows.listing listed the workspace's own row at the daemon's place (the end) and forget dropped the pending row. That frame was drawn. 4. bridge.place sends the move; the next frame has the row at its slot. A passing run is one where the store still showed the stand-in at step 3. The race is old (red at 58af48e, #27014, and at every later point tried), not a recent regression. Fix: a workspace whose slot WindowManager.pendingPlacements holds keeps its pending row at that slot until the placement is sent. The entry is cleared after the refresh, right before the move, and the observed PendingWorkspaceRows.unsentSlots (kept per id from pendingPlacements' didSet, so two apps in one process do not overwrite each other) re-runs the rows pass when it goes, also for a placement that sends no move. An anchor wait now updates its entry in place instead of clearing and setting it again. No wait, retry or delay. Line-neutral in CmuxNextApp (arch-ratchet app_lines).
GitHub Actions
Inactive
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41330433296 at e0fa06f failed: cmux-next crash ends the app. |
|
Found 4 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
|
Independent review subagent (correctness first), read-only, no build on the reviewing machine. It checked every path that sets or clears Review (subagent, correctness first): SHIP at 4cad31b |
lawrencecchen
added
the
ready-to-land
|
Review (subagent, correctness first): SHIP at 4cad31b
|
Summary
Base red on
feat-cmux-next(priority zero). Three tests check that a Cmd-N workspace row draws only at its slot from the first frame. On the base they fail often but not every time:CmdNRowSlotTests.cmdNDrawsTheNewRowOnlyAtItsSlot(:79)CmdNInGroupRowSlotTests.cmdNInAGroupDrawsTheNewRowOnlyAtItsSlotInTheGroup(:209)ColdCmdNRowSlotTests.firstColdCmdNWithRestoredFollowingRowsStaysAfterCurrent(:148)What the person sees: after Cmd-N, the new row draws at the end of the sidebar for one frame, then jumps to its slot.
Cause (an old race, not a recent commit). A temporary event-order trace (scratch branch, job
c8b7e3ff4e954c8e515b260c) shows this order in every failing run:applyPendingPlacementstakes the placement and clears its entry.placementSections→sameTurn.syncNow) runs a rows pass before the move is sent. The sidebar document does not place the item yet and does not lag (version == shownVersion). SoPendingWorkspaceRows.listinglists the workspace's own row at the daemon's place (the end),forgetdrops the pending row, and that frame is drawn.bridge.placesends the move, and the next frame has the row at its slot.A run passes only when the store still shows the stand-in at step 3. The tests also fail at #27014's merge (
58af48e51d8) and at every later point tried (jobs501830b7,76d6a7e9,fdc057e7,989300c4). No single commit caused it, so there is nothing to revert.Fix. A workspace whose slot
WindowManager.pendingPlacementsholds keeps its pending row at that slot until the placement is sent:PendingWorkspaceRows.unsentSlots, observed and kept per id frompendingPlacements'didSet, makeslistingkeep the pending row while the id is in the set. When the id leaves the set, the rows pass runs again, also for a placement that sends no move.There is no wait, retry or delay. The change adds no lines to CmuxNextApp (arch-ratchet
app_lines).The same fix makes
NewInNewGroupRowSlotTestspass: it is red at the base and green at the head.Testing
All runs are on the fleet (
scripts/ci/package-test-lane.sh suite Packages/macOS/CmuxNext …). Each suite runs in its own process, by filterCmuxNextAppTests.<Suite>.The base has an unrelated compile red:
AgentSessionAttachClientCloudRepositoryTestsfrom #30832, being reverted in #31023. Because of it, red and green ran on a scratch pair that is never merged:b33ff910=origin/revert/30832-cloud-repo-testsbf5d0de3= this branch merged with that revert568bf412e4357b6fb3679c3apassed (CmuxNext, app, cmux-tui).b33ff910, job569b74b326d9215319079152bf5d0de3, jobd6325ac61aa65ab5f2b77b46afterCurrent, bothinstantPage)More runs:
42e34005985a404cb3bcb9db: ColdCmdN 4 of 4 green (9 of 9 at head in total); CmdNInGroup 2 of 2 green isolated.9c948a88edacbc16cef0a662: cmdNDraws red (:79).396855a8a312466572ff426dand7adc0ba81250c7e855359687(ColdCmdN 3 of 5 red), both ate25bc25a. Isolated at feat tip86989822:78e8567a77c1016de04b3237.Neighbour suites (17), base
5a165f5875349b38b22b60acvs head05a772b114c67a826903c257:NewInNewGroupRowSlotTestsis red at base and green at head.SidebarCmdNumberTwiceTestsis red at both: a base finding, not caused by this PR.Observed / Not run
42e34005), one case failed atCmdNRowSlotTests.swift:162: the test's own New Group setup did not form within 30 s. That is a different check from the slot. The same :162 failure shows at base commits without this change (34129b0catd2b6b65d,55865c40at49f371cc), so it is a separate intermittent base finding.SidebarInsertBurstTestson a display worker (it skips without one); a tagged dogfood build. Not measured: frame timing (the change removes one rows pass's wrong frame; it adds no work on the main thread except a set update when a placement is added or removed).Changelog
Fixed: A new workspace's sidebar row no longer appears at the bottom for a moment before it moves to its place after Cmd-N.
Proof
No capture: the bug lasts one frame and depends on timing. The frame-by-frame row checks in the three tests above are the evidence. A still image cannot show a one-frame jump.
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes a one-frame race where, after Cmd-N, a new workspace row drew at the end of the sidebar before jumping to its slot. A workspace whose slot
WindowManager.pendingPlacementsholds now keeps its pending row at that slot until the placement is sent, so the row draws correctly on the first frame.Bug Fixes
PendingWorkspaceRows.unsentSlotskeeps the pending row listed while the placement is unsent, and re-runs the rows pass when the placement goes, even for placements that send no move.CmuxNextApp.Written for commit 4cad31b. Summary will update on new commits.