feat: add Elements panel & promote annotations to map elements #1524 - #1567
Conversation
|
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:
📝 WalkthroughWalkthroughThe change adds transparent generated-image fallbacks, annotation visibility filtering, and initialization wiring. The annotations plugin gains pin, sticky-note, and placed-image tools, HTML markers, dialogs, linked overlays, an Elements panel, management helpers, localization, and tests. ChangesMap elements and annotation integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MapClick
participant AnnotationPlugin
participant AppStore
participant ElementsPanel
MapClick->>AnnotationPlugin: choose element and location
AnnotationPlugin->>AnnotationPlugin: open element dialog
AnnotationPlugin->>AppStore: persist annotation feature
AnnotationPlugin->>AppStore: create linked overlay for extent image
AppStore->>ElementsPanel: update grouped element state
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 821-824: Update the plugin deactivation cleanup near
elementMarkerUnsub to call closeElementDialog() alongside
clearAllElementMarkers() and closeElementPopup(). Ensure the active element
dialog is removed before deactivate() completes so its Save callback cannot
mutate the store after the plugin is disabled.
- Around line 594-596: Update the existing-marker branches in the marker refresh
logic, including the paths around activePinMarkers updates, to refresh each
marker’s content and popup configuration from the latest annotation data, not
just its coordinates. Ensure title, description, color, image URL, and popup
closure are replaced while preserving the existing marker instance and
coordinate update behavior.
- Around line 1822-1830: Update the raw feature normalization flow around the
feature-management loop and the related rendering/reordering logic to assign
stable annotationId values to legacy features before they are grouped or
managed. Persist each generated ID on the feature properties, reuse it on
subsequent renders, and ensure features without existing annotationId values
remain included during updates, deletes, rendering, and reordering instead of
relying on Math.random() or dropping them.
- Around line 567-575: Validate pinColor and the corresponding color value used
in the code around the marker construction and lines 641-655 before using them
in SVG markup or CSS. Accept only safe color values (for example, a validated
hex color or approved color format) and fall back to the existing default when
validation fails; ensure the sanitized value is the one interpolated into fill
attributes and style declarations.
- Around line 1880-1884: Update the rename handling around updateElementProps so
changing an element title does not overwrite text for non-text elements.
Preserve the existing empty text value used by pins, notes, and images, while
continuing to update text for elements whose text should mirror the title.
- Around line 807-808: Update the subscriptions around elementMarkerUnsub and
the related panel subscription to use a selector or listener that runs only when
state.layers !== previousState.layers. Preserve the initial
syncAllElementMarkers() call, and ensure both annotation sync paths stop
reacting to unrelated store mutations that can disrupt inline edits.
🪄 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 Plus
Run ID: 6d493b49-c687-4784-8563-e16f3e69aee4
📒 Files selected for processing (5)
packages/map/src/generated-images.tspackages/map/src/layer-sync.tspackages/map/src/map-controller.tspackages/plugins/src/plugins/maplibre-annotations.tstests/elements-panel.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/src/plugins/maplibre-annotations.ts (1)
2054-2076: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the MapUp/MapDown button labels or semantics.
reorderElements(el.id, "up")swaps the element toward the end of the feature array, while the panel is rendered top-to-bottom in that same order (elements.forEach). So “Move Up” actually moves an item lower in the panel unless the label/action semantics are explicitly defined as z-order/sequence ordering. Rename the actions/buttons to match the rendered UI, or reverse the display if those labels are meant to control back/front ordering.🤖 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 `@packages/plugins/src/plugins/maplibre-annotations.ts` around lines 2054 - 2076, Align the button labels and actions in the controls created around reorderElements so the rendered top-to-bottom panel behavior matches the user-facing “Move Up” and “Move Down” semantics. Since reorderElements(..., "up") moves toward the end of the elements array and the panel uses elements.forEach order, either reverse the mapped actions/labels for visual movement or explicitly rename them to z-order terminology; keep the disabled-state logic consistent with the chosen semantics.
🤖 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/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 606-621: The marker synchronization logic is duplicated across
syncPinMarkers, syncStickyNoteMarkers, and syncPlacedImageMarkers. Extract the
shared filtering, visibility handling, popup closing/re-showing,
create-versus-update flow, and orphan cleanup into a syncMarkers helper
parameterized by marker kind, marker map, create callback, and update callback;
keep each existing function responsible only for its element-specific factory
and update behavior.
- Around line 1917-1943: Move the legacy annotationId backfill out of the
Elements-panel-only flow and run it during annotation-layer discovery or within
syncAllElementMarkers, before marker keys are computed, so every id-less feature
has a stable unique identifier regardless of panel visibility. Update the
backfill logic around rawFeatures and updatedFeatures to mint a UUID for missing
annotationId values rather than reusing potentially duplicated props.id or f.id
values, while preserving existing annotationId values.
- Around line 623-625: The popup positioning in the annotation update paths,
including the activePopupAnnotationId checks near the affected locations, only
refreshes on layers changes and becomes detached during map movement. Update the
popup lifecycle to respond to map move events by reprojecting the open popup or
closing it, and apply the same behavior to all corresponding popup paths rather
than only the first occurrence.
---
Outside diff comments:
In `@packages/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 2054-2076: Align the button labels and actions in the controls
created around reorderElements so the rendered top-to-bottom panel behavior
matches the user-facing “Move Up” and “Move Down” semantics. Since
reorderElements(..., "up") moves toward the end of the elements array and the
panel uses elements.forEach order, either reverse the mapped actions/labels for
visual movement or explicitly rename them to z-order terminology; keep the
disabled-state logic consistent with the chosen semantics.
🪄 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 Plus
Run ID: b1f689b2-697d-4bc8-949e-bfaea224442f
📒 Files selected for processing (1)
packages/plugins/src/plugins/maplibre-annotations.ts
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| function reorderElements(annotationId: string, direction: "up" | "down"): void { | ||
| const store = useAppStore.getState(); | ||
| const layer = findAnnotationLayer(store.layers); | ||
| if (!layer || !layer.geojson) return; | ||
|
|
||
| const features = layer.geojson.features; | ||
| const ids: string[] = []; | ||
| for (const f of features) { | ||
| const id = String((f.properties as Record<string, unknown>)?.annotationId || ""); | ||
| if (id && !ids.includes(id)) ids.push(id); | ||
| } | ||
|
|
||
| const idx = ids.indexOf(annotationId); | ||
| if (idx === -1) return; | ||
| const targetIdx = direction === "up" ? idx + 1 : idx - 1; | ||
| if (targetIdx < 0 || targetIdx >= ids.length) return; | ||
|
|
||
| const temp = ids[idx]; | ||
| ids[idx] = ids[targetIdx]; | ||
| ids[targetIdx] = temp; |
There was a problem hiding this comment.
Bug: Move Up / Move Down are inverted in the Elements panel.
reorderElements uses the same "up" → idx + 1 convention as the core store's reorderLayer (packages/core/src/store.ts), which is correct there because LayerPanel.tsx renders [...layers].reverse() — so a higher array index shows up higher on screen.
The Elements panel, however, builds elements directly from Array.from(map.values()) (insertion order) and renders them with a plain elements.forEach(...) — no .reverse(). So index 0 is the first row displayed (top of the list), and increasing its index moves it later in the array and later (down) in the panel.
Concretely: for elements [A, B, C] displayed top-to-bottom, clicking "▲ Move Up" on A (already at the top) computes targetIdx = idx + 1, producing [B, A, C] — which pushes A down to the second row instead of leaving it at the top. The disabled-state logic is consistent with this same (inverted) convention (up.disabled at the last row, down.disabled at the first row — see lines 2059/2071), so the buttons never error, they just do the opposite of what their icons/labels say.
Fix: either reverse elements for display (matching LayerPanel's convention) or flip the targetIdx calculation ("up" ? idx - 1 : idx + 1) to match the non-reversed display order.
| @@ -186,6 +230,10 @@ let labels: AnnotationLabels = { | |||
| deleteLast: "Delete last annotation", | |||
| clearAll: "Clear all annotations", | |||
| textPlaceholder: "Type label, Enter to place", | |||
| pinTitlePrompt: "Pin title:", | |||
There was a problem hiding this comment.
CLAUDE.md requires new user-facing strings to go through t()/the AnnotationLabels i18n wiring, but this PR only partially does that:
pinTitlePrompt,pinDescPrompt,stickyNotePrompt,imageUrlPromptare declared here with hardcoded English defaults but are never read anywhere in the file (the actual dialog atopenElementDialog, ~line 1061 onward, hardcodes its own strings:"Title","Description"/"Note Content","Image URL","Cancel","Save Element","At Point","Pinned to Extent", etc., instead of using these fields). That's dead code plus untranslatable UI text.- The new tool labels (
pin,sticky_note,placed_image) andelementsPanelTitleare added to theAnnotationLabelsshape, butTopToolbar.tsx'ssetAnnotationLabels({...})call (the one place that pipes i18n catalog strings in) was not updated to pass them, anden.json'sannotations.toolsblock still only hastext/arrow/rectangle/ellipse/freehand. So these new labels will always render in English regardless of locale, unlike every other tool in this plugin.
Given the app explicitly supports RTL/translated locales, worth wiring these through (or dropping the unused prompt fields) rather than leaving a half-migrated i18n surface.
| import type maplibregl from "maplibre-gl"; | ||
| import maplibregl from "maplibre-gl"; | ||
| import type { GeoLibreAppAPI, GeoLibreMapControlPosition, GeoLibrePlugin } from "../types"; | ||
| import { openRightPanel } from "../right-panel-registry"; |
There was a problem hiding this comment.
Minor: openRightPanel is imported here but never referenced — the file only calls it indirectly via appApi.openRightPanel(...) (line ~406). This is an unused import; worth removing (or using it directly instead of the optional appApi accessor if that was the intent).
| import { openRightPanel } from "../right-panel-registry"; |
| } = require("../packages/plugins/src/plugins/maplibre-annotations"); | ||
| const { LngLat } = require("maplibre-gl"); |
There was a problem hiding this comment.
Nit: these require(...) calls could just be top-level imports alongside the existing ones (pinFeature, stickyNoteFeature, placedImageFeature, updateElementProps, deleteElementById are all exported, and LngLat is exported from maplibre-gl). Mixing CJS require into an otherwise ESM/import-based test suite is inconsistent with the rest of tests/*.test.ts and only works here because of the tsx loader's CJS interop.
Code reviewBugs
Quality / CLAUDE.md
Performance
Security
No issues found in |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/elements-panel.test.ts (1)
165-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the tracking feature’s visibility as well.
These assertions only verify the linked overlay layer. A regression that updates the overlay but leaves the annotation feature’s
properties.visibleunchanged would still pass; assert both states after each visibility update.🤖 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 `@tests/elements-panel.test.ts` around lines 165 - 176, Extend the visibility-cascading assertions in the test around updateElementProps to also inspect the tracking annotation feature’s properties.visible after both the false and true updates. Keep the existing overlay-layer assertions and verify both states match the requested visibility.packages/plugins/src/plugins/maplibre-annotations.ts (2)
2062-2083: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign move controls with the displayed element order.
elementsis rendered in array order, so index0is the top row. The current disabled checks and direction mapping treat the array as reversed: “Move Up” is enabled at the top while “Move Down” is disabled there, and clicking the controls moves elements opposite to their labels.Also applies to: 2212-2249
🤖 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 `@packages/plugins/src/plugins/maplibre-annotations.ts` around lines 2062 - 2083, Update the move controls in the element-row rendering logic and the corresponding block around reorderElements so they follow displayed array order: disable “Move Up” for index 0, disable “Move Down” for the last index, and map each click handler to the reorder direction matching its label. Apply the same corrections to both control implementations.
915-924: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh active popups when the map moves.
The popup is positioned from projected coordinates, but this subscription only reacts to layer mutations. Panning or zooming can therefore leave an open element popup detached from its marker. Reproject it on map movement and remove that listener during unbind.
🤖 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 `@packages/plugins/src/plugins/maplibre-annotations.ts` around lines 915 - 924, Update the map binding setup around syncAllElementMarkers to subscribe to map movement events and refresh the active element popup/marker positioning after panning or zooming. Store the movement listener or unsubscribe handle alongside the existing elementMarkerUnsub state, and remove it during the corresponding unbind cleanup.
♻️ Duplicate comments (1)
packages/plugins/src/plugins/maplibre-annotations.ts (1)
1925-1940: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBackfill annotation IDs before marker synchronization.
ID normalization currently runs only when the Elements panel renders, after the initial marker sync. Id-less legacy features can therefore share an
undefinedmarker key until the panel is opened, causing markers and management actions to collapse onto the wrong element. Move this migration into annotation-layer discovery or marker synchronization, and persist unique IDs there instead of relying on potentially duplicated legacyidvalues.🤖 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 `@packages/plugins/src/plugins/maplibre-annotations.ts` around lines 1925 - 1940, Move the annotation ID backfill from the Elements-panel render path into annotation-layer discovery or the marker synchronization flow, before marker keys are derived. Update the rawFeatures normalization around updatedFeatures so every legacy feature receives and persists a unique annotationId, using existing IDs only when they are validated as unique and generating distinct IDs otherwise. Preserve unchanged features while ensuring synchronization and management actions never use an undefined or duplicated marker key.
🤖 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.
Outside diff comments:
In `@packages/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 2062-2083: Update the move controls in the element-row rendering
logic and the corresponding block around reorderElements so they follow
displayed array order: disable “Move Up” for index 0, disable “Move Down” for
the last index, and map each click handler to the reorder direction matching its
label. Apply the same corrections to both control implementations.
- Around line 915-924: Update the map binding setup around syncAllElementMarkers
to subscribe to map movement events and refresh the active element popup/marker
positioning after panning or zooming. Store the movement listener or unsubscribe
handle alongside the existing elementMarkerUnsub state, and remove it during the
corresponding unbind cleanup.
In `@tests/elements-panel.test.ts`:
- Around line 165-176: Extend the visibility-cascading assertions in the test
around updateElementProps to also inspect the tracking annotation feature’s
properties.visible after both the false and true updates. Keep the existing
overlay-layer assertions and verify both states match the requested visibility.
---
Duplicate comments:
In `@packages/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 1925-1940: Move the annotation ID backfill from the Elements-panel
render path into annotation-layer discovery or the marker synchronization flow,
before marker keys are derived. Update the rawFeatures normalization around
updatedFeatures so every legacy feature receives and persists a unique
annotationId, using existing IDs only when they are validated as unique and
generating distinct IDs otherwise. Preserve unchanged features while ensuring
synchronization and management actions never use an undefined or duplicated
marker key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ed328b5-9ba6-425b-8a1f-5e0614096f9c
📒 Files selected for processing (4)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonpackages/plugins/src/plugins/maplibre-annotations.tstests/elements-panel.test.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/map/src/generated-images.ts (1)
118-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the placeholder registration into a helper.
The same
map.addImage(id, { width: 1, height: 1, data: TRANSPARENT_1X1 })literal is repeated four times; a localaddPlaceholder(map, id)keeps the fallback in one place.♻️ Proposed refactor
const TRANSPARENT_1X1 = new Uint8Array([0, 0, 0, 0]); + +function addTransparentPlaceholder(map: maplibregl.Map, id: string): void { + if (map.hasImage(id)) return; + map.addImage(id, { width: 1, height: 1, data: TRANSPARENT_1X1 }); +} function addGeneratedImage(map: maplibregl.Map, id: string): void { if (map.hasImage(id)) return; const factory = factories.get(id); if (!factory) { - map.addImage(id, { width: 1, height: 1, data: TRANSPARENT_1X1 }); + addTransparentPlaceholder(map, id); return; } let result: ReturnType<GeneratedImageFactory>; try { result = factory(); } catch { - map.addImage(id, { width: 1, height: 1, data: TRANSPARENT_1X1 }); + addTransparentPlaceholder(map, id); return; } if (!result) { - map.addImage(id, { width: 1, height: 1, data: TRANSPARENT_1X1 }); + addTransparentPlaceholder(map, id); return; } if (result instanceof Promise) { result .then((resolved) => { if (resolved && !map.hasImage(id)) { map.addImage(id, resolved.image, { pixelRatio: resolved.pixelRatio }); - } else if (!map.hasImage(id)) { - map.addImage(id, { width: 1, height: 1, data: TRANSPARENT_1X1 }); + } else { + addTransparentPlaceholder(map, id); } }) - .catch(() => { - if (!map.hasImage(id)) { - map.addImage(id, { width: 1, height: 1, data: TRANSPARENT_1X1 }); - } - }); + .catch(() => addTransparentPlaceholder(map, id));🤖 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 `@packages/map/src/generated-images.ts` around lines 118 - 155, Extract the repeated transparent-image fallback from addGeneratedImage into a local addPlaceholder(map, id) helper, using TRANSPARENT_1X1 and the existing 1x1 image dimensions. Replace all four direct map.addImage placeholder calls with this helper while preserving the current fallback behavior.
🤖 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/generated-images.ts`:
- Around line 121-126: The generated-image registration flow must replace
existing transparent placeholders once a real factory is registered. Update
registerGeneratedImage() to detect an existing placeholder, remove or replace
it, persist the factory, and invoke addGeneratedImage() so the real sprite is
materialized; preserve the current behavior for genuinely missing images.
In `@packages/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 1237-1240: Update the placed_image flow around openElementDialog
so imageUrl is validated in saveBtn.onclick before the dialog closes. When the
URL is empty, keep the dialog open and mark the image field invalid; only
proceed with element creation after a valid imageUrl, removing the silent early
return in the post-dialog callback.
- Around line 237-244: Update openElementDialog to use the localized labels
pinTitlePrompt, pinDescPrompt, stickyNotePrompt, imageUrlPrompt, cancel,
saveElement, atPoint, and pinnedToExtent instead of hardcoded English text.
Replace mode-toggle onclick comparisons based on el.textContent with an explicit
button state such as dataset.mode, while preserving the existing mode behavior
when translations collide.
In `@tests/elements-panel.test.ts`:
- Around line 27-39: Replace the any cast in the mockApp setup for
maplibreAnnotationsPlugin.activate and deactivate with a typed
Partial<GeoLibreAppAPI> or small typed factory. Ensure the mock satisfies the
required activate/deactivate API signatures while preserving the existing test
behavior.
- Around line 43-87: Extend the test case around the store setup to call
updateElementProps for "elem-1" with the renamed title, then assert that the
first feature reflects "Renamed" while the sibling feature’s properties,
including its title, remain unchanged. Keep the existing addLayer GeoJSON
assertions and use the store’s element-management path directly.
---
Outside diff comments:
In `@packages/map/src/generated-images.ts`:
- Around line 118-155: Extract the repeated transparent-image fallback from
addGeneratedImage into a local addPlaceholder(map, id) helper, using
TRANSPARENT_1X1 and the existing 1x1 image dimensions. Replace all four direct
map.addImage placeholder calls with this helper while preserving the current
fallback behavior.
🪄 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 Plus
Run ID: 9d363bab-1eda-489f-b815-b41105b8f2c1
📒 Files selected for processing (7)
apps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/i18n/locales/en.jsonpackages/map/src/generated-images.tspackages/map/src/layer-sync.tspackages/map/src/map-controller.tspackages/plugins/src/plugins/maplibre-annotations.tstests/elements-panel.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/plugins/src/plugins/maplibre-annotations.ts (2)
100-107: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRe-activation can orphan the previous right-panel disposer.
unregisterRightPanelDisposeris a module-scoped single slot. Ifactivate()runs a second time beforedeactivate()unregisters the panel, the earlier disposer reference is overwritten and never invoked, leaving a stale "geolibre-elements-panel" registration alive.🛡️ Proposed guard
if (typeof app.registerRightPanel === "function") { + unregisterRightPanelDisposer?.(); unregisterRightPanelDisposer = app.registerRightPanel({🤖 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 `@packages/plugins/src/plugins/maplibre-annotations.ts` around lines 100 - 107, Update activate’s right-panel registration flow around unregisterRightPanelDisposer to invoke the existing disposer before assigning a new one. Ensure repeated activation unregisters the prior “geolibre-elements-panel” registration and then stores the newly returned disposer, preserving normal deactivate behavior.
1066-1156: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTitle/Color labels still hardcoded for sticky-note and placed-image dialogs.
Line 1069 only swaps in
labels.pinTitlePromptfortype === "pin"; sticky_note/placed_image fall back to the literal"Title".colorLabel.textContent = "Color"(line 1155) is hardcoded for all three types. This is the same class of issue previously raised (and partly fixed in commit 0e98f07) for this dialog — the fix just didn't cover every type/field.Based on learnings, `packages/plugins/src/plugins` is vanilla-JS code and hardcoded UI text there is normally not flagged as missing i18n; however this file already wires a custom `AnnotationLabels` mechanism and uses it inconsistently within the same function, which is a code-consistency defect rather than a blanket "no i18n" gap.🌐 Proposed fix
- titleLabel.textContent = type === "pin" ? labels.pinTitlePrompt : "Title"; + titleLabel.textContent = type === "pin" ? labels.pinTitlePrompt : labels.titlePrompt;🤖 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 `@packages/plugins/src/plugins/maplibre-annotations.ts` around lines 1066 - 1156, Update the dialog label assignments in the annotation creation flow so sticky_note and placed_image titles use their corresponding localized AnnotationLabels entries instead of the literal "Title", while preserving the existing pinTitlePrompt behavior. Replace the hardcoded "Color" assignment in colorLabel with the appropriate localized label from the same labels object.Source: Learnings
🤖 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.
Outside diff comments:
In `@packages/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 100-107: Update activate’s right-panel registration flow around
unregisterRightPanelDisposer to invoke the existing disposer before assigning a
new one. Ensure repeated activation unregisters the prior
“geolibre-elements-panel” registration and then stores the newly returned
disposer, preserving normal deactivate behavior.
- Around line 1066-1156: Update the dialog label assignments in the annotation
creation flow so sticky_note and placed_image titles use their corresponding
localized AnnotationLabels entries instead of the literal "Title", while
preserving the existing pinTitlePrompt behavior. Replace the hardcoded "Color"
assignment in colorLabel with the appropriate localized label from the same
labels object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3ed8ac8-064c-45c4-8e39-3674a4d2276b
📒 Files selected for processing (3)
packages/map/src/generated-images.tspackages/plugins/src/plugins/maplibre-annotations.tstests/elements-panel.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/src/plugins/maplibre-annotations.ts (1)
2162-2166: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPanel re-render on ANY layer change can wipe in-progress title edits.
The subscription re-renders whenever
state.layers !== previous.layers, which is true for any layer mutation in the project (not just the annotations layer). Sinceupdate()fully rebuilds the DOM (including the currently-focused edit<input>, reset to the staleel.title), a user mid-rename loses their unsaved typed text and input focus the moment an unrelated layer changes elsewhere in the app.🐛 Proposed fix — scope the re-render trigger to the annotation layer's own data
update(); const unsub = useAppStore.subscribe((state, previous) => { - if (state.layers !== previous.layers) { - update(); - } + const layer = findAnnotationLayer(state.layers); + const prevLayer = findAnnotationLayer(previous.layers); + if (layer?.geojson !== prevLayer?.geojson) { + update(); + } });🤖 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 `@packages/plugins/src/plugins/maplibre-annotations.ts` around lines 2162 - 2166, Update the useAppStore subscription near update() so it re-renders only when the annotations layer’s own data changes, rather than whenever state.layers changes. Compare the relevant annotation-layer data in state and previous, while preserving update() for genuine annotation changes and avoiding DOM rebuilds during unrelated layer mutations.
🤖 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/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 420-446: Extract the shared popup offset and clamping calculations
from repositionElementPopup and showElementPopup into a computePopupPosition
helper that accepts the map and coordinates and returns the computed position.
Update both functions to reuse this helper, preserving the current box
dimensions, offsets, and edge-clamping behavior.
---
Outside diff comments:
In `@packages/plugins/src/plugins/maplibre-annotations.ts`:
- Around line 2162-2166: Update the useAppStore subscription near update() so it
re-renders only when the annotations layer’s own data changes, rather than
whenever state.layers changes. Compare the relevant annotation-layer data in
state and previous, while preserving update() for genuine annotation changes and
avoiding DOM rebuilds during unrelated layer mutations.
🪄 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 Plus
Run ID: f0211c40-bc58-4629-bcbe-0e41ae63ab60
📒 Files selected for processing (2)
packages/plugins/src/plugins/maplibre-annotations.tstests/elements-panel.test.ts
giswqs
left a comment
giswqs
left a comment
There was a problem hiding this comment.
This is great! Thank you for your contribution.
map annotations from basic decorations to first-class map elements (Pins, Sticky Notes, and Placed Images) and adds an Elements Panel in the right sidebar to manage them.
Summary by CodeRabbit
visible: false.