code/core/src/manager/components/preview/tools/share.tsx (2)

15-15: Consider migrating to qrcode.react for better TypeScript support.

The current import suggests you've already migrated to qrcode.react which provides better TypeScript support and maintenance than the previously problematic react-qr-code. This is the correct choice.

---

91-96: Extract copyStoryLink to avoid potential undefined access.

While the code has been updated to use optional chaining (shortcutKeys?.copyStoryLink), extracting it to a variable would be cleaner and ensure consistency.

Apply this diff for clarity:

 const api = useStorybookApi();
 const shortcutKeys = api.getShortcutKeys();
 const enableShortcuts = !!shortcutKeys;
 const [copied, setCopied] = useState(false);
-const copyStoryLink = shortcutKeys?.copyStoryLink;
+const copyLinkKeys = shortcutKeys?.copyStoryLink;

And update line 105:

-right: enableShortcuts ? <Shortcut keys={copyStoryLink} /> : null,
+right: enableShortcuts ? <Shortcut keys={copyLinkKeys} /> : null,

And line 141:

-}, [baseUrl, storyId, queryParams, copied, qrUrl, enableShortcuts, copyStoryLink]);
+}, [baseUrl, storyId, queryParams, copied, qrUrl, enableShortcuts, copyLinkKeys]);

code/core/src/component-testing/components/Subnav.tsx (2)

51-53: Collapse conditions into a single, intention‑revealing boolean.

Compute isEditorOpenable = Boolean(canOpenInEditor && importPath) and use it in the render. Keeps logic centralized and avoids accidental regressions.

Example (outside the diffed lines):

const isEditorOpenable = Boolean(canOpenInEditor && importPath);
...
{isEditorOpenable ? (/* clickable */) : (/* text */)}

Also applies to: 127-129

---

192-214: Guard importPath and use a semantic button; avoid asserting undefined.

Clickable branch should render only when a valid importPath exists; otherwise you risk firing a request with “undefined” and creating a dangling listener. Also, render a real button for a11y. Add a label fallback when storyFileName is absent.

-              {canOpenInEditor ? (
+              {canOpenInEditor && Boolean(importPath) ? (
                 <WithTooltip
                   trigger="hover"
                   hasChrome={false}
                   tooltip={<Note note="Open in editor" />}
                 >
-                  <StyledLocation
-                    aria-label="Open in editor"
-                    onClick={() => {
-                      openInEditor({
-                        file: importPath as string,
-                      });
-                    }}
-                  >
-                    {storyFileName}
-                  </StyledLocation>
+                  <StyledLocation
+                    as="button"
+                    type="button"
+                    aria-label="Open in editor"
+                    onClick={() => openInEditor({ file: importPath! })}
+                  >
+                    {storyFileName ?? importPath!.split(/[\\/]/).pop()}
+                  </StyledLocation>
                 </WithTooltip>
               ) : (
-                <StyledLocation isText={true}>{storyFileName}</StyledLocation>
+                <StyledLocation isText>
+                  {storyFileName ?? importPath?.split(/[\\/]/).pop()}
+                </StyledLocation>
               )}

code/core/src/manager-api/lib/open-in-editor.ts (1)

14-25: Prevent dangling listeners: add timeout/cleanup and guard empty file.

If no response arrives (e.g., no middleware in prod/composed), the promise never resolves and the listener leaks. Also bail out early on empty file.

 export async function openInEditor(payload: {
   file: string;
   line?: number;
   column?: number;
 }): Promise<OpenInEditorResponsePayload> {
-  return new Promise((resolve) => {
-    const channel = addons.getChannel();
-    const { file, line, column } = payload;
-    const handler = (res: OpenInEditorResponsePayload) => {
-      if (res.file === file && res.line === line && res.column === column) {
-        channel.off(OPEN_IN_EDITOR_RESPONSE, handler);
-        resolve(res);
-      }
-    };
-    channel.on(OPEN_IN_EDITOR_RESPONSE, handler);
-    channel.emit(OPEN_IN_EDITOR_REQUEST, payload);
-  });
+  return new Promise((resolve) => {
+    const channel = addons.getChannel();
+    const { file, line, column } = payload;
+    if (!file?.trim()) {
+      resolve({ file, line, column, error: 'Missing file path' });
+      return;
+    }
+    let timer: ReturnType<typeof setTimeout>;
+    const handler = (res: OpenInEditorResponsePayload) => {
+      if (res.file === file && res.line === line && res.column === column) {
+        channel.off(OPEN_IN_EDITOR_RESPONSE, handler);
+        clearTimeout(timer);
+        resolve(res);
+      }
+    };
+    channel.on(OPEN_IN_EDITOR_RESPONSE, handler);
+    channel.emit(OPEN_IN_EDITOR_REQUEST, payload);
+    timer = setTimeout(() => {
+      channel.off(OPEN_IN_EDITOR_RESPONSE, handler);
+      resolve({ file, line, column, error: 'Timed out waiting for editor response' });
+    }, 10000);
+  });
 }

code/core/src/manager-api/lib/stories.ts (1)

334-338: Add defensive check before accessing children array.

While the component node should always have children (established by prior logic), adding a safety check prevents potential runtime errors.

Apply this diff to add a safety check:

 if (item.type === 'component') {
   // attach importPath to the component node which should be the same for all children
   // this way we can add "open in editor" to the component node
-  item.importPath = acc[item.children[0]].importPath;
+  if (item.children?.length > 0 && acc[item.children[0]]?.importPath) {
+    item.importPath = acc[item.children[0]].importPath;
+  }
 }

code/core/src/manager/components/preview/tools/share.tsx (1)

155-157: Consider URL construction robustness.

The logic assumes STORYBOOK_NETWORK_ADDRESS is a complete URL with protocol. Consider validating or normalizing it to handle edge cases where the address might be provided without a protocol.

Consider adding validation:

-const storyUrl = global.STORYBOOK_NETWORK_ADDRESS
-  ? `${global.STORYBOOK_NETWORK_ADDRESS}${window.location.search}`
-  : window.location.href;
+const networkAddress = global.STORYBOOK_NETWORK_ADDRESS;
+const storyUrl = networkAddress
+  ? networkAddress.startsWith('http') 
+    ? `${networkAddress}${window.location.search}`
+    : `http://${networkAddress}${window.location.search}`
+  : window.location.href;

code/e2e-tests/manager.spec.ts (1)

48-49: Update selector to match renamed label.

The test still references the old "Shortcuts" label but should use "Settings" to match the UI change.

Apply this diff:

// toggle with menu item
-await sbPage.page.locator('[aria-label="Shortcuts"]').click();
+await sbPage.page.locator('[aria-label="Settings"]').click();
await sbPage.page.locator('#list-item-S').click();

Also applies to: 132-133, 171-171, 222-222, 251-251

code/core/src/component-testing/components/Subnav.tsx (1)

79-83: Reset native button chrome for StyledLocation.

When rendered “as=button”, ensure no default button styling leaks through.

 const StyledLocation = styled(P)<{ isText?: boolean }>(({ theme, isText }) => ({
   color: isText ? theme.textMutedColor : theme.color.secondary,
   cursor: isText ? 'default' : 'pointer',
   fontWeight: isText ? theme.typography.weight.regular : theme.typography.weight.bold,
+  background: 'transparent',
+  border: 0,
+  padding: 0,
   justifyContent: 'flex-end',
   textAlign: 'right',
   whiteSpace: 'nowrap',
   marginTop: 'auto',
   marginBottom: 1,
   paddingRight: 15,
   fontSize: 13,
 }));

code/core/src/manager-api/lib/open-in-editor.ts (1)

15-15: Optional: avoid getChannel() to improve determinism/testability.

Consider accepting an optional channel/api in args, defaulting to addons.getChannel(), so callers can inject and tests can stub.

code/core/src/manager/components/sidebar/Menu.tsx (4)

84-93: Mobile label is inconsistent; use “Settings” for parity

The mobile button still says “About Storybook” while the desktop one is “Settings”. Align for consistency and a11y.

Apply:

-          title="About Storybook"
-          aria-label="About Storybook"
+          title="Settings"
+          aria-label="Settings"

---

108-126: Add aria-expanded/aria-haspopup for a11y

Expose tooltip/menu state to assistive tech.

Apply:

       <SidebarIconButton
         title="Settings"
         aria-label="Settings"
+        aria-haspopup="menu"
+        aria-expanded={isTooltipVisible}
         highlighted={!!isHighlighted}
         active={isTooltipVisible}
         size="medium"
         isMobile={false}
       >

---

17-22: Avoid leaking styling-only props to the DOM

highlighted/isMobile may get forwarded to the DOM via IconButton. Use transient props and filter.

Apply:

-export const SidebarIconButton = styled(IconButton)<
-  ComponentProps<typeof Button> & {
-    highlighted: boolean;
-    isMobile: boolean;
-  }
->(({ highlighted, theme, isMobile }) => ({
+export const SidebarIconButton = styled(IconButton, {
+  // emotion option; supported by storybook/theming’s styled
+  shouldForwardProp: (prop) => prop !== '$highlighted' && prop !== '$isMobile',
+})<
+  ComponentProps<typeof Button> & {
+    $highlighted: boolean;
+    $isMobile: boolean;
+  }
+>(({ $highlighted: highlighted, theme, $isMobile: isMobile }) => ({

Also update usages in this file to pass $highlighted/$isMobile instead of highlighted/isMobile.

---

70-74: Remove ts-expect-error by aligning onClick types

Use the Button/IconButton onClick type to satisfy TS and drop the suppression.

Apply:

 export interface SidebarMenuProps {
   menu: MenuList;
   isHighlighted?: boolean;
-  onClick?: React.MouseEventHandler<HTMLButtonElement>;
+  onClick?: ComponentProps<typeof Button>['onClick'];
 }

And remove the suppression:

-          // @ts-expect-error (non strict)
           onClick={onClick}

Also applies to: 88-90

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

code/e2e-tests/manager.spec.ts (1)

93-95: Consider adding assertion for exact clipboard content.

The test currently uses toContain('Primary') which could match partial strings. Consider using a more precise assertion to ensure the exact story name is copied.

-      await expect(page.evaluate(() => navigator.clipboard.readText())).resolves.toContain(
-        'Primary'
-      );
+      await expect(page.evaluate(() => navigator.clipboard.readText())).resolves.toBe('Primary');

code/core/src/manager-api/modules/open-in-editor.tsx (5)

16-18: Remove redundant SubState interface.

The SubState interface with notifications field appears to be unnecessary since this module doesn't manage notifications state - it only uses the notifications API to display errors. The notifications state is already managed by the notifications module.

-export interface SubState {
-  notifications: API_Notification[];
-}

---

42-46: Potential race condition with concurrent requests.

The current implementation matches responses based on file, line, and column. If multiple requests for the same location are made concurrently, only the first one will resolve while others may hang indefinitely since the handler is removed after the first match.

Consider using a unique request ID to properly match request-response pairs:

-        const handler = (res: OpenInEditorResponsePayload) => {
-          if (res.file === file && res.line === line && res.column === column) {
-            provider.channel?.off(OPEN_IN_EDITOR_RESPONSE, handler);
-            resolve(res);
-          }
+        const requestId = `${Date.now()}-${Math.random()}`;
+        const handler = (res: OpenInEditorResponsePayload & { requestId?: string }) => {
+          if (res.requestId === requestId || 
+              (res.file === file && res.line === line && res.column === column)) {
+            provider.channel?.off(OPEN_IN_EDITOR_RESPONSE, handler);
+            resolve(res);
+          }
         };
         provider.channel?.on(OPEN_IN_EDITOR_RESPONSE, handler);
-        provider.channel?.emit(OPEN_IN_EDITOR_REQUEST, payload);
+        provider.channel?.emit(OPEN_IN_EDITOR_REQUEST, { ...payload, requestId });

---

40-50: Add timeout handling for unresponsive requests.

The promise never rejects if no response is received, which could leave the UI in a pending state indefinitely.

Consider adding a timeout:

       return new Promise((resolve) => {
         const { file, line, column } = payload;
+        let timeoutId: NodeJS.Timeout;
         const handler = (res: OpenInEditorResponsePayload) => {
           if (res.file === file && res.line === line && res.column === column) {
+            clearTimeout(timeoutId);
             provider.channel?.off(OPEN_IN_EDITOR_RESPONSE, handler);
             resolve(res);
           }
         };
+        timeoutId = setTimeout(() => {
+          provider.channel?.off(OPEN_IN_EDITOR_RESPONSE, handler);
+          resolve({ file, line, column, error: 'Request timed out' });
+        }, 5000);
         provider.channel?.on(OPEN_IN_EDITOR_RESPONSE, handler);
         provider.channel?.emit(OPEN_IN_EDITOR_REQUEST, payload);
       });

---

54-54: Remove unused state declaration.

The state variable initializes notifications but this module doesn't manage notification state.

-  const state: SubState = { notifications: [] };

And update the return statement:

   return {
     api,
-    state,
+    state: {},
     init: async () => {

---

67-68: Improve error message fallback.

The fallback uses logical OR which could show "Check the Storybook process..." even when payload.error is an empty string.

-              subHeadline:
-                payload.error ||
-                'Check the Storybook process on the command line for more details.',
+              subHeadline: payload.error || undefined,
+              ...(payload.error ? {} : {
+                subHeadline: 'Check the Storybook process on the command line for more details.'
+              }),

Or more simply:

-              subHeadline:
-                payload.error ||
-                'Check the Storybook process on the command line for more details.',
+              subHeadline: payload.error ? 
+                payload.error : 
+                'Check the Storybook process on the command line for more details.',

code/core/src/manager-api/modules/shortcuts.ts (2)

393-398: Add null safety for getCurrentStoryData().

The code assumes getCurrentStoryData() returns a valid object with an importPath property, but it might return null or undefined.

         case 'openInEditor': {
           if (global.CONFIG_TYPE === 'DEVELOPMENT') {
-            fullAPI.openInEditor({
-              file: fullAPI.getCurrentStoryData().importPath,
-            });
+            const storyData = fullAPI.getCurrentStoryData();
+            if (storyData?.importPath) {
+              fullAPI.openInEditor({
+                file: storyData.importPath,
+              });
+            }
           }
           break;
         }

---

409-410: Consider adding user feedback for successful copy.

The copy operation happens silently. Consider adding a notification to confirm the link was copied to clipboard.

         case 'copyStoryLink': {
-          copy(window.location.href);
+          const success = copy(window.location.href);
+          if (success && fullAPI.addNotification) {
+            fullAPI.addNotification({
+              id: 'copy-story-link',
+              content: { 
+                headline: 'Story link copied to clipboard' 
+              },
+              duration: 2000,
+            });
+          }
           break;
         }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

code/e2e-tests/manager.spec.ts (7)

8-8: Normalize STORYBOOK_TYPE and accept “development”

Make env check case‑insensitive and handle common alias.

-const isStorybookDev = (process.env.STORYBOOK_TYPE || 'dev') === 'dev';
+const isStorybookDev = ['dev', 'development'].includes(
+  (process.env.STORYBOOK_TYPE ?? 'dev').toLowerCase()
+);

---

20-35: Reduce flakiness when dismissing the tooltip

Clicking 'body' can hit overlays. Prefer a guaranteed page‑outside click.

-      await page.click('body');
+      await page.mouse.click(0, 0);

---

49-49: Prefer role+name over aria‑label CSS selector

More resilient to attribute drift and improves readability.

-      await sbPage.page.locator('[aria-label="Settings"]').click();
+      await sbPage.page.getByRole('button', { name: 'Settings' }).click();

---

136-136: Use role+name selector for Settings trigger

Keep selectors consistent and accessible.

-      await sbPage.page.locator('[aria-label="Settings"]').click();
+      await sbPage.page.getByRole('button', { name: 'Settings' }).click();
@@
-      await sbPage.page.locator('[aria-label="Settings"]').click();
+      await sbPage.page.getByRole('button', { name: 'Settings' }).click();

Also applies to: 139-139

---

175-175: Use role+name for Settings opener

Same rationale as above.

-        await sbPage.page.locator('[aria-label="Settings"]').click();
+        await sbPage.page.getByRole('button', { name: 'Settings' }).click();

---

225-225: Use role+name for Settings trigger

Consistency and robustness.

-      await sbPage.page.locator('[aria-label="Settings"]').click();
+      await sbPage.page.getByRole('button', { name: 'Settings' }).click();

---

255-255: Use role+name for Settings trigger

Same improvement here.

-      await sbPage.page.locator('[aria-label="Settings"]').click();
+      await sbPage.page.getByRole('button', { name: 'Settings' }).click();

code/core/src/manager-api/tests/stories.test.ts (1)

1539-1581: Optional: make assertions less snapshot‑fragile

To reduce churn, consider asserting only the critical bits (presence of importPath on 'a' and on its children) with toMatchObject, keeping the rest as-is.

Apply this focused assertion after the snapshot:

+      // Ensure editor integration metadata is present without over-specifying the structure
+      expect(filteredIndex!.a).toMatchObject({ type: 'component', importPath: './a.ts' });
+      expect(filteredIndex!['a--1']).toMatchObject({ type: 'story', importPath: './a.ts' });
+      expect(filteredIndex!['a--2']).toMatchObject({ type: 'story', importPath: './a.ts' });

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

code/e2e-tests/manager.spec.ts (4)

11-15: Grant clipboard permissions before navigation to deflake clipboard assertions

Without explicit permissions, navigator.clipboard.readText() is flaky across browsers. Grant permissions for the Storybook origin before page.goto().

Apply this diff:

   test.beforeEach(async ({ page }) => {
-    await page.goto(storybookUrl);
+    await page.context().grantPermissions(['clipboard-read', 'clipboard-write'], {
+      origin: storybookUrl,
+    });
+    await page.goto(storybookUrl);
 
     await new SbPage(page, expect).waitUntilLoaded();
   });

---

58-101: Use polling for clipboard; keep context‑menu checks as-is

Great dev-only gating via test.skip. Replace direct clipboard read with expect.poll for stability.

Apply this diff:

-      await expect(page.evaluate(() => navigator.clipboard.readText())).resolves.toContain(
-        'Primary'
-      );
+      await expect
+        .poll(() => page.evaluate(() => navigator.clipboard.readText()))
+        .toContain('Primary');

---

102-114: Dev share test: poll clipboard result

The dev gating is correct. Swap to polling to avoid timing flakes after clicking “Copy story link”.

Apply this diff:

-      await expect(page.evaluate(() => navigator.clipboard.readText())).resolves.toContain(
-        `${storybookUrl}/?path=/story/example-button--primary`
-      );
+      await expect
+        .poll(() => page.evaluate(() => navigator.clipboard.readText()))
+        .toContain(`${storybookUrl}/?path=/story/example-button--primary`);

---

116-128: Build share test: poll clipboard result

Same clipboard flake here; use polling.

Apply this diff:

-      await expect(page.evaluate(() => navigator.clipboard.readText())).resolves.toContain(
-        `${storybookUrl}/?path=/story/example-button--primary`
-      );
+      await expect
+        .poll(() => page.evaluate(() => navigator.clipboard.readText()))
+        .toContain(`${storybookUrl}/?path=/story/example-button--primary`);

code/e2e-tests/manager.spec.ts (1)

8-8: Name the env flag more explicitly

Consider renaming type to storybookType for clarity and to avoid confusion with the TS type keyword in declarations.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro