Open
Conversation
Contributor
WalkthroughThis pull request introduces two comprehensive migration packages for integrating Beehiiv data into Ghost: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Member
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
![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: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
packages/mg-beehiiv-api-members/README.md-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorReplace the placeholder README before merge.
This package is set up for public consumption, but the README still has only
.... Please add the auth requirements, CLI usage, expected outputs, and any Beehiiv limitations so consumers can use it without reading the source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api-members/README.md` around lines 1 - 3, Replace the placeholder README in packages/mg-beehiiv-api-members/README.md with a complete user-facing document: add an "Authentication" section that lists required env vars or API token names and scopes, a "CLI usage" section with example commands and required flags/arguments for the package's CLI entrypoint, an "Expected outputs" section showing sample successful and error JSON responses from the key functions (e.g., member create/list endpoints), and a "Beehiiv limitations" section describing rate limits, pagination behavior, and any unsupported fields; include short usage examples, exit codes, and troubleshooting tips so consumers can use the package without reading the source.packages/migrate/commands/beehiiv-api.js-16-16 (1)
16-16:⚠️ Potential issue | 🟡 MinorFix misleading command description.
The description says "Migrate from beehiiv using CSV files" but this command uses the beehiiv API, not CSV files. The existing
beehiivcommand handles CSV-based migration.📝 Proposed fix
-const desc = 'Migrate from beehiiv using CSV files'; +const desc = 'Migrate from beehiiv using the beehiiv API';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/migrate/commands/beehiiv-api.js` at line 16, Update the misleading command description stored in the const desc in the beehiiv-api command so it accurately reflects that this command migrates from Beehiiv via the Beehiiv API (not CSV files); locate the const desc in packages/migrate/commands/beehiiv-api.js and replace the current text 'Migrate from beehiiv using CSV files' with a short, accurate description such as 'Migrate from Beehiiv using the Beehiiv API' or similar.packages/migrate/commands/beehiiv-api.js-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorRemove unused import.
The
inspectimport fromnode:utilis not used anywhere in this file.🧹 Proposed fix
-import {inspect} from 'node:util'; import {ui} from '@tryghost/pretty-cli';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/migrate/commands/beehiiv-api.js` at line 1, The file imports inspect from node:util but never uses it; remove the unused import statement "import {inspect} from 'node:util';" (from the top of the module) and verify there are no remaining references to inspect in functions like any helper or export in this module so ESLint/TS will stop flagging an unused import.packages/migrate/commands/beehiiv-api.js-147-150 (1)
147-150:⚠️ Potential issue | 🟡 MinorRemove dead code handling
argv.url.This code handles
argv.urlbut there is no--urloption defined in theoptionsarray. This code will never execute meaningfully.🧹 Proposed fix
- // Remove trailing slash from URL - if (argv.url && argv.url.endsWith('/')) { - argv.url = argv.url.slice(0, -1); - } - // If no publication ID is provided, list publications and exit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/migrate/commands/beehiiv-api.js` around lines 147 - 150, The trailing-slash handling for argv.url is dead code because no --url option exists; remove the entire conditional block that checks and slices argv.url (the if (argv.url && argv.url.endsWith('/')) { argv.url = argv.url.slice(0, -1); }) from the migration command, or alternatively add a corresponding --url option to the options array if the URL behavior was intended—pick one and implement consistently so argv.url is never referenced without a defined option.packages/migrate/commands/beehiiv-api-members.js-124-125 (1)
124-125:⚠️ Potential issue | 🟡 MinorVerbose logging points at a non-existent result field.
The members pipeline populates
context.result.members, soinspect(context.result.data, ...)will just printundefinedon success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/migrate/commands/beehiiv-api-members.js` around lines 124 - 125, The verbose log is printing context.result.data which is undefined because the members pipeline puts results in context.result.members; update the ui.log.info call (the block guarded by argv.verbose and context.result) to inspect and log context.result.members (e.g., inspect(context.result.members, false, 2)) so the actual member data is shown; ensure you reference ui.log.info, argv.verbose and context.result.members when making the change and keep the existing null-check on context.result or add a fallback if needed.packages/migrate/commands/beehiiv-api-members.js-105-110 (1)
105-110:⚠️ Potential issue | 🟡 MinorAvoid raw
console.tablein the CLI command.This currently fails the repo lint rule and bypasses the command's normal UI/logging path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/migrate/commands/beehiiv-api-members.js` around lines 105 - 110, The CLI code uses raw console.table on getPubs which violates lint and bypasses the command's logging/UI; replace the console.table call with the command's logger/UI output (e.g., use the command context logger or existing processLogger) and render the same rows by formatting getPubs.map(...) into a string or using the repo's table helper before calling logger.info/log or the command's print function; update the invocation around getPubs and the anonymous map so it passes the formatted string to the logger instead of calling console.table.packages/mg-beehiiv-api-members/src/test/fetch.test.ts-179-182 (1)
179-182:⚠️ Potential issue | 🟡 MinorMock
discover()with the real publication payload.
fetchTasks()callsdiscover(), anddiscover()readsresponse.json().data.stats.active_subscriptions. These stubs returntotal_resultsinstead, soestimatedPagesbecomesNaNand the pagination/progress path is never actually exercised.Also applies to: 201-204, 245-248, 276-279, 309-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api-members/src/test/fetch.test.ts` around lines 179 - 182, The mocked fetch responses used in the tests return an object shaped for list endpoints ({ total_results }) but discover() expects the publication payload and reads response.json().data.stats.active_subscriptions; update the fetchMock.mock.mockImplementationOnce stubs used by tests that exercise fetchTasks() (and indirectly discover()) to return a JSON shape matching the real publication payload—i.e. response.json() should resolve to an object with a data.stats.active_subscriptions numeric field (and any other fields discover() needs such as has_more/page indicators) so estimatedPages is computed correctly; apply the same change to the other test stubs referenced (the blocks at the other locations listed) to ensure pagination/progress paths are exercised.packages/mg-beehiiv-api/src/lib/process.ts-483-499 (1)
483-499:⚠️ Potential issue | 🟡 MinorGuard against undefined values to prevent runtime errors.
Several potential issues:
- Line 486:
firstImgSrccould beundefined(fromattr('src')), causing.split()to throw.- Line 489: If URLs don't contain
/uploads/asset/,split()[1]returnsundefined, andundefined === undefinedevaluates totrue, potentially removing non-duplicate images.Proposed fix with safety checks
- // Both images usually end in the same way, so we can split the URL and compare the last part - const firstImageSplit = firstImgSrc.split('/uploads/asset/'); - const featureImageSplit = featureSrc.split('/uploads/asset/'); - - if (firstImageSplit[1] === featureImageSplit[1]) { - $html(theElementItself).remove(); - } - - if (featureSrc.length > 0 && firstImgSrc) { + if (firstImgSrc && featureSrc) { + // Both images usually end in the same way, so we can split the URL and compare the last part + const firstImageSplit = firstImgSrc.split('/uploads/asset/'); + const featureImageSplit = featureSrc.split('/uploads/asset/'); + + if (firstImageSplit[1] && featureImageSplit[1] && firstImageSplit[1] === featureImageSplit[1]) { + $html(theElementItself).remove(); + return $html.html(); + } + let normalizedFirstSrc = firstImgSrc.replace('fit=scale-down,format=auto,onerror=redirect,quality=80', 'quality=100'); if (featureSrc === normalizedFirstSrc) { $html(theElementItself).remove(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api/src/lib/process.ts` around lines 483 - 499, Guard against undefined and missing segments before splitting and comparing: ensure firstImgSrc is truthy before calling split or replace, and verify both firstImgSrc and featureSrc contain '/uploads/asset/' (or that split()[1] exists) before comparing their [1] parts; also check featureSrc is non-empty before normalizing and comparing to normalizedFirstSrc. Specifically, update the logic around variables firstImgSrc, firstImageSplit, featureImageSplit and normalizedFirstSrc in the block using $html(theElementItself) so you only call .split(), .replace(), or access [1] when the source strings are defined and include the expected substring, and bail out (skip removal) if those guards fail.
🧹 Nitpick comments (8)
packages/mg-beehiiv-api/tsconfig.json (2)
110-110:src/index.jsinincludeis problematic.Two issues with this entry:
- The glob
"src/**/*"already covers all files insrc/, making"src/index.js"redundant if it's a TypeScript file.- If
src/index.jsis actually a JavaScript file, it won't be processed becauseallowJsis not enabled (line 48 is commented out).If this package uses TypeScript throughout, remove the redundant entry. If there's genuinely a JS entry point, enable
allowJs.♻️ Suggested fix (assuming TypeScript only)
- "include": ["src/**/*", "src/index.js"] + "include": ["src/**/*"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api/tsconfig.json` at line 110, The tsconfig's "include" currently lists "src/**/*" and the redundant "src/index.js"; remove "src/index.js" from the "include" array if the codebase is TypeScript-only (so only "src/**/*" remains), otherwise if there truly is a JavaScript entry point enable "allowJs": true (the commented out "allowJs" setting around line 48) so the JS file will be picked up—apply the appropriate change in the tsconfig.json.
86-87:noImplicitAnyis redundant withstrictmode.When
strict: trueis enabled, it implicitly enablesnoImplicitAny(among other strict checks). The explicit setting is harmless but unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api/tsconfig.json` around lines 86 - 87, The tsconfig.json contains a redundant "noImplicitAny" entry because "strict": true already enables it; remove the explicit "noImplicitAny" property (or alternatively set it to identical value via a comment if you prefer explicitness) so only "strict": true remains as the single source of truth for strict checks; locate the JSON object containing the "strict" and "noImplicitAny" keys and delete the "noImplicitAny" line.packages/mg-beehiiv-api/src/lib/client.ts (1)
1-18: Avoid maintaining a second publications client here.
packages/mg-beehiiv-api/src/lib/list-pubs.tsalready wraps the authenticated publications request on top ofauthedClient, andpackages/mg-beehiiv-api/src/index.tsdoes not even export thisclient. Keeping both paths means the shipped code and the tested code can drift—they already differ on theexpand[]=statsquery parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api/src/lib/client.ts` around lines 1 - 18, The file defines a duplicate async function client that issues a GET to /publications; instead of keeping this second publications client, remove or replace its usage and delegate to the existing authedClient used by list-pubs.ts to avoid divergence (ensure the same query params such as expand[]=stats are preserved). Locate the client function in this file and either delete it or refactor callers to call the shared authedClient wrapper (the same helper used by packages/mg-beehiiv-api/src/lib/list-pubs.ts) so the exported surface matches index.ts and the requests remain identical.packages/mg-beehiiv-api-members/src/index.ts (1)
5-9: Consider using named exports for utilities.Same feedback as for
packages/mg-beehiiv-api/src/index.ts— these are utility functions that would be better served by named exports per coding guidelines.♻️ Suggested refactor using named exports
import {listPublications} from './lib/list-pubs.js'; import {fetchTasks} from './lib/fetch.js'; import {mapMembersTasks} from './lib/mapper.js'; -export default { - listPublications, - fetchTasks, - mapMembersTasks -}; +export { + listPublications, + fetchTasks, + mapMembersTasks +};As per coding guidelines: "Named exports should be used for utilities, and default exports for main functionality (classes)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api-members/src/index.ts` around lines 5 - 9, Current default export is exporting utility functions as a single default object; change this to named exports by exporting listPublications, fetchTasks, and mapMembersTasks individually (remove the default-exported object) so callers can import them as named imports; update any internal references or import sites to use the named imports if necessary (search for import of the default from this module and switch to { listPublications, fetchTasks, mapMembersTasks }).packages/mg-beehiiv-api/src/index.ts (1)
5-9: Consider using named exports for utilities.Per coding guidelines, named exports should be used for utilities in index files, while default exports are reserved for main functionality (classes). Since
listPublications,fetchTasks, andmapPostsTasksare utility functions, named exports would be more appropriate.♻️ Suggested refactor using named exports
import {listPublications} from './lib/list-pubs.js'; import {mapPostsTasks} from './lib/mapper.js'; import {fetchTasks} from './lib/fetch.js'; -export default { - listPublications, - fetchTasks, - mapPostsTasks -}; +export { + listPublications, + fetchTasks, + mapPostsTasks +};Note: This would require updating consumers (e.g.,
packages/migrate/sources/beehiiv-api.jsandpackages/migrate/commands/beehiiv-api.js) to use named imports instead of default import.As per coding guidelines: "Named exports should be used for utilities, and default exports for main functionality (classes)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api/src/index.ts` around lines 5 - 9, Replace the default export object with named exports for the utility functions: export the functions listPublications, fetchTasks, and mapPostsTasks as named exports (e.g., export { listPublications, fetchTasks, mapPostsTasks }) and update any consumers (such as migrate/sources/beehiiv-api.js and migrate/commands/beehiiv-api.js) to import those functions by name instead of using the default import.packages/mg-beehiiv-api-members/src/lib/list-pubs.ts (1)
9-11: Consider using@tryghost/errorsfor structured error handling.Per coding guidelines, errors should use
@tryghost/errorswith message and context properties for better debugging and error tracking.♻️ Suggested refactor using `@tryghost/errors`
+import errors from '@tryghost/errors'; import {authedClient} from './fetch.js'; const listPublications = async (apiKey: string) => { const url = new URL(`https://api.beehiiv.com/v2/publications`); url.searchParams.append('expand[]', 'stats'); const response = await authedClient(apiKey, url); if (!response.ok) { - throw new Error(`Request failed: ${response.status} ${response.statusText}`); + throw new errors.InternalServerError({ + message: `Request failed: ${response.status} ${response.statusText}`, + context: { + url: url.toString(), + status: response.status, + statusText: response.statusText + } + }); } const data = await response.json(); return data.data; };As per coding guidelines: "Use
@tryghost/errorsfor structured error handling with message and context properties."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api-members/src/lib/list-pubs.ts` around lines 9 - 11, The current plain Error thrown when response.ok is false should be replaced with an `@tryghost/errors` structured error: import errors from '@tryghost/errors' and throw an instance that includes a descriptive message and a context object (e.g., {status: response.status, statusText: response.statusText, url: <request url or variable>}) in the list-pubs code path where response is checked; update the code around response.ok in the listPubs (or the function that performs the fetch) to construct and throw that `@tryghost/errors` error instead of new Error(...) so logs and error handling get structured message and context.packages/mg-beehiiv-api/src/lib/process.ts (2)
3-5: Remove unused imports.
sanitizeHtml(line 3) andimageCard(line 5) are imported but only referenced in commented-out code. Remove these to avoid confusion and dead imports.Proposed fix
import * as cheerio from 'cheerio'; import _ from 'lodash'; -import sanitizeHtml from 'sanitize-html'; import SimpleDom from 'simple-dom'; -import imageCard from '@tryghost/kg-default-cards/lib/cards/image.js'; import embedCard from '@tryghost/kg-default-cards/lib/cards/embed.js'; import bookmarkCard from '@tryghost/kg-default-cards/lib/cards/bookmark.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api/src/lib/process.ts` around lines 3 - 5, Remove the dead imports sanitizeHtml and imageCard from the top of the file—keep only the actually used SimpleDom import; update the import list in packages/mg-beehiiv-api/src/lib/process.ts by deleting the sanitizeHtml and imageCard import statements (they are only referenced in commented-out code) so there are no unused imports left.
160-430: Remove or relocate commented-out code.This block contains ~270 lines of commented-out code, which is a significant maintenance burden. If this code is experimental or for future use, consider:
- Removing it entirely (it's preserved in version control history)
- Moving it to a separate file or documentation if it serves as reference
- Creating a tracking issue if it represents planned functionality
Keeping large blocks of dead code makes the file harder to read and maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mg-beehiiv-api/src/lib/process.ts` around lines 160 - 430, The large ~270-line commented block in process.ts (inside the HTML cleaning logic that manipulates $html, options, postData, imageCard, embedCard, bookmarkCard, serializer, getYouTubeID, etc.) should be removed or moved out of the file: either delete it (history preserves it), extract it into a dedicated reference file (e.g., a legacy or experimental helpers file) or add it to a design doc/issue tracking planned features; update imports/usages if you relocate helpers and ensure the active processing functions (the HTML-cleanup routine surrounding $html and related handlers) remain clean and focused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/mg-beehiiv-api-members/.eslintrc.cjs`:
- Around line 10-12: The config currently disables the enforced `@tryghost/errors`
rules; revert that by removing the two overrides
('ghost/ghost-custom/no-native-error' and
'ghost/ghost-custom/ghost-error-usage') from .eslintrc.cjs and re-enable
linting. In code, replace native Error usages in src/lib/list-pubs.ts and
src/lib/fetch.ts: import and throw the appropriate `@tryghost/errors` class (e.g.,
GhostError or a more specific error like BadRequestError/NotFoundError) instead
of new Error(...), and ensure any error construction/messaging matches the
chosen `@tryghost/errors` API so the linter passes. Ensure imports reference
`@tryghost/errors` at the top of each file and update thrown instances to use
those classes.
In `@packages/mg-beehiiv-api-members/src/lib/fetch.ts`:
- Around line 19-20: Replace raw Error throws in the response.ok checks with a
structured error from `@tryghost/errors`: import the package and throw a
`@tryghost/errors` error (e.g. new errors.InternalServerError or a suitable
subclass) that includes a descriptive message and a context object containing
pubId, cursor, cursorIndex and the HTTP status/statusText; update both places
where the code currently does throw new Error(...) (the response.ok check around
the first failure and the second occurrence at the later response check) so the
thrown error uses the `@tryghost/errors` API and attaches the request context for
debuggability.
In `@packages/mg-beehiiv-api-members/src/types.d.ts`:
- Around line 1-45: The types are currently declared as ambient globals and must
be exported to avoid polluting consumers; change the type declarations
(BeehiivSubscription, BeehiivPublicationResponse, BeehiivSubscriptionsResponse,
GhostMemberObject, MappedMembers) to exported types (e.g., export type ...) and
then update mapper.ts and fetch.ts to import the needed types instead of relying
on globals; alternatively move the two declare module shims into a separate
ambient declaration file (e.g., a .d.ts) and keep only exported type definitions
in this file so consumers only receive explicit exports.
In `@packages/mg-beehiiv-api/.eslintrc.cjs`:
- Around line 10-12: Remove the three disabled ESLint rules
('ghost/ghost-custom/no-native-errors', 'ghost/ghost-custom/no-native-error',
'ghost/ghost-custom/ghost-error-usage') from .eslintrc.cjs so the Ghost error
linting is enforced, then update the native throw in src/lib/client.ts (the
"throw new Error(...)" at the top of the file) to use the structured error types
from `@tryghost/errors`: import the appropriate error class from `@tryghost/errors`
(e.g., errors.GhostError or the most semantically appropriate errors.* class)
and throw that instead with the same message/metadata so the package conforms to
the project's structured error handling.
In `@packages/mg-beehiiv-api/README.md`:
- Around line 1-3: The README.md is a placeholder and must be expanded into a
full package README: add a brief package description and purpose, installation
instructions (npm/yarn/pnpm commands), usage examples including CLI commands and
code snippets showing how to call the package, API documentation listing
exported functions/endpoints and expected parameters, configuration details for
Beehiiv credentials and env vars, examples of input/output formats and sample
responses, and a troubleshooting/FAQ section; update the top-level README.md in
this package (packages/mg-beehiiv-api/README.md) to include these sections and
concrete examples matching the package's exported API and configuration keys.
In `@packages/mg-beehiiv-api/src/lib/client.ts`:
- Around line 11-12: Replace the generic throw new Error in the response.ok
check with a Ghost structured error: import errors from '@tryghost/errors' (or
ensure it's imported) and throw new errors.InternalServerError({message:
`Beehiiv API request failed: ${response.statusText}`, context: {status:
response.status, statusText: response.statusText, url: url.toString()}}) so the
check around response.ok (the existing if (!response.ok) block) includes status,
statusText and the request URL in the context for consistent migration
reporting.
In `@packages/mg-beehiiv-api/src/lib/fetch.ts`:
- Around line 18-19: Replace plain Error throws that use response data with
structured `@tryghost/errors.InternalServerError` instances and pass the local
pubId and page in the context; specifically, import errors from
'@tryghost/errors' if not already present and change the two throw sites that
currently do throw new Error(`Request failed: ${response.status}
${response.statusText}`) to throw new errors.InternalServerError({ message:
`Request failed: ${response.status} ${response.statusText}`, context: {pubId,
page} }); do this for both occurrences so the error includes diagnostic context
when propagated.
In `@packages/mg-beehiiv-api/src/lib/list-pubs.ts`:
- Around line 9-10: Replace the raw thrown Error in listPublications (in
packages/mg-beehiiv-api/src/lib/list-pubs.ts) with a structured `@tryghost/errors`
instance: import errors from '@tryghost/errors' at the top and change the throw
to throw new errors.InternalServerError({message: `Request failed:
${response.status} ${response.statusText}`}) so the HTTP failure uses the
codebase's standardized error type and includes the same status/text message.
In `@packages/mg-beehiiv-api/src/lib/mapper.ts`:
- Around line 11-13: The mapping in mapper.ts incorrectly sets updated_at from
postData.created; change the mapping for updated_at to use the source update
timestamp (e.g., postData.updated or postData.modified) when available, and only
fall back to omitting updated_at (or leaving it undefined/null) if the source
does not provide an update field; update the mapping that assigns published_at,
updated_at, created_at (references: postData.publish_date,
postData.updated/postData.modified, postData.created) so updated_at reflects the
real source update time instead of created.
In `@packages/mg-beehiiv-api/src/test/process.test.ts`:
- Around line 223-230: The test currently asserts that
removeDuplicateFeatureImage removes an image when featureSrc is empty, which
locks in a bug; update the test in process.test.ts to expect the image to be
preserved when featureSrc === '' by changing the assertion to check that result
includes the original <img> (e.g., assert.ok(result.includes('<img') or
assert.strictEqual(result, html) or check the src remains unchanged) and ensure
the test references the removeDuplicateFeatureImage function and the existing
html/featureSrc variables so it verifies preservation rather than removal.
In `@packages/mg-beehiiv-api/src/types.d.ts`:
- Around line 1-76: Add explicit exports for the top-level type declarations to
avoid creating ambient globals: update each type declaration
(beehiivPostDataObject, tagsObject, authorsObject, mappedDataObject) to be
exported (e.g., export type ...) so they are module-scoped and available to
consumers; keep or adjust the existing declare module lines as needed but do not
leave the plain types unexported—alternatively move these types into a
module-scoped file and re-export them from your package index so the published
"types" entry no longer pollutes the global scope.
In `@packages/migrate/commands/beehiiv-api-members.js`:
- Around line 96-115: Move the no---id publication listing branch inside the
existing try block within run() so any rejection from
mgBeehiivApiMembers.listPublications(argv.key) is caught; replace
process.exit(0) with a return to exit the function gracefully, and ensure errors
use ui.log.error (preserving the current table/log output and the ui.log.warn
message) so listPublications() failures are handled by the surrounding catch.
In `@packages/migrate/commands/beehiiv-api.js`:
- Around line 180-183: The verbose flag check (argv.verbose) is used but the
corresponding option in the options array is commented out, which breaks verbose
behavior and also leaves AssetScraper expecting options.verbose; restore the CLI
option by uncommenting the verbose option declaration in the options array so
argv.verbose and options.verbose are defined, or alternatively remove the
argv.verbose check and any references to options.verbose in the AssetScraper
task in sources/beehiiv-api.js—specifically ensure the CLI options array
includes the verbose option name/flag so argv.verbose and options.verbose are
available to the code paths that log the detailed inspect output.
In `@packages/migrate/sources/beehiiv-api-members.js`:
- Around line 154-155: The two error pushes using ctx.errors.push('Failed to
write and upload output file', error) and ctx.errors.push('Failed to clear
cache', error) should be changed to the structured-object form used elsewhere;
replace those calls so they push a single object like ctx.errors.push({message:
'Failed to write and upload output file', error}) and ctx.errors.push({message:
'Failed to clear cache', error}) so the message/error pairing is preserved and
consistent with other uses of ctx.errors.push in this module.
In `@packages/migrate/sources/beehiiv-api.js`:
- Around line 72-129: Re-enable the post-processing tasks that run after JSON
formatting: restore the link map build (ctx.linkFixer.buildMap), the link-fixing
task (ctx.linkFixer.fix) and the HTML->Lexical conversion
(mgHtmlLexical.convert) into the task list surrounding the toGhostJSON and asset
scraping steps so exported data has links and HTML transformed before writing;
ensure the tasks use the same error handling pattern (push to ctx.errors and
rethrow) and return their task runners (makeTaskRunner) like the other tasks so
the pipeline executes them in order.
- Around line 61-68: The catch blocks push heterogeneous error entries into
ctx.errors; update each ctx.errors.push call (the ones inside the async task
that calls mgBeehiiv.mapPostsTasks and the other catches that log "Failed to
format data as Ghost JSON" and "Failed to write Ghost import JSON File") to use
the consistent structured object form used elsewhere: ctx.errors.push({ message:
'...', error }); — locate the catches around the async task that returns
makeTaskRunner(tasks, {...options}) and the other catch sites and replace the
two-argument pushes with a single object containing message and error so all
entries match the pattern at the earlier line that uses {message, error}.
---
Minor comments:
In `@packages/mg-beehiiv-api-members/README.md`:
- Around line 1-3: Replace the placeholder README in
packages/mg-beehiiv-api-members/README.md with a complete user-facing document:
add an "Authentication" section that lists required env vars or API token names
and scopes, a "CLI usage" section with example commands and required
flags/arguments for the package's CLI entrypoint, an "Expected outputs" section
showing sample successful and error JSON responses from the key functions (e.g.,
member create/list endpoints), and a "Beehiiv limitations" section describing
rate limits, pagination behavior, and any unsupported fields; include short
usage examples, exit codes, and troubleshooting tips so consumers can use the
package without reading the source.
In `@packages/mg-beehiiv-api-members/src/test/fetch.test.ts`:
- Around line 179-182: The mocked fetch responses used in the tests return an
object shaped for list endpoints ({ total_results }) but discover() expects the
publication payload and reads response.json().data.stats.active_subscriptions;
update the fetchMock.mock.mockImplementationOnce stubs used by tests that
exercise fetchTasks() (and indirectly discover()) to return a JSON shape
matching the real publication payload—i.e. response.json() should resolve to an
object with a data.stats.active_subscriptions numeric field (and any other
fields discover() needs such as has_more/page indicators) so estimatedPages is
computed correctly; apply the same change to the other test stubs referenced
(the blocks at the other locations listed) to ensure pagination/progress paths
are exercised.
In `@packages/mg-beehiiv-api/src/lib/process.ts`:
- Around line 483-499: Guard against undefined and missing segments before
splitting and comparing: ensure firstImgSrc is truthy before calling split or
replace, and verify both firstImgSrc and featureSrc contain '/uploads/asset/'
(or that split()[1] exists) before comparing their [1] parts; also check
featureSrc is non-empty before normalizing and comparing to normalizedFirstSrc.
Specifically, update the logic around variables firstImgSrc, firstImageSplit,
featureImageSplit and normalizedFirstSrc in the block using
$html(theElementItself) so you only call .split(), .replace(), or access [1]
when the source strings are defined and include the expected substring, and bail
out (skip removal) if those guards fail.
In `@packages/migrate/commands/beehiiv-api-members.js`:
- Around line 124-125: The verbose log is printing context.result.data which is
undefined because the members pipeline puts results in context.result.members;
update the ui.log.info call (the block guarded by argv.verbose and
context.result) to inspect and log context.result.members (e.g.,
inspect(context.result.members, false, 2)) so the actual member data is shown;
ensure you reference ui.log.info, argv.verbose and context.result.members when
making the change and keep the existing null-check on context.result or add a
fallback if needed.
- Around line 105-110: The CLI code uses raw console.table on getPubs which
violates lint and bypasses the command's logging/UI; replace the console.table
call with the command's logger/UI output (e.g., use the command context logger
or existing processLogger) and render the same rows by formatting
getPubs.map(...) into a string or using the repo's table helper before calling
logger.info/log or the command's print function; update the invocation around
getPubs and the anonymous map so it passes the formatted string to the logger
instead of calling console.table.
In `@packages/migrate/commands/beehiiv-api.js`:
- Line 16: Update the misleading command description stored in the const desc in
the beehiiv-api command so it accurately reflects that this command migrates
from Beehiiv via the Beehiiv API (not CSV files); locate the const desc in
packages/migrate/commands/beehiiv-api.js and replace the current text 'Migrate
from beehiiv using CSV files' with a short, accurate description such as
'Migrate from Beehiiv using the Beehiiv API' or similar.
- Line 1: The file imports inspect from node:util but never uses it; remove the
unused import statement "import {inspect} from 'node:util';" (from the top of
the module) and verify there are no remaining references to inspect in functions
like any helper or export in this module so ESLint/TS will stop flagging an
unused import.
- Around line 147-150: The trailing-slash handling for argv.url is dead code
because no --url option exists; remove the entire conditional block that checks
and slices argv.url (the if (argv.url && argv.url.endsWith('/')) { argv.url =
argv.url.slice(0, -1); }) from the migration command, or alternatively add a
corresponding --url option to the options array if the URL behavior was
intended—pick one and implement consistently so argv.url is never referenced
without a defined option.
---
Nitpick comments:
In `@packages/mg-beehiiv-api-members/src/index.ts`:
- Around line 5-9: Current default export is exporting utility functions as a
single default object; change this to named exports by exporting
listPublications, fetchTasks, and mapMembersTasks individually (remove the
default-exported object) so callers can import them as named imports; update any
internal references or import sites to use the named imports if necessary
(search for import of the default from this module and switch to {
listPublications, fetchTasks, mapMembersTasks }).
In `@packages/mg-beehiiv-api-members/src/lib/list-pubs.ts`:
- Around line 9-11: The current plain Error thrown when response.ok is false
should be replaced with an `@tryghost/errors` structured error: import errors from
'@tryghost/errors' and throw an instance that includes a descriptive message and
a context object (e.g., {status: response.status, statusText:
response.statusText, url: <request url or variable>}) in the list-pubs code path
where response is checked; update the code around response.ok in the listPubs
(or the function that performs the fetch) to construct and throw that
`@tryghost/errors` error instead of new Error(...) so logs and error handling get
structured message and context.
In `@packages/mg-beehiiv-api/src/index.ts`:
- Around line 5-9: Replace the default export object with named exports for the
utility functions: export the functions listPublications, fetchTasks, and
mapPostsTasks as named exports (e.g., export { listPublications, fetchTasks,
mapPostsTasks }) and update any consumers (such as
migrate/sources/beehiiv-api.js and migrate/commands/beehiiv-api.js) to import
those functions by name instead of using the default import.
In `@packages/mg-beehiiv-api/src/lib/client.ts`:
- Around line 1-18: The file defines a duplicate async function client that
issues a GET to /publications; instead of keeping this second publications
client, remove or replace its usage and delegate to the existing authedClient
used by list-pubs.ts to avoid divergence (ensure the same query params such as
expand[]=stats are preserved). Locate the client function in this file and
either delete it or refactor callers to call the shared authedClient wrapper
(the same helper used by packages/mg-beehiiv-api/src/lib/list-pubs.ts) so the
exported surface matches index.ts and the requests remain identical.
In `@packages/mg-beehiiv-api/src/lib/process.ts`:
- Around line 3-5: Remove the dead imports sanitizeHtml and imageCard from the
top of the file—keep only the actually used SimpleDom import; update the import
list in packages/mg-beehiiv-api/src/lib/process.ts by deleting the sanitizeHtml
and imageCard import statements (they are only referenced in commented-out code)
so there are no unused imports left.
- Around line 160-430: The large ~270-line commented block in process.ts (inside
the HTML cleaning logic that manipulates $html, options, postData, imageCard,
embedCard, bookmarkCard, serializer, getYouTubeID, etc.) should be removed or
moved out of the file: either delete it (history preserves it), extract it into
a dedicated reference file (e.g., a legacy or experimental helpers file) or add
it to a design doc/issue tracking planned features; update imports/usages if you
relocate helpers and ensure the active processing functions (the HTML-cleanup
routine surrounding $html and related handlers) remain clean and focused.
In `@packages/mg-beehiiv-api/tsconfig.json`:
- Line 110: The tsconfig's "include" currently lists "src/**/*" and the
redundant "src/index.js"; remove "src/index.js" from the "include" array if the
codebase is TypeScript-only (so only "src/**/*" remains), otherwise if there
truly is a JavaScript entry point enable "allowJs": true (the commented out
"allowJs" setting around line 48) so the JS file will be picked up—apply the
appropriate change in the tsconfig.json.
- Around line 86-87: The tsconfig.json contains a redundant "noImplicitAny"
entry because "strict": true already enables it; remove the explicit
"noImplicitAny" property (or alternatively set it to identical value via a
comment if you prefer explicitness) so only "strict": true remains as the single
source of truth for strict checks; locate the JSON object containing the
"strict" and "noImplicitAny" keys and delete the "noImplicitAny" line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be04ed1d-a0da-4510-90ed-e88179c30dbc
⛔ Files ignored due to path filters (2)
packages/mg-beehiiv-api/src/test/fixtures/posts.csvis excluded by!**/*.csvyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (37)
packages/mg-beehiiv-api-members/.eslintrc.cjspackages/mg-beehiiv-api-members/README.mdpackages/mg-beehiiv-api-members/package.jsonpackages/mg-beehiiv-api-members/src/index.tspackages/mg-beehiiv-api-members/src/lib/fetch.tspackages/mg-beehiiv-api-members/src/lib/list-pubs.tspackages/mg-beehiiv-api-members/src/lib/mapper.tspackages/mg-beehiiv-api-members/src/test/fetch.test.tspackages/mg-beehiiv-api-members/src/test/index.test.tspackages/mg-beehiiv-api-members/src/test/mapper.test.tspackages/mg-beehiiv-api-members/src/types.d.tspackages/mg-beehiiv-api-members/tsconfig.jsonpackages/mg-beehiiv-api/.eslintrc.cjspackages/mg-beehiiv-api/.gitignorepackages/mg-beehiiv-api/LICENSEpackages/mg-beehiiv-api/README.mdpackages/mg-beehiiv-api/package.jsonpackages/mg-beehiiv-api/src/index.tspackages/mg-beehiiv-api/src/lib/client.tspackages/mg-beehiiv-api/src/lib/fetch.tspackages/mg-beehiiv-api/src/lib/list-pubs.tspackages/mg-beehiiv-api/src/lib/mapper.tspackages/mg-beehiiv-api/src/lib/process.tspackages/mg-beehiiv-api/src/test/client.test.tspackages/mg-beehiiv-api/src/test/fetch.test.tspackages/mg-beehiiv-api/src/test/index.test.tspackages/mg-beehiiv-api/src/test/list-pubs.test.tspackages/mg-beehiiv-api/src/test/mapper.test.tspackages/mg-beehiiv-api/src/test/process.test.tspackages/mg-beehiiv-api/src/types.d.tspackages/mg-beehiiv-api/tsconfig.jsonpackages/migrate/bin/cli.jspackages/migrate/commands/beehiiv-api-members.jspackages/migrate/commands/beehiiv-api.jspackages/migrate/package.jsonpackages/migrate/sources/beehiiv-api-members.jspackages/migrate/sources/beehiiv-api.js
Comment on lines
+10
to
+12
| 'ghost/ghost-custom/no-native-errors': 'off', | ||
| 'ghost/ghost-custom/no-native-error': 'off', | ||
| 'ghost/ghost-custom/ghost-error-usage': 'off', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/mg-beehiiv-api-members/.eslintrc.cjs | head -20Repository: TryGhost/migrate
Length of output: 689
🏁 Script executed:
# Search for ESLint rule definitions
fd -e js -e cjs . --exec grep -l "no-native-errors\|no-native-error\|ghost-error-usage" {} \;Repository: TryGhost/migrate
Length of output: 101
🏁 Script executed:
# Check if `@tryghost/errors` is used elsewhere in the project
rg "@tryghost/errors" --type js --type ts -c | head -20Repository: TryGhost/migrate
Length of output: 925
🏁 Script executed:
# Look for Ghost custom ESLint rules directory
fd -type d -name "ghost-custom" .Repository: TryGhost/migrate
Length of output: 230
🏁 Script executed:
# Find the ghost ESLint plugin
find . -name "package.json" | xargs grep -l "eslint-plugin-ghost" 2>/dev/null | head -5Repository: TryGhost/migrate
Length of output: 196
🏁 Script executed:
# Check beehiiv package source files
ls -la packages/mg-beehiiv-api-members/ | head -20Repository: TryGhost/migrate
Length of output: 478
🏁 Script executed:
# Look for source code files in beehiiv package
fd . packages/mg-beehiiv-api-members --type f -name "*.js" -o -name "*.ts" | grep -v node_modules | head -20Repository: TryGhost/migrate
Length of output: 290
🏁 Script executed:
# Check what errors are actually used in beehiiv package
rg "throw\s|Error" packages/mg-beehiiv-api-members --type js --type ts -A 2 | head -50Repository: TryGhost/migrate
Length of output: 3798
🏁 Script executed:
# Check the ESLint plugin rules definitions
cat node_modules/eslint-plugin-ghost/lib/rules/*.js | grep -A 10 "no-native-errors\|no-native-error\|ghost-error-usage" | head -50Repository: TryGhost/migrate
Length of output: 141
🏁 Script executed:
# List available rules in the plugin
ls -la node_modules/eslint-plugin-ghost/lib/rules/Repository: TryGhost/migrate
Length of output: 150
🏁 Script executed:
# Search for documentation or rule definitions
rg "no-native-errors|no-native-error|ghost-error-usage" node_modules/eslint-plugin-ghost --type js -B 2 -A 5 | head -60Repository: TryGhost/migrate
Length of output: 4497
🏁 Script executed:
# Check if `@tryghost/errors` is referenced elsewhere or in docs
rg "@tryghost/errors" . --type md --type txt | head -20Repository: TryGhost/migrate
Length of output: 168
🏁 Script executed:
# Check the actual rule implementations
cat node_modules/eslint-plugin-ghost/lib/custom/rules/no-native-error.jsRepository: TryGhost/migrate
Length of output: 659
🏁 Script executed:
# Check the ghost-error-usage rule
cat node_modules/eslint-plugin-ghost/lib/custom/rules/ghost-error-usage.jsRepository: TryGhost/migrate
Length of output: 1119
🏁 Script executed:
# Check if there's a no-native-errors rule (note: plural)
ls node_modules/eslint-plugin-ghost/lib/custom/rules/ | grep nativeRepository: TryGhost/migrate
Length of output: 78
🏁 Script executed:
# Check if no-native-errors (plural) rule exists in the plugin index
rg "no-native-errors" node_modules/eslint-plugin-ghostRepository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
# Check the complete list of custom rules in the plugin
cat node_modules/eslint-plugin-ghost/lib/custom/index.jsRepository: TryGhost/migrate
Length of output: 629
🏁 Script executed:
# Double-check what rules are actually exported in the node config
cat node_modules/eslint-plugin-ghost/lib/config/node.js | grep -A 20 "rules:"Repository: TryGhost/migrate
Length of output: 1012
Address the native Error instances and re-enable the lint rules.
The package disables two real rules (ghost/ghost-custom/no-native-error and ghost/ghost-custom/ghost-error-usage) that enforce @tryghost/errors usage. The actual code contains violations of these rules: src/lib/list-pubs.ts and src/lib/fetch.ts both use native Error(). Rather than disabling enforcement, update these files to use @tryghost/errors and remove the rule overrides. Note: ghost/ghost-custom/no-native-errors (as written in the config) does not exist as a rule in the plugin; only no-native-error (singular) is enforced.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mg-beehiiv-api-members/.eslintrc.cjs` around lines 10 - 12, The
config currently disables the enforced `@tryghost/errors` rules; revert that by
removing the two overrides ('ghost/ghost-custom/no-native-error' and
'ghost/ghost-custom/ghost-error-usage') from .eslintrc.cjs and re-enable
linting. In code, replace native Error usages in src/lib/list-pubs.ts and
src/lib/fetch.ts: import and throw the appropriate `@tryghost/errors` class (e.g.,
GhostError or a more specific error like BadRequestError/NotFoundError) instead
of new Error(...), and ensure any error construction/messaging matches the
chosen `@tryghost/errors` API so the linter passes. Ensure imports reference
`@tryghost/errors` at the top of each file and update thrown instances to use
those classes.
Comment on lines
+19
to
+20
| if (!response.ok) { | ||
| throw new Error(`Request failed: ${response.status} ${response.statusText}`); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view its contents
cat -n "packages/mg-beehiiv-api-members/src/lib/fetch.ts"Repository: TryGhost/migrate
Length of output: 4386
🏁 Script executed:
# Search for `@tryghost/errors` usage in the project to understand the pattern
rg "@tryghost/errors" --type ts -A 3 -B 3Repository: TryGhost/migrate
Length of output: 3368
🏁 Script executed:
# Check if `@tryghost/errors` is available in dependencies
rg "@tryghost/errors" package.json | head -20Repository: TryGhost/migrate
Length of output: 92
🏁 Script executed:
# Look for how errors are structured in similar files in the codebase
fd "fetch.ts\|error" packages/*/src --type f | head -20Repository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
# Check how `@tryghost/errors` is used in existing files with context
rg "errors\." packages/mg-medium-members/src/lib/read-txt.ts -A 2 -B 1Repository: TryGhost/migrate
Length of output: 293
🏁 Script executed:
# Get more comprehensive examples of `@tryghost/errors` usage
rg "new errors\.|errors\.BadRequest|errors\.NotFound|errors\.([\w]+)" packages/ -A 1 -B 1 | head -60Repository: TryGhost/migrate
Length of output: 3910
🏁 Script executed:
# Check the node_modules or package definition for `@tryghost/errors` to understand the API
find . -name "errors" -type d 2>/dev/null | head -5Repository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
# Search for structured error handling patterns in similar API modules
rg "throw new errors\." packages/ --type ts -B 2 -A 2 | head -80Repository: TryGhost/migrate
Length of output: 6520
Use @tryghost/errors for structured Beehiiv API errors.
Replace raw Error throws with structured errors using @tryghost/errors. Include request context (pubId, cursor, cursorIndex) to improve debuggability.
This file matches the coding guideline: "Use @tryghost/errors for structured error handling with message and context properties."
Also applies to: 52–53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mg-beehiiv-api-members/src/lib/fetch.ts` around lines 19 - 20,
Replace raw Error throws in the response.ok checks with a structured error from
`@tryghost/errors`: import the package and throw a `@tryghost/errors` error (e.g.
new errors.InternalServerError or a suitable subclass) that includes a
descriptive message and a context object containing pubId, cursor, cursorIndex
and the HTTP status/statusText; update both places where the code currently does
throw new Error(...) (the response.ok check around the first failure and the
second occurrence at the later response check) so the thrown error uses the
`@tryghost/errors` API and attaches the request context for debuggability.
Comment on lines
+1
to
+45
| declare module '@tryghost/mg-fs-utils'; | ||
| declare module '@tryghost/string'; | ||
|
|
||
| type BeehiivSubscription = { | ||
| id: string; | ||
| email: string; | ||
| status: 'active' | 'inactive' | 'validating' | 'pending'; | ||
| created: number; | ||
| subscription_tier: 'free' | 'premium'; | ||
| subscription_premium_tier_names: string[]; | ||
| stripe_customer_id: string | null; | ||
| custom_fields: Array<{name: string; value: string}>; | ||
| tags: string[]; | ||
| }; | ||
|
|
||
| type BeehiivPublicationResponse = { | ||
| data: { | ||
| stats: { | ||
| active_subscriptions: number; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| type BeehiivSubscriptionsResponse = { | ||
| data: BeehiivSubscription[]; | ||
| total_results: number; | ||
| has_more: boolean; | ||
| next_cursor: string | null; | ||
| }; | ||
|
|
||
| type GhostMemberObject = { | ||
| email: string; | ||
| name: string | null; | ||
| note: string | null; | ||
| subscribed_to_emails: boolean; | ||
| stripe_customer_id: string; | ||
| complimentary_plan: boolean; | ||
| labels: string[]; | ||
| created_at: Date; | ||
| }; | ||
|
|
||
| type MappedMembers = { | ||
| free: GhostMemberObject[]; | ||
| paid: GhostMemberObject[]; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual types.d.ts file
cat -n packages/mg-beehiiv-api-members/src/types.d.tsRepository: TryGhost/migrate
Length of output: 1422
🏁 Script executed:
# Check if there are any imports of these types from the declaration file
rg -r "from.*types|import.*BeehiivSubscription|import.*GhostMemberObject|import.*MappedMembers" packages/mg-beehiiv-api-members/Repository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
# Check the package.json to understand the package structure
cat packages/mg-beehiiv-api-members/package.jsonRepository: TryGhost/migrate
Length of output: 1348
🏁 Script executed:
# Look for tsconfig.json to understand type declaration handling
find packages/mg-beehiiv-api-members -name "tsconfig.json" -o -name "tsconfig*.json" | head -5Repository: TryGhost/migrate
Length of output: 105
🏁 Script executed:
# Check tsconfig.json
cat packages/mg-beehiiv-api-members/tsconfig.jsonRepository: TryGhost/migrate
Length of output: 1740
🏁 Script executed:
# Look for any TypeScript source files that might use these types
find packages/mg-beehiiv-api-members/src -name "*.ts" -type fRepository: TryGhost/migrate
Length of output: 469
🏁 Script executed:
# Check main index.ts file
cat packages/mg-beehiiv-api-members/src/index.tsRepository: TryGhost/migrate
Length of output: 283
🏁 Script executed:
# Check mapper.ts which likely uses MappedMembers type
cat packages/mg-beehiiv-api-members/src/lib/mapper.tsRepository: TryGhost/migrate
Length of output: 3809
🏁 Script executed:
# Check fetch.ts which likely uses response types
cat packages/mg-beehiiv-api-members/src/lib/fetch.tsRepository: TryGhost/migrate
Length of output: 3602
🏁 Script executed:
# Check list-pubs.ts as well
cat packages/mg-beehiiv-api-members/src/lib/list-pubs.tsRepository: TryGhost/migrate
Length of output: 545
🏁 Script executed:
# Verify the build output includes types.d.ts and check if there are any export wrappers
find packages/mg-beehiiv-api-members/build -name "*.d.ts" 2>/dev/null | head -5Repository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
# Check if build/types.d.ts exists and what it contains (to verify ambient behavior)
cat packages/mg-beehiiv-api-members/build/types.d.ts 2>/dev/null | head -50Repository: TryGhost/migrate
Length of output: 42
Export types from the module instead of declaring them as ambient globals.
The types in this file (BeehiivSubscription, GhostMemberObject, MappedMembers, BeehiivPublicationResponse, BeehiivSubscriptionsResponse) have no imports or exports, making them ambient globals. When compiled and published via the "types" entry in package.json, they will pollute the namespace of every consumer.
Instead, export these types from the module and import them where needed in mapper.ts and fetch.ts. Alternatively, move the declare module shims to a separate ambient declaration file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mg-beehiiv-api-members/src/types.d.ts` around lines 1 - 45, The
types are currently declared as ambient globals and must be exported to avoid
polluting consumers; change the type declarations (BeehiivSubscription,
BeehiivPublicationResponse, BeehiivSubscriptionsResponse, GhostMemberObject,
MappedMembers) to exported types (e.g., export type ...) and then update
mapper.ts and fetch.ts to import the needed types instead of relying on globals;
alternatively move the two declare module shims into a separate ambient
declaration file (e.g., a .d.ts) and keep only exported type definitions in this
file so consumers only receive explicit exports.
Comment on lines
+10
to
+12
| 'ghost/ghost-custom/no-native-errors': 'off', | ||
| 'ghost/ghost-custom/no-native-error': 'off', | ||
| 'ghost/ghost-custom/ghost-error-usage': 'off', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the .eslintrc.cjs file to verify the configuration
cat -n packages/mg-beehiiv-api/.eslintrc.cjsRepository: TryGhost/migrate
Length of output: 689
🏁 Script executed:
# Check if the client.ts file exists and examine its content for native throws
if [ -f "packages/mg-beehiiv-api/src/lib/client.ts" ]; then
echo "=== File exists, checking for native throws ==="
cat -n packages/mg-beehiiv-api/src/lib/client.ts
else
echo "File does not exist at expected path"
# Try to find it
find packages/mg-beehiiv-api -name "client.ts" -o -name "client.js"
fiRepository: TryGhost/migrate
Length of output: 727
🏁 Script executed:
# Search for Ghost error handling patterns and guidelines in the repository
rg "@tryghost/errors" --type js --type ts -B 2 -A 2 | head -50Repository: TryGhost/migrate
Length of output: 3272
🏁 Script executed:
# Look for ESLint rule definitions for the ghost/ghost-custom rules
find . -name "*.js" -o -name "*.cjs" | xargs grep -l "no-native-errors\|no-native-error\|ghost-error-usage" 2>/dev/null | head -20Repository: TryGhost/migrate
Length of output: 756
Remove the disabled ESLint rules to enforce structured error handling.
Lines 10-12 disable the Ghost error linting rules, which allows native errors to bypass the package's error handling standards. The code at src/lib/client.ts:12 already contains a native throw new Error(...) that violates the structured error pattern used across the codebase. Remove these rule disables and migrate the error handling to @tryghost/errors instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mg-beehiiv-api/.eslintrc.cjs` around lines 10 - 12, Remove the three
disabled ESLint rules ('ghost/ghost-custom/no-native-errors',
'ghost/ghost-custom/no-native-error', 'ghost/ghost-custom/ghost-error-usage')
from .eslintrc.cjs so the Ghost error linting is enforced, then update the
native throw in src/lib/client.ts (the "throw new Error(...)" at the top of the
file) to use the structured error types from `@tryghost/errors`: import the
appropriate error class from `@tryghost/errors` (e.g., errors.GhostError or the
most semantically appropriate errors.* class) and throw that instead with the
same message/metadata so the package conforms to the project's structured error
handling.
Comment on lines
+1
to
+3
| # Migrate beehiiv API | ||
|
|
||
| ... |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Complete the README documentation.
The README is currently a placeholder and needs comprehensive documentation before this PR can be merged. A migration package should include:
- Package description and purpose
- Installation instructions
- Usage examples and CLI commands
- API documentation
- Configuration options (e.g., Beehiiv API credentials)
- Input/output format examples
- Troubleshooting guide
Would you like me to generate a comprehensive README template based on the package's functionality described in the AI summary?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mg-beehiiv-api/README.md` around lines 1 - 3, The README.md is a
placeholder and must be expanded into a full package README: add a brief package
description and purpose, installation instructions (npm/yarn/pnpm commands),
usage examples including CLI commands and code snippets showing how to call the
package, API documentation listing exported functions/endpoints and expected
parameters, configuration details for Beehiiv credentials and env vars, examples
of input/output formats and sample responses, and a troubleshooting/FAQ section;
update the top-level README.md in this package
(packages/mg-beehiiv-api/README.md) to include these sections and concrete
examples matching the package's exported API and configuration keys.
Comment on lines
+96
to
+115
| // If no publication ID is provided, list publications and exit | ||
| if (!argv.id) { | ||
| const getPubs = await mgBeehiivApiMembers.listPublications(argv.key); | ||
|
|
||
| if (!getPubs || !getPubs.length) { | ||
| ui.log.error('Error fetching publications:', getPubs?.errors || 'No publications found'); | ||
| return; | ||
| } | ||
|
|
||
| console.table(getPubs.map(pub => ({ | ||
| name: pub.name, | ||
| id: pub.id, | ||
| created: new Date(pub.created * 1000), | ||
| subscribers: pub.stats?.active_subscriptions || '-' | ||
| }))); // eslint-disable-line no-console | ||
|
|
||
| ui.log.warn('No publication ID provided. Please provide an ID using the --id flag to run the migration.'); | ||
|
|
||
| process.exit(0); | ||
| } |
Contributor
There was a problem hiding this comment.
The publication-listing path bypasses your catch block.
If listPublications() rejects here, run() exits before the try at Line 117 and ui.log.error() never runs. Move the no---id branch inside the existing try and return instead of hard-exiting the process.
Suggested fix
- // If no publication ID is provided, list publications and exit
- if (!argv.id) {
- const getPubs = await mgBeehiivApiMembers.listPublications(argv.key);
-
- if (!getPubs || !getPubs.length) {
- ui.log.error('Error fetching publications:', getPubs?.errors || 'No publications found');
- return;
- }
-
- console.table(getPubs.map(pub => ({
- name: pub.name,
- id: pub.id,
- created: new Date(pub.created * 1000),
- subscribers: pub.stats?.active_subscriptions || '-'
- }))); // eslint-disable-line no-console
-
- ui.log.warn('No publication ID provided. Please provide an ID using the --id flag to run the migration.');
-
- process.exit(0);
- }
-
try {
+ // If no publication ID is provided, list publications and exit
+ if (!argv.id) {
+ const getPubs = await mgBeehiivApiMembers.listPublications(argv.key);
+
+ if (!getPubs || !getPubs.length) {
+ ui.log.error('Error fetching publications:', getPubs?.errors || 'No publications found');
+ return;
+ }
+
+ console.table(getPubs.map(pub => ({
+ name: pub.name,
+ id: pub.id,
+ created: new Date(pub.created * 1000),
+ subscribers: pub.stats?.active_subscriptions || '-'
+ }))); // eslint-disable-line no-console
+
+ ui.log.warn('No publication ID provided. Please provide an ID using the --id flag to run the migration.');
+ return;
+ }
+
// Fetch the tasks, configured correctly according to the options passed in
let migrate = beehiivApiMembers.getTaskRunner(argv);📝 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.
Suggested change
| // If no publication ID is provided, list publications and exit | |
| if (!argv.id) { | |
| const getPubs = await mgBeehiivApiMembers.listPublications(argv.key); | |
| if (!getPubs || !getPubs.length) { | |
| ui.log.error('Error fetching publications:', getPubs?.errors || 'No publications found'); | |
| return; | |
| } | |
| console.table(getPubs.map(pub => ({ | |
| name: pub.name, | |
| id: pub.id, | |
| created: new Date(pub.created * 1000), | |
| subscribers: pub.stats?.active_subscriptions || '-' | |
| }))); // eslint-disable-line no-console | |
| ui.log.warn('No publication ID provided. Please provide an ID using the --id flag to run the migration.'); | |
| process.exit(0); | |
| } | |
| try { | |
| // If no publication ID is provided, list publications and exit | |
| if (!argv.id) { | |
| const getPubs = await mgBeehiivApiMembers.listPublications(argv.key); | |
| if (!getPubs || !getPubs.length) { | |
| ui.log.error('Error fetching publications:', getPubs?.errors || 'No publications found'); | |
| return; | |
| } | |
| console.table(getPubs.map(pub => ({ | |
| name: pub.name, | |
| id: pub.id, | |
| created: new Date(pub.created * 1000), | |
| subscribers: pub.stats?.active_subscriptions || '-' | |
| }))); // eslint-disable-line no-console | |
| ui.log.warn('No publication ID provided. Please provide an ID using the --id flag to run the migration.'); | |
| return; | |
| } | |
| // Fetch the tasks, configured correctly according to the options passed in | |
| let migrate = beehiivApiMembers.getTaskRunner(argv); |
🧰 Tools
🪛 ESLint
[error] 105-105: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/commands/beehiiv-api-members.js` around lines 96 - 115, Move
the no---id publication listing branch inside the existing try block within
run() so any rejection from mgBeehiivApiMembers.listPublications(argv.key) is
caught; replace process.exit(0) with a return to exit the function gracefully,
and ensure errors use ui.log.error (preserving the current table/log output and
the ui.log.warn message) so listPublications() failures are handled by the
surrounding catch.
Comment on lines
+180
to
+183
| if (argv.verbose && context.result) { | ||
| // ui.log.info('Done', inspect(context.result.data, false, 2)); | ||
| ui.log.info('Done'); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing verbose option causes broken functionality.
The code checks argv.verbose but the --verbose option is commented out in the options array (lines 121-125). Additionally, the task runner in sources/beehiiv-api.js also expects options.verbose for the AssetScraper task. This will cause the verbose functionality to silently fail.
Either uncomment the verbose option or remove this check.
🐛 Proposed fix: Uncomment the verbose option
Uncomment lines 121-125:
- // {
- // type: 'boolean',
- // flags: '-V --verbose',
- // defaultValue: Boolean(process?.env?.DEBUG),
- // desc: 'Show verbose output'
- // },
+ {
+ type: 'boolean',
+ flags: '-V --verbose',
+ defaultValue: Boolean(process?.env?.DEBUG),
+ desc: 'Show verbose output'
+ },📝 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.
Suggested change
| if (argv.verbose && context.result) { | |
| // ui.log.info('Done', inspect(context.result.data, false, 2)); | |
| ui.log.info('Done'); | |
| } | |
| { | |
| type: 'boolean', | |
| flags: '-V --verbose', | |
| defaultValue: Boolean(process?.env?.DEBUG), | |
| desc: 'Show verbose output' | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/commands/beehiiv-api.js` around lines 180 - 183, The verbose
flag check (argv.verbose) is used but the corresponding option in the options
array is commented out, which breaks verbose behavior and also leaves
AssetScraper expecting options.verbose; restore the CLI option by uncommenting
the verbose option declaration in the options array so argv.verbose and
options.verbose are defined, or alternatively remove the argv.verbose check and
any references to options.verbose in the AssetScraper task in
sources/beehiiv-api.js—specifically ensure the CLI options array includes the
verbose option name/flag so argv.verbose and options.verbose are available to
the code paths that log the detailed inspect output.
Comment on lines
+154
to
+155
| } catch (error) { | ||
| ctx.errors.push('Failed to write and upload output file', error); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/migrate/sources && wc -l beehiiv-api-members.jsRepository: TryGhost/migrate
Length of output: 86
🏁 Script executed:
cd packages/migrate/sources && head -n 175 beehiiv-api-members.js | tail -n +145Repository: TryGhost/migrate
Length of output: 1523
🏁 Script executed:
cd packages/migrate/sources && sed -n '69p;116p' beehiiv-api-members.jsRepository: TryGhost/migrate
Length of output: 233
🏁 Script executed:
cd packages/migrate && grep -n "@tryghost/errors\|import.*errors" sources/beehiiv-api-members.jsRepository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
cd packages/migrate/sources && head -n 30 beehiiv-api-members.jsRepository: TryGhost/migrate
Length of output: 1166
🏁 Script executed:
cd packages/migrate/sources && grep -n "ctx\." beehiiv-api-members.js | head -n 20Repository: TryGhost/migrate
Length of output: 1621
🏁 Script executed:
cd packages/migrate/sources && grep -n "ctx.errors" beehiiv-api-members.jsRepository: TryGhost/migrate
Length of output: 403
🏁 Script executed:
cd packages/migrate && grep -r "ctx.errors" --include="*.js" | head -n 20Repository: TryGhost/migrate
Length of output: 2347
🏁 Script executed:
cd packages/migrate && grep -n "\.errors\s*=" --include="*.js" -r lib/Repository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
cd packages/migrate && grep -B5 -A5 "ctx.errors\s*=" sources/beehiiv-api-members.jsRepository: TryGhost/migrate
Length of output: 42
🏁 Script executed:
cd packages/migrate && grep -n "errors.*=\|\.errors" --include="*.js" lib/utilties/json.js | head -n 20Repository: TryGhost/migrate
Length of output: 201
🏁 Script executed:
cd packages/migrate && grep -B10 "ctx.errors" sources/beehiiv-api-members.js | head -n 25Repository: TryGhost/migrate
Length of output: 1512
Use consistent structured error objects across all error pushes in this file.
Lines 155 and 167 call ctx.errors.push(message, error), which adds two separate entries to the array. Lines 69 and 116 use ctx.errors.push({message: '...', error}), storing a single object. This inconsistency makes downstream inspection of ctx.errors unpredictable and loses the message/error pairing.
Update lines 155 and 167 to match the pattern used elsewhere in the file:
ctx.errors.push({message: 'Failed to write and upload output file', error});
ctx.errors.push({message: 'Failed to clear cache', error});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/sources/beehiiv-api-members.js` around lines 154 - 155, The
two error pushes using ctx.errors.push('Failed to write and upload output file',
error) and ctx.errors.push('Failed to clear cache', error) should be changed to
the structured-object form used elsewhere; replace those calls so they push a
single object like ctx.errors.push({message: 'Failed to write and upload output
file', error}) and ctx.errors.push({message: 'Failed to clear cache', error}) so
the message/error pairing is preserved and consistent with other uses of
ctx.errors.push in this module.
Comment on lines
+61
to
+68
| title: 'Map & process content', | ||
| task: async (ctx) => { | ||
| try { | ||
| let tasks = await mgBeehiiv.mapPostsTasks(options, ctx); | ||
| return makeTaskRunner(tasks, {...options}); | ||
| } catch (error) { | ||
| ctx.errors.push('Failed to process beehiiv content', error); // eslint-disable-line no-console | ||
| throw error; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the file and check its contents
find . -name "beehiiv-api.js" -type fRepository: TryGhost/migrate
Length of output: 144
🏁 Script executed:
# Read the relevant sections of the file
if [ -f "packages/migrate/sources/beehiiv-api.js" ]; then
echo "=== Lines 50-75 ===" && sed -n '50,75p' packages/migrate/sources/beehiiv-api.js
echo ""
echo "=== Lines 80-95 ===" && sed -n '80,95p' packages/migrate/sources/beehiiv-api.js
echo ""
echo "=== Lines 126-140 ===" && sed -n '126,140p' packages/migrate/sources/beehiiv-api.js
fiRepository: TryGhost/migrate
Length of output: 2322
🏁 Script executed:
# Check if `@tryghost/errors` is imported in this file
grep -n "@tryghost/errors" packages/migrate/sources/beehiiv-api.js || echo "Not found in this file"Repository: TryGhost/migrate
Length of output: 82
🏁 Script executed:
# Check how ctx.errors is typically populated across the codebase
echo "=== Searching for ctx.errors.push patterns ===" && \
rg "ctx\.errors\.push" --context 2 packages/migrate/ | head -100Repository: TryGhost/migrate
Length of output: 7026
🏁 Script executed:
# Check if `@tryghost/errors` is used in the codebase
echo "=== Checking `@tryghost/errors` usage ===" && \
rg "@tryghost/errors" packages/migrate/ | head -20Repository: TryGhost/migrate
Length of output: 99
Use consistent structured error format across all catch blocks.
Lines 68, 90, and 137 push message and error as separate arguments, while line 55 correctly uses {message, error}. This creates a heterogeneous error array that breaks message/error pairing and inconsistency with the established pattern in other source files (e.g., ghost.js).
Affected lines
- Line 68:
ctx.errors.push('Failed to process beehiiv content', error); - Line 90:
ctx.errors.push('Failed to format data as Ghost JSON', error); - Line 137:
ctx.errors.push('Failed to write Ghost import JSON File', error);
All should match the structured format at line 55: ctx.errors.push({message: 'message text', error});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/sources/beehiiv-api.js` around lines 61 - 68, The catch
blocks push heterogeneous error entries into ctx.errors; update each
ctx.errors.push call (the ones inside the async task that calls
mgBeehiiv.mapPostsTasks and the other catches that log "Failed to format data as
Ghost JSON" and "Failed to write Ghost import JSON File") to use the consistent
structured object form used elsewhere: ctx.errors.push({ message: '...', error
}); — locate the catches around the async task that returns
makeTaskRunner(tasks, {...options}) and the other catch sites and replace the
two-argument pushes with a single object containing message and error so all
entries match the pattern at the earlier line that uses {message, error}.
Comment on lines
+72
to
+129
| // { | ||
| // title: 'Build Link Map', | ||
| // task: async (ctx) => { | ||
| // // 4. Create a map of all known links for use later | ||
| // try { | ||
| // ctx.linkFixer.buildMap(ctx); | ||
| // } catch (error) { | ||
| // ctx.errors.push('Failed to build link map', error); // eslint-disable-line no-console | ||
| // throw error; | ||
| // } | ||
| // } | ||
| // }, | ||
| { | ||
| title: 'Format data as Ghost JSON', | ||
| task: async (ctx) => { | ||
| // 5. Format the data as a valid Ghost JSON file | ||
| try { | ||
| ctx.result = await toGhostJSON(ctx.result, ctx.options, ctx); | ||
| } catch (error) { | ||
| ctx.errors.push('Failed to format data as Ghost JSON', error); // eslint-disable-line no-console | ||
| throw error; | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| title: 'Fetch images via AssetScraper', | ||
| skip: ctx => !ctx.allowScrape.assets, | ||
| task: async (ctx) => { | ||
| // 6. Format the data as a valid Ghost JSON file | ||
| let tasks = ctx.assetScraper.getTasks(); | ||
| return makeTaskRunner(tasks, { | ||
| verbose: options.verbose, | ||
| exitOnError: false, | ||
| concurrent: false | ||
| }); | ||
| } | ||
| }, | ||
| // { | ||
| // title: 'Update links in content via LinkFixer', | ||
| // task: async (ctx, task) => { | ||
| // // 7. Process the content looking for known links, and update them to new links | ||
| // let tasks = ctx.linkFixer.fix(ctx, task); | ||
| // return makeTaskRunner(tasks, options); | ||
| // } | ||
| // }, | ||
| // { | ||
| // title: 'Convert HTML -> Lexical', | ||
| // task: (ctx) => { | ||
| // // 8. Convert post HTML -> Lexical | ||
| // try { | ||
| // let tasks = mgHtmlLexical.convert(ctx); | ||
| // return makeTaskRunner(tasks, options); | ||
| // } catch (error) { | ||
| // ctx.errors.push('Failed to convert HTML -> Lexical', error); // eslint-disable-line no-console | ||
| // throw error; | ||
| // } | ||
| // } | ||
| // }, |
Contributor
There was a problem hiding this comment.
Re-enable the post-processing steps before writing the import.
packages/migrate/sources/beehiiv.js:175-230 still runs link fixing and HTML→Lexical conversion after JSON formatting. In this new pipeline both stages are commented out, so the export is written with Beehiiv links/raw HTML untouched.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/sources/beehiiv-api.js` around lines 72 - 129, Re-enable the
post-processing tasks that run after JSON formatting: restore the link map build
(ctx.linkFixer.buildMap), the link-fixing task (ctx.linkFixer.fix) and the
HTML->Lexical conversion (mgHtmlLexical.convert) into the task list surrounding
the toGhostJSON and asset scraping steps so exported data has links and HTML
transformed before writing; ensure the tasks use the same error handling pattern
(push to ctx.errors and rethrow) and return their task runners (makeTaskRunner)
like the other tasks so the pipeline executes them in order.
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.
WIP