Skip to content
Draft
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
36 changes: 30 additions & 6 deletions static/js/stat_var_hierarchy/stat_var_group_node.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ export class StatVarGroupNode extends React.Component<
context: ContextType;
// the list of entities for which data fetch has begun, but not finished.
dataFetchingEntities: NamedNode[];
// Abort controller to cancel any in-flight requests on re-render
private _abortController: AbortController;

constructor(props: StatVarGroupNodePropType) {
super(props);
Expand Down Expand Up @@ -130,6 +132,13 @@ export class StatVarGroupNode extends React.Component<
}
}

componentWillUnmount(): void {
if (this._abortController) {
// Cancel any existing requests on unmount
this._abortController.abort();
}
}

componentDidUpdate(prevProps: StatVarGroupNodePropType): void {
const newSelectionCount = this.getSelectionCount();
const newState = { ...this.state };
Expand Down Expand Up @@ -260,6 +269,13 @@ export class StatVarGroupNode extends React.Component<
}

private fetchData(): void {
if (this._abortController) {
// Cancel any existing requests
this._abortController.abort();
}
this._abortController = new AbortController();
const signal = this._abortController.signal;

const entityList = this.props.entities;
this.dataFetchingEntities = this.props.entities;
let numEntitiesExistence = this.props.numEntitiesExistence;
Expand All @@ -272,11 +288,15 @@ export class StatVarGroupNode extends React.Component<
numEntitiesExistence = entityDcids.length;
}
axios
.post("/api/variable-group/info", {
dcid: this.props.data.id,
entities: entityDcids,
numEntitiesExistence,
})
.post(
"/api/variable-group/info",
{
dcid: this.props.data.id,
entities: entityDcids,
numEntitiesExistence,
},
{ signal }
)
.then((resp) => {
const data = resp.data;
const childSV: StatVarInfo[] = data["childStatVars"] || [];
Expand All @@ -290,7 +310,11 @@ export class StatVarGroupNode extends React.Component<
});
}
})
.catch(() => {
.catch((error) => {
if (axios.isCancel(error) || error.name === "AbortError") {
// Ignore abort errors
return;
}
this.dataFetchingEntities = null;
if (_.isEqual(entityList, this.props.entities)) {
this.setState({
Expand Down
87 changes: 68 additions & 19 deletions static/js/stat_var_hierarchy/stat_var_hierarchy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ export class StatVarHierarchy extends React.Component<
StatVarHierarchyPropType,
StatVarHierarchyStateType
> {
// Abort controller to cancel any in-flight data requests
private _dataAbortController: AbortController;
// Abort controller to cancel any in-flight search requests
private _searchAbortController: AbortController;

constructor(props: StatVarHierarchyPropType) {
super(props);
this.state = {
Expand All @@ -121,6 +126,17 @@ export class StatVarHierarchy extends React.Component<
this.fetchData();
}

componentWillUnmount(): void {
if (this._dataAbortController) {
// Abort any in-flight data requests
this._dataAbortController.abort();
}
if (this._searchAbortController) {
// Abort any in-flight search requests
this._searchAbortController.abort();
}
}

componentDidUpdate(prevProps: StatVarHierarchyPropType): void {
if (this.state.searchSelectionCleared) {
this.setState({ searchSelectionCleared: false });
Expand Down Expand Up @@ -257,6 +273,13 @@ export class StatVarHierarchy extends React.Component<
}

private async fetchData(): Promise<void> {
if (this._dataAbortController) {
// Abort the previous data request
this._dataAbortController.abort();
}
this._dataAbortController = new AbortController();
const signal = this._dataAbortController.signal;

loadSpinner(SV_HIERARCHY_SECTION_ID);
const entityList = this.props.entities.map((entity) => entity.dcid);
const variableGroupInfoPromises: Promise<StatVarGroupNodeType>[] =
Expand All @@ -267,11 +290,15 @@ export class StatVarHierarchy extends React.Component<
? [statVarHierarchyConfigNode.dataSourceDcid]
: [];
return axios
.post("/api/variable-group/info", {
dcid: statVarHierarchyConfigNode.dcid,
entities: [...entityList, ...dataSourceEntities],
numEntitiesExistence: this.props.numEntitiesExistence,
})
.post(
"/api/variable-group/info",
{
dcid: statVarHierarchyConfigNode.dcid,
entities: [...entityList, ...dataSourceEntities],
numEntitiesExistence: this.props.numEntitiesExistence,
},
{ signal }
)
.then((resp) => {
return resp.data;
});
Expand All @@ -283,7 +310,7 @@ export class StatVarHierarchy extends React.Component<
if (this.state.svPath && sv in this.state.svPath) {
svPath[sv] = this.state.svPath[sv];
} else {
statVarPathPromises.push(this.getPath(sv));
statVarPathPromises.push(this.getPath(sv, signal));
}
}
}
Expand Down Expand Up @@ -329,26 +356,42 @@ export class StatVarHierarchy extends React.Component<
rootSVGs,
svPath,
});
} catch {
} catch (error) {
removeSpinner(SV_HIERARCHY_SECTION_ID);
// Ignore request cancellation errors
if (axios.isCancel(error) || error.name === "AbortError") {
return;
}
this.setState({
errorMessage: "Error retrieving stat var group root nodes",
});
}
}

private onSearchSelectionChange(selection: string): void {
this.getPath(selection).then((path) => {
const searchSelectionCleared =
!_.isEmpty(this.state.focusPath) && _.isEmpty(path);
this.setState({
focus: selection,
focusPath: path,
searchSelectionCleared,
expandedPath: searchSelectionCleared ? this.state.focusPath : [],
if (this._searchAbortController) {
this._searchAbortController.abort();
}
this._searchAbortController = new AbortController();
this.getPath(selection, this._searchAbortController.signal)
.then((path) => {
const searchSelectionCleared =
!_.isEmpty(this.state.focusPath) && _.isEmpty(path);
this.setState({
focus: selection,
focusPath: path,
searchSelectionCleared,
expandedPath: searchSelectionCleared ? this.state.focusPath : [],
});
this.togglePath(selection, path, /*isSearchSelection=*/ true);
})
.catch((error) => {
if (axios.isCancel(error) || error.name === "AbortError") {
// Ignore abort errors
return;
}
console.error(error);
});
this.togglePath(selection, path, /*isSearchSelection=*/ true);
});
}
Comment on lines 371 to 395
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The _abortController is managed by fetchData and is designed to cancel requests on re-renders or component unmount. Reusing it here for search selection changes can lead to issues. For example, if a user makes two search selections quickly, the request for the first selection won't be cancelled. Also, a fetchData call could unintentionally cancel a search request.

It would be better to use a separate AbortController for search-related requests to manage their lifecycle independently. You could introduce a _searchAbortController class property, create a new controller in this method (aborting the previous one if it exists), and also abort it in componentWillUnmount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good bot. Added separate AbortControllers for data vs search fetches.


// Add or remove a stat var and its path from the state.
Expand Down Expand Up @@ -387,16 +430,22 @@ export class StatVarHierarchy extends React.Component<
}

// Get the path of a stat var from the hierarchy.
private getPath(sv: string): Promise<string[]> {
private getPath(sv: string, signal?: AbortSignal): Promise<string[]> {
if (sv == "") {
return Promise.resolve([]);
}
return axios
.get(`/api/variable/path?dcid=${encodeURIComponent(sv)}`)
.get(`/api/variable/path?dcid=${encodeURIComponent(sv)}`, { signal })
.then((resp) => {
// This is to make jest test working, should find a better way to let
// mock return new object each time.
return _.cloneDeep(resp.data).reverse();
}).catch((error) => {
if (axios.isCancel(error) || error.name === "AbortError") {
// Ignore abort errors
return;
}
console.error(error);
});
}

Expand Down
16 changes: 15 additions & 1 deletion static/js/tools/map/fetcher/all_dates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ export function useFetchAllDates(dispatch: Dispatch<ChartStoreAction>): void {
if (!contextOk) {
return;
}
// Use AbortController to cancel the request if the component unmounts
const abortController = new AbortController();
const signal = abortController.signal;

const action: ChartStoreAction = {
type: ChartDataType.ALL_DATES,
error: null,
Expand All @@ -60,6 +64,7 @@ export function useFetchAllDates(dispatch: Dispatch<ChartStoreAction>): void {
childType: placeInfo.value.enclosedPlaceType,
variable: statVar.value.dcid,
},
signal,
})
.then((resp) => {
const data = resp.data as ObservationDatesResponse;
Expand All @@ -77,10 +82,19 @@ export function useFetchAllDates(dispatch: Dispatch<ChartStoreAction>): void {
console.log("[Map Fetch] all dates");
dispatch(action);
})
.catch(() => {
.catch((error) => {
if (axios.isCancel(error) || error.name === "AbortError") {
// Ignore abort errors
return;
}
action.error = "error fetching all the dates";
dispatch(action);
});

return () => {
// Cancel the request if the component unmounts
abortController.abort();
};
}, [
display.value.showTimeSlider,
placeInfo.value.enclosingPlace.dcid,
Expand Down
16 changes: 15 additions & 1 deletion static/js/tools/map/fetcher/all_stat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ export function useFetchAllStat(dispatch: Dispatch<ChartStoreAction>): void {
if (!contextOk) {
return;
}
// Use AbortController to cancel the request if the component unmounts.
const abortController = new AbortController();
const signal = abortController.signal;

const action: ChartStoreAction = {
type: ChartDataType.ALL_STAT,
context: {
Expand Down Expand Up @@ -72,6 +76,7 @@ export function useFetchAllStat(dispatch: Dispatch<ChartStoreAction>): void {
},
paramsSerializer: stringifyFn,
headers: WEBSITE_SURFACE_HEADER,
signal,
})
.then((resp) => {
if (_.isEmpty(resp.data.data[statVar.value.dcid])) {
Expand All @@ -85,10 +90,19 @@ export function useFetchAllStat(dispatch: Dispatch<ChartStoreAction>): void {
console.log(`[Map Fetch] all stat for date: ${date}`);
dispatch(action);
})
.catch(() => {
.catch((error) => {
if (axios.isCancel(error) || error.name === "AbortError") {
// Ignore abort errors
return;
}
action.error = "error fetching all stat data";
dispatch(action);
});

return () => {
// Abort the request if the component unmounts.
abortController.abort();
};
}, [
placeInfo.value.enclosingPlace.dcid,
placeInfo.value.enclosedPlaceType,
Expand Down
27 changes: 22 additions & 5 deletions static/js/tools/map/fetcher/border_geojson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ export function useFetchBorderGeoJson(
if (!contextOk || !shouldShowBorder(placeInfo.value.enclosedPlaceType)) {
return;
}
// Use AbortController to cancel the request if the component unmounts
const abortController = new AbortController();
const signal = abortController.signal;

const action: ChartStoreAction = {
type: ChartDataType.BORDER_GEO_JSON,
error: null,
Expand All @@ -59,10 +63,14 @@ export function useFetchBorderGeoJson(
},
};
axios
.post("/api/choropleth/node-geojson", {
nodes: [placeInfo.value.enclosingPlace.dcid],
geoJsonProp: BORDER_GEOJSON_PROPERTY,
})
.post(
"/api/choropleth/node-geojson",
{
nodes: [placeInfo.value.enclosingPlace.dcid],
geoJsonProp: BORDER_GEOJSON_PROPERTY,
},
{ signal }
)
.then((resp) => {
if (_.isEmpty(resp.data)) {
action.error = "error fetching border geo json data";
Expand All @@ -71,10 +79,19 @@ export function useFetchBorderGeoJson(
}
dispatch(action);
})
.catch(() => {
.catch((error) => {
if (axios.isCancel(error) || error.name === "AbortError") {
// Ignore abort errors
return;
}
action.error = "error fetching border geo json data";
dispatch(action);
});

return () => {
// Cancel request if component unmounts
abortController.abort();
};
}, [
placeInfo.value.enclosingPlace,
placeInfo.value.enclosedPlaceType,
Expand Down
Loading
Loading