-
-
Notifications
You must be signed in to change notification settings - Fork 453
fix(virtual-core): don't let a stale scroll target clobber the prepend anchor #1248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <!doctype html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| </head> | ||
| <body> | ||
| <div id="root"></div> | ||
| <script type="module" src="./main.tsx"></script> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| import React from 'react' | ||
| import { createRoot } from 'react-dom/client' | ||
| import { useVirtualizer } from '@tanstack/react-virtual' | ||
|
|
||
| // End-anchored list built for one scenario: a long smooth scrollToIndex that is | ||
| // still in flight when history is prepended. The list is deliberately tall | ||
| // (200 x 50px against a 300px viewport) so the animation lasts long enough for | ||
| // the test to reliably observe it mid-flight and prepend into that window. | ||
|
|
||
| type Message = { | ||
| id: string | ||
| text: string | ||
| } | ||
|
|
||
| const makeMessage = (index: number): Message => ({ | ||
| id: `m-${index}`, | ||
| text: `Message ${index}`, | ||
| }) | ||
|
|
||
| const initialMessages = Array.from({ length: 200 }, (_, index) => | ||
| makeMessage(index), | ||
| ) | ||
|
|
||
| function App() { | ||
| const [messages, setMessages] = React.useState(initialMessages) | ||
| const [didInitialScroll, setDidInitialScroll] = React.useState(false) | ||
| const parentRef = React.useRef<HTMLDivElement>(null) | ||
| const firstMessageIndexRef = React.useRef(0) | ||
|
|
||
| const virtualizer = useVirtualizer({ | ||
| count: messages.length, | ||
| getScrollElement: () => parentRef.current, | ||
| estimateSize: () => 50, | ||
| getItemKey: (index) => messages[index]!.id, | ||
| anchorTo: 'end', | ||
| followOnAppend: true, | ||
| overscan: 4, | ||
| }) | ||
|
|
||
| React.useLayoutEffect(() => { | ||
| if (didInitialScroll) return | ||
| virtualizer.scrollToEnd() | ||
| setDidInitialScroll(true) | ||
| }, [didInitialScroll, virtualizer]) | ||
|
|
||
| return ( | ||
| <div> | ||
| <button | ||
| id="smooth-to-0" | ||
| onClick={() => virtualizer.scrollToIndex(0, { behavior: 'smooth' })} | ||
| > | ||
| Smooth to 0 | ||
| </button> | ||
| <button | ||
| id="prepend" | ||
| onClick={() => { | ||
| const start = firstMessageIndexRef.current - 5 | ||
| firstMessageIndexRef.current = start | ||
| setMessages((current) => [ | ||
| ...Array.from({ length: 5 }, (_, offset) => | ||
| makeMessage(start + offset), | ||
| ), | ||
| ...current, | ||
| ]) | ||
| }} | ||
| > | ||
| Prepend | ||
| </button> | ||
|
|
||
| <div | ||
| ref={parentRef} | ||
| id="scroll-container" | ||
| style={{ | ||
| height: 300, | ||
| overflow: 'auto', | ||
| width: 420, | ||
| border: '1px solid #ddd', | ||
| }} | ||
| > | ||
| <div | ||
| style={{ | ||
| height: virtualizer.getTotalSize(), | ||
| position: 'relative', | ||
| width: '100%', | ||
| }} | ||
| > | ||
| {virtualizer.getVirtualItems().map((item) => { | ||
| const message = messages[item.index]! | ||
|
|
||
| return ( | ||
| <div | ||
| key={item.key} | ||
| ref={virtualizer.measureElement} | ||
| data-index={item.index} | ||
| data-message-id={message.id} | ||
| data-testid={`message-${message.id}`} | ||
| style={{ | ||
| position: 'absolute', | ||
| top: 0, | ||
| left: 0, | ||
| transform: `translateY(${item.start}px)`, | ||
| width: '100%', | ||
| }} | ||
| > | ||
| <div | ||
| style={{ | ||
| boxSizing: 'border-box', | ||
| height: 50, | ||
| padding: 8, | ||
| borderBottom: '1px solid #eee', | ||
| }} | ||
| > | ||
| {message.text} | ||
| </div> | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| createRoot(document.getElementById('root')!).render(<App />) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||||||||||||
| import { expect, test } from '@playwright/test' | ||||||||||||||||
| import type { Page } from '@playwright/test' | ||||||||||||||||
|
|
||||||||||||||||
| const scrollTop = (page: Page) => | ||||||||||||||||
| page.evaluate(() => { | ||||||||||||||||
| const container = document.querySelector('#scroll-container') | ||||||||||||||||
| if (!container) throw new Error('Container not found') | ||||||||||||||||
| return container.scrollTop | ||||||||||||||||
| }) | ||||||||||||||||
|
|
||||||||||||||||
| async function waitForEnd(page: Page) { | ||||||||||||||||
| await expect | ||||||||||||||||
| .poll(async () => | ||||||||||||||||
| page.evaluate(() => { | ||||||||||||||||
| const container = document.querySelector('#scroll-container') | ||||||||||||||||
| if (!container) throw new Error('Container not found') | ||||||||||||||||
| return Math.abs( | ||||||||||||||||
| container.scrollHeight - container.scrollTop - container.clientHeight, | ||||||||||||||||
| ) | ||||||||||||||||
| }), | ||||||||||||||||
| ) | ||||||||||||||||
| .toBeLessThan(1.01) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // KNOWN BUG, not a guard on current behaviour — test.fail() asserts this still | ||||||||||||||||
| // reproduces and turns red the moment it is fixed, at which point drop the | ||||||||||||||||
| // annotation and keep the assertions. | ||||||||||||||||
| // | ||||||||||||||||
| // A prepend that lands while a scrollToIndex is still travelling strands it. The | ||||||||||||||||
| // anchor sync in _willUpdate writes scrollTop, which cancels the browser's | ||||||||||||||||
| // smooth animation, and reconcileScroll never resumes the journey because its | ||||||||||||||||
| // `else` branch only re-asserts when the *target* changed. With uniform rows | ||||||||||||||||
| // index 0 sits at offset 0 both before and after the prepend, so the target is | ||||||||||||||||
| // unchanged and the loop just idles. "Jump to the oldest message" therefore dies | ||||||||||||||||
| // halfway whenever history streams in mid-animation. | ||||||||||||||||
| // | ||||||||||||||||
| // Reproduces identically on the commit before the stale-target fix (stranded at | ||||||||||||||||
| // ~3900 vs ~3500), so it is pre-existing and independent of it. Resuming an | ||||||||||||||||
| // unfinished scroll needs its own change: reconcileScroll idling is exactly what | ||||||||||||||||
| // stops it fighting a reader who deliberately scrolls away mid-scroll, so making | ||||||||||||||||
| // it re-assert is a behavioural decision rather than a local patch. | ||||||||||||||||
| test.fail() | ||||||||||||||||
| test('a prepend mid-flight does not abandon a smooth scrollToIndex', async ({ | ||||||||||||||||
| page, | ||||||||||||||||
| }) => { | ||||||||||||||||
|
Comment on lines
+42
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the installed Playwright version declaration and all test.fail() uses.
fd -a 'package.json|pnpm-lock.yaml' . -x rg -n -C2 '`@playwright/test`|playwright' {}
rg -n -C3 'test\.fail\(' packages/react-virtual/e2e/app/test/smooth-prepend.spec.tsRepository: TanStack/virtual Length of output: 3169 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== target file context =="
sed -n '35,55p' packages/react-virtual/e2e/app/test/smooth-prepend.spec.ts
echo
echo =="playwright test types around test.fail (if available)==="
if [ -f node_modules/@playwright/test/types/test.d.ts ]; then
rg -n -C4 'fail\(' node_modules/@playwright/test/types/test.d.ts
else
fd 'test.d.ts|index.d.ts' node_modules/@playwright -x sh -c 'echo "-- $1"; rg -n -C4 "fail\\(" "$1"' sh {}
fi
echo
echo "== lock/test package context =="
python3 - <<'PY'
import json
from pathlib import Path
for p in Path('.').glob('packages/react-virtual/e2e/app/package.json'):
data=json.loads(p.read_text())
print(p, 'scripts:', data.get('test:e2e'), 'dependencies:', data.get('dependencies'), 'devDependencies:', {k:v for k,v in data.get('devDependencies',{}).items() if 'playwright' in k.lower()})
PYRepository: TanStack/virtual Length of output: 1174 🌐 Web query:
💡 Result: In Playwright, calling test.fail without arguments inside a test body is a valid way to mark the current test as expected to fail at runtime [1][2]. When invoked this way, Playwright ensures the test actually fails during its execution [2][3]. If the test unexpectedly passes, Playwright will report an error [3][4]. While test.fail can be called without arguments, official documentation generally recommends declaring failing tests using the arguments-based syntax—such as test.fail(title, body) or test.fail(condition, description)—for better readability and intent [1][2]. If you need to include a description when using the unconditional form, you can use the syntax test.fail(true, 'description') as a workaround [5]. This runtime annotation method is useful for acknowledging known bugs or issues that should be addressed later, as it allows the test suite to remain "green" (passing) even when the specific test is known to fail [1][2]. Citations:
🌐 Web query:
💡 Result: In Playwright Test, annotations like test.fail, test.skip, and test.fixme have distinct execution behaviors [1][2]. Understanding why an annotation might appear "not called" or why unexpected skipping occurs requires identifying which annotation is used and where it is placed [1][2]. Execution Behaviors - test.fail: Playwright executes the test body and expects the test to fail [1][2]. If the test passes instead of failing, Playwright reports an error [1][3]. - test.skip: Playwright does not execute the test at all; it is marked as skipped [1][2]. - test.fixme: Playwright does not execute the test; it is marked as fixme, similar to skip, but specifically intended for tests known to be broken or needing work [1][2]. Why tests may be skipped unexpectedly or annotations appear not to take effect: 1. Test Failure or Uncaught Exceptions: If an error occurs (such as an unawaited promise or an uncaught exception) before the annotation or during test execution, it may cause the test worker to crash or terminate early, leading to subsequent tests being skipped or the test not reporting as expected [4][5]. 2. Placement in Hooks: Annotations placed inside beforeEach or beforeAll hooks affect the tests within those scopes [1][6]. If a failure occurs in a hook, it may prevent the test body from running, causing the annotation logic not to be reached [6]. 3. Execution Context: Annotations like test.slow or test.fail called inside the test body only register once that line of code is executed [2][7]. If a test fails or hangs before that line is reached, the annotation will not be active for that specific test run [7]. 4. Parallelism and Worker Issues: In parallel mode, unhandled rejections or severe errors can kill a worker process, which causes other tests assigned to that worker to be skipped [4][5]. Best Practices - Always await asynchronous operations within your tests [4]. Failure to await promises, particularly when using expect or other Playwright actions, is a common cause of flaky or skipped tests [4]. - Use static annotations in the test declaration if you need them to be available immediately, such as in onTestBegin in custom reporters [7]. - Ensure your test environment configuration (e.g., timeouts) aligns with your intended test behavior [8]. If a test times out before reaching a test.fail call, it may be reported as a timeout failure rather than a successful "expected failure" [8]. Citations:
Remove the unsupported Call Proposed fix-test.fail()
-test('a prepend mid-flight does not abandon a smooth scrollToIndex', async ({
+test.fail('a prepend mid-flight does not abandon a smooth scrollToIndex', async ({📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| await page.goto('/smooth-prepend/') | ||||||||||||||||
| await waitForEnd(page) | ||||||||||||||||
|
|
||||||||||||||||
| const start = await scrollTop(page) | ||||||||||||||||
| expect(start).toBeGreaterThan(9000) // 200 x 50 - 300 | ||||||||||||||||
|
|
||||||||||||||||
| // Ask for index 0 and catch the animation in flight — well clear of both | ||||||||||||||||
| // ends, so this asserts on a genuinely mid-scroll prepend. | ||||||||||||||||
| await page.click('#smooth-to-0') | ||||||||||||||||
| await expect | ||||||||||||||||
| .poll(() => scrollTop(page), { timeout: 5000 }) | ||||||||||||||||
| .toBeLessThan(start - 1000) | ||||||||||||||||
| expect(await scrollTop(page)).toBeGreaterThan(500) | ||||||||||||||||
|
|
||||||||||||||||
| // History arrives while we are still moving. | ||||||||||||||||
| await page.click('#prepend') | ||||||||||||||||
|
|
||||||||||||||||
| // The requested scroll should still complete. Index 0 sits at offset 0 both | ||||||||||||||||
| // before and after the prepend (uniform 50px rows), so the destination is | ||||||||||||||||
| // unambiguous: the top. | ||||||||||||||||
| await expect.poll(() => scrollTop(page), { timeout: 3000 }).toBeLessThan(1.01) | ||||||||||||||||
| }) | ||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unnecessary non-null assertions.
ESLint reports errors at Line 34 and Line 88. Remove
!from both array accesses.Proposed fix
Also applies to: 88-88
🧰 Tools
🪛 ESLint
[error] 34-34: This assertion is unnecessary since it does not change the type of the expression.
(
@typescript-eslint/no-unnecessary-type-assertion)🤖 Prompt for AI Agents
Source: Linters/SAST tools