Skip to content

fix(chat): drop viewport-height spacer when no anchor (Codex feedback on #32583)#32596

Merged
dvargasfuertes merged 1 commit into
mainfrom
apollo/avatar-no-viewport-spacer-when-no-anchor
May 29, 2026
Merged

fix(chat): drop viewport-height spacer when no anchor (Codex feedback on #32583)#32596
dvargasfuertes merged 1 commit into
mainfrom
apollo/avatar-no-viewport-spacer-when-no-anchor

Conversation

@vellum-apollo-bot
Copy link
Copy Markdown
Contributor

Follow-up to #32583 addressing both Codex review comments. One was a legitimate bug; the other was a misread of React reconciliation, locked in as a regression test instead.

Codex P2 #1 — Legitimate bug, fixed

Avoid auto-pinning below assistant-only history. The scroll coordinator auto-pins to the bottom on conversation switches, so assistant-only histories/onboarding content land just above the viewport and the user sees blank space plus the avatar instead of the latest message.

The merged code applied style={{ minHeight: viewportMinHeight }} to the latest-edge wrapper whenever renderAvatar was present — even with partition.anchorMessage === null (assistant-only history: recovered conversations whose user message was lost, onboarding before the first submit).

useViewportMinHeight reports the scroll container's clientHeight ≈ a full viewport. refactor(web): scroll to bottom on transcript container DOM attach (#32239) snaps the scroll to bottom on conversation switch. With no anchor at the top of the spacer, the bottom-pinned viewport lands on blank space + the avatar — the actual latest assistant message sits one viewport above and is invisible until the user scrolls up.

Fix: gate both minHeight: viewportMinHeight and the flex-1 spacer on partition.anchorMessage. Without an anchor, the avatar renders inline directly below the last history item.

Codex P2 #2 — Misread, locked in as a regression test

Preserve the avatar when the first turn is inserted. React reconciles the following <div>s by index, so the current avatar wrapper is reused as the spacer and ChatAvatar unmounts/remounts, replaying the entrance animation this change is trying to avoid.

Empirically verified that this does not happen. React's reconcileChildrenArray tracks fiber.index (the previous render's position in the children array, including false-skipped slots). When slot 0 transitions from false to <LatestTurnRow>, the existing avatar fiber at fiber.index=2 still matches newIdx=2 in the next render — same <div> type, fiber reused, ChatAvatar is not unmounted.

Locked in with a DOM-lifecycle regression test that:

  1. Renders the transcript with no anchor + a mount-tracking avatar.
  2. Asserts mount=1, unmount=0.
  3. Rerenders with the first user message inserted (no-anchor → anchor).
  4. Asserts mount=1, unmount=0 (no remount).
  5. Rerenders back to no-anchor.
  6. Asserts mount=1, unmount=0 again.

A sanity check (forcing a conversationId change, which re-keys the scroll container) was used to confirm the mount tracker actually detects remounts when they happen (mount=2, unmount=1). The test framework can fail; the assertions are real.

Tests

  • 13 transcript.test.tsx tests pass.
  • 3 new Codex feat: initialize Next.js app in /web directory #1 cases: no-anchor wrapper has no min-height; anchor wrapper has min-height; anchor-without-avatar still has min-height (regression guard for the original viewport-pinning behavior).
  • 1 new Codex feat: add platform terraform for GKE deployment #2 case: the mount-tracker test above.
  • Typecheck clean (tsc --noEmit --skipLibCheck in apps/web).
  • ESLint clean on touched files.
  • 4 pre-existing transcript-message-body.test.tsx failures in the directory-wide run reproduce on main too — caused by mock bleed between test files, not by this PR.

Why this is a follow-up rather than amending the original

#32583 was already merged before Codex's review surfaced. Per the SE-skill pr-lifecycle.md rule on bot reviewers being real reviewers, every comment gets a response — either an iteration or a rationale. This PR is both: iterates on #1, rationalizes #2 with empirical evidence.

… on #32583)

Two pieces of Codex feedback on #32583. One was a legitimate bug, the
other a misread of React reconciliation that this PR locks in as a
regression test.

## Codex #1 — Legitimate bug, fixed

The merged code applies `minHeight: viewportMinHeight` to the
latest-edge wrapper whenever `renderAvatar` is present, even when
`partition.anchorMessage` is null (assistant-only history: recovered
conversations whose user message was lost, onboarding before the
first submit).

`useViewportMinHeight` reports the scroll container's `clientHeight`
≈ a full viewport. `refactor(web): scroll to bottom on transcript
container DOM attach (#32239)` snaps the scroll to bottom on
conversation switch. With no anchor at the top of the spacer, the
bottom-pinned viewport lands on blank space + the avatar — the actual
latest assistant message sits one viewport above and is invisible
until the user scrolls up.

Fix: gate both `minHeight: viewportMinHeight` and the `flex-1`
spacer on `partition.anchorMessage`. Without an anchor, the avatar
renders inline directly below the last history item.

## Codex #2 — Misread, locked in as a regression test

Codex claimed that inserting `<LatestTurnRow>` at slot 0 (was
`false`) on the no-anchor → anchor transition would cause React to
reconcile the following `<div>` siblings by index, reusing the
current avatar wrapper as the spacer and remounting `ChatAvatar` —
replaying the entrance-spring animation #32583 is trying to avoid.

Empirically verified that this does not happen. React's
`reconcileChildrenArray` tracks `fiber.index` (the previous render's
position in the children array, INCLUDING `false`-skipped slots). When
slot 0 transitions from `false` to `<LatestTurnRow>`, the existing
avatar fiber at `fiber.index=2` still matches `newIdx=2` in the
next render — same `<div>` type, fiber reused, `ChatAvatar` is
not unmounted.

Locked in with a DOM-lifecycle regression test that:
  1. Renders the transcript with no anchor + a mount-tracking avatar.
  2. Asserts mount=1, unmount=0.
  3. Rerenders with the first user message inserted (no-anchor → anchor).
  4. Asserts mount=1, unmount=0 (no remount).
  5. Rerenders back to no-anchor.
  6. Asserts mount=1, unmount=0 again.

A sanity-check version of the test (forcing a conversationId change,
which re-keys the scroll container) was used to verify the mount
tracker actually detects remounts — confirmed mount=2/unmount=1
when remount really happens, so the assertions can fail.

## Tests

- 13 transcript.test.tsx tests pass.
- 3 new Codex #1 cases: no-anchor has no min-height; anchor has
  min-height; anchor-without-avatar still has min-height (regression
  guard for the original viewport-pinning behavior).
- 1 new Codex #2 case: the mount-tracker test above.
- Typecheck clean (`tsc --noEmit --skipLibCheck` in `apps/web`).
- ESLint clean on touched files.
- 4 pre-existing `transcript-message-body.test.tsx` failures in the
  directory-wide run are also present on `main` — caused by mock
  bleed between test files, not by this PR.
@dvargasfuertes dvargasfuertes merged commit 2e2be53 into main May 29, 2026
7 checks passed
@dvargasfuertes dvargasfuertes deleted the apollo/avatar-no-viewport-spacer-when-no-anchor branch May 29, 2026 19:31
@vellum-apollo-bot vellum-apollo-bot Bot restored the apollo/avatar-no-viewport-spacer-when-no-anchor branch May 29, 2026 19:36
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.

1 participant