refactor: textresources in globaldata #17589
Conversation
github-actions
bot
added
skip-releasenotes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughBackend accepts an optional lang query and slug-constrained routes; bootstrap returns resolved text resources. Frontend replaces provider/context text-resource plumbing with React Query hooks, removes LangToolsStore/provider, adds useLangToolsDataSources and useTextResources, and updates many tests to mock the hook-based API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant HomeCtrl as HomeController
participant Router as ASP.NET Router
participant Bootstrap as BootstrapGlobalService
participant Profile as ProfileClient
participant Resources as AppResources
Client->>Router: GET /{org:slug}/{app:slug}?lang=en
Router->>HomeCtrl: route to HomeController.Index(org, app, returnUrl, lang)
HomeCtrl->>Bootstrap: GetGlobalState(org, app, returnUrl, "en")
Bootstrap->>Bootstrap: Determine language (URL -> cookie -> user pref -> default)
Bootstrap->>Profile: GetUserProfile()
Profile-->>Bootstrap: user preferences
Bootstrap->>Resources: GetTextResources(org, app, resolvedLanguage)
Resources-->>Bootstrap: TextResource result
Bootstrap-->>HomeCtrl: BootstrapGlobalResponse (includes TextResources, AvailableLanguages)
HomeCtrl-->>Client: Rendered HTML with embedded bootstrap state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cammiida
force-pushed
the
refactor/textresources-in-globaldata
branch
from
b4e8c16 to
1bea713
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/App/backend/src/Altinn.App.Api/Controllers/HomeController.cs (1)
78-93: Validate/normaliselangbefore use.
langis passed straight through; unsupported values (e.g., region variants) can cause missing resources or unexpected fallbacks. Consider normalising and rejecting invalid codes so the system behaves predictably.🔧 Suggested guard (normalise and fallback)
@@ public async Task<IActionResult> Index( [FromRoute] string org, [FromRoute] string app, [FromQuery] bool dontChooseReportee, [FromQuery] string? returnUrl, [FromQuery] string? lang = null ) { + if (!string.IsNullOrWhiteSpace(lang)) + { + lang = lang.Trim().ToLowerInvariant(); + var isValid = lang.Length is >= 2 and <= 3; + if (isValid) + { + for (var i = 0; i < lang.Length; i++) + { + var c = lang[i]; + if (c < 'a' || c > 'z') + { + isValid = false; + break; + } + } + } + + if (!isValid) + { + _logger.LogWarning("Ignoring unsupported lang '{Lang}'", lang); + lang = null; + } + } + // See comments in the configuration of Antiforgery in MvcConfiguration.cs. var tokens = _antiforgery.GetAndStoreTokens(HttpContext);Based on learnings: “text resource files only support ISO 639 standard parts 1 and 3 language codes (2-3 letter codes like "nb", "en", "nno"). Extended language variants with region codes (like "nb_NO" or "en-GB") are not supported.”
🤖 Fix all issues with AI agents
In
`@src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs`:
- Around line 159-164: The deserialization of the language cookie using
JsonSerializer.Deserialize<string> in BootstrapGlobalService can throw
JsonException on malformed JSON; wrap the call that produces languageCookieValue
in a try-catch that catches JsonException (and optionally Exception), log a
warning or error via _logger including the offending languageCookie value and
context (org/app) and return null when deserialization fails so bootstrap flow
continues safely instead of throwing.
In
`@src/App/frontend/src/features/language/textResources/TextResourcesProvider.tsx`:
- Around line 16-18: getTextResourcesFromWindow currently assumes
window.altinnAppGlobalData exists and will throw if not; change the
implementation to safely access window.altinnAppGlobalData.textResources (e.g.,
using optional chaining or a null check) and return a sensible default (like an
empty array/object) when the global is undefined so Storybook/tests/non-app
contexts don't crash; update the function getTextResourcesFromWindow to use this
guarded access and return the safe default.
In `@src/App/frontend/src/features/language/useLangToolsDataSources.tsx`:
- Around line 28-31: selectedAppLanguage can include region variants like
"nb-NO" which bypass validation and cause getLanguageFromCode to fall back to
"en"; fix by normalising region-formatted codes before lookup in
useLangToolsDataSources (or update getLanguageFromCode) — extract the primary
language subtag (substring before '-') or lower-case and trim the value, then
call getLanguageFromCode(normalised) and/or add explicit fallback mapping for
common region codes (e.g., treat "nb-..." as "nb" and "nn-..." as "nn") so
language resolution never silently defaults to "en".
🧹 Nitpick comments (12)
src/App/frontend/src/features/language/textResources/TextResourcesProvider.tsx (1)
19-39: Align React Query key/config with the shared queryOptions pattern.Consider extracting query configuration into a
queryOptionshelper and using an object-based key to match the project convention and keep query config shareable across callers. Please confirmqueryOptionsis available in the TanStack Query version used here. As per coding guidelines, "Use objects for managing TanStack Query keys and functions; usequeryOptionsfor sharing query configuration centrally".src/App/frontend/src/features/expressions/shared-functions.test.tsx (1)
25-25: Consider seeding the QueryClient instead of mocking the hook.For React Query-backed data, seeding
QueryClient.setQueryDatakeeps tests closer to real cache behaviour and avoids hook-level mocks. Based on learnings, "In Altinn/altinn-studio PR reviews, when unit tests need React Query/TanStack Query data, prefer seeding with QueryClient.setQueryData over rendering/mocking use…Query hooks and waiting for isSuccess. Flag patterns like renderHookWithMockStore(() => use…Query(...)) and waitFor(() => expect(result.current.isSuccess).toBe(true)) and recommend the simpler setQueryData approach. Cite the internal Confluence guideline on component tests and the best-practice example in useAddItemToLayoutMutation.test.ts (lines updated 08.08.2025)."Also applies to: 189-194
src/App/frontend/test/e2e/integration/frontend-test/language.ts (1)
28-28: Test name may need updating to reflect cookie-based storage.The test is named
'should not crash if language is stored as "null" in local storage'but now clears a cookie instead. Consider renaming to reflect the actual storage mechanism.📝 Suggested test name update
- it('should not crash if language is stored as "null" in local storage', () => { + it('should not crash if language cookie is cleared', () => {src/App/frontend/test/e2e/integration/frontend-test/instantiation.ts (1)
124-139: Consider extracting the duplicated text resource definition.This text resource definition is identical to lines 68-83. While duplication in tests is often acceptable for clarity, you could extract a shared constant if preferred:
const customErrorTooLongResource = { id: 'custom_error_too_long', value: 'Verdien kan ikke være lengre enn {0}, den er nå {1}', variables: [ { key: 'max_length', dataSource: 'customTextParameters' }, { key: 'current_length', dataSource: 'customTextParameters' }, ], };src/App/frontend/src/index.tsx (1)
49-71: Router created inline on every render - consider hoisting.
createBrowserRouteris called inline within the JSX, meaning it will be recreated on every render of the parent component. React Router recommends creating the router instance outside of the React tree to avoid unnecessary re-instantiation.♻️ Proposed fix: hoist router creation
+const router = createBrowserRouter( + [ + { + path: '*', + element: ( + <NavigationEffectProvider> + <ErrorBoundary> + <Root /> + </ErrorBoundary> + </NavigationEffectProvider> + ), + }, + ], + { + future: { + v7_relativeSplatPath: true, + }, + basename: `/${window.org}/${window.app}`, + }, +); document.addEventListener('DOMContentLoaded', () => { propagateTraceWhenPdf(); const container = document.getElementById('root'); const root = container && createRoot(container); root?.render( <AppQueriesProvider {...queries}> <ErrorBoundary> <AppPrefetcher /> - <RouterProvider - router={createBrowserRouter( - [ - { - path: '*', - element: ( - <NavigationEffectProvider> - <ErrorBoundary> - <Root /> - </ErrorBoundary> - </NavigationEffectProvider> - ), - }, - ], - { - future: { - v7_relativeSplatPath: true, - }, - basename: `/${window.org}/${window.app}`, - }, - )} - future={{ v7_startTransition: true }} - /> + <RouterProvider router={router} future={{ v7_startTransition: true }} /> </ErrorBoundary> </AppQueriesProvider>, ); });src/App/frontend/src/layout/RepeatingGroup/Summary/SummaryRepeatingGroup.test.tsx (1)
10-23: Consider using the global mock from setupTests.ts or extracting shared mock.This mock pattern is identical to the one in
SummaryGroupComponent.test.tsx. SincesetupTests.tsalready provides a global mock foruseTextResources, you could either:
- Add these keys to
getTextResourceMapMock()if they're commonly needed, or- Extract this mock setup to a shared test utility if multiple tests need the same overrides.
That said, the current approach works correctly and provides clear isolation for these tests.
src/App/frontend/src/layout/Group/SummaryGroupComponent.test.tsx (1)
9-22: Identical mock to SummaryRepeatingGroup.test.tsx - consider consolidation.This mock is an exact duplicate of the one in
SummaryRepeatingGroup.test.tsx. Both files could share a common mock setup to reduce maintenance burden.src/App/frontend/src/components/form/Form.test.tsx (1)
9-9: Useimport typefor type-only imports.
TextResourceMapis only used as a type annotation. Usingimport typeensures it's erased at compile time and signals intent.Suggested fix
-import { TextResourceMap } from 'src/features/language/textResources'; +import type { TextResourceMap } from 'src/features/language/textResources';src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs (1)
102-102: Use structured logging placeholders instead of string interpolation.String interpolation in log messages bypasses structured logging benefits like filtering and performance optimisations when the log level is disabled.
Suggested fix
- _logger.LogDebug($"languageCookieValue: {languageCookieValue}"); + _logger.LogDebug("languageCookieValue: {LanguageCookieValue}", languageCookieValue);src/App/backend/test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json (1)
2529-2536: Clarify supported language code format forlang.Consider stating that the value must be an ISO 639-1/3 code (e.g.,
nb,en,nno) and that region variants are not supported. This avoids ambiguity for API consumers.Based on learnings, text resource files only support ISO 639 standard parts 1 and 3 language codes (2-3 letter codes like "nb", "en", "nno"), while extended variants like "nb_NO" or "en-GB" are not supported.
src/App/frontend/src/core/texts/appTexts.ts (1)
17-21: Consider renaming to reflect actual behaviour.The function
useHasAppTextsYetnow only checks for orgs availability, but the name suggests it checks for text resources. Since text resources are now loaded via global data (window), this check may be appropriate, but the name could be misleading to future maintainers.Consider renaming to something like
useHasAppDataYetor adding a comment explaining why orgs availability is sufficient.src/App/frontend/src/features/language/useLangToolsDataSources.tsx (1)
11-19: Remove'node'and'transposeSelector'from theOmitstatement—they do not exist onTextResourceVariablesDataSources.The interface currently contains only
defaultDataType,formDataTypes, andformDataSelectoramong the keys listed for omission. Keeping non-existent keys in theOmitis redundant and obscures the actual contract of the type, potentially masking future type drift. Trim the statement to:export type LimitedTextResourceVariablesDataSources = Omit< TextResourceVariablesDataSources, 'defaultDataType' | 'formDataTypes' | 'formDataSelector' >;
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Outdated
Show resolved
Hide resolved
src/App/frontend/src/features/language/textResources/TextResourcesProvider.tsx
Show resolved
Hide resolved
src/App/frontend/src/features/language/useLangToolsDataSources.tsx
Outdated
Show resolved
Hide resolved
|
🔍 Lighthouse Report |
|
🔍 Lighthouse Report |
cammiida
force-pushed
the
refactor/textresources-in-globaldata
branch
from
1bea713 to
7391537
Compare
|
🔍 Lighthouse Report |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/App/backend/src/Altinn.App.Api/Controllers/HomeController.cs`:
- Around line 56-58: The XML doc for the HomeController constructor is missing a
<param> entry for the ILogger<HomeController> parameter named logger; update the
XML documentation block above the HomeController constructor to add a <param
name="logger">...</param> describing the logger parameter (place it alongside
the existing <param> for appMetadata) so static analysis no longer flags the
missing tag.
- Line 34: Remove the unused private readonly ILogger<HomeController> _logger
field from the HomeController class and its assignment in the HomeController
constructor (where _logger is currently set) to eliminate dead code;
alternatively, if logging is intended, use the _logger instance in relevant
methods (e.g., inside HomeController actions) for logging calls—but since the
reviewer asked to remove it, delete the _logger field and its constructor
parameter/assignment references (symbols: _logger, ILogger<HomeController>,
HomeController constructor) to keep the class clean.
In
`@src/App/frontend/src/features/language/textResources/TextResourcesProvider.tsx`:
- Around line 48-57: The warning is fired any time query.data is falsy
(including during loading); update useTextResources to only call
window.logWarnOnce after the query has finished (e.g., check query.isSuccess or
query.isFetched) so you warn only when the fetch completed but returned no
data—locate the useTextResources function and the useTextResourcesQuery call and
move the logWarnOnce behind a conditional that verifies the query finished and
still has no data.
In
`@src/App/frontend/src/layout/RepeatingGroup/Container/RepeatingGroupContainer.test.tsx`:
- Around line 24-33: Replace the manual jest.mock of useTextResources with
seeding the React Query cache: in the test that calls
renderWithInstanceAndLayout (which wraps AppQueriesProvider), obtain the
QueryClient used by that provider (or create one) and call
queryClient.setQueryData(['fetchTextResources', language], textResourceMap) with
the same map currently returned by the mock; remove the jest.mock block
referencing useTextResources and ensure the test imports/uses the same language
key as the query key so useTextResources (which calls
useQuery(['fetchTextResources', language])) reads the seeded data.
🧹 Nitpick comments (4)
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs (1)
102-102: Avoid string interpolation in logging calls.Using
$"languageCookieValue: {languageCookieValue}"bypasses structured logging benefits and incurs string allocation even when the log level is disabled. Use structured logging parameters instead.Suggested fix
- _logger.LogDebug($"languageCookieValue: {languageCookieValue}"); + _logger.LogDebug("languageCookieValue: {LanguageCookieValue}", languageCookieValue);src/App/backend/test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json (1)
2529-2537: Consider aligninglangwith existinglanguagequery parameters (or document the difference).Most other endpoints expose
language, while these Home endpoints addlang. If this isn’t intentionally different, a consistent name helps codegen clients and avoids confusion; otherwise, a brief note explaining the divergence would help API consumers.Also applies to: 2586-2594, 2643-2651, 2701-2708, 2766-2773, 2831-2838, 2904-2911
src/App/frontend/src/features/validation/ValidationPlugin.test.tsx (1)
30-33: Consider moving mock insidebeforeEachfor consistency.The mock is currently at the describe block level, which executes once when the module loads. While this works for the current tests (all using the same text resources), placing the mock inside a
beforeEachor therenderfunction would be more consistent with the pattern used in other test files in this PR and would make it easier to override text resources for specific tests in the future.♻️ Suggested refactor
describe('ValidationPlugin', () => { - jest.mocked(useTextResources).mockImplementation(() => ({ - Form: { value: 'This is a page title' }, - NextPage: { value: 'This is the next page title' }, - })); - + beforeEach(() => { + jest.mocked(useTextResources).mockImplementation(() => ({ + Form: { value: 'This is a page title' }, + NextPage: { value: 'This is the next page title' }, + })); + }); + describe('validation visibility', () => {src/App/frontend/src/__mocks__/getTextResourcesMock.ts (1)
1-35: Avoid duplicate mock data by deriving the map from the array.Having two separate mock datasets risks drift. Consider building the map from
getTextResourcesMock()so the list and map stay consistent.♻️ Suggested refactor
export function getTextResourceMapMock(): TextResourceMap { - return { - 'option.from.rep.group.label': { - value: 'The value from the group is: {0}', - variables: [ - { - dataSource: 'dataModel.default', - key: 'someGroup[{0}].labelField', - }, - ], - }, - 'option.from.rep.group.description': { - value: 'Description: The value from the group is: {0}', - variables: [ - { - dataSource: 'dataModel.default', - key: 'someGroup[{0}].labelField', - }, - ], - }, - 'option.from.rep.group.helpText': { - value: 'Help Text: The value from the group is: {0}', - variables: [ - { - dataSource: 'dataModel.default', - key: 'someGroup[{0}].labelField', - }, - ], - }, - 'accordion.title': { value: 'This is a title' }, - FormLayout: { value: 'This is a page title' }, - }; + return getTextResourcesMock().reduce<TextResourceMap>((acc, { id, ...resource }) => { + acc[id] = resource; + return acc; + }, {}); }
src/App/frontend/src/features/language/textResources/TextResourcesProvider.tsx
Show resolved
Hide resolved
src/App/frontend/src/layout/RepeatingGroup/Container/RepeatingGroupContainer.test.tsx
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Fixed
Show fixed
Hide fixed
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Fixed
Show fixed
Hide fixed
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Fixed
Show fixed
Hide fixed
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Fixed
Show fixed
Hide fixed
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Fixed
Show fixed
Hide fixed
|
🔍 Lighthouse Report |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/App/backend/src/Altinn.App.Api/Controllers/HomeController.cs (1)
88-145: Preservelangwhen redirecting to authentication. With the newlangquery, unauthenticated users will lose the selected language because the redirect URL drops it. Consider forwardinglangingoToUrl.Proposed fix
- string goToUrl = HttpUtility.UrlEncode($"{scheme}://{Request.Host}/{org}/{app}"); + string goToUrl = $"{scheme}://{Request.Host}/{org}/{app}"; + if (!string.IsNullOrEmpty(lang)) + { + goToUrl = $"{goToUrl}?lang={lang}"; + } + goToUrl = HttpUtility.UrlEncode(goToUrl);
🧹 Nitpick comments (3)
src/App/frontend/src/features/language/textResources/TextResourcesProvider.tsx (1)
21-41: Centralise React Query keys/config with object-based keys +queryOptions.
This keeps keys consistent and makes reuse across hooks/tests easier.Suggested refactor
-import { useQuery } from '@tanstack/react-query'; +import { queryOptions, useQuery } from '@tanstack/react-query'; +const textResourceQueryKeys = { + byLanguage: (language: string) => ['textResources', language] as const, +}; + +const textResourcesQueryOptions = ( + selectedLanguage: string, + textResourcesFromWindow: ITextResourceResult | undefined, +) => + queryOptions<TextResourceMap, HttpClientError>({ + queryKey: textResourceQueryKeys.byLanguage(selectedLanguage), + queryFn: async () => { + const textResourceResult = + textResourcesFromWindow && textResourcesFromWindow.language === selectedLanguage + ? textResourcesFromWindow + : await fetchTextResources(selectedLanguage); + return toTextResourceMap(textResourceResult); + }, + }); -function getTextResourceQueryKey(selectedLanguage: string) { - return ['fetchTextResources', selectedLanguage] as const; -} - - const query = useQuery<TextResourceMap, HttpClientError>({ - queryKey: getTextResourceQueryKey(selectedLanguage), - queryFn: async () => { - const textResourceResult = - textResourcesFromWindow && textResourcesFromWindow.language === selectedLanguage - ? textResourcesFromWindow - : await fetchTextResources(selectedLanguage); - - return toTextResourceMap(textResourceResult); - }, - }); + const query = useQuery(textResourcesQueryOptions(selectedLanguage, textResourcesFromWindow));As per coding guidelines, “Use objects for managing TanStack Query keys and functions; use
queryOptionsfor sharing query configuration centrally.”src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs (2)
148-158: Consider using Debug level for normal flow logging.Lines 148 and 155-158 use
LogInformationfor cases that are part of normal operation (missing cookie or successful deserialization). However, theGetTextResourcesmethod usesLogDebugfor similar decision-point logging (lines 108, 118, 125, 129). This inconsistency may result in noisy logs in production.🔧 Suggested change to align logging levels
if (!_httpContextAccessor.HttpContext.Request.Cookies.TryGetValue(cookieKey, out var languageCookie)) { - _logger.LogInformation("No language cookie found for cookieKey {CookieKey}", cookieKey); + _logger.LogDebug("No language cookie found for cookieKey {CookieKey}", cookieKey); return null; } try { var languageCookieValue = JsonSerializer.Deserialize<string>(languageCookie); - _logger.LogInformation( + _logger.LogDebug( "Successfully deserialized language cookie value for cookieKey {CookieKey}", cookieKey ); return languageCookieValue; }
163-163: Minor grammatical issue in log message."failed deserialize" should be "failed to deserialize".
✏️ Suggested fix
- _logger.LogWarning(ex, "Language cookie with key {CookieKey} found, but failed deserialize it.", cookieKey); + _logger.LogWarning(ex, "Language cookie with key {CookieKey} found, but failed to deserialize it.", cookieKey);
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Dismissed
Show dismissed
Hide dismissed
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Dismissed
Show dismissed
Hide dismissed
src/App/backend/src/Altinn.App.Core/Features/Bootstrap/BootstrapGlobalService.cs
Dismissed
Show dismissed
Hide dismissed
|
🔍 Lighthouse Report |
1 participant
Description
Verification
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.