[v3] Start At Login #3910
[v3] Start At Login #3910
Conversation
WalkthroughThe pull request introduces multiple enhancements and fixes across various platform-specific application files. Key updates include the addition of methods to manage application startup behavior on login for macOS, Linux, and Windows, alongside new options in the application configuration. The changelog has been updated to reflect these changes, including support for new features like Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
v3/pkg/application/application_linux.go (2)
301-301: Consider more restrictive file permissionsThe file permissions
0600are acceptable but could be more restrictive since this is an autostart entry that shouldn't be modified after creation.- file, err := os.OpenFile(autostartPath, os.O_RDWR|os.O_CREATE, 0600) + file, err := os.OpenFile(autostartPath, os.O_RDWR|os.O_CREATE, 0400)
307-307: Pre-compile the template at package initializationFor better performance and early error detection, consider pre-compiling the template at init time.
+var autostartTemplate = template.Must(template.New("autostart").Parse(`[Desktop Entry] +Name={{.Name}} +Comment=Autostart service for {{.Name}} +Type=Application +Exec={{.Cmd}} +Hidden=true +X-GNOME-Autostart-enabled=true +`)) + func (l *linuxApp) setStartAtLogin(enabled bool) error { // ... - t := template.Must(template.New("autostart").Parse(tpl)) - return t.Execute(file, struct { + return autostartTemplate.Execute(file, struct {v3/pkg/application/application_options.go (2)
117-118: Enhance documentation for StartAtLogin fieldWhile the basic purpose is clear, consider enhancing the documentation to:
- Describe platform-specific behaviors or limitations
- Note any prerequisites (e.g., proper app bundling on macOS)
- Reference related platform-specific methods
Example documentation:
- // StartAtLogin determines if the application should start when the user logs in + // StartAtLogin determines if the application should start when the user logs in. + // On macOS: Requires proper app bundling and uses LaunchServices + // On Windows: Adds a registry key under Run + // On Linux: Creates an XDG autostart desktop entry
117-118: Consider platform-specific startup configurationsThe current boolean field provides basic functionality, but platforms might benefit from additional configuration options. Consider extending the platform-specific option structs (
MacOptions,WindowsOptions,LinuxOptions) with startup-related configurations:type WindowsOptions struct { + // StartupRegistryKey allows customizing the registry key location for startup + // Default: "SOFTWARE\Microsoft\Windows\CurrentVersion\Run" + StartupRegistryKey string } type LinuxOptions struct { + // AutostartDesktopEntry allows customizing the .desktop file name + // Default: application name + AutostartDesktopEntry string } type MacOptions struct { + // LaunchServicesOptions contains additional launch options when starting at login + // Example: hide on launch + LaunchServicesOptions map[string]interface{} }v3/pkg/application/application_darwin.go (1)
412-425: Add documentation and improve error handlingThe implementation correctly checks the prerequisites for start-at-login functionality, but could benefit from better documentation and error handling.
Consider these improvements:
+// canStartAtLogin checks if the application can be registered as a login item. +// Returns true if: +// 1. The application has a valid bundle ID +// 2. The application is running from a proper macOS .app package structure func (m *macosApp) canStartAtLogin() bool { bundleID := mac.GetBundleID() if bundleID == "" { + m.parent.debug("Cannot start at login: missing bundle ID") return false } exe, err := os.Executable() if err != nil { + m.parent.debug("Cannot start at login: failed to get executable path: %v", err) return false } binName := filepath.Base(exe) + // Check if the app is running from a proper macOS .app package structure + // e.g., MyApp.app/Contents/MacOS/MyApp return strings.HasSuffix(exe, "/Contents/MacOS/"+binName) }v3/pkg/application/application_windows.go (1)
380-383: Enhance error handling and add path validationWhile the error handling is generally good, it could be improved with more descriptive messages and path validation.
Consider these improvements:
exePath, err := os.Executable() if err != nil { - return fmt.Errorf("failed to get executable path: %s", err) + return fmt.Errorf("failed to get application executable path: %w", err) +} + +// Validate executable path exists +if _, err := os.Stat(exePath); err != nil { + return fmt.Errorf("executable path is invalid: %w", err) } key, err := registry.OpenKey(registry.CURRENT_USER, `Software\Microsoft\Windows\CurrentVersion\Run`, registry.ALL_ACCESS) if err != nil { - return fmt.Errorf("failed to open registry key: %s", err) + return fmt.Errorf("failed to open Windows autostart registry key: %w", err) }Also applies to: 388-390
mkdocs-website/docs/en/changelog.md (1)
21-21: LGTM with a minor suggestion.The changelog entry correctly documents the new StartAtLogin feature. Consider enhancing it to explicitly mention the cross-platform support:
-Added Support for StartAtLogin by [ansxuman](https://github.com/ansxuman) in [#3910](https://github.com/wailsapp/wails/pull/3910) +Added Support for StartAtLogin (Windows, macOS, Linux) by [ansxuman](https://github.com/ansxuman) in [#3910](https://github.com/wailsapp/wails/pull/3910)v3/pkg/application/application.go (2)
349-350: Consider grouping related fields together.The
startAtLoginOnRunfield would be better placed near other startup/initialization related fields in the struct for better code organization.
1038-1044: Add documentation for SetStartAtLogin method.The method has good implementation but lacks documentation. Consider adding a doc comment explaining:
- The method's purpose
- Return value behavior (returns nil when not supported)
- Platform support limitations
Add this documentation:
+// SetStartAtLogin configures whether the application should start automatically when the user logs in. +// Returns nil if the feature is not supported on the current platform or configuration. +// Returns an error if the operation fails. func (a *App) SetStartAtLogin(enabled bool) error {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/pkg/application/application.go(5 hunks)v3/pkg/application/application_darwin.go(2 hunks)v3/pkg/application/application_linux.go(2 hunks)v3/pkg/application/application_options.go(1 hunks)v3/pkg/application/application_windows.go(3 hunks)
🔇 Additional comments (7)
v3/pkg/application/application_linux.go (1)
266-329: Verify desktop environment compatibility
The implementation follows the XDG autostart specification but should be tested across different desktop environments (GNOME, KDE, XFCE, etc.) to ensure consistent behavior.
✅ Verification successful
Implementation follows XDG specifications and is consistent across the codebase
The implementation in application_linux.go correctly follows the XDG autostart specification and is consistent with other desktop entry implementations in the codebase. The code:
- Uses the standard
.config/autostartdirectory as per XDG spec - Follows the same desktop entry format used in other parts of the codebase
- Properly handles file permissions and directory creation
- Is consistent with the experimental plugin implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for desktop environment specific autostart implementations
# Check for common desktop environment specific autostart locations
echo "Checking desktop environment specific autostart locations..."
for de in \
".config/autostart" \
".kde/Autostart" \
".kde4/Autostart" \
".config/plasma-workspace/autostart"
do
rg -l "Exec=" "$HOME/$de/*.desktop" 2>/dev/null
done
# Check if any desktop environments have their own autostart APIs
echo "Checking for desktop environment specific autostart APIs..."
for service in \
"org.gnome.SessionManager" \
"org.kde.ksmserver"
do
dbus-send --session --dest=$service \
--type=method_call --print-reply \
/ org.freedesktop.DBus.Introspectable.Introspect 2>/dev/null
done
Length of output: 1316
Script:
#!/bin/bash
# Let's verify the implementation against desktop environment standards and existing code
# Check if there are any other autostart implementations in the codebase
echo "Checking for other autostart implementations..."
rg -l "autostart" --type go
# Look for desktop entry related code
echo "Checking for desktop entry related code..."
rg -l "Desktop Entry" --type go
# Check for any other platform-specific start at login implementations
echo "Checking for other start at login implementations..."
ast-grep --pattern 'func $_($_) setStartAtLogin($_) $_'
Length of output: 660
Script:
#!/bin/bash
# Let's examine the content of the related files to understand the implementation details
# Check the content of the experimental plugin implementation
echo "Content of experimental start at login plugin:"
cat v3/plugins/experimental/start_at_login/plugin_linux.go
# Check the desktop entry related code
echo "Content of desktop entry related code:"
cat v3/internal/commands/dot_desktop.go
# Look for any XDG specific implementations
echo "Checking for XDG specific code:"
rg "XDG" --type go -C 5
Length of output: 11550
v3/pkg/application/application_options.go (1)
117-118: Verify platform-specific implementations
Let's ensure the feature is properly implemented across all platforms.
✅ Verification successful
Platform-specific implementations and error handling are properly implemented
The verification shows comprehensive implementation across all major platforms:
- Windows: Uses registry-based implementation with proper error handling
- macOS: Implements bundle-aware login item management
- Linux: Handles autostart through XDG autostart mechanism
Key findings:
- Each platform has proper error handling for executable path resolution
- Platform-specific checks (e.g., bundle ID on macOS) are in place
- The feature is properly integrated into the main application lifecycle
- Graceful fallback when the feature is not supported in current configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify platform-specific implementations and error handling
# Check for platform-specific implementations
echo "Checking platform-specific implementations..."
rg -l "setStartAtLogin|SetStartAtLogin" --type go
# Check for proper error handling
echo -e "\nChecking error handling..."
rg "setStartAtLogin|SetStartAtLogin" -A 5 -B 5 --type go
# Check for usage in main application code
echo -e "\nChecking usage in application code..."
rg "StartAtLogin" -A 3 -B 3 --type go
Length of output: 14705
v3/pkg/application/application_darwin.go (1)
164-176: LGTM: Imports are appropriate and well-organized
The new imports are necessary for implementing the "Start At Login" functionality, providing access to file operations, command execution, and error handling.
v3/pkg/application/application_windows.go (1)
379-407: Add documentation and verify implementation
The implementation follows Windows best practices but would benefit from documentation about the Windows-specific behavior.
- Add documentation comments:
+// setStartAtLogin configures the application to start automatically when the user logs into Windows.
+// It uses the Windows Registry Run key (HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Run)
+// to achieve this functionality.
func (m *windowsApp) setStartAtLogin(enabled bool) error {
+// canStartAtLogin returns true as Windows supports automatic startup through registry.
+// This is part of the cross-platform startup management interface.
func (m *windowsApp) canStartAtLogin() bool {Let's verify the implementation:
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check the implementation thoroughly:
This will help us verify:
- The interface contract and other platform implementations
- Existing documentation patterns
- Test coverage
- Related experimental implementations
Let me try different search patterns to gather the necessary information:
Let me check one more aspect to make a complete assessment:
Based on the gathered information, I can now provide the final response:
Documentation improvements recommended with consistent implementation
The Windows implementation follows platform best practices and aligns with other platform implementations (macOS and Linux also implement setStartAtLogin). Add documentation to maintain consistency:
+// setStartAtLogin configures the application to start automatically when the user logs into Windows.
+// It uses the Windows Registry Run key (HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Run)
+// to achieve this functionality.
func (m *windowsApp) setStartAtLogin(enabled bool) error {
+// canStartAtLogin returns true as Windows supports automatic startup through registry.
+// This is part of the cross-platform startup management interface.
func (m *windowsApp) canStartAtLogin() bool {The implementation is verified to be:
- Consistent with other platform implementations (macOS and Linux)
- Following Windows best practices using registry
- Properly handling error cases
- Aligned with the experimental start-at-login plugin implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the start at login implementation
# Test 1: Check if there are any other implementations of setStartAtLogin
rg -l "setStartAtLogin"
# Test 2: Check if the registry key path is consistently used across the codebase
rg -l "CurrentVersion.Run"
# Test 3: Look for related test files
fd -g "*_test.go" -x grep -l "StartAtLogin"
# Test 4: Check for any existing error handling patterns
rg "failed to" -A 2
Length of output: 38454
Script:
#!/bin/bash
# Check the interface definition and other platform implementations
ast-grep --pattern 'type $_ interface {
$$$
setStartAtLogin($_) $_
$$$
}'
# Check for any existing documentation patterns in the application package
rg "^// \w+ configures" v3/pkg/application/
# Look for any test files related to the Windows implementation
fd -g "*_windows_test.go" v3/pkg/application/
# Check the experimental start_at_login plugin implementation
rg -A 10 "setStartAtLogin" v3/plugins/experimental/start_at_login/
Length of output: 242
Script:
#!/bin/bash
# Check the interface definition
rg -p "type.*interface.*\{" v3/pkg/application/application.go
# Check other platform implementations
rg -p "func.*setStartAtLogin" v3/pkg/application/application_darwin.go
rg -p "func.*setStartAtLogin" v3/pkg/application/application_linux.go
# Check for any existing godoc style comments in the application package
rg "^// \w+App " v3/pkg/application/
# Check the experimental plugin implementation
rg -p "func.*[Ss]tart[Aa]t[Ll]ogin" v3/plugins/experimental/start_at_login/plugin_window.go
Length of output: 571
Script:
#!/bin/bash
# Check the interface definition and implementation patterns
rg -p "type App interface" -A 20 v3/pkg/application/application.go
# Check for existing documentation patterns in other platform implementations
rg "^// set[A-Z]\w+ " v3/pkg/application/
rg "^// can[A-Z]\w+ " v3/pkg/application/
# Check if there are any related tests
rg "Test.*StartAtLogin" v3/pkg/application/
Length of output: 706
mkdocs-website/docs/en/changelog.md (1)
Line range hint 1-20: LGTM! The changelog follows the documented format.
The file properly adheres to the Keep a Changelog format with clear categorization and consistent entry formatting.
🧰 Tools
🪛 LanguageTool
[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...erabilities. --> ## [Unreleased] ### Added - Added Support for StartAtLogin by [ansxuman](...
(ENGLISH_WORD_REPEAT_RULE)
v3/pkg/application/application.go (2)
197-198: LGTM! Well-designed interface methods.
The new interface methods provide a clean separation between capability checking and actual implementation.
627-631: Verify error handling strategy for startup features.
While logging the error is reasonable for a non-critical feature, consider if users should be notified of the failure to set up start-at-login functionality, especially since this was explicitly requested via configuration.
✅ Verification successful
Based on the search results, I can see that the application has a comprehensive error handling system that includes:
- Error dialogs for user-facing errors via
application.ErrorDialog() - Logging mechanisms for system errors via
Logger.Error() - Different error handling strategies based on criticality:
- Fatal errors that terminate the application
- User-facing errors with dialogs
- Background errors that are only logged
For the specific case of SetStartAtLogin(), the current error handling (just logging) appears to be appropriate because:
- It's a non-critical startup feature that shouldn't prevent the application from running
- The error is properly logged for debugging purposes
- Similar non-critical startup features in the codebase follow the same pattern of logging-only error handling
Error handling strategy is appropriate for this startup feature
- The current implementation aligns with the codebase's error handling patterns for non-critical features
- Logging provides necessary debugging information while allowing the application to continue
- More aggressive error handling (like showing error dialogs) would be disproportionate for this startup convenience feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing error notifications or user feedback mechanisms in the codebase
rg -A 2 "Error\(" --type go
Length of output: 99080
| bin, err := os.Executable() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate executable path
The executable path should be validated to ensure it exists and is executable.
bin, err := os.Executable()
if err != nil {
return err
}
+
+ // Validate executable
+ if info, err := os.Stat(bin); err != nil {
+ return fmt.Errorf("invalid executable path: %w", err)
+ } else if info.Mode()&0111 == 0 {
+ return fmt.Errorf("file is not executable: %s", bin)
+ }📝 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.
| bin, err := os.Executable() | |
| if err != nil { | |
| return err | |
| } | |
| bin, err := os.Executable() | |
| if err != nil { | |
| return err | |
| } | |
| // Validate executable | |
| if info, err := os.Stat(bin); err != nil { | |
| return fmt.Errorf("invalid executable path: %w", err) | |
| } else if info.Mode()&0111 == 0 { | |
| return fmt.Errorf("file is not executable: %s", bin) | |
| } |
| func (l *linuxApp) canStartAtLogin() bool { | ||
| homedir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| autostartDir := filepath.Join(homedir, ".config", "autostart") | ||
| if err := os.MkdirAll(autostartDir, 0755); err != nil { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor capability check to be non-mutating
The canStartAtLogin method currently creates directories, which is unexpected behavior for a capability check. This should be split into separate "check" and "ensure" operations.
func (l *linuxApp) canStartAtLogin() bool {
homedir, err := os.UserHomeDir()
if err != nil {
+ l.parent.error("Failed to get user home directory: %v", err)
return false
}
autostartDir := filepath.Join(homedir, ".config", "autostart")
- if err := os.MkdirAll(autostartDir, 0755); err != nil {
+ _, err = os.Stat(autostartDir)
+ if os.IsNotExist(err) {
+ return true
+ } else if err != nil {
+ l.parent.error("Failed to check autostart directory: %v", err)
return false
}
return true
}
+
+// ensureAutostartDir ensures the autostart directory exists
+func (l *linuxApp) ensureAutostartDir() error {
+ homedir, err := os.UserHomeDir()
+ if err != nil {
+ return fmt.Errorf("failed to get user home directory: %w", err)
+ }
+
+ autostartDir := filepath.Join(homedir, ".config", "autostart")
+ return os.MkdirAll(autostartDir, 0755)
+}Then update setStartAtLogin to use this:
func (l *linuxApp) setStartAtLogin(enabled bool) error {
+ if err := l.ensureAutostartDir(); err != nil {
+ return err
+ }Committable suggestion skipped: line range outside the PR's diff.
| func (m *macosApp) setStartAtLogin(enabled bool) error { | ||
| exe, err := os.Executable() | ||
| if err != nil { | ||
| return errors.Wrap(err, "Error running os.Executable:") | ||
| } | ||
|
|
||
| binName := filepath.Base(exe) | ||
| if !strings.HasSuffix(exe, "/Contents/MacOS/"+binName) { | ||
| return fmt.Errorf("app needs to be running as package.app file to start at login") | ||
| } | ||
|
|
||
| appPath := strings.TrimSuffix(exe, "/Contents/MacOS/"+binName) | ||
| var command string | ||
| if enabled { | ||
| command = fmt.Sprintf("tell application \"System Events\" to make login item at end with properties {name: \"%s\",path:\"%s\", hidden:false}", binName, appPath) | ||
| } else { | ||
| command = fmt.Sprintf("tell application \"System Events\" to delete login item \"%s\"", binName) | ||
| } | ||
|
|
||
| cmd := exec.Command("osascript", "-e", command) | ||
| _, err = cmd.CombinedOutput() | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add input validation
While the implementation is generally sound, there are a few areas that could be improved:
- The output from
cmd.CombinedOutput()might contain valuable error information that's currently being discarded - The
binNameshould be validated/escaped to prevent AppleScript injection - Consider checking if the login item already exists before adding it
Here's a suggested improvement:
func (m *macosApp) setStartAtLogin(enabled bool) error {
exe, err := os.Executable()
if err != nil {
return errors.Wrap(err, "Error running os.Executable:")
}
binName := filepath.Base(exe)
+ // Escape special characters in binName to prevent AppleScript injection
+ binName = strings.ReplaceAll(binName, "\"", "\\\"")
+
if !strings.HasSuffix(exe, "/Contents/MacOS/"+binName) {
return fmt.Errorf("app needs to be running as package.app file to start at login")
}
appPath := strings.TrimSuffix(exe, "/Contents/MacOS/"+binName)
var command string
if enabled {
command = fmt.Sprintf("tell application \"System Events\" to make login item at end with properties {name: \"%s\",path:\"%s\", hidden:false}", binName, appPath)
} else {
command = fmt.Sprintf("tell application \"System Events\" to delete login item \"%s\"", binName)
}
cmd := exec.Command("osascript", "-e", command)
- _, err = cmd.CombinedOutput()
- return err
+ output, err := cmd.CombinedOutput()
+ if err != nil {
+ return errors.Wrapf(err, "failed to execute AppleScript command: %s", string(output))
+ }
+ return nil
}📝 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.
| func (m *macosApp) setStartAtLogin(enabled bool) error { | |
| exe, err := os.Executable() | |
| if err != nil { | |
| return errors.Wrap(err, "Error running os.Executable:") | |
| } | |
| binName := filepath.Base(exe) | |
| if !strings.HasSuffix(exe, "/Contents/MacOS/"+binName) { | |
| return fmt.Errorf("app needs to be running as package.app file to start at login") | |
| } | |
| appPath := strings.TrimSuffix(exe, "/Contents/MacOS/"+binName) | |
| var command string | |
| if enabled { | |
| command = fmt.Sprintf("tell application \"System Events\" to make login item at end with properties {name: \"%s\",path:\"%s\", hidden:false}", binName, appPath) | |
| } else { | |
| command = fmt.Sprintf("tell application \"System Events\" to delete login item \"%s\"", binName) | |
| } | |
| cmd := exec.Command("osascript", "-e", command) | |
| _, err = cmd.CombinedOutput() | |
| return err | |
| } | |
| func (m *macosApp) setStartAtLogin(enabled bool) error { | |
| exe, err := os.Executable() | |
| if err != nil { | |
| return errors.Wrap(err, "Error running os.Executable:") | |
| } | |
| binName := filepath.Base(exe) | |
| // Escape special characters in binName to prevent AppleScript injection | |
| binName = strings.ReplaceAll(binName, "\"", "\\\"") | |
| if !strings.HasSuffix(exe, "/Contents/MacOS/"+binName) { | |
| return fmt.Errorf("app needs to be running as package.app file to start at login") | |
| } | |
| appPath := strings.TrimSuffix(exe, "/Contents/MacOS/"+binName) | |
| var command string | |
| if enabled { | |
| command = fmt.Sprintf("tell application \"System Events\" to make login item at end with properties {name: \"%s\",path:\"%s\", hidden:false}", binName, appPath) | |
| } else { | |
| command = fmt.Sprintf("tell application \"System Events\" to delete login item \"%s\"", binName) | |
| } | |
| cmd := exec.Command("osascript", "-e", command) | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return errors.Wrapf(err, "failed to execute AppleScript command: %s", string(output)) | |
| } | |
| return nil | |
| } |
| func (m *macosApp) setStartAtLogin(enabled bool) error { | ||
| exe, err := os.Executable() | ||
| if err != nil { | ||
| return errors.Wrap(err, "Error running os.Executable:") | ||
| } | ||
|
|
||
| binName := filepath.Base(exe) | ||
| if !strings.HasSuffix(exe, "/Contents/MacOS/"+binName) { | ||
| return fmt.Errorf("app needs to be running as package.app file to start at login") | ||
| } | ||
|
|
||
| appPath := strings.TrimSuffix(exe, "/Contents/MacOS/"+binName) | ||
| var command string | ||
| if enabled { | ||
| command = fmt.Sprintf("tell application \"System Events\" to make login item at end with properties {name: \"%s\",path:\"%s\", hidden:false}", binName, appPath) | ||
| } else { | ||
| command = fmt.Sprintf("tell application \"System Events\" to delete login item \"%s\"", binName) | ||
| } | ||
|
|
||
| cmd := exec.Command("osascript", "-e", command) | ||
| _, err = cmd.CombinedOutput() | ||
| return err | ||
| } | ||
|
|
||
| func (m *macosApp) canStartAtLogin() bool { | ||
| bundleID := mac.GetBundleID() | ||
| if bundleID == "" { | ||
| return false | ||
| } | ||
|
|
||
| exe, err := os.Executable() | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| binName := filepath.Base(exe) | ||
| return strings.HasSuffix(exe, "/Contents/MacOS/"+binName) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing required permissions for AppleScript automation
The implementation lacks the required NSAppleEventsUsageDescription permission in the Info.plist file, which is necessary for AppleScript automation on macOS. This may cause permission issues when trying to modify login items.
- Add
NSAppleEventsUsageDescriptionto Info.plist to request System Events automation permission - Add error handling to guide users when permissions are denied
- Consider documenting the required permissions in the README
🔗 Analysis chain
Verify security implications and permissions
The implementation uses AppleScript to modify login items, which requires appropriate permissions. We should verify:
- Required permissions are documented
- Error messages guide users when permissions are missing
- No sensitive information is exposed in logs
Let's check for similar implementations and security considerations:
Let me gather more information about error handling and permission requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar login item implementations and permission handling
rg -g '*.go' -A 5 'System Events.*login item'
# Look for permission-related code
rg -g '*.go' 'NSAppleEventsUsageDescription'
# Check for security-related comments or documentation
rg -g '*.md' -i 'login item|start at login|permission|security'
Length of output: 4515
Script:
#!/bin/bash
# Check Info.plist for permission requirements
fd -e plist -x cat {}
# Look for error handling patterns around osascript
rg -g '*.go' -B 3 -A 3 'osascript.*err'
# Search for documentation about permissions
fd -g 'README*.md' -x cat {}
Length of output: 109485
There was a problem hiding this comment.
@ansxuman this is important, if true...
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@leaanthony Most likely it's not needed. I had tested it on my machine. I'll do one thing to be double sure, I'll build and sign it using GitHub Actions and then will check how it behaves on my machine.
There was a problem hiding this comment.
It may be needed on later versions of macOS. The rabbit may be on to something.....
https://developer.apple.com/documentation/bundleresources/information-property-list/nsappleeventsusagedescription
There was a problem hiding this comment.
@leaanthony I build the package on github action and tested on my machine, it seems to be working without NSAppleEventsUsageDescription. Do you still want me to add it ?
- Macbook Air M1 running on 15.1.1 (24B91)
Relevant Screenshot
There was a problem hiding this comment.
If it works, then that's all good
| registryKey := strings.Split(filepath.Base(exePath), ".")[0] | ||
|
|
||
| key, err := registry.OpenKey(registry.CURRENT_USER, `Software\Microsoft\Windows\CurrentVersion\Run`, registry.ALL_ACCESS) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve registry key name handling and access rights
The current implementation has potential issues with registry key handling:
- Registry key name derivation using
strings.Splitis fragile ALL_ACCESSrights might be more permissive than necessary
Consider these improvements:
-registryKey := strings.Split(filepath.Base(exePath), ".")[0]
+registryKey := strings.TrimSuffix(filepath.Base(exePath), filepath.Ext(exePath))
-key, err := registry.OpenKey(registry.CURRENT_USER, `Software\Microsoft\Windows\CurrentVersion\Run`, registry.ALL_ACCESS)
+key, err := registry.OpenKey(registry.CURRENT_USER, `Software\Microsoft\Windows\CurrentVersion\Run`, registry.SET_VALUE|registry.QUERY_VALUE)📝 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.
| registryKey := strings.Split(filepath.Base(exePath), ".")[0] | |
| key, err := registry.OpenKey(registry.CURRENT_USER, `Software\Microsoft\Windows\CurrentVersion\Run`, registry.ALL_ACCESS) | |
| registryKey := strings.TrimSuffix(filepath.Base(exePath), filepath.Ext(exePath)) | |
| key, err := registry.OpenKey(registry.CURRENT_USER, `Software\Microsoft\Windows\CurrentVersion\Run`, registry.SET_VALUE|registry.QUERY_VALUE) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
v3/pkg/application/application_linux.go (1)
266-315: Add error logging for better debuggingConsider adding error logging to help diagnose issues when the start at login feature fails.
func (l *linuxApp) setStartAtLogin(enabled bool) error { homedir, err := os.UserHomeDir() if err != nil { + l.parent.error("Failed to get user home directory: %v", err) return err } bin, err := os.Executable() if err != nil { + l.parent.error("Failed to get executable path: %v", err) return err }mkdocs-website/docs/en/changelog.md (1)
21-21: Enhance the changelog entry with platform-specific details.While the entry follows the correct format, it would be more helpful for users if it included:
- The supported platforms (Windows, macOS, Linux)
- Any platform-specific considerations or limitations
- A brief description of how to use the feature
Consider expanding the entry like this:
-Added Support for StartAtLogin by [ansxuman](https://github.com/ansxuman) in [#3910](https://github.com/wailsapp/wails/pull/3910) +Added Support for StartAtLogin on Windows, macOS, and Linux platforms. This feature allows applications to configure automatic startup when the user logs in. Implementation details vary by platform: +- Windows: Uses the Run registry key +- macOS: Uses LaunchServices +- Linux: Uses XDG autostart +By [ansxuman](https://github.com/ansxuman) in [#3910](https://github.com/wailsapp/wails/pull/3910)v3/pkg/application/application.go (3)
359-360: Consider a more descriptive field name.The field name
startAtLoginOnRuncould be more descriptive to better convey its purpose. Consider alternatives likeinitialStartAtLoginPreferenceorstartAtLoginEnabled.
648-652: Consider enhancing error handling for startup configuration.While the error is logged, failing to set the start at login preference might be important for the application's startup state. Consider:
- Adding the error to a startup diagnostics collection
- Notifying the user if this preference couldn't be applied
- Providing a way to retry setting this preference
1059-1065: Consider improving the unsupported case handling.The current implementation has a few points that could be enhanced:
- Returning nil when the feature is not supported might mask the actual state from callers
- The warning message could be more informative about why it's not supported
Consider this alternative implementation:
func (a *App) SetStartAtLogin(enabled bool) error { if !a.impl.canStartAtLogin() { - a.Logger.Warn("SetStartAtLogin: Not supported in current configuration") - return nil + return fmt.Errorf("start at login is not supported in the current configuration: %s %s", + runtime.GOOS, runtime.GOARCH) } return a.impl.setStartAtLogin(enabled) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/pkg/application/application.go(5 hunks)v3/pkg/application/application_linux.go(2 hunks)v3/pkg/application/application_options.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/application_options.go
🔇 Additional comments (5)
v3/pkg/application/application_linux.go (3)
272-275: Validate executable path before using it
The executable path should be validated to ensure it exists and is executable.
317-329: Separate capability check from directory creation
The method currently has side effects by creating directories, which is unexpected behavior for a capability check.
266-329: Implementation integrates well with the existing codebase
The platform-specific implementation follows established patterns and properly integrates with the parent application structure.
mkdocs-website/docs/en/changelog.md (1)
Line range hint 1-24: LGTM! The changelog structure is well-maintained.
The changelog follows the Keep a Changelog format correctly, with proper categorization and version sections.
🧰 Tools
🪛 LanguageTool
[duplication] ~20-~20: Possible typo: you repeated a word
Context: ...erabilities. --> ## [Unreleased] ### Added - Added Support for StartAtLogin by [ansxuman](...
(ENGLISH_WORD_REPEAT_RULE)
v3/pkg/application/application.go (1)
207-208: LGTM: Well-designed interface methods for platform-specific start at login functionality.
The new interface methods follow Go naming conventions and have appropriate signatures for their purposes:
setStartAtLogin(enabled bool) error: Handles the platform-specific implementationcanStartAtLogin() bool: Provides capability checking
| func (l *linuxApp) setStartAtLogin(enabled bool) error { | ||
| homedir, err := os.UserHomeDir() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| bin, err := os.Executable() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| name := filepath.Base(bin) | ||
| autostartFile := fmt.Sprintf("%s-autostart.desktop", name) | ||
| autostartPath := filepath.Join(homedir, ".config", "autostart", autostartFile) | ||
|
|
||
| if !enabled { | ||
| err := os.Remove(autostartPath) | ||
| if os.IsNotExist(err) { | ||
| return nil | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| const tpl = `[Desktop Entry] | ||
| Name={{.Name}} | ||
| Comment=Autostart service for {{.Name}} | ||
| Type=Application | ||
| Exec={{.Cmd}} | ||
| Hidden=true | ||
| X-GNOME-Autostart-enabled=true | ||
| ` | ||
| if err := os.MkdirAll(filepath.Dir(autostartPath), 0755); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| file, err := os.OpenFile(autostartPath, os.O_RDWR|os.O_CREATE, 0600) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer file.Close() | ||
|
|
||
| t := template.Must(template.New("autostart").Parse(tpl)) | ||
| return t.Execute(file, struct { | ||
| Name string | ||
| Cmd string | ||
| }{ | ||
| Name: name, | ||
| Cmd: bin, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add security measures against path manipulation
The implementation should include additional security measures:
- Resolve any symlinks in the executable path
- Validate the final path is within expected boundaries
- Sanitize the application name used in the file path
bin, err := os.Executable()
if err != nil {
return err
}
+
+ // Resolve symlinks and validate path
+ bin, err = filepath.EvalSymlinks(bin)
+ if err != nil {
+ return fmt.Errorf("failed to resolve executable path: %w", err)
+ }
+
+ // Sanitize name
+ name := filepath.Base(bin)
+ name = strings.Map(func(r rune) rune {
+ if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' {
+ return r
+ }
+ return '_'
+ }, name)📝 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.
| func (l *linuxApp) setStartAtLogin(enabled bool) error { | |
| homedir, err := os.UserHomeDir() | |
| if err != nil { | |
| return err | |
| } | |
| bin, err := os.Executable() | |
| if err != nil { | |
| return err | |
| } | |
| name := filepath.Base(bin) | |
| autostartFile := fmt.Sprintf("%s-autostart.desktop", name) | |
| autostartPath := filepath.Join(homedir, ".config", "autostart", autostartFile) | |
| if !enabled { | |
| err := os.Remove(autostartPath) | |
| if os.IsNotExist(err) { | |
| return nil | |
| } | |
| return err | |
| } | |
| const tpl = `[Desktop Entry] | |
| Name={{.Name}} | |
| Comment=Autostart service for {{.Name}} | |
| Type=Application | |
| Exec={{.Cmd}} | |
| Hidden=true | |
| X-GNOME-Autostart-enabled=true | |
| ` | |
| if err := os.MkdirAll(filepath.Dir(autostartPath), 0755); err != nil { | |
| return err | |
| } | |
| file, err := os.OpenFile(autostartPath, os.O_RDWR|os.O_CREATE, 0600) | |
| if err != nil { | |
| return err | |
| } | |
| defer file.Close() | |
| t := template.Must(template.New("autostart").Parse(tpl)) | |
| return t.Execute(file, struct { | |
| Name string | |
| Cmd string | |
| }{ | |
| Name: name, | |
| Cmd: bin, | |
| }) | |
| } | |
| func (l *linuxApp) setStartAtLogin(enabled bool) error { | |
| homedir, err := os.UserHomeDir() | |
| if err != nil { | |
| return err | |
| } | |
| bin, err := os.Executable() | |
| if err != nil { | |
| return err | |
| } | |
| // Resolve symlinks and validate path | |
| bin, err = filepath.EvalSymlinks(bin) | |
| if err != nil { | |
| return fmt.Errorf("failed to resolve executable path: %w", err) | |
| } | |
| // Sanitize name | |
| name := filepath.Base(bin) | |
| name = strings.Map(func(r rune) rune { | |
| if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' { | |
| return r | |
| } | |
| return '_' | |
| }, name) | |
| autostartFile := fmt.Sprintf("%s-autostart.desktop", name) | |
| autostartPath := filepath.Join(homedir, ".config", "autostart", autostartFile) | |
| if !enabled { | |
| err := os.Remove(autostartPath) | |
| if os.IsNotExist(err) { | |
| return nil | |
| } | |
| return err | |
| } | |
| const tpl = `[Desktop Entry] | |
| Name={{.Name}} | |
| Comment=Autostart service for {{.Name}} | |
| Type=Application | |
| Exec={{.Cmd}} | |
| Hidden=true | |
| X-GNOME-Autostart-enabled=true | |
| ` | |
| if err := os.MkdirAll(filepath.Dir(autostartPath), 0755); err != nil { | |
| return err | |
| } | |
| file, err := os.OpenFile(autostartPath, os.O_RDWR|os.O_CREATE, 0600) | |
| if err != nil { | |
| return err | |
| } | |
| defer file.Close() | |
| t := template.Must(template.New("autostart").Parse(tpl)) | |
| return t.Execute(file, struct { | |
| Name string | |
| Cmd string | |
| }{ | |
| Name: name, | |
| Cmd: bin, | |
| }) | |
| } |
Description
Added
Support for Start At Login #1789
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails3 doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails3 doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements