From c9b2a47d02a49f91d4e261e6813408f043331182 Mon Sep 17 00:00:00 2001 From: swithek <52840391+swithek@users.noreply.github.com> Date: Sun, 4 Oct 2026 19:13:44 +0200 Subject: [PATCH] add branch id to the url --- AGENTS.md | 2 +- Makefile | 8 +-- e2e/helpers/auth.ts | 8 +++ e2e/tests/login.test.ts | 40 ++++++++++- e2e/tests/review.test.ts | 70 +++++++++++++++++++ web/AGENTS.md | 4 ++ .../components/DocumentHeader.nuxt.test.ts | 18 +++++ web/app/components/DocumentHeader.vue | 5 +- .../components/NotificationBox.nuxt.test.ts | 14 ++-- web/app/components/NotificationBox.vue | 8 +-- .../[[organizationSlug]]/[[documentSlug]].vue | 64 ++++++++++++++--- web/app/pages/login.vue | 19 +++-- web/app/utils/auth.test.ts | 23 ++++++ web/app/utils/auth.ts | 10 +++ 14 files changed, 255 insertions(+), 38 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 09b84217..773fdcfd 100644 --- a/AGENTS.md +++ b/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) ``` diff --git a/Makefile b/Makefile index 54e2c55c..06b25ba6 100644 --- a/Makefile +++ b/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 diff --git a/e2e/helpers/auth.ts b/e2e/helpers/auth.ts index 75ccc0cb..646a510c 100644 --- a/e2e/helpers/auth.ts +++ b/e2e/helpers/auth.ts @@ -92,7 +92,15 @@ export async function submitLoginForm( origin = BASE_URL, ): Promise { 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 { const email = page.getByPlaceholder( t("onboarding.login.email-password-form.email-placeholder"), ) diff --git a/e2e/tests/login.test.ts b/e2e/tests/login.test.ts index 05e8b365..3af48853 100644 --- a/e2e/tests/login.test.ts +++ b/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() + }) }) diff --git a/e2e/tests/review.test.ts b/e2e/tests/review.test.ts index e35a91f9..e36a2ee8 100644 --- a/e2e/tests/review.test.ts +++ b/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}$/) + }) }) diff --git a/web/AGENTS.md b/web/AGENTS.md index 6d1a378d..f3fc82cb 100644 --- a/web/AGENTS.md +++ b/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=` 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 diff --git a/web/app/components/DocumentHeader.nuxt.test.ts b/web/app/components/DocumentHeader.nuxt.test.ts index d4b778da..597f1d94 100644 --- a/web/app/components/DocumentHeader.nuxt.test.ts +++ b/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("", { 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, }) => { diff --git a/web/app/components/DocumentHeader.vue b/web/app/components/DocumentHeader.vue index fb316c9b..cb7f578e 100644 --- a/web/app/components/DocumentHeader.vue +++ b/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) { diff --git a/web/app/components/NotificationBox.nuxt.test.ts b/web/app/components/NotificationBox.nuxt.test.ts index 086be48b..f9247a2e 100644 --- a/web/app/components/NotificationBox.nuxt.test.ts +++ b/web/app/components/NotificationBox.nuxt.test.ts @@ -548,7 +548,7 @@ describe("", { 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("", { 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("", { 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("", { 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("", { 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("", { 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("", { 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", () => ({})) diff --git a/web/app/components/NotificationBox.vue b/web/app/components/NotificationBox.vue index 5336b845..9ee642e7 100644 --- a/web/app/components/NotificationBox.vue +++ b/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}` diff --git a/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue b/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue index 18a7f1fa..647c0aea 100644 --- a/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue +++ b/web/app/pages/[[organizationSlug]]/[[documentSlug]].vue @@ -256,23 +256,33 @@ if (import.meta.dev) { ) } -// a ?branch= 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= 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, }) } diff --git a/web/app/pages/login.vue b/web/app/pages/login.vue index 78e42334..fb1a1900 100644 --- a/web/app/pages/login.vue +++ b/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 | 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, }) }) diff --git a/web/app/utils/auth.test.ts b/web/app/utils/auth.test.ts index 5695f65b..63788b4f 100644 --- a/web/app/utils/auth.test.ts +++ b/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( diff --git a/web/app/utils/auth.ts b/web/app/utils/auth.ts index 88a1c06f..ddb6d0d0 100644 --- a/web/app/utils/auth.ts +++ b/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).