Moved CSV export filename ownership to the API#28395
Merged
Merged
Conversation
ref #28310 - the members and post-analytics CSV exports each defined the `<slug>.ghost.<type>.<date>.csv` filename in up to four places (two API endpoints and two admin helpers), and slugified the site title inconsistently (hand-rolled regex on the client vs `@tryghost/string` on the server) - made the API the single source of truth: added getCSVExportFileName() and a shared streaming-response helper, and taught the admin blobDownload helper to read the filename from the response's Content-Disposition header - this also fixes the members export: the streaming serializer previously hardcoded `members.<date>.csv`, so the API's prefixed Content-Disposition never reached direct/API consumers - removed the now-duplicated frontend filename helpers, the siteTitle prop-drilling and the orphaned usePostsExports hook
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:
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 (6)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR centralizes CSV export filename generation on the backend (getCSVExportFileName), introduces createCSVStreamResponse for streaming CSV responses, and updates frontend helpers (getFilenameFromContentDisposition, blobDownload, blobDownloadFromEndpoint) to prefer filenames from Content-Disposition with fallbacks. Frontend export code and tests were simplified to use endpoint downloads; serializers and endpoint code were updated to delegate streaming and provide consistent filenames. Tests and e2e expectations were added/updated for parsing, filename formats, streaming behavior, and error propagation. Possibly related PRs
Suggested labels
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 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 |
![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: 1
🤖 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-framework/src/utils/helpers.ts`:
- Around line 48-52: The current RFC5987 parsing only strips the exact "UTF-8''"
token and misses non-empty language tags (e.g. UTF-8'en'), so update the
header.match call that produces extendedMatch to use a pattern that accepts an
optional charset followed by an optional non-empty language tag and the two
single-quote delimiter (i.e., allow charset'lang'') before capturing the encoded
filename; then continue to trim surrounding quotes and pass the captured group
to decodeURIComponent as you already do (adjust the match on the line that
defines extendedMatch and keep the existing decode/trim/try-catch flow).
🪄 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: 02c7efdc-f60b-46e5-a02e-fa92789e0a1e
📒 Files selected for processing (16)
apps/admin-x-framework/src/api/posts.tsapps/admin-x-framework/src/utils/helpers.tsapps/admin-x-framework/test/unit/utils/helpers.test.tsapps/admin-x-settings/src/components/settings/advanced/migration-tools/migration-tools-export.tsxapps/admin-x-settings/test/unit/utils/post-analytics-export-filename.test.tsapps/posts/src/views/members/components/members-actions.tsxapps/posts/src/views/members/members.tsxapps/posts/test/unit/views/members/members-actions.test.tsxghost/core/core/server/api/endpoints/members.jsghost/core/core/server/api/endpoints/posts.jsghost/core/core/server/api/endpoints/utils/csv-export-filename.jsghost/core/core/server/api/endpoints/utils/serializers/output/members.jsghost/core/core/server/api/endpoints/utils/serializers/output/posts.jsghost/core/core/server/api/endpoints/utils/serializers/output/stream-csv-response.tsghost/core/test/unit/api/canary/utils/serializers/output/members-export-csv.test.tsghost/core/test/unit/api/endpoints/utils/csv-export-filename.test.js
💤 Files with no reviewable changes (2)
- apps/admin-x-settings/test/unit/utils/post-analytics-export-filename.test.ts
- apps/admin-x-framework/src/api/posts.ts
ref #28310 - routing the members CSV export through the shared streaming-response helper changed three response headers that the e2e-api snapshots assert on: the Content-Disposition is now the site-prefixed `<slug>.ghost.members.<date>.csv` (capitalised to match the posts export), Cache-Control gains `no-transform`, and the now-redundant `Vary: Accept-Encoding` is dropped - updated the backup, members-exporter and members snapshots accordingly
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #28395 +/- ##
=======================================
Coverage 73.77% 73.78%
=======================================
Files 1536 1538 +2
Lines 130934 130965 +31
Branches 15690 15694 +4
=======================================
+ Hits 96597 96630 +33
- Misses 33347 33369 +22
+ Partials 990 966 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ref #28310 - getFilenameFromContentDisposition only stripped the literal `UTF-8''` token, so an extended `filename*` value with a non-empty language tag (e.g. `UTF-8'en'`) or a non-UTF-8 charset (e.g. `ISO-8859-1''`) leaked the prefix into the parsed filename - match the full RFC 5987 `charset'language'` prefix before the encoded value
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ref #28310 - CodeQL flagged the RFC 5987 extended-form regex as a polynomial ReDoS: the nested `[^';]*'[^';]*'` runs overlap with the following capture when the header has no closing quote - replaced it with a single linear capture plus plain string-op prefix stripping; behaviour is unchanged and covered by the existing unit tests
ref #28310 - dropped redundant inline comments from the members and analytics export callsites (the behaviour is documented once on the shared blobDownload helper) - tightened the remaining JSDoc on the filename helper, the stream-response helper and the Content-Disposition parser
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 #28310
Why
The members and post-analytics CSV exports each defined the
<slug>.ghost.<type>.<date>.csvfilename in up to four places — two API endpoints (members,posts) and two admin helpers (getMembersExportFileName,getPostAnalyticsExportFileName) — and slugified the site title two different ways (a hand-rolled regex on the client vs@tryghost/string'sslugifyon the server, which transliterate non-ASCII titles differently).This duplication exists only because admin downloads fetch the export into a blob and set the filename via
a.download, which bypasses the server'sContent-Dispositionheader. So the client re-implemented what the server already knew.What
Make the API the single source of truth for export filenames, and have the admin client read it back from the response:
Backend (
ghost/core)endpoints/utils/csv-export-filename.js(getCSVExportFileName(type)) — the one definition of the convention.serializers/output/stream-csv-response.tsand routed both the members and posts CSV serializers through it.members.<date>.csv, so the prefixedContent-Dispositionnever reached direct/API consumers. It now uses the filename plumbed from the endpoint, so the API download is named correctly.Frontend
blobDownload/blobDownloadFromEndpointnow read the filename from the response'sContent-Dispositionheader (newgetFilenameFromContentDispositionparser); the explicitfilenamearg is now an optional fallback.siteTitleprop-drilling, and unified the post-analytics export ontoblobDownloadFromEndpoint(removing the now-orphanedusePostsExportshook and its bespoke blob/anchor download).Net behaviour is unchanged for users (same filenames in the admin UI), but the convention now lives in exactly one place and the direct API download is finally correct.
Test plan
ghost/core: new unit tests forgetCSVExportFileName(slug + transliteration + fallback) and the members CSV serializer (Content-Disposition, legacy fallback, stream errors). Posts serializer tests still pass. 14 passing.admin-x-framework:blobDownloadnow covered for header-driven naming, fallback, and thegetFilenameFromContentDispositionparser. 25 passing.posts:members-actionsexport test updated (server names the file). 6 passing.tscclean across all touched files.blobDownloadFromEndpoint(same-origin, cookie-authed — same path the members export already uses). Theanalytics.test.tsacceptance test mocks by URL and should pass, but it's a Playwright test worth confirming in CI.