-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Sitewide socials datamodel #23812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Sitewide socials datamodel #23812
Conversation
WalkthroughThis change introduces seven new site-wide settings for various social media and content platforms: "threads," "bluesky," "mastodon," "tiktok," "youtube," "instagram," and "linkedin." The migration script adds these settings atomically, initializing each as a string with an empty default value. Corresponding updates are made to the site settings schema, the public settings cache typedef, and the allowlist for public settings. Unit tests are updated to reflect the increased number of settings, including adjustments to the expected key count and the integrity hash for the default settings. Possibly related PRs
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
It looks like this PR contains a migration 👀 General requirements
Schema changes
Data changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ghost/core/test/unit/server/data/exporter/index.test.js (1)
237-241
: Hard-codingallowedKeysLength
is brittle – consider deriving itEvery new public setting requires manual edits in two places (default-settings + this magic number).
A small helper would keep the assertion but remove the maintenance hotspot:-const allowedKeysLength = 97; +const allowedKeysLength = Object.keys(defaultSettings).reduce((acc, curr) => { + return acc + Object.keys(defaultSettings[curr]).length; +}, 0) - SETTING_KEYS_BLOCKLIST.length;The comment above the check still serves as a reminder, but the test no longer fails on purely additive, non-blocking keys.
ghost/core/core/shared/settings-cache/public.js (1)
14-20
: Minor ordering / grouping nitThe newly-added social handles break the roughly alphabetical grouping of this allow-list.
Re-ordering for readability (e.g. keep all socials together:bluesky, facebook, instagram, linkedin, mastodon, threads, tiktok, twitter, youtube
) would make future diffs easier to scan.
No functional impact; feel free to skip if list ordering is considered legacy-locked.ghost/core/core/server/data/migrations/versions/5.124/2025-06-11-20-00-00-add-sitewide-data.js (1)
1-45
: Reduce duplication with an array-driven helperSeven near-identical
addSetting
calls make the migration verbose.
A compact loop is easier to scan and less error-prone:-const {combineTransactionalMigrations, addSetting} = require('../../utils'); +const {combineTransactionalMigrations, addSetting} = require('../../utils'); -module.exports = combineTransactionalMigrations( - addSetting({key: 'threads', value: '', type: 'string', group: 'site'}), - addSetting({key: 'bluesky', value: '', type: 'string', group: 'site'}), - ... - addSetting({key: 'linkedin', value: '', type: 'string', group: 'site'}) -); +const socials = ['threads', 'bluesky', 'mastodon', 'tiktok', 'youtube', 'instagram', 'linkedin']; + +module.exports = combineTransactionalMigrations( + ...socials.map(key => addSetting({key, value: '', type: 'string', group: 'site'})) +);Same behaviour, half the lines.
ghost/core/core/server/data/schema/default-settings/default-settings.json (1)
144-171
: Consider enforcing URL or handle validation
Currently these settings accept any string. To prevent misconfiguration, you might add a validation rule (e.g."isURL": true
or a regex pattern) per key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/content/__snapshots__/settings.test.js.snap
is excluded by!**/*.snap
📒 Files selected for processing (6)
ghost/core/core/server/data/migrations/versions/5.124/2025-06-11-20-00-00-add-sitewide-data.js
(1 hunks)ghost/core/core/server/data/schema/default-settings/default-settings.json
(1 hunks)ghost/core/core/shared/settings-cache/CacheManager.js
(1 hunks)ghost/core/core/shared/settings-cache/public.js
(1 hunks)ghost/core/test/unit/server/data/exporter/index.test.js
(1 hunks)ghost/core/test/unit/server/data/schema/integrity.test.js
(1 hunks)
🔇 Additional comments (3)
ghost/core/test/unit/server/data/schema/integrity.test.js (1)
38-41
: Ensure accompanying DB version bump or migration reference is present
currentSettingsHash
change reflects an altered default-settings payload (7 new socials keys).
Please double-check that:
- The
settings
table migration (2025-06-11-20-00-00-add-sitewide-data.js
) is included in the DB version map so that Ghost knows the DB is ahead of earlier builds.knex-migrator migrate
on a fresh DB produces the new hash (sanity check).If both are already covered, feel free to ignore.
ghost/core/core/shared/settings-cache/CacheManager.js (1)
25-32
: Docs in sync – nice catchTypedef updated to include all seven new socials; keeps editor intellisense happy.
ghost/core/core/server/data/schema/default-settings/default-settings.json (1)
144-171
: New site-wide social settings integrated
The seven new string-typed keys (threads
,bluesky
,mastodon
,tiktok
,youtube
,"site"
with empty defaults, matching the existing pattern for social handles.
This PR adds seven new columns to the settings data model, to hold seven new sitewide socials. I'm using the same social platforms as chosen for #22877 for consistency.
I've opted for empty strings rather than Ghost's socials for these new settings, because I've observed new Ghost users being confused by having Ghost's socials present, and not understanding how to remove unwanted social icons. It seems preferable to leave these blank and let the user discover them in due time.
Additional feature-flagged PRs to update the settings cache, update the social media entries in the admin app, and to ensure that these are available to handlebars in the @site object will follow.