feat(KFLUXUI-1048/49): Segment config and analytics initialisation for Active users telemetry#694
feat(KFLUXUI-1048/49): Segment config and analytics initialisation for Active users telemetry#694milantaky wants to merge 16 commits intokonflux-ci:mainfrom
Conversation
Assisted-by: Cursor
Assisted-by: Cursor
Assisted-by: Cursor
Assisted-by: Cursor
Assisted-by: Cursor
milantaky
requested review from
JoaoPedroPP,
StanislavJochman,
abhinandan13jan,
janaki29,
marcin-michal,
rakshett,
rrosatti,
sahil143 and
testcara
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
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:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Type Definitions & Runtime @types/index.d.ts, public/runtime-config.js |
Add ANALYTICS_ENABLED, ANALYTICS_WRITE_KEY, ANALYTICS_API_URL to Window.KONFLUX_RUNTIME and provide runtime defaults (ANALYTICS_ENABLED='false', empty key/url). |
Analytics Types & Loader src/analytics/types.ts, src/analytics/load-config.ts |
Introduce AnalyticsConfig and loadAnalyticsConfig() which parses KONFLUX_RUNTIME, returns disabled defaults when unset/false, and provides writeKey/apiUrl when enabled. |
Analytics Core & Tests src/analytics/index.ts, src/analytics/__tests__/* |
New lazy analytics module with initAnalytics() and getAnalytics() (validates config, dynamic import of @segment/analytics-next, loads AnalyticsBrowser, stores instance, logs/errors reported). Add unit tests covering config parsing, init paths, trimming, errors, and getAnalytics behavior. |
Integration & Dependency package.json, src/main.tsx |
Add dependency @segment/analytics-next. Initialize analytics in parallel with monitoring via Promise.allSettled, logging and reporting per-initializer failures. |
Sequence Diagram(s)
sequenceDiagram
participant App as App (main.tsx)
participant Analytics as Analytics Module
participant Config as Config Loader
participant Segment as Segment SDK
participant Monitoring as Monitoring Service
App->>Analytics: initAnalytics()
activate Analytics
Analytics->>Config: loadAnalyticsConfig()
activate Config
Config->>Config: read window.KONFLUX_RUNTIME, parse booleans
Config-->>Analytics: AnalyticsConfig
deactivate Config
Analytics->>Analytics: validate enabled, writeKey, apiUrl
alt enabled && valid
Analytics->>Segment: dynamic import `@segment/analytics-next`
Analytics->>Segment: AnalyticsBrowser.load(writeKey, { host: apiUrl })
Segment-->>Analytics: analyticsInstance
Analytics-->>App: resolved (instance stored)
else disabled or invalid
Analytics-->>App: skip (undefined)
else error
Analytics->>Monitoring: captureException / captureMessage
Analytics-->>App: handled (undefined)
end
deactivate Analytics
App->>App: render React app
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
A tiny key, a little URL bright,
Lazy imports wake the analytics light.
Configs trimmed tidy, errors caught with care,
Monitoring listens — the data will share.
Cheers to init in parallel flair 🎉
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title accurately reflects the main objective: implementing Segment analytics configuration and initialization for active users telemetry, directly addressing KFLUXUI-1048/49. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
![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
🤖 Fix all issues with AI agents
In `@src/analytics/index.ts`:
- Around line 43-48: The catch block in initAnalytics (or the module-level error
handler in src/analytics/index.ts) is sending duplicate Sentry events by calling
monitoringService.captureException(error, { context: 'initAnalytics' }) and then
monitoringService.captureMessage('Error loading Analytics', 'error', { error });
remove the redundant monitoringService.captureMessage call so only
captureException is sent for the same failure, keeping the console.error and
captureException call intact to preserve error logging and context.
- Around line 23-27: The code reads config.apiUrl into apiHost and passes it as
a top-level option, but Segment requires apiHost under
integrations['Segment.io'] and protocol set separately; update the
AnalyticsBrowser.load call to pass { writeKey } plus an options object with
integrations: { 'Segment.io': { apiHost: <host+path without scheme>, protocol:
<scheme> } }; add logic in the config handling around config.apiUrl
(ANALYTICS_API_URL) to accept full URLs by parsing and stripping the scheme into
protocol and host/path (or validate that callers supply host+path only), and
then use those parsed values when calling AnalyticsBrowser.load (referencing
writeKey, apiHost, protocol, AnalyticsBrowser.load, and
integrations['Segment.io']).
In `@src/analytics/load-config.ts`:
- Around line 4-5: The JSDoc comment above the default analytics configuration
mistakenly says "monitoring"; update the comment text to refer to "analytics"
instead (i.e., change "Default monitoring configuration" to "Default analytics
configuration") on the JSDoc that documents the module's exported default
analytics config in load-config.ts so the comment accurately describes the
exported analytics configuration object.
🧹 Nitpick comments (4)
src/analytics/load-config.ts (1)
34-38: Consider trimmingwriteKeyandapiUrlhere rather than relying on the caller.
initAnalytics()insrc/analytics/index.ts(lines 23-24) trims these values before use. Moving the.trim()into the config loader would make the returned config consistently clean and reduce burden on every future consumer.Optional diff
return { enabled: true, - writeKey: runtime.ANALYTICS_WRITE_KEY || '', - apiUrl: runtime.ANALYTICS_API_URL || '', + writeKey: (runtime.ANALYTICS_WRITE_KEY || '').trim(), + apiUrl: (runtime.ANALYTICS_API_URL || '').trim(), };src/analytics/index.ts (1)
41-42:eslint-disablecomments suppressno-console— consider a proper logging approach.Per project conventions, suppressing lint rules should be avoided in favor of fixing the underlying issue. If the project has a logging utility or plans for structured logging, these should use that instead. If
consoleis the accepted approach for pre-app-init diagnostics, consider adding a project-level ESLint override for this analytics module rather than per-line disables.Based on learnings: "In the konflux-ci/konflux-ui repository, if code contains comments that suppress lint warnings or errors (e.g., eslint-disable, ts-ignore, ts-expect-error), remind contributors to fix the underlying issue rather than silencing the rule."
Also applies to: 44-45
src/main.tsx (1)
51-61: Index-based labeling is fragile.The
i === 0 ? 'monitoring' : 'analytics'mapping will silently produce wrong labels if the array order changes. Consider a more explicit approach:Suggested refactor
- const results = await Promise.allSettled([initMonitoring(), initAnalytics()]); - results.forEach((result, i) => { - if (result.status === 'rejected') { - const name = i === 0 ? 'monitoring' : 'analytics'; - // eslint-disable-next-line no-console - console.error(`Failed to initialize ${name}`, result.reason); - monitoringService?.captureException(result.reason, { - context: i === 0 ? 'initMonitoring' : 'initAnalytics', - }); - } - }); + const initializers = [ + { name: 'monitoring', context: 'initMonitoring', fn: initMonitoring }, + { name: 'analytics', context: 'initAnalytics', fn: initAnalytics }, + ] as const; + + const results = await Promise.allSettled(initializers.map(({ fn }) => fn())); + results.forEach((result, i) => { + if (result.status === 'rejected') { + const { name, context } = initializers[i]; + // eslint-disable-next-line no-console + console.error(`Failed to initialize ${name}`, result.reason); + monitoringService?.captureException(result.reason, { context }); + } + });src/analytics/__tests__/index.spec.ts (1)
120-133: Test name slightly misleading —apiUrlis also empty here.The test named "should not load SDK when write key is missing" also has
apiUrl: '', so it tests a combined condition. Consider either providing a validapiUrlto isolate the writeKey check, or renaming the test.Suggested fix to isolate the writeKey condition
it('should not load SDK when write key is missing', async () => { const mockConfig: AnalyticsConfig = { enabled: true, writeKey: '', - apiUrl: '', + apiUrl: 'https://api.segment.io/v1', };Same applies to the whitespace writeKey test on lines 135-148.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/analytics/index.ts`:
- Around line 50-53: In the catch block that currently logs errors and calls
monitoringService.captureMessage('Error loading Analytics', 'error', { error }),
replace the lower-fidelity captureMessage call with
monitoringService.captureException(error) so the full error object, stack trace
and type are sent to Sentry; keep the existing console.error line, and remove or
stop calling captureMessage for this error to avoid redundancy (refer to the
catch handler and the monitoringService.captureMessage / captureException
methods).
🧹 Nitpick comments (2)
src/analytics/index.ts (1)
48-49: Consider addressing the lint suppression rather than silencingno-console.Two
eslint-disable-next-line no-consolecomments are used here. If the project has a logging utility or ifconsole.info/console.errorare intentional for this module, consider configuring the ESLint rule to allow them in this file (or using a dedicated logger) instead of inline suppressions. Based on learnings, contributors should fix the underlying issue rather than silencing the rule.Also applies to: 51-52
src/analytics/__tests__/index.spec.ts (1)
179-200: Update error-handling assertions ifcaptureExceptionreplacescaptureMessagein the implementation.If the implementation is updated per the suggestion on
src/analytics/index.ts(Line 53), this test should assertcaptureExceptioninstead ofcaptureMessage.Additionally, consider adding a test for double initialization — calling
initAnalytics()twice — to verify idempotency. Currently,analyticsInstancewould be silently overwritten on a second call; an explicit test documents whether that's the intended behavior.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (72.65%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #694 +/- ##
==========================================
- Coverage 87.23% 87.14% -0.10%
==========================================
Files 764 767 +3
Lines 58225 58333 +108
Branches 5658 6893 +1235
==========================================
+ Hits 50795 50835 +40
+ Misses 7376 7370 -6
- Partials 54 128 +74
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 65 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
src/main.tsx
Outdated
| const results = await Promise.allSettled([initMonitoring(), initAnalytics()]); | ||
| results.forEach((result, i) => { | ||
| if (result.status === 'rejected') { | ||
| const name = i === 0 ? 'monitoring' : 'analytics'; |
There was a problem hiding this comment.
I think that it might get complicated if we start adding more initializers in the future, how about something like this ->
const initializers = [
{ name: 'monitoring', context: 'initMonitoring', init: initMonitoring },
{ name: 'analytics', context: 'initAnalytics', init: initAnalytics },
] as const;
const results = await Promise.allSettled(initializers.map(({ init }) => init()));
results.forEach((result, i) => {
if (result.status === 'rejected') {
const { name, context } = initializers[i];
// eslint-disable-next-line no-console
console.error(`Failed to initialize ${name}`, result.reason);
monitoringService?.captureException(result.reason, { context });
}
});so in the feature, if a new initializer needs to be added, we'll just have to update the initializers array. Let me know what you guys think :)
There was a problem hiding this comment.
Nice catch, thank you! Updated
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main.tsx`:
- Around line 51-65: The concurrent startup allows initAnalytics to hit its
internal catch before monitoringService is set; change startup to run
initMonitoring first then start the rest: call await initMonitoring() (the
initMonitoring function) and only after it resolves build/run Promise.allSettled
for the remaining initializers (e.g., initAnalytics) so monitoringService is
initialized before any analytics error handling runs; update the code that
currently maps initializers (and the results loop) to exclude the already-run
monitoring initializer.
JoaoPedroPP
left a comment
JoaoPedroPP
left a comment
There was a problem hiding this comment.
Tested locally and the output match to what was described in the How to test or reproduce section. What are next step on this?
src/main.tsx
Outdated
| { name: 'analytics', context: 'initAnalytics', init: initAnalytics }, | ||
| ] as const; | ||
|
|
||
| void (await Promise.allSettled(initializers.map(({ init }) => init())).then((results) => { |
There was a problem hiding this comment.
I believe we don't need the await here, you can call it as void Promise.allSettled(...) :)
|
@JoaoPedroPP I believe that at this point, this should be sufficient testing for the initialisation. Seeing the (failed) API call should be correct now:) |
4 participants
Fixes
https://issues.redhat.com/browse/KFLUXUI-1048
https://issues.redhat.com/browse/KFLUXUI-1049
Description
@segment/analytics-nextpackageAnalytics initialisationfunction -> non-blocking, dynamically loadedType of change
Screen shots / Gifs for design review
How to test or reproduce?
In
src/analytics/index.tscomment out line 21 and addconst config = { enabled: true, writeKey: 'test-write-key-123', apiUrl: 'https://api.segment.io/v1', };to see failed api call in the console (returns 404 now, because no proper api, and write-key is provided).
Browser conformance:
Summary by CodeRabbit