Repository navigation
arch: active-look glass scope, batch-close set and kill-switch reader become instances - #28266
Conversation
… become instances Lowers the arch-ratchet rows these sites raised with no reviewed exception (audit-rh, cmux-ad); no behavior change. - CmuxNextDesign/GlassPanelView.swift: the seven ActiveLookScope statics (windows, pending, candidate, materialSite, installed, counts, kinds; 1335e10, #25704) are one ActiveLookScope instance. The AppKit getter hook that its first ActiveLookGlassEffectView installs captures it. AppServices makes it once and hands it to the app's ThemeStore (ThemeStore.activeLook). The main window gets it through WindowRootView(activeLook:), every window-kit window through NSWindow.install (scope.store.activeLook) and WindowSurfaceView, and the backdrop through WindowMaterialView(activeLook:) and GlassPanelView(activeLook:). The hook itself is unchanged. - CmuxNextAppCore/WorkspaceSelectionCloseFlow.swift: the static running set (a439d29, #24341) is WorkspaceSelectionCloseFlow.Running, one per app (AppServices.selectionCloses), passed to every batch close and read by Close Workspace. It stays app-wide, as the static was. - CmuxNextUpdater/KillSwitchReader.swift: the static weak app reader and start(updater:) (52c918f, #24212) are gone; UpdaterService stops its previous reader through its own killSwitchReader property. - CmuxNextApp/MainMenu+Placements.swift: removes the unused helpGroups, groupedItems and otherItems (dead since #24615), which pays for the AppServices wiring (app_lines -24). Tests construct their own ActiveLookScope (InactiveWindowTintTests), and one more check proves a backdrop with no scope keeps plain glass. Not compiled or run here (no Swift on the agent VM); fleet compile-check of the pushed head follows.
|
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)
red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: Shipped in:
|
|
Found 27 test failures on Blacksmith runners: Failures
...and 17 more test failures. View all on Blacksmith
|
…re slot; one getter hook per process Review of 723ce04 (FIX): ThemeStore.activeLook put the one ActiveLookScope in a public settable slot on ThemeScope.app.store, a static by another name, read by six window owners through NSWindow.install. - ThemeStore.activeLook is removed. AppServices holds the scope (AppServices.activeLook) and passes it down. - NSWindow.install(kind:content:scope:activeLook:) takes the scope as a parameter with no default, and every caller passes it: - the main window (WindowController), from AppServices; - browser popups (BrowserPopupWindows, from AppServices) and sign-in panels (WebAuthSessionWindows from AppDelegate, then WebAuthSessionWindow, then BrowserPopupWindow); - the onboarding tool window (OnboardingService, then OnboardingWindowPresenter, then OnboardingWindowController); - Debug Settings (DebugSettingsService, then DebugSettingsWindowController); - DevTools and the page info windows, which are child windows: they pass their owner window's NSWindow.backdropActiveLook, which reads the owner's own WindowMaterialView (as they already take the owner's themeScope); - BrowserDebugWindow passes nil: it has no entry point. - ActiveLookScope.install adds a marker method to NSWindow with class_addMethod. Only the first install can add it, so the getter hook is installed once per process and a later scope (a test's) never stacks a second hook over the first. A stacked hook could push the reader past the 16-frame forcingDepth. - Tests pass activeLook: nil where they built these windows. Not compiled here (no Swift on the agent VM); the fleet compile-check of the pushed head follows.
Default changesThis pull request changes 4 defaults at
Needs a review. It passes with an independent review receipt on this commit: a comment or review whose first line is Merge receipt: merged as Regenerate the snapshot with |
|
Review (independent review subagent, arch-rise): SHIP at d535691 Summary:
Not blocking: |
…der, not the dead Help groups #28266 (fix/arch-rise-glass-close) already removes MainMenu.helpGroups/groupedItems/otherItems for its own lines; the second PR to land would lose its payment. MainMenu+Placements.swift is back to the base. PageFactory.homeChannelsEditRoute had no production caller (the page builds its PageEditProvider route inline); HomeChannelsComposerEditTests now builds that route the way the page does.
…composition root, never the global NSApp (#28321) * test(app): a key-window lookup in a test process with no NSApplication must not trap AppServices' default key-window source implicitly unwraps the global NSApp, which is nil in a test process that never made NSApplication.shared; the trap ends the whole CmuxNextAppTests process. Red on the base: the suite runs alone (swift test --filter), NSApp is nil, and the first lookup traps at AppServices.swift:87. * fix(app): AppServices takes its key-window source from the composition root, never the global NSApp AppServices.keyWindowSource defaulted to { NSApp.keyWindow }. NSApp is NSApplication!, nil in a test process that never made NSApplication.shared, so any test that built AppServices and reached a key-window lookup (windows.active, keyWindowRole, the quit sheet, ListHandlers) trapped and ended the whole CmuxNextAppTests process (#28275, #28090 green jobs). - AppServices.init takes keyWindowSource with no default; AppDelegate (the composition root) passes { NSApp.keyWindow }, so production cannot forget it. - The test target alone has the convenience init with no key window ({ nil }); no test call site changes. - QuitCoordinator asks the sender (the NSApplication AppKit passed) for isActive and activate(), and reads NSApp?.mainWindow for the sheet; the auto-archive toast reads NSApp?.mainWindow: no implicit unwrap of the global on a path a test can reach. Regression test: AppServicesKeyWindowSourceTests (red on the base: traps at AppServices.swift:87). * fix(app tests): import LaunchReveal for the test-target AppServices init Compile job 26f2055725867054aa419409 failed: AppServices+TestKeyWindow.swift did not see LaunchReveal (CmuxNextDesign), so the convenience init did not compile and every test call site fell to the designated init (missing keyWindowSource). Every other error in that log follows from this one. * fix(app): AppServices takes whether the app is active from the composition root too The second trap (#28275 green eb20257db2553e2e4a4dd378, NotificationCenterService+ProgramStatusSeen.swift:57): AppServices.init starts notifications, whose viewed-program-status observation calls viewedTabs, which read NSApp.isActive. Any test that built AppServices and then awaited (each quit step awaits) trapped. - AppServices.init takes appActiveSource with no default; AppDelegate passes { NSApp.isActive }. - The notifications' six reads (viewedTabs, isViewed, the program-status visibility, arrive, the hand-off and sandbox-run banners) ask services.appActiveSource(). - The test target's init reads the application only if the test made one (NSApp?.keyWindow, NSApp?.isActive ?? false), so tests behave as before and never unwrap the global. - AppServicesKeyWindowSourceTests: the recording test also checks the app-active lookups and awaits the started observation's first ask of the injected source (no clock). * fix(app): the feed presence, Chief pet, new window focus and New Tab spares read the injected sources Review of #28321 (SHIP with gaps at 3980462): - FeedService read NSApp.isActive in observePresence and startIfSignedIn; its sign-in task starts with AppServices, so a signed-in test with no NSApplication trapped. FeedService takes appActive (no default); AppServices passes its appActiveSource. - Re-scan of what AppServices builds and starts: ChiefPetRoutes' appIsActive (made from the notifications service), WindowController's first .appActive focus event and NewTabSparePool's key-window reads (started after the first frame) now ask services.appActiveSource() / keyWindowSource(). - appActiveSource has its own doc; keyWindowSource's doc says debug.key replaces it alone, with the reason. - app_lines: the three joined doc comments are split again; paid by removing dead code instead: MainMenu.helpGroups, groupedItems and otherItems have no caller since #24615 (2e71847). - AppServicesKeyWindowSourceTests: the first test checks only that the lookups return (a GUI lane can have a real key window); it also calls the notifications' viewedTabs and isViewed. * fix(app): pay app_lines with the Channels page's test-only route builder, not the dead Help groups #28266 (fix/arch-rise-glass-close) already removes MainMenu.helpGroups/groupedItems/otherItems for its own lines; the second PR to land would lose its payment. MainMenu+Placements.swift is back to the base. PageFactory.homeChannelsEditRoute had no production caller (the page builds its PageEditProvider route inline); HomeChannelsComposerEditTests now builds that route the way the page does. * fix(app): the input invariant check reads the injected key window; one Channels edit route builder Delta review of #28321 at 923b306 (FIX): - InputObservationBuilder.keyWindow read NSApp.keyWindow; AppServices starts input verification, whose presentation-change subscription runs it (DEBUG invariant monitor, window geometry). It now takes the key window from services.keyWindowSource(). - Re-scan of subscriptions, timers and tasks AppServices starts: KeyRouter's menu gate (keyWindowFocus), the refusal HUD's window (AppActionContext, ChatsOpenCoordinator, HomeChiefActions, LinkHandlers, ViewerService) ask services.keyWindowSource(); the agent cursor snapshot, a pane's surface diagnostics (SurfaceInvariantMonitor) and SheetDismissal.endAll's default read NSApp? (never the unwrapped global). - HomeChannelsPage: PageFactory.homeChannelsEditRoute() builds the edit route and gives back its provider; homeChannelsWebPage uses it and sets run once its view exists, and HomeChannelsComposerEditTests calls it. - app_lines: paid by removing FilePageService.openEmpty, which has no caller.
…(build skew of #28266 and #27638) #28266 turned GlassPanelView(keepsActiveLook:) into GlassPanelView(activeLook:) while #27638 added Glass.makePanel/makeOverlayPanel(keepsActiveLook:) for the dialog card. Both merged, and feat-cmux-next stopped compiling (Glass.swift:79 extra argument 'keepsActiveLook'). Glass.makePanel, Glass.makeOverlayPanel and OverlaySurfaceView now take activeLook: ActiveLookScope?. CmuxDialogView takes it from CmuxDialogCenter, which passes the dialog window's backdropActiveLook. An app-scoped dialog (no window) follows the key state. Compiled: 9b4cea495b0bc379ed0794d1
|
Base red from this PR: the feat-cmux-next "Crash safety" step (check-crash-safety.sh) is red since 346c50a. The new |
…tor waiver (#28266) (#28783) #28266 (346c50a) added NSSelectorFromString("cmux_activeLookScopeInstalled") in CmuxNextDesign/GlassPanelView.swift with a reviewed crash-allow (our own marker selector, added with class_addMethod and never called), but did not record the waiver: CmuxNextDesign dynamic_dispatch.waived 5 -> 6 turned "Crash safety" red on every feat-cmux-next push since 346c50a. crash_ratchet.py --record-waivers writes the row (and lowers the rows that went down: acpmux and cmux-tui-core unwrap). No source change. check-crash-safety.sh: red at 18757c2 (base), ok at this commit.
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
Three groups of global state that rose on
feat-cmux-nextwith no reviewed exception become instances made once at the composition root and passed throughinit. Two small behavior changes (below, "Behavior changes").Sites fixed
Packages/macOS/CmuxNext/Sources/CmuxNextDesign/GlassPanelView.swift:289, :291, :293, :295, :296, :395, :426: the sevenActiveLookScopestatics (windows,pending,candidate,materialSite,installed,counts,kinds; 1335e10, cmux-next: no window background change when switching apps #25704). They are now oneActiveLookScopeinstance:ActiveLookGlassEffectViewinstalls the hook.AppServicesmakes the instance once (AppServices.activeLook). It is in no store and no global; review FIX at 723ce04 found the earlierThemeStore.activeLookslot to be a static by another name, and d535691 removes it.WindowController->WindowRootView(activeLook:). Every window-kit window gets it as a required parameter ofNSWindow.install(kind:content:scope:activeLook:)and thenWindowSurfaceView: browser popups (AppServices -> BrowserPopupWindows -> BrowserPopupWindow), sign-in panels (AppDelegate -> WebAuthSessionWindows -> WebAuthSessionWindow -> BrowserPopupWindow), onboarding (OnboardingService -> OnboardingWindowPresenter -> OnboardingWindowController) and Debug Settings (DebugSettingsService -> DebugSettingsWindowController).NSWindow.backdropActiveLook, the owner's ownWindowMaterialView.activeLook), the same way they already take the owner'sthemeScope. With no owner window they get plain glass.BrowserDebugWindowpasses nil: its init is private and nothing outside its file refers to it (dead code).NSWindow(class_addMethod), and only that install hooks the getter. It replaces the base'sprivate static var installed. The ratchet does not count a method marker, so the -7 here does not mean that all process-wide state is gone: this guard remains, by design. In the app the first and only installing scope is AppServices' (ActiveLookScope()is made only atAppServices.swift:52).WindowMaterialView(activeLook:)and thenGlassPanelView(activeLook:), which replaceskeepsActiveLook: Bool.Packages/macOS/CmuxNext/Sources/CmuxNextAppCore/WorkspaceSelectionCloseFlow.swift:25public private(set) static var running(a439d29, Close a sidebar selection as one batch: one pass, one question, one undo #24341): this is nowWorkspaceSelectionCloseFlow.Running. The app has one (AppServices.selectionCloses). It goes to every batch close and Close Workspace reads it. It stays app-wide, as the static was (see the reviewer notes).Packages/macOS/CmuxNext/Sources/CmuxNextUpdater/KillSwitchReader.swift:33static weak var appandstart(updater:)(52c918f, cmux NEXT: per-PR kill switches (registry, expiry lint, signed Swift reader) #24212): removed.UpdaterService.startKillSwitchReader()stops the previous reader through its ownkillSwitchReaderproperty.MainMenu+Placements.swift. It loseshelpGroups,groupedItemsandotherItems, which nothing has used since feat(cmux-next): trim native app menu bar #24615.CmuxNextCompat/KillSwitches.swiftstatic let sharedstays as it is (an exception candidate).Ratchet delta
python3 scripts/cmux-next/arch_counts.py --delta origin/feat-cmux-next HEAD:No other row changes.
arch-ratchet.py --base origin/feat-cmux-next --head HEAD: "no architecture count rises".arch-ratchet.jsonis not changed (no--update, no exception).Behavior changes
.apptheme in the same case.WindowMaterialViewmade with no scope now gives plain glass. All production backdrops that can show glass pass the scope.Testing
InactiveWindowTintTestsnow makes its ownActiveLookScopefor the attach, detach, scope and backdrop checks. One new check proves that aWindowMaterialViewwith no scope keeps plain glass.python3 scripts/cmux-next/prepush-gate.py: no new violations../scripts/lint-ios-package-conventions.sh --namespace-fix: no violations.Observed / Not run
Reviewer notes
activeLook,WindowMaterialView(frame:)now makes plain glass. The production backdrops that can show glass (the main window root andWindowSurfaceView) pass the scope. The pane art view is always.translucent(PaneBackdropPlan), so it never makes glass.InactiveWindowTintTestscheck scope state directly).WindowRootView(sidebar:activeLook:)keeps a nil default for 15 test call sites; its one production caller passes the scope.Runningis app-wide, not per window (the audit proposal said per window). The old static gave dedupe across windows. Also,WorkspaceSelectionCloseand Close Workspace can both run with no controller or owner window.NSWindow._hasActiveAppearanceIgnoringKeyFocuscalls AppKit's getter. Once the scope is calibrated, a read that answers inactive in a window that holds backdrop glass takes a 16-framebacktrace()plus cacheddladdrimage lookups. Every other read is oneNSMapTablelookup. While the scope calibrates, a read inside that window's glass update takes a 32-framebacktrace(). Off the main thread it does nothing.NSGlassEffectViewhas no public active-state API. A candidate public route that nobody has checked: draw the backdrop with SwiftUIglassEffectin our own hosting view, with\.appearsActive/\.controlActiveStateforced in its environment. Owner: Performance Lead.Changelog
none