Polished offers UI with consistent styling and improved navigation#26462
Polished offers UI with consistent styling and improved navigation#26462minimaluminium wants to merge 7 commits intomainfrom
Conversation
…n signup and retention offers
|
No actionable comments were generated in the recent review. 🎉 WalkthroughUI and spacing tweaks across offer-related components: ButtonSelect description margin adjusted; multiple modal sections reduced vertical spacing (mt-4 → mt-2) and a Toggle gained align='center'. AddOfferModal now initializes durationInMonths to 1 and wraps the duration input with a -mt-4 container plus placeholder/coercion behavior for zero. EditOfferModal adds a goBack navigation helper and replaces previewToolbar with breadcrumb-based navigation. Offers list and filter visuals updated (filter trigger button, row border color classes, status rendered as rounded badges). An e2e util now targets the duration input by test-id. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/add-offer-modal.tsx`:
- Around line 261-265: When duration === 'repeating' the durationInMonths field
can be cleared and coerced to 0 causing duration_in_months: 0 to be submitted;
update the validation in onValidate to explicitly reject falsy/zero values for
overrides.durationInMonths when overrides.duration === 'repeating' (e.g. check
overrides.durationInMonths > 0) and return a validation error message so the API
never receives 0, and ensure TextField/handleDurationInMonthsInput behavior
remains unchanged so the UI shows the validation state.
In
`@apps/admin-x-settings/src/components/settings/growth/offers/offers-retention.tsx`:
- Around line 81-83: The Active badge in offers-retention.tsx (the JSX <span>
rendering "Active") is missing dark-mode classes and should mirror the Inactive
badge's pattern; update the Active <span> (the one using
bg-[rgba(48,207,67,0.15)] and text-green) to include appropriate dark variants
such as a dark:bg-... and dark:text-... (matching contrast used for the Inactive
badge) so the green pill remains visible in dark mode while keeping the existing
light-mode rgba background and text-green classes.
| overrides.duration === 'repeating' && <div className='-mt-4'> | ||
| <TextField rightPlaceholder={`${overrides.durationInMonths === 1 ? 'month' : 'months'}`} type='number' value={overrides.durationInMonths === 0 ? '' : String(overrides.durationInMonths)} onChange={(e) => { | ||
| handleDurationInMonthsInput(e); | ||
| }} /> | ||
| </div> |
There was a problem hiding this comment.
Add validation for durationInMonths when duration is repeating.
Clearing this field stores 0 (via Number('')), and onValidate has no guard for it — duration_in_months: 0 would be submitted to the API. Now that the field defaults to 1 and is more visible, this gap is easier to trigger.
🛡️ Proposed fix — add guard in `onValidate`
if (formState.type === 'percent' && (formState.percentAmount < 0 || formState.percentAmount > 100)) {
newErrors.amount = 'Amount must be between 0 and 100%.';
}
+if (formState.duration === 'repeating' && (!formState.durationInMonths || formState.durationInMonths < 1)) {
+ newErrors.durationInMonths = 'Please enter a duration of at least 1 month.';
+}🤖 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/add-offer-modal.tsx`
around lines 261 - 265, When duration === 'repeating' the durationInMonths field
can be cleared and coerced to 0 causing duration_in_months: 0 to be submitted;
update the validation in onValidate to explicitly reject falsy/zero values for
overrides.durationInMonths when overrides.duration === 'repeating' (e.g. check
overrides.durationInMonths > 0) and return a validation error message so the API
never receives 0, and ensure TextField/handleDurationInMonthsInput behavior
remains unchanged so the UI shows the validation state.
| <span className='inline-flex items-center rounded-full bg-[rgba(48,207,67,0.15)] px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-green'>Active</span> | ||
| ) : ( | ||
| <span className='text-sm text-grey-700'>Inactive</span> | ||
| <span className='inline-flex items-center rounded-full bg-grey-200 px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-grey-700 dark:bg-grey-900 dark:text-grey-500'>Inactive</span> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Tailwind config and design-system files
find . -type f -name 'tailwind.config.*' -o -name 'theme*' -o -name 'config*' | grep -E '(tailwind|theme|config)' | head -20Repository: TryGhost/Ghost
Length of output: 1224
🏁 Script executed:
# Search for text-2xs definition in the codebase
rg 'text-2xs|"2xs"' --type ts --type tsx --type js --type json -B 2 -A 2 | head -100Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Check the actual file to confirm the code snippet
cat -n apps/admin-x-settings/src/components/settings/growth/offers/offers-retention.tsx | sed -n '75,90p'Repository: TryGhost/Ghost
Length of output: 1236
🏁 Script executed:
# Look for similar badge patterns in the codebase
rg 'className.*inline-flex.*rounded-full' --type tsx --type ts -B 1 -A 1 | head -60Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
# Search for text-2xs definition without file type specification
rg 'text-2xs|"2xs"' -B 2 -A 2 | head -150Repository: TryGhost/Ghost
Length of output: 5076
🏁 Script executed:
# Check the admin-x-settings Tailwind config
cat apps/admin-x-settings/tailwind.config.cjsRepository: TryGhost/Ghost
Length of output: 346
🏁 Script executed:
# Check admin-x-design-system Tailwind config
cat apps/admin-x-design-system/tailwind.config.cjsRepository: TryGhost/Ghost
Length of output: 11646
🏁 Script executed:
# Search for fontSize or theme config with 2xs
rg 'fontSize|2xs' apps/admin-x-design-system/ -A 2 -B 2 | head -100Repository: TryGhost/Ghost
Length of output: 3096
Active badge missing dark-mode background and text adjustments
The Active badge on line 81 lacks dark-mode color overrides, while the Inactive badge (line 83) includes them. In dark mode, the semi-transparent green background bg-[rgba(48,207,67,0.15)] may render poorly or become barely visible against a dark canvas. Add dark-mode variants to match the pattern used elsewhere.
🎨 Suggested fix
-<span className='inline-flex items-center rounded-full bg-[rgba(48,207,67,0.15)] px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-green'>Active</span>
+<span className='inline-flex items-center rounded-full bg-[rgba(48,207,67,0.15)] dark:bg-[rgba(48,207,67,0.08)] px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-green dark:text-green-400'>Active</span>📝 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.
| <span className='inline-flex items-center rounded-full bg-[rgba(48,207,67,0.15)] px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-green'>Active</span> | |
| ) : ( | |
| <span className='text-sm text-grey-700'>Inactive</span> | |
| <span className='inline-flex items-center rounded-full bg-grey-200 px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-grey-700 dark:bg-grey-900 dark:text-grey-500'>Inactive</span> | |
| <span className='inline-flex items-center rounded-full bg-[rgba(48,207,67,0.15)] dark:bg-[rgba(48,207,67,0.08)] px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-green dark:text-green-400'>Active</span> | |
| ) : ( | |
| <span className='inline-flex items-center rounded-full bg-grey-200 px-1.5 py-0.5 text-2xs font-semibold uppercase tracking-wide text-grey-700 dark:bg-grey-900 dark:text-grey-500'>Inactive</span> |
🤖 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/offers-retention.tsx`
around lines 81 - 83, The Active badge in offers-retention.tsx (the JSX <span>
rendering "Active") is missing dark-mode classes and should mirror the Inactive
badge's pattern; update the Active <span> (the one using
bg-[rgba(48,207,67,0.15)] and text-green) to include appropriate dark variants
such as a dark:bg-... and dark:text-... (matching contrast used for the Inactive
badge) so the green pill remains visible in dark mode while keeping the existing
light-mode rgba background and text-green classes.
minimaluminium
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/admin-x-settings/src/components/settings/growth/offers/edit-offer-modal.tsx (1)
268-291:⚠️ Potential issue | 🟡 Minor
afterClosebypassesgoBack, creating inconsistent navigation behavior.
afterClose(line 270) always callsupdateRoute('offers'), whileonCancelandonBreadcrumbsBackboth callgoBack, which may route to'offers/edit'. If a user dismisses the modal via the backdrop, they land on a different page than if they click Cancel. Verify this inconsistency is intentional; if not, unify them.🤖 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-offer-modal.tsx` around lines 268 - 291, The afterClose prop currently calls updateRoute('offers') which diverges from onCancel and onBreadcrumbsBack that call goBack; change afterClose to use the same navigation handler as the other cancel flows (i.e., call goBack or delegate to the same handler used by onCancel/onBreadcrumbsBack) so dismissing the modal via backdrop uses identical routing behavior—update the call site for afterClose in the PreviewModalContent usage to invoke goBack (or a shared function) instead of updateRoute('offers').
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/growth/offers/edit-offer-modal.tsx (1)
258-266: DRY:sessionStorage.removeItemis duplicated in both branches.Both the
ifandelsearms unconditionally remove the same key — extract it before the conditional.♻️ Proposed refactor
const goBack = () => { + const source = sessionStorage.getItem('editOfferPageSource'); + sessionStorage.removeItem('editOfferPageSource'); - if (sessionStorage.getItem('editOfferPageSource') === 'offers') { - sessionStorage.removeItem('editOfferPageSource'); + if (source === 'offers') { updateRoute('offers'); } else { - sessionStorage.removeItem('editOfferPageSource'); updateRoute('offers/edit'); } };🤖 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-offer-modal.tsx` around lines 258 - 266, The goBack function duplicates sessionStorage.removeItem('editOfferPageSource') in both branches; refactor by removing the key once before the conditional and then call updateRoute based on the value — e.g., read const source = sessionStorage.getItem('editOfferPageSource'), call sessionStorage.removeItem('editOfferPageSource') once, then if (source === 'offers') updateRoute('offers') else updateRoute('offers/edit'); keep function name goBack and the same updateRoute calls.
🤖 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-offer-modal.tsx`:
- Around line 258-266: The goBack function currently treats any non-'offers'
value (including null) as an instruction to navigate to 'offers/edit'; change it
to read const src = sessionStorage.getItem('editOfferPageSource'), remove the
item, then: if (src === 'offers') call updateRoute('offers'), else if (src ===
'offers/edit') call updateRoute('offers/edit'), otherwise call
updateRoute('offers') as the safe default; keep the sessionStorage.removeItem
call in the same flow. This uses the existing goBack,
sessionStorage.getItem('editOfferPageSource') and updateRoute(...) symbols to
make the default behavior explicit.
- Around line 258-266: The goBack() handler relies on
sessionStorage.getItem('editOfferPageSource') and falls back to
updateRoute('offers/edit'), causing unreliable behavior when other code paths
(e.g., openOfferListModal(), OfferSuccess navigation, tab switches in
offers-index-retention.tsx) don't set that key; replace this fragile
sessionStorage coupling by passing a scoped route query param (e.g.,
?source=offers or ?source=offers-edit) or by introducing a small React
context/provider that stores the originating page and is consumed by goBack();
update all entry points that currently call updateRoute('offers/edit') or open
the edit modal (openOfferListModal, OfferSuccess navigation, tab switch
handlers) to set the query param or context value when navigating/opening,
remove reads/writes to sessionStorage from edit-offer-modal.tsx (goBack) and
instead read the query param via the router or the context to decide whether to
navigate back to 'offers' or 'offers/edit'.
---
Outside diff comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-offer-modal.tsx`:
- Around line 268-291: The afterClose prop currently calls updateRoute('offers')
which diverges from onCancel and onBreadcrumbsBack that call goBack; change
afterClose to use the same navigation handler as the other cancel flows (i.e.,
call goBack or delegate to the same handler used by onCancel/onBreadcrumbsBack)
so dismissing the modal via backdrop uses identical routing behavior—update the
call site for afterClose in the PreviewModalContent usage to invoke goBack (or a
shared function) instead of updateRoute('offers').
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-offer-modal.tsx`:
- Around line 258-266: The goBack function duplicates
sessionStorage.removeItem('editOfferPageSource') in both branches; refactor by
removing the key once before the conditional and then call updateRoute based on
the value — e.g., read const source =
sessionStorage.getItem('editOfferPageSource'), call
sessionStorage.removeItem('editOfferPageSource') once, then if (source ===
'offers') updateRoute('offers') else updateRoute('offers/edit'); keep function
name goBack and the same updateRoute calls.
| const goBack = () => { | ||
| if (sessionStorage.getItem('editOfferPageSource') === 'offers') { | ||
| sessionStorage.removeItem('editOfferPageSource'); | ||
| updateRoute('offers'); | ||
| } else { | ||
| sessionStorage.removeItem('editOfferPageSource'); | ||
| updateRoute('offers/edit'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Wrong default route when editOfferPageSource is absent.
sessionStorage.getItem('editOfferPageSource') returns null when the key was never written (direct navigation, fresh tab, cleared storage). null !== 'offers', so the else branch fires and sends the user to 'offers/edit' instead of 'offers'. The safer and more intuitive default should be 'offers'.
🐛 Proposed fix
const goBack = () => {
+ sessionStorage.removeItem('editOfferPageSource');
if (sessionStorage.getItem('editOfferPageSource') === 'offers') {
- sessionStorage.removeItem('editOfferPageSource');
updateRoute('offers');
} else {
- sessionStorage.removeItem('editOfferPageSource');
- updateRoute('offers/edit');
+ updateRoute('offers');
}
};If 'offers/edit' as a fallback is intentional for a specific source value, introduce an explicit check for that case instead of relying on the else-branch default.
📝 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.
| const goBack = () => { | |
| if (sessionStorage.getItem('editOfferPageSource') === 'offers') { | |
| sessionStorage.removeItem('editOfferPageSource'); | |
| updateRoute('offers'); | |
| } else { | |
| sessionStorage.removeItem('editOfferPageSource'); | |
| updateRoute('offers/edit'); | |
| } | |
| }; | |
| const goBack = () => { | |
| const source = sessionStorage.getItem('editOfferPageSource'); | |
| sessionStorage.removeItem('editOfferPageSource'); | |
| if (source === 'offers') { | |
| updateRoute('offers'); | |
| } else { | |
| updateRoute('offers'); | |
| } | |
| }; |
🤖 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-offer-modal.tsx`
around lines 258 - 266, The goBack function currently treats any non-'offers'
value (including null) as an instruction to navigate to 'offers/edit'; change it
to read const src = sessionStorage.getItem('editOfferPageSource'), remove the
item, then: if (src === 'offers') call updateRoute('offers'), else if (src ===
'offers/edit') call updateRoute('offers/edit'), otherwise call
updateRoute('offers') as the safe default; keep the sessionStorage.removeItem
call in the same flow. This uses the existing goBack,
sessionStorage.getItem('editOfferPageSource') and updateRoute(...) symbols to
make the default behavior explicit.
🧩 Analysis chain
🏁 Script executed:
rg -n "editOfferPageSource" --type=ts --type=tsx -A 2 -B 2Repository: TryGhost/Ghost
Length of output: 85
🏁 Script executed:
rg -n "editOfferPageSource" -A 2 -B 2Repository: TryGhost/Ghost
Length of output: 2905
🏁 Script executed:
rg "offers/edit" --type-not test -A 1 -B 1Repository: TryGhost/Ghost
Length of output: 86
🏁 Script executed:
rg "offers/edit" -A 1 -B 1 | head -80Repository: TryGhost/Ghost
Length of output: 7373
Unguarded navigation paths to offers/edit cause unreliable fallback behavior in goBack().
Multiple code paths navigate to offers/edit without first setting editOfferPageSource in sessionStorage (e.g., openOfferListModal() in offers.tsx, OfferSuccess modal navigation, and tab switches in offers-index-retention.tsx). When goBack() reads a missing or stale sessionStorage value, it falls through to the else clause and calls updateRoute('offers/edit') again, which can cause navigation loops or unexpected behavior.
Replace sessionStorage-based routing state with route query parameters or a scoped React context to eliminate the coupling between multiple entry points and the goBack handler.
🤖 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-offer-modal.tsx`
around lines 258 - 266, The goBack() handler relies on
sessionStorage.getItem('editOfferPageSource') and falls back to
updateRoute('offers/edit'), causing unreliable behavior when other code paths
(e.g., openOfferListModal(), OfferSuccess navigation, tab switches in
offers-index-retention.tsx) don't set that key; replace this fragile
sessionStorage coupling by passing a scoped route query param (e.g.,
?source=offers or ?source=offers-edit) or by introducing a small React
context/provider that stores the originating page and is consumed by goBack();
update all entry points that currently call updateRoute('offers/edit') or open
the edit modal (openOfferListModal, OfferSuccess navigation, tab switch
handlers) to set the query param or context value when navigating/opening,
remove reads/writes to sessionStorage from edit-offer-modal.tsx (goBack) and
instead read the query param via the router or the context to decide whether to
navigate back to 'offers' or 'offers/edit'.
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 3. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
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 `@ghost/core/test/e2e-browser/utils/e2e-browser-utils.js`:
- Around line 301-302: The selector chain using getByText('month', {exact:
true}).locator('..').locator('input[type="number"]') is brittle; update the UI
to add data-testid="duration-months-input" on the numeric duration input and
replace the fragile locator with a resilient
getByTestId('duration-months-input') usage (update the variable durationInput
and any code that fills it) so tests no longer rely on parent traversal or exact
text matching.
1 participant
no issues