feat(marko-virtual): add @tanstack/marko-virtual adapter for Marko v6#1156
Open
defunkt-dev wants to merge 13 commits intoTanStack:mainfrom
Open
feat(marko-virtual): add @tanstack/marko-virtual adapter for Marko v6#1156defunkt-dev wants to merge 13 commits intoTanStack:mainfrom
defunkt-dev wants to merge 13 commits intoTanStack:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new Marko 6 adapter package ( Changes
Sequence DiagramsequenceDiagram
participant Mount as Marko Mount
participant Tag as Marko Tag (virtualizer/window-virtualizer)
participant VC as `@tanstack/virtual-core`
participant Store as Instance Store
participant Obs as Observers (rect/offset/scroll)
participant DOM as Scroll Container / Window
Mount->>Tag: instantiate with Input (count, estimateSize, getScrollElement, ...)
Tag->>VC: create Virtualizer(options)
Tag->>Store: setInstance(id, { v, cleanup })
VC->>Obs: register observeRect/observeOffset/scroll
Obs->>DOM: attach listeners
Tag->>VC: call _didMount()
VC->>VC: compute initial virtualItems/totalSize
VC-->>Tag: onChange callback
Tag->>DOM: render virtual items via input.content
DOM->>Obs: user scrolls / resize
Obs->>VC: notify offset/rect changes
VC->>VC: recalc range
VC-->>Tag: onChange callback
Tag->>DOM: patch rendered items
Mount->>Tag: destroy
Tag->>Store: run cleanup and deleteInstance(id)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (3)
examples/marko/variable/src/data.ts (1)
1-2: Use deterministic generated sizes (or wire these exports into the page) to avoid demo drift.Lines 1-2 generate random data at module load, while
examples/marko/variable/src/routes/+page.markocurrently uses deterministic formulas and hardcoded counts. This creates two competing size sources.♻️ Suggested change
-export const rowSizes = Array.from({ length: 10000 }, () => 25 + Math.round(Math.random() * 100)) -export const colSizes = Array.from({ length: 10000 }, () => 75 + Math.round(Math.random() * 100)) +export const ROW_COUNT = 10000 +export const COL_COUNT = 10000 + +export const rowSizes = Array.from( + { length: ROW_COUNT }, + (_, i) => 25 + ((i * 17 + 31) % 100), +) +export const colSizes = Array.from( + { length: COL_COUNT }, + (_, i) => 75 + ((i * 13 + 43) % 100), +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/marko/variable/src/data.ts` around lines 1 - 2, The module exports rowSizes and colSizes are generated with Math.random() causing non-deterministic demo drift; replace the random generation with deterministic logic (e.g., a seeded PRNG or the same formula used in +page.marko) or expose these exports so +page.marko reads the authoritative sizes instead of duplicating logic; update rowSizes and colSizes generation (the Array.from calls) to produce stable values matching the counts/formulas used by +page.marko (or wire the page to import and use these exports) so both sources stay in sync.packages/marko-virtual/tests/index.test.ts (1)
199-221: Strengthen the horizontal sizing test to actually verify width behaviorLine 217 calls
_didMount()only._willUpdate()is what wires observers, so this test currently doesn’t validate the “uses width not height” claim.Proposed test update
- v._didMount() - // After mount with a 400px-wide container, virtual items should exist - const items = v.getVirtualItems() - expect(Array.isArray(items)).toBe(true) + const cleanup = v._didMount() + try { + v._willUpdate() + expect(v.scrollRect?.width).toBe(400) + expect(Array.isArray(v.getVirtualItems())).toBe(true) + } finally { + cleanup() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/marko-virtual/tests/index.test.ts` around lines 199 - 221, The test claims it verifies horizontal sizing but only calls v._didMount(), so it never wires the observers; update the test to call v._willUpdate() (or both v._didMount() then v._willUpdate()) to trigger the observeElementRect callback and ensure horizontal sizing logic runs; then assert that virtual items were produced based on width (e.g., container width 400 and estimateSize 100 should yield ~4 items) by using v.getVirtualItems() and checking its length or item sizes to confirm width was used instead of height.packages/marko-virtual/src/tags/virtualizer/index.marko (1)
64-66: Syncitems/sizeimmediately after initial_willUpdate()
onUpdatedoes an explicitnotify()after_willUpdate(), butonMountdoesn’t. Adding it here makes first render deterministic when observer callbacks are delayed.Proposed change
setInstance(id, { v, cleanup: v._didMount() }) v._willUpdate() + notify()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/marko-virtual/src/tags/virtualizer/index.marko` around lines 64 - 66, The initial mount path calls setInstance(id, { v, cleanup: v._didMount() }) and then v._willUpdate() but does not synchronize items/size or call the same notify() that onUpdate uses, which can make first render non-deterministic; after v._willUpdate() in the mount flow (within the code that uses setInstance and v._didMount()), invoke the same notify() used by onUpdate (or otherwise sync items/size immediately) so the first render state is consistent with update behavior—locate the mount branch that calls setInstance, v._didMount, and v._willUpdate and add the notify()/items/size synchronization there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/framework/marko/examples/dynamic.md`:
- Around line 7-11: The iframe element in the Marko example lacks a title
attribute required for screen readers; update the iframe markup (the <iframe ...
/> tag) to include a concise, descriptive title (e.g., "StackBlitz: Marko
dynamic example") so assistive tech can provide context, making sure the title
accurately reflects the embedded content.
In `@docs/framework/marko/examples/fixed.md`:
- Around line 7-11: The iframe element in the example lacks a title attribute
required for accessibility; update the iframe (the <iframe ... /> element shown)
to include a descriptive title attribute (e.g., title="StackBlitz:
TanStack/virtual Marko fixed example") so screen readers have context, leaving
the existing src, style, and sandbox attributes intact.
In `@docs/framework/marko/examples/grid.md`:
- Around line 7-11: The iframe in the Marko grid example is missing a title
attribute for accessibility; update the iframe element (the <iframe ... /> tag)
to include a descriptive title such as "StackBlitz: Marko Grid example" (or
similar) so screen readers get context, ensuring the title text clearly
identifies the embedded content.
In `@docs/framework/marko/examples/infinite-scroll.md`:
- Around line 7-11: The iframe in the Markdown example is missing a required
accessibility title; update the iframe element (the <iframe ... /> tag in the
example) to include a descriptive title attribute such as "StackBlitz: Marko
Infinite Scroll example" so screen readers can convey its purpose; ensure the
title is concise, descriptive, and added alongside the existing src, style, and
sandbox attributes.
In `@docs/framework/marko/examples/smooth-scroll.md`:
- Around line 7-11: The embedded iframe element lacks an accessible title;
update the iframe element (the <iframe ... /> in the example) to include a
descriptive title attribute (e.g., title="Smooth scroll Marko example" or other
meaningful text) so screen readers can identify its purpose.
In `@docs/framework/marko/examples/variable.md`:
- Around line 7-11: The iframe in the Marko example is missing a required
accessibility title; update the iframe element in
docs/framework/marko/examples/variable.md to include a descriptive title
attribute (e.g., title="StackBlitz: Marko variable example") so screen readers
have context. Locate the iframe tag shown in the diff and add a concise,
meaningful title attribute to the existing attributes string, ensuring it
remains relevant to the embedded example.
In `@docs/framework/marko/examples/window.md`:
- Around line 7-11: The iframe element in the example lacks a title attribute
required for accessibility (WCAG 2.4.2/4.1.2); update the iframe tag to include
a descriptive title (e.g., "StackBlitz: Marko window example" or similar) so
screen readers have context, keeping all existing attributes (src, style,
sandbox) unchanged.
In `@docs/framework/marko/marko-virtual.md`:
- Around line 173-177: The dynamic-sizing snippet is missing the required
data-index attribute on the measured element; update the <div> used with
measureElement to include data-index=item.index so the virtualizer can map DOM
measurements back to their corresponding items (ensure the same element that
calls measureElement(el()) — the div with style and the script block — has
data-index set to item.index to match the implementation in
packages/marko-virtual/README.md).
- Around line 195-196: Update the API docs to reflect the actual tag
implementations: change the types for scrollToIndex and scrollToOffset to use a
single ScrollToOptions type (replace references to ScrollToIndexOptions and
ScrollToOffsetOptions with ScrollToOptions) and update the method signatures for
scrollToIndex and scrollToOffset accordingly; also update the
<window-virtualizer> props section to list that it omits getScrollElement,
horizontal, and initialOffset (where initialOffset is present on <virtualizer>
but hardcoded/omitted in <window-virtualizer>), referencing the tag names
<window-virtualizer> and <virtualizer> and the prop names getScrollElement,
horizontal, and initialOffset so readers can find the implementations easily.
- Around line 9-10: Update the incorrect statement that Marko tags are
auto-discovered: change the text to say manual setup is required and instruct
users to add "@tanstack/marko-virtual/marko.json" to their project's marko.json
under the "taglib-imports" array (referencing the package README). Replace the
sentence "Tags are discovered automatically..." with a clear onboarding note
that includes the exact string "@tanstack/marko-virtual/marko.json" and mentions
updating the marko.json "taglib-imports" entry.
In `@examples/marko/fixed/src/routes/`+page.marko:
- Around line 30-37: Replace the plain browser <script> block that sets mounted
with a Marko reactive script so the component state updates; specifically,
change the existing plain <script> that assigns mounted to true to a reactive
<script()> block that sets mounted = true (ensuring the same mounted identifier
used by the <if=mounted> gate and the rowScroll virtualizer is updated
reactively).
In `@examples/marko/grid/src/data.ts`:
- Around line 3-4: The random-sizing lines use Math.round(Math.random()*N)
causing skew and extra endpoint; update rowSizes and colSizes to deterministic,
floor-based values computed from each index (ROW_COUNT/COL_COUNT) instead of
Math.random(); specifically replace the Array.from callbacks for rowSizes and
colSizes to use the element index (e.g., i) and a deterministic formula with
Math.floor (matching the route sizing logic) to produce values in the intended
ranges (30–50 for rows and 80–160 for cols) without rounding bias.
In `@examples/marko/variable/src/routes/`+page.marko:
- Line 25: Update the user-facing copy in the Marko page: replace the phrase
"knowable size" with "known size" in the sentence that currently reads "Each
item has a variable but knowable size — passed directly via" so the text becomes
clear and user-friendly.
In `@packages/marko-virtual/README.md`:
- Line 15: Update the README's peer dependency line to match package.json's
declared peer range: change the documented "marko >= 6.0.0" to "marko ^6.0.0"
(or otherwise mirror the exact string in package.json's peerDependencies) so the
README and the package.json peerDependencies entry remain consistent; locate the
README peer dependency line and the package.json "peerDependencies" object to
confirm and apply the exact same version specifier.
In `@packages/marko-virtual/src/marko.d.ts`:
- Around line 18-23: Add a second ambient module declaration for
"@marko/run/vite" that mirrors the existing "@marko/vite" declaration: import
Plugin from "vite" and declare the function marko(options?: Record<string,
unknown>): Plugin and export default marko, so consumers who import from either
"@marko/vite" or "@marko/run/vite" get the same type assistance; reference the
existing marko function declaration and the Plugin type import to duplicate for
the "@marko/run/vite" module.
In `@packages/marko-virtual/src/tags/virtualizer/index.marko`:
- Around line 99-104: The current onDestroy() calls entry.cleanup() and then
deleteInstance(id), but if entry.cleanup() throws the deleteInstance(id) is
never executed; modify onDestroy() so that getInstance(id) is retrieved once,
then execute entry.cleanup() inside a try/catch or try/finally and always call
deleteInstance(id) in the finally (or after catching) to guarantee the store
entry is removed even when cleanup throws; reference the onDestroy(),
getInstance(id), entry.cleanup(), and deleteInstance(id) symbols when making the
change.
In `@packages/marko-virtual/src/tags/window-virtualizer/index.marko`:
- Around line 88-89: The onUpdate path is calling v._willUpdate() but not
triggering a refresh of derived state, which can leave items and size stale;
update the handler (the onUpdate logic around v._willUpdate()) to call notify()
immediately after v._willUpdate() (mirroring virtualizer/index.marko) so items
and size are recomputed and the UI updates synchronously rather than waiting for
the next tick.
In `@packages/marko-virtual/tests/index.test.ts`:
- Around line 147-173: The test calls v._didMount() which returns a cleanup
function but never invokes it, risking leaked observer state; capture the
returned cleanup (e.g., const cleanup = v._didMount()) and invoke cleanup()
after the test assertions (or in a finally block) to unregister observers and
clean up state; ensure you still call v._willUpdate() as before and run
cleanup() at the end of the test.
- Around line 13-24: Reorder the imported specifiers to satisfy sort-imports:
alphabetize the vitest import as "beforeEach, describe, expect, test, vi" and
alphabetize the local import from '../src/index' as "defaultKeyExtractor,
defaultRangeExtractor, elementScroll, observeElementOffset, observeElementRect,
observeWindowOffset, observeWindowRect, Virtualizer, windowScroll"; update the
two import lines (import from 'vitest' and import from '../src/index')
accordingly so lint no longer flags sort-imports.
In `@packages/marko-virtual/tests/tags.test.ts`:
- Around line 40-50: mountFixture currently discards the Marko instance returned
by Template.mount so tests only clear document.body.innerHTML and never run
component onDestroy, leaking store entries (entry.cleanup in
virtualizer/index.marko). Change mountFixture to return the mounted instance
(capture the value from Template.mount) and then in afterEach call
instance.destroy() for any mounted instance(s) (or track and destroy all
returned instances) before clearing document.body.innerHTML so
onDestroy/onCleanup runs and store entries are cleaned up.
---
Nitpick comments:
In `@examples/marko/variable/src/data.ts`:
- Around line 1-2: The module exports rowSizes and colSizes are generated with
Math.random() causing non-deterministic demo drift; replace the random
generation with deterministic logic (e.g., a seeded PRNG or the same formula
used in +page.marko) or expose these exports so +page.marko reads the
authoritative sizes instead of duplicating logic; update rowSizes and colSizes
generation (the Array.from calls) to produce stable values matching the
counts/formulas used by +page.marko (or wire the page to import and use these
exports) so both sources stay in sync.
In `@packages/marko-virtual/src/tags/virtualizer/index.marko`:
- Around line 64-66: The initial mount path calls setInstance(id, { v, cleanup:
v._didMount() }) and then v._willUpdate() but does not synchronize items/size or
call the same notify() that onUpdate uses, which can make first render
non-deterministic; after v._willUpdate() in the mount flow (within the code that
uses setInstance and v._didMount()), invoke the same notify() used by onUpdate
(or otherwise sync items/size immediately) so the first render state is
consistent with update behavior—locate the mount branch that calls setInstance,
v._didMount, and v._willUpdate and add the notify()/items/size synchronization
there.
In `@packages/marko-virtual/tests/index.test.ts`:
- Around line 199-221: The test claims it verifies horizontal sizing but only
calls v._didMount(), so it never wires the observers; update the test to call
v._willUpdate() (or both v._didMount() then v._willUpdate()) to trigger the
observeElementRect callback and ensure horizontal sizing logic runs; then assert
that virtual items were produced based on width (e.g., container width 400 and
estimateSize 100 should yield ~4 items) by using v.getVirtualItems() and
checking its length or item sizes to confirm width was used instead of height.
🪄 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: d1126e20-7a2b-4c15-981b-d9bccbf20c45
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
docs/config.jsondocs/framework/marko/examples/dynamic.mddocs/framework/marko/examples/fixed.mddocs/framework/marko/examples/grid.mddocs/framework/marko/examples/infinite-scroll.mddocs/framework/marko/examples/smooth-scroll.mddocs/framework/marko/examples/variable.mddocs/framework/marko/examples/window.mddocs/framework/marko/marko-virtual.mdexamples/marko/dynamic/marko.jsonexamples/marko/dynamic/package.jsonexamples/marko/dynamic/src/data.tsexamples/marko/dynamic/src/routes/+page.markoexamples/marko/dynamic/vite.config.tsexamples/marko/fixed/package.jsonexamples/marko/fixed/src/routes/+page.markoexamples/marko/fixed/vite.config.tsexamples/marko/grid/marko.jsonexamples/marko/grid/package.jsonexamples/marko/grid/src/data.tsexamples/marko/grid/src/routes/+page.markoexamples/marko/grid/vite.config.tsexamples/marko/infinite-scroll/marko.jsonexamples/marko/infinite-scroll/package.jsonexamples/marko/infinite-scroll/src/routes/+page.markoexamples/marko/infinite-scroll/vite.config.tsexamples/marko/smooth-scroll/marko.jsonexamples/marko/smooth-scroll/package.jsonexamples/marko/smooth-scroll/src/routes/+page.markoexamples/marko/smooth-scroll/vite.config.tsexamples/marko/variable/marko.jsonexamples/marko/variable/package.jsonexamples/marko/variable/src/data.tsexamples/marko/variable/src/routes/+page.markoexamples/marko/variable/vite.config.tsexamples/marko/window/marko.jsonexamples/marko/window/package.jsonexamples/marko/window/src/routes/+page.markoexamples/marko/window/vite.config.tsknip.jsonpackages/marko-virtual/README.mdpackages/marko-virtual/eslint.config.jspackages/marko-virtual/marko.jsonpackages/marko-virtual/package.jsonpackages/marko-virtual/src/index.tspackages/marko-virtual/src/marko.d.tspackages/marko-virtual/src/tags/virtualizer/index.markopackages/marko-virtual/src/tags/virtualizer/store.tspackages/marko-virtual/src/tags/window-virtualizer/index.markopackages/marko-virtual/src/tags/window-virtualizer/store.tspackages/marko-virtual/tests/fixtures/virtualizer-fixture.markopackages/marko-virtual/tests/fixtures/window-virtualizer-fixture.markopackages/marko-virtual/tests/index.test.tspackages/marko-virtual/tests/setup.tspackages/marko-virtual/tests/tags.test.tspackages/marko-virtual/tsconfig.jsonpackages/marko-virtual/vite.config.tspackages/marko-virtual/vitest.config.tspnpm-workspace.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/marko-virtual/tests/tags.test.ts (1)
40-55: ESLint: Use function property syntax for the type annotation.Line 40 uses shorthand method signature
{ destroy(): void }which violates the@typescript-eslint/method-signature-stylerule.🔧 Proposed fix
-const instances: Array<{ destroy(): void }> = [] +const instances: Array<{ destroy: () => void }> = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/marko-virtual/tests/tags.test.ts` around lines 40 - 55, The type annotation for the elements in the instances array uses a method-style signature which triggers the lint rule; change the declaration of instances to use a function property type instead (i.e., use { destroy: () => void }), updating the Array<{ destroy(): void }> annotation so instances, mountFixture, and afterEach remain unaffected except for the type change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/marko-virtual/tests/index.test.ts`:
- Around line 219-223: The test calls v._didMount() but doesn't capture or
invoke its returned cleanup function, causing observer state leaks; update the
test to assign the return value of v._didMount() to a variable (e.g., const
cleanup = v._didMount()) and call cleanup() at the end of the test (after using
v.getVirtualItems()) to ensure observers are disposed; reference the
v._didMount() call and getVirtualItems() usage and ensure cleanup() is invoked
before the test finishes.
---
Nitpick comments:
In `@packages/marko-virtual/tests/tags.test.ts`:
- Around line 40-55: The type annotation for the elements in the instances array
uses a method-style signature which triggers the lint rule; change the
declaration of instances to use a function property type instead (i.e., use {
destroy: () => void }), updating the Array<{ destroy(): void }> annotation so
instances, mountFixture, and afterEach remain unaffected except for the type
change.
🪄 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: f801023a-5375-4307-b320-2eaf0a14e1ba
📒 Files selected for processing (7)
examples/marko/grid/src/data.tspackages/marko-virtual/README.mdpackages/marko-virtual/src/marko.d.tspackages/marko-virtual/src/tags/virtualizer/index.markopackages/marko-virtual/src/tags/window-virtualizer/index.markopackages/marko-virtual/tests/index.test.tspackages/marko-virtual/tests/tags.test.ts
✅ Files skipped from review due to trivial changes (2)
- examples/marko/grid/src/data.ts
- packages/marko-virtual/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/marko-virtual/src/marko.d.ts
- packages/marko-virtual/src/tags/virtualizer/index.marko
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/marko-virtual/tests/index.test.ts (1)
14-24:⚠️ Potential issue | 🟡 MinorFix import member order to satisfy
sort-imports.
Virtualizeris still out of order in this import block, so lint will fail.Proposed fix
import { + Virtualizer, defaultKeyExtractor, defaultRangeExtractor, elementScroll, observeElementOffset, observeElementRect, observeWindowOffset, observeWindowRect, - Virtualizer, windowScroll, } from '../src/index'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/marko-virtual/tests/index.test.ts` around lines 14 - 24, The named import list in packages/marko-virtual/tests/index.test.ts is not alphabetically ordered per the sort-imports rule; move the Virtualizer specifier into the correct alphabetical position among the other named imports (i.e., reorder the named imports so all members including Virtualizer are sorted alphabetically), update the import line that currently lists defaultKeyExtractor, defaultRangeExtractor, elementScroll, observeElementOffset, observeElementRect, observeWindowOffset, observeWindowRect, Virtualizer, windowScroll accordingly, and re-run the linter to confirm the import order is satisfied.
🧹 Nitpick comments (2)
packages/marko-virtual/tests/index.test.ts (1)
8-11: Top-level test-scope comment is outdated.This section says tag-level integration tests are not set up, but they now exist in
packages/marko-virtual/tests/tags.test.ts. Please update/remove this note to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/marko-virtual/tests/index.test.ts` around lines 8 - 11, The top-level comment in packages/marko-virtual/tests/index.test.ts that states "Tag-level integration tests ... are not set up here" is outdated; remove or update that note to reflect that tag-level tests now exist in packages/marko-virtual/tests/tags.test.ts — locate the comment block in index.test.ts and either delete it or change its wording to reference the existing tags.test.ts (and any relevant test utilities) so the file no longer claims those tests are missing.packages/marko-virtual/tests/tags.test.ts (1)
153-173: Test name does not match behavior.The test says “count decrease” but only verifies increments (
5 → 6 → 7). Rename it or actually add a decrease path assertion.Minimal rename option
- test("count decrease removes items from DOM", async () => { + test("count increase adds items to DOM", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/marko-virtual/tests/tags.test.ts` around lines 153 - 173, The test named "count decrease removes items from DOM" only performs increments on the CountUpdate fixture (clicking the element queried by "[data-testid='increment-btn']") and asserts 5→6→7, so either rename the test to reflect increasing behavior (e.g., "count increase updates items in DOM") or add the decrease path: click the decrement control (or dispatch a decrement event via the appropriate testid) and assert counts decrease accordingly; update the test name to match whichever behavior you implement and keep references to CountUpdate, "[data-testid='increment-btn']" and the virtual-item selector so reviewers can locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/marko-virtual/tests/tags.test.ts`:
- Line 42: The literal type for instances uses a method-signature style which
violates `@typescript-eslint/method-signature-style`; update the type for the
instances variable (currently declared as instances: Array<{ destroy(): void }>)
to use a function-property style (e.g., instances: Array<{ destroy: () => void
}>) or introduce a named interface/type with destroy: () => void and use that
type in the declaration; ensure you update the declaration of the instances
variable and any related usages to match the new function-property signature.
---
Duplicate comments:
In `@packages/marko-virtual/tests/index.test.ts`:
- Around line 14-24: The named import list in
packages/marko-virtual/tests/index.test.ts is not alphabetically ordered per the
sort-imports rule; move the Virtualizer specifier into the correct alphabetical
position among the other named imports (i.e., reorder the named imports so all
members including Virtualizer are sorted alphabetically), update the import line
that currently lists defaultKeyExtractor, defaultRangeExtractor, elementScroll,
observeElementOffset, observeElementRect, observeWindowOffset,
observeWindowRect, Virtualizer, windowScroll accordingly, and re-run the linter
to confirm the import order is satisfied.
---
Nitpick comments:
In `@packages/marko-virtual/tests/index.test.ts`:
- Around line 8-11: The top-level comment in
packages/marko-virtual/tests/index.test.ts that states "Tag-level integration
tests ... are not set up here" is outdated; remove or update that note to
reflect that tag-level tests now exist in
packages/marko-virtual/tests/tags.test.ts — locate the comment block in
index.test.ts and either delete it or change its wording to reference the
existing tags.test.ts (and any relevant test utilities) so the file no longer
claims those tests are missing.
In `@packages/marko-virtual/tests/tags.test.ts`:
- Around line 153-173: The test named "count decrease removes items from DOM"
only performs increments on the CountUpdate fixture (clicking the element
queried by "[data-testid='increment-btn']") and asserts 5→6→7, so either rename
the test to reflect increasing behavior (e.g., "count increase updates items in
DOM") or add the decrease path: click the decrement control (or dispatch a
decrement event via the appropriate testid) and assert counts decrease
accordingly; update the test name to match whichever behavior you implement and
keep references to CountUpdate, "[data-testid='increment-btn']" and the
virtual-item selector so reviewers can locate the changes.
🪄 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: 218e22c2-bd4d-4491-bb72-7a9486e9795e
📒 Files selected for processing (5)
packages/marko-virtual/src/tags/virtualizer/index.markopackages/marko-virtual/src/tags/window-virtualizer/index.markopackages/marko-virtual/tests/fixtures/count-update-fixture.markopackages/marko-virtual/tests/index.test.tspackages/marko-virtual/tests/tags.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/marko-virtual/tests/fixtures/count-update-fixture.marko
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/marko-virtual/src/tags/virtualizer/index.marko
| // Test helpers | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const instances: Array<{ destroy(): void }> = [] |
There was a problem hiding this comment.
Use function-property style for the instance type.
This violates @typescript-eslint/method-signature-style.
Proposed fix
-const instances: Array<{ destroy(): void }> = []
+const instances: Array<{ destroy: () => void }> = []📝 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
| const instances: Array<{ destroy(): void }> = [] | |
| const instances: Array<{ destroy: () => void }> = [] |
🧰 Tools
🪛 ESLint
[error] 42-42: Shorthand method signature is forbidden. Use a function property instead.
(@typescript-eslint/method-signature-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/marko-virtual/tests/tags.test.ts` at line 42, The literal type for
instances uses a method-signature style which violates
`@typescript-eslint/method-signature-style`; update the type for the instances
variable (currently declared as instances: Array<{ destroy(): void }>) to use a
function-property style (e.g., instances: Array<{ destroy: () => void }>) or
introduce a named interface/type with destroy: () => void and use that type in
the declaration; ensure you update the declaration of the instances variable and
any related usages to match the new function-property signature.
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
1 participant
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
Adds
@tanstack/marko-virtual— a headless virtualisation adapter for Marko 6 (runtime-tags API).Package (
packages/marko-virtual/):<virtualizer>tag for scrollable container virtualisation<window-virtualizer>tag for full-page window scrollingexport interface Inputin each tagExamples (
examples/marko/):fixed— fixed-size rows, columns, and gridvariable— variable sizes viaestimateSizedynamic— unknown sizes measured viameasureElementgrid— two virtualizers sharing one scroll elementsmooth-scroll—scrollToIndexwith smooth scrollinginfinite-scroll— lazy loading with fixed total countwindow—<window-virtualizer>full-page scrolling✅ Checklist
pnpm run test:pr.🚀 Release Impact
Note
Pre-existing fails
❌ react-virtual:test:types,
❌ vue-virtual:test:types,
❌ svelte-virtual:test:types,
❌ solid-virtual:test:types
❌ react-virtual:test:e2e
❌ root:test:docs
Marko checklist
What's included
Package (
packages/marko-virtual/)<virtualizer>tag — row, column, and grid virtualisation<window-virtualizer>tag — full-page window scrollingintegration tests mounting real
.markofixtures in jsdomExamples (
examples/marko/)fixed— fixed-size rows, columns, and gridvariable— variable sizes via deterministicestimateSizedynamic— unknown sizes measured viameasureElementgrid— two<virtualizer>tags sharing one scroll elementsmooth-scroll—scrollToIndexwith native CSS smooth scrollinginfinite-scroll— lazy page loading with fixed total countwindow—<window-virtualizer>with full-page scrollingUsage
Summary by CodeRabbit
New Features
Examples
Documentation
Tests
Chores