Made rule translation the default tab and added category translations #165
Conversation
WalkthroughAdded an optional Dialog prop Changes
Sequence Diagram(s)sequenceDiagram
participant RouterForm as RouterLocalizationForm
participant Dialog as Dialog
RouterForm->>Dialog: render(props including tabs?, homeTabName="Rule Translations")
Dialog->>Dialog: set activeTab = props.tabs ? props.tabs.length : -1
alt props.tabs && props.tabs.length > 0
Dialog->>Dialog: create homeTab { label: props.homeTabName || "Home", body: children }
Dialog->>Dialog: append homeTab to tabs
else
Dialog->>Dialog: do not append homeTab
end
RouterForm->>Dialog: pass defaultTabBody as children (rule or category view)
Dialog->>RouterForm: render UI showing tabs and active content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/dialog/Dialog.tsx (1)
45-45: Avoid duplicating showHomeTab in state (derived from props)Storing
showHomeTabin state invites drift and edge cases (e.g., when tabs are initially empty). Prefer deriving it from props during render/initialization. If you keep it in state, ensure it’s set in both constructor branches and synced incomponentDidUpdate.Possible simplification:
- Remove
showHomeTabfromDialogState.- In render, compute
const showHomeTab = this.props.showHomeTab !== false;and use that.- In constructor, only compute
activeTab(using the revised logic above).Also applies to: 50-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/components/flow/routers/localization/__snapshots__/RouterLocalizationForm.test.ts.snapis excluded by!**/*.snapsrc/components/flow/routers/subflow/__snapshots__/SubflowRouterForm.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/components/dialog/Dialog.tsx(4 hunks)src/components/flow/routers/localization/RouterLocalizationForm.tsx(1 hunks)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
src/components/flow/routers/localization/RouterLocalizationForm.tsx (1)
250-250: LGTM: Home tab explicitly disabled for this dialogPassing
showHomeTab={false}matches the intent for this flow and prevents showing the extra “Home” tab.
…and update related logic
akanshaaa19
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/flow/routers/localization/RouterLocalizationForm.tsx (1)
216-216: Fix ESLint build error: remove unused defaultTab.defaultTab is assigned but never used; it breaks the build. Dialog now manages the active tab internally, so delete this vestigial code.
Apply this diff:
- let defaultTab; @@ - defaultTab = 0;Also applies to: 240-240
🧹 Nitpick comments (5)
src/components/flow/routers/localization/RouterLocalizationForm.tsx (5)
220-222: Localize the new tab label.Hardcoded "Category Translations" should use i18n for consistency.
- name: 'Category Translations', + name: i18n.t('tabs.category_translations', 'Category Translations'),
223-226: Localize the new instructions for the Category tab.These are new user-visible strings; prefer i18n.t.
- <p data-spec="instructions"> - When category names are referenced later in the flow, the appropriate language for the - category will be used. If no translation is provided, the original text will be used. - </p> + <p data-spec="instructions"> + {i18n.t( + 'forms.routerLocalization.category_instructions', + 'When category names are referenced later in the flow, the appropriate language for the category will be used. If no translation is provided, the original text will be used.' + )} + </p>
231-233: Avoid magic number duplication for scroll threshold.The threshold 5 is duplicated for categories and cases; factor it into a constant to keep behavior consistent.
Example:
// near top of render() const SCROLL_THRESHOLD = 5; // then: (this.state.categories.length > SCROLL_THRESHOLD ? styles.scrolling : '') (this.state.cases.length > SCROLL_THRESHOLD ? styles.scrolling : '')Also applies to: 259-260
252-254: Localize the new instructions for the Home tab.Wrap with i18n.t for translation.
- <p data-spec="instructions"> - Sometimes languages need special rules to route things properly. If a translation is not - provided, the original rule will be used. - </p> + <p data-spec="instructions"> + {i18n.t( + 'forms.routerLocalization.rule_instructions', + 'Sometimes languages need special rules to route things properly. If a translation is not provided, the original rule will be used.' + )} + </p>
208-214: Nit: variable name typo.orginalCase → originalCase. Improves readability.
- const orginalCase = getOriginalCase(this.props.nodeSettings, kase.uuid) as Case; + const originalCase = getOriginalCase(this.props.nodeSettings, kase.uuid) as Case; return ( - orginalCase.arguments && - orginalCase.arguments.length > 0 && - orginalCase.type !== Operators.has_number_between + originalCase.arguments && + originalCase.arguments.length > 0 && + originalCase.type !== Operators.has_number_between );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/components/flow/routers/localization/__snapshots__/RouterLocalizationForm.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/components/dialog/Dialog.tsx(2 hunks)src/components/flow/routers/localization/RouterLocalizationForm.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/dialog/Dialog.tsx
🧰 Additional context used
🪛 GitHub Actions: Build
src/components/flow/routers/localization/RouterLocalizationForm.tsx
[error] 216-216: yarn build failed due to ESLint error: 'defaultTab' is assigned a value but never used. (no-unused-vars)
🔇 Additional comments (2)
src/components/flow/routers/localization/RouterLocalizationForm.tsx (2)
235-236: Render helper swap looks correct.Using renderCategories() within the Category tab aligns with the tab’s purpose.
263-264: Render helper remains correct for the Home tab.Using renderCases() under the "Rule Translations" Home tab matches the intent.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/flow/routers/localization/RouterLocalizationForm.tsx (1)
269-269: Localize the home tab label "Rule Translations"Use the new Dialog prop with a translated label instead of a hardcoded string. Update snapshots accordingly.
- homeTabName="Rule Translations" + homeTabName={i18n.t('tabs.rule_translations', 'Rule Translations')}Also refresh the snapshot:
- src/components/flow/routers/localization/snapshots/RouterLocalizationForm.test.ts.snap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/components/flow/routers/localization/RouterLocalizationForm.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/flow/routers/localization/RouterLocalizationForm.tsx (1)
src/components/dialog/Dialog.tsx (1)
Dialog(55-268)
⏰ 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: Coverage
- GitHub Check: Tests (12.x)
🔇 Additional comments (2)
src/components/flow/routers/localization/RouterLocalizationForm.tsx (2)
261-262: LGTM: default body logic matches the PR objectiveSelecting ruleTranslationsTab as the default body when cases with arguments exist makes “Rule Translations” the default view. This aligns with the stated goal.
271-271: LGTM: render default body via Dialog childrenRendering {defaultTabBody} as Dialog children correctly feeds the “Home” tab content. Works with and without additional tabs.
| const ruleTranslationsTab = ( | ||
| <> | ||
| <p data-spec="instructions"> | ||
| Sometimes languages need special rules to route things properly. If a translation is not | ||
| provided, the original rule will be used. | ||
| </p> | ||
| <div | ||
| className={ | ||
| styles.translating_list_container + | ||
| ' ' + | ||
| (this.state.cases.length > 5 ? styles.scrolling : '') | ||
| } | ||
| tabIndex={0} | ||
| > | ||
| <div className={styles.translating_item_list}>{this.renderCases()}</div> | ||
| </div> | ||
| </> | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Localize the Rule Translations instructions text
Wrap the instructions paragraph in i18n to avoid hardcoded English.
- <p data-spec="instructions">
- Sometimes languages need special rules to route things properly. If a translation is not
- provided, the original rule will be used.
- </p>
+ <p data-spec="instructions">
+ {i18n.t(
+ 'forms.router_localization.rule_instructions',
+ 'Sometimes languages need special rules to route things properly. If a translation is not provided, the original rule will be used.'
+ )}
+ </p>📝 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 ruleTranslationsTab = ( | |
| <> | |
| <p data-spec="instructions"> | |
| Sometimes languages need special rules to route things properly. If a translation is not | |
| provided, the original rule will be used. | |
| </p> | |
| <div | |
| className={ | |
| styles.translating_list_container + | |
| ' ' + | |
| (this.state.cases.length > 5 ? styles.scrolling : '') | |
| } | |
| tabIndex={0} | |
| > | |
| <div className={styles.translating_item_list}>{this.renderCases()}</div> | |
| </div> | |
| </> | |
| ); | |
| const ruleTranslationsTab = ( | |
| <> | |
| <p data-spec="instructions"> | |
| {i18n.t( | |
| 'forms.router_localization.rule_instructions', | |
| 'Sometimes languages need special rules to route things properly. If a translation is not provided, the original rule will be used.' | |
| )} | |
| </p> | |
| <div | |
| className={ | |
| styles.translating_list_container + | |
| ' ' + | |
| (this.state.cases.length > 5 ? styles.scrolling : '') | |
| } | |
| tabIndex={0} | |
| > | |
| <div className={styles.translating_item_list}>{this.renderCases()}</div> | |
| </div> | |
| </> | |
| ); |
🤖 Prompt for AI Agents
In src/components/flow/routers/localization/RouterLocalizationForm.tsx around
lines 207 to 225, the instructions paragraph is hardcoded in English; replace
that plain text with your project's internationalization API (e.g., wrap the
string in the translation function or use the Trans/Translation component your
app uses), preserving the <p data-spec="instructions"> element, punctuation and
intent, and ensure the translated key/string is added to the appropriate locale
files; keep the surrounding JSX structure and classNames unchanged.
| const categoryTranslationsTab = ( | ||
| <> | ||
| <p data-spec="instructions"> | ||
| When category names are referenced later in the flow, the appropriate language for the | ||
| category will be used. If no translation is provided, the original text will be used. | ||
| </p> | ||
| <div | ||
| className={ | ||
| styles.translating_list_container + | ||
| ' ' + | ||
| (this.state.categories.length > 5 ? styles.scrolling : '') | ||
| } | ||
| tabIndex={0} | ||
| > | ||
| <div className={styles.translating_item_list}>{this.renderCategories()}</div> | ||
| </div> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Localize the Category Translations instructions text
Same as above—please i18n-ize the prose.
- <p data-spec="instructions">
- When category names are referenced later in the flow, the appropriate language for the
- category will be used. If no translation is provided, the original text will be used.
- </p>
+ <p data-spec="instructions">
+ {i18n.t(
+ 'forms.router_localization.category_instructions',
+ 'When category names are referenced later in the flow, the appropriate language for the category will be used. If no translation is provided, the original text will be used.'
+ )}
+ </p>📝 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 categoryTranslationsTab = ( | |
| <> | |
| <p data-spec="instructions"> | |
| When category names are referenced later in the flow, the appropriate language for the | |
| category will be used. If no translation is provided, the original text will be used. | |
| </p> | |
| <div | |
| className={ | |
| styles.translating_list_container + | |
| ' ' + | |
| (this.state.categories.length > 5 ? styles.scrolling : '') | |
| } | |
| tabIndex={0} | |
| > | |
| <div className={styles.translating_item_list}>{this.renderCategories()}</div> | |
| </div> | |
| </> | |
| ); | |
| const categoryTranslationsTab = ( | |
| <> | |
| <p data-spec="instructions"> | |
| {i18n.t( | |
| 'forms.router_localization.category_instructions', | |
| 'When category names are referenced later in the flow, the appropriate language for the category will be used. If no translation is provided, the original text will be used.' | |
| )} | |
| </p> | |
| <div | |
| className={ | |
| styles.translating_list_container + | |
| ' ' + | |
| (this.state.categories.length > 5 ? styles.scrolling : '') | |
| } | |
| tabIndex={0} | |
| > | |
| <div className={styles.translating_item_list}>{this.renderCategories()}</div> | |
| </div> | |
| </> | |
| ); |
🤖 Prompt for AI Agents
In src/components/flow/routers/localization/RouterLocalizationForm.tsx around
lines 226 to 243, the explanatory paragraph text is hardcoded English and must
be internationalized; replace the literal string with a call to the project's
i18n helper (e.g., t('...') or intl.formatMessage) using a clear translation key
like 'routerLocalization.categoryTranslations.instructions' and include the
original text as the default message, import the i18n helper at the top of the
file if not already present, and add the new key and default text to the locale
files so translators can provide translations.
| name: 'Category Translations', | ||
| body: categoryTranslationsTab | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Localize the tab label "Category Translations"
Avoid hardcoded labels in tabs; use i18n with a sensible key.
- tabs.push({
- name: 'Category Translations',
- body: categoryTranslationsTab
- });
+ tabs.push({
+ name: i18n.t('tabs.category_translations', 'Category Translations'),
+ body: categoryTranslationsTab
+ });📝 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.
| name: 'Category Translations', | |
| body: categoryTranslationsTab | |
| }); | |
| tabs.push({ | |
| name: i18n.t('tabs.category_translations', 'Category Translations'), | |
| body: categoryTranslationsTab | |
| }); |
🤖 Prompt for AI Agents
In src/components/flow/routers/localization/RouterLocalizationForm.tsx around
lines 256 to 258, the tab label is hardcoded as "Category Translations"; replace
it with a call to the i18n translator (e.g. useTranslation hook) and a sensible
key such as 'router.tabs.categoryTranslations' (or
'flow.tabs.categoryTranslations') so the label is localizable; ensure you
import/use the hook at the top of the component, call
t('router.tabs.categoryTranslations') when building the tab object, and keep a
readable default/fallback in your translation files.
There was a problem hiding this comment.
Functionality-wise wise it's working fine.
Few comments:
- Can we please not mix multiple functionalities in the same PR? It can be confusing when it's being reviewed. It's ok for this PR.
- We should create a separate base branch for 1.26.x series.
v1.26.xand use that for release etc. Currently, this is raised againstglific-master
target issue is glific/glific-frontend#3425
Summary by CodeRabbit
New Features
Bug Fixes
Chores