fix: Analytics identify user not called correctly #37303
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Tool Failures:Tool Failure Count: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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/ce/sagas/userSagas.tsx(2 hunks)app/client/src/utils/AppsmithUtils.tsx(2 hunks)
🔇 Additional comments (5)
app/client/src/utils/AppsmithUtils.tsx (3)
16-17: LGTM: Required imports added correctly
The new imports for User type and ANONYMOUS_USERNAME constant are properly placed and necessary for the enhanced user identification functionality.
112-114: LGTM: User identification logic is well implemented
The implementation correctly:
- Filters out anonymous users using both isAnonymous flag and username check
- Uses proper async/await pattern
- Is placed appropriately after analytics initialization
19-19: Verify all callers are updated with the new parameter
The function signature change is a breaking change that requires updates to all calling code.
Let's verify all callers are updated:
✅ Verification successful
Function signature change is properly handled
The only caller of initializeAnalyticsAndTrackers in app/client/src/ce/sagas/userSagas.tsx correctly passes the currentUser parameter, which is available in the enclosing initTrackers function. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to initializeAnalyticsAndTrackers
rg "initializeAnalyticsAndTrackers\(" --type ts --type tsx
Length of output: 88
Script:
#!/bin/bash
# Search for all calls to initializeAnalyticsAndTrackers using ripgrep without file type filters
rg "initializeAnalyticsAndTrackers\("
# Also search for the function definition to understand its location and usage
rg "export const initializeAnalyticsAndTrackers"
Length of output: 316
Script:
#!/bin/bash
# Let's check the context of the caller to verify if it's passing the currentUser parameter correctly
rg "initializeAnalyticsAndTrackers\(" -B 5 -A 5
Length of output: 755
app/client/src/ce/sagas/userSagas.tsx (2)
210-210: LGTM! Correct usage of fork for non-blocking analytics initialization.
The change properly ensures analytics initialization doesn't block the main flow.
Line range hint 192-210: Verify complete removal of SmartLook initialization.
Let's ensure there are no remaining references to the old analytics initialization.
| function* initTrackers(currentUser: User) { | ||
| const initializeSentry = initializeAnalyticsAndTrackers(currentUser); | ||
|
|
||
| const sentryInitialized: boolean = yield initializeSentry; | ||
|
|
||
| if (sentryInitialized) { | ||
| yield put(segmentInitSuccess()); | ||
| } else { | ||
| yield put(segmentInitUncertain()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve type safety.
The function should handle potential initialization errors and have proper type checking:
-function* initTrackers(currentUser: User) {
+function* initTrackers(currentUser: User) {
+ try {
const initializeSentry = initializeAnalyticsAndTrackers(currentUser);
- const sentryInitialized: boolean = yield initializeSentry;
+ const sentryInitialized: boolean = yield Promise.resolve(initializeSentry);
if (sentryInitialized) {
yield put(segmentInitSuccess());
} else {
yield put(segmentInitUncertain());
}
+ } catch (error) {
+ yield put(segmentInitUncertain());
+ Sentry.captureException("Analytics initialization failed", {
+ level: Severity.Error,
+ extra: { error },
+ });
+ }
}Also, consider renaming the function to better reflect its responsibility (e.g., initSentryAnalytics).
Committable suggestion skipped: line range outside the PR's diff.
## Description Fix the bug where return was called before the await for cloud conditions. This made the identifyUser case being skipped almost always Missed in #37303 ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/11791461362> > Commit: 94bc989 > Workflow: `PR Automation test suite` > Tags: `@tag.Sanity` > Spec: `` > <hr>Tue, 12 Nov 2024 05:44:55 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced analytics initialization process to ensure completion before user identification. - **Bug Fixes** - Improved error handling for analytics setup with Sentry logging. - **Documentation** - Updated function signatures for clarity and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description Update the call sequence during init to solve for analytics issue. It was found that the `identifyUser` in analytics was not called properly when in edit mode. This change will fix the call sequence. It also ensures that the init of analytics is not a blocker by forking out the call into a different generator function ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11742239861> > Commit: 984e325 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11742239861&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Fri, 08 Nov 2024 13:16:33 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced analytics tracking capabilities by integrating user identification into the analytics setup process. - Simplified the initialization of analytics trackers for improved performance. - **Bug Fixes** - Improved error handling for analytics initialization and user tracking. - **Documentation** - Updated comments for clarity regarding the new analytics tracking logic. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description Fix the bug where return was called before the await for cloud conditions. This made the identifyUser case being skipped almost always Missed in appsmithorg#37303 ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/11791461362> > Commit: 94bc989 > Workflow: `PR Automation test suite` > Tags: `@tag.Sanity` > Spec: `` > <hr>Tue, 12 Nov 2024 05:44:55 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced analytics initialization process to ensure completion before user identification. - **Bug Fixes** - Improved error handling for analytics setup with Sentry logging. - **Documentation** - Updated function signatures for clarity and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Update the call sequence during init to solve for analytics issue. It was found that the
identifyUserin analytics was not called properly when in edit mode. This change will fix the call sequence. It also ensures that the init of analytics is not a blocker by forking out the call into a different generator functionAutomation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11742239861
Commit: 984e325
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 08 Nov 2024 13:16:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation