fix: use correct languageInfo when validating Mt settings #3357
Conversation
WalkthroughUpdated Machine Translation settings validation to use full language info objects per target language during validation and updates, ensuring each settings entry is validated against its specific language's supported services rather than a shared language ID lookup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
webapp/**/*.tsx📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,kt,kts}⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (3)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx (2)
49-62: Type annotation should reflect nullable parameter.The function checks
!langInfoon line 55, implying the parameter can beundefined, but the type annotation isLanguageInfoModelwithout| undefined. This creates a type safety gap where TypeScript won't warn callers about passing undefined.🔎 Suggested fix:
function isSupported( service: MtServiceInfo | undefined, - langInfo: LanguageInfoModel + langInfo: LanguageInfoModel | undefined ) {
99-117: Add defensive handling for missingtargetLangInfo.The
.find()on line 102 can returnundefinediftargetLanguageIdhas no matching entry inlanguageInfos. Using non-null assertions (targetLangInfo!) masks this from TypeScript but doesn't prevent runtime issues.When
targetLangInfois undefined, the!langInfocheck inisSupportedcauses all services to be considered "supported", potentially saving invalid service configurations.🔎 Suggested fix - filter entries without valid language info:
const mtSettingsValidated = updatedMtSettings + .filter(({ targetLanguageId }) => { + // Default settings (null targetLanguageId) or settings with valid language info + return ( + targetLanguageId === null || + languageInfos.data?._embedded?.languageInfos?.some( + (l) => l.languageId === targetLanguageId + ) + ); + }) .map( ({ targetLanguageId, primaryServiceInfo, enabledServicesInfo }) => { const targetLangInfo = - languageInfos.data!._embedded!.languageInfos!.find( + languageInfos.data?._embedded?.languageInfos?.find( (l) => l.languageId === targetLanguageId ); return { targetLanguageId, primaryServiceInfo: validateService( - targetLangInfo!, + targetLangInfo, primaryServiceInfo ), enabledServicesInfo: enabledServicesInfo - ?.map((service) => validateService(targetLangInfo!, service)) + ?.map((service) => validateService(targetLangInfo, service)) .filter((service) => Boolean(service)) as MtServiceInfo[], }; } );This also requires updating
validateServiceto acceptLanguageInfoModel | undefined.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
webapp/**/*.{ts,tsx}: Use TypeScript path aliases (tg.component/, tg.service/, tg.hooks/, tg.views/, tg.globalContext/*) instead of relative imports
After backend API changes, regenerate TypeScript types by runningnpm run schema(andnpm run billing-schemaif applicable) in the webapp directory
Use typed React Query hooks fromuseQueryApi.tsfor API communication, not raw React Query
Use Tolgee-specific hooksuseReportEventanduseReportOncefrom 'tg.hooks/useReportEvent' for business event tracking and analytics
Files:
webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx
webapp/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Always use
data-cyattributes for E2E test selectors, never rely on text content
Files:
webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx
🧬 Code graph analysis (1)
webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx (1)
webapp/src/views/projects/languages/MachineTranslation/types.ts (1)
LanguageInfoModel(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: BT 🔎 (ktlint:test)
- GitHub Check: BT 🔎 (security:test)
- GitHub Check: BT 🔎 (server-app:runWebsocketTests)
- GitHub Check: BT 🔎 (ee-test:test)
- GitHub Check: BT 🔎 (server-app:runContextRecreatingTests)
- GitHub Check: BT 🔎 (server-app:runStandardTests)
- GitHub Check: BT 🔎 (data:test)
- GitHub Check: BT 🔎 (server-app:runWithoutEeTests)
- GitHub Check: Build frontend 🏗️
- GitHub Check: Frontend static check 🪲
🔇 Additional comments (2)
webapp/src/views/projects/languages/MachineTranslation/useMachineTranslationSettings.tsx (2)
64-78: LGTM!The
validateServicefunction correctly passes thelangInfoobject toisSupported, aligning with the updated signature.
80-97: Core fix correctly addresses the bug.The approach of filtering out the current language's settings from existing settings and then re-adding the updated settings ensures that changes to one language don't affect other languages' configurations. This directly addresses issue #3353.
9e3222b to
82f04f4
Compare
|
I need to test this locally – something is failing in the E2E tests. I'd also like to add an E2E test for this. |
|
(Probably after the new year.) |
Fixes #3353
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.