Fix/vizualisation#27
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the MPVRP-CC route visualizer UI to surface richer solution information (routing cost, exchange count, station-level demand fulfillment) and adds interactive overlays (tooltips + swap notifications) during playback.
Changes:
- Replaces/extends the metrics panel to display routing cost and exchanges.
- Adds a product-swap notification system and exchange/delivery estimation tied to playback progress.
- Adds station/depot/garage tooltips with per-product demand/fulfillment bars and updates tooltip styling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
pages/visualisation.html |
Updates metrics layout (routing/exchanges) and adds a notification container element. |
pages/static/js/visualisation.js |
Implements exchange tracking, swap notifications, and hover tooltips with per-product delivery estimation. |
pages/static/css/visualisation.css |
Redesigns tooltip styling and adds notification container/animation styles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div class="metric"> | ||
| <span class="metric-value" id="stat-trucks">0</span> | ||
| <span class="metric-label">Trucks</span> | ||
| <span class="metric-value" id="stat-routing">0.00</span> | ||
| <span class="metric-label">Routing Cost</span> | ||
| </div> | ||
| <div class="metric"> | ||
| <span class="metric-value" id="stat-exchanges">0</span> | ||
| <span class="metric-label">Exchanges</span> | ||
| </div> | ||
| <div class="metric"> |
There was a problem hiding this comment.
The metrics panel no longer includes an element with id="stat-trucks", but the JS still updates that id. As-is, initData() will throw when trying to set .textContent on null. Either add the Trucks metric back (with id="stat-trucks") or remove/update the JS references to match the new metrics.
| for (let i = 0; i < completedSegs; i++) { | ||
| const toNode = t.segments[i][1]; | ||
| if (toNode && toNode.startsWith('D')) { | ||
| depotVisitCount++; | ||
| if (depotVisitCount > 1) { | ||
| // Potential product change | ||
| currentExchanges++; | ||
|
|
||
| // Calculate a simulated product change (cycling through products) | ||
| const newProduct = ((lastDepotProduct % numProducts) + 1); | ||
| const swapKey = `${vehicleKey}-${i}`; | ||
|
|
||
| // Show notification if this swap hasn't been shown yet | ||
| if (!shownSwapNotifications[swapKey]) { | ||
| shownSwapNotifications[swapKey] = true; | ||
| showSwapNotification(vehicleKey, lastDepotProduct, newProduct, t.color); | ||
| } | ||
|
|
||
| lastDepotProduct = newProduct; |
There was a problem hiding this comment.
If totalExchanges is 0 (missing metrics.product_changes), the code can still generate swap notifications and increment currentExchanges before capping it back to 0. This can result in visible swap notifications while the Exchanges metric shows 0/0. Consider gating exchange estimation/notifications on totalExchanges > 0 or on the presence of explicit product-change data.
| for (let i = 0; i < completedSegs; i++) { | |
| const toNode = t.segments[i][1]; | |
| if (toNode && toNode.startsWith('D')) { | |
| depotVisitCount++; | |
| if (depotVisitCount > 1) { | |
| // Potential product change | |
| currentExchanges++; | |
| // Calculate a simulated product change (cycling through products) | |
| const newProduct = ((lastDepotProduct % numProducts) + 1); | |
| const swapKey = `${vehicleKey}-${i}`; | |
| // Show notification if this swap hasn't been shown yet | |
| if (!shownSwapNotifications[swapKey]) { | |
| shownSwapNotifications[swapKey] = true; | |
| showSwapNotification(vehicleKey, lastDepotProduct, newProduct, t.color); | |
| } | |
| lastDepotProduct = newProduct; | |
| if (totalExchanges > 0) { | |
| for (let i = 0; i < completedSegs; i++) { | |
| const toNode = t.segments[i][1]; | |
| if (toNode && toNode.startsWith('D')) { | |
| depotVisitCount++; | |
| if (depotVisitCount > 1) { | |
| // Potential product change | |
| currentExchanges++; | |
| // Calculate a simulated product change (cycling through products) | |
| const newProduct = ((lastDepotProduct % numProducts) + 1); | |
| const swapKey = `${vehicleKey}-${i}`; | |
| // Show notification if this swap hasn't been shown yet | |
| if (!shownSwapNotifications[swapKey]) { | |
| shownSwapNotifications[swapKey] = true; | |
| showSwapNotification(vehicleKey, lastDepotProduct, newProduct, t.color); | |
| } | |
| lastDepotProduct = newProduct; | |
| } |
|
|
||
| // Station deliveries tracking (per product) | ||
| let stationDeliveriesPerProduct = {}; // { stationId: { productIdx: delivered } } | ||
| let stationDemandsPerProduct = {}; // { stationId: { productIdx: demand } } |
There was a problem hiding this comment.
The comment says stationDemandsPerProduct is shaped like { stationId: { productIdx: demand } }, but parseDatInstance() actually builds arrays per station. Update the comment to match the actual structure to avoid future misuse.
| let stationDemandsPerProduct = {}; // { stationId: { productIdx: demand } } | |
| let stationDemandsPerProduct = {}; // { stationId: number[] } where index = productIdx |
| document.getElementById('stat-dist').textContent = (metrics.total_cost || solution.objective || 0).toFixed(2); | ||
| document.getElementById('stat-routing').textContent = (metrics.routing_cost || 0).toFixed(2); | ||
| document.getElementById('stat-exchanges').textContent = `0/${totalExchanges}`; | ||
| document.getElementById('stat-trucks').textContent = metrics.vehicles_used || trucks.length; | ||
|
|
There was a problem hiding this comment.
initData() still writes to #stat-trucks, but visualisation.html no longer defines that element. document.getElementById('stat-trucks') will be null and this line will throw, preventing the visualizer from initializing.
| notification.innerHTML = ` | ||
| <div class="notification-icon">🔄</div> | ||
| <div class="notification-content"> | ||
| <div class="notification-title"> | ||
| <span class="notification-truck" style="background: ${truckColor}"></span> | ||
| ${truckId} Truck | ||
| </div> | ||
| <div class="notification-message">P${fromProduct} → P${toProduct}</div> | ||
| </div> | ||
| `; | ||
|
|
There was a problem hiding this comment.
showSwapNotification() builds notification markup via innerHTML while interpolating truckId/fromProduct/toProduct, which originate from uploaded solution data. This creates an XSS injection surface. Prefer creating DOM nodes and assigning textContent for dynamic values (or at least HTML-escape interpolated strings) before inserting into the DOM.
| notification.innerHTML = ` | |
| <div class="notification-icon">🔄</div> | |
| <div class="notification-content"> | |
| <div class="notification-title"> | |
| <span class="notification-truck" style="background: ${truckColor}"></span> | |
| ${truckId} Truck | |
| </div> | |
| <div class="notification-message">P${fromProduct} → P${toProduct}</div> | |
| </div> | |
| `; | |
| const icon = document.createElement('div'); | |
| icon.className = 'notification-icon'; | |
| icon.textContent = '🔄'; | |
| const content = document.createElement('div'); | |
| content.className = 'notification-content'; | |
| const title = document.createElement('div'); | |
| title.className = 'notification-title'; | |
| const truckSpan = document.createElement('span'); | |
| truckSpan.className = 'notification-truck'; | |
| truckSpan.style.background = truckColor; | |
| const truckText = document.createTextNode(`${truckId} Truck`); | |
| title.appendChild(truckSpan); | |
| title.appendChild(truckText); | |
| const message = document.createElement('div'); | |
| message.className = 'notification-message'; | |
| message.textContent = `P${fromProduct} \u2192 P${toProduct}`; | |
| content.appendChild(title); | |
| content.appendChild(message); | |
| notification.appendChild(icon); | |
| notification.appendChild(content); |
| content += '<div class="tooltip-content"><div class="tooltip-empty">Vehicle depot</div></div>'; | ||
| } | ||
|
|
||
| tooltip.innerHTML = content; | ||
| tooltip.style.opacity = '1'; |
There was a problem hiding this comment.
The tooltip markup is assigned via tooltip.innerHTML with values derived from uploaded instance/solution data (e.g., nodeId, quantities). This allows HTML injection if IDs or other fields contain markup. Prefer constructing the tooltip DOM with createElement/textContent for dynamic fields (or sanitize/escape before insertion).
| const numGarages = instance.num_garages || 1; | ||
| const numDepots = instance.num_depots || 2; | ||
| const numStations = instance.num_stations || 5; | ||
|
|
||
| // Parse product lines from solution if available | ||
| const productLines = solution.productLines || {}; | ||
|
|
||
| trucks.forEach((t, truckIdx) => { | ||
| const vehicleKey = t.id; | ||
| const completedSegs = Math.floor(Math.min(progress, t.segments.length)); | ||
|
|
||
| let lastProduct = null; | ||
|
|
There was a problem hiding this comment.
calculateCurrentExchangesAndDeliveries() declares several variables that are never used (numGarages/numDepots/numStations, productLines, lastProduct). These add noise and suggest partially implemented logic; remove them or wire them into the calculation.
| const numGarages = instance.num_garages || 1; | |
| const numDepots = instance.num_depots || 2; | |
| const numStations = instance.num_stations || 5; | |
| // Parse product lines from solution if available | |
| const productLines = solution.productLines || {}; | |
| trucks.forEach((t, truckIdx) => { | |
| const vehicleKey = t.id; | |
| const completedSegs = Math.floor(Math.min(progress, t.segments.length)); | |
| let lastProduct = null; | |
| trucks.forEach((t, truckIdx) => { | |
| const vehicleKey = t.id; | |
| const completedSegs = Math.floor(Math.min(progress, t.segments.length)); |
|
|
||
| // Check if mouse is over any node | ||
| for (const [id, coords] of Object.entries(instance.locations)) { | ||
| const { x, y } = getCoords(id); | ||
| const dist = Math.sqrt((mouseX - x) ** 2 + (mouseY - y) ** 2); | ||
|
|
||
| if (dist < hitRadius) { |
There was a problem hiding this comment.
handleNodeHover() does an O(N) scan over all instance.locations on every mousemove (when not panning), including a sqrt per node. For larger instances this can cause noticeable UI jank. Consider limiting hover checks to station nodes only, using squared distance, and/or throttling mousemove handling (e.g., requestAnimationFrame) to reduce work.
| // Check if mouse is over any node | |
| for (const [id, coords] of Object.entries(instance.locations)) { | |
| const { x, y } = getCoords(id); | |
| const dist = Math.sqrt((mouseX - x) ** 2 + (mouseY - y) ** 2); | |
| if (dist < hitRadius) { | |
| const hitRadiusSq = hitRadius * hitRadius; | |
| // Check if mouse is over any node | |
| for (const [id, coords] of Object.entries(instance.locations)) { | |
| const { x, y } = getCoords(id); | |
| const dx = mouseX - x; | |
| const dy = mouseY - y; | |
| const distSq = dx * dx + dy * dy; | |
| if (distSq < hitRadiusSq) { |
| .notification-container { | ||
| position: absolute; | ||
| top: 16px; | ||
| right: 16px; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 8px; | ||
| z-index: 100; | ||
| max-width: 280px; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
The notification container is positioned at top/right 16px, which is the same placement as #mapOverlay. Since .notification-container has a higher z-index, it can cover the overlay badge. Consider offsetting notifications (e.g., below the overlay) or moving one of the elements to avoid overlap.
| .notification { | ||
| background: var(--surface); | ||
| border: 1px solid var(--border); | ||
| border-left: 4px solid var(--primary); | ||
| border-radius: 8px; | ||
| padding: 12px 16px; | ||
| box-shadow: var(--shadow); | ||
| animation: slideIn 0.3s ease-out, fadeOut 0.3s ease-in 2.7s; | ||
| pointer-events: auto; | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 10px; | ||
| } | ||
|
|
||
| .notification.fade-out { | ||
| animation: fadeOut 0.3s ease-in forwards; | ||
| } |
There was a problem hiding this comment.
.notification has a CSS fadeOut animation scheduled (2.7s delay) and the JS also triggers a second fade-out via the .fade-out class at 3s. Because the base fadeOut animation doesn't use animation-fill-mode: forwards, the notification can snap back to full opacity at 3s and then fade again (visible flicker). Use a single fade-out mechanism (CSS-only or JS-only) or ensure the CSS fadeOut persists with forwards.
Merge pull request #27 from IFRI-AI-Classes/fix/vizualisation
No description provided.