fix(desktop): open HTML preview in background tab and close modal (MUL-2418) (#2854)

Two independent root causes made "Open in new tab" on a desktop
attachment-preview modal feel like "the popup is still there and the
current tab got replaced":

1. `AttachmentPreviewModal.handleOpenInNewTab` never called `onClose()`,
   so the modal stayed mounted over the new tab.
2. Both `DesktopNavigationProvider.openInNewTab` and
   `TabNavigationProvider.openInNewTab` called
   `store.setActiveTab(tabId)` after `store.openTab(...)`, which stole
   focus to the new tab — violating the type contract
   ("Desktop only: open a path in a new background tab") and matching
   neither Chrome's cmd+click default nor the user's expectation.

Fixes:
- Modal: always call `onClose()` after dispatching the navigation
  (desktop adapter path and web `window.open` fallback path).
- Desktop navigation: drop the post-`openTab` `setActiveTab` call in both
  providers. `openTab` already preserves `activeTabId` for new paths and
  switches to the existing tab when the path is already open, which is
  exactly the background-tab semantics the type contract advertises.

Tests:
- `attachment-preview-modal.test.tsx`: assert `onClose` is invoked on
  both the desktop and web fallback branches.
- `pageview-tracker.test.tsx`: rename the "openInNewTab / addTab" case
  so the comment no longer claims `openInNewTab` activates the new tab.
- New `apps/desktop/.../platform/navigation.test.tsx`: assert that
  `openInNewTab` on both providers calls `openTab` and never
  `setActiveTab` for same-workspace paths, and routes cross-workspace
  paths through `switchWorkspace`.

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: multica-agent <github@multica.ai>

apps/desktop/src/renderer/src/components/pageview-tracker.test.tsx

@@ -116,7 +116,7 @@ describe("PageviewTracker", () => {
     expect(state.capturePageview).not.toHaveBeenCalled();
   });
 
-  it("fires pageview when a new tab is opened (openInNewTab / addTab)", () => {
+  it("fires pageview when a foreground tab is added (addTab path)", () => {
     state.byWorkspace = {
       acme: {
         activeTabId: "tA",
@@ -128,7 +128,10 @@ describe("PageviewTracker", () => {
     const { rerender } = render(<PageviewTracker />);
     state.capturePageview.mockClear();
 
-    // Simulate openInNewTab("/acme/agents") → new tab tC added and activated.
+    // Simulate a foreground new-tab action (e.g. cmd+k → "Open in new tab"
+    // that explicitly switches focus) — tC is appended AND becomes active.
+    // Note: `openInNewTab` itself opens tabs in the background and does NOT
+    // change `activeTabId`, so it does not trigger this path on its own.
     state.byWorkspace = {
       acme: {
         activeTabId: "tC",

apps/desktop/src/renderer/src/platform/navigation.test.tsx

@@ -0,0 +1,178 @@
+import { describe, expect, it, vi, beforeEach } from "vitest";
+import { render } from "@testing-library/react";
+import { useEffect } from "react";
+
+// Shared in-memory state that the mocked tab store reads / mutates. The test
+// records every method call so we can assert openInNewTab does NOT activate
+// the new tab (i.e. setActiveTab is never invoked on the same-workspace path).
+const state = vi.hoisted(() => ({
+  activeWorkspaceSlug: "acme" as string | null,
+  byWorkspace: {
+    acme: {
+      activeTabId: "tA",
+      tabs: [{ id: "tA", path: "/acme/issues" }],
+    },
+  } as Record<
+    string,
+    { activeTabId: string; tabs: { id: string; path: string }[] }
+  >,
+  openTab: vi.fn<(path: string, title?: string, icon?: string) => string>(),
+  setActiveTab: vi.fn<(tabId: string) => void>(),
+  switchWorkspace: vi.fn<(slug: string, openPath?: string) => void>(),
+}));
+
+vi.mock("@/stores/tab-store", () => {
+  const store = {
+    get activeWorkspaceSlug() {
+      return state.activeWorkspaceSlug;
+    },
+    get byWorkspace() {
+      return state.byWorkspace;
+    },
+    openTab: state.openTab,
+    setActiveTab: state.setActiveTab,
+    switchWorkspace: state.switchWorkspace,
+  };
+  const useTabStore = Object.assign(
+    (selector?: (s: typeof store) => unknown) =>
+      selector ? selector(store) : store,
+    { getState: () => store },
+  );
+  const getActiveTab = () => {
+    const slug = state.activeWorkspaceSlug;
+    if (!slug) return null;
+    const group = state.byWorkspace[slug];
+    if (!group) return null;
+    return group.tabs.find((t) => t.id === group.activeTabId) ?? null;
+  };
+  const useActiveTabIdentity = () => ({
+    slug: state.activeWorkspaceSlug,
+    tabId: state.activeWorkspaceSlug
+      ? (state.byWorkspace[state.activeWorkspaceSlug]?.activeTabId ?? null)
+      : null,
+  });
+  const useActiveTabRouter = () => null;
+  const resolveRouteIcon = () => "File";
+  return {
+    useTabStore,
+    getActiveTab,
+    useActiveTabIdentity,
+    useActiveTabRouter,
+    resolveRouteIcon,
+  };
+});
+
+vi.mock("@/stores/window-overlay-store", () => ({
+  useWindowOverlayStore: Object.assign(
+    () => null,
+    { getState: () => ({ overlay: null, open: vi.fn(), close: vi.fn() }) },
+  ),
+}));
+
+vi.mock("@multica/core/auth", () => ({
+  useAuthStore: Object.assign(
+    () => null,
+    { getState: () => ({ logout: vi.fn() }) },
+  ),
+}));
+
+vi.mock("@multica/core/paths", () => ({
+  isReservedSlug: (s: string) =>
+    ["login", "workspaces", "invite", "onboarding", "invitations"].includes(s),
+}));
+
+// DesktopNavigationProvider reads window.desktopAPI.runtimeConfig synchronously.
+beforeEach(() => {
+  state.openTab.mockReset();
+  state.setActiveTab.mockReset();
+  state.switchWorkspace.mockReset();
+  state.openTab.mockImplementation(() => "tNew");
+  state.activeWorkspaceSlug = "acme";
+  state.byWorkspace = {
+    acme: {
+      activeTabId: "tA",
+      tabs: [{ id: "tA", path: "/acme/issues" }],
+    },
+  };
+  Object.defineProperty(window, "desktopAPI", {
+    configurable: true,
+    value: {
+      runtimeConfig: { ok: true, config: { appUrl: "https://app.example" } },
+    },
+  });
+});
+
+import {
+  DesktopNavigationProvider,
+  TabNavigationProvider,
+} from "./navigation";
+import { useNavigation } from "@multica/views/navigation";
+
+function captureAdapter(onAdapter: (adapter: ReturnType<typeof useNavigation>) => void) {
+  function Probe() {
+    const nav = useNavigation();
+    useEffect(() => {
+      onAdapter(nav);
+    }, [nav]);
+    return null;
+  }
+  return Probe;
+}
+
+describe("DesktopNavigationProvider.openInNewTab", () => {
+  it("opens a background tab (no setActiveTab) for a same-workspace path", () => {
+    let adapter: ReturnType<typeof useNavigation> | null = null;
+    const Probe = captureAdapter((a) => {
+      adapter = a;
+    });
+    render(
+      <DesktopNavigationProvider>
+        <Probe />
+      </DesktopNavigationProvider>,
+    );
+    expect(adapter).not.toBeNull();
+    adapter!.openInNewTab!("/acme/agents", "Agents");
+    expect(state.openTab).toHaveBeenCalledWith("/acme/agents", "Agents", "File");
+    expect(state.setActiveTab).not.toHaveBeenCalled();
+    expect(state.switchWorkspace).not.toHaveBeenCalled();
+  });
+
+  it("delegates to switchWorkspace for a cross-workspace path", () => {
+    let adapter: ReturnType<typeof useNavigation> | null = null;
+    const Probe = captureAdapter((a) => {
+      adapter = a;
+    });
+    render(
+      <DesktopNavigationProvider>
+        <Probe />
+      </DesktopNavigationProvider>,
+    );
+    adapter!.openInNewTab!("/butter/inbox");
+    expect(state.switchWorkspace).toHaveBeenCalledWith("butter", "/butter/inbox");
+    expect(state.openTab).not.toHaveBeenCalled();
+    expect(state.setActiveTab).not.toHaveBeenCalled();
+  });
+});
+
+describe("TabNavigationProvider.openInNewTab", () => {
+  it("opens a background tab (no setActiveTab) for a same-workspace path", () => {
+    let adapter: ReturnType<typeof useNavigation> | null = null;
+    const Probe = captureAdapter((a) => {
+      adapter = a;
+    });
+    const fakeRouter = {
+      state: { location: { pathname: "/acme/issues", search: "" } },
+      subscribe: () => () => {},
+      navigate: vi.fn(),
+    } as unknown as Parameters<typeof TabNavigationProvider>[0]["router"];
+    render(
+      <TabNavigationProvider router={fakeRouter}>
+        <Probe />
+      </TabNavigationProvider>,
+    );
+    adapter!.openInNewTab!("/acme/agents", "Agents");
+    expect(state.openTab).toHaveBeenCalledWith("/acme/agents", "Agents", "File");
+    expect(state.setActiveTab).not.toHaveBeenCalled();
+    expect(state.switchWorkspace).not.toHaveBeenCalled();
+  });
+});

apps/desktop/src/renderer/src/platform/navigation.tsx

@@ -180,7 +180,9 @@ export function DesktopNavigationProvider({
       searchParams: new URLSearchParams(location.search),
       openInNewTab: (path: string, title?: string) => {
         // Cross-workspace "open in new tab" switches workspace and opens
-        // the path there; same-workspace just adds a tab in the current group.
+        // the path there; same-workspace just adds a background tab. The
+        // type contract (NavigationAdapter.openInNewTab) is explicitly a
+        // background-tab operation — focus stays on the current tab.
         const slug = extractWorkspaceSlug(path);
         const store = useTabStore.getState();
         if (slug && slug !== store.activeWorkspaceSlug) {
@@ -188,8 +190,7 @@ export function DesktopNavigationProvider({
           return;
         }
         const icon = resolveRouteIcon(path);
-        const tabId = store.openTab(path, title ?? path, icon);
-        if (tabId) store.setActiveTab(tabId);
+        store.openTab(path, title ?? path, icon);
       },
       getShareableUrl: (path: string) => `${appUrl}${path}`,
     }),
@@ -249,8 +250,7 @@ export function TabNavigationProvider({
           return;
         }
         const icon = resolveRouteIcon(path);
-        const tabId = store.openTab(path, title ?? path, icon);
-        if (tabId) store.setActiveTab(tabId);
+        store.openTab(path, title ?? path, icon);
       },
       getShareableUrl: (path: string) => `${appUrl}${path}`,
     }),

packages/views/editor/attachment-preview-modal.test.tsx

@@ -398,7 +398,7 @@ describe("AttachmentPreviewModal — open-in-new-tab (HTML only)", () => {
     expect(screen.getByTitle("Open in new tab")).toBeTruthy();
   });
 
-  it("invokes navigation.openInNewTab with the preview path when available (desktop)", async () => {
+  it("invokes navigation.openInNewTab with the preview path and closes the modal (desktop)", async () => {
     getAttachmentTextContentMock.mockResolvedValueOnce({
       text: "<p>hi</p>",
       originalContentType: "text/html",
@@ -407,11 +407,12 @@ describe("AttachmentPreviewModal — open-in-new-tab (HTML only)", () => {
       filename: "report.html",
       content_type: "text/html",
     });
+    const onClose = vi.fn();
     render(
       <AttachmentPreviewModal
         source={{ kind: "full", attachment: att }}
         open
-        onClose={() => {}}
+        onClose={onClose}
       />,
     );
     fireEvent.click(screen.getByTitle("Open in new tab"));
@@ -419,9 +420,10 @@ describe("AttachmentPreviewModal — open-in-new-tab (HTML only)", () => {
       "/acme/attachments/att-1/preview?name=report.html",
       "report.html",
     );
+    expect(onClose).toHaveBeenCalledTimes(1);
   });
 
-  it("falls back to window.open against the shareable URL on web", async () => {
+  it("falls back to window.open against the shareable URL and closes the modal (web)", async () => {
     navState.hasOpenInNewTab = false;
     getAttachmentTextContentMock.mockResolvedValueOnce({
       text: "<p>hi</p>",
@@ -434,11 +436,12 @@ describe("AttachmentPreviewModal — open-in-new-tab (HTML only)", () => {
       filename: "report.html",
       content_type: "text/html",
     });
+    const onClose = vi.fn();
     render(
       <AttachmentPreviewModal
         source={{ kind: "full", attachment: att }}
         open
-        onClose={() => {}}
+        onClose={onClose}
       />,
     );
     fireEvent.click(screen.getByTitle("Open in new tab"));
@@ -448,6 +451,7 @@ describe("AttachmentPreviewModal — open-in-new-tab (HTML only)", () => {
       "_blank",
       "noopener,noreferrer",
     );
+    expect(onClose).toHaveBeenCalledTimes(1);
   });
 
   it("does not render the new-tab button for non-HTML kinds", () => {

packages/views/editor/attachment-preview-modal.tsx

@@ -222,10 +222,11 @@ export function AttachmentPreviewModal({
     const path = `${paths.workspace(slug).attachmentPreview(state.attachmentId)}${nameQuery}`;
     if (navigation.openInNewTab) {
       navigation.openInNewTab(path, state.filename);
-      return;
+    } else {
+      const url = navigation.getShareableUrl(path);
+      window.open(url, "_blank", "noopener,noreferrer");
     }
-    const url = navigation.getShareableUrl(path);
-    window.open(url, "_blank", "noopener,noreferrer");
+    onClose();
   };
 
   if (!open || typeof document === "undefined") return null;