Repository navigation
Disable titlebar page help tooltips - #1038
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA static method is added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/WorkspaceContentViewVisibilityTests.swift`:
- Around line 93-100: The test currently only asserts the policy method
ContentView.titlebarPageControlsShouldInstallHelpTooltips() returns false;
instead, render a titlebar control at runtime (via a small test seam or
test-only factory such as adding a TestTitlebarControl or a
ContentView.makeTitlebarControlForTesting() that produces the actual control
used in the titlebar) and assert no AppKit help tooltip gets registered on the
rendered view (e.g., verify the view and its subviews have no toolTip/helpTag
set and that addToolTip/removeToolTip were not applied) so the test checks
observable behavior rather than the policy return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 920fd5c7-738c-4249-b903-fff63bf025f9
📒 Files selected for processing (2)
Sources/ContentView.swiftcmuxTests/WorkspaceContentViewVisibilityTests.swift
| final class TitlebarPageTooltipPolicyTests: XCTestCase { | ||
| func testTitlebarPageControlsNeverInstallAppKitHelpTooltips() { | ||
| XCTAssertFalse( | ||
| ContentView.titlebarPageControlsShouldInstallHelpTooltips(), | ||
| "Transient titlebar page controls must not register AppKit help tooltips." | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This guards the helper, not the tooltip behavior.
Line 96 only asserts that the policy method currently returns false, so this still passes if a future titlebar control forgets to honor that policy and installs .help(...) anyway. Please test through a small runtime seam closer to the rendered control/configuration so the assertion covers “no AppKit help tooltip gets installed” rather than the helper’s literal return value. As per coding guidelines, "Do not add tests that only verify source code text, method signatures, AST fragments, or grep-style patterns; tests must verify observable runtime behavior" and "If a behavior cannot be exercised end-to-end yet, add a small runtime seam or harness first, then test through that seam rather than testing implementation details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WorkspaceContentViewVisibilityTests.swift` around lines 93 - 100,
The test currently only asserts the policy method
ContentView.titlebarPageControlsShouldInstallHelpTooltips() returns false;
instead, render a titlebar control at runtime (via a small test seam or
test-only factory such as adding a TestTitlebarControl or a
ContentView.makeTitlebarControlForTesting() that produces the actual control
used in the titlebar) and assert no AppKit help tooltip gets registered on the
rendered view (e.g., verify the view and its subviews have no toolTip/helpTag
set and that addToolTip/removeToolTip were not applied) so the test checks
observable behavior rather than the policy return value.
Greptile SummaryThis PR fixes a use-after-free crash (Sentry
Confidence Score: 5/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Render titlebar page strip] --> B{isTitlebarHovered?}
B -- No --> C[Render page tabs only]
B -- Yes --> D[Create newPageButton\nwith .accessibilityLabel]
C --> E{titlebarPageControlsShouldInstallHelpTooltips?}
D --> E
E -- true\n never --> F[Apply .help tooltip\nAppKit tracking area registered]
E -- false\n always --> G[Render button as-is\nNo AppKit tracking area]
F --> H[Crash risk: UAF on\ntracking area during churn]
G --> I[Safe: no tooltip\ntracking area installed]
Last reviewed commit: 54afe8f |
NSToolTipManager installs per-view tooltip handlers via SwiftUI's .help() modifier. When a hosting view is removed during split churn or workspace teardown, the NSView backing the tooltip may be freed while NSToolTipManager still holds a reference, causing a use-after-free crash on the next mouse-enter event. Removed .safeHelp() from the three high-churn titlebar control buttons (toggle sidebar, notifications, new workspace) which remount frequently during tab management. The .accessibilityLabel() calls already present on all three buttons preserve VoiceOver discoverability without touching NSToolTipManager. The safeHelp() extension remains available for use on stable views that do not remount during normal workspace interactions. Reported-by: @austinywang <upstream issue manaflow-ai/cmux#1036> Reference: manaflow-ai/cmux#1038 by @lawrencecchen Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…cks) (#87) * fix: propagate SSH_AUTH_SOCK through SSH subprocess environment macOS GUI apps launched from Finder or Dock do not inherit SSH_AUTH_SOCK or SSH_AGENT_PID because those are set by ssh-agent in the user's shell, not in the launch environment. Previously, WorkspaceRemoteDaemonRPCClient.start() and startReverseRelayLocked() created Process objects without setting process.environment, leaving it nil and relying on implicit inheritance. Explicit assignment of ProcessInfo.processInfo.environment to both SSH Process objects ensures agent socket variables pass through and SSH key authentication works from the GUI app. Reported-by: @knight42 <upstream issue manaflow-ai/cmux#3162> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: parse K/M/G byte-count suffixes in scrollback-limit config The Ghostty config format documents that scrollback-limit accepts K, M, and G suffixes (kibibytes, mebibytes, gibibytes). The previous parser used plain Int(value) and silently discarded values like "1G", "512M", or "100K", leaving the scrollback limit at the 10000-line default. Added a private parseByteCount helper that strips a trailing case-insensitive K/M/G suffix, parses the numeric prefix, and multiplies by the appropriate power-of-1024 factor. Plain integers continue to work unchanged. The Ghostty Zig layer expects a raw usize byte count, so expansion is handled entirely in the Swift config reader. Reported-by: @shaun0927 <upstream issue manaflow-ai/cmux#2950> Reference: manaflow-ai/cmux#2952 by @shaun0927 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: resolve named palette colors in WorkspaceApplyPlan customColor Config and CLI paths could supply a color as a human-readable name like "Red" or "Blue" rather than a hex string. The executor passed the raw string directly to setCustomColor, which normalizes only valid 6-digit hex values and silently no-ops everything else, causing the workspace tab to render with no color. Added resolveColorToHex, a nonisolated static helper that first tries normalizedHex for valid hex input and then does a case-insensitive lookup in WorkspaceTabColorSettings.defaultPaletteWithOverrides. Strings that are neither valid hex nor known palette names produce an ApplyFailure with code "unknown_color_name" rather than a silent no-op. Reported-by: @zacharygutt <upstream issue manaflow-ai/cmux#3075> Reference: manaflow-ai/cmux#3095 by @austinywang Reference: manaflow-ai/cmux#3149 by @lawrencecchen Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: invalidate GhosttyConfig cache on system appearance change light:X,dark:Y paired themes resolve to the correct variant via GhosttyConfig.currentColorSchemePreference(), but the load cache keyed on ColorSchemePreference was never cleared when macOS switched between light and dark mode. The stale cached config would be returned on the first load after an appearance change, keeping the wrong theme active until the next explicit config reload. Added a NSKeyValueObservation on NSApp.effectiveAppearance in AppDelegate.applicationDidFinishLaunching that calls GhosttyConfig.invalidateLoadCache() and then triggers GhosttyApp.shared.reloadConfiguration so terminals pick up the correct light or dark theme variant immediately. Reported-by: @Corey-T1000 <upstream issue manaflow-ai/cmux#2922> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: include .badge in notification authorization request requestAuthorizationIfNeeded called UNUserNotificationCenter.requestAuthorization with options [.alert, .sound] but omitted .badge. On macOS, setting NSApp.dockTile.badgeLabel requires .badge authorization — without it the assignment silently no-ops and the badge counter never appears in the Dock. The rest of the badge pipeline (isDockBadgeEnabled check, dockBadgeLabel computation, dockTile.badgeLabel assignment) was already correct. Adding .badge to the authorization options closes both #1764 and #2718 which report the same symptom. Reported-by: @gilsiun <upstream issue manaflow-ai/cmux#1764> Also-closes: manaflow-ai/cmux#2718 (reported by @tuzisang) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: remove .safeHelp from high-churn titlebar buttons to prevent UAF NSToolTipManager installs per-view tooltip handlers via SwiftUI's .help() modifier. When a hosting view is removed during split churn or workspace teardown, the NSView backing the tooltip may be freed while NSToolTipManager still holds a reference, causing a use-after-free crash on the next mouse-enter event. Removed .safeHelp() from the three high-churn titlebar control buttons (toggle sidebar, notifications, new workspace) which remount frequently during tab management. The .accessibilityLabel() calls already present on all three buttons preserve VoiceOver discoverability without touching NSToolTipManager. The safeHelp() extension remains available for use on stable views that do not remount during normal workspace interactions. Reported-by: @austinywang <upstream issue manaflow-ai/cmux#1036> Reference: manaflow-ai/cmux#1038 by @lawrencecchen Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: subtract legacy scrollbar gutter from terminal width in .legacy mode When a mouse is connected, macOS switches to legacy (always-visible) scrollers. The scroll view was initialized with scrollerStyle = .overlay but the system can override this, causing the vertical scrollbar to occupy physical column space. The previous synchronizeCoreSurface() used scrollView.contentSize.width directly, which does not account for the scrollbar gutter in legacy mode, so the rightmost terminal columns render under the scrollbar. The fix checks scrollView.scrollerStyle and subtracts NSScroller.scrollerWidth(for:.regular, scrollerStyle:.legacy) only when the effective style is .legacy. Overlay mode behavior is unchanged — that path was a prior deliberate decision to prevent column-flap during split churn (comment at line 8544 is preserved). Reported-by: @Thinkscape <upstream issue manaflow-ai/cmux#2997> Reference: manaflow-ai/cmux#3001 by @rdsciv Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: add coalescing guard to palette overlay update to reduce idle spin The focus-reassert path (reassertTerminalSurfaceFocus, 0.05s throttle) and first-responder scheduling (scheduleAutomaticFirstResponderApply, pendingAutomaticFirstResponderApply flag) were already guarded in c11 against redundant work. The third idle runloop offender was the WindowCommandPaletteOverlayController.update() path: SwiftUI calls it on every parent render cycle, and when the palette is hidden each call re-assigned hostingView.rootView = AnyView(EmptyView()), which drives a redundant SwiftUI layout pass. Added an early-exit guard: when both the current and new visibility state are false, skip the update entirely. The transition from visible to hidden is preserved — the guard only elides the no-op hidden-to-hidden calls. Reported-by: @lawrencecchen <upstream issue manaflow-ai/cmux#2996> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: prevent white-on-white text in light mode and honor config palette entries Two-part fix for the light-mode text regression introduced in 0.63.2: 1. Contrast fallback: added applyContrastFallbackIfNeeded(), a mutating helper called at the end of loadFromDisk. When both backgroundColor and foregroundColor have luminance > 0.5 (both near-white), the foreground is replaced with #1A1A1A. This closes the case where a light theme loads the background correctly but the foreground defaults to the near-white Monokai value (#fdfff1) because the theme file does not explicitly set foreground. This fix layers on Pick 5 (cache invalidation on appearance change) which ensures the correct light-vs-dark config is loaded after a system switch. 2. Palette override: applyPalette(to:config:) now checks config.palette[i] before falling back to the Monokai hardcoded default. When a theme file or config key sets palette entries, those colors are used for the ANSI palette instead of the defaults. Reported-by: @exlaw <upstream issue manaflow-ai/cmux#2708> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ghostty: bump submodule to Stage-11-Agentics fork, fix PageList SIGSEGV race (#2738) Point the ghostty submodule at the Stage-11-Agentics/ghostty fork (all other c11 submodules already live under Stage-11-Agentics). The fork is based on manaflow-ai/ghostty main and carries one additional fix: terminal: snapshot page rows in SlidingWindow.Meta to fix SIGSEGV race SlidingWindow.highlight() was reading meta.node.data.size.rows directly from the live page node without holding the terminal lock. The IO thread calls resizeCols() (with the terminal lock) and frees old page nodes, so a concurrent search thread calling next() could crash with a use-after-free SIGSEGV. The fix snapshots the row count into Meta.rows during append() (which IS called under the terminal lock) and uses that snapshot throughout highlight(). Reported-by: @tmad4000 <upstream issue manaflow-ai/cmux#2738> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * localize: add workspace.apply.unknownColorName translations for 6 locales Covers C11-22 pick 4 (#3075) error string: unknown named color in config. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: guard parseByteCount against overflow and negative values scrollback-limit = 9000000000G caused an Int multiplication trap at config load. Add overflow check via multipliedReportingOverflow and reject negative prefixes before the multiply. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: update ghostty-fork.md for Stage-11-Agentics fork and PageList race fix Document the submodule URL change (manaflow-ai -> Stage-11-Agentics), the PageList SIGSEGV fix in sliding_window.zig, and conflict notes for future upstream syncs. Required by CLAUDE.md: "Keep docs/ghostty-fork.md up to date with any fork changes and conflict notes." Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: log when applyContrastFallbackIfNeeded overrides foreground color Silent override produced confusing 'my foreground setting is not applying' bugs. Log the old/new values so users can find this via Console.app or debug output. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: document named palette color support for customColor in schema WorkspaceLayoutExecutor now accepts named palette colors (e.g. "Red") in addition to hex strings. Unknown names produce an ApplyFailure with code unknown_color_name. Schema previously said hex-only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor: make parseByteCount a static func (reads no instance state) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ghostty: rebase PageList race fix onto c11 theme-picker fork tip (bc9be90a) The previous submodule bump (df2a237) was based on a fresh manaflow-ai/ghostty main, accidentally dropping 7 c11 theme-picker commits that c11 requires. This commit rebases the PageList SIGSEGV race fix onto bc9be90a (the existing c11 fork tip) so both the theme-picker hooks and the PageList fix are present. The 7 theme-picker commits modify src/cli/list_themes.zig to read CMUX_THEME_PICKER_COLOR_SCHEME, CMUX_THEME_PICKER_INITIAL_LIGHT, and CMUX_THEME_PICKER_INITIAL_DARK -- env vars set by c11 before calling ghostty +list-themes. Without them the theme picker ignores c11's setup. Reported-by: @tmad4000 <upstream issue manaflow-ai/cmux#2738> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: correct ghostty-fork.md ancestry and rename cmux to c11 in theme picker sections The fork base is bc9be90a (c11 theme-picker tip), not manaflow-ai main. Rename 'cmux theme picker' to 'c11 theme picker' per naming policy. Update PageList fix SHA to c64952975 (cherry-picked commit). Add upstream-sync note for list_themes.zig conflict guidance. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: reject plain negative values in parseByteCount fallback path scrollback-limit = -1 was accepted as -1 (no suffix branch, guard did not apply). Guard the Int(s) fallback the same way. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: substitute color name into ApplyFailure message for unknown_color_name String(localized:defaultValue:"\(color)") returns the xcstrings value verbatim in non-development builds; %@ was not substituted. Use String(format:) to fill the placeholder. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: fix customColor example to use a confirmed palette color name "Ocean Blue" does not exist in the palette; use a name that does. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: validate palette index is in 0...15 before assignment Out-of-bounds indices were silently accepted and could write past the 16-entry palette array. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: update customColor doc comment to mention named palette colors Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * ci: fix build-ghosttykit and download to use Stage-11-Agentics/ghostty The workflow was publishing xcframework releases to manaflow-ai/ghostty using a token that only has write access to Stage-11-Agentics repos, causing exit code 4 on every PR. Similarly the download script was fetching from manaflow-ai/ghostty where no release exists for the Stage-11-Agentics fork's ghostty SHA. Fixes: - build-ghosttykit.yml: check-release and upload steps now target Stage-11-Agentics/ghostty - build-ghosttykit.yml: add "Pin checksum" step that computes SHA256 of the tarball post-upload and commits it to ghosttykit-checksums.txt, eliminating the manual step and the chicken-and-egg where the checksum guard fires before the release exists - download-prebuilt-ghosttykit.sh: default DOWNLOAD_URL now points to Stage-11-Agentics/ghostty Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: publish GhosttyKit releases to c11 repo using GITHUB_TOKEN GHOSTTY_RELEASE_TOKEN is not configured on this fork. Switch the build-ghosttykit workflow to publish xcframework releases to Stage-11-Agentics/c11 instead, using GITHUB_TOKEN with an explicit contents:write permission grant. Update download-prebuilt-ghosttykit.sh to fetch from the same location. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: checkout branch ref so git push works in Pin checksum step Actions checks out a detached HEAD for PRs by default, causing git push to fail with exit 128. Checking out the actual branch ref (head_ref for PRs, ref_name for push events) puts us on a real branch so the push succeeds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: always run Pin checksum, download tarball when release pre-exists If the release was created in a previous CI run (exists=true), the old conditional skipped Pin checksum entirely, leaving ghosttykit-checksums.txt unpopulated forever. Remove the condition and download the tarball from the existing release when the local file isn't present, so the checksum is always committed regardless of which run built the release. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: pin GhosttyKit checksum for ghostty c649529750b12e7fde7a33b74d5310a1b988cb67 --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Summary
.help()tooltips on transient titlebar page controlsTesting
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -only-testing:cmuxTests/TitlebarPageTooltipPolicyTests/testTitlebarPageControlsNeverInstallAppKitHelpTooltips test./scripts/reload.sh --tag issue-1036-tooltip-tracking-uafIssues
Context
Sentry issue
CMUXTERM-MACOS-1X1first appeared on 2026-02-28 in stablecom.cmuxterm.app@0.61.0+73, so the groupedNSWindow.cmux_sendEventtracking-area crash already existed. The nightly event in #1036 first appears incom.cmuxterm.app.nightly@0.61.0-nightly.20260307+2279003110001, and the most likely recent trigger is commit4de975e6a4cc84fd099aa1673348617cafd0e50afrom #1030, which added.help()tracking to the new titlebar page strip.Summary by cubic
Disable AppKit help tooltips on titlebar page controls to prevent tracking-area crashes during hover, drag, close, and selection changes. Keeps the New Page button accessible and adds a guard test.
Written for commit 54afe8f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests