-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: route to the onboarding flow when clicking the sending logs in case of no logs #4600
Conversation
Warning Rate Limit Exceeded@Vikrant2520 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 19 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update enhances the onboarding experience for users by introducing new routes and permissions for application monitoring, logs management, and infrastructure monitoring. It also includes UI adjustments to accommodate these new features and ensures a more guided and informative onboarding process. The changes streamline the next steps in user onboarding and improve the application's configurability for different user needs. Changes
Related issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (9)
- frontend/src/AppRoutes/routes.ts (1 hunks)
- frontend/src/constants/routes.ts (1 hunks)
- frontend/src/container/AppLayout/index.tsx (1 hunks)
- frontend/src/container/OnboardingContainer/OnboardingContainer.tsx (6 hunks)
- frontend/src/container/OnboardingContainer/utils/dataSourceUtils.ts (3 hunks)
- frontend/src/container/TopNav/DateTimeSelection/config.ts (1 hunks)
- frontend/src/container/TopNav/DateTimeSelectionV2/config.ts (1 hunks)
- frontend/src/utils/app.ts (1 hunks)
- frontend/src/utils/permission/index.ts (1 hunks)
Additional comments: 11
frontend/src/constants/routes.ts (1)
- 10-13: The addition of new routes for application monitoring, logs management, and infrastructure monitoring under the
GET_STARTED
section is a positive enhancement. It aligns with the objectives to improve the onboarding flow and routing logic, enhancing the application's navigation and user experience.frontend/src/container/TopNav/DateTimeSelection/config.ts (1)
- 84-86: The addition of new routes for application monitoring, infrastructure monitoring, and logs management to the list of routes to skip in the top navigation bar configuration is logical. It enhances the user experience by not displaying irrelevant UI elements on onboarding or setup pages.
frontend/src/utils/permission/index.ts (1)
- 85-87: The addition of permissions for application monitoring, infrastructure monitoring, and logs management under the
GET_STARTED
section is crucial for access control. It ensures that only authorized users can access these features, supporting the application's security and user management strategy.frontend/src/container/TopNav/DateTimeSelectionV2/config.ts (1)
- 120-122: The addition of new routes for application monitoring, infrastructure monitoring, and logs management to the list of routes to skip in the top navigation bar configuration for
DateTimeSelectionV2
is consistent with improving user experience. It avoids displaying unnecessary UI components on specific pages.frontend/src/container/OnboardingContainer/utils/dataSourceUtils.ts (3)
- 6-10: The addition of the
ModulesMap
enum is a positive change, providing a clear and maintainable way to reference different modules within the onboarding flow.- 91-131: Updating image URLs for various supported languages ensures that the correct assets are used, enhancing the UI's visual aspects. Please verify the correctness and accessibility of these URLs in the deployed environment.
- 288-293: The introduction of the
moduleRouteMap
to map modules to specific routes supports the new routing logic, directly aligning with the PR's objectives to refine the onboarding flow and enhance navigation.frontend/src/AppRoutes/routes.ts (1)
- 60-60: Changing the
exact
property fromtrue
tofalse
for theROUTES.GET_STARTED
route allows for more flexible route matching. This change likely accommodates the new onboarding flow routes, enabling nested routing under theGET_STARTED
path. Please verify its impact on the application's routing behavior to ensure it aligns with the intended user experience.frontend/src/container/OnboardingContainer/OnboardingContainer.tsx (2)
- 9-9: The addition of imports for
ROUTES
,moduleRouteMap
, andhistory
is appropriate for the enhancements made to the onboarding flow and routing logic. Ensure that these imports are used efficiently throughout the file to avoid any unnecessary overhead.- 205-243: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [187-212]
The introduction of the
handleNextStep
function and modifications to thehandleNext
function to incorporate route changes based on selected modules are significant improvements. However, ensure that theactiveStep
andcurrent
state variables are managed correctly to prevent any off-by-one errors or unintended behavior during the onboarding process.frontend/src/container/AppLayout/index.tsx (1)
- 234-238: The expansion of the
renderFullScreen
condition to include checks forGET_STARTED_APPLICATION_MONITORING
,GET_STARTED_INFRASTRUCTURE_MONITORING
, andGET_STARTED_LOGS_MANAGEMENT
routes is a logical enhancement that aligns with the objectives of improving the onboarding flow and user experience. Ensure that these route constants are correctly defined in theROUTES
object and that the logic correctly handles all scenarios where a full-screen layout is desired.
frontend/src/container/OnboardingContainer/OnboardingContainer.tsx
Outdated
Show resolved
Hide resolved
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- frontend/src/container/NoLogs/NoLogs.tsx (3 hunks)
Additional comments: 1
frontend/src/container/NoLogs/NoLogs.tsx (1)
- 39-39: The use of
onClick={handleLinkClick}
within theTypography.Link
component is a good practice for enhancing user experience through dynamic routing. However, ensure that thehref
attribute's value is still accessible to users with JavaScript disabled in their browsers. This ensures that the application remains accessible to all users.Consider verifying the accessibility of links with JavaScript disabled and ensure fallback URLs are provided if necessary.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
75df8c4
to
f38436d
Compare
The simple re-route logic will be annoying because it could be possible that the user sets up the logging by following instructions but stops sending data for some reason or it could be that their ingest pattern is not continuous logging. At this point, they shouldn't be redirected because they already completed the onboarding workflow and sent some logs. The only time the user should be redirected is when they are on the platform for the first time and didn't send any logs/traces in the past. |
@srikanthccv we are not re-routing the user to the onboarding page , we render the logs screen itself and on click of |
The specifics of re-route are not important. It is annoying if you ask to send logs instead of showing no logs when they already completed the onboarding workflow and sent some logs. |
I don't know the details about the onboarding related things but we shouldn't show that screen if they already sent some logs in the past and completed the onboarding flow. |
b818534
to
e63d515
Compare
e63d515
to
2684173
Compare
Summary
module
in the URL params when selecting it.Related Issues / PR's
https://linear.app/signoz-io/issue/SIG-534/empty-logs-text-update
Screenshots
NA
Affected Areas and Manually Tested Areas
Summary by CodeRabbit