Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis pull request refactors the Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| @@ -1,201 +1,4 @@ | |||
| import type { Either } from '@lokalise/node-core' | |||
There was a problem hiding this comment.
Most of the changes in this PR are about splitting this big index file
| export const getLocaleDirection = (tag: Locale): LanguageDirection | null => { | ||
| try { | ||
| const locale = new Intl.Locale(tag) | ||
| const info = locale.getTextInfo?.() ?? locale.textInfo | ||
| return info?.direction ?? null | ||
| } catch (_) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
This is the newly added method.
I am not sure if it is ok to use getTextInfo (doc), the alternative is to define a constant with the RTL languages but I don't know if it is a good idea.
@szymonchudy @kibertoad what do you think?
There was a problem hiding this comment.
I’ve looked into it a bit, and I don’t think we should rely on getTextInfo. It still isn’t consistently supported across environments, and the behavior may vary.
I think the other solution we discussed could work - using a list of known RTL languages.
Alternatively, we could combine both approaches, falling back to the list when textInfo is not available.
There was a problem hiding this comment.
We have the list of supported languages, so we will never fallback into textInfo. I will change the approach to use the list of languages, Claude should do it easily by reviewing our supported languages
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/app/supported-languages/src/constants/index.spec.ts (1)
10-38: Accessor tests are too weak to protect against data regressions.
length > 0won’t catch missing entries. Prefer asserting full equality with the source constants (or at least exact size + known sentinel entries).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/supported-languages/src/constants/index.spec.ts` around lines 10 - 38, Replace the weak "length > 0" tests with strict assertions that validate the returned arrays exactly (or at minimum check exact expected length plus known sentinel entries) for each accessor: getAllLanguages, getAllRegions, getAllScripts, getStandardLocales, and getLokaliseSupportedLanguagesAndLocales; locate the canonical source constants used by these accessors and assert deep equality (or assert length === <expected> and includes('<known-sentinel>')) so tests will catch missing or mutated entries.packages/app/supported-languages/src/locale-lookup.ts (1)
33-33: Misleading comment: describes the wrong mapping direction.The comment says "languages to common regions" but this function actually maps regions to languages.
✏️ Suggested fix
- // Create a mapping of languages to common regions up front + // Create a mapping of regions to common languages up front🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/supported-languages/src/locale-lookup.ts` at line 33, The comment in locale-lookup.ts currently reads "languages to common regions" but the code actually creates a mapping from regions to languages; update the comment above the mapping creation to accurately state "regions to common languages" (or similar) so it reflects the mapping direction used by the mapping constant (e.g., the regions-to-languages map in locale-lookup.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/supported-languages/src/constants/lokalise-languages.ts`:
- Line 2: The public export lokaliseSupportedLanguagesAndLocales was changed
from an Array to a Set and that breaks consumers; revert the exported symbol to
the original Array (e.g., make lokaliseSupportedLanguagesAndLocales a plain
array again) and introduce a new Set export for internal/optimized usage (e.g.,
lokaliseSupportedLanguagesAndLocalesSet) so callers depending on array semantics
remain compatible; update any internal usages to switch to the new Set export
where O(1) lookup is needed and keep export names stable.
In `@packages/app/supported-languages/src/constants/regions.ts`:
- Line 11: The export named regions should keep its original array shape to
avoid breaking downstream code that expects an array; change back so regions is
an array (e.g., export const regions = [...]) and add a new Set-based export for
set usage (e.g., export const regionsSet = new Set(regions)); update any
internal references in this file from regions to regionsSet where the Set
behavior is required and keep the public export name regions as an array.
In `@packages/app/supported-languages/src/constants/rtl-languages.ts`:
- Around line 14-18: The rtl-language constant includes non-default-RTL entries
(notably 'ha' and 'ku') causing getLocaleDirection(...) to return incorrect
directions; remove 'ha' and 'ku' from the RTL list in the rtl-languages.ts
constant (the array exported as the default RTL languages used by
getLocaleDirection) and keep this array strictly to languages that are RTL by
default while handling script-specific RTL via script subtags elsewhere in
getLocaleDirection.
In `@packages/app/supported-languages/src/locale.ts`:
- Around line 60-64: The JSDoc for parseLocale incorrectly states it throws a
RangeError; update the comment for the parseLocale function (in locale.ts) to
remove the `@throws` tag and instead document the actual return shape: that it
returns an Either/Result-like object with either a parsed locale value or an
error string/object on the error property, and briefly describe the conditions
that populate the error field (e.g., structurally invalid or unsupported
values).
In `@packages/app/supported-languages/vitest.config.ts`:
- Line 13: The coverage exclusion pattern in vitest.config.ts currently excludes
runtime-containing barrel files via exclude: ['src/**/index.ts']; update this to
only exclude pure top-level barrels so runtime modules like
getAllLanguages/getAllRegions/getAllScripts in files such as
src/constants/index.ts remain covered — replace the broad pattern with a
specific one (e.g., 'src/index.ts') or an equivalently narrow pattern; modify
the exclude array where it is defined in the config to reference the new
pattern.
---
Nitpick comments:
In `@packages/app/supported-languages/src/constants/index.spec.ts`:
- Around line 10-38: Replace the weak "length > 0" tests with strict assertions
that validate the returned arrays exactly (or at minimum check exact expected
length plus known sentinel entries) for each accessor: getAllLanguages,
getAllRegions, getAllScripts, getStandardLocales, and
getLokaliseSupportedLanguagesAndLocales; locate the canonical source constants
used by these accessors and assert deep equality (or assert length ===
<expected> and includes('<known-sentinel>')) so tests will catch missing or
mutated entries.
In `@packages/app/supported-languages/src/locale-lookup.ts`:
- Line 33: The comment in locale-lookup.ts currently reads "languages to common
regions" but the code actually creates a mapping from regions to languages;
update the comment above the mapping creation to accurately state "regions to
common languages" (or similar) so it reflects the mapping direction used by the
mapping constant (e.g., the regions-to-languages map in locale-lookup.ts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dee77ad-a4f5-4e51-a6ad-3f4af7fd18a3
📒 Files selected for processing (22)
packages/app/supported-languages/package.jsonpackages/app/supported-languages/src/constants/index.spec.tspackages/app/supported-languages/src/constants/index.tspackages/app/supported-languages/src/constants/languages.tspackages/app/supported-languages/src/constants/lokalise-languages.spec.tspackages/app/supported-languages/src/constants/lokalise-languages.tspackages/app/supported-languages/src/constants/regions.tspackages/app/supported-languages/src/constants/rtl-languages.spec.tspackages/app/supported-languages/src/constants/rtl-languages.tspackages/app/supported-languages/src/constants/scripts.tspackages/app/supported-languages/src/constants/standard-locales.spec.tspackages/app/supported-languages/src/constants/standard-locales.tspackages/app/supported-languages/src/display-names.spec.tspackages/app/supported-languages/src/display-names.tspackages/app/supported-languages/src/either.tspackages/app/supported-languages/src/index.test.tspackages/app/supported-languages/src/index.tspackages/app/supported-languages/src/locale-lookup.spec.tspackages/app/supported-languages/src/locale-lookup.tspackages/app/supported-languages/src/locale.spec.tspackages/app/supported-languages/src/locale.tspackages/app/supported-languages/vitest.config.ts
💤 Files with no reviewable changes (3)
- packages/app/supported-languages/src/either.ts
- packages/app/supported-languages/package.json
- packages/app/supported-languages/src/index.test.ts
| /** | ||
| * Parse locale string into object. | ||
| * | ||
| * @throws {RangeError} If locale is structurally invalid or values are not in our supported values | ||
| */ |
There was a problem hiding this comment.
JSDoc incorrectly claims the function throws.
The @throws {RangeError} annotation is misleading since parseLocale returns an Either type and never throws. The error is communicated via the error property of the returned object.
📝 Proposed fix to correct the JSDoc
/**
* Parse locale string into object.
- *
- * `@throws` {RangeError} If locale is structurally invalid or values are not in our supported values
+ *
+ * `@returns` Either an error string (if locale is unsupported) or the parsed LocaleObject
*/
export const parseLocale = (tag: Locale): Either<string, LocaleObject> => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Parse locale string into object. | |
| * | |
| * @throws {RangeError} If locale is structurally invalid or values are not in our supported values | |
| */ | |
| /** | |
| * Parse locale string into object. | |
| * | |
| * `@returns` Either an error string (if locale is unsupported) or the parsed LocaleObject | |
| */ | |
| export const parseLocale = (tag: Locale): Either<string, LocaleObject> => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app/supported-languages/src/locale.ts` around lines 60 - 64, The
JSDoc for parseLocale incorrectly states it throws a RangeError; update the
comment for the parseLocale function (in locale.ts) to remove the `@throws` tag
and instead document the actual return shape: that it returns an
Either/Result-like object with either a parsed locale value or an error
string/object on the error property, and briefly describe the conditions that
populate the error field (e.g., structurally invalid or unsupported values).
2 participants
Changes
Adding
getLocaleDirectionutilityChecklist
major,minor,patchorskip-releaseAI Assistance Tracking
We're running a metric to understand where AI assists our engineering work. Please select exactly one of the options below:
Mark "Yes" if AI helped in any part of this work, for example: generating code, refactoring, debugging support,
explaining something, reviewing an idea, or suggesting an approach.