🎨 Updated private page access and signup flow#26762
Conversation
WalkthroughThis pull request introduces a new private signup and access flow for Ghost's private blogging feature. It includes new helper functions for color manipulation (color_to_rgba, contrast_text_color) and utilities (admin_url, json), updates the private blogging template with a modal dialog for access codes and signup form, adds client-side JavaScript (private.js) to handle signup interactions, introduces corresponding CSS styles, adds static asset routes to serve the new scripts, and includes comprehensive test coverage and localization strings for multiple languages. 🚥 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🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
ghost/core/test/unit/frontend/apps/private-blogging/contrast-text-color.test.js (1)
19-20: Use a malformed color here to cover the fallback branch.
''is normalized to the default background color inghost/core/core/frontend/apps/private-blogging/lib/helpers/contrast_text_color.js, so this case never exercises thecatchpath. A non-empty invalid value would give you real coverage for the error fallback.Proposed test update
- it('falls back to white for invalid colors', function () { - assert.equal(contrast_text_color(''), '#FFFFFF'); + it('falls back to white for malformed colors', function () { + assert.equal(contrast_text_color('not-a-color'), '#FFFFFF'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/apps/private-blogging/contrast-text-color.test.js` around lines 19 - 20, The test "falls back to white for invalid colors" currently passes an empty string which is normalized before hitting the error branch; update the test to pass a malformed non-empty color (e.g. 'not-a-color' or '#GGGGGG') to force the parse error and exercise the catch/fallback path in contrast_text_color; modify the assertion in the same test (still named 'falls back to white for invalid colors') to expect '#FFFFFF' when calling contrast_text_color('not-a-color') so the fallback branch in contrast_text_color is covered.ghost/core/test/unit/frontend/apps/private-blogging/color-to-rgba.test.js (1)
19-20: This case doesn't exercise the malformed-color fallback.An empty string takes the helper's default-color path, so it won't catch regressions in the actual error-handling branch for bad color input. Using a non-empty invalid value would make the test match its intent.
Proposed test update
- it('falls back for invalid colors', function () { - assert.equal(color_to_rgba('', 0.25), 'rgba(21, 23, 26, 0.25)'); + it('falls back for malformed colors', function () { + assert.equal(color_to_rgba('not-a-color', 0.25), 'rgba(21, 23, 26, 0.25)'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/frontend/apps/private-blogging/color-to-rgba.test.js` around lines 19 - 20, The test is using an empty string which triggers the helper's default-color path rather than exercising the malformed-color fallback; update the test case in color-to-rgba.test.js to pass a non-empty invalid color (e.g. 'not-a-color' or '#zzz') to color_to_rgba so it hits the error-handling branch and asserts the fallback rgba('rgba(21, 23, 26, 0.25)') result for malformed input.ghost/core/core/frontend/public/private.js (1)
119-136: Error message normalization relies on exact string matching.The normalization for
restrictedDomain,tooManyAttempts, andfailedMagicLinkuses exact string comparison (Lines 123-133), whileinvalidEmailuses regex (Line 119). If server-side messages change, these exact matches will silently fall through togenericError.This is acceptable given the fallback behavior, but consider using case-insensitive partial matching for consistency if server message stability is uncertain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/frontend/public/private.js` around lines 119 - 136, The current normalization block uses exact string equality for messages.restrictedDomain, messages.tooManyAttempts, and messages.failedMagicLink which is brittle; update those checks in the same function that returns messages.* to use case-insensitive partial matching (e.g. convert message to lower case and use .includes(...) or use /.../i regex) instead of === so they behave like the invalidEmail regex check and will still match if server wording changes.ghost/core/core/frontend/public/ghost.css (2)
789-805: Duplicate selector block for loading/success icons.Lines 789-799 and 801-805 both target
.gh-private-signup-btn-loading, .gh-private-signup-btn-success. The second block (801-805) setswidthandheight, which could be consolidated into the first block for clarity.♻️ Proposed consolidation
.gh-private-signup-btn-loading, .gh-private-signup-btn-success { position: absolute; top: 50%; left: 50%; display: block; visibility: hidden; opacity: 0; overflow: visible; transform: translate(-50%, -50%); + width: 18px; + height: 18px; } - -.gh-private-signup-btn-loading, -.gh-private-signup-btn-success { - width: 18px; - height: 18px; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/frontend/public/ghost.css` around lines 789 - 805, There are duplicate selector blocks for .gh-private-signup-btn-loading and .gh-private-signup-btn-success; consolidate the width and height declarations into the first rule that sets position/top/left/visibility/opacity/transform, remove the redundant second block, and ensure the combined rule preserves the existing properties (position, top, left, display, visibility, opacity, overflow, transform, width, height) and ordering so styles remain identical.
807-819: Consider respectingprefers-reduced-motionfor the loading animation.The
gh-private-signup-dotsanimation (Lines 807-819) continues to run even when users have enabled reduced motion preferences. Currently, only dialog transitions are handled in theprefers-reduced-motionmedia query (Lines 1068-1072).For improved accessibility, consider pausing or simplifying the animation for users who prefer reduced motion.
♻️ Proposed addition
`@media` (prefers-reduced-motion: no-preference) { .gh-private-dialog { transition: transform 180ms ease, opacity 180ms ease, background-color 180ms ease; } } + +@media (prefers-reduced-motion: reduce) { + .gh-private-signup-btn-loading circle { + animation: none; + opacity: 0.7; + } +}Also applies to: 1068-1072
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/frontend/public/ghost.css` around lines 807 - 819, The loading-dot animation for .gh-private-signup-btn-loading using animation gh-private-signup-dots currently ignores users' reduced motion preference; add a prefers-reduced-motion media query that targets .gh-private-signup-btn-loading circle (and the nth-child selectors if needed) to disable or greatly simplify the animation (e.g., set animation: none and remove transform/opacity animation) so the dots do not animate when prefers-reduced-motion: reduce is set; mirror the approach used for dialog transitions in the existing prefers-reduced-motion block to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/frontend/apps/private-blogging/lib/helpers/json.js`:
- Around line 12-19: JSON.stringify(payload) can return undefined which makes
the chained .replace() calls throw; guard the serialization by capturing the
result of JSON.stringify(payload) into the serialized variable, check if
serialized === undefined and if so set serialized to a safe fallback (e.g.
'null' or ''), then perform the replace() calls on that string and return new
SafeString(serialized); update the code around the JSON.stringify(payload) ->
serialized assignment (and references to payload coming from
value/hash/options.hash) so templates receiving undefined don't cause a
TypeError.
In `@ghost/core/core/frontend/apps/private-blogging/lib/views/private.hbs`:
- Around line 37-40: The new password/access-code form must have a non-dialog
fallback so users without JS or without <dialog> support can submit and see
server errors; keep a plain form (or a <noscript> duplicate) outside the
<dialog> and render server-side error messages into that outer container so
failed POSTs surface, and make the existing <dialog> usage in private.js
progressive (JS should only open/replace the fallback form, not be required to
submit); specifically: retain a visible form when
document.documentElement.classList contains 'no-js' (or add a <noscript> form),
move server-rendered error output out of the closed <dialog> into a shared
container, and ensure private.js only enhances (opens) the dialog rather than
being the sole path to submit or show errors.
- Around line 58-60: The email input (class="gh-input gh-private-signup-input",
data-members-email) uses only a placeholder and needs an accessible name; add a
proper label or aria-label to ensure assistive tech can read it when the
placeholder is gone. Fix by either adding a visually-hidden <label> tied to the
input via id (create a unique id on the input and a corresponding label) or by
adding a descriptive aria-label attribute (e.g., "Email address") to the input
element inside the form (data-ghost-private-subscribe-form /
data-members-form="subscribe") so the field is properly exposed to screen
readers.
- Line 113: Replace the hard-coded footer link label "Site owner login" in
private.hbs with a translation helper (e.g., use the template translation helper
around a new key like "private.site_owner_login") so the link text renders via
i18n, and add the corresponding locale entries for that key to the fixture
locale files so translated pages don't mix languages; update the template
reference to use the translation key and ensure the key exists in all relevant
locale fixtures.
In `@ghost/core/core/frontend/public/private.js`:
- Around line 259-265: The integrity token fetch is not checked for errors
before calling integrityTokenRes.text(), which can pass invalid data to
sendMagicLink; update the logic around the fetch to
'/members/api/integrity-token/' so you explicitly check integrityTokenRes.ok
(and handle non-2xx) and wrap the fetch in try/catch to handle network failures,
aborting or returning an error response instead of continuing; ensure you
reference integrityTokenRes and integrityToken and only call
integrityTokenRes.text() when the response is ok, and if not, surface a clear
error (or short-circuit) before making the fetch to
'/members/api/send-magic-link/'.
In `@ghost/core/test/utils/fixtures/themes/locale-theme/locales/de.json`:
- Around line 17-20: The new German translations use ASCII
transliterations—replace "Abonnement bestaetigt!" with "Abonnement bestätigt!",
"Anmeldungen mit dieser E-Mail-Domain sind derzeit eingeschraenkt." with
"Anmeldungen mit dieser E-Mail‑Domain sind derzeit eingeschränkt.", and "Zu
viele Anmeldeversuche. Bitte versuche es spaeter erneut." with "Zu viele
Anmeldeversuche. Bitte versuche es später erneut." by updating the corresponding
JSON values for the keys "Subscription confirmed!", "Signups from this email
domain are currently restricted.", and "Too many sign-up attempts, try again
later" so the file consistently uses proper UTF-8 German characters (including
the umlauts and the hyphen in E‑Mail‑Domain).
---
Nitpick comments:
In `@ghost/core/core/frontend/public/ghost.css`:
- Around line 789-805: There are duplicate selector blocks for
.gh-private-signup-btn-loading and .gh-private-signup-btn-success; consolidate
the width and height declarations into the first rule that sets
position/top/left/visibility/opacity/transform, remove the redundant second
block, and ensure the combined rule preserves the existing properties (position,
top, left, display, visibility, opacity, overflow, transform, width, height) and
ordering so styles remain identical.
- Around line 807-819: The loading-dot animation for
.gh-private-signup-btn-loading using animation gh-private-signup-dots currently
ignores users' reduced motion preference; add a prefers-reduced-motion media
query that targets .gh-private-signup-btn-loading circle (and the nth-child
selectors if needed) to disable or greatly simplify the animation (e.g., set
animation: none and remove transform/opacity animation) so the dots do not
animate when prefers-reduced-motion: reduce is set; mirror the approach used for
dialog transitions in the existing prefers-reduced-motion block to keep behavior
consistent.
In `@ghost/core/core/frontend/public/private.js`:
- Around line 119-136: The current normalization block uses exact string
equality for messages.restrictedDomain, messages.tooManyAttempts, and
messages.failedMagicLink which is brittle; update those checks in the same
function that returns messages.* to use case-insensitive partial matching (e.g.
convert message to lower case and use .includes(...) or use /.../i regex)
instead of === so they behave like the invalidEmail regex check and will still
match if server wording changes.
In `@ghost/core/test/unit/frontend/apps/private-blogging/color-to-rgba.test.js`:
- Around line 19-20: The test is using an empty string which triggers the
helper's default-color path rather than exercising the malformed-color fallback;
update the test case in color-to-rgba.test.js to pass a non-empty invalid color
(e.g. 'not-a-color' or '#zzz') to color_to_rgba so it hits the error-handling
branch and asserts the fallback rgba('rgba(21, 23, 26, 0.25)') result for
malformed input.
In
`@ghost/core/test/unit/frontend/apps/private-blogging/contrast-text-color.test.js`:
- Around line 19-20: The test "falls back to white for invalid colors" currently
passes an empty string which is normalized before hitting the error branch;
update the test to pass a malformed non-empty color (e.g. 'not-a-color' or
'#GGGGGG') to force the parse error and exercise the catch/fallback path in
contrast_text_color; modify the assertion in the same test (still named 'falls
back to white for invalid colors') to expect '#FFFFFF' when calling
contrast_text_color('not-a-color') so the fallback branch in contrast_text_color
is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a419b92-b7e6-4390-9088-0a8de041c735
📒 Files selected for processing (21)
ghost/core/bin/minify-assets.jsghost/core/core/frontend/apps/private-blogging/lib/helpers/color_to_rgba.jsghost/core/core/frontend/apps/private-blogging/lib/helpers/contrast_text_color.jsghost/core/core/frontend/apps/private-blogging/lib/helpers/input_password.jsghost/core/core/frontend/apps/private-blogging/lib/helpers/json.jsghost/core/core/frontend/apps/private-blogging/lib/views/private.hbsghost/core/core/frontend/helpers/admin_url.jsghost/core/core/frontend/public/ghost.cssghost/core/core/frontend/public/private.jsghost/core/core/frontend/web/routers/serve-public-file.jsghost/core/test/unit/frontend/apps/private-blogging/color-to-rgba.test.jsghost/core/test/unit/frontend/apps/private-blogging/contrast-text-color.test.jsghost/core/test/unit/frontend/apps/private-blogging/controller.test.jsghost/core/test/unit/frontend/apps/private-blogging/input-password.test.jsghost/core/test/unit/frontend/apps/private-blogging/json.test.jsghost/core/test/unit/frontend/helpers/admin-url.test.jsghost/core/test/unit/frontend/public/private.test.jsghost/core/test/unit/frontend/services/theme-engine/handlebars/helpers.test.jsghost/core/test/unit/frontend/web/middleware/serve-public-file.test.jsghost/core/test/utils/fixtures/themes/locale-theme/locales/de.jsonghost/core/test/utils/fixtures/themes/locale-theme/locales/en.json
ghost/core/core/frontend/apps/private-blogging/lib/helpers/json.js
Outdated
Show resolved
Hide resolved
ghost/core/core/frontend/apps/private-blogging/lib/views/private.hbs
Outdated
Show resolved
Hide resolved
ghost/core/core/frontend/apps/private-blogging/lib/views/private.hbs
Outdated
Show resolved
Hide resolved
|
@JohnONolan checking a detail or two about your intentions for how this works. Q1. Is it intended that the JS for the page ended up a separate asset There are 4 new helpers added: Q2. Was this setup this way intentionally, or is it AI side-effects? I was curious why we don't already have the colour helpers - turns out there are multiple bugs and/or workarounds in existing themes... so it seems like these would be globally useful. Pretty sure I've seen quite a few requests for the JSON one too. Lastly there's Q3. Any further thoughts about safety here or strong opinions? On naming of the helper. Existing patterns are Q4. Any strong opinions on naming? None of this is particularly blocking! |
There was a problem hiding this comment.
Review Summary
8 commits of fixes and improvements on top of the original PR. gscan PR opened separately (gscan #742).
Scoring: 1/5 = fine to ignore, 5/5 = MUST fix.
Must Fix Before Merge
-
[5/5] Guard
jsonhelper againstJSON.stringify(undefined)- Added
undefinedguard, returns'null'. Test added.
- Added
-
[5/5] gscan helper awareness for new helpers
- Moved
json,color_to_rgba,contrast_text_colorfrom app dir tocore/frontend/helpers/. - Removed redundant
admin_urlhelper (superseded by{{@site.admin_url}}). - Added gscan cross-reference test that fails for helpers missing from gscan — currently fails for 5 helpers (
json,color_to_rgba,contrast_text_color,raw,search). - gscan PR: TryGhost/gscan#742
- Moved
-
[4/5] Handle integrity token fetch non-2xx explicitly
- Added
if (!integrityTokenRes.ok) throwguard. Test added.
- Added
-
[4/5] Localize "Site owner login"
- Wrapped in
{{t}}inprivate.hbs. Locale fixtures updated. German translation assertion added.
- Wrapped in
-
[4/5] Acceptaction=subscribein confirmation-state handler- FALSE POSITIVE. Members middleware always normalizes redirect to
action=signup. No code path producesaction=subscribe.
- FALSE POSITIVE. Members middleware always normalizes redirect to
Should Fix (done)
-
[3/5] Add accessible name for signup email input
- Added
aria-label="{{t "Your email address"}}".
- Added
-
[3/5] Trim email before validity checks
- Added
subscribeInput.value.trim()beforecheckValidity(). Test added.
- Added
-
[3/5] Resolve helper scope ambiguity
- Moved all 3 helpers to
core/frontend/helpers/. Tests moved alongside. gscan test added.
- Moved all 3 helpers to
-
[3/5 → Design Discussion] Gate signup on
allow_external_signup- Terms are NOT enforced server-side (Portal-only UI). Unusual scenario (private site + open signups + mandatory terms). Not blocking this PR.
Also Fixed
-
[2/5] Reverted
console.error→logging.errorinbin/minify-assets.js- Unrelated change in the PR.
console.erroris correct for CLI build scripts.
- Unrelated change in the PR.
-
[2/5] Fixed error normalization — removed dead code, switched to type-based matching
normalizeSubscribeErrorMessagenow receives the full error object, matches ontypefield (TooManyRequestsError) and keyword regex (/restricted/i) instead of exact strings.- Removed
failedMagicLinkcode path entirely (server never sends this string). - Two of the three original string comparisons were dead code that could never match.
-
[2/5] Fixed
@site.admin_urldoubling/ghost/pathurlUtils.urlFor('admin', true)joins admin URL with/ghost/, but whenadmin:urlconfig already contains/ghost/this produces/ghost/ghost/. Fixed to useurlUtils.getAdminUrl()with fallback.
-
[2/5] Admin URL exposure
- Reviewed.
admin_urlis in@sitetemplate data (for "Site owner login" link) but intentionally NOT in Content API/site/endpoint.
- Reviewed.
-
[1/5] Test coverage improvements
- Added 3 unhappy path tests: client-side validation failure, network error (fetch throws), unparseable error response body.
- Added tests for rate-limit and restricted-domain error normalization.
- Added middleware test for
admin_urlin@sitedata.
Not Fixing (follow-up / not warranted)
- CSS in ghost.css — Loading a separate CSS file for the private page would add an HTTP request for minimal savings. The CSS is already cached via ghost.css.
- Success-state duration (1000ms) — Subjective UX tuning, can be adjusted later.
- Motion/CSS polish — Reduced-motion handling, duplicate selectors. Follow-up territory.
|
Q1 - no specific intent. I think css was already loaded via global file and JS wasn’t (?) so it went this route Q2 - aware yes, but no strong opinions about any of these. An argument could be made that helpers for text contrast are outdated and should use contrast-color() pure css instead, which is about to land in chrome 147… but I think the helpers might be useful anyway Q3+4 - no strong opinions from me |
ErisDS
force-pushed
the
codex/private-page-subscribe-flow
branch
2 times, most recently
from
44d8afc to
d0d2ebd
Compare
Reworked Ghost core's private page to use the new access dialog, optional subscribe form, accent-aware styling, and a dedicated runtime script without pulling Portal onto the page. Also adds the helper, asset pipeline, and test coverage needed to ship and maintain the new flow.
…g, and i18n gap
- Guard json helper against JSON.stringify(undefined) returning undefined
- Check integrityTokenRes.ok before calling .text() in private.js
- Wrap "Site owner login" in {{t}} for i18n support
- Add locale fixture entries and test coverage for all three fixes
- Moved json, color_to_rgba, contrast_text_color from private-blogging app to core/frontend/helpers/
- Added admin_url to @site template context, updated private.hbs to use {{@site.admin_url}}
- Added gscan cross-reference test to catch helpers missing from gscan knownHelpers
- Updated helpers.test.js allowlist with the three new global helpers
- Replaced brittle exact-string error matching with type-based matching (TooManyRequestsError) and keyword regex (restricted domain) - Removed dead failedMagicLink code path that could never match - Added aria-label to signup email input for screen readers - Trim email whitespace before checkValidity() runs - Reverted unrelated console.error to logging.error change in bin script
…ved tests, fixed gscan test - Removed admin_url helper file (superseded by @site.admin_url data) - Moved json, color_to_rgba, contrast_text_color tests to helpers dir - Fixed gscan cross-reference test: tracks exact pending set so it fails if a new helper is added without updating gscan or the pending list - Added middleware test verifying admin_url in @site data
- Client-side validation failure (checkValidity returns false) - Network error (fetch throws, e.g. offline) - Unparseable error response body (API returns non-JSON)
Removed pendingGscanUpdate workaround — the test now correctly fails for color_to_rgba, contrast_text_color, json until they are added to gscan knownHelpers.
Removed raw and search from internalHelpers exclusion list — they should be in gscan knownHelpers. Test now fails for all 5 helpers pending gscan PR: TryGhost/gscan#742
…gured
urlUtils.urlFor("admin", true) joins the admin URL with "/ghost/",
but when admin:url already contains "/ghost/" this produces a doubled
path like "/ghost/ghost/". Use getAdminUrl() which returns the URL
directly from config, falling back to urlFor for environments where
no separate admin URL is configured.
ErisDS
force-pushed
the
codex/private-page-subscribe-flow
branch
from
c8e57cd to
5c48a5b
Compare
Final review statusRebased onto latest main (gscan 5.4.0 is already there). All 57 affected tests pass, including the gscan cross-reference test which now passes with the 5 new helpers in 9 commits on top of the original PR:
Items intentionally not addressed:
|
ref #26762 - privacy pages were updated, but browser tests reference the old flow - this fixes them to use the new flow, whereby the user clicks "Enter access code" and the input and button are visible
2 participants
What
Why
The private page now needs a richer branded experience and signup flow, but Portal is not available on this template. Keeping the behavior local to Ghost core gives us the required UX without broadening the page runtime.
Testing