diff --git a/web/src/routes/new-session.test.tsx b/web/src/routes/new-session.test.tsx index 6393217..8875895 100644 --- a/web/src/routes/new-session.test.tsx +++ b/web/src/routes/new-session.test.tsx @@ -28,9 +28,10 @@ import { NewSessionRoute } from './new-session' * moment it mounts and the spawn cannot go out until the socket is up. * * `from` is the other arrival: a tab that is already connected and navigates - * here, which is what a blocked popup falls back to. The page then mounts into - * an open socket and spawns from its effect body rather than from a status - * change — a different code path, and the one the double-mount guard is for. + * here, as a pasted address or a history-restored one does. The page then + * mounts into an open socket and spawns from its effect body rather than from + * a status change — a different code path, and the one the double-mount guard + * is for. */ async function mountNew( url: string, diff --git a/web/src/sessions/open-new-session.test.tsx b/web/src/sessions/open-new-session.test.tsx new file mode 100644 index 0000000..7a2de71 --- /dev/null +++ b/web/src/sessions/open-new-session.test.tsx @@ -0,0 +1,48 @@ +import { screen } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { useOpenNewSession } from '@/sessions/open-new-session' +import { renderWithRouter } from '@/testing/render' + +function OpenButton() { + const openNewSession = useOpenNewSession() + return ( + + ) +} + +describe('useOpenNewSession', () => { + afterEach(() => { + vi.restoreAllMocks() + }) + + it('opens the new-session page in a tab, with the request in the query', async () => { + const open = vi.spyOn(window, 'open').mockReturnValue(null) + await renderWithRouter() + + await userEvent.click(screen.getByRole('button', { name: 'open' })) + + expect(open).toHaveBeenCalledTimes(1) + const [href, target, features] = open.mock.calls[0] ?? [] + expect(String(href)).toMatch(/^\/new\?/) + expect(String(href)).toContain('d=attic-pi') + expect(target).toBe('_blank') + expect(features).toBe('noopener') + }) + + it('does not also navigate this tab: noopener makes window.open return null on every call, not only on a blocked popup', async () => { + // The regression this pins: a popup-blocked fallback keyed on the return + // value fires unconditionally under noopener, so one click spawned two + // sessions — one in the new tab, one here (#69). + const open = vi.spyOn(window, 'open').mockReturnValue(null) + const { router } = await renderWithRouter() + + await userEvent.click(screen.getByRole('button', { name: 'open' })) + + expect(open).toHaveBeenCalledTimes(1) + expect(router.state.location.pathname).toBe('/sessions') + }) +}) diff --git a/web/src/sessions/open-new-session.ts b/web/src/sessions/open-new-session.ts index b18daa5..3f876af 100644 --- a/web/src/sessions/open-new-session.ts +++ b/web/src/sessions/open-new-session.ts @@ -27,10 +27,13 @@ export type NewSessionOrigin = Partial * Called straight out of a click or a form submission, never out of a reply, * which is the whole reason the page exists — see the note on NewSessionRoute. * - * A blocked popup falls back to this tab rather than to nothing at all. It is - * not what was asked for, but a button that visibly does nothing is worse than - * a button that does the old thing, and the reader can still get back with the - * browser's own back button. + * The tab is the only path: there is deliberately no popup-blocked fallback. + * With `noopener`, window.open returns null on every call — that is specified + * behaviour, the option severs the handle the caller would otherwise get — so + * the return value cannot distinguish a block from success. A fallback keyed + * on it fired unconditionally and spawned two sessions per click, one in the + * new tab and one here. A genuinely blocked popup is rare, and the browser's + * own blocked-popup UI is the recovery. */ export function useOpenNewSession(): (want: NewSessionRequest) => void { const router = useRouter() @@ -40,8 +43,7 @@ export function useOpenNewSession(): (want: NewSessionRequest) => void { to: NEW_SESSION_PATH, search: newSessionSearch(want), }).href - const tab = window.open(href, '_blank', 'noopener') - if (tab === null) void router.navigate({ to: href }) + window.open(href, '_blank', 'noopener') }, [router], )