fix(web): require user intent to cancel initial scroll - #1291
Conversation
There was a problem hiding this comment.
Findings
-
[Major] Preserve keyboard intent before consuming it —
consumeExplicitUpwardIntent()clearskeyboardResumeUntil, then the new settling guard re-reads the cleared state. When Home/PageUp starts outside the preload margin but lands inside it during initial settling, the gesture neither cancels settling nor starts the user history load, so a remaining timer can snap the viewport back to the bottom. Evidence:web/src/components/AssistantChat/HappyThread.tsx:677Suggested fix:
const hadExplicitUpwardIntent = hasExplicitUpwardIntent(intent) const explicitUpwardIntent = needsCoverage && consumeExplicitUpwardIntent(intent) if (shouldCancelInitialScrollSettling(intent, hadExplicitUpwardIntent)) { // existing cancellation body }
Summary
Review mode: initial. One Major finding. The added helper tests do not cover the event-state transition where keyboard intent is consumed before the settling decision.
Testing
Not run (automation; static review only). Add an interaction test for Home/PageUp beginning outside coverage, landing inside coverage during initial settling, and asserting that settling is cancelled and the request source is user.
HAPI Bot
|
|
||
| if (isInitialScrollSettling()) { | ||
| if (shouldCancelInitialScrollSettling(intent)) { | ||
| if (shouldCancelInitialScrollSettling(intent, hasExplicitUpwardIntent(intent))) { |
There was a problem hiding this comment.
[MAJOR] Preserve keyboard intent before consuming it
explicitUpwardIntent is computed first, and consumeExplicitUpwardIntent() clears keyboardResumeUntil. This new call then rechecks hasExplicitUpwardIntent() and sees false. If Home/PageUp starts outside preload coverage but its resulting scroll lands inside coverage during the initial 1.8-second window, the keyboard gesture neither cancels settling nor starts the user history load; a remaining settling timer can snap the viewport back to the bottom.
Suggested fix:
const hadExplicitUpwardIntent = hasExplicitUpwardIntent(intent)
const explicitUpwardIntent = needsCoverage && consumeExplicitUpwardIntent(intent)
if (shouldCancelInitialScrollSettling(intent, hadExplicitUpwardIntent)) {There was a problem hiding this comment.
Findings
- None.
Summary
Review mode: follow-up after new commits. No issues found in the current full diff. The prior Major ordering issue is no longer present: explicit upward intent is captured before the consuming history-load check. Residual risk: the added tests exercise the pure cancellation helper, but not the event-state/timer interaction.
Testing
Not run (automation; static review only). GitHub test and pr-review checks were pending during review. Add an interaction test covering keyboard or wheel input during initial settling, including entry into the preload area and cancellation of remaining settle timers.
HAPI Bot
Summary
Fixes #1288
Testing
bun typecheckbun run test:web— 204 files, 1761 tests passedHappyThread.test.tsx— 22 tests passed