Refactor core UI rendering, search matching, and service maintenance loops - #4906
Refactor core UI rendering, search matching, and service maintenance loops#4906vyas-devgna wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request updates search, system checks, tweak execution, ISO handling, logging, UI grouping, update repair, dialog setup, and parser tests. It adds literal matching, explicit loops, timeout handling, fallback cleanup, and consolidated state handling. ChangesPowerShell workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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: 4
🧹 Nitpick comments (1)
functions/private/Find-TweaksByNameOrDescription.ps1 (1)
69-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
$matchesto avoid PowerShell's automatic variable. Use$isMatch; PSScriptAnalyzer flags assignments to$matches, which stores regex results.🤖 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/Find-TweaksByNameOrDescription.ps1` around lines 69 - 72, Rename the local `$matches` variable in `Find-TweaksByNameOrDescription` to `$isMatch`, updating both its assignment and the subsequent conditional reference. Preserve the existing search logic and matching behavior.
🤖 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/Find-TweaksByNameOrDescription.ps1`:
- Around line 34-44: Update the reset branch in Find-TweaksByNameOrDescription
so it restores each category according to its collapsed-label prefix instead of
making every item visible. Reuse the approach from Find-AppsByNameOrDescription:
detect labels starting with "+", keep those category items collapsed, and ensure
the label prefix remains consistent with item visibility.
In `@functions/private/Invoke-WinUtilCurrentSystem.ps1`:
- Around line 104-110: Update the service validation logic in
Invoke-WinUtilCurrentSystem so a missing service returned by Get-Service is
treated as a mismatch. Clear $allMatch and exit the validation loop whenever
$Service is null or its StartType differs from $tweak.StartupType; preserve the
existing matching behavior for present services with the expected startup type.
In `@functions/private/Invoke-WinUtilISO.ps1`:
- Around line 62-67: Update the timeout handling in the ISO mount flow around
Mount-DiskImage and the $mountElapsed check to dismount the mounted ISO before
raising the timeout error. Invoke the existing dismount mechanism for the target
image, then throw the same timeout message, while preserving normal mounting
behavior when the drive letter appears.
In `@functions/private/Test-WinUtilPackageManager.ps1`:
- Around line 18-24: Update the package-manager selection and validation in
Test-WinUtilPackageManager so passing both -winget and -choco cannot silently
check only winget; either reject the mutually exclusive combination or evaluate
both requested managers and return "installed" only when all requested commands
are available, while preserving the existing single-switch results.
---
Nitpick comments:
In `@functions/private/Find-TweaksByNameOrDescription.ps1`:
- Around line 69-72: Rename the local `$matches` variable in
`Find-TweaksByNameOrDescription` to `$isMatch`, updating both its assignment and
the subsequent conditional reference. Preserve the existing search logic and
matching behavior.
🪄 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: fb0f9660-cec9-48e4-81b6-214661337bc8
📒 Files selected for processing (12)
functions/private/Find-AppsByNameOrDescription.ps1functions/private/Find-TweaksByNameOrDescription.ps1functions/private/Invoke-WinUtilCurrentSystem.ps1functions/private/Invoke-WinUtilISO.ps1functions/private/Invoke-WinUtilTweaks.ps1functions/private/Show-CustomDialog.ps1functions/private/Test-WinUtilPackageManager.ps1functions/private/Write-WinUtilLog.ps1functions/public/Invoke-WPFFixesUpdate.ps1functions/public/Invoke-WPFUIElements.ps1pester/sanity.Tests.ps1scripts/main.ps1
| $cmd = if ($winget) { "winget" } else { "choco" } | ||
| if (Get-Command $cmd -ErrorAction SilentlyContinue) { | ||
| Write-Host "$cmd is installed" -ForegroundColor Green | ||
| return "installed" | ||
| } else { | ||
| Write-Host "$cmd is not installed" -ForegroundColor Red | ||
| return "not-installed" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle both package-manager switches.
When callers pass -winget -choco, $cmd is always "winget". The function ignores the Chocolatey check and can return "installed" although Chocolatey is not installed.
Either validate that exactly one switch is set, or check both commands and return "installed" only when both requested package managers are installed.
🤖 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/Test-WinUtilPackageManager.ps1` around lines 18 - 24,
Update the package-manager selection and validation in
Test-WinUtilPackageManager so passing both -winget and -choco cannot silently
check only winget; either reject the mutually exclusive combination or evaluate
both requested managers and return "installed" only when all requested commands
are available, while preserving the existing single-switch results.
![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: af9d24ea5e
ℹ️ 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".
| do { | ||
| Start-Sleep -Milliseconds 500 | ||
| $mountElapsed += 0.5 | ||
| if ($mountElapsed -ge $mountTimeout) { throw "ISO mount timed out after $($mountTimeout)s - drive letter never appeared." } |
There was a problem hiding this comment.
When Mount-DiskImage succeeds but Windows does not assign a drive letter within 30 seconds, this new timeout throws directly into the catch block and the catch path does not dismount the image. In environments with automount disabled or slow volume arrival, the UI is re-enabled while the ISO remains attached, so a retry can run against stale mounted media; dismount the image before throwing or add cleanup in the catch/finally path.
Useful? React with 👍 / 👎.
| }) | ||
| Add-Content -Path (Join-Path $workDir "WinUtil_Win11ISO.log") -Value "[$ts] $msg" | ||
| # Write to host only; transcript captures it without file-locking conflicts | ||
| Write-Host "[$ts] $msg" |
There was a problem hiding this comment.
During Win11 ISO modification and cleanup, this replaces the per-workdir WinUtil_Win11ISO.log write with Write-Host, so completed or failed ISO runs no longer leave a persistent log beside the copied media. If the UI closes or a user resumes/export-diagnoses a prior work directory, the detailed DISM/robocopy/customization history is gone; keep writing the workdir log while avoiding only the transcript-owned file.
Useful? React with 👍 / 👎.
ChrisTitusTech
left a comment
ChrisTitusTech
left a comment
There was a problem hiding this comment.
Feedback needs to be resolved or this PR will be closed out.
- Find-TweaksByNameOrDescription: respect collapsed category state on search reset (mirrors Find-AppsByNameOrDescription); rename $matches to $isMatch to avoid shadowing the PS automatic variable - Invoke-WinUtilCurrentSystem: treat a missing service as a mismatch instead of silently passing validation - Invoke-WinUtilISO: dismount ISO before throwing timeout error to prevent stale mounts; restore per-workdir log file for diagnostics - Test-WinUtilPackageManager: check both managers when both -winget and -choco switches are passed - Invoke-WPFFixesUpdate: restore per-service PercentComplete in the Stop-Service loop; abort on failure instead of silently continuing
|
All review feedback addressed in b4b04e6: CodeRabbit + Codex feedback:
@mewclouds feedback:
|
coderabbitai
Bot
added
bug
|
Thanks! Will review shortly. |
|
Could the ISO log write directly to |
Both fair, thanks. The workdir log is still gated on Win11ISOWorkDir which is only set after the modify run succeeds, so I will point Log at the workDir the runspace already receives and create the folder before the first line, and since the failure path deletes workDir I will preserve the log before cleanup. For the test, the empty search case builds the category as "+ Privacy" and still expects everything visible, so I will update it to keep collapsed categories collapsed and add a "- Privacy" case for the other branch. Pushing shortly. |
|
Both fixed in d20279d. The log now goes to The empty-search tweak test now covers both branches — Thanks for the catch. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pester/search-filter.Tests.ps1 (1)
395-414: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the category markers after resetting the search.
The fixture uses
+ Privacyand- Explorerto represent collapsed and expanded states, but the test checks only visibility. A regression that changes either marker can pass. Add content assertions for both categories.Proposed test assertions
$collapsedCategory.Label.Visibility | Should -Be ([Windows.Visibility]::Visible) + $collapsedCategory.Label.Content | Should -Be "+ Privacy" $collapsedItem.Visibility | Should -Be ([Windows.Visibility]::Collapsed) $expandedCategory.Border.Visibility | Should -Be ([Windows.Visibility]::Visible) $expandedCategory.Label.Visibility | Should -Be ([Windows.Visibility]::Visible) + $expandedCategory.Label.Content | Should -Be "- Explorer" $expandedItem.Visibility | Should -Be ([Windows.Visibility]::Visible)🤖 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/search-filter.Tests.ps1` around lines 395 - 414, Update the test “restores category labels and respects collapsed category state for empty search” to assert each category label’s Content after Find-TweaksByNameOrDescription resets the search. Verify the collapsed category retains “+ Privacy” and the expanded category retains “- Explorer”, alongside the existing visibility assertions.
🤖 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/search-filter.Tests.ps1`:
- Around line 395-414: Update the test “restores category labels and respects
collapsed category state for empty search” to assert each category label’s
Content after Find-TweaksByNameOrDescription resets the search. Verify the
collapsed category retains “+ Privacy” and the expanded category retains “-
Explorer”, alongside the existing visibility assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15635d3a-b994-4486-b1aa-ad71e104143d
📒 Files selected for processing (3)
docs/src/content/docs/code-reference/architecture.mdxfunctions/private/Invoke-WinUtilISO.ps1pester/search-filter.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/private/Invoke-WinUtilISO.ps1
coderabbitai
Bot
added
ui update
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b9a01e78d
ℹ️ 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".
| $pct = [int](($i / $services.Count) * 100) | ||
| Write-Progress -Id 2 -ParentId 0 -Activity "Stopping Services" -Status "Stopping $svc..." -PercentComplete $pct | ||
| try { | ||
| Stop-Service -Name $svc -Force -ErrorAction Stop |
There was a problem hiding this comment.
When Stop-Service fails for one of these services (for example because the service is protected, absent, or the current elevated context still cannot control it; the Stop-Service docs note that -Force only helps with dependent services and service control still depends on permissions), this new terminating path aborts before the later block that restarts BITS, wuauserv, AppIDSvc, and CryptSvc. If an earlier service in the loop was already stopped, Windows Update Reset can leave the machine with update services stopped and the repair half-applied; collect/report the stop failure but still run the restart/cleanup path.
Useful? React with 👍 / 👎.
| $Service = Get-Service -Name $tweak.Name -ErrorAction SilentlyContinue | ||
| if (-not $Service -or $tweak.StartupType -ne $Service.StartType) { |
There was a problem hiding this comment.
On Windows editions/builds where a configured tweak service is absent (for example optional services such as CscService or MapsBroker), applying the tweak already only logs a warning and continues via Set-WinUtilService, but this detection path now marks the whole tweak as not applied whenever Get-Service returns nothing. That makes “Get Installed” leave a successfully applied tweak unchecked forever on those systems; keep missing services neutral as before, or align apply/detect semantics so absent services do not block detection.
Useful? React with 👍 / 👎.
…ce loops - Replace O(N^2) array concatenations with generic lists in GUI item rendering and tweak checks - Convert slow pipeline loops (ForEach-Object) to direct foreach runtime enumeration - Replace wildcard regex matches in app and tweak search with fast string index lookups - Add timeout protection and batch file cleanup in ISO mounting workflows - Prevent file-locking exceptions when logging within an active transcript session - Streamline Windows Update service repair routines and throttle progress updates during DLL reregistration - Ensure command quote resilience in sanity tests when invoking nested Windows PowerShell parsers
- Find-TweaksByNameOrDescription: respect collapsed category state on search reset (mirrors Find-AppsByNameOrDescription); rename $matches to $isMatch to avoid shadowing the PS automatic variable - Invoke-WinUtilCurrentSystem: treat a missing service as a mismatch instead of silently passing validation - Invoke-WinUtilISO: dismount ISO before throwing timeout error to prevent stale mounts; restore per-workdir log file for diagnostics - Test-WinUtilPackageManager: check both managers when both -winget and -choco switches are passed - Invoke-WPFFixesUpdate: restore per-service PercentComplete in the Stop-Service loop; abort on failure instead of silently continuing
Write the modify log to <workDir>.log in %TEMP% instead of a file gated on $sync["Win11ISOWorkDir"], which is only assigned after a successful run. The log now starts at the first line, before the work directory is created, and survives the cleanup that removes that directory, so early failures leave a diagnostic behind. Update the empty-search tweak test for the collapsed-category reset behavior and cover the expanded branch as well.
The reset branch leaves Label.Content alone while the search branch rewrites "+ X" to "- X". Without these assertions a reset that started rewriting the marker would desync the label from its collapsed items and still pass.
3b9a01e to
3e0fa36
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e0fa360bc
ℹ️ 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".
| $ic = $categoryBorder.Child.Children | Where-Object { $_ -is [Windows.Controls.ItemsControl] } | Select-Object -First 1 | ||
| if ($null -eq $ic) { continue } |
There was a problem hiding this comment.
When the real Tweaks/AppX tabs are rendered through Invoke-WPFUIElements, each category border's DockPanel contains a StackPanel directly (or a ScrollViewer in targets without an outer scroll viewer), not an ItemsControl. This new lookup therefore leaves $ic null and skips every category, so typing in the search box on the Tweaks or AppX tab becomes a no-op against the actual UI even though the synthetic tests use ItemsControl.
Useful? React with 👍 / 👎.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Find-TweaksByNameOrDescription.ps1`:
- Around line 106-108: Update the category expansion logic around
$categoryLabel.Content to record categories expanded by search in $sync before
changing the "+ " prefix to "- ". When the search is cleared, use that shared
state to restore each affected category’s "+ " prefix and collapsed items, while
preserving categories that were already expanded and using $sync for the shared
state and UI references.
🪄 Autofix
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: c7600339-7466-4ce4-88d1-905adee87230
📒 Files selected for processing (8)
docs/src/content/docs/code-reference/architecture.mdxfunctions/private/Find-AppsByNameOrDescription.ps1functions/private/Find-TweaksByNameOrDescription.ps1functions/private/Invoke-WinUtilCurrentSystem.ps1functions/private/Invoke-WinUtilTweaks.ps1functions/public/Invoke-WPFUIElements.ps1pester/search-filter.Tests.ps1scripts/main.ps1
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/src/content/docs/code-reference/architecture.mdx
- pester/search-filter.Tests.ps1
- scripts/main.ps1
- functions/public/Invoke-WPFUIElements.ps1
- functions/private/Invoke-WinUtilCurrentSystem.ps1
- functions/private/Invoke-WinUtilTweaks.ps1
3 participants
Type of Change
Description
A performance and correctness pass over a few hot paths.
Invoke-WPFUIElements,Invoke-WinUtilTweaks,Invoke-WinUtilCurrentSystem—List[object]instead of+=array growth, and no intermediate hashtable conversions.IndexOfmatching instead of wildcard-like, so[,*and?are treated as text. Clearing the search now leaves collapsed categories collapsed.Invoke-WinUtilISO— 30s mount timeout that dismounts before it throws, and the modify log writes beside the work directory so it exists from the first line and survives cleanup.Invoke-WPFFixesUpdate— one loop for the repeated service commands, and throttled progress during DLL re-registration so it stops flooding the UI thread.Write-WinUtilLog— skipAdd-ContentwhenStart-Transcriptalready owns the log file.Invoke-WinUtilCurrentSystem, andTest-WinUtilPackageManagerchecks both managers when-wingetand-chocoare passed together.Invoke-Pester -Path 'pester/*.Tests.ps1'passes 471/471 andCompile.ps1is clean.Issue related to PR