Repository navigation
Fix sidebar tint transparency backdrop ownership - #3382
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes window backdrop and glass-effect logic into a WindowBackdropController with plan-based snapshots, switches overlay installation/hit-testing to a container/reference targeting model, updates overlay controllers and file-drop overlay attachment accordingly, and adds/adjusts tests and project entries. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant CV as ContentView
participant GTV as GhosttyTerminalView
participant WAS as WindowAppearanceSnapshot
participant WBC as WindowBackdropController
participant NSW as NSWindow
participant WGE as WindowGlassEffect
App->>WAS: currentFromUserDefaults(...)
WAS-->>App: WindowAppearanceSnapshot
App->>WAS: backdropPlan(...)
WAS-->>App: WindowBackdropPlan
App->>WBC: apply(snapshot:to:window)
WBC->>WBC: compute hostingPhase & plan
WBC->>NSW: set backgroundColor / isOpaque
alt plan.useWindowGlass
WBC->>WGE: install/update glass effect (tint/style)
else
WBC->>WGE: remove glass effect if present
end
WBC-->>App: WindowBackdropApplicationResult
CV->>CV: windowContentOverlayInstallationTarget(for:)
CV-->>CV: (containerView, referenceView)
CV->>CV: install overlays constrained to referenceView bounds
CV->>CV: hit-test overlays against containerView
GTV->>WBC: apply(snapshot:to:itsWindow)
WBC->>NSW: reflect hostingPhase on window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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. Review rate limit: 6/8 reviews remaining, refill in 14 minutes and 49 seconds.Comment |
Greptile SummaryThis PR centralizes window backdrop selection into Confidence Score: 4/5Safe to merge; only P2 findings around a dead stub property and a theoretical unhandled struct combination All findings are P2. The refactoring is well-structured and the new plan-based backdrop system is a clear improvement. The two P2 issues — a dead Sources/Windowing/WindowAppearanceSnapshot.swift — Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WindowAppearanceSnapshot] -->|backdropPlan| B[WindowBackdropPlan]
B --> C{hostingPhase}
C -->|opaqueWindowFill| D[remove glass\nset opaque background]
C -->|transparentRootBackdrop| E[remove glass\nclear background\napply blur if needed]
C -->|windowGlass| F[apply WindowGlassEffect\nclear background]
F -->|didChangeGlassRoot| G[re-install overlays]
G --> H{glass active?}
H -->|yes| I[container = foregroundContainer\nreference = originalContentView]
H -->|no| J[container = themeFrame\nreference = contentView]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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/ContentView.swift (1)
1267-1287: 💤 Low valueMinor inconsistency: No debug logging in TmuxWorkspacePaneOverlayController.ensureInstalled.
CommandPaletteOverlayController logs overlay installation details at lines 792-798, but this controller does not. Consider adding equivalent debug logging for consistency in diagnosing overlay installation issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1267 - 1287, Add debug logging to ensureInstalled in TmuxWorkspacePaneOverlayController (the ensureInstalled() method) to mirror CommandPaletteOverlayController's install logs: when early-returning because window or target is missing, log that installation failed with window/target state; before changing parents (when containerView.superview !== target.container || installedReferenceView !== target.reference), log the target.container and target.reference and the current installedReferenceView; and after activating installConstraints and setting installedReferenceView, log successful installation details. Use the same logger and log level used by CommandPaletteOverlayController so messages are consistent with existing installation logs and reference the symbols ensureInstalled, windowContentOverlayInstallationTarget(for:), installConstraints, containerView, installedReferenceView, and target.reference.
🤖 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/ContentView.swift`:
- Around line 1267-1287: Add debug logging to ensureInstalled in
TmuxWorkspacePaneOverlayController (the ensureInstalled() method) to mirror
CommandPaletteOverlayController's install logs: when early-returning because
window or target is missing, log that installation failed with window/target
state; before changing parents (when containerView.superview !==
target.container || installedReferenceView !== target.reference), log the
target.container and target.reference and the current installedReferenceView;
and after activating installConstraints and setting installedReferenceView, log
successful installation details. Use the same logger and log level used by
CommandPaletteOverlayController so messages are consistent with existing
installation logs and reference the symbols ensureInstalled,
windowContentOverlayInstallationTarget(for:), installConstraints, containerView,
installedReferenceView, and target.reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cddffcf-85b1-4aa6-9b52-731175fc291c
📒 Files selected for processing (7)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/GhosttyTerminalView.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowGlassEffect.swiftSources/cmuxApp.swiftcmuxTests/GhosttyConfigTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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/WindowAppearanceSnapshotTests.swift`:
- Around line 55-60: The test is non-deterministic because
shouldUseTransparentHosting() and windowGlassSettings.shouldApply() read the
runtime WindowGlassEffect.isAvailable default while the assertion on
backdropPlan passes glassEffectAvailable: true; make the test deterministic by
forcing glass availability for the assertions: either set
WindowGlassEffect.isAvailable = true at the start of the test (and restore it
after) or, if available, call the variants that accept a glassEffectAvailable
parameter (e.g., snapshot.shouldUseTransparentHosting(glassEffectAvailable:
true) and snapshot.windowGlassSettings.shouldApply(glassEffectAvailable: true));
update the assertions to use the same glassEffectAvailable: true so all branches
exercise the glass-clear path consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15aba44f-643f-4eab-91b6-c27f9c6a4b9e
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftSources/Windowing/WindowAppearanceSnapshot.swiftSources/Windowing/WindowBackdropController.swiftSources/cmuxApp.swiftcmuxTests/GhosttyConfigTests.swiftcmuxTests/WindowAppearanceSnapshotTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/GhosttyConfigTests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/cmuxApp.swift
This branch was successfully deployed
1 participant
Summary
WindowBackdropPlan/WindowBackdropControllerTests
xcodebuild test -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -only-testing:cmuxTests/WindowAppearanceSnapshotTests -derivedDataPath /tmp/cmux-transparency-pr-test./scripts/reload.sh --tag transfixSummary by CodeRabbit