que01
e08af11b7d
Three of this task's defects were cross-layer wiring rather than logic, so they belong in the spec where the next session will read them, not only in the task's research notes. Frontend: a new client spec covering layer ownership (baseline is never written, preview hides rather than overwrites), why a handle carries only its id, why a handle's position must derive from the clamped value, the ban on computing inside a setState updater, and why per-pointermove React state is the wrong owner of per-frame feedback. Backend: the left/right convention now states the three places that must agree and why a sign convention can be wrong on both sides of the wire at once; a table of the four profile fields the geometry stage actually reads, so nobody builds UI for interval-scoped editing again before the compiler honours it; and the save-is-not-applying contract, including that compileFresh must be installed by every path that sets session.area. Also records that an identity test cannot catch an ignored editsFile, and that a regression test is only trustworthy once it has been seen to fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
165 lines
6.6 KiB
Markdown
165 lines
6.6 KiB
Markdown
# Direct Edit Client
|
|
|
|
> Trigger: any change under `workbench/client/src/edit/`, `MapCanvas.tsx`, or the
|
|
> direct-edit wiring in `App.tsx`. Read this before editing handles, ghosts,
|
|
> previews, or the save path.
|
|
|
|
Related: [Direct Edit Solver And API](../backend/direct-edit-api.md) for the wire
|
|
contract this client consumes.
|
|
|
|
---
|
|
|
|
## 1. Layer ownership
|
|
|
|
Four layer groups, and only three of them are ever written by the client:
|
|
|
|
| Group | Written by | Rule |
|
|
| --- | --- | --- |
|
|
| baseline (`createLayers()`) | `updateLayers()` only | **Never** written by edit code |
|
|
| `editHandles` | `EditHandleLayer.render()` | Handle features carry only `handleId` |
|
|
| `editGhost` | `EditGhostLayer` | Client-side, non-authoritative |
|
|
| `editPreview` | `EditPreviewLayer.show()` | Server geometry; **hides** baseline, never overwrites it |
|
|
|
|
`EditPreviewLayer` builds its own layer set from the same `createLayers()` factory
|
|
so preview and baseline cannot drift in styling, and `show()` hides only the
|
|
baseline layers it actually supersedes. `clear()` restores visibility from the
|
|
layer switches, never from a hardcoded default.
|
|
|
|
**Forbidden**: writing compiler output into a baseline source. It is the invariant
|
|
the whole split exists to protect, and nothing in the type system enforces it.
|
|
|
|
---
|
|
|
|
## 2. A handle carries nothing but its id
|
|
|
|
Handle features hold `handleId` and nothing else. Kind, anchor, axis, range and
|
|
disabled reason are looked up in the manifest. That is why `EditHandleLayer` owns
|
|
both the source and the index — they cannot fall out of step.
|
|
|
|
**Forbidden**: copying manifest fields onto the feature. It creates a second
|
|
constraint model on the client, which is the failure the cross-layer guide warns
|
|
about.
|
|
|
|
---
|
|
|
|
## 3. Handle position comes from the value, never the cursor
|
|
|
|
`handlePositionFor(handle, value)` is the inverse of `projectHandleValue()`. A
|
|
drag projects onto the manifest's axis, clamps to the manifest's range, and the
|
|
handle is then placed from the clamped value.
|
|
|
|
This is not cosmetic. The ol-ext probe translated its proxy by the raw pointer
|
|
delta, and a drag reading `-24.146 m` left the handle 24 m out while the
|
|
constraint clamped at `-5.400 m` — the handle pointed at a road shape that cannot
|
|
exist. Any input adapter must own the position update for this reason.
|
|
|
|
---
|
|
|
|
## 4. Sides: left is `tangent - 90`
|
|
|
|
The manifest reports `axisAzimuth = tangent + 90` for **both** sides, and that
|
|
azimuth is the geometry compiler's *right*. See the backend spec for why. On the
|
|
client this means `outwardSign()` negates the left side, and a test that asserts a
|
|
side must name the direction geographically (`OUTWARD_LEFT` / `OUTWARD_RIGHT`)
|
|
rather than by axis sign.
|
|
|
|
**Mistake made**: the first implementation assumed left was the positive axis, so
|
|
dragging the visually-left handle moved the right kerb. Both the server placement
|
|
and the client sign were wrong together, so neither side's tests caught it.
|
|
|
|
---
|
|
|
|
## 5. A constraint must travel with its operation
|
|
|
|
`validateEditDocument()` rejects any constraint whose `provenance.operationId` is
|
|
not a recorded operation, and the check covers already-saved constraints too.
|
|
Build the pair with `draftConstraint()` + `operationFor()`, and send
|
|
`EditSession.fragment()`, which assembles both halves.
|
|
|
|
One gesture mints one operation id, released on pointerup. Reusing an id across
|
|
gestures puts two operations with the same id in the document, which is also
|
|
rejected.
|
|
|
|
**Mistake made**: sending `constraints` alone returned HTTP 400 on every drag.
|
|
|
|
---
|
|
|
|
## 6. Never compute inside a `setState` updater
|
|
|
|
React defers updater functions and, under StrictMode, calls them more than once.
|
|
By the time one runs, closure variables captured during the gesture may already be
|
|
cleared.
|
|
|
|
```ts
|
|
// WRONG — threw `toLonLat(null)` and white-screened the page
|
|
setReport((current) => ({ ...current, meters: projectHandleValue(handle, start!, now) }));
|
|
|
|
// RIGHT — compute in the event handler, pass plain values in
|
|
const meters = projectHandleValue(handle, start, now);
|
|
setReport((current) => ({ ...current, meters }));
|
|
```
|
|
|
|
The `start!` non-null assertion is what hid the runtime problem from the type
|
|
checker. The same rule covers reading live OpenLayers state inside an updater: it
|
|
samples a different moment than the event did.
|
|
|
|
---
|
|
|
|
## 7. Per-frame feedback does not go through React
|
|
|
|
The ghost writes its OpenLayers source directly. Driving a readout from state on
|
|
every `pointermove` produced ~1600 renders in a single probe session.
|
|
`requestAnimationFrame` coalescing is the minimum; writing straight to the source
|
|
is the rule for production.
|
|
|
|
---
|
|
|
|
## 8. `EditSession` owns the constraint set, and is mutated in place
|
|
|
|
`EditSession` holds the saved baseline, its operations, the command stack, and the
|
|
`previewSeq` watermark. `PreviewRequester` captures the session object once, so
|
|
adopting a server document uses `load()` rather than constructing a new session —
|
|
a swapped object would leave the requester arbitrating for a session nobody reads.
|
|
|
|
`previewSeq` arbitration lives in `acceptPreview()`, not in the request layer: the
|
|
request layer only sends and cancels. The discard rule is worth unit-testing and
|
|
must not depend on network timing.
|
|
|
|
Undo of a **saved** gesture appends an inverse operation; persisted history is
|
|
never rewritten. Undo of an unsaved one just moves the cursor.
|
|
|
|
---
|
|
|
|
## 9. The `directEdit` flag means nothing exists
|
|
|
|
With the flag off: no edit source is created, no layer is added, no interaction is
|
|
registered, and no manifest request is made. The network trace and the canvas must
|
|
match `main` exactly. Guard once at construction, not per call site.
|
|
|
|
`intervalEditingSupported` is a second, narrower switch: range handles stay hidden
|
|
while `compileGeometry()` ignores `profile.interval`. A control whose drag changes
|
|
nothing is worse than no control.
|
|
|
|
---
|
|
|
|
## 10. Saving is not the same as applying
|
|
|
|
`POST /api/edits` writes the document; `/api/state` serves the outputs of the last
|
|
compile. Saving without recompiling leaves a refresh showing the pre-edit geometry,
|
|
which is indistinguishable from a failed save. Report the save and the recompile
|
|
separately — if the recompile fails, the save still succeeded and the message must
|
|
say so.
|
|
|
|
---
|
|
|
|
## Checklist before committing edit-client changes
|
|
|
|
- [ ] No baseline source is written
|
|
- [ ] No manifest field is copied onto a feature
|
|
- [ ] Handle positions derive from clamped values
|
|
- [ ] Sides asserted geographically, not by axis sign
|
|
- [ ] Constraints sent with their operations
|
|
- [ ] No geometry or OL reads inside a `setState` updater
|
|
- [ ] No per-`pointermove` React state updates
|
|
- [ ] Flag off ⇒ no sources, no layers, no interactions, no requests
|