Skip to content

fix(conversation): make the top scroll FAB an overlay instead of a blocker - #476

Merged
RonenMars merged 1 commit into
land/integration-prepfrom
fix/top-fab-overlap
Aug 1, 2026
Merged

fix(conversation): make the top scroll FAB an overlay instead of a blocker#476
RonenMars merged 1 commit into
land/integration-prepfrom
fix/top-fab-overlap

Conversation

@RonenMars

@RonenMars RonenMars commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Problem

The ↑ Top FAB in the conversation detail view appears once you scroll past ~200px, horizontally centered at the top of the list, and sits on top of message content.
It was fully opaque, so text behind it was unreadable, and it intercepted touches in its rect.
Message bubbles span nearly the full width and the pill is centered, so it lands squarely on the text column rather than in a margin.

Occlusions captured during the #470 investigation: …write the spec doc for th[↑ Top]n plan. and | Cypress | Testing | s[↑ Top]ntend | strong |.

Root cause

ConversationHistoryList was the odd one out among three sibling implementations of the same affordance.

background show/hide wrapper
ConversationHistoryList (before) theme.text.accent, opaque conditional mount/unmount, no fade none
ConversationList rgba(31, 111, 235, 0.14) + rgba(88, 166, 255, 0.2) border Animated.View + useAnimatedStyle opacity, 220ms withTiming pointerEvents="box-none"
TerminalOutput rgba(31, 111, 235, 0.18) same animated pattern pointerEvents="box-none"

The two working versions use a translucent chip with muted text that reads as an overlay you can see through, and they fade rather than pop.
ConversationHistoryList used a solid accent-blue pill with white bold text, which reads as a modal blocker.

Fix

ConversationHistoryList now matches its two siblings rather than introducing a third visual language: translucent background, muted 12px text, Animated.View + pointerEvents="box-none" wrapper, 220ms fade in/out.
The show/hide condition moves from a useState toggle to a shared value, so the pill is always mounted and fades instead of popping.
pointerEvents tracks opacity inside the same animated style, so a faded-out pill cannot intercept list touches.

This is fixed purely as an overlay.
There are no contentContainerStyle padding, drawDistance, maintainVisibleContentPosition, or getItemType changes — those lines are load-bearing for the FlashList v2 anchoring stabilized in #470, which measured content-height swings up to ±19,898pt causing viewport teleports.
Padding that toggles with FAB visibility would mutate content height mid-scroll and reintroduce that class of bug.

The bottom CaretDown FAB is untouched; it sits bottom-right in the gutter and was not reported.

Before / after

Same fixture conversation, same scroll offset, driven by an identical scripted gesture sequence on a booted simulator against two Release builds.
In the before frame the table row reads | Express | Language | s[↑ Top] backend |.
In the after frame the same row reads | Express | Language | senior | backend | straight through the pill.

Before After

Accessibility labels

The top FAB's accessibilityLabel was a hardcoded English "Scroll to top" while the bottom FAB directly below it already used t('action.scrollToBottom').
The same gap existed in ConversationList ("Scroll to top", "Scroll to bottom") and TerminalOutput ("Jump to top", "Jump to bottom"), five hardcoded strings in total.
New nav.scrollToTop and nav.scrollToBottom keys land in common.json for en, he, ru, and ar, since all three components already load that namespace.
The existing conversation.json action.scrollToBottom key stays in use by the bottom FAB, so no key is orphaned.
The terminal labels change wording from "Jump" to "Scroll" so the announced label is consistent app-wide, and TerminalOutput.test.tsx is updated to match.

Verification

e2e/07_conversation_scroll_gaps.yaml gains a closing block that scrolls back down past the 200pt threshold, waits for the FAB by testID, taps it, and asserts SCROLL-GAPS-TOP-MARKER (the first message) is visible.
That proves the control is still hittable and still scrolls to the top after the restyle, which is a real risk now that the pill is always mounted: an opacity-0 overlay stuck on pointerEvents: auto would swallow list touches, and one that never regains auto would stop working.
All of the flow's existing assertions are intact, including the SPEC-MARKER duplicate-append guard from #470.
The flow already runs in test:e2e:mock, so no script wiring changed.

The full flow passes end to end on a Release build.

  • npm run lint — 0 errors, 5 warnings, all pre-existing and none in the touched files.
  • Conversation and terminal Jest suites — 26 suites, 277 tests, all passing.
  • npm run test:i18n — 3 suites, 55 passed, 1 skipped.

Two pre-existing conditions on this base, neither introduced here.
npx tsc --noEmit reports 14 errors, and the count is identical on the base branch with these changes stashed.
npm run lint:i18n fails its --max-warnings=0 gate on a react-hooks/exhaustive-deps warning in app/session/[id].tsx, and it fails identically on the base branch.

Follow-ups, not in this PR

All three copies of this FAB are now near-identical and could be extracted into one shared component.
Three near-identical copies is a pre-existing condition, so that is deliberately left alone here.

LiveConversationView renders its own inline FlashList and was noted in #470 as deserving the same list-prop treatment; it has no top FAB and is unaffected by this change.

@RonenMars
RonenMars force-pushed the land/integration-prep branch from d343bd5 to 4b309f3 Compare August 1, 2026 10:34
…ocker

The `↑ Top` pill in the conversation detail view is horizontally centered over the message column, and message bubbles span nearly the full width, so it lands on the text rather than in a margin.
It was a solid accent-blue pill with opaque white bold text, conditionally mounted with no fade, which made it read as a modal blocker: `| Cypress | Testing | s[↑ Top]ntend | strong |`.

Its two sibling implementations already solve this correctly.
`ConversationList` and `TerminalOutput` both use a translucent chip with muted text inside an `Animated.View` that fades opacity over 220ms and wraps it in `pointerEvents="box-none"`.
This brings `ConversationHistoryList` in line with them rather than inventing a third visual language.

The show/hide condition moves from a `useState` toggle to a shared value, so the pill fades instead of popping and is always mounted.
`pointerEvents` tracks opacity in the same animated style, so a faded-out pill cannot intercept list touches.

Fixed purely as an overlay.
No `contentContainerStyle` padding, `drawDistance`, `maintainVisibleContentPosition`, or `getItemType` changes — those are load-bearing for the FlashList v2 anchoring stabilized in #470, and padding that toggles with FAB visibility would mutate content height mid-scroll.

Also localizes the five hardcoded English accessibility labels on these FABs.
`ConversationHistoryList` had `"Scroll to top"`, `ConversationList` had `"Scroll to top"` and `"Scroll to bottom"`, and `TerminalOutput` had `"Jump to top"` and `"Jump to bottom"`, all while the bottom FAB one line below already used a translation key.
New `nav.scrollToTop` and `nav.scrollToBottom` keys land in `common.json` for en, he, ru, and ar, since all three components already load that namespace.
The terminal labels change wording from "Jump" to "Scroll" so the label is consistent app-wide; `TerminalOutput.test.tsx` is updated to match.

`e2e/07_conversation_scroll_gaps.yaml` gains a closing block that scrolls back down past the 200pt threshold, asserts the FAB by `testID`, taps it, and requires the first message to be visible.
All existing assertions are untouched, including the `SPEC-MARKER` duplicate-append guard from #470.
@RonenMars
RonenMars force-pushed the fix/top-fab-overlap branch from 32a4134 to ad16c92 Compare August 1, 2026 10:50
@RonenMars
RonenMars merged commit a9f15ac into land/integration-prep Aug 1, 2026
10 checks passed
@RonenMars
RonenMars deleted the fix/top-fab-overlap branch August 1, 2026 11:00
RonenMars added a commit that referenced this pull request Aug 1, 2026
…audit [skip-ci]

Both existed only as untracked files in the working tree, so neither survived a fresh clone.

The scroll-FAB plan covers the extraction that #476 deferred when the top-FAB overlay fix made the third copy converge on the other two, and no commit since has done it.
`docs/superpowers/plans/` is where the repo already keeps active plans, so it goes there alongside the other fourteen.

The audit inventories user-facing strings in `app/` and `components/` that are not routed through `react-i18next`.
The i18n CI job enforces locale key parity rather than coverage, so nothing else in the repo records which strings are still hardcoded.
Its line numbers will drift, but the inventory itself is still accurate — spot-checked against `components/ui/FAB.tsx` and `components/ui/AvatarMenu.tsx`.

[skip-ci]
RonenMars added a commit that referenced this pull request Aug 1, 2026
…audit (#486)

Both files existed only as untracked files in a working tree, so neither survived a fresh clone.

The scroll-FAB plan covers the extraction that #476 deferred when the top-FAB overlay fix made the third copy converge on the other two, and no commit since has done it.
`docs/superpowers/plans/` is where the repo already keeps its active plans.

The audit inventories user-facing strings in `app/` and `components/` that are not routed through `react-i18next`.
The i18n CI job enforces locale key parity rather than coverage, so nothing else in the repo records which strings are still hardcoded.
Its line numbers will drift, but the inventory is still accurate — spot-checked against `components/ui/FAB.tsx` and `components/ui/AvatarMenu.tsx`.
RonenMars added a commit that referenced this pull request Aug 1, 2026
…ocker (#476)

## Problem

The `↑ Top` FAB in the conversation detail view appears once you scroll
past ~200px, horizontally centered at the top of the list, and sits on
top of message content.
It was fully opaque, so text behind it was unreadable, and it
intercepted touches in its rect.
Message bubbles span nearly the full width and the pill is centered, so
it lands squarely on the text column rather than in a margin.

Occlusions captured during the #470 investigation: `…write the spec doc
for th[↑ Top]n plan.` and `| Cypress | Testing | s[↑ Top]ntend | strong
|`.

## Root cause

`ConversationHistoryList` was the odd one out among three sibling
implementations of the same affordance.

| | background | show/hide | wrapper |
|---|---|---|---|
| `ConversationHistoryList` (before) | `theme.text.accent`, opaque |
conditional mount/unmount, no fade | none |
| `ConversationList` | `rgba(31, 111, 235, 0.14)` + `rgba(88, 166, 255,
0.2)` border | `Animated.View` + `useAnimatedStyle` opacity, 220ms
`withTiming` | `pointerEvents="box-none"` |
| `TerminalOutput` | `rgba(31, 111, 235, 0.18)` | same animated pattern
| `pointerEvents="box-none"` |

The two working versions use a translucent chip with muted text that
reads as an overlay you can see through, and they fade rather than pop.
`ConversationHistoryList` used a solid accent-blue pill with white bold
text, which reads as a modal blocker.

## Fix

`ConversationHistoryList` now matches its two siblings rather than
introducing a third visual language: translucent background, muted 12px
text, `Animated.View` + `pointerEvents="box-none"` wrapper, 220ms fade
in/out.
The show/hide condition moves from a `useState` toggle to a shared
value, so the pill is always mounted and fades instead of popping.
`pointerEvents` tracks opacity inside the same animated style, so a
faded-out pill cannot intercept list touches.

This is fixed purely as an overlay.
There are no `contentContainerStyle` padding, `drawDistance`,
`maintainVisibleContentPosition`, or `getItemType` changes — those lines
are load-bearing for the FlashList v2 anchoring stabilized in #470,
which measured content-height swings up to ±19,898pt causing viewport
teleports.
Padding that toggles with FAB visibility would mutate content height
mid-scroll and reintroduce that class of bug.

The bottom `CaretDown` FAB is untouched; it sits bottom-right in the
gutter and was not reported.

## Before / after

Same fixture conversation, same scroll offset, driven by an identical
scripted gesture sequence on a booted simulator against two Release
builds.
In the before frame the table row reads `| Express | Language | s[↑ Top]
backend |`.
In the after frame the same row reads `| Express | Language | senior |
backend |` straight through the pill.

| Before | After |
|---|---|
| <img
src="https://raw.githubusercontent.com/RonenMars/threadbase-mobile/ad16c92e491e26b7e350bc35b6bc3c46acebd887/docs/screenshots/top-fab-overlap-before.png"
width="320"> | <img
src="https://raw.githubusercontent.com/RonenMars/threadbase-mobile/ad16c92e491e26b7e350bc35b6bc3c46acebd887/docs/screenshots/top-fab-overlap-after.png"
width="320"> |

## Accessibility labels

The top FAB's `accessibilityLabel` was a hardcoded English `"Scroll to
top"` while the bottom FAB directly below it already used
`t('action.scrollToBottom')`.
The same gap existed in `ConversationList` (`"Scroll to top"`, `"Scroll
to bottom"`) and `TerminalOutput` (`"Jump to top"`, `"Jump to bottom"`),
five hardcoded strings in total.
New `nav.scrollToTop` and `nav.scrollToBottom` keys land in
`common.json` for en, he, ru, and ar, since all three components already
load that namespace.
The existing `conversation.json` `action.scrollToBottom` key stays in
use by the bottom FAB, so no key is orphaned.
The terminal labels change wording from "Jump" to "Scroll" so the
announced label is consistent app-wide, and `TerminalOutput.test.tsx` is
updated to match.

## Verification

`e2e/07_conversation_scroll_gaps.yaml` gains a closing block that
scrolls back down past the 200pt threshold, waits for the FAB by
`testID`, taps it, and asserts `SCROLL-GAPS-TOP-MARKER` (the first
message) is visible.
That proves the control is still hittable and still scrolls to the top
after the restyle, which is a real risk now that the pill is always
mounted: an opacity-0 overlay stuck on `pointerEvents: auto` would
swallow list touches, and one that never regains `auto` would stop
working.
All of the flow's existing assertions are intact, including the
`SPEC-MARKER` duplicate-append guard from #470.
The flow already runs in `test:e2e:mock`, so no script wiring changed.

The full flow passes end to end on a Release build.

- `npm run lint` — 0 errors, 5 warnings, all pre-existing and none in
the touched files.
- Conversation and terminal Jest suites — 26 suites, 277 tests, all
passing.
- `npm run test:i18n` — 3 suites, 55 passed, 1 skipped.

Two pre-existing conditions on this base, neither introduced here.
`npx tsc --noEmit` reports 14 errors, and the count is identical on the
base branch with these changes stashed.
`npm run lint:i18n` fails its `--max-warnings=0` gate on a
`react-hooks/exhaustive-deps` warning in `app/session/[id].tsx`, and it
fails identically on the base branch.

## Follow-ups, not in this PR

All three copies of this FAB are now near-identical and could be
extracted into one shared component.
Three near-identical copies is a pre-existing condition, so that is
deliberately left alone here.

`LiveConversationView` renders its own inline FlashList and was noted in
#470 as deserving the same list-prop treatment; it has no top FAB and is
unaffected by this change.
RonenMars added a commit that referenced this pull request Aug 1, 2026
…audit (#486)

Both files existed only as untracked files in a working tree, so neither survived a fresh clone.

The scroll-FAB plan covers the extraction that #476 deferred when the top-FAB overlay fix made the third copy converge on the other two, and no commit since has done it.
`docs/superpowers/plans/` is where the repo already keeps its active plans.

The audit inventories user-facing strings in `app/` and `components/` that are not routed through `react-i18next`.
The i18n CI job enforces locale key parity rather than coverage, so nothing else in the repo records which strings are still hardcoded.
Its line numbers will drift, but the inventory is still accurate — spot-checked against `components/ui/FAB.tsx` and `components/ui/AvatarMenu.tsx`.
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