diff --git a/API.md b/API.md index 4c448ec9..356c1cf4 100644 --- a/API.md +++ b/API.md @@ -33,6 +33,7 @@ has one word, the same as in pages. | `/@/dumps/` | | anyone; a private dump, its poster | `Dump` | | `/@/dumps//edit` | | its poster, `dump:moderate` | `DumpEdit` | | `/@/playlists/` | | anyone; a private playlist, its owner | `PlaylistDetail` | +| `/@/playlists//edit` | | its owner, `playlist:moderate` | `PlaylistDetail` | | `/search?q=` | `/search/users?q=`, `/search/playlists?q=` | anyone | `Search` | | `/notifications` | | signed in | `Notifications` | | `/community` | `/community/monthly`; the bare path is the week | signed in | `Community` | diff --git a/api/lib/self-url.ts b/api/lib/self-url.ts index d1903124..0ca8d748 100644 --- a/api/lib/self-url.ts +++ b/api/lib/self-url.ts @@ -30,12 +30,12 @@ export function isSelfHost(host: string): boolean { /** * Checks whether `url` links to one of this site's own dump, user or playlist - * pages, by `parsePagePath`; a dump's editor doesn't count. Matched on the + * pages, by `parsePagePath`; an editor doesn't count. Matched on the * host as well as the path: `/@bob` on some other site is someone else's * page, not ours. */ export function isSelfUrl(url: URL): boolean { if (!isSelfHost(url.host)) return false; const page = parsePagePath(url.pathname); - return !!page && !(page.kind === "dump" && page.edit); + return !!page && !(page.kind !== "user" && page.edit); } diff --git a/api/middleware/og.ts b/api/middleware/og.ts index 21069f41..b52876f7 100644 --- a/api/middleware/og.ts +++ b/api/middleware/og.ts @@ -119,7 +119,7 @@ export async function ogMiddleware( if (pathname.startsWith("/api/")) return await next(); const page = parsePagePath(pathname); - if (!page || (page.kind === "dump" && page.edit)) return await next(); + if (!page || (page.kind !== "user" && page.edit)) return await next(); const origin = PUBLIC_URL; let meta: OGMeta | null = null; diff --git a/shared/paths.ts b/shared/paths.ts index d6282788..8ec620d3 100644 --- a/shared/paths.ts +++ b/shared/paths.ts @@ -111,9 +111,15 @@ export function dumpPath(dump: OwnedItem, view?: "edit"): string { return view ? `${path}/${view}` : path; } -/** Returns the path of a playlist's page, `/@owner/playlists/`. */ -export function playlistPath(playlist: OwnedItem): string { - return `${userPath(playlist.ownerUsername)}/playlists/${seg(playlist.slug)}`; +/** + * Returns the path of a playlist's page, `/@owner/playlists/`, or of + * the page with its editor open, one segment more. + */ +export function playlistPath(playlist: OwnedItem, view?: "edit"): string { + const path = `${userPath(playlist.ownerUsername)}/playlists/${ + seg(playlist.slug) + }`; + return view ? `${path}/${view}` : path; } /** @@ -171,11 +177,11 @@ export function communityPath(view: DigestCadence = "weekly"): string { export type PagePath = | { kind: "user"; username: string; rest: string[] } | { kind: "dump"; username: string; slug: string; edit: boolean } - | { kind: "playlist"; username: string; slug: string }; + | { kind: "playlist"; username: string; slug: string; edit: boolean }; /** * Returns what a page path under a user names: a profile (with its tab), a - * dump (or its editor) or a playlist. It returns null for any other path, a + * dump or a playlist (or its editor). It returns null for any other path, a * feed or an app page, and for one under a user that no page answers. The * names are decoded but not looked up, so the user or the item may not exist. */ @@ -195,18 +201,11 @@ export function parsePagePath(pathname: string): PagePath | null { const username = handle.slice(1); if ((section === "dumps" || section === "playlists") && slug) { - if (section === "dumps" && more.length <= 1) { - if (more.length === 0) { - return { kind: "dump", username, slug, edit: false }; - } - if (more[0] === "edit") { - return { kind: "dump", username, slug, edit: true }; - } + if (more.length > 1 || (more.length === 1 && more[0] !== "edit")) { + return null; } - if (section === "playlists" && more.length === 0) { - return { kind: "playlist", username, slug }; - } - return null; + const kind = section === "dumps" ? "dump" : "playlist"; + return { kind, username, slug, edit: more.length === 1 }; } const rest = segments.slice(1); diff --git a/shared/test/paths.test.ts b/shared/test/paths.test.ts index f4269f3c..b37ef3e5 100644 --- a/shared/test/paths.test.ts +++ b/shared/test/paths.test.ts @@ -66,7 +66,18 @@ Deno.test("parsePagePath: what a path under a user names", () => { kind: "playlist", username: "bob", slug: "mix", + edit: false, }); + assertEquals(parsePagePath("/@bob/playlists/mix/edit"), { + kind: "playlist", + username: "bob", + slug: "mix", + edit: true, + }); + assertEquals( + playlistPath({ ownerUsername: "bob", slug: "mix" }, "edit"), + "/@bob/playlists/mix/edit", + ); for ( const other of [ "/", @@ -78,7 +89,7 @@ Deno.test("parsePagePath: what a path under a user names", () => { "/@bob/nope", "/@bob/follow/users", "/@bob/dumps/x/y", - "/@bob/playlists/x/edit", + "/@bob/playlists/x/y", "/@bob/%E0%A4%A", ] ) { @@ -96,6 +107,7 @@ Deno.test("handles: a user from another instance is /@name@host", () => { kind: "playlist", username: "alice@music.example", slug: "mix", + edit: false, }); assertEquals(parsePagePath("/@alice%40music.example"), { kind: "user", diff --git a/src/App.tsx b/src/App.tsx index 26c90dfd..3cf57e4a 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -27,7 +27,7 @@ * - `/@:username/*` is a user's (`UserRoutes`): their profile and its tabs, * their dumps at `dumps/:slug` (`Dump`) and `dumps/:slug/edit` (`DumpEdit`, * signed in only), and their playlists at `playlists/:slug` - * (`PlaylistDetail`). + * (`PlaylistDetail`), with its editor open at `playlists/:slug/edit`. * - `/:categorySlug` and `/:categorySlug/:feedTab` are a dump category's feed * (`Index`), with its `/follow/playlists` too. * - Any other path is `NotFound`. @@ -185,6 +185,7 @@ function UserRoutes() { } /> } /> + } /> } /> ); diff --git a/src/hooks/playlist/usePlaylist.ts b/src/hooks/playlist/usePlaylist.ts index 3d3da4c0..61adc806 100644 --- a/src/hooks/playlist/usePlaylist.ts +++ b/src/hooks/playlist/usePlaylist.ts @@ -100,12 +100,14 @@ function withDetails( * see in it. The socket's events keep its details and dumps live: a dump * another client removes fades out, one it adds is fetched, a new order is * applied, one that goes private leaves and comes back at its place, and a - * deleted playlist sends the page home. A new slug replaces the URL, and a - * path that names the playlist on screen, as that one does, fetches nothing. + * deleted playlist sends the page home. A new slug replaces the URL, keeping + * the page's `view`, and a path that names the playlist on screen, as that + * one does, fetches nothing. */ export function usePlaylist( username: string | undefined, slug: string | undefined, + view?: "edit", ): UsePlaylistResult { const navigate = useNavigate(); const { user, authFetch } = useAuth(); @@ -142,7 +144,7 @@ export function usePlaylist( ) { setTarget({ username, slug }); } - useCanonicalPath(playlist && playlistUrl(playlist)); + useCanonicalPath(playlist && playlistUrl(playlist, view)); // The socket and the API's writes name playlists by id. const id = playlist?.id; const url = `${API_URL}/api/playlists/${id}`; diff --git a/src/pages/PlaylistDetail.tsx b/src/pages/PlaylistDetail.tsx index 3f8b8ffb..313f5fd8 100644 --- a/src/pages/PlaylistDetail.tsx +++ b/src/pages/PlaylistDetail.tsx @@ -5,8 +5,8 @@ * @module */ -import { useState } from "react"; -import { useNavigate, useParams } from "react-router"; +import { startTransition, useState } from "react"; +import { Navigate, useLocation, useNavigate, useParams } from "react-router"; import { t } from "@lingui/core/macro"; import { Trans } from "@lingui/react/macro"; import { can } from "../utils/auth/permissions.ts"; @@ -21,16 +21,31 @@ import { ConfirmModal } from "../components/shared/ConfirmModal.tsx"; import { PlaylistHeader } from "../components/playlist/PlaylistHeader.tsx"; import { PlaylistEditForm } from "../components/playlist/PlaylistEditForm.tsx"; import { PlaylistDumpList } from "../components/playlist/PlaylistDumpList.tsx"; +import { playlistUrl } from "../utils/shared/urls.ts"; +import type { OwnedItem } from "../../shared/paths.ts"; +import { loginPath } from "../utils/auth/next.ts"; + +/** Passed to {@link PlaylistDetail} by `App`'s route. */ +export interface PlaylistDetailProps { + /** Set at `/edit`, where the editor is open. */ + editing?: boolean; +} /** * Renders `/@:username/playlists/:slug`, for anyone who may see the playlist: * a private one only to its owner. `usePlaylist` loads it, by a former name or - * slug too, and keeps it live. Its owner, or a user with `playlist:moderate`, can edit or - * delete it, drag its dumps into a new order, and remove one, with a short - * window to undo. + * slug too, and keeps it live. Its owner, or a user with `playlist:moderate`, + * can drag its dumps into a new order and remove one, with a short window to + * undo, and edit or delete it in the editor that opens over its header at + * `/@:username/playlists/:slug/edit`. Both paths render the page at the same + * place, so opening the editor keeps the playlist loaded. Anyone else there + * gets the page, or is asked to sign in first. */ -export function PlaylistDetail(): React.JSX.Element { +export function PlaylistDetail( + { editing = false }: PlaylistDetailProps, +): React.JSX.Element { const { handle, slug } = useParams(); + const { pathname } = useLocation(); const navigate = useNavigate(); const { user } = useAuth(); const { @@ -43,9 +58,12 @@ export function PlaylistDetail(): React.JSX.Element { move, saveOrder, deletePlaylist, - } = usePlaylist(handle?.slice(1), slug); + } = usePlaylist(handle?.slice(1), slug, editing ? "edit" : undefined); - const [editOpen, setEditOpen] = useState(false); + const loaded = state.status === "loaded" ? state.data : null; + const canEdit = !!loaded && + ((!!user && user.id === loaded.userId) || can(user, "playlist:moderate")); + const editOpen = editing && canEdit; const headerRef = useHeightTransition(editOpen); const [confirmDelete, setConfirmDelete] = useState(false); @@ -85,8 +103,15 @@ export function PlaylistDetail(): React.JSX.Element { } const playlist = state.data; - const canEdit = (!!user && user.id === playlist.userId) || - can(user, "playlist:moderate"); + if (editing && !canEdit) { + return user + ? + : ; + } + // The editor is a view of the page: opening it adds a history entry, which + // closing it, or saving, takes the place of. + const closeEditor = (current: OwnedItem = playlist) => + navigate(playlistUrl(current), { replace: true }); return ( @@ -100,13 +125,17 @@ export function PlaylistDetail(): React.JSX.Element { ? ( setEditOpen(false)} + onCancel={() => closeEditor()} onRequestDelete={() => setConfirmDelete(true)} onSaved={(updated) => { - setEditOpen(false); - // Shown at once rather than left to the socket's echo, - // which may never come (disconnected). - applyDetails(updated); + // Shown at once rather than left to the socket's echo, which + // may never come (disconnected). The router navigates in a + // transition; were the details applied apart, the page would + // render them still at /edit, and put a new slug's /edit back. + startTransition(() => { + applyDetails(updated); + closeEditor({ ...playlist, slug: updated.slug }); + }); }} /> ) @@ -114,7 +143,9 @@ export function PlaylistDetail(): React.JSX.Element {
setEditOpen(true) : undefined} + onEdit={canEdit + ? () => navigate(playlistUrl(playlist, "edit")) + : undefined} />
)} diff --git a/src/test/pages/PlaylistDetail.test.tsx b/src/test/pages/PlaylistDetail.test.tsx index 7174afd0..752934ce 100644 --- a/src/test/pages/PlaylistDetail.test.tsx +++ b/src/test/pages/PlaylistDetail.test.tsx @@ -59,7 +59,15 @@ function open( path: "/:handle/playlists/:slug", session, categories, - extraRoutes: home

} />, + extraRoutes: ( + <> + home

} /> + } + /> + + ), }); const socket = FakeWebSocket.last; socket.welcome(); @@ -142,12 +150,39 @@ describe("PlaylistDetail", () => { await user.clear(box); await user.type(box, "Better tunes"); await user.click(screen.getByRole("button", { name: "Save" })); - expect(await screen.findByText("Better tunes")).toBeInTheDocument(); + // Saving closes the editor, whose text would be found first. + await waitFor(() => expect(currentPath()).toBe("/@owner/playlists/mix")); + expect(screen.getByText("Better tunes")).toBeInTheDocument(); expect(calls("PUT /api/playlists/:id")[0].body).toEqual({ description: "Better tunes", isPublic: true, }); + }); + + test("the editor is a page: /edit opens it, closing goes back to the page", async () => { + const { user } = open({ url: "/@owner/playlists/mix/edit" }); + expect(await screen.findByRole("textbox", { name: "Description" })) + .toBeInTheDocument(); + await user.click(screen.getByRole("button", { name: "Cancel" })); expect(currentPath()).toBe("/@owner/playlists/mix"); + await user.click(screen.getByRole("button", { name: "Edit" })); + expect(currentPath()).toBe("/@owner/playlists/mix/edit"); + expect(calls("GET /api/users/:u/playlists/:slug")).toHaveLength(1); + }); + + test("at /edit, anyone else gets the page", async () => { + open({ session: visitor, url: "/@owner/playlists/mix/edit" }); + await waitFor(() => expect(currentPath()).toBe("/@owner/playlists/mix")); + expect(screen.queryByRole("textbox", { name: "Description" })).toBeNull(); + }); + + test("at /edit, a guest is asked to sign in first", async () => { + open({ session: null, url: "/@owner/playlists/mix/edit" }); + await waitFor(() => + expect(currentPath()).toBe( + "/login?next=%2F%40owner%2Fplaylists%2Fmix%2Fedit", + ) + ); }); test("its categories show, and editing them sends the new set", async () => { @@ -444,7 +479,7 @@ describe("PlaylistDetail", () => { }), ); expect(await screen.findByRole("alert")).toHaveTextContent("Forbidden"); - expect(currentPath()).toBe("/@owner/playlists/mix"); + expect(currentPath()).toBe("/@owner/playlists/mix/edit"); }); test("live: added dumps appear, removed ones fade, a deleted playlist sends you home", async () => { diff --git a/src/utils/shared/urls.ts b/src/utils/shared/urls.ts index 497f7e3b..5b258159 100644 --- a/src/utils/shared/urls.ts +++ b/src/utils/shared/urls.ts @@ -138,9 +138,12 @@ export function userApiUrl( .join("/"); } -/** Returns the path of the page of `playlist`, `/@owner/playlists/`. */ -export function playlistUrl(playlist: OwnedItem): string { - return playlistPath(playlist); +/** + * Returns the path of the page of `playlist`, `/@owner/playlists/`, or + * of the page with its editor open (`playlistPath`). + */ +export function playlistUrl(playlist: OwnedItem, view?: "edit"): string { + return playlistPath(playlist, view); } /**