Feat: back navigation support throughout the app #6701
+554
−134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 88598ed in 1 minute and 49 seconds
More details
- Looked at
560lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6drafted 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:
Replacehistory.replacewithsafeNavigatefor 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:
Removeconsole.groupandconsole.logstatements 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:
UsesafeNavigateinstead ofhistory.replacefor navigation to ensure consistency and safety in URL handling. This is also applicable in other parts of the code wherehistory.replaceis used. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/providers/QueryBuilder.tsx:749
- Draft comment:
UsesafeNavigateinstead ofhistory.replacefor navigation to ensure consistency and safety in URL handling. This is also applicable in other parts of the code wherehistory.replaceis 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
force-pushed
the
feat/back-navigation-support-throughout-the-app
branch
from
88598ed to
3e487eb
Compare
|
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.
👍 Looks good to me! Reviewed everything up to 10cf077 in 1 minute and 49 seconds
More details
- Looked at
1908lines of code in37files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. frontend/src/hooks/useSafeNavigate.ts:108
- Draft comment:
Remove theconsole.groupandconsole.logstatements 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 theconsole.groupandconsole.logstatements 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 theconsole.groupandconsole.logstatements 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 theconsole.groupandconsole.logstatements 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 theconsole.groupandconsole.logstatements 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.
|
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.
👍 Looks good to me! Incremental review on b43aa08 in 55 seconds
More details
- Looked at
61lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4drafted 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.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
ahmadshaheer
force-pushed
the
feat/back-navigation-support-throughout-the-app
branch
from
b43aa08 to
1474cf4
Compare
|
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.
❌ Changes requested. Incremental review on 81e31e5 in 54 seconds
More details
- Looked at
47lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/tests/test-utils.tsx:14
- Draft comment:
Avoid usingCompatRouterfromreact-router-dom-v5-compatas it introduces inline styles. Consider using external stylesheets or styled components instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This is a test utilities file, not a production component. The CompatRouter is being used to provide compatibility between different React Router versions for testing purposes. The presence of inline styles in a testing environment is not a significant concern since this code is not part of the production bundle. The comment seems to misunderstand the purpose of this file and CompatRouter's role in testing.
Maybe inline styles could affect test reliability or maintainability even in test files? Maybe there's a better way to handle router version compatibility?
While code quality in test files matters, CompatRouter is a standard utility for handling React Router version compatibility in tests. Any inline styles it adds are part of its core functionality and don't impact test reliability.
The comment should be deleted. It misunderstands the purpose of CompatRouter in a test utility file and applies production styling guidelines to test code where they aren't relevant.
Workflow ID: wflow_lsg5G0oZz9tu5DFb
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
…params with URL comparison
…gation works properly
…ted relative time
…he existing back navigation
…from alert creation page
…ectionV2 component it also fixes the issue of relativeTime not being added to the url on mounting
…iews and useUrlQueryData
…ination with url param
… redirecting in NewWidget
ahmadshaheer
force-pushed
the
feat/back-navigation-support-throughout-the-app
branch
from
8adcde3 to
34b6c1a
Compare
|
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.
👍 Looks good to me! Incremental review on 34b6c1a in 1 minute and 27 seconds
More details
- Looked at
171lines of code in10files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. frontend/src/container/LogsExplorerViews/tests/LogsExplorerViews.test.tsx:75
- Draft comment:
Consider centralizing theuseSafeNavigatemock in a common test setup file to avoid redundancy across multiple test files. - Reason this comment was not posted:
Confidence changes required:50%
The code is mocking theuseSafeNavigatehook in multiple test files. This is a repetitive pattern and can be centralized to avoid redundancy. Centralizing the mock in a common test setup file would make the tests cleaner and easier to maintain.
2. frontend/src/container/NewDashboard/DashboardDescription/__tests__/DashboardDescription.test.tsx:28
- Draft comment:
Consider centralizing theuseSafeNavigatemock in a common test setup file to avoid redundancy across multiple test files. - Reason this comment was not posted:
Confidence changes required:50%
The code is mocking theuseSafeNavigatehook in multiple test files. This is a repetitive pattern and can be centralized to avoid redundancy. Centralizing the mock in a common test setup file would make the tests cleaner and easier to maintain.
3. frontend/src/container/NewWidget/RightContainer/ColumnUnitSelector/__tests__/ColumnSelector.test.tsx:90
- Draft comment:
Consider centralizing theuseSafeNavigatemock in a common test setup file to avoid redundancy across multiple test files. - Reason this comment was not posted:
Confidence changes required:50%
The code is mocking theuseSafeNavigatehook in multiple test files. This is a repetitive pattern and can be centralized to avoid redundancy. Centralizing the mock in a common test setup file would make the tests cleaner and easier to maintain.
4. frontend/src/container/QueryTable/__test__/QueryTable.test.tsx:26
- Draft comment:
Consider centralizing theuseSafeNavigatemock in a common test setup file to avoid redundancy across multiple test files. - Reason this comment was not posted:
Confidence changes required:50%
The code is mocking theuseSafeNavigatehook in multiple test files. This is a repetitive pattern and can be centralized to avoid redundancy. Centralizing the mock in a common test setup file would make the tests cleaner and easier to maintain.
5. frontend/src/hooks/useResourceAttribute/__tests__/useResourceAttribute.test.tsx:8
- Draft comment:
Consider centralizing theuseSafeNavigatemock in a common test setup file to avoid redundancy across multiple test files. - Reason this comment was not posted:
Confidence changes required:50%
The code is mocking theuseSafeNavigatehook in multiple test files. This is a repetitive pattern and can be centralized to avoid redundancy. Centralizing the mock in a common test setup file would make the tests cleaner and easier to maintain.
6. frontend/src/pages/DashboardsListPage/__tests__/DashboardListPage.test.tsx:32
- Draft comment:
Consider centralizing theuseSafeNavigatemock in a common test setup file to avoid redundancy across multiple test files. - Reason this comment was not posted:
Confidence changes required:50%
The code is mocking theuseSafeNavigatehook in multiple test files. This is a repetitive pattern and can be centralized to avoid redundancy. Centralizing the mock in a common test setup file would make the tests cleaner and easier to maintain.
7. frontend/src/pages/LogsExplorer/__tests__/LogsExplorer.test.tsx:70
- Draft comment:
Consider centralizing theuseSafeNavigatemock in a common test setup file to avoid redundancy across multiple test files. - Reason this comment was not posted:
Confidence changes required:50%
The code is mocking theuseSafeNavigatehook in multiple test files. This is a repetitive pattern and can be centralized to avoid redundancy. Centralizing the mock in a common test setup file would make the tests cleaner and easier to maintain.
8. frontend/src/pages/TracesExplorer/Filter/Filter.tsx:188
- 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 codebase. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_jjGurUwQHylTgAM0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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.
👍 Looks good to me! Incremental review on a56f011 in 49 seconds
More details
- Looked at
33lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/AlertRuleDocumentationRedirection.test.tsx:30
- Draft comment:
The mock implementation foruseSafeNavigateis duplicated in both test files. Consider refactoring to a common test utility to avoid redundancy. - Reason this comment was not posted:
Confidence changes required:50%
The mock implementation foruseSafeNavigateis duplicated in both test files. This can be refactored to avoid redundancy.
2. frontend/src/container/CreateAlertRule/AlertRuleDocumentationRedirection.test.tsx:30
- Draft comment:
Avoid repeating the mock for 'hooks/useSafeNavigate' in multiple test files. Consider creating a separate mock file or utility to handle this. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While having shared mocks can be good practice, this mock is very simple and specific to test needs. Creating a separate file for such a simple mock might be overengineering. The mock is only 5 lines long and may need to be customized per test file. Also, this comment is about an existing pattern in the codebase (mock organization) rather than about a specific change in this diff.
The comment does point out a valid code organization concern. Centralizing mocks could make the codebase more maintainable.
However, the mock is so simple that centralizing it might add more complexity than it solves. Plus, we don't have context about whether this pattern exists elsewhere in the codebase.
Delete the comment. While the suggestion isn't wrong, it's more about general code organization than about a specific change, and the benefit would be minimal for such a simple mock.
Workflow ID: wflow_5L8TCvPp9BYgj0lL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Service details
Query Builder
Traces explorer / Logs explorer
Dashboards
Important
Implemented back navigation support with query parameter retention across the app using a new
useSafeNavigatehook.useSafeNavigatehook for safer URL navigation, replacinghistoryinEditRules,DashboardWidget,TracesTableComponent, and other components.Services,Service details,Query Builder,Traces explorer,Logs explorer, andDashboards.DynamicColumnTable,FormAlertRules,FullView,WidgetGraphComponent,GridCardLayout,WidgetHeader,DashboardsList,ImportJSON,LogsExplorerChart,LogsExplorerViews,MetricsApplication,NewWidget,QueryTable,DateTimeSelectionV2,TracesTableComponent,ResourceProvider,useResourceAttribute,useSafeNavigate,useUrlQueryData,AlertDetails,AlertList,DashboardWidget,DashboardsListPage,EditRules,LogsExplorer,TracesExplorer,Dashboard,QueryBuilder,test-utils,timeUtilsto usesafeNavigate.ColumnSelector.test.tsx,QueryTable.test.tsx,DashboardListPage.test.tsx,LogsExplorer.test.tsx,TracesExplorer.test.tsx,useResourceAttribute.test.tsxto mockuseSafeNavigate.normalizeTimeToMsintimeUtilsfor timestamp conversion.useSafeNavigateto prevent unnecessary URL updates and handle default navigation.This description was created by
for a56f011. It will automatically update as commits are pushed.