support iframe and nested iframes. use playwright instead of patchright - #57
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| encodedId = createEncodedId(frameIndex, node.backendDOMNodeId); | ||
| console.log( | ||
| ` ✗ No match for frame ${frameIndex}, using fallback encodedId="${encodedId}"` | ||
| ); |
There was a problem hiding this comment.
Bug: Unconditional Console Log Causes Production Spam
The console.log statement for iframe detection in buildBackendIdMaps runs unconditionally. This can lead to console spam in production environments, as it bypasses the debug parameter.
Additional Locations (1)
|
@claude Use the repository's CLAUDE.md for guidance on style and conventions. Be constructive and helpful in your feedback. |
1 similar comment
|
@claude Use the repository's CLAUDE.md for guidance on style and conventions. Be constructive and helpful in your feedback. |
|
Claude finished @NikhilShahi's task —— View job Pull Request Code Review: iframe Support & Playwright MigrationI've completed a comprehensive review of this PR. Below is my detailed feedback on code quality, potential bugs, performance, and security considerations. Critical Issues1. Out-of-Bounds Frame Index Bug (Confirmed)Location: The existing review correctly identifies a critical bug. The Issue: If multiple iframes have similar structure or the frame collection order changes, the wrong frame could be matched, leading to element targeting failures. Recommendation: Add validation to ensure the matched frame's index corresponds to the expected 2. Unconditional Console Logging (Confirmed)Locations:
Multiple Examples: // build-tree.ts:96
console.log(
`[buildHierarchicalTree] Frame ${frameIndex}: backendNodeId=${node.backendDOMNodeId} not in DOM map...`
);
// build-maps.ts:153
console.log(
`[DOM] Iframe detected: frameIndex=${iframeFrameIndex}...`
);Recommendation: Guard all console.log statements with Major Issues3. Potential Memory Leaks in Frame ResolutionLocation: The Issue: for (const frame of allFrames) {
const iframeElement = await frame.frameElement();
const matches = await parentFrame.evaluate(...); // Evaluates in parent for each frame
}Recommendation: Consider caching frame resolutions or building a frame index once rather than searching on every element lookup. 4. Race Condition in Frame LoadingLocation: The code attempts to wait for iframe content with a timeout, but continues execution if the wait fails: try {
await targetFrame.waitForLoadState("domcontentloaded", { timeout: 5000 });
} catch {
// Continue anyway - frame might already be loaded
}Issue: This could lead to element targeting failures if the frame isn't actually loaded yet. The catch block silently swallows all errors without differentiating between "already loaded" and "failed to load" scenarios. Recommendation: Check the frame's load state before attempting to wait, or add better error handling to distinguish between timeout and actual errors. 5. Missing Error Handling in CDP SessionLocation: The CDP session is created but error handling during accessibility tree fetching is limited: const client = await page.context().newCDPSession(page);
try {
await client.send("Accessibility.enable");
// ... operations
} finally {
await client.detach();
}Issue: If Recommendation: Wrap Code Quality Issues6. Inconsistent Frame Index TypingLocation: Multiple files Frame indices are inconsistently typed as // build-maps.ts:990
const [frameIndexStr] = encodedId.split("-");
const frameIndex = parseInt(frameIndexStr!, 10);
// agent/index.ts:1114
const frameIndex = id.split("-")[0];Issue: The second example doesn't parse to number, keeping it as a string, which could cause bugs in comparisons. Recommendation: Create a utility function to parse encoded IDs consistently and return typed results. 7. Large Configuration Constants at Class LevelLocation: Configuration constants are defined as static readonly, but they're never modified or overridden: private static readonly AIACTION_CONFIG = {
MAX_RETRIES: 10,
RETRY_DELAY_MS: 1000,
// ...
};Issue: This makes configuration inflexible. Users cannot adjust retry behavior without modifying code. Recommendation: Consider moving these to constructor parameters or a separate configuration object that can be passed in. 8. Debug Info Collection Always RunsLocation: Debug-related maps are created conditionally but the logic is verbose: const domNodeCounts = debug ? new Map<number, number>() : null;
const inputElementsByFrame = debug ? new Map<number, number>() : null;Issue: Every check for these maps requires null-checking: Recommendation: Consider a cleaner pattern, such as always creating the maps but only using/logging them when debug is true. Performance Considerations9. Excessive DOM RetriesLocation: The for (let attempt = 0; attempt < maxRetries; attempt++) {
await waitForSettledDOM(page);
domState = await getA11yDOM(page, this.debug);
// ... check if element found
if (attempt < maxRetries - 1) {
await new Promise((resolve) => setTimeout(resolve, retryDelayMs));
}
}Issue: For pages with large DOM trees and many iframes, fetching the full accessibility tree 10 times could add 10+ seconds of latency per action. Recommendation:
10. Synchronous Frame Tree TraversalLocation: The private async getParentFrame(
page: Page,
parentFrameIndex: number,
frameMap: Map<number, IframeInfo>
): Promise<ReturnType<Page["frames"]>[number] | null> {
// Recursively get grandparent frame
const grandparentFrame = await this.getParentFrame(
page,
parentInfo.parentFrameIndex,
frameMap
);
// ...
}Issue: Each recursive call awaits the previous one. For 5 levels of nesting, this is 5 sequential async operations. Recommendation: Build the full frame path iteratively rather than recursively, or cache resolved frames. Security Considerations11. No Validation of Frame Content OriginLocation: The code fetches accessibility trees from all iframes without checking their origin: for (const [frameIndex, frameInfo] of maps.frameMap?.entries() ?? []) {
const result = await client.send("Accessibility.getPartialAXTree", {
backendNodeId: contentDocumentBackendNodeId,
fetchRelatives: true,
});
}Issue: Cross-origin iframes may not allow CDP access, potentially causing errors or exposing security boundaries. Recommendation: Add try-catch for cross-origin access errors and log warnings when frames cannot be accessed due to security restrictions. 12. XPath Injection RiskLocation: XPath strings from the map are used directly in locators: const xpath = rawXpath.replace(/\/text\(\)(\[\d+\])?$/iu, "");
return { locator: page.locator(`xpath=${xpath}`), xpath };Issue: If the xpath map is ever populated from untrusted sources, this could lead to XPath injection. Recommendation: While this appears safe in current usage (xpaths are generated internally), add a comment documenting that xpaths must never come from user input. Best Practices & Suggestions13. Magic Numbers in CodeLocation: Multiple locations Various magic numbers appear without explanation: // build-tree.ts:190
const sampleNodes = debugFrame.totalNodes <= 15 ? debugFrame.rawNodes.slice(0, 15) : undefined;
// agent/index.ts:1047
timeout: 5000 // Why 5 seconds?Recommendation: Extract to named constants with comments explaining the rationale. 14. Incomplete Error MessagesLocation: Error messages could be more helpful: throw new HyperagentError(
`Frame not found for element ${elementId} using XPath: ${iframeInfo.xpath}`,
404
);Recommendation: Include more context in error messages, such as the parent frame index, available frames, and whether this is a same-origin or cross-origin frame. 15. Missing TypeScript Strict ChecksLocation: Multiple files Several places use non-null assertions ( // agent/index.ts:990
const frameIndex = parseInt(frameIndexStr!, 10);
// build-tree.ts:210
path: joinStep(path, segments[i]!),Recommendation: Add runtime checks or refactor to avoid non-null assertions where possible. Positive Aspects
SummaryThis PR adds substantial new functionality for iframe support, but contains several critical bugs that should be fixed before merging: Must Fix:
Should Fix: Nice to Have: Overall, the architecture is sound and the implementation is comprehensive. Once the critical bugs are addressed, this will be a solid addition to the codebase. |
| const page = await agent.newPage(); | ||
| page.ai( | ||
| "Go to https://flights.google.com and find a round-trip flight from Rio de Janeiro to Los Angeles, leaving on November 11, 2025, and returning on November 22, 2025, and select the option with the least carbon dioxide emissions." | ||
| page.goto("https://flights.google.com"); |
There was a problem hiding this comment.
Bug: Async navigation mis-timed due to missing await
Missing await keyword before page.goto(). The goto method is asynchronous and returns a Promise. Without await, the subsequent aiAction calls will execute before the page has finished navigating to the URL, causing the actions to fail or execute on the wrong page. This should be await page.goto("https://flights.google.com");.
| grandparentFrame, | ||
| parentInfo.xpath, | ||
| page.frames() | ||
| ); |
There was a problem hiding this comment.
Bug: Fix OOPIF frame resolution by bypassing XPath lookup
The getParentFrame method doesn't handle OOPIF (Out-of-Process iframe) frames correctly. For OOPIF frames, the xpath field is set to an empty string (as seen in src/context-providers/a11y-dom/index.ts:312), but the method tries to use findFrameByXPath with this empty xpath at line 968-971, which will fail to match any iframe element. The method should check if parentInfo.playwrightFrame exists and return it directly for OOPIF frames, instead of trying to use XPath-based lookup. This will cause frame resolution to fail for elements inside OOPIF frames.
|
|
||
| // Update nextFrameIndex to account for any nested frames we just merged | ||
| // This prevents frameIndex collisions when processing subsequent OOPIF frames | ||
| nextFrameIndex = Math.max(nextFrameIndex, ...Array.from(maps.frameMap?.keys() || [0])) + 1; |
There was a problem hiding this comment.
Bug: Incorrect Fallback Causes Frame Index Skipping
Incorrect fallback value in nextFrameIndex calculation causes frame index skipping. When maps.frameMap is undefined, the expression maps.frameMap?.keys() || [0] evaluates to [0], which incorrectly includes frame index 0 in the Math.max calculation. This can cause frame indices to be skipped. For example, if nextFrameIndex is 2 and maps.frameMap is undefined, the calculation becomes Math.max(2, 0) + 1 = 3, skipping frame index 2. The fallback should be an empty array [] instead of [0], or use nullish coalescing: maps.frameMap?.keys() ?? [].
|
@claude
|
|
Claude finished @Dingway98's task —— View job Pull Request Code Review: iframe Support & Playwright MigrationI've completed a comprehensive review of this PR. Below is my detailed feedback on code quality, potential bugs, performance, and security considerations. SummaryThis PR introduces substantial improvements for iframe support (both same-origin and OOPIF) and migrates from Patchright to Playwright Core. The architecture is sound with frame-aware accessibility tree building, but several critical bugs and performance issues need addressing before merge. Critical Issues1. Missing
|
2 participants
Note
Replaces Patchright with Playwright Core and overhauls a11y DOM to reliably handle same-origin/OOPIF iframes, with improved element resolution, scrolling, and debug data.
patchrighttoplaywright-core.playwright-coredependency.frameMap,accessibleNameMap) from DOM; tag elements byframeIndex.IframeInfoto Playwright Frames for reliable locator resolution; return combinedelements,xpathMap, and optionalframeDebugInfo.aiActionelement finding uses new a11y state; retries; writes enriched debug artifacts (including frame info).frameMapand frame-scoped XPath; improvedscrollTobehavior.debug.waitForSettledDOM, DOM/context providers, and message builders to Playwright types.aicall with stepwiseaiActionflow for Google Flights demo.Written by Cursor Bugbot for commit 80282da. This will update automatically on new commits. Configure here.