🐛 Fixed newsletter reply-to verification links failing after signin#27660
🐛 Fixed newsletter reply-to verification links failing after signin#27660
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a Playwright e2e test that navigates to a Ghost admin React route with a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/tests/admin/signin.test.ts (2)
39-39: 💤 Low valueTest name doesn't follow the required
'what is tested - expected outcome'format.The guideline requires a hyphen-separated format. Consider:
- test('query params on a deep link survive signin redirect', async ({page, ghostAccountOwner}) => { + test('deep link query params - survive signin redirect', async ({page, ghostAccountOwner}) => {As per coding guidelines: "Test names should follow format: 'what is tested - expected outcome' in lowercase."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/signin.test.ts` at line 39, Rename the test declaration in signin.test.ts so its description follows the required "what is tested - expected outcome" lowercase hyphen-separated format; for example, change the test name used in the test(...) call from "query params on a deep link survive signin redirect" to "deep link query params - survive signin redirect" (update the test(...) first argument where the test is declared).
40-45: 💤 Low valueTrim verbose comments — prefer clear names over explanatory prose.
The 6-line preamble (lines 40–45) and the 3-line inline note (lines 55–57) add noise that the test name and assertion message can convey. A single short comment anchoring the ticket reference, or none at all, aligns better with the guideline.
✂️ Suggested trim
test('deep link query params - survive signin redirect', async ({page, ghostAccountOwner}) => { - // Newsletter reply-to verification emails point at - // /settings/newsletters/?verifyEmail=<token>. If the user clicks the - // link in a browser that isn't signed in, the param needs to survive - // the signin round-trip, otherwise the verify-on-mount handler in - // newsletters.tsx no-ops and the customer thinks their reply-to - // address didn't save (ONC-1618 / ONC-1642). await logout(page); await page.goto('/ghost/#/settings/newsletters/?verifyEmail=fake-token-xyz'); const loginPage = new LoginPage(page); await expect(loginPage.signInButton).toBeVisible(); await loginPage.signIn(ghostAccountOwner.email, ghostAccountOwner.password); - // The error modal is the signal that the React verify handler ran: - // it only renders when newsletters.tsx attempted to redeem a token - // and the API rejected it (expected, since the token is fake). await expect(page.getByRole('heading', {name: 'Error verifying email address'})).toBeVisible(); expect(page.url()).toContain('verifyEmail=fake-token-xyz'); });As per coding guidelines: "Prefer less comments and giving things clear names in E2E tests."
Also applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/signin.test.ts` around lines 40 - 45, Remove the verbose 6-line preamble and the 3-line inline note around the newsletter reply-to verification in the signin E2E test; instead shorten to a single brief comment with the ticket reference (e.g., "ONC-1618") or remove it entirely and rely on a clear test name and assertion message in the test that verifies the /settings/newsletters/?verifyEmail=<token> round-trip; apply the same trimming to the comment block at lines 55–57 so the test remains readable and self-explanatory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/admin/signin.test.ts`:
- Line 39: Rename the test declaration in signin.test.ts so its description
follows the required "what is tested - expected outcome" lowercase
hyphen-separated format; for example, change the test name used in the test(...)
call from "query params on a deep link survive signin redirect" to "deep link
query params - survive signin redirect" (update the test(...) first argument
where the test is declared).
- Around line 40-45: Remove the verbose 6-line preamble and the 3-line inline
note around the newsletter reply-to verification in the signin E2E test; instead
shorten to a single brief comment with the ticket reference (e.g., "ONC-1618")
or remove it entirely and rely on a clear test name and assertion message in the
test that verifies the /settings/newsletters/?verifyEmail=<token> round-trip;
apply the same trimming to the comment block at lines 55–57 so the test remains
readable and self-explanatory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ef12b23-2915-4d72-b4e0-1aa64c91e3de
📒 Files selected for processing (2)
e2e/tests/admin/signin.test.tsghost/admin/app/services/session.js
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
Looks good! Thank you for fixing this. Tested and verified locally as well.
ref https://linear.app/ghost/issue/ONC-1658/ - when a customer clicks a newsletter reply-to verification email in a browser that isn't signed in to admin, the link sends them through the signin redirect introduced in #27316 — but the post-signin replay used router.transitionTo, which drops query params not declared on the resolved route's controller - react-fallback is a wildcard route with no queryParams config, so the ?verifyEmail=<token> param was silently stripped on replay; the React verify-on-mount handler in newsletters.tsx then no-ops because the token is missing, and the customer thinks the address didn't save (root cause for ONC-1618 and ONC-1642) - swap to windowProxy.replaceLocation, which is already the auth-flow primitive used in authenticated.js and application.js — the hard navigation feeds the URL straight to the browser and bypasses Ember's URL-rebuilding entirely - e2e covers the round-trip; verified to fail on unfixed code
rob-ghost
force-pushed
the
steve-onc-1658-preserve-query-params-after-signin-redirect
branch
from
d58e24a to
a62dfc5
Compare
2 participants
ref https://linear.app/ghost/issue/ONC-1658/
Summary
When a customer clicks a newsletter reply-to verification email in a browser that isn't signed in to admin, the link silently no-ops — they have to click it a second time for the address to actually save. Two customers hit this in the past few weeks (ONC-1618, ONC-1642).
The signin-redirect flow added in #27316 stores the original URL in
sessionStorageand replays it viarouter.transitionTo(redirectUrl)after auth. Butreact-fallbackis a wildcard route with no controller-declaredqueryParams, so Ember strips?verifyEmail=<token>on the replay. The verify-on-mountuseEffectinnewsletters.tsxthen short-circuits because the token is missing.This swaps the soft
transitionToforwindowProxy.replaceLocation— already the auth-flow primitive used inauthenticated.js:20andapplication.js:113. A hard navigation feeds the URL straight to the browser and sidesteps Ember's URL-rebuilding entirely, so the query param survives byte-for-byte.Test plan — base case
pnpm devrunning, sign out of admin.http://localhost:2368/ghost/#/settings/newsletters/?verifyEmail=fake-token-xyz?verifyEmail=fake-token-xyz, and within a moment an "Error verifying email address" / "Verification link has expired" modal appears (because the token is fake — that's success: it means the round-trip worked and the API was actually called)./settings/newsletters/with no query param, no modal — the silent no-op customers reported.Test plan — regression checks
/ghost/#/tags→ signin → land on Tags. (covered by existing e2e)/ghost/#/posts→ signin → land on Posts. (covered by existing e2e)/ghost/(nosessionStorageredirect): land on home/dashboard as before.http://localhost:8025), open it in an incognito window, sign in, confirm the "Reply-to address verified" success modal.Automated coverage
Added an e2e in
e2e/tests/admin/signin.test.tsthat round-trips a?verifyEmail=deep link through signin and asserts both that the URL retains the param and that the verify error modal renders (which only happens ifnewsletters.tsxactually called the API). Verified to fail on the unfixed code.