[FEATURE] Add Extra Args option to browser settings#4402
Open
DavidGBrett wants to merge 2 commits intoFlow-Launcher:devfrom
Open
[FEATURE] Add Extra Args option to browser settings#4402DavidGBrett wants to merge 2 commits intoFlow-Launcher:devfrom
DavidGBrett wants to merge 2 commits intoFlow-Launcher:devfrom
Conversation
- added new prop ExtraArgs to CustomBrowserViewModel - add new row in SelectBrowserWindow to edit this - Update OpenInBrowserWindow and OpenInBrowserTab to pass along these extra arguments
DavidGBrett
changed the title
|
🥷 Code experts: Jack251970 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: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
![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 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher/PublicAPIInstance.cs">
<violation number="1" location="Flow.Launcher/PublicAPIInstance.cs:467">
P2: Raw browser `ExtraArgs` are logged in exception messages, potentially exposing sensitive command-line data in persistent logs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
📝 WalkthroughWalkthroughAdds an ExtraArgs string to custom browser profiles, surfaces it in the UI/localization, ensures it copies with the profile, and forwards the value when launching browsers via OpenUri/OpenInBrowserTab/OpenInBrowserWindow. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as SelectBrowserWindow
participant VM as CustomBrowserViewModel
participant API as PublicAPIInstance
participant SW as SearchWeb
User->>UI: Enter ExtraArgs
UI->>VM: Save ExtraArgs
VM->>VM: Copy() includes ExtraArgs
User->>API: Request OpenUri(url)
API->>SW: OpenInBrowserTab/Window(url, browserPath, inPrivate, privateArg, extraArgs)
SW->>SW: Build command: browserPath [privateArg] [extraArgs] url
SW-->>API: Launch process
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (2)
Flow.Launcher.Plugin/SharedCommands/SearchWeb.cs (2)
55-55: RenamebrowserArguementstobrowserArguments.Minor naming typo; worth cleaning up to satisfy lint/spell checks and improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Plugin/SharedCommands/SearchWeb.cs` at line 55, The local variable browserArguements has a spelling typo; rename it to browserArguments everywhere it's declared and referenced (e.g., the declaration in SearchWeb.cs and any usages in methods that build the browser command string) to fix lint/spelling checks and improve readability, ensuring you update all occurrences and preserve the existing logic (including the iexplore.exe conditional).
55-58: Consider centralizing browser-argument composition.The same
privateArg + extraArgs + urlconcatenation now exists in two methods. A small shared helper would keep ordering/spacing behavior consistent over time.Also applies to: 105-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Plugin/SharedCommands/SearchWeb.cs` around lines 55 - 58, The browser argument string construction for the SearchWeb class is duplicated (see the browserArguements concatenation that uses browserExecutableName, inPrivate/privateArg, extraArgs and url); extract this logic into a single helper method (e.g., BuildBrowserArguments or ComposeBrowserArguments) that accepts browserExecutableName, inPrivate, privateArg, extraArgs and url and returns the correctly ordered/spaced argument string, then replace both duplicate concatenations (lines around browserArguements and the similar block at 105-107) with calls to that helper to ensure consistent ordering and spacing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Line 467: The LogException call is logging raw browserInfo.PrivateArg and
browserInfo.ExtraArgs which can leak sensitive user-provided arguments; update
the LogException invocation in PublicAPIInstance.cs (the line that calls
LogException(ClassName, $"Failed to open URL in browser {tabOrWindow}: {path},
{inPrivate ?? browserInfo.EnablePrivate}, {browserInfo.PrivateArg},
{browserInfo.ExtraArgs}", e)) to redact those values—replace them with either
booleans indicating presence (e.g., PrivateArg != null, ExtraArgs != null) or a
fixed redacted string like "[REDACTED]" while keeping tabOrWindow, path,
inPrivate (or browserInfo.EnablePrivate) and the exception variable e unchanged.
---
Nitpick comments:
In `@Flow.Launcher.Plugin/SharedCommands/SearchWeb.cs`:
- Line 55: The local variable browserArguements has a spelling typo; rename it
to browserArguments everywhere it's declared and referenced (e.g., the
declaration in SearchWeb.cs and any usages in methods that build the browser
command string) to fix lint/spelling checks and improve readability, ensuring
you update all occurrences and preserve the existing logic (including the
iexplore.exe conditional).
- Around line 55-58: The browser argument string construction for the SearchWeb
class is duplicated (see the browserArguements concatenation that uses
browserExecutableName, inPrivate/privateArg, extraArgs and url); extract this
logic into a single helper method (e.g., BuildBrowserArguments or
ComposeBrowserArguments) that accepts browserExecutableName, inPrivate,
privateArg, extraArgs and url and returns the correctly ordered/spaced argument
string, then replace both duplicate concatenations (lines around
browserArguements and the similar block at 105-107) with calls to that helper to
ensure consistent ordering and spacing.
🪄 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: 5b126954-3b24-495f-9420-a5d7d5794693
📒 Files selected for processing (5)
Flow.Launcher.Infrastructure/UserSettings/CustomBrowserViewModel.csFlow.Launcher.Plugin/SharedCommands/SearchWeb.csFlow.Launcher/Languages/en.xamlFlow.Launcher/PublicAPIInstance.csFlow.Launcher/SelectBrowserWindow.xaml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Flow.Launcher/PublicAPIInstance.cs (1)
467-471:⚠️ Potential issue | 🟠 MajorRedact
PrivateArgin logs too (raw arg still exposed).Line 471 still logs
browserInfo.PrivateArgverbatim. That can leak sensitive user-provided flags/paths in logs.🔒 Suggested fix
- string includesExtraArgs = string.IsNullOrWhiteSpace(browserInfo.ExtraArgs) - ? "" - : ", [including omitted Extra Args]"; - - LogException(ClassName, $"Failed to open URL in browser {tabOrWindow}: {path}, {inPrivate ?? browserInfo.EnablePrivate}, {browserInfo.PrivateArg}{includesExtraArgs}", e); + var hasPrivateArg = !string.IsNullOrWhiteSpace(browserInfo.PrivateArg); + var hasExtraArgs = !string.IsNullOrWhiteSpace(browserInfo.ExtraArgs); + LogException( + ClassName, + $"Failed to open URL in browser {tabOrWindow}: path={path}, inPrivate={inPrivate ?? browserInfo.EnablePrivate}, hasPrivateArg={hasPrivateArg}, hasExtraArgs={hasExtraArgs}", + e + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/PublicAPIInstance.cs` around lines 467 - 471, The log call inside the failed-open-browser block is leaking browserInfo.PrivateArg; update the LogException invocation in the same method (the block that builds includesExtraArgs) to avoid logging the raw PrivateArg value—replace browserInfo.PrivateArg with a redacted indicator (e.g., "[REDACTED_PRIVATE_ARG]" or a boolean/presence marker) or omit it entirely, keeping the rest of the message format and symbols (ClassName, tabOrWindow, path, inPrivate/browserInfo.EnablePrivate, includesExtraArgs) unchanged so LogException still provides context without exposing sensitive flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Flow.Launcher/PublicAPIInstance.cs`:
- Around line 467-471: The log call inside the failed-open-browser block is
leaking browserInfo.PrivateArg; update the LogException invocation in the same
method (the block that builds includesExtraArgs) to avoid logging the raw
PrivateArg value—replace browserInfo.PrivateArg with a redacted indicator (e.g.,
"[REDACTED_PRIVATE_ARG]" or a boolean/presence marker) or omit it entirely,
keeping the rest of the message format and symbols (ClassName, tabOrWindow,
path, inPrivate/browserInfo.EnablePrivate, includesExtraArgs) unchanged so
LogException still provides context without exposing sensitive flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24b14ce7-2a3e-4b34-8052-3727083a7e0e
📒 Files selected for processing (1)
Flow.Launcher/PublicAPIInstance.cs
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.
Summary
Should close #2653
Doesn't explicitly handle profiles but this should allow users to set that up themselves.
UI Change
Summary by cubic
Adds an Extra Args field to custom browser settings so users can pass command-line flags when opening URLs. Works for both new window and new tab, and logs now indicate when Extra Args were provided without recording their contents.
OpenInBrowserWindow/OpenInBrowserTabaccept optionalextraArgsand include them inProcessStartInfo.Arguments;PublicAPIInstance.OpenUriforwardsExtraArgs; error logs now omitExtraArgsvalues and add a notice when present;SelectBrowserWindowlayout updated to fit the new field.ExtraArgstoCustomBrowserViewModel(and itsCopy()), TextBox binding inSelectBrowserWindow.xaml, and locale keydefaultBrowser_extraArgs.ExtraArgs, reducing risk of leaking sensitive flags.Written for commit 246910c. Summary will update on new commits.