fix: act on PR #34 Copilot review comments#35
Merged
Conversation
1. IAppConfiguration.AudioDeviceId doc: clarify it is a Core Audio endpoint ID string (opaque), not a plain GUID 2. VolumeWatcherService: rename _defaultDevice -> _monitoredDevice to better reflect semantics after audio device selector change 3. VolumeWatcherService: extract device resolution into internal static ResolveMonitoredDevice() with delegate injection for testability 4. SettingsManager: omit/remove AudioDeviceId key from JSON when value is empty (null/empty = Windows default, key should not be written) 5. Tests: 6 new [Fact] tests for ResolveMonitoredDevice covering all branches (configured device, empty/null -> default, COMException fallback with warning, log kind assertions); no COM/Windows required for these tests 6. SettingsManagerTests: update assertions to expect AudioDeviceId key omitted when empty; add test for reverting to default (key removal)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR follows up on PR #34 review feedback by refining audio-device selection semantics, improving testability of device resolution, and adjusting settings persistence to better represent “Windows default” behavior.
Changes:
- Renames
VolumeWatcherServicefield_defaultDeviceto_monitoredDeviceand extracts device selection intointernal static ResolveMonitoredDevice(...). - Updates
SettingsManager.SaveSettings()to omit/removeHomeAssistant:AudioDeviceIdfrom JSON when empty (default device). - Updates/adds tests for the new device resolution logic and the updated settings persistence behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/VolumeWatcherServiceTests.cs | Adds tests for ResolveMonitoredDevice routing/logging behavior. |
| tests/SettingsManagerTests.cs | Updates expectations for omitted AudioDeviceId key and adds a removal test. |
| src/VolumeWatcherService.cs | Renames monitored device field and introduces ResolveMonitoredDevice helper used by ExecuteAsync. |
| src/SettingsManager.cs | Omits/removes AudioDeviceId key when empty to represent “use Windows default”. |
| src/IAppConfiguration.cs | Clarifies XML doc wording for AudioDeviceId as an opaque Core Audio endpoint ID string. |
1. Rename test: SaveSettings_OmitsAudioDeviceId_WhenNotProvided (was 'Saves') 2. VolumeWatcherService.ResolveMonitoredDevice: update delegates to MMDevice? nullable; use Copilot-suggested null-safe pattern (device is not null guard before FriendlyName; COMException falls through to getDefaultDevice) 3. Update tests to remove stubDevice! null-forgiveness; update routing assertions to match new null-safe fallback semantics; add comments explaining why logInfo logging tests cannot assert 'configured'/'default' kind without a real COM MMDevice (integration-tested on Windows CI)
1. ResolveMonitoredDevice made generic (TDevice) with a getName delegate so
tests can use string stubs instead of COM MMDevice — fully cross-platform
2. All ResolveMonitoredDevice tests rewritten using string-typed stubs:
- routing tests now return real non-null values and assert correctly
- LogsConfiguredKind and LogsDefaultKind now properly assert 'configured'
and 'default' log kinds
- 3 new tests: ReturnsConfiguredDevice, ReturnsDefaultDevice,
ReturnsDefaultDevice_WhenConfiguredDeviceNotFound
- no MMDevice or COM references in test file; NAudio using removed
3. Fix stale comment in ResolveMonitoredDevice_UsesGetDevice test (removed
incorrect 'throw from getDefaultDevice' claim)
4. Tests use [Fact] not [WindowsFact] — intentional since no NAudio types
are referenced; pure logic test via string stub
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/VolumeWatcherService.cs:171
- This log message refers to the "default audio endpoint", but the service can now monitor a specifically configured device as well. To avoid misleading diagnostics, update the message to refer to the monitored/resolved audio endpoint (or include whether it was configured vs default).
else
{
_logger.LogWarning("Could not access the default audio endpoint. Volume monitoring is disabled.");
}
Comment on lines
+551
to
+552
| // so these tests are fully cross-platform and require no Windows/COM dependencies. | ||
| // [WindowsFact] is NOT used here intentionally — NAudio types are not referenced. |
There was a problem hiding this comment.
The comment says these tests are "fully cross-platform", but the test project targets net8.0-windows, so they still can’t run on non-Windows hosts. Consider rewording to clarify that the tests avoid COM/NAudio calls (pure logic), rather than claiming cross-platform execution.
Suggested change
| // so these tests are fully cross-platform and require no Windows/COM dependencies. | |
| // [WindowsFact] is NOT used here intentionally — NAudio types are not referenced. | |
| // so these tests exercise pure logic only and introduce no additional Windows/COM/NAudio | |
| // dependencies beyond the test project's net8.0-windows target. | |
| // [WindowsFact] is NOT used here intentionally because no NAudio or COM types are referenced. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses all 4 comments from the Copilot review on PR #34.
Changes
1.
IAppConfiguration.AudioDeviceId— doc wording fixCorrected XML doc to say "Core Audio endpoint ID string" (opaque, e.g.
{0.0.0.00000000}.{...}), not "GUID string". Matches Copilot suggestion.2.
VolumeWatcherService._defaultDevice→_monitoredDeviceRenamed the field throughout the class — semantics are clearer now that it can hold any configured device, not just the Windows default.
3.
VolumeWatcherService— device resolution is now testableExtracted the device selection logic into
internal static ResolveMonitoredDevice()with injected delegates (getDevice,getDefaultDevice,logWarning,logInfo).ExecuteAsyncdelegates to it.4.
SettingsManager— omitAudioDeviceIdkey when emptyWhen
audioDeviceIdis empty (= Windows default selected), the key is now removed from JSON rather than written as"". Bothnulland""mean "use Windows default" — no key needed.Tests added
6 new
[Fact]tests inVolumeWatcherServiceTestscoveringResolveMonitoredDevice:getDevicewhenAudioDeviceIdis configuredgetDefaultDevicewhenAudioDeviceIdis emptygetDefaultDevicewhenAudioDeviceIdis nullCOMException)"configured"kind when specific device selected"default"kind when no device configured2 updated
SettingsManagerTests:audioDeviceIdnow asserts key is absent from JSONAudioDeviceIdkeyPlatform note
ResolveMonitoredDevicetests use[Fact]— pure logic, no COM/Windows required. All existing[WindowsFact]tests still run on CI (windows-latest).