--- name: core-components-code-review description: Review a Pull Request or diff in the @alfalab/core-components UI library — correctness bugs, public API/breaking changes, accessibility, keyboard/focus/pointer interaction, component states, visual/layout regressions, SSR/browser compatibility, performance, and test coverage. Use when asked to review a PR, review changes to a component under packages/*, or check a diff before merge. license: MIT --- # Core Components Code Review Skill ## Назначение Этот skill помогает провести ревью Pull Request или diff в библиотеке UI-компонентов `@alfalab/core-components`. Он предназначен именно для **ревью изменений компонентов**: корректность поведения, публичный API, accessibility, состояния, визуальные регрессии, платформенная совместимость, тесты — а не для общей проверки code style или вкусовых предпочтений. ## Главный принцип > **Лучше пропустить сомнительное низкоприоритетное замечание, чем создать ложное или необоснованное замечание.** False positive хуже, чем пропущенный низкоприоритетный issue. Каждое существенное замечание должно быть доказуемо кодом или доступным контекстом, а не предположением. ## Контекст: компонент как публичный API В отличие от ревью обычного приложения, изменения в `core-components` затрагивают множество потребителей библиотеки: продуктовые команды, интеграции, десятки использований одного и того же компонента. Небольшое изменение в публичном компоненте может повлиять на props, TypeScript-типы, accessibility, визуал, SSR, разные браузеры и design system conventions одновременно. Поэтому любое изменение в `packages/` нужно оценивать не только как локальный код, а как **изменение публичного контракта**. Успешная компиляция и прохождение тестов не являются доказательством того, что поведение компонента корректно. ## Входные данные Собери максимум доступного контекста PR, прежде чем делать выводы: - diff PR и список изменённых файлов; - описание PR и связанную задачу; - тип изменения по conventional commit префиксу (`fix`/`feat`/`hotfix`/`chore`/`ci`/`test`) — подсказка о характере правки; - затронутые пакеты в `packages//` — это единица публичного API; - `typings.ts` компонента — публичные props и их типы; - окружающий код: `Component.responsive.tsx`, `desktop/`/`mobile/` варианты, вложенные `components/`, `utils.ts`; - тесты: `*.test.tsx` (unit) и `*.screenshots.test.tsx` (визуальные, Playwright + jest-image-snapshot); - Storybook: `docs/*.stories.tsx`, `*.docs.mdx`, `description.mdx`; - changeset-файл (`.changeset/*.md`) — обязателен для PR, меняющих публикуемый код пакета (влияет на semver); не требуется, если PR затрагивает только Storybook (`*.stories.tsx`/`*.mdx`) или служебные скрипты/тулинг репозитория, не влияющие на опубликованные пакеты; - обсуждение PR: замечания людей, ответы автора и статусы CI-джобов для последнего коммита PR (см. «Повторное ревью» и «Проверка запуском»); - существующие project conventions — `docs/code-review.stories.mdx` (чек-лист ревью проекта) и `.github/pull_request_template.md` (чек-лист автора PR). Эти доки могли устареть (давно не обновлялись) — используй их как отправную точку, но не доверяй им безоговорочно: конкретные утверждения (авточеки, правила) перепроверяй по актуальному состоянию репозитория (реальные workflow в `.github/workflows/`, реальный код). Если для подтверждения проблемы не хватает контекста — сначала попытайся его получить (прочитать соседний файл, типы, тесты), а не делать предположение. ## Процесс ревью Проходи по этапам последовательно. Каждый этап, отсылающий к файлу в `references/`, требует **прочитать этот файл целиком через Read** перед тем, как делать выводы по соответствующей области — это не необязательная ссылка "см. также", а обязательный шаг. ### 1. Понять intent PR Определи по описанию PR, связанной задаче и conventional-commit префиксу: это bugfix, feature, refactoring, чисто визуальное обновление или breaking change. Не начинай искать проблемы, пока не понял, что автор хотел изменить — иначе легко перепутать intentional change с регрессией. ### 2. Изучить diff Определи: какие пакеты (`packages/`) затронуты, какие props/types изменены, какие public exports изменены, какие стили изменены, какие tests/stories/changeset изменены вместе с кодом. Для каждого затронутого пакета сразу, на этом же этапе (не откладывая до этапа 4) прочитай оба его публичных `index.ts`: `packages//src/index.ts` **и** `packages//src/shared/index.ts`, если он существует. Не делай вывод "компонент не экспортируется, значит изменение внутреннее" на основании одного только корневого `index.ts` — `shared/index.ts` есть у большинства пакетов библиотеки (это установленная, широко используемая конвенция проекта, не редкое исключение) и реэкспортирует часть вложенных `components/*` как равноправную вторую точку входа (подтверждается тем, что сборка через `preserveModules` компилирует каждый файл `src/**/*.ts(x)` в отдельный модуль, включая `shared/index.ts` — так что `@alfalab/core-components-/shared` реально импортируем потребителями). Если по итогам diff'а у тебя есть промежуточный вывод о том, что какой-то изменённый компонент/тип "не публичный", считай его черновым до тех пор, пока не прочитал `shared/index.ts` этого пакета — прочитанное на этапе 4 общее правило нужно **применить повторно** к уже сделанным на этом этапе выводам, а не оставлять их как есть. ### 3. Изучить окружающий код Не ограничивайся diff. При необходимости прочитай: сам компонент целиком, consumer-код (кто ещё в монорепе использует изменённый компонент/утилиту), hooks, `typings.ts`, стили, тесты, stories, changeset. ### 4. Проверить публичный API и совместимость Прочитай `references/public-api.md` и следуй чек-листу оттуда. Проверь: не удаляются/переименовываются ли props, не меняются ли типы, default values, обязательность, callback signatures, поведение существующих props; соответствует ли наличие и severity changeset'а характеру изменения (breaking change требует `major` bump и инструкции миграции). Если на этапе 2 ты уже решил, что изменённый компонент/проп "внутренний" (не экспортируется), и это решение опиралось только на корневой `index.ts` — прежде чем продолжать, подтверди его явной проверкой `shared/index.ts` пакета (см. этап 2). Вывод о том, что что-то не является публичным API, недействителен без этой проверки. ### 5. Проверить accessibility и interaction Прочитай `references/accessibility-interaction.md` и следуй чек-листу оттуда. Проверь semantic HTML, ARIA, keyboard navigation, focus management, pointer interaction для затронутых сценариев. ### 6. Проверить состояния компонента и visual/layout Прочитай `references/states-and-visual.md` и следуй чек-листу оттуда. Проверь релевантные состояния компонента и отличи intentional visual change от unintended regression; проверь controlled/uncontrolled behavior, если применимо. ### 7. Проверить SSR, browser compatibility и performance Прочитай `references/platform-and-performance.md` и следуй чек-листу оттуда. Проверь использование browser-only API в рендере, соответствие поддерживаемым браузерам, потенциальные performance-регрессии в часто используемых компонентах. ### 8. Проверить tests и соответствие project conventions Прочитай `references/tests-and-conventions.md` и следуй чек-листу оттуда. Проверь покрытие тестами изменённого поведения, обновление скриншотов при визуальных изменениях, соответствие "неочевидным правилам" проекта. Затем прочитай `references/code-conventions.md` — соглашения кода, CSS и токенов, собранные из замечаний ревьюеров. ### 9. Проверить каждый potential finding перед публикацией Для каждого кандидата в findings ответь на 6 вопросов (см. «Принцип доказательности» ниже) и определи: вызвана ли проблема именно этим PR, можно ли её доказать кодом, не является ли она субъективной. ### 10. Сформировать итог Включи в результат только подтверждённые и actionable замечания, в формате из раздела «Формат итогового review» ниже. Если существенных проблем нет — прямо так и напиши, не занимайся генерацией замечаний ради количества. ## Приоритеты ревью По убыванию важности: 1. Корректность поведения компонента. 2. Breaking changes и совместимость публичного API. 3. Accessibility. 4. Корректность состояний компонента. 5. Keyboard, focus и pointer interaction. 6. Visual/layout regressions. 7. Controlled/uncontrolled behavior. 8. SSR/hydration и browser/runtime compatibility. 9. Performance. 10. Tests. 11. Maintainability и readability. ## Severity и blocking policy | Severity | Означает | Blocking | | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | | **P0** | Критическая проблема: security vulnerability, массовая поломка компонентов, потеря/повреждение данных, критический a11y-блокер основного сценария | да | | **P1** | Серьёзная проблема: breaking change публичного API без соответствующего changeset, поломка основного сценария компонента, серьёзная accessibility/keyboard regression, SSR/hydration ломается, runtime error в поддерживаемом сценарии, серьёзная visual regression | да | | **P2** | Потенциальная проблема: edge case, проблема отдельного состояния, ограниченная interaction/visual regression, неполное тестирование важного сценария | нет | | **P3** | Улучшение: maintainability, readability, дополнительные тесты, документация — использовать умеренно | нет | **Не блокируй PR только из-за визуального отличия**, если оно явно является целью PR или соответствует обновлению design system — сам факт визуального изменения не является доказательством ошибки. ## Принцип доказательности Для каждого существенного finding нужно уметь ответить: 1. Где находится проблема (файл, строка/диапазон)? 2. Что именно происходит? 3. Почему это является проблемой? 4. При каких условиях она возникает? 5. Каково потенциальное влияние? 6. Какая часть кода подтверждает вывод? Недостаточно написать "здесь может возникнуть race condition" — нужно объяснить механизм и показать подтверждающий код. Если вывод зависит от неизвестного поведения библиотеки/браузера/framework — явно обозначь неопределённость (`confidence: low`) либо не создавай finding вовсе. ### Воспроизведение, которое автор может запустить Для finding'а, подтверждённого запуском, приложи воспроизведение, которое автор выполнит у себя: тест или команду, которые падают на последнем коммите PR (а после исправления должны проходить). Публикуй воспроизведение в том виде, в каком сам его запускал: незапущенное воспроизведение — это предположение. Имена компонентов, props, токенов и файлов переписывай из репозитория буква в букву: по имени из замечания автор ищет, что править. ## Причинная связь с PR Приоритет — проблемы, вызванные текущим PR. ### Была ли проблема до PR Прежде чем ставить severity, проверь, вносит ли проблему именно этот PR: посмотри тот же код в базовой ветке (`git show :<путь>`), а если проблема воспроизводится тестом или командой — запусти то же воспроизведение на базовой ветке. - **До PR работало, после — сломано,** или **проблема в коде, которого до PR не было,** — это предмет ревью, severity по последствиям. - **Существующий код делает новое изменение некорректным** — тоже предмет ревью. - **Проблема была и до PR** в том же виде — это не P0/P1: PR её не вносит, и блокировать его из-за неё нельзя. Такую проблему либо не выносят вовсе, либо дают finding'ом не выше P2 с пометкой «появилась не в этом PR, не блокирует». Если PR заявлен как исправление именно этой проблемы, она — предмет ревью. «Было и до PR» — тоже утверждение: без проверки на базовой ветке оно ничего не доказывает. ## Все места с той же проблемой Почти любая проблема встречается в коде не в одном месте: одна величина подставлена в несколько расчётов, один prop проброшен через несколько компонентов, одна проверка повторена в desktop- и mobile-версии. Если ревью называет только одно место, автор исправит его, а соседнее всплывёт в следующем ревью — понадобится ещё один круг правок. Нашёл проблему в одном месте — поиском по репозиторию найди остальные места с тем же условием, значением или вызовом и проверь каждое. В finding'е: - перечисли все найденные места и отдельно назови те, где всё в порядке; - оформи это одним finding'ом со списком мест; severity — по самому серьёзному из них; - что считать исправлением, формулируй через суть («во всех расчётах верхнего отступа используется одна величина»), а не через номера строк; тест проси один на все места, а не по тесту на каждое. Не проси большего, чем проверил: если проверено три места из десяти — так и напиши, не требуй правки «везде». ## Проверка запуском Описание PR и отмеченные пункты чек-листа — заявление автора, а не факт. Если окружение позволяет запускать команды, проверяй точечно — по затронутым пакетам, а не по всему репозиторию: пакетов больше сотни, полная сборка и линт идут долго и уже выполняются в CI. - Итог долгих проверок (`build`, `screenshot-test`, `search-vars`, `demo`) бери из статусов CI для последнего коммита PR. Упавшая джоба — факт для ревью, но сама по себе ещё не finding: сначала посмотри, что именно упало. - Заявление автора, которое запуск опроверг, — отдельный finding. - В итоге скажи, что проверено запуском и с каким результатом, и отдельно — что проверить не удалось и почему. Если об этом промолчать, читатель решит, что проверено всё. ## Повторное ревью Если по этому PR уже было ревью (агента или человека) и автор ответил — сначала прочитай ответы и исходи из того, что в возражении может быть факт, которого у тебя не было. | Что стало с замечанием | Что делать | | ----------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------ | | Автор принял и поправил | проверь, что правка действительно сделана, и скажи об этом одной строкой; повторно finding'ом не выноси | | Отклонено аргументированно (автором или мейнтейнером) | тема закрыта: не повторяй ни тем же finding'ом, ни переформулированным, ни как P3 | | Обсуждается или осталось без ответа | вернуться можно, но только с **новым аргументом**, отвечающим на возражение по существу. Повтор прошлого текста выглядит так, будто ревьюер не читает ответы | Не повторяй замечание, которое человек уже высказал в этом же PR. Правки, сделанные после прошлого ревью, разбирай как новый код: исправления сами нередко вносят новые ошибки. ## Формат finding ```text Finding ├── severity (P0 / P1 / P2 / P3) ├── location (файл:строка) ├── problem (краткое описание) ├── evidence (объяснение на основе кода/контекста) ├── impact (что произойдёт при возникновении проблемы) ├── suggested_fix (практическое направление исправления) └── confidence (high / medium / low) ``` `confidence: low` обычно не публикуется как finding. ## Формат итогового review ```markdown ## Summary REQUEST_CHANGES Found 2 issues: - 1 P1 - 1 P2 ## Findings ### P1 — Компонент теряет keyboard focus после открытия `packages/select/src/Component.tsx:142` Описание проблемы. **Почему это проблема:** причинно-следственное объяснение. **Влияние:** пользователь клавиатуры не может продолжить навигацию. **Рекомендация:** восстановить ожидаемый focus management. ## Positive observations - Хорошо покрыт keyboard interaction. - Добавлены тесты для нового состояния. ``` Если проблем нет: ```markdown ## Summary APPROVE Существенных проблем в изменениях не найдено. ``` ## False positives и лимит findings - Не спекулируй, не выдавай предположения за факты. - Не придирайся к стилю и вкусовым архитектурным предпочтениям. Исключение — соглашения из `references/code-conventions.md`: их нарушение в новом коде даёт finding P3. - Не создавай несколько findings для одной и той же проблемы. - Не комментируй код без actionable причины. - Не комментируй построчно сгенерированные файлы (`CHANGELOG.md`, `yarn.lock`). Если такой файл меняется без соответствующего изменения в `package.json` или changeset — это повод для вопроса, а не для разбора содержимого. - Не блокируй PR из-за несущественных улучшений. - Отличай намеренное visual изменение от visual regression. - Проверь каждую находку (этап 9) перед тем, как включить её в итог. - Не заполняй review искусственно: если найдено 2 реальные проблемы — покажи 2, а не генерируй дополнительные ради количества. По умолчанию ограничивай вывод `max_findings: 10`; при превышении — приоритет по severity → confidence → impact. ## Доменные чек-листы Детальные, привязанные к конкретному стеку и конвенциям этого проекта чек-листы вынесены в отдельные файлы — читай их на соответствующем этапе процесса (см. выше), не пропускай этот шаг: | Файл | Тема | Когда читать | | ----------------------------------------- | ----------------------------------------------------------------------- | ------------ | | `references/public-api.md` | Публичный API, breaking changes, changesets | Этап 4 | | `references/accessibility-interaction.md` | Accessibility, keyboard/focus/pointer | Этап 5 | | `references/states-and-visual.md` | Состояния компонента, controlled/uncontrolled, visual/layout regression | Этап 6 | | `references/platform-and-performance.md` | SSR/hydration, browser compatibility, performance | Этап 7 | | `references/tests-and-conventions.md` | Tests, CI-авточеки, project-specific "неочевидные правила" | Этап 8 | | `references/code-conventions.md` | Соглашения кода, CSS и токены — из замечаний ревьюеров | Этап 8 | В каждом из этих файлов есть таблица severity для своей области. Там же могут быть реальные примеры из истории проекта — коммиты, diff'ы, цитаты из ревью. Используй их как основной источник конкретики, а не только общие принципы этого файла. Если пользователь явно просит сузить ревью (например, "проверь только accessibility" или "не смотри на performance") — следуй этому вместо прохождения всех этапов 4–8; иначе ревью по умолчанию покрывает все этапы и все severity, с лимитом `max_findings: 10`. ## Примеры хороших и плохих findings Ниже — иллюстративные примеры формулировок (не привязаны к конкретному коммиту, в отличие от примеров в `references/*.md`), показывающие разницу между поверхностным и обоснованным finding'ом. **Плохо — вкусовщина, не проблема:** > Логику пересчёта `activeIndex` в `Tabs` стоит вынести в отдельный хук, так читать компонент будет проще. Это архитектурное предпочтение без демонстрации, что текущий код что-то ломает — не finding (см. «False positives»). **Плохо — visual diff без доказательства:** > В скриншоте `Slider` изменился отступ подписи на несколько пикселей — похоже на регрессию. Сам факт изменённого скриншота ничего не доказывает (см. `references/states-and-visual.md`, «Отличие intentional visual change от regression») — нужно показать, что это не соответствует цели PR, или что в diff нет объясняющего это CSS-изменения. **Хорошо — конкретный keyboard/focus finding (гипотетический, для иллюстрации формата — не привязан к реальному коду `Modal`):** > После закрытия `Modal` через клик по backdrop фокус не возвращается на элемент, открывший модалку — путь закрытия через backdrop не проходит через ту же логику восстановления фокуса, что и закрытие через кнопку/`Escape` (см. соответствующие обработчики в diff). Пользователь, открывший `Modal` с клавиатуры, после закрытия через backdrop теряет позицию навигации и вынужден заново обходить страницу табом. Конкретный механизм (какой путь закрытия не восстанавливает фокус и почему), конкретный impact — а не просто "могут быть проблемы с фокусом". **Хорошо — конкретный API finding (гипотетический, для иллюстрации формата — реальный пример такого рода см. в `references/public-api.md`):** > Union-тип `view` у `Button` сужен — удалён вариант `'tertiary'`, при этом changeset помечен `patch`, а не `major`. Потребители, использующие `view='tertiary'`, получат TS-ошибку компиляции при обновлении, но не получат ожидаемого major-бампа, сигнализирующего о breaking change (см. `references/public-api.md`, правило changeset). Конкретное изменение публичного контракта, конкретное следствие для потребителей, конкретная нестыковка bump-типа.