Fix About page icon + version, tray icon size, and taskbar icon cache#8
Merged
Conversation
The About page previously rendered a generic Fluent speaker glyph and a
hardcoded "Version 1.0.0" string — both drifted from reality once the
app started shipping under the new icon and auto-release started cutting
real version tags. Replace them with:
- An Image bound to a new 256x256 Assets/app-icon.png rasterised from
design/app-icon.svg (a PNG, not the multi-image app.ico, because
BitmapImage picks the first frame of an ICO — which for our
16/20/24/... layout is the tiny 16x16 glyph, producing a blurry result).
- A TextBlock bound to AppVersionDisplay, which reads the assembly's
AssemblyInformationalVersion at runtime so CI's -p:Version flows
through without any further hardcoding.
Add a "Check for updates" button that hits the GitHub Releases API via a
new UpdateService, compares tags, and shows a one-line status plus an
"Open release page" hyperlink when a newer version is available. The
service is pointer-only — no auto-download — so no elevation, hosting,
or code-signing rotation is required.
Fix the tray icon, which was loading via Icon.ExtractAssociatedIcon
(returns one arbitrary frame, typically 32x32, then scaled down to tray
size). Pack app.ico as a WPF resource and load it with the Icon ctor
passing SystemInformation.SmallIconSize — a DPI-aware size that matches
our embedded 16/20/24 frames exactly, so Windows gets an unscaled glyph.
Set Window.Icon explicitly to Assets/app-icon.png so the taskbar gets a
crisp high-res source rather than whatever WPF's default ICO-frame
picker returns.
Add a post-install ie4uinit.exe -show step to installer.iss so users who
pinned the app to their taskbar before an update see the new icon
without waiting for Explorer's shell-icon cache to roll over.
Extend IconBuilder to dispatch on output extension: .ico keeps the
current multi-size pack, .png emits a single rasterisation (used to
generate app-icon.png here). Regenerate with:
dotnet run --project design/tools/IconBuilder -- \
design/app-icon.svg AudioMonitorRouter/Assets/app-icon.png
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughAdds WPF value converters and app icon resources, implements a GitHub-based UpdateService plus UpdateCheckResult variants, integrates update UI and ViewModel commands, switches tray icon loading to packed resources, extends an icon-builder tool for .ico/.png output, and adds an installer step to refresh Explorer icon cache. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as MainWindow (UI)
participant VM as MainWindowViewModel
participant Svc as UpdateService
participant GH as GitHub Releases API
UI->>VM: Click "Check for updates"
VM->>VM: set IsCheckingForUpdate = true\nUpdateStatus = "Checking…"
VM->>Svc: CheckForUpdateAsync(ct)
Svc->>GH: GET /repos/.../releases/latest\n(User-Agent, Accept: application/vnd.github.v3+json)
alt 200 OK with valid tag_name
GH-->>Svc: { tag_name, html_url }
Svc-->>VM: UpToDate or UpdateAvailable(result)
else HTTP 403 (rate limit)
GH-->>Svc: 403
Svc-->>VM: Failed(result)
else Timeout/Network/Error
GH-->>Svc: timeout/error
Svc-->>VM: NetworkError(result)
end
VM->>UI: update UpdateStatus, LatestReleaseUrl\nset IsCheckingForUpdate = false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
AudioMonitorRouter/Views/MainWindow.xaml (1)
559-569: Prevent update status text from overflowing on long messages.At Line 564, status text sits in a horizontal
StackPanel; longer messages (e.g., network error details) can run wide and hurt readability. A small layout change to aGridwith a star column makes this robust.♻️ Proposed layout tweak
- <StackPanel Orientation="Horizontal" Margin="0,0,0,8"> - <wpfui:Button Content="Check for updates" - Icon="ArrowSync24" - Command="{Binding CheckForUpdatesCommand}" - IsEnabled="{Binding IsCheckingForUpdate, Converter={StaticResource InverseBooleanConverter}}"/> - <TextBlock Text="{Binding UpdateStatus}" - VerticalAlignment="Center" - Margin="12,0,0,0" - FontSize="13" - Foreground="{DynamicResource TextFillColorSecondaryBrush}"/> - </StackPanel> + <Grid Margin="0,0,0,8"> + <Grid.ColumnDefinitions> + <ColumnDefinition Width="Auto"/> + <ColumnDefinition Width="12"/> + <ColumnDefinition Width="*"/> + </Grid.ColumnDefinitions> + <wpfui:Button Grid.Column="0" + Content="Check for updates" + Icon="ArrowSync24" + Command="{Binding CheckForUpdatesCommand}" + IsEnabled="{Binding IsCheckingForUpdate, Converter={StaticResource InverseBooleanConverter}}"/> + <TextBlock Grid.Column="2" + Text="{Binding UpdateStatus}" + VerticalAlignment="Center" + FontSize="13" + TextWrapping="Wrap" + Foreground="{DynamicResource TextFillColorSecondaryBrush}"/> + </Grid>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AudioMonitorRouter/Views/MainWindow.xaml` around lines 559 - 569, Replace the horizontal StackPanel containing the Check for updates Button and the UpdateStatus TextBlock with a Grid so the status TextBlock can live in a star-sized column and wrap/truncate; specifically, keep the Button bound to CheckForUpdatesCommand (and IsEnabled bound to IsCheckingForUpdate with the InverseBooleanConverter) in a fixed-width column and move the UpdateStatus TextBlock (Binding: UpdateStatus) into a second column with TextWrapping="Wrap" or TextTrimming="CharacterEllipsis" and VerticalAlignment="Center" so long status messages do not overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AudioMonitorRouter/Services/UpdateService.cs`:
- Around line 105-115: Replace the System.Version parse-and-fallback logic in
UpdateService (the block using Version.TryParse on variables latest and current
and returning UpdateCheckResult.UpToDate/UpdateAvailable) with proper SemVer
parsing and comparison using NuGet.Versioning.NuGetVersion: try
NuGetVersion.TryParse on both latest and current, and if both parse compare them
(NuGetVersion implements correct SemVer ordering) to decide UpToDate vs
UpdateAvailable; if only one parses, use the parsed value and the other as a
prerelease/unknown per SemVer rules (or conservatively treat as update) and
preserve the existing construction of UpdateCheckResult.UpdateAvailable/UpToDate
and the release.HtmlUrl fallback. Ensure you reference
NuGet.Versioning.NuGetVersion.TryParse and use NuGetVersion.CompareTo/Compare
operators instead of System.Version.
In `@design/tools/IconBuilder/Program.cs`:
- Around line 90-113: The WriteIco method fails if the target folder doesn't
exist; before creating the FileStream in WriteIco(string path) ensure the parent
directory exists by calling Directory.CreateDirectory on
Path.GetDirectoryName(path) (handle null/empty root safely) so the directory is
created when needed, then open the FileStream as before; this mirrors the
behavior in WritePng and prevents DirectoryNotFoundException for .ico outputs.
In `@installer/installer.iss`:
- Around line 84-89: The ie4uinit invocation runs in the elevated installer
token and needs the runasoriginaluser flag so it runs in the signed-in user's
context; update the installer entry that calls ie4uinit.exe (the line with
Filename: "{sys}\ie4uinit.exe"; Parameters: "-show"; Flags: ...) to include
runasoriginaluser alongside runhidden and skipifdoesntexist so Explorer's icon
cache is refreshed for the original user rather than the elevation/admin token.
---
Nitpick comments:
In `@AudioMonitorRouter/Views/MainWindow.xaml`:
- Around line 559-569: Replace the horizontal StackPanel containing the Check
for updates Button and the UpdateStatus TextBlock with a Grid so the status
TextBlock can live in a star-sized column and wrap/truncate; specifically, keep
the Button bound to CheckForUpdatesCommand (and IsEnabled bound to
IsCheckingForUpdate with the InverseBooleanConverter) in a fixed-width column
and move the UpdateStatus TextBlock (Binding: UpdateStatus) into a second column
with TextWrapping="Wrap" or TextTrimming="CharacterEllipsis" and
VerticalAlignment="Center" so long status messages do not overflow.
🪄 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 Plus
Run ID: f99e4364-77fd-482c-8de0-faab742c3f41
⛔ Files ignored due to path filters (1)
AudioMonitorRouter/Assets/app-icon.pngis excluded by!**/*.png
📒 Files selected for processing (9)
AudioMonitorRouter/App.xamlAudioMonitorRouter/App.xaml.csAudioMonitorRouter/AudioMonitorRouter.csprojAudioMonitorRouter/Services/UpdateService.csAudioMonitorRouter/ViewModels/MainWindowViewModel.csAudioMonitorRouter/Views/MainWindow.xamlAudioMonitorRouter/Views/MainWindow.xaml.csdesign/tools/IconBuilder/Program.csinstaller/installer.iss
Comment on lines
+105
to
+115
| if (!Version.TryParse(latest, out var latestVer) || | ||
| !Version.TryParse(current, out var currentVer)) | ||
| { | ||
| // If either side doesn't parse (e.g. "1.2.3-beta"), fall back to | ||
| // a simple string compare. Better to surface uncertainty than | ||
| // silently claim "up to date". | ||
| return string.Equals(latest, current, StringComparison.Ordinal) | ||
| ? new UpdateCheckResult.UpToDate(current) | ||
| : new UpdateCheckResult.UpdateAvailable(current, latest, | ||
| release.HtmlUrl ?? $"https://github.com/twibster/AudioMonitorRouter/releases/tag/{release.TagName}"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does .NET System.Version.TryParseaccept semantic-version prerelease strings like1.2.3-beta.1, and if not, what .NET-supported library is commonly used to compare SemVer versions with prerelease labels?
💡 Result:
No, .NET System.Version.TryParse does not accept semantic-version prerelease strings like "1.2.3-beta.1". It requires the format major.minor[.build[.revision]] with numeric components only and fails on non-numeric prerelease suffixes, returning false. The commonly used .NET-supported library for comparing SemVer versions with prerelease labels is NuGet.Versioning from the NuGet package manager, specifically the NuGetVersion class (which inherits from SemanticVersion). It provides Parse/TryParse methods that handle prerelease labels like "-beta.1" and proper comparison logic following SemVer rules (with some NuGet-specific extensions). It is available via the NuGet.Versioning NuGet package. Alternatives include the "semver" NuGet package (SemVersion class) and older NuGet.Core SemanticVersion, but NuGet.Versioning is the most authoritative and widely used in the .NET ecosystem for package versioning.
Citations:
- 1: Proposal: Adding System.SemanticVersion dotnet/runtime#19317
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.version.tryparse?view=net-9.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.version.parse?view=net-9.0
- 4: https://stackoverflow.com/questions/24769091/is-there-a-nuget-package-that-contains-a-semantic-version-parser-in-net
- 5: https://github.com/NuGet/NuGet2/blob/2.13/src/Core/SemanticVersion.cs
- 6: https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Versioning/NuGetVersion.cs
- 7: https://github.com/NuGetArchive/NuGet.Versioning/blob/master/src/NuGet.Versioning/SemanticVersion.cs
- 8: https://learn.microsoft.com/en-us/nuget/concepts/package-versioning?tabs=semver20sort
🏁 Script executed:
# Find and read the UpdateService.cs file around lines 105-115
fd -t f "UpdateService.cs" | head -5Repository: twibster/AudioMonitorRouter
Length of output: 115
🏁 Script executed:
# Look for project files to check dependencies
fd -t f "\.(csproj|fsproj|vbproj)$" | head -10Repository: twibster/AudioMonitorRouter
Length of output: 159
🏁 Script executed:
# Check if NuGet.Versioning is already referenced in the project
rg "NuGet\.Versioning|System\.Version" --type csharpRepository: twibster/AudioMonitorRouter
Length of output: 53
🏁 Script executed:
# Read the UpdateService.cs file to verify the snippet
cat -n AudioMonitorRouter/Services/UpdateService.cs | sed -n '100,120p'Repository: twibster/AudioMonitorRouter
Length of output: 1313
🏁 Script executed:
# Read the main project file to check current dependencies
cat AudioMonitorRouter/AudioMonitorRouter.csprojRepository: twibster/AudioMonitorRouter
Length of output: 2584
🏁 Script executed:
# Search for any mention of SemVer, semantic versioning, or prerelease handling in codebase
rg -i "semver|semantic.*version|prerelease" --type csharpRepository: twibster/AudioMonitorRouter
Length of output: 53
🏁 Script executed:
# Check git log for recent commits mentioning version comparison or SemVer
git log --oneline --all -20 | head -20Repository: twibster/AudioMonitorRouter
Length of output: 1227
System.Version doesn't parse SemVer prerelease strings; the fallback approach has correctness implications.
When either version contains a prerelease label (e.g., 1.2.3-beta.1), System.Version.TryParse fails, and the code falls back to ordinal string equality. This means versions like 1.0.0 and 1.0.0-beta are treated as unequal, always triggering an "update available" result.
While this conservative fallback avoids false "up to date" claims, it doesn't implement proper SemVer comparison. Consider using NuGet.Versioning.NuGetVersion instead, which the .NET ecosystem standard library for SemVer parsing and comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AudioMonitorRouter/Services/UpdateService.cs` around lines 105 - 115, Replace
the System.Version parse-and-fallback logic in UpdateService (the block using
Version.TryParse on variables latest and current and returning
UpdateCheckResult.UpToDate/UpdateAvailable) with proper SemVer parsing and
comparison using NuGet.Versioning.NuGetVersion: try NuGetVersion.TryParse on
both latest and current, and if both parse compare them (NuGetVersion implements
correct SemVer ordering) to decide UpToDate vs UpdateAvailable; if only one
parses, use the parsed value and the other as a prerelease/unknown per SemVer
rules (or conservatively treat as update) and preserve the existing construction
of UpdateCheckResult.UpdateAvailable/UpToDate and the release.HtmlUrl fallback.
Ensure you reference NuGet.Versioning.NuGetVersion.TryParse and use
NuGetVersion.CompareTo/Compare operators instead of System.Version.
The previous design had 58px of padding top AND bottom (45% of the 256px
canvas empty vertically), which is why the taskbar glyph looked visibly
smaller than neighbour icons — at 24px cell size our artwork was only
~13px tall while Chrome/Explorer/etc fill ~22px.
Redraw with the same semantic layout (widescreen monitor + speaker +
soundwave + outbound arrow crossing the right bezel), but:
• Monitor aspect 5:3 → 1.25:1 (170x100 → 170x140), shifted up/down to
sit close to both canvas edges.
• Strokes bumped 12→14 / 13→15 so linework doesn't look spindly at the
new scale.
• Speaker, soundwave, and arrow re-positioned so the arrow tail still
starts just past the wave and the tip clears the bezel cleanly.
Content bounds:
before after
width fill 84% 89%
height fill 55% 79%
glyph@24px ~20x13 ~21x19 (+45% visible area)
Regenerate both assets from the new SVG:
dotnet run --project design/tools/IconBuilder -- \
design/app-icon.svg AudioMonitorRouter/app.ico
dotnet run --project design/tools/IconBuilder -- \
design/app-icon.svg AudioMonitorRouter/Assets/app-icon.png
v2 still sat ~9%/12%/5%/6% padded (top/bottom/left/right) while peer icons (Chrome, Explorer, etc.) fill ~3-4% per side. Pushing the glyph harder into the canvas makes it read at a comparable weight on the taskbar and tray. - Content bounds now ~94% x 85% (was 89% x 79% in v2). - Monitor aspect relaxed to 1.18:1 (was 1.25:1) to recover vertical height the old proportion was spending on empty padding. The stand still grounds it so it reads as a display, not a picture frame. - Stand base widened (62..140 vs v2's narrower base) so the wider monitor frame still feels supported rather than top-heavy. - Speaker, soundwave, and outbound arrow scaled proportionally; the arrow tip is now flush with the right canvas edge (round cap ends at x=247.5).
The bullet list of features was restating the app description in a less
skimmable form — anyone who reads the one-line description already knows
roughly what the app does, and the bullet format was the visual heaviest
block on the page.
Swapped it for a compact footer with:
- A copyright line pulled from [AssemblyCopyright] (MSBuild fills it
from the csproj's <Copyright>) with "(c)" rewritten to © for display.
- Two external links via wpfui:Hyperlink with the Link24 glyph:
Project website on GitHub (repo root)
Latest release on GitHub (/releases/latest, GitHub redirects
to the current tag)
UpdateService gained a Copyright property alongside CurrentVersion so
the VM has one place to read assembly metadata from.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
AudioMonitorRouter/Services/UpdateService.cs (1)
115-125:⚠️ Potential issue | 🟠 MajorUse a real SemVer comparison here.
This PR reads
CurrentVersionfromAssemblyInformationalVersion, so prerelease values like1.2.4-beta.1are a real input.System.Version.TryParserejects those and drops into ordinal string comparison, which can misreport update availability.Does .NET `System.Version.TryParse` accept semantic-version prerelease strings like `1.2.3-beta.1`, and what .NET-supported library is recommended for correct SemVer parsing and comparison?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AudioMonitorRouter/Services/UpdateService.cs` around lines 115 - 125, Replace the System.Version-based parsing with a real SemVer parser: add a NuGet semver library (e.g., NuGet.Versioning or Semver) and use its TryParse/Parse for the variables latest and current instead of Version.TryParse; then compare the parsed semantic versions (e.g., SemanticVersion or SemVersion comparison) to decide whether to return UpdateCheckResult.UpToDate(current) or UpdateCheckResult.UpdateAvailable(current, latest, ...). If either semver parse fails, fall back to the existing ordinal string comparison fallback. Update the logic around the Version.TryParse block in UpdateService.cs (the latest/current parsing and comparison) and keep the same UpdateCheckResult branches and release.HtmlUrl/release.TagName fallback URL.
🧹 Nitpick comments (1)
AudioMonitorRouter/Services/UpdateService.cs (1)
153-155: Don't show raw exception text to users.
AudioMonitorRouter/ViewModels/MainWindowViewModel.cs:704-718displaysFailed.Messageverbatim. Returningex.Messagehere leaks low-level parser/network wording to the About page and still drops the stack/context you need for diagnosis. Log the exception and return a stable user-facing message instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AudioMonitorRouter/Services/UpdateService.cs` around lines 153 - 155, In UpdateService.cs replace the catch block that returns new UpdateCheckResult.Failed(ex.Message) so you log the full exception for diagnostics (e.g., _logger.Error(ex, "Update check failed") or similar existing logging/telemetry) and return a stable, user-facing message instead (for example: new UpdateCheckResult.Failed("Could not check for updates at this time. Please try again later.")); keep the exception out of the Failed.Message shown in the UI and ensure the exception object is logged/preserved for debugging while UpdateCheckResult.Failed carries only the friendly text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AudioMonitorRouter/Services/UpdateService.cs`:
- Around line 123-124: When constructing the fallback release URL in
UpdateService (where UpdateCheckResult.UpdateAvailable is created), escape the
tag before concatenation to avoid broken links for tags with reserved
characters; replace direct use of release.TagName with an encoded form (e.g.,
Uri.EscapeDataString or WebUtility.UrlEncode) in both locations (the current
fallback at the shown line and the similar occurrence around lines 132-133) so
the generated "releases/tag/{...}" segment is URL-safe.
---
Duplicate comments:
In `@AudioMonitorRouter/Services/UpdateService.cs`:
- Around line 115-125: Replace the System.Version-based parsing with a real
SemVer parser: add a NuGet semver library (e.g., NuGet.Versioning or Semver) and
use its TryParse/Parse for the variables latest and current instead of
Version.TryParse; then compare the parsed semantic versions (e.g.,
SemanticVersion or SemVersion comparison) to decide whether to return
UpdateCheckResult.UpToDate(current) or
UpdateCheckResult.UpdateAvailable(current, latest, ...). If either semver parse
fails, fall back to the existing ordinal string comparison fallback. Update the
logic around the Version.TryParse block in UpdateService.cs (the latest/current
parsing and comparison) and keep the same UpdateCheckResult branches and
release.HtmlUrl/release.TagName fallback URL.
---
Nitpick comments:
In `@AudioMonitorRouter/Services/UpdateService.cs`:
- Around line 153-155: In UpdateService.cs replace the catch block that returns
new UpdateCheckResult.Failed(ex.Message) so you log the full exception for
diagnostics (e.g., _logger.Error(ex, "Update check failed") or similar existing
logging/telemetry) and return a stable, user-facing message instead (for
example: new UpdateCheckResult.Failed("Could not check for updates at this time.
Please try again later.")); keep the exception out of the Failed.Message shown
in the UI and ensure the exception object is logged/preserved for debugging
while UpdateCheckResult.Failed carries only the friendly text.
🪄 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 Plus
Run ID: 6b161d27-9114-4c97-8e8d-746b778ebe9f
⛔ Files ignored due to path filters (3)
AudioMonitorRouter/Assets/app-icon.pngis excluded by!**/*.pngAudioMonitorRouter/app.icois excluded by!**/*.icodesign/app-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
AudioMonitorRouter/Services/UpdateService.csAudioMonitorRouter/ViewModels/MainWindowViewModel.csAudioMonitorRouter/Views/MainWindow.xaml
🚧 Files skipped from review as they are similar to previous changes (2)
- AudioMonitorRouter/Views/MainWindow.xaml
- AudioMonitorRouter/ViewModels/MainWindowViewModel.cs
Three adjustments to the About card:
1. Copyright moves into the identity block next to the icon, stacked
directly under the version line. One visual unit now answers "what
is this and who made it" instead of scattering that info across
the card.
2. The one-sentence app description is dropped. It was re-explaining
what the sidebar and home page already make obvious.
3. Links section lifts into the vacated slot; Updates drops to the
bottom. Reading order is now identity → external links → action
(check for updates), which matches what someone actually comes to
the About page to do.
Also updated the <Copyright> csproj property (and the UpdateService
fallback that mirrors it) from "twibster" to "Omar Omran" — that's the
real copyright holder; "twibster" is just the GitHub handle.
The two hyperlinks were being rendered with wpfui:Button's default Padding (11,5,11,6), which made the stack read as two spaced-out buttons instead of a compact link list like the rest of the app's peers (LLT, EarTrumpet). Dropped Padding to 8,2 and removed the 4px inter-link margin. The horizontal hit target stays generous (16px total) so clicking remains forgiving; only the vertical whitespace shrinks.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
AudioMonitorRouter/Services/UpdateService.cs (1)
165-179: Consider caching the version string to avoid repeated reflection.
GetInformationalVersion()is called from bothCurrentVersionandBuildUserAgent(). While the overhead is minimal, the value never changes at runtime. Astatic readonlyfield would eliminate redundant reflection calls.♻️ Suggested refactor
+ private static readonly string _informationalVersion = ComputeInformationalVersion(); + public string CurrentVersion => GetInformationalVersion(); - private static string GetInformationalVersion() + private static string GetInformationalVersion() => _informationalVersion; + + private static string ComputeInformationalVersion() { var asm = Assembly.GetEntryAssembly() ?? typeof(UpdateService).Assembly; // ... rest unchanged }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AudioMonitorRouter/Services/UpdateService.cs` around lines 165 - 179, GetInformationalVersion() performs reflection every time it’s called from CurrentVersion and BuildUserAgent; replace that repeated work by computing the value once into a static readonly string (e.g., a private static readonly field like s_informationalVersion) and have GetInformationalVersion() (or callers) return that cached field instead; update references in CurrentVersion and BuildUserAgent to use the cached value and ensure initialization happens once at type load so the reflection call is eliminated at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@AudioMonitorRouter/Services/UpdateService.cs`:
- Around line 165-179: GetInformationalVersion() performs reflection every time
it’s called from CurrentVersion and BuildUserAgent; replace that repeated work
by computing the value once into a static readonly string (e.g., a private
static readonly field like s_informationalVersion) and have
GetInformationalVersion() (or callers) return that cached field instead; update
references in CurrentVersion and BuildUserAgent to use the cached value and
ensure initialization happens once at type load so the reflection call is
eliminated at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa0c76f4-5813-4ab2-bd17-8213616de195
📒 Files selected for processing (3)
AudioMonitorRouter/AudioMonitorRouter.csprojAudioMonitorRouter/Services/UpdateService.csAudioMonitorRouter/Views/MainWindow.xaml
🚧 Files skipped from review as they are similar to previous changes (1)
- AudioMonitorRouter/AudioMonitorRouter.csproj
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AudioMonitorRouter/Views/MainWindow.xaml`:
- Around line 560-569: The UpdateStatus TextBlock can be clipped because it's
inside a horizontal StackPanel; change the layout so the button gets auto width
and the TextBlock can take remaining space and wrap. Replace the StackPanel
containing the Button (Command="{Binding CheckForUpdatesCommand}",
IsEnabled="{Binding IsCheckingForUpdate, Converter={StaticResource
InverseBooleanConverter}}") and the TextBlock (Text="{Binding UpdateStatus}")
with a Grid with two columns (first "Auto" for the Button, second "*" for the
TextBlock), and set the TextBlock properties TextWrapping="Wrap" and
TextTrimming="CharacterEllipsis" (and optionally a MaxWidth or Margin) so long
UpdateStatus strings from MainWindowViewModel (e.g., the long network/failure
messages) render correctly.
🪄 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 Plus
Run ID: 04de2bc4-06f4-4a1f-9797-091f5780f847
📒 Files selected for processing (1)
AudioMonitorRouter/Views/MainWindow.xaml
- UpdateService: replace Version.TryParse-only compare with a local CompareSemVer helper so "1.2.3-beta" vs "1.2.3" no longer falls back to an ordinal string compare (which would incorrectly report the prerelease as a pending "update"). Numeric core via System.Version, prerelease tail with the rule that release outranks prerelease. - UpdateService: Uri.EscapeDataString the tag name when composing the fallback release URL — tag names can legally contain '/' or '#', which would break the path segment. - IconBuilder WriteIco: mirror WritePng's Directory.CreateDirectory so pointing at a not-yet-existing Assets/ subfolder doesn't throw. - installer.iss: add runasoriginaluser to the ie4uinit -show step so over-the-shoulder UAC installs refresh the signed-in user's Explorer icon cache instead of the admin's. - MainWindow About/Updates: swap the horizontal StackPanel for a Grid (Auto + *) with TextWrapping=Wrap on the status line, so long "Update available: x.y.z" messages wrap inside the card instead of clipping off the right edge.
|
Released as v1.3.0. |
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
1 participant
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.
Summary
Addresses three issues spotted after installing v1.2.3:
About page shows wrong icon and "Version 1.0.0" — the
wpfui:SymbolIcon Speaker224and hardcoded version string are replaced with anImagebound to a new 256x256Assets/app-icon.pngand aTextBlockbound toAppVersionDisplay, which readsAssemblyInformationalVersionat runtime. CI's-p:Version=X.Y.Zalready flows into that attribute, so no further hardcoding is needed.Tray icon smaller than peers —
NotifyIcon.Iconwas being assigned fromIcon.ExtractAssociatedIcon(exePath), which pulls a single arbitrary frame (typically 32x32) and then gets scaled down to tray size. Now it loadsapp.icoas a WPF resource and passesSystemInformation.SmallIconSizeto theIconctor — a DPI-aware size (16/20/24) that matches our embedded frames exactly, so Windows picks an unscaled glyph.Taskbar still shows old icon after update — this is Explorer's shell-icon cache holding a stale copy of the pinned shortcut. The installer now runs
ie4uinit.exe -showpost-install to refresh it. Also setWindow.Iconexplicitly to the new PNG so taskbar/alt-tab renders get a high-res source instead of WPF's default ICO-frame-picker result.Bonus: "Check for updates" in About
New
UpdateServicequeriesapi.github.com/repos/.../releases/latest, compares semver tags, and returns one of four results. The About page renders a status line + "Open release page" hyperlink when an update is available. It's pointer-only (no auto-download), so we don't need admin elevation, update hosting, or code-signing rotation — the user clicks through and runs the installer themselves.IconBuilder extended
Now dispatches on output extension:
.icokeeps the current multi-size pack,.pngemits a single rasterisation (used to generateapp-icon.png). Regenerate either with:Test plan
dotnet build -c Releasepassesrelease:minorlabel triggers auto-release to v1.3.0 on mergeSummary by CodeRabbit
New Features
Chores