Dev: Improve the browser opening experience #32488
Conversation
…opening logic - Introduced `openBrowser.applescript` to manage tab reuse in Google Chrome on macOS. - Updated `openInBrowser` function to utilize the new `openBrowser` utility for improved error handling. - Created `opener.ts` to encapsulate browser opening logic, including handling different browser environments and executing scripts.
WalkthroughAdds an AppleScript to open/reuse Chrome tabs, introduces a new URL-opening utility with environment-driven behavior, updates open-in-browser to use the new utility with a fallback, and adjusts an import path in the dev server. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant DevServer
participant OpenInBrowser
participant Opener as openBrowser()
participant OpenLib as open()
DevServer->>OpenInBrowser: openInBrowser(address)
OpenInBrowser->>Opener: openBrowser(address)
alt Success
Opener-->>OpenInBrowser: true
OpenInBrowser-->>DevServer: done
else Throws / Fails
Opener-->>OpenInBrowser: error
OpenInBrowser->>OpenLib: open(address)
alt Fallback success
OpenLib-->>OpenInBrowser: ok
OpenInBrowser-->>DevServer: done
else Fallback fails
OpenLib-->>OpenInBrowser: error
OpenInBrowser-->>DevServer: log error
end
end
sequenceDiagram
autonumber
participant Opener as openBrowser()
participant Env as getBrowserEnv()
participant AppleScript as openBrowser.applescript
participant OpenLib as open()
Opener->>Env: resolve action/value/args
alt Action = SCRIPT
Opener->>Opener: executeNodeScript(path, url)
Opener-->>Opener: boolean result
else Action = BROWSER on macOS
Opener->>AppleScript: try reuse tab (Chrome/Chromium list)
alt Reuse succeeded
AppleScript-->>Opener: true
else Reuse not possible
Opener->>OpenLib: open(url, app/args)
OpenLib-->>Opener: result
end
else Action = NONE
Opener-->>Opener: false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 63a55b0
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
code/core/assets/server/openBrowser.applescript (1)
41-50: Chromium “new tab” URL is Chrome‑specificYou’re matching
chrome://newtab/. Other Chromium browsers can use different URLs (e.g.,edge://newtab/,brave://newtab/,about:blank). This doesn’t break functionality (we still create a tab), but it reduces reuse likelihood outside Chrome.Consider checking both
about:blankand a program‑specific new‑tab URL derived fromtheProgram, falling back to the currentchrome://newtab/.code/core/src/core-server/utils/open-browser/opener.ts (3)
109-121: Type and guard fixes around macOS "open" special caseThe
@ts-expect-errorcomments are unnecessary if we typebrowserto include strings. Also add a type guard to avoid comparing non-strings.Apply:
- // @ts-expect-error - browser is a string - if (process.platform === 'darwin' && browser === 'open') { + if (process.platform === 'darwin' && typeof browser === 'string' && browser === 'open') { browser = undefined; }And see the type cleanups below.
11-11: Clean up ‘open’ types to avoid@ts-expect-errorand better reflect realityDerive the
appoption type fromopento stay aligned across versions. This removes the need for@ts-expect-errorand makes string handling first-class.-import open, { type App } from 'open'; +import open from 'open'; +type AppOption = NonNullable<Parameters<typeof open>[1]>['app']; @@ -function startBrowserProcess( - browser: App | readonly App[] | undefined, +function startBrowserProcess( + browser: AppOption | undefined, url: string, args: string[] ) { @@ - if (typeof browser === 'string' && args.length > 0) { - // @ts-expect-error - browser is a string - browser = [browser].concat(args); + if (typeof browser === 'string' && args.length > 0) { + browser = [browser].concat(args) as unknown as AppOption; } @@ -export function openBrowser(url: string) { +export function openBrowser(url: string) { const { action, value, args } = getBrowserEnv(); switch (action) { @@ - case Actions.BROWSER: { - return startBrowserProcess(value as App | readonly App[] | undefined, url, args); + case Actions.BROWSER: { + return startBrowserProcess(value as AppOption | undefined, url, args); }Also applies to: 62-66, 118-123, 139-154
25-43: Space-splitting BROWSER_ARGS is brittle
split(' ')breaks on quoted args or escaped spaces.Use a robust parser like
string-argvorshell-quote, or minimally split with a small state machine. Example:import { parseArgsStringToArgv } from 'string-argv'; const args = process.env.BROWSER_ARGS ? parseArgsStringToArgv(process.env.BROWSER_ARGS) : [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/assets/server/openBrowser.applescript(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(1 hunks)code/core/src/core-server/utils/open-in-browser.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
code/core/src/core-server/utils/open-browser/opener.ts (1)
code/core/src/shared/utils/module.ts (1)
resolvePackageDir(27-32)
code/core/src/core-server/utils/open-in-browser.ts (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
openBrowser(139-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
92-100: Pathing/package check — openBrowser.applescript present and includedFound code/core/assets/server/openBrowser.applescript and code/core/package.json lists "assets/**/*" in its "files" array, so the AppleScript will be published. If resolvePackageDir('storybook') resolves to a different package, confirm that package's "files" includes the assets path.
| on run argv | ||
| set theURL to item 1 of argv | ||
| set matchURL to item 2 of argv | ||
|
|
||
| -- Allow requested program to be optional, | ||
| -- default to Google Chrome | ||
| if (count of argv) > 2 then | ||
| set theProgram to item 3 of argv | ||
| end if | ||
|
|
There was a problem hiding this comment.
Arg parsing mismatch with caller causes wrong browser control and tab matching
opener.ts passes only two args (url, program name). Here, arg 2 is interpreted as matchURL and the program remains the default ("Google Chrome"). Result:
- The script looks for a tab whose URL contains the browser name (never matches).
- Actions run against Google Chrome even when Edge/Brave/Vivaldi were detected.
Apply one of the fixes below. I recommend doing both for robustness.
- Keep script backward-robust: default
matchURLtotheURLwhen only two args are provided, and treat arg 2 as program if it doesn’t look like a URL.
on run argv
- set theURL to item 1 of argv
- set matchURL to item 2 of argv
+ set theURL to item 1 of argv
+ -- default: try to match the exact URL we plan to open
+ set matchURL to theURL
+ if (count of argv) ≥ 2 then
+ set secondArg to item 2 of argv
+ if (secondArg contains "://") then
+ set matchURL to secondArg
+ else
+ set theProgram to secondArg
+ end if
+ end if
-- Allow requested program to be optional,
-- default to Google Chrome
- if (count of argv) > 2 then
- set theProgram to item 3 of argv
- end if
+ if (count of argv) > 2 then set theProgram to item 3 of argv- In
opener.ts, pass three args:url(open),url(match), and the program name (see my comment there for a patch).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on run argv | |
| set theURL to item 1 of argv | |
| set matchURL to item 2 of argv | |
| -- Allow requested program to be optional, | |
| -- default to Google Chrome | |
| if (count of argv) > 2 then | |
| set theProgram to item 3 of argv | |
| end if | |
| on run argv | |
| set theURL to item 1 of argv | |
| -- default: try to match the exact URL we plan to open | |
| set matchURL to theURL | |
| if (count of argv) ≥ 2 then | |
| set secondArg to item 2 of argv | |
| if (secondArg contains "://") then | |
| set matchURL to secondArg | |
| else | |
| set theProgram to secondArg | |
| end if | |
| end if | |
| -- Allow requested program to be optional, | |
| -- default to Google Chrome | |
| if (count of argv) > 2 then set theProgram to item 3 of argv |
🤖 Prompt for AI Agents
In code/core/assets/server/openBrowser.applescript around lines 12 to 21, the
script currently treats argv[2] as matchURL and only uses a third arg for the
program, which mis-parses the two-argument caller and causes wrong browser/tab
selection; update parsing so that when only two args are passed set matchURL to
theURL by default, and if a second arg is present detect whether it looks like a
URL (e.g., starts with "http" or contains "://") — if it looks like a URL treat
it as matchURL, otherwise treat it as theProgram; also accept a third arg as
theProgram if provided. Ensure theProgram defaults to "Google Chrome" only if no
program is identified.
…nd `opener` - Improved error handling in `openInBrowser` by logging errors and conditionally opening the address based on previous errors. - Added support for the 'Arc' browser in the `startBrowserProcess` function. - Refactored command construction for executing AppleScript to improve readability and maintainability.
ndelangen
changed the title
…pener` - Removed unnecessary error logging in `openInBrowser` and `opener` to streamline the code. - Improved readability by eliminating commented-out code and redundant console statements.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
code/core/src/core-server/utils/open-browser/opener.ts (2)
71-89: AppleScript gate is too narrow; skips tab‑reuse when BROWSER is Brave/Edge/etc.Only enabling reuse when BROWSER is unset or “google chrome” excludes other supported Chromium browsers explicitly set by users.
Apply this diff:
- const shouldTryOpenChromiumWithAppleScript = - process.platform === 'darwin' && (typeof browser !== 'string' || browser === OSX_CHROME); + const shouldTryOpenChromiumWithAppleScript = process.platform === 'darwin'; @@ - if (shouldTryOpenChromiumWithAppleScript) { + if (shouldTryOpenChromiumWithAppleScript) { @@ - for (const chromiumBrowser of supportedChromiumBrowsers) { + const candidates = typeof browser === 'string' ? [browser] : supportedChromiumBrowsers; + for (const program of candidates) { + if (!supportedChromiumBrowsers.includes(program)) continue; @@ - execSync(`ps cax | grep "${chromiumBrowser}"`); + execSync(`ps cax | grep "${program}"`); @@ - .concat(chromiumBrowser, '"'); + .concat(program, '"');
45-60: executeNodeScript reports success even when the script fails; callers can’t decide correctlyThis returns true immediately; failures only log later, breaking fallback decisions upstream.
Apply this diff:
-function executeNodeScript(scriptPath: string, url: string) { - const extraArgs = process.argv.slice(2); - const child = spawn(process.execPath, [scriptPath, ...extraArgs, url], { - stdio: 'inherit', - }); - child.on('close', (code) => { - if (code !== 0) { - console.log(); - console.log(picocolors.red('The script specified as BROWSER environment variable failed.')); - console.log(`${picocolors.cyan(scriptPath)} exited with code ${code}.`); - console.log(); - return; - } - }); - return true; -} +function executeNodeScript(scriptPath: string, url: string) { + const extraArgs = process.argv.slice(2); + const { status } = spawnSync(process.execPath, [scriptPath, ...extraArgs, url], { + stdio: 'inherit', + }); + if (status !== 0) { + console.log(); + console.log(picocolors.red('The script specified as BROWSER environment variable failed.')); + console.log(`${picocolors.cyan(scriptPath)} exited with code ${status}.`); + console.log(); + return false; + } + return true; +}Additionally (imports):
-import { execSync } from 'node:child_process'; +import { execSync, spawnSync } from 'node:child_process'; -import spawn from 'cross-spawn';
🧹 Nitpick comments (2)
code/core/src/core-server/utils/open-browser/opener.ts (2)
25-43: Parse BROWSER_ARGS like a shell; split(' ') breaks quotes/escaped spacesUsers commonly pass quoted args; naive split misparses.
Apply this diff:
- const args = process.env.BROWSER_ARGS ? process.env.BROWSER_ARGS.split(' ') : []; + const args = process.env.BROWSER_ARGS + ? require('string-argv').parseArgsStringToArgv(process.env.BROWSER_ARGS) + : [];If avoiding a new dep, mirror a minimal shell-like parser locally.
136-145: Return value from open() is optimistic; consider async return to reflect failuresstartBrowserProcess always returns true after scheduling open(), even if it rejects later.
Make startBrowserProcess async and await open(url, options) to propagate a real boolean up to openBrowser/openInBrowser, or document the optimistic behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/core-server/dev-server.ts(1 hunks)code/core/src/core-server/utils/open-browser/open-in-browser.ts(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- code/core/src/core-server/dev-server.ts
🧰 Additional context used
🧬 Code graph analysis (2)
code/core/src/core-server/utils/open-browser/open-in-browser.ts (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
openBrowser(151-171)
code/core/src/core-server/utils/open-browser/opener.ts (1)
code/core/src/shared/utils/module.ts (1)
resolvePackageDir(27-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/core/src/core-server/utils/open-browser/open-in-browser.ts (1)
6-6: Good integration pointImporting the centralized opener here keeps dev-server logic clean and future-proof.
code/core/src/core-server/utils/open-browser/opener.ts (1)
93-99: ```shell
#!/bin/bash
set -euo pipefailecho "pwd: $(pwd)"
echo1) Locate the AppleScript
aspath=$(find code -type f -path '*/assets/server/openBrowser.applescript' -print -quit || true)
echo "AppleScript path: '${aspath}'"
echoif [ -z "$aspath" ]; then
echo "No AppleScript found under code/; exiting."
exit 0
fi2) Find nearest package.json by walking up directories from the AppleScript
pdir=$(dirname "$aspath")
found_pkg=""
while [ "$pdir" != "/" ] && [ -n "$pdir" ]; do
if [ -f "$pdir/package.json" ]; then
found_pkg="$pdir/package.json"
echo "Nearest package.json: $found_pkg"
echo "Selected keys from that package.json:"
PY_PKG="$found_pkg" python3 - <<'PY'
import os,json
p=os.environ['PY_PKG']
j=json.load(open(p))
for k in ("name","private","files","exports","main","version","publishConfig"):
if k in j:
print(f"{k}: {j[k]}")
PY
break
fi
pdir=$(dirname "$pdir")
done3) List all package.json under code/ and show name/files/exports for each
echo
echo "All package.json files under code/:"
find code -type f -name package.json -print0 | while IFS= read -r -d '' pkg; do
echo "---- $pkg ----"
PY_PKG="$pkg" python3 - <<'PY'
import os,json
p=os.environ['PY_PKG']
try:
j=json.load(open(p))
except Exception as e:
print("error reading:", e); raise SystemExit
print("name:", j.get("name"))
print("private:", j.get("private"))
if "files" in j:
print("files:", j["files"])
if "exports" in j:
print("exports:", j["exports"])
PY
done4) Print .npmignore / .gitignore contents in ancestors of the AppleScript
echo
echo ".npmignore / .gitignore in ancestors of the AppleScript:"
pdir=$(dirname "$aspath")
while [ "$pdir" != "/" ] && [ -n "$pdir" ]; do
if [ -f "$pdir/.npmignore" ]; then
echo "---- $pdir/.npmignore ----"
sed -n '1,200p' "$pdir/.npmignore"
fi
if [ -f "$pdir/.gitignore" ]; then
echo "---- $pdir/.gitignore ----"
sed -n '1,200p' "$pdir/.gitignore"
fi
pdir=$(dirname "$pdir")
done5) Grep repo for literal mentions
echo
echo "Looking for literal 'assets/server' or 'openBrowser.applescript' in repo:"
rg -n 'assets/server|openBrowser.applescript' || trueecho
echo "Done."</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| let errorOccured = false; | ||
|
|
||
| try { | ||
| await openBrowser(address); | ||
| } catch (error) { | ||
| errorOccured = true; | ||
| } |
There was a problem hiding this comment.
Fix fallback gating: handle boolean return from openBrowser; respect BROWSER=none; typo
openBrowser returns boolean; you ignore it and only fall back on throw, so valid “false” (including script failures) never triggers the fallback. Also, respect BROWSER=none to avoid logging/noise. And fix “errorOccured” typo.
Apply this diff:
- let errorOccured = false;
-
- try {
- await openBrowser(address);
- } catch (error) {
- errorOccured = true;
- }
-
- try {
- if (errorOccured) {
- await open(address);
- errorOccured = false;
- }
- } catch (error) {
- errorOccured = true;
- }
-
- if (errorOccured) {
+ const browserNone = process.env.BROWSER?.toLowerCase() === 'none';
+ let opened = false;
+ try {
+ opened = openBrowser(address); // returns boolean
+ } catch {
+ opened = false;
+ }
+ if (!opened && !browserNone) {
+ try {
+ await open(address);
+ opened = true;
+ } catch {
+ opened = false;
+ }
+ }
+ if (!opened && !browserNone) {Also applies to: 17-24, 26-26
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/open-browser/open-in-browser.ts around lines
9-15 (and similarly 17-24, 26), the code ignores the boolean return value from
openBrowser, only treating thrown errors as failures, uses a typoed variable
name errorOccured, and doesn't honor BROWSER=none; update the logic to (1)
rename errorOccured to errorOccurred, (2) capture the boolean result from await
openBrowser(address) and treat a false return the same as a caught error to
trigger fallback handling, and (3) short-circuit and do nothing (no
logging/fallback) when process.env.BROWSER === 'none'; apply the same fixes at
the other indicated line ranges.
| const command = `osascript "${pathToApplescript}" \"` | ||
| .concat(encodeURI(url), '" "') | ||
| .concat( | ||
| process.env.OPEN_MATCH_HOST_ONLY === 'true' | ||
| ? encodeURI(new URL(url).origin) | ||
| : encodeURI(url), | ||
| '" "' | ||
| ) | ||
| .concat(chromiumBrowser, '"'); | ||
|
|
||
| execSync(command, { | ||
| cwd: __dirname, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shell string building; pass args with execFileSync to handle quoting safely
Safer and simpler, especially with arbitrary URLs.
Apply this diff:
- const command = `osascript "${pathToApplescript}" \"`
- .concat(encodeURI(url), '" "')
- .concat(
- process.env.OPEN_MATCH_HOST_ONLY === 'true'
- ? encodeURI(new URL(url).origin)
- : encodeURI(url),
- '" "'
- )
- .concat(chromiumBrowser, '"');
-
- execSync(command, {
- cwd: __dirname,
- });
+ const matchArg =
+ process.env.OPEN_MATCH_HOST_ONLY === 'true'
+ ? encodeURI(new URL(url).origin)
+ : encodeURI(url);
+ execFileSync('osascript', [pathToApplescript, encodeURI(url), matchArg, program], {
+ stdio: 'ignore',
+ });Also update imports:
-import { execSync } from 'node:child_process';
+import { execSync, execFileSync } from 'node:child_process';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/open-browser/opener.ts around lines 100 to
113, the current code builds a shell command string for osascript and calls
execSync which is unsafe for arbitrary URLs; replace this with execFileSync from
child_process and pass an args array instead of a single quoted string: import
execFileSync (replace or add to the existing child_process imports), build args
as [pathToApplescript, encodedUrl, encodedHostOrUrl, chromiumBrowser] (use
encodeURI and new URL(url).origin when OPEN_MATCH_HOST_ONLY==='true'), and call
execFileSync('osascript', args, { cwd: __dirname }); ensure no shell quoting is
used and cwd remains set.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
code/core/src/core-server/utils/open-browser/opener.ts (2)
88-118: AppleScript invocation looks correct nowThree args are passed: open URL, match URL (origin toggle), and the target program. This resolves the earlier argument‑order issue.
45-60: executeNodeScript returns success before the child exits; callers can’t detect failuresRun the script synchronously and return a boolean based on the exit status.
Apply:
- import spawn from 'cross-spawn'; + import { spawnSync } from 'node:child_process';function executeNodeScript(scriptPath: string, url: string) { - const extraArgs = process.argv.slice(2); - const child = spawn(process.execPath, [scriptPath, ...extraArgs, url], { - stdio: 'inherit', - }); - child.on('close', (code) => { - if (code !== 0) { - console.log(); - console.log(picocolors.red('The script specified as BROWSER environment variable failed.')); - console.log(`${picocolors.cyan(scriptPath)} exited with code ${code}.`); - console.log(); - return; - } - }); - return true; + const extraArgs = process.argv.slice(2); + const { status } = spawnSync(process.execPath, [scriptPath, ...extraArgs, url], { + stdio: 'inherit', + }); + if (status !== 0) { + console.log(); + console.log(picocolors.red('The script specified as BROWSER environment variable failed.')); + console.log(`${picocolors.cyan(scriptPath)} exited with code ${status}.`); + console.log(); + return false; + } + return true; }
🧹 Nitpick comments (4)
code/core/src/core-server/utils/open-browser/opener.ts (3)
71-73: Make the Chrome gate case‑insensitiveHonor BROWSER="Google Chrome" (mixed case) for AppleScript reuse.
- const shouldTryOpenChromiumWithAppleScript = - process.platform === 'darwin' && (typeof browser !== 'string' || browser === OSX_CHROME); + const shouldTryOpenChromiumWithAppleScript = + process.platform === 'darwin' && + (typeof browser !== 'string' || + (typeof browser === 'string' && browser.toLowerCase() === OSX_CHROME));
110-113: __dirname may be undefined under ESMAvoid relying on __dirname; execSync doesn’t need cwd here.
- execSync(command, { - cwd: __dirname, - }); + execSync(command);Alternatively compute cwd via fileURLToPath(new URL('.', import.meta.url)).
25-43: BROWSER_ARGS splitting is not shell‑awareSpace‑split breaks quoted args. Consider a shell‑aware split.
Example with shell-quote (minimal dep):
import { parse } from 'shell-quote'; const args = process.env.BROWSER_ARGS ? (parse(process.env.BROWSER_ARGS).filter((x) => typeof x === 'string') as string[]) : [];code/core/src/core-server/utils/open-browser/open-in-browser.ts (1)
26-33: Only log when both attempts failed; fix variable typoSimplify and correct “Occurred”.
- if (errorOccured) { + if (!(await openBrowser(address))) { + // Second attempt already failed above logger.error(dedent` Could not open ${address} inside a browser. If you're running this command inside a docker container or on a CI, you need to pass the '--ci' flag to prevent opening a browser by default. `); - } + }If you keep a flag, prefer “errorOccurred”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/core-server/dev-server.ts(1 hunks)code/core/src/core-server/utils/open-browser/open-in-browser.ts(1 hunks)code/core/src/core-server/utils/open-browser/opener.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
code/core/src/core-server/utils/open-browser/opener.ts (1)
code/core/src/shared/utils/module.ts (1)
resolvePackageDir(27-32)
code/core/src/core-server/utils/open-browser/open-in-browser.ts (1)
code/core/src/core-server/utils/open-browser/opener.ts (1)
openBrowser(151-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/core-server/dev-server.ts (1)
19-19: Import path update looks rightMatches the new open-browser module structure.
| let errorOccured = false; | ||
|
|
||
| try { | ||
| await openBrowser(address); | ||
| } catch (error) { | ||
| errorOccured = true; | ||
| } | ||
|
|
||
| try { | ||
| await open(address); | ||
| if (errorOccured) { | ||
| await open(address); | ||
| errorOccured = false; | ||
| } | ||
| } catch (error) { | ||
| errorOccured = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle boolean result from openBrowser; perform fallback only on false
With the current code, awaiting a boolean won’t throw, so the fallback never runs. Align with the two‑stage design after making openBrowser async/boolean‑returning.
- let errorOccured = false;
-
- try {
- await openBrowser(address);
- } catch (error) {
- errorOccured = true;
- }
-
- try {
- if (errorOccured) {
- await open(address);
- errorOccured = false;
- }
- } catch (error) {
- errorOccured = true;
- }
+ const ok = await openBrowser(address);
+ if (!ok) {
+ try {
+ await open(address);
+ return;
+ } catch {}
+ }And keep the error log block to run only when both attempts failed (see next hunk).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let errorOccured = false; | |
| try { | |
| await openBrowser(address); | |
| } catch (error) { | |
| errorOccured = true; | |
| } | |
| try { | |
| await open(address); | |
| if (errorOccured) { | |
| await open(address); | |
| errorOccured = false; | |
| } | |
| } catch (error) { | |
| errorOccured = true; | |
| } | |
| const ok = await openBrowser(address); | |
| if (!ok) { | |
| try { | |
| await open(address); | |
| return; | |
| } catch {} | |
| } |
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/open-browser/open-in-browser.ts around lines
9 to 25, the current try/catch assumes await openBrowser(address) will throw on
failure but openBrowser is async and returns a boolean; change the logic to
check the boolean result instead of relying on exceptions: await
openBrowser(address) into a const result; if result === false then attempt the
fallback await open(address). Track failure state so that the final error
logging block runs only when both the primary returned false (or threw) and the
fallback either returned/failed or threw — i.e., set errorOccurred only when
both attempts failed and only then emit the error log.
| // Fallback to open | ||
| // (It will always open new tab) | ||
| try { | ||
| const options = { app: browser, wait: false, url: true }; | ||
| open(url, options).catch(() => {}); // Prevent `unhandledRejection` error. | ||
| return true; | ||
| } catch (err) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Return a truthful result; don’t swallow open() failures, and avoid duplicating the fallback
open() is async; the current code always returns true and hides failures, breaking the two‑stage flow in open-in-browser.ts.
Make startBrowserProcess and openBrowser async and propagate success/failure:
-function startBrowserProcess(
+async function startBrowserProcess(
browser: App | readonly App[] | undefined,
url: string,
args: string[]
) {
@@
- try {
- const options = { app: browser, wait: false, url: true };
- open(url, options).catch(() => {}); // Prevent `unhandledRejection` error.
- return true;
- } catch (err) {
- return false;
- }
+ try {
+ const options = { app: browser, wait: false, url: true };
+ await open(url, options);
+ return true;
+ } catch {
+ return false;
+ }-export function openBrowser(url: string) {
+export async function openBrowser(url: string): Promise<boolean> {
const { action, value, args } = getBrowserEnv();
switch (action) {
@@
- case Actions.BROWSER: {
- return startBrowserProcess(value as App | readonly App[] | undefined, url, args);
+ case Actions.BROWSER: {
+ return await startBrowserProcess(value as App | readonly App[] | undefined, url, args);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Fallback to open | |
| // (It will always open new tab) | |
| try { | |
| const options = { app: browser, wait: false, url: true }; | |
| open(url, options).catch(() => {}); // Prevent `unhandledRejection` error. | |
| return true; | |
| } catch (err) { | |
| return false; | |
| } | |
| } | |
| async function startBrowserProcess( | |
| browser: App | readonly App[] | undefined, | |
| url: string, | |
| args: string[] | |
| ) { | |
| // ... other attempts before fallback ... | |
| // Fallback to open | |
| // (It will always open new tab) | |
| try { | |
| const options = { app: browser, wait: false, url: true }; | |
| await open(url, options); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| export async function openBrowser(url: string): Promise<boolean> { | |
| const { action, value, args } = getBrowserEnv(); | |
| switch (action) { | |
| case Actions.BROWSER: { | |
| return await startBrowserProcess(value as App | readonly App[] | undefined, url, args); | |
| } | |
| // other cases unchanged... | |
| } | |
| } |
🤖 Prompt for AI Agents
In code/core/src/core-server/utils/open-browser/opener.ts around lines 136 to
145, the fallback currently calls open(url, options).catch(()=>{}) and always
returns true, swallowing async failures and duplicating the fallback; make the
containing functions (startBrowserProcess and openBrowser) async so you can
await open(url, options) instead of fire-and-forget, remove the empty catch that
hides errors, and return a boolean based on the awaited call (true on success,
false on failure) or propagate the error to the caller—ensure you only call the
fallback once and do not suppress the open() rejection.
ndelangen
added
the
needs qa
yannbf
removed
the
needs qa
What I did
I've remove the
better-opnpackage, and embedded the code directly in the core.This includes a applescript file, which checks if a tab is already open.
This used to be embedded in the
better-opnpackage, but by moving it inside our own repo, we are in control of this pretty sensitive script. We can also improve it, and bundle better and have fewer transitive dependencies.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Ensure that when running
storybook deva browser opens, if allowed by the users, an applescript is run that searches for the best tab to refresh/reload or open a new tab.When it's not allowed or the browser does not support applescript interacting with it in that way, it always opens a new tab,
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Bug Fixes
Refactor