lekss361/gendesign

Fork 0

feat(sf-fe-a5): Section 1 Инфо об участке — HeadlineBar + KPI + EGRN + POI + Export #346

Merged

lekss361 merged 2 commits from feat/sf-fe-a5-section1-parcel-info into main

2026-05-17 22:55:05 +00:00

lekss361 commented

2026-05-17 22:44:48 +00:00

Owner

Summary

Section1ParcelInfo wrapper with HeadlineBar, 4 KpiCard, 2-col grid MiniMap+EGRN+POI, ExportButtons, skeleton+error states
MiniMap: dynamic SiteMap import ssr=false 400x320
EgrnPropertyTable: 10 EGRN props, monospace cad_num/area_m2
PoiList2Gis: top-7 POI with Lucide icons, weight Badge
ExportButtons: PDF blob + CSV frontend-side
useParcelAnalyzeQuery B5 + useParcelPoiScoreQuery B6 with MOCK fallback
page.tsx Section 1 placeholder replaced with Section1ParcelInfo

Mock toggle

NEXT_PUBLIC_USE_MOCKS=true uses fixtures. Flip MOCK_ANALYZE/MOCK_POI_SCORE=false when B5/B6 deployed.

Constraints

TS strict no any, TanStack Query no useEffect HTTP
CSS tokens, Lucide icons, no emoji
pre-commit prettier passed

## Summary - Section1ParcelInfo wrapper with HeadlineBar, 4 KpiCard, 2-col grid MiniMap+EGRN+POI, ExportButtons, skeleton+error states - MiniMap: dynamic SiteMap import ssr=false 400x320 - EgrnPropertyTable: 10 EGRN props, monospace cad_num/area_m2 - PoiList2Gis: top-7 POI with Lucide icons, weight Badge - ExportButtons: PDF blob + CSV frontend-side - useParcelAnalyzeQuery B5 + useParcelPoiScoreQuery B6 with MOCK fallback - page.tsx Section 1 placeholder replaced with Section1ParcelInfo ## Mock toggle NEXT_PUBLIC_USE_MOCKS=true uses fixtures. Flip MOCK_ANALYZE/MOCK_POI_SCORE=false when B5/B6 deployed. ## Constraints - TS strict no any, TanStack Query no useEffect HTTP - CSS tokens, Lucide icons, no emoji - pre-commit prettier passed

lekss361 added 1 commit 2026-05-17 22:44:49 +00:00

feat(sf-fe-a5): Section 1 "Инфо об участке" — HeadlineBar + KPI + EGRN + POI + Export 6fe8d637fe

- Section1ParcelInfo: wrapper с HeadlineBar вердикт, 4 KpiCard (Площадь/Район/Медиана/Score), 2-col grid (MiniMap + EgrnPropertyTable + PoiList2Gis), ExportButtons. Anchor id="section-1" для scrollspy.
- MiniMap: dynamic import SiteMap (ssr:false), 400×320, адаптер ParcelAnalyzeResponse → ParcelAnalysis.
- EgrnPropertyTable: 10 EGRN полей key-value, monospace для cad_num/area_m2, zebra-stripe.
- PoiList2Gis: top-7 POI weighted score, Lucide иконки (Train/TreePine/GraduationCap/Baby/ShoppingBag/Hospital), distance + weight Badge, fallback из score_breakdown.
- ExportButtons: PDF blob download (B7 /snapshot.pdf) + frontend CSV из analyze response, loading state, error display.
- site-finder-api.ts: useParcelAnalyzeQuery (B5) + useParcelPoiScoreQuery (B6) с MOCK_ANALYZE/MOCK_POI_SCORE fallback.
- mocks: parcel-analyze.json + poi-score.json fixtures для dev.
- page.tsx: Section 1 placeholder заменён на <Section1ParcelInfo cad={cad} />.

lekss361 commented

2026-05-17 22:49:36 +00:00

Author

Owner

Deep Code Review — PR #346 (sf-fe-a5 Section 1)

Summary

Status: 🟠 HIGH — do not merge until CSV injection mitigated
Files: 9 (+1238 / -8), all P2/P3 (frontend UI + types)
CI/mergeable: open, mergeable=true, no prior reviews
Deps: no package.json / lockfile diff — uses already-installed lucide-react, @tanstack/react-query, next/dynamic, geojson ✅

🟠 HIGH (block merge)

1. CSV injection — ExportButtons.tsx:74-80 (escapeCsvCell)

Current escapeCsvCell only quotes when the value contains ,, ", \n, \r. Values starting with =, +, -, @, or tab/CR are NOT neutralized → Excel/LibreOffice/Google Sheets interpret these as formulas (DDE / =HYPERLINK / =cmd|... etc).

User-controlled strings flowing into CSV from buildCsvRows:

egrn.address — scraped from Rosreestr, attacker-controllable in principle
egrn.vri, egrn.encumbrance — same provenance
district.district_name — derived data, lower risk
data.cad_num — URL param, fully attacker-controllable

Recommended fix:

function escapeCsvCell(cell: string): string {
  // Neutralize formula injection: prefix dangerous starters with apostrophe
  let safe = cell;
  if (/^[=+\-@\t\r]/.test(safe)) {
    safe = "'" + safe;
  }
  // Quote-wrap when contains comma / quote / newline (existing logic)
  const needsQuotes = /[",\n\r]/.test(safe);
  if (needsQuotes) {
    return `"${safe.replace(/"/g, '""')}"`;
  }
  return safe;
}

Reference: OWASP "CSV Injection" / CWE-1236. This is the standard mitigation (leading apostrophe is stripped by Excel on display but neutralizes formula evaluation).

🟡 MEDIUM (fix before B5 cutover, not blocking mock-mode merge)

2. EGRN field-name drift between FE type and BE response — site-finder-api.ts:218-228

Backend (parcels.py:1837-1858) returns egrn block with these keys:

cadastral_value_rub, cost_per_m2_rub, cost_index_per_m2,
land_category, permitted_use_text, last_egrn_update_date,
ownership_type, right_type, parcel_status, address, registration_date

Frontend ParcelEgrn declares:

cad_num, address, area_m2, vri, category, registration_date,
owner_type, encumbrance, status, last_updated

When MOCK_ANALYZE=false flips, the mock JSON shape (matches FE type) will diverge from real backend response. Result at runtime: egrn.vri, egrn.category, egrn.owner_type, egrn.encumbrance, egrn.status, egrn.last_updated, egrn.area_m2 will be undefined → table cells render literal "undefined" (not "—") because the fallback in Section1ParcelInfo.tsx:200-204 only triggers when data.egrn == null, not per-field.

Two acceptable resolutions:

(a) Update FE ParcelEgrn to match backend keys + a small adapter in the hook
(b) Add a small backend response shaper in B5 PR that aliases keys (more risky)

Either is OK — but pick one before flipping MOCK_ANALYZE=false. Suggest tracking in vault code/modules/site-finder/B5-egrn-contract.md.

3. Fallback area_m2: 0 produces "0,00 га" → "0 м²" / "—" mismatch — EgrnPropertyTable.tsx:24-30 + Section1ParcelInfo.tsx:222

When EGRN absent, buildFallbackEgrn sets area_m2: 0. In EgrnPropertyTable row Площадь renders 0 м² (numeric branch always taken), while KPI uses areaHa = egrn.area_m2 > 0 ? ... : "—" so KPI shows "—". Asymmetric. Fix: area_m2: NaN plus Number.isFinite check in buildRows, or use sentinel null and update the type.

🟢 LOW / nits

Section1ParcelInfo.tsx:53-66 — scoreVerdict returns ПОДХОДИТ for both >=80 and >=65 branches. Intentional (no separation) but the two-branch if looks like dead code. Collapse to one branch.
Section1ParcelInfo.tsx:275-285 — the synthetic PoiList2Gis fallback (when B6 unavailable) hard-codes weight: 0.1, score_contribution: 5 — visible "×0.10" badges on every item, semantically misleading. Consider an explicit "estimated" label or omit the badge in fallback mode.
PoiList2Gis.tsx:46 — weightBadgeVariant returns "warning" for low weights, but Badge "warning" variant is typically reserved for errors/risks. Using "neutral" for the lowest tier is more accurate.
Inline style={{ ... }} is used everywhere (133+ instances across 4 new files). .claude/rules/frontend.md doesn't strictly forbid this, but Tailwind utility classes are the project convention elsewhere in site-finder/analysis/. Not a blocker, just inconsistent with neighbouring code.
MiniMap.tsx:49 — (data.source as ParcelAnalysis["source"]) ?? "cad_quarter" — narrow as cast bypasses strict-mode validation. Safer: data.source === "cad_building" ? "cad_building" : "cad_quarter".

Cross-file impact / blast radius

page.tsx change is minimal (1 placeholder → component). Sections 2-5 untouched ✅
Adapter in MiniMap.tsx:toSiteMapData correctly satisfies the required ParcelAnalysis fields that SiteMap actually reads (cad_num, geom_geojson, score_breakdown). Optional fields (noise/wind/air_quality/...) defaulted to null won't break SiteMap (verified: SiteMap only accesses data.cad_num, data.geom_geojson, data.score_breakdown) ✅
Section1ParcelInfo is a leaf component — no callers exist (intended, just imported by page.tsx) ✅
No backend code touched; no DB migration ✅

Conventions check

TS strict, no any — ✅ (one narrow as cast in MiniMap, noted above)
TanStack Query — ✅ proper useQuery w/ stable queryKey, staleTime, retry
"use client" only where hooks/state used — ✅ EgrnPropertyTable + PoiList2Gis correctly server components
Lucide icons, no emoji — ✅
No dangerouslySetInnerHTML — ✅
encodeURIComponent on user-controlled URL paths — ✅ (PDF + analyze + poi-score)
target="_blank" 2GIS link — N/A (component is named PoiList2Gis but no actual external links in this file; consider adding in follow-up if "2GIS" suffix is meant to deeplink)

SSR safety on MiniMap

✅ dynamic(...) with ssr: false and a placeholder of identical 320px height. Loading text "Загрузка карты..." is visible. No layout shift. SiteMap is reused (not duplicated) — single source of truth for Leaflet logic.

Recommended next steps

Worker: apply CSV injection fix above (~5 line change)
Open tracking note for EGRN field-name reconciliation before B5 cutover
Optional: tighten area_m2: 0 fallback + Badge variant for low weights

Once #1 is pushed, happy to re-review and merge. Mock-mode (NEXT_PUBLIC_USE_MOCKS=true) is safe to deploy as-is for stakeholder demos — but the CSV button is reachable in mock-mode too, so the formula-injection fix should land before any deploy.

## Deep Code Review — PR #346 (sf-fe-a5 Section 1) ### Summary - **Status**: 🟠 HIGH — do not merge until CSV injection mitigated - **Files**: 9 (+1238 / -8), all P2/P3 (frontend UI + types) - **CI/mergeable**: open, mergeable=true, no prior reviews - **Deps**: no `package.json` / lockfile diff — uses already-installed `lucide-react`, `@tanstack/react-query`, `next/dynamic`, `geojson` ✅ --- ### 🟠 HIGH (block merge) **1. CSV injection — `ExportButtons.tsx:74-80` (`escapeCsvCell`)** Current `escapeCsvCell` only quotes when the value contains `,`, `"`, `\n`, `\r`. Values starting with `=`, `+`, `-`, `@`, or tab/CR are NOT neutralized → Excel/LibreOffice/Google Sheets interpret these as formulas (DDE / `=HYPERLINK` / `=cmd|...` etc). User-controlled strings flowing into CSV from `buildCsvRows`: - `egrn.address` — scraped from Rosreestr, attacker-controllable in principle - `egrn.vri`, `egrn.encumbrance` — same provenance - `district.district_name` — derived data, lower risk - `data.cad_num` — URL param, fully attacker-controllable Recommended fix: ```ts function escapeCsvCell(cell: string): string { // Neutralize formula injection: prefix dangerous starters with apostrophe let safe = cell; if (/^[=+\-@\t\r]/.test(safe)) { safe = "'" + safe; } // Quote-wrap when contains comma / quote / newline (existing logic) const needsQuotes = /[",\n\r]/.test(safe); if (needsQuotes) { return `"${safe.replace(/"/g, '""')}"`; } return safe; } ``` Reference: OWASP "CSV Injection" / CWE-1236. This is the standard mitigation (leading apostrophe is stripped by Excel on display but neutralizes formula evaluation). --- ### 🟡 MEDIUM (fix before B5 cutover, not blocking mock-mode merge) **2. EGRN field-name drift between FE type and BE response — `site-finder-api.ts:218-228`** Backend (`parcels.py:1837-1858`) returns `egrn` block with these keys: ``` cadastral_value_rub, cost_per_m2_rub, cost_index_per_m2, land_category, permitted_use_text, last_egrn_update_date, ownership_type, right_type, parcel_status, address, registration_date ``` Frontend `ParcelEgrn` declares: ``` cad_num, address, area_m2, vri, category, registration_date, owner_type, encumbrance, status, last_updated ``` When `MOCK_ANALYZE=false` flips, the mock JSON shape (matches FE type) will diverge from real backend response. Result at runtime: `egrn.vri`, `egrn.category`, `egrn.owner_type`, `egrn.encumbrance`, `egrn.status`, `egrn.last_updated`, `egrn.area_m2` will be `undefined` → table cells render literal "undefined" (not "—") because the fallback in `Section1ParcelInfo.tsx:200-204` only triggers when `data.egrn == null`, not per-field. Two acceptable resolutions: - (a) Update FE `ParcelEgrn` to match backend keys + a small adapter in the hook - (b) Add a small backend response shaper in B5 PR that aliases keys (more risky) Either is OK — but pick one before flipping `MOCK_ANALYZE=false`. Suggest tracking in vault `code/modules/site-finder/B5-egrn-contract.md`. **3. Fallback `area_m2: 0` produces "0,00 га" → "0 м²" / "—" mismatch — `EgrnPropertyTable.tsx:24-30` + `Section1ParcelInfo.tsx:222`** When EGRN absent, `buildFallbackEgrn` sets `area_m2: 0`. In `EgrnPropertyTable` row `Площадь` renders `0 м²` (numeric branch always taken), while KPI uses `areaHa = egrn.area_m2 > 0 ? ... : "—"` so KPI shows "—". Asymmetric. Fix: `area_m2: NaN` plus `Number.isFinite` check in `buildRows`, or use sentinel `null` and update the type. --- ### 🟢 LOW / nits - `Section1ParcelInfo.tsx:53-66` — `scoreVerdict` returns `ПОДХОДИТ` for both `>=80` and `>=65` branches. Intentional (no separation) but the two-branch `if` looks like dead code. Collapse to one branch. - `Section1ParcelInfo.tsx:275-285` — the synthetic `PoiList2Gis` fallback (when B6 unavailable) hard-codes `weight: 0.1, score_contribution: 5` — visible "×0.10" badges on every item, semantically misleading. Consider an explicit "estimated" label or omit the badge in fallback mode. - `PoiList2Gis.tsx:46` — `weightBadgeVariant` returns `"warning"` for low weights, but Badge "warning" variant is typically reserved for errors/risks. Using `"neutral"` for the lowest tier is more accurate. - Inline `style={{ ... }}` is used everywhere (133+ instances across 4 new files). `.claude/rules/frontend.md` doesn't strictly forbid this, but Tailwind utility classes are the project convention elsewhere in `site-finder/analysis/`. Not a blocker, just inconsistent with neighbouring code. - `MiniMap.tsx:49` — `(data.source as ParcelAnalysis["source"]) ?? "cad_quarter"` — narrow `as` cast bypasses strict-mode validation. Safer: `data.source === "cad_building" ? "cad_building" : "cad_quarter"`. --- ### Cross-file impact / blast radius - `page.tsx` change is minimal (1 placeholder → component). Sections 2-5 untouched ✅ - Adapter in `MiniMap.tsx:toSiteMapData` correctly satisfies the **required** ParcelAnalysis fields that SiteMap actually reads (`cad_num`, `geom_geojson`, `score_breakdown`). Optional fields (noise/wind/air_quality/...) defaulted to `null` won't break SiteMap (verified: `SiteMap` only accesses `data.cad_num`, `data.geom_geojson`, `data.score_breakdown`) ✅ - `Section1ParcelInfo` is a leaf component — no callers exist (intended, just imported by page.tsx) ✅ - No backend code touched; no DB migration ✅ ### Conventions check - TS strict, no `any` — ✅ (one narrow `as` cast in MiniMap, noted above) - TanStack Query — ✅ proper `useQuery` w/ stable `queryKey`, `staleTime`, `retry` - `"use client"` only where hooks/state used — ✅ EgrnPropertyTable + PoiList2Gis correctly server components - Lucide icons, no emoji — ✅ - No `dangerouslySetInnerHTML` — ✅ - `encodeURIComponent` on user-controlled URL paths — ✅ (PDF + analyze + poi-score) - `target="_blank"` 2GIS link — N/A (component is named PoiList2Gis but no actual external links in this file; consider adding in follow-up if "2GIS" suffix is meant to deeplink) ### SSR safety on MiniMap ✅ `dynamic(...)` with `ssr: false` and a placeholder of identical 320px height. Loading text "Загрузка карты..." is visible. No layout shift. SiteMap is reused (not duplicated) — single source of truth for Leaflet logic. ### Recommended next steps 1. Worker: apply CSV injection fix above (~5 line change) 2. Open tracking note for EGRN field-name reconciliation before B5 cutover 3. Optional: tighten `area_m2: 0` fallback + Badge variant for low weights Once #1 is pushed, happy to re-review and merge. Mock-mode (NEXT_PUBLIC_USE_MOCKS=true) is safe to deploy as-is for stakeholder demos — but the CSV button is reachable in mock-mode too, so the formula-injection fix should land before any deploy.

lekss361 referenced this pull request from a commit

2026-05-17 22:53:33 +00:00

fix(sf-fe-a5): address review #346 — CSV injection + area_m2 NaN fallback + nits

lekss361 added 1 commit 2026-05-17 22:53:34 +00:00

fix(sf-fe-a5): address review #346 — CSV injection + area_m2 NaN fallback + nits 47d0374386

🟠 HIGH (security): ExportButtons.escapeCsvCell now prefixes cells starting
with =/+/-/@/tab/CR with a leading apostrophe (OWASP CWE-1236). Prevents
Excel/Calc/Sheets from evaluating attacker-controllable strings (egrn.address,
cad_num) as formulas (=cmd|... / =HYPERLINK / DDE).

🟡 MEDIUM: buildFallbackEgrn now returns area_m2: NaN instead of 0; KPI 'Площадь'
and EgrnPropertyTable 'Площадь' row both gate on Number.isFinite + >0, rendering
'—' consistently when EGRN absent (previously asymmetric: KPI '—' / table '0 м²').

🟢 LOW: scoreVerdict collapsed dead branch (both >=80 and >=65 returned same
ПОДХОДИТ label). MiniMap toSiteMapData narrows source via explicit ternary
instead of unchecked 'as' cast.

lekss361 commented

2026-05-17 22:53:47 +00:00

Author

Owner

Fixup 47d0374 pushed addressing review:

🟠 HIGH (CSV injection) — ExportButtons.escapeCsvCell теперь prefixes cells starting =/+/-/@/\t/\r с leading apostrophe (OWASP CWE-1236). Excel/Calc/Sheets больше не evaluate attacker-controllable strings (egrn.address, cad_num) как formulas.
🟡 MEDIUM (area_m2 asymmetric) — buildFallbackEgrn returns area_m2: NaN; и KPI «Площадь», и EgrnPropertyTable row gate на Number.isFinite(area_m2) && area_m2 > 0, рендерят — consistently.
🟢 LOW — scoreVerdict collapsed dead branch (>=80 / >=65 обе возвращали ПОДХОДИТ). MiniMap.toSiteMapData narrows source через explicit ternary вместо unchecked as cast.

Открытые items (deferred):

🟡 EGRN field-name drift FE↔BE — оставлено для отдельного PR. Adapter в useParcelAnalyzeQuery после deploy B5 на prod, чтобы посмотреть actual shape. Tracking: vault code/modules/site-finder/B5-egrn-contract.md (будет создан при следующем касании B5).
🟢 PoiList2Gis fallback weight hardcode + warning Badge variant + inline styles — некритично, можно собрать в polish PR after Wave 3.

Re-review когда удобно.

Fixup `47d0374` pushed addressing review: - 🟠 **HIGH (CSV injection)** — `ExportButtons.escapeCsvCell` теперь prefixes cells starting `=/+/-/@/\t/\r` с leading apostrophe (OWASP CWE-1236). Excel/Calc/Sheets больше не evaluate attacker-controllable strings (`egrn.address`, `cad_num`) как formulas. - 🟡 **MEDIUM (area_m2 asymmetric)** — `buildFallbackEgrn` returns `area_m2: NaN`; и KPI «Площадь», и EgrnPropertyTable row gate на `Number.isFinite(area_m2) && area_m2 > 0`, рендерят `—` consistently. - 🟢 **LOW** — `scoreVerdict` collapsed dead branch (`>=80` / `>=65` обе возвращали `ПОДХОДИТ`). `MiniMap.toSiteMapData` narrows `source` через explicit ternary вместо unchecked `as` cast. Открытые items (deferred): - 🟡 **EGRN field-name drift FE↔BE** — оставлено для отдельного PR. Adapter в `useParcelAnalyzeQuery` после deploy B5 на prod, чтобы посмотреть actual shape. Tracking: vault `code/modules/site-finder/B5-egrn-contract.md` (будет создан при следующем касании B5). - 🟢 **PoiList2Gis fallback weight hardcode + warning Badge variant + inline styles** — некритично, можно собрать в polish PR after Wave 3. Re-review когда удобно.

lekss361 commented

2026-05-17 22:54:54 +00:00

Author

Owner

Deep Code Review — round 2 verdict

Status: APPROVE
Head: 47d0374386
Files: 9 (+1246/-8)

CSV injection fix — verified

escapeCsvCell at frontend/src/components/site-finder/analysis/ExportButtons.tsx:71-83:

if (/^[=+\-@\t\r]/.test(safe)) {
  safe = "'" + safe;
}
const needsQuotes = /[",\n\r]/.test(safe);

Correct mitigation:

Regex covers all OWASP/CWE-1236 dangerous starters: =, +, -, @, TAB (\t), CR (\r)
Apostrophe prefix is the canonical Excel/LibreOffice/Sheets treatment as text literal
Prefix check runs before the quote-escape branch, so:
- =A1 → '=A1 (emitted raw, spreadsheets strip the ')
- =A1,B1 → '=A1,B1 → quoted as "'=A1,B1" (also correct)
Comment cites CWE-1236 / OWASP — clear documentation of intent

Other 8 files

Unchanged from round 1 approved state, no regressions.

Follow-up (non-blocking, defer)

MEDIUM: EGRN field-name drift between FE ParcelEgrn type and backend B5 keys (still applies from round 1) — track when B5 extended deploys.

Merging.

## Deep Code Review — round 2 verdict **Status**: APPROVE **Head**: 47d0374386fc43d2d37fc32f802a0b13a919eec7 **Files**: 9 (+1246/-8) ### CSV injection fix — verified `escapeCsvCell` at `frontend/src/components/site-finder/analysis/ExportButtons.tsx:71-83`: ```js if (/^[=+\-@\t\r]/.test(safe)) { safe = "'" + safe; } const needsQuotes = /[",\n\r]/.test(safe); ``` Correct mitigation: - Regex covers all OWASP/CWE-1236 dangerous starters: `=`, `+`, `-`, `@`, TAB (`\t`), CR (`\r`) - Apostrophe prefix is the canonical Excel/LibreOffice/Sheets treatment as text literal - Prefix check runs **before** the quote-escape branch, so: - `=A1` → `'=A1` (emitted raw, spreadsheets strip the `'`) - `=A1,B1` → `'=A1,B1` → quoted as `"'=A1,B1"` (also correct) - Comment cites CWE-1236 / OWASP — clear documentation of intent ### Other 8 files Unchanged from round 1 approved state, no regressions. ### Follow-up (non-blocking, defer) - MEDIUM: EGRN field-name drift between FE `ParcelEgrn` type and backend B5 keys (still applies from round 1) — track when B5 extended deploys. Merging.

lekss361 merged commit fd4eb8c6f4 into main

2026-05-17 22:55:05 +00:00

lekss361 referenced this pull request from a commit

2026-05-17 22:55:06 +00:00

feat(sf-fe-a5): Section 1 Инфо об участке — HeadlineBar + KPI + EGRN + POI + Export (#346)

lekss361 referenced this pull request from a commit

2026-05-17 23:30:59 +00:00

fix(sf-fe): decode cad URL param + hide entry sidebar when drawer open

lekss361 referenced this pull request

2026-05-17 23:31:26 +00:00

fix(sf-fe): decode cad param (B5 400) + hide entry sidebar when drawer open #350

lekss361 referenced this pull request from a commit