Skip to content

fix(cli): accept browser --window placed after the subcommand (#1850) - #1926

Closed
LeoLin990405 wants to merge 1 commit into
jackwener:mainfrom
LeoLin990405:fix/browser-window-option-position
Closed

fix(cli): accept browser --window placed after the subcommand (#1850)#1926
LeoLin990405 wants to merge 1 commit into
jackwener:mainfrom
LeoLin990405:fix/browser-window-option-position

Conversation

@LeoLin990405

Copy link
Copy Markdown
Contributor

What

opencli browser <session> <subcommand> ... --window <mode> fails with error: unknown option '--window' (#1850).

--window is registered on the browser parent command, so commander only accepts it before the leaf subcommand:

  • opencli browser work --window background open https://x.com
  • opencli browser work open https://x.com --window background ← the obvious placement

Fix

rewriteBrowserArgv (the existing browser <session> <subcommand> argv preprocessor) now hoists a trailing --window <mode> / --window=<mode> to the parent option slot, so both placements parse identically. argv without --window is unchanged; only the first occurrence is hoisted; the scan stops at --.

Tests / verification

  • 4 new rewriteBrowserArgv cases: trailing form, = form, already-in-slot no-op, and with a leading --profile. vitest run src/cli-argv-preprocess.test.ts32 passed.
  • Live: opencli browser <session> open https://example.com --window background now opens the page (previously errored with unknown option '--window').

Fixes #1850

`--window` is a `browser` parent-command option, so commander only accepted
it before the leaf subcommand. The natural placement
`opencli browser work open <url> --window background` failed with
`error: unknown option '--window'` (jackwener#1850).

rewriteBrowserArgv now hoists a trailing `--window <mode>` / `--window=<mode>`
to the parent option slot (before the subcommand), so both placements parse the
same. argv without `--window` is untouched.

Tests: 4 new rewriteBrowserArgv cases (trailing form, `=` form, already-in-slot
no-op, with `--profile`); `vitest run src/cli-argv-preprocess.test.ts` -> 32 passed.
Verified live: `browser <session> open <url> --window background` now opens the page.

Fixes jackwener#1850
@jackwener

Copy link
Copy Markdown
Owner

Thanks for the focused fix. I compared this with #1899 because both PRs address #1850.

I opened a replacement PR that uses your lower-blast approach as the core design: keep --window as a browser namespace option and hoist the natural trailing form in rewriteBrowserArgv, instead of changing the browser command tree.

Replacement PR: #1963

I also credited you in the replacement commit as a co-author:

Co-authored-by: Zhongyue Lin <zhongyuelin990405@gmail.com>

Main differences in #1963:

  • adds nested command coverage like browser work get url --window background and browser work tab close ... --window background
  • avoids rewriting anything after literal --
  • avoids hoisting a bare --window without a value, so Commander cannot accidentally consume the subcommand token as the option value
  • adds a browser help example for the trailing placement

@LeoLin990405

Copy link
Copy Markdown
Contributor Author

Makes sense — #1963 is the better version (nested-command coverage + the -- / bare---window edge cases). Closing this in favor of it. Thanks for folding in the approach and the co-author credit! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] opencli browser --window only works before the leaf subcommand

2 participants