Repository navigation
Chore/cleanup and tests - #9
Merged
Merged
Conversation
The in-memory job queue (job_runner.py) had no crash recovery: if the backend process died mid-job, a field's processing_status stayed "processing" forever since nothing else ever updated it, and the frontend would show "analysis in progress" indefinitely with no way to recover short of a manual DB edit. - job_runner.py: add reconcile_stale_processing(), which marks any field stuck in "processing" past job_stale_timeout_minutes (default 15) as "failed" with a clear retry message, while skipping anything genuinely still running in this process's own in-memory _active set. Runs once at startup and then on a job_reconcile_interval_seconds loop (default 5 min) via a daemon thread. - config.py: add job_stale_timeout_minutes / job_reconcile_interval_seconds. - main.py: start the sweep on app startup, stop it on shutdown. - FieldDetailPage.tsx: fix a gap this exposed — the "failed" banner only ever checked the poll-derived status, so a field already "failed" on a fresh page load (exactly what the new sweep produces) showed nothing. It now also checks the field's own processing_status/processing_error from initial load. Verified: backend imports cleanly, FastAPI startup/shutdown lifecycle runs without hanging (TestClient), a mocked-Supabase check confirms the query correctly excludes actively-running jobs and only reconciles genuinely stale ones, and `tsc -b` passes. Addresses docs/AUDIT-FINDINGS.md item #2 (the single-process crash-recovery half of it; cross-replica dedupe for horizontal scaling is still open, as noted in the doc). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- gee_client.py: gee_health_probe() now caches for 60s instead of forever (lru_cache meant it reported the first-ever result for the process lifetime); added a bounded retry (2 retries, exponential backoff) around the actual GEE .getInfo() data calls in fetch_tier3_from_gee, fetch_tier2_composite_from_gee, and fetch_chirps_rainfall, so a single transient GEE failure no longer fails the whole tier fetch. The health probe itself intentionally stays unretried -- it should reflect live state, not mask it. - field_service.py: remove a redundant duplicate log call in get_field_imagery's error handler (kept the specific 502 handling, which is more useful than falling through to the generic 500 handler). - baseline.py: replace per-row insert-with-try/except with a single batched insert (deduping on the table's unique key first, matching the previous "skip duplicate" behavior). - RegisterFieldPage.tsx: navigate to the field detail page immediately after creation instead of waiting up to 6 minutes on this page for processing to finish. Previously, a timeout here left the user on a generic error with no link back to a field that had already been created server-side, inviting a duplicate submission. The detail page already polls processing status and handles in-progress/failed states. - VegetationChart.tsx: memoize derived chart data with useMemo. - constants.ts: remove unused MAP_TILE_URL/MAP_ATTRIBUTION (superseded by mapBasemaps.ts). - Remove vestigial empty frontend/src/mock directories and the dead tsconfig.app.json stub (confirmed unreferenced; tsc -b already uses tsconfig.json directly). Addresses docs/AUDIT-FINDINGS.md items #9, #10, #11, #15, #16, #17, #18, #21, #22. Verified: backend modules import cleanly, py_compile passes, gee_health_probe() still degrades gracefully without GEE configured, tsc -b passes.
…r, and CI Addresses docs/AUDIT-FINDINGS.md #1 (no automated tests/CI) and #19 (no ESLint/Prettier) -- the two remaining structural gaps from the audit. Backend (backend/tests/, pytest.ini, requirements-dev.txt): - 33 unit tests over pure-logic modules: risk_engine (z-score classification, insufficient-data branches, rainfall cross-check), geometry, vegetation_index, and security (error sanitization). Plus mocked-Supabase tests for job_runner.reconcile_stale_processing and the report_service N+1 fix, asserting risk_assessments is queried exactly once per report. - Deliberately disables ml_risk in risk_engine tests -- the trained model (backend/models/ml_risk/model.joblib) exists locally but is gitignored, so a test implicitly depending on it would behave differently in CI than locally. - No live Supabase/GEE credentials needed anywhere in this suite. - Run: cd backend && python -m pytest Frontend (frontend/src/**/*.test.{ts,tsx}, vitest via vite.config.ts): - 27 tests: boundaryParser.ts (GeoJSON/KML/coordinate-list parsing, including the lat/lng-order heuristic and ring-closing), mapStatus.ts (map-quality and draw-hint branching logic), and the RiskBadge component. - Run: pnpm run test Tooling: - eslint.config.js (flat config, ESLint 9 + typescript-eslint + react-hooks + react-refresh recommended rules) -- runs clean on the existing codebase, zero errors/warnings. - .prettierrc.json / .prettierignore -- added but deliberately not run across the existing codebase yet (would touch all 42 source files in one diff, since it predates Prettier); format/format:check scripts exist for a separate, deliberate reformat commit later. - .gitignore: untrack frontend/tsconfig.tsbuildinfo (a machine-generated tsc build-cache file that was accidentally committed) and ignore *.tsbuildinfo going forward. CI (.github/workflows/ci.yml): - backend job: pip install -r requirements-dev.txt && python -m pytest - frontend job: pnpm install --frozen-lockfile, pnpm run lint, pnpm run test, pnpm run build - Runs on push to main and on every PR. Verified locally end-to-end before committing (both jobs' exact command sequence run and passing).
Updates AUDIT-FINDINGS.md (#1 and #19 now fixed, with what's covered vs still manual), CONTRIBUTING.md (CI commands, what CI does/doesn't cover), ARCHITECTURE.md (known-limitations line updated), and README.md (CI badge, new Automated tests section separate from the manual verify-stack scripts).
…on in prod Addresses docs/AUDIT-FINDINGS.md #13 and #4, the two remaining findings that were genuinely fixable without a product/process decision. - job_runner.enqueue_field_processing and field_service.process_field now take and enforce institution_id, instead of process_field trusting every caller to have already checked ownership. process_field filters .eq("institution_id", institution_id) when loading the field, raising 404 on a mismatch -- the same defense every other tenant-scoped lookup already uses. Threaded through all three field_service.py call sites and scripts/reprocess_all_fields.py (the one caller outside field_service.py). Regression-tested in backend/tests/test_field_service.py. - main.py now logs a warning at startup if APP_ENV=production and ALLOW_OPEN_REGISTRATION=true, so a misconfigured production deployment doesn't silently stay open to self-registration. Tested in backend/tests/test_main_startup.py. With this, every AUDIT-FINDINGS.md item fixable by editing code has been fixed. What's left (#14 migration framework fragility, #20 duplicate list+table DOM, the crop_type/boundary half of #12) is explicitly a process/product decision or needs live browser verification -- see the doc for why each was left alone rather than forced through blind. Verified: 38/38 backend tests pass, frontend build (tsc -b + vite build) still succeeds.
Found by actually running the app end-to-end against the live stack, not by code review: every authenticated request failed with a generic "Invalid token" 401, including the very first request right after a fresh, genuinely valid sign-in. Root cause, isolated with a standalone repro script: jwt_verify.py's JWKS/PyJWT branch had zero leeway on the `iat` claim. This dev machine's clock was ~24s behind the real Supabase auth server's clock (confirmed via `date -u` vs. the server's HTTP Date header), so a freshly-issued token looked like it was issued in the future from this machine's perspective -- jwt.exceptions.ImmatureSignatureError. This isn't a local-machine-only issue: any real deployment with even minor clock drift (VM clock skew, a missed NTP sync) would reject every single login the same way, with nothing in the error message hinting at why. Added a 30s leeway to both the JWKS/PyJWT branch and the legacy HS256/python-jose branch. Regression-tested in backend/tests/test_jwt_verify.py, including a check that the leeway is actually load-bearing (without it, the same slightly-future token is provably rejected) and that it doesn't become a security hole (a token genuinely issued 10 minutes in the future is still rejected).
…orms Found while writing E2E tests: page.getByLabel(...) couldn't locate any input on either form. On inspection, every <label>/<input> pair was two sibling elements with no htmlFor/id connecting them (RegisterFieldPage already did this correctly, so it wasn't a project-wide pattern). This isn't just a testability inconvenience -- a screen reader announces these inputs with no accessible name at all, on the two most fundamental account-entry screens in the app. Added matching id/htmlFor pairs to every field in both forms.
A different layer from the pytest/Vitest unit suites (mocked/faked dependencies, no live services) -- this runs against a real Supabase project (Auth + Postgres) and real Google Earth Engine, driving the actual app in a browser. It's what found and proved the fixes in the two preceding commits; see frontend/e2e/README.md for what it covers, prerequisites, and why it isn't wired into CI yet (needs live credentials as GitHub secrets -- a real decision, not a config change). - playwright.config.ts: chromium only, starts/reuses the Vite dev server, does not start the backend (must already be running). - e2e/full-walkthrough.spec.ts: registers a fresh institution + admin account, registers a field via GeoJSON upload, waits for the real GEE processing job to resolve, then verifies the fields-list responsive layout (docs/AUDIT-FINDINGS.md #20) at both desktop and mobile widths. Deliberately does not assert a specific risk tier/outcome -- that depends on real, variable satellite data. - vite.config.ts: scope Vitest's `include` to src/ -- its default glob was picking up the Playwright spec too and failing on it (different test runner/API). - package.json/pnpm-lock.yaml: @playwright/test, @types/node (needed for __dirname-equivalent in the spec -- this project is ESM). - .gitignore: Playwright's own artifact directories (test-results/, playwright-report/, e2e/screenshots/). Verified: full run passes end to end (1.7 min, real GEE call included) -- see docs/WALKTHROUGH.md for the actual screenshots from this run.
…cklog - docs/WALKTHROUGH.md + docs/screenshots/walkthrough/: real screenshots captured by frontend/e2e/full-walkthrough.spec.ts against the live stack -- field registration through a HIGH RISK assessment (the test AOI landed mostly over a river channel, a legitimate result demonstrating the pipeline reacts to what's actually on the ground), the ML stress model's elevate-only behavior visibly not overriding an already-higher z-score tier, and the fields-list responsive layout at both desktop and mobile widths. - docs/AUDIT-FINDINGS.md: added #25 (the JWT clock-skew bug) and #26 (the missing label associations), both found via the live E2E run rather than code review; gave #20 (duplicate list+table DOM) a definitive live-verified answer instead of a deferred guess -- confirmed the toggle is genuinely mutually exclusive in a real browser at both widths. Updated the order-of-attack summary: every item fixable by editing code is now fixed and verified: what's left is a process/product decision (#14, part of #12) or low-priority cleanup (#24). - README.md/CHANGELOG.md: link the new walkthrough doc, record this pass.
Found while adding a new basemap layer for area-name/place identification --
both real, pre-existing bugs, not introduced by this change.
- mapBasemaps.ts: CARTO's free anonymous tile service (used by "Street")
now gates tiles behind an API key we don't have. The request still
returns HTTP 200 (confirmed via curl), it's the tile *image content*
that's a watermark reading "API KEY REQUIRED" -- nothing in error
handling would ever catch this, only looking at the rendered map would.
Switched "Street" to Esri's free, no-key Canvas/World_Light_Gray_Base
(same provider already used and confirmed working for Satellite/Hybrid).
- Added a genuine "Map" basemap (Esri World_Street_Map, free, no key) with
legible town/village names, roads, and landmarks -- the closest existing
option ("Street") is a near-blank canvas by design, not meant for
place-name lookup.
- MapBasemapLayers.tsx: "Hybrid" was registering as two duplicate entries
in the layer switcher -- two sibling TileLayers under one
LayersControl.BaseLayer register as two separate control entries in
react-leaflet instead of one combined layer. Fixed by wrapping both in a
LayerGroup so they toggle together as a single layer. Regression-tested
in frontend/e2e/full-walkthrough.spec.ts (asserts exactly one switcher
entry per BASEMAPS config, no duplicate labels).
- RegisterFieldPage.tsx: mention the new "Map" layer in the in-app
map-layer-help text.
Verified visually against the live dev server (not just curl/status codes,
since the API-key issue specifically wouldn't show up that way) and via a
full E2E run.
- index.css: Leaflet's internal <label> markup for the layer switcher was
unintentionally inheriting the site-wide uppercase/letter-spaced form-
label rule (a bare `label { text-transform: uppercase; ... }` selector
meant for form fields, leaking into third-party widget markup) -- made
it look like an oversized, shouty block list overlapping the map.
Reset it to a compact, normal-case menu with proper hover states.
- MapStatusOverlay.tsx: the dark-green status banner (draw hints / map-
quality messages) had no way to dismiss it, so it could sit over parts
of the map a user wanted to see. Added a close (x) button. Dismissal is
tracked by the exact message shown, not a plain boolean, so a *new*
status (finishing the boundary, switching layers) still shows up rather
than staying hidden forever after one dismissal.
Verified visually against the live dev server: layer switcher panel is now
compact and readable, and clicking the close button removes the banner.
…log and changelog
Sentinel-2's visible bands are natively ~10m/pixel. Sampling a typical field at that scale directly (the old behavior) produced pixel arrays as small as a few dozen per side, converted straight to a PNG with zero upscaling -- the browser then stretched that tiny image to fill the display, which is exactly what "blurry" looks like. Confirmed with a standalone repro against a real field: 101x113px at native scale. - Resample server-side in GEE (bicubic interpolation of the real reflectance values, not a learned/hallucinated reconstruction) and reproject to a finer scale before sampling. The scale is chosen per field from its own footprint (_display_scale_m) so small fields get real added density and large fields don't get resampled far past what's useful for display. - Replaced the fixed RGB_MIN=0/RGB_MAX=3000 reflectance window with a per-band 2nd-98th percentile linear stretch -- the standard remote-sensing visualization technique. A fixed window looks over- or under-exposed depending on what's actually in the scene (bare soil, water, and dense canopy sit in very different reflectance ranges); a scene-adaptive stretch fixes that regardless of pixel count. - Final resize to a consistent display size (LANCZOS) plus a light unsharp-mask pass, applied to real sampled pixels -- sharpens/smooths existing values, doesn't add information that wasn't there. - Also removed a redundant duplicate GEE sample per scene (the old code sampled each scene once inside _pack_scene and again for the NDVI delta calculation) -- now sampled once and reused, which matters more now that sampling is doing more work per call. Verified visually against a live field through the running app (not just pixel-array shapes) -- see docs/WALKTHROUGH.md for the before/after.
Resolves the crop_type half of docs/AUDIT-FINDINGS.md #12 with a factual answer instead of leaving it an open product question. Checked what the baseline engine actually supports: scripts/build_district_baseline.py only ever builds a baseline for settings.pilot_crop, and confirmed live against the real database, all 192 rows in baseline_stats are crop_type='wheat'. There is no code path by which a baseline for any other crop could exist. Accepting another crop_type value wasn't a genuine multi-crop capability, it was a silent trap: the field would get created, a real GEE job would run (real cost, real quota), and the result would just be INSUFFICIENT_DATA after the fact. Added _validate_crop_type() in field_service.py, called from both create_field and update_field, rejecting anything but settings.pilot_crop (case-insensitive) with a 400 and a clear message before any GEE/Supabase work happens. Regression-tested in test_crop_type_validation.py, including a test proving the rejection happens before touching Supabase or GEE at all.
…l error Found by the E2E suite failing on an otherwise-unrelated change: a real field finished processing successfully in ~80s (confirmed directly in the database: processing_status='ready'), but the UI stayed stuck showing "Satellite analysis in progress" for the rest of a 4-minute wait. Root cause: FieldDetailPage's isAnalyzing = polling || reprocessing || detail.processing_status === 'processing'. After the earlier #8 fix, a poll error stops `polling`, but nothing ever refreshed `detail` -- so detail.processing_status stayed at its stale initial-load value of 'processing' forever, keeping isAnalyzing true regardless of what actually happened server-side. This also silently blocked the #8 pollError message itself from ever showing (its !isAnalyzing guard was never satisfied), and would have equally masked the #2 stuck-failed-field display. useFieldProcessing's completion callback (renamed onSettled, was onReady) now fires whenever polling stops for ANY reason -- ready, failed, max-polls timeout, or a poll request erroring -- not just success, so the caller always re-fetches the field's true current state. If that refresh finds the job still genuinely processing, polling resumes automatically (self-healing past transient blips); if it finds a resolved state, the UI reflects it correctly. Added clearPollError() so a resolved outcome doesn't leave a stale "couldn't confirm status" error showing alongside a successful result. Verified by re-running the full live E2E suite after the fix: 20s, versus hitting the 4-minute timeout before.
…'s changes - guideContent.ts: mention the new "Map" basemap layer and what each layer is for in the "Register field" guide block; add a data-quality tip about confirming a field's location with the Map layer before trusting a risk flag (ties directly to a real finding from live testing -- a field drawn over a river legitimately produced a HIGH RISK flag, since boundary validation is geometric only with no land-cover check). - USER-FLOW.md: explain what the imagery-sharpening fix does and doesn't mean -- display-quality processing on real pixels (smooth resampling, scene-adaptive contrast, light sharpening), not a learned reconstruction, so what's shown stays a faithful rendering of the actual satellite data. - ARCHITECTURE.md: note how field imagery is now processed in the field-processing data flow section, cross-referencing USER-FLOW.md.
…cklog and changelog
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
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.
No description provided.