discord-arm: Add version 1.0.11#16905
discord-arm: Add version 1.0.11#16905zeldrisho wants to merge 1 commit intoScoopInstaller:masterfrom
Conversation
WalkthroughThis PR adds ARM64 support for Discord by introducing a new manifest file ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
All changes look good. Wait for review from human collaborators. discord-arm
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bucket/discord-arm.jsonscripts/discord-arm/makelnk.ps1
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WindowsPowerShell
🔇 Additional comments (7)
bucket/discord-arm.json (3)
16-23: Pre-install steps follow Squirrel installer conventions correctly.The extraction and file organization logic is appropriate for Discord's Squirrel-based packaging. The script copies
makelnk.ps1from the bucket's scripts directory to enable the notification fix mentioned in notes.
33-43: Checkver and autoupdate configuration is correct.The regex
arm64/([\\d.]+)successfully extracts the version from the Discord manifest API response, and the autoupdate URL structure is properly formed with valid $version substitution.
1-15: Manifest metadata, architecture block, and URL are correct.The manifest follows standard Scoop conventions with proper metadata fields. The SHA256 hash format (64 hex characters) is correct, and the
#/dl.7zURL fragment instructs Scoop to treat the downloaded EXE as a 7z archive. The download URL is accessible and returns HTTP 200.scripts/discord-arm/makelnk.ps1 (4)
6-14: Parameters correctly configured for Discord Squirrel installer.The
AppUserModelIDvaluecom.squirrel.Discord.Discordis the standard identifier used by Discord for Windows notification grouping, which is essential for proper toast notification functionality.
80-107: PropVariant implementation handles memory correctly.The
PropVariantclass properly implementsIDisposablewith finalizer for memory cleanup of the CoTaskMem-allocated string. The VT_LPWSTR (31) handling is correct.One minor note: The
FieldOffsetlayout assumes standard Windows x64 alignment where the pointer is at offset 8. This is correct for the target ARM64 Windows platform.
109-142: ShortcutCreator implementation is correct.The implementation correctly:
- Creates the ShellLink COM object
- Sets standard shortcut properties
- Uses the correct
PKEY_AppUserModel_IDproperty key ({9F4C2855-9F79-4B39-A8D0-E1D42DE1D5F3}, 5)- Saves via
IPersistFileinterface
156-176: Execution logic is well-structured.The script properly:
- Ensures the destination directory exists before creating the shortcut
- Provides informative console output for debugging
- Wraps the shortcut creation in try-catch for graceful error handling
| "notes": [ | ||
| "Fix autostart: re-enable startup in Discord settings", | ||
| "Fix notifications by running: $dir\\makelnk.ps1" | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the actual content of the file being reviewed
if [ -f "bucket/discord-arm.json" ]; then
echo "=== discord-arm.json lines 6-9 ==="
sed -n '6,9p' bucket/discord-arm.json
echo ""
fi
# Search for $dir usage in notes across all Scoop manifests
echo "=== Checking $dir usage in notes fields ==="
rg -n '\$dir' bucket/*.json -B2 -A2 | grep -E 'notes|$dir' | head -40Repository: ScoopInstaller/Extras
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Look for notes sections in manifests to see patterns of variable usage
echo "=== Checking how variables are used in notes sections ==="
rg -B5 '"notes"' bucket/*.json | head -80Repository: ScoopInstaller/Extras
Length of output: 4407
🏁 Script executed:
#!/bin/bash
# Check if discord-arm.json file exists and show actual content
echo "=== Looking for discord-arm.json ==="
find . -name "discord-arm.json" -type f
# Check the actual file if found
if [ -f "./bucket/discord-arm.json" ]; then
echo ""
echo "=== Content of discord-arm.json ==="
cat -n ./bucket/discord-arm.json | head -20
fi
# Search specifically for $dir in notes fields
echo ""
echo "=== Searching for \$dir variable in notes sections ==="
rg -B3 'notes' bucket/*.json | rg -B3 -A3 '\$dir'Repository: ScoopInstaller/Extras
Length of output: 11736
🌐 Web query:
Scoop package manager $dir variable expansion in notes field behavior
💡 Result:
Short answer: Scoop does not expand manifest-installation variables (like $dir) inside the notes field. The notes field is treated as a static string/array to display after install; variable expansion is documented only for scripts/args/pre/post install and similar places (e.g. $dir, $persist_dir, $version). Use a post_install command (which supports $dir) to print a message that includes the expanded path. [1][2]
Sources:
- Scoop App Manifests (notes vs. variables available to scripts/args). [1]
- Scoop Concepts / pre/post_install docs (lists $dir and other variables available to install scripts). [2]
Remove $dir variable from notes; use post_install script instead.
Scoop does not expand variables like $dir in the notes field—it treats notes as static text displayed after installation. Users will see the literal $dir\makelnk.ps1 instead of the actual path, making the instruction unusable.
Move the notification fix to a post_install script using PowerShell to output the expanded path, or provide explicit instructions (e.g., "Run <your-install-path>\makelnk.ps1").
🤖 Prompt for AI Agents
In bucket/discord-arm.json around lines 6 to 9, the notes entry uses the literal
`$dir\makelnk.ps1` which Scoop will not expand; remove the `$dir` reference from
notes and instead add a post_install script that runs a PowerShell command to
resolve the install directory and either execute or print the full path to
makelnk.ps1 for the user. Update notes to a generic instruction if needed (e.g.,
"Run makelnk.ps1 in the install directory") and implement a post_install
PowerShell snippet that expands $env:APPDATA or uses Scoop's environment
variables to output or call the expanded "<install-path>\makelnk.ps1".
| try { | ||
| Add-Type -TypeDefinition $code -Language CSharp -ErrorAction Stop | ||
| } catch { | ||
| # Ignore error if type already exists (e.g. running script multiple times in same session) | ||
| if ($_.Exception.GetType().Name -ne "TypeLoadException") { | ||
| Write-Error "Definition error: $_" | ||
| } | ||
| } |
There was a problem hiding this comment.
Exception type check may not work as intended.
When Add-Type encounters an already-defined type, it typically throws an InvalidOperationException or a compilation error, not a TypeLoadException. The current check on line 151 may not correctly suppress the "type already exists" error.
🔎 Proposed fix
try {
Add-Type -TypeDefinition $code -Language CSharp -ErrorAction Stop
} catch {
# Ignore error if type already exists (e.g. running script multiple times in same session)
- if ($_.Exception.GetType().Name -ne "TypeLoadException") {
+ if ($_.Exception.Message -notmatch "already exists") {
Write-Error "Definition error: $_"
}
}📝 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.
| try { | |
| Add-Type -TypeDefinition $code -Language CSharp -ErrorAction Stop | |
| } catch { | |
| # Ignore error if type already exists (e.g. running script multiple times in same session) | |
| if ($_.Exception.GetType().Name -ne "TypeLoadException") { | |
| Write-Error "Definition error: $_" | |
| } | |
| } | |
| try { | |
| Add-Type -TypeDefinition $code -Language CSharp -ErrorAction Stop | |
| } catch { | |
| # Ignore error if type already exists (e.g. running script multiple times in same session) | |
| if ($_.Exception.Message -notmatch "already exists") { | |
| Write-Error "Definition error: $_" | |
| } | |
| } |
🤖 Prompt for AI Agents
In scripts/discord-arm/makelnk.ps1 around lines 147 to 154, the catch currently
only ignores exceptions named "TypeLoadException" but Add-Type commonly throws
InvalidOperationException or compilation errors when the type already exists;
update the catch to detect and suppress those cases by checking for multiple
exception types (e.g., "InvalidOperationException", "TypeLoadException") and/or
inspecting $_.Exception.Message for indicators like "already exists" or "type"
duplication, and only call Write-Error for other errors so legitimate failures
are still reported.
2 participants
Closes #16636, portapps/discord-portable/issues/137
Relates to #16316
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.