Add warning when user is running in ARM translation mode #1560
Conversation
WalkthroughThis pull request introduces enhancements related to ARM64 architecture support and configuration flexibility. The changes span multiple files, adding a new configuration option to dismiss architecture warnings, implementing a warning mechanism for ARM64 translation, and updating documentation to provide more detailed platform requirements and guidance. The modifications aim to improve user experience and provide clearer information about application compatibility across different hardware architectures. Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/docs/faq.mdx (2)
40-40: Improve sentence structureThe sentence using "however" should be restructured for better readability.
-macOS and Windows both have compatibility layers that allow x64 applications to run on ARM computers. This helps more apps run on these systems while developers work to add native ARM support to their applications, however it comes with significant performance tradeoffs. +macOS and Windows both have compatibility layers that allow x64 applications to run on ARM computers. This helps more apps run on these systems while developers work to add native ARM support to their applications. However, it comes with significant performance tradeoffs.🧰 Tools
🪛 LanguageTool
[typographical] ~40-~40: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...native ARM support to their applications, however it comes with significant performance t...(HOWEVER_SENTENCE)
42-42: Fix hyphenation in compound modifierRemove the hyphen from "natively-compiled" as it's redundant with the -ly adverb.
-To get the best experience using Wave, it is recommended that you uninstall Wave and reinstall the version that is natively-compiled for your computer. +To get the best experience using Wave, it is recommended that you uninstall Wave and reinstall the version that is natively compiled for your computer.🧰 Tools
🪛 LanguageTool
[uncategorized] ~42-~42: The hyphen in natively-compiled is redundant.
Context: ... Wave and reinstall the version that is natively-compiled for your computer. You can find the rig...(ADVERB_LY_HYPHEN_FIX)
emain/platform.ts (1)
44-63: Consider enhancing the warning dialog with more specific guidanceThe implementation is solid, but the warning message could be more actionable:
- The message could specify which native version to download based on the architecture
- Consider adding a "Don't show again" button directly in the dialog
Consider this enhanced dialog implementation:
const dialogOpts: Electron.MessageBoxOptions = { type: "warning", - buttons: ["See documentation", "Dismiss"], + buttons: ["See documentation", "Don't show again", "Dismiss"], title: "Wave has detected a performance issue", - message: `Wave has detected that it is running in ARM64 translation mode.\n\nThis may cause performance issues.\n\nPlease download the native version of Wave for your architecture (${unameArch})`, + message: `Wave is running in ARM64 translation mode which may impact performance.\n\nRecommendation: Download the native ${unameArch} version from our website for optimal performance.\n\nWould you like to learn more or disable this warning?`, }; const choice = dialog.showMessageBoxSync(null, dialogOpts); if (choice === 0) { console.log("Opening documentation URL"); fireAndForget(() => shell.openExternal("https://docs.waveterm.dev")); -} else { +} else if (choice === 1) { + console.log("User chose to disable the warning"); + // Update the configuration to disable future warnings + fireAndForget(() => SetBaseConfigValue({ "app:dismissarchitecturewarning": true })); +} else { console.log("User dismissed the dialog"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Taskfile.yml(1 hunks)docs/docs/config.mdx(1 hunks)docs/docs/faq.mdx(1 hunks)docs/docs/gettingstarted.mdx(1 hunks)emain/emain.ts(2 hunks)emain/platform.ts(2 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/docs/faq.mdx
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...d by Wave to work and will not function outside of the app. ## Why does Wave warn me abou...
(OUTSIDE_OF)
[typographical] ~40-~40: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...native ARM support to their applications, however it comes with significant performance t...
(HOWEVER_SENTENCE)
[uncategorized] ~42-~42: The hyphen in natively-compiled is redundant.
Context: ... Wave and reinstall the version that is natively-compiled for your computer. You can find the rig...
(ADVERB_LY_HYPHEN_FIX)
🔇 Additional comments (12)
docs/docs/faq.mdx (2)
38-44: LGTM! Clear explanation of ARM translation warning
The new FAQ section effectively explains:
- Why the warning appears
- The performance implications
- The recommended solution
- How to disable the warning
🧰 Tools
🪛 LanguageTool
[typographical] ~40-~40: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...native ARM support to their applications, however it comes with significant performance t...
(HOWEVER_SENTENCE)
[uncategorized] ~42-~42: The hyphen in natively-compiled is redundant.
Context: ... Wave and reinstall the version that is natively-compiled for your computer. You can find the rig...
(ADVERB_LY_HYPHEN_FIX)
46-59: LGTM! Well-structured beta builds instructions
The instructions for joining beta builds are clear and cover both package manager and Snap installation methods.
pkg/wconfig/metaconsts.go (1)
11-11: LGTM! Well-placed configuration constant
The new configuration constant follows the established naming pattern and is appropriately placed in the app configuration section.
docs/docs/gettingstarted.mdx (2)
17-45: LGTM! Comprehensive platform requirements section
The new platform requirements section effectively:
- Lists supported architectures per platform
- Specifies OS version requirements
- Includes relevant notes about ARM support
33-33: Verify the issue link is accurate
Please ensure the referenced GitHub issue for Windows ARM Support is the correct and most up-to-date issue.
✅ Verification successful
Issue link is accurate and relevant
The referenced GitHub issue #928 exists in the wavetermdev/waveterm repository, is currently open, and specifically addresses Windows ARM support, which aligns perfectly with the documentation's context about ARM64 support being blocked by upstream dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the GitHub issue exists and is relevant
gh issue view 928 --repo wavetermdev/waveterm
Length of output: 502
emain/platform.ts (1)
4-5: LGTM: Clean import statements
The imports are correctly organized, bringing in necessary Electron modules and utility functions.
Taskfile.yml (1)
99-99: LGTM: Enhanced build flexibility
The addition of {{.CLI_ARGS}} to the electron-builder command enables passing platform-specific build arguments, which is essential for ARM architecture support.
pkg/wconfig/settingsconfig.go (1)
36-38: LGTM: Well-structured configuration addition
The new configuration field follows the established patterns:
- Consistent field alignment
- Proper JSON tag naming
- Appropriate use of omitempty for optional setting
emain/emain.ts (2)
50-50: LGTM: Clean import of the architecture check function.
The function is imported alongside other related platform utilities.
592-592: LGTM: Appropriate placement of architecture check.
The function is called at the right time in the startup sequence:
- After full configuration is loaded (needed for checking dismissal settings)
- Before UI initialization (allows for early warning)
docs/docs/config.mdx (1)
29-29: LGTM: Well-documented configuration option.
The new configuration key is:
- Properly formatted and consistent with other entries
- Includes clear type specification
- Contains helpful description
- Links to FAQ for detailed information
frontend/types/gotypes.d.ts (1)
621-621: LGTM: Properly typed configuration setting.
The type definition is:
- Correctly marked as optional using
? - Properly typed as boolean
- Consistent with the documented configuration key
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/docs/faq.mdx (1)
44-44: Consider enhancing the configuration instructions.While the current instructions point to the configuration documentation, it would be more user-friendly to include a complete JSON example, similar to other FAQs in this document.
Consider adding:
-You can disable this warning by setting `app:dismissarchitecturewarning=true` in [your configurations](./config.mdx). +You can disable this warning by setting `app:dismissarchitecturewarning=true` in [your configurations](./config.mdx): + +```json +{ + "app:dismissarchitecturewarning": true +} +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/docs/faq.mdx(1 hunks)emain/platform.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- emain/platform.ts
🧰 Additional context used
🪛 LanguageTool
docs/docs/faq.mdx
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...d by Wave to work and will not function outside of the app. ## Why does Wave warn me abou...
(OUTSIDE_OF)
🔇 Additional comments (3)
docs/docs/faq.mdx (3)
31-31: LGTM!
The JSON configuration example is correctly formatted with properly escaped backslashes.
36-36: LGTM!
🧰 Tools
🪛 LanguageTool
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...d by Wave to work and will not function outside of the app. ## Why does Wave warn me abou...
(OUTSIDE_OF)
46-59: LGTM!
The beta builds FAQ is well-structured with clear instructions for different installation methods and proper code examples.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
44-47: Consider adding a note about ARM translation mode for WindowsSince this PR introduces ARM translation mode warnings, it would be helpful to add a note about Windows on ARM64 running in translation mode. This would provide upfront clarity for ARM-based Windows users.
Wave Terminal runs on the following platforms: - macOS 11 or later (arm64, x64) -- Windows 10 1809 or later (x64) +- Windows 10 1809 or later (x64, arm64 via translation) - Linux based on glibc-2.28 or later (Debian 10, RHEL 8, Ubuntu 20.04, etc.) (arm64, x64)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🔇 Additional comments (1)
README.md (1)
50-54: LGTM! Clear separation of WSH helper platform requirements
The addition of separate WSH helper platform requirements provides clear distinction between Wave Terminal and WSH helper architecture support. The detailed Linux kernel version requirements for different architectures are particularly helpful.
Let's verify these platform requirements against the codebase:
✅ Verification successful
Let me gather more information about the build configurations and architecture support.
Let me check the release configurations and build scripts to verify the platform support.
Based on the gathered information, I can now generate the final response:
Platform requirements in README.md are accurate and consistent with codebase
The WSH helper platform requirements are verified through:
- Build configurations in Taskfile.yml support all listed architectures (arm64, x64)
- Platform-specific code in
pkg/util/shellutil/shellutil.gohandles architecture normalization (amd64 → x64) - Build constraints in Go files (
//go:build) properly handle Windows/Unix separation - Runtime checks via
runtime.GOOSandruntime.GOARCHsupport all listed platforms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify platform requirements are consistent with build configurations
# Check build constraints and platform-specific code
rg -g '*.go' "GOOS|GOARCH" --type go
# Check for WSH helper related platform checks
rg -g '*.go' "runtime.GOOS|runtime.GOARCH" --type go
Length of output: 2265
Script:
#!/bin/bash
# Check build tags and constraints
rg -g '*.go' "^//\s*\+build" --type go
# Check Go files for build constraints
rg -g '*.go' "^//go:build" --type go
# Look for architecture-specific code or build settings
fd -e yaml -e yml | xargs rg "GOARCH|arm64|amd64|x64"
# Check for any build or platform configuration files
fd -e json -e toml | xargs rg "GOARCH|arm64|amd64|x64"
Length of output: 2454
Script:
#!/bin/bash
# Check release configurations and build scripts
fd -e sh -e bash | xargs rg "GOARCH|arm64|amd64|x64"
# Look for platform-specific code in WSH helper
rg -g 'cmd/wsh/**/*.go' "runtime.GOOS|runtime.GOARCH|linux|windows|darwin"
# Check for kernel version checks
rg "kernel|version" -g '*.go' -A 3 -B 3
Length of output: 53973
Adds warning and docs to let users know when they're running in ARM translation mode. Also lets them configure to always dismiss if they don't care.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores