feat(web): add scroll-to-top button - #201
Conversation
|
@todorkolev готов за ревю 🙏 — rebase-нат на main, CI зелен, prettier-чист, CSS промените в styles/* (app.css само @import). Резолвнати нишки. Approve-ни когато ти е удобно. |
|
Одобрявам на същество. Прегледах дифа изцяло, сравних с issue #176, и проверих локално за SQL/инжекционни и други уязвимости. Обхват на прегледаPR #201 — rebase на #176 (@Hard-system) върху актуалния Съответствие с issue #176Имплементацията покрива описанието на #176 точно: нов компонент с поява при скрол >400px, глобална интеграция в Сигурност / OWASP / целостност на данните
Качество и конвенции
Незадължителни бележки (не блокират)
Нищо от горните не е пречка за merge. Verdict: Approve на същество — сигурност, OWASP и целостта на данните са чисти; кодът е малък, коректен и съответства на #176. |
|
Проверих локално целия diff, историята на PR-а, съответствието с issue #176 и взаимодействието с останалия „chrome" на приложението. Ето обобщението. Обхват и съответствие с issue #176PR-ът е rebase на #176 (@Hard-system) върху актуалния Сигурност / OWASPЧисто. Компонентът е изцяло клиентски:
Не намерих нищо злонамерено или подозрително. Забележки (не блокиращи)
Нито една от трите не е блокираща за функционалност или сигурност. Вердикт: Approve на същество — сигурността е чиста, съответства на issue #176; препоръчвам само да се провери мобилното застъпване с a11y бутона (т.1) преди merge. |
|
Одобрено ✅ (Approve на същество) Ревю на PR #201 — feat(web): add scroll-to-top buttonПрегледах целия diff локално, ред по ред, с фокус върху сигурност, интегритет на данните и евентуален зловреден код. PR-ът е малък, изцяло frontend, добавя един презентационен компонент ( Сигурност / OWASP
Интегритет на даннитеНеприложимо — компонентът не чете и не пише данни. Няма промяна в state отвъд един локален булев флаг. Достъпност (силна страна)
Забележки (незадължителни, не блокират)
Съответствие с описаниетоИмплементацията отговаря на потребителската история от #176: fade-in след 400px, плавно връщане, фокус-мениджмънт, дизайн токъни ( ВерификацияDiff-ът е чист, без TODO/FIXME, без dead code, без дублиране. CI е зелен и prettier минава според нишката за ревю. Забележките по-горе са полиране на UX/a11y и не са пречка за merge. Благодаря за спретнатата работа и подробното PR описание. 🙏 |
|
Прегледах rebase-а на #176 (mobile-first + a11y). Компонентът е чист: видимостта е gate-ната за AT и клавиатура ( Една мобилна бележка (не блокер): бутонът е Одобрявам (rebase на #176 върху main, prettier-чист). Оригиналът #176 да се затвори след merge. |
|
@StanislavBG — rebase-ът вече пази @Hard-system като commit author, добре. За да оцелее авторството и при merge (ако сливането е squash — не мога да проверя настройката с Triage права), добави Co-authored-by trailer на комита: След като този PR влезе, ще затворим оригинала #176 (същата функционалност, но разминат с main). Благодаря, че го rebase-на. |
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: feat(web): add scroll-to-top button
Обобщение
PR добавя бутон „Към началото“ (ScrollToTop), който се появява след скролване надолу и връща потребителя в горната част на страницата. Реализацията е чиста, съобразена с достъпността (aria-label, aria-hidden, управление на tabIndex, focus-visible стилове, passive scroll listener, respekt към мобилни размери). Няма открити проблеми със сигурността във Фаза 0 — няма твърдо кодирани тайни, нови URL адреси, нови зависимости или зловредни шаблони. Резултат от сигурността: ЧИСТ.
Силни страни
- Коректна SSR безопасност —
windowсе достъпва само вътре вuseEffect, началното състояние еfalse. - Добра достъпност:
aria-hidden={!isVisible},tabIndexпревключване,focus-visibleстилове, смислен българскиaria-label. - Почистване на event listener в return на
useEffect— няма изтичане на ресурси. - CSS уважава мобилни размери и използва съществуващите CSS променливи (
--ink,--paper,--accent).
Проблеми, изискващи внимание
- Липсват тестове (блокиращо спрямо изискванията). Няма добавен нито един тест за новия компонент. Инструкциите изискват покритие ≥90% и смислени тестове за всяка функция. Необходими са тестове за: превключване на видимостта при скрол праг 400px, извикване на
scrollTo, ARIA атрибути според състоянието. - Възможно припокриване с
AccessibilityWidget. И двата компонента вероятно саposition: fixedв долния десен ъгъл. Трябва да се провери визуално, че двата бутона не се застъпват на десктоп и мобилни устройства. - Незадължителна оптимизация на scroll handler-а (rAF/throttle) — незначителна, тъй като React прекъсва повторните рендери при непроменена стойност.
Липсваща документация
Няма актуализация на документация/changelog за новия UI елемент. Ако проектът поддържа такъв, добавете кратко описание.
Заключение
Заявявам REQUEST_CHANGES основно поради липсата на тестове (изискване 3.0/3.0 не е покрито) и необходимостта да се потвърди липсата на визуално припокриване с AccessibilityWidget. Функционалният код е с добро качество и след добавяне на тестове PR може да бъде одобрен.
…tle scroll handler - pin scroll-to-top to bottom-left instead of bottom-right so it never shares a corner with the third-party accessibility launcher, which anchors right: 0 at every breakpoint with an unreliable vertical position - throttle the scroll listener with requestAnimationFrame and a ticking guard to avoid running toggleVisibility on every scroll event
|
Fixed both points, pushed in 2b0d5df:
|
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: feat(web): add scroll-to-top button
ВЕРДИКТ: COMMENT — чист код без блокиращи проблеми, но липсват тестове и една препоръка за достъпност (prefers-reduced-motion).
Фаза 0 — Сканиране за сигурност (задължително, изпълнено първо)
- Твърдо кодирани тайни (API ключове/пароли/токени): няма. ✅
- Промени по URL адреси / нови endpoint-и: няма. ✅
- Зловреден код (backdoor, инжекция на код, обфускация,
eval,dangerouslySetInnerHTML): няма. ✅ - SQL / NoSQL инжекция: неприложимо — промяната е чисто клиентски UI компонент, не докосва база данни или заявки. ✅
- XSS: няма динамичен HTML; SVG-то е статично,
aria-labelе литерал. ✅ - Нови зависимости: няма — използват се само
reactи нативни browser API-та. ✅
Резултат от Фаза 0: CLEAN. Няма основание за автоматично REQUEST_CHANGES по сигурност. OWASP: не се откриват релевантни рискове (A03 Injection / A07 / A08) в обхвата на промяната.
Общ преглед на качеството
Компонентът е малък, добре структуриран и следва добри практики:
- Скролът е throttle-нат чрез
requestAnimationFrame+passive: trueслушател → без performance регресия. ✅ - Слушателят се премахва при unmount (
removeEventListener) → няма изтичане на ресурси. ✅ - Достъпност:
type="button",aria-label,aria-hidden+tabIndexсинхронизирани със състоянието на видимост; при скрит бутонvisibility: hiddenпредотвратява кликане. ✅ - Позиционирането долу-вляво е документирано с ясен коментар защо не се стакова с third-party a11y бутона. ✅
- Няма SSR/hydration несъответствие —
isVisibleстартира сfalseи на сървъра, и на клиента.
Съответствие с CLAUDE.md / Quality Gates
- Тестове (изисквани 3.0/3.0): ❌ Не са добавени тестове за
ScrollToTop. Правилото „COMPREHENSIVE TESTS for every function“ не е изпълнено. Липсва покритие за: превключване на видимостта при праг 400px, throttle логиката и почистването на слушателя. Това е основната причина да не давам APPROVE. - Документация: промяната е самодостатъчна; API документация не се изисква. ✅
- Дублиране / мъртъв код / смесени отговорности: няма. ✅
- Обхват: атомарна промяна, фокусирана върху една задача, без scope creep. ✅
Забележки (непокриващи, вж. инлайн коментарите)
- prefers-reduced-motion не се зачита — гладкият скрол и CSS преходите могат да причинят дискомфорт на потребители с вестибуларни нарушения (WCAG 2.3.3). Препоръчвам да се уважи медия заявката.
- Прагът
400и другите „магически“ стойности биха могли да са именувани константи, но това е дребна стилистична бележка.
Препоръка
Кодът е сигурен и годен за продукция от гледна точка на сигурност/производителност. Преди merge моля добавете unit тестове за компонента и обмислете prefers-reduced-motion. След добавяне на тестове PR-ът може да бъде одобрен.
- honor prefers-reduced-motion by using instant scroll instead of smooth - disable the button's opacity/transform/background transitions under reduced motion - extract the 400px show threshold into a named SHOW_AFTER_PX constant
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: feat(web): add scroll-to-top button
Резюме
Чист и добре структуриран PR, който добавя бутон „Към началото". Кодът е с високо качество: throttling на scroll слушателя през requestAnimationFrame, passive: true, коректно премахване на слушателя при unmount (без изтичане на ресурси), безопасен за SSR (достъпът до window е само в useEffect и в обработчиците, а началното isVisible=false избягва разминаване при хидратация), спазване на prefers-reduced-motion едновременно в JS и в CSS, и добра достъпност (aria-label, управление на aria-hidden/tabIndex). Коментарът в CSS, който обяснява защо бутонът е долу-вляво (за да не се застъпва с third-party accessibility launcher-а вдясно), е много полезен.
Сигурност (Phase 0 — задължителен сканинг)
- Няма зашити тайни (API ключове, пароли, токени).
- Няма нови или променени URL адреси.
- Няма нови зависимости.
- Няма опасни модели: липсва
dangerouslySetInnerHTML, няма потребителски вход, SVG-то е статично и вградено, няма SQL/команден injection повърхнина, няма XSS вектор. - OWASP: не се откриват релевантни рискове за този обхват.
Резултат от Phase 0: CLEAN — няма блокиращи проблеми със сигурността.
Забележки (незадължителни)
- Липса на тестове. Спрямо стриктните критерии за качество (покритие ≥90% за нов код, смислени тестове) този PR не добавя тестове за новия компонент. За презентационен компонент това е приемливо в много проекти, но за пълно одобрение по вашите правила препоръчвам поне минимален тест (превключване на видимостта над/под прага, извикване на
scrollTo, спазване на reduced-motion). - Обхват на скролване.
window.scrollYпредполага, че страницата скролва през window, а не през вътрешен контейнер. Ако някои изгледи използват скролиращ контейнер, бутонът няма да реагира — струва си да се потвърди, че layout-ът скролва на ниво window. - Козметично (CSS): при
:focus-visiblebox-shadowсе презаписва изцяло с фокус-пръстена и се губи основната „повдигната" сянка. Ако е умишлено — окей; иначе комбинирайте двете сенки.
CLAUDE.md / готовност за деплой
- Атомарна промяна, без scope creep. Реализацията използва минимални промени и съществуващи CSS токени (
--ink,--paper,--accent). - Без мъртъв код, без дублиране, без смесване на отговорности.
- Единствената незакрита точка спрямо стриктните gate-ове е покритието с тестове.
Вердикт на отделен ред за бърз преглед от други ревюъри:
ВЕРДИКТ: COMMENT — сигурността е чиста и кодът е с високо качество; преди пълно одобрение добавете тестове за новия компонент (и потвърдете обхвата на скролване).
Covers visibility toggling, scrollTo invocation, and reduced-motion behavior for ScrollToTop per ydimitrof's review on midt-bg#201. Also preserves the button's raised drop-shadow under :focus-visible (previously overwritten by the focus-ring box-shadow) and documents the document/window-level scroll assumption.
|
За атрибуция (on-record): първоначалната реализация на този scroll-to-top компонент е от #176 (@Hard-system). Този PR стъпва на нея и я надгражда (rAF throttling, |
ydimitrof
left a comment
There was a problem hiding this comment.
🇧🇬 Български вариант
Резюме на прегледа — feat(web): add scroll-to-top button
Прегледах промените стриктно, с приоритет върху сигурност и цялост на данните.
Фаза 0 — Сканиране за сигурност: ЧИСТО
- Няма зашити тайни (API ключове, пароли, токени).
- Няма добавени или променени URL адреси; няма мрежови заявки.
- Няма SQL, няма
eval/Function, нямаdangerouslySetInnerHTML— SVG е статичен inline маркъп. Няма вектори за инжекции/XSS. - Единствената нова зависимост е
jsdom(само за разработка/тестове). Всички добавки вpnpm-lock.yamlса легитимни транзитивни зависимости наjsdom. Няма признаци за компрометирана верига на доставки. OWASP: няма отклонения.
Коректност: Много добра. useEffect работи само на клиента (SSR-безопасно; началното isVisible=false съвпада със сървърния рендер → няма hydration несъответствие). Scroll слушателят е passive и throttle-нат през requestAnimationFrame с ticking флаг. Cleanup функцията коректно премахва слушателя (няма изтичане на ресурси). Reduced-motion е спазено и в JS (matchMedia), и в CSS.
Достъпност: aria-hidden и tabIndex се превключват заедно правилно; visibility: hidden спира кликовете, когато бутонът е скрит; има aria-label; декоративният SVG е с aria-hidden="true". Отлично.
Дребни, неблокиращи бележки:
- Граничната стойност точно на
SHOW_AFTER_PX(400, строго>) не е покрита с тест — тества се 399 и 401, но не и 400. - Пътят на почистване при unmount (премахване на слушателя) не е тестван.
- Тестовете извикват
.click()докато бутонът е логически скрит (scrollY=0) — това упражнява handler-а, но не реалната pointer семантика приvisibility: hidden.
Съответствие с CLAUDE.md: Няма частична имплементация, TODO-та, дублиран или мъртъв код. Именуването е консистентно. Разделението на отговорностите е чисто (компонент + стилове + тестове).
ВЕРДИКТ: COMMENT — няма блокиращи или свързани със сигурността проблеми; препоръчвам да се добавят 2 малки теста (граница 400 и cleanup при unmount) преди merge.
🇬🇧 English version
Review summary — feat(web): add scroll-to-top button
I reviewed the changes strictly, prioritizing security and data integrity.
Phase 0 — Security scan: CLEAN
- No hardcoded secrets (API keys, passwords, tokens).
- No new/modified URLs; no network calls introduced.
- No SQL, no
eval/Function, nodangerouslySetInnerHTML— the SVG is static inline markup. No injection/XSS vectors. - The only new dependency is
jsdom(dev/test only). Allpnpm-lock.yamladditions are its legitimate transitive deps. No supply-chain red flags. OWASP: no deviations.
Correctness: Strong. useEffect is client-only (SSR-safe; initial isVisible=false matches the server render → no hydration mismatch). The scroll listener is passive and rAF-throttled with a ticking guard. The cleanup removes the listener (no resource leak). Reduced motion is respected in both JS (matchMedia) and CSS.
Accessibility: aria-hidden and tabIndex toggle together correctly; visibility: hidden blocks clicks while hidden; aria-label present; decorative SVG marked aria-hidden="true". Excellent.
Minor, non-blocking notes:
- The exact
SHOW_AFTER_PXboundary (400, strict>) is untested — 399 and 401 are covered, but not 400. - The unmount cleanup path (listener removal) is untested.
- Tests call
.click()while the button is logically hidden (scrollY=0) — this exercises the handler but not real pointer semantics undervisibility: hidden.
CLAUDE.md compliance: No partial implementation, TODOs, duplicated or dead code. Naming is consistent. Concerns are cleanly separated (component + styles + tests).
VERDICT: COMMENT — no blocking or security issues; recommend adding 2 small tests (the 400 boundary and unmount cleanup) before merge.
todorkolev
left a comment
There was a problem hiding this comment.
Прегледано и тествано локално: rAF-throttle на scroll handler-а, prefers-reduced-motion в JS и CSS, позициониране вляво без колизия с accessibility widget-а, тестове за компонента. Бележките на ydimitrof са адресирани в комитите от 10-11.07.
What changed
Scroll-to-top button, moved to bottom-left (to avoid overlapping the third-party accessibility launcher), with rAF-throttled scroll handling. Review fixes: respects
prefers-reduced-motion(both the JS scroll behavior and CSS transitions), extracted the400pxthreshold into a namedSHOW_AFTER_PXconstant, and preserves the raised drop-shadow under:focus-visible.How it was tested
pnpm --filter web test— new coverage for visibility toggling,scrollToinvocation, and reduced-motion behavior.prefers-reduced-motion: reduceemulated in devtools.Quality checks