diff --git a/DECISIONS.md b/DECISIONS.md index 266025ec..9edfdf93 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,25 @@ 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**: 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 @@ -3301,39 +3313,43 @@ 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. -Menu hints show it too; `ComboboxItem` has only `id`, `name`, `hint`. `placeKey` in -`place-search.tsx` keys on the provider's label. +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. ## 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.test.ts b/src/lib/validations/trip.test.ts index 454070ba..00def51c 100644 --- a/src/lib/validations/trip.test.ts +++ b/src/lib/validations/trip.test.ts @@ -89,8 +89,8 @@ describe("tripFormSchema", () => { 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); }); 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(), })