Skip to content

fix: rtl edge cases#1061

Merged
ErnestM1234 merged 10 commits intomainfrom
e/core/rtl-direction-fallback
Mar 4, 2026
Merged

fix: rtl edge cases#1061
ErnestM1234 merged 10 commits intomainfrom
e/core/rtl-direction-fallback

Conversation

@ErnestM1234
Copy link
Contributor

@ErnestM1234 ErnestM1234 commented Mar 3, 2026

This issue was first pointed out in https://discord.com/channels/1337588892672725085/1477045064327758039 by user ibr.lol95 who noticed that getLocaleDirection("ar-EG") was incorrectly outputting "ltr". We narrowed this behavior down to browser environments only. This is caused by an older version of the Intl in browser environments. We had two options here which is to fix via polyfill or by manually checking for rtl languages. We decided to go with the latter to avoid potential side effects.

Greptile Summary

This PR fixes a real browser-environment bug where getLocaleDirection("ar-EG") (and similar region-tagged Arabic locales) incorrectly returned "ltr" because older browser Intl implementations do not expose the textInfo property on Intl.Locale. The fix introduces a three-tier fallback strategy: prefer textInfo when available, then resolve via the maximized script code (from _getLocaleProperties), and finally fall back to a hardcoded RTL_LANGUAGES set. New test files correctly simulate both a browser-without-textInfo environment and a Node.js environment.

  • Bug fix validated: For ar-EG without textInfo, _getLocaleProperties calls maximize() to discover the Arab script, which isRtlScript correctly identifies as RTL.
  • RTL_SCRIPTS is slightly incomplete: yezi (Yezidi — an actively-used living script) is absent, meaning locales with an explicit Yezidi script tag would fall through to a language-based check. See inline comment.
  • Redundant truthy guard in extractDirectionWithTextInfo (line 85) can be removed without changing behavior.
  • _getLocaleProperties as the fallback vehicle is a heavyweight call (creates DisplayNames objects, calls maximize() and minimize()) just to obtain scriptCode and languageCode. This is only triggered when textInfo is unavailable so the practical performance impact is low, but a lighter helper would be cleaner long-term.
  • Misleading test variable name (browserLocale) in the Node-environment test file — minor readability issue.

Confidence Score: 4/5

  • Safe to merge; the core bug fix is correct and well-tested. Minor completeness gaps in the RTL script list and a redundant guard do not affect correctness for the reported issue.
  • The layered fallback logic is sound and correctly resolves the originally-reported ar-EG regression in browsers without textInfo. The heuristic sets are reasonable for practical locales. No debug logging remains in production code. The small issues found (missing yezi script, redundant truthy check, misleading variable name) are all style-level and do not break any tested behavior.
  • Pay close attention to packages/core/src/locales/getLocaleDirection.ts — specifically the completeness of RTL_SCRIPTS.

Important Files Changed

Filename Overview
packages/core/src/locales/getLocaleDirection.ts Core fix: replaces the single textInfo check with a layered strategy (textInfo → script heuristic → language heuristic). Logic is correct; the RTL_SCRIPTS set is missing a handful of active RTL scripts, and the extractDirectionWithTextInfo guard contains one redundant truthy check.
packages/core/src/locales/tests/getLocaleDirection.browser.test.ts New browser simulation tests cover textInfo-stripped Intl.Locale and full Intl.Locale absence; missing a test for the explicit-script-in-locale case (e.g. ar-EG) when Intl.Locale is completely unavailable.
packages/core/src/locales/tests/getLocaleDirection.node.test.ts New Node.js tests correctly verify textInfo is preferred over script heuristics; test variable named browserLocale is misleading in a node-environment file but otherwise correct.
packages/core/src/locales/tests/getLocaleDirection.test.ts Minor cleanup removing stale comments; existing test coverage for ltr/rtl, casing, edge cases, and the originally failing ar-EG case is preserved.
.changeset/calm-spies-build.md Changeset entry looks correct; previously-noted spelling error "compatability" is fixed in the current revision.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["_getLocaleDirection(code)"] --> B["intlCache.get('Locale', code)"]
    B -->|throws| C["silent catch"]
    B -->|success| D["extractDirectionWithTextInfo(locale)"]
    D -->|"textInfo present & valid ('ltr'|'rtl')"| E["return textInfo.direction"]
    D -->|"textInfo absent or invalid"| F["fall through"]
    C --> F
    F --> G["_getLocaleProperties(code)"]
    G --> H{scriptCode?}
    H -->|non-empty| I["isRtlScript(scriptCode)"]
    I -->|true| J["return 'rtl'"]
    I -->|false| K["return 'ltr'"]
    H -->|empty| L{languageCode?}
    L -->|non-empty| M["isRtlLanguage(languageCode)"]
    M -->|true| N["return 'rtl'"]
    M -->|false| O["return 'ltr'"]
    L -->|empty| P["return 'ltr'"]
Loading

Last reviewed commit: 9aa338c

@ErnestM1234 ErnestM1234 requested a review from a team as a code owner March 3, 2026 18:12
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 files reviewed, 8 comments

Edit Code Review Agent Settings | Greptile

@ErnestM1234
Copy link
Contributor Author

@greptileai rereview plz

@ErnestM1234
Copy link
Contributor Author

@greptileai plz re-review

@ErnestM1234
Copy link
Contributor Author

@greptileai plz re-review

ErnestM1234 and others added 2 commits March 3, 2026 11:32
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@ErnestM1234
Copy link
Contributor Author

@greptileai plz review

ErnestM1234 and others added 3 commits March 4, 2026 10:21
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@ErnestM1234 ErnestM1234 enabled auto-merge (squash) March 4, 2026 18:28
@ErnestM1234 ErnestM1234 merged commit 21b3304 into main Mar 4, 2026
21 checks passed
@ErnestM1234 ErnestM1234 deleted the e/core/rtl-direction-fallback branch March 4, 2026 18:29
@github-actions github-actions bot mentioned this pull request Mar 4, 2026
ErnestM1234 pushed a commit that referenced this pull request Mar 4, 2026
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## gtx-cli@2.6.29

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13

## @generaltranslation/compiler@1.1.22

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13

## generaltranslation@8.1.13

### Patch Changes

- [#1061](#1061)
[`21b3304`](21b3304)
Thanks [@ErnestM1234](https://github.com/ErnestM1234)! - fix: language
direction browser compatibility

## gt-i18n@0.4.1

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13
    -   @generaltranslation/supported-locales@2.0.46

## locadex@1.0.107

### Patch Changes

-   Updated dependencies \[]:
    -   gtx-cli@2.6.29

## gt-next@6.13.2

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13
    -   @generaltranslation/compiler@1.1.22
    -   gt-i18n@0.4.1
    -   gt-react@10.11.1
    -   @generaltranslation/supported-locales@2.0.46

## @generaltranslation/gt-next-lint@11.0.2

### Patch Changes

-   Updated dependencies \[]:
    -   gt-next@6.13.2

## gt-node@0.2.7

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13
    -   gt-i18n@0.4.1

## gt-react@10.11.1

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13
    -   @generaltranslation/react-core@1.5.1
    -   @generaltranslation/supported-locales@2.0.46

## @generaltranslation/react-core@1.5.1

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13
    -   gt-i18n@0.4.1
    -   @generaltranslation/supported-locales@2.0.46

## gt-sanity@1.1.20

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13

## @generaltranslation/supported-locales@2.0.46

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13

## gt-tanstack-start@0.1.8

### Patch Changes

- Updated dependencies
\[[`21b3304`](21b3304)]:
    -   generaltranslation@8.1.13
    -   gt-i18n@0.4.1
    -   gt-react@10.11.1
    -   @generaltranslation/react-core@1.5.1

## gt-next-middleware-e2e@0.1.2

### Patch Changes

-   Updated dependencies \[]:
    -   gt-next@6.13.2

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants