Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions code/core/assets/server/openBrowser.applescript
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
(*
Copyright (c) 2015-present, Facebook, Inc.
This source code is licensed under the MIT license found in the
LICENSE file in the root directory of this source tree.
*)

property targetTab: null
property targetTabIndex: -1
property targetWindow: null
property theProgram: "Google Chrome"

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

Comment on lines +12 to +21

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

  1. Keep script backward-robust: default matchURL to theURL when 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
  1. 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.

Suggested change
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.

using terms from application "Google Chrome"
tell application theProgram

if (count every window) = 0 then
make new window
end if

-- 1: Looking for tab running debugger
-- then, Reload debugging tab if found
-- then return
set found to my lookupTabWithUrl(matchURL)
if found then
set targetWindow's active tab index to targetTabIndex
tell targetTab to reload
tell targetWindow to activate
set index of targetWindow to 1
return
end if

-- 2: Looking for Empty tab
-- In case debugging tab was not found
-- We try to find an empty tab instead
set found to my lookupTabWithUrl("chrome://newtab/")
if found then
set targetWindow's active tab index to targetTabIndex
set URL of targetTab to theURL
tell targetWindow to activate
return
end if

-- 3: Create new tab
-- both debugging and empty tab were not found
-- make a new tab with url
tell window 1
activate
make new tab with properties {URL:theURL}
end tell
end tell
end using terms from
end run

-- Function:
-- Lookup tab with given url
-- if found, store tab, index, and window in properties
-- (properties were declared on top of file)
on lookupTabWithUrl(lookupUrl)
using terms from application "Google Chrome"
tell application theProgram
-- Find a tab with the given url
set found to false
set theTabIndex to -1
repeat with theWindow in every window
set theTabIndex to 0
repeat with theTab in every tab of theWindow
set theTabIndex to theTabIndex + 1
if (theTab's URL as string) contains lookupUrl then
-- assign tab, tab index, and window to properties
set targetTab to theTab
set targetTabIndex to theTabIndex
set targetWindow to theWindow
set found to true
exit repeat
end if
end repeat

if found then
exit repeat
end if
end repeat
end tell
end using terms from
return found
end lookupTabWithUrl
2 changes: 1 addition & 1 deletion code/core/src/core-server/dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { getServerChannel } from './utils/get-server-channel';
import { getAccessControlMiddleware } from './utils/getAccessControlMiddleware';
import { getStoryIndexGenerator } from './utils/getStoryIndexGenerator';
import { getMiddleware } from './utils/middleware';
import { openInBrowser } from './utils/open-in-browser';
import { openInBrowser } from './utils/open-browser/open-in-browser';
import { getServerAddresses } from './utils/server-address';
import { getServer } from './utils/server-init';
import { useStatics } from './utils/server-statics';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,27 @@ import { logger } from 'storybook/internal/node-logger';
import open from 'open';
import { dedent } from 'ts-dedent';

import { openBrowser } from './opener';

export async function openInBrowser(address: string) {
let errorOccured = false;

try {
await openBrowser(address);
} catch (error) {
errorOccured = true;
}
Comment on lines +9 to +15

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


try {
await open(address);
if (errorOccured) {
await open(address);
errorOccured = false;
}
} catch (error) {
errorOccured = true;
}

Comment on lines +9 to +25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

if (errorOccured) {
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
Expand Down
171 changes: 171 additions & 0 deletions code/core/src/core-server/utils/open-browser/opener.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the LICENSE file in the root
* directory of this source tree.
*/
import { execSync } from 'node:child_process';
import { join } from 'node:path';

import spawn from 'cross-spawn';
import open, { type App } from 'open';
import picocolors from 'picocolors';

import { resolvePackageDir } from '../../../common';

// https://github.com/sindresorhus/open#app
const OSX_CHROME = 'google chrome';

const Actions = Object.freeze({
NONE: 0,
BROWSER: 1,
SCRIPT: 2,
});

function getBrowserEnv() {
// Attempt to honor this environment variable.
// It is specific to the operating system.
// See https://github.com/sindresorhus/open#app for documentation.
const value = process.env.BROWSER;
const args = process.env.BROWSER_ARGS ? process.env.BROWSER_ARGS.split(' ') : [];
let action;
if (!value) {
// Default.
action = Actions.BROWSER;
} else if (value.toLowerCase().endsWith('.js')) {
action = Actions.SCRIPT;
} else if (value.toLowerCase() === 'none') {
action = Actions.NONE;
} else {
action = Actions.BROWSER;
}
return { action, value, args };
}

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;
}
Comment thread
ndelangen marked this conversation as resolved.

function startBrowserProcess(
browser: App | readonly App[] | undefined,
url: string,
args: string[]
) {
// If we're on OS X, the user hasn't specifically
// requested a different browser, we can try opening
// Chrome with AppleScript. This lets us reuse an
// existing tab when possible instead of creating a new one.
const shouldTryOpenChromiumWithAppleScript =
process.platform === 'darwin' && (typeof browser !== 'string' || browser === OSX_CHROME);

if (shouldTryOpenChromiumWithAppleScript) {
// Will use the first open browser found from list
const supportedChromiumBrowsers = [
'Google Chrome Canary',
'Google Chrome Dev',
'Google Chrome Beta',
'Google Chrome',
'Microsoft Edge',
'Brave Browser',
'Arc',
'Vivaldi',
'Chromium',
];

for (const chromiumBrowser of supportedChromiumBrowsers) {
try {
// Try our best to reuse existing tab
// on OSX Chromium-based browser with AppleScript
execSync(`ps cax | grep "${chromiumBrowser}"`);
const pathToApplescript = join(
resolvePackageDir('storybook'),
'assets',
'server',
'openBrowser.applescript'
);

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,
});

Comment on lines +100 to +113

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

return true;
} catch (err) {
// Ignore errors.
}
}
}

// Another special case: on OS X, check if BROWSER has been set to "open".
// In this case, instead of passing `open` to `opn` (which won't work),
// just ignore it (thus ensuring the intended behavior, i.e. opening the system browser):
// https://github.com/facebook/create-react-app/pull/1690#issuecomment-283518768
// @ts-expect-error - browser is a string
if (process.platform === 'darwin' && browser === 'open') {
browser = undefined;
}

// If there are arguments, they must be passed as array with the browser
if (typeof browser === 'string' && args.length > 0) {
// @ts-expect-error - browser is a string
browser = [browser].concat(args);
}

// 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;
}
}
Comment on lines +136 to +145

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
// 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.


/**
* Reads the BROWSER environment variable and decides what to do with it. Returns true if it opened
* a browser or ran a node.js script, otherwise false.
*/
export function openBrowser(url: string) {
const { action, value, args } = getBrowserEnv();
switch (action) {
case Actions.NONE: {
// Special case: BROWSER="none" will prevent opening completely.
return false;
}
case Actions.SCRIPT: {
if (!value) {
throw new Error('BROWSER environment variable is not set.');
}
return executeNodeScript(value, url);
}
case Actions.BROWSER: {
return startBrowserProcess(value as App | readonly App[] | undefined, url, args);
}
default: {
throw new Error('Not implemented.');
}
}
}
Loading