Retro

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

PR: https://bitbucket.org/stevens_edu/stevens_main_nextjs/pull-requests/832 Задача: IX-SIT60 — Add schema for pages

Таймлайн PR (детальный)

ДатаСобытие
2026-02-17В задаче (Jira IX-SIT60) доступна спека 📄Stevens_Schema_Templates.odt
2026-02-19Старт работ по задаче
2026-02-21Иван открыл PR
2026-02-22Иван — 2 коммита 431f32a, d220ebb
2026-02-22Vercel deploy ready (last update Apr 24)
2026-02-22Иван — коммит 4e71733
2026-02-23Alexis Watson — поставил hold (ждать решение UA по uploadDate) + спросил про pre-deployment steps
2026-02-24Bemin Shaker — Minisite Right Nav pages не включены в PR
2026-02-24Иван ответил Alexis: "no additional steps required before deployment"
2026-02-24Иван — 3 коммита 57ffbf8, dcd1005, e4df8e0 (добавил page-right-nav)
2026-02-24Bemin Shaker — contact pages: PostalAddress данные в rich text, нельзя вытащить
2026-03-03Иван — 1 коммит 4f3cd42
2026-03-04Иван — 4 коммита 9c32a31, b83c3d5, f6eb700, 41c11cb
2026-03-06Иван — 4 коммита 0df4d04, 83871a8, 77b0306, d6527a6
2026-03-20Иван — 3 коммита b4e20b5, 34ca0f0, 8327de7
2026-03-23Иван — 4 коммита be40619, c47be27, bf978fc, fbe52f5
2026-03-30Иван — 5 коммитов 9b105b1, 8b94d29, 8cb6100, f079a9e, 2ed7958
2026-03-31Michael Forbes — комментарий по <VideoJsonLd> (hero video, invalid без uploadDate)
2026-03-31Michael Forbes — REQUESTED CHANGES на PR
2026-04-01Иван: "Removed video scheme from main page" + коммит ec6b8eb
2026-04-03Michael — комментарий по video-with-caption.js (whitespace-only)
2026-04-03Michael — комментарий по featured-video.js (whitespace-only, конфликтует с другим PR)
2026-04-06Иван: "I removed whitespaces from video components" + 3 коммита 3787977, 6274a22, 91fbec7
2026-04-08Bemin Shaker — 4 комментария: undefined base URL, hardcoded /news/, hardcoded creditText (events), hardcoded creditText (program)
2026-04-24Vercel last deploy update

Сводка по объёму

  • Длительность: ~2 месяца (21.02 → как минимум 24.04, всё ещё OPEN)
  • Коммитов всего (по таймлайну): не менее 27 в 9 батчах апдейтов
  • Авторы ревью: Bemin Shaker, Michael Forbes, Alexis Watson
  • REQUESTED CHANGES: да (Michael Forbes, 31.03)
  • Замержен? Нет на момент скринов

Это самый большой и проблемный PR из разобранных. По всем параметрам прямо иллюстрирует все 4 пункта письма Alexis.

Comment #1 — Bemin Shaker: не включён тип Minisite Right Nav - Скриншот:

  • Автор: Bemin Shaker

  • Дата: 2026-02-24

  • Цитата:

    Some of the examples in the schema templates doc are the Careers at Stevens and Undergraduate Scholarships & Aid pages which are Minisite Right Nav pages. However, it looks like that page type is not included in this PR.

  • Суть: Bemin указал, что в спеке (schema templates doc) фигурируют примеры Careers at Stevens и Undergraduate Scholarships & Aid — это страницы типа Minisite Right Nav. Этот тип страниц не был включён в первоначальный PR.

  • Реакция Ивана:

    For page-minisite-landing was added in first commit. I added for page-right-nav right now. And this page Undergraduate Scholarships & Aid looks like page-chapter and was added in first commit.

    Иван дополнил page-right-nav (3 коммита 24.02).

  • Обоснованно? Да — пропущен тип страниц, явно фигурирующий в примерах спеки.

  • Что важно: промах — полнота покрытия типов страниц в первой версии PR (Minisite Right Nav из примеров в schema templates doc), а не отсутствие спеки: материалы были доступны. Иван исправил оперативно.

Comment #2 — Alexis Watson: hold + вопрос про pre-deployment steps - Скриншот:

  • Автор: Alexis Watson (та же, что писала письмо 29.04)

  • Дата: 2026-02-23

  • Цитата:

    Stevens Platform: Approval and deployment here is to be held until UA determines how they want the upload date to be handled (either as a field or omitted entirely).

    @Ivan.Kotovsky @Anton Moskalenko and iCrossing: Are there any additional steps we need to take prior to deployment? This would include any changes to Contentful fields and the like.

  • Суть:

    • PR заморожен пока UA (University Advancement) не решит, как обрабатывать uploadDate.
    • Alexis задаёт прямой вопрос про любые pre-deployment шаги (в т.ч. поля Contentful).

  • Реакция Ивана (2026-02-24):

    At this point, no additional steps are required before deployment. All schema changes have been implemented on the front end. We are not adding any new mandatory fields in Contentful as part of this iteration.

  • Обоснованно ли требование Alexis? Да — прямой запрос процессной информации, ровно та же тема, что в письме 29.04 ("PRs that require post-deployment tasks always accompanied by such").

  • Иван ответил, что доп.шагов нет. Это надо независимо проверить:

    • Действительно ли PR не требует ничего в Contentful?
    • Не происходит ли тут история, аналогичная PR #842, где "доп.шагов нет" обернулось сломанным build'ом у ревьюера?

  • КЛЮЧЕВОЕ ДЛЯ ВСТРЕЧИ: Alexis уже 23 февраля прямо спрашивал про pre-deployment steps. То, что 2 месяца спустя в письме он снова просит "PRs require post-deployment tasks accompanied by such" — означает, что этот сигнал уже звучал и не был системно отработан. Это критичный момент: проблема не новая для нас, нас на неё указывали явно.

  • Внутренний контекст: я (Андрей) несколько раз поднимал эту тему — говорил и писал в Slack, что нас панчат за это. Точные даты найти не получилось.

Comment #3 — Bemin Shaker: contact pages, PostalAddress в rich text - Скриншот:

  • Автор: Bemin Shaker

  • Дата: 2026-02-24

  • Цитата:

    One observation: For contact pages like https://www.stevens.edu/contact-stevensonline, the examples show a PostalAddress object, but that information is inside a rich text field (not a separate field which would make it possible to pull that into the schema).

  • Суть: для contact pages в спеке предусмотрена PostalAddress schema. Но в Contentful адрес лежит внутри rich text field, а не как отдельные поля → программно вытащить в schema нельзя без парсинга текста.

  • Это не вина Ивана — это структурное ограничение модели Contentful. Скорее observation/блокер.

  • Что нужно: уточнить, было ли решение по этому пункту (отдельные поля в Contentful? skip PostalAddress для contact pages?).

Comment #4 — Michael Forbes: invalid VideoJsonLd для hero/ambient video → REQUESTED CHANGES - Скриншот:

  • Файл: pages/index.js

  • Автор: Michael Forbes

  • Дата: 2026-03-31

  • Действие на PR: REQUESTED CHANGES

  • Цитата:

    I think Michelle Strauss mentioned that a VideoObject is invalid if it doesn't have an uploadDate property. If that's true, we should not have this <VideoJsonLd> for the ambient/hero video, right?

    I realize this is just refactoring the old JSON-LD which had the same problem of a missing uploadDate, but we need to make sure that the new JSON-LD is clean.

  • Суть: точно та же проблема, что и в PR #842 (Comment #3 там) — VideoObject без uploadDate невалидный. Здесь это касается hero/ambient video на главной.

  • Реакция Ивана (2026-04-01):

    For the background video, we can add the upload date if necessary, provided it's important. But since there's no date for it, we can just remove it.

    Убрал VideoJsonLd для main page, коммит ec6b8eb.

  • Обоснованно? Да, абсолютно. И это уже второй раз Michael ловит ровно ту же ошибку у Ивана (первый — в PR #842).

  • КЛЮЧЕВОЕ: повторяющаяся ошибка — это паттерн, не разовый случай. Иван не усвоил урок про валидность schema.org-объектов между PR.

  • REQUESTED CHANGES на PR — это формальный блокер мержа, не просто комментарий. Серьёзная процессная отметка.

Comment #5 — Michael: whitespace-only изменения, убрать конфликт с другим PR - Скриншоты:

  • (video-with-caption.js)

  • (featured-video.js)

  • Автор: Michael Forbes

  • Дата: 2026-04-03

  • Цитата (на featured-video.js):

    Since this only changes whitespace without doing anything else, I think we should eliminate this change from this PR, so that we don't create an unnecessary conflict with the more meaningful change to these same lines in that other PR.

  • Суть: Иван внёс изменения, которые не несут логики — только форматирование пробелов в JSX. Эти строки конфликтуют с другим PR, где те же строки реально меняются по смыслу. Michael просит убрать noise-changes.

  • Реакция Ивана (2026-04-06):

    I removed whitespaces from video components, if I need to add something else, I ready to fix it

    Поправил.

  • Обоснованно? Да. Это аккуратность работы с git history — лишние косметические правки в больших PR создают конфликты и шум.

  • Связь с письмом Alexis: прямо — про "rebase / commit squashing" / историю, которую читают ревьюеры. Шумные diff'ы усложняют ревью.

Comment #6 — Bemin Shaker: undefined base URL в JSON-LD - Скриншот:

  • Файл: pages/profile/[slug].js

  • Автор: Bemin Shaker

  • Дата: 2026-04-08

  • Код в PR:

    url={getExternalPath(PageProfile.typeName, slug, true)}

  • Цитата:

    The base URL is being returned as undefined. So the full property is something like "url":"undefined/profile/fkim" as seen on this page.

  • Суть: функциональный баг. В сгенерированном JSON-LD на проде уже видно "url":"undefined/profile/fkim" — то есть getExternalPath возвращает строку с undefined в начале вместо реального base URL. Это попало на production preview.

  • Обоснованно? Да, критично. Невалидные URL в schema.org — это поломка SEO-фичи в продуктовом смысле. И Bemin поймал это только спустя 1.5 месяца работы над PR.

Comment #7 — Bemin Shaker: hardcoded /news/ префикс - Скриншот:

  • Файл: pages/news/[slug].js

  • Автор: Bemin Shaker

  • Дата: 2026-04-08

  • Код в PR:

    <WebPageJsonLd
  name={title}
  url={`/news/${slug}`}

  • Цитата:

    URL structure is subject to change, so we should not be hardcoding the /news/ prefix here. It's best to use the getExternalPath function here.

  • Суть: хардкод URL-префикса — нарушение существующей конвенции (есть getExternalPath для централизованного формирования URL). Это знание codebase, которым Иван не воспользовался.

  • Обоснованно? Да. Bemin указывает на нарушение принятой в проекте абстракции.

Comment #8 + #9 — Bemin Shaker: hardcoded creditText для ImageJsonLd (2 места) - Скриншоты:

  • (pages/events/[slug].js)

  • (pages/program/[slug].js)

  • Автор: Bemin Shaker

  • Дата: 2026-04-08

  • Код в PR (оба файла):

    <ImageJsonLd
  contentUrl={image?.image.url}
  creditText='Stevens Institute of Technology'

  • Цитата:

    The Image Wrapper component has a credit field, so we probably should be using that here.

  • Суть: Иван захардкодил creditText='Stevens Institute of Technology' вместо использования существующего поля credit из компонента Image Wrapper. Это значит, что:

    • Если у изображения есть свой автор / источник — он не попадёт в schema.
    • При смене бренда придётся править все места.

  • Обоснованно? Да. Это снова незнание/неиспользование существующих абстракций в codebase, и в двух местах сразу.

  • Что важно: Bemin указал на это в двух разных файлах одного PR в один день — это значит, паттерн ошибки повторяющийся внутри одного PR.