feat: auto add capture fields for list selection #710
Conversation
RohitR311
added
Type: Enhancement
WalkthroughA new state variable was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserWindow
participant DOM
User->>BrowserWindow: Select group element for list step
BrowserWindow->>DOM: Query group elements and bounding rectangles
DOM-->>BrowserWindow: Return group elements with bounding rects
BrowserWindow->>BrowserWindow: Store group coordinates in state
BrowserWindow->>BrowserWindow: Create candidate fields from child selectors
BrowserWindow->>BrowserWindow: Add list step with generated fields
BrowserWindow->>BrowserWindow: Render overlay with highlights, labels, and scanning animation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (3)
src/components/browser/BrowserWindow.tsx (3)
532-611: Performance concern: Dynamic CSS keyframe generation.Creating unique CSS keyframes for each element (
scanDown-${index}) could lead to performance issues and memory bloat, especially with large lists. Consider using a shared animation with CSS custom properties instead.Replace dynamic keyframes with CSS custom properties:
- animation: `scanDown-${index} 2s ease-in-out infinite`, + animation: `scanDown 2s ease-in-out infinite`, + '--scan-height': `${groupElement.rect.height}px`, - <style>{` - @keyframes scanDown-${index} { - 0% { - transform: translateY(-8px); - } - 100% { - transform: translateY(${groupElement.rect.height}px); - } - } - `}</style>Add the shared keyframe to your main CSS file:
@keyframes scanDown { 0% { transform: translateY(-8px); } 100% { transform: translateY(var(--scan-height, 100px)); } }
532-643: Add accessibility consideration for reduced motion preference.The scanning animations don't respect users' motion preferences. Consider adding support for
prefers-reduced-motion.Add motion preference handling:
+const prefersReducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches; style={{ // ... other styles - animation: `scanDown-${index} 2s ease-in-out infinite`, + animation: prefersReducedMotion + ? 'none' + : `scanDown-${index} 2s ease-in-out infinite`, }}
614-642: Potential memory leak from dynamic style elements.The dynamic
<style>elements created for each group item are not cleaned up when the component re-renders or unmounts, which could lead to memory leaks.Consider moving animations to a shared stylesheet or implement proper cleanup:
+useEffect(() => { + const styleElements = document.querySelectorAll('style[data-scan-animation]'); + return () => { + // Cleanup dynamic styles on unmount + styleElements.forEach(el => el.remove()); + }; +}, [processingGroupCoordinates]); - <style>{` + <style data-scan-animation>{`
🧹 Nitpick comments (3)
src/components/browser/BrowserWindow.tsx (3)
169-169: LGTM with a minor consideration for memory management.The state variable declaration is well-typed and follows React patterns correctly. However, consider implementing cleanup logic to prevent potential memory leaks from holding HTMLElement references.
Consider adding cleanup logic when the component unmounts or when the list selection is reset:
+useEffect(() => { + return () => { + // Cleanup element references on unmount + setProcessingGroupCoordinates([]); + }; +}, []);
790-797: Good implementation with room for error handling improvements.The coordinate capture logic correctly maps group elements and stores their bounding rectangles. However, consider adding error handling for edge cases.
Add error handling for detached elements:
if (highlighterData?.groupInfo.groupElements) { setProcessingGroupCoordinates( highlighterData.groupInfo.groupElements.map((element) => ({ element, - rect: element.getBoundingClientRect(), + rect: element.isConnected ? element.getBoundingClientRect() : new DOMRect(), })) ); }
515-529: Consider extracting background overlay to a separate component.The background overlay setup is clean, but this entire section could benefit from being extracted into a dedicated component for better maintainability.
Extract to a separate component:
+const ProcessingOverlay = ({ + processingGroupCoordinates, + dimensions +}: { + processingGroupCoordinates: Array<{ element: HTMLElement; rect: DOMRect }>; + dimensions: { width: number; height: number }; +}) => ( + // Move overlay logic here +); -{isCachingChildSelectors && ( +{isCachingChildSelectors && ( + <ProcessingOverlay + processingGroupCoordinates={processingGroupCoordinates} + dimensions={dimensions} + /> )}
RohitR311
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/browser/BrowserWindow.tsx (2)
562-689: Consider performance optimizations and documentation.The helper functions are well-structured but could benefit from improvements:
Improvements to consider:
- Performance:
removeParentChildDuplicateshas O(n²) complexity that could be optimized- Documentation: Add JSDoc comments for complex functions
- Constants: Replace magic number
20in depth limit with a named constant+/** Maximum depth to traverse when calculating element depth from list */ +const MAX_ELEMENT_DEPTH = 20; +/** + * Determines if an element is a leaf node (has no contentful children) + * @param element - The HTML element to check + * @returns true if the element is a leaf node + */ const isLeafElement = (element: HTMLElement): boolean => { // ... existing implementation }; const getElementDepthFromList = ( element: HTMLElement, listSelector: string, document: Document ): number => { // ... existing logic - if (depth > 20) break; + if (depth > MAX_ELEMENT_DEPTH) break; // ... rest of implementation };
1914-2043: Consider extracting styles and optimizing animations for better performance.The enhanced loading overlay provides excellent user feedback, but the implementation could be optimized:
Concerns:
- Performance: Dynamic keyframe generation in render could cause issues
- Maintainability: Extensive inline styles make the code harder to maintain
- Memory: Dynamic style injection may cause memory leaks
Improvements:
+// Extract to CSS module or styled-components +const ProcessingOverlay = styled.div` + position: absolute; + top: 0; + left: 0; + width: 100%; + height: 100%; + background: rgba(255, 255, 255, 0.8); + z-index: 9999; + pointer-events: none; + border-radius: 0px 0px 5px 5px; +`; + +// Use CSS variables for dynamic values instead of inline keyframes +const ScanningAnimation = styled.div` + --scan-height: ${props => props.height}px; + animation: scanDown 2s ease-in-out infinite; +`;Consider using CSS-in-JS libraries or CSS modules for better style management and performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/browser/BrowserWindow.tsx(6 hunks)
🔇 Additional comments (3)
src/components/browser/BrowserWindow.tsx (3)
169-169: LGTM! Clean state addition for coordinate tracking.The new state variable appropriately tracks group element coordinates with proper TypeScript typing.
723-740: LGTM! Good integration of automatic field creation.The integration correctly calls the field creation function and conditionally updates state only when fields are generated. The logic follows existing patterns and appropriately uses the current list context.
1189-1196: LGTM! Smart coordinate preservation for group elements.This correctly captures group element coordinates at the moment of selection, ensuring they're available for UI rendering during the processing phase.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/recorder/DOMBrowserRenderer.tsx (1)
695-705: Ensure the click handler skips events during selector cachingThe
clickHandlercurrently only guards againstisInCaptureMode, but other handlers (e.g. scroll) also bail out whenisCachingChildSelectorsis true. To prevent clicks during caching, update its guard:• File:
src/components/recorder/DOMBrowserRenderer.tsx(around line 695)const clickHandler: EventListener = (e: Event) => { if (e.target && !iframeDoc.contains(e.target as Node)) { return; } - if (isInCaptureMode) { + if (isInCaptureMode || isCachingChildSelectors) { e.preventDefault(); e.stopPropagation(); return; } };
🧹 Nitpick comments (1)
src/components/recorder/DOMBrowserRenderer.tsx (1)
1018-1019: Simplify redundant overflow condition.The
overflowproperty uses the same value regardless of theisCachingChildSelectorscondition. ThepointerEventsconditional logic is correct for disabling interactions during caching.Apply this diff to simplify the redundant condition:
- overflow: isCachingChildSelectors ? "hidden !important" : "hidden !important", + overflow: "hidden !important", pointerEvents: isCachingChildSelectors ? "none" : "auto",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/recorder/DOMBrowserRenderer.tsx(2 hunks)
🔇 Additional comments (1)
src/components/recorder/DOMBrowserRenderer.tsx (1)
670-674: LGTM! Proper interaction blocking during caching.The early exit logic correctly prevents wheel events during child selector caching by calling
preventDefault()andstopPropagation(). This aligns well with the PR objective to enhance list selection UI.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/browser/BrowserWindow.tsx (1)
311-972: This function still needs the refactoring suggested in the previous review.The
createFieldsFromChildSelectorsfunction remains monolithic at 660+ lines and continues to mix multiple concerns. The issues identified in the previous review (nested helper functions, magic numbers, complex DOM traversal logic, and code duplication) are still present.
🧹 Nitpick comments (2)
src/components/browser/BrowserWindow.tsx (2)
974-1098: Extract helper functions outside the component.These utility functions (
isLeafElement,getElementDepthFromList,removeParentChildDuplicates,removeDuplicateContent) should be extracted outside the component to improve reusability and reduce the component's size. Consider moving them to a separate utility file or at least outside the component scope.+// Extract to utils/fieldProcessing.ts or similar +export const isLeafElement = (element: HTMLElement): boolean => { + // ... existing implementation +}; + +export const getElementDepthFromList = ( + element: HTMLElement, + listSelector: string, + document: Document +): number => { + // ... existing implementation +}; + +// ... other utilities export const BrowserWindow = () => { // ... component code - const isLeafElement = (element: HTMLElement): boolean => { - // ... implementation - }; - // ... other helper functions
2323-2452: Consider extracting UI styles and animations.The enhanced loading overlay provides excellent user feedback, but the implementation could be improved:
- Inline styles: Consider using CSS modules or styled-components for better maintainability
- Dynamic animations: The template literal keyframes could cause performance issues with many elements
- Magic numbers: Values like
-20pxfor label positioning should be constantsConsider refactoring to:
+// Extract to constants +const LABEL_OFFSET = 20; +const SCANNING_HEIGHT = 8; +// Consider using CSS modules or styled-components +const ScanningOverlay = styled.div<{ height: number; index: number }>` + position: absolute; + left: 0; + width: 100%; + height: ${SCANNING_HEIGHT}px; + background: linear-gradient(90deg, transparent 0%, rgba(255, 0, 195, 0.6) 50%, transparent 100%); + animation: scanDown 2s ease-in-out infinite; + will-change: transform; + + @keyframes scanDown { + 0% { transform: translateY(-${SCANNING_HEIGHT}px); } + 100% { transform: translateY(${props => props.height}px); } + } +`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/browser/BrowserWindow.tsx(6 hunks)
🔇 Additional comments (2)
src/components/browser/BrowserWindow.tsx (2)
169-169: LGTM - Clean state addition.The new state variable for tracking group element coordinates is well-typed and appropriately initialized.
1598-1605: LGTM - Good coordination between state and UI.The logic to capture group coordinates before setting the list selector is well-implemented and enables the enhanced visual feedback during processing.
| const autoFields = createFieldsFromChildSelectors( | ||
| childSelectors, | ||
| listSelector | ||
| ); | ||
|
|
||
| if (Object.keys(autoFields).length > 0) { | ||
| setFields(autoFields); | ||
|
|
||
| addListStep( | ||
| listSelector, | ||
| autoFields, | ||
| currentListId || Date.now(), | ||
| currentListActionId || `list-${crypto.randomUUID()}`, | ||
| { type: "", selector: paginationSelector }, | ||
| undefined, | ||
| false | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing dependency to useEffect.
The createFieldsFromChildSelectors function should be included in the useEffect dependency array to prevent stale closure issues.
}, [
isDOMMode,
listSelector,
socket,
getList,
currentSnapshot,
cachedListSelector,
pendingNotification,
notify,
+ createFieldsFromChildSelectors,
]);📝 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.
| const autoFields = createFieldsFromChildSelectors( | |
| childSelectors, | |
| listSelector | |
| ); | |
| if (Object.keys(autoFields).length > 0) { | |
| setFields(autoFields); | |
| addListStep( | |
| listSelector, | |
| autoFields, | |
| currentListId || Date.now(), | |
| currentListActionId || `list-${crypto.randomUUID()}`, | |
| { type: "", selector: paginationSelector }, | |
| undefined, | |
| false | |
| ); | |
| } | |
| }, [ | |
| isDOMMode, | |
| listSelector, | |
| socket, | |
| getList, | |
| currentSnapshot, | |
| cachedListSelector, | |
| pendingNotification, | |
| notify, | |
| createFieldsFromChildSelectors, | |
| ]); |
🤖 Prompt for AI Agents
In src/components/browser/BrowserWindow.tsx around lines 1132 to 1149, the
useEffect hook that uses the createFieldsFromChildSelectors function is missing
this function in its dependency array. To fix this, add
createFieldsFromChildSelectors to the dependency array of the useEffect hook to
ensure it updates correctly and avoids stale closures.
What this PR does?
2025-07-28.16-46-55.mp4
Summary by CodeRabbit
Summary by CodeRabbit