Skip to content

base red: move the Invite People views and the update surface JSON out of CmuxNextApp (app_lines +198) - #34570

Merged
lawrencecchen merged 3 commits into
feat-cmux-nextfrom
base-red/app-lines-33967
Nov 4, 2026
Merged

lawrencecchen merged 3 commits into
feat-cmux-nextfrom
base-red/app-lines-33967

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The arch ratchet row app_lines is red on feat-cmux-next. At the merged base 0418d016332: [app_lines] CmuxNextApp Swift lines: +198 over the newest green arch tip 323284030a3c. (No first-parent tip after 323284030a3c is green: I ran the ratchet over 323284030a3c at each of the 104 tips.)

This PR has two fix commits. Each moves logic out of CmuxNextApp into a package that owns it. The behavior does not change.

Design surface: chat-multiplayer.md (leo-workspace design/chat-multiplayer.md). This PR does not change the design. It only moves files and changes their paths in .github/design-surfaces.json.

Per-commit culprit table

python3 scripts/cmux-next/arch-ratchet.py --base C^ --head C for each first-parent commit in 323284030a3c..0418d016332 (104 commits). The table shows only the commits that change app_lines. All other 94 commits say "no rise".

Commit PR app_lines This PR
26fd47135b9b #33967 Invite People sheet for a shared chat +361 fixed by 2e70de939c5 (-339)
8a26a6b7623 #33256 cap untouched hidden New Tab pages -65
06f250b0f6c fix(browser): every hidden page keeps its leave picture -53
8aece823dc9 #34405 a slow group collapse save retries quietly -6
4f13a238f24 resize keys move the divider in the arrow's direction -6
b7c9441d175 #34477 the What's New popover fits its lines +9 fixed by 676c963330f (-9)
431286d1831 #34367 a stand-in tab or pane closes on the click +20 already taken back by da508af687c
da508af687c #34530 a stand-in pane's close never leaves its intent behind -25
c0cc4bfaeae Show Chat Minimap and Prompt Rail on Right Edge -31
72eecbaab17 perf(browser): one browser chrome per pane -6
Net +198 -150 at this head

431286d1831 (#34367) added 20 lines to CmuxNextApp/Handlers/PaneHandlers+Verbs.swift. Its follow-up da508af687c (#34530) moved that close logic out of the same handler (to DaemonService.closePane, -25). I do not change it.

The fix

1. 2e70de939c5: the Invite People card and rows move to CmuxNextNotifications (for #33967)

The culprit added CmuxNextApp/Share/AgentShareInviteSheet.swift (301 lines) and CmuxNextApp/Share/AgentShareInviteRowView.swift (59 lines). The model (AgentShareInviteSheetModel), the invite rules (AgentShareInvites) and the strings (AgentShareStrings, AgentShare.xcstrings) are already in CmuxNextDaemonService. Only the views were in the App.

  • AgentShareInviteCard (the AppKit dialog card) moves to CmuxNextNotifications/Share/AgentShareInviteCard.swift.
  • AgentShareInviteRowView moves to CmuxNextNotifications/Share/AgentShareInviteRowView.swift, with no change.
  • AgentShareInviteSheetPresenter stays in CmuxNextApp/Share/AgentShareInviteSheet.swift. It is only the wiring: the overlay host, AppServices, the close icon and the first load().

Why CmuxNextNotifications: it is a ui-layer target (moved out of CmuxNextApp before) that already declares CmuxNextDaemonService (the sheet model) and CmuxNextDesign (the design kit). CmuxNextApp and CmuxNextAppTests already declare it.

CmuxNextNotifications does not declare CmuxNextIcons. Thus the presenter gives the card its close image: NSImage.icon(.actionClose, size: 12), the same image as before.

Access control: the card is package, not public. package members: init(model:closeImage:), onClose, focusField(), and the members that the compiler requires for a package class (the two overrides, the required init?(coder:) and the two NSTextFieldDelegate methods). The row view stays internal.

Tests: AgentShareTests adds @testable import CmuxNextNotifications and gives the card closeImage: NSImage(). No test logic changes.

2. 676c963330f: the update surface's highlights and JSON move to CmuxNextUpdater (for #34477)

b7c9441d175 added the "delivery": "background" patch of the Updated card's JSON to the App's UpdateSurfaceProvider. CmuxNextUpdater owns UpdateHighlights and UpdaterService and already declares CmuxNextSettings (JSONValue). New file CmuxNextUpdater/UpdaterService+SurfaceHighlights.swift:

  • UpdateHighlights.surfaceJSON(background:): encodes the highlights and adds "delivery": "background" when background is true.
  • UpdaterService.surfaceHighlights(updated:): the Ready popover's or the Updated card's highlights (the same rule as before).

The provider keeps only the App facts: the surface kind and AppServices.environment.relaunchBehind.

For both

  • No Package.swift change, no new target, no new dependency edge.
  • Isolation: CmuxNextApp, CmuxNextNotifications and CmuxNextUpdater all use uiSwiftSettings (default isolation MainActor), so the isolation of the moved code does not change.

Takes back #33967: its Invite People views were in CmuxNextApp, which made the app_lines arch ratchet red at the feat-cmux-next tip. This PR keeps all of its behavior and moves only the views.

Takes back #34477: its update surface JSON logic was in CmuxNextApp (+9 App lines). This PR keeps the behavior and moves the logic to CmuxNextUpdater.

Ratchet

At the merged base tip 0418d016332:

arch-ratchet: [app_lines] CmuxNextApp Swift lines: +198 (CmuxNextApp +198)
arch-ratchet: push refused

At this head (python3 scripts/cmux-next/arch-ratchet.py --base 323284030a3c --head HEAD):

arch-ratchet: app_lines -150 (CmuxNextApp -150)
arch-ratchet: no architecture count rises

Per fix commit: 2e70de939c5 app_lines -339, 676c963330f app_lines -9.

Changelog

none

Observed / Not run

Observed:

  • python3 scripts/cmux-next/arch-ratchet.py --base 323284030a3c --head HEAD: "no architecture count rises", app_lines -150.
  • Per-commit ratchet for each first-parent commit in the range: the table above. Ratchet over 323284030a3c at each tip: none is green.
  • python3 scripts/cmux-next/prepush-gate.py: "no new violations".
  • ./scripts/lint-ios-package-conventions.sh: "OK: no unjustified convention violations."
  • python3 scripts/cmux-next/check-package-dependency-lists.py: exit 0.
  • Earlier head 2e70de939c5: fleet compile job 140d71ae66ce21c0d3a96aca passed (230 s); fleet step 6dc184ab5bbb63da4e1f1ec7: AgentShareTests (19 tests) and AgentShareConsentTests (9 tests) passed.
  • Fleet compile cmux-ci compile-check --ref 676c963330fb406085350c110d39df99eae540ff --package CmuxNext: job e05a8b489f5cb07797495f1e, "compile check CmuxNext: passed (374 s)". CI cmux-next/compile passed on the same head.
  • Fleet suites scripts/ci/package-test-lane.sh suite Packages/macOS/CmuxNext AgentShareTests,AgentShareConsentTests,UpdatedCardDismissPathTests,UpdatedCardCurrentBuildNotesTests,LaunchPolishVisibilityTests on 676c963330f: step ea10ac52ebf52290d1ebc959 (aws-m4pro-2), "All 5 suites passed" (19, 9, 2, 2 and 8 tests).

Base findings (not caused by this PR):

Not run:

  • No strings moved, so the xcstrings checks are not necessary and I did not run them.
  • No Swift test covers the new "delivery": "background" field; the move keeps its text. I did not add a test.
  • No live app run and no capture: no UI code or layout changes (No capture: a file move with no UI change).

The arch ratchet row app_lines is red on feat-cmux-next: CmuxNextApp
Swift lines +231 over the last green arch tip 3232840. The cause is
26fd471 (#33967), which added the Invite People sheet views to
CmuxNextApp (+361 lines). Later commits removed 130 App lines.

This change moves the sheet views to CmuxNextNotifications, a ui-layer
target that already declares CmuxNextDaemonService (the sheet model)
and CmuxNextDesign (the design kit), and that CmuxNextAppTests declares:
- AgentShareInviteCard (Share/AgentShareInviteCard.swift)
- AgentShareInviteRowView (Share/AgentShareInviteRowView.swift)

Only the presenter stays in CmuxNextApp, in the old file
Share/AgentShareInviteSheet.swift (AgentShareInviteSheetPresenter:
the overlay host, AppServices, the first load). CmuxNextNotifications
does not declare CmuxNextIcons, so the presenter gives the card its close
icon (the same NSImage.icon(.actionClose, size: 12) as before).

The card is package access: its init, onClose, focusField, and the
members that the compiler requires (the overrides, the required init and
the text field delegate methods). The row view stays internal. No
Package.swift change, no new target and no new dependency edge.

The behavior does not change. AgentShareTests imports
CmuxNextNotifications and gives the card an empty close image.

Takes back #33967: its Invite People views are in CmuxNextApp, which
made the app_lines arch ratchet red at the feat-cmux-next tip.

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

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

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

Review 676c963: 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=pr34570 -f ref=base-red/app-lines-33967 -f platform=macos -f backend_mode=local

Shipped in:

@blacksmith-sh

blacksmith-sh Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Found 5 test failures on Blacksmith runners:

Failures

Test View Logs
main.DeveloperIdAppTests.test_download_prints_the_active_profile_only/
test_download_prints_the_active_profile_only
View Logs
stubs: /System/Library/Frameworks/Security.framework/Versions/A/Security has no stub/
Security has no stub
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_CFRetain']/
CoreFoundation, missing 1: ['_CFRetain']
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_log2']/
CoreFoundation, missing 1: ['_log2']
View Logs
test_the_type_check_covers_every_webviews_test_file (main.WorkflowShape.test_the_ty
pe_check_covers_every_webviews_test_file)/
test_the_type_check_covers_every_webviews_test_file (main.WorkflowShape.test_the_ty
pe_check_covers_every_webviews_test_file)
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 subagent): SHIP at 2e70de9

I found no blocking problem. I did not compile or run tests. I read the diff, the base file at a7072f8788c and the head files, and I ran the static gates.

Blocking

  • None.

What I checked

  1. Behavior is the same. I compared the old card text (base CmuxNextApp/Share/AgentShareInviteSheet.swift, lines 24 to the end) with the new CmuxNextNotifications/Share/AgentShareInviteCard.swift line by line. The only changes are the access words (package), the new closeImage stored property and init parameter, and NSButton(image: closeImage, ...) at line 181. The presenter passes NSImage.icon(.actionClose, size: 12), which is the same call as before. The Observation chain, the generation counter, all cmux.agentShare.invites.* identifiers, the accessibility role, subrole, label and modal flag, focusField(), the three task-owner tasks and the layout are the same text. AgentShareInviteRowView.swift is a pure rename (the blob is identical).
  2. Access control is correct. With InternalImportsByDefault, package import AppKit and package import CmuxNextDaemonService are necessary because NSView, NSImage and AgentShareInviteSheetModel are in package signatures. Foundation types (Notification, Selector) come through the AppKit package import (the same pattern as CmuxNextTerminal/TextBox/TextBoxView.swift:96 and DesktopNotifier.swift:215). The two overrides, the required init?(coder:) and the two NSTextFieldDelegate witnesses must be package for a package class, and they are. The private @objc selectors and the internal static members are in the same module, so they compile. The test uses inviteButton, empty, account, detail, removePressed and AgentShareInviteRowView (all internal) through @testable import CmuxNextNotifications, which other CmuxNextAppTests files also use. roleTitle is in CmuxNextDaemonService, which the test already imports with @testable.
  3. Isolation does not change. CmuxNextApp, CmuxNextNotifications and CmuxNextAppTests all use uiSwiftSettings (default isolation MainActor).
  4. Module boundaries are correct. CmuxNextNotifications already declares CmuxNextDaemonService and CmuxNextDesign. CmuxNextApp and CmuxNextAppTests already declare CmuxNextNotifications. There is no Package.swift change and no new edge. The card uses no Bundle.module strings (all strings come from the model in CmuxNextDaemonService), so no resource moves. I looked at all other non-App targets that declare both CmuxNextDaemonService and CmuxNextDesign (CmuxNextDaemonRecovery, CmuxNextChiefViews, CmuxNextMobileHost). None of them is a better owner.
  5. No stale references. .github/design-surfaces.json lists the two new paths and keeps the presenter path. git grep finds no other reference to the old row path or to the old AgentShareInviteCard(model:) init.
  6. Static gates on a7072f8788c..2e70de939c5: arch-ratchet.py gives app_lines -339 and "no architecture count rises". prepush-gate.py gives "no new violations".

Non-blocking

  • The descriptions of CmuxNextNotifications do not mention the Invite People card: the comment in Packages/macOS/CmuxNext/Package.swift:354-356 and the note in plans/cmux-next/architecture-layers.json:121 say only "notification banners". Add the share card to both notes in a follow-up, so that the next reader can find the owner.
  • The red check cmux-next crash-class rules is not caused by this PR. Its step "Chromium Objective-C contract" fails because scripts/cmux-next/chromium-objc-contract.json is reviewed for cef-154.0.34-cmux.38 and the pinned engine is cef-156.0.15-cmux.43. This PR does not touch these files. It is a base finding.
  • Not run by me: the Swift compile and the AgentShareTests suite. The fleet compile and the fleet suites in the PR description were still running when I read the PR.

lawrencecchen and others added 2 commits November 4, 2026 02:40
…dater

b7c9441 (#34477) added 9 App lines (arch ratchet app_lines +9): the
"delivery": "background" patch of the Updated card's JSON, in the App's
UpdateSurfaceProvider.

This change moves that logic to CmuxNextUpdater, which owns
UpdateHighlights and UpdaterService and already declares
CmuxNextSettings (JSONValue):
- UpdateHighlights.surfaceJSON(background:) encodes the highlights and
  adds "delivery": "background" when background is true.
- UpdaterService.surfaceHighlights(updated:) chooses the highlights of
  the Ready popover or of the Updated card (the same rule as before).

The provider keeps only the App facts: the surface kind and
AppServices.environment.relaunchBehind. Both targets use uiSwiftSettings
(MainActor default), so the isolation does not change. No Package.swift
change. The behavior does not change.

Takes back #34477: its App lines made the app_lines arch ratchet red.

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

Copy link
Copy Markdown
Collaborator

For awareness: #34656 (Invite People: Copy Invite Link, live refresh) is stacked on this move and edits CmuxNextNotifications/Share/AgentShareInviteCard.swift. It holds until this lands, so nothing here needs to change for it.

@lawrencecchen lawrencecchen changed the title base red: move the Invite People card and rows out of CmuxNextApp (app_lines +231) base red: move the Invite People views and the update surface JSON out of CmuxNextApp (app_lines +198) Nov 4, 2026
@github-actions github-actions Bot added risk:high Risk tier (docs/ci/risk-tiers.md) and removed risk:normal Risk tier (docs/ci/risk-tiers.md) labels Nov 4, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent subagent): SHIP at 676c963

I did this review only by reading. I did not compile or run tests.

Blocking findings

None.

Checks

  • 2e70de939c5 after the merge: git diff 0418d016332 6a2040257fd and git diff a7072f8788c 2e70de939c5 are byte-identical. The merge 6a2040257fd has no conflict result that changes code. The earlier SHIP for 2e70de939c5 (comment 6620794100) stays valid.
  • 676c963330f behavior is the same as the old provider at 6a2040257fd:
    • Nil highlights: try highlights?.surfaceJSON(...) ?? .null gives .null. Optional chaining does not evaluate the argument, so relaunchBehind is not read when the highlights are nil. This is the same as the old guard let highlights else { return .null }.
    • Ready or Updated: surfaceHighlights(updated: kind == .updated) returns stagedHighlights when the kind is not .updated, else lastUpdateHighlights ?? updatedWithoutRecordHighlights. This is the same rule.
    • "delivery": "background": it is added only when the kind is .updated, services?.environment.relaunchBehind != nil and the encoded value is an object. This is the same condition. && short-circuits, so relaunchBehind is read only for .updated, as before.
    • Observation: the ObservationStream closure still reads self?.highlights, which reads stagedHighlights, or lastUpdateHighlights and then updatedWithoutRecordHighlights, on the @Observable UpdaterService. These are the same properties. relaunchBehind was not in the tracked closure before and is not in it now.
  • Access control and compile (by reading):
    • public import CmuxNextSettings is necessary because the public surfaceJSON returns JSONValue. Five other files in CmuxNextUpdater already import CmuxNextSettings. Foundation (JSONEncoder) is not in a public signature, so the internal import Foundation is correct.
    • JSONValue: CmuxNextPages has public import CmuxNextSettings in PageProvider.swift. CmuxNextUpdater does not declare CmuxNextDaemon (which has a second JSONValue). Thus both sides use CmuxNextSettings.JSONValue.
    • try x?.f() ?? .null: try on the left covers the full expression. The result type JSONValue selects the T? / T overload of ??. .null is JSONValue.null.
    • services?.updater.surfaceHighlights(...): AppServices.updater is a non-optional UpdaterService. The optional chain flattens to UpdateHighlights?.
  • Isolation: CmuxNextApp and CmuxNextUpdater both use uiSwiftSettings (defaultIsolation(MainActor)). UpdaterService is @MainActor @Observable. JSONValue.parse is in CmuxNextSettings (daemonSwiftSettings, no default isolation), so it is nonisolated and can be called from the main actor. UpdateHighlights is nonisolated public struct ... Codable, so JSONEncoder().encode(self) can be called from the main actor.
  • Module boundaries: CmuxNextUpdater already declares CmuxNextSettings in Package.swift. The commit does not change Package.swift. In architecture-layers.json, CmuxNextUpdater is ui (it mixes service and domain) and CmuxNextSettings is domain. This edge already exists. The new public API is two instance methods in extensions (no caseless enum, no all-static type), so the namespace convention holds. There is no name collision for surfaceHighlights or surfaceJSON.

Non-blocking findings

  1. CmuxNextUpdater/UpdaterService+SurfaceHighlights.swift:15: extension UpdateHighlights does not have nonisolated. With defaultIsolation(MainActor), surfaceJSON(background:) is main-actor isolated, but the struct itself is nonisolated. This does not change behavior: the only caller (UpdateSurfaceProvider) is @MainActor, and the old code also ran on the main actor. If an off-main caller becomes necessary, use nonisolated extension UpdateHighlights.
  2. No Swift test covers "delivery": "background" or surfaceHighlights(updated:). The PR body says this in "Not run". A small CmuxNextUpdaterTests case for surfaceJSON(background: true/false) costs little and keeps the moved rule safe.
  3. Compile evidence: at this review, the fleet compile and the CI compile for 676c963330f are still pending. risk:high needs a green compile on this exact head before it lands. cmux-next crash-class rules is red. The PR body says it is a base finding (it is also red on the unrelated cmux NEXT: Hide the Tab of a Chat Alone in Its Pane, an opt-in setting (carries #34554 onto base) #34609). I did not check this independently.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Base findings at head 676c963 (not caused by this PR):

@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 4, 2026
@lawrencecchen
lawrencecchen merged commit c3d6334 into feat-cmux-next Nov 4, 2026
193 of 203 checks passed
@lawrencecchen
lawrencecchen deleted the base-red/app-lines-33967 branch November 4, 2026 03:27

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 676c9633 Deployed Nov 4, 2026 by lawrencecchen via wait for same-key tree publisher #108961
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:high Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants