Include site title in members export filename#28358
Conversation
|
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 selected for processing (1)
WalkthroughThis PR adds the site title to the members CSV export filename to match the naming convention of JSON content exports. A new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/posts/src/views/members/components/members-actions.tsx (1)
191-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
siteTitlein the backup export flow too.
handleExportBackupstill callsexportMembers(nql, search)withoutsiteTitle, so backup exports keep the oldghost.members...filename and diverge from the main export action.Proposed fix
const handleExportBackup = useCallback(async () => { try { - await exportMembers(nql, search); + await exportMembers(nql, search, siteTitle); } catch (e) { toast.error('Export failed', { description: 'There was a problem downloading your backup. Please check your connection and try again.' }); throw e; } -}, [nql, search]); +}, [nql, search, siteTitle]);🤖 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/posts/src/views/members/components/members-actions.tsx` around lines 191 - 200, handleExportBackup currently calls exportMembers(nql, search) without the siteTitle so backup filenames use the old ghost.members... name; update the call inside handleExportBackup to include the siteTitle argument (i.e., exportMembers(nql, search, siteTitle)) and ensure siteTitle is in the hook’s closure (add it to the useCallback dependency array if not already present) so the backup export uses the correct filename.
🧹 Nitpick comments (1)
apps/posts/test/unit/views/members/members-actions.test.tsx (1)
22-31: ⚡ Quick winAdd regression coverage for the
onExportBackupcallback path.The new tests assert
exportMembers()directly, but they don’t cover theDeleteModal-driven backup export path inMembersActions. CapturingDeleteModalprops and invokingonExportBackupwould have caught the missingsiteTitlepropagation regression.Also applies to: 97-107
🤖 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/posts/test/unit/views/members/members-actions.test.tsx` around lines 22 - 31, The test is missing coverage for the DeleteModal-driven export path: add a captured props ref for DeleteModal (e.g., deleteModalPropsRef.current) in the same mock block that captures ImportMembersModal, set DeleteModal to a function that stores props and returns a simple element, then in the test call MembersActions/trigger the branch that renders DeleteModal and invoke deleteModalPropsRef.current.onExportBackup(...); assert that exportMembers was called and that the payload includes the expected siteTitle to catch the regression where siteTitle wasn't propagated. Ensure you reference the DeleteModal mock, deleteModalPropsRef, onExportBackup, exportMembers and MembersActions when making the change.
🤖 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/posts/src/views/members/components/members-actions.tsx`:
- Around line 25-29: The current titleSlug computation drops non-ASCII
characters because it restricts to [a-z0-9]; update members-actions.tsx to use
the project’s Unicode-aware slugification used by other exports (or a
Unicode-safe slug strategy) instead of that regex: replace the titleSlug logic
that depends on siteTitle?.toLowerCase().replace(/[^a-z0-9]+/g,
'-').replace(/^-|-$/g, '') with a call to the shared slug function (or a
Unicode-safe slugifier) so titleSlug (and consequently titlePrefix) preserves
non-Latin titles like 日本語ブログ and produces a non-empty prefix when siteTitle
exists (refer to the existing slug utility or export code for the exact function
name to reuse).
---
Outside diff comments:
In `@apps/posts/src/views/members/components/members-actions.tsx`:
- Around line 191-200: handleExportBackup currently calls exportMembers(nql,
search) without the siteTitle so backup filenames use the old ghost.members...
name; update the call inside handleExportBackup to include the siteTitle
argument (i.e., exportMembers(nql, search, siteTitle)) and ensure siteTitle is
in the hook’s closure (add it to the useCallback dependency array if not already
present) so the backup export uses the correct filename.
---
Nitpick comments:
In `@apps/posts/test/unit/views/members/members-actions.test.tsx`:
- Around line 22-31: The test is missing coverage for the DeleteModal-driven
export path: add a captured props ref for DeleteModal (e.g.,
deleteModalPropsRef.current) in the same mock block that captures
ImportMembersModal, set DeleteModal to a function that stores props and returns
a simple element, then in the test call MembersActions/trigger the branch that
renders DeleteModal and invoke deleteModalPropsRef.current.onExportBackup(...);
assert that exportMembers was called and that the payload includes the expected
siteTitle to catch the regression where siteTitle wasn't propagated. Ensure you
reference the DeleteModal mock, deleteModalPropsRef, onExportBackup,
exportMembers and MembersActions when making the change.
🪄 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: fd4d3830-1baf-4ee2-9134-1a7c73bf6cfb
📒 Files selected for processing (3)
apps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/members.tsxapps/posts/test/unit/views/members/members-actions.test.tsx
| const titleSlug = siteTitle | ||
| ?.toLowerCase() | ||
| .replace(/[^a-z0-9]+/g, '-') | ||
| .replace(/^-|-$/g, ''); | ||
| const titlePrefix = titleSlug ? `${titleSlug}.` : ''; |
There was a problem hiding this comment.
Non-ASCII site titles are dropped from the filename prefix.
At Line 25-Line 29, the slug regex only keeps [a-z0-9]. Titles like 日本語ブログ become an empty slug and incorrectly fall back to ghost.members... even though siteTitle is present. Please switch this path to the same slugification behavior used by other exports (or a Unicode-safe slug strategy) so non-empty titles consistently contribute a prefix.
🤖 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/posts/src/views/members/components/members-actions.tsx` around lines 25
- 29, The current titleSlug computation drops non-ASCII characters because it
restricts to [a-z0-9]; update members-actions.tsx to use the project’s
Unicode-aware slugification used by other exports (or a Unicode-safe slug
strategy) instead of that regex: replace the titleSlug logic that depends on
siteTitle?.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-|-$/g, '') with
a call to the shared slug function (or a Unicode-safe slugifier) so titleSlug
(and consequently titlePrefix) preserves non-Latin titles like 日本語ブログ and
produces a non-empty prefix when siteTitle exists (refer to the existing slug
utility or export code for the exact function name to reuse).
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
LGTM, thanks! I'm going to follow up with another PR to unify our handling, since we're now re-creating this title handling (not to mention streaming exports) in multiple places.
ref TryGhost#28310 - the members CSV export filename now follows `<site>.ghost.members.<date>.csv`, so the previous `startsWith('members')` assertion no longer holds and the e2e suite failed - assert the `ghost.members.<date>.csv` convention instead, which holds whether or not a site title prefix is present
2 participants
Fixes #28310
Summary
titlevalue from the members page into the export actionghost.members.YYYY-MM-DD.csvfallback when the site title is unavailableVerification
pnpm vitest run test/unit/views/members/members-actions.test.tsxfromapps/posts(7 tests passed)pnpm --dir apps/posts exec eslint src/views/members/components/members-actions.tsx test/unit/views/members/members-actions.test.tsxapps/posts/src/views/members/members.tsxpnpm --dir apps/posts exec tsc --noEmitgit diff --checkpnpm --filter @tryghost/posts buildgot through TypeScript and then failed in Vite/Rollup on virtual\0reactwith Nodev24.15.0; this checkout warns the repo expects Node^22.13.1AI-assisted: yes. I reviewed the generated changes and test output before opening this PR.