Farabi/migration-to-deriv-com-translations

[farabi-deriv]

This pull request introduces significant improvements to translation management, environment variable handling, and dependency updates. The most notable changes are the addition of a workflow for syncing translations with Crowdin, the integration of new translation-related environment variables throughout the build and deployment process, and the modernization of dependencies to support React 18 and new testing libraries. Additionally, translation extraction scripts and legacy test files have been removed in favor of a new approach using the @deriv-com/translations package.

Translation management and automation:

• Added .github/workflows/sync-translations.yml workflow to automate translation extraction and synchronization with Crowdin for both staging and production environments, leveraging new secrets and variables for secure access and configuration.
• Integrated translation-related environment variables (R2_PROJECT_NAME, CROWDIN_URL, CROWDIN_BRANCH_NAME) into the build steps for production and staging workflows to support translation sync and deployment.
• Updated Webpack plugins to inject translation-related environment variables into the client-side build, ensuring runtime access to translation configuration.
• Modified Babel loader configuration to allow transpilation of the new @deriv-com/translations package.

Dependency updates and modernization:

• Upgraded React and ReactDOM to version 18, replaced Enzyme with Testing Library packages, added dotenv and @deriv-com/translations, and removed legacy translation dependencies (node-gettext, gettext-parser, react-render-html).

Build and environment setup:

• Added support for loading environment variables from .env files in the build initialization script to simplify configuration management.

Codebase cleanup and translation extraction refactor:

• Removed legacy translation extraction and static string test scripts, indicating a shift to the new translation workflow and package.

Development server and routing simplification:

• Simplified language-based routing and 404 handling in the development server by removing language fallbacks and related logic, reflecting the new translation approach.

[farabi-deriv]

@claude

[claude]

Claude finished @farabi-deriv's task —— View job

---

PR Review - Translation Migration & React 18 Upgrade

Tasks

• Gather context and read key files
• Review translation system migration
• Review React 18 upgrade changes
• Review build and workflow changes
• Review code quality and potential issues
• Provide comprehensive feedback

---

Overall Assessment

This is a well-executed migration that successfully modernizes the translation system and upgrades to React 18. The changes are comprehensive and follow good patterns. However, there are several critical issues and recommendations that should be addressed before merging.

🔴 Critical Issues

1. Missing Environment Variable Documentation

Severity: High

The PR introduces three new required environment variables but provides no documentation:

• R2_PROJECT_NAME
• CROWDIN_URL
• CROWDIN_BRANCH_NAME

Issues:

• No .env.example file exists to guide developers
• No documentation in README or setup guides about these variables
• Local development will fail without these variables set
• translation-init.js:13 will throw errors if these are undefined

Recommendation:

# Create .env.example with:
CROWDIN_URL=https://distributions.crowdin.net
R2_PROJECT_NAME=smarttrader
CROWDIN_BRANCH_NAME=staging

Add setup instructions to the README explaining how to configure these for local development.

2. Translation Initialization May Fail Silently

Location: src/javascript/_common/translation-init.js:40-44

The error handling catches and logs errors but returns null, which could cause cascading failures:

} catch (error) {
    console.error('Failed to initialize SmartTrader translations:', error);
    isInitialized = true; // ⚠️ Prevents retry but app continues with broken translations
    return null;
}

Issue: Setting isInitialized = true on failure prevents retries and the app continues running with broken translations. Code using localize() will fail.

Recommendation: Either:

1. Throw the error and halt the app (fail-fast)
2. Provide a fallback translation system
3. Add graceful degradation with English-only fallback

3. React 18 Callback Timing Issue

Location: src/javascript/_common/react_root_manager.js:38-40

if (callback && typeof callback === 'function') {
    setTimeout(callback, 0);
}

Issue: Using setTimeout(callback, 0) is unreliable for post-render callbacks in React 18. The callback may fire before the DOM is fully updated.

Recommendation: Use requestIdleCallback or queueMicrotask for better timing:

if (callback && typeof callback === 'function') {
    queueMicrotask(callback);
}

Or better yet, if callbacks need DOM access, use flushSync:

import { flushSync } from 'react-dom';

flushSync(() => {
    root.render(component);
});
if (callback) callback();

4. Potential Memory Leak in Root Manager

Location: src/javascript/_common/react_root_manager.js:26-31

The roots Map never removes entries except via explicit unmount() calls. If containers are removed from DOM without calling unmountReactComponent(), roots accumulate.

Recommendation: Add cleanup using MutationObserver or document the requirement to call unmountReactComponent() before removing containers.

⚠️ High Priority Issues

5. Language Test Coverage Reduced

Location: src/javascript/_common/__tests__/language.js

The test file was updated but doesn't test the new language detection logic:

• No tests for localStorage-based language detection
• No tests for changeSelectedLanguage() function
• No tests for Crowdin integration
• URL parameter cleaning not tested

Recommendation: Add tests for:

describe('.changeSelectedLanguage()', () => {
    it('updates localStorage', () => { ... });
    it('reloads the page', () => { ... });
    it('rejects unsupported languages', () => { ... });
});

6. Babel Configuration Whitelist

Location: build/webpack/config_common.js:37

exclude: /node_modules\/(?!@deriv-com\/translations)/,

This is correct, but should be documented. The @deriv-com/translations package needs transpilation because it uses modern syntax.

Recommendation: Add a comment explaining why this package needs transpilation:

// Include @deriv-com/translations for transpilation as it uses modern JS syntax
exclude: /node_modules\/(?!@deriv-com\/translations)/,

7. URL Language Parameter Handling

Location: src/javascript/_common/language.js:34-57

The function languageFromUrl() always returns null but has side effects (saves to localStorage and cleans URL). This is confusing.

Current behavior:

languageFromUrl(); // Always returns null, but modifies localStorage

Recommendation: Either:

1. Rename to processUrlLanguageParameter() to clarify it's side-effect based
2. Return a boolean indicating if a parameter was processed
3. Document the behavior with JSDoc

8. Missing Error Boundary for React 18

React 18 has stricter error handling. The app should have an error boundary component to catch rendering errors gracefully.

Recommendation: Add an error boundary wrapper in react_root_manager.js:

class ErrorBoundary extends React.Component {
    componentDidCatch(error) {
        console.error('React rendering error:', error);
    }
    render() {
        return this.props.children;
    }
}

📝 Medium Priority Issues

9. Workflow Triggers May Cause Excessive Runs

Location: .github/workflows/sync-translations.yml:4-9

on:
  push:
    branches:
      - master
  schedule:
    - cron: '0 */12 * * *'

Issue: Translation sync runs on every push to master AND every 12 hours. This could be expensive if there are frequent pushes.

Recommendation: Consider using paths filter to only trigger on relevant file changes:

on:
  push:
    branches:
      - master
    paths:
      - 'src/**/*.js'
      - 'src/**/*.jsx'

10. Duplicate Translation Jobs

The workflow has two jobs (sync_translations and sync_translations_production) that run sequentially. They could run in parallel since they target different environments.

Recommendation: Both jobs can run in parallel - remove any implicit dependencies.

11. Console Logs in Production Code

Multiple locations use console.log and console.error which will appear in production:

• translation-init.js:37
• react_root_manager.js:20, 44, 55, 67

Recommendation: Either:

1. Use a proper logging library
2. Strip console logs in production builds
3. Use a __DEV__ flag check

12. Missing PropTypes Validation

Location: Various React components

After migration to React 18, PropTypes are still defined but could be replaced with TypeScript or at minimum should use prop-types package's strict mode.

Recommendation: Add to webpack config:

if (process.env.NODE_ENV === 'production') {
    config.resolve.alias = {
        'prop-types': path.resolve(__dirname, 'node_modules/prop-types'),
    };
}

✅ Good Practices Observed

1. Proper React 18 Migration Pattern: Using createRoot() instead of ReactDOM.render() ✓
2. Centralized Root Management: The react_root_manager.js is a clean abstraction ✓
3. Translation Package Integration: Using @deriv-com/translations standardizes translations across Deriv products ✓
4. Environment Variable Injection: Proper webpack DefinePlugin usage ✓
5. Backward Compatibility: Maintaining Crowdin integration for legacy workflows ✓
6. Cache Clearing: language.js:146 properly clears cache on language change ✓
7. URL Cleaning: Removing lang parameter after processing prevents URL pollution ✓

🔍 Code Quality Notes

Language Detection Flow

The new language detection logic is clean but complex:

1. Check for Crowdin override (ACH language)
2. Process URL parameter (save and clean)
3. Read from localStorage (JSON-stringified)
4. Fall back to default (EN)

This is well-implemented but should be documented with a flowchart or diagram.

React Root Management

The singleton pattern for managing roots is good, but consider:

• Adding TypeScript for type safety
• Exporting root count for debugging: export const getRootCount = () => roots.size;
• Adding a development-mode warning when roots exceed a threshold

🧪 Testing Recommendations

1. Test translation initialization failure scenarios
2. Test React root lifecycle (create, render, unmount, re-render)
3. Test language switching with page reloads
4. Test URL parameter processing and cleanup
5. Integration test for the full translation flow from CDN
6. Test environment variables missing/invalid scenarios

📚 Documentation Needed

1. Setup guide for new environment variables
2. Migration guide for developers on other branches
3. Architecture decision record explaining why @deriv-com/translations was chosen
4. API documentation for react_root_manager utilities
5. Troubleshooting guide for common translation issues

🚀 Before Merging

Must Fix:

• Add .env.example with required variables
• Fix translation initialization error handling (issue build(deps): bump actions/upload-artifact from 4.6.0 to 4.6.2 #2)
• Fix React 18 callback timing (issue build(deps): bump actions/checkout from 4.1.1 to 5.0.0 #3)
• Document the React root cleanup requirement

Should Fix:

• Add test coverage for new language detection logic
• Add error boundary for React 18
• Fix URL language function naming/behavior
• Add comments for Babel whitelist

Nice to Have:

• Optimize workflow triggers
• Add development mode logging flag
• Add TypeScript for React root manager
• Add architecture documentation

Summary

This PR represents a significant and necessary modernization. The migration is well-structured and follows React 18 best practices. The main concerns are around error handling, documentation, and production readiness. Once the critical issues are addressed, this should be safe to merge.

Estimated risk: Medium (due to missing env var docs and error handling)
Recommendation: Fix critical issues before merging to production

Great work on this complex migration! 🎉

---