fix(articles): five defects found reviewing the editor

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<void>((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`, () =>

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<string | null>(initial?.id ?? null);
     const draftRef = useRef(draft);
     const coverFileRef = useRef<File | null>(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<Article | null> => {
+        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<Article | null> => {
         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<boolean> => {
         const id = articleIdRef.current;
@@ -286,7 +308,7 @@ export function useArticleEditor(initial: Article | null) {
         isBusy,
         saveState,
         saveError,
-        save,
+        save: saveChain,
         publish,
         archive,
         remove,

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<string, string>();
+    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<typeof import("react-router-dom")>(
+            "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<typeof useAuthStore>,
+    );
+});
+
+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(<ArticleEditorPage />);
+        expect(screen.getByLabelText("Title")).toHaveValue("Article A");
+
+        params.slug = "slug-b";
+        showArticle(article("b", "Article B", "Body of B."));
+        rerender(<ArticleEditorPage />);
+
+        await waitFor(() =>
+            expect(screen.getByLabelText("Title")).toHaveValue("Article B"),
+        );
+        expect(
+            screen.getByLabelText("Write your article in Markdown..."),
+        ).toHaveValue("Body of 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(<ArticleEditorPage />);
 
         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 () => {

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 <Editor initial={null} />;
 
+    return <EditExisting slug={slug} />;
+}
+
+function EditExisting({ slug }: { slug: string }) {
+    const { t } = useI18n();
+    const { article, isLoading, error, retry } = useArticle(slug);
+
     if (isLoading) {
         return (
             <PageShell width="reading">
@@ -49,14 +56,26 @@ export default function ArticleEditorPage() {
     if (error || !article) {
         return (
             <PageShell width="reading">
-                <div className="p-10 text-center text-white/40">
-                    {error ?? ""}
+                <div className="flex flex-col items-center gap-4 p-10 text-center">
+                    <p className="text-sm text-white/40">
+                        {error ?? t("page.articleNotFound")}
+                    </p>
+                    {error && (
+                        <Button variant="outline" size="sm" onClick={retry}>
+                            {t("articleList.tryAgain")}
+                        </Button>
+                    )}
                 </div>
             </PageShell>
         );
     }
 
-    return <Editor initial={article} />;
+    // 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 <Editor key={article.id} initial={article} />;
 }
 
 /**

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() {
                             );
                         }}
                     />
-                    <CommentBox
-                        target={
-                            comment.postId
-                                ? { type: "post", id: comment.postId }
-                                : {
-                                      type: "article",
-                                      id: comment.articleId ?? "",
-                                  }
-                        }
-                        parentId={id!}
-                        onCommentCreated={(newReply) => {
-                            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 && (
+                        <CommentBox
+                            target={commentTarget}
+                            parentId={id!}
+                            onCommentCreated={(newReply) => {
+                                addReply(newReply);
+                            }}
+                        />
+                    )}
                     {repliesLoading ? (
                         <div className="p-8 text-white/40">
                             {t("page.loadingReplies")}