diff --git a/apps/server/src/navigation/routes.test.ts b/apps/server/src/navigation/routes.test.ts index edbf07874..9ff60dbec 100644 --- a/apps/server/src/navigation/routes.test.ts +++ b/apps/server/src/navigation/routes.test.ts @@ -272,6 +272,37 @@ describe("navigation routes", () => { expect(await repeated!.json()).toEqual(await added!.json()); }); + it("removes a project once, only from the same origin, leaving it to be added again", async () => { + let { router, github, cookie } = await setup(); + github.repositories.set("octo-org/score", repository("R_score", "octo-org", "score")); + let headers = { "content-type": "application/json", origin: "https://chopin.test" }; + let add = () => + router.handle(request("/api/navigation/projects", cookie, { + method: "POST", + headers, + body: JSON.stringify({ owner: "octo-org", repository: "score" }), + })); + let remove = (origin = "https://chopin.test", id = "R_score") => + router.handle(request(`/api/navigation/projects/${id}`, cookie, { + method: "DELETE", + headers: { origin }, + })); + expect((await add())!.status).toBe(201); + expect((await remove("https://elsewhere.test"))!.status).toBe(403); + expect( + (await router.handle(request(`/api/navigation/projects/R_score`, undefined, { + method: "DELETE", + headers: { origin: "https://chopin.test" }, + })))!.status, + ).toBe(401); + expect((await remove())!.status).toBe(204); + expect((await remove())!.status).toBe(404); + expect((await remove(undefined, "not%20an%20id"))!.status).toBe(404); + let listed = await (await router.handle(request("/api/navigation", cookie)))!.json(); + expect(listed.projects).toEqual([]); + expect((await add())!.status).toBe(201); + }); + it("rejects cross-origin navigation mutations", async () => { let { router, cookie } = await setup(); let headers = { "content-type": "application/json", origin: "https://elsewhere.test" }; diff --git a/apps/server/src/navigation/routes.ts b/apps/server/src/navigation/routes.ts index c20a106fd..2ecaf9de4 100644 --- a/apps/server/src/navigation/routes.ts +++ b/apps/server/src/navigation/routes.ts @@ -218,6 +218,29 @@ export function registerNavigationRoutes( } }); + /** Take a repository out of the person's projects. Their documents and access are unchanged. */ + router.on("DELETE", "/api/navigation/projects/:repositoryId", async (request, _url, params) => { + if (!ownOrigin(auth.config, request.headers.get("origin"))) { + return json({ error: "origin is not allowed" }, 403); + } + try { + let session = await auth.sessions.authenticate(request); + if (!session) return json({ error: "authentication required" }, 401); + let repositoryId = params.repositoryId ?? ""; + if (!/^[A-Za-z0-9_=-]{1,128}$/.test(repositoryId)) { + return json({ error: "project not found" }, 404); + } + let removed = await storage.navigation.removeProject( + session.user.id, + repositoryId, + auth.clock(), + ); + return removed ? empty(204) : json({ error: "project not found" }, 404); + } catch (err) { + return failure(err); + } + }); + router.on("PATCH", "/api/navigation", async request => { if (!ownOrigin(auth.config, request.headers.get("origin"))) { return json({ error: "origin is not allowed" }, 403); diff --git a/apps/server/src/storage/contract.ts b/apps/server/src/storage/contract.ts index 21081ce98..20ff83a50 100644 --- a/apps/server/src/storage/contract.ts +++ b/apps/server/src/storage/contract.ts @@ -236,6 +236,53 @@ export function storageContract(name: string, factory: Factory): void { } }); + it("removes a project, forgetting the last document only when it is in that repository", async () => { + let storage = await opened(factory); + try { + let now = new Date("2026-01-02T03:04:05.000Z"); + let { userId, channelId, repositoryId } = await userAndChannel(storage); + let channel = (await storage.channels.get(channelId))!; + for (let [id, name] of [[repositoryId, channel.repositoryName], ["R_other", "other"]]) { + await storage.navigation.addProject({ + userId, + repositoryId: id!, + repositoryOwner: channel.repositoryOwner, + repositoryName: name!, + now, + }); + } + let visited = await storage.navigation.setLastDocument(userId, channelId, now); + + // Another repository: the last document stays. + expect(await storage.navigation.removeProject(userId, "R_other", now)).toBe(true); + expect(await storage.navigation.get(userId)).toEqual(visited); + expect(await storage.navigation.removeProject(userId, "R_other", now)).toBe(false); + + // The last document's own repository: it is forgotten, with a new revision. + let later = new Date(now.getTime() + 1); + expect(await storage.navigation.removeProject(userId, repositoryId, later)).toBe(true); + expect(await storage.navigation.projects(userId)).toEqual([]); + expect(await storage.navigation.get(userId)).toMatchObject({ + lastDocumentId: undefined, + revision: visited.revision + 1, + }); + + // Visiting one of its documents adds it back. + await storage.navigation.recordVisit({ + userId, + repositoryId, + repositoryOwner: channel.repositoryOwner, + repositoryName: channel.repositoryName, + documentId: channelId, + now: later, + }); + expect((await storage.navigation.projects(userId)).map(value => value.repositoryId)) + .toEqual([repositoryId]); + } finally { + await storage.close(); + } + }); + it("stores the last document with a monotonic navigation revision", async () => { let storage = await opened(factory); try { diff --git a/apps/server/src/storage/memory/adapter.ts b/apps/server/src/storage/memory/adapter.ts index 235c00e4d..7e50596ab 100644 --- a/apps/server/src/storage/memory/adapter.ts +++ b/apps/server/src/storage/memory/adapter.ts @@ -285,6 +285,8 @@ export class MemoryStorage implements StorageAdapter { return Promise.resolve(undefined); }, addProject: input => this.#addProject(input), + removeProject: (userId, repositoryId, now) => + Promise.resolve(this.#removeProject(userId, repositoryId, now)), get: userId => { this.#requireUser(userId); let found = this.#navigation.get(userId); @@ -445,6 +447,25 @@ export class MemoryStorage implements StorageAdapter { return Promise.resolve({ project: project(saved), added: true }); } + #removeProject(userId: string, repositoryId: string, now: Date): boolean { + this.#requireUser(userId); + let projects = this.#projects.get(userId) ?? []; + let kept = projects.filter(value => value.repositoryId !== repositoryId); + if (kept.length === projects.length) return false; + this.#projects.set(userId, kept); + let saved = this.#navigation.get(userId); + let last = saved?.lastDocumentId ? this.#channels.get(saved.lastDocumentId) : undefined; + if (saved && last?.repositoryId === repositoryId) { + this.#navigation.set(userId, { + ...saved, + lastDocumentId: undefined, + revision: saved.revision + 1, + updatedAt: now, + }); + } + return true; + } + async #setLastDocument( userId: string, documentId: string | undefined, diff --git a/apps/server/src/storage/port.ts b/apps/server/src/storage/port.ts index 603b73be7..0a24c0643 100644 --- a/apps/server/src/storage/port.ts +++ b/apps/server/src/storage/port.ts @@ -216,6 +216,12 @@ export interface NavigationStore { projects(userId: string): Promise; firstDocument(userId: string, repositoryIds: string[]): Promise; addProject(input: AddUserProject): Promise; + /** + * Takes a repository out of a person's projects; false when it wasn't one. Their last + * document is forgotten when it is in that repository. Visiting one of its documents adds + * the project again. + */ + removeProject(userId: string, repositoryId: string, now: Date): Promise; get(userId: string): Promise; setLastDocument( userId: string, diff --git a/apps/server/src/storage/postgres/navigation.ts b/apps/server/src/storage/postgres/navigation.ts index eca023f24..647a32fb9 100644 --- a/apps/server/src/storage/postgres/navigation.ts +++ b/apps/server/src/storage/postgres/navigation.ts @@ -210,6 +210,29 @@ export class PostgresNavigationStore implements NavigationStore { () => this.#sql.begin(transaction => this.#addProject(transaction, input)), ); + readonly removeProject = (userId: string, repositoryId: string, now: Date): Promise => + this.#run("remove project", () => + this.#sql.begin(async transaction => { + await this.#lockUser(transaction, userId); + let removed = await transaction<{ repositoryId: string }[]>` + DELETE FROM user_projects + WHERE user_id = ${userId} AND repository_id = ${repositoryId} + RETURNING repository_id AS "repositoryId" + `; + if (!removed.length) return false; + await transaction` + UPDATE user_navigation SET + last_document_id = NULL, + revision = revision + 1, + updated_at = ${now} + WHERE user_id = ${userId} + AND last_document_id IN ( + SELECT id FROM channels WHERE repository_id = ${repositoryId} + ) + `; + return true; + })); + readonly get = (userId: string): Promise => this.#run("read navigation", async () => { await this.#requireUser(userId); diff --git a/apps/web/src/add-project-dialog.tsx b/apps/web/src/add-project-dialog.tsx index 4eb569bf4..a9f301224 100644 --- a/apps/web/src/add-project-dialog.tsx +++ b/apps/web/src/add-project-dialog.tsx @@ -21,18 +21,41 @@ function message(error: unknown): string { return error instanceof Error ? error.message : "Could not load repositories."; } +/** Take a repository out of the signed-in person's projects; its documents stay as they are. */ +async function removeProject(repositoryId: string): Promise { + let result = await fetch(`/api/navigation/projects/${encodeURIComponent(repositoryId)}`, { + method: "DELETE", + }); + if (result.status === 204) return; + let failure = await result.json().catch(() => undefined) as { error?: string } | undefined; + throw new Api.ApiError(failure?.error ?? `request failed (${result.status})`, result.status); +} + +/** A change to one repository that is under way. */ +type Pending = "adding" | "removing"; + +/** + * Choose which repositories are projects, as GitHub's multi-select repository picker does: each + * row is a checkbox and the repository's name, projects listed first as it opens, and choosing a + * row adds or removes it at once. It stays open for several changes and closes with Escape or a + * click outside. With no projects yet, the first one added closes it, so a first run goes + * straight to its documents. + */ export function AddProjectDialog( { added, motion, onAdded, onDismiss, + onRemoved, userId, }: { added: Api.NavigationProject[]; motion: NavigationDialogMotion; - onAdded: (project: Api.NavigationProject) => void; + /** A project was added; `closing` when the dialog closes with it, so focus can follow it. */ + onAdded: (project: Api.NavigationProject, closing: boolean) => void; onDismiss: () => void; + onRemoved: (repositoryId: string) => void; userId: string; }, ) { @@ -43,7 +66,12 @@ export function AddProjectDialog( let [error, setError] = useState(); let [query, setQuery] = useState(""); let [retry, setRetry] = useState(0); - let [adding, setAdding] = useState(); + let [pending, setPending] = useState>(new Map()); + /** Choices made here, shown at once and kept until the projects list agrees. */ + let [chosen, setChosen] = useState>(new Map()); + let firstRun = useRef(added.length === 0); + /** Projects as the dialog opened, listed first; a choice made here never reorders the list. */ + let openedWith = useRef(new Set(added.map(project => project.repositoryId))); useEffect(() => { let active = true; @@ -71,23 +99,59 @@ export function AddProjectDialog( ? installedRepositoryGroups(snapshot).flatMap(group => group.repositories) : []; let addedIds = new Set(added.map(project => project.repositoryId)); + let isProject = (id: string) => chosen.get(id) ?? addedIds.has(id); let visible = repositories.filter(repository => !normalized || `${repository.owner}/${repository.name}`.toLocaleLowerCase().includes(normalized) - ); + ).sort((a, b) => Number(openedWith.current.has(b.id)) - Number(openedWith.current.has(a.id))); - async function select(repository: Api.Repository) { - if (adding || addedIds.has(repository.id)) return; - setAdding(repository.id); + function settle(id: string, value?: boolean) { + setPending(current => { + let next = new Map(current); + next.delete(id); + return next; + }); + setChosen(current => { + let next = new Map(current); + if (value === undefined) next.delete(id); + else next.set(id, value); + return next; + }); + } + + async function toggle(repository: Api.Repository) { + if (pending.has(repository.id)) return; + let adding = !isProject(repository.id); + setPending(current => new Map(current).set(repository.id, adding ? "adding" : "removing")); + setChosen(current => new Map(current).set(repository.id, adding)); setError(undefined); try { - onAdded(await Api.addProject(repository.owner, repository.name)); - onDismiss(); + if (adding) { + onAdded(await Api.addProject(repository.owner, repository.name), firstRun.current); + } else { + await removeProject(repository.id); + onRemoved(repository.id); + } + settle(repository.id, adding); + if (adding && firstRun.current) onDismiss(); } catch (reason) { setError(reason); - setAdding(undefined); + settle(repository.id); } } + // Once the projects list agrees with a choice made here, the list is the truth again. + useEffect(() => { + setChosen(current => { + let stale = [...current].filter(([id, value]) => + addedIds.has(id) === value && !pending.has(id) + ); + if (stale.length === 0) return current; + let next = new Map(current); + for (let [id] of stale) next.delete(id); + return next; + }); + }, [added]); + return ( 0 && ( void select(repository)} + isProject={isProject} + onChoose={repository => void toggle(repository)} + pending={pending} query={query} repositories={visible} /> @@ -164,53 +228,44 @@ export function AddProjectDialog( ); } -/** The repositories a person can add, with ones already added shown but not offered. */ +/** Each repository a person can add, checked when it is one of their projects. */ export function RepositoryOptions( - { addedIds, adding, input, onChoose, query, repositories }: { - addedIds: ReadonlySet; - adding?: string; + { input, isProject, onChoose, pending, query, repositories }: { input: RefObject; + isProject: (repositoryId: string) => boolean; onChoose: (repository: Api.Repository) => void; + pending: ReadonlyMap; query: string; repositories: Api.Repository[]; }, ) { return ( !addedIds.has(repository.id)} + checked={repository => isProject(repository.id)} input={input} itemKey={repository => repository.id} - itemLabel={repository => - `${repository.owner}/${repository.name}${addedIds.has(repository.id) ? ", added" : ""}`} + itemLabel={repository => `${repository.owner}/${repository.name}`} items={repositories} label="Repositories" onChoose={onChoose} query={query} renderItem={repository => { - let alreadyAdded = addedIds.has(repository.id); - let busy = adding === repository.id; + let member = isProject(repository.id); + let busy = pending.get(repository.id); return ( <> + + {repository.owner}/{repository.name} - {alreadyAdded - ? ( - - - ) - : ( - - {busy ? "Adding…" : "Add"} - - )} ); }} diff --git a/apps/web/src/design-audit/controls.tsx b/apps/web/src/design-audit/controls.tsx index ac4ac9853..f915beea2 100644 --- a/apps/web/src/design-audit/controls.tsx +++ b/apps/web/src/design-audit/controls.tsx @@ -81,16 +81,18 @@ const AUDIT_REPOSITORIES: Api.Repository[] = ["chopin", "design-notes", "editor- url: `https://github.com/githubnext/${name}`, })); -/** The live palette listbox as Add project shows it: one repository already added and not offered. */ +/** The live palette listbox as Add project shows it: projects checked, one being added. */ function SelectionList() { let input = useRef(null); + let projects = new Set(["R_design-notes"]); return (
projects.has(id)} onChoose={() => {}} + pending={new Map([[AUDIT_REPOSITORIES.at(-1)!.id, "adding" as const]])} query="githubnext" repositories={AUDIT_REPOSITORIES} /> diff --git a/apps/web/src/design-audit/specimens.test.tsx b/apps/web/src/design-audit/specimens.test.tsx index 440268a5c..96c3e0645 100644 --- a/apps/web/src/design-audit/specimens.test.tsx +++ b/apps/web/src/design-audit/specimens.test.tsx @@ -205,7 +205,10 @@ describe("design audit specimens", () => { expect(markup).toContain('aria-label="Add document"'); expect(markup).toContain("disabled"); expect(plate(markup, "selections")).toContain( - 'aria-disabled="true" aria-label="githubnext/design-notes, added"', + 'aria-checked="true" aria-label="githubnext/design-notes"', + ); + expect(plate(markup, "selections")).toContain( + 'aria-checked="false" aria-label="githubnext/chopin"', ); expect(plate(markup, "selections")).toContain('class="navigation-palette-option"'); expect(markup).not.toContain("design-audit-selection-list"); diff --git a/apps/web/src/navigation-palette.tsx b/apps/web/src/navigation-palette.tsx index a4e120d3b..33d9cefb5 100644 --- a/apps/web/src/navigation-palette.tsx +++ b/apps/web/src/navigation-palette.tsx @@ -9,6 +9,7 @@ import type { KeyboardEvent, ReactNode, RefObject } from "react"; */ export function PaletteListbox( { + checked, enabled = () => true, input, itemKey, @@ -19,6 +20,8 @@ export function PaletteListbox( renderItem, itemLabel, }: { + /** Rows that toggle membership say whether each is in; the highlight stays `aria-selected`. */ + checked?: (item: T) => boolean; enabled?: (item: T) => boolean; input: RefObject; itemKey: (item: T) => string; @@ -108,6 +111,7 @@ export function PaletteListbox( let on = enabled(item); return (
{ + onAdded={(project, closing) => { catalogueRefreshes.current.set(project.repositoryId, Date.now()); - setFocusProjectId(project.repositoryId); + // Focus moves to the new project only as the dialog closes; open, it keeps focus. + if (closing) { + setFocusProjectId(project.repositoryId); + } setCollapsed(false); if (mode === "drawer") { setDrawerOpen(true); @@ -1117,6 +1120,10 @@ export function NavigationShell( void refresh(); }} onDismiss={dismissDialog} + onRemoved={repositoryId => { + catalogueRefreshes.current.delete(repositoryId); + void refresh(); + }} userId={user.id} /> diff --git a/apps/web/src/navigation.css b/apps/web/src/navigation.css index 3e95288ac..c6557ea21 100644 --- a/apps/web/src/navigation.css +++ b/apps/web/src/navigation.css @@ -1281,6 +1281,38 @@ visibility: visible; } +/* A project row's checkbox: the choice-control checkbox, drawn inside its option. */ +.navigation-palette-check { + display: grid; + width: calc(var(--spacing) * 4); + height: calc(var(--spacing) * 4); + flex-shrink: 0; + place-items: center; + border: var(--edge-width) solid var(--color-control-edge); + border-radius: var(--radius-sm); + background: var(--color-page); + color: var(--color-text-tertiary); + transition: + background-color var(--duration-base) var(--ease-out), + border-color var(--duration-base) var(--ease-out); +} + +.navigation-palette-check[data-checked] { + border-color: var(--color-brand); + background: var(--color-brand); + color: var(--color-page); +} + +/* The check and the loader take the box's ink, as icons in a primary button do. */ +.navigation-palette-check [data-nucleo-icon] { + color: inherit; +} + +/* At 12px the line icons' stroke is a single pixel; the check carries the weight choice-control's does. */ +.navigation-palette-check[data-checked] path { + stroke-width: 2.5; +} + /* Touch has no hover to disclose the action, so every addable row shows it. */ @media (pointer: coarse) { .navigation-palette-option { @@ -1304,7 +1336,8 @@ } @media (prefers-reduced-motion: no-preference) { - .navigation-palette-status [data-palette-loader] { + .navigation-palette-status [data-palette-loader], + .navigation-palette-check [data-palette-loader] { animation: button-loader-spin var(--duration-linger) linear infinite; } } diff --git a/e2e/hosted.e2e.ts b/e2e/hosted.e2e.ts index 3a8076d36..624b9b9c1 100644 --- a/e2e/hosted.e2e.ts +++ b/e2e/hosted.e2e.ts @@ -644,3 +644,54 @@ test("expired GitHub authorization returns to sign in without losing the documen await expect(page.getByRole("link", { name: "Continue with GitHub" })).toBeVisible(); await expect(page).toHaveURL(path); }); + +test("the Add project dialog adds and removes projects as checkboxes, projects first, until it is closed", async ({ baseURL, page }) => { + await authenticate(page, `project-toggler-${crypto.randomUUID()}`, baseURL!); + let added = await page.request.post("/api/navigation/projects", { + data: { owner: "octo-org", repository: "score" }, + headers: { origin: baseURL! }, + }); + expect(added.status()).toBe(201); + await page.goto("/"); + let projects = page.getByRole("complementary", { name: "Projects", exact: true }); + let project = (name: string) => + projects.getByRole("button", { + name: new RegExp(`^${name}(?:, \\d+ unanswered decisions?)?$`), + }); + await expect(project("score")).toBeVisible(); + await projects.getByRole("button", { exact: true, name: "Add project" }).click(); + + let dialog = addProjectDialog(page); + let score = dialog.getByRole("option", { name: "octo-org/score", exact: true }); + let notes = dialog.getByRole("option", { name: "octocat/notes", exact: true }); + await expect(score).toHaveAttribute("aria-checked", "true"); + await expect(notes).toHaveAttribute("aria-checked", "false"); + // As GitHub's picker does: projects first, each row only its checkbox and name. + await expect(dialog.getByRole("option").first()).toHaveAccessibleName("octo-org/score"); + await expect(score).toHaveText("octo-org/score"); + + // Adding keeps the dialog open; the sidebar follows. + await notes.click(); + await expect(notes).toHaveAttribute("aria-checked", "true"); + await expect(project("notes")).toBeVisible(); + await expect(dialog).toBeVisible(); + // The sidebar shows the new project without taking focus from the open dialog. + expect(await dialog.evaluate(node => node.contains(document.activeElement))).toBe(true); + // A choice made here never reorders the list. + await expect(dialog.getByRole("option").first()).toHaveAccessibleName("octo-org/score"); + + // Removing from the keyboard, with Space on the focused row. + let search = dialog.getByRole("textbox", { name: "Search repositories" }); + await search.fill("score"); + await search.press("ArrowDown"); + await expect(score).toBeFocused(); + await page.keyboard.press(" "); + await expect(score).toHaveAttribute("aria-checked", "false"); + await expect(project("score")).toHaveCount(0); + + await page.keyboard.press("Escape"); + await expect(dialog).toBeHidden(); + await page.reload(); + await expect(project("notes")).toBeVisible(); + await expect(project("score")).toHaveCount(0); +}); diff --git a/e2e/machine-choice.e2e.ts b/e2e/machine-choice.e2e.ts index 6a9395bda..d7b932aac 100644 --- a/e2e/machine-choice.e2e.ts +++ b/e2e/machine-choice.e2e.ts @@ -495,7 +495,13 @@ for (let height of [1080, 969]) { await sidebar.getByRole("button", { name: "Add project", exact: true }).click(); } await add.getByRole("textbox", { name: "Search repositories" }).fill(repository); - await add.getByRole("option", { name: `octo-org/${repository}`, exact: true }).click(); + let option = add.getByRole("option", { name: `octo-org/${repository}`, exact: true }); + await option.click(); + // The first project closes the dialog; later ones keep it open for more changes. + if (repository !== "score") { + await expect(option).toHaveAttribute("aria-checked", "true"); + await page.keyboard.press("Escape"); + } await expect(add).toBeHidden(); let create = sidebar.getByRole("button", { name: `New document in ${repository}`, diff --git a/scripts/design-contract/exceptions/dynamic-web.json b/scripts/design-contract/exceptions/dynamic-web.json index fa73f18f6..406481b74 100644 --- a/scripts/design-contract/exceptions/dynamic-web.json +++ b/scripts/design-contract/exceptions/dynamic-web.json @@ -143,7 +143,7 @@ 1 ] ], - "sourceHash": "f14be4530ef2ef97cad54e8e7a343f90e8e8a8a0462d5c45571d798fcf7092f4" + "sourceHash": "67417dc4f0e93a10f76583d07b30b2a4980e9018cbdb72d4627ae6b0f37170e7" }, { "file": "apps/web/src/document-actions-menu.tsx", @@ -227,7 +227,7 @@ 1 ] ], - "sourceHash": "e2e9b967e71700303d65b4f88aa3c6f5c119d176218a81a2e3d33205bc01736c" + "sourceHash": "1159e1e1ac0de083c4d278fbd86d872847522533172dcdc18cc22eef2a551527" }, { "file": "apps/web/src/navigation-drawer.tsx",