Fix categorized styling for marker icons - #1782
Conversation
Apply data-driven category colors to built-in and parameterized SVG marker sprites. Support inline, data URL, and remote SVG sources with focused regression coverage.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCategorized marker colors now produce dynamic marker-image expressions. Custom SVG markers support color overrides and remote or data sources. External control and GeoJSON layers use resolved marker images, with KML icons retained as fallback. ChangesCategorized marker rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LayerSync
participant markerImageValue
participant prepareMarker
participant MapLibre
LayerSync->>markerImageValue: Resolve marker image property specification
markerImageValue->>prepareMarker: Register color-specific marker variant
prepareMarker->>MapLibre: Add marker image
LayerSync->>MapLibre: Set dynamic icon-image specification
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
![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 `@packages/map/src/layer-sync.ts`:
- Around line 1924-1926: The KML icon expression currently falls back to the
fixed markerImageId, dropping categorized markers. Update the surrounding layer
expression and prepareKmlFeatureIcons usage so KML icons take priority while the
fallback uses markerImage, preserving categorized non-KML features; add coverage
for a layer containing both KML-icon and categorized features.
In `@packages/map/src/markers.ts`:
- Around line 261-264: Update the imageFor helper used by markerImageValue so
non-string or invalid-hex branch values return baseId instead of the original
value; preserve prepareMarker for valid hex colors. Add a regression test
covering an invalid branch output such as "red" and verify it resolves to the
base marker.
🪄 Autofix
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 Plus
Run ID: d10db00c-1d94-41b1-867b-1d74b5942460
📒 Files selected for processing (3)
packages/map/src/layer-sync.tspackages/map/src/markers.tstests/marker-categories.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Performance
Quality
Security
CLAUDE.md
|
- preserve categorized fallbacks for mixed KML icon layers - fall back safely for invalid color outputs - recursively translate zoom-scoped rule color expressions
- cache remote SVG source text across per-color sprite generation - bound the source cache to avoid unbounded session growth
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/map/src/markers.ts`:
- Around line 22-24: Update the SVG source caching flow around svgSourceCache so
resolved null results are not retained indefinitely: preserve in-flight promise
deduplication, then remove failed entries after a null result or apply a short
negative-cache TTL while retaining successful sources. Add a regression test
covering a failed fetch followed by a successful retry for the same source.
🪄 Autofix
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 Plus
Run ID: ec968697-1f78-4c8b-aaf2-101b97e972f4
📒 Files selected for processing (1)
packages/map/src/markers.ts
Code reviewBugs
Performance
Quality
Security / CLAUDE.md
I traced the |
- markerImageValue: route a flat resolved color through imageFor instead of returning the base sprite. Rule-based mode with no drawable rules returns the else rule's color, which need not equal the layer's markerColor, so the marker was baked in the wrong color while the circle paint used the else color. - colorizedSvgSource: drop a failed remote-SVG fetch from svgSourceCache after the await, so a transient error no longer disables QGIS color-parameter resolution for that URL for the rest of the session; in-flight dedup is kept. - layer-sync: remove the redundant prepareMarker call. markerImageValue resolves the same base marker internally, so markerImageId was non-null exactly when markerImage was — the `?? markerImageId ?? ""` fallback was dead code. - markers: hoist the match/step/case head set to a module constant instead of allocating it on every (recursive) imageFor call. - tests: cover the else-rule-only color bake and a failed-then-successful remote SVG fetch.
|
Both inline comments posted. Now writing the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- markerImageValue: pass the normalized hex color to prepareMarker instead of the raw expression output. normalizeHexColor accepts bare and shorthand hex, but prepareMarker uses the color verbatim for the sprite id and the canvas fill, so an expression emitting "fff" baked a sprite that drew black, and "#FDE725" baked a duplicate of an already-registered lowercase color. - tests: add a graduated-mode case (the numeric step from graduatedStops) and a shorthand/upper-case hex case.
| const imageFor = (value: unknown): unknown => { | ||
| if (typeof value === "string") { | ||
| // Bake the canonical form: prepareMarker uses the color verbatim for both | ||
| // the sprite id and the fill, so a bare or shorthand hex ("fff") from a | ||
| // hand-authored expression would otherwise draw black, and "#FDE725" | ||
| // would bake a second sprite for a color already registered lowercase. | ||
| const normalized = normalizeHexColor(value); | ||
| return normalized ? (prepareMarker(style, normalized) ?? baseId) : baseId; | ||
| } | ||
| if (!Array.isArray(value)) return baseId; | ||
|
|
||
| const expression = [...value]; | ||
| const firstOutput = expression[0] === "match" ? 3 : 2; | ||
| if (!COLOR_BRANCH_HEADS.has(String(expression[0]))) return baseId; | ||
| for (let index = firstOutput; index < expression.length; index += 2) { | ||
| expression[index] = imageFor(expression[index]); | ||
| } | ||
| if (expression[0] !== "step") { | ||
| expression[expression.length - 1] = imageFor(expression[expression.length - 1]); | ||
| } | ||
| return expression; | ||
| }; |
There was a problem hiding this comment.
Quality / completeness (medium confidence): imageFor only recolors a branch when its output is a literal hex string (or a nested match/step/case). In "expression" mode (free-form JSON typed by the user), a color output that is a CSS named color ("red", "steelblue"), an rgb()/rgba()/hsl() string, or a nested color-producing sub-expression (["to-color", …], ["rgb", …]) fails normalizeHexColor/COLOR_BRANCH_HEADS and silently collapses to the single fallback marker color for that whole branch — with no warning surfaced to the user.
This is intentional per the "uses the base marker for invalid expression color outputs" test, but the mismatch could be confusing: the same expression correctly colors fill/line/circle layers per-feature (via vectorFillColorValue/vectorLineColorValue, which pass the raw expression straight to MapLibre) while the marker icon quietly reverts to one flat color. Worth either a one-line note in the markerImageValue JSDoc calling this out as a known limitation, or (as a follow-up) resolving arbitrary CSS colors to hex via a throwaway canvas ctx.fillStyle round-trip before falling back.
| async function colorizedSvgSource(markup: string, color: string): Promise<string | null> { | ||
| let sourceMarkup = markup; | ||
| if (/^(?:https?:|data:image\/svg\+xml)/i.test(markup)) { | ||
| let pending = svgSourceCache.get(markup); | ||
| if (!pending) { | ||
| pending = fetch(markup) | ||
| .then((response) => (response.ok ? response.text() : null)) | ||
| .catch(() => null); | ||
| if (svgSourceCache.size >= MAX_SVG_SOURCE_CACHE) { | ||
| const oldest = svgSourceCache.keys().next().value; | ||
| if (oldest !== undefined) svgSourceCache.delete(oldest); | ||
| } | ||
| svgSourceCache.set(markup, pending); | ||
| } | ||
| const fetched = await pending; | ||
| if (fetched !== null) { | ||
| sourceMarkup = fetched; | ||
| } else { | ||
| // Do not keep a failed fetch cached: a transient network error would | ||
| // otherwise block every later color variant of the same source (and any | ||
| // styleimagemissing retry) until the entry is evicted. Dropping it only | ||
| // after the await still lets concurrent callers share the in-flight | ||
| // promise. | ||
| if (svgSourceCache.get(markup) === pending) svgSourceCache.delete(markup); | ||
| // Preserve the original source when a remote host blocks CORS. The | ||
| // marker still renders, although its QGIS color parameters cannot be | ||
| // resolved without access to the SVG text. | ||
| } | ||
| } | ||
| return resolveSvgSource(replaceSvgColorParameters(sourceMarkup, color)); | ||
| } |
There was a problem hiding this comment.
Security note (low confidence): this is the first place in markers.ts that reads a remote/data-URL SVG marker's text into JS (previously loadSvgMarker/loadMarkerSvgImage only ever set it as an <img src>, so the response body itself was never exposed to script). markerSvg is free-text project data — a shared/imported .geolibre.json (or a "expression"-mode style) can already point it at an arbitrary URL, and this fetch() runs automatically whenever the marker sprite is generated, with no user interaction beyond opening the project.
The existing console.warn in resolveSvgSource (packages/core/src/marker-shape.ts) already accepts that a remote markerSvg triggers a cross-origin request, so this may be an accepted extension of that trust boundary rather than a new one — flagging mainly because reading the response text (vs. only rendering it as an image) is a meaningfully larger capability than what existed before, worth a second look given untrusted projects are a supported flow (collaboration / imported style files).
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Two inline comments posted on |
Summary
Verification
Related to #1780
Summary by CodeRabbit
New Features
Bug Fixes
Tests