feat: add showCompactContentInExpandedMode for hybrid notch layouts #1
Conversation
Adds support for "hybrid" layouts where compact leading/trailing content remains visible while in expanded state. This enables use cases like dictation UIs with waveform indicators alongside the notch while transcript content shows below. Changes: - Add `showCompactContentInExpandedMode` property (defaults to false) - Add init parameter for convenience - Implement symmetric layout in NotchView for hybrid expanded mode - Add comprehensive documentation for layout logic - Add test cases for notch and floating styles Also fixes: - Prevent continuation leak in hide() when closePanelTask is cancelled 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a hybrid layout mode so compact indicators can remain visible with expanded content; exposes compact center content and toggle, reworks expand/compact/hide flow and hover/lifecycle handling, updates views/environment for hybrid layouts, makes NSScreen extension public, and adds hybrid-focused tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App / Caller
participant Notch as DynamicNotch
participant Screen as NSScreen
participant View as NotchView/NotchlessView
Note over App,Notch: User/Code requests state change
App->>Notch: call compact(on: NSScreen?) / expand(on: NSScreen?)
alt screen param nil
Notch->>Screen: resolve main or first screen
end
Notch->>Notch: update hybrid flags (floatingHybridModeActive / showCompactContentInExpandedMode)
Notch->>View: publish state changes (isHybridModeEnabled, isHovering, animations)
Note right of View: renders compact indicators row or overlay\nuses compactCenterContent / compactSideContent
View-->>App: visual state updated
Note over Notch: hover retries / animation sequencing / screenObserverTask lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (4)Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (2)
Sources/DynamicNotchKit/Views/NotchView.swift (1)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
Sources/DynamicNotchKit/Views/NotchlessView.swift (2)
🔇 Additional comments (17)
Comment |
Summary of ChangesHello @dima6312, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature to Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Expose `hasNotch`, `notchSize`, `notchFrame`, `menubarHeight`, and `screenWithMouse` for client apps that need to detect notch presence and adjust their behavior accordingly (e.g., always using expand() instead of compact() on non-notch Macs). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/Views/NotchView.swiftTests/DynamicNotchKitTests/DynamicNotchKitTests.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (2)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (3)
expand(169-171)compact(208-210)hide(257-263)Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (3)
expand(104-111)compact(113-120)hide(122-124)
Sources/DynamicNotchKit/Views/NotchView.swift (4)
Sources/DynamicNotchKit/Views/NotchlessView.swift (1)
notchContent(46-56)Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (2)
compact(208-210)updateHoverState(153-163)Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
compact(113-120)Sources/DynamicNotchKit/Utility/BlurModifier.swift (2)
blur(31-36)scale(38-43)
🔇 Additional comments (7)
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (2)
271-285: LGTM! Good test coverage for both styles.The hybrid mode test entry points are well-structured, following the existing pattern in the test file. The comment on lines 281-283 appropriately documents the expected graceful degradation behavior for floating style.
287-320: LGTM! Comprehensive test flow for hybrid mode.The helper function exercises the full lifecycle: expand (hybrid) → compact → expand again → hide. This validates that the
showCompactContentInExpandedMode: trueparameter works correctly and that transitions between states remain functional.Sources/DynamicNotchKit/Views/NotchView.swift (3)
79-111: LGTM! Clean layout logic with good documentation.The hybrid layout logic is well-structured:
showCompactInExpandedanduseHybridLayoutflags clearly express intent- The conditional width/height handling on lines 103-105 properly accommodates both normal and hybrid modes
- Documentation comments explain the layout behavior clearly
113-170: LGTM! Well-refactored compact content handling.The visibility and layout mode logic is clean:
showContentcorrectly determines visibility based on mode (hybrid vs normal)useSymmetricLayoutenables equal-width containers in hybrid-expanded mode- The refactoring to use
compactSideContenthelper reduces duplication
185-224: LGTM! Solid helper method implementation.The
compactSideContenthelper:
- Properly handles both symmetric and normal layout modes
- Uses appropriate edge padding (15pt for symmetric, 8pt for normal)
- Correctly applies environment, safe area insets, and width measurement
- Documentation is thorough
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (2)
78-81: LGTM! Clean property addition with good documentation.The new
showCompactContentInExpandedModeproperty:
- Is well-documented with clear use case explanation
- Defaults to
falsepreserving backwards compatibility- Uses
@Publishedappropriately for SwiftUI reactivity
99-109: LGTM! Backwards-compatible initializer update.The new parameter
showCompactContentInExpandedMode: Bool = falseis correctly positioned and defaults tofalse, ensuring existing code continues to work without modification.
When compact() is called in floating mode, instead of hiding the window, expand with hybrid mode enabled. This ensures compact indicators (like waveforms, status icons, cancel buttons) remain visible on non-notch Macs, providing consistent UX across all Mac hardware. Previously, non-notch Mac users would not see compact content at all since floating mode has no physical notch to flank. Now they see the same indicators as notch Mac users, displayed alongside the expanded content. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift">
<violation number="1" location="Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift:218">
P2: Mutating the public `showCompactContentInExpandedMode` property as a side effect of `compact()` will permanently overwrite any user-configured value. Consider using a separate internal flag or saving/restoring the original value to avoid changing user configuration.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
291-294: Continuation leak fix is incomplete—cleanup still skipped on cancellation.While calling
completion?()on cancellation prevents the continuation from waiting indefinitely, the window is never deinitialized when the task is cancelled. The past review comment specifically requested thatdeinitializeWindow()be called before resuming the continuation, even on cancellation, so thatawait hide()guarantees full cleanup. The current implementation still violates the documented contract: "waits for the animation to complete before returning" (line 268).🔎 Proposed fix
closePanelTask = Task { try? await Task.sleep(for: .seconds(0.4)) // Wait for animation to complete - guard Task.isCancelled != true else { - completion?() // Always resume continuation to prevent leak - return - } - deinitializeWindow() - completion?() + if !Task.isCancelled { + // Animation completed normally + } + // Always cleanup, even on cancellation, before resuming + deinitializeWindow() + completion?() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/Utility/NSScreen+Extensions.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
78-81: LGTM! Property is well-documented and backwards-compatible.The new
showCompactContentInExpandedModeproperty is clearly documented and defaults tofalse, ensuring existing clients are unaffected.Sources/DynamicNotchKit/Utility/NSScreen+Extensions.swift (1)
10-10: LGTM! Public exposure of NSScreen utilities is appropriate.Making the extension public allows clients to access notch detection and metric utilities, which aligns with the PR objectives. This is a purely additive change with no breaking impact.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'hybrid' layout mode, allowing compact content to remain visible in the expanded state, which is a great feature enhancement. The implementation is well-structured, especially the refactoring in NotchView.swift to support the new layout logic. The addition of documentation and tests is also appreciated. I've included one suggestion to improve the implementation of the continuation leak bug fix, making it more concise and maintainable. Overall, this is a solid contribution.
| closePanelTask?.cancel() | ||
| closePanelTask = Task { | ||
| try? await Task.sleep(for: .seconds(0.4)) // Wait for animation to complete | ||
| guard Task.isCancelled != true else { return } | ||
| guard Task.isCancelled != true else { | ||
| completion?() // Always resume continuation to prevent leak | ||
| return |
There was a problem hiding this comment.
While this correctly fixes the continuation leak, the call to completion?() is now duplicated. You can make this code more concise and avoid redundancy by using a single completion?() call at the end of the task block.
Additionally, comparing a boolean with != true is less idiomatic in Swift. It's clearer to use the ! operator for negation (i.e., !Task.isCancelled).
| closePanelTask?.cancel() | |
| closePanelTask = Task { | |
| try? await Task.sleep(for: .seconds(0.4)) // Wait for animation to complete | |
| guard Task.isCancelled != true else { return } | |
| guard Task.isCancelled != true else { | |
| completion?() // Always resume continuation to prevent leak | |
| return | |
| if !Task.isCancelled { | |
| deinitializeWindow() | |
| } | |
| completion?() |
- Add internal floatingHybridModeActive flag to avoid mutating user property - Add isHybridModeEnabled computed property for unified hybrid mode check - Update compact() to auto-enable hybrid mode on floating style - Add compact indicators overlay to NotchlessView for floating panels - Fix animation when transitioning to hybrid mode from expanded state - Add test cases for hybrid mode and floating fallback behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (1)
315-356: Consider adding an assertion for the floating fallback behavior.The comment at lines 352-353 mentions verifying that
showCompactContentInExpandedModeshould NOT be mutated, but there's no actual assertion. While the file states tests are "examples of usage," adding one assertion here would validate the fix for the previously flagged issue about mutating user configuration.🔎 Suggested assertion
// Verify: user-facing property should NOT be mutated // (floatingHybridModeActive is internal, showCompactContentInExpandedMode stays false) + #expect(notch.showCompactContentInExpandedMode == false, "User-facing property should not be mutated by compact()") await notch.hide() }Sources/DynamicNotchKit/Views/NotchlessView.swift (1)
62-83: Compact indicators overlay implemented correctly.The overlay mirrors the structure in
NotchView.compactContent()with appropriate environment injection and transitions. The scale anchors (.trailingfor leading content,.leadingfor trailing content) create a natural expand-from-center effect.One minor inconsistency: NotchView's
compactSideContentuses vertical safe area insets of 4pt top and 8pt bottom, while this uses equalsafeAreaInset(15pt) for top only. This may cause visual inconsistency between notch and floating hybrid modes.🔎 Consider aligning vertical insets with NotchView
.padding(.horizontal, safeAreaInset) - .padding(.top, safeAreaInset) + .padding(.top, 4)Alternatively, keep the current padding if the floating style intentionally differs from notch style.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/Views/NotchView.swiftSources/DynamicNotchKit/Views/NotchlessView.swiftTests/DynamicNotchKitTests/DynamicNotchKitTests.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/DynamicNotchKit/Views/NotchView.swift (2)
Sources/DynamicNotchKit/Views/NotchlessView.swift (1)
notchContent(46-61)Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (2)
compact(218-220)updateHoverState(163-173)
Sources/DynamicNotchKit/Views/NotchlessView.swift (2)
Sources/DynamicNotchKit/Views/NotchView.swift (1)
expandedContent(172-184)Sources/DynamicNotchKit/Utility/BlurModifier.swift (2)
blur(31-36)scale(38-43)
🔇 Additional comments (9)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (4)
27-29: Documentation updated correctly.The documentation now accurately reflects the new floating mode behavior where
compact()expands with hybrid mode enabled instead of hiding. This addresses the previous review feedback about outdated documentation.
105-119: Init parameter correctly wired.The new
showCompactContentInExpandedModeparameter is properly documented, defaulted tofalsefor backwards compatibility, and assigned in the initializer.
226-243: Floating mode hybrid behavior implemented correctly.The logic properly handles both cases:
- Already expanded → animate hybrid mode activation
- Hidden → enable hybrid mode and expand
Using the internal
floatingHybridModeActiveflag instead of mutating the public property addresses the previous review feedback.
311-317: Continuation leak fix applied correctly.The cleanup now always calls
deinitializeWindow()andcompletion?()regardless of cancellation state, addressing the previous review concern about the continuation being resumed before cleanup completed.Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (1)
271-313: Hybrid mode tests provide good usage examples.The test helper
_dynamicNotchHybridModeproperly demonstrates the hybrid mode workflow with explicitshowCompactContentInExpandedMode: trueinitialization. The tests cover both notch and floating styles.Sources/DynamicNotchKit/Views/NotchlessView.swift (1)
46-61: ZStack enables proper overlay composition for hybrid mode.The container change from
VStacktoZStack(alignment: .top)correctly allows the compact indicators to overlay the expanded content. The conditional rendering based onisHybridModeEnabledis clean.Sources/DynamicNotchKit/Views/NotchView.swift (3)
79-111: Well-documented layout logic for hybrid mode.The documentation clearly explains the two layout behaviors (normal vs hybrid). The computed flags
showCompactInExpandedanduseHybridLayoutmake the intent clear. The conditional frame constraints correctly handle the hybrid layout where compact content fills the expanded width.
113-170: Compact content visibility and layout refactored cleanly.The visibility logic correctly differentiates between hybrid mode (visible in non-hidden states) and normal mode (visible only in compact state). The
useSymmetricLayoutflag appropriately enables equal-width containers for hybrid expanded mode.Good use of
compactSideContenthelper to reduce duplication.
185-224: Well-structured helper for per-side compact content.The
compactSideContentfunction consolidates the rendering logic with proper parameterization for:
- Edge-specific padding (8pt normal, 15pt symmetric)
- Scale animation anchors
- Width tracking via binding
- Environment injection
The symmetric layout branch correctly uses
Spacerto push content to the outer edge while filling available width.
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift">
<violation number="1" location="Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift:234">
P2: Missing state reset: `floatingHybridModeActive` is set to `true` but never reset to `false`. This will cause hybrid mode to persist unexpectedly across `hide()` → `expand()` transitions. Add `floatingHybridModeActive = false` at the beginning of `_hide()` to properly clean up state.</violation>
<violation number="2" location="Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift:314">
P1: Race condition: removing the `Task.isCancelled` check allows `deinitializeWindow()` to run after cancellation, potentially closing a window that was just opened by `expand()` or `compact()`. The guard should skip `deinitializeWindow()` when cancelled but still call `completion?()` to prevent continuation leaks.</violation>
</file>
<file name="Sources/DynamicNotchKit/Views/NotchlessView.swift">
<violation number="1" location="Sources/DynamicNotchKit/Views/NotchlessView.swift:66">
P2: The `HStack` in `compactIndicatorsOverlay()` won't expand to fill the ZStack's width because a `Spacer`'s ideal width is 0. Without `.frame(maxWidth: .infinity)`, the compact indicators will cluster together instead of positioning at opposite corners. Compare with `NotchView.swift` which properly uses `.frame(maxWidth: .infinity)` in hybrid mode.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
- Add compactCenterContent property for center UI in floating fallback - Add compactCenter case to DynamicNotchSection enum - Update NotchlessView to show indicators row with center content - Remove top inset gap when in hybrid mode - Reset floatingHybridModeActive on hide to prevent state persistence 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
318-324: Race condition:deinitializeWindow()can close a newly opened window after task cancellation.When
expand()orcompact()cancelsclosePanelTaskand immediately callsinitializeWindow(), the cancelled task's sleep returns (error caught bytry?), then execution continues todeinitializeWindow(), closing the just-opened window.The
Task.isCancelledguard should be restored to skipdeinitializeWindow()when cancelled, while still callingcompletion?()to prevent continuation leaks:Proposed fix
closePanelTask = Task { try? await Task.sleep(for: .seconds(0.4)) // Wait for animation to complete - // Always cleanup and resume continuation, even on cancellation - deinitializeWindow() + if !Task.isCancelled { + deinitializeWindow() + } completion?() }
🧹 Nitpick comments (1)
Sources/DynamicNotchKit/Views/NotchlessView.swift (1)
79-82: Consider adding a conditional check for center content consistency.Leading and trailing content are conditionally rendered based on
disableCompactLeading/disableCompactTrailing, but center content is always rendered. While this works since the default isEmptyView, it's inconsistent. Consider adding adisableCompactCenterflag or conditionally rendering only whencompactCenterContentis non-empty for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/Utility/EnvironmentValues+Extensions.swiftSources/DynamicNotchKit/Views/NotchlessView.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Sources/DynamicNotchKit/Views/NotchlessView.swift (2)
Sources/DynamicNotchKit/Views/NotchView.swift (1)
expandedContent(172-184)Sources/DynamicNotchKit/Utility/BlurModifier.swift (2)
blur(31-36)scale(38-43)
🔇 Additional comments (7)
Sources/DynamicNotchKit/Utility/EnvironmentValues+Extensions.swift (1)
19-19: LGTM!The new
compactCentercase properly extends the enum to support the hybrid layout feature, aligning with thecompactCenterContentproperty inDynamicNotchand the environment bindings inNotchlessView.Sources/DynamicNotchKit/Views/NotchlessView.swift (1)
46-64: LGTM!The
VStacklayout correctly propagates the width fromexpandedContenttocompactIndicatorsRow(), so theSpacers will expand properly. The conditional top safe area inset logic correctly avoids double-spacing when the indicators row is present.Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (5)
27-29: LGTM!Documentation correctly reflects the new floating mode behavior where
compact(on:)expands with hybrid mode enabled instead of hiding.
79-95: LGTM!Good separation of concerns: the internal
floatingHybridModeActiveflag avoids mutating the user-facingshowCompactContentInExpandedModeproperty, addressing the previous review concern. The computedisHybridModeEnabledcleanly combines both sources.
113-130: LGTM!The new
showCompactContentInExpandedModeparameter defaults tofalse, maintaining backwards compatibility with existing callers.
229-247: LGTM!The floating mode fallback correctly uses the internal
floatingHybridModeActiveflag to avoid mutating user configuration. The fire-and-forgetTaskfor animation is appropriate here since it only triggers UI updates.
302-303: LGTM!Correctly resets
floatingHybridModeActiveat the start of_hide()to prevent hybrid mode from persisting across hide/expand cycles, addressing the previous review concern.
Remove intermediate hide step when transitioning from expanded to compact, eliminating the visual flash. Uses conversionAnimation for seamless morphing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Only call deinitializeWindow() if task wasn't cancelled, since a new window may have been opened by expand()/compact() after cancellation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift
🔇 Additional comments (7)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (7)
27-29: LGTM! Documentation accurately reflects the new floating mode behavior.The updated documentation clearly explains that floating-style devices will expand with hybrid mode when
compact()is called, showing compact indicators alongside expanded content.
79-86: LGTM! Well-designed public API additions.Both properties are properly documented and use safe defaults that maintain backwards compatibility.
88-95: LGTM! Clean separation of user configuration and internal state.The computed
isHybridModeEnabledproperty properly combines the user-facing setting with the internal floating mode flag, avoiding mutation of user configuration.
109-123: LGTM! Initializer properly extended with new parameter.The
showCompactContentInExpandedModeparameter is correctly added with a default value, maintaining backwards compatibility.
262-265: LGTM! Direct transition improves UX.The direct expanded → compact transition using
conversionAnimationeliminates the visual flash mentioned in the PR objectives.
293-294: LGTM! Proper state cleanup on hide.Resetting
floatingHybridModeActivewhen hiding prevents the flag from persisting across show/hide cycles.
312-317: LGTM! Correct cancellation handling.The implementation properly:
- Guards
deinitializeWindow()to prevent closing a window that was reopened after cancellation- Always calls
completion?()to prevent continuation leaks- Uses idiomatic
!Task.isCancelledcheck
Changed fire-and-forget Task to awaited MainActor.run and added sleep to wait for animation completion before compact() returns. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Allow clients to observe hover state changes via Combine to implement hover-based activation patterns (e.g., activate panel when mouse enters). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Thread Safety & Race Conditions: - Use structured concurrency with MainActor.run instead of unstructured Tasks - Make state checks atomic in floating mode logic - Add task cancellation on all early return paths Memory Management: - Track observeScreenParameters Task and cancel in deinit - Use weak self capture to prevent retain cycles - Only reinitialize window when state is hidden Animation & State Transitions: - Extract animationDuration constant from DynamicNotchStyle - Make compact<->expanded transitions symmetric (both direct) - Reset floatingHybridModeActive even when already expanded - Move flag reset after animation completes to prevent visual glitch API Improvements: - Make floatingHybridModeActive private(set) - Optimize redundant hybrid mode animations - Add max retry count for hover-blocked hide operations View Fixes: - Constrain compact indicators row height in NotchlessView Tests: - Add comprehensive assertions for state verification - Add tests for flag resets, computed property logic, rapid transitions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
203-234: Critical bug:floatingHybridModeActivereset breaks compact() flow from hidden state.Line 207 resets
floatingHybridModeActive = falsebefore the guard check at line 209, which breaks the floating fallback whencompact()is called from.hiddenstate:Scenario:
- Floating style, state is
.hidden- User calls
compact()_compact()line 274 setsfloatingHybridModeActive = true_compact()line 276 calls_expand()- Line 207 resets
floatingHybridModeActive = false❌- Line 209 guard checks
state != .expanded(state is.hidden, so continues)- Result: Flag is
false, but should betruefor hybrid mode to workThe existing test (
dynamicNotchFloatingFallback) doesn't catch this because it always callsexpand()first (line 377), so the state is already.expandedand the guard returns early at line 209 before any damage is done.🔎 Proposed fix
Option 1: Move the reset after the guard check so it only runs when actually expanding:
func _expand(on screen: NSScreen = NSScreen.screens[0], skipHide: Bool) async { - // Reset floating hybrid mode when explicitly expanding - // (compact indicators should only show when compact() is called) - // This must happen even if already expanded (e.g., after floating fallback compact()) - floatingHybridModeActive = false - guard state != .expanded else { closePanelTask?.cancel() + // Reset hybrid mode when already expanded (user called expand() again) + floatingHybridModeActive = false return } + + // Reset hybrid mode when transitioning from compact to expanded + floatingHybridModeActive = false closePanelTask?.cancel()Option 2: Add a parameter to skip the reset when called from
_compact():-func _expand(on screen: NSScreen = NSScreen.screens[0], skipHide: Bool) async { +func _expand(on screen: NSScreen = NSScreen.screens[0], skipHide: Bool, resetHybridMode: Bool = true) async { - floatingHybridModeActive = false + if resetHybridMode { + floatingHybridModeActive = false + }Then at line 276 in
_compact():-await _expand(on: screen, skipHide: skipHide) +await _expand(on: screen, skipHide: skipHide, resetHybridMode: false)Option 1 is simpler and doesn't add new parameters.
🧹 Nitpick comments (1)
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (1)
338-398: Add test coverage for compact() from hidden state on floating style.The test correctly verifies floating fallback when transitioning from expanded→compact (via line 377 expand() then line 386 compact()). However, there's an untested code path in
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftlines 272-277 wherecompact()is called from.hiddenstate on floating style.🔎 Suggested test case
Add a test that calls
compact()directly on a floating-style notch without callingexpand()first:@Test("DynamicNotch - Floating fallback from hidden state", .tags(.floatingStyle)) func dynamicNotchFloatingFallbackFromHidden() async throws { let notch = DynamicNotch(style: .floating) { Text("Test") } compactLeading: { Image(systemName: "circle") } compactTrailing: { Image(systemName: "square") } // Verify initial hidden state #expect(notch.state == .hidden) #expect(notch.floatingHybridModeActive == false) // Call compact() directly from hidden state await notch.compact() // Should auto-expand with hybrid mode enabled #expect(notch.state == .expanded, "Should expand from hidden state") #expect(notch.floatingHybridModeActive == true, "Hybrid mode should be active") #expect(notch.isHybridModeEnabled == true) await notch.hide() }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/DynamicNotch/DynamicNotchStyle.swiftSources/DynamicNotchKit/Views/NotchlessView.swiftTests/DynamicNotchKitTests/DynamicNotchKitTests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/DynamicNotchKit/DynamicNotch/DynamicNotchStyle.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/DynamicNotchKit/Views/NotchlessView.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (2)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (3)
expand(199-201)compact(236-238)hide(307-313)Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (3)
expand(104-111)compact(113-120)hide(122-124)
🔇 Additional comments (8)
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (3)
271-336: LGTM!The hybrid mode tests are well-structured with clear assertions for each state transition. The tests correctly verify that:
- Notch style transitions to compact state while keeping hybrid mode enabled
- Floating style remains expanded and auto-enables hybrid mode
- The
compactCenterContentproperty is properly exercised
400-450: LGTM!Excellent test coverage for the flag reset logic and computed property behavior. The tests correctly verify:
- Line 419:
expand()resetsfloatingHybridModeActive- Line 433: User setting takes precedence
- Line 447: User property
showCompactContentInExpandedModeis never mutated by internal logic
452-471: LGTM!Good stress test for rapid state transitions. This helps ensure the task cancellation logic in
_expand(),_compact(), and_hide()is robust and prevents crashes during quick state changes.Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (5)
27-29: LGTM!Documentation correctly reflects the new floating style behavior where
compact()enables hybrid mode instead of hiding the window. This addresses the past review feedback about updating the documentation.
79-129: LGTM!Property declarations and initialization are well-designed:
- Appropriate access control (
private(set)forfloatingHybridModeActiveprevents external mutation)- Clear separation between user-configurable (
showCompactContentInExpandedMode) and internal state (floatingHybridModeActive)- Computed
isHybridModeEnabledproperty correctly combines both flags with OR logic- Init parameter defaults to
falsefor backwards compatibility
160-179: LGTM!Excellent memory management and thread safety:
weak selfcapture prevents retain cycles in the long-lived notification observer- Task cancellation in
deinitensures clean cleanupMainActor.runensures UI state is accessed safely- Smart check to only reinitialize when hidden (line 166) prevents disrupting active animations
240-305: LGTM on thread safety and atomic state checks.The floating style logic uses proper synchronization:
- Lines 255-257: Atomically reads both
stateandfloatingHybridModeActiveto avoid race conditions- Line 261: Smart check to only animate if not already in hybrid mode (prevents redundant animations)
- Lines 262-266: Properly wraps state mutation in
MainActor.runwith animation- Lines 290-301: Consistent use of
MainActor.runfor notch style state changesNote: The critical bug with
_expand()resetting the flag (line 276 calling into line 207) is flagged in a separate comment.
319-363: LGTM!The
_hide()implementation correctly addresses all past review feedback:
- Lines 327-336: Retry logic with
maxHideRetries(50 × 0.1s = 5s timeout) prevents infinite loops when hover state is stuck- Lines 346-350:
do-catchblock handles task cancellation gracefully- Line 354: Resetting
floatingHybridModeActiveafter animation completes prevents visual glitch where indicators disappear before the window closes- Line 357-359: Only deinitializes if not cancelled, preventing race condition where a new window gets closed
- Line 361: Always calls
completion()to prevent continuation leaksThe comment on line 328-329 correctly explains the timeout safety mechanism.
…x code review issues (#2) * fix: preserve floatingHybridModeActive when compact() calls _expand() from hidden state The floatingHybridModeActive flag was being reset in _expand() before the guard check, which broke the floating fallback when compact() was called from .hidden state. The fix adds a resetHybridMode parameter to _expand() that defaults to true but is set to false when called from _compact(), preserving the hybrid mode flag set by the floating fallback logic. Also adds a test case to verify compact() works correctly from hidden state. * docs: add hybrid mode documentation to README and DocC Document the new hybrid mode feature: - README: Add Hybrid Mode section with code example and floating style behavior - DocC: Add Hybrid Mode section explaining the feature and automatic fallback * fix: improve floating hybrid mode reliability and documentation - Clarify README wording: indicators are "displayed when requested" not "always visible" - Fix icon sizing in floating hybrid mode by adding proper safeAreaInsets - Fix rapid state transition rendering by using opacity instead of conditional rendering - Add animation delay in test for state transition reliability - Apply swiftformat 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: prevent compact indicator layout compression during rapid transitions Add .fixedSize() and .layoutPriority(1) to compact leading/trailing content in floating view so SwiftUI preserves their intrinsic sizes even during rapid state changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Sources/DynamicNotchKit/Views/NotchlessView.swift">
<violation number="1" location="Sources/DynamicNotchKit/Views/NotchlessView.swift:78">
P2: Views hidden with `.opacity(0)` remain accessible to VoiceOver. Consider adding `.accessibilityHidden(dynamicNotch.disableCompactLeading)` to prevent hidden content from being announced to screen reader users.</violation>
<violation number="2" location="Sources/DynamicNotchKit/Views/NotchlessView.swift:95">
P2: Views hidden with `.opacity(0)` remain accessible to VoiceOver. Consider adding `.accessibilityHidden(dynamicNotch.disableCompactTrailing)` for consistency with leading content.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
203-203: Consider removing unusedskipHideparameter.The
skipHideparameter is declared with an underscore prefix (skipHide _:), explicitly marking it as unused. While this prevents compiler warnings, the parameter is threaded through the call chain (DynamicNotchInfo.compact()→_compact()→_expand()) but never actually used. If this parameter is no longer needed after refactoring, consider removing it from the signature to reduce confusion. If it's kept for API compatibility or future use, a comment explaining the intent would be helpful.🔎 Option 1: Remove if truly unused
- func _expand(on screen: NSScreen = NSScreen.screens[0], skipHide _: Bool, resetHybridMode: Bool = true) async { + func _expand(on screen: NSScreen = NSScreen.screens[0], resetHybridMode: Bool = true) async {Note: This would require updating callers in
_compact()and potentiallyDynamicNotchInfo.🔎 Option 2: Add clarifying comment if intentionally kept
+ // skipHide parameter kept for backwards compatibility with internal call sites func _expand(on screen: NSScreen = NSScreen.screens[0], skipHide _: Bool, resetHybridMode: Bool = true) async {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdSources/DynamicNotchKit/Documentation.docc/Documentation.mdSources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo+HelperViews.swiftSources/DynamicNotchKit/Views/NotchView.swiftSources/DynamicNotchKit/Views/NotchlessView.swiftTests/DynamicNotchKitTests/DynamicNotchKitTests.swift
🧰 Additional context used
🧬 Code graph analysis (4)
Sources/DynamicNotchKit/Views/NotchlessView.swift (2)
Sources/DynamicNotchKit/Views/NotchView.swift (1)
expandedContent(172-184)Sources/DynamicNotchKit/Utility/BlurModifier.swift (1)
blur(31-36)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
compact(113-120)
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (2)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (3)
expand(199-201)compact(239-241)hide(310-316)Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (3)
expand(104-111)compact(113-120)hide(122-124)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo+HelperViews.swift (1)
Sources/DynamicNotchKit/Utility/BlurModifier.swift (2)
body(13-16)body(24-27)
🔇 Additional comments (22)
README.md (1)
49-71: LGTM! Clear documentation for hybrid mode.The documentation clearly explains the hybrid layout concept with a practical code example and covers the floating style fallback behavior. This helps users understand when and how to use the feature.
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo+HelperViews.swift (3)
19-19: LGTM! Appropriate visibility reduction.Reducing the
bodyproperty visibility frompublicto internal is a good encapsulation practice for helper views that are implementation details.
37-37: LGTM! Consistent visibility change.Consistent with
CompactLeadingView, this helper view's body is correctly internalized.
52-52: LGTM! Helper view properly encapsulated.The
InfoViewbody is appropriately made internal, completing the visibility reduction for all helper views in this file.Sources/DynamicNotchKit/Views/NotchlessView.swift (3)
48-51: LGTM! Hybrid mode integration is correct.The conditional rendering of
compactIndicatorsRow()based onisHybridModeEnabledis the right approach for showing compact indicators alongside expanded content in floating mode.
55-58: LGTM! Proper safe area adjustment for hybrid mode.Suppressing the top inset when hybrid mode is active makes sense since the
compactIndicatorsRow()provides its own spacing. This avoids double-padding the top edge.
66-101: LGTM! Well-implemented compact indicators row.The implementation correctly:
- Uses opacity instead of conditional rendering to avoid SwiftUI layout issues
- Applies proper environment values for each section
- Uses fixed sizing and layout priority for stable layouts
- Includes appropriate transitions and padding
The approach of always rendering but hiding with opacity is a good pattern for avoiding SwiftUI's conditional rendering glitches.
Sources/DynamicNotchKit/Documentation.docc/Documentation.md (1)
17-22: LGTM! Consistent DocC documentation.The Hybrid Mode documentation is well-written and consistent with the README, properly explaining both the explicit flag-based activation and the automatic floating fallback behavior.
Sources/DynamicNotchKit/Views/NotchView.swift (3)
79-111: LGTM! Well-structured hybrid layout logic.The refactored
notchContent()correctly computes hybrid mode state and applies appropriate sizing constraints. The logic foruseHybridLayout(hybrid mode + expanded state) and the conditional.frame(maxWidth: .infinity)properly enables full-width compact content in hybrid mode while maintaining compact sizing in normal mode.
113-170: LGTM! Clean refactoring with proper visibility logic.The
compactContent()refactoring correctly:
- Determines visibility based on hybrid vs normal mode
- Uses symmetric layout for equal-width containers in hybrid expanded state
- Delegates to the new
compactSideContenthelper for cleaner code- Maintains width tracking for offset calculations
The visibility logic (
showContent) properly shows compact content in both states when hybrid mode is enabled, and only in compact state otherwise.
186-224: LGTM! Excellent helper abstraction.The
compactSideContenthelper cleanly encapsulates single-side rendering with:
- Proper environment value propagation
- Correct safe area insets (adaptive edge padding based on layout mode)
- Width measurement binding for offset calculations
- Appropriate transitions for animations
The symmetric layout creates equal-width containers with content aligned to outer edges, which is exactly what's needed for balanced hybrid mode indicators.
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (1)
271-500: LGTM! Comprehensive hybrid mode test coverage.The test suite thoroughly covers:
- Explicit hybrid mode activation for both notch and floating styles
- Automatic floating fallback behavior when
compact()is called- Proper flag reset on
expand()andhide()- Computed property logic for
isHybridModeEnabled- Edge case of
compact()from hidden state- Rapid state transitions for race condition testing
All tests properly verify state transitions, flag values, and behavior differences between notch and floating styles. The use of expectations and appropriate delays for animations ensures reliable test execution.
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (10)
27-29: Documentation accurately reflects new floating mode behavior.The updated documentation correctly describes that
compact(on:)on floating-style devices now expands with hybrid mode enabled, showing compact indicators alongside content rather than hiding the window.
79-96: Well-designed property separation for hybrid mode state.The separation between
showCompactContentInExpandedMode(user-facing) andfloatingHybridModeActive(internal) is a good design choice that prevents automatic floating fallback behavior from overwriting user configuration. TheisHybridModeEnabledcomputed property provides clean abstraction.
105-109: Retry limit prevents infinite loops in hover-blocking scenarios.The
maxHideRetrieslimit (50 retries × 0.1s = 5 seconds) is a sensible safety mechanism to prevent the hide operation from blocking indefinitely if hover state becomes stuck.
115-129: Init parameter addition maintains backwards compatibility.The
showCompactContentInExpandedModeparameter defaults tofalse, preserving existing behavior while enabling the new hybrid mode feature.
161-174: Screen parameter observation properly manages lifecycle and threading.The observer task correctly uses weak self to prevent retain cycles, checks state before reinitializing to avoid disrupting active animations, and ensures window initialization happens on MainActor.
176-179: Proper task cleanup in deinit.Both observer and close tasks are correctly cancelled during deinitialization, preventing resource leaks.
204-215: Hybrid mode reset logic correctly preserves state when needed.The
resetHybridModeparameter ensures thatfloatingHybridModeActiveis preserved when_expand()is called from_compact()'s floating fallback path, while resetting it for explicit expand calls. The early return with task cancellation is appropriate.
243-280: Floating mode hybrid logic is well-structured and race-safe.The implementation correctly handles the floating fallback by:
- Atomically checking state and hybrid mode status to avoid races
- Optimizing to skip redundant animations when already in hybrid mode
- Preserving the
floatingHybridModeActiveflag when calling_expand()viaresetHybridMode: falseThe atomic state read pattern (lines 258-260) followed by conditional state mutation (265-270) is appropriate since all state changes occur on MainActor.
293-307: Compact transition logic mirrors expand behavior.The direct state transition from expanded to compact (without intermediate hide) provides consistent animation behavior in both directions and improves UX by reducing visual flashing.
318-366: Hide implementation correctly handles hover retry and cancellation.The implementation properly addresses previous race conditions and continuation leaks:
- Retry logic with
maxHideRetrieslimit prevents infinite loops while respecting hover behaviorfloatingHybridModeActivereset occurs after animation to avoid visual glitchesdeinitializeWindow()is skipped when cancelled to prevent closing a newly created windowcompletion?()is always called to ensure continuation is resumedThe timeout mechanism (5 seconds) is a reasonable fallback if hover state becomes stuck.
- Remove unused skipHide parameter from _expand() and _compact() - Add .accessibilityHidden() to opacity-hidden views for VoiceOver - Fix rapid transition rendering by always rendering compact row (use frame height instead of conditional rendering to prevent SwiftUI view identity corruption) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
62-62: Remove unusedshouldSkipHideWhenConvertingflag.This flag is set on line 99 but is never read after the internal method calls were updated to use
_expandand_compact(which no longer utilize this parameter). This is dead code that should be removed.🔎 Proposed fix
- @Published var shouldSkipHideWhenConverting: Bool = falseAnd remove the assignment on line 99:
if let compactLeading { self.compactLeading = compactLeading } else { self.compactLeading = icon - self.shouldSkipHideWhenConverting = true }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swiftSources/DynamicNotchKit/Views/NotchlessView.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (3)
_expand(203-237)compact(239-241)_compact(243-308)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
compact(110-114)
🔇 Additional comments (10)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
107-107: Behavior is correct and intentional.The switch to internal
_expand(on:)and_compact(on:)methods is properly designed.DynamicNotchInfo.expand()correctly defaults toresetHybridMode=true, which resets the floating hybrid mode flag when explicitly called. This is intentional behavior documented in the code comments ("Reset floating hybrid mode when explicitly expanding"). The public API should reset hybrid mode for consistent user-facing behavior, while internal calls (like from within_compact) preserve the mode by passingresetHybridMode=false.Sources/DynamicNotchKit/Views/NotchlessView.swift (2)
48-59: LGTM: Clean approach to maintain view identity during transitions.The use of
.frame(height:)with.clipped()instead of conditional rendering avoids SwiftUI's identity issues during rapid state changes. The conditional top safe area inset correctly prevents double-spacing when the indicators row is visible.
70-103: LGTM: Indicators row implementation is complete and accessible.The method properly handles:
- Stable rendering with opacity-based visibility to avoid conditional rendering issues
- Accessibility via
.accessibilityHidden()for disabled content (addressing past feedback)- Proper environment values and spacing for all three content sections
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (7)
27-29: LGTM: Documentation updated to reflect new floating behavior.The documentation now correctly states that
compact()on floating devices expands with hybrid mode, addressing past feedback about the outdated "automatically hide" statement.
79-96: LGTM: Clean separation of user config and internal state.The dual-flag design (
showCompactContentInExpandedModefor user config,floatingHybridModeActivefor floating fallback) correctly avoids mutating user settings as a side effect, addressing past feedback.
104-109: LGTM: Proper task lifecycle management and timeout safety.The
screenObserverTasktracking enables proper cleanup in deinit, andmaxHideRetriesprovides a safety net against infinite retry loops (5-second timeout).
160-179: LGTM: Proper async lifecycle management.The screen observer correctly:
- Uses weak self to avoid retain cycles
- Only reinitializes when hidden to avoid disrupting animations
- Cancels tasks in deinit for proper cleanup
203-237: LGTM: Hybrid mode reset logic is well-designed.The
resetHybridModeparameter correctly handles two cases:
- Explicit
expand()calls reset hybrid mode (even if already expanded)- Calls from
_compact()preserve the hybrid flagThe pre-guard reset is intentional per the inline comment and ensures consistent behavior.
243-308: LGTM: Floating fallback and direct transitions are well-implemented.The logic correctly handles:
- Floating mode expanding with hybrid indicators (addressing past "mutating user config" issue)
- Atomic state reads to avoid race conditions
- Conditional animation only when needed (prevents redundant animations)
- Direct compact↔expanded transitions with consistent animation handling
310-366: LGTM: Robust hide logic with proper timeout and cancellation handling.The implementation correctly addresses all past feedback:
- Retry timeout prevents infinite loops (50 retries × 0.1s = 5s max)
- Cancellation handling avoids race conditions (only deinitializes if not cancelled)
- Continuation always resumed to prevent leaks
- Hybrid flag reset after animation prevents visual glitches
- Apply animation at VStack level for smooth container resize when hiding top row - Use explicit frame constraints for compact icons instead of fixedSize() - Disable matchedGeometryEffect in hybrid mode to prevent icon animation conflicts - Forward objectWillChange from internal DynamicNotch to DynamicNotchInfo - Reset shouldSkipHideWhenConverting when compactLeading is explicitly changed - Animate floatingHybridModeActive reset with conversionAnimation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Sources/DynamicNotchKit/Views/NotchlessView.swift">
<violation number="1" location="Sources/DynamicNotchKit/Views/NotchlessView.swift:78">
P3: Comment references removed code. The `4pt` and `8pt` insets mentioned were removed in this same diff (the `safeAreaInset` modifiers are gone). Consider updating the comment to reflect the actual purpose, e.g., "Leave visual margin within the row frame" or similar.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotchStyle.swift (1)
66-91: Good centralization of animation duration constant.The refactor to use
Self.standardAnimationDurationimproves maintainability. However, there's duplicated documentation between the private constant (lines 66-68) and the computed property (lines 87-88). Consider keeping the doc comment only on the public-facinganimationDurationproperty.🔎 Suggested simplification
- /// The duration of animations in seconds. - /// Used internally to coordinate async operations with animation completion. - /// This constant is used by all animation properties to ensure consistency. private static let standardAnimationDuration: TimeInterval = 0.4Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
117-127: Calling internal_expand/_compactbypasses public API.These methods now call the internal
_expand(on:)and_compact(on:)instead of the public wrappers. This is intentional to avoid double-wrapping, but it creates a coupling to internal implementation details. Consider whether exposing these internal methods is the right design, or if the forwarding should go through the public API.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swiftSources/DynamicNotchKit/DynamicNotch/DynamicNotchStyle.swiftSources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo+HelperViews.swiftSources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swiftSources/DynamicNotchKit/Utility/DynamicNotchControllable.swiftSources/DynamicNotchKit/Views/NotchlessView.swiftTests/DynamicNotchKitTests/DynamicNotchKitTests.swift
🧰 Additional context used
🧬 Code graph analysis (4)
Sources/DynamicNotchKit/Views/NotchlessView.swift (2)
Sources/DynamicNotchKit/Views/NotchView.swift (2)
notchContent(85-111)expandedContent(172-184)Sources/DynamicNotchKit/Utility/BlurModifier.swift (1)
blur(31-36)
Sources/DynamicNotchKit/Utility/DynamicNotchControllable.swift (2)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (2)
expand(198-200)compact(243-245)Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (2)
expand(117-121)compact(123-127)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (3)
_expand(202-241)compact(243-245)_compact(247-309)
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (1)
Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (2)
expand(117-121)compact(123-127)
🔇 Additional comments (12)
Sources/DynamicNotchKit/Utility/DynamicNotchControllable.swift (1)
21-29: Protocol signature updated to accept optionalNSScreen.The change from
NSScreentoNSScreen?aligns with the implementations inDynamicNotch.swiftandDynamicNotchInfo.swift, which provide default parameter values (= nil). The documentation correctly describes the fallback behavior.Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo.swift (1)
101-106: Good use of Combine to forwardobjectWillChangeevents.The sink pattern correctly propagates state changes from the internal
DynamicNotchto observers ofDynamicNotchInfo, ensuring views update when hybrid mode state changes. The cancellable is properly stored to prevent premature deallocation.Sources/DynamicNotchKit/Views/NotchlessView.swift (2)
46-49: Verify padding calculation inindicatorsRowHeight.The comment says "frame height + vertical padding" but the actual vertical padding in
compactIndicatorsRow()at line 112 is.padding(.vertical, 10), which adds 20pt total (10pt top + 10pt bottom). This matches the+ 20calculation, so the implementation is correct.
81-113: Well-structured hybrid mode indicators row.The implementation addresses previous review feedback:
accessibilityHiddenis correctly applied to opacity-hidden content (lines 92, 108)- The row uses explicit frame constraints for consistent icon sizing
- Center content is properly exposed via the environment
The animation at the container level (line 73) ensures smooth height transitions when toggling hybrid mode.
Tests/DynamicNotchKitTests/DynamicNotchKitTests.swift (3)
271-336: Comprehensive hybrid mode test coverage.The new tests thoroughly validate:
- User-configured hybrid mode (
showCompactContentInExpandedMode = true)- Style-specific behavior differences between notch and floating modes
- State assertions after each transition
Good practice including
compactCenterContentsetup to test the full hybrid mode layout.
340-398: Floating fallback test validates intentional behavior change.This test documents the key behavioral change:
compact()on floating style now expands with hybrid mode instead of hiding. The assertions clearly verify:
- User property (
showCompactContentInExpandedMode) is NOT mutated- Internal flag (
floatingHybridModeActive) IS set- Computed property (
isHybridModeEnabled) correctly reflects the combined state
481-500: Rapid state transition test provides regression coverage.This test helps catch race conditions and ensures the async state machine handles rapid transitions gracefully. Consider adding assertions for intermediate states if flakiness is observed.
Sources/DynamicNotchKit/DynamicNotch/DynamicNotch.swift (4)
80-97: Clean separation of user setting vs internal hybrid mode state.The design correctly separates:
showCompactContentInExpandedMode: User-configured public propertyfloatingHybridModeActive: Internal flag for floating fallbackisHybridModeEnabled: Computed property combining bothThis addresses the past review feedback about not mutating user configuration.
175-178: Good resource cleanup indeinit.Cancelling both
screenObserverTaskandclosePanelTaskprevents resource leaks and potential crashes from tasks running after deallocation.
319-369: Robust hide implementation with proper error handling.The implementation addresses all past review feedback:
- Hover retry limit (lines 331-340):
maxHideRetries = 50prevents infinite loops- Cancellation handling (lines 350-365): Explicitly catches
CancellationError, resetsfloatingHybridModeActiveafter animation, and only deinitializes if not cancelled- Continuation safety:
completion?()is always called to prevent leaksThe do-catch structure at lines 350-356 is slightly verbose but correctly handles the limited exception types from
Task.sleep.
265-278: Hybrid mode animation is now properly awaited.This addresses the past review feedback about fire-and-forget Tasks. The animation completion is awaited with
Task.sleepbefore returning, ensuring proper sequencing with subsequent operations.Sources/DynamicNotchKit/DynamicNotchInfo/DynamicNotchInfo+HelperViews.swift (1)
19-32: Force unwrap is safe given the current initialization order.The force unwrap of
internalDynamicNotchat line 20 (and line 59 inInfoView) is safe becauseinternalDynamicNotchis initialized on line 91 beforecompactLeadingandcompactTrailingare assigned (line 108+), which means the didSet observers execute only after initialization. View bodies are only rendered afterinitcompletes, ensuring the force unwrap never encounters nil.The hybrid mode logic for conditionally disabling
matchedGeometryEffectis well-designed with clear comments explaining the rationale.
- Fix race condition in closePanelTask that caused missing trailing icon on rapid transitions - Add explicit frame constraints for compact icons in NotchView (fixes gradient sizing in notch mode) - Update gradient test to skip compact() in floating mode (tested separately in hybrid mode tests) - Expose showCompactContentInExpandedMode parameter on DynamicNotchInfo - Remove duplicated doc comments in DynamicNotchStyle - Update comment to reflect actual purpose (removed inset references) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Adds "hybrid mode" layout support where compact leading/trailing indicators remain visible alongside expanded content. This enables use cases like audio players with playback indicators, dictation UIs with waveforms, or any interface where status icons should persist while showing detailed content.
New Features
Hybrid Mode (
showCompactContentInExpandedMode)falsefor backwards compatibility)Floating Style Fallback
compact()now enables hybrid mode and expands (instead of hiding)compactCenterContentproperty for optional center content in floating fallbackPublic API Additions
showCompactContentInExpandedMode: Bool- enable hybrid layoutscompactCenterContent: AnyView- center content for floating modeisHovering: Bool(read-only) - observe hover stateisHybridModeEnabled: Bool(computed) - check if hybrid mode is activehasNotch,notchSize,notchFrame,menubarHeight,screenWithMouse)Bug Fixes
closePanelTaskis cancelled.accessibilityHidden()to opacity-hidden views for VoiceOverCode Quality
skipHideparameter from internal methodsanimationDurationproperty to centralize timingTest Coverage
Backwards Compatibility
✅ Fully backwards compatible:
showCompactContentInExpandedModedefaults tofalsecompact()on floating style now expands with hybrid mode instead of hidinghide()explicitlyDocumentation