Moved @tryghost/members-csv into ghost/core#29491
Merged
Merged
Conversation
Contributor
|
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 ignored due to path filters (17)
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (13)
WalkthroughThe PR adds an internal members CSV service with typed parsing and serialization. Parsing supports header mappings, BOM handling, value coercion, labels, defaults, duplicate and unmapped columns, and stream errors. Serialization normalizes member fields, derives export values, adds error columns, and escapes formulae. Import and export consumers switch to the internal service, the external dependency is removed, and unit, snapshot, BOM, and round-trip tests are added. Suggested labels: 🚥 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 ghost:test:ci:integration |
✅ Succeeded | 2m 39s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin,gho... |
✅ Succeeded | 7m 58s | View ↗ |
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 6m 36s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 1m 58s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 20s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 19s | View ↗ |
nx run-many -t lint -p ghost-monorepo,@tryghost... |
✅ Succeeded | 3m 37s | View ↗ |
nx run ghost-admin:test |
✅ Succeeded | 3m 10s | View ↗ |
Additional runs (9) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-21 19:34:03 UTC
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #29491 +/- ##
==========================================
+ Coverage 74.53% 74.55% +0.02%
==========================================
Files 1605 1609 +4
Lines 141105 141329 +224
Branches 17199 17248 +49
==========================================
+ Hits 105169 105369 +200
- Misses 34878 34928 +50
+ Partials 1058 1032 -26
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:
|
rob-ghost
marked this pull request as ready for review
July 21, 2026 19:19
ref BER-3812 The module has only ever had two consumers, both in ghost/core, so publishing it to npm bought nothing and cost a release step. It now lives at core/server/services/members/csv, alongside the exporter and importer it serves in both directions, with its tests and fixtures under test/unit/server/services/members/csv, and the dependency is gone from package.json, the lockfile and the renovate group. Two source changes were forced by the move. pump is not a ghost/core dependency and is used nowhere else in core, so parsing uses node:stream's pipeline, which has the same call shape and return value. More significantly, the published package pinned papaparse 5.3.2 and shipped it nested, so the module had never run against core's 5.5.4: it abused the transformHeader rename hook to filter columns, returning a falsy value for columns outside the caller's mapping, and on 5.5.4 that hook is invoked more than once per column with the already-mapped name fed back in and every header passes through a byte-order-mark strip that assumes a string. Left alone, every non-identity mapping - subscribed_to_emails to subscribed above all - would have been silently dropped from imported rows. Parsing therefore reads raw headers and maps names and coerces values in the row handler instead, and the hand-rolled byte-order-mark strip goes with it because papaparse strips the mark before any hook runs. One behaviour change follows from the version difference and is now pinned by a test: duplicate columns in an uploaded CSV resolve first-wins rather than last-wins, since papaparse renames the second occurrence and the renamed column falls outside the mapping. Recovering the old answer would mean hand-handling that renaming, and for a malformed file neither answer is more correct. Tests also cover two unparse branches that were live on the export path but untested, complimentary_plan read from comped and stripe_customer_id read from the first subscription's customer. Moving the mapping and coercion into the row handler also means the handler sees __parsed_extra, the key papaparse uses to collect the overflow from a row carrying more fields than there are headers. Its value is an array rather than a string, so coercing it threw, and because a throw inside a stream data handler escapes as an uncaught exception rather than a rejection, the parse promise never settled. Non-string values are now skipped and the handler body rejects rather than throwing.
ref BER-3812 The value coercion for a labels column did not return when the value was empty or not a string, so it fell through to the generic coercion below, which turned an empty string into null, which the row handler then replaced with the default labels. The result was right but the route to it read like a missing return, so the branch is now total and returns an empty array directly. The row handler also assigned the caller's defaultLabels array by reference whenever a row had no labels of its own, so every such row shared one array and a mutation by any consumer would have affected all of them; it is now a single spread expression giving each row its own array. Neither case had a fixture, so one covering an empty labels column was added along with tests for the fallback and for rows not aliasing each other or the caller's array.
ref BER-3812 The check deciding whether a column was mapped used Reflect.has, which is equivalent to the `in` operator and walks the prototype chain, so a column called toString or constructor passed even when absent from the caller's mapping. Indexing the mapping with it then yielded a function, which became the mapped name and left a junk key on the parsed row. Object.hasOwn confines the check to the mapping's own keys. Custom field key minting already blocks prototype-named keys, so Ghost cannot emit such a column on export; this closes the remaining hand-authored CSV path.
ref BER-3812 The unparse module pulled in the whole of lodash for a single _.get on a subscription id that optional chaining expresses directly, so the import is dropped. The error column is now appended once up front from a rows.some check into a separate outputColumns array rather than being pushed onto a reassigned parameter from inside the row mapper, making it evident that the caller's column array is never mutated; the map above the push already copied, so this was already true, just not visible. Because that also lets the defensive copy on the DEFAULT_COLUMNS default parameter go, a test now asserts that a call carrying error rows does not leave an error column behind for the next call. The tests move to node:assert/strict to match the other suites under test/unit/server/services/members, and two test names with a stray leading space and broken grammar are fixed. No behaviour change.
ref BER-3812 Both consumers are still JavaScript and require() the module, so index.ts re-exports parse and unparse as named exports and neither consumer needed changing: under module commonjs that emits exports.parse and exports.unparse, which is what require() resolves. A MemberCsvRow interface in types.ts writes the member CSV column vocabulary down in one place, with an index signature for custom field columns which are not known ahead of time. Recording it as a single interface makes the asymmetries explicit rather than leaving them implicit across the separate import and export lists: tiers and deleted_at are written on export but not accepted on import, import_tier is accepted on import and never written, and the email subscription state is called subscribed coming in and subscribed_to_emails going out. The coerced fields are typed as a ParsedValue union rather than string, because any column that is not special-cased goes through the same coercion, so a member genuinely named TRUE parses to a boolean and an empty cell to null. The conversion surfaced two type errors the JavaScript could not express: the missing-file rejection assertion indexed code on an untyped error, and the round-trip test's temporary file variable was an implicit any. Mocking moves from sinon to vitest's vi, since sinon predates the move to vitest here and vitest cannot be required from CommonJS, so vi only became available once the tests were modules; the single console spy was the only use, so the dependency leaves the module entirely. The test files also move out of the lint:test glob, which matches only test/**/*.js, and into lint:types, which additionally runs tsc, so they are type-checked from now on.
ref BER-3812 The import modal auto-detects columns by matching raw CSV header keys against the supported field names, so a file saved with a UTF-8 byte order mark would leave the first header prefixed and silently fail to match, dropping the user into the mapping step with email unmatched. papaparse already strips the mark from headers, but nothing pinned that behaviour, so a version bump could regress it without a failing test. Folding members-csv into core surfaced a hand-rolled strip in that module which was dead for the same reason, which prompted checking the admin side for equivalent cover. Test only, no production code changed.
rob-ghost
force-pushed
the
chore/ber-3812-fold-members-csv-into-core
branch
from
July 21, 2026 19:22
725915e to
218cde3
Compare
![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
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/csv/parse.ts (1)
68-109: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStream keeps processing after the promise rejects.
pipeline()'s automatic error cleanup only covers errors from the streams themselves; a throw inside this'data'handler is caught and turned into areject(), but the underlying read stream is never destroyed. For a large member CSV, this means the rest of the file continues to be parsed and pushed intorowsafter the promise has already settled — wasted CPU/memory that's easy to avoid.♻️ Proposed fix: destroy the stream on error
} catch (err) { - reject(err); + csvFileStream.destroy(err); + reject(err); }🤖 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/services/members/csv/parse.ts` around lines 68 - 109, Update the error path in the parsedCSVStream 'data' handler to destroy the underlying stream before rejecting, so parsing stops immediately when row transformation fails. Preserve the existing rejection behavior and ensure the stream is destroyed only for caught processing errors.
🤖 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/services/members/importer/members-csv-importer.js`:
- Line 5: Update the members CSV import flow after parsing, before the code that
calls Object.keys(rows[0]), to check whether the parsed rows array is empty.
Handle a header-only or otherwise empty import through the existing controlled
import result/error path, preventing access to rows[0] when no rows were parsed.
---
Nitpick comments:
In `@ghost/core/core/server/services/members/csv/parse.ts`:
- Around line 68-109: Update the error path in the parsedCSVStream 'data'
handler to destroy the underlying stream before rejecting, so parsing stops
immediately when row transformation fails. Preserve the existing rejection
behavior and ensure the stream is destroyed only for caught processing errors.
🪄 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: 249eab8f-5fc7-40fe-bb51-75a798b3f197
⛔ Files ignored due to path filters (17)
ghost/core/test/unit/server/services/members/csv/fixtures/duplicate-headers.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/empty.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/long-row.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/members-with-complimentary-plan.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/members-with-empty-labels.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/members-with-labels.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/members-with-undefined-values.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/multiple-records-with-empty-values.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/prototype-named-column.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/single-column-with-header-bom.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/single-column-with-header.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/snapshots/comprehensive-members.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/subscribed-to-emails-header.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/two-columns-mapping-header.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/two-columns-obscure-header.csvis excluded by!**/*.csvghost/core/test/unit/server/services/members/csv/fixtures/two-columns-with-header.csvis excluded by!**/*.csvpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.github/renovate.json5apps/admin/src/members/components/bulk-action-modals/import-members/csv.test.tsghost/core/core/server/api/endpoints/utils/serializers/output/members.jsghost/core/core/server/services/members/csv/index.tsghost/core/core/server/services/members/csv/parse.tsghost/core/core/server/services/members/csv/types.tsghost/core/core/server/services/members/csv/unparse.tsghost/core/core/server/services/members/importer/members-csv-importer.jsghost/core/package.jsonghost/core/test/unit/server/services/members/csv/fixtures/snapshots/comprehensive-members-expected.jsonghost/core/test/unit/server/services/members/csv/fixtures/snapshots/roundtrip-expected.jsonghost/core/test/unit/server/services/members/csv/fixtures/snapshots/roundtrip-input.jsonghost/core/test/unit/server/services/members/csv/parse.test.tsghost/core/test/unit/server/services/members/csv/snapshot.test.tsghost/core/test/unit/server/services/members/csv/unparse.test.ts
💤 Files with no reviewable changes (2)
- .github/renovate.json5
- ghost/core/package.json
| const fs = require('fs-extra'); | ||
| const metrics = require('@tryghost/metrics'); | ||
| const membersCSV = require('@tryghost/members-csv'); | ||
| const membersCSV = require('../csv'); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle an empty parsed CSV before reading the first row.
The internal parser skips empty/unmapped rows, so a header-only CSV produces []; Line 112 then throws on Object.keys(rows[0]) instead of returning a controlled import result/error. Guard rows.length immediately after parsing and handle the empty import explicitly.
🤖 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/services/members/importer/members-csv-importer.js` at
line 5, Update the members CSV import flow after parsing, before the code that
calls Object.keys(rows[0]), to check whether the parsed rows array is empty.
Handle a header-only or otherwise empty import through the existing controlled
import result/error path, preventing access to rows[0] when no rows were parsed.
This was referenced Jul 21, 2026
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.
Problem
@tryghost/members-csvwas published to npm from the SDK repo but had no consumers outside Ghost — two of them, both in Ghost core. Shipping it as a package meant a release round-trip for every change, and left a third of the member CSV column vocabulary on a separate release cycle from the code that has to agree with it.It also turned out to be broken in a way nobody could see. The published package pinned an old version of papaparse and shipped it nested, so it never ran against the version Ghost core itself uses. On the version core has, the trick the module used to filter out unmapped columns no longer works, and every column whose name changes on the way in — most importantly the email subscription flag — would have been silently dropped from imported member rows. The split across two repos is what kept that hidden.
Solution
The module now lives in Ghost core alongside the members importer and exporter, and the dependency is gone. It no longer misuses papaparse's rename hook to do column filtering, which is what broke against the newer version; it maps column names and coerces values itself.
Beyond the move, a few things it was carrying are fixed: a column named after a built-in object property no longer slips past the mapping check, label handling no longer depends on a fall-through and no longer hands every row the same array, and a row with more fields than its header row can no longer take down the import. The module and its tests are now TypeScript, with the column vocabulary written down in one place for the first time.
Duplicate columns in an uploaded file now resolve to the first rather than the last, which is a consequence of the newer papaparse and is covered by a test.
Notes
Follow-up work will unify the column vocabulary, which is currently defined in four separate places. The npm package should be deprecated once the SDK maintainers confirm — the absence of external consumers is inference from search and download data, not proof.
https://linear.app/ghost/issue/BER-3812