Conversation
WalkthroughThe changes update the frontchannel logout and registration components. The frontchannel logout flow now waits for all logout client iframes to load before redirecting, using a reactive counter and effect. The registration component switches the reCAPTCHA callback from a dynamic Angular binding to a global function managed via Angular lifecycle hooks. Changes
Sequence Diagram(s)Frontchannel Logout Flow (After Changes)sequenceDiagram
participant User
participant FrontchannelLogoutComponent
participant LogoutClientIframe
participant Window
User->>FrontchannelLogoutComponent: Loads component
FrontchannelLogoutComponent->>LogoutClientIframe: Renders iframe(s) for each logout client
LogoutClientIframe-->>FrontchannelLogoutComponent: load event (onIframeLoaded)
FrontchannelLogoutComponent->>FrontchannelLogoutComponent: Increment iframeLoadCount
FrontchannelLogoutComponent->>FrontchannelLogoutComponent: Effect checks if all iframes loaded
alt All iframes loaded and logoutRedirectUri defined
FrontchannelLogoutComponent->>Window: Redirect to logoutRedirectUri
end
Register Component reCAPTCHA Callback (After Changes)sequenceDiagram
participant User
participant RegisterComponent
participant Window
participant reCAPTCHA
RegisterComponent->>Window: Defines window.onSubmitRecaptcha in ngOnInit
User->>reCAPTCHA: Completes challenge
reCAPTCHA->>Window: Calls window.onSubmitRecaptcha
Window->>RegisterComponent: window.onSubmitRecaptcha submits form
RegisterComponent->>Window: Removes window.onSubmitRecaptcha in ngOnDestroy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
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: 3
🔭 Outside diff range comments (1)
src/login/pages/register/register.component.html (1)
91-98: ReCAPTCHA callback hookup looks correct; add disabled state parity with the non-recaptcha submit.The static
data-callback="onSubmitRecaptcha"aligns with the global defined in the component. However, unlike the non-recaptcha submit (Lines 107–112), this button lacks the disabled binding, allowing clicks when the form shouldn’t be submittable.Apply:
<button class="g-recaptcha" type="submit" [kcClass]="['kcButtonClass', 'kcButtonPrimaryClass', 'kcButtonBlockClass', 'kcButtonLargeClass']" [attr.data-sitekey]="recaptchaSiteKey" - data-callback="onSubmitRecaptcha" + data-callback="onSubmitRecaptcha" + [disabled]="!isFormSubmittable() || (termsAcceptanceRequired && !areTermsAccepted())" [attr.data-action]="recaptchaAction" >Note: If you later rename the global callback, remember to update the string here too.
🧹 Nitpick comments (3)
src/login/pages/register/register.component.ts (1)
72-75: Clean up defensively in OnDestroy.Minor: Guard against undefined window in SSR environments.
ngOnDestroy(): void { - // eslint-disable-next-line - delete (window as any)['onSubmitRecaptcha']; + if (typeof window !== 'undefined') { + delete window.onSubmitRecaptcha; + } }src/login/pages/frontchannel-logout/frontchannel-logout.component.ts (1)
42-42: Scope the signal to the class (nit).The signal isn't referenced in the template; you can narrow visibility for clarity.
-readonly iframeLoadCount = signal(0); +private readonly iframeLoadCount = signal(0);src/login/pages/frontchannel-logout/frontchannel-logout.component.html (1)
8-8: Loop changes look good; validate trackBy uniqueness.Using
track client.nameis fine if names are unique. If not guaranteed, prefer a stable unique key (e.g., clientId or URL).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/login/pages/frontchannel-logout/frontchannel-logout.component.html(1 hunks)src/login/pages/frontchannel-logout/frontchannel-logout.component.ts(3 hunks)src/login/pages/register/register.component.html(1 hunks)src/login/pages/register/register.component.ts(3 hunks)
🧰 Additional context used
🪛 ESLint
src/login/pages/register/register.component.ts
[error] 68-68: Include a description after the "@ts-expect-error" directive to explain why the @ts-expect-error is necessary. The description must be 3 characters or longer.
(@typescript-eslint/ban-ts-comment)
🔇 Additional comments (6)
src/login/pages/register/register.component.ts (3)
1-14: Imports update LGTM.OnInit/OnDestroy are appropriate for wiring/unwiring the global callback.
34-34: Component imports LGTM.AsyncPipe/NgClass removal seems safe; not used in the template.
42-42: Class implements OnInit/OnDestroy — LGTM.Matches the lifecycle usage below.
src/login/pages/frontchannel-logout/frontchannel-logout.component.ts (1)
1-1: Good adoption of Angular signals/effect.Imports look correct for using signals/effect with OnPush and the new
viewChildfunction.src/login/pages/frontchannel-logout/frontchannel-logout.component.html (2)
14-14: LGTM: (load) binding correctly integrates with the component counter.The event binding pairs well with the new
onIframeLoaded()handler.
19-19: Unify redirect-URI presence check between template and TSThe template (frontchannel-logout.component.html:19) uses
@if (logout.logoutRedirectUri !== undefined)but the TS effect (frontchannel-logout.component.ts:48) does
if (!logout.logoutRedirectUri) { return; }This mismatch can show a “Continue” link with an empty
hrefwhile the auto-redirect never fires.Two options to align behavior:
• Preferred: use a strict undefined check in TS
--- src/login/pages/frontchannel-logout/frontchannel-logout.component.ts @@ effect(() => { - if (!logout.logoutRedirectUri) { + if (logout.logoutRedirectUri === undefined) { return; }• Or make the template use a truthy check to match TS
--- src/login/pages/frontchannel-logout/frontchannel-logout.component.html @@ -@if (logout.logoutRedirectUri !== undefined) { +@if (logout.logoutRedirectUri) {Please confirm whether
logout.logoutRedirectUrican ever be an empty string—if so, the strict=== undefinedcheck is safer.
| constructor() { | ||
| super(); | ||
| effect(() => { | ||
| const logout = this.kcContext.logout; | ||
| if (!logout.logoutRedirectUri) { | ||
| return; | ||
| } | ||
| if (this.iframeLoadCount() !== logout.clients.length) { | ||
| return; | ||
| } | ||
| window.location.replace(logout.logoutRedirectUri); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redirect gating has two pitfalls: mismatched redirectUri check and equals-only count.
- The effect uses a falsy check (
!logout.logoutRedirectUri), while the template renders the "Continue" link when the URI is notundefined. This can render an anchor with an empty href but never redirect. - Using strict equality (
===) means a duplicate load event can overshoot the count and block redirect forever. Use>=.
Apply this minimal diff to harden the logic and align with the template:
constructor() {
super();
effect(() => {
const logout = this.kcContext.logout;
- if (!logout.logoutRedirectUri) {
+ const redirectUri = logout.logoutRedirectUri;
+ if (redirectUri === undefined) {
return;
}
- if (this.iframeLoadCount() !== logout.clients.length) {
+ const expected = logout.clients.length;
+ if (this.iframeLoadCount() < expected) {
return;
}
- window.location.replace(logout.logoutRedirectUri);
+ if (typeof window !== 'undefined') {
+ window.location.replace(redirectUri);
+ }
});
}Optional but recommended for resilience (no diff provided to avoid noise):
- Add a fallback timeout (e.g., 5–10s) to redirect even if some iframes never load.
- If some clients can lack a
frontChannelLogoutUrl, consider counting only rendered iframes or guarding template rendering for those without URLs so the counts match exactly.
🤖 Prompt for AI Agents
In src/login/pages/frontchannel-logout/frontchannel-logout.component.ts around
lines 44 to 56, replace the falsy check on logoutRedirectUri with an explicit
undefined check to match the template logic and avoid rendering an empty href.
Also, change the iframe load count comparison from strict equality to greater
than or equal to (>=) to prevent blocking the redirect if duplicate load events
occur. Optionally, consider adding a fallback timeout to trigger the redirect
after a delay and ensure the iframe count matches only those with valid
frontChannelLogoutUrl to keep counts consistent.
There was a problem hiding this comment.
The effect uses a falsy check (!logout.logoutRedirectUri), while the template renders the "Continue" link when the URI is not undefined. This can render an anchor with an empty href but never redirect.
Good rabit.
Using strict equality (===) means a duplicate load event can overshoot the count and block redirect forever. Use >=.
No, it can't possibly overshoot. Bad rabbit.
Add a fallback timeout (e.g., 5–10s) to redirect even if some iframes never load.
No
If some clients can lack a frontChannelLogoutUrl, consider counting only rendered iframes or guarding template rendering for those without URLs so the counts match exactly.
No, we won't start hazardously handling cases that aren't handled in keycloak
There was a problem hiding this comment.
@garronej, you're absolutely right about the overshoot - each iframe in the @for loop can only trigger its load event once, so the count cannot exceed the number of clients. My apologies for the confusion.
Here's the focused fix for just the redirectUri check inconsistency:
constructor() {
super();
effect(() => {
const logout = this.kcContext.logout;
if (logout.logoutRedirectUri === undefined) {
return;
}
if (this.iframeLoadCount() !== logout.clients.length) {
return;
}
window.location.replace(logout.logoutRedirectUri);
});
}This aligns the effect's check with the template's @if (logout.logoutRedirectUri !== undefined) condition, ensuring consistent behavior between automatic redirect and manual "Continue" link rendering.
| onIframeLoaded() { | ||
| this.iframeLoadCount.update(count => count + 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against duplicate iframe load events to avoid missing the redirect.
If an iframe fires load more than once, the count can overshoot, and with an equals-only check the redirect never triggers. Even after adopting >=, clamping keeps the counter bounded.
onIframeLoaded() {
- this.iframeLoadCount.update(count => count + 1);
+ const expected = this.kcContext.logout.clients.length;
+ this.iframeLoadCount.update(count => Math.min(count + 1, expected));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onIframeLoaded() { | |
| this.iframeLoadCount.update(count => count + 1); | |
| } | |
| onIframeLoaded() { | |
| const expected = this.kcContext.logout.clients.length; | |
| this.iframeLoadCount.update(count => Math.min(count + 1, expected)); | |
| } |
🤖 Prompt for AI Agents
In src/login/pages/frontchannel-logout/frontchannel-logout.component.ts around
lines 58 to 60, the onIframeLoaded method increments iframeLoadCount without
guarding against multiple load events, which can cause the count to overshoot
and miss the redirect trigger. Modify the method to clamp the iframeLoadCount so
it does not exceed the expected maximum, ensuring that the redirect logic
triggers correctly even if the iframe fires load multiple times.
There was a problem hiding this comment.
He's potentially right here, I don't think in practice that the iframed page will trigger a navigation so the load event will only be loaded once.
However this is a valid concern, it should be refactored but not in the way sugessted, it introduce another bug. (What if on iframe fire load twice before all iframe have fired load?)
The sugested fix is worst than the current code. Bad rabbit.
There was a problem hiding this comment.
@garronej You're absolutely right! My suggestion was flawed. If one iframe fires load twice before others have loaded, clamping would allow the count to reach the target prematurely, triggering the redirect before all iframes have actually loaded. That's indeed worse than the current implementation.
A better approach would be to track which specific iframes have loaded using a Set to ensure each is counted only once:
readonly loadedIframes = signal(new Set<number>());
constructor() {
super();
effect(() => {
const logout = this.kcContext.logout;
if (!logout.logoutRedirectUri) {
return;
}
if (this.loadedIframes().size !== logout.clients.length) {
return;
}
window.location.replace(logout.logoutRedirectUri);
});
}
onIframeLoaded(index: number) {
this.loadedIframes.update(set => new Set(set).add(index));
}And in the template, pass the index:
@for (client of logout.clients; track client; let idx = $index) {
<iframe src="{{ client }}" (load)="onIframeLoaded(idx)"></iframe>
}This ensures each iframe is counted exactly once, regardless of how many times its load event fires.
🐰💭
| ngOnInit(): void { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (window as any)['onSubmitRecaptcha'] = () => { | ||
| // @ts-expect-error | ||
| document.getElementById('kc-register-form').requestSubmit(); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove any/ts-expect-error; add null-safety and cross-browser fallback.
Current code triggers ESLint’s ban-ts-comment (missing description) and risks a null deref on getElementById. Also, requestSubmit may be missing on older browsers.
Apply:
ngOnInit(): void {
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- (window as any)['onSubmitRecaptcha'] = () => {
- // @ts-expect-error
- document.getElementById('kc-register-form').requestSubmit();
- };
+ window.onSubmitRecaptcha = (_token?: string) => {
+ const form = document.getElementById('kc-register-form') as HTMLFormElement | null;
+ if (!form) {
+ return;
+ }
+ type FormWithRequestSubmit = HTMLFormElement & { requestSubmit?: () => void };
+ const f = form as FormWithRequestSubmit;
+ if (typeof f.requestSubmit === 'function') {
+ f.requestSubmit();
+ } else {
+ form.submit();
+ }
+ };
}Add this type augmentation (outside the above range) to avoid window casts:
declare global {
interface Window {
onSubmitRecaptcha: (token?: string) => void;
}
}If SSR is in play, consider guarding the assignment with if (typeof window === 'undefined') return;.
🧰 Tools
🪛 ESLint
[error] 68-68: Include a description after the "@ts-expect-error" directive to explain why the @ts-expect-error is necessary. The description must be 3 characters or longer.
(@typescript-eslint/ban-ts-comment)
🤖 Prompt for AI Agents
In src/login/pages/register/register.component.ts around lines 65 to 71, remove
the use of 'any' and '@ts-expect-error' by adding a proper type declaration for
'window.onSubmitRecaptcha' outside the ngOnInit method. Add null checks to
safely access the element returned by getElementById and provide a fallback for
browsers that do not support requestSubmit, such as calling the form's submit
method. Also, if server-side rendering is used, guard the assignment with a
check for 'typeof window === "undefined"' before assigning the function to avoid
runtime errors.
There was a problem hiding this comment.
No, this is a very very bad rabbit. We won't start to check for things that we know won't happen just because typescript has no way to statically understand the code.
And the global augmentation is leaky and totally unessesary. We use that function only here.
Bad rabbit
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Hello @kathari00, Thank you for implementing this. I let you merge, realease, and bump the version of Best, |
kathari00
deleted the
34-apply-patches-front-channel-logoutftl-and-registerftl
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor