ERA-13076: Remove approach to loading icons via one enormous sprite by jeslefcourt · Pull Request #1529 · PADAS/das-web-react

jeslefcourt · 2026-04-08T21:55:20Z

Summary

Replaces SVG sprite-based icon loading with direct <img> tags referencing static SVG file URLs
Removes the preloaded ReportTypeIconSprite from App.js and the calcIconUrl sprite-lookup logic from DasIcon
Updates CSS selectors in patrol/report list and header styles from svg to img, replacing fill with filter: invert(1)

Jira

Test plan

Event icons render correctly in the report list
Patrol icons render correctly in the patrol list and detail view
Collection icons show the correct badge icon
Generic fallback icon displays when an icon is missing
Community-scoped routes load icons from the community API endpoint

🤖 Generated with Claude Code

Replace SVG sprite-based icon loading with direct img tags using static SVG file URLs, simplifying the icon rendering pipeline and removing the preloaded sprite dependency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

luixlive

A couple small suggestions and a couple questions. Nothing hard-blocking, but worth taking a minute to think them through.

…mage files. Had a bit of a snowball effect into things impacted by cascading styles.

- Remove !important flags on svgs - Fixed icon colors in clusters and cluster popups changing when editing events before saving - Sanitize SVGs to avoid injection attacks

Copilot

Pull request overview

Copilot reviewed 22 out of 27 changed files in this pull request and generated 3 comments.

JoshuaVulcan

Looks and feels pretty good so far! Will keep manually testing. My feedback on this review pass is around finishing off the refactor by making sure files and names are cleaned up.

JoshuaVulcan · 2026-04-09T15:37:48Z

 import mapboxgl from 'mapbox-gl';

 import { CLUSTER_CLICK_ZOOM_THRESHOLD, LAYER_IDS, SUBJECT_FEATURE_CONTENT_TYPE } from '../constants';
+import { calcSvgImageIconId } from '../MapImageFromSvgSpriteRenderer';


It seems like MapImageFromSvgSpriteRenderer should be removed entirely, and the utility function to calculate image URLS lifted/shifted elsewhere.

JoshuaVulcan · 2026-04-09T15:38:50Z

-const getFeatureIcon = (feature, mapImages) =>
-  mapImages[`${feature.properties.icon_id}-${feature.properties.priority}`]?.image;
+const getFeatureIcon = (feature, mapImages, locallyEditedEvent, eventStore) => {
+  const isLocally = locallyEditedEvent?.id === feature.properties.id;


variable naming nit: isLocally 😢

Agreed. isLocally is ambiguous. The code that defines the variable isn't, but I'd rather have something like isLocallyEditedEventFeature.

JoshuaVulcan · 2026-04-09T15:44:16Z

@@ -0,0 +1,64 @@
+import React from 'react';


Love the changes here. Seems like we'd want to rename away from DasIcon as well?

luixlive

Just a couple more comments and questions. I see now we are also updating the clusters when we have a locally edited event, which is a nice addition, but it's increasing the size of this PR considerably.

Once these comments are solved / responded, I'll be ready to approve. Still think we will need a special treatment from QA to make sure all icons render correctly and that clusters work and update correctly when we have local changes in an event that haven't been saved yet 👍 @AlanCalvillo

luixlive · 2026-04-09T16:00:44Z


  useMapEventBinding('sourcedata', onSourceData);

+  useEffect(() => {


I think these useEffects can have a performance impact. Their dependency arrays include the callback itself (updateClusterMarkersCallback), this means that everytime the callback gets redefined, they will be called. In other words, whenever any of the following variables changes, we will be running the markers update:
addClusterPolygon, eventStore, locallyEditedEvent, map, onShowClusterSelectPopup, removeClusterPolygon.

Probably we could stabilize this with a useRef or a more complex useEffect that only triggers the callback specifically when locallyEditedEvent and mapImages change.

luixlive · 2026-04-09T16:14:39Z

-  mapImages[`${feature.properties.icon_id}-${feature.properties.priority}`]?.image;
+const getFeatureIcon = (feature, mapImages, locallyEditedEvent, eventStore) => {
+  const isLocally = locallyEditedEvent?.id === feature.properties.id;
+  if (isLocally) {


This code block gives me the impression that the agent thought locally edited events could update their icons, but they can't. All the user can changes that results in an update to the icon is priority. I feel this method can be simplified, something like:

const getFeatureIcon = (feature, mapImages) => { const priority = locallyEditedEvent?.id === feature.properties.id ? locallyEditedEvent.priority : feature.properties.priority return mapImages[calcSvgImageIconId({ icon_id: feature.properties.icon_id, priority })]?.image; }

Unless of course I'm missing something 🤔

luixlive · 2026-04-09T16:26:51Z

+
+const CONTAINER_SELECTOR = 'svg,g,defs,symbol,marker,clipPath,mask,pattern';
+
+const sanitizeSvg = (text) => {


Feels like a brute-force approach to clean the icons. I guess it's better to keep this file than the sprites file. Probably would try a last prompt to Claude like: "Refactor this file and apply clean code practices plus explanatory comments to improve its readability and maintainability". And I would say this requires some thorough manual testing in a couple envs just to make sure all icons render correctly 👍

feat: ERA-13076 remove sprite approach to loading icons

4a685e2

Replace SVG sprite-based icon loading with direct img tags using static SVG file URLs, simplifying the icon rendering pipeline and removing the preloaded sprite dependency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jeslefcourt self-assigned this Apr 8, 2026

jeslefcourt requested a review from luixlive April 8, 2026 21:55

luixlive requested changes Apr 8, 2026

View reviewed changes

Comment thread src/DasIcon/index.js Outdated

Comment thread src/DasIcon/index.js Outdated

Comment thread src/EventIcon/index.js Outdated

Comment thread src/EventIcon/index.js

Comment thread src/PatrolDetailView/Header/styles.module.scss Outdated

Comment thread src/App.js

luixlive requested a review from Copilot April 8, 2026 22:56

Copilot started reviewing on behalf of luixlive April 8, 2026 22:57 View session

This comment was marked as outdated.

Sign in to view

jeslefcourt added 2 commits April 8, 2026 20:21

Addressed feedback from Luis. Added conditional support for SVGs or i…

cd89936

…mage files. Had a bit of a snowball effect into things impacted by cascading styles.

Removed no longer necessary file

5b2e7f2

jeslefcourt requested a review from Copilot April 9, 2026 03:23

Copilot started reviewing on behalf of jeslefcourt April 9, 2026 03:23 View session

This comment was marked as outdated.

Sign in to view

Feedback from Copilot

3d4d8ab

- Remove !important flags on svgs - Fixed icon colors in clusters and cluster popups changing when editing events before saving - Sanitize SVGs to avoid injection attacks

jeslefcourt requested a review from Copilot April 9, 2026 04:52

This comment was marked as outdated.

Sign in to view

jeslefcourt requested a review from Copilot April 9, 2026 05:45

Copilot started reviewing on behalf of jeslefcourt April 9, 2026 05:46 View session

This comment was marked as outdated.

Sign in to view

jeslefcourt force-pushed the ERA-13076-remove-sprite-approach-to-loading-icons branch from d4bcd19 to 3d4d8ab Compare April 9, 2026 06:20

Copilot feedback

33378d0

jeslefcourt requested a review from Copilot April 9, 2026 07:15

Copilot started reviewing on behalf of jeslefcourt April 9, 2026 07:15 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

Comment thread src/DasIcon/index.js Outdated

Comment thread src/DasIcon/index.js

Comment thread src/EventIcon/index.js Outdated

jeslefcourt changed the title ~~ERA-13076: Remove sprite approach to loading icons~~ ERA-13076: Remove approach to loading icons via one enormous sprite Apr 9, 2026

jeslefcourt added 2 commits April 9, 2026 00:35

Simplified special handling for community case

d5ba80c

Copilot feedback

392cb26

jeslefcourt requested a review from luixlive April 9, 2026 07:40

JoshuaVulcan requested changes Apr 9, 2026

View reviewed changes

luixlive reviewed Apr 9, 2026

View reviewed changes


		useMapEventBinding('sourcedata', onSourceData);

		useEffect(() => {


		const CONTAINER_SELECTOR = 'svg,g,defs,symbol,marker,clipPath,mask,pattern';

		const sanitizeSvg = (text) => {

Conversation

jeslefcourt commented Apr 8, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Jira

Test plan

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoshuaVulcan left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

luixlive Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

JoshuaVulcan Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

luixlive Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

luixlive Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

luixlive Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jeslefcourt commented Apr 8, 2026 •

edited by atlassian Bot

Loading