fix(conversation): make the top scroll FAB an overlay instead of a blocker - #476
Merged
Merged
Conversation
RonenMars
force-pushed
the
land/integration-prep
branch
from
August 1, 2026 10:34
d343bd5 to
4b309f3
Compare
…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
force-pushed
the
fix/top-fab-overlap
branch
from
August 1, 2026 10:50
32a4134 to
ad16c92
Compare
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`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
↑ TopFAB 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
ConversationHistoryListwas the odd one out among three sibling implementations of the same affordance.ConversationHistoryList(before)theme.text.accent, opaqueConversationListrgba(31, 111, 235, 0.14)+rgba(88, 166, 255, 0.2)borderAnimated.View+useAnimatedStyleopacity, 220mswithTimingpointerEvents="box-none"TerminalOutputrgba(31, 111, 235, 0.18)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.
ConversationHistoryListused a solid accent-blue pill with white bold text, which reads as a modal blocker.Fix
ConversationHistoryListnow 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
useStatetoggle to a shared value, so the pill is always mounted and fades instead of popping.pointerEventstracks 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
contentContainerStylepadding,drawDistance,maintainVisibleContentPosition, orgetItemTypechanges — 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
CaretDownFAB 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.Accessibility labels
The top FAB's
accessibilityLabelwas a hardcoded English"Scroll to top"while the bottom FAB directly below it already usedt('action.scrollToBottom').The same gap existed in
ConversationList("Scroll to top","Scroll to bottom") andTerminalOutput("Jump to top","Jump to bottom"), five hardcoded strings in total.New
nav.scrollToTopandnav.scrollToBottomkeys land incommon.jsonfor en, he, ru, and ar, since all three components already load that namespace.The existing
conversation.jsonaction.scrollToBottomkey 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.tsxis updated to match.Verification
e2e/07_conversation_scroll_gaps.yamlgains a closing block that scrolls back down past the 200pt threshold, waits for the FAB bytestID, taps it, and assertsSCROLL-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: autowould swallow list touches, and one that never regainsautowould stop working.All of the flow's existing assertions are intact, including the
SPEC-MARKERduplicate-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.npm run test:i18n— 3 suites, 55 passed, 1 skipped.Two pre-existing conditions on this base, neither introduced here.
npx tsc --noEmitreports 14 errors, and the count is identical on the base branch with these changes stashed.npm run lint:i18nfails its--max-warnings=0gate on areact-hooks/exhaustive-depswarning inapp/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.
LiveConversationViewrenders 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.