prime lazy loaded accessibility tree#277
Conversation
WebView accessibility tree populates asynchronously on first access. First call primes the tree, second call returns complete cached data.
WalkthroughThe change updates 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/android.ts (1)
352-353: Avoid parsing XML in the priming call.Line [353] primes via
getUiAutomatorXml()but discards the parsed object. This adds unnecessary parse/allocation work on every call. Prime with dump only, then parse once for the returned tree.Proposed diff
public async getElementsOnScreen(): Promise<ScreenElement[]> { // Prime lazy loaded accessibility tree - await this.getUiAutomatorXml(); + await this.getUiAutomatorDump(); const parsedXml = await this.getUiAutomatorXml(); const hierarchy = parsedXml.hierarchy; const elements = this.collectElements(hierarchy.node); return elements; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/android.ts` around lines 352 - 353, The current priming call uses getUiAutomatorXml() which parses and allocates the XML even though its parsed result is discarded; change the priming step to use a dump-only path (e.g., call a raw dump method such as getUiAutomatorDump() or change getUiAutomatorXml to accept a flag like skipParse/parse=false) so the code only retrieves the XML string without parsing, and then parse the XML exactly once when you actually need the accessibility tree (the existing parse logic can remain in the code that consumes the returned tree).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/android.ts`:
- Around line 352-353: The current priming call uses getUiAutomatorXml() which
parses and allocates the XML even though its parsed result is discarded; change
the priming step to use a dump-only path (e.g., call a raw dump method such as
getUiAutomatorDump() or change getUiAutomatorXml to accept a flag like
skipParse/parse=false) so the code only retrieves the XML string without
parsing, and then parse the XML exactly once when you actually need the
accessibility tree (the existing parse logic can remain in the code that
consumes the returned tree).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/android.ts
Optimize the fix by using getUiAutomatorDump() (raw XML) for priming instead of getUiAutomatorXml() which parses the XML. The XML is now parsed exactly once when needed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/android.ts (1)
351-359: Consider expanding the comment to explain the WebView-specific behavior.The current comment mentions "Prime lazy loaded accessibility tree" but doesn't explain why two calls are needed. Future maintainers may not understand the double-call pattern or might "optimize" it away.
📝 Suggested comment improvement
public async getElementsOnScreen(): Promise<ScreenElement[]> { - // Prime lazy loaded accessibility tree (raw dump, no parsing) + // WebView accessibility tree populates asynchronously on first access, + // returning incomplete data. A second call consistently returns the + // complete cached tree, so we prime here before the actual fetch. await this.getUiAutomatorDump(); const parsedXml = await this.getUiAutomatorXml();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/android.ts` around lines 351 - 359, The comment for getElementsOnScreen should explain the WebView-specific double-call pattern: the first call to getUiAutomatorDump() is intentional to prime lazy-loaded accessibility trees (notably WebViews that defer accessibility node population until a raw dump is requested), and the subsequent call to getUiAutomatorXml() reads the now-populated parsed XML; update the comment above getElementsOnScreen to explicitly state this reason and that both getUiAutomatorDump() and getUiAutomatorXml() are required and must not be removed or merged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/android.ts`:
- Around line 351-359: The comment for getElementsOnScreen should explain the
WebView-specific double-call pattern: the first call to getUiAutomatorDump() is
intentional to prime lazy-loaded accessibility trees (notably WebViews that
defer accessibility node population until a raw dump is requested), and the
subsequent call to getUiAutomatorXml() reads the now-populated parsed XML;
update the comment above getElementsOnScreen to explicitly state this reason and
that both getUiAutomatorDump() and getUiAutomatorXml() are required and must not
be removed or merged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/android.ts
|
Holy wow! I was not aware of this! Let me try this out to reproduce. Alternatively, we can use cdp to get the Chrome that's on screen. That will also allow you do to other things, like list DOM that is not visible on screen and directly click on such elements (or inject js). Thanks for this PR, I'll take a look later to reproduce this. |
|
@gmegidish awesome, looking forward to hearing how it goes! |
|
@gmegidish did you manage to have a look at this, I'm running this locally and have had no issues since implementing this. |
WebView accessibility tree populates asynchronously on first access. The second call always seemed to work so after some research it seems this is the case. This is the best solution I could come up with.
First call primes the tree, second call returns complete cached data.