🎨 improve handling of comments for themes with darkmode toggles #22407
Conversation
WalkthroughThe pull request modifies the Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-activitypub".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-activitypub/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/admin-x-activitypub/src/api/activitypub.tsOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-activitypub".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-activitypub/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/admin-x-activitypub/src/components/feed/ArticleModal.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-activitypub".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-activitypub/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/comments-ui/src/components/ContentBox.tsx (1)
62-62: Consider memoizing the darkMode function.Good conversion of the static containerClass to a state variable. Since
darkMode()is a relatively complex calculation involving DOM access and style computation, consider memoizing this function or extracting the logic to a custom hook if it's used elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/comments-ui/src/components/ContentBox.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/comments-ui/src/components/ContentBox.tsx
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Comments-UI tests
🔇 Additional comments (2)
apps/comments-ui/src/components/ContentBox.tsx (2)
3-3: Correctly updated imports for React hooks.The addition of
useEffectanduseStateimports is appropriate for implementing the dynamic dark mode functionality.
49-62: The implementation aligns well with the PR objectives.The mutation observer implementation successfully addresses the PR objective of improving handling of comments for themes with darkmode toggles. By monitoring changes to the parent element's style and class attributes, the component can now dynamically adjust its appearance when theme changes occur.
This is a good localized approach as mentioned in the PR objectives, avoiding monitoring broader elements like body or html which could cause conflicts with unrelated changes.
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (1)
871-892:⚠️ Potential issueAdd a key prop to the DeletedFeedItem component
React elements in an iterable require a unique key prop for proper reconciliation and performance.
item.object.type === 'Tombstone' ? ( - <DeletedFeedItem last={false} /> + <DeletedFeedItem key={item.id} last={false} /> ) : (🧰 Tools
🪛 Biome (1.9.4)
[error] 872-872: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 874-890: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🧹 Nitpick comments (68)
ghost/admin/app/templates/stats.hbs (1)
5-12: Added toggle button for switching between test and live dataA new button has been implemented that allows users to toggle between test and live data. The button's appearance changes based on the current state (green when using test data) and its text updates accordingly.
The implementation is functional, but consider adding accessibility improvements:
<button type="button" class="gh-btn {{if this.mockData "gh-btn-green"}}" data-test-button="toggle-data-mode" + aria-pressed="{{this.mockData}}" + aria-label="{{if this.mockData "Switch to live data" "Switch to test data"}}" {{on "click" this.toggleMockData}} > <span>{{if this.mockData "Using Test Data" "Using Live Data"}}</span> </button>ghost/i18n/locales/nl/ghost.json (2)
30-30: Translation Update: Improved Passive ConstructionThe phrase now reads "Je zult niet worden geregistreerd en er zal geen account voor je worden aangemaakt." This update enhances the grammatical structure. Consider reviewing if the use of "Je" versus "Als jij" (used in earlier messages) should be standardized for consistency.
31-31: Translation Update: Consistent Future Tense"Je zult niet worden geabonneerd." is now correctly phrased. For overall consistency, you might want to align the pronoun style across similar messages.
ghost/core/core/frontend/src/cards/css/cta.css (4)
45-48: Adjust Sponsor Label Wrapper Spacing
The.kg-cta-sponsor-label-wrapperclass now usesmargin: 0 1.5em;andpadding: .7em 0;. These changes help standardize spacing with an em-based scale. Please ensure the updated spacing aligns with the overall design system and visual expectations.
63-65: Conditional Styling for Image-Containing CTAs
The complex selector targeting
.kg-cta-has-img .kg-cta-sponsor-label-wrapper:not(.kg-cta-bg-none .kg-cta-sponsor-label-wrapper):not(.kg-cta-minimal .kg-cta-sponsor-label-wrapper)
removes the bottom border (border-bottom: 0;) when the CTA includes an image. While the specificity is high, consider adding a comment or documentation to clarify the rationale for future maintainers.
67-73: Define Sponsor Label Typography
The newly introduced.kg-cta-sponsor-labelclass clearly defines typography attributes such as font family, size, weight, and text transformation. One point to review: the use oftext-wrap: pretty;is non-standard. Please verify cross-browser support or intended behavior.
183-187: Refine CTA Button Styling for Improved Usability
In thea.kg-cta-buttonrule, the updatedpadding: 0 1em;,height: 2.5em;, andfont-size: 0.95em;contribute to a more streamlined and visually balanced button appearance. Check that these adjustments meet both the aesthetic criteria and accessibility standards.ghost/web-analytics/entrypoint.sh (1)
1-24: Good developer experience enhancement for Tinybird integration.This shell script improves the developer experience by showing Tinybird branch information in the command prompt. The implementation correctly handles different region formats and gracefully handles the absence of a
.tinybfile.A couple of suggestions for robustness:
- Consider adding error handling for the grep commands to suppress errors if patterns aren't found
- It might be helpful to add a comment explaining the Unicode character being used (chick emoji)
#!/bin/bash # Function to prompt Tinybird branch information prompt_tb() { if [ -e ".tinyb" ]; then - TB_CHAR=$'\U1F423' + TB_CHAR=$'\U1F423' # Chick emoji 🐣 - branch_name=$(grep '"name":' .tinyb | cut -d : -f 2 | cut -d '"' -f 2) - region=$(grep '"host":' .tinyb | cut -d / -f 3 | cut -d . -f 2 | cut -d : -f 1) + branch_name=$(grep '"name":' .tinyb 2>/dev/null | cut -d : -f 2 | cut -d '"' -f 2) + region=$(grep '"host":' .tinyb 2>/dev/null | cut -d / -f 3 | cut -d . -f 2 | cut -d : -f 1) if [ "$region" = "tinybird" ]; then - region=$(grep '"host":' .tinyb | cut -d / -f 3 | cut -d . -f 1) + region=$(grep '"host":' .tinyb 2>/dev/null | cut -d / -f 3 | cut -d . -f 1) fi TB_BRANCH="${TB_CHAR}tb:${region}=>${branch_name}" else TB_BRANCH='' fi echo $TB_BRANCH } # Export the prompt with Tinybird branch information export PS1="\w\$(prompt_tb)\$ " exec "$@"ghost/i18n/locales/ja/newsletter.json (1)
2-3: Missing translations for Japanese localeNew localization keys have been added for premium content prompts and upgrade actions, but all have empty translations. These strings will fall back to English for Japanese users until translations are provided.
Consider working with a translator to provide Japanese translations for these new strings before deploying to production, particularly if these features will be immediately available to Japanese users.
Also applies to: 19-20
ghost/i18n/locales/sr-Cyrl/newsletter.json (1)
19-20: Upgrade Keys Pending TranslationThe keys
"Upgrade"and"Upgrade to continue reading."have been added with empty values. This is consistent with localization efforts across other languages, but please confirm with the localization team that these placeholders suffice until proper translations are submitted.ghost/i18n/locales/et/newsletter.json (1)
19-20: Upgrade Keys Added with Empty TranslationsThe new keys
"Upgrade"and"Upgrade to continue reading."are correctly added. It would be beneficial to track these keys for future updates once the translations are available.ghost/i18n/locales/el/newsletter.json (1)
19-20: Upgrade Prompt Keys Without TranslationsThe keys
"Upgrade"and"Upgrade to continue reading."are added correctly but remain empty. Confirm if these are placeholders pending proper translation, and consider adding a comment or TODO for clarity.ghost/i18n/locales/id/newsletter.json (1)
19-20: Empty Upgrade Keys Added ConsistentlyThe additions of
"Upgrade"and"Upgrade to continue reading."follow the standardized approach seen in other languages. Ensure these are tracked for translation in future updates.ghost/i18n/locales/sq/newsletter.json (1)
19-20: Upgrade Message Keys Added and Marked for TranslationThe inclusion of
"Upgrade"and"Upgrade to continue reading."as keys is consistent with similar changes in other locales. These placeholders should be revisited and updated with proper translations when available.ghost/core/core/server/data/schema/default-settings/default-settings.json (1)
607-612: New security section is properly structured.The security section with email MFA settings is well-implemented and follows the standard format for boolean settings. Note that this setting uses a boolean type with
false(without quotes), which is different from the string-boolean pattern used in the captcha setting ("false"). This is inconsistent with other boolean settings in the file but may be intentional.Consider using the same format for boolean values throughout the file for consistency:
- "defaultValue": false, + "defaultValue": "false", + "validations": { + "isEmpty": false, + "isIn": [["true", "false"]] + },ghost/i18n/locales/he/newsletter.json (2)
2-3: Localization Key Addition: Date Placeholder & Membership Prompt
The keys"{date}"and"Become a paid member of {site} to get access to all premium content."are newly introduced. Please confirm that the empty translation for the membership prompt is intentional and will be updated later if needed.
19-20: Localization Key Addition: Upgrade Prompts
The keys"Upgrade"and"Upgrade to continue reading."have been added with empty string values. Verify that these placeholders suit your localization strategy and that proper translations will be provided at a later stage.ghost/i18n/locales/fi/newsletter.json (2)
2-3: Localization Key Addition: Date Placeholder & Membership Prompt
New keys"{date}"and"Become a paid member of {site} to get access to all premium content."are added. Please ensure the empty string for the membership message is acceptable as a placeholder until the translation becomes available.
19-20: Localization Key Addition: Upgrade Prompts
The addition of"Upgrade"and"Upgrade to continue reading."with empty values should be verified with your translation team to ensure these keys are updated when required.ghost/i18n/locales/bs/newsletter.json (2)
2-3: Localization Key Addition: Date Placeholder & Membership Prompt
The keys"{date}"and"Become a paid member of {site} to get access to all premium content."have been introduced. Please confirm that using an empty string for the membership prompt is intentional as a placeholder for a later translation.
19-20: Localization Key Addition: Upgrade Prompts
New keys"Upgrade"and"Upgrade to continue reading."now appear with empty values. It is advisable to ensure that these entries are either updated with proper translations or have fallback text if that fits the localization strategy.ghost/i18n/locales/ms/newsletter.json (2)
2-3: Localization Key Addition: Date Placeholder & Membership Prompt
The keys"{date}"and"Become a paid member of {site} to get access to all premium content."are added with the expected placeholders; please verify that the empty translation is intended pending future localization efforts.
19-20: Localization Key Addition: Upgrade Prompts
The keys"Upgrade"and"Upgrade to continue reading."are introduced with empty values. Confirm that these placeholders are acceptable or update them with default texts if necessary before final release.ghost/i18n/locales/uk/newsletter.json (2)
2-3: Localization Key Addition: Date Placeholder & Membership Prompt
Newly added keys"{date}"and"Become a paid member of {site} to get access to all premium content."require attention. Ensure that the use of empty translations is acceptable and that appropriate translations will be provided for the Ukrainian locale.
19-20: Localization Key Addition: Upgrade Prompts
The keys"Upgrade"and"Upgrade to continue reading."now exist with empty values. Please verify that these placeholders align with your localization workflow, and update them with the correct translations as needed.ghost/i18n/locales/lv/newsletter.json (2)
2-3: Add Premium Content Keys
The new keys"{date}"and
"Become a paid member of {site} to get access to all premium content."have been added with placeholder translations. Please ensure these placeholders are later replaced with proper translations when available.
19-20: Pending Translation for Upgrade Prompts
The keys"Upgrade"and"Upgrade to continue reading."currently have empty values. Ensure that appropriate translations are provided to maintain consistency across locales.ghost/i18n/locales/lt/newsletter.json (2)
2-3: Introduce Premium Content Localization Keys
New entries for"{date}"and
"Become a paid member of {site} to get access to all premium content."are introduced with empty placeholders. Verify that these will be updated with finalized translations for Lithuanian as soon as they become available.
19-20: Empty Upgrade Prompt Translations
Keys"Upgrade"and"Upgrade to continue reading."are added with empty strings. Make sure to fill in the appropriate Lithuanian translations to complete the localization update.ghost/i18n/locales/hr/newsletter.json (2)
2-3: Premium Content Keys Added for Croatian Locale
The new keys for"{date}"and
"Become a paid member of {site} to get access to all premium content."have been added as placeholders. Confirm that accurate Croatian translations will be provided when ready.
19-20: Awaiting Translation for Upgrade Prompts
The"Upgrade"and"Upgrade to continue reading."keys are present with empty values. Ensure these are updated with proper Croatian translations to deliver a consistent user experience.ghost/i18n/locales/bg/newsletter.json (2)
2-3: Enhance Bulgarian Localization with New Premium Content Keys
New entries for the date placeholder and the premium content message have been added. The key
"Become a paid member of {site} to get access to all premium content."currently has an empty value; please update it with an appropriate Bulgarian translation when available.
19-20: Translation Pending for Upgrade Keys in Bulgarian
The keys"Upgrade"and"Upgrade to continue reading."remain with empty values. It is important to provide the correct translations to maintain consistency in the Bulgarian locale.ghost/i18n/locales/ar/newsletter.json (2)
2-3: Localization Update: Premium Content Keys for Arabic
New keys for"{date}"and
"Become a paid member of {site} to get access to all premium content."have been added with placeholder values. Confirm that appropriate Arabic translations are supplied for these keys when available.
19-20: Pending Arabic Translation for Upgrade Keys
The keys"Upgrade"and"Upgrade to continue reading."currently have empty translations. Please update these with the accurate Arabic text to ensure complete localization.ghost/i18n/locales/pt-BR/newsletter.json (1)
19-20: Missing translations for upgrade promptsTwo new upgrade-related strings have been added but currently have empty translations. These strings are likely important for the user experience when prompting users to upgrade their membership.
Consider providing translations for these strings:
- "Upgrade": "", - "Upgrade to continue reading.": "", + "Upgrade": "Atualizar", + "Upgrade to continue reading.": "Atualize para continuar lendo.",ghost/i18n/locales/fr/newsletter.json (2)
2-3: Date placeholder added and missing membership translationThe date placeholder is correctly preserved as
"{date}"but the new membership string lacks a French translation. For consistency with the rest of the file, a translation should be provided.Consider adding a translation:
- "Become a paid member of {site} to get access to all premium content.": "", + "Become a paid member of {site} to get access to all premium content.": "Devenez membre payant de {site} pour accéder à tout le contenu premium.",
19-20: Missing translations for upgrade promptsThe upgrade-related strings have been added but remain untranslated. These strings are important for the user experience when navigating premium content.
Consider adding translations:
- "Upgrade": "", - "Upgrade to continue reading.": "", + "Upgrade": "Mettre à niveau", + "Upgrade to continue reading.": "Mettez à niveau pour continuer la lecture.",ghost/i18n/locales/ko/newsletter.json (2)
3-3: Pending Translation for Membership PromptThe key
"Become a paid member of {site} to get access to all premium content."is added with an empty string. Please confirm whether this is intentional (e.g. awaiting translation) or if a default Korean translation should be provided.
19-20: Pending Translation for Upgrade PromptsBoth
"Upgrade"and"Upgrade to continue reading."are currently empty. Ensure that proper Korean translations will be supplied to avoid untranslated UI elements.ghost/i18n/locales/zh-Hant/newsletter.json (2)
3-3: Pending Translation for Membership PromptThe new key
"Become a paid member of {site} to get access to all premium content."has an empty translation. Please verify if this is planned for a future update or if a temporary translation should be provided.
19-20: Pending Upgrade TranslationThe keys
"Upgrade"and"Upgrade to continue reading."are inserted with empty values. Consider adding translations or clear placeholder text to ensure a seamless user experience.ghost/i18n/locales/gd/newsletter.json (2)
3-3: Pending Membership Prompt TranslationThe membership prompt key is added with an empty value. Confirm if the translation is pending or if a default should be provided.
19-20: Incomplete Upgrade TranslationsBoth
"Upgrade"and"Upgrade to continue reading."have empty translations. Please ensure that these are updated to provide a complete localized experience.ghost/i18n/locales/ne/newsletter.json (2)
3-3: Pending Membership Prompt TranslationThe key
"Become a paid member of {site} to get access to all premium content."currently has an empty translation—please verify if this is intentional or requires an immediate translation update.
19-20: Upgrade Prompt Translations NeededBoth
"Upgrade"and"Upgrade to continue reading."are left empty. Ensure that these keys are later populated to avoid missing text in the user interface.ghost/i18n/locales/eo/newsletter.json (2)
3-3: Pending Membership Prompt TranslationThe key
"Become a paid member of {site} to get access to all premium content."is added with an empty value. Please confirm if this is pending translation for Esperanto.
19-20: Missing Upgrade TranslationsBoth
"Upgrade"and"Upgrade to continue reading."have empty strings. It is recommended to update these with proper translations to ensure full localization.ghost/i18n/locales/ta/newsletter.json (2)
2-3: New Localization Keys for Date and Membership PromptThe new keys
"{date}"with value"{date}"and
"Become a paid member of {site} to get access to all premium content."with an empty value have been added.
Please verify that the placeholder for the second key is intentional and that a translation or default text will be provided later if needed.
19-20: Upgrade Message Keys with Pending TranslationsThe keys
"Upgrade"and"Upgrade to continue reading."were introduced with empty string values. Confirm that leaving these values empty is acceptable as placeholders for future translations in Tamil.ghost/i18n/locales/is/newsletter.json (2)
2-3: Addition of Date and Membership Prompt KeysThe keys
"{date}"and"Become a paid member of {site} to get access to all premium content."have been added. Ensure that the empty translation for the membership prompt is expected and to be updated when translations are available.
19-20: Upgrade Key PlaceholdersNew upgrade-related keys (
"Upgrade"and"Upgrade to continue reading.") have empty values. Please confirm that this is by design and that appropriate translations will be supplied in due course.ghost/i18n/locales/ru/newsletter.json (2)
2-3: New Localization Entries for RussianThe introduction of the
"{date}"key and the membership prompt key with an empty value is noted. Verify that the empty string for the membership prompt is acceptable for Russian or if it requires a prompt translation update.
19-20: Review Pending Translations for Upgrade KeysThe keys
"Upgrade"and"Upgrade to continue reading.", added with empty strings, should be revisited to ensure that either a default or translated value is provided later.ghost/i18n/locales/sk/newsletter.json (2)
2-3: Introduction of New Date and Membership Prompt Keys in SlovakThe file now includes the keys
"{date}"and"Become a paid member of {site} to get access to all premium content."(with an empty translation). Confirm that the empty value is intended as a placeholder pending translation.
19-20: Empty Upgrade Message EntriesThe upgrade-related keys
"Upgrade"and"Upgrade to continue reading."are added but left empty. Please verify whether this omission is acceptable for Slovak or if translations are anticipated in a forthcoming update.ghost/i18n/locales/da/newsletter.json (2)
2-3: Date and Membership Prompt Keys Added for DanishThe keys
"{date}"and"Become a paid member of {site} to get access to all premium content."have been introduced. Make sure that the empty value for the membership prompt is intended and will be updated when a Danish translation is available.
19-20: Upgrade Keys Awaiting TranslationThe empty strings for
"Upgrade"and"Upgrade to continue reading."indicate missing translations for these messages. Confirm if these are placeholders pending translation or if a default value should be provided.ghost/i18n/locales/en/newsletter.json (1)
2-3: Empty translation values for English localeWhile adding new keys with empty values is common practice for non-English locales, it's unusual to have empty values in the English locale since it's typically the base language. Consider adding the actual English strings here instead of empty values.
Also applies to: 19-20
apps/admin-x-settings/src/components/settings/general/Users.tsx (1)
301-327: New security settings UI implemented correctlyThe implementation properly uses a feature flag (
labs.staff2fa) to conditionally render the 2FA security settings. The toggle component is correctly bound to therequire2fastate and includes proper error handling in the onChange handler.One small improvement could be to add a loading state to the toggle while the setting is being updated, to provide better feedback to the user.
<Toggle checked={require2fa} direction='rtl' gap='gap-0' + disabled={isUpdating} onChange={async () => { const newValue = !require2fa; + let isUpdating = true; try { await editSettings([{ key: 'require_email_mfa', value: newValue }]); } catch (error) { handleError(error); + } finally { + isUpdating = false; } }} />apps/admin-x-settings/src/components/settings/advanced/SpamFilters.tsx (1)
28-35: Added captcha settings state and handler.The state variable for captcha settings is initialized correctly. Consider using a more specific naming for the toggle handler if it's only used for captcha.
-const handleToggleChange = (key: string, e: React.ChangeEvent<HTMLInputElement>) => { +const handleCaptchaToggleChange = (e: React.ChangeEvent<HTMLInputElement>) => { - updateSetting(key, e.target.checked); + updateSetting('captcha_enabled', e.target.checked); handleEditingChange(true); };apps/admin-x-activitypub/src/lib/feature-flags.tsx (1)
8-8: Type has been made more flexibleThe
FeatureFlagtype has been updated to accept any string in addition to predefined flags. This provides more flexibility but could lead to typos in flag names going undetected at compile time.Consider maintaining a strict type system by using a string union type for common flags while allowing dynamic flags:
-export type FeatureFlag = typeof FEATURE_FLAGS[number] | string; +type PredefinedFlag = typeof FEATURE_FLAGS[number]; +export type FeatureFlag = PredefinedFlag | (string & {});apps/admin-x-activitypub/src/components/layout/Onboarding/Onboarding.tsx (2)
38-62: Clean step component implementationThe first two step components are cleanly implemented with a consistent structure and proper props typing. However, the static text mentions specific brand names ("404Media"), which should ideally be parameterized.
Consider extracting the hardcoded brand name into a configurable parameter:
- <p>404Media is now part of the world's largest open network.</p> + <p>{brandName} is now part of the world's largest open network.</p>
80-82: Missing loading state during completionWhen handling the completion action, there's no indication to the user that an async operation is in progress, which could lead to a poor user experience if the network request takes time.
Consider adding a loading state:
const Onboarding: React.FC = () => { const {setOnboarded} = useOnboardingStatus(); const [currentStep, setCurrentStep] = useState(1); + const [isCompleting, setIsCompleting] = useState(false); const handleComplete = async () => { + setIsCompleting(true); await setOnboarded(true); + setIsCompleting(false); }; // Then in the Step3 component, pass isCompleting and use it to disable the button // <Step3 onComplete={handleComplete} isCompleting={isCompleting} />.github/workflows/ci.yml (1)
913-920: Shell script quote warningThere's a potential issue with unquoted variables in the shell script that could lead to unexpected behavior if variables contain spaces or special characters.
- output=$(tb --host ${{ secrets.TB_HOST }} --token ${{ secrets.TB_ADMIN_TOKEN_STAGING }} branch ls) + output=$(tb --host "${{ secrets.TB_HOST }}" --token "${{ secrets.TB_ADMIN_TOKEN_STAGING }}" branch ls)🧰 Tools
🪛 actionlint (1.7.4)
913-913: shellcheck reported issue in this script: SC2086:info:7:13: Double quote to prevent globbing and word splitting
(shellcheck)
ghost/i18n/locales/de-CH/newsletter.json (3)
2-3: New Key Additions & Missing Translations
The new key"{date}": "{date}"maintains the placeholder format for dates as expected. However, the key
"Become a paid member of {site} to get access to all premium content."has an empty translation value. Please confirm if this is intentional or if a proper German translation should be provided to avoid missing content in the UI.
11-14: Subscription Management Labels
The translations for subscription-related UI elements—namely"Manage subscription"→"Abos verwalten","Member since"→"Mitglied seit","More like this"→"Mehr davon", and"Name"→"Name"—are correct.
Note: Consider verifying whether"Abos verwalten"(plural) is contextually appropriate, or if a singular form such as"Abo verwalten"might be more consistent with other locales.
15-18: Subscription and Trial Terminology Review
The translation for"paid"is rendered as"zahlendes", which might benefit from a review for consistency with other language files (e.g., comparing with"bezahlt"or similar variants). The translations for"Subscription details","trialing", and"Unsubscribe"are acceptable.
🛑 Comments failed to post (9)
apps/portal/src/data-attributes.js (2)
106-106:
⚠️ Potential issueFixed logical operator for correct locale fallback
Changed from bitwise OR (
|) to logical OR (||) to properly handle falsy values forsite.locale. This ensures that ifsite.localeis undefined, null, or empty, the default 'en' language will be correctly applied.
189-189:
⚠️ Potential issueFixed logical operator for correct locale fallback
Changed from bitwise OR (
|) to logical OR (||) to properly handle falsy values forsite.locale. This ensures that ifsite.localeis undefined, null, or empty, the default 'en' language will be correctly applied.ghost/web-analytics/pipes/mv_sessions.pipe (1)
27-27: 💡 Verification agent
🧩 Analysis chain
Updated source table from '_hits' to 'hits'.
This change aligns with the new structure introduced by the included file. Ensure that the 'hits' table contains all the necessary fields expected by this query.
🏁 Script executed:
#!/bin/bash # Check if hits table is defined in the included file grep -A 10 "hits" "_hits.incl" 2>/dev/null || echo "Warning: 'hits' table not found in _hits.incl"Length of output: 135
Action Required: Update '_hits.incl' to Include the 'hits' Table Definition
After verifying with the grep command, it appears that the
'hits'table definition is missing in the_hits.inclfile. While the change inghost/web-analytics/pipes/mv_sessions.pipeupdates the source from'_hits'to'hits', the included file does not have the corresponding definition or fields expected by the query.
- File:
_hits.incl- Issue: Missing definition for the
hitstable.- Action: Update
_hits.inclto define thehitstable with all the necessary fields expected by the query.ghost/core/test/unit/server/data/schema/integrity.test.js (1)
40-40: 💡 Verification agent
🧩 Analysis chain
Updated settings hash to match current default settings.
This change correctly updates the expected hash for the current settings configuration. This is necessary to ensure the integrity check passes after changes to the default settings.
🏁 Script executed:
#!/bin/bash # Verify the hash was calculated correctly for the updated settings echo "Verifying that the currentSettingsHash matches the actual hash of default settings..." node -e " const crypto = require('crypto'); const fs = require('fs'); const path = require('path'); const settingsPath = path.join(process.cwd(), 'core/server/data/schema/default-settings/default-settings.json'); try { const settings = JSON.parse(fs.readFileSync(settingsPath, 'utf-8')); const hash = crypto.createHash('md5').update(JSON.stringify(settings), 'binary').digest('hex'); console.log('Calculated hash:', hash); console.log('Expected hash: 5eb97fee379c1d0420de58957d29fe26'); console.log('Hash matches?', hash === '5eb97fee379c1d0420de58957d29fe26' ? 'Yes' : 'No'); } catch (err) { console.error('Error verifying hash:', err.message); } "Length of output: 1083
Action Required: Correct File Path for Default Settings
- The updated expected hash (5eb97fee379c1d0420de58957d29fe26) appears to be correct based on the new default settings configuration.
- However, the verification script is failing with an ENOENT error because it cannot locate the default settings file at the specified path (core/server/data/schema/default-settings/default-settings.json).
- Please verify whether the default settings file has been moved or if the file path in the test script needs to be updated to reflect the correct location. Once the proper file path is in place, the integrity check for the default settings hash can be validated reliably.
apps/portal/src/components/pages/NewsletterSelectionPage.js (1)
107-109:
⚠️ Potential issueChange from ID to name might impact downstream functionality
The newsletter object structure passed to
onActionhas been changed from{id: d.id}to{name: d.name}. This could potentially break downstream functionality if other components or API endpoints expect to receive newsletter IDs instead of names.To ensure compatibility with existing systems, verify that downstream components and API endpoints can handle names instead of IDs, or consider maintaining backward compatibility by including both properties:
- return {name: d.name}; + return {id: d.id, name: d.name};📝 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.let newsletters = subscribedNewsletters.map((d) => { return {id: d.id, name: d.name}; });ghost/i18n/locales/ca/newsletter.json (1)
2-3: 💡 Verification agent
🧩 Analysis chain
Missing translations for Catalan locale
New localization keys have been added without translations for Catalan users. These might be needed for new features but currently show as English fallbacks.
While it's acceptable to add keys now and translations later, consider adding a comment or TODO for tracking translation needs, or create a dedicated task for completing translations across all affected locales.
Also applies to: 19-20
🌐 Web query:
What is the best practice for managing empty translation strings in i18n JSON files?💡 Result:
Managing empty translation strings in i18n JSON files requires balancing development workflows, translation processes, and runtime behavior. Here are key best practices:
1. Handling Fallbacks
- i18next Configuration: Enable
missingKeyNoValueFallbackToKey: falseto prevent empty values from triggering fallbacks to keys or other languages[1][3].- Explicit Fallbacks: Use i18next's
fallbackLngto designate a default language when translations are missing[3].2. Linting and Validation
- ESLint Rules: Use
@maccuaa/eslint-plugin-i18n-jsonto:
- Ignore empty keys via
i18n-json/ignore-keysduring development[2][5].- Validate syntax while excluding work-in-progress translations[2][5].
- Key Tracking: Enable
saveMissing: truein i18next to log untranslated keys during development[1].3. Translation File Practices
- Avoid Empty Placeholders: Use source-language values instead of empty strings to maintain UI integrity[9].
- Structured Metadata: Include comments or metadata fields (e.g.,
translation-note) to flag untranslated keys[11].4. Workflow Integration
- Translation Systems: Configure tools like Smartling to:
- Parse JSON values while ignoring empty strings via directives like
translate_paths[11].- Use machine translation to auto-fill empty values where appropriate[10].
- Version Control: Track empty keys as untranslated entries and enforce completion before releases[8][12].
5. Pluralization and Interpolation
- Reserved Variables: Use
countfor pluralization instead of empty strings to leverage i18n library handling[6][9].- ICU Syntax: Validate message syntax with linters to catch invalid empty interpolations[5][9].
Configuration Example (i18next)
i18next.init({ saveMissing: true, missingKeyNoValueFallbackToKey: false, fallbackLng: 'en', interpolation: { escapeValue: false } });By combining proper tool configuration, linting rules, and workflow practices, teams can manage empty translations effectively while avoiding runtime issues and maintaining translation integrity.
Citations:
- 1: https://www.i18next.com/overview/configuration-options
- 2: https://www.npmjs.com/package/@maccuaa%2Feslint-plugin-i18n-json
- 3: https://www.i18next.com/principles/fallback
- 4: https://shopify.engineering/internationalization-i18n-best-practices-front-end-developers
- 5: https://www.npmjs.com/package/eslint-plugin-i18n-json/v/2.2.0
- 6: https://inveniordm.docs.cern.ch/develop/howtos/i18n/
- 7: https://discourse.gohugo.io/t/best-practices-for-i18n/1276
- 8: Empty string as message content is ignored nicksnyder/go-i18n#329
- 9: https://www.i18next.com/principles/best-practices
- 10: https://lokalise.com/blog/json-l10n/
- 11: https://help.smartling.com/hc/en-us/articles/360008000733-JSON
- 12: Is there a way to clean unused translations from json files automatically? mashpie/i18n-node#336
- 13: Empty string causes fallback i18next/i18next#80
- 14: https://lists.cloudfoundry.org/g/cf-dev/attachment/5702/0/attachment.html
Action Required: Add TODOs or comments for pending Catalan translations
It appears that the newly added keys in this JSON file are missing translations for Catalan. Based on current best practices for i18n management, leaving translation strings empty can lead to unintended fallbacks at runtime. Instead of leaving these fields blank, please consider:
Annotating the keys:
Add inline comments or a TODO next to these keys so that it’s clear these require future translation work.Configuring fallbacks explicitly:
Make sure your i18next (or equivalent) configuration is set to handle missing translations properly (e.g., using options likemissingKeyNoValueFallbackToKey: falseand a designatedfallbackLng).Reviewing related entries:
The same issue applies to lines 19–20. Please review and annotate those entries as well.This change not only tracks translation needs but also prevents unexpected UI behavior due to empty strings in localized content.
ghost/i18n/locales/pt-BR/newsletter.json (1)
2-3: 💡 Verification agent
🧩 Analysis chain
Missing translations for new membership strings
Added placeholder for date formatting and a new string for premium content access, but the premium content string currently has no translation. Consider providing a Portuguese translation for this important membership message.
🌐 Web query:
What is "Become a paid member of {site} to get access to all premium content" in Brazilian Portuguese?💡 Result:
A tradução para o português brasileiro da frase "Become a paid member of {site} to get access to all premium content" pode ser:
Opção 1:
"Torne-se um membro premium do {site} para ter acesso a todo o conteúdo premium."Opção 2 (mais coloquial):
"Seja um membro pago do {site} para acessar todo o conteúdo exclusivo."Detalhes linguísticos:
- "Paid member" pode ser traduzido como "membro pago" ou "membro premium", dependendo do contexto (sendo a segunda opção mais comum em plataformas digitais brasileiras)[6].
- "Premium content" geralmente mantém o termo "premium" em português brasileiro, mas também pode ser adaptado para "conteúdo exclusivo" para maior naturalidade[6].
- A construção "to get access" usa o verbo "ter acesso" ou "acessar", ambos amplamente utilizados[6].
Para sites de assinatura, recomenda-se usar "plano premium" ou "membro premium" como termos padrão no mercado brasileiro[3][6][9].
Citations:
- 1: https://portuguesepedia.com/faq/
- 2: https://portuguesewitheli.com/club/
- 3: https://www.pimsleur.com/learn-portuguese-brazilian/lifetime-subscription/
- 4: https://www.practiceportuguese.com/become-a-premium-member/
- 5: https://www.pimsleur.com/learn-portuguese-european/lifetime-subscription/
- 6: https://www.paidmembershipspro.com/paid-memberships-pro-in-your-language/
- 7: https://en.wikipedia.org/wiki/Pimsleur_Language_Programs
- 8: https://reallylearnportuguese.com/rbc-subscription-plan/
- 9: https://reallylearnportuguese.com/subscription-plan/
Translation Update Needed for Premium Membership Message
The membership string in
ghost/i18n/locales/pt-BR/newsletter.jsoncurrently has an empty translation. Based on the latest guidance, please update it with one of the following options:
- Option 1:
"Torne-se um membro premium do {site} para ter acesso a todo o conteúdo premium."- Option 2:
"Seja um membro pago do {site} para acessar todo o conteúdo exclusivo."Choose the option that best fits the tone and style of your platform.
ghost/i18n/locales/de-CH/newsletter.json (2)
19-20:
⚠️ Potential issueMissing Translations for Upgrade Keys
Both"Upgrade"and"Upgrade to continue reading."remain with empty values. This can result in missing UI text when these keys are used. Please confirm if these are placeholders pending a translation or if a default string should be provided. For example, you might consider something like:-"Upgrade": "", -"Upgrade to continue reading.": "", +"Upgrade": "Upgrade", +"Upgrade to continue reading.": "Zum Weiterlesen upgraden."📝 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."Upgrade": "Upgrade", "Upgrade to continue reading.": "Zum Weiterlesen upgraden."
22-22: 💡 Verification agent
❓ Verification inconclusive
Placeholder Format Inconsistency
The source string contains the placeholder{site}, but in the translation the placeholder appears as{{site}}. To avoid potential runtime issues with placeholder substitution, please ensure that the placeholder format remains consistent. For example:-"You are receiving this because you are a <strong>%%{status}%% subscriber</strong> to {site}.": "Sie erhalten diese Nachricht, da Sie ein <strong>%%{status}%% Mitglied</strong> unserer Seite «{{site}}» sind.", +"You are receiving this because you are a <strong>%%{status}%% subscriber</strong> to {site}.": "Sie erhalten diese Nachricht, da Sie ein <strong>%%{status}%% Mitglied</strong> unserer Seite «{site}» sind.",
Placeholder Format Issue – Update Translation
The file
ghost/i18n/locales/de-CH/newsletter.jsonat line 22 still shows an inconsistency between the source and the translated placeholder. The source string uses{site}while the translation mistakenly uses{{site}}, which can lead to runtime substitution issues. Please update the translated placeholder to use{site}for consistency.Required Change:
- File:
ghost/i18n/locales/de-CH/newsletter.json(Line 22)- Diff Suggestion:
-"You are receiving this because you are a <strong>%%{status}%% subscriber</strong> to {site}.": "Sie erhalten diese Nachricht, da Sie ein <strong>%%{status}%% Mitglied</strong> unserer Seite «{{site}}» sind.", +"You are receiving this because you are a <strong>%%{status}%% subscriber</strong> to {site}.": "Sie erhalten diese Nachricht, da Sie ein <strong>%%{status}%% Mitglied</strong> unserer Seite «{site}» sind.",📝 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."You are receiving this because you are a <strong>%%{status}%% subscriber</strong> to {site}.": "Sie erhalten diese Nachricht, da Sie ein <strong>%%{status}%% Mitglied</strong> unserer Seite «{site}» sind.",
Closes #21103
"Auto" for comments works great for themes that detect browser preferences or theme settings. However, themes with toggle switches can end up looking rather strange when the toggle is flipped.
This PR adds a mutation observer that watches the parent of #ghost-comments-root for class or style changes. If either is detected, it re-runs the darkmode function and updates new state, triggering a re-render in the correct color scheme.
Theme creators with toggle switches can trigger the behavior either by adding a class to the parent of #ghost-comments-root or by directly adding a color to that element's style. Note that mutation observers do not detect when CSS variables or inherited colors change, so other change types (inherited colors from an ancestor or changes to a variable) will not trigger an update.
I considered detecting changes at body or html, but some themes make a lot of unrelated changes to those elements, so it seemed safest to stay local, at the risk of not "just working". I considered some heavier packages that attempt to observe all style changes, but wasn't convinced that these workarounds were both robust and lightweight.