Skip to content
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: back navigation support throughout the app #6701

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Dec 23, 2024

Summary

Handled back navigation throughout the app

Note: This PR handles the back navigation throughout the app wherever we have handled state management with query params, there are some pages where the query params are not being added, for those cases, the query params should first be added in the URL then we can handle the back navigation.

Related Issues / PR's

Screenshots

NA

Affected Areas and Manually Tested Areas

  • Services

    • Go to a service, and navigate back to the services page
    • Select environment from the dropdown, and go back, the environments should be removed
    • Filter services by any of the filters (e.g. host.name), and go back, the filters should be removed
    • Update the time range, and go back, the time range picker should retain the previous time range
  • Service details

    • Select any range in any of the graphs, and go back, the graph and time picker should retain the previous state
    • Click on any key operation, it should redirect to traces page, and go back, the service details page should keep it's last state
    • Switch between the different tabs (i.e. Overview, DB Calls, External Metrics), and go back, the most recent tab should be selected
    • Click on any point in the graph, click on go to traces, it should redirect to traces page, and go back, the service details page should keep it's last state
    • Click on any point in the graph, click on go to logs, it should redirect to logs page, and go back, the service details page should keep it's last state
  • Query Builder

    • Select any filters, stage query, and go back, the previous query should be retained
    • Add Group by filter, stage query, and go back, the previous query should be retained
    • Add Limit filter, stage query, and go back, the previous query should be retained
    • Add Having filter, stage query, and go back, the previous query should be retained
    • Change the aggregate every, stage query, and go back, the previous query should be retained
    • Clone the query, stage query, and go back, the previous query should be retained
    • Hide the query, stage query, and go back, the previous query should be retained
    • Add new query, stage query, and go back, the previous query should be retained
    • Add a new formula, stage query, and go back, the previous query should be retained
  • Traces explorer / Logs explorer

    • Switch between the different tabs, and go back, the most recent tab should be selected
    • Change the range picker, go back, the range picker should retain the previous state
    • Create an alert from traces explorer, go back, it should redirect to traces explorer page
    • Add to dashboard, go back, it should redirect to traces explorer page
    • Choose any quick filters, go back, the previous state should be retained
    • Choose a saved view, go back, the previous state should be retained
  • Dashboards

    • Go to a dashboard, and press back button, it should redirect to dashboard list
    • Dashboard
      • Click on new panel, choose a type, go back, it should redirect to dashboard page
      • Change the time range, go back, it should retain the previous state
      • Edit a panel, go back, it should be redirect to dashboard page

Important

Introduce useSafeNavigate for safe URL navigation and enhance back navigation support across the app.

  • Navigation:
    • Introduce useSafeNavigate hook for safe URL navigation.
    • Replace history.push and history.replace with safeNavigate in components like TracesTableComponent.tsx, DashboardWidget/index.tsx, and EditRules/index.tsx.
    • Handle URL parameter updates and prevent unnecessary navigation if URLs are effectively the same.
  • Time Handling:
    • Add normalizeTimeToMs function in timeUtils.ts to convert timestamps to milliseconds.
    • Use normalizeTimeToMs in DateTimeSelectionV2/index.tsx for time synchronization.
  • State Management:
    • Sync time picker state with URL on browser navigation in DateTimeSelectionV2/index.tsx.
    • Update pagination state and URL in TracesTableComponent.tsx using safeNavigate.

This description was created by Ellipsis for b43aa08. It will automatically update as commits are pushed.

@ahmadshaheer ahmadshaheer marked this pull request as draft December 23, 2024 15:21
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the enhancement New feature or request label Dec 23, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 88598ed in 1 minute and 49 seconds

More details
  • Looked at 560 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. frontend/src/utils/timeUtils.ts:133
  • Draft comment:
    Remove the console.log statement to avoid unnecessary console output.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The console.log statement in a utility function is unnecessary and could clutter the console output. It should be removed for cleaner code.
2. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:511
  • Draft comment:
    Replace history.replace with safeNavigate for consistency and safe navigation.
		safeNavigate(generatedUrl);
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
3. frontend/src/hooks/useSafeNavigate.ts:108
  • Draft comment:
    Remove console.group and console.log statements to avoid cluttering the console in production.
			```
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    Console statements in production code can impact performance and expose potentially sensitive information. However, these particular logs seem intentionally added for debugging navigation issues, which can be complex. They provide useful information about why certain navigations are prevented or how they're handled. The code is new, so this was a deliberate choice by the author.
    The console statements could be wrapped in a development-only check or use a proper logging system. They might be useful for debugging in production in some cases.
    While the comment raises a valid point about console statements in production, these seem intentionally added for debugging complex navigation scenarios. If they're problematic, they should be handled through proper environment-based logging rather than removed entirely.
    The comment should be deleted. The console statements appear to be an intentional debugging feature, and their complete removal might make troubleshooting harder. If there are concerns about production logging, a more nuanced approach would be needed.
4. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:373
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other parts of the code where inline styles are used.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
5. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:621
  • Draft comment:
    Use safeNavigate instead of history.replace for navigation to ensure consistency and safety in URL handling. This is also applicable in other parts of the code where history.replace is used.
  • Reason this comment was not posted:
    Marked as duplicate.
6. frontend/src/providers/QueryBuilder.tsx:749
  • Draft comment:
    Use safeNavigate instead of history.replace for navigation to ensure consistency and safety in URL handling. This is also applicable in other parts of the code where history.replace is used.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_CUoLraS1fsj6U3Wt


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ahmadshaheer ahmadshaheer force-pushed the feat/back-navigation-support-throughout-the-app branch from 88598ed to 3e487eb Compare December 29, 2024 12:26
@ahmadshaheer ahmadshaheer marked this pull request as ready for review December 30, 2024 16:00
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 10cf077 in 1 minute and 49 seconds

More details
  • Looked at 1908 lines of code in 37 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. frontend/src/hooks/useSafeNavigate.ts:108
  • Draft comment:
    Remove the console.group and console.log statements to avoid unnecessary console output in production.
			```
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    While removing console logs before production is generally good practice, these logs seem intentionally added for debugging complex navigation logic. They provide useful information about URL comparisons and navigation decisions. The logs are structured (using console.group) and contain emojis, suggesting they were carefully designed for development debugging.
    The comment raises a valid point about production console output. However, I might be overlooking the possibility that these logs are actually causing performance issues or cluttering production logs.
    Given the complexity of the navigation logic and the structured nature of the logs, these console statements appear to be intentional development tools rather than oversight. If needed, they can be stripped out during production builds using standard build tools.
    The comment should be deleted as these console logs appear to be intentionally added debugging tools, and their removal, if needed, should be handled by build configuration rather than code changes.
2. frontend/src/hooks/useSafeNavigate.ts:109
  • Draft comment:
    Remove the console.group and console.log statements to avoid unnecessary console output in production.
			```
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/hooks/useSafeNavigate.ts:110
  • Draft comment:
    Remove the console.group and console.log statements to avoid unnecessary console output in production.
			```
  • Reason this comment was not posted:
    Marked as duplicate.
4. frontend/src/hooks/useSafeNavigate.ts:116
  • Draft comment:
    Remove the console.group and console.log statements to avoid unnecessary console output in production.
				```
  • Reason this comment was not posted:
    Marked as duplicate.
5. frontend/src/hooks/useSafeNavigate.ts:123
  • Draft comment:
    Remove the console.group and console.log statements to avoid unnecessary console output in production.
			```
  • Reason this comment was not posted:
    Marked as duplicate.
6. frontend/src/pages/AlertList/index.tsx:68
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other parts of the code.
  • Reason this comment was not posted:
    Comment was on unchanged code.
7. frontend/src/pages/TracesExplorer/index.tsx:198
  • Draft comment:
    Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other parts of the code.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_UycnqfbXwaq4uN15


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on b43aa08 in 55 seconds

More details
  • Looked at 61 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/hooks/useSafeNavigate.ts:111
  • Draft comment:
    Remove unnecessary console logs to avoid cluttering the console in production.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/utils/timeUtils.ts:138
  • Draft comment:
    Update the comment to accurately reflect that the function converts nanoseconds to milliseconds.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. frontend/src/container/MetricsApplication/Tabs/util.ts:50
  • Draft comment:
    Avoid using inline styles. Consider using CSS classes or styled components instead. This applies to the onGraphClickHandler function.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
4. frontend/src/hooks/useSafeNavigate.ts:108
  • Draft comment:
    Remove unnecessary console logs to clean up the code. This applies to other console logs in this file as well.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_g5twMcH7FGUpnKE7


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

github-actions bot commented Jan 2, 2025

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant