Repository navigation
Move sidebar appearance support out of ContentView - #3106
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughSidebar appearance rendering components—including background, border, glass effect, and tint configuration types—are relocated from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 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 |
Greptile SummaryThis PR is a pure file-organization refactoring: all sidebar appearance types ( Confidence Score: 5/5Safe to merge — pure file-organization refactoring with no behavioral changes. All moved code is byte-for-byte identical in logic; access-level adjustments are intentional and correct; no callers are broken; no new bugs introduced. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CV[ContentView.swift]
CA[cmuxApp.swift]
KS[KeyboardShortcutSettingsFileStore.swift]
WC[WorkspaceContentView.swift]
CV -->|uses| SB[SidebarBackdrop]
CV -->|uses| STB[SidebarTrailingBorder]
CV -->|uses| SBMO[SidebarBlendModeOption]
CA -->|uses| SMP[SidebarMaterialOption]
CA -->|uses| SBMO
CA -->|uses| SSO[SidebarStateOption]
CA -->|uses| SPO[SidebarPresetOption]
CA -->|uses| STD[SidebarTintDefaults]
KS -->|uses| STD
WC -->|uses| HEX["NSColor.hexString()"]
subgraph SA["SidebarAppearanceSupport.swift - after PR"]
SB
STB
SMP
SBMO
SSO
SPO
STD
HEX
SVEB["SidebarVisualEffectBackground - private"]
STBV["SidebarTerminalBackgroundView - private"]
SB --> SVEB
SB --> STBV
end
Reviews (1): Last reviewed commit: "refactor: move sidebar appearance suppor..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Sidebar/SidebarAppearanceSupport.swift (1)
423-441:terminalBackgroundColoris written but never read.The
@Stateat Line 423 is only assigned in theonReceivehandler at Lines 438–440 and never consumed in the view body —SidebarTerminalBackgroundViewreadsGhosttyApp.shared.defaultBackgroundColordirectly at Line 434. The write still triggers a re-render that picks up the fresh shared value, so behavior is correct, but the state variable is effectively a dummy re-render signal and is easy to misread as the real source.Behavior-preserving move, so non-blocking — consider threading the state into the background color (or replacing it with a bump counter) in a follow-up for clarity.
♻️ Suggested cleanup
- `@State` private var terminalBackgroundColor: NSColor = GhosttyBackgroundTheme.currentColor() + `@State` private var terminalBackgroundColor: NSColor = GhosttyBackgroundTheme.currentColor() @@ - let alpha = CGFloat(GhosttyApp.shared.defaultBackgroundOpacity) + let alpha = CGFloat(GhosttyApp.shared.defaultBackgroundOpacity) return AnyView( SidebarTerminalBackgroundView( - backgroundColor: GhosttyApp.shared.defaultBackgroundColor, + backgroundColor: terminalBackgroundColor, opacity: alpha ) .clipShape(RoundedRectangle(cornerRadius: cornerRadius, style: .continuous)) .onReceive(NotificationCenter.default.publisher(for: .ghosttyDefaultBackgroundDidChange)) { _ in terminalBackgroundColor = GhosttyBackgroundTheme.currentColor() } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/SidebarAppearanceSupport.swift` around lines 423 - 441, The `@State` terminalBackgroundColor is only written in the onReceive handler and never read, making it a dummy re-render signal; fix by either (A) thread the state into the view by replacing GhosttyApp.shared.defaultBackgroundColor with terminalBackgroundColor in the SidebarTerminalBackgroundView initializer and set terminalBackgroundColor = GhosttyBackgroundTheme.currentColor() in the onReceive, or (B) replace terminalBackgroundColor with a clearer bump counter (e.g., backgroundChangeCounter) that you increment in onReceive and keep using GhosttyApp.shared.defaultBackgroundColor, so the intent is explicit; update references to terminalBackgroundColor, the onReceive closure, and SidebarTerminalBackgroundView usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Sidebar/SidebarAppearanceSupport.swift`:
- Around line 423-441: The `@State` terminalBackgroundColor is only written in the
onReceive handler and never read, making it a dummy re-render signal; fix by
either (A) thread the state into the view by replacing
GhosttyApp.shared.defaultBackgroundColor with terminalBackgroundColor in the
SidebarTerminalBackgroundView initializer and set terminalBackgroundColor =
GhosttyBackgroundTheme.currentColor() in the onReceive, or (B) replace
terminalBackgroundColor with a clearer bump counter (e.g.,
backgroundChangeCounter) that you increment in onReceive and keep using
GhosttyApp.shared.defaultBackgroundColor, so the intent is explicit; update
references to terminalBackgroundColor, the onReceive closure, and
SidebarTerminalBackgroundView usage accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2627b32a-7c62-4704-86f3-800ff43f7bbf
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/Sidebar/SidebarAppearanceSupport.swift
💤 Files with no reviewable changes (1)
- Sources/ContentView.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Sidebar/SidebarAppearanceSupport.swift">
<violation number="1" location="Sources/Sidebar/SidebarAppearanceSupport.swift:374">
P1: Avoid calling `getRed` on the original NSColor if sRGB conversion fails, as this will crash if the color is not RGB-compatible.
(Based on your team's feedback about using safe fallbacks when NSColor conversion fails.) [FEEDBACK_USED]</violation>
<violation number="2" location="Sources/Sidebar/SidebarAppearanceSupport.swift:697">
P1: Return a safe fallback string instead of calling `getRed` on the original color if sRGB conversion fails.
(Based on your team's feedback about using safe fallbacks when NSColor conversion fails.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| extension NSColor { | ||
| func hexString(includeAlpha: Bool = false) -> String { | ||
| let color = usingColorSpace(.sRGB) ?? self |
There was a problem hiding this comment.
P1: Return a safe fallback string instead of calling getRed on the original color if sRGB conversion fails.
(Based on your team's feedback about using safe fallbacks when NSColor conversion fails.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Sidebar/SidebarAppearanceSupport.swift, line 697:
<comment>Return a safe fallback string instead of calling `getRed` on the original color if sRGB conversion fails.
(Based on your team's feedback about using safe fallbacks when NSColor conversion fails.) </comment>
<file context>
@@ -268,3 +268,445 @@ func sidebarWorkspaceRowBackgroundStyle(
+
+extension NSColor {
+ func hexString(includeAlpha: Bool = false) -> String {
+ let color = usingColorSpace(.sRGB) ?? self
+ var red: CGFloat = 0
+ var green: CGFloat = 0
</file context>
| let color = usingColorSpace(.sRGB) ?? self | |
| guard let color = usingColorSpace(.sRGB) else { return includeAlpha ? "#00000000" : "#000000" } |
There was a problem hiding this comment.
Verified. This code was moved unchanged in this refactor, so I am keeping this PR behavior-preserving. The safe fallback change is valid follow-up work for a separate behavior-fix PR.
— Claude Code
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
| /// Replicates bonsplit TabBarColors.nsColorSeparator derivation from chrome background. | ||
| private static func chromeSeparatorColor() -> NSColor { | ||
| let chrome = GhosttyBackgroundTheme.currentColor() | ||
| let srgb = chrome.usingColorSpace(.sRGB) ?? chrome |
There was a problem hiding this comment.
P1: Avoid calling getRed on the original NSColor if sRGB conversion fails, as this will crash if the color is not RGB-compatible.
(Based on your team's feedback about using safe fallbacks when NSColor conversion fails.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Sidebar/SidebarAppearanceSupport.swift, line 374:
<comment>Avoid calling `getRed` on the original NSColor if sRGB conversion fails, as this will crash if the color is not RGB-compatible.
(Based on your team's feedback about using safe fallbacks when NSColor conversion fails.) </comment>
<file context>
@@ -268,3 +268,445 @@ func sidebarWorkspaceRowBackgroundStyle(
+ /// Replicates bonsplit TabBarColors.nsColorSeparator derivation from chrome background.
+ private static func chromeSeparatorColor() -> NSColor {
+ let chrome = GhosttyBackgroundTheme.currentColor()
+ let srgb = chrome.usingColorSpace(.sRGB) ?? chrome
+ var r: CGFloat = 0, g: CGFloat = 0, b: CGFloat = 0, a: CGFloat = 0
+ srgb.getRed(&r, green: &g, blue: &b, alpha: &a)
</file context>
| let srgb = chrome.usingColorSpace(.sRGB) ?? chrome | |
| guard let srgb = chrome.usingColorSpace(.sRGB) else { return NSColor(white: 0.5, alpha: 0.26) } |
There was a problem hiding this comment.
Verified. This is pre-existing behavior in moved code. I am leaving it unchanged here to keep the refactor move-only; the safe fallback belongs in a separate focused fix.
— Claude Code
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary\n- move sidebar background/material/preset support from ContentView into SidebarAppearanceSupport\n- keep behavior unchanged and avoid project-file churn by using the existing sidebar support source\n\n## Verification\n- ./scripts/reload.sh --tag sbarapp
Summary by cubic
Moved sidebar appearance logic out of ContentView into SidebarAppearanceSupport to isolate sidebar UI and simplify ContentView. No UI or behavior changes.
SidebarBackdrop,SidebarTrailingBorder,SidebarVisualEffectBackground, related enums) intoSources/Sidebar/SidebarAppearanceSupport.swift.Written for commit c437f06. Summary will update on new commits.
Summary by CodeRabbit