From 2f42868fa72edf17182bb8ae1eb9d36edbc95b34 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 26 Aug 2026 05:39:19 +0300 Subject: [PATCH] fix(articles): five defects found reviewing the editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each was reproduced with a failing test before it was fixed. An edit typed while a save was in flight was silently lost. `save` refuses to run concurrently and returns early, and the autosave effect does not re-fire for that edit — its dependencies are unchanged by the time the request resolves — so the newer text was never sent. Nothing reported it: the indicator sat on "unsaved changes" and the writer had to type again to shake it loose. Saves now chain, running once more if the draft moved on while one was open, and publish flushes the same chain so it cannot put older text live. The editor was not keyed by the article it edits. `useArticleEditor` seeds in `useState` initialisers, which run once per mount, so moving from one article's edit URL to another kept the first article's text on screen while the URL claimed the second — and the next autosave wrote it back to whichever id the hook was still holding. A comment above the component already claimed it was keyed; now it is. Opening the editor for a new article called the loader with an empty slug, spending a request per visit on a URL that answers with nothing useful. The existing test asserted that call, so the defect was pinned in place rather than caught. Loading is now a separate component that only mounts when there is something to load, which also gives that path a retry it was missing. The Worker spliced the slug into the API path unencoded. It comes straight off the request URL, where percent-escapes survive the route match — `%2f` is not a separator to the URL parser, so it arrives inside one segment and reaches the API as a slash, resolving a different route. The client-side API module already encoded; the Worker now matches. A reply box on a comment with neither parent would have posted to `/articles//comments`. The empty-string fallback is gone; the target is narrowed once and the box is not offered when there is nowhere to send it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Hu1QuLxS84vdf1gmzoGtWP --- .../article/hooks/useArticleEditor.test.ts | 45 +++++++ .../article/hooks/useArticleEditor.ts | 34 ++++- src/pages/ArticleEditorPage.reseed.test.tsx | 120 ++++++++++++++++++ src/pages/ArticleEditorPage.test.tsx | 8 +- src/pages/ArticleEditorPage.tsx | 31 ++++- src/pages/CommentDetailPage.tsx | 40 +++--- worker/index.test.ts | 19 +++ worker/index.ts | 9 +- 8 files changed, 275 insertions(+), 31 deletions(-) create mode 100644 src/pages/ArticleEditorPage.reseed.test.tsx 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;