Skip to content

fix(apollo-react): fix translations#42

Merged
CalinaCristian merged 1 commit intomainfrom
fix/translations
Dec 20, 2025
Merged

fix(apollo-react): fix translations#42
CalinaCristian merged 1 commit intomainfrom
fix/translations

Conversation

@CalinaCristian
Copy link
Collaborator

No description provided.

@vercel
Copy link

vercel bot commented Dec 20, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
apollo-ui-react Ready Ready Preview, Comment Dec 20, 2025 8:29pm
apollo-vertex Ready Ready Preview, Comment Dec 20, 2025 8:29pm

@github-actions
Copy link

github-actions bot commented Dec 20, 2025

🤖 AI Code Review (Claude)

⚠️ Automated Review: This is an AI-generated review. Use as guidance, not gospel.

Code Review: fix(apollo-react): fix translations

Summary

This PR fixes translation handling in the Apollo React package by:

  1. Removing dynamic locale loading in favor of pre-imported locales only
  2. Updating Lingui config to use message keys instead of English fallback
  3. Adding a build plugin to copy locale JSON files to the dist folder
  4. Simplifying test setup to use synchronous locale loading
  5. Fixing locale file formatting (variable interpolation syntax)

Code Quality

✅ Strengths

  • Simplified architecture: Removed complex async locale loading logic in favor of pre-imported locales, making the code more predictable
  • Better build process: Added explicit locale copying plugin ensures locale files are properly distributed
  • Cleaner test structure: Removed unnecessary async/await from tests since locales are now synchronous
  • Consistent formatting: Properly formatted all variable interpolations in locale files

⚠️ Minor Issues

  1. Error handling could be more robust in plugin-copy-locales.ts:

    • The plugin doesn't handle errors during directory traversal or file copying
    • Consider wrapping in try-catch and logging errors appropriately
  2. Missing validation in locale-registry.ts:

    • The extractMessages function logs a warning but returns empty object for unexpected formats
    • This is acceptable but could potentially mask issues during development

Security

No security concerns

  • The build plugin operates on repository files at build time (trusted source)
  • No user input is processed
  • No XSS vulnerabilities introduced in components
  • Locale files are from trusted design team exports

Type Safety

Type safety maintained

  • All TypeScript types are properly preserved
  • Removed unused getPreImportedMessages export (good cleanup)
  • LocaleMessages type properly constrains message format

Testing

Tests appropriately updated

  • Removed flaky async patterns from tests
  • Tests now properly set up i18n state before rendering
  • Registry tests added to verify pre-imported locale availability
  • Tests are more reliable with synchronous locale loading

Note: Tests are for package code (not playground apps), so this is appropriate.

Performance

Performance improvements

  • Pre-imported locales load synchronously = faster initial render
  • No dynamic imports = no waterfall loading delays
  • All locales bundled upfront may increase bundle size, but this is intentional for better UX

Additional Observations

Positive Changes

  1. Lingui config change (sourceLocale commented out): This makes sense when using message IDs as keys - allows missing translations to show the key instead of English text
  2. Build config update: Excluding both .js and .json locale files from source prevents duplication
  3. Code formatting improvements: The ap-chat.tsx formatting changes improve readability

Minor Suggestion

In plugin-copy-locales.ts, consider adding error handling:

api.onAfterBuild(() => {
  try {
    const srcDir = join(packageRoot, 'src');
    const distDir = join(packageRoot, 'dist');
    
    function findAndCopyLocales(dir: string, basePath = '') {
      try {
        for (const entry of readdirSync(dir)) {
          // ... existing logic
        }
      } catch (error) {
        console.error(`Error processing directory ${dir}:`, error);
      }
    }
    
    findAndCopyLocales(srcDir);
  } catch (error) {
    console.error('Failed to copy locale files:', error);
    throw error; // Fail build if locales can't be copied
  }
});

Recommendation

APPROVE

This is a solid refactoring that simplifies the translation system and makes it more reliable. The changes are well-structured, tests are properly updated, and there are no security or blocking issues. The minor error handling suggestion above is optional and shouldn't block the merge.

The PR successfully addresses translation issues by:

  • Removing fragile dynamic loading
  • Ensuring locales are properly distributed in builds
  • Making the system more predictable and testable

This automated review is temporary for solo development. Will be replaced with human reviews once the team grows.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ AI Review: Changes look good. Auto-approved by Claude AI.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI Review: Please review the concerns raised above before merging.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI Review: Please review the concerns raised above before merging.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI Review: Please review the concerns raised above before merging.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ AI Review: Changes look good. Auto-approved by Claude AI.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI Review: Please review the concerns raised above before merging.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI Review: Please review the concerns raised above before merging.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ AI Review: Please review the concerns raised above before merging.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ AI Review: Changes look good. Auto-approved by Claude AI.

@CalinaCristian CalinaCristian merged commit b6d61db into main Dec 20, 2025
9 checks passed
@CalinaCristian CalinaCristian deleted the fix/translations branch December 20, 2025 20:36
CalinaCristian pushed a commit that referenced this pull request Dec 30, 2025
* fix: timeline player color (#AG-123)

* fix: timeline player color (#AG-123)

* fix: timeline player speed (#AG-123)

* fix: timeline player speed (#AG-123)
CalinaCristian pushed a commit that referenced this pull request Dec 30, 2025
* fix: timeline player color (#AG-123)

* fix: timeline player color (#AG-123)

* fix: timeline player speed (#AG-123)

* fix: timeline player speed (#AG-123)
CalinaCristian pushed a commit that referenced this pull request Dec 30, 2025
* fix: timeline player color (#AG-123)

* fix: timeline player color (#AG-123)

* fix: timeline player speed (#AG-123)

* fix: timeline player speed (#AG-123)
CalinaCristian pushed a commit that referenced this pull request Dec 31, 2025
* fix: timeline player color (#AG-123)

* fix: timeline player color (#AG-123)

* fix: timeline player speed (#AG-123)

* fix: timeline player speed (#AG-123)
CalinaCristian pushed a commit that referenced this pull request Jan 6, 2026
* fix: timeline player color (#AG-123)

* fix: timeline player color (#AG-123)

* fix: timeline player speed (#AG-123)

* fix: timeline player speed (#AG-123)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant