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 conflict-marker duplicated #6839

Merged
merged 14 commits into from
Aug 24, 2023
Merged

Fix conflict-marker duplicated #6839

merged 14 commits into from
Aug 24, 2023

Conversation

134130
Copy link
Contributor

@134130 134130 commented Aug 18, 2023

Description

Test URLs

Screenshot

image

@fregante
Copy link
Member

This unfortunately triggers 25 HTTP requests every time you open the pull requests page, which isn't acceptable. We need to use batched-function to compose the old GraphQL call and inside it add the icons to the DOM. I don’t think we used this exact pattern in the extension yet but we need to start. The .gql file cannot be used in this case because of the composed GraphQL call.

@134130
Copy link
Contributor Author

134130 commented Aug 18, 2023

This unfortunately triggers 25 HTTP requests every time you open the pull requests page, which isn't acceptable. We need to use batched-function to compose the old GraphQL call and inside it add the icons to the DOM. I don’t think we used this exact pattern in the extension yet but we need to start. The .gql file cannot be used in this case because of the composed GraphQL call.

Ok I'll make bunch API call as it was before

@@ -72,25 +51,22 @@ async function init(): Promise<false | void> {
}
}

function init(signal: AbortSignal): void {
observe('#js-issues-toolbar', addConflictMarkers, {signal});
Copy link
Member

Choose a reason for hiding this comment

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

What is this element? Does it exist on the milestones page? Does it work there now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works ;)
image

Copy link
Member

Choose a reason for hiding this comment

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

I think this will fail on slower connections because the observer will call the callback as soon as #js-issues-toolbar is found, but at that point the long list of PRs might not be done downloading yet. This is why I was suggesting to use batched-function and the link selector

const addIcon = batchedFunction((icons) => {
  // create query, etc
});

observe('.js-issue-row:has(.octicon-git-pull-request.color-fg-open) a.js-navigation-open', addIcon, {signal})

Copy link
Contributor Author

@134130 134130 Aug 19, 2023

Choose a reason for hiding this comment

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

🤔 I've tested a lot but It can detect icons successly. But I'll test more slow network conditions.
And is your suggestion works correctly? As I know, the selector-observe only works as single element, not all elements.

Edit: I think there is some other api; batchedFunction. I'll check this later

Copy link
Member

@fregante fregante Aug 19, 2023

Choose a reason for hiding this comment

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

As I know, the selector-observe only works as single element, not all elements

No, the observer detects any and all elements that appear on the page until the next page load.

Here's what happens with the suggested code:

  1. observe finds 3 elements
  2. calls addIcon once per element
  3. batched function then calls the anonymous function with a single array: [prLink1, prLink2, prLink3]
  4. you can use this array to construct the GQL call
  5. if more items are detected later, it repeats from step 1

On fast connections maybe batchedFunction will call the function with 25 items, on slower connections it'll make multiple API calls, that's ok. The difference is that no element risks being forgotten + it always happens as soon as possible, regardless of your connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, that was I thought.
I'm just wondering is there existing some utility function: batchedFunction, and the batchedFunction's throttle (or debounce) time.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean? It does exist and we're already using it. We don't need to change it at this point https://github.com/Richienb/batched-function

source/features/conflict-marker.tsx Outdated Show resolved Hide resolved
@fregante fregante added the bug label Aug 19, 2023
@fregante
Copy link
Member

Nice, that looks pretty simple to implement in more features

Test URLs
https://github.com/pulls
https://github.com/refined-github/sandbox/issues?q=conflict
https://github.com/kubernetes/kubernetes/milestone/62
Copy link
Member

Choose a reason for hiding this comment

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

This section exists to simplify testing so we should try to use URLs that do and always will include the feature. Our sandbox repo is where these scenarios exist and can be created.

);
}
}
}

function init(signal: AbortSignal): void {
observe('.js-issue-row:has(.octicon-git-pull-request.color-fg-open) a.js-navigation-open', batchedFunction(addIcon, {delay: 100}), {signal});
Copy link
Member

Choose a reason for hiding this comment

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

Without a delay it's too fast even for high-speed internet:

Screenshot

100ms seems to fix it:

Screenshot 1

We have 2 more functions that use batchedFunction and likely suffer the same issue. PR welcome in that case too, inlining the batchedFunction() call this way

Ridiculous that the rule wouldn't work until the function expression was turned into a declaration
@fregante fregante enabled auto-merge (squash) August 24, 2023 11:57
@fregante fregante merged commit 31d0f77 into refined-github:main Aug 24, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

conflict-marker duplicated
2 participants