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

fix: disabled same url check for redirect in logs explorer #7187

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sawhil
Copy link
Member

@sawhil sawhil commented Feb 26, 2025

Summary

Related Issues / PR's

Screenshots

NA

Affected Areas and Manually Tested Areas


Important

Adds option to disable same URL check in useSafeNavigate and applies it to LOGS data source in QueryBuilderProvider.

  • Behavior:
    • Adds enableSameURLCheck option to useSafeNavigate in useSafeNavigate.ts to control same URL navigation check.
    • In QueryBuilder.tsx, disables same URL check for LOGS data source by passing enableSameURLCheck: false to useSafeNavigate.
  • Functions:
    • Modifies useSafeNavigate to accept UseSafeNavigateProps with enableSameURLCheck defaulting to true.
    • Updates safeNavigate function to respect enableSameURLCheck flag.
  • Misc:
    • Updates dependencies in useSafeNavigate hook to include enableSameURLCheck.

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

@sawhil sawhil requested a review from YounixM as a code owner February 26, 2025 03:36
@github-actions github-actions bot added the bug Something isn't working label Feb 26, 2025
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 84c61f8 in 1 minute and 14 seconds

More details
  • Looked at 59 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. frontend/src/hooks/useSafeNavigate.ts:86
  • Draft comment:
    Nice work adding an option to disable the same URL check. Ensure that tests cover both enabled and disabled scenarios.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. frontend/src/hooks/useSafeNavigate.ts:117
  • Draft comment:
    Good conditional check based on enableSameURLCheck. Double-check if other parts of URL comparison need similar flexibility.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
3. frontend/src/providers/QueryBuilder.tsx:766
  • Draft comment:
    Passing enableSameURLCheck as a function of the data source is a clear solution for logs explorer redirection.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
4. frontend/src/hooks/useSafeNavigate.ts:86
  • Draft comment:
    Good implementation: the new enableSameURLCheck parameter defaults to true and is correctly used in the safeNavigate callback. The dependency array now includes enableSameURLCheck, ensuring that its changes are respected. Consider adding a brief comment or JSDoc note explaining its usage.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. frontend/src/providers/QueryBuilder.tsx:766
  • Draft comment:
    The safeNavigate hook is now invoked with enableSameURLCheck disabled when initialDataSource equals DataSource.LOGS, which fits the logs explorer fix. Make sure that initialDataSource updates appropriately to reflect the logs data source when needed.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. frontend/src/providers/QueryBuilder.tsx:169
  • Draft comment:
    Typo: The variable name 'setupedQueryData' is misspelled. Consider renaming it to 'setupQueryData' or 'preparedQueryData' for improved clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_lB64EAMdqKRhLnC0


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

@@ -763,7 +763,9 @@ export function QueryBuilderProvider({
[panelType, stagedQuery],
);

const { safeNavigate } = useSafeNavigate();
const { safeNavigate } = useSafeNavigate({
enableSameURLCheck: !(initialDataSource === DataSource.LOGS),
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we checked the same on traces page? Why only logs here.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, just did for logs. After Shaheer shares his reasoning for the same change. Will either remove the whole check or add the flag from required sources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sawhil ,

in the areUrlsEffectivelySame function, we are checking if the urls are effectively the same i.e. if the composite query and other params are the same. if so, we prevent the navigation. (#6701)

I think we need to add the enableSameURLCheck flag for traces as well.

@@ -78,7 +82,9 @@ const isDefaultNavigation = (currentUrl: URL, targetUrl: URL): boolean => {

return newKeys.length > 0;
};
export const useSafeNavigate = (): {
export const useSafeNavigate = (
{ enableSameURLCheck }: UseSafeNavigateProps = { enableSameURLCheck: true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we slightly change this to make it more readable?

e.g.

we can change this to

	{ preventSameUrlNavigation }: UseSafeNavigateProps = { preventSameUrlNavigation: true },

if we want to enable same url navigation in the consumers. we could say

	const { safeNavigate } = useSafeNavigate({
		preventSameUrlNavigation: false,
	});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants