fix: list selector highlighting #544
Conversation
RohitR311
added
Type: Bug
WalkthroughThe changes update the logic for selecting elements at a specific point on a web page within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page
participant SelectorModule
User->>Page: Hover/click at (x, y)
Page->>SelectorModule: Call getElementInformation/getRect/getNonUniqueSelectors(x, y)
SelectorModule->>Page: document.elementsFromPoint(x, y)
Page-->>SelectorModule: List of elements at point
SelectorModule->>SelectorModule: Select first container element (≥30x30px, contains others)
SelectorModule-->>Page: Return selected element info/rect/selector
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 2
🧹 Nitpick comments (1)
server/src/workflow-management/selector.ts (1)
318-330: Potential O(n²) traversal inside the heuristicInside
findContainerElement, the innerelements.someis executed for each
element, yielding O(n²)containschecks. On pages with many overlapped DOM
nodes (e.g. complex SVGs), this can noticeably slow highlight calculation.A micro‑optimisation is to pre‑compute an
Setof ancestors for each element or
short‑circuit once a suitable container is found. A very small refactor:- for (let i = 0; i < elements.length; i++) { - const element = elements[i]; - … - const hasChildrenInList = elements.some((other, j) => i !== j && element.contains(other)); - if (hasChildrenInList) return element; - } + for (const element of elements) { + const rect = element.getBoundingClientRect(); + if (rect.width < 30 || rect.height < 30) continue; + for (const child of elements) { + if (child !== element && element.contains(child)) { + return element; // early‑exit + } + } + }Also applies to: 828-840, 2003-2015
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/workflow-management/selector.ts(3 hunks)
🔇 Additional comments (1)
server/src/workflow-management/selector.ts (1)
322-330: Hard‑coded 30 × 30 threshold – verify it fits all list itemsThe new rule ignores any candidate whose bounding‑box is smaller than
30 px × 30 px. Small interactive targets (icons, checkboxes, compact list
rows, etc.) will now be skipped and may revert to the first element in the
stack, changing previous behaviour.Please confirm (or make the threshold configurable) before release to avoid
regressions.Also applies to: 832-840, 2007-2013
| const findContainerElement = (elements: HTMLElement[]): HTMLElement | null => { | ||
| if (!elements.length) return null; | ||
| if (elements.length === 1) return elements[0]; | ||
|
|
||
| let deepestElement = elements[0]; | ||
| let maxDepth = 0; | ||
|
|
||
| for (const element of elements) { | ||
| let depth = 0; | ||
| let current = element; | ||
| for (let i = 0; i < elements.length; i++) { | ||
| const element = elements[i]; | ||
| const rect = element.getBoundingClientRect(); | ||
|
|
||
| while (current) { | ||
| depth++; | ||
| if (current.parentElement) { | ||
| current = current.parentElement; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (depth > maxDepth) { | ||
| maxDepth = depth; | ||
| deepestElement = element; | ||
| if (rect.width >= 30 && rect.height >= 30) { | ||
| const hasChildrenInList = elements.some((otherElement, j) => | ||
| i !== j && element.contains(otherElement) | ||
| ); | ||
|
|
||
| if (hasChildrenInList) { | ||
| return element; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return deepestElement; | ||
| return elements[0]; | ||
| }; | ||
|
|
||
| // Logic to get list container element | ||
| let targetElement = null; | ||
|
|
||
| for (const element of elements) { | ||
| const deepestEl = findDeepestElement(elements); | ||
|
|
||
| if (deepestEl && element !== deepestEl) { | ||
| if (element.contains(deepestEl) && | ||
| element !== deepestEl.parentElement && | ||
| element.tagName !== 'HTML' && | ||
| element.tagName !== 'BODY') { | ||
| targetElement = element; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let deepestElement = targetElement || findDeepestElement(elements); | ||
| let deepestElement = findContainerElement(elements); | ||
| if (!deepestElement) return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate “container element” heuristic – consider extracting to a helper
The identical findContainerElement implementation now appears three times
(getElementInformation, getRect, getNonUniqueSelectors). Besides the
maintenance burden, any future tweak (e.g. size threshold, containment rule)
must be triplicated and risks drifting out of sync.
Suggestion:
-
Define the helper once in TypeScript:
function findContainerElement(elements: HTMLElement[]): HTMLElement { // …body… }
-
Pass it into each
page.evaluatevia
page.addInitScript(findContainerElement.toString())or by serialising the
function in theevaluatecall (evaluate(findContainerElement, …)).
This keeps the heuristic single‑sourced and testable.
Also applies to: 824-847, 1999-2022
| page.on('console', msg => { | ||
| console.log(`Browser console: ${msg.text()}`); | ||
| }); |
There was a problem hiding this comment.
Avoid adding a new page.on('console') listener on every call
page.on('console', …) is executed every time getElementInformation enters this
branch. After just a few invocations the page will have dozens of identical
listeners, causing:
- memory‑leak‐like growth,
- duplicated logs,
- slower teardown.
- page.on('console', msg => {
- console.log(`Browser console: ${msg.text()}`);
- });
+ if (page.listenerCount('console') === 0) {
+ page.on('console', msg => console.log(`Browser console: ${msg.text()}`));
+ }(You could also register the listener once at module‑initialisation time instead.)
📝 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.
| page.on('console', msg => { | |
| console.log(`Browser console: ${msg.text()}`); | |
| }); | |
| if (page.listenerCount('console') === 0) { | |
| page.on('console', msg => console.log(`Browser console: ${msg.text()}`)); | |
| } |
What this PR does?
Improved list selector highlighting based on element dimensions and children elements.
Fixes: #543
Summary by CodeRabbit
Refactor
New Features