diff --git a/src/features/article/hooks/useArticleEditor.test.ts b/src/features/article/hooks/useArticleEditor.test.ts index 174fd0f..bcb445f 100644 --- a/src/features/article/hooks/useArticleEditor.test.ts +++ b/src/features/article/hooks/useArticleEditor.test.ts @@ -183,6 +183,51 @@ describe("useArticleEditor", () => { expect(updates).toHaveLength(0); }); + // An edit made while a save is in flight must not be dropped. The save + // captures the draft as it was when it started, and nothing re-runs the + // autosave effect afterwards, so without an explicit follow-up the newer + // text is never sent and the writer loses it with no error shown. + it("saves again for an edit made while a save was in flight", async () => { + let releaseCreate: (() => void) | null = null; + const bodies: string[] = []; + + server.use( + http.post(`${BASE}/articles`, async ({ request }) => { + const json = (await request.json()) as { body: string }; + bodies.push(json.body); + await new Promise((resolve) => { + releaseCreate = resolve; + }); + return HttpResponse.json({ data: article() }); + }), + http.patch(`${BASE}/articles/:id`, async ({ request }) => { + const json = (await request.json()) as { body: string }; + bodies.push(json.body); + return HttpResponse.json({ data: article() }); + }), + ); + + const { result } = renderHook(() => useArticleEditor(null)); + type(result, "My Article", "First version."); + await flushAutosave(); + await waitFor(() => expect(bodies).toHaveLength(1)); + + // Typed while the create is still open. + act(() => { + result.current.update("body", "Second version."); + }); + await flushAutosave(); + + await act(async () => { + releaseCreate?.(); + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + await flushAutosave(); + + await waitFor(() => expect(bodies).toContain("Second version.")); + expect(result.current.isDirty).toBe(false); + }); + it("reports a failed save and keeps the text dirty", async () => { server.use( http.post(`${BASE}/articles`, () => diff --git a/src/features/article/hooks/useArticleEditor.ts b/src/features/article/hooks/useArticleEditor.ts index 9d7a9f7..6d11e27 100644 --- a/src/features/article/hooks/useArticleEditor.ts +++ b/src/features/article/hooks/useArticleEditor.ts @@ -83,6 +83,8 @@ export function useArticleEditor(initial: Article | null) { initial ? JSON.stringify(draftOf(initial)) : "", ); const isSavingRef = useRef(false); + // Set when an edit arrives mid-save; cleared by the follow-up save. + const resaveRef = useRef(false); const articleIdRef = useRef(initial?.id ?? null); const draftRef = useRef(draft); const coverFileRef = useRef(null); @@ -131,7 +133,13 @@ export function useArticleEditor(initial: Article | null) { if (current.title.trim() === "" || current.body.trim() === "") { return null; } - if (isSavingRef.current) return null; + // A save already running has captured an older draft. Rather than + // dropping this one, mark that another is owed — the running save + // picks it up when it finishes. + if (isSavingRef.current) { + resaveRef.current = true; + return null; + } isSavingRef.current = true; setSaveState("saving"); @@ -195,6 +203,20 @@ export function useArticleEditor(initial: Article | null) { } }, [resolveCoverKey]); + /** + * Runs the save, then runs it again if the writer typed while it was in + * flight. The autosave effect will not re-fire on its own for that edit — + * its dependencies are unchanged by the time the request resolves — so + * without this the newer text is never sent and is lost with no error + * shown anywhere. + */ + const saveChain = useCallback(async (): Promise
=> { + const first = await save(); + if (!resaveRef.current) return first; + resaveRef.current = false; + return (await save()) ?? first; + }, [save]); + const isDirty = JSON.stringify(draft) !== savedSnapshot || coverFile !== null || @@ -206,16 +228,16 @@ export function useArticleEditor(initial: Article | null) { useEffect(() => { if (!canSave || !isDirty || isBusy) return; const timer = setTimeout(() => { - void save(); + void saveChain(); }, AUTOSAVE_DELAY_MS); return () => clearTimeout(timer); - }, [draft, coverFile, coverRemoved, canSave, isDirty, isBusy, save]); + }, [draft, coverFile, coverRemoved, canSave, isDirty, isBusy, saveChain]); /** Saves anything outstanding, then moves the article out of DRAFT. */ const publish = useCallback(async (): Promise
=> { setIsBusy(true); try { - const saved = await save(); + const saved = await saveChain(); const id = articleIdRef.current; if (!id) return null; // A save that failed leaves the server holding older text; going @@ -233,7 +255,7 @@ export function useArticleEditor(initial: Article | null) { } finally { setIsBusy(false); } - }, [save, isDirty]); + }, [saveChain, isDirty]); const archive = useCallback(async (): Promise => { const id = articleIdRef.current; @@ -286,7 +308,7 @@ export function useArticleEditor(initial: Article | null) { isBusy, saveState, saveError, - save, + save: saveChain, publish, archive, remove, diff --git a/src/pages/ArticleEditorPage.reseed.test.tsx b/src/pages/ArticleEditorPage.reseed.test.tsx new file mode 100644 index 0000000..5bbbbe4 --- /dev/null +++ b/src/pages/ArticleEditorPage.reseed.test.tsx @@ -0,0 +1,120 @@ +import "@testing-library/jest-dom"; +import { render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.hoisted(() => { + const _map = new Map(); + vi.stubGlobal("localStorage", { + getItem: (key: string) => _map.get(key) ?? null, + setItem: (key: string, value: string) => { + _map.set(key, String(value)); + }, + removeItem: (key: string) => { + _map.delete(key); + }, + clear: () => { + _map.clear(); + }, + get length() { + return _map.size; + }, + key: (i: number) => [..._map.keys()][i] ?? null, + }); +}); + +const navigate = vi.fn(); +const params: { slug?: string } = {}; +vi.mock("react-router-dom", async () => { + const actual = + await vi.importActual( + "react-router-dom", + ); + return { + ...actual, + useNavigate: () => navigate, + useParams: () => params, + }; +}); + +vi.mock("../shared/layout/PageShell", () => ({ + PageShell: ({ children }: { children: React.ReactNode }) => <>{children}, +})); +vi.mock("../shared/components/ui/SEO", () => ({ SEO: () => null })); +vi.mock("../features/article/hooks/useArticle", () => ({ + useArticle: vi.fn(), +})); +vi.mock("../core/auth/auth.store", () => ({ useAuthStore: vi.fn() })); + +// Deliberately NOT mocking useArticleEditor: this file exists to check that +// the editor's seeded state follows the article being edited, which is a +// property of the real hook plus how the page mounts it. +import { useArticle } from "../features/article/hooks/useArticle"; +import { useAuthStore } from "../core/auth/auth.store"; +import ArticleEditorPage from "./ArticleEditorPage"; +import type { Article } from "../features/article/api/article.types"; + +const article = (id: string, title: string, body: string): Article => ({ + id, + slug: `slug-${id}`, + title, + excerpt: "", + body, + coverImageUrl: null, + coverImageAlt: null, + readingTimeMinutes: 1, + likeCount: 0, + commentCount: 0, + isLiked: false, + isBookmarked: false, + status: "DRAFT", + publishedAt: null, + createdAt: new Date().toISOString(), + author: { id: "user-1", username: "testuser", avatarUrl: "" }, + tags: [], + categories: [], +}); + +const showArticle = (a: Article) => { + vi.mocked(useArticle).mockReturnValue({ + article: a, + isLoading: false, + error: null, + retry: vi.fn(), + }); +}; + +beforeEach(() => { + vi.clearAllMocks(); + localStorage.clear(); + vi.mocked(useAuthStore).mockReturnValue( + true as unknown as ReturnType, + ); +}); + +describe("ArticleEditorPage, moving between articles", () => { + /** + * `useArticleEditor` seeds its state in `useState` initialisers, which run + * once per mount. Going from one article's edit URL to another keeps the + * component mounted, so without a key the editor keeps the first + * article's text while the URL claims the second — and the next autosave + * writes it back to whichever id the hook is still holding. + */ + it("reseeds the editor when the article being edited changes", async () => { + params.slug = "slug-a"; + showArticle(article("a", "Article A", "Body of A.")); + + const { rerender } = render(); + expect(screen.getByLabelText("Title")).toHaveValue("Article A"); + + params.slug = "slug-b"; + showArticle(article("b", "Article B", "Body of B.")); + rerender(); + + await waitFor(() => + expect(screen.getByLabelText("Title")).toHaveValue("Article B"), + ); + expect( + screen.getByLabelText("Write your article in Markdown..."), + ).toHaveValue("Body of B."); + }); +}); diff --git a/src/pages/ArticleEditorPage.test.tsx b/src/pages/ArticleEditorPage.test.tsx index 5f439d3..02dcb16 100644 --- a/src/pages/ArticleEditorPage.test.tsx +++ b/src/pages/ArticleEditorPage.test.tsx @@ -122,12 +122,14 @@ describe("ArticleEditorPage", () => { expect(screen.queryByLabelText("Title")).not.toBeInTheDocument(); }); - it("opens a blank editor when there is no slug", () => { + it("opens a blank editor without asking the API for anything", () => { render(); expect(screen.getByLabelText("Title")).toBeInTheDocument(); - // Nothing to load, so nothing should be waiting on the network. - expect(useArticle).toHaveBeenCalledWith(""); + // There is nothing to load. Calling the loader with an empty slug + // spends a request on every visit to the editor and answers with + // nothing useful — this assertion used to bake that in. + expect(useArticle).not.toHaveBeenCalled(); }); it("types into the title and body", async () => { diff --git a/src/pages/ArticleEditorPage.tsx b/src/pages/ArticleEditorPage.tsx index 6595d1a..19a473c 100644 --- a/src/pages/ArticleEditorPage.tsx +++ b/src/pages/ArticleEditorPage.tsx @@ -29,13 +29,20 @@ export default function ArticleEditorPage() { if (!isAuthenticated) navigate("/", { replace: true }); }, [isAuthenticated, navigate]); - const { article, isLoading, error } = useArticle(slug ?? ""); - if (!isAuthenticated) return null; - // A new article has no slug to load, so the editor mounts straight away. + // A new article has nothing to load, so it never mounts the loader — + // asking the API for an empty slug spends a request on every visit to + // the editor and answers with nothing useful. if (!slug) return ; + return ; +} + +function EditExisting({ slug }: { slug: string }) { + const { t } = useI18n(); + const { article, isLoading, error, retry } = useArticle(slug); + if (isLoading) { return ( @@ -49,14 +56,26 @@ export default function ArticleEditorPage() { if (error || !article) { return ( -
- {error ?? ""} +
+

+ {error ?? t("page.articleNotFound")} +

+ {error && ( + + )}
); } - return ; + // Keyed so moving from one article's edit URL to another remounts + // the editor. `useArticleEditor` seeds from `initial` in `useState` + // initialisers, which run once per mount — without this the editor keeps + // the first article's text while the URL claims the second, and the next + // autosave writes it back to whichever id the hook is still holding. + return ; } /** diff --git a/src/pages/CommentDetailPage.tsx b/src/pages/CommentDetailPage.tsx index 7487f76..4a3fe90 100644 --- a/src/pages/CommentDetailPage.tsx +++ b/src/pages/CommentDetailPage.tsx @@ -3,7 +3,10 @@ import { useNavigate, useParams } from "react-router-dom"; import { PageShell } from "../shared/layout/PageShell"; import { TrendingTopicsWidget } from "../shared/components/TrendingTopicsWidget"; import { commentApi } from "../features/comment/api/comment.api"; -import type { Comment } from "../features/comment/api/comment.types"; +import type { + Comment, + CommentTarget, +} from "../features/comment/api/comment.types"; import { CommentCard } from "../features/comment/components/CommentCard"; import { Button } from "../shared/components/ui/Button"; import { useCommentReplies } from "../features/comment/hooks/useCommentReplies"; @@ -30,6 +33,14 @@ export default function CommentDetailPage() { removeReply, } = useCommentReplies(id!); + // A comment hangs off a post or an article, never both. Derived once so + // the reply box and anything else that needs it cannot disagree. + const commentTarget: CommentTarget | null = comment?.postId + ? { type: "post", id: comment.postId } + : comment?.articleId + ? { type: "article", id: comment.articleId } + : null; + const handleBack = () => { if (window.history.length > 1) { navigate(-1); @@ -115,20 +126,19 @@ export default function CommentDetailPage() { ); }} /> - { - addReply(newReply); - }} - /> + {/* Narrowed rather than asserted. The database guarantees + exactly one parent, but a comment that somehow arrives + with neither has nowhere to send a reply — offering the + box anyway would post to `/articles//comments`. */} + {commentTarget && ( + { + addReply(newReply); + }} + /> + )} {repliesLoading ? (
{t("page.loadingReplies")} diff --git a/worker/index.test.ts b/worker/index.test.ts index 997f9ef..0d8c29f 100644 --- a/worker/index.test.ts +++ b/worker/index.test.ts @@ -244,6 +244,25 @@ describe("worker routing", () => { expect(html).toContain("<script>"); }); + // The slug arrives straight off the request URL, where percent-escapes + // survive the route match: `%2f` is not a path separator to the URL + // parser, so it reaches here inside a single segment. Spliced in raw + // it reaches the API as a slash and resolves to a different route. + it("encodes the slug before putting it in the API path", async () => { + let requested: string | null = null; + server.use( + http.get(`${API}/articles/:slug`, ({ request }) => { + requested = new URL(request.url).pathname; + return HttpResponse.json({ data: article }); + }), + ); + const { env } = makeEnv(); + + await worker.fetch(get("/articles/a%2fb"), env); + + expect(requested).toBe("/api/v1/articles/a%252fb"); + }); + // `/articles` is the list page, one segment long — it must not be // mistaken for an article slug and sent to the API. it("leaves the list page on the site defaults", async () => { diff --git a/worker/index.ts b/worker/index.ts index d47ef11..c2a752d 100644 --- a/worker/index.ts +++ b/worker/index.ts @@ -128,7 +128,14 @@ async function fetchArticle( slug: string, ): Promise { try { - const res = await fetch(`${apiBase}/articles/${slug}`); + // Encoded before it is spliced into the path. The value comes + // straight off the request URL, where percent-escapes survive the + // route match — `%2e%2e` is read as a path segment by the URL parser + // and would resolve the request somewhere other than this article. + // The client-side API module already encodes; this matched it. + const res = await fetch( + `${apiBase}/articles/${encodeURIComponent(slug)}`, + ); if (!res.ok) return null; const json = (await res.json()) as { data: ArticleMeta }; return json.data;