feat: unify transfer risk rule and fix tight-transfer display - #784
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
reidzeibel
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Transfer-risk severity handling appears incomplete/inconsistent with the PR description (missing/undifferentiated “unlikely” vs “uncertain” UI states), which can lead to incorrect user-facing status messaging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns planner-web’s transfer-risk classification and UI with the shared rule in @atb-as/utils, and fixes tight-transfer rendering issues (wait-row visibility and arrival/departure minute rounding) so card and details views agree.
Changes:
- Replace local overlap/guaranteed-interchange logic with transfer-risk stamping/aggregation from
@atb-as/utils, and separate “freshness” (TripPatternStatus) from “transfer risk”. - Add arrival rounding logic (
arrivalRoundingMethod) plusnextDisplayedDepartureto avoid same-minute transfers displaying as “departure before arrival”. - Update assistant UI/translations and details wait section to surface “Usikker overgang” and show any wait time (including very short waits).
File summaries
| File | Description |
|---|---|
| src/utils/date.ts | Adds arrivalRoundingMethod for same-minute transfer display correctness. |
| src/utils/tests/arrival-rounding-method.test.ts | Tests for the new arrival rounding decision logic. |
| src/translations/pages/assistant.ts | Adds new strings for transfer uncertainty and wait messaging. |
| src/page-modules/assistant/types.ts | Introduces transferRisk on legs/trips and narrows TripPatternStatus to freshness-only. |
| src/page-modules/assistant/trip/utils.ts | Adds nextDisplayedDeparture; updates travel-card status logic to use transfer risk. |
| src/page-modules/assistant/trip/trip-pattern-collapse/index.tsx | Wires nextDisplayedDeparture into trip details rendering. |
| src/page-modules/assistant/trip/travel-card/tests/travel-card.test.tsx | Updates status summary test expectations for uncertain transfers. |
| src/page-modules/assistant/trip/tests/next-displayed-departure.test.ts | Tests for nextDisplayedDeparture and trip-level rounding behavior. |
| src/page-modules/assistant/server/journey-planner/refresh-trip-utils.ts | Removes local overlap rule; re-exports and integrates transfer-risk utilities. |
| src/page-modules/assistant/server/journey-planner/index.ts | Applies withTransferRisk and exposes trip-level transferRisk. |
| src/page-modules/assistant/details/trip-section/wait-section.tsx | Shows wait/transfer messages in details view; introduces transfer-risk message row. |
| src/page-modules/assistant/details/trip-section/trip-section.module.css | Styles for the new wait/transfer message row layout and emphasis coloring. |
| src/page-modules/assistant/details/trip-section/index.tsx | Uses arrivalRoundingMethod when rendering arrival times. |
| src/page-modules/assistant/details-body/index.tsx | Supplies nextDisplayedDeparture to leg sections in details body. |
| src/page-modules/assistant/tests/refresh-trip-utils.test.ts | Updates refresh tests to validate transfer-risk stamping/aggregation. |
| package.json | Bumps @atb-as/utils dependency version. |
| pnpm-lock.yaml | Lockfile update for @atb-as/utils version bump (and related metadata). |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 16/17 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gorandalum
left a comment
gorandalum
left a comment
There was a problem hiding this comment.
Is the description correct? It mentions "Two state instead of one", but the code changes doesn't really seem like a new state is added. The table also says "Ikke mulig" for multiple situations, but the text "Ikke mulig" is no longer found in the code.
|
@gorandalum Oopsie, it was removed sometime after this PR is created, I'll take another look and fix it! |
There was a problem hiding this comment.
🔵 Needs a closer look
The details-screen “Uncertain transfer” message is currently styled as an error, conflicting with the intended info-level severity and the rest of the UI’s transfer-risk presentation.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
src/page-modules/assistant/details/trip-section/wait-section.tsx:68
getTransferMessagestyles the "Uncertain transfer" message withemphasis: 'error'(and an "Unknown" status icon), but this transfer risk is intended to be an info-level warning in this PR (and the travel-card status chip maps it tostatusType: 'info'). This currently renders the details-screen warning with error semantics/colors inconsistent with the rest of the UI.
function getTransferMessage(
transferRisk: TransferRisk | undefined,
t: TranslateFunction,
): WaitMessage | undefined {
if (!transferRisk) return undefined;
const texts = PageText.Assistant.details.tripSection.wait.transfer.uncertain;
return {
icon: 'status/Unknown',
emphasis: 'error',
title: t(texts.label),
message: t(texts.message),
};
- Files reviewed: 17/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
Part of https://github.com/AtB-AS/kundevendt/issues/24416
Background
The same transfer situation was classified by two different rules in planner web and app, they could disagree about the same trip.
The rule now lives in
@atb-as/utils(8.0.1), imported by planner-web (this PR), atb-bff (AtB-AS/atb-bff#478) and mittatb-app (AtB-AS/mittatb-app#6309).What changes
hasTemporalOverlap/hasGuaranteedInterchangeIntoand usesgetLegTransferRiskfrom the package instead. The guaranteed-interchange handling from fix: respect guaranteed interchanges when determining trip status #781 is preservedTripPatternStatusnarrows to'valid' | 'stale'(data freshness only) and risk is reported per leg astransferRisk, plus the worst value at trip level.impossible/ "Ikke mulig" is gone. Any transfer where the next departure is before the previous arrival isuncertain("Usikker overgang", info), however large the gap. The card now shows the same verdict as the details screen.How will it look like
maximumWaitTimemaximumWaitTimestartedandendedoutranksstale, so a started trip will not show "Resultatene kan være utdaterte" message.Tight transfers
The trip details had no transfer-risk UI at all. It now shows "Usikker overgang" on the leg, styled like the app.
Two display fixes came out of testing a real 23-second transfer, which showed as
15:18 → 15:17with no message at all:Rounding
main15:18 → 15:1715:17 → 15:1715:18 → 15:1715:17 → 15:1715:18 → 15:1815:18 → 15:2015:19 → 15:17Kort byttetid message
mainNote on a zero-second transfer
A 0 s transfer now shows neither a warning nor a wait row, since the wait section only renders when the gap is above zero. It will be followed-up later
Other changes
Two existing strings changed to match the app:
wait.shortTime(nn)wait.shortTime(en)