fix: resolve SonarCloud critical issues and bugs#1509
fix: resolve SonarCloud critical issues and bugs#1509eran132 wants to merge 1 commit intohasadna:mainfrom
Conversation
- Refactor LocationObservable to use static factory method instead of async operation in constructor (critical: S6637) - Fix conditional value leak in BusToolTip JSX by using != null check instead of truthy check on route.id (bug: could render 0 in DOM) - Fix dead code in WorstLinesChart where template literal was always truthy, making the || 'Unknown' fallback unreachable (bug: S3563) - Skipped TimelinePage.ts false positive (Playwright fixture use() incorrectly flagged as React Hook) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Addresses SonarCloud-reported issues by fixing two React rendering edge cases and refactoring the vehicle-locations observable to avoid starting async work inside a constructor.
Changes:
- Fix unreachable fallback logic in
WorstLinesChartby only interpolating ids when required fields are present. - Prevent conditional value leakage in
BusToolTipwhenroute.idis0by checking for null/undefined explicitly. - Refactor
LocationObservableto use a static factory (create) rather than performing async work in the constructor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/pages/dashboard/WorstLinesChart/WorstLinesChart.tsx | Makes the id derivation conditional so the 'Unknown' fallback is reachable. |
| src/pages/components/map-related/MapLayers/BusToolTip.tsx | Fixes conditional JSX rendering to avoid rendering 0 into the DOM. |
| src/hooks/useVehicleLocations.ts | Introduces a factory method to trigger async loading outside the constructor and updates cache initialization accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ({ | ||
| id: `${item.lineRef}|${item.operatorRef?.operatorRef}` || 'Unknown', | ||
| id: | ||
| item.lineRef && item.operatorRef?.operatorRef |
There was a problem hiding this comment.
The new guard uses truthiness (item.lineRef && item.operatorRef?.operatorRef), which will treat valid numeric values like 0 as missing and return 'Unknown'. If these refs can be 0, switch to an explicit null/undefined check (e.g. != null) so 0 remains a valid id component.
| item.lineRef && item.operatorRef?.operatorRef | |
| item.lineRef != null && item.operatorRef?.operatorRef != null |
| static create(query: VehicleLocationQuery) { | ||
| const instance = new LocationObservable() | ||
| void instance.#loadData(query) | ||
| return instance | ||
| } |
There was a problem hiding this comment.
LocationObservable now relies on the create() factory to start loading, but the default constructor is still publicly usable (within this module) and would create an instance that never loads data. Consider making the constructor private/protected (and optionally adding an explicit return type to create) to enforce correct initialization via the factory.
Summary
Fixes critical issues and bugs identified by SonarCloud static analysis (see dashboard):
LocationObservableto use a static factory method instead of launching async operations in the constructorBusToolTipJSX —route?.id && (...)could render0in the DOM whenidis0. Changed toroute?.id != nullWorstLinesChart— template literal`${x}|${y}` || 'Unknown'is always truthy, so the'Unknown'fallback was unreachable. Now checks values before interpolationSkipped
TimelinePage.tsfalse positive — Playwright's fixtureuse()was incorrectly flagged as a React Hook.Test plan
🤖 Generated with Claude Code