# Review, master (uncommitted), 2026-10-27

**Reviewed by**: sonnet (author on composer)
**Scope**: 9 files (excluding generated min.css), uncommitted vs master
**Verdict**: Changes requested

## Summary
Adds the spec 0002 attendance locations feature: a role-gated list page, a Leaflet map editor, one engine, one AJAX dispatcher, a Nominatim proxy and branch-school copies. The structure follows AGENTS.md (engine takes mysqli + $ctx, never reads superglobals, prepared statements, JSON flags, CSRF, tenant `select_db` restored in `finally`) and I verified the Leaflet SRI hashes against jsDelivr. One real usability bug (the radius number input clobbers typing) is a major; the rest is minor.

## Major
### 🟠 Radius number field rewrites the value on every keystroke, `app/admin/scripts/staff-attendance-location-edit.js:408`
**Problem**: `number.addEventListener('input')` calls `setRadius`, which runs `clampRadius` (min 20, round to step 10) and writes the clamped value back into the input. Typing "1" instantly becomes "20", then "0" makes "200", then "0" makes "2000". Typing 500 or 150 by hand is effectively impossible.
**Why it matters**: The number field is half of the spec's AC-3 radius control (slider + number). On a phone keyboard admins cannot enter a value without fighting the field.
**Suggested fix**: On `input`, update the circle only when the parsed value is already within 20 to 2000 and do not rewrite the field. Normalise (round to 10, clamp) and write back on `change`/`blur`. The server already refuses out-of-range radius, so there is no safety loss.

## Minor
### 🟡 Map retry discards unsaved pin and keeps Save enabled, `staff-attendance-location-edit.js:257-290,453`
`retry` calls `initMap` -> `destroyMap`, which drops the marker. An edit page restores only the originally saved point, not an edited one; an add page loses the pin while the Save button stays enabled (submit is guarded by the "Drop a pin" message, so it is not harmful). Remember the marker latlng across re-init, or disable Save again when the marker is cleared.

### 🟡 DB errors are not handled the way the code suggests, `admin/includes/staff_attendance_locations_engine.php` (many `if (!$stmt)` / `if (!$ok)` branches)
On PHP 8.2 mysqli defaults to throwing `mysqli_sql_exception` (no `mysqli_report` call in the repo), so these `sal_fail('Could not save...')` branches are mostly dead on the current-school path. A SQL failure surfaces as a fatal and the generic 500 JSON from `init.php` (no `msg`), not the spec's HTTP 200 `{status:error,msg}`. Only the branch loop catches `Throwable`. Wrap the current-school work in try/catch for `mysqli_sql_exception`, or accept and note it.

### 🟡 "Every other location" on the edit page is capped at 50, `admin/includes/staff_attendance_locations_engine.php:236` and `app/admin/staff_attendance_location_edit.php:26`
`sal_others` reuses `sal_list` (LIMIT 50), so faded circles and the fitBounds logic silently drop rows beyond 50. AC-3 says every other location. Likely fine for this slice (spec accepts the 50 cap on the list), but name-clash and match queries are uncapped, so behaviour differs. Query others without the limit.


### 🟡 Table existence is probed with 4 `SHOW TABLES` per call, `engine.php:53-58`
`sal_tables_ready` runs on every list, get, toggle, delete and save, and `delete`/`others` call `sal_list` again, so one action can issue 8+ `SHOW TABLES` queries. Cache per request per database (key by current DB), noting the cache must be invalidated across `select_db`.

### 🟡 Flash filtering depends on the English word "Several", `admin/includes/staff_attendance_locations_ajax.php:147`
`sal_set_branch_flash` decides which success rows to show by `stripos($msg,'Several')`. A reworded message silently loses the notice. Have the engine return an explicit flag (e.g. `notice`) instead.

## Nits
- ⚪ `engine.php:46`, `SHOW TABLES LIKE` treats `_` as a wildcard; harmless given the fixed names, but `information_schema` or escaping is cleaner.
- ⚪ `staff-attendance-location-edit.js:488-491`, both branches of the `loadPage` check do the same `window.location.href`; collapse.
- ⚪ `engine.php:196-199`, SQL `ORDER BY name` is followed by a PHP `usort`; the usort alone is what matters for the case-insensitive spec, the SQL order only decides which 50 rows survive the LIMIT (collation dependent). Fine, but a comment would help.
- ⚪ `engine.php:575`, `sal_geocode` takes `$db` it never uses.

## Strengths
- Engine honours the AGENTS.md contract: mysqli + `$ctx` in, arrays out, no echo, no superglobals; all SQL is prepared; names compared in PHP with `mb_strtolower` as the spec requires.
- Branch copies are safe: school ids intersected with the server-side switch list, client never sends `db_name`, `select_db` restored in `finally`, errors isolated per school.
- Dispatcher runs before `authenticate.php`, so dead sessions get JSON 401 rather than an HTML redirect; order of 401, CSRF 403, role 403 is sensible and each response returns a fresh token that the JS adopts.
- Leaflet SRI hashes match jsDelivr's served files (checked), output is escaped (`htmlspecialchars`, JSON hex flags, `esc()` in JS, `textContent` for search hits).
- Delete correctly trusts server `mode`, never hard-deletes referenced rows, and the toggle is absolute not a flip.

## Test coverage
TESTS = none-by-design (gate is `/check verify` on the real app). No findings raised on this. Points `/check verify` should exercise: typing a radius by hand in the number field, map retry after a tile failure, a duplicate name differing only by case, deleting a referenced row (active then inactive), a branch write with a school not on the list, and that mysqli is back on the session DB after an extra-school failure.