fix: extract clean data #822
Merged
+151
−41
Conversation
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
RohitR311
added
Type: Bug
WalkthroughRefactors selector generation in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page
participant SelectorGen as clientSelectorGenerator
participant Shadow as Shadow/Subtree
User->>Page: Click / Point(x,y)
Page->>SelectorGen: getDeepestElementFromPoint(x,y)
SelectorGen->>Page: elementsFromPoint(x,y)
alt deepest is meaningful
SelectorGen-->>Page: return deepestElement
else not meaningful
SelectorGen->>Shadow: findAtomicChildAtPoint(deepest, bbox)
Shadow-->>SelectorGen: atomicMeaningfulDescendant?
alt found
SelectorGen-->>Page: return atomicDescendant
else not found
SelectorGen-->>Page: return original deepest
end
end
sequenceDiagram
autonumber
participant Gen as Selector Builder
participant Target as TargetElement
participant Lists as OtherListElements
participant Utils as Path/Attr Utils
Gen->>Utils: getElementPath(Target)
Gen->>Lists: findCorrespondingElement(root, path)*
note over Gen,Lists: Align competitor elements by DOM path
loop Candidate attributes/classes
Gen->>Utils: isAttributeCommonAcrossLists(attrName, attrValue, Lists)
alt common across lists
Gen-->>Gen: emit attribute predicate
else not common
Gen-->>Gen: skip predicate
end
end
Gen-->>Target: Final selector
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helpers/clientSelectorGenerator.ts (1)
976-993: Invert grouped parent/child filtering to keep leaf-most elements.filterParentChildGroupedElements currently keeps parents that contain other grouped elements. Usually we want the deepest (leaf) grouped elements to avoid duplicate highlights.
Apply this diff:
private filterParentChildGroupedElements( groupedElements: HTMLElement[] ): HTMLElement[] { const result: HTMLElement[] = []; for (const element of groupedElements) { const hasGroupedChild = groupedElements.some( (other) => other !== element && element.contains(other) ); - if (hasGroupedChild) { - result.push(element); - } + if (!hasGroupedChild) { + result.push(element); + } } return result.length > 0 ? result : groupedElements; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/helpers/clientSelectorGenerator.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/helpers/clientSelectorGenerator.ts (1)
maxun-core/src/browserSide/scraper.js (1)
MAX_DEPTH(955-955)
🔇 Additional comments (1)
src/helpers/clientSelectorGenerator.ts (1)
2552-2552: Ensure MAX_DEPTH divergence is intentional
MAX_DEPTH increased from 4→20 in clientSelectorGenerator.ts, while upstream scraper and server selectors cap at 5/4. If this deeper traversal interacts with those components, confirm it won’t degrade performance or cause selector mismatches—consider aligning or documenting the rationale.
Comment on lines
2699
to
2715
| if (!addPositionToAll) { | ||
| const meaningfulAttrs = ["role", "type", "name", "src", "aria-label"]; | ||
| const meaningfulAttrs = ["role", "type"]; | ||
| for (const attrName of meaningfulAttrs) { | ||
| if (element.hasAttribute(attrName)) { | ||
| const value = element.getAttribute(attrName)!.replace(/'/g, "\\'"); | ||
| return `${tagName}[@${attrName}='${value}']`; | ||
| const isCommonAttribute = this.isAttributeCommonAcrossLists( | ||
| element, | ||
| attrName, | ||
| value, | ||
| otherListElements | ||
| ); | ||
| if (isCommonAttribute) { | ||
| return `${tagName}[@${attrName}='${value}']`; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Cross-list attribute check maps by absolute path; compute relative path to the list root and fail closed.
isAttributeCommonAcrossLists builds a path from the document root (getElementPath) then applies it starting from other list roots. This mismaps descendants and treats “corresponding element not found” as implicitly OK, letting non-common attributes pass. Compute a path relative to the current list root and require a match in every other list.
Apply these diffs:
@@
- if (!addPositionToAll) {
- const meaningfulAttrs = ["role", "type"];
+ if (!addPositionToAll) {
+ const meaningfulAttrs = ["role", "type"];
for (const attrName of meaningfulAttrs) {
if (element.hasAttribute(attrName)) {
const value = element.getAttribute(attrName)!.replace(/'/g, "\\'");
- const isCommonAttribute = this.isAttributeCommonAcrossLists(
- element,
- attrName,
- value,
- otherListElements
- );
+ const isCommonAttribute = rootElement
+ ? this.isAttributeCommonAcrossLists(
+ element,
+ rootElement,
+ attrName,
+ value,
+ otherListElements
+ )
+ : false;
if (isCommonAttribute) {
return `${tagName}[@${attrName}='${value}']`;
}
}
}
}
@@
- const testId = element.getAttribute("data-testid");
+ const testId = element.getAttribute("data-testid");
if (testId && !addPositionToAll) {
- const isCommon = this.isAttributeCommonAcrossLists(
- element,
- "data-testid",
- testId,
- otherListElements
- );
+ const isCommon = rootElement
+ ? this.isAttributeCommonAcrossLists(
+ element,
+ rootElement,
+ "data-testid",
+ testId,
+ otherListElements
+ )
+ : false;
if (isCommon) {
return `${tagName}[@data-testid='${testId}']`;
}
}
@@
- if (element.id && !element.id.match(/^\d/) && !addPositionToAll) {
- const isCommon = this.isAttributeCommonAcrossLists(
- element,
- "id",
- element.id,
- otherListElements
- );
+ if (element.id && !element.id.match(/^\d/) && !addPositionToAll) {
+ const isCommon = rootElement
+ ? this.isAttributeCommonAcrossLists(
+ element,
+ rootElement,
+ "id",
+ element.id,
+ otherListElements
+ )
+ : false;
if (isCommon) {
return `${tagName}[@id='${element.id}']`;
}
}
@@
- if (!addPositionToAll) {
+ if (!addPositionToAll) {
for (const attr of Array.from(element.attributes)) {
if (
attr.name.startsWith("data-") &&
attr.name !== "data-testid" &&
attr.name !== "data-mx-id" &&
attr.value
) {
- const isCommon = this.isAttributeCommonAcrossLists(
- element,
- attr.name,
- attr.value,
- otherListElements
- );
+ const isCommon = rootElement
+ ? this.isAttributeCommonAcrossLists(
+ element,
+ rootElement,
+ attr.name,
+ attr.value,
+ otherListElements
+ )
+ : false;
if (isCommon) {
return `${tagName}[@${attr.name}='${attr.value}']`;
}
}
}
}
@@
- private isAttributeCommonAcrossLists(
- targetElement: HTMLElement,
- attrName: string,
- attrValue: string,
- otherListElements: HTMLElement[]
- ): boolean {
- if (otherListElements.length === 0) {
- return true;
- }
-
- const targetPath = this.getElementPath(targetElement);
-
- for (const otherListElement of otherListElements) {
- const correspondingElement = this.findCorrespondingElement(
- otherListElement,
- targetPath
- );
- if (correspondingElement) {
- const otherValue = correspondingElement.getAttribute(attrName);
- if (otherValue !== attrValue) {
- return false;
- }
- }
- }
-
- return true;
- }
+ private isAttributeCommonAcrossLists(
+ targetElement: HTMLElement,
+ rootElement: HTMLElement,
+ attrName: string,
+ attrValue: string,
+ otherListElements: HTMLElement[]
+ ): boolean {
+ if (otherListElements.length === 0) return true;
+ const relPath = this.getElementPath(targetElement, rootElement);
+ if (!relPath) return false; // cannot map reliably relative to root
+ for (const otherRoot of otherListElements) {
+ const corresponding = this.findCorrespondingElement(otherRoot, relPath);
+ if (!corresponding) return false;
+ if (corresponding.getAttribute(attrName) !== attrValue) return false;
+ }
+ return true;
+ }
@@
- private getElementPath(element: HTMLElement): number[] {
- const path: number[] = [];
- let current: HTMLElement | null = element;
-
- while (current && current.parentElement) {
- const siblings = Array.from(current.parentElement.children);
- path.unshift(siblings.indexOf(current));
- current = current.parentElement;
- }
-
- return path;
- }
+ private getElementPath(
+ element: HTMLElement,
+ rootElement: HTMLElement
+ ): number[] | null {
+ const path: number[] = [];
+ let current: HTMLElement | null = element;
+ // Walk up until rootElement (crossing shadow hosts if needed)
+ while (current && current !== rootElement) {
+ const parent =
+ current.parentElement ||
+ ((current.getRootNode() as ShadowRoot).host as HTMLElement | null);
+ if (!parent) return null;
+ const siblings = Array.from(parent.children);
+ const idx = siblings.indexOf(current);
+ if (idx < 0) return null;
+ path.unshift(idx);
+ current = parent;
+ }
+ return current === rootElement ? path : null;
+ }Also applies to: 2717-2728, 2730-2740, 2749-2759, 2926-2953, 2954-2966, 2967-2983
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 2699-2715 (and also apply
same change to ranges 2717-2728, 2730-2740, 2749-2759, 2926-2953, 2954-2966,
2967-2983): the current isAttributeCommonAcrossLists logic builds element paths
from the document root and then queries other list roots using those absolute
paths, which can map to wrong descendants and treats "not found" as success;
change it to compute the element path relative to the current list root (strip
or build the path starting from the list root node) and when checking
otherListElements require that the corresponding relative path resolves to an
element and that its attribute value equals the expected value in every other
list (i.e., treat missing matches as failure), returning false if any list does
not have a matching element or attribute value; update all mentioned code ranges
to use the relative-path lookup and strict matching.
Comment on lines
+3993
to
+3999
| if (!this.isMeaningfulElementCached(deepestElement)) { | ||
| const atomicChild = this.findAtomicChildAtPoint(deepestElement, x, y); | ||
| if (atomicChild) { | ||
| return atomicChild; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Atomic-child fallback misses shadow DOM descendants.
findAtomicChildAtPoint only walks light DOM children. If the meaningful descendant is in a shadow root, the fallback won’t find it.
Apply this diff:
- if (!this.isMeaningfulElementCached(deepestElement)) {
- const atomicChild = this.findAtomicChildAtPoint(deepestElement, x, y);
+ if (!this.isMeaningfulElementCached(deepestElement)) {
+ const atomicChild = this.findAtomicChildAtPoint(deepestElement, x, y);
if (atomicChild) {
return atomicChild;
}
}And enhance traversal:
private findAtomicChildAtPoint(
parent: HTMLElement,
x: number,
y: number
): HTMLElement | null {
const stack: HTMLElement[] = [parent];
const visited = new Set<HTMLElement>();
while (stack.length > 0) {
const element = stack.pop()!;
if (visited.has(element)) continue;
visited.add(element);
if (element !== parent && this.isMeaningfulElementCached(element)) {
const rect = element.getBoundingClientRect();
if (x >= rect.left && x <= rect.right && y >= rect.top && y <= rect.bottom) {
return element;
}
}
- for (let i = element.children.length - 1; i >= 0; i--) {
+ // Light DOM children (iterate from topmost in stacking order approximation)
+ for (let i = element.children.length - 1; i >= 0; i--) {
const child = element.children[i] as HTMLElement;
const rect = child.getBoundingClientRect();
if (x >= rect.left && x <= rect.right && y >= rect.top && y <= rect.bottom) {
stack.push(child);
}
}
+ // Shadow DOM children (if any)
+ if ((element as HTMLElement).shadowRoot) {
+ const shadowChildren = (element as HTMLElement).shadowRoot!.children;
+ for (let i = shadowChildren.length - 1; i >= 0; i--) {
+ const child = shadowChildren[i] as HTMLElement;
+ const rect = child.getBoundingClientRect();
+ if (x >= rect.left && x <= rect.right && y >= rect.top && y <= rect.bottom) {
+ stack.push(child);
+ }
+ }
+ }
}
return null;
}Also applies to: 4003-4034
🤖 Prompt for AI Agents
In src/helpers/clientSelectorGenerator.ts around lines 3993-3999 (and similarly
for 4003-4034), the atomic-child fallback uses findAtomicChildAtPoint which only
traverses light DOM children and therefore misses meaningful descendants inside
shadow roots; update the traversal to also check shadow DOM by: when examining a
candidate element at the point, if it has a shadowRoot call
shadowRoot.elementFromPoint(x,y) (or shadowRoot.deep element lookup) and
continue descending into that shadow root’s composed tree (including following
assigned nodes for slots and checking shadow-inserted children) before falling
back to light DOM children; ensure the function returns the deepest atomic
descendant found in either the shadowRoot or light children and mirror this
shadow-aware logic in the other affected block (4003-4034).
amhsirak
approved these changes
Oct 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What this PR does?
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Refactor