Support Logon Task for Faster Startup Experience #3218
Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe changes introduce a new boolean property, Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
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: 1
🧹 Nitpick comments (2)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (1)
64-85: Switching startup method is done effectively.
TogglingUseLogonTaskForStartuptriggers a quick disable-enable routine to transition smoothly between registry and task-based startup. As a good-to-have, consider refactoring into a single switch function if multiple toggles become too frequent.Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml (1)
39-44: Consider adding an icon for consistency.The new card for logon task setting is missing an icon attribute, while other cards in this file use icons. Consider adding an icon for visual consistency.
Apply this diff to add an icon:
- <cc:Card Title="{DynamicResource useLogonTaskForStartup}"> + <cc:Card Title="{DynamicResource useLogonTaskForStartup}" Icon="">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Flow.Launcher.Infrastructure/UserSettings/Settings.cs(1 hunks)Flow.Launcher/App.xaml.cs(1 hunks)Flow.Launcher/Flow.Launcher.csproj(1 hunks)Flow.Launcher/Helper/AutoStartup.cs(3 hunks)Flow.Launcher/Languages/en.xaml(1 hunks)Flow.Launcher/Languages/zh-cn.xaml(2 hunks)Flow.Launcher/Languages/zh-tw.xaml(2 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs(2 hunks)Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (16)
Flow.Launcher/Helper/AutoStartup.cs (8)
2-4: Imports look correct for newly introduced functionalities.
No concerns found with these added using statements.Also applies to: 8-8
15-16: Consider clarifying task naming.
The constants for task name and description look fine. However, ensure that the naming remains consistent across logs and user-facing messages so users can identify the task more easily.
22-27: Centralized check is valid but consider text clarifications.
Comment indicates checking the logon task first, which aligns with the new logic. No immediate issues are found here.
70-85: Disable logic is straightforward and consistent.
The updated signature adds flexibility for disabling either the registry entry or the scheduled task. Looks good.
91-103: Enable logic is well structured.
Similar to the disable method, enabling either logon task or registry-based startup is properly separated by parameter.
111-138: Check administrative privileges for correct run level execution.
The scheduling logic is robust. The conditional for settingRunLevel.Highestis appropriate.
140-153: Unscheduling approach is appropriate.
This routine correctly removes existing tasks by name and handles exceptions gracefully.
155-160: Role check for administrative context is correct.
Implementation usingWindowsPrincipalis standard practice. No changes necessary.Flow.Launcher/App.xaml.cs (1)
122-122: Method call respects user preference for logon task startup.
Passing_settings.UseLogonTaskForStartupintoEnable()aligns with the new approach. The existing exception handling ensures graceful fallback if enabling fails.Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs (2)
45-48: Selective enable logic is correct.
The property setter checks if startup is toggled on and correctly callsAutoStartup.Enable(UseLogonTaskForStartup).
50-54: Disabling multiple startup methods ensures consistency.
CallingDisable(true)followed byDisable(false)is thorough, preventing leftover startup entries.Flow.Launcher.Infrastructure/UserSettings/Settings.cs (1)
241-241: LGTM!The new property is well-placed near other startup-related settings and follows the existing code style.
Flow.Launcher/Languages/zh-cn.xaml (1)
50-50: LGTM!The translation is accurate and appropriately placed in the startup settings section.
Flow.Launcher/Languages/zh-tw.xaml (1)
50-50: LGTM!The translation is accurate and appropriately placed in the startup settings section.
Flow.Launcher/Languages/en.xaml (1)
49-49: LGTM!The string resource is well-written, follows naming conventions, and clearly explains the benefit of using a logon task for startup.
Flow.Launcher/Flow.Launcher.csproj (1)
103-103: Verify the TaskScheduler package version.The package version is fixed at 2.11.0. Let's verify if this is the latest stable version and check for any security advisories.
✅ Verification successful
TaskScheduler package version 2.11.0 is current and secure
The project is using the latest stable version available on NuGet, and no security vulnerabilities were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for latest version and security advisories of the TaskScheduler package. # Check NuGet for latest versions curl -s https://api.nuget.org/v3/registration5-gz-semver2/taskscheduler/index.json | jq -r '.items[0].upper' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NUGET, package: "TaskScheduler") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 517
Script:
#!/bin/bash # Description: Check latest version of TaskScheduler package using alternative NuGet API endpoint curl -s "https://api.nuget.org/v3-flatcontainer/taskscheduler/index.json" | jq -r '.versions[]' | sort -V | tail -n 1Length of output: 124
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Flow.Launcher/Helper/AutoStartup.cs (1)
44-69:⚠️ Potential issueLogic error: returns true even when the task doesn't exist.
If
taskis null, the method proceeds to returntrue, which incorrectly signals that the logon task is enabled.Apply a fix to return
falsewhen the task is not found:private static bool CheckLogonTask() { using var taskService = new TaskService(); var task = taskService.RootFolder.AllTasks.FirstOrDefault(t => t.Name == LogonTaskName); - if (task != null) + if (task == null) + { + return false; + } { try {
🧹 Nitpick comments (3)
Flow.Launcher/Helper/AutoStartup.cs (3)
22-27: Add error handling for logon task check.The logon task check should be wrapped in a try-catch block similar to the registry check to ensure robustness.
// Check if logon task is enabled -if (CheckLogonTask()) +try { - return true; + if (CheckLogonTask()) + { + return true; + } +} +catch (Exception e) +{ + Log.Error("AutoStartup", $"Ignoring non-critical logon task error (querying if enabled): {e}"); }
Line range hint
71-111: Add parameter validation.The methods should validate the state before making changes to avoid potential race conditions.
public static void Enable(bool logonTask) { + // Avoid enabling if already enabled with the same method + if (IsEnabled && CheckLogonTask() == logonTask) + { + return; + } + try {
113-161: Enhance security settings for the scheduled task.Consider adding these security-related improvements:
- Set
AllowHardTerminateto false to prevent force-stopping- Enable
RunOnlyIfNetworkAvailableif network connectivity is required- Add
SecurityDescriptorto restrict task accesstd.Settings.StopIfGoingOnBatteries = false; td.Settings.DisallowStartIfOnBatteries = false; td.Settings.ExecutionTimeLimit = TimeSpan.Zero; +td.Settings.AllowHardTerminate = false; +td.Settings.RunOnlyIfNetworkAvailable = true; + +// Restrict access to SYSTEM and Administrators +td.SecurityDescriptor = "D:(A;;FRFX;;;SY)(A;;FRFX;;;BA)";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/Helper/AutoStartup.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Flow.Launcher/Helper/AutoStartup.cs (1)
8-8: LGTM!The new imports and constants are well-defined and appropriate for the task scheduler functionality.
Also applies to: 15-16
This comment has been minimized.
This comment has been minimized.
|
@Jack251970 I noticed PT had issues removing schedule logon task when uninstalling. Could you test this part please- how does the schedule task gets handled by installed flow version? If it is a manual process to remove the task when uninstalling flow, especially for portable version, I think we need to have this explained under the option so user is aware that manual removal is needed. |
|
@jjw24 Yeah, when I uninstall the FL, I find the logon task will not be deleted. I need to add tooltip for this. |
This comment has been minimized.
This comment has been minimized.
@jjw24 Please check. |
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: Yusyuriv, onesounds Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Creating a string key on HKLM\...\Run to launch FL is a tiny tiny bit faster than task scheduler, and I think it's easier to delete it. |
|
@rottakore Thanks for your idea! I have decided to use this way. |
|
@jjw24 I attempted to use the HKLM...\Run registry key, but it requires significant adjustments since FL is not designed to run with administrator privileges. For now, I’ll continue using the logon task to avoid complications. |
| AutoStartup.Disable(); | ||
| { | ||
| // Disable both registry and task scheduler | ||
| AutoStartup.Disable(true); |
There was a problem hiding this comment.
One nitpick, could we change this to DisableViaLogonTask & EnableViaLogonTask and same type of naming for registry.
There was a problem hiding this comment.
Keep these original methods as private and just add the additional wrapper functions for easier understanding.
There was a problem hiding this comment.
Well, that is fine. Please check the latest commit.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Flow.Launcher/Helper/AutoStartup.cs (3)
99-139: Consider more specific error handling.The catch blocks could be more specific about the types of exceptions they handle to provide better error messages and recovery options.
Consider this pattern:
- catch (Exception e) + catch (UnauthorizedAccessException e) + { + Log.Error("AutoStartup", $"Access denied while managing auto-startup: {e}"); + throw; + } + catch (Exception e)
141-167: Document task configuration choices.The task configuration includes specific choices (2-second delay, battery settings disabled) that should be documented to explain the rationale.
Add comments explaining these choices:
+ // Add a small delay to ensure system services are ready td.Triggers.Add(new LogonTrigger { UserId = WindowsIdentity.GetCurrent().Name, Delay = TimeSpan.FromSeconds(2) }); td.Actions.Add(Constant.ExecutablePath); + // Ensure the application runs regardless of power source td.Settings.StopIfGoingOnBatteries = false; td.Settings.DisallowStartIfOnBatteries = false;
184-189: Consider caching admin status check.The IsCurrentUserIsAdmin check might be called frequently. Consider caching the result since admin status rarely changes during runtime.
+ private static bool? _isAdmin; private static bool IsCurrentUserIsAdmin() { + if (_isAdmin.HasValue) + return _isAdmin.Value; var identity = WindowsIdentity.GetCurrent(); var principal = new WindowsPrincipal(identity); - return principal.IsInRole(WindowsBuiltInRole.Administrator); + return _isAdmin = principal.IsInRole(WindowsBuiltInRole.Administrator); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher/App.xaml.cs(1 hunks)Flow.Launcher/Helper/AutoStartup.cs(3 hunks)Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Flow.Launcher/App.xaml.cs
- Flow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Flow.Launcher/Helper/AutoStartup.cs (2)
14-42: LGTM! Well-structured property implementation.The IsEnabled property cleanly handles both logon task and registry checks with proper error handling and logging.
71-97: LGTM! Well-designed public API.The public methods are well-named and follow the Single Responsibility Principle, making the API intuitive to use.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
Please check |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Flow.Launcher/Helper/AutoStartup.cs (1)
53-53:⚠️ Potential issueAdd null check for task action.
The null-forgiving operator (
!) on line 53 could cause aNullReferenceExceptionif the task has no actions defined. This was identified in a previous review but has not been addressed.Apply this fix to properly handle potential null references:
- var action = task.Definition.Actions.FirstOrDefault()!.ToString().Trim(); + var action = task.Definition.Actions.FirstOrDefault()?.ToString()?.Trim(); + if (action == null) + { + UnscheduleLogonTask(); + ScheduleLogonTask(); + return true; + }
🧹 Nitpick comments (5)
Flow.Launcher/Helper/AutoStartup.cs (5)
184-189: Fix method name with redundant "is".The method name
IsCurrentUserIsAdmin()contains a redundant "is" which affects readability.- private static bool IsCurrentUserIsAdmin() + private static bool IsCurrentUserAdmin()Update all references to this method accordingly.
145-145: Consider making the delay configurable.The current implementation uses a fixed 2-second delay after logon before starting the application. Consider making this delay configurable through the settings to accommodate different system performance characteristics.
- td.Triggers.Add(new LogonTrigger { UserId = WindowsIdentity.GetCurrent().Name, Delay = TimeSpan.FromSeconds(2) }); + td.Triggers.Add(new LogonTrigger { UserId = WindowsIdentity.GetCurrent().Name, Delay = TimeSpan.FromSeconds(Settings.StartupDelay) });This would require adding a new
StartupDelaysetting with a reasonable default of 2 seconds.
159-166: Enhance error handling and logging.The current error handling captures and logs exceptions but doesn't provide specific guidance for users or administrators when task registration fails.
Consider enhancing the error message to include more specific information about the failure reason and potential remediation steps:
catch (Exception e) { - Log.Error("AutoStartup", $"Failed to schedule logon task: {e}"); + string errorDetails = e is UnauthorizedAccessException + ? "Access denied. Try running Flow Launcher as administrator." + : e.Message; + Log.Error("AutoStartup", $"Failed to schedule logon task: {errorDetails}", e); return false; }This would provide more actionable information to users experiencing issues.
153-156: Consider adding additional task settings for power management.The current implementation disables stopping if going on batteries and starting if on batteries, but there are other power-related settings that might be worth configuring.
Consider adding these additional settings to ensure the task behaves correctly under all power conditions:
td.Settings.StopIfGoingOnBatteries = false; td.Settings.DisallowStartIfOnBatteries = false; + td.Settings.AllowHardTerminate = true; + td.Settings.StartWhenAvailable = true; + td.Settings.RestartOnFailure = new RestartSettings(new TimeSpan(0, 1, 0), 3); td.Settings.ExecutionTimeLimit = TimeSpan.Zero;These settings would ensure the task can start when the system becomes available, can be forcefully terminated if needed, and can restart on failure.
22-28: Improve comment clarity for better code readability.The current comments are functional but could be more descriptive to better explain the priority order of startup methods.
- // Check if logon task is enabled + // First check if logon task is enabled (preferred method for faster startup) if (CheckLogonTask()) { return true; } - // Check if registry is enabled + // Fall back to registry method if logon task is not enabled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/Helper/AutoStartup.cs(3 hunks)
🔇 Additional comments (1)
Flow.Launcher/Helper/AutoStartup.cs (1)
171-182: Verify task cleanup during uninstallation.Per PR comments, the logon task is not automatically removed during uninstallation. Consider integrating the
UnscheduleLogonTask()method with the application's uninstallation process.This would prevent orphaned tasks from remaining on the system after the application is uninstalled, improving user experience.
Add support for FL to use logon task instead of startup entry for faster startup experience.
As described.
Related issue: #3212.
Test
Latest screenshot:
After using logon task for startup, FL launches as fast as PowerToys (the center app in the tray) which uses logon task for startup as well.
Also, it launchers faster than other apps using startup entry, like Microsoft Security Center (You cannot see it here because it has not launched yet.)
As for precise performance: This PR improves startup time by about 80%. (from chrisl8 in #3212)