CNV-76888:t function for translations works only on page reload#3473
Open
aviavissar wants to merge 1 commit intokubevirt-ui:mainfrom
Conversation
Collaborator
|
@aviavissar: This pull request references CNV-76888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBulk i18n changes: many locale JSONs updated and numerous modules adjusted to either remove in-file translation calls (replacing with static English strings) or add runtime translation wrappers via the translation hook; instance-type series label handling and i18n extraction comments were also modified. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
Collaborator
|
@aviavissar: This pull request references CNV-76888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils/components/AddBootableVolumeModal/components/SourceTypeSelection/SourceTypeSelection.tsx (1)
33-47:⚠️ Potential issue | 🟠 MajorStale closure:
formSelectionis read insideonSelectbut missing from the dependency array.Line 40 reads
formSelectionto decide whether to callresetDiskSize, but theuseCallbackdeps on Line 46 omit it. AfterformSelectionchanges, the callback still captures the old value, so theresetDiskSizeguard may fire incorrectly (or not fire when it should).Proposed fix
- [setFormSelection, resetDiskSize], + [setFormSelection, resetDiskSize, formSelection],src/utils/components/FirmwareBootloaderModal/utils/constants.ts (1)
17-42:⚠️ Potential issue | 🟠 MajorWrap all bootloader option strings with
t()at render time.The
description,title, andselectedLabelin bothFirmwareBootloaderModal.tsxandTemplateBootloaderModal.tsxare user-facing strings that must be translated. Currently they render withoutt():
- Line 62/65:
selectedLabel={BootModeTitles[selectedFirmwareBootloader]}- Line 65-68 / 68-72: Option
descriptionandtitlepassed directly toSelectOptionWrap each string with the
t()function already imported in both components.src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.ts (1)
26-76:⚠️ Potential issue | 🟠 MajorFix missing i18n translation for hardcoded menu labels and series labels.
The constants file contains user-facing strings that are used without consistent translation:
initialMenuItemslabels ('Red Hat provided','User provided'): Used directly in Tab titles inSelectInstanceTypeSection.tsxwithoutt()wrapping.
instanceTypeSeriesNameMapper.seriesLabel('CX series','D series', etc.):
- In chart rendering (
vms-per-resource-card/VMsPerResourceChart.tsxline 57),getInstanceTypeSeriesLabel()returns the raw untranslated label without applyingt().- Some components (DrilldownMenuItemWrapper, RedHatSeriesMenuCard) have fallback logic using
getSeriesLabel(seriesName, t)which translates, but this is inconsistent and doesn't cover all consumers.Move the
t()wrapping to render time for all consumers, or add translation keys to the constants and applyt()at all usage sites consistently.
🤖 Fix all issues with AI agents
In
`@src/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/utils/constants.ts`:
- Around line 17-37: URLSourceHelperText.tsx is rendering the OS_NAME_TYPES
fields (beforeLabelText, label, afterLabelText) without translation; import the
useKubevirtTranslation hook from
`@kubevirt-plugin/utils/hooks/useKubevirtTranslation` in URLSourceHelperText.tsx,
call const { t } = useKubevirtTranslation(), and wrap each user-facing value
(beforeLabelText, label, afterLabelText) with t(...) when rendering; keep the
existing fields for now but ensure every string passed to the UI is wrapped with
t() (e.g., t(osEntry.beforeLabelText), t(osEntry.label),
t(osEntry.afterLabelText)); optionally note for a follow-up to replace the
three-part pattern with a single interpolated translation key to avoid i18n
concatenation issues.
In `@src/utils/components/GuidedTour/utils/constants.tsx`:
- Around line 12-87: The tour strings in tourSteps are not translated because
TourPopover renders step.title/step.content raw; update either TourPopover (in
GuidedTour.tsx) to call t() on string titles/contents before rendering, or
(preferred) wrap all plain string user-facing values in the tourSteps array with
the translation function used in the project (e.g., t()) and pass translated
title prop into NewStepTitle (replace title="Tree view" with title={t('Tree
view')}), ensuring every plain string in content and title fields is translated
while leaving components like AddVolumeContent/EndTourContent unchanged.
In `@src/utils/components/ListPageFilter/constants.ts`:
- Around line 6-14: The getSearchTextPlaceholder function currently returns
nameFilterPlaceholder || STATIC_SEARCH_FILTERS_PLACEHOLDERS.name which can
produce an untranslated fallback; update getSearchTextPlaceholder to pass the
resolved string through the i18n translator (t) — e.g. call
t(nameFilterPlaceholder || STATIC_SEARCH_FILTERS_PLACEHOLDERS.name) or use the
conditional form t(nameFilterPlaceholder) :
t(STATIC_SEARCH_FILTERS_PLACEHOLDERS.name) so
STATIC_SEARCH_FILTERS_PLACEHOLDERS.name is translated when used as the fallback;
reference getSearchTextPlaceholder and STATIC_SEARCH_FILTERS_PLACEHOLDERS.name
when making the change.
In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx`:
- Around line 23-27: Object.keys(NumberOperator) is being treated as string[]
which allows unsafe access into numberOperatorInfo; cast the result to
NumberOperator[] (e.g., const operators = Object.keys(NumberOperator) as
NumberOperator[]) before mapping so the map uses a properly typed array and you
can safely access numberOperatorInfo[operator]; update the mapping that builds
content/selected/value to iterate over that typed operators array and keep
references to NumberOperator, numberOperatorInfo and selected unchanged.
In `@src/utils/components/SSHAccess/constants.ts`:
- Around line 12-16: serviceTypeTitles contains human-readable strings used via
t(serviceTypeTitles[sshServiceType]) which static i18n extractors cannot find;
open the i18n.ts module and add explicit dummy t() calls for each literal used
(e.g., the three strings 'SSH over LoadBalancer', 'SSH over NodePort', 'None')
so the extractor registers them—this ensures the keys referenced by
serviceTypeTitles (and used with t()) are preserved during i18n extraction.
In
`@src/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/StorageSection/constants.ts`:
- Around line 55-93: sourceOptions in constants.ts contains user-facing
label/description strings but SelectSourceOption.tsx renders them raw; update
SelectSourceOption.tsx to use the translation hook and wrap those strings at
render time. Specifically, import and call useKubevirtTranslation (or the
appropriate t() from your i18n hook) inside SelectSourceOption.tsx, then pass
t(description) and t(label) (or t() around the extracted description/label from
sourceOptions) into FormPFSelect and SelectOption instead of the raw values so
the UI shows translated text. Ensure you only translate at render time in
SelectSourceOption.tsx and do not change the sourceOptions shape in
constants.ts.
In
`@src/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/TemplateInfoSection.tsx`:
- Around line 62-64: The template literal for descriptionData uses
`t(WORKLOADS_LABELS[workload]) ?? t('Other')` which never falls back because
`t()` always returns a string and also lets `isDefaultTemplate &&
t('(default)')` produce the string "false"; change to a guarded lookup and
explicit ternary: check `WORKLOADS_LABELS[workload]` first and call `t(...)`
only if that exists, otherwise use `t('Other')`, and replace `isDefaultTemplate
&& t('(default)')` with a ternary that appends `t('(default)')` only when
`isDefaultTemplate` is true (reference: descriptionData prop, WORKLOADS_LABELS,
t(), isDefaultTemplate).
In `@src/views/catalog/templatescatalog/components/TemplatesCatalogRow.tsx`:
- Line 68: The render of the workload label in TemplatesCatalogRow uses
t(WORKLOADS_LABELS?.[workload]) without a fallback, so if workload is unknown
t(undefined) will output "undefined"; update that expression to provide a safe
fallback (e.g., use WORKLOADS_LABELS?.[workload] || workload or
WORKLOADS_LABELS?.[workload] ?? t('Other')) so the translation call always
receives a string — locate the TemplatesCatalogRow component and change the
t(...) invocation that references WORKLOADS_LABELS?.[workload] to include this
fallback.
In `@src/views/catalog/templatescatalog/components/TemplatesCatalogTile.tsx`:
- Line 101: The current expression calls t(...) and then uses ?? t('Other'),
which never runs because t() doesn't return null/undefined; update
TemplatesCatalogTile to compute the fallback key before calling t — e.g., use
the WORKLOADS_LABELS?.[workload] ?? 'Other' as the argument to t (so call
t(fallbackKey)) or use a logical OR to pick the label key first, then pass that
to t; locate the expression in TemplatesCatalogTile.tsx that references
WORKLOADS_LABELS and the t(...) call and move the fallback inside the t call.
In `@src/views/catalog/utils/constants.ts`:
- Around line 1-3: The NOT_SUPPORTED_VM_ERROR constant's message must be passed
through the i18n translator and the ErrorAlert component must render translated
messages: update the code that returns the message in CreateVMFooter-related
logic to use t(NOT_SUPPORTED_VM_ERROR.message) instead of returning
NOT_SUPPORTED_VM_ERROR.message, and modify the ErrorAlert component
(ErrorAlert.tsx) to wrap the displayed {error.message} with t(...) so any
consumer (e.g., CreateVMFooter and TemplatesCatalogDrawerCreateForm) shows the
translated string; ensure you import/use the same t function used across the
project.
In `@src/views/catalog/wizard/tabs/overview/components/WizardOverviewGrid.tsx`:
- Line 315: The current expression
description={t(WORKLOADS_LABELS?.[workloadAnnotation]) ?? t('Other')} never uses
the ?? fallback because t(...) always returns a string; instead, coalesce the
raw label lookup before calling t so missing keys hit the fallback. Change the
call to use t(WORKLOADS_LABELS?.[workloadAnnotation] ?? 'Other') (or compute
const label = WORKLOADS_LABELS?.[workloadAnnotation] ?? 'Other' and then call
t(label)) so WORKLOADS_LABELS, workloadAnnotation and t are referenced as shown
and the fallback is effective.
In
`@src/views/clusteroverview/OverviewTab/vms-per-resource-card/utils/constants.ts`:
- Around line 10-23: The option titles in vmsPerResourceOptions are now plain
strings and must be translated at render time; update the consuming component
VMsPerResourceCard.tsx to call the i18n translator when rendering each scope
title (replace the current {scope.title} with a translation call such as
{t(scope.title)}), ensuring the component imports/has access to t() from
react-i18next and continues to use the same vmsPerResourceOptions constant.
In `@src/views/migrationpolicies/list/utils/constants.ts`:
- Around line 1-3: Move the exported createItems object into the
MigrationPoliciesCreateButton component and wrap the label strings with the
translation function (e.g., inside MigrationPoliciesCreateButton define const
createItems = { form: t('With form'), yaml: t('With YAML') };), then remove the
exported createItems from constants.ts so the component uses the locally
defined, translated createItems instead.
In
`@src/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx`:
- Line 33: The code calls t(WORKLOADS_LABELS[vmWorkload]) without guarding
against WORKLOADS_LABELS[vmWorkload] being undefined; update
VMWorkloadProfileDetailsItem to check that WORKLOADS_LABELS[vmWorkload] is
truthy before calling t — e.g., use a conditional that calls t(...) only when
WORKLOADS_LABELS[vmWorkload] exists, otherwise fall back to vmWorkload (so
replace t(WORKLOADS_LABELS[vmWorkload]) || vmWorkload with a guarded expression
that avoids t(undefined)); refer to the WORKLOADS_LABELS constant and the
vmWorkload prop to locate the change.
In
`@src/views/virtualmachines/details/tabs/configuration/details/DetailsSection.tsx`:
- Line 133: The code calls t(WORKLOADS_LABELS[vmWorkload]) || vmWorkload which
can call t(undefined) and yield the string "undefined" so the fallback never
triggers; change it to first read the map value and guard it before calling t —
i.e., get WORKLOADS_LABELS[vmWorkload] into a variable (e.g., workloadKey) and
if workloadKey is truthy call t(workloadKey) otherwise render vmWorkload (use
the same symbols t, WORKLOADS_LABELS, vmWorkload to locate the change).
🧹 Nitpick comments (1)
src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx (1)
31-31: Pre-existing:screenreaderTextis not translated.
"Loading contents"on line 31 is a user-facing string that should be wrapped witht(). This isn't introduced by this PR, but worth noting since you're already in this file.As per coding guidelines: "Never hardcode user-facing strings; always use the
t()function for internationalization."
...nents/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/utils/constants.ts
Show resolved
Hide resolved
src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx
Outdated
Show resolved
Hide resolved
src/views/catalog/wizard/tabs/overview/components/WizardOverviewGrid.tsx
Outdated
Show resolved
Hide resolved
src/views/clusteroverview/OverviewTab/vms-per-resource-card/utils/constants.ts
Show resolved
Hide resolved
...etailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx
Outdated
Show resolved
Hide resolved
src/views/virtualmachines/details/tabs/configuration/details/DetailsSection.tsx
Outdated
Show resolved
Hide resolved
adamviktora
requested changes
Feb 16, 2026
...umeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.ts
Show resolved
Hide resolved
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
6cb8836 to
5670009
Compare
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aviavissar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Collaborator
|
@aviavissar: This pull request references CNV-76888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx (1)
14-15:⚠️ Potential issue | 🟡 MinorAdd
selectedLabel={t(vmResourceOption)}to render the toggle button with translated text.The dropdown options are correctly translated via
{t(scope.title)}on line 35, but the toggle button displays the raw untranslated English title becauseFormPFSelectis not provided with aselectedLabelprop. Without it, the component uses theselectedvalue (raw English string) as the toggle display text.When
selected={vmResourceOption}is rendered withoutselectedLabel, theSelectTogglecomponent outputs the untranslated text directly. To match the pattern used elsewhere in the codebase (e.g.,WorkloadProfileModal,SSHServiceSelect), passselectedLabel={t(vmResourceOption)}to ensure the toggle button also displays the translated label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx` around lines 14 - 15, The FormPFSelect toggle is showing the raw English title because the component is not given a translated label; update the FormPFSelect usage in VMsPerResourceCard to pass selectedLabel={t(vmResourceOption)} so the toggle displays the localized string (keep vmResourceOption as the selected value and continue translating options with {t(scope.title)}); locate the FormPFSelect render inside VMsPerResourceCard and add the selectedLabel prop using the existing t() translator and vmResourceOption variable.locales/en/plugin__kubevirt-plugin.json (1)
1-2066:⚠️ Potential issue | 🟠 MajorUser-facing strings hardcoded in constants without locale entries prevent translation for non-English users.
Strings were removed from
locales/en/plugin__kubevirt-plugin.jsonbut remain hardcoded in TypeScript constants and are wrapped witht()at render time, breaking i18n for non-English locales:
src/utils/resources/template/utils/constants.ts:WORKLOADS_LABELSandWORKLOADS_DESCRIPTIONS('Desktop', 'High performance', 'Server', 'Balances performance, compatible with a broad range of workloads', etc.)src/utils/utils/constants.ts:NumberOperatortext values ('Equals', 'Greater or equals', 'Greater than', 'Less or equals', 'Less than')When
t(WORKLOADS_DESCRIPTIONS[key])is called inWorkloadProfileModal.tsxand similar components, the translation key no longer exists in the locale file, causingt()to silently fall back to the hardcoded English string for all languages. This means non-English users see English UI labels regardless of their locale selection.Fix: Either restore these strings to
locales/en/plugin__kubevirt-plugin.jsonwith corresponding translations in other locale files, or wrap the string definitions themselves witht()at the point of definition in the constants files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/en/plugin__kubevirt-plugin.json` around lines 1 - 2066, Constants used as translation keys (WORKLOADS_LABELS, WORKLOADS_DESCRIPTIONS, NumberOperator) were left as hardcoded English strings while their keys were removed from locales, causing t(...) to fall back to English; fix by restoring the corresponding entries in the en locale (and other locales as needed) or by moving localization to the constant definitions (wrap the values with t(...) when defining WORKLOADS_LABELS/WORKLOADS_DESCRIPTIONS and NumberOperator) so that callers like WorkloadProfileModal.tsx call t() on actual locale keys rather than raw English literals.src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx (1)
26-35:⚠️ Potential issue | 🟡 Minor
t(undefined)called before guards whenconditionis missing or out-of-range.
conditionisstring | undefined. When it'sundefined(or not a key ofVALUE_TO_LABLE),VALUE_TO_LABLE[condition]resolves toundefined, sot(undefined)is invoked on every render until data loads and theisEmptyguard fires. While i18next handles this gracefully at runtime, it may generate dev-mode warnings.Since
tis a plain function (not a hook), it can safely be called after the early returns:🛠️ Proposed fix
const condition = queryData?.data?.result?.[0]?.value?.[1]; - const label = t(VALUE_TO_LABLE[condition]); if (!loaded || !hyperLoaded) return ( <StackItem> <Skeleton screenreaderText="Loading contents" /> </StackItem> ); if (isEmpty(condition)) return null; + const label = t(VALUE_TO_LABLE[condition]); return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx` around lines 26 - 35, The code calls t(VALUE_TO_LABLE[condition]) before checking condition, which can call t(undefined) and trigger dev warnings; move the label lookup and call to t so it happens only after the early returns (after the loaded/hyperLoaded guard and after the isEmpty(condition) check), and only call t when condition is defined and VALUE_TO_LABLE[condition] is a valid key (use a fallback string if mapping is missing); update the Conditions component to compute label after the guards (referencing VALUE_TO_LABLE, t, condition, loaded, hyperLoaded, and isEmpty).src/utils/components/FirmwareBootloaderModal/utils/constants.ts (1)
10-42:⚠️ Potential issue | 🟠 MajorApply
t()wrapper to bootloader titles and descriptions in render time.The translation hook is imported in both
FirmwareBootloaderModal.tsx(line 6) andTemplateBootloaderModal.tsx(line 14) and used for some strings (e.g.,t('Boot mode')), but the bootloader titles and descriptions from the constants are rendered directly without translation:
FirmwareBootloaderModal.tsxline 62:selectedLabel={BootModeTitles[selectedFirmwareBootloader]}should wrap witht()FirmwareBootloaderModal.tsxlines 66–67:{description}and{title}from bootloaderOptions are rendered withoutt()TemplateBootloaderModal.tsxline 65:selectedLabel={BootModeTitles[selectedFirmwareBootloader]}should wrap witht()The i18n.ts file contains translation comments for all these strings (lines 67–71), indicating they were extracted for translation but never applied. This is a regression from the previous behavior where these strings were translated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/FirmwareBootloaderModal/utils/constants.ts` around lines 10 - 42, Bootloader titles/descriptions in BootModeTitles, defaultBootloaderOptions and s390xBootloaderOptions are currently plain strings and must be translated at render time; update usages in FirmwareBootloaderModal and TemplateBootloaderModal so you call the i18n t() wrapper when rendering: replace selectedLabel={BootModeTitles[selectedFirmwareBootloader]} with selectedLabel={t(BootModeTitles[selectedFirmwareBootloader])} and render option fields with t(title) and t(description) where title/description come from BootloaderOption (e.g., in the map/render of bootloaderOptions use t(option.title) and t(option.description)); do not change the constants file contents—only wrap those values with t() in the components (affecting the selectedLabel prop and the places that render {title} and {description}).
🧹 Nitpick comments (3)
src/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsx (1)
18-21: Consider memoizingcreateItemsto stabilize the prop reference.
createItemsis a new object literal on every render and is passed directly as a prop toListPageCreateDropdown. If that component is memoized (common for SDK list-page components), every parent render will trigger an unnecessary child re-render. Wrapping withuseMemocosts nothing here.♻️ Proposed refactor
- const createItems = { - form: t('From Form'), - yaml: t('With YAML'), - }; + const createItems = React.useMemo( + () => ({ + form: t('From Form'), + yaml: t('With YAML'), + }), + [t], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsx` around lines 18 - 21, createItems is recreated on every render and passed to ListPageCreateDropdown, causing unnecessary re-renders; wrap the object creation in a useMemo inside the MigrationPoliciesCreateButton component (e.g., const createItems = useMemo(() => ({ form: t('From Form'), yaml: t('With YAML') }), [t]) ) so the prop reference is stable when t hasn't changed.src/utils/components/SSHAccess/components/SSHServiceSelect.tsx (1)
50-50: Prefer enum-keyed access for consistency with line 46.Lines 50, 60, and 77 use string-literal dot notation (
serviceTypeTitles.None,.LoadBalancer,.NodePort) while line 46 uses bracket notation (serviceTypeTitles[sshServiceType]). If an enum value changes (e.g.,SERVICE_TYPES.NONE = 'none'), the dot-notation access silently returnsundefined.♻️ Proposed refactor for consistency and refactoring safety
- {t(serviceTypeTitles.None)} + {t(serviceTypeTitles[SERVICE_TYPES.NONE])}- {t(serviceTypeTitles.LoadBalancer)} + {t(serviceTypeTitles[SERVICE_TYPES.LOAD_BALANCER])}- {t(serviceTypeTitles.NodePort)} + {t(serviceTypeTitles[SERVICE_TYPES.NODE_PORT])}Also applies to: 60-60, 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/SSHAccess/components/SSHServiceSelect.tsx` at line 50, The code uses dot notation (serviceTypeTitles.None / .LoadBalancer / .NodePort) which can break if enum values change; update those occurrences to use enum-keyed bracket access instead (e.g. serviceTypeTitles[SERVICE_TYPES.NONE], serviceTypeTitles[SERVICE_TYPES.LOAD_BALANCER], serviceTypeTitles[SERVICE_TYPES.NODE_PORT]) or, where appropriate, reuse the existing sshServiceType variable (serviceTypeTitles[sshServiceType]) as on line 46 so lookups are resilient to enum value changes; update references to serviceTypeTitles and the enum constants accordingly.src/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/URLSourceHelperText.tsx (1)
15-25: Replace three-part sentence fragmentation with theTranscomponent.The three separate
t()calls on lines 19, 22, and 25 translate disjoint fragments of a single sentence. Translators have no way to reorder the words around the embedded<Link>, which breaks grammatical correctness for languages with different sentence structure (e.g., Japanese, German). The TODO on lines 15–16 already acknowledges this.The idiomatic fix is react-i18next's
Transcomponent, which lets you embed a React element as a numbered slot inside a single translation string, giving translators the full sentence context.Transis already used throughout the codebase (e.g.,WelcomeModal.tsx,EjectCDROMModal.tsx,VirtualMachineEmptyState.tsx), so this pattern is established.As per coding guidelines: "Use complete sentences in translation strings, avoid string concatenation for translated content, and keep context in a single string using interpolation for dynamic parts."
♻️ Proposed refactor using `Trans`
import React, { FC } from 'react'; import { Link } from 'react-router-dom-v5-compat'; +import { Trans } from 'react-i18next'; import { useKubevirtTranslation } from '@kubevirt-utils/hooks/useKubevirtTranslation'; import { OS_IMAGE_LINKS, OS_NAME_TYPES } from '@kubevirt-utils/resources/template'; import { urlSourceHelperTextMapper } from './utils/constants'; import { DiskSourceUrlInputProps } from './utils/types'; const URLSourceHelperText: FC<DiskSourceUrlInputProps> = ({ os }) => { const { t } = useKubevirtTranslation(); - const { afterLabelText, beforeLabelText, label } = - urlSourceHelperTextMapper[os || OS_NAME_TYPES.fedora]; - - // TODO: Replace three-part concatenation with single interpolated translation key - // to avoid i18n concatenation issues (e.g., t('Example: For {{os}}, visit the {{link}} and copy...')) + const { fullSentence } = urlSourceHelperTextMapper[os || OS_NAME_TYPES.fedora]; return ( - <> - {t(beforeLabelText)} - <strong> - <Link rel="noreferrer" target="_blank" to={OS_IMAGE_LINKS[os || OS_NAME_TYPES.fedora]}> - {t(label)} - </Link> - </strong> - {t(afterLabelText)} - </> + <Trans + t={t} + defaults={fullSentence} + components={[ + <strong key="link"> + <Link rel="noreferrer" target="_blank" to={OS_IMAGE_LINKS[os || OS_NAME_TYPES.fedora]} /> + </strong>, + ]} + /> ); };Update
urlSourceHelperTextMapperentries to use a single string with a component slot:[OS_NAME_TYPES.centos]: { fullSentence: 'Example: For CentOS, visit the <0>CentOS cloud image list</0> and copy the download link URL for the cloud base image', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/URLSourceHelperText.tsx` around lines 15 - 25, Current code concatenates three separate t() calls (beforeLabelText, label, afterLabelText) in URLSourceHelperText.tsx which prevents translators from reordering sentence parts; replace this with react-i18next's Trans so the whole sentence is one translation with an embedded component. Update the urlSourceHelperTextMapper entries to provide a single translation string (e.g., "Example: For {{os}}, visit the <0>CentOS cloud image list</0> and copy..."), then in the URLSourceHelperText component render <Trans> with the Link as the <0> child (using OS_IMAGE_LINKS[os || OS_NAME_TYPES.fedora] for the to prop) instead of calling t(beforeLabelText)/t(label)/t(afterLabelText); ensure you keep the dynamic os/label values as interpolation/slots and remove the three-part concatenation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.ts`:
- Around line 31-37: The constants file currently contains hardcoded labels
('Red Hat provided', 'User provided') and module-level seriesLabel fallbacks
that bypass runtime translation; fix by removing module-level t() usage and
ensuring translations happen at render time: update DrilldownMenuItemWrapper to
prefer the translated defaultSeriesLabel (call t() when rendering) instead of
falling back to seriesLabel, and add a small translation wrapper or helper
around initialMenuItems inside InstanceTypeDrilldownSelect that maps each menu
item (the objects in initialMenuItems / MENUS entries) to produce their label
via t() at render time so menu labels are translated when rendered.
In
`@src/views/templates/list/components/VirtualMachineTemplatesSource/VirtualMachineTemplatesSource.tsx`:
- Line 55: The boot source label strings are not registered for static i18n
extraction because VirtualMachineTemplatesSource.tsx calls t(bootSourceLabel)
with a variable; open src/utils/i18n.ts and add a Boot source labels block
mirroring the WORKLOADS_LABELS pattern that registers the keys used by
getVMBootSourceLabel() (e.g., "Container disk", "PVC", "No boot source",
"Registry", "Snapshot") so the extractor picks them up and translations are
emitted for VirtualMachineTemplatesSource and components that call
t(bootSourceLabel).
---
Outside diff comments:
In `@locales/en/plugin__kubevirt-plugin.json`:
- Around line 1-2066: Constants used as translation keys (WORKLOADS_LABELS,
WORKLOADS_DESCRIPTIONS, NumberOperator) were left as hardcoded English strings
while their keys were removed from locales, causing t(...) to fall back to
English; fix by restoring the corresponding entries in the en locale (and other
locales as needed) or by moving localization to the constant definitions (wrap
the values with t(...) when defining WORKLOADS_LABELS/WORKLOADS_DESCRIPTIONS and
NumberOperator) so that callers like WorkloadProfileModal.tsx call t() on actual
locale keys rather than raw English literals.
In `@src/utils/components/FirmwareBootloaderModal/utils/constants.ts`:
- Around line 10-42: Bootloader titles/descriptions in BootModeTitles,
defaultBootloaderOptions and s390xBootloaderOptions are currently plain strings
and must be translated at render time; update usages in FirmwareBootloaderModal
and TemplateBootloaderModal so you call the i18n t() wrapper when rendering:
replace selectedLabel={BootModeTitles[selectedFirmwareBootloader]} with
selectedLabel={t(BootModeTitles[selectedFirmwareBootloader])} and render option
fields with t(title) and t(description) where title/description come from
BootloaderOption (e.g., in the map/render of bootloaderOptions use
t(option.title) and t(option.description)); do not change the constants file
contents—only wrap those values with t() in the components (affecting the
selectedLabel prop and the places that render {title} and {description}).
In
`@src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx`:
- Around line 14-15: The FormPFSelect toggle is showing the raw English title
because the component is not given a translated label; update the FormPFSelect
usage in VMsPerResourceCard to pass selectedLabel={t(vmResourceOption)} so the
toggle displays the localized string (keep vmResourceOption as the selected
value and continue translating options with {t(scope.title)}); locate the
FormPFSelect render inside VMsPerResourceCard and add the selectedLabel prop
using the existing t() translator and vmResourceOption variable.
In
`@src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx`:
- Around line 26-35: The code calls t(VALUE_TO_LABLE[condition]) before checking
condition, which can call t(undefined) and trigger dev warnings; move the label
lookup and call to t so it happens only after the early returns (after the
loaded/hyperLoaded guard and after the isEmpty(condition) check), and only call
t when condition is defined and VALUE_TO_LABLE[condition] is a valid key (use a
fallback string if mapping is missing); update the Conditions component to
compute label after the guards (referencing VALUE_TO_LABLE, t, condition,
loaded, hyperLoaded, and isEmpty).
---
Duplicate comments:
In `@src/utils/components/ListPageFilter/constants.ts`:
- Around line 6-14: Search all usages of STATIC_SEARCH_FILTERS_LABELS and
STATIC_SEARCH_FILTERS_PLACEHOLDERS and ensure every consumer calls the
translation function at render time (i.e., wrap accesses with t(...)) rather
than relying on pre-translated values; specifically audit callers such as
TextFiltersToolbarItem.tsx and getSearchTextPlaceholder in utils.ts and update
any usage that directly reads STATIC_SEARCH_FILTERS_LABELS.name/labels or
STATIC_SEARCH_FILTERS_PLACEHOLDERS.* to instead pass the raw string into t(...)
at render time so no code path returns untranslated constants.
In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx`:
- Line 22: The current line const operators = Object.keys(NumberOperator) as
NumberOperator[] is correct for this string enum where keys equal values; keep
this type assertion as-is and no change is required in NumberOperatorSelect
(referencing the operators constant and NumberOperator enum).
In `@src/utils/components/SSHAccess/components/SSHServiceSelect.tsx`:
- Line 46: The t() calls in SSHServiceSelect
(selectedLabel={t(serviceTypeTitles[sshServiceType])} and other uses around
lines 50, 60, 77) pass dynamic keys (serviceTypeTitles[...] / sshServiceType) so
static i18n extractors won't discover the literal strings; open
src/utils/i18n.ts and ensure the three human-readable keys (e.g., "SSH over
LoadBalancer", "SSH over NodePort", "plugin__kubevirt-plugin~None" or whichever
exact strings are in serviceTypeTitles) are explicitly registered via the
extractor comments/registration block used in that file; if missing, add those
registration comments or a marked list of string literals so the extraction tool
picks them up.
In `@src/utils/components/SSHAccess/constants.ts`:
- Around line 12-16: The module-level t() calls caused translations to only
refresh on reload; ensure serviceTypeTitles (the object keyed by SERVICE_TYPES)
contains plain string literals (as shown) and move all i18n t() invocations into
the render path of SSHServiceSelect (e.g., inside its component function where
the selected SERVICE_TYPES value is mapped to a display label), and confirm the
i18n extraction/registration script referenced in the SSHServiceSelect.tsx
review comment is run so those keys are picked up for translation.
In `@src/views/catalog/utils/constants.ts`:
- Around line 1-4: The NOT_SUPPORTED_VM_ERROR constant's message should be
treated as an i18n key rather than a human-readable string; confirm that
NOT_SUPPORTED_VM_ERROR.message remains the key string (not wrapped in t() here)
and that callers (e.g., where utils.ts uses t(NOT_SUPPORTED_VM_ERROR.message))
perform the translation—leave the export of NOT_SUPPORTED_VM_ERROR unchanged and
ensure no other call sites directly display the raw message string.
---
Nitpick comments:
In
`@src/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/URLSourceHelperText.tsx`:
- Around line 15-25: Current code concatenates three separate t() calls
(beforeLabelText, label, afterLabelText) in URLSourceHelperText.tsx which
prevents translators from reordering sentence parts; replace this with
react-i18next's Trans so the whole sentence is one translation with an embedded
component. Update the urlSourceHelperTextMapper entries to provide a single
translation string (e.g., "Example: For {{os}}, visit the <0>CentOS cloud image
list</0> and copy..."), then in the URLSourceHelperText component render <Trans>
with the Link as the <0> child (using OS_IMAGE_LINKS[os || OS_NAME_TYPES.fedora]
for the to prop) instead of calling
t(beforeLabelText)/t(label)/t(afterLabelText); ensure you keep the dynamic
os/label values as interpolation/slots and remove the three-part concatenation.
In `@src/utils/components/SSHAccess/components/SSHServiceSelect.tsx`:
- Line 50: The code uses dot notation (serviceTypeTitles.None / .LoadBalancer /
.NodePort) which can break if enum values change; update those occurrences to
use enum-keyed bracket access instead (e.g.
serviceTypeTitles[SERVICE_TYPES.NONE],
serviceTypeTitles[SERVICE_TYPES.LOAD_BALANCER],
serviceTypeTitles[SERVICE_TYPES.NODE_PORT]) or, where appropriate, reuse the
existing sshServiceType variable (serviceTypeTitles[sshServiceType]) as on line
46 so lookups are resilient to enum value changes; update references to
serviceTypeTitles and the enum constants accordingly.
In
`@src/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsx`:
- Around line 18-21: createItems is recreated on every render and passed to
ListPageCreateDropdown, causing unnecessary re-renders; wrap the object creation
in a useMemo inside the MigrationPoliciesCreateButton component (e.g., const
createItems = useMemo(() => ({ form: t('From Form'), yaml: t('With YAML') }),
[t]) ) so the prop reference is stable when t hasn't changed.
...umeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.ts
Show resolved
Hide resolved
...ws/templates/list/components/VirtualMachineTemplatesSource/VirtualMachineTemplatesSource.tsx
Show resolved
Hide resolved
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
5670009 to
20b2604
Compare
Collaborator
|
@aviavissar: This pull request references CNV-76888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx (1)
31-31:⚠️ Potential issue | 🟡 MinorPre-existing:
screenreaderTextis a user-facing accessibility string and should be translated
screenreaderText="Loading contents"is announced to screen-reader users and is not wrapped witht(). While this is not introduced by this PR, it is directly in scope of the broader i18n fix being made here.✏️ Proposed fix
- <Skeleton screenreaderText="Loading contents" /> + <Skeleton screenreaderText={t('Loading contents')} />Based on learnings: "Applies to **/*.tsx : Translate labels, headings, button text, error messages, help text, tooltips, and placeholders."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx` at line 31, The Skeleton's screenreaderText prop is a user-facing string that must be localized; replace the hard-coded "Loading contents" with a translated string (use t('Loading contents') or a namespaced key) and ensure the component calls useTranslation() to get t; specifically update the Skeleton usage in Conditions (the call to Skeleton and its screenreaderText prop) and add/import useTranslation from react-i18next (or the project's i18n helper) and call it (e.g., const { t } = useTranslation()) so screenreaderText uses the t function.
🧹 Nitpick comments (6)
src/views/virtualmachines/details/tabs/configuration/details/DetailsSection.tsx (1)
133-136: Optional: replace IIFE with a pre-computed variable for cleaner JSX.An IIFE inside a JSX prop value adds cognitive overhead. Since
vmWorkloadandWORKLOADS_LABELSare both available before thereturn, the label can be derived there.♻️ Proposed refactor
Add a derived variable alongside the existing pre-
returncomputations (around line 97):const isExpandableSpec = isExpandableSpecVM(vm); const deletionProtectionEnabled = isDeletionProtectionEnabled(vm); + const workloadKey = WORKLOADS_LABELS[vmWorkload]; + const workloadLabel = workloadKey ? t(workloadKey) : vmWorkload;Then simplify the JSX prop:
- vmWorkload ? ( - (() => { - const workloadKey = WORKLOADS_LABELS[vmWorkload]; - return workloadKey ? t(workloadKey) : vmWorkload; - })() - ) : ( + vmWorkload ? ( + workloadLabel + ) : ( <MutedTextSpan text={t('Not available')} /> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/virtualmachines/details/tabs/configuration/details/DetailsSection.tsx` around lines 133 - 136, Replace the IIFE used to render the workload label by computing the label before the component's return: derive a const (e.g., workloadLabel) using WORKLOADS_LABELS[vmWorkload] ? t(WORKLOADS_LABELS[vmWorkload]) : vmWorkload near the existing pre-return computations in DetailsSection (so it uses vmWorkload and t), then replace the IIFE in the JSX with that workloadLabel variable.src/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsx (1)
18-21: Optional: wrapcreateItemsinuseMemo.
createItemsis a new object on every render. While the allocation is trivially cheap here, the component ordering guidelines recommenduseMemofor values derived from hook results. Wrapping avoids redundant object creation and keepscreateItemsco-located with the other memoized values in the render cycle.♻️ Proposed refactor
- const createItems = { - form: t('From Form'), - yaml: t('With YAML'), - }; + const createItems = React.useMemo( + () => ({ + form: t('From Form'), + yaml: t('With YAML'), + }), + [t], + );Based on learnings: "Use
useMemofor expensive calculations" and the component-function ordering guideline:useKubevirtTranslation, router hooks, data fetching hooks,useState,useMemo,useCallback,useEffect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsx` around lines 18 - 21, The object createItems is recreated on every render; wrap its definition in React's useMemo to memoize it (e.g., const createItems = useMemo(() => ({ form: t('From Form'), yaml: t('With YAML') }), [t])) and ensure useMemo is imported; this keeps createItems stable across renders and follows the existing hook ordering alongside useKubevirtTranslation and other memoized values.src/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.ts (1)
7-33: Missing explicit return type ongetDisableButtonTooltipContent.The function can return
string | undefinedbut no return type is declared. As per coding guidelines, all functions must have explicit return types.♻️ Proposed fix
-export const getDisableButtonTooltipContent = ( +export const getDisableButtonTooltipContent = ( instanceTypeVMStore: InstanceTypeVMStore, canCreate: boolean, udnConfigurationIssue: boolean, -) => { +): string | undefined => {As per coding guidelines: "Always define explicit return types on functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.ts` around lines 7 - 33, The function getDisableButtonTooltipContent currently lacks an explicit return type; add an explicit return type of string | undefined to its signature (for the function that accepts InstanceTypeVMStore, canCreate, udnConfigurationIssue) so the compiler and reviewers see it returns either a localized string or undefined; update the declaration for getDisableButtonTooltipContent to include : string | undefined and ensure any related helper types/exports (e.g., NOT_SUPPORTED_VM_ERROR, isValidVMName) remain unchanged.src/utils/components/ListPageFilter/utils.ts (1)
156-170:nameFilterPlaceholdertype is too narrow andgetSearchTextPlaceholderlacks an explicit return typeThe truthiness check on line 162 (
nameFilterPlaceholder ? ...) signals the parameter can be falsy (undefinedor empty string), but the declared type isstring. UnderstrictNullChecksthis means callers cannot legally passundefined, even though the function is clearly designed to handle that case. The parameter should bestring | undefined.Additionally, the coding guideline requires all functions to have explicit return types.
♻️ Proposed fix
-export const getSearchTextPlaceholder = ( +export const getSearchTextPlaceholder = ( searchType: string, selectedSearchFilter: RowFilter, - nameFilterPlaceholder: string, -) => { + nameFilterPlaceholder: string | undefined, +): string => {As per coding guidelines: "Always define explicit return types on functions" and "Use optional chaining (
?.) for nullable access."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/ListPageFilter/utils.ts` around lines 156 - 170, Update the getSearchTextPlaceholder signature to allow a nullable placeholder and add an explicit return type: change the nameFilterPlaceholder parameter to accept undefined (e.g., nameFilterPlaceholder?: string or nameFilterPlaceholder: string | undefined) and declare the function return type as string; keep (or ensure) optional chaining when accessing selectedSearchFilter?.filterGroupName so the fallback t('Search by {{filterName}}...') call is safe. This touches the getSearchTextPlaceholder function and its parameters in utils.ts.src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/components/DrilldownMenuItem/DrilldownMenuItemWrapper.tsx (1)
45-45: RedundantIcon && Iconexpression.
Icon && Iconevaluates toIconwhen truthy andundefined/false-ish when falsy — equivalent to justIcon. Since line 45 is part of this diff, consider simplifying.♻️ Simplify `Icon` prop
- <DrilldownMenuItem Icon={Icon && Icon} id={seriesName} key={seriesName} label={seriesLabel}> + <DrilldownMenuItem Icon={Icon} id={seriesName} key={seriesName} label={seriesLabel}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/components/DrilldownMenuItem/DrilldownMenuItemWrapper.tsx` at line 45, The prop expression Icon={Icon && Icon} in DrilldownMenuItem inside DrilldownMenuItemWrapper is redundant; update the DrilldownMenuItem usage to pass the Icon prop directly (Icon={Icon}) so the component receives the same value without the unnecessary logical check, keeping seriesName and seriesLabel as the id/key/label props unchanged.src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx (1)
33-33: Consider memoizing theonSelecthandler withuseCallback.The inline arrow function creates a new reference on every render. Wrapping it with
useCallbackis consistent with the component already usinguseMemo, and avoids unnecessary re-renders ifSimpleSelectever becomes memoized.♻️ Proposed refactor
+ const handleSelect = useCallback( + (_: unknown, selection: NumberOperator) => onSelect(selection), + [onSelect], + ); + return ( <SimpleSelect initialOptions={initialOptions} - onSelect={(_, selection: NumberOperator) => onSelect(selection)} + onSelect={handleSelect} {...getSelectDataTestProps(dataTest)} /> );Also add
useCallbackto the React import on line 1:-import React, { FC, useMemo } from 'react'; +import React, { FC, useCallback, useMemo } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx` at line 33, The inline prop handler on the NumberOperatorSelect component (onSelect={(_, selection: NumberOperator) => onSelect(selection)}) creates a new function each render; wrap it in React.useCallback to memoize it: import useCallback from React and create a callback like const handleSelect = useCallback((_: unknown, selection: NumberOperator) => onSelect(selection), [onSelect]) and pass handleSelect to SimpleSelect’s onSelect prop; this keeps behavior identical but prevents unnecessary re-renders when SimpleSelect is memoized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/utils/components/DiskModal/components/DiskInterfaceSelect/DiskInterfaceSelect.tsx`:
- Line 31: selectedLabel may pass undefined into the i18n function t() when
diskInterface (from disk.bus) is not a known key in diskInterfaceOptions; update
the DiskInterfaceSelect logic to guard/fallback so t() always receives a string
by using the diskInterfaceOptions lookup (diskInterfaceOptions, diskInterface)
and fallback to an empty string (or a safe default label) when the label is
missing; locate the selectedLabel assignment in DiskInterfaceSelect and change
it to call t(...) with a nullish-coalesced or conditional fallback to ensure the
argument is never undefined.
In `@src/utils/components/ListPageFilter/utils.ts`:
- Around line 166-170: STATIC_SEARCH_FILTERS_PLACEHOLDERS is a const object with
literal keys but the code indexes it with searchType: string, which is unsafe;
narrow searchType to the actual key union before indexing (e.g., create a type
alias like type PlaceholderKey = keyof typeof STATIC_SEARCH_FILTERS_PLACEHOLDERS
and cast/validate searchType as PlaceholderKey) and then use that typed key when
accessing STATIC_SEARCH_FILTERS_PLACEHOLDERS to ensure type safety while keeping
the existing fallback (t('Search by {{filterName}}...', { filterName:
selectedSearchFilter?.filterGroupName })) intact.
In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx`:
- Around line 21-28: NumberOperatorSelect currently computes initialOptions with
useMemo but passes them to SimpleSelect which only reads initialOptions on
mount, so updates to the selected prop are ignored; fix by forcing SimpleSelect
to remount when selection changes: add a key prop to the SimpleSelect instance
derived from the selected value (e.g., key={`number-operator-${selected}`}) so
the component unmounts/remounts when selected changes, or alternatively replace
SimpleSelect with the lower-level Select from `@patternfly/react-core` and render
options from the current initialOptions each render to support true controlled
selection; update references in NumberOperatorSelect (initialOptions,
SimpleSelect, selected, numberOperatorInfo) accordingly.
In
`@src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx`:
- Line 26: Compute the translated label only after the early-return guards or
provide a nullish fallback to avoid calling t(undefined): move the line that
sets label (currently using t(VALUE_TO_LABLE[condition])) to after the
loading/error guards (so it runs only when condition is defined), or change it
to call t(VALUE_TO_LABLE[condition] ?? 'fallbackKey') (or similar) so t never
receives undefined; reference the VALUE_TO_LABLE lookup and the t(...)
invocation in Conditions.tsx to locate the change.
---
Outside diff comments:
In
`@src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx`:
- Line 31: The Skeleton's screenreaderText prop is a user-facing string that
must be localized; replace the hard-coded "Loading contents" with a translated
string (use t('Loading contents') or a namespaced key) and ensure the component
calls useTranslation() to get t; specifically update the Skeleton usage in
Conditions (the call to Skeleton and its screenreaderText prop) and add/import
useTranslation from react-i18next (or the project's i18n helper) and call it
(e.g., const { t } = useTranslation()) so screenreaderText uses the t function.
---
Duplicate comments:
In
`@src/views/clusteroverview/OverviewTab/vms-per-resource-card/utils/constants.ts`:
- Around line 10-23: The module-level constants in vmsPerResourceOptions were
calling t() at import time which freezes the locale; remove any t() usage from
the vmsPerResourceOptions initializer so entries use plain title keys (as shown:
'Show VirtualMachine per Templates', 'Show VirtualMachine per InstanceTypes',
UNCATEGORIZED_VM) and ensure translation happens at render time in the consumer
(VMsPerResourceCard.tsx) by calling t(scope.title); update or confirm code that
references vmsPerResourceOptions, TemplateModel.kind,
VirtualMachineClusterInstancetypeModel.kind, and UNCATEGORIZED_VM to use the
plain string keys and perform translation inside the component render.
In
`@src/views/templates/list/components/VirtualMachineTemplatesSource/VirtualMachineTemplatesSource.tsx`:
- Line 55: The runtime call t(bootSourceLabel) uses variable translation keys
(from getVMBootSourceLabel / BOOT_SOURCE_LABELS) which static extractors can't
find, so add comment-style registration entries for the boot source strings
("Container disk", "No boot source", "Registry", "Snapshot", "PVC", etc.) into
src/utils/i18n.ts so they are picked up by static extraction; locate
BOOT_SOURCE_LABELS and getVMBootSourceLabel to enumerate exact keys/phrases and
add matching i18n comment registrations in i18n.ts.
---
Nitpick comments:
In
`@src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/components/DrilldownMenuItem/DrilldownMenuItemWrapper.tsx`:
- Line 45: The prop expression Icon={Icon && Icon} in DrilldownMenuItem inside
DrilldownMenuItemWrapper is redundant; update the DrilldownMenuItem usage to
pass the Icon prop directly (Icon={Icon}) so the component receives the same
value without the unnecessary logical check, keeping seriesName and seriesLabel
as the id/key/label props unchanged.
In `@src/utils/components/ListPageFilter/utils.ts`:
- Around line 156-170: Update the getSearchTextPlaceholder signature to allow a
nullable placeholder and add an explicit return type: change the
nameFilterPlaceholder parameter to accept undefined (e.g.,
nameFilterPlaceholder?: string or nameFilterPlaceholder: string | undefined) and
declare the function return type as string; keep (or ensure) optional chaining
when accessing selectedSearchFilter?.filterGroupName so the fallback t('Search
by {{filterName}}...') call is safe. This touches the getSearchTextPlaceholder
function and its parameters in utils.ts.
In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx`:
- Line 33: The inline prop handler on the NumberOperatorSelect component
(onSelect={(_, selection: NumberOperator) => onSelect(selection)}) creates a new
function each render; wrap it in React.useCallback to memoize it: import
useCallback from React and create a callback like const handleSelect =
useCallback((_: unknown, selection: NumberOperator) => onSelect(selection),
[onSelect]) and pass handleSelect to SimpleSelect’s onSelect prop; this keeps
behavior identical but prevents unnecessary re-renders when SimpleSelect is
memoized.
In
`@src/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.ts`:
- Around line 7-33: The function getDisableButtonTooltipContent currently lacks
an explicit return type; add an explicit return type of string | undefined to
its signature (for the function that accepts InstanceTypeVMStore, canCreate,
udnConfigurationIssue) so the compiler and reviewers see it returns either a
localized string or undefined; update the declaration for
getDisableButtonTooltipContent to include : string | undefined and ensure any
related helper types/exports (e.g., NOT_SUPPORTED_VM_ERROR, isValidVMName)
remain unchanged.
In
`@src/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsx`:
- Around line 18-21: The object createItems is recreated on every render; wrap
its definition in React's useMemo to memoize it (e.g., const createItems =
useMemo(() => ({ form: t('From Form'), yaml: t('With YAML') }), [t])) and ensure
useMemo is imported; this keeps createItems stable across renders and follows
the existing hook ordering alongside useKubevirtTranslation and other memoized
values.
In
`@src/views/virtualmachines/details/tabs/configuration/details/DetailsSection.tsx`:
- Around line 133-136: Replace the IIFE used to render the workload label by
computing the label before the component's return: derive a const (e.g.,
workloadLabel) using WORKLOADS_LABELS[vmWorkload] ?
t(WORKLOADS_LABELS[vmWorkload]) : vmWorkload near the existing pre-return
computations in DetailsSection (so it uses vmWorkload and t), then replace the
IIFE in the JSX with that workloadLabel variable.
src/utils/components/DiskModal/components/DiskInterfaceSelect/DiskInterfaceSelect.tsx
Outdated
Show resolved
Hide resolved
src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsx
Outdated
Show resolved
Hide resolved
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
20b2604 to
ee47c83
Compare
Collaborator
|
@aviavissar: This pull request references CNV-76888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx (1)
14-14:⚠️ Potential issue | 🟠 MajorToggle button will display untranslated English text; setter naming violates camelCase convention.
The toggle button displays the raw English title because:
- Line 30 passes
selected={vmResourceOption}(raw English, e.g.,'Show VirtualMachine per Templates')- FormPFSelect does not receive a
selectedLabelprop to override the toggle display- SelectToggle renders the
selectedstring directly in the MenuToggle- While the dropdown list items are correctly translated via
{t(scope.title)}on line 35, the toggle button shows the untranslated English fromvmResourceOptionFix: Pass a translated label for toggle display while keeping the raw English as the matching key:
Suggested fix
- const [vmResourceOption, setvmResourceOption] = useState(vmsPerResourceOptions[0]?.title); + const [vmResourceOption, setVmResourceOption] = useState(vmsPerResourceOptions[0]?.title); const handleSelect = (_event, value) => { const selected = vmsPerResourceOptions?.find((option) => option.title === value); - setvmResourceOption(selected?.title); + setVmResourceOption(selected?.title); setType(selected?.type); }; return ( <Card data-test-id="vms-per-template-card"> <CardHeader actions={{ actions: ( <FormPFSelect onSelect={handleSelect} selected={vmResourceOption} + selectedLabel={vmResourceOption ? t(vmResourceOption) : undefined} toggleProps={{ id: 'overview-vms-per-resource-card' }} >Also fix the setter name
setvmResourceOptiontosetVmResourceOption(camelCase convention).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx` at line 14, The toggle shows untranslated English because the component passes the raw key (vmResourceOption) into SelectToggle via selected and FormPFSelect lacks a translated selectedLabel; also the state setter name setvmResourceOption violates camelCase. Fix by renaming setvmResourceOption to setVmResourceOption and update state usage around vmResourceOption, and change the FormPFSelect/SelectToggle usage to pass a translated label (e.g., compute selectedLabel = t(vmResourceOption) or map vmResourceOption through vmsPerResourceOptions to call t(scope.title)) while keeping vmResourceOption as the key for selection so the dropdown items remain translated with t(scope.title).
🧹 Nitpick comments (6)
src/utils/components/ListPageFilter/utils.ts (1)
156-157: HoistisValidPlaceholderKeyto module scopeThe type guard (lines 168–169) is re-created on every call to
getSearchTextPlaceholder. Since it closes over nothing and is logically paired withPlaceholderKey, it belongs at module level immediately after the type alias on line 157.♻️ Proposed refactor
type PlaceholderKey = keyof typeof STATIC_SEARCH_FILTERS_PLACEHOLDERS; +const isValidPlaceholderKey = (key: string): key is PlaceholderKey => + key in STATIC_SEARCH_FILTERS_PLACEHOLDERS; export const getSearchTextPlaceholder = ( searchType: string, selectedSearchFilter: RowFilter, nameFilterPlaceholder: string, ) => { if (searchType === STATIC_SEARCH_FILTERS.name) return nameFilterPlaceholder ? t(nameFilterPlaceholder) : t(STATIC_SEARCH_FILTERS_PLACEHOLDERS.name); - const isValidPlaceholderKey = (key: string): key is PlaceholderKey => - key in STATIC_SEARCH_FILTERS_PLACEHOLDERS; - return isValidPlaceholderKey(searchType) ? t(STATIC_SEARCH_FILTERS_PLACEHOLDERS[searchType]) : t('Search by {{filterName}}...', { filterName: selectedSearchFilter?.filterGroupName, }); };Also applies to: 168-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/ListPageFilter/utils.ts` around lines 156 - 157, The local type guard isValidPlaceholderKey currently recreated inside getSearchTextPlaceholder; move it to module scope right after the type alias PlaceholderKey so it’s defined once and reused; update getSearchTextPlaceholder to call the module-level isValidPlaceholderKey and remove the nested definition (ensure the guard signature remains: (k: string | undefined) => k is PlaceholderKey and references STATIC_SEARCH_FILTERS_PLACEHOLDERS as before).src/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.ts (1)
7-11: Optional: add explicit return type to the function.The function implicitly returns
string | undefinedbut carries no return type annotation. Per coding guidelines, all functions should declare an explicit return type.♻️ Proposed change
-export const getDisableButtonTooltipContent = ( +export const getDisableButtonTooltipContent = ( instanceTypeVMStore: InstanceTypeVMStore, canCreate: boolean, udnConfigurationIssue: boolean, -) => { +): string | undefined => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.ts` around lines 7 - 11, The function getDisableButtonTooltipContent currently has an implicit return type; add an explicit return type annotation (string | undefined) to its declaration so the signature reads something like export const getDisableButtonTooltipContent = (...): string | undefined => { ... }; ensure the annotation matches the actual returns and references the existing parameter types (InstanceTypeVMStore, canCreate, udnConfigurationIssue) without changing implementation.src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx (1)
34-34: Consider memoizing theonSelecthandler withuseCallback.The inline arrow function is recreated on every render. While the impact is low here (since
SimpleSelectalready remounts onkeychanges and isn't a memoized child), wrapping it aligns with the project convention of stable callback references.♻️ Proposed refactor
const NumberOperatorSelect: FC<NumberOperatorSelectProps> = ({ 'data-test': dataTest, onSelect, selected, }) => { const { t } = useKubevirtTranslation(); + + const handleSelect = useCallback( + (_: unknown, selection: NumberOperator) => onSelect(selection), + [onSelect], + ); const initialOptions = useMemo<SimpleSelectOption[]>(() => { ... }, [selected, t]); return ( <SimpleSelect initialOptions={initialOptions} key={`number-operator-${selected}`} - onSelect={(_, selection: NumberOperator) => onSelect(selection)} + onSelect={handleSelect} {...getSelectDataTestProps(dataTest)} /> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx` at line 34, The inline arrow passed to SimpleSelect's onSelect prop is recreated each render; wrap it in a useCallback to provide a stable reference. In NumberOperatorSelect create a memoized handler like handleSelect = useCallback((_, selection: NumberOperator) => onSelect(selection), [onSelect]) (import useCallback if not present) and pass handleSelect to SimpleSelect's onSelect prop instead of the inline function.src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx (1)
18-18: Optional chaining on a static import is unnecessary.
vmsPerResourceOptionsis a module-level constant and can never benullorundefined;?.find()and?.map()add noise without defensive value.♻️ Proposed cleanup
- const selected = vmsPerResourceOptions?.find((option) => option.title === value); + const selected = vmsPerResourceOptions.find((option) => option.title === value); ... - {vmsPerResourceOptions?.map((scope) => ( + {vmsPerResourceOptions.map((scope) => (Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx` at line 18, The code uses optional chaining on the module-level constant vmsPerResourceOptions (e.g., vmsPerResourceOptions?.find(...) and vmsPerResourceOptions?.map(...)) even though that constant cannot be null/undefined; remove the unnecessary ?. so that you call vmsPerResourceOptions.find(...) and vmsPerResourceOptions.map(...). Update the references around the selected variable assignment (const selected = vmsPerResourceOptions.find((option) => option.title === value)) and any mapping logic to drop the optional chaining.src/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx (1)
25-36: Extract IIFE to a pre-computed variable for readability.The IIFE
(() => { ... })()inside JSX is unusual and adds visual noise. The same logic can be expressed with a simple derived variable before thereturn.♻️ Proposed refactor
const vmWorkload = getWorkload(vm); const vmName = getName(vm); + + const workloadKey = WORKLOADS_LABELS[vmWorkload]; + const workloadLabel = workloadKey ? t(workloadKey) : vmWorkload; return ( !getInstanceTypeMatcher(vm) && ( <DescriptionItem descriptionData={ vmWorkload ? ( - (() => { - const workloadKey = WORKLOADS_LABELS[vmWorkload]; - return workloadKey ? t(workloadKey) : vmWorkload; - })() + workloadLabel ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx` around lines 25 - 36, Extract the IIFE used inside the DescriptionItem JSX into a pre-computed variable before the return: compute a readable variable (e.g., resolvedWorkloadLabel) using vmWorkload, WORKLOADS_LABELS and t (fall back to vmWorkload if no label) and then pass that variable into DescriptionItem; update references around getWorkload(vm), getName(vm) and the conditional using getInstanceTypeMatcher(vm) so the JSX no longer contains the (() => { ... })() expression.src/utils/components/WorkloadProfileModal/WorkloadProfileModal.tsx (1)
11-12: Merge duplicate PatternFly imports.
FormGroupandSelectOptionare both imported from'@patternfly/react-core'on separate lines. These should be consolidated into a single import statement.♻️ Proposed fix
-import { FormGroup } from '@patternfly/react-core'; -import { SelectOption } from '@patternfly/react-core'; +import { FormGroup, SelectOption } from '@patternfly/react-core';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/WorkloadProfileModal/WorkloadProfileModal.tsx` around lines 11 - 12, The imports for PatternFly are duplicated; consolidate the two separate imports into a single import from '@patternfly/react-core' that includes both FormGroup and SelectOption to avoid redundant import lines in WorkloadProfileModal (look for the import statements at the top of WorkloadProfileModal.tsx referencing FormGroup and SelectOption).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/components/DiskModal/components/utils/constants.ts`:
- Around line 1-3: The constants OTHER, CONTAINER_EPHERMAL and DYNAMIC should
remain plain strings in constants.ts but be wrapped with t() where they are
rendered: update DiskRow.tsx so the source render path uses t(OTHER) or
t(CONTAINER_EPHERMAL) instead of outputting the constant directly; update the
readableSizeUnit code (used in DiskRow.tsx) so when it returns a non-numeric
string it returns t(DYNAMIC) instead of DYNAMIC; and update DynamicSize.tsx to
pass t(DYNAMIC) as the TextInput value. Ensure you only add t() at these render
sites (DiskRow.tsx, readableSizeUnit usage, DynamicSize.tsx) and do not change
the constant declarations.
In `@src/utils/components/FirmwareBootloaderModal/utils/constants.ts`:
- Around line 11-14: BootModeTitles must stay as raw keys/strings but be
translated at render time: in FirmwareBootloaderModal (where
BootModeTitles[selectedFirmwareBootloader] is rendered and where options map
supplies title/description), call the t() function from useKubevirtTranslation()
around each BootModeTitles[...] usage; similarly update getBootloaderTitle() in
utils.ts to accept/obtain t (or accept a translate callback) and return
t(BootModeTitles[bootloader]); and update TemplateBootloaderModal to wrap its
BootModeTitles lookups with t() as well—ensure you import/use
useKubevirtTranslation() in the components that render these values and do not
perform translation at module init.
In `@src/utils/components/WorkloadProfileModal/WorkloadProfileModal.tsx`:
- Line 53: Add the three workload description strings to the i18n extraction
file by inserting the same extraction-comment pattern used for WORKLOADS_LABELS
into i18n.ts so the dynamic keys in WORKLOADS_DESCRIPTIONS can be picked up by
the extractor (include the three t('...') lines exactly as in the review: "Small
scale consumption, recommended for using the graphical console", "Optimized for
High resource consumption workloads", "Balances performance, compatible with a
broad range of workloads"); then add corresponding entries in the en locale JSON
(plugin__kubevirt-plugin.json) and run npm run i18n to update extraction for
other locales so description={t(WORKLOADS_DESCRIPTIONS[key])} is translatable.
---
Outside diff comments:
In
`@src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx`:
- Line 14: The toggle shows untranslated English because the component passes
the raw key (vmResourceOption) into SelectToggle via selected and FormPFSelect
lacks a translated selectedLabel; also the state setter name setvmResourceOption
violates camelCase. Fix by renaming setvmResourceOption to setVmResourceOption
and update state usage around vmResourceOption, and change the
FormPFSelect/SelectToggle usage to pass a translated label (e.g., compute
selectedLabel = t(vmResourceOption) or map vmResourceOption through
vmsPerResourceOptions to call t(scope.title)) while keeping vmResourceOption as
the key for selection so the dropdown items remain translated with
t(scope.title).
---
Duplicate comments:
In
`@src/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/utils/constants.ts`:
- Around line 16-37: The current urlSourceHelperTextMapper entries split
sentences into beforeLabelText, label, and afterLabelText which is an i18n
anti-pattern; update each entry in urlSourceHelperTextMapper (keys under
OS_NAME_TYPES like centos, fedora, rhel, windows) to provide a single
translatable string that includes an interpolated placeholder for the link
(e.g., "Example: For CentOS, visit the {{link}} and copy the download link URL")
so consumers can pass the <a> element as the interpolation value; remove the
three-part fields and replace them with one field (e.g., fullSentence or
message) per entry to ensure correct word order in translations.
In `@src/utils/components/ListPageFilter/utils.ts`:
- Around line 158-162: Add an explicit string return type to the
getSearchTextPlaceholder function and change the parameter type of
nameFilterPlaceholder from string to string | undefined; update the function
signature for getSearchTextPlaceholder(searchType: string, selectedSearchFilter:
RowFilter, nameFilterPlaceholder: string | undefined): string so callers and the
internal falsy checks align with the declared types (adjust any local
assumptions inside getSearchTextPlaceholder accordingly).
In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx`:
- Around line 19-28: Keep the changes: in NumberOperatorSelect ensure
Object.keys(...) is cast to NumberOperator[] when building initialOptions,
generate options with content: t(numberOperatorInfo[operator].text), selected
flag comparing operator === selected, and value: operator (this is in the
initialOptions useMemo), and force SimpleSelect to remount by using
key={`number-operator-${selected}`} so options are re-evaluated when selected
changes; these symbols to look for are NumberOperatorSelect, initialOptions,
NumberOperator, numberOperatorInfo, selected, t, and the SimpleSelect key prop.
---
Nitpick comments:
In `@src/utils/components/ListPageFilter/utils.ts`:
- Around line 156-157: The local type guard isValidPlaceholderKey currently
recreated inside getSearchTextPlaceholder; move it to module scope right after
the type alias PlaceholderKey so it’s defined once and reused; update
getSearchTextPlaceholder to call the module-level isValidPlaceholderKey and
remove the nested definition (ensure the guard signature remains: (k: string |
undefined) => k is PlaceholderKey and references
STATIC_SEARCH_FILTERS_PLACEHOLDERS as before).
In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx`:
- Line 34: The inline arrow passed to SimpleSelect's onSelect prop is recreated
each render; wrap it in a useCallback to provide a stable reference. In
NumberOperatorSelect create a memoized handler like handleSelect =
useCallback((_, selection: NumberOperator) => onSelect(selection), [onSelect])
(import useCallback if not present) and pass handleSelect to SimpleSelect's
onSelect prop instead of the inline function.
In `@src/utils/components/WorkloadProfileModal/WorkloadProfileModal.tsx`:
- Around line 11-12: The imports for PatternFly are duplicated; consolidate the
two separate imports into a single import from '@patternfly/react-core' that
includes both FormGroup and SelectOption to avoid redundant import lines in
WorkloadProfileModal (look for the import statements at the top of
WorkloadProfileModal.tsx referencing FormGroup and SelectOption).
In
`@src/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.ts`:
- Around line 7-11: The function getDisableButtonTooltipContent currently has an
implicit return type; add an explicit return type annotation (string |
undefined) to its declaration so the signature reads something like export const
getDisableButtonTooltipContent = (...): string | undefined => { ... }; ensure
the annotation matches the actual returns and references the existing parameter
types (InstanceTypeVMStore, canCreate, udnConfigurationIssue) without changing
implementation.
In
`@src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx`:
- Line 18: The code uses optional chaining on the module-level constant
vmsPerResourceOptions (e.g., vmsPerResourceOptions?.find(...) and
vmsPerResourceOptions?.map(...)) even though that constant cannot be
null/undefined; remove the unnecessary ?. so that you call
vmsPerResourceOptions.find(...) and vmsPerResourceOptions.map(...). Update the
references around the selected variable assignment (const selected =
vmsPerResourceOptions.find((option) => option.title === value)) and any mapping
logic to drop the optional chaining.
In
`@src/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx`:
- Around line 25-36: Extract the IIFE used inside the DescriptionItem JSX into a
pre-computed variable before the return: compute a readable variable (e.g.,
resolvedWorkloadLabel) using vmWorkload, WORKLOADS_LABELS and t (fall back to
vmWorkload if no label) and then pass that variable into DescriptionItem; update
references around getWorkload(vm), getName(vm) and the conditional using
getInstanceTypeMatcher(vm) so the JSX no longer contains the (() => { ... })()
expression.
adamviktora
reviewed
Feb 23, 2026
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
ee47c83 to
dfe724d
Compare
Collaborator
|
@aviavissar: This pull request references CNV-76888 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/views/templates/list/components/VirtualMachineTemplatesSource/VirtualMachineTemplatesSource.tsx (1)
55-55: Register boot source labels for static i18n extraction.
t(bootSourceLabel)uses a variable key, so the extractor won’t discover the boot source strings and non‑English locales won’t get entries. Please register these labels insrc/utils/i18n.ts(mirroring theWORKLOADS_LABELSpattern) or mapbootSourceLabelto explicitt('…')calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/list/components/VirtualMachineTemplatesSource/VirtualMachineTemplatesSource.tsx` at line 55, The call t(bootSourceLabel) uses a dynamic key so i18n extractors won't pick up the boot source strings; update registration by adding a constant map of boot source labels in src/utils/i18n.ts (follow the WORKLOADS_LABELS pattern) containing all literal keys used by bootSourceLabel, then replace or augment uses in the VirtualMachineTemplatesSource component (VirtualMachineTemplatesSource, bootSourceLabel) to reference the mapped constants (or explicit t('...') calls) so all labels are statically discoverable by the extractor.src/utils/components/DiskModal/components/utils/constants.ts (1)
1-3: Verify render sites now wrap these constants witht().Removing
t()from constants is the correct fix for the page-reload-only bug. However, per previous analysis, three render sites were identified that displayed these constants untranslated:
DiskRow.tsx:{!hasPVC && source}(rendersOTHER/CONTAINER_EPHERMALraw)readableSizeUnitusage inDiskRow.tsx(returnsDYNAMICraw for non-numeric strings)DynamicSize.tsx:<TextInput value={DYNAMIC} />(passes the constant directly)Confirm each of these now calls
t(OTHER),t(CONTAINER_EPHERMAL), andt(DYNAMIC)respectively at the render site.#!/bin/bash # Verify render sites wrap OTHER, CONTAINER_EPHERMAL, DYNAMIC with t() echo "=== DiskRow.tsx: source render path ===" rg -n "OTHER\|CONTAINER_EPHERMAL\|DYNAMIC\|source\}" \ src/views/virtualmachines/details/tabs/configuration/storage/components/tables/disk/DiskRow.tsx \ -B 2 -A 2 echo "" echo "=== DynamicSize.tsx: TextInput value ===" rg -n "DYNAMIC\|TextInput" \ src/utils/components/DiskModal/components/DiskSizeInput/DynamicSize.tsx \ -B 2 -A 2 echo "" echo "=== readableSizeUnit: check if DYNAMIC returned via t() ===" rg -n "DYNAMIC\|readableSizeUnit" src/utils/utils/ -B 2 -A 2 echo "" echo "=== Any remaining bare (non-t()) usages of these three constants ===" rg -n "\b(OTHER|CONTAINER_EPHERMAL|DYNAMIC)\b" -g "*.tsx" -g "*.ts" | grep -v "t(OTHER\|t(CONTAINER_EPHERMAL\|t(DYNAMIC\|constants\.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/DiskModal/components/utils/constants.ts` around lines 1 - 3, Three render sites still output untranslated constants; update each render site to call the i18n translator on the constant instead of passing the raw string: in DiskRow.tsx wrap the source usage so it renders t(OTHER) or t(CONTAINER_EPHERMAL) when appropriate (ensure t is in scope via useTranslation), update readableSizeUnit usage so it returns or maps to t(DYNAMIC) for non-numeric strings (or wrap the returned DYNAMIC with t(...) at the call site), and change DynamicSize.tsx to pass t(DYNAMIC) into the TextInput value (again ensuring t is available). Verify imports/useTranslation are present where you add t(...) so the translator is in scope.src/utils/components/FirmwareBootloaderModal/utils/constants.ts (1)
10-15: Removal of module-initt()calls is correct — verify consuming components wrap at render time.Moving translations out of module initialization is the right fix for the initialization-ordering bug. However, every consuming site that renders these values must now call
t()at render time (e.g.,t(BootModeTitles[selectedFirmwareBootloader])). Based on the surrounding context, the consuming components (FirmwareBootloaderModal.tsx,TemplateBootloaderModal.tsx,utils.ts) still need those render-time wrappers added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/FirmwareBootloaderModal/utils/constants.ts` around lines 10 - 15, BootModeTitles should remain a plain map (no t() at module init); update each consumer to call the i18n translator at render time instead of relying on pre-translated strings: in FirmwareBootloaderModal.tsx, TemplateBootloaderModal.tsx and utils.ts replace usages like BootModeTitles[someKey] with t(BootModeTitles[someKey]) (or t(...) when passing into JSX/props) and ensure the components import/use the i18n hook/context so translation runs during render; also handle null/undefined keys defensively (e.g., fallback string) when wrapping with t().
🧹 Nitpick comments (6)
src/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx (1)
33-36: Consider extracting the workload label translation into a shared helper.This IIFE pattern is duplicated in
DetailsSection.tsxandWizardOverviewGrid.tsx. A small helper (e.g.,getTranslatedWorkloadLabel(t, workload)) would reduce repetition and centralize the guard logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx` around lines 33 - 36, Extract the duplicated IIFE translation logic into a shared helper function, e.g., getTranslatedWorkloadLabel(t, workload), that looks up WORKLOADS_LABELS[workload] and returns t(label) if present or the raw workload otherwise; add the helper to a common utilities module and replace the inline IIFE in VMWorkloadProfileDetailsItem, DetailsSection.tsx and WizardOverviewGrid.tsx with calls to getTranslatedWorkloadLabel(t, vmWorkload) (or the corresponding prop name) to centralize the guard and translation behavior.src/utils/i18n.ts (1)
102-114: The TODO on line 103 highlights a real i18n issue with concatenated translations.The URL source helper texts (lines 104-114) are split across three separate translation keys per OS (prefix, link text, suffix), which breaks sentence structure for non-English locales. The existing TODO correctly identifies this. Consider tracking it as a follow-up issue.
Would you like me to open an issue to track replacing these concatenated translations with single interpolated keys?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/i18n.ts` around lines 102 - 114, The i18n strings for URL source helper text are split into three concatenated translation keys which breaks sentence structure in other locales; replace the tri-part keys with single interpolated translation keys and update callers (e.g., where t(osEntry.field) is used in DiskSourceUrlInput / URLSourceHelperText.tsx) to pass any dynamic values via interpolation rather than concatenation—create one translation entry per full sentence (with placeholders for the OS-specific link text) and switch uses of t(...) to a single t('key', { linkText: ... }) call so the sentence stays atomic for translators.src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx (1)
21-28:selecteddependency inuseMemois redundant with thekeyremount strategy.Because
key={number-operator-${selected}}already forcesSimpleSelectto fully unmount and remount wheneverselectedchanges, the memoizedinitialOptionsfrom the previous render is never reused across aselectedchange — the component starts fresh. The only dependency that meaningfully protects the memo ist(language switch). Removingselectedfrom the dependency array makes the intent clearer.♻️ Proposed simplification
- const initialOptions = useMemo<SimpleSelectOption[]>(() => { + const initialOptions = useMemo<SimpleSelectOption[]>(() => { const operators = Object.keys(NumberOperator) as NumberOperator[]; return operators.map((operator) => ({ content: t(numberOperatorInfo[operator].text), selected: operator === selected, value: operator, })); - }, [selected, t]); + }, [selected, t]); // `selected` is technically redundant here since `key` forces remount; kept for clarityOr, if you want to be explicit about the intent:
- }, [selected, t]); + }, [t]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx` around lines 21 - 28, The useMemo for initialOptions in NumberOperatorSelect recalculates based on selected even though the parent uses key={`number-operator-${selected}`} to force remounts; remove selected from the dependency array so only t is a dependency. Update the useMemo call that defines initialOptions (in NumberOperatorSelect, which builds SimpleSelect options from NumberOperator and numberOperatorInfo) to depend solely on t to reflect that remounting handles selected changes and to clarify intent.src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.ts (1)
26-26: Add an issue reference to these explanatory comments (or remove if redundant).If these notes document a workaround or non-obvious decision, please add the CNV-76888 reference to align with the comment policy.
As per coding guidelines, "Avoid comments in code; write self-documenting code instead. Use comments only for non-obvious decisions; include links to issues for workarounds".📝 Suggested tweak
-// Note: labels are translated at render time in InstanceTypeDrilldownSelect.tsx +// Note (CNV-76888): labels are translated at render time in InstanceTypeDrilldownSelect.tsx @@ -// Note: series labels are generated via getSeriesLabel() at render time using t('{{symbol}} series', { symbol }) +// Note (CNV-76888): series labels are generated via getSeriesLabel() at render time using t('{{symbol}} series', { symbol })Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.ts` at line 26, The inline explanatory comment in constants.ts above InstanceTypeDrilldownSelect (note: "labels are translated at render time in InstanceTypeDrilldownSelect.tsx") either needs an issue reference for the non-obvious behavior or should be removed; update that comment to include the CNV-76888 issue ID if this documents a workaround/decision, e.g., append "See CNV-76888" (or delete the comment if it is redundant) so the rationale is traceable for InstanceTypeDrilldownSelect and related constants.src/utils/components/SSHAccess/components/SSHServiceSelect.tsx (1)
50-50: UseSERVICE_TYPESenum keys for consistent access toserviceTypeTitles.Line 46 correctly accesses
serviceTypeTitles[sshServiceType]using the enum value as the key. Lines 50, 60, and 77 access the same object via hardcoded property names (.None,.LoadBalancer,.NodePort). While both approaches work because the enum string values match the computed object keys, using the enum-based pattern consistently throughout the component improves maintainability and self-documentation.Suggested refactor for consistency
- {t(serviceTypeTitles.None)} + {t(serviceTypeTitles[SERVICE_TYPES.NONE])}- {t(serviceTypeTitles.LoadBalancer)} + {t(serviceTypeTitles[SERVICE_TYPES.LOAD_BALANCER])}- {t(serviceTypeTitles.NodePort)} + {t(serviceTypeTitles[SERVICE_TYPES.NODE_PORT])}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/SSHAccess/components/SSHServiceSelect.tsx` at line 50, Replace direct property access like serviceTypeTitles.None / .LoadBalancer / .NodePort with enum-keyed access using SERVICE_TYPES to match the existing pattern (e.g., change t(serviceTypeTitles.None) to t(serviceTypeTitles[SERVICE_TYPES.None])). Update every occurrence in SSHServiceSelect (the three spots using .None, .LoadBalancer, .NodePort) to use serviceTypeTitles[SERVICE_TYPES.<KEY>] so the keys are consistently derived from the SERVICE_TYPES enum and improve maintainability.locales/ko/plugin__kubevirt-plugin.json (1)
1249-1249: Consolidate near-duplicate keys that differ only by punctuationThere is already a matching key without the trailing period; keeping both can split translations and cause inconsistent lookups. Consider normalizing punctuation in the source string and keeping a single key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/ko/plugin__kubevirt-plugin.json` at line 1249, The JSON contains two near-duplicate translation keys that differ only by a trailing period (the string "Optimized for best performance. Supported by most Linux distributions. Windows requires additional drivers to use this model." vs the same without the trailing period); remove the duplicate key with the trailing period and consolidate its translation into the canonical key (prefer the punctuation-normalized source string without the trailing period), then update any references to use that single key so lookups are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/components/FirmwareBootloaderModal/utils/constants.ts`:
- Line 19: Update the description strings in the FirmwareBootloaderModal
constants so they are punctuated consistently: add a terminal period to the
'description' values for the BIOS, UEFI and IPL entries (the entries labeled
"BIOS", "UEFI", and "IPL" in the constants object in utils/constants.ts) so all
three descriptions end with a period.
In
`@src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx`:
- Around line 33-36: The FormPFSelect in VMsPerResourceCard is missing a
selectedLabel, so the toggle shows untranslated raw values; update the
FormPFSelect instance used with vmsPerResourceOptions to pass
selectedLabel={t(vmResourceOption)} (or the appropriate selected value variable)
so the toggle button displays the translated string consistently with the
SelectOption entries; locate the FormPFSelect in VMsPerResourceCard and add the
selectedLabel prop using the t(...) call.
---
Duplicate comments:
In `@src/utils/components/DiskModal/components/utils/constants.ts`:
- Around line 1-3: Three render sites still output untranslated constants;
update each render site to call the i18n translator on the constant instead of
passing the raw string: in DiskRow.tsx wrap the source usage so it renders
t(OTHER) or t(CONTAINER_EPHERMAL) when appropriate (ensure t is in scope via
useTranslation), update readableSizeUnit usage so it returns or maps to
t(DYNAMIC) for non-numeric strings (or wrap the returned DYNAMIC with t(...) at
the call site), and change DynamicSize.tsx to pass t(DYNAMIC) into the TextInput
value (again ensuring t is available). Verify imports/useTranslation are present
where you add t(...) so the translator is in scope.
In `@src/utils/components/FirmwareBootloaderModal/utils/constants.ts`:
- Around line 10-15: BootModeTitles should remain a plain map (no t() at module
init); update each consumer to call the i18n translator at render time instead
of relying on pre-translated strings: in FirmwareBootloaderModal.tsx,
TemplateBootloaderModal.tsx and utils.ts replace usages like
BootModeTitles[someKey] with t(BootModeTitles[someKey]) (or t(...) when passing
into JSX/props) and ensure the components import/use the i18n hook/context so
translation runs during render; also handle null/undefined keys defensively
(e.g., fallback string) when wrapping with t().
In
`@src/views/templates/list/components/VirtualMachineTemplatesSource/VirtualMachineTemplatesSource.tsx`:
- Line 55: The call t(bootSourceLabel) uses a dynamic key so i18n extractors
won't pick up the boot source strings; update registration by adding a constant
map of boot source labels in src/utils/i18n.ts (follow the WORKLOADS_LABELS
pattern) containing all literal keys used by bootSourceLabel, then replace or
augment uses in the VirtualMachineTemplatesSource component
(VirtualMachineTemplatesSource, bootSourceLabel) to reference the mapped
constants (or explicit t('...') calls) so all labels are statically discoverable
by the extractor.
---
Nitpick comments:
In `@locales/ko/plugin__kubevirt-plugin.json`:
- Line 1249: The JSON contains two near-duplicate translation keys that differ
only by a trailing period (the string "Optimized for best performance. Supported
by most Linux distributions. Windows requires additional drivers to use this
model." vs the same without the trailing period); remove the duplicate key with
the trailing period and consolidate its translation into the canonical key
(prefer the punctuation-normalized source string without the trailing period),
then update any references to use that single key so lookups are consistent.
In
`@src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.ts`:
- Line 26: The inline explanatory comment in constants.ts above
InstanceTypeDrilldownSelect (note: "labels are translated at render time in
InstanceTypeDrilldownSelect.tsx") either needs an issue reference for the
non-obvious behavior or should be removed; update that comment to include the
CNV-76888 issue ID if this documents a workaround/decision, e.g., append "See
CNV-76888" (or delete the comment if it is redundant) so the rationale is
traceable for InstanceTypeDrilldownSelect and related constants.
In `@src/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsx`:
- Around line 21-28: The useMemo for initialOptions in NumberOperatorSelect
recalculates based on selected even though the parent uses
key={`number-operator-${selected}`} to force remounts; remove selected from the
dependency array so only t is a dependency. Update the useMemo call that defines
initialOptions (in NumberOperatorSelect, which builds SimpleSelect options from
NumberOperator and numberOperatorInfo) to depend solely on t to reflect that
remounting handles selected changes and to clarify intent.
In `@src/utils/components/SSHAccess/components/SSHServiceSelect.tsx`:
- Line 50: Replace direct property access like serviceTypeTitles.None /
.LoadBalancer / .NodePort with enum-keyed access using SERVICE_TYPES to match
the existing pattern (e.g., change t(serviceTypeTitles.None) to
t(serviceTypeTitles[SERVICE_TYPES.None])). Update every occurrence in
SSHServiceSelect (the three spots using .None, .LoadBalancer, .NodePort) to use
serviceTypeTitles[SERVICE_TYPES.<KEY>] so the keys are consistently derived from
the SERVICE_TYPES enum and improve maintainability.
In `@src/utils/i18n.ts`:
- Around line 102-114: The i18n strings for URL source helper text are split
into three concatenated translation keys which breaks sentence structure in
other locales; replace the tri-part keys with single interpolated translation
keys and update callers (e.g., where t(osEntry.field) is used in
DiskSourceUrlInput / URLSourceHelperText.tsx) to pass any dynamic values via
interpolation rather than concatenation—create one translation entry per full
sentence (with placeholders for the OS-specific link text) and switch uses of
t(...) to a single t('key', { linkText: ... }) call so the sentence stays atomic
for translators.
In
`@src/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsx`:
- Around line 33-36: Extract the duplicated IIFE translation logic into a shared
helper function, e.g., getTranslatedWorkloadLabel(t, workload), that looks up
WORKLOADS_LABELS[workload] and returns t(label) if present or the raw workload
otherwise; add the helper to a common utilities module and replace the inline
IIFE in VMWorkloadProfileDetailsItem, DetailsSection.tsx and
WizardOverviewGrid.tsx with calls to getTranslatedWorkloadLabel(t, vmWorkload)
(or the corresponding prop name) to centralize the guard and translation
behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
locales/en/plugin__kubevirt-plugin.jsonlocales/es/plugin__kubevirt-plugin.jsonlocales/fr/plugin__kubevirt-plugin.jsonlocales/ja/plugin__kubevirt-plugin.jsonlocales/ko/plugin__kubevirt-plugin.jsonlocales/zh/plugin__kubevirt-plugin.jsonsrc/utils/components/AddBootableVolumeModal/components/SourceTypeSelection/SourceTypeSelection.tsxsrc/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/InstanceTypeDrilldownSelect.tsxsrc/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/components/DrilldownMenuItem/DrilldownMenuItemWrapper.tsxsrc/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/utils/constants.tssrc/utils/components/AddBootableVolumeModal/utils/constants.tssrc/utils/components/DiskModal/components/DiskInterfaceSelect/DiskInterfaceSelect.tsxsrc/utils/components/DiskModal/components/DiskInterfaceSelect/utils/constants.tssrc/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/URLSourceHelperText.tsxsrc/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/utils/constants.tssrc/utils/components/DiskModal/components/utils/constants.tssrc/utils/components/FirmwareBootloaderModal/utils/constants.tssrc/utils/components/ListPageFilter/components/TextFiltersToolbarItem.tsxsrc/utils/components/ListPageFilter/constants.tssrc/utils/components/ListPageFilter/hooks/useNameAndLabelParameters.tssrc/utils/components/ListPageFilter/utils.tssrc/utils/components/NumberOperatorSelect/NumberOperatorSelect.tsxsrc/utils/components/NumberOperatorSelect/constants.tssrc/utils/components/SSHAccess/components/SSHServiceSelect.tsxsrc/utils/components/SSHAccess/constants.tssrc/utils/components/WorkloadProfileModal/WorkloadProfileModal.tsxsrc/utils/i18n.tssrc/utils/resources/template/utils/constants.tssrc/utils/utils/constants.tssrc/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.tssrc/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/StorageSection/CustomizeSource/SelectSourceOption.tsxsrc/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/StorageSection/constants.tssrc/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/TemplateInfoSection.tsxsrc/views/catalog/templatescatalog/components/TemplatesCatalogFilters/TemplatesCatalogFilters.tsxsrc/views/catalog/templatescatalog/components/TemplatesCatalogRow.tsxsrc/views/catalog/templatescatalog/components/TemplatesCatalogTile.tsxsrc/views/catalog/utils/constants.tssrc/views/catalog/wizard/tabs/overview/components/WizardOverviewGrid.tsxsrc/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsxsrc/views/clusteroverview/OverviewTab/vms-per-resource-card/utils/constants.tssrc/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/Conditions.tsxsrc/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/constants.tssrc/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsxsrc/views/migrationpolicies/list/utils/constants.tssrc/views/templates/list/components/VirtualMachineTemplatesSource/VirtualMachineTemplatesSource.tsxsrc/views/topology/components/vm/VMDetailsPanel/components/VMDetailsPanelRightColumn/components/VMWorkloadProfileDetailsItem.tsxsrc/views/virtualmachines/details/tabs/configuration/details/DetailsSection.tsxsrc/views/virtualmachines/search/components/NameFilter.tsx
💤 Files with no reviewable changes (2)
- src/utils/components/NumberOperatorSelect/constants.ts
- src/views/migrationpolicies/list/utils/constants.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- src/utils/components/DiskModal/components/DiskInterfaceSelect/utils/constants.ts
- src/views/catalog/templatescatalog/components/TemplatesCatalogRow.tsx
- src/utils/components/SSHAccess/constants.ts
- src/utils/resources/template/utils/constants.ts
- locales/zh/plugin__kubevirt-plugin.json
- locales/fr/plugin__kubevirt-plugin.json
- src/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/StorageSection/CustomizeSource/SelectSourceOption.tsx
- src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/InstanceTypeDrilldownSelect.tsx
- src/views/catalog/templatescatalog/components/TemplatesCatalogFilters/TemplatesCatalogFilters.tsx
- src/views/catalog/CreateFromInstanceTypes/components/CreateVMFooter/components/ActionButtons/utils.ts
- src/views/dashboard-extensions/KubevirtHealthPopup/components/Conditions/constants.ts
- src/utils/components/AddBootableVolumeModal/components/VolumeMetadata/components/InstanceTypeDrilldownSelect/components/DrilldownMenuItem/DrilldownMenuItemWrapper.tsx
- src/utils/components/ListPageFilter/constants.ts
- src/utils/components/AddBootableVolumeModal/utils/constants.ts
- src/utils/utils/constants.ts
- src/views/clusteroverview/OverviewTab/vms-per-resource-card/utils/constants.ts
- src/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/TemplateInfoSection.tsx
- src/views/migrationpolicies/list/components/MigrationPoliciesCreateButton/MigrationPoliciesCreateButton.tsx
- src/utils/components/WorkloadProfileModal/WorkloadProfileModal.tsx
- src/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/URLSourceHelperText.tsx
- src/utils/components/DiskModal/components/DiskSourceSelect/components/DiskSourceUrlInput/utils/constants.ts
- src/views/catalog/templatescatalog/components/TemplatesCatalogTile.tsx
- src/utils/components/ListPageFilter/hooks/useNameAndLabelParameters.ts
- src/views/catalog/templatescatalog/components/TemplatesCatalogDrawer/StorageSection/constants.ts
- src/utils/components/DiskModal/components/DiskInterfaceSelect/DiskInterfaceSelect.tsx
src/views/clusteroverview/OverviewTab/vms-per-resource-card/VMsPerResourceCard.tsx
Show resolved
Hide resolved
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
dfe724d to
1dd5334
Compare
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
2 times, most recently
from
bd18472 to
bea2a2f
Compare
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
bea2a2f to
31920fc
Compare
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
31920fc to
ddc8c5b
Compare
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
ddc8c5b to
c0c2558
Compare
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
11171f8 to
535a88f
Compare
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
535a88f to
daa85c0
Compare
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
2 times, most recently
from
08ad9a3 to
516ebdb
Compare
adamviktora
reviewed
Mar 9, 2026
| const { afterLabelText, beforeLabelText, label } = | ||
| urlSourceHelperTextMapper[os || OS_NAME_TYPES.fedora]; | ||
|
|
||
| // TODO: Replace three-part concatenation with single interpolated translation key |
Member
There was a problem hiding this comment.
Can we fix this TODO in this PR or open a followup Jira ticket?
|
|
||
| return BootModeTitles[bootloader]; | ||
| const title = BootModeTitles[bootloader]; | ||
| return t ? t(title) : title; |
Member
There was a problem hiding this comment.
Can we make the t argument required in the function? So we can just do return t(title);
Comment on lines
+183
to
+184
| // Strings below are extracted from t() calls in source; listed here so every key in | ||
| // locales/en/plugin__kubevirt-plugin.json has a comment for translator context. |
Member
There was a problem hiding this comment.
I think this is not a maintainable approach. This is just a copy-pasted current state of locales/en/plugin__kubevirt-plugin.json which doesn't really help anything.
Ideally we would have the translation comments directly in the files that need the translation. So developer can see that such string will have a translation string created. E.g. do:
// t('Desktop')
// t('High performance')
// t('Server')
export const WORKLOADS_LABELS = {
[WORKLOADS.desktop]: 'Desktop',
[WORKLOADS.highperformance]: 'High performance',
[WORKLOADS.server]: 'Server',
};
Or simply continue with the approach above - have the comments in this central i18n.ts file, but explain for what constants in which file are they needed.
|
|
||
| if (isEmpty(condition)) return null; | ||
|
|
||
| const label = VALUE_TO_LABLE[condition] ? t(VALUE_TO_LABLE[condition]) : condition; |
Member
There was a problem hiding this comment.
typo: should be VALUE_TO_LABEL
Comment on lines
+138
to
+142
| (() => { | ||
| if (source === OTHER) return t(OTHER); | ||
| if (source === CONTAINER_EPHERMAL) return t(CONTAINER_EPHERMAL); | ||
| return source; | ||
| })()} |
Member
There was a problem hiding this comment.
Please create a separate function instead of the inline function call
| "DS": "DS", | ||
| "Duplicate keys found": "Duplicate keys found", | ||
| "Duration": "Duration", | ||
| "Dynamic": "Dynamic", |
Member
There was a problem hiding this comment.
Missing translation for this string
| "Ephemeral disk (Container image)": "Ephemeral disk (Container image)", | ||
| "Ephemeral storage limits": "Ephemeral storage limits", | ||
| "Ephemeral storage requests": "Ephemeral storage requests", | ||
| "Equals": "Equals", |
Member
There was a problem hiding this comment.
Missing translation for this string
Comment on lines
-873
to
-874
| "Greater or equals": "Greater or equals", | ||
| "Greater than": "Greater than", |
Member
There was a problem hiding this comment.
Missing translation for this string
Comment on lines
-1042
to
-1044
| "Less or equals": "Less or equals", | ||
| "less than": "less than", | ||
| "Less than": "Less than", |
Member
There was a problem hiding this comment.
Missing translation for these strings
aviavissar
force-pushed
the
t-function-for-translations-works-only-on-page-reload-consts-files-changes
branch
from
516ebdb to
6558101
Compare
Contributor
|
@aviavissar: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
4 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.
📝 Description
Fixing a translation problem in constants Removing translation from the constants files and changing the component that enabled the translation
Summary by CodeRabbit