feat(web): договори — обзор (лещи време/CPV/кръстосано) - #170
feat(web): договори — обзор (лещи време/CPV/кръстосано)#170StanislavBG wants to merge 28 commits into
Conversation
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Пуснах и този локално и го изпробвах от всеки ъгъл; тук почти всичко е добре. Проверих и потвърдих: прогнозата клампва растежа в [0.5, 2] и пада на 1 при non-finite/≤0 (clampGrowth), брои само пълни години и пропуска partial-ата, стартира след последния пълен месец (partial-ът не се рисува като реален спад) и се самоизключва при липсваща сезонна база (без фалшив „cliff"); YoY на годишната таблица е guard-нат (partial || !prev || prev<=0 → null); fillPeriods нулира дупките, та вътрешните години винаги имат 12 месеца (което пази consecutive-year допускането на growth estimate-а); RSS-ът escape-ва всичките 5 XML entity-та и маха забранените C0 байтове (няма injection през имена/subject); chart геометрията е floor-ната навсякъде (Math.max(1, …), niceCeil(≤0)→1, n>1 guard), та degenerate domain (една точка, само нули) не чупи SVG-то; а cache-параметрите на /trends са само sector/funding (и двата keyed) — granularity-то е client state, без cache impact. Силна, дисциплинирана работа. 👏
Две неща, нито едно блокиращо (детайли инлайн): quarter-0 ръбът в QUARTER_EXPR, и value-basis-ът, който сумира отрицателни (коректно огледало на rollup-ите — същият tracked global issue като HHI в #153).
Координация: това е PR 2/4 и носи cache-key промяната от #169, която има отделен блокер (изтрит CWE-349 drift guard). Не може да влезе преди #169 да се оправи; merge-редът (dash-base → trends) така или иначе го налага.
Approve по същество — изчистете #169 блокера + зависимостта, и е готов.
|
Благодаря (force-push • quarter-0 ръбът: добавих guard seasonality заявката да изключва само-година дати. Малка корекция към бележката — разделителят е на позиция 5 ( |
StanislavBG
changed the title
|
Ребейзнах върху върха на #169 (fdc149a), за да сподели целият стек една база — обзорът е непроменен по съдържание. Единствена интеграционна корекция: 4 CSS класа се сблъскваха с дефинициите от dash-base (ov-panel, ov-panel-title, ov-legend, ov-seg) и на страницата на обзора са преименувани на ovz-*. Нов head: bc4cbde; typecheck 7/7, тестовете зелени. |
|
Дребно почистване: преработката носеше +70 реда локални бележки в |
|
Добавих още едно подобрение в кръстосаната леща ( Нови върхове на стека след rebase: |
|
Локален преглед на обзора (@
Дребно (не блокер): YoY взима Не одитирах SVG рендера на Координация с #171/#188: и трите въвеждат |
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).
|
Analysis complete. The code layer is clean — fully parameterized SQL, validated/bounded inputs, React auto-escaping throughout, no secrets. Writing the verdict. Преглед на PR #170 — „Договори — обзор" (лещи време/CPV/кръстосано)Здравейте, @StanislavBG 👋 Направих задълбочен преглед — не само на diff-а, а и на data слоя, SQL заявките и локална проверка за инжекции и злонамерен код. Прочетох и цялата дискусия с @lyubomir-bozhinov. Работата е много силна и дисциплинирана; по-долу са наблюденията, разделени по тежест. Сигурност и цялост на данните — чисто ✅
Незначителни бележки (не блокиращи)
Съответствие с описанието на PR-аИмплементацията покрива описаното: три лещи (време / CPV / кръстосано), споделен списък с филтри по година и CPV, комбиниран чарт брой+обем с кликаеми години, CPV разпределения (медиана, p10–p90, лог-скала, отличени ≥5×), изцяло SSR през query params, edge cache ключове покриващи новите параметри. Съответствието е пълно. Едно нещо за потвърждение преди merge
Отлична работа — кодът е чист, добре тестван и OWASP-съобразен. 👏 Verdict: APPROVE (по същество) — при зелен CI и спазен merge ред след #169; преномерирайте дублирания |
|
Rebase-нат върху main с css split (обзор стиловете → styles/pages.css), prettier-чист, линеен. Резолвнах старите нишки — и двете бележки бяха затворени в предишните fix-ове (quarter-guard + adjacent-year YoY). |
|
Здравейте, @StanislavBG 👋 Направих повторен, задълбочен преглед на текущия връх на клона ( Сигурност и цялост на данните — чисто ✅
Незначителни бележки (не блокиращи)
Съответствие с описанието на PR-аПълно. Три лещи (време / CPV / кръстосано), споделен списък с филтри по година и CPV, комбиниран чарт брой+обем с кликаеми години, CPV разпределения (медиана, p10–p90, лог-скала, отличени ≥5×), изцяло SSR през query params, edge cache ключове покриващи новите параметри — всичко присъства. Строго-съседният YoY ( За потвърждение преди merge
Отлична, чиста и добре тествана работа — OWASP-съобразна на слоя на кода. 👏 Verdict: APPROVE (по същество) — при зелен CI и спазен merge ред след #169; преномерирайте дублирания |
|
прогнозата е извадена в отделен draft PR (#192) по продуктово решение; графиката показва само реални данни, текущият месец е изключен по подразбиране (контрол „вкл. текущия месец", |
|
Здравейте, @StanislavBG 👋 Направих повторен, задълбочен преглед на текущия връх на клона ( Сигурност и цялост на данните — чисто ✅
Незначителни бележки (не блокиращи, пренесени)
За потвърждение преди merge (блокиращо на ниво стек)
Кодът на самия обзор е чист, добре тестван и OWASP-съобразен. 👏 Verdict: APPROVE (по същество) — при условие, че поправката на #169 (връщане на |
…ns, overrun index)
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
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обзор на PR: „договори — обзор (лещи време/CPV/кръстосано)"
Какво прави PR-ът
Добавя ново табло „Договори — обзор" в apps/web с три аналитични лещи — време (time), CPV и кръстосано (cross). Включва графики (ComboTrendChart, разширен TrendChart до гранулярност месец/тримесечие/година), CPV мулти-селект, типични цени по група (медиани/percentile), fullscreen модал за графика, metric-info popover и list-search. Промените обхващат нови компоненти и loader (trends.tsx), значителни CSS добавки (components.css +902, pages.css +3074, tokens.css), DB заявки (trend.ts + миграция 0002 за индекс), cache-key логика (cache-key.ts) и API-контракта.
Сигурност и интегритет на данните — ЧИСТО ✅
- SQL инжекция: Всички заявки (
getSpendingTrend,getCpvGroupStats,getCpvGroupMedians,listOverviewContracts) са параметризирани (?+.bind). Нито една потребителска стойност не се интерполира в SQL. - Валидация на вход: CPV кодовете се филтрират с
^\d{5}$, дедуплицират и ограничават доMAX_CPV_GROUP_SELECTION(10);year→^20\d\d$;angle/step/sort/cpvSortминават презpick()срещу фиксирани списъци. Добра защита в дълбочина. - Кеш (CWE-349): Новите параметри са добавени в
CACHE_QUERY_PARAMS; drift-guard тест налага allow-list и ще счупи CI при липсващ ключ. Каноничен URL чрез сортирана селекция (hrefToggleCpv). - XSS: Всички стойности се рендират като JSX текст; няма
dangerouslySetInnerHTML. - Няма твърдо кодирани тайни, нови URL адреси, backdoor-и, обфускация или нови зависимости. Няма изтичане на PII (публични данни за поръчки; консорциуми се сгъват до „X и др.").
Блокиращи концерни (изискват промени)
- Мъртъв импорт
singleSelectFiltersвtrends.tsx— новият loader вече не го използва; нарушава „NO DEAD CODE" и вероятно чупиnoUnusedLocals/ESLint. Трябва да се премахне. - Липса на тестове за новата UI логика —
ComboTrendChart,MetricInfo,FullscreenButtonи loader-ът наtrends.tsxнямат тестове (самоfilters.tsе покрит). При праг ≥90% за нов код това не покрива Tests gate; добавете поне тестове за чистата логика (periodLabel,relLabel/multText) и поведението на loader-а. (Забележка: DB слоят и cache-key са добре покрити.)
За потвърждение преди merge
- Че
MetricInfoиFullscreenButton/useFullscreenреално се консумират някъде (иначе са мъртъв код). pages.css(+3074) беше прегледан без наличен diff — необходимо е ръчно потвърждение за: дублиран/неизползван код, външниurl()/@importкъм одобрени домейни, липса на скрити интерактивни елементи иexpression()/-moz-binding, отсъствие на scope creep извън таблото.
Дребни бележки (не блокиращи)
- Дублиране на CSS компоненти:
.fs-btnи.trend-fs-btn(вcomponents.css) са почти идентични fullscreen бутони — обединете в базов клас или премахнете остатъчния. Плюс непоследователен serif fallback и повтарящ се'IBM Plex Mono'стек (~15 правила). - Достъпност: тултипът на
ComboTrendChartеrole="status"(aria-live) и се обновява при всеки hover — възможен шум за екранни четци при pointer потребители. (Иначе достъпността е силна: SSR/no-JS,role="img",aria-label,:focus-visible, ≥44px touch-таргети.) - В леща „time" изборът на година филтрира само списъка, но не и графиката (
getSpendingTrendне получаваyear) — потвърдете, че е по замисъл, за да не заблуждава подсказката. - Премахнатият тест за обратната посока на drift-guard отслабва маркирането на остарели allow-list записи (компромис по поддръжка, не дупка в сигурността).
getCpvGroupStatsправи пълни сканирания с GLOB на всеки прозорец на кеша — приемливо за ~190k реда, но заслужава внимание при бъдещ ръст.- Миграцията
0002е forward-only (без rollback) — съответства на стила на репото.
Съответствие с тикета
Реализацията отговаря на заглавието: трите лещи (time/cpv/cross), стъпки месец/тримесечие/година, CPV мулти-селект и типични цени по група. Няма забелязан scope creep (подлежи на потвърждение за pages.css).
Обща препоръка: REQUEST_CHANGES — след (1) премахване на мъртвия импорт singleSelectFilters и (2) добавяне на тестове за новата UI логика. Сигурността, интегритетът на данните и кеш-логиката са в добро състояние; останалите бележки са незадължителни почиствания.
- trends.tsx: remove dead singleSelectFilters import - cache-key.test.ts: restore soft, non-failing allow-list drift check (reusing the pattern already landed on pr/dash-base midt-bg#169, not a second copy) - trend.ts: document getCpvGroupStats' full-scan perf tradeoff with a TODO for future precompute if corpus size grows ComboTrendChart's tooltip role="status" stays as-is: its per-hover data isn't exposed accessibly anywhere else on the page, so removing the live region would silently drop that data for screen readers. filters.ts:41 (cpvGroupSelection validation) verified correct as-is per reviewer confirmation — no code change.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обзор на PR — договори: обзор (лещи време/CPV/кръстосано)
ВЕРДИКТ: COMMENT — няма блокиращи проблеми със сигурността или коректността. Препоръчват се няколко дребни корекции преди сливане.
Какво прави PR-ът
Добавя ново табло „Тенденции" (маршрут /trends) за обзор на договори с три лещи — по време, по CPV и кръстосано. Включва React компоненти за графики (ComboTrendChart, TrendChart, MetricInfo, FullscreenButton), помощна логика за филтри и лещи (analytics-lenses.ts, filters.ts), параметризирани SQL заявки (packages/db/src/queries/trend.ts), разширяване на кеш ключа (cache-key.ts), нова миграция за индекс (0002_contracts_overrun_index.sql), както и значителни CSS добавки (components.css, pages.css). Реализацията е SSR-first, с <Link>-базирани контроли и съвместимост без JS.
Сигурност — ЧИСТО ✅
- SQL инжекции: няма. Всички заявки са изцяло параметризирани (
.bind(...)); динамично се сглобяват само плейсхолдъри, никога потребителски стойности. - Валидация на вход (защита в дълбочина): CPV групите се валидират с
/^\d{5}$/, дедуплицират се и се ограничават доMAX_CPV_GROUP_SELECTION = 10. Полуотвореният диапазонcpvGroupRangeе коректен, включително за префикс завършващ на „9". Покрива CWE-349. - Кеш ключ (CWE-349): коректно разширен с всички релевантни параметри; свръх-ключирането е безопасно.
- XSS: няма — React екранира по подразбиране, SVG съдържанието е числово,
ariaLabelвгражда само валидирани CPV кодове. - Без твърдо кодирани тайни, без нови зависимости, без промени по URL/whitelist, без обфускация/бекдори/инжектиране.
⚠️ pages.css(+3074 реда) не беше достъпен за преглед по редове (липсва diff/съдържание). Необходима е ръчна проверка за външниurl()/@import,data:/javascript:URI и остарели вектори, както и за обхват спрямо тикета.
Качество и тестове
Като цяло висококачествен, добре коментиран PR, последователен със съществуващите шаблони. Тестовете са обстойни и смислени (не са „cheater"): дедупликация и хигиена на CPV ключове, поправка на YoY при празнина, тримесечно зърно, floor-rank перцентили, сгъване на консорциуми и др.
Второстепенни бележки (незадължителни)
- Дублиране на код (CLAUDE.md: NO CODE DUPLICATION). Изчислението на
yearStart/ticksе идентично вComboTrendChart.tsxиTrendChart.tsx— извлечете в споделен помощник. Аналогично в CSS:.fs-btnи.trend-fs-btnса почти идентични бутони за цял екран. - Деление на нула в
relLabel(trends.tsx).valueEur / medianEurдаваInfinity, акоmedianEur === 0. На практика медианата вероятно е > 0, но добавете защита (пропускане на етикета приmedianEur <= 0). - Кеш фрагментация при повтарящ се
cpv. Равностойни набори с различен ред дават различни кеш ключове — разчита се на UI да сортира предварително. Обмислете канонизиране (сортиране) в самияcacheKey. - Отслабен drift guard (cache-key.test.ts). Тестът за неизползвани allow-list записи е сведен от твърд провал до
console.info— мъртви записи вече могат да останат незабелязани в CI (спрямо „NO DEAD CODE"). - Хигиена на edge-cache ключа отвъд
cpv.pick()запазва невалидни/неразпознати параметри в URL-а — потенциален вектор за наводняване на кеша, ако CDN ключова по пълния query string. Обмислете канонизиране. - Семантика на
totalGroups(getCpvGroupStats): броячът и класирането минават по различни множества — потвърдете, че KPI > показани групи е търсеното поведение. - Обратимост на миграцията (
0002_contracts_overrun_index.sql): идемпотентна, но без down-миграция — добаветеDROP INDEX, ако проектът изисква обратимост. - UX бележка: в лещите „time"/„cross" изборът на година филтрира само списъка, но не и графиката — възможно е да подведе потребителя; добавете пояснение.
- Дребни: непоследователен шрифтов стек в CSS;
backdrop-filterбез-webkit-префикс; крайни случаи вhasPartialи съобщението „(показани първите 24)".
Заключение
Сигурен, добре тестван PR, следващ установените модели — няма основания за REQUEST_CHANGES. Препоръчва се преди сливане да се адресират (или изрично да се приемат) бележки 1–5; 6–9 са за потвърждение. Задължително се изисква ръчна проверка на pages.css за външни url()/@import, тъй като файлът не беше достъпен за автоматичен преглед.
# Conflicts: # apps/web/app/lib/filters.test.ts
…ses) - Extract shared yearAxisTicks/periodLabel helper (lib/trendAxis.ts) so ComboTrendChart and TrendChart no longer duplicate the X-axis year-start/ tick computation. - Document the hasPartial index-0 invariant in ComboTrendChart. - Add trendAngle/trendStep/trendSort validate-or-fallback helpers in filters.ts, colocated with cpvGroupSelection, and use them in trends.tsx instead of a local unvalidated pick(). - Consolidate .fs-btn/.trend-fs-btn into one shared rule in components.css and align their mono-font declaration with the rest of the file's 'IBM Plex Mono', var(--font-mono) convention.
…#188 Twelve straightforward review threads left over after two prior rounds (PRDs 426/506): dedupe the year-axis-tick logic between TrendChart and ComboTrendChart into a shared trendAxis.ts helper (noting it should fold into midt-bg#170's copy at merge time), document the deliberate totalGroups vs top-N amount_eur difference, harden the rank-fallback in toGroupStat to log instead of silently returning the sample minimum, fix the /quality histogram's zone-link click targets being shadowed by the bins' full- height hit-rects (SVG paint order), add id="main" to the /quality empty state, skip the getCpvGroupMedians round-trip on /trends when nothing is missing from the top-N stats, and replace migrations.test.ts's soft length check with an exact file-list assertion so a deleted migration fails loudly. The remaining threads (rdir/sel/band parameterization, quality.ts sort direction, analytics.tsx divide-by-zero guard, trends.tsx input validation) were reviewer-confirmed correct as-is — acknowledged, no code change. Out of scope per this round: ship-domain.mjs / import.mjs / derive-contract-features.sql integrity-gate behavior — those are being decided separately with the repo owner.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Ре-ревю на връх 2c1ab8b (делта). По оста точност е чисто (проследих overruns/CPV сметките — guard-нати); две неща за оправяне преди merge, плюс дребни.
Major — TotalsStrip смесва филтрирана и нефилтрирана база. При активен CPV facet обща стойност идва от getSpendingTrend(…, { cpvGroups: cpvSel }) (филтрирано, trends.tsx:83-85 → :238), но CPV групи = stats.totalGroups от getCpvGroupStats(db, 10) (trends.tsx:88 — без cpvSel, винаги корпусно, → :240). Филтрирай към 1 група → обща стойност се свива до групата, а CPV групи пак показва всички, без етикет за базата — в един и същ strip. Не е грешна сметка, а подвеждащо съпоставяне. Fix: подай cpvSel в getCpvGroupStats, или маркирай броя групи като корпусен.
Major — най-тежкият loader (cost). След основния Promise.all, getCpvGroupMedians (trends.tsx:99) прави по една заявка на липсваща карта-група, а getCpvGroupStats вътрешно пуска втора вълна distribution заявки на топ групите — при cold load десетки D1 statement-а на няколко последователни await бариери. Смекчено от 1800s edge cache (върви веднъж на прозорец) и обхватът е bounded, но кодът сам маркира precompute-rollup TODO. Batch-ни median-ите/разпределенията в един CTE/UNION.
Дребни: amount_eur > 0 (listOverviewContracts) vs IS NOT NULL (trend series scope()) — нулев value_low ред влиза в брой договори на бара, но не в списъка за същия scope; избери една база. И CPV групи брои distinct групи от tenders (не по amount_eur база като другите две числа в strip-а).
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обзор на ревюто — feat(web): договори — обзор (лещи време/CPV/кръстосано)
Общ вердикт: COMMENT — няма блокиращи проблеми. Одобрение след няколко незадължителни уточнения.
Какво прави PR-ът
Добавя нов изглед „Договори — обзор" в /trends с лещи по време (месец/тримесечие/година), по CPV групи и кръстосан (cohort/A-B) анализ. Промените обхващат: валидиране на входовете от URL и слоя за данни (apps/web), нови агрегиращи заявки в @sigma/db (getSpendingTrend, getCpvGroupStats, getCpvGroupMedians, listOverviewContracts), извличане на обща логика за осите (lib/trendAxis.ts), нов индекс в миграция 0002, плюс обемни презентационни промени в components.css (+897), pages.css (+3074) и tokens.css.
Силни страни
- Сигурност/цялост на данните: строга валидация на всички входове — CPV кодове през allow-list
/^\d{5}$/с дедупликация и таван,year/angle/step/sortспрямо allow-list с безопасен fallback. Всички нови заявки са параметризирани (?+.bind()), със защита в дълбочина. Няма твърдо кодирани тайни, нови външни URL адреси или зависимости. XSS е покрит от React екранирането. Полуотвореният CPV диапазон, перцентилите и YoY сравнението (строго съседна година, деление на нула →null) са коректни и тествани. - Кеш (CWE-349): новите параметри, влияещи на отговора, са добавени към keyed allow-list с тестове.
- Качество: премахнато дублиране чрез
lib/trendAxis.ts; детерминиранjitter(SSR-безопасен). CSS мести inlinestyle=към класове (по-добро спрямо CSP), с коректни:focus-visible, хит-зони ≥24/44px, достъпен popover и последователни z-index слоеве. - Тестове: отлично, смислено покритие на чистата логика и заявките (гранулярност, празни години, отхвърляне на невалидни CPV, фолдване на консорциуми).
Незадължителни бележки (не блокират)
- Отслабен drift-guard: тест, който преди е падал при неизползвани allow-list записи, вече само логва
console.info— печатни/остарели записи могат да останат незабелязани. Предложение: запазетеexpect.softили CI annotation. - Некононизиран ред на CPV мулти-селект в кеш ключа:
cpv=A&cpv=Bиcpv=B&cpv=Aдават различни записи — само фрагментация на кеша, ниска тежест. - Cross-batch потвърждение:
gе премахнат отCACHE_QUERY_PARAMS(заменен отstep) — потвърдете, че нито един маршрут извън тази партида вече не четеg. - Дребни дублирания:
pickvspickEnum; повтарящ се шрифтов shorthand в CSS (може да е обща променлива). hasPartial = partialIdx > 0тихо премълчава случая на първи частичен период — препоръчва се dev-инвариант/гард срещу регресия.
Производителност
getCpvGroupStats прави пълен GROUP BY скан (GLOB не е индексируем), но маршрутът е edge-кеширан и има ясен TODO за rollup — приемливо. Per-group разпределенията коректно ползват idx_tenders_cpv.
Ограничения на ревюто
apps/web/app/styles/pages.css(+3074) не беше достъпен като diff и не можа да бъде проверен автоматично — предвид обема препоръчвам ръчен преглед за дублиран/мъртъв стил и scope creep, плюс визуален/скрийншот тест на засегнатите страници.- MCP конекторите (Atlassian, Google Drive) не бяха достъпни в тази сесия, затова описанието на тикета не беше сверено директно.
Препоръка: одобрение след адресиране на т.1 и потвърждение на т.3, и ръчен преглед на pages.css.
- 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.
- filters.ts/trends.tsx: move cpvSort validation into a shared trendCpvSort (built on pickEnum), removing the local `pick` duplicate in trends.tsx - ComboTrendChart.tsx: dev-only console.warn when the partial-is-always-last invariant is violated, instead of silently masking it - components.css: extract the repeated 'IBM Plex Mono', var(--font-mono) shorthand into a shared --font-mono-plex token - cache-key.ts: sort cpv values before composing the edge-cache key so cpv=A&cpv=B and cpv=B&cpv=A collapse to one cache entry - cache-key.test.ts: restore the CACHE_QUERY_PARAMS drift guard as a soft (non-blocking) assertion, fix a scanner regex blind spot on multi-line .getAll() calls, and exclude documented stacked-later params via the new PLANNED_QUERY_PARAMS set so only genuine drift fails the check
# Conflicts: # apps/web/app/lib/filters.test.ts # apps/web/app/styles/components.css # apps/web/workers/cache-key.test.ts # apps/web/workers/cache-key.ts
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обзор на ревюто — feat(web): договори — обзор (лещи време/CPV/кръстосано)
Какво прави PR-ът
Добавя ново табло /trends за преглед на договори с три лещи (време / CPV / кръстосано). Включва: рефакторинг на общата ос-логика в lib/trendAxis.ts (премахва дублирането между TrendChart и ComboTrendChart), последователно валидиране на URL параметрите, нови компоненти (MetricInfo, FullscreenButton, useFullscreen), и значителен обем нови стилове (components.css +865, pages.css +3074). Прегледът е извършен на 4 партиди (patch за pages.css не беше наличен за автоматичен анализ).
Обща оценка
Кодовата логика (партида 1) е чиста, добре структурирана, с ясна защитна дисциплина при обработката на вход от URL и смислени регресионни тестове за trendAxis. Скановете за сигурност са чисти: няма твърдо кодирани тайни, нови URL адреси, външни зависимости, обфускация или инжекционни вектори; входните стойности се валидират по модел „валидирай-или-падни-към-безопасно-по-подразбиране" (allowlist за enum-ите, ^\d{5}$ за CPV, ^20\d\d$ за година).
Най-важни находки (не блокиращи)
- Деление на нула в
relLabel(trends.tsx).valueEur / medianEurприmedianEur === 0даваInfinity/NaNи извежда×Infinity. Препоръка: guardmedianEur > 0преди изчислението. — препоръчва се да се адресира преди merge. - Канонизация на кеш-ключа за повтарящ се
?cpv.cpvGroupSelectionзапазва реда от URL, докато приложението генерира сортирани връзки. Различен ред/състав на кодовете дава идентичен резултат, но различни необработени query низове → потенциална амплификация на кеш-варианти (близко до CWE-770). Препоръка: нормализирайте (сортиране + дедупликация) при генериране на кеш-ключа или редиректвайте към канонична форма. (Кеш-логиката е извън прегледаните партиди — да се потвърди.) - Недокументирана промяна в поведението: миграция
g→step. Старите отметки/trends?g=yearтихо падат към стъпка по подразбиране. Функционално безопасно, но струва си ред в бележките към PR.
По-малки бележки
hasPartial = partialIdx > 0не улавя регресия при индекс 0 (ниска тежест, инвариантът е документиран).useFullscreenне се вижда да се използва в прегледаните партиди — да се потвърди употребата (иначе dead code).- CSS (
components.css):.metric-info-popне възстановяваpointer-events: autoпри отваряне (текстът не е селектируем); непоследователни fallback-ове за шрифт (--font-serif/--font-monovs--font-mono-plex); липсваprefers-reduced-motionза преходите. Положително::focus-visibleoutline-и и ≥44px touch targets.
Изисква ръчна проверка
pages.css(+3074/-3) не можа да бъде прегледан автоматично (липсва patch). Поради обема се препоръчва ръчна проверка за дублиране на стилове, мъртъв/неизползван CSS, използване на design tokens вместо твърдо кодирани стойности и липса на нежелани външниurl(...)/@import.- Да се потвърди, че DB слоят използва параметризирани заявки и че всеки нов CSS клас реално се използва в компонентите (правила „NO CODE DUPLICATION" / „NO DEAD CODE").
Тестове
Покритието на trendAxis е добро и смислено. Липсват тестове за рендиране на ComboTrendChart, за граничните случаи на relLabel/multText (медиана 0, mult точно 1.3/0.75) и за клиентското поведение на MetricInfo/FullscreenButton — препоръчва се добавяне.
Заключение
COMMENT — няма блокиращи проблеми по сигурност или коректност в прегледаните партиди. Преди merge препоръчвам да се адресират деление на нула (т.1) и кеш-канонизацията (т.2), да се документира миграцията g → step, и да се извърши ръчен преглед на pages.css, чийто diff не беше наличен за автоматичен анализ.
…pover pointer-events
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обзор на ревюто — feat(web): договори — обзор (лещи време/CPV/кръстосано)
Вердикт: COMMENT — няма блокиращи проблеми; препоръчва се човешко потвърждение преди финално одобрение.
Какво прави PR-ът
Въвежда таблото „Договори — обзор" на /trends с три лещи за анализ (време / CPV / кръстосано) и стъпка на периода (месец m / тримесечие q / година y). Промяната обхваща: фронтенд логика и компоненти за визуализация (тренд графики, popover за метрики, режим цял екран, лента за търсене), централизирана логика за осите на трендовете, валидация на URL параметри, презентационен CSS, както и параметризирани DB заявки за трендове, медиани по CPV групи и списък на договорите, заедно с миграция за индекс.
Сигурност и цялост на данните — чисто
Проверката (Phase 0 + ниво агент) не откри блокиращи проблеми в нито една партида:
- Няма твърдо кодирани тайни, подозрителни/външни URL адреси,
@import, обфускация, бекдори или нови зависимости (важи и за CSS файловете). - SQL-инжекции (OWASP A03): всички заявки са изцяло параметризирани (
?); единствените вмъквани в SQL низове са константи или клауза само от?. Потребителските CPV кодове се валидират с/^\d{5}$/и се изхвърлят при несъответствие (тестове с"45'--",%27--,abcdeпотвърждават защитата в дълбочина). Enum параметрите (trendAngle/Step/Sort и др.) минават през allowlist с безопасен fallback. - Кеш-отравяне (CWE-349): CPV множеството се канонизира (дедупликация, канонично сортиране, ограничение до 10 групи) в
cache-key.ts, така че еднакви множества с различен ред колапсват в един кеш запис. Добро покритие с тестове. - Цялост на данните: коректни са поправката на YoY „строго съседна предходна година", сгъването на месеци в тримесечия, изключването на текущия (as_of) период и огледалният percentile (SQL floor-rank ↔ JS
rankOf).
Незначителни (не блокиращи) бележки
- ComboTrendChart.tsx —
hasPartial = partialIdx > 0: ако частичен период попадне на индекс 0, редът се рисува плътен и DEV предупреждението не се задейства. Инвариантът „частичният е винаги последен" е документиран, но остава тих провал. - trends.tsx —
mult = valueEur / medianEurняма защита приmedianEur === 0(води до „×Infinity типичното"). Малко вероятно, но евтин guard. - trends.tsx —
hrefToggleCpvизгражда селекция без горна граница, а loaderът я отрязва до 10 лексикографски — добавянето на 11-та група изглежда без ефект в URL (объркващо UX при 10+ избрани). - query-params.ts / filters.ts — премахнатият параметър
g(заменен съсstep) означава, че стари връзки с?g=…падат към стойност по подразбиране (тримесечно). Не се чупи нищо, но заслужава миграционна бележка. - DB заявки —
getCpvGroupStatsсъдържа inlineTODOза бъдещ per-group rollup (имплементацията е пълна); препоръчва се преобразуване в тикет вместо TODO (правило „NO SIMPLIFICATION with TODO"). - DB заявки —
listOverviewContracts/getSpendingTrendразчитат на маршрута за таван наlimit(няма клампване тук); потвърдете разумен таван срещу прекомерно сканиране. - CSS — смесени препратки към mono/serif шрифтове (
--font-mono-plexvsvar(--font-mono)vs вграден fallback); липсва guard@media (prefers-reduced-motion: reduce)за преходите на popover/модал. Козметично.
Точки за човешка проверка
- Партида 3/4 (
apps/web/app/styles/pages.css, +3074/-3): patch-ът не е наличен (бинарен/твърде голям), затова редови преглед не беше възможен. Необходима е ръчна проверка за: евентуалниurl(...)/@importкъм външни ресурси, дублиране на стилове (правило „NO CODE DUPLICATION") и спазване на конвенциите за именуване. - Финалният вердикт следва да се потвърди след човешко ревю на този файл и агрегиране на всички партиди.
Силни страни
Фокусирана промяна без scope creep; централизиран trendAxis.ts без дублиране; смислени тестове (референтен оракул за поведенческа идентичност, враждебни входове); SSR-безопасни ефекти; достъпни атрибути (aria-label, role="status", role="img", :focus-visible, hit-таргети ≥44px); изнасяне на inline стилове в CSS заради CSP чистота; обратима миграция по шаблона на проекта.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Ре-верифицирах на HEAD (aae0b5ae): новият комит канонизира реда на CPV селекцията + pointer-events на popover-а — презентационно; data-слоят (trend.ts, стойностна база amount_eur, YoY, partial-period) е непроменен. Чисто, както преди. Одобрявам.
…undant mono fallback ComboTrendChart now warns in DEV when the partial period lands at index 0 (previously only checked partialIdx > 0 && !== n-1). components.css list-search-btn used a redundant ', monospace' fallback since --font-mono is always defined in tokens.css.
hasPartial relied on partialIdx > 0, treating a partial period at index 0 the same as no partial period at all and silently rendering the whole series solid. Use the -1 not-found sentinel from findIndex instead
--font-mono-plex already falls back to --font-mono, so consolidating loses nothing while removing the ambiguity ydimitrof flagged between the two tokens on PR midt-bg#170
…ter RSC CVE postcss 8.5.15 -> ^8.5.18 fixes GHSA-r28c-9q8g-f849 (path traversal via sourceMappingURL auto-load); valibot 1.4.0 -> ^1.4.2 fixes GHSA-5qjj-4xww-7phc (flatten() crash on inherited-property keys). Both are non-breaking patch-level bumps via pnpm overrides. react-router's GHSA-qwww-vcr4-c8h2 (CSRF in unstable RSC code paths, fixed only in 8.x) is suppressed in osv-scanner.toml with a time-boxed ignore - this app never uses RSC APIs (verified via repo-wide grep), and bumping to react-router 8.x is a breaking major version change out of scope for a security patch to an unused code path
- osv-scanner.toml: keep both independently-added suppressions (sharp + react-router RSC CVE); pr/trends' entries were a strict superset - packages/db/src/migrations.test.ts + migrations/: both branches added a migration numbered 0002 for unrelated changes (main's 0002_current_value_currency.sql vs pr/trends' overrun index); renumbered pr/trends' migration to 0003_contracts_overrun_index.sql and merged the test assertions from both branches - apps/web/app/routes/trends.tsx: main's only change since the merge base was the getDb() read-only-D1 chokepoint refactor (midt-bg#225); kept pr/trends' full obzor rewrite and applied that same chokepoint to its loader - pnpm-lock.yaml: regenerated via pnpm install rather than hand-merged
pnpm-lock.yaml regenerated by the origin/main merge resolves sharp to 0.35.3, past the 0.35.0 fix for GHSA-f88m-g3jw-g9cj; osv-scanner.toml's own policy requires deleting a suppression once the package clears its fixed version
… + 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>
|
Perf / D1-разход на cache miss в const missing = contracts.map((c) => c.cpvGroup).filter((g) => g != null && !known.has(g));
for (const g of cpvSel) if (!known.has(g)) missing.push(g);
const medians = await getCpvGroupMedians(db, [...new Set(missing)]); // ← без горна граница
Поправка: сложи горна граница на (Euro-annex капанът (#245) НЕ важи тук — trend-овете агрегират по |
|
Този клон е в конфликт с |
4 participants
What changed
Contracts overview (
/trends) — three lenses (time / CPV / cross-filter) over the contractsdataset, plus five rounds of review fixes from
ydimitrof.Feature
/trendsroute: time-series, CPV-group, and cross-filter lenses over contracts, withSSR/no-JS-safe URL-driven filters (angle, step, sort, cpv, cpvSort, year range).
lib/trendAxis.tsto remove duplication betweenTrendChartand the newComboTrendChart.MetricInfo(SSR-safe info popover) andFullscreenButton/useFullscreencomponents.Review rounds addressed (all threads resolved against the landed diff)
cpvmulti-select is now order-canonical —cpvGroupSelection(apps/web/app/lib/filters.ts) sorts the deduped/validated codes beforecapping, so
?cpv=A&cpv=Band?cpv=B&cpv=Aproduce an identical array and therefore oneedge-cache key instead of two (cache-fragmentation / CWE-770-adjacent).
query-params.ts'scpv/cpvSortcache-key params flow through this same canonical accessor — no second sortwas added. Restored
pointer-events: autoon the open-state.metric-info-poprule incomponents.cssso the popover is interactive (text selectable, hover-hold works) once open.cpvSortvalidation into a sharedtrendCpvSort(built on the existingpickEnum), removing a localpickduplicate; dev-onlyconsole.warninComboTrendChartwhen the "partial period is always last" invariant is violated; extracted the repeated
'IBM Plex Mono', var(--font-mono)shorthand into a--font-mono-plextoken; sortedcpvvalues before composing the edge-cache key in
cache-key.ts; restored theCACHE_QUERY_PARAMSdrift guard as a soft assertion (was a bare
console.info), fixed a scanner regex blind spoton multi-line
.getAll()calls, and addedPLANNED_QUERY_PARAMSfor documented stacked-laterparams; removed a stray internal draft-reply file accidentally committed under
apps/web/.singleSelectFiltersimport intrends.tsx; confirmed theg→stepquery-param rename has no remaining readers andcpvGroupRangecorrectly handles5-digit prefixes ending in
9via its half-open range; addressed a11y/duplication notes onComboTrendChart's tooltip and sharedyearStart/tickslogic.MetricInfo/FullscreenButton/useFullscreenare consumed by thisbatch (not dead code); consolidated duplicate
.fs-btn/.trend-fs-btnCSS; validation orderin
cpvGroupSelection(dedupe → validate → cap) confirmed correct..trend-chart-panel--full .trend-chart-bodyCSS rule and astray
overruns-dashboardcomment marker left over from an earlier refactor; addressed theuseLayoutEffect-on-SSR warning inMetricInfo.How it was tested
pnpm --filter web run typecheck— clean.pnpm --filter web test— 37 files / 389 tests passing, including the round-5cpvGroupSelectionorder-canonicalization case (cpv=b&cpv=aandcpv=a&cpv=byield the samesorted array) and the restored
CACHE_QUERY_PARAMSdrift guard./contractsoverview across all three lenses (time/CPV/cross-filter).Quality checks
aae0b5a).ydimitrof) re-verified against the landed diff and resolved.