new screen recording hud update - #552
Conversation
|
@Ruken16 please drop the changes done to the and @coderabbitai please review everything |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 552 --repo Ebullioscopic/Atoll --json title,body,files,additions,deletions,changedFiles -q '.'
echo "-----FILES-----"
gh pr diff 552 --repo Ebullioscopic/Atoll --name-onlyLength of output: 2026 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 552 --repo Ebullioscopic/Atoll -- DynamicIsland/ContentView.swift DynamicIsland/DynamicIslandApp.swift DynamicIsland/components/Recording/RecordingLiveActivity.swift DynamicIsland/components/Settings/SettingsView.swift DynamicIsland/managers/ScreenRecordingManager.swift DynamicIsland/models/Constants.swiftLength of output: 693 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 552 --repo Ebullioscopic/Atoll > /tmp/pr552.diff
wc -l /tmp/pr552.diff
echo "---split by file---"
csplit -z -f /tmp/pr552_file_ -b '%02d.diff' /tmp/pr552.diff '/^diff --git/' '{*}' 2>/dev/null || true
ls /tmp/pr552_file_*.diffLength of output: 685 🏁 Script executed: #!/bin/bash
set -euo pipefail
awk '/^diff --git/{n++} {print > ("/tmp/pr552_part_" n ".diff")}' /tmp/pr552.diff
ls -la /tmp/pr552_part_*.diff
for f in /tmp/pr552_part_*.diff; do
echo "=== FILE: $f ==="
head -1 "$f"
doneLength of output: 2062 🏁 Script executed: #!/bin/bash
cat /tmp/pr552_part_2.diff
echo "=================================="
cat /tmp/pr552_part_3.diffLength of output: 10808 🏁 Script executed: #!/bin/bash
cat /tmp/pr552_part_5.diffLength of output: 11644 🏁 Script executed: #!/bin/bash
cat /tmp/pr552_part_7.diff
echo "===================="
cat /tmp/pr552_part_6.diff
echo "===================="
cat /tmp/pr552_part_8.diffLength of output: 13680 🏁 Script executed: #!/bin/bash
grep -rn "isRecorderIdle" --include=*.swift .
echo "---"
grep -rn "canStopFromHUD\|stopActiveRecordingOrSharing" --include=*.swift .Length of output: 2519
Here's my review of the rest of the PR: Summary of changes
Concerns
Nothing else stood out as blocking — the sizing/hover-state plumbing across ✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable screen-recording HUD styles and stop controls, integrates recording visibility with closed-notch sizing and gestures, synchronizes notch windows across Spaces, and adjusts LLM usage notch layout sizing and provider card presentation. ChangesRecording Hover Style & Stop Workflow
LLM Usage Notch Layout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RecordingLiveActivity
participant ContentView
participant ScreenRecordingManager
participant SystemEvents
User->>RecordingLiveActivity: hover recording HUD
RecordingLiveActivity->>ContentView: update hover and gesture bindings
ContentView->>ContentView: compute closed-notch size and gesture lock
User->>RecordingLiveActivity: select stop control
RecordingLiveActivity->>ScreenRecordingManager: stopActiveRecordingOrSharing()
ScreenRecordingManager->>SystemEvents: send stop shortcut
ScreenRecordingManager->>ScreenRecordingManager: refresh recording status
ScreenRecordingManager-->>RecordingLiveActivity: publish stop-request and failure state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DynamicIsland/ContentView.swift (1)
2133-2146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRe-check the recording HUD lock after the hover delay.
Line 2134 runs before the async sleep. If recording starts while the pointer is already hovering, the delayed task can still open the notch.
Proposed fix
await MainActor.run { guard self.vm.notchState == .closed, self.isHovering, + !self.recordingLiveActivityVisibleOnClosedNotch, !self.isSneakPeekVisibleOnCurrentScreen, !self.coordinator.isHoverOpenSuppressed else { return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/ContentView.swift` around lines 2133 - 2146, The hover-open flow in ContentView’s hoverTask should re-check the recording HUD lock after the sleep, not only before it. Update the delayed Task that follows the initial guard so it verifies the recording/locked state again inside the MainActor.run block before opening the notch, alongside the existing vm.notchState, isHovering, isSneakPeekVisibleOnCurrentScreen, and coordinator.isHoverOpenSuppressed checks.
🧹 Nitpick comments (3)
DynamicIsland/Localizable.xcstrings (1)
20248-20251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew localization keys lack
comment/isCommentAutoGeneratedmetadata.Other recently-added string entries in this file include a
commentdescribing UI context for translators. These two new keys ("Default uses the expanded recording HUD..." and "Recording hover style") have empty bodies. Not blocking, but consider adding comments for translator context consistency with the rest of the catalog.Also applies to: 48164-48167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/Localizable.xcstrings` around lines 20248 - 20251, The two new localization entries in DynamicIsland/Localizable.xcstrings are missing translator metadata, unlike nearby keys that include context comments. Update the entries for the recording HUD description and the “Recording hover style” key to include appropriate comment text and set isCommentAutoGenerated consistently with the surrounding catalog, using the same structure as other localization items in this file.DynamicIsland/components/Recording/RecordingLiveActivity.swift (1)
214-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate gesture handlers on stop buttons.
Both
stopButtonandinlineStopButtonattach.onTapGesture { ... }and.highPriorityGesture(TapGesture().onEnded { ... })invoking the identicalstopActiveRecordingOrSharing()call.highPriorityGesturealways wins gesture resolution, soonTapGesturehere is effectively dead code. Harmless today only becausestopActiveRecordingOrSharing()guards re-entrancy, but it's worth simplifying to one gesture handler per button.♻️ Proposed simplification (apply to both buttons)
.contentShape(Circle()) - .onTapGesture { - recordingManager.stopActiveRecordingOrSharing() - } - .highPriorityGesture( - TapGesture().onEnded { - recordingManager.stopActiveRecordingOrSharing() - } - ) + .highPriorityGesture( + TapGesture().onEnded { + recordingManager.stopActiveRecordingOrSharing() + } + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/components/Recording/RecordingLiveActivity.swift` around lines 214 - 260, Both stop button views, stopButton and inlineStopButton, attach two tap handlers that both call stopActiveRecordingOrSharing(), which is redundant because the highPriorityGesture already handles the tap. Simplify each view to a single gesture handler by removing the duplicate onTapGesture or the highPriorityGesture, keeping the one intended to win gesture resolution and still invoking recordingManager.stopActiveRecordingOrSharing().DynamicIsland/managers/ScreenRecordingManager.swift (1)
147-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate debug
print()statements behind#if DEBUGor use a logger.Numerous new
print()calls were added across the stop workflow (e.g. lines 153, 209, 211, 217, 235, 256, 310, 326) that will also run and spam Console.app in Release builds. Consider gating these behind#if DEBUGor switching toos.Logger, consistent with the concern already raised in review notes about debug artifacts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/managers/ScreenRecordingManager.swift` around lines 147 - 329, The stop-flow in ScreenRecordingManager is adding debug-only console noise, so gate the new print() calls in stopActiveRecordingOrSharing(), sendStopRecordingShortcutViaSystemEvents(), sendStopRecordingShortcut(), stopSystemScreenCaptureProcesses(), checkRecordingStatus(), refreshStopAvailability(), and isProcessRunning(named:) behind `#if` DEBUG or replace them with a Logger-based approach. Keep the existing behavior intact, but ensure release builds no longer emit these diagnostics or spam Console.app.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DynamicIsland.xcodeproj/project.pbxproj`:
- Around line 459-464: The shared project settings for the macOS target are
carrying environment-specific signing values that should not be committed here.
Remove the macOS-specific CODE_SIGN_IDENTITY override and the hard-coded
DEVELOPMENT_TEAM from the project.pbxproj entry for the affected
target/configuration, and move those signing settings into local or CI-specific
configuration instead. Keep the project file limited to general build settings
so Sparkle and release signing do not inherit an ad hoc identity from this
shared diff.
In `@DynamicIsland/ContentView.swift`:
- Around line 2324-2331: The scroll-open guard in ContentView’s gesture handling
is too broad because recordingOpenGestureLocked blocks the gesture even when the
recording HUD is not actually visible. Update the guard logic around the
gestureProgress reset path and the matching code at the other scroll-open check
so it only locks scroll-open when the recording HUD is currently shown, and make
sure it does not interfere with music pairing or the indicator setting.
- Around line 465-470: The recording HUD logic is ignoring the “Show Recording
Indicator” setting in multiple paths, so the indicator and related closed-notch
guards can still activate when it should be off. Update the visibility/guard
checks in recordingLiveActivityVisibleOnClosedNotch and the other recording HUD
paths referenced in the diff to include the Show Recording Indicator preference
alongside detection and recording state, so all recording UI paths consistently
honor that setting.
In `@DynamicIsland/DynamicIslandApp.swift`:
- Around line 405-427: The recording-HUD sizing branch in DynamicIslandApp’s
closed-notch layout currently expands the window for any active recording, even
when ContentView would not render the HUD. Update this condition to match the
actual visibility rules used by ContentView, including Show Recording Indicator
and the same music-pairing suppression, so the panel only grows when the
recording HUD can truly appear. Prefer extracting or reusing a shared visibility
helper instead of duplicating the predicate around vm.notchState,
ScreenRecordingManager.shared.isRecording, and Defaults checks.
In `@DynamicIsland/managers/ScreenRecordingManager.swift`:
- Around line 300-329: The synchronous process check in ScreenRecordingManager’s
isProcessRunning(named:) blocks the main thread because it calls Process.run()
followed by waitUntilExit() inside a `@MainActor` type. Move the pgrep/killall
work off the main actor using a background Task or DispatchQueue, then hop back
to `@MainActor` only to update canStopFromHUD and isRecording in
refreshStopAvailability(recording:) and the related status paths used by
updateDuration() and checkRecordingStatus().
- Around line 238-259: The fallback in stopSystemScreenCaptureProcesses is too
broad because it includes ScreenCaptureKitAgent, which can impact unrelated
apps’ screen-sharing sessions. Update the attempts list in
ScreenRecordingManager.stopSystemScreenCaptureProcesses to remove
ScreenCaptureKitAgent, or guard that killall path behind an explicit
confirmation/extra check so it only targets the native macOS recorder flow
already identified by isNativeMacOSScreenRecordingProcessActive.
- Around line 200-236: The stop-recording flow in ScreenRecordingManager
currently only prints errors from sendStopRecordingShortcutViaSystemEvents and
sendStopRecordingShortcut, then clears state even if stopping still failed.
Update the stop-request path to surface a visible error/alert when both shortcut
attempts fail and isRecording remains true, using the existing
ScreenRecordingManager stop helpers and the isSendingStopRequest/isRecording
state to keep the HUD from silently implying success.
---
Outside diff comments:
In `@DynamicIsland/ContentView.swift`:
- Around line 2133-2146: The hover-open flow in ContentView’s hoverTask should
re-check the recording HUD lock after the sleep, not only before it. Update the
delayed Task that follows the initial guard so it verifies the recording/locked
state again inside the MainActor.run block before opening the notch, alongside
the existing vm.notchState, isHovering, isSneakPeekVisibleOnCurrentScreen, and
coordinator.isHoverOpenSuppressed checks.
---
Nitpick comments:
In `@DynamicIsland/components/Recording/RecordingLiveActivity.swift`:
- Around line 214-260: Both stop button views, stopButton and inlineStopButton,
attach two tap handlers that both call stopActiveRecordingOrSharing(), which is
redundant because the highPriorityGesture already handles the tap. Simplify each
view to a single gesture handler by removing the duplicate onTapGesture or the
highPriorityGesture, keeping the one intended to win gesture resolution and
still invoking recordingManager.stopActiveRecordingOrSharing().
In `@DynamicIsland/Localizable.xcstrings`:
- Around line 20248-20251: The two new localization entries in
DynamicIsland/Localizable.xcstrings are missing translator metadata, unlike
nearby keys that include context comments. Update the entries for the recording
HUD description and the “Recording hover style” key to include appropriate
comment text and set isCommentAutoGenerated consistently with the surrounding
catalog, using the same structure as other localization items in this file.
In `@DynamicIsland/managers/ScreenRecordingManager.swift`:
- Around line 147-329: The stop-flow in ScreenRecordingManager is adding
debug-only console noise, so gate the new print() calls in
stopActiveRecordingOrSharing(), sendStopRecordingShortcutViaSystemEvents(),
sendStopRecordingShortcut(), stopSystemScreenCaptureProcesses(),
checkRecordingStatus(), refreshStopAvailability(), and isProcessRunning(named:)
behind `#if` DEBUG or replace them with a Logger-based approach. Keep the existing
behavior intact, but ensure release builds no longer emit these diagnostics or
spam Console.app.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c3da2ad-5594-47f5-8a78-0d16bd3f8b9c
📒 Files selected for processing (8)
DynamicIsland.xcodeproj/project.pbxprojDynamicIsland/ContentView.swiftDynamicIsland/DynamicIslandApp.swiftDynamicIsland/Localizable.xcstringsDynamicIsland/components/Recording/RecordingLiveActivity.swiftDynamicIsland/components/Settings/SettingsView.swiftDynamicIsland/managers/ScreenRecordingManager.swiftDynamicIsland/models/Constants.swift
| "CODE_SIGN_IDENTITY[sdk=macosx*]" = "-"; | ||
| CODE_SIGN_STYLE = Automatic; | ||
| COMBINE_HIDPI_IMAGES = YES; | ||
| CURRENT_PROJECT_VERSION = 12; | ||
| DEVELOPMENT_ASSET_PATHS = "\"DynamicIsland/Preview Content\""; | ||
| DEVELOPMENT_TEAM = 9Y64TRM77N; | ||
| DEVELOPMENT_TEAM = RPC964BP8A; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the effective signing settings without building.
xcodebuild -project DynamicIsland.xcodeproj \
-target DynamicIsland \
-configuration Release \
-showBuildSettings 2>/dev/null |
awk '/CODE_SIGN_IDENTITY|EXPANDED_CODE_SIGN_IDENTITY|DEVELOPMENT_TEAM|CODE_SIGN_STYLE/'Repository: Ebullioscopic/Atoll
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant pbxproj sections and any signing-related settings.
FILE="DynamicIsland.xcodeproj/project.pbxproj"
echo "== Relevant line numbers =="
rg -n 'CODE_SIGN_IDENTITY|DEVELOPMENT_TEAM|CODE_SIGN_STYLE|EXPANDED_CODE_SIGN_IDENTITY|Sparkle|CODE_SIGN' "$FILE"
echo
echo "== Surrounding context around the reported lines =="
sed -n '440,475p' "$FILE"
echo
sed -n '520,550p' "$FILE"
echo
echo "== Project-wide signing-related search =="
rg -n 'PROVISIONING_PROFILE|CODE_SIGN_ALLOW|CODE_SIGN_IDENTITY\[sdk=macosx\*\]|DEVELOPMENT_TEAM\[sdk=macosx\*\]|CODE_SIGN_STYLE' "$FILE"Repository: Ebullioscopic/Atoll
Length of output: 4906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Try to inspect the effective build settings if Xcode is available.
# This is read-only and does not build the project.
xcodebuild -project DynamicIsland.xcodeproj \
-target DynamicIsland \
-configuration Release \
-showBuildSettingsRepository: Ebullioscopic/Atoll
Length of output: 205
Keep the macOS signing overrides out of the shared project file. CODE_SIGN_IDENTITY[sdk=macosx*] = "-" makes the macOS Release build ad hoc-signed, and the Sparkle re-sign step uses that same identity via $EXPANDED_CODE_SIGN_IDENTITY. The hard-coded DEVELOPMENT_TEAM also bakes environment-specific signing into the feature diff. Move these settings to local/CI config unless this PR is meant to change release signing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DynamicIsland.xcodeproj/project.pbxproj` around lines 459 - 464, The shared
project settings for the macOS target are carrying environment-specific signing
values that should not be committed here. Remove the macOS-specific
CODE_SIGN_IDENTITY override and the hard-coded DEVELOPMENT_TEAM from the
project.pbxproj entry for the affected target/configuration, and move those
signing settings into local or CI-specific configuration instead. Keep the
project file limited to general build settings so Sparkle and release signing do
not inherit an ad hoc identity from this shared diff.
|
@Ruken16 Need to perform some changes:
|
|
The PR description image shows the "Screen Recording" settings panel: a dark-themed card with "Enable Screen Recording Detection" (toggle on), "Show Recording Indicator" (toggle on), "Recording hover style" with a segmented control ("Predefinito" / "In linea", with "In linea" selected in blue) and caption "Default uses the expanded recording HUD. Inline keeps the stop control inside the notch height.", a "Detection Status" row showing "Active - No Recording" in green, and a footer note "Uses event-driven private API for real-time screen recording detection."
On your points 1–3 (settings granularity, floating stop button + multi-live-activity support for recording Atoll itself, and gating the ✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DynamicIsland/components/Notch/NotchLLMUsageView.swift (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed card height may clip richer content states.
llmUsageProviderCardHeight(164) is applied uniformly across loading/failure/success states, including the success branch with two quota gauges plus a compact window pair (Lines 69-76). If a provider ever reports both session and week limits, the stackedquotaGauge+windowviews could exceed the fixed height. Worth a visual sanity check with a provider that has both limits populated.Also applies to: 42-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/components/Notch/NotchLLMUsageView.swift` around lines 27 - 29, The fixed `llmUsageProviderCardHeight` used by `NotchLLMUsageView` may be too small for the success layout when both session and week quotas are shown. Review the `body` success branch and the `quotaGauge`/`window` stacking to either increase or make the card height adaptive so richer provider states are not clipped, and verify the rendering with a provider that has both limits populated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@DynamicIsland/components/Notch/NotchLLMUsageView.swift`:
- Around line 27-29: The fixed `llmUsageProviderCardHeight` used by
`NotchLLMUsageView` may be too small for the success layout when both session
and week quotas are shown. Review the `body` success branch and the
`quotaGauge`/`window` stacking to either increase or make the card height
adaptive so richer provider states are not clipped, and verify the rendering
with a provider that has both limits populated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd1e8c28-8c77-4d02-a506-2ec1372f139f
📒 Files selected for processing (10)
DynamicIsland/ContentView.swiftDynamicIsland/DynamicIslandApp.swiftDynamicIsland/components/Notch/NotchLLMUsageView.swiftDynamicIsland/components/Recording/RecordingLiveActivity.swiftDynamicIsland/components/Settings/SettingsView.swiftDynamicIsland/components/Tabs/TabSelectionView.swiftDynamicIsland/managers/ScreenRecordingManager.swiftDynamicIsland/models/Constants.swiftDynamicIsland/models/DynamicIslandViewModel.swiftDynamicIsland/sizing/matters.swift
✅ Files skipped from review due to trivial changes (1)
- DynamicIsland/components/Tabs/TabSelectionView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- DynamicIsland/DynamicIslandApp.swift
- DynamicIsland/components/Recording/RecordingLiveActivity.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DynamicIsland/DynamicIslandApp.swift (1)
532-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the music-pairing eligibility helper
closedMusicPairingEligibleForSizing()still mirrorsContentView.closedMusicPairingEligiblewith the same gating (closedMusicContentEnabled/shouldDelayPostUnlockMusicHUDequivalents). Extract this into a shared visibility helper so the sizing path and view logic can’t drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/DynamicIslandApp.swift` around lines 532 - 545, The music-pairing eligibility logic in closedMusicPairingEligibleForSizing() is duplicated from the ContentView closed-state visibility check and can drift over time. Extract the shared gating into a common helper used by both closedMusicPairingEligibleForSizing() and ContentView.closedMusicPairingEligible, keeping the same conditions around MusicManager, coordinator.musicLiveActivityEnabled, Defaults flags, vm.hideOnClosed, and LockScreenManager state so both paths stay aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@DynamicIsland/DynamicIslandApp.swift`:
- Around line 532-545: The music-pairing eligibility logic in
closedMusicPairingEligibleForSizing() is duplicated from the ContentView
closed-state visibility check and can drift over time. Extract the shared gating
into a common helper used by both closedMusicPairingEligibleForSizing() and
ContentView.closedMusicPairingEligible, keeping the same conditions around
MusicManager, coordinator.musicLiveActivityEnabled, Defaults flags,
vm.hideOnClosed, and LockScreenManager state so both paths stay aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7970d544-dc2a-4eb1-9416-73f735ea6e9f
📒 Files selected for processing (1)
DynamicIsland/DynamicIslandApp.swift
StudioKeys
left a comment
StudioKeys
left a comment
There was a problem hiding this comment.
Appreciate the effort and the implementation @Ruken16, but this needs some changes
- Code Complexity Explosion
Issue: The 80-line recording indicator expanded to 400+ lines with 3 nearly identical HUD variants.
Specific Problems:
a. Three HUD variants (compactRecordingHUD, inlineRecordingHUD, expandedRecordingHUD) with ~90% shared code
b. 8+ overlapping state flags instead of clean state management
c. Six different async tasks running simultaneously
- Architectural Anti-Patterns
Issue: Copy-paste programming and mixed responsibilities.
Specific Problems:
a. Duplicate PulsingModifier between RecordingLiveActivity and PrivacyLiveActivity
b. ScreenRecordingManager handles monitoring, stopping, AND process killing
c. Hard-to-test coupled logic
- Performance Risks
Issue: Animation thrash and timer overhead during recording.
Specific Problems:
a. Constant switching between three view layouts
b. Multiple timers: durationTimer, stopAvailabilityRefreshInterval, and task chains
c. Background killall and pgrep calls
- Backward Compatibility
Issue: Changes to notch behavior and window attributes.
Specific Problems:
a. Removed .never hide option from CGSSpace membership
b. Added .utilityWindow, .hudWindow style masks
c. Window now always joins spaces (previously conditional)
Hope these will be looked upon and addressed!
Cheers
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DynamicIsland/components/Recording/RecordingLiveActivity.swift (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecording HUD extra-size constants (132/140/176/70) are duplicated across three files. Any future tweak to one of these layout numbers is easy to miss in the other two, causing sizing drift between the actual
RecordingLiveActivityview and the window/notch sizing computed independently inContentViewandDynamicIslandApp.
DynamicIsland/components/Recording/RecordingLiveActivity.swift#L218-237: keepRecordingHUDPresentationas the single source of truth for these values (already the most centralized definition).DynamicIsland/ContentView.swift#L519-531: reuseRecordingHUDPresentation's width/height instead of re-declaring132/140/176/70inrecordingHUDExtraWidth/recordingHUDExtraHeight.DynamicIsland/DynamicIslandApp.swift#L463-483: reuse the same shared source instead of the inline closures re-encoding132/140/176/70.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DynamicIsland/components/Recording/RecordingLiveActivity.swift` at line 1, Centralize the recording HUD sizing constants in RecordingHUDPresentation and remove duplicate numeric values from ContentView.recordingHUDExtraWidth/recordingHUDExtraHeight and DynamicIslandApp’s inline sizing closures. Reuse RecordingHUDPresentation’s shared width and height values so the live activity view and window/notch sizing remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@DynamicIsland/components/Recording/RecordingLiveActivity.swift`:
- Line 1: Centralize the recording HUD sizing constants in
RecordingHUDPresentation and remove duplicate numeric values from
ContentView.recordingHUDExtraWidth/recordingHUDExtraHeight and
DynamicIslandApp’s inline sizing closures. Reuse RecordingHUDPresentation’s
shared width and height values so the live activity view and window/notch sizing
remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbeba5e9-d41d-431c-b01c-b627fbaca2ed
📒 Files selected for processing (7)
DynamicIsland/ContentView.swiftDynamicIsland/DynamicIslandApp.swiftDynamicIsland/components/Live activities/PulsingModifier.swiftDynamicIsland/components/Notch/DynamicIslandWindow.swiftDynamicIsland/components/Notch/NotchLLMUsageView.swiftDynamicIsland/components/Recording/RecordingLiveActivity.swiftDynamicIsland/components/Settings/SettingsView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- DynamicIsland/components/Notch/NotchLLMUsageView.swift
- DynamicIsland/components/Settings/SettingsView.swift
StudioKeys
left a comment
StudioKeys
left a comment
There was a problem hiding this comment.
Took the time to go through the changes which were updated and kept, have some stuff that still needs attention
- Add comment warning about private API usage in ScreenRecordingManager.swift just for maintainability by other contributors in future
- Consider extracting shared recording HUD visibility/sizing logic to avoid duplication between ContentView and AppDelegate
- Verify space membership behavior change is intentional (the .never hide option gating was removed)
Others than these, rest looks all good from my end :)
|
Solid work on the Screen Recording HUD and the Space pinning fix — both are genuinely useful improvements. That said, there are a few things that need to land before this can merge: Must fix (blockers):
Should fix:
Nits:
Good stuff:
CI is green, but the branch is Happy to pair on the blocker fixes if useful. |
3 participants
Registrazione.schermo.Lug.1.2026.mov
Registrazione.schermo.Lug.01.2026.mov
Summary by CodeRabbit
New Features
Bug Fixes