feat(intercom): add diagnostic logging for bot boot flow (LFXV2-2024)#820
Conversation
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds ordered runtime guards and logging to Intercom boot on the client (service and component) and emits SSR-side Intercom context during server request handling. ChangesIntercom boot logging enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-820.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
There was a problem hiding this comment.
Pull request overview
This PR improves observability for the Intercom “bot boot” flow across SSR (Express) and the Angular client by adding additional diagnostics around configuration presence, identity/JWT availability, and script load outcomes.
Changes:
- Added SSR debug logging to capture whether Intercom-related inputs (app id / jwt / user id) are present for each request.
- Added client-side console diagnostics when Intercom boot is skipped and when a boot command is dispatched.
- Added additional IntercomService diagnostics around boot invocation and widget script load success/failure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/lfx-one/src/server/server.ts | Adds SSR debug log for resolved Intercom runtime/auth context. |
| apps/lfx-one/src/app/shared/services/intercom.service.ts | Adds more explicit client logging around boot calls and script load outcomes. |
| apps/lfx-one/src/app/app.component.ts | Adds client diagnostics when boot is skipped or dispatched, including presence checks for required fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
Security Review — LFX Security Engineer Pass ✅
Ran a full Phase 1 + Phase 2 security scan against the 3 changed files. All checks passed:
- Secrets/credentials: No hardcoded API keys, tokens, or credentials —
INTERCOM_APP_IDconsumed viaprocess.env. - PII/sensitive data in logs: Every new log statement emits boolean presence flags (
!!value) only — raw JWT, email, userId, or username values are never logged to the browser console or server log stream. This is the correct approach. - Injection — querySelector interpolation:
scriptSrcis built fromappId, which flows fromprocess.env['INTERCOM_APP_ID'](trusted config, not user-controlled). No injection vector. - No SQL/command injection, no XSS (
innerHTML) patterns, no auth bypass.
AI Bot Reconciliation
Copilot left 3 inline comments. All three are resolved:
| Copilot Comment | Status | Resolving Commit |
|---|---|---|
#1 — isBootRequested log wording was misleading |
✅ Resolved | a13ffdc6 — message now reads 'boot ignored — already requested' |
#2 — duplicate <script> tags on retry after load failure |
✅ Resolved | f9059b1b — added scriptElement tracking + dedup guard; onerror removes the element and nulls the ref, allowing a clean retry |
#3 — snake_case vs camelCase key naming inconsistency |
✅ Resolved | a13ffdc6 — app.component.ts client logs now uniformly use camelCase; server.ts uses snake_case per the server-side logging convention in .claude/rules/logging-patterns.md |
CodeRabbit: No actionable comments generated. 🎉
Findings
Minor (1)
See inline comment on server.ts:324.
Nits (2)
One inline on intercom.service.ts:70. One note here (not on a diff line):
[nit]
app.component.ts:116— The pre-existing warn at line 116 ('Intercom boot skipped: App ID present but missing identity') uses a slightly different message prefix style than the two new logs added in this PR ('Intercom: boot skipped — …','Intercom: dispatching boot'). Not introduced here, so not a blocker — but if message-grep consistency matters for log filtering, worth a follow-up align in a cleanup commit.
Summary
Hey Audi 👋 — clean, purposeful PR. The diagnostic scaffolding is exactly right: every log emits boolean presence flags rather than raw values, the server-side log uses the logger service correctly (right method, right signature, right log level for a request-scoped debug trace), and the retry hardening in IntercomService is a genuine improvement on top of the logging. All three Copilot findings were addressed before review — well done iterating before asking for approval. One minor note on log volume (inline below), two nits, zero blockers.
0 blocking issues · 1 minor · 2 nits
Decision: ✅ Approved with minor comments
| intercomAppId: process.env['INTERCOM_APP_ID'] || '', | ||
| }; | ||
|
|
||
| logger.debug(req, 'intercom_ssr_context', 'Intercom SSR inputs resolved', { |
There was a problem hiding this comment.
[minor] This logger.debug fires on every request that hits the SSR catch-all (/**), not just Intercom-relevant ones. At DEBUG level it is gated in production, so the log-volume impact is low — but it is worth deciding upfront whether this is meant to be permanent observability (fine as-is, no action needed) or temporary diagnostic scaffolding to triage the current boot issue (in which case a follow-up PR should remove or gate it once the root cause is confirmed). A brief comment in the code — // TODO(LFXV2-2024): remove once boot-flow is stable — would make intent clear for the next reviewer.
| const scriptSrc = `https://widget.intercom.io/widget/${appId}`; | ||
| if (this.scriptElement?.isConnected || document.querySelector(`script[src="${scriptSrc}"]`)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[nit] The new dedup + cleanup logic here (lines 67–70: scriptElement.isConnected guard, document.querySelector fallback, script.remove() + null-reset in onerror) is actual behavior change, not just logging. The retry path was verified manually — onerror resets isBootRequested, removes the script element, and nulls the ref, so a subsequent boot() call passes the guard and re-injects cleanly. Correct. No automated test coverage, but the repo is E2E-only with no unit-test surface for services, so this is expected and acceptable. Flagging for awareness if a unit-test layer is ever introduced.
🧹 Deployment RemovedThe deployment for PR #820 has been removed. |
Summary
This pull request improves observability and error handling for the Intercom integration in both the frontend (
AppComponent,IntercomService) and backend (server.ts) of the application. The main focus is on adding detailed logging to help diagnose issues with Intercom widget initialization and script loading.Improved logging and diagnostics for Intercom integration:
AppComponentto indicate when Intercom boot is skipped due to missing configuration, and to log the presence of key user and JWT fields when dispatching the boot command. [1] [2]IntercomServicewith:app_id, and info logs when boot is ignored due to an existing session or when the boot command is dispatched.Backend observability:
server.tsto record the resolved Intercom SSR context, including presence of app ID, JWT, user ID, and authentication/impersonation status for each request.