Skip to content

Added reusable external media imports - #30332

Merged
PaulAdamDavis merged 2 commits into
mainfrom
external-media-inliner-url-api
Aug 27, 2026
Merged

Added reusable external media imports#30332
PaulAdamDavis merged 2 commits into
mainfrom
external-media-inliner-url-api

Conversation

@PaulAdamDavis

Copy link
Copy Markdown
Member

no ref

CSV imports need to store one discovered media URL at a time without inheriting the existing batch service's content-matching rules. Expose a typed per-URL result directly from the existing CommonJS service while preserving the batch importer's failure boundaries and established module contract, allowing future callers to reuse media downloading and storage without changing existing batch behavior.

no ref

CSV imports need to store one discovered media URL at a time without inheriting the existing batch service's content-matching rules. Expose a typed per-URL result directly from the existing CommonJS service while preserving the batch importer's failure boundaries and established module contract, allowing future callers to reuse media downloading and storage without changing existing batch behavior.
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 680d77c3-5ca3-4866-bc44-686d06985e5b

📥 Commits

Reviewing files that changed from the base of the PR and between 9e4d3d6 and 98d6581.

📒 Files selected for processing (4)
  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/external-media-inliner.js
  • ghost/core/core/server/services/media-inliner/types.ts
  • ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (14)
  • GitHub Check: Legacy tests (Node 22.23.1, better-sqlite3)
  • GitHub Check: Legacy tests (Node 22.23.1, mysql8)
  • GitHub Check: Build Docker Images
  • GitHub Check: Stripe fixture checks
  • GitHub Check: Acceptance tests (Node 22.23.1, better-sqlite3)
  • GitHub Check: Acceptance tests (Node 22.23.1, mysql8)
  • GitHub Check: Unit tests (Node 22.23.1)
  • GitHub Check: Build Admin
  • GitHub Check: Build E2E Public App Assets
  • GitHub Check: Check app version bump
  • GitHub Check: i18n
  • GitHub Check: Lint
  • GitHub Check: Detect Tinybird changes
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (10)
Review new or changed service boundaries for explicit dependency ownership,

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/external-media-inliner.js
  • ghost/core/core/server/services/media-inliner/types.ts
Review whether tests prove changed behaviour, meaningful error/edge paths, and

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js
New source files must be TypeScript: flag new JS files as a required change

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.js
  • ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js
Review lens: "where does this data become trusted?"

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/types.ts
Prioritise concrete correctness, security, data-integrity, compatibility,

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/external-media-inliner.js
  • ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js
  • ghost/core/core/server/services/media-inliner/types.ts
Boot owns service initialization; do not

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/external-media-inliner.js
  • ghost/core/core/server/services/media-inliner/types.ts
Type-safe boundaries: Fail only if the PR:

📄 CodeRabbit inference engine (Custom checks)

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/types.ts
New standalone services use TypeScript; keep CommonJS only

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/types.ts
New files are TypeScript: Fail if the PR adds a new .js/.jsx/.cjs/.mjs source file, unless it is: a DB

📄 CodeRabbit inference engine (Custom checks)

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.js
  • ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js
Always use `pnpm`, never npm or Yarn.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts
  • ghost/core/core/server/services/media-inliner/external-media-inliner.js
  • ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js
  • ghost/core/core/server/services/media-inliner/types.ts
🔇 Additional comments (5)
ghost/core/core/server/services/media-inliner/types.ts (1)

1-21: LGTM!

ghost/core/core/server/services/media-inliner/external-media-inliner.js (2)

180-268: LGTM!


296-343: LGTM!

ghost/core/test/unit/server/services/media-inliner/test/external-media-inliner.test.js (1)

70-221: LGTM!

ghost/core/core/server/services/media-inliner/external-media-inliner.d.ts (1)

1-8: 📐 Maintainability & Code Quality

No change is required. TypeScript supports implements on declare class, but ghost/core/tsconfig.json sets skipLibCheck: true; the compiler skips type-checking this .d.ts, so it will not detect an ExternalMediaImporter signature mismatch there.


Walkthrough

The media inliner now exposes a typed importUrl operation. It returns structured success or failure results for download, extraction, unsupported media, and storage outcomes. inlineContent and inlineFields use this operation and preserve existing error propagation behavior. Unit tests cover successful imports, each failure stage, and legacy error handling.

Suggested reviewers: acburdine

Merge Risk: ⚪ Minimal · up to 98d65

This change adds reusable per-URL external media imports while preserving existing batch behavior; no actionable merge-blocking risk remains, so it is merge-ready after normal checks and review.

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the reusable per-URL external media import, typed result, CSV workflow, and preservation of existing batch behavior.
Title check ✅ Passed The title concisely identifies the main change: reusable external media imports.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Type-Safe Boundaries ✅ Passed PASS. The PR adds internal result types and an importUrl wrapper. It does not add any, unchecked as, @ts-nocheck, or @ts-ignore. The raw HTTP response access remains in the pre-existing `get…
New Files Are Typescript ✅ Passed The PR adds only one new source file, ghost/core/core/server/services/media-inliner/types.ts, which is TypeScript. The .js files and the .js test file are modified files that existed in the pare…
Full details: Type-Safe Boundaries

Explanation

PASS. The PR adds internal result types and an importUrl wrapper. It does not add any, unchecked as, @ts-nocheck, or @ts-ignore. The raw HTTP response access remains in the pre-existing getRemoteMedia and extractFileDataFromResponse flow; the new method only calls those internal methods. No Zod schema describes ExternalMediaImportResult, so types.ts does not duplicate a Zod-described shape.

Full details: New Files Are Typescript

Explanation

The PR adds only one new source file, ghost/core/core/server/services/media-inliner/types.ts, which is TypeScript. The .js files and the .js test file are modified files that existed in the parent revision. No new .js, .jsx, .cjs, or .mjs source file was added.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch external-media-inliner-url-api

Comment @coderabbitai help to get the list of available commands.

@nx-cloud

nx-cloud Bot commented Aug 27, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit 98d6581

Command Status Duration Result
nx run ghost:test:ci:integration ✅ Succeeded 2m 26s View ↗
nx run ghost:test:integration ✅ Succeeded 3m 34s View ↗
nx run ghost:test:legacy ✅ Succeeded 3m 7s View ↗
nx run ghost:test:e2e ✅ Succeeded 3m 2s View ↗
nx run-many -t test:unit -p ghost ✅ Succeeded 37s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded 22s View ↗
nx run-many -t lint -p ghost,ghost-monorepo ✅ Succeeded 20s View ↗
nx run-many --target=build --projects=tag:publi... ✅ Succeeded 1s View ↗
Additional runs (2) ✅ Succeeded ... View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-08-27 09:48:27 UTC

@PaulAdamDavis
PaulAdamDavis enabled auto-merge (squash) August 27, 2026 09:37
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 37.79528% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.91%. Comparing base (51d05ab) to head (568c897).

Files with missing lines Patch % Lines
...r/services/media-inliner/external-media-inliner.js 46.60% 55 Missing ⚠️
...t/core/core/server/services/media-inliner/types.ts 0.00% 21 Missing ⚠️
...services/media-inliner/external-media-inliner.d.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #30332      +/-   ##
==========================================
- Coverage   75.93%   75.91%   -0.03%     
==========================================
  Files        1670     1671       +1     
  Lines      159193   159291      +98     
  Branches    19353    19361       +8     
==========================================
+ Hits       120884   120920      +36     
- Misses      37325    37387      +62     
  Partials      984      984              
Flag Coverage Δ
e2e-tests 77.57% <37.79%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@PaulAdamDavis
PaulAdamDavis merged commit 84835b5 into main Aug 27, 2026
54 checks passed
@PaulAdamDavis
PaulAdamDavis deleted the external-media-inliner-url-api branch August 27, 2026 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant