[Agent] refactor(recording): @MainActor, Swift 6 async/await, single source of truth#2860
Conversation
github-actions
bot
added
agent-work
|
🤖 PR created. AI review starting automatically. |
github-actions
bot
force-pushed
the
agent/recording-mainactor
branch
from
d380a5a to
9f6456b
Compare
|
|
github-actions
bot
force-pushed
the
agent/recording-mainactor
branch
2 times, most recently
from
5a19e32 to
6d3919f
Compare
|
PR 2860 approved - ready for human review. JoeMatt assigned. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review Cycle 1/3 - Reviewer: claude-opus-4-6 - APPROVE - Well-structured refactor. No CRITICAL or MAJOR issues. MINOR: PreviewDelegate nested class at PVRecordingManager.swift:115 should be annotated @mainactor for Swift 6 strict concurrency (it calls dismiss which is MainActor-isolated). NIT: Task { @mainactor in } wrappers in PVEmulatorViewController+Recording.swift:31,47 are redundant since UIViewController is already MainActor.
|
No description provided. |
github-actions
bot
added
ready-for-review
github-actions
bot
force-pushed
the
agent/recording-mainactor
branch
from
6d3919f to
efbf5cd
Compare
✅ Ready for Human ReviewCompleted
Completed: 2026-03-10 02:07 UTC |
There was a problem hiding this comment.
AI Code Review - Cycle 1/3
Reviewer: claude-opus-4-6
Clean refactor that adds @mainactor isolation to PVRecordingManager, converts the completion-handler API to async throws, extracts RPPreviewViewControllerDelegate into a private PreviewDelegate inner class, and establishes AppState.shared.emulationUIState.isRecording as the single source of truth for SwiftUI. The changes are well-scoped and introduce no new safety issues. CRITICAL - none. MAJOR - none. MINOR - In stopRecording(presenter:), if the RPScreenRecorder.stopRecording callback returns an error, PVRecordingManager.isRecording remains true (old code always reset to false). This is arguably better behavior (allows retry) and the VC layer correctly resets AppState.isRecording on both paths. NIT - none. Verdict: APPROVE
|
No description provided. |
github-actions
bot
force-pushed
the
agent/recording-mainactor
branch
from
efbf5cd to
6612ed0
Compare
github-actions
bot
added
ai-reviewing
github-actions
bot
force-pushed
the
agent/recording-mainactor
branch
from
6612ed0 to
b7a1c4a
Compare
There was a problem hiding this comment.
AI Code Review Cycle 1/3 - Reviewer: claude-opus-4-6. Clean refactoring: @mainactor isolation on PVRecordingManager, async throws API, extracted PreviewDelegate, single source of truth via AppState. No CRITICAL or MAJOR issues. MINOR: discardScreenRecording sets AppState synchronously while RPScreenRecorder cleanup is async (not harmful, worth a comment). NIT: RECORDING.md could live in docs/. Verdict: APPROVE
|
No description provided. |
github-actions
bot
force-pushed
the
agent/recording-mainactor
branch
from
b7a1c4a to
f5ea97d
Compare
- Annotate PVRecordingManager with @mainactor for Swift 6 compile-time safety - Convert startRecording/stopRecording to async throws, removing DispatchQueue.main.async - Extract RPPreviewViewControllerDelegate to private PreviewDelegate (removes NSObject inheritance) - Update PVEmulatorViewController+Recording to use Task { @mainactor in try await ... } - Add discardScreenRecording() to VC extension so AppState stays consistent on discard - Fix RetroMenuView.recordingButton to read from AppState (single source of truth) - Add RECORDING.md with architecture notes and rules for future agents Addresses AI review feedback on #2752 (MAJOR/MINOR/NIT items) Part of #2714 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
force-pushed
the
agent/recording-mainactor
branch
from
f5ea97d to
d3bfae5
Compare
1 participant
Summary
Addresses all AI review feedback from #2752 in a single focused refactor:
PVRecordingManagerannotated@MainActorfor Swift 6 compile-time concurrency safety.startRecording()andstopRecording(presenter:)converted toasync throws, replacingDispatchQueue.main.asyncwith direct assignment.RPPreviewViewControllerDelegateextracted to a privatePreviewDelegate: NSObjectinner class, removingNSObjectinheritance fromPVRecordingManageritself.RetroMenuView.recordingButtonnow reads fromAppState.shared.emulationUIState.isRecording(the SwiftUI-observable flag) instead ofPVRecordingManager.shared.isRecordingdirectly.discardScreenRecording()toPVEmulatorViewController+Recording— the canonical call site that also updatesAppState, keeping all recording state changes consistent.RECORDING.mdwith architecture notes and rules for future agents/contributors.Changes
PVRecordingManager.swift—@MainActor,async throwsAPI,PreviewDelegateinner classPVEmulatorViewController+Recording.swift—Task { @MainActor in try await ... }call sites, newdiscardScreenRecording()RetroMenuView.swift— read recording state fromAppState(single source of truth)RECORDING.md— architecture documentationDesign
AppState.shared.emulationUIState.isRecordingis the single SwiftUI-observable source of truth for recording state.PVRecordingManager.isRecordingis the manager's internal guard only (prevents double-start/stop). The VC extension is the only layer that updates AppState.Depends on: #2752
Part of #2714
Generated with Claude Code