Commit ca00645
authored
fix: moderation preview scope, comment vote races, and email link origin (#1344)
* fix: moderation preview scope, comment vote races, and email link origin
Three fixes, consolidated into one PR by request.
1. Move the moderation preview off the public reader routes (#1340
follow-up)
#1340 let admins resolve in_review/rejected posts at their public URLs.
That put the public reader — vote, bookmark and comment controls — on a
post that may be about to be rejected, and `post.vote` has no status
guard, so a misclick writes a vote and author points onto content the
moderator is declining. It also stopped admins seeing the site the way
readers do, and made the rejected-post banner's "not visible to anyone
else" untrue.
Preview now lives at /admin/moderation/preview/{id}: read-only, inside
the admin gate. The public routes and their visibility filter revert.
It also fixes the link path. #1340 sent Preview straight off-site, so the
member's own title/excerpt/body — where a spammer would put the payload —
was never shown; the preview renders both halves. And that off-site href
skipped `safeExternalHref` and rel, so an externalUrl that never passed
`httpUrl()` validation would run as a `javascript:` URL inside the
authenticated admin session, and the page under review received the admin
surface as its referrer.
2. Drop the frozen sort snapshot, serialise votes per comment (#1341
follow-up)
Freezing sort scores was more than the fix needed and wrong on its own
terms: counts still updated on refetch while the order did not, so a
thread could show a 42-point comment below a 3-point one; the documented
"re-pick the sort" escape hatch never fired, because selecting the
already-selected option is a no-op; and the added tiebreak made Top
identical to New on the common all-zero thread.
Not refetching after a successful vote is the whole fix. Ordering is
derived from the data on screen again, so it cannot contradict the counts
beside it. Votes are serialised per comment (newest click wins), since
#1341 dropped the in-flight guard without replacing it and overlapping
writes could land in either order. The resync remount is per comment too.
3. Email links pointed at the deployment, not the site
`getAppOrigin()` fell back to VERCEL_URL, which is the unique
per-deployment hostname — and Vercel sets it in production too. With
DOMAIN_NAME unset, every link it built (the admin's "post awaiting
review" deep link, report emails, the verification link) went out as
*.vercel.app. Production now resolves to the project's production domain,
falling back to the canonical origin; preview deploys still get their own
URL. The duplicate copy of this logic in utils/emailToken.ts is gone.
* fix: address review of the consolidated fixes
Review of #1344 found nine issues, three of them mine from the previous
round.
Draft exposure (the serious one): the new preview route loaded any post
by id with no status predicate, so an admin with a post id could read a
member's private, never-submitted draft. It is now restricted to work
that actually entered the pipeline — published, in_review, rejected — and
`postVisibility.ts` is back, minus the admin bypass, carrying tests for
both rules including "never exposes a draft, whoever is looking".
Deleting that module was never required to drop the bypass, and doing so
had also left the same rule inlined twice in two different shapes.
Failed votes stranded earlier ones: I removed the pre-remount refetch on
the reasoning that the cache never saw the failed vote. That is true of
the failed vote but not of earlier successful ones, which never refetch
by design — so remounting reseeded the control from a cache that predates
them. The refetch is back, before the key bump.
Comment ordering on ties was arbitrary: the "Top" comparator leaned on
sort stability to keep "the server's order", but the server orders by
ltree path, built from a random uuid. On a thread where most scores are
0, that is no order at all. Ties now sort oldest-first, so Top degrades
to chronological rather than to a copy of New.
Reordering under the reader: with the score snapshot gone, a window-focus
refetch could re-rank the thread mid-read. That query no longer refetches
on focus; the explicit refetches after create/edit/delete stay, since
those follow something the reader did.
Also: raw tRPC error text could reach the vote toast (only the rate-limit
message, which is written for readers, is surfaced now); a malformed post
id 500'd on the uuid cast instead of 404ing; the preview's two queries ran
serially when both key off the route param; and a fork deploying to
production without VERCEL_PROJECT_PRODUCTION_URL got codu.co hardcoded
over its own configured NEXTAUTH_URL.
* ci(e2e): serve a production build instead of the dev server
The suite has been failing on CI while staying green locally: 19 failures
spread across admin navigation, the editor publish flow, bookmarking, the
feed sidebar and moderation. Identical failures on develop and on every
branch off it, so nothing in the feature work caused them.
They all share a cause. Playwright's webServer ran `next dev`, so the
first request to each route blocked on an on-demand Turbopack compile.
Locally that is under a second; on a cold runner with three workers
compiling at once it outlasts the 10s expect timeout — hence assertions
like `toHaveURL(/admin/users)` polling 13 times and giving up while the
navigation was still compiling.
CI now builds once and serves it. Measured on the same machine, a cold
route costs ~0.7-1.2s under `next dev` and ~0.02-0.03s prebuilt.
Local runs keep `next dev` for the fast feedback loop.
EMAIL_AUTH_ENABLED is set for the job because a production build runs with
NODE_ENV=production, which would otherwise disable the passwordless
provider that dev turns on implicitly.
* fix: review round three, and build the app inside the e2e webServer
The e2e harness fix in the previous commit did not take. The job is
triggered by `pull_request_target`, which takes the WORKFLOW from the base
branch and the CODE from the PR head — so the build step added to the
workflow never ran, while playwright.config (from the head) had already
switched to serving a prebuilt app. Result: "Could not find a production
build". The build now happens inside the webServer command, where head and
workflow cannot disagree, and the env it needs moved into the npm scripts
for the same reason.
Review fixes:
- moderationPreviewFilter missed `scheduled` and `unlisted`, so preview
404'd on a post the admin had just approved-with-schedule. The status
list is now derived from the enum instead of hand-listed.
- The "top" tie-break went back to newest-first. Oldest-first read better
as conversation order but buried a comment the moment you posted it,
which is worse than Top resembling New on an unvoted thread.
- create/edit awaited react-query's void `mutate`, so their try/catch was
dead and the editor cleared before the request finished: a failed post
discarded what you typed, silently. Both use mutateAsync now.
- The preview fetched the cover image and never rendered it. Clean body
copy under an abusive image would have sailed through; it is shown now,
behind the same scheme guard as the external URL.
- PostBody rendered the site-wide 404 component for an empty tiptap body,
which put a "page not found" panel inside the admin shell. The empty
state is the caller's to choose now.
- getAppOrigin fell back to the hardcoded codu.co ahead of the deployment's
own URL, so an unconfigured fork mailed its users to this site.
- A failed vote now clears its queued follow-up explicitly.
Accepted, not fixed: the thread no longer refetches on window focus, so an
open tab does not pick up other people's comments until you post, navigate
or reload. That is the cost of not moving comments under someone mid-read.
* ci(e2e): trust the request host when serving the production build
The prebuilt server ran, but every authenticated test failed: NextAuth
rejects the request host under NODE_ENV=production unless AUTH_TRUST_HOST
is set, so /api/auth/session returned UntrustedHost, session.user was
undefined, and pages blew up on `session.user.username`. 159 failures.
Dev never hits this — it trusts the host implicitly — which is why the
suite passed locally. My earlier local check passed for the wrong reason:
I had happened to pass AUTH_URL on the command line, which also satisfies
the trust check, so the gap only showed up on CI where it is not set.
Reproduced locally against a production build: without AUTH_TRUST_HOST,
/admin redirects and the log carries UntrustedHost; with it, /admin and
/admin/moderation both return 200 and the log is clean.
* test(e2e): let the suite point at a throwaway database
`setup.ts` and `teardown.ts` hardcoded localhost:5432/postgres, which is
also the dev database. Running the suite locally therefore writes fixtures
into whatever you have been working on, so the remaining pre-existing
failures cannot be debugged without risking your own data.
Both now honour DATABASE_URL, falling back to the same string, so CI is
unaffected and a local run can be aimed at a scratch database instead.
* test(e2e): enable moderation, fix the sidebar viewport, centralise the DB url
Three causes behind the long-standing failures, found by running the suite
locally against a throwaway database.
MODERATION_ENABLED was never set for the e2e app. The whole pipeline —
publish gating, the review queue, link dedupe — is behind that flag, so
e2e/moderation.spec.ts was asserting behaviour the server had switched
off. All four of its tests pass with the flag on.
The feed sidebar test asserted the right rail is visible on any non-mobile
viewport, but .app-main folds the rail away under 1300px and Playwright's
desktop viewport is 1280 — so it was asserting against a width where the
rail is correctly hidden. The test now widens past the breakpoint.
Four helpers in e2e/utils/utils.ts still hardcoded the connection string
that the file had already centralised, so a run aimed at a scratch
DATABASE_URL wrote its fixtures into the default database instead and then
failed on foreign keys.
Local suite on Desktop Chrome: 129 passed, 1 failed (a flaky multi-user
notification test), down from 6 failures before these changes.
* test(e2e): give the bookmark specs their own fixture per browser project
Bookmarks are per-user and all four browser projects run as the same e2e
user, so a shared article made the bookmark specs race each other: one
project saved it while another was asserting it was still unsaved. That is
why they failed on Firefox and mobile but never on Desktop Chrome, which
happened to get there first. saved.spec.ts already ran serially, but serial
mode only orders tests within a project, not across them.
Each project now creates and cleans up its own article. Reproduced the
failure locally across all four projects, and all 52 tests in those two
files pass afterwards. The /saved assertion no longer needs its "or the
empty state" escape hatch either, since nothing else can unbookmark it.
Also gives the discussion editor's submit button a data-testid. "Reply" is
the label of both the editor's submit button and every comment's
expand-reply button, so the notification spec was picking it out with
.last() — ambiguous as soon as a comment has nested children. That spec
still fails intermittently for a separate reason (its reply lands as a
top-level comment, so the server correctly raises "commented on your post"
rather than "replied to your comment"); it retries green and is left for a
follow-up rather than papered over here.
* test(e2e): check the card's content link, not its first anchor
The routeless-/[id] guard took the FIRST anchor inside a content card, but
that is the author handle ("/{username}") — a real page that nonetheless
reads as a bare single-segment path to the shape check, so the test failed
with 'href "/e2e-test-user-one-111" looks like a routeless /[id] page'. It
only surfaced on some browsers because which card ranks first varies, and a
source card's handle link ("/s/{slug}") has two segments and slips through.
The card's content link now carries data-testid="content-card-link" and the
test targets that, so it checks the link the guard is actually about.
All 44 tests in the file pass across all four browser projects.
* test(e2e): bypass rate limits, and wait for comments to actually post
Two causes behind the long tail of failures, found once the suite could be
run locally against a throwaway database on the Node version .nvmrc pins.
`discussion-create` allows 10 comments per 10 minutes per user, and the
suite drives all four browser projects through the same two seeded users —
so later projects were silently throttled, their comments never reached the
database, and the tests that depend on them failed. Rate limiting now has an
explicit `RATE_LIMIT_DISABLED` bypass that the e2e scripts set. It is
guarded on that one env var and documented as test-only.
The notification specs asserted a comment had posted with
`getByText(commentText)`, which also matches the editor's own textarea — so
it resolved the instant `fill()` ran, before the request had been sent. The
test then switched users mid-flight, and the comment landed under whoever
was authenticated by the time it went out: the "user one comments, user two
replies" scenario ended up with both comments written by user two, replying
to itself, so no reply notification was ever generated. They now wait for
the create response and assert on a rendered `section.group/comment`.
Local suite across all four browser projects, fresh database: 518 passed,
2 failed — both Mobile Safari, a publish redirect and a dedupe toast — down
from 19 failures.1 parent 33771d3 commit ca00645
26 files changed
Lines changed: 755 additions & 369 deletions
File tree
- app
- (admin)/admin/moderation
- preview/[postId]
- (app)
- [username]/[slug]
- d/[slug]
- components
- ContentDetail
- Discussion
- DiscussionEditor
- UnifiedContentCard
- e2e
- utils
- server
- api
- router
- lib
- utils
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
40 | 40 | | |
41 | 41 | | |
42 | 42 | | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
76 | 58 | | |
77 | 59 | | |
78 | 60 | | |
| |||
310 | 292 | | |
311 | 293 | | |
312 | 294 | | |
313 | | - | |
| 295 | + | |
314 | 296 | | |
315 | 297 | | |
316 | 298 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
32 | 32 | | |
33 | 33 | | |
34 | 34 | | |
35 | | - | |
36 | 35 | | |
37 | 36 | | |
38 | 37 | | |
| |||
42 | 41 | | |
43 | 42 | | |
44 | 43 | | |
45 | | - | |
| 44 | + | |
46 | 45 | | |
47 | 46 | | |
48 | 47 | | |
| |||
363 | 362 | | |
364 | 363 | | |
365 | 364 | | |
366 | | - | |
367 | | - | |
368 | | - | |
369 | | - | |
370 | | - | |
371 | | - | |
| 365 | + | |
372 | 366 | | |
373 | 367 | | |
374 | 368 | | |
| |||
525 | 519 | | |
526 | 520 | | |
527 | 521 | | |
528 | | - | |
529 | | - | |
530 | | - | |
531 | | - | |
532 | | - | |
533 | | - | |
| 522 | + | |
534 | 523 | | |
535 | 524 | | |
536 | 525 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
27 | 27 | | |
28 | 28 | | |
29 | 29 | | |
30 | | - | |
31 | 30 | | |
32 | 31 | | |
33 | 32 | | |
| |||
68 | 67 | | |
69 | 68 | | |
70 | 69 | | |
71 | | - | |
| 70 | + | |
72 | 71 | | |
73 | 72 | | |
74 | 73 | | |
| |||
147 | 146 | | |
148 | 147 | | |
149 | 148 | | |
150 | | - | |
151 | | - | |
152 | | - | |
153 | | - | |
154 | | - | |
| 149 | + | |
155 | 150 | | |
156 | 151 | | |
157 | 152 | | |
| |||
201 | 196 | | |
202 | 197 | | |
203 | 198 | | |
204 | | - | |
205 | | - | |
206 | | - | |
207 | | - | |
208 | | - | |
| 199 | + | |
209 | 200 | | |
210 | 201 | | |
211 | 202 | | |
| |||
0 commit comments