English support#92
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR switches the app default from Hebrew/RTL to English/LTR, introduces a ChangesRTL/LTR Localization and CSS Logical Properties
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.html`:
- Line 2: In the changeLanguage() function in App.tsx, after calling
i18n.changeLanguage(lng), add a listener to the i18n object that updates the
document root attributes whenever the language changes. Specifically, listen for
the 'languageChanged' event and update document.documentElement.lang to the new
language code, and set document.documentElement.dir to 'rtl' for RTL languages
(Arabic and Hebrew) or 'ltr' for other languages. This ensures the HTML root
element reflects the current language settings, properly supporting RTL layouts
and accessibility when switching between languages.
In `@src/components/UI/Slider/SGLSlider.tsx`:
- Around line 7-9: The sliderProps interface in SGLSlider.tsx has a typing
conflict where it inherits value and defaultValue properties from MuiSliderProps
(which can be number or number[]) but defines onChange to only accept a single
number parameter. To enforce a single-value contract, expand the Omit statement
to also exclude the value and defaultValue properties from MuiSliderProps in
addition to onChange, then remove the type assertion "as number" at line 19
where value is used. This ensures the interface accurately represents a
single-value slider and eliminates the type mismatch between the props and the
onChange callback signature.
In `@src/hooks/useIsRtl.ts`:
- Line 6: The RTL detection logic in the isRtl variable only matches exact
language codes like 'he' and 'ar', but fails for regional locale variants such
as 'he-IL' or 'ar-SA'. Extract the base language code from i18n.language by
splitting on the hyphen and taking the first part, then compare this extracted
base code against 'he' and 'ar' to properly handle all locale variants.
In `@src/i18n.ts`:
- Around line 26-27: The theme direction is hardcoded as RTL in theme.ts while
the i18n configuration defaults to English in i18n.ts, creating a mismatch where
the UI always renders in RTL mode regardless of the selected language. To fix
this, synchronize the theme direction with the current language by either
extracting the language from i18n and passing it to ThemeProvider to dynamically
set the direction property based on whether the locale is RTL or LTR, or by
listening to i18n language change events and recreating the theme with the
correct direction when the language changes. Ensure that languages like English
and Russian correctly set direction to LTR while RTL languages set it
appropriately.
In `@src/pages/lifeStyle/physioAndTrainingTab/balanceTraining/styles.ts`:
- Line 14: The marginInline property at line 14 applies margins to both left and
right sides instead of just the right side like the original marginRight did,
effectively doubling the horizontal margin. Replace marginInline with
marginInlineEnd to maintain the original single-sided margin behavior while
keeping it direction-aware (marginInlineEnd equals marginRight in LTR and
marginLeft in RTL). Apply this same fix to the other occurrence of marginInline
noted at line 29 in the styles file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ab65a9c-59d3-4310-9c17-67a28a0bc0a1
📒 Files selected for processing (14)
index.htmlsrc/components/NavBar/styles.tssrc/components/UI/Slider/SGLSlider.tsxsrc/components/UI/Slider/styles.tssrc/hooks/useIsRtl.tssrc/i18n.tssrc/layouts/styles.tssrc/pages/dailyReports/metricsReport/style.tssrc/pages/dailyReports/style.tssrc/pages/home/checkIn/CheckIn.tsxsrc/pages/home/checkIn/style.tssrc/pages/lifeStyle/LifeStyle.tsxsrc/pages/lifeStyle/physioAndTrainingTab/balanceTraining/styles.tssrc/pages/schedule/styles.ts
💤 Files with no reviewable changes (2)
- src/components/NavBar/styles.ts
- src/pages/lifeStyle/LifeStyle.tsx
| @@ -1,5 +1,5 @@ | |||
| <!doctype html> | |||
| <html lang="he" dir="rtl"> | |||
| <html lang="en" dir="ltr"> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for runtime root-attribute sync on language changes.
rg -n "document\\.documentElement\\.(dir|lang)|setAttribute\\(['\"](dir|lang)|i18n\\.on\\(['\"]languageChanged" --type=ts --type=tsx --type=js srcRepository: ColmanDevClubORG/Sagol360Management
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check available file types and search for language switching logic
echo "=== Searching for language switching logic ==="
rg -n "document\.documentElement\.(dir|lang)|setAttribute.*(?:dir|lang)|languageChanged|language.*change" src --max-count=20
echo -e "\n=== Checking for i18n configuration ==="
fd -e js -e ts -e json src | xargs grep -l "i18n\|language" | head -10
echo -e "\n=== List files in src ==="
find src -type f \( -name "*.js" -o -name "*.ts" -o -name "*.json" -o -name "*.html" \) | head -20Repository: ColmanDevClubORG/Sagol360Management
Length of output: 1023
🏁 Script executed:
#!/bin/bash
# Look more broadly for language/direction handling
rg -n "lang|dir|hebrew|arabic" src --type-list | grep "type:"Repository: ColmanDevClubORG/Sagol360Management
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Search for any language switching mechanism
rg -i "setlanguage|changelanguage|selectlanguage|language.*switch" srcRepository: ColmanDevClubORG/Sagol360Management
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Check the i18n configuration and see if it already updates document attributes
rg -n "document\.documentElement" src
echo -e "\n=== Full changeLanguage implementation in App.tsx ==="
rg -B5 -A10 "changeLanguage" src/App.tsx | head -40Repository: ColmanDevClubORG/Sagol360Management
Length of output: 1317
🏁 Script executed:
#!/bin/bash
# Look for i18n setup and configuration
find src -name "*i18n*" -o -name "*locali*" | head -10Repository: ColmanDevClubORG/Sagol360Management
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check index.html for current state
cat index.htmlRepository: ColmanDevClubORG/Sagol360Management
Length of output: 456
🏁 Script executed:
#!/bin/bash
# Read the i18n configuration file
cat src/i18n.tsRepository: ColmanDevClubORG/Sagol360Management
Length of output: 745
Update document root lang and dir attributes when language changes.
The changeLanguage() function in App.tsx calls i18n.changeLanguage(lng) but does not update document.documentElement.lang and document.documentElement.dir. When switching to Hebrew or Arabic, the root element remains lang="en" dir="ltr", breaking RTL layout and accessibility. Add a listener to update these attributes on language changes (e.g., i18n.on('languageChanged', (lng) => { document.documentElement.lang = lng; document.documentElement.dir = lng === 'ar' || lng === 'he' ? 'rtl' : 'ltr'; })).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@index.html` at line 2, In the changeLanguage() function in App.tsx, after
calling i18n.changeLanguage(lng), add a listener to the i18n object that updates
the document root attributes whenever the language changes. Specifically, listen
for the 'languageChanged' event and update document.documentElement.lang to the
new language code, and set document.documentElement.dir to 'rtl' for RTL
languages (Arabic and Hebrew) or 'ltr' for other languages. This ensures the
HTML root element reflects the current language settings, properly supporting
RTL layouts and accessibility when switching between languages.
| interface sliderProps extends Omit<MuiSliderProps, 'onChange'> { | ||
| onChange: (value: number) => void | ||
| style?: CSSProperties |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "SGLSlider.tsx" --exec cat -n {} \;Repository: ColmanDevClubORG/Sagol360Management
Length of output: 909
🏁 Script executed:
# Check MUI Slider props definition to confirm range mode support
rg "SliderProps" --type ts --type tsx -A 5 -B 2 | head -50Repository: ColmanDevClubORG/Sagol360Management
Length of output: 107
🏁 Script executed:
# Search for usages of SGLSlider to see if range mode is ever attempted
rg "SGLSlider" --type ts --type tsx -A 3 -B 2Repository: ColmanDevClubORG/Sagol360Management
Length of output: 107
🌐 Web query:
MUI Slider component value prop number array range mode type definition
💡 Result:
In the Material UI (MUI) Slider component, the value prop accepts either a number or an array of numbers, depending on whether you are implementing a single-value slider or a range slider [1][2]. Type Definition and Usage: - Single-value mode: The value prop expects a number [1][3]. - Range mode: To enable range selection, provide an array containing exactly two numbers (e.g., [start, end]) to the value prop [1][4][2]. When using the component with TypeScript, the value is typically represented as number or number[] [3][5]. For range sliders, the component manages two thumbs corresponding to the two values in the array [4][6]. Key Implementation Details: - onChange handler: When the slider's value changes, the onChange callback receives the event and the new value. For range sliders, the second argument (the value) will be an array of numbers (e.g., number[] or readonly number[]) [3][2]. - Minimum Distance: When using a range slider, you can manage the spacing between the two thumbs using the disableSwap prop or by handling the activeThumb parameter provided in the onChange callback [4][6]. The Slider component automatically detects that it should operate in range mode when an array is provided to the value or defaultValue prop [4][2].
Citations:
- 1: https://mui.com/material-ui/api/slider/
- 2: https://mui.com/material-ui/react-slider.md
- 3: https://github.com/mui/material-ui/blob/7481efaa/packages/mui-material/src/Slider/Slider.spec.tsx
- 4: https://mui.com/material-ui/react-slider/
- 5: https://cdn.jsdelivr.net/npm/@mui/material@5.11.10/Slider/Slider.d.ts
- 6: https://next.mui.com/material-ui/react-slider/
🏁 Script executed:
# Search for SGLSlider usages without explicit file type filtering
rg "SGLSlider" -A 3 -B 2Repository: ColmanDevClubORG/Sagol360Management
Length of output: 2044
🏁 Script executed:
# Check how SliderProps is imported and what it provides
rg "SliderProps as MuiSliderProps" -A 5Repository: ColmanDevClubORG/Sagol360Management
Length of output: 550
onChange typing conflicts with inherited Slider range mode.
The interface extends MuiSliderProps but only removes onChange, leaving value and defaultValue as number | number[]. However, the custom onChange callback accepts only number. At line 19, value as number masks this contract mismatch. Either explicitly narrow the interface to single-value mode or handle both number and number[] safely.
Suggested patch (enforce single-value contract)
-interface sliderProps extends Omit<MuiSliderProps, 'onChange'> {
+interface sliderProps extends Omit<MuiSliderProps, 'onChange' | 'value' | 'defaultValue'> {
+ value?: number
+ defaultValue?: number
onChange: (value: number) => void
style?: CSSProperties
}
@@
- onChange={(_, value) => onChange(value as number)}
+ onChange={(_, value) => {
+ if (typeof value === 'number') onChange(value)
+ }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/UI/Slider/SGLSlider.tsx` around lines 7 - 9, The sliderProps
interface in SGLSlider.tsx has a typing conflict where it inherits value and
defaultValue properties from MuiSliderProps (which can be number or number[])
but defines onChange to only accept a single number parameter. To enforce a
single-value contract, expand the Omit statement to also exclude the value and
defaultValue properties from MuiSliderProps in addition to onChange, then remove
the type assertion "as number" at line 19 where value is used. This ensures the
interface accurately represents a single-value slider and eliminates the type
mismatch between the props and the onChange callback signature.
| export const useIsRtl = () => { | ||
| const { i18n } = useTranslation() | ||
|
|
||
| const isRtl = i18n.language === 'he' || i18n.language === 'ar' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/hooks/useIsRtl.tsRepository: ColmanDevClubORG/Sagol360Management
Length of output: 334
🏁 Script executed:
# Search for i18next usage patterns in the codebase
rg "i18n\.(language|resolvedLanguage)" -A 2 -B 2Repository: ColmanDevClubORG/Sagol360Management
Length of output: 662
🌐 Web query:
i18next language property regional locale subtags behavior
💡 Result:
In i18next, regional locale subtags (e.g., 'en-US', 'zh-HK') are handled through a flexible resolution system that balances specificity with fallback strategies [1][2]. Language Resolution Behavior When a language is set (e.g., 'en-US'), i18next populates an internal array of language codes used for key lookups [3]. By default, this array follows a 'most specific to least specific' order, which includes the full code, the language-only part, and finally any defined fallback languages [3][1]. For example, if the language is 'en-US', i18next will typically look up keys in 'en-US' first, then 'en', and then proceed to configured fallbacks [3][4]. Configuration Options You can control how i18next processes these regional subtags using the 'load' configuration option [5]: 1. 'all' (default): i18next searches for the full locale, then the language-only part, then the fallback [4][5]. 2. 'currentOnly': i18next restricts lookups to the exact language code provided (e.g., 'en-US' only), disabling the automatic fallback to the language-only version [4][5]. 3. 'languageOnly': i18next ignores the regional subtag and only looks up the language-only part (e.g., 'en') [5]. Formatting and Best Practices i18next strongly recommends using BCP 47 language tags (e.g., 'en-US', 'zh-Hant-HK') for all language codes to ensure proper integration with built-in features like pluralization and fallback resolution [6]. If you use a dash ('-') in your language codes, i18next attempts to format them using 'Intl.getCanonicalLocales' [6]. Handling Regional Fallbacks If you require specific fallback behavior (e.g., a regional variant falling back to a different regional variant rather than a neutral language), you can define a custom fallback strategy by providing an object or a function to the 'fallbackLng' configuration option [2]. If you are using the i18next-browser-languageDetector, you may need to use the 'convertDetectedLanguage' option to manipulate detected locale strings (e.g., replacing '-' with '_') before they reach the core i18next engine, ensuring they align with your specific resource file naming conventions [7][8].
Citations:
- 1: https://www.i18next.com/principles/translation-resolution
- 2: https://www.i18next.com/principles/fallback
- 3: https://www.i18next.com/overview/api.md
- 4: Regional variations fall back to regionless before specified fallbackLng rules i18next/i18next#1506
- 5: https://www.i18next.com/overview/configuration-options
- 6: https://www.i18next.com/how-to/faq
- 7: https://github.com/i18next/i18next-browser-languageDetector
- 8: https://github.com/i18next/i18next-browser-languageDetector/blob/master/README.md
🌐 Web query:
i18next resolvedLanguage vs language property
💡 Result:
In i18next, the language and resolvedLanguage properties serve different purposes regarding how the library tracks and identifies the active language [1][2]. i18next.language The language property is set to the language that was requested or detected (e.g., via a language detector or a call to changeLanguage) [1][3]. It represents the user's preferred language or the language you have explicitly set [1][2]. This value is used for event reporting and initial state identification, but it does not necessarily represent the language code of the actual translation resources currently in use by the system [2]. i18next.resolvedLanguage The resolvedLanguage property represents the actual language code currently resolved by i18next based on your configuration, such as supportedLngs and load settings [1]. While language tracks what the user prefers, resolvedLanguage tells you which specific language code i18next matched against your available resources [2]. For example, if a user has a browser preference of en-US but you only support en, language might remain en-US (depending on your detection settings), while resolvedLanguage will be en [2]. Key Takeaways: - Use resolvedLanguage (or i18next.languages[0]) when you need the primary language currently being used for translations, such as when updating a language switcher UI or setting the lang attribute on an HTML element [1][3]. - language should be treated as the intended or requested language, whereas resolvedLanguage is the effective language resulting from i18next's internal resolution process [1][2].
Citations:
- 1: https://www.i18next.com/overview/api
- 2: supportedLngs does not correctly resolve regional language codes (e.g., 'en-US' to 'en') when load: 'languageOnly' is used i18next/i18next#2354
- 3: https://www.i18next.com/misc/migration-guide.md
Extract base language code for RTL detection to handle regional locales.
At Line 6, the exact checks against 'he' and 'ar' fail for regional locale variants (e.g., he-IL, ar-SA), causing incorrect RTL detection. Extract the base language code before comparison:
Suggested patch
- const isRtl = i18n.language === 'he' || i18n.language === 'ar'
+ const baseLang = i18n.language.split('-')[0]
+ const isRtl = baseLang === 'he' || baseLang === 'ar'📝 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.
| const isRtl = i18n.language === 'he' || i18n.language === 'ar' | |
| const baseLang = i18n.language.split('-')[0] | |
| const isRtl = baseLang === 'he' || baseLang === 'ar' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useIsRtl.ts` at line 6, The RTL detection logic in the isRtl
variable only matches exact language codes like 'he' and 'ar', but fails for
regional locale variants such as 'he-IL' or 'ar-SA'. Extract the base language
code from i18n.language by splitting on the hyphen and taking the first part,
then compare this extracted base code against 'he' and 'ar' to properly handle
all locale variants.
| lng: 'en', | ||
| fallbackLng: 'en', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direction source-of-truth and runtime synchronization points.
rg -n "createTheme\\(|direction\\s*:" --type=ts --type=tsx src
rg -n "ThemeProvider|document\\.documentElement\\.(dir|lang)|setAttribute\\(['\"](dir|lang)|languageChanged" --type=ts --type=tsx srcRepository: ColmanDevClubORG/Sagol360Management
Length of output: 135
🏁 Script executed:
cd src && ls -la *.ts 2>/dev/null | head -20Repository: ColmanDevClubORG/Sagol360Management
Length of output: 254
🏁 Script executed:
cat -n src/i18n.tsRepository: ColmanDevClubORG/Sagol360Management
Length of output: 976
🏁 Script executed:
cat -n src/theme.tsRepository: ColmanDevClubORG/Sagol360Management
Length of output: 3648
🏁 Script executed:
rg -n "direction|lng|createTheme" src --max-count=50Repository: ColmanDevClubORG/Sagol360Management
Length of output: 515
🏁 Script executed:
rg -n "ThemeProvider|useTranslation|i18next|useEffect.*direction|useEffect.*lng" src --max-count=50Repository: ColmanDevClubORG/Sagol360Management
Length of output: 10561
🏁 Script executed:
cat -n src/main.tsx | head -60Repository: ColmanDevClubORG/Sagol360Management
Length of output: 2311
🏁 Script executed:
cat -n src/App.tsxRepository: ColmanDevClubORG/Sagol360Management
Length of output: 2845
🏁 Script executed:
rg -n "useMemo|useEffect|createTheme" src/main.tsx src/App.tsx --max-count=20Repository: ColmanDevClubORG/Sagol360Management
Length of output: 61
🏁 Script executed:
rg -n "theme.*=|setTheme|useTheme" src --max-count=30Repository: ColmanDevClubORG/Sagol360Management
Length of output: 6892
Update theme direction when language changes to prevent RTL rendering in LTR locales.
With lng: 'en' in i18n.ts (line 26) but direction: 'rtl' hardcoded in theme.ts, the MUI theme will always render RTL regardless of language. When users switch to English or Russian, components still receive the RTL theme, causing layout misalignment and broken localization.
Pass the current language direction to ThemeProvider or recreate the theme when language changes to keep direction in sync with locale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n.ts` around lines 26 - 27, The theme direction is hardcoded as RTL in
theme.ts while the i18n configuration defaults to English in i18n.ts, creating a
mismatch where the UI always renders in RTL mode regardless of the selected
language. To fix this, synchronize the theme direction with the current language
by either extracting the language from i18n and passing it to ThemeProvider to
dynamically set the direction property based on whether the locale is RTL or
LTR, or by listening to i18n language change events and recreating the theme
with the correct direction when the language changes. Ensure that languages like
English and Russian correctly set direction to LTR while RTL languages set it
appropriately.
| alignItems: 'center', | ||
| gap: '0.5rem', | ||
| marginRight: '1rem', | ||
| marginInline: '1rem', |
There was a problem hiding this comment.
marginInline applies margins to both sides, unlike marginRight.
The migration from marginRight: '1rem' to marginInline: '1rem' changes the spacing behavior. marginInline is shorthand for both marginInlineStart and marginInlineEnd, so it applies 1rem margin on both the left and right sides (in LTR), effectively doubling the horizontal margin compared to the old marginRight which only applied to the right side.
If the intent is to preserve the original single-sided margin while making it direction-aware, use marginInlineEnd: '1rem' instead (which equals marginRight in LTR and marginLeft in RTL).
If symmetric horizontal margins are desired, the current change is correct.
🔧 Proposed fix if single-sided margin was intended
export const titleRowStyle: CSSProperties = {
display: 'flex',
flexDirection: 'row',
alignItems: 'center',
gap: '0.5rem',
- marginInline: '1rem',
+ marginInlineEnd: '1rem',
marginTop: '1rem',
} export const bodyContainerStyle: CSSProperties = {
display: 'flex',
flexDirection: 'column',
- marginInline: '1rem',
+ marginInlineEnd: '1rem',
marginTop: '0.5rem',
gap: '1rem',
marginBottom: '1rem',
}Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/lifeStyle/physioAndTrainingTab/balanceTraining/styles.ts` at line
14, The marginInline property at line 14 applies margins to both left and right
sides instead of just the right side like the original marginRight did,
effectively doubling the horizontal margin. Replace marginInline with
marginInlineEnd to maintain the original single-sided margin behavior while
keeping it direction-aware (marginInlineEnd equals marginRight in LTR and
marginLeft in RTL). Apply this same fix to the other occurrence of marginInline
noted at line 29 in the styles file.
| export const useIsRtl = () => { | ||
| const { i18n } = useTranslation() | ||
|
|
||
| const isRtl = i18n.language === 'he' || i18n.language === 'ar' |
There was a problem hiding this comment.
What about the other 2 languages ?
There was a problem hiding this comment.
because only this 2 lunguages is rtl - then the "if" will be true
isRtl - return boolean value
There was a problem hiding this comment.
ok, i would create an enum for all of the supported lnaguages
| @@ -0,0 +1,8 @@ | |||
| import { useTranslation } from 'react-i18next' | |||
|
|
|||
| export const useIsRtl = () => { | |||
There was a problem hiding this comment.
I would name this useDirection so that we will be able to export other stuff in the future and the name would fit
2 participants
Description
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Changes