add branch id to the url

AGENTS.md

@@ -113,7 +113,7 @@ make dev                          # backend containers + web dev server on :3000
 make check-env / sync-env         # report / reconcile *.local.env against templates
 make lint / check-lint            # fix / verify lint, format and types everywhere
 make test                         # unit tests + coverage gates everywhere
-make check-all                    # lint, test, e2e-dev, e2e-prod; run before a commit
+make check-all                    # check-lint, test, e2e-dev, e2e-prod; run before a commit
 make prod-build / prod-run[-no-email] / prod-stop  # all-in-one image on :8080 (mailpit :8025)
 make e2e-{dev,prod}[-stack-build|-stack-stop]  # dev stack :18080, prod image :19080 (no email :19081)
 ```

Makefile

@@ -127,12 +127,12 @@ test:
 	@$(QUIET) "testing datagen" sh -c 'cd datagen && make check-coverage'
 	@$(QUIET) "testing launcher" sh -c 'cd docker/prod/launcher && pnpm run test'
 
-# everything CI checks, to run before a commit. The steps must run one
-# after another, because lint rewrites the files the tests read. Listing
-# them as `check-all: lint test ...` would let `make -j` run them at once.
+# everything CI checks, to run before a commit. Nothing here modifies a
+# file. The steps are recipe lines so that `make -j` cannot start both e2e
+# stacks at once.
 .PHONY: check-all
 check-all:
-	@$(MAKE) lint
+	@$(MAKE) check-lint
 	@$(MAKE) test
 	@$(MAKE) e2e-dev
 	@$(MAKE) e2e-prod

e2e/helpers/auth.ts

@@ -92,7 +92,15 @@ export async function submitLoginForm(
 	origin = BASE_URL,
 ): Promise<void> {
 	await visit(page, `${origin}/login`)
+	await fillLoginForm(page, credentials)
+}
 
+// fillLoginForm signs in through the login page the browser is already
+// on, so whatever its url carries reaches the sign-in.
+export async function fillLoginForm(
+	page: Page,
+	credentials: Credentials,
+): Promise<void> {
 	const email = page.getByPlaceholder(
 		t("onboarding.login.email-password-form.email-placeholder"),
 	)

e2e/tests/login.test.ts

@@ -1,11 +1,12 @@
 import { expect, test } from "@playwright/test"
 import {
+	fillLoginForm,
 	newCredentials,
 	signUpAndVerify,
 	submitLoginForm,
 	submitSignupForm,
 } from "../helpers/auth"
-import { waitForEditor } from "../helpers/editor"
+import { contentEditor, createDocument, waitForEditor } from "../helpers/editor"
 import { t } from "../helpers/i18n"
 import { visit } from "../helpers/page"
 import { signUpWithWorkspace } from "../helpers/workspace"
@@ -126,4 +127,41 @@ test.describe("login", () => {
 
 		await context.close()
 	})
+
+	test("returns a signed-out visitor to the linked block after login", async ({
+		page,
+		request,
+		browser,
+	}) => {
+		// a second login and a second cold document load come on top of the
+		// usual signup-and-workspace setup
+		test.slow()
+
+		const { credentials } = await signUpWithWorkspace(page, request)
+		await createDocument(page)
+		// the linked block sits more than a screen down, so only a scroll to
+		// it brings it into view
+		await contentEditor(page).click()
+		for (let line = 0; line < 40; line += 1) {
+			await page.keyboard.press("Enter")
+		}
+		await page.keyboard.type("Linked block")
+		const block = contentEditor(page).getByText("Linked block", { exact: true })
+		await expect(block).toHaveAttribute("id", /.+/)
+		const uid = (await block.getAttribute("id")) ?? ""
+		const link = `${page.url()}#${uid}`
+
+		const context = await browser.newContext()
+		const visitor = await context.newPage()
+		await visit(visitor, link)
+		await expect(visitor).toHaveURL(/\/login/, { timeout: 15_000 })
+
+		await fillLoginForm(visitor, credentials)
+
+		await expect(visitor).toHaveURL(link, { timeout: 15_000 })
+		await waitForEditor(visitor)
+		await expect(visitor.locator(`[id="${uid}"]`)).toBeInViewport()
+
+		await context.close()
+	})
 })

e2e/tests/review.test.ts

@@ -2,10 +2,13 @@ import { expect, test } from "@playwright/test"
 import { joinAsSecondUser } from "../helpers/collaboration"
 import {
 	contentEditor,
+	contentPane,
 	createDocument,
 	documentPersisted,
 	editorText,
+	openBlockMenu,
 	readModeToggle,
+	sidebarDocument,
 	titleEditor,
 	waitForEditor,
 } from "../helpers/editor"
@@ -572,4 +575,71 @@ test.describe("review workflow", () => {
 
 		await other.context.close()
 	})
+
+	test("opens a copied block link on the draft, at that block", async ({
+		page,
+		request,
+	}) => {
+		await page.context().grantPermissions(["clipboard-read", "clipboard-write"])
+		await signUpWithWorkspace(page, request)
+		await createDocument(page)
+		await makeReviewable(page)
+		await switchToBranch(page, "draft")
+		// the link is built from the address, which names the draft only
+		// once the switch has reached it
+		await expect(page).toHaveURL(/New-Page-[a-z0-9]{20}\?branch=[a-z0-9]{20}$/)
+		// the linked block sits more than a screen down, so only a scroll to
+		// it brings it into view
+		await contentEditor(page).click()
+		for (let line = 0; line < 40; line += 1) {
+			await page.keyboard.press("Enter")
+		}
+		await page.keyboard.type("Only on the draft")
+		const block = contentEditor(page).getByText("Only on the draft", {
+			exact: true,
+		})
+		await openBlockMenu(page, contentPane(page), block)
+		await page
+			.getByRole("menuitem", {
+				name: t("editor.drag-handle.options.copy-link"),
+			})
+			.click()
+		await expect
+			.poll(() => page.evaluate(() => navigator.clipboard.readText()), {
+				message: "the block link reaches the clipboard",
+			})
+			.toContain("?branch=")
+		const link = await page.evaluate(() => navigator.clipboard.readText())
+		const uid = new URL(link).hash.slice(1)
+
+		const fresh = await page.context().newPage()
+		await visit(fresh, link)
+
+		await expect(fresh).toHaveURL(link)
+		await expect(branchSwitcher(fresh)).toContainText(branchLabel("draft"), {
+			timeout: 15_000,
+		})
+		await waitForEditor(fresh)
+		// the first page's caret label renders inside the paragraph, so the
+		// text is matched as a part
+		const linked = fresh.locator(`[id="${uid}"]`)
+		await expect(linked).toContainText("Only on the draft")
+		await expect(linked).toBeInViewport()
+	})
+
+	test("opens the main branch when the open page is picked in the sidebar", async ({
+		page,
+		request,
+	}) => {
+		await signUpWithWorkspace(page, request)
+		await createDocument(page)
+		await makeReviewable(page)
+		await switchToBranch(page, "draft")
+		await expect(page).toHaveURL(/New-Page-[a-z0-9]{20}\?branch=[a-z0-9]{20}$/)
+
+		await sidebarDocument(page, t("editor.new-document-name")).click()
+
+		await expect(branchSwitcher(page)).toContainText(branchLabel("main"))
+		await expect(page).toHaveURL(/New-Page-[a-z0-9]{20}$/)
+	})
 })

web/AGENTS.md

@@ -105,6 +105,10 @@ there when none exists.
   [app/middleware/01.redirect.global.ts](app/middleware/01.redirect.global.ts)
   handles auth gating, onboarding and the root redirect.
   `definePageMeta({ skipAuth: true })` marks signed-out pages.
+- **Branch links**: a document URL without `?branch=<branchId>` opens the
+  default branch. The page rewrites the address bar to name the active
+  branch, so a link built from the route already carries it; a link built
+  elsewhere adds the param itself.
 - **Page load**: the content container mounts its editors hidden as soon
   as the branch syncs and reports `initial-load-complete` only once every
   asynchronously rendered block has settled (`settledBlockRenders` in the

web/app/components/DocumentHeader.nuxt.test.ts

@@ -26,6 +26,9 @@ vi.mock("vue-sonner", () => ({
 const DOC_ID = "doc1".padEnd(20, "0")
 const MAIN_BRANCH = "main".padEnd(20, "0")
 const DRAFT_BRANCH = "draft".padEnd(20, "0")
+// a branch that is still being created has a temporary id, one character
+// longer than the id the server gives it
+const UNSAVED_DRAFT_BRANCH = "draft".padEnd(21, "0")
 // the divider between the document options and the assistant button is a
 // bare div, and the header draws no other one
 const ASSISTANT_SEPARATOR_SELECTOR = "div.w-px"
@@ -231,6 +234,21 @@ describe("<DocumentHeader>", { concurrent: false }, () => {
 		)
 	})
 
+	it("offers no branch picker while the draft branch is still being created", async ({
+		expect,
+	}) => {
+		seedBranches([
+			{ branchId: MAIN_BRANCH, branch: "main", default: true },
+			{ branchId: UNSAVED_DRAFT_BRANCH, branch: "draft", default: false },
+		])
+
+		const wrapper = await mountHeader()
+
+		expect(wrapper.text()).not.toContain(
+			t("editor.navbar.document-modes.main.title"),
+		)
+	})
+
 	it("names the draft version when the draft branch is active", async ({
 		expect,
 	}) => {

web/app/components/DocumentHeader.vue

@@ -32,8 +32,11 @@ const { updateDocumentBranch, createDocumentBranch, deleteDocumentBranch } =
 	useDocumentAPI()
 const isReviewable = computed(() => {
 	// NOTE this will be removed in the future
+	//
+	// a branch that is still being created has a temporary id and does not
+	// count. A switch to it would be undone once the server answers
 	const branches = fetchBranches.state.value.data
-	return !!branches && branches.length > 1
+	return !!branches && branches.filter((b) => isXid(b.branchId)).length > 1
 })
 const modeTooltip = computed(() => {
 	if (isBranchProtected.value) {

web/app/components/NotificationBox.nuxt.test.ts

@@ -548,7 +548,7 @@ describe("<NotificationBox>", { concurrent: false }, () => {
 					code: NotificationCode.DocumentReviewRequest,
 					metadata: { documentId: DOC_ID, branchId: "b" },
 				},
-				expected: `/acme/Runbook-${DOC_ID}`,
+				expected: `/acme/Runbook-${DOC_ID}?branch=b`,
 			},
 			{
 				name: "opens a hook trigger at the block that fired",
@@ -561,7 +561,7 @@ describe("<NotificationBox>", { concurrent: false }, () => {
 						type: DocumentHookType.URLWatcher,
 					},
 				},
-				expected: `/acme/Runbook-${DOC_ID}#block-7`,
+				expected: `/acme/Runbook-${DOC_ID}?branch=b#block-7`,
 			},
 			{
 				name: "opens a document-wide hook trigger at the document",
@@ -574,7 +574,7 @@ describe("<NotificationBox>", { concurrent: false }, () => {
 						type: DocumentHookType.URLWatcher,
 					},
 				},
-				expected: `/acme/Runbook-${DOC_ID}`,
+				expected: `/acme/Runbook-${DOC_ID}?branch=b`,
 			},
 			{
 				name: "opens a hook that needs attention at the block it watches",
@@ -601,7 +601,7 @@ describe("<NotificationBox>", { concurrent: false }, () => {
 						anchorBlockId: "block-3",
 					},
 				},
-				expected: `/acme/Runbook-${DOC_ID}#block-3`,
+				expected: `/acme/Runbook-${DOC_ID}?branch=b#block-3`,
 			},
 			{
 				name: "opens an unanchored comment at the document",
@@ -615,7 +615,7 @@ describe("<NotificationBox>", { concurrent: false }, () => {
 						anchorBlockId: null,
 					},
 				},
-				expected: `/acme/Runbook-${DOC_ID}`,
+				expected: `/acme/Runbook-${DOC_ID}?branch=b`,
 			},
 			{
 				name: "opens a comment reply at the block it is anchored to",
@@ -630,7 +630,7 @@ describe("<NotificationBox>", { concurrent: false }, () => {
 						anchorBlockId: "block-4",
 					},
 				},
-				expected: `/acme/Runbook-${DOC_ID}#block-4`,
+				expected: `/acme/Runbook-${DOC_ID}?branch=b#block-4`,
 			},
 			{
 				name: "opens an unanchored comment reply at the document",
@@ -645,7 +645,7 @@ describe("<NotificationBox>", { concurrent: false }, () => {
 						anchorBlockId: null,
 					},
 				},
-				expected: `/acme/Runbook-${DOC_ID}`,
+				expected: `/acme/Runbook-${DOC_ID}?branch=b`,
 			},
 		])("$name", async ({ notification, expected }, { expect }) => {
 			mockEndpoint("PUT", "/api/notifications/read-status", () => ({}))

web/app/components/NotificationBox.vue

@@ -81,7 +81,7 @@ async function buildNotificationHref(notification: Notification) {
 			const documentName = findDocumentName(metadata.documentId) || ""
 			const docSlug = createNameSlugWithId(documentName, metadata.documentId)
 
-			return `/${orgRealName}/${docSlug}`
+			return `/${orgRealName}/${docSlug}?branch=${encodeURIComponent(metadata.branchId)}`
 		}
 		case NotificationCode.DocumentHookTrigerred:
 		case NotificationCode.DocumentHookNeedsAttention: {
@@ -90,7 +90,7 @@ async function buildNotificationHref(notification: Notification) {
 
 			const documentName = findDocumentName(metadata.documentId) || ""
 			const docSlug = createNameSlugWithId(documentName, metadata.documentId)
-			const baseHref = `/${orgRealName}/${docSlug}`
+			const baseHref = `/${orgRealName}/${docSlug}?branch=${encodeURIComponent(metadata.branchId)}`
 
 			return metadata.blockId ? `${baseHref}#${metadata.blockId}` : baseHref
 		}
@@ -100,7 +100,7 @@ async function buildNotificationHref(notification: Notification) {
 
 			const documentName = findDocumentName(metadata.documentId) || ""
 			const docSlug = createNameSlugWithId(documentName, metadata.documentId)
-			const baseHref = `/${orgRealName}/${docSlug}`
+			const baseHref = `/${orgRealName}/${docSlug}?branch=${encodeURIComponent(metadata.branchId)}`
 
 			return metadata.anchorBlockId
 				? `${baseHref}#${metadata.anchorBlockId}`
@@ -112,7 +112,7 @@ async function buildNotificationHref(notification: Notification) {
 
 			const documentName = findDocumentName(metadata.documentId) || ""
 			const docSlug = createNameSlugWithId(documentName, metadata.documentId)
-			const baseHref = `/${orgRealName}/${docSlug}`
+			const baseHref = `/${orgRealName}/${docSlug}?branch=${encodeURIComponent(metadata.branchId)}`
 
 			return metadata.anchorBlockId
 				? `${baseHref}#${metadata.anchorBlockId}`

web/app/pages/[[organizationSlug]]/[[documentSlug]].vue

@@ -256,23 +256,33 @@ if (import.meta.dev) {
 	)
 }
 
-// a ?branch=<id> query opens that branch of the document. It keys on the
-// route rather than the store so a later switch through the header is not
-// undone; an id the document does not have is ignored and the watch below
-// falls back to the default branch
+// a document url opens the branch its ?branch=<id> names, or the default
+// branch when it names none the document has. This runs only when the
+// query changes or the branch list loads for the first time. A refetch
+// of the list is skipped. The header switches the branch before the url
+// is updated, so a refetch in between would read the old branch from
+// the url and switch back to it
 watchImmediate(
 	[() => pageRoute.query.branch, fetchBranches.state],
-	([queryBranch, branchState]) => {
+	([queryBranch, branchState], [oldQueryBranch, oldBranchState]) => {
 		const branches = branchState.data
 
-		if (typeof queryBranch !== "string" || !branches?.length) {
+		if (!branches?.length) {
 			return
 		}
 
-		const branch = branches.find((b) => b.branchId === queryBranch)
-		const defaultBranch = branches.find((b) => b.default)
+		if (queryBranch === oldQueryBranch && oldBranchState?.data?.length) {
+			return
+		}
 
+		const defaultBranch = branches.find((b) => b.default)
+		const branch =
+			branches.find((b) => b.branchId === queryBranch) ?? defaultBranch
+
+		// a query naming the default branch, or none the document has, is
+		// rewritten here. The watcher below only runs when the branch changes
 		if (!branch || branch.branchId === editorStore.activeBranchId) {
+			syncBranchQuery()
 			return
 		}
 
@@ -345,6 +355,9 @@ watchImmediate(
 	},
 )
 
+// the address bar names the active branch, so a copied url opens it
+watchImmediate(() => editorStore.activeBranchId, syncBranchQuery)
+
 if (import.meta.client) {
 	watchImmediate(
 		() => activeDocMetadata.value?.id,
@@ -416,6 +429,37 @@ function toggleNotificationSidebar() {
 	notificationSidebarOpen.value = !notificationSidebarOpen.value
 }
 
+// the query of this page's url with ?branch= naming the active branch.
+// The default branch is left unnamed. Every replace of the url must take
+// its query from here. A replace with the route's old query would cancel
+// one that syncBranchQuery started in the same tick
+function branchQuery() {
+	const activeBranch = fetchBranches.state.value.data?.find(
+		(b) => b.branchId === editorStore.activeBranchId,
+	)
+	if (!activeBranch) {
+		return pageRoute.query
+	}
+
+	return {
+		...pageRoute.query,
+		branch: activeBranch.default ? undefined : activeBranch.branchId,
+	}
+}
+
+function syncBranchQuery() {
+	const query = branchQuery()
+	if (query.branch === pageRoute.query.branch) {
+		return
+	}
+
+	void pageRouter.replace({
+		path: pageRoute.path,
+		query: query,
+		hash: pageRoute.hash,
+	})
+}
+
 function applyDocumentNameChange(name: string) {
 	if (!activeDocMetadata.value) {
 		return
@@ -441,7 +485,7 @@ function applyDocumentNameChange(name: string) {
 			name,
 			activeDocMetadata.value.id,
 		)}`,
-		query: pageRoute.query,
+		query: branchQuery(),
 		hash: pageRoute.hash,
 	})
 }
@@ -456,7 +500,7 @@ function refreshOrganizationRouteSlug() {
 			activeDocMetadata.value.name,
 			activeDocMetadata.value.id,
 		)}`,
-		query: pageRoute.query,
+		query: branchQuery(),
 		hash: pageRoute.hash,
 	})
 }

web/app/pages/login.vue

@@ -4,7 +4,7 @@ import { useForm } from "vee-validate"
 import * as z from "zod"
 // for some reason this isn't auto imported
 import { FormField as ShadcnUiFormField } from "@/components/shadcn/ui/form"
-import { postAuthDocumentUrl } from "#imports"
+import { nextUrlWithHash, postAuthDocumentUrl } from "#imports"
 import { showToastMessage } from "~/components/toast"
 import { cn } from "~/lib/utils"
 
@@ -97,6 +97,9 @@ const oauthContinueUrl = computed(() => {
 
 	return `${config.public.authRealtimeAPIBaseHttpURL}/api/auth/oauth2/authorize?${params.toString()}`
 })
+const nextUrl = computed(() =>
+	nextUrlWithHash(route.query.next as string | undefined, route.hash),
+)
 
 let redirectTimeout: ReturnType<typeof setTimeout> | undefined
 
@@ -123,13 +126,10 @@ async function logInWithProvider(provider: "github" | "google" | "slack") {
 		provider,
 		callbackURL:
 			oauthContinueUrl.value ??
-			postAuthDocumentUrl(
-				config.public.appBaseURL,
-				route.query.next as string | undefined,
-			),
+			postAuthDocumentUrl(config.public.appBaseURL, nextUrl.value),
 		newUserCallbackURL: postAuthDocumentUrl(
 			config.public.appBaseURL,
-			route.query.next as string | undefined,
+			nextUrl.value,
 			"/welcome",
 		),
 		fetchOptions: { query: route.query },
@@ -152,10 +152,9 @@ const onEmailPasswordSubmit = emailPasswordForm.handleSubmit(async (values) => {
 	// middleware forwards the signed-in visitor to next. Absolute against
 	// the app origin because a relative path would resolve against the auth
 	// server's origin, which does not serve the frontend in host-dev mode.
-	const nextUrl = route.query.next as string | undefined
 	const callbackQuery = new URLSearchParams({ verified: "true" })
-	if (nextUrl) {
-		callbackQuery.set("next", nextUrl)
+	if (nextUrl.value) {
+		callbackQuery.set("next", nextUrl.value)
 	}
 
 	const res = (await signInEmailPassword({
@@ -217,7 +216,7 @@ const onEmailPasswordSubmit = emailPasswordForm.handleSubmit(async (values) => {
 	// /login
 	await fetchAuthSession.refetch()
 
-	void navigateTo(nextUrl ? decodeURIComponent(nextUrl) : "/", {
+	void navigateTo(nextUrl.value ? decodeURIComponent(nextUrl.value) : "/", {
 		replace: true,
 	})
 })

web/app/utils/auth.test.ts

@@ -2,6 +2,7 @@ import { describe, it } from "vitest"
 import {
 	acceptInvitationUrl,
 	isInvitationRedirect,
+	nextUrlWithHash,
 	postAuthDocumentUrl,
 	postEmailVerificationUrl,
 } from "./auth"
@@ -43,6 +44,28 @@ describe("postAuthDocumentUrl", () => {
 	})
 })
 
+describe("nextUrlWithHash", () => {
+	it.for([
+		{
+			name: "appends the encoded hash to the next path",
+			input: { next: "%2Fdocs%2Fabc%3Fbranch%3Db", hash: "#block-1" },
+			expected: "%2Fdocs%2Fabc%3Fbranch%3Db%23block-1",
+		},
+		{
+			name: "leaves the next path alone without a hash",
+			input: { next: "%2Fdocs%2Fabc", hash: "" },
+			expected: "%2Fdocs%2Fabc",
+		},
+		{
+			name: "returns nothing without a next path",
+			input: { next: undefined, hash: "#block-1" },
+			expected: undefined,
+		},
+	])("$name", ({ input, expected }, { expect }) => {
+		expect(nextUrlWithHash(input.next, input.hash)).toBe(expected)
+	})
+})
+
 describe("acceptInvitationUrl", () => {
 	it("carries everything the accept page renders, encoded", ({ expect }) => {
 		expect(

web/app/utils/auth.ts

@@ -17,6 +17,16 @@ export function postAuthDocumentUrl(
 	return `${baseUrl || ""}${next ? decodeURIComponent(next) : defaultPath}`
 }
 
+// a server redirect to the login page cannot put the hash in next, because
+// the server never sees one. The browser keeps it on the login url instead,
+// so it is added back here
+export function nextUrlWithHash(
+	next: string | undefined,
+	hash: string,
+): string | undefined {
+	return next ? next + encodeURIComponent(hash) : undefined
+}
+
 // the accept page renders from these parameters before the invitee has a
 // session. auth-realtime builds the same link for the invitation email
 // (invitationLink in server/auth-realtime/src/auth.ts).