Skip to content

Commit 285e16a

Browse files
committed
Address Claude review feedback
- index.css: theme the placement popup tip for left/right anchors too, so the tip is not white in dark mode when the pin is near a map edge. - map-controller.ts: coalesce the drag handler to one update per animation frame so a heavier onMove cannot stutter the drag. - PhotosSource.tsx: dispose the drag pin and popup if the layer is removed before placement is finished, via a store subscription (the dialog closes right after, so the subscription owns the cleanup, not a React effect). - tests: cover loadPhotosAtLocation (placed at center, never skips).
1 parent 50b56d3 commit 285e16a

4 files changed

Lines changed: 71 additions & 11 deletions

File tree

apps/geolibre-desktop/src/components/layout/add-data/sources/PhotosSource.tsx

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -123,14 +123,31 @@ export function PhotosSource() {
123123
// Hand the user a draggable pin on the map to fine-tune the position. It
124124
// lives outside React, so closing the dialog (below) does not cancel it;
125125
// each drag rewrites the layer's coordinates in the store.
126-
source.shell.mapControllerRef.current?.startManualPlacement(manualCenter, {
127-
hint: t("addData.photos.manualHint"),
128-
doneLabel: t("common.done"),
129-
onMove: (lngLat) =>
130-
updateLayer(layer.id, {
131-
geojson: relocatePhotoFeatures(result.featureCollection, lngLat),
132-
}),
133-
});
126+
let unsubscribe = () => {};
127+
const dispose = source.shell.mapControllerRef.current?.startManualPlacement(
128+
manualCenter,
129+
{
130+
hint: t("addData.photos.manualHint"),
131+
doneLabel: t("common.done"),
132+
onMove: (lngLat) =>
133+
updateLayer(layer.id, {
134+
geojson: relocatePhotoFeatures(result.featureCollection, lngLat),
135+
}),
136+
onDone: () => unsubscribe(),
137+
},
138+
);
139+
// If the user deletes the layer before finishing placement, the pin and its
140+
// popup would otherwise linger on the map. Watch the store and dispose them
141+
// when the layer disappears. (This dialog closes right after, so the
142+
// subscription, not a React effect, owns the cleanup.)
143+
if (dispose) {
144+
unsubscribe = useAppStore.subscribe((state) => {
145+
if (!state.layers.some((l) => l.id === layer.id)) {
146+
dispose();
147+
unsubscribe();
148+
}
149+
});
150+
}
134151
// Close the dialog so the map (and the drag pin) become interactive; the
135152
// modal overlay would otherwise block dragging.
136153
source.shell.closeDialog();

apps/geolibre-desktop/src/index.css

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1872,6 +1872,16 @@ body,
18721872
border-top-color: hsl(var(--popover));
18731873
}
18741874

1875+
.geolibre-placement-popup-root.maplibregl-popup-anchor-left
1876+
.maplibregl-popup-tip {
1877+
border-right-color: hsl(var(--popover));
1878+
}
1879+
1880+
.geolibre-placement-popup-root.maplibregl-popup-anchor-right
1881+
.maplibregl-popup-tip {
1882+
border-left-color: hsl(var(--popover));
1883+
}
1884+
18751885
.geolibre-placement-popup {
18761886
display: flex;
18771887
flex-direction: column;

packages/map/src/map-controller.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -913,10 +913,20 @@ export class MapController {
913913
.addTo(map);
914914

915915
let disposed = false;
916+
// The `drag` event fires once per pointer-move frame (60-120 Hz), and each
917+
// call rewrites the store and re-syncs the source. Coalesce to one update
918+
// per animation frame so a heavier `onMove` cannot stutter the drag.
919+
let rafPending = false;
916920
const handleDrag = () => {
917-
const next = marker.getLngLat();
918-
popup.setLngLat(next);
919-
options.onMove([next.lng, next.lat]);
921+
if (rafPending) return;
922+
rafPending = true;
923+
requestAnimationFrame(() => {
924+
rafPending = false;
925+
if (disposed) return;
926+
const next = marker.getLngLat();
927+
popup.setLngLat(next);
928+
options.onMove([next.lng, next.lat]);
929+
});
920930
};
921931
const dispose = () => {
922932
if (disposed) return;

tests/geotagged-photos.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
isPhotoDropFileName,
88
isPhotoFileName,
99
isValidLngLat,
10+
loadPhotosAtLocation,
1011
PHOTO_IMAGE_EXTENSIONS,
1112
relocatePhotoFeatures,
1213
} from "../apps/geolibre-desktop/src/lib/geotagged-photos";
@@ -165,3 +166,25 @@ describe("relocatePhotoFeatures", () => {
165166
assert.deepEqual(collection.features[1].geometry.coordinates, [-100, 40]);
166167
});
167168
});
169+
170+
describe("loadPhotosAtLocation", () => {
171+
it("places every photo at the center and never skips one", async () => {
172+
// A buffer with no EXIF: GPS is absent, so the GPS importer would skip it,
173+
// but manual placement must still produce a feature at the center. The
174+
// thumbnail is null here (no canvas decoder under node), which is fine.
175+
const files = [
176+
new File([new Uint8Array([0, 1, 2, 3])], "a.jpg", { type: "image/jpeg" }),
177+
new File([new Uint8Array([4, 5, 6, 7])], "b.jpg", { type: "image/jpeg" }),
178+
];
179+
const result = await loadPhotosAtLocation(files, [-100, 40]);
180+
assert.equal(result.total, 2);
181+
assert.equal(result.located, 2);
182+
assert.equal(result.skipped, 0);
183+
assert.equal(result.featureCollection.features.length, 2);
184+
for (const feature of result.featureCollection.features) {
185+
assert.deepEqual(feature.geometry.coordinates, [-100, 40]);
186+
}
187+
assert.equal(result.featureCollection.features[0].properties?.name, "a.jpg");
188+
assert.equal(result.featureCollection.features[1].properties?.name, "b.jpg");
189+
});
190+
});

0 commit comments

Comments
 (0)