Add data source selection to population calculation#1767
Add data source selection to population calculation#1767GabrielBruno24 wants to merge 1 commit intochairemobilite:mainfrom
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 population data-source support across the accessibility-map feature. Requests gain Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx (1)
86-86:⚠️ Potential issue | 🔴 Critical
useStateinside.map()violates the Rules of Hooks.
useState(Line 86) is called inside thefeatures.map()callback. React hooks must only be called at the top level of a component — never inside loops, conditions, or nested functions. This will cause runtime errors or unpredictable behavior when the number of features changes.Extract each feature's rendering into its own child component so the hook call is at that component's top level.
Proposed approach
// Extract a child component: const FeatureStatItem: React.FC<{ feature: GeoJSON.Feature; calculatePopulation?: boolean }> = ({ feature, calculatePopulation }) => { const { t, i18n } = useTranslation(['transit', 'main']); const [showEmptyDetailedCategories, setShowEmptyDetailedCategories] = useState(false); // ... rest of the per-feature rendering logic }; // Then in the parent: const featureStats = features.map((feature) => ( <FeatureStatItem key={feature.properties?.durationMinutes} feature={feature} calculatePopulation={props.calculatePopulation} /> ));
🤖 Fix all issues with AI agents
In `@docs/APIv1/accessibilityMap.yml`:
- Around line 252-266: Update the schema in accessibilityMap.yml to include
populationData in the required array (alongside durationSeconds and areaSqM) so
the API contract matches the implementation that always emits populationData
(defaulting to { population: null, dataSourceAreaRatio: 0 }); locate the object
that defines populationData and the surrounding required list and add the key
"populationData" to that required list.
- Around line 263-266: The description for the dataSourceAreaRatio field
contains a typo ("polyon"); update the description text in the
dataSourceAreaRatio YAML entry so "polyon" is corrected to "polygon" (ensure the
description string for dataSourceAreaRatio is updated exactly and scan nearby
description fields for the same misspelling).
In `@locales/fr/transit.json`:
- Around line 550-551: The two French translation strings for keys
DataSourceNullWarningPopup and DataSourceNullWarningComparisonPopup are missing
the preposition "dans"; update both values to replace "il est contenu une
étendue d'eau" with "il est contenu dans une étendue d'eau" (or alternatively
"il se trouve dans une étendue d'eau") so the sentences read correctly in
French.
In `@packages/transition-backend/src/api/public/AccessibilityMapAPIResponse.ts`:
- Around line 95-96: The code is using falsy coercion for dataSourceId which
treats values like empty string as undefined; update the assignment where you
set dataSourceId from queryParams (the place using queryParams.dataSourceId ||
undefined) to use the nullish coalescing operator (queryParams.dataSourceId ??
undefined) so only null or undefined become undefined while preserving other
falsy-but-intentional values; adjust the same spot where calculatePopulation and
dataSourceId are assembled to use ?? for clarity.
- Line 36: The public API type AccessibilityMapAPIResponse has changed: the old
population field was replaced by a non-optional populationData object ({
population: number | null; dataSourceAreaRatio: number }), which is a breaking
change; update the public contract either by documenting this change in the
release notes and calling out the breaking change, or by introducing API
versioning (e.g., bumping the API version and retaining the old population field
on the v1 shape while returning the new populationData on v2). Locate the
AccessibilityMapAPIResponse type definition and ensure you: (1) add a migration
note to the release notes describing the new populationData structure and that
population can be null when calculatePopulation is false, and (2) if you need
backward compatibility, add a compatibility layer or preserved population field
that maps from populationData.population for the previous API version, and bump
the API version accordingly.
In `@packages/transition-backend/src/models/db/__tests__/census.db.test.ts`:
- Around line 17-24: The object literal assigned to dataSourceAttributes is
missing a trailing semicolon; update the declaration for dataSourceAttributes
(the const dataSourceAttributes = { ... }) to end with a semicolon after the
closing brace to match file formatting and prevent lint/style issues.
In `@packages/transition-backend/src/models/db/census.db.queries.ts`:
- Around line 83-86: The expression computing data_source_area_ratio incorrectly
passes the JSON bind :polygon into ST_AREA; change the right-hand
ST_AREA(:polygon) call to ST_AREA(ST_GeomFromGeoJSON(:polygon)) so both area
calculations operate on geometries (keep the left side using
ST_GeomFromGeoJSON(:polygon) and the alias data_source_area_ratio unchanged).
- Around line 88-89: The SQL interpolates dataSourceId directly into the query
(risking SQL injection) — change the query to use a named parameter (e.g.
:dataSourceId) instead of '${dataSourceId}' and supply dataSourceId in the query
parameters when calling the execution path that uses this SQL (update the code
that constructs/executes the query in the same function or module that
references dataSourceId, so the parameter name matches). Ensure the parameter
type/format matches existing bindings (like :polygon) and remove any string
interpolation of dataSourceId from the query string.
In
`@packages/transition-backend/src/services/accessibilityMap/TransitAccessibilityMapCalculator.ts`:
- Around line 106-107: The object initialization in
TransitAccessibilityMapCalculator (properties calculatePopulation and
dataSourceId) uses a redundant fallback; remove the unnecessary "|| undefined"
and set dataSourceId directly from attributes (i.e., replace dataSourceId:
attributes.dataSourceId || undefined with dataSourceId: attributes.dataSourceId)
while leaving calculatePopulation as attributes.calculatePopulation || false
unchanged or, if desired, use Boolean coercion consistently.
In
`@packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonForm.tsx`:
- Around line 539-548: The zonesDataSources mapping can crash if
this.state.dataSourceCollection is undefined and is accessing fragile internal
_attributes; add a null/undefined guard like checking
this.state.dataSourceCollection and its .features before mapping, and replace
direct _attributes usage with the public accessors used elsewhere (e.g., use
source.id and source.toString() or source.name if available) inside the
zonesDataSources computation so it safely returns an empty array when the
collection is missing and uses the object's public API (zonesDataSources,
this.state.dataSourceCollection.features, source.id,
source.toString()/source.name).
In
`@packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx`:
- Around line 296-310: The tooltip render uses the magic string check (content
=== '1') to pick between properties1 and properties2 and also assumes
populationData exists; update the implementation to avoid the magic-string
fragility and null access: either split into two explicit Tooltip components
(one for result 1 and one for result 2) or replace the string comparison with a
clear enum/constant or numeric check, and guard access to
populationData.dataSourceAreaRatio with optional chaining (e.g.,
populationData?.dataSourceAreaRatio) and a safe fallback percentage; update the
render callback (render) and the DataSourceWarningPopup translation usage
accordingly so it never dereferences missing populationData on
properties1/properties2.
- Around line 282-290: The null-population warning accesses
properties1.populationData.population and properties2.populationData.population
without guarding for missing populationData; update the check in
AccessibilityComparisonStatsComponent (the JSX block that renders the warning
when props.calculatePopulation is true) to use optional chaining for both
properties1 and properties2 (e.g., properties1.populationData?.population and
properties2.populationData?.population) so the condition won’t throw when
populationData is undefined.
- Around line 186-192: Replace the inline style={{ color: 'yellow' }} used on
the warning span elements in AccessibilityComparisonStatsComponent with a SCSS
class (e.g., _warning-icon); update the span(s) that have
data-tooltip-id={`data-source-incomplete-area-warning-${durationMinutes}`} (and
the second identical occurrence) to use className="_warning-icon" instead of the
inline style, and add the corresponding ._warning-icon rule in the component's
SCSS file to set the color and any tooltip-related styles so visual behavior
remains identical.
- Line 291: Remove the stray <br /> used between table rows in
AccessibilityComparisonStatsComponent's JSX (inside the
AccessibilityComparisonStatsComponent render/return) because a <br /> is not a
valid direct child of a table row group; replace it by either adding spacing via
CSS on the row/cell elements (e.g., margin/padding classes on the <tr> or <td>
elements used in the component) or inserting a proper spacer <tr> with a single
empty <td> (and appropriate colspan) to maintain valid HTML structure.
- Around line 177-178: The null/undefined guard is unsafe because expressions
like properties1.populationData.population will throw if populationData is
missing; update all checks and uses to use optional chaining (e.g., replace
properties1.populationData.population and properties2.populationData.population
with properties1.populationData?.population and
properties2.populationData?.population) and similarly change any other access
patterns (e.g., populationData.data, populationData.whatever) inside
AccessibilityComparisonStatsComponent to use ?. so existence checks and
comparisons no longer throw when populationData is undefined.
In
`@packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx`:
- Around line 401-413: When calculatePopulation
(routing.attributes.calculatePopulation) is true and the InputSelect (id
formFieldTransitAccessibilityMapDataSourceSelect...) is rendered, ensure the UI
handles empty zonesDataSources and keeps the model in sync: if zonesDataSources
is empty, render a fallback message/disabled select with a helpful label;
otherwise, if routing.attributes.dataSourceId is undefined, programmatically set
dataSourceId to zonesDataSources[0].id by calling the same handler used by the
component (onValueChange('dataSourceId', { value: ... })) so the InputSelect and
model stay consistent and validation won't fail.
- Line 68: The field dataSourceCollection in AccessibilityMapForm.tsx is
currently typed as any; replace it with a specific interface or existing type
instead of any (e.g., DataSourceCollection, IDataSource[], or reusing the same
type used for scenarioCollection) to restore type safety. Locate the declaration
of dataSourceCollection in the AccessibilityMapForm component and import or
declare the appropriate type (or a generic type parameter) that matches the
shape used by form logic and consumers, update the property signature to that
concrete type, and adjust any places that read/write it to satisfy the new type
(adding lightweight type guards or casts only where necessary).
- Around line 253-262: The code reads this.state.dataSourceCollection.features
without guarding for undefined, which can throw if data sources haven't loaded;
update the zonesDataSources computation (the variable zonesDataSources and its
use) to first check this.state.dataSourceCollection (or default it) before
accessing .features — e.g., use a null guard or default empty collection so
.filter/.map run safely when dataSourceCollection is undefined or features is
missing, preserving the current filter on source._attributes.type === 'zones'
and the mapping to value/label.
In
`@packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx`:
- Around line 192-207: The Tooltip elements currently rendered inside the table
in AccessibilityMapStatsComponent (the Tooltip with id "accessible-POI-header"
and the id using properties.durationMinutes) are invalid children of a <table>;
move these Tooltip components out of the table markup so they are siblings
outside the table container (for example, render them immediately before or
after the table element or at the component root). Ensure you keep the same
props (id, opacity, style, and content using t(...) and
properties.populationData.dataSourceAreaRatio/toLocaleString(language)) and
update any references to properties.durationMinutes used in the id so the ids
remain unique.
- Line 135: The JSX checks properties.populationData.population directly which
can throw when populationData is undefined; update
AccessibilityMapStatsComponent to first guard that properties.populationData
exists (e.g., check typeof properties.populationData === 'object' or
properties.populationData != null) before accessing
properties.populationData.population, and change the conditional to something
like "properties.populationData && typeof properties.populationData.population
=== 'number'" so the render only accesses population when populationData is
present.
packages/transition-backend/src/api/public/AccessibilityMapAPIResponse.ts
Outdated
Show resolved
Hide resolved
packages/transition-backend/src/api/public/AccessibilityMapAPIResponse.ts
Outdated
Show resolved
Hide resolved
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx
Show resolved
Hide resolved
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx
Outdated
Show resolved
Hide resolved
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx
Outdated
Show resolved
Hide resolved
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Outdated
Show resolved
Hide resolved
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 14 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="locales/fr/transit.json">
<violation number="1" location="locales/fr/transit.json:550">
P3: Correction de français: il manque "dans" dans "il est contenu une étendue d'eau".</violation>
<violation number="2" location="locales/fr/transit.json:551">
P3: Correction de français: il manque "dans" dans "il est contenu une étendue d'eau".</violation>
</file>
<file name="docs/APIv1/accessibilityMap.yml">
<violation number="1" location="docs/APIv1/accessibilityMap.yml:265">
P3: Fix the typo in the dataSourceAreaRatio description to avoid confusing API consumers.</violation>
</file>
<file name="packages/transition-backend/src/models/db/census.db.queries.ts">
<violation number="1" location="packages/transition-backend/src/models/db/census.db.queries.ts:85">
P1: SQL type error: `ST_AREA(:polygon)` will fail because `:polygon` is a JSON string, not a geometry. It needs `ST_GeomFromGeoJSON` conversion like the other usages.</violation>
<violation number="2" location="packages/transition-backend/src/models/db/census.db.queries.ts:88">
P0: SQL injection vulnerability: `dataSourceId` is directly interpolated into the SQL string. Use a parameterized query binding like the `polygon` parameter does.
Change the WHERE clause to use a binding placeholder (e.g., `:dataSourceId`) and add it to the bindings object.</violation>
</file>
<file name="packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx">
<violation number="1" location="packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx:253">
P2: Guard access to dataSourceCollection in render; the new code dereferences `dataSourceCollection.features` without a null check and can crash while data sources are still loading.</violation>
</file>
<file name="packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx">
<violation number="1" location="packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx:135">
P1: Potential TypeError: Accessing `properties.populationData.population` will throw if `populationData` is undefined (e.g., when population calculation is disabled or data comes from an older result). Add a guard or use optional chaining: `properties.populationData && typeof properties.populationData.population === 'number'`.</violation>
<violation number="2" location="packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx:192">
P2: Invalid HTML: `<Tooltip>` elements are rendered as direct children of `<table>`, but only `<thead>`, `<tbody>`, `<tfoot>`, `<colgroup>`, `<col>`, and `<caption>` are valid direct children. This may cause rendering issues or hydration mismatches in React. Move the tooltips outside the `<table>` element.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Outdated
Show resolved
Hide resolved
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx
Outdated
Show resolved
Hide resolved
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Outdated
Show resolved
Hide resolved
GabrielBruno24
force-pushed
the
selectPopulationDataSource
branch
2 times, most recently
from
bcf047c to
b0d414e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/transition-backend/src/models/db/__tests__/census.db.test.ts (1)
94-147: 🧹 Nitpick | 🔵 TrivialConsider test.each for population/ratio cases.
These cases share the same structure and use multiple expects; a table-driven test would be clearer.As per coding guidelines, "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests".
packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonForm.tsx (1)
73-81:⚠️ Potential issue | 🟠 MajorImport and use
DataSourceCollectiontype instead ofany.Replace
dataSourceCollection: any;withDataSourceCollectionin the state interface (line 81) and wherever else it appears in the component. Add the import:import DataSourceCollection from 'chaire-lib-common/lib/services/dataSource/DataSourceCollection';This aligns with the coding guideline to avoid
anytypes and matches the pattern used elsewhere in the codebase (e.g., LayersAndCollectionsService.ts).packages/transition-backend/src/services/accessibilityMap/__tests__/TransitAccessibilityMapCalculator.test.ts (1)
291-371: 🧹 Nitpick | 🔵 TrivialMissing test:
calculatePopulation: truewithoutdataSourceId.The calculator skips population when
dataSourceIdis falsy (Line 519 inTransitAccessibilityMapCalculator.ts). Consider adding a test forcalculatePopulation: truewith nodataSourceIdto confirm it yields{ population: null, dataSourceAreaRatio: 0 }.packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx (2)
86-86:⚠️ Potential issue | 🔴 CriticalCritical:
useStatecalled inside.map()— violates Rules of Hooks.React hooks must only be called at the top level of a function component, never inside loops, conditions, or nested callbacks. Calling
useStateinsidefeatures.map()will cause unpredictable state behavior and likely runtime errors when the number of features changes.Extract each feature's rendering into a separate component so each has its own valid hook scope.
Proposed fix — extract a child component
+const FeatureStatsRow: React.FunctionComponent<{ + feature: GeoJSON.Feature; + calculatePopulation?: boolean; + language: string; +}> = ({ feature, calculatePopulation, language }) => { + const { t } = useTranslation(['transit', 'main']); + const [showEmptyDetailedCategories, setShowEmptyDetailedCategories] = useState(false); + const collapsibleBackgroundColor = 'rgba(255, 255, 255, 0.05)'; + const textEmphasizedColor = 'rgb(216, 239, 253)'; + + const properties = feature.properties; + if (!properties) return null; + + // ... move the per-feature logic here ... +};Then in the parent:
- const featureStats = features.map((feature) => { - // ...all current logic including useState... - }); + const featureStats = features.map((feature) => ( + <FeatureStatsRow + key={feature.properties?.durationMinutes} + feature={feature} + calculatePopulation={props.calculatePopulation} + language={language} + /> + ));
31-38:⚠️ Potential issue | 🟠 Major
features.sort()mutates props in place.
Array.sort()mutates the original array. Sincefeaturescomes fromprops.accessibilityPolygons.features, this directly mutates props, which is a React anti-pattern and can cause subtle bugs.Fix — sort a copy
- features.sort((feat1, feat2) => { + const sortedFeatures = [...features].sort((feat1, feat2) => {Then use
sortedFeaturesbelow.
🤖 Fix all issues with AI agents
In
`@packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx`:
- Around line 294-313: The Tooltip in AccessibilityComparisonStatsComponent is
using an inline style prop; remove the style prop from the Tooltip (the instance
with id `data-source-incomplete-area-warning-${durationMinutes}`) and instead
add a descriptive className (e.g. `data-source-incomplete-area-warning`) to the
Tooltip component, then place the equivalent rules (max-width: 90%; z-index: 100
and any other visual tweaks) into the component SCSS file under that class so
styling is applied via SCSS and not inline.
...ntend/src/components/forms/accessibilityComparison/AccessibilityComparisonStatsComponent.tsx
Outdated
Show resolved
Hide resolved
| return Number(result.row_count) === 0 ? null : Number(result.weighted_population); | ||
|
|
||
| return { | ||
| population: Number(result.row_count) === 0 ? null : Number(result.weighted_population), |
There was a problem hiding this comment.
0 === null? Et si oui, pourquoi dataSourceAreaRatio ne serait pas null aussi? En fait dans ce cas null === 0! D'ailleurs si data_source_area_ratio === null, est-ce que ça ne veut pas dire qu'il n'y a aucune intersection avec le polygon d'accessibilité? Dans le cas où la population === 0, il y a le cas où la population est réellement de 0 (le data_source_area_ratio ne sera pas null) et le cas où il n'y a pas de population (data_source_area_ratio null)
There was a problem hiding this comment.
On met a null spécifiquement le nombre de rows est 0, pas la population.
Pour dataSourceAreaRatio je voulais le mettre juste en nombre pour simplifier le type de retour. Il n'y aura jamais de situation où il serait null mais population n'est pas null, donc coté frontend il suffit juste de checker que population = null avant de regarder dataSourceAreaRatio.
GabrielBruno24
force-pushed
the
selectPopulationDataSource
branch
from
b0d414e to
e8b8494
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/transition-backend/src/models/db/__tests__/census.db.test.ts (1)
103-147: 🧹 Nitpick | 🔵 TrivialPrefer table-driven assertions for these population cases.
These tests have multiple expects; considertest.eachwith expected{ population, dataSourceAreaRatio }to align with the parametric-tests guideline.As per coding guidelines: "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests."
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx (1)
86-86:⚠️ Potential issue | 🔴 CriticalHooks violation:
useStatecalled inside.map()callback.
useStateat Line 86 is called within thefeatures.map()loop. This breaks the Rules of Hooks — hooks must not be called inside loops, conditions, or nested functions. If the number of features changes between renders, React will misalign hook state.Extract each feature's rendering into a separate child component that owns its own
useState.Suggested approach
+const FeatureStats: React.FunctionComponent<{ feature: GeoJSON.Feature; calculatePopulation?: boolean }> = ({ feature, calculatePopulation }) => { + const { t, i18n } = useTranslation(['transit', 'main']); + const language = i18n.language; + const [showEmptyDetailedCategories, setShowEmptyDetailedCategories] = useState(false); + const properties = feature.properties; + if (!properties) return null; + // ... move all the per-feature rendering logic here ... +};Then in the parent:
- const featureStats = features.map((feature) => { - // ... all the logic with useState inside ... - }); + const featureStats = features.map((feature) => ( + <FeatureStats + key={feature.properties?.durationMinutes} + feature={feature} + calculatePopulation={props.calculatePopulation} + /> + ));
🤖 Fix all issues with AI agents
In `@docs/APIv1/accessibilityMap.yml`:
- Around line 88-91: Fix the typo in the YAML schema by renaming the property
key from dataSourceaName to dataSourceName; update the description/example block
that currently references dataSourceaName so it uses dataSourceName and verify
any other references or tests expecting dataSourceName are consistent (look for
occurrences of dataSourceaName and replace them with dataSourceName in the
accessibilityMap schema and related docs).
In `@packages/transition-backend/src/models/db/census.db.queries.ts`:
- Around line 92-94: The SQL string that builds the query joining ${tableName}
and ${parentTable} contains a trailing space at the end of the line with "AND
ST_INTERSECTS(geography::geometry, ST_GeomFromGeoJSON(:polygon));" — remove the
trailing whitespace from that line in the query literal so it ends cleanly with
the semicolon; ensure the SQL string in the census DB query (the fragment
referencing tableName, parentTable and ST_INTERSECTS(...)) has no trailing
spaces.
e8b8494 to
0b712ed
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/transition-backend/src/models/db/__tests__/census.db.test.ts (1)
103-147: 🧹 Nitpick | 🔵 TrivialConsider table-driven cases for the polygon population tests.
These tests share the same structure;
test.eachwould reduce repetition and make failures easier to scan.As per coding guidelines: "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests."♻️ Example table-driven structure
- test('get population inside polygon', async () => { - const population = await dbQueries.getPopulationInPolygon(polygon, 'Test datasource'); - expect(population.population).toEqual(123456 + 500 / 2); - expect(population.dataSourceAreaRatio).toEqual(1); - }); - test('population partially outside data source', async () => { - const population = await dbQueries.getPopulationInPolygon(polygon, 'Test datasource'); - expect(population.population).toEqual(123456 / 2); - expect(population.dataSourceAreaRatio).toEqual(0.5); - }); + test.each([ + ['inside polygon', polygonInside, 123456 + 500 / 2, 1, 'Test datasource'], + ['partially outside data source', polygonPartial, 123456 / 2, 0.5, 'Test datasource'] + ])('%s', async (_name, polygon, expectedPopulation, expectedRatio, dataSource) => { + const population = await dbQueries.getPopulationInPolygon(polygon, dataSource); + expect(population.population).toEqual(expectedPopulation); + expect(population.dataSourceAreaRatio).toEqual(expectedRatio); + });packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx (1)
86-86:⚠️ Potential issue | 🔴 CriticalExtract per-feature rendering into a separate component —
useStateinside.map()violates Rules of Hooks.
useStateat line 86 is called insidefeatures.map()(line 40). React hooks must be called at the top level of a component, not inside loops, conditions, or nested functions. If the number of features changes between renders, hook call order will break React's internal tracking and cause crashes.Extract the per-feature rendering into a separate component so each feature instance has its own stable hook scope.
Proposed fix — extract a child component
+const FeatureStatsEntry: React.FunctionComponent<{ feature: GeoJSON.Feature; calculatePopulation?: boolean; language: string; t: any; collapsibleBackgroundColor: string; textEmphasizedColor: string }> = ({ feature, calculatePopulation, language, t, collapsibleBackgroundColor, textEmphasizedColor }) => { + const properties = feature.properties; + if (!properties) return null; + + const [showEmptyDetailedCategories, setShowEmptyDetailedCategories] = useState(false); + + // ... move the rest of the per-feature logic here ... +};Then in the parent:
- const featureStats = features.map((feature) => { - // ... all logic including useState ... - }); + const featureStats = features.map((feature, index) => ( + <FeatureStatsEntry + key={feature.properties?.durationMinutes ?? index} + feature={feature} + calculatePopulation={props.calculatePopulation} + language={language} + t={t} + collapsibleBackgroundColor={collapsibleBackgroundColor} + textEmphasizedColor={textEmphasizedColor} + /> + ));
GabrielBruno24
force-pushed
the
selectPopulationDataSource
branch
from
0b712ed to
287c8f5
Compare
| <InputSelect | ||
| id={`formFieldTransitAccessibilityMapDataSourceSelect${routingId}`} | ||
| value={routing.attributes.dataSourceName} | ||
| choices={zonesDataSources} |
There was a problem hiding this comment.
Est-ce que les zones ont forcément des données de population? Aussi, s'il n'y a pas de data source de population, est-ce que la possibilité de calculer la population est cachée?
There was a problem hiding this comment.
S'il n'y a pas de data source, on a quand même l'option mais le drop down pour choisir la source sera vide.
Les data source de type zone n'ont pas forcément de données de population, effectivement. Si on essaye de faire un calcul avec, on va avoir l'avertissement que 0% du polygone couvre le data, mais je ne suis pas sur de comment on ferait pour faire la différence avec un polygone qui est vraiment en dehors des zones importées. Peut etre qu'on devrait rajouter un autre type de data source pour la population?
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/transition-backend/src/models/db/__tests__/census.db.test.ts (1)
94-147: 🧹 Nitpick | 🔵 TrivialConsider a
test.eachtable for these population/ratio cases.These four tests share the same shape and multiple assertions; a table-driven test would reduce duplication and align with the testing guideline.
💡 Example refactor sketch
- test('get population inside polygon', async () => { ... }); - test('population partially outside data source', async () => { ... }); - test('try to get population of polygon outside of zones in db', async () => { ... }); - test('try to get population with data source not in db', async () => { ... }); + test.each([ + ['inside polygon', polygonInside, 'Test datasource', 123456 + 500 / 2, 1], + ['partially outside', polygonPartial, 'Test datasource', 123456 / 2, 0.5], + ['outside zones', polygonOutside, 'Test datasource', null, 0], + ['missing datasource', polygonInside, 'other datasource', null, 0] + ])('%s', async (_label, polygon, dataSourceName, expectedPopulation, expectedRatio) => { + const population = await dbQueries.getPopulationInPolygon(polygon, dataSourceName); + expect(population.population).toEqual(expectedPopulation); + expect(population.dataSourceAreaRatio).toEqual(expectedRatio); + });As per coding guidelines: "Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests".
packages/transition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonForm.tsx (1)
73-81:⚠️ Potential issue | 🟡 MinorReplace
anytypes for bothdataSourceCollectionandscenarioCollection.Use
DataSourceCollectionfromchaire-lib-common/lib/services/dataSource/DataSourceCollectionandScenarioCollectionfromtransition-common/lib/services/scenario/ScenarioCollectionto maintain type safety, as per coding guidelines.packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx (1)
40-86:⚠️ Potential issue | 🔴 Critical
useStateinside.map()violates Rules of Hooks.
useStateon line 86 is called inside thefeatures.map()callback. React hooks must be called at the top level of a component — never inside loops, conditions, or nested functions. This will cause unpredictable state behavior when the number of features changes between renders.Extract each feature's rendering into a separate child component so each instance owns its own
useState.Suggested approach
+const FeatureStatsItem: React.FunctionComponent<{ + feature: GeoJSON.Feature; + calculatePopulation?: boolean; + language: string; +}> = ({ feature, calculatePopulation, language }) => { + const { t } = useTranslation(['transit', 'main']); + const [showEmptyDetailedCategories, setShowEmptyDetailedCategories] = useState(false); + const collapsibleBackgroundColor = 'rgba(255, 255, 255, 0.05)'; + const textEmphasizedColor = 'rgb(216, 239, 253)'; + + const properties = feature.properties; + if (!properties) return null; + + // ... move the per-feature rendering logic here ... +};Then in the parent:
- const featureStats = features.map((feature) => { - // ... all current logic including useState ... - }); + const featureStats = features.map((feature, idx) => ( + <FeatureStatsItem + key={feature.properties?.durationMinutes ?? idx} + feature={feature} + calculatePopulation={props.calculatePopulation} + language={language} + /> + ));
🤖 Fix all issues with AI agents
In
`@packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx`:
- Around line 178-184: In AccessibilityMapStatsComponent,
accessiblePlacesCountByCategory is an array of <tr> elements but is being
rendered inside a <div> (around collapsibleBackgroundColor) which produces
invalid HTML; update the JSX where accessiblePlacesCountByCategory and
detailedCategoryCollapsible are rendered so the <tr> elements are placed inside
a proper table structure (e.g., wrap them with
<table><tbody>{accessiblePlacesCountByCategory}</tbody></table>) or convert the
row elements to div-based markup, ensuring the render location (the div with
collapsibleBackgroundColor) contains valid children; adjust only the rendering
around accessiblePlacesCountByCategory and detailedCategoryCollapsible in
AccessibilityMapStatsComponent.
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Show resolved
Hide resolved
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Show resolved
Hide resolved
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Outdated
Show resolved
Hide resolved
GabrielBruno24
force-pushed
the
selectPopulationDataSource
branch
from
287c8f5 to
cf4ec97
Compare
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
Comme ce n'est plus très urgent, ce serait bien de cacher l'option du calcul de population dans les instances ne permettant pas ce choix.
...transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapStatsComponent.tsx
Show resolved
Hide resolved
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx
Outdated
Show resolved
Hide resolved
GabrielBruno24
force-pushed
the
selectPopulationDataSource
branch
2 times, most recently
from
d46b99e to
87f0d7a
Compare
...sition-frontend/src/components/forms/accessibilityComparison/AccessibilityComparisonForm.tsx
Outdated
Show resolved
Hide resolved
packages/transition-frontend/src/components/forms/accessibilityMap/AccessibilityMapForm.tsx
Outdated
Show resolved
Hide resolved
Calculating population in accessibility maps will now ask to enter a data source of the zones type. The population function also returns a second parameter which indicates how much of the polygon is covered by the zones in the data source, and will warn the user when it is below 99% (for example, if the accessibility map is calculated just outside of the data source's border).
GabrielBruno24
force-pushed
the
selectPopulationDataSource
branch
from
87f0d7a to
715417f
Compare
3 participants
Calculating population in accessibility maps will now ask to enter a data source of the zones type. The population function also returns a second parameter which indicates how much of the polygon is covered by the zones in the data source, and will warn the user when it is below 99% (for example, if the accessibility map is calculated just outside of the data source's border).
Summary by CodeRabbit
New Features
Improvements
Documentation / Localization