feat: selection revamp #266
Conversation
| Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces modifications to the selector generation and element information retrieval logic in the workflow management system. Changes are primarily focused on the Changes
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 0
🔭 Outside diff range comments (1)
server/src/workflow-management/selector.ts (1)
Line range hint
857-971: Refactor Duplicate Code to Improve MaintainabilityThe
ifandelseblocks within thegetNonUniqueSelectorsfunction contain duplicated code, particularly the definitions ofgetNonUniqueSelectorandgetSelectorPathfunctions and related logic. To enhance maintainability and reduce code duplication, consider extracting the common functions and logic outside of the conditional blocks.Apply this diff to refactor the duplicated code:
export const getNonUniqueSelectors = async (page: Page, coordinates: Coordinates, listSelector: string): Promise<SelectorResult> => { try { + // Define common functions outside of conditional blocks + const getNonUniqueSelector = (element: HTMLElement): string => { + let selector = element.tagName.toLowerCase(); + if (element.className) { + const classes = element.className.split(/\s+/).filter((cls: string) => Boolean(cls)); + if (classes.length > 0) { + const validClasses = classes.filter((cls: string) => !cls.startsWith('!') && !cls.includes(':')); + if (validClasses.length > 0) { + selector += '.' + validClasses.map(cls => CSS.escape(cls)).join('.'); + } + } + } + return selector; + }; + + const getSelectorPath = (element: HTMLElement | null): string => { + const path: string[] = []; + let depth = 0; + const maxDepth = 2; + while (element && element !== document.body && depth < maxDepth) { + const selector = getNonUniqueSelector(element); + path.unshift(selector); + element = element.parentElement; + depth++; + } + return path.join(' > '); + }; if (!listSelector) { console.log(`NON UNIQUE: MODE 1`); const selectors = await page.evaluate(({ x, y }) => { - function getNonUniqueSelector(element: HTMLElement): string { /* ... */ } - function getSelectorPath(element: HTMLElement | null): string { /* ... */ } const originalEl = document.elementFromPoint(x, y) as HTMLElement; if (!originalEl) return null; let element = originalEl; while (element.parentElement) { // ... existing logic ... } const generalSelector = getSelectorPath(element); return { generalSelector }; }, coordinates); return selectors || { generalSelector: '' }; } else { console.log(`NON UNIQUE: MODE 2`); const selectors = await page.evaluate(({ x, y }) => { - function getNonUniqueSelector(element: HTMLElement): string { /* ... */ } - function getSelectorPath(element: HTMLElement | null): string { /* ... */ } const originalEl = document.elementFromPoint(x, y) as HTMLElement; if (!originalEl) return null; let element = originalEl; const generalSelector = getSelectorPath(element); return { generalSelector }; }, coordinates); return selectors || { generalSelector: '' }; } } catch (error) { console.error('Error in getNonUniqueSelectors:', error); return { generalSelector: '' }; } };
🧹 Nitpick comments (3)
server/src/workflow-management/selector.ts (1)
Line range hint
185-188: Use Consistent Error Logging MechanismIn the
getElementInformationfunction, the error is logged usingconsole.error, whereas elsewhere in the file, errors are logged using theloggermodule. For consistency and better control over logging levels and outputs, consider replacingconsole.errorwithlogger.log('error', ...). This will ensure uniformity across your logging strategy.Apply this diff to update the error logging:
} catch (error) { const { message, stack } = error as Error; - console.error('Error while retrieving selector:', message); - console.error('Stack:', stack); + logger.log('error', `Error while retrieving selector: ${message}`); + logger.log('error', `Stack: ${stack}`); }server/src/workflow-management/classes/Generator.ts (2)
Line range hint
572-579: Simplify Nested Conditionals for Better ReadabilityThe nested
ifstatements in thegenerateDataForHighlighterfunction can be simplified to reduce complexity and improve readability. By consolidating conditions, you can minimize code duplication.Apply this diff to streamline the conditional logic:
if (rect) { - if (this.getList === true) { - if (this.listSelector !== '') { + if (this.getList && this.listSelector !== '') { const childSelectors = await getChildSelectors(page, this.listSelector); this.socket.emit('highlighter', { rect, selector: displaySelector, elementInfo, childSelectors }); - } else { - this.socket.emit('highlighter', { rect, selector: displaySelector, elementInfo }); - } } else { this.socket.emit('highlighter', { rect, selector: displaySelector, elementInfo }); } }
Line range hint
185-188: Use Consistent Error Logging MechanismIn the
getRectfunction, errors are logged usinglogger.log, which differs from theconsole.errorused ingetElementInformation. For consistency, ensure that all error logging within the file uses the same mechanism.Apply this diff to maintain consistent logging:
} catch (error) { const { message, stack } = error as Error; - console.error('Error while retrieving selector:', message); - console.error('Stack:', stack); + logger.log('error', `Error while retrieving selector: ${message}`); + logger.log('error', `Stack: ${stack}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/workflow-management/classes/Generator.ts(2 hunks)server/src/workflow-management/selector.ts(4 hunks)
🔇 Additional comments (3)
server/src/workflow-management/selector.ts (2)
23-25: Review Conditional Logic for Correctness
The conditional statement if (!getList || listSelector !== '') determines which block of code to execute based on the values of getList and listSelector. Please verify that this logic accurately reflects the intended behavior. Specifically, ensure that when getList is false or listSelector is not an empty string, the first block executes; otherwise, the second block executes.
20-20: Verify Proper Update of Function Calls with New Parameter
The function getElementInformation now includes a new parameter getList: boolean. Ensure that all calls to this function throughout the codebase are updated to include this new parameter to prevent potential runtime errors.
Run the following script to identify function calls that may not include the new parameter:
server/src/workflow-management/classes/Generator.ts (1)
Line range hint 544-551: Confirm Function Call Updates with New Parameter
The generateSelector function now calls getElementInformation with the new getList parameter. Ensure that the addition of this.getList aligns with the updated function signature and that all calls to getElementInformation include the new parameter.
amhsirak added Type: Enhancement 
amhsirak removed the Status: Work In Progess 
Summary by CodeRabbit
New Features
Bug Fixes
Documentation