fix(desktop): skip the PowerShell profile probe when node is already resolvable - #5771
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
ApprovabilityVerdict: Approved ccb618c Performance optimization that skips expensive PowerShell profile probe when node is already available. Self-contained change with good test coverage covering both code paths. You can customize Macroscope's approvability policy. Learn more. |
|
Closing this one. #5878 landed the concurrency fix on the same function and removes most of what this PR was for: with both probes running under What remains is one fewer PowerShell spawn. That is real but small, and I cannot put a measurement behind it, so it is not worth carrying a conflicting branch through a rewritten file. If profile load does turn out to dominate on machines with heavy profiles, that is a separate PR with numbers attached. |
Problem
On Windows,
installWindowsEnvironmentruns two PowerShell probes on every desktop start: one with-NoProfile, then one that loads the user profile. Each probe walkspwsh.exeand thenpowershell.exe, and every candidate gets its own 5 second timeout, so a slow or failing profile can burn up to four timeout budgets before the window appears.The profile probe was never meant to be unconditional. #1729 added it as a fallback for when node is missing, and
resolveWindowsEnvironmentinpackages/shared/src/shell.tsstill works that way:The desktop copy lost that gate in #2546 when the app was ported to Effect, so both probes now run every time.
Fix
FNM_DIRandFNM_MULTISHELL_PATHCommandAvailabilityinmakealongside the other services, so the check is injectable in testsPATH precedence is unchanged: profile, then known CLI directories, then the no-profile probe, then the inherited environment.
Validation
vp test run src/shell/DesktopShellEnvironment.test.tsinapps/desktop(10 passed)vp run --filter @t3tools/desktop typecheckvp lint --report-unused-disable-directives(clean; the one warning it reports is pre-existing inapps/web)The new test fails with
expected 2 to equal 1when the gate is removed, so it covers the regression rather than just the current behavior.I did not measure packaged-app startup traces, so the probe count is covered by the test rather than by a trace comparison.
Closes #4403
Note
Low Risk
Localized Windows shell hydration logic with shared parity to
resolveWindowsEnvironment; profile fallback remains when node is missing.Overview
Windows desktop startup no longer always runs the slow PowerShell profile probe. After the existing no-profile PATH probe, known CLI directories, and inherited
PATHare merged into a baseline, the app uses sharedCommandAvailabilityto check whethernoderesolves; if it does, it returns early and skips the profile probe (and fnm env vars unless needed).When
nodeis still missing, behavior matches before: profile probe runs to pick up profilePATH,FNM_DIR, andFNM_MULTISHELL_PATH, with the same PATH merge order (profile ahead of baseline).CommandAvailabilityis resolved inmakeand injected intoinstallIntoProcessso tests can stub it.Tests gain a
nodeAvailablehook and a case that asserts only one PowerShell invocation (-NoProfile) when node is already available.Reviewed by Cursor Bugbot for commit ccb618c. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Skip PowerShell profile probe on Windows when
nodeis already resolvable-NoProfilePATH probe, and the existing process PATH before any profile probing occurs.nodeis resolvable in the baseline environment, the full PowerShell profile probe is skipped entirely, avoiding slow or side-effectful profile execution.nodeis not resolvable, the existing profile-loading behavior is preserved and FNM-related variables (FNM_DIR,FNM_MULTISHELL_PATH) are still populated.DesktopShellEnvironment.makenow explicitly depends onPath.PathandCommandAvailabilityservices to support the new resolution check.Macroscope summarized ccb618c.