Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The chart flow does not yet separate mixed Pegel/mNN selections, and some excluded labels are misclassified.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds station-label classification and formatting helpers intended to keep Pegel and mNN stations in separate comparison charts.
Changes:
- Classifies and formats water-level station labels.
- Adds grouping helpers, tests, and public exports.
- Adds two patch changesets.
File summaries
| File | Summary |
|---|---|
packages/count-aggregator-ui/src/lib/stations.ts |
Adds classification and grouping helpers. The chart flow does not use the grouping helper (moderate, 2 votes), and exclusion predicates run after mNN classification (moderate, 2 votes). |
packages/count-aggregator-ui/src/lib/stations.test.ts |
Adds classification and grouping tests. |
packages/count-aggregator-ui/src/index.ts |
Exports the helpers, but the production chart path does not consume them, leaving mixed selections combined (moderate, 1 vote). |
.changeset/water-level-pegel-mnn.md |
Adds a patch release note. |
.changeset/water-level-pegel-mnn-swap.md |
Adds a patch release note. |
Review details
Suppressed comments (1)
packages/count-aggregator-ui/src/index.ts:74
- Within this package these helpers are only exported; no production chart path calls them.
ResultStepandOverviewChartPanelstill pass the complete station ID list to oneTimeSeriesChart, so a mixed Pegel/mNN selection continues to render one combined chart rather than separate comparison charts. Apply the grouping in the wizard/overview chart data-fetch and render flow, or adjust the PR scope and description.
stationIdsForWaterLevelGroup,
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ( | ||
| /\bmnn\b/.test(normalized) || | ||
| /\bm\.?\s*nn\b/.test(normalized) || | ||
| normalized.includes("wasserstand mnn") | ||
| ) { | ||
| return "mnn"; | ||
| } | ||
|
|
||
| if ( | ||
| /\b(ow|uw)\b/.test(normalized) || | ||
| normalized.includes("oberwasser") || | ||
| normalized.includes("unterwasser") || | ||
| normalized.includes("geländeoberkante") || | ||
| normalized.includes("tagesmittel") || | ||
| /[⌀∅ø]\s*\/\s*d/.test(normalized) | ||
| ) { | ||
| return null; | ||
| } |
| export function stationIdsForWaterLevelGroup( | ||
| stationsById: ReadonlyMap<number, Station>, | ||
| group: WaterLevelComparisonGroup, | ||
| ): number[] { | ||
| const ids: number[] = []; |
|
Copilot’s note that the package wizard still draws one combined chart is expected for this extraction.
|
75f6635 to
d04c5e0
Compare
pjeweb
changed the title
|
Withdrawn.
City-specific Pegel / mNN label strings stay in the plan app. They should not land in
@mapsight/count-aggregator-ui. If this grouping is shared later, it belongs on the platform, not hardcoded in the OSS UI package.