fix(virtual-core): don't let a stale scroll target clobber the prepend anchor - #1248
fix(virtual-core): don't let a stale scroll target clobber the prepend anchor#1248piecyk wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change adds a smooth-prepend virtualized list example, registers it in the Vite build, and adds a Playwright regression test for scrolling interrupted by prepended history. ChangesSmooth prepend end-to-end scenario
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit b96bba2
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/virtual-core/src/index.ts`:
- Around line 670-684: The anchorResolved handling must not clear an active
smooth scroll. Update the scroll-state retirement logic near reconcileScroll so
anchor correction only retires stale programmatic targets after the destination
has been reached and the user has moved away, while preserving in-flight
scrollToIndex({ behavior: "smooth" }) state through prepend/setOptions updates;
add regression coverage for prepending during an active smooth scroll.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28b4746c-99bc-4c31-84d9-5e8c279082c0
📒 Files selected for processing (3)
.changeset/lucky-pandas-listen.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
4bf3ab9 to
709d666
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/virtual-core/src/index.ts`:
- Around line 1132-1142: Update the reachedTarget re-arm logic in the scroll
target handling block to use the same approximately 1.01 measurement-settling
tolerance as the retirement check, rather than the viewport distance. Re-arm
this.scrollState.reachedTarget whenever the target changes beyond that
tolerance, including changes smaller than a viewport but larger than the
settling threshold; preserve the existing behavior for minor measurement nudges.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae39a292-8f42-4251-a538-6cd0c1e82b54
📒 Files selected for processing (3)
.changeset/lucky-pandas-listen.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/lucky-pandas-listen.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/react-virtual/e2e/app/smooth-prepend/main.tsx`:
- 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.
In `@packages/react-virtual/e2e/app/test/smooth-prepend.spec.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 512dd6ae-228e-488e-95e4-7b06c3040ef4
📒 Files selected for processing (4)
packages/react-virtual/e2e/app/smooth-prepend/index.htmlpackages/react-virtual/e2e/app/smooth-prepend/main.tsxpackages/react-virtual/e2e/app/test/smooth-prepend.spec.tspackages/react-virtual/e2e/app/vite.config.ts
| count: messages.length, | ||
| getScrollElement: () => parentRef.current, | ||
| estimateSize: () => 50, | ||
| getItemKey: (index) => messages[index]!.id, |
There was a problem hiding this comment.
📐 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
| test.fail() | ||
| test('a prepend mid-flight does not abandon a smooth scrollToIndex', async ({ | ||
| page, | ||
| }) => { |
There was a problem hiding this comment.
🎯 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:
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:
- 1: https://github.com/microsoft/playwright/blob/d3d436d3/docs/src/test-api/class-test.md
- 2: https://playwright.dev/docs/api/class-test
- 3: https://github.com/microsoft/playwright/blob/main/docs/src/test-annotations-js.md
- 4: https://playwright.dev/docs/test-annotations
- 5: [Feature] Allow to fail tests unconditionally with description microsoft/playwright#23042
🌐 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:
- 1: https://playwright.dev/docs/test-annotations
- 2: https://playwright.dev/docs/api/class-test
- 3: https://playwright.dev/docs/next/test-annotations
- 4: [BUG] Missing await in test with
test.fail()causes other tests to be skipped microsoft/playwright#26435 - 5: [Bug]:
test.failandaddLocatorHandlercauses other test cases to be skipped microsoft/playwright#34807 - 6: https://stackoverflow.com/questions/77650020/testinfo-annotations-push-doesnt-work-when-test-failed-at-test-beforeach-or-glo
- 7: [Bug]: Can't access test.slow in onTestBegin microsoft/playwright#36385
- 8: [BUG] test.fail() does not work with test timeouts microsoft/playwright#16317
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.
| 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.
9f9632b to
b96bba2
Compare
1 participant
With anchorTo: 'end', a chat-style list that mounts, runs scrollToEnd(), and is then scrolled away from could jump the reader several screens on the next prepend instead of holding their position.
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit