fix: include visual containment for capture list selection #241
Conversation
WalkthroughThe changes in this pull request focus on enhancing the logic within the Changes
Possibly related PRs
Suggested labels
Poem
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 (4)
server/src/workflow-management/selector.ts (4)
Line range hint
867-881: Consider extracting visual containment logic into a reusable function.The visual containment check combines full containment and overlap ratio calculations. This logic is duplicated across multiple functions and uses a hardcoded threshold.
Consider extracting this into a utility function:
+ function isVisuallyContained(parentRect: DOMRect, childRect: DOMRect, overlapThreshold = 0.5): boolean { + const fullyContained = + parentRect.left <= childRect.left && + parentRect.right >= childRect.right && + parentRect.top <= childRect.top && + parentRect.bottom >= childRect.bottom; + + const significantOverlap = + (childRect.width * childRect.height) / + (parentRect.width * parentRect.height) > overlapThreshold; + + return fullyContained && significantOverlap; + }
Line range hint
892-895: Standardize error handling across the file.Error handling is inconsistent across functions. Some use
console.errorwhile others use theloggermodule. This inconsistency could make debugging more difficult.Standardize error handling using the logger:
- console.error('Error in getNonUniqueSelectors:', error); + const { message, stack } = error as Error; + logger.log('error', `Error in getNonUniqueSelectors: ${message}`); + logger.log('error', `Stack: ${stack}`);
Line range hint
4-4: Add missing function documentation and tests.The TODO comment indicates missing tests. Additionally, the selector generation functions would benefit from JSDoc documentation explaining the selection strategy.
Would you like me to help generate:
- Unit tests for the selector generation functions?
- JSDoc documentation for the selection strategy?
Line range hint
1-1024: Consider splitting the file into separate modules.The file handles multiple distinct concerns:
- Element selection
- Rectangle calculation
- Selector generation
- Visual containment logic
Consider splitting these into separate modules for better maintainability and testability.
Suggested structure:
src/workflow-management/ ├── selectors/ │ ├── element-finder.ts │ ├── rect-calculator.ts │ ├── selector-generator.ts │ └── visual-containment.ts └── constants/ └── container-tags.ts
| const containerTags = ['DIV', 'SECTION', 'ARTICLE', 'MAIN', 'HEADER', 'FOOTER', 'NAV', 'ASIDE', | ||
| 'ADDRESS', 'BLOCKQUOTE', 'DETAILS', 'DIALOG', 'FIGURE', 'FIGCAPTION', 'MAIN', 'MARK', 'SUMMARY', 'TIME', | ||
| 'TABLE', 'THEAD', 'TBODY', 'TFOOT', 'TR', 'TH', 'TD', 'CAPTION', 'COLGROUP', 'COL', 'FORM', 'FIELDSET', | ||
| 'LEGEND', 'LABEL', 'INPUT', 'BUTTON', 'SELECT', 'DATALIST', 'OPTGROUP', 'OPTION', 'TEXTAREA', 'OUTPUT', | ||
| 'PROGRESS', 'METER', 'DETAILS', 'SUMMARY', 'MENU', 'MENUITEM', 'MENUITEM', 'APPLET', 'EMBED', 'OBJECT', | ||
| 'PARAM', 'VIDEO', 'AUDIO', 'SOURCE', 'TRACK', 'CANVAS', 'MAP', 'AREA', 'SVG', 'IFRAME', 'FRAME', 'FRAMESET', | ||
| 'LI', 'UL', 'OL', 'DL', 'DT', 'DD', 'HR', 'P', 'PRE', 'LISTING', 'PLAINTEXT', 'A' | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated container tags into a shared constant.
The container tags list is duplicated across multiple functions. Additionally, 'MAIN' and 'MENUITEM' are repeated in the list.
Consider moving this to a shared constant:
+ const CONTAINER_TAGS = ['DIV', 'SECTION', 'ARTICLE', 'MAIN', 'HEADER', 'FOOTER', 'NAV', 'ASIDE',
+ 'ADDRESS', 'BLOCKQUOTE', 'DETAILS', 'DIALOG', 'FIGURE', 'FIGCAPTION', 'MARK', 'SUMMARY', 'TIME',
+ 'TABLE', 'THEAD', 'TBODY', 'TFOOT', 'TR', 'TH', 'TD', 'CAPTION', 'COLGROUP', 'COL', 'FORM', 'FIELDSET',
+ 'LEGEND', 'LABEL', 'INPUT', 'BUTTON', 'SELECT', 'DATALIST', 'OPTGROUP', 'OPTION', 'TEXTAREA', 'OUTPUT',
+ 'PROGRESS', 'METER', 'DETAILS', 'SUMMARY', 'MENU', 'MENUITEM', 'APPLET', 'EMBED', 'OBJECT',
+ 'PARAM', 'VIDEO', 'AUDIO', 'SOURCE', 'TRACK', 'CANVAS', 'MAP', 'AREA', 'SVG', 'IFRAME', 'FRAME', 'FRAMESET',
+ 'LI', 'UL', 'OL', 'DL', 'DT', 'DD', 'HR', 'P', 'PRE', 'LISTING', 'PLAINTEXT', 'A'] as const;Also applies to: 851-858
Summary by CodeRabbit
New Features
Bug Fixes