feat: enable modal/dialog highlighting #750
Conversation
WalkthroughAdds caching, dialog/modal-aware element selection, optimized descendant/XPath generation, spatial indexing, and explicit primary/fallback selector computation. Public APIs now return both primary and fallback selectors, and highlighter data includes a fallback. Cleanup clears new caches. Logging and guards added for shadow DOM and frames. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Caller
participant CSG as ClientSelectorGenerator
participant DOM as iframeDocument
participant Cache as Caches
participant HL as Highlighter
UI->>CSG: generateSelector(doc, coords, action)
CSG->>DOM: elementsFromPoint / dialog-aware deepest lookup
CSG->>Cache: check selector/meaningful/path caches
CSG->>DOM: collect meaningful descendants (limits, shadow)
CSG->>CSG: buildOptimizedAbsoluteXPath -> primary, fallback
CSG-->>UI: { primary, fallback }
UI->>CSG: generateDataForHighlighter(coords, doc, isDOMMode)
CSG->>DOM: resolve element(s) (dialog-first if present)
CSG->>CSG: compute selector + fallback
CSG-->>HL: { rect, selector, fallbackSelector, elementInfo }
note right of HL: Highlighter can use fallback if primary fails
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/helpers/clientSelectorGenerator.ts (4)
2613-2616: Bug: overlay-controls filter condition is always truthyOperator precedence makes
!hoveredElement.closest(...) != nullalways true. Use a direct null check.Apply this diff:
- if ( - hoveredElement != null && - !hoveredElement.closest("#overlay-controls") != null - ) { + if ( + hoveredElement != null && + hoveredElement.closest("#overlay-controls") === null + ) {
2837-2927: Attribute quoting in XPath step generationFor attributes like role/type/name/src/aria-label and data-* below, direct interpolation can break XPath on quotes. Use quoteXPathLiteral.
Apply this diff:
- for (const attrName of meaningfulAttrs) { - if (element.hasAttribute(attrName)) { - const value = element.getAttribute(attrName)!.replace(/'/g, "\\'"); - return `${tagName}[@${attrName}='${value}']`; - } - } + for (const attrName of meaningfulAttrs) { + if (element.hasAttribute(attrName)) { + const value = element.getAttribute(attrName)!; + return `${tagName}[@${attrName}=${this.quoteXPathLiteral(value)}]`; + } + } ... - for (const attr of Array.from(element.attributes)) { + for (const attr of Array.from(element.attributes)) { if ( attr.name.startsWith("data-") && attr.name !== "data-testid" && attr.name !== "data-mx-id" && attr.value ) { - return `${tagName}[@${attr.name}='${attr.value}']`; + return `${tagName}[@${attr.name}=${this.quoteXPathLiteral(attr.value)}]`; } }
3602-3623: Choose the correct list container for targetbuildTargetXPath assumes parentElements[0] contains target. Use findListElementForTarget() first.
Apply this diff:
- const parentElements = this.evaluateXPath(listSelector, document); - const containingParent = parentElements[0]; + const parentElements = this.evaluateXPath(listSelector, document); + const containingParent = + this.findListElementForTarget(targetElement, listSelector, document) || + parentElements[0];
4536-4611: Refactor clientSelectorGenerator usage to handle new{ primary, fallback }return shapeThe recent change to
generateSelectorinsrc/helpers/clientSelectorGenerator.tsnow returns an object{ primary: string | null; fallback: string | null }instead of a plain string. All call sites still treat its return value as a single string, which will break at runtime. You must update these consumers to:
- Destructure the result and choose the appropriate selector (e.g.
primary || fallback).- Update any truthiness checks (
if (selector)) to inspectprimary(or both).- Pass the correct string into events or APIs instead of the whole object.
Specifically, in src/components/recorder/DOMBrowserRenderer.tsx around lines 443–450, 473–480 and 631–637:
- const selector = clientSelectorGenerator.generateSelector( - iframeDoc, - { x: iframeX, y: iframeY }, - ActionType.Click - ); + const { primary, fallback } = clientSelectorGenerator.generateSelector( + iframeDoc, + { x: iframeX, y: iframeY }, + ActionType.Click + ); + const selector = primary || fallback;Then update subsequent checks and the payload:
- if (selector && socket) { + if (selector && socket) { socket.emit("dom:click", { - selector, + selector, … });Repeat this pattern for all three call sites in that file.
Additionally, note that there are multiple
ActionTypedefinitions in the repo (client helper enum, React context union, server/types). To avoid confusion and accidental import collisions, consider aliasing the client export:import { ActionType as SelectorActionType, clientSelectorGenerator } from "../../helpers/clientSelectorGenerator";—and update references to
SelectorActionType.Clickaccordingly.
🧹 Nitpick comments (5)
src/helpers/clientSelectorGenerator.ts (5)
512-526: Dialog prioritization is helpful; consider narrowing the scanfindAllDialogElements traverses the entire DOM on each new analysis. It's gated by getList/listSelector, but still heavy on large pages. Consider early-filtering with a targeted query ("dialog,[role='dialog'],.modal,.dialog,.popup,.overlay") and only if elementsFromPoint() intersects a likely overlay. This would avoid scanning “*” repeatedly.
663-697: Include textarea in “fast meaningful” setFast path omits textarea. Add it to avoid missing obvious actionable inputs.
Apply this diff:
- if (["a", "img", "input", "button", "select"].includes(tagName)) { + if (["a", "img", "input", "button", "select", "textarea"].includes(tagName)) { return true; }
1047-1049: Remove noisy console logs or behind a debug flagThese logs will spam the console on every mouse move. Remove or guard via an instance-level debug flag.
Apply this diff:
- console.log("Elements at point:", elementsAtPoint); - console.log("Grouped elements:", this.groupedElements); + // debug: uncomment if needed + // console.debug("Elements at point:", elementsAtPoint, "grouped:", this.groupedElements);
2645-2653: Selector caching looks good; consider making parent limit configurableCache key includes document URL which is enough. The hard-coded limit of 10 parent elements could be part of performanceConfig for tuning.
Also applies to: 2665-2668
3232-3233: Gate console.time/timeEnd behind a debug flagThese timers will clutter consoles in production. Suggest guarding with a debug toggle.
Also applies to: 3270-3271
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/helpers/clientSelectorGenerator.ts(47 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/helpers/clientSelectorGenerator.ts (3)
maxun-core/src/browserSide/scraper.js (9)
parentElement(914-914)pathParts(569-573)current(1010-1010)tagName(450-450)position(1114-1114)fallbackSelector(1117-1120)MAX_DEPTH(955-955)doc(225-225)shadowRoot(992-992)server/src/types/index.ts (2)
Selectors(147-159)Action(278-288)src/context/browserActions.tsx (1)
ActionType(9-9)
🔇 Additional comments (8)
src/helpers/clientSelectorGenerator.ts (8)
112-116: Good caching additions; verified cleanup clears themThe new WeakMaps/Maps are appropriate, and cleanup() resets them. Nice.
184-224: XPath fallback: quote attribute values safely (single quotes break current output)You interpolate data-mx-id into XPath using single quotes. Values containing a single quote (or odd characters) will generate invalid XPath. Move to a robust literal quoting helper and use it here; also consider downgrading console.warn to debug to avoid log spam.
[submit_essential_refactor]
Apply this diff:- if (childMxId) { - return `//${parentTagName}[@data-mx-id='${parentMxId}']//${childTagName}[@data-mx-id='${childMxId}']`; - } else { + const parentPredicate = `@data-mx-id=${this.quoteXPathLiteral(parentMxId)}`; + if (childMxId) { + const childPredicate = `@data-mx-id=${this.quoteXPathLiteral(childMxId)}`; + return `//${parentTagName}[${parentPredicate}]//${childTagName}[${childPredicate}]`; + } else { const pathElements = this.getMandatoryFallbackPath( childElement, parentElement ); if (pathElements.length > 0) { - const parentPath = `//${parentTagName}[@data-mx-id='${parentMxId}']`; + const parentPath = `//${parentTagName}[${parentPredicate}]`; const childPath = pathElements.join("/"); return `${parentPath}/${childPath}`; } }Add these helpers once in the class (outside the touched ranges):
// Safely embed an arbitrary string as an XPath literal private quoteXPathLiteral(value: string): string { if (value.indexOf("'") === -1) return `'${value}'`; const parts = value.split("'"); return `concat(${parts.map(p => `'${p}'`).join(`, "'", `)})`; } // Minimal escaping for CSS attribute values inside double quotes private escapeCssAttrValue(value: string): string { return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); }
616-616: Depth guard is prudentDepth limit in getMeaningfulChildren prevents runaway recursion. Good.
646-658: Meaningfulness caching LGTMWeakMap cache is appropriate and correctly populated.
1807-1810: Good: exclude data-mx-id from general attribute-based CSS generationPrevents primary selectors from leaning on internal IDs reserved for fallbacks.
2703-2787: Descendant BFS with shadow support + caps looks solidNice balance between breadth, depth, and caps; caching meaningfulness avoids redundant work. Good.
4180-4201: Dialog prioritization: smart guard to ensure pointer is inside the dialogThe click-area check prevents selecting backdrop overlays. Nice improvement.
4529-4533: Cleanup extended to new cachesGood attention to memory safety; all caches cleared.
| private getMandatoryFallbackPath( | ||
| targetElement: HTMLElement, | ||
| rootElement: HTMLElement | ||
| ): string[] { | ||
| const pathParts: string[] = []; | ||
| let current = targetElement; | ||
|
|
||
| while (current && current !== rootElement && current.parentElement) { | ||
| const mxId = current.getAttribute("data-mx-id"); | ||
| const tagName = current.tagName.toLowerCase(); | ||
|
|
||
| if (mxId) { | ||
| // Always prefer data-mx-id when available | ||
| pathParts.unshift(`${tagName}[@data-mx-id='${mxId}']`); | ||
| } else { | ||
| // Use position as fallback when data-mx-id is not available | ||
| const position = | ||
| Array.from(current.parentElement.children) | ||
| .filter((child) => child.tagName === current.tagName) | ||
| .indexOf(current) + 1; | ||
| pathParts.unshift(`${tagName}[${position}]`); | ||
| } | ||
|
|
||
| current = current.parentElement; | ||
| } | ||
|
|
||
| return pathParts; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Path builder: same XPath quoting issue as above
When mxId is present, use the quoting helper; current string interpolation will break on single quotes.
Apply this diff:
- if (mxId) {
- // Always prefer data-mx-id when available
- pathParts.unshift(`${tagName}[@data-mx-id='${mxId}']`);
+ if (mxId) {
+ // Always prefer data-mx-id when available
+ pathParts.unshift(`${tagName}[@data-mx-id=${this.quoteXPathLiteral(mxId)}]`);📝 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.
| private getMandatoryFallbackPath( | |
| targetElement: HTMLElement, | |
| rootElement: HTMLElement | |
| ): string[] { | |
| const pathParts: string[] = []; | |
| let current = targetElement; | |
| while (current && current !== rootElement && current.parentElement) { | |
| const mxId = current.getAttribute("data-mx-id"); | |
| const tagName = current.tagName.toLowerCase(); | |
| if (mxId) { | |
| // Always prefer data-mx-id when available | |
| pathParts.unshift(`${tagName}[@data-mx-id='${mxId}']`); | |
| } else { | |
| // Use position as fallback when data-mx-id is not available | |
| const position = | |
| Array.from(current.parentElement.children) | |
| .filter((child) => child.tagName === current.tagName) | |
| .indexOf(current) + 1; | |
| pathParts.unshift(`${tagName}[${position}]`); | |
| } | |
| current = current.parentElement; | |
| } | |
| return pathParts; | |
| } | |
| private getMandatoryFallbackPath( | |
| targetElement: HTMLElement, | |
| rootElement: HTMLElement | |
| ): string[] { | |
| const pathParts: string[] = []; | |
| let current = targetElement; | |
| while (current && current !== rootElement && current.parentElement) { | |
| const mxId = current.getAttribute("data-mx-id"); | |
| const tagName = current.tagName.toLowerCase(); | |
| if (mxId) { | |
| // Always prefer data-mx-id when available | |
| pathParts.unshift(`${tagName}[@data-mx-id=${this.quoteXPathLiteral(mxId)}]`); | |
| } else { | |
| // Use position as fallback when data-mx-id is not available | |
| const position = | |
| Array.from(current.parentElement.children) | |
| .filter((child) => child.tagName === current.tagName) | |
| .indexOf(current) + 1; | |
| pathParts.unshift(`${tagName}[${position}]`); | |
| } | |
| current = current.parentElement; | |
| } | |
| return pathParts; | |
| } |
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 229 to 256, the XPath
fragment built when data-mx-id is present interpolates the raw mxId inside
single quotes which will break if mxId contains single quotes; use the project’s
XPath-quoting helper to escape/wrap the mxId value (e.g., produce a properly
quoted string like "'...'" or concat(...) as the helper returns) and insert that
quoted value into the selector (producing tagName[@data-mx-id=<quotedValue>]);
if the helper is in another module, import it at the top of the file.
| private generateMandatoryGroupFallback( | ||
| elements: HTMLElement[] | ||
| ): string | null { | ||
| try { | ||
| if (!elements || elements.length === 0) return null; | ||
|
|
||
| const tagName = elements[0].tagName.toLowerCase(); | ||
|
|
||
| const mxIds = elements | ||
| .map((el) => el.getAttribute("data-mx-id")) | ||
| .filter(Boolean); | ||
|
|
||
| if (mxIds.length > 0) { | ||
| if (mxIds.length === 1) { | ||
| return `//${tagName}[@data-mx-id='${mxIds[0]}']`; | ||
| } else { | ||
| const mxIdConditions = mxIds | ||
| .map((id) => `@data-mx-id='${id}'`) | ||
| .join(" or "); | ||
| return `//${tagName}[${mxIdConditions}]`; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } catch (error) { | ||
| console.error("Error generating mandatory group fallback:", error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Group fallback XPath: quote each mx-id safely
Same XPath quoting concern; also guard ids typed as strings.
Apply this diff:
- if (mxIds.length > 0) {
+ if (mxIds.length > 0) {
if (mxIds.length === 1) {
- return `//${tagName}[@data-mx-id='${mxIds[0]}']`;
+ return `//${tagName}[@data-mx-id=${this.quoteXPathLiteral(String(mxIds[0]))}]`;
} else {
- const mxIdConditions = mxIds
- .map((id) => `@data-mx-id='${id}'`)
+ const mxIdConditions = mxIds
+ .map((id) => `@data-mx-id=${this.quoteXPathLiteral(String(id))}`)
.join(" or ");
return `//${tagName}[${mxIdConditions}]`;
}
}📝 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.
| private generateMandatoryGroupFallback( | |
| elements: HTMLElement[] | |
| ): string | null { | |
| try { | |
| if (!elements || elements.length === 0) return null; | |
| const tagName = elements[0].tagName.toLowerCase(); | |
| const mxIds = elements | |
| .map((el) => el.getAttribute("data-mx-id")) | |
| .filter(Boolean); | |
| if (mxIds.length > 0) { | |
| if (mxIds.length === 1) { | |
| return `//${tagName}[@data-mx-id='${mxIds[0]}']`; | |
| } else { | |
| const mxIdConditions = mxIds | |
| .map((id) => `@data-mx-id='${id}'`) | |
| .join(" or "); | |
| return `//${tagName}[${mxIdConditions}]`; | |
| } | |
| } | |
| return null; | |
| } catch (error) { | |
| console.error("Error generating mandatory group fallback:", error); | |
| return null; | |
| } | |
| } | |
| private generateMandatoryGroupFallback( | |
| elements: HTMLElement[] | |
| ): string | null { | |
| try { | |
| if (!elements || elements.length === 0) return null; | |
| const tagName = elements[0].tagName.toLowerCase(); | |
| const mxIds = elements | |
| .map((el) => el.getAttribute("data-mx-id")) | |
| .filter(Boolean); | |
| if (mxIds.length > 0) { | |
| if (mxIds.length === 1) { | |
| return `//${tagName}[@data-mx-id=${this.quoteXPathLiteral(String(mxIds[0]))}]`; | |
| } else { | |
| const mxIdConditions = mxIds | |
| .map((id) => `@data-mx-id=${this.quoteXPathLiteral(String(id))}`) | |
| .join(" or "); | |
| return `//${tagName}[${mxIdConditions}]`; | |
| } | |
| } | |
| return null; | |
| } catch (error) { | |
| console.error("Error generating mandatory group fallback:", error); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 261 to 289, the XPath
builder uses raw mx-id values which can break selectors when IDs contain quotes
and doesn't ensure IDs are strings; change the code to coerce each id to a
string (e.g. String(id)) and use a helper that returns a safe XPath literal: if
the id contains no single quotes wrap in single quotes, else if it contains no
double quotes wrap in double quotes, else build an XPath concat(...) expression
to represent the value; use that helper when constructing each @data-mx-id='...'
clause so all mx-id conditions are safely quoted and robust.
| /** | ||
| * Generate CSS fallback selector using data-mx-id attribute | ||
| */ | ||
| private generateMandatoryCSSFallback(element: HTMLElement): string | null { | ||
| try { | ||
| const mxId = element.getAttribute("data-mx-id"); | ||
| if (!mxId) { | ||
| console.warn("Element missing data-mx-id attribute for CSS fallback"); | ||
| return null; | ||
| } | ||
|
|
||
| const tagName = element.tagName.toLowerCase(); | ||
| return `${tagName}[data-mx-id='${mxId}']`; | ||
| } catch (error) { | ||
| console.error("Error generating mandatory CSS fallback:", error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
CSS fallback: escape attribute values properly
data-mx-id values containing double quotes will break the selector. Use the CSS escaping helper with double-quoted attribute values.
Apply this diff:
- private generateMandatoryCSSFallback(element: HTMLElement): string | null {
+ private generateMandatoryCSSFallback(element: HTMLElement): string | null {
try {
const mxId = element.getAttribute("data-mx-id");
if (!mxId) {
console.warn("Element missing data-mx-id attribute for CSS fallback");
return null;
}
const tagName = element.tagName.toLowerCase();
- return `${tagName}[data-mx-id='${mxId}']`;
+ return `${tagName}[data-mx-id="${this.escapeCssAttrValue(mxId)}"]`;
} catch (error) {📝 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.
| /** | |
| * Generate CSS fallback selector using data-mx-id attribute | |
| */ | |
| private generateMandatoryCSSFallback(element: HTMLElement): string | null { | |
| try { | |
| const mxId = element.getAttribute("data-mx-id"); | |
| if (!mxId) { | |
| console.warn("Element missing data-mx-id attribute for CSS fallback"); | |
| return null; | |
| } | |
| const tagName = element.tagName.toLowerCase(); | |
| return `${tagName}[data-mx-id='${mxId}']`; | |
| } catch (error) { | |
| console.error("Error generating mandatory CSS fallback:", error); | |
| return null; | |
| } | |
| } | |
| /** | |
| * Generate CSS fallback selector using data-mx-id attribute | |
| */ | |
| private generateMandatoryCSSFallback(element: HTMLElement): string | null { | |
| try { | |
| const mxId = element.getAttribute("data-mx-id"); | |
| if (!mxId) { | |
| console.warn("Element missing data-mx-id attribute for CSS fallback"); | |
| return null; | |
| } | |
| const tagName = element.tagName.toLowerCase(); | |
| return `${tagName}[data-mx-id="${this.escapeCssAttrValue(mxId)}"]`; | |
| } catch (error) { | |
| console.error("Error generating mandatory CSS fallback:", error); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 1559 to 1576, the current
generateMandatoryCSSFallback builds an attribute selector using raw mxId which
breaks when the value contains quotes; update the method to properly escape the
attribute value and use double-quoted attribute syntax: call the standard
CSS.escape(mxId) (or a local polyfill if CSS.escape is unavailable) and return
`${tagName}[data-mx-id="${CSS.escape(mxId)}"]`, keeping the null checks and
error handling intact.
| if (this.pathCache.has(targetElement)) { | ||
| return this.pathCache.get(targetElement)!; | ||
| } | ||
|
|
There was a problem hiding this comment.
Cache key bug: pathCache ignores rootElement, causing cross-root mis-hits
getOptimizedStructuralPath caches by targetElement only, but the path depends on rootElement and even otherListElements. This can return a wrong path on subsequent calls with a different root. Cache per (target, root).
Apply this diff:
- private pathCache = new WeakMap<HTMLElement, string | null>();
+ private pathCache = new WeakMap<HTMLElement, Map<HTMLElement, string | null>>();- if (this.pathCache.has(targetElement)) {
- return this.pathCache.get(targetElement)!;
- }
+ const rootMap = this.pathCache.get(targetElement);
+ if (rootMap && rootMap.has(rootElement)) {
+ return rootMap.get(rootElement)!;
+ }- if (current !== rootElement) {
- this.pathCache.set(targetElement, null);
+ if (current !== rootElement) {
+ let map = this.pathCache.get(targetElement);
+ if (!map) {
+ map = new Map<HTMLElement, string | null>();
+ this.pathCache.set(targetElement, map);
+ }
+ map.set(rootElement, null);
return null;
}- const result = pathParts.length > 0 ? "/" + pathParts.join("/") : null;
-
- this.pathCache.set(targetElement, result);
-
- return result;
+ const result = pathParts.length > 0 ? "/" + pathParts.join("/") : null;
+ let map = this.pathCache.get(targetElement);
+ if (!map) {
+ map = new Map<HTMLElement, string | null>();
+ this.pathCache.set(targetElement, map);
+ }
+ map.set(rootElement, result);
+ return result;Also update the reset:
- this.pathCache = new WeakMap<HTMLElement, string | null>();
+ this.pathCache = new WeakMap<HTMLElement, Map<HTMLElement, string | null>>();(lines 2651-2653 and in cleanup at 4530-4533)
Also applies to: 3082-3091
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 3027-3030, the pathCache
currently keys only by targetElement causing incorrect cache hits across
different rootElement contexts; change the cache key to include both
targetElement and rootElement (and otherListElements if relevant) when storing
and retrieving paths so entries are unique per (target, root) combination.
Update all related accesses (also at ~3082-3091) to compute and use the
composite key consistently. Additionally update cache reset/cleanup code at
lines ~2651-2653 and ~4530-4533 to clear entries keyed by the composite key or
to rebuild the cache appropriately when a root changes. Ensure the composite key
generation is deterministic and avoids memory leaks (e.g., use a string/id
serialization or a nested Map keyed by root then target).
| if (otherListElements.length === 0) { | ||
| return this.normalizeClasses(targetElement.classList).split(" ").filter(Boolean); | ||
| } | ||
|
|
||
| const targetClasses = this.normalizeClasses(targetElement.classList).split(" ").filter(Boolean); | ||
|
|
||
| // Fast path for elements with no classes | ||
| if (targetClasses.length === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| const otherListsKey = otherListElements.map(el => `${el.tagName}-${el.className}`).sort().join('|'); | ||
| const cacheKey = `${targetElement.tagName}-${targetClasses.sort().join(',')}-${otherListsKey}`; | ||
| // Create optimized cache key | ||
| const cacheKey = `${targetElement.tagName}_${targetClasses.join(',')}_${otherListElements.length}`; | ||
|
|
||
| // Check cache first | ||
| if (this.classCache.has(cacheKey)) { | ||
| return this.classCache.get(cacheKey)!; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
classCache key is lossy; avoid caching when otherListElements is non-empty
The cache key uses only tagName, target classes, and the count of otherListElements. Different sets with the same count can collide, returning wrong classes. Easiest fix: bypass cache entirely when otherListElements.length > 0.
Apply this diff:
- const cacheKey = `${targetElement.tagName}_${targetClasses.join(',')}_${otherListElements.length}`;
+ const cacheKey = `${targetElement.tagName}_${targetClasses.join(',')}_${otherListElements.length}`;
...
- if (this.classCache.has(cacheKey)) {
+ if (otherListElements.length === 0 && this.classCache.has(cacheKey)) {
return this.classCache.get(cacheKey)!;
}
...
- this.classCache.set(cacheKey, targetClasses);
+ if (otherListElements.length === 0) this.classCache.set(cacheKey, targetClasses);
...
- this.classCache.set(cacheKey, targetClasses);
+ if (otherListElements.length === 0) this.classCache.set(cacheKey, targetClasses);
...
- this.classCache.set(cacheKey, commonClasses);
+ if (otherListElements.length === 0) this.classCache.set(cacheKey, commonClasses);Also applies to: 3135-3176
| private generateGroupContainerSelector(group: ElementGroup): { | ||
| primary: string; | ||
| fallback: string | null; | ||
| } { | ||
| const { elements } = group; | ||
|
|
||
| if (!elements || elements.length === 0) return ""; | ||
|
|
||
| // 1. Tag name (ensure all tags match first) | ||
| const tagName = elements[0].tagName.toLowerCase(); | ||
| if (!elements.every((el) => el.tagName.toLowerCase() === tagName)) { | ||
| throw new Error("Inconsistent tag names in group."); | ||
| if (!elements || elements.length === 0) { | ||
| return { primary: "", fallback: null }; | ||
| } | ||
|
|
||
| let xpath = `//${tagName}`; | ||
| const predicates: string[] = []; | ||
| // ALWAYS generate primary XPath | ||
| let primary = ""; | ||
| try { | ||
| const tagName = elements[0].tagName.toLowerCase(); | ||
| if (!elements.every((el) => el.tagName.toLowerCase() === tagName)) { | ||
| throw new Error("Inconsistent tag names in group."); | ||
| } | ||
|
|
||
| // 2. Get common classes | ||
| const commonClasses = this.getCommonStrings( | ||
| elements.map((el) => | ||
| (el.getAttribute("class") || "").split(/\s+/).filter(Boolean) | ||
| ) | ||
| ); | ||
| if (commonClasses.length > 0) { | ||
| predicates.push( | ||
| ...commonClasses.map((cls) => `contains(@class, '${cls}')`) | ||
| let xpath = `//${tagName}`; | ||
| const predicates: string[] = []; | ||
|
|
||
| // Get common classes | ||
| const commonClasses = this.getCommonStrings( | ||
| elements.map((el) => | ||
| (el.getAttribute("class") || "").split(/\s+/).filter(Boolean) | ||
| ) | ||
| ); | ||
| } | ||
| if (commonClasses.length > 0) { | ||
| predicates.push( | ||
| ...commonClasses.map((cls) => `contains(@class, '${cls}')`) | ||
| ); | ||
| } | ||
|
|
||
| // 3. Get common attributes (excluding id, style, dynamic ones) | ||
| const commonAttributes = this.getCommonAttributes(elements, [ | ||
| "id", | ||
| "style", | ||
| "class" | ||
| ]); | ||
| for (const [attr, value] of Object.entries(commonAttributes)) { | ||
| predicates.push(`@${attr}='${value}'`); | ||
| } | ||
| // Get common attributes (excluding id, style, data-mx-id, class) | ||
| const commonAttributes = this.getCommonAttributes(elements, [ | ||
| "id", | ||
| "style", | ||
| "data-mx-id", | ||
| "class", | ||
| ]); | ||
| for (const [attr, value] of Object.entries(commonAttributes)) { | ||
| predicates.push(`@${attr}='${value}'`); | ||
| } | ||
|
|
||
| // 4. Optional: Common child count | ||
| const childrenCountSet = new Set(elements.map((el) => el.children.length)); | ||
| if (childrenCountSet.size === 1) { | ||
| predicates.push(`count(*)=${[...childrenCountSet][0]}`); | ||
| } | ||
| // Optional: Common child count | ||
| const childrenCountSet = new Set( | ||
| elements.map((el) => el.children.length) | ||
| ); | ||
| if (childrenCountSet.size === 1) { | ||
| predicates.push(`count(*)=${[...childrenCountSet][0]}`); | ||
| } | ||
|
|
||
| // 5. Build XPath | ||
| if (predicates.length > 0) { | ||
| xpath += `[${predicates.join(" and ")}]`; | ||
| if (predicates.length > 0) { | ||
| xpath += `[${predicates.join(" and ")}]`; | ||
| } | ||
|
|
||
| primary = xpath; | ||
| } catch (error) { | ||
| console.warn("Primary group XPath generation failed:", error); | ||
| const tagName = elements[0].tagName.toLowerCase(); | ||
| primary = `//${tagName}`; | ||
| } | ||
|
|
||
| return xpath; | ||
| const fallback = this.generateMandatoryGroupFallback(elements); | ||
|
|
||
| return { primary, fallback }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Group selector: quote class and attribute values in XPath
contains(@Class, '...') and @attr='...' both need robust quoting. Use quoteXPathLiteral.
Apply this diff:
- if (commonClasses.length > 0) {
- predicates.push(
- ...commonClasses.map((cls) => `contains(@class, '${cls}')`)
- );
+ if (commonClasses.length > 0) {
+ predicates.push(
+ ...commonClasses.map((cls) => `contains(@class, ${this.quoteXPathLiteral(cls)})`)
+ );
}
...
- for (const [attr, value] of Object.entries(commonAttributes)) {
- predicates.push(`@${attr}='${value}'`);
+ for (const [attr, value] of Object.entries(commonAttributes)) {
+ predicates.push(`@${attr}=${this.quoteXPathLiteral(value)}`);
}📝 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.
| private generateGroupContainerSelector(group: ElementGroup): { | |
| primary: string; | |
| fallback: string | null; | |
| } { | |
| const { elements } = group; | |
| if (!elements || elements.length === 0) return ""; | |
| // 1. Tag name (ensure all tags match first) | |
| const tagName = elements[0].tagName.toLowerCase(); | |
| if (!elements.every((el) => el.tagName.toLowerCase() === tagName)) { | |
| throw new Error("Inconsistent tag names in group."); | |
| if (!elements || elements.length === 0) { | |
| return { primary: "", fallback: null }; | |
| } | |
| let xpath = `//${tagName}`; | |
| const predicates: string[] = []; | |
| // ALWAYS generate primary XPath | |
| let primary = ""; | |
| try { | |
| const tagName = elements[0].tagName.toLowerCase(); | |
| if (!elements.every((el) => el.tagName.toLowerCase() === tagName)) { | |
| throw new Error("Inconsistent tag names in group."); | |
| } | |
| // 2. Get common classes | |
| const commonClasses = this.getCommonStrings( | |
| elements.map((el) => | |
| (el.getAttribute("class") || "").split(/\s+/).filter(Boolean) | |
| ) | |
| ); | |
| if (commonClasses.length > 0) { | |
| predicates.push( | |
| ...commonClasses.map((cls) => `contains(@class, '${cls}')`) | |
| let xpath = `//${tagName}`; | |
| const predicates: string[] = []; | |
| // Get common classes | |
| const commonClasses = this.getCommonStrings( | |
| elements.map((el) => | |
| (el.getAttribute("class") || "").split(/\s+/).filter(Boolean) | |
| ) | |
| ); | |
| } | |
| if (commonClasses.length > 0) { | |
| predicates.push( | |
| ...commonClasses.map((cls) => `contains(@class, '${cls}')`) | |
| ); | |
| } | |
| // 3. Get common attributes (excluding id, style, dynamic ones) | |
| const commonAttributes = this.getCommonAttributes(elements, [ | |
| "id", | |
| "style", | |
| "class" | |
| ]); | |
| for (const [attr, value] of Object.entries(commonAttributes)) { | |
| predicates.push(`@${attr}='${value}'`); | |
| } | |
| // Get common attributes (excluding id, style, data-mx-id, class) | |
| const commonAttributes = this.getCommonAttributes(elements, [ | |
| "id", | |
| "style", | |
| "data-mx-id", | |
| "class", | |
| ]); | |
| for (const [attr, value] of Object.entries(commonAttributes)) { | |
| predicates.push(`@${attr}='${value}'`); | |
| } | |
| // 4. Optional: Common child count | |
| const childrenCountSet = new Set(elements.map((el) => el.children.length)); | |
| if (childrenCountSet.size === 1) { | |
| predicates.push(`count(*)=${[...childrenCountSet][0]}`); | |
| } | |
| // Optional: Common child count | |
| const childrenCountSet = new Set( | |
| elements.map((el) => el.children.length) | |
| ); | |
| if (childrenCountSet.size === 1) { | |
| predicates.push(`count(*)=${[...childrenCountSet][0]}`); | |
| } | |
| // 5. Build XPath | |
| if (predicates.length > 0) { | |
| xpath += `[${predicates.join(" and ")}]`; | |
| if (predicates.length > 0) { | |
| xpath += `[${predicates.join(" and ")}]`; | |
| } | |
| primary = xpath; | |
| } catch (error) { | |
| console.warn("Primary group XPath generation failed:", error); | |
| const tagName = elements[0].tagName.toLowerCase(); | |
| primary = `//${tagName}`; | |
| } | |
| return xpath; | |
| const fallback = this.generateMandatoryGroupFallback(elements); | |
| return { primary, fallback }; | |
| } | |
| private generateGroupContainerSelector(group: ElementGroup): { | |
| primary: string; | |
| fallback: string | null; | |
| } { | |
| const { elements } = group; | |
| if (!elements || elements.length === 0) { | |
| return { primary: "", fallback: null }; | |
| } | |
| // ALWAYS generate primary XPath | |
| let primary = ""; | |
| try { | |
| const tagName = elements[0].tagName.toLowerCase(); | |
| if (!elements.every((el) => el.tagName.toLowerCase() === tagName)) { | |
| throw new Error("Inconsistent tag names in group."); | |
| } | |
| let xpath = `//${tagName}`; | |
| const predicates: string[] = []; | |
| // Get common classes | |
| const commonClasses = this.getCommonStrings( | |
| elements.map((el) => | |
| (el.getAttribute("class") || "").split(/\s+/).filter(Boolean) | |
| ) | |
| ); | |
| if (commonClasses.length > 0) { | |
| predicates.push( | |
| ...commonClasses.map((cls) => | |
| // Use quoteXPathLiteral for robust quoting | |
| `contains(@class, ${this.quoteXPathLiteral(cls)})` | |
| ) | |
| ); | |
| } | |
| // Get common attributes (excluding id, style, data-mx-id, class) | |
| const commonAttributes = this.getCommonAttributes(elements, [ | |
| "id", | |
| "style", | |
| "data-mx-id", | |
| "class", | |
| ]); | |
| for (const [attr, value] of Object.entries(commonAttributes)) { | |
| // Use quoteXPathLiteral here as well | |
| predicates.push(`@${attr}=${this.quoteXPathLiteral(value)}`); | |
| } | |
| // Optional: Common child count | |
| const childrenCountSet = new Set( | |
| elements.map((el) => el.children.length) | |
| ); | |
| if (childrenCountSet.size === 1) { | |
| predicates.push(`count(*)=${[...childrenCountSet][0]}`); | |
| } | |
| if (predicates.length > 0) { | |
| xpath += `[${predicates.join(" and ")}]`; | |
| } | |
| primary = xpath; | |
| } catch (error) { | |
| console.warn("Primary group XPath generation failed:", error); | |
| const tagName = elements[0].tagName.toLowerCase(); | |
| primary = `//${tagName}`; | |
| } | |
| const fallback = this.generateMandatoryGroupFallback(elements); | |
| return { primary, fallback }; | |
| } |
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 4045 to 4111, the XPath
predicates currently interpolate class and attribute values with single quotes
which can break for values containing quotes; update predicates to use
quoteXPathLiteral for safe quoting. Replace contains(@class, '${cls}') with
contains(@class, ${quoteXPathLiteral(cls)}) when mapping commonClasses, and
replace @${attr}='${value}' with @${attr}=${quoteXPathLiteral(value)} when
adding commonAttributes; ensure quoteXPathLiteral is imported/available in this
module and use it in both places so all string literals in XPath predicates are
robustly quoted.
|
@RohitR311 sync cloud changes here |
amhsirak
added
Type: Bug
|
closing as covered in #751 |
1 participant
closes #738
Summary by CodeRabbit
New Features
Refactor
Chores
Note