Updated offers table to use Shade primitives#29534
Conversation
no ref Moves the dense offers minefield onto shared table, badge and layout primitives before the remaining legacy list and table components are removed.
WalkthroughThe offers modal now uses design-system table components, renders retention and signup offers through dedicated row components, and sorts the combined list by name, redemptions, or creation date. Discount and retention-term formatting is updated, retention offers expose creation timestamps, and acceptance tests cover sorting and row selection. Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 6m 22s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin-x-s... |
✅ Succeeded | 2m 20s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 21s | View ↗ |
nx run-many -t lint -p @tryghost/admin-x-settin... |
✅ Succeeded | 1m 5s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 17s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-22 16:10:34 UTC
no ref Sorting previously applied only to signup offers, leaving the two retention rows permanently pinned and making every option appear ineffective on small offer lists.
![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
🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsx (1)
100-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated button-cell markup between
RetentionOfferRowandSignupOfferRow.Both row components repeat the same
<TableCell className='p-0 whitespace-nowrap'><button className='block w-full cursor-pointer p-5 text-left' type='button' onClick={onClick}>...wrapper many times. Extracting a smallClickableCellhelper would reduce this repetition and centralize the click-cell styling.🤖 Prompt for 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. In `@apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsx` around lines 100 - 217, Extract the repeated clickable TableCell/button wrapper from RetentionOfferRow and SignupOfferRow into a shared ClickableCell helper, accepting onClick and rendering children. Replace the duplicated non-link cell markup in both row components with this helper while preserving each cell’s content, styling, and click behavior.
🤖 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/admin-x-settings/src/components/settings/growth/offers/offers-index.tsx`:
- Around line 227-259: Update sortOfferListItems so missing createdAt
comparisons assign their ordering to result instead of returning early, ensuring
those comparisons also pass through the sort-direction multiplier. Preserve the
existing placement rules for missing dates and the name/date sorting behavior
for populated dates.
- Around line 32-33: Update the formatPrice function to pass
minimumFractionDigits: 2 alongside maximumFractionDigits: 2 to formatNumber,
ensuring whole-dollar price and terms labels retain two decimal places.
---
Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsx`:
- Around line 100-217: Extract the repeated clickable TableCell/button wrapper
from RetentionOfferRow and SignupOfferRow into a shared ClickableCell helper,
accepting onClick and rendering children. Replace the duplicated non-link cell
markup in both row components with this helper while preserving each cell’s
content, styling, and click behavior.
🪄 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: 2f688b4d-3520-4fed-b728-38144465e1d1
📒 Files selected for processing (5)
apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsxapps/admin-x-settings/src/components/settings/growth/offers/offers-retention.tsxapps/admin/src/settings/offers.acceptance.test.tsxapps/admin/src/settings/offers.screen.tspackages/testing/test-data/src/selectors/offers.ts
| const formatToTwoDecimals = (num: number): number => parseFloat(num.toFixed(2)); | ||
| const formatPrice = (num: number): string => formatNumber(formatToTwoDecimals(currencyToDecimal(num)), {maximumFractionDigits: 2}); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== offers-index.tsx ==\n'
sed -n '1,220p' apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsx
printf '\n== search formatNumber definitions/usages ==\n'
rg -n "function formatNumber|const formatNumber|formatNumber\(" apps/admin-x-settings src packages -g '!**/node_modules/**' | sed -n '1,200p'Repository: TryGhost/Ghost
Length of output: 20328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsxRepository: TryGhost/Ghost
Length of output: 6205
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate shade formatNumber implementation ==\n'
rg -n "export .*formatNumber|function formatNumber|const formatNumber" . -g '!**/node_modules/**' | sed -n '1,200p'
printf '\n== currency price formatting call sites ==\n'
rg -n "maximumFractionDigits: 2|minimumFractionDigits: 2|formatNumber\(.*currencyToDecimal|currencyToDecimal\(.*formatNumber" apps/admin-x-settings packages apps -g '!**/node_modules/**' | sed -n '1,220p'Repository: TryGhost/Ghost
Length of output: 2515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '170,220p' apps/shade/src/lib/ds-utils.tsRepository: TryGhost/Ghost
Length of output: 1798
Preserve two decimal places in price labels. formatNumber(..., {maximumFractionDigits: 2}) will render whole-dollar values like 10.00 as 10; add minimumFractionDigits: 2 so the Price and Terms columns stay consistent.
🤖 Prompt for 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.
In `@apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsx`
around lines 32 - 33, Update the formatPrice function to pass
minimumFractionDigits: 2 alongside maximumFractionDigits: 2 to formatNumber,
ensuring whole-dollar price and terms labels retain two decimal places.
| const sortOfferListItems = (items: OfferListItem[], sortOption: string, sortDirection: string): OfferListItem[] => { | ||
| const multiplier = sortDirection === 'desc' ? -1 : 1; | ||
|
|
||
| return [...items].sort((item1, item2) => { | ||
| let result: number; | ||
|
|
||
| switch (sortOption) { | ||
| case 'name': | ||
| result = getOfferListItemName(item1).localeCompare(getOfferListItemName(item2)); | ||
| break; | ||
| case 'redemptions': | ||
| result = getOfferListItemRedemptions(item1) - getOfferListItemRedemptions(item2); | ||
| break; | ||
| default: { | ||
| const date1 = getOfferListItemCreatedAt(item1); | ||
| const date2 = getOfferListItemCreatedAt(item2); | ||
|
|
||
| if (!date1 && !date2) { | ||
| result = 0; | ||
| } else if (!date1) { | ||
| return 1; | ||
| } else if (!date2) { | ||
| return -1; | ||
| } else { | ||
| result = new Date(date1).getTime() - new Date(date2).getTime(); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return (result || getOfferListItemName(item1).localeCompare(getOfferListItemName(item2))) * multiplier; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sort direction is ignored for items with a missing createdAt.
The else if (!date1) { return 1; } / else if (!date2) { return -1; } branches return directly instead of assigning to result, so they never pass through * multiplier. Toggling ascending/descending has no effect on where null-date items land, while all other comparisons correctly flip. Since getRetentionOffers (offers-retention.tsx) can return createdAt: null for a cadence that has never had an offer, this is a realistic scenario, not just a theoretical edge case.
🐛 Proposed fix
if (!date1 && !date2) {
result = 0;
} else if (!date1) {
- return 1;
+ result = 1;
} else if (!date2) {
- return -1;
+ result = -1;
} else {
result = new Date(date1).getTime() - new Date(date2).getTime();
}
break;📝 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 sortOfferListItems = (items: OfferListItem[], sortOption: string, sortDirection: string): OfferListItem[] => { | |
| const multiplier = sortDirection === 'desc' ? -1 : 1; | |
| return [...items].sort((item1, item2) => { | |
| let result: number; | |
| switch (sortOption) { | |
| case 'name': | |
| result = getOfferListItemName(item1).localeCompare(getOfferListItemName(item2)); | |
| break; | |
| case 'redemptions': | |
| result = getOfferListItemRedemptions(item1) - getOfferListItemRedemptions(item2); | |
| break; | |
| default: { | |
| const date1 = getOfferListItemCreatedAt(item1); | |
| const date2 = getOfferListItemCreatedAt(item2); | |
| if (!date1 && !date2) { | |
| result = 0; | |
| } else if (!date1) { | |
| return 1; | |
| } else if (!date2) { | |
| return -1; | |
| } else { | |
| result = new Date(date1).getTime() - new Date(date2).getTime(); | |
| } | |
| break; | |
| } | |
| } | |
| return (result || getOfferListItemName(item1).localeCompare(getOfferListItemName(item2))) * multiplier; | |
| }); | |
| }; | |
| const sortOfferListItems = (items: OfferListItem[], sortOption: string, sortDirection: string): OfferListItem[] => { | |
| const multiplier = sortDirection === 'desc' ? -1 : 1; | |
| return [...items].sort((item1, item2) => { | |
| let result: number; | |
| switch (sortOption) { | |
| case 'name': | |
| result = getOfferListItemName(item1).localeCompare(getOfferListItemName(item2)); | |
| break; | |
| case 'redemptions': | |
| result = getOfferListItemRedemptions(item1) - getOfferListItemRedemptions(item2); | |
| break; | |
| default: { | |
| const date1 = getOfferListItemCreatedAt(item1); | |
| const date2 = getOfferListItemCreatedAt(item2); | |
| if (!date1 && !date2) { | |
| result = 0; | |
| } else if (!date1) { | |
| result = 1; | |
| } else if (!date2) { | |
| result = -1; | |
| } else { | |
| result = new Date(date1).getTime() - new Date(date2).getTime(); | |
| } | |
| break; | |
| } | |
| } | |
| return (result || getOfferListItemName(item1).localeCompare(getOfferListItemName(item2))) * multiplier; | |
| }); | |
| }; |
🤖 Prompt for 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.
In `@apps/admin-x-settings/src/components/settings/growth/offers/offers-index.tsx`
around lines 227 - 259, Update sortOfferListItems so missing createdAt
comparisons assign their ordering to result instead of returning early, ensuring
those comparisons also pass through the sort-direction multiplier. Preserve the
existing placement rules for missing dates and the name/date sorting behavior
for populated dates.
- main's offers table rework (#29534) fixed sorting to apply across signup and retention rows together and added acceptance coverage; ported the unified sort (name tiebreak, null dates last) and the offers-table-body testid into the native offers index dialog - main's color picker migration (#29538/#29542) renamed the hex input's accessible name to "Hex color" and the design suite now targets it; matched the native color picker field
1 participant
What changed
TableprimitivesBadgeWhy
The settings rip-out plan identified Offers as the dense table minefield in the lists/tables group. Splitting it into a focused PR keeps its interaction and visual review separate from the broader legacy
List/Tablecomponent deletion that follows.The existing sort implementation only sorted signup offers. Retention rows were permanently pinned, so on sites with one signup offer—such as the local fixture—every sorting option appeared to do nothing.
Impact
Signup and retention offers now form one sortable visible list. Archived filtering, editor navigation and redemption links are preserved. Retention rows use their aggregated redemption count and latest underlying creation date when sorting.
Validation
pnpm --filter @tryghost/admin-x-settings lintpnpm --filter @tryghost/admin-x-settings test:unit— 206 testspnpm --filter @tryghost/admin buildManual testing