Skip to content

feat(web): analyze index — five equal analysis cards - #172

Open
StanislavBG wants to merge 41 commits into
midt-bg:mainfrom
StanislavBG:pr/analyze
Open

feat(web): analyze index — five equal analysis cards#172
StanislavBG wants to merge 41 commits into
midt-bg:mainfrom
StanislavBG:pr/analyze

Conversation

@StanislavBG

@StanislavBG StanislavBG commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

What changed

Analyze index (five equal analysis cards) plus four rounds of review fixes from ydimitrof:

  • MetricInfo's aria-expanded now syncs with the hover/focus-within reveal, not just the click-toggled state.
  • Dropped a dead ternary branch in ComboTrendChart; hardened hasPartial edge case.
  • estimateYoyGrowth now throws loudly on non-monthly input instead of silently returning a flat {value:1,count:1}, and the analytics-landing loader wires growth.value - 1 (not the raw multiplier) into formatYearlyGrowth's ratio-diff contract — added a regression test proving monthly points fold into full calendar years correctly.
  • getOverrunAnnexes' contract-id IN-list is chunked into D1_MAX_BOUND_PARAMS-sized batches (D1 caps bound parameters at 100/statement); the leaderboard row count feeding it is confirmed bounded (MAX_LIMIT = 200, default 50).
  • cpvBucket hardened with an explicit CPV-division partition test so a forgotten CPV added to CPV_SECTORS fails CI instead of silently landing in "goods".
  • Corrected two inaccurate code comments that conflated cache-key cardinality (keyed on the raw request URL) with input content validation (CWE-349 does not apply to either ?cpv or ?cur).
  • cache-key.ts now canonicalizes (sorts) the repeatable ?cpv param before hashing, so ?cpv=A&cpv=B and ?cpv=B&cpv=A map to one cache entry; the cpv-only special-case is now documented inline (fragmentation-only, never poisoning — if a future repeatable param needs the same treatment, add its key alongside).
  • Deduplicated the hardcoded 'IBM Plex Mono', var(--font-mono) stack into a single --font-mono-plex token used across all ~15 rules.
  • Unified .fs-btn/.trend-fs-btn :focus-visible styling (matching outline-offset) for consistent keyboard-focus visibility.
  • Documented the UTC-vs-local year-boundary behavior of strftime('%Y', 'now') in competition.ts; colocated the year lower/upper bound checks in getOpaqueShareByYear for readability (no behavior change).
  • i18n: the abbreviated М (millions) axis label now uses the Bulgarian decimal comma, matching multText.
  • /trends combo-chart hover tooltip is now aria-hidden (dropped the now-contradictory role="status") since it re-renders on every hover move and the same data already lives in the accessible year cards.
  • Consolidated the hardcoded slate swatch/bar-fill colors onto the existing --slate token via color-mix, so the legend swatch and count bars can't drift apart on theme change.
  • Unified the font-serif fallback stack (Georgia, serif) across all three trend title rules.

How it was tested

  • pnpm --filter web test and pnpm --filter api-contract test (includes regression tests: estimateYoyGrowth full-calendar-year folding, cpvBucket explicit division partitioning).
  • pnpm typecheck across affected packages.
  • Manual exercise of the analyze cards and the overruns leaderboard at full page size (annex fan-out).

Quality checks

  • CI green on the current head commit (9a8a6f3).
  • All 7 threads from ydimitrof's 2026-07-20 review round are resolved:
    • 5 addressed with code (c47366d): tooltip aria-hidden, slate color-token consolidation, font-serif fallback unification, cache-key.ts intentional-behavior comment, getOpaqueShareByYear bound colocation.
    • 2 verify-only, no code change needed — traced and confirmed on the current head, with the trace posted as an explanatory reply on each thread: the /trends loader routes through cpvGroupSelection into a fully parameterized-bind query (no raw ?cpv, no string interpolation), and estimateYoyGrowth's sole caller (analytics.tsx) pins granularity: 'month', so the throw can't reach /analytics.

@nedda76 nedda76 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прегледах PR-а (пет равностойни аналитични карти на /analytics + специализирани headline заявки). Кодът е стегнат и добре тестван; по-долу са находките, подредени по тежест. Две са с потребителски ефект (счупена връзка и премахната CI защита), останалите са за съответствие на текст с данни и за дублиране.

Прегледът е автоматизиран; преценете всяка бележка по същество.

Comment thread apps/web/workers/cache-key.ts Outdated
Comment thread apps/web/app/lib/analytics-lenses.ts Outdated
Comment thread apps/web/app/routes/analytics.tsx Outdated
Comment thread apps/web/app/lib/analytics-stats.ts
Comment thread packages/db/src/queries/regions.ts Outdated
Comment thread packages/db/src/queries/regions.ts
@StanislavBG

Copy link
Copy Markdown
Contributor Author

Благодаря — адресирано (force-push 83083e8):

drift guard: възстановен (в базовия #169).
/compare връзка: махнах /compare lens-а от този PR — маршрутът е извън stack-а тук (умишлено изключен), та плочката 404-ваше. На пълния branch /compare съществува.
opaque-share текст: изравнен с метриката → „процедури само с една оферта" (заявката мери само bids_received = 1).
growthMultiple: делегира на formatGrowthFactor — еднакъв формат на /analytics и /overruns.
sectorOptions: консолидиран в споделения sectors.ts (махнати 3-те копия).
мъртви полета: sofiaEur/totalEur махнати от RegionHeadline.

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверих срещу 83083e8: всичките 6 находки на Неда са затворени — CWE-349 drift guard-ът е възстановен (consumed ⊆ allowed, expect(undeclared).toEqual([])), няма линк към несъществуващ /compare, картовият текст за единствена оферта е прецизиран, growthMultiple/sectorOptions са унифицирани. Share-математиката е guard-ната (opaqueHeadline филтрира valueEur > 0, value-weighted, текущата непълна година е изключена). „Конкуренция" картата показва дела за последната пълна година (етикетиран с годината, ред 409) — различен обхват от общата цифра на /competition, не противоречие. Една дребна бележка (не блокер): тази карта всъщност сканира contracts (не rollup, за разлика от описанието в PR-а) — чисто perf. Одобрявам.

@StanislavBG

Copy link
Copy Markdown
Contributor Author

Благодаря за прегледа — по бележката за „Конкуренция" (contracts vs rollup):

Проверка: нито един rollup не носи bids_received (нито разбивка „една оферта") — facet_counts има само година/стойност без split, home_totals/sector_totals също. Т.е. headline-ът не може да се пренасочи към rollup без нова таблица.
Семантика: картата ползва първата и последната пълна година (дял + промяна в пр.п.), така че многогодишният scan е нужен по същество; ограничен е с signed_at >= '2020-01-01' върху idx_contracts_signed.
Действие: без схема-промени в този PR — коригирах описанието на PR-а да казва точно кои headline-и четат rollup-и (Flows, Map) и кои агрегират contracts (Trends, Competition, Overruns), и вписах per-year competition rollup (value + single-bid value) като follow-up.
Loader trace: 7 statements на cold load, без дублирани scan-ове (trend е с includeSectors:false, includeInsights:false, така че тежките insights заявки не се изпълняват).
• Без нов commit — само описанието е обновено. Тестове/typecheck зелени (@sigma/db 170 pass, web 362 pass; 32-те db fail-а са само sqlite3 ENOENT на машината, идентични и на main).

@StanislavBG

Copy link
Copy Markdown
Contributor Author

Рестакнат върху новия #171 — старите trends/overruns къмити отпадат от историята. Една реална адаптация към обзора: estimateYoyGrowth е пренесен в lib/analytics-stats (старият lib/trends-forecast вече не съществува) и отпадна includeInsights от getSpendingTrend; иначе diff-ът спрямо pr/overruns показва само analyze файловете. Нов head: 5ee4d0d; typecheck 7/7, тестовете зелени.

todorkolev pushed a commit that referenced this pull request Jul 3, 2026
Splits the 3353-line app.css into app.css (import manifest) + 9 focused files under styles/ (tokens, base, chrome, layout, components, tables, home, flow, pages). Verified rule-for-rule equivalent to the previous monolith (525 selectors / 1926 declarations, 0 dropped or altered); app.css is now @import-only. typecheck + test + lint green.

Open css-touching PRs (#126, #131, #170-#172) now rebase onto the split (target the styles/ files, not app.css).
@ydimitrof

Copy link
Copy Markdown
Contributor

Прегледах PR #172 стриктно, с акцент върху сигурност и цялост на данните. По-долу е обобщението; прегледът е локален и адверсариален (проверих всяка заявка за инжекции, XSS, CWE-349 дрейф и деление на нула).

Сигурност (OWASP)

SQL инжекции — няма. Проверих всяка нова/променена заявка в overruns.ts, competition.ts, regions.ts, flows.ts, trend.ts. Всички SQL-фрагменти, които се интерполират в стринга (OVERRUN_WHERE, SECTOR_KEY_SQL, CPV_CLEAN, PCT, DELTA, OVERRUN_MIN_SIGNING_EUR, cpvGroupsClause), са изцяло константи или структурни ?-плейсхолдъри — никаква потребителска стойност не влиза в текста на заявката. Целият вход от URL минава през bind-параметри:

  • getOpaqueShareByYear / getFlowsHeadline / getRegionHeadline — статичен SQL без параметри.
  • CPV multi-select (?cpv) минава през cpvGroupSelectionvalidCpvGroups (^\d{5}$, дедуп, cap 10), а cpvGroupRange подава двете граници като bind-параметри в (t.cpv_code >= ? AND t.cpv_code < ?). Half-open диапазонът е коректен и за код, завършващ на 9 (следващ ASCII символ : > 9).

XSS — няма. Няма dangerouslySetInnerHTML, innerHTML, eval или new Function в новите маршрути/компоненти; React екранира всичко.

CSRF / достъп — n/a. Маршрутите са read-only, edge-кеширани GET loaders над публични данни за обществени поръчки; няма мутации, няма секрети, няма външни fetch-ове, няма четене на process.env. analytics.tsx loader-ът не приема потребителски параметри изобщо.

CWE-349 (кеш дрейф). CACHE_QUERY_PARAMS е разширен коректно (cpv, by, step, angle, cohort, metric, a/b …), а дрейф-гардът (consumed ⊆ allowed) е възстановен в базовия PR и покрит от cache-key.test.ts (изпълних го локално — 10/10 зелени).

Цялост на данните

  • Всяко деление е защитено: sofiaShare (totalEur > 0), opaqueShare (filter(valueEur > 0)), PCT (WHERE изисква signing_value_eur >= 1000 + двоен JS guard срещу Infinity/NaN), estimateYoyGrowth (clampGrowth + Number.isFinite, clamp 0.5–2.0). Всеки non-finite вход връща em-dash, не измислена цифра.
  • Миграция 0002_contracts_overrun_index.sql е адитивна (CREATE INDEX IF NOT EXISTS, partial index), без колизия в номерацията (0000/0001/0002 подредени), и е покрита с регресионен тест в migrations.test.ts.

Съответствие с описанието

Описанието вече е приведено в съответствие с реалността след прегледа на @nedda76 и @lyubomir-bozhinov: „Конкуренция" картата коректно е обозначена като contracts-скан (не rollup), опейк-текстът е изравнен с метриката (bids_received = 1), growthMultiple/sectorOptions са унифицирани, мъртвите полета са премахнати. Всичките 6 предходни находки са затворени.

Незадължителни бележки (не-блокери)

  1. CI не се изпълнява за клонаgh pr checks 172 връща „no checks reported on the 'pr/analyze' branch". Зелените тестове са само по думите на автора (170 db / 362 web); препоръчвам да се потвърди, че workflow-ът се закача за този клон преди merge, за да има обективна следа.
  2. Обем/обхват — суровият diff срещу main е 10 091/1 800 по 37 файла, защото това е върхът на 4-PR cross-fork stack. Ревюто по същество трябва да е спрямо pr/overruns; спазвайте реда на merge (dash-base → trends → overruns → analyze), за да не влезе непълен стек в main.
  3. Локално не можах да изпълня analytics-stats/filters тестовете (@sigma/* workspace пакетите не са билднати в worktree-а — среда, не код); cache-key тестовете минаха.

Кодът е чист, добре тестван и без открити уязвимости. Отлична дисциплина по параметризацията и guard-овете.

Вердикт: ОДОБРЯВАМ — няма блокери по сигурност или цялост на данните; адресирайте само CI видимостта преди merge.

@StanislavBG

Copy link
Copy Markdown
Contributor Author

Rebase-нат върху main с css split (az-* → styles/pages.css), prettier-чист, линеен. Старите нишки резолвнати — шестте находки бяха затворени още на 83083e8 и препотвърдени.

@StanislavBG
StanislavBG force-pushed the pr/analyze branch 3 times, most recently from 87546d3 to ab9c6a2 Compare July 3, 2026 17:36
@ydimitrof

Copy link
Copy Markdown
Contributor

Прегледах PR #172 стриктно и локално, с приоритет върху сигурността и целостта на данните. За разлика от предходните прегледи този път инсталирах зависимостите в worktree-а и изпълних тестовете и typecheck-а сам — по-долу са резултатите, а не думи на автора.

Сигурност (OWASP)

A03 Injection — няма. Прочетох ред по ред всяка нова/променена заявка в overruns.ts, competition.ts, regions.ts, flows.ts, trend.ts. Всички SQL-фрагменти, които се интерполират в текста на заявката, са константи или чисто структурни ?-плейсхолдъри; никаква потребителска стойност не влиза в стринга:

  • OVERRUN_WHERE, DELTA, PCT, SECTOR_KEY_SQL, CPV_CLEAN, CPV_GROUP_GLOB, MEDIAN_PCT_SQL — статични константи.
  • cpvGroupsClause генерира само броя (t.cpv_code >= ? AND t.cpv_code < ?) групи; самите кодове идват през bind. Границите се смятат от cpvGroupRange, а не се вграждат.
  • Целият URL-вход е валидиран преди заявката: angle/step/sort/cpvSort минават през pick() allow-list, year през /^20\d\d$/, а CPV multi-select през cpvGroupSelection (^\d{5}$, дедуп, cap 10). Заявките listOverviewContracts и getCpvGroupMedians при това ре-валидират ^\d{5}$ — защита в дълбочина. getOpaqueShareByYear/getFlowsHeadline/getRegionHeadline са без параметри.
  • Проверих граничния случай на half-open диапазона: за код, завършващ на 9, String.fromCharCode('9'+1) дава : (ASCII 0x3A, точно след 9), така че [45239, 4523:) покрива коректно всички под-кодове без препокриване. Пинато и от overruns-sql.test.ts, който изпълних срещу реалния sqlite3cpvDivision(SECTOR_KEY_SQL(code)) ≡ cpvDivision(code) върху „мръсния" корпус: 2/2 зелени.

A03 XSS — няма. Нула dangerouslySetInnerHTML, innerHTML, eval, new Function в новите маршрути/компоненти; React екранира. SVG-графиките (ComboTrendChart, overruns) рендерират само числови геометрии, не сурови стрингове.

CWE-349 (кеш дрейф) — чисто. CACHE_QUERY_PARAMS е разширен коректно (angle, step, cpv, by, cohort, metric, cpvSort, a/b…), старият g е заменен от step, а дрейф-гардът consumed ⊆ allowed е запазен. cache-key.test.ts мина при мен.

Достъп/секрети — n/a. Read-only, edge-кеширани GET loader-и над публични данни; няма мутации, няма process.env, няма външни fetch, няма localStorage. analytics.tsx loader-ът не приема потребителски вход.

Цялост на данните

  • Всяко деление е защитено: opaqueHeadline филтрира valueEur > 0; PCT изисква signing_value_eur >= 1000 в OVERRUN_WHERE; clampGrowth отхвърля non-finite и ≤0 (band 0.5–2.0); overrunBarGeometry колабира до празен бар при непозитивна стойност. Непълни/невалидни входове връщат em-dash, не измислена цифра.
  • „Конкуренция" изключва текущата непълна година (substr(signed_at,1,4) < strftime('%Y','now')) — симетрично с изхвърлянето на частичния as_of период в trend.ts. Тримесечното сгъване (quarterOf/fillPeriods) е коректно и запазва подредбата.
  • Добавените вторични ключове за подредба (…, authority_id, bidder_id / …, c.id) правят LIMIT-натите резултати детерминистични — добра корекция.
  • Миграция 0002_contracts_overrun_index.sql е адитивна и идемпотентна (CREATE INDEX IF NOT EXISTS, partial WHERE annex_count > 0), номерацията 0000/0001/0002 е последователна, покрита с регресионен тест.

Съответствие с описанието

Описанието е приведено в съответствие след предходните прегледи (@nedda76, @lyubomir-bozhinov): „Конкуренция" е коректно обозначена като contracts-скан, опейк-текстът съответства на bids_received = 1, growthMultiple/sectorOptions са унифицирани, мъртвите полета — премахнати, линкът към несъществуващия /compare — махнат (потвърдих, че в diff-а няма /compare href). Няма свързан тикет за съпоставка; описанието на PR-а служи като спецификация и имплементацията му отговаря.

Локална проверка (резултати)

  • @sigma/web: filters + analytics-stats + overruns-chart + overruns-inspector + cache-key71/71 зелени.
  • @sigma/db: overruns + competition + trend + flows + regions + migrations80/80 зелени; overruns-sql (реален sqlite3) → 2/2.
  • pnpm turbo run typecheck7/7 успешни.
  • gh pr checks 172 вече връща зелен check (CI е закачен) — предходната ми бележка за липсваща CI следа отпада.

Незадължителни бележки (не-блокери)

  1. getCpvGroupMedians ползва целочислен ранг (cnt-1)*5/10 + 1, т.е. long-median при четен брой (за cnt=2 взима по-ниската стойност). За baseline-„спрямо типичното" е приемливо и документирано; само отбелязвам, че не е интерполирана медиана.
  2. Обхватът е голям, защото това е върхът на 4-PR cross-fork stack — ревюто по същество е спрямо pr/overruns. Спазвайте реда на merge (dash-base → trends → overruns → analyze), за да не влезе непълен стек в main.

Кодът е чист, дисциплиниран по параметризация и guard-ове, и напълно покрит с тестове, които този път изпълних сам. Не откривам блокери по сигурност или цялост на данните.

Вердикт: ОДОБРЯВАМ — няма блокери; тестовете, typecheck-ът и CI са зелени, съответствието с описанието е потвърдено.

@ydimitrof

Copy link
Copy Markdown
Contributor

Всички проверки минаха. db 238/238, web 375/375, typecheck 7/7. Пиша финалния коментар.


Прегледах PR #172 стриктно и локално — с приоритет върху сигурността и целостта на данните. Този път инсталирах зависимостите в worktree-а и изпълних целия тестов пакет, typecheck-а и SQL-инжекционния анализ сам; по-долу са резултатите, а не думи на автора. Прегледът е адверсариален: третирах всяка нова заявка като потенциален вектор за инжекция и всяко деление като потенциален източник на измислена цифра.

Сигурност (OWASP)

A03 Injection — няма. Прочетох ред по ред всеки интерполиран SQL-фрагмент в overruns.ts, competition.ts, regions.ts, flows.ts, trend.ts. Всичко, което влиза в текста на заявката чрез ${…}, е константа или чисто структурен ?-плейсхолдър — никаква потребителска стойност не се вгражда в стринга:

  • OVERRUN_WHERE, DELTA, PCT, SIGNING, SECTOR_KEY_SQL, CPV_CLEAN, MEDIAN_PCT_SQL, CPV_GROUP_GLOB, YEAR_KNOWN, OVERRUN_MIN_SIGNING_EUR — статични константи, дефинирани веднъж.
  • Всеки where.join(' AND ') / s.join се сглобява само от литерални фрагменти; всяка стойност (sector, year, authorityId, bidderId) отива през .bind(...params).
  • cpvGroupsClause генерира само броя (t.cpv_code >= ? AND t.cpv_code < ?) групи; границите идват от cpvGroupRange и се подават като bind-параметри, не се вграждат.
  • periodLen в getSpendingTrend е 4 или 7, изведени от whitelist-нат granularity (year|quarter|month) — никога от суров стринг.

Валидация на входа (защита в дълбочина). Целият URL-вход е валидиран преди заявката: angle/step/sort/cpvSort/by минават през pick()/allow-list, cpv multi-select през cpvGroupSelection (^\d{5}$, дедуп, cap 10), а getMulti cap-ва на 50 и филтрира sector срещу KNOWN_SECTORS. getOpaqueShareByYear/getFlowsHeadline/getRegionHeadline са без параметри. Проверих граничния случай на half-open диапазона: за код, завършващ на 9 (напр. 45239), горната граница е 4523 + String.fromCharCode(':'/0x3A) = 4523:, което коректно покрива 45239… и изключва 45240 (45240 > 4523:) — без препокриване. Пинато от overruns-sql.test.ts, който изпълних срещу реалния sqlite3 и мина.

A03 XSS — няма. Нула dangerouslySetInnerHTML, innerHTML, eval, new Function в новите маршрути/компоненти (grep потвърди). React екранира; SVG-графиките (ComboTrendChart, overruns scatter) рендерират само числови геометрии, не сурови стрингове.

CWE-349 (кеш дрейф) — чисто. CACHE_QUERY_PARAMS е разширен коректно (angle, step, cur, cpv, by, cohort, metric, cpvSort, a/b…); старият g е заменен от step. Дрейф-гардът consumed ⊆ CACHE_QUERY_PARAMS ∪ INTENTIONALLY_UNKEYED е запазен и cache-key.test.ts мина при мен. Потвърдих, че trends.tsx наистина чете step/cur/cpv и че всичките са в allow-list-а.

Достъп/секрети — n/a. Read-only, edge-кеширани GET loader-и над публични данни за обществени поръчки; няма мутации, няма process.env, няма външни fetch, няма localStorage/document.cookie. analytics.tsx loader-ът не приема потребителски вход. Няма нови зависимости в package.json/lock-файла (проверено) — нулев supply-chain риск. FullscreenButton/MetricInfo feature-детектват requestFullscreen и разкачват всичките си listener-и в useEffect cleanup — без ресурсни течове.

Цялост на данните

  • Всяко деление е защитено: sofiaShare (totalEur > 0), opaqueHeadline (filter(valueEur > 0)), PCT (WHERE изисква signing_value_eur >= 1000 + двоен JS guard срещу Infinity/NaN), clampGrowth (отхвърля non-finite и ≤0, band 0.5–2.0), overrunBarGeometry (колабира при непозитивна стойност). Непълни входове връщат em-dash, не измислена цифра.
  • „Конкуренция" изключва текущата непълна година — симетрично с изхвърлянето на частичния as_of период в trend.ts. Тримесечното сгъване (quarterOf/fillPeriods) запазва подредбата.
  • Вторичните ключове за подредба (…, authority_id, bidder_id / …, c.id) правят LIMIT-натите резултати детерминистични.
  • Миграция 0002_contracts_overrun_index.sql е адитивна и идемпотентна (CREATE INDEX IF NOT EXISTS, partial WHERE annex_count > 0), номерацията 0000/0001/0002 е последователна, покрита с регресионен тест.
  • CSS регресия — няма. Изтритите в layout.css .lens-* правила вече не се ползват никъде — analytics.tsx е пренаписан към новия „пет карти" дизайн (az-* класове), които са дефинирани в pages.css (потвърдено). За разлика от други PR-и в stack-а, тук няма orphan-нати класове.

Съответствие с описанието

Няма свързан тикет за съпоставка; описанието на PR-а служи като спецификация и имплементацията му отговаря (пет равностойни карти, 7 bounded statements на cold load). Описанието е приведено в съответствие след предходните прегледи (@nedda76, @lyubomir-bozhinov): „Конкуренция" е коректно обозначена като contracts-скан, опейк-текстът съответства на bids_received = 1, growthMultiple/sectorOptions са унифицирани, мъртвите полета и линкът към несъществуващия /compare — премахнати.

Локална проверка (резултати)

  • @sigma/db: 238/238 зелени (вкл. overruns, competition, trend, flows, regions, migrations, overruns-sql срещу реален sqlite3).
  • @sigma/web: 375/375 зелени (вкл. analytics-stats, filters, overruns-chart, overruns-inspector, cache-key).
  • pnpm turbo run typecheck7/7 успешни.

Незадължителни бележки (не-блокери)

  1. Header-коментарът на cache-key.ts е съкратен и вече не описва механиката на дрейф-гарда (кои източници сканира, кои са слепите му петна). Самият guard и INTENTIONALLY_UNKEYED са запазени, така че защитата е налична — само документацията олекна. Струва си да се върнат двата реда контекст.
  2. getCpvGroupMedians ползва целочислен ранг (cnt-1)*5/10 + 1, т.е. long-median при четен брой. За baseline „спрямо типичното" е приемливо и документирано; само отбелязвам, че не е интерполирана медиана.
  3. Обхватът е голям, защото това е върхът на 4-PR cross-fork stack — ревюто по същество е спрямо pr/overruns. Спазвайте реда на merge (dash-base → trends → overruns → analyze), за да не влезе непълен стек в main.

Кодът е чист, дисциплиниран по параметризация и guard-ове, и напълно покрит с тестове, които този път изпълних сам. Не откривам блокери по сигурност или цялост на данните, нито следа от зловреден код.

Вердикт: ОДОБРЯВАМ — няма блокери по сигурност или цялост на данните; тестовете, typecheck-ът и SQL-инжекционният анализ са зелени.

The popover often renders inside a thead th whose white-space: nowrap is
inherited by every line of the card, so long summaries and the mono
readout ran past the right edge. Reset wrapping on .metric-info-pop
(white-space: normal + overflow-wrap: anywhere), widen the card to
320px clamped to the viewport (min(320px, 100vw - 16px)), and add the
JS shift-into-viewport + coarse-pointer 44px hit area so all copies of
the component behave identically.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Обобщено ревю на PR: feat(web): analyze index — five equal analysis cards

Какво прави PR-ът

PR-ът преработва аналитичния индекс към пет равностойни аналитични карти и пренарежда презентационния слой. Обхваща:

  • Нова страница „Раздуване" (overruns.tsx) и реорганизация на analytics.tsx към петте карти.
  • Пренаписване на „Тренд във времето" в „Договори — обзор" (trends.tsx) с три ъгъла на анализ (време / CPV / кръстосано), изцяло SSR/no-JS чрез <Link>-мутации на query string.
  • Нови/променени презентационни React компоненти и SSR-safe геометрия/форматиране в lib/*.
  • Голям обем CSS (нови components.css, pages.css; изчистване на стар .lens-* markup в layout.css; tokens.css).
  • Слой за кеш-ключове, API типове, CPV конфигурация, адитивна миграция 0002 и нови SQL заявки в packages/db (overruns, competition, flows).

Обща оценка

ВЕРДИКТ: COMMENT — няма блокиращи проблеми по сигурност или интегритет в нито един от седемте batch-а. Одобрението изчаква няколко уточнения и ръчни проверки, изброени по-долу.

Сигурност (Phase 0) — чисто

  • Няма твърдо кодирани тайни, нови външни URL адреси, нови зависимости, обфускация или backdoor/eval шаблони.
  • Няма SQL инжекция: целият нов SQL използва параметризирани bind(...) placeholder-и (по един на id в IN (...)); вградените в текста изрази (by=absolute|percent, SECTOR_KEY_SQL) са фиксирани константи, не потребителски вход.
  • Няма XSS: React екранира по подразбиране, няма dangerouslySetInnerHTML.
  • Силна входна валидация: cpvGroupSelection (^\d{5}$, дедуп, таван MAX_CPV_GROUP_SELECTION), whitelisting на angle/step/sort, year (/^20\d\d$/) — адресира CWE-349 (замърсяване/колапс на кеш ключове), с drift-guard тестове в двете посоки.
  • Положителна CSP практика: статичните стилове са изнесени от inline style={…}, за да не се налага 'unsafe-inline'.

Интегритет на данните — силни страни

  • Защита срещу деление на нула на всички показатели, покрита с тестове.
  • Единна CPV нормализация (cpvDivision), пинната срещу SQL SECTOR_KEY_SQL в реален SQLite — предпазва от „truncate-before-normalize" разминаване между leaderboard и by-sector.
  • Изключване на текущата частична година, детерминистични tie-breaker-и за стабилна пагинация, адитивна и идемпотентна миграция 0002.

Находки за уточнение (неблокиращи, подредени по важност)

  1. Кросс-страничен риск за консистентност (среден, analytics.tsx). Картата „СРЕДЕН РЪСТ" ползва estimateYoyGrowth(trend.points) при granularity: 'month', но подсказката твърди „същата стойност като на страницата „Тренд" за последните 3 пълни години". Потвърдете, че месеците се агрегират до пълни години и дават идентичен резултат — иначе подсказката подвежда.
  2. Информационна архитектура (нисък–среден, overruns.tsx). Лийдът описва ред „списък → облак → таблица", но JSX рендерира AuthoritySection/SectorSection преди OverrunsDashboard. Уточнете дали редът е умишлен, или изравнете текста/секциите.
  3. Осиротели .lens-* класове (изисква кръстосана проверка). layout.css премахва .tiles.analytics-lenses и .lens-* (−122 реда). Потвърдете, че старият lens markup (lens-card, lens-preview, lens-list, lens-chart, lens-map и т.н.) е премахнат/заменен навсякъде, иначе индексът ще остане без стил.
  4. Различни форматери/източници за един показател. Лендингът ползва growthMultiple(overruns.medianPct), детайлът — formatGrowthFactor(corpus.medianPct); потвърдете идентичен изход. Аналогично тоталът „договора" в trends.tsx се смята от CPV-филтриран, но не year-филтриран набор — проверете дали е желаното поведение.
  5. Непоследователна обработка на празни низове (overruns.tsx). authorityEik || '—' срещу bidderEik ?? 'непотвърден' — при празен низ за изпълнителя се получава висящ разделител. Уеднаквете (напр. bidderEik || 'непотвърден').
  6. Излишни/невалидни заявки при празен вход (trends.tsx). getCpvGroupMedians(db, missing) се извиква и при празен missing — пропуснете заявката при missing.length === 0.
  7. CSS дублиране и поддръжка. .fs-btn и .trend-fs-btn са почти идентични; шрифтовият стек 'IBM Plex Mono', var(--font-mono) се повтаря ~15 пъти; backdrop-filter е без -webkit- префикс. Дребно.
  8. Достъпност. В AnalyzeCard/ov-mast-kpis <dd> предхожда <dt> — екранните четци четат стойността преди етикета; обмислете размяна на реда с CSS. (Положително: фокус-стилове, role="img"/aria-label, role="status" sr-only обявявания.)

Отворени въпроси преди APPROVE

  • Покритие с тестове. Чистите функции и новите SQL заявки са много добре покрити (смислени тестове). Но overruns.tsx, trends.tsx и React компонентите нямат render/интеракционни тестове (репо-конвенция) — покритието ≥90% за новите route файлове трябва да се потвърди преди одобрение.
  • pages.css (+3080/−3) не беше наличен като diff за ред-по-ред преглед. Необходима е ръчна проверка на пълния diff за дублиране на селектори, мъртви правила и съответствие с „петте еднакви карти", плюс потвърждение за затягането на CSP в свързания код (lib/security.ts).

Заключение

Няма доказани блокиращи дефекти. PR-ът е добре структуриран, с добра сигурност и интегритет на данните. За финално одобрение остават: потвърждение по находки 1–4, изчистване на осиротелите .lens-* класове, проверка на тестовото покритие и ръчен преглед на непоказания pages.css.

Comment thread apps/web/app/components/MetricInfo.tsx
Comment thread apps/web/app/lib/analytics-stats.ts Outdated
Comment thread apps/web/app/routes/overruns.tsx
Comment thread apps/web/app/routes/trends.tsx Outdated
Comment thread apps/web/app/styles/components.css Outdated
Comment thread apps/web/app/styles/components.css
Comment thread apps/web/app/styles/components.css
Comment thread apps/web/app/styles/tokens.css Outdated
Comment thread packages/db/migrations/0003_contracts_overrun_index.sql
Comment thread packages/db/src/overruns-sql.test.ts Outdated
- MetricInfo: recompute the popover shift on resize/scroll while open, not just on open toggle
- analytics-stats: clamp firstShare/latestShare to [0,1]
- overruns: align bidderEik empty-string handling with authorityEik (||, not ??)
- trends: guard the getCpvGroupMedians call when there are no missing groups
- components.css: consolidate .fs-btn/.trend-fs-btn into a shared base + modifiers, add
  -webkit-backdrop-filter for the trend fullscreen backdrop, restore pointer-events on the
  visible metric-info popover
- tokens: convert --paper-raised to oklch(100% 0 0) to match the file's color convention
- overruns-sql.test: replace all tab occurrences (/\t/g) in the sqlLiteral test helper

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ревю на PR: feat(web): analyze index — five equal analysis cards

Какво прави PR-ът

Редизайн на аналитичния индекс към пет равностойни аналитични карти и нов маршрут /overruns. Промените обхващат: нови презентационни/графични компоненти (ComboTrendChart, FullscreenButton, MetricInfo, промени в TrendChart); чисти помощни функции за аналитиката (analytics-stats, overruns-chart, overruns-inspector); route loader-и (analytics.tsx, overruns.tsx, trends.tsx); навигация и парсване на филтри (filters.ts, analytics-lenses, routes.ts); DB/кеш слой (cache-key.ts, SQL хелпъри) и придружаващи тестове; и CSS (components.css, layout.css). Разделянето на отговорностите е чисто — логиката е изнесена в тествани хелпъри, компонентите остават презентационни.

Сигурност (Phase 0) — ЧИСТО

Във всички партиди: няма твърдо кодирани тайни, няма нови/променени външни URL адреси, няма нови зависимости, няма обфускация/backdoor. Всички връзки са вътрешни маршрути. SQL заявките са параметризирани (.bind()) или статични фрагменти; единственото стрингово сглобяване на SQL е в тест с коректно екраниране. React екранира изхода → няма XSS повърхност.

Особено силна е защитната валидация на входа:

  • cpvGroupSelection в filters.ts — строг /^\d{5}$/, дедупликация и твърд лимит (MAX_CPV_GROUP_SELECTION) преди стойностите да достигнат SQL обхват или edge-кеш ключ (коректно адресира CWE-349; тестван с враждебни входове).
  • trends.tsx — allow-list за angle/step/sort/cpvSort, regex за year, точна стойност за cur, ограничени списъци към DB.
  • cache-key.ts — всички response-affecting параметри добавени към allow-list, усилени drift-guard тестове.

Качество и цялост на данните

Чистите помощници са честни при оскъдни данни (връщат „—", никога не измислят стойност), клампват дялове и обработват празни серии без NaN/деление на нула. estimateYoyGrowth изключва непълни години; getOpaqueShareByYear изключва текущата непълна година; overruns праговете (€1000 floor) предпазват от деление на нула. Достъпността е на много добро ниво (role="img", aria-label, aria-pressed/current, :focus-visible, ≥44px touch targets). Тестовото покритие на изнесените хелпъри и SQL слоя е смислено и обхваща гранични случаи.

Точки за потвърждение преди merge (не-блокиращи, но зависимости между партидите)

  1. Гранулярност в analytics.tsx: извикването вече е granularity: 'month', но коментарът твърди „3-годишна плъзгаща медиана за пълни години" — потвърдете, че estimateYoyGrowth вътрешно агрегира месеците до пълни години, иначе средният ръст ще е сгрешен.
  2. TrendGranularity + 'quarter': разширяването на типа е потенциално чупещо за switch без default при консуматорите — потвърдете, че рендер логиката обработва 'quarter'.
  3. Недефиниран CSS токен --paper-raised: използван за фон на филтър-чиповете и полето за търсене, но не е дефиниран в tokens.css — потвърдете, че PR-ът го добавя, иначе контролите губят повдигнатата повърхност.
  4. Премахнати .analytics-lenses / .lens-* стилове: коректно само ако съответният markup се пренаписва в същия PR — потвърдете координацията, иначе /analytics ще се рендира без стилове.
  5. IN заявка за анекси (overruns.tsx): уверете се, че лидербордът е ограничен, така че броят contractId да не надхвърля лимита за bound параметри на Cloudflare D1.

Дребни забележки (незадължителни)

  • Етикетът „(показани първите 24)" в trends.tsx се задейства при точно 24 резултата дори без реално отрязване — обмислете hasMore флаг.
  • Уточнете дали избраната година умишлено не филтрира графиката/картите (подава се само на долния списък).
  • Хардкоднат цвят rgb(94 124 139 / 0.55) и непоследователни font fallback-ове в CSS — обмислете токени за консистентност.
  • Несортиран ред на cpv в външни линкове фрагментира кеша (безопасно, документирано, но неефективно).

Заключение

COMMENT — кодът е с високо качество, без блокери по сигурност или цялост на данните в нито една партида. Одобрението зависи от потвърждаване на петте точки по-горе, тъй като са междупартидни зависимости, които не могат да бъдат проверени изолирано.

Comment thread apps/web/app/components/MetricInfo.tsx
Comment thread apps/web/app/components/ComboTrendChart.tsx
Comment thread apps/web/app/routes/analytics.tsx
Comment thread apps/web/app/routes/overruns.tsx
Comment thread packages/api-contract/src/index.ts
Comment thread packages/db/migrations/0003_contracts_overrun_index.sql
Comment thread packages/db/src/queries/competition.ts
# Conflicts:
#	apps/web/app/lib/filters.test.ts
…ards)

- MetricInfo: sync aria-expanded with the actual CSS hover/focus-within
  reveal, not just the click-toggled open state
- ComboTrendChart: drop dead n<=1 branch in the x() ternary (guard above
  already ensures n>=2)
- analytics-stats: add a test proving estimateYoyGrowth correctly folds
  monthly TrendPoints into full calendar years before computing ratios
- overruns: chunk getOverrunAnnexes' IN-list at D1's 100-bound-parameter
  cap so a full 200-row leaderboard page can't exceed it
- 0002_contracts_overrun_index.sql: document the partial-index tradeoff
  (annex_count only, not the full OVERRUN_WHERE predicate)
…l SQLite

Adds a real-SQLite fixture covering topRecurringPairs' authority_id/bidder_id
tie-breakers (ydimitrof review, PR midt-bg#172), proving the ORDER BY columns
resolve against the query's own SELECT aliases and that ties sort
deterministically rather than falling back to insertion order. The
TrendGranularity widening thread required no code change: every
switch/if-else consumer already handles 'quarter' explicitly.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ревю на PR — обобщение: „feat(web): analyze index — five equal analysis cards"

PR-ът преработва аналитичния индекс на уеб приложението: заменя старите „lens" карти с пет равни аналитични карти, добавя нова страница /overruns („Раздуване"), обновява маршрутите analytics.tsx, overruns.tsx и trends.tsx, изнася оформлението от inline style= в CSS (за да остане маршрутът CSP-чист), добавя нови SQL заявки за overruns/региони/тенденции, миграция 0002 (частичен индекс WHERE annex_count > 0) и разширява allow-list-а на кеш-ключовете с нови параметри (angle, by, cpv, cpvSort, cur, step). Логиката за форматиране и статистика е изнесена в тестваеми чисти функции.

Обща оценка: COMMENT — няма блокиращи проблеми

Прегледът е извършен на 7 партиди. Не са открити проблеми със сигурността или целостта на данните.

Сигурност (Фаза 0) — ЧИСТО

  • Без SQL-инжекции: всички заявки използват параметризирано свързване (.bind(...), ? плейсхолдъри); интерполираните SQL низове са изградени изцяло от модулни константи, без потребителски вход. Входовете (by, angle, sort, year, cpv) минават през allow-list/regex валидация преди слоя за данни.
  • Без XSS: всички стойности се рендерират през JSX с авто-екраниране; няма dangerouslySetInnerHTML/eval.
  • Без твърдо кодирани тайни, нови външни URL адреси, нови зависимости или обфускация. PEG = 1.95583 е легитимният фиксиран курс BGN/EUR.
  • Кеш дисциплина: allow-list на кеш-ключовете с drift-guard предотвратява колабиране на различни отговори (CWE-349). cpvGroupSelection е строго валидиран (^\d{5}$, дедупликация, таван 10).

Тестове

Силно и смислено покритие в партидите с тестове — гранични случаи (NULL, нулев знаменател, отрицателни делти, chunking на IN-списъци, детерминистично tie-breaking с реален SQLite, malformed CPV кодове). Тестовете търсят дефекти, а не минават тривиално.

Точки за потвърждаване (не блокиращи)

  1. Семантика на растежа (партида 1–2): estimateYoyGrowth връща множител (1.2 = +20%), докато formatYearlyGrowth очаква разлика-съотношение (0.18 → "+18%/год"). Потвърдете, че извикващият код подава value − 1, иначе карта „СРЕДЕН РЪСТ" ще показва грешна стойност.
  2. Гранулярност на трендовете (партида 1–2): estimateYoyGrowth работи само с месечни серии (months === 12); при 'quarter'/'year' тихо връща плоско число. Потвърдете агрегацията на месечните точки към години.
  3. authorityEik (партида 7): ЕИК на възложителя преизползва authoritySlug(authority_id). Потвърдете, че връща суровия ЕИК, а не форматиран слъг.
  4. Крос-партидна проверка (партида 4): уверете се, че нито един TSX/TS вече не реферира премахнатите .lens-* / analytics-lenses класове.
  5. Резервиран параметър g (партида 6): потвърдете, че /trends вече не чете стария параметър g (иначе тест „keeps RESERVED_CACHE_PARAMS honest" ще падне).

Незадължителни козметични бележки

  • Подредба <dt>/<dd> в <dl> (<dd> преди <dt>) е неконформна по HTML спецификацията.
  • Непоследователен десетичен разделител (1.5М с точка срещу запетая по българската конвенция) в aria-hidden SVG етикети.
  • Дублиране на шрифтовия стек 'IBM Plex Mono', var(--font-mono) (~15 пъти) в components.css — препоръчва се токен.
  • Твърдението за „ограничено пространство на кеш-ключове" в trends.tsx е неточно — edge кешът ключи по суров URL; реалната защита изисква нормализация на cache key или каноничен redirect.
  • SVG scatter точките (<circle onClick>) не са клавиатурно фокусируеми (смекчено от паралелен списък с бутони).
  • Дребна неточност в етикета „(показани първите 24)" при точно 24 записа.
  • overruns.tsx: анексите за целия leaderboard се извличат нетърпеливо в loader payload-а — приемливо, но може да порасне при по-голям набор.

Обхват, изискващ ръчна проверка

  • pages.css (+3080/−3, партида 5): diff не беше наличен за преглед. Обем от +3080 реда е необичайно голям за чисто презентационна промяна — препоръчва се ръчна проверка за дублирани/мъртви правила и за евентуални външни url()/@import към неодобрени домейни.

Заключение

Кодът е с високо качество, добре структуриран и документиран, SSR/no-JS съвместим, с осезаема грижа за валидация на входа и целостта на данните. Няма блокиращи проблеми. Пълно одобрение (APPROVE) изисква потвърждение на изброените крос-партидни точки, ръчен преглед на pages.css и потвърждение на CI/покритието.

Comment thread apps/web/app/lib/analytics-stats.ts
Comment thread apps/web/app/lib/analytics-stats.ts
Comment thread apps/web/app/lib/filters.ts
Comment thread apps/web/app/routes/overruns.tsx
Comment thread apps/web/app/routes/trends.tsx Outdated
Comment thread apps/web/app/routes/trends.tsx Outdated
Comment thread apps/web/app/styles/components.css Outdated
Comment thread apps/web/app/styles/components.css Outdated
Comment thread apps/web/workers/cache-key.ts Outdated
Comment thread packages/db/src/queries/competition.ts
nikimilenkov added a commit to nikimilenkov/sigma that referenced this pull request Jul 16, 2026
… review)

The 14 subject-risk columns were added by editing 0000_init.sql. The served D1
is persistent and `wrangler d1 migrations apply` is filename-tracked, so an
edit to the already-applied 0000_init reaches the work DB (green local CI) but
never prod — precompute's UPDATE would then fail on missing columns (the
midt-bg#188/midt-bg#239 trap the reviewer reproduced).

Extract the columns into 0006_subject_risk_columns.sql (ALTER TABLE ADD
COLUMN) and remove them from 0000_init (SQLite has no ADD COLUMN IF NOT EXISTS,
so a fresh DB can't carry both). 0006 clears the 0002 claimants
(midt-bg#226/midt-bg#193/midt-bg#172) to avoid a duplicate-version at apply. The precompute
CREATE TABLE IF NOT EXISTS mirror stays a no-op on the existing tables. The
four risk tests now apply 0006 after 0000_init, mirroring the served-D1 chain.

Reword the 0000_init header to distinguish the rebuilt-every-import work DB
from the persistent served D1, so the next person adds objects via a new
migration instead of editing 0000_init. Also document that value_flag='ok' is
the exact complement of the hidden suspect set.
nikimilenkov added a commit to nikimilenkov/sigma that referenced this pull request Jul 17, 2026
… review)

The 14 subject-risk columns were added by editing 0000_init.sql. The served D1
is persistent and `wrangler d1 migrations apply` is filename-tracked, so an
edit to the already-applied 0000_init reaches the work DB (green local CI) but
never prod — precompute's UPDATE would then fail on missing columns (the
midt-bg#188/midt-bg#239 trap the reviewer reproduced).

Extract the columns into 0006_subject_risk_columns.sql (ALTER TABLE ADD
COLUMN) and remove them from 0000_init (SQLite has no ADD COLUMN IF NOT EXISTS,
so a fresh DB can't carry both). 0006 clears the 0002 claimants
(midt-bg#226/midt-bg#193/midt-bg#172) to avoid a duplicate-version at apply. The precompute
CREATE TABLE IF NOT EXISTS mirror stays a no-op on the existing tables. The
four risk tests now apply 0006 after 0000_init, mirroring the served-D1 chain.

Reword the 0000_init header to distinguish the rebuilt-every-import work DB
from the persistent served D1, so the next person adds objects via a new
migration instead of editing 0000_init. Also document that value_flag='ok' is
the exact complement of the hidden suspect set.
…al threads)

Guard estimateYoyGrowth against non-monthly TrendPoint series instead of
silently returning a flat 1.0 factor; reword two /trends comments that
overstated the CPV/cur param validation as a cache-key-cardinality control
(the edge cache keys on the raw request URL, not validated content); format
axisLabel's millions label with the Bulgarian decimal comma multText already
uses; introduce --font-mono-plex to de-duplicate the repeated IBM Plex Mono
font stack across ~17 CSS rules; unify .trend-fs-btn:focus-visible with the
shared .fs-btn:focus-visible ring; canonicalize repeated ?cpv values when
building the edge cache key so equal CPV sets in any order share one entry
(behavior change — flips the cache-key test that previously asserted
different order produces a different key); and document the UTC-vs-local
caveat on competition.ts's current-year cutoff.
Resolve conflicts from today's round-3 review-fix commits landing on main:
- apps/web/app/lib/query-params.ts: keep main's shared CANONICAL_QUERY_PARAMS
  source of truth, add this PR's params (angle, by, cpv, cpvSort, cur, step)
  used by the redesigned /trends and /overruns routes, and restore the
  RESERVED_CACHE_PARAMS reservation for 'g' (midt-bg#144, still open).
- apps/web/workers/cache-key.ts: adopt main's shared-import structure, keep
  this PR's cpv multi-select grouping/sort-canonicalization logic.
- apps/web/workers/cache-key.test.ts: merge both branches' drift-guard tests;
  restore whitespace-tolerant regex so multi-line .getAll() chains are scanned.
- apps/web/app/styles/components.css: concatenate additive, non-overlapping
  blocks from both sides (fullscreen/metric-info/list-search vs EU-benchmark).
- packages/config/src/index.test.ts, packages/db/src/competition-sql.test.ts:
  merge additive imports/fixtures/tests from both sides.

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прегледах #172 (индексът „Анализи" + петте карти) на дълбочината на #171 — сметките и свързването държат. Какво проверих, че е коректно:

  • YoY на „Тренд" картата: estimateYoyGrowth връща множител (1,18), а свързването конвертира към ставка (growth.value − 1) преди formatYearlyGrowth → „+18%/год", а не „+118%". Класическият капан е избегнат. Прозорецът е 3 последни ПЪЛНИ години (12 месеца, не partial, >0), клампнат [0,5; 2], с loud assert за не-месечна серия.
  • „×" множителят: growthMultiple → formatGrowthFactor = 1 + pct (медиана +210% → „3,1×"), споделен с /overruns → еднакъв низ на двете места.
  • „Непрозрачност" картата: getOpaqueShareByYear брои single-offer по bids_received = 1 със знаменател „договори с известен брой оферти" — умишлено същата основа като competitionTotals.singleOfferValueShare (не се разминава с /competition; „admitted = bids_received − bids_rejected" е отделната риск-логика, не тази метрика). opaqueHeadline клампва дела [0,1] и пази знаменателя (>0).
  • Flows/Region картите: честни броячи/дялове (sofiaShare с пазач за деление, неатрибутираните региони извън знаменателя).
  • Loader без параметри → няма cache-key/инжекция изненади и няма имена на субекти на индекса (PII не стои тук). Колоните съществуват в схемата.

Две бележки (не блокират):

  1. Зависимост от #257 (евро-анекси): картите четат amount_eur, който е наполовина за договорите в лева с евро-анекс от 2026. Тук ефектът е второстепенен — това са отношения, числителят и знаменателят се скалират заедно — за разлика от /overruns, където е разрушителен. Просто за координация.
  2. Микро-нюанс: getOpaqueShareByYear не слага филтъра amount_eur > 0, който competitionTotals слага. На практика безобидно (нулите са no-op, отрицателни стойности няма), но коментарът „same basis" леко надценява — или добави > 0 за буквална идентичност, или смекчи формулировката.

Одобрявам.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Обобщен преглед на PR: feat(web): analyze index — five equal analysis cards

Какво прави PR-ът

Преработва аналитичния index към пет равнопоставени карти за анализ, като въвежда нови графики и компоненти (ComboTrendChart, FullscreenButton, MetricInfo), нов маршрут /overruns, преработен изглед /trends (три „ъгъла" — време / CPV / кръстосано, с общ списък договори), чисти помощни функции (analytics-stats, overruns-chart, overruns-inspector), нови SQL заявки (overruns.ts, regions.ts, trend.ts, flows.ts, competition.ts), една SQL миграция, дизайн-токени, обновени филтри/кеш параметри и обширни промени по стиловете (components.css, layout.css, pages.css). Старите класове .analytics-lenses / .lens-card са премахнати.

Обща оценка на качеството

Кодът е с високо качество: помощните функции са чисти и добре типизирани, компонентите са SSR-безопасни (feature-detection за Fullscreen, изчистване на слушателите, без течове), достъпността е изпипана (role="status", aria-*, :focus-visible, ≥44px touch зони, no-JS съвместимост). Тестовете са подробни и смислени — целят да разкриват дефекти, а не да минават тривиално.

Сигурност — ЧИСТО

Няма зашити тайни, нови рискови зависимости, външни URL-и, обфускация или backdoor шаблони. Ключовата защита срещу инжекция и cache poisoning (CWE-349) е коректна и потвърдена в целия PR:

  • cpvGroupSelection / validCpvGroups валидират строго ^\d{5}$, дедуплицират и ограничават бройката (MAX_CPV_GROUP_SELECTION = 10) преди стойностите да достигнат заявка или кеш-ключ.
  • Целият SQL е статични литерали + свързани ? параметри; входовете минават през whitelist (pick), regex или clampLimit. Няма конкатенация на потребителски вход. Тестове изрично проверяват, че злонамерени CPV ("45'--") не пораждат join/диапазон.
  • Канонизацията на cpv кеш-ключа е стабилна (URLSearchParams.sort()), едно логическо множество → един ключ.
  • D1 лимитът на параметри се спазва (chunking по 100).

Първоначалната зависимост между партидите (валидация в filters.ts + параметризиран SQL в @sigma/db) е потвърдена в по-късните партиди.

Findings за потвърждение преди сливане (не блокиращи)

  1. authorityEik в mapOverrunRows (overruns.ts) — пълни се с authoritySlug(r.authority_id), докато bidderEik ползва суровата колона. Ако authoritySlug() slug-ифицира вместо да запази цифрите, инспекторът ще показва slug вместо ЕИК. Да се потвърди, че authority_id е самият ЕИК; ако не — да се селектира реалната ЕИК колона (несиметрия спрямо bidderEik).
  2. pages.css (+3080/-3) не е прегледан по същество — diff липсва („no patch available"). Phase 0 не може да се затвори като CLEAN за този файл. Нужна е ръчна проверка за външни url()/@import/data: препратки и за дублиране/мъртви селектори преди сливане.
  3. Премахнати .analytics-lenses / .lens-card — да се потвърди, че тези класове вече не се реферират в никой .tsx/шаблон извън PR-а (rg "lens-card|analytics-lenses|lens-preview|lens-metrics|lens-link").
  4. getCpvGroupStats.totalGroups брои DISTINCT CPV групи без филтър за стойност, докато топ-групите изискват amount_eur > 0 — семантично приемливо, но да се потвърди намерението.

Дребни бележки (нисък приоритет)

  • Магическото число 24 в trends.tsx се дублира; индикаторът „(показани първите 24)" е неточен при точно 24 договора — препоръка за именувана константа и заявка на limit + 1.
  • year се валидира само с /^20\d\d$/ (пропуска бъдещи години като 2099) → празен списък вместо явно „непознат филтър" състояние.
  • estimateYoyGrowth хвърля грешка при не-месечни серии — уверете се, че loader винаги подава месечна серия (иначе 500).
  • CSS: зашит цвят rgb(94 124 139 / 0.55) в .trend-sw-box вместо токен; непоследователен fallback за --font-serif; потвърдете, че всички използвани токени (--font-mono-plex, --paper-raised, --rule-soft, --ink-soft) са дефинирани.

Блокиращи концерни

Няма findings по сигурност или цялост на данните, които да блокират. Единственият формален блокер е невъзможността pages.css да бъде прегледан по същество (finding #2) — изисква ръчна проверка. Финалната оценка на ниво целокупен PR (покритие ≥90%, зелен CI) следва да се потвърди отделно.

Общ вердикт: COMMENT — качествен, добре тестван и сигурен PR; сливане след ръчна проверка на pages.css и потвърждаване на findings #1, #3 и #4.

Comment thread apps/web/app/lib/filters.ts
Comment thread apps/web/app/lib/analytics-stats.ts
Comment thread apps/web/app/components/ComboTrendChart.tsx
Comment thread apps/web/app/styles/components.css Outdated
Comment thread apps/web/app/styles/components.css
Comment thread apps/web/workers/cache-key.ts
Comment thread packages/db/src/queries/competition.ts
- hide the /trends combo-chart hover tooltip from AT (aria-hidden, drop the
  now-contradictory role=status) since it re-renders on every hover and the
  same data already lives in the accessible year cards
- consolidate the hardcoded slate swatch/bar-fill colors onto the existing
  --slate token via color-mix, so the legend swatch and count bars can't
  drift apart on theme change
- unify the font-serif fallback stack across all three trend title rules
- document that cache-key.ts's cpv-only param sort is intentional (cache
  fragmentation only, never poisoning) rather than a gap
- colocate the year lower/upper bound checks in getOpaqueShareByYear for
  readability; no behavior change

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Обобщено ревю на PR: feat(web): analyze index — five equal analysis cards

Какво прави PR-ът

Редизайнва аналитичния индекс към „пет равностойни аналитични карти", добавя нов изглед „Раздуване/Overruns" (/overruns) и обновена страница за тенденции (/trends). Промяната обхваща целия стек: чисти помощни функции и презентационни React компоненти с unit тестове, route loader-и, SQL заявки в @sigma/db (overruns/regions/trend/competition/flows), нова миграция с частичен индекс, канонизиране на кеш-ключове, разширение на API контракта (TrendGranularity с quarter) и значителен обем нови CSS стилове. Старите „lens" карти и свързаните компоненти/стилове (Choropleth, ANALYTICS_LENSES, .analytics-lenses) са премахнати.

Сигурност — ЧИСТО

Във всичките 7 партиди не са открити твърдо кодирани тайни, нови външни URL адреси, нови зависимости, обфускация или бекдори. Валидацията на входа е силна страна на PR-а: cpvGroupSelection ограничава CPV входа (само 5-цифрени кодове, dedup, лимит 10), route параметрите минават през whitelist/regex, а всички SQL заявки използват ? placeholder-и и .bind(...) — интерполираните части са изцяло вътрешни константи. Кеш-ключовете са канонизирани, което адресира CWE-349 (cache confusion). Няма нарушения по OWASP A01–A10.

Качество и коректност

Кодът е с високо качество: чисти, добре тествани помощни функции, внимателна обработка на NULL/гранични случаи (деление на нула, липсващ fx, празни списъци), детерминистична подредба чрез tie-breakers и стабилен SSR изход. Достъпността е последователно добра (aria-*, role="img"/"status", sr-only, :focus-visible, ≥44px тъч зони). Тестовете са смислени и нетривиални (реален SQLite, chunking на IN-списъци, drift-guard, partition тестове).

Незадължителни забележки (небло­киращи)

  1. topGrowthCode (overruns.tsx): акцентът „най-голям растеж" се избира по s.code; ако водещият сектор има празен CPV код, всички редове с празен код ще получат класа is-top. Препоръка: избор по уникален идентификатор/индекс.
  2. Лимит на CPV групи (trend.ts): validCpvGroups валидира формата, но не и броя групи; при ~50 групи bound-параметрите могат да надхвърлят D1 капа от 100. Симетричен горен предел би бил защита в дълбочина.
  3. Възможно подвеждащо броене (trends.tsx): броячът на договори се изчислява от trend.points (фасетиран само по CPV), затова филтър по year не го стеснява — да се уеднакви или поясни.
  4. ComboTrendChart: x-axis етикетите нямат хоризонтално позициониране като в TrendChart — да се потвърди подравняване спрямо стълбовете.

Точки за потвърждение преди merge

  • --font-mono-plex (CSS): използва се многократно без inline fallback. Партида 6 потвърждава, че токенът е добавен в tokens.css (plex mono) — да се провери, че дефиницията реално покрива всички употреби, иначе font: шорткътовете стават невалидни и типографията тихо се губи.
  • Обем на CSS (pages.css, +3080): реалният diff не беше достъпен за ред-по-ред проверка в партида 5. Да се провери ръчно за дублиране/мъртъв код и че „петте карти" ползват споделен клас, а не почти идентични селектори.
  • Мъртъв код: да се потвърди, че след премахването на lens компонентите не остават неизползвани вносове или референции (lens-card, sector/funding/g параметри, външни връзки/отметки).
  • granularity: 'quarter' в контракта да има реален производител в trend.ts (кръстосана зависимост между партидите).
  • Миграция 0002: само напред (без rollback) — съответства на модела в repo-то; при бъдещи breaking промени добавете обратим път.

Заключение

Няма блокиращи проблеми със сигурността или целостта на данните в нито една партида. Кодът е чист, добре структуриран, тестван и с внимателна достъпност. Останалите точки са дребни забележки по качеството и кръстосани зависимости за потвърждение. Общ вердикт: COMMENT — препоръчва се сливане след изчистване на точките за потвърждение (най-вече --font-mono-plex и ръчна проверка на големия CSS diff за дублиране).

Comment thread apps/web/app/components/ComboTrendChart.tsx
Comment thread apps/web/app/lib/filters.ts
Comment thread apps/web/app/lib/query-params.ts Outdated
Comment thread apps/web/app/styles/components.css
Comment thread apps/web/workers/cache-key.ts
Comment thread packages/db/src/queries/trend.ts

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ре-верифицирах на HEAD (9a8a6f33): двата нови комита са ydimitrof review-round (5 нишки) + prettier — не пипат analytics-stats.ts нито петте заявки, които проверих. Верификациите ми стоят (multiplier→rate конверсията избягва +118% капана; single-offer базата = competitionTotals). Одобрявам.

- position .combo-xlab year labels at x(t.i) like TrendChart's SVG
  text, instead of justify-content: space-between, so they stay
  under their tick even when years aren't evenly spaced
- clarify PARAM_ORDER only covers /contracts; the /trends and
  /overruns params are still kept by withParams (CANONICAL_QUERY_PARAMS
  gate), just sorted after this list rather than at a curated position
- correct the cpv param comment: /contracts does not read `cpv` yet,
  only /trends does (validated 5-digit by cpvGroupSelection)
…endChart

Both components duplicated identical tick-computation and periodLabel
logic after ComboTrendChart's x(t.i) label-positioning fix landed
separately from TrendChart. Consolidate into apps/web/app/lib/trendAxis.ts
so the two implementations can't drift, and add a regression test
asserting ComboTrendChart labels track tick x-position (not flow
spacing) for unevenly-distributed years.
…, suppress RSC-only CSRF

Bump postcss to ^8.5.18 (GHSA-r28c-9q8g-f849) and valibot to ^1.4.2
(GHSA-5qjj-4xww-7phc) via pnpm overrides - both patch-level, non-breaking
fixes. Bump react-router/@react-router/dev to ^7.18.0 via override, fixing
4 real advisories (SSR hydration constructor injection, unauthenticated DoS,
RSCErrorHandler XSS, open-redirect via backslash) - all fixed within the 7.x
line, no major bump needed. Add a time-boxed osv-scanner.toml suppression for
the one remaining advisory, GHSA-qwww-vcr4-c8h2, a CSRF flaw scoped to
unstable RSC APIs this app does not use (verified via repo-wide grep) with
no fix in the 7.x line; bumping to 8.x is out of scope for this patch.
- osv-scanner.toml: keep both independent CVE suppressions (react-router
  RSC-only CSRF from this branch, sharp/miniflare from main)
- apps/web/app/routes/analytics.tsx, trends.tsx: keep this branch's
  rewritten loaders, route D1 access through main's getDb() chokepoint
  (midt-bg#199/midt-bg#225) instead of raw context.cloudflare.env.DB
- apps/web/app/routes/overruns.tsx: same getDb() chokepoint fix, applied
  here too since this file predates midt-bg#199/midt-bg#225 and never conflicted but
  still read env.DB directly (caught by readonly-db-chokepoint.test.ts)
- packages/db/migrations: renumber this branch's 0002_contracts_overrun_index
  to 0003 to resolve the numbering collision with main's independently
  added 0002_current_value_currency; migrations.test.ts now exercises both
- pnpm-lock.yaml: regenerated via pnpm install
The lockfile in 9b3e26f was regenerated with npx pnpm@12.0.0-alpha.21 after
corepack installed a broken placeholder shim, prepending a spurious
pnpm-12-alpha bootstrap YAML document (@pnpm/exe / packageManagerDependencies)
ahead of the real lockfile. Regenerated with the repo-pinned pnpm 10.33.0.
todorkolev added a commit that referenced this pull request Jul 29, 2026
… + 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>
@nedda76

nedda76 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Този клон е в конфликт с main, тъй че към момента не може да се ревюира — дифът, който GitHub показва, вече не отговаря на това, което би влязло. Ще го пребазираш ли върху актуалния main (или merge на main в клона) и да разрешиш конфликтите? След това веднага го поглеждам. Благодаря! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants