refactor: extract record GIS feature hydration into a shared useRecordFeatures module - #2212
Draft
jayenashar wants to merge 3 commits into
Draft
refactor: extract record GIS feature hydration into a shared useRecordFeatures module#2212jayenashar wants to merge 3 commits into
jayenashar wants to merge 3 commits into
Conversation
E2E ✅ passed
After downloading and extracting the .zip file, open Updated for commit 091cccf |
jayenashar
added a commit
that referenced
this pull request
Jul 26, 2026
…ord failures Addresses the multi-model review findings on PR #2212. Geometry extraction goes back to the notebook-wide field list, reverting the per-form GIS field scoping. Per-form scoping silently dropped geometry: a record's AVPs can hold any field it was ever saved with, and the designer can move a spatial field to another form (or drop it from every view) long after capture, leaving the AVP intact but unreachable from the record's own form. This also removes a getFieldNamesForViewset call that threw during render on a viewset with a dangling view reference, and closes a query-key gap where the key tracked only the flat field list while extraction read the per-form map. The cost is giving up the skipped revision read for records of non-spatial forms; correctness outranks that, and the old code already skipped absent AVPs, so the saving was one getRevision per such record. A revision that is missing (DocumentNotFoundError) now skips just its record instead of rejecting the whole query. Any other revision-read failure is rethrown, so a systemic fault still surfaces as an error rather than as an empty map. RecordFeatureProps drops the unused `conflicts` property (no reader in either repo) while conflicts stays in the records signature, so a conflicting head syncing in still refreshes the map. The hook stops spreading the React Query result. Spreading touches every key on v5's tracked-props proxy, including `promise`, which rejects the observer's internal thenable when prefetch-in-render is off, and re-renders on unrelated state such as isFetching and dataUpdatedAt. It now returns only data/isLoading/isError/error alongside dataEngine and gisFields. Tests go from 3 to 8: a regression guard for the dropped-geometry case that fails against the per-form implementation, the FeatureCollection branch, malformed AVP values, missing-vs-systemic read failures, and the disabled-query path. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jayen <jayen@jayenashar.org>
jayenashar
marked this pull request as draft
July 27, 2026 02:24
…cordFeatures module Record geometry extraction lived inline in OverviewMap, so nothing else could hydrate features from records. It is generally useful, so it moves into app/src/gui/components/notebook/recordFeatures.ts (with unit tests) exposing getGISFields, getGISFieldsByForm, extractFeaturesFromRecord and the useRecordFeatures React Query hook. OverviewMap now consumes the shared module and drops the inline copy. One behaviour difference against the old inline code: each record's geometry is hydrated through its OWN form's GIS fields (via getGISFieldsByForm) rather than scanning every GIS field in the notebook. That is identical for any record captured through its form, and it skips database reads entirely for records of non-spatial forms. Feature properties additionally carry `conflicts`, and the query key now tracks it. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jayen <jayen@jayenashar.org>
…ord failures Addresses the multi-model review findings on PR #2212. Geometry extraction goes back to the notebook-wide field list, reverting the per-form GIS field scoping. Per-form scoping silently dropped geometry: a record's AVPs can hold any field it was ever saved with, and the designer can move a spatial field to another form (or drop it from every view) long after capture, leaving the AVP intact but unreachable from the record's own form. This also removes a getFieldNamesForViewset call that threw during render on a viewset with a dangling view reference, and closes a query-key gap where the key tracked only the flat field list while extraction read the per-form map. The cost is giving up the skipped revision read for records of non-spatial forms; correctness outranks that, and the old code already skipped absent AVPs, so the saving was one getRevision per such record. A revision that is missing (DocumentNotFoundError) now skips just its record instead of rejecting the whole query. Any other revision-read failure is rethrown, so a systemic fault still surfaces as an error rather than as an empty map. RecordFeatureProps drops the unused `conflicts` property (no reader in either repo) while conflicts stays in the records signature, so a conflicting head syncing in still refreshes the map. The hook stops spreading the React Query result. Spreading touches every key on v5's tracked-props proxy, including `promise`, which rejects the observer's internal thenable when prefetch-in-render is off, and re-renders on unrelated state such as isFetching and dataUpdatedAt. It now returns only data/isLoading/isError/error alongside dataEngine and gisFields. Tests go from 3 to 8: a regression guard for the dropped-geometry case that fails against the per-form implementation, the FeatureCollection branch, malformed AVP values, missing-vs-systemic read failures, and the disabled-query path. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jayen <jayen@jayenashar.org>
Nothing exercised the notebook overview map before. The notebook page renders a tab's children only while that tab is selected, so the whole record-geometry hydration path went untested. The seeded e2e-minimal notebook gains an optional TakePoint field, plus a geolocation stub helper: headless Chrome has no location provider, and this suite runs WebDriver Classic so BiDi's browser.emulate is unavailable. Capacitor's web plugin reads navigator.geolocation at call time, so shadowing the method is enough. Page-object helpers capture a point and open the map tab, and the new spec creates a record with a point then asserts the map plots it. The spec waits for a terminal state (map mounted, or an empty/error state rendered) before asserting, so a hydration regression reports the actual cause instead of timing out. It does not depend on basemap tiles, which need an API key CI has no secret for; OpenLayers builds its viewport regardless. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Jayen <jayen@jayenashar.org>
jayenashar
force-pushed
the
feat/shared-record-features
branch
from
July 27, 2026 10:05
4811246 to
08a2cbb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refactor: extract record GIS feature hydration into a shared useRecordFeatures module
Ticket
n/a
Description
Record GIS geometry extraction lived inline inside
OverviewMap.tsx, so nothing else in the app could hydrate GeoJSON features from records. This moves it into its own module with tests, andOverviewMapconsumes it. Behaviour is unchanged: the map renders exactly as before.The map also had no e2e coverage, because
TabPanelrenders a tab's children only while that tab is selected and no spec ever opened it. This adds that coverage.Proposed Changes
app/src/gui/components/notebook/recordFeatures.ts:getGISFields,extractFeaturesFromRecord, and theuseRecordFeaturesReact Query hook, which returns the query state plus the memoizeddataEngineandgisFieldsso consumers share them.app/src/gui/components/notebook/recordFeatures.test.tsx: 8 tests.app/src/gui/components/notebook/OverviewMap.tsx: consumes the hook. Rendering, styling, tap handling and popover code are untouched; props and mount contract are unchanged.e2e/test/specs/app/notebook-map.e2e.tsplus harness:api/notebooks/e2e-minimal.jsongains an optionalTakePointfield so the seeded notebook has geometry, ande2e/test/helpers/geolocation.tsstubsnavigator.geolocation.getCurrentPosition(headless Chrome has no location provider, and this suite pins WebDriver Classic sobrowser.emulateis unavailable).Three decisions in the new module are worth flagging for review:
sectionMovedToForm/fieldMovedToSectioncan reparent a spatial field long after capture, leaving the AVP intact but unreachable from the record's own form. Scoping to the record's own form silently drops that geometry.getRevisionsits inside the per-record guard, so oneDocumentNotFoundErrorno longer rejects the batch and blanks the whole map. Any other revision-read failure is still rethrown, so a systemic fault surfaces as an error rather than as a misleadingly empty map.gettrap marks props tracked, so spreading re-rendersOverviewMaponisFetching/dataUpdatedAt/failureCount, none of which it reads, and touches the own enumerablepromisekey.The React Query key prefix changes from
overview-map-featurestorecord-features. Nothing invalidates the old key, so the rename is inert.How to Test
pnpm --filter @faims3/app compile,pnpm --filter @faims3/e2e typecheck,pnpm run format:check: cleanpnpm --filter @faims3/app test: 25 files / 120 tests pass, including the 8 new onesapp/notebook-map.e2e.tsAdditional Information
The e2e spec's load-bearing assertion is the absence of the "No records with location data found" empty state, which renders precisely when hydration yields zero features, so it fails if geometry stops being extracted. It waits for a terminal state before asserting, so a regression reports its cause instead of timing out. No basemap API key is needed, since OpenLayers builds its viewport whether or not a tile loads.
Checklist
🤖 Generated with Claude Code