perf(zag-component): implement element caching to reduce DOM traversal costs#20
perf(zag-component): implement element caching to reduce DOM traversal costs#20dgknbtl wants to merge 6 commits intodashersw:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughZagComponent adds an Changes
Sequence Diagram(s)sequenceDiagram
participant Component as ZagComponent
participant Cache as _elementCache
participant DOM as Document/querySelectorAll
participant Slider as Slider (subclass)
rect rgba(100,150,240,0.5)
Component->>Cache: lookup selector -> elements?
end
alt cache hit
Cache-->>Component: return elements
else cache miss
Component->>DOM: _queryAllIncludingSelf(selector)
DOM-->>Component: NodeList -> elements
Component->>Cache: store elements
end
Slider->>Component: _syncThumbs() then clear cache
Component->>Component: _applyAllSpreads() (use cache or query)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 2
🧹 Nitpick comments (1)
packages/gea-ui/tests/zag-element-caching.test.ts (1)
43-47: Consider robustness of regex-based code transformation.The regex replacements for stripping imports and modifying class inheritance are pragmatic but can be fragile if the source code format changes (e.g., multiline imports, different spacing). For now this works, but consider using a more structured AST-based approach or explicit test exports if this pattern expands to more tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/tests/zag-element-caching.test.ts` around lines 43 - 47, The current fragile regex-based transformations applied to the transpileTs output (the chain on the js variable that removes imports, replaces "extends Component" with "extends MockComponent", and strips export declarations) should be replaced with a structured AST transform: parse the transpiled JS using a parser (TypeScript compiler API or Babel/parser), remove ImportDeclaration nodes, locate ClassDeclaration nodes and change their superClass identifier from "Component" to "MockComponent", and remove ExportNamed/DefaultDeclaration wrappers before printing the code; update the code that currently does the .replace(...) chain to perform these AST operations on the transpileTs(src) result so the transformations are robust to whitespace, multiline imports, and other formatting variations.
🤖 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/gea-ui/src/primitives/zag-component.ts`:
- Around line 117-122: The element cache can become stale during state-machine
transitions because _scheduleSpreadApplication() queues _applyAllSpreads() via
queueMicrotask without clearing _elementCache, so update the logic to invalidate
the cache before applying spreads: either clear this._elementCache at the start
of _applyAllSpreads() (so newly-created elements from Slider._syncThumbs() are
discovered) or clear this._elementCache inside _scheduleSpreadApplication()
immediately before queueing the microtask; ensure the fix references
_elementCache, _applyAllSpreads(), _scheduleSpreadApplication(), and that
super._applyAllSpreads() still runs after the cache is cleared.
In `@packages/gea-ui/tests/zag-element-caching.test.ts`:
- Around line 104-106: Uncomment the final assertion so the test verifies
dispose() clears the internal cache: re-enable the assertion checking
instance._elementCache.size is 0 after calling instance.dispose(), ensuring it
validates the explicit _elementCache.clear() behavior on the instance under
test.
---
Nitpick comments:
In `@packages/gea-ui/tests/zag-element-caching.test.ts`:
- Around line 43-47: The current fragile regex-based transformations applied to
the transpileTs output (the chain on the js variable that removes imports,
replaces "extends Component" with "extends MockComponent", and strips export
declarations) should be replaced with a structured AST transform: parse the
transpiled JS using a parser (TypeScript compiler API or Babel/parser), remove
ImportDeclaration nodes, locate ClassDeclaration nodes and change their
superClass identifier from "Component" to "MockComponent", and remove
ExportNamed/DefaultDeclaration wrappers before printing the code; update the
code that currently does the .replace(...) chain to perform these AST operations
on the transpileTs(src) result so the transformations are robust to whitespace,
multiline imports, and other formatting variations.
🪄 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: 668a00ba-7421-4ea8-82fe-052c1575e39b
📒 Files selected for processing (2)
packages/gea-ui/src/primitives/zag-component.tspackages/gea-ui/tests/zag-element-caching.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gea-ui/src/primitives/zag-component.ts (1)
149-156: Minor redundancy in cache clearing (optional cleanup).The
_elementCache.clear()at line 151 is immediately followed by_scheduleSpreadApplication()which also clears the cache at line 87. This is harmless but slightly redundant.That said, keeping it explicit here is defensively safe if
_scheduleSpreadApplication()behavior changes in the future, and the cost is negligible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/src/primitives/zag-component.ts` around lines 149 - 156, The explicit cache clear in __geaSyncMap is redundant because _scheduleSpreadApplication already clears _elementCache; remove the this._elementCache.clear() call from the __geaSyncMap method to avoid duplication, leaving the subsequent _scheduleSpreadApplication() call intact so existing cache-clearing logic in that function handles it (adjust tests if any rely on the immediate clear).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/gea-ui/src/primitives/zag-component.ts`:
- Around line 149-156: The explicit cache clear in __geaSyncMap is redundant
because _scheduleSpreadApplication already clears _elementCache; remove the
this._elementCache.clear() call from the __geaSyncMap method to avoid
duplication, leaving the subsequent _scheduleSpreadApplication() call intact so
existing cache-clearing logic in that function handles it (adjust tests if any
rely on the immediate clear).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6940652e-7204-471d-b670-4f80358ae538
📒 Files selected for processing (2)
packages/gea-ui/src/primitives/zag-component.tspackages/gea-ui/tests/zag-element-caching.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gea-ui/tests/zag-element-caching.test.ts
There was a problem hiding this comment.
The coalescing guard in _scheduleSpreadApplication already ensures that rapid state transitions (hovers, focus, etc.) produce a single _applyAllSpreads call. Since the cache is cleared at every entry point (_scheduleSpreadApplication, onAfterRender, __geaSyncMap), it gets populated during _applyAllSpreads but is always cleared before the next invocation — so it never serves a cache hit in normal operation.
Could you clarify what call pattern you're seeing where the cache actually provides a hit? The test demonstrates hits by calling _applyAllSpreads() directly twice, but I can't find a production code path that does that.
|
In the previous implementation, clearing the cache inside I've optimized this by removing the clear() call from I've also: Verified Persistent Hits: Updated Tests: Removed the stray Turkish comments and added assertions for |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/gea-ui/src/primitives/zag-component.ts (1)
118-122:⚠️ Potential issue | 🟠 MajorStale selector cache can skip spreads after DOM-mutating overrides.
When a subclass mutates DOM inside
_applyAllSpreads()before callingsuper._applyAllSpreads()(seepackages/gea-ui/src/components/slider.tsx), cached selector results can be stale and newly added elements may not receivespreadProps.Suggested fix (invalidate before scheduled apply)
_scheduleSpreadApplication() { if (this._spreadScheduled) return + this._elementCache.clear() this._spreadScheduled = true queueMicrotask(() => { this._spreadScheduled = false this._applyAllSpreads() }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gea-ui/src/primitives/zag-component.ts` around lines 118 - 122, The selector cache in _elementCache can return stale NodeLists after subclasses mutate the DOM in _applyAllSpreads; update the logic in the getter that currently does "let elements = this._elementCache.get(selector) ... _queryAllIncludingSelf(selector)" to ensure cache is invalidated before using it (e.g., call this._elementCache.delete(selector) or clear the whole cache at the start of _applyAllSpreads) so newly added elements are retrieved by _queryAllIncludingSelf and receive spreadProps; target the _elementCache accesses in the methods _applyAllSpreads and the query routine that calls _queryAllIncludingSelf to implement the invalidation.
🤖 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/gea-ui/src/primitives/zag-component.ts`:
- Around line 118-122: The selector cache in _elementCache can return stale
NodeLists after subclasses mutate the DOM in _applyAllSpreads; update the logic
in the getter that currently does "let elements =
this._elementCache.get(selector) ... _queryAllIncludingSelf(selector)" to ensure
cache is invalidated before using it (e.g., call
this._elementCache.delete(selector) or clear the whole cache at the start of
_applyAllSpreads) so newly added elements are retrieved by
_queryAllIncludingSelf and receive spreadProps; target the _elementCache
accesses in the methods _applyAllSpreads and the query routine that calls
_queryAllIncludingSelf to implement the invalidation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0a41daa-8216-4064-9044-f354793143db
📒 Files selected for processing (2)
packages/gea-ui/src/primitives/zag-component.tspackages/gea-ui/tests/zag-element-caching.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gea-ui/tests/zag-element-caching.test.ts
|
I took a closer look at the codebase and verified that Slider is currently the only component mutating the DOM inside If we invalidate the selector cache at the I’d suggest keeping the current caching strategy as-is and handling this locally in Slider instead. For example:
This keeps the hot path fast while still addressing the edge case in a targeted way. |
… and update spreadProps mock.
dashersw
force-pushed
the
perf/zag-component-caching
branch
from
68d326e to
771bb5b
Compare
Yes please, can you change the PR accordingly? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/gea-ui/src/primitives/zag-component.ts`:
- Around line 117-122: The Slider class's _applyAllSpreads() calls _syncThumbs()
which mutates the DOM (adding/removing thumbs) but doesn't clear cached query
results, so cached selectors return stale elements; fix by invoking
this._elementCache.clear() immediately before calling super._applyAllSpreads()
in Slider._applyAllSpreads() so subsequent calls (including
super._applyAllSpreads()) re-query the DOM and discover newly created thumb
elements.
🪄 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: ac2f89be-b793-480a-b353-6364bb8e6140
📒 Files selected for processing (2)
packages/gea-ui/src/primitives/zag-component.tspackages/gea-ui/tests/zag-element-caching.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/gea-ui/tests/zag-element-caching.test.ts
Done! I've updated the PR to handle the cache invalidation locally in |
2 participants
Optimize ZagComponent with Element Caching
Summary
This PR introduces a DOM element caching mechanism to
packages/gea-ui/src/primitives/zag-component.tsto significantly reduce redundant DOM queries during Zag state machine transitions. By memoizing the results of selector-based queries, we minimize performance overhead.Problem
The
_applyAllSpreadsmethod is triggered on every state machine transition (hover, focus, input, etc.). Each trigger executes multiplethis.$$(querySelectorAll) calls. In complex components likeDatePickerorCombobox, this leads to repetitive and costly DOM traversals, impacting UI responsiveness.Solution
_elementCacheMap to store DOM query results._applyAllSpreadsnow checks the cache before performing a fresh query.__geaSyncMap(dynamic lists).onAfterRender(full render cycle consistency).disposemethod._queryAllIncludingSelfto correctly include the root element._cacheArrayContainersfor collection-based components.Performance Impact
DatePickerorSelectnow feel significantly smoother because we read from memory instead of the DOM.Verification Plan
A new unit test file has been added:
packages/gea-ui/tests/zag-element-caching.test.ts.It verifies three key scenarios:
__geaSyncMaporonAfterRenderis called.Execution & Results:
Test command executed successfully with the following results: