Skip to content

ERA-9761: Add page_size to event feed requests #1123

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

ERA-9761: Add page_size to event feed requests #1123

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f60754
Add page_size to event feed requests
luixlive May 22, 2024
f307b12
Add include_updates=false to the feed fetch and load every single rep…
luixlive May 24, 2024
0bf3a01
ERA-9761: Add more query params narrow down the response size in the …
luixlive May 30, 2024
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
13 changes: 7 additions & 6 deletions src/ReportManager/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import useNavigate from '../hooks/useNavigate';
import { uuid } from '../utils/string';

import DelayedUnmount from '../DelayedUnmount';
import LoadingOverlay from '../LoadingOverlay';
import ReportDetailView from './ReportDetailView';

import styles from './styles.module.scss';
Expand Down Expand Up @@ -90,10 +91,10 @@ const ReportManager = ({ onReportBeingAdded }) => {
}, [onCancelAddedReport]);

useEffect(() => {
if (isNewReport || eventStore[reportId]) {
if (isNewReport) {
setIsLoadingReport(false);
}
}, [eventStore, isNewReport, reportId]);
}, [isNewReport]);

useEffect(() => {
if (isNewReport) {
Expand All @@ -109,24 +110,24 @@ const ReportManager = ({ onReportBeingAdded }) => {
}, [isNewReport, location.pathname, location.search, location.state, navigate, newReportTemporalId, reportType]);

useEffect(() => {
if (!isNewReport && !eventStore[reportId]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer check if the event is in the store, cause probably it is if it was loaded by the feed, but it won't have the updates property.

Copy link
Collaborator

@JoshuaVulcan JoshuaVulcan May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't mind this, but it's a little blunt and takes away an (overall helpful) optimization. Clicking any previously-viewed event will cause duplicate requests. Could we eval with a function to test if the event's data structure is complete and only fetch if not?

if (!isNewReport) {
setIsLoadingReport(true);
dispatch(fetchEvent(reportId))
.then(() => setIsLoadingReport(false))
.catch(() => navigate(`/${TAB_KEYS.EVENTS}`, { replace: true }));
}
}, [dispatch, eventStore, isNewReport, navigate, reportId]);
}, [dispatch, isNewReport, navigate, reportId]);

return <TrackerContext.Provider value={reportTracker}>
{shouldRenderReportDetailView && <ReportDetailView
{shouldRenderReportDetailView ? <ReportDetailView
formProps={navigationData?.formProps}
isNewReport={isNewReport}
key={reportId} // This resets component state when the id changes
newReportTypeId={newReportTypeId}
onAddReport={onAddReport}
reportData={reportData}
reportId={reportId}
/>}
/> : <LoadingOverlay />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a loading spinner


<DelayedUnmount isMounted={showAddedReport}>
<ReportDetailView
Expand Down
4 changes: 2 additions & 2 deletions src/SideBar/useReportsFeed/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const useReportsFeed = () => {
return isEqual(restEventFilter, INITIAL_FILTER_STATE);
}, [eventFilter]);
const eventParams = useRef(calcEventFilterForRequest(
{ params: { exclude_contained: shouldExcludeContained }, format: 'object' },
{ params: { exclude_contained: shouldExcludeContained, include_updates: false, page_size: 25 }, format: 'object' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We exclude the updates property from the feed request and make sure to only load 25 events

Copy link
Collaborator

Choose a reason for hiding this comment

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

include_related_events, include_notes, include_details, and include_files are also available as parameters here. Should we slim this down even further?

feedSort
));

Expand Down Expand Up @@ -61,7 +61,7 @@ const useReportsFeed = () => {
}, [geoResrictedUserLocationCoords, loadFeedEvents]);

useEffect(() => {
const params = {};
const params = { include_updates: false, page_size: 25 };
if (shouldExcludeContained) {
params.exclude_contained = true;
}
Expand Down