Revert pull request #1437 #1441
Conversation
|
This PR changes 617 lines. If it contains multiple concerns, consider splitting it. Large PRs are fine when the changes are mechanical, intentionally grouped, or belong together as part of the same feature. Please make sure the PR description gives reviewers enough context about what matters most to review. |
📝 WalkthroughWalkthroughThis PR overhauls Windows context-menu registration/unregistration for both Windows 10 (legacy shell, registry-based) and Windows 11 (MSIX sparse package), updates native COM shell extension code and build settings, softens installer failure handling, and adds timestamp-based file modification detection to backup diff logic with corresponding tests. ChangesWindows Context Menu Extension
Backup File Diff Timestamp Comparison
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant RegisterScript as register-context-menu-extension.ps1
participant Registry
participant AppxPackage
Installer->>RegisterScript: run script
RegisterScript->>RegisterScript: Get-WindowsBuild
alt relaunch needed
RegisterScript->>RegisterScript: Invoke-NativePowerShellRegistration
end
alt Windows 10 build
RegisterScript->>Registry: Register-Windows10ContextMenuExtension
else Windows 11 build
RegisterScript->>AppxPackage: Register-Windows11ContextMenuExtension
end
RegisterScript->>RegisterScript: Write-RegistrationLog
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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.
🧹 Nitpick comments (2)
src/apps/backups/diff/calculate-files-diff.test.ts (1)
35-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
isLocalFileNewerfallback branches.The two new tests cover the main paths (local newer, timestamps equal), but the fallback branches in
isLocalFileNewerare untested:
remote.modificationTimemissing, falls back toremote.updatedAt- Both
modificationTimeandupdatedAtmissing → returnstrue- Invalid/unparseable
remoteTimestring →Number.isNaNpath- Missing
local.stats.mtime→ would crash (see related NPE concern)Adding these cases would protect against regressions if the fallback logic changes.
🤖 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 `@src/apps/backups/diff/calculate-files-diff.test.ts` around lines 35 - 90, The new tests in calculateFilesDiff only cover the primary timestamp comparisons, so add coverage for the fallback logic in isLocalFileNewer. Include cases where remote.modificationTime is missing and remote.updatedAt is used, where both remote timestamps are missing and the function returns true, and where the remote time string is invalid so the Number.isNaN path is exercised. Also add a guard-rail test for missing local.stats.mtime to document the expected failure mode or prevent an unexpected crash.packages/context-menu/src/context_menu.cpp (1)
170-201: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: global bitmap cache is not thread-safe.
ContextMenuBitmapis a process-global lazily initialized fromGetContextMenuBitmap(). Under theApartmentthreading model, concurrent STA threads could both observe a null cache and each create a bitmap, leaking oneHBITMAP. A one-time init (InitOnceExecuteOnce) or interlocked compare-exchange would remove the race. Low priority given the small window and single-leak impact.🤖 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 `@packages/context-menu/src/context_menu.cpp` around lines 170 - 201, The lazy global cache in GetContextMenuBitmap is not thread-safe, so multiple Apartment/STA threads can race on ContextMenuBitmap and create duplicate HBITMAPs. Update the one-time initialization in GetContextMenuBitmap to use a thread-safe guard such as InitOnceExecuteOnce or an interlocked compare-exchange around the bitmap creation and assignment, ensuring only one bitmap is created and cached.
🤖 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 `@packages/context-menu/src/context_menu.cpp`:
- Around line 170-201: The lazy global cache in GetContextMenuBitmap is not
thread-safe, so multiple Apartment/STA threads can race on ContextMenuBitmap and
create duplicate HBITMAPs. Update the one-time initialization in
GetContextMenuBitmap to use a thread-safe guard such as InitOnceExecuteOnce or
an interlocked compare-exchange around the bitmap creation and assignment,
ensuring only one bitmap is created and cached.
In `@src/apps/backups/diff/calculate-files-diff.test.ts`:
- Around line 35-90: The new tests in calculateFilesDiff only cover the primary
timestamp comparisons, so add coverage for the fallback logic in
isLocalFileNewer. Include cases where remote.modificationTime is missing and
remote.updatedAt is used, where both remote timestamps are missing and the
function returns true, and where the remote time string is invalid so the
Number.isNaN path is exercised. Also add a guard-rail test for missing
local.stats.mtime to document the expected failure mode or prevent an unexpected
crash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58d519af-275e-46d1-a9d5-a6a5e36e6362
📒 Files selected for processing (9)
assets/context-menu-installer.nshpackages/context-menu/InternxtContextMenu.vcxprojpackages/context-menu/InternxtContextMenuHost.vcxprojpackages/context-menu/register-context-menu-extension.ps1packages/context-menu/src/context_menu.cpppackages/context-menu/unregister-context-menu-extension.ps1src/apps/backups/backups.test.tssrc/apps/backups/diff/calculate-files-diff.test.tssrc/apps/backups/diff/calculate-files-diff.ts
|
2 participants
This pr reverts #1437 so we add back #1433 and #1430
Summary by CodeRabbit
New Features
Bug Fixes