Refactor shell plugin, add tests & fix bugs#4554
Open
DavidGBrett wants to merge 9 commits into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
📝 WalkthroughWalkthrough
ChangesShell Plugin Refactor and Tests
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Flow.Launcher.Test/Plugins/ShellPluginTest.cs (1)
133-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the PowerShell branch assertions.
These tests only verify marker tokens (
wt.exe,"powershell",-NoExit, missing-Command). A regression that drops the command payload or the WT-specific escaped\;terminator inMain.ConfigurePowershellProcessStartInfo(...)would still pass. Please assert the fullArgumentListfor the WT and leave-open paths.Example assertions
[Test] public void Powershell_WT_UsesWindowsTerminal() { var info = Create( shell: Shell.Powershell, useWindowsTerminal: true); Assert.That(info.FileName, Is.EqualTo("wt.exe")); - Assert.That(info.ArgumentList, Does.Contain("powershell")); + Assert.That(info.ArgumentList, Is.EqualTo(["powershell", "-Command", "test\\;"])); } ... [Test] public void Powershell_LeaveShellOpen_OmitsCommandSwitch() { var info = Create( shell: Shell.Powershell, leaveShellOpen: true); - Assert.That(info.ArgumentList, Does.Not.Contain("-Command")); + Assert.That(info.ArgumentList, Is.EqualTo(["-NoExit", "test"])); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher.Test/Plugins/ShellPluginTest.cs` around lines 133 - 165, The PowerShell tests in ShellPluginTest are too loose and can miss regressions in Main.ConfigurePowershellProcessStartInfo(...). Tighten the assertions in Powershell_WT_UsesWindowsTerminal and Powershell_LeaveShellOpen_OmitsCommandSwitch to verify the full ArgumentList, not just marker tokens, including the command payload and the WT-specific escaped terminator sequence. Use the existing Create helper and Shell.Powershell path to assert the exact argument shape for both the Windows Terminal and leave-shell-open cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher.Test/Plugins/ShellPluginTest.cs`:
- Around line 69-77: The Cmd_CloseShellAfterPress_AppendsPause test only checks
a substring of the synthesized Arguments, so malformed trailing tokens could
still slip through. Update this assertion in
Cmd_CloseShellAfterPress_AppendsPause to verify the full command string produced
by Create/ConfigureCmdProcessStartInfo for Shell.Cmd when closeShellAfterPress
is true, including the exact pause sequence and any surrounding tokens.
In `@Plugins/Flow.Launcher.Plugin.Shell/Main.cs`:
- Around line 382-390: The Shell command parsing in Main should preserve the raw
tail of the command instead of adding it as a single argv item. Update the logic
in the command-splitting block around ExistInPath so the extracted arguments are
assigned through info.Arguments rather than info.ArgumentList.Add(arguments),
and keep the executable name in info.FileName. Add a test covering a known
executable invoked with multiple arguments to verify the tail is passed through
unchanged.
---
Nitpick comments:
In `@Flow.Launcher.Test/Plugins/ShellPluginTest.cs`:
- Around line 133-165: The PowerShell tests in ShellPluginTest are too loose and
can miss regressions in Main.ConfigurePowershellProcessStartInfo(...). Tighten
the assertions in Powershell_WT_UsesWindowsTerminal and
Powershell_LeaveShellOpen_OmitsCommandSwitch to verify the full ArgumentList,
not just marker tokens, including the command payload and the WT-specific
escaped terminator sequence. Use the existing Create helper and Shell.Powershell
path to assert the exact argument shape for both the Windows Terminal and
leave-shell-open cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24730a6a-8fc7-4f77-ad81-2b86239f72b4
📒 Files selected for processing (2)
Flow.Launcher.Test/Plugins/ShellPluginTest.csPlugins/Flow.Launcher.Plugin.Shell/Main.cs
DavidGBrett
force-pushed
the
clean-up-shell-plugin
branch
from
f1a863a to
3271072
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
DavidGBrett
changed the title
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
DavidGBrett
force-pushed
the
clean-up-shell-plugin
branch
from
e8c679d to
4344cdc
Compare
…ach shell type in shell plugin
This is a clearer name which is more important now that its also a param
Logically equivalent as per De Morgan's Laws This also makes the intent clearer - we use "runas" if either runAsAdministrator or _settings.RunAsAdministrator is true
…artInfo in shell plugin
…pecific methods so full pipeline is tested The shell specific methods such as ConfigureCmdProcessStartInfo are now private to prevent them from being tested instead of CreateProcessStartInfo
Existing two tests are preserved but renamed and rewritten slightly
…plugin The trailing /c after pause > nul had no purpose as pause ignores arguments. Adjusted the test to match.
…path handling - Uses Arguments instead of ArgumentList so that OS parser can correctly split the args by spaces. - Quoted executable paths with spaces are now parsed correctly. - Arguments are now always trimmed to catch extra whitespace
DavidGBrett
force-pushed
the
clean-up-shell-plugin
branch
from
4344cdc to
049ffe5
Compare
DavidGBrett
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up PR from #4545
Finish the job of refactoring the shell plugin and in doing that make it testable and then add tests for it.
Also did some bug fixes in the process.
Bug fixes
Argumentsinstead ofArgumentListso OS correctly splits args by spaces, add support for quoted executablepaths with spaces, and improve whitespace handling in args.
echo %USERPROFILE%) instead of expanded environment variables, matching what the user actually typed./cafterpause > nulin the pause command.pauseignores arguments, so it had no effect, but cleaning it up prevents confusion.Summary by cubic
Refactored the shell plugin to centralize process creation and add per‑shell helpers, fixing argument parsing, elevation, and Windows Terminal quirks. Improves reliability across CMD, PowerShell, pwsh, Windows Terminal, and RunCommand; history now records exactly what you typed.
Summary of changes
CreateProcessStartInfo(trim, env var expansion, working dir,UseShellExecute = true). Simplified admin elevation (Verb = "runas"if either flag is true). Windows Terminal escaping and argument handling are more robust. PowerShell now adds-NoExitwhen leaving open and omits-Commandin that case;pwshalways uses-Commandand adds-NoExitwhen leaving open. CMD close-after-press usespause > nul(no trailing/c). RunCommand now usesArgumentsfor the tail, handles quoted executable paths, trims leading whitespace, and falls back to the whole command when the executable isn’t found.ConfigureCmdProcessStartInfo,ConfigurePowershellProcessStartInfo,ConfigurePwshProcessStartInfo,ConfigureRunCommandStartInfo). Unified close prompt parameter (closePrompt).UseShellExecuteassignment in RunCommand. Public visibility of shell-specific config methods (now private).CreateProcessStartInfoto validate the full pipeline across shells and Windows Terminal. Added coverage for leave-open/close,-NoExit/-Commandbehavior, close prompt, quoting and quoted-paths, env var expansion, working dir, PATH lookup, admin verb, and multi-word arguments in RunCommand; updated the CMD pause assertion.Release Note
Commands now launch more reliably across shells; multi-word arguments and quoted paths work in RunCommand, and your history shows exactly what you typed.
Written for commit 049ffe5. Summary will update on new commits.