fix: include site name in members CSV export filename (#28310)#28362
Open
amal-hafis wants to merge 1 commit into
Open
fix: include site name in members CSV export filename (#28310)#28362amal-hafis wants to merge 1 commit into
amal-hafis wants to merge 1 commit into
Conversation
Contributor
WalkthroughThe members API endpoint now imports Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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 `@ghost/core/core/server/api/endpoints/members.js`:
- Around line 383-384: Compute the slug once and base the fallback on the slug
result (not raw title): call slugify(title) into a local variable (e.g., slug),
then set titlePrefix = slug ? `${slug}.` : '' so you avoid emitting a leading
dot when slugify(title) returns an empty string; keep the existing return
`${titlePrefix}ghost.members.${datetime}.csv`.
- Around line 382-385: The endpoint builds a prefixed filename (the const that
returns `${titlePrefix}ghost.members.${datetime}.csv`) but the stream serializer
membersCsvSerializer in serializers/output/members.js still hardcodes
Content-Disposition to `members.{YYYY-MM-DD}.csv`, so the header is overridden;
update the stream serializer to accept a filename option (or read
resp.locals.filename) and use that value when setting Content-Disposition, and
update the members endpoint (where the filename is constructed) to pass the
computed filename into membersCsvSerializer (or set resp.locals.filename) so the
serializer uses the endpoint-provided name rather than the hardcoded one.
🪄 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: ed613f9f-887a-4600-b537-34812b2d4aa3
📒 Files selected for processing (1)
ghost/core/core/server/api/endpoints/members.js
Comment on lines
+382
to
385
| const title = settingsCache.get('title'); | ||
| const titlePrefix = title ? `${slugify(title)}.` : ''; | ||
| return `${titlePrefix}ghost.members.${datetime}.csv`; | ||
| } |
Contributor
There was a problem hiding this comment.
Filename prefix logic may never reach clients because CSV serializer hardcodes Content-Disposition.
At Line 384 you return a prefixed filename, but ghost/core/core/server/api/endpoints/utils/serializers/output/members.js (Lines 469-493 in provided context) still sets Content-Disposition to members.{YYYY-MM-DD}.csv. If that stream serializer runs for this endpoint, it overrides this PR’s header contract and the new filename format won’t be applied.
Suggested fix (propagate endpoint filename into stream serializer)
# ghost/core/core/server/api/endpoints/utils/serializers/output/members.js
- res.setHeader('Content-Disposition', `attachment; filename="members.${datetime}.csv"`);
+ const fallbackFilename = `members.${datetime}.csv`;
+ const disposition = res.getHeader('Content-Disposition');
+ if (!disposition) {
+ res.setHeader('Content-Disposition', `attachment; filename="${fallbackFilename}"`);
+ }# ghost/core/core/server/api/endpoints/members.js
value() {
const datetime = (new Date()).toJSON().substring(0, 10);
const title = settingsCache.get('title');
const titlePrefix = title ? `${slugify(title)}.` : '';
return `${titlePrefix}ghost.members.${datetime}.csv`;
}🤖 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 `@ghost/core/core/server/api/endpoints/members.js` around lines 382 - 385, The
endpoint builds a prefixed filename (the const that returns
`${titlePrefix}ghost.members.${datetime}.csv`) but the stream serializer
membersCsvSerializer in serializers/output/members.js still hardcodes
Content-Disposition to `members.{YYYY-MM-DD}.csv`, so the header is overridden;
update the stream serializer to accept a filename option (or read
resp.locals.filename) and use that value when setting Content-Disposition, and
update the members endpoint (where the filename is constructed) to pass the
computed filename into membersCsvSerializer (or set resp.locals.filename) so the
serializer uses the endpoint-provided name rather than the hardcoded one.
Comment on lines
+383
to
+384
| const titlePrefix = title ? `${slugify(title)}.` : ''; | ||
| return `${titlePrefix}ghost.members.${datetime}.csv`; |
Contributor
There was a problem hiding this comment.
Guard against empty slug output to preserve the documented fallback filename.
At Line 383, fallback is based on raw title truthiness. If title exists but slugifies to an empty string, Line 384 can emit .ghost.members.{date}.csv instead of ghost.members.{date}.csv.
Suggested fix (fallback on slug result, not raw title)
- const title = settingsCache.get('title');
- const titlePrefix = title ? `${slugify(title)}.` : '';
+ const title = settingsCache.get('title');
+ const titleSlug = title ? slugify(title) : '';
+ const titlePrefix = titleSlug ? `${titleSlug}.` : '';
return `${titlePrefix}ghost.members.${datetime}.csv`;🤖 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 `@ghost/core/core/server/api/endpoints/members.js` around lines 383 - 384,
Compute the slug once and base the fallback on the slug result (not raw title):
call slugify(title) into a local variable (e.g., slug), then set titlePrefix =
slug ? `${slug}.` : '' so you avoid emitting a leading dot when slugify(title)
returns an empty string; keep the existing return
`${titlePrefix}ghost.members.${datetime}.csv`.
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.
Why are you making it?
The members CSV export filename did not include the site name, unlike the JSON export and the already-fixed analytics CSV export (#28227). This inconsistency makes it harder to identify which site the data comes from when managing multiple Ghost instances.
What does it do?
Updates the members CSV export filename to follow the same convention:
<site-name>.ghost.members.YYYY-MM-DD.csvFalls back to
ghost.members.YYYY-MM-DD.csvif no site title is set.Why is this something Ghost users need?
Consistency across all exports and easier file management when handling multiple Ghost sites.