feat: recorder revamp #654
Conversation
RohitR311
added
Type: Feature
WalkthroughThis update introduces a comprehensive DOM streaming and rendering feature across both backend and frontend. It adds rrweb-based DOM snapshot capture, resource caching, and event-driven streaming on the server, and a new interactive DOM renderer with client-side data extraction, highlight, and picker integration on the client. Input handling, workflow generation, and browser management are refactored to support both DOM and screenshot modes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as BrowserWindow/DOMBrowserRenderer
participant Socket
participant RemoteBrowser
participant ChromeCDP
Client->>Socket: Request DOM streaming (initialize)
Socket->>RemoteBrowser: subscribeToDOM()
RemoteBrowser->>ChromeCDP: Inject rrweb, start network monitoring
loop On DOM/content load, scroll, or network idle
RemoteBrowser->>ChromeCDP: Capture rrweb snapshot
RemoteBrowser->>RemoteBrowser: Process snapshot, embed resources
RemoteBrowser->>Socket: Emit domcast event with snapshot
Socket->>Client: domcast (rrweb snapshot)
Client->>DOMBrowserRenderer: Render snapshot in iframe
end
Client->>Socket: User events (click, keypress, picker, etc.)
Socket->>RemoteBrowser: Forward DOM event
RemoteBrowser->>ChromeCDP: Perform action, possibly emit new snapshot
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
amhsirak
removed
the
Status: Work In Progess
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (2)
server/src/browser-management/classes/rrweb-bundle.js (1)
1-2: Avoid committing minified third-party code directly.This appears to be a minified version of the rrweb library. Committing minified code makes it difficult to review, audit for security issues, and maintain.
Consider these alternatives:
- Use npm/yarn to install rrweb as a dependency and import it normally
- If customization is needed, maintain the source code and use a build process to generate the bundle
- At minimum, include the source maps and original source for review
Additionally, the
"use strict"directive is redundant in ES modules as noted by static analysis.src/components/browser/BrowserWindow.tsx (1)
145-1284: Consider splitting this large component for better maintainability.The
BrowserWindowcomponent has grown to over 1200 lines and handles multiple responsibilities. This makes it difficult to maintain and test.Consider extracting these into separate components:
- Picker Management: Extract date/time/dropdown picker state and handlers into a
PickerManagercomponent- DOM Mode Renderer: The DOM mode logic (lines 1180-1241) is already partially extracted but could be enhanced
- Screenshot Mode: Extract screenshot canvas and related logic
- Highlighter Logic: The complex highlighter calculations could be moved to a custom hook
- Socket Event Handlers: Group related socket handlers into custom hooks
Example structure:
// hooks/useDOMMode.ts export const useDOMMode = (socket: Socket, userId?: string) => { const [isDOMMode, setIsDOMMode] = useState(false); const [currentSnapshot, setCurrentSnapshot] = useState<ProcessedSnapshot | null>(null); // All DOM mode related handlers and effects return { isDOMMode, currentSnapshot, /* ... */ }; }; // hooks/usePickerManagement.ts export const usePickerManagement = () => { // All picker state and handlers return { datePickerInfo, setDatePickerInfo, /* ... */ }; }; // components/BrowserRenderer.tsx const BrowserRenderer = ({ isDOMMode, snapshot, dimensions, /* ... */ }) => { if (isDOMMode) { return <DOMBrowserRenderer {...props} />; } return <Canvas {...props} />; };This refactoring would:
- Improve testability
- Make the code more maintainable
- Follow Single Responsibility Principle
- Make it easier to add new features
♻️ Duplicate comments (2)
src/components/recorder/RightSidePanel.tsx (1)
195-212: Use shared utility for iframe selection.This iframe selection logic is duplicated from
DateTimeLocalPicker.tsx. As suggested in that file's review, extract this to a shared utility function.+import { findDOMIframe } from '../../helpers/domUtils'; // Find the DOM iframe element -let iframeElement = document.querySelector( - "#dom-browser-iframe" -) as HTMLIFrameElement; - -if (!iframeElement) { - iframeElement = document.querySelector( - "#browser-window iframe" - ) as HTMLIFrameElement; -} - -if (!iframeElement) { - const browserWindow = document.querySelector("#browser-window"); - if (browserWindow) { - iframeElement = browserWindow.querySelector( - "iframe" - ) as HTMLIFrameElement; - } -} +const iframeElement = findDOMIframe(); if (!iframeElement) { console.error( "Could not find the DOM iframe element for extraction" ); return; }src/components/browser/BrowserWindow.tsx (1)
417-434: Use shared utility for iframe selection.This is another instance of the duplicated iframe selection logic. Use the shared utility function suggested in the
DateTimeLocalPicker.tsxreview.+import { findDOMIframe } from '../../helpers/domUtils'; -let iframeElement = document.querySelector( - "#dom-browser-iframe" -) as HTMLIFrameElement; - -if (!iframeElement) { - iframeElement = document.querySelector( - "#browser-window iframe" - ) as HTMLIFrameElement; -} - -if (!iframeElement) { - const browserWindow = document.querySelector("#browser-window"); - if (browserWindow) { - iframeElement = browserWindow.querySelector( - "iframe" - ) as HTMLIFrameElement; - } -} +const iframeElement = findDOMIframe(); if (!iframeElement) { console.error("Could not find iframe element for DOM highlighting"); return; }
🧹 Nitpick comments (10)
src/components/pickers/DatePicker.tsx (1)
69-69: Consider error handling for DOM update failures.The
updateDOMElementcall doesn't check for success/failure, which could lead to inconsistent state between the socket emission and DOM update.- updateDOMElement(selector, selectedDate); + const updated = updateDOMElement(selector, selectedDate); + if (!updated) { + console.warn('Failed to update DOM element, relying on server-side update'); + }src/helpers/clientListExtractor.ts (1)
413-430: Improve type safety by avoiding 'any' type.The
fieldsparameter usesanytype, which reduces type safety. Based on theconvertFieldsmethod, the structure is well-defined and should have proper typing.interface FieldDefinition { label: string; selectorObj: { selector: string; attribute: string; tag?: string; shadow?: boolean; }; } interface FieldsInput { [key: string]: TextStep; } private convertFields = ( fields: FieldsInput ): Record<string, { selector: string; attribute: string }> => { const convertedFields: Record< string, { selector: string; attribute: string } > = {}; for (const [key, field] of Object.entries(fields)) { convertedFields[field.label] = { selector: field.selectorObj.selector, attribute: field.selectorObj.attribute, }; } return convertedFields; }; public extractListData = ( iframeDocument: Document, listSelector: string, fields: FieldsInput, // Use proper type instead of any limit: number = 5 ): ExtractedListData[] => { // ... };src/components/recorder/RightSidePanel.tsx (1)
100-107: Use optional chaining for cleaner code.The condition can be simplified using optional chaining.
const domcastHandler = (data: any) => { if (!data.userId || data.userId === id) { - if (data.snapshotData && data.snapshotData.snapshot) { + if (data.snapshotData?.snapshot) { setCurrentSnapshot(data.snapshotData); setIsDOMMode(true); } } };src/components/browser/BrowserWindow.tsx (2)
242-257: Use optional chaining for cleaner code.Simplify the condition check using optional chaining.
const rrwebSnapshotHandler = useCallback( (data: RRWebDOMCastData) => { if (!data.userId || data.userId === user?.id) { - if (data.snapshotData && data.snapshotData.snapshot) { + if (data.snapshotData?.snapshot) { setCurrentSnapshot(data.snapshotData); setIsDOMMode(true); socket?.emit("dom-mode-enabled");
1152-1177: Remove unnecessary Fragment wrapper.The Fragment is redundant as it contains only a single child element.
{isDOMMode && highlighterData && ( - <> <div style={{ position: "absolute", left: Math.max(0, highlighterData.rect.x), top: Math.max(0, highlighterData.rect.y), width: Math.min( highlighterData.rect.width, dimensions.width ), height: Math.min( highlighterData.rect.height, dimensions.height ), background: "rgba(255, 0, 195, 0.15)", border: "2px solid #ff00c3", borderRadius: "3px", pointerEvents: "none", zIndex: 1000, boxShadow: "0 0 0 1px rgba(255, 255, 255, 0.8)", transition: "all 0.1s ease-out", }} /> - </> )}src/components/recorder/DOMBrowserRenderer.tsx (2)
84-92: Improve RRWebSnapshot interface typing.The interface uses generic typing with
[key: string]: anywhich reduces type safety. Consider defining more specific types for known properties or using a discriminated union based on thetypefield.-interface RRWebSnapshot { - type: number; - childNodes?: RRWebSnapshot[]; - tagName?: string; - attributes?: Record<string, string>; - textContent: string; - id: number; - [key: string]: any; -} +interface RRWebSnapshot { + type: number; + childNodes?: RRWebSnapshot[]; + tagName?: string; + attributes?: Record<string, string>; + textContent?: string; + id: number; + // Define specific properties as needed instead of using index signature +}
140-1139: Consider implementing virtualization for better performance.For large DOM snapshots, rendering the entire DOM tree might cause performance issues. Consider implementing virtualization or lazy loading of DOM nodes that are outside the viewport to improve performance for complex pages.
server/src/browser-management/classes/RemoteBrowser.ts (2)
475-696: Improve type safety for resources parameterThe
processRRWebSnapshotmethod usesanytype for the resources parameter in helper functions. Consider using the already defined resource structure for better type safety.Replace the
anytype with a proper interface:- resources?: any + resources?: ProcessedSnapshot['resources']This applies to both
processRRWebSnapshotandprocessCSSmethods.
794-816: Consider making cache limits configurableThe cache cleanup uses hardcoded values (5 minutes max age, 200 max size). These limits might need adjustment based on usage patterns.
Consider making these configurable:
+private readonly CACHE_CONFIG = { + maxAge: 5 * 60 * 1000, // 5 minutes + maxSize: 200, + cleanupBatchSize: 50 +}; + private cleanupResourceCache(): void { const now = Date.now(); - const maxAge = 5 * 60 * 1000; // 5 minutes + const maxAge = this.CACHE_CONFIG.maxAge; for (const [url, resource] of this.networkResourceCache.entries()) { if (now - resource.timestamp > maxAge) { this.networkResourceCache.delete(url); } } - if (this.networkResourceCache.size > 200) { + if (this.networkResourceCache.size > this.CACHE_CONFIG.maxSize) { const entries = Array.from(this.networkResourceCache.entries()); entries.sort((a, b) => a[1].timestamp - b[1].timestamp); - for (let i = 0; i < 50; i++) { + for (let i = 0; i < this.CACHE_CONFIG.cleanupBatchSize; i++) { this.networkResourceCache.delete(entries[i][0]); } }server/src/browser-management/inputHandlers.ts (1)
648-665: Consider making SPA timeouts configurableThe SPA detection logic uses hardcoded timeouts (1500ms and 2000ms) which might not be suitable for all applications.
Consider making these configurable:
+const SPA_WAIT_TIME = 1500; +const POST_ACTION_WAIT_TIME = 2000; + if (isSPA) { logger.log("debug", `SPA interaction detected for selector: ${selector}`); - await new Promise((resolve) => setTimeout(resolve, 1500)); + await new Promise((resolve) => setTimeout(resolve, SPA_WAIT_TIME)); } else { // ... navigation logic } -await new Promise((resolve) => setTimeout(resolve, 2000)); +await new Promise((resolve) => setTimeout(resolve, POST_ACTION_WAIT_TIME));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
server/src/browser-management/classes/RemoteBrowser.ts(11 hunks)server/src/browser-management/classes/bundle-rrweb.js(1 hunks)server/src/browser-management/classes/rrweb-bundle.js(1 hunks)server/src/browser-management/classes/rrweb-entry.js(1 hunks)server/src/browser-management/controller.ts(2 hunks)server/src/browser-management/inputHandlers.ts(17 hunks)server/src/workflow-management/classes/Generator.ts(6 hunks)src/components/browser/BrowserWindow.tsx(10 hunks)src/components/pickers/DatePicker.tsx(1 hunks)src/components/pickers/DateTimeLocalPicker.tsx(2 hunks)src/components/pickers/Dropdown.tsx(1 hunks)src/components/recorder/DOMBrowserRenderer.tsx(1 hunks)src/components/recorder/RightSidePanel.tsx(6 hunks)src/helpers/clientListExtractor.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/helpers/clientListExtractor.ts
[error] 145-145: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
src/components/recorder/RightSidePanel.tsx
[error] 102-102: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
server/src/browser-management/classes/rrweb-bundle.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 1-1: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1-1: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 1-1: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 1-1: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1-1: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1-1: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1-1: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1-1: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1-1: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1-1: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 1-1: Shouldn't redeclare 'h'. Consider to delete it or rename it.
'h' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: Shouldn't redeclare 'X'. Consider to delete it or rename it.
'X' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: Shouldn't redeclare 'z'. Consider to delete it or rename it.
'z' is defined here:
(lint/suspicious/noRedeclare)
[error] 1-1: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
src/components/recorder/DOMBrowserRenderer.tsx
[error] 247-247: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 416-416: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/browser/BrowserWindow.tsx
[error] 245-245: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 475-476: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1152-1175: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
server/src/browser-management/classes/RemoteBrowser.ts
[error] 513-514: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 538-538: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 594-595: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (13)
server/src/browser-management/classes/rrweb-entry.js (1)
1-2: Clean implementation for rrweb integration.The entry point correctly imports and exposes the rrweb snapshot functionality for browser injection. The approach is straightforward and follows standard patterns for creating global APIs.
server/src/browser-management/classes/bundle-rrweb.js (1)
3-10: Well-configured build script for browser injection.The esbuild configuration is appropriate for creating a browser-injectable bundle:
- IIFE format ensures proper execution in browser contexts
- Global name provides accessible API
- Minification optimizes bundle size
- Entry point correctly references the rrweb-entry.js file
server/src/browser-management/controller.ts (1)
41-47: Verify the default mode change impact.The default mode has changed from screenshot to DOM streaming. This could be a breaking change if other parts of the codebase expect screenshot mode by default.
#!/bin/bash # Description: Find all calls to initializeRemoteBrowserForRecording to verify impact # Expected: Calls without explicit mode parameter will now default to DOM mode rg -A 3 -B 3 "initializeRemoteBrowserForRecording"src/components/recorder/DOMBrowserRenderer.tsx (1)
886-892: Good security implementation for HTML sanitization.The code properly sanitizes the rendered HTML by removing scripts, event handlers, and javascript: protocols. This is crucial for preventing XSS attacks when rendering untrusted DOM content.
server/src/workflow-management/classes/Generator.ts (2)
367-455: Well-structured DOM interaction methods.The new DOM methods follow consistent patterns with the existing codebase and properly handle:
- Special cases for input/textarea elements with cursor positioning
- Encryption of keyboard input for security
- Proper selector and URL handling
- Integration with the existing workflow generation system
140-151: ```bash
#!/bin/bashDeep search for socket emit calls for DOM and screenshot modes
rg ".emit(['"]dom-mode-enabled['"]" -n .
rg ".emit(['"]screenshot-mode-enabled['"]" -n .
rg "dom-mode-enabled" -n .
rg "screenshot-mode-enabled" -n .</details> <details> <summary>server/src/browser-management/classes/RemoteBrowser.ts (4)</summary> `20-86`: **Well-structured interfaces for DOM streaming** The global declarations and interfaces are properly defined. The `ProcessedSnapshot` interface provides comprehensive resource tracking with appropriate categorization for different resource types. --- `193-231`: **Appropriate state management for DOM streaming** The new properties effectively manage DOM streaming state, network resource caching, and request tracking. The use of Map for caching with timestamps enables proper cache management. --- `2031-2224`: **Well-implemented DOM streaming lifecycle** The DOM streaming lifecycle methods properly handle initialization, snapshot creation, and cleanup. The error handling for navigation context destruction is particularly well done, preventing unnecessary error emissions during expected page transitions. --- `195-196`: **Verify renderingMode initialization** The `renderingMode` property defaults to "screenshot" but there's no visible method to set it to "dom". This needs to be configurable. ```shell #!/bin/bash # Search for where renderingMode is set or could be set rg -A 5 "renderingMode" --type ts # Check if there's a setter method or constructor parameter ast-grep --pattern 'renderingMode = $_'Also applies to: 2420-2424
server/src/browser-management/inputHandlers.ts (3)
30-59: Clean refactoring to RemoteBrowser abstractionThe refactoring of
handleWrapperto useRemoteBrowserimproves the architecture by providing handlers with full browser context while maintaining the existing interpretation checks.
140-147: Navigation timeout increased - verify impactThe navigation timeout was increased from 1000ms to 2000ms. This might affect the responsiveness of click actions.
Was this change intentional? Consider making the timeout configurable or documenting why 2000ms is necessary. The increased timeout could make the UI feel less responsive during navigation events.
821-823: Clean integration of DOM event handlersThe new DOM event handlers are properly registered following the existing pattern, maintaining consistency with the codebase.
| * @category BrowserManagement-Controller | ||
| */ | ||
| export const initializeRemoteBrowserForRecording = (userId: string): string => { | ||
| export const initializeRemoteBrowserForRecording = (userId: string, mode: string = "dom"): string => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type annotation for the mode parameter.
The mode parameter should be typed for better type safety and API clarity.
-export const initializeRemoteBrowserForRecording = (userId: string, mode: string = "dom"): string => {
+export const initializeRemoteBrowserForRecording = (userId: string, mode: "dom" | "screenshot" = "dom"): string => {📝 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.
| export const initializeRemoteBrowserForRecording = (userId: string, mode: string = "dom"): string => { | |
| export const initializeRemoteBrowserForRecording = (userId: string, mode: "dom" | "screenshot" = "dom"): string => { |
🤖 Prompt for AI Agents
In server/src/browser-management/controller.ts at line 23, the mode parameter in
the initializeRemoteBrowserForRecording function lacks a type annotation. Add an
explicit type annotation to the mode parameter, such as a string literal union
type or a specific string type, to improve type safety and clarify the expected
values for this parameter.
| const updateDOMElement = (selector: string, value: string) => { | ||
| try { | ||
| let iframeElement = document.querySelector('#dom-browser-iframe') as HTMLIFrameElement; | ||
|
|
||
| if (!iframeElement) { | ||
| iframeElement = document.querySelector('#browser-window iframe') as HTMLIFrameElement; | ||
| } | ||
|
|
||
| if (!iframeElement) { | ||
| const browserWindow = document.querySelector('#browser-window'); | ||
| if (browserWindow) { | ||
| iframeElement = browserWindow.querySelector('iframe') as HTMLIFrameElement; | ||
| } | ||
| } | ||
|
|
||
| if (!iframeElement) { | ||
| console.error('Could not find iframe element for DOM update'); | ||
| return; | ||
| } | ||
|
|
||
| const iframeDoc = iframeElement.contentDocument; | ||
| if (!iframeDoc) { | ||
| console.error('Could not access iframe document'); | ||
| return; | ||
| } | ||
|
|
||
| const element = iframeDoc.querySelector(selector) as HTMLInputElement; | ||
| if (element) { | ||
| element.value = value; | ||
|
|
||
| const changeEvent = new Event('change', { bubbles: true }); | ||
| element.dispatchEvent(changeEvent); | ||
|
|
||
| const inputEvent = new Event('input', { bubbles: true }); | ||
| element.dispatchEvent(inputEvent); | ||
| } else { | ||
| console.warn(`Could not find element with selector: ${selector}`); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error updating DOM element:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the iframe selection strategy for better maintainability.
The current approach tries multiple selectors to find the iframe, which is fragile and indicates tight coupling. Consider:
- Centralize iframe reference: Create a shared service or context that provides access to the DOM iframe
- Use consistent iframe identification: Standardize on a single ID/selector across the application
- Add type safety: The iframe document access lacks proper type checking
Consider creating a shared utility:
// utils/domInteraction.ts
export const getDOMIframe = (): HTMLIFrameElement | null => {
return document.querySelector('#dom-browser-iframe') as HTMLIFrameElement;
};
export const updateDOMElement = (selector: string, value: string): boolean => {
const iframe = getDOMIframe();
if (!iframe?.contentDocument) return false;
const element = iframe.contentDocument.querySelector(selector) as HTMLInputElement;
if (!element) return false;
element.value = value;
['change', 'input'].forEach(eventType => {
element.dispatchEvent(new Event(eventType, { bubbles: true }));
});
return true;
};🤖 Prompt for AI Agents
In src/components/pickers/DatePicker.tsx around lines 19 to 60, refactor the
iframe selection logic by centralizing the iframe reference into a shared
utility function that consistently returns the iframe element using a single
standardized selector like '#dom-browser-iframe'. Replace the multiple
conditional queries with a single call to this utility. Add proper type safety
by ensuring the iframe and its contentDocument are correctly typed and checked
before accessing elements. Move the updateDOMElement logic to use this utility,
simplify event dispatching, and return a boolean indicating success or failure
for better maintainability and clarity.
| const updateDOMElement = (selector: string, value: string) => { | ||
| try { | ||
| let iframeElement = document.querySelector('#dom-browser-iframe') as HTMLIFrameElement; | ||
|
|
||
| if (!iframeElement) { | ||
| iframeElement = document.querySelector('#browser-window iframe') as HTMLIFrameElement; | ||
| } | ||
|
|
||
| if (!iframeElement) { | ||
| const browserWindow = document.querySelector('#browser-window'); | ||
| if (browserWindow) { | ||
| iframeElement = browserWindow.querySelector('iframe') as HTMLIFrameElement; | ||
| } | ||
| } | ||
|
|
||
| if (!iframeElement) { | ||
| console.error('Could not find iframe element for DOM update'); | ||
| return; | ||
| } | ||
|
|
||
| const iframeDoc = iframeElement.contentDocument; | ||
| if (!iframeDoc) { | ||
| console.error('Could not access iframe document'); | ||
| return; | ||
| } | ||
|
|
||
| const selectElement = iframeDoc.querySelector(selector) as HTMLSelectElement; | ||
| if (selectElement) { | ||
| selectElement.value = value; | ||
|
|
||
| const optionElements = selectElement.querySelectorAll('option'); | ||
| optionElements.forEach(option => { | ||
| if (option.value === value) { | ||
| option.selected = true; | ||
| option.setAttribute('selected', 'selected'); | ||
| } else { | ||
| option.selected = false; | ||
| option.removeAttribute('selected'); | ||
| } | ||
| }); | ||
|
|
||
| const changeEvent = new Event('change', { bubbles: true }); | ||
| selectElement.dispatchEvent(changeEvent); | ||
|
|
||
| const inputEvent = new Event('input', { bubbles: true }); | ||
| selectElement.dispatchEvent(inputEvent); | ||
| } else { | ||
| console.warn(`Could not find select element with selector: ${selector}`); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error updating DOM select element:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicate DOM manipulation logic into shared utility.
This updateDOMElement function is nearly identical to the one in DatePicker.tsx, violating the DRY principle. The logic should be extracted into a shared utility that can handle different element types.
Create a shared utility that handles both input and select elements:
// utils/domInteraction.ts
interface DOMUpdateOptions {
selector: string;
value: string;
elementType?: 'input' | 'select';
}
export const updateDOMElement = ({ selector, value, elementType }: DOMUpdateOptions): boolean => {
const iframe = getDOMIframe();
if (!iframe?.contentDocument) return false;
const element = iframe.contentDocument.querySelector(selector);
if (!element) return false;
if (elementType === 'select' || element.tagName.toLowerCase() === 'select') {
const selectElement = element as HTMLSelectElement;
selectElement.value = value;
// Update option states
Array.from(selectElement.options).forEach(option => {
option.selected = option.value === value;
});
} else {
(element as HTMLInputElement).value = value;
}
// Dispatch events
['change', 'input'].forEach(eventType => {
element.dispatchEvent(new Event(eventType, { bubbles: true }));
});
return true;
};Then use it in both components:
updateDOMElement({ selector, value: selectedDate }); // DatePicker
updateDOMElement({ selector, value, elementType: 'select' }); // Dropdown🤖 Prompt for AI Agents
In src/components/pickers/Dropdown.tsx lines 21 to 73, the updateDOMElement
function duplicates DOM manipulation logic found in DatePicker.tsx, violating
the DRY principle. Extract this logic into a shared utility function in a new
file like utils/domInteraction.ts that accepts parameters for selector, value,
and elementType (input or select). This utility should locate the iframe and
element, update the element's value accordingly, handle select option states if
applicable, and dispatch change and input events. Then replace the existing
updateDOMElement in Dropdown.tsx and DatePicker.tsx with calls to this shared
utility, passing the appropriate parameters.
| const updateDOMElement = (selector: string, value: string) => { | ||
| try { | ||
| let iframeElement = document.querySelector('#dom-browser-iframe') as HTMLIFrameElement; | ||
|
|
||
| if (!iframeElement) { | ||
| iframeElement = document.querySelector('#browser-window iframe') as HTMLIFrameElement; | ||
| } | ||
|
|
||
| if (!iframeElement) { | ||
| const browserWindow = document.querySelector('#browser-window'); | ||
| if (browserWindow) { | ||
| iframeElement = browserWindow.querySelector('iframe') as HTMLIFrameElement; | ||
| } | ||
| } | ||
|
|
||
| if (!iframeElement) { | ||
| console.error('Could not find iframe element for DOM update'); | ||
| return; | ||
| } | ||
|
|
||
| const iframeDoc = iframeElement.contentDocument; | ||
| if (!iframeDoc) { | ||
| console.error('Could not access iframe document'); | ||
| return; | ||
| } | ||
|
|
||
| const element = iframeDoc.querySelector(selector) as HTMLInputElement; | ||
| if (element) { | ||
| element.value = value; | ||
|
|
||
| const changeEvent = new Event('change', { bubbles: true }); | ||
| element.dispatchEvent(changeEvent); | ||
|
|
||
| const inputEvent = new Event('input', { bubbles: true }); | ||
| element.dispatchEvent(inputEvent); | ||
| } else { | ||
| console.warn(`Could not find element with selector: ${selector}`); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error updating DOM element:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract iframe selection logic to a shared utility and add parameter validation.
The iframe selection logic (lines 21-32) appears to be duplicated across multiple picker components. This violates the DRY principle and makes maintenance harder.
Additionally, the selector parameter should be validated before use.
Consider creating a shared utility:
// src/helpers/domUtils.ts
export const findDOMIframe = (): HTMLIFrameElement | null => {
let iframeElement = document.querySelector('#dom-browser-iframe') as HTMLIFrameElement;
if (!iframeElement) {
iframeElement = document.querySelector('#browser-window iframe') as HTMLIFrameElement;
}
if (!iframeElement) {
const browserWindow = document.querySelector('#browser-window');
if (browserWindow) {
iframeElement = browserWindow.querySelector('iframe') as HTMLIFrameElement;
}
}
return iframeElement;
};
export const updateDOMElement = (selector: string, value: string) => {
if (!selector || typeof selector !== 'string') {
console.error('Invalid selector provided to updateDOMElement');
return;
}
try {
const iframeElement = findDOMIframe();
if (!iframeElement) {
console.error('Could not find iframe element for DOM update');
return;
}
const iframeDoc = iframeElement.contentDocument;
if (!iframeDoc) {
console.error('Could not access iframe document');
return;
}
const element = iframeDoc.querySelector(selector) as HTMLInputElement;
if (element) {
element.value = value;
const changeEvent = new Event('change', { bubbles: true });
element.dispatchEvent(changeEvent);
const inputEvent = new Event('input', { bubbles: true });
element.dispatchEvent(inputEvent);
} else {
console.warn(`Could not find element with selector: ${selector}`);
}
} catch (error) {
console.error('Error updating DOM element:', error);
}
};Then import and use it in this component:
+import { updateDOMElement } from '../../helpers/domUtils';
- const updateDOMElement = (selector: string, value: string) => {
- // ... entire function ...
- };🤖 Prompt for AI Agents
In src/components/pickers/DateTimeLocalPicker.tsx around lines 19 to 60, the
iframe selection logic is duplicated and the selector parameter lacks
validation. Extract the iframe selection code into a shared utility function
(e.g., findDOMIframe) in a new helper file like src/helpers/domUtils.ts. Add
validation to ensure the selector parameter is a non-empty string before
proceeding. Refactor updateDOMElement to use the shared findDOMIframe utility
and include the selector validation, then import and use this updated function
in the component.
| public extractListData = ( | ||
| iframeDocument: Document, | ||
| listSelector: string, | ||
| fields: any, | ||
| limit: number = 5 | ||
| ): ExtractedListData[] => { | ||
| try { | ||
| // Convert fields to the format expected by the extraction logic | ||
| const convertedFields = this.convertFields(fields); | ||
|
|
||
| // Get all container elements matching the list selector | ||
| let containers = this.queryElementAll(iframeDocument, listSelector); | ||
|
|
||
| if (containers.length === 0) { | ||
| console.warn("No containers found for listSelector:", listSelector); | ||
| return []; | ||
| } | ||
|
|
||
| // Enhanced container discovery: find similar elements if we need more containers | ||
| if (limit > 1 && containers.length === 1) { | ||
| const baseContainer = containers[0]; | ||
| const similarContainers = this.findSimilarElements( | ||
| baseContainer, | ||
| iframeDocument, | ||
| 0.7 | ||
| ); | ||
|
|
||
| if (similarContainers.length > 0) { | ||
| const newContainers = similarContainers.filter( | ||
| (container) => !container.matches(listSelector) | ||
| ); | ||
| containers = [...containers, ...newContainers]; | ||
| } | ||
| } | ||
|
|
||
| console.log("📦 Found containers:", containers.length); | ||
|
|
||
| // Analyze fields for table vs non-table context | ||
| const containerFields: ContainerFields[] = containers.map(() => ({ | ||
| tableFields: {}, | ||
| nonTableFields: {}, | ||
| })); | ||
|
|
||
| containers.forEach((container, containerIndex) => { | ||
| for (const [label, field] of Object.entries(convertedFields)) { | ||
| const sampleElement = this.queryElement(container, field.selector); | ||
|
|
||
| if (sampleElement) { | ||
| const ancestor = this.findTableAncestor(sampleElement); | ||
| if (ancestor) { | ||
| containerFields[containerIndex].tableFields[label] = { | ||
| ...field, | ||
| tableContext: ancestor.type, | ||
| cellIndex: | ||
| ancestor.type === "TD" | ||
| ? this.getCellIndex(ancestor.element) | ||
| : -1, | ||
| }; | ||
| } else { | ||
| containerFields[containerIndex].nonTableFields[label] = field; | ||
| } | ||
| } else { | ||
| containerFields[containerIndex].nonTableFields[label] = field; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Extract table data | ||
| const tableData: ExtractedListData[] = []; | ||
| for ( | ||
| let containerIndex = 0; | ||
| containerIndex < containers.length; | ||
| containerIndex++ | ||
| ) { | ||
| const container = containers[containerIndex]; | ||
| const { tableFields } = containerFields[containerIndex]; | ||
|
|
||
| if (Object.keys(tableFields).length > 0) { | ||
| const firstField = Object.values(tableFields)[0]; | ||
| const firstElement = this.queryElement( | ||
| container, | ||
| firstField.selector | ||
| ); | ||
| let tableContext: Element | null = firstElement; | ||
|
|
||
| // Find the table context | ||
| while ( | ||
| tableContext && | ||
| tableContext.tagName !== "TABLE" && | ||
| tableContext !== container | ||
| ) { | ||
| if (tableContext.getRootNode() instanceof ShadowRoot) { | ||
| tableContext = (tableContext.getRootNode() as ShadowRoot).host; | ||
| continue; | ||
| } | ||
|
|
||
| if ( | ||
| tableContext.tagName === "IFRAME" || | ||
| tableContext.tagName === "FRAME" | ||
| ) { | ||
| try { | ||
| const frameElement = tableContext as | ||
| | HTMLIFrameElement | ||
| | HTMLFrameElement; | ||
| tableContext = frameElement.contentDocument?.body || null; | ||
| } catch (e) { | ||
| break; | ||
| } | ||
| } else { | ||
| tableContext = tableContext.parentElement; | ||
| } | ||
| } | ||
|
|
||
| if (tableContext) { | ||
| const rows: Element[] = []; | ||
| rows.push(...Array.from(tableContext.getElementsByTagName("TR"))); | ||
|
|
||
| if ( | ||
| tableContext.tagName === "IFRAME" || | ||
| tableContext.tagName === "FRAME" | ||
| ) { | ||
| try { | ||
| const frameElement = tableContext as | ||
| | HTMLIFrameElement | ||
| | HTMLFrameElement; | ||
| const frameDoc = | ||
| frameElement.contentDocument || | ||
| frameElement.contentWindow?.document; | ||
| if (frameDoc) { | ||
| rows.push(...Array.from(frameDoc.getElementsByTagName("TR"))); | ||
| } | ||
| } catch (e) { | ||
| console.warn( | ||
| `Cannot access ${tableContext.tagName.toLowerCase()} rows:`, | ||
| e | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const processedRows = this.filterRowsBasedOnTag(rows, tableFields); | ||
|
|
||
| for ( | ||
| let rowIndex = 0; | ||
| rowIndex < Math.min(processedRows.length, limit); | ||
| rowIndex++ | ||
| ) { | ||
| const record: ExtractedListData = {}; | ||
| const currentRow = processedRows[rowIndex]; | ||
|
|
||
| for (const [ | ||
| label, | ||
| { selector, attribute, cellIndex }, | ||
| ] of Object.entries(tableFields)) { | ||
| let element: Element | null = null; | ||
|
|
||
| if (cellIndex !== undefined && cellIndex >= 0) { | ||
| let td: Element | null = | ||
| currentRow.children[cellIndex] || null; | ||
|
|
||
| if (!td && currentRow.shadowRoot) { | ||
| const shadowCells = currentRow.shadowRoot.children; | ||
| if (shadowCells && shadowCells.length > cellIndex) { | ||
| td = shadowCells[cellIndex]; | ||
| } | ||
| } | ||
|
|
||
| if (td) { | ||
| element = this.queryElement(td, selector); | ||
|
|
||
| if ( | ||
| !element && | ||
| selector | ||
| .split(/(?:>>|:>>)/) | ||
| .pop() | ||
| ?.includes("td:nth-child") | ||
| ) { | ||
| element = td; | ||
| } | ||
|
|
||
| if (!element) { | ||
| const tagOnlySelector = selector.split(".")[0]; | ||
| element = this.queryElement(td, tagOnlySelector); | ||
| } | ||
|
|
||
| if (!element) { | ||
| let currentElement: Element | null = td; | ||
| while ( | ||
| currentElement && | ||
| currentElement.children.length > 0 | ||
| ) { | ||
| let foundContentChild = false; | ||
| for (const child of Array.from( | ||
| currentElement.children | ||
| )) { | ||
| if (this.extractValue(child, attribute)) { | ||
| currentElement = child; | ||
| foundContentChild = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!foundContentChild) break; | ||
| } | ||
| element = currentElement; | ||
| } | ||
| } | ||
| } else { | ||
| element = this.queryElement(currentRow, selector); | ||
| } | ||
|
|
||
| if (element) { | ||
| const value = this.extractValue(element, attribute); | ||
| if (value !== null && value !== "") { | ||
| record[label] = value; | ||
| console.log(`✅ Extracted ${label}:`, value); | ||
| } else { | ||
| console.warn( | ||
| `❌ No value for ${label} in row ${rowIndex + 1}` | ||
| ); | ||
| record[label] = ""; | ||
| } | ||
| } else { | ||
| console.warn( | ||
| `❌ Element not found for ${label} with selector:`, | ||
| selector | ||
| ); | ||
| record[label] = ""; | ||
| } | ||
| } | ||
|
|
||
| if (Object.values(record).some((value) => value !== "")) { | ||
| tableData.push(record); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract non-table data | ||
| const nonTableData: ExtractedListData[] = []; | ||
| for ( | ||
| let containerIndex = 0; | ||
| containerIndex < containers.length; | ||
| containerIndex++ | ||
| ) { | ||
| if (nonTableData.length >= limit) break; | ||
|
|
||
| const container = containers[containerIndex]; | ||
| const { nonTableFields } = containerFields[containerIndex]; | ||
|
|
||
| if (Object.keys(nonTableFields).length > 0) { | ||
| const record: ExtractedListData = {}; | ||
|
|
||
| for (const [label, { selector, attribute }] of Object.entries( | ||
| nonTableFields | ||
| )) { | ||
| const relativeSelector = selector.split(/(?:>>|:>>)/).slice(-1)[0]; | ||
| const element = this.queryElement(container, relativeSelector); | ||
|
|
||
| if (element) { | ||
| const value = this.extractValue(element, attribute); | ||
| if (value !== null && value !== "") { | ||
| record[label] = value; | ||
| console.log(`✅ Extracted ${label}:`, value); | ||
| } else { | ||
| console.warn( | ||
| `❌ No value for ${label} in container ${containerIndex + 1}` | ||
| ); | ||
| record[label] = ""; | ||
| } | ||
| } else { | ||
| console.warn( | ||
| `❌ Element not found for ${label} with selector:`, | ||
| selector | ||
| ); | ||
| record[label] = ""; | ||
| } | ||
| } | ||
|
|
||
| if (Object.values(record).some((value) => value !== "")) { | ||
| nonTableData.push(record); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Combine and limit results | ||
| const extractedData = [...tableData, ...nonTableData].slice(0, limit); | ||
|
|
||
| console.log("🎉 Client extraction complete:", { | ||
| totalRecords: extractedData.length, | ||
| tableRecords: tableData.length, | ||
| nonTableRecords: nonTableData.length, | ||
| data: extractedData, | ||
| }); | ||
|
|
||
| return extractedData; | ||
| } catch (error) { | ||
| console.error("Error in client-side extractListData:", error); | ||
| return []; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor extractListData method to reduce complexity.
This method is nearly 300 lines long and handles multiple responsibilities, making it difficult to maintain and test. Consider breaking it down into smaller, focused methods.
public extractListData = (
iframeDocument: Document,
listSelector: string,
fields: any,
limit: number = 5
): ExtractedListData[] => {
try {
const convertedFields = this.convertFields(fields);
const containers = this.findContainers(iframeDocument, listSelector, limit);
if (containers.length === 0) {
console.warn("No containers found for listSelector:", listSelector);
return [];
}
const containerFields = this.analyzeContainerFields(containers, convertedFields);
const tableData = this.extractTableData(containers, containerFields, limit);
const nonTableData = this.extractNonTableData(containers, containerFields, limit);
return this.combineAndLimitResults(tableData, nonTableData, limit);
} catch (error) {
console.error("Error in client-side extractListData:", error);
return [];
}
};
private findContainers(
iframeDocument: Document,
listSelector: string,
limit: number
): Element[] {
let containers = this.queryElementAll(iframeDocument, listSelector);
if (limit > 1 && containers.length === 1) {
containers = this.enhanceContainerDiscovery(containers[0], iframeDocument, listSelector);
}
console.log("📦 Found containers:", containers.length);
return containers;
}
private analyzeContainerFields(
containers: Element[],
convertedFields: Record<string, { selector: string; attribute: string }>
): ContainerFields[] {
return containers.map((container) => {
const fields: ContainerFields = { tableFields: {}, nonTableFields: {} };
for (const [label, field] of Object.entries(convertedFields)) {
this.categorizeField(container, label, field, fields);
}
return fields;
});
}
// ... additional helper methodsThis refactoring would improve:
- Readability and maintainability
- Testability (each method can be tested independently)
- Adherence to Single Responsibility Principle
- Code reusability
🤖 Prompt for AI Agents
In src/helpers/clientListExtractor.ts from lines 432 to 731, the extractListData
method is too long and handles multiple responsibilities, making it hard to
maintain and test. Refactor by splitting it into smaller focused methods such as
findContainers to handle container discovery, analyzeContainerFields to
categorize fields per container, extractTableData and extractNonTableData for
data extraction, and combineAndLimitResults to merge and limit the output. Each
helper method should encapsulate a distinct part of the logic, improving
readability, testability, and adherence to the Single Responsibility Principle.
| } else if (childSelectors && childSelectors.includes(selector)) { | ||
| shouldHighlight = true; | ||
| } else if (elementInfo?.isIframeContent && childSelectors) { | ||
| const isIframeChild = childSelectors.some( | ||
| (childSelector: string) => | ||
| selector.includes(":>>") && | ||
| childSelector | ||
| .split(":>>") | ||
| .some((part) => selector.includes(part.trim())) | ||
| ); | ||
| shouldHighlight = isIframeChild; | ||
| } else if (selector.includes(":>>") && hasValidChildSelectors) { | ||
| const selectorParts = selector | ||
| .split(":>>") | ||
| .map((part: string) => part.trim()); | ||
| const isValidMixedSelector = selectorParts.some((part: any) => | ||
| childSelectors!.some((childSelector) => | ||
| childSelector.includes(part) | ||
| ) | ||
| ); | ||
| } else if (elementInfo?.isShadowRoot && childSelectors) { | ||
| const isShadowChild = childSelectors.some( | ||
| (childSelector: string) => | ||
| selector.includes(">>") && | ||
| childSelector | ||
| .split(">>") | ||
| .some((part) => selector.includes(part.trim())) | ||
| ); | ||
| } else if (selector.includes(">>") && hasValidChildSelectors) { | ||
| const selectorParts = selector | ||
| .split(">>") | ||
| .map((part: string) => part.trim()); | ||
| const isValidMixedSelector = selectorParts.some((part: any) => | ||
| childSelectors!.some((childSelector) => | ||
| childSelector.includes(part) | ||
| ) | ||
| ); | ||
| } else { | ||
| shouldHighlight = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate shadow DOM handling logic and use optional chaining.
The shadow DOM handling logic is duplicated. Additionally, the static analysis tool correctly identified that optional chaining would improve safety at line 247.
- } else if (elementInfo?.isShadowRoot && childSelectors) {
- const isShadowChild = childSelectors.some(
- (childSelector: string) =>
- selector.includes(">>") &&
- childSelector
- .split(">>")
- .some((part) => selector.includes(part.trim()))
- );
- } else if (selector.includes(">>") && hasValidChildSelectors) {
- const selectorParts = selector
- .split(">>")
- .map((part: string) => part.trim());
- const isValidMixedSelector = selectorParts.some((part: any) =>
- childSelectors!.some((childSelector) =>
- childSelector.includes(part)
- )
- );
+ } else if (elementInfo?.isShadowRoot && childSelectors) {
+ const isShadowChild = childSelectors.some(
+ (childSelector: string) =>
+ selector.includes(">>") &&
+ childSelector
+ .split(">>")
+ .some((part) => selector.includes(part.trim()))
+ );
+ shouldHighlight = isShadowChild;
+ } else if (selector.includes(">>") && hasValidChildSelectors) {
+ const selectorParts = selector
+ .split(">>")
+ .map((part: string) => part.trim());
+ const isValidMixedSelector = selectorParts.some((part: any) =>
+ childSelectors?.some((childSelector) =>
+ childSelector.includes(part)
+ )
+ );
+ shouldHighlight = isValidMixedSelector;📝 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.
| } else if (childSelectors && childSelectors.includes(selector)) { | |
| shouldHighlight = true; | |
| } else if (elementInfo?.isIframeContent && childSelectors) { | |
| const isIframeChild = childSelectors.some( | |
| (childSelector: string) => | |
| selector.includes(":>>") && | |
| childSelector | |
| .split(":>>") | |
| .some((part) => selector.includes(part.trim())) | |
| ); | |
| shouldHighlight = isIframeChild; | |
| } else if (selector.includes(":>>") && hasValidChildSelectors) { | |
| const selectorParts = selector | |
| .split(":>>") | |
| .map((part: string) => part.trim()); | |
| const isValidMixedSelector = selectorParts.some((part: any) => | |
| childSelectors!.some((childSelector) => | |
| childSelector.includes(part) | |
| ) | |
| ); | |
| } else if (elementInfo?.isShadowRoot && childSelectors) { | |
| const isShadowChild = childSelectors.some( | |
| (childSelector: string) => | |
| selector.includes(">>") && | |
| childSelector | |
| .split(">>") | |
| .some((part) => selector.includes(part.trim())) | |
| ); | |
| } else if (selector.includes(">>") && hasValidChildSelectors) { | |
| const selectorParts = selector | |
| .split(">>") | |
| .map((part: string) => part.trim()); | |
| const isValidMixedSelector = selectorParts.some((part: any) => | |
| childSelectors!.some((childSelector) => | |
| childSelector.includes(part) | |
| ) | |
| ); | |
| } else { | |
| shouldHighlight = false; | |
| } else if (childSelectors && childSelectors.includes(selector)) { | |
| shouldHighlight = true; | |
| } else if (elementInfo?.isIframeContent && childSelectors) { | |
| const isIframeChild = childSelectors.some( | |
| (childSelector: string) => | |
| selector.includes(":>>") && | |
| childSelector | |
| .split(":>>") | |
| .some((part) => selector.includes(part.trim())) | |
| ); | |
| shouldHighlight = isIframeChild; | |
| } else if (selector.includes(":>>") && hasValidChildSelectors) { | |
| const selectorParts = selector | |
| .split(":>>") | |
| .map((part: string) => part.trim()); | |
| const isValidMixedSelector = selectorParts.some((part: any) => | |
| childSelectors!.some((childSelector) => | |
| childSelector.includes(part) | |
| ) | |
| ); | |
| } else if (elementInfo?.isShadowRoot && childSelectors) { | |
| const isShadowChild = childSelectors.some( | |
| (childSelector: string) => | |
| selector.includes(">>") && | |
| childSelector | |
| .split(">>") | |
| .some((part) => selector.includes(part.trim())) | |
| ); | |
| shouldHighlight = isShadowChild; | |
| } else if (selector.includes(">>") && hasValidChildSelectors) { | |
| const selectorParts = selector | |
| .split(">>") | |
| .map((part: string) => part.trim()); | |
| const isValidMixedSelector = selectorParts.some((part: any) => | |
| childSelectors?.some((childSelector) => | |
| childSelector.includes(part) | |
| ) | |
| ); | |
| shouldHighlight = isValidMixedSelector; | |
| } else { | |
| shouldHighlight = false; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 247-247: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In src/components/recorder/DOMBrowserRenderer.tsx between lines 247 and 285, the
shadow DOM handling logic is duplicated and optional chaining is not used
consistently. Remove the redundant shadow DOM check block and consolidate the
logic into a single block. Also, update the code to use optional chaining (?.)
when accessing nested properties like elementInfo and childSelectors to improve
safety and prevent runtime errors.
| public onClick = async (coordinates: Coordinates, page: Page) => { | ||
| let where: WhereWhatPair["where"] = { url: this.getBestUrl(page.url()) }; | ||
| const selector = await this.generateSelector(page, coordinates, ActionType.Click); | ||
| console.log("COOORDINATES: ", coordinates); |
There was a problem hiding this comment.
Remove debug console.log statement.
Debug console.log statements should not be committed to production code. Use the logger utility instead if this information needs to be logged.
- console.log("COOORDINATES: ", coordinates);
+ logger.log('debug', `Click coordinates: ${JSON.stringify(coordinates)}`);📝 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.
| console.log("COOORDINATES: ", coordinates); | |
| logger.log('debug', `Click coordinates: ${JSON.stringify(coordinates)}`); |
🤖 Prompt for AI Agents
In server/src/workflow-management/classes/Generator.ts at line 465, remove the
debug console.log statement that prints the coordinates. If logging this
information is necessary, replace the console.log with a call to the appropriate
logger utility to ensure consistent and configurable logging in production.
| private setupScrollEventListener(): void { | ||
| this.socket.on( | ||
| "dom:scroll", | ||
| async (data: { deltaX: number; deltaY: number }) => { | ||
| if (!this.isDOMStreamingActive || !this.currentPage) return; | ||
|
|
||
| try { | ||
| logger.debug( | ||
| `Received scroll event: deltaX=${data.deltaX}, deltaY=${data.deltaY}` | ||
| ); | ||
|
|
||
| await this.currentPage.mouse.wheel(data.deltaX, data.deltaY); | ||
|
|
||
| const scrollInfo = await this.currentPage.evaluate(() => ({ | ||
| x: window.scrollX, | ||
| y: window.scrollY, | ||
| maxX: Math.max( | ||
| 0, | ||
| document.documentElement.scrollWidth - window.innerWidth | ||
| ), | ||
| maxY: Math.max( | ||
| 0, | ||
| document.documentElement.scrollHeight - window.innerHeight | ||
| ), | ||
| documentHeight: document.documentElement.scrollHeight, | ||
| viewportHeight: window.innerHeight, | ||
| })); | ||
|
|
||
| const scrollDelta = | ||
| Math.abs(scrollInfo.y - this.lastScrollPosition.y) + | ||
| Math.abs(scrollInfo.x - this.lastScrollPosition.x); | ||
|
|
||
| logger.debug( | ||
| `Scroll delta: ${scrollDelta}, threshold: ${this.scrollThreshold}` | ||
| ); | ||
|
|
||
| if (scrollDelta > this.scrollThreshold) { | ||
| this.lastScrollPosition = { x: scrollInfo.x, y: scrollInfo.y }; | ||
| this.isScrollTriggeredSnapshot = true; | ||
|
|
||
| if (this.snapshotDebounceTimeout) { | ||
| clearTimeout(this.snapshotDebounceTimeout); | ||
| } | ||
|
|
||
| this.snapshotDebounceTimeout = setTimeout(async () => { | ||
| logger.info( | ||
| `Triggering snapshot due to scroll. Position: ${scrollInfo.y}/${scrollInfo.maxY}` | ||
| ); | ||
|
|
||
| await this.makeAndEmitDOMSnapshot(); | ||
| }, 300); | ||
| } | ||
| } catch (error) { | ||
| logger.error("Error handling scroll event:", error); | ||
| } | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove scroll event listener when DOM streaming stops
The scroll event listener added in setupScrollEventListener is not removed when DOM streaming stops, which could cause memory leaks or unexpected behavior.
Track and remove the event listener:
private setupScrollEventListener(): void {
+ const scrollHandler = async (data: { deltaX: number; deltaY: number }) => {
+ if (!this.isDOMStreamingActive || !this.currentPage) return;
+ // ... existing handler code
+ };
+
+ this.socket.on("dom:scroll", scrollHandler);
+
+ // Store reference for cleanup
+ this.domScrollHandler = scrollHandler;
}
// In stopDOM method:
+if (this.domScrollHandler) {
+ this.socket.off("dom:scroll", this.domScrollHandler);
+ this.domScrollHandler = null;
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/src/browser-management/classes/RemoteBrowser.ts around lines 1027 to
1084, the scroll event listener added in setupScrollEventListener is not removed
when DOM streaming stops, risking memory leaks. To fix this, store the scroll
event handler function as a class property so it can be referenced later, then
add a method to remove this listener from the socket when DOM streaming stops by
calling socket.off with the stored handler. Ensure this cleanup method is
invoked appropriately when streaming ends.
|
|
||
| // Clean up cache periodically | ||
| setInterval(() => { | ||
| this.cleanupResourceCache(); | ||
| }, 60000); | ||
| } catch (error) { | ||
| logger.error("Failed to initialize enhanced network monitoring:", error); | ||
| this.isNetworkMonitoringActive = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear cleanup interval to prevent memory leak
The cleanup interval created in initializeNetworkMonitoring is not cleared when network monitoring stops, which could lead to a memory leak.
Store and clear the interval:
+private cleanupIntervalId: NodeJS.Timeout | null = null;
+
// In initializeNetworkMonitoring:
-setInterval(() => {
+this.cleanupIntervalId = setInterval(() => {
this.cleanupResourceCache();
}, 60000);
// In stopNetworkMonitoring:
+if (this.cleanupIntervalId) {
+ clearInterval(this.cleanupIntervalId);
+ this.cleanupIntervalId = null;
+}🤖 Prompt for AI Agents
In server/src/browser-management/classes/RemoteBrowser.ts around lines 922 to
931, the setInterval for cleanupResourceCache is not cleared when network
monitoring stops, causing a potential memory leak. Fix this by storing the
interval ID returned by setInterval in a class property and clear this interval
using clearInterval when network monitoring is stopped or disabled.
| private processCSS( | ||
| cssContent: string, | ||
| cssUrl: string, | ||
| baseUrl: string, | ||
| resources?: any | ||
| ): string { | ||
| try { | ||
| let processedContent = cssContent; | ||
|
|
||
| logger.debug(`Processing CSS from: ${cssUrl}`); | ||
|
|
||
| // Process @font-face declarations and collect font resources | ||
| processedContent = processedContent.replace( | ||
| /@font-face\s*\{([^}]*)\}/gi, | ||
| (fontFaceMatch, fontFaceContent) => { | ||
| let newFontFaceContent = fontFaceContent; | ||
|
|
||
| logger.debug( | ||
| `Processing @font-face block: ${fontFaceContent.substring( | ||
| 0, | ||
| 100 | ||
| )}...` | ||
| ); | ||
|
|
||
| newFontFaceContent = newFontFaceContent.replace( | ||
| /src\s*:\s*([^;}]+)[;}]/gi, | ||
| (srcMatch: any, srcValue: any) => { | ||
| let newSrcValue = srcValue; | ||
|
|
||
| newSrcValue = newSrcValue.replace( | ||
| /url\s*\(\s*['"]?([^'")]+)['"]?\s*\)(\s*format\s*\(\s*['"]?[^'")]*['"]?\s*\))?/gi, | ||
| (urlMatch: any, url: string, formatPart: any) => { | ||
| const originalUrl = url.trim(); | ||
|
|
||
| logger.debug(`Found font URL in @font-face: ${originalUrl}`); | ||
|
|
||
| if ( | ||
| originalUrl.startsWith("data:") || | ||
| originalUrl.startsWith("blob:") | ||
| ) { | ||
| return urlMatch; | ||
| } | ||
|
|
||
| try { | ||
| let absoluteUrl: string; | ||
| try { | ||
| absoluteUrl = new URL(originalUrl).href; | ||
| } catch (e) { | ||
| absoluteUrl = new URL(originalUrl, cssUrl || baseUrl) | ||
| .href; | ||
| } | ||
|
|
||
| const cachedResource = | ||
| this.networkResourceCache.get(absoluteUrl); | ||
| if (cachedResource && resources) { | ||
| const dataUrl = cachedResource.base64Encoded | ||
| ? `data:${cachedResource.mimeType};base64,${cachedResource.content}` | ||
| : `data:${cachedResource.mimeType};base64,${Buffer.from( | ||
| cachedResource.content, | ||
| "utf-8" | ||
| ).toString("base64")}`; | ||
|
|
||
| resources.fonts.push({ | ||
| url: absoluteUrl, | ||
| dataUrl, | ||
| format: originalUrl.split(".").pop()?.split("?")[0], | ||
| }); | ||
| } | ||
|
|
||
| // Keep original URL in CSS | ||
| return urlMatch; | ||
| } catch (e) { | ||
| logger.warn( | ||
| "Failed to process font URL in @font-face:", | ||
| originalUrl, | ||
| e | ||
| ); | ||
| return urlMatch; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| return `src: ${newSrcValue};`; | ||
| } | ||
| ); | ||
|
|
||
| return `@font-face {${newFontFaceContent}}`; | ||
| } | ||
| ); | ||
|
|
||
| // Process other url() references and collect resources | ||
| processedContent = processedContent.replace( | ||
| /url\s*\(\s*['"]?([^'")]+)['"]?\s*\)/gi, | ||
| (match, url) => { | ||
| const originalUrl = url.trim(); | ||
|
|
||
| if ( | ||
| originalUrl.startsWith("data:") || | ||
| originalUrl.startsWith("blob:") | ||
| ) { | ||
| return match; | ||
| } | ||
|
|
||
| try { | ||
| let absoluteUrl: string; | ||
| try { | ||
| absoluteUrl = new URL(originalUrl).href; | ||
| } catch (e) { | ||
| absoluteUrl = new URL(originalUrl, cssUrl || baseUrl).href; | ||
| } | ||
|
|
||
| const cachedResource = this.networkResourceCache.get(absoluteUrl); | ||
| if (cachedResource && resources) { | ||
| const lowerMimeType = cachedResource.mimeType.toLowerCase(); | ||
|
|
||
| if (lowerMimeType.includes("image/")) { | ||
| const dataUrl = cachedResource.base64Encoded | ||
| ? `data:${cachedResource.mimeType};base64,${cachedResource.content}` | ||
| : `data:${cachedResource.mimeType};base64,${Buffer.from( | ||
| cachedResource.content, | ||
| "utf-8" | ||
| ).toString("base64")}`; | ||
|
|
||
| resources.images.push({ | ||
| src: absoluteUrl, | ||
| dataUrl, | ||
| alt: "", | ||
| }); | ||
| } else if ( | ||
| lowerMimeType.includes("font/") || | ||
| lowerMimeType.includes("application/font") | ||
| ) { | ||
| const dataUrl = cachedResource.base64Encoded | ||
| ? `data:${cachedResource.mimeType};base64,${cachedResource.content}` | ||
| : `data:${cachedResource.mimeType};base64,${Buffer.from( | ||
| cachedResource.content, | ||
| "utf-8" | ||
| ).toString("base64")}`; | ||
|
|
||
| resources.fonts.push({ | ||
| url: absoluteUrl, | ||
| dataUrl, | ||
| format: originalUrl.split(".").pop()?.split("?")[0], | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Keep original URL in CSS | ||
| return match; | ||
| } catch (e) { | ||
| logger.warn(`Failed to process CSS URL: ${originalUrl}`, e); | ||
| return match; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| // Process @import statements and collect stylesheets | ||
| processedContent = processedContent.replace( | ||
| /@import\s+(?:url\s*\(\s*)?['"]?([^'")]+)['"]?\s*\)?([^;]*);?/gi, | ||
| (match, url, mediaQuery) => { | ||
| const originalUrl = url.trim(); | ||
|
|
||
| if ( | ||
| originalUrl.startsWith("data:") || | ||
| originalUrl.startsWith("blob:") | ||
| ) { | ||
| return match; | ||
| } | ||
|
|
||
| try { | ||
| let absoluteUrl: string; | ||
| try { | ||
| absoluteUrl = new URL(originalUrl).href; | ||
| } catch (e) { | ||
| absoluteUrl = new URL(originalUrl, cssUrl || baseUrl).href; | ||
| } | ||
|
|
||
| const cachedResource = this.networkResourceCache.get(absoluteUrl); | ||
| if ( | ||
| cachedResource && | ||
| resources && | ||
| cachedResource.mimeType.includes("css") | ||
| ) { | ||
| const content = cachedResource.base64Encoded | ||
| ? Buffer.from(cachedResource.content, "base64").toString( | ||
| "utf-8" | ||
| ) | ||
| : cachedResource.content; | ||
|
|
||
| resources.stylesheets.push({ | ||
| href: absoluteUrl, | ||
| content: this.processCSS( | ||
| content, | ||
| absoluteUrl, | ||
| baseUrl, | ||
| resources | ||
| ), | ||
| media: mediaQuery ? mediaQuery.trim() : "all", | ||
| }); | ||
| } | ||
|
|
||
| // Keep original @import | ||
| return match; | ||
| } catch (e) { | ||
| logger.warn(`Failed to process CSS @import: ${originalUrl}`, e); | ||
| return match; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| logger.debug(`CSS processing completed for: ${cssUrl}`); | ||
| return processedContent; | ||
| } catch (error) { | ||
| logger.error("Failed to process CSS content:", error); | ||
| return cssContent; // Return original content if processing fails | ||
| } | ||
| } |
There was a problem hiding this comment.
Add protection against circular CSS imports
The CSS processing method handles imports recursively but lacks protection against circular references. This could lead to infinite recursion if CSS files import each other circularly.
Consider tracking processed URLs to prevent circular imports:
private processCSS(
cssContent: string,
cssUrl: string,
baseUrl: string,
- resources?: any
+ resources?: any,
+ processedUrls: Set<string> = new Set()
): string {
try {
+ // Prevent circular imports
+ if (processedUrls.has(cssUrl)) {
+ logger.warn(`Circular import detected for CSS: ${cssUrl}`);
+ return cssContent;
+ }
+ processedUrls.add(cssUrl);
+
let processedContent = cssContent;And update the recursive call:
content: this.processCSS(
content,
absoluteUrl,
baseUrl,
- resources
+ resources,
+ processedUrls
),🤖 Prompt for AI Agents
In server/src/browser-management/classes/RemoteBrowser.ts between lines 257 and
473, the processCSS method recursively processes @import CSS statements but does
not track which URLs have already been processed, risking infinite recursion
from circular imports. To fix this, add a Set or similar data structure
parameter to track processed CSS URLs and pass it along in recursive calls.
Before processing an @import URL, check if it is already in the set; if so, skip
processing to prevent recursion. Initialize this tracking set on the initial
call and update the recursive call to processCSS to include it.
amhsirak
left a comment
amhsirak
left a comment
There was a problem hiding this comment.
@RohitR311 same changes as cloud - yellow border & cursor pointer
What this PR does?
Revamps the browser recorder to render the entire DOM instead of rendering screenshots.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores