feat: conditional selection #246
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
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: 1
🧹 Outside diff range and nitpick comments (3)
server/src/workflow-management/selector.ts (3)
77-83: Remove duplicate entries incontainerTagsarrayThe
containerTagsarray contains duplicate entries such as'MAIN'and'MENUITEM', which could lead to redundant checks. Cleaning up the array improves efficiency and readability.Apply this diff to remove duplicates:
const containerTags = ['DIV', 'SECTION', 'ARTICLE', - 'MAIN', 'HEADER', 'FOOTER', 'NAV', 'ASIDE', + 'HEADER', 'FOOTER', 'NAV', 'ASIDE', 'ADDRESS', 'BLOCKQUOTE', 'DETAILS', 'DIALOG', 'FIGURE', 'FIGCAPTION', - 'MAIN', 'MARK', 'SUMMARY', 'TIME', + 'MARK', 'SUMMARY', 'TIME', // ... - 'MENU', 'MENUITEM', 'MENUITEM', 'APPLET', + 'MENU', 'MENUITEM', 'APPLET', // ... ];Also applies to: 205-212, 917-925
153-159: Use consistent logging method for error handlingThere is inconsistent use of
console.errorandlogger.logfor error logging. Standardizing onlogger.errorensures uniformity and better control over log output.Apply this diff to standardize error logging:
// In getElementInformation - console.error('Error while retrieving selector:', message); - console.error('Stack:', stack); + logger.error(`Error while retrieving selector: ${message}`); + logger.error(`Stack: ${stack}`); // In getRect (if not already using logger.error) - logger.log('error', `Error while retrieving selector: ${message}`); - logger.log('error', `Stack: ${stack}`); + logger.error(`Error while retrieving selector: ${message}`); + logger.error(`Stack: ${stack}`);Also applies to: 258-264
22-70: Consider extracting shared logic fromgetElementInformationandgetRectBoth functions share similar logic for element traversal and extraction. Extracting this logic into shared helper functions can enhance code reuse and maintainability.
Also applies to: 71-153, 175-198, 201-258
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
server/src/workflow-management/classes/Generator.ts(2 hunks)server/src/workflow-management/selector.ts(3 hunks)
🔇 Additional comments (2)
server/src/workflow-management/classes/Generator.ts (2)
572-574: LGTM!
The inclusion of this.listSelector when calling getRect and getElementInformation enhances functionality and aligns with the updated method signatures.
544-545: Ensure this.listSelector is properly initialized
The variable this.listSelector is used as a parameter but may be undefined if not set. Ensure it has a default value or is validated before use to prevent potential runtime errors.
Run the following script to check where this.listSelector is initialized:
✅ Verification successful
Let me gather more context about the initialization and usage of this.listSelector.
Let me try a different approach to gather information about the initialization and usage.
No initialization issues with this.listSelector
The variable this.listSelector is properly initialized with a default empty string value in the Generator class and is updated through a socket event listener. The code is safe as:
- It's initialized as
private listSelector: string = ''; - The
getElementInformationfunction handles empty string cases withif (listSelector !== '') - The value is updated via socket event:
this.socket.on('listSelector', (data: { selector: string }) => { this.listSelector = data.selector; })
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all assignments to `this.listSelector`.
# Find all assignments to `this.listSelector`
rg --type ts --pcre2 "this\.listSelector\s*=" --context 2
Length of output: 448
Script:
#!/bin/bash
# Search for the class definition and constructor
ast-grep --pattern 'class Generator {
$$$
constructor($_) {
$$$
}
$$$
}'
# Search for getElementInformation function to understand the parameter usage
rg --type ts "function getElementInformation" -B 2 -A 5
Length of output: 144
Script:
#!/bin/bash
# Search for the class properties and their initialization
ast-grep --pattern 'class Generator {
listSelector: $_
$$$
}'
# Search for the usage of listSelector in a broader context
rg --type ts "listSelector" -B 3 -A 3
Length of output: 18850
| if (listSelector !== '') { | ||
| const elementInfo = await page.evaluate( | ||
| async ({ x, y }) => { | ||
| const el = document.elementFromPoint(x, y) as HTMLElement; | ||
| if (el) { | ||
| const { parentElement } = el; | ||
| const element = parentElement?.tagName === 'A' ? parentElement : el; | ||
| let info: { | ||
| tagName: string; | ||
| hasOnlyText?: boolean; | ||
| innerText?: string; | ||
| url?: string; | ||
| imageUrl?: string; | ||
| attributes?: Record<string, string>; | ||
| innerHTML?: string; | ||
| outerHTML?: string; | ||
| } = { | ||
| tagName: element?.tagName ?? '', | ||
| }; | ||
| if (element) { | ||
| info.attributes = Array.from(element.attributes).reduce( | ||
| (acc, attr) => { | ||
| acc[attr.name] = attr.value; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, string> | ||
| ); | ||
| } | ||
| // Gather specific information based on the tag | ||
| if (element?.tagName === 'A') { | ||
| info.url = (element as HTMLAnchorElement).href; | ||
| info.innerText = element.innerText ?? ''; | ||
| } else if (element?.tagName === 'IMG') { | ||
| info.imageUrl = (element as HTMLImageElement).src; | ||
| } else { | ||
| info.hasOnlyText = element?.children?.length === 0 && | ||
| element?.innerText?.length > 0; | ||
| info.innerText = element?.innerText ?? ''; | ||
| } | ||
| info.innerHTML = element.innerHTML; | ||
| info.outerHTML = element.outerHTML; | ||
| return info; | ||
| } | ||
| return null; | ||
| }, | ||
| { x: coordinates.x, y: coordinates.y }, | ||
| ); | ||
| return elementInfo; | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate logic in getElementInformation function
The getElementInformation function contains duplicated code in both the if and else blocks, particularly the logic for extracting element information. Refactoring this code into a shared helper function can improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle.
Apply this diff to extract the common logic:
+// Define a helper function inside the evaluate callback
+function extractElementInfo(element) {
+ let info = {
+ tagName: element?.tagName ?? '',
+ };
+ if (element) {
+ info.attributes = Array.from(element.attributes).reduce(
+ (acc, attr) => {
+ acc[attr.name] = attr.value;
+ return acc;
+ },
+ {} as Record<string, string>
+ );
+ }
+ // Gather specific information based on the tag
+ if (element?.tagName === 'A') {
+ info.url = (element as HTMLAnchorElement).href;
+ info.innerText = element.innerText ?? '';
+ } else if (element?.tagName === 'IMG') {
+ info.imageUrl = (element as HTMLImageElement).src;
+ } else {
+ info.hasOnlyText = element?.children?.length === 0 &&
+ element?.innerText?.length > 0;
+ info.innerText = element?.innerText ?? '';
+ }
+ info.innerHTML = element.innerHTML;
+ info.outerHTML = element.outerHTML;
+ return info;
+}
// Inside both branches, replace the duplicated code with:
- // Existing code to extract element info
+ return extractElementInfo(element);Also applies to: 71-153
For capture list, based on whethere we are selecting the parent list or the children, rectangle and element information will be calculated. This change is to ensure proper child selection along with proper element calculation for the lists.
Summary by CodeRabbit
New Features
listSelectorparameter.listSelector, allowing for more detailed data collection.Bug Fixes
Documentation