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
7 changes: 4 additions & 3 deletions web/src/routes/new-session.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
48 changes: 48 additions & 0 deletions web/src/sessions/open-new-session.test.tsx
Original file line number Diff line number Diff line change
@@ -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 (
<button onClick={() => openNewSession({ machineId: 'attic-pi', cwd: '/tmp', name: '', tags: [] })}>
open
</button>
)
}

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(<OpenButton />)

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(<OpenButton />)

await userEvent.click(screen.getByRole('button', { name: 'open' }))

expect(open).toHaveBeenCalledTimes(1)
expect(router.state.location.pathname).toBe('/sessions')
})
})
14 changes: 8 additions & 6 deletions web/src/sessions/open-new-session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@ export type NewSessionOrigin = Partial<NewSessionRequest>
* 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()
Expand All @@ -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],
)
Expand Down