Skip to content

🐛 Fixed duplicate requests for image dimensions of the same URL - #30383

Merged
acburdine merged 2 commits into
TryGhost:mainfrom
muratcorlu:fix/coalesce-image-size-lookups
Aug 31, 2026
Merged

🐛 Fixed duplicate requests for image dimensions of the same URL#30383
acburdine merged 2 commits into
TryGhost:mainfrom
muratcorlu:fix/coalesce-image-size-lookups

Conversation

@muratcorlu

@muratcorlu muratcorlu commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

no ref

A post's coverImage, ogImage, and twitterImage frequently resolve to the same feature_image URL, and getImageDimensions() looks all three up concurrently via Promise.all. Since ImageSize.getImageSizeFromUrl() had no in-flight deduplication, a single cold pageview could fire off multiple simultaneous origin requests (HTTP probe or storage read) for the exact same image. This causes slow page loads when imageSize cache is not filled for an external image url (including S3Storage setups).

This PR coalesces concurrent lookups for the same URL into one underlying request, regardless of which cache adapter (or none) is configured, since the dedup lives below the caching layer. The fix is applied before the internal/external url decision intentionally, so same optimization will be used to avoid re-reading same image multiple times at the same time from the storage.

Got some code for us? Awesome 🎊!

Please take a minute to explain the change you're making:

  • Why are you making it?
  • What does it do?
  • Why is this something Ghost users or developers need?

Please check your PR against these items:

  • I've read and followed the Contributor Guide
  • I've explained my change
  • I've written an automated test to prove my change works

We appreciate your contribution! 🙏

muratcorlu and others added 2 commits August 31, 2026 14:06
no ref

A post's coverImage, ogImage, and twitterImage frequently resolve to the
same feature_image URL, and getImageDimensions() looks all three up
concurrently via Promise.all. Since ImageSize.getImageSizeFromUrl() had no
in-flight deduplication, a single cold pageview could fire off multiple
simultaneous origin requests (HTTP probe or storage read) for the exact
same image. This coalesces concurrent lookups for the same URL into one
underlying request, regardless of which cache adapter (or none) is
configured, since the dedup lives below the caching layer.
@coderabbitai

coderabbitai Bot commented Aug 31, 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: 1e6f0e22-257a-484a-9bae-d360cdf73c55

📥 Commits

Reviewing files that changed from the base of the PR and between c69dd87 and db61339.

📒 Files selected for processing (2)
  • ghost/core/core/server/lib/image/image-size.js
  • ghost/core/test/unit/server/lib/image/image-size.test.js

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

📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (5)
Review whether tests prove changed behaviour, meaningful error/edge paths, and

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/test/unit/server/lib/image/image-size.test.js
New source files must be TypeScript: flag new JS files as a required change

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/test/unit/server/lib/image/image-size.test.js
  • ghost/core/core/server/lib/image/image-size.js
Prioritise concrete correctness, security, data-integrity, compatibility,

⚙️ CodeRabbit configuration file

Files:

  • ghost/core/test/unit/server/lib/image/image-size.test.js
  • ghost/core/core/server/lib/image/image-size.js
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/test/unit/server/lib/image/image-size.test.js
  • ghost/core/core/server/lib/image/image-size.js
Always use `pnpm`, never npm or Yarn.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • ghost/core/test/unit/server/lib/image/image-size.test.js
  • ghost/core/core/server/lib/image/image-size.js
🔇 Additional comments (2)
ghost/core/core/server/lib/image/image-size.js (1)

34-39: LGTM!

Also applies to: 162-179

ghost/core/test/unit/server/lib/image/image-size.test.js (1)

578-603: LGTM!

Also applies to: 605-627, 629-647, 649-677


Walkthrough

ImageSize now coalesces concurrent image-size lookups for the same URL. It tracks in-flight promises, removes entries after success or failure, and preserves fresh requests after completion. Unit tests cover same-URL sharing, separate URLs, cleanup after success, and retry behavior after failures.

Suggested reviewers: 9larsons, acburdine

Merge Risk: ⚪ Minimal · up to db613

This PR coalesces concurrent image-dimension requests for identical URLs, reducing duplicate origin or storage reads without changing the requested image data. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
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 implementation diff adds an in-flight Map and delegates getImageSizeFromUrl(imagePath) to the existing resolution logic. It does not add a new HTTP, storage, filesystem, configuration, o…
New Files Are Typescript ✅ Passed The PR changes only two pre-existing JavaScript files: ghost/core/core/server/lib/image/image-size.js and ghost/core/test/unit/server/lib/image/image-size.test.js. Both paths were tracked in the P…
Title check ✅ Passed The title clearly and concisely describes the main change: preventing duplicate image-dimension requests for the same URL.
Description check ✅ Passed The description explains the cause, implementation, scope, and automated tests for concurrent image-size request coalescing.
Full details: Type-Safe Boundaries

Explanation

PASS. The implementation diff adds an in-flight Map and delegates getImageSizeFromUrl(imagePath) to the existing resolution logic. It does not add a new HTTP, storage, filesystem, configuration, or API response read. The external response handling at _fetchImageSizeFromUrl remains pre-existing. The new imagePath use is an internal method/module call, which this check excludes from runtime validation. The changed implementation adds no any, as, @ts-nocheck, or @ts-ignore, and the added tests are excluded by the check.

Full details: New Files Are Typescript

Explanation

The PR changes only two pre-existing JavaScript files: ghost/core/core/server/lib/image/image-size.js and ghost/core/test/unit/server/lib/image/image-size.test.js. Both paths were tracked in the PR base, and the PR diff reports M for each path with no added .js/.jsx/.cjs/.mjs files. The check does not fail for modifications to pre-existing JavaScript files.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@nx-cloud

nx-cloud Bot commented Aug 31, 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 db61339

Command Status Duration Result
nx run ghost:test:ci:integration ✅ Succeeded 4m 33s View ↗
nx run ghost:test:legacy ✅ Succeeded 2m 25s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded 26s View ↗
nx run-many -t test:unit -p ghost ✅ Succeeded 40s View ↗
nx run-many -t lint -p ghost,ghost-monorepo ✅ Succeeded 23s View ↗
nx run @tryghost/admin:build ✅ Succeeded 4s View ↗
nx run-many --target=build --projects=tag:publi... ✅ Succeeded <1s View ↗
nx run @tryghost/e2e:test:fixtures ✅ Succeeded <1s View ↗

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


☁️ Nx Cloud last updated this comment at 2026-08-31 15:09:17 UTC

@acburdine acburdine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for the fix!

@acburdine
acburdine enabled auto-merge (squash) August 31, 2026 15:05
@acburdine
acburdine merged commit c55b904 into TryGhost:main Aug 31, 2026
47 checks passed
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.08%. Comparing base (15cd05b) to head (db61339).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
ghost/core/core/server/lib/image/image-size.js 87.50% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #30383   +/-   ##
=======================================
  Coverage   76.07%   76.08%           
=======================================
  Files        1679     1679           
  Lines      160133   160157   +24     
  Branches    19618    19621    +3     
=======================================
+ Hits       121829   121850   +21     
- Misses      37285    37288    +3     
  Partials     1019     1019           
Flag Coverage Δ
e2e-tests 77.74% <87.50%> (+<0.01%) ⬆️

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.

@muratcorlu
muratcorlu deleted the fix/coalesce-image-size-lookups branch August 31, 2026 15:11
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.

3 participants