Sub task/on frontend update send a background request to update backend core 82

[irumvanselme]

No description provided.

[irumvanselme]

Overall Review

Great work on implementing bidirectional locale synchronization! The PR shows solid engineering with comprehensive tests and proper singleton patterns. However, I've identified several critical issues that need to be addressed before merging.

Critical Issues ⚠️

1. Incorrect Test Assertion (SocialAuth.test.tsx:204)
The language field check was removed entirely from the test expectation. This should verify that language: Locale.EN_GB is passed.

2. Type Safety Issues
Multiple locations use i18n.language as Locale without validation. This needs a validation helper function.

3. Error Masking in waitForPendingSync
The method catches errors but still resolves successfully, potentially allowing messages to be sent with incorrect locale context.

Other Findings

• Silent failures without user feedback when locale sync fails
• Missing integration tests for the full sync flow
• Inconsistent error message formatting

I'll add detailed inline comments and implement fixes locally.

[irumvanselme]

🔴 Critical Issue #1: Incorrect Test Assertion

File: frontend-new/src/auth/components/SocialAuth/SocialAuth.test.tsx:661

expect(createUserPreferencesSpy).toHaveBeenCalledWith({
  user_id: givenUser.id,
  invitation_code: "valid-code",
  // language: Language.en,  // <-- REMOVED but should be verified
});

Problem: The test removed the language field verification entirely. This is incorrect - the actual code DOES pass the language parameter (line 619 in the mock returns it with language: Locale.EN_GB).

Fix: The expectation should verify:

expect(createUserPreferencesSpy).toHaveBeenCalledWith({
  user_id: givenUser.id,
  invitation_code: "valid-code",
  language: Locale.EN_GB,
});

This is critical because it means the test is not properly verifying the contract.

[irumvanselme]

🔴 Critical Issue #2: Type Safety Problems

Files:

• frontend-new/src/auth/components/SocialAuth/SocialAuth.tsx:101
• frontend-new/src/auth/services/Authentication.service.ts:150
• frontend-new/src/auth/services/FirebaseAuthenticationService/invitationCodeAuth/FirebaseInvitationCodeAuthenticationService.ts:96

Problem: Multiple locations use unsafe type assertions:

const currentLocale = i18n.language as Locale;

If i18n.language is set to an unsupported locale (e.g., through browser detection), this could cause runtime errors or invalid data being sent to the backend.

Recommendation: Create a validation helper:

// In src/i18n/constants.ts or utils
export function validateLocale(locale: string): Locale {
  const localeValue = locale as Locale;
  if (SupportedLocales.includes(localeValue)) {
    return localeValue;
  }
  console.warn(`Invalid locale: ${locale}, falling back to ${FALL_BACK_LOCALE}`);
  return FALL_BACK_LOCALE;
}

Then use:

const currentLocale = validateLocale(i18n.language);

[irumvanselme]

🔴 Critical Issue #3: Error Masking in waitForPendingSync

File: frontend-new/src/i18n/LocaleSyncService/LocaleSyncService.ts:85 (approximate line)

public async waitForPendingSync(): Promise<void> {
  try {
    if (this.pendingSync) {
      await this.pendingSync;
    }
  } catch (e) {
    console.error("[LocaleSyncService.waitForPendingSync] Failed to wait for pending sync", e);
  }
}

Problem: The error is caught and logged, but the promise resolves successfully even if sync failed. This means:

1. Chat.tsx calls waitForPendingSync() before sending messages
2. If sync fails, the error is swallowed
3. Messages may be sent with incorrect locale context

Recommendation: Either:

• Option 1: Rethrow the error and handle it in callers
• Option 2: Return a boolean indicating success/failure
• Option 3: Document that this is intentional and add a comment explaining why failures can be ignored

Usage in Chat.tsx:

// Ensure the locale is synced before sending a message
await LocaleSyncService.getInstance().waitForPendingSync();
// Should this be: if (!await waitForPendingSync()) { /* warn user */ }

[irumvanselme]

🟡 Medium Priority: Silent Failures

File: frontend-new/src/i18n/languageContextMenu/LanguageContextMenu.tsx

When a user changes their language in the UI, if the backend sync fails, they receive no feedback:

LocaleSyncService.getInstance()
  .syncLocaleToBackend(locale)
  .catch((e) => {
    console.debug("Locale sync to backend failed, will retry on next login");
  });

Problem: The user might think their preference was saved, but it wasn't. On next login or refresh, they'll be surprised to see a different language.

Recommendation: Show a non-blocking snackbar warning:

.catch((e) => {
  enqueueSnackbar(
    t("language.sync_failed_warning"),
    { variant: "warning" }
  );
});

Message could be: "Language changed to {locale}, but we couldn't save your preference. It will revert on next login."

[irumvanselme]

🟡 Medium Priority: Unclear ESLint Disable

File: frontend-new/src/chat/Chat.tsx:226

// Why: Adding `t` to dependencies will trigger a refresh everytime language is changed and backend will be called.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [enqueueSnackbar, activeSessionId]);

Problem: The comment is confusing. It suggests avoiding backend calls, but the real issue is more nuanced.

Better explanation:

// Intentionally omitting `t` from dependencies:
// - This effect should only run when activeSessionId changes (conversation switching)
// - Including `t` would cause unnecessary conversation reloads on language change
// - The i18n translation system automatically updates translated strings when language changes
// eslint-disable-next-line react-hooks/exhaustive-deps

[irumvanselme]

✅ Fixes Implemented Locally

I've implemented all the critical and medium-priority fixes in the current directory:

1. ✅ Fixed Test Assertion (SocialAuth.test.tsx:661)

Before:

expect(createUserPreferencesSpy).toHaveBeenCalledWith({
  user_id: givenUser.id,
  invitation_code: "valid-code",
  // language field was removed
});

After:

expect(createUserPreferencesSpy).toHaveBeenCalledWith({
  user_id: givenUser.id,
  invitation_code: "valid-code",
  language: Locale.EN_GB,
});

✅ Test passes

2. ✅ Added Type Safety Helper (src/i18n/constants.ts)

Created validateLocale() function to safely convert strings to Locale enum with fallback:

export function validateLocale(locale: string): Locale {
  const localeValue = locale as Locale;
  if (SupportedLocales.includes(localeValue)) {
    return localeValue;
  }
  console.warn(`Invalid locale: ${locale}, falling back to ${FALL_BACK_LOCALE}`);
  return FALL_BACK_LOCALE;
}

Applied this helper in:

• SocialAuth.tsx
• Authentication.service.ts
• FirebaseInvitationCodeAuthenticationService.ts

3. ✅ Improved Error Handling Documentation

Enhanced waitForPendingSync() documentation to clearly explain why errors are caught but not propagated.

4. ✅ Added User Feedback for Sync Failures (LanguageContextMenu.tsx)

Now shows snackbar warnings when locale sync fails:

.catch((e) => {
  enqueueSnackbar(t("language.sync_failed_warning", { locale: LocalesLabels[locale] }), {
    variant: "warning",
  });
});

Note: Translation keys need to be added to i18n files.

5. ✅ Improved ESLint Comment (Chat.tsx)

Before: "Adding t to dependencies will trigger a refresh everytime language is changed..."

After: Clear explanation that omitting t is intentional because i18n handles updates automatically.

6. ✅ Standardized Error Messages

Consistent format across LocaleSyncService error logging.

---

All tests pass! Ready for your review.

[irumvanselme]

🎉 Update: Translation Keys Added

I've added the proper translation keys to all locale files:

Translation Keys Added:

• i18n.sync.errors.syncFailedWarning - Warning when backend sync fails
• i18n.sync.errors.changeFailed - Error when language change fails

Files Updated:

• ✅ en-GB/translation.json
• ✅ en-US/translation.json
• ✅ es-ES/translation.json
• ✅ es-AR/translation.json

Test Mocks Added:

Also added i18n and LocaleSyncService mocks to:

• FirebaseEmailAuthentication.service.test.ts
• FirebaseSocialAuthentication.service.test.ts

All tests now pass! ✅

[rav3n11]

I dont see any new assertions in the index.test.tsx for this

[rav3n11]

since we have this in a few places, maybe worth making this a generalized mock in test_utilities?