Merged
Conversation
minimaluminium
changed the title
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:
WalkthroughRetention-offer UI placeholder text was adjusted. The retention-offer creation now builds a descriptive name using a short random hash and a computed offer description (free months or percent with duration) instead of the previous generic name. Free-month offer components accept a subscription prop, use optional chaining to derive and format a renewal date from subscription.current_period_end, and render an OfferTagIcon alongside the label. getOfferMessage gained a subscription parameter. Test fixtures and assertions were updated to reflect the new date formatting and offer-name pattern. 🚥 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)
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: 3
🧹 Nitpick comments (2)
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx (1)
242-242: Prefer double-quoted attribute over the'HTML entity.
'in a single-quoted JSX string literal is decoded correctly by Babel at compile time, so this is functionally valid. That said, switching to a double-quoted attribute avoids the entity entirely and is more idiomatic JSX.♻️ Suggested cleanup
- placeholder='We'd hate to see you leave. How about a special offer to stay?' + placeholder="We'd hate to see you leave. How about a special offer to stay?"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx` at line 242, In the EditRetentionOfferModal JSX, replace the single-quoted placeholder attribute that uses the ' entity with a double-quoted attribute containing the raw apostrophe (e.g., change placeholder='We'd hate to see you leave...' to placeholder="We'd hate to see you leave...") so the string is more idiomatic JSX and removes the HTML entity; update the placeholder on the same JSX element in the render/return of EditRetentionOfferModal.apps/portal/test/unit/components/pages/account-plan-page.test.js (1)
396-439: Explicitly setcurrentPeriodEndin the test to make the date derivation self-contained.The assertion on line 438 hardcodes
"5 Nov 2022"while relying on the defaultcurrentPeriodEnd: '2022-10-05T03:18:30.000Z'baked into the fixture. If that default changes, the test fails with a confusing date mismatch rather than a clear explanation of what changed.♻️ Suggested improvement
+ const currentPeriodEnd = '2022-10-05T03:18:30.000Z'; // Oct 5 + 1 month = Nov 5 const member = getMemberData({ paid: true, subscriptions: [ getSubscriptionData({ status: 'active', interval: 'month', amount: paidProduct.monthlyPrice.amount, currency: 'USD', - priceId: paidProduct.monthlyPrice.id + priceId: paidProduct.monthlyPrice.id, + currentPeriodEnd }) ] });- expect(queryByText('Enjoy 1 month free on us. Your next billing date will be 5 Nov 2022.')).toBeInTheDocument(); + expect(queryByText('Enjoy 1 month free on us. Your next billing date will be 5 Nov 2022.')).toBeInTheDocument(); // Nov 2022 derived from currentPeriodEnd above🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/test/unit/components/pages/account-plan-page.test.js` around lines 396 - 439, The test "renders free months retention offers" relies on a fixture default currentPeriodEnd which can change; make the date derivation explicit by setting currentPeriodEnd on the subscription returned by getSubscriptionData when building member.subscriptions (e.g., add currentPeriodEnd: '2022-10-05T03:18:30.000Z' or another fixed ISO) so the expectation for "Your next billing date will be 5 Nov 2022." is deterministic; update the member creation where getSubscriptionData(...) is called inside getMemberData to include this field.
🤖 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/components/pages/account-plan-page.js`:
- Around line 254-266: The billing message in getOfferMessage can produce a
dangling sentence when subscription or subscription.current_period_end is
missing and can compute incorrect dates using date.setMonth(date.getMonth() +
months); fix by: when subscription?.current_period_end is falsy, return a
message that omits the billing date clause (or uses a fallback like "when your
next billing cycle starts"); and when computing the new billing date, replace
setMonth arithmetic with a robust end-of-month aware increment (compute target
year/month by adding months to current month, then set the day to the
min(originalDay, daysInTargetMonth) before creating the new Date) so
newBillingDate is accurate; use the existing symbols getOfferMessage,
subscription, current_period_end, newBillingDate, setMonth and getDateString to
locate and update the logic.
In
`@apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js`:
- Around line 205-210: FreeMonthsLabel builds its renewal date from
subscription?.trial_end_at which is for free trials and will often be undefined
for free-months offers; change it to use the subscription's next billing date
(subscription.current_period_end or the same value derived in
account-plan-page.js) so renewalDate is populated correctly; update the
reference in FreeMonthsLabel (and keep nextPayment and subscription parameters)
to compute renewalDate from subscription.current_period_end instead of
trial_end_at.
- Line 206: The assignment to months uses direct property access on nextPayment
and can throw if nextPayment is undefined; update the months assignment in
paid-account-actions.js (the line that sets const months =
nextPayment.discount.amount) to guard against missing nextPayment by using
optional chaining or an explicit null check and provide a safe default (e.g., 0)
when nextPayment or discount is absent; ensure this change aligns with the logic
in subscriptionHasFreeMonthsOffer so the rest of the code handles the default
correctly.
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx`:
- Line 242: In the EditRetentionOfferModal JSX, replace the single-quoted
placeholder attribute that uses the ' entity with a double-quoted attribute
containing the raw apostrophe (e.g., change placeholder='We'd hate to see
you leave...' to placeholder="We'd hate to see you leave...") so the string is
more idiomatic JSX and removes the HTML entity; update the placeholder on the
same JSX element in the render/return of EditRetentionOfferModal.
In `@apps/portal/test/unit/components/pages/account-plan-page.test.js`:
- Around line 396-439: The test "renders free months retention offers" relies on
a fixture default currentPeriodEnd which can change; make the date derivation
explicit by setting currentPeriodEnd on the subscription returned by
getSubscriptionData when building member.subscriptions (e.g., add
currentPeriodEnd: '2022-10-05T03:18:30.000Z' or another fixed ISO) so the
expectation for "Your next billing date will be 5 Nov 2022." is deterministic;
update the member creation where getSubscriptionData(...) is called inside
getMemberData to include this field.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsxapps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.jsapps/portal/src/components/pages/account-plan-page.jsapps/portal/src/utils/fixtures.jsapps/portal/test/unit/components/pages/account-plan-page.test.js
apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js
Outdated
Show resolved
Hide resolved
apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/portal/src/components/pages/account-plan-page.js (1)
254-271: Billing date computation: previously flagged issues correctly addressed.Both concerns from the prior round — the dangling-period sentence when
subscriptionis absent and theDate.setMonthend-of-month rollover — are properly resolved. The manual year/month arithmetic (Math.floor(targetMonth / 12)/targetMonth % 12+Math.min(originalDay, daysInTargetMonth)) handles edge cases like Oct 31 + 1 → Nov 30 correctly, and the guard onsubscription?.current_period_endcleanly omits the date clause rather than producing a broken sentence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/components/pages/account-plan-page.js` around lines 254 - 271, The getOfferMessage implementation (function getOfferMessage) correctly avoids the dangling period by guarding on subscription?.current_period_end and properly computes the new billing date using manual month/year arithmetic and Math.min to handle end-of-month rollovers; no changes required—keep the current subscription guard and the targetMonth/targetYear/daysInTargetMonth logic as-is.
🤖 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/components/pages/AccountHomePage/components/paid-account-actions.js`:
- Around line 207-210: FreeMonthsLabel currently builds a label using
renewalDate = getDateString(subscription?.current_period_end) which can be empty
and produces a trailing " - Renews " with no date; update the label construction
in FreeMonthsLabel (the code that sets renewalDate, months and label) to
conditionally append the " - Renews {renewalDate}" clause only when
subscription?.current_period_end (or renewalDate) is present, mirroring the
guard used in getOfferMessage so the label becomes either "1 month free" / "N
months free" or "1 month free - Renews {date}" / "N months free - Renews {date}"
as appropriate.
---
Duplicate comments:
In `@apps/portal/src/components/pages/account-plan-page.js`:
- Around line 254-271: The getOfferMessage implementation (function
getOfferMessage) correctly avoids the dangling period by guarding on
subscription?.current_period_end and properly computes the new billing date
using manual month/year arithmetic and Math.min to handle end-of-month
rollovers; no changes required—keep the current subscription guard and the
targetMonth/targetYear/daysInTargetMonth logic as-is.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.jsapps/portal/src/components/pages/account-plan-page.js
apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/portal/src/components/pages/account-plan-page.js (1)
254-271: Both past issues resolved — UTC arithmetic is correct.The end-of-month clamping (
Date.UTC(targetYear, targetMonth + 1, 0)) and fallback to a clean sentence whencurrent_period_endis absent both work correctly.One minor inconsistency:
toLocaleDateString('en-GB', ...)is used here whilegetDateStringis used for every other date in this file (lines 131, 179). ThetimeZone: 'UTC'option is the key reason for deviating, but it's worth checking whethergetDateStringcould be extended to accept a timezone option rather than forking the formatting path — or at minimum confirming'en-GB'produces the same visual format asgetDateString.#!/bin/bash # Verify getDateString implementation and check for any other getOfferMessage callers fd -e js "date-time" --exec cat {} echo "---" rg -n "getOfferMessage" --type=js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/components/pages/account-plan-page.js` around lines 254 - 271, The date formatting in getOfferMessage uses toLocaleDateString('en-GB', {timeZone: 'UTC'}) which is inconsistent with the rest of the file that uses getDateString; update getDateString to accept an optional timezone/format options parameter (e.g., timeZone: 'UTC') and then replace the toLocaleDateString usage in getOfferMessage with a call to getDateString(subscription.current_period_end, { timeZone: 'UTC' }) so all dates use the same helper; ensure getDateString retains its current default behavior when no options are provided and that getOfferMessage still clamps end-of-month as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/portal/src/components/pages/account-plan-page.js`:
- Around line 254-271: The date formatting in getOfferMessage uses
toLocaleDateString('en-GB', {timeZone: 'UTC'}) which is inconsistent with the
rest of the file that uses getDateString; update getDateString to accept an
optional timezone/format options parameter (e.g., timeZone: 'UTC') and then
replace the toLocaleDateString usage in getOfferMessage with a call to
getDateString(subscription.current_period_end, { timeZone: 'UTC' }) so all dates
use the same helper; ensure getDateString retains its current default behavior
when no options are provided and that getOfferMessage still clamps end-of-month
as before.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.jsapps/portal/src/components/pages/account-plan-page.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/portal/src/components/pages/AccountHomePage/components/paid-account-actions.js
Replaced 'Before you go...' with 'Before you go', changed 'see you go' to 'see you leave', updated free month offer message to show the actual next billing date instead of 'pushed back by N days', and added renewal date to the free months label on the account page.
The preview member subscription used a hardcoded date from 2021, causing the retention offer preview to show unrealistic past dates.
trial_end_at is for free trials and is often undefined for free-months offers; current_period_end is the correct field for the renewal date
- Used end-of-month aware date arithmetic to prevent setMonth rollover bugs - Handled missing subscription data gracefully by omitting billing date clause
The date arithmetic used local timezone methods which produced different dates depending on the runner's timezone, breaking CI.
minimaluminium
force-pushed
the
retention-offer-copy-update-ber-3259-3260
branch
from
b9610e4 to
e97306f
Compare
sagzy
approved these changes
Feb 24, 2026
| return `Enjoy ${monthLabel} free on us. Your next billing date will be ${newBillingDate}.`; | ||
| } | ||
|
|
||
| return `Enjoy ${monthLabel} free on us.`; |
Contributor
There was a problem hiding this comment.
Peter used "Enjoy 1 free month on us" (instead of "Enjoy 1 month free on us")
https://linear.app/ghost/issue/BER-3259/copy-review-for-retention-offer-popup#comment-f3fadae4
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx`:
- Around line 446-455: The repeating-duration text uses plural "months"
indiscriminately; update the logic around formTerms.duration === 'repeating'
(and the durationInMonths value) to choose "month" when
formTerms.durationInMonths === 1 and "months" otherwise, e.g. build durationText
using a conditional on formTerms.durationInMonths so offerDesc becomes `for 1
month` when durationInMonths === 1 and `for N months` for other values; ensure
the change is applied where durationText is computed before offerDesc is set.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsxapps/admin-x-settings/test/acceptance/membership/offers.test.tsapps/portal/src/components/pages/account-plan-page.js
Comment on lines
446
to
455
| let durationText: string; | ||
| if (formTerms.duration === 'once') { | ||
| durationText = 'next payment'; | ||
| } else if (formTerms.duration === 'repeating') { | ||
| durationText = `for ${formTerms.durationInMonths} months`; | ||
| } else { | ||
| durationText = 'forever'; | ||
| } | ||
| offerDesc = `${formTerms.amount}% off ${durationText}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix singular/plural for repeating duration text.
For a 1‑month repeating offer, the name currently becomes “for 1 months.” Use a singular label when durationInMonths === 1.
📝 Proposed fix
- } else if (formTerms.duration === 'repeating') {
- durationText = `for ${formTerms.durationInMonths} months`;
+ } else if (formTerms.duration === 'repeating') {
+ const months = formTerms.durationInMonths;
+ const monthLabel = months === 1 ? 'month' : 'months';
+ durationText = `for ${months} ${monthLabel}`;📝 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
| let durationText: string; | |
| if (formTerms.duration === 'once') { | |
| durationText = 'next payment'; | |
| } else if (formTerms.duration === 'repeating') { | |
| durationText = `for ${formTerms.durationInMonths} months`; | |
| } else { | |
| durationText = 'forever'; | |
| } | |
| offerDesc = `${formTerms.amount}% off ${durationText}`; | |
| } | |
| let durationText: string; | |
| if (formTerms.duration === 'once') { | |
| durationText = 'next payment'; | |
| } else if (formTerms.duration === 'repeating') { | |
| const months = formTerms.durationInMonths; | |
| const monthLabel = months === 1 ? 'month' : 'months'; | |
| durationText = `for ${months} ${monthLabel}`; | |
| } else { | |
| durationText = 'forever'; | |
| } | |
| offerDesc = `${formTerms.amount}% off ${durationText}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx`
around lines 446 - 455, The repeating-duration text uses plural "months"
indiscriminately; update the logic around formTerms.duration === 'repeating'
(and the durationInMonths value) to choose "month" when
formTerms.durationInMonths === 1 and "months" otherwise, e.g. build durationText
using a conditional on formTerms.durationInMonths so offerDesc becomes `for 1
month` when durationInMonths === 1 and `for N months` for other values; ensure
the change is applied where durationText is computed before offerDesc is set.
sagzy
reviewed
Feb 24, 2026
| const createRetentionOffer = async (status: 'active' | 'archived') => { | ||
| // Generate a random 8-character hex string | ||
| const hash = Array.from(crypto.getRandomValues(new Uint8Array(4)), b => b.toString(16).padStart(2, '0')).join(''); | ||
| const shortHash = hash.slice(0, 4); |
Contributor
There was a problem hiding this comment.
nitpick:
Could use
const hash = crypto.getRandomValues(new Uint16Array(1))[0].toString(16).padStart(4, '0')
directly here, instead of generating a 8 char hash and then slicing
sagzy
approved these changes
Feb 24, 2026
Changelog for v2.64.8 -> 2.64.9:
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/ghost/issue/BER-3259/copy-review-for-retention-offer-popup
ref https://linear.app/ghost/issue/BER-3260/copy-review-for-member-account-page
Replaced 'Before you go...' with 'Before you go', changed 'see you go' to 'see you leave', updated free month offer message to show the actual next billing date instead of 'pushed back by N days', and added renewal date to the free months label on the account page.