-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Feat: back navigation support throughout the app #6701
Conversation
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> |
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.
👍 Looks good to me! Reviewed everything up to 88598ed in 1 minute and 49 seconds
More details
- Looked at
560
lines of code in6
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:
Replacehistory.replace
withsafeNavigate
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:
Removeconsole.group
andconsole.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:
UsesafeNavigate
instead ofhistory.replace
for navigation to ensure consistency and safety in URL handling. This is also applicable in other parts of the code wherehistory.replace
is used. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/providers/QueryBuilder.tsx:749
- Draft comment:
UsesafeNavigate
instead ofhistory.replace
for navigation to ensure consistency and safety in URL handling. This is also applicable in other parts of the code wherehistory.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.
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.
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 in37
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 theconsole.group
andconsole.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 theconsole.group
andconsole.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 theconsole.group
andconsole.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 theconsole.group
andconsole.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 theconsole.group
andconsole.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.
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.
👍 Looks good to me! Incremental review on b43aa08 in 55 seconds
More details
- Looked at
61
lines of code in3
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.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on 81e31e5 in 54 seconds
More details
- Looked at
47
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/tests/test-utils.tsx:14
- Draft comment:
Avoid usingCompatRouter
fromreact-router-dom-v5-compat
as 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.
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.
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 34b6c1a in 1 minute and 27 seconds
More details
- Looked at
171
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. frontend/src/container/LogsExplorerViews/tests/LogsExplorerViews.test.tsx:75
- Draft comment:
Consider centralizing theuseSafeNavigate
mock 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 theuseSafeNavigate
hook 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 theuseSafeNavigate
mock 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 theuseSafeNavigate
hook 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 theuseSafeNavigate
mock 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 theuseSafeNavigate
hook 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 theuseSafeNavigate
mock 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 theuseSafeNavigate
hook 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 theuseSafeNavigate
mock 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 theuseSafeNavigate
hook 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 theuseSafeNavigate
mock 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 theuseSafeNavigate
hook 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 theuseSafeNavigate
mock 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 theuseSafeNavigate
hook 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.
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 a56f011 in 49 seconds
More details
- Looked at
33
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/AlertRuleDocumentationRedirection.test.tsx:30
- Draft comment:
The mock implementation foruseSafeNavigate
is 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 foruseSafeNavigate
is 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.
a56f011
to
fa74a32
Compare
…params with URL comparison
…gation works properly
…ted relative time
…he existing back navigation
4bb94fc
a933e44
to
4bb94fc
Compare
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.
👍 Looks good to me! Incremental review on 4bb94fc in 1 minute and 37 seconds
More details
- Looked at
33
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/container/CreateAlertRule/AlertRuleDocumentationRedirection.test.tsx:30
- Draft comment:
Consider moving the common jest.mock('hooks/useSafeNavigate') into a shared test helper file to avoid duplication across tests. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the suggestion could potentially reduce duplication, I don't have evidence that this mock is actually duplicated across tests. The comment is speculative about potential future duplication. Additionally, for such a simple mock (3 lines), the overhead of moving it to a shared file might not be worth it unless there's clear evidence of widespread duplication.
I might be undervaluing the benefits of test helper consolidation. Moving mocks to helper files can make tests more maintainable if the mock implementation needs to change.
Without evidence of actual duplication, this comment is speculative and could lead to premature abstraction. The mock is simple enough that duplication may be preferable to indirection.
Delete the comment as it's speculative and suggests a refactor without clear evidence of a problem. We should wait until we see actual duplication before suggesting moving mocks to shared files.
2. frontend/src/container/CreateAlertRule/AnomalyAlertDocumentationRedirection.test.tsx:39
- Draft comment:
Consider moving the common jest.mock('hooks/useSafeNavigate') into a shared test helper file to avoid duplication across tests. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/CreateAlertRule/AlertRuleDocumentationRedirection.test.tsx:27
- Draft comment:
Consider centralizing the useSafeNavigate mock (added at lines 27–33) into a shared test setup if it’s used across multiple test files. This will reduce duplication and ease maintenance if the hook’s implementation changes. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. frontend/src/container/CreateAlertRule/AnomalyAlertDocumentationRedirection.test.tsx:39
- Draft comment:
Consider centralizing the useSafeNavigate mock (added at lines 39–43) into a shared test setup if it’s used across multiple tests. This helps avoid duplication and simplifies future updates. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_IGI2GHSrsdtDj3Q7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@ahmadshaheer can we merge this? |
Yes, @ankitnayan, enabled auto merge 👍 |
1a395a9
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.
👍 Looks good to me! Incremental review on 1a395a9 in 1 minute and 49 seconds
More details
- Looked at
17
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/GridCardLayout/GridCard/WidgetGraphComponent.test.tsx:26
- Draft comment:
Avoid using 'any' for the return type in the useSafeNavigate mock. Consider providing a more specific type for better type safety in tests. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/container/GridCardLayout/GridCard/WidgetGraphComponent.test.tsx:26
- Draft comment:
Good to see the useSafeNavigate hook is mocked for testing. Consider providing a proper type signature instead of using 'any' for improved type safety, and if this mock is used in multiple tests, consider centralizing it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the suggestion would improve type safety, this is a test file and the mock is very simple. The current implementation withany
works fine for the test's purpose. The suggestion is more of a nice-to-have improvement rather than a critical issue. Test files often useany
for simple mocks without significant downsides.
The comment does point out a legitimate way to improve type safety. Using proper types even in tests can help catch errors earlier and improve maintainability.
However, this is a very simple mock in a test file where the type doesn't significantly impact the test's functionality or reliability. The benefit of adding the explicit type is minimal compared to the effort of making the change.
The comment should be deleted as it suggests a minor improvement to test code that doesn't meaningfully impact the code's correctness or maintainability.
Workflow ID: wflow_0iRMApgnWbIHyqXo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on cdd8f6c in 1 minute and 3 seconds
More details
- Looked at
17
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/components/ExplorerCard/test/ExplorerCard.test.tsx:23
- Draft comment:
Good use of jest.mock for the useSafeNavigate hook. Ensure that the mock reflects the expected behavior of the actual hook to avoid false positives in tests. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. frontend/src/components/ExplorerCard/test/ExplorerCard.test.tsx:23
- Draft comment:
The new mock for useSafeNavigate is added correctly. Consider verifying its usage in tests if back navigation behavior should be explicitly tested. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_NURkT1kKP10RjdWv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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
Implement back navigation support across the app using a new
safeNavigate
function to handle query parameters and prevent unnecessary navigation.useSafeNavigate
hook inuseSafeNavigate.ts
to replacehistory.push
for safer navigation.history.push
withsafeNavigate
inTracesTableComponent.tsx
,DashboardWidget/index.tsx
, andEditRules/index.tsx
.useUrlQueryData.ts
to usesafeNavigate
for URL query updates.TracesExplorer/index.tsx
andTracesExplorer/Filter/Filter.tsx
to handle query updates and navigation usingsafeNavigate
.DateTimeSelectionV2/index.tsx
to sync time picker state with URL on browser navigation.useSafeNavigate
in various test files likeDashboardListPage.test.tsx
andLogsExplorer.test.tsx
.normalizeTimeToMs
intimeUtils.ts
for timestamp conversion.Dashboard.tsx
andQueryBuilder.tsx
to usesafeNavigate
for navigation actions.This description was created by
for cdd8f6c. It will automatically update as commits are pushed.