front: fix selected track display #10394

theocrsb · 2025-01-15T15:06:39Z

fix #10372
fix #10414

codecov-commenter · 2025-01-15T15:29:42Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.50%. Comparing base (9ddc06c) to head (0b3c5f8).
Report is 30 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##              dev   #10394       +/-   ##
===========================================
+ Coverage   81.61%   87.50%    +5.89%     
===========================================
  Files        1067       31     -1036     
  Lines      105535     1537   -103998     
  Branches      727        0      -727     
===========================================
- Hits        86128     1345    -84783     
+ Misses      19366      192    -19174     
+ Partials       41        0       -41

Flag	Coverage Δ
editoast	`?`
front	`?`
gateway	`?`
osrdyne	`?`
railjson_generator	`87.50% <ø> (ø)`
tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: theocrsb <[email protected]>

RomainValls · 2025-01-17T08:25:36Z

Nice fix ! It also closes #10299, there were duplicates :)

emersion · 2025-01-17T08:47:48Z

...les/trainschedule/components/ManageTrainSchedule/ManageTrainScheduleMap/ItineraryMarkers.tsx

+  pathSteps
+    .map((pathStep, index): MarkerProperties | null => {
+      const matchingOp = suggestedOP.find((op) =>
+        matchPathStepAndOp(pathStep as PathItemLocation, op)


Hm, this is dangerous… What is the reason why we can't replace or update MarkerInformation?

I just removed the cast

emersion · 2025-01-17T09:15:28Z

front/src/applications/stdcm/components/StdcmResults/StdcmResults.tsx

-            simulationPathSteps={markersInfo}
-          />
+          {infraId && (
+            <ScenarioContextProvider infraId={infraId}>


Does the new code require a scenario context? It seems a bit weird to use something specific to operational studies in STDCM.

As you suggested, I use useCachedTrackSections instead

SharglutDev · 2025-01-17T07:55:42Z

front/src/applications/stdcm/components/StdcmForm/StdcmConfig.tsx

-            simulationPathSteps={markersInfo}
-          />
+          {infra && (
+            <ScenarioContextProvider infraId={infra.id}>


I don't think it's a good idea to depend on something from another app (operational studies).

We could remove this and in ItineraryMarkers use the custom hook instead :
const { getTrackSectionsByIds } = useCachedTrackSections(infraId);

If it works, and as useCachedTrackSections will be used in both operational studies and stdcm, you can move this hook in src/utils/hooks (in a separate commit probably, before the main one).

Then you should be able to remove ScenarioContextProvider from here and StdcmResults

SharglutDev · 2025-01-17T08:18:30Z

...les/trainschedule/components/ManageTrainSchedule/ManageTrainScheduleMap/ItineraryMarkers.tsx

+        matchPathStepAndOp(pathStep as PathItemLocation, op)
+      );
+
+      if (!matchingOp) return null;


Can't me keep reduce which should allow to handle that case instead of having to do a map and then a filter ?

Personally I find map + filter more readable than reduce.

I was sure you had something to do with it 😏

Most I did was nod when Theo suggested to get rid of the reduce :P

I also find it easier to read

SharglutDev · 2025-01-17T08:19:44Z

...les/trainschedule/components/ManageTrainSchedule/ManageTrainScheduleMap/ItineraryMarkers.tsx

+
+      if (!matchingOp) return null;
+
+      if (pathStep && pathStep.coordinates) {


pathStep is always defined

Suggested change

if (pathStep && pathStep.coordinates) {

if (pathStep.coordinates) {

SharglutDev · 2025-01-17T08:27:30Z

...les/trainschedule/components/ManageTrainSchedule/ManageTrainScheduleMap/ItineraryMarkers.tsx

@@ -31,7 +35,8 @@ enum MARKER_TYPE {
 }

 type MarkerProperties = {
-  marker: MarkerInformation;
+  op: SuggestedOP;
+  pathStep: MarkerInformation;


Doesn't come from you but I still don't like to see a pathStep associated with a MarkerInformation type.

In a separate commit, you could rename and update the type MarkerInformation like this :

type PathStepMarker = Pick<PathStep, 'name' | 'coordinates' | 'metadata'>

SharglutDev · 2025-01-17T08:43:52Z

...les/trainschedule/components/ManageTrainSchedule/ManageTrainScheduleMap/ItineraryMarkers.tsx

+        if (index > 0 && index < pathSteps.length - 1) {
+          return {
+            coordinates: pathStep.coordinates,
+            type: MARKER_TYPE.VIA,
+            imageSource: showStdcmAssets ? stdcmVia : viaSVG,
+            index,
+            op: matchingOp,
+            pathStep,
+          };
+        }


Not from your PR, but you could remove the if and move the return at the end since if it's not the first or last index, it can only be a via

Suggested change

if (index > 0 && index < pathSteps.length - 1) {

return {

coordinates: pathStep.coordinates,

type: MARKER_TYPE.VIA,

imageSource: showStdcmAssets ? stdcmVia : viaSVG,

index,

op: matchingOp,

pathStep,

};

}

return {

coordinates: pathStep.coordinates,

type: MARKER_TYPE.VIA,

imageSource: showStdcmAssets ? stdcmVia : viaSVG,

index,

op: matchingOp,

pathStep,

};

SharglutDev · 2025-01-17T09:26:20Z

...les/trainschedule/components/ManageTrainSchedule/ManageTrainScheduleMap/ItineraryMarkers.tsx

@@ -117,70 +162,60 @@ const ItineraryMarkers = ({ map, simulationPathSteps, showStdcmAssets }: Itinera
        return formatPointWithNoName(markerLineCode, markerLineName, markerTrackName, markerType);
      }

-      if (!markerCoordinates) return null;
+      const { op } = markerInfo;


Can't we move this logic (no markerMetaData) directly in extractMarkerInformation ? In the function, we can check if the pathStep has no name and no metadata and add a metadata to it.
This will allow us not to add this extra op property in MarkerProperties and not to have some code duplication getMarkerDisplayInformation

github-actions bot added the area:front Work on Standard OSRD Interface modules label Jan 15, 2025

theocrsb force-pushed the tce/front/bug-wrong-track-display branch 7 times, most recently from 6ebdd08 to 91c86cb Compare January 16, 2025 17:14

theocrsb marked this pull request as ready for review January 16, 2025 17:20

theocrsb requested a review from a team as a code owner January 16, 2025 17:20

theocrsb requested review from SharglutDev and Caracol3 January 16, 2025 17:21

front: fix selected track display

aa9a0a2

Signed-off-by: theocrsb <[email protected]>

theocrsb force-pushed the tce/front/bug-wrong-track-display branch from 91c86cb to aa9a0a2 Compare January 17, 2025 08:25

emersion reviewed Jan 17, 2025

View reviewed changes

SharglutDev reviewed Jan 17, 2025

View reviewed changes

fixup! front: fix selected track display

b9316ab

theocrsb force-pushed the tce/front/bug-wrong-track-display branch from d408ed1 to b9316ab Compare January 17, 2025 11:18

fixup! front: fix selected track display

0b3c5f8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

front: fix selected track display #10394

front: fix selected track display #10394

theocrsb commented Jan 15, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading

RomainValls commented Jan 17, 2025

emersion Jan 17, 2025

theocrsb Jan 17, 2025

emersion Jan 17, 2025

theocrsb Jan 17, 2025

SharglutDev Jan 17, 2025

theocrsb Jan 17, 2025

SharglutDev Jan 17, 2025

emersion Jan 17, 2025

SharglutDev Jan 17, 2025

emersion Jan 17, 2025

theocrsb Jan 17, 2025

SharglutDev Jan 17, 2025

theocrsb Jan 17, 2025

SharglutDev Jan 17, 2025

SharglutDev Jan 17, 2025

theocrsb Jan 17, 2025

SharglutDev Jan 17, 2025


		if (!matchingOp) return null;

		if (pathStep && pathStep.coordinates) {

	if (pathStep && pathStep.coordinates) {
	if (pathStep.coordinates) {

front: fix selected track display #10394

Are you sure you want to change the base?

front: fix selected track display #10394

Conversation

theocrsb commented Jan 15, 2025 • edited Loading

codecov-commenter commented Jan 15, 2025 • edited Loading

Codecov Report

RomainValls commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theocrsb commented Jan 15, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading