front: fix operational points names display in itinerary #10949

RomainValls · 2025-02-25T09:25:12Z

The title says the issue happened after a viriato import.
In fact, it was happening with all types of files during import when the name wasn't given in the file.

codecov-commenter · 2025-02-25T09:26:22Z

⚠️ 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 80.89%. Comparing base (4e88804) to head (c8c2869).
Report is 4 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   #10949      +/-   ##
==========================================
+ Coverage   80.86%   80.89%   +0.02%     
==========================================
  Files        1118     1119       +1     
  Lines      112736   112894     +158     
  Branches      759      759              
==========================================
+ Hits        91164    91322     +158     
  Misses      21517    21517              
  Partials       55       55

Flag	Coverage Δ
editoast	`72.61% <ø> (ø)`
front	`89.91% <100.00%> (+0.02%)`	⬆️
gateway	`2.49% <ø> (ø)`
osrdyne	`2.53% <ø> (ø)`
railjson_generator	`87.58% <ø> (ø)`
tests	`88.82% <ø> (ø)`

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.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

clarani

Thank you for your PR @RomainValls 🙏

It's exactly what should be done, but I think it might be better to move this code in usePathfinding, what do you think ?
If there is no pathfindingInput, then we should call your code to retrieve the additional data of the path steps, even if we can't launch a pathfinding

front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx

Synar

Nice work, this was an annoying issue indeed ^^

front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx

emersion · 2025-03-03T14:18:42Z

front/src/modules/pathfinding/hooks/usePathfinding.ts

+          name,
+        };
+      });
+      dispatch(updatePathSteps(updatedSteps));


As a more general comment: I don't really like the pattern where a useEffect() listens for pathSteps changes, then transforms the data and overwrites path steps again by calling updatePathSteps(). It's very complicated to reason about the data flow (since pathSteps may be mutated basically anywhere in the app) and it might result in infinite feedback loops.

In fact, doesn't this PR make the app search for OPs in an infinite loop?

I see a useCallback, not a useEffect, so it should be fine, beyond redefining the function often. Unless you're talking about a different useEffect deeper in the code.

I thought the callback was invoked from a useEffect(), but it seems like that's not the case.

emersion · 2025-03-03T14:20:22Z

front/src/modules/pathfinding/hooks/usePathfinding.ts

+    if (!infraId || !pathSteps.length) return;
+
+    const searchPayload = buildOpSearchQuery(infraId, [
+      { path: pathSteps.filter((step): step is PathStep => step !== null) } as TrainScheduleResult,


I think instead of casting (and sending a value which isn't actually a TrainScheduleResult), we can adapt buildOpSearchQuery() to take as input a list of path steps instead of TrainScheduleResult[] (ideally in a separate commit before this call is added here).

emersion · 2025-03-03T14:24:45Z

front/src/modules/pathfinding/hooks/usePathfinding.ts

+          acc[`87${op.ci}`] = op.name;
+          acc[op.trigram] = op.name;
+          acc[op.obj_id] = op.name;


This can result in conflicts: OP IDs might collide with trigrams and UICs.

Moreover, the input list may contain multiple path steps with the same trigram/UIC, but with a different secondary_code.

I agree that having 3 dicts/a struct of dicts would be cleaner to avoid collisions, though I'm unsure they're possible in practice.

For the same primary identifier but different secondary code case, won't these steps share the same name though?

They're maybe not happening with the France infrastructure, but may happen if another country decides to use numerical IDs for OP IDs for instance.

For the same primary identifier but different secondary code case, won't these steps share the same name though?

I don't believe that's guaranteed by the data model. Each OP object is independent in the database and has its own name.

(I don't think we need any kind of map/record here. We can just iterate over all results instead. The number of results should be low™: it cannot exceed the number of path steps.)

Synar · 2025-03-04T06:40:53Z

front/src/modules/pathfinding/hooks/usePathfinding.ts

+    } catch (error) {
+      console.error('Error fetching operational points:', error);
+    }
+  }, [infraId, pathSteps]);


The caller of this function takes pathSteps as an argument, not a dependency. I believe we should either do the same (using the functional form of set state if we really want to be cautious), or update the dependency array of the caller.

Synar · 2025-03-04T06:44:00Z

front/src/modules/pathfinding/hooks/usePathfinding.ts

+          acc[`87${op.ci}`] = op.name;
+          acc[op.trigram] = op.name;
+          acc[op.obj_id] = op.name;


I agree that having 3 dicts/a struct of dicts would be cleaner to avoid collisions, though I'm unsure they're possible in practice.

For the same primary identifier but different secondary code case, won't these steps share the same name though?

Synar · 2025-03-04T06:48:33Z

front/src/modules/pathfinding/hooks/usePathfinding.ts

+
+      const opMap = results.reduce(
+        (acc, op) => {
+          acc[`87${op.ci}`] = op.name;


Feel free to ignore, but I'm curious, is the 87 in front of a ci to give an uic a sncf thing? Is it something general?

Yes it's a sncf thing :/ the pr's uic is 87 + the pr's ci in France (87 is an id for SNCF, see https://fr.wikipedia.org/wiki/Liste_des_codes_pays_UIC)

Thanks! So ideally we would need to know the op's country in order to correctly match with a path step, but that seems painful.

Another stupid question way beyond this pr, but why do we use a ci to identify each op and not directly an uic in the first place?

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

…cheduleresult

RomainValls requested a review from a team as a code owner February 25, 2025 09:25

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

RomainValls requested review from Synar and clarani February 25, 2025 09:25

RomainValls self-assigned this Feb 25, 2025

RomainValls force-pushed the rvs/front-fix-op-name-display branch from 75a99d4 to 4cd0d42 Compare February 25, 2025 09:26

clarani reviewed Feb 25, 2025

View reviewed changes

emersion reviewed Feb 25, 2025

View reviewed changes

front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx Outdated Show resolved Hide resolved

Synar reviewed Feb 25, 2025

View reviewed changes

front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx Outdated Show resolved Hide resolved

front/src/modules/pathfinding/components/Itinerary/DisplayItinerary/Vias.tsx Outdated Show resolved Hide resolved

RomainValls force-pushed the rvs/front-fix-op-name-display branch from b1b4715 to 5653974 Compare February 27, 2025 16:24

RomainValls requested review from emersion, Synar and clarani February 27, 2025 16:26

emersion reviewed Mar 3, 2025

View reviewed changes

Synar reviewed Mar 4, 2025

View reviewed changes

RomainValls added 2 commits March 17, 2025 15:00

front: fix operational points names display in itinerary

bf57743

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

front: use buildopsearchquery to retrieve opnames in usepathfinding

09d32d8

RomainValls force-pushed the rvs/front-fix-op-name-display branch from 5653974 to 09d32d8 Compare March 17, 2025 14:00

front: adapt buildopsearchquery to receive either pathsteps or trains…

c8c2869

…cheduleresult

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

front: fix operational points names display in itinerary #10949

front: fix operational points names display in itinerary #10949

RomainValls commented Feb 25, 2025

codecov-commenter commented Feb 25, 2025 •

edited

Loading

clarani left a comment

Synar left a comment

emersion Mar 3, 2025

Synar Mar 4, 2025

emersion Mar 4, 2025

emersion Mar 3, 2025

emersion Mar 3, 2025

Synar Mar 4, 2025

emersion Mar 4, 2025

emersion Mar 4, 2025

Synar Mar 4, 2025

Synar Mar 4, 2025

Synar Mar 4, 2025

clarani Mar 5, 2025

Synar Mar 5, 2025 •

edited

Loading

front: fix operational points names display in itinerary #10949

Are you sure you want to change the base?

front: fix operational points names display in itinerary #10949

Conversation

RomainValls commented Feb 25, 2025

codecov-commenter commented Feb 25, 2025 • edited Loading

Codecov Report

clarani left a comment

Choose a reason for hiding this comment

Synar left a 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

Synar Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Feb 25, 2025 •

edited

Loading

Synar Mar 5, 2025 •

edited

Loading