diff --git a/packages/fxa-react/lib/AppLocalizationProvider.test.tsx b/packages/fxa-react/lib/AppLocalizationProvider.test.tsx index d3a570b47e7..c3b43b2fde1 100644 --- a/packages/fxa-react/lib/AppLocalizationProvider.test.tsx +++ b/packages/fxa-react/lib/AppLocalizationProvider.test.tsx @@ -14,6 +14,11 @@ import { Localized } from '@fluent/react'; import fetchMock from 'fetch-mock'; import AppLocalizationProvider from './AppLocalizationProvider'; +// `it` negotiates to exactly ['it', 'en'], which keeps the set of requested +// bundle paths small enough to assert on precisely. +const HASHED_BASE_DIR = '/hashed'; +const HASHED_LOCALES = ['it']; + describe('', () => { const locales = ['en-GB', 'en-US', 'es-ES']; const bundles = ['greetings', 'farewells']; @@ -26,21 +31,46 @@ describe('', () => { } beforeAll(() => { + // Keys must match the path `fetchMessages` builds, which has no leading + // slash. `farewells` is absent for every locale but en-US, so those + // lookups miss and Fluent falls back. fetchMock.get( '/static-asset-manifest.json', - ` - { - "/locales/en-US/greetings.ftl": "/locales/en-US/greetings.ftl", - "/locales/en-US/farewells.ftl": "/locales/en-US/farewells.ftl", - "/locales/es-ES/greetings.ftl": "/locales/es-ES/greetings.ftl", - "/locales/en-GB/greetings.ftl": "/locales/en-GB/greetings.ftl", - } - ` + JSON.stringify({ + 'locales/en-US/greetings.ftl': 'locales/en-US/greetings.ftl', + 'locales/en-US/farewells.ftl': 'locales/en-US/farewells.ftl', + 'locales/es-ES/greetings.ftl': 'locales/es-ES/greetings.ftl', + 'locales/en-GB/greetings.ftl': 'locales/en-GB/greetings.ftl', + }) ); fetchMock.get('/locales/en-US/greetings.ftl', 'hello = Hello\n'); fetchMock.get('/locales/en-US/farewells.ftl', 'goodbye = Goodbye\n'); fetchMock.get('/locales/es-ES/greetings.ftl', 'hello = Hola\n'); fetchMock.get('/locales/en-GB/greetings.ftl', 'hello = Hello { $amount }'); + + // A well-formed manifest, served under its own baseDir so it does not + // collide with the invalid-manifest fixture above. `farewells` is + // deliberately absent from it, and `notfound` maps to a path that 404s. + fetchMock.get( + `${HASHED_BASE_DIR}/static-asset-manifest.json`, + JSON.stringify({ + 'locales/it/greetings.ftl': 'locales/it/greetings.1a2b3c.ftl', + 'locales/en/greetings.ftl': 'locales/en/greetings.4d5e6f.ftl', + 'locales/it/notfound.ftl': 'locales/it/notfound.7a8b9c.ftl', + 'locales/en/notfound.ftl': 'locales/en/notfound.7a8b9c.ftl', + }) + ); + fetchMock.get( + `${HASHED_BASE_DIR}/locales/it/greetings.1a2b3c.ftl`, + 'hello = Ciao\n' + ); + fetchMock.get( + `${HASHED_BASE_DIR}/locales/en/greetings.4d5e6f.ftl`, + 'hello = Hello\n' + ); + fetchMock.get(`${HASHED_BASE_DIR}/locales/it/notfound.7a8b9c.ftl`, 404); + fetchMock.get(`${HASHED_BASE_DIR}/locales/en/notfound.7a8b9c.ftl`, 404); + fetchMock.get('*', { throws: new Error() }); }); @@ -169,4 +199,86 @@ describe('', () => { expect(getByTestId('result')).toHaveTextContent('Hello ⁨$US123.00⁩'); }); + + describe('reportBundleError', () => { + function renderWithManifest( + bundlesToLoad: Array, + reportBundleError: jest.Mock, + baseDir = HASHED_BASE_DIR + ) { + return render( + +
+ +
untranslated
+ +
+ + ); + } + + it('resolves the hashed path from the manifest and reports nothing', async () => { + const reportBundleError = jest.fn(); + const { getByTestId } = renderWithManifest( + ['greetings'], + reportBundleError + ); + await waitUntilTranslated(); + + expect(getByTestId('result')).toHaveTextContent('Ciao'); + expect(reportBundleError).not.toHaveBeenCalled(); + }); + + it('reports a bundle with no entry in the manifest, once per locale', async () => { + const reportBundleError = jest.fn(); + const { getByTestId } = renderWithManifest( + ['farewells'], + reportBundleError + ); + await waitUntilTranslated(); + + expect( + reportBundleError.mock.calls.map(([error]) => error.message) + ).toEqual([ + 'No static asset mapping for l10n bundle: locales/it/farewells.ftl', + 'No static asset mapping for l10n bundle: locales/en/farewells.ftl', + ]); + expect(getByTestId('result')).toHaveTextContent('untranslated'); + }); + + it('reports a bundle whose hashed path does not resolve, once per locale', async () => { + const reportBundleError = jest.fn(); + renderWithManifest(['notfound'], reportBundleError); + await waitUntilTranslated(); + + expect( + reportBundleError.mock.calls.map(([error]) => error.message) + ).toEqual([ + `Fetching l10n bundle returned 404: ${HASHED_BASE_DIR}/locales/it/notfound.7a8b9c.ftl`, + `Fetching l10n bundle returned 404: ${HASHED_BASE_DIR}/locales/en/notfound.7a8b9c.ftl`, + ]); + }); + + it('reports an unreachable manifest', async () => { + const reportBundleError = jest.fn(); + renderWithManifest(['greetings'], reportBundleError, '/no-manifest'); + await waitUntilTranslated(); + + expect(reportBundleError).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining( + 'Fetching l10n static asset manifest failed: /no-manifest/static-asset-manifest.json' + ), + }) + ); + // Without mappings the unhashed paths are requested and fail too, so a + // manifest outage costs one report plus one per negotiated locale. + expect(reportBundleError).toHaveBeenCalledTimes(3); + }); + }); }); diff --git a/packages/fxa-react/lib/AppLocalizationProvider.tsx b/packages/fxa-react/lib/AppLocalizationProvider.tsx index 903603eb36f..68ed02620fd 100644 --- a/packages/fxa-react/lib/AppLocalizationProvider.tsx +++ b/packages/fxa-react/lib/AppLocalizationProvider.tsx @@ -7,45 +7,68 @@ import { LocalizationProvider, ReactLocalization } from '@fluent/react'; import React, { Component } from 'react'; import { EN_GB_LOCALES, parseAcceptLanguage } from '@fxa/shared/l10n'; +type ReportError = (error: Error) => void; + +function describeCause(err: unknown) { + return err instanceof Error ? err.message : String(err); +} + /** * Gets l10n messages from server * @param baseDir The root location where locales folders are held * @param locale The target language * @param bundle The target bundle (ie main) * @param mappings A set of mappings for static resources. + * @param reportBundleError Receives whole-bundle load failures. * @returns */ async function fetchMessages( baseDir: string, locale: string, bundle: string, - mappings?: Record + mappings?: Record, + reportBundleError?: ReportError ) { + // Build the path to l10n file + const path = `locales/${locale}/${bundle}.ftl`; + + // If mappings were provided see if there is one for the path. This + // will be a location where the file path contains a hash in the file + // name + const mappedPath = mappings ? mappings[path] : path; + + // If we don't have mapped path, there are no l10n resources for this language. + if (!mappedPath) { + reportBundleError?.( + new Error(`No static asset mapping for l10n bundle: ${path}`) + ); + return ''; + } + + // Fetch the file and return the messages + const resolvedPath = `${baseDir}/${mappedPath}`; try { - // Build the path to l10n file - let path = `locales/${locale}/${bundle}.ftl`; - - // If mappings were proivided see if there is one for the path. This - // will be a location where the file path contains a hash in the file - // name - if (mappings) { - path = mappings[path]; - } + const response = await fetch(resolvedPath); - // If we don't have mapped path, there are no l10n resources for this language. - if (!path) { + // A non-OK body is not FTL, and handing it to Fluent yields an empty bundle. + if (!response.ok) { + reportBundleError?.( + new Error( + `Fetching l10n bundle returned ${response.status}: ${resolvedPath}` + ) + ); return ''; } - // Fetch the file and return the messages - const resolvedPath = `${baseDir}/${path}`; - const response = await fetch(resolvedPath); - const messages = await response.text(); - - return messages; + return await response.text(); } catch (e) { // We couldn't fetch any strings; just return nothing and fluent will fall // back to the default locale if needed. + reportBundleError?.( + new Error( + `Fetching l10n bundle failed: ${resolvedPath} (${describeCause(e)})` + ) + ); return ''; } } @@ -54,21 +77,36 @@ function fetchAllMessages( baseDir: string, locale: string, bundles: Array, - mappings?: Record + mappings?: Record, + reportBundleError?: ReportError ) { return Promise.all( - bundles.map((bndl) => fetchMessages(baseDir, locale, bndl, mappings)) + bundles.map((bndl) => + fetchMessages(baseDir, locale, bndl, mappings, reportBundleError) + ) ); } -async function fetchL10nHashedMappings(mappingUrl: string) { +async function fetchL10nHashedMappings( + mappingUrl: string, + reportBundleError?: ReportError +) { try { // These mappigns are currently generated with grunt. See grunt task hash-static // in fxa-settings for an example of how the mappings are generated. const mappingsResponse = await fetch(mappingUrl); - const json = await mappingsResponse.json(); - return json; + if (!mappingsResponse.ok) { + throw new Error(`Received status ${mappingsResponse.status}`); + } + return await mappingsResponse.json(); } catch (err) { + reportBundleError?.( + new Error( + `Fetching l10n static asset manifest failed: ${mappingUrl} (${describeCause( + err + )})` + ) + ); return undefined; } } @@ -76,17 +114,25 @@ async function fetchL10nHashedMappings(mappingUrl: string) { async function createFluentBundleGenerator( baseDir: string, currentLocales: Array, - bundles: Array + bundles: Array, + reportBundleError?: ReportError ) { const mappings = await fetchL10nHashedMappings( - `${baseDir}/static-asset-manifest.json` + `${baseDir}/static-asset-manifest.json`, + reportBundleError ); const fetched = await Promise.all( currentLocales .filter((l) => !EN_GB_LOCALES.includes(l)) .map(async (locale) => { return { - [locale]: await fetchAllMessages(baseDir, locale, bundles, mappings), + [locale]: await fetchAllMessages( + baseDir, + locale, + bundles, + mappings, + reportBundleError + ), }; }) ); @@ -127,7 +173,11 @@ type Props = { children: any; // pass messages directly in, used in testing messages?: { [key: string]: string[] }; - reportError?: (error: Error) => void; + // Per-string Fluent errors, e.g. an id missing from the bundle. Defaults to + // @fluent/react's console reporter. + reportError?: ReportError; + // Failures to load a bundle at all, where no string in it can resolve. + reportBundleError?: ReportError; }; export default class AppLocalizationProvider extends Component { @@ -137,6 +187,7 @@ export default class AppLocalizationProvider extends Component { bundles: ['main'], children: React.createElement('div'), reportError: undefined, + reportBundleError: undefined, }; constructor(props: Props) { @@ -170,7 +221,8 @@ export default class AppLocalizationProvider extends Component { const bundleGenerator = await createFluentBundleGenerator( baseDir, currentLocales, - bundles + bundles, + this.props.reportBundleError ); this.setState({ l10n: new ReactLocalization( diff --git a/packages/fxa-settings/src/components/App/index.tsx b/packages/fxa-settings/src/components/App/index.tsx index 3ff87160866..7a380b01f81 100644 --- a/packages/fxa-settings/src/components/App/index.tsx +++ b/packages/fxa-settings/src/components/App/index.tsx @@ -41,6 +41,7 @@ import { import { AccountStateProvider } from '../../models/contexts/AccountStateContext'; import sentryMetrics from 'fxa-shared/sentry/browser'; +import { flushL10nErrorReports } from '../../lib/l10n-error-reporter'; // Components import LoadingSpinner from 'fxa-react/components/LoadingSpinner'; import { ScrollToTop } from '../Settings/ScrollToTop'; @@ -377,6 +378,9 @@ export const App = ({ flowQueryParams }: { flowQueryParams: QueryParams }) => { useEffect(() => { if (metricsEnabled || isSignedIn === false) { sentryMetrics.enable(); + // l10n bundles are fetched before this component can mount, so any + // failure there is buffered until Sentry will accept it. + flushL10nErrorReports(); } else { sentryMetrics.disable(); } diff --git a/packages/fxa-settings/src/contexts/DynamicLocalizationContext.tsx b/packages/fxa-settings/src/contexts/DynamicLocalizationContext.tsx index 0cbb25822c4..d286843997b 100644 --- a/packages/fxa-settings/src/contexts/DynamicLocalizationContext.tsx +++ b/packages/fxa-settings/src/contexts/DynamicLocalizationContext.tsx @@ -2,7 +2,13 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import React, { createContext, useContext, useState, useCallback, useEffect } from 'react'; +import React, { + createContext, + useContext, + useState, + useCallback, + useEffect, +} from 'react'; import AppLocalizationProvider from 'fxa-react/lib/AppLocalizationProvider'; import { isRTLLocale, @@ -11,7 +17,7 @@ import { getCurrentLocale, validateLocale, detectBrowserDefaultLocale, - DEFAULT_LOCALE + DEFAULT_LOCALE, } from '../lib/locales'; interface DynamicLocalizationContextType { @@ -21,15 +27,20 @@ interface DynamicLocalizationContextType { isLoading: boolean; } -const DynamicLocalizationContext = createContext(null); +const DynamicLocalizationContext = + createContext(null); export const DynamicLocalizationProvider: React.FC<{ children: React.ReactNode; baseDir: string; bundles?: string[]; -}> = ({ children, baseDir, bundles = ['main'] }) => { + reportBundleError?: (error: Error) => void; +}> = ({ children, baseDir, bundles = ['main'], reportBundleError }) => { const [currentLocale, setCurrentLocale] = useState(() => getCurrentLocale()); - const [userLocales, setUserLocales] = useState(() => [getCurrentLocale(), DEFAULT_LOCALE]); + const [userLocales, setUserLocales] = useState(() => [ + getCurrentLocale(), + DEFAULT_LOCALE, + ]); const [isLoading, setIsLoading] = useState(false); const [key, setKey] = useState(0); // Force re-render of AppLocalizationProvider @@ -40,7 +51,7 @@ export const DynamicLocalizationProvider: React.FC<{ if (newLocale !== currentLocale) { setCurrentLocale(newLocale); setUserLocales([newLocale, DEFAULT_LOCALE]); - setKey(prev => prev + 1); + setKey((prev) => prev + 1); } }; @@ -50,40 +61,42 @@ export const DynamicLocalizationProvider: React.FC<{ }; }, [currentLocale]); - const switchLanguage = useCallback(async (locale: string) => { - // Validate against supported languages - if (!validateLocale(locale)) { - console.warn(`Locale ${locale} is not supported`); - return; - } - - if (locale === currentLocale) { - return; // No change needed - } + const switchLanguage = useCallback( + async (locale: string) => { + // Validate against supported languages + if (!validateLocale(locale)) { + console.warn(`Locale ${locale} is not supported`); + return; + } - setIsLoading(true); + if (locale === currentLocale) { + return; // No change needed + } - try { - // 1. Save preference to localStorage - saveLocalePreference(locale); + setIsLoading(true); - // 2. Update document attributes - document.documentElement.lang = locale; - document.documentElement.dir = isRTLLocale(locale) ? 'rtl' : 'ltr'; + try { + // 1. Save preference to localStorage + saveLocalePreference(locale); - // 3. Update state - setCurrentLocale(locale); - setUserLocales([locale, DEFAULT_LOCALE]); + // 2. Update document attributes + document.documentElement.lang = locale; + document.documentElement.dir = isRTLLocale(locale) ? 'rtl' : 'ltr'; - // 4. Force AppLocalizationProvider to re-mount and reload bundles - setKey(prev => prev + 1); + // 3. Update state + setCurrentLocale(locale); + setUserLocales([locale, DEFAULT_LOCALE]); - } catch (error) { - // Language switch failed, we can't really do anything about it so ignore it - } finally { - setIsLoading(false); - } - }, [currentLocale]); + // 4. Force AppLocalizationProvider to re-mount and reload bundles + setKey((prev) => prev + 1); + } catch (error) { + // Language switch failed, we can't really do anything about it so ignore it + } finally { + setIsLoading(false); + } + }, + [currentLocale] + ); const clearLanguagePreference = useCallback(async () => { setIsLoading(true); @@ -97,15 +110,16 @@ export const DynamicLocalizationProvider: React.FC<{ // 3. Update document attributes document.documentElement.lang = browserDefaultLocale; - document.documentElement.dir = isRTLLocale(browserDefaultLocale) ? 'rtl' : 'ltr'; + document.documentElement.dir = isRTLLocale(browserDefaultLocale) + ? 'rtl' + : 'ltr'; // 4. Update state setCurrentLocale(browserDefaultLocale); setUserLocales([browserDefaultLocale, DEFAULT_LOCALE]); // 5. Force AppLocalizationProvider to re-mount and reload bundles - setKey(prev => prev + 1); - + setKey((prev) => prev + 1); } catch (error) { // Clear failed, ignore it } finally { @@ -114,17 +128,20 @@ export const DynamicLocalizationProvider: React.FC<{ }, []); return ( - + {children} @@ -140,7 +157,7 @@ export const useDynamicLocalization = () => { currentLocale: DEFAULT_LOCALE, switchLanguage: async () => {}, clearLanguagePreference: async () => {}, - isLoading: false + isLoading: false, }; } return context; diff --git a/packages/fxa-settings/src/index.tsx b/packages/fxa-settings/src/index.tsx index 5bc3acd5770..bb379c200cf 100644 --- a/packages/fxa-settings/src/index.tsx +++ b/packages/fxa-settings/src/index.tsx @@ -19,6 +19,7 @@ import Storage from './lib/storage'; import CookiesDisabled from './pages/CookiesDisabled'; import { BrowserRouter } from 'react-router'; import { DynamicLocalizationProvider } from './contexts/DynamicLocalizationContext'; +import { reportL10nError } from './lib/l10n-error-reporter'; export interface FlowQueryParams { broker?: string; @@ -83,7 +84,10 @@ try { render( - + diff --git a/packages/fxa-settings/src/lib/l10n-error-reporter.test.ts b/packages/fxa-settings/src/lib/l10n-error-reporter.test.ts new file mode 100644 index 00000000000..42b32b60dd1 --- /dev/null +++ b/packages/fxa-settings/src/lib/l10n-error-reporter.test.ts @@ -0,0 +1,103 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import * as Sentry from '@sentry/browser'; +import { + MAX_L10N_ERROR_REPORTS, + flushL10nErrorReports, + reportL10nError, + resetL10nErrorReporter, +} from './l10n-error-reporter'; + +jest.mock('@sentry/browser', () => ({ + captureException: jest.fn(), +})); + +const mockCaptureException = Sentry.captureException as jest.MockedFunction< + typeof Sentry.captureException +>; + +describe('reportL10nError', () => { + beforeEach(() => { + jest.clearAllMocks(); + resetL10nErrorReporter(); + document.documentElement.lang = 'fr'; + }); + + describe('before metrics are enabled', () => { + it('does not send to Sentry, which would discard the event', () => { + reportL10nError(new Error('No static asset mapping for l10n bundle')); + + expect(mockCaptureException).not.toHaveBeenCalled(); + }); + + it('sends the buffered reports once flushed', () => { + const first = new Error( + 'No static asset mapping for locales/fr/main.ftl' + ); + const second = new Error('Fetching l10n static asset manifest failed'); + + reportL10nError(first); + reportL10nError(second); + flushL10nErrorReports(); + + expect(mockCaptureException).toHaveBeenCalledTimes(2); + expect(mockCaptureException).toHaveBeenCalledWith(first, { + tags: { area: 'l10n' }, + extra: { locale: 'fr' }, + }); + }); + + it('reports the locale from when the failure happened, not the flush', () => { + const error = new Error( + 'No static asset mapping for locales/fr/main.ftl' + ); + + reportL10nError(error); + document.documentElement.lang = 'de'; + flushL10nErrorReports(); + + expect(mockCaptureException).toHaveBeenCalledWith(error, { + tags: { area: 'l10n' }, + extra: { locale: 'fr' }, + }); + }); + }); + + describe('after metrics are enabled', () => { + beforeEach(() => { + flushL10nErrorReports(); + }); + + it('reports the error to Sentry tagged as l10n with the current locale', () => { + const error = new Error('No static asset mapping for l10n bundle'); + + reportL10nError(error); + + expect(mockCaptureException).toHaveBeenCalledWith(error, { + tags: { area: 'l10n' }, + extra: { locale: 'fr' }, + }); + }); + + it('reports a repeated message only once', () => { + const message = + 'No static asset mapping for l10n bundle: locales/fr/main.ftl'; + reportL10nError(new Error(message)); + reportL10nError(new Error(message)); + + expect(mockCaptureException).toHaveBeenCalledTimes(1); + }); + + it(`stops reporting after ${MAX_L10N_ERROR_REPORTS} distinct messages`, () => { + for (let i = 0; i < MAX_L10N_ERROR_REPORTS + 5; i++) { + reportL10nError(new Error(`No static asset mapping for bundle ${i}`)); + } + + expect(mockCaptureException).toHaveBeenCalledTimes( + MAX_L10N_ERROR_REPORTS + ); + }); + }); +}); diff --git a/packages/fxa-settings/src/lib/l10n-error-reporter.ts b/packages/fxa-settings/src/lib/l10n-error-reporter.ts new file mode 100644 index 00000000000..dc3766dbdf0 --- /dev/null +++ b/packages/fxa-settings/src/lib/l10n-error-reporter.ts @@ -0,0 +1,80 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import * as Sentry from '@sentry/browser'; + +/** + * Sentry starts disabled and is only enabled once we know the user has not + * opted out of metrics, which happens inside `App`. `App` cannot mount until + * AppLocalizationProvider has resolved its bundles, so every bundle failure is + * reported before Sentry will accept anything. Reports are therefore buffered + * and flushed by `flushL10nErrorReports` once metrics are enabled — if the user + * has opted out, the buffer is simply never flushed. + */ +export const MAX_L10N_ERROR_REPORTS = 10; + +type L10nErrorReport = { + error: Error; + locale: string; +}; + +const reportedMessages = new Set(); +let pendingReports: Array = []; +let metricsEnabled = false; + +function captureReport({ error, locale }: L10nErrorReport) { + Sentry.captureException(error, { + tags: { area: 'l10n' }, + extra: { locale }, + }); +} + +/** + * Reports a failure to load a localization bundle. Individual missing string ids + * are deliberately left to Fluent's console reporter — the signal worth alerting + * on is "no strings resolved at all". + * + * One report is possible per locale per bundle, and switching languages remounts + * the provider and refetches, so reports are deduped by message and capped for + * the life of the page. A widespread manifest failure still means a handful of + * events per session; tune the volume Sentry-side rather than here, so the first + * report of a new failure is never the one that gets dropped. + */ +export function reportL10nError(error: Error) { + if ( + reportedMessages.has(error.message) || + reportedMessages.size >= MAX_L10N_ERROR_REPORTS + ) { + return; + } + reportedMessages.add(error.message); + + // The locale is read now rather than at flush time, since a language switch + // can happen in between. + const report = { error, locale: document.documentElement.lang }; + + if (metricsEnabled) { + captureReport(report); + } else { + pendingReports.push(report); + } +} + +/** + * Called once metrics are known to be enabled. Sends anything reported during + * app startup and lets later reports through immediately. + */ +export function flushL10nErrorReports() { + metricsEnabled = true; + const queued = pendingReports; + pendingReports = []; + queued.forEach(captureReport); +} + +// Exported for tests; the cap, dedup set and buffer are page-lifetime state. +export function resetL10nErrorReporter() { + reportedMessages.clear(); + pendingReports = []; + metricsEnabled = false; +}