Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/react-virtual/e2e/app/smooth-prepend/index.html
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>
124 changes: 124 additions & 0 deletions packages/react-virtual/e2e/app/smooth-prepend/main.tsx
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,

Copy link
Copy Markdown

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
-    getItemKey: (index) => messages[index]!.id,
+    getItemKey: (index) => messages[index].id,
...
-            const message = messages[item.index]!
+            const message = messages[item.index]

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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/react-virtual/e2e/app/smooth-prepend/main.tsx` at line 34, Remove
the unnecessary non-null assertions from the array accesses in the getItemKey
callback and the corresponding access at the second reported location,
preserving their existing indexing and key behavior.

Source: Linters/SAST tools

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 />)
67 changes: 67 additions & 0 deletions packages/react-virtual/e2e/app/test/smooth-prepend.spec.ts
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.ts

Repository: 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()})
PY

Repository: TanStack/virtual

Length of output: 1174


🌐 Web query:

Playwright @playwright/test test.fail zero argument test fail runtime annotation only during execution

💡 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:

Playwright TestAnnotations fail not called during test execution fail during test execution

💡 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 test.fail() annotation.

Call test.fail(...) only inside a test body during execution. This call runs during module evaluation, before Playwright registers the test, so Playwright throws when the spec is loaded.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test.fail()
test('a prepend mid-flight does not abandon a smooth scrollToIndex', async ({
page,
}) => {
test.fail('a prepend mid-flight does not abandon a smooth scrollToIndex', async ({
page,
}) => {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/react-virtual/e2e/app/test/smooth-prepend.spec.ts` around lines 42 -
45, Remove the top-level test.fail() call before the smooth-scroll test
declaration; leave the test('a prepend mid-flight does not abandon a smooth
scrollToIndex', ...) registration and body unchanged.

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)
})
1 change: 1 addition & 0 deletions packages/react-virtual/e2e/app/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default defineConfig({
'measure-element/index.html',
),
'smooth-scroll': path.resolve(__dirname, 'smooth-scroll/index.html'),
'smooth-prepend': path.resolve(__dirname, 'smooth-prepend/index.html'),
'stale-index': path.resolve(__dirname, 'stale-index/index.html'),
'direct-dom-updates': path.resolve(
__dirname,
Expand Down
Loading