feat(web): dashboard design-system base (MetricInfo, fullscreen, tokens, overrun index) - #169
feat(web): dashboard design-system base (MetricInfo, fullscreen, tokens, overrun index)#169StanislavBG wants to merge 20 commits into
Conversation
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Благодаря — основата е добре структурирана: MetricInfo и useFullscreen са чисти (SSR-safe, listener cleanup-ът го има, feature-detected), а миграцията е идемпотентна и partial index-ът върху annex_count > 0 е правилно стесненият набор. Но пуснах PR-а локално и има две неща за оправяне преди merge — и двете минават под зелените тестове.
🔴 Премахнат е CWE-349 drift guard-ът на cache-key. cache-key.test.ts губи consumedQueryParams() + целия describe('CACHE_QUERY_PARAMS drift guard') (тестовете, които статично сканират app/** и падат CI, ако consumed param липсва от allow-list-а), а INTENTIONALLY_UNKEYED е махнат от cache-key.ts. Описанието казва само, че allow-list-ът се разширява — не че guard-ът пада. След това: забравен response-affecting param → два различни изгледа колабират в един cache entry → грешни данни под общ ключ, без CI мрежа. Точно за това стоеше guard-ът (#56), и точно него цитират docs-овете в #143/#149 като жива защита.
Виждам и защо е паднал: добавяш a/b/by/metric за /compare и /overruns, чиито route-ове ги няма още на този branch, та „no stale entries" тестът гърми. Но това е perf-hygiene посоката. Сигурностната посока — consumed ⊆ CACHE_QUERY_PARAMS — трябва да остане. Махни само stale-entries assert-а (или премести добавянето на параметрите в PR-овете, които въвеждат route-овете им) — не целия guard. Иначе следващият забравен param (точно в #170/#171) минава тихо.
🟠 app.css трие стиловете на жив компонент. -178-те реда махат .risk-indicators / .risk-title / .risk-list и не ги връщат под друго име (нула .risk* селектора остават в app.css), но RiskIndicators.tsx още ги ползва и се рендва на routes/contract.tsx — страницата на договора. Резултат: risk-кутията на всеки договор става неоформен текст (без warning фон/бордюр/булети). Или върни правилата, или мигрирай RiskIndicators към новите токени в този PR.
Останалото е чисто. Но и двете горе трябва да се затворят преди merge — а понеже това е PR 1/4, блокира целия stack.
|
Благодаря за внимателния преглед — и двата блокера са затворени (force-push 🔴 CWE-349 drift guard възстановен. Върнах 🟠 Risk стиловете върнати. |
|
Благодаря за бързата и прецизна реакция — пуснах branch-а локално и го прегледах стриктно за сигурност и интегритет на данните; и двата блокера на @lyubomir-bozhinov са наистина затворени. Какво потвърдих локално (
Две малки, неблокиращи бележки:
Нито една от двете бележки не блокира merge. Заключение: Одобрявам — няма открити проблеми със сигурността или интегритета на данните; OWASP-съвместимо. Двете бележки по-горе са по желание. |
|
|
Потвърждавам за merge: двата предишни блокера са затворени ( |
|
Прегледах PR #169 стриктно и локално — за сигурност, интегритет на данните, SQL/injection вектори и злонамерен код. Пуснах тестовете сам и проверих всяко твърдение от дискусията вместо да го приема наготово. Обхват на промяната Какво потвърдих локално (
Заключение по OWASP: не откривам критични уязвимости; входната валидация, cache-ключването и DDL-ът са коректни и съвместими с OWASP Top 10. Двамата предходни рецензенти вече затвориха двата реални блокера; независимата ми проверка го потвърждава. Останалите бележки са по желание и не блокират merge. Вердикт: Одобрявам — няма открити проблеми със сигурността или интегритета на данните; OWASP-съвместимо. |
|
Rebase-нат върху новия main (5d99fd9) с css split: токените → styles/tokens.css, MetricInfo/fullscreen → styles/components.css, останалото → styles/pages.css; app.css е недокоснат (само @imports). Проверено: компилираният CSS е правило-по-правило идентичен с пред-rebase версията (1298=1298), prettier-чист, линейна история, пълен локален browser тест на всички страници без грешки. @todorkolev — стекът е готов в ред #169 → #170 → #171 → #172 (нови heads: 764a4be / 02e3e42 / 1c120d9 / e884d6c). |
|
Verification complete. All checks pass on current HEAD ( Прегледах PR #169 стриктно и локално на текущия връх ( Обхват Какво потвърдих локално
Заключение по OWASP: не откривам критични уязвимости; cache-ключването, входната валидация и DDL-ът са коректни и съвместими с OWASP Top 10. Неблокиращи бележки (по желание):
Вердикт: Одобрявам — няма открити проблеми със сигурността или интегритета на данните; OWASP-съвместимо. Двете бележки по-горе не блокират merge. |
…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.
|
@todorkolev готов за ревю 🙏 — rebase-нат на main, CI зелен, prettier-чист, CSS промените в styles/* (app.css само @import). Резолвнати нишки. Approve-ни когато ти е удобно. |
|
Прегледах PR #169 наново и стриктно — локално на текущия връх ( Обхват Какво потвърдих локално (
Съответствие с тикета: двата блокера на @lyubomir-bozhinov и моят предишен lens-блокер са затворени; независимата проверка го потвърждава. Обхватът е атомарен и подходящ за база на стек. Неблокираща бележка (по желание):
Заключение по OWASP: не откривам критични уязвимости; cache-ключването, входната валидация и DDL-ът са коректни и съвместими с OWASP Top 10. Вердикт: Одобрявам — няма открити проблеми със сигурността или интегритета на данните; OWASP-съвместимо. Бележката по-горе не блокира merge. |
|
Одобрявам. Ре-проверих стриктно на текущия връх
Без находки по базата на design-system-а. |
De-conflict the migration number: 0002 is claimed by the contracts_overrun_index family (midt-bg#169/midt-bg#170/midt-bg#171/midt-bg#172), 0003 by midt-bg#188 (contract_health), and 0004 by midt-bg#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.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: feat(web): dashboard design-system base (MetricInfo, fullscreen, tokens, overrun index)
Какво прави PR-ът
Този PR полага основата на дизайн системата за dashboard-а. Включва два малки React компонента (FullscreenButton.tsx, MetricInfo.tsx) и значителен обем нови стилове: изнасяне на inline стилове в CSS класове в components.css (добро за CSP чистота) и ~2373 реда нови стилове в pages.css за мрежов граф с fullscreen/zoom, choropleth карта, analyze-landing, price-anomaly и overruns дашбордите, базирани на дизайн токени.
Сигурност — CLEAN
И в двете прегледани партиди Phase 0 сканирането е чисто: няма твърдо кодирани тайни/ключове, няма нови или променени URL адреси, няма външни @import/url()/expression(), няма нови зависимости и няма обфускирани шаблони. Няма блокиращи проблеми по сигурността.
Силни страни
- Последователно използване на дизайн токени (
var(--ink),var(--accent),var(--rule)) с разумни fallback стойности. - Добро внимание към достъпност:
aria-label,focus-visible, touch hit-area ≥44px, спазване на WCAG 1.4.1 (цветът не е единственият носител на информация), клавиатурни пътища иprefers-reduced-motion. - Обмислени отзивчиви breakpoint-и и добре документиран код с коментари.
Основни забележки (не-блокиращи)
useLayoutEffectи SSR (MetricInfo.tsx): коментарът твърди „SSR-safe", ноuseLayoutEffectпредизвиква предупреждение при server render дори при раненreturn. Препоръка: isomorphic layout effect или обикновенuseEffect, тъй като изчислението и без това е клиентски път (приopen === true).- Остаряло изместване при
[open](MetricInfo.tsx): хоризонталният clamp се смята веднъж при отваряне; приresize/scrollдокато popover-ът е отворен стойността остарява и картата отново може да излезе извън екрана. Обмислете слушатели заresize/scrollдокато е отворен. aria-expandedспрямо hover/focus (MetricInfo.tsx): popover-ът се показва визуално чрез:hover/:focus-within, ноaria-expandedсе обновява само при click и сочи към постоянноaria-hidden="true"контейнер — семантично несъответствие. Влиянието е малко (текстът е вaria-label), но е добре да се изчисти.- Грешни CSS секционни маркери (
components.css): секцията отваря сtrends-dashboard, а затваря сend overruns-dashboard; маркерътend metric-info popoverреално затваря блокаlist-search. Козметично, но обърква навигацията. - Дублиран коментар (
pages.css, ред 869): коментарът за „Regional choropleth (/map)" е добавен наново, макар идентичен вече да съществува преди.map-wrap svg— премахнете единия. - Непоследователна
line-clampсъвместимост (pages.css):.clamp2включва и стандартнияline-clamp, но.az-card-desc,.pa-cardи.ov-annex-reasonразчитат само на-webkit-line-clamp— добавете и стандартното свойство. - Натрупване на
!important(pages.css): в.ov-auth-total,.ov-auth-growth(.is-top)и.ov-scatter-dot:hover— приемливо за таблични override-и, но е сигнал за специфичност. Освен това.ov-board-scaleе разделен на два блока, които могат да се обединят по-чисто.
Тестове / покритие
В прегледаните партиди няма тестове за новите компоненти (popover toggle, outside-click/Esc, fullscreen feature-detection). За база на дизайн система е приемливо, но абсолютната порта за покритие (≥90%) не е удовлетворена — отбелязано за общата оценка. Забележка: useFullscreen hook-ът е експортиран, но не се използва от FullscreenButton (компонентът приема active/onToggle отвън) — вероятно свързването е в друга част; ако не, hook-ът остава неизползван.
Вердикт
COMMENT — няма блокиращи или security проблеми. Изброените точки са подобрения по достъпност, коректност и козметика.
- MetricInfo: use an isomorphic layout effect (useEffect on the server) to drop the "useLayoutEffect does nothing on the server" SSR warning. - MetricInfo: re-clamp the popover horizontal shift on resize/scroll while open (rAF-throttled), not only on initial open, so it can't drift out of the viewport. - MetricInfo: sync aria-expanded with keyboard focus (not just click), since the popover also reveals on CSS :focus-within. - components.css: fix mismatched section markers (open/close names now match: trends-dashboard, list-search). - pages.css: remove duplicated "Regional choropleth" comment.
|
Addressed all 5 threads in 7c7e56a:
typecheck/test/format all green. |
- MetricInfo: isomorphic layout-effect guard to drop SSR warning (as midt-bg#169) - overruns: bidderEik falls back on empty string too, not just null/undefined - trends: guard CPV cohort relLabel against a zero median (no ×Infinity) - overruns: fix SECTION comment numbering + mast lede to match actual render order - components.css: unify open/close dashboard section markers, dedupe .fs-btn/.trend-fs-btn, add missing :focus-visible - tokens.css: --paper-raised as oklch(100% 0 0) instead of raw hex - cache-key.test.ts: soft guard requiring a route/pending note on unconsumed allow-list keys - ComboTrendChart: document the partialIdx > 0 assumption
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обобщение на прегледа — feat(web): dashboard design-system base
ВЕРДИКТ: COMMENT — няма блокиращи проблеми и няма проблеми със сигурността. Остават неблокиращи бележки и едно кросбач потвърждение преди APPROVE.
Какво прави PR-ът
Полага базата на дизайн-системата за дашборда: нов компонент MetricInfo (popover с подсказка) и FullscreenButton, ~1000 реда стилове в components.css, +2372 реда в pages.css, нови токени в tokens.css (slate/tan/ochre + paper-raised, Inter премахнат), разширяване на allow-list-а за кеш ключове (cache-key.ts) за предстоящите маршрути (/compare, /overruns, /price-anomaly) и нов частичен индекс за overrun агрегати (0002_contracts_overrun_index.sql) с тестове.
Сигурност — ЧИСТО ✅
Всички три партиди преминават Фаза 0 без забележки: няма твърдо кодирани тайни, няма нови/променени URL адреси, зависимости, dangerouslySetInnerHTML, eval, инжекция или обфускация. Целият текст минава през React (авто-escape). SQL миграцията е статичен DDL; тестовете подават фиксиран SQL без интерполация. Промяната по cache-key.ts дори подобрява защитата срещу CWE-349 (грешно споделяне на кеш) — критичният drift-guard е запазен и се налага в CI, така че свръх-кеиране е максималната цена, никога грешно тяло.
Силни страни
- SSR-безопасност (
useIsoLayoutEffect, feature-detection), коректно почистване на всички event listener-и, rAF коалесциране на scroll/resize. - Отлична достъпност: пълен
aria-label,:focus-within, touch fallback, outside-click/Esc затваряне, ≥44px touch target;prefers-reduced-motion; цветът никога не е единствен носител на смисъл (WCAG 1.4.1). - Миграцията е адитивна, идемпотентна (
IF NOT EXISTS), с реален тест за наличие на индекса; намалява пълните сканирания.
Неблокиращи бележки
- Два механизма за цял екран — native Fullscreen API hook (
useFullscreen) срещу CSS модал (.trend-chart-panel--full). Потвърдете кой се използва, за да няма мъртъв код. - Празен
.catch(() => {})вuseFullscreen.toggleпоглъща грешки без лог. - Esc не затваря popover, отворен само чрез фокус (ARIA tooltip шаблон изисква скриване).
cursor: helpвърху интерактивен toggle бутон —pointerе по-подходящо.- Потвърдете
color-mix(in oklch)/ самостоятелнотоtranslateспрямо матрицата от поддържани браузъри. - Дублиран селектор
.ov-board-scaleи непоследователенline-clamp(липсва стандартното свойство до-webkit-line-clampв.az-card-desc,.ov-annex-reason). !importantв няколко.ov-*селектора — потвърдете, че е за data-driven стойности, а не специфичностен конфликт.- Козметично: коментарът
end trends-dashboardзатваря блок, отворен катоprice-anomaly.
Гейт за тестове
Има солидно покритие за миграцията и drift-guard-а, но липсват unit тестове за MetricInfo/useFullscreen (clamp/shift, toggle, outside-click, Esc). Ако не съществуват в друга партида, гейтът за покритие ≥90% не е изпълнен.
Кросбач потвърждение преди merge
Пълнотата на allow-list-а за кеш ключове зависи от това маршрутите (в другите партиди) да четат query параметрите с литерални ключове (sp.get('cpv')), а не динамично — иначе минават покрай статичния скенер. Моля потвърдете това при ревюто на партидите с маршрутите.
Обща оценка: нисък риск, качествена и фокусирана промяна без scope creep. Готово за merge след изясняване на въпроса за тестовете и кросбач потвърждението по-горе.
…n index - log FullscreenButton's requestFullscreen failure instead of swallowing it - MetricInfo: Esc now dismisses a hover- or focus-only-opened popover (not just click-opened), aria-expanded reflects hover too, and the popover's shift-clamp math moves to a testable pure function with unit coverage at a 320px viewport - restore a soft, non-failing cache-key allow-list drift check (info-only) - widen idx_contracts_overrun to a composite (signing_value_eur, current_value_eur) partial index — EXPLAIN QUERY PLAN against a ~190k-row fixture showed it fully covers OVERRUN_WHERE's aggregates, ~4x faster than the annex_count-only index - convert --paper-raised to oklch(100% 0 0) to match the file's token convention (identical rendering to #ffffff) Confirmed no dynamic (non-literal) query-param reads in the cache-key allow-listed routes (trends/competition/map/flows) — all sel() call sites pass literal keys, so no code change was needed for that thread.
- 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: dashboard design-system база (MetricInfo, fullscreen, tokens, overrun index)
Какво прави PR-ът
Въвежда базата на design-system за dashboard-а: презентационни React/TSX компоненти (MetricInfo, FullscreenButton), чиста помощна функция за клампване на popover (metric-info-clamp.ts) с тестове, и значителен набор от стилове (components.css и +2372 реда в pages.css, покриващи .net-*, .map-*, .az-*, .pa-*, .ov-* overrun index). Промените са изцяло във фронтенд презентационния слой (React + CSS), без бекенд логика.
Сигурност — CLEAN
И двете партиди преминаха задължителния security скрийнинг чисто: няма твърдо кодирани тайни, няма нови/променени URL адреси или външни заявки, няма нови зависимости, няма инжекционна повърхност (без dangerouslySetInnerHTML, eval, expression(), url(javascript:) или външни @import). Не се изисква human security approval.
Блокиращи констатации
- [Коректност] Клампът на popover-а не е идемпотентен спрямо resize/scroll. В
MetricInfo.tsxrecompute()четеgetBoundingClientRect()след катоtranslate: ${shift}pxвече е приложен, затова при следващ resize/scroll изчислява от отместения rect и на практика нулира клампа — popover-ът пак излиза извън екрана (напр. естествено[96,328]@vw=320 →shift=-16→ rect[80,312]→ resize →dx=0→ отново клипва). Това обезсмисля логиката, за която са писани тестовете. Клампвай спрямо неотместена база. - [Покритие] Липсват тестове за интерактивните компоненти. Покрита е само чистата clamp функция;
MetricInfo(Esc / outside-click / state машина) иuseFullscreen/FullscreenButtonнямат тестове. Гейтът иска ≥90% покритие за нов код, а най-регресивно-податливата логика остава непокрита.
Не-блокиращи (дребни)
- Клампът се смята само при клик-
open, не при hover/:focus-withinразкриване — приalign='end'на тесен екран картата може да клипне. - Заблуждаващ тест: „prioritizes the left clamp when both edges clip“ реално задейства само лявото условие (
right=304 < 312); добави кейс, в който клипват и двата ръба. - CSS: дублирани правила за един селектор (
.ov-board-scale,.ov-panel-note) — струва си консолидация; непоследователенline-clamp(липсва стандартният до-webkit-line-clamp); локализирани!important(specificity смущение); зависимост от модерни фийчъри (:has(),color-mix,:fullscreen) без graceful fallback — да се потвърди спрямо матрицата за поддръжка.
Положителни неща
SSR-безопасни ефекти (useLayoutEffect→useEffect, четене на document само в ефекти), изрядно почистване на всички listener-и (без изтичане на ресурси), feature-detection на requestFullscreen/exitFullscreen, солидна достъпност (aria-pressed/aria-expanded/aria-label, aria-hidden на декоративен SVG, ≥44px touch target, :focus-visible рингове, цветът не е единствен носител на информация, prefers-reduced-motion). CSS е изнесен от inline style= заради CSP.
Отворени въпроси (за проверка в останалите партиди)
useFullscreenизнасяisFullscreen/toggle, ноFullscreenButtonприемаactive/onToggle— да се потвърди, че hook-ът реално се ползва (иначе потенциален мъртъв код).- Фокус-trap/възстановяване на фокуса за fullscreen модала (
.trend-chart-panel--full) вероятно е вroutes/trends.tsx.
ВЕРДИКТ: Заявка за промени (REQUEST_CHANGES) — заради самонулиращия се кламп (№1) и липсващото тестово покритие за интерактивните компоненти (№2). Няма проблеми със сигурността; останалите находки са дребни/козметични.
…-bg#169 review) - MetricInfo: recompute the horizontal clamp from the popover's unshifted natural rect (getBoundingClientRect includes the currently-applied translate), so repeated resize/scroll recomputes converge instead of compounding or cancelling the previous shift. - MetricInfo: the clamp effect now re-runs on any reveal path (click, hover, keyboard focus), not just the click-toggled `open` state, so a narrow-screen hover/focus reveal gets clamped too. - metric-info-clamp.test.ts: fix the "both edges clip" fixture so the right clamp genuinely fires before the left clamp overrides it, and assert both clamp conditions are true.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR — dashboard design-system база (MetricInfo, fullscreen, tokens, overrun индекс)
Обобщен вердикт: REQUEST_CHANGES — блокира само по един gate (тестово покритие); няма проблеми със сигурността.
Какво прави PR-ът
Въвежда базата на дизайн-системата за dashboard-а: компонент MetricInfo с popover и clamp-логика, fullscreen функционалност (FullscreenButton + useFullscreen), CSS токени (OKLch цветове, --paper-raised) и стилове за страници/компоненти, разширяване на cache-key параметрите в worker-а и нов частичен покриващ индекс за overrun заявки (0002_contracts_overrun_index.sql). Прегледът е направен на 3 партиди.
Сигурност — ЧИСТО (и трите партиди)
Няма твърдо кодирани тайни, нови/променени URL адреси, нови зависимости или зловредни шаблони (eval, innerHTML, dangerouslySetInnerHTML, expression(), javascript:). Текстът се рендира като auto-escape JSX; SQL миграцията е статичен DDL без интерполиран вход — няма вектор за инжекция.
Силни страни
clampPopoverShiftе коректно отделена чиста функция с добро тестово покритие (математиката е проверена ръчно).- SSR-безопасност навсякъде, изрядно почистване на listeners/rAF (без течове),
shiftRefизбягва stale closures. - Добра достъпност:
aria-label, Esc-затваряне, ≥44px touch target, WCAG 1.4.1 (цветът не е единствен носител на смисъл),prefers-reduced-motion, видими focus рингове. - Cache-key промените само добавят ключувани параметри — безопасно (по-грануларен ключ никога не връща грешно тяло); drift guard-ът остава твърд.
- Частичният индекс е добре проектиран (предикатът съвпада със заявката,
IF NOT EXISTS→ идемпотентност).
Блокиращ проблем
- Тестово покритие под gate-а (≥90%). Тестван е само чистият
clampPopoverShift. Стейт-машината наMetricInfo(open/hover/focus/dismissed, Esc, outside-click, recompute) и целиятuseFullscreenhook са без тестове — точно най-рисковият за регресии код. Нужни са компонентни тестове (напр.@testing-library/react).
Незадължителни бележки
- Един въпрос за потвърждение (cache-key): обработва ли
cacheKey()повтарящия сеcohortпо всички стойности? Ако пази само първата,?cohort=a&cohort=bи?cohort=aможе да се сблъскат в кеша (CWE-349). - Кросбраузър: fullscreen (
useFullscreenи:fullscreenCSS) използва само стандартния API без webkit-префикс — тихо no-op на по-стари Safari. Да се потвърди дали е извън обхвата. - CSS токен/коментар:
--trend-count-ink: #5e7c8bе коментиран „darker slate", но е идентичен на--trend-count— да се коригира стойността или коментара. - CSS дублиране:
.fs-btn/.trend-fs-btnи дублираният.ov-board-scale— по-чисто с общ базов клас/сливане (функционално коректни). - Дребно CSS: липсва стандартен
line-clampдо-webkit-line-clampна места; няколко!importantподсказват проблем със специфичността. - A11y дребно:
aria-expandedсе сменя, но popover-ът еaria-hiddenбезaria-controls— няма реален разгънат регион (не блокира предвид дизайна). - Обратимост: обмислете down-миграция/бележка за индекса и проверка на частичния предикат (не само името) в теста.
- Да се потвърди, че
useFullscreenсе консумира някъде (иначе потенциален dead code).
Заключение: PR-ът е чист откъм сигурност и добре документиран. Единственият блокер е тестовото покритие на интерактивната логика (т.1); останалите бележки са козметични/незадължителни.
- add MetricInfo component tests (click toggle, Esc-on-hover close, symmetric resize/scroll listener cleanup) via @testing-library/react - remove the dead useFullscreen hook (zero consumers); keep the presentational FullscreenButton for a future caller to wire up - correct the misleading --trend-count-ink comment (byte-identical to --trend-count, unconsumed) instead of inventing a new color - lock in repeated-cohort cache-key behavior with a regression test - document why the stale-allow-list-entry check is intentionally unbounded console.info, not a hard failure - assert the idx_contracts_overrun index's actual predicate + column order, not just its presence - document 0002's forward-only migration convention instead of adding a DROP INDEX
# Conflicts: # apps/web/app/styles/components.css # apps/web/workers/cache-key.test.ts # apps/web/workers/cache-key.ts # pnpm-lock.yaml
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR — dashboard design-system base (MetricInfo, fullscreen, tokens, overrun index)
ВЕРДИКТ: COMMENT — без блокиращи проблеми; няколко бележки за уточняване преди merge.
Какво прави PR-ът
Този PR полага базата на дизайн-системата за таблото и е част от stacked серия. Обхватът включва:
- Frontend/презентационни компоненти:
FullscreenButton,MetricInfo(popover с clamp-математика), чиста функцияclampPopoverShiftс unit тестове. - Дизайн токени: нов
tokens.css(OKLCh цветове, документирано WCAG 1.4.1 съответствие — цветът не е единствен носител на смисъл). - Кеширане/маршрути: разширение на
CANONICAL_QUERY_PARAMSи нови тестове вcache-key.test.ts. - База данни: миграция
0002— частичен, покриващ индекс за overrun (WHERE annex_count > 0) с гардиращ тест. - Зависимости: нова devDependency
@testing-library/react(+ транзитивни вpnpm-lock.yaml).
Сигурност — ЧИСТО
И трите партиди преминаха Фаза 0 без блокиране: няма твърдо кодирани тайни, няма продукционни URL промени (само тестови фикстури), няма зловреден код, backdoor, обфускация или injection повърхност. Текстът се рендира като екраниран React children; миграцията е статичен DDL без динамичен вход; новите зависимости са широко разпознати dev-only пакети. Тестът за cache-key дори затваря CWE-349 (#56), предотвратявайки колизия/отравяне на кеша при повтарящи се allow-list параметри.
Силни страни
clampPopoverShiftе коректно изнесена чиста функция с добро покритие, включително граничния случай ляв+десен clamp.MetricInfoбалансира listener-ите (resize/scroll/keydown/pointerdown) с коректен cleanup — няма изтичане; SSR-безопасност чрезuseIsoLayoutEffect; rAF-throttle и синхроненshiftRefса добре обмислени.- Миграцията гардира предиката и реда на колоните на индекса; forward-only без down-миграция е в съответствие с конвенцията на репото.
Бележки (незадължителни, non-blocking)
FullscreenButtonбез consumer в тази серия файлове — по CLAUDE.md „NO DEAD CODE / NO PARTIAL IMPLEMENTATION“ това би било проблем, но е обозначен като умишлена база на stacked PR. Моля потвърдете, че consumer-ът идва в следваща партида.- a11y несъгласуваност:
aria-expandedвърху бутон, чийто popover еaria-hidden="true"и безaria-controls. - Отслабен drift-guard в
cache-key.test.ts: тестът „does not retain allow-list entries that nothing reads“ е сведен отexpect(stale).toEqual([])доconsole.info. Засяга само несигурностната посока (сигурностната остава твърдо асертирана) и е добре обоснован, но потвърдете, че е съзнателно и че over-keying на edge кеша е приемлив. - Дребно за производителност: capture-phase
scrolllistener може да еpassive: true.
Тестове
Тестовете за metric-info-clamp, MetricInfo, cache-key и миграцията са смислени (не тривиални). От предоставения diff не може да се потвърди зелен CI и покритие ≥90% за новия код — препоръчва се да се верифицира преди merge.
Заключение: код с високо качество, чист откъм сигурност. Одобрението зависи от потвърждаване на consumer-а за FullscreenButton, съзнателното отслабване на drift-guard теста, зелен CI/покритие и дребните a11y/производителностни бележки.
…cInfo (midt-bg#169) - Remove aria-expanded from the trigger button: the popover is purely visual (aria-hidden), and its text is already conveyed via aria-label, so the two were inconsistent. - Make the capture-phase window scroll listener passive (never calls preventDefault), keeping capture:true symmetric on add/remove so cleanup still detaches it. - Update MetricInfo.test.tsx's Escape-dismiss assertion to check the is-dismissed class instead of the removed aria-expanded attribute.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR — обобщение
Заглавие: feat(web): dashboard design-system base (MetricInfo, fullscreen, tokens, overrun index)
ВЕРДИКТ: COMMENT — няма блокиращи проблеми по сигурност или коректност; препоръчват се няколко уточнения преди merge.
Какво прави PR-ът
Въвежда базовия слой на dashboard design-system, прегледан на 3 партиди:
- Партида 1 — React/TS компоненти:
MetricInfo(popover с clamp математика, достъпност, SSR-безопасен),FullscreenButton,metric-info-clamp.tsи разширяване наCANONICAL_QUERY_PARAMSвquery-params.ts. - Партида 2 — презентационни стилове в
pages.css(+2372/−3):.net-*,.map-*,.az-*,.pa-*,.ov-*и native fullscreen, с внимание към достъпност (prefers-reduced-motion, WCAG бележки). - Партида 3 — токени (
tokens.css), частичен overrun индекс (SQL миграция0002), тестове за миграции и cache-key, нова devDependency@testing-library/react.
Сигурност (Phase 0) — ЧИСТО ✅
Във всичките три партиди: няма твърдо кодирани тайни, няма нови/променени URL адреси, няма инжекции (XSS/SQL), обфускация или бекдори. Всички JSX стойности се escape-ват от React; CSS файлът не изпълнява код; SQL миграцията е идемпотентна CREATE INDEX IF NOT EXISTS без injection повърхност. Единствената нова зависимост (@testing-library/react ^16.3.2, devDependency) е с очаквани транзитивни пакети и съвпадаща резолвната версия. pnpm-lock.yaml е консистентен.
Коректност и качество
metric-info-clamp.ts— математиката е вярна и покрита с тестове; съответства на CSSmax-width: min(320px, calc(100vw - 16px)).MetricInfo.tsx— внимателно управление на състояния, симетрични listener-и, rAF коалесциране, коректна достъпност (popoveraria-hidden, пълен текст вaria-label).- Миграция
0002— добре обоснован частичен индекс (WHERE annex_count > 0, колонен ред(signing_value_eur, current_value_eur)); forward-only без rollback е в съответствие с конвенцията на репото. migrations.test.ts— отличен тест, който пази самата дефиниция на индекса срещу тиха регресия.tokens.css/pages.css— чисти токен-базирани добавки с изрична WCAG 1.4.1 грижа (цветът не е единственият носител на смисъл).
Незадължителни бележки (не блокиращи)
- CSS дублиране —
.fs-btnvs.trend-fs-btn(партида 1) и двойно дефиниран.ov-board-scale(партида 2); обмислете обща база/сливане. FullscreenButtonбез call site — потвърдете, че свързването идва в следваща партида, за да не остане мъртъв код.query-params.ts— новите канонични параметри са безопасни срещу cache poisoning; единствен ефект е потенциална over-keying фрагментация. Уверете се, че drift guard-ът (cache-key.test.ts) действително покрива новите записи.- Козметика в стиловете — премахнат
outlineв.network-svg a:focus-visible(компенсиран със stroke пръстен — потвърдете контраста), смесени цветови пространства вcolor-mix(oklchvssrgb), нарастващ брой!important. - cache-key тест — бившата твърда проверка е превърната в „info“ тест, който винаги минава — гранично „cheater test“.
За потвърждение извън прегледа
- Реалното минаване на тестовете и покритието (не могат да се изпълнят тук).
- Твърдението „fully covering“ за overrun индекса зависи от
OVERRUN_WHEREвqueries/overruns.ts(друга партида). - Изричен тест за приложения
translate/shiftстил при видим popover би бил полезно допълнение.
Заключение: Чист, добре документиран design-system слой без проблеми по сигурност или данни. Забележките са за поддръжка и потвърждения — не блокират merge, но си струва да се адресират.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Ре-верифицирах на HEAD (ffd9d844): новият комит е a11y полиране (махнат stray aria-expanded + passive scroll listener в MetricInfo) — презентационно, не мени cache-key нито данните. Предишният ми преглед (cache-key/CWE-349 чист) стои. Одобрявам.
…ricInfo TypeScript's EventListenerOptions type for removeEventListener only accepts capture, not passive (passive is addEventListener-only) — was breaking CI typecheck.
…mp constant - cache-key.test.ts: the info-only "stale allow-list entries" check was wrapped in an `it()` with no assertion, so it could never fail. Runs as plain code in the describe body instead — same console.info signal, not registered as a graded test. - 0002_contracts_overrun_index.sql: the "fully covering" claim only holds for the single-pass corpus aggregate; the leaderboard and by-authority/by-sector breakdowns JOIN on c.tender_id and read further contracts columns, so they still take one table lookup per matching row. Comment corrected. - metric-info-clamp.ts: named the 8px inset as VIEWPORT_INSET_PX so it can't drift from the CSS's `calc(100vw - 16px)` (2x the inset).
The CANONICAL_QUERY_PARAMS drift guard's stale-entry check ran as plain describe-body code with no expect(), so it could never fail regardless of whether the guarded behavior broke (CLAUDE.md NO CHEATER TESTS). Re-register it as a real it() that asserts no unexpected stale entries, excluding the documented stacked-PR-ahead-of-route params (a, b, by, cohort, cpv, metric) so legitimate stacked work still doesn't block unrelated PRs.
…ed composite Replace the annex_count-only partial index with PR midt-bg#169's composite (signing_value_eur, current_value_eur) index and its EXPLAIN QUERY PLAN benchmark comment, so both PRs converge on one canonical migration instead of shipping conflicting copies of 0002_contracts_overrun_index.sql
…ter RSC CVE Bump postcss to ^8.5.18 (GHSA-r28c-9q8g-f849) and valibot to ^1.4.2 (GHSA-5qjj-4xww-7phc) via pnpm overrides - both patch-level, non-breaking fixes. Add a time-boxed osv-scanner.toml suppression for react-router's GHSA-qwww-vcr4-c8h2, a CSRF flaw scoped to unstable RSC APIs this app does not use (verified via repo-wide grep); no fix exists in the 7.x line and bumping to 8.x is out of scope for this patch.
- osv-scanner.toml: keep both independently-added suppressions (sharp GHSA-f88m-g3jw-g9cj from main, react-router GHSA-qwww-vcr4-c8h2 from this branch) - packages/db/migrations: both branches added an unrelated migration numbered 0002 (main's 0002_current_value_currency.sql, this branch's 0002_contracts_overrun_index.sql); renumbered the latter to 0003_contracts_overrun_index.sql since they are independent (no shared table/column) and updated migrations.test.ts to apply both - pnpm-lock.yaml: regenerated via pnpm install rather than hand-merging
… + 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>
|
Ъпдейт на миграционната номерация (замества по-ранната бележка за Стекът носи Значи номер (Странична бележка: euro-annex капанът вече е затворен на main чрез |
|
Този клон е в конфликт с |
4 participants
What changed
Dashboard design-system base (MetricInfo popover, fullscreen toggle, design tokens, overrun index), now through four rounds of review fixes:
MetricInfopopover clamp recomputes from the unshifted rect (idempotent across repeated resize/scroll), re-triggers on hover/focus reveal (not just click), andaria-expanded/Esc now track hover+focus visibility, not justopen.MetricInfo: removed the strayaria-expandedfrom the trigger button — the popover it referenced is purely visual andaria-hidden, and its text is already conveyed viaaria-label, so the two were inconsistent.MetricInfo: the capture-phasewindowscrolllistener never callspreventDefault, so it's now registered{ passive: true, capture: true }(with matchingcapture: trueonremoveEventListenerso cleanup still detaches it).useFullscreen(zero call sites) removed as dead code;FullscreenButtonstays purely presentational — an intentional staged design-system base whose consumer (wiringactive/onToggle+ the native Fullscreen API, plus any webkit-prefix fallback decision) lands in a follow-on stacked PR (feat(web): overruns dashboard #171,pr/overruns— seeapps/web/app/routes/overruns.tsx).lib/query-params.ts: the new canonical params (a,b,by,cohort,cpv,metric) are added ahead of their consumer routes deliberately as part of this stacked base. The security-critical direction (consumed ⊆ allow-list, CWE-349) stays hard-asserted incache-key.test.ts; only the reverse stale-entry direction is intentionally softened (see next bullet).trends-dashboard/overruns-dashboardsection-marker typo, the duplicate choropleth comment, and--trend-count-ink(no longer claims "darker" than--trend-count— they're byte-identical; the comment now says so and explains why that's acceptable).cache-key.ts) draws its response-affecting param set from the sharedCANONICAL_QUERY_PARAMSsource of truth (CWE-349, issue Web cache poisoning: /contracts edge-cache key omits a response-affecting query parameter #56); confirmed thecohortrepeatable-param case is handled (URLSearchParamsiteration preserves every occurrence, not just the first).cache-key.test.ts: the removed hard-fail invariant is now an info-onlyconsole.info(stale allow-list entries are harmless — lower cache-hit-rate only, not correctness) with a comment explaining the tradeoff. Kept asconsole.info-only by explicit decision — no threshold/periodic-review follow-up planned; the hard-asserted direction (consumed ⊆ allow-list) is the one that actually prevents cache collapse.0002_contracts_overrun_index.sql: composite covering index on(signing_value_eur, current_value_eur)(~4x faster than anannex_count-only index perEXPLAIN QUERY PLANon a 190k-row fixture). No down-migration, matching this repo's forward-only convention (0000_init.sql,0001_flow_pairs_bidder_index.sqlhave none either;wrangler d1 migrationshas no rollback primitive) — documented in a migration comment rather than fabricating aDROP INDEX.migrations.test.tsstrengthened to assert the index's actual partial predicate and column order, not just its presence.MetricInfo.test.tsxadded — covers the open/hover/focus/dismissed state machine, Esc-close, and listener cleanup on unmount.--paper-raisedusesoklch(100% 0 0)for token consistency.How it was tested
pnpm vitest run app/components/MetricInfo.test.tsx app/components/metric-info-clamp.test.ts(round 4) — green after thearia-expandedremoval and passive-listener change; the one assertion that referenced the removed attribute was updated to check the dismissed-state class instead.pnpm --filter web test— 37 files / 383 tests passed, including the newMetricInfo.test.tsxand the correctedmetric-info-clamp.test.ts"both edges clip" fixture.pnpm --filter @sigma/db test— 28 files / 199 tests passed, including the strengthenedmigrations.test.tsassertion on the overrun index's predicate/column order.tsc --noEmitclean for bothapps/webandpackages/db.Quality checks
IF NOT EXISTS).