Repository navigation
Use workspace color for notification ring and selection bar - #664
Merged
Merged
Conversation
- Notification/focus flash uses workspace customColor (fallback: accent) - Selection bar/indicator uses workspace customColor when set - Flash color propagated through Panel.triggerFlash(color:) API - Browser panel flash overlay uses workspace color - Regression tests for flash color resolution Fixes #557
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe pull request adds optional color customization for focus/notification flash visuals throughout the application. Color parameters are threaded through the Panel protocol and concrete implementations (TerminalPanel, BrowserPanel), enabling runtime color selection for flash animations. Workspace-level color resolution provides hex string validation and fallback logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
2956-2970:⚠️ Potential issue | 🟠 MajorNotification flash API still blocks per-notification color and non-terminal flash paths.
Line 2961 hard-restricts this flow to
TerminalPanel, and Line 2969 always uses workspace-derived color. That bypasses the new panel-agnostictriggerFlash(color:)path and leaves no entry point for CLI/socket-supplied notification colors.💡 Proposed fix
- func triggerNotificationFocusFlash( - panelId: UUID, - requiresSplit: Bool = false, - shouldFocus: Bool = true - ) { - guard let terminalPanel = terminalPanel(for: panelId) else { return } + func triggerNotificationFocusFlash( + panelId: UUID, + requiresSplit: Bool = false, + shouldFocus: Bool = true, + colorHex: String? = nil + ) { + guard let panel = panels[panelId] else { return } if shouldFocus { focusPanel(panelId) } let isSplit = bonsplitController.allPaneIds.count > 1 || panels.count > 1 if requiresSplit && !isSplit { return } - terminalPanel.triggerFlash(color: focusFlashColor) + let resolvedColor = Self.resolvedFocusFlashColor( + customColorHex: colorHex ?? customColor + ) + panel.triggerFlash(color: resolvedColor) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 2956 - 2970, The current triggerNotificationFocusFlash function restricts flashes to TerminalPanel and always uses workspace focusFlashColor; change it so it locates the generic panel (use the panel lookup used elsewhere instead of terminalPanel(for:)), add an optional color parameter (e.g., color: Color? = nil) and pass (color ?? focusFlashColor) to the panel's triggerFlash(color:) method, preserve the requiresSplit/isSplit check and focusPanel(panelId) behavior, and remove the TerminalPanel-specific casting so both terminal and non-terminal panels (and CLI/socket-supplied colors) go through the panel-agnostic triggerFlash(color:) path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4158-4166: setFocusFlashColor(_:) is overwriting explicit flash
colors because setNotificationRing(visible:) calls it with nil; update
setFocusFlashColor(_:updateFlash:) (or create separate setRingColor(_:) and
setFlashColor(_:) methods) so ring updates can opt out of mutating flashLayer;
modify setNotificationRing(visible:) to call the ring-only path (e.g.,
setRingColor(resolved) or setFocusFlashColor(resolved, updateFlash: false)) and
keep triggerFlash(color:) writing the flashLayer color directly so explicit
flashes are not clobbered.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 2956-2970: The current triggerNotificationFocusFlash function
restricts flashes to TerminalPanel and always uses workspace focusFlashColor;
change it so it locates the generic panel (use the panel lookup used elsewhere
instead of terminalPanel(for:)), add an optional color parameter (e.g., color:
Color? = nil) and pass (color ?? focusFlashColor) to the panel's
triggerFlash(color:) method, preserve the requiresSplit/isSplit check and
focusPanel(panelId) behavior, and remove the TerminalPanel-specific casting so
both terminal and non-terminal panels (and CLI/socket-supplied colors) go
through the panel-agnostic triggerFlash(color:) path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Sources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/Panel.swiftSources/Panels/TerminalPanel.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
Comment on lines
+4158
to
+4166
| private func setFocusFlashColor(_ color: NSColor?) { | ||
| let resolved = color ?? resolvedWorkspaceFocusFlashColor() | ||
| CATransaction.begin() | ||
| CATransaction.setDisableActions(true) | ||
| notificationRingLayer.strokeColor = resolved.cgColor | ||
| notificationRingLayer.shadowColor = resolved.cgColor | ||
| flashLayer.strokeColor = resolved.cgColor | ||
| flashLayer.shadowColor = resolved.cgColor | ||
| CATransaction.commit() |
There was a problem hiding this comment.
setNotificationRing can unintentionally overwrite explicit flash colors.
setFocusFlashColor(_:) updates both ring and flash layers. At Line 4194, setNotificationRing(visible:) calls it with nil, so frequent SwiftUI updates can reset flashLayer to workspace/accent and clobber the explicit color passed by triggerFlash(color:) at Line 4425.
Consider splitting ring and flash color application, or adding a flag so ring updates do not mutate flashLayer color.
Suggested fix
- private func setFocusFlashColor(_ color: NSColor?) {
+ private func setFocusFlashColor(_ color: NSColor?, includeFlashLayer: Bool = true, includeRingLayer: Bool = true) {
let resolved = color ?? resolvedWorkspaceFocusFlashColor()
CATransaction.begin()
CATransaction.setDisableActions(true)
- notificationRingLayer.strokeColor = resolved.cgColor
- notificationRingLayer.shadowColor = resolved.cgColor
- flashLayer.strokeColor = resolved.cgColor
- flashLayer.shadowColor = resolved.cgColor
+ if includeRingLayer {
+ notificationRingLayer.strokeColor = resolved.cgColor
+ notificationRingLayer.shadowColor = resolved.cgColor
+ }
+ if includeFlashLayer {
+ flashLayer.strokeColor = resolved.cgColor
+ flashLayer.shadowColor = resolved.cgColor
+ }
CATransaction.commit()
}
func setNotificationRing(visible: Bool) {
if !Thread.isMainThread {
DispatchQueue.main.async { [weak self] in
self?.setNotificationRing(visible: visible)
}
return
}
- setFocusFlashColor(nil)
+ setFocusFlashColor(nil, includeFlashLayer: false, includeRingLayer: true)
CATransaction.begin()
CATransaction.setDisableActions(true)
notificationRingOverlayView.isHidden = !visible
notificationRingLayer.opacity = visible ? 1 : 0
CATransaction.commit()
}
func triggerFlash(color: NSColor? = nil) {
DispatchQueue.main.async { [weak self] in
guard let self else { return }
- self.setFocusFlashColor(color)
+ self.setFocusFlashColor(color, includeFlashLayer: true, includeRingLayer: false)
self.updateFlashPath()
...
}
}Also applies to: 4194-4194, 4417-4426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 4158 - 4166,
setFocusFlashColor(_:) is overwriting explicit flash colors because
setNotificationRing(visible:) calls it with nil; update
setFocusFlashColor(_:updateFlash:) (or create separate setRingColor(_:) and
setFlashColor(_:) methods) so ring updates can opt out of mutating flashLayer;
modify setNotificationRing(visible:) to call the ring-only path (e.g.,
setRingColor(resolved) or setFocusFlashColor(resolved, updateFlash: false)) and
keep triggerFlash(color:) writing the flashLayer color directly so explicit
flashes are not clobbered.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a64e82858a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch activeTabIndicatorStyle { | ||
| case .leftRail: | ||
| if isActive { return Color(nsColor: sidebarSelectedWorkspaceBackgroundNSColor(for: colorScheme)) } | ||
| if isActive { return activeSelectionColor } |
There was a problem hiding this comment.
Switching the active background to activeSelectionColor means an active workspace can now use arbitrary custom hex colors, but the active foreground still comes from sidebarSelectedWorkspaceForegroundNSColor (hardcoded white in ContentView.swift). For light custom colors (for example #F5E663), the title/metadata/icons become very low-contrast and hard to read, which is a regression from the previous fixed accent background. Please derive active foreground from the resolved custom color (or compute contrast) when isActive uses resolvedCustomTabColor.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryThis PR replaces hardcoded
The implementation properly propagates colors through the Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Workspace
participant Panel
participant TerminalPanel
participant GhosttySurfaceScrollView
participant BrowserPanel
participant BrowserPanelView
User->>Workspace: Trigger flash (Cmd+Shift+H)
Workspace->>Workspace: resolvedFocusFlashColor(customColorHex)
alt Custom color set
Workspace-->>Workspace: NSColor from hex
else No custom color
Workspace-->>Workspace: cmuxAccentNSColor()
end
Workspace->>Panel: triggerFlash(color: focusFlashColor)
alt Terminal Panel
Panel->>TerminalPanel: triggerFlash(color)
TerminalPanel->>GhosttySurfaceScrollView: triggerFlash(color)
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: setFocusFlashColor(color)
alt color is nil
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: resolvedWorkspaceFocusFlashColor()
end
GhosttySurfaceScrollView->>GhosttySurfaceScrollView: Update CALayer colors
else Browser Panel
Panel->>BrowserPanel: triggerFlash(color)
BrowserPanel->>BrowserPanel: focusFlashColor = color ?? accent
BrowserPanel->>BrowserPanelView: Updates @Published property
BrowserPanelView->>BrowserPanelView: Render overlay with color
end
Last reviewed commit: a64e828 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .foregroundColor(activeSecondaryColor(0.8)) | ||
| } | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
Extra blank line
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This was referenced Feb 28, 2026
This was referenced Mar 8, 2026
This was referenced Mar 18, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…-ai#664) - Notification/focus flash uses workspace customColor (fallback: accent) - Selection bar/indicator uses workspace customColor when set - Flash color propagated through Panel.triggerFlash(color:) API - Browser panel flash overlay uses workspace color - Regression tests for flash color resolution Fixes manaflow-ai#557
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…anaflow-ai#664)" This reverts commit fcec53e.
This was referenced Apr 4, 2026
This was referenced Sep 19, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
customColorinstead of hardcodedsystemBlue(falls back to app accent color)Panel.triggerFlash(color:)APIFixes #557
What's NOT included
Test plan
Summary by CodeRabbit