Schedule segment times per period and checkpoints#1892
Open
iTzxMigz wants to merge 29 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds period- and service-scoped segment timing and checkpoint support across backend and frontend. Backend: new period types and utilities, trip bucketing by period/service, checkpoint derivation from timepoints, per-service/per-period segment aggregation, GTFS import/export changes (checkpoint-aware timepoint export), schedule generation using period/service data, and many tests. Frontend: a complete “segment times by period” modal with components, hook logic, styles, localization entries, and tests. Shared libraries: new path/segment timing types, grouping, remapping, routing helpers, and utilities for checkpoint resolution. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transition-common/src/services/schedules/Schedule.ts (1)
235-258:⚠️ Potential issue | 🟠 MajorRound the unit state on the same boundary as the trip record.
These timestamps are rounded now, but
processDeparture()still keepsunit.expectedArrivalTime = currentTime + totalTimeSecondsas a fractional value. Since availability is checked against integercurrentTime, a trip stored as arriving at08:10:00can keep the vehicle blocked until08:10:01.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/schedules/Schedule.ts` around lines 235 - 258, processDeparture() currently sets unit.expectedArrivalTime as a fractional value (currentTime + totalTimeSeconds) while trips are stored rounded, causing off-by-one availability checks; change the assignment in processDeparture() so unit.expectedArrivalTime is rounded the same way as trip records (e.g., use Math.round on the computed arrival seconds or assign the already-rounded trip.arrival_time_seconds) to ensure availability checks against integer currentTime behave consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts`:
- Around line 55-63: The code currently builds checkpointNodeIds as a
Set<string> of node IDs and keys timepoints by node ID, which mislabels repeated
nodes; change the logic in ScheduleExporter.ts to record checkpoint positions
(sequence indices) instead of node IDs: when iterating checkpoints (the
checkpoints variable) map each checkpoint's from/to occurrences to the
corresponding index(es) along newPath (use the path node list or coordinates
from newPath and newPath.getCoordinatesDistanceTraveledMeters() to determine
sequence positions) and store those indices (e.g., Set<number> or boolean array)
in the pathsById entry (replace checkpointNodeIds with checkpointPositions or
similar); then use those position indices when marking timepoints rather than
checking membership by node ID. Also update the same pattern at the other
occurrences flagged (around lines referenced for 70 and 133) so all exports key
by path position/sequence, not node ID.
In `@packages/transition-backend/src/services/gtfsImport/PathImporter.ts`:
- Around line 134-136: The timepointNodeIds array may include undefined entries
because importData.nodeIdsByStopGtfsId[st.stop_id] can be missing; update the
generation of timepointNodeIds (in the block using representativeStopTimes and
importData.nodeIdsByStopGtfsId) to skip undefined values so only valid node IDs
are collected (e.g., map then filter out undefined or flatMap conditional),
ensuring downstream checkpoint data only contains concrete node IDs.
In
`@packages/transition-common/src/services/path/__tests__/PathGeographyGenerator.test.ts`:
- Around line 1382-2159: The tests under describe 'segmentsByServiceAndPeriod
remapping on path edits' repeat the same setup/assertion pattern; refactor them
into parametric tests using test.each: extract the varying inputs
(lastNodeChange / lastWaypointChangedSegmentIndex / forceRecalculate,
pathData.nodes and data.segmentsByServiceAndPeriod mutations,
routingResult.matchings[0].legs, and expected assertions such as expected
preserved indices, new-segment ratio expectations, expected lengths and dwell
adjustments) into a table of cases, then write a single test.each that for each
row creates the TransitPathStub from the case's pathData, calls
PathGeographyUtils.prepareNodesAndWaypoints and
generatePathGeographyFromRouting, and runs the existing assertion logic via the
helper functions computeExpectedPeriodRatio and verifyPeriodAggregates; keep
unique symbols like generatePathGeographyFromRouting,
computeExpectedPeriodRatio, verifyPeriodAggregates, initialPathData, and
DEFAULT_DWELL_TIME to locate and reuse the existing code and assertions. Ensure
special cases (waypoint change, forceRecalculate, baked-in dwell, multiple
services) are separate rows and include any per-case extra expectations in small
per-row callbacks to preserve clarity.
In
`@packages/transition-common/src/services/path/__tests__/PathGeographyUtils.test.ts`:
- Around line 878-905: Convert the four standalone tests into table-driven
parametric tests using test.each for PathGeographyUtils.scaleTimesToTarget:
create a table with columns for input segments array, target value, and an
expected-check type (e.g., 'sumEquals', 'isNull', 'nonNegativeAll',
'singleValue') and expected value where applicable; then implement a single
test.each that calls PathGeographyUtils.scaleTimesToTarget with the row inputs
and performs the appropriate assertion (for 'sumEquals' assert the reduced sum
equals target, for 'isNull' assert null, for 'nonNegativeAll' assert non-null
and every element >= 0, for 'singleValue' assert non-null and first element
equals expected), so failures are isolated per row while keeping the same
coverage as the original tests.
In
`@packages/transition-common/src/services/path/__tests__/PathSegmentTimeUtils.test.ts`:
- Around line 44-88: Add tests for checkpoints where fromNodeId resolves to an
index greater than or equal to toNodeId to ensure these are treated as invalid:
in the resolveCheckpoint tests add a case like checkpoint { fromNodeId: 'nodeC',
toNodeId: 'nodeC' } and another reversed case { fromNodeId: 'nodeD', toNodeId:
'nodeB' } asserting resolveCheckpoint returns undefined; in the
resolveCheckpoints suite add a checkpoint list containing such
same-node/reversed entries and assert they are filtered out (e.g., resolved
length excludes them and other valid checkpoints keep their indices). Ensure the
tests reference resolveCheckpoint, resolveCheckpoints and the existing nodeIds
variable.
In
`@packages/transition-common/src/services/path/__tests__/PathServiceGrouping.test.ts`:
- Around line 273-298: The test file contains a lingering TODO comment asking to
"Check if we keep this test" inside the test 'service with no trips for the path
produces empty fingerprint'; decide and act: either remove the TODO line and
leave the test as-is (or replace the TODO with a short rationale comment
explaining why this edge-case test using makePath, makeServicesCollection and
groupServicesByTravelTimes is required), or remove the entire test block if it's
no longer needed; update the test name or comment accordingly so future
reviewers understand the intent of the test referencing
groupServicesByTravelTimes, makePath, and makeServicesCollection.
In `@packages/transition-common/src/services/path/Path.ts`:
- Around line 101-102: The checkpoint endpoints stored in
segmentTimesCheckpoints are using only node ids which cannot distinguish
repeated occurrences; update the type in Path.ts (segmentTimesCheckpoints) to
include occurrence-level identifiers (e.g., add fromNodeIndex/toNodeIndex or
occurrenceId alongside fromNodeId/toNodeId) and modify any resolver/cleanup
logic that uses indexOf (the code that resolves endpoints) to match on both node
id and the occurrence identifier (or position) instead of using indexOf on
nodeId alone so checkpoints target the correct occurrence when nodes repeat.
- Around line 951-1008: The _averagePeriodSegmentData implementation currently
truncates to Math.min lengths of PeriodSegmentData.segments and
.dwellTimeSeconds which drops trailing entries when any service has
stale/malformed data; update _averagePeriodSegmentData to iterate to the full
canonical length (use the path/global baseline like
this.attributes.data.segments.length and .dwellTimeSeconds length) and for each
index skip or substitute service entries that are missing/short (i.e., if
d.segments[i] or d.dwellTimeSeconds[i] is undefined, do not include it in the
weighted sum and adjust the divisor/weights accordingly, or use the global
segment/dwell fallback from this.attributes.data for that index), ensure
tripCount weighting still works when entries are skipped, and keep returning
arrays with the full canonical length so Schedule.generateTrip and downstream
code never receive truncated segments/dwell arrays.
In `@packages/transition-common/src/services/path/PathGeographyGenerator.ts`:
- Around line 851-902: The remapping rebuilds dwell times via
getDwellTimeSecondsForNode and thus discards any edited
initialPeriod.dwellTimeDurationsSeconds; update the dwell-time logic so
preserved segments and the final node use the same preserved/resolve helpers as
the base flow: replace the getDwellTimeSecondsForNode calls inside the
preserved-segment branch (where isNew === false and where dwellTime is assigned)
and the final remappedDwellTimes.push(...) with calls to
getPreservedNodeDwellTime or resolveSegmentDwellTime (passing path, nodeIds[i]
or last node, initialIndex, periodInitial.dwellTimeDurationsSeconds and
changesInfo as appropriate) so that edits in
initialPeriod.dwellTimeDurationsSeconds are honored when building
remappedDwellTimes (leave the new-segment branch using physicsDurations *
periodRatio).
In `@packages/transition-common/src/services/path/PathGeographyUtils.ts`:
- Around line 400-404: The call to computeSegmentTimesFromLegs inside
calculateSegmentTimesForCheckpoint can throw (via calculateSegmentDuration) and
should not reject the Promise; wrap the computeSegmentTimesFromLegs(path, legs,
checkpointPoints, segmentCount) call in a try/catch and on any error return null
instead of letting the exception propagate, preserving the declared
Promise<number[] | null> contract; keep the existing early return when legs is
falsy and ensure calculateSegmentTimesForCheckpoint still returns the computed
array on success.
- Around line 383-398: The code unconditionally converts
path.getData('defaultRunningSpeedKmH') to m/s which yields NaN for blank values;
change the logic around defaultRunningSpeedKmH/defaultRunningSpeedMps so you
only call kphToMps when the data is a valid number (e.g., typeof value ===
'number' and !isNaN(value')), otherwise leave defaultRunningSpeedMps undefined
(or the same sentinel used by normal path routing) and pass that through
prepareNodesAndWaypoints, getRoutingSegments, and routeSegments; update the
variables defaultRunningSpeedKmH and defaultRunningSpeedMps and the call sites
prepareNodesAndWaypoints(...) and routeSegments(..., defaultRunningSpeedMps) so
checkpoint routing treats blank speeds the same as normal getPathGeography
routing.
In `@packages/transition-common/src/services/path/PathSegmentTimeUtils.ts`:
- Around line 38-39: The current getCheckpointKey implementation concatenates
fromNodeId and toNodeId with a hyphen which can collide when IDs contain
hyphens; change getCheckpointKey(checkpoint: Checkpoint) to return an
unambiguous representation such as JSON.stringify([checkpoint.fromNodeId,
checkpoint.toNodeId]) (or another reversible encoding like length-prefixing) so
keys are unique and safe for UUIDs.
In `@packages/transition-common/src/services/path/PathServiceGrouping.ts`:
- Around line 371-384: The group builder currently hard-codes hasHolidayService:
false, so groups that include holiday services (including those appended by
matchHolidaysToGroups) are not flagged; update the builtServiceGroups creation
to compute a boolean (e.g. hasHoliday) by checking each id in ids via
servicesCollection?.getById(serviceId) and isHolidayService(service) (use
ids.some(...)), keep skipping holiday services when collecting activeDays as
before, and set hasHolidayService to that computed boolean; reference the
existing symbols builtServiceGroups, isHolidayService,
servicesCollection.getById, getAverageSegmentTimesByPeriod and
matchHolidaysToGroups to locate where to change.
- Around line 78-129: The buildGroupLabel function is returning hardcoded
user-facing strings; change it to use i18next by either accepting a t callback
parameter (e.g., add t: (key: string, opts?: any) => string to buildGroupLabel
signature) or return translation keys instead of literal labels; replace every
occurrence of language-dependent labels (everyDayLabel, weekdayLabel,
weekendLabel, holidayLabel and dayAbbreviations lookups) with calls to
t('path.everyDay'), t('path.weekday'), t('path.weekend'), t('path.holiday') and
t(...) for individual day abbreviations (or return those keys like
'path.monday_abbrev') so the UI layer controls localization, and update callers
of buildGroupLabel to pass the t function when rendering.
In `@packages/transition-common/src/services/periods/__tests__/Period.test.ts`:
- Around line 17-22: Replace the multi-assert test blocks that call
findPeriodShortname with parametric tests using test.each to make each
input/expected pair a separate case; locate the tests in Period.test.ts that
reference findPeriodShortname and the periods fixture (e.g., the block with
times 7*3600, 12*3600, 16*3600, 20*3600 and the other mentioned blocks) and
convert them to table-driven tests where each row is [timeInSeconds,
expectedShortname] so failures report the specific case instead of bundling
multiple expect calls.
- Around line 16-55: Add a regression test in the existing
describe('findPeriodShortname') suite that asserts findPeriodShortname([], <some
time like 12 * 3600>) returns null; specifically, create a new test case (e.g.
"returns null for empty periods array") that calls findPeriodShortname with an
empty array and a sample timestamp and expects null so the empty-array edge case
for the function findPeriodShortname is covered.
In `@packages/transition-common/src/services/periods/Period.ts`:
- Around line 34-56: The function findPeriodShortname currently dereferences
periods[0] and periods[periods.length-1] without checking for an empty array,
causing a runtime crash when periods = []; add an early guard at the top of
findPeriodShortname that returns null if !periods || periods.length === 0, so
subsequent references to firstPeriod, lastPeriod and calculations with
MAX_OVERFLOW_SECONDS are safe; keep the rest of the logic unchanged but ensure
the guard prevents any access to periods[0] or periods[periods.length - 1].
In
`@packages/transition-common/src/services/schedules/__tests__/Schedule.test.ts`:
- Around line 873-874: Test currently only verifies fallback to
attributes.data.segments; add assertions to ensure the service-aware lookup is
used by asserting the mocked getSegmentsForPeriodAndService (from the setup) is
called with the expected periodShortname and serviceId and that its return value
is used (not attributes.data.segments). Specifically, in the tests around
getSegmentsForPeriodAndService/ processDeparture add
expect(getSegmentsForPeriodAndService).toHaveBeenCalledWith(periodShortname,
serviceId) and assert the resulting segments come from that mock (and optionally
spy on attributes.data.segments to ensure it was not accessed).
In `@packages/transition-frontend/src/components/forms/gtfs/GtfsImportForm.tsx`:
- Around line 77-82: The updateSelectedValue helper is typed with keyof
GtfsImportData which allows array/object keys (e.g., agencies, lines, services,
periodsGroup) but the implementation only accepts scalars and silences
TypeScript via an any cast; restrict the function to a new union type of
editable scalar fields (e.g., create EditableGtfsScalarKeys = 'feedName' |
'description' | ... matching your scalar properties on GtfsImportData), change
the signature to (path: EditableGtfsScalarKeys, value:
string|boolean|number|undefined), remove the any cast and assign the value
directly to availableImportData[path], and keep calling
setAvailableImportData(importData) so unsafe writes to collection/object fields
are prevented at compile time (references: updateSelectedValue, GtfsImportData,
availableImportData, setAvailableImportData).
In `@packages/transition-frontend/src/components/forms/path/TransitPathEdit.tsx`:
- Around line 636-645: The segment-times editor Button in TransitPathEdit.tsx is
not respecting frozen paths; update the render that shows the Button for
pathData.segments (the one that calls openSegmentTimesByPeriodModal) to guard
with the same frozen-path logic used elsewhere: either add disabled={isFrozen}
to the Button or wrap it in a conditional render (e.g., isFrozen !== true &&
...) so the control is not active when isFrozen is true; apply the same change
to the other instance of this Button render further down (the block
corresponding to the second occurrence around the 859–865 area).
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/ScrollableDropdown.tsx`:
- Around line 41-70: The ScrollableDropdown component is missing ARIA roles and
keyboard handling: add role="button" and aria-expanded={isOpen} to the trigger
button (or keep button and add aria-haspopup="listbox"), add role="listbox" to
the menu container and role="option" plus aria-selected on each item, and ensure
items are focusable (tabIndex={-1} when closed/0 when focused) using refs;
implement an onKeyDown handler on the trigger/menu to support Enter/Space to
open/select, ArrowUp/ArrowDown to move the active index, and Escape to close,
updating isOpen, calling onSelect(value) and managing focus via dropdownRef or
per-item refs so keyboard users can navigate choices.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentCarousel.tsx`:
- Around line 34-41: The aria-labels in the SegmentCarousel component are
hardcoded ("Previous" and "Next"); import and use i18next's t() (e.g., via
useTranslation) and replace those literals on the button elements (the ones
using onPrevious and onNext with className "carousel-arrow") with translated
keys (e.g., t('carousel.previous') and t('carousel.next')) and ensure the
translation keys are added to the relevant locale files.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx`:
- Around line 91-104: The memoized callbacks stored in periodCallbacks currently
only update when activeSegmentIndex changes, but they close over
handleCellChange and periods; update the logic so callbacks are recreated
whenever handleCellChange or periods change as well: include handleCellChange
and periods in the dependency array of the React.useEffect that writes
periodCallbacks.current, and ensure the initial population (the periods.forEach
before the effect) uses the same creation logic or is removed in favor of the
effect so callbacks always capture the latest handleCellChange and periods;
reference periodCallbacks, handleCellChange, activeSegmentIndex, and periods
when making this change.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentTimesToolbar.tsx`:
- Around line 67-70: The onChange handler for the From input currently
increments To using segmentCount which can select a To not present in the
filtered options; change the clamp to use newCheckpointMaxTo instead of
segmentCount so the auto-updated newCheckpointTo is bounded by the filtered
maximum. Specifically, in the onChange callback where you call
onNewCheckpointToChange(Math.min(v + 1, segmentCount)), replace segmentCount
with newCheckpointMaxTo (keep the same Math.min logic) and ensure you still call
onNewCheckpointFromChange(v) first; this ensures newCheckpointTo stays within
the available filtered To options.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TimeInput.tsx`:
- Around line 60-67: The TimeInput JSX is rendering hardcoded unit labels ("min"
and "sec"); import and use i18next's t function (e.g., via useTranslation from
react-i18next) inside the TimeInput component and replace the literal "min" and
"sec" strings with t('path.unit.min') and t('path.unit.sec') (or your project's
agreed translation keys) so both the readonly span (currently rendering mins and
secs) and the other occurrence mentioned are internationalized; ensure
useTranslation is called at the top of the component and t is used in the <span
className="time-input-unit"> elements.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/NodeDots.tsx`:
- Around line 55-76: The node-dot div in NodeDots.tsx is not keyboard
accessible; add keyboard support by making the interactive div focusable and
operable: add tabIndex={0}, role="button", and an onKeyDown handler that maps
Enter and Space to the same logic used in the onClick (use onSegmentClick with
idx or idx-1 as in the existing onClick), and ensure the element still uses
getNodeColor, nodeToCol, isFirstOrLast etc. for styling; also ensure any ARIA or
title remains intact and that default scrolling for Space is prevented in the
onKeyDown handler.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/SegmentLines.tsx`:
- Around line 42-48: The segment hitbox currently uses a non-focusable div so
keyboard users cannot select segments; update the element in SegmentLines.tsx to
be keyboard-accessible by making it a semantic control (prefer replacing the div
with a <button> or adding role="button" plus tabIndex={0}), add an onKeyDown
handler that calls onSegmentClick(idx) when Enter or Space is pressed, and
provide an accessible name (aria-label or aria-labelledby) that identifies the
segment (e.g., include the segment index or line name). Ensure existing
onMouseEnter/onMouseLeave still call onHoverChange and adjust any styling so
focus outlines remain visible.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/TransitLineOverview.tsx`:
- Around line 146-156: The aria-labels for the overview nav buttons in the
TransitLineOverview component are hard-coded; update them to use the i18next t()
translation function so labels follow the active locale. Locate the left and
right nav buttons (the elements with onClick handlers handleScrollLeft and
handleScrollRight and className "overview-nav-btn") and replace the literal
aria-label strings ("Scroll overview left" and the corresponding right label)
with calls to t('...') using appropriate translation keys (e.g.,
'transit.overview.scrollLeft' and 'transit.overview.scrollRight'), ensuring you
import/use the t function from react-i18next in TransitLineOverview.
- Around line 97-118: The useEffect that reads
checkpoints[activeCheckpointIndex] must include checkpoints in its dependency
array so it re-runs when the checkpoints array changes; update the dependency
list for the effect that references scrollRef, needsCarousel,
activeSegmentIndex, activeCheckpointIndex, editMode, viewportNodes to also
include checkpoints and ensure related values like slotWidth and scrollToNode
remain stable or are included if they are not stable; additionally replace the
hard-coded aria-labels on the navigation buttons referenced in
TransitLineOverview (the two nav buttons currently using literal strings) with
localized strings using the i18next t(...) call so aria labels are localized.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitPathSegmentTimesByPeriodModal.tsx`:
- Around line 112-114: The close button's aria-label in
TransitPathSegmentTimesByPeriodModal is hard-coded ("Close"); update the
component to use i18next by importing and calling useTranslation (e.g., const {
t } = useTranslation()) in TransitPathSegmentTimesByPeriodModal and change the
button's aria-label to use t with an appropriate key (for example
t('common.close') or t('modal.close')) while keeping the existing props.onClose
and className="close-btn" intact so the label is localized for screen readers.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts`:
- Around line 246-276: These helpers (e.g., getCheckpointTargetKey,
getCheckpointTarget, getCheckpointCurrentTotal, getCheckpointAverageTotal,
getCheckpointTotalDwellTime, getCheckpointTotalDwellTime) close over
selectedServiceId which causes incorrect redistribution when handleSave iterates
groups for different services; refactor these functions to accept an explicit
serviceId (and periodShortname where needed) as parameters instead of reading
selectedServiceId from closure, update all call sites (including handleSave's
group iteration and any uses at lines referenced) to pass the appropriate
serviceId for each group, and ensure getCheckpointTargetKey builds the key from
the passed serviceId so per-group targets are looked up correctly.
- Around line 171-179: The fallback period labels produced inside
useSegmentTimesByPeriod (the .map using periodNamesByShortname, shortname and
language) and the save error message shown in the save handler should be
localized via i18next; replace hardcoded English fallbacks like `${shortname}
(${period.start_at_hour}h-${period.end_at_hour}h)` with calls to t() (e.g.,
t('period.fallback', { shortname, start: period.start_at_hour, end:
period.end_at_hour }, { lng: language })) and ensure the save error text used in
the save handler (e.g., handleSave/onSave) also uses t() with an appropriate key
and interpolation, and import/use the t function from useTranslation or the
passed translation/context used in this module.
- Around line 379-387: The delete only removes checkpointTargets entry for the
base key returned by getCheckpointKey(resolved) but checkpointTargets uses keys
in the form `${checkpointKey}_${serviceId}`, so stale per-service entries
remain; update the setCheckpointTargets updater (the one that currently does
const next = { ...prev }; delete next[key]; return next;) to instead copy prev
and remove all keys whose name starts with `${key}_` (or equals key) before
returning so re-adding a checkpoint won’t resurrect old per-service targets;
reference getCheckpointKey, setCheckpoints, setCheckpointTargets and
checkpointTargets when locating the code to change.
- Line 49: Replace all `any` escapes in useSegmentTimesByPeriod by introducing
explicit TypeScript interfaces for the persisted segment and checkpoint shapes
(e.g., Segment, Checkpoint, PersistedSegmentData) and use those types instead of
`any`; specifically replace the `const line: any = path.getLine()` with the
correct return type from path.getLine() (or cast to the new Line type), and
update the other `any` usages referenced (around the variables handling segments
and checkpoints at the other noted locations) to use the new interfaces or
generics. Ensure functions and variables in this hook (useSegmentTimesByPeriod,
path.getLine, any variables named like segment, checkpoint,
savedSegments/savedCheckpoints) are typed to reflect the persisted data shape so
the compiler enforces shape correctness.
In `@packages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scss`:
- Around line 18-20: The focus rule in _segmentTimesByPeriodModal.scss currently
removes the focus indicator (the &:focus selector uses outline: none), which
breaks keyboard accessibility; instead, update the &:focus (and preferably
&:focus-visible) styles on the modal/dialog root to remove outline: none and
apply an accessible custom focus ring (e.g., a visible border or box-shadow ring
with sufficient contrast and spacing) so keyboard users retain a clear focus
indicator while preserving your visual design.
- Around line 131-142: The CSS rule uses the invalid property value
"white-space: wrap" in the .carousel-label-text block (and the other identical
block later); replace it with a valid wrapping combination such as "white-space:
normal" (or "pre-wrap" if preserving newlines is desired) and add
"overflow-wrap: anywhere" to ensure long words break and text wraps correctly,
updating both occurrences of the .carousel-label-text rule.
---
Outside diff comments:
In `@packages/transition-common/src/services/schedules/Schedule.ts`:
- Around line 235-258: processDeparture() currently sets
unit.expectedArrivalTime as a fractional value (currentTime + totalTimeSeconds)
while trips are stored rounded, causing off-by-one availability checks; change
the assignment in processDeparture() so unit.expectedArrivalTime is rounded the
same way as trip records (e.g., use Math.round on the computed arrival seconds
or assign the already-rounded trip.arrival_time_seconds) to ensure availability
checks against integer currentTime behave consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78c3e38d-33c9-48fb-860f-168fe6793f51
📒 Files selected for processing (46)
locales/en/main.jsonlocales/en/transit.jsonlocales/fr/main.jsonlocales/fr/transit.jsonpackages/transition-backend/src/api/__tests__/gtfs.routes.test.tspackages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.tspackages/transition-backend/src/services/gtfsImport/GtfsImportTypes.tspackages/transition-backend/src/services/gtfsImport/GtfsImporter.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsImport/ScheduleImporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/path/PathGtfsGeographyGenerator.tspackages/transition-backend/src/services/path/__tests__/PathGtfsGeographyGenerator.test.tspackages/transition-common/src/services/gtfs/GtfsImportTypes.tspackages/transition-common/src/services/path/Path.tspackages/transition-common/src/services/path/PathGeographyGenerator.tspackages/transition-common/src/services/path/PathGeographyUtils.tspackages/transition-common/src/services/path/PathSegmentTimeUtils.tspackages/transition-common/src/services/path/PathServiceGrouping.tspackages/transition-common/src/services/path/PathTypes.tspackages/transition-common/src/services/path/__tests__/Path.test.tspackages/transition-common/src/services/path/__tests__/PathGeographyGenerator.test.tspackages/transition-common/src/services/path/__tests__/PathGeographyUtils.test.tspackages/transition-common/src/services/path/__tests__/PathSegmentTimeUtils.test.tspackages/transition-common/src/services/path/__tests__/PathServiceGrouping.test.tspackages/transition-common/src/services/periods/Period.tspackages/transition-common/src/services/periods/__tests__/Period.test.tspackages/transition-common/src/services/schedules/Schedule.tspackages/transition-common/src/services/schedules/__tests__/Schedule.test.tspackages/transition-frontend/src/components/forms/gtfs/GtfsImportForm.tsxpackages/transition-frontend/src/components/forms/path/TransitPathEdit.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/CheckpointPeriodTimesTable.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/ScrollableDropdown.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentCarousel.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentTimesToolbar.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TimeInput.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/CheckpointLines.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/NodeDots.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/SegmentLines.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/TransitLineOverview.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitPathSegmentTimesByPeriodModal.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.tspackages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scsspackages/transition-frontend/src/styles/transition.scss
Comment on lines
+55
to
+63
| const checkpointNodeIds = new Set<string>(); | ||
| const checkpoints = newPath.attributes.data.segmentTimesCheckpoints; | ||
| if (checkpoints && checkpoints.length > 0) { | ||
| for (const cp of checkpoints) { | ||
| checkpointNodeIds.add(cp.fromNodeId); | ||
| checkpointNodeIds.add(cp.toNodeId); | ||
| } | ||
| } | ||
| pathsById[trip.path_id] = { path: newPath, distances: newPath.getCoordinatesDistanceTraveledMeters(), checkpointNodeIds }; |
There was a problem hiding this comment.
Please key exported timepoints by path position, not node ID.
This breaks on loop or repeated-stop paths. Once a node ID is added to checkpointNodeIds, every occurrence of that node in pathNodeIds is exported as timepoint = 1, even if only one sequence position belongs to a checkpoint. GTFS timepoints are sequence-based here, so this needs checkpoint indices/occurrences rather than a plain Set<string>.
Also applies to: 70-70, 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts`
around lines 55 - 63, The code currently builds checkpointNodeIds as a
Set<string> of node IDs and keys timepoints by node ID, which mislabels repeated
nodes; change the logic in ScheduleExporter.ts to record checkpoint positions
(sequence indices) instead of node IDs: when iterating checkpoints (the
checkpoints variable) map each checkpoint's from/to occurrences to the
corresponding index(es) along newPath (use the path node list or coordinates
from newPath and newPath.getCoordinatesDistanceTraveledMeters() to determine
sequence positions) and store those indices (e.g., Set<number> or boolean array)
in the pathsById entry (replace checkpointNodeIds with checkpointPositions or
similar); then use those position indices when marking timepoints rather than
checking membership by node ID. Also update the same pattern at the other
occurrences flagged (around lines referenced for 70 and 133) so all exports key
by path position/sequence, not node ID.
Comment on lines
+134
to
+136
| const timepointNodeIds = representativeStopTimes | ||
| .filter((st) => st.timepoint === 1) | ||
| .map((st) => importData.nodeIdsByStopGtfsId[st.stop_id]); |
There was a problem hiding this comment.
Potential undefined node IDs in timepoint mapping.
If importData.nodeIdsByStopGtfsId[st.stop_id] returns undefined for any stop, the timepointNodeIds array could contain undefined values, leading to invalid checkpoint data.
Proposed fix to filter out undefined node IDs
const timepointNodeIds = representativeStopTimes
.filter((st) => st.timepoint === 1)
- .map((st) => importData.nodeIdsByStopGtfsId[st.stop_id]);
+ .map((st) => importData.nodeIdsByStopGtfsId[st.stop_id])
+ .filter((nodeId): nodeId is string => nodeId !== undefined);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-backend/src/services/gtfsImport/PathImporter.ts` around
lines 134 - 136, The timepointNodeIds array may include undefined entries
because importData.nodeIdsByStopGtfsId[st.stop_id] can be missing; update the
generation of timepointNodeIds (in the block using representativeStopTimes and
importData.nodeIdsByStopGtfsId) to skip undefined values so only valid node IDs
are collected (e.g., map then filter out undefined or flatMap conditional),
ensuring downstream checkpoint data only contains concrete node IDs.
Comment on lines
+379
to
+387
| const key = getCheckpointKey(resolved); | ||
| // Find and remove the matching checkpoint from the stored array by node IDs | ||
| setCheckpoints((prev) => | ||
| prev.filter((cp) => cp.fromNodeId !== resolved.fromNodeId || cp.toNodeId !== resolved.toNodeId) | ||
| ); | ||
| setCheckpointTargets((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[key]; | ||
| return next; |
There was a problem hiding this comment.
Removing a checkpoint leaves stale per-service targets behind.
checkpointTargets is keyed as ${checkpointKey}_${serviceId}, but Line 386 deletes only checkpointKey. Re-adding the same checkpoint in the same session can resurrect old values.
Suggested fix
- const key = getCheckpointKey(resolved);
+ const keyPrefix = `${getCheckpointKey(resolved)}_`;
// Find and remove the matching checkpoint from the stored array by node IDs
setCheckpoints((prev) =>
prev.filter((cp) => cp.fromNodeId !== resolved.fromNodeId || cp.toNodeId !== resolved.toNodeId)
);
setCheckpointTargets((prev) => {
const next = { ...prev };
- delete next[key];
+ for (const key of Object.keys(next)) {
+ if (key.startsWith(keyPrefix)) {
+ delete next[key];
+ }
+ }
return next;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const key = getCheckpointKey(resolved); | |
| // Find and remove the matching checkpoint from the stored array by node IDs | |
| setCheckpoints((prev) => | |
| prev.filter((cp) => cp.fromNodeId !== resolved.fromNodeId || cp.toNodeId !== resolved.toNodeId) | |
| ); | |
| setCheckpointTargets((prev) => { | |
| const next = { ...prev }; | |
| delete next[key]; | |
| return next; | |
| const keyPrefix = `${getCheckpointKey(resolved)}_`; | |
| // Find and remove the matching checkpoint from the stored array by node IDs | |
| setCheckpoints((prev) => | |
| prev.filter((cp) => cp.fromNodeId !== resolved.fromNodeId || cp.toNodeId !== resolved.toNodeId) | |
| ); | |
| setCheckpointTargets((prev) => { | |
| const next = { ...prev }; | |
| for (const key of Object.keys(next)) { | |
| if (key.startsWith(keyPrefix)) { | |
| delete next[key]; | |
| } | |
| } | |
| return next; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts`
around lines 379 - 387, The delete only removes checkpointTargets entry for the
base key returned by getCheckpointKey(resolved) but checkpointTargets uses keys
in the form `${checkpointKey}_${serviceId}`, so stale per-service entries
remain; update the setCheckpointTargets updater (the one that currently does
const next = { ...prev }; delete next[key]; return next;) to instead copy prev
and remove all keys whose name starts with `${key}_` (or equals key) before
returning so re-adding a checkpoint won’t resurrect old per-service targets;
reference getCheckpointKey, setCheckpoints, setCheckpointTargets and
checkpointTargets when locating the code to change.
Comment on lines
+18
to
+20
| &:focus { | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
Keep a visible focus style on the dialog.
Removing the outline here leaves keyboard users without a focus indicator on the modal itself. Please replace it with a custom ring instead of outline: none.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scss`
around lines 18 - 20, The focus rule in _segmentTimesByPeriodModal.scss
currently removes the focus indicator (the &:focus selector uses outline: none),
which breaks keyboard accessibility; instead, update the &:focus (and preferably
&:focus-visible) styles on the modal/dialog root to remove outline: none and
apply an accessible custom focus ring (e.g., a visible border or box-shadow ring
with sufficient contrast and spacing) so keyboard users retain a clear focus
indicator while preserving your visual design.
Comment on lines
+131
to
+142
| .carousel-label-text { | ||
| position: absolute; | ||
| top: 10px; | ||
| left: 50%; | ||
| transform: translateX(-50%); | ||
| font-size: 0.8em; | ||
| font-weight: bold; | ||
| text-align: center; | ||
| white-space: wrap; | ||
| line-height: 1.2; | ||
| width: 300px; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is white-space: wrap a valid CSS value? If not, what CSS should be used to wrap long text labels?
💡 Result:
No. white-space: wrap is not a valid value for the CSS white-space property (valid keywords include things like normal, nowrap, pre, pre-wrap, break-spaces, etc.). [1]
To wrap long text labels, use normal wrapping plus a “break long words/URLs” rule:
.label{
white-space: normal; /* allow wrapping (this is the default) */
overflow-wrap: anywhere; /* or: overflow-wrap: break-word; */
}overflow-wrap is specifically meant to prevent overflow by allowing breaks inside otherwise unbreakable strings. [2][3]
Sources:
[1] MDN — white-space property values https://developer.mozilla.org/en-US/docs/Web/CSS/white-space
[2] MDN — overflow-wrap https://developer.mozilla.org/docs/Web/CSS/overflow-wrap
[3] MDN Guide — Wrapping and breaking text https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Text/Wrapping_breaking_text
white-space: wrap is invalid CSS and won't wrap text. Use white-space: normal (or pre-wrap) with overflow-wrap: anywhere to enable wrapping.
Applies to lines 131-142 and 448-459.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scss`
around lines 131 - 142, The CSS rule uses the invalid property value
"white-space: wrap" in the .carousel-label-text block (and the other identical
block later); replace it with a valid wrapping combination such as "white-space:
normal" (or "pre-wrap" if preserving newlines is desired) and add
"overflow-wrap: anywhere" to ensure long words break and text wraps correctly,
updating both occurrences of the .carousel-label-text rule.
iTzxMigz
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
from
7dabf52 to
48bdfe0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/transition-backend/src/services/gtfsImport/PathImporter.ts (1)
134-143:⚠️ Potential issue | 🟡 MinorFilter unresolved stop→node mappings before building checkpoints.
If a stop ID is absent from
importData.nodeIdsByStopGtfsId, checkpoint pairs may include invalid node IDs. Filter undefined values first.Proposed fix
const timepointNodeIds = representativeStopTimes .filter((st) => st.timepoint === 1) - .map((st) => importData.nodeIdsByStopGtfsId[st.stop_id]); + .map((st) => importData.nodeIdsByStopGtfsId[st.stop_id]) + .filter((nodeId): nodeId is string => typeof nodeId === 'string');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/gtfsImport/PathImporter.ts` around lines 134 - 143, In PathImporter, when building timepointNodeIds from representativeStopTimes and importData.nodeIdsByStopGtfsId, filter out missing/undefined mappings before creating checkpoint pairs: change the pipeline that defines timepointNodeIds to map stop_id to importData.nodeIdsByStopGtfsId[st.stop_id] and then filter out undefined (e.g., .filter((id): id is string => id !== undefined)), so checkpoints (created in the checkpoints loop) only contain valid fromNodeId/toNodeId values and the existing length checks against nodeIds remain correct.packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts (1)
55-61:⚠️ Potential issue | 🟠 MajorUse checkpoint positions (stop sequence), not node IDs, for
timepoint.This mislabels loop/repeated-stop paths: once a node ID is in the set, every occurrence gets
timepoint = 1. Use matched checkpoint indices and compare withkinstead.Proposed fix
-const pathsById: { [key: string]: { path: Path; distances: number[]; checkpointNodeIds: Set<string> } } = {}; +const pathsById: { [key: string]: { path: Path; distances: number[]; checkpointPositions: Set<number> } } = {}; ... -const checkpointNodeIds = new Set<string>(); +const checkpointPositions = new Set<number>(); const checkpoints = newPath.attributes.data.segmentTimesCheckpoints; +const pathNodeIds = newPath.attributes.nodes; if (checkpoints && checkpoints.length > 0) { + let searchFrom = 0; for (const cp of checkpoints) { - checkpointNodeIds.add(cp.fromNodeId); - checkpointNodeIds.add(cp.toNodeId); + const fromIdx = pathNodeIds.indexOf(cp.fromNodeId, searchFrom); + if (fromIdx >= 0) checkpointPositions.add(fromIdx); + const toIdx = pathNodeIds.indexOf(cp.toNodeId, fromIdx >= 0 ? fromIdx : searchFrom); + if (toIdx >= 0) { + checkpointPositions.add(toIdx); + searchFrom = toIdx; + } } } pathsById[trip.path_id] = { path: newPath, distances: newPath.getCoordinatesDistanceTraveledMeters(), - checkpointNodeIds + checkpointPositions }; ... -const { path, distances: pathDistancesTraveledMeters, checkpointNodeIds } = pathsById[trip.path_id]; +const { path, distances: pathDistancesTraveledMeters, checkpointPositions } = pathsById[trip.path_id]; ... - checkpointNodeIds.size === 0 || + checkpointPositions.size === 0 || k === 0 || k === countK - 1 || - checkpointNodeIds.has(pathNodeIds[k]) + checkpointPositions.has(k) ? (1 as const) : (0 as const)Also applies to: 137-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts` around lines 55 - 61, The code currently builds checkpointNodeIds from newPath.attributes.data.segmentTimesCheckpoints and uses node IDs to mark GTFS timepoint (causing repeated stops with the same node ID to all become timepoints); instead, map checkpoints to stop sequence positions and compare against the loop index k: iterate newPath.attributes.data.segmentTimesCheckpoints and for each checkpoint find the corresponding stop index(es) in the stops array (matching stop.nodeId to cp.fromNodeId/cp.toNodeId) to build a set of checkpointIndices, then when building each stop record (the loop using variable k) set timepoint = 1 only if k is in checkpointIndices. Apply the same replacement wherever checkpointNodeIds is used (including the other block mentioned around the code handling lines ~137-143).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts`:
- Around line 640-699: Add a new unit test (next to the existing 'Test
checkpoint nodes are exported as timepoints, others as approximate') that
constructs a ScheduleAttributes where the path contains a repeated node ID
(e.g., nodes = [A, B, A, C]) and checkpoint boundaries are defined so only one
of the repeated occurrences should be a checkpoint; mockReadForLines to return
that schedule, call exportSchedule (same usage as the existing test), read the
stop times from mockWriteStopTimeStream.write, parse the final CSV field
(timepoint) for each stop, and assert the timepoints array marks only the
intended occurrence of the repeated node as 1 while the other occurrence remains
0 (and last stop still 1). Use the same helpers/variables from the file
(mockReadForLines, exportSchedule, mockWriteStopTimeStream, quoteFct,
serviceToGtfsId) and mirror the parsing/assertion style from the existing test
so the new test covers the sequence-vs-node-ID regression.
---
Duplicate comments:
In `@packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts`:
- Around line 55-61: The code currently builds checkpointNodeIds from
newPath.attributes.data.segmentTimesCheckpoints and uses node IDs to mark GTFS
timepoint (causing repeated stops with the same node ID to all become
timepoints); instead, map checkpoints to stop sequence positions and compare
against the loop index k: iterate
newPath.attributes.data.segmentTimesCheckpoints and for each checkpoint find the
corresponding stop index(es) in the stops array (matching stop.nodeId to
cp.fromNodeId/cp.toNodeId) to build a set of checkpointIndices, then when
building each stop record (the loop using variable k) set timepoint = 1 only if
k is in checkpointIndices. Apply the same replacement wherever checkpointNodeIds
is used (including the other block mentioned around the code handling lines
~137-143).
In `@packages/transition-backend/src/services/gtfsImport/PathImporter.ts`:
- Around line 134-143: In PathImporter, when building timepointNodeIds from
representativeStopTimes and importData.nodeIdsByStopGtfsId, filter out
missing/undefined mappings before creating checkpoint pairs: change the pipeline
that defines timepointNodeIds to map stop_id to
importData.nodeIdsByStopGtfsId[st.stop_id] and then filter out undefined (e.g.,
.filter((id): id is string => id !== undefined)), so checkpoints (created in the
checkpoints loop) only contain valid fromNodeId/toNodeId values and the existing
length checks against nodeIds remain correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 036c5ed1-0b98-4fd2-9c1a-87fadc9e7604
📒 Files selected for processing (4)
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-test (24.x)
- GitHub Check: test-sequential (24.x)
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: pr-build-check
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
🧠 Learnings (16)
📚 Learning: 2025-09-25T17:08:56.828Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:105-107
Timestamp: 2025-09-25T17:08:56.828Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred changing the wrapBatchAccessMap input file validation from `hasInputFile()` to `getFilePath('input')` (actual file existence check), indicating it will be addressed in an upcoming file access refactor.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
📚 Learning: 2025-11-04T19:35:10.045Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1554
File: packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts:23-36
Timestamp: 2025-11-04T19:35:10.045Z
Learning: In the OdTripSimulationMethod (packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts), the team plans to move away from the datasource path and use batch calculations instead. The current non-functional dataSourceId selector will be reworked as part of this architectural change.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsImport/PathImporter.ts
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: When modifying transit data models that use Cap'n Proto caching, update the Cap'n Proto schema, regenerate TypeScript types, and regenerate the cache
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts
📚 Learning: 2026-01-28T18:01:07.845Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1736
File: packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts:54-63
Timestamp: 2026-01-28T18:01:07.845Z
Learning: In packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts, the headway thresholds in getHeadwayThresholdSeconds() are intentionally hard-coded constants rather than configurable parameters. These values should be based on international consensus to allow consistent comparison of transit service quality across different cities. The TODO comments indicate that these values need more research and community discussion before being finalized.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/PathImporter.ts
📚 Learning: 2026-01-28T18:00:30.001Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1736
File: packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts:34-47
Timestamp: 2026-01-28T18:00:30.001Z
Learning: In packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts, the getHeadways method intentionally returns undefined if any leg is missing headway data (rather than handling missing headways per-leg) because applying headway penalties only to some legs could bias the calibration. This all-or-nothing approach ensures consistent treatment of incomplete data.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/PathImporter.ts
📚 Learning: 2025-11-25T21:28:28.491Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1604
File: packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts:74-79
Timestamp: 2025-11-25T21:28:28.491Z
Learning: In packages/transition-backend/src/services/weighting/NodeWeightCalculator.ts, the maxBirdDistanceMeters, maxNetworkDistanceMeters, and maxTravelTimeSeconds parameters should not accept 0 or negative values, as they are semantically invalid for distance and time thresholds. Explicit validation should be preferred over relying on falsy coercion.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/PathImporter.ts
📚 Learning: 2025-11-25T21:24:26.424Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1604
File: packages/transition-backend/src/models/db/geometryUtils.db.queries.ts:13-16
Timestamp: 2025-11-25T21:24:26.424Z
Learning: In packages/transition-backend/src/models/db/geometryUtils.db.queries.ts and the node weighting feature, POI weights should default to 0 (not 1) when undefined or null, because decay functions can return very small values much smaller than 1, and defaulting to 1 would incorrectly dominate the calculation.
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsImport/PathImporter.ts
📚 Learning: 2025-11-06T23:09:02.166Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1550
File: packages/transition-backend/src/services/transitRouting/batchRouteCalculation/BatchRouteFileResultVisitor.ts:30-34
Timestamp: 2025-11-06T23:09:02.166Z
Learning: In PathCollection.loadFromCollection (packages/transition-common/src/services/path/PathCollection.ts), despite the parameter being named `collection`, it actually expects an array of GeoJSON.Feature objects, not a FeatureCollection. The method delegates to CollectionLoadable.loadGeojsonFromCollection which expects features array directly. When calling loadFromCollection with results from pathDbQueries.geojsonCollection(), pass `pathGeojson.features` (the array) rather than `pathGeojson` (the full FeatureCollection object).
Applied to files:
packages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
📚 Learning: 2026-03-02T21:57:21.005Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1830
File: packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts:114-122
Timestamp: 2026-03-02T21:57:21.005Z
Learning: In test files for database query modules, ensure mocked functions match the async/sync nature of the production implementations. If a function returns a Promise in production (async), use an async mock (e.g., jest.fn().mockImplementation(async (...args) => { ... })). If a function is synchronous (e.g., returns a stream or uses direct values), use a synchronous mock. This helps prevent false positives/negatives due to mismatched mock behavior.
Applied to files:
packages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
📚 Learning: 2026-02-24T00:36:44.483Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:44.483Z
Learning: In test files, it is acceptable to use 'any' types for error handling and test scenarios. Reserve strict TypeScript typing for production code; in tests, prioritize pragmatic, readable test cases, and use 'any' only where it clearly improves test clarity or reduces boilerplate.
Applied to files:
packages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
📚 Learning: 2026-02-06T19:06:19.438Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1767
File: packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx:177-178
Timestamp: 2026-02-06T19:06:19.438Z
Learning: In the transition accessibility map API responses (packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx), the populationData property is always present as an object with the structure { population: number | null; dataSourceAreaRatio: number }. Even when population calculation is disabled or the data source is missing, populationData is returned with population set to null rather than populationData being undefined. Therefore, accessing properties1.populationData.population without optional chaining is safe.
Applied to files:
packages/transition-backend/src/services/gtfsImport/PathImporter.ts
📚 Learning: 2026-01-20T22:00:37.310Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1710
File: packages/chaire-lib-backend/src/models/db/zones.db.queries.ts:206-215
Timestamp: 2026-01-20T22:00:37.310Z
Learning: In packages/chaire-lib-backend/src/models/db/zones.db.queries.ts, the ST_MAKEVALID function is used without ST_CollectionExtract wrapping when converting zone boundaries to geography. The current census/zone data being imported does not produce GeometryCollections, so the potential casting issue is deferred for future consideration if different data sources are used.
Applied to files:
packages/transition-backend/src/services/gtfsImport/PathImporter.ts
📚 Learning: 2025-09-25T16:40:55.314Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/executableJob/ExecutableJob.ts:152-158
Timestamp: 2025-09-25T16:40:55.314Z
Learning: In packages/transition-backend/src/services/executableJob/ExecutableJob.ts, greenscientist deferred improving the hasFile() method to check actual file existence on disk rather than just metadata, indicating it should be addressed in a later PR.
Applied to files:
packages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.test.{ts,tsx} : Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Applied to files:
packages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.ts
🔇 Additional comments (1)
packages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.ts (1)
544-629: Nice coverage on checkpoint derivation scenarios.Good additions for mixed/all/no-shape timepoint cases; this materially improves confidence in
segmentTimesCheckpointsmapping.
Collaborator
|
@iTzxMigz Donc ceci est la branche expérimentale qu'on pourrait partager aux partenaires? Pouvez-vous me confirmer que vous avez validé qu'un path généré/importé depuis la branche main et édité par cette branche restera fonctionnel en retournant sur |
thomasspina
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
from
48bdfe0 to
184d0ac
Compare
Contributor
Oui le behaviour actuellement sur le main reste dans cette branche. On a juste ajouter des fonctionalités et des attributs dans "data" dans le path. On a rien enlevé de calculs actuellement sur main. Également, si on importe un gtfs avec cette branche et on fait des modifications, on peut encore utiliser ces données sauvegardées dans la DB sur la main. Donc on a une compatibilité backwards de sorte. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (28)
packages/transition-frontend/src/components/forms/gtfs/GtfsImportForm.tsx (1)
77-82:⚠️ Potential issue | 🟠 MajorPlease remove the
anycast and narrow writable keys.Line 81 bypasses type safety and allows assigning scalar values to non-scalar fields of
GtfsImportData(e.g.,agencies,lines,services,periodsGroup). This is the same unresolved issue flagged earlier.Suggested fix
+type EditableImportField = + | 'stopAggregationWalkingRadiusSeconds' + | 'periodsGroupShortname' + | 'agencies_color' + | 'nodes_color' + | 'mergeSameDaysServices' + | 'generateFrequencyBasedSchedules'; + -const updateSelectedValue = (path: keyof GtfsImportData, value: string | boolean | number | undefined) => { +const updateSelectedValue = <K extends EditableImportField>(path: K, value: GtfsImportData[K]) => { const importData = { ...availableImportData }; if (!importData) return; - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - (importData as any)[path] = value; + importData[path] = value; setAvailableImportData(importData); };#!/bin/bash # Verify current callsites only pass scalar-editable keys and inspect GtfsImportData shape. rg -nP --type=tsx '\bupdateSelectedValue\(\s*' -C2 rg -nP --type=ts 'export interface GtfsImportData\b' -A20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/gtfs/GtfsImportForm.tsx` around lines 77 - 82, The helper updateSelectedValue currently casts importData to any and writes arbitrary keys on GtfsImportData, bypassing type safety; change its signature to accept a narrowed key union (e.g. type EditableGtfsKey = keyof Pick<GtfsImportData, 'someScalar1' | 'someScalar2' | ...>) or a mapped generic like <K extends EditableGtfsKey>(path: K, value: GtfsImportData[K]) so you can remove the any cast, copy availableImportData, set importData[path] = value with correct typing, and call setAvailableImportData(importData); update any callsites to only use the defined editable scalar keys and adjust the EditableGtfsKey list to exclude complex fields like agencies, lines, services, periodsGroup.packages/transition-common/src/services/periods/__tests__/Period.test.ts (2)
16-22:⚠️ Potential issue | 🟡 MinorAdd a test for empty
periodsarray.Please add a regression test for
findPeriodShortname([], someTime)returningnull.describe('findPeriodShortname', () => { + test('returns null when periods array is empty', () => { + expect(findPeriodShortname([], 8 * 3600)).toBeNull(); + }); + test('returns correct period for time within a period', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/periods/__tests__/Period.test.ts` around lines 16 - 22, Add a regression test in Period.test.ts that calls findPeriodShortname with an empty periods array and a sample time and asserts it returns null; locate the describe('findPeriodShortname', ...) block and add a new test (e.g., "returns null for empty periods") that invokes findPeriodShortname([], 7 * 3600) and expects null to ensure the function handles an empty periods input.
17-42: 🧹 Nitpick | 🔵 TrivialConsider using parametric tests.
Multiple
expectcalls in single tests make debugging harder. Usingtest.eachwould pinpoint failures more easily. As per coding guidelines: "Favor parametric tests when more than one expect statement is used."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/periods/__tests__/Period.test.ts` around lines 17 - 42, Replace tests that contain multiple expect calls with parametric tests using test.each: convert the blocks testing findPeriodShortname(periods, ...)—specifically the tests titled "returns correct period for time within a period", "returns correct period at exact boundary", "returns last period for post-last-period overflow within 6 hours", "returns null for post-last-period beyond 6 hour overflow", and "returns first period for pre-first-period within 6 hour overflow"—into test.each tables of [timestamp, expectedShortname] rows so each assertion is a single expect inside the table-driven test; keep using the same periods fixture and the findPeriodShortname symbol and ensure cases that expect null use toBeNull while others use toBe for concise failure localization.packages/transition-common/src/services/periods/Period.ts (1)
34-43:⚠️ Potential issue | 🔴 CriticalGuard against empty
periodsarray.If
periodsis empty, accessingperiods[0]at line 45 will throw. Please add an early return.🐛 Proposed fix
export const findPeriodShortname = (periods: Period[], timeSecondsSinceMidnight: number): string | null => { + if (periods.length === 0) { + return null; + } for (let i = 0, count = periods.length; i < count; i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/periods/Period.ts` around lines 34 - 43, The function findPeriodShortname should guard against an empty periods array: add an early return (e.g., return null) at the top of findPeriodShortname when periods.length === 0 to avoid later accessing periods[0]; keep the existing loop and behavior otherwise so callers still get the matching period.shortname or null.packages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scss (2)
131-142:⚠️ Potential issue | 🟡 Minor
white-space: wrapis invalid CSS.Use
white-space: normalwithoverflow-wrap: anywhereto enable text wrapping. This also applies to line 454..carousel-label-text { position: absolute; top: 10px; left: 50%; transform: translateX(-50%); font-size: 0.8em; font-weight: bold; text-align: center; - white-space: wrap; + white-space: normal; + overflow-wrap: anywhere; line-height: 1.2; width: 300px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scss` around lines 131 - 142, The CSS uses the invalid property value `white-space: wrap` in the `.carousel-label-text` rule; replace `white-space: wrap` with `white-space: normal` and add `overflow-wrap: anywhere` to enable wrapping, and also find and update any other occurrences of `white-space: wrap` in the stylesheet (the duplicate reported further down) to the same `white-space: normal` + `overflow-wrap: anywhere` combination.
18-20:⚠️ Potential issue | 🟠 MajorKeep a visible focus style for keyboard accessibility.
Removing
outline: noneleaves keyboard users without a focus indicator. Please replace it with a custom visible focus ring.&:focus { - outline: none; + outline: 2px solid rgba(79, 195, 247, 0.8); + outline-offset: 2px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scss` around lines 18 - 20, The current &:focus rule removes the focus indicator; replace that with a visible, accessible focus state by removing outline: none and adding a custom focus ring for the same selector (and use &:focus-visible to limit to keyboard focus). Implement a clear ring via an outline or focus ring box-shadow with an accessible color variable (e.g., --focus-color) and sufficient thickness/contrast, ensure border-radius is preserved, and keep the style in _segmentTimesByPeriodModal.scss so keyboard users can see focus.packages/transition-frontend/src/components/forms/path/TransitPathEdit.tsx (1)
636-645:⚠️ Potential issue | 🟠 MajorGuard this action on frozen paths.
Lines 636-644 still let a frozen path open the segment-times editor, unlike the other mutating controls in this form. Please disable or hide this button when
isFrozenis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathEdit.tsx` around lines 636 - 645, The segment-times editor button isn't guarded by the frozen state; update the JSX rendering around pathData.segments to disable or hide the Button when isFrozen is true by checking the isFrozen flag (the same way other mutating controls do). Specifically, modify the block that renders the Button (icon {faClock}, label this.props.t('transit:transitPath:EditSegmentTimesByPeriod'), onClick this.openSegmentTimesByPeriodModal) to either not render it or set a disabled prop when isFrozen is true so openSegmentTimesByPeriodModal cannot be invoked for frozen paths.packages/transition-common/src/services/path/PathServiceGrouping.ts (1)
78-128: 🛠️ Refactor suggestion | 🟠 MajorMove these labels behind i18next.
buildGroupLabelstill returns hardcoded English/French text from a.tshelper, so this path bypasses the repo’s translation flow. Please return translation keys or acceptthere instead.As per coding guidelines "Use i18next and the
t()function for all user-facing strings to support internationalization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/path/PathServiceGrouping.ts` around lines 78 - 128, buildGroupLabel currently emits hardcoded English/French strings; update it to use the i18next translation flow by either accepting a translation function parameter (t: TFunction) or returning translation keys instead of literal text. Replace the hardcoded holidayLabel, weekdayLabel, weekendLabel, everyDayLabel and any direct lookups from dayAbbreviations that produce user-facing text with calls to t('...') (or corresponding keys) inside buildGroupLabel so all branches (every day, weekday, weekend, individual days, holiday suffix) produce translated strings; ensure the function signature is updated (and callers adjusted) if you choose the t parameter approach.packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts (1)
42-42:⚠️ Potential issue | 🟠 MajorResolve checkpoints to path positions before writing
timepoint.
checkpointNodeIdsbreaks loop/repeated-stop paths: once a node ID is in the set, every occurrence of that node exports astimepoint = 1. Please resolve each checkpoint pair to concrete indices inpathNodeIdsorder and key the export onkinstead of node ID membership.Also applies to: 55-67, 74-75, 137-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/gtfsExport/ScheduleExporter.ts` at line 42, The code currently stores checkpoint node IDs in pathsById[*].checkpointNodeIds and uses membership to set timepoint, which marks all repeated occurrences of a node as timepoint=1; instead, when building pathsById (and where checkpoints are processed in ScheduleExporter methods that reference pathNodeIds and timepoint), resolve each checkpoint pair to the concrete index(ices) within the Path.nodeIds order (e.g., compute the k positions for each checkpoint occurrence) and store those indices (e.g., checkpointIndices: Set<number> or array of numbers) keyed by the path and index, then when emitting rows check k (the loop index over pathNodeIds) against checkpointIndices rather than node ID membership; apply this change consistently for the symbols pathsById, pathNodeIds, checkpointNodeIds (replace with checkpointIndices) and the timepoint assignment logic in all affected blocks referenced in the review.packages/transition-backend/src/services/gtfsImport/PathImporter.ts (1)
123-146:⚠️ Potential issue | 🟡 MinorFilter undefined node IDs to prevent invalid checkpoints.
importData.nodeIdsByStopGtfsId[st.stop_id]may returnundefined, leading to invalid checkpoint data with undefinedfromNodeId/toNodeIdvalues.Proposed fix
const timepointNodeIds = representativeStopTimes .filter((st) => st.timepoint === 1) - .map((st) => importData.nodeIdsByStopGtfsId[st.stop_id]); + .map((st) => importData.nodeIdsByStopGtfsId[st.stop_id]) + .filter((nodeId): nodeId is string => nodeId !== undefined);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-backend/src/services/gtfsImport/PathImporter.ts` around lines 123 - 146, The deriveCheckpointsFromTimepoints function can produce checkpoints with undefined node IDs because importData.nodeIdsByStopGtfsId[st.stop_id] may be undefined; update the logic that builds timepointNodeIds to filter out undefined values (e.g., map then filter or reduce) so only valid string node IDs are collected, then construct checkpoints from that filtered array and only assign newPath.attributes.data.segmentTimesCheckpoints when the filtered array has at least two entries and still represents actual segmentation (length < nodeIds.length).packages/transition-common/src/services/schedules/__tests__/Schedule.test.ts (1)
903-924: 🧹 Nitpick | 🔵 TrivialConsider asserting mock invocation.
The test passes
periodShortnameandserviceIdbut doesn't verifygetSegmentsForPeriodAndServicewas called with these values. IfprocessDeparturestops forwarding them, this test would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/schedules/__tests__/Schedule.test.ts` around lines 903 - 924, The test for strategy['processDeparture'] is missing an assertion that getSegmentsForPeriodAndService was invoked with the passed periodShortname and serviceId; add an expectation that the getSegmentsForPeriodAndService spy/mock was called with 'test_period' and 'test_service' (and any other required args as per its signature) after calling processDeparture to ensure those parameters are forwarded correctly (reference symbols: processDeparture and getSegmentsForPeriodAndService).packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TimeInput.tsx (1)
60-67: 🧹 Nitpick | 🔵 TrivialTranslate unit labels.
The "min" and "sec" labels are hardcoded. Per coding guidelines, use
i18nextandt()for internationalization.Also applies to: 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TimeInput.tsx` around lines 60 - 67, Replace hardcoded "min" and "sec" labels in the TimeInput component with i18next translations: import and use the t() translator (e.g., via useTranslation from 'react-i18next') and replace the literal strings in the JSX (the span with className "time-input-unit" in the TimeInput render and the similar occurrences around lines 86-88) with t('time.unit.min') and t('time.unit.sec') or the existing translation keys used in the app; ensure the component calls useTranslation() at the top and uses the returned t function for both minute and second labels.packages/transition-common/src/services/path/__tests__/PathSegmentTimeUtils.test.ts (1)
44-65: 🧹 Nitpick | 🔵 TrivialConsider testing same-node or reversed checkpoints.
Tests cover missing node IDs but not cases where
fromNodeIndex >= toNodeIndex(e.g., same node or reversed order), which may also be invalid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/path/__tests__/PathSegmentTimeUtils.test.ts` around lines 44 - 65, Add tests for invalid checkpoint ordering: create two new test cases calling resolveCheckpoint with (1) a same-node checkpoint where fromNodeId and toNodeId are identical (e.g., { fromNodeId: 'nodeB', toNodeId: 'nodeB' }) and (2) a reversed checkpoint where fromNodeId comes after toNodeId in nodeIds (e.g., { fromNodeId: 'nodeD', toNodeId: 'nodeB' }); for both expect resolveCheckpoint(checkpoint, nodeIds) to be undefined to assert the function rejects fromNodeIndex >= toNodeIndex. Ensure you reference resolveCheckpoint, Checkpoint and the existing nodeIds used in the test file when adding these assertions.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentCarousel.tsx (1)
34-41: 🧹 Nitpick | 🔵 TrivialTranslate aria-labels for accessibility.
The aria-labels "Previous" and "Next" are hardcoded. Per coding guidelines, use
i18nextandt()for all user-facing strings.Also applies to: 52-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentCarousel.tsx` around lines 34 - 41, The two carousel arrow buttons use hardcoded aria-labels ("Previous" and "Next"); update the JSX in the SegmentCarousel component to import and use the i18next translator (t) and replace the static aria-label values with t('...') calls (e.g., aria-label={t('transit.carousel.previous')} on the button using onPrevious and aria-label={t('transit.carousel.next')} on the button using onNext) so both accessibility strings are localized; ensure the translator import/usage matches the project's i18next conventions.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/NodeDots.tsx (1)
55-76: 🧹 Nitpick | 🔵 TrivialAdd keyboard accessibility for clickable dots.
The node dots are clickable but lack keyboard support (
tabIndex,role="button",onKeyDown).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/NodeDots.tsx` around lines 55 - 76, The node dots are mouse-clickable but not keyboard accessible; update the clickable div inside NodeDots (the element using nodeToCol, getNodeColor, isFirstOrLast and onSegmentClick) to add tabIndex={0}, role="button", and an onKeyDown handler that triggers the exact same segment selection logic as the onClick (call onSegmentClick(idx) when idx < nodeCount - 1, otherwise onSegmentClick(idx - 1) if idx > 0) when Enter or Space is pressed (handle Space with preventDefault to avoid page scrolling), and also add aria-label (use the existing label) to improve screen-reader accessibility.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/ScrollableDropdown.tsx (1)
41-70: 🧹 Nitpick | 🔵 TrivialAdd accessibility attributes for keyboard users.
The dropdown lacks ARIA attributes (
aria-haspopup,aria-expanded,role="listbox") and keyboard navigation. This was already flagged in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/ScrollableDropdown.tsx` around lines 41 - 70, The ScrollableDropdown component lacks ARIA attributes and keyboard handling; update the component (ScrollableDropdown) to add aria-haspopup="listbox" and aria-expanded={isOpen} on the trigger button, add role="listbox" and aria-activedescendant (or focus management) on the menu element, and set role="option" and aria-selected on each item; implement keyboard handlers (onKeyDown) on the button to open with Enter/Space and on the menu/items to navigate with ArrowUp/ArrowDown/Home/End, select with Enter/Space, and close with Escape by using setIsOpen, onSelect, and managing a focusedIndex state (and use dropdownRef or per-item refs to move DOM focus). Ensure items are focusable (tabIndex={-1} for roving focus or 0 for the active item) and update className/aria-selected when focused/selected so screen readers and keyboard users can operate the dropdown.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/SegmentLines.tsx (1)
42-62:⚠️ Potential issue | 🟠 MajorAdd keyboard accessibility for segment selection.
The clickable
<div>lackstabIndex,role="button", andonKeyDownhandler. Keyboard-only users cannot select segments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/SegmentLines.tsx` around lines 42 - 62, The segment hitbox div lacks keyboard accessibility; update the element (in SegmentLines.tsx where the JSX uses onSegmentClick, onHoverChange, nodeToCol, getSegColor, hoveredSegmentIndex, isActive and idx) to be focusable and operable via keyboard by adding tabIndex={0}, role="button", and an onKeyDown handler that triggers onSegmentClick(idx) when Enter or Space is pressed (for Space also preventDefault), and optionally call onHoverChange(idx)/onHoverChange(null) on focus/blur to mirror mouse hover behavior; ensure the existing onClick and mouse handlers remain.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitPathSegmentTimesByPeriodModal.tsx (1)
112-114:⚠️ Potential issue | 🟡 MinorLocalize the close button label.
aria-label="Close"stays English for screen readers in every locale.💡 Proposed fix
- <button onClick={props.onClose} className="close-btn" aria-label="Close"> + <button + onClick={props.onClose} + className="close-btn" + aria-label={t('main:Close')} + type="button" + >As per coding guidelines "Use i18next and the
t()function for all user-facing strings to support internationalization".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitPathSegmentTimesByPeriodModal.tsx` around lines 112 - 114, The close button's aria-label is hardcoded ("Close") in TransitPathSegmentTimesByPeriodModal; update the component to use i18next by importing and calling useTranslation() and replace aria-label="Close" with aria-label={t('close')} (or the appropriate key like t('common:close') used across the app) so screen readers get the localized string; ensure the useTranslation hook is called in the TransitPathSegmentTimesByPeriodModal component and that the translation key exists in the relevant namespace.packages/transition-common/src/services/path/PathGeographyUtils.ts (2)
383-385:⚠️ Potential issue | 🟠 MajorHandle blank default speed the same way as normal geography routing.
This converts a blank
defaultRunningSpeedKmHtoNaN, whilegetPathGeography()falls back to 15 km/h. Engine-routed paths can therefore work in normal routing and fail only when checkpoint times are distributed.💡 Proposed fix
- const defaultRunningSpeedKmH = path.getData('defaultRunningSpeedKmH') as number; - const defaultRunningSpeedMps = kphToMps(defaultRunningSpeedKmH); + const defaultRunningSpeedMps = kphToMps(path.getData('defaultRunningSpeedKmH', 15));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/path/PathGeographyUtils.ts` around lines 383 - 385, The code casts path.getData('defaultRunningSpeedKmH') directly and converts it to m/s which turns an empty string into NaN; update the logic around defaultRunningSpeedKmH/defaultRunningSpeedMps to mirror getPathGeography() by treating blank/undefined/non-numeric values as the 15 km/h default: read the raw value from path.getData('defaultRunningSpeedKmH'), coerce/validate it (e.g., parse to number and check isNaN), and if invalid use 15 before calling kphToMps; keep the routingEngine/path.getData references intact so engine-routed paths continue to use a sane default.
400-404:⚠️ Potential issue | 🟠 MajorReturn
nullwhen segment-duration calculation fails.
calculateSegmentDuration(...)can throw on zero or negative routed durations. Right now that escapes as a rejected promise, so callers never reach the advertisednullfallback path.💡 Proposed fix
const legs = this.extractLegsFromResults(segmentResults); if (!legs) return null; const segmentCount = toNodeIndex - fromNodeIndex; - return this.computeSegmentTimesFromLegs(path, legs, checkpointPoints, segmentCount); + try { + return this.computeSegmentTimesFromLegs(path, legs, checkpointPoints, segmentCount); + } catch (_error) { + return null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/path/PathGeographyUtils.ts` around lines 400 - 404, The call chain can throw when calculating segment durations (calculateSegmentDuration), so wrap the computeSegmentTimesFromLegs invocation in a try/catch and return null on any error; specifically, after extracting legs with extractLegsFromResults, call computeSegmentTimesFromLegs inside a try block and catch synchronous exceptions (or await and catch rejections if the function is async) and return null from the catch so callers receive the advertised null fallback instead of a rejected promise.packages/transition-common/src/services/path/Path.ts (2)
101-102:⚠️ Potential issue | 🟠 MajorCheckpoint endpoints need occurrence-level identity.
A path can revisit the same node id later in the sequence. With only
{ fromNodeId, toNodeId }, the cleanup logic resolves endpoints withindexOf(...), so deleting a later occurrence can remove or preserve the wrong checkpoint.Also applies to: 427-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/path/Path.ts` around lines 101 - 102, The checkpoint endpoint type `{ fromNodeId: string; toNodeId: string }` in Path (property `segmentTimesCheckpoints`) is ambiguous for paths that revisit the same node; update the checkpoint shape to include occurrence-level identity (e.g., add `fromNodeIndex` and `toNodeIndex` or `fromOccurrence`/`toOccurrence`) and update any cleanup/lookup logic (references around `segmentTimesCheckpoints` and the cleanup logic currently at the other occurrence mentioned) to match both id and occurrence instead of using `indexOf(...)` on nodeId; ensure creation, serialization, and deletion code (where checkpoints are resolved) use the new fields to find the exact checkpoint occurrence.
953-954:⚠️ Potential issue | 🟠 MajorDo not truncate averaged period data to the shortest service entry.
Math.min(...)silently drops trailing segments or stops as soon as one service has stale period data. Downstream code still works against the full path length, so this can return shortersegments/dwellTimeSecondsarrays and fail later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-common/src/services/path/Path.ts` around lines 953 - 954, The code currently uses Math.min to compute numSegments and numStops, which truncates averaged period arrays to the shortest service entry; change the calculations in Path.ts to use Math.max(...dataArray.map(...)) for numSegments and numStops instead, and update the aggregation logic that iterates over segments/dwellTimeSeconds (e.g., the loops that reference numSegments/numStops) to defensively read missing entries (treat undefined as 0 or an appropriate default) when a service has fewer entries so the averaged output preserves the full path length.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentTimesToolbar.tsx (1)
67-70:⚠️ Potential issue | 🟠 MajorClamp the auto-updated
Tovalue withnewCheckpointMaxTo.When
newCheckpointMaxTois smaller thansegmentCount, this can select aTovalue that is not present in the filtered options and leave the control in an invalid state.💡 Proposed fix
- if (v >= newCheckpointTo) onNewCheckpointToChange(Math.min(v + 1, segmentCount)); + if (v >= newCheckpointTo) onNewCheckpointToChange(Math.min(v + 1, newCheckpointMaxTo));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentTimesToolbar.tsx` around lines 67 - 70, The onChange handler for the "From" control currently auto-updates the "To" via onNewCheckpointToChange(Math.min(v + 1, segmentCount)), which can select a value larger than the filtered options when newCheckpointMaxTo < segmentCount; change the clamp to use newCheckpointMaxTo instead of segmentCount (i.e., when v >= newCheckpointTo call onNewCheckpointToChange(Math.min(v + 1, newCheckpointMaxTo))) so the auto-selected To value is always within the filtered max; update the onChange in the component containing onNewCheckpointFromChange/onNewCheckpointToChange/newCheckpointTo/segmentCount to use newCheckpointMaxTo for clamping.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx (1)
91-104:⚠️ Potential issue | 🟠 MajorCallback memoization is missing dependencies.
The
useEffecton line 99 only depends onactiveSegmentIndex, buthandleCellChangealso capturesselectedServiceIdin its closure. When the service group changes, callbacks won't update until the segment selection changes, causing edits to write to the wrong service.Suggested fix using useMemo
- const periodCallbacks = React.useRef<Record<string, (newSeconds: number) => void>>({}); - periods.forEach((period) => { - if (!periodCallbacks.current[period.shortname]) { - periodCallbacks.current[period.shortname] = (newSeconds: number) => - handleCellChange(activeSegmentIndex, period.shortname, newSeconds); - } - }); - // Update callbacks when activeSegmentIndex changes - React.useEffect(() => { - periods.forEach((period) => { - periodCallbacks.current[period.shortname] = (newSeconds: number) => - handleCellChange(activeSegmentIndex, period.shortname, newSeconds); - }); - }, [activeSegmentIndex]); + const periodCallbacks = React.useMemo<Record<string, (newSeconds: number) => void>>( + () => + Object.fromEntries( + periods.map((period) => [ + period.shortname, + (newSeconds: number) => handleCellChange(activeSegmentIndex, period.shortname, newSeconds) + ]) + ), + [activeSegmentIndex, handleCellChange, periods] + );Then update Line 164:
- onSegmentTimeChange={periodCallbacks.current[period.shortname]} + onSegmentTimeChange={periodCallbacks[period.shortname]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx` around lines 91 - 104, The periodCallbacks stored in periodCallbacks.current are recreated in a useEffect that only depends on activeSegmentIndex, but handleCellChange closes over selectedServiceId so callbacks become stale when selectedServiceId changes; update the creation logic so callbacks are refreshed whenever selectedServiceId or periods change (e.g., include selectedServiceId and periods in the React.useEffect dependency array or build the callbacks with React.useMemo that depends on [activeSegmentIndex, selectedServiceId, periods]) so periodCallbacks.current[period.shortname] always calls handleCellChange with the current selectedServiceId and activeSegmentIndex.packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts (4)
379-387:⚠️ Potential issue | 🟡 MinorPlease clear all per-service checkpoint target keys when removing a checkpoint.
The target map is keyed with
${checkpointKey}_${serviceId}. Deleting only the base key leaves stale values.Suggested fix
-const key = getCheckpointKey(resolved); +const keyPrefix = `${getCheckpointKey(resolved)}_`; setCheckpointTargets((prev) => { const next = { ...prev }; - delete next[key]; + for (const key of Object.keys(next)) { + if (key === getCheckpointKey(resolved) || key.startsWith(keyPrefix)) { + delete next[key]; + } + } return next; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` around lines 379 - 387, When removing a checkpoint in useSegmentTimesByPeriod (the block using getCheckpointKey, setCheckpoints and setCheckpointTargets), also remove all per-service entries keyed as `${key}_${serviceId}` instead of only deleting the base key; update the setCheckpointTargets callback to iterate the existing keys on the next object and delete any property that startsWith(`${key}_`) (or matches the `${key}_${serviceId}` pattern) so no stale per-service checkpoint target keys remain.
171-179:⚠️ Potential issue | 🟡 MinorPlease localize the fallback period label and save-error fallback text.
These are user-facing strings and should go through
t().Suggested fix
+import { useTranslation } from 'react-i18next'; ... +const { t } = useTranslation(); -`${shortname} (${period.start_at_hour}h-${period.end_at_hour}h)` +t('transit:path.segmentTimes.periodFallback', { + shortname, + start: period.start_at_hour, + end: period.end_at_hour, + lng: language +}) -setSaveError((error as Error).message || 'An error occurred while saving segment times.'); +setSaveError( + (error as Error).message || + t('transit:path.segmentTimes.saveError', { lng: language }) +);As per coding guidelines, "Use i18next and the
t()function for all user-facing strings to support internationalization."Also applies to: 476-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` around lines 171 - 179, The fallback user-facing strings are not localized: wrap the generated period label fallback (currently `${shortname} (${period.start_at_hour}h-${period.end_at_hour}h)`) used when building the name in useSegmentTimesByPeriod and the save-error fallback text referenced later (around the save handler) with the i18next t() function; update the logic that composes the label to call t(...) with appropriate keys and interpolation values (referencing periodNamesByShortname, shortname, start_at_hour, end_at_hour) and replace the hardcoded save-error fallback string with t(...) as well so both user-facing fallbacks go through translation.
246-276:⚠️ Potential issue | 🟠 MajorCheckpoint redistribution is using the selected service instead of the iterated service during save.
getCheckpointCurrentTotal/ target-key helpers close overselectedServiceId, buthandleSaveiterates all groups. This can skip or mis-scale non-selected groups.Suggested fix (pass serviceId explicitly)
-const getCheckpointCurrentTotal = (checkpoint: ResolvedCheckpoint, periodShortname: string): number => { +const getCheckpointCurrentTotal = ( + serviceId: string, + checkpoint: ResolvedCheckpoint, + periodShortname: string +): number => { let total = 0; for (let i = checkpoint.fromNodeIndex; i < checkpoint.toNodeIndex; i++) { - total += getTimeForCell(i, periodShortname); + const override = localData[serviceId]?.[periodShortname]?.[i]; + total += override !== undefined ? override : getDefaultTime(i, periodShortname); } return total; }; -const getCheckpointTargetKey = (checkpoint: ResolvedCheckpoint): string => - `${getCheckpointKey(checkpoint)}_${selectedServiceId}`; +const getCheckpointTargetKey = (checkpoint: ResolvedCheckpoint, serviceId: string): string => + `${getCheckpointKey(checkpoint)}_${serviceId}`; -const currentTotal = getCheckpointCurrentTotal(checkpoint, periodShortname); +const currentTotal = getCheckpointCurrentTotal(serviceId, checkpoint, periodShortname); -const key = getCheckpointTargetKey(checkpoint); +const key = getCheckpointTargetKey(checkpoint, selectedServiceId); -const key = getCheckpointTargetKey(checkpoint); +const key = getCheckpointTargetKey(checkpoint, selectedServiceId);Also applies to: 302-309, 404-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` around lines 246 - 276, The helpers getCheckpointCurrentTotal, getCheckpointTargetKey, and getCheckpointTarget currently close over selectedServiceId causing incorrect calculations when handleSave iterates multiple services; change these functions to accept an explicit serviceId parameter (e.g., getCheckpointCurrentTotal(checkpoint, periodShortname, serviceId), getCheckpointTargetKey(checkpoint, serviceId), getCheckpointTarget(checkpoint, periodShortname, serviceId)) and update every caller (including handleSave's loop over groups and any other references noted around lines ~302-309 and ~404-431) to pass the iterated serviceId instead of relying on the module-level selectedServiceId.
49-49:⚠️ Potential issue | 🟠 MajorPlease remove the
anyescapes in this hook.These spots handle schedule/period/checkpoint shapes and should be strongly typed to prevent runtime shape mismatches.
Suggested typed direction
-const line: any = path.getLine(); +const line = path.getLine(); +type LegacyCheckpoint = Checkpoint | { fromNodeIndex: number; toNodeIndex: number }; +type SchedulePeriod = { + period_shortname?: string; + start_at_hour: number; + end_at_hour: number; + trips?: unknown[]; +}; -const raw = _cloneDeep(path.attributes.data.segmentTimesCheckpoints || []); +const raw = _cloneDeep((path.attributes.data.segmentTimesCheckpoints || []) as LegacyCheckpoint[]); -const collectPeriodsWithTripsForGroup = (group: ServiceGroup | undefined): any[] => { - const periodsByShortname = new Map<string, any>(); +const collectPeriodsWithTripsForGroup = (group: ServiceGroup | undefined): SchedulePeriod[] => { + const periodsByShortname = new Map<string, SchedulePeriod>(); - .map((period: any) => { + .map((period: SchedulePeriod) => {As per coding guidelines, "Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application."Also applies to: 102-103, 140-141, 159-159, 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` at line 49, Replace all `any` escapes in this hook by defining and using proper TypeScript interfaces for the domain shapes (e.g., Line, Schedule, Period, Checkpoint) and apply them to the variables and return types in useSegmentTimesByPeriod; specifically replace the `const line: any = path.getLine()` with a typed call/assignment (e.g., const line: Line = path.getLine() or change path.getLine(): Line), and similarly replace other `any` usages at the other flagged locations (lines handling schedule/period/checkpoint shapes) so variables and function signatures consistently use the new interfaces; ensure any helper functions inside useSegmentTimesByPeriod and its params/returns are updated to accept/return these concrete types to preserve type-safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28917928-39f3-4740-9810-d1bbf34bec89
📒 Files selected for processing (46)
locales/en/main.jsonlocales/en/transit.jsonlocales/fr/main.jsonlocales/fr/transit.jsonpackages/transition-backend/src/api/__tests__/gtfs.routes.test.tspackages/transition-backend/src/services/gtfsExport/ScheduleExporter.tspackages/transition-backend/src/services/gtfsExport/__tests__/ScheduleExporter.test.tspackages/transition-backend/src/services/gtfsImport/GtfsImportTypes.tspackages/transition-backend/src/services/gtfsImport/GtfsImporter.tspackages/transition-backend/src/services/gtfsImport/PathImporter.tspackages/transition-backend/src/services/gtfsImport/ScheduleImporter.tspackages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.tspackages/transition-backend/src/services/path/PathGtfsGeographyGenerator.tspackages/transition-backend/src/services/path/__tests__/PathGtfsGeographyGenerator.test.tspackages/transition-common/src/services/gtfs/GtfsImportTypes.tspackages/transition-common/src/services/path/Path.tspackages/transition-common/src/services/path/PathGeographyGenerator.tspackages/transition-common/src/services/path/PathGeographyUtils.tspackages/transition-common/src/services/path/PathSegmentTimeUtils.tspackages/transition-common/src/services/path/PathServiceGrouping.tspackages/transition-common/src/services/path/PathTypes.tspackages/transition-common/src/services/path/__tests__/Path.test.tspackages/transition-common/src/services/path/__tests__/PathGeographyGenerator.test.tspackages/transition-common/src/services/path/__tests__/PathGeographyUtils.test.tspackages/transition-common/src/services/path/__tests__/PathSegmentTimeUtils.test.tspackages/transition-common/src/services/path/__tests__/PathServiceGrouping.test.tspackages/transition-common/src/services/periods/Period.tspackages/transition-common/src/services/periods/__tests__/Period.test.tspackages/transition-common/src/services/schedules/Schedule.tspackages/transition-common/src/services/schedules/__tests__/Schedule.test.tspackages/transition-frontend/src/components/forms/gtfs/GtfsImportForm.tsxpackages/transition-frontend/src/components/forms/path/TransitPathEdit.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/CheckpointPeriodTimesTable.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/ScrollableDropdown.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentCarousel.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentTimesToolbar.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TimeInput.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/CheckpointLines.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/NodeDots.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/SegmentLines.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitLineOverview/TransitLineOverview.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitPathSegmentTimesByPeriodModal.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.tspackages/transition-frontend/src/styles/_segmentTimesByPeriodModal.scsspackages/transition-frontend/src/styles/transition.scss
Comment on lines
+544
to
+629
| describe('Timepoint to checkpoint mapping', () => { | ||
| beforeEach(() => { | ||
| importData.shapeById = {}; | ||
| importData.shapeById['timepointShape'] = [ | ||
| { shape_id: 'timepointShape', shape_pt_lat: 45.538, shape_pt_lon: -73.614, shape_pt_sequence: 0 }, | ||
| { shape_id: 'timepointShape', shape_pt_lat: 45.539, shape_pt_lon: -73.613, shape_pt_sequence: 5 }, | ||
| { shape_id: 'timepointShape', shape_pt_lat: 45.540, shape_pt_lon: -73.612, shape_pt_sequence: 10 }, | ||
| { shape_id: 'timepointShape', shape_pt_lat: 45.541, shape_pt_lon: -73.611, shape_pt_sequence: 20 }, | ||
| ]; | ||
| importData.stopCoordinatesByStopId = { | ||
| stop1: [-73.614, 45.538] as [number, number], | ||
| stop2: [-73.613, 45.539] as [number, number], | ||
| stop3: [-73.612, 45.540] as [number, number], | ||
| stop4: [-73.611, 45.541] as [number, number] | ||
| }; | ||
| }); | ||
|
|
||
| test('Creates checkpoints from mixed timepoint values', async () => { | ||
| const tripId = 'timepointTrip'; | ||
| const tripsByRouteId = {}; | ||
| tripsByRouteId[routeId] = [{ | ||
| trip: { route_id: routeId, service_id: uuidV4(), trip_id: tripId, trip_headsign: 'Test', direction_id: 0, shape_id: 'timepointShape' }, | ||
| stopTimes: [ | ||
| { trip_id: tripId, stop_id: 'stop1', stop_sequence: 1, arrivalTimeSeconds: 36000, departureTimeSeconds: 36000, timepoint: 1 }, | ||
| { trip_id: tripId, stop_id: 'stop2', stop_sequence: 2, arrivalTimeSeconds: 36090, departureTimeSeconds: 36100, timepoint: 0 }, | ||
| { trip_id: tripId, stop_id: 'stop3', stop_sequence: 3, arrivalTimeSeconds: 36180, departureTimeSeconds: 36200, timepoint: 1 }, | ||
| { trip_id: tripId, stop_id: 'stop4', stop_sequence: 4, arrivalTimeSeconds: 36300, departureTimeSeconds: 36300, timepoint: 1 } | ||
| ] | ||
| }]; | ||
|
|
||
| const result = await PathImporter.generateAndImportPaths(tripsByRouteId, importData, collectionManager) as any; | ||
| expect(result.status).toEqual('success'); | ||
|
|
||
| const createdPathAttribs = (pathsDbQueries.createMultiple as any).mock.calls[0][0][0]; | ||
| // Timepoint stops: stop1 (nodeId1), stop3 (nodeId3), stop4 (nodeId4) | ||
| // → checkpoints: [{nodeId1, nodeId3}, {nodeId3, nodeId4}] | ||
| expect(createdPathAttribs.data.segmentTimesCheckpoints).toEqual([ | ||
| { fromNodeId: nodeId1, toNodeId: nodeId3 }, | ||
| { fromNodeId: nodeId3, toNodeId: nodeId4 } | ||
| ]); | ||
| }); | ||
|
|
||
| test('Does not create checkpoints when all stops are timepoints', async () => { | ||
| const tripId = 'allTimepointTrip'; | ||
| const tripsByRouteId = {}; | ||
| tripsByRouteId[routeId] = [{ | ||
| trip: { route_id: routeId, service_id: uuidV4(), trip_id: tripId, trip_headsign: 'Test', direction_id: 0, shape_id: 'timepointShape' }, | ||
| stopTimes: [ | ||
| { trip_id: tripId, stop_id: 'stop1', stop_sequence: 1, arrivalTimeSeconds: 36000, departureTimeSeconds: 36000, timepoint: 1 }, | ||
| { trip_id: tripId, stop_id: 'stop2', stop_sequence: 2, arrivalTimeSeconds: 36090, departureTimeSeconds: 36100, timepoint: 1 }, | ||
| { trip_id: tripId, stop_id: 'stop3', stop_sequence: 3, arrivalTimeSeconds: 36180, departureTimeSeconds: 36200, timepoint: 1 }, | ||
| { trip_id: tripId, stop_id: 'stop4', stop_sequence: 4, arrivalTimeSeconds: 36300, departureTimeSeconds: 36300, timepoint: 1 } | ||
| ] | ||
| }]; | ||
|
|
||
| const result = await PathImporter.generateAndImportPaths(tripsByRouteId, importData, collectionManager) as any; | ||
| expect(result.status).toEqual('success'); | ||
|
|
||
| const createdPathAttribs = (pathsDbQueries.createMultiple as any).mock.calls[0][0][0]; | ||
| expect(createdPathAttribs.data.segmentTimesCheckpoints).toBeUndefined(); | ||
| }); | ||
|
|
||
| test('Creates checkpoints from timepoints without shape', async () => { | ||
| const tripId = 'noShapeTimepointTrip'; | ||
| const tripsByRouteId = {}; | ||
| tripsByRouteId[routeId] = [{ | ||
| trip: { route_id: routeId, service_id: uuidV4(), trip_id: tripId, trip_headsign: 'Test', direction_id: 0 }, | ||
| stopTimes: [ | ||
| { trip_id: tripId, stop_id: 'stop1', stop_sequence: 1, arrivalTimeSeconds: 36000, departureTimeSeconds: 36000, timepoint: 1 }, | ||
| { trip_id: tripId, stop_id: 'stop2', stop_sequence: 2, arrivalTimeSeconds: 36090, departureTimeSeconds: 36100, timepoint: 0 }, | ||
| { trip_id: tripId, stop_id: 'stop3', stop_sequence: 3, arrivalTimeSeconds: 36180, departureTimeSeconds: 36200, timepoint: 0 }, | ||
| { trip_id: tripId, stop_id: 'stop4', stop_sequence: 4, arrivalTimeSeconds: 36300, departureTimeSeconds: 36300, timepoint: 1 } | ||
| ] | ||
| }]; | ||
|
|
||
| const result = await PathImporter.generateAndImportPaths(tripsByRouteId, importData, collectionManager) as any; | ||
| expect(result.status).toEqual('success'); | ||
|
|
||
| const createdPathAttribs = (pathsDbQueries.createMultiple as any).mock.calls[0][0][0]; | ||
| // Timepoint stops: stop1 (nodeId1), stop4 (nodeId4) | ||
| // → checkpoints: [{nodeId1, nodeId4}] | ||
| expect(createdPathAttribs.data.segmentTimesCheckpoints).toEqual([ | ||
| { fromNodeId: nodeId1, toNodeId: nodeId4 } | ||
| ]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use test.each for the checkpoint-mapping matrix.
These three cases only vary the stop-time fixture and expected checkpoints. A table-driven test will be easier to extend and will pinpoint the failing scenario faster.
As per coding guidelines, "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-backend/src/services/gtfsImport/__tests__/PathImporter.test.ts`
around lines 544 - 629, These three similar tests in PathImporter.test.ts (test
names: "Creates checkpoints from mixed timepoint values", "Does not create
checkpoints when all stops are timepoints", and "Creates checkpoints from
timepoints without shape") should be converted to a single table-driven test
using test.each: create an array of cases where each case supplies the stopTimes
fixture and the expected segmentTimesCheckpoints (use undefined for no
checkpoints), then inside the test body build tripsByRouteId/trip objects as
before, call PathImporter.generateAndImportPaths, and assert that
(pathsDbQueries.createMultiple as
any).mock.calls[0][0][0].data.segmentTimesCheckpoints matches the expected
value; reference PathImporter.generateAndImportPaths, tripsByRouteId, and
pathsDbQueries.createMultiple to locate where to plug the parameters and
assertions.
Comment on lines
+591
to
+709
| describe('computeSegmentTimesByServiceAndPeriod', () => { | ||
| const makeTrip = (times: [number, number][], serviceId: string): TripStopTimesWithService => ({ | ||
| stopTimes: times.map(([arrival, departure], i) => ({ | ||
| trip_id: 'trip1', | ||
| stop_id: `stop${i}`, | ||
| stop_sequence: i, | ||
| arrivalTimeSeconds: arrival, | ||
| departureTimeSeconds: departure | ||
| })), | ||
| serviceId | ||
| }); | ||
|
|
||
| const amPeak: Period = { shortname: 'am_peak', name: { en: 'AM Peak' }, startAtHour: 6, endAtHour: 9 }; | ||
| const midday: Period = { shortname: 'midday', name: { en: 'Midday' }, startAtHour: 9, endAtHour: 15 }; | ||
| const pmPeak: Period = { shortname: 'pm_peak', name: { en: 'PM Peak' }, startAtHour: 15, endAtHour: 18 }; | ||
| const periods: Period[] = [amPeak, midday, pmPeak]; | ||
|
|
||
| test('should return empty object when periods array is empty', () => { | ||
| const trip = makeTrip([[36000, 36000], [36100, 36100]], 'svc1'); | ||
| const result = computeSegmentTimesByServiceAndPeriod([trip], [null], [], 1000); | ||
| expect(result).toEqual({}); | ||
| }); | ||
|
|
||
| test('should group by period and service', () => { | ||
| // 7:00 AM trip, service A — 100s segment | ||
| const amTripA = makeTrip([[25200, 25200], [25300, 25300]], 'svcA'); | ||
| // 7:30 AM trip, service B — 120s segment | ||
| const amTripB = makeTrip([[27000, 27000], [27120, 27120]], 'svcB'); | ||
| // 10:00 AM trip, service A — 80s segment | ||
| const middayTripA = makeTrip([[36000, 36000], [36080, 36080]], 'svcA'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([amTripA, amTripB, middayTripA], [500], periods, 1000); | ||
|
|
||
| expect(Object.keys(result)).toEqual(['svcA', 'svcB']); | ||
| expect(Object.keys(result['svcA'])).toEqual(['am_peak', 'midday']); | ||
| expect(result['svcA']['am_peak'].segments[0].travelTimeSeconds).toEqual(100); | ||
| expect(result['svcA']['am_peak'].tripCount).toEqual(1); | ||
| expect(result['svcB']['am_peak'].segments[0].travelTimeSeconds).toEqual(120); | ||
| expect(result['svcB']['am_peak'].tripCount).toEqual(1); | ||
| expect(Object.keys(result['svcA'])).toEqual(['am_peak', 'midday']); | ||
| expect(result['svcA']['midday'].segments[0].travelTimeSeconds).toEqual(80); | ||
| }); | ||
|
|
||
| test('should average trips within same period and service', () => { | ||
| const trip1 = makeTrip([[25200, 25200], [25300, 25310]], 'svcA'); | ||
| const trip2 = makeTrip([[27000, 27000], [27120, 27130]], 'svcA'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([trip1, trip2], [500], periods, 1000); | ||
|
|
||
| expect(result['svcA']['am_peak'].tripCount).toEqual(2); | ||
| expect(result['svcA']['am_peak'].segments[0].travelTimeSeconds).toEqual(110); | ||
| }); | ||
|
|
||
| test('should assign trips shortly before first period to first period', () => { | ||
| // 3:00 AM (10800s) is 3h before am_peak start (6:00 AM), within 6h overflow | ||
| const earlyTrip = makeTrip([[10800, 10800], [10900, 10900]], 'svc1'); | ||
| const amTrip = makeTrip([[25200, 25200], [25300, 25300]], 'svc1'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([earlyTrip, amTrip], [null], periods, 1000); | ||
|
|
||
| expect(Object.keys(result)).toEqual(['svc1']); | ||
| expect(result['svc1']['am_peak'].tripCount).toEqual(2); | ||
| }); | ||
|
|
||
| test('should assign trip shortly after last period to last period', () => { | ||
| // 20:00 is 2h past pm_peak end (18:00), within 6h overflow | ||
| const lateTrip = makeTrip([[72000, 72000], [72100, 72100]], 'svc1'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([lateTrip], [null], periods, 1000); | ||
|
|
||
| expect(Object.keys(result)).toEqual(['svc1']); | ||
| expect(result['svc1']['pm_peak'].tripCount).toEqual(1); | ||
| }); | ||
|
|
||
| test('should skip trip far beyond last period overflow threshold', () => { | ||
| // 01:00 next day = 90000s, which is 7h past pm_peak end (18:00), beyond 6h overflow | ||
| const veryLateTrip = makeTrip([[90000, 90000], [90100, 90100]], 'svc1'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([veryLateTrip], [null], periods, 1000); | ||
|
|
||
| expect(result).toEqual({}); | ||
| }); | ||
|
|
||
| test('should calculate speeds from total distance and period times', () => { | ||
| const trip = makeTrip([ | ||
| [25200, 25200], | ||
| [25300, 25310], | ||
| [25410, 25410] | ||
| ], 'svc1'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([trip], [500, 500], periods, 1000); | ||
|
|
||
| const periodData = result['svc1']['am_peak']; | ||
| expect(periodData.travelTimeWithoutDwellTimesSeconds).toEqual(200); | ||
| expect(periodData.operatingTimeWithoutLayoverTimeSeconds).toEqual(210); | ||
| expect(periodData.averageSpeedWithoutDwellTimesMetersPerSecond).toEqual(5); | ||
| expect(periodData.operatingSpeedMetersPerSecond).toEqual(4.76); | ||
| }); | ||
|
|
||
| test('should set speeds to 0 when travel time is zero', () => { | ||
| const trip = makeTrip([[25200, 25200], [25200, 25200]], 'svc1'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([trip], [null], periods, 1000); | ||
|
|
||
| expect(result['svc1']['am_peak'].averageSpeedWithoutDwellTimesMetersPerSecond).toEqual(0); | ||
| expect(result['svc1']['am_peak'].operatingSpeedMetersPerSecond).toEqual(0); | ||
| }); | ||
|
|
||
| test('should keep different services separate in the same period', () => { | ||
| // Same departure time, different services, different travel times | ||
| const weekdayTrip = makeTrip([[25200, 25200], [25400, 25400]], 'weekday_svc'); | ||
| const weekendTrip = makeTrip([[25200, 25200], [25300, 25300]], 'weekend_svc'); | ||
|
|
||
| const result = computeSegmentTimesByServiceAndPeriod([weekdayTrip, weekendTrip], [null], periods, 1000); | ||
|
|
||
| expect(result['weekday_svc']['am_peak'].segments[0].travelTimeSeconds).toEqual(200); | ||
| expect(result['weekend_svc']['am_peak'].segments[0].travelTimeSeconds).toEqual(100); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Convert this service/period matrix to test.each.
The new cases mostly differ by trip times, service IDs, and expected bucket values. A parameterized table will cut duplication and make the failing scenario clearer.
As per coding guidelines, "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-backend/src/services/path/__tests__/PathGtfsGeographyGenerator.test.ts`
around lines 591 - 709, Multiple similar tests in this suite should be
consolidated into a parameterized table using Jest's test.each to reduce
duplication and clarify failures; create a table of cases (columns: name, trips
array built with makeTrip, distances array, periods, resolution, expected
keys/values) and replace the individual tests ('should group by period and
service', 'should average trips...', 'should assign trips shortly before...',
'should assign trip shortly after...', 'should skip trip...', 'should calculate
speeds...', 'should set speeds to 0...', 'should keep different services
separate...') with a single test.each block that calls
computeSegmentTimesByServiceAndPeriod for each row and asserts the expected
outputs, referencing makeTrip and computeSegmentTimesByServiceAndPeriod to build
inputs and validate outputs; ensure each row includes clear expected assertions
(service keys, period keys, tripCount, segment travelTimeSeconds, computed
speeds) and keep the original helper Periods (amPeak, midday, pmPeak) and
makeTrip factory for reuse.
Comment on lines
+786
to
+990
| describe('getSegmentTravelTimeForPeriod', () => { | ||
| const lineId = uuidV4(); | ||
| const segmentsData = [ | ||
| { distanceMeters: 510, travelTimeSeconds: 120 }, | ||
| { distanceMeters: 515, travelTimeSeconds: 90 }, | ||
| { distanceMeters: 444, travelTimeSeconds: 85 } | ||
| ]; | ||
|
|
||
| test('should return base segment time when no period data exists', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).segments = segmentsData; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimeForPeriod(0, 'am_peak')).toBe(120); | ||
| expect(path.getSegmentTravelTimeForPeriod(1, 'am_peak')).toBe(90); | ||
| expect(path.getSegmentTravelTimeForPeriod(2, 'am_peak')).toBe(85); | ||
| }); | ||
|
|
||
| test('should return period-specific time when data exists', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).segments = segmentsData; | ||
| (attributes.data as any).segmentsByServiceAndPeriod = { | ||
| service1: { | ||
| am_peak: { | ||
| segments: [ | ||
| { distanceMeters: 510, travelTimeSeconds: 200 }, | ||
| { distanceMeters: 515, travelTimeSeconds: 150 }, | ||
| { distanceMeters: 444, travelTimeSeconds: 130 } | ||
| ], | ||
| dwellTimeSeconds: [0, 0, 0, 0], | ||
| travelTimeWithoutDwellTimesSeconds: 480, | ||
| operatingTimeWithoutLayoverTimeSeconds: 480, | ||
| averageSpeedWithoutDwellTimesMetersPerSecond: 3.06, | ||
| operatingSpeedMetersPerSecond: 3.06, | ||
| tripCount: 1 | ||
| } | ||
| } | ||
| }; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimeForPeriod(0, 'am_peak')).toBe(200); | ||
| expect(path.getSegmentTravelTimeForPeriod(1, 'am_peak')).toBe(150); | ||
| expect(path.getSegmentTravelTimeForPeriod(2, 'am_peak')).toBe(130); | ||
| }); | ||
|
|
||
| test('should return base time for period without data', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).segments = segmentsData; | ||
| (attributes.data as any).segmentsByServiceAndPeriod = { | ||
| service1: { | ||
| am_peak: { | ||
| segments: [ | ||
| { distanceMeters: 510, travelTimeSeconds: 200 }, | ||
| { distanceMeters: 515, travelTimeSeconds: 150 }, | ||
| { distanceMeters: 444, travelTimeSeconds: 130 } | ||
| ], | ||
| dwellTimeSeconds: [0, 0, 0, 0], | ||
| travelTimeWithoutDwellTimesSeconds: 480, | ||
| operatingTimeWithoutLayoverTimeSeconds: 480, | ||
| averageSpeedWithoutDwellTimesMetersPerSecond: 3.06, | ||
| operatingSpeedMetersPerSecond: 3.06, | ||
| tripCount: 1 | ||
| } | ||
| } | ||
| }; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimeForPeriod(0, 'midday')).toBe(120); | ||
| }); | ||
|
|
||
| test('should return undefined for out-of-range segment index', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).segments = segmentsData; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimeForPeriod(999, 'am_peak')).toBeUndefined(); | ||
| }); | ||
|
|
||
| test('should return undefined when no segments exist', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| delete (attributes.data as any).segments; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimeForPeriod(0, 'am_peak')).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getSegmentTravelTimesForPeriod', () => { | ||
| const lineId = uuidV4(); | ||
| const segmentsData = [ | ||
| { distanceMeters: 510, travelTimeSeconds: 120 }, | ||
| { distanceMeters: 515, travelTimeSeconds: 90 }, | ||
| { distanceMeters: 444, travelTimeSeconds: 85 } | ||
| ]; | ||
|
|
||
| test('should return base times when no period data exists', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).segments = segmentsData; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimesForPeriod('am_peak')).toEqual([120, 90, 85]); | ||
| }); | ||
|
|
||
| test('should return period times when data exists', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).segments = segmentsData; | ||
| (attributes.data as any).segmentsByServiceAndPeriod = { | ||
| service1: { | ||
| am_peak: { | ||
| segments: [ | ||
| { distanceMeters: 510, travelTimeSeconds: 200 }, | ||
| { distanceMeters: 515, travelTimeSeconds: 150 }, | ||
| { distanceMeters: 444, travelTimeSeconds: 130 } | ||
| ], | ||
| dwellTimeSeconds: [0, 0, 0, 0], | ||
| travelTimeWithoutDwellTimesSeconds: 480, | ||
| operatingTimeWithoutLayoverTimeSeconds: 480, | ||
| averageSpeedWithoutDwellTimesMetersPerSecond: 3.06, | ||
| operatingSpeedMetersPerSecond: 3.06, | ||
| tripCount: 1 | ||
| } | ||
| } | ||
| }; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimesForPeriod('am_peak')).toEqual([200, 150, 130]); | ||
| }); | ||
|
|
||
| test('should fall back to base for periods without data', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).segments = segmentsData; | ||
| (attributes.data as any).segmentsByServiceAndPeriod = { | ||
| service1: { | ||
| am_peak: { | ||
| segments: [ | ||
| { distanceMeters: 510, travelTimeSeconds: 200 }, | ||
| { distanceMeters: 515, travelTimeSeconds: 150 }, | ||
| { distanceMeters: 444, travelTimeSeconds: 130 } | ||
| ], | ||
| dwellTimeSeconds: [0, 0, 0, 0], | ||
| travelTimeWithoutDwellTimesSeconds: 480, | ||
| operatingTimeWithoutLayoverTimeSeconds: 480, | ||
| averageSpeedWithoutDwellTimesMetersPerSecond: 3.06, | ||
| operatingSpeedMetersPerSecond: 3.06, | ||
| tripCount: 1 | ||
| } | ||
| } | ||
| }; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimesForPeriod('midday')).toEqual([120, 90, 85]); | ||
| }); | ||
|
|
||
| test('should return empty array when no segments exist', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| delete (attributes.data as any).segments; | ||
| const path = new Path(attributes, false); | ||
| expect(path.getSegmentTravelTimesForPeriod('am_peak')).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getSegmentsForPeriod - averaging with tripCount 0', () => { | ||
| const lineId = uuidV4(); | ||
|
|
||
| test('should average correctly when all services have tripCount 0', () => { | ||
| const attributes = getPathAttributesWithData(true, { lineId }); | ||
| (attributes.data as any).totalDistanceMeters = 1000; | ||
| (attributes.data as any).segmentsByServiceAndPeriod = { | ||
| service1: { | ||
| am_peak: { | ||
| segments: [ | ||
| { distanceMeters: 500, travelTimeSeconds: 100 }, | ||
| { distanceMeters: 500, travelTimeSeconds: 200 } | ||
| ], | ||
| dwellTimeSeconds: [0, 10, 15], | ||
| travelTimeWithoutDwellTimesSeconds: 300, | ||
| operatingTimeWithoutLayoverTimeSeconds: 325, | ||
| averageSpeedWithoutDwellTimesMetersPerSecond: 3.33, | ||
| operatingSpeedMetersPerSecond: 3.08, | ||
| tripCount: 0 | ||
| } | ||
| }, | ||
| service2: { | ||
| am_peak: { | ||
| segments: [ | ||
| { distanceMeters: 500, travelTimeSeconds: 200 }, | ||
| { distanceMeters: 500, travelTimeSeconds: 400 } | ||
| ], | ||
| dwellTimeSeconds: [0, 20, 25], | ||
| travelTimeWithoutDwellTimesSeconds: 600, | ||
| operatingTimeWithoutLayoverTimeSeconds: 645, | ||
| averageSpeedWithoutDwellTimesMetersPerSecond: 1.67, | ||
| operatingSpeedMetersPerSecond: 1.55, | ||
| tripCount: 0 | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const path = new Path(attributes, false); | ||
| const result = path.getSegmentsForPeriod('am_peak'); | ||
|
|
||
| expect(result).toBeDefined(); | ||
| // With equal weights, should be simple average: (100+200)/2=150, (200+400)/2=300 | ||
| expect(result!.segments[0].travelTimeSeconds).toBe(150); | ||
| expect(result!.segments[1].travelTimeSeconds).toBe(300); | ||
| expect(result!.dwellTimeSeconds[1]).toBe(15); // (10+20)/2 | ||
| // Verify no NaN values | ||
| expect(Number.isFinite(result!.segments[0].travelTimeSeconds)).toBe(true); | ||
| expect(Number.isFinite(result!.segments[1].travelTimeSeconds)).toBe(true); | ||
| expect(Number.isFinite(result!.dwellTimeSeconds[1])).toBe(true); | ||
| expect(Number.isFinite(result!.averageSpeedWithoutDwellTimesMetersPerSecond)).toBe(true); | ||
| expect(Number.isFinite(result!.operatingSpeedMetersPerSecond)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Please collapse these period-API cases into each(...) tables.
Most of this block repeats the same attribute setup and only changes the period payload and expected values. Table-driven cases will be shorter and easier to debug.
As per coding guidelines, "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-common/src/services/path/__tests__/Path.test.ts` around
lines 786 - 990, The tests duplicate identical setup for Path and only vary
period and expected outputs; refactor the repetitive cases for
getSegmentTravelTimeForPeriod, getSegmentTravelTimesForPeriod and the
period-specific expectations into table-driven tests using test.each (or
describe.each) with rows containing the period name, index (when needed) and
expected values; keep the shared setup (creating attributes, assigning segments
and segmentsByServiceAndPeriod, and new Path(attributes, false)) outside the
table and inside a beforeEach or the iterator body, then assert expected scalar
or array results for each row to replace the multiple similar test blocks.
Comment on lines
+988
to
+999
| // Remap period segment data or clear it on forceRecalculate | ||
| if (initialPeriodData && !changesInfo.forceRecalculate) { | ||
| (newData as any).segmentsByServiceAndPeriod = remapAllServicePeriodSegmentData( | ||
| path, | ||
| initialPeriodData, | ||
| physicsDurations, | ||
| geometryResult.segmentsData.slice(0, realSegmentCount), | ||
| changesInfo | ||
| ); | ||
| } else if (initialPeriodData) { | ||
| (newData as any).segmentsByServiceAndPeriod = undefined; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider avoiding as any cast.
The cast on line 990 bypasses type safety. If newData type doesn't include segmentsByServiceAndPeriod, consider extending the return type of buildPathData or using a properly typed intermediate object.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-common/src/services/path/PathGeographyGenerator.ts`
around lines 988 - 999, The code is using an unsafe cast "(newData as
any).segmentsByServiceAndPeriod"; instead, add a properly typed field to the
buildPathData return type (or extend its interface) so newData includes
segmentsByServiceAndPeriod, then assign the remapAllServicePeriodSegmentData
result directly to newData.segmentsByServiceAndPeriod; update related
types/interfaces in PathGeographyGenerator (and any callers) to accept the new
property, and remove the "as any" cast when calling
remapAllServicePeriodSegmentData(path, initialPeriodData, physicsDurations,
geometryResult.segmentsData.slice(0, realSegmentCount), changesInfo).
Comment on lines
+371
to
+383
| const builtServiceGroups = Object.values(serviceGroups).map((ids): ServiceGroup => { | ||
| const activeDaysSet = new Set<string>(); | ||
| for (const serviceId of ids) { | ||
| const service = servicesCollection?.getById(serviceId); | ||
| if (!service || isHolidayService(service)) continue; | ||
| allDayKeys.filter((day) => service.get(day)).forEach((d) => activeDaysSet.add(d)); | ||
| } | ||
| const averageTimesByPeriod = getAverageSegmentTimesByPeriod(path, ids[0]); | ||
| return { | ||
| serviceIds: ids, | ||
| activeDays: [...activeDaysSet], | ||
| hasHolidayService: false, | ||
| averageTimesByPeriod |
There was a problem hiding this comment.
Average the grouped services instead of picking ids[0].
Line 378 uses the first service as averageTimesByPeriod. These buckets are built from bucketed total times, not identical per-segment profiles, so the modal can show one service’s segment split and then propagate it to the whole group on save. Compute the per-period per-segment average across ids instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-common/src/services/path/PathServiceGrouping.ts` around
lines 371 - 383, The code currently calls getAverageSegmentTimesByPeriod(path,
ids[0]) and thus uses the first service’s per-period segment buckets for the
whole group; instead compute the per-period, per-segment average across all
non-holiday services in ids: iterate ids, fetch each service via
servicesCollection.getById, skip missing or isHolidayService(service),
accumulate segment bucket totals keyed by period+segment, track counts per
period/segment, then divide totals by counts to produce averageTimesByPeriod for
the group. Either extend getAverageSegmentTimesByPeriod to accept an array of
serviceIds or add a new helper (e.g., getAverageSegmentTimesByPeriodForGroup)
and replace the call in builtServiceGroups so averageTimesByPeriod reflects the
aggregated averages and handle zero-count periods safely.
Comment on lines
+202
to
+205
| {t('transit:transitPath:Checkpoint')} {activeCheckpointIndex + 1}/ | ||
| {checkpoints.length} ( | ||
| {activeCheckpoint.toNodeIndex - activeCheckpoint.fromNodeIndex}{' '} | ||
| {t('transit:transitPath:Segments').toLowerCase()}) |
There was a problem hiding this comment.
Do not lowercase translated strings in code.
toLowerCase() is locale-sensitive and can break non-English translations. Translate the full "(n segments)" fragment instead of transforming t('...:Segments') at runtime.
As per coding guidelines "Use i18next and the t() function for all user-facing strings to support internationalization".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/TransitPathSegmentTimesByPeriodModal.tsx`
around lines 202 - 205, The code is lowercasing a translated string
(t('transit:transitPath:Segments').toLowerCase()), which is locale-sensitive and
violates i18n guidelines; update the TransitPathSegmentTimesByPeriodModal
rendering to remove .toLowerCase() and instead use a single translated key for
the whole "(n segments)" fragment (or a pluralized key using i18next
interpolation/count) referencing activeCheckpoint, activeCheckpointIndex and
checkpoints to compute the count, e.g., add a new translation like
'transit:transitPath:SegmentsCount' or pluralization for segments and call
t(...) with the computed count so the entire fragment is translated correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/transition-frontend/src/services/map/events/NodeLayerMapEvents.ts`:
- Around line 53-55: In NodeLayerMapEvents.ts inside the handler that checks
nodeFeature, clear the hover UI/state before the early return: locate the block
that reads "if (!nodeFeature) { return; }" and add a call to the component's
hover-reset routine (for example invoke the existing clearHover /
resetHoverState / setHoveredNode(null) or emit the hover-out event) immediately
before returning so any stale hover UI is removed during reload/race conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59cfd510-c7d1-4507-afe1-70992add6401
📒 Files selected for processing (2)
packages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-frontend/src/services/map/events/NodeLayerMapEvents.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pr-build-check
- GitHub Check: build-and-run-UI-tests (24.x)
- GitHub Check: test-sequential (24.x)
- GitHub Check: code-lint
- GitHub Check: check-format
- GitHub Check: build-and-test (24.x)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-frontend/src/services/map/events/NodeLayerMapEvents.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/transition-frontend/src/services/map/events/NodeLayerMapEvents.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
📚 Learning: 2026-04-08T18:54:34.951Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1782
File: packages/transition-frontend/src/components/map/TransitionMainMap.tsx:544-563
Timestamp: 2026-04-08T18:54:34.951Z
Learning: In `packages/transition-frontend/src/components/map/TransitionMainMap.tsx`, `handleLayerChange` intentionally uses an optimistic-update pattern: it updates `activeBasemapShortname` state and persists `map.basemapShortname` to preferences immediately, before the async basemap style fetch in `MapRenderer` completes. On fetch failure the map stays on the previous visible style (MapLibre does not crash). The preference records user intent, not rendering state, so transient network failures should not lose the user's selection. There is no `switchBasemap` API or awaitable promise to hook into — the style switch lives in a `useEffect` inside a functional component. Do not flag this pattern as needing rollback logic.
Applied to files:
packages/transition-frontend/src/services/map/events/NodeLayerMapEvents.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.ts
📚 Learning: 2026-04-06T16:24:58.451Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1782
File: packages/transition-frontend/src/components/map/MapRenderer.tsx:131-142
Timestamp: 2026-04-06T16:24:58.451Z
Learning: In `packages/transition-frontend/src/components/map/TransitionMainMap.tsx`, the `MapStyleSwitch` progress notification is intentionally cleared inside `onStyleChange` after map layers are re-added following a style change, not inside the `style.load` handler in `MapRenderer.tsx`. Clearing it earlier (e.g., on `style.load`) would be premature because layers haven't been restored yet at that point.
Applied to files:
packages/transition-frontend/src/services/map/events/NodeLayerMapEvents.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.ts
📚 Learning: 2026-02-06T19:06:19.438Z
Learnt from: GabrielBruno24
Repo: chairemobilite/transition PR: 1767
File: packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx:177-178
Timestamp: 2026-02-06T19:06:19.438Z
Learning: In the transition accessibility map API responses (packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx), the populationData property is always present as an object with the structure { population: number | null; dataSourceAreaRatio: number }. Even when population calculation is disabled or the data source is missing, populationData is returned with population set to null rather than populationData being undefined. Therefore, accessing properties1.populationData.population without optional chaining is safe.
Applied to files:
packages/chaire-lib-frontend/src/services/map/MapLayerManager.ts
🔇 Additional comments (1)
packages/chaire-lib-frontend/src/services/map/MapLayerManager.ts (1)
286-287: Good defensive fix for transient missing sources.Nice change—this prevents
setDatacrashes when MapLibre sources are briefly unavailable.Also applies to: 303-304
Comment on lines
+53
to
+55
| if (!nodeFeature) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Please clear hover state before early return.
At Line 53, returning immediately can leave stale hover UI from a previous node during reload races. Please reset hover state before returning.
Suggested patch
const nodesCollection = serviceLocator.collectionManager?.get('nodes');
const nodeFeature = nodesCollection?.getById(hoverNodeId);
if (!nodeFeature) {
+ removeHoverClass();
+ if (map._hoverNodeIntegerId && map._hoverNodeSource && map._hoverNodeId) {
+ unhoverNode(map._hoverNodeId);
+ e.target.setFeatureState(
+ { source: map._hoverNodeSource, id: map._hoverNodeIntegerId },
+ { size: 1, hover: false }
+ );
+ map._hoverNodeIntegerId = null;
+ map._hoverNodeId = null;
+ map._hoverNodeSource = null;
+ }
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/transition-frontend/src/services/map/events/NodeLayerMapEvents.ts`
around lines 53 - 55, In NodeLayerMapEvents.ts inside the handler that checks
nodeFeature, clear the hover UI/state before the early return: locate the block
that reads "if (!nodeFeature) { return; }" and add a call to the component's
hover-reset routine (for example invoke the existing clearHover /
resetHoverState / setHoveredNode(null) or emit the hover-out event) immediately
before returning so any stale hover UI is removed during reload/race conditions.
tahini
requested changes
Apr 13, 2026
tahini
left a comment
tahini
left a comment
Collaborator
There was a problem hiding this comment.
J'ai survolé la PR, c'est costaud... ça aurait pu être divisé en plusieurs morceaux, checkpoint et division par période, par exemple, sont indépendants et auraient pu être reviewés séparément.
Par contre, j'ai testé le UI. J'aime bien votre fenêtre d'édition des horaires par période. Ça permet de bien voir et éditer les segments et de fine-tuner les horaires, avec les données importées comme référence.
Quelques petits bugs et constatations:
- Si je change la durée d'un segment, ça semble affecter 2 segments: celui que je suis en train d'éditer ET un autre (celui que j'éditais avant?). J'ai sauvegardé la fenêtre et en la réouvrant, il y a 2 segments qui ont mon nouveau temps édité. Ceci devra être corrigé avant de partager la branche. Ça arrive quand je sélectionne le segment en cliquant dessus sur la ligne en haut, en changeant manuellement le nombre de minutes et en cliquant sauvegarder tout de suite après.
- Le choix de service n'est pas clair. Il y a la liste des services pour lesquels un horaire existe (en cochant la case pour les voir individuellement), puis il y a les services par défaut, qui utilise le periodGroup "default", mais si je veux modifier plutôt un autre groupe de période. Je crois qu'il faudrait montrer soit un service particulier, comme vous le faites déjà, ou sinon, utiliser les groupes de période définis dans l'application.
- Pour ce dernier point, je n'ai pas de solution, mais les statistiques montrées ne sont pas modifiées par les changements. C,est normal car elles n'affectent pas les stats globales, mais il faudrait pouvoir voir les stats globales pour chaque période/horaire. Car, dès qu'on a des valeurs différentes par période/horaire, que signifient les stats globales? C'est simplement une moyenne? Il faudrait pouvoir voir et sélectionner une période pour laquelle on veut voir des stats. Ou à défaut d'avoir ça maintenant, au moins avoir un message indiquant que ces stats sont une moyenne et qu'elles sont différentes pour différentes périodes, s'il y a lieu.
- J'ai importé la 45 Papineau de la STM, ça a importé plusieurs services. Mais quand j'ouvre la fenêtre d'édition, si la case de montrer les services individuels n'est pas cochée, il y a 3 services listées: semaine, semaine et semaine... avec chacun des temps différents (qui doivent en fait correspondre aux services semaine, samedi et dimanche). Et il se trouve qu'en modifiant un de ces services de "semaine", ça a aussi modifié le service spécifique correspondant! Ce qui est un peu contre-intuitif. Ça revient un peu à mon 2e point, de repenser le choix de service. Peut-être que dans un premier, ce serait mieux de dire aux gens de 1- générer des horaires une première fois et 2- aller éditer les temps pour le service associé à cet horaire (donc des services particuliers). S'il n'y a pas d'horaire, la fenêtre ne montrera rien (à part inviter les gens à aller créer un horaire). C'est pas idéal, mais mieux vaut ça qu'un comportement encore moins intuitif (à moins que qqn ait une meilleure suggestion à court terme prou le UX de ce feature)
| nodes: [uuidV4(), uuidV4(), uuidV4(), uuidV4()], | ||
| data: { | ||
| segmentTimesCheckpoints: [ | ||
| { fromNodeId: '', toNodeId: '' } // will be set after node IDs are known |
Collaborator
There was a problem hiding this comment.
Puisque vous avez besoin des nodeIds from/to, ici et probablement dans les tests plus tard, vous pouvez les assigner directement dasn une variable avant de créer le Path. const withCheckpointPathNodes.
| const checkpoints = newPath.attributes.data.segmentTimesCheckpoints; | ||
| if (checkpoints && checkpoints.length > 0) { | ||
| for (const cp of checkpoints) { | ||
| checkpointNodeIds.add(cp.fromNodeId); |
Collaborator
There was a problem hiding this comment.
Que se passe-t-il si un noeud d'arrêt faisant partie d'un checkpoint a plus d'une occurence sur la ligne? C'est possible, par exemple, la ligne 36 Monk de la STM autour de Notre-Dame/Sainte-Marguerite, qui revient 2 fois. Il faudrait aussi avoir la séquence de l'arrêt. Mais là, attention de bien le mettre à jour si le path est modifié.
| const allStopTimes = trips.map((t) => t.stopTimes); | ||
| const tripsWithService: TripStopTimesWithService[] = trips.map((t) => ({ | ||
| stopTimes: t.stopTimes, | ||
| serviceId: importData.serviceIdsByGtfsId[t.trip.service_id] ?? t.trip.service_id |
Collaborator
There was a problem hiding this comment.
Normalement, on ne devrait pas avoir de service ID undefined ici, non? Soit on importe le service du trip, dans lequel cas, il devrait y avoir un service ID dans Transition. Soit on ne l'importe cas et là, on ignore.
| return allData[0]; | ||
| } | ||
| if (allData.length > 1) { | ||
| return this._averagePeriodSegmentData(allData); |
Collaborator
There was a problem hiding this comment.
Je ne suis pas certaine que c'est ce qu'on veut dans ce cas-ci: soit on demande pour un service en particulier, comme plus haut, soit on a des valeurs "par défaut" pour un groupe de période donné. Je ne crois pas qu'on veuille faire la moyenne pour tous les services sur cette période, ça n'aurait pas de sens. Par exemple, si on s'est donné le mal de définir des temps semaine et fin de semaine différents et qu'on veut créer un nouvel horaire, ce serait contre-intuitif d'obtenir une moyenne des 2 temps... Je ne saurais dire par contre, ce qu'on veut, peut-être à discuter avec les utilisateurs ou @kaligrafy
| const unitTransit = this.findBestUnit(currentTime, direction, units); | ||
| if (unitTransit) { | ||
| const dwellTimesData = path.getData('dwellTimeSeconds'); | ||
| const periodData = path.getSegmentsForPeriodAndService(periodShortname, serviceId); |
Collaborator
There was a problem hiding this comment.
Cette fonction (ou une autre peut-être) devrait retourner directement les données utiles pour le calcul des cédules (ie les données pour la période ou le défaut), ceci afin d'éviter le if/else quelques lignes plus bas. Il y a maintenant un type pour encapsuler les données du Path (dwell times, semgnets, etc), aussi bien l'utiliser et avoir une fonction qui retourne directement ce qu'on veut.
| units: TransitUnit[]; | ||
| outboundPath: TransitPath; | ||
| inboundPath?: TransitPath; | ||
| periodShortname: string; |
Collaborator
There was a problem hiding this comment.
Au lieu de passer periodShortname et serviceId, vous pourriez passer le PeriodSegmentData directement, outbound et inbound. Ces données pourraient peut-être remplacer totalement les paramètres ouboundPath/inboundPath, mais aussi outboundTotalTimeSeconds/inboundTotalTimeSeconds?
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts (4)
49-49:⚠️ Potential issue | 🟠 MajorPlease remove the
anyescapes in this hook.These spots still bypass type safety on core path/schedule/checkpoint data and can hide shape regressions.
As per coding guidelines, "Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application".Also applies to: 102-103, 140-141, 159-159, 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` at line 49, The hook uses `any` to escape type safety (e.g., `const line: any = path.getLine()` and other occurrences at lines ~102-103, 140-141, 159, 171); replace these `any` usages by importing and using the correct types (e.g., Line, Path, Schedule, Checkpoint or the specific interfaces exported by the core path/schedule module), type the return of `path.getLine()` as `Line | undefined` (or the actual type), add null/undefined guards or narrow with a type-guard/assertion where the code currently assumes presence, and update variables that currently use `any` to the precise types (or properly typed locals) so the compiler enforces shape correctness across useSegmentTimesByPeriod and the other noted variable sites.
382-390:⚠️ Potential issue | 🟡 MinorRemove all per-service checkpoint target keys when deleting a checkpoint.
Current deletion only removes the base key, leaving
${checkpointKey}_${serviceId}entries behind.Suggested fix
- const key = getCheckpointKey(resolved); + const keyPrefix = `${getCheckpointKey(resolved)}_`; setCheckpointTargets((prev) => { const next = { ...prev }; - delete next[key]; + for (const key of Object.keys(next)) { + if (key === getCheckpointKey(resolved) || key.startsWith(keyPrefix)) { + delete next[key]; + } + } return next; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` around lines 382 - 390, When removing a checkpoint in useSegmentTimesByPeriod, the code currently deletes only the base key returned by getCheckpointKey(resolved) (variable key) from setCheckpointTargets, leaving per-service keys like `${key}_${serviceId}` behind; update the setCheckpointTargets callback to iterate the keys of the copied map/object (next) and delete any entry whose key is exactly key or startsWith(`${key}_`) so all per-service checkpoint target keys are removed when you call setCheckpoints and setCheckpointTargets.
171-179:⚠️ Potential issue | 🟡 MinorLocalize the fallback period label and save error message.
These strings are still hardcoded in English, so they won’t follow active locale settings.
As per coding guidelines, "Use i18next and the
t()function for all user-facing strings to support internationalization".Also applies to: 479-479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` around lines 171 - 179, The period label fallback and the save error message are hardcoded; update useSegmentTimesByPeriod to use the i18next t() function: import/use t and replace the fallback template `${shortname} (${period.start_at_hour}h-${period.end_at_hour}h)` with t('period.fallbackLabel', { shortname, start: period.start_at_hour, end: period.end_at_hour }) (or similar key), and replace the save error string used in the save handler (the other occurrence referenced around line 479) with t('errors.saveFailed') (or your chosen i18n keys); ensure you pass the language/locale context to t if needed and keep using periodNamesByShortname and language as before.
274-280:⚠️ Potential issue | 🟠 MajorCheckpoint redistribution is still scoped to the selected service.
handleSaveloops all groups, but helper reads/comparisons still close overselectedServiceId. This can apply or skip redistributions for the wrong service.Also applies to: 311-313, 407-435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts` around lines 274 - 280, The helpers getCheckpointTargetKey and getCheckpointTarget close over selectedServiceId causing redistribution to be applied only to the selected service; change them to accept a serviceId parameter (e.g., getCheckpointTargetKey(checkpoint, serviceId) and getCheckpointTarget(checkpoint, periodShortname, serviceId)) and use that serviceId when building the key and when reading from checkpointTargets or falling back to getCheckpointCurrentTotal; update every call site in handleSave and the other locations mentioned (including usages near getCheckpointCurrentTotal and where checkpointTargets is indexed) to pass the current group's serviceId so redistributions are evaluated/applied per-group rather than always for selectedServiceId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts`:
- Line 49: The hook uses `any` to escape type safety (e.g., `const line: any =
path.getLine()` and other occurrences at lines ~102-103, 140-141, 159, 171);
replace these `any` usages by importing and using the correct types (e.g., Line,
Path, Schedule, Checkpoint or the specific interfaces exported by the core
path/schedule module), type the return of `path.getLine()` as `Line | undefined`
(or the actual type), add null/undefined guards or narrow with a
type-guard/assertion where the code currently assumes presence, and update
variables that currently use `any` to the precise types (or properly typed
locals) so the compiler enforces shape correctness across
useSegmentTimesByPeriod and the other noted variable sites.
- Around line 382-390: When removing a checkpoint in useSegmentTimesByPeriod,
the code currently deletes only the base key returned by
getCheckpointKey(resolved) (variable key) from setCheckpointTargets, leaving
per-service keys like `${key}_${serviceId}` behind; update the
setCheckpointTargets callback to iterate the keys of the copied map/object
(next) and delete any entry whose key is exactly key or startsWith(`${key}_`) so
all per-service checkpoint target keys are removed when you call setCheckpoints
and setCheckpointTargets.
- Around line 171-179: The period label fallback and the save error message are
hardcoded; update useSegmentTimesByPeriod to use the i18next t() function:
import/use t and replace the fallback template `${shortname}
(${period.start_at_hour}h-${period.end_at_hour}h)` with
t('period.fallbackLabel', { shortname, start: period.start_at_hour, end:
period.end_at_hour }) (or similar key), and replace the save error string used
in the save handler (the other occurrence referenced around line 479) with
t('errors.saveFailed') (or your chosen i18n keys); ensure you pass the
language/locale context to t if needed and keep using periodNamesByShortname and
language as before.
- Around line 274-280: The helpers getCheckpointTargetKey and
getCheckpointTarget close over selectedServiceId causing redistribution to be
applied only to the selected service; change them to accept a serviceId
parameter (e.g., getCheckpointTargetKey(checkpoint, serviceId) and
getCheckpointTarget(checkpoint, periodShortname, serviceId)) and use that
serviceId when building the key and when reading from checkpointTargets or
falling back to getCheckpointCurrentTotal; update every call site in handleSave
and the other locations mentioned (including usages near
getCheckpointCurrentTotal and where checkpointTargets is indexed) to pass the
current group's serviceId so redistributions are evaluated/applied per-group
rather than always for selectedServiceId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1600d8a-b7e5-44d1-9ec0-463cc20ee649
📒 Files selected for processing (2)
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsxpackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.tspackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx
packages/transition-frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
React components should use Redux for state management, i18next for translations, and SCSS for styling
Files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx
🧠 Learnings (11)
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/routers/*.tsx : Application routing should be defined in `TransitionRouter.tsx` using React Router with feature-based route organization
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.tspackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/**/*.tsx : React components should use Redux for state management, i18next for translations, and SCSS for styling
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.tspackages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.ts : Use TypeScript strictly and avoid `any` types - maintain type safety throughout the application
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📚 Learning: 2025-11-18T19:37:07.564Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1588
File: packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts:11-23
Timestamp: 2025-11-18T19:37:07.564Z
Learning: In the chairemobilite/transition repository TypeScript code, greenscientist prefers that reviews highlight when interfaces could be replaced with type aliases for stylistic consistency, especially when the structure is just a data shape.
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📚 Learning: 2026-02-24T00:36:50.541Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:50.541Z
Learning: In test files (*.test.ts, *.spec.ts), `any` types are acceptable for error handling and test scenarios, even though strict TypeScript typing is preferred in production code.
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📚 Learning: 2025-09-25T17:09:26.332Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/tasks/TransitionWorkerPool.ts:85-91
Timestamp: 2025-09-25T17:09:26.332Z
Learning: In packages/transition-backend/src/tasks/TransitionWorkerPool.ts, greenscientist deferred improving input file validation to check actual file existence on disk (using getFilePath('input') instead of just hasInputFile() metadata check), indicating it should be addressed when file access is refactored soon.
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to **/*.{ts,tsx} : Use i18next and the `t()` function for all user-facing strings to support internationalization
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📚 Learning: 2026-01-28T18:01:07.845Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1736
File: packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts:54-63
Timestamp: 2026-01-28T18:01:07.845Z
Learning: In packages/transition-backend/src/services/generalizedCostFunction/GeneralizedCostFunction.ts, the headway thresholds in getHeadwayThresholdSeconds() are intentionally hard-coded constants rather than configurable parameters. These values should be based on international consensus to allow consistent comparison of transit service quality across different cities. The TODO comments indicate that these values need more research and community discussion before being finalized.
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/useSegmentTimesByPeriod.ts
📚 Learning: 2026-04-08T18:54:34.951Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1782
File: packages/transition-frontend/src/components/map/TransitionMainMap.tsx:544-563
Timestamp: 2026-04-08T18:54:34.951Z
Learning: In `packages/transition-frontend/src/components/map/TransitionMainMap.tsx`, `handleLayerChange` intentionally uses an optimistic-update pattern: it updates `activeBasemapShortname` state and persists `map.basemapShortname` to preferences immediately, before the async basemap style fetch in `MapRenderer` completes. On fetch failure the map stays on the previous visible style (MapLibre does not crash). The preference records user intent, not rendering state, so transient network failures should not lose the user's selection. There is no `switchBasemap` API or awaitable promise to hook into — the style switch lives in a `useEffect` inside a functional component. Do not flag this pattern as needing rollback logic.
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx
📚 Learning: 2026-01-21T14:59:52.530Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
Applied to files:
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx
🔇 Additional comments (1)
packages/transition-frontend/src/components/forms/path/TransitPathSegmentTimesByPeriodModal/SegmentPeriodTimesTable.tsx (1)
90-100: Nice fix on callback memoization.This correctly refreshes handlers when
activeSegmentIndex,periods, orhandleCellChangechange, preventing stale writes.
thomasspina
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
2 times, most recently
from
d8fa65a to
23c65cc
Compare
iTzxMigz
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
from
23c65cc to
1e4ee02
Compare
thomasspina
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
2 times, most recently
from
4f5adb2 to
85b4680
Compare
iTzxMigz
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
from
85b4680 to
a555c97
Compare
tahini
reviewed
Apr 14, 2026
tahini
left a comment
tahini
left a comment
Collaborator
There was a problem hiding this comment.
Quelques autres commentaires. Les bugs ont été réglés, je vais en faire une branche expérimentale à partager aux partenaires.
| toNodeIndex: number; | ||
| }; | ||
|
|
||
| export type EditMode = 'segment' | 'checkpoint'; |
Collaborator
There was a problem hiding this comment.
Ceci est probablement un type plus frontend que common
| a.fromNodeIndex < b.toNodeIndex && b.fromNodeIndex < a.toNodeIndex; | ||
|
|
||
| /** Format seconds as "XmYYs", e.g. 75 → "1m15s", 120 → "2m00s", 45 → "0m45s" */ | ||
| export const formatSeconds = (seconds: number): string => { |
Collaborator
There was a problem hiding this comment.
Regardez si toXXhrYYminZZsec dans DateTimeUtils fait ce que vous voulez, ce serait préférable d'utiliser ce qui existe déjà plutôt qu'écrire une nouvelle fonction.
Contributor
Author
There was a problem hiding this comment.
@tahini j'ai regardé et on peut pas la use telle quelle pcq y'a les unités heure que nous on n'a pas dans notre format. je pourrais modifier cette fonction avec un paramètre optionnel pour dire si on veut les heures et là je pourrais là use mais telle quelle je peux pas y'a aussi un espacement entre les unités faudraient je vois au visuel ça ferait quoi dans le modal. Si vous voulez ça je peux le faire.
Collaborator
There was a problem hiding this comment.
L'idée c'est d'être cohérent. Pour le UI dans TranstiPathNodeButton les durées sont affichées avec le component DurationUnitFormatter, vous pourriez donc l'utiliser aussi à la place de cette fonction
| "RemoveCheckpoint": "Supprimer le point de contrôle", | ||
| "FromStation": "De", | ||
| "ToStation": "À", | ||
| "NoTripsGeneratedInfo": "Aucun voyage n'a été généré pour ce service. Pour voir et modifier les temps de segment par période, générez d'abord les voyages pour l'horaire dans la section des horaires de cette ligne.", |
Collaborator
There was a problem hiding this comment.
On n'utilise pas vraiment le terme voyage dans cette partie de Transition. Aussi, je crois que ce message apparaît seulement s'il n'y a pas de service? ou aussi s'il n'y a pas de voyage associés au service? C'est peut-être alors 2 messages différents: pas d'horaire pour le path, ou horaire, mais pas de trajets? Dans le premier cas, le texte devrait plutôt être "Aucun horaire n'a été généré pour ce trajet. Pour voir et modifier les temps de segments par période, générez d'abord des horaires dans la section "Horaires" de cette ligne."
Contributor
Author
There was a problem hiding this comment.
@tahini en anglais c'est tu still trip ou ça devrait être autre chose? J'ai ajouté le message pour aucun service aussi.
Collaborator
There was a problem hiding this comment.
En anglais, ce serait Schedule
iTzxMigz
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
2 times, most recently
from
c8aebe4 to
71e49f1
Compare
thomasspina
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
2 times, most recently
from
10d1372 to
ee574d2
Compare
Contributor
Author
|
@tahini j'ai add un pop up pour quand on arrive pour enregistrer avec le bouton vert (ça l'appelle generateTrip éventuellement qui est la même chose dans Schedule.ts) celui là ici le bouton: Donc, le bouton a une possibilité de fonctionner pour générer des trips c'est juste que y'a une erreur qui se faisait pas envoyer pour expliquer à l'utilisateur pourquoi ça marche pas et on pourrait croire que ça marche vu qu'on a pas d'erreur. Dans le fond, si le temps entre 2 véhicules (intervalle) ou le nombre de véhicule est pas défini dans le service, ça génère pas les trips. Si c'est défini ça les génère. Donc, mon pop up liste tous les services qui ont pas ça de setup lorsqu'on clique sur le bouton vert comme suit: |
thomasspina
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
from
eaeaed8 to
0fa84fe
Compare
thomasspina
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
from
0fa84fe to
03b28a0
Compare
tahini
reviewed
Apr 15, 2026
| if (this._map && this.layerIsEnabled(layerName)) { | ||
| (this._map.getSource(layerName) as GeoJSONSource).setData(this._layersByName[layerName].source.data); | ||
| const source = this._map.getSource(layerName) as GeoJSONSource | undefined; | ||
| source?.setData(this._layersByName[layerName].source.data); |
Collaborator
There was a problem hiding this comment.
Ce changement ne semble pas relié à votre feature? Est-ce un bug que vous avez rencontré? Si oui, expliquer et fixer dans une PR à part.
Contributor
There was a problem hiding this comment.
oui. Je pensais qu'on allait tout mettre dans ce PR mais je l'ai split. #1902
Collaborator
Oui, super, excellente idée. Présentement l'utilisteur ne sait juste pas que l'horaire n'a pas vraiment été regénéré. Ceci par contre peut être une PR toute seule, ça fixerait l'issue #852 |
Collaborator
|
Nous avons partagé une branche expérimentale avec les partenaires. On va attendre d'avoir de leurs nouvelles pour un review plus complet. En attendant, s'il vous est possible de diviser cette branche en plus petites PRs distinctes, plus facile à reviewer et régler, un peu comme #1899, ça permet d'enlever du bruit à la grosse branche. S'il y a du refactoring du code existant qui a été fait, des nouveaux types/concepts qu'il vaudrait la peine de discuter séparément, faites-en une PR. Vous pouvez garder cette branche pour avoir tous les features et faire tester la fonctionnalité, mais pour les reviews, ce sera plus simple avec des branches plus petites. |
Extract Period, PeriodsGroup types and findPeriodShortname() from backend-only files into transition-common so frontend can share them.
- Add calculateSegmentTimesForCheckpoint in PathGeographyUtils to route a subset of nodes through OSRM and compute physics-based segment times - Add scaleTimesToTarget to scale OSRM times to match checkpoint target - Extract helper functions: getFeatureIndexRangeForNodes, extractLegsFromResults, computeSegmentTimesFromLegs - Update handleDistribute to use OSRM times, skip unchanged periods - Auto-distribute undistributed checkpoints on save for all service groups - Index checkpoint targets by service group to avoid cross-group interference - Export calculateSegmentDuration from PathGeographyGenerator for reuse
Add remapPeriodSegmentData to preserve period/service-specific travel times when nodes are inserted/removed or waypoints changed. Each period computes its own congestion ratio from preserved segments and applies it to new segments. Tests cover all edit types, aggregate fields, speeds, distances, dwell time unbaking, and the ratio=1.0 fallback.
- Segment view: show stop time (editable), departure, segment time, arrival - First segment has no stop time (layover separate) - Checkpoint view: add total stop time and total with stops columns - Target total and total with stops are both editable and linked - Stop time changes update node default_dwell_time_seconds and save to DB on path save - Extract ScrollableDropdown component for seconds picker (0-59) - Remove snapSeconds utility and 5-second increment constraint
…, and checkpointsOverlap
…ent time calculations Add safety checks for edge cases in OSRM duration calculations, period segment averaging, and GTFS data expansion. Fix missing test fixture fields.
…r events
After GTFS import, map update events fire before MapLibre sources are added and before collections are registered in the collection manager. This causes TypeErrors when getSource() returns undefined in MapLayerManager and when collectionManager.get('nodes') is undefined during mouse hover. Add null checks with optional chaining and early returns to handle these timing gaps gracefully.
expandGroupedDataToServices was doing a full replace of each non-edited service with the representative's edits, which injected periods (e.g. EV) onto services that were merged in via mergeServicesWithSubsetPeriods and never had those periods originally. Propagate edits only for periods the target service already had, and add tests covering the leak case as well as confirming that serviceIds[0] is always the service with the most periods regardless of input order.
…recalculation The "Recalculate Route" button and the "Recalculate all segment travel times" toggle both cause path.data.segmentsByServiceAndPeriod to be wiped on the next geography update. Add a confirmation modal on the button (shown only when there is per-period data to lose) and a persistent warning below the toggle while it is enabled, so the user knows their custom segment times will be erased before committing to the action.
…ripCount, and sync base path data on segment times modal save
When a path is edited and the user chooses to update affected schedules, Schedule.updateForAllPeriods used to swallow the errors returned by generateForPeriodFunction, leaving the user with no feedback that some periods (e.g. those without number of vehicles or interval set) did not actually get any trips generated. Propagate per-service regeneration counts up through Line.updateSchedules and surface a warning modal listing every service whose schedule could not be regenerated at all. The modal points the user to the timetable section of the line to set the missing configuration.
Move `findDistinguishingToken`, `collectServiceNames`, `computeGroupCommonName`, and `assignCommonNamesForDuplicates` from `PathServiceGrouping.ts` in transition-common to a new `PathServiceGroupingLabels.ts` in transition-frontend. These functions produce UI display names for service groups and have no backend consumers. The domain grouping logic stays in common.
thomasspina
force-pushed
the
schedule-segment-times-per-period-and-checkpoints
branch
from
5079011 to
f3d8795
Compare
This was referenced Apr 15, 2026
Collaborator
|
Premier feedback des utilisateurs: Porter attention au dernier commentaire sur les stats. Mais je crois que ça s'applique à la première fonctionnalité que vous avez faite, soit sur la modification d'un trajet importé de GTFS. |
Contributor
Author
Je suis allé tester. En effet ça l'arrive j'ai un fix que je peux faire mais ça règle pas tout. Vu qu'on se base sur OSRM pour le no dwell time et que le withDwellTime se base sur GTFS, c'est des choses qui peuvent arriver. Notamment, en testant sur la ligne Ma solution pour fixer le plus possible c'est au lieu de prendre mon Je suis allé vérifier en allant dans un commit avant mes changements voir si le temps était le même et c'est le même temps. |
Collaborator
Ça semble un peu arbitraire comme approche, non? La gestion du dwell time serait probablement à revoir, il y a beaucoup de code relié à ça et si ça pose problème c'est pas super. On ne devrait pas se baser sur OSRM pour certaines choses et GTFS pour d'autres pour un même segment. C'est soit un, soit l'autre. Et s'il n'y avait pas de dwell time à l'origine, pas besoin d'en ajouter un après mise à jour. Mais pour un nouveau noeud dans le path, là oui, il faut ajouter le dwell time |
Collaborator
|
Au fait, je suis tombée sur l'issue #802, alors le problème mentionné ne vient peut-être pas de vous, il semble qu'il y avait déjà des problèmes avec ces temps, mais bon, si jamais vous le fixez, tant mieux! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1755
Closes #1756
Closes #1757
Add support for per-period, per-service segment travel times on paths, along with a checkpoint
system that maps to GTFS timepoints.
Segment times per period and service
PeriodSegmentDatatype on Path to store segment travel times broken down by period andservice
Checkpoints
Segment times modal
similarity
Preview of modal
Here is a preview of what the modal looks like:
Summary by CodeRabbit