Repository navigation
Render panel header glyphs through the resolved-icon path (#8558) - #10272
BorisLoveDev wants to merge 1 commit into
Conversation
…i#8558) The markdown viewer and file preview headers draw their leading file icon and every trailing action glyph through the SwiftUI symbol path. On macOS 15 that path rasterizes fully transparent in this header while the buttons keep their frames and hit areas, so the whole action group is invisible yet still responds to clicks. Verified on a local build: instrumenting the glyphs with a colored background shows all six button frames laid out and empty, and a side-by-side render proves both `Image(systemName:)` and the shared template `NSImage` behind `CmuxSystemSymbolImage` draw nothing there while `CmuxResolvedIconImage` draws the glyph. Forcing an explicit foreground color does not bring the symbols back, so this is a rasterization failure rather than a tint that resolves to the background. Draw the header glyphs through `CmuxResolvedIconRenderer`, which rasterizes into an explicit bitmap context under the resolved appearance, verifies the output has visible pixels, and re-renders when the window or effective appearance changes. The tint becomes an explicit color carried by a new `panelHeaderIconTint` environment value, because the AppKit-backed icon resolves the window appearance rather than the panel's SwiftUI `colorScheme` override; disabled glyphs keep the color at reduced alpha. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughPanel header icons now use appearance-resolved rendering with explicit tint propagation. Header controls remove secondary foreground modifiers. New tests validate tinting, disabled states, glyph sizes, and light/dark rendering. ChangesPanel Header Icon Rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change updates panel header icons to use the resolved rendering path to restore visibility on affected macOS versions; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Thank you for this, @BorisLoveDev! Same story as #9144: the panel header glyph fix landed on main in #12126 and #12145 after you opened this, so this one's covered. Really appreciate it :) |
Summary
PanelFilePathHeader) now draw their leading file icon and every trailing action glyph throughCmuxResolvedIconImageinstead of the SwiftUI symbol path. The glyph tint becomes an explicitNSColorcarried by a newpanelHeaderIconTintenvironment value, so.foregroundColor(.secondary)is dropped fromPanelHeaderIconButton,MarkdownTypographyControl, andFileExternalOpenHeaderMenuButton.Root cause
Reproduced deterministically on macOS 15.7.4 (Apple Silicon), Xcode 16.4. In a markdown panel the whole header action group and the leading
doc.richtexticon render blank, in every state — before and after the app-activation cycle from the report.Three instrumented builds narrowed it down:
PanelHeaderIconGlypha colored background shows all six button frames laid out and empty. The 20×20 frame andcontentShape(Rectangle())survive whether or not a glyph draws, which is exactly the "invisible but clickable" symptom..foregroundStyle(.red)on the glyph changes nothing, so this is not a tint resolving to the background color.CmuxSystemSymbolImage's image resolution. Painting the nil-image fallback branch green shows it is never taken — theNSImageresolves fine. SettingcacheMode = .neverplusrecache()on the cached image does not help either, so a poisoned shared raster is not the cause.A side-by-side render of all three paths in the live header settles it:
Image(systemName:)CmuxSystemSymbolImage(shared templateNSImage)CmuxResolvedIconImageSo the SwiftUI symbol path itself fails to rasterize in this header context, and the AppKit renderer introduced in #7729 does not.
CmuxResolvedIconRendererdraws into an explicit bitmap context underappearance.performAsCurrentDrawingAppearance, verifies the output contains visible pixels, and re-renders on window attachment and effective-appearance changes.The tint has to be explicit rather than a hierarchical style: the AppKit-backed icon resolves the window appearance, not the panel's SwiftUI
colorSchemeoverride, so.secondarywould no longer track the panel theme.PanelFilePathHeaderpublishes the panel's theme foreground at 0.55 alpha; disabled glyphs keep the color at reduced alpha.Relationship to existing work
This is the same surface and the same approach as #9144, which has been open since 29 July and is currently conflicting with
main. That PR's analysis and its choice of the resolved-icon path informed this one; close whichever you prefer. Related reports of the same class: #8352, #7725, #4476.Scope is deliberately limited to the panel header. Other
CmuxSystemSymbolImagecall sites are untouched.Testing
cmuxTests/PanelHeaderIconGlyphTests.swift(Swift Testing, wired into thecmuxTeststarget;scripts/lint-pbxproj-test-wiring.shpasses): request construction (symbol source, size, explicit tint, disabled alpha, fallback tint), theme-tint derivation, and a parameterized pass rendering every header symbol under bothaquaanddarkAqua, asserting the renderer reports visible pixels.xcodebuild test -scheme cmux-unitaborts inswift-frontendwhile deserializing thecmux_DEVmodule (While finishing conformance for protocol conformance GhosttyNSView: TerminalRenderedFrameReceiving→While cross-referencing conformance for 'NSResponder'→abort). I verified this is not caused by this change: the identical crash reproduces on the unmodified base commit, on a cleanderivedDataPath, in a compile batch that contains none of my files. It is Xcode 16.4 / Swift 6.1.2 on my machine; CI pins its own Xcode. Please let CI run the suite.String(localized:)labels, help text, and accessibility labels are untouched, so noResources/Localizable.xcstringsorweb/messages/*.jsonchanges are needed.Note on the base commit
This branch is based on
5734a451crather than currentmainbecausemainhas not compiled since04ff18eea(14 Aug):Sources/Workspace+PanelLifecycle.swift:455passesworkspaceID:toTerminalController.cleanupSurfaceState, which takes(surfaceIds:paneIds:).Sources/TerminalController+MobileSurfaces.swiftreferencespanelArtifactAuthorizationStore, which is not declared anywhere in the repository, plus three non-exhaustive switches over the newMobileSurfaceKindcases.5734a451cis an ancestor ofmain, so this PR's diff is just the one commit. Happy to rebase oncemainbuilds again.Checklist
Fixes #8558.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Render panel header icons through the resolved AppKit path with an explicit tint to fix invisible-but-clickable glyphs on macOS 15. Old: SwiftUI symbol path produced transparent glyphs; new:
CmuxResolvedIconImagedraws visible pixels; side effect: tint now comes from a new environment value.PanelFilePathHeadertoCmuxResolvedIconImage(16×16 lead, 13×13 glyphs) and publishespanelHeaderIconTint..foregroundColor(.secondary)from header buttons; derives tint from the header theme foreground at 0.55 alpha; disabled state keeps color, reduces alpha by 0.45; falls back toNSColor.secondaryLabelColor.PanelHeaderIconGlyphTestscovering request construction, tint derivation, and visible-pixel rendering for all header symbols inaquaanddarkAqua.CmuxSystemSymbolImagecall sites unchanged.Fixes #8558.
Written for commit 40824fb. Summary will update on new commits.
Summary by CodeRabbit
Style
Tests