fix(articles): five defects found reviewing the editor - #133
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hu1QuLxS84vdf1gmzoGtWP
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
tdn-client | 2f42868 | Commit Preview URL Branch Preview URL |
Aug 26 2026, 02:41 AM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review pass over the articles feature, with attention on the writing half. Each defect was reproduced with a failing test on
mainbefore it was fixed.Ordered by what they cost.
1. A lost edit — silent data loss
Type, pause, keep typing while the save is still in flight, and the second batch never reaches the server.
saverefuses 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. Nothing reported it either: the indicator sat on "unsaved changes" and the writer had to type again to shake it loose. On a slow connection this is exactly the moment you would lose a paragraph.Saves now chain: one more runs if the draft moved on while a save was open. Publish flushes the same chain, so it cannot put older text live.
2. The editor was not keyed by the article it edits
useArticleEditorseeds inuseStateinitialisers, which run once per mount. 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. It wasn't. Now it is.
3. A wasted request on every new article
Opening
/articles/newcalled the loader with an empty slug, spending aPUBLIC-limited request on a URL that answers with nothing useful.Worth calling out: 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 the retry it was missing.
4. The Worker spliced the slug into the API path unencoded
The slug comes straight off the request URL, where percent-escapes survive the route match.
%2fis not a separator to the URL parser, so it arrives inside a single segment and reaches the API as a slash, resolving a different route than the one intended.Low severity — the value cannot escape the path into the authority, and everything injected into meta tags is still escaped — but the client-side API module already encoded, so this was an inconsistency with a sharp edge. Now matched.
5. A reply box with nowhere to send
A comment carrying neither
postIdnorarticleIdwould 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 no parent. Also removes a!assertion the API notes explicitly warn against.What I checked and found clean
Markdown sanitisation (
skipHtml, norehype-raw, tests in place), cover and avatar URLs throughgetSafeImageSrceverywhere they are rendered, the Worker's HTML escaping of article title and excerpt, the cover upload's size and type gates, tag normalisation against the server pattern, and the rate-limit shapes — one upload per chosen file, one create then updates.Verification
pnpm build,tsc -b,pnpm lintclean. 473 unit tests across 66 files and 29 Playwright tests pass. Three of the five have a test that fails onmainand passes here; the other two are covered by assertions on the request the code makes.🤖 Generated with Claude Code
https://claude.ai/code/session_01Hu1QuLxS84vdf1gmzoGtWP