chore: Analytics util refactor #38089
Conversation
WalkthroughThis pull request introduces significant modifications to the analytics framework within the Appsmith client application. Key changes include the addition of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
app/client/src/utils/Analytics/sentry.ts (1)
6-97: Refactor SentryUtil to use plain functions instead of static classThe
SentryUtilclass contains only static methods and no instance properties. It's advisable to refactor it into a module of plain functions to reduce complexity and improve readability.Apply this diff to convert the class into a module of functions:
-import * as Sentry from "@sentry/react"; -import { getAppsmithConfigs } from "ee/configs"; -import log from "loglevel"; -import type { User } from "constants/userConstants"; -class SentryUtil { - static init() { +export function init() { // ... existing code ... - } - public static identifyUser(userId: string, userData: User) { +export function identifyUser(userId: string, userData: User) { // ... existing code ... - } - private static extractSentryException(event: Sentry.Event) { +function extractSentryException(event: Sentry.Event) { // ... existing code ... - } -} - -export default SentryUtil;🧰 Tools
🪛 Biome (1.9.4)
[error] 5-97: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
app/client/src/utils/Analytics/smartlook.ts (1)
4-17: Refactor class with only static members into a moduleSince
SmartlookUtilcontains only static members, consider refactoring it into a module of functions.🧰 Tools
🪛 Biome (1.9.4)
[error] 3-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 12-12: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 14-14: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
app/client/src/pages/Editor/gitSync/GitSettingsModal/TabBranch/hooks.ts (1)
11-19: Consider adding a default source valueWhile error handling is good, consider defining a constant for the default "unknown" value.
- let source = "unknown"; + const DEFAULT_SOURCE = "unknown"; + let source = DEFAULT_SOURCE;app/client/src/pages/AdminSettings/index.tsx (2)
35-43: Consider adding error handling for Mixpanel operationsWhile the recording control is implemented correctly, consider adding error handling for the Mixpanel calls.
useEffect(() => { - MixpanelSingleton.getInstance().stopRecording(); + try { + MixpanelSingleton.getInstance().stopRecording(); + } catch (e) { + log.error("Failed to stop Mixpanel recording:", e); + } return () => { - MixpanelSingleton.getInstance().startRecording(); + try { + MixpanelSingleton.getInstance().startRecording(); + } catch (e) { + log.error("Failed to start Mixpanel recording:", e); + } }; }, []);
16-16: Consider using path alias for importThe relative import could be replaced with a path alias for better maintainability.
-import MixpanelSingleton from "../../utils/Analytics/mixpanel"; +import MixpanelSingleton from "@appsmith/utils/Analytics/mixpanel";app/client/src/ce/sagas/userSagas.tsx (1)
136-142: Improved error handling for analytics initializationThe addition of try-catch with proper error logging and state management is a good practice for handling third-party service initialization.
Consider adding more context to the error log:
- log.error(e); + log.error("Failed to initialize analytics:", e);app/client/src/utils/AppsmithUtils.tsx (1)
6-6: Verify the import of 'loglevel'Ensure that
loglevelsupports default imports. If not, consider importing it as a namespace:import * as log from "loglevel";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
app/client/package.json(1 hunks)app/client/public/index.html(0 hunks)app/client/src/api/interceptors/request/addAnonymousUserIdHeader.ts(1 hunks)app/client/src/ce/sagas/analyticsSaga.ts(2 hunks)app/client/src/ce/sagas/userSagas.tsx(1 hunks)app/client/src/ce/utils/Analytics/trackedUser.ts(1 hunks)app/client/src/ce/utils/AnalyticsUtil.tsx(3 hunks)app/client/src/ee/utils/Analytics/trackedUser.ts(1 hunks)app/client/src/pages/AdminSettings/index.tsx(2 hunks)app/client/src/pages/Editor/gitSync/GitSettingsModal/TabBranch/hooks.ts(1 hunks)app/client/src/utils/Analytics/mixpanel.ts(1 hunks)app/client/src/utils/Analytics/segment.ts(1 hunks)app/client/src/utils/Analytics/sentry.ts(1 hunks)app/client/src/utils/Analytics/smartlook.ts(1 hunks)app/client/src/utils/AppsmithUtils.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/public/index.html
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ee/utils/Analytics/trackedUser.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/utils/Analytics/smartlook.ts
[error] 3-17: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 12-12: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 14-14: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
app/client/src/utils/Analytics/sentry.ts
[error] 5-97: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
app/client/src/ce/utils/AnalyticsUtil.tsx
[error] 53-53: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 55-55: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 61-61: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 98-98: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 103-103: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 105-105: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 124-124: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 137-137: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 156-156: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 157-157: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 176-176: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 176-176: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (9)
app/client/src/ce/utils/Analytics/trackedUser.ts (1)
1-111: TrackedUser singleton implementation looks solid
The TrackedUser class is well-implemented, following the singleton pattern correctly. It efficiently manages user tracking and encapsulates user properties appropriately.
app/client/src/utils/Analytics/sentry.ts (1)
1-97: Sentry initialization and error handling are well-implemented
The integration with Sentry, including custom error handling and event filtering, is correctly set up and aligns with best practices.
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-97: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
app/client/src/utils/Analytics/segment.ts (1)
10-125: SegmentSingleton is correctly managing Segment integration
The singleton pattern is appropriately used, and the methods for tracking, identifying, and initializing are well-defined.
app/client/src/utils/Analytics/mixpanel.ts (1)
7-130: MixpanelSingleton effectively integrates Mixpanel analytics
The implementation correctly initializes Mixpanel, manages session recording, and integrates with Segment's middleware. The use of middleware to enrich events with session recording properties is appropriate.
app/client/src/api/interceptors/request/addAnonymousUserIdHeader.ts (1)
6-6: Ensure all callers provide the required 'anonymousId' parameter
The anonymousId parameter is now mandatory. Verify that all calls to addAnonymousUserIdHeader provide the anonymousId to prevent TypeScript errors.
Run the following script to check for any calls missing anonymousId:
✅ Verification successful
All callers correctly provide the required 'anonymousId' parameter
Based on the codebase analysis, the only usage of addAnonymousUserIdHeader is in the test file and through a wrapper function in apiRequestInterceptor.ts, both of which properly provide the anonymousId parameter:
- Test file correctly passes
anonymousId: "anonymousUserId" - Wrapper function in
apiRequestInterceptor.tspassesanonymousIdobtained fromAnalyticsUtil.getAnonymousId()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to 'addAnonymousUserIdHeader' missing the 'anonymousId' parameter.
rg -t ts "addAnonymousUserIdHeader\(\s*config\s*,\s*\{[^}]*\}(?!.*anonymousId)" -A 5
Length of output: 409
Script:
#!/bin/bash
# Let's try a different approach using ast-grep to find function calls
ast-grep --pattern 'addAnonymousUserIdHeader($_, $_)'
# Also search for the function definition to understand its usage
rg -t ts "addAnonymousUserIdHeader.*=.*\(" -A 5
Length of output: 2470
app/client/src/ce/sagas/analyticsSaga.ts (2)
14-20: LGTM! Clean interface refactoring
Good separation of concerns by removing user-related fields from AppDetails interface.
23-42: Verify all consumers of getAppDetails
The function has been correctly updated to match the new interface, but we should verify all consumers are updated.
✅ Verification successful
No consumers found requiring update
Based on the search results, getAppDetails is only used once in the codebase as a yield call in the same file (analyticsSaga.ts). The function is correctly updated to match its new interface, and there are no other consumers that need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to getUserAndAppDetails
rg "getUserAndAppDetails"
# Search for destructuring that might expect old fields
ast-grep --pattern 'const { userId, email, source } = yield call(getAppDetails)'
Length of output: 108
Script:
#!/bin/bash
# Search for calls to getAppDetails
rg "getAppDetails\(" -A 2
# Search for any saga that imports and uses getAppDetails
ast-grep --pattern 'import { getAppDetails } from'
# Search for yield call/select patterns with getAppDetails
ast-grep --pattern 'yield call(getAppDetails)'
ast-grep --pattern 'yield select(getAppDetails)'
Length of output: 66634
app/client/src/pages/Editor/gitSync/GitSettingsModal/TabBranch/hooks.ts (1)
Line range hint 21-31: LGTM! Clean URL construction
Good use of URL API and proper memoization with updated dependencies.
app/client/package.json (1)
91-91: Verify the stability of @segment/analytics-next version
The addition of this dependency aligns with the analytics refactoring objective. However, we should verify the stability of version 1.76.0.
✅ Verification successful
@segment/analytics-next version 1.76.0 is stable and up-to-date
The version is not deprecated and is currently the latest release in the npm registry.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any known issues or breaking changes in the specified version
curl -s https://registry.npmjs.org/@segment/analytics-next | jq '.versions["1.76.0"].deprecated, .versions["1.76.0"].deprecated_reason'
# Check the latest version and its release date
curl -s https://registry.npmjs.org/@segment/analytics-next/latest | jq '.version, .time'
Length of output: 252
| if (!writeKey) { | ||
| log.error("Segment key was not found."); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Return false when Segment key is not found
In the init method, when the write key is not found, the function logs an error but still returns true. This could lead to misinterpretation that Segment was initialized successfully.
Apply this diff to return false when the Segment key is missing:
if (!writeKey) {
log.error("Segment key was not found.");
- return true;
+ return false;
}📝 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.
| if (!writeKey) { | |
| log.error("Segment key was not found."); | |
| return true; | |
| } | |
| if (!writeKey) { | |
| log.error("Segment key was not found."); | |
| return false; | |
| } |
| if (mixpanel.enabled) { | ||
| this.mixpanel = loadedMixpanel; | ||
| this.mixpanel.init(mixpanel.apiKey, { | ||
| record_sessions_percent: 100, | ||
| }); | ||
|
|
||
| await this.addSegmentMiddleware(); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Adjust return value when Mixpanel is not enabled
In the init method, if mixpanel.enabled is false, Mixpanel isn't initialized, but the method still returns true. This could be misleading since initialization didn't occur.
Consider returning false when Mixpanel is not enabled:
if (mixpanel.enabled) {
this.mixpanel = loadedMixpanel;
this.mixpanel.init(mixpanel.apiKey, {
record_sessions_percent: 100,
});
await this.addSegmentMiddleware();
+ return true;
}
- return true;
+ return false;📝 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.
| if (mixpanel.enabled) { | |
| this.mixpanel = loadedMixpanel; | |
| this.mixpanel.init(mixpanel.apiKey, { | |
| record_sessions_percent: 100, | |
| }); | |
| await this.addSegmentMiddleware(); | |
| } | |
| return true; | |
| if (mixpanel.enabled) { | |
| this.mixpanel = loadedMixpanel; | |
| this.mixpanel.init(mixpanel.apiKey, { | |
| record_sessions_percent: 100, | |
| }); | |
| await this.addSegmentMiddleware(); | |
| return true; | |
| } | |
| return false; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
app/client/src/utils/Analytics/segment.ts (5)
22-26: Explicitly return null ingetUser()when analytics is uninitializedWhen
this.analyticsis not initialized,getUser()doesn't return a value. Consider returningnullexplicitly for clarity.
92-96: Log a message when tracking without initializationIf
this.analyticsis not initialized, thetrack()method silently fails. Adding a log message can help in debugging missing analytics events.
98-102: Log a message when identifying without initializationSimilar to
track(), theidentify()method does nothing ifthis.analyticsis uninitialized. Consider logging this scenario.
104-108: Log when adding middleware without initializationIn
addMiddleware(), ifthis.analyticsis not initialized, the middleware isn't added and no feedback is provided. Logging can aid in troubleshooting.
49-53: Adjust logging for already initialized SegmentLogging a warning when Segment is already initialized may not be necessary and could clutter logs. Consider using a debug log level or removing it.
app/client/src/ce/utils/AnalyticsUtil.tsx (3)
14-16: Consider expanding the AnalyticsEventType enumThe enum currently only includes 'error'. Consider adding other common event types like 'page_view', 'user_action', etc., for better event categorization.
18-20: Consider using a configuration object patternGlobal variables for state management could lead to issues in testing and maintainability. Consider encapsulating these in a configuration object.
-let instanceId = ""; -let blockErrorLogs = false; -let segmentAnalytics: SegmentSingleton | null = null; +const config = { + instanceId: "", + blockErrorLogs: false, + segmentAnalytics: null as SegmentSingleton | null, +};
54-76: Improve type safety in logEvent functionConsider adding runtime type checks for eventData to prevent potential issues.
function logEvent( eventName: EventName, eventData?: EventProperties, eventType?: AnalyticsEventType, ) { + if (eventData && typeof eventData !== 'object') { + log.warn('Invalid eventData type provided'); + return; + } if (blockErrorLogs && eventType === AnalyticsEventType.error) { return; } // ... rest of the function }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/ce/utils/AnalyticsUtil.tsx(1 hunks)app/client/src/utils/Analytics/segment.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/ce/utils/AnalyticsUtil.tsx
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
app/client/src/utils/Analytics/segment.ts (1)
57-61: Return false when Segment key is not found
In the init method, returning true when the writeKey is missing could indicate successful initialization, which is misleading.
app/client/src/ce/utils/AnalyticsUtil.tsx (3)
22-34:
Add error handling to initialization sequence
The initialization sequence should handle failures gracefully to prevent cascading issues.
async function initialize(user: User) {
+ try {
SentryUtil.init();
await SmartlookUtil.init();
segmentAnalytics = SegmentSingleton.getInstance();
await segmentAnalytics.init();
await MixpanelSingleton.getInstance().init();
await identifyUser(user);
+ } catch (error) {
+ log.error("Failed to initialize analytics:", error);
+ // Consider implementing a fallback strategy
+ }
}Likely invalid or redundant comment.
136-146:
Address TODO and improve type safety in reset function
The function needs proper typing and optional chaining.
function reset() {
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- const windowDoc: any = window;
+ interface CustomWindow extends Window {
+ Intercom?: (command: string) => void;
+ }
+ const windowDoc = window as CustomWindow;
if (windowDoc.Intercom) {
windowDoc.Intercom("shutdown");
}
- segmentAnalytics && segmentAnalytics.reset();
+ segmentAnalytics?.reset();
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
78-112:
Add error handling for analytics service calls
Failures in individual analytics services should not affect others.
async function identifyUser(userData: User, sendAdditionalData?: boolean) {
+ const handleError = (service: string, error: Error) => {
+ log.error(`Failed to identify user in ${service}:`, error);
+ };
// ... existing code ...
if (segmentAnalytics) {
+ try {
await segmentAnalytics.identify(trackedUser.userId, userProperties);
+ } catch (error) {
+ handleError('Segment', error);
+ }
}
+ try {
SentryUtil.identifyUser(trackedUser.userId, userData);
+ } catch (error) {
+ handleError('Sentry', error);
+ }
if (trackedUser.email) {
+ try {
SmartlookUtil.identify(trackedUser.userId, trackedUser.email);
+ } catch (error) {
+ handleError('Smartlook', error);
+ }
}
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/client/src/ce/utils/AnalyticsUtil.tsx (2)
19-21: Consider encapsulating global variablesThese variables could be better encapsulated within a configuration class or the AnalyticsUtil class itself to prevent global state mutations.
-let instanceId = ""; -let blockErrorLogs = false; -let segmentAnalytics: SegmentSingleton | null = null; +class AnalyticsConfig { + private static instance: AnalyticsConfig; + private _instanceId = ""; + private _blockErrorLogs = false; + private _segmentAnalytics: SegmentSingleton | null = null; + + static getInstance(): AnalyticsConfig { + if (!AnalyticsConfig.instance) { + AnalyticsConfig.instance = new AnalyticsConfig(); + } + return AnalyticsConfig.instance; + } + + // Add getters and setters +}
23-41: Enhance error handling specificityThe error handling is good, but consider handling service-specific failures separately to provide more detailed error messages and potential recovery options.
async function initialize(user: User) { try { SentryUtil.init(); + } catch (e) { + log.error("Sentry initialization failed", e); + } + + try { await SmartlookUtil.init(); - + } catch (e) { + log.error("Smartlook initialization failed", e); + } + + try { segmentAnalytics = SegmentSingleton.getInstance(); - await segmentAnalytics.init(); - + } catch (e) { + log.error("Segment initialization failed", e); + } + + try { // Mixpanel needs to be initialized after Segment await MixpanelSingleton.getInstance().init(); - + } catch (e) { + log.error("Mixpanel initialization failed", e); + } + + try { // Identify the user after all services are initialized await identifyUser(user); } catch (e) { - log.error("Error initializing analytics", e); - toast.show("Error initializing analytics"); + log.error("User identification failed", e); + toast.show("Error identifying user"); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/ce/utils/AnalyticsUtil.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/ce/utils/AnalyticsUtil.tsx
[error] 151-151: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
app/client/src/ce/utils/AnalyticsUtil.tsx (2)
142-152: Clean up TODO and improve type safety
The reset function needs cleanup and type safety improvements.
🧰 Tools
🪛 Biome (1.9.4)
[error] 151-151: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
154-163: LGTM! Clean and well-organized exports
The exports are well-structured and include all necessary functions.
| const finalEventData = { | ||
| ...eventData, | ||
| ...getEventExtraProperties(), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for event data merging
The spread operator merging of eventData with extra properties lacks type safety. Consider adding proper type definitions.
+interface ExtraProperties {
+ instanceId: string;
+ version: string;
+ userData: Record<string, unknown>;
+}
const finalEventData = {
- ...eventData,
- ...getEventExtraProperties(),
+ ...(eventData as EventProperties),
+ ...(getEventExtraProperties() as ExtraProperties),
} as const;Committable suggestion skipped: line range outside the PR's diff.
|
This PR has increased the number of cyclic dependencies by 296, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
This PR has increased the number of cyclic dependencies by 296, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
This PR has increased the number of cyclic dependencies by 297, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/ce/utils/AnalyticsUtil.tsx (1)
52-59: Use optional chaining for safer null handling.The null check can be simplified using optional chaining.
- if (segmentAnalytics) { - segmentAnalytics.track(eventName, finalEventData); - } + segmentAnalytics?.track(eventName, finalEventData);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/ce/sagas/userSagas.tsx(2 hunks)app/client/src/ce/utils/AnalyticsUtil.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/sagas/userSagas.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/ce/utils/AnalyticsUtil.tsx
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
app/client/src/ce/utils/AnalyticsUtil.tsx (5)
1-19: LGTM! Well-organized imports with clear separation of concerns.
The modular approach with separate analytics service imports and proper type definitions enhances maintainability.
129-139: LGTM! Well-organized exports.
All necessary functions are properly exported in a single, organized statement.
28-41: Verify initialization order dependencies.
The initialization sequence appears correct, but let's verify the dependency between Segment and Mixpanel initialization.
✅ Verification successful
Initialization order is correctly implemented
The dependency between Segment and Mixpanel is verified and properly handled:
- Mixpanel requires Segment for its middleware integration
- The code adds Mixpanel's session recording properties to Segment events
- Clear documentation links confirm this integration pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or comments about initialization order requirements
rg -l "mixpanel.*segment|segment.*mixpanel" --type ts
Length of output: 98
Script:
#!/bin/bash
# Let's check the content of the mixpanel file to understand the dependency
cat app/client/src/utils/Analytics/mixpanel.ts
Length of output: 3572
82-96: 🛠️ Refactor suggestion
Add error handling for analytics service updates.
Consider adding try-catch blocks around service updates to handle potential failures gracefully.
+ try {
if (segmentAnalytics) {
const userProperties = {
...trackedUser,
...(sendAdditionalData ? additionalData : {}),
};
log.debug("Identify User " + trackedUser.userId);
await segmentAnalytics.identify(trackedUser.userId, userProperties);
}
SentryUtil.identifyUser(trackedUser.userId, userData);
if (trackedUser.email) {
SmartlookUtil.identify(trackedUser.userId, trackedUser.email);
}
+ } catch (error) {
+ log.error("Failed to update analytics services:", error);
+ }Likely invalid or redundant comment.
117-126: 🛠️ Refactor suggestion
Address TODO and improve type safety.
Multiple improvements needed:
- The TODO comment should be addressed
- Window type can be improved
- Use optional chaining as suggested by static analysis
function reset() {
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- const windowDoc: any = window;
+ interface CustomWindow extends Window {
+ Intercom?: (command: string) => void;
+ }
+ const windowDoc = window as CustomWindow;
if (windowDoc.Intercom) {
windowDoc.Intercom("shutdown");
}
- segmentAnalytics && segmentAnalytics.reset();
+ segmentAnalytics?.reset();
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
This PR has increased the number of cyclic dependencies by 297, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/ce/utils/Analytics/trackedUser.ts (1)
78-92: Consider injecting location dependencyThe direct usage of
window.locationmakes the code harder to test and less portable. Consider passing the location as a parameter.- public getEventUserProperties() { + public getEventUserProperties(location: Location = window.location) { const { email, userId } = this.getUser(); if (this.userId === ANONYMOUS_USERNAME) { return undefined; } - const appId = getApplicationId(window.location); + const appId = getApplicationId(location);app/client/src/ce/utils/AnalyticsUtil.tsx (1)
103-115: Simplify string manipulation for localStorage valueThe string manipulation for the localStorage value can be simplified using JSON.parse.
} else if (segment.enabled) { - return localStorage.getItem("ajs_anonymous_id")?.replaceAll('"', ""); + const storedId = localStorage.getItem("ajs_anonymous_id"); + return storedId ? JSON.parse(storedId) : null; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/ce/utils/Analytics/trackedUser.ts(1 hunks)app/client/src/ce/utils/AnalyticsUtil.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/ce/utils/AnalyticsUtil.tsx
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
app/client/src/ce/utils/Analytics/trackedUser.ts (1)
31-48: LGTM! Secure user ID generation and proper singleton implementation.
The implementation correctly handles both self-hosted and cloud scenarios, with secure user ID generation using SHA-256 for self-hosted users.
app/client/src/ce/utils/AnalyticsUtil.tsx (2)
117-127: 🛠️ Refactor suggestion
Address TODO and improve type safety
The reset function needs several improvements:
- The TODO comment should be addressed
- Window type can be properly typed
- Optional chaining would be clearer
function reset() {
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- const windowDoc: any = window;
+ interface CustomWindow extends Window {
+ Intercom?: (command: string) => void;
+ }
+ const windowDoc = window as CustomWindow;
if (windowDoc.Intercom) {
windowDoc.Intercom("shutdown");
}
- segmentAnalytics && segmentAnalytics.reset();
+ segmentAnalytics?.reset();
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
28-41: 🛠️ Refactor suggestion
Add error handling and optimize initialization
The sequential initialization of services could impact performance. Consider parallel initialization where possible and add proper error handling.
async function initialize(user: User) {
+ try {
+ const [segmentInstance, smartlookInit] = await Promise.all([
+ SegmentSingleton.getInstance(),
+ SmartlookUtil.init(),
+ ]);
SentryUtil.init();
- await SmartlookUtil.init();
- segmentAnalytics = SegmentSingleton.getInstance();
+ segmentAnalytics = segmentInstance;
await segmentAnalytics.init();
await MixpanelSingleton.getInstance().init();
await identifyUser(user);
+ } catch (error) {
+ log.error("Failed to initialize analytics:", error);
+ // Consider fallback strategy
+ }
}Likely invalid or redundant comment.
|
This PR has increased the number of cyclic dependencies by 293, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Description
Break the AnalyticsUtil monolith into smaller parts for easier management and readability
Mixpanel Session recordings
As we move from smartlook to mixpanel gradually, made some changes to have it work on CE instances as well
Also check out companion EE PR to check how the classes are extended
https://github.com/appsmithorg/appsmith-ee/pull/5727
Fixes #38122
Fixes #38142
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12407787035
Commit: 87d3493
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 19 Dec 2024 07:38:52 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements
Bug Fixes