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

Merged
merged 25 commits into from
Feb 14, 2025

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

Implement back navigation support across the app using a new safeNavigate function to handle query parameters and prevent unnecessary navigation.

  • Navigation:
    • Introduce useSafeNavigate hook in useSafeNavigate.ts to replace history.push for safer navigation.
    • Replace history.push with safeNavigate in TracesTableComponent.tsx, DashboardWidget/index.tsx, and EditRules/index.tsx.
    • Update useUrlQueryData.ts to use safeNavigate for URL query updates.
  • Components:
    • Modify TracesExplorer/index.tsx and TracesExplorer/Filter/Filter.tsx to handle query updates and navigation using safeNavigate.
    • Update DateTimeSelectionV2/index.tsx to sync time picker state with URL on browser navigation.
  • Testing:
    • Mock useSafeNavigate in various test files like DashboardListPage.test.tsx and LogsExplorer.test.tsx.
  • Utilities:
    • Add normalizeTimeToMs in timeUtils.ts for timestamp conversion.
  • Providers:
    • Update Dashboard.tsx and QueryBuilder.tsx to use safeNavigate for navigation actions.

This description was created by Ellipsis for cdd8f6c. 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>

@ahmadshaheer ahmadshaheer force-pushed the feat/back-navigation-support-throughout-the-app branch from b43aa08 to 1474cf4 Compare January 20, 2025 03:07
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.

❌ Changes requested. Incremental review on 81e31e5 in 54 seconds

More details
  • Looked at 47 lines of code in 1 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 using CompatRouter from react-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.

frontend/src/tests/test-utils.tsx Outdated Show resolved Hide resolved
@ahmadshaheer ahmadshaheer force-pushed the feat/back-navigation-support-throughout-the-app branch from 8adcde3 to 34b6c1a Compare January 21, 2025 15:48
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 34b6c1a in 1 minute and 27 seconds

More details
  • Looked at 171 lines of code in 10 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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 the useSafeNavigate 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.

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 a56f011 in 49 seconds

More details
  • Looked at 33 lines of code in 2 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 for useSafeNavigate 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 for useSafeNavigate 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.

@YounixM YounixM force-pushed the feat/back-navigation-support-throughout-the-app branch from a56f011 to fa74a32 Compare January 28, 2025 11:55
SagarRajput-7
SagarRajput-7 previously approved these changes Feb 11, 2025
amlannandy
amlannandy previously approved these changes Feb 11, 2025
@ahmadshaheer ahmadshaheer force-pushed the feat/back-navigation-support-throughout-the-app branch from a933e44 to 4bb94fc Compare February 12, 2025 08:21
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 4bb94fc in 1 minute and 37 seconds

More details
  • Looked at 33 lines of code in 2 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% <= threshold 50%
    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% <= threshold 50%
    None

Workflow ID: wflow_IGI2GHSrsdtDj3Q7


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

amlannandy
amlannandy previously approved these changes Feb 12, 2025
SagarRajput-7
SagarRajput-7 previously approved these changes Feb 13, 2025
@ankitnayan
Copy link
Collaborator

@ahmadshaheer can we merge this?

@ahmadshaheer
Copy link
Collaborator Author

@ahmadshaheer can we merge this?

Yes, @ankitnayan, enabled auto merge 👍

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 1a395a9 in 1 minute and 49 seconds

More details
  • Looked at 17 lines of code in 1 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 with any 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 use any 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.

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 cdd8f6c in 1 minute and 3 seconds

More details
  • Looked at 17 lines of code in 1 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% <= threshold 50%
    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% <= threshold 50%
    None

Workflow ID: wflow_NURkT1kKP10RjdWv


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

@ahmadshaheer ahmadshaheer enabled auto-merge (squash) February 13, 2025 07:01
@ahmadshaheer ahmadshaheer enabled auto-merge (squash) February 14, 2025 03:46
@ahmadshaheer ahmadshaheer merged commit 82d84c0 into main Feb 14, 2025
14 of 16 checks passed
@ahmadshaheer ahmadshaheer deleted the feat/back-navigation-support-throughout-the-app branch February 14, 2025 03:54
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.

4 participants