Skip to content

feat(web): productivity tools - copy citation button and print styles - #206

Open
StanislavBG wants to merge 21 commits into
midt-bg:mainfrom
StanislavBG:feat/productivity-tools
Open

feat(web): productivity tools - copy citation button and print styles#206
StanislavBG wants to merge 21 commits into
midt-bg:mainfrom
StanislavBG:feat/productivity-tools

Conversation

@StanislavBG

@StanislavBG StanislavBG commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

What changed

Productivity tools: copy-citation button and print styles. Includes review fixes across four
rounds: unmount-safe clipboard state updates, aria-label reflecting the copied state (matching
aria-live), test-mock cleanup restoring navigator.clipboard/document.execCommand between
tests, a single shared FALLBACK_ORIGIN constant instead of duplicated per-route values, the
print stylesheet's href-reveal rule scoped to external links only, buildContractCitation guarding
a null bidder (unawarded contracts), and buildCompanyCitation dropping the redundant hasEik
field (the ETL only ever populates eik when eik_valid=1, so the flag was a dead branch that
could hide a present EIK).

Latest round (2026-07-17) addressed the two remaining non-blocking ydimitrof notes:

  • CopyCitationButton.tsx: documented, with an inline comment, why the execCommand('copy')
    fallback intentionally stays inside the rejected-writeText .catch despite the user-gesture
    context sometimes being gone by then — writeText (tried first) succeeds in the overwhelming
    majority of real browsers, so the ordering is kept rather than chasing the rarer edge case.
  • citation.test.ts: dropped the stray hasEik fixture field left over from the earlier
    hasEik removal — buildCompanyCitation's type no longer declares it, so the fixtures no
    longer pass it either.

How it was tested

  • pnpm --filter web test — clipboard success + execCommand fallback paths, copied/failed
    states, unmount cleanup, and the citation-builder suite (EIK-present/absent, no-bidder
    contract).
  • pnpm --filter web typecheck.
  • Manual exercise of the copy button and a printed-page preview.

Quality checks

  • CI green on the current head commit.
  • All ydimitrof review threads verified against the current diff and resolved.

@StanislavBG

Copy link
Copy Markdown
Contributor Author

@todorkolev готов за ревю 🙏 — rebase-нат на main, CI зелен, prettier-чист, CSS промените в styles/* (app.css само @import). Резолвнати нишки. Approve-ни когато ти е удобно.

@ydimitrof

Copy link
Copy Markdown
Contributor

Ревю на PR #206 — Инструменти за продуктивност (copy-citation бутон + print стилове)

Здравейте 👋 Благодаря за чистия rebase на #157 — историята е подредена, CI е зелен, а разделянето на CSS в styles/* (chrome.css + base.css вместо директно в app.css) е точно спазено съгласно #84.

Направих задълбочен преглед с акцент върху сигурност, целостта на данните и OWASP. Обобщение по-долу.

Сигурност и OWASP — чисто ✅

  • Няма SQL повърхност. Промяната е изцяло frontend — три нови/променени route файла, един компонент, един lib модул и CSS. Нула заявки към базата, нула конкатенация на SQL. Няма нова attack surface за инжекции.
  • Няма XSS. Никъде няма dangerouslySetInnerHTML; целият текст минава през стандартното escape-ване на React. Цитатът, който отива в клипборда, е plain text (.join('\n')), а не HTML — не се интерпретира като разметка.
  • origin е безопасен. Взима се от root loader-а (url.origin, изведен от заявката, SSR-safe), с hardcoded резервен https://sigma.midt.bg. Няма user-controlled URL, следователно няма SSRF/open-redirect риск — стойността се използва само за построяване на display текст.
  • Clipboard API-то е защитено правилно. Guard-ът typeof navigator !== 'undefined' && navigator.clipboard покрива SSR и несигурен контекст; грешките се хващат в .catch. Таймерът за „Копирано!" се почиства при unmount (useEffect cleanup + useRef) — няма resource leak, няма setState върху размонтиран компонент.
  • Достъпност: налични са aria-label, title и aria-live="polite" — добре.

Цялост на данните — проверено ✅

Съпоставих сигнатурите на билдърите с реалните типове и всичко съвпада: ContractDetail (subject, authority.name, bidder.displayName, value.currentEur, id), CompanyDetail (displayName, eik, hasEik, wonEur, contracts, slug) и AuthorityDetail (name, spentEur, contracts, slug). money()/count() са null-safe (връщат „—"), а тестовете покриват и null стойност, и липсващ ЕИК. Логиката hasEik && eik ? eik : 'Няма' коректно избягва показване на невалиден ЕИК.

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

  1. Цитат на непотвърдена стойност. buildContractCitation копира money(c.value.currentEur) без надписа „стойност с непотвърдена достоверност", който страницата показва при v.suspect. Така копиран цитат на договор със съмнителна стойност представя непотвърдено число като факт. Струва си или да се добави каветата, или да се пропусне стойността при suspect — в тон с иначе внимателното отношение на проекта към достоверността на данните.
  2. Дублиран fallback origin. 'https://sigma.midt.bg' е hardcode-нат в три route файла (authority.tsx, company.tsx, contract.tsx). Един споделен константен export до getRootOrigin в lib/meta.ts би премахнал повторението и единичната точка за промяна при смяна на домейна.
  3. Покритие на компонента. citation.ts е добре тестван (110 реда), но CopyCitationButton (clipboard успех/грешка, таймер, cleanup при unmount) няма собствен тест. Не е блокер, но е тънкото място в покритието.

Нито едно от горните не е пречка за merge. Обхватът е атомарен и съответства на описанието на PR-а (copy-as-citation + print-friendly изглед).


Вердикт: Approve на същество (Approve) — сигурност, SQL и OWASP чисти; забележките са дребни и незадължителни.

@ydimitrof

Copy link
Copy Markdown
Contributor

Клийн, само-frontend PR. Проверих типовете, произхода (origin), сигурността и данните. Пиша ревюто.

Ревю на PR #206 — feat(web): инструменти за продуктивност (copy-citation бутон + print стилове)

Здравейте, @Hard-system и @StanislavBG 🙏 Прегледах промяната изключително внимателно — с фокус върху сигурност, целостта на данните и евентуален зловреден код. По-долу са резултатите.

Обхват и съответствие с описанието

PR-ът е коректен rebase на #157 върху актуалния main и прави точно каквото обещава — нищо повече:

  • CopyCitationButton на страниците Договор, Компания и Институция (contract.tsx, company.tsx, authority.tsx);
  • чисти citation-строители в lib/citation.ts + тестове;
  • print stylesheet в styles/base.css и бутон-правила в styles/chrome.css.

Rebase-бележките са спазени: app.css остава само @import, а CSS е разпределен в styles/* съгласно #84 (.header-actions/.save-btn до .source-cta в chrome.css; @media print до другите глобални блокове в base.css). Няма scope creep, няма несвързани промени.

Сигурност / OWASP

  • XSS: Няма. Цитатът никога не се вкарва в DOM — подава се единствено на navigator.clipboard.writeText() като plain text. Няма dangerouslySetInnerHTML, няма innerHTML. React екранира всичко останало.
  • SQL / инжекции: Няма никакъв backend/DB код в този PR — чисто клиентска и CSS промяна. Нищо за експлоатиране локално.
  • Зловреден код / backdoors / обфускация: Няма. Прегледах всеки нов файл ред по ред — няма съмнителни URL-и, eval, динамичен import, network calls или нови зависимости. navigator.clipboard е guard-нат с typeof navigator !== 'undefined' && navigator.clipboard, а .catch() логва тихо без да чупи UI.
  • Твърдо зададен произход: fallback 'https://sigma.midt.bg' при липсващ root origin — съвпада с продукцията и се използва само при SSR/error boundary, преди useMatches() да върне root data. Приемливо.

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

Сверих сигнатурите на citation-строителите срещу @sigma/api-contract:

  • ContractDetail: subject, authority: ContractParty (name), bidder: ContractParty (non-null → displayName безопасен), value.currentEur: number | nullmoney() обработва null → „—" (има тест).
  • CompanyDetail: displayName, eik, hasEik, wonEur, contracts, slug — всички налични; hasEik && eik guard-ва празно ЕИК → „Няма".
  • AuthorityDetail: name, spentEur, contracts, slug — всички налични.

Няма риск от undefined при рендиране. Ресурсите се чистят коректно — useEffect cleanup изчиства setTimeout, а timeoutRef се нулира при повторно копиране.

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

  • Тавтологични тестове: citation.test.ts интерполира money(...)/count(...) от същия източник, който имплементацията ползва — тества структурата на реда, но не и самото форматиране. Приемлив компромис (избягва härд-кодване на локал), но при бъдеща смяна на money() тестът няма да хване регресия. Ако искате — един тест с литерален очакван низ би затворил дупката.
  • Дублирана button-логика: .save-btn вече споделя декларациите на .source-cta — добро преизползване. Само проверете визуално, че flex-wrap на .header-actions изглежда добре на тесни екрани, когато и двата бутона (CTA + copy) присъстват на страницата Договор.
  • #main в print CSS — уверете се, че селекторът съвпада с реалния контейнер (иначе max-width: none няма ефект). Козметично.

CI е зелен, prettier-чист, typecheck минава. Локално не успях да пусна vitest (зависимостите не са инсталирани в worktree-а), но тестовете са тривиални и CI ги потвърждава.

Вердикт: Approve на същество (одобрявам). Няма блокиращи проблеми по сигурност или цялост на данните; бележките са по желание.

@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

Одобрявам (aa7585b): цитатът ползва c.value.currentEur (citation.ts:14 money(c.value.currentEur)) — текущата евро стойност, не сурова/стара, точно както трябва; company/authority ползват wonEur/spentEur; money(null) → em-dash. SSR-safe (typeof navigator !== 'undefined' + navigator.clipboard guard, тих fallback на insecure/legacy контекст); timeout изчистен на unmount (cleanup ок). Print стиловете скриват chrome-а и показват href-овете, break-inside:avoid.

Mobile: .save-btn ~36px (<44px), но споделя спецификацията на съществуващия .source-cta — консистентно с езика на бутоните, не нова регресия; .header-actions wrap-ва, без overflow на 320px.

@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

@StanislavBG — rebase-ът вече пази @Hard-system като commit author, добре. За да оцелее авторството и при merge (ако сливането е squash — не мога да проверя настройката с Triage права), добави Co-authored-by trailer на комита:

Co-authored-by: Miro <techhardgreenlife@gmail.com>

След като този PR влезе, ще затворим оригинала #157 (същата функционалност, но разминат с main). Благодаря, че го rebase-на.

Both buttons were ~34px tall (11px padding + 12px mono label) — below the 44px floor already
applied to .nav-search/.nav-toggle in the same media block.
@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

Проверих 458c4d0: source-cta/save-btn вече достигат 44px на touch (padding-block: 16px + margin-block: -5px — невидима hit-зона, без layout shift, същият trick като nav-search). Затваря mobile бележката. Approve остава.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Преглед на PR: „productivity tools — copy citation button and print styles"

Фаза 0 — Проверка за сигурност: ЧИСТА ✅

  • Няма твърдо кодирани тайни (API ключове, пароли, токени).
  • URL промените са към вече използвани в проекта домейни (app.eop.bg, sigma.midt.bg) — в whitelist.
  • Няма зловредни шаблони (backdoor, инжектиране на код, обфускация).
  • Няма нови зависимости — използват се съществуващи @sigma/shared, react-router, vitest.

Обобщение

Чист, добре организиран и фокусиран PR. Добавя бутон за копиране на цитат (CopyCitationButton), помощни функции за изграждане на цитати (citation.ts) с тестове, и print стилове. Почистването на timeout при unmount е коректно, rel="noopener" е запазен, а print стиловете скриват интерактивните елементи. Отдолу има няколко несъществени препоръки за подобрение — нищо блокиращо.

Основни наблюдения (несъществени)

  1. Качество на тестовете (среден приоритет). Очакваните стойности в citation.test.ts се изчисляват чрез същите money() / count() функции, вместо чрез литерален очакван низ. Това прави тестовете самоизпълняващи се — регресия във форматирането на money/count няма да бъде хваната, защото и двете страни ще се променят едновременно. Препоръчвам да се твърдят конкретните форматирани низове (напр. „125 000,50 €").
  2. Тихо неуспешно копиране (нисък приоритет). Ако navigator.clipboard липсва (несигурен контекст/HTTP или по-стар браузър), handleCopy не прави нищо и потребителят не получава обратна връзка — бутонът изглежда счупен. Помислете за fallback или поне за визуален индикатор за грешка.
  3. Именуване (нисък приоритет). CSS класът save-btn / is-saved описва действие „запазване", докато функцията е „копиране". За консистентност на именуването по-подходящ би бил напр. copy-btn / is-copied.
  4. Достъпност (информативно). aria-label="Копирай данните като цитат" замества видимия текст като достъпно име, така че при промяна на състоянието screen reader-ът няма да прочете „Копирано!" от името на бутона (макар aria-live спанът да го обявява). Приемливо, но си струва да се провери реалното поведение.
  5. Дублиране на SVG (стил). Двете икони в CopyCitationButton споделят почти всички атрибути — може да се извади обща обвивка, за да се спази правилото срещу дублиране.

Оценка на съответствие с CLAUDE.md

Няма частична имплементация, TODO коментари, мъртъв код или смесени отговорности. Тестовете покриват основните пътища на новите функции (с горната забележка за тяхната сила).

Заключение: COMMENT

Няма блокери за сигурност, коректност или производителност. Препоръчвам да се адресира качеството на тестовете (т.1) и тихото неуспешно копиране (т.2) преди merge; останалите са по преценка.

Comment thread apps/web/app/lib/citation.test.ts Outdated
Comment thread apps/web/app/components/CopyCitationButton.tsx
Comment thread apps/web/app/components/CopyCitationButton.tsx Outdated
…ia money()

The expected strings were built with the same money() the implementation
uses, so a formatting regression in money() would pass silently. Assert
the concrete localized output instead.
navigator.clipboard is unavailable in insecure contexts / older browsers,
so the copy button silently did nothing there. Fall back to
document.execCommand('copy') and show a visible failure state when both
paths fail. Also rename the save-btn/is-saved classes to copy-btn/is-copied
to match what the component actually does.
@StanislavBG

Copy link
Copy Markdown
Contributor Author

Addressed all 3 review threads:

  • Self-fulfilling citation test → asserted concrete literals instead of re-deriving via money()/count() (17c8a25)
  • Clipboard fallback + failure feedback for handleCopydocument.execCommand('copy') fallback + visible failed state (b10aa22)
  • save-btn/is-savedcopy-btn/is-copied, updated in chrome.css and base.css (b10aa22)

Typecheck and tests green.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Преглед на PR: „productivity tools — copy citation button and print styles“

ВЕРДИКТ: REQUEST_CHANGES (изисква корекции / потвърждение преди сливане)

Обобщение

PR добавя бутон за копиране на „цитат“ (основни факти) в страниците за договор, компания и институция, помощни функции за съставяне на цитата, unit тестове за тях и стилове за печат. Кодът е чист, добре структуриран и достъпен (ARIA етикети, aria-live, изчистване на timeout в useEffect — без изтичане на ресурси). Няма проблеми по сигурността: липсват твърдо кодирани тайни, SQL, инжекции или нови зависимости; текстът на цитата отива към clipboard, а не към innerHTML, така че няма XSS вектор. Промените в URL (app.eop.bg, sigma.midt.bg) са съществуващи/легитимни.

Причини за искане на корекции

  1. Логиката за ЕИК може да скрие валиден ЕИК. В buildCompanyCitation полето hasEik е незадължително (hasEik?: boolean). Ако обектът от loader-а (loaderData.company) не съдържа hasEik, но има валиден eik, изразът c.hasEik && c.eik е falsy и цитатът винаги ще показва „ЕИК: Няма“. Тестовете подават hasEik изрично, затова не улавят този случай. Трябва да се потвърди, че loader-ът винаги връща hasEik, или логиката да разчита директно на c.eik.
  2. Възможен null при изпълнител. buildContractCitation чете c.bidder.displayName без защита. Ако договор без класиран изпълнител (отменен/без възлагане) има bidder === null, това ще хвърли грешка при рендиране на страницата. Нужна е проверка спрямо реалния тип ContractDetail.
  3. Липса на тестове за компонента CopyCitationButton. При изисквано покритие ≥90% за нов код, React компонентът (Clipboard API, fallback към execCommand, състояния copied/failed, изчистване на timeout) е напълно непокрит. Помощните функции имат добри тестове, но интерактивната логика — не.

По-малки бележки

  • Дублиран hardcoded fallback 'https://sigma.midt.bg' в три файла (authority/company/contract) — да се изнесе в обща константа (принцип „без дублиране“).
  • Печатното правило a[href]:not([href^='#'])::after разкрива URL и за вътрешните относителни връзки (напр. „ (/contracts/abc) “), което може да е шумно; интенцията е обяснена в коментара, но си струва да се прецени.

Заключение

Функционалността е добре изпълнена и безопасна. Преди одобрение е нужно да се потвърдят/адресират точки 1–3 (особено коректността на ЕИК и null-безопасността на изпълнителя) и да се добави тестово покритие за компонента.

Comment thread apps/web/app/lib/citation.ts Outdated
Comment thread apps/web/app/lib/citation.ts Outdated
Comment thread apps/web/app/components/CopyCitationButton.tsx
Comment thread apps/web/app/routes/authority.tsx Outdated
Comment thread apps/web/app/styles/base.css Outdated
- add CopyCitationButton test coverage (clipboard success, execCommand
  fallback, copied/failed states, unmount timeout cleanup); wires up
  jsdom + testing-library for the first React component tests in the repo
- extract the duplicated fallback origin literal into FALLBACK_ORIGIN in
  lib/meta.ts and use it in authority/company/contract routes
- restrict the print-mode href-reveal rule to absolute http(s) links so
  internal relative links no longer get their href appended
…ier format

environmentMatchGlobs is not a valid Vitest 4.1.7 InlineConfig property (TS2769);
jsdom is already set per-file via the @vitest-environment docblock in
CopyCitationButton.test.tsx.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Преглед на PR: feat(web): productivity tools — бутон „Копирай цитат“ и стилове за печат

ВЕРДИКТ: COMMENT — няма блокиращи проблеми; препоръчват се няколко дребни подобрения преди merge.

Обхват и съответствие с описанието

PR-ът е атомарен и фокусиран: добавя (1) споделени билдъри за цитати (citation.ts), (2) компонент CopyCitationButton, (3) вграждане на бутона в страниците за институция/компания/договор, (4) стилове за печат в base.css и обединяване на стиловете на действията в chrome.css, (5) тестова инфраструктура (@testing-library/react, jsdom). Няма разширяване на обхвата извън заявеното.

Фаза 0 — Сигурност (сканиране)

  • Тайни/ключове: няма hardcoded секрети. ✅
  • URL промени: новите/променените URL адреси са https://sigma.midt.bg (FALLBACK_ORIGIN — собственият домейн на проекта) и вече съществуващият https://app.eop.bg. Легитимни, не сочат към външни неодобрени хостове. ✅
  • Зловреден код / backdoor / обфускация: няма открити модели. ✅
  • Зависимости: @testing-library/react и jsdom са стандартни dev-зависимости; добавени са само в devDependencies. Голямото разрастване на pnpm-lock.yaml е транзитивно от jsdom и е очаквано. ✅
  • Инжекции (SQL/XSS): няма SQL в диапазона на промените. Копираният текст отива в клипборда, не се рендира като HTML → няма XSS вектор. Билдърите на цитати само конкатенират низове. ✅

Функционален преглед

  • copyWithExecCommand е коректен fallback за среди без navigator.clipboard (несигурен контекст/HTTP); textarea е скрит и се почиства коректно.
  • Изчистването на таймаута при unmount е реализирано → няма изтичане на ресурс.
  • Тестовете покриват успех през Clipboard API, fallback към execCommand, двоен провал, изключение и почистване при unmount — добро покритие на новия код.
  • Билдърите на цитати са тествани за случаи с null стойност и с/без ЕИК.

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

  1. Възможен setState след unmount при висящ writeText промис (виж инлайн коментар) — безвредно в React 19, но лесно за подсигуряване с флаг mounted/AbortController.
  2. Достъпност: достъпното име на бутона (aria-label) остава статично при успех; промяната се обявява само чрез aria-live регион — приемливо, но заслужава преглед.
  3. Тестова хигиена: глобалните navigator/document се мутират без възстановяване (restoreAllMocks не връща Object.assign) — при бъдещи тестове в същия файл това може да доведе до споделено състояние.

Заключение

Кодът е чист, добре тестван и следва съществуващите шаблони. Няма проблеми със сигурността или целостта на данните. Препоръчвам разглеждане на дребните забележки, но те не блокират merge.

Comment thread apps/web/app/components/CopyCitationButton.tsx
Comment thread apps/web/app/components/CopyCitationButton.tsx Outdated
Comment thread apps/web/app/components/CopyCitationButton.test.tsx
# Conflicts:
#	apps/web/app/routes/contract.tsx
…tion button)

- guard async clipboard callbacks with a mounted ref to skip state
  updates after unmount
- reflect the copied state in the button's aria-label so it matches
  the aria-live announcement
- restore navigator.clipboard/document.execCommand in afterEach so
  test mutations don't leak between tests
- add a regression test for the unmount-before-resolve guard
hasEik is redundant with eik (ETL only ever populates eik when
eik_valid=1), so drop the optional hasEik gate that silently hid a
present EIK when a caller omitted the field. Guard bidder null in
buildContractCitation to avoid throwing on an unawarded contract.
@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

За атрибуция (on-record): първоначалната реализация на copy-citation/print е от #157 (@Hard-system). Този PR стъпва на нея и я надгражда (unmount-safe състояние, tri-state, тестове, print стилове). При merge @Hard-system се кредитира като съавтор чрез Co-authored-by в squash commit-а — оригиналното авторство е запазено.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ревю на PR: feat(web): productivity tools - copy citation button and print styles

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


Обобщение

Фокусиран, атомарен PR: бутон за копиране на цитат (CopyCitationButton), чисти билдъри на цитати (citation.ts), print стилове и съответните тестове. Промяната е с ясен обхват, без scope creep.

Фаза 0 — Сигурност (задължителна): ЧИСТО ✅

  • Тайни/ключове: няма.
  • URL промени: единственият нов URL FALLBACK_ORIGIN = 'https://sigma.midt.bg' е собственият канoничен домейн на сайта; app.eop.bg е съществуващ. Whitelisted.
  • Malicious patterns / backdoors / обфускация: няма.
  • SQL / инжекции: няма достъп до база. Билдърите само конкатенират данни от loader-а в текст за клипборда и в React атрибути/деца, които се екранират автоматично (няма dangerouslySetInnerHTML). OWASP-съвместимо (A03 Injection не е приложимо тук).
  • Зависимости: jsdom, @testing-library/react — стандартни dev/test инструменти, коректно добавени в devDependencies и lock файла.

Качество на кода / архитектура

  • Чисто разделяне: логиката за цитати е изнесена в citation.ts и е реизползвана в трите route-а — без дублиране на бизнес логика.
  • CopyCitationButton коректно управлява ресурси: mountedRef предотвратява setState след unmount, timeoutRef се изчиства в cleanup — няма resource/state leak.
  • Достъпност: aria-label, aria-live="polite", aria-hidden на иконите, скрит textarea за fallback — добре.
  • CSS: .header-actions реюзва бутонния език на .source-cta; hit-area на мобилно е коригирана до 44px минимум.

Тестове

Покритието е много добро — успех, fallback към execCommand, двоен провал, хвърляне на изключение, изчистване на timeout при unmount и late-resolve след unmount. Тестовете са смислени (проверяват реално поведение, не тривиални assert-и).

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

  1. handleCopy: fallback-ът към document.execCommand('copy') вътре в .catch на отхвърлен writeText в реален браузър може да се провали, защото след await-ната промис верига се губи контекстът на потребителския жест (user-gesture). Не е дефект — деградацията е плавна и завършва в коректно failed състояние — но заслужава да се отбележи.
  2. citation.ts: тип-сигнатурата на buildCompanyCitation не включва hasEik, докато тестовите обекти го подават. Функцията коректно го игнорира (използва c.eik ?? 'Няма'); подравняването на тестовите данни към типа би премахнало подвеждащото поле.
  3. Print CSS a[href^='http']::after разкрива URL-а на всеки външен линк на страницата при печат — това изглежда е желаното, просто потвърдете за страници с много външни връзки.

Готовност за деплой

Няма миграции, няма breaking changes, промяната е back-compatible и не чупи main. Може да се мерджне.

Comment thread apps/web/app/components/CopyCitationButton.tsx
Comment thread apps/web/app/lib/citation.ts

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Прегледах стриктно на връх 9ba5636. #206 е строго подобрение над #157: tri-state UX, execCommand fallback, mountedRef unmount guard, компонентни тестове с конкретни литерали, 44px touch targets, print href-reveal само за абсолютни URL. Без регресия спрямо #157. SSR-safety на navigator.clipboard guard-а е коректна.

Едно за координация (не е дефект в кода ви, cross-PR зависимост):

  • apps/web/app/lib/citation.ts — цитираният URL е ${origin}/contracts/${c.id}, а c.id = contractSlug(r.id) (details.ts:666). На текущия main contractSlug НЕ кодира /, тъй че за договор с „/" в id-то цитатът дава счупен линк (404). PR #221 въвежда точно това кодиране; след неговия merge c.id идва вече %2F-кодиран и цитатът става коректен — не добавяйте втори encodeURIComponent тук (би довело до double-encode след #221). Т.е.: merge #221 първо, после този.

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

  • citation.ts — при value_flag от suspect-множеството UI-ят показва „непотвърдена достоверност", но цитатът копира голото число без уговорка. Струва си follow-up (уговорка или „—").
  • CopyCitationButton.tsxexecCommand fallback-ът в .catch тече след изгубен user-gesture → в реален браузър ще fail-не (в jsdom минава); поне коментар да го отбележи.
  • print: a[href^='http']::after разкрива URL-а на всеки външен линк — на плътна contract страница става шумно; решение за @todorkolev дали да се стесни.

Атрибуция: PR-ът стъпва на #157 (@Hard-system) — при squash увери се, че commit-ът носи Co-authored-by: Miro <…>, за да се запази оригиналното авторство.

Одобрявам — след merge на #221 (заради линка в цитата).

- CopyCitationButton: document the execCommand-after-rejected-writeText
  gesture-context tradeoff instead of silently leaving it unexplained
- citation.test.ts: drop stray hasEik fixture field unused by
  buildCompanyCitation (a sibling test already proves output only
  depends on eik)
# Conflicts:
#	apps/web/app/routes/authority.tsx
#	pnpm-lock.yaml

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ревю на PR: feat(web): productivity tools — copy citation button and print styles

ВЕРДИКТ: COMMENT — няма блокиращи проблеми; препоръчани са малки корекции преди merge.

Обобщение

Чиста, добре тествана и фокусирана промяна. Добавя бутон „Копирай като цитат“ (CopyCitationButton), помощни функции за изграждане на цитати (citation.ts) и стилове за печат. Кодът следва съществуващите шаблони, изнесен е добре по слоеве (логиката за цитата е отделена от UI), а тестовете покриват успех/провал/fallback и почистване на таймери при unmount.

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

  • Няма хардкоднати тайни. FALLBACK_ORIGIN = 'https://sigma.midt.bg' е публичният домейн на приложението, не credential.
  • Няма SQL / injection. Билдърите на цитати правят само конкатенация на текст, който отива в клипборда — никъде не се рендира като HTML, няма dangerouslySetInnerHTML, няма eval.
  • Fallback чрез execCommand използва <textarea>.value (безопасно присвояване, не HTML) и коректно почиства елемента.
  • URL промени (app.eop.bg — вече съществуващ; sigma.midt.bg — собствен домейн) са легитимни.
  • Нова зависимост @testing-library/react@^16.3.2 е само dev-dependency, утвърдена библиотека; транзитивните пакети в lock файла изглеждат нормални.
  • OWASP: няма изложени рискове от Top 10 в обхвата на промяната.

Качество / коректност (за проверка)

  1. Несъответствие в URL на договораbuildCompanyCitation и buildAuthorityCitation използват slug, докато buildContractCitation използва суровото c.id. Route файлът внася contractSlug/contractIdFromSlug, което подсказва slug-базирани канонични URL-и. Моля потвърдете, че /contracts/{id} резолвва коректно (или дали не води до redirect), за да е консистентна връзката в цитата с останалите. Виж inline коментар.
  2. Дублиран/подвеждащ тест — тестът „shows the EIK when hasEik is absent from loaderData…“ е идентичен по данни и очаквания с „builds a company citation with EIK“, а името споменава hasEik/loaderData, каквито не съществуват във функцията. Изглежда остатък от предишен дизайн — за премахване или преформулиране. Виж inline коментар.

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

  • Print стилът a[href^='http']::after ще добави URL и към абсолютни вътрешни линкове (не само външни) — очаквано ли е това при печат?
  • Достъпност: aria-live="polite" и aria-label са налични — добре.

Тестове

Покритието на новия код е добро (Clipboard API успех, execCommand fallback, провал на двата пътя, изчистване на timeout при unmount, без state update след unmount). Тестовете са смислени, не тривиални.

Няма установени резервни ресурси/течове — useEffect cleanup изчиства таймера и пази с mountedRef.

Comment thread apps/web/app/lib/citation.ts
Comment thread apps/web/app/lib/citation.test.ts

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ревю на PR: feat(web): productivity tools — copy citation button and print styles

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

Фаза 0 — Сканиране за сигурност (ЗАДЪЛЖИТЕЛНО): ЧИСТО ✅

  • Твърдо кодирани тайни: няма. Няма API ключове, пароли или токени.
  • URL адреси: FALLBACK_ORIGIN = https://sigma.midt.bg е собственият домейн на проекта; https://app.eop.bg/... е съществуващ и непроменен (само преместен в .header-actions); https://sigma.test се използва само в тестове. Всички са безопасни.
  • Злонамерени модели: няма. Няма eval, обфускация, задни вратички или инжектиране на код.
  • Инжекции (SQLi/XSS): няма поле за атака. Текстът на цитата се вмъква само като React текстово дете (автоматично екраниране) и се записва в клипборда като plain text — никъде няма innerHTML/dangerouslySetInnerHTML. Няма SQL в тази промяна. Fallback-ът с document.execCommand('copy') през временно <textarea> е стандартна безопасна практика. Print CSS attr(href) е инертен.
  • Нови зависимости: @testing-library/react@^16.3.2 — легитимна, широко използвана dev зависимост; транзитивните пакети (@testing-library/dom, dom-accessibility-api, pretty-format и т.н.) са стандартни. Оправдана и правилно ограничена до devDependencies.

OWASP: съответства — няма повърхност за инжектиране, изходът се екранира от React, обработката на грешки е контролирана.

Качество на кода: отлично

  • Проверих локално, че PageHeader приема children, а money(null) връща „—" и count форматира числа — поведението съвпада с очакванията в тестовете.
  • Рефакторингът в contract.tsx (обвиване на съществуващия source-CTA в .header-actions) е чист; margin-top е коректно преместен от .source-cta към .header-actions.
  • Разделяне на отговорностите: чистите функции за цитати в lib/citation.ts са отделени от UI компонента — добре.

Тестове: изчерпателни

CopyCitationButton.test.tsx покрива успех през Clipboard API, fallback към execCommand, двоен провал, хвърляне на изключение, изчистване на timeout при unmount и защита срещу закъсняло resolve след unmount. citation.test.ts покрива null стойност, липсващ изпълнител, с/без ЕИК и празен ЕИК. Смислени тестове, не тривиални.

Достъпност

aria-label/title се сменят по състояние, има aria-live="polite", иконите са aria-hidden, а мобилната hit-area е разширена до 44px — добре обмислено.

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

  1. buildCompanyCitation: c.eik ?? 'Няма' пропуска празен низ '' и произвежда празен ред „ЕИК: " (поведението е съзнателно тествано, но е леко несъвършенство в данните). Обмислете c.eik || 'Няма'.
  2. Print стил a[href^='http']::after добавя URL след всяка абсолютна връзка в съдържанието — може да дублира текст, когато видимият текст вече е URL, и не покрива protocol-relative (//) връзки. Козметично.

Композитна оценка: ~9.4/10. Одобрявам по същество, но оставям вердикт COMMENT, за да можете да прегледате бележките преди сливане.

Comment thread apps/web/app/lib/citation.ts Outdated
Comment thread apps/web/app/styles/base.css Outdated
…nt links

Empty-string eik previously rendered a blank ЕИК line via ?? (only catches null/undefined); switched to || so falsy empty string also falls back. Print URL suffix now also matches // protocol-relative links, not just http(s).
…, suppress RSC-only CSRF

Bump postcss to ^8.5.18 (GHSA-r28c-9q8g-f849) and valibot to ^1.4.2
(GHSA-5qjj-4xww-7phc) via pnpm overrides - both patch-level, non-breaking
fixes. Bump react-router/@react-router/dev to ^7.18.0 via override, fixing
4 real advisories (SSR hydration constructor injection, unauthenticated DoS,
RSCErrorHandler XSS, open-redirect via backslash) - all fixed within the 7.x
line, no major bump needed. Add a time-boxed osv-scanner.toml suppression for
the one remaining advisory, GHSA-qwww-vcr4-c8h2, a CSRF flaw scoped to
unstable RSC APIs this app does not use (verified via repo-wide grep) with
no fix in the 7.x line; bumping to 8.x is out of scope for this patch.
- osv-scanner.toml: add/add conflict, both suppression entries (react-router
  RSC CSRF, sharp/miniflare) are independent and target different packages;
  kept both
- pnpm-lock.yaml: regenerated via pnpm install after resolving other files
@nedda76

nedda76 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants