chore(angular-virtual): proper angular 19#1158
Open
benjavicente wants to merge 12 commits intoTanStack:mainfrom
Open
chore(angular-virtual): proper angular 19#1158benjavicente wants to merge 12 commits intoTanStack:mainfrom
benjavicente wants to merge 12 commits intoTanStack:mainfrom
Conversation
📝 WalkthroughWalkthroughRefactors the Angular virtualizer to a signal-driven architecture, migrates the package build from ng-packagr to Vite and updates package exports, adds an Angular 19 E2E app with Playwright tests, and bumps TypeScript/Vite/tooling versions across examples and root tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Angular Component
participant Signal as linkedSignal/Signal
participant Virtualizer as Virtualizer (lazy)
participant AppRef as ApplicationRef / DOM tick
Component->>Signal: create options() / provide scrollElement
Signal->>Virtualizer: lazy create / _didMount
Component->>Virtualizer: call virtualizer.getVirtualItems() (via signalProxy)
Virtualizer-->>Signal: update internal state (virtual items, total size)
Signal->>Component: computed signals update (getVirtualItems, getTotalSize, ...)
Component->>AppRef: if useApplicationRefTick -> schedule tick (microtask)
AppRef->>Component: flush change detection / render DOM
Component->>Virtualizer: afterRenderEffect -> measureElement(...) for rendered rows
Virtualizer-->>Signal: updates measurementsCache -> signals propagate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
4 tasks
|
View your CI Pipeline Execution ↗ for commit a422a2d
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Upgrades the @tanstack/angular-virtual adapter to Angular 19, refactors the adapter to expose state via computed/linked signals (instead of lazy signal init), and switches packaging/build to Vite rather than ng-packagr.
Changes:
- Update adapter implementation to use
linkedSignal/computedwith a new genericsignalProxyproxy layer. - Replace ng-packagr build output with Vite-based
distoutput + updatedpackage.jsonexports. - Bump Angular (adapter + examples) to
^19.0.0and update docs to reflect Angular 19+ support.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/angular-virtual/vite.config.ts | Adds Vite/Vitest config merged with shared TanStack Vite config. |
| packages/angular-virtual/tsconfig.json | Simplifies TS config and includes vite.config.ts. |
| packages/angular-virtual/tsconfig.build.json | Removes ng-packagr build TS config. |
| packages/angular-virtual/src/types.ts | Extends AngularVirtualizer surface area (adds callable signature + measurementsCache signal). |
| packages/angular-virtual/src/proxy.ts | Replaces proxyVirtualizer with generic signalProxy implementation. |
| packages/angular-virtual/src/index.ts | Refactors virtualizer creation lifecycle to linked/computed signals + new proxy. |
| packages/angular-virtual/package.json | Switches outputs/exports to dist (CJS+ESM), updates scripts, bumps Angular/TS versions. |
| packages/angular-virtual/ng-package.json | Removes ng-packagr packaging config. |
| packages/angular-virtual/README.md | Updates stated minimum Angular version to 19. |
| examples/angular/*/package.json | Bumps Angular/TS versions across Angular examples to 19/5.6. |
| docs/framework/angular/angular-virtual.md | Documents Angular 19+/LTS intent and signal-exposed adapter state. |
| .changeset/gold-days-greet.md | Adds changeset for adapter release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "./package.json": "./package.json" | ||
| }, | ||
| "engines": { | ||
| "node": ">=12" |
There was a problem hiding this comment.
"engines.node" is still set to ">=12", but this package now builds with Vite ("build": "vite build"). Vite’s supported Node versions are much newer than 12, so consumers/contributors on Node 12–17 will hit build/install failures. Please bump the Node engine range to match the Vite version used in the repo (and any workspace-wide engines policy).
Suggested change
| "node": ">=12" | |
| "node": ">=18" |
Comment on lines
+54
to
+56
| afterRenderEffect((cleanup) => { | ||
| cleanup(lazyVirtualizer()._didMount()) | ||
| }) |
There was a problem hiding this comment.
"_didMount()" is typically a one-time mount hook that registers observers/listeners and returns a teardown. Calling it inside "afterRenderEffect" risks re-mounting on every render cycle (and relying on cleanup between renders), which can cause listener churn and subtle behavior differences vs the previous "afterNextRender" one-shot mount. Consider ensuring "_didMount()" is invoked exactly once per instance (one-time post-render hook or a guard), while still registering teardown on destroy.
Author
There was a problem hiding this comment.
lazyVirtualizer() does not change, so it runs once.
|
|
||
| inject(DestroyRef).onDestroy(() => cleanup?.()) | ||
| afterRenderEffect(() => { | ||
| reactiveVirtualizer()._willUpdate() |
There was a problem hiding this comment.
Calling "_willUpdate()" after every render can be significantly more frequent than the prior behavior (which only invoked it when the scroll element changed). If "_willUpdate()" performs DOM reads/writes or sets up observers, this can become a hot path. Consider narrowing the trigger (e.g., react only to scroll element changes / option changes that matter) or otherwise avoid invoking on renders that don’t affect the virtualizer.
Suggested change
| reactiveVirtualizer()._willUpdate() | |
| const virtualizer = reactiveVirtualizer() | |
| const nextScrollElement = resolvedOptions().getScrollElement() | |
| if (virtualizer.scrollElement !== nextScrollElement) { | |
| virtualizer._willUpdate() | |
| } |
Author
There was a problem hiding this comment.
The change looks more aligned with react, where _willUpdate will be called on every re-render.
Open
4 tasks
Collaborator
|
@benjavicente awesome work! I’ll play around with it a bit. Could we add a simple e2e test here, similar to what we have in react-virtual, to cover the basic flow? |
benjavicente
force-pushed
the
angular-19
branch
from
a422a2d to
962df91
Compare
Align angular-virtual and Angular example dependencies with Angular 19 and update lockfile/docs to match.
Adopt Angular 19 signal primitives in the adapter/proxy implementation to improve lazy initialization and reactive tracking behavior.
Replace ng-packagr build artifacts/config with Vite library outputs and update package exports/scripts for ESM+CJS distribution.
benjavicente
force-pushed
the
angular-19
branch
from
962df91 to
4352093
Compare
Author
|
All green now 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (10)
packages/virtual-core/tests/index.test.ts (1)
340-353: Consider extracting a sharedcreateMockResizeObserver()helper.Lines 340-346 duplicate the same mock factory from Lines 170-176. Pulling this into one local helper would reduce drift risk across tests.
♻️ Suggested refactor
+function createMockResizeObserver() { + return vi.fn(function () { + return { + observe: vi.fn(), + unobserve: vi.fn(), + disconnect: vi.fn(), + } + }) +} + test('should not throw when component unmounts during scrollToIndex rAF loop', () => { - const MockResizeObserver = vi.fn(function () { - return { - observe: vi.fn(), - unobserve: vi.fn(), - disconnect: vi.fn(), - } - }) + const MockResizeObserver = createMockResizeObserver() const mockWindow = { requestAnimationFrame: mockRaf, cancelAnimationFrame: vi.fn(), ResizeObserver: MockResizeObserver, } @@ function createMockEnvironment() { @@ - const MockResizeObserver = vi.fn(function () { - return { - observe: vi.fn(), - unobserve: vi.fn(), - disconnect: vi.fn(), - } - }) + const MockResizeObserver = createMockResizeObserver()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/virtual-core/tests/index.test.ts` around lines 340 - 353, The test suite defines the same ResizeObserver mock factory twice (MockResizeObserver) — extract a shared helper named createMockResizeObserver() that returns the vi.fn factory producing { observe, unobserve, disconnect } mocks, replace both MockResizeObserver occurrences with calls to createMockResizeObserver() and assign its result to mockWindow.ResizeObserver and the earlier test’s equivalent; this ensures both tests reuse the same factory function (refer to MockResizeObserver and mockWindow.ResizeObserver to locate the spots to replace).packages/angular-virtual/e2e/app/test/measure-element.spec.ts (1)
29-41: Poll the actual layout condition instead of sleeping.
waitForTimeout(200)may not reliably cover ResizeObserver/layout timing on loaded CI. Polling the non-overlap assertion makes the test wait only as long as needed.♻️ Proposed refactor
- // Wait for ResizeObserver to measure the expanded B - await page.waitForTimeout(200) - - // C should be positioned after the expanded B, not overlapping it - const bBox = await page.locator('[data-testid="item-b"]').boundingBox() - const cBox = await page.locator('[data-testid="item-c"]').boundingBox() - - expect(bBox).not.toBeNull() - expect(cBox).not.toBeNull() - - // C's top should be at or after B's bottom (with no overlap) - const bBottom = bBox!.y + bBox!.height - expect(cBox!.y).toBeGreaterThanOrEqual(bBottom - 1) // 1px tolerance + // C should be positioned after the expanded B, not overlapping it + await expect + .poll(async () => { + const bBox = await page.locator('[data-testid="item-b"]').boundingBox() + const cBox = await page.locator('[data-testid="item-c"]').boundingBox() + + if (!bBox || !cBox) { + return false + } + + return cBox.y >= bBox.y + bBox.height - 1 // 1px tolerance + }) + .toBe(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/e2e/app/test/measure-element.spec.ts` around lines 29 - 41, Replace the brittle fixed sleep (page.waitForTimeout(200)) with a poll that retries the layout assertion until it's true or a timeout elapses: use page.waitForFunction (or a small loop) to repeatedly query the locators via page.locator('[data-testid="item-b"]').boundingBox() and page.locator('[data-testid="item-c"]').boundingBox() and assert that cBox.y >= (bBox.y + bBox.height) - 1 (preserve the 1px tolerance); ensure you still null-check bounding boxes and fail the test if the waitForFunction times out so CI will report the layout regression.packages/angular-virtual/e2e/app/test/stale-index.spec.ts (1)
13-37: Replace fixed sleeps with a deterministic observer/layout flush.
waitForTimeout(100/200)can be either too short on loaded CI or unnecessarily slow locally. Prefer waiting on a concrete browser condition, e.g. a doublerequestAnimationFrameflush after scrolling/removal, then assert the error element is absent.♻️ Proposed direction
+const flushLayout = async () => { + await page.evaluate( + () => + new Promise<void>((resolve) => { + requestAnimationFrame(() => { + requestAnimationFrame(() => resolve()) + }) + }), + ) +} + // Scroll to the bottom so the last items are rendered and observed by ResizeObserver const container = page.locator('[data-testid="scroll-container"]') await container.evaluate((el) => (el.scrollTop = el.scrollHeight)) -await page.waitForTimeout(100) +await flushLayout() // Remove 5 items from the end — the RO may still fire for the now-disconnected nodes await page.click('[data-testid="remove-items"]') await expect(page.locator('[data-testid="item-count"]')).toHaveText( 'Count: 15', ) // Wait for any pending ResizeObserver callbacks -await page.waitForTimeout(200) +await flushLayout() // No error should have been thrown await expect(page.locator('[data-testid="error"]')).not.toBeVisible() // Remove 5 more to stress it await page.click('[data-testid="remove-items"]') await expect(page.locator('[data-testid="item-count"]')).toHaveText( 'Count: 10', ) -await page.waitForTimeout(200) +await flushLayout()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/e2e/app/test/stale-index.spec.ts` around lines 13 - 37, Replace the non-deterministic waits (await page.waitForTimeout(100) and 200) with a deterministic layout/paint flush using a double requestAnimationFrame promise after operations that change layout (the scrollTop set on container and after clicking '[data-testid="remove-items"]'). Specifically, after manipulating the DOM in the test (the container.evaluate(...) call and the await page.click('[data-testid="remove-items"]') calls) insert a browser-side double RAF flush (via page.evaluate(() => new Promise(r => requestAnimationFrame(() => requestAnimationFrame(r))))) and/or use a page.waitForFunction that checks the state of the '[data-testid="item-count"]' or absence of '[data-testid="error"]' before asserting; replace references to waitForTimeout with that deterministic flush around the item-count and error assertions.packages/angular-virtual/e2e/app/src/app/scroll.component.ts (1)
11-25: Make E2E row heights deterministic.
Math.random()makes the virtual layout differ across test runs, which makes alignment failures harder to reproduce. A key-based pseudo-random height keeps varied row sizes without nondeterminism.♻️ Proposed deterministic height helper
-function getRandomInt(min: number, max: number) { - return Math.floor(Math.random() * (max - min + 1)) + min -} - -const randomHeightForKey = (() => { - const cache = new Map<string, number>() - return (id: string) => { - const value = cache.get(id) - if (value !== undefined) { - return value - } - const v = getRandomInt(25, 100) - cache.set(id, v) - return v - } -})() +const randomHeightForKey = (id: string) => { + let hash = 0 + for (let i = 0; i < id.length; i++) { + hash = (hash * 31 + id.charCodeAt(i)) | 0 + } + return 25 + (Math.abs(hash) % 76) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/e2e/app/src/app/scroll.component.ts` around lines 11 - 25, The current randomHeightForKey uses Math.random via getRandomInt which produces non-deterministic row heights; replace the randomness with a key-based pseudo-random generator so heights are stable across runs: inside randomHeightForKey (and remove reliance on getRandomInt/Math.random for this path) compute a deterministic numeric seed from the id (e.g., a simple string hash of id), map that seed into the desired range [25,100] to produce v, store v in the existing cache Map<string, number>, and return it; keep the cache logic unchanged so repeated calls return the same value for the same key.packages/angular-virtual/e2e/app/src/app/smooth-scroll.component.ts (1)
11-25: Make the smooth-scroll fixture reproducible.The current cached
Math.random()heights change between runs. Use a deterministic key-based height so smooth-scroll failures can be reproduced with the same layout.♻️ Proposed deterministic height helper
-function getRandomInt(min: number, max: number) { - return Math.floor(Math.random() * (max - min + 1)) + min -} - -const randomHeightForKey = (() => { - const cache = new Map<string, number>() - return (id: string) => { - const value = cache.get(id) - if (value !== undefined) { - return value - } - const v = getRandomInt(25, 100) - cache.set(id, v) - return v - } -})() +const randomHeightForKey = (id: string) => { + let hash = 0 + for (let i = 0; i < id.length; i++) { + hash = (hash * 31 + id.charCodeAt(i)) | 0 + } + return 25 + (Math.abs(hash) % 76) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/e2e/app/src/app/smooth-scroll.component.ts` around lines 11 - 25, The current randomHeightForKey uses Math.random so heights vary between runs; replace the randomness with a deterministic function that maps the id string to a stable integer in the 25–100 range. Modify the randomHeightForKey closure (and keep the cache Map) to compute a simple string hash from the id (e.g., accumulate char codes with multipliers) and then normalize that hash into the desired range instead of calling getRandomInt/Math.random; store and return the computed value from the existing cache so repeated calls still return the same height for the same id.packages/angular-virtual/e2e/app/test/scroll.spec.ts (1)
25-124: Replace fixed sleeps with condition-based polling for test determinism.The six
waitForTimeout()calls (lines 26, 39, 60, 80, 120, 124) introduce flakiness—too long on local machines, too short on CI. Useexpect.poll()to wait for actual scroll/layout conditions, or rely on locator assertions:♻️ Example pattern
- // Wait for scroll effect (including retries) - await page.waitForTimeout(1000) - await expect(page.locator('[data-testid="item-1000"]')).toBeVisible() - const delta = await page.evaluate(check) // Angular layout/scroll alignment needs a slightly higher tolerance than React. - expect(delta).toBeLessThan(4.5) + await expect.poll(() => page.evaluate(check), { + timeout: 5000, + }).toBeLessThan(4.5)Apply this pattern to all six instances for reliable test execution across environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/e2e/app/test/scroll.spec.ts` around lines 25 - 124, Replace the fixed waits (await page.waitForTimeout(...)) with condition-based polling/assertions: for tests that wait for an item to appear use await expect(page.locator('[data-testid="item-1000"]')).toBeVisible() / '[data-testid="item-1001"]' instead of sleep; for "scrolls to last item" poll the container scroll position (query '#scroll-container' and assert Math.abs(scrollTop + clientHeight - scrollHeight) < 1.01) using expect.poll or a locator assertion; for initialOffset replace the 500ms sleep by polling until the minimum [data-index] in '#scroll-container' is > 0, and after performing the user scroll poll until container.scrollTop has decreased by ~2000; for layout checks poll until layout.items.length > 3 and layout.maxGap < 2; for the final "scrolls to index 0" replace the two 1s sleeps with polling for visibility of '[data-testid="item-1000"]' after first click and then for '[data-testid="item-0"]' (or container scrollTop≈0) after the second click. Use the existing selectors ('#scroll-container', '[data-index]', '[data-testid="item-*"]') and helper checks inside expect.poll to determine readiness instead of fixed timeouts.packages/angular-virtual/e2e/app/test/smooth-scroll.spec.ts (1)
7-139: Prefer condition-based waits over fixed sleeps.The repeated
waitForTimeout(2000)calls throughout this file (lines 8, 32, 42, 47, 62, 87, 113, 118, 123, and 139) make the suite unnecessarily slow and can still be flaky on busy CI if the animation takes longer than expected. Replace these with locator auto-waiting andexpect.poll()to wait for actual settled conditions instead.♻️ Example pattern
- await page.waitForTimeout(2000) - - await expect(page.locator('[data-testid="item-1000"]')).toBeVisible() - - const delta = await page.evaluate(() => { + await expect(page.locator('[data-testid="item-1000"]')).toBeVisible({ + timeout: 5000, + }) + + await expect.poll(() => page.evaluate(() => { const item = document.querySelector('[data-testid="item-1000"]') const container = document.querySelector('#scroll-container') if (!item || !container) throw new Error('Elements not found') @@ - return Math.abs(bottom - containerBottom) - }) - expect(delta).toBeLessThan(1.01) + return Math.abs(bottom - containerBottom) + }), { timeout: 5000 }).toBeLessThan(1.01)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/e2e/app/test/smooth-scroll.spec.ts` around lines 7 - 139, The tests use many fixed page.waitForTimeout(2000) sleeps (e.g., around interactions with '#scroll-to-1000', '#scroll-to-500', '#scroll-to-100', '#scroll-to-0', '#scroll-to-500-start', '#scroll-to-500-center') which slows and flakes CI; replace each sleep with condition-based waits such as awaiting the locator to be visible (await expect(page.locator('[data-testid="item-1000"]')).toBeVisible()) or using expect.poll()/locator.waitFor() to wait until the scroll container ('#scroll-container') has the expected scrollTop or the item geometry (computed via page.evaluate) is within tolerance, and for interrupt tests poll until the final target is visible/settled instead of arbitrary timeouts so each test waits for the actual condition rather than a fixed delay.packages/angular-virtual/src/index.ts (3)
82-82:computedused as a memoized lazy singleton.
lazyVirtualizerhas no tracked dependencies (the options read is wrapped inuntracked), so this is effectivelynew Virtualizer(...)memoized once. Usingcomputedfor a side‑effectful constructor is unconventional and makes the intent harder to read — a plain lazily‑initializedlet virtualizer: Virtualizer | undefined(or a one‑shot factory) would express the same thing without participating in the reactive graph. Optional clean‑up; current behavior is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/src/index.ts` at line 82, The computed wrapper around the side‑effectful constructor (const lazyVirtualizer = computed(() => new Virtualizer(untracked(resolvedOptions)))) should be replaced with an explicit lazy initialization to avoid participating in the reactive graph; introduce a module/local variable like let virtualizer: Virtualizer | undefined and replace uses of lazyVirtualizer.value with a one‑shot factory/getter that does if (!virtualizer) virtualizer = new Virtualizer(untracked(resolvedOptions)), returning virtualizer; optionally wire any cleanup logic to Virtualizer disposal where appropriate. Ensure you reference Virtualizer, resolvedOptions, untracked and remove the computed import/usages for lazyVirtualizer.
186-195: SSR guards checktypeof documentbut accesswindow.Lines 188 and 192‑193 gate on
typeof document !== 'undefined'yet the branch they protect useswindow/window.scrollY. In the common SSR case both are undefined together so this works in practice, but the asymmetry is misleading and fails in environments that polyfill one but not the other (some test runners, edge runtimes). Recommend checking the symbol you actually read.♻️ Proposed diff
return injectVirtualizerBase<Window, TItemElement>(() => ({ - getScrollElement: () => (typeof document !== 'undefined' ? window : null), + getScrollElement: () => (typeof window !== 'undefined' ? window : null), observeElementRect: observeWindowRect, observeElementOffset: observeWindowOffset, scrollToFn: windowScroll, initialOffset: () => - typeof document !== 'undefined' ? window.scrollY : 0, + typeof window !== 'undefined' ? window.scrollY : 0, ...options(), }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/src/index.ts` around lines 186 - 195, The SSR guards currently check typeof document but then read window (in getScrollElement and initialOffset), which is misleading and can break in runtimes that polyfill one but not the other; update the guards to check typeof window !== 'undefined' where you actually access window (change the checks used in the AngularVirtualizer factory for getScrollElement and initialOffset), and ensure you still return null/0 when window is unavailable so observeElementRect/observeElementOffset/scrollToFn logic remains safe.
99-139: Double cast bypasses validation of array coverage againstAngularVirtualizer.The four explicit string arrays passed to
signalProxyat the call site are not type-checked against theAngularVirtualizertype definition. The union of these arrays must cover every property that's omitted from the baseVirtualizerand given a signal wrapper inAngularVirtualizer(includinggetTotalSize,getVirtualItems,isScrolling,measurementsCache,options,range,scrollDirection,scrollElement,scrollOffset,scrollRect). Since the call is cast asunknown as AngularVirtualizer<...>, TypeScript provides no compile-time assertion. If a new signal-wrapped property is added toAngularVirtualizerwithout updating these arrays, the code will compile silently while consumers receive an unwrapped value at runtime.Consider adding a
satisfiescheck or exhaustiveness assertion at the call site to validate that the union of array keys matches the set of properties added back to theOmit<Virtualizer, ...>type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/src/index.ts` around lines 99 - 139, The call to signalProxy is double-cast to AngularVirtualizer and the four string-array lists (methods pass-through, attributes-to-signals, tracked methods, zero-arg computed) are not type-checked against the AngularVirtualizer/Virtualizer shape, so new signal-wrapped properties can be missed at compile time; update the call site to assert exhaustiveness by replacing the unknown cast with a TypeScript-checked construct (e.g., use a typed helper or the satisfies operator) that verifies the union of the arrays equals the set of signal-wrapped keys on AngularVirtualizer<TScrollElement, TItemElement>, referencing signalProxy, AngularVirtualizer, and the specific property names (e.g., getTotalSize, getVirtualItems, isScrolling, measurementsCache, options, range, scrollDirection, scrollElement, scrollOffset, scrollRect) so the compiler will error if any property is omitted or changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/svelte/dynamic/package.json`:
- Line 22: The package.json currently sets "vite": "^6.4.2" which is
incompatible with the pinned "@sveltejs/vite-plugin-svelte@^3.1.2" that requires
Vite ^5; fix by making the two dependencies compatible: either downgrade the
Vite entry in package.json to a Vite 5.x range (e.g. ^5.0.0) so it satisfies
"@sveltejs/vite-plugin-svelte@^3.1.2", or upgrade "@sveltejs/vite-plugin-svelte"
to a release that explicitly supports Vite 6 and adjust the plugin version
string accordingly; find and update the "vite" and
"@sveltejs/vite-plugin-svelte" dependency entries in package.json to match one
of these compatible pairs.
In `@examples/svelte/smooth-scroll/package.json`:
- Around line 21-22: The package.json shows an incompatible dependency set:
"@sveltejs/vite-plugin-svelte@^3.1.2" requires Vite ^5.x but "vite": "^6.4.2"
was added; fix by aligning versions—either upgrade the Svelte Vite plugin to a
release that lists Vite 6 as a peer (replace "@sveltejs/vite-plugin-svelte"
version accordingly) or downgrade "vite" to a 5.x version that satisfies the
plugin's peer dependency; update the package.json dependency entries for "vite"
or "@sveltejs/vite-plugin-svelte" and run install to verify peer compatibility.
In `@packages/angular-virtual/e2e/app/angular.json`:
- Line 2: The "$schema" path in e2e/app/angular.json is incorrect (uses
"../../../node_modules/...") and points to packages/node_modules; update the
schema string to traverse four levels up (use
"../../../../node_modules/@angular/cli/lib/config/schema.json") so it resolves
to the repository root node_modules; locate the "$schema" property in that file
and replace the relative path accordingly.
In `@packages/angular-virtual/e2e/app/src/app/app.config.ts`:
- Line 1: The import of ApplicationConfig should be a type-only import to
satisfy `@typescript-eslint/consistent-type-imports`: replace the current value
import for ApplicationConfig with a type import by changing the import statement
that references ApplicationConfig to use "import type { ApplicationConfig } from
'@angular/core'"; locate the existing import of ApplicationConfig (symbol:
ApplicationConfig) and update it to a type import to ensure it's treated as a
purely type-only import by the compiler/linter.
In `@packages/angular-virtual/e2e/app/src/app/app.routes.ts`:
- Line 1: The import of Routes should be a type-only import; replace the value
import "import { Routes } from '@angular/router'" with a type-only import for
the Routes type (e.g., use "import type { Routes } from '@angular/router'") so
the compiler/ESLint knows Routes is used only as a type.
In `@packages/angular-virtual/e2e/app/src/app/measure-element.component.ts`:
- Around line 1-9: ElementRef is only used as a type and the private field
`#measureItems` is assigned but never read; change the import of ElementRef to a
type-only import, remove the unused private field `#measureItems`, and move the
afterRenderEffect registration out of the class field initializer into the
component's constructor so the effect still runs — update any type annotations
referencing ElementRef accordingly and keep the afterRenderEffect callback logic
intact in the constructor.
In `@packages/angular-virtual/e2e/app/src/app/scroll.component.ts`:
- Around line 1-8: ElementRef is only used as a type and the private field
`#measureItems` is unused; update the import to import ElementRef as a type (e.g.,
import type { ElementRef } from '@angular/core') and remove the unused private
class field `#measureItems` to satisfy no-unused-private-class-members; to
preserve behavior, move the afterRenderEffect call currently defined outside the
constructor into the class constructor (keeping the same callback and references
to viewChild/viewChildren) so the render effect runs as before without leaving
an unused private member.
In `@packages/angular-virtual/e2e/app/src/app/smooth-scroll.component.ts`:
- Around line 1-8: ElementRef is only used as a type parameter for viewChild and
viewChildren and afterRenderEffect is called but its returned disposable is
stored in the unused private field `#measureItems`; change the import of
ElementRef to a type-only import, remove the unused private field `#measureItems`,
and invoke afterRenderEffect directly in the component constructor without
assigning its return value (keep the call to afterRenderEffect(...) as before).
Ensure the viewChild and viewChildren usages still reference ElementRef as a
type parameter and that no other code relies on the removed `#measureItems` field.
In `@packages/angular-virtual/e2e/app/src/app/stale-index.component.ts`:
- Around line 1-9: ElementRef is only used as a type and the private field
`#measureItems` is unused; update the import to import ElementRef as a type-only
import and remove the private `#measureItems` field, then call afterRenderEffect
inside the component constructor without assigning its return value to any
property (i.e., invoke afterRenderEffect(...) in the constructor), keeping
references to the functions/classes like afterRenderEffect, ElementRef, and the
component constructor to locate the changes.
In `@packages/angular-virtual/package.json`:
- Around line 39-52: The package.json currently sets "engines.node" to ">=12",
which is too low for Angular 19 + Vite; update the "engines" entry in
packages/angular-virtual package.json so the "node" constraint is bumped to a
Vite-compatible minimum (e.g., ">=20.19.0") to match toolchain requirements, and
ensure any related CI configs or documentation that reference Node version are
aligned with the new constraint; locate the "engines" object and change the
"node" value accordingly.
In `@packages/angular-virtual/README.md`:
- Line 7: Update the README note that currently reads "NOTE: Angular Virtual
requires Angular 19." to explicitly state the supported floor as "Angular 19+"
so it matches the package peer range (>=19) and the project’s LTS-and-up policy;
locate the note string in the README (the line beginning with "NOTE: Angular
Virtual requires") and change the trailing text from "Angular 19." to "Angular
19+".
In `@packages/angular-virtual/src/index.ts`:
- Around line 95-97: Add a short explanatory comment at the afterRenderEffect
that reads reactiveVirtualizer()._willUpdate(), describing the expected
re-entrancy convergence: explain that reactiveVirtualizer reads trigger the
linkedSignal (equal: () => false) which schedules maybeNotify →
reactiveVirtualizer.set(...) but the loop terminates because _willUpdate only
rebinds when this.scrollElement changes and maybeNotify's memo prevents spurious
onChange calls; also add an E2E test exercising scroll + ResizeObserver-style
measurement updates to assert no infinite re-render and that only a single extra
render per frame occurs at worst to prevent regressions.
In `@packages/angular-virtual/src/proxy.ts`:
- Around line 79-91: The getOwnPropertyDescriptor trap is returning a descriptor
for every property which violates proxy invariants and conflicts with
has/ownKeys; update getOwnPropertyDescriptor in the proxy to check existence via
untracked(inputSignal) (or Reflect.has/Reflect.getOwnPropertyDescriptor on that
object) and return undefined when the property does not exist, and when it does
exist return the actual descriptor (or a descriptor that preserves
configurable/enumerable/writable from the real property) instead of a blanket
always-present descriptor; adjust references to getOwnPropertyDescriptor, has,
ownKeys, untracked, and inputSignal accordingly.
---
Nitpick comments:
In `@packages/angular-virtual/e2e/app/src/app/scroll.component.ts`:
- Around line 11-25: The current randomHeightForKey uses Math.random via
getRandomInt which produces non-deterministic row heights; replace the
randomness with a key-based pseudo-random generator so heights are stable across
runs: inside randomHeightForKey (and remove reliance on getRandomInt/Math.random
for this path) compute a deterministic numeric seed from the id (e.g., a simple
string hash of id), map that seed into the desired range [25,100] to produce v,
store v in the existing cache Map<string, number>, and return it; keep the cache
logic unchanged so repeated calls return the same value for the same key.
In `@packages/angular-virtual/e2e/app/src/app/smooth-scroll.component.ts`:
- Around line 11-25: The current randomHeightForKey uses Math.random so heights
vary between runs; replace the randomness with a deterministic function that
maps the id string to a stable integer in the 25–100 range. Modify the
randomHeightForKey closure (and keep the cache Map) to compute a simple string
hash from the id (e.g., accumulate char codes with multipliers) and then
normalize that hash into the desired range instead of calling
getRandomInt/Math.random; store and return the computed value from the existing
cache so repeated calls still return the same height for the same id.
In `@packages/angular-virtual/e2e/app/test/measure-element.spec.ts`:
- Around line 29-41: Replace the brittle fixed sleep (page.waitForTimeout(200))
with a poll that retries the layout assertion until it's true or a timeout
elapses: use page.waitForFunction (or a small loop) to repeatedly query the
locators via page.locator('[data-testid="item-b"]').boundingBox() and
page.locator('[data-testid="item-c"]').boundingBox() and assert that cBox.y >=
(bBox.y + bBox.height) - 1 (preserve the 1px tolerance); ensure you still
null-check bounding boxes and fail the test if the waitForFunction times out so
CI will report the layout regression.
In `@packages/angular-virtual/e2e/app/test/scroll.spec.ts`:
- Around line 25-124: Replace the fixed waits (await page.waitForTimeout(...))
with condition-based polling/assertions: for tests that wait for an item to
appear use await expect(page.locator('[data-testid="item-1000"]')).toBeVisible()
/ '[data-testid="item-1001"]' instead of sleep; for "scrolls to last item" poll
the container scroll position (query '#scroll-container' and assert
Math.abs(scrollTop + clientHeight - scrollHeight) < 1.01) using expect.poll or a
locator assertion; for initialOffset replace the 500ms sleep by polling until
the minimum [data-index] in '#scroll-container' is > 0, and after performing the
user scroll poll until container.scrollTop has decreased by ~2000; for layout
checks poll until layout.items.length > 3 and layout.maxGap < 2; for the final
"scrolls to index 0" replace the two 1s sleeps with polling for visibility of
'[data-testid="item-1000"]' after first click and then for
'[data-testid="item-0"]' (or container scrollTop≈0) after the second click. Use
the existing selectors ('#scroll-container', '[data-index]',
'[data-testid="item-*"]') and helper checks inside expect.poll to determine
readiness instead of fixed timeouts.
In `@packages/angular-virtual/e2e/app/test/smooth-scroll.spec.ts`:
- Around line 7-139: The tests use many fixed page.waitForTimeout(2000) sleeps
(e.g., around interactions with '#scroll-to-1000', '#scroll-to-500',
'#scroll-to-100', '#scroll-to-0', '#scroll-to-500-start',
'#scroll-to-500-center') which slows and flakes CI; replace each sleep with
condition-based waits such as awaiting the locator to be visible (await
expect(page.locator('[data-testid="item-1000"]')).toBeVisible()) or using
expect.poll()/locator.waitFor() to wait until the scroll container
('#scroll-container') has the expected scrollTop or the item geometry (computed
via page.evaluate) is within tolerance, and for interrupt tests poll until the
final target is visible/settled instead of arbitrary timeouts so each test waits
for the actual condition rather than a fixed delay.
In `@packages/angular-virtual/e2e/app/test/stale-index.spec.ts`:
- Around line 13-37: Replace the non-deterministic waits (await
page.waitForTimeout(100) and 200) with a deterministic layout/paint flush using
a double requestAnimationFrame promise after operations that change layout (the
scrollTop set on container and after clicking '[data-testid="remove-items"]').
Specifically, after manipulating the DOM in the test (the
container.evaluate(...) call and the await
page.click('[data-testid="remove-items"]') calls) insert a browser-side double
RAF flush (via page.evaluate(() => new Promise(r => requestAnimationFrame(() =>
requestAnimationFrame(r))))) and/or use a page.waitForFunction that checks the
state of the '[data-testid="item-count"]' or absence of '[data-testid="error"]'
before asserting; replace references to waitForTimeout with that deterministic
flush around the item-count and error assertions.
In `@packages/angular-virtual/src/index.ts`:
- Line 82: The computed wrapper around the side‑effectful constructor (const
lazyVirtualizer = computed(() => new Virtualizer(untracked(resolvedOptions))))
should be replaced with an explicit lazy initialization to avoid participating
in the reactive graph; introduce a module/local variable like let virtualizer:
Virtualizer | undefined and replace uses of lazyVirtualizer.value with a
one‑shot factory/getter that does if (!virtualizer) virtualizer = new
Virtualizer(untracked(resolvedOptions)), returning virtualizer; optionally wire
any cleanup logic to Virtualizer disposal where appropriate. Ensure you
reference Virtualizer, resolvedOptions, untracked and remove the computed
import/usages for lazyVirtualizer.
- Around line 186-195: The SSR guards currently check typeof document but then
read window (in getScrollElement and initialOffset), which is misleading and can
break in runtimes that polyfill one but not the other; update the guards to
check typeof window !== 'undefined' where you actually access window (change the
checks used in the AngularVirtualizer factory for getScrollElement and
initialOffset), and ensure you still return null/0 when window is unavailable so
observeElementRect/observeElementOffset/scrollToFn logic remains safe.
- Around line 99-139: The call to signalProxy is double-cast to
AngularVirtualizer and the four string-array lists (methods pass-through,
attributes-to-signals, tracked methods, zero-arg computed) are not type-checked
against the AngularVirtualizer/Virtualizer shape, so new signal-wrapped
properties can be missed at compile time; update the call site to assert
exhaustiveness by replacing the unknown cast with a TypeScript-checked construct
(e.g., use a typed helper or the satisfies operator) that verifies the union of
the arrays equals the set of signal-wrapped keys on
AngularVirtualizer<TScrollElement, TItemElement>, referencing signalProxy,
AngularVirtualizer, and the specific property names (e.g., getTotalSize,
getVirtualItems, isScrolling, measurementsCache, options, range,
scrollDirection, scrollElement, scrollOffset, scrollRect) so the compiler will
error if any property is omitted or changed.
In `@packages/virtual-core/tests/index.test.ts`:
- Around line 340-353: The test suite defines the same ResizeObserver mock
factory twice (MockResizeObserver) — extract a shared helper named
createMockResizeObserver() that returns the vi.fn factory producing { observe,
unobserve, disconnect } mocks, replace both MockResizeObserver occurrences with
calls to createMockResizeObserver() and assign its result to
mockWindow.ResizeObserver and the earlier test’s equivalent; this ensures both
tests reuse the same factory function (refer to MockResizeObserver and
mockWindow.ResizeObserver to locate the spots to replace).
🪄 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
Run ID: 9a5dc6a9-4037-48e0-83c0-e29cc3810f96
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (70)
.changeset/gold-days-greet.mddocs/framework/angular/angular-virtual.mdexamples/angular/dynamic/package.jsonexamples/angular/fixed/package.jsonexamples/angular/infinite-scroll/package.jsonexamples/angular/padding/package.jsonexamples/angular/smooth-scroll/package.jsonexamples/angular/sticky/package.jsonexamples/angular/table/package.jsonexamples/angular/variable/package.jsonexamples/angular/window/package.jsonexamples/lit/dynamic/package.jsonexamples/lit/fixed/package.jsonexamples/react/dynamic/package.jsonexamples/react/fixed/package.jsonexamples/react/infinite-scroll/package.jsonexamples/react/padding/package.jsonexamples/react/scroll-padding/package.jsonexamples/react/smooth-scroll/package.jsonexamples/react/sticky/package.jsonexamples/react/table/package.jsonexamples/react/variable/package.jsonexamples/react/window/package.jsonexamples/svelte/dynamic/package.jsonexamples/svelte/fixed/package.jsonexamples/svelte/infinite-scroll/package.jsonexamples/svelte/smooth-scroll/package.jsonexamples/svelte/sticky/package.jsonexamples/svelte/table/package.jsonexamples/vue/dynamic/package.jsonexamples/vue/fixed/package.jsonexamples/vue/infinite-scroll/package.jsonexamples/vue/padding/package.jsonexamples/vue/scroll-padding/package.jsonexamples/vue/smooth-scroll/package.jsonexamples/vue/sticky/package.jsonexamples/vue/table/package.jsonexamples/vue/variable/package.jsonknip.jsonpackage.jsonpackages/angular-virtual/README.mdpackages/angular-virtual/angular.jsonpackages/angular-virtual/e2e/app/angular.jsonpackages/angular-virtual/e2e/app/src/app/app.component.tspackages/angular-virtual/e2e/app/src/app/app.config.tspackages/angular-virtual/e2e/app/src/app/app.routes.tspackages/angular-virtual/e2e/app/src/app/measure-element.component.tspackages/angular-virtual/e2e/app/src/app/scroll.component.tspackages/angular-virtual/e2e/app/src/app/smooth-scroll.component.tspackages/angular-virtual/e2e/app/src/app/stale-index.component.tspackages/angular-virtual/e2e/app/src/index.htmlpackages/angular-virtual/e2e/app/src/main.tspackages/angular-virtual/e2e/app/src/styles.csspackages/angular-virtual/e2e/app/test/measure-element.spec.tspackages/angular-virtual/e2e/app/test/scroll.spec.tspackages/angular-virtual/e2e/app/test/smooth-scroll.spec.tspackages/angular-virtual/e2e/app/test/stale-index.spec.tspackages/angular-virtual/e2e/app/tsconfig.app.jsonpackages/angular-virtual/e2e/app/tsconfig.jsonpackages/angular-virtual/ng-package.jsonpackages/angular-virtual/package.jsonpackages/angular-virtual/playwright.config.tspackages/angular-virtual/src/index.tspackages/angular-virtual/src/proxy.tspackages/angular-virtual/src/types.tspackages/angular-virtual/tsconfig.build.jsonpackages/angular-virtual/tsconfig.jsonpackages/angular-virtual/vite.config.tspackages/react-virtual/e2e/app/test/scroll.spec.tspackages/virtual-core/tests/index.test.ts
💤 Files with no reviewable changes (2)
- packages/angular-virtual/ng-package.json
- packages/angular-virtual/tsconfig.build.json
| "typescript": "5.4.5", | ||
| "vite": "^5.4.19" | ||
| "typescript": "5.6.3", | ||
| "vite": "^6.4.2" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What Vite versions are supported by @sveltejs/vite-plugin-svelte 3.1.2? Is Vite 6.4.2 supported?
💡 Result:
The exact Vite peer dependency range for @sveltejs/vite-plugin-svelte 3.1.2 is not explicitly stated in the searched sources. However, based on the changelog and release history: - Version 3.0.0 bumped support to Vite 5 (breaking change from previous versions). - Version 3.1.0 (close to 3.1.2) added Svelte 5 HMR support but no Vite change mentioned. - Later versions like 6.x bumped to ^6.3.0 || ^7.0.0 (PR #1130, June 2025). Vite 6.4.2 (a 6.x version post-6.3.0) is likely supported by 3.1.2 if 3.1.2 uses Vite ^5.0.0, as semver ^5 allows 6.x. But without the exact package.json for 3.1.2, confirmation is medium confidence. Official sources recommend checking the package.json peerDependencies directly via npm view @sveltejs/vite-plugin-svelte@3.1.2 peerDependencies.
Citations:
- 1: https://github.com/sveltejs/vite-plugin-svelte/releases
- 2: https://github.com/sveltejs/vite-plugin-svelte/blob/main/packages/vite-plugin-svelte/CHANGELOG.md
- 3: https://github.com/sveltejs/vite-plugin-svelte/blob/HEAD/packages/vite-plugin-svelte/CHANGELOG.md
- 4: https://newreleases.io/project/github/sveltejs/vite-plugin-svelte/release/@sveltejs%2Fvite-plugin-svelte@3.1.0
- 5: update vite peerdependency sveltejs/vite-plugin-svelte#1130
🏁 Script executed:
npm view `@sveltejs/vite-plugin-svelte`@3.1.2 peerDependencies --jsonRepository: TanStack/virtual
Length of output: 121
🏁 Script executed:
# Also check if there are known compatibility issues in the GitHub repo
# Search for any discussions about Vite 6 compatibility with 3.1.2
cd /tmp && git clone --depth 1 https://github.com/sveltejs/vite-plugin-svelte.git 2>/dev/null || true
if [ -d vite-plugin-svelte ]; then
cd vite-plugin-svelte
# Check recent issues/discussions about Vite 6 compatibility
git log --oneline --grep="Vite 6" --all | head -20
git log --oneline --grep="3.1.2" --all | head -20
fiRepository: TanStack/virtual
Length of output: 42
Fix Vite and Svelte plugin version incompatibility.
Line 22 upgrades to vite@^6.4.2, but @sveltejs/vite-plugin-svelte@^3.1.2 requires vite@^5.0.0. This incompatibility will break the example. Either upgrade the plugin to a version supporting Vite 6 or downgrade Vite to 5.x.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/svelte/dynamic/package.json` at line 22, The package.json currently
sets "vite": "^6.4.2" which is incompatible with the pinned
"@sveltejs/vite-plugin-svelte@^3.1.2" that requires Vite ^5; fix by making the
two dependencies compatible: either downgrade the Vite entry in package.json to
a Vite 5.x range (e.g. ^5.0.0) so it satisfies
"@sveltejs/vite-plugin-svelte@^3.1.2", or upgrade "@sveltejs/vite-plugin-svelte"
to a release that explicitly supports Vite 6 and adjust the plugin version
string accordingly; find and update the "vite" and
"@sveltejs/vite-plugin-svelte" dependency entries in package.json to match one
of these compatible pairs.
Comment on lines
+21
to
+22
| "typescript": "5.6.3", | ||
| "vite": "^6.4.2" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only dependency compatibility check (no repo files required)
set -euo pipefail
npm view vite@6.4.2 version
npm view `@sveltejs/vite-plugin-svelte`@3.1.2 peerDependencies --json
npm view `@sveltejs/vite-plugin-svelte` version
npm view `@sveltejs/vite-plugin-svelte` peerDependencies --jsonRepository: TanStack/virtual
Length of output: 196
Resolve Vite 6 compatibility: @sveltejs/vite-plugin-svelte@3.1.2 requires Vite ^5.0.0, not 6.x.
Line 22 upgrades Vite to ^6.4.2, but the Svelte Vite plugin at Line 16 (@sveltejs/vite-plugin-svelte@^3.1.2) has a peer dependency of vite@^5.0.0. This creates an incompatibility. Either upgrade the Svelte plugin to a version supporting Vite 6 or downgrade Vite to 5.x.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/svelte/smooth-scroll/package.json` around lines 21 - 22, The
package.json shows an incompatible dependency set:
"@sveltejs/vite-plugin-svelte@^3.1.2" requires Vite ^5.x but "vite": "^6.4.2"
was added; fix by aligning versions—either upgrade the Svelte Vite plugin to a
release that lists Vite 6 as a peer (replace "@sveltejs/vite-plugin-svelte"
version accordingly) or downgrade "vite" to a 5.x version that satisfies the
plugin's peer dependency; update the package.json dependency entries for "vite"
or "@sveltejs/vite-plugin-svelte" and run install to verify peer compatibility.
| @@ -0,0 +1,60 @@ | |||
| { | |||
| "$schema": "../../../node_modules/@angular/cli/lib/config/schema.json", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
base = Path("packages/angular-virtual/e2e/app")
current = (base / "../../../node_modules/@angular/cli/lib/config/schema.json").resolve()
expected = (base / "../../../../node_modules/@angular/cli/lib/config/schema.json").resolve()
print("current:", current)
print("expected:", expected)
PYRepository: TanStack/virtual
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Check if the file exists and show its content around line 2
if [ -f "packages/angular-virtual/e2e/app/angular.json" ]; then
echo "=== File exists ==="
head -10 "packages/angular-virtual/e2e/app/angular.json"
else
echo "File does not exist"
fi
# Check node_modules structure
echo ""
echo "=== Checking node_modules locations ==="
if [ -d "node_modules/@angular/cli" ]; then
echo "✓ Root node_modules/@angular/cli exists"
ls -la "node_modules/@angular/cli/lib/config/schema.json" 2>/dev/null | head -1 || echo " schema.json not found at root"
fi
if [ -d "packages/node_modules/@angular/cli" ]; then
echo "✓ packages/node_modules/@angular/cli exists"
ls -la "packages/node_modules/@angular/cli/lib/config/schema.json" 2>/dev/null | head -1 || echo " schema.json not found in packages"
else
echo "✗ packages/node_modules/@angular/cli does not exist"
fi
# Check directory structure
echo ""
echo "=== Directory structure ==="
ls -la "packages/angular-virtual/e2e/app/" 2>/dev/null | head -10 || echo "Directory not found"Repository: TanStack/virtual
Length of output: 818
🏁 Script executed:
#!/bin/bash
# Check if the schema exists at the root level (the expected path)
if [ -f "node_modules/@angular/cli/lib/config/schema.json" ]; then
echo "✓ Root level schema exists: node_modules/@angular/cli/lib/config/schema.json"
wc -l "node_modules/@angular/cli/lib/config/schema.json"
else
echo "✗ Root level schema does NOT exist"
fi
# Double check what's in packages root
echo ""
echo "=== Checking packages/ root ==="
if [ -d "packages/" ]; then
ls -la packages/ | head -20
fiRepository: TanStack/virtual
Length of output: 677
🏁 Script executed:
#!/bin/bash
# Search for `@angular/cli` anywhere in the repo
echo "=== Searching for `@angular/cli` ==="
find . -type d -name "@angular" 2>/dev/null | head -20
echo ""
echo "=== Checking if node_modules exists at repo root ==="
if [ -d "node_modules" ]; then
echo "✓ node_modules exists"
ls -la node_modules | head -20
else
echo "✗ node_modules does NOT exist at repo root"
fi
echo ""
echo "=== Checking root package.json for dependencies ==="
if [ -f "package.json" ]; then
cat package.json | head -30
fiRepository: TanStack/virtual
Length of output: 2597
🏁 Script executed:
#!/bin/bash
# Check if the e2e/app/angular.json is referenced anywhere
echo "=== Searching for references to e2e/app/angular.json ==="
rg "e2e/app" packages/angular-virtual/ 2>/dev/null || echo "No references found"
echo ""
echo "=== Check packages/angular-virtual package.json ==="
if [ -f "packages/angular-virtual/package.json" ]; then
cat "packages/angular-virtual/package.json"
fi
echo ""
echo "=== Check packages/angular-virtual/e2e structure ==="
find packages/angular-virtual/e2e -type f 2>/dev/null | head -20Repository: TanStack/virtual
Length of output: 3644
🏁 Script executed:
#!/bin/bash
# Since this is a pnpm monorepo, check where `@angular/cli` actually is
echo "=== Searching for `@angular/cli` in .pnpm ==="
find node_modules/.pnpm -type d -name "@angular/cli*" 2>/dev/null | head -5
echo ""
echo "=== Check if schema exists in the pnpm structure ==="
find node_modules -name "schema.json" -path "*@angular/cli*" 2>/dev/null
echo ""
echo "=== Check what's actually resolved from packages/angular-virtual/e2e/app ==="
python3 <<'EOF'
from pathlib import Path
import os
# Simulate from the e2e/app directory
os.chdir("packages/angular-virtual/e2e/app")
current_dir = Path.cwd()
# Try to resolve the current path
current_path = current_dir / "../../../node_modules/@angular/cli/lib/config/schema.json"
expected_path = current_dir / "../../../../node_modules/@angular/cli/lib/config/schema.json"
print("Current working dir:", current_dir)
print()
print("Current path resolution:")
print(f" Path: {current_path}")
print(f" Resolved: {current_path.resolve()}")
print(f" Exists: {current_path.resolve().exists()}")
print()
print("Expected path resolution:")
print(f" Path: {expected_path}")
print(f" Resolved: {expected_path.resolve()}")
print(f" Exists: {expected_path.resolve().exists()}")
print()
# Check what's actually at each resolved location
current_resolved = current_path.resolve()
expected_resolved = expected_path.resolve()
# Check parents
print("Current path parent exists:", current_resolved.parent.exists())
print("Expected path parent exists:", expected_resolved.parent.exists())
if expected_resolved.parent.exists():
print("Files in expected parent:", list(expected_resolved.parent.parent.iterdir())[:5])
EOFRepository: TanStack/virtual
Length of output: 871
🏁 Script executed:
#!/bin/bash
# Let's check how other Angular configs in this monorepo reference node_modules
echo "=== Check packages/angular-virtual/angular.json (parent config) ==="
grep -n "schema" packages/angular-virtual/angular.json
echo ""
echo "=== Check if there are other nested angular.json files ==="
find . -name "angular.json" -type f 2>/dev/null
echo ""
echo "=== Verify pnpm structure - check if packages has its own node_modules ==="
ls -la packages/ | grep node_modules
echo "Exit code: $?"
echo ""
echo "=== In a pnpm monorepo, check root .pnpm for `@angular` ==="
ls -d node_modules/.pnpm/@angular* 2>/dev/null | head -5 || echo "Not found in .pnpm"Repository: TanStack/virtual
Length of output: 865
🏁 Script executed:
#!/bin/bash
# Check how other nested angular.json files (in examples) reference the schema
echo "=== Check one of the example angular.json files (nested) ==="
head -5 examples/angular/dynamic/angular.json
echo ""
echo "=== Verify directory depth for examples vs e2e/app ==="
python3 <<'EOF'
from pathlib import Path
# Compare relative paths
example = Path("examples/angular/dynamic/angular.json")
e2e = Path("packages/angular-virtual/e2e/app/angular.json")
parent = Path("packages/angular-virtual/angular.json")
print(f"Parent config depth: {len(parent.parts)}")
print(f"E2E config depth: {len(e2e.parts)}")
print(f"Example config depth: {len(example.parts)}")
print()
print(f"Parent relative to root: {parent.parts}")
print(f"E2E relative to root: {e2e.parts}")
print(f"Example relative to root: {example.parts}")
print()
# Calculate how many levels up each needs to go to reach root node_modules
parent_levels = len(parent.parts) - 1 # -1 for the filename
e2e_levels = len(e2e.parts) - 1
example_levels = len(example.parts) - 1
print(f"Parent needs to go up: {parent_levels} levels")
print(f"E2E needs to go up: {e2e_levels} levels")
print(f"Example needs to go up: {example_levels} levels")
EOFRepository: TanStack/virtual
Length of output: 699
Fix the nested Angular schema path to reach the root node_modules.
The e2e/app/angular.json is 5 levels deep from the repository root, requiring 4 directory traversals to reach the root node_modules. The current path uses only 3 traversals (../../../), which points to the non-existent packages/node_modules.
🛠️ Proposed fix
- "$schema": "../../../node_modules/@angular/cli/lib/config/schema.json",
+ "$schema": "../../../../node_modules/@angular/cli/lib/config/schema.json",📝 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
| "$schema": "../../../node_modules/@angular/cli/lib/config/schema.json", | |
| "$schema": "../../../../node_modules/@angular/cli/lib/config/schema.json", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-virtual/e2e/app/angular.json` at line 2, The "$schema" path
in e2e/app/angular.json is incorrect (uses "../../../node_modules/...") and
points to packages/node_modules; update the schema string to traverse four
levels up (use "../../../../node_modules/@angular/cli/lib/config/schema.json")
so it resolves to the repository root node_modules; locate the "$schema"
property in that file and replace the relative path accordingly.
Comment on lines
+1
to
+9
| import { | ||
| ChangeDetectionStrategy, | ||
| Component, | ||
| ElementRef, | ||
| afterRenderEffect, | ||
| signal, | ||
| viewChild, | ||
| viewChildren, | ||
| } from '@angular/core' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bElementRef\b|#measureItems|afterRenderEffect' packages/angular-virtual/e2e/app/src/app/stale-index.component.tsRepository: TanStack/virtual
Length of output: 568
🏁 Script executed:
cat -n packages/angular-virtual/e2e/app/src/app/stale-index.component.tsRepository: TanStack/virtual
Length of output: 3366
Clean up the lint blockers.
ElementRef is used only as a type parameter and should be imported as a type. The private #measureItems field is unused—the afterRenderEffect should be called in the constructor without storing the handle.
🧹 Proposed lint fix
import {
ChangeDetectionStrategy,
Component,
- ElementRef,
afterRenderEffect,
signal,
viewChild,
viewChildren,
} from '@angular/core'
+import type { ElementRef } from '@angular/core'
import { injectVirtualizer } from '@tanstack/angular-virtual'
@@
- `#measureItems` = afterRenderEffect({
- read: () => {
- this.virtualItems().forEach((el) => {
- this.virtualizer.measureElement(el.nativeElement)
- })
- },
- })
-
virtualizer = injectVirtualizer(() => ({
scrollElement: this.scrollElement(),
count: this.items().length,
estimateSize: () => 50,
@@
return items[index]!.id
},
}))
+
+ constructor() {
+ afterRenderEffect({
+ read: () => {
+ this.virtualItems().forEach((el) => {
+ this.virtualizer.measureElement(el.nativeElement)
+ })
+ },
+ })
+ }🧰 Tools
🪛 ESLint
[error] 1-9: Imports "ElementRef" are only used as type.
(@typescript-eslint/consistent-type-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-virtual/e2e/app/src/app/stale-index.component.ts` around
lines 1 - 9, ElementRef is only used as a type and the private field
`#measureItems` is unused; update the import to import ElementRef as a type-only
import and remove the private `#measureItems` field, then call afterRenderEffect
inside the component constructor without assigning its return value to any
property (i.e., invoke afterRenderEffect(...) in the constructor), keeping
references to the functions/classes like afterRenderEffect, ElementRef, and the
component constructor to locate the changes.
Comment on lines
+39
to
+52
| "engines": { | ||
| "node": ">=12" | ||
| }, | ||
| "files": [ | ||
| "build" | ||
| "dist", | ||
| "src" | ||
| ], | ||
| "scripts": { | ||
| "clean": "premove ./build", | ||
| "test:types": "tsc --noEmit", | ||
| "clean": "premove ./dist ./build ./coverage", | ||
| "test:eslint": "eslint ./src", | ||
| "build": "ng-packagr -p ng-package.json -c tsconfig.build.json" | ||
| "test:build": "publint --strict", | ||
| "test:types": "tsc", | ||
| "build": "vite build", | ||
| "test:e2e": "../../node_modules/.bin/playwright test" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
npm view `@angular/cli`@19.0.0 engines --json
npm view `@angular-devkit/build-angular`@19.0.0 engines --json
npm view vite engines --jsonRepository: TanStack/virtual
Length of output: 323
Update engines.node to match Angular 19 and Vite requirements.
The package currently advertises Node 12 support, but the build toolchain now requires a newer version. Angular 19 and Vite have stricter engine requirements. Vite specifically requires Node 20.19.0 or 22.12.0+, which is more restrictive than Angular's requirements alone.
🛠️ Corrected metadata update
"engines": {
- "node": ">=12"
+ "node": "^20.19.0 || >=22.12.0"
},📝 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
| "engines": { | |
| "node": ">=12" | |
| }, | |
| "files": [ | |
| "build" | |
| "dist", | |
| "src" | |
| ], | |
| "scripts": { | |
| "clean": "premove ./build", | |
| "test:types": "tsc --noEmit", | |
| "clean": "premove ./dist ./build ./coverage", | |
| "test:eslint": "eslint ./src", | |
| "build": "ng-packagr -p ng-package.json -c tsconfig.build.json" | |
| "test:build": "publint --strict", | |
| "test:types": "tsc", | |
| "build": "vite build", | |
| "test:e2e": "../../node_modules/.bin/playwright test" | |
| "engines": { | |
| "node": "^20.19.0 || >=22.12.0" | |
| }, | |
| "files": [ | |
| "dist", | |
| "src" | |
| ], | |
| "scripts": { | |
| "clean": "premove ./dist ./build ./coverage", | |
| "test:eslint": "eslint ./src", | |
| "test:build": "publint --strict", | |
| "test:types": "tsc", | |
| "build": "vite build", | |
| "test:e2e": "../../node_modules/.bin/playwright test" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-virtual/package.json` around lines 39 - 52, The package.json
currently sets "engines.node" to ">=12", which is too low for Angular 19 + Vite;
update the "engines" entry in packages/angular-virtual package.json so the
"node" constraint is bumped to a Vite-compatible minimum (e.g., ">=20.19.0") to
match toolchain requirements, and ensure any related CI configs or documentation
that reference Node version are aligned with the new constraint; locate the
"engines" object and change the "node" value accordingly.
| # Quick Start | ||
|
|
||
| > NOTE: Angular Virtual requires Angular 17. | ||
| > NOTE: Angular Virtual requires Angular 19. |
There was a problem hiding this comment.
Clarify version floor as “Angular 19+” to match support policy.
Line 7 currently reads as exactly Angular 19; this can conflict with the stated LTS-and-up direction and >=19 peer range. Consider updating to “requires Angular 19+”.
✏️ Suggested doc tweak
-> NOTE: Angular Virtual requires Angular 19.
+> NOTE: Angular Virtual requires Angular 19+.📝 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
| > NOTE: Angular Virtual requires Angular 19. | |
| > NOTE: Angular Virtual requires Angular 19+. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-virtual/README.md` at line 7, Update the README note that
currently reads "NOTE: Angular Virtual requires Angular 19." to explicitly state
the supported floor as "Angular 19+" so it matches the package peer range (>=19)
and the project’s LTS-and-up policy; locate the note string in the README (the
line beginning with "NOTE: Angular Virtual requires") and change the trailing
text from "Angular 19." to "Angular 19+".
Comment on lines
+95
to
+97
| afterRenderEffect(() => { | ||
| reactiveVirtualizer()._willUpdate() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether _willUpdate paths in virtual-core can call onChange and under what conditions.
rg -nP -C3 '_willUpdate|_didMount' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 342
🏁 Script executed:
# Get the full _willUpdate method from virtual-core
sed -n '534,570p' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 1095
🏁 Script executed:
# Check the angular-virtual implementation around lines 73, 89, 95-97
cat -n packages/angular-virtual/src/index.ts | sed -n '70,100p'Repository: TanStack/virtual
Length of output: 1084
🏁 Script executed:
# Search for onChange calls in virtual-core to understand when it's triggered
rg -nP 'onChange' packages/virtual-core/src/index.ts | head -20Repository: TanStack/virtual
Length of output: 152
🏁 Script executed:
# Check what maybeNotify does in virtual-core
rg -nP -A10 'maybeNotify\s*=\s*\(' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 42
🏁 Script executed:
# Check if measurement callbacks (observeElementRect) trigger onChange flow
rg -nP 'measureElement|observeElementRect' packages/virtual-core/src/index.ts | head -15Repository: TanStack/virtual
Length of output: 535
🏁 Script executed:
# Search for where onChange is actually called in virtual-core
rg -nP -B5 -A5 'options\.onChange' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 284
🏁 Script executed:
# Get the full maybeNotify implementation
rg -nP -A20 'private maybeNotify' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 742
🏁 Script executed:
# Check the memo function and what it returns
rg -nP -A30 'maybeNotify = memo' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 1059
🏁 Script executed:
# Check how observeElementRect and observeElementOffset callbacks work
rg -nP -B3 -A10 'observeElementRect\(this,' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 517
🏁 Script executed:
# Verify that the linkedSignal change detection and effect scheduling
rg -nP 'linkedSignal|afterRenderEffect' packages/angular-virtual/src/index.tsRepository: TanStack/virtual
Length of output: 223
🏁 Script executed:
# Check if there are any guards or conditions that prevent re-triggering
rg -nP -B5 -A5 'reactiveVirtualizer.set' packages/angular-virtual/src/index.tsRepository: TanStack/virtual
Length of output: 494
🏁 Script executed:
# Check if there are any existing comments around the linkedSignal or afterRenderEffect
cat -n packages/angular-virtual/src/index.ts | sed -n '60,100p'Repository: TanStack/virtual
Length of output: 1569
🏁 Script executed:
# Check the entire _willUpdate method implementation to understand if it's safe to call multiple times
rg -nP -A60 '_willUpdate = \(\) => {' packages/virtual-core/src/index.ts | head -80Repository: TanStack/virtual
Length of output: 1899
🏁 Script executed:
# Look for any debouncing or guards that might prevent the re-entrancy issue
rg -nP 'debounce|throttle|guard|pending|inProgress' packages/angular-virtual/src/index.tsRepository: TanStack/virtual
Length of output: 42
🏁 Script executed:
# Check if there are any tests that cover this scenario
fd -e '\.spec\.ts' -e '\.test\.ts' packages/angular-virtual/ | head -10Repository: TanStack/virtual
Length of output: 465
🏁 Script executed:
# Look for any existing convergence checks or guards in the codebase
rg -nP 'scrollElement|getScrollElement' packages/angular-virtual/src/index.tsRepository: TanStack/virtual
Length of output: 460
🏁 Script executed:
# Check the full implementation to see if there's idempotency
cat -n packages/angular-virtual/src/index.ts | sed -n '84,97p'Repository: TanStack/virtual
Length of output: 577
🏁 Script executed:
# Look at the measurement flow more carefully - check if onChange is throttled/debounced
rg -nP -B10 -A10 'notify.*=' packages/virtual-core/src/index.ts | head -40Repository: TanStack/virtual
Length of output: 612
🏁 Script executed:
# Check what happens when scrollElement doesn't change on subsequent _willUpdate calls
rg -nP -B5 -A15 'this.scrollElement !== scrollElement' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 692
🏁 Script executed:
# Check if there are measurement-based tests
fd . packages/angular-virtual/ -name '*.spec.ts' -o -name '*.test.ts'Repository: TanStack/virtual
Length of output: 290
🏁 Script executed:
# Check the calculateRange implementation to understand memo dependencies
rg -nP -A15 'calculateRange\s*=' packages/virtual-core/src/index.ts | head -30Repository: TanStack/virtual
Length of output: 553
🏁 Script executed:
# Check if there's any documentation or tests about the re-entrancy behavior
rg -nP 'afterRenderEffect|linkedSignal' packages/angular-virtual/src/index.ts -A2 -B2Repository: TanStack/virtual
Length of output: 688
🏁 Script executed:
# Verify the actual control flow - does setOptions call onChange?
rg -nP -A20 'setOptions\s*=' packages/virtual-core/src/index.ts | head -40Repository: TanStack/virtual
Length of output: 1093
🏁 Script executed:
# Check if there's any test covering measurement updates
fd . packages/angular-virtual --type f -name '*.ts' | xargs rg -l 'measure|scroll' 2>/dev/null | head -5Repository: TanStack/virtual
Length of output: 462
🏁 Script executed:
# Check setOptions more carefully to see if it calls onChange
rg -nP -A30 'setOptions = \(opts' packages/virtual-core/src/index.tsRepository: TanStack/virtual
Length of output: 1107
🏁 Script executed:
# Check if there are any E2E tests for scroll measurement scenarios
fd . packages/angular-virtual --type f | xargs rg -l 'e2e|integration' 2>/dev/nullRepository: TanStack/virtual
Length of output: 227
🏁 Script executed:
# Verify the actual behavior by checking what happens in the linked-signal computation
cat -n packages/angular-virtual/src/index.ts | sed -n '82,98p'Repository: TanStack/virtual
Length of output: 686
🏁 Script executed:
# Check if there are E2E tests with measurement scenarios
fd . packages/angular-virtual/e2e --type f -name '*.ts' -o -name '*.spec.ts'Repository: TanStack/virtual
Length of output: 290
🏁 Script executed:
# Check the comment that's already there about setOptions
cat -n packages/angular-virtual/src/index.ts | sed -n '84,90p'Repository: TanStack/virtual
Length of output: 371
🏁 Script executed:
# Final verification - check if reactiveVirtualizer is actually reassigned or if set() creates a new instance
rg -nP 'linkedSignal.*equal' packages/angular-virtual/src/index.ts -B2 -A5Repository: TanStack/virtual
Length of output: 42
Document the expected re-entrancy convergence mechanism for this afterRenderEffect + linkedSignal pattern.
The effect reads reactiveVirtualizer(), which triggers the linkedSignal computation (due to equal: () => false at line 89), then calls _willUpdate(). When measurements change, maybeNotify() fires, calling onChange and setting reactiveVirtualizer.set(instance), which schedules another effect run.
This naturally converges because:
_willUpdatechecksif (this.scrollElement !== scrollElement)— observer setup only runs when the scroll element actually changes- On stable measurements,
maybeNotify's memo dependencies prevent spuriousonChangecalls
However, in measurement-heavy scenarios (e.g., dynamic content with ResizeObserver), this can cause an extra render cycle per frame. Consider adding a brief comment at line 95–97 explaining why the loop is expected to terminate, and cover this behavior with an E2E scroll-plus-measure test to prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-virtual/src/index.ts` around lines 95 - 97, Add a short
explanatory comment at the afterRenderEffect that reads
reactiveVirtualizer()._willUpdate(), describing the expected re-entrancy
convergence: explain that reactiveVirtualizer reads trigger the linkedSignal
(equal: () => false) which schedules maybeNotify → reactiveVirtualizer.set(...)
but the loop terminates because _willUpdate only rebinds when this.scrollElement
changes and maybeNotify's memo prevents spurious onChange calls; also add an E2E
test exercising scroll + ResizeObserver-style measurement updates to assert no
infinite re-render and that only a single extra render per frame occurs at worst
to prevent regressions.
Comment on lines
79
to
91
| has(_, property: string) { | ||
| return !!untracked(virtualizerSignal)[property as keyof V] | ||
| return property in untracked(inputSignal) | ||
| }, | ||
| ownKeys() { | ||
| return Reflect.ownKeys(untracked(virtualizerSignal)) | ||
| return Reflect.ownKeys(untracked(inputSignal)) | ||
| }, | ||
| getOwnPropertyDescriptor() { | ||
| return { | ||
| enumerable: true, | ||
| configurable: true, | ||
| writable: true, | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
const proxy = new Proxy(function () {}, {
has() {
return false
},
getOwnPropertyDescriptor() {
return { enumerable: true, configurable: true, writable: true }
},
})
console.log('missing' in proxy)
console.log(Object.hasOwn(proxy, 'missing'))
NODERepository: TanStack/virtual
Length of output: 70
🏁 Script executed:
cd packages/angular-virtual && find . -name "proxy.ts" -type fRepository: TanStack/virtual
Length of output: 74
🏁 Script executed:
cat -n packages/angular-virtual/src/proxy.ts | head -100Repository: TanStack/virtual
Length of output: 4377
🏁 Script executed:
wc -l packages/angular-virtual/src/proxy.tsRepository: TanStack/virtual
Length of output: 100
Fix proxy invariants: only return descriptors for existing properties.
The getOwnPropertyDescriptor trap currently returns a descriptor for every property, causing Object.hasOwn(proxy, 'missing') to return true even when missing in proxy is false. This violates proxy invariants and conflicts with the has trap. Only return a descriptor if the property exists in the target or input signal.
🐛 Proposed proxy descriptor fix
- has(_, property: string) {
+ has(_, property: PropertyKey) {
return property in untracked(inputSignal)
},
ownKeys() {
return Reflect.ownKeys(untracked(inputSignal))
},
- getOwnPropertyDescriptor() {
+ getOwnPropertyDescriptor(target, property) {
+ const targetDescriptor = Reflect.getOwnPropertyDescriptor(target, property)
+ if (targetDescriptor) {
+ return targetDescriptor
+ }
+
+ if (!(property in untracked(inputSignal))) {
+ return undefined
+ }
+
return {
enumerable: true,
configurable: true,
writable: true,
}📝 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
| has(_, property: string) { | |
| return !!untracked(virtualizerSignal)[property as keyof V] | |
| return property in untracked(inputSignal) | |
| }, | |
| ownKeys() { | |
| return Reflect.ownKeys(untracked(virtualizerSignal)) | |
| return Reflect.ownKeys(untracked(inputSignal)) | |
| }, | |
| getOwnPropertyDescriptor() { | |
| return { | |
| enumerable: true, | |
| configurable: true, | |
| writable: true, | |
| } | |
| }, | |
| has(_, property: PropertyKey) { | |
| return property in untracked(inputSignal) | |
| }, | |
| ownKeys() { | |
| return Reflect.ownKeys(untracked(inputSignal)) | |
| }, | |
| getOwnPropertyDescriptor(target, property) { | |
| const targetDescriptor = Reflect.getOwnPropertyDescriptor(target, property) | |
| if (targetDescriptor) { | |
| return targetDescriptor | |
| } | |
| if (!(property in untracked(inputSignal))) { | |
| return undefined | |
| } | |
| return { | |
| enumerable: true, | |
| configurable: true, | |
| writable: true, | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/angular-virtual/src/proxy.ts` around lines 79 - 91, The
getOwnPropertyDescriptor trap is returning a descriptor for every property which
violates proxy invariants and conflicts with has/ownKeys; update
getOwnPropertyDescriptor in the proxy to check existence via
untracked(inputSignal) (or Reflect.has/Reflect.getOwnPropertyDescriptor on that
object) and return undefined when the property does not exist, and when it does
exist return the actual descriptor (or a descriptor that preserves
configurable/enumerable/writable from the real property) instead of a blanket
always-present descriptor; adjust references to getOwnPropertyDescriptor, has,
ownKeys, untracked, and inputSignal accordingly.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/angular-virtual/package.json (1)
44-64:⚠️ Potential issue | 🟠 MajorMake the new Vite/Playwright toolchain self-contained and restore the Node floor.
buildnow invokesvite, whiletest:e2ehard-codes a rootnode_modules/.binpath for Playwright, but neither tool is declared in this package’s dev deps. That makes the package scripts depend on workspace layout and can break package-isolated installs or non-POSIX shells. Also, the Node engine guard is still missing after the Angular/Vite upgrade; Angular’s compatibility table and Vite’s guide make the Vite-backed floor stricter than the old package metadata. Sources: Angular version compatibility, Vite getting started.Suggested metadata direction
"files": [ "dist", "src" ], + "engines": { + "node": "^20.19.0 || ^22.12.0" + }, "scripts": { "clean": "premove ./dist ./build ./coverage", "test:eslint": "eslint ./src", "test:build": "publint --strict", "test:types": "tsc", "build": "vite build", - "test:e2e": "../../node_modules/.bin/playwright test" + "test:e2e": "playwright test" },Also declare
viteand the Playwright package that provides theplaywrightbinary indevDependencies, using the workspace’s chosen ranges/catalog entries.You can verify the current metadata with:
#!/bin/bash # Description: Check whether package scripts rely on undeclared local tool binaries # and compare current Angular/Vite engine requirements. set -euo pipefail node <<'NODE' const fs = require('fs') const pkg = JSON.parse(fs.readFileSync('packages/angular-virtual/package.json', 'utf8')) const rootPkg = JSON.parse(fs.readFileSync('package.json', 'utf8')) const allLocalDeps = { ...pkg.dependencies, ...pkg.devDependencies, ...pkg.peerDependencies, ...pkg.optionalDependencies, } const allRootDeps = { ...rootPkg.dependencies, ...rootPkg.devDependencies, ...rootPkg.peerDependencies, ...rootPkg.optionalDependencies, } for (const name of ['vite', 'playwright', '@playwright/test']) { console.log(`${name}: package=${allLocalDeps[name] ?? '<missing>'}, root=${allRootDeps[name] ?? '<missing>'}`) } console.log(`engines.node: ${pkg.engines?.node ?? '<missing>'}`) NODE npm view vite engines --json npm view `@angular/cli`@19 engines --json npm view `@angular-devkit/build-angular`@19 engines --json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-virtual/package.json` around lines 44 - 64, The package.json scripts are using tools that are not declared and a Node engine floor is missing: add vite and the Playwright package that provides the CLI (e.g. "playwright" or "@playwright/test" per your workspace catalog) to devDependencies (use workspace:* ranges consistent with the monorepo), update "test:e2e" to call the binary directly (e.g. "playwright test") instead of the hard-coded ../../node_modules/.bin path, and add an engines.node entry (set a Vite/Angular-compatible minimum, e.g. ">=18") to package.json to restore the Node floor. Ensure you update the devDependencies block (symbols: "vite", "playwright" or "@playwright/test") and the scripts ("build", "test:e2e") and add "engines.node".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/angular-virtual/package.json`:
- Around line 44-64: The package.json scripts are using tools that are not
declared and a Node engine floor is missing: add vite and the Playwright package
that provides the CLI (e.g. "playwright" or "@playwright/test" per your
workspace catalog) to devDependencies (use workspace:* ranges consistent with
the monorepo), update "test:e2e" to call the binary directly (e.g. "playwright
test") instead of the hard-coded ../../node_modules/.bin path, and add an
engines.node entry (set a Vite/Angular-compatible minimum, e.g. ">=18") to
package.json to restore the Node floor. Ensure you update the devDependencies
block (symbols: "vite", "playwright" or "@playwright/test") and the scripts
("build", "test:e2e") and add "engines.node".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 732c23d8-9a00-4fc8-9702-7b25cc415173
📒 Files selected for processing (8)
packages/angular-virtual/e2e/app/src/app/app.config.tspackages/angular-virtual/e2e/app/src/app/app.routes.tspackages/angular-virtual/e2e/app/src/app/measure-element.component.tspackages/angular-virtual/e2e/app/src/app/scroll.component.tspackages/angular-virtual/e2e/app/src/app/smooth-scroll.component.tspackages/angular-virtual/e2e/app/src/app/stale-index.component.tspackages/angular-virtual/package.jsonpackages/angular-virtual/src/proxy.ts
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.
🎯 Changes
items = this.rowVirtualizer.getVirtualItems.About #1096 and #1109, I couldn't replicate the issue with the latest version of virtual. With this upgrade to Angular 19, no effect could trigger directly set a value to a signal.
Pending:
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Documentation
Chores