PR #847 — комментарии и разбор

PR: https://bitbucket.org/stevens_edu/stevens_main_nextjs/pull-requests/847 Контекст: hotfix для FAQPage schema (добавление @id = URL страницы) Branch: ICUS-216-faq (переиспользована после PR #837)

Таймлайн PR

PR opened — пятница 2026-04-24 ("6 days ago" при today=30.04)

Скриншот:
Иван открыл PR
Добавил 1 ревьюера (по аватару — Michael Forbes)
Отредактировал title: ~~ICUS-216 faq~~ → ICUS-216 Implemented faq
Все три действия — в один день, "6 days ago"

Замечание по таймингу: PR #847 открыт 24.04, письмо Alexis — 29.04 (через 5 дней). То есть Alexis на момент письма уже видел PR #847 со всеми его свойствами (включая историю коммитов / squashing — что бы там ни было).

Замечание по title: изначально PR назывался просто "ICUS-216 faq" (формальный код тикета), потом переименован в "ICUS-216 Implemented faq". Это говорит о том, что Иван сначала открыл PR с минимальным шаблонным заголовком, потом сам же дополнил. Не критично, но это маркер того, что PR открывался впопыхах без полной подготовки заголовка/описания.

Зафиксированный фон до PR #847

2026-03-24: PR #837 замержен (FAQPageJsonLd per Accordion)
(после) Обнаружена проблема: множественные FAQPage на одной URL → Google Rich Results error
Michael Forbes предложил решение через @id
Michael Forbes явно предложил переиспользовать ветку ICUS-216-faq
Открыт PR #847

Главный вопрос для разбора

Письмо Alexis (29.04) ссылается на PR #847 как пример commit squashing после расшаривания ветки. Нужно установить:

Был ли squash-merge при PR #837 (стандартный путь через UI)?
Были ли force-push'и в ICUS-216-faq между PR #837 и PR #847?
Был ли локальный rebase/squash перед открытием PR #847?

⏳ Без доступа к локальному репо Ивана мы это сейчас не проверим. Откладываем на встречу завтра — Иван открывает Bitbucket UI и/или локальный git, смотрим вместе. См. чеклист в (Приоритет 2 — squashing/git-история).

PR Description (как Иван оформил PR)

Скриншоты:
- — текст description
- — preview-deploy + Fast Schema Analyzer показывает корректный единственный FAQPage

Текст description (полный)

Summary

Implemented page-level FAQ JSON-LD aggregation to prevent multiple FAQPage schema blocks from being rendered when a page contains more than one FAQ accordion. FAQ accordions now remain responsible only for visual rendering, while page templates collect all FAQ accordion items and output a single consolidated FAQPage structured data block for the current URL.

The fix preserves all FAQ questions and answers across multiple accordions, supports nested component structures where applicable, and applies the same schema behavior consistently across right-nav pages, basic pages, chapter pages, and program detail pages. This improves structured data correctness and avoids duplicate FAQ schema scripts on pages with multiple FAQ sections.

Compare pages: https://www.stevens.edu/page-basic/mobile-duckcard-faqs https://stevens-site-redesign-git-icus-216-faq-stevens.vercel.app/page-basic/mobile-duckcard-faqs

Что показывает второй скрин (Fast Schema Analyzer от Vryse)

URL: stevens-site-redesign-git-icus-216-faq-stevens.vercel.app/page-basic/mobile-duckcard-faqs

Schemas Detected: ✓ FAQPage (один объект)

{
  "@context": "https://schema.org",
  "@type": "FAQPage",
  "@id": "https://stevens-site-redesign-git-develop-stevens.vercel.app/page-basic/mobile-duckcard-faqs#faq",
  "mainEntity": [
    { "@type": "Question", "name": "Do I need the Mobile DuckCard?", "acceptedAnswer": {...} },
    { "@type": "Question", "name": "My mobile has been lost/stolen/damaged, what do I do?", ... },
    ...
  ]
}

→ На странице действительно генерируется один FAQPage с правильным @id.

Что важно отметить (расхождение с предложением Michael)

Michael Forbes предлагал shortcut: оставить отдельные FAQPage на каждый Accordion, добавив одинаковый @id всем — чтобы Google трактовал их как ссылки на одну сущность. Прямая цитата:

"we will be compliant... if we generate separate JSON-LD for each Accordion component as we currently do... if we just do one simple thing: add an @id property to each of the FAQPage objects, where its value is the same for each object... no need to merge the question/answer pairs which would be quite difficult"

Что в PR реализовано (по тексту description; подход aggregation — согласован командой: Саша Федоренко, менеджер, Иван):

"Implemented page-level FAQ JSON-LD aggregation... page templates collect all FAQ accordion items and output a single consolidated FAQPage structured data block"

То есть в PR взят более сложный путь, чем shortcut Michael:

✓ Аггрегация на уровне page template (вместо отдельных JSON-LD per accordion).
✓ Плюс ещё добавлен @id (видно в скрине Fast Schema Analyzer).

Это значит:

Page-level aggregation — согласованное командное решение (Саша Федоренко, менеджер, Иван); в PR Иван реализовал более качественное и правильное решение — один консолидированный объект.
Решение @id (shortcut Michael) сохранено как "запасной круг" — но оно тут уже не первичный механизм, потому что в любом случае один JSON-LD теперь.
Аргумент "это сложно" (как писал Michael) опровергнут реализацией aggregation на уровне page template — она работает.

Это важный плюс к репутации Ивана для встречи

Решение архитектурно правильнее того, что предлагал Michael.
Покрывает все типы страниц: right-nav / basic / chapter / program detail.
Поддерживает nested component structures.
В description чётко описано что и зачем.
Preview-deploy с готовым тестом (Fast Schema Analyzer) вложен в description — клиенту не надо проверять самому, всё уже валидировано.

Что не отвечает на претензию Alexis из письма

Письмо Alexis касалось процесса (squashing после расшаривания ветки), а не качества фикса. То есть:

Технически PR #847 хорошее.
Процессно — претензия про squashing остаётся, нужно установить факт через git log.

Comment #1 — Bemin Shaker: aggregation пропускает `bottomComponentsCollection` - Скриншот:

Файл: components/seo-json-ld/faq-page-json-ld.js
Автор: Bemin Shaker
Дата: "2 days ago" (~вторник 2026-04-28)

Код в PR:

(components || []).filter(Boolean).flatMap(component => {
  const questions = isFaqAccordion(component) ? getAccordionQuestions(component) : []
  const nestedQuestions = collectFaqQuestionsFromComponents(
    component?.componentsCollection?.items
  )
  ...
  return [...questions, ...nestedQuestions]
})

Цитата:

This doesn't take into account Right Nav and Program Detail pages which have componentsCollection (displayed to the left of the sidebar on wide viewports) AND bottomComponentsCollection (displayed at full-width). If an FAQ accordion is added in the full-width area, it would be excluded from the FAQPage object as a result.
Суть: Иван в flatMap рекурсивно собирает FAQ-аккордеоны из componentsCollection, но не из bottomComponentsCollection. На страницах Right Nav и Program Detail есть оба контейнера — full-width FAQ-секции (внизу) молча не попадают в JSON-LD.
Реакция Ивана:

Ivan.Kotovsky resolved this comment thread.

→ судя по индикатору в треде — Иван закрыл/исправил.
Обоснованно? Да. Это реальный gap покрытия, прямо противоречащий тому, что Иван заявил в description PR:

"applies the same schema behavior consistently across right-nav pages, basic pages, chapter pages, and program detail pages"

То есть в description обещано полное покрытие, в коде — нет. Это типичный кейс расхождения декларации и реализации, и именно тот класс проблем, на который Alexis жалуется в письме.
Серьёзность: Высокая — функциональный gap на двух важных типах страниц. К счастью, Bemin поймал это на ревью, не на проде.
Связь с письмом Alexis:
- deviate from spec — да, заявленное поведение шире фактического.
- internal validation pass — да, должен был сам Иван прогнать на right-nav / program detail страницах с FAQ в bottom-области и убедиться, что попадает.
Что важно для встречи (нюансы):
1. Иван сам в description обозначил правильный объём ("right-nav / basic / chapter / program detail") — то есть знал, что нужно покрыть.
2. Но реализация на момент открытия PR не покрывала bottomComponentsCollection.
3. Bemin закрыл этот gap на ревью — это нормальная роль ревьюера. Но гэп не должен был дойти до ревью при правильном self-QA.
Вопрос Ивану: тестировал ли страницу Right Nav или Program Detail с FAQ-аккордеоном в bottom-области до открытия PR? Если да — почему не заметил, что не попал в JSON-LD? Если нет — почему не покрыл этот сценарий, зная про два контейнера?

Comment #2 — Vercel bot + апдейт 87d2cde

Скриншот:
Дата: "yesterday" (~2026-04-29)
Vercel deploy: Ready, last update Apr 30, 2026 10:20am UTC
Иван запушил коммит 87d2cde
Не претензия, служебный.

Comment #3 — Michael Forbes: workflow-нарушение (commit после approve) - Скриншот:

Файл: components/seo-json-ld/faq-page-json-ld.js
Автор: Michael Forbes
Дата: Michael — "2 days ago" (~28.04), Иван ответил "yesterday" (~29.04)
Что было в коде:
```
export const collectFaqQuestionsFromComponents = components =>
  (components || []).filter(Boolean).flatMap(component => {
    // const questions = isFaqAccordion(component) ? getAccordionQuestions(component) : []
    // remove and uncomment const questions when approve PR
```
То есть Иван закоммитил закомментированный код с TODO-инструкцией:

"remove and uncomment const questions when approve PR"

→ Иван намеренно оставил placeholder, планируя вернуться и поменять код после получения approve.
Цитата Michael:

The sequence we use to ensure accurate approvals is {commit → approve → merge}, not {commit → approve → commit → merge}. What is the reason we need to make another change to the code after approving?
Реакция Ивана:

I added comment and code to check changes. I removed my comment and added correct code.

→ удалил TODO-комментарий, поставил правильный код.
Это критическое процессное нарушение:

Michael прямо называет workflow-правило: ревьюеры утверждают конкретное состояние кода. Если код меняется после approve — approve становится недействительным, ревьюер должен пересмотреть.

То, что сделал Иван (запланировал изменить код после approval) — это обходной приём:
- Получить approve на "временной" версии.
- Подменить код после approval.
- Получается approve на код, которого не было в момент approval.
Это прямо подрывает доверие к процессу review, и Michael это явно зафиксировал.
Связь с письмом Alexis: очень прямая. Письмо говорит про "ensure accurate approvals" косвенно — через:

"the use of commit squashing... can make minor adjustments very difficult to parse out"

"team has to review all of it once more to ensure nothing is missed, as opposed to focusing on what has changed"

Тут история не про squash, но про тот же класс проблем: Иван манипулирует процессом ревью (commit → approve → commit → merge), что противоречит тому, как клиент хочет работать.
Серьёзность: Это самое серьёзное процессное нарушение, что мы видели в всех PR.
- Squash после расшаривания (PR #837/847) — нарушает читаемость истории.
- Это нарушение — подрывает сам смысл approve.
Что важно для встречи:
- Это нельзя защитить. Не "недопонимание", не "плохой английский", не "пробел в спеке". Это сознательное намерение изменить код после approve, оставленное прямо в комментарии.
- Иван исправил, когда указали — поправил. Это в плюс.
- Но сам факт того, что подобная конструкция дошла до PR — знак, что у Ивана нет понимания, что approve = утверждение конкретного состояния кода.
- Главный вопрос Ивану: ты понимаешь, что approve привязан к коммиту? Если ты планируешь менять код после approve — нужно повторное ревью. Это базовое правило git/PR-workflow.
Это нужно отдельно отразить во встрече с Антоном:
- Критично: заводим правило — никаких "TODO when approve" комментариев в коде PR.
- Если код не готов — PR в draft, а не в Open.
- Если после approve нужны изменения — повторно просим re-approve.

Comment #4 — Bemin Shaker: `NEXT_PUBLIC_SITE_URL` всегда undefined? - Скриншот:

Файл: components/seo-json-ld/faq-page-json-ld.js
Автор: Bemin Shaker
Дата: "yesterday" (~29.04)