Skip to content

Let the Cloud toolbar name the machine-list failure it has - #15236

Merged
teamleaderleo merged 5 commits into
manaflow-ai:mainfrom
teamleaderleo:cloud-sidebar-toolbar-status
Sep 28, 2026
Merged

teamleaderleo merged 5 commits into
manaflow-ai:mainfrom
teamleaderleo:cloud-sidebar-toolbar-status

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

When a machine-list read fails while cached machines stay on screen, the Cloud toolbar was the only one of the three status surfaces that did not say what went wrong. MachinesListStatusToolbarRow matched .failed without looking at the reason and drew one warning triangle plus "Machine list unavailable — showing last known", with nothing to click. So a user whose plan lapsed (HTTP 402) and a user whose session was rejected (HTTP 401) both read the same sentence, and neither was offered the thing that would fix it. The notice and the empty state have routed all three reasons correctly for a while: .unreachable to Retry, .sessionRejected to a fresh sign-in, .requiresPro to an upgrade. The file's own doc comment says all three surfaces read one presentation "so they never disagree"; the toolbar row was the one that did not read it.

This is audit finding A5 from https://github.com/manaflow-ai/cmuxterm-hq/issues/853.

The row now reads MachineListStatusPresentation:

  • the glyph comes from presentation.symbolName, so a rejected session gets the account badge and a lapsed plan gets the Pro sparkle instead of a generic triangle
  • the text comes from a new staleTitle, the one-line form for a toolbar that still has cached rows under it. It sits next to the full title it has to agree with, rather than being a separate literal in the view
  • the action comes from presentation.action and runs through the same performListStatusAction the notice and the empty state use, so the toolbar is no longer a dead end
  • Action.shortTitle exists because the toolbar has one line beside the status text. "Sign Out & Sign In Again" becomes "Sign In" there; the notice and the empty state keep the full titles

Orange tint, the raw error on hover, the copy-error menu and the dismiss button stay bound to failures only, as before.

The two new stale strings use a comma rather than an em dash, which also keeps them parseable by scripts/localize-changes.

Testing

Added cmuxTests/MachinesListStatusToolbarRowTests (5 tests). They mount the production row in an NSHostingView with accessibility enabled and read the accessibility tree, which is what the row actually exposes:

  • each of the three failures exposes its own action button identifier
  • the three failure lines are pairwise different
  • every failure still says "last known", so the qualifier was not lost
  • .waitingForNetwork offers no action and still reads "Offline"
  • pressing the upgrade button reaches the handler with .upgrade

Red then green, same focused command both times:

SHA What Run
f3278d4 tests + the perform parameter, body unchanged https://github.com/manaflow-ai/cmux/actions/runs/36397551599
91f128b the fix https://github.com/manaflow-ai/cmux/actions/runs/36397834894

Results are pending as of this description; I will post the red and green outcomes as a comment.

Not run locally: this machine does not build cmux, so there is no local scoped verification for the Swift changes. python3 scripts/localization_catalog.py check did run: 9 catalogs, 9 locales, 0 parity errors.

Localization audit: two new keys (machines.sessionRejected.stale, machines.requiresPro.stale) and two new short action titles (machines.sessionRejected.signInAgain.short, machines.requiresPro.upgrade.short), all four with entries for every one of the nine macOS locales, prepared through ./scripts/localize-changes and imported by the catalog tool. machines.unavailable.retry needed no short form, so shortTitle returns title for .retry rather than adding a duplicate key. No web strings changed.

Changelog

Fixed: When the Cloud machine list fails to load and cached machines stay on screen, the toolbar now says whether the sign-in or the plan is the problem and offers the matching fix, instead of showing one generic "unavailable" message

Demo Video

Frames for the Cloud sidebar surfaces are being captured in CI for the audit; the three toolbar failure states are not reachable in a signed-out CI run, so this PR leans on the accessibility-tree assertions above rather than a screenshot.

Checklist

  • Behavior changes have added or updated tests, or Testing says why not
  • UI, settings, menu, schema, help-text or user-facing docs change: localization audited, and the result is stated above
  • Reviewed with a subagent before merge

🤖 Generated with Claude Code


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


Summary by cubic

The Cloud toolbar now names the specific machine-list failure it hit and offers the fix for it instead of showing one generic "unavailable" message with nothing to click. A rejected session gets a Sign In link, a lapsed plan an Upgrade link, and an unreachable read keeps Retry, routed through the same handler the notice and empty state use.

  • Adds staleTitle and shortTitle to MachineListStatusPresentation so the toolbar's one-line copy stays beside the full copy it has to agree with.
  • Failure-only affordances (hover error, copy menu, dismiss) no longer leak onto the waiting-for-network and reconnecting states, which previously inherited an empty context menu that suppressed the header's.
  • Adds accessibility-tree tests covering each failure's action, distinct text, and the shared handler path; assertions now read the presentation and catalog rather than English literals.

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

Review in cubic

teamleaderleo and others added 2 commits September 28, 2026 01:26
With cached machines still on screen, the toolbar row renders one warning
triangle and one "Machine list unavailable" line for all three failures and
offers nothing to click. The notice and the empty state already route
.unreachable to Retry, .sessionRejected to a fresh sign-in and .requiresPro
to an upgrade, from the same MachineListStatusPresentation.

This commit adds the failing coverage and the `perform` parameter the row
needs to reach the handler. The body still hardcodes the triangle and the
generic string, so the new tests are red.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MachinesListStatusToolbarRow now reads MachineListStatusPresentation the way
the notice and the empty state already do, instead of hardcoding one warning
triangle and one "Machine list unavailable" line. A rejected session shows the
account glyph and a Sign In link; a lapsed plan shows the Pro glyph and an
Upgrade link; an unreachable read keeps Retry. Both go through the same
performListStatusAction the other two surfaces use.

The presentation gains `staleTitle`, the one-line form for the toolbar, so the
shorter copy still lives beside the full copy it has to agree with, and Action
gains `shortTitle` because the toolbar has one line beside the status text.

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

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 15 seconds.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 8cdfb759-01a8-4e65-a41d-31d310163f4e

📥 Commits

Reviewing files that changed from the base of the PR and between d2877b2 and a87e1f9.

📒 Files selected for processing (6)
  • Resources/Localizable.xcstrings
  • Sources/Cloud/MachinesCloudStatus.swift
  • Sources/Cloud/MachinesListStatusViews.swift
  • Sources/Cloud/MachinesPanelView.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/MachinesListStatusToolbarRowTests.swift

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

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

… states, and make the tests discriminate

Review follow-up.

The row applied `.help(failure ?? "")` and `.cloudErrorCopyMenu(failure)`
unconditionally, so waiting and reconnecting got an empty tooltip and,
worse, an empty `.contextMenu {}`, which on macOS suppresses whatever menu
that region would otherwise inherit. Base applied neither to those
states. Both now hang off the failure branch. The action button also
takes `.fixedSize()`: in a narrow sidebar, truncating the status line is
survivable, losing the only affordance that fixes the failure is not.

Three test problems, all of which let a regression through:

- Nothing covered `failure = presentation.isFailure ? error : nil`, the
  one genuinely new branch. Simplifying it to `let failure = error` kept
  every test green while offline gained an orange dismissable chip. The
  offline case now asserts no `CloudBannerDismissButton` for waiting or
  reconnecting, and one for each failure.
- "The three failures do not read the same line" compared the three
  rendered strings pairwise, but each row already carries a distinct glyph
  and a distinct button, so three identical sentences would have passed.
  Each row is now matched against its own `staleTitle`, the three stale
  lines are checked to be distinct, and the panel's paragraph is checked
  not to leak into the toolbar's single line. That subsumes the separate
  stale-qualifier test, which is gone.
- Assertions used the English literals "last known" and "Offline", which
  a copy edit or a non-`en` host would have reddened for no behavioral
  reason. They read the presentation and the catalog now.

Also removed `.environment(\.accessibilityEnabled, true)` from the test
host and the comment claiming it switched SwiftUI's accessibility output
on. It does not: the key is a read-only signal for app code, and it is
deprecated. The window is what populates the tree, which the comment now
says.

## Changelog

Fixed: the Cloud sidebar toolbar no longer suppresses the header's context menu while waiting for the network or reconnecting.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review

Review subagent, correctness first. It cleared the view body (valid @ViewBuilder, the dismiss button lands in the same TupleView position base put it in, gating is equivalent) and confirmed no existing test asserts on any of this copy. Nine findings; five fixed in a3c75896ee6, four left with reasons.

Fixed

  • .help(failure ?? "") and .cloudErrorCopyMenu(failure) applied to the non-failure states, which base never did. cloudErrorCopyMenu(nil) attaches an empty .contextMenu {}, and on macOS that suppresses whatever menu the header region would otherwise inherit. So waiting and reconnecting silently lost a context menu. Both modifiers hang off the failure branch now.
  • The one genuinely new branch was untested. Nothing covered failure = presentation.isFailure ? error : nil. Simplifying it to let failure = error left all five tests green while offline gained an orange dismissable chip. The offline test now asserts no CloudBannerDismissButton for waiting or reconnecting, and one for each failure.
  • "The three failures do not read the same line" did not discriminate. Each row already carries a distinct glyph and a distinct button, so three identical sentences would have compared unequal and passed. Each row is now matched against its own staleTitle, the stale lines are checked to be distinct, and the panel's paragraph is checked not to leak into the toolbar's one line. That subsumes the separate stale-qualifier test, which is gone, so the suite is four tests.
  • Hardcoded English literals ("last known", "Offline") against repo convention; a copy edit or a non-en host would have reddened them for no behavioral reason. They read the presentation and the catalog now.
  • .environment(\.accessibilityEnabled, true) in the test host did nothing, and its comment said it switched SwiftUI's accessibility output on. It does not: the key is a read-only signal for app code, and it is deprecated. Removed, and the comment now credits the window, which is what actually populates the tree.
  • Narrow-sidebar layout: the action button takes .fixedSize(). Truncating the status line in a narrow sidebar is survivable; losing the only affordance that fixes the failure is not. I did not add the notice's Spacer(minLength: 4): that would make the row greedy inside the team-picker header, which is a layout change base did not make and which I have no dogfood for yet.

Left

  • The CI evidence was the biggest finding and it was right. Run 36397834894 for 91f128b3fc4 sat queued for over half an hour, so at the time of review the body rewrite had never been compiled or executed anywhere. Redispatched at a3c75896ee6; that result gets posted here, and a skipped lane is not coverage.
  • upgradeActionFires rests on accessibilityPerformPress against a .link-styled button, which no other test in the repo does. The redispatched run settles it. I softened the #require message, which claimed more than press can check: it only reports that a press selector exists, and the performed.actions == [.upgrade] assertion is what proves the press reached the handler.
  • signOutForFreshSignIn silently returns when accountFlow is nil, so the Sign In affordance can do nothing. Pre-existing and shared by the notice and the empty state; it is a separate fix, not something to bolt onto this one.
  • The test windows are never closed. About fifteen zero-styled offscreen windows for the life of the test process. A deinit that closes them has to hop to the main actor or risk trapping, and that is more machinery than the leak is worth.

Disclosure: python3 scripts/verify-local.py could not be run in this environment. python3 scripts/sync_test_wiring.py --check passes (1128 test files). Localization: the four new keys are in Localizable.xcstrings across all nine locales, all translated; this commit adds no new key.

Fleet dogfood requested for the three failure states at minimum sidebar width, since a signed-out CI run cannot reach them.

… through

Run 36401958401 failed every test in this suite with nil elements and
empty text. The previous commit removed `.environment(\.accessibilityEnabled,
true)` on the claim that it did nothing and a window was the real
requirement. The window is not sufficient: the same assertions passed in
run 36397834894 with the modifier and no window, and fail with a window
and no modifier. In-process there is no assistive client to switch
SwiftUI's accessibility output on, so the hierarchy has to ask for it.

The window is kept, since querying a detached hosting view is worth
avoiding on its own, and the comment now records which of the two is
load-bearing so it is not removed again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

CI receipt, and a correction to my review comment above.

Correction. In the review I claimed .environment(\.accessibilityEnabled, true) in the test harness "did nothing" and that a window was the actual requirement, and I removed it in a3c75896ee6. That was wrong and CI caught it:

  • run 36397834894 at 91f128b3fc4 (modifier, no window): cmuxTests/MachinesListStatusToolbarRowTests: 5 test(s) executed, all passed.
  • run 36401958401 at a3c75896ee6 (window, no modifier): all 4 tests failed with 11 issues, every element lookup nil and every extracted string empty.

So the modifier is what publishes SwiftUI's accessibility tree in process; the window is not a substitute for it. 25c3793840b restores it, keeps the window anyway, and records in the comment which of the two is load-bearing along with both run numbers, so the next person does not delete it on the same reasoning I did. Re-dispatched at that SHA.

Red/green for the behavior, from the same focused command:

  • red: f3278d43247 (regression only) — the toolbar rendered one line and one glyph for all three failures with nothing to click.
  • green: run 36397834894 at 91f128b3fc4 — 5/5 executed and passed.

The tip commit's test rewrite is in flight; I will post its result rather than claim it.

python3 scripts/verify-local.py is blocked in this session's sandbox, so CI is the only check that has run on this branch.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

CI receipt for the accessibility-environment fix.

  • Red, run 36401958401 at 8fb16647b0c: all four MachinesListStatusToolbarRowTests failed. That commit was my own mistake — I had dropped .environment(\.accessibilityEnabled, true) from the harness, so the rows under test never took the accessibility path the assertions read.
  • Green, run 36405621370 at 25c3793840b: cmuxTests/MachinesListStatusToolbarRowTests passes, same focused filter, same runner.

Same command both times: python3 scripts/ci/dispatch-focused-test.py cmuxTests/MachinesListStatusToolbarRowTests --ref <sha>.

python3 scripts/verify-local.py is denied by this session's permission classifier, so nothing here was verified locally; both results above are CI.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Subagent review at a3c7589: nine findings, five fixed in a3c7589, four left with reasons (comment above). The one regression from that fix (dropped accessibility environment in the test harness) was restored in 25c3793 and is green in CI. Landing: resolving the main conflict, merging main after #15414, then auto-merge.

@cursor

cursor Bot commented Sep 28, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@teamleaderleo
teamleaderleo enabled auto-merge (squash) September 28, 2026 17:09
@teamleaderleo
teamleaderleo merged commit 97491a7 into manaflow-ai:main Sep 28, 2026
61 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for a87e1f95a4: every check was green at merge (16 verified; 17 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 28, 2026
762c3ed Recover a Cloud machine graph stuck on an equal-cursor conflict (manaflow-ai#15328)
524ebff ci: replay the fuzz regressions on sidebar, split and window changes (manaflow-ai#15412)
818d475 Let a user's Cloud open dial even right after a background link failure (manaflow-ai#15291)
97491a7 Let the Cloud toolbar name the machine-list failure it has (manaflow-ai#15236)
0abac32 PR media: adopt CI's build only, start when CI completes, run for every app PR (manaflow-ai#15418)
7f08715 ci(seed): keep the trusted seed on the Mac before the R2 upload (manaflow-ai#15411)
5663c13 Finish the destroy work where a Cloud machine is first found gone (manaflow-ai#15359)

# Conflicts:
#	.github/workflows/ci.yml
#	.github/workflows/pr-media.yml
#	.github/workflows/seed-derived-data.yml
#	.github/workflows/test-e2e.yml
austinywang added a commit that referenced this pull request Sep 28, 2026
Main moved each Cloud row's tooltip and accessibility label into
CloudTreeRowToolTip (#15326), so the Cloud Machines header's plan help
and its "Cloud Machines, 1 of 50 machines" label move there too.
MachinesCloudStatus keeps this branch's collapse-when-empty layout and
takes main's performListStatusAction (#15236).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant