feat(web): force-directed network graph (progressive enhancement) (#142) - #144
feat(web): force-directed network graph (progressive enhancement) (#142)#144StanislavBG wants to merge 19 commits into
Conversation
db19212 to
a4628f7
Compare
|
Follow-up: the graph capped direct counterparties at 6, and the relations table was built from the same bounded set — so a hub's real counterparties (Sofia: 310) were unreachable and the cap read as "all there is".
Local: |
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Силна работа. Data слоят е коректен (няма промяна в GROUP BY/SUM/JOIN на ego-заявката); counterpartyTotal е добавен като required в контракта и getEntityNetwork го попълва по всички пътища, така че authority.tsx/company.tsx компилират без промяна; SSR-ът е запазен както трябва (физиката е в useEffect, interactive пуска client-only markup-а след mount, seed-ът е същата детерминистична функция и на сървъра — без hydration mismatch); симулацията оперира върху копия, не върху props-овете; prefers-reduced-motion е спазен на ниво физика; d3 модулите са легитимни (^3.0.0, заключени, без изненади в lock-а). 👏
Едно нещо, което е добре да се оправи (D1 cost):
Един и същ COUNT(*) се изпълнява два пъти на зареждане на /network. getEntityNetwork вече прави SELECT COUNT(*) … FROM flow_pairs WHERE ${centerCol} = ? и връща counterpartyTotal (а в route-а data.counterpartyTotal е в обхват). После getEntityCounterparties прави абсолютно същия COUNT(*) пак, и route-ът ползва неговия total. D1 таксува сканирани редове — това е същото броене два пъти на заявка. getEntityCounterparties може да приеме вече известния total (или route-ът да ползва data.counterpartyTotal за пагинацията), и втората заявка отпада.
Дребни (LOW, не блокират):
-
getEntityCounterpartiesтества самоkind:'authority'; company-центърът (neighborCol='authority_id', друг ORDER BY) не минава през тест. -
getKey={(r) => \${r.from}-${r.to}`}` ползва етикети, не ID-та — два различни (възложител, фирма) с еднакви имена след де-брандиране ще дадат key collision (React warning, не грешка в данните).
Mergeable, без блокери. Но горните подобрения са добре дошли.
…y slug Review feedback on midt-bg#144: - The same COUNT(*) over flow_pairs ran twice per /network load (getEntityNetwork for counterpartyTotal, then getEntityCounterparties for its total). D1 bills per row scanned, so it was the same count paid twice. getEntityCounterparties now takes an optional total; the route passes data.counterpartyTotal and the second scan is gone (standalone callers still count once, in parallel). - /network table getKey now keys on the slug hrefs, not display labels (two de-branded same-name entities could collide). - test the company-centre counterparties path (authority_id keyset + ORDER BY) and the caller-supplied-total shortcut.
|
Review notes addressed in
This PR also contains #140's commits and replaces its table with the shared linked |
…tabIndex (review) Review feedback on midt-bg#144: - **perf** — a client re-centre (`fetcher.load`) re-ran the whole loader and threw away the centre-picker options + the counterparties page. The re-centre URL now carries `g=1`; the loader skips both for graph-only fetches, so a node click no longer pays for queries nothing reads. - **correctness** — bind the counterparties cursor to the centre (`filterSignature({ center })`): a cursor minted for centre A is otherwise structurally valid on B, so a shared/edited `?center=B&cursor=<from A>` would mispaginate; now it decodes to null → page 1. - **a11y** — graph node <a>s get `tabIndex={-1}`: a role=img SVG must not hold interactive descendants, and the relations table is the keyboard/AT path. - tests: forward→backward keyset round-trip (before/reverse) and the cross-centre cursor reset. Caption link uses the raw token, matching recentre().
|
@StanislavBG — също дребно по линкването: PR-ът реализира #142 (force-directed граф като прогресивно подобрение), но го споменава като проза, без closing keyword. Би ли добавил ред |
|
Много добра работа, а историята със сигурността и целостта на данните издържа при внимателен преглед. 👏 Издърпах клона и проверих локално:
Едно нещо за оправяне преди merge: клонът в момента е в конфликт с Дребно, не блокира: добавянето на След rebase-а с удоволствие ще одобря. Благодаря за внимателното описание и покритието с тестове. |
…y slug Review feedback on midt-bg#144: - The same COUNT(*) over flow_pairs ran twice per /network load (getEntityNetwork for counterpartyTotal, then getEntityCounterparties for its total). D1 bills per row scanned, so it was the same count paid twice. getEntityCounterparties now takes an optional total; the route passes data.counterpartyTotal and the second scan is gone (standalone callers still count once, in parallel). - /network table getKey now keys on the slug hrefs, not display labels (two de-branded same-name entities could collide). - test the company-centre counterparties path (authority_id keyset + ORDER BY) and the caller-supplied-total shortcut.
…tabIndex (review) Review feedback on midt-bg#144: - **perf** — a client re-centre (`fetcher.load`) re-ran the whole loader and threw away the centre-picker options + the counterparties page. The re-centre URL now carries `g=1`; the loader skips both for graph-only fetches, so a node click no longer pays for queries nothing reads. - **correctness** — bind the counterparties cursor to the centre (`filterSignature({ center })`): a cursor minted for centre A is otherwise structurally valid on B, so a shared/edited `?center=B&cursor=<from A>` would mispaginate; now it decodes to null → page 1. - **a11y** — graph node <a>s get `tabIndex={-1}`: a role=img SVG must not hold interactive descendants, and the relations table is the keyboard/AT path. - tests: forward→backward keyset round-trip (before/reverse) and the cross-centre cursor reset. Caption link uses the raw token, matching recentre().
bcb4934 to
9ec970f
Compare
|
Rebased onto the latest Conflicts resolved:
Verification on the rebased head: Also added |
|
След чистия rebase ( |
|
Провери всичко необходимо. Изготвям финалния преглед. Преглед на PR #144 — Force-directed граф като прогресивно подобрение (
|
…collision-guarded Upgrades the static ego-graph into an explorer (progressive enhancement, URL stays static): - each edge shows its € value, with a pure-CSS on/off toggle (no JS); - with JS, clicking a node re-centres the graph in place — fetches that node's own ego-network via the /network loader (useFetcher) and redraws without changing the URL; 'Отвори профила' opens the focused node's page, 'Върни се в началото' resets to the URL's entity; without JS a node click falls back to opening its profile (crawler-safe); - node labels are de-cluttered per side and edge labels biased toward the outer node so they no longer overlap each other or the hub; - the connections table's entity cells are now links — the accessible navigation path. Hardening from review: reset browse state when the page's data changes; an intent ref so Reset cancels an in-flight load; only adopt valid (>=2-node) results; inline error + move focus to Reset after a re-centre.
… with midt-bg#124 Addresses review on midt-bg#140: - Extract the a:/c: `?center` grammar (centerToken + parseCenter) into one shared module apps/web/app/lib/network-center.ts so the link and loader halves can't drift; add network-center.test.ts (round-trip per node kind incl. name-keyed base64url slug, malformed-token rejection, isAdoptableNetwork). Follows the repo's pure-logic lib/*.test.ts convention. - Drop the duplicated inline grammar in network.tsx (dropdown centerValue) and NetworkGraph.tsx (centerToken). - Centre node renders no text label (already named in title/dropdown/legend), aligning with midt-bg#124 to avoid a render-block conflict.
…dt-bg#142) On hydration the static SSR ego-graph animates via d3-force, with d3-drag to move nodes, d3-zoom pan/zoom, discoverable +/↺/− zoom controls and a full-screen (⤢) toggle. SSR and the first client render keep the deterministic radial layout (no hydration mismatch); the sim is bounded (alpha decay → settles, no infinite RAF), re-inits on the midt-bg#140 browse re-centre, and pins the centre. Hovering a node fills a side Information Card (like /map) and emphasises that node + its neighbours, dimming the rest; edges show the € value plus the contract count ('дог.') when >1. Reduced-motion settles once without animation; a drag still repaints. The connections table stays the keyboard/AT path: /network now reuses the shared networkColumns/networkRows from lib/entity-tables (dropping the duplicated inline table), which also links its cells to each entity's profile. Pure geometry/physics in lib/network-layout.ts (+tests); d3 lifecycle in lib/useForceGraph.ts. Deps: d3-force/-drag/-zoom (+ d3-selection, their required .call() companion).
The network graph capped direct counterparties at HOP1 (6) for readability, but the /network relations table was built from the same bounded edge set — so a big hub's real counterparties (e.g. Sofia: 310) were unreachable and the cap read as "this is all there is". That is a silent truncation. - packages/db: add getEntityCounterparties — a keyset-paginated, exhaustive list of the centre's direct counterparties (same cursor scheme as the contracts list); add counterpartyTotal (COUNT(*)) to getEntityNetwork. - web /network: render an exhaustive, paginated „Всички преки контрагенти (N)" table below the graph; the in-graph „Връзки в графа" table stays as the literal picture (hop-1 + hop-2 edges). - NetworkGraph: caption names the cap („показва N от общо M преки контрагента") with a link to the full list, so the truncation is never silent — on every page the graph appears (entity profiles + /network). - also keep steep edge value-labels horizontal instead of rotating them bottom-to-top against the viewBox. - tests for the new query, the cursor, and counterpartyTotal.
The hover card's „Връзки в графа" used the in-graph degree, which for the centre is the HOP1 cap (6) — misleading next to the full list of 310. Reuse the counterpartyTotal already on NetworkData: for the centre show „Преки контрагенти" = the true total; other nodes keep the in-graph degree (their real degree is unknown in this bounded ego view).
…t graph-edge copy /network had two tables: „Връзки в графа" (just re-listed the drawn edges) and the full paginated counterparty list. That second table was confusing and the edge copy was redundant with the graph itself. Collapse to a single, full, paginated „Всички връзки (N)" table — the exhaustive connection list — and use „връзки" wording throughout (graph caption + centre info card) instead of „контрагенти". Entity profile pages keep their compact in-graph table plus the „Виж всички" link into /network#links.
…y slug Review feedback on midt-bg#144: - The same COUNT(*) over flow_pairs ran twice per /network load (getEntityNetwork for counterpartyTotal, then getEntityCounterparties for its total). D1 bills per row scanned, so it was the same count paid twice. getEntityCounterparties now takes an optional total; the route passes data.counterpartyTotal and the second scan is gone (standalone callers still count once, in parallel). - /network table getKey now keys on the slug hrefs, not display labels (two de-branded same-name entities could collide). - test the company-centre counterparties path (authority_id keyset + ORDER BY) and the caller-supplied-total shortcut.
…tabIndex (review) Review feedback on midt-bg#144: - **perf** — a client re-centre (`fetcher.load`) re-ran the whole loader and threw away the centre-picker options + the counterparties page. The re-centre URL now carries `g=1`; the loader skips both for graph-only fetches, so a node click no longer pays for queries nothing reads. - **correctness** — bind the counterparties cursor to the centre (`filterSignature({ center })`): a cursor minted for centre A is otherwise structurally valid on B, so a shared/edited `?center=B&cursor=<from A>` would mispaginate; now it decodes to null → page 1. - **a11y** — graph node <a>s get `tabIndex={-1}`: a role=img SVG must not hold interactive descendants, and the relations table is the keyboard/AT path. - tests: forward→backward keyset round-trip (before/reverse) and the cross-centre cursor reset. Caption link uses the raw token, matching recentre().
413af0b to
67c7b40
Compare
|
Rebase-нат върху новия main (linear); 232-те CSS реда са преместени в styles/pages.css при съществуващите .network-/.net- правила, app.css остава само @imports; prettier-чист, lint/typecheck 7/7, 360 web теста зелени, frozen install и audit чисти. |
Преглед на PR #144 — Force-directed граф като прогресивно подобрение (
|
|
@todorkolev готов за ревю 🙏 — rebase-нат на main, CI зелен, prettier-чист, CSS промените в styles/* (app.css само @import). Резолвнати нишки. Approve-ни когато ти е удобно. |
|
Одобрявам. Mobile — една бележка за втори поглед на 360px: |
.net-zoom buttons were 30px with only a 4px gap between them — below the touch floor, and too tight for the invisible ::after overlay trick (the two hit areas would overlap). Grow them directly and widen the gap to match.
|
Проверих |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR — feat(web): force-directed network graph (progressive enhancement) (#142)
Какво прави PR-ът
Добавя force-directed мрежова графика с progressive enhancement към уеб приложението. SSR рендира статична графика на база детерминистичен seed, а d3-физиката (drag/zoom, симулация) стартира едва след hydration в post-mount ефект — без hydration mismatch. Zoom контролите се появяват само след hydration, а таблицата остава каноничната клавиатурна пътека. На бекенда се добавя нова заявка getEntityCounterparties (keyset пагинация) и COUNT(*) за общия брой контрагенти, заедно с нови интерфейси в api-contract (NetworkCounterparty, NetworkCounterpartyPage, counterpartyTotal). Изнесени са чисти помощници (network-center.ts, network-layout.ts) с добро unit покритие.
Обща оценка
Промяната е чиста, добре структурирана и добре документирана. Progressive enhancement е коректно реализиран, ресурсите (fullscreen listener, d3 sim, drag, zoom) се почистват коректно, prefers-reduced-motion се уважава, а race условията при re-centre са обмислени (pending ref). Сигурност: чисто — няма тайни, нови URL адреси или злонамерен код; новите d3 зависимости са утвърдени и фиксирани в lock файла, @types/* са само в devDependencies. Тестовото покритие на новата loader/query логика е смислено и нетривиално (пагинация, курсори, company-център, отхвърляне на чужд курсор).
Най-важни забележки (не блокиращи)
- Коректност — броене на директни връзки:
edges.filter((e) => e.from === center.id)приема, че всички ръбове до центъра са ориентираниfrom = center, но ориентацията в топологията е произволна (затова старатаnetworkRowsнормализира authority→company). Ако центърът е фирма и ръбовете саauthority → center, броят става 0 и надписът „показва 0 от общо N" е грешен. Препоръка: бройте по двата края. - Коректност — резервен total:
totalRow?.n ?? hop1.lengthподменя реалния брой с изчертания cap (≤6) при липсващ/проваленCOUNT, което би показало грешно „top N of M". - Производителност:
getEntityNetworkвече винаги пуска допълнителенCOUNT(*)при всяко зареждане (паралелизиран). Тъй като D1 таксува на прочетен ред, потвърдете, чеflow_pairs.authority_id/bidder_idса индексирани. - Дребни: при
!centerрано се връщаcounterpartyTotal: 0, въпреки че стойността вече е изчислена (може да се преизползва);role="img"SVG съдържа интерактивни<a>наследници (ARIA несъответствие), но е смекчено сtabIndex={-1}и документирано като приемливо.
Блокиращи проблеми
Няма блокиращи проблеми по сигурността. Логиката и тестовете изглеждат солидни. Препоръчва се да се адресират бележки 1 и 2 (коректност на броенето/total), а ако тестовете за самия loader на network.tsx (пагинация, g=1 разклонение, 404 при несъществуващ център) липсват, покритието да се допълни.
…total honest Edge orientation in flow_pairs is arbitrary, so directShown (and the /network relations hint) undercounted when direct edges pointed neighbour→centre. Extract a shared countDirectEdges helper used by both NetworkGraph and the /network route so they can't diverge again. Also stop masking a failed COUNT(*) as the HOP1 draw cap: getEntityNetwork now returns counterpartyTotal: null on failure instead of hop1.length, and getEntityCounterparties only reuses a passed total when it's a real number, re-querying otherwise so a failure is never silently reported as a small M. Confirmed flow_pairs.authority_id / bidder_id are both covering-index seeks (EXPLAIN QUERY PLAN), not full scans, for the per-load COUNT(*).
|
Addressed in 80035b2:
Typecheck + tests green for |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Обобщено ревю на PR #142 — feat(web): force-directed мрежов граф (progressive enhancement)
Какво прави PR-ът
Добавя интерактивен force-directed мрежов граф на страницата /network, реализиран чрез прогресивно подобрение: SSR/no-JS вариант с <a> fallback, а физиката (d3) стартира едва след mount, за да няма hydration mismatch. Архитектурата е чисто разделена — network-center (граматика на ?center), network-layout (чиста геометрия/физика) и useForceGraph (жизнен цикъл на d3). Към графа е добавена и изчерпателна, keyset-пагинирана таблица с контрагенти, преизползваща съществуващите компоненти (DataTable, Pagination, Section). Добавени зависимости: d3-drag, d3-force, d3-selection, d3-zoom (^3.0.0) + @types/*.
Обща оценка
Промяната е с високо качество, атомарна и изцяло в обхвата на #142 — без scope creep, без мъртъв код и без частични TODO-реализации. Тестовете са смислени и реално разкриват дефекти (round-trip на граматиката, детерминизъм, немутиране на входа, пълен count vs. drawn cap, null при провал на COUNT, нормализация authority→company, next/prev курсори, обвързване на курсора към центъра).
Сигурност (Фаза 0 — CLEAN)
- Няма твърдо кодирани тайни.
- SQL инжекция: суровият SQL в
getEntityCounterpartiesинтерполира само фиксирани литерали (authority_id/bidder_idот булев тернар) и allowlist-нати имена на колони (regexIDENTIFIER); всички стойности са bound параметри. Курсорът е ограничен по дължина (MAX_CURSOR_CHARS = 512) и обвързан с центъра чрезfilterSignature— форжнат курсор се нулира до страница 1. Няма инжекционна повърхност. ✅ - Без XSS: няма
dangerouslySetInnerHTML; всички етикети минават през React escaping. - Зависимости: d3-* са легитимни, широко използвани пакети;
pnpm-lock.yamlсъдържа коректни integrity хешове. - OWASP A01/A03/A06 — без установени рискове (всички данни са публични).
Находки (незадължителни, НЕ блокиращи)
- Непоследователност при провал на
COUNT(*): вgetEntityNetworkпровалът даваnull(„неизвестно"), а вgetEntityCounterpartiesсе свежда доtotal: totalRow?.n ?? 0(валидна нула). Приемливо предвидNetworkCounterpartyPage.total: number, но е добре да е съзнателно решение. - Slug в URL без
encodeURIComponent(centerToken/recentre): днес безопасно (граматиката е URL-safe и тествана), но е крехък инвариант. - Козметично: bias
t = 0.62на етикета на реброто приема, чеtoе винаги по-периферният край; при обратно ориентирано ребро етикетът може визуално да клони към грешния край. - За потвърждение: при смяна на центъра през dropdown-а на
/networkда се провери, чеcursorот URL се нулира, за да не се ползва stale cursor за първата страница на новия списък. - Формулировката „N от общо —" при
counterpartyTotal === null(деградира меко, без NaN/crash).
Блокиращи концерни
Няма. Сигурността, целостта на данните и коректността са потвърдени в двете партиди. Единствената препоръка преди merge е да се потвърди зелен CI и покритие (≥90%), тъй като това не може да се верифицира само от дифа.
ВЕРДИКТ: COMMENT — сигурна, качествена промяна, готова за merge; остават само дребни незадължителни бележки и потвърждение на CI.
Defense-in-depth per review: centerToken now percent-encodes the entity slug before embedding it in the `?center=` token, and parseCenter decodes it and falls back to null on a malformed %-escape instead of throwing. Also clarify the truncation caption when counterpartyTotal is unknown (COUNT failed) so it reads unambiguously instead of a bare em dash.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR #142 — force-directed мрежова графика (прогресивно подобрение)
Какво прави PR-ът
Добавя интерактивна force-directed мрежова графика като прогресивно подобрение над съществуващия SSR/без-JS изглед. Без JavaScript графиката остава напълно функционална: същите статични позиции, <a> fallback-и и таблица с връзки за клавиатура/помощни технологии. Физиката (d3-force/drag/zoom) стартира едва в ефект след mount, така че няма hydration mismatch. Чистата геометрия/физика е изнесена в network-layout.ts (с детерминистични unit тестове), а ?center граматиката — в network-center.ts с round-trip тестове. От страна на данните е добавено COUNT(*) в getEntityNetwork и нов помощник getEntityCounterparties с keyset пагинация, като семантиката „null = неизвестно“ е внимателно запазена.
Обща оценка
Висококачествена, добре документирана и добре тествана промяна. Фокусирана върху issue #142 без scope creep; без мъртъв код, TODO-та или дублиране; консистентно именуване и чисто разделяне SSR/клиент.
Сигурност (ЧИСТА)
Няма открити проблеми в двете партиди:
- SQL инжекция: Няма. Имената на колоните са фиксирани в кода и се извеждат само от
p.kind, никога от вход на потребителя. Всички стойности минават през bound параметри. Помощникътkeyset()валидира колоните срещу regex + allowlist (authority_id,bidder_id). - DoS/ресурси: Курсорът е ограничен до 512 символа преди
atob/JSON.parse;LIMIT/HOP1/HOP2_SCANкапват сканирането. - Курсорна цялостност: Курсорът е връзан към конкретния център и се нулира при смяна на центъра (покрито с тест). Токенът нарочно не е HMAC — всички данни са публични.
- XSS: Данните се рендерират през React (екранирани), без
dangerouslySetInnerHTML.parseCenterобвиваdecodeURIComponentв try/catch и отхвърля неправилни токени. - Зависимости: Добавени
d3-drag,d3-force,d3-selection,d3-zoom(^3) +@types— утвърдени пакети;pnpm-lock.yamlе консистентен, без закачени уязвими версии.
Основни находки (неблокиращи, заслужават внимание)
useForceGraph.ts— link дистанция vs. ориентация на ребрата.linkDistance((l.target).hop)приема, чеtargetвинаги е по-периферният възел, но ребрата не са гарантирано ориентирани център→периферия (нормализацията authority→company не пипаdata.edges). За hop-2 ребро, съхранено като hop2→hop1, това дава грешна дължина.NetworkGraph.tsx— защита приcounterpartyTotal === null. В картата за център се рендерираcount(hoveredRelations), къдетоhoveredRelationsможе да еnullпри неуспешен COUNT. Да се потвърди, чеcountприемаnumber | null, или да се подаде fallback.getEntityCounterparties— маскиране на провал. При самостоятелно извикване връщаtotal: totalRow?.n ?? 0— ако COUNT се провали, това представя провала като „0 контрагента“, което противоречи на грижливо запазената null=„неизвестно“ семантика вgetEntityNetwork. Съзнателен избор (типът еnumber), но си струва да се отбележи.network-center.ts— асиметрична валидация. Заc:slug има null-guard презbidderIdFromSlug, но заa:id-то се приема безусловно. Поведението е същото като преди (само преместено), но заслужава изравняване.
Тестово покритие
Чистите helper-и (layout, center round-trip) и логиката в слоя с данни са отлично покрити (null vs. cap, провал на COUNT, център-компания, напред/назад през keyset, отхвърляне на чужд курсор). Основната липса: интерактивната логика в useForceGraph.ts (sim lifecycle, drag/zoom, reduced-motion) и разклоненията в NetworkGraph.tsx (adopt/reset/hover/fullscreen) нямат тестове. При праг ≥90% за нов код препоръчвам jsdom + testing-library тестове поне за adopt/reset и re-centre потока.
Дребни наблюдения
- CSS
transition: opacityпо време на симулация може да добави пребоядисване при много възли, но е задprefers-reduced-motion: no-preference— приемливо. - Новото
COUNT(*)добавя по една заявка на зареждане; паралелизирано е, но маршрутът може да подадеtotalкъмgetEntityCounterparties, за да избегне втори скан.
Присъда: COMMENT
Кодът е с високо качество, без проблеми по сигурността, SQL или целостта на данните. Преди APPROVE препоръчвам да се затвори липсата в тестовото покритие на хука/компонента и да се потвърдят находки 1 и 2.
- lib/useForceGraph: linkDistance now reads Math.max(source.hop, target.hop) instead of target.hop alone, since edges aren't normalised by direction and target isn't guaranteed to be the more-peripheral end (extracted as a pure, tested linkHop helper) - lib/network-center + db/queries/identity: a: authority slugs now validate against the EIK shape (isValidEikSlug) before acceptance, same as c: company slugs already do via bidderIdFromSlug - db/queries/network (getEntityCounterparties) + api-contract: preserve total: null on a failed COUNT(*) instead of coalescing to 0, matching getEntityNetwork's counterpartyTotal convention - routes/network + lib/filters + components/Pagination: handle a null counterparty total the same way NetworkGraph already handles a null counterpartyTotal (no fabricated count in the section title/hint); widen pageNav/PageNav to accept/report an unknown total and gate Next on the cursor alone rather than fabricating a page bound from a rows.length floor
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR #142 — feat(web): force-directed мрежова графика (прогресивно подобрение)
ВЕРДИКТ: APPROVE с коментари — няма блокиращи проблеми.
Какво прави PR-ът
Въвежда force-directed мрежова графика с прогресивно подобрение: чиста геометрия/физика (network-layout.ts), изолиран d3 lifecycle (useForceGraph.ts) и единствен източник на ?center граматиката (network-center.ts), плюс keyset-базирана пагинация на контрагентите в слоя @sigma/db. Добавят се зависимостите d3-force, d3-drag, d3-selection, d3-zoom (^3.0.0) със съответните @types/*.
Сигурност — ЧИСТО
- Тайни/URL-и/зловреден код: няма hardcoded креденшъли, няма нови външни URL-и (само относителни вътрешни пътища), няма
eval/dangerouslySetInnerHTML/обфускация или backdoor-и. - SQL инжекция (OWASP A03) — безопасно. Входът е стриктно валидиран преди заявка (
parseCenter+isValidEikSlug/bidderIdFromSlug, невалиден токен →null). В@sigma/dbвсички стойности се подават като bound параметри (.bind(...)); имената на колони са твърдо зададени литерали, аkeyset()валидираsortCol/idColчрез regex и явен allowlist. Никакъв интерполиран вход не достига до SQL. Това затваря и двата gate-а, отворени в партида 1. - Устойчивост на курсора: защита срещу DoS (
MAX_CURSOR_CHARS = 512предиatob/JSON.parse); курсорът е обвързан с конкретния център, така че курсор от център A не може да странира погрешно център B. - Зависимости (OWASP A06): новите d3 пакети са канонични, пиннати и заключени в
pnpm-lock.yamlс integrity хешове.
Цялост на данните
- Обработката на неизвестен total (
null) спрямо реална0е обмислена и последователна между графа, таблицата и DB слоя — отказ наCOUNT(*)се запазва като „неизвестно“, а не се маскира като нула. Добре документирано и тествано. countDirectEdgesброи ребра независимо от ориентацията; ключовете в таблицата са по slug (fromHref-toHref), не по етикет — без колизии при де-брандирани имена.- Детерминираният
seedPositionsе чист и тестван байт-за-байт → без hydration mismatch.
Производителност
COUNT(*) и страницата с редове вървят паралелно (Promise.all); дублиращо сканиране на D1 се избягва чрез подаване на вече изчисления total. Keyset пагинацията е O(1) на всяка дълбочина.
Достъпност / CSS
Внимателна работа: 44px touch мишени при pointer: coarse, focus-visible контури, семантичен dl, съобразяване с prefers-reduced-motion. Cleanup спира симулацията и маха всички слушатели — без ресурсни течове.
Незадължителни бележки (неблокиращи)
- Reduced-motion не е реактивен:
prefers-reduced-motionсе чете еднократно при init; смяна по време на сесия не се отразява до re-init. - Фокус при „Върни се в началото“: при
reset()бутонът се демонтира и фокусът пада към<body>вместо към смислен елемент (напр. заглавието на секцията). - Пре-render на цялата графа за всеки tick: приемливо за ~15 възела (и коментирано в кода); при значително по-големи графи би било кандидат за императивно писане по DOM.
network.ts: малка бележка относно връщането наcounterpartyTotal: 0в клонаif (!center)— виж inline коментара.- Тестово покритие: чистата логика е добре покрита; d3 DOM-lifecycle-ът (drag/zoom/reduced-motion) остава непокрит от unit тестове — заслужава поне интеграционен/e2e тест за drag→no-navigate и re-centre поведението.
Заключение
Висококачествен, добре коментиран и добре тестван код с ясно разделение на слоеве. Няма блокиращи проблеми по сигурност или цялост на данните. Препоръка: APPROVE.
- centerToken now handles authority/company as explicit cases and throws on any other kind instead of silently minting a company token - useForceGraph now subscribes to prefers-reduced-motion change events so a mid-session flip is honoured, reusing SiteHeader's matchMedia pattern - getEntityNetwork's centre-load-failure early exit preserves the already-computed counterpartyTotal (incl. null) instead of fabricating 0
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR #142 — batch 2/2 (feat(web): force-directed network graph)
Прегледани файлове: apps/web/app/routes/network.tsx, apps/web/app/styles/pages.css, apps/web/package.json, packages/api-contract/src/index.ts, packages/db/src/queries/identity.ts, packages/db/src/queries/network.test.ts, packages/db/src/queries/network.ts, pnpm-lock.yaml.
Забележка: това е чернова за Ваша проверка преди изпращане. Не са публикувани inline коментари (по Ваше указание). Verdict-ът е COMMENT — нито блокирам, нито автоматично одобрявам, тъй като покритието на тестовете и CI не могат да бъдат потвърдени локално.
Фаза 0 — Задължителен security скан (SQL/OWASP)
РЕЗУЛТАТ: ЧИСТ (CLEAN) — без блокиращи находки.
- SQL инжекция (проверено срещу зависимостта
packages/db/src/queries/keyset.ts): Новата заявка вgetEntityCounterpartiesконструира SQL чрез интерполация само на имена на колони (won_eur,authority_id/bidder_id). Тези имена:- идват от булев флаг
isAuth, а не от вход на потребителя; - минават през
assertSafeColumnсIDENTIFIERregex + явен allowlistallowedIdCols: ['authority_id','bidder_id']вkeyset().
Всички стойности (p.id, cursor параметри,pageSize+1) са параметризирани през.bind(...). Няма възможност за SQL инжекция.
- идват от булев флаг
- Обработка на cursor (OWASP: недоверен вход):
decodeCursorотхвърля cursor >MAX_CURSOR_CHARS(512) предиatob/JSON.parse(защита срещу изчерпване на памет), и обвързва cursor-а към sort/filter подпис. Cursor, издаден за центърauth:C, декодира доnullпри центърauth:X→ връща се на страница 1 вместо грешно странициране. Тестът „drops a cursor minted for a different centre" го покрива. - Секрети: няма hardcoded ключове/пароли/токени.
- URL промени: няма.
- Зловреден код / backdoors / обфускация: няма.
- Зависимости: добавени са
d3-drag,d3-force,d3-selection,d3-zoom(^3.0.0) +@types/*. Това са каноничните официални D3 пакети;pnpm-lock.yamlсъдържа валидни integrity хешове и коректно дърво на транзитивните зависимости (d3-dispatch,d3-quadtree,d3-timerи т.н.). Обосновани от изискването за force-directed граф. Одобрени.
Коректност и цялост на данните
Реализацията е издържана и добре обмислена по отношение на цялостта на данните:
- Разграничаване на „неизвестно" от „нула":
counterpartyTotal/totalоставатnull, когато самиятCOUNT(*)се провали (D1 грешка), вместо да бъдат подменени сHOP1cap-а или с фалшива 0. Конвенцията е консистентна междуNetworkData.counterpartyTotal,NetworkCounterpartyPage.totalи UI-я (cpTotal === null→ отделен текст). Отлично — избягва подвеждащ етикет „top N of M". - Преизползване на COUNT: маршрутът
/networkподава вече изчисления брой (total: data.counterpartyTotal) къмgetEntityCounterparties, спестявайки втори идентичен scan (D1 таксува на прочетен ред). Логиката коректно прибягва до собственCOUNT(*), когато е подаденоnull(провал), а не се доверява на липсваща стойност — покрито от тест. - Нормализация авторитет → фирма: редовете винаги се нормализират независимо от вида на центъра; keyset tiebreak-ът е на правилната колона (
bidder_idза авторитетен център,authority_idза фирмен). И двата пътя вече са тествани (предишният преглед е бил прав, че фирменият път е бил непокрит). - Ключ на React ред:
getKeyе на slug href-овете, не на етикетите — коментарът правилно отбелязва, че две различни същности могат да споделят де-брандирано име.
Не открих логически дефекти, изтичане на ресурси или race conditions в предоставения diff.
Тестове
Силна страна на PR-а. network.test.ts добавя целенасочени тестове, които разкриват дефекти, а не тривиално минават: fake D1, ключиран по SQL маркери, покрива провал на COUNT, провал при зареждане на центъра (запазване на реалния total), легитимна нула, forward/backward keyset разходка, преизползване на подаден total и отхвърляне на cursor от друг център. Няма „cheater" тестове. (Точното % покритие не може да се потвърди локално — моля, проверете CI гейта ≥90%.)
Производителност
- Заявките за редове и COUNT вървят паралелно (
Promise.all). - Keyset пагинация (O(1) на всяка дълбочина) вместо OFFSET.
g=1пътят пропуска picker опциите и таблицата при client re-centre — избягва заплащане за отхвърлени резултати.touch-action: noneпредава touch жестовете на d3-zoom. Няма регресии.
Документация / стил
Изчерпателни, точни коментари в контракта и заявките. CSS е консистентен с проектните променливи (var(--accent) и т.н.). Дребна забележка (не блокира): :fullscreen е без -webkit- префикс — приемливо предвид progressive-enhancement подхода.
CLAUDE.md съответствие
Без частична имплементация, без TODO, без дублиране, без мъртъв код, без смесени отговорности, без изтичане на ресурси. Промяната е атомарна и в обхвата на тикета (#142).
VERDICT: COMMENT — няма блокиращи или security находки; препоръчвам одобрение след потвърждаване на CI покритието (≥90%) и зеления status на checks.
# Conflicts: # pnpm-lock.yaml
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.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR #142 — force-directed мрежов граф (progressive enhancement)
Какво прави PR-ът
PR добавя интерактивен force-directed мрежов граф на страницата /network с progressive enhancement: сървърът рендира детерминистичен статичен SVG (без JS), а d3 симулацията (d3-force/drag/selection/zoom@^3) стартира едва след hydration на клиента. Отговорностите са чисто разделени между чиста геометрия/физика (network-layout.ts), жизнен цикъл на d3 (useForceGraph.ts) и презентация (NetworkGraph.tsx), с един източник на истина за layout-а между SSR и клиента, което предотвратява hydration mismatch. Добавени са също параметризирани заявки към D1 (network.ts), keyset пагинация, ?center ре-центриране и таблица с връзки като каноничен клавиатурен/AT път.
Сигурност — ЧИСТО
И двете партиди преминаха задължителния скенер за сигурност без блокиращи находки:
- Няма зашити тайни, креденшъли или токени; няма нови/външни URL адреси.
- Няма злонамерени шаблони (
eval,Function,dangerouslySetInnerHTML,innerHTML, динамиченimport(), обфускация); целият изход минава през JSX escaping на React. - SQL заявките към D1 са параметризирани (
.bind(...)); интерполират се само вътрешни константи, ограничени чрез whitelist (allowedIdCols: ['authority_id','bidder_id']). Потребителският вход (?center,?cursor) не достига до текста на SQL. ?centerсе валидира стриктно (parseCenter,isValidEikSlug,bidderIdFromSlug);centerTokenизползваencodeURIComponent, а декодирането връщаnullпри повреден вход вместо да хвърля. Курсорът е обвързан към същността чрезfilterSignature, което нулира чужди курсори до страница 1.- Новите d3 зависимости са легитимни; integrity хешовете в
pnpm-lock.yamlсъвпадат с публичните (препоръка: да се потвърди присъствие в allowlist на монорепото).
Силни страни
- Целостност на данните: конвенцията „неуспешен COUNT →
null(неизвестно)" е приложена последователно (counterpartyTotal,total,cpTotal === null), без да се маскира като реална нула или капа. Броячът и списъкът споделят един и същWHEREфилтър. - Достъпност: таблицата с връзки остава клавиатурен/AT път; интерактивните възли са извън tab-реда; спазено
prefers-reduced-motion; touch таргети до 44px. - Ресурси: всички listener-и, абонаменти и d3 симулация се почистват коректно; редът на hooks е стабилен.
- Тестове: силно покритие на чистите помощници и заявките (round-trip токени,
nullпри неуспешен COUNT, keyset разходка напред/назад, нулиране на курсор, company-център път).
Находка за потвърждение преди merge (единствена същинска)
hoveredRelations за централния възел приема counterpartyTotal, който може да е null при неуспешен COUNT(*), и извиква count(null) без предпазната клауза, която авторите вече прилагат в надписа отдолу (counterpartyTotal === null ? … : count(counterpartyTotal)). Тази асиметрия подсказва, че count(null) не е доверен вход. Моля потвърдете, че count(null) рендира смислено (напр. „—"); в противен случай Information Card ще покаже подвеждаща/счупена стойност в ръбовия случай на провалено броене — приложете същата предпазна клауза.
Незадължителни бележки
COUNTERPARTY_PAGE_SIZE = 25вnetwork.tsслужи като default, но маршрутът винаги подаваPAGE_SIZE.network— потвърдете, че двете стойности са синхронизирани (един източник на истина).heroHrefне енкодираn.slug; безопасно днес (base64url/цифри), ноencodeURIComponentби бил защита в дълбочина, ако инвариантът за slug се промени.- Жизненият цикъл на самата симулация (drag/zoom/tick през DOM) не е покрит с интеграционен тест — приемливо предвид сложността на DOM средата.
Заключение
Силно, добре структурирано и добре документирано PR с чиста сигурност, последователна цялост на данните и добро тестово покритие. Няма блокиращи проблеми със сигурността. Преди merge: потвърдете поведението на count(null) в Information Card (партида 1). Останалите бележки са незадължителни.
Обща оценка: одобрено при потвърждение на находката count(null).
…ross-referencing comments Documents that the /network route's explicit pageSize (apps/web/app/lib/filters.ts) and the packages/db query's default pageSize must stay in sync, since packages/db cannot import from apps/web without inverting the package dependency direction.
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR #142 — „force-directed мрежова графика (progressive enhancement)"
Какво прави PR-ът
PR-ът добавя интерактивна force-directed мрежова графика към /network като прогресивно подобрение. Клиентската част (React/TypeScript) използва d3-force, d3-drag, d3-selection и d3-zoom, рендира се коректно при SSR без hydration mismatch (детерминистичен seed, споделен между SSR и симулацията), уважава reduced-motion и предлага достъпна алтернатива чрез канонична таблица с връзки. Сървърната част добавя keyset-пагинация, нормализация authority→company и повторно използване на вече изчисления COUNT(*), за да се избегне дублиран скан в D1. Тестовете са значително разширени (+232 реда) и покриват реалните разклонения — провал на COUNT, незареждащ се център, легитимна нула, next/prev обхождане и нулиране на чужд курсор.
Сигурност — ЧИСТО
И двете партиди минаха скенинг за сигурност без блокери:
- Няма SQL инжекции: всички стойности се подават през параметризирани
.bind(...); имената на колони идват от фиксиран whitelist (allowedIdCols: ['authority_id','bidder_id']), а не от вход на потребителя. - Строга валидация на входа:
parseCenterвалидира?centerтокена (isValidEikSlug/bidderIdFromSlug), отхвърля непознатиkindи малформирани токени; кръговата симетрия сcenterTokenе покрита с тестове. - Няма зашити тайни, XSS,
dangerouslySetInnerHTML,evalили обфускация. Етикетите иhref-овете минават през автоматичното екраниране на React. - Нови зависимости:
d3-*(^3.0.0) +@types/*— утвърдени пакети;pnpm-lock.yamlфиксира точни версии3.0.0с валидни integrity хешове и коректно транзитивно дърво. Няма признаци за supply-chain риск.
Интегритет на данните — добре обмислено
Разграничението между „неизвестно" (total === null при провал на COUNT(*)) и реалната нула е последователно спазено през целия стек (getEntityNetwork, getEntityCounterparties и UI надписа под графа) и е покрито с целенасочени тестове. Курсорът е обвързан с центъра чрез filterSignature, така че ръчно редактиран ?center=B&cursor=<от A> се нулира до страница 1 вместо да мис-пагинира.
Основно намиране за потвърждение (не-блокер)
Информационната карта може да подаде null на count() (NetworkGraph.tsx, блок net-card-stats): const hoveredRelations = hoveredIsCenter ? counterpartyTotal : hoveredDegree; → <dd>{count(hoveredRelations)}</dd>. Когато counterpartyTotal === null и потребителят посочи централния възел, на count() се подава null — при hover това може да доведе до runtime crash или до показване на фалшива стойност (напр. „0"), което противоречи на собствения принцип на PR-а „никога не твърди пълнота". За разлика от надписа под графа, картата не обработва null явно.
Препоръка: обработете null явно в картата (напр. „—"/„неизвестно"), симетрично с логиката на надписа. Моля потвърдете дали g=1 (client re-centre) връща counterpartyTotal, за да се прецени честотата.
Дребни бележки (незадължителни)
- UX: ако
reset()се извика по време на fetch, индикаторът „Зареждане…" остава видим до приключване на игнорирания fetch — визуално подвеждащо за кратко, но функционално безопасно (pendingкоректно отхвърля резултата). - Дублирана константа за размер на страница — приемлива предвид границите на пакетите, но носи риск от тихо разминаване.
Силни страни
Коректен ред на hooks преди early-return; чисто освобождаване на ресурси (спиране на симулацията, откачане на drag/zoom/fullscreen/reduced-motion listener-и); d3 работи върху копия и не мутира props (с тест); премислена достъпност; смислени, не-тривиални тестове с негативни входове и проверки за детерминизъм.
Заключение
Вердикт: APPROVE с една уговорка. Няма блокери по сигурността или интегритета на данните. Единственото нещо, което изисква внимание преди merge, е обработката на count(null) в информационната карта (намиране по-горе) — моля потвърдете поведението/честотата и добавете явна обработка на null. Merge при условие че CI е зелен (pnpm test, типова проверка и gh pr checks).
…_PAGE_SIZE Exports COUNTERPARTY_PAGE_SIZE from @sigma/db and asserts equality with apps/web/app/lib/filters.ts PAGE_SIZE.network, so the two values (bound only by cross-referencing comments since bcf45b3) can't silently diverge.
…ly CSRF - pin postcss ^8.5.18 (GHSA-r28c-9q8g-f849, path traversal via sourceMappingURL auto-load), patch-level, fixes it for real - pin valibot ^1.4.2 (GHSA-5qjj-4xww-7phc, flatten() crash on inherited-property keys), patch-level, fixes it for real - add osv-scanner.toml suppression for GHSA-qwww-vcr4-c8h2 (react-router CSRF, unstable RSC code paths only; this app has zero RSC usage, verified via repo-wide grep); no fix exists in the 7.x line, and bumping to 8.x is an out-of-scope major version change
|
Reopening to force a fresh CI trigger — the last several pushes to this PR haven't produced any GitHub Actions run at all (verified: not queued, not pending, simply never created). |
Resolve conflicts: osv-scanner.toml (keep the superset - sharp + react-router suppressions), packages/db/src/queries/identity.ts (keep isValidEikSlug plus main's updated companySlug doc comment), apps/web/app/routes/network.tsx (keep this branch's counterparty pagination/graphOnly loader while adopting main's getDb() wrapper convention), pnpm-lock.yaml (regenerated via pnpm install)
|
d3-zoom вече филтрира wheel до Поправка: |
|
Този клон е в конфликт с |
4 participants
What changed
Force-directed network graph (progressive enhancement) plus a full round of review fixes: percent-encoded center slugs (defense-in-depth), a shared
countCounterpartyEdges/linkHophelper so direct-edge counts and force-layoutlinkDistanceare direction-independent,null-preserving COUNT-failure handling (never fabricates a count as 0), and EIK-shape validation for authority slugs matching the existing bidder-slug check.How it was tested
pnpm --filter web testandpnpm --filter db test— unit coverage for the new helpers, slug validation, and null-total propagation.EXPLAIN QUERY PLANconfirmedflow_pairslookups stay index seeks, not full scans.Quality checks