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

Tweaks for integrations #2869

Merged
merged 29 commits into from
Aug 30, 2023
Merged

Tweaks for integrations #2869

merged 29 commits into from
Aug 30, 2023

Conversation

teodosii
Copy link
Member

@teodosii teodosii commented Aug 23, 2023

What this PR does

This PR adds a few tweaks to the integrations page

  • prevent calling /counters on the integration page, instead call it with counters/[id]
  • call to /counters will be done only once when we load all the integrations and prevent calling it again once we switch pages. Prior to this change every page resulted in a call to /counters, and due to the fact the request took on average 3-5s it was being stalled by the browser on subsequent calls.
  • Call to /connected_contact_points is now conditioned by the integration type
  • No more page flicking when going back from integration page to the list of integrations
  • No more page flickering when switching pages in table view if there's more requests ongoing
  • Prevent discarding filters when going back from Integration view to the table view
  • Fixed routes not moving up/down
  • Fixed routes flickering when adding/removing a route

These changes should result in slightly loader times because /counters was time-consuming request, and less UX issues.

Which issue(s) this PR fixes

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@teodosii teodosii requested a review from a team August 23, 2023 13:51
@teodosii teodosii added the pr:no public docs Added to a PR that does not require public documentation updates label Aug 23, 2023
results: results.map((item: User) => item.pk),
};

return response;
Copy link
Member Author

Choose a reason for hiding this comment

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

this part is not touched, I only removed the wrapped new Promise (...) and instead returned the response which is a Promise too.

@@ -697,6 +692,17 @@ class Integration extends React.Component<IntegrationProps, IntegrationState> {
})
.finally(() => this.setState({ isLoading: false }));
}

async loadContactPointsMaybe(id: AlertReceiveChannel['id']) {
const { alertReceiveChannelStore } = this.props.store;
Copy link
Member Author

Choose a reason for hiding this comment

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

the page will not wait until contact points are loaded, but instead render them when they're available so that we don't slow down the page load.

@@ -17,6 +17,9 @@ export class FiltersStore extends BaseStore {
@observable.shallow
public values: { [page: string]: FiltersValues } = {};

@observable
public isLoading = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

easier to track now if the filters are loading or not

Copy link
Contributor

Choose a reason for hiding this comment

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

isLoading===true is the same as store.filters.options[page]===undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

so don't see the reason of having isLoading flag

@@ -697,6 +705,17 @@ class Integration extends React.Component<IntegrationProps, IntegrationState> {
})
.finally(() => this.setState({ isLoading: false }));
}

async loadExtraDataMaybe(id: AlertReceiveChannel['id']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just loadExtraData is better

@@ -79,6 +80,9 @@ export class RootBaseStore {
@observable
incidentsPage: any = this.initialQuery.p ? Number(this.initialQuery.p) : 1;

@observable
currentPage: { [key: string]: number } = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use src/pages/index.tsx -> getMatchedPage to get current page

Copy link
Member Author

Choose a reason for hiding this comment

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

this gets you the page number, not the page string

return alertReceiveChannelStore
.updatePaginatedItems(integrationsFilters, newPage, false, () => this.invalidateRequestFn(newPage))
.then(() => {
store.setCurrentPage(PAGE.Integrations, newPage);
Copy link
Contributor

@maskin25 maskin25 Aug 30, 2023

Choose a reason for hiding this comment

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

did not get why do we need store.setCurrentPage... to navigate we use history.push(${PLUGIN_ROOT}/fooor react router links, to get current page we can usegetMatchedPage`

Copy link
Member Author

Choose a reason for hiding this comment

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

store.setCurrentPage is not for navigation, is for updating the store to know which was the last page we hit (the page num, not the page string)

@teodosii teodosii added this pull request to the merge queue Aug 30, 2023
Merged via the queue into dev with commit 26dee30 Aug 30, 2023
@teodosii teodosii deleted the rares/2843 branch August 30, 2023 11:11
brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

This PR adds a few tweaks to the integrations page

- prevent calling `/counters` on the integration page, instead call it
with `counters/[id]`
- call to `/counters` will be done only once when we load all the
integrations and prevent calling it again once we switch pages. Prior to
this change every page resulted in a call to `/counters`, and due to the
fact the request took on average `3-5s` it was being stalled by the
browser on subsequent calls.
- Call to `/connected_contact_points` is now conditioned by the
integration type
- No more page flicking when going back from integration page to the
list of integrations
- No more page flickering when switching pages in table view if there's
more requests ongoing
- Prevent discarding filters when going back from Integration view to
the table view
- Fixed routes not moving up/down
- Fixed routes flickering when adding/removing a route

These changes should result in slightly loader times because `/counters`
was time-consuming request, and less UX issues.

## Which issue(s) this PR fixes

- described at #2843 but
incompletely

## Checklist

- [ ] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required)
- [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants