-
Notifications
You must be signed in to change notification settings - Fork 663
fix: preserve component tree structure for React.Children APIs #1476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: preserve component tree structure for React.Children APIs #1476
Conversation
This fixes issue lingodotdev#1105 where the compiler broke React components that traverse their immediate children using React.Children.map or similar patterns. The Problem: The compiler was replacing entire JSX elements with LingoComponent instances, which changed the component tree structure. Parent components expecting specific child types (like Tab, Accordion.Item, etc.) could no longer find them. The Solution: For React components (uppercase or member expressions), preserve the original component wrapper and only wrap the text content with LingoText component. Before fix: <Tab>Home</Tab> -> <LingoComponent ={Tab}>Home</LingoComponent> Parent can't find Tab components After fix: <Tab>Home</Tab> -> <Tab><LingoText>Home</LingoText></Tab> Parent finds Tab components correctly Changes: - Modified jsx-scope-inject.ts to detect React components vs HTML elements - For React components: keep wrapper, replace only text children with LingoText - For HTML elements: use existing approach (replace entire element) - Created LingoText component for both client and RSC modes - Returns translated text wrapped in Fragment (no DOM wrapper) Fixes lingodotdev#1105
999803f to
1041c78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes issue #1105 by preserving component tree structure when internationalizing React components that use React.Children APIs. The compiler now distinguishes between React components (uppercase or member expressions) and HTML elements, wrapping only the text children of React components instead of replacing the entire component.
Key changes:
- Component detection logic added to differentiate React components from HTML elements
- New
LingoTextcomponent introduced for wrapping text-only children in React components - Existing
LingoComponentapproach retained for HTML elements
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compiler/src/jsx-scope-inject.ts | Added component detection logic and branching for React components vs HTML elements; creates LingoText for React components |
| packages/react/src/rsc/text-component.tsx | New RSC-compatible LingoText component for text-only translation |
| packages/react/src/client/text-component.tsx | New client-side LingoText component for text-only translation |
| packages/react/src/rsc/index.ts | Exports LingoText from RSC package |
| packages/react/src/client/index.ts | Exports LingoText from client package |
Comments suppressed due to low confidence (1)
packages/react/src/client/text-component.tsx:1
- The function
$functionsparameter is accepted but never used in the translation logic. If functions are intended to be supported (as the prop exists), they should be processed; otherwise, the parameter should be removed or documented as unsupported.
"use client";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Replace only the text children with LingoText, keeping the component wrapper | ||
| jsxScope.node.children = [lingoTextNode]; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing all children with only LingoText loses nested JSX elements. When React components have nested elements like <Tab>Hello <strong>world</strong></Tab>, only the text will be preserved, but the <strong> element will be lost. Consider extracting and preserving nested elements using getNestedJsxElements() similar to the HTML elements branch (line 130).
| jsxScope.node.children = [lingoTextNode]; | |
| // Preserve nested JSX elements, only wrap text nodes in LingoText | |
| const newChildren = []; | |
| for (const child of jsxScope.node.children) { | |
| if ( | |
| t.isJSXText(child) || | |
| (t.isJSXExpressionContainer(child) && !t.isJSXEmptyExpression(child.expression)) | |
| ) { | |
| // Wrap text or expression in LingoText | |
| // Clone the lingoTextNode to avoid sharing the same node instance | |
| const lingoTextNodeClone = t.jsxElement( | |
| t.jsxOpeningElement( | |
| t.jsxIdentifier(lingoTextImport.importedName), | |
| [], | |
| true, | |
| ), | |
| null, | |
| [], | |
| true, | |
| ); | |
| const lingoTextNodePathClone = { node: lingoTextNodeClone } as any; | |
| setJsxAttributeValue(lingoTextNodePathClone, "$fileKey", payload.relativeFilePath); | |
| setJsxAttributeValue(lingoTextNodePathClone, "$entryKey", getJsxScopeAttribute(jsxScope)!); | |
| const $variables = getJsxVariables(jsxScope); | |
| if ($variables.properties.length > 0) { | |
| setJsxAttributeValue(lingoTextNodePathClone, "$variables", $variables); | |
| } | |
| const $functions = getJsxFunctions(jsxScope); | |
| if ($functions.properties.length > 0) { | |
| setJsxAttributeValue(lingoTextNodePathClone, "$functions", $functions); | |
| } | |
| const $expressions = getJsxExpressions(jsxScope); | |
| if ($expressions.elements.length > 0) { | |
| setJsxAttributeValue(lingoTextNodePathClone, "$expressions", $expressions); | |
| } | |
| if (mode === "server") { | |
| const loadDictionaryImport = getOrCreateImport(payload.ast, { | |
| exportedName: "loadDictionary", | |
| moduleName: ModuleId.ReactRSC, | |
| }); | |
| setJsxAttributeValue( | |
| lingoTextNodePathClone, | |
| "$loadDictionary", | |
| t.arrowFunctionExpression( | |
| [t.identifier("locale")], | |
| t.callExpression(t.identifier(loadDictionaryImport.importedName), [ | |
| t.identifier("locale"), | |
| ]), | |
| ), | |
| ); | |
| } | |
| // Place the original child as the child of LingoText | |
| lingoTextNodeClone.children = [child]; | |
| newChildren.push(lingoTextNodeClone); | |
| } else { | |
| // Preserve nested JSX elements as is | |
| newChildren.push(child); | |
| } | |
| } | |
| jsxScope.node.children = newChildren; |
| // Replace variables | ||
| if (variables) { | ||
| Object.entries(variables).forEach(([key, value]) => { | ||
| text = text.replace(new RegExp(`\\{${key}\\}`, "g"), String(value)); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable key from Object.entries(variables) is used directly in a RegExp constructor without escaping special regex characters. If a key contains regex metacharacters (e.g., $, ., *), it could cause regex errors or unexpected behavior. Use a regex escape function or escape special characters before constructing the RegExp.
| // Replace variables | ||
| if (variables) { | ||
| Object.entries(variables).forEach(([key, value]) => { | ||
| text = text.replace(new RegExp(`\\{${key}\\}`, "g"), String(value)); |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable key from Object.entries(variables) is used directly in a RegExp constructor without escaping special regex characters. If a key contains regex metacharacters (e.g., $, ., *), it could cause regex errors or unexpected behavior. Use a regex escape function or escape special characters before constructing the RegExp.
| function getTranslation( | ||
| dictionary: any, | ||
| fileKey: string, | ||
| entryKey: string, | ||
| variables?: Record<string, any>, | ||
| functions?: Record<string, Function>, | ||
| expressions?: any[] | ||
| ): string { |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getTranslation function is duplicated across both text-component.tsx files (RSC and client) with identical implementation. Consider extracting this to a shared utility module to reduce code duplication and ensure consistency.
| const isMemberExpression = originalJsxElementName.includes("."); | ||
| const isComponent = /^[A-Z]/.test(originalJsxElementName); | ||
| const isReactComponent = isMemberExpression || isComponent; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Member expression detection using includes('.') is fragile. For HTML elements like <svg.circle> (though uncommon), this would incorrectly identify them as React components. Also, member expressions where the root object starts with lowercase (e.g., lib.Component) would be incorrectly classified as React components. Consider checking if the first character after the last dot is uppercase, or if the base name is uppercase.
| text = text.replace(`{${index}}`, String(expr)); | ||
| }); | ||
| } | ||
|
|
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function $functions parameter is accepted but never used in the translation logic. If functions are intended to be supported (as the prop exists), they should be processed; otherwise, the parameter should be removed or documented as unsupported.
| // Handle function placeholders: {fn:functionName} | |
| if (functions) { | |
| text = text.replace(/\{fn:([a-zA-Z0-9_]+)\}/g, (match, fnName) => { | |
| if (typeof functions[fnName] === "function") { | |
| // Optionally, pass variables/expressions as arguments if needed | |
| try { | |
| return String(functions[fnName](variables, expressions)); | |
| } catch (e) { | |
| return ""; | |
| } | |
| } | |
| return ""; | |
| }); | |
| } |
Major fixes:
1. Preserve nested JSX elements in React components
- Added $elements support to LingoText component
- Extract nested elements and pass them through translation system
- Elements are preserved as placeholders {0}, {1} in translations
2. Improve member expression detection
- Check if property after dot starts with uppercase (e.g., form.Button)
- Avoid misclassifying HTML namespaced elements (e.g., svg.circle)
- More robust detection of React components vs HTML elements
3. Fix RegEx injection vulnerabilities
- Escape special regex characters in variable keys
- Prevents errors when keys contain metacharacters like $, ., *
4. Implement function placeholder support
- Added {fn:functionName} syntax for dynamic content
- Functions receive context (variables, expressions) as arguments
5. Extract shared translation logic
- Created shared/translation-utils.ts to avoid code duplication
- Both client and RSC components now use the same logic
All changes maintain backward compatibility with existing translations
while fixing the critical issue of losing nested elements in React components.
Addresses all review comments from PR lingodotdev#1476
This fixes issue #1105 where the compiler broke React components that traverse their immediate children using React.Children.map or similar patterns.
The Problem:
The compiler was replacing entire JSX elements with LingoComponent instances, which changed the component tree structure. Parent components expecting specific child types (like Tab, Accordion.Item, etc.) could no longer find them.
Example:
// Before fix - broken:
Home →
Settings
// TabContainer.children.map can't find Tab components anymore
The Solution:
For React components (uppercase or member expressions), preserve the original component wrapper and only wrap the text content with LingoText component.
// After fix - working:
Home →
Settings
// TabContainer.children.map finds Tab components correctly
Changes:
Impact:
Fixes #1105