Skip to content

base red: the tab bar's menu counts for New Tab and Reopen Closed Tab (ActionSurfaceParityTests) - #33092

Merged
lawrencecchen merged 1 commit into
feat-cmux-nextfrom
base-red-tab-strip-parity
Nov 3, 2026
Merged

lawrencecchen merged 1 commit into
feat-cmux-nextfrom
base-red-tab-strip-parity

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Base red on feat-cmux-next: CmuxNextActionsTests.ActionSurfaceParityTests.everyTargetKindMenuShowsItsActions fails at ActionSurfaceParityTests.swift:79 with gaps newTab.default (tab) and reopenClosedBrowserPanel (tab).

Cause: a9c5b0d (#32127, vertical tabs) added the tab bar's empty-area menu (ActionMenuContext.tabStrip, whose target is the pane) and placed New Tab and Reopen Closed Tab there, as Chrome's and Edge's tab strip menus do. A placement replaces an action's menu exemption, so both lost theirs. The parity rule then saw a tab action placed only in a pane menu.

The rows work as placed: the app passes the pane, newTab.default takes a pane target as its place (TabLifecycle.newTabOfPaneKind), and Reopen Closed Tab reopens the key window's newest closed group whatever the target. The rule had no way to say that the tab bar's rows act on what its pane holds. ActionMenuContext.contentKinds now says it, for the tab bar only ([.tab]). Every other menu keeps the rule as it was: a pane's own menu still does not count for a tab action.

Owner: no OWNERS.md row covers CmuxNextActions/ActionTarget.swift; the tab strip menu came from #32127 (teamleaderleo).

Testing

Command: cmux-ci run --class light --display --script scripts/ci/package-test-lane.sh --ref SHA --arg=suite --arg=Packages/macOS/CmuxNext --arg=CmuxNextActionsTests.ActionSurfaceParityTests (5 runs in one step).

  • Base (feat 73eaec7, two hosts; and feat 11dfe91, job 2bf90b6c18ca136f3fb6c15d): fails at ActionSurfaceParityTests.swift:79 with the two gaps.
  • Tip: job a554671b75a6b853405d5c5d at feat 11dfe91 (cmux8s) is red on the first two of five rounds for this suite.
  • Head: job fd9d5806aa9d105236d0a0ef (cmux14, GUI) and job 34296cd108549dff83f6d6df (aws-m4pro-3, no GUI) at de406cb: the suite passed 5 of 5 in each.

Each job's lane summary is red for a reason outside this change: on these hosts the target's XCTest pass aborts (signal 6, after Contacts/AddressBook CoreData store errors) or hangs ~680-900 s with no test in flight, after the Swift Testing pass. The Swift Testing results below are the suite's own.

Observed / Not run

Observed: the fleet runs above. python3 scripts/cmux-next/prepush-gate.py passes.
Not run: no app build or manual check of the tab bar menu; the change touches only the parity rule's input, not the menu.

Changelog

none

… (ActionSurfaceParityTests)

everyTargetKindMenuShowsItsActions failed at ActionSurfaceParityTests.swift:79
with gaps newTab.default (tab) and reopenClosedBrowserPanel (tab). a9c5b0d
(#32127, vertical tabs) added the tab bar's empty-area menu
(ActionMenuContext.tabStrip, whose target is the pane) and placed both there,
as Chrome's and Edge's tab strip menus do. A placement replaces an action's
menu exemption, so both lost theirs (newTab.default's duplicateOfDefault was
removed; reopenClosedBrowserPanel's noObject no longer applies), and the
parity rule found a tab action placed only in a pane menu.

The rows work: the app passes the pane, and newTab.default's handler takes a
pane target as its place (TabLifecycle.newTabOfPaneKind, namesPane);
Reopen Closed Tab reopens the key window's newest closed group whatever the
target. What the rule could not say is that the tab bar's rows act on what
its pane holds. ActionMenuContext.contentKinds now says it, for the tab bar
only ([.tab]); every other menu keeps the rule as it was (a pane's own menu
still does not count for a tab action).

Culprit: a9c5b0d (#32127).
Red: fleet job 2bf90b6c18ca136f3fb6c15d at feat 11dfe91.
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review de406cb: HQ feed card · gallery before/after runs · checks

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=pr33092 -f ref=base-red-tab-strip-parity -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 6b31a88 to compare; do not merge until green; see lane-rules

Shipped in:

@blacksmith-sh

blacksmith-sh Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Found 6 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmppdpmuyqw/summary-missing.json (live):/summary-missing.json (live): View Logs
/tmp/tmpqlbq0rcs/summary-fail.json (live):/summary-fail.json (live): View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows
View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/
[tabs] debug.tab_switch timed 0 of 40 switches
View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames 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

Review (independent review subagent, Opus 5.5 claude-opus-5-5, read-only): SHIP at de406cb

  • The diagnosis is correct. cmux NEXT: vertical tabs per workspace or for all workspaces #32127 (a9c5b0d) placed newTab.default and reopenClosedBrowserPanel in .tabStrip (ActionSurfaceCatalog+Menus.swift:17-18). A placement replaces an exemption (ActionSurfaceCatalog.swift:13-16). Both actions target .tab, but .tabStrip maps to .pane (ActionTarget.swift:153, PaneController+Intents.swift:432). The parity rule therefore reported both as gaps.
  • Nothing changes at run time. contentKinds is read only by ActionSurfaceParityTests.swift:76. Menu building, enabling, routing and the surface export do not read it. The routing that already ships is correct: New Tab goes to TabLifecycle.newTabOfPaneKind with the pane target (AppActionContext+Targets.swift:123). Reopen Closed Tab reopens the key window's newest closed group (TabHandlers.swift:95).
  • Exempting the two actions would be worse. An exemption has no effect while a placement exists, so it would mean removing the rows from the tab bar menu.
  • Rules and scope: the namespace rule is fine (an instance property on a public enum with cases). No handler change and no CI change. 2 files, +13/−1.
  • Fleet: ActionSurfaceParityTests passed 5 of 5 in job fd9d5806aa9d (cmux14) and 5 of 5 in job 34296cd10854 (aws-m4pro-3). Both lanes are red overall only because of the known fleet XCTest-stage AddressBook crash (STATUS 7919/7933), which this PR does not change.

Follow-ups (nits):

  • Count contentKinds only for .create placements. Then a tab-object action placed only in the tab bar menu, such as Pin or Close, still shows as a gap.
  • The doc comment at ActionTarget.swift:175-176 says both rows act on the pane's tabs. Reopen Closed Tab reopens the window's newest closed group, whatever the target.
  • The .noObject exemptions for reopenClosedBrowserPanel and palette.openTaskManager (ActionSurfaceCatalog+MenuExemptions.swift:33) do nothing now. This was already true before this PR.

@lawrencecchen
lawrencecchen marked this pull request as ready for review November 3, 2026 07:35
@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Correction to my review receipt above: the lanes were red overall because of the test-bundle run-loop guard, which held the main thread in xctest. It was not the macOS AddressBook store; that was only a symptom. #33023 and #33013 fixed it on feat (ecfbb44). This head is older than that fix, so its lane still shows the XCTest stage red. The suite's own result (passes at this head, fails at the tip) is unchanged. No action is needed on this PR.

@lawrencecchen
lawrencecchen merged commit 1e5ac8a into feat-cmux-next Nov 3, 2026
317 of 354 checks passed
@lawrencecchen
lawrencecchen deleted the base-red-tab-strip-parity branch November 3, 2026 08:03

This branch was previously deployed

1 inactive deployment
ci-routing-agent — de406cb6 Deployed Nov 3, 2026 by lawrencecchen via namespace autofix #13095
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant