Improve registry backup safety and add optional backup skipping - #710
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds an optional registry-backup deployment setting, propagates ChangesRegistry backup option and propagation
Registry snapshot safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant Win11Debloat
participant InvokeAllChanges
participant Registry
User->>MainWindow: uncheck RegistryBackupCheckBox
MainWindow->>Win11Debloat: queue SkipRegistryBackup
Win11Debloat->>InvokeAllChanges: pass SkipRegistryBackup
InvokeAllChanges->>Registry: apply features without registry backup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Owner
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Scripts/Features/Restore-RegistryApplyState.ps1 (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep docstrings complete and in sync with current behavior.
As per coding guidelines, functions added or changed in PowerShell scripts must have comment-based docstrings that are kept in sync with their behavior. Several docstrings need updates to reflect the new changes or describe all parameters:
Scripts/Features/Restore-RegistryApplyState.ps1#L72-L75: Add the missing.PARAMETERdescription for$Snapshotand update the.SYNOPSISto mention that this function also validates subkey paths, not just values.Scripts/Features/Restore-RegistryApplyState.ps1#L101-L104: Add missing.PARAMETERdescriptions for$ParentPathand$ChildPath.Scripts/Features/Backup-RegistrySnapshotCapture.ps1#L198-L205: Update the.DESCRIPTIONto mention that the function will now throw an exception if a requested subkey cannot be read.Scripts/Features/Invoke-Changes.ps1#L299-L312: Update the.DESCRIPTIONto note that Phase 1 (Registry backup) is skipped whenSkipRegistryBackupis present.🤖 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 `@Scripts/Features/Restore-RegistryApplyState.ps1` around lines 72 - 75, The comment-based docstrings require synchronization across four locations: in Scripts/Features/Restore-RegistryApplyState.ps1 lines 72-75, update the function synopsis to mention validation of subkey paths as well as registry values and add a .PARAMETER description for $Snapshot; in lines 101-104, add .PARAMETER descriptions for $ParentPath and $ChildPath; in Scripts/Features/Backup-RegistrySnapshotCapture.ps1 lines 198-205, update the function .DESCRIPTION to state that unreadable requested subkeys cause an exception; and in Scripts/Features/Invoke-Changes.ps1 lines 299-312, document that Phase 1 registry backup is skipped when SkipRegistryBackup is present.Source: Coding guidelines
🤖 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 `@Scripts/Features/Restore-RegistryApplyState.ps1`:
- Around line 72-75: The comment-based docstrings require synchronization across
four locations: in Scripts/Features/Restore-RegistryApplyState.ps1 lines 72-75,
update the function synopsis to mention validation of subkey paths as well as
registry values and add a .PARAMETER description for $Snapshot; in lines
101-104, add .PARAMETER descriptions for $ParentPath and $ChildPath; in
Scripts/Features/Backup-RegistrySnapshotCapture.ps1 lines 198-205, update the
function .DESCRIPTION to state that unreadable requested subkeys cause an
exception; and in Scripts/Features/Invoke-Changes.ps1 lines 299-312, document
that Phase 1 registry backup is skipped when SkipRegistryBackup is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5669f08b-cad4-4793-a4f6-b7dc0ba0d6f1
📒 Files selected for processing (14)
Config/DefaultSettings.jsonSchemas/MainWindow.xamlScripts/Features/Backup-RegistrySnapshotCapture.ps1Scripts/Features/Invoke-Changes.ps1Scripts/Features/Restore-RegistryApplyState.ps1Scripts/GUI/Show-ImportExportConfigWindow.ps1Scripts/GUI/Show-MainWindow.ps1Scripts/Get.ps1Scripts/Helpers/Import-ConfigToParams.ps1Tests/Import-ConfigToParams.Tests.ps1Tests/Invoke-Changes.Tests.ps1Tests/Registry-SnapshotApply.Tests.ps1Tests/TestData/JsonFileLoading/ExportedConfig.WithSettings.jsonWin11Debloat.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)
Scripts/Features/Invoke-Changes.ps1 (1)
329-330: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
SkipRegistryBackupout of$applyIds.When the option is enabled,
SkipRegistryBackupis appended to$applyIdsand later treated as a feature byInvoke-ApplyFeatures. This creates a bogus apply step with an empty label and inflatestotalSteps. Exclude it alongsideCreateRestorePoint, and strengthenTests/Invoke-Changes.Tests.ps1to assert that only real feature IDs are passed.Proposed fix
if ($key -eq 'CreateRestorePoint') { continue } + if ($key -eq 'SkipRegistryBackup') { continue } $applyIds += $key🤖 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 `@Scripts/Features/Invoke-Changes.ps1` around lines 329 - 330, Exclude the SkipRegistryBackup key from $applyIds in the feature-ID collection logic alongside CreateRestorePoint, so Invoke-ApplyFeatures receives only real feature IDs. Strengthen the relevant Invoke-Changes tests to assert that the resulting IDs contain no SkipRegistryBackup and include only applicable features.
🤖 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 `@Scripts/Features/Invoke-Changes.ps1`:
- Around line 329-330: Exclude the SkipRegistryBackup key from $applyIds in the
feature-ID collection logic alongside CreateRestorePoint, so
Invoke-ApplyFeatures receives only real feature IDs. Strengthen the relevant
Invoke-Changes tests to assert that the resulting IDs contain no
SkipRegistryBackup and include only applicable features.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be4ec9b0-81b0-4f1e-b6f3-faccbcaf41cb
📒 Files selected for processing (1)
Scripts/Features/Invoke-Changes.ps1
…t and update tests for SkipRegistryBackup
This was referenced Jul 25, 2026
4 tasks
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
2 participants
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.
This should address the concerns raised in #686
Summary by CodeRabbit
SkipRegistryBackupflag (unchecked =>SkipRegistryBackup=true, checked =>SkipRegistryBackup=false), including updated options text and import persistence.-SkipRegistryBackupand pass it through toWin11Debloat.ps1, plus updatedImport-ConfigToParamsto readSkipRegistryBackupfrom exported configs.Invoke-AllChangesso the registry-backup phase (and step counting) runs only when registry backup is needed andSkipRegistryBackupis not set.Convert-RegistryKeyToSnapshotnow throws when an included child subkey can’t be opened; registry restore adds a full snapshot preflight check and restores more safely by enforcing direct parent→child relationships and avoiding the prior “force full tree” deletion behavior.SkipRegistryBackupand backup/restore safety (including unreadable subkey snapshot failure and new snapshot preflight validation), plus added new exported config/test data for skipping registry backup.Config/DefaultSettings.json(re-emitted the final closing-brace line).