Skip to content

fix(comments): return readers to the post after comment sign-in#154

Merged
axross merged 1 commit into
mainfrom
claude/clerk-signin-redirect-n84kp3
Jul 22, 2026
Merged

fix(comments): return readers to the post after comment sign-in#154
axross merged 1 commit into
mainfrom
claude/clerk-signin-redirect-n84kp3

Conversation

@axross

@axross axross commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Summary

The comment composer's only sign-in surface — the "Sign in with GitHub" button shown under a post to signed-out readers — used Clerk's SignInButton (mode="modal") with no redirect props. In modal mode Clerk never records a redirect_url in the URL, and GitHub OAuth is a full-page round-trip, so on return Clerk fell back to its default post-auth destination of / — sending readers to the home page regardless of which post they were reading.

This pins the sign-in and sign-up redirects to the current path, captured with usePathname() (an existing pattern in app/(app)/_components/page-view-tracking.tsx), so a reader returns to the page they signed in from. forceRedirectUrl covers the sign-in path and signUpForceRedirectUrl covers first-time GitHub account creation via the same button. Because the redirect is derived from wherever the button renders, it also works for a sign-in begun from any other page — it isn't hard-coded to posts.

The change is one component, +12/−1. It produces no DOM/appearance change: Clerk destructures these props to build its OAuth flow options and never spreads them onto the rendered button.

Current Clerk docs consulted (July 2026): the SignInButton reference and "Customize your redirect URLs".

Related issues

Closes #153

Verification

Commands run in this checkout:

  • npm run format — clean, no fixes applied.
  • npm run lintbiome check clean (only a pre-existing, unrelated warning in scripts/prune-preview-blobs.mjs; none in the changed file).
  • npm run build — succeeded end-to-end (compile, type-check, prerender of all routes) after applying local Payload migrations.
  • npm run test:unit — 186/186 across 23 suites.
  • ⚠️ npm run test:e2ecould not run in this checkout: the environment has Playwright browser build 1194 but @playwright/test ^1.61.1 pins build 1228, and this environment is configured not to run playwright install. The changed code path is Clerk-gated (Clerk is disabled here) and produces no DOM change, so the suite could not exercise it regardless. The per-PR preview-deploy pipeline runs the e2e suite against a Clerk-configured preview as the gate.

Acceptance criteria

  • met — Signing in from a post's comment composer returns the reader to that post page, not /. Verified against the installed @clerk/nextjs types: forceRedirectUrl takes precedence over Clerk's default / fallback, and usePathname() resolves to the post's own path (no locale-prefix routing in play).
  • 👀 needs manual/preview check — The return-to-post behavior also holds for a first-time GitHub sign-up (account creation) via the same button. Statically correct — signUpForceRedirectUrl is wired per Clerk's documented API — but whether Clerk classifies the OAuth callback as a sign-up completion is runtime behavior only a live check can confirm. On the Clerk-configured preview, click sign-in with a GitHub account that has never signed into the app, complete account creation, and confirm the browser lands back on the post URL, not /. (The independent review restated this as its one Important finding, per the repo review policy that flags any criterion unconfirmable from the diff.)
  • met — The composer's visual appearance, copy, and unconfigured/disabled behavior are unchanged. Clerk consumes both props before cloning the child element, so they never reach the DOM; the call site (comments.tsx) and its Clerk-unavailable short-circuit are untouched.
  • metusePathname() is called unconditionally at the top of the component (no Rules-of-Hooks violation), and the change is confined to comment-composer.tsx. Corroborated by the useHookAtTopLevel Biome rule in the clean lint pass.
  • metnpm run format, npm run lint, and npm run build pass (see above).

Notes for reviewers

Start with the single changed file, app/(app)/posts/[slug]/_components/comments/comment-composer.tsx. The behavioral criteria (the real OAuth redirect) aren't observable in this checkout — Clerk is disabled locally and the redirect URL is consumed by Clerk's internal handler rather than rendered — so the honest verification path for those is the Clerk-configured preview deploy. No new automated test is added: the repo's Jest suite is pure-logic only (no component/jsdom/Clerk-mock harness), and the redirect isn't DOM-observable for Playwright; the sign-in scenarios in e2e/scenarios.md are may-priority and Clerk-token-gated.

The comment composer's `<SignInButton mode="modal">` passed no redirect
props, so after the GitHub OAuth full-page round-trip Clerk fell back to
its default destination of `/` — sending readers to the home page instead
of back to the post they were commenting on.

Pin the sign-in and sign-up redirects to the current path via
`usePathname()`, so a reader returns to the page they signed in from.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BDuq4Dh5vNvggsSmLfXHcr
@axross

axross commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

@claude review


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

🚀 Preview deployment

https://btnopen-pr-154.vercel.app

Deployed ef92fe3. Served by an isolated Turso database (preview-pr-154) seeded from repository fixtures — no production data — with media in a dedicated preview store; both are destroyed automatically when this pull request closes. This URL is stable for the pull request and always serves the latest preview.

Comment on lines +61 to +65
<SignInButton
mode="modal"
forceRedirectUrl={pathname}
signUpForceRedirectUrl={pathname}
>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important — per REVIEW.md's mandatory acceptance-criteria check against #153: criterion "the return-to-post behavior also holds for a first-time GitHub sign-up" isn't confirmable from this diff. signUpForceRedirectUrl is wired correctly per Clerk's documented API, but whether Clerk actually classifies the GitHub OAuth callback as a sign-up completion (vs. a plain sign-in, which would use forceRedirectUrl instead) is live provider behavior, not something static review of the diff can prove.

The PR description already tracks this as an open check ("needs manual/preview check"), so this is a required restatement of that gap per the repo's review policy rather than a newly discovered issue — please confirm it on the Clerk-configured preview before merging.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Code review

1 important, 0 nits.

Important

  • Acceptance criterion 2 from Return readers to the post they were on after comment sign-in (instead of /) #153 ("the return-to-post behavior also holds for a first-time GitHub sign-up") is not confirmable from the diff — see the inline comment on comment-composer.tsx. signUpForceRedirectUrl is wired correctly per Clerk's documented API, but whether Clerk classifies the OAuth callback as a sign-up completion is live provider behavior only a Clerk-configured preview check can confirm. The PR description already tracks this as an open item, so this restates a policy-mandated gap rather than a new finding.

Mandatory checks (per REVIEW.md)

  • Skill conformance — reviewed against the AGENTS.md skill index (React component, development, maintainable-code, and application-security-requirements guidelines): no violations. usePathname() is called unconditionally ahead of the component's early returns (no Rules-of-Hooks issue), the change stays confined to the single file, and forceRedirectUrl / signUpForceRedirectUrl are real SignInButton props — checked directly against @clerk/nextjs@7.5.19's published type declarations — that accept the string usePathname() returns.
  • Acceptance criteria against Return readers to the post they were on after comment sign-in (instead of /) #153 — closes the linked issue. 4 of 5 criteria are confirmable from the diff plus the verified Clerk type contract (redirect precedence over Clerk's / default, unchanged appearance/DOM, unconditional hook placement confined to one file, and format/lint/build). The 5th (sign-up redirect) is the Important finding above.

@axross
axross marked this pull request as ready for review July 22, 2026 07:33
@axross
axross merged commit b34cf1f into main Jul 22, 2026
5 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🚀 Preview deployment

Torn down — the isolated Turso database (preview-pr-154) and this pull request's preview media were destroyed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Return readers to the post they were on after comment sign-in (instead of /)

2 participants