vscode: Correct file association icon paths#17196
vscode: Correct file association icon paths#17196Timabliss wants to merge 2 commits intoScoopInstaller:masterfrom
Conversation
WalkthroughAdds PowerShell post-install logic to compute a dynamic install target path (from Changes
Sequence Diagram(s)sequenceDiagram
participant Installer
participant PowerShell
participant FileSystem
participant Registry
Installer->>PowerShell: run post_install
PowerShell->>FileSystem: check installDir\bin\code for VERSIONFOLDER
alt VERSIONFOLDER found
PowerShell->>FileSystem: set targetPath = installDir\VERSIONFOLDER
else
PowerShell->>FileSystem: compute hash-based targetPath
end
PowerShell->>FileSystem: update .reg files (replace $codedir with targetPath or $dirpath for GitHub)
PowerShell->>Registry: import updated uninstall .reg files
alt other-version reg files present
PowerShell->>Registry: run regedit /u on matching uninstall .reg files
end
Installer->>Registry: on uninstall import uninstall-associations.reg
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Your changes did not pass all checks. Please address the issues in the manifest and comment starting with vscode
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bucket/vscode.json`:
- Around line 37-42: Reduce the indentation of the nested if/else block inside
the Test-Path section so lines assigning $targetDir and the subsequent
$content.Replace call align with the surrounding block: move the lines
containing "if ($_ -match 'github') {", "$targetDir = $dirpath", "} else {",
"$targetDir = $hashPath", "}" and "$content = $content.Replace('$codedir',
$targetDir)" left by 4 spaces to match the indentation of the Test-Path block
and surrounding statements; ensure $targetDir, $dirpath, $hashPath and the
$content.Replace('$codedir', $targetDir) call remain unchanged.
🧹 Nitpick comments (1)
bucket/vscode.json (1)
32-33: Hash directory detection is a reasonable heuristic, but consider tightening the pattern.The regex
^[a-z0-9]{10,}$(case-insensitive via-match) could match directories unrelated to VS Code's resource hash. If the hash is always a hex string of a known fixed length (e.g., the PR description shows an 8-char prefix591199df), you could narrow the pattern (e.g.,^[a-f0-9]{8,}$) to reduce false-positive risk.Also,
Get-ChildItemwithout-Forcewon't list hidden directories, which is fine here, but note that if multiple directories match,-First 1picks alphabetically — not necessarily the correct one. Worth confirming VS Code only ever creates a single such directory.
Timabliss
force-pushed
the
fix/vscode-icon
branch
from
bd1f3e3 to
bcd7e57
Compare
|
/verify |
|
All changes look good. Wait for review from human collaborators. vscode
|
Timabliss
force-pushed
the
fix/vscode-icon
branch
from
bcd7e57 to
937e8d4
Compare
|
/verify |
|
Your changes did not pass all checks. Please address the issues in the manifest and comment starting with vscode
|
Major refactor of the `post_install` logic to improve robustness and registry hygiene: 1. Deterministic Version Detection (Source of Truth): Instead of unreliable directory sniffing, we now parse the official `bin/code` shell script to extract the `VERSIONFOLDER` variable. This is the upstream source of truth for the version hash. 2. Dynamic Registry Template Injection: - Context Menus & Associations: Injected with the resolved hash path (e.g., `$dir\<hash>`). - GitHub Integration: Injected with the root path (`$dir`), as per standard behavior. - Handled via variable replacement in `.reg` templates during runtime. 3. Update-Triggered Registry Cleanup: Implemented a "Clean Slate" policy. During an update (detected by the presence of older version directories matching `^[0-9.]+$`), the script automatically runs uninstall routines for the previous registration. This prevents registry pollution and ensures that HKCU keys strictly correspond to the current installation.
Timabliss
force-pushed
the
fix/vscode-icon
branch
from
937e8d4 to
b7e7357
Compare
|
/verify |
|
All changes look good. Wait for review from human collaborators. vscode
|
|
Title: Description: Key Changes:
Ready for review! 🚀 |
z-Fng
changed the title
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 `@bucket/vscode.json`:
- Around line 68-77: The cleanup loop that builds $regFile in the block starting
with "$appRoot = Split-Path $dir -Parent" uses "regedit /s \"$regFile\"" which
is GUI-based and async; replace that call with the synchronous CLI "reg import
\"$regFile\"" to match the rest of the manifest. Locate the loop that iterates
over 'uninstall-associations', 'uninstall-context',
'uninstall-github-integration' (the $regFile variable) and swap the regedit
invocation for reg import so registry files are imported reliably under
UAC/elevation.
---
Duplicate comments:
In `@bucket/vscode.json`:
- Around line 46-47: The line containing "if ($_ -match 'github') {$tmppath =
$dirpath} else {$tmppath = $targetpath}" inside the same ForEach-Object block is
indented with 8 spaces while the other statements (e.g., the surrounding lines
that set $content and replace '$codedir') use 4 spaces; change that line's
indentation to 4 spaces so it matches the block's existing indentation style and
aligns with the other statements in the ForEach-Object.
| "$appRoot = Split-Path $dir -Parent", | ||
| "$otherVersions = @(Get-ChildItem $appRoot -Directory | Where-Object { $_.FullName -ne $dir -and $_.Name -ne 'current' -and ($_.Name -match '^[0-9.]+$') })", | ||
| "if ($otherVersions.Count -gt 0) {", | ||
| " 'uninstall-associations', 'uninstall-context', 'uninstall-github-integration' | ForEach-Object {", | ||
| " $regFile = \"$dir\\$_.reg\"", | ||
| " if (Test-Path $regFile) {", | ||
| " regedit /s \"$regFile\"", | ||
| " }", | ||
| " }", | ||
| "}" |
There was a problem hiding this comment.
Use reg import instead of regedit /s for consistency and reliability.
The uninstaller block (lines 83–85) uses reg import, but this cleanup block uses regedit /s. regedit is a GUI application — even with /s, it runs asynchronously and can behave differently under UAC/elevation. reg import is the CLI counterpart, runs synchronously, and is the standard used throughout Scoop manifests.
Proposed fix
"'uninstall-associations', 'uninstall-context', 'uninstall-github-integration' | ForEach-Object {",
"$regFile = \"$dir\\$_.reg\"",
"if (Test-Path $regFile) {",
- "regedit /s \"$regFile\"",
+ "reg import \"$regFile\"",
"}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "$appRoot = Split-Path $dir -Parent", | |
| "$otherVersions = @(Get-ChildItem $appRoot -Directory | Where-Object { $_.FullName -ne $dir -and $_.Name -ne 'current' -and ($_.Name -match '^[0-9.]+$') })", | |
| "if ($otherVersions.Count -gt 0) {", | |
| " 'uninstall-associations', 'uninstall-context', 'uninstall-github-integration' | ForEach-Object {", | |
| " $regFile = \"$dir\\$_.reg\"", | |
| " if (Test-Path $regFile) {", | |
| " regedit /s \"$regFile\"", | |
| " }", | |
| " }", | |
| "}" | |
| "$appRoot = Split-Path $dir -Parent", | |
| "$otherVersions = @(Get-ChildItem $appRoot -Directory | Where-Object { $_.FullName -ne $dir -and $_.Name -ne 'current' -and ($_.Name -match '^[0-9.]+$') })", | |
| "if ($otherVersions.Count -gt 0) {", | |
| " 'uninstall-associations', 'uninstall-context', 'uninstall-github-integration' | ForEach-Object {", | |
| " $regFile = \"$dir\\$_.reg\"", | |
| " if (Test-Path $regFile) {", | |
| " reg import \"$regFile\"", | |
| " }", | |
| " }", | |
| "}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bucket/vscode.json` around lines 68 - 77, The cleanup loop that builds
$regFile in the block starting with "$appRoot = Split-Path $dir -Parent" uses
"regedit /s \"$regFile\"" which is GUI-based and async; replace that call with
the synchronous CLI "reg import \"$regFile\"" to match the rest of the manifest.
Locate the loop that iterates over 'uninstall-associations',
'uninstall-context', 'uninstall-github-integration' (the $regFile variable) and
swap the regedit invocation for reg import so registry files are imported
reliably under UAC/elevation.
1 participant
Description
This PR fixes an issue where file association icons were missing because VS Code extracts resources into a dynamic hash directory (e.g.,
current\591199df...\resources), but the registry templates pointed to the static root.Changes
post_install:$codedirvariable in.regtemplates to use the hash path forinstall-associations.reg.install-github-integration.reg(as it requires the executable path).uninstaller:reg import "$dir\uninstall-associations.reg"which was missing, ensuring registry keys are properly cleaned up upon uninstall.Testing
vscodelocally using the modified manifest..regfiles are generated with the correct hash path..regfiles and verified that icons appear correctly in the context menu and file explorer.vscodeand verified that registry keys (includingCodeOSS)Summary by CodeRabbit
New Features
Bug Fixes