Feature: System-Wide Audio Recording Option#9
Conversation
|
Great work. Will review soon! |
|
@rawandahmad698 @dennisimoo, is there a chance that it can be merged? |
|
@wobondar yes. Sorry. I have been really busy. Will review certainly |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a system-wide audio recording feature that allows capturing audio from all applications simultaneously, rather than just a specific application. This addresses use cases where users need to record multiple audio sources during meetings or presentations.
- Adds "All Apps" option to the app selection dropdown using a special ID of -1
- Implements
SystemWideTapandSystemWideTapRecorderclasses using Core Audio's global tap APIs - Updates the recording pipeline to handle both process-specific and system-wide audio capture
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| AppSelectionViewModel.swift | Prepends "All Apps" option to available apps list |
| AppSelectionDropdown.swift | Adds dedicated UI row for system-wide recording option |
| RecordingConfiguration.swift | Updates application name handling for system-wide mode |
| RecordingSessionManager.swift | Implements branching logic for system-wide vs process-specific recording |
| AudioRecordingCoordinator.swift | Extends coordinator to support both tap types with unified interface |
| SelectableApp.swift | Adds system-wide app representation with special ID -1 |
| AudioProcess.swift | Adds commented placeholders for future system case |
| SystemWideTap.swift | New implementation of system-wide audio capture using Core Audio APIs |
| ProcessTap.swift | Adds protocol conformance for unified tap interface |
| AudioTapType.swift | New protocol definitions for audio tap abstraction |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| try tap.invalidate() | ||
| } catch { | ||
| logger.error("Stop failed: \(error, privacy: .public)") | ||
| } |
There was a problem hiding this comment.
The tap property is computed and calls invalidate() on the SystemWideTap instance, but invalidate() doesn't throw. This try is unnecessary and the method signature should be updated.
| } | |
| logger.debug(#function) | |
| guard isRecording else { return } | |
| currentFile = nil | |
| isRecording = false | |
| tap.invalidate() |
| if let systemWideTap = systemWideTap { | ||
| await MainActor.run { | ||
| systemWideTap.activate() | ||
| } |
There was a problem hiding this comment.
The systemWideTap.activate() is called twice - once at line 33 and again at line 59. This redundant activation could cause issues or unexpected behavior.
| } |
| let isMeetingApp: Bool | ||
| let isAudioActive: Bool | ||
| private let originalAudioProcess: AudioProcess | ||
| let isSystemWide: Bool |
There was a problem hiding this comment.
[nitpick] The isSystemWide property is redundant since it can be derived from checking if id == -1. Consider removing this property and using a computed property instead.
| let isSystemWide: Bool |
| self.originalAudioProcess = audioProcess | ||
| } | ||
|
|
||
| private init(systemWide: Bool) { |
There was a problem hiding this comment.
The systemWide parameter in this initializer is always true when called. Consider removing the parameter and making this a parameterless private initializer.
| private init(systemWide: Bool) { | |
| private init() { |
| @ObservationIgnored | ||
| private(set) var tapStreamDescription: AudioStreamBasicDescription? | ||
| @ObservationIgnored | ||
| private var invalidationHandler: InvalidationHandler? |
There was a problem hiding this comment.
[nitpick] Multiple @ObservationIgnored annotations could be grouped together for better readability, or consider using a single comment explaining why these properties are ignored.
| private var invalidationHandler: InvalidationHandler? | |
| // The following properties are ignored for observation because they are internal implementation details | |
| // and do not affect the observable state of the SystemWideTap object. | |
| @ObservationIgnored private var processTapID: AudioObjectID = .unknown | |
| @ObservationIgnored private var aggregateDeviceID = AudioObjectID.unknown | |
| @ObservationIgnored private var deviceProcID: AudioDeviceIOProcID? | |
| @ObservationIgnored private(set) var tapStreamDescription: AudioStreamBasicDescription? | |
| @ObservationIgnored private var invalidationHandler: InvalidationHandler? |
|
@rawandahmad698 @dennisimoo, have you had a chance to review? If it requires fixes/changes - don't hesitate, just request changes or assign it back to me. |
|
I have been testing this and I wanted to share the test outcome The system recording is an empty file! Also, when I switch the output device while the recording is in progress, these logs appear |
|
I am also still testing this. Will let you know |
3 participants
Description
Feature: System-Wide Audio Recording Option
Adds the ability to record System audio from all applications, not just a specific one.
Q: Why is this required in the first place?
A: Often, I ran meetings and, for example, do "show and tell" at the same time during the meeting from multiple audio sources(apps), so i wanted Recap to record all system audio.
How it works
Demo for screenshots below
Important bits
Type of Change
Testing
Checklist
Screenshots (if applicable)
Default view
App selector
Selected "All Apps"
Recording
Summary Transcribing
Summarizing
Final Summary
Additional Notes
Everything I've already explained above.