From d11fa3baa55ef59e3d13d7231a45ca0216d72dea Mon Sep 17 00:00:00 2001 From: Aleksei Vesnin Date: Mon, 21 Sep 2026 21:44:15 +0300 Subject: [PATCH 1/3] feat(location)!: one place picker and one place label everywhere A dive site's location is the object a trip part's already was: a name, a fuller name, a centre and a box. Both forms fill it from one mapping, and every surface that read the member as text reads its name. Co-Authored-By: Claude Opus 5 --- DECISIONS.md | 119 +++++++----- src/app/dives/page.tsx | 2 +- src/app/sites/[id]/page.tsx | 6 +- src/app/sites/page.tsx | 2 +- src/app/trips/[id]/page.tsx | 46 ++--- ...elete-with-reassign-dialog.render.test.tsx | 2 +- .../dives/delete-with-reassign-dialog.tsx | 2 +- src/components/dives/dive-detail-sidebar.tsx | 4 +- .../dive-site-multi-select.render.test.tsx | 8 +- .../dives/dive-site-multi-select.tsx | 6 +- .../dives/dive-sites-label.render.test.tsx | 6 +- src/components/dives/recent-dives-card.tsx | 4 +- .../map/locations-map.browser.test.tsx | 4 +- src/components/map/locations-map.tsx | 5 + .../sites/dive-site-dialog.render.test.tsx | 91 +++++++++ src/components/sites/dive-site-dialog.tsx | 70 +++++-- src/components/sites/place-search.tsx | 4 +- .../trip-locations-label.render.test.tsx | 16 +- src/components/trips/trip-locations-label.tsx | 19 +- .../trips/trip-parts-field.render.test.tsx | 15 +- src/components/trips/trip-parts-field.test.ts | 76 +++----- src/components/trips/trip-parts-field.tsx | 69 ++----- src/hooks/useGeocodedLocation.render.test.tsx | 77 +++++--- src/hooks/useGeocodedLocation.ts | 68 ++++--- src/lib/api/dive-sites.ts | 30 ++- src/lib/api/dives.ts | 6 +- src/lib/api/geocoding.ts | 7 +- src/lib/api/location.ts | 40 ++++ src/lib/api/trips.ts | 23 +-- src/lib/locations.test.ts | 177 ++++++++++++++++++ src/lib/locations.ts | 93 +++++++++ src/lib/trip-locations.test.ts | 130 +++---------- src/lib/trip-locations.ts | 76 ++------ src/lib/trip-parts.ts | 7 +- src/lib/validations/dive-site.test.ts | 2 +- src/lib/validations/dive-site.ts | 11 +- src/lib/validations/location.ts | 55 ++++++ src/lib/validations/trip.ts | 42 +---- 38 files changed, 898 insertions(+), 522 deletions(-) create mode 100644 src/lib/api/location.ts create mode 100644 src/lib/locations.test.ts create mode 100644 src/lib/locations.ts create mode 100644 src/lib/validations/location.ts diff --git a/DECISIONS.md b/DECISIONS.md index 266025ec..df82753a 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2623,18 +2623,20 @@ follows `NEXT_PUBLIC_MAP_TILE_URL` to a self-hosted server. ## A trip part's place is self-describing, so nothing has to be resolved `TripPartsField` is built on the same `CreatableCombobox` as `DiveSiteMultiSelect` but holds -`{name, display_name, latitude, longitude, bbox_*}` objects, the snapshot the API stores, rather -than uuids, so a row renders from its own content with nothing to fetch and no loading state. - -Places therefore have no id. `locationKey` derives one from content: -`geo:{lat}:{lon}:{display_name}` for a geocoded place, `txt:{name}` lowercased and trimmed for a -typed one. `mapSearchResults` collapses results sharing a key, since Nominatim can return the same -place twice, and the row's `value`/`selectedItem` pair is keyed by it. Nothing compares keys across -parts: two parts may name the same place — Dahab, then Sharm, then back to Dahab — so there is no -`excludeIds` and no "already in the list" refusal. Rows are keyed and removed by position, or a -repeat would go as a pair. Position-keyed rows swap content under a focused drag handle, so -`useDragSort` moves focus to the destination handle (`data-drag-handle`) a frame after a keyboard -reorder; for id-keyed lists that is a no-op. +`{name, full_name, latitude, longitude, bbox_*}` objects, the snapshot the API stores, rather than +uuids, so a row renders from its own content with nothing to fetch and no loading state. A row shows +the name alone: `name` carries the place's country, and the only string a second line could add is +the provider's postal chain, which nothing renders. + +Places therefore have no id. `locationKey` derives one from content: `geo:{lat}:{lon}:{full_name}` +for a geocoded place, `txt:{name}` lowercased and trimmed for a typed one. `mapSearchResults` +collapses results sharing a key, since Nominatim can return the same place twice, and the row's +`value`/`selectedItem` pair is keyed by it. Nothing compares keys across parts: two parts may name +the same place — Dahab, then Sharm, then back to Dahab — so there is no `excludeIds` and no "already +in the list" refusal. Rows are keyed and removed by position, or a repeat would go as a pair. +Position-keyed rows swap content under a focused drag handle, so `useDragSort` moves focus to the +destination handle (`data-drag-handle`) a frame after a keyboard reorder; for id-keyed lists that is +a no-op. ## An unmatched query is addable as text, and that is an outage hatch as much as a long tail @@ -2673,12 +2675,14 @@ otherwise write the stale name back. ## Coordinates that were picked, not typed, are numbers -`lib/validations/dive-site.ts` validates coordinates as strings against a regex because a diver -typing a latitude passes through "-" and "-17." on the way to "-17.9". A trip location's coordinates +`lib/validations/dive-site.ts` validates a _site's own pin_ as strings against a regex because a +diver typing a latitude passes through "-" and "-17." on the way to "-17.9". A place's coordinates arrive whole inside an object picked from a menu, or are absent for a typed-in place; there is no -half-entered state. So `tripLocationSchema` uses plain `z.number()` with the API's bounds, catching -a nonsense object before the round trip. It carries no `z.preprocess` or `.transform`, which keeps -`z.input<>` inference working and `TripLocationFormValue` usable as the form's own type. +half-entered state, and no field for one. So `locationSchema` in `lib/validations/location.ts` uses +plain `z.number()` with the API's bounds, catching a nonsense object before the round trip. It +carries no `z.preprocess` or `.transform`, which keeps `z.input<>` inference working and +`LocationFormValue` usable as either form's own type — one schema, because a dive site's locality +and a trip part's are one object. ## The confirmation map is not `MapPicker`, and that is most of why it is short @@ -2757,17 +2761,24 @@ skeleton's height cannot drift from the map's. ## A "+N" is a promise that hovering will say what N was -`Moalboal, Bohol +2` compacts a list the payload already holds, so both surfaces carry the full list -as a `title`. +`Dahab, Egypt +2` compacts a list the payload already holds, so every surface showing it carries the +full list as a `title`. The hint sits on the whole label, not the "+N": a two-character badge is a small hover target and splits the answer in two. A hint that repeats the label is worse than none, so `formatTripLocationNamesHint` sits beside `formatTripLocationNames`, answers `undefined` when nothing is hidden, and decides that under the -same blank-dropping rule: `["Moalboal", " ", "Bohol"]` under `max: 2` shows no "+N" and gets no -tooltip. `TripLocationsLabel` owns the limit and calls both, as `DiveSitesLabel` does for dives; the -trips table and the dashboard card pass locations and a fallback only. +same blank-dropping rule: `["Moalboal", " "]` under `max: 1` shows no "+N" and gets no tooltip. +`TripLocationsLabel` calls both, as `DiveSitesLabel` does for dives; the trips table and the +dashboard card pass locations and a fallback only. + +`SHOWN_LOCATIONS` is **one**, and exported rather than private. One because a place's name carries +its country now, so two of "Dahab, Egypt" do not fit a table cell; exported because the trip page's +subtitle joins the same names without rendering this component, and a surface capping differently +would contradict a "+N" beside it. Every such list separates with `; ` — a comma is +indistinguishable from the commas inside each name. `LocationsMap`'s accessible label takes the +separator and not the cap: a cap withholds names from the one reader who cannot count the pins. `title` answers a mouse and nobody else: no hover on touch, unreachable by keyboard on a ``. The alternative is a `Popover` trigger nested in a link; hover-only stands until the app has a @@ -3301,39 +3312,44 @@ threaded through three shared primitives. ## A location's full label is trimmed of the name it sits beside, at render time -Nominatim's `display_name` opens with the name it matched, and surfaces show the name first. -`formatLocationContext` in `lib/trip-locations.ts` drops the leading parts of the label the name -repeats, returning `undefined` when nothing is left so callers drop the element with `&&`. +Nominatim's label opens with the name it matched, and the surface shows the name first. +`formatLocationContext` in `lib/locations.ts` drops the leading parts of the label the name repeats, +returning `undefined` when nothing is left so callers drop the element with `&&`. It aligns whole comma-separated parts, never substrings: "Dahab" is a duplicate in "Dahab, South Sinai" and context in "Blue Hole, Dahab, South Sinai", and whole parts stop "Ko Tao" eating "Ko Tao Island". -It runs at render, not in `geocodeResultToLocation`: `display_name` is stored on the part's place, -so saved trips would stay untrimmed. It is what the menu's `hint` shows, so the row the diver reads -is the one that lands in the field a click later. The stored label is the short form; see "The label -a trip location keeps is the API's short form, chosen on the way in". +Its one caller is the dive site place search, and both arguments come from a `GeocodeResult`: the +row's own bare name against the API's composed form, so a row reading "Dahab" carries "Egypt" beside +it and two same-named results come apart. Nothing renders a _stored_ place's `full_name`, so there +is no saved shape left to trim — a place's `name` carries its country, and a line under it would be +the provider's postal chain. See "The label a trip location keeps is the API's short form, chosen on +the way in". ## The label a trip location keeps is the API's short form, chosen on the way in -`geocodeResultToLocation` stores `GeocodeResult.location` — the API's `_short_location`, place plus -country from the provider's structured address — as a trip part's `location.display_name`, not -Nominatim's `display_name`; a dive log records "Dahab, Egypt", as the dive site form does -(`dive_site.location`). +`geocodeResultToLocation` (`lib/locations.ts`) stores `GeocodeResult.location` — the API's +`_short_location`, place plus country from the provider's structured address — as a place's `name`, +on a dive site and a trip part alike, rather than Nominatim's `display_name`; a dive log records +"Dahab, Egypt". It is received, not derived: the flat string cannot say whether the name is the settlement ("Dahab" → "Dahab, Egypt") or sits inside one ("Blue Hole" → "Blue Hole, Dahab, Egypt"); the structured address can. -Costs: a place saved before this keeps the provider's label until re-picked, and `locationKey` -(`geo:{lat}:{lon}:{label}`) keys it differently from a fresh pick of the same place. A trip stops -matching its region in search: `search_conditions` in `crud_trips.py` ORs the term against a part's -`display_name`, and `_short_location` composes place or region, never both; -`test_the_display_name_matches_too` hand-writes its fixture and misses this. Neither repair (a -second stored field, a geocoder-backed search) is worth it. +Costs: a place saved before this keeps the provider's label as its name until re-picked, and +`locationKey` (`geo:{lat}:{lon}:{full_name}`) keys it differently from a fresh pick of the same +place. + +The provider's whole label is now stored beside it as `full_name` — the second stored field this +entry used to reject, which DiveJSON asks for. It is written and rendered nowhere, so the choice +above is unchanged for every reader; what it settles is the search cost this entry recorded, the API +matching a term against both text columns and `full_name` carrying the region `_short_location` +omits. -Menu hints show it too; `ComboboxItem` has only `id`, `name`, `hint`. `placeKey` in -`place-search.tsx` keys on the provider's label. +The dive site place search's menu hint shows the short form too; `ComboboxItem` has only `id`, +`name`, `hint`. `placeKey` in `place-search.tsx` keys on the provider's label. ## The geocoder's attribution is a wire format, not display copy @@ -3371,6 +3387,14 @@ A position arrives three ways — pin, place search, pasted latitude/longitude p `DiveSiteDialog` sees all three, so the reverse geocode and its guards live in `hooks/useGeocodedLocation.ts`; `DiveSiteMapField` renders search, map and credit. +**A reverse-geocoded place is a name and nothing else**, where a forward search fills the whole +place. The coordinates that come back are the _site's_ — it is the pin the diver just dropped that +was looked up — so taking them would file the wreck as the centre of the town around it, on the one +path where the two points are identical and nothing on screen could show it. The Location field +holds the whole object and shows its name, so a save sends back the fuller name, the centre and the +extent a pick brought with it; typing a name replaces the place outright, and clearing the box sends +an explicit `null`. + The guards — newest request wins, the reply checked against the fields as they stand on arrival, `unknown` never clearing a field while `nameless` does, a nameless answer silent with nothing to clear — are reasoned in "The map writes into the coordinate fields". @@ -5171,12 +5195,13 @@ come first; the `hint` slot, not a new `CreatableCombobox` prop, marks which is A pick is `{ kind: "catalog", site }` or `{ kind: "geocode", result }`, and `DiveSiteDialog` forks on the tag, not on the namespaced row id. A catalog pick always fills Name; a geocoded one does not. -Location is `region, country`, never an ISO code; where neither resolved the field stays as typed, -so `adopt` takes `AdoptedPlace | null`. `suggestDiveSites` guards its own query length because the -combobox calls `onSearch` with `""` on open. Distance is computed here (`haversineMeters`, -`formatDistance`) so the unit preference holds. Catalog `attribution` joins the search credit, never -the map's. `DiveSiteMapField` passes the form's position whole or not at all; the endpoint answers -422 to half. +Location is a place named `region, country`, never an ISO code, and nothing else — the record's +coordinates are the site's, and the catalog resolved no centre or extent for the region it names. +Where neither resolved, the field stays as it was, so `adopt` takes `AdoptedPlace | null`. +`suggestDiveSites` guards its own query length because the combobox calls `onSearch` with `""` on +open. Distance is computed here (`haversineMeters`, `formatDistance`) so the unit preference holds. +Catalog `attribution` joins the search credit, never the map's. `DiveSiteMapField` passes the form's +position whole or not at all; the endpoint answers 422 to half. ## A refused save has to be announced, and `role="alert"` alone does not do it diff --git a/src/app/dives/page.tsx b/src/app/dives/page.tsx index ec8d5707..e055fda3 100644 --- a/src/app/dives/page.tsx +++ b/src/app/dives/page.tsx @@ -124,7 +124,7 @@ export default function DivesPage() { {formatDiveDateTime(dive.start_time)} {/* The primary site's location - the site the first column names. */} - {dive.dive_sites[0]?.location ?? "-"} + {dive.dive_sites[0]?.location?.name ?? "-"} {formatDurationHoursMinutes(dive.duration)} diff --git a/src/app/sites/[id]/page.tsx b/src/app/sites/[id]/page.tsx index 8e2fa0c0..e2b4953c 100644 --- a/src/app/sites/[id]/page.tsx +++ b/src/app/sites/[id]/page.tsx @@ -88,7 +88,7 @@ export default function DiveSiteDetailPage() { backHref="/sites" backLabel="Back to dive sites" title={diveSite.name} - subtitle={diveSite.location ? diveSite.location : undefined} + subtitle={diveSite.location?.name ?? undefined} actions={ <> @@ -146,14 +153,29 @@ describe("useGeocodedLocation", () => { render(); placePin().click(); - // No confirmation step. The Location field stays an ordinary text input, so - // a diver who wants something else types over it. + // No confirmation step. The Location field's visible half stays an ordinary + // text input, so a diver who wants something else types over it. await waitFor(() => - expect(onUseLocation).toHaveBeenCalledWith("Dahab, Egypt"), + expect(onUseLocation).toHaveBeenCalledWith({ name: "Dahab, Egypt" }), ); expect(reverseGeocode).toHaveBeenCalledWith(28.5717, 34.5372); }); + it("fills the place's name and nothing else, the pin being the site's own", async () => { + // The position a reverse geocode answers with is the *host's* - it is the + // pin the diver just dropped that was looked up - so adopting it would file + // the wreck's coordinates as the centre of the town around it. Nothing on + // screen shows either number, so this is the only place the rule is visible. + const onUseLocation = vi.fn(); + render(); + placePin().click(); + + await waitFor(() => expect(onUseLocation).toHaveBeenCalled()); + // Exact equality, not a per-key check: a new member arriving from the + // result by accident is precisely what this guards against. + expect(onUseLocation).toHaveBeenCalledWith({ name: "Dahab, Egypt" }); + }); + it("credits the place name, which is a licence condition of the data", async () => { render(); placePin().click(); @@ -219,7 +241,7 @@ describe("useGeocodedLocation", () => { render( , ); placePin().click(); @@ -237,12 +259,12 @@ describe("useGeocodedLocation", () => { render( , ); placePin().click(); - await waitFor(() => expect(onUseLocation).toHaveBeenCalledWith("")); + await waitFor(() => expect(onUseLocation).toHaveBeenCalledWith(null)); // Nothing was named, so there is nothing to credit. expect(credit()).not.toBeInTheDocument(); }); @@ -268,7 +290,12 @@ describe("useGeocodedLocation", () => { // of this form. reverseGeocode.mockResolvedValue({ status: "nameless" }); const onUseLocation = vi.fn(); - render(); + render( + , + ); placePin().click(); await waitFor(() => expect(reverseGeocode).toHaveBeenCalled()); @@ -278,7 +305,7 @@ describe("useGeocodedLocation", () => { it("announces a clearing, which is otherwise entirely silent", async () => { reverseGeocode.mockResolvedValue({ status: "nameless" }); - render(); + render(); placePin().click(); await waitFor(() => @@ -356,7 +383,7 @@ describe("useGeocodedLocation", () => { resolve(named(RESULT)); await waitFor(() => expect(reverseGeocode).toHaveBeenCalled()); - expect(onUseLocation).not.toHaveBeenCalledWith("Dahab, Egypt"); + expect(onUseLocation).not.toHaveBeenCalledWith({ name: "Dahab, Egypt" }); expect(credit()).not.toBeInTheDocument(); }); @@ -379,11 +406,11 @@ describe("useGeocodedLocation", () => { placePin().click(); await waitFor(() => - expect(onUseLocation).toHaveBeenCalledWith("Newer, Egypt"), + expect(onUseLocation).toHaveBeenCalledWith({ name: "Newer, Egypt" }), ); resolveFirst(named({ ...RESULT, location: "Staler, Egypt" })); await waitFor(() => expect(reverseGeocode).toHaveBeenCalledTimes(2)); - expect(onUseLocation).not.toHaveBeenCalledWith("Staler, Egypt"); + expect(onUseLocation).not.toHaveBeenCalledWith({ name: "Staler, Egypt" }); }); }); @@ -393,8 +420,12 @@ describe("useGeocodedLocation, adopting a searched place", () => { render(); screen.getByRole("button", { name: "pick a place" }).click(); + // The whole place, not just its name: a forward search answered about the + // place itself, so its centre and its extent are the locality's own. await waitFor(() => - expect(onUseLocation).toHaveBeenCalledWith("Dahab, Egypt"), + expect(onUseLocation).toHaveBeenCalledWith( + geocodeResultToLocation(RESULT), + ), ); expect(reverseGeocode).not.toHaveBeenCalled(); expect(credit()).toBeInTheDocument(); @@ -429,8 +460,8 @@ describe("useGeocodedLocation, adopting a searched place", () => { resolve(named({ ...RESULT, location: "Staler, Egypt" })); await waitFor(() => expect(reverseGeocode).toHaveBeenCalled()); - expect(onUseLocation).not.toHaveBeenCalledWith("Staler, Egypt"); - expect(onUseLocation).toHaveBeenCalledWith("Dahab, Egypt"); + expect(onUseLocation).not.toHaveBeenCalledWith({ name: "Staler, Egypt" }); + expect(onUseLocation).toHaveBeenCalledWith(geocodeResultToLocation(RESULT)); }); }); @@ -449,7 +480,7 @@ describe("useGeocodedLocation, adopting a row that named nowhere", () => { render( , ); pickNowhere(); diff --git a/src/hooks/useGeocodedLocation.ts b/src/hooks/useGeocodedLocation.ts index 490730c5..24604f48 100644 --- a/src/hooks/useGeocodedLocation.ts +++ b/src/hooks/useGeocodedLocation.ts @@ -2,6 +2,7 @@ import { useEffect, useLayoutEffect, useRef, useState } from "react"; import { geocodingAPI } from "@/lib/api/geocoding"; +import type { LocationFormValue } from "@/lib/validations/location"; // A whole position, as the form holds it. export interface GeocodedPosition { @@ -13,35 +14,37 @@ export interface UseGeocodedLocationOptions { // Whether the dialog this belongs to is open, so a reopen starts clean - see // the reset below. open: boolean; - // The live form values, as strings. + // The live coordinate fields, as strings. latitude?: string; longitude?: string; - location?: string; - onUseLocation: (location: string) => void; + // The live Location field, which holds the whole place rather than its name. + location?: LocationFormValue | null; + onUseLocation: (location: LocationFormValue | null) => void; } /** - * The parts of a picked row this hook uses: what to write into the Location - * field, and who to credit for it. + * The parts of a picked row this hook uses: the place to write into the + * Location field, and who to credit for it. * * Structural rather than `GeocodeResult`, because two things are picked from the * search now - a geocoded place and a dive site out of the catalog - and neither * one's whole shape is any of this hook's business. */ export interface AdoptedPlace { - location: string; + location: LocationFormValue; attribution: string; } export interface GeocodedLocation { /** - * Name the position that was just placed, and write the answer into the - * location field a round trip later. For a position that arrived without a - * name of its own: a pin on the map, or a pair pasted into the coordinates. + * Name the position that was just placed, and write a place carrying that + * name - and nothing else - into the location field a round trip later. For a + * position that arrived without a name of its own: a pin on the map, or a + * pair pasted into the coordinates. */ lookup: (position: GeocodedPosition) => void; /** - * Take the name a picked row already came with, which needs no lookup - and + * Take the place a picked row already came with, which needs no lookup - and * cancel any that is still in the air, since it is about the old pin. * * Pass `null` for a row that carries no place context at all. That is not the @@ -62,6 +65,11 @@ export interface GeocodedLocation { * The dive site form's Location field, in so far as anything but the diver fills * it in - the geocoder by lookup, a picked row by `adopt`. * + * The field holds a whole place, so what is written into it is one too. How much + * of a place each way in supplies is the interesting part: a forward search + * answers about the place itself and fills all of it, while a reverse geocode + * answers about the host's own pin and fills the name alone. + * * Owned by `DiveSiteDialog` rather than by the map beneath it, because three * separate things now place a position - the map, the place search above it, and * a coordinate pair pasted into the latitude/longitude inputs - and only the @@ -132,9 +140,12 @@ export function useGeocodedLocation({ const lookup = (placed: GeocodedPosition) => { const request = ++requestRef.current; - // What the Location field held when the position was placed. A reply is only - // allowed to write over *this*, never over something typed since. - const locationAtPick = location; + // What the Location field named when the position was placed. A reply is + // only allowed to write over *this*, never over something typed since. The + // name rather than the object, because that is the half the diver can edit + // while a reply is in the air - the rest of a place only ever arrives with + // a pick, and a pick bumps the counter above. + const nameAtPick = location?.name; setGeocoded(null); geocodingAPI .reverseGeocode(Number(placed.latitude), Number(placed.longitude)) @@ -153,7 +164,7 @@ export function useGeocodedLocation({ // cost; "typing after placing the pin gets overwritten anyway" is not, // and it is the very thing the `unknown`-does-not-clear rule below // protects. - if (live.location !== locationAtPick) return; + if (live.location?.name !== nameAtPick) return; // An `unknown` outcome is not an answer about the position - geocoding // switched off, the instance over its provider cap (one request a // second, counted across everybody), or the provider timing out. None of @@ -167,7 +178,8 @@ export function useGeocodedLocation({ // the way `isSet` in `lib/validations/dive-site.ts` treats every other // field here: a stray space is not something a diver typed on purpose, // and emptying it looks identical on screen to emptying nothing. - if (outcome.status === "nameless" && !live.location?.trim()) return; + if (outcome.status === "nameless" && !live.location?.name?.trim()) + return; // Whereas `nameless` *is* an answer: the API looked the position up and // there is no name there, so whatever is in the field describes where // the pin used to be and goes. Both branches write straight into the @@ -176,17 +188,31 @@ export function useGeocodedLocation({ // the trade being that moving the pin afterwards writes over whatever // they typed, since a placement is what this is answering. const result = outcome.status === "named" ? outcome.result : null; + // A reverse-geocoded place is a name and nothing else. The coordinates + // that came back describe where the *site* is - it is the pin the diver + // just dropped that was looked up - so writing them into the locality's + // centre would claim the town sits exactly on the wreck. The two are + // different facts, and this is the one path where they are guaranteed + // to be the same point, so nothing on screen would show the mistake. + // The provider's full label is left behind for the same reason: this + // answered "what is here", not "where is that place". + const place: AdoptedPlace | null = result + ? { + location: { name: result.location }, + attribution: result.attribution, + } + : null; setGeocoded({ ...placed, - place: result, + place, // Said out loud because the Location field writes itself a round trip // after the position was placed, and nobody is looking at it when it // happens - least of all when what it did was empty the field. - announcement: result - ? `Location set to ${result.location}.` + announcement: place + ? `Location set to ${place.location.name}.` : "This position has no name, so the location was cleared.", }); - onUseLocation(result?.location ?? ""); + onUseLocation(place?.location ?? null); }) // A failure is not an answer, so the location is left exactly as it is. // Geocoding is optional on the API - it can be switched off, the provider @@ -210,10 +236,10 @@ export function useGeocodedLocation({ // announces the placement alone - saying the location was set would be // describing something that did not happen. announcement: place - ? `Placed at ${placed.latitude}, ${placed.longitude}. Location set to ${place.location}.` + ? `Placed at ${placed.latitude}, ${placed.longitude}. Location set to ${place.location.name}.` : `Placed at ${placed.latitude}, ${placed.longitude}.`, }); - // Deliberately not called with `""` for a row that named nowhere. Emptying + // Deliberately not called with `null` for a row that named nowhere. Emptying // the field would be claiming the pick answered a question it never asked - // the same distinction `lookup` draws between a `nameless` position and an // `unknown` one, and only the first is grounds to clear what a diver typed. diff --git a/src/lib/api/dive-sites.ts b/src/lib/api/dive-sites.ts index cfe64386..46d36e06 100644 --- a/src/lib/api/dive-sites.ts +++ b/src/lib/api/dive-sites.ts @@ -1,6 +1,12 @@ import { apiClient } from "./client"; import type { PaginatedResponse } from "./client"; +import type { Location } from "./location"; +// A site carries two positions that mean different things: `latitude`/ +// `longitude` are the pin a diver dropped, and `location.latitude`/ +// `location.longitude` are the centre of the town the geocoder resolved. +// Nothing fills either from the other. +// // `latitude`/`longitude` are both-or-neither on the API, and the rule is about // the request body rather than the resulting row: naming one without the other // is a 422, so is naming both with only one value, and the stored row is never @@ -10,7 +16,7 @@ import type { PaginatedResponse } from "./client"; export interface DiveSite { uuid: string; name: string; - location?: string; + location?: Location | null; latitude?: number | null; longitude?: number | null; notes?: string; @@ -20,15 +26,24 @@ export interface DiveSite { export interface DiveSiteCreate { name: string; - location?: string; + location?: Location | null; latitude?: number | null; longitude?: number | null; notes?: string; } +// The `PATCH /dive-site/{uuid}` body, which is the API's nested +// `DiveSiteUpdateRequest` rather than the flat `DiveSiteUpdate` beside it - +// that one is the admin panel's form and is swept against the table's columns, +// so it spells the locality as eight `location_*` fields and cannot nest. +// +// Naming `location` **replaces** the stored place wholesale: it is a value +// object with nothing to merge into, and a partial update would leave a cleared +// locality's centre and box behind. An explicit `null` clears it, which is how +// a site entered with the wrong place is corrected back to "not recorded". export interface DiveSiteUpdate { name?: string; - location?: string; + location?: Location | null; latitude?: number | null; longitude?: number | null; notes?: string; @@ -38,8 +53,8 @@ export type PaginatedDiveSitesResponse = PaginatedResponse; /** * Dive-site CRUD. `getDiveSites` takes a `search` the API matches server-side against - * name and location, which is what lets the dive form's picker narrow as you type instead - * of loading a diver's whole site list. + * the site's name and both of its locality's names, which is what lets the dive form's + * picker narrow as you type instead of loading a diver's whole site list. */ export const diveSitesAPI = { // Create a new dive site, owned by the signed-in user. @@ -49,8 +64,9 @@ export const diveSitesAPI = { }, // Get a user's dive sites (paginated, name-ascending). `search` narrows to sites - // whose name *or* location contains it, case-insensitively - the API caps - // `items_per_page` at 100, so this is a page of matches, never the whole set. + // whose name *or* either of their locality's names contains it, case-insensitively - + // the API caps `items_per_page` at 100, so this is a page of matches, never the whole + // set. async getDiveSites( page: number = 1, items_per_page: number = 10, diff --git a/src/lib/api/dives.ts b/src/lib/api/dives.ts index cc395bd9..f7c512fe 100644 --- a/src/lib/api/dives.ts +++ b/src/lib/api/dives.ts @@ -1,6 +1,7 @@ import { apiClient } from "./client"; import type { PaginatedResponse } from "./client"; import { GearItemSummary } from "./gear"; +import type { Location } from "./location"; import { SpeciesSummary } from "./species"; // A single gas mixture / scuba tank used during a dive. @@ -100,7 +101,10 @@ export interface DiveMixture { export interface DiveSiteSummary { uuid: string; name: string; - location?: string; + // The whole place, not a trimmed copy of it: one shape named `location` on + // the wire wherever it appears. Its own coordinates are the locality's + // centre, a different fact from the site's pin below. + location?: Location | null; // Where the site is, so a dive can be mapped from its own response instead of // fetching every linked site separately. The API kept these off the embedded // summary while no map view existed (see its DECISIONS.md); the dive page's diff --git a/src/lib/api/geocoding.ts b/src/lib/api/geocoding.ts index 72d00f53..2f5a7d81 100644 --- a/src/lib/api/geocoding.ts +++ b/src/lib/api/geocoding.ts @@ -14,9 +14,10 @@ export const MAX_PLACE_QUERY_LENGTH = 200; * One place, normalized by the API away from whichever provider answered. * * `location` and `display_name` answer different questions. `location` is the - * short, composed form that goes onto `dive_site.location` - divers write - * "Dahab, Egypt", not a seven-part postal address. `display_name` is the - * provider's full label, which is what tells two similar results apart. + * short, composed form that becomes a place's `name` - divers write "Dahab, + * Egypt", not a seven-part postal address. `display_name` is the provider's + * full label, which is what tells two similar results apart in a menu and what + * becomes the place's `full_name`, stored for the export and rendered nowhere. * * `attribution` rides on each result rather than in an envelope because it is a * licence condition of the data itself, and it must be rendered wherever the diff --git a/src/lib/api/location.ts b/src/lib/api/location.ts new file mode 100644 index 00000000..8eaf7e65 --- /dev/null +++ b/src/lib/api/location.ts @@ -0,0 +1,40 @@ +/** + * A place, as the geocoder described it when the diver picked it - or as they + * typed it. + * + * One object with one pair of names, carried by a dive site and by a part of a + * trip alike. It lives in its own module because both hosts reference it and + * neither owns it: putting it in either one would make the other import a + * sibling resource's type for a shape that is not about that resource at all. + * + * A value object, not a resource: it has no uuid, it belongs to exactly one + * host, and it is a snapshot of what the geocoder said at the time rather than + * a row in a shared gazetteer. So a write replaces the stored one wholesale, + * and clearing it is an explicit `null`. + * + * **Two names, and only the short one is ever rendered.** `name` is the place + * as a person writes it - the name alone ("Moalboal"), or the name with its + * country ("Dahab, Egypt"). `full_name` is the fullest written form the lookup + * returned ("Dahab, South Sinai Governorate, Egypt"); it is stored so an export + * carries what the source held, and no surface in this app shows it. Nothing + * binds the two: a lookup asked about a local name often answers with the + * district around it, so "Sipadan Island Park" may carry "Sabah, Malaysia" - + * shorter, and not containing it. + * + * **A locality's position is not its host's.** A dive site carries its own pin + * as well, and the two are different facts - the entry point against the town + * the geocoder resolved. Nothing fills either from the other. + */ +export interface Location { + name: string; + full_name?: string | null; + latitude?: number | null; + longitude?: number | null; + // The place's extent, when the provider gave one. All four or none: a box is + // only meaningful whole, and it needs a position. West may exceed east - a + // box straddling the antimeridian is not malformed. + bbox_south?: number | null; + bbox_north?: number | null; + bbox_west?: number | null; + bbox_east?: number | null; +} diff --git a/src/lib/api/trips.ts b/src/lib/api/trips.ts index 949ae7cf..50f68834 100644 --- a/src/lib/api/trips.ts +++ b/src/lib/api/trips.ts @@ -1,25 +1,6 @@ import { apiClient } from "./client"; import type { PaginatedResponse } from "./client"; - -/** - * The place half of a trip part, as the API stores it. - * - * A value object, not a resource: it has no uuid, it belongs to exactly one - * part, and it is a snapshot of what the geocoder said at the time rather than a - * row in a shared gazetteer. `name` is the only field that is always there - a - * place typed in by hand, because the geocoder had nothing for it, has a name - * and nothing else. - */ -export interface TripLocation { - name: string; - display_name?: string | null; - latitude?: number | null; - longitude?: number | null; - bbox_south?: number | null; - bbox_north?: number | null; - bbox_west?: number | null; - bbox_east?: number | null; -} +import type { Location } from "./location"; /** * One stretch of a trip: an optional date range and an optional place. @@ -33,7 +14,7 @@ export interface TripLocation { export interface TripPart { start_date?: string | null; end_date?: string | null; - location?: TripLocation | null; + location?: Location | null; } // What a write sends. Identical in shape to `TripPart` - parts are replaced diff --git a/src/lib/locations.test.ts b/src/lib/locations.test.ts new file mode 100644 index 00000000..fa77c9ee --- /dev/null +++ b/src/lib/locations.test.ts @@ -0,0 +1,177 @@ +import { describe, expect, it } from "vitest"; +import { formatLocationContext, geocodeResultToLocation } from "./locations"; +import type { GeocodeResult } from "@/lib/api/geocoding"; + +describe("formatLocationContext", () => { + it("drops the name the label repeats at the front", () => { + // The whole point: the row shows the name and then this, and Nominatim's + // label opens with the name it was matched by. + expect( + formatLocationContext({ + name: "Dahab", + full_name: "Dahab, South Sinai, 45214, Egypt", + }), + ).toBe("South Sinai, 45214, Egypt"); + expect( + formatLocationContext({ + name: "Ko Tao", + full_name: + "Ko Tao, Ko Pha-ngan District, Surat Thani Province, Thailand", + }), + ).toBe("Ko Pha-ngan District, Surat Thani Province, Thailand"); + }); + + it("keeps a repeat that is not at the front", () => { + // "Dahab" is context for a site called Blue Hole, not a duplicate of it - + // and it is the context that tells two Blue Holes apart. + expect( + formatLocationContext({ + name: "Blue Hole", + full_name: "Blue Hole, Dahab, South Sinai, Egypt", + }), + ).toBe("Dahab, South Sinai, Egypt"); + }); + + it("keeps a part the name only prefixes", () => { + // Matching on the whole part, not on the characters: "Ko Tao" must not eat + // the front of "Ko Tao Island". + expect( + formatLocationContext({ + name: "Ko Tao", + full_name: "Ko Tao Island, Surat Thani Province, Thailand", + }), + ).toBe("Ko Tao Island, Surat Thani Province, Thailand"); + }); + + it("answers undefined when the label says no more than the name", () => { + // So the caller drops the element rather than rendering an empty one. + expect( + formatLocationContext({ name: "Bohol", full_name: "Bohol" }), + ).toBeUndefined(); + expect( + formatLocationContext({ name: "Bohol", full_name: " bohol " }), + ).toBeUndefined(); + }); + + it("has nothing to say about a place with no label", () => { + // Nothing to trim and nothing left over, so the caller drops the hint. + expect(formatLocationContext({ name: "The Boat" })).toBeUndefined(); + expect( + formatLocationContext({ name: "The Boat", full_name: null }), + ).toBeUndefined(); + expect( + formatLocationContext({ name: "The Boat", full_name: " " }), + ).toBeUndefined(); + }); + + it("gives the whole label to a place with no name to trim off it", () => { + expect( + formatLocationContext({ full_name: "Dahab, South Sinai, Egypt" }), + ).toBe("Dahab, South Sinai, Egypt"); + expect( + formatLocationContext({ + name: " ", + full_name: "Dahab, South Sinai, Egypt", + }), + ).toBe("Dahab, South Sinai, Egypt"); + }); + + it("trims each part of the label it keeps", () => { + // The label is the provider's, and its spacing is not this app's to + // reproduce faithfully. + expect( + formatLocationContext({ + name: "Dahab", + full_name: "Dahab,South Sinai , Egypt", + }), + ).toBe("South Sinai, Egypt"); + }); + + it("trims a multi-part name the label opens with", () => { + // An address-only result has no name of its own and falls back to the + // composed "Dahab, Egypt", which the label can repeat whole. + expect( + formatLocationContext({ + name: "Dahab, Egypt", + full_name: "Dahab, Egypt, South Sinai", + }), + ).toBe("South Sinai"); + }); +}); + +describe("geocodeResultToLocation", () => { + const DAHAB: GeocodeResult = { + latitude: 28.4949, + longitude: 34.5136, + location: "Dahab, Egypt", + display_name: "Dahab, South Sinai, 45214, Egypt", + name: "Dahab", + attribution: "Data © OpenStreetMap contributors, ODbL 1.0.", + bbox_south: 28.45, + bbox_north: 28.54, + bbox_west: 34.47, + bbox_east: 34.55, + }; + + it("names the place the way a person writes it, not the way a provider does", () => { + // The API's composed place-plus-country, which is what every surface + // renders - never the provider's postal chain, which nothing does. + expect(geocodeResultToLocation(DAHAB).name).toBe("Dahab, Egypt"); + }); + + it("keeps the provider's whole label as the fuller form", () => { + // Stored so an export carries what the source held. It is not derivable + // from the short form, which is the whole reason it is kept at all. + expect(geocodeResultToLocation(DAHAB).full_name).toBe( + "Dahab, South Sinai, 45214, Egypt", + ); + }); + + it("ignores the result's own bare name, which is not what a log records", () => { + // "Dahab" alone says nothing about which Dahab. It still has a job in the + // menu, where it is the row's title - but not in the field. + expect(geocodeResultToLocation({ ...DAHAB, name: "Dahab" }).name).toBe( + "Dahab, Egypt", + ); + // And an address-only result has no name at all, which used to be why the + // mapping needed a fallback. + expect(geocodeResultToLocation({ ...DAHAB, name: null }).name).toBe( + "Dahab, Egypt", + ); + }); + + it("takes the centre and the extent a forward search answered with", () => { + // These are the *place's*, which is what a search by name returns. A + // reverse geocode does not come through here, because its coordinates are + // the host's own pin. + expect(geocodeResultToLocation(DAHAB)).toEqual({ + name: "Dahab, Egypt", + full_name: "Dahab, South Sinai, 45214, Egypt", + latitude: 28.4949, + longitude: 34.5136, + bbox_south: 28.45, + bbox_north: 28.54, + bbox_west: 34.47, + bbox_east: 34.55, + }); + }); + + it("leaves the box out whole when the provider gave none", () => { + // The API sends all four or nothing, and a half box is refused on the way + // back in - so an absent one must stay absent rather than become a corner. + expect( + geocodeResultToLocation({ + ...DAHAB, + bbox_south: null, + bbox_north: null, + bbox_west: null, + bbox_east: null, + }), + ).toMatchObject({ + bbox_south: null, + bbox_north: null, + bbox_west: null, + bbox_east: null, + }); + }); +}); diff --git a/src/lib/locations.ts b/src/lib/locations.ts new file mode 100644 index 00000000..0a2ae942 --- /dev/null +++ b/src/lib/locations.ts @@ -0,0 +1,93 @@ +// A place, between the geocoder and the two forms that record one. Both the +// dive site form and a trip part's row fill the same object from the same +// response, so the mapping is written once here rather than slightly +// differently in each of them. + +import type { GeocodeResult } from "@/lib/api/geocoding"; +import type { LocationFormValue } from "@/lib/validations/location"; + +/** + * A geocoder result as a place. + * + * `name` is the API's composed short form - the place and its country, "Dahab, + * Egypt" - because that is the place as a person writes it, and it is what + * every surface renders. `full_name` is the provider's own label, "Dahab, South + * Sinai, 45214, Egypt": a postcode and an administrative level nobody writes in + * a dive log, kept because an export should carry the fullest form the source + * held, and shown nowhere. + * + * The centre and the box are the *place's*, which is what a forward search + * answers with. A reverse geocode must not go through here: the coordinates it + * returns are the host's own position, so adopting them would file the pin a + * diver dropped as the centre of the town around it. + */ +export function geocodeResultToLocation( + result: GeocodeResult, +): LocationFormValue { + return { + name: result.location, + full_name: result.display_name, + latitude: result.latitude, + longitude: result.longitude, + bbox_south: result.bbox_south, + bbox_north: result.bbox_north, + bbox_west: result.bbox_west, + bbox_east: result.bbox_east, + }; +} + +interface LabelledPlace { + name?: string | null; + full_name?: string | null; +} + +/** + * A place's fuller label with the leading repeat of its own name taken off, or + * `undefined` when that leaves nothing. + * + * For a surface that shows a name and a label beside it, where the label starts + * with the name: "Dahab" and "Dahab, Egypt" read together as "Dahab, Dahab, + * Egypt". Only the leading parts the name itself repeats are dropped, so a site + * named "Blue Hole" keeps every word of "Dahab, Egypt" - what goes is a + * duplicate, not context, and the context is the whole reason the label is on + * screen. + * + * Nothing renders a stored place's `full_name`, so the one caller left is the + * dive site place search, which composes both arguments from a `GeocodeResult`: + * the row's own bare name against the API's composed form, which is what + * separates two same-named results in the menu. + * + * `undefined` rather than "" so a caller can drop the element entirely with + * `&&` - a place whose label says no more than its name gets no second line + * rather than an empty one. + */ +export function formatLocationContext( + place: LabelledPlace, +): string | undefined { + const label = labelParts(place.full_name); + const name = labelParts(place.name); + + // Aligned part by part, not "does the label contain the name": "Dahab" is a + // repeat at the front of "Dahab, South Sinai" and a genuine part of "Blue + // Hole, Dahab, South Sinai". + let repeated = 0; + while ( + repeated < name.length && + repeated < label.length && + label[repeated].toLowerCase() === name[repeated].toLowerCase() + ) { + repeated++; + } + + const rest = label.slice(repeated); + return rest.length > 0 ? rest.join(", ") : undefined; +} + +// A comma-separated label as its parts, blanks dropped - which is also what +// makes a missing label an empty list rather than [""]. +function labelParts(label?: string | null): string[] { + return (label ?? "") + .split(",") + .map((part) => part.trim()) + .filter(Boolean); +} diff --git a/src/lib/trip-locations.test.ts b/src/lib/trip-locations.test.ts index b0a8d990..3f605605 100644 --- a/src/lib/trip-locations.test.ts +++ b/src/lib/trip-locations.test.ts @@ -1,6 +1,5 @@ import { describe, expect, it } from "vitest"; import { - formatLocationContext, formatTripLocationNames, formatTripLocationNamesHint, } from "./trip-locations"; @@ -13,7 +12,18 @@ describe("formatTripLocationNames", () => { { name: "Bohol" }, { name: "Malapascua" }, ]), - ).toBe("Moalboal, Bohol, Malapascua"); + ).toBe("Moalboal; Bohol; Malapascua"); + }); + + it("separates the names with a semicolon, not a comma", () => { + // A place's own name carries its country now, so a comma join reads as one + // list of four things rather than two places. + expect( + formatTripLocationNames([ + { name: "Dahab, Egypt" }, + { name: "Sharm El Sheikh, Egypt" }, + ]), + ).toBe("Dahab, Egypt; Sharm El Sheikh, Egypt"); }); it("counts the names past the limit rather than dropping them silently", () => { @@ -27,7 +37,7 @@ describe("formatTripLocationNames", () => { ], { max: 2 }, ), - ).toBe("Moalboal, Bohol +2"); + ).toBe("Moalboal; Bohol +2"); }); it("adds no suffix when the list exactly fills the limit", () => { @@ -35,7 +45,7 @@ describe("formatTripLocationNames", () => { formatTripLocationNames([{ name: "Moalboal" }, { name: "Bohol" }], { max: 2, }), - ).toBe("Moalboal, Bohol"); + ).toBe("Moalboal; Bohol"); }); it("answers undefined for a trip with no locations", () => { @@ -53,7 +63,7 @@ describe("formatTripLocationNames", () => { { name: " " }, { name: "Bohol" }, ]), - ).toBe("Moalboal, Bohol"); + ).toBe("Moalboal; Bohol"); }); it("answers undefined when every name is blank", () => { @@ -83,7 +93,7 @@ describe("formatTripLocationNamesHint", () => { [{ name: "Moalboal" }, { name: "Bohol" }, { name: "Malapascua" }], { max: 2 }, ), - ).toBe("Moalboal, Bohol, Malapascua"); + ).toBe("Moalboal; Bohol; Malapascua"); }); it("answers undefined when the label already shows them all", () => { @@ -119,109 +129,23 @@ describe("formatTripLocationNamesHint", () => { ).toBeUndefined(); }); - it("trims the names it reveals", () => { + it("separates the names it reveals the same way the label does", () => { + // The hint is read against the label it explains, so a reader comparing the + // two must not meet two different list conventions. expect( formatTripLocationNamesHint( - [{ name: " Moalboal " }, { name: " Bohol" }], + [{ name: "Dahab, Egypt" }, { name: "Sharm El Sheikh, Egypt" }], { max: 1 }, ), - ).toBe("Moalboal, Bohol"); + ).toBe("Dahab, Egypt; Sharm El Sheikh, Egypt"); }); -}); -describe("formatLocationContext", () => { - it("drops the name the label repeats at the front", () => { - // The whole point: the row shows the name and then this, and Nominatim's - // label opens with the name it was matched by. - expect( - formatLocationContext({ - name: "Dahab", - display_name: "Dahab, South Sinai, 45214, Egypt", - }), - ).toBe("South Sinai, 45214, Egypt"); - expect( - formatLocationContext({ - name: "Ko Tao", - display_name: - "Ko Tao, Ko Pha-ngan District, Surat Thani Province, Thailand", - }), - ).toBe("Ko Pha-ngan District, Surat Thani Province, Thailand"); - }); - - it("keeps a repeat that is not at the front", () => { - // "Dahab" is context for a site called Blue Hole, not a duplicate of it - - // and it is the context that tells two Blue Holes apart. - expect( - formatLocationContext({ - name: "Blue Hole", - display_name: "Blue Hole, Dahab, South Sinai, Egypt", - }), - ).toBe("Dahab, South Sinai, Egypt"); - }); - - it("keeps a part the name only prefixes", () => { - // Matching on the whole part, not on the characters: "Ko Tao" must not eat - // the front of "Ko Tao Island". - expect( - formatLocationContext({ - name: "Ko Tao", - display_name: "Ko Tao Island, Surat Thani Province, Thailand", - }), - ).toBe("Ko Tao Island, Surat Thani Province, Thailand"); - }); - - it("answers undefined when the label says no more than the name", () => { - // So the caller drops the element rather than rendering an empty one. - expect( - formatLocationContext({ name: "Bohol", display_name: "Bohol" }), - ).toBeUndefined(); - expect( - formatLocationContext({ name: "Bohol", display_name: " bohol " }), - ).toBeUndefined(); - }); - - it("has nothing to say about a place with no label", () => { - // A place typed in by hand, which is every field this can be handed. - expect(formatLocationContext({ name: "The Boat" })).toBeUndefined(); - expect( - formatLocationContext({ name: "The Boat", display_name: null }), - ).toBeUndefined(); - expect( - formatLocationContext({ name: "The Boat", display_name: " " }), - ).toBeUndefined(); - }); - - it("gives the whole label to a place with no name to trim off it", () => { - expect( - formatLocationContext({ display_name: "Dahab, South Sinai, Egypt" }), - ).toBe("Dahab, South Sinai, Egypt"); - expect( - formatLocationContext({ - name: " ", - display_name: "Dahab, South Sinai, Egypt", - }), - ).toBe("Dahab, South Sinai, Egypt"); - }); - - it("trims each part of the label it keeps", () => { - // The label is the provider's, and its spacing is not this app's to - // reproduce faithfully. - expect( - formatLocationContext({ - name: "Dahab", - display_name: "Dahab,South Sinai , Egypt", - }), - ).toBe("South Sinai, Egypt"); - }); - - it("trims a multi-part name the label opens with", () => { - // An address-only result has no name of its own and falls back to the - // composed "Dahab, Egypt", which the label can repeat whole. + it("trims the names it reveals", () => { expect( - formatLocationContext({ - name: "Dahab, Egypt", - display_name: "Dahab, Egypt, South Sinai", - }), - ).toBe("South Sinai"); + formatTripLocationNamesHint( + [{ name: " Moalboal " }, { name: " Bohol" }], + { max: 1 }, + ), + ).toBe("Moalboal; Bohol"); }); }); diff --git a/src/lib/trip-locations.ts b/src/lib/trip-locations.ts index 1185ea88..3876303c 100644 --- a/src/lib/trip-locations.ts +++ b/src/lib/trip-locations.ts @@ -1,20 +1,26 @@ // Where a trip went, as text. Every compact surface - the trips table, the // dashboard's recent-trips card, a detail page subtitle - shows the same joined -// line, and every surface with room for one place's full label trims the same -// repeat out of it, so both are composed once here rather than slightly -// differently in each of them. +// line, so it is composed once here rather than slightly differently in each of +// them. interface NamedLocation { name: string; } +// What separates one place from the next. A semicolon rather than a comma +// because a place's own name carries its country now ("Dahab, Egypt"), so a +// comma join would read as one list of five things instead of two places: +// "Dahab, Egypt, Sharm El Sheikh, Egypt". The same reason the CSV export joins +// a trip's places this way. +const PLACE_SEPARATOR = "; "; + export interface FormatTripLocationNamesOptions { // Show at most this many names, and count the rest as "+N". Omit to show all. max?: number; } /** - * The trip's locations as "Moalboal, Bohol +2", or `undefined` when there are + * The trip's locations as "Dahab, Egypt +2", or `undefined` when there are * none. * * `undefined` rather than "" so callers can pick their own placeholder - the @@ -29,7 +35,7 @@ export function formatTripLocationNames( if (names.length === 0) return undefined; const limit = resolveLimit(names.length, max); - const shown = names.slice(0, limit).join(", "); + const shown = names.slice(0, limit).join(PLACE_SEPARATOR); const hidden = names.length - Math.min(limit, names.length); return hidden > 0 ? `${shown} +${hidden}` : shown; } @@ -49,7 +55,7 @@ export function formatTripLocationNamesHint( const names = usableNames(locations); const limit = resolveLimit(names.length, max); if (names.length <= limit) return undefined; - return names.join(", "); + return names.join(PLACE_SEPARATOR); } // How many names are on screen. Shared rather than repeated in both functions: @@ -59,64 +65,6 @@ function resolveLimit(count: number, max?: number): number { return max !== undefined && max > 0 ? max : count; } -interface LabelledLocation { - name?: string | null; - // The label kept beside the name: "Dahab, Egypt" for a place picked since the - // picker started storing the API's short composed form, and the geocoder's - // own "Dahab, South Sinai, 45214, Egypt" for one saved before it. - display_name?: string | null; -} - -/** - * A location's label with the leading repeat of the place's own name taken off, - * or `undefined` when that leaves nothing. - * - * Every surface that has room for the label shows the name first and the label - * after it, and the label starts with the name: "Dahab" and "Dahab, Egypt" read - * together as "Dahab, Dahab, Egypt". Only the leading parts the name itself - * repeats are dropped, so a site named "Blue Hole" keeps every word of "Dahab, - * Egypt" - what goes is a duplicate, not context, and the context is the whole - * reason the label is on screen. - * - * How short the label is, is not this function's business: it trims a repeat, - * and both the short form and a provider label saved before the picker switched - * to it go through unchanged otherwise. - * - * `undefined` rather than "" so a caller can drop the element entirely with - * `&&` - a place whose label says no more than its name gets no second line - * rather than an empty one. - */ -export function formatLocationContext( - location: LabelledLocation, -): string | undefined { - const label = labelParts(location.display_name); - const name = labelParts(location.name); - - // Aligned part by part, not "does the label contain the name": "Dahab" is a - // repeat at the front of "Dahab, South Sinai" and a genuine part of "Blue - // Hole, Dahab, South Sinai". - let repeated = 0; - while ( - repeated < name.length && - repeated < label.length && - label[repeated].toLowerCase() === name[repeated].toLowerCase() - ) { - repeated++; - } - - const rest = label.slice(repeated); - return rest.length > 0 ? rest.join(", ") : undefined; -} - -// A comma-separated label as its parts, blanks dropped - which is also what -// makes a missing label an empty list rather than [""]. -function labelParts(label?: string | null): string[] { - return (label ?? "") - .split(",") - .map((part) => part.trim()) - .filter(Boolean); -} - // A name is all a location is guaranteed to have, so a blank one is unusable // rather than merely unhelpful - dropping it beats rendering ", , Bohol". function usableNames(locations?: NamedLocation[] | null): string[] { diff --git a/src/lib/trip-parts.ts b/src/lib/trip-parts.ts index 41c2a535..7ae42878 100644 --- a/src/lib/trip-parts.ts +++ b/src/lib/trip-parts.ts @@ -4,7 +4,8 @@ // dashboard card disagreeing about where a trip was is invisible until someone // holds two of them side by side. -import type { TripLocation, TripPart } from "@/lib/api/trips"; +import type { Location } from "@/lib/api/location"; +import type { TripPart } from "@/lib/api/trips"; import { formatTripDateRange } from "@/lib/date-time"; export interface TripSpan { @@ -68,8 +69,8 @@ export function formatTripSpan( * `components/map/locations-map.tsx`). A placeless part contributes nothing * here rather than an unnamed gap. */ -export function tripPartLocations(parts?: TripPart[] | null): TripLocation[] { +export function tripPartLocations(parts?: TripPart[] | null): Location[] { return (parts ?? []) .map((part) => part.location) - .filter((location): location is TripLocation => !!location); + .filter((location): location is Location => !!location); } diff --git a/src/lib/validations/dive-site.test.ts b/src/lib/validations/dive-site.test.ts index b387d6b5..4e2a773e 100644 --- a/src/lib/validations/dive-site.test.ts +++ b/src/lib/validations/dive-site.test.ts @@ -31,7 +31,7 @@ describe("diveSiteFormSchema", () => { it("accepts optional location/notes", () => { const result = diveSiteFormSchema.safeParse({ name: "Blue Hole", - location: "Dahab, Egypt", + location: { name: "Dahab, Egypt" }, notes: "Famous for its arch", }); expect(result.success).toBe(true); diff --git a/src/lib/validations/dive-site.ts b/src/lib/validations/dive-site.ts index b6a8f4e9..782c399b 100644 --- a/src/lib/validations/dive-site.ts +++ b/src/lib/validations/dive-site.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { locationSchema } from "./location"; // Latitude/longitude are edited as free-typed, regex-validated strings and // converted to numbers right before the API call, exactly like the dive form's @@ -31,10 +32,12 @@ export const diveSiteFormSchema = z .string() .min(1, "Dive site name is required") .max(255, "Dive site name cannot exceed 255 characters"), - location: z - .string() - .max(255, "Location cannot exceed 255 characters") - .optional(), + // The whole place, not a text box over its name. The dialog seeds this from + // the site it was handed and PATCHes it back, so a field holding only the + // name would drop a picked locality's full name, its centre and its box on + // every edit of an existing site. `null` is a site with no locality + // recorded, and sending it explicitly is how a wrong one is corrected. + location: locationSchema.nullish(), latitude: coordinateField(90, "Latitude", "27.8506"), longitude: coordinateField(180, "Longitude", "34.3136"), notes: z diff --git a/src/lib/validations/location.ts b/src/lib/validations/location.ts new file mode 100644 index 00000000..0b1af1ec --- /dev/null +++ b/src/lib/validations/location.ts @@ -0,0 +1,55 @@ +import { z } from "zod"; + +// Coordinates here are plain numbers, not the regex-validated strings the dive +// site form uses for the site's own pin (DECISIONS.md): nobody types a +// locality's centre. It arrives whole from a geocoder result the diver picked, +// or it is absent because the place was typed in by hand - there is no +// half-entered "-" state to be tolerant of. The bounds are the API's, so a +// nonsense object is caught before the round trip. +const nullableNumber = (limit: number) => + z.number().min(-limit).max(limit).nullish(); + +// The API's own ceilings, exported because the fields that build a place have +// to enforce them as they go rather than leave them to submit. A place sits +// inside a form value under one `FormField`, and `FormMessage` renders +// `String(error.message)` - which, for an error react-hook-form reports at +// `parts.0.location.name`, is the word "undefined" in red. The diver would be +// told nothing, by a form that had also stopped saving. +export const MAX_LOCATION_NAME_LENGTH = 255; +export const MAX_LOCATION_FULL_NAME_LENGTH = 512; + +/** + * One place as either form holds it - a dive site's locality and a trip part's + * are the same object, so they are the same schema. + * + * `name` is the only member that is always there: a place typed in by hand, + * because the geocoder had nothing for it, has a name and nothing else. + * `full_name` is the fullest form a lookup returned, stored for the export and + * rendered nowhere. + */ +export const locationSchema = z.object({ + name: z + .string() + .min(1, "Location name is required") + .max( + MAX_LOCATION_NAME_LENGTH, + `Location name cannot exceed ${MAX_LOCATION_NAME_LENGTH} characters`, + ), + full_name: z + .string() + .max( + MAX_LOCATION_FULL_NAME_LENGTH, + `Location description cannot exceed ${MAX_LOCATION_FULL_NAME_LENGTH} characters`, + ) + .nullish(), + latitude: nullableNumber(90), + longitude: nullableNumber(180), + bbox_south: nullableNumber(90), + bbox_north: nullableNumber(90), + bbox_west: nullableNumber(180), + bbox_east: nullableNumber(180), +}); + +// One place as a form holds it: self-describing, so a picker never has to +// resolve an id back into something to show. +export type LocationFormValue = z.input; diff --git a/src/lib/validations/trip.ts b/src/lib/validations/trip.ts index bc2961bd..b2f95dbd 100644 --- a/src/lib/validations/trip.ts +++ b/src/lib/validations/trip.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { locationSchema } from "./location"; // Date fields use a plain "YYYY-MM-DD" string (the native value format of // ``) rather than the "" placeholder trick used for @@ -7,47 +8,8 @@ import { z } from "zod"; // back to re-displaying the field's default value. `normalizeTripParts` // converts "" to `undefined` right before sending data to the API. -// Coordinates here are plain numbers, not the regex-validated strings the dive -// site form uses (DECISIONS.md): nobody types these. They arrive whole from a -// geocoder result the diver picked, or they are absent because the place was -// typed in by hand - there is no half-entered "-" state to be tolerant of. The -// bounds are the API's, so a nonsense object is caught before the round trip. -const nullableNumber = (limit: number) => - z.number().min(-limit).max(limit).nullish(); - -// The API's own ceilings, exported because the field has to enforce them as it -// builds the list rather than leave them to submit. A part is an object inside -// an array under one `FormField`, and `FormMessage` renders -// `String(error.message)` - which, for an error react-hook-form reports at -// `parts.0.location.name`, is the word "undefined" in red. The diver would be -// told nothing, by a form that had also stopped saving. -export const MAX_LOCATION_NAME_LENGTH = 255; export const MAX_TRIP_PARTS = 20; -const tripLocationSchema = z.object({ - name: z - .string() - .min(1, "Location name is required") - .max( - MAX_LOCATION_NAME_LENGTH, - `Location name cannot exceed ${MAX_LOCATION_NAME_LENGTH} characters`, - ), - display_name: z - .string() - .max(512, "Location description cannot exceed 512 characters") - .nullish(), - latitude: nullableNumber(90), - longitude: nullableNumber(180), - bbox_south: nullableNumber(90), - bbox_north: nullableNumber(90), - bbox_west: nullableNumber(180), - bbox_east: nullableNumber(180), -}); - -// One location as the form holds it: self-describing, so the picker never has to -// resolve an id back into something to show. -export type TripLocationFormValue = z.input; - // One stretch of a trip. Both halves optional: a transit day is dates with no // place, and a stop whose timing is not filled in yet is a place with no dates. // @@ -56,7 +18,7 @@ export type TripLocationFormValue = z.input; // instead of under the first one. const tripPartSchema = z .object({ - location: tripLocationSchema.nullish(), + location: locationSchema.nullish(), start_date: z.string().optional(), end_date: z.string().optional(), }) From eb86c6867258383d5f3c7c5efaabc88554fa3957 Mon Sep 17 00:00:00 2001 From: Aleksei Vesnin Date: Mon, 21 Sep 2026 21:54:32 +0300 Subject: [PATCH 2/3] fix(location): cap the typed locality, and cover the renamed member The nested schema error has no renderable message, so the Location field enforces its own width. The trip schema's fixtures spelled the old member name, which Zod stripped, so two of them asserted nothing. Co-Authored-By: Claude Opus 5 --- .../sites/dive-site-dialog.render.test.tsx | 18 +++++++++++++ src/components/sites/dive-site-dialog.tsx | 16 +++++++++++- src/lib/validations/trip.test.ts | 26 ++++++++++++++++--- 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/components/sites/dive-site-dialog.render.test.tsx b/src/components/sites/dive-site-dialog.render.test.tsx index 6398984a..8bf3db9f 100644 --- a/src/components/sites/dive-site-dialog.render.test.tsx +++ b/src/components/sites/dive-site-dialog.render.test.tsx @@ -475,6 +475,24 @@ describe("DiveSiteDialog location writes", () => { expect(updateDiveSite.mock.calls[0][1].location).toBeNull(); }); + it("stops a typed name at the width the API stores", async () => { + // Not left to the resolver: the failure would land at `location.name`, + // where `FormMessage` reads `errors.location` and finds a container with + // no message - the word "undefined" in red, over a save that stopped. + renderEdit(); + const location = screen.getByLabelText("Location") as HTMLInputElement; + expect(location.maxLength).toBe(255); + + await userEvent.clear(location); + await userEvent.paste("a".repeat(300)); + await save(); + + await waitFor(() => expect(updateDiveSite).toHaveBeenCalled()); + expect(updateDiveSite.mock.calls[0][1].location).toEqual({ + name: "a".repeat(255), + }); + }); + it("replaces the place outright when a new name is typed over it", async () => { // A full name, a centre and an extent resolved for Dahab say nothing true // about Moalboal, so they go with the name they belonged to rather than diff --git a/src/components/sites/dive-site-dialog.tsx b/src/components/sites/dive-site-dialog.tsx index 9e295ef5..d0ee895a 100644 --- a/src/components/sites/dive-site-dialog.tsx +++ b/src/components/sites/dive-site-dialog.tsx @@ -16,7 +16,10 @@ import { import { diveSitesAPI, DiveSite } from "@/lib/api/dive-sites"; import { GeocodeResult } from "@/lib/api/geocoding"; import { geocodeResultToLocation } from "@/lib/locations"; -import type { LocationFormValue } from "@/lib/validations/location"; +import { + MAX_LOCATION_NAME_LENGTH, + type LocationFormValue, +} from "@/lib/validations/location"; import { diveSitePlaceContext, DiveSiteSuggestion, @@ -341,6 +344,17 @@ export function DiveSiteDialog({ { parts: [ { location: { - name: "Moalboal", - display_name: "Moalboal, Cebu, Central Visayas, Philippines", + name: "Moalboal, Philippines", + full_name: "Moalboal, Cebu, Central Visayas, Philippines", latitude: 9.9366, longitude: 123.396, bbox_south: 9.87, @@ -102,6 +102,11 @@ describe("tripFormSchema", () => { ], }); expect(result.success).toBe(true); + // Named, because an unknown member is stripped rather than refused: a + // fixture spelling this wrong would pass while covering nothing. + expect(result.data?.parts?.[0].location?.full_name).toBe( + "Moalboal, Cebu, Central Visayas, Philippines", + ); }); it("accepts the nulls the API sends for an unknown position", () => { @@ -113,7 +118,7 @@ describe("tripFormSchema", () => { { location: { name: "Somewhere", - display_name: null, + full_name: null, latitude: null, longitude: null, bbox_south: null, @@ -127,6 +132,16 @@ describe("tripFormSchema", () => { expect(result.success).toBe(true); }); + it("rejects a fuller name past the API's ceiling", () => { + const result = tripFormSchema.safeParse({ + ...validTrip, + parts: [ + { location: { name: "Dahab, Egypt", full_name: "a".repeat(513) } }, + ], + }); + expect(result.success).toBe(false); + }); + it("rejects a place with no name", () => { const result = tripFormSchema.safeParse({ ...validTrip, @@ -178,7 +193,10 @@ describe("normalizeTripParts", () => { }); it("carries a place through untouched", () => { - const location = { name: "Dahab", display_name: "Dahab, Egypt" }; + const location = { + name: "Dahab, Egypt", + full_name: "Dahab, South Sinai, Egypt", + }; expect(normalizeTripParts([{ location }])[0].location).toEqual(location); }); From 733db1f39677bc3b94cf254269bee80c13a28585 Mon Sep 17 00:00:00 2001 From: Aleksei Vesnin Date: Mon, 21 Sep 2026 22:09:28 +0300 Subject: [PATCH 3/3] refactor(location): the trip subtitle renders the label rather than joining The cap belongs to one component, which every surface that joins a trip's places renders - so the constant stays private and the page's own call is a presence check. Co-Authored-By: Claude Opus 5 --- DECISIONS.md | 22 +++++++++---------- src/app/trips/[id]/page.tsx | 17 +++++--------- src/components/trips/trip-locations-label.tsx | 13 +++++------ 3 files changed, 23 insertions(+), 29 deletions(-) diff --git a/DECISIONS.md b/DECISIONS.md index df82753a..9edfdf93 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2773,12 +2773,13 @@ same blank-dropping rule: `["Moalboal", " "]` under `max: 1` shows no "+N" and g `TripLocationsLabel` calls both, as `DiveSitesLabel` does for dives; the trips table and the dashboard card pass locations and a fallback only. -`SHOWN_LOCATIONS` is **one**, and exported rather than private. One because a place's name carries -its country now, so two of "Dahab, Egypt" do not fit a table cell; exported because the trip page's -subtitle joins the same names without rendering this component, and a surface capping differently -would contradict a "+N" beside it. Every such list separates with `; ` — a comma is -indistinguishable from the commas inside each name. `LocationsMap`'s accessible label takes the -separator and not the cap: a cap withholds names from the one reader who cannot count the pins. +`SHOWN_LOCATIONS` is **one**: a place's name carries its country, so two of "Dahab, Egypt" do not +fit a table cell. It stays private because every surface that joins a trip's places for a reader — +the trips table, the dashboard card, the dive sidebar and the trip page's own subtitle — renders +this component, so there is one number and nothing for a second one to disagree with. Every such +list separates with `; `, a comma being indistinguishable from the commas inside each name. +`LocationsMap`'s accessible label takes the separator and not the cap: a cap withholds names from +the one reader who cannot count the pins. `title` answers a mouse and nobody else: no hover on touch, unreachable by keyboard on a ``. The alternative is a `Popover` trigger nested in a link; hover-only stands until the app has a @@ -3342,11 +3343,10 @@ Costs: a place saved before this keeps the provider's label as its name until re `locationKey` (`geo:{lat}:{lon}:{full_name}`) keys it differently from a fresh pick of the same place. -The provider's whole label is now stored beside it as `full_name` — the second stored field this -entry used to reject, which DiveJSON asks for. It is written and rendered nowhere, so the choice -above is unchanged for every reader; what it settles is the search cost this entry recorded, the API -matching a term against both text columns and `full_name` carrying the region `_short_location` -omits. +The provider's whole label is stored beside it as `full_name`, because the format asks for the +fullest form the source held. It is written on every geocoded pick and read by `locationKey`, and no +surface renders it, so the choice above decides everything a diver sees. It also carries the region +`_short_location` omits, which is what the API matches a search term against alongside the name. The dive site place search's menu hint shows the short form too; `ComboboxItem` has only `id`, `name`, `hint`. `placeKey` in `place-search.tsx` keys on the provider's label. diff --git a/src/app/trips/[id]/page.tsx b/src/app/trips/[id]/page.tsx index b95dc39d..1636c879 100644 --- a/src/app/trips/[id]/page.tsx +++ b/src/app/trips/[id]/page.tsx @@ -8,10 +8,7 @@ import { useDeleteResource } from "@/hooks/useDeleteResource"; import { tripsAPI, Trip } from "@/lib/api/trips"; import { formatDateTime, formatTripDateRange } from "@/lib/date-time"; import { formatTripLocationNames } from "@/lib/trip-locations"; -import { - SHOWN_LOCATIONS, - TripLocationsLabel, -} from "@/components/trips/trip-locations-label"; +import { TripLocationsLabel } from "@/components/trips/trip-locations-label"; import { formatTripSpan, tripPartLocations } from "@/lib/trip-parts"; import { RecentDivesCard } from "@/components/dives/recent-dives-card"; import { Button } from "@/components/ui/button"; @@ -66,13 +63,11 @@ export default function TripDetailPage() { const tripDateRange = formatTripSpan(tripParts, LONG_DATE); const tripLocations = tripPartLocations(tripParts); - // Only whether there is a place to name; the text and its hover hint are - // `TripLocationsLabel`'s, under the same cap the trips table and the - // dashboard card use. A subtitle that joined every name would turn a - // three-place trip into six comma-separated segments. - const tripLocationNames = formatTripLocationNames(tripLocations, { - max: SHOWN_LOCATIONS, - }); + // Only whether there is a place to name, which is what decides the separator + // below. The subtitle's own text and hover hint come from rendering + // `TripLocationsLabel`, so the cap lives there with the trips table's and the + // dashboard card's rather than being passed a second time from here. + const tripLocationNames = formatTripLocationNames(tripLocations); // Only places the geocoder gave a position to can be drawn; the parts below // list all of them either way, so a typed-in place isn't silently dropped. const mappedLocations = tripLocations.filter( diff --git a/src/components/trips/trip-locations-label.tsx b/src/components/trips/trip-locations-label.tsx index 400735e3..c560214c 100644 --- a/src/components/trips/trip-locations-label.tsx +++ b/src/components/trips/trip-locations-label.tsx @@ -6,14 +6,13 @@ import { } from "@/lib/trip-locations"; // How many names a compact surface shows before the rest become "+N". One, -// because a place's own name carries its country now - two of "Dahab, Egypt" -// do not fit a table cell, and the hover hint is what keeps the count honest. +// because a place's own name carries its country - two of "Dahab, Egypt" do not +// fit a table cell, and the hover hint is what keeps the count honest. // -// Exported because the trip page's subtitle joins the same names without -// rendering this component, and the two agreeing is the invariant: every -// surface that joins a trip's places for a reader shows the same number of -// them. -export const SHOWN_LOCATIONS = 1; +// Private, as the limit for every surface that joins a trip's places for a +// reader: each of them renders this component rather than joining the names +// itself, so there is one number and nothing for a second one to disagree with. +const SHOWN_LOCATIONS = 1; export interface TripLocationsLabelProps { locations?: Location[] | null;