feat: better selection #396
Conversation
WalkthroughThe pull request introduces improvements to selector generation in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 1
🧹 Nitpick comments (2)
server/src/workflow-management/selector.ts (2)
1657-1681: Consider caching selector results for performance optimization.The new sibling comparison logic requires additional DOM traversal and string operations for each element. For large DOMs with many identical siblings, this could impact performance.
Consider implementing a simple cache to store computed selectors:
+ const selectorCache = new WeakMap<HTMLElement, string>(); function getNonUniqueSelector(element: HTMLElement): string { + const cachedSelector = selectorCache.get(element); + if (cachedSelector) return cachedSelector; let selector = element.tagName.toLowerCase(); // ... existing logic ... + selectorCache.set(element, selector); return selector; }
1663-1665: Improve robustness of className type checking.The current implementation handles non-string className values, but the type checking could be more explicit and comprehensive.
Consider enhancing the type checking:
- const siblingClassName = typeof sibling.className === 'string' ? sibling.className : ''; + const siblingClassName = (sibling.className?.toString() || '').trim();This change:
- Handles
undefinedclassName values safely- Trims whitespace to avoid empty class names
- Uses toString() to handle SVGAnimatedString class names
Also applies to: 1928-1930, 2084-2086
| if (element.parentElement) { | ||
| // Look for identical siblings | ||
| const siblings = Array.from(element.parentElement.children); | ||
| const identicalSiblings = siblings.filter(sibling => { | ||
| if (sibling === element) return false; | ||
|
|
||
| let siblingSelector = sibling.tagName.toLowerCase(); | ||
| const siblingClassName = typeof sibling.className === 'string' ? sibling.className : ''; | ||
| if (siblingClassName) { | ||
| const siblingClasses = siblingClassName.split(/\s+/).filter(Boolean); | ||
| const validSiblingClasses = siblingClasses.filter(cls => !cls.startsWith('!') && !cls.includes(':')); | ||
| if (validSiblingClasses.length > 0) { | ||
| siblingSelector += '.' + validSiblingClasses.map(cls => CSS.escape(cls)).join('.'); | ||
| } | ||
| } | ||
|
|
||
| return siblingSelector === selector; | ||
| }); | ||
|
|
||
| if (identicalSiblings.length > 0) { | ||
| const position = siblings.indexOf(element) + 1; | ||
| selector += `:nth-child(${position})`; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated selector generation logic into a shared function.
The identical sibling handling logic is duplicated in three locations. This violates the DRY principle and makes maintenance more difficult.
Consider extracting the common logic into a shared utility function:
+ function appendNthChildSelectorIfNeeded(element: HTMLElement, selector: string): string {
+ if (element.parentElement) {
+ const siblings = Array.from(element.parentElement.children);
+ const identicalSiblings = siblings.filter(sibling => {
+ if (sibling === element) return false;
+
+ let siblingSelector = sibling.tagName.toLowerCase();
+ const siblingClassName = typeof sibling.className === 'string' ? sibling.className : '';
+ if (siblingClassName) {
+ const siblingClasses = siblingClassName.split(/\s+/).filter(Boolean);
+ const validSiblingClasses = siblingClasses.filter(cls => !cls.startsWith('!') && !cls.includes(':'));
+ if (validSiblingClasses.length > 0) {
+ siblingSelector += '.' + validSiblingClasses.map(cls => CSS.escape(cls)).join('.');
+ }
+ }
+
+ return siblingSelector === selector;
+ });
+
+ if (identicalSiblings.length > 0) {
+ const position = siblings.indexOf(element) + 1;
+ selector += `:nth-child(${position})`;
+ }
+ }
+ return selector;
+ }
function getNonUniqueSelector(element: HTMLElement): string {
let selector = element.tagName.toLowerCase();
// ... existing class handling logic ...
- if (element.parentElement) {
- // ... duplicate sibling handling logic ...
- }
+ selector = appendNthChildSelectorIfNeeded(element, selector);
return selector;
}Also applies to: 1922-1946, 2078-2102
amhsirak
changed the title
amhsirak
added
Type: Enhancement
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/browser/BrowserWindow.tsx (1)
Line range hint
266-294: Consider extracting selector cleaning logic into a utility function.The selector cleaning logic is duplicated and could be reused in other components. Consider extracting it into a utility function for better maintainability and reusability.
Create a new utility function:
// utils/selectorUtils.ts export const cleanNthChildSelector = (selector: string): string => { return selector.replace(/:nth-child\(\d+\)/g, ''); };Then use it in the component:
- let cleanedSelector = highlighterData.selector; - if (cleanedSelector.includes('nth-child')) { - cleanedSelector = cleanedSelector.replace(/:nth-child\(\d+\)/g, ''); - } + const cleanedSelector = cleanNthChildSelector(highlighterData.selector);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/browser/BrowserWindow.tsx(2 hunks)
🔇 Additional comments (2)
src/components/browser/BrowserWindow.tsx (2)
301-301: LGTM!The use of the cleaned
currentSelectorin the TextStep object is correct and aligns with the PR's objective to improve selector generation.
Line range hint
266-294: Add unit tests for selector cleaning logic.The new selector cleaning functionality lacks unit tests. This is critical for ensuring the reliability of the selector generation improvements.
Would you like me to help create unit tests for the selector cleaning logic? The tests should cover:
- Basic selector cleaning
- Complex selectors with multiple nth-child
- Child selector handling
- Edge cases (empty selectors, no nth-child, etc.)
| let cleanedSelector = highlighterData.selector; | ||
| if (cleanedSelector.includes('nth-child')) { | ||
| cleanedSelector = cleanedSelector.replace(/:nth-child\(\d+\)/g, ''); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix unused cleaned selector.
The cleaned selector is computed but never used. The original selector with nth-child is still being set as the listSelector, which defeats the purpose of cleaning the selector.
Apply this diff to fix the issue:
let cleanedSelector = highlighterData.selector;
if (cleanedSelector.includes('nth-child')) {
cleanedSelector = cleanedSelector.replace(/:nth-child\(\d+\)/g, '');
}
-
- setListSelector(highlighterData.selector);
+ setListSelector(cleanedSelector);Committable suggestion skipped: line range outside the PR's diff.
| let currentSelector = highlighterData.selector; | ||
|
|
||
| if (currentSelector.includes('>')) { | ||
| const [firstPart, ...restParts] = currentSelector.split('>').map(p => p.trim()); | ||
| const listSelectorRightPart = listSelector.split('>').pop()?.trim().replace(/:nth-child\(\d+\)/g, ''); | ||
|
|
||
| if (firstPart.includes('nth-child') && | ||
| firstPart.replace(/:nth-child\(\d+\)/g, '') === listSelectorRightPart) { | ||
| currentSelector = `${firstPart.replace(/:nth-child\(\d+\)/g, '')} > ${restParts.join(' > ')}`; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness and readability of child selector handling.
The current implementation has potential issues:
- Missing null checks for split operations
- Assumes listSelector always contains '>'
- Complex logic could benefit from better readability
Apply this diff to improve the code:
let currentSelector = highlighterData.selector;
if (currentSelector.includes('>')) {
const [firstPart, ...restParts] = currentSelector.split('>').map(p => p.trim());
- const listSelectorRightPart = listSelector.split('>').pop()?.trim().replace(/:nth-child\(\d+\)/g, '');
+ // Extract the rightmost part of the list selector (if it contains '>')
+ const listSelectorParts = listSelector.split('>');
+ const listSelectorRightPart = listSelectorParts.length > 1
+ ? listSelectorParts.pop()?.trim().replace(/:nth-child\(\d+\)/g, '')
+ : listSelector.replace(/:nth-child\(\d+\)/g, '');
+ // Only clean nth-child if the base selectors match
if (firstPart.includes('nth-child') &&
- firstPart.replace(/:nth-child\(\d+\)/g, '') === listSelectorRightPart) {
+ listSelectorRightPart &&
+ firstPart.replace(/:nth-child\(\d+\)/g, '') === listSelectorRightPart) {
currentSelector = `${firstPart.replace(/:nth-child\(\d+\)/g, '')} > ${restParts.join(' > ')}`;
}
}📝 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.
| let currentSelector = highlighterData.selector; | |
| if (currentSelector.includes('>')) { | |
| const [firstPart, ...restParts] = currentSelector.split('>').map(p => p.trim()); | |
| const listSelectorRightPart = listSelector.split('>').pop()?.trim().replace(/:nth-child\(\d+\)/g, ''); | |
| if (firstPart.includes('nth-child') && | |
| firstPart.replace(/:nth-child\(\d+\)/g, '') === listSelectorRightPart) { | |
| currentSelector = `${firstPart.replace(/:nth-child\(\d+\)/g, '')} > ${restParts.join(' > ')}`; | |
| } | |
| } | |
| let currentSelector = highlighterData.selector; | |
| if (currentSelector.includes('>')) { | |
| const [firstPart, ...restParts] = currentSelector.split('>').map(p => p.trim()); | |
| // Extract the rightmost part of the list selector (if it contains '>') | |
| const listSelectorParts = listSelector.split('>'); | |
| const listSelectorRightPart = listSelectorParts.length > 1 | |
| ? listSelectorParts.pop()?.trim().replace(/:nth-child\(\d+\)/g, '') | |
| : listSelector.replace(/:nth-child\(\d+\)/g, ''); | |
| // Only clean nth-child if the base selectors match | |
| if (firstPart.includes('nth-child') && | |
| listSelectorRightPart && | |
| firstPart.replace(/:nth-child\(\d+\)/g, '') === listSelectorRightPart) { | |
| currentSelector = `${firstPart.replace(/:nth-child\(\d+\)/g, '')} > ${restParts.join(' > ')}`; | |
| } | |
| } |
RohitR311
removed
the
Status: 🚫🚫 DO NOT MERGE 🚫🚫
Better Selector generation for child selectors to handle identical classes
Summary by CodeRabbit
nth-childpseudo-classes to ensure consistency.BrowserWindowcomponent for better formatting.