fix(common): avoid icon extractor type-init crash on invalid fallback resources#2244
Conversation
… resources Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesDefault Icon Fallback and Test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
SoundSwitch.Tests/AudioDeviceIconExtractorTests.cs (1)
14-22: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffConsider validating the correct fallback icon type is returned.
The test effectively verifies non-null fallback icons are returned for invalid paths, which matches the PR objective. However, it could be strengthened by validating that
DataFlow.Renderreturns the speakers icon andDataFlow.Capturereturns the microphone icon, ensuring the dataflow-specific fallback logic is correct.💡 Optional enhancement to validate icon types
This would require comparing the returned icon against the expected fallback (e.g., by comparing icon dimensions, pixel data, or other properties), which may be complex and brittle. The current test provides good coverage of the non-null guarantee, so this enhancement is optional.
🤖 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 `@SoundSwitch.Tests/AudioDeviceIconExtractorTests.cs` around lines 14 - 22, The test ExtractIconFromPath_WhenPathIsInvalid_ReturnsFallbackIcon currently only validates that fallback icons are non-null, but does not verify that the correct icon type is returned for each DataFlow parameter. Add assertions to validate that when dataFlow is DataFlow.Render, a speakers icon is returned, and when dataFlow is DataFlow.Capture, a microphone icon is returned. This can be done by comparing icon properties or dimensions against the expected fallback icons to ensure the dataflow-specific fallback logic functions correctly.
🤖 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 `@SoundSwitch.Common/Framework/Audio/Icon/AudioDeviceIconExtractor.cs`:
- Around line 36-47: The CreatePermanentDefaultIcon method is missing XML
documentation comments that should describe its purpose, parameters, and return
value. Add XML documentation above the method signature that includes a summary
explaining what the method does, parameter descriptions for bundledIconFactory,
fallbackIconFactory, and resourceName parameters, and a returns section
describing the IconHandle that is returned. This will ensure the method complies
with coding guidelines requiring proper documentation for all public members.
In `@SoundSwitch.Tests/AudioDeviceIconExtractorTests.cs`:
- Around line 11-13: The AudioDeviceIconExtractorTests class is missing XML
documentation comments. Add XML documentation above the class declaration using
the standard C# documentation syntax (/// comments) to describe the purpose and
responsibility of the test class. Include a summary tag that explains what the
AudioDeviceIconExtractorTests class tests and its overall purpose within the
test suite.
---
Nitpick comments:
In `@SoundSwitch.Tests/AudioDeviceIconExtractorTests.cs`:
- Around line 14-22: The test
ExtractIconFromPath_WhenPathIsInvalid_ReturnsFallbackIcon currently only
validates that fallback icons are non-null, but does not verify that the correct
icon type is returned for each DataFlow parameter. Add assertions to validate
that when dataFlow is DataFlow.Render, a speakers icon is returned, and when
dataFlow is DataFlow.Capture, a microphone icon is returned. This can be done by
comparing icon properties or dimensions against the expected fallback icons to
ensure the dataflow-specific fallback logic functions correctly.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15e70577-3a2a-4923-99d1-003a28fe3d8d
📒 Files selected for processing (2)
SoundSwitch.Common/Framework/Audio/Icon/AudioDeviceIconExtractor.csSoundSwitch.Tests/AudioDeviceIconExtractorTests.cs
| private static IconHandle CreatePermanentDefaultIcon(Func<System.Drawing.Icon> bundledIconFactory, Func<System.Drawing.Icon> fallbackIconFactory, string resourceName) | ||
| { | ||
| try | ||
| { | ||
| return IconExtractor.CreatePermanent(bundledIconFactory()); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Log.Warning(e, "Can't load bundled fallback icon {resourceName}, using system icon fallback", resourceName); | ||
| return IconExtractor.CreatePermanent((System.Drawing.Icon)fallbackIconFactory().Clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add XML documentation to the helper method.
The CreatePermanentDefaultIcon method lacks XML documentation. As per coding guidelines, classes and their members should use proper XML documentation to explain their purpose, parameters, and return values.
📝 Proposed XML documentation
+ /// <summary>
+ /// Creates a permanent icon handle from a bundled resource with fallback to system icons on failure.
+ /// </summary>
+ /// <param name="bundledIconFactory">Factory that returns the bundled icon resource.</param>
+ /// <param name="fallbackIconFactory">Factory that returns a system icon if bundled resource fails to load.</param>
+ /// <param name="resourceName">Name of the bundled resource for logging purposes.</param>
+ /// <returns>A permanent <see cref="IconHandle"/> that must be manually disposed.</returns>
private static IconHandle CreatePermanentDefaultIcon(Func<System.Drawing.Icon> bundledIconFactory, Func<System.Drawing.Icon> fallbackIconFactory, string resourceName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static IconHandle CreatePermanentDefaultIcon(Func<System.Drawing.Icon> bundledIconFactory, Func<System.Drawing.Icon> fallbackIconFactory, string resourceName) | |
| { | |
| try | |
| { | |
| return IconExtractor.CreatePermanent(bundledIconFactory()); | |
| } | |
| catch (Exception e) | |
| { | |
| Log.Warning(e, "Can't load bundled fallback icon {resourceName}, using system icon fallback", resourceName); | |
| return IconExtractor.CreatePermanent((System.Drawing.Icon)fallbackIconFactory().Clone()); | |
| } | |
| } | |
| /// <summary> | |
| /// Creates a permanent icon handle from a bundled resource with fallback to system icons on failure. | |
| /// </summary> | |
| /// <param name="bundledIconFactory">Factory that returns the bundled icon resource.</param> | |
| /// <param name="fallbackIconFactory">Factory that returns a system icon if bundled resource fails to load.</param> | |
| /// <param name="resourceName">Name of the bundled resource for logging purposes.</param> | |
| /// <returns>A permanent <see cref="IconHandle"/> that must be manually disposed.</returns> | |
| private static IconHandle CreatePermanentDefaultIcon(Func<System.Drawing.Icon> bundledIconFactory, Func<System.Drawing.Icon> fallbackIconFactory, string resourceName) | |
| { | |
| try | |
| { | |
| return IconExtractor.CreatePermanent(bundledIconFactory()); | |
| } | |
| catch (Exception e) | |
| { | |
| Log.Warning(e, "Can't load bundled fallback icon {resourceName}, using system icon fallback", resourceName); | |
| return IconExtractor.CreatePermanent((System.Drawing.Icon)fallbackIconFactory().Clone()); | |
| } | |
| } |
🤖 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 `@SoundSwitch.Common/Framework/Audio/Icon/AudioDeviceIconExtractor.cs` around
lines 36 - 47, The CreatePermanentDefaultIcon method is missing XML
documentation comments that should describe its purpose, parameters, and return
value. Add XML documentation above the method signature that includes a summary
explaining what the method does, parameter descriptions for bundledIconFactory,
fallbackIconFactory, and resourceName parameters, and a returns section
describing the IconHandle that is returned. This will ensure the method complies
with coding guidelines requiring proper documentation for all public members.
Source: Coding guidelines
| [TestFixture] | ||
| public class AudioDeviceIconExtractorTests | ||
| { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add XML documentation to the test class.
The test class lacks XML documentation. As per coding guidelines, classes should use proper XML documentation to describe their purpose.
📝 Proposed XML documentation
+ /// <summary>
+ /// Tests for <see cref="AudioDeviceIconExtractor"/> icon extraction and fallback behavior.
+ /// </summary>
[TestFixture]
public class AudioDeviceIconExtractorTests🤖 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 `@SoundSwitch.Tests/AudioDeviceIconExtractorTests.cs` around lines 11 - 13, The
AudioDeviceIconExtractorTests class is missing XML documentation comments. Add
XML documentation above the class declaration using the standard C#
documentation syntax (/// comments) to describe the purpose and responsibility
of the test class. Include a summary tag that explains what the
AudioDeviceIconExtractorTests class tests and its overall purpose within the
test suite.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
Hardens AudioDeviceIconExtractor static initialization to avoid startup crashes when bundled fallback icon resources fail to deserialize, ensuring tray/icon initialization can proceed with a system-icon fallback.
Changes:
- Replaced eager static fallback icon initialization with guarded creation that logs and falls back to cloned
SystemIconson resource load failure. - Kept existing
ExtractIconFromPathexception-based fallback behavior, now backed by resilient default icon handles. - Added regression tests verifying invalid icon paths still produce a usable fallback icon for both
DataFlow.RenderandDataFlow.Capture.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| SoundSwitch.Common/Framework/Audio/Icon/AudioDeviceIconExtractor.cs | Wraps bundled fallback icon creation in a try/catch and falls back to cloned system icons to prevent type init crashes. |
| SoundSwitch.Tests/AudioDeviceIconExtractorTests.cs | Adds a regression test ensuring invalid icon paths return a non-null fallback icon for render/capture flows. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Belphemur
changed the title
After updating, SoundSwitch could crash during startup with a
TypeInitializationExceptioninAudioDeviceIconExtractorif bundled fallback icon resources failed deserialization. This blocked tray icon initialization and app boot.Crash root cause addressed (static initialization hardening)
Resources.defaultSpeakers/Resources.defaultMicrophonefail to load, code now logs and falls back to clonedSystemIconsinstead of failing type initialization.Fallback behavior preserved for invalid device icon paths
ExtractIconFromPathfallback flow (Render/Capture) remains intact, now backed by resilient default-handle initialization.Regression coverage
DataFlow.RenderandDataFlow.Capture.