🎨 Added SelectWithOther component for publication language settings #25774
Conversation
betschki
requested review from
9larsons,
cmraible and
ibalosh
as code owners
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces a new SelectWithOther component for the admin design system that extends a standard Select input with an "Other" option, enabling users to enter custom values. Supporting this addition, a new validateLocale utility function validates BCP 47 locale strings with comprehensive format checks. The publication-language settings component is refactored to use SelectWithOther instead of TextField. The i18n package is restructured to export LOCALE_DATA from a new locale-data.json file, replacing a hard-coded SUPPORTED_LOCALES array. Corresponding E2E and unit tests are added to verify the new language selection flow and locale validation behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
betschki
changed the title
![cursor[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 3
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
20-47: LGTM! Well-structured Page Object method.The
setLanguage()method correctly implements the dual-path logic for selecting from the dropdown or entering custom input. The implementation follows Page Object best practices with semantic method naming and appropriate use of Playwright's auto-waiting.Minor observation: The method checks for option existence at line 35 by counting matches, which is a valid approach. Consider using
await optionLocator.count()result directly in the condition rather than comparing with 0.Optional: Simplify the condition check
- const optionExists = await optionLocator.count() > 0; + const optionCount = await optionLocator.count(); - if (optionExists) { + if (optionCount > 0) {apps/admin-x-settings/src/components/settings/general/publication-language.tsx (1)
21-29: Consider consolidating validation logic.There's potential overlap in empty-value validation:
onValidatereturns'Enter a value'when emptyvalidateLocalereturns'Locale is required'when emptySelectWithOtherinternally sets'This field is required'when empty andallowEmptyis falseThis works functionally but could show inconsistent error messages. Consider relying on a single validation path for consistency.
Also applies to: 82-82
apps/admin-x-design-system/src/global/form/select-with-other.tsx (2)
37-43: Potential stale value issue withisInitiallyCustomValue.
isInitiallyCustomValueis computed once during the initial render fromselectedValue. IfselectedValueprop changes to a new custom value while the component is mounted,isInitiallyCustomValuewon't update, but it's still used inshowCustomInput(line 95).Consider deriving this check inside
useMemooruseEffectto handle prop changes, or rely solely onisOtherSelectedstate after initial mount.🔎 Proposed fix using derived state
- // Check if the current value is a custom value (not in predefined options) - const isInitiallyCustomValue = selectedValue && - !options.some(opt => opt.value === selectedValue) && - selectedValue !== otherOption.value; - - const [isOtherSelected, setIsOtherSelected] = useState(isInitiallyCustomValue); + // Check if the current value is a custom value (not in predefined options) + const isCustomValue = React.useMemo(() => { + return selectedValue && + !options.some(opt => opt.value === selectedValue) && + selectedValue !== otherOption.value; + }, [selectedValue, options, otherOption.value]); + + const [isOtherSelected, setIsOtherSelected] = useState(!!isCustomValue); const [validationError, setValidationError] = useState<string | null>(null);And update line 95:
- const showCustomInput = isOtherSelected || isInitiallyCustomValue; + const showCustomInput = isOtherSelected || isCustomValue;
45-57: Selecting "Other" clears the current value.When users select "Other...",
onSelect('')is called (line 52), which clears any existing selection. If a user accidentally clicks "Other" while having a valid selection, their value is lost. Consider preserving the existing value or confirming the action.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsxapps/admin-x-design-system/src/global/form/select-with-other.tsxapps/admin-x-design-system/src/index.tsapps/admin-x-settings/package.jsonapps/admin-x-settings/src/components/settings/general/publication-language.tsxapps/admin-x-settings/src/utils/locale-validation.tsapps/admin-x-settings/test/acceptance/general/publication-language.test.tsapps/admin-x-settings/test/unit/utils/locale-validation.test.tsapps/admin-x-settings/vite.config.mjse2e/helpers/pages/admin/settings/sections/publications-section.tse2e/tests/admin/settings/publication-language.test.tsghost/i18n/index.mjsghost/i18n/lib/i18n.jsghost/i18n/lib/locale-data.jsonghost/i18n/package.json
🧰 Additional context used
📓 Path-based instructions (4)
e2e/**/*.test.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/**/*.test.ts: Always follow ADRs in../adr/folder (ADR-0001: AAA pattern, ADR-0002: Page Objects)
Never use CSS/XPath selectors - only use semantic locators or data-testid
Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Test names should be lowercase and follow the format 'what is tested - expected outcome'
Each test should represent one scenario only - never mix multiple scenarios in a single test
Follow the AAA (Arrange, Act, Assert) pattern in test structure
Prefer semantic locators (getByRole,getByLabel,getByText) over test IDs and never use CSS selectors, XPath, nth-child, or class names
UsegetByTestId()only when semantic locators are unavailable, and suggest addingdata-testidto Ghost codebase when needed
Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Never use hard-coded waits likewaitForTimeout()
Never usenetworkidlein waits
Use Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Usetest.only()for debugging single tests
Manual login should not be used - authentication is automatic via fixture
Files:
e2e/tests/admin/settings/publication-language.test.ts
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/tests/admin/settings/publication-language.test.tse2e/helpers/pages/admin/settings/sections/publications-section.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/tests/admin/settings/publication-language.test.tse2e/helpers/pages/admin/settings/sections/publications-section.ts
e2e/helpers/pages/**/*.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/helpers/pages/**/*.ts: Page Objects should be located inhelpers/pages/directory
Expose locators aspublic readonlyin Page Objects when used with assertions
Page Object methods should use semantic names (e.g.,login()instead ofclickLoginButton())
UsewaitFor()for guards in Page Objects, never useexpect()in page objects
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
🧠 Learnings (29)
📓 Common learnings
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Expose locators as `public readonly` in Page Objects when used with assertions
Applied to files:
e2e/tests/admin/settings/publication-language.test.ts
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
apps/admin-x-settings/src/utils/locale-validation.tsghost/i18n/index.mjsghost/i18n/lib/locale-data.jsonapps/admin-x-settings/test/acceptance/general/publication-language.test.tsghost/i18n/lib/i18n.jsapps/admin-x-settings/package.jsonghost/i18n/package.jsonapps/admin-x-settings/test/unit/utils/locale-validation.test.tsapps/admin-x-settings/vite.config.mjsapps/admin-x-settings/src/components/settings/general/publication-language.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories should demonstrate key variants and states (sizes, disabled/loading, critical props) with minimal but representative examples
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories for new components should include an overview explaining what the component does and its primary use case
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: For new UI components, prioritize comprehensive Storybook stories; add focused unit tests where they provide real value (e.g., hooks, utils, logic-heavy parts)
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/ui/**/*.{ts,tsx} : Atomic UI components should be placed in `src/components/ui/*` and each component must have a corresponding `*.stories.tsx` file next to it for Storybook documentation
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Centralized i18n translations should be maintained in `ghost/i18n/locales/{locale}/{namespace}.json` files with 60+ supported locales
Applied to files:
ghost/i18n/index.mjsghost/i18n/lib/locale-data.jsonghost/i18n/lib/i18n.jsapps/admin-x-settings/package.jsonghost/i18n/package.jsonapps/admin-x-settings/vite.config.mjs
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/{portal,comments,signup-form,search}.json : Add UI translations to namespace-specific files in `ghost/i18n/locales/en/` for public UI apps (portal.json for Portal, comments.json for Comments UI, etc.)
Applied to files:
ghost/i18n/index.mjsghost/i18n/lib/locale-data.jsonghost/i18n/lib/i18n.jsapps/admin-x-settings/package.jsonghost/i18n/package.jsonapps/admin-x-settings/vite.config.mjs
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
ghost/i18n/index.mjsghost/i18n/lib/locale-data.jsonapps/admin-x-settings/test/acceptance/general/publication-language.test.tsghost/i18n/lib/i18n.jsapps/admin-x-settings/package.jsonghost/i18n/package.jsonapps/admin-x-settings/vite.config.mjs
📚 Learning: 2025-01-29T15:35:26.447Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 21651
File: ghost/i18n/locales/ar/comments.json:1-1
Timestamp: 2025-01-29T15:35:26.447Z
Learning: All JSON files containing translation strings in ghost/i18n/locales must include both opening and closing braces to maintain valid JSON structure.
Applied to files:
ghost/i18n/lib/locale-data.jsonghost/i18n/lib/i18n.jsapps/admin-x-settings/vite.config.mjs
📚 Learning: 2025-05-20T21:08:21.026Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Applied to files:
ghost/i18n/lib/locale-data.jsonghost/i18n/package.json
📚 Learning: 2025-01-29T15:27:29.391Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 0
File: :0-0
Timestamp: 2025-01-29T15:27:29.391Z
Learning: When reviewing translation files in ghost/i18n/locales, translators should only modify the translated values and must not change the keys or placeholder styles (e.g., %%{status}%%, {site}, etc.) as they serve specific purposes like Mailgun string replacement.
Applied to files:
ghost/i18n/lib/locale-data.json
📚 Learning: 2025-01-29T15:23:58.658Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 21982
File: ghost/i18n/locales/hu v2/portal.json:70-70
Timestamp: 2025-01-29T15:23:58.658Z
Learning: Typos and spelling errors within localization files (ghost/i18n/locales/*.json) must be treated as important problems, not nitpicks, as they directly affect the user experience for speakers of that language. These issues should be flagged with high priority during code review.
Applied to files:
ghost/i18n/lib/locale-data.jsonghost/i18n/lib/i18n.js
📚 Learning: 2025-04-30T16:27:03.542Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 23070
File: ghost/i18n/locales/hu/comments.json:45-45
Timestamp: 2025-04-30T16:27:03.542Z
Learning: In the Ghost project, new translation string keys are intentionally added with empty values initially and will be translated in a separate PR. Do not report missing translations for entirely new string additions.
Applied to files:
ghost/i18n/lib/locale-data.json
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts
📚 Learning: 2025-05-14T14:39:09.136Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-08-12T18:33:15.524Z
Learnt from: cmraible
Repo: TryGhost/Ghost PR: 24658
File: ghost/admin/package.json:3-3
Timestamp: 2025-08-12T18:33:15.524Z
Learning: In Ghost's admin package.json, third-party packages like ember-cli-postcss, ember-exam, and ember-power-select have their own independent versioning schemes that are unrelated to Ghost's version numbers. Version number coincidences between Ghost versions and these packages should not trigger update suggestions.
Applied to files:
apps/admin-x-settings/package.json
📚 Learning: 2025-08-26T16:47:28.150Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 24749
File: ghost/core/core/server/services/members/SingleUseTokenProvider.js:3-5
Timestamp: 2025-08-26T16:47:28.150Z
Learning: When checking for dependencies in Ghost project, ensure to look directly in the specific package.json files rather than relying only on automated searches, as otplib dependency exists at line 204 in ghost/core/package.json version "12.0.1"
Applied to files:
apps/admin-x-settings/package.json
📚 Learning: 2025-08-01T12:44:07.467Z
Learnt from: niranjan-uma-shankar
Repo: TryGhost/Ghost PR: 24557
File: apps/admin-x-settings/src/components/settings/general/TimeZone.tsx:7-7
Timestamp: 2025-08-01T12:44:07.467Z
Learning: In Ghost development, PRs may depend on unpublished changes from SDK packages. When this occurs, TypeScript compilation errors for missing exports are expected and documented in the PR description until the dependency packages are published and updated. This is normal workflow for cross-repository feature development.
Applied to files:
ghost/i18n/package.json
📚 Learning: 2025-05-29T07:45:35.714Z
Learnt from: ErisDS
Repo: TryGhost/Ghost PR: 23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
Applied to files:
ghost/i18n/package.json
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
Applied to files:
apps/admin-x-design-system/src/index.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/test/unit/**/*.test.{ts,tsx,js} : Vitest unit tests should be located in `test/unit/*` with pattern `test/unit/**/*.(test).(ts|tsx|js)` and use the `render` helper from `test/unit/utils/test-utils.tsx`
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test names should be lowercase and follow the format 'what is tested - expected outcome'
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Admin-x React apps build to `apps/*/dist` using Vite, which are then copied by `ghost/admin/lib/asset-delivery` to `ghost/core/core/built/admin/assets/*`
Applied to files:
apps/admin-x-settings/vite.config.mjs
📚 Learning: 2025-10-21T13:04:40.040Z
Learnt from: jonatansberg
Repo: TryGhost/Ghost PR: 24967
File: apps/admin/vite.config.ts:1-8
Timestamp: 2025-10-21T13:04:40.040Z
Learning: In Vite config files (vite.config.ts, vite.config.js), __dirname can be used directly even when the package.json has "type": "module". Vite handles config file transpilation in a way that makes __dirname available, so no ESM shim (fileURLToPath/dirname) is needed.
Applied to files:
apps/admin-x-settings/vite.config.mjs
📚 Learning: 2025-12-01T08:42:35.320Z
Learnt from: jonatansberg
Repo: TryGhost/Ghost PR: 25552
File: e2e/helpers/environment/service-managers/dev-ghost-manager.ts:210-247
Timestamp: 2025-12-01T08:42:35.320Z
Learning: In e2e/helpers/environment/service-managers/dev-ghost-manager.ts, the hardcoded volume name 'ghost-dev_shared-config' at line 231 is intentional. E2E tests run under the 'ghost-dev-e2e' project namespace but deliberately mount the shared-config volume from the main 'ghost-dev' project to access Tinybird credentials created by yarn dev:forward. This is cross-project volume sharing by design.
Applied to files:
apps/admin-x-settings/vite.config.mjs
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Use React with Vite for Admin app development (admin-x-settings, admin-x-activitypub, posts, stats)
Applied to files:
apps/admin-x-settings/vite.config.mjs
🧬 Code graph analysis (4)
ghost/i18n/index.mjs (1)
ghost/i18n/lib/i18n.js (2)
SUPPORTED_LOCALES(11-11)LOCALE_DATA(8-8)
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts (1)
apps/admin-x-framework/src/test/acceptance.ts (3)
mockApi(211-272)globalDataRequests(161-166)updatedSettingsResponse(274-287)
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts (1)
apps/admin-x-settings/src/utils/locale-validation.ts (1)
validateLocale(6-188)
e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
Error(101-108)
🪛 Biome (2.1.2)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
[error] 101-101: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Setup
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (23)
apps/admin-x-settings/vite.config.mjs (1)
32-32: LGTM!The addition of
/ghost\/i18n/to the CommonJS include patterns correctly aligns with the new i18n dependency and follows the existing pattern for custom-fonts.apps/admin-x-settings/package.json (1)
44-44: LGTM!The addition of
@tryghost/i18ndependency is correctly placed and necessary for the locale selection feature.ghost/i18n/package.json (2)
8-14: LGTM!The addition of the
modulefield andexportsmapping correctly enables ESM support for the i18n package while maintaining CJS compatibility. This follows Node.js package dual-format best practices.
33-33: LGTM!Correctly includes
index.mjsin the published files array to support the new ESM entry point.e2e/helpers/pages/admin/settings/sections/publications-section.ts (2)
8-9: LGTM!The locator updates correctly reflect the new UI structure with
localeSelectfor the dropdown andcustomLanguageFieldfor custom input. UsinggetByTestIdfor the select is appropriate when semantic locators are insufficient for dynamic dropdowns.Also applies to: 16-17
49-51: LGTM!The refactored
resetToDefaultLanguage()method correctly delegates tosetLanguage(), reducing code duplication and ensuring consistent behavior.e2e/tests/admin/settings/publication-language.test.ts (1)
21-22: LGTM!The assertion correctly verifies the locale selection by checking the dropdown text. This aligns with the new SelectWithOther UI where the selected value displays as "French (fr)".
apps/admin-x-design-system/src/index.ts (1)
47-48: LGTM!The exports for
SelectWithOtherandSelectWithOtherPropsfollow the established pattern and are correctly positioned in the exports list.ghost/i18n/lib/locale-data.json (1)
1-64: No issues found. The locale data inghost/i18n/lib/locale-data.jsonis complete and correctly aligned with all available translation directories.ghost/i18n/index.mjs (1)
1-7: LGTM! Clean ESM entry point.The implementation correctly provides a single source of truth for locale data by re-exporting
LOCALE_DATAand derivingSUPPORTED_LOCALESfrom it. The use of JSON import attributes withwith { type: 'json' }is the current standardized syntax (requires Node.js v18.20.0+).ghost/i18n/lib/i18n.js (2)
7-11: LGTM! Clean refactor to centralize locale data.The approach of loading
LOCALE_DATAfrom JSON and derivingSUPPORTED_LOCALESfrom it maintains backward compatibility while adding the new export. This aligns with the learning that existing locale codes require backward compatibility handling.
153-153: Appropriate addition of new export.The new
LOCALE_DATAexport enables consumers to access both locale codes and their human-readable labels, supporting the newSelectWithOthercomponent's needs.apps/admin-x-settings/test/unit/utils/locale-validation.test.ts (1)
1-122: Comprehensive test coverage for locale validation.The test suite covers a wide range of valid and invalid BCP 47 locale formats including edge cases like grandfathered tags, private use tags, case insensitivity, and whitespace trimming. Good coverage of common mistakes users might make.
apps/admin-x-settings/src/components/settings/general/publication-language.tsx (2)
34-41: Good use of memoization for locale options.The
localeOptionsarray is properly memoized since it's derived from staticLOCALE_DATAand doesn't depend on any changing values.
72-85: Well-integrated SelectWithOther component.The component is properly configured with appropriate defaults (
defaultListValue="en"), helpful user hints, and validation wired tovalidateLocale. TheisSearchableprop improves UX for the 60+ locale options.apps/admin-x-settings/src/utils/locale-validation.ts (2)
1-72: Thorough BCP 47 validation with proper grandfathered tag support.The implementation correctly handles both irregular and regular grandfathered tags from the IANA Language Subtag Registry, with helpful comments explaining each tag's purpose and modern replacements.
97-160: Correct BCP 47 structure parsing.The parsing logic correctly handles the hierarchical structure: language → script → region → variants → extensions → private-use tail. The extension singleton validation properly excludes 'x' and requires at least one 2-8 char subtag.
apps/admin-x-design-system/src/global/form/select-with-other.tsx (2)
78-92: Good fallback logic inhandleBackToList.The function correctly prioritizes
defaultListValueif provided and valid, otherwise falls back to the first available option. Validation error is properly cleared.
101-127: TextField rendering handles error states correctly.The custom input mode properly displays validation errors, uses the appropriate hints, and provides a clear "back to list" action for users to revert to the dropdown.
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts (3)
5-35: Good test for dropdown-based locale selection.The test properly verifies the full flow: initial value display, opening dropdown, selecting a new locale, saving, and checking the API payload. The rename to "Supports selecting a language from the dropdown" accurately reflects the new UI.
37-64: Good coverage of custom locale entry flow.This test exercises the complete "Other" option workflow: opening dropdown, selecting "Other...", entering a custom BCP 47 locale code, saving, and verifying the API payload.
66-85: Proper validation error test.Using
'invalid--locale'(consecutive hyphens) correctly triggers the validation error, and the test verifies the error message is displayed.apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
1-39: Good Storybook coverage for the new component.The stories demonstrate key variants and states including default rendering, selected values, hints, searchable mode, custom values, validation, error states, disabled state, and custom labels. This aligns with best practices for Storybook documentation.
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts
Show resolved
Hide resolved
betschki
force-pushed
the
feature/select-with-other-publication-language
branch
from
ee87700 to
be21759
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
101-108: RenameErrorstory to avoid shadowing global.The export name
Errorshadows the globalErrorconstructor. Consider renaming toErrorStateorWithErroras previously suggested.🔎 Proposed fix
-export const Error: Story = { +export const ErrorState: Story = { args: { title: 'Site language', options: languageOptions, error: true, hint: 'Please select a valid language' } };apps/admin-x-settings/src/utils/locale-validation.ts (1)
164-173: Critical: indexOf bugs cause incorrect private-use validation.Two issues prevent correct validation of private-use sections:
Case-sensitive 'x' search (line 170):
segments.indexOf('x')fails for uppercase 'X' in tags likeen-X-private, causing private-use segments to be incorrectly validated againstinvalidWords.Wrong index lookup (line 171):
segments.indexOf(segment)returns the first occurrence index, not the current iteration's position. Foren-x-en, the secondenincorrectly matches index 0, breaking the private-use check.🔎 Fix using index-based iteration
- for (const segment of segments) { + for (let i = 0; i < segments.length; i++) { + const segment = segments[i]; // Skip grandfathered prefixes and private use if (['i', 'x', 'sgn', 'art', 'cel', 'no', 'zh'].includes(segment.toLowerCase())) { continue; } // Skip segments in private use (after 'x') - const xIndex = segments.indexOf('x'); - if (xIndex !== -1 && segments.indexOf(segment) > xIndex) { + const xIndex = segments.findIndex(s => s.toLowerCase() === 'x'); + if (xIndex !== -1 && i > xIndex) { continue; }apps/admin-x-settings/test/acceptance/general/publication-language.test.ts (1)
108-120: Mock path mismatch prevents override from taking effect.The
browseSettingsoverride uses exact path'/settings/', but the actual request includes query parameters like/settings/?group=site,theme,private,.... Per themockApiimplementation (line 232 in acceptance.ts), exact string paths use strict equality, so the override never matches. The test falls back toglobalDataRequests.browseSettings, which may return the full settings fixture instead of just thelocalesetting.🔎 Fix by using regex path pattern
browseSettings: { method: 'GET', - path: '/settings/', + path: /^\/settings\/\?group=/, response: { settings: [ {key: 'locale', value: 'cy'} ] } }
🧹 Nitpick comments (2)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
8-18: Consider adding a component overview to the meta configuration.Based on retrieved learnings for Storybook stories, new components should include an overview explaining what the component does and its primary use case. Consider adding a
parameters.docs.description.componentfield to the meta configuration.🔎 Suggested enhancement
const meta = { title: 'Global / Form / Select With Other', component: SelectWithOther, tags: ['autodocs'], decorators: [(_story: () => ReactNode) => (<div style={{maxWidth: '400px'}}>{_story()}</div>)], + parameters: { + docs: { + description: { + component: 'A select dropdown component with an "Other..." option that allows users to either choose from predefined options or enter a custom value. Useful for scenarios like language selection where users may need flexibility beyond the preset list.' + } + } + }, argTypes: { hint: { control: 'text' } } } satisfies Meta<typeof SelectWithOther>;Based on learnings, comprehensive Storybook documentation improves component discoverability and understanding.
e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
42-42: Consider using a more specific selector for the "Other" option.Using
hasText: 'Other'could potentially match unintended options if any other option text contains the word "Other".🔎 More robust alternatives
Option 1: Use exact text match if Playwright supports it in your version:
-await this.page.getByTestId('select-option').filter({hasText: 'Other'}).click(); +await this.page.getByTestId('select-option').filter({hasText: /^Other\.\.\.$/}).click();Option 2: Add a specific test ID to the "Other" option in the component:
-await this.page.getByTestId('select-option').filter({hasText: 'Other'}).click(); +await this.page.getByTestId('select-option-other').click();
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsxapps/admin-x-design-system/src/global/form/select-with-other.tsxapps/admin-x-design-system/src/index.tsapps/admin-x-settings/package.jsonapps/admin-x-settings/src/components/settings/general/publication-language.tsxapps/admin-x-settings/src/utils/locale-validation.tsapps/admin-x-settings/test/acceptance/general/publication-language.test.tsapps/admin-x-settings/test/unit/utils/locale-validation.test.tsapps/admin-x-settings/vite.config.mjse2e/helpers/pages/admin/settings/sections/publications-section.tse2e/tests/admin/settings/publication-language.test.tsghost/i18n/index.jsghost/i18n/lib/i18n.jsghost/i18n/lib/locale-data.jsonghost/i18n/package.json
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/admin-x-settings/package.json
- apps/admin-x-settings/vite.config.mjs
- ghost/i18n/package.json
- ghost/i18n/lib/i18n.js
- e2e/tests/admin/settings/publication-language.test.ts
- apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
- ghost/i18n/lib/locale-data.json
- apps/admin-x-design-system/src/global/form/select-with-other.tsx
- apps/admin-x-design-system/src/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
e2e/helpers/pages/**/*.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/helpers/pages/**/*.ts: Page Objects should be located inhelpers/pages/directory
Expose locators aspublic readonlyin Page Objects when used with assertions
Page Object methods should use semantic names (e.g.,login()instead ofclickLoginButton())
UsewaitFor()for guards in Page Objects, never useexpect()in page objects
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
apps/admin-x-settings/src/utils/locale-validation.tsapps/admin-x-settings/test/acceptance/general/publication-language.test.tsapps/admin-x-settings/src/components/settings/general/publication-language.tsxghost/i18n/index.js
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories should demonstrate key variants and states (sizes, disabled/loading, critical props) with minimal but representative examples
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories for new components should include an overview explaining what the component does and its primary use case
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: For new UI components, prioritize comprehensive Storybook stories; add focused unit tests where they provide real value (e.g., hooks, utils, logic-heavy parts)
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/ui/**/*.{ts,tsx} : Atomic UI components should be placed in `src/components/ui/*` and each component must have a corresponding `*.stories.tsx` file next to it for Storybook documentation
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/{portal,comments,signup-form,search}.json : Add UI translations to namespace-specific files in `ghost/i18n/locales/en/` for public UI apps (portal.json for Portal, comments.json for Comments UI, etc.)
Applied to files:
ghost/i18n/index.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Centralized i18n translations should be maintained in `ghost/i18n/locales/{locale}/{namespace}.json` files with 60+ supported locales
Applied to files:
ghost/i18n/index.js
🧬 Code graph analysis (3)
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts (1)
apps/admin-x-framework/src/test/acceptance.ts (3)
mockApi(211-272)globalDataRequests(161-166)updatedSettingsResponse(274-287)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
apps/admin-x-design-system/src/index.ts (1)
SelectOption(46-46)
e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
Error(101-108)
🪛 Biome (2.1.2)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
[error] 101-101: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Lint
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (16)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
34-127: Comprehensive story coverage looks excellent.The story variants effectively demonstrate the component's key features including: default behavior, selected values, hints, searchability, custom values, validation, error states, disabled state, and label customization. This aligns well with best practices for component documentation.
e2e/helpers/pages/admin/settings/sections/publications-section.ts (2)
8-9: LGTM! Locators follow Page Object best practices.The new locators are properly exposed as
readonlyand use semantic selectors (getByTestIdfor the dropdown,getByLabelfor the custom input field).Also applies to: 16-17
49-51: Good refactoring - proper delegation.The method now correctly delegates to
setLanguage()instead of duplicating logic, following the DRY principle.ghost/i18n/index.js (1)
1-8: LGTM! Clean dual-export pattern for bundler compatibility.The explicit exports handle both CommonJS (
require) and ESM (import) consumers correctly. This addresses the mentioned CommonJS export/import issues in the PR objectives.apps/admin-x-settings/src/components/settings/general/publication-language.tsx (4)
1-8: LGTM! Imports align with the new architecture.The imports correctly reference the new static JSON locale data, validation utility, and SelectWithOther component introduced in this PR.
34-41: LGTM! Locale options correctly derived from LOCALE_DATA.The memoized options properly transform the static locale data into the SelectOption format with human-readable labels.
43-48: LGTM! Handler correctly updated for SelectWithOther.The signature change from event handler to value handler aligns with the SelectWithOther component's API.
72-85: LGTM! SelectWithOther properly integrated with validation.The component is configured correctly with locale options, validation, error handling, and custom input support. The
validateLocalefunction integration ensures BCP 47 compliance.apps/admin-x-settings/src/utils/locale-validation.ts (4)
6-18: LGTM! Clear validation function signature and basic checks.The function correctly validates required input and handles whitespace, returning
nullfor valid values and descriptive error strings for invalid ones.
20-72: LGTM! Comprehensive grandfathered tag support.The implementation correctly handles the 26 legacy BCP 47 tags with case-insensitive matching, well-documented with their purposes and modern equivalents.
74-95: LGTM! Solid basic format and private-use validation.The regex correctly enforces BCP 47 character rules, and private-use tags starting with 'x' are properly validated with case-insensitive matching.
97-160: LGTM! Comprehensive BCP 47 structure validation.The sequential parsing correctly validates all BCP 47 components (language, script, region, variants, extensions, private-use) with appropriate regex patterns and position tracking.
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts (4)
5-35: LGTM! Comprehensive test for dropdown selection.The test correctly verifies the full flow: initial display, dropdown interaction, selection, save, and API payload validation.
37-64: LGTM! Test covers custom locale entry via Other option.The test properly validates the alternate path for entering non-predefined locales through the custom input.
66-85: LGTM! Validation error handling tested.The test correctly verifies that invalid locale formats trigger validation errors without submitting.
87-106: LGTM! Tests the ability to return to dropdown.The test ensures users can switch back from custom input to the predefined list, validating good UX reversibility.
e2e/helpers/pages/admin/settings/sections/publications-section.ts
Outdated
Show resolved
Hide resolved
betschki
force-pushed
the
feature/select-with-other-publication-language
branch
from
be21759 to
2be929b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
101-108: RenameErrorstory to avoid shadowing global.The export name
Errorshadows the globalErrorconstructor. Consider renaming toErrorStateorWithError.🔎 Proposed fix
-export const Error: Story = { +export const ErrorState: Story = { args: { title: 'Site language', options: languageOptions, error: true, hint: 'Please select a valid language' } };e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
31-35: Potential race condition when checking option existence.After clicking the dropdown (line 31), using
count()immediately (line 35) may return 0 if the dropdown options haven't fully rendered yet, causing the logic to incorrectly take the "Other" path even when the option exists.Per coding guidelines: "Use
waitFor()for guards in Page Objects". This check is a guard determining which path to take.🔎 Proposed fix to ensure dropdown is populated before checking
// Try to select from the dropdown first await this.localeSelect.click(); +// Wait for dropdown options to be visible +await this.page.getByTestId('select-option').first().waitFor({state: 'visible'}); + // Check if the language code exists in the dropdown options const optionLocator = this.page.getByTestId('select-option').filter({hasText: `(${language.trim()})`}); const optionExists = await optionLocator.count() > 0;apps/admin-x-settings/src/utils/locale-validation.ts (2)
86-95: Bug: Private use tag "x" alone incorrectly accepted as valid.When
segments = ["x"], the for loop never executes (sincei = 1andsegments.length = 1), and the function returnsnullindicating validity. Per BCP 47 (RFC 5646), private use tags require at least one subtag after "x" (format:x-subtag).🔎 Proposed fix
// Private use tags (x-something) if (segments[0].toLowerCase() === 'x') { + // Must have at least one subtag after 'x' + if (segments.length < 2) { + return errorMessage; + } // All remaining segments must be 1-8 alphanumeric characters for (let i = 1; i < segments.length; i++) {
164-173: Bug:indexOfdoesn't identify the current segment's position.Line 171 uses
segments.indexOf(segment)which returns the index of the first occurrence of that segment value, not the current iteration's position. For tags likeen-x-en, the secondenwould incorrectly match against index 0.Additionally, line 170's
segments.indexOf('x')is case-sensitive, but BCP 47 tags are case-insensitive. A tag likeen-X-customwould fail this check.🔎 Proposed fix using index-based iteration
- for (const segment of segments) { + for (let segIdx = 0; segIdx < segments.length; segIdx++) { + const segment = segments[segIdx]; // Skip grandfathered prefixes and private use if (['i', 'x', 'sgn', 'art', 'cel', 'no', 'zh'].includes(segment.toLowerCase())) { continue; } // Skip segments in private use (after 'x') - const xIndex = segments.indexOf('x'); - if (xIndex !== -1 && segments.indexOf(segment) > xIndex) { + const xIndex = segments.findIndex(s => s.toLowerCase() === 'x'); + if (xIndex !== -1 && segIdx > xIndex) { continue; }
🧹 Nitpick comments (1)
apps/admin-x-design-system/src/global/form/select-with-other.tsx (1)
134-134: Consider makingasyncprop configurable.The
asyncprop is hardcoded tofalsewhen rendering the Select component. If async select functionality is needed in the future, this would require modifying the component.🔎 Suggested enhancement
Allow the
asyncprop to be passed through from the parent viarestProps:return ( <Select {...selectProps} - async={false} error={error}Note: Since
asyncis part ofSelectProps, it's already available inrestPropsand will be applied via the spread operator. Simply removing the explicit override enables this functionality.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsxapps/admin-x-design-system/src/global/form/select-with-other.tsxapps/admin-x-design-system/src/index.tsapps/admin-x-settings/package.jsonapps/admin-x-settings/src/components/settings/general/publication-language.tsxapps/admin-x-settings/src/utils/locale-validation.tsapps/admin-x-settings/test/acceptance/general/publication-language.test.tsapps/admin-x-settings/test/unit/utils/locale-validation.test.tsapps/admin-x-settings/vite.config.mjse2e/helpers/pages/admin/settings/sections/publications-section.tse2e/tests/admin/settings/publication-language.test.tsghost/i18n/index.jsghost/i18n/lib/i18n.jsghost/i18n/lib/locale-data.jsonghost/i18n/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
- apps/admin-x-settings/package.json
- ghost/i18n/package.json
- ghost/i18n/index.js
- apps/admin-x-design-system/src/index.ts
- ghost/i18n/lib/locale-data.json
- apps/admin-x-settings/vite.config.mjs
- apps/admin-x-settings/test/acceptance/general/publication-language.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.tse2e/tests/admin/settings/publication-language.test.ts
e2e/helpers/pages/**/*.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/helpers/pages/**/*.ts: Page Objects should be located inhelpers/pages/directory
Expose locators aspublic readonlyin Page Objects when used with assertions
Page Object methods should use semantic names (e.g.,login()instead ofclickLoginButton())
UsewaitFor()for guards in Page Objects, never useexpect()in page objects
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.tse2e/tests/admin/settings/publication-language.test.ts
e2e/**/*.test.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/**/*.test.ts: Always follow ADRs in../adr/folder (ADR-0001: AAA pattern, ADR-0002: Page Objects)
Never use CSS/XPath selectors - only use semantic locators or data-testid
Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Test names should be lowercase and follow the format 'what is tested - expected outcome'
Each test should represent one scenario only - never mix multiple scenarios in a single test
Follow the AAA (Arrange, Act, Assert) pattern in test structure
Prefer semantic locators (getByRole,getByLabel,getByText) over test IDs and never use CSS selectors, XPath, nth-child, or class names
UsegetByTestId()only when semantic locators are unavailable, and suggest addingdata-testidto Ghost codebase when needed
Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Never use hard-coded waits likewaitForTimeout()
Never usenetworkidlein waits
Use Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Usetest.only()for debugging single tests
Manual login should not be used - authentication is automatic via fixture
Files:
e2e/tests/admin/settings/publication-language.test.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: For new UI components, prioritize comprehensive Storybook stories; add focused unit tests where they provide real value (e.g., hooks, utils, logic-heavy parts)
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/{portal,comments,signup-form,search}.json : Add UI translations to namespace-specific files in `ghost/i18n/locales/en/` for public UI apps (portal.json for Portal, comments.json for Comments UI, etc.)
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: When changing tokens/config, verify Storybook and a library build still succeed
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories should demonstrate key variants and states (sizes, disabled/loading, critical props) with minimal but representative examples
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories for new components should include an overview explaining what the component does and its primary use case
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: For new UI components, prioritize comprehensive Storybook stories; add focused unit tests where they provide real value (e.g., hooks, utils, logic-heavy parts)
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/ui/**/*.{ts,tsx} : Atomic UI components should be placed in `src/components/ui/*` and each component must have a corresponding `*.stories.tsx` file next to it for Storybook documentation
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
apps/admin-x-settings/src/utils/locale-validation.tsapps/admin-x-settings/src/components/settings/general/publication-language.tsxghost/i18n/lib/i18n.js
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.tsx
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use `getByTestId()` only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed
Applied to files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Expose locators as `public readonly` in Page Objects when used with assertions
Applied to files:
e2e/helpers/pages/admin/settings/sections/publications-section.tse2e/tests/admin/settings/publication-language.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Use `waitFor()` for guards in Page Objects, never use `expect()` in page objects
Applied to files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Centralized i18n translations should be maintained in `ghost/i18n/locales/{locale}/{namespace}.json` files with 60+ supported locales
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/{portal,comments,signup-form,search}.json : Add UI translations to namespace-specific files in `ghost/i18n/locales/en/` for public UI apps (portal.json for Portal, comments.json for Comments UI, etc.)
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-05-20T21:08:21.026Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-01-29T15:23:58.658Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 21982
File: ghost/i18n/locales/hu v2/portal.json:70-70
Timestamp: 2025-01-29T15:23:58.658Z
Learning: Typos and spelling errors within localization files (ghost/i18n/locales/*.json) must be treated as important problems, not nitpicks, as they directly affect the user experience for speakers of that language. These issues should be flagged with high priority during code review.
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-01-29T15:35:26.447Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 21651
File: ghost/i18n/locales/ar/comments.json:1-1
Timestamp: 2025-01-29T15:35:26.447Z
Learning: All JSON files containing translation strings in ghost/i18n/locales must include both opening and closing braces to maintain valid JSON structure.
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-05-14T14:39:09.136Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
Applied to files:
ghost/i18n/lib/i18n.js
🧬 Code graph analysis (4)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
apps/admin-x-design-system/src/index.ts (1)
SelectOption(46-46)
apps/admin-x-design-system/src/global/form/select-with-other.tsx (2)
apps/admin-x-design-system/src/index.ts (3)
SelectWithOtherProps(48-48)SelectProps(46-46)SelectOption(46-46)apps/shade/src/components/ui/select.tsx (1)
Select(148-148)
e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
Error(101-108)
e2e/tests/admin/settings/publication-language.test.ts (1)
e2e/helpers/playwright/fixture.ts (1)
expect(136-136)
🪛 Biome (2.1.2)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
[error] 101-101: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (5)
e2e/tests/admin/settings/publication-language.test.ts (1)
21-22: LGTM! Test correctly updated for new SelectWithOther UI.The assertion now validates the displayed dropdown text ("French (fr)") rather than the underlying field value, which correctly reflects the migration from TextField to SelectWithOther component.
apps/admin-x-settings/src/components/settings/general/publication-language.tsx (2)
34-41: Clean implementation of locale options with proper memoization.The locale options are correctly derived from LOCALE_DATA with appropriate memoization, ensuring the options array is only recreated when dependencies change.
1-1: The import path is correctly configured and will resolve without issues. The package.json explicitly exports./lib/locale-data.json, the file exists at the expected location, and LOCALE_DATA is re-exported from the main entry point as well, providing redundant access paths.ghost/i18n/lib/i18n.js (2)
153-153: Good addition of LOCALE_DATA export for enhanced i18n integration.Exporting LOCALE_DATA alongside SUPPORTED_LOCALES provides richer locale information (codes + labels) for UI components while maintaining the existing SUPPORTED_LOCALES export for backward compatibility.
7-11: Locale codes are preserved for backward compatibility.The refactored
SUPPORTED_LOCALESderived fromlocale-data.jsoncontains all the same locale codes as the previous hardcoded array. Backward compatibility is maintained—the set of supported locales remains unchanged. Note that the JSON structure alphabetically sorts the locales, which differs from the previous inline array order, but this does not affect functionality since the code iterates through the array rather than relying on specific index positions.
apps/admin-x-design-system/src/global/form/select-with-other.tsx
Outdated
Show resolved
Hide resolved
betschki
force-pushed
the
feature/select-with-other-publication-language
branch
from
2be929b to
4869c80
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
apps/admin-x-settings/src/utils/locale-validation.ts (2)
86-95: Verify that bare "x" is rejected.Per a previous review comment, BCP 47 requires at least one subtag after "x" (format:
x-subtag). Whensegments = ["x"], the for loop at line 89 never executes, and the function returnsnullindicating validity.Add a check to ensure
segments.length > 1before the loop:🔎 Proposed fix
// Private use tags (x-something) if (segments[0].toLowerCase() === 'x') { + if (segments.length < 2) { + return errorMessage; + } // All remaining segments must be 1-8 alphanumeric characters for (let i = 1; i < segments.length; i++) { if (segments[i].length < 1 || segments[i].length > 8) { return errorMessage; } } return null; }
164-185: Fix indexOf usage for correct segment position tracking.The loop uses
segments.indexOf(segment)which returns the index of the first occurrence of that value. For tags likeen-x-en, this would incorrectly match the secondenagainst index 0. Additionally,segments.indexOf('x')is case-sensitive and won't match uppercase 'X'.🔎 Proposed fix using index-based iteration
+ // Find the position of 'x' once (case-insensitive) + const xIndex = segments.findIndex(s => s.toLowerCase() === 'x'); + // Additional validation for common mistakes // Check if any segment looks like a full word rather than a code - for (const segment of segments) { + for (let i = 0; i < segments.length; i++) { + const segment = segments[i]; // Skip grandfathered prefixes and private use if (['i', 'x', 'sgn', 'art', 'cel', 'no', 'zh'].includes(segment.toLowerCase())) { continue; } // Skip segments in private use (after 'x') - const xIndex = segments.indexOf('x'); - if (xIndex !== -1 && segments.indexOf(segment) > xIndex) { + if (xIndex !== -1 && i > xIndex) { continue; } // Reject segments that are obviously too long (but be lenient for private use) if (segment.length > 16) { return errorMessage; } // Common English words that might be mistakenly used const invalidWords = ['english', 'french', 'spanish', 'german', 'italian', 'chinese', 'japanese', 'korean', 'russian', 'arabic', 'united', 'states', 'kingdom', 'traditional', 'simplified']; if (invalidWords.includes(segment.toLowerCase())) { return errorMessage; } }apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
101-108: Rename story to avoid shadowing global Error.The export name
Errorshadows the globalErrorconstructor. Consider renaming toErrorStateorWithError.🔎 Proposed fix
-export const Error: Story = { +export const ErrorState: Story = { args: { title: 'Site language', options: languageOptions, error: true, hint: 'Please select a valid language' } };e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
25-47: Potential race condition when checking option existence.After clicking the dropdown (line 31), using
count()immediately (line 35) may return 0 if the dropdown options haven't fully rendered yet. Per coding guidelines, "UsewaitFor()for guards in Page Objects".🔎 Proposed fix to ensure dropdown is populated
// Try to select from the dropdown first await this.localeSelect.click(); +// Wait for dropdown options to be visible +await this.page.getByTestId('select-option').first().waitFor({state: 'visible'}); + // Check if the language code exists in the dropdown options const optionLocator = this.page.getByTestId('select-option').filter({hasText: `(${language.trim()})`}); const optionExists = await optionLocator.count() > 0;apps/admin-x-design-system/src/global/form/select-with-other.tsx (1)
38-43: State synchronization issue confirmed.The past review correctly identified that
isOtherSelectedstate doesn't sync whenselectedValueprop changes externally. When the parent updatesselectedValuefrom a custom value to a predefined option,isInitiallyCustomValuerecalculates tofalse, butisOtherSelectedremainstrue, causingshowCustomInput(line 95) to incorrectly show the TextField instead of the Select dropdown.🔎 Proposed fix using useEffect
+ React.useEffect(() => { + setIsOtherSelected(isInitiallyCustomValue); + }, [isInitiallyCustomValue]); + const [isOtherSelected, setIsOtherSelected] = useState(isInitiallyCustomValue); const [validationError, setValidationError] = useState<string | null>(null);
🧹 Nitpick comments (2)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
8-18: Consider adding a component overview in the meta.Per coding guidelines, Storybook stories should include an overview explaining what the component does and its primary use case. Consider adding a
parameters.docs.description.componentfield to document that SelectWithOther provides a dropdown with an "Other" option that switches to a text input.Based on coding guidelines.
apps/admin-x-design-system/src/global/form/select-with-other.tsx (1)
97-100: Consider clarifying error/hint handling.The logic at line 99 reuses
hintas error text whenvalidationErroris absent. While this pattern works (wherehintcan contain either helpful text or error messages depending on theerrorboolean), the intent isn't immediately clear. Consider adding a comment explaining this dual-purpose usage, or separating error message and hint props more explicitly.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsxapps/admin-x-design-system/src/global/form/select-with-other.tsxapps/admin-x-design-system/src/index.tsapps/admin-x-settings/package.jsonapps/admin-x-settings/src/components/settings/general/publication-language.tsxapps/admin-x-settings/src/utils/locale-validation.tsapps/admin-x-settings/test/acceptance/general/publication-language.test.tsapps/admin-x-settings/test/unit/utils/locale-validation.test.tse2e/helpers/pages/admin/settings/sections/publications-section.tse2e/tests/admin/settings/publication-language.test.tsghost/i18n/index.jsghost/i18n/lib/i18n.jsghost/i18n/lib/locale-data.jsonghost/i18n/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/i18n/package.json
- ghost/i18n/lib/locale-data.json
- apps/admin-x-settings/package.json
- e2e/tests/admin/settings/publication-language.test.ts
- ghost/i18n/index.js
🧰 Additional context used
📓 Path-based instructions (3)
e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (e2e/AGENTS.md)
Prefer less comments and give things clear names
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
e2e/helpers/pages/**/*.ts
📄 CodeRabbit inference engine (e2e/AGENTS.md)
e2e/helpers/pages/**/*.ts: Page Objects should be located inhelpers/pages/directory
Expose locators aspublic readonlyin Page Objects when used with assertions
Page Object methods should use semantic names (e.g.,login()instead ofclickLoginButton())
UsewaitFor()for guards in Page Objects, never useexpect()in page objects
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
e2e/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
E2E tests should use Playwright with Docker container isolation
Files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
🧠 Learnings (26)
📓 Common learnings
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test suite names should follow the format 'Ghost Admin - Feature' or 'Ghost Public - Feature'
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/test/unit/**/*.test.{ts,tsx,js} : Vitest unit tests should be located in `test/unit/*` with pattern `test/unit/**/*.(test).(ts|tsx|js)` and use the `render` helper from `test/unit/utils/test-utils.tsx`
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Test names should be lowercase and follow the format 'what is tested - expected outcome'
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-11-24T11:12:15.712Z
Learnt from: sagzy
Repo: TryGhost/Ghost PR: 24673
File: ghost/i18n/lib/i18n.js:34-35
Timestamp: 2025-11-24T11:12:15.712Z
Learning: In the Ghost i18n package (ghost/i18n/lib/i18n.js), changing existing locale codes requires backwards compatibility handling for users who have already configured those locales. Such changes should be done in a separate PR with migration logic rather than included in feature PRs.
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.tsapps/admin-x-settings/src/components/settings/general/publication-language.tsxapps/admin-x-settings/src/utils/locale-validation.tsghost/i18n/lib/i18n.jsapps/admin-x-settings/test/acceptance/general/publication-language.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-03-13T09:00:20.205Z
Learnt from: mike182uk
Repo: TryGhost/Ghost PR: 22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.tsapps/admin-x-settings/test/acceptance/general/publication-language.test.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Always follow ADRs in `../adr/` folder (ADR-0001: AAA pattern, ADR-0002: Page Objects)
Applied to files:
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
Applied to files:
apps/admin-x-design-system/src/index.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/**/*.{ts,tsx,js} : Use the `@` alias for internal imports (e.g., `@/lib/utils`)
Applied to files:
apps/admin-x-design-system/src/index.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.tsx
📚 Learning: 2025-11-06T05:35:41.162Z
Learnt from: danielraffel
Repo: TryGhost/Ghost PR: 25366
File: apps/admin/src/layout/app-sidebar/NavHeader.tsx:13-23
Timestamp: 2025-11-06T05:35:41.162Z
Learning: In apps/admin/src/layout/app-sidebar/NavHeader.tsx, the React component dispatches a synthetic KeyboardEvent to trigger the Ember keymaster.js search modal shortcut. This approach is known to have cross-browser reliability issues but was deferred for architectural refactoring in a separate PR. The recommended fix is to expose a global function or custom DOM event from the Ember app instead of relying on synthetic keyboard events with keymaster.js.
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.tsx
📚 Learning: 2025-12-16T12:24:41.848Z
Learnt from: jonatansberg
Repo: TryGhost/Ghost PR: 25725
File: apps/shade/src/components/ui/filters.tsx:1227-1233
Timestamp: 2025-12-16T12:24:41.848Z
Learning: In apps/shade/src/components/ui/filters.tsx, when displaying a single selected option with detail in the filter list (around line 1227), the label and detail are intentionally rendered horizontally as sibling spans rather than stacked vertically. A stacked layout would break the filter list UI alignment. This is different from the dropdown/popover contexts where vertical stacking is used.
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.tsx
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/**/*.test.ts : Use `getByTestId()` only when semantic locators are unavailable, and suggest adding `data-testid` to Ghost codebase when needed
Applied to files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Expose locators as `public readonly` in Page Objects when used with assertions
Applied to files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
📚 Learning: 2025-11-24T17:29:43.865Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.865Z
Learning: Applies to e2e/helpers/pages/**/*.ts : Use `waitFor()` for guards in Page Objects, never use `expect()` in page objects
Applied to files:
e2e/helpers/pages/admin/settings/sections/publications-section.ts
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories for new components should include an overview explaining what the component does and its primary use case
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/**/*.stories.{ts,tsx} : Storybook stories should demonstrate key variants and states (sizes, disabled/loading, critical props) with minimal but representative examples
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: For new UI components, prioritize comprehensive Storybook stories; add focused unit tests where they provide real value (e.g., hooks, utils, logic-heavy parts)
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/ui/**/*.{ts,tsx} : Atomic UI components should be placed in `src/components/ui/*` and each component must have a corresponding `*.stories.tsx` file next to it for Storybook documentation
Applied to files:
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Centralized i18n translations should be maintained in `ghost/i18n/locales/{locale}/{namespace}.json` files with 60+ supported locales
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/{portal,comments,signup-form,search}.json : Add UI translations to namespace-specific files in `ghost/i18n/locales/en/` for public UI apps (portal.json for Portal, comments.json for Comments UI, etc.)
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
ghost/i18n/lib/i18n.jsapps/admin-x-settings/test/acceptance/general/publication-language.test.ts
📚 Learning: 2025-01-29T15:23:58.658Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 21982
File: ghost/i18n/locales/hu v2/portal.json:70-70
Timestamp: 2025-01-29T15:23:58.658Z
Learning: Typos and spelling errors within localization files (ghost/i18n/locales/*.json) must be treated as important problems, not nitpicks, as they directly affect the user experience for speakers of that language. These issues should be flagged with high priority during code review.
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-01-29T15:35:26.447Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 21651
File: ghost/i18n/locales/ar/comments.json:1-1
Timestamp: 2025-01-29T15:35:26.447Z
Learning: All JSON files containing translation strings in ghost/i18n/locales must include both opening and closing braces to maintain valid JSON structure.
Applied to files:
ghost/i18n/lib/i18n.js
📚 Learning: 2025-05-14T14:39:09.136Z
Learnt from: cathysarisky
Repo: TryGhost/Ghost PR: 23161
File: ghost/i18n/lib/i18n.js:107-144
Timestamp: 2025-05-14T14:39:09.136Z
Learning: In Ghost's i18n system, translation keys are the actual English phrases, so when a translation is missing, displaying the key itself serves as the English fallback, eliminating the need for explicit fallback resource loading.
Applied to files:
ghost/i18n/lib/i18n.js
🧬 Code graph analysis (6)
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts (1)
apps/admin-x-settings/src/utils/locale-validation.ts (1)
validateLocale(6-188)
apps/admin-x-design-system/src/global/form/select-with-other.tsx (1)
apps/admin-x-design-system/src/index.ts (3)
SelectWithOtherProps(48-48)SelectProps(46-46)SelectOption(46-46)
e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
Error(101-108)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
apps/admin-x-design-system/src/index.ts (1)
SelectOption(46-46)
ghost/i18n/lib/i18n.js (1)
ghost/core/core/server/services/i18n.js (1)
locale(12-12)
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts (1)
apps/admin-x-framework/src/test/acceptance.ts (3)
mockApi(211-272)globalDataRequests(161-166)updatedSettingsResponse(274-287)
🪛 Biome (2.1.2)
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx
[error] 101-101: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (16)
ghost/i18n/lib/i18n.js (2)
7-11: LGTM! Locale data centralization improves maintainability.The refactoring to load
LOCALE_DATAfrom a JSON file and deriveSUPPORTED_LOCALESfrom it is a solid improvement. This approach maintains backward compatibility while providing a single source of truth for locale information.
153-153: Good addition of LOCALE_DATA export.Exposing
LOCALE_DATAallows external consumers to access both locale codes and labels, supporting the UI changes in admin-x-settings.apps/admin-x-design-system/src/index.ts (1)
47-48: LGTM! Export follows design system conventions.The SelectWithOther component and its props type are correctly exported, consistent with the pattern used for other form components in the design system.
apps/admin-x-settings/test/unit/utils/locale-validation.test.ts (1)
4-122: Excellent test coverage for locale validation.The test suite comprehensively exercises the
validateLocalefunction with a wide range of valid and invalid BCP 47 locale formats. The test structure follows best practices with clear describe blocks and descriptive test names.apps/admin-x-settings/src/components/settings/general/publication-language.tsx (2)
34-41: Well-structured option generation with proper memoization.The
localeOptionsmemo correctly transformsLOCALE_DATAintoSelectOptionformat and avoids unnecessary recalculations.
72-85: SelectWithOther integration is well-configured.The component configuration correctly includes:
- Default list value for fallback
- Locale validation via
validateLocale- Custom input hints and placeholder
- Searchability for better UX
apps/admin-x-settings/test/acceptance/general/publication-language.test.ts (5)
5-35: LGTM! Dropdown selection test correctly updated.The test properly verifies:
- Initial display of the default locale
- Dropdown interaction and option selection
- Visual confirmation of the selection
- API payload validation
37-64: Good test coverage for custom locale entry.The test correctly exercises the "Other" option workflow, verifying that custom locale codes can be entered and saved.
66-85: Validation error handling tested appropriately.The test confirms that invalid locales trigger the expected validation error message.
87-106: Good test for mode switching behavior.This test ensures users can navigate back from custom input to the dropdown, covering a key usability scenario.
108-126: Correctly addresses previous review feedback.The test now uses the regex path pattern
/^\/settings\/\?group=/to properly match the settings request with query parameters, as recommended in the previous review.apps/admin-x-settings/src/utils/locale-validation.ts (1)
1-187: Comprehensive BCP 47 validation with excellent documentation.The validation logic is thorough and well-documented, covering:
- Grandfathered tags (both irregular and regular)
- Private use tags
- Language, script, region, variant, and extension subtags
- Common user mistakes
The inline comments explaining IANA registry compliance and the rationale for each check are particularly valuable.
e2e/helpers/pages/admin/settings/sections/publications-section.ts (1)
8-9: Locators correctly exposed per coding guidelines.The page object properly exposes
localeSelectandcustomLanguageFieldas properties, following the guideline to expose locators aspublic readonlyin Page Objects.Based on coding guidelines.
Also applies to: 16-17
apps/admin-x-design-system/src/global/form/select-with-other.stories.tsx (1)
34-127: Excellent story coverage demonstrating key variants.The stories comprehensively cover:
- Default state
- Selected values
- Hints and validation
- Searchable mode
- Custom values not in the list
- Error and disabled states
- Custom labels
This provides strong documentation for the component's capabilities.
Based on coding guidelines for comprehensive Storybook coverage.
apps/admin-x-design-system/src/global/form/select-with-other.tsx (2)
1-16: Interface design looks solid.The props interface appropriately extends
SelectPropswhile omitting conflicting props and adding necessary customization options for the "Other" behavior.
101-143: Conditional rendering logic is sound.The component correctly switches between TextField and Select based on
showCustomInput. The back-to-list button appropriately resets state and selects a fallback value. Once the state synchronization issue is addressed, this rendering logic should work correctly for all scenarios.
|
@coderabbitai no more reviews please |
|
@betschki: Understood! I've paused automatic reviews for this PR. You can resume them anytime by mentioning me with a resume request. 🧠 Learnings used✅ Actions performedReviews paused. |
betschki
force-pushed
the
feature/select-with-other-publication-language
branch
from
4869c80 to
00d9dff
Compare
ref TryGhost#24673 - Added SelectWithOther component to admin-x-design-system that provides a dropdown with an "Other..." option that switches to a text input - Added locale validation utility with support for BCP 47 language tags - Updated PublicationLanguage to use SelectWithOther for better UX - Added LOCALE_DATA to @tryghost/i18n with human-readable labels - Created ESM entry point for i18n to support admin-x apps imports - Added comprehensive unit and E2E tests
betschki
force-pushed
the
feature/select-with-other-publication-language
branch
from
00d9dff to
96dbcdc
Compare
apps/admin-x-settings/src/components/settings/general/publication-language.tsx
Show resolved
Hide resolved
- Fixed locale-validation indexOf bugs for case-insensitive 'x' detection - Fixed stale isInitiallyCustomValue using useMemo for reactivity - Renamed Error story to ErrorState to avoid shadowing global - Standardized validation error messages to 'Enter a value'
- Reject private use tag 'x' without subtags - Enforce locale format validation on save - Fix race condition in E2E dropdown option check
| title={title} | ||
| value={selectedValue} | ||
| onChange={handleTextChange} | ||
| /> |
There was a problem hiding this comment.
TextField missing disabled prop in custom input mode
The disabled prop is passed to Select via the spread {...selectProps} but is never passed to TextField when rendering the custom input mode. If the component is initialized with a custom value (not in predefined options) while disabled is true, the text input remains editable. The disabled prop needs to be extracted from restProps and explicitly passed to TextField.
1 participant
ref #24673 − since the old PR was quite outdated, it was easier to close that and start fresh. Most changes have been migrated here, though the
LOCALE_DATAhas been moved to a static JSON due to CommonJS export/import issues.Note
Enhances publication language UX with a searchable dropdown and custom value support, backed by centralized i18n locale data.
SelectWithOtherin admin-x-design-system; Storybook addedPublicationLanguageswitched from text field toSelectWithOther, sourcing options from@tryghost/i18n/lib/locale-data.json; introducesvalidateLocale(BCP 47)@tryghost/i18n: addslib/locale-data.json, exportsLOCALE_DATA, updates CommonJS exports andpackage.jsonexportsghost/i18nin relevant build stages (settings/react) for admin buildsWritten by Cursor Bugbot for commit 9adb2db. This will update automatically on new commits. Configure here.