Modify direction filter to handle multiple IDs#253
Conversation
📝 WalkthroughWalkthroughAdds multi-direction support: a new public config array Changes
Sequence DiagramsequenceDiagram
autonumber
participant Config as Configuration
participant Fetcher as DepartureFetcher
participant HAFAS as HAFAS Client
participant Agg as Aggregator
Config->>Fetcher: Load config (directions or direction)
activate Fetcher
Note over Fetcher: Normalize to directions array (use directions if present,\nelse wrap deprecated direction)
loop For each direction in directions (or [null])
Fetcher->>HAFAS: departures(options + direction)
activate HAFAS
HAFAS-->>Fetcher: departures response / error
deactivate HAFAS
Fetcher->>Agg: send successful results
end
activate Agg
Agg->>Agg: Flatten results
Agg->>Agg: Sort by departure time
Agg->>Agg: Deduplicate (when, stop.id, line.id)
Agg-->>Fetcher: aggregated departures
deactivate Agg
Fetcher->>Fetcher: Apply filters (transport types, ignored lines, reachability, surplus)
Fetcher->>Fetcher: Slice to maxElements
deactivate Fetcher
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🧹 Nitpick comments (2)
README.md (1)
125-125: Use consistent type terminology.The type is documented as
list, but throughout the README other array options consistently usearrayas the type (e.g.,ignoredLines,excludedTransportationTypes,excludeDirections). Usearrayfor consistency.🔎 Suggested fix
-| `directions` | <p>IDs of a stations. It is used to display only those departures heading to these stations.</p><p>**Type:** `list`<br>**Example:** `["954609"]`<br>**Default value:** `[]`</p><p>**Note:** It is not necessary to find the ID of the end station. Just use the next station which is on the route you are after.</p><p>**Note!** This option doesn't work anymore using the default `db` profile. Please try `dbweb` (limited to query only one hour in the future) or another regional profile. Check out the issue [#193](https://github.com/KristjanESPERANTO/MMM-PublicTransportHafas/issues/193#issuecomment-2599743884) for more details.</p>| +| `directions` | <p>IDs of a stations. It is used to display only those departures heading to these stations.</p><p>**Type:** `array`<br>**Example:** `["954609"]`<br>**Default value:** `[]`</p><p>**Note:** It is not necessary to find the ID of the end station. Just use the next station which is on the route you are after.</p><p>**Note!** This option doesn't work anymore using the default `db` profile. Please try `dbweb` (limited to query only one hour in the future) or another regional profile. Check out the issue [#193](https://github.com/KristjanESPERANTO/MMM-PublicTransportHafas/issues/193#issuecomment-2599743884) for more details.</p>|core/DepartureFetcher.mjs (1)
84-106: Consider adding error handling for individual direction fetches.The loop fetches departures for each direction sequentially but doesn't handle errors at the per-direction level. If a single direction fetch fails, the entire operation fails and no departures are returned, even if other directions could have succeeded.
Consider wrapping each fetch in a try-catch block to handle individual failures gracefully, logging the error and continuing with other directions.
🔎 Proposed enhancement
for (const direction of directions) { const options = { duration: this.getTimeInFuture(), when: this.getDepartureTime() }; if (direction) { options.direction = direction; } - const result = await this.hafasClient.departures( - this.config.stationID, - options - ); - - allDepartures.push(...result.departures); + try { + const result = await this.hafasClient.departures( + this.config.stationID, + options + ); + + allDepartures.push(...result.departures); + } catch (error) { + Log.error(`[MMM-PublicTransportHafas] Failed to fetch departures for direction ${direction}:`, error); + // Continue with other directions + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MMM-PublicTransportHafas.jsREADME.mdcore/DepartureFetcher.mjs
🧰 Additional context used
🧬 Code graph analysis (2)
core/DepartureFetcher.mjs (1)
MMM-PublicTransportHafas.js (1)
directions(86-86)
MMM-PublicTransportHafas.js (1)
core/DepartureFetcher.mjs (1)
directions(84-86)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
README.md (1)
170-170: LGTM!The example configuration correctly demonstrates the new
directionsarray syntax with an empty array as the default.MMM-PublicTransportHafas.js (3)
33-34: LGTM!The defaults correctly define both the deprecated
direction(string) and the newdirections(array), enabling backward compatibility while guiding users toward the new option.
85-91: LGTM!The backward compatibility logic is well-implemented. It prioritizes the new
directionsarray when provided, and gracefully falls back to wrapping the legacydirectionstring in an array when needed. This ensures existing configurations continue to work while supporting the new multi-direction feature.
99-99: LGTM!Correctly passes the
directionsarray to the fetcher, aligning with the new multi-direction support implemented incore/DepartureFetcher.mjs.core/DepartureFetcher.mjs (3)
28-28: LGTM!JSDoc correctly updated to reflect the new
directionsarray parameter.
108-117: LGTM!The deduplication logic is well-implemented. Sorting by departure time first ensures chronological order, and the composite key (
when,stop.id,line.id) effectively identifies duplicate departures across multiple direction queries.
120-134: LGTM!The
maxElementscalculation has been correctly moved to after aggregation and deduplication, ensuring the limit applies to the final combined result set. The subsequent filter chain maintains the correct order of operations.
| | `updatesEvery` | <p>The time in seconds when the displayed departures should be updated.</p><p>**Type:** `integer`<br>**Example:** `60` (The departures will be refreshed every minute.)<br>**Default value:** `120`<br>**Unit:** `seconds`</p><p>**Note:** The minimal refresh time is 30 seconds.</p>| | ||
| | `discardSocketErrorThreshold` | <p>How many consecutive socket errors should be tolerated before showing an error message.</p><p>**Type:** `integer`<br>**Example:** `5` (Tolerate 5 consecutive errors before showing error message.)<br>**Default value:** `3`</p><p>**Note:** When temporary network issues occur, the module will keep displaying the last successfully fetched departures instead of showing an error immediately. The number of socket issues is displayed in the "Last update" line (e.g., "Update (socket issues: 2)"). Only after exceeding this threshold will an error message be displayed. Set to `0` to show errors immediately. Errors below the threshold are logged as warnings, errors above as errors.</p>| | ||
| | `direction` | <p>An ID of a station. It is used to display only those departures heading to this station.</p><p>**Type:** `string`<br>**Example:** `"954609"`<br>**Default value:** `""`</p><p>**Note:** It is not necessary to find the ID of the end station. Just use the next station which is on the route you are after.<br>It is not possible to list multiple IDs. If you want to display different directions for one station use multiple instances of this module.</p><p>**Note!** This option doesn't work anymore using the default `db` profile. Please try `dbweb` (limited to query only one hour in the future) or another regional profile. Check out the issue [#193](https://github.com/KristjanESPERANTO/MMM-PublicTransportHafas/issues/193#issuecomment-2599743884) for more details.</p>| | ||
| | `direction` | <p>**Deprecated, use `directions` instead**<br>An ID of a station. It is used to display only those departures heading to this station.</p><p>**Type:** `string`<br>**Example:** `"954609"`<br>**Default value:** `""`</p><p>**Note:** It is not necessary to find the ID of the end station. Just use the next station which is on the route you are after.<br>It is not possible to list multiple IDs. If you want to display different directions for one station use multiple instances of this module.</p><p>**Note!** This option doesn't work anymore using the default `db` profile. Please try `dbweb` (limited to query only one hour in the future) or another regional profile. Check out the issue [#193](https://github.com/KristjanESPERANTO/MMM-PublicTransportHafas/issues/193#issuecomment-2599743884) for more details.</p>| |
There was a problem hiding this comment.
Update the deprecation note to remove outdated information.
The deprecation note still states "It is not possible to list multiple IDs. If you want to display different directions for one station use multiple instances of this module." This is now outdated since the new directions option explicitly supports multiple IDs. This sentence should be removed from the deprecation notice to avoid confusion.
🔎 Suggested update
-| `direction` | <p>**Deprecated, use `directions` instead**<br>An ID of a station. It is used to display only those departures heading to this station.</p><p>**Type:** `string`<br>**Example:** `"954609"`<br>**Default value:** `""`</p><p>**Note:** It is not necessary to find the ID of the end station. Just use the next station which is on the route you are after.<br>It is not possible to list multiple IDs. If you want to display different directions for one station use multiple instances of this module.</p><p>**Note!** This option doesn't work anymore using the default `db` profile. Please try `dbweb` (limited to query only one hour in the future) or another regional profile. Check out the issue [#193](https://github.com/KristjanESPERANTO/MMM-PublicTransportHafas/issues/193#issuecomment-2599743884) for more details.</p>|
+| `direction` | <p>**Deprecated, use `directions` instead**<br>An ID of a station. It is used to display only those departures heading to this station.</p><p>**Type:** `string`<br>**Example:** `"954609"`<br>**Default value:** `""`</p><p>**Note:** It is not necessary to find the ID of the end station. Just use the next station which is on the route you are after.</p><p>**Note!** This option doesn't work anymore using the default `db` profile. Please try `dbweb` (limited to query only one hour in the future) or another regional profile. Check out the issue [#193](https://github.com/KristjanESPERANTO/MMM-PublicTransportHafas/issues/193#issuecomment-2599743884) for more details.</p>|🤖 Prompt for AI Agents
In README.md around line 124, the deprecation note for `direction` contains an
outdated sentence claiming multiple IDs are not supported; remove the sentence
"It is not possible to list multiple IDs. If you want to display different
directions for one station use multiple instances of this module." from the
deprecation paragraph and, if needed, adjust surrounding text to reference the
new `directions` option that supports multiple IDs (keeping the deprecation and
profile note intact).
|
Hey @drtorchwood! Thanks for the PR – the feature is useful and the backward compatibility is nicely done! 🎉 I think there is an issue: the Can you refactor this to use async fetchDepartures () {
const directions = Array.isArray(this.config.directions) && this.config.directions.length > 0
? this.config.directions
: [null];
const promises = directions.map((direction) => {
const options = {
duration: this.getTimeInFuture(),
when: this.getDepartureTime()
};
if (direction) options.direction = direction;
return this.hafasClient.departures(this.config.stationID, options);
});
const results = await Promise.all(promises);
const allDepartures = results.flatMap(r => r.departures);
allDepartures.sort((item1, item2) => new Date(item1.when) - new Date(item2.when));
// ... rest of your code
}This also makes the code ready for better error handling later. Do you want to give it a try? 🙂 |
Will try to do it in the next few days |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @core/DepartureFetcher.mjs:
- Around line 117-125: The dedupe key for filteredDepartures currently uses
dep.when + dep.stop.id + dep.line?.id which collapses different departures when
line is null; update the key generation inside the filteredDepartures filter
(the seen Set logic) to include additional distinguishing fields when line is
missing — e.g., use dep.line?.id ?? `line:null:${dep.direction ??
""}:${dep.platform ?? ""}` or build a JSON.stringify key of {when: dep.when,
stop: dep.stop.id, lineId: dep.line?.id, direction: dep.direction, platform:
dep.platform} so departures without line info are not incorrectly deduplicated;
keep the symbol names seen, filteredDepartures, dep.when, dep.stop.id,
dep.line?.id, dep.direction and dep.platform to locate the change.
🧹 Nitpick comments (1)
core/DepartureFetcher.mjs (1)
83-85: Optional: Deduplicate input directions array.If the user provides duplicate station IDs in
config.directions, the code will make redundant requests. While the results are deduplicated later, avoiding duplicate fetches would improve efficiency.♻️ Proposed optimization
const directions = Array.isArray(this.config.directions) && this.config.directions.length > 0 - ? this.config.directions + ? [...new Set(this.config.directions)] : [null];
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/DepartureFetcher.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
core/DepartureFetcher.mjs (2)
MMM-PublicTransportHafas.js (1)
directions(86-86)node_helper.js (1)
Log(1-1)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
core/DepartureFetcher.mjs (9)
28-28: LGTM! Documentation updated correctly.The JSDoc accurately reflects the new
directionsarray parameter.
83-85: LGTM! Backward compatibility preserved.The fallback to
[null]ensures that existing configurations withoutdirectionscontinue to work by performing a single undirected request.
87-90: LGTM! Clean setup of shared options.Extracting common options into
optionsBaseimproves readability and reduces duplication.
92-98: LGTM! Request mapping is correct.The conditional spread operator cleanly handles both directed and undirected requests.
99-99: Excellent choice usingPromise.allSettledoverPromise.all!This approach is more resilient than the reviewer's
Promise.allsuggestion: if one direction fails, the other directions still return results. This ensures users see partial data rather than no data when a single direction fetch fails.
103-113: LGTM! Robust error handling.The error logging defensively extracts messages with appropriate fallbacks, and successful departures are correctly aggregated.
115-115: LGTM! Sorting ensures chronological order.The departures are correctly sorted by time for proper display.
128-130: LGTM! Correct capacity calculation.The
maxElementscorrectly represents the total display capacity (reachable + unreachable departures).
132-142: LGTM! Filter pipeline correctly applied to aggregated results.The existing filters operate correctly on the merged departure list, and the final slice enforces the capacity limit.
| const seen = new Set(); | ||
| let filteredDepartures = allDepartures.filter((dep) => { | ||
| const id = `${dep.when}-${dep.stop.id}-${dep.line?.id}`; | ||
| if (seen.has(id)) { | ||
| return false; | ||
| } | ||
| seen.add(id); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
Consider strengthening the deduplication key.
The composite key uses dep.line?.id, which becomes the string "undefined" when line is null. This could incorrectly deduplicate distinct departures that lack line information but occur at the same time and stop.
Consider either:
- Filtering out or handling departures without line information separately, or
- Including additional distinguishing fields (e.g.,
direction,platform) in the key.
🔍 Example of potential false positive
// Two different departures, both missing line info:
// Departure A: when="2026-01-07T10:00", stop.id="123", line=null
// Departure B: when="2026-01-07T10:00", stop.id="123", line=null
// Both produce key "2026-01-07T10:00-123-undefined" → one is incorrectly removed🤖 Prompt for AI Agents
In @core/DepartureFetcher.mjs around lines 117 - 125, The dedupe key for
filteredDepartures currently uses dep.when + dep.stop.id + dep.line?.id which
collapses different departures when line is null; update the key generation
inside the filteredDepartures filter (the seen Set logic) to include additional
distinguishing fields when line is missing — e.g., use dep.line?.id ??
`line:null:${dep.direction ?? ""}:${dep.platform ?? ""}` or build a
JSON.stringify key of {when: dep.when, stop: dep.stop.id, lineId: dep.line?.id,
direction: dep.direction, platform: dep.platform} so departures without line
info are not incorrectly deduplicated; keep the symbol names seen,
filteredDepartures, dep.when, dep.stop.id, dep.line?.id, dep.direction and
dep.platform to locate the change.
|
Tested it by giving one non-existing direction. The others were displayed correctly. |
KristjanESPERANTO
merged commit 37e01cf
into
KristjanESPERANTO:develop
Replace dayjs with temporal-kit v0.2.1 for modern date/time handling using the TC39 Temporal proposal. Add TemporalHelper.mjs with formatting utilities. Breaking changes: - Config: `displayLastUpdateFormat` (string) → `displayLastUpdateOptions` (object) - Config: New `language` option (defaults to MagicMirror's config.language) Improvements: - Modern ES syntax: nullish coalescing (??), simplified conditionals - Better parameter naming (departure → when for clarity) - Convert Temporal to Date for hafas-client compatibility - All 55 tests updated and passing Closes #253
|
Thanks again! Your change is now included in today's new release 🙂 Please check it out. |
2 participants
The current version only supports one station ID for the direction filter. This version offers the possibility to give a list of IDs via the new option
directions. The old optiondirection(a string) can still be used. Internally, each item of the list triggers a separate hafas fetch. The results are then merged.Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.