🎨 Improved sign-in to return you to the comment you were replying to#28439
Open
luissazevedo wants to merge 5 commits into
Open
🎨 Improved sign-in to return you to the comment you were replying to#28439luissazevedo wants to merge 5 commits into
luissazevedo wants to merge 5 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements a feature enabling users to be redirected to a specific comment after signing in from comment CTAs. Changes span comments-ui and portal: ReplyButton and Like/Dislike include commentId when opening the CTA popup; CTAPopup/CTABox forward commentId and CTABox builds and encodes a comment permalink into the Portal signin redirect; getPageUrl and cleanPageUrl remove transient auth markers from permalinks; Portal adds isSameOriginUrl and merges a validated redirect into signin pageData; closePopup clears one-shot redirects. Tests (unit and E2E) exercise these paths. Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/comments-ui/src/index.tsx`:
- Around line 51-54: The code unconditionally deletes the 'action' query param
in apps/comments-ui/src/index.tsx which strips publisher-specific values; change
this to only remove Ghost auth markers by checking
url.searchParams.get('action') and deleting it only when it equals known Ghost
values (e.g., 'signin', 'signup', 'giftRedeem' or whatever markers your Ghost
install uses); leave 'success' handling as-is if it should always be removed,
and ensure this conditional targets the same location where you build permalinks
(referencing index.tsx and the buildCommentPermalink flow) so other publisher
query params are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a9012cd-e0cb-4b32-9d70-83806e884f4a
📒 Files selected for processing (11)
apps/comments-ui/src/components/content/buttons/reply-button.tsxapps/comments-ui/src/components/content/comment.tsxapps/comments-ui/src/components/content/cta-box.tsxapps/comments-ui/src/components/popups/cta-popup.tsxapps/comments-ui/src/index.tsxapps/comments-ui/test/e2e/reply-signin-redirect.test.tsapps/portal/src/actions.jsapps/portal/src/app.jsapps/portal/src/utils/helpers.jsapps/portal/test/actions.test.tsapps/portal/test/utils/helpers.test.js
luissazevedo
force-pushed
the
BER-3648/reply-sign-in-scroll-position
branch
from
c7357a4 to
8c71734
Compare
ref https://linear.app/tryghost/issue/BER-3648/ - clicking Reply while logged out, then signing in, used to reload the post and drop the reader at the top; now it returns them to the comment they wanted to reply to (scrolled + highlighted) - the reply CTA's "Sign in" asks Portal to return to that comment via a same-site redirect (#/portal/signin?redirect=<comment permalink>); Portal threads pageData.redirect through the magic-link round trip and core preserves the fragment, so the existing permalink path does the scroll/highlight — no client-side storage needed - portal: the signin route reads a redirect from its hash query, validated same-origin (isSameOriginUrl) to prevent open redirects - portal: closePopup clears a one-shot redirect from pageData so a dismissed sign-in can't leak its redirect into a later, unrelated sign-in on the same page - comments-ui: getPageUrl strips success/action auth markers so they don't leak into comment permalinks (or the redirect built from the page URL) - scope: sign-in only; free/paid sign-up deferred (same redirect mechanism)
luissazevedo
force-pushed
the
BER-3648/reply-sign-in-scroll-position
branch
from
8c71734 to
db53d44
Compare
ref https://linear.app/tryghost/issue/BER-3648/ - review follow-up: likes/dislikes opened the same sign-in CTA as replies but without the comment id, so readers signing in from a like lost their place — they now get the same redirect treatment - gated the post-auth `action` param strip on `success` being present, since Ghost only ever sets them together — a publisher's own standalone `?action=…` param is no longer dropped from permalinks - extracted the stripping into a pure cleanPageUrl() helper so it could be unit tested
…-in-scroll-position # Conflicts: # apps/comments-ui/package.json # apps/portal/package.json
ref https://linear.app/tryghost/issue/BER-3648/ - the pre-commit hook auto-bumped @tryghost/activitypub to 3.1.37 during a non-interactive merge of main, because the merge staged main's activitypub changes and the hook's tty prompt defaulted to yes - this PR doesn't touch activitypub, so the version should stay at main's 3.1.36
ref https://linear.app/tryghost/issue/BER-3648/ - a comment permalink is the post's canonical URL plus an anchor; Ghost routes posts purely by path, so no query param is needed to resolve it - guessing which params were safe to strip was both over- and under-inclusive: it dropped a publisher's own ?success=…&action=… params while letting Ghost's errorCode marker (and a potential one-time token) leak into permalinks and the post-sign-in redirect - stripping everything also keeps ?/& out of the encoded redirect value, which avoids mis-parsing in browsers that return location.hash percent-decoded (Firefox)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/tryghost/issue/BER-3648/
What
When a logged-out reader clicks Reply on a comment and signs in, the page used to reload and drop them at the top of the post. Now it returns them to the comment they were replying to (scrolled + highlighted).
How
#/portal/signin?redirect=<comment permalink>.pageData.redirectthrough the magic-link round-trip and core preserves the URL fragment, so the existing permalink path does the scroll/highlight — no client-side storage, no new restore logic.🔍 Reviewer notes — please weigh in on these two
1.
closePopupclears a one-shot redirect (shared Portal action) —apps/portal/src/actions.jsTo stop a dismissed reply-sign-in from leaking its
pageData.redirectinto a later, unrelated sign-in on the same page,closePopupnow stripsredirectfrompageData(other keys preserved). This runs on every popup close across Portal — please confirm no existing flow relies onpageData.redirectsurviving a close. Account-page / feedback redirects consume their redirect during sign-in (not after a close), so I believe it's safe; covered by a newactions.test.tstest, and existing signin-flow tests still pass.2.
getPageUrlstrips a genericactionparam —apps/comments-ui/src/index.tsxWe strip
success/actionso they don't leak into comment permalinks (and the redirect built from the page URL).successis unambiguously a Ghost auth marker, butactionis generic — a publisher could legitimately use?action=…and we'd silently drop it. Options: only stripactionwhensuccessis present, or only stripaction=signinor not implement thisSecurity
The sign-in link can now carry a
redirect(where to send the reader after they log in). It's validated same-site only — via a newisSameOriginUrl()helper that compares URL origins rather than a string prefix, so it can't be fooled by look-alike hosts or a missing trailing slash — meaning it can't bounce anyone to an external site. This makes "redirect after sign-in" usable by any page on the site, not just comments, and Ghost core has an older, looser version of the same check elsewhere. Neither is exploitable in this PR, but flagging in case we want to tighten or standardise it.