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

Throttle last visited calls. #1930

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions packages/chrome/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
"url": "https://github.com/RedHatInsights/frontend-components/issues"
},
"homepage": "https://github.com/RedHatInsights/frontend-components/tree/master/packages/chrome#readme",
"dependencies": {
"lodash": "^4.17.21"
},
"peerDependencies": {
"@scalprum/react-core": "^0.5.1",
"react": "^18.2.0",
Expand Down
18 changes: 8 additions & 10 deletions packages/chrome/src/ChromeProvider/ChromeProvider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import { PluginStore } from '@openshift/dynamic-plugin-sdk';
import ChromeProvider from './ChromeProvider';
import * as fetch from '../utils/fetch';

const flushPromises = () => new Promise(setImmediate);

describe('ChromeProvider', () => {
const getSpy = jest.spyOn(fetch, 'get');
const postSpy = jest.spyOn(fetch, 'post');
Expand All @@ -34,6 +32,7 @@ describe('ChromeProvider', () => {
});

test('should post new data on title change', async () => {
jest.useFakeTimers();
getSpy.mockResolvedValueOnce([]);
postSpy.mockResolvedValue(['/', '/bar']);
const DocumentMutator = () => {
Expand Down Expand Up @@ -75,17 +74,19 @@ describe('ChromeProvider', () => {
screen.getByText('/foo/bar').click();
});

// debounce timer
// wait for calls to be finished
await act(async () => {
await flushPromises();
await jest.advanceTimersByTime(5001);
});
expect(postSpy).toHaveBeenCalledTimes(2);
// should be called anly once because of the debounce
expect(postSpy).toHaveBeenCalledTimes(1);
expect(postSpy).toHaveBeenLastCalledWith('/api/chrome-service/v1/last-visited', {
pathname: '/foo/bar',
title: 'Foo title',
bundle: 'bundle-title',
});
});
}, 10000);

test('should not update state on mount if received error response', async () => {
const errorResponse = { errors: [{ status: 404, meta: { response_by: 'gateway' }, detail: 'Undefined Insights application' }] };
Expand Down Expand Up @@ -116,11 +117,8 @@ describe('ChromeProvider', () => {
);
});

expect(consoleSpy).toHaveBeenCalledTimes(2);
expect(consoleSpy.mock.calls).toEqual([
['Unable to update last visited pages!', errorResponse],
['Unable to initialize ChromeProvider!', errorResponse],
]);
expect(consoleSpy).toHaveBeenCalledTimes(1);
expect(consoleSpy.mock.calls).toEqual([['Unable to initialize ChromeProvider!', errorResponse]]);
consoleSpy.mockRestore();
});
});
22 changes: 16 additions & 6 deletions packages/chrome/src/ChromeProvider/ChromeProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,33 @@ import React, { useEffect, useRef, useState } from 'react';
import { useScalprum } from '@scalprum/react-core';
import { ChromeAPI } from '@redhat-cloud-services/types';
import { useLocation } from 'react-router-dom';
import debounce from 'lodash/debounce';

import { ChromeContext } from '../ChromeContext';
import chromeState, { LastVisitedPage, UserIdentity } from './chromeState';
import { IDENTITY_URL, LAST_VISITED_URL, get, post } from '../utils/fetch';

const getUserIdentity = () => get<UserIdentity>(IDENTITY_URL);
const postDataDebounced = debounce(async (pathname: string, title: string, bundle: string) => {
const data = await post<LastVisitedPage[], { pathname: string; title: string; bundle: string }>(LAST_VISITED_URL, {
pathname,
title,
bundle,
});
return data;
// count page as visited after 5 second of being on the page
// should help limit number of API calls
}, 5000);

const useLastPageVisitedUploader = (providerState: ReturnType<typeof chromeState>) => {
const scalprum = useScalprum<{ initialized: boolean; api: { chrome: ChromeAPI } }>();
const { pathname } = useLocation();
const postData = async (pathname: string, title: string, bundle: string) => {
try {
const data = await post<LastVisitedPage[], { pathname: string; title: string; bundle: string }>(LAST_VISITED_URL, {
pathname,
title,
bundle,
});
providerState.setLastVisited(data);
const data = await postDataDebounced(pathname, title, bundle);
if (data) {
providerState.setLastVisited(data);
}
} catch (error) {
console.error('Unable to update last visited pages!', error);
}
Expand Down Expand Up @@ -59,6 +68,7 @@ const useLastPageVisitedUploader = (providerState: ReturnType<typeof chromeState
}
return () => {
titleObserver?.disconnect();
postDataDebounced?.cancel();
};
}, [pathname]);
};
Expand Down
2 changes: 1 addition & 1 deletion packages/chrome/src/ChromeProvider/chromeState.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('chromeState', () => {
expect(state.getState().lastVisitedPages).toEqual(lastVisitedPayload);
});

test('should correctly update favrite pages data via dedicated callback', () => {
test('should correctly update favorite pages data via dedicated callback', () => {
state.setFavoritePages([{ pathname: 'favorite', favorite: true }]);
expect(state.getState().favoritePages).toEqual([{ pathname: 'favorite', favorite: true }]);
});
Expand Down