fix: wire onsupportclick in setLinks() so Support item appears in header#501
Merged
Conversation
setSupportClickHandler() was a separate method called after setLinks(), but lfx-header-v2 only renders the Support menu item when properties are set together with the other header config. Moving onsupportclick into setLinks() alongside element.links ensures it is applied at the right time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
WalkthroughAppComponent now implements AfterViewInit, injects LfxHeaderService and calls setSupportClickHandler() in ngAfterViewInit. LfxHeaderService had its setSupportClickHandler() method removed; constructor no longer calls it. setLinks() was hardened to guard for a missing ChangesHeader API & Wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/shared/services/lfx-header.service.ts`:
- Around line 36-42: The code assigns properties to the DOM node with id
'lfx-header-v2' without checking for null; update the logic in
src/app/shared/services/lfx-header.service.ts (the block that gets
document.getElementById('lfx-header-v2')) to first guard that the element exists
(e.g. if (!element) return or skip setup) before calling element.links =
this.links and setting element.onsupportclick; keep the window.Intercom check
intact inside the guarded block so no assignments occur when the element is
absent.
- Around line 36-40: In setLinks() ensure you null-check and properly type the
custom element before dereferencing: retrieve the element with const element:
HTMLElement | null = document.getElementById('lfx-header-v2'); if (!element)
return; then assign links and onsupportclick on a correctly typed interface
(e.g., define LfxHeaderElement extending HTMLElement with links and
onsupportclick) or cast only where needed (element as any). Also replace direct
window.Intercom('show') with IntercomService.show() for consistency with
setCallBackUrl() and setUserInLFxHeader().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0910ab45-2304-44a6-9244-afa487e41c96
📒 Files selected for processing (1)
src/app/shared/services/lfx-header.service.ts
There was a problem hiding this comment.
Pull request overview
This PR fixes the missing “Support” menu item in the lfx-header-v2 header by wiring the onsupportclick handler during the same configuration pass as the header’s links, and removing the now-redundant separate support handler method.
Changes:
- Move
onsupportclickassignment intosetLinks()alongsideelement.links. - Remove
setSupportClickHandler()and its invocation from the service constructor.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…em appears The previous fix moved onsupportclick into setLinks(), which was incorrect. The real issue is timing: calling it from the service constructor or setLinks() runs before the view is initialized. ngAfterViewInit is the correct lifecycle hook for DOM-dependent setup — the same pattern used in easycla-landing-page. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/app.component.ts (1)
4-4: ⚡ Quick winImplement
OnInitexplicitly in the component contract.
ngOnInit()is present, butOnInitis not in theimplementslist. Add it (and keep lifecycle signatures explicitly typed).Proposed refactor
-import { AfterViewInit, Component, OnDestroy } from '@angular/core'; +import { AfterViewInit, Component, OnDestroy, OnInit } from '@angular/core'; ... -export class AppComponent implements AfterViewInit, OnDestroy { +export class AppComponent implements OnInit, AfterViewInit, OnDestroy {As per coding guidelines, "
**/*.component.ts: Implement OnInit lifecycle hook for initialization logic" and "**/*.ts: Use proper TypeScript typing for all properties and methods".Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/app.component.ts` at line 4, The component declares ngOnInit but does not include OnInit in its implements list; update the AppComponent class declaration to implement OnInit (in addition to AfterViewInit and OnDestroy) and ensure the ngOnInit(): void signature remains explicitly typed so the lifecycle contract is enforced (refer to AppComponent and its ngOnInit method to locate the change).src/app/shared/services/lfx-header.service.ts (1)
52-57: ⚡ Quick winTighten typing in callback URL wiring (
any+ implicit return type).Use an explicit return type and a typed element shape here to match strict TypeScript patterns.
Proposed refactor
+interface LfxHeaderElement extends HTMLElement { + callbackurl?: string; +} + - setCallBackUrl() { - const lfHeaderEl: any = document.getElementById('lfx-header-v2'); + setCallBackUrl(): void { + const lfHeaderEl = document.getElementById('lfx-header-v2') as LfxHeaderElement | null; if (lfHeaderEl) { lfHeaderEl.callbackurl = this.auth.auth0Options.callbackUrl; } }As per coding guidelines, "
**/*.ts: Use proper TypeScript typing for all properties and methods".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/shared/services/lfx-header.service.ts` around lines 52 - 57, The method setCallBackUrl uses an untyped any and has an implicit return type; change its signature to setCallBackUrl(): void and replace the any with a typed element interface (e.g., declare interface LfxHeaderElement extends HTMLElement { callbackurl?: string } and cast document.getElementById('lfx-header-v2') to LfxHeaderElement | null), then assign lfHeaderEl.callbackurl = this.auth.auth0Options.callbackUrl after a null check; this tightens types around setCallBackUrl, lfHeaderEl, and the callbackurl property.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/app.component.ts`:
- Around line 32-34: ngAfterViewInit currently calls
this.lfxHeaderService.setSupportClickHandler() immediately which can race with
the async registration of the custom element 'lfx-header-v2' (see mountHeader);
change the call to wait for the element to be defined by using
customElements.whenDefined('lfx-header-v2').then(() =>
this.lfxHeaderService.setSupportClickHandler()) so the LfxHeaderService's
setSupportClickHandler() runs only after the custom element is registered and
avoids the early-return path that prevents support linking.
---
Nitpick comments:
In `@src/app/app.component.ts`:
- Line 4: The component declares ngOnInit but does not include OnInit in its
implements list; update the AppComponent class declaration to implement OnInit
(in addition to AfterViewInit and OnDestroy) and ensure the ngOnInit(): void
signature remains explicitly typed so the lifecycle contract is enforced (refer
to AppComponent and its ngOnInit method to locate the change).
In `@src/app/shared/services/lfx-header.service.ts`:
- Around line 52-57: The method setCallBackUrl uses an untyped any and has an
implicit return type; change its signature to setCallBackUrl(): void and replace
the any with a typed element interface (e.g., declare interface LfxHeaderElement
extends HTMLElement { callbackurl?: string } and cast
document.getElementById('lfx-header-v2') to LfxHeaderElement | null), then
assign lfHeaderEl.callbackurl = this.auth.auth0Options.callbackUrl after a null
check; this tightens types around setCallBackUrl, lfHeaderEl, and the
callbackurl property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d2f6e2f-91c6-42fc-b563-2f79ac5de9d0
📒 Files selected for processing (2)
src/app/app.component.tssrc/app/shared/services/lfx-header.service.ts
setCallBackUrl already guards against a missing lfx-header-v2 element; setLinks was missing the same check, causing a potential null dereference if the element is not yet in the DOM when the service is constructed. Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
Contributor
Author
Review Feedback AddressedCommit: ef2a28e Changes Made
Declined
Threads Resolved9 of 9 unresolved threads addressed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/shared/services/lfx-header.service.ts (1)
55-60: ⚡ Quick winUse strict typing in
setCallBackUrl()(voidreturn + typed element).Line 55 and Line 56 still use implicit return type and
any, which weakens strict-mode guarantees.Suggested fix
+interface LfxHeaderElement extends HTMLElement { + callbackurl?: string; +} + - setCallBackUrl() { - const lfHeaderEl: any = document.getElementById('lfx-header-v2'); + setCallBackUrl(): void { + const lfHeaderEl = document.getElementById('lfx-header-v2') as LfxHeaderElement | null; if (lfHeaderEl) { lfHeaderEl.callbackurl = this.auth.auth0Options.callbackUrl; } }As per coding guidelines,
**/*.ts: "Use TypeScript strict mode patterns" and "Use proper TypeScript typing for all properties and methods".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/shared/services/lfx-header.service.ts` around lines 55 - 60, setCallBackUrl currently uses implicit any and no return type; change its signature to return void and strongly type the DOM element (e.g., const lfHeaderEl = document.getElementById('lfx-header-v2') as HTMLElement | null or a more specific element interface) instead of any, check for null, and assign the callback using the typed property (or cast to the custom element interface if the element exposes callbackurl). Reference symbols: setCallBackUrl, lfHeaderEl, this.auth.auth0Options.callbackUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/shared/services/lfx-header.service.ts`:
- Around line 37-40: The constructor of LfxHeaderService wires DOM elements too
early (setLinks and setCallBackUrl) causing silent failures when `#lfx-header-v2`
mounts later; move those calls out of the constructor and into a new
initializeHeaderBindings() method that performs the same DOM wiring (invoking
setLinks(), setCallBackUrl(), and setSupportClickHandler() safely) and call
initializeHeaderBindings() from AppComponent.ngAfterViewInit() so bindings occur
after the header mounts; also fix TypeScript strict types by replacing any with
explicit types for links (e.g., Array<Link> or appropriate interface),
lfHeaderEl (HTMLElement | null), and add explicit return types to setLinks() and
setCallBackUrl() to satisfy strict typing.
---
Nitpick comments:
In `@src/app/shared/services/lfx-header.service.ts`:
- Around line 55-60: setCallBackUrl currently uses implicit any and no return
type; change its signature to return void and strongly type the DOM element
(e.g., const lfHeaderEl = document.getElementById('lfx-header-v2') as
HTMLElement | null or a more specific element interface) instead of any, check
for null, and assign the callback using the typed property (or cast to the
custom element interface if the element exposes callbackurl). Reference symbols:
setCallBackUrl, lfHeaderEl, this.auth.auth0Options.callbackUrl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 467d7f1a-f4ab-4072-b719-aa87b45684a5
📒 Files selected for processing (1)
src/app/shared/services/lfx-header.service.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes missing "Support" item in the LFX header help menu by calling
setSupportClickHandler()fromngAfterViewInitinstead of the service constructor.Root cause
setSupportClickHandler()was being called fromLfxHeaderService's constructor, which runs before Angular initializes the view. At that pointgetElementById('lfx-header-v2')returnsnulland the handler is never set.The fix mirrors the pattern in
easycla-landing-page, which callssetSupportClickHandler()fromAppComponent.ngAfterViewInit()— the correct lifecycle hook for DOM-dependent setup.Changes
app.component.ts: implementAfterViewInit, injectLfxHeaderService, callsetSupportClickHandler()fromngAfterViewInit()lfx-header.service.ts: restoresetSupportClickHandler()as a standalone method (reverts the previous incorrect fix that placedonsupportclickinsidesetLinks())Test plan
?help icon in the LFX header — verify "Support" item appears alongside "Docs" and "FAQ"🤖 Generated with Claude Code