feat(intercom): add Intercom Messenger support bot (LFXV2-1598)#661
Open
audigregorie wants to merge 18 commits into
Open
feat(intercom): add Intercom Messenger support bot (LFXV2-1598)#661audigregorie wants to merge 18 commits into
audigregorie wants to merge 18 commits into
Conversation
Signed-off-by: Audi Young <audi.mycloud@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR integrates Intercom as a customer support chat widget throughout the LFX application. Changes include new shared Intercom type definitions, an Angular Intercom service with boot/lifecycle management, a reusable OpenIntercom directive, runtime configuration to pass the Intercom app ID from environment variables, AppComponent bootstrap logic to conditionally initialize Intercom, and UI updates replacing external support links with Intercom triggers. Configuration and documentation are updated for local and production environments. ChangesIntercom Support Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds Intercom Messenger as the primary in-app support mechanism for LFX One, wiring it to runtime configuration (via SSR TransferState) and gating boot via a LaunchDarkly/OpenFeature flag, while migrating several UI “Support” entry points to a shared directive.
Changes:
- Introduces an
IntercomService(script injection + boot/show/update/shutdown) and shared Intercom typings/window globals. - Adds an
lfxOpenIntercomdirective to open Messenger with a Jira Service Desk fallback, and updates multiple UI components to use it. - Extends runtime config/docs/env to include
INTERCOM_APP_ID(intercomAppId) and passes it from server → browser.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/runtime-config.interface.ts | Adds intercomAppId to the runtime config contract. |
| packages/shared/src/interfaces/intercom.interface.ts | Adds shared Intercom boot/options and function typings. |
| packages/shared/src/interfaces/index.ts | Re-exports new Intercom interfaces from the shared barrel. |
| packages/shared/src/interfaces/auth.interface.ts | Adds namespaced user claim for Intercom identity-verification JWT. |
| docs/runtime-configuration.md | Documents intercomAppId / INTERCOM_APP_ID and updates examples. |
| Dockerfile | Updates build comment to include Intercom runtime-injected ID. |
| apps/lfx-one/src/types/window.d.ts | Adds window.Intercom and window.intercomSettings typings. |
| apps/lfx-one/src/server/server.ts | Injects INTERCOM_APP_ID into SSR runtimeConfig. |
| apps/lfx-one/src/app/shared/services/intercom.service.ts | New browser-only wrapper for loading/booting Intercom Messenger. |
| apps/lfx-one/src/app/shared/providers/runtime-config.provider.ts | Extends default runtime config with intercomAppId. |
| apps/lfx-one/src/app/shared/directives/open-intercom.directive.ts | New directive to open Intercom or fallback to Jira support URL. |
| apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.ts | Imports the Intercom launcher directive for the lens menu UI. |
| apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.html | Replaces Jira support links with lfxOpenIntercom buttons. |
| apps/lfx-one/src/app/modules/profile/identities/profile-identities.component.ts | Imports the Intercom launcher directive for profile identities UI. |
| apps/lfx-one/src/app/modules/profile/identities/profile-identities.component.html | Replaces Jira “contact support” link with lfxOpenIntercom. |
| apps/lfx-one/src/app/modules/meetings/meeting-not-found/meeting-not-found.component.ts | Removes explicit support URL usage; imports Intercom launcher directive. |
| apps/lfx-one/src/app/modules/meetings/meeting-not-found/meeting-not-found.component.html | Changes “Contact Support” action to use lfxOpenIntercom. |
| apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.ts | Imports the Intercom launcher directive for mailing-list dashboard. |
| apps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.html | Replaces Jira support button link with lfxOpenIntercom. |
| apps/lfx-one/src/app/app.component.ts | Boots Intercom after feature flag init when enable-intercom is enabled. |
| apps/lfx-one/.env.example | Documents INTERCOM_APP_ID for local/runtime configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/runtime-configuration.md (1)
157-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the
INTERCOM_APP_IDdocs consistent with the examples below.The table now presents this as required, but the Local Development, Docker, and Kubernetes snippets still omit it. Right now a reader copying those examples will end up with Intercom disabled even though the table says otherwise.
🤖 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 `@docs/runtime-configuration.md` around lines 157 - 165, The Required Variables table lists INTERCOM_APP_ID as required but the Local Development, Docker, and Kubernetes example snippets omit it; either move INTERCOM_APP_ID out of the "Required Variables" table into an "Optional Variables" section (or annotate it as optional) OR add INTERCOM_APP_ID with a sample value to the Local Development, Docker, and Kubernetes snippets so they match the table; update references to INTERCOM_APP_ID everywhere in runtime-configuration.md (the "Required Variables" table and the example snippets) to keep the doc consistent.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/app.component.ts (1)
86-91: ⚡ Quick winExtract this into an async initializer instead of chaining
.then().This is easier to follow as a small
asynchelper invoked from the constructor, and it matches the repo convention for async flows.Suggested refactor
- this.featureFlagService - .initialize(authedUser) - .then(() => { - if (!isImpersonating) { - this.bootIntercomIfEnabled(authedUser); - } - }) - .catch((error) => { - console.error('Failed to initialize feature flags:', error); - }); + void this.initializeAuthenticatedIntegrations(authedUser, isImpersonating);private async initializeAuthenticatedIntegrations(user: User, isImpersonating: boolean): Promise<void> { try { await this.featureFlagService.initialize(user); if (!isImpersonating) { this.bootIntercomIfEnabled(user); } } catch (error) { console.error('Failed to initialize feature flags:', error); } }As per coding guidelines, "Use async/await instead of .then() chains for asynchronous operations".
🤖 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 `@apps/lfx-one/src/app/app.component.ts` around lines 86 - 91, Replace the .then()/.catch() chain in the constructor with an async helper: add a private async method (e.g. initializeAuthenticatedIntegrations(user: User, isImpersonating: boolean)) that awaits this.featureFlagService.initialize(user), calls this.bootIntercomIfEnabled(user) only when not impersonating, and catches/logs errors (preserving the existing console.error message); then invoke that helper from the constructor instead of chaining .then().
🤖 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 `@apps/lfx-one/src/app/app.component.ts`:
- Around line 83-88: The code boots Intercom with this.auth.user which is the
impersonated identity during impersonation; change the flow in app.component.ts
so bootIntercomIfEnabled is only called when not impersonating (e.g., guard with
if (!this.auth.isImpersonating)
this.featureFlagService.initialize(authedUser).then(() =>
this.bootIntercomIfEnabled(authedUser))), or alternatively pass the impersonator
identity to bootIntercomIfEnabled (use the real user/impersonator property on
the auth service instead of this.auth.user) so Intercom never receives the
impersonated user's profile/JWT.
In
`@apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.html`:
- Around line 177-180: The template incorrectly treats the template reference
userMenu as a function call (userMenu()?.hide()), causing TS2349; update the
click handler in lens-switcher.component.html to call the Popover method
directly by replacing the function-style call with a property access invoking
hide on the template reference (use userMenu.hide() or userMenu?.hide() if null
safety is needed) so the Popover instance's hide method is called correctly.
In `@apps/lfx-one/src/app/shared/directives/open-intercom.directive.ts`:
- Around line 27-33: The click handler currently doesn't cancel the host
element's native click, so add event interception: change the HostListener to
capture the event (use `@HostListener`('click', ['$event'])) and update the
onClick signature to accept an Event, then call event.preventDefault() and
event.stopPropagation() at the top before invoking
intercomService.isIntercomBooted()/intercomService.show() or
window.open(environment.urls.support, ...), ensuring native navigation from
<a>/routerLink hosts is suppressed.
In `@apps/lfx-one/src/app/shared/services/intercom.service.ts`:
- Around line 55-80: The bug is racey because concurrent boot() calls mutate
global window.intercomSettings and then later use different captured options;
fix by taking a snapshot of the options immediately before touching globals
inside the checkLoaded block (e.g., const snapshot = { ...options }), then set
window.intercomSettings.intercom_user_jwt = snapshot.intercom_user_jwt and call
window.Intercom('boot', this.stripJwt(snapshot)); also use snapshot for any
subsequent update()/toUserAttributes(snapshot) calls and set isBooted after
creating the snapshot to ensure the same user payload and JWT are used
atomically for a single boot.
In `@Dockerfile`:
- Around line 33-36: The Docker build fails at the step running "RUN yarn
workspace lfx-one-ui build:${BUILD_ENV}" due to a TypeScript compilation error
not visible in the PR comment — reproduce the failure locally (or inspect the
full Docker build log) to get the exact tsc error, then fix the mismatched type
reported; likely candidates to check are RuntimeConfig.intercomAppId and its
implementations (server.ts, provider.ts), the intercom.interface.ts export, and
any usages in open-intercom.directive.ts, intercom.service.ts, app.component.ts
or window.d.ts — correct the offending type/signature to match the declared
interface or update the interface/usages consistently and re-run the yarn
workspace lfx-one-ui build to verify the pipeline step succeeds.
---
Outside diff comments:
In `@docs/runtime-configuration.md`:
- Around line 157-165: The Required Variables table lists INTERCOM_APP_ID as
required but the Local Development, Docker, and Kubernetes example snippets omit
it; either move INTERCOM_APP_ID out of the "Required Variables" table into an
"Optional Variables" section (or annotate it as optional) OR add INTERCOM_APP_ID
with a sample value to the Local Development, Docker, and Kubernetes snippets so
they match the table; update references to INTERCOM_APP_ID everywhere in
runtime-configuration.md (the "Required Variables" table and the example
snippets) to keep the doc consistent.
---
Nitpick comments:
In `@apps/lfx-one/src/app/app.component.ts`:
- Around line 86-91: Replace the .then()/.catch() chain in the constructor with
an async helper: add a private async method (e.g.
initializeAuthenticatedIntegrations(user: User, isImpersonating: boolean)) that
awaits this.featureFlagService.initialize(user), calls
this.bootIntercomIfEnabled(user) only when not impersonating, and catches/logs
errors (preserving the existing console.error message); then invoke that helper
from the constructor instead of chaining .then().
🪄 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: 6de15ded-5b13-453d-a016-3b4e4880387a
📒 Files selected for processing (21)
Dockerfileapps/lfx-one/.env.exampleapps/lfx-one/src/app/app.component.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.htmlapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.tsapps/lfx-one/src/app/modules/meetings/meeting-not-found/meeting-not-found.component.htmlapps/lfx-one/src/app/modules/meetings/meeting-not-found/meeting-not-found.component.tsapps/lfx-one/src/app/modules/profile/identities/profile-identities.component.htmlapps/lfx-one/src/app/modules/profile/identities/profile-identities.component.tsapps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.htmlapps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.tsapps/lfx-one/src/app/shared/directives/open-intercom.directive.tsapps/lfx-one/src/app/shared/providers/runtime-config.provider.tsapps/lfx-one/src/app/shared/services/intercom.service.tsapps/lfx-one/src/server/server.tsapps/lfx-one/src/types/window.d.tsdocs/runtime-configuration.mdpackages/shared/src/interfaces/auth.interface.tspackages/shared/src/interfaces/index.tspackages/shared/src/interfaces/intercom.interface.tspackages/shared/src/interfaces/runtime-config.interface.ts
- Fix TS2349 build failure: userMenu()?.hide() -> userMenu.hide() in lens-switcher template - Skip Intercom boot during impersonation to prevent sending impersonated identity to third-party widget - Fail fast on script load error (onerror rejects immediately instead of waiting 10s timeout) - Prevent race condition in concurrent boot() calls by snapshotting options and setting JWT atomically - Add event.preventDefault() to OpenIntercomDirective to prevent native navigation on <a> hosts - Add SSR guard to window.open fallback in OpenIntercomDirective Signed-off-by: Audi Young <audi.mycloud@gmail.com>
MRashad26
reviewed
May 7, 2026
Contributor
|
Process: AI co-author in commit history. Commit Suggested fix: Amend the commit to remove the trailer before this PR is merged: git rebase -i HEAD~<n> # edit the affected commit
# Remove the Co-authored-by: Cursor line from the commit message
git push --force-with-lease |
Collapse multi-line JSDoc blocks to 1-line per project comment rules, wire script.onerror to immediately clear the boot() polling interval, and remove stray whitespace in the mailing-list support button. Signed-off-by: Audi Young <audi.mycloud@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/app.component.ts (1)
86-95: ⚡ Quick winReplace promise chains with
async/awaitfollowing the repository's async pattern guidelines.The code uses
.then()/.catch()chains at lines 88–95 and 124–126, which violates the guideline to useasync/awaitfor asynchronous operations. Convert these toasync/awaitfor consistency with repository patterns.Proposed refactor
- this.featureFlagService - .initialize(authedUser) - .then(() => { - if (!isImpersonating) { - this.bootIntercomIfEnabled(authedUser); - } - }) - .catch((error) => { - console.error('Failed to initialize feature flags:', error); - }); + void (async () => { + try { + await this.featureFlagService.initialize(authedUser); + if (!isImpersonating) { + await this.bootIntercomIfEnabled(authedUser); + } + } catch (error) { + console.error('Failed to initialize feature flags:', error); + } + })(); @@ - private bootIntercomIfEnabled(user: User): void { + private async bootIntercomIfEnabled(user: User): Promise<void> { @@ - this.intercomService - .boot({ - app_id: intercomAppId, - intercom_user_jwt: intercomJwt, - user_id: userId, - name: user.name, - email: user.email, - }) - .catch((error) => { - console.error('Intercom boot failed', error); - }); + try { + await this.intercomService.boot({ + app_id: intercomAppId, + intercom_user_jwt: intercomJwt, + user_id: userId, + name: user.name, + email: user.email, + }); + } catch (error) { + console.error('Intercom boot failed', error); + } }🤖 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 `@apps/lfx-one/src/app/app.component.ts` around lines 86 - 95, Replace the .then/.catch promise chain around featureFlagService.initialize(...) with an async/await pattern: await featureFlagService.initialize(authedUser) inside the surrounding async lifecycle method (e.g., ngOnInit or the async initializer), wrap it in try/catch to handle and log errors, and keep the isImpersonating check so bootIntercomIfEnabled(authedUser) is only called when appropriate; also find the other .then/.catch usage elsewhere in this component and refactor it similarly to await the promise and handle errors with try/catch to match the repository async/await style.
🤖 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.
Nitpick comments:
In `@apps/lfx-one/src/app/app.component.ts`:
- Around line 86-95: Replace the .then/.catch promise chain around
featureFlagService.initialize(...) with an async/await pattern: await
featureFlagService.initialize(authedUser) inside the surrounding async lifecycle
method (e.g., ngOnInit or the async initializer), wrap it in try/catch to handle
and log errors, and keep the isImpersonating check so
bootIntercomIfEnabled(authedUser) is only called when appropriate; also find the
other .then/.catch usage elsewhere in this component and refactor it similarly
to await the promise and handle errors with try/catch to match the repository
async/await style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f8a982b-d320-42f7-bdfb-05aac223fa4b
📒 Files selected for processing (6)
apps/lfx-one/src/app/app.component.tsapps/lfx-one/src/app/modules/mailing-lists/mailing-list-dashboard/mailing-list-dashboard.component.htmlapps/lfx-one/src/app/shared/directives/open-intercom.directive.tsapps/lfx-one/src/app/shared/services/intercom.service.tspackages/shared/src/interfaces/intercom.interface.tspackages/shared/src/interfaces/runtime-config.interface.ts
✅ Files skipped from review due to trivial changes (2)
- packages/shared/src/interfaces/intercom.interface.ts
- packages/shared/src/interfaces/runtime-config.interface.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/lfx-one/src/app/shared/directives/open-intercom.directive.ts
- apps/lfx-one/src/app/shared/services/intercom.service.ts
LFXV2-1598) PR #660 changed folderOptions.length to folderOptions?.length, causing two template compiler errors: TS2532 (result is number|undefined) and NG8107 (?: unnecessary since the type is non-nullable array). Signed-off-by: Audi Young <audi.mycloud@gmail.com>
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-661.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
…2-1598) Adds a console.warn when the enable-intercom LD flag is on but appId, intercom JWT, or userId is missing, providing a visible breadcrumb for diagnosing silent boot failures in dev/staging. Signed-off-by: Audi Young <audi.mycloud@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/lfx-one/src/app/app.component.ts (1)
108-110: ⚡ Quick winExtract JWT claim name constants to shared module.
'http://lfx.dev/claims/intercom'and'https://sso.linuxfoundation.org/claims/username'are hardcoded across multiple files:app.component.ts,profile-affiliations.component.ts(3 occurrences),server.ts, and more. These should be defined as constants in@lfx-one/sharedand imported rather than hardcoded string literals. This follows the coding guideline: "All shared constants and interfaces must live in@lfx-one/shared— no module-level consts or local interface definitions inside apps/lfx-one/".🤖 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 `@apps/lfx-one/src/app/app.component.ts` around lines 108 - 110, The hardcoded JWT claim names used when deriving intercomJwt and userId (e.g., the string literals in app.component.ts where intercomJwt = user['http://lfx.dev/claims/intercom'] and userId = user['https://sso.linuxfoundation.org/claims/username'] || user.sub) must be moved to the shared constants module; add exported constants (e.g., INTERCOM_CLAIM and SSO_USERNAME_CLAIM) in `@lfx-one/shared` and then replace the literal usages in this file and other occurrences (including where profile-affiliations and server logic reference them) to import and use those constants instead of raw strings so all claim names are centralized and consistent.
🤖 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.
Nitpick comments:
In `@apps/lfx-one/src/app/app.component.ts`:
- Around line 108-110: The hardcoded JWT claim names used when deriving
intercomJwt and userId (e.g., the string literals in app.component.ts where
intercomJwt = user['http://lfx.dev/claims/intercom'] and userId =
user['https://sso.linuxfoundation.org/claims/username'] || user.sub) must be
moved to the shared constants module; add exported constants (e.g.,
INTERCOM_CLAIM and SSO_USERNAME_CLAIM) in `@lfx-one/shared` and then replace the
literal usages in this file and other occurrences (including where
profile-affiliations and server logic reference them) to import and use those
constants instead of raw strings so all claim names are centralized and
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ba31ed9-a42f-49cc-8cbf-6d7f3164ea9e
📒 Files selected for processing (1)
apps/lfx-one/src/app/app.component.ts
asithade
requested changes
May 7, 2026
Contributor
asithade
left a comment
asithade
left a comment
There was a problem hiding this comment.
Thanks for iterating — 13 of 16 prior review threads were addressed and the snapshot pattern fix for the JWT race is solid. A few items still need attention before this can merge.
Critical — deployment readiness
INTERCOM_APP_IDis read at runtime by the SSR server but is not defined inlinuxfoundation/lfx-v2-argocdvalues/global/lfx-v2-ui.yaml(or any per-env override). Without the companion argocd PR, the LD flag can flip on butgetRuntimeConfig().intercomAppIdwill be''andbootIntercomIfEnabledwill silently no-op in dev/staging/prod. The workspace IDs are already known from the existinglfit-identity-cookie-helperconfigs in argocd:mxl90k6y(dev/staging) andw29sqomy(prod).
Should fix
- Drop the
enable-intercomLaunchDarkly flag. It doesn't add value:INTERCOM_APP_ID=""is already a kill-switch (server.ts +bootIntercomIfEnabled'sif (!intercomAppId)guard), the flag is sampled once at boot so it can't act as a live kill-switch either, and there's no per-user/per-org targeting wired into thegetBooleanFlagcall. See the inline comment onapp.component.tsfor the proposed simplification. - PR title —
feat(intercom): add Intercom Messenger support bot (LFXV2-1598)includes the JIRA ticket and uses TitleCase. Per.claude/rules/commit-workflow.md: "Do not include the JIRA ticket in the title" and "Everything should be in lowercase". Suggest:feat(intercom): add intercom messenger support bot. Branch namefeat/LFXV2-1598-add-intercom-botis fine. IntercomServicepolling pattern is unnecessary per Intercom's official installation guide. The documented stub-queue pattern absorbsIntercom('boot', ...)calls placed before the script loads — noscript.onloadhandler, nosetIntervalpolling, nosetTimeoutwatchdog needed. A drop-in refactor (~46% smaller, same public API) is included as a suggestion in the inline comment.
Nits
- The Dockerfile change is a 2-line comment update with no functional effect — please drop it to keep the PR focused.
runtime-config.interface.ts:35JSDoc still misnames the Auth0 claim.- Four methods in
intercom.service.tsstill have multi-line JSDoc content blocks (would be moot if the refactor is taken).
…-1598) Remove the LaunchDarkly enable-intercom flag check, pass JWT directly in the boot payload instead of splitting it onto window.intercomSettings, and drop the redundant post-boot update call and shutdown JWT cleanup. Signed-off-by: Audi Young <audi.mycloud@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
The current IntercomService wraps boot() in a 100ms-poll + 10s-timeout state machine, but Intercom's queue stub (initializeIntercomFunction) already buffers calls made before the script loads. Replace the polling machinery with the same fire-and-forget pattern PlausibleService uses, and remove update()/hide()/trackEvent()/toUserAttributes() which have no callers. Keep shutdown() for future impersonation identity reset. Also drop the open-intercom-bot host class binding (no stylesheet references it) and trim IntercomFunction overloads to match. ~265 LOC -> ~80 LOC. No behavior change for booted or fallback users. Signed-off-by: Audi Young <audi.mycloud@gmail.com>
Revert Dockerfile inline comment (no functional change). Fix runtime-config.interface.ts JSDoc: `intercom_user_jwt` is the Intercom boot option name, not the Auth0 claim — actual claim is `http://lfx.dev/claims/intercom`. Collapse multi-line JSDoc on boot/show/shutdown to single-line comments per project rules. Signed-off-by: Audi Young <audi.mycloud@gmail.com>
…V2-1598) Signed-off-by: Audi Young <audi.mycloud@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
Signed-off-by: Audi Young <audi.mycloud@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Audi Young <audi.mycloud@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Audi Young <audi.mycloud@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # apps/lfx-one/src/app/shared/components/lens-switcher/lens-switcher.component.ts # docs/runtime-configuration.md
…undation/lfx-v2-ui into feat/LFXV2-1598-add-intercom-bot Signed-off-by: Audi Young <audi.mycloud@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
MRashad26
reviewed
May 19, 2026
Contributor
MRashad26
left a comment
MRashad26
left a comment
There was a problem hiding this comment.
All 6 prior comments are addressed at this HEAD — JSDoc trimming and the script.onerror/polling-interval cleanup both landed cleanly, and the broader refactor that swapped ~10 hardcoded Jira URLs to the new lfxOpenIntercom directive is a nice maintainability win on top of the feature. Three minor nits below (not blockers) — feel free to land first and address in a follow-up.
|
|
||
| this.loadIntercomScript(options.app_id, options.api_base); | ||
| window.Intercom!('boot', options); | ||
| this.isBooted = true; |
Contributor
There was a problem hiding this comment.
Problem: this.isBooted = true is set synchronously, before the Intercom script has actually loaded:
public boot(options: IntercomBootOptions): void {
if (typeof window === 'undefined' || !options.app_id || this.isBooted) {
return;
}
this.loadIntercomScript(options.app_id, options.api_base); // creates <script>, async
window.Intercom!('boot', options); // queued on stub
this.isBooted = true; // ← set before load completes
}At this moment, window.Intercom is the stub queue, not the real SDK. OpenIntercomDirective.onClick (L19 of open-intercom.directive.ts) reads isBooted to decide whether to call show() or fall back. Between this assignment and script.onload/script.onerror, the flag claims "booted" while the widget is in fact "boot-queued".
Why this is a problem:
- The flag name lies —
isBootedreads as "the messenger is ready to receive commands". The actual semantic is "a boot has been requested". The stub queue happens to bail us out (queuedshowcalls replay on load), but a future maintainer reading the directive will reasonably expectisBooted === trueto mean the widget is actually live. script.onerrormid-window bug class — if the user clicks Support afterisBootedis set but before the script-load failure callback fires, the click is routed tointercomService.show(), queued onto the stub, and then lost forever whenonerrorsetsisBooted = falseon L68 (the queue is never replayed; the real SDK never loads). A second click then falls back to Jira correctly, but the first click silently does nothing.- Semantic drift over time — code that later wants to check "is Intercom really ready" (e.g. to gate an unsolicited
trackEventcall, or to wait before showing a tooltip explaining the widget) has no signal to read.
Suggested fix: Split intent from readiness — isBootRequested for the directive's gate (since the stub queue makes that the right test) and isBooted for actually-loaded state set only from script.onload:
public isBootRequested = false; // directive reads this — stub queue handles the gap
public isBooted = false; // set only from script.onload
public boot(options: IntercomBootOptions): void {
if (typeof window === 'undefined' || !options.app_id || this.isBootRequested) return;
this.loadIntercomScript(options.app_id, options.api_base);
window.Intercom!('boot', options);
this.isBootRequested = true;
}
private loadIntercomScript(appId: string, apiBase?: string): void {
// ...
script.onload = () => { this.isLoaded = true; this.isBooted = true; };
script.onerror = (error) => { this.isBootRequested = false; console.error(...); };
}Then OpenIntercomDirective reads isBootRequested (its current semantic), and any future code wanting true readiness reads isBooted. Same external behavior; the names match reality.
|
|
||
| script.onerror = (error) => { | ||
| this.isBooted = false; | ||
| console.error('IntercomService: Failed to load script', error); |
Contributor
There was a problem hiding this comment.
Problem: script.onerror is a single-shot dead end:
script.onerror = (error) => {
this.isBooted = false;
console.error('IntercomService: Failed to load script', error);
};If widget.intercom.io is flaky for the duration of a single TCP attempt (transient CDN hiccup, brief network blip), the user is stuck in fallback mode for the entire session. They get the Jira link every time they click Support, even after the network has recovered.
Why this is a problem:
- No retry — the prior implementation (which the previous PR review flagged for an unrelated bug) at least kept polling. The refactor correctly dropped the polling for the success path (
onloadis the right signal) but inadvertently dropped the retry path too. A single failure now causes a session-long degradation. - No way for the user to recover — there's no manual retry surface. The Support button always lands them on Jira, never on the chat they would have gotten if the script had loaded.
- Hard to diagnose —
console.erroris local to the user's browser. Ops has no signal that a non-trivial percentage of users are stuck in fallback. The DataDog RUM integration that's already wired up (dataDogRumServiceis injected inapp.component.ts:30) could emit a custom error here.
Suggested fix: Two cheap additions, in priority order.
Most valuable — emit a RUM error so degradation is visible:
script.onerror = (error) => {
this.isBooted = false;
console.error('IntercomService: Failed to load script', error);
// tell RUM, so we can dashboard "users stuck in Jira fallback"
this.dataDogRumService.addError(new Error('Intercom script failed to load'), { context: 'intercom_load' });
};Optional — retry once after a brief delay:
private scriptLoadAttempts = 0;
// ...
script.onerror = (error) => {
console.warn('IntercomService: Script load failed, will retry once', error);
if (this.scriptLoadAttempts < 1) {
this.scriptLoadAttempts++;
setTimeout(() => { this.isLoaded = false; this.loadIntercomScript(appId, apiBase); }, 2000);
return;
}
this.isBooted = false;
console.error('IntercomService: Script load failed after retry', error);
};The RUM error is the higher-leverage change — without it, this failure mode is invisible to the team. The retry is a stretch goal.
| (command: 'show'): void; | ||
| (command: 'shutdown'): void; | ||
| (command: 'reattach_activator'): void; | ||
| (command: string, ...args: unknown[]): void; |
Contributor
There was a problem hiding this comment.
Problem: The IntercomFunction interface declares typed overloads, then a variadic catch-all:
export interface IntercomFunction {
(command: 'boot', options: IntercomBootOptions): void;
(command: 'show'): void;
(command: 'shutdown'): void;
(command: 'reattach_activator'): void;
(command: string, ...args: unknown[]): void; // ← swallows everything above
q?: unknown[][];
c?: (args: unknown[]) => void;
}TypeScript's overload resolution will match the typed overloads when the literal 'boot' | 'show' | ... is used as the first argument, but the catch-all (command: string, ...args: unknown[]) makes the typed overloads structurally redundant for any non-literal — and worse, it erases the type-checking benefit at every callsite where command flows in from a variable.
Why this is a problem:
- The overloads are decorative — they look like they constrain callers, but
intercomService.ts:24callswindow.Intercom!('boot', options)with a string literal where the typed overload does fire, and the stub at L91-95 callsstub.c?.(args)where the catch-all is the only matching signature. There's no callsite where the typed overloads provide a benefit the catch-all doesn't also allow. ('boot', someStringVariable)would silently typecheck even ifsomeStringVariableis'shutdown'— there's no way to catch a typo'd command at the callsite, because the catch-all accepts any string.- Future readers will assume the typed overloads enforce a discriminated command set — they don't.
Suggested fix: Drop the catch-all so the typed overloads are load-bearing, and add a typed overload for any command the stub actually queues. The current call sites are:
intercom.service.ts:24:('boot', options)— coveredintercom.service.ts:32:('show')— coveredintercom.service.ts:41:('shutdown')— coveredintercom.service.ts:88:ic('reattach_activator')— coveredintercom.service.ts:89:ic('update', window.intercomSettings)— not covered — add(command: 'update', settings: IntercomSettings): void;
export interface IntercomFunction {
(command: 'boot', options: IntercomBootOptions): void;
(command: 'show'): void;
(command: 'shutdown'): void;
(command: 'update', settings: IntercomSettings): void;
(command: 'reattach_activator'): void;
q?: unknown[][];
c?: (args: unknown[]) => void;
}The stub's (...args: unknown[]) => void definition at L92 of intercom.service.ts already satisfies all overloads structurally (TypeScript treats the unknown-rest as assignable to each), so dropping the catch-all does not break the stub assignment. New commands added in the future force a new overload — which is what we want.
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
This pull request introduces Intercom Messenger as the primary support contact mechanism throughout the LFX One UI, replacing previous Jira Service Desk links and enabling feature-flagged, runtime-configurable support chat. It adds a reusable Angular directive to launch Intercom, updates environment and runtime configuration, and ensures all relevant UI components now use the new support mechanism.
Intercom Integration and Feature Flag
enable-intercomLaunchDarkly feature flag, using theINTERCOM_APP_IDenvironment variable and a user-specific JWT claim for secure identity verification. (apps/lfx-one/src/app/app.component.ts,apps/lfx-one/.env.example,Dockerfile,runtime-config.provider.ts) [1] [2] [3] [4] [5] [6]Reusable Intercom Launcher Directive
OpenIntercomDirective(lfxOpenIntercomattribute), which opens Intercom Messenger or falls back to the Jira Service Desk if Intercom is unavailable. This directive is now imported and used in multiple UI components. (apps/lfx-one/src/app/shared/directives/open-intercom.directive.ts)UI Updates: Replace Jira Support Links with Intercom
mailing-list-dashboard.component.html/.ts,meeting-not-found.component.html/.ts,profile-identities.component.html/.ts,lens-switcher.component.html/.ts/.ts) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Environment and Runtime Configuration
INTERCOM_APP_IDto.env.exampleand runtime config for environment-based configuration of Intercom. (apps/lfx-one/.env.example,runtime-config.provider.ts) [1] [2]These changes ensure that support requests are routed through Intercom when enabled, providing a consistent and modern user experience while maintaining a fallback to the Jira Service Desk when Intercom is unavailable.