🐛 Fixed data-locale being ignored in Portal#25190
Conversation
kevinansfield
changed the title
data-locale being overridden with site localedata-locale being ignored in Portal
WalkthroughRemoved runtime language initialization calls to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/portal/src/tests/data-attributes.test.js (1)
1-407: Consider adding test coverage for the data-locale fix.While the author's rationale for not testing i18n directly is valid (the code no longer calls
i18n.changeLanguage), the core bug being fixed—thatdata-localeattributes were being ignored—remains untested. Consider adding integration tests to verify:
- Portal respects a
data-localeattribute when present on the script tag- Portal falls back to site locale when
data-localeis absent- Custom forms honor the
data-localeoverride (addressing the preexisting bug mentioned in the PR)These tests would validate the actual user-facing behavior rather than implementation details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 96c5399 and e6ac05549bb769d95f2c45b6b94127f405edb691.
📒 Files selected for processing (2)
apps/portal/src/data-attributes.js(0 hunks)apps/portal/src/tests/data-attributes.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- apps/portal/src/data-attributes.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T07:53:49.814Z
Learnt from: CR
PR: TryGhost/Ghost#0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-10-15T07:53:49.814Z
Learning: Use Vitest + Testing Library + jsdom for tests; prioritize focused unit tests for hooks/utils/logic-heavy parts
Applied to files:
apps/portal/src/tests/data-attributes.test.js
⏰ 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). (2)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Build & Push
🔇 Additional comments (1)
apps/portal/src/tests/data-attributes.test.js (1)
6-6: LGTM: Import correctly updated.The removal of
handleDataAttributesfrom the import statement aligns with its removal from the production code.
kevinansfield
force-pushed
the
fix-data-attributes-locale-override
branch
from
e6ac055 to
316ee46
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/portal/src/data-attributes.js (1)
207-418: Re-add and update tests for handleDataAttributes
handleDataAttributes remains exported and used in apps/portal/src/App.js, so reintroduce tests covering its event-listener setup (forms, plans, billing, sign-out, subscriptions). Remove or adjust any tests fordata-localeoverrides, as that support was removed.
🧹 Nitpick comments (1)
apps/portal/src/tests/data-attributes.test.js (1)
6-6: Consider adding tests for thedata-localeoverride fix.While removing tests for language-change logic is appropriate given the refactor, the PR objectives mention that no tests were added for the
data-localeoverride edge case (the core bug fix). Consider adding tests to verify that:
- When
data-localeis specified on the Portal script tag, forms honor that locale instead of falling back to the site locale- The
t()function returns translated strings in the correct locale based on thedata-localeattribute- Custom forms respect the locale override (the preexisting bug mentioned in the PR objectives)
This would help prevent regression of the locale override functionality and provide confidence that the fix works as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e6ac05549bb769d95f2c45b6b94127f405edb691 and 316ee4662963641e7f73ad9921d69c82e01b3aad.
📒 Files selected for processing (2)
apps/portal/src/data-attributes.js(1 hunks)apps/portal/src/tests/data-attributes.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T07:53:49.814Z
Learnt from: CR
PR: TryGhost/Ghost#0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-10-15T07:53:49.814Z
Learning: Use Vitest + Testing Library + jsdom for tests; prioritize focused unit tests for hooks/utils/logic-heavy parts
Applied to files:
apps/portal/src/tests/data-attributes.test.js
⏰ 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). (4)
- GitHub Check: E2E Tests (Shard 2/2)
- GitHub Check: Inspect Docker Image
- GitHub Check: E2E Tests (Shard 1/2)
- GitHub Check: Unit tests (Node 22.13.1)
🔇 Additional comments (1)
apps/portal/src/data-attributes.js (1)
4-4: Approve removal ofi18nimport; no remainingi18nreferences found.
no issue - when we updated `data-attributes.js` to use the importable `t` function that uses a single instance of the `i18n` library we inadvertently introduced an override of the locale so it always matches the site locale despite using a `data-locale=` attribute in Portal's `<script>` tag - ultimately this uncovered a long-standing bug from before the switch to importable `t` where custom forms would always use the site locale rather than a data-attribute override - removed the now-unnecessary language change calls along with any tests for them - added test to ensure all calls to `changeLanguage` that occur during App startup use the prop rather than site locale
kevinansfield
force-pushed
the
fix-data-attributes-locale-override
branch
from
316ee46 to
53dea1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/portal/src/tests/App.test.js (2)
25-48: Good refactor with the setupApi helper.The helper function effectively reduces test duplication by parameterizing site and member fixtures. The implementation correctly merges custom fixtures with defaults and returns a properly mocked ghostApi.
Consider extracting this helper to
test-utils.jsif it's likely to be reused across other test files:// In apps/portal/src/utils/test-utils.js export const setupMockApi = ({site = {}, member = {}, siteUrl = 'http://example.com'} = {}) => { const defaultSite = FixtureSite.singleTier.basic; const defaultMember = FixtureMember.free; const siteFixtures = { ...defaultSite, ...site }; const memberFixtures = { ...defaultMember, ...member }; const ghostApi = setupGhostApi({siteUrl}); ghostApi.init = vi.fn(() => { return Promise.resolve({ site: siteFixtures, member: memberFixtures }); }); return ghostApi; };
65-81: Validates the PR objective effectively.This test correctly verifies that the
localeprop takes precedence over the site locale by asserting allchangeLanguagecalls use the prop value.Consider adding an explicit assertion that
changeLanguagewas called at least once to prevent the test from passing vacuously if the function is never called:i18n.changeLanguage.mock.calls.forEach((call) => { expect(call[0]).toBe('en'); }); +expect(i18n.changeLanguage).toHaveBeenCalled();Alternatively, use a more idiomatic matcher:
-i18n.changeLanguage.mock.calls.forEach((call) => { - expect(call[0]).toBe('en'); -}); +expect(i18n.changeLanguage).toHaveBeenCalledWith('en'); +i18n.changeLanguage.mock.calls.forEach((call) => { + expect(call[0]).toBe('en'); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 316ee4662963641e7f73ad9921d69c82e01b3aad and 53dea1e.
📒 Files selected for processing (3)
apps/portal/src/data-attributes.js(1 hunks)apps/portal/src/tests/App.test.js(2 hunks)apps/portal/src/tests/data-attributes.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/portal/src/tests/data-attributes.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/portal/src/data-attributes.js
🧰 Additional context used
🧬 Code graph analysis (1)
apps/portal/src/tests/App.test.js (1)
apps/portal/src/utils/test-utils.js (4)
utils(34-34)utils(45-45)appRender(42-50)appRender(42-50)
⏰ 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). (2)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Build & Push
🔇 Additional comments (2)
apps/portal/src/tests/App.test.js (2)
5-15: LGTM! Comprehensive i18n mock setup.The mock covers both the default export (full i18n object) and the named export (t function), which aligns with the dual import pattern used elsewhere in the codebase. The mock implementations are appropriate for these tests.
50-63: LGTM! Cleaner test with the helper.The test now benefits from the setupApi helper, making it more maintainable. The explicit
siteUrlprop improves clarity.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 18567214516 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 participant
no issue
data-attributes.jsto use the importabletfunction that uses a single instance of thei18nlibrary we inadvertently introduced an override of the locale so it always matches the site locale despite using adata-locale=attribute in Portal's<script>tagtwhere custom forms would always use the site locale rather than a data-attribute overridechangeLanguagethat occur during App startup use the prop rather than site locale