Add min waypoint zoom#1933
Conversation
WalkthroughThis change introduces configurable minimum zoom level for path waypoint map layers. It adds a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
kaligrafy
force-pushed
the
addMinWaypointZoom
branch
from
0cfcab8 to
6191981
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@locales/en/transit.json`:
- Around line 399-401: Update the "WaypointMinZoomRefreshHint" translation value
to remove the instruction to refresh and instead state that the waypoint min
zoom is applied immediately when the preference changes (e.g., "This change is
applied immediately; no page refresh required."), and make the equivalent edit
to the corresponding French translation key so both locales reflect the new
live-update behavior.
In `@packages/chaire-lib-common/src/config/Preferences.ts`:
- Around line 192-202: The validation for waypointMinZoom (computed from
_get(this.attributes, 'map.pathWaypointMinZoom') /
'transit.paths.waypointMinZoom') wrongly uses _isNumber which accepts NaN and
decimals; change the check to ensure the value is a finite integer within
MIN_WAYPOINT_MIN_ZOOM and MAX_WAYPOINT_MIN_ZOOM (e.g. replace the _isNumber test
with a combination like Number.isFinite/Number.isInteger or lodash _.isInteger
and a finite check) so NaN and non-integers fail validation and the existing
error push for WaypointMinZoomOutOfRange and setting this._isValid = false
remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60e55d42-de72-4232-ad20-c4b24adb0427
📒 Files selected for processing (12)
locales/en/transit.jsonlocales/fr/transit.jsonpackages/chaire-lib-common/src/config/Preferences.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/chaire-lib-common/src/config/defaultPreferences.config.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-common/src/services/line/__tests__/Line.test.tspackages/transition-common/src/services/path/Path.tspackages/transition-common/src/services/path/__tests__/Path.test.tspackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/config/layers.config.ts
📜 Review details
⏰ 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). (7)
- GitHub Check: test-sequential (24.x)
- GitHub Check: code-lint
- GitHub Check: build-and-test (24.x)
- GitHub Check: check-format
- GitHub Check: Json2capnp
- GitHub Check: pr-build-check
- GitHub Check: build-and-run-UI-tests (24.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/transition-common/src/services/line/__tests__/Line.test.tspackages/chaire-lib-common/src/config/defaultPreferences.config.tspackages/chaire-lib-common/src/config/Preferences.tspackages/transition-frontend/src/config/layers.config.tspackages/transition-common/src/services/path/Path.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-common/src/services/path/__tests__/Path.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/transition-common/src/services/line/__tests__/Line.test.tspackages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/chaire-lib-common/src/config/defaultPreferences.config.tspackages/chaire-lib-common/src/config/Preferences.tspackages/transition-frontend/src/config/layers.config.tspackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-common/src/services/path/Path.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-common/src/services/path/__tests__/Path.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/transition-common/src/services/line/__tests__/Line.test.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-common/src/services/path/__tests__/Path.test.ts
packages/transition-frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
React components should use Redux for state management, i18next for translations, and SCSS for styling
Files:
packages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx
**/*.json
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Translation files in
locales/directory should organize strings by namespace (auth.json, transit.json, main.json, etc.) with support for both English (en/) and French (fr/) languages
Files:
locales/en/transit.jsonlocales/fr/transit.json
🧠 Learnings (5)
📚 Learning: 2026-02-24T00:36:44.483Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:44.483Z
Learning: In test files, it is acceptable to use 'any' types for error handling and test scenarios. Reserve strict TypeScript typing for production code; in tests, prioritize pragmatic, readable test cases, and use 'any' only where it clearly improves test clarity or reduces boilerplate.
Applied to files:
packages/transition-common/src/services/line/__tests__/Line.test.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-common/src/services/path/__tests__/Path.test.ts
📚 Learning: 2026-03-02T21:57:21.005Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1830
File: packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts:114-122
Timestamp: 2026-03-02T21:57:21.005Z
Learning: In test files for database query modules, ensure mocked functions match the async/sync nature of the production implementations. If a function returns a Promise in production (async), use an async mock (e.g., jest.fn().mockImplementation(async (...args) => { ... })). If a function is synchronous (e.g., returns a stream or uses direct values), use a synchronous mock. This helps prevent false positives/negatives due to mismatched mock behavior.
Applied to files:
packages/transition-common/src/services/line/__tests__/Line.test.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-common/src/services/path/__tests__/Path.test.ts
📚 Learning: 2026-04-13T19:53:39.405Z
Learnt from: thomasspina
Repo: chairemobilite/transition PR: 1892
File: packages/transition-common/src/services/path/PathServiceGrouping.ts:78-129
Timestamp: 2026-04-13T19:53:39.405Z
Learning: In this repo, `packages/transition-common` must not use `i18next` (it is not a dependency). Any code that generates user-facing strings via `i18next` (e.g., calling `t()`) must be implemented in `packages/transition-frontend` (or another package that explicitly depends on `i18next`). When reviewing files under `packages/transition-common`, flag any imports/usages of `i18next` or direct/indirect calls to `t()` that would produce user-facing strings there.
Applied to files:
packages/transition-common/src/services/line/__tests__/Line.test.tspackages/transition-common/src/services/path/Path.tspackages/transition-common/src/services/path/__tests__/Path.test.ts
📚 Learning: 2026-01-21T14:59:52.530Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
Applied to files:
packages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx
📚 Learning: 2026-04-20T20:48:11.341Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1916
File: packages/transition-frontend/src/components/forms/gtfs/GtfsExportForm.tsx:111-118
Timestamp: 2026-04-20T20:48:11.341Z
Learning: In the Transition frontend codebase, `service.attributes.scheduled_lines` should be treated as always initialized to an array (at minimum `[]`) and not `undefined`. Therefore, you can call array methods like `.some()` or iterate over it directly without adding a null/undefined guard such as `scheduled_lines ?? []`. Only add such a guard if the value’s type/initialization guarantee changes and `scheduled_lines` can become `undefined`.
Applied to files:
packages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx
🔇 Additional comments (6)
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts (1)
200-255: LGTM!Thorough test coverage for the new
map.pathWaypointMinZoompreference. Good use of parametric tests for range validation and nice inclusion of the legacy fallback case.packages/transition-frontend/src/config/layers.config.ts (2)
26-43: LGTM!The helper function correctly reads from preferences with fallback to legacy path, applies clamping, and returns a validated number. The exported
TRANSIT_PATH_WAYPOINT_MAP_LAYER_NAMESarray enables dynamic layer updates elsewhere.
378-432: LGTM!Layer configs now dynamically fetch waypoint min zoom from preferences.
packages/transition-common/src/services/path/__tests__/Path.test.ts (2)
62-62: LGTM!Using
mergeAttributesinstead ofsetAttributesaligns with the updated test setup pattern.
587-597: LGTM!Test expectations correctly reflect the updated
removeNodebehavior where waypoints from the preceding segment and the removed node's segment are concatenated onto the merged segment.packages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx (1)
22-68: LGTM!Clean implementation with proper i18n, reasonable defensive coding with optional chaining, and good documentation of the validation bypass logic. The reset button and help text provide a complete UX.
kaligrafy
force-pushed
the
addMinWaypointZoom
branch
from
6191981 to
76aa436
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
locales/en/transit.json (1)
401-401:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh hint is stale with live min-zoom updates.
Line 401 says a page refresh is needed, but this PR applies waypoint min-zoom dynamically at runtime. Please update this key (and the matching FR key) to avoid misleading users.
Proposed wording update
- "WaypointMinZoomRefreshHint": "You need to refresh the page for this change to apply on the map.", + "WaypointMinZoomRefreshHint": "This change is applied immediately on the map."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locales/en/transit.json` at line 401, Update the "WaypointMinZoomRefreshHint" translation to remove the mention of needing to refresh the page since waypoint min-zoom is applied dynamically at runtime; change the English string to something like "Changes to waypoint min-zoom take effect immediately on the map." and update the corresponding French translation key to match this new wording so users are not misled. Ensure you edit the "WaypointMinZoomRefreshHint" entry in locales/en/transit.json and the matching key in the French locale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/transition-frontend/src/services/map/PathWaypointZoomSync.ts`:
- Around line 49-62: The code mixes two ways to access the EventEmitter:
applyNow(eventManager: EventEmitter) accepts an emitter while
_onPreferencesUpdated uses the instance field _eventManager; make this
consistent by changing applyNow to a parameterless method that uses
this._eventManager (ensure start() sets this._eventManager before use), update
_onPreferencesUpdated to call this.applyNow(), and update any external callers
of applyNow to call it without arguments; keep the
MAP_UPDATE_LAYERS_MIN_ZOOM_EVENT payload construction
(MapUpdateLayersMinZoomPayload and getWaypointMinZoom()) unchanged.
- Around line 32-38: The start method silently ignores being called again with a
different EventEmitter; update start(eventManager: EventEmitter) to detect when
this._eventManager is already set and the passed eventManager !==
this._eventManager, and in that case either log a warning via the existing
logger or throw/assert (pick one consistent with project style) before
returning; reference the start method, the this._eventManager field, and the
this._onPreferencesUpdated handler to locate the code to modify.
---
Duplicate comments:
In `@locales/en/transit.json`:
- Line 401: Update the "WaypointMinZoomRefreshHint" translation to remove the
mention of needing to refresh the page since waypoint min-zoom is applied
dynamically at runtime; change the English string to something like "Changes to
waypoint min-zoom take effect immediately on the map." and update the
corresponding French translation key to match this new wording so users are not
misled. Ensure you edit the "WaypointMinZoomRefreshHint" entry in
locales/en/transit.json and the matching key in the French locale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 795199cf-fbd0-41c5-802b-e0e97517b573
📒 Files selected for processing (11)
locales/en/transit.jsonlocales/fr/transit.jsonpackages/chaire-lib-common/src/config/Preferences.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/chaire-lib-common/src/config/defaultPreferences.config.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/config/layers.config.tspackages/transition-frontend/src/services/map/PathWaypointZoomSync.tspackages/transition-frontend/src/services/map/events/PathSectionMapEvents.ts
📜 Review details
⏰ 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). (5)
- GitHub Check: pr-build-check
- GitHub Check: build-and-test (24.x)
- GitHub Check: test-sequential (24.x)
- GitHub Check: code-lint
- GitHub Check: build-and-run-UI-tests (24.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/chaire-lib-common/src/config/defaultPreferences.config.tspackages/transition-frontend/src/services/map/PathWaypointZoomSync.tspackages/chaire-lib-common/src/config/Preferences.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-frontend/src/config/layers.config.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-frontend/src/services/map/events/PathSectionMapEvents.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/chaire-lib-common/src/config/defaultPreferences.config.tspackages/transition-frontend/src/services/map/PathWaypointZoomSync.tspackages/chaire-lib-common/src/config/Preferences.tspackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/config/layers.config.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-frontend/src/services/map/events/PathSectionMapEvents.ts
**/*.json
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Translation files in
locales/directory should organize strings by namespace (auth.json, transit.json, main.json, etc.) with support for both English (en/) and French (fr/) languages
Files:
locales/en/transit.jsonlocales/fr/transit.json
packages/transition-frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
React components should use Redux for state management, i18next for translations, and SCSS for styling
Files:
packages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-frontend/src/components/map/TransitionMainMap.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts
🧠 Learnings (4)
📚 Learning: 2026-01-21T14:59:52.530Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
Applied to files:
packages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-frontend/src/components/map/TransitionMainMap.tsx
📚 Learning: 2026-04-20T20:48:11.341Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1916
File: packages/transition-frontend/src/components/forms/gtfs/GtfsExportForm.tsx:111-118
Timestamp: 2026-04-20T20:48:11.341Z
Learning: In the Transition frontend codebase, `service.attributes.scheduled_lines` should be treated as always initialized to an array (at minimum `[]`) and not `undefined`. Therefore, you can call array methods like `.some()` or iterate over it directly without adding a null/undefined guard such as `scheduled_lines ?? []`. Only add such a guard if the value’s type/initialization guarantee changes and `scheduled_lines` can become `undefined`.
Applied to files:
packages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-frontend/src/components/map/TransitionMainMap.tsx
📚 Learning: 2026-02-24T00:36:44.483Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:44.483Z
Learning: In test files, it is acceptable to use 'any' types for error handling and test scenarios. Reserve strict TypeScript typing for production code; in tests, prioritize pragmatic, readable test cases, and use 'any' only where it clearly improves test clarity or reduces boilerplate.
Applied to files:
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts
📚 Learning: 2026-03-02T21:57:21.005Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1830
File: packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts:114-122
Timestamp: 2026-03-02T21:57:21.005Z
Learning: In test files for database query modules, ensure mocked functions match the async/sync nature of the production implementations. If a function returns a Promise in production (async), use an async mock (e.g., jest.fn().mockImplementation(async (...args) => { ... })). If a function is synchronous (e.g., returns a stream or uses direct values), use a synchronous mock. This helps prevent false positives/negatives due to mismatched mock behavior.
Applied to files:
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts
🔇 Additional comments (13)
packages/transition-frontend/src/services/map/PathWaypointZoomSync.ts (1)
65-65: LGTM — singleton export is appropriate here.packages/transition-frontend/src/services/map/events/PathSectionMapEvents.ts (3)
86-89: LGTM — the layer restriction nicely sidesteps the deck.gl overlay false-positives.
338-340: LGTM — dynamic min-zoom lookup at click-time keeps things in sync with preferences.Also applies to: 383-385
225-243: ⚡ Quick winNo changes needed.
transitPathWaypointscontains all waypoints regardless of selection status, so waypoint click detection (lines 311-330) works correctly for both selected and unselected waypoints. ThetransitPathWaypointsSelectedlayer is purely for visual rendering during editing, not functional click detection.> Likely an incorrect or invalid review comment.packages/transition-frontend/src/components/map/TransitionMainMap.tsx (3)
214-216: LGTM — initial sync viaapplyNowafterupdateEnabledLayersis well-ordered.
222-225: LGTM — keeping the handler generic (not waypoint-specific) is a nice touch.It simply forwards the payload to
layerManager, which keepsTransitionMainMapfrom owning waypoint-specific knowledge.
288-289: LGTM — symmetric register/unregister withpathWaypointZoomSynclifecycle.Also applies to: 335-336
packages/chaire-lib-common/src/config/defaultPreferences.config.ts (1)
46-47: Good addition of typed default for waypoint min zoom.The new
map.pathWaypointMinZoomfield is clearly documented and defaulted consistently.Also applies to: 67-68
packages/chaire-lib-common/src/config/Preferences.ts (1)
32-34: Validation logic is solid here.Using
Number.isIntegerwith explicit bounds is a clean and reliable check for this preference.Also applies to: 192-201
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts (1)
29-35: Nice test coverage for the new zoom preference.The new cases cover defaults, full valid range, invalid inputs, reset behavior, and missing-key validation.
Also applies to: 200-246
packages/chaire-lib-frontend/src/services/map/MapLayerManager.ts (1)
334-362: Good map-layer min zoom update API.Updating both the in-memory layer spec and active map layer state is the right approach.
packages/transition-frontend/src/config/layers.config.ts (1)
10-34: Nice move to a centralized dynamic min-zoom getter.Using
getWaypointMinZoom()across all waypoint layers improves consistency and keeps behavior configurable.Also applies to: 371-371, 397-397, 423-423
packages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx (1)
22-62: Good implementation of the transit-path preference section.The field wiring, reset action, translated labels/help, and range constraints are well integrated.
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
Quelques questions à valider
| this.layerManager.updateEnabledLayers(this.state.layers); | ||
| // Trigger an initial generic min-zoom update for any layer whose min zoom | ||
| // is driven by a preference (e.g. path waypoints). | ||
| pathWaypointZoomSync.applyNow(serviceLocator.eventManager); |
There was a problem hiding this comment.
Le commentaire dit "Trigger and initial generic min-zoom update", mais le nom de la variable n'a rien de générique. Renommer?
There was a problem hiding this comment.
Aussi, as-tu vraiment besoin d,appeler le applyNow? Il semble que le layer.config va déjà chercher le min zoom depuis les préférences, donc ça devrait être ok? Tu n'as peut-être pas besoin du tout de cette fonction.
Add a preference for the min zoom for waypoints .
Use Number.isInteger so NaN, decimals, strings and null fail validation explicitly instead of slipping through _isNumber. Add unit tests for NaN, decimal and undefined.
- Use "integer" instead of "whole number" in EN/FR error string. - Add a short explanation of the 1-15 zoom range so users have a reference (1 = world view, 15 = street level).
The legacy key never existed on main, so reading it as a fallback was misleading. Validation and getWaypointMinZoom now read only map.pathWaypointMinZoom; remove the related test case.
Function components should use the useTranslation hook rather than the withTranslation HOC.
Introduce PathWaypointZoomSync, which listens on preferences.updated
and emits a generic 'map.updateLayersMinZoom' event with
{ layerNames, minZoom }. TransitionMainMap now subscribes to that
generic event and forwards to MapLayerManager.updateLayersMinZoom,
removing the waypoint-specific micro-management from the map class.
The same pattern can be reused for any other layer whose minzoom is
driven by a preference.
kaligrafy
force-pushed
the
addMinWaypointZoom
branch
from
fba3b00 to
d12c973
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
locales/en/transit.json (1)
401-401:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the refresh hint to reflect immediate map updates.
Line 401 states a page refresh is required, but
PathWaypointZoomSyncapplies preference changes to map layers immediately via themap.updateLayersMinZoomevent. This text misleads users.📝 Suggested correction
- "WaypointMinZoomRefreshHint": "You need to refresh the page for this change to apply on the map.", + "WaypointMinZoomRefreshHint": "Changes are applied immediately to the map.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@locales/en/transit.json` at line 401, Update the "WaypointMinZoomRefreshHint" localization string to remove the instruction to refresh the page and instead indicate that changes take effect immediately; reference the preference/flag PathWaypointZoomSync and the runtime event map.updateLayersMinZoom so translators know to convey that the map layers update instantly when the setting is changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@locales/fr/transit.json`:
- Line 402: The current translation for WaypointMinZoomRefreshHint incorrectly
tells users to refresh the page; update the French string to inform users that
changing the preference applies immediately (no refresh needed) because
PathWaypointZoomSync and TransitionMainMap perform live updates via the
MAP_UPDATE_LAYERS_MIN_ZOOM_EVENT so the map layers update in real time when the
setting changes.
---
Duplicate comments:
In `@locales/en/transit.json`:
- Line 401: Update the "WaypointMinZoomRefreshHint" localization string to
remove the instruction to refresh the page and instead indicate that changes
take effect immediately; reference the preference/flag PathWaypointZoomSync and
the runtime event map.updateLayersMinZoom so translators know to convey that the
map layers update instantly when the setting is changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 979a7e30-cbbd-430b-a433-a2e36476f8b7
📒 Files selected for processing (11)
locales/en/transit.jsonlocales/fr/transit.jsonpackages/chaire-lib-common/src/config/Preferences.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/chaire-lib-common/src/config/defaultPreferences.config.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/config/layers.config.tspackages/transition-frontend/src/services/map/PathWaypointZoomSync.tspackages/transition-frontend/src/services/map/events/PathSectionMapEvents.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.json
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Translation files in
locales/directory should organize strings by namespace (auth.json, transit.json, main.json, etc.) with support for both English (en/) and French (fr/) languages
Files:
locales/en/transit.jsonlocales/fr/transit.json
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Use TypeScript strictly and avoid
anytypes - maintain type safety throughout the application
Files:
packages/chaire-lib-common/src/config/Preferences.tspackages/transition-frontend/src/services/map/PathWaypointZoomSync.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-frontend/src/services/map/events/PathSectionMapEvents.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-frontend/src/config/layers.config.tspackages/chaire-lib-common/src/config/defaultPreferences.config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/chaire-lib-common/src/config/Preferences.tspackages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/services/map/PathWaypointZoomSync.tspackages/chaire-lib-frontend/src/services/map/MapLayerManager.tspackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsxpackages/transition-frontend/src/services/map/events/PathSectionMapEvents.tspackages/chaire-lib-common/src/config/__tests__/Preferences.test.tspackages/transition-frontend/src/config/layers.config.tspackages/chaire-lib-common/src/config/defaultPreferences.config.ts
packages/transition-frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
React components should use Redux for state management, i18next for translations, and SCSS for styling
Files:
packages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Favor parametric tests when more than one expect statement is used to simplify debugging of failed tests
Files:
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts
🧠 Learnings (4)
📚 Learning: 2026-01-21T14:59:52.530Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
Applied to files:
packages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx
📚 Learning: 2026-04-20T20:48:11.341Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1916
File: packages/transition-frontend/src/components/forms/gtfs/GtfsExportForm.tsx:111-118
Timestamp: 2026-04-20T20:48:11.341Z
Learning: In the Transition frontend codebase, `service.attributes.scheduled_lines` should be treated as always initialized to an array (at minimum `[]`) and not `undefined`. Therefore, you can call array methods like `.some()` or iterate over it directly without adding a null/undefined guard such as `scheduled_lines ?? []`. Only add such a guard if the value’s type/initialization guarantee changes and `scheduled_lines` can become `undefined`.
Applied to files:
packages/transition-frontend/src/components/map/TransitionMainMap.tsxpackages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx
📚 Learning: 2026-02-24T00:36:44.483Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1779
File: packages/chaire-lib-backend/src/models/db/__tests__/propertyRegistry.db.test.ts:253-258
Timestamp: 2026-02-24T00:36:44.483Z
Learning: In test files, it is acceptable to use 'any' types for error handling and test scenarios. Reserve strict TypeScript typing for production code; in tests, prioritize pragmatic, readable test cases, and use 'any' only where it clearly improves test clarity or reduces boilerplate.
Applied to files:
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts
📚 Learning: 2026-03-02T21:57:21.005Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1830
File: packages/transition-backend/src/services/transitRouting/__tests__/TrRoutingBatch.test.ts:114-122
Timestamp: 2026-03-02T21:57:21.005Z
Learning: In test files for database query modules, ensure mocked functions match the async/sync nature of the production implementations. If a function returns a Promise in production (async), use an async mock (e.g., jest.fn().mockImplementation(async (...args) => { ... })). If a function is synchronous (e.g., returns a stream or uses direct values), use a synchronous mock. This helps prevent false positives/negatives due to mismatched mock behavior.
Applied to files:
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts
🔇 Additional comments (12)
packages/chaire-lib-common/src/config/defaultPreferences.config.ts (1)
46-47: LGTM!Also applies to: 67-68
packages/chaire-lib-common/src/config/Preferences.ts (1)
32-34: LGTM!Also applies to: 192-201
packages/chaire-lib-common/src/config/__tests__/Preferences.test.ts (1)
29-38: LGTM!Also applies to: 200-246
packages/transition-frontend/src/components/forms/preferences/sections/PreferencesSectionTransitPaths.tsx (1)
22-63: LGTM!locales/en/transit.json (2)
399-400: LGTM!
525-526: LGTM!locales/fr/transit.json (1)
400-401: LGTM!Also applies to: 526-526
packages/transition-frontend/src/config/layers.config.ts (1)
8-34: LGTM!Also applies to: 371-371, 397-397, 423-423
packages/transition-frontend/src/services/map/PathWaypointZoomSync.ts (1)
1-66: LGTM!packages/chaire-lib-frontend/src/services/map/MapLayerManager.ts (1)
334-362: LGTM!packages/transition-frontend/src/components/map/TransitionMainMap.tsx (1)
31-34: LGTM!Also applies to: 214-217, 223-226, 289-290, 336-337
packages/transition-frontend/src/services/map/events/PathSectionMapEvents.ts (1)
15-15: LGTM!Also applies to: 86-89, 225-243, 338-338, 383-383
| "HidePaths": "Cacher les lignes et parcours", | ||
| "WaypointMinZoom": "Zoom minimal de la carte pour les points de repère de parcours", | ||
| "WaypointMinZoomHelp": "Les points de repère s'affichent et peuvent être ajoutés ou retirés à partir de ce niveau de zoom (plage permise : 1 à 15, où 1 correspond à la vue mondiale et 15 à la rue). Une valeur plus basse permet d'éditer à une vue plus large.", | ||
| "WaypointMinZoomRefreshHint": "Il faut actualiser la page pour que ce réglage s'applique sur la carte.", |
There was a problem hiding this comment.
Refresh hint contradicts runtime sync behavior.
The text states users must refresh the page (Il faut actualiser la page), but PathWaypointZoomSync and TransitionMainMap implement live synchronization. When the preference changes, the map layers update immediately via the MAP_UPDATE_LAYERS_MIN_ZOOM_EVENT. No page refresh is required.
📝 Suggested correction
-"WaypointMinZoomRefreshHint": "Il faut actualiser la page pour que ce réglage s'applique sur la carte."
+"WaypointMinZoomRefreshHint": "Les changements s'appliquent immédiatement sur la carte."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "WaypointMinZoomRefreshHint": "Il faut actualiser la page pour que ce réglage s'applique sur la carte.", | |
| "WaypointMinZoomRefreshHint": "Les changements s'appliquent immédiatement sur la carte.", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@locales/fr/transit.json` at line 402, The current translation for
WaypointMinZoomRefreshHint incorrectly tells users to refresh the page; update
the French string to inform users that changing the preference applies
immediately (no refresh needed) because PathWaypointZoomSync and
TransitionMainMap perform live updates via the MAP_UPDATE_LAYERS_MIN_ZOOM_EVENT
so the map layers update in real time when the setting changes.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes