fix(discussion): stop comments jumping around when you vote - #1341
Merged
Conversation
Voting refetched the thread, and "Top" re-sorts by score, so liking a comment yanked it up the page mid-read. A successful vote no longer refetches — VoteControl already updates optimistically — and each comment's sort score is now frozen the first time it is seen, so later data refreshes cannot reshuffle a thread somebody is reading. Ordering still updates, just on the next load, or immediately if the reader re-picks the sort. Two things fall out of that: - The global "a vote is in flight" guard is gone. It blocked votes on every other comment while one was pending, and swallowed the click after VoteControl had already toggled itself, leaving the UI showing a vote that was never sent. - With clicks no longer serialised, the vote mutation's read-then-write could race itself: two overlapping requests both saw "no vote" and both inserted, tripping comment_votes_comment_id_user_id_key. It is now a single delete-by-key or upsert, so whichever request lands last wins. Failed votes still resync: the refetch now completes before the controls are remounted, otherwise they would reseed from the pre-vote cache and strand earlier successful votes showing their old counts.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughThe PR changes discussion voting to use atomic server writes and client-side error recovery. It also stabilizes top-score ordering by snapshotting scores and applying creation-time tie-breaking. ChangesDiscussion voting and ordering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VoteControl
participant DiscussionArea
participant DiscussionAPI
participant VoteDatabase
VoteControl->>DiscussionArea: submit vote
DiscussionArea->>DiscussionAPI: invoke vote mutation
DiscussionAPI->>VoteDatabase: delete or upsert vote
VoteDatabase-->>DiscussionAPI: return mutation result
DiscussionAPI-->>DiscussionArea: return success or error
alt vote error
DiscussionArea->>DiscussionAPI: refetch discussion
DiscussionArea->>VoteControl: increment reset key
VoteControl-->>DiscussionArea: remount with server state
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
NiallJoeMaher
had a problem deploying
to
production
August 12, 2026 06:51 — with 
GitHub Actions
Failure
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/Discussion/DiscussionArea.tsx (2)
190-212: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
sortDiscussionsreadssortScoresbut is not memoized per subtree.
generateDiscussionscallssortDiscussionsonce per node and again for each child list at line 481. Each call copies and sorts the array. For a deep thread this repeats work on every render, including every keystroke in an open editor, becauseshowCommentBoxId,editContent, andvoteResetKeyall live in this component.The current thread sizes probably make this acceptable. If threads grow, memoize the sorted tree once per
discussions/sortOrder/sortScoreschange instead of sorting during render.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/Discussion/DiscussionArea.tsx` around lines 190 - 212, Memoize the sorted discussion tree so sorting is recomputed only when discussions, sortOrder, or sortScores changes. Update the sortDiscussions/generateDiscussions flow to reuse the memoized result for the root and child lists rather than copying and sorting each subtree during every render.
169-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the snapshot reset out of the render phase.
sortScoresmutatesfrozenSortScores.currentand assignsfrozenForSort.currentwhile rendering. React 19 can start a render, discard it, and render again. If a render that clears and re-captures the snapshot is discarded, the captured scores come from a tree version that React throws away. StrictMode double-invokes the memo, which also runs the reset branch during render.The capture itself is idempotent, so this is unlikely to produce a visible defect today. It is still fragile under concurrent rendering.
A safer shape keys the snapshot by
sortOrderand avoids the reset branch entirely.♻️ Proposed refactor: key the snapshot by sort order
- const frozenSortScores = useRef(new Map<string, number>()); - const frozenForSort = useRef<SortOrder>(sortOrder); + const frozenSortScores = useRef(new Map<SortOrder, Map<string, number>>());const sortScores = useMemo(() => { - // Re-picking a sort is a deliberate "show me the current ranking", so let - // that re-rank from live scores. Passive vote traffic must not. - if (frozenForSort.current !== sortOrder) { - frozenForSort.current = sortOrder; - frozenSortScores.current.clear(); - } - const captured = frozenSortScores.current; + // Re-picking a sort is a deliberate "show me the current ranking", so it + // starts a fresh snapshot. Passive vote traffic reuses the existing one. + let captured = frozenSortScores.current.get(sortOrder); + if (!captured) { + captured = new Map<string, number>(); + frozenSortScores.current.set(sortOrder, captured); + }Note that this variant keeps a snapshot per sort order, so re-picking a previously used sort reuses its old snapshot. If you want re-picking to always re-rank, keep a monotonic sort-selection counter in state and clear the map in an effect instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/Discussion/DiscussionArea.tsx` around lines 169 - 188, Refactor sortScores to avoid mutating frozenForSort.current or frozenSortScores.current during render: key the stored snapshots by sortOrder and reuse the corresponding snapshot without a reset branch. Update the capture logic to populate only that sort order’s snapshot, preserving frozen scores across passive vote updates and allowing React to discard or replay renders safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@components/Discussion/DiscussionArea.tsx`:
- Around line 190-212: Memoize the sorted discussion tree so sorting is
recomputed only when discussions, sortOrder, or sortScores changes. Update the
sortDiscussions/generateDiscussions flow to reuse the memoized result for the
root and child lists rather than copying and sorting each subtree during every
render.
- Around line 169-188: Refactor sortScores to avoid mutating
frozenForSort.current or frozenSortScores.current during render: key the stored
snapshots by sortOrder and reuse the corresponding snapshot without a reset
branch. Update the capture logic to populate only that sort order’s snapshot,
preserving frozen scores across passive vote updates and allowing React to
discard or replay renders safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a7f4140-c54a-4c97-a53a-dfc22bb2e71c
📒 Files selected for processing (2)
components/Discussion/DiscussionArea.tsxserver/api/router/discussion.ts
|
Uh oh! @vercel[bot], the image you shared is missing helpful alt text. Check #1341 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
This was referenced Aug 12, 2026
NiallJoeMaher
added a commit
that referenced
this pull request
Aug 20, 2026
…gin (#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.
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
1 participant
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.
What
Liking a comment made it jump up the thread under your cursor. Voting refetched the discussion, and "Top" re-sorts by score, so the comment you just liked was immediately re-ranked mid-read.
Ordering by votes is still the point — it just shouldn't happen while someone is reading. So:
VoteControlalready updates optimistically, so the count responds instantly and the thread never reflows.The trade-off is deliberate and commented: a tab left open for hours keeps the ranking it loaded with.
Two things that fell out of it
The global in-flight guard is gone.
voteStatus === "pending"blocked voting on every comment while any one vote was in flight, and it swallowed the click afterVoteControlhad already toggled itself — leaving the UI showing a vote that was never sent.So the vote mutation had to become race-safe. With clicks no longer serialised,
discussion.vote's SELECT-then-INSERT could race itself: two overlapping requests both see "no vote" and both insert, trippingcomment_votes_comment_id_user_id_key(a 500), or the delete variant no-ops and leaves a vote the UI doesn't show. It's now a single delete-by-key orinsert … onConflictDoUpdate, so whichever request lands last simply wins. The count triggers are unaffected — a same-value update is a no-op fortr_comment_vote_counts.Failed votes still resync. The refetch now completes before the controls are remounted; bumping the remount key first would reseed them from the pre-vote cache and strand earlier successful votes showing their old counts.
Verified locally
npm run lint,npm run prettier,npm run test:unit(118 passing),npm run build.