Load application favicons without blocking the UI - #4889
Conversation
github-actions
Bot
added
new feature
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds asynchronous favicon loading for app entries. It includes URL construction, queueing, bounded runspace execution, circuit-breaker handling, dispatcher-based WPF updates, fallback rendering, and shutdown cleanup. ChangesFavicon loading
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AppEntry
participant InstallRendering
participant FaviconFetch
participant RunspacePool
participant CircuitBreaker
participant Dispatcher
participant WPFControls
AppEntry->>InstallRendering: queue favicon request after app rendering
InstallRendering->>FaviconFetch: start favicon loading
FaviconFetch->>CircuitBreaker: check request eligibility
FaviconFetch->>RunspacePool: submit asynchronous HTTP fetch
RunspacePool->>CircuitBreaker: report fetch result
Dispatcher->>FaviconFetch: complete finished operation
FaviconFetch->>WPFControls: show bitmap or fallback
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🤖 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 `@functions/private/Close-WinUtilFaviconRunspacePool.ps1`:
- Around line 19-31: The Close-WinUtilFaviconRunspacePool cleanup must not call
PowerShell.Stop() for each pending favicon operation, since it can block the
form’s Closing path. Remove the per-operation Stop invocation while retaining
disposal and operation collection cleanup, and ensure the circuit breaker or
runspace pool is closed through the existing non-blocking cleanup path.
In `@functions/private/Initialize-WinUtilFaviconRunspacePool.ps1`:
- Around line 10-16: Update the stale-pool branch in
Initialize-WinUtilFaviconRunspacePool so it disposes and clears only
$sync.FaviconRunspace instead of calling Close-WinUtilFaviconRunspacePool.
Preserve the shared $sync.FaviconCircuitBreaker and $sync.FaviconOperations
entries while recreating the runspace pool.
In `@functions/private/Invoke-WinUtilFaviconFetch.ps1`:
- Around line 1-275: Split the four functions into separate files named after
their primary functions: Initialize-WinUtilFaviconCircuitBreaker,
Complete-WinUtilFaviconFetch, Start-WinUtilFaviconPolling, and
Invoke-WinUtilFaviconFetch. Preserve each function’s implementation and ensure
the project loads or includes all four files wherever this file was previously
referenced.
🪄 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: 32039875-3a3a-4b0a-808e-3db49ddefd2b
📒 Files selected for processing (8)
functions/private/Close-WinUtilFaviconRunspacePool.ps1functions/private/Get-WinUtilFaviconUrl.ps1functions/private/Initialize-InstallAppEntry.ps1functions/private/Initialize-WinUtilFaviconRunspacePool.ps1functions/private/Invoke-WinUtilFaviconFetch.ps1pester/favicon-loading.Tests.ps1pester/xaml.Tests.ps1scripts/main.ps1
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pester/favicon-loading.Tests.ps1 (1)
147-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the cancellation path this test claims to cover.
The test reports failures after a success but never requests cancellation, so it cannot verify “ignores cancellations.” Rename the test to describe reset behavior or explicitly invoke cancellation and assert post-cancellation behavior.
🤖 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 `@pester/favicon-loading.Tests.ps1` around lines 147 - 158, Update the test around Initialize-WinUtilFaviconCircuitBreaker so it either renames the test to cover only consecutive-failure reset or explicitly invokes the circuit breaker’s cancellation operation and asserts the expected post-cancellation behavior. If retaining “ignores cancellations” in the name, exercise cancellation through the relevant FaviconCircuitBreaker member and verify its effect alongside the existing failure-count assertion.
🤖 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.
Outside diff comments:
In `@pester/favicon-loading.Tests.ps1`:
- Around line 147-158: Update the test around
Initialize-WinUtilFaviconCircuitBreaker so it either renames the test to cover
only consecutive-failure reset or explicitly invokes the circuit breaker’s
cancellation operation and asserts the expected post-cancellation behavior. If
retaining “ignores cancellations” in the name, exercise cancellation through the
relevant FaviconCircuitBreaker member and verify its effect alongside the
existing failure-count assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e0526459-f065-4eaa-96c5-a8fc8594cfd4
📒 Files selected for processing (2)
functions/private/Initialize-WinUtilFaviconRunspacePool.ps1pester/favicon-loading.Tests.ps1
|
@codex review Check if consolidation is possible with existing runspace init function |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1837a8dbaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
functions/private/Invoke-WinUtilFaviconFetch.ps1 (2)
108-140: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUpdate the circuit breaker for every favicon failure.
ReportFailure()is only called forNetworkFailurereturns from the worker. Complete-WinUtilFaviconFetch also hides empty successful results and exceptions thrown byEndInvoke()without counting them as failures, so eight consecutive failures can bypass the cutoff. Count empty-success andEndInvoke()failures explicitly.🤖 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 `@functions/private/Invoke-WinUtilFaviconFetch.ps1` around lines 108 - 140, Update Complete-WinUtilFaviconFetch so FaviconCircuitBreaker.ReportFailure() is called for every failed favicon attempt, including empty or invalid worker results and exceptions from EndInvoke(). Preserve the existing fallback visibility behavior while adding failure reporting in the outer catch and non-success/empty-result paths, without double-counting failures already reported by the worker.
212-275: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up failed PowerShell submissions.
The operation is registered before
BeginInvoke()andStart-WinUtilFaviconPolling. If setup, submission, or polling startup fails,$sync.FaviconOperations[$AppKey]can keep a record with$operation.Handle = $null, and$powershellmay not be disposed. Wrap the creation/assignment/submission/polling path intry/catch, remove only the failed operation, dispose$powershell, then rethrow or return a controlled failure.🤖 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 `@functions/private/Invoke-WinUtilFaviconFetch.ps1` around lines 212 - 275, Wrap the operation registration, Handle assignment via BeginInvoke, and Start-WinUtilFaviconPolling call in a try/catch. On failure, remove only the matching $sync.FaviconOperations[$AppKey] entry, dispose $powershell, and then rethrow or return the function’s controlled failure result; preserve the existing cancellation cleanup before registration.
🤖 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.
Outside diff comments:
In `@functions/private/Invoke-WinUtilFaviconFetch.ps1`:
- Around line 108-140: Update Complete-WinUtilFaviconFetch so
FaviconCircuitBreaker.ReportFailure() is called for every failed favicon
attempt, including empty or invalid worker results and exceptions from
EndInvoke(). Preserve the existing fallback visibility behavior while adding
failure reporting in the outer catch and non-success/empty-result paths, without
double-counting failures already reported by the worker.
- Around line 212-275: Wrap the operation registration, Handle assignment via
BeginInvoke, and Start-WinUtilFaviconPolling call in a try/catch. On failure,
remove only the matching $sync.FaviconOperations[$AppKey] entry, dispose
$powershell, and then rethrow or return the function’s controlled failure
result; preserve the existing cancellation cleanup before registration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fb60808-b6b9-4e34-9c0c-4ba4c2d4054d
📒 Files selected for processing (3)
functions/private/Initialize-InstallAppEntry.ps1functions/private/Invoke-WinUtilFaviconFetch.ps1pester/favicon-loading.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (2)
- functions/private/Initialize-InstallAppEntry.ps1
- pester/favicon-loading.Tests.ps1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 196854264e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pester/favicon-loading.Tests.ps1 (1)
199-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace source-text checks with executable failure-path tests.
Both tests inspect
Invoke-WinUtilFaviconFetch.ps1as text. Unrelated copies of these fragments can satisfy the tests while runtime behavior is broken.
pester/favicon-loading.Tests.ps1#L199-L204: execute completion handling and verify unexpected failures increment the breaker once.pester/favicon-loading.Tests.ps1#L206-L212: force submission failure and verify operation removal,$powershell.Dispose(), and rethrow behavior.🤖 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 `@pester/favicon-loading.Tests.ps1` around lines 199 - 204, The tests at pester/favicon-loading.Tests.ps1 lines 199-204 and 206-212 must replace source-text matching with executable failure-path coverage. Exercise completion handling to verify an unexpected failure increments FaviconCircuitBreaker exactly once, and force submission failure to verify operation removal, $powershell.Dispose(), and exception rethrow; update both test sites accordingly.
🤖 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.
Nitpick comments:
In `@pester/favicon-loading.Tests.ps1`:
- Around line 199-204: The tests at pester/favicon-loading.Tests.ps1 lines
199-204 and 206-212 must replace source-text matching with executable
failure-path coverage. Exercise completion handling to verify an unexpected
failure increments FaviconCircuitBreaker exactly once, and force submission
failure to verify operation removal, $powershell.Dispose(), and exception
rethrow; update both test sites accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fbc639b-2cfa-4fd1-be2e-5137516717c6
📒 Files selected for processing (2)
functions/private/Invoke-WinUtilFaviconFetch.ps1pester/favicon-loading.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/private/Invoke-WinUtilFaviconFetch.ps1
|
@coderabbitai The comment is technically reasonable, but replacing only these two tests with complex mocks would be inconsistent and disproportionate for this PR. Mocking this would be substantially more complicated. You’d need fake PowerShell objects, async handles, WPF controls, the synchronized operation table, and failure injection for BeginInvoke()/EndInvoke(). |
|
Tip For best results, initiate chat on the files or code changes.
I withdraw the comment. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@mewclouds , Cool feature, the blocking favicon loads during UI generation is a real pain point. Had a thought on a lighter way to get the same result, wanted to share in case it's useful. Approach: Instead of a dedicated
The key difference is that WPF control references never cross the runspace boundary, only byte arrays flow through Happy to open a PR with this if you'd like to compare side by side! |
|
@vyas-devgna Thanks for sharing the alternative. I spent some time comparing it with the current implementation, and I do not think the two approaches are equivalent. We need to be careful here because the differences affect concurrency, cleanup, and how the rest of WinUtil behaves. The dedicated pool is intentional. It limits favicon downloads to 2 to 8 workers and keeps them separate from installs and other work using the general WinUtil pool. Using Invoke-WPFRunspace would remove that isolation and, as proposed, replace bounded parallel loading with one sequential job that starts only after the app list finishes rendering. It stays off the UI thread, but the downloads themselves are sequential and the job still occupies one slot in the shared pool. The C# circuit breaker is there because several favicon workers can complete at the same time. Its lock serializes failure counts and success resets, while the cancellation token provides thread-safe cancellation. A plain counter is safe in the suggested approach only because one sequential worker is updating it. If the requests were made concurrent again, shared thread-safe state would still be required. WPF controls are also not crossing into the worker runspaces. Each worker receives only the URL, connection limit, circuit breaker, and timeout. It returns a status and downloaded byte array. TargetImage and Fallback remain in the UI-side operation record and are only updated by the dispatcher timer. The cleanup behavior is different as well. Setting a cancellation flag can prevent the next request from starting, but it does not stop or dispose a request that is already running. The current implementation tracks each operation and cleans up the timer, active workers, pool, and circuit breaker when the window closes. Also, 228 of the 646 added lines are tests. I appreciate the suggestion, but the shorter implementation gets there by changing the concurrency, isolation, and lifecycle behavior. For those reasons, I am going to keep the current design. |
fair points, and thanks for actually working through the comparison instead of waving it off, you're right that a single sequential worker gives up the bounded concurrency your pool provides, and a cancellation flag alone won't stop a request that's already in flight over the wire, that's a real gap I hadn't accounted for to be clear the sketch wasn't meant as a like for like replacement, just a lighter starting point aimed at the same isolation goal, something like a small bounded pool of two to four runspaces reusing the Invoke-WPFRunspace pattern instead of a dedicated one, with cancellation handled through a CancellationTokenSource passed into the HTTP calls so in flight requests actually abort, that would keep real concurrency and real cancellation while trimming some of the added surface area worth weighing that against scale too, applications json sits at 215 entries, not the thousands a heavier isolated pipeline is usually justified for, at that size the failure modes you're guarding against, several workers finishing at once, a machine blocking every request, are real but rare enough in practice that a smaller bounded pool with proper cancellation could likely absorb them without the dedicated C# circuit breaker and the six file split, so the six hundred plus lines might be buying more resilience than the current list actually calls for your call either way, I respect the work here and the points you raised are genuinely valid and impressive, happy to open a separate PR to compare concretely if that would help, otherwise no objection to shipping as is |
|
Moving this back to draft while I make a few small changes to the favicon scheduling and initial UI rendering. The core concurrency and cleanup design will remain intact. |
Render app cards with fallback letters before starting favicon work. Feed the dedicated pool from a bounded queue while preserving the circuit breaker, operation tracking, and shutdown cleanup.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
… after benchmarking
There was a problem hiding this comment.
🧹 Nitpick comments (1)
functions/private/Initialize-WinUtilFaviconRunspacePool.ps1 (1)
29-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid failing favicon initialization on an invalid environment value.
If
NUMBER_OF_PROCESSORSis non-numeric, the[int]cast throws before the pool is created.Start-WinUtilFaviconLoadingthen clears$sync.FaviconQueue, so favicon loading stops for the session. Use a validated value with a runtime fallback.Proposed fix
- $maxthreads = [Math]::Max(1, [int]$env:NUMBER_OF_PROCESSORS) + $processorCount = 0 + if (-not [int]::TryParse($env:NUMBER_OF_PROCESSORS, [ref]$processorCount)) { + $processorCount = [System.Environment]::ProcessorCount + } + $maxThreads = [Math]::Max(1, $processorCount)🤖 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 `@functions/private/Initialize-WinUtilFaviconRunspacePool.ps1` around lines 29 - 40, Update Initialize-WinUtilFaviconRunspacePool’s processor-count handling to validate NUMBER_OF_PROCESSORS before converting it to an integer, and use a safe runtime fallback when the environment value is missing or non-numeric. Ensure favicon runspace pool creation continues with at least one thread so Start-WinUtilFaviconLoading does not lose the queued work.
🤖 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.
Nitpick comments:
In `@functions/private/Initialize-WinUtilFaviconRunspacePool.ps1`:
- Around line 29-40: Update Initialize-WinUtilFaviconRunspacePool’s
processor-count handling to validate NUMBER_OF_PROCESSORS before converting it
to an integer, and use a safe runtime fallback when the environment value is
missing or non-numeric. Ensure favicon runspace pool creation continues with at
least one thread so Start-WinUtilFaviconLoading does not lose the queued work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77d766cd-8ad4-4c92-9fd7-6a7e6b88afa1
📒 Files selected for processing (2)
functions/private/Initialize-WinUtilFaviconRunspacePool.ps1pester/favicon-loading.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- pester/favicon-loading.Tests.ps1
3 participants
Type of Change
Description
This changes how application favicons are loaded on the Install tab. Previously, favicon loading started while application entries were being created, causing favicon setup and request submission to run synchronously, blocking the app for some time.
Application entries now render completely with fallback letters before favicon work begins. Once rendering finishes, a lightweight pending queue feeds a dedicated favicon runspace pool. Only enough operations to fill the pool are active at once, and available capacity is refilled as requests complete.
The dedicated pool uses the machine's available logical processor count, with a minimum of one worker. It remains separate from WinUtil's general runspace pool, so favicon downloads do not compete with installs or other background operations, while preserving the concurrency and safety guarantees built around the dedicated pool.
A thread-safe circuit breaker stops additional requests after 8 consecutive failures. This prevents machines that block Google or have network problems from continuing through the remaining queue. Fallback letters remain visible when requests fail or the circuit opens.
Pending requests, active operations, the polling timer, runspace pool, and circuit breaker are cleaned up when WinUtil closes. This does not add local caching or change
applications.json.Demo
load_favicons_async.mp4
Verification
Manual verification confirmed that the application list renders without hanging before favicon loading begins.
Rough worker-count benchmarking on a 32-logical-processor machine:
Full Pester 5.8.0 suite passes.
Issue related to PR
N/A