-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: fixing usage pulse for anon user #37940
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🔇 Additional comments (5)app/client/src/ce/sagas/userSagas.tsx (3)
The refactoring improves code organization by separating tracking logic into its own function while maintaining existing telemetry initialization. Line range hint The function waits for feature flags but doesn't handle the failure case. If feature flags fetch fails, the saga might be stuck. Consider adding error case handling: if (!isFFFetched) {
yield call(fetchFeatureFlagsInit);
- yield take(ReduxActionTypes.FETCH_FEATURE_FLAGS_SUCCESS);
+ const result = yield take([
+ ReduxActionTypes.FETCH_FEATURE_FLAGS_SUCCESS,
+ ReduxActionTypes.FETCH_FEATURE_FLAGS_ERROR,
+ ]);
+ if (result.type === ReduxActionTypes.FETCH_FEATURE_FLAGS_ERROR) {
+ // Handle error case
+ return;
+ }
}
The code waits for editor or viewer initialization, but there might be other valid initialization paths. ✅ Verification successfulLet me search for initialization-related code to better understand the flow. Let me check the actual initialization flows to understand where these actions are dispatched. The initialization paths for anonymous users are correctly handled The code appropriately waits for either editor or viewer initialization. From the codebase analysis, these are the only two valid initialization paths:
Both paths are properly covered in the current implementation. No other initialization paths exist that would need to be considered for anonymous users. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other initialization actions
rg "INITIALIZE_.*SUCCESS" -t ts
Length of output: 1704 Script: #!/bin/bash
# Search for initialization saga implementations
ast-grep --pattern 'function* initialize$_($$$) {
$$$
}'
# Search for initialization action types
rg "INITIALIZE_" app/client/src/ce/constants/ReduxActionConstants.tsx -A 1
Length of output: 590 Script: #!/bin/bash
# Search for where initialization actions are dispatched
rg "INITIALIZE_EDITOR|INITIALIZE_PAGE_VIEWER" -A 5 -B 5
# Look for any other initialization-related sagas
ast-grep --pattern 'function* $_(action: InitializeEditorPayload) {
$$$
}'
Length of output: 14983 app/client/src/ce/sagas/NavigationSagas.ts (2)
The addition of Also applies to: 7-10
The addition of initialization checks is a good solution for ensuring proper application state before URL tracking. However, please verify the behavior when multiple initialization events occur. Let's check for potential multiple initialization events: ✅ Verification successfulThe previous search didn't yield useful results. Let's try with more targeted ripgrep commands to find:
This will help us understand the initialization flow and potential for multiple events. Multiple initialization events are handled correctly - no race condition risk The code is well-structured to handle initialization events. The
The current implementation ensures proper sequencing without risk of duplicate processing. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for dispatch of initialization actions
rg "dispatch.*(?:INITIALIZE_EDITOR_SUCCESS|INITIALIZE_PAGE_VIEWER_SUCCESS)" -A 3
# Search for any other watchers of these actions
ast-grep --pattern 'yield take($$$INITIALIZE_EDITOR_SUCCESS$$$)'
ast-grep --pattern 'yield take($$$INITIALIZE_PAGE_VIEWER_SUCCESS$$$)'
Length of output: 66309 Script: #!/bin/bash
# Search for dispatch of initialization actions with better regex pattern
rg "dispatch.*(?:INITIALIZE_EDITOR_SUCCESS|INITIALIZE_PAGE_VIEWER_SUCCESS)" -A 3 --type ts --type js
# Search for any other watchers of these actions with ripgrep
rg "take\(.*(?:INITIALIZE_EDITOR_SUCCESS|INITIALIZE_PAGE_VIEWER_SUCCESS)" -A 3 --type ts --type js
# Search for the action type definitions
rg "INITIALIZE_EDITOR_SUCCESS|INITIALIZE_PAGE_VIEWER_SUCCESS" -A 1 --type ts --type js
Length of output: 3966 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
|
/build-deploy-preview |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12157578742. |
Deploy-Preview-URL: https://ce-37940.dp.appsmith.com |
## Description - For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](#37904) ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 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 user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
fix: fixing usage pulse for anon user (#37940)
- For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](#37904) /ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 UTC <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Enhanced user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description - For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](#37904) ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 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 user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description - For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](appsmithorg#37904) ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 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 user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
- For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](#37904) /ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 UTC <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Enhanced user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
- For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](#37904) /ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 UTC <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Enhanced user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Description - For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](#37904) ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 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 user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes 37904
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12157463790
Commit: b4c4df1
Cypress dashboard.
Tags:
@tag.All
Spec:
Wed, 04 Dec 2024 11:40:43 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor