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.

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

front: call pathfinding only once #9733

Closed
wants to merge 1 commit into from
Closed

front: call pathfinding only once #9733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 30 additions & 3 deletions front/src/modules/pathfinding/hooks/usePathfinding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import type { PathStep } from 'reducers/osrdconf/types';
import { useAppDispatch } from 'store';
import { isEmptyArray } from 'utils/array';
import { castErrorToFailure } from 'utils/error';
import { usePrevious } from 'utils/hooks/state';

import useInfraStatus from './useInfraStatus';

Expand Down Expand Up @@ -150,17 +151,28 @@ export const usePathfinding = (
) => {
const { t } = useTranslation(['operationalStudies/manageTrainSchedule']);
const dispatch = useAppDispatch();
const { getInfraID, getOrigin, getDestination, getVias, getPathSteps, getPowerRestriction } =
useOsrdConfSelectors();
const {
getInfraID,
getOrigin,
getDestination,
getVias,
getPathSteps,
getPowerRestriction,
getPathStepsCompletedWithPFResult,
} = useOsrdConfSelectors();
const infraId = useSelector(getInfraID, isEqual);
const origin = useSelector(getOrigin, isEqual);
const destination = useSelector(getDestination, isEqual);
const vias = useSelector(getVias(), isEqual);
const pathSteps = useSelector(getPathSteps);
const pathStepsCompletedWithPF = useSelector(getPathStepsCompletedWithPFResult);
const powerRestrictions = useSelector(getPowerRestriction);
const { infra, reloadCount, setIsInfraError } = useInfraStatus();
const { rollingStock } = useStoreDataForRollingStockSelector();

const previousOrigin = usePrevious(origin);
const previousDestination = usePrevious(destination);

const initializerArgs = {
origin,
destination,
Expand Down Expand Up @@ -196,6 +208,11 @@ export const usePathfinding = (
};

useEffect(() => {
// ignore update made automatically to vias from pathfinding result
// we want only the changes asked manually by user
if (pathStepsCompletedWithPF) {
return;
}
if (isPathfindingInitialized) {
pathfindingDispatch({
type: 'VIAS_CHANGED',
Expand All @@ -209,6 +226,12 @@ export const usePathfinding = (
}, [vias]);

useEffect(() => {
if (
(previousOrigin !== origin || previousDestination !== destination) &&
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is enough: an intermediary waypoint might've changed too, or rolling stock, or something else…

But, what is the root cause? Why does pathfinding run twice, which state change causes this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

intermediary waypoint don’t do aything in this useEffect, and rolling don’t create the bug.

Root cause is pathfinding updating vias when it succeeds. So we need to ignore exactly one call made and resume listening after, which is what pathStepsCompletedWithPF does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably gonna change after the refacto #9924

pathStepsCompletedWithPF
) {
return;
}
if (isPathfindingInitialized) {
pathfindingDispatch({
type: 'PATHFINDING_PARAM_CHANGED',
Expand Down Expand Up @@ -339,7 +362,11 @@ export const usePathfinding = (
);
}
dispatch(
updatePathSteps({ pathSteps: updatedPathSteps, resetPowerRestrictions: true })
updatePathSteps({
pathSteps: updatedPathSteps,
resetPowerRestrictions: true,
completedWithPFResult: true,
})
);

const allWaypoints = upsertPathStepsInOPs(
Expand Down
10 changes: 10 additions & 0 deletions front/src/reducers/osrdconf/osrdConfCommon/__tests__/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
const state = store.getState()[slice.name];
expect(state.pathSteps[1]?.stopFor).toEqual('PT60S');
expect(state.pathSteps[1]?.stopType).toEqual('serviceStop');
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

it('should handle updateFeatureInfoClick', () => {
Expand Down Expand Up @@ -237,6 +238,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
defaultStore.dispatch(slice.actions.updatePathSteps({ pathSteps }));
const state = defaultStore.getState()[slice.name];
expect(state.pathSteps).toEqual(pathSteps);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

it('should handle updateOrigin', () => {
Expand All @@ -247,6 +249,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
defaultStore.dispatch(slice.actions.updateOrigin(newOrigin));
const state = defaultStore.getState()[slice.name];
expect(state.pathSteps[0]).toEqual(newOrigin);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

it('should handle updateDestination', () => {
Expand All @@ -255,6 +258,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
defaultStore.dispatch(slice.actions.updateDestination(newDestination));
const state = defaultStore.getState()[slice.name];
expect(last(state.pathSteps)).toEqual(newDestination);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

it('should handle deleteItinerary', () => {
Expand All @@ -265,6 +269,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
store.dispatch(slice.actions.deleteItinerary());
const state = store.getState()[slice.name];
expect(state.pathSteps).toEqual([null, null]);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

it('should handle clearVias', () => {
Expand All @@ -275,6 +280,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
store.dispatch(slice.actions.clearVias());
const state = store.getState()[slice.name];
expect(state.pathSteps).toEqual([pathSteps[0], last(pathSteps)]);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

it('should handle deleteVia', () => {
Expand All @@ -285,6 +291,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
store.dispatch(slice.actions.deleteVia(0));
const state = store.getState()[slice.name];
expect(state.pathSteps).toEqual(removeElementAtIndex(pathSteps, 1));
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

describe('should handle addVia', () => {
Expand All @@ -305,6 +312,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
store.dispatch(slice.actions.addVia({ newVia: paris, pathProperties }));
const state = store.getState()[slice.name];
expect(state.pathSteps).toStrictEqual([null, parisNoPosition, strasbourg]);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

it('should handle insertion for a route with no existing via and no destination', () => {
Expand Down Expand Up @@ -372,6 +380,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
store.dispatch(slice.actions.moveVia(pathSteps, 0, 2));
const state = store.getState()[slice.name];
expect(state.pathSteps).toStrictEqual([brest, lemans, paris, rennes, strasbourg]);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});

describe('should handle upsertViaFromSuggestedOP', () => {
Expand Down Expand Up @@ -442,6 +451,7 @@ const testCommonConfReducers = (slice: OperationalStudiesConfSlice | StdcmConfSl
store.dispatch(slice.actions.upsertViaFromSuggestedOP(newVia));
const state = store.getState()[slice.name];
expect(state.pathSteps).toEqual([brest, rennes, updatedVia, paris, strasbourg]);
expect(state.pathStepsCompletedWithPFResult).toEqual(false);
});
});

Expand Down
24 changes: 22 additions & 2 deletions front/src/reducers/osrdconf/osrdConfCommon/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export const defaultCommonConf: OsrdConfState = {
featureInfoClick: { displayPopup: false },
// Corresponds to origin and destination not defined
pathSteps: [null, null],
pathStepsCompletedWithPFResult: false,
rollingStockComfort: 'STANDARD' as const,
startTime: new Date().toISOString(),
};
Expand Down Expand Up @@ -75,7 +76,11 @@ interface CommonConfReducers<S extends OsrdConfState> extends InfraStateReducers
['updateFeatureInfoClick']: CaseReducer<S, PayloadAction<S['featureInfoClick']>>;
['updatePathSteps']: CaseReducer<
S,
PayloadAction<{ pathSteps: S['pathSteps']; resetPowerRestrictions?: boolean }>
PayloadAction<{
pathSteps: S['pathSteps'];
resetPowerRestrictions?: boolean;
completedWithPFResult?: boolean;
}>
>;
['deleteItinerary']: CaseReducer<S>;
['clearVias']: CaseReducer<S>;
Expand Down Expand Up @@ -165,6 +170,7 @@ export function buildCommonConfReducers<S extends OsrdConfState>(): CommonConfRe
}
return pathStep;
});
state.pathStepsCompletedWithPFResult = false;
},
updateGridMarginBefore(state: Draft<S>, action: PayloadAction<S['gridMarginBefore']>) {
state.gridMarginBefore = action.payload;
Expand All @@ -178,23 +184,31 @@ export function buildCommonConfReducers<S extends OsrdConfState>(): CommonConfRe
},
updatePathSteps(
state: Draft<S>,
action: PayloadAction<{ pathSteps: S['pathSteps']; resetPowerRestrictions?: boolean }>
action: PayloadAction<{
pathSteps: S['pathSteps'];
resetPowerRestrictions?: boolean;
completedWithPFResult?: boolean;
}>
) {
state.pathSteps = action.payload.pathSteps;
state.pathStepsCompletedWithPFResult = action.payload.completedWithPFResult || false;
if (action.payload.resetPowerRestrictions) {
state.powerRestriction = [];
}
},
deleteItinerary(state: Draft<S>) {
state.pathSteps = [null, null];
state.pathStepsCompletedWithPFResult = false;
},
clearVias(state: Draft<S>) {
state.pathSteps = [state.pathSteps[0], state.pathSteps[state.pathSteps.length - 1]];
state.pathStepsCompletedWithPFResult = false;
},
// Use this action in the via list, not the suggested op list
deleteVia(state: Draft<S>, action: PayloadAction<number>) {
// Index takes count of the origin in the array
state.pathSteps = removeElementAtIndex(state.pathSteps, action.payload + 1);
state.pathStepsCompletedWithPFResult = false;
},
// Use this action only to via added by click on map
addVia(
Expand All @@ -209,10 +223,12 @@ export function buildCommonConfReducers<S extends OsrdConfState>(): CommonConfRe
action.payload.newVia,
action.payload.pathProperties
);
state.pathStepsCompletedWithPFResult = false;
},
moveVia: {
reducer: (state: Draft<S>, action: PayloadAction<S['pathSteps']>) => {
state.pathSteps = action.payload;
state.pathStepsCompletedWithPFResult = false;
},
prepare: (vias: S['pathSteps'], from: number, to: number) => {
const newVias = Array.from(vias);
Expand All @@ -227,11 +243,13 @@ export function buildCommonConfReducers<S extends OsrdConfState>(): CommonConfRe
// from the suggested via modal
upsertViaFromSuggestedOP(state: Draft<S>, action: PayloadAction<SuggestedOP>) {
upsertPathStep(state.pathSteps, action.payload);
state.pathStepsCompletedWithPFResult = false;
},
upsertSeveralViasFromSuggestedOP(state: Draft<S>, action: PayloadAction<SuggestedOP[]>) {
action.payload.forEach((suggestedOp) => {
upsertPathStep(state.pathSteps, suggestedOp);
});
state.pathStepsCompletedWithPFResult = false;
},
updateRollingStockComfort(state: Draft<S>, action: PayloadAction<S['rollingStockComfort']>) {
state.rollingStockComfort = action.payload;
Expand All @@ -249,6 +267,7 @@ export function buildCommonConfReducers<S extends OsrdConfState>(): CommonConfRe
}
: null;
state.pathSteps = updateOriginPathStep(state.pathSteps, newPoint, true);
state.pathStepsCompletedWithPFResult = false;
},
updateDestination(state: Draft<S>, action: PayloadAction<ArrayElement<S['pathSteps']>>) {
const prevDestinationArrivalType = state.pathSteps.at(-1)?.arrivalType;
Expand All @@ -260,6 +279,7 @@ export function buildCommonConfReducers<S extends OsrdConfState>(): CommonConfRe
}
: null;
state.pathSteps = updateDestinationPathStep(state.pathSteps, newPoint, true);
state.pathStepsCompletedWithPFResult = false;
},
};
}
Expand Down
1 change: 1 addition & 0 deletions front/src/reducers/osrdconf/osrdConfCommon/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const buildCommonConfSelectors = <ConfState extends OsrdConfState>(
getPowerRestriction: makeOsrdConfSelector('powerRestriction'),
getFeatureInfoClick: makeOsrdConfSelector('featureInfoClick'),
getPathSteps,
getPathStepsCompletedWithPFResult: makeOsrdConfSelector('pathStepsCompletedWithPFResult'),
getOrigin: (state: RootState) => {
const pathSteps = getPathSteps(state);
return pathSteps[0];
Expand Down
1 change: 1 addition & 0 deletions front/src/reducers/osrdconf/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export interface OsrdConfState extends InfraState {
gridMarginAfter?: number;
featureInfoClick: { displayPopup: boolean; feature?: Feature; coordinates?: number[] };
pathSteps: (PathStep | null)[];
pathStepsCompletedWithPFResult: boolean;
rollingStockComfort: Comfort;
// Format ISO 8601
startTime: string;
Expand Down
Loading