Open
Conversation
60 tasks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new LocaleSelector React control that lazy-loads, fetches site locales on mount, formats locale labels, sets a default when unset, manages loading/error states with retry, and registers the control in the control map. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/Browser
participant Component as LocaleSelector
participant Service as SiteService (fetchSiteLocales)
participant Store as Form State
UI->>Component: mount (active siteId)
Component->>Service: fetchSiteLocales(siteId)
activate Service
Service-->>Component: locales + defaultLocale OR error
deactivate Service
alt success
Component->>Component: map locales via getLocaleLabel()
Component-->>UI: render Select (MenuItems, default selected if needed)
UI->>Component: user selects locale
Component->>Store: setValue(selectedLocale)
else error
Component-->>UI: render Alert with Retry
UI->>Component: click Retry
Component->>Service: fetchSiteLocales(siteId) (retry)
end
UI->>Component: unmount
Component->>Component: unsubscribe / cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx`:
- Around line 97-101: getLocaleLabel currently assumes exactly two
underscore-separated parts and treats the second token as the region, which
breaks on inputs like "zh_Hant_TW" or hyphenated tags; update getLocaleLabel to
split the input on both '_' and '-' into parts, use the first part as the
language, detect the region as the last part only if it looks like a region
subtag (e.g. two letters or three digits) and uppercase it, treat any middle
part(s) as script or variant subtags (title-case the script subtag) and include
them between language and region with hyphens, and fall back to returning just
the language when no valid region is detected so labels like "zh-Hant-TW",
"zh-Hant", "en-US" and "en" are produced correctly.
- Around line 67-79: The component currently returns null when !isFetching &&
!localeData, causing the control to disappear after a fetch error; instead
update LocaleSelector to always render the control even if localeData is null by
removing the early return and rendering a disabled/select-with-fallback state
when localeData is missing; in the error handler (where setIsFetching(false) and
dispatch(pushErrorDialog(...)) run) ensure you do not clear the current form
value—use existing refs/setValue to populate the select's value and pass
disabled={true} or show a "Failed to load locales" placeholder so the control
degrades gracefully while keeping the current selection visible.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/controls/LocaleSelector.tsxui/app/src/components/FormsEngine/lib/controlMap.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx (2)
76-76: Consider removing stable references from dependency array.
setIsFetching(state setter) andrefs(ref object fromuseUpdateRefs) are stable and don't need to be in the dependency array. IfsetValueisn't stable, it could cause unnecessary re-fetching.♻️ Suggested dependency array
- }, [siteId, dispatch, refs, setValue, setIsFetching]); + }, [siteId, dispatch, setValue]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx` at line 76, The effect dependency array on the useEffect that currently lists [siteId, dispatch, refs, setValue, setIsFetching] should be tightened: remove stable references refs (from useUpdateRefs) and setIsFetching (state setter) because they are stable across renders, leaving only true changing values such as siteId and dispatch; if setValue is not a stable function, either memoize it where it's created or include it deliberately, but avoid leaving in stable refs that cause needless runs—update the useEffect dependency array accordingly and, if you keep setValue, add a comment explaining why it must remain.
85-85: Consider providing a fallback forvalueto avoid MUI warnings.If
valueisundefinedinitially, MUI's Select may log a warning about switching from uncontrolled to controlled. The component already auto-selectsdefaultLocaleCodewhen there's no value, but there's a brief moment wherevaluecould be undefined.♻️ Proposed change
- <Select value={value} onChange={handleChange} autoFocus={autoFocus}> + <Select value={value ?? ''} onChange={handleChange} autoFocus={autoFocus}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx` at line 85, The Select component may receive undefined briefly causing MUI uncontrolled->controlled warnings; update the LocaleSelector component so the Select's value uses a safe fallback (e.g. value ?? defaultLocaleCode or ''), ensuring the prop passed to Select is never undefined; adjust any related logic in handleChange or where defaultLocaleCode is determined so the fallback matches expected option keys and preserves autoFocus behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx`:
- Around line 97-120: The getLocaleLabel function currently calls new
Intl.Locale(normalized) outside the try/catch so malformed or empty localeCode
throws before DisplayNames are attempted; move the parsing (const normalized =
localeCode.replace(...); const parsed = new Intl.Locale(normalized); const lang
= parsed.language; const region = parsed.region; const locale =
parsed.toString();) inside the existing try block that creates
Intl.DisplayNames, so any RangeError from Intl.Locale is caught; in the catch
ensure you set languageLabel = langFallback (use either parsed.language if
available or the original localeCode or an empty string) and regionLabel =
regionFallback (uppercased region if present or undefined) so the function
returns a safe fallback like "en (US)" or the raw code when parsing fails.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx`:
- Line 76: The effect dependency array on the useEffect that currently lists
[siteId, dispatch, refs, setValue, setIsFetching] should be tightened: remove
stable references refs (from useUpdateRefs) and setIsFetching (state setter)
because they are stable across renders, leaving only true changing values such
as siteId and dispatch; if setValue is not a stable function, either memoize it
where it's created or include it deliberately, but avoid leaving in stable refs
that cause needless runs—update the useEffect dependency array accordingly and,
if you keep setValue, add a comment explaining why it must remain.
- Line 85: The Select component may receive undefined briefly causing MUI
uncontrolled->controlled warnings; update the LocaleSelector component so the
Select's value uses a safe fallback (e.g. value ?? defaultLocaleCode or ''),
ensuring the prop passed to Select is never undefined; adjust any related logic
in handleChange or where defaultLocaleCode is determined so the fallback matches
expected option keys and preserves autoFocus behavior.
rart
requested changes
Apr 7, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx`:
- Line 37: The component ignores the control contract's readonly flag: update
the props destructuring in LocaleSelector (add readonly to const { field, value,
setValue, autoFocus }) and make the rendered <Select> respect that flag (in the
Select element at the render block around where onChange/setValue are used, pass
disabled or readOnly based on readonly and avoid calling setValue when readonly
so selection changes are no-ops). Ensure both the destructured readonly name and
the Select's disabled/readOnly and onChange behavior are updated so the control
is truly read-only.
- Line 55: The success handler for fetchSiteLocales in LocaleSelector.tsx is
destructuring its parameter directly (next: ({ localeCodes, defaultLocaleCode })
=> ...) which will throw if the callback receives undefined; change the handler
to accept a single param (e.g., result) and safely extract with a fallback (for
example: const { localeCodes = [], defaultLocaleCode } = result || {}) or use
optional chaining/defaults so localeCodes and defaultLocaleCode are always
defined before using them in the function body.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f727a6a-a025-4f9f-bd86-76d88f8c9988
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx`:
- Around line 60-72: The default-locale write can run before localeData is
populated because setValue(defaultLocaleCode) is outside the if (localeCodes)
guard; move the default-value assignment inside the same if (localeCodes) block
(after calling setLocaleData(...)) or otherwise ensure setLocaleData is always
called when a response contains defaultLocaleCode, so that refs.current.value,
defaultLocaleCode, setLocaleData and setValue are updated atomically and the
component never ends up with a value but no localeData/renderable control.
- Line 105: The JSX in LocaleSelector.tsx currently renders "{error.message}.
{error.remedialAction}" which prints "undefined" when remedialAction is missing;
update the rendering in the component (where error is used) to include
remedialAction only if it is defined or non-empty (e.g., render
"{error.message}{error.remedialAction ? '. ' + error.remedialAction : ''}"), so
the message ends cleanly when error.remedialAction is absent; ensure you
reference the same error object used by the component and keep the punctuation
only when remedialAction exists.
- Around line 70-72: The code currently auto-sets the form value when
defaultLocaleCode exists even if the field is readonly; update the init/cleanup
logic in LocaleSelector so the default write is skipped when readonly is true by
guarding the call to setValue(defaultLocaleCode) with the readonly flag (i.e.
only call setValue when !readonly && !refs.current.value && defaultLocaleCode).
Also ensure the ref object used (refs.current) includes the readonly state so
the component and its Select control remain consistent with the
readonly/disabled behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 424e8d55-96e9-4cc9-8640-3c5db2084507
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx
Comment on lines
+60
to
+72
| setIsFetching(false); | ||
| if (localeCodes) { | ||
| setLocaleData({ | ||
| localeCodes: localeCodes.map((code) => ({ | ||
| code, | ||
| label: getLocaleLabel(code) | ||
| })), | ||
| defaultLocaleCode | ||
| }); | ||
| } | ||
| if (!refs.current.value && defaultLocaleCode) { | ||
| setValue(defaultLocaleCode); | ||
| } |
There was a problem hiding this comment.
Default-locale write can run while localeData remains unset.
The if (!refs.current.value && defaultLocaleCode) block sits outside the if (localeCodes) guard, so a response shaped { defaultLocaleCode, localeCodes: null } will call setValue(defaultLocaleCode) while the component then early-returns null at Line 88 (since localeData stays undefined and error is null). The form ends up with a value but no visible control. Consider nesting the default-value write under the same localeCodes guard, or ensuring localeData is always populated when a response is received.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx` around lines
60 - 72, The default-locale write can run before localeData is populated because
setValue(defaultLocaleCode) is outside the if (localeCodes) guard; move the
default-value assignment inside the same if (localeCodes) block (after calling
setLocaleData(...)) or otherwise ensure setLocaleData is always called when a
response contains defaultLocaleCode, so that refs.current.value,
defaultLocaleCode, setLocaleData and setValue are updated atomically and the
component never ends up with a value but no localeData/renderable control.
Comment on lines
+70
to
+72
| if (!refs.current.value && defaultLocaleCode) { | ||
| setValue(defaultLocaleCode); | ||
| } |
There was a problem hiding this comment.
Don't auto-apply default locale when the field is read-only.
The default-value write runs regardless of readonly, which mutates the form value on read-only forms (and defeats the purpose of the disabled Select added at Line 108). Guard the call with the readonly flag.
Proposed fix
- if (!refs.current.value && defaultLocaleCode) {
+ if (!refs.current.value && defaultLocaleCode && !refs.current.readonly) {
setValue(defaultLocaleCode);
}And include readonly in the ref:
- const refs = useUpdateRefs({ value });
+ const refs = useUpdateRefs({ value, readonly });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx` around lines
70 - 72, The code currently auto-sets the form value when defaultLocaleCode
exists even if the field is readonly; update the init/cleanup logic in
LocaleSelector so the default write is skipped when readonly is true by guarding
the call to setValue(defaultLocaleCode) with the readonly flag (i.e. only call
setValue when !readonly && !refs.current.value && defaultLocaleCode). Also
ensure the ref object used (refs.current) includes the readonly state so the
component and its Select control remain consistent with the readonly/disabled
behavior.
| </Button> | ||
| } | ||
| > | ||
| {error.message}. {error.remedialAction} |
There was a problem hiding this comment.
Guard against undefined remedialAction.
If error.remedialAction is not provided by the API, this renders a trailing ". undefined". Render it conditionally.
Proposed fix
- {error.message}. {error.remedialAction}
+ {error.message}{error.remedialAction ? `. ${error.remedialAction}` : ''}📝 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.
Suggested change
| {error.message}. {error.remedialAction} | |
| {error.message}{error.remedialAction ? `. ${error.remedialAction}` : ''} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/src/components/FormsEngine/controls/LocaleSelector.tsx` at line 105,
The JSX in LocaleSelector.tsx currently renders "{error.message}.
{error.remedialAction}" which prints "undefined" when remedialAction is missing;
update the rendering in the component (where error is used) to include
remedialAction only if it is defined or non-empty (e.g., render
"{error.message}{error.remedialAction ? '. ' + error.remedialAction : ''}"), so
the message ends cleanly when error.remedialAction is absent; ensure you
reference the same error object used by the component and keep the punctuation
only when remedialAction exists.
rart
requested changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
craftercms/craftercms#7418
Summary by CodeRabbit