Media library#27856
Draft
JohnONolan wants to merge 3 commits into
Draft
Conversation
Adds a standalone Admin media library so editors can browse, search, filter, upload, inspect, and copy URLs for Ghost-hosted assets.
Adds lightweight folder metadata so media can be grouped in Admin without changing storage paths or file URLs.
Added metadata editing, folder navigation, media viewer routing, delete safeguards, source filtering, generated-size cleanup, and usage tracking so the new Admin media library behaves like a complete standalone library instead of a simple upload index.
Contributor
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
github-actions
Bot
added
the
migration
Contributor
WalkthroughThis PR implements a complete media library feature for Ghost's admin interface. It adds a React-based media browsing and management page with image editing via Pintura, alongside a backend service that indexes media files, tracks their usage across posts and other resources, and exposes REST APIs for CRUD operations. The implementation spans the admin-x-framework (types and hooks), the admin UI (the MediaLibrary component), a new media-library service (indexing and lifecycle management), database schema and migrations (media_files, media_folders, media_file_usages tables), API endpoints with permission-based access control, and integration with post/page creation and deletion workflows to keep media usage metadata synchronized. The feature includes role-based permissions, folder organization, and comprehensive e2e testing. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
ghost/core/core/server/services/media-library/index.js (1)
1045-1060: ⚖️ Poor tradeoff
ensureBackfilledalways runsnormalizeIndexedSourcesandmarkSystemSettingMediaFileson every browse/read.These database update operations run synchronously before every media browse/read request, even after backfill is complete. While the queries are efficient (batched updates with WHERE clauses), this adds latency to every request.
Consider caching a "normalized" flag or moving these to a one-time post-backfill cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/services/media-library/index.js` around lines 1045 - 1060, ensureBackfilled currently calls normalizeIndexedSources and markSystemSettingMediaFiles on every browse/read, adding unnecessary latency; change the flow so those DB update functions only run once after a successful backfill (or when a persisted "media_normalized" flag is false) instead of on every call: move the normalizeIndexedSources() and markSystemSettingMediaFiles() calls into the backfill completion path (or guard them behind a cached boolean/persistent system setting) and set that flag to true after they run; update references in ensureBackfilled, backfill, backfillPromise, normalizeIndexedSources, and markSystemSettingMediaFiles accordingly so subsequent ensureBackfilled calls skip the work.apps/admin/src/media/media-library.tsx (1)
1326-1335: 💤 Low valueConsider error handling for sequential uploads.
If an upload fails in the middle of the batch, remaining files won't be uploaded. Consider wrapping individual uploads in try-catch to allow partial success, or collect errors for user feedback.
♻️ Suggested improvement for resilient batch uploads
const onUpload = async (event: ChangeEvent<HTMLInputElement>) => { const files = Array.from(event.target.files || []); + const errors: Error[] = []; for (const file of files) { - await upload.mutateAsync({file, folderId: activeFolderId}); + try { + await upload.mutateAsync({file, folderId: activeFolderId}); + } catch (error) { + errors.push(error instanceof Error ? error : new Error(String(error))); + } } await refetch(); event.target.value = ""; + // Optionally show toast with error count if errors.length > 0 };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/admin/src/media/media-library.tsx` around lines 1326 - 1335, The onUpload handler currently awaits upload.mutateAsync for each file sequentially, so a failure aborts the rest; change onUpload to wrap each upload.mutateAsync({file, folderId: activeFolderId}) in a try-catch inside the for loop, continue to the next file on error, collect failed file(s) or error messages in an array, call refetch() after the loop regardless, reset event.target.value = "" as now, and surface aggregated errors to the user (e.g., via a toast or setState) so partial successes are preserved and failures reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/admin-x-framework/src/api/media.ts`:
- Around line 15-17: The TypeScript types in
apps/admin-x-framework/src/api/media.ts declare resource_id and field (and
updated_at) as plain strings while the backend allows null; update the
interface/type declarations that contain resource_id, field, and updated_at to
use string | null (e.g., change resource_id: string -> resource_id: string |
null, field: string -> field: string | null, updated_at: string -> updated_at:
string | null) and ensure any exported types or consumers of that type (the
interface/type that includes created_at/resource_id/field/updated_at) reflect
the nullable signatures so callers handle null safely.
In `@apps/admin/src/layout/app-sidebar/nav-content.tsx`:
- Around line 22-25: getMediaFolderSlug currently calls decodeURIComponent on
the matched slug which can throw a URIError for malformed encodings and break
rendering; wrap the decodeURIComponent call in a try/catch and return a safe
fallback (null or the raw matched segment) on error so sidebar rendering won't
fail. Locate the getMediaFolderSlug function and replace the direct
decodeURIComponent(match[1]) usage with a guarded decode (try { return
decodeURIComponent(match[1]); } catch { return /* fallback */; }) ensuring the
match check (pathname.match(/^\/?media\/([^/]+)/)) remains unchanged.
In `@apps/posts/src/components/label-picker/label-picker.tsx`:
- Around line 329-331: The input can become unnamed when inputAriaLabel is
undefined and placeholder is set to '' after selection; update the combobox
input element in LabelPicker (the JSX that sets aria-label, placeholder, and
uses selectedLabels) to ensure it always has an accessible name by falling back
to a sensible string (e.g., placeholder or a default like "Select labels") when
inputAriaLabel is falsy; implement the fallback for aria-label (or add
aria-labelledby) so aria-label={inputAriaLabel ?? placeholder ?? 'Select
labels'} is guaranteed.
In `@ghost/core/core/server/api/endpoints/images.js`:
- Around line 61-70: The upload flow currently awaits mediaLibrary.indexUpload
after the file write (calls to mediaLibrary.indexUpload with file path built via
getUploadFolderId and frame.file), so any DB/indexing error will fail the HTTP
response despite the file being persisted; change this so indexing errors do not
cause the upload to fail — call mediaLibrary.indexUpload in a fire-and-forget
manner or wrap it in a try/catch (for both places where mediaLibrary.indexUpload
is used) and log any error via the existing logger instead of rethrowing, or
enqueue the indexing work to a background job, ensuring the HTTP response
returns success once the file write completes.
In `@ghost/core/core/server/api/endpoints/posts.js`:
- Around line 177-178: The post-write media sync calls
(mediaLibrary.syncPostResourceUsage and the post-cleanup calls) must be made
best-effort so media errors don't convert a successful DB commit into a 5xx;
wrap each call in a try/catch (for all occurrences of
mediaLibrary.syncPostResourceUsage and the cleanup call sites in the same file)
and on error log a warning/error with the caught error details (using the
existing logger) and do not re-throw; alternatively run the sync/fire-and-forget
(e.g., setImmediate/Promise.resolve()) if you must not await it, but ensure
errors are still caught and logged so the HTTP response remains successful.
In `@ghost/core/core/server/models/media-file.js`:
- Around line 24-27: The defaultRelations method currently replaces any
caller-provided options.withRelated when methodName is 'findOne'; change it to
merge instead: if options.withRelated is missing, set it to ['usages'],
otherwise normalize options.withRelated to an array (handle string or array),
push 'usages' only if not already present, and assign the de-duplicated array
back to options.withRelated so caller-requested relations are preserved; refer
to defaultRelations, methodName and options.withRelated/usages when making the
change.
In `@ghost/core/core/server/models/settings.js`:
- Around line 261-265: When a tracked setting like pintura_js_url or
pintura_css_url is updated, the code currently only calls
syncSystemMediaVisibilityForSetting for the new URL; you must also sync the
previous URL so stale media entries aren't left marked system. Update the
save/return branch that calls syncSystemMediaVisibilityForSetting(setting) to
detect the previous value (e.g. via setting.previous('value') or the model's
stored previous attributes) and, when the key is one of
pintura_js_url/pintura_css_url and the value changed, call
syncSystemMediaVisibilityForSetting for both the new setting and the previous
URL (or perform the equivalent cleanup) instead of only syncing the current
setting. Ensure this logic runs both in the saved promise branch that uses
setting.save(...) and in the immediate-return branch that calls
syncSystemMediaVisibilityForSetting(setting).
---
Nitpick comments:
In `@apps/admin/src/media/media-library.tsx`:
- Around line 1326-1335: The onUpload handler currently awaits
upload.mutateAsync for each file sequentially, so a failure aborts the rest;
change onUpload to wrap each upload.mutateAsync({file, folderId:
activeFolderId}) in a try-catch inside the for loop, continue to the next file
on error, collect failed file(s) or error messages in an array, call refetch()
after the loop regardless, reset event.target.value = "" as now, and surface
aggregated errors to the user (e.g., via a toast or setState) so partial
successes are preserved and failures reported.
In `@ghost/core/core/server/services/media-library/index.js`:
- Around line 1045-1060: ensureBackfilled currently calls
normalizeIndexedSources and markSystemSettingMediaFiles on every browse/read,
adding unnecessary latency; change the flow so those DB update functions only
run once after a successful backfill (or when a persisted "media_normalized"
flag is false) instead of on every call: move the normalizeIndexedSources() and
markSystemSettingMediaFiles() calls into the backfill completion path (or guard
them behind a cached boolean/persistent system setting) and set that flag to
true after they run; update references in ensureBackfilled, backfill,
backfillPromise, normalizeIndexedSources, and markSystemSettingMediaFiles
accordingly so subsequent ensureBackfilled calls skip the work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4244954-c87c-46b0-a401-2da7d17ecc26
📒 Files selected for processing (42)
apps/admin-x-framework/src/api/media.tsapps/admin-x-framework/src/api/users.tsapps/admin-x-framework/test/unit/api/media.test.tsxapps/admin/src/hooks/user-preferences.test.tsxapps/admin/src/hooks/user-preferences.tsapps/admin/src/layout/app-sidebar/nav-content.test.tsxapps/admin/src/layout/app-sidebar/nav-content.tsxapps/admin/src/media/media-library.test.tsxapps/admin/src/media/media-library.tsxapps/admin/src/routes.tsxapps/posts/package.jsonapps/posts/src/components/label-picker/label-picker.tsxghost/admin/app/services/state-bridge.jsghost/admin/package.jsonghost/admin/tests/unit/services/state-bridge-test.jsghost/core/core/server/api/endpoints/files.jsghost/core/core/server/api/endpoints/images.jsghost/core/core/server/api/endpoints/media.jsghost/core/core/server/api/endpoints/pages.jsghost/core/core/server/api/endpoints/posts.jsghost/core/core/server/api/endpoints/utils/serializers/output/media.jsghost/core/core/server/data/exporter/table-lists.jsghost/core/core/server/data/migrations/versions/6.38/2026-05-04-04-14-16-add-media-library-tables.jsghost/core/core/server/data/migrations/versions/6.38/2026-05-04-18-49-06-add-media-library-folders.jsghost/core/core/server/data/migrations/versions/6.38/2026-05-08-17-39-54-add-media-file-metadata.jsghost/core/core/server/data/migrations/versions/6.38/2026-05-11-02-43-32-add-media-file-visibility.jsghost/core/core/server/data/schema/fixtures/fixtures.jsonghost/core/core/server/data/schema/schema.jsghost/core/core/server/models/index.jsghost/core/core/server/models/media-file-usage.jsghost/core/core/server/models/media-file.jsghost/core/core/server/models/media-folder.jsghost/core/core/server/models/settings.jsghost/core/core/server/services/media-library/index.jsghost/core/core/server/web/api/endpoints/admin/middleware.jsghost/core/core/server/web/api/endpoints/admin/routes.jsghost/core/package.jsonghost/core/test/e2e-api/admin/media-library.test.jsghost/core/test/integration/exporter/exporter.test.jsghost/core/test/integration/migrations/migration.test.jsghost/core/test/unit/server/data/schema/integrity.test.jsghost/core/test/utils/fixtures/fixtures.json
Comment on lines
+15
to
+17
| resource_id: string; | ||
| field: string; | ||
| created_at: string; |
Contributor
There was a problem hiding this comment.
Align nullable API fields with backend schema in TypeScript types
Line 15, Line 16, and Line 48 are typed as always-present strings, but backend schema allows null for resource_id, field, and updated_at. This can cause runtime null handling bugs in consumers.
Proposed type fix
export interface MediaFileUsage {
id: string;
media_file_id: string;
resource_type: string;
- resource_id: string;
- field: string;
+ resource_id: string | null;
+ field: string | null;
created_at: string;
@@
export interface MediaFile {
@@
- updated_at: string;
+ updated_at: string | null;
usages?: MediaFileUsage[];
}Also applies to: 47-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/admin-x-framework/src/api/media.ts` around lines 15 - 17, The TypeScript
types in apps/admin-x-framework/src/api/media.ts declare resource_id and field
(and updated_at) as plain strings while the backend allows null; update the
interface/type declarations that contain resource_id, field, and updated_at to
use string | null (e.g., change resource_id: string -> resource_id: string |
null, field: string -> field: string | null, updated_at: string -> updated_at:
string | null) and ensure any exported types or consumers of that type (the
interface/type that includes created_at/resource_id/field/updated_at) reflect
the nullable signatures so callers handle null safely.
Comment on lines
+22
to
+25
| const getMediaFolderSlug = (pathname: string) => { | ||
| const match = pathname.match(/^\/?media\/([^/]+)/); | ||
| return match ? decodeURIComponent(match[1]) : null; | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard against malformed URL encoding in folder slug parsing.
Line 24 can throw a URIError for malformed path segments and break sidebar rendering. Add a safe decode fallback.
Suggested hardening
const getMediaFolderSlug = (pathname: string) => {
const match = pathname.match(/^\/?media\/([^/]+)/);
- return match ? decodeURIComponent(match[1]) : null;
+ if (!match) {
+ return null;
+ }
+
+ try {
+ return decodeURIComponent(match[1]);
+ } catch {
+ return match[1];
+ }
};📝 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
| const getMediaFolderSlug = (pathname: string) => { | |
| const match = pathname.match(/^\/?media\/([^/]+)/); | |
| return match ? decodeURIComponent(match[1]) : null; | |
| }; | |
| const getMediaFolderSlug = (pathname: string) => { | |
| const match = pathname.match(/^\/?media\/([^/]+)/); | |
| if (!match) { | |
| return null; | |
| } | |
| try { | |
| return decodeURIComponent(match[1]); | |
| } catch { | |
| return match[1]; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/admin/src/layout/app-sidebar/nav-content.tsx` around lines 22 - 25,
getMediaFolderSlug currently calls decodeURIComponent on the matched slug which
can throw a URIError for malformed encodings and break rendering; wrap the
decodeURIComponent call in a try/catch and return a safe fallback (null or the
raw matched segment) on error so sidebar rendering won't fail. Locate the
getMediaFolderSlug function and replace the direct decodeURIComponent(match[1])
usage with a guarded decode (try { return decodeURIComponent(match[1]); } catch
{ return /* fallback */; }) ensuring the match check
(pathname.match(/^\/?media\/([^/]+)/)) remains unchanged.
Comment on lines
+329
to
+331
| aria-label={inputAriaLabel} | ||
| className="min-w-[80px] flex-1 bg-transparent text-sm outline-hidden placeholder:text-muted-foreground" | ||
| placeholder={selectedLabels.length === 0 ? 'Search labels...' : ''} | ||
| placeholder={selectedLabels.length === 0 ? placeholder : ''} |
Contributor
There was a problem hiding this comment.
Add a fallback accessible name for the combobox input.
On Line 329, aria-label is optional. On Line 331, placeholder becomes '' once labels are selected. In that state, the input can end up without an accessible name.
Proposed fix
- <input
+ <input
ref={inputRef}
- aria-label={inputAriaLabel}
+ aria-label={inputAriaLabel ?? placeholder}
className="min-w-[80px] flex-1 bg-transparent text-sm outline-hidden placeholder:text-muted-foreground"
placeholder={selectedLabels.length === 0 ? placeholder : ''}📝 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
| aria-label={inputAriaLabel} | |
| className="min-w-[80px] flex-1 bg-transparent text-sm outline-hidden placeholder:text-muted-foreground" | |
| placeholder={selectedLabels.length === 0 ? 'Search labels...' : ''} | |
| placeholder={selectedLabels.length === 0 ? placeholder : ''} | |
| aria-label={inputAriaLabel ?? placeholder} | |
| className="min-w-[80px] flex-1 bg-transparent text-sm outline-hidden placeholder:text-muted-foreground" | |
| placeholder={selectedLabels.length === 0 ? placeholder : ''} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/posts/src/components/label-picker/label-picker.tsx` around lines 329 -
331, The input can become unnamed when inputAriaLabel is undefined and
placeholder is set to '' after selection; update the combobox input element in
LabelPicker (the JSX that sets aria-label, placeholder, and uses selectedLabels)
to ensure it always has an accessible name by falling back to a sensible string
(e.g., placeholder or a default like "Select labels") when inputAriaLabel is
falsy; implement the fallback for aria-label (or add aria-labelledby) so
aria-label={inputAriaLabel ?? placeholder ?? 'Select labels'} is guaranteed.
Comment on lines
+61
to
+70
| await mediaLibrary.indexUpload({ | ||
| url: processedImageUrl, | ||
| storageType: 'images', | ||
| file: { | ||
| ...frame.file, | ||
| path: out | ||
| }, | ||
| createdBy: frame.options.context?.user, | ||
| folderId: getUploadFolderId(frame) | ||
| }); |
Contributor
There was a problem hiding this comment.
Upload can return failure after the file is already saved if indexing errors.
On Line 61 and Line 97 branches, indexUpload is awaited post-save. Any DB/indexing error causes a failed response despite persisted storage write, which can lead to retries and duplicate/orphaned files.
Suggested mitigation (do not fail upload response on indexing error)
+const logging = require('@tryghost/logging');
const path = require('path');
const errors = require('@tryghost/errors');
@@
- await mediaLibrary.indexUpload({
- url: processedImageUrl,
- storageType: 'images',
- file: {
- ...frame.file,
- path: out
- },
- createdBy: frame.options.context?.user,
- folderId: getUploadFolderId(frame)
- });
+ try {
+ await mediaLibrary.indexUpload({
+ url: processedImageUrl,
+ storageType: 'images',
+ file: {
+ ...frame.file,
+ path: out
+ },
+ createdBy: frame.options.context?.user,
+ folderId: getUploadFolderId(frame)
+ });
+ } catch (err) {
+ logging.error(err, 'Failed to index uploaded image in media library');
+ }
@@
- await mediaLibrary.indexUpload({
- url: imageUrl,
- storageType: 'images',
- file: frame.file,
- createdBy: frame.options.context?.user,
- folderId: getUploadFolderId(frame)
- });
+ try {
+ await mediaLibrary.indexUpload({
+ url: imageUrl,
+ storageType: 'images',
+ file: frame.file,
+ createdBy: frame.options.context?.user,
+ folderId: getUploadFolderId(frame)
+ });
+ } catch (err) {
+ logging.error(err, 'Failed to index uploaded image in media library');
+ }Also applies to: 97-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/api/endpoints/images.js` around lines 61 - 70, The
upload flow currently awaits mediaLibrary.indexUpload after the file write
(calls to mediaLibrary.indexUpload with file path built via getUploadFolderId
and frame.file), so any DB/indexing error will fail the HTTP response despite
the file being persisted; change this so indexing errors do not cause the upload
to fail — call mediaLibrary.indexUpload in a fire-and-forget manner or wrap it
in a try/catch (for both places where mediaLibrary.indexUpload is used) and log
any error via the existing logger instead of rethrowing, or enqueue the indexing
work to a background job, ensuring the HTTP response returns success once the
file write completes.
Comment on lines
+177
to
+178
| await mediaLibrary.syncPostResourceUsage(model); | ||
|
|
Contributor
There was a problem hiding this comment.
Post mutations can fail after commit due to post-write media sync calls.
On Line 177, Line 233, Line 284, and Line 313, a sync/cleanup error will throw after the primary post mutation already succeeded, causing 5xx responses with partial completion semantics.
Suggested mitigation (best-effort metadata sync)
+const logging = require('@tryghost/logging');
const urlUtils = require('../../../shared/url-utils');
const models = require('../../models');
const mediaLibrary = require('../../services/media-library');
@@
async query(frame) {
const model = await models.Post.add(frame.data.posts[0], frame.options);
- await mediaLibrary.syncPostResourceUsage(model);
+ try {
+ await mediaLibrary.syncPostResourceUsage(model);
+ } catch (err) {
+ logging.error(err, 'Failed to sync media usage for added post');
+ }
@@
- await mediaLibrary.syncPostResourceUsage(model);
+ try {
+ await mediaLibrary.syncPostResourceUsage(model);
+ } catch (err) {
+ logging.error(err, 'Failed to sync media usage for edited post');
+ }
@@
const result = await postsService.bulkDestroy(frame.options);
- await mediaLibrary.clearResourceUsages('post', result.deleteIds);
+ try {
+ await mediaLibrary.clearResourceUsages('post', result.deleteIds);
+ } catch (err) {
+ logging.error(err, 'Failed to clear media usages for bulk-destroyed posts');
+ }
return result;
@@
const model = await models.Post.destroy({...frame.options, require: true});
- await mediaLibrary.clearResourceUsages('post', frame.options.id);
+ try {
+ await mediaLibrary.clearResourceUsages('post', frame.options.id);
+ } catch (err) {
+ logging.error(err, 'Failed to clear media usages for destroyed post');
+ }
return model;Also applies to: 233-234, 283-285, 311-314
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/api/endpoints/posts.js` around lines 177 - 178, The
post-write media sync calls (mediaLibrary.syncPostResourceUsage and the
post-cleanup calls) must be made best-effort so media errors don't convert a
successful DB commit into a 5xx; wrap each call in a try/catch (for all
occurrences of mediaLibrary.syncPostResourceUsage and the cleanup call sites in
the same file) and on error log a warning/error with the caught error details
(using the existing logger) and do not re-throw; alternatively run the
sync/fire-and-forget (e.g., setImmediate/Promise.resolve()) if you must not
await it, but ensure errors are still caught and logged so the HTTP response
remains successful.
Comment on lines
+24
to
+27
| defaultRelations(methodName, options) { | ||
| if (['findOne'].includes(methodName)) { | ||
| options.withRelated = ['usages']; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not overwrite caller-provided withRelated in defaultRelations
Line 26 replaces options.withRelated entirely, so any explicit relations requested by callers are dropped on findOne.
Proposed merge-safe fix
defaultRelations(methodName, options) {
if (['findOne'].includes(methodName)) {
- options.withRelated = ['usages'];
+ const existing = options.withRelated || [];
+ options.withRelated = [...new Set([...existing, 'usages'])];
}
return options;
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/models/media-file.js` around lines 24 - 27, The
defaultRelations method currently replaces any caller-provided
options.withRelated when methodName is 'findOne'; change it to merge instead: if
options.withRelated is missing, set it to ['usages'], otherwise normalize
options.withRelated to an array (handle string or array), push 'usages' only if
not already present, and assign the de-duplicated array back to
options.withRelated so caller-requested relations are preserved; refer to
defaultRelations, methodName and options.withRelated/usages when making the
change.
Comment on lines
+261
to
265
| return setting.save(null, options).then(syncSystemMediaVisibilityForSetting); | ||
| } | ||
|
|
||
| return setting; | ||
| return syncSystemMediaVisibilityForSetting(setting); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle previous system-media URL when a tracked setting value changes
Line 261 and Line 264 only sync the current URL. If pintura_js_url/pintura_css_url changes, the old URL can remain marked as system, which can stale the media visibility index.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ghost/core/core/server/models/settings.js` around lines 261 - 265, When a
tracked setting like pintura_js_url or pintura_css_url is updated, the code
currently only calls syncSystemMediaVisibilityForSetting for the new URL; you
must also sync the previous URL so stale media entries aren't left marked
system. Update the save/return branch that calls
syncSystemMediaVisibilityForSetting(setting) to detect the previous value (e.g.
via setting.previous('value') or the model's stored previous attributes) and,
when the key is one of pintura_js_url/pintura_css_url and the value changed,
call syncSystemMediaVisibilityForSetting for both the new setting and the
previous URL (or perform the equivalent cleanup) instead of only syncing the
current setting. Ensure this logic runs both in the saved promise branch that
uses setting.save(...) and in the immediate-return branch that calls
syncSystemMediaVisibilityForSetting(setting).
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 / Draft
Contains some significant changes that require scrutiny
medialib-small.mp4