Skip to content

Revert "Fix SF Symbol materialization during window layout" - #10584

Merged
austinywang merged 1 commit into
mainfrom
revert-10549-issue-10547-sfsymbol-layout-hang
Aug 22, 2026
Merged

austinywang merged 1 commit into
mainfrom
revert-10549-issue-10547-sfsymbol-layout-hang

Conversation

@austinywang

@austinywang austinywang commented Aug 22, 2026 •

Copy link
Copy Markdown
Contributor

Reverts #10549


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

Reverts the SF Symbol eager materialization and prewarming introduced in #10549 to restore default AppKit/SwiftUI behavior. Previously we materialized symbols into bitmaps and prewarmed them before window layout; now we return to lazy symbol images without prewarming, and use SwiftUI Image(systemName:) in titlebar buttons.

  • RenderableSystemSymbol: removed bitmap materialization and prewarmAppKitImages; configuredAppKitImage now returns a template NSImage copy sized via symbolImageSize and caches it.
  • Titlebar buttons: replaced CmuxSystemSymbolImage with Image(systemName:) configured via .font(.system(size:weight:)).
  • UpdateTitlebarAccessory: dropped the prewarmTitlebarSymbols() call and helper.
  • Tests: removed the assertion that images are eagerly materialized into NSBitmapImageRep.

Written for commit 63649bf. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved system symbol rendering for AppKit layouts, including more reliable sizing and template behavior.
    • Updated title bar icons to use native SwiftUI system images while preserving their appearance.
  • Performance

    • Reduced unnecessary upfront icon processing during application and update accessory startup.

@coderabbitai

coderabbitai Bot commented Aug 22, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b46cd3d2-51b9-4915-9d7e-3bb2e14e73a5

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1b41a and 63649bf.

📒 Files selected for processing (4)
  • Sources/RenderableSystemSymbol.swift
  • Sources/Update/TitlebarCloudVMButton.swift
  • Sources/Update/UpdateTitlebarAccessory.swift
  • cmuxTests/RenderableSystemSymbolTests.swift
💤 Files with no reviewable changes (2)
  • cmuxTests/RenderableSystemSymbolTests.swift
  • Sources/Update/UpdateTitlebarAccessory.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The change removes eager AppKit symbol bitmap materialization and titlebar symbol prewarming. AppKit images now use copied template images with assigned sizes. Three titlebar icons now use SwiftUI system images. The materialization test was removed.

Changes

Symbol rendering and titlebar integration

Layer / File(s) Summary
AppKit image configuration
Sources/RenderableSystemSymbol.swift, cmuxTests/RenderableSystemSymbolTests.swift
configuredAppKitImage now copies the configured image, marks it as a template, and assigns its size without bitmap materialization. The eager-materialization test was removed.
Titlebar symbol lifecycle
Sources/Update/TitlebarCloudVMButton.swift, Sources/Update/UpdateTitlebarAccessory.swift
The new-tab, cloud-menu, and cloud VM icons now use SwiftUI system images. Startup no longer prewarms titlebar symbols.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 63649

This change restores the prior lazy SF Symbol behavior and removes eager prewarming; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: lawrencecchen

🚥 Pre-merge checks | ✅ 23 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description states the revert and summarizes the changes, but it omits the required testing, demo video, review trigger, and checklist sections. Add the required Summary, Testing, Demo Video, Review Trigger, and Checklist sections, including test results and completed checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (23 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies that the pull request reverts the SF Symbol materialization fix, matching the stated objective.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed The diff adds only NSImage handling and SwiftUI Image calls, removes prewarm/materialization helpers, and keeps the controller MainActor-isolated; no listed actor-isolation mistake is introduced.
Cmux Swift Blocking Runtime ✅ Passed The complete diff adds only image copying, sizing, and SwiftUI Image calls; the added-line scan finds no policy-listed blocking or timing primitive, while prewarming is removed.
Cmux Browser Automation Off-Main ✅ Passed The patch changes only SF Symbol, titlebar, and related test files; it contains no browser/WebKit/socket-worker changes, and both rule-scoped automation files are unchanged.
Cmux Expensive Synchronous Load ✅ Passed The diff removes synchronous symbol prewarming and bitmap materialization; it adds no agent-history loader, large-file parse, directory scan, or interactive-path load.
Cmux Cache Substitution Correctness ✅ Passed The diff changes only transient AppKit/SwiftUI symbol rendering and prewarming; it does not replace a fresh persistence, history, undo, or snapshot read with a cache.
Cmux No Hacky Sleeps ✅ Passed The diff changes only Swift source and Swift tests; the rule covers non-Swift runtime code, and the diff adds no sleep, timer, delay, polling, or wait logic.
Cmux Algorithmic Complexity ✅ Passed The diff removes the bounded symbol prewarm nested loop; new SwiftUI icon code adds no collection traversal, and no scalable scan or slower batch algorithm is introduced.
Cmux Swift Concurrency ✅ Passed The diff only removes symbol prewarming/materialization and replaces three views with SwiftUI Image; it adds no Dispatch, Combine, completion-handler, or fire-and-forget Task pattern.
Cmux Swift @Concurrent ✅ Passed The commit changes only synchronous symbol/image code and removes prewarming; the concurrency-token diff adds no async, nonisolated, or @concurrent behavior.
Cmux Swift Package Boundaries ✅ Passed The diff removes AppKit prewarming/materialization and replaces three titlebar icons with SwiftUI images; it adds no independent domain logic requiring a SwiftPM package.
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only four Swift source/test files. It changes no Package.swift, Package.resolved, .gitignore, workflow, or Xcode project package-reference files.
Cmux Swift Logging ✅ Passed The HEAD^..HEAD diff adds no print, debugPrint, dump, NSLog, file/stdout logging, or Logger; the existing cmuxDebugLog remains #if DEBUG, while changes only alter symbol rendering and remove code.
Cmux User-Facing Error Privacy ✅ Passed The production diff only changes SF Symbol rendering and removes prewarming; it adds no user-facing errors, alerts, diagnostics, provider details, credentials, or payloads.
Cmux Full Internationalization ✅ Passed The diff changes SF Symbol rendering and removes prewarming/tests. It adds no user-facing text, localization keys, catalogs, or locale data.
Cmux Swiftui State Layout ✅ Passed The diff only replaces three symbol views with Image(systemName:) and font modifiers; it adds no state, GeometryReader measurement, lazy/list store reference, or render-time state mutation.
Cmux Architecture Rethink ✅ Passed The diff removes prewarming and bitmap timing workarounds; added code only uses native SwiftUI images and font sizing, with no new sleeps, polling, locks, observers, state owners, or lifecycle brid...
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The diff only changes SF Symbol rendering, removes prewarming, and deletes a test. It adds or materially changes no standalone NSWindow, NSPanel, controller, Window, or WindowGroup code.
Cmux Source Artifacts ✅ Passed The diff changes only four existing Swift source/test files, with no artifact-like paths, binaries, logs, caches, or scratch directories added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The HEAD^..HEAD diff adds no DEBUG/test guard, seam-like member, or widened wrapper; it removes prewarming/materialization and changes icons to Image, while existing seams remain unchanged.
Cmux No Ambient Global State ✅ Passed The commit adds no top-level declarations, mutable globals, namespace types, or singletons. It only removes helpers and changes existing image expressions; existing static APIs are touched incident...
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert-10549-issue-10547-sfsymbol-layout-hang

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.

@greptile-apps

greptile-apps Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR reverts eager SF Symbol bitmap materialization and titlebar prewarming, restores direct SwiftUI system images for cloud controls, and removes the corresponding materialization test.

  • Restores lazy AppKit symbol-image handling and removes startup prewarming.
  • Restores direct Image(systemName:) usage for the cloud titlebar controls.
  • Removes the test requiring bitmap-backed AppKit images.

Confidence Score: 4/5

The PR should not merge until lazy SF Symbol materialization is kept out of the titlebar’s main-thread window-layout path.

The reverted helper again returns lazy system-symbol representations, while titlebar startup no longer prewarms them before SwiftUI/AppKit intrinsic-size work, reintroducing a reachable multi-second window-layout stall.

Files Needing Attention: Sources/RenderableSystemSymbol.swift, Sources/Update/UpdateTitlebarAccessory.swift

Important Files Changed

Filename Overview
Sources/RenderableSystemSymbol.swift Reverts bitmap materialization, allowing lazy SF Symbol provider work to return to layout-sensitive AppKit callers.
Sources/Update/UpdateTitlebarAccessory.swift Removes titlebar symbol prewarming before existing windows are attached.
Sources/Update/TitlebarCloudVMButton.swift Restores direct SwiftUI system images for plus, dropdown, and cloud icons without an independently established defect.
cmuxTests/RenderableSystemSymbolTests.swift Removes the regression test that required configured AppKit symbols to be eagerly bitmap-backed.

Sequence Diagram

sequenceDiagram
  participant Window as NSWindow
  participant Accessory as Titlebar accessory
  participant SwiftUI as TitlebarControlsView
  participant Symbol as configuredAppKitImage
  Window->>Accessory: Attach accessory
  Accessory->>Accessory: Invalidate intrinsic content size
  Accessory->>SwiftUI: Evaluate/layout body
  SwiftUI->>Symbol: Request system symbol
  Symbol-->>SwiftUI: Return lazy NSImage representation
  SwiftUI-->>Window: Materialize during layout
Loading

Reviews (1): Last reviewed commit: "Revert "Fix SF Symbol materialization du..." | Re-trigger Greptile

Comment on lines +189 to +191
let image = (configuredImage.copy() as? NSImage) ?? configuredImage
image.isTemplate = true
image.size = symbolImageSize(configuredImage.size, fallbackDimension: rasterSize)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Lazy symbols block titlebar layout

When a titlebar accessory performs its first intrinsic-size pass, configuredAppKitImage returns a lazy SF Symbol representation that is materialized synchronously during main-thread window layout, causing the window and titlebar to hang for several seconds.

@austinywang
austinywang merged commit d482696 into main Aug 22, 2026
11 checks passed
@austinywang austinywang mentioned this pull request Sep 14, 2026
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