feat(web): overruns dashboard - #171
Conversation
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
Преглед на PR #171 (overruns dashboard)
Прегледах PR-а в реалния му обхват — само overruns промените върху trends базата (git diff pr-170...pr-171): новият маршрут /overruns, заявката queries/overruns.ts, lib-овете за графиката/инспектора и cpvBucket в config. Кодът е добре структуриран — чиста геометрия в overruns-chart.ts, споделени SQL константи (DELTA/PCT/SIGNING/OVERRUN_WHERE), маршрутът ползва @sigma/shared форматерите. Едно реално correctness нещо плюс няколко по-малки за подреждане. Виж inline. 🙏
Две бележки извън дифа:
- Покритие на
cpvBucket: bucket-ите (works/goods/services) се водят от ръчно поддържан списък service-дивизии. Днес и 21-те съвпадат сCPV_SECTORS, но нов service-код, добавен вCPV_SECTORSбез обновяване на сета, тихо ще попадне в „goods". Струва си тест, който твърди, че всеки код отCPV_SECTORSсе резолвва до non-default bucket. - Тяло на PR-а: описанието завършва с „🤖 Generated with Claude Code". Конвенцията на репото забранява Anthropic атрибуция в GitHub съдържанието (виж
AGENTS.md— чиста история, CI може да grep-ва за това); бих помолила да отпадне.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Пуснах целия stack локално и този е силен — проверих го от всеки ъгъл и почти всичко е както трябва. Потвърдих: €1000 подовият праг е удвоен и в JS-а (mapOverrunRows), та near-zero signing не може да гръмне pct-а до Infinity/NaN; NULL-овете (value_suspect/annex_suspect) падат сами през current > signing; растежът по възложител/сектор е €-претеглен (SUM(delta)/SUM(signing), не средно на процентите) и делението е guard-нато; медианата е коректна (window + integer-division прозорец, COALESCE(AVG,0) при n=0); scatter-ът floor-ва log-оста на minPctFloor и пази logSpan || 1; overrunBarGeometry има current<=0 guard; cpvBucket е чист дял (минах всичките 45 division-а срещу CPV каталога + Директива 2014/24/EU — services сетът е пълен, 44/43→goods, 45→works, unknown→other); а /overruns чете само by (keyed).
И най-важното — framing-ът е честен: headline KPI-ят е МЕДИАНА, а средното е само в tooltip с изричното „изкривено нагоре от малкото огромни раздувания". Точно така трябва. 👏
Едно watch-item (детайл инлайн) и една координационна бележка:
- perf (watch, не блокер): corpus single-pass-ът е единственото пълно сканиране на таблицата.
- това е PR 3/4 и наследява cache-key промяната от #169 — чийто изтрит CWE-349 drift guard е отделният блокер на стека. /overruns сам не въвежда нов drift (чете само
by, keyed), но #169 трябва да се оправи преди което и да е от стека да влезе; merge-редът (dash-base → trends → overruns) така или иначе го налага.
Approve по същество.
|
Благодаря за щателния преглед — всичко адресирано (force-push 🔴 Анекс хронология: |
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Върнах се на този след fix-овете — анекс хронологията (nulls last) и median dedup-ът са наред. Но CPV нормализацията е приложена върху грешната стойност, та не затваря докрай ръба, за който беше добавена.
by-sector заявката selectва substr(t.cpv_code, 1, 2) AS division и после JS re-keyва cpvDivision(r.division) (≈ред 524) — т.е. нормализира вече отрязаните 2 символа. Leaderboard-ът го прави правилно върху пълния код: cpvDivision(r.cpv_code) (ред 269). При водещ не-цифров символ двете се разминават:
| cpv_code | leaderboard | by-sector |
|---|---|---|
45000000 |
45 | 45 |
' 45000000' |
45 | 4 |
' 9000000' |
90 | 9 |
Тоест същият договор попада в различен CPV сектор в класацията спрямо таблицата по сектори, а fix-ът не постига заявената цел („stray leading char"). cpv_code се пази суров (ingest-ът го взима като text, normalize-raw.sql не го trim-ва), та водещ боклук може да стигне до сервирания tenders.
Защитната поправка: selectни пълния t.cpv_code в by-sector заявката и пусни cpvDivision() върху него (както на ред 269) — тогава двете повърхности съвпадат. Дребно и зависи от това колко мръсни CPV кода реално има, но е дефанзивно.
|
Отразено в cdfe174:
|
|
Perf доуточнение върху предишния fix ( |
|
По молбата —
Approve; merge след #170. Same 0002-миграция бележка като на #170 — преномерирай на |
|
Преглед на PR #171 — „overruns dashboard" ( Сигурност / SQL (OWASP A03 — Injection)Прегледах всяка заявка в
Цялост на даннитеПотвърждавам наблюденията на колегите: €1000 подов праг двойно защитен (WHERE + Забележки (не блокери)
Обхватът е чист (само overruns файловете спрямо ВЕРДИКТ: APPROVE (по същество) — при условие че миграцията се преномерира на |
|
Rebase-нат върху main с css split (ov-* → styles/pages.css; 0002_contracts_overrun_index остава валиден номер), prettier-чист, линеен. Старите нишки резолвнати — всички находки бяха адресирани и препотвърдени в последвалите прегледи. |
|
Добавени са ⓘ дефиниции на колоните в двете таблици („Кои институции раздуват най-много" и „Раздуване по сектори") — какво означава метриката и как се смята (растежът е претеглен по €, не средно на процентите; включени са само договори с анекс, текуща > подписана и подписана ≥ 1000 €). Плюс дишащо поле вдясно в редовете на „Договори по мащаб". Нов head: 52687a6. |
108d095 to
b99bb14
Compare
Преглед на PR #171 — „overruns dashboard" (
|
Преглед на PR #171 — „overruns dashboard" (
|
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR: feat(web): overruns dashboard
Какво прави PR-ът
PR-ът въвежда ново публично, кеширано (publicCache(1800)) табло „Раздуване" (overruns) в apps/web и свързаните с него пренаписвания. Основните части са:
- Нови маршрути:
routes/overruns.tsx(табло с раздувания по орган/сектор и inspector за анекси) и пренаписанroutes/trends.tsxв изглед „Договори — обзор" с три ъгъла на гледане (време / CPV / кръстосано). И двата са SSR-safe, без-JS, управлявани изцяло през query string. - Чиста логика/математика, изнесена за тестируемост:
overruns-chart.ts,overruns-inspector.ts,ComboTrendChart/TrendChart(рефакториран къмTrendGranularity). - Валидация на вход и кеш-хигиена:
filters.ts(cpvGroupSelectionс whitelist^\d{5}$, dedupe, таван 10) иcache-key.ts(премахнатg, добавенstep). - DB слой: нова заявка
overruns.ts,SECTOR_KEY_SQL,config(cpvBucket/cpvDivision), миграция0002_contracts_overrun_index.sql(частичен индекс поannex_count > 0). - Стилове: голям прираст в
styles/components.css(+904) иstyles/pages.css(+3080). - Обширни, смислени unit тестове върху чистите модули, SQL хелперите и drift guard-овете.
Сигурност (Phase 0 / OWASP) — ЧИСТО
Във всички прегледани партиди: няма хардкоднати тайни, нови/променени URL адреси, нови зависимости или злонамерени шаблони (eval, dangerouslySetInnerHTML, обфускация, backdoor). Обработката на вход всъщност е сигурностно подобрение: cpvGroupSelection, pick() whitelist-и, строга валидация на year (/^20\d\d$/), нормализация на by до 'percent'|'absolute' — предотвратяват SQL-scope отравяне и разрастване на cache-key варианти (CWE-349). XSS повърхността е покрита от автоматичното React екраниране.
Най-важни находки (незадължителни, не блокиращи)
- Кеш колизия при миграция на
g→step(cache-key.ts):gе премахнат от allow-list-а; акоtrends.tsxвсе още четеsp.get('g'), два изгледа с различна гранулярност биха споделили кеш запис. Drift guard-ът вcache-key.test.tsпроваля CI при все още консумиранg, така че е покрито — моля потвърдете, чеtrends.tsxе мигриран къмstep. - Отслабен dead-key guard (
cache-key.test.ts): вече се допуска allow-list ключ без консуматор, ако носи route-бележка. Съзнателен компромис, но донякъде отслабва откриването на мъртъв код. pages.css(+3080) не бе прегледан — diff-ът липсва (бинарен/твърде голям). Нужен е ръчен преглед заurl()/@importкъм външни домейни и за дублиран/мъртъв код.- Дребни устойчивости: non-null assertion
rows[selected] ?? rows[0]!иkey={s.code}(възможен празен низ) вoverruns.tsx; дедупликация наmissingпрезSetвtrends.tsx; повтарящ се шрифтов литерал и възможен мъртъв.trend-colвcomponents.css. - UX уточнение: при ъгъл „Време × CPV" изборът на година филтрира само списъка договори, но не и графиката — потвърдете, че е желаното поведение.
- Достъпност: tooltip в
ComboTrendChartе самоonMouseEnter(недостъпен с клавиатура) — приемливо, тъй като данните дублират в достъпни year-карти.
Блокиращи концерни
Няма потвърдени блокиращи проблеми в прегледаните файлове. Единствено условие преди пълно одобрение: ръчен преглед на непрегледания pages.css (партида 5) и потвърждение, че trends.tsx е мигриран към step (точка 1). Пълните гейтове за покритие и интеграция на продукционния SQL с маршрутите се затварят едва при консолидация на всички партиди.
Обща оценка
Чиста, добре структурирана и добре тествана работа с осезаема сигурностна закалка при обработката на вход. 5 от 7 партиди са APPROVE; 2 са COMMENT само поради липсващ diff и междупартидни зависимости, а не поради дефекти. Препоръка: одобрение при затваряне на двете условия по-горе.
# Conflicts: # apps/web/app/lib/filters.test.ts
…rity Steps 1-4 of the requested review fixes (CSS section naming, .fs-btn/ .trend-fs-btn consolidation, --paper-raised token, cache-key dead-entry test) were already landed in an earlier push on this branch; verified against current HEAD and left untouched. Only the ComboTrendChart onFocus mirror was outstanding.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обобщено ревю на PR: feat(web): overruns dashboard
Какво прави PR-ът
Добавя ново табло „Overruns" (преразходи по договори) в уеб приложението и рефакторира свързаната страница „Trends/Договори — обзор". Промяната обхваща целия стек:
- Презентационен слой (React/SVG): нови графики и компоненти —
ComboTrendChart,TrendChart(с добавенаquarterгранулярност),FullscreenButton/useFullscreen,MetricInfo, чиста геометрия за диаграми (overruns-chart.ts) и inspector-логика (overruns-inspector.ts). - Route-ове:
overruns.tsx(ново табло с scatter/inspector) и рефакториранtrends.tsxс строго валидиране на URL-вход. - Стилове:
components.css,pages.css(+3080 реда),tokens.css. - Инфраструктура/данни: разширяване на
CACHE_QUERY_PARAMSвworkers/cache-key.ts,api-contract,config(CPV нормализация:cpvDivision/cpvBucket), нова миграция0002_contracts_overrun_index.sqlи нови/променени SQL заявки вpackages/db(overruns.ts,trend.ts).
Обща оценка
Кодът е с високо качество, атомарен и фокусиран върху обхвата на тикета. Сигурността е чиста по цялата верига и е основната силна страна на PR-а.
Сигурност (Phase 0 / OWASP) — ЧИСТО ✅
- Няма твърдо кодирани тайни, нови външни URL адреси, нови зависимости или обфускирани/backdoor шаблони в нито една партида.
- Без XSS/инжекции: целият текст от БД се рендира като React children (авто-escape); няма
dangerouslySetInnerHTML/eval. - SQL инжекции: всички потребителски входове минават през
?placeholder-и и.bind(); интерполираните SQL низове (OVERRUN_WHERE,SECTOR_KEY_SQL,PCT,DELTAи др.) са само от твърдо кодирани константи. CPV групите се валидират с/^\d{5}$/преди SQL (тест потвърждава отхвърляне на45'--). - Защита срещу отравяне на кеша (CWE-349):
cpvGroupSelectionналага allowlist, дедупликация и таван (MAX_CPV_GROUP_SELECTION = 10);CACHE_QUERY_PARAMSе коректно разширен и покрит с тестове. - Устойчивост: ограничени лимити (
clampLimit,MAX_LIMIT = 200), защита срещу деление на нула на всички нива, N+1 избягнат чрез една bounded IN-list заявка.
Най-важни находки (за адресиране преди merge)
- [trends.tsx] Неизползван импорт
singleSelectFilters(мъртъв код). Нарушава правилото „NO DEAD CODE" и вероятно ще счупи lint-а (no-unused-vars). Единствената находка, маркирана като REQUEST_CHANGES. - [overruns.ts] Уточняване на
authorityEik. ВmapOverrunRowsполетоauthorityEikполучаваauthoritySlug(...)— същата стойност като slug-а. За bidder-а има отделна реална ЕИК колона, но за възложителя не. Моля потвърдете, че slug-ът наистина е ЕИК, или добавете реална ЕИК колона вleaderboardSql. - Тестово покритие за нов код.
overruns.ts(561 реда) и чистите функции вtrends.tsx(pick,logMaxи др.) нямат видими unit тестове в своите партиди.trend.tsи слоятconfig/queriesса отлично покрити. При изискване за покритие ≥90% това трябва да се осигури преди merge.
По-дребни, незадължителни находки
- [trends.tsx] Несъответствие в „Обобщение" при активен CPV филтър: „обща стойност"/„договори" са филтрирани, но „CPV групи" (
stats.totalGroups) е глобален топ — може да обърка; означете като глобален контекст или приведете към обхвата. - [trends.tsx] Възможни дубликати към
getCpvGroupMedians— дедупликирайтеmissingпрезSet. - Клавиатурна достъпност: интерактивните scatter-точки (
<circle onClick>) и SVG правоъгълниците вComboTrendChartне са фокусируеми/нямат клавиатурен обработчик. Смекчено от паралелните списъци/бутони иrole="img"; нисък приоритет. - [pages.css] Липсва видим patch (+3080 реда) — изисква ръчна проверка за дублиране/мъртъв CSS и външни
url()препратки. - [CSS] Модерни функции (
color-mix(in oklch),backdrop-filter,min()) — потвърдете спрямо матрицата за поддържани браузъри; повторение на шрифтовия стек в ~15 правила. - [миграция 0002] Частичен индекс покрива само
annex_count; обмислете composite индекс, ако агрегатите станат тесни.
Блокиращи концерни
Няма блокиращи проблеми по сигурност или коректност. Единственото формално блокиране е находка №1 (мъртъв код в trends.tsx) заради строгите quality gates. След премахване на неизползвания импорт, потвърждаване на ЕИК-логиката (№2) и осигуряване на тестово покритие за overruns.ts (№3), PR-ът е готов за merge.
Обща препоръка: REQUEST_CHANGES — само заради мъртвия код; всичко останало са уточнения/препоръки. Качеството и сигурността са на много високо ниво (~9.3/10).
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах стриктно на връх a1acc38. Архитектурата е solid: споделен OVERRUN_WHERE (leaderboard и агрегати не могат да се разминат), €-претеглен SUM(delta)/SUM(signing) (а не AVG(pct)), SECTOR_KEY_SQL ≡ cpvDivision доказан на dirty корпуса, no N+1, коректно ORDER BY на анексите (недатираните последни).
Блокер (все още отворен на HEAD — причината за CHANGES_REQUESTED на @ydimitrof):
apps/web/app/routes/trends.tsx:17—singleSelectFiltersе import-нат, но не се вика никъде (ползва се самоcpvGroupSelection, ред 63). Dead import → no-dead-code/lint блокер. Fix:import { cpvGroupSelection } from '../lib/filters';
Major (accuracy/framing, ново):
packages/db/src/queries/overruns.ts:154—OVERRUN_WHEREнямаvalue_flagфилтър.signing/current IS NOT NULLизключватvalue_suspect/annex_suspect, ноreview-флагнати договори (напр. ≥10× прогноза, с попълнени EUR) влизат в /overruns. Разминава се с #239, който gate-ва целия anomaly корпус наvalue_flag = 'ok'. Решение за автора: илиAND c.value_flag IN ('ok','review')+ уговорка в методологията, или само'ok'за консистентност с #239. В момента е тихо включване, недокументирано в бележката под таблицата.
Дребни:
apps/web/app/routes/overruns.tsx:437и:503—{count(r.annexCount)} анексадава „1 анекса" приannexCount === 1; ползвайтеplural(r.annexCount, 'анекс', 'анекса')(helper-ът съществува в@sigma/shared).packages/db/migrations/0002_contracts_overrun_index.sql— номерът се сблъсква с #212/#170/#172; преномерирай на следващия свободен (0003+) преди merge, иначеwrangler d1 migrations applyдава duplicate-version.
#239 overlap: вече няма файлов конфликт; семантичното припокриване е легитимно (/overruns = пълен подреден корпус, по-нисък праг; #239 annex_growth = scored сигнал, ok-only).
Изисквам промени (блокерът + решението за review-флаговете).
- import.mjs: gate checkContractFeaturesIntegrity in runFullDerive/runSliceDerive too, matching ship-domain.mjs, so a local derive enforces the same contract_features invariants as the daily prod ETL. - precompute.sql: stop guessing an FX rate from today's date for tenders with a NULL published_at — leave estimated_value_eur NULL explicitly instead. - validate-health.mjs: exclude the synthetic NA bucket from the >60%-NULL threshold check (it's informational-only, out of the check's stated 2020-2026 scope); guard the decile-correlation check against zero-variance pillars instead of printing NaN. - trendAxis.ts: drop the stale TODO(midt-bg#170), already tracked by the issue itself. Reply drafts for the 4 confirmation-only threads (no code change needed): - filters.ts qualityRankingControls: confirmed — quality.ts's rankSql call binds every rank param exclusively via `.prepare(...).bind(minScored, ...rangeParams, top)` (packages/db/src/queries/quality.ts:235-236), never string-interpolated; rankFrom/rankTo/rankDir are re-validated in qualityRankingControls (regex + numeric clamp) before reaching the query. - analytics.tsx getQualitySummary(...).catch(...): thanks, confirmed. - 0003_contract_health.sql numbering: confirmed both — (1) 0002_contracts_overrun_index (PR midt-bg#170/midt-bg#171, not yet merged) adds no column/table these ALTERs read, so applying 0003 first is safe; (2) every environment applies migrations only via `wrangler d1 migrations apply` (tracked, once-only), never raw re-execution. - ship-domain.mjs DROP+CREATE window: acknowledged as a real operational risk, tracked as a separate follow-up per PRD 530 — not re-litigated here.
Make the ComboTrendChart bar keyboard-focusable so its existing onFocus handler actually fires, trim the raw ?cpv query values to the selection cap before the flatMap/map chain, drop an unused filters import in trends.tsx, document the deferred composite-index tradeoff on the overrun partial index, and add an invariant test guarding CPV_BUCKET_WORKS/SERVICES against drifting out of CPV_DIVISION_SET.
# Conflicts: # apps/web/app/styles/components.css # apps/web/workers/cache-key.test.ts # apps/web/workers/cache-key.ts # packages/config/src/index.test.ts
…merge Merging origin/main into pr/overruns surfaced a stale 'g' entry in PARAM_ORDER (superseded by 'step' on main's query-params refactor); add the missing /trends and /overruns params (angle, step, cpvSort, cur, by) alongside it.
…ct resolution .list-search-btn:hover was missing its closing brace after a hand-merge of two branches' additive CSS blocks, swallowing the EU-benchmark indicator block comment and rules into the hover rule and breaking CssSyntaxError in pnpm lint.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах data-слоя стриктно — силна работа. Медианата (rn IN ((n+1)/2,(n+2)/2)), €-претегленият растеж (SUM(delta)/SUM(signing), не средно от проценти), CPV-огледалото SECTOR_KEY_SQL, параметризацията и защитите от деление на нула държат; всички колони съществуват в обслужващата схема, а миграцията добавя само идемпотентен частичен индекс. Две неща:
1. Зависимост от #257 (евро-анекси), тук с разрушителна посока. Предикатът current_value_eur > signing_value_eur и делтата четат current_value_eur, който за договор в лева с евро-анекс от 2026 е наполовина (÷1,95583). Ефектът е от „подценена делта ~20×" (удвояване изглежда като +5% и потъва в класацията) до „изпада напълно" (наполовинената текуща пада под подписаната). И си личи на самата страница: инспекторът на анексите конвертира всеки анекс по неговата am.currency (коректно) и показва вдигане до €X, а редът в класацията показва €X/2. Не е дефект на този PR, но /overruns е най-потърпевш — редно е да влезе след #257.
2. Физически лица (политика, не блокер). /overruns е индексируема (seoMeta, без noindex) и показва име+ЕИК; entityName не маскира физически лица. Съвпада с досегашното поведение (flows/competition правят същото) — не е регресия. Но изгледът by=percent изкарва отгоре малките високопроцентни договори, а именно там излизат ЕТ/физически лица, при това под рискова рамка („раздуване"). Същият клас като #236 — струва си да се реши на ниво политика за целия сайт, не тук.
Механиката — одобрявам. Останалото е координация с #257.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR — feat(web): overruns dashboard
ВЕРДИКТ: COMMENT — няма блокиращи проблеми по сигурност, коректност или производителност. Няколко незадължителни бележки и две точки за потвърждение с автора преди финално одобрение.
Какво прави PR-ът
Добавя таблото „overruns“ (раздути договори) към уеб приложението заедно със свързания „trends“ обзор. Промяната обхваща: SSR графики и UI помощни компоненти (ComboTrendChart, TrendChart, FullscreenButton, MetricInfo), чиста геометрия (overruns-chart, overruns-inspector), парсване/валидиране на URL параметри (filters, query-params), маршрутите /overruns и /trends, презентационен CSS (components.css, pages.css), нови DB заявки в packages/db (агрегати за раздуване, CPV фасетиране, перцентили, медиани, списък с договори), нов частичен индекс с миграция, типове в api-contract и токени в tokens.css. Придружено е от силни, нетривиални тестове.
Сигурност (Фаза 0) — ЧИСТО
- Няма твърдо кодирани тайни, нови/променени външни URL адреси, нови зависимости или злонамерени шаблони (
eval,dangerouslySetInnerHTML, обфускация, backdoor).PEG = 1.95583е официалният фиксиран курс BGN→EUR, не тайна. - SQL инжекции — покрито (OWASP A03): всички потребителски стойности минават през параметризирани заявки (
?+.bind(...)); интерполираните в SQL низове са статични константи или избор между два фиксирани литерала (by/sort). CPV групите се валидират с/^\d{5}$/, деупликират и ограничават по брой — защита срещу отравяне на edge-cache ключ (CWE-349) и неограничена SQL работа. Тестовете реално изпълняват SQL в SQLite и покриват опити катоcpv=%27--и"45'--". - XSS: всички стойности минават през JSX escaping; няма
hrefот потребителски вход. Изнасянето на inlinestyle=в CSS пази маршрута CSP-чист — добра практика.
Коректност и цялост на данните — силно
- Геометричните/агрегационните функции са чисти и SSR-безопасни, с честни предпазители срещу деление на нула, NaN и Infinity (двойна защита в SQL и JS за прага
signing_value_eur,Math.max(1, …),> 0 ? … : 0). OVERRUN_WHEREе дефиниран веднъж и споделен;SECTOR_KEY_SQLогледално отговаря наcpvDivision, а JS re-key гарантира консистентност между leaderboard и секторна разбивка. Медианата е коректна за четно/нечетно n. Поправката на YoY (String(Number(year) - 1)) е коректна.key={by}remount-ва компонента и нулираselected; защитни fallback-и и празни състояния са добре обработени. Достъпността е над средното ниво (aria-*,role,sr-only,:focus-visible, touch targets).
Производителност — добро
Новият частичен индекс idx_contracts_overrun е обоснован; страницата прави фиксиран брой ограничени заявки; тестовете изрично пазят срещу дублиран COUNT(*) и N+1 при анексите.
Точки за потвърждение преди APPROVE (не блокиращи)
- Тип на годината (
trends.tsx): сравнениетоy.year === yearразчитаTrendYear.yearда еstring; ако еnumber, картите никога няма да са активни. Моля потвърдете типа. - Знаменател в
getCpvGroupStats:totalGroupsброи групи вtendersбез филтърamount_eur > 0, докатоgroupsидват от договори с> 0— вероятно умишлено, но да се документира, за да не изглежда като разминаване.
Незадължителни бележки
cpvGroupSelection:.slice()преди валидацията може да отреже валидни кодове при враждебен вход (поведението остава безопасно).- Каноничност на CPV за кеша: нормализирайте (сортирайте)
cpvSelв loader-а, за да не се разпилява edge-cache при различен ред на?cpv. - Възможни
RangeErrorпри spread върху голям масив (Math.max(1, ...rows.map())) —reduceпремахва риска; възможна колизия на Reactkeyпри празенcode; дублирана магическа стойност24; дедупликация наmissingпрезSet. - Секторната
GROUP BY sector_keyняма SQLLIMIT(secLimitсе прилага чак в JS) — нищожен риск, но обмислете таван/мониторинг. - Миграцията
0002_contracts_overrun_index.sqlняма down-стъпка — добаветеDROP INDEX, ако проектът не е само-forward. - CSS: непоследователни font fallback-и; повтарящ се стек
'IBM Plex Mono', var(--font-mono)(капсулирайте в токен); проверетеcolor-mix/backdrop-filterспрямо матрицата от браузъри и z-index скалата.
Ограничение на прегледа
Партида 5 (pages.css, +3080/−3) не беше достъпна за инспекция (липсващ diff/бинарен). За нея сигурността и качеството не са верифицирани — препоръчва се ръчна проверка за външни url()/@import, легаси вектори и дублиране/мъртви селектори, преди окончателно сливане.
Обобщено: кодът е с високо качество, добре структуриран, параметризиран и OWASP-съобразен, с честна обработка на гранични данни и силно тестово покритие. Няма блокиращи проблеми. Препоръка: адресирайте точки 1 и 2, потвърдете съдържанието на pages.css, след което PR-ът е готов за одобрение.
…e key - cpvGroupSelection: keep a loose raw-input bound (MAX_CPV_RAW_VALUES) to cap flood work, but apply the precise MAX_CPV_GROUP_SELECTION slice only after comma-split + trim + validation + dedup, so valid codes are never truncated by leading invalid ?cpv= params. - trends loader: sort cpvSel before use so a reordered ?cpv= URL produces the same canonical edge-cache key, matching hrefToggleCpv's own sort.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR: feat(web): overruns dashboard
ОБЩ ВЕРДИКТ: COMMENT (незадължителни бележки) — няма блокиращи проблеми. Сигурността е чиста във всичките 7 партиди; един пакет (query слой) получи APPROVE. Одобрете след потвърждаване на дребните точки по-долу.
Какво прави PR-ът
Добавя ново табло „Overruns" и преработва страницата „Тренд във времето" в „Договори — обзор" с три ъгъла на гледане (време / CPV / кръстосано). Включва:
- Нови маршрути
apps/web/app/routes/overruns.tsxиtrends.tsx— чисти, SSR/no-JS съвместими презентационни компоненти (всички контроли са GET<Link>). - Изнесена чиста геометрия/логика в
lib/overruns-chart,overruns-inspectorи др., с задълбочени unit тестове (clamp-ове, враждебни входове, празни/изродени случаи). - CPV мулти-селекция с валидация, SVG разпределителни ленги и scatter графики.
- Нов query слой в
@sigma/db(overruns.ts,trend.ts) с 620+ реда тестове, адитивна миграция (CREATE INDEX IF NOT EXISTS), промени вapi-contract/config. - Голям обем нови стилове:
components.css(+905),pages.css(+3080),tokens.css.
Сигурност — ЧИСТО във всички партиди ✅
- Няма твърдо кодирани тайни, нови зависимости, промени по URL адреси,
eval/dangerouslySetInnerHTML, обфускация или бекдори. - SQL инжекция (OWASP A03) — оценено и отхвърлено: целият SQL се сглобява от константи; всички стойности от повикващия минават параметрично през
.bind(?)/IN (?, …); лимитите презclampLimit;by/ORDER BYизбират между фиксирани низове. CPV входът се валидира с/^\d{5}$/преди SQL. CPV фасетирането интерполира само структурата на клаузата, не операнди. - Отравяне на кеш-ключ (CWE-349): смекчено — строга валидация, дедупликация, двоен лимит, сортиране и канонична подредба на CPV параметрите.
- XSS: няма — целият изход минава през JSX escaping.
Коректност и цялост на данните — солидно
- Навсякъде защити срещу деление на нула,
log(0)и NaN (clamp-ове,Math.max(1, …),> 0guard-ове,COALESCE). SECTOR_KEY_SQLе доказано огледало наcpvDivision; медианите са коректни за нечетно/четно n; статусите се извеждат само от реални дати.- Тестовото покритие е смислено и насочено към разкриване на дефекти, не тривиално.
Незадължителни бележки за потвърждение (не блокират)
- Кросс-пакетна съгласуваност
?g→step/angle: уверете се, че няма останали четци на?g=и че старите кеширани линкове се игнорират (потвърдете сcache-key.test.ts). - YoY върху нулево-запълнени gap-години (
trend.ts~ред 220): липсваща година дава −100% YoY. Потвърдете, че това е желаното продуктово поведение и че фронтендът визуално различава „дупка" от реален спад. - Консистентност на филтъра по стойност: тренд
scope()използваamount_eur IS NOT NULL, докатоlistOverviewContracts/разпределенията използватamount_eur > 0— потвърдете, че тоталите не се разминават. - Достъпност: фокусируеми
<rect>/<circle>вътре вrole="img"SVG може да не се обявяват от екранни четци; смекчено чрез достъпни алтернативи (year-карти, бутони сaria-pressed) — струва си проверка с екранен четец. - Документация vs. поведение: JSDoc за
bigвoverruns-chart.tsи коментарите за „~30 rows" вGROUP_DIST_SQLса леко подвеждащи (поведението е коректно). - CSS поддръжка: дублиран селектор
.trend-chart-panel--full .trend-chart-body; смесване на литерал'IBM Plex Mono'с токен; възможен мъртъв.trend-col; голямото нарастване наpages.css(+3080) си струва проверка за дублиране. - Дребни оптимизации: възможни дублирани CPV групи в
missingкъмgetCpvGroupMedians;topGrowthCodeръбов случай при празен код;rows[0]!non-null assertion.
Отворени за верификация точки
pages.css(+3080/−3): diff-ът не беше наличен в партида 5 — окончателната преценка изисква ръчна проверка върху пълния diff (външниurl()/@import, дублиране, размер).- Тестово покритие за loader-логиката на самите route-ове (
pick,relLabel,logMax) не се видя в прегледаните партиди — потвърдете, че е добавено, за да се спази изискването за тестове.
Кодът е с високо качество (~9.5/10 в query слоя), чиста функционална декомпозиция, консистентно именуване, без частична имплементация/TODO/дублиране. Няма находки, изискващи промяна преди merge — само изброените потвърждения.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Ре-верифицирах на HEAD (d6949279): новият комит („cap cpv selection after validation + canonical /trends cache key") е валидация/кеш-ключ — не пипа OVERRUN_WHERE или стойностната база. Двете ми бележки стоят: (1) зависимост от #257 (евро-анекси), където /overruns е най-потърпевш; (2) физически лица под рискова рамка на индексируема класация. За (2) вече има прецедент — #237 добавя noindex за договори с изпълнител ЕТ; същият модел е редно да се приложи и тук. Механиката остава одобрена.
Address ydimitrof's review on PR midt-bg#171: - overruns-chart.ts: sample x-axis ticks evenly across the log range (keeping first+last) instead of the lowest 5, so wide ranges label their upper end too - overruns-chart.ts: correct the `big` field's JSDoc to match its real definition (>=50% of the corpus's max € overrun, not top-half-by-€) - ComboTrendChart.tsx: drop tabIndex from bars — they were focusable children of an opaque role="img" svg, producing nameless focus stops; hover tooltip remains for pointer users - trends.tsx: dedup CPV groups via a Set before getCpvGroupMedians to avoid redundant lookups when several contracts share a group
…ed composite Replace the annex_count-only partial index with PR midt-bg#169's composite (signing_value_eur, current_value_eur) index and its EXPLAIN QUERY PLAN benchmark comment, so both PRs converge on one canonical migration instead of shipping conflicting copies of 0002_contracts_overrun_index.sql
…ter RSC CVE Bump postcss to ^8.5.18 (GHSA-r28c-9q8g-f849, path traversal via sourceMappingURL auto-load) and valibot to ^1.4.2 (GHSA-5qjj-4xww-7phc, flatten() crash on inherited-property keys) via pnpm-workspace.yaml overrides — both patch-level, non-breaking fixes. Add osv-scanner.toml with a suppression for GHSA-qwww-vcr4-c8h2 (react-router CSRF in unstable RSC code paths only), following the same documented, time-boxed pattern as the existing sharp suppression. This app doesn't use RSC (verified via repo-wide grep) and no fix exists in the 7.x line; the fix requires a major 7.x->8.x bump that is out of scope here.
- apps/web/app/routes/trends.tsx: kept pr/overruns's full obzor (time/cpv/cross lens) rewrite, applied main's getDb() read-only D1 chokepoint guard instead of direct context.cloudflare.env.DB access - apps/web/app/routes/overruns.tsx: this branch's own pre-existing direct env.DB usage also needed the getDb() chokepoint guard (caught by main's readonly-db-chokepoint.test.ts after merging) - osv-scanner.toml: kept both independently-added suppressions (react-router RSC CSRF from this branch, sharp/miniflare from main) - packages/db migrations: renumbered this branch's 0002_contracts_overrun_index.sql to 0003 to resolve a numbering collision with main's 0002_current_value_currency.sql; updated migrations.test.ts to load both in the chain - pnpm-lock.yaml: regenerated via pnpm install
… + hardening) (#212) * perf(db): ordering indexes for the non-default list sorts The list pages keyset-paginate with ORDER BY <sortExpr> <dir>, <id> <dir> LIMIT N. Six user-selectable sorts had no matching index, so the planner fell back to a full table SCAN + temp-B-tree ORDER BY on every page (D1 bills rows scanned): /contracts date-desc, date-asc (idx_contracts_signed is on the bare column, not the COALESCE(signed_at, ...) expr the query uses) /companies count, authorities /authorities count, avg Add one index per missing sort, matching the exact ORDER BY expression plus the keyset id tiebreak, so SQLite walks the index and stops at LIMIT. Additive, idempotent; rollup tables are DELETE+INSERT-refreshed so the indexes survive ships. A sqlite3 EXPLAIN QUERY PLAN test proves each sort full-scans before and index-walks after. * fix(web): escape < in the JSON-LD data island (defense-in-depth) root.tsx embeds JSON-LD via dangerouslySetInnerHTML with a raw JSON.stringify. JSON.stringify does not escape '<', so a '</script>' in any string value would close the <script> element early (stored XSS) — the exact sink the project's own review standard (docs/review-security.md) requires be escaped. Today only the request origin reaches the graph (new URL() cannot make it carry '</script>'), so this is not currently exploitable; the jsonLdScript helper closes the sink pre-emptively for any DB/user-derived field added later. A unit test proves '<' is escaped, U+2028/U+2029 are escaped, and the output stays JSON-equivalent. * chore(db): renumber list-sort-indexes migration 0002 → 0005 De-conflict the migration number: 0002 is claimed by the contracts_overrun_index family (#169/#170/#171/#172), 0003 by #188 (contract_health), and 0004 by #210 (cpv_division_stats). 0005 is the next free number. Additive/idempotent, so final merge order stays the maintainer's call; this just removes the known 0002 clash. * test(db): apply all migrations + cover keyset pages; guard jsonLdScript(undefined) Address the review notes on the list-sort-indexes PR: 1. The sort-index test now applies EVERY migration on the branch (discovered from the migrations dir), not a hardcoded 0000/0001/000N subset. The "BEFORE" base is exactly the real served schema minus this PR's index, and the test survives any renumbering. (Confirmed: company_totals/authority_totals are created in 0000 and nothing between affects these sort plans.) 2. Each sort now asserts the plan on the keyset page too - the real paginated path `WHERE (expr <cmp> ? OR (expr = ? AND id <cmp> ?))`, not only the first page. Full-scans BEFORE and index-walks (no temp B-tree) AFTER, on both pages. 3. jsonLdScript now returns "null" when JSON.stringify yields undefined (undefined / function / symbol) instead of throwing on the following .replace - defense-in-depth for the documented "safe for any future field" helper. Covered by a test. * refactor(web): rename jsonLdScript → serializeJsonForScript + document sort-index sync Address the (non-blocking) review nits: - Rename jsonLdScript to serializeJsonForScript: the helper returns a serialized JSON string safe to embed in an inline <script>, not a <script> element (review ydimitrof). Updates root.tsx and the test. - Document the sentinel sync: the COALESCE defaults in queries/contracts.ts SORTS ('' / '9999-99') must stay byte-identical to the expression indexes, or SQLite silently drops the index and falls back to a full scan + temp-B-tree sort. Added reciprocal SYNC comments in the migration and the SORTS map, both noting that list-sort-indexes.test.ts's EXPLAIN assertions catch a drift. * docs(db): state the boundaries of the sort-index guarantee (review) Document the two known limits of the EXPLAIN-plan proof, per review: (1) the local sqlite3 CLI planner is not version-identical to Cloudflare D1's (a strong indication, not a bit-exact production proof; the binary itself is a pre-existing suite-wide dependency), and (2) the index-walk guarantee covers the UNFILTERED sort paths - with an active filter the planner may prefer the filter's index and temp-sort the much smaller filtered set, which is the correct trade. Comment-only. * chore: drop internal review-marker traces from code comments Remove the '(review ydimitrof)' attribution artifacts from json-ld.ts and list-sort-indexes.test.ts comments; the explanations stay. Comment-only. * refactor(web): share one JSON-for-script serializer between the JSON-LD island and .json route The .json contract endpoint had its own safeJson escaper, a second implementation of the same <script>/separator escaping as serializeJsonForScript - a DRY smell the comment itself admitted, and a drift risk (one could add a U+2028 escape the other lacks). Route it through the shared serializer instead. It escapes every `<` (vs the old `</`-only form) - JSON-equivalent, harmless for the JSON body, strictly safer. Also document, in the shared helper, why `>` and `&` are deliberately left unescaped (only `<` can start a token in a script raw-text context), with a test that locks it. * fix(web): set nosniff on the .json route; add planner-independent sentinel-sync test - contract.json.tsx: the actual MIME-sniffing defense is X-Content-Type-Options: nosniff, not the content escaping. The worker already sets it globally (baseSecurityHeaders); set it explicitly on this resource route too so it is safe on its own, and correct the comment that over-credited the escaping (review). - Add sort-index-sentinel-sync.test.ts: the date-sort index only matches while its COALESCE sentinel is byte-identical to SORTS in queries/contracts.ts. A .sql migration can't import a TS constant, so guard the coupling with a static cross-file check of the sentinels ('' and '9999-99') that fails on drift regardless of the DB engine - independent of the local sqlite3 planner the EXPLAIN test relies on (review). * chore: drop stray review-marker artifacts from this PR's comments Remove the bare '(review ...)' attribution notes I left in contract.json.tsx and sort-index-sentinel-sync.test.ts; the explanations stay. The pre-existing '(review #80)' issue references elsewhere are an established convention and are untouched. Comment-only. * test(db): cover filtered list sorts and guard the sqlite3 dependency Two review follow-ups on the ordering-index test: - Filtered sorts were documented as out of scope, leaving the reader unable to tell whether an active list filter makes the ordering index redundant. It does not: with a sector (tenders.cpv_code) or eu-funded filter the planner still walks idx_contracts_signed_desc and drops the sort step, while the pre-index baseline sorts the whole table. Asserted both directions. - A missing sqlite3 CLI surfaced as an opaque ENOENT. Probe it in beforeAll and fail with the fix. Deliberately not a skip: this is a perf/cost gate, and silently passing it on an image without sqlite3 would retire the gate. --------- Co-authored-by: Rumen Slavov <26761822+B353N@users.noreply.github.com> Co-authored-by: todorkolev <tkolev@obecto.com>
|
Този клон е в конфликт с |
4 participants
What changed
Overruns dashboard (
/overruns) and the обзор /trends page, plus a full pass of ydimitrof's review rounds:d694927— moved the preciseMAX_CPV_GROUP_SELECTIONcap incpvGroupSelection(lib/filters.ts) to after comma-split/trim/validation/dedup, keeping only a loose raw-input bound (MAX_CPV_RAW_VALUES) beforehand so leading invalid?cpv=params can no longer crowd out valid codes.d694927— sortedcpvSelin the/trendsloader to matchhrefToggleCpv's own sort, so a reordered?cpv=URL with the same selection collapses onto one canonical edge-cache key.TrendYear.yearis declaredstringin@sigma/api-contract, matching the loader's regex-derivedyear, so they.year === yeartoggle comparison is correct as written.tabIndex={0}+onFocusmirroringonMouseEnter.flatMap/dedup, since superseded by the post-validation cap above.singleSelectFiltersimport fromtrends.tsx(lint fix).0002_contracts_overrun_index.sql(annex_count-only partial is sufficient today; composite alternative noted and deferred).CPV_BUCKET_WORKS/SERVICES⊆CPV_DIVISION_SET) so future drift fails CI instead of silently falling toother.mainin (resolved a real query-param-canonicalization conflict) and fixed a real unclosed-CSS-block bug introduced by that merge.How it was tested
pnpm --filter web typecheckandpnpm --filter web test— full web suite, includingComboTrendChart,filters.test.tscpv-cap coverage, and cache-key coverage.pnpm --filter db test— full db suite, including the CPV-bucket invariant test./overrunsand/trendsafter the merge to confirm the CSS fix and param canonicalization didn't regress rendering.Quality checks
d694927).