-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
Added sniperlink when signing up #25968
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
Conversation
WalkthroughThis pull request introduces a sniper links feature that enables platform-specific email opening capabilities. The changes extend the authentication flows (signin/signup) to capture and propagate sniperLinks data from the API response. A new SniperLinkButton component renders conditional platform-specific links in the magic link page. Supporting utilities (isAndroid, isAndroidChrome) detect the execution environment. The magic-link-page component now conditionally renders sniper link buttons for Android Chrome or defaults to the existing close button. Additionally, the API documentation is updated to reflect the new return structure, and a "Open email" translation key is added across all 60+ supported locale files. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
0e70aea to
a4da768
Compare
ghost/core/core/server/services/members/members-api/controllers/router-controller.js
Outdated
Show resolved
Hide resolved
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.
Pull request overview
This PR adds "sniper links" functionality that provides direct links to email providers (currently Gmail) when users sign up with a Gmail address. After signing up, users see an "Open email" button that takes them directly to their Gmail inbox to find the verification email.
Changes:
- Implements server-side sniper link generation for Gmail, Googlemail, and Google.com addresses
- Updates the Portal UI to display an "Open email" button on the magic link confirmation page when sniper links are available
- Adds Android detection to serve appropriate deep links for mobile users
Reviewed changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
ghost/core/core/server/lib/get-sniperlinks.ts |
New module that generates provider-specific links to open email clients |
ghost/core/core/server/services/members/members-api/controllers/router-controller.js |
Integrates sniper links into the sendMagicLink API response |
ghost/core/core/server/services/members/members-api/members-api.js |
Passes settingsHelpers dependency to router controller |
apps/portal/src/utils/is-android.js |
Utility to detect Android devices for serving appropriate deep links |
apps/portal/src/utils/api.js |
Updates API client to handle sniperLinks in sendMagicLink response |
apps/portal/src/components/common/action-button.js |
Extends ActionButton to render as anchor tag when href is provided |
apps/portal/src/components/pages/magic-link-page.js |
Displays "Open email" button when sniper links are available |
apps/portal/src/actions.js |
Captures and stores sniperLinks from API response during signup |
apps/portal/src/app.js |
Adds sniperLinks to app context |
ghost/core/test/unit/server/lib/get-sniperlinks.test.ts |
Unit tests for sniper link generation |
ghost/core/test/e2e-api/members/send-magic-link.test.js |
E2E tests verifying API returns sniper links |
apps/portal/test/utils/is-android.test.js |
Tests for Android detection logic |
apps/portal/test/signup-flow.test.js |
Integration test verifying sniper link button rendering |
ghost/i18n/locales/*/portal.json |
Adds "Open email" translation key across all locales |
ghost/i18n/locales/context.json |
Adds context entry for "Open email" translation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e4f5151 to
59d3a66
Compare
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ghost/core/core/server/lib/get-sniper-links.ts`:
- Around line 70-83: parseEmailDomain currently returns the raw slice of the
domain which can include uppercase letters or surrounding whitespace and cause
missed provider lookups; update parseEmailDomain to trim() and toLowerCase() the
extracted domain before returning (i.e., after computing atIndex and slicing,
call .trim().toLowerCase() on the result) while preserving the length guard and
other early returns.
In `@ghost/core/test/unit/server/lib/get-sniper-links.test.ts`:
- Line 5: Update the test title string to reflect the actual assertion and
return type: replace the it(...) description currently reading "returns null for
invalid recipient emails" with "returns undefined for invalid recipient emails"
(or similar wording) so it matches the assertion that expects undefined; no code
logic changes are needed in the test body or the tested function.
♻️ Duplicate comments (2)
ghost/i18n/locales/context.json (1)
177-177: Add translator context for the new label.This key is used as a button label in the magic-link confirmation flow; a short description here will help translators pick the right phrasing.
✍️ Suggested context text
- "Open email": "", + "Open email": "Button label on the magic-link confirmation screen that opens the user's email client"apps/portal/src/components/common/action-button.js (1)
105-120: Handle disabled anchors explicitly for accessibility.
disabledisn’t a valid<a>attribute and doesn’t prevent keyboard activation. Consider usingaria-disabled, removinghref(or preventing default), andtabIndex={-1}when disabled.♻️ Suggested adjustment
- const commonProps = { + const commonProps = { className, style: Style.button, - disabled, tabIndex, 'data-test-button': dataTestId }; @@ - if (href) { - return <a {...commonProps} href={href} target={target} rel={rel}>{children}</a>; + if (href) { + const isDisabled = disabled; + return ( + <a + {...commonProps} + href={isDisabled ? undefined : href} + target={target} + rel={rel} + aria-disabled={isDisabled} + tabIndex={isDisabled ? -1 : tabIndex} + onClick={isDisabled ? e => e.preventDefault() : undefined} + > + {children} + </a> + ); } else { return <button {...commonProps} onClick={onClick} type='submit'>{children}</button>; }
🧹 Nitpick comments (2)
apps/portal/src/components/pages/magic-link-page.js (1)
295-304: Guard against non-browser environments when readingnavigator.Directly referencing
navigatorcan throw in SSR/tests; add a safe fallback before callingisAndroid.♻️ Suggested hardening
- const href = isAndroid(navigator) ? sniperLinks.android : sniperLinks.desktop; + const nav = (typeof navigator !== 'undefined') ? navigator : undefined; + const href = nav && isAndroid(nav) ? sniperLinks.android : sniperLinks.desktop;apps/portal/test/signup-flow.test.js (1)
92-103: Guard the mock against missing/undefined email.If
sendMagicLinkis ever invoked without{email}(or withendsWithwill throw. A safe default keeps the test helper resilient to future call-site changes.♻️ Suggested tweak
- ghostApi.member.sendMagicLink = vi.fn(async ({email}) => { - if (email.endsWith('@test-sniper-link.example')) { + ghostApi.member.sendMagicLink = vi.fn(async ({email} = {}) => { + if (email?.endsWith('@test-sniper-link.example')) { return { sniperLinks: { android: 'https://test.example/', desktop: 'https://test.example/' } }; } else { return {}; } });
59d3a66 to
c9cb3cf
Compare
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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ghost/core/test/e2e-api/members/send-magic-link.test.js (1)
263-277: Inconsistent body assertion style.This test still uses
.expectEmptyBody()while other similar tests in this file were updated to useassert.deepEqual(res.body, {}). For signup requests, the response may now includesniperLinksdepending on the email domain, which would cause.expectEmptyBody()to fail.💡 Suggested fix
- await membersAgent.post('/api/send-magic-link') + const res = await membersAgent.post('/api/send-magic-link') .body({ email, emailType: 'signup', urlHistory: [ { path: '/test-path', time: Date.now() } ] }) - .expectEmptyBody() .expectStatus(201); + + // Body may contain sniperLinks depending on email domain + assert.ok(res.body);apps/portal/src/actions.js (1)
160-188: Signup flow doesn't clear sniperLinks for paid plans.When
plan.toLowerCase() !== 'free', the function returns{page: 'loading'}without settingsniperLinks, allowing stale values to persist.🔧 Suggested fix
} else { if (tierId && cadence) { await api.member.checkoutPlan({plan, tierId, cadence, email, name, newsletters, offerId}); } else { ({tierId, cadence} = getProductCadenceFromPrice({site: state?.site, priceId: plan})); await api.member.checkoutPlan({plan, tierId, cadence, email, name, newsletters, offerId}); } return { - page: 'loading' + page: 'loading', + sniperLinks: undefined }; }
🤖 Fix all issues with AI agents
In `@ghost/i18n/locales/de/portal.json`:
- Line 125: The "Open email" key in portal.json currently has an empty value;
replace the empty string for the "Open email" entry with a German translation
such as "E‑Mail öffnen" so the CTA renders in German (update the value for the
"Open email" key in the locales/de/portal.json file).
In `@ghost/i18n/locales/eu/portal.json`:
- Line 125: The "Open email" key in ghost/i18n/locales/eu/portal.json is empty
which may render a blank button; either supply the Basque translation by
replacing the empty string for the "Open email" property with the correct Basque
text (e.g., "Ireki posta" or your preferred translation) or, if the i18n system
reliably falls back to the key or default locale, confirm that fallback behavior
in the localization configuration and add a comment documenting that fallback;
locate the "Open email" key in portal.json and update its value or document
fallback accordingly.
In `@ghost/i18n/locales/lv/portal.json`:
- Line 125: The "Open email" key in portal.json currently has an empty value, so
add the Latvian translation for that label by replacing the empty string for the
"Open email" key with a proper Latvian phrase (e.g., "Atvērt e‑pastu") so
Latvian users see the localized label; update the value for the "Open email" key
in the lv locale object (portal.json) accordingly.
In `@ghost/i18n/locales/pt-BR/portal.json`:
- Around line 123-126: The "Open email" localization key in the pt-BR portal
JSON is empty causing the button label to render blank; update the translation
value for the "Open email" key (e.g., set it to "Abrir e‑mail" or another
appropriate Portuguese string) in the pt-BR portal locale file so the button
displays correctly.
In `@ghost/i18n/locales/sl/portal.json`:
- Line 125: The "Open email" key in ghost/i18n/locales/sl/portal.json is empty;
replace the empty string value for the "Open email" key with the Slovenian
translation (e.g., "Odpri e-pošto") so the UI shows a proper label for the sl
locale.
♻️ Duplicate comments (7)
ghost/i18n/locales/ja/portal.json (1)
125-125: Empty "Open email" translationSame concern as in mk locale: ensure empty values don’t render a blank CTA; add a translation or confirm fallback.
ghost/i18n/locales/uk/portal.json (1)
125-125: Empty "Open email" translationSame concern as in mk locale: ensure empty values don’t render a blank CTA; add a translation or confirm fallback.
ghost/i18n/locales/nb/portal.json (1)
125-125: Empty "Open email" translationSame concern as in mk locale: ensure empty values don’t render a blank CTA; add a translation or confirm fallback.
ghost/i18n/locales/sv/portal.json (1)
125-125: Empty "Open email" translationSame concern as in mk locale: ensure empty values don’t render a blank CTA; add a translation or confirm fallback.
ghost/i18n/locales/ca/portal.json (1)
125-125: Empty "Open email" translationSame concern as in mk locale: ensure empty values don’t render a blank CTA; add a translation or confirm fallback.
apps/portal/src/components/common/action-button.js (1)
105-120: Prevent “disabled” anchors from remaining keyboard-activatable.
disabledis not a valid<a>attribute andpointer-events: nonedoesn’t block keyboard activation. Whenhrefis present, a disabled ActionButton can still be focused and activated via keyboard, which undermines the disabled state and accessibility.🛠️ Suggested fix
- const commonProps = { - className, - style: Style.button, - disabled, - tabIndex, - 'data-test-button': dataTestId - }; + const commonProps = { + className, + style: Style.button, + tabIndex, + 'data-test-button': dataTestId + }; const loaderClassName = isPrimary ? 'gh-portal-loadingicon' : 'gh-portal-loadingicon dark'; const children = isRunning ? <LoaderIcon className={loaderClassName} /> : label; if (href) { - return <a {...commonProps} href={href} target={target} rel={rel}>{children}</a>; + const linkProps = { + ...commonProps, + href: disabled ? undefined : href, + target, + rel, + 'aria-disabled': disabled || undefined, + tabIndex: disabled ? -1 : tabIndex, + onClick: disabled ? (e) => e.preventDefault() : undefined + }; + return <a {...linkProps}>{children}</a>; } else { - return <button {...commonProps} onClick={onClick} type='submit'>{children}</button>; + return <button {...commonProps} disabled={disabled} onClick={onClick} type='submit'>{children}</button>; }Is the `disabled` attribute valid on HTML <a> elements, and what is the recommended way to implement a disabled link (including keyboard accessibility)?apps/portal/src/actions.js (1)
90-101: Stale sniperLinks may persist after navigation.When
sniperLinksisundefinedin the API response, React'ssetStatemerge won't clear any previously cachedsniperLinksfrom state. If a user starts a Gmail signup, navigates away, then signs in with a non-Gmail address, stale sniper links could be displayed.🔧 Suggested fix
return { page: 'magiclink', lastPage: 'signin', ...(otcRef ? {otcRef} : {}), - // TODO: Display these sniperlinks in the UI. See NY-946. - sniperLinks, + // TODO: Display these sniperlinks in the UI. See NY-946. + sniperLinks: sniperLinks || undefined, + otcRef: otcRef || undefined, pageData: { ...(state.pageData || {}), email: (data?.email || '').trim() } };Actually, since
sniperLinksmay already beundefined, the issue is that you need to explicitly set it to clear old state:return { page: 'magiclink', lastPage: 'signin', - ...(otcRef ? {otcRef} : {}), - // TODO: Display these sniperlinks in the UI. See NY-946. - sniperLinks, + otcRef: otcRef ?? undefined, + sniperLinks: sniperLinks ?? undefined, pageData: { ...(state.pageData || {}), email: (data?.email || '').trim() } };
🧹 Nitpick comments (4)
ghost/i18n/locales/id/portal.json (1)
125-125: Missing Indonesian translation for "Open email".The new key is correctly placed, but the translation value is empty. This follows the existing pattern in this file where several other keys also have empty translations pending. Ensure this gets translated before release to provide a complete Indonesian localization.
ghost/i18n/locales/de-CH/portal.json (1)
125-125: Missing translation for new UI string.The translation value is empty. Swiss German users will either see the English fallback "Open email" or potentially an empty button label depending on how the i18n framework handles empty strings.
Consider adding the translation:
- "Open email": "", + "Open email": "E-Mail öffnen",ghost/core/core/server/lib/get-sniper-links.ts (1)
72-86: Case normalization implemented, but trimming is missing.The
toLowerCase()on line 85 addresses the case-sensitivity concern. However, leading/trailing whitespace in the email could still cause missed matches. Consider trimming the input:♻️ Suggested improvement
function parseEmailDomain(email: string): null | string { + const trimmedEmail = email.trim(); // Add a simple limit to avoid someone causing significant performance // issues. SMTP's max email length is 986 octets. 1000 UTF-16 units should // be fine. - if (email.length > 1000) { + if (trimmedEmail.length > 1000) { return null; } - const atIndex = email.lastIndexOf('@'); + const atIndex = trimmedEmail.lastIndexOf('@'); if (atIndex === -1) { return null; } - return email.slice(atIndex + 1).toLowerCase(); + return trimmedEmail.slice(atIndex + 1).toLowerCase(); }apps/portal/test/signup-flow.test.js (1)
254-275: Good test coverage for the sniper link feature.The test correctly validates:
- Sniper link button renders when API returns
sniperLinks- Button has correct
hrefandtarget="_blank"attributesFor consistency with other tests in this file (e.g., lines 245-251), consider also verifying the mock was called with expected parameters:
💡 Optional: Add mock call verification
const sniperLinkButton = await within(popupIframeDocument).findByText(/open email/i); expect(sniperLinkButton).toBeInTheDocument(); expect(sniperLinkButton).toHaveAttribute('href', 'https://test.example/'); expect(sniperLinkButton).toHaveAttribute('target', '_blank'); + + expect(ghostApi.member.sendMagicLink).toHaveBeenLastCalledWith({ + email: 'test@test-sniper-link.example', + emailType: 'signup', + name: 'Jamie Larsen', + plan: 'free', + integrityToken: 'testtoken' + });This would require adding
ghostApito the destructured setup result.
9larsons
left a comment
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.
Generally some questions here. I'm fine with this PR on the whole, though with future changes it'd be helpful to break them up a bit more. I know this one got held up by the other PR w/ package location changes (framework v ghost) that didn't help.
| const {otcRef, sniperLinks} = this.context; | ||
|
|
||
| /** @type {ReactNode} */ let footer; | ||
| if (otcRef) { |
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.
Have we validated this behavior, that we only show sniper links if there's no OTC? OTC goes to inbox anyways, so I'm a little confused here.
ref https://linear.app/ghost/issue/NY-943 When you sign up with a `@gmail.com` email, the "Now check your email!" has an updated button which takes you to `mail.google.com`. - This also happens for `@googlemail.com` and `@google.com` addresses. - We plan to extend this in the future in three significant ways: - Support additional mail providers, like Outlook and Proton. - Do DNS lookups for MX records, which can get more domains. For example, `ghost.org`'s MX records refer to Gmail. - Show this link in other parts of the portal, such as signin.
ref https://linear.app/ghost/issue/NY-943 ref #25968 This updates the `send-magic-links` endpoint to return sniper links. This change should have no user impact yet, but will soon once the frontend supports it.
ref https://linear.app/ghost/issue/NY-943 ref #25968 This updates the `send-magic-links` endpoint to return sniper links. This change should have no user impact yet, but will soon once the frontend supports it.
ref https://linear.app/ghost/issue/NY-943 ref #25968 This updates the `send-magic-links` endpoint to return sniper links. This change should have no user impact yet, but will soon once the frontend supports it.
c9cb3cf to
6ae85a9
Compare
EvanHahn
left a comment
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.
Reworked this to be frontend-only.
| border: none; | ||
| } | ||
|
|
||
| .gh-portal-btn-primary { |
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.
The changes in this file could be a separate PR, but I didn't think it was worth the time.
There is no reason to set the font color with JavaScript as far as I could see, so I just set it with regular CSS. That's a useful improvement on its own but also makes it easier to use this class in <SniperLinkButton>.
| * }>} navigator | ||
| * @returns {boolean} | ||
| */ | ||
| export const isAndroidChrome = navigator => isAndroid(navigator) && isChrome(navigator); No newline at end of file |
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.
I realized these links only work on Android Chrome, so I changed this from isAndroid to isAndroidChrome. (The internal isAndroid function still remained.)
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/portal/src/components/pages/magic-link-page.js`:
- Around line 1-8: The portal UI change in magic-link-page.js requires the
portal app's UMD bundles to be regenerated: run the portal package build (yarn
build) to produce updated UMD .min.js files (umd/*.min.js), verify the generated
bundles include the change, and add/commit those built UMD files to the PR so
the CDN-distributed artifacts (UMD bundles) are up-to-date.
In `@apps/portal/test/signup-flow.test.js`:
- Around line 92-103: The mock for ghostApi.member.sendMagicLink destructures
{email} and calls email.endsWith, which throws if called with undefined; update
the mock to guard against missing args by using a default param or optional
chaining (e.g., accept ({email} = {}) or check args?.email) and only call
endsWith when email is a string, returning the same sniperLinks or {}
accordingly so the test remains robust when sendMagicLink is invoked without
arguments.
| import React from 'react'; | ||
| import ActionButton from '../common/action-button'; | ||
| import CloseButton from '../common/close-button'; | ||
| import SniperLinkButton from '../common/sniper-link-button'; | ||
| import AppContext from '../../app-context'; | ||
| import {ReactComponent as EnvelopeIcon} from '../../images/icons/envelope.svg'; | ||
| import {t} from '../../utils/i18n'; | ||
| import {isAndroidChrome} from '../../utils/is-android-chrome'; |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether UMD bundle artifacts exist and are updated
ls -la apps/portal/umd
echo "---"
git status --short apps/portal/umdRepository: TryGhost/Ghost
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Check portal app structure
echo "=== Portal app structure ==="
ls -la apps/portal/
echo "=== Search for .min.js files in portal ==="
find apps/portal -name "*.min.js" -type f
echo "=== Check if umd directory exists anywhere ==="
find apps -type d -name "umd"
echo "=== Check git history for umd bundles ==="
git ls-files | grep -E "apps/portal.*\.min\.js|apps/portal/umd"Repository: TryGhost/Ghost
Length of output: 1088
🏁 Script executed:
#!/bin/bash
# Check vite config for output path
echo "=== vite.config.mjs content ==="
cat apps/portal/vite.config.mjs
echo -e "\n=== Build scripts from package.json ==="
cat apps/portal/package.json | grep -A 5 '"scripts"'Repository: TryGhost/Ghost
Length of output: 3325
Run yarn build in apps/portal/ to regenerate the UMD bundle for CDN distribution.
The Vite configuration is set to build UMD bundles to apps/portal/umd/*.min.js. Please ensure this build step is completed before merging, as per the requirement that public UI apps must be built as UMD bundles for CDN distribution.
🤖 Prompt for AI Agents
In `@apps/portal/src/components/pages/magic-link-page.js` around lines 1 - 8, The
portal UI change in magic-link-page.js requires the portal app's UMD bundles to
be regenerated: run the portal package build (yarn build) to produce updated UMD
.min.js files (umd/*.min.js), verify the generated bundles include the change,
and add/commit those built UMD files to the PR so the CDN-distributed artifacts
(UMD bundles) are up-to-date.
| ghostApi.member.sendMagicLink = vi.fn(async ({email}) => { | ||
| if (email.endsWith('@test-sniper-link.example')) { | ||
| return { | ||
| sniperLinks: { | ||
| android: 'https://test.example/', | ||
| desktop: 'https://test.example/' | ||
| } | ||
| }; | ||
| } else { | ||
| return {}; | ||
| } | ||
| }); |
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.
Guard against undefined args in the mock.
If sendMagicLink is invoked without an арг, destructuring throws and email.endsWith crashes. Add defaults/optional chaining so the test harness stays robust.
🛠️ Suggested fix
- ghostApi.member.sendMagicLink = vi.fn(async ({email}) => {
- if (email.endsWith('@test-sniper-link.example')) {
+ ghostApi.member.sendMagicLink = vi.fn(async ({email} = {}) => {
+ if (email?.endsWith('@test-sniper-link.example')) {
return {
sniperLinks: {
android: 'https://test.example/',
desktop: 'https://test.example/'
}
};
} else {
return {};
}
});🤖 Prompt for AI Agents
In `@apps/portal/test/signup-flow.test.js` around lines 92 - 103, The mock for
ghostApi.member.sendMagicLink destructures {email} and calls email.endsWith,
which throws if called with undefined; update the mock to guard against missing
args by using a default param or optional chaining (e.g., accept ({email} = {})
or check args?.email) and only call endsWith when email is a string, returning
the same sniperLinks or {} accordingly so the test remains robust when
sendMagicLink is invoked without arguments.
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| > | ||
| {label} | ||
| </a> | ||
| ); |
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.
SniperLinkButton missing width style causes narrower button
Low Severity
The SniperLinkButton component is missing width: '100%' in its styles. The original ActionButton in renderCloseButton received style={{width: '100%'}} to span the full container width. The new SniperLinkButton doesn't accept a style prop and only applies background: brandColor internally. Since .gh-portal-btn uses display: flex with min-width: 80px but no full-width default, the sniper link button will render narrower than the close button it replaces, causing visual inconsistency on the "Now check your email!" screen.
Additional Locations (1)
9larsons
left a comment
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.
question: Why did we choose to only display OTCs if available, rather than both sniper links and OTC entry?
I have minor concerns the css change will cause issues.. somewhere.. however, it does make sense for it to be in css.
That's the next thing I'll be working on! |
ref https://linear.app/ghost/issue/NY-943
When you sign up with a
@gmail.comemail, the "Now check your email!" has an updated button which takes you tomail.google.com.Screencast.mp4
Note: I stubbed real email sending, so this screencast takes me to a bogus place in Gmail.
In the future, we plan to show this link in other parts of the portal.