[NAE-2188] Wrong remote configuration loading order - #297
Conversation
- Refactored configuration service to handle remote properties loading. - Introduced `loadRemoteConfiguration` function to fetch configuration data from a remote source. - Updated example app to initialize environment with remote configuration. - Added `loaded_properties` field to environment configuration files. - Minor formatting adjustments for consistency.
- Removed the `authenticationServiceFactory` method and corresponding tests as it is no longer in use. - Eliminated support for `loaded_properties` and remote configuration loading from environment configurations to streamline setup. - Updated the `AbstractResourceService` to delay initialization of `_SERVER_URL` until first access for better resource usage. - Introduced observables to track configuration loading states in `ConfigurationService`. - Simplified the configuration initialization logic and added a snapshot getter for safe configuration access in `ConfigurationService`. - Updated `UserService` and `AuthenticationService` to wait for configuration to load before performing operations. - Replaced `mongoId` with `id.keyword` in `CaseMetaField` to align with updated backend identifiers. - Removed deprecated documentation references to authenticationServiceFactory.
WalkthroughReplaces the authentication factory with a ProxyAuthenticationService and updates the module/provider and public exports; introduces reactive configuration streams (config$, loaded$) and defers initialization in multiple services until configuration is loaded; adds pagination mapping, alters an enum member value, removes the old factory and its test, adds new tests, and refreshes documentation references. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Init
participant Config as ConfigurationService
participant AuthMod as AuthenticationModule
participant Proxy as ProxyAuthenticationService
participant Basic as BasicAuthenticationService
participant BasicRealm as BasicWithRealmAuthenticationService
participant NullAuth as NullAuthenticationService
App->>Config: loadConfiguration()
activate Config
Config-->>Config: initialize() and emit config$ / loaded$
Config-->>App: loaded$ = true
deactivate Config
AuthMod->>Proxy: instantiate (inject HttpClient, Config)
Proxy->>Config: subscribe loaded$ (once)
Note right of Proxy: On first loaded=true
Config-->>Proxy: snapshot.providers.auth.authentication
alt "basic"
Proxy->>Basic: select strategy
else "basicwithrealm"
Proxy->>BasicRealm: select strategy
else other/absent
Proxy->>NullAuth: select strategy
end
App->>Proxy: login(credentials)
Proxy->>Proxy: delegate to selected strategy
Proxy-->>App: Observable<UserResource>
sequenceDiagram
autonumber
participant Config as ConfigurationService
participant AuthSvc as AuthenticationService
participant Session as SessionService
participant Idle as SessionIdleTimerService
participant UserSvc as UserService
Note over AuthSvc,UserSvc: Services defer init until configuration is loaded
Config-->>AuthSvc: loaded$ = true
AuthSvc-->>AuthSvc: init _authenticated$, subscribe to session$
Config-->>Session: loaded$ = true
Session-->>Session: resolve storage & header, initial load
Config-->>Idle: loaded$ = true
Idle-->>Idle: read timeout, enable service, start timer
Config-->>UserSvc: loaded$ = true
UserSvc-->>UserSvc: subscribe to auth/anonymous streams
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labelsbugfix, refactor, large ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
docs/compodoc/components-core/coverage.html (1)
748-755: Enforce kebab-case naming for ProxyAuthenticationService and remove outdated coverage entry
- Rename projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts to proxy-authentication.service.ts and update all imports (in authentication.module.ts and the spec) to use the new path.
- docs/compodoc/components-core/coverage.html still lists a non-existent function authenticationServiceFactory for this file; regenerate the docs (or remove that entry) so coverage reflects the actual code.
docs/compodoc/components-core/miscellaneous/functions.html (2)
3228-3230: Broken Compodoc rendering (“[object Object]”) in deprecated section.The page renders raw objects instead of readable text. Regenerate docs or adjust JSDoc for those entries.
4032-4032: Typo in generated link path (“classess” → “classes”).This breaks the hyperlink; please fix the generator template or JSDoc tags.
projects/netgrif-components-core/src/lib/authentication/services/authentication/authentication.service.ts (1)
39-49: Optional: surface login failures distinctly.Returning
of(null)masks failure causes; consider rethrowing or mapping to a typed error to inform the UI.projects/netgrif-components-core/src/lib/authentication/session/services/session-idle-timer.service.ts (1)
35-49: Consider starting the timer after config loads when enabled.If callers invoke
startTimer()beforeloaded$, nothing happens. Option: auto-start on firstloaded$when_enableServiceis true, or queue a pending start.projects/netgrif-components-core/src/lib/navigation/navigation-tree/abstract-navigation-tree.component.spec.ts (1)
34-34: Entire suite is skipped viaxdescribeIf unintended, switch to
describeto re-enable coverage for the navigation tree tests.projects/netgrif-components-core/src/lib/resources/abstract-endpoint/abstract-resource.service.ts (1)
62-76:getResourcePagereturns raw response on falsy input and fragile pagination check
- Returning
responsewhen falsy violates thePage<T>contract.- Checking
hasOwnProperty(PaginationParams.PAGE_NUMBER)to decide onresponse.pageis likely incorrect and brittle.Apply this diff to:
- Return an empty
Page<T>on falsy input.- Prefer
mapToPagewhen backend sends SpringPageImpl(contentpresent).- Fall back to
_embeddedextraction with robustpagedetection.- protected getResourcePage<T>(response: any, propertiesParams: string): Page<T> { - if (!response) { - return response; - } - const defaultPage: Pagination = { - number: -1, - size: 0, - totalPages: 0, - totalElements: 0 - }; - - return { - content: this.changeType<Array<T>>(response, propertiesParams), - pagination: response.hasOwnProperty(PaginationParams.PAGE_NUMBER) ? response.page : defaultPage - }; - } + protected getResourcePage<T>(response: any, propertiesParams: string): Page<T> { + const defaultPage: Pagination = { + number: -1, + size: 0, + totalPages: 0, + totalElements: 0 + }; + + if (!response) { + return { content: [], pagination: defaultPage }; + } + + // Spring PageImpl style + if ('content' in response) { + return this.mapToPage<T>(response); + } + + // HAL style with _embedded + optional page + return { + content: this.changeType<Array<T>>(response, propertiesParams), + pagination: response?.page && typeof response.page === 'object' ? response.page : defaultPage + }; + }projects/netgrif-components-core/src/lib/user/services/user.service.ts (2)
26-27: Uninitialized subscriptions can cause ngOnDestroy crashWith gated initialization, subs may be undefined if config never loads.
Apply:
- protected _subAuth: Subscription; - protected _subAnonym: Subscription; + protected _subAuth: Subscription = Subscription.EMPTY; + protected _subAnonym: Subscription = Subscription.EMPTY; @@ - this._subAuth.unsubscribe(); - this._subAnonym.unsubscribe(); + this._subAuth?.unsubscribe(); + this._subAnonym?.unsubscribe();Also applies to: 85-90
77-79: anonymousUser getter is recursively calling itselfThis is a hard runtime error on access.
Apply:
- get anonymousUser(): User { - return this.anonymousUser; - } + get anonymousUser(): User { + return this._user; + }projects/netgrif-components-core/src/lib/configuration/configuration.service.ts (2)
11-11: Make _dataFieldConfiguration optional to reflect new lazy initializationinitialize() may not run immediately (or at all on error), so this field can be undefined. Align the type with runtime reality.
- private _dataFieldConfiguration: Services['dataFields']; + private _dataFieldConfiguration?: Services['dataFields'];
274-283: JSDoc is stale vs new return type and behaviorDocs still mention returning null/any and do not describe the fallback behavior. Update to match Observable and the initialization semantics.
- /** - * Loads and initializes application configuration from the backend. - * If configuration resolution is disabled in APPLICATION_CONFIG, returns null Observable. - * Otherwise fetches public configuration via ConfigurationResourceService. - * - * @returns Observable<any> that emits null if resolution is disabled, otherwise emits the loaded configuration - * @fires initialize() Upon successful configuration load to setup endpoints and data field configurations - * @see ApplicationConfiguration - * @see NetgrifApplicationEngine - */ + /** + * Loads and initializes application configuration from the backend. + * If resolution is disabled, completes immediately. + * When resolution is enabled, fetches public configuration via ConfigurationResourceService. + * On 404 or errors, falls back to the locally provided configuration and still initializes. + * + * @returns Observable<void> that completes when initialization has been attempted + * @fires initialize() after loading remote config or falling back to local config + * @see ApplicationConfiguration + * @see NetgrifApplicationEngine + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
docs/compodoc/components-core/coverage.html(1 hunks)docs/compodoc/components-core/miscellaneous/functions.html(94 hunks)docs/compodoc/components-core/unit-test.html(1 hunks)docs/typedoc/components-core/functions/authenticationServiceFactory.html(1 hunks)projects/nae-example-app/src/app/nae-example-app-configuration.service.ts(1 hunks)projects/nae-example-app/src/environments/environment.prod.ts(1 hunks)projects/nae-example-app/src/environments/environment.ts(1 hunks)projects/nae-example-app/src/main.ts(1 hunks)projects/netgrif-components-core/src/lib/authentication/authentication.factory.spec.ts(0 hunks)projects/netgrif-components-core/src/lib/authentication/authentication.factory.ts(0 hunks)projects/netgrif-components-core/src/lib/authentication/authentication.module.ts(2 hunks)projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.spec.ts(1 hunks)projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts(1 hunks)projects/netgrif-components-core/src/lib/authentication/public-api.ts(1 hunks)projects/netgrif-components-core/src/lib/authentication/services/authentication/authentication.service.ts(2 hunks)projects/netgrif-components-core/src/lib/authentication/session/services/session-idle-timer.service.ts(3 hunks)projects/netgrif-components-core/src/lib/authentication/session/services/session.service.ts(9 hunks)projects/netgrif-components-core/src/lib/configuration/configuration.service.ts(4 hunks)projects/netgrif-components-core/src/lib/header/case-header/case-menta-enum.ts(1 hunks)projects/netgrif-components-core/src/lib/navigation/navigation-tree/abstract-navigation-tree.component.spec.ts(1 hunks)projects/netgrif-components-core/src/lib/resources/abstract-endpoint/abstract-resource.service.ts(3 hunks)projects/netgrif-components-core/src/lib/user/services/user.service.ts(3 hunks)
💤 Files with no reviewable changes (2)
- projects/netgrif-components-core/src/lib/authentication/authentication.factory.ts
- projects/netgrif-components-core/src/lib/authentication/authentication.factory.spec.ts
🧰 Additional context used
🧬 Code graph analysis (3)
projects/nae-example-app/src/app/nae-example-app-configuration.service.ts (2)
projects/nae-example-app/src/environments/environment.prod.ts (1)
environment(1-7)projects/nae-example-app/src/environments/environment.ts (1)
environment(5-11)
projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts (3)
projects/netgrif-components-core/src/lib/configuration/configuration.service.ts (1)
snapshot(28-30)projects/netgrif-components-core/src/lib/authentication/models/credentials.ts (1)
Credentials(1-5)projects/netgrif-components-core/src/lib/resources/interface/user-resource.ts (1)
UserResource(10-18)
projects/netgrif-components-core/src/lib/configuration/configuration.service.ts (2)
projects/netgrif-components-core/src/commons/schema.ts (1)
NetgrifApplicationEngine(18-27)projects/netgrif-components-core/src/lib/configuration/application-configuration.ts (1)
ApplicationConfiguration(27-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (24)
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (22)
🔇 Additional comments (20)
projects/netgrif-components-core/src/lib/header/case-header/case-menta-enum.ts (1)
2-2: Confirm ES mapping for 'id' before using 'id.keyword'.
- MONGO_ID = 'id.keyword' is defined in projects/netgrif-components-core/src/lib/header/case-header/case-menta-enum.ts and CaseMetaField.MONGO_ID is referenced across the codebase (e.g. projects/netgrif-components-core/src/lib/header/case-header/case-header.service.ts:41). No literal "mongoId" strings found.
- Action: Verify your Elasticsearch index mappings in every environment — if the mapping defines id as keyword (no .keyword subfield) or id.keyword may be missing, set MONGO_ID = 'id' or add the .keyword subfield/alias; otherwise keep 'id.keyword'.
projects/nae-example-app/src/environments/environment.prod.ts (1)
3-6: Based on the scan, only environment.prod.ts uses unsafe property access; the asset scripts first define window.env and then assign to its properties, so they’re safe as-is.Fix unsafe access in environment.prod.ts
Use optional chaining and nullish coalescing:projects/nae-example-app/src/environments/environment.prod.ts @@ - resolve_configuration: window['env']['resolve_configuration'] || false, - gateway_url: window['env']['gateway_url'] || 'http://localhost:8800/api', - application_identifier: window['env']['application_identifier'] || 'nae', - type_identifier: window['env']['type_identifier'] || 'default', + resolve_configuration: window['env']?.['resolve_configuration'] ?? false, + gateway_url: window['env']?.['gateway_url'] ?? 'http://localhost:8800/api', + application_identifier: window['env']?.['application_identifier'] ?? 'nae', + type_identifier: window['env']?.['type_identifier'] ?? 'default',projects/nae-example-app/src/environments/environment.ts (1)
7-10: LGTM — safer optional chaining for runtime config.projects/nae-example-app/src/app/nae-example-app-configuration.service.ts (1)
15-16: LGTM — trailing comma only; no behavioral change.projects/nae-example-app/src/main.ts (1)
9-13: LGTM — whitespace-only change; behavior unchanged.docs/typedoc/components-core/functions/authenticationServiceFactory.html (3)
1-1: Regenerate docs to avoid manual edits and ensure source links use the correct revisionIf TypeDoc/Compodoc generated this, fix it at the source (generator config or reflection metadata) and regenerate rather than committing a hand-edited HTML. Also ensure the GitHub source-link uses the current PR head commit, not a stale SHA.
Would you like me to outline a docs regeneration checklist tailored to your repo scripts to keep these links consistent across pages?
1-1: Confirm the API surface: should this page still be “authenticationServiceFactory”?Given the shift to
ProxyAuthenticationService, verify whether the factory function remains part of the public API. If it was removed or is now an internal alias, this page should be removed/renamed to avoid API drift in published docs.
1-1: Broken "Defined in" link: href points to old file while label shows new fileThe anchor’s href still targets
authentication.factory.ts, but the visible text saysproxyAuthentication.service.ts:6. This mismatch will mislead users and likely 404.Apply this minimal fix:
-<li>Defined in <a href="https://github.com/netgrif/components/blob/fe552ef0da4b26b132db4ed525905b543313b49b/projects/netgrif-components-core/src/lib/authentication/authentication.factory.ts#L6">projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts:6</a></li> +<li>Defined in <a href="https://github.com/netgrif/components/blob/fe552ef0da4b26b132db4ed525905b543313b49b/projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts#L6">projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts:6</a></li>Likely an incorrect or invalid review comment.
projects/netgrif-components-core/src/lib/authentication/authentication.module.ts (2)
3-9: Imports updated: verify ProxyAuthenticationService path and DI deps
- Path casing matters on some filesystems. Confirm file name is
proxyAuthentication.service.tsand the import path./proxyAuthentication.servicematches exactly.- Ensure
ProxyAuthenticationService’s constructor deps are provided in this module/app (e.g.,ConfigurationService,HttpClient) souseClasscan resolve them without the old factory.
24-27: Provider switch touseClasslooks good; confirm interceptor order is intentional
- Mapping
AuthenticationMethodService → ProxyAuthenticationServiceviauseClassis correct.- Double-check interceptor order: Angular applies request interceptors in the order provided (Line 24 before 25). Verify this ordering achieves the intended auth/anonymous behavior.
projects/netgrif-components-core/src/lib/navigation/navigation-tree/abstract-navigation-tree.component.spec.ts (1)
399-402: Constructor update wired correctly; ensure config “loaded” semantics in testsPassing
config: ConfigurationServicetosuper(...)aligns with the new signature. VerifyTestConfigurationService(used viaConfigurableTestConfigurationService) emits/flags configuration as loaded so services that gate onloaded$don’t hang in tests.projects/netgrif-components-core/src/lib/resources/abstract-endpoint/abstract-resource.service.ts (1)
102-125:mapToPagehelper is solid; ensure it’s actually usedAfter the
getResourcePagechange above, this method becomes utilized forcontent-style responses. If you choose not to call it, remove it to avoid dead code.projects/netgrif-components-core/src/lib/authentication/session/services/session.service.ts (2)
96-105: Idle timer lifecycle improvements look goodReset/stop calls are correctly placed around verification and clearing.
Also applies to: 136-146, 159-160
71-74: sessionHeader usage is safe: the getter always calls ensureConfigInitialized() before returning, and a repo‐wide search found only internal references.projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts (1)
20-26: Config gating looks correctOne-shot subscription with filter+take(1) is appropriate.
projects/netgrif-components-core/src/lib/configuration/configuration.service.ts (5)
2-2: RxJS core imports added — OKThe added imports are correct and minimal.
5-5: RxJS operators imports — OKAll imported operators are used and appropriate.
15-18: Reactive config API looks goodBehaviorSubject + derived config$/loaded$ shape is sound.
35-35: Emit config after initialization — OKPushing the deep copy via _config$ ensures subscribers are notified.
129-131: getConfigurationSubtreeByPath() — OK
- Removed unused `Injector` and `HttpBackend` imports from `proxyAuthentication.service.ts` to clean up dependencies. - Ensured the implementation aligns with updated configuration and authentication service standards. - Minor adjustments to improve module code readability and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts (2)
1-12: Don’t instantiate Angular services withnew; resolve viaInjector.Direct
newbypasses DI and any transitive deps/interceptors. UseInjectorto obtain implementations. Also drop the now-unneededHttpClientimport/param.-import {HttpClient} from '@angular/common/http'; +// HttpClient import not needed here when resolving concrete services via Injector @@ -import {Injectable} from "@angular/core"; +import {Injectable, Injector} from "@angular/core"; @@ - private _proxyAuthMethod: AuthenticationMethodService = new NullAuthenticationService(); + private _proxyAuthMethod: AuthenticationMethodService = new NullAuthenticationService(); @@ - constructor(private _config: ConfigurationService, private _http: HttpClient) { + constructor(private _config: ConfigurationService, private _injector: Injector) { super(); @@ - if (!auth || !auth.authentication) { - this._proxyAuthMethod = new NullAuthenticationService(); + if (!auth || !auth.authentication) { + this._proxyAuthMethod = this._injector.get(NullAuthenticationService); return; } @@ - if (authType === 'basic') { - this._proxyAuthMethod = new BasicAuthenticationService(this._http, this._config); - } else if (authType === 'basicwithrealm') { - this._proxyAuthMethod = new BasicWithRealmAuthenticationService(this._http, this._config); + if (authType === 'basic') { + this._proxyAuthMethod = this._injector.get(BasicAuthenticationService); + } else if (authType === 'basicwithrealm') { + this._proxyAuthMethod = this._injector.get(BasicWithRealmAuthenticationService); } else { - this._proxyAuthMethod = new NullAuthenticationService(); + this._proxyAuthMethod = this._injector.get(NullAuthenticationService); }To confirm this change is safe, verify that
BasicAuthenticationService,BasicWithRealmAuthenticationService, andNullAuthenticationServiceare@Injectableand provided (ideallyprovidedIn: 'root'):#!/bin/bash fd -a 'basic-authentication.service.ts' | xargs -I{} rg -n '^@Injectable' {} fd -a 'basic-with-realm-authentication.service.ts' | xargs -I{} rg -n '^@Injectable' {} fd -a 'null-authentication.service.ts' | xargs -I{} rg -n '^@Injectable' {}Also applies to: 16-41
6-6: Fix race: defer login/logout until configuration has loaded.Avoid delegating to
NullAuthenticationServicebefore config resolves.-import {filter, take} from "rxjs/operators"; +import {filter, take, switchMap} from "rxjs/operators"; @@ - login(credentials: Credentials): Observable<UserResource> { - return this._proxyAuthMethod.login(credentials); - } + login(credentials: Credentials): Observable<UserResource> { + return this._config.loaded$.pipe( + filter(Boolean), + take(1), + switchMap(() => this._proxyAuthMethod.login(credentials)) + ); + } @@ - logout(): Observable<object> { - return this._proxyAuthMethod.logout(); - } + logout(): Observable<object> { + return this._config.loaded$.pipe( + filter(Boolean), + take(1), + switchMap(() => this._proxyAuthMethod.logout()) + ); + }Also applies to: 45-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts (4)
projects/netgrif-components-core/src/lib/authentication/services/authentication/authentication.service.ts (1)
Injectable(12-77)projects/netgrif-components-core/src/lib/configuration/configuration.service.ts (1)
snapshot(28-30)projects/netgrif-components-core/src/lib/authentication/models/credentials.ts (1)
Credentials(1-5)projects/netgrif-components-core/src/lib/resources/interface/user-resource.ts (1)
UserResource(10-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (22)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (1)
projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts (1)
13-14: Provider binding check.Ensure the module/provider binds
AuthenticationMethodServicetoProxyAuthenticationService(e.g.,{ provide: AuthenticationMethodService, useExisting: ProxyAuthenticationService }), aligning with the factory removal.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
projects/netgrif-components-core/src/lib/translate/language.service.ts (4)
67-75: SSR-safe localStorage read and undefined browserLang guardAccessing localStorage during SSR or in restricted environments can throw; getBrowserLang() may be undefined. Guard both.
- protected checkLocalStorage() { - const lang = localStorage.getItem('Language'); - if (lang === null) { - const browserLang = this._translate.getBrowserLang(); - this.setLanguage(browserLang); - } else { - this.setLanguage(lang); - } - } + protected checkLocalStorage() { + const lang = this.safeGetLocalStorageItem('Language'); + if (lang === null) { + const browserLang = this._translate.getBrowserLang() || this._defaultLanguage; + this.setLanguage(browserLang); + } else { + this.setLanguage(lang); + } + }Add these helpers and browser flag (outside the shown range):
// ctor param: inject platformId constructor( protected _translate: TranslateService, protected _preferenceService: UserPreferenceService, protected _logger: LoggerService, @Inject(PLATFORM_ID) private platformId: Object ) { // ... } private get isBrowser(): boolean { return isPlatformBrowser(this.platformId); } private safeGetLocalStorageItem(key: string): string | null { if (!this.isBrowser) { return null; } try { return localStorage.getItem(key); } catch { return null; } } private safeSetLocalStorageItem(key: string, value: string): void { if (!this.isBrowser) { return; } try { localStorage.setItem(key, value); } catch { /* no-op */ } }
85-92: Save the resolved language consistently; de-duplicate checks and use SSR-safe storageCurrently setLocale(lang) may persist an unsupported lang while use()/localStorage store the fallback. Compute once and reuse.
- public setLanguage(lang: string, saveToPreferences = false) { - this._translate.use( this.checkIfLangExists(lang) ? lang : this._defaultLanguage); - if (saveToPreferences) { - this._preferenceService.setLocale(lang); - } - localStorage.setItem('Language', this.checkIfLangExists(lang) ? lang : this._defaultLanguage); - this._langChange$.next(this.checkIfLangExists(lang) ? lang : this._defaultLanguage); - } + public setLanguage(lang: string, saveToPreferences = false) { + const safeLang = this.checkIfLangExists(lang) ? lang : this._defaultLanguage; + this._translate.use(safeLang); + if (saveToPreferences) { + this._preferenceService.setLocale(safeLang); + } + this.safeSetLocalStorageItem('Language', safeLang); + this._langChange$.next(safeLang); + }
55-57: Use correct event type for onLangChangeonLangChange emits LangChangeEvent, not TranslationChangeEvent. Types are structurally similar, but this improves accuracy.
- this.subTranslate = _translate.onLangChange.subscribe((event: TranslationChangeEvent) => { + this.subTranslate = _translate.onLangChange.subscribe((event: LangChangeEvent) => { this._logger.debug('Language changed to ' + event.lang); });Also add the import:
-import {TranslateService, TranslationChangeEvent} from '@ngx-translate/core'; +import {TranslateService, TranslationChangeEvent, LangChangeEvent} from '@ngx-translate/core';
94-98: Tighten type of translation parameterPrefer a safer structural type over Object.
- public addLanguage(lang: string, translation: Object) { + public addLanguage(lang: string, translation: Record<string, unknown>) {projects/netgrif-components-core/src/lib/user/services/user.service.ts (2)
78-80: Fix infinite recursion in anonymousUser getterReturns itself; stack overflows at runtime.
get anonymousUser(): User { - return this.anonymousUser; + return this._user; }
89-91: Guard unsubscribes in ngOnDestroyIf config never loads, subscriptions may be undefined.
- this._subAuth.unsubscribe(); - this._subAnonym.unsubscribe(); + this._subAuth?.unsubscribe(); + this._subAnonym?.unsubscribe();
♻️ Duplicate comments (6)
projects/netgrif-components-core/src/lib/authentication/session/services/session.service.ts (4)
112-121: Guard verify() against missing/late configuration and log early-call warningAvoids crashes on cfg access when called before config load; keeps behavior predictable.
public verify(token?: string): Observable<boolean> { this.ensureConfigInitialized(); this._verifying.on(); token = !!token ? token : this.sessionToken; - const authConfig = this._config.get().providers.auth; + const cfg = this._config.get(); + if (!cfg || !cfg.providers || !cfg.providers.auth) { + this._log.warn('SessionService.verify called before configuration load; using defaults.'); + this._verifying.off(); + this._initialized.on(); + return throwError(new Error('Cannot verify session token. Configuration not loaded.')); + } + const authConfig = cfg.providers.auth; let url = authConfig.address; url += authConfig.endpoints && authConfig.endpoints['verification'] ? authConfig.endpoints['verification'] : (authConfig.endpoints && authConfig.endpoints['login'] ? authConfig.endpoints['login'] : '');
44-50: Remove duplicate initialization; rely on ensureConfigInitialized()Avoid re-reading config here; keep a single init path.
.subscribe(() => { - this._storage = this.resolveStorage(this._config.get().providers.auth['sessionStore']); - this._sessionHeader = this._config.get().providers.auth.sessionBearer ? - this._config.get().providers.auth.sessionBearer : SessionService.SESSION_BEARER_HEADER_DEFAULT; - this.ensureConfigInitialized(); - this.load(); + this.ensureConfigInitialized(); + this.load(); });
73-76: Don’t force-unwrap header; return a safe defaultPrevents NPEs when called before config is available.
get sessionHeader(): string { this.ensureConfigInitialized(); - return this._sessionHeader!; + return this._sessionHeader ?? SessionService.SESSION_BEARER_HEADER_DEFAULT; }
174-184: Harden ensureConfigInitialized() for absent config; set safe defaultsPrevents runtime errors when cfg not yet available; keeps method idempotent.
private ensureConfigInitialized(): void { if (this._sessionHeader && !(this._storage instanceof NullStorage)) { return; } - const cfg = this._config.get(); - const sessionStore = cfg.providers.auth['sessionStore']; - this._storage = this.resolveStorage(sessionStore); - this._sessionHeader = cfg.providers.auth.sessionBearer - ? cfg.providers.auth.sessionBearer - : SessionService.SESSION_BEARER_HEADER_DEFAULT; + const cfg = this._config.get(); + if (!cfg || !cfg.providers || !cfg.providers.auth) { + this._storage = new NullStorage(); + this._sessionHeader = SessionService.SESSION_BEARER_HEADER_DEFAULT; + return; + } + const sessionStore = cfg.providers.auth['sessionStore']; + this._storage = this.resolveStorage(sessionStore); + this._sessionHeader = cfg.providers.auth.sessionBearer + ? cfg.providers.auth.sessionBearer + : SessionService.SESSION_BEARER_HEADER_DEFAULT; }projects/netgrif-components-core/src/lib/user/services/user.service.ts (2)
2-2: Verify DI wiring for ConfigurationService in app and testsEnsure a provider is available wherever UserService is constructed (or rely on providedIn root if that’s the case). Update TestBed modules as needed.
#!/bin/bash # Check for explicit providers for ConfigurationService in app/tests rg -nP -C2 'provide\(\s*ConfigurationService' --type=ts rg -nP -C2 'TestBed\.configureTestingModule\(' --type=ts | sed -n '1,200p'Also applies to: 39-39
44-67: Align subscriptions; remove setTimeout to avoid race/order issuesRegister both subscriptions in the same microtask after config loads.
this._config.loaded$ .pipe( filter(loaded => loaded), take(1) ).subscribe(() => { - setTimeout(() => { - this._subAuth = this._authService.authenticated$.subscribe(auth => { - if (auth && !this._loginCalled) { - this.loadUser(); - } else if (!auth) { - this.clearUser(); - this.publishUserChange(); - } - }); - }); - this._subAnonym = this._anonymousService.tokenSet.subscribe(token => { - if (token) { - this.loadPublicUser(); - } else { - this.clearUser(); - this.publishAnonymousUserChange(); - } - }); + Promise.resolve().then(() => { + this._subAuth = this._authService.authenticated$.subscribe(auth => { + if (auth && !this._loginCalled) { + this.loadUser(); + } else if (!auth) { + this.clearUser(); + this.publishUserChange(); + } + }); + this._subAnonym = this._anonymousService.tokenSet.subscribe(token => { + if (token) { + this.loadPublicUser(); + } else { + this.clearUser(); + this.publishAnonymousUserChange(); + } + }); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
projects/netgrif-components-core/src/lib/authentication/session/services/session.service.ts(9 hunks)projects/netgrif-components-core/src/lib/data-fields/date-field/date-default-field/abstract-date-default-field.component.spec.ts(0 hunks)projects/netgrif-components-core/src/lib/translate/language.service.ts(1 hunks)projects/netgrif-components-core/src/lib/user/services/user.service.ts(3 hunks)
💤 Files with no reviewable changes (1)
- projects/netgrif-components-core/src/lib/data-fields/date-field/date-default-field/abstract-date-default-field.component.spec.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: machacjozef
PR: netgrif/components#297
File: projects/netgrif-components-core/src/lib/authentication/proxyAuthentication.service.ts:18-43
Timestamp: 2025-09-05T09:19:53.792Z
Learning: In ProxyAuthenticationService, manual instantiation of authentication services (BasicAuthenticationService, BasicWithRealmAuthenticationService) with new is intentional to avoid circular dependencies. Using Angular's Injector would trigger full DI resolution (HttpClient → interceptors → SessionService), breaking session initialization. The manual approach bypasses interceptors during login, keeping the session lifecycle intact.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: task-list-completed
- GitHub Check: Test with SonarCloud
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (22)
- GitHub Check: Matrix Test (24)
- GitHub Check: task-list-completed
🔇 Additional comments (1)
projects/netgrif-components-core/src/lib/authentication/session/services/session.service.ts (1)
98-101: LGTM: idle timer integration is consistentReset/stop placements align with verification and lifecycle events.
Also applies to: 105-107, 139-147, 156-162
3 participants
Description
Fixes for applying a remotely fetched configuration so that the dependent service waits for the load.
Fixes NAE-2188
Dependencies
No new dependencies were introduced>
How Has Been This Tested?
Manually
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores