feat: Smart/Origin parcours 3 itinévert #4504
Conversation
… being chosen when navitia isochron threshold reached
…lti search filter now working
…licking on side menu label
…access wp are not in area of type 'range'
(cherry picked from commit c6398ffe7d74202a9f74806fb26f5afda4aba9a0)
WalkthroughAdds the Itinevert feature: multi-step public-transport journey wizard, filtering and result views, map enhancements for area polygons and new tab link behavior, Photon-based address/autocomplete inputs, a new ItinevertService (Navitia integrations), route, UI and router additions, and multiple supporting components and translations. Changes
Sequence DiagramsequenceDiagram
actor User
participant Portal as ItinevertView
participant Wizard as ItinevertWizard
participant Service as ItinevertService
participant Navitia as Navitia API
participant Photon as Photon API
participant Map as OlMap/MapView
User->>Portal: Open Itinevert
Portal->>Wizard: mount (form view)
User->>Wizard: submit form (origin, dest, datetime, activities)
Wizard->>Service: formSearch / buildQuery
Service->>Navitia: GET coverage / isochrones / polygon
Navitia-->>Service: geometry, coverage, counts
Service-->>Wizard: initial results
Wizard->>Wizard: switch to filter view
User->>Wizard: apply filters
Wizard->>Service: enhanceQuery(filters)
Service->>Navitia: GET reachable routes/waypoints (paginated)
Navitia-->>Service: paginated documents
Service-->>Wizard: aggregated results (with progress)
Wizard->>Map: render documents & polygons (openInNewTab considered)
User->>Map: click document
Map->>Map: open document (new tab if openInNewTab or modifier)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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)
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: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
src/components/generics/inputs/InputAutocomplete.vue-44-44 (1)
44-44: DefaultformatSuggestionreturnsundefined.The default
() => {}returnsundefined, which will display as text when rendering suggestions. Consider returning an empty string or the suggestion itself.- formatSuggestion: { type: Function, default: () => {} }, + formatSuggestion: { type: Function, default: (s) => s ?? '' },src/components/generics/inputs/InputAutocomplete.vue-2-2 (1)
2-2: Incorrect class binding syntax.The class binding
['control, autocomplete-wrapper', { fullwidth }]treats'control, autocomplete-wrapper'as a single class name string instead of two separate classes.- <div :class="['control, autocomplete-wrapper', { fullwidth }]"> + <div :class="['control', 'autocomplete-wrapper', { fullwidth }]">src/components/itinevert/ItinevertBanner.vue-28-28 (1)
28-28: Potentially incorrect router-link name.
{ name: '/' }is likely incorrect—'/'is typically a path, not a named route. This may result in a navigation failure.- <router-link :to="{ name: '/' }" class="menu-brand has-text-centered"> + <router-link to="/" class="menu-brand has-text-centered">Or use the correct named route if one exists (e.g.,
{ name: 'home' }).src/components/generics/inputs/InputAutocomplete.vue-46-55 (1)
46-55: Incorrect type initialization forvalueSuggestions.
valueSuggestionsis initialized as an empty string''but should be an array[]since it's used with.filter(),.slice(), and.lengtharray operations.localData: { value: this.defaultValue ? this.formatSuggestion(this.defaultValue) : '', selectedValue: '', - valueSuggestions: '', + valueSuggestions: [], showSuggestions: this.showSuggestions, },src/views/documents/utils/url-mixin.js-22-28 (1)
22-28: Missing fallback whendocumentTypequery parameter is absent.If the route name starts with "Itinevert" but
this.$route.query.documentTypeis undefined, this will returnundefined, potentially breaking thefieldscomputed property which accessesconstants.objectDefinitions[this.documentType].Consider adding a fallback:
// check if it starts with "Itinevert", so that we can retrieve the doc type if (this.$route.name.startsWith('Itinevert')) { - return this.$route.query.documentType; + return this.$route.query.documentType ?? 'route'; }src/components/itinevert/ItinevertNoResultView.vue-6-7 (1)
6-7: Fixerrorprop default factory (currently returnsundefined) and optionally guard the error lineThe
errorprop default is:default: () => {},This arrow function returns
undefined, not an empty object. If you intend an object default, it should be:error: { type: Object, - default: () => {}, + default: () => ({}), },Also, since the template always renders the error line:
<span class="no-result-found"> {{ errorMessages[error?.id] ?? error?.message }} </span>you might want to wrap it in a
v-ifso the red line only appears when there is an actual message:- <span class="no-result-found">{{ errorMessages[error?.id] ?? error?.message }}</span> + <span + class="no-result-found" + v-if="errorMessages[error?.id] ?? error?.message" + > + {{ errorMessages[error?.id] ?? error?.message }} + </span>Also applies to: 14-18
src/components/map/map-utils.js-306-345 (1)
306-345: MovezIndexfrom stroke to style to make polygon highlighting effectiveIn OpenLayers 10.7.0,
zIndexis a property ofol.style.Style, notol.style.Stroke. Setting it on the stroke object is ignored, so highlighted polygons won't actually be drawn above others.Move
zIndexto the style object:export const getDocumentPolygonStyle = function (title, highlight, properties) { // Highlighted polygon (properties provided) if (highlight && properties) { return new ol.style.Style({ fill: new ol.style.Fill({ color: properties.color, }), stroke: new ol.style.Stroke({ color: properties.strokeColor, width: 4, - zIndex: 1, }), text: buildTextStyle(properties.name || title, highlight, true), + zIndex: 1, }); }src/components/itinevert/ItinevertFilterView.vue-133-135 (1)
133-135: Potential runtime error:filteredRoutes.totalmay be undefined.The
routeCountcomputed property directly accessesfilteredRoutes.total, but the default prop value is an empty object without atotalproperty. This could cause display issues or errors.routeCount() { - return this.filteredRoutes.total; + return this.filteredRoutes?.total ?? 0; },src/components/itinevert/ItinevertFilterItem.vue-154-156 (1)
154-156: Incorrect initial value fornumericalRangeSlider.The
initialValue()method usesfield.valuesfor thenumericalRangeSlidercase, but numerical range sliders usefield.minandfield.max(as seen in lines 20-21), notfield.values.if (this.field.queryMode === 'valuesRangeSlider' || this.field.queryMode === 'numericalRangeSlider') { - return [this.field.values?.[0] ?? 0, this.field.values?.[this.field.values.length - 1] ?? 0]; + if (this.field.queryMode === 'valuesRangeSlider') { + return [this.field.values?.[0] ?? 0, this.field.values?.[this.field.values.length - 1] ?? 0]; + } + return [this.field.min ?? 0, this.field.max ?? 0]; }src/views/portals/ItinevertView.vue-73-76 (1)
73-76: localStorage key collision risk and missing restoration.The localStorage key is just the property name (e.g.,
"visible"), which could conflict with other components using the same key. Additionally, the stored value is never restored on component mount.Consider namespacing the key and restoring on mount:
+ mounted() { + const stored = this.$localStorage.get('itinevert_visible'); + if (stored !== null) { + this.visible = stored; + } + }, + methods: { changeView(view) { this.wizardView = view; }, toggleProperty(property) { this.setProperty(property, !this[property]); }, setProperty(property, value) { this[property] = value; - this.$localStorage.set(`${property}`, this[property]); + this.$localStorage.set(`itinevert_${property}`, this[property]); }, },Committable suggestion skipped: line range outside the PR's diff.
src/components/generics/inputs/InputAddress.vue-47-50 (1)
47-50: Hardcoded French placeholder should use internationalization.The default placeholder is hardcoded in French. This should either be internationalized or the default should be empty with the parent component providing a translated placeholder.
src/components/generics/inputs/InputAddress.vue-64-74 (1)
64-74: Incorrect type foraddressPropositionsinitialization.
addressPropositionsis initialized as an empty string but is used as an array throughout the component (e.g.,slice(0, 6)on line 145,.lengthcheck on line 16). This could cause runtime errors if methods are called before the first successful search.data() { return { localData: { address: this.defaultAddress ? this.formatProposition(this.defaultAddress) : '', selectedAddress: '', - addressPropositions: '', + addressPropositions: [], showAddressPropositions: this.showAddressPropositions, coordinates: null, }, }; },src/components/itinevert/ItinevertResultView.vue-362-366 (1)
362-366: Potential null reference when mapping document IDs.If
this.documents?.documentsisundefined,oldIdswill beundefinedand theincludescheck on line 390 will fail.// get list of previous document IDs - const oldIds = this.documents?.documents?.map((doc) => doc.document_id); + const oldIds = this.documents?.documents?.map((doc) => doc.document_id) ?? [];src/components/generics/inputs/InputAddress.vue-128-152 (1)
128-152: Missing timeout cleanup on component unmount.
searchTimeoutis used but not declared indata(), and more importantly, the timeout is never cleared when the component is destroyed. This could cause errors or memory leaks if the component unmounts while a search is pending.Add cleanup in a
beforeDestroyhook:+ beforeDestroy() { + if (this.searchTimeout) { + clearTimeout(this.searchTimeout); + } + }, methods: {Alternatively, declare
searchTimeoutindata()for consistency, though it works as an instance property too.Committable suggestion skipped: line range outside the PR's diff.
src/components/itinevert/ItinevertResultView.vue-202-226 (1)
202-226: Hardcodeddocument_id: 1may conflict with real documents.The synthetic area document uses
document_id: 1, which could match a real document ID, potentially causing highlighting or interaction issues. Consider using a unique identifier like a negative number or a UUID.const areaDocument = { - document_id: 1, + document_id: -1, // synthetic ID to avoid conflicts with real documents type: 'a',
🧹 Nitpick comments (25)
src/components/generics/inputs/InputRadioButton.vue (2)
11-14: Conditionally render reason text.The reason paragraph renders for all options, creating empty
<p>elements when no reason exists. Consider usingv-ifto only render when a reason is present.Apply this diff:
<div class="option"> {{ option.text }} - <p class="optionDisabled">{{ reasons[option.value] }}</p> + <p v-if="reasons[option.value]" class="optionDisabled">{{ reasons[option.value] }}</p> </div>
87-91: Remove!importantif possible.The
!importantflag on line 89 suggests specificity conflicts. In scoped component CSS, this is usually avoidable. Consider removing it and testing whether the styles still apply correctly.src/js/constants/index.js (1)
39-143: Inconsistent key casing incategorizedFieldsDefault.The key
ratings(line 55) uses lowercase while other keys (General,Terrain,Miscs) are capitalized. This inconsistency could lead to confusion or issues when iterating over categories.General: [ ... ], - ratings: [ + Ratings: [ ... ],If this is intentional (e.g., for i18n keys), please add a comment explaining the convention.
src/components/itinevert/ItinevertBanner.vue (1)
5-5: Multiple TODO placeholders remain in production code.This component contains several TODO comments indicating incomplete implementation:
- Line 5:
"todo : texte descriptif"- Lines 12-22: Multiple placeholder router-links with
"todo : des liens vers d'autres contenus"These should be addressed before merging or tracked in a follow-up issue.
Would you like me to open an issue to track completing this banner component?
Also applies to: 12-22
src/translations/fr.json (1)
36-36: Minor plural inconsistency.English key
"Access points"(plural) is translated to French"Point d'accès"(singular). Consider using"Points d'accès"for consistency.- "Access points": "Point d'accès", + "Access points": "Points d'accès",src/components/generics/inputs/InputAutocomplete.vue (1)
65-91: UndeclaredsearchTimeoutproperty.
this.searchTimeoutis used for debouncing but is never declared indata(). This works but is not idiomatic Vue—instance properties should be declared.Add
searchTimeout: nulltodata()or initialize it in acreated()hook:data() { return { + searchTimeout: null, localData: { ... }, }; },src/js/apis/itinevert-service.js (4)
200-212: Side effect in getter methods.
getJourneyReachableWaypointsandgetJourneyReachableRoutesresetthis.jobID = nullas a side effect. This couples reading results with clearing state, which could cause issues if the result needs to be fetched again (e.g., on network error retry).Consider separating job lifecycle management from result retrieval, or letting the caller explicitly clear the job.
336-340: Use saferObject.hasOwn()instead ofhasOwnProperty.
newQuery.hasOwnProperty(key)can fail ifnewQueryhas a property namedhasOwnPropertyor if it's created withObject.create(null).- if (newQuery.hasOwnProperty(key) && newQuery[key]) { + if (Object.hasOwn(newQuery, key) && newQuery[key]) {
468-476: Unreachable code block.The check at lines 469-476 for
Array.isArray(initialVal) && typeof initialVal[0] === 'number'is unreachable because arrays are already handled at line 455-461.// Compare for boolean if (typeof initialVal === 'boolean') { return initialVal === fieldValue; } - // Compare for numerical ranges - if (Array.isArray(initialVal) && typeof initialVal[0] === 'number') { - return ( - Array.isArray(fieldValue) && - fieldValue.length === 2 && - initialVal[0] === fieldValue[0] && - initialVal[1] === fieldValue[1] - ); - } - // General compare for strings return initialVal === fieldValue;
82-126: Consider extracting duplicatedCancelablePromiselogic.
getAllReachableRoutesandgetAllReachableWaypointscontain nearly identical implementations with inlineCancelablePromiseclass definitions.Extract the common pattern into a shared helper to improve maintainability and reduce duplication.
Also applies to: 154-198
src/js/vue-plugins/router.js (1)
19-20: Route works, but consider normalizing path casing and/or lazy‑loadingThe Itinevert route is correctly wired, but you may want to:
- Use a lowercase path (e.g.
/itinevert) for consistency with existing routes.- Optionally lazy‑load
ItinevertViewlike other portal views to keep the initial bundle smaller.Example for the path:
- { path: '/Itinevert', name: 'Itinevert', component: ItinevertView }, + { path: '/itinevert', name: 'Itinevert', component: ItinevertView },(adjust any hardcoded URLs accordingly if you change it).
Also applies to: 56-56
src/views/SideMenu.vue (1)
62-68: Anchor‑based Itinevert link is fine; confirm icon registration and reload trade‑offUsing a plain
<a>with$router.resolvematches the intent to force a full refresh, but note:
- This will always reload the whole SPA, even when navigating from another route (performance trade‑off versus a
<router-link>).<icon-itinevert>must be globally registered (unlikeIconYeti, which is imported locally) or the menu will show an unknown element warning.If the only requirement is “refresh when already on Itinevert”, you could alternatively keep
<router-link>and add a click handler that forces a reload when$route.name === 'Itinevert'.src/components/generics/icons/IconItinevert.vue (1)
1-14: Icon wrapper is straightforward and consistentThe
IconItinevertwrapper around<fa-icon>with afixedWidthBoolean prop looks good and matches typical icon component patterns. Optionally addname: 'IconItinevert'for clearer component traces.src/components/itinevert/ItinevertPageSelector.vue (1)
68-72: Consider initializingtotalfromdocumentsprop.The hardcoded initial value of
30fortotalcould cause a brief display glitch before the watcher updates it. Consider initializing from the prop if available.data() { return { - total: 30, + total: this.documents?.total ?? 0, }; },src/components/itinevert/ItinevertWizard.vue (3)
196-202: DuplicatecategoryIcondefinition.This
categoryIconobject is also defined inItinevertFilterView.vue(lines 74-80). Consider extracting to a shared constant to maintain consistency.// In a shared constants file export const itinevertCategoryIcon = { General: 'filter', MultiSearch: 'filter', Miscs: 'database', Terrain: ['waypoint', 'summit'], ratings: 'tachometer-alt', };
310-318:noResultsFounduseshasOwnPropertyinconsistently with optional chaining.The code mixes
hasOwnPropertychecks with optional chaining, which is inconsistent. Also, usingObject.prototype.hasOwnProperty.callwould be safer.noResultsFound() { if (this.formData?.searchKind?.selected === 'route') { - return !this.filteredRoutes.hasOwnProperty('documents') || this.filteredRoutes?.documents?.length === 0; + return !this.filteredRoutes?.documents?.length; } else if (this.formData?.searchKind?.selected === 'waypoint') { - return !this.filteredWaypoints.hasOwnProperty('documents') || this.filteredWaypoints?.documents?.length === 0; + return !this.filteredWaypoints?.documents?.length; } else { return false; } },
128-132: Search button validation uses method call in template.Calling
isSearchEnabled()as a method in the template means it runs on every render. Consider making it a computed property for better performance and caching.+ computed: { + // ... existing computed properties ... + isSearchEnabled() { + if (this.formData.searchKind.selected === 'route' && this.formData.activities.length < 1) { + return false; + } + if ( + this.formData.destinationKind.selected === 'mountain range' && + this.formData.mountainRange.selected === null + ) { + return false; + } + if (this.formData.startingPoint.selectedAddress === null || this.formData.startingPoint.selectedAddress === '') { + return false; + } + return true; + }, + }, methods: { - isSearchEnabled() { ... },Then update template:
- <button class="button is-primary" :disabled="!isSearchEnabled()" @click="formSearch"> + <button class="button is-primary" :disabled="!isSearchEnabled" @click="formSearch">src/components/generics/inputs/InputAddress.vue (5)
86-89: Misleading comment: no date picker in this component.The comment references "Firefox's date picker calendar" but this component doesn't contain a date picker. This appears to be copy-pasted from another component. Consider removing this code if not needed, or updating the comment to explain its actual purpose.
96-126: Inconsistent API usage and missing internationalization.
This method uses
fetch()directly whilesearchAddressPropositionsuses thephotonAPI wrapper. Consider usingphotonfor consistency and centralized error handling.Alert messages are hardcoded in English instead of using
$gettextfor internationalization:(error) => { console.error('Geolocation error:', error); - alert('Unable to get your current location.'); + alert(this.$gettext('Unable to get your current location.')); } ); } else { - alert('Geolocation is not supported by your browser.'); + alert(this.$gettext('Geolocation is not supported by your browser.')); }
179-181: Simplify string concatenation.The template literal
${' '}is unnecessarily verbose.if (props.street) { - formattedAddress = `${formattedAddress}${' '}${props.street}`; + formattedAddress = `${formattedAddress} ${props.street}`; }
198-242: Consider using the photon API wrapper for consistency.Both
reverseGeocodeUserLocationanduseCurrentLocationmake directfetchcalls tophoton.komoot.io, whilesearchAddressPropositionsuses thephotonAPI wrapper. For maintainability and consistent error handling, consider adding a reverse geocoding method to the photon API module.
29-31: Improve button accessibility.The geolocation button lacks
type="button"(defaults to "submit" in forms) and anaria-labelfor screen readers.- <button class="geolocalisation" @click="useCurrentLocation"> + <button type="button" class="geolocalisation" @click="useCurrentLocation" :aria-label="$gettext('Use current location')"> <img class="geolocalisation-img" src="@/assets/img/boxes/geoloc.svg" alt="geoloc" /> </button>src/components/itinevert/ItinevertResultView.vue (3)
388-388: Typo in comment."IDsw" should be "IDs".
- // filter only matching old IDsw + // filter only matching old IDs
255-257: Typo in method name:toogleProperty→toggleProperty.The method name has a spelling error.
- toogleProperty(property) { + toggleProperty(property) { this.setProperty(property, !this[property]); },Also update the template reference on line 20:
- <span @click="toogleProperty('listMode')" ...> + <span @click="toggleProperty('listMode')" ...>
231-235: Unnecessaryasynckeyword in watch handler.The handler doesn't use
await, soasyncis not needed.'$route.query': { - async handler(newQuery) { + handler(newQuery) { this.filterPostNavitiaDocuments(newQuery); }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/img/itinevert/itinevert-banner.pngis excluded by!**/*.png
📒 Files selected for processing (27)
src/components/datatable/DocumentsTable.vue(10 hunks)src/components/datatable/cell-renderers/DocumentLinkNewTab.vue(1 hunks)src/components/generics/icons/IconItinevert.vue(1 hunks)src/components/generics/inputs/InputAddress.vue(1 hunks)src/components/generics/inputs/InputAutocomplete.vue(1 hunks)src/components/generics/inputs/InputRadioButton.vue(1 hunks)src/components/itinevert/ItinevertBanner.vue(1 hunks)src/components/itinevert/ItinevertFilterItem.vue(1 hunks)src/components/itinevert/ItinevertFilterView.vue(1 hunks)src/components/itinevert/ItinevertLoadingView.vue(1 hunks)src/components/itinevert/ItinevertNoResultView.vue(1 hunks)src/components/itinevert/ItinevertPageSelector.vue(1 hunks)src/components/itinevert/ItinevertResultView.vue(1 hunks)src/components/itinevert/ItinevertWizard.vue(1 hunks)src/components/map/OlMap.vue(4 hunks)src/components/map/map-utils.js(1 hunks)src/js/apis/itinevert-service.js(1 hunks)src/js/apis/photon.js(1 hunks)src/js/apis/transport-service.js(1 hunks)src/js/constants/index.js(1 hunks)src/js/vue-plugins/font-awesome-config.js(2 hunks)src/js/vue-plugins/router.js(2 hunks)src/translations/fr.json(21 hunks)src/views/SideMenu.vue(1 hunks)src/views/documents/utils/QueryItems.vue(4 hunks)src/views/documents/utils/url-mixin.js(1 hunks)src/views/portals/ItinevertView.vue(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-02T19:55:04.817Z
Learnt from: flob38
Repo: c2corg/c2c_ui PR: 4382
File: src/views/DfmAdLarge.vue:17-19
Timestamp: 2025-08-02T19:55:04.817Z
Learning: The DfmAdLarge component in src/views/DfmAdLarge.vue is only used on large screens (width >= 1216px), so fixed min-width values are appropriate and won't cause layout issues on smaller viewports.
Applied to files:
src/components/itinevert/ItinevertWizard.vuesrc/components/itinevert/ItinevertResultView.vue
🪛 GitHub Actions: Continuous integration
src/components/itinevert/ItinevertWizard.vue
[error] 781-781: stylelint: declaration-block-no-duplicate-properties: Unexpected duplicate 'width' declaration.
🔇 Additional comments (20)
src/js/vue-plugins/font-awesome-config.js (1)
9-9: LGTM!The new
faFaceFrownRegularicon is properly imported and registered following the existing patterns.Also applies to: 367-367
src/js/apis/transport-service.js (1)
21-29: LGTM!The new
getWaypointsAccessibleTCmethod follows the existing pattern used byisReachable, properly documented and consistent with the service's style.src/components/itinevert/ItinevertBanner.vue (1)
58-69: Counterintuitive responsive logo sizing.The logo is set to
max-width: 20%on desktop but shrinks tomax-width: 10%on tablet. Typically, relative sizing increases on smaller screens for better visibility. Was this intentional?src/js/apis/itinevert-service.js (1)
28-28: Constant appears to represent runtime state.
MAX_NAVITIA_ISOCHRONES_REQUEST_REACHEDis exported as a constant with valuefalse, but semantically it seems like it should track whether a limit has been reached at runtime. If this is intentional configuration, consider renaming for clarity (e.g.,NAVITIA_ISOCHRONES_REQUEST_LIMIT_ENABLED).src/js/apis/photon.js (1)
16-24: Safer handling of optional center coordinates looks correctGuarding
centerWgs84before indexing avoids crashes when it’s not provided, without changing behavior when it is. No further changes needed here.src/views/documents/utils/QueryItems.vue (1)
68-80: Configurable print button and centralized categorized fields look good
showPrintButtoncleanly controls both inline and dropdownPrintButtonvisibility and defaults totrue, so existing behavior is preserved.- Reusing
constants.categorizedFieldsDefaultfor all document types keeps the categorization centralized; this is fine as long as those defaults cover all current types.No issues spotted with these changes.
Also applies to: 101-112, 135-143
src/components/datatable/cell-renderers/DocumentLinkNewTab.vue (1)
1-9: Thedocument-linkcomponent is registered globally via thegenericComponentsplugin (src/js/vue-plugins/generic-components.js), which automatically registers all components from@/components/genericswith kebab-case naming. This component works as intended and follows the established pattern used by other cell-renderers in the same directory (e.g.,DocumentAuthor.vue,AreaList.vue), which similarly depend on globally-registered components without local imports or explicitnameproperties.src/components/map/OlMap.vue (3)
854-884: LGTM! Area document polygon support is well-structured.The conditional handling for area documents (
type === 'a') correctly renders filled polygons fromgeom_detailusing the newgetDocumentPolygonStyle. The logic appropriately separates area documents from other types that use point/line rendering.One minor note: area features don't have their ID set (unlike non-area features at line 874), which may affect ID-based lookups if needed in the future.
269-273: LGTM!The
openInNewTabprop is properly defined with appropriate type and default value.
1141-1141: LGTM!The click handler correctly uses the
openInNewTabprop as an alternative to Ctrl-click for opening documents in new tabs.src/components/datatable/DocumentsTable.vue (3)
70-73: LGTM!The
openInNewTabprop is properly defined.
82-86: Column definitions won't update ifopenInNewTabchanges at runtime.The watcher only triggers
computeColumnDefswhendocumentTypechanges. IfopenInNewTabcan change while the component is mounted, the column definitions won't reflect the new value.If
openInNewTabis expected to change at runtime, add it to the watch:watch: { documentType: { handler: 'computeColumnDefs', immediate: true, }, + openInNewTab: 'computeColumnDefs', highlightedDocument(newValue, oldValue) {
130-133: LGTM!The conditional renderer pattern is applied consistently across all document types.
src/components/itinevert/ItinevertLoadingView.vue (1)
1-47: LGTM!Well-implemented loading component with:
- Proper accessibility via ARIA attributes on the progress bar
- Safe division-by-zero handling in
computedPercent- Clean separation between spinner and progress bar modes
- Smooth CSS transitions for progress updates
src/components/itinevert/ItinevertPageSelector.vue (1)
99-101: Unused methodhideOnclick.The
hideOnclickmethod is only called fromselectLimit, so it could be inlined. However, this is a minor style preference.src/components/itinevert/ItinevertFilterView.vue (2)
139-141: Edge case:canDisplayResultreturnsfalsewhentotalis0but also whenundefined.With the current logic and the potential
undefinedfromfilteredRoutes.total, the button will be disabled correctly, but for clarity the null-safe access would be better.
74-80: Using$optionsfor static data is a valid Vue 2 pattern.The
categoryIconmapping as a component option is correctly accessed via$options.categoryIconin the template. This is appropriate for static configuration that doesn't need reactivity.src/views/portals/ItinevertView.vue (1)
49-60: LGTM!The conditional initial state based on user login status is a reasonable UX decision - logged-in users likely don't need the banner repeatedly.
src/components/itinevert/ItinevertWizard.vue (1)
373-380: Unconditional URL parameter clearing on mount may cause issues.Clearing URL query parameters on mount could disrupt deep linking or page refresh scenarios where users expect their search state to be preserved.
Is this intentional? If users should be able to bookmark or share Itinevert URLs with parameters, this behavior would break that use case.
src/components/itinevert/ItinevertResultView.vue (1)
93-101: LGTM: Document table and map integration.The integration of
DocumentsTableandmap-viewwith highlighting and new-tab opening is well implemented. Theopen-in-new-tabprop propagation is consistent across both views.
| :value="option.value" | ||
| v-model="selected" | ||
| @change="emitSelectedValue" | ||
| :disabled="disabledOptions.map((option) => option.value).includes(option.value)" |
There was a problem hiding this comment.
Optimize disabled check with a computed Set.
The disabled check creates a new array via .map() and searches it for every option on every render. With many options, this becomes expensive.
Create a computed property for disabled values:
computed: {
+ disabledValues() {
+ return new Set(this.disabledOptions.map((option) => option.value));
+ },
reasons() {Then update the template:
<input
type="radio"
:value="option.value"
v-model="selected"
@change="emitSelectedValue"
- :disabled="disabledOptions.map((option) => option.value).includes(option.value)"
+ :disabled="disabledValues.has(option.value)"
/>🤖 Prompt for AI Agents
In src/components/generics/inputs/InputRadioButton.vue around line 9, the
template builds a new array via disabledOptions.map(...) and calls
.includes(...) for every option on each render; instead create a computed Set of
disabled values (e.g., disabledValues = computed(() => new Set((disabledOptions
|| []).map(o => o.value)))) and then change the template to check membership
with disabledValues.has(option.value); also guard against undefined
disabledOptions when building the Set so the computed is safe.
There was a problem hiding this comment.
@Nayor, thank you! The computed Set approach will improve performance, especially with larger option lists. 👍
| props: { | ||
| options: { | ||
| type: Array, | ||
| required: true, | ||
| }, | ||
| disabledOptions: { | ||
| type: Array, | ||
| default: () => [], | ||
| }, | ||
| defaultValue: { | ||
| type: String, | ||
| default: '', | ||
| }, | ||
| displayVertically: { | ||
| type: Boolean, | ||
| default: false, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Missing value prop breaks v-model.
The component references this.value on line 41 and watches value on lines 54-56, but doesn't declare a value prop. For Vue 2 v-model to work, the component must accept a value prop.
Apply this diff to add the missing prop:
props: {
+ value: {
+ type: String,
+ default: '',
+ },
options: {
type: Array,
required: true,
},🤖 Prompt for AI Agents
In src/components/generics/inputs/InputRadioButton.vue around lines 21 to 38,
the component is missing a declared value prop which breaks v-model usage; add a
value prop to props (type: [String, Number, Boolean, Object] or at least String)
with a suitable default (e.g. empty string) so the component accepts the v-model
value, matching the existing references to this.value and the value watcher.
| data() { | ||
| return { | ||
| selected: this.value || this.defaultValue, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Emit initial value to parent.
When defaultValue is used, the parent component won't receive the initial selected value, causing potential state desynchronization.
Add a mounted hook to emit the initial value:
watch: {
value(newValue) {
this.selected = newValue;
},
},
+ mounted() {
+ if (this.selected) {
+ this.$emit('input', this.selected);
+ }
+ },
methods: {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/generics/inputs/InputRadioButton.vue around lines 39-43, the
component sets selected from this.value || this.defaultValue but never notifies
the parent when defaultValue is used; add a mounted() hook that checks if
this.value is undefined (or null) and this.defaultValue is defined then emits
the initial selected value to the parent (e.g., this.$emit('input',
this.selected) or this.$emit('update:value', this.selected) consistent with the
component's existing event naming), so the parent receives the initial state and
stays synchronized.
| <div v-else-if="field.queryMode === 'date'"> | ||
| <date-query-item /> | ||
| </div> | ||
|
|
||
| <div v-else-if="field.queryMode === 'dates'"> | ||
| <dates-query-item /> | ||
| </div> |
There was a problem hiding this comment.
Missing v-model bindings for date query items.
DateQueryItem and DatesQueryItem are rendered without any v-model or event bindings, so their selected values won't propagate to the parent. If these are meant to be functional, they need value binding similar to other query modes.
<div v-else-if="field.queryMode === 'date'">
- <date-query-item />
+ <date-query-item v-model="selected" :field="field" />
</div>
<div v-else-if="field.queryMode === 'dates'">
- <dates-query-item />
+ <dates-query-item v-model="selected" :field="field" />
</div>📝 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.
| <div v-else-if="field.queryMode === 'date'"> | |
| <date-query-item /> | |
| </div> | |
| <div v-else-if="field.queryMode === 'dates'"> | |
| <dates-query-item /> | |
| </div> | |
| <div v-else-if="field.queryMode === 'date'"> | |
| <date-query-item v-model="selected" :field="field" /> | |
| </div> | |
| <div v-else-if="field.queryMode === 'dates'"> | |
| <dates-query-item v-model="selected" :field="field" /> | |
| </div> |
🤖 Prompt for AI Agents
In src/components/itinevert/ItinevertFilterItem.vue around lines 70 to 76, the
DateQueryItem and DatesQueryItem are rendered without model bindings so their
selected values never reach the parent; add the same v-model wiring used by
other query modes (e.g. v-model="field.value") when rendering these components
and ensure the child components use/emit the standard update:modelValue event so
the parent field.value is kept in sync.
| oninput: debounce(function () { | ||
| this.selected = this.$refs.input.value; | ||
| }, 300), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dead code: oninput method is never used.
The debounced oninput method is defined but never bound to any element in the template. The input element on line 46 uses v-model directly instead.
Either remove this dead code or wire it to the text input if debouncing is intended:
- oninput: debounce(function () {
- this.selected = this.$refs.input.value;
- }, 300),Or, if debouncing is needed for the input field, bind it:
- <input ref="input" :type="field.type" v-model="selected" class="input" :placeholder="$gettext(field.name)" />
+ <input ref="input" :type="field.type" :value="selected" @input="oninput" class="input" :placeholder="$gettext(field.name)" />📝 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.
| oninput: debounce(function () { | |
| this.selected = this.$refs.input.value; | |
| }, 300), |
| mounted() { | ||
| // init filtered documents | ||
| this.filteredDocumentsBeforePagination = { | ||
| documents: this.documents.documents, | ||
| total: this.documents.total, | ||
| }; | ||
| this.filteredDocuments = { | ||
| documents: this.documents.documents.slice(this.offset, this.offset + this.limit), | ||
| total: this.documents.total, | ||
| }; | ||
| this.promise = { | ||
| data: this.filteredDocuments, | ||
| error: null, | ||
| cancel: () => {}, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Potential null reference error in mounted hook.
If documents prop is null (its default), accessing this.documents.documents and this.documents.total on lines 241-246 will throw an error.
mounted() {
// init filtered documents
+ if (!this.documents) {
+ this.filteredDocuments = { documents: [], total: 0 };
+ this.filteredDocumentsBeforePagination = { documents: [], total: 0 };
+ this.promise = { data: this.filteredDocuments, error: null, cancel: () => {} };
+ return;
+ }
this.filteredDocumentsBeforePagination = {
documents: this.documents.documents,
total: this.documents.total,
};🤖 Prompt for AI Agents
In src/components/itinevert/ItinevertResultView.vue around lines 238 to 253, the
mounted hook assumes this.documents is non-null and directly accesses
this.documents.documents and this.documents.total which will throw if the prop
is null; guard these accesses by using safe defaults (e.g. const docs =
this.documents?.documents ?? [] and const total = this.documents?.total ?? 0),
use docs.slice(this.offset, this.offset + this.limit) when building
filteredDocuments, and set the promise.data to the safe filteredDocuments result
so the component works when documents is null.
| } else { | ||
| this.queryError = JSON.parse(data); | ||
| this.filteredRoutes = {}; | ||
| this.filteredWaypoints = {}; | ||
| } |
There was a problem hiding this comment.
Unsafe JSON.parse on potentially non-JSON data.
If data doesn't have documents property, the code assumes it's a JSON string and calls JSON.parse(data). This could throw a SyntaxError if the response is not valid JSON.
} else {
- this.queryError = JSON.parse(data);
+ try {
+ this.queryError = typeof data === 'string' ? JSON.parse(data) : data;
+ } catch (e) {
+ this.queryError = { message: 'Unknown error occurred' };
+ }
this.filteredRoutes = {};
this.filteredWaypoints = {};
}The same pattern appears at lines 562 and 592 - consider applying similar defensive handling.
📝 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.
| } else { | |
| this.queryError = JSON.parse(data); | |
| this.filteredRoutes = {}; | |
| this.filteredWaypoints = {}; | |
| } | |
| } else { | |
| try { | |
| this.queryError = typeof data === 'string' ? JSON.parse(data) : data; | |
| } catch (e) { | |
| this.queryError = { message: 'Unknown error occurred' }; | |
| } | |
| this.filteredRoutes = {}; | |
| this.filteredWaypoints = {}; | |
| } |
🤖 Prompt for AI Agents
In src/components/itinevert/ItinevertWizard.vue around lines 523-527 (and
similarly at ~562 and ~592), the code calls JSON.parse(data) on the assumption
that non-`documents` responses are valid JSON, which can throw a SyntaxError;
change this to defensively parse: first check if data is a string, then wrap
JSON.parse in try/catch (or use a safe JSON parse helper) and on parse failure
set this.queryError to a safe fallback (e.g., the raw string or a structured
error message) and clear filteredRoutes/filteredWaypoints as currently done;
apply the same defensive parsing pattern at the other two locations to prevent
runtime exceptions.
There was a problem hiding this comment.
Vérifier que le json a bien la bonne forme avant de le parser et/ou capturer l'erreur de parsing
There was a problem hiding this comment.
@Nayor, thank you for confirming! Yes, the suggestion I provided includes both safeguards:
- Type checking before parsing (
typeof data === 'string') - Try/catch block to handle any parsing errors gracefully
As mentioned in my initial comment, this same defensive pattern should be applied at all three locations where JSON.parse is called on potentially unsafe data:
- Line ~523 (in the isochrones flow)
- Line ~562 (in the
computeJourneyReachableRouteserror handler) - Line ~592 (in the
filterSearcherror handler)
This will prevent runtime SyntaxError exceptions if the response isn't valid JSON.
| function ItinevertService() { | ||
| this.axios = axios.create({ | ||
| baseURL: config.urls.api, | ||
| }); | ||
| this.jobID = null; | ||
| } |
There was a problem hiding this comment.
Singleton with mutable jobID may cause concurrency issues.
The service is exported as a singleton (new ItinevertService()), but this.jobID is mutable instance state. If multiple Itinevert searches run concurrently, they will overwrite each other's job IDs.
Consider either:
- Returning
jobIDfrom start methods and passing it to progress/result methods - Using a Map to track multiple jobs
There was a problem hiding this comment.
Bon travail !
Quelques remarques de finition + les commentaires de rabbitai à corriger, mais rien de structurel pour moi 👍
Ma seule interrogation se porte sur le fait que les filtres (côté back) ne se reposent plus sur elastic pour faire la recherche (la recherche par dictionnaire c'est vraiment pas terrible, et on a une evol elastic de prévu très bientôt pour corriger ça)
| }, | ||
| placeholder: { | ||
| type: String, | ||
| default: 'Entrez une adresse', |
There was a problem hiding this comment.
Utiliser l'internationalisation (i18n)
| this.localData.coordinates = coords; | ||
|
|
||
| try { | ||
| const response = await fetch( |
There was a problem hiding this comment.
à mettre dans une fonction, dans photon.js
| if (data.features && data.features.length > 0) { | ||
| const location = data.features[0]; | ||
| this.selectAddress(location); | ||
| } |
There was a problem hiding this comment.
manque un fallback si aucune adresse correspondante n'est trouvé (mettre les coordonées par exemple)
| this.selectAddress(location); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error during reverse geolocation:', error); |
There was a problem hiding this comment.
C'est un peu excessif le console.error non ? étant donné que ça n'est pas un composant central de l'application
.warn c'est bien
| }, | ||
| (error) => { | ||
| console.error('Geolocation error:', error); | ||
| alert('Unable to get your current location.'); |
There was a problem hiding this comment.
utiliser toast avec un type is-warning plutôt
| <section> | ||
| <div class="columns"> | ||
| <div class="column is-12-mobile"> | ||
| <div class="subtitle">todo : texte descriptif</div> |
| <span slot="button" class="button is-size-7-mobile" :disabled="category.fields.length === 0"> | ||
| <fa-icon :icon="$options.categoryIcon[category.name]" /> | ||
| <span class="is-hidden-mobile"> | ||
| <!-- $gettext('General') --> |
| </script> | ||
|
|
||
| <style scoped lang="scss"> | ||
| $section-padding: 1.5rem; //TODO find this variable |
| <style scoped lang="scss"> | ||
| $section-padding: 1.5rem; //TODO find this variable | ||
| $header-height: 34px; | ||
| $header-padding-bottom: 1.5rem; //TODO find this variable |
| } else { | ||
| this.queryError = JSON.parse(data); | ||
| this.filteredRoutes = {}; | ||
| this.filteredWaypoints = {}; | ||
| } |
There was a problem hiding this comment.
Vérifier que le json a bien la bonne forme avant de le parser et/ou capturer l'erreur de parsing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.