Repository navigation
STU-386: Restyle magic-link sign-in and preserve return-to-task #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,11 @@ const STRIPE_PRICE_IDS = { | |
| } | ||
|
|
||
| const AUTH_SESSION_STORAGE_KEY = 'ccc_auth_session' | ||
| // Where the user asked to sign in from (composer, plans, billing…), so a | ||
| // magic-link round trip — which reloads the page with no memory of the DOM — | ||
| // can return them to that surface instead of always landing on home. | ||
| const SIGNIN_RETURN_STORAGE_KEY = 'ccc_signin_return' | ||
| const SIGNIN_RETURN_TTL_MS = 30 * 60 * 1000 | ||
|
|
||
| const proGateAttachedSet = new WeakSet() | ||
|
|
||
|
|
@@ -4534,8 +4539,19 @@ async function initializeAuth() { | |
| try { | ||
| const { email, sessionToken } = await verifyMagicLink(magicToken) | ||
| saveSession(email, sessionToken) | ||
| closeSignInModal() | ||
| // Land back on whichever surface (composer, plans, billing) the user | ||
| // asked to sign in from, unless the link itself already carried a hash. | ||
| const returnHash = consumeSignInReturnSurface() | ||
| if (returnHash && !window.location.hash) window.location.hash = returnHash | ||
| } catch (err) { | ||
| showToast(err.message || 'Sign-in link invalid or expired.', 'error') | ||
| consumeSignInReturnSurface() | ||
| const message = err.message || 'Sign-in link invalid or expired.' | ||
| showToast(message, 'error') | ||
| // Surface the failure inline too, with the retry control right there, | ||
| // instead of leaving the user to hunt for a way back in. | ||
| openSignInModal() | ||
|
Comment on lines
+4548
to
+4553
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When verification fails, Greptile automatically discovered a related ticket stating that expired-link retry must preserve the initiating surface, which informed this comment. Source Used: Linear — Restyle magic-link sign-in and preserve return-to-task behavior Prompt To Fix With AIThis is a comment left during a code review.
Path: app.js
Line: 4278-4283
Comment:
**Retry loses return surface**
When verification fails, `consumeSignInReturnSurface()` deletes the initiating surface before `openSignInModal()` records the callback page’s empty hash as `#home`. If a user started on Plans or Account, follows an expired link, and then successfully retries from the reopened modal, the new link returns them Home instead of to their original task. Preserve the stored surface through the inline retry rather than consuming and replacing it.
Greptile automatically discovered a related ticket stating that expired-link retry must preserve the initiating surface, which informed this comment.
**Source Used:** Linear — [Restyle magic-link sign-in and preserve return-to-task behavior](https://linear.app/stuart-gardoll/issue/STU-386/restyle-magic-link-sign-in-and-preserve-return-to-task-behavior)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
||
| setSignInMessage(message, 'error') | ||
|
Comment on lines
+4553
to
+4554
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 First-visit token failure hides retry For first-time free users, Learn moreThe shared modal manager allows only one active modal. Opening a second modal forcibly closes the first through openModal. After token verification fails, these lines open the sign-in modal, but startup continues and showWalkthroughIfNeeded opens onboarding for users without Example: A new free user opens an expired link. The sign-in error appears during Recommended fix: Suppress or defer Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } | ||
| } else { | ||
| const { email, sessionToken } = getStoredSession() | ||
|
|
@@ -4552,9 +4568,43 @@ async function initializeAuth() { | |
| updateAuthUI() | ||
| } | ||
|
|
||
| // Records which surface (home/composer, plans, or account/billing) the user | ||
| // was on when they opened the sign-in modal, so a magic-link click — which | ||
| // lands as a fresh page load — can return them there. Keyed by URL hash, | ||
| // which is how switchView() already tracks the current surface. | ||
| function rememberSignInReturnSurface() { | ||
| try { | ||
| localStorage.setItem(SIGNIN_RETURN_STORAGE_KEY, JSON.stringify({ | ||
| hash: window.location.hash || '#home', | ||
| ts: Date.now(), | ||
| })) | ||
|
Comment on lines
+4577
to
+4580
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Concurrent sign-ins swap return destinations Opening Learn more
Example: Alice opens sign-in from Plans in tab A, then opens sign-in from Account in tab B. Tab B overwrites Recommended fix: Correlate each magic-link request with a unique flow identifier carried through the callback and key return data by that identifier. If the backend contract cannot carry state, use per-tab storage and ensure links return to their originating tab rather than relying on a singleton origin-wide record. Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } catch (err) { | ||
| console.warn('rememberSignInReturnSurface: failed to persist return surface:', err) | ||
| } | ||
| } | ||
|
|
||
| function consumeSignInReturnSurface() { | ||
| try { | ||
| const raw = localStorage.getItem(SIGNIN_RETURN_STORAGE_KEY) | ||
| localStorage.removeItem(SIGNIN_RETURN_STORAGE_KEY) | ||
| if (!raw) return null | ||
| const { hash, ts } = JSON.parse(raw) | ||
| if (!hash || typeof ts !== 'number' || Date.now() - ts > SIGNIN_RETURN_TTL_MS) return null | ||
| return hash | ||
|
Comment on lines
+4591
to
+4593
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } catch (err) { | ||
| console.warn('consumeSignInReturnSurface: failed to read return surface:', err) | ||
| return null | ||
| } | ||
| } | ||
|
|
||
| function openSignInModal() { | ||
| const modal = document.getElementById('signin-modal') | ||
| if (modal) openModal(modal) | ||
| if (!modal) return | ||
| rememberSignInReturnSurface() | ||
| const input = document.getElementById('signin-email-input') | ||
| input?.removeAttribute('aria-invalid') | ||
| setSignInMessage('') | ||
| openModal(modal) | ||
| } | ||
|
|
||
| function closeSignInModal(event) { | ||
|
|
@@ -4563,36 +4613,57 @@ function closeSignInModal(event) { | |
| if (modal) closeModal(modal) | ||
| } | ||
|
|
||
| function setSignInMessage(text, variant) { | ||
| const msg = document.getElementById('signin-message') | ||
| if (!msg) return | ||
| msg.textContent = text || '' | ||
| msg.classList.remove('signin-message-success', 'signin-message-error') | ||
| if (variant) msg.classList.add(`signin-message-${variant}`) | ||
| } | ||
|
|
||
| async function handleMagicLinkRequest() { | ||
| const input = document.getElementById('signin-email-input') | ||
| const btn = document.getElementById('signin-submit-btn') | ||
| const msg = document.getElementById('signin-message') | ||
| const email = trimEmail(input?.value) | ||
|
|
||
| const emailRegex = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/ | ||
| if (!email || !emailRegex.test(email) || email.length > 254) { | ||
| if (msg) msg.textContent = 'Please enter a valid email address.' | ||
| input?.setAttribute('aria-invalid', 'true') | ||
| setSignInMessage('Please enter a valid email address.', 'error') | ||
| return | ||
| } | ||
| input?.removeAttribute('aria-invalid') | ||
|
|
||
| if (btn) { btn.disabled = true; btn.textContent = 'Sending…' } | ||
|
|
||
| // Explain the rule for known providers, but do not block submission here. | ||
| // Existing accounts on plus-tagged addresses are still allowed to recover | ||
| // because the server performs the authoritative check. | ||
| const plusAliasHint = explainPlusAliasRule(email) | ||
| if (msg && plusAliasHint) msg.textContent = plusAliasHint | ||
| if (plusAliasHint) setSignInMessage(plusAliasHint, 'error') | ||
|
|
||
| try { | ||
| const data = await sendMagicLink(email) | ||
| if (input && data?.code !== PLUS_ALIAS_REJECTED_CODE) input.value = '' | ||
| if (msg) msg.textContent = getMagicLinkResultMessage(data, email) | ||
| if (btn) btn.textContent = data?.code === PLUS_ALIAS_REJECTED_CODE ? 'Send Link' : 'Sent!' | ||
| if (btn && data?.code === PLUS_ALIAS_REJECTED_CODE) btn.disabled = false | ||
| const isAliasRejected = data?.code === PLUS_ALIAS_REJECTED_CODE | ||
| if (input && !isAliasRejected) input.value = '' | ||
| input?.setAttribute('aria-invalid', String(isAliasRejected)) | ||
| setSignInMessage(getMagicLinkResultMessage(data, email), isAliasRejected ? 'error' : 'success') | ||
| // A successful send must stay retryable — the user may want to resend to | ||
| // a different address, or send another link if the first one expires — | ||
| // so the button is re-enabled either way, never left permanently disabled. | ||
| // "Sent!" is shown briefly for confirmation, then reverts so the control | ||
| // clearly reads as usable again. | ||
| if (btn) { | ||
| btn.disabled = false | ||
| btn.textContent = isAliasRejected ? 'Send Sign-in Link' : 'Sent!' | ||
| if (!isAliasRejected) { | ||
| setTimeout(() => { if (btn.textContent === 'Sent!') btn.textContent = 'Send Sign-in Link' }, 2500) | ||
| } | ||
| } | ||
| } catch (err) { | ||
| console.error('handleMagicLinkRequest: sendMagicLink failed', { email, err }) | ||
| if (msg) msg.textContent = 'Something went wrong. Please try again.' | ||
| if (btn) { btn.disabled = false; btn.textContent = 'Send Link' } | ||
| setSignInMessage('Something went wrong. Please try again.', 'error') | ||
| if (btn) { btn.disabled = false; btn.textContent = 'Send Sign-in Link' } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,198 @@ | ||
| import { test, expect } from "@playwright/test"; | ||
| import { | ||
| applyDefaultRoutes, | ||
| guestIdentity, | ||
| freeSubscription, | ||
| professionalSubscription, | ||
| signedInSession, | ||
| magicLinkSent, | ||
| magicLinkAliasRejected, | ||
| magicLinkSendFailure, | ||
| magicLinkVerifyFailure, | ||
| ENDPOINTS, | ||
| } from "./fixtures/apiFixtures.js"; | ||
|
|
||
| /** | ||
| * Restyled magic-link sign-in (STU-386): validation, sending, sent, | ||
| * expired-link, failed-request, retry, alias-policy and return-to-task | ||
| * behavior. Every auth endpoint is intercepted with deterministic fixtures; | ||
| * no real email is sent and no billing/checkout call completes. | ||
| */ | ||
| test.describe("Magic-link sign-in", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.addInitScript(() => { | ||
| localStorage.setItem("hasSeenWalkthrough", "true"); | ||
| }); | ||
| }); | ||
|
|
||
| test("rejects an invalid email inline without calling the backend", async ({ page }) => { | ||
| let sendCalled = false; | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.getSubscription]: freeSubscription(), | ||
| }); | ||
| await page.route(ENDPOINTS.authSendMagicLink, async (route) => { | ||
| sendCalled = true; | ||
| await route.fulfill(magicLinkSent()); | ||
| }); | ||
|
|
||
| await page.goto("/#account"); | ||
| await page.locator("#auth-signedout button", { hasText: "Sign In" }).click(); | ||
|
|
||
| const modal = page.locator("#signin-modal"); | ||
| await expect(modal).toHaveClass(/open/); | ||
|
|
||
| const input = page.locator("#signin-email-input"); | ||
| await input.fill("not-an-email"); | ||
| await page.locator("#signin-submit-btn").click(); | ||
|
|
||
| await expect(page.locator("#signin-message")).toContainText("valid email"); | ||
| await expect(input).toHaveAttribute("aria-invalid", "true"); | ||
| expect(sendCalled).toBe(false); | ||
| }); | ||
|
|
||
| test("sends a link, then leaves the submit control usable to resend", async ({ page }) => { | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.getSubscription]: freeSubscription(), | ||
| [ENDPOINTS.authSendMagicLink]: magicLinkSent(), | ||
| }); | ||
|
|
||
| await page.goto("/#account"); | ||
| await page.locator("#auth-signedout button", { hasText: "Sign In" }).click(); | ||
|
|
||
| const input = page.locator("#signin-email-input"); | ||
| const submit = page.locator("#signin-submit-btn"); | ||
| await input.fill("person@example.com"); | ||
| await submit.click(); | ||
|
|
||
| await expect(page.locator("#signin-message")).toContainText( | ||
| "Check your email", | ||
| ); | ||
| // Acceptance criterion: retry/send-again must stay usable — the button | ||
| // is never left permanently disabled after a successful request. | ||
| await expect(submit).toBeEnabled(); | ||
|
|
||
| // Resend to a second address works without reloading or reopening the modal. | ||
| await input.fill("person-two@example.com"); | ||
| await submit.click(); | ||
| await expect(page.locator("#signin-message")).toContainText( | ||
| "person-two@example.com", | ||
| ); | ||
| await expect(submit).toBeEnabled(); | ||
| }); | ||
|
|
||
| test("shows an inline alias-policy error and keeps normal addresses working", async ({ page }) => { | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.getSubscription]: freeSubscription(), | ||
| [ENDPOINTS.authSendMagicLink]: magicLinkAliasRejected("user+tag@gmail.com"), | ||
| }); | ||
|
|
||
| await page.goto("/#account"); | ||
| await page.locator("#auth-signedout button", { hasText: "Sign In" }).click(); | ||
|
|
||
| const input = page.locator("#signin-email-input"); | ||
| const submit = page.locator("#signin-submit-btn"); | ||
| await input.fill("user+tag@gmail.com"); | ||
| await submit.click(); | ||
|
|
||
| // STU-149's alias-policy error is surfaced inline, not swallowed. | ||
| await expect(page.locator("#signin-message")).toContainText("primary email address"); | ||
| await expect(input).toHaveAttribute("aria-invalid", "true"); | ||
| await expect(submit).toBeEnabled(); | ||
| // The address is preserved so the user can see what was rejected. | ||
| await expect(input).toHaveValue("user+tag@gmail.com"); | ||
| }); | ||
|
Comment on lines
+85
to
+106
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| test("recovers from a failed send, and retry succeeds", async ({ page }) => { | ||
| let attempt = 0; | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.getSubscription]: freeSubscription(), | ||
| }); | ||
| await page.route(ENDPOINTS.authSendMagicLink, async (route) => { | ||
| attempt += 1; | ||
| const fixture = attempt === 1 ? magicLinkSendFailure() : magicLinkSent(); | ||
| await route.fulfill(fixture); | ||
| }); | ||
|
|
||
| await page.goto("/#account"); | ||
| await page.locator("#auth-signedout button", { hasText: "Sign In" }).click(); | ||
|
|
||
| const input = page.locator("#signin-email-input"); | ||
| const submit = page.locator("#signin-submit-btn"); | ||
| await input.fill("person@example.com"); | ||
| await submit.click(); | ||
|
|
||
| await expect(page.locator("#signin-message")).toContainText("Something went wrong"); | ||
| await expect(submit).toBeEnabled(); | ||
|
|
||
| // Retry with the same control, no reload required. | ||
| await submit.click(); | ||
| await expect(page.locator("#signin-message")).toContainText("Check your email"); | ||
| expect(attempt).toBe(2); | ||
| }); | ||
|
|
||
| test("an expired or invalid token surfaces an inline retry in the sign-in modal", async ({ page }) => { | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.getSubscription]: freeSubscription(), | ||
| [ENDPOINTS.authVerifyMagicLink]: magicLinkVerifyFailure(), | ||
| }); | ||
|
|
||
| await page.goto("/?token=expired-token-0001"); | ||
|
|
||
| const modal = page.locator("#signin-modal"); | ||
| await expect(modal).toHaveClass(/open/); | ||
| await expect(page.locator("#signin-message")).toContainText("invalid or expired"); | ||
|
|
||
| // The token param is stripped so retrying never resubmits the dead token. | ||
| await expect(page).toHaveURL(/^(?!.*token=).*$/); | ||
| }); | ||
|
|
||
| test("a successful token verification refreshes the session and subscription", async ({ page }) => { | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.authVerifyMagicLink]: signedInSession(), | ||
| [ENDPOINTS.getSubscription]: professionalSubscription(), | ||
| }); | ||
|
|
||
| await page.goto("/?token=good-token-0001#account"); | ||
|
|
||
| // Session refresh completes and the account surface reflects it — no | ||
| // auto-purchase or auto-deploy is triggered by this flow. | ||
| await expect(page.locator("#auth-signedin")).toBeVisible(); | ||
| await expect(page.locator("#auth-user-email")).toContainText("test@example.com"); | ||
| await expect(page.locator("#subscription-tier-badge")).toContainText("Pro"); | ||
| await expect(page.locator("#signin-modal")).not.toHaveClass(/open/); | ||
| }); | ||
|
|
||
| test("signing in from the Plans surface returns to Plans after verification", async ({ page }) => { | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.getSubscription]: freeSubscription(), | ||
| }); | ||
|
|
||
| await page.goto("/#plans"); | ||
| await expect(page.locator("#plans-view")).toBeVisible(); | ||
|
|
||
| // Subscribing while signed out opens the sign-in modal instead of checkout. | ||
| await page.locator("#plans-checkout-btn-professional").click(); | ||
| await expect(page.locator("#signin-modal")).toHaveClass(/open/); | ||
|
|
||
| // The magic-link round trip is a fresh navigation with no memory of the | ||
| // DOM — the return surface must survive via storage, not page state. | ||
| await applyDefaultRoutes(page, { | ||
| [ENDPOINTS.identity]: guestIdentity(), | ||
| [ENDPOINTS.authVerifyMagicLink]: signedInSession(), | ||
| [ENDPOINTS.getSubscription]: professionalSubscription(), | ||
| }); | ||
| await page.goto("/?token=good-token-0002"); | ||
|
|
||
| await expect(page).toHaveURL(/#plans$/); | ||
| await expect(page.locator("#plans-view")).toBeVisible(); | ||
| // Returning to Plans must not auto-purchase: no checkout redirect happens. | ||
| await expect(page).not.toHaveURL(/checkout\.stripe\.com/); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Expired-link retry loses return task
After a token fails,
consumeSignInReturnSurface()deletes the initiating surface.openSignInModal()then records home, so a replacement link cannot return to the task.Learn more
A return surface is consumed by reading and immediately deleting its storage entry. The failure path discards that value, then calls openSignInModal, which stores the callback page's hash or
#home. A successful replacement link therefore restores the callback location instead of the surface where sign-in began.Example: A user starts sign-in from Plans and opens an expired link without a hash. The failure consumes
#plans, then retry records#home. After requesting and opening a new link, the app lands on Home instead of Plans.Recommended fix: Preserve a valid return hash across verification failure and inline retry. Either leave the entry unconsumed on failure, or pass the consumed hash into the retry modal and write it back without replacing it with the callback location.
Was this helpful? React with 👍 or 👎 to provide feedback.