Added gift purchase success modal in Portal#27074
Merged
Conversation
Contributor
|
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 ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR adds handling for Stripe gift purchase callbacks: when the URL contains 🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (2)
apps/portal/src/components/pages/gift-success-page.js (1)
115-115: Encode the token before composing the redemption URL.Using the raw token in a path segment can break the link if token format ever includes URL-unsafe characters.
🔗 Proposed fix
- const redeemUrl = `${siteUrl.replace(/\/$/, '')}/gift/${token}`; + const encodedToken = token ? encodeURIComponent(token) : ''; + const redeemUrl = `${siteUrl.replace(/\/$/, '')}/gift/${encodedToken}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/components/pages/gift-success-page.js` at line 115, The redeemUrl currently interpolates the raw token into the path (redeemUrl = `${siteUrl.replace(/\/$/, '')}/gift/${token}`), which can break for URL-unsafe characters; fix by URL-encoding the token before composing the URL (use encodeURIComponent(token)) so redeemUrl uses the encoded token, keeping the existing siteUrl.replace(/\/$/, '') logic intact.apps/portal/test/portal-links.test.js (1)
383-406: Add an assertion for query-param cleanup in the success-path test.The new flow depends on URL cleanup after handling success, but the test currently only checks rendering. Asserting
history.replaceState(or final URL/search) would catch regressions in that behavior.🧪 Suggested test addition
test('opens gift success page when giftSubscriptions labs flag is enabled', async () => { + const replaceStateSpy = vi.spyOn(window.history, 'replaceState'); window.location.href = 'https://portal.localhost/?stripe=gift-purchase-success&gift_token=abc123'; window.location.search = '?stripe=gift-purchase-success&gift_token=abc123'; window.location.hash = ''; window.location.pathname = '/'; @@ const redeemUrl = within(popupFrame.contentDocument).queryByText(/\/gift\/abc123/); expect(redeemUrl).toBeInTheDocument(); + expect(replaceStateSpy).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/test/portal-links.test.js` around lines 383 - 406, Update the test "opens gift success page when giftSubscriptions labs flag is enabled" to assert that the query param cleanup occurs after handling the success flow: after locating popupFrame and redeemUrl, verify history.replaceState was called (or that window.location.search is now empty) to ensure the "?stripe=gift-purchase-success&gift_token=abc123" params are removed; use the existing setup/test scaffolding (the setup helper, popupFrame variable and window.location stub) to spy on history.replaceState or check window.location.search and add the assertion accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/portal/src/app.js`:
- Around line 498-510: The cleanup of gift checkout query params should run
regardless of whether the popup is shown: when qParams.get('stripe') ===
'gift-purchase-success' always call clearURLParams(['stripe','gift_token'])
before any returns, then only return the popup object (showPopup/page/pageData)
if hasGiftSubscriptions({site}) is true and a token exists; update the branch
that currently checks qParams/hasGiftSubscriptions/token so clearURLParams
executes unconditionally and only the return of the popup is conditional.
In `@apps/portal/src/components/pages/gift-success-page.js`:
- Around line 117-121: The handleCopy handler starts a setTimeout to reset
copied state but doesn't clear it on unmount; store the timer id (e.g., in a
ref) when calling setTimeout inside handleCopy, and add a cleanup that clears
the timeout on unmount (useEffect cleanup) or when starting a new timeout so
setCopied(false) cannot run after the component unmounts; reference the
handleCopy function, the setCopied state setter, and redeemUrl to locate where
to add the ref and the clearTimeout cleanup.
---
Nitpick comments:
In `@apps/portal/src/components/pages/gift-success-page.js`:
- Line 115: The redeemUrl currently interpolates the raw token into the path
(redeemUrl = `${siteUrl.replace(/\/$/, '')}/gift/${token}`), which can break for
URL-unsafe characters; fix by URL-encoding the token before composing the URL
(use encodeURIComponent(token)) so redeemUrl uses the encoded token, keeping the
existing siteUrl.replace(/\/$/, '') logic intact.
In `@apps/portal/test/portal-links.test.js`:
- Around line 383-406: Update the test "opens gift success page when
giftSubscriptions labs flag is enabled" to assert that the query param cleanup
occurs after handling the success flow: after locating popupFrame and redeemUrl,
verify history.replaceState was called (or that window.location.search is now
empty) to ensure the "?stripe=gift-purchase-success&gift_token=abc123" params
are removed; use the existing setup/test scaffolding (the setup helper,
popupFrame variable and window.location stub) to spy on history.replaceState or
check window.location.search and add the assertion accordingly.
🪄 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: 03063e0f-dfae-48fe-a2a2-41f0e3193df5
⛔ Files ignored due to path filters (1)
apps/portal/src/images/icons/gift.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
apps/portal/src/app.jsapps/portal/src/components/frame.styles.jsapps/portal/src/components/pages/gift-success-page.jsapps/portal/src/pages.jsapps/portal/test/portal-links.test.js
apps/portal/src/app.js
Outdated
Comment on lines
+498
to
+510
| if (qParams.get('stripe') === 'gift-purchase-success' && hasGiftSubscriptions({site})) { | ||
| const token = qParams.get('gift_token'); | ||
| if (token) { | ||
| clearURLParams(['stripe', 'gift_token']); | ||
| return { | ||
| showPopup: true, | ||
| page: 'giftSuccess', | ||
| pageData: { | ||
| token | ||
| } | ||
| }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Clear gift checkout query params even when popup is not opened.
Right now, stripe/gift_token are only removed on the happy path. If the lab is disabled (or token is missing), those params persist in the URL/history. Move cleanup to run for any stripe=gift-purchase-success callback.
🔧 Proposed fix
- if (qParams.get('stripe') === 'gift-purchase-success' && hasGiftSubscriptions({site})) {
- const token = qParams.get('gift_token');
- if (token) {
- clearURLParams(['stripe', 'gift_token']);
- return {
- showPopup: true,
- page: 'giftSuccess',
- pageData: {
- token
- }
- };
- }
+ if (qParams.get('stripe') === 'gift-purchase-success') {
+ const token = qParams.get('gift_token');
+ clearURLParams(['stripe', 'gift_token']);
+ if (token && hasGiftSubscriptions({site})) {
+ return {
+ showPopup: true,
+ page: 'giftSuccess',
+ pageData: {
+ token
+ }
+ };
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (qParams.get('stripe') === 'gift-purchase-success' && hasGiftSubscriptions({site})) { | |
| const token = qParams.get('gift_token'); | |
| if (token) { | |
| clearURLParams(['stripe', 'gift_token']); | |
| return { | |
| showPopup: true, | |
| page: 'giftSuccess', | |
| pageData: { | |
| token | |
| } | |
| }; | |
| } | |
| } | |
| if (qParams.get('stripe') === 'gift-purchase-success') { | |
| const token = qParams.get('gift_token'); | |
| clearURLParams(['stripe', 'gift_token']); | |
| if (token && hasGiftSubscriptions({site})) { | |
| return { | |
| showPopup: true, | |
| page: 'giftSuccess', | |
| pageData: { | |
| token | |
| } | |
| }; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/portal/src/app.js` around lines 498 - 510, The cleanup of gift checkout
query params should run regardless of whether the popup is shown: when
qParams.get('stripe') === 'gift-purchase-success' always call
clearURLParams(['stripe','gift_token']) before any returns, then only return the
popup object (showPopup/page/pageData) if hasGiftSubscriptions({site}) is true
and a token exists; update the branch that currently checks
qParams/hasGiftSubscriptions/token so clearURLParams executes unconditionally
and only the return of the popup is conditional.
Comment on lines
+117
to
+121
| const handleCopy = () => { | ||
| copyTextToClipboard(redeemUrl); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }; |
Contributor
There was a problem hiding this comment.
Cleanup the copy-reset timeout on unmount to avoid stale state updates.
If the modal closes before 2 seconds, setCopied(false) can still fire against an unmounted component.
🧹 Proposed fix
-import {useContext, useState} from 'react';
+import {useContext, useEffect, useRef, useState} from 'react';
@@
const GiftSuccessPage = () => {
const {site, pageData} = useContext(AppContext);
const [copied, setCopied] = useState(false);
+ const copiedResetTimeoutRef = useRef(null);
@@
const handleCopy = () => {
copyTextToClipboard(redeemUrl);
setCopied(true);
- setTimeout(() => setCopied(false), 2000);
+ clearTimeout(copiedResetTimeoutRef.current);
+ copiedResetTimeoutRef.current = setTimeout(() => setCopied(false), 2000);
};
+
+ useEffect(() => {
+ return () => clearTimeout(copiedResetTimeoutRef.current);
+ }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleCopy = () => { | |
| copyTextToClipboard(redeemUrl); | |
| setCopied(true); | |
| setTimeout(() => setCopied(false), 2000); | |
| }; | |
| import {useContext, useEffect, useRef, useState} from 'react'; | |
| const GiftSuccessPage = () => { | |
| const {site, pageData} = useContext(AppContext); | |
| const [copied, setCopied] = useState(false); | |
| const copiedResetTimeoutRef = useRef(null); | |
| const handleCopy = () => { | |
| copyTextToClipboard(redeemUrl); | |
| setCopied(true); | |
| clearTimeout(copiedResetTimeoutRef.current); | |
| copiedResetTimeoutRef.current = setTimeout(() => setCopied(false), 2000); | |
| }; | |
| useEffect(() => { | |
| return () => clearTimeout(copiedResetTimeoutRef.current); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/portal/src/components/pages/gift-success-page.js` around lines 117 -
121, The handleCopy handler starts a setTimeout to reset copied state but
doesn't clear it on unmount; store the timer id (e.g., in a ref) when calling
setTimeout inside handleCopy, and add a cleanup that clears the timeout on
unmount (useEffect cleanup) or when starting a new timeout so setCopied(false)
cannot run after the component unmounts; reference the handleCopy function, the
setCopied state setter, and redeemUrl to locate where to add the ref and the
clearTimeout cleanup.
ref https://linear.app/ghost/issue/BER-3484 After completing a Stripe gift checkout, Portal detects if the purchase was successful and displays a modal with the shareable redemption link
mike182uk
force-pushed
the
BER-3484-gift-purchase-success
branch
from
2ce058c to
89b4464
Compare
|
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
1 participant
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/ghost/issue/BER-3484
After completing a Stripe gift checkout, Portal detects if the purchase was successful and displays a modal with the shareable redemption link