Added getDomSnapshot feature by olesho · Pull Request #81 · BrowserOperator/browser-operator-core

olesho · 2025-12-10T01:19:08Z

Summary by CodeRabbit

New Features
- Added DOM snapshot capture functionality with configurable options for computed styles, DOM rectangle measurements, and paint order data (requires Chrome 74+ with Chrome DevTools Protocol enabled).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

chatgpt-codex-connector · 2025-12-10T01:19:13Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

claude · 2025-12-10T01:19:44Z

Claude finished @olesho's task —— View job

Code Review for getDomSnapshot Feature ✅

I've completed the comprehensive review of the new getDomSnapshot feature implementation.

Todo List:

Read repository guidelines (CLAUDE.md)
Analyze modified files and implementation
Review code quality and best practices
Check for potential bugs and issues
Evaluate performance considerations
Assess security concerns
Review test coverage
Provide comprehensive feedback

Summary

The PR adds a new /page/dom-snapshot API endpoint that leverages Chrome DevTools Protocol's DOMSnapshot.captureSnapshot to provide structured DOM data capture functionality.

✅ Code Quality & Best Practices

Excellent implementation overall:

Consistent naming: Functions and variables follow established conventions (getDOMSnapshot, captureDOMSnapshot)
Parameter validation: Proper validation of required fields (clientId, tabId) in api-server.js:344-350
Error handling: Comprehensive error handling with detailed logging and informative error messages
Code organization: Clean separation between API layer (api-server.js) and CDP implementation (BrowserAgentServer.js)
Documentation: Well-documented JSDoc comments explaining parameters and return values

✅ Security Assessment

No security concerns identified:

Input validation prevents injection attacks through proper parameter checking
Uses established CDP communication patterns already proven secure in the codebase
No sensitive data exposure (DOM content is expected to be captured)
Follows existing authentication/authorization patterns with baseClientId validation

⚠️ Potential Issues & Improvements

1. Error Handling Enhancement (BrowserAgentServer.js:1364-1366)

// Current implementation
if (error.message && error.message.includes('was not found')) {
  throw new Error('DOMSnapshot domain not available. Requires Chrome 74+ with CDP enabled.');
}

Recommendation: Make error detection more robust:

if (error.message && (error.message.includes('was not found') || error.message.includes('not supported'))) {
  throw new Error('DOMSnapshot domain not available. Requires Chrome 74+ with CDP enabled.');
}

2. API Response Structure Inconsistency (api-server.js:369-376)

The response structure differs slightly from other endpoints:

getPageContent() returns {clientId, tabId, content, format, length, timestamp, frameCount?}
getDOMSnapshot() returns {clientId, tabId, snapshot, format, timestamp}

Recommendation: Consider adding size information for consistency:

return {
  clientId: baseClientId,
  tabId: result.tabId,
  snapshot: result.snapshot,
  format: 'dom-snapshot',
  nodeCount: result.snapshot.documents?.reduce((sum, doc) => sum + (doc.nodes?.nodeType?.length || 0), 0),
  timestamp: Date.now()
};

🚀 Performance Considerations

Generally good performance design:

✅ Uses efficient CDP commands directly rather than JavaScript evaluation
✅ Includes configurable options (computedStyles, includeDOMRects, includePaintOrder) for performance tuning
✅ Proper logging for monitoring capture performance

Potential concerns:

Large DOM trees could result in substantial memory usage
No built-in size limits or warnings for extremely large snapshots

Recommendation: Consider adding optional size validation:

// In BrowserAgentServer.js captureDOMSnapshot method
const totalNodes = result.documents.reduce((sum, doc) => sum + (doc.nodes?.nodeType?.length || 0), 0);
if (totalNodes > 50000) {
  logger.warn('Large DOM snapshot captured', { tabId, totalNodes });
}

📋 Test Coverage Assessment

Missing test coverage - This is the primary concern:

No unit tests for the new API endpoint
No integration tests for CDP snapshot functionality
No error scenario testing

Recommended tests to add:

Unit tests for getDOMSnapshot() API method
Integration tests for captureDOMSnapshot() CDP functionality
Error handling tests for invalid parameters and CDP failures
Performance tests for large DOM structures

🎯 Overall Assessment

Excellent implementation with high code quality and proper architectural patterns. The feature is production-ready with minor improvements recommended.

Strengths:

Clean, well-documented code following project conventions
Robust error handling and logging
Proper validation and security practices
Configurable options for different use cases

Areas for improvement:

Add comprehensive test coverage
Minor API consistency enhancements
Enhanced error detection robustness

Recommendation: ✅ Approve with minor suggestions - The core implementation is solid and ready for production use.

coderabbitai · 2025-12-10T01:19:52Z

Walkthrough

The changes introduce DOM snapshot capture functionality by adding a POST API endpoint /page/dom-snapshot that accepts client and tab identifiers with optional parameters, validates inputs, and delegates to a new BrowserAgentServer method that communicates with Chrome DevTools Protocol to capture and return DOM snapshot data.

Changes

Cohort / File(s)	Summary
DOM Snapshot Feature `agent-server/nodejs/src/api-server.js`, `agent-server/nodejs/src/lib/BrowserAgentServer.js`	Added `/page/dom-snapshot` POST route with `getDOMSnapshot(payload)` method in APIServer that validates clientId and tabId, accepts optional computedStyles, includeDOMRects, and includePaintOrder parameters, and delegates to new `captureDOMSnapshot(tabId, options)` method in BrowserAgentServer. The BrowserAgentServer method sends a CDP DOMSnapshot.captureSnapshot command, validates response structure, logs snapshot details, and includes specialized error handling for unavailable DOMSnapshot domain (Chrome 74+ requirement).

Sequence Diagram

sequenceDiagram
    participant Client
    participant APIServer
    participant BrowserAgentServer as BrowserAgent<br/>Server
    participant CDP as Chrome DevTools<br/>Protocol

    Client->>APIServer: POST /page/dom-snapshot<br/>(clientId, tabId, options)
    activate APIServer
    
    Note over APIServer: Validate payload<br/>(clientId, tabId required)
    Note over APIServer: Derive baseClientId<br/>Log invocation
    
    APIServer->>BrowserAgentServer: captureDOMSnapshot(tabId, options)
    activate BrowserAgentServer
    
    Note over BrowserAgentServer: Log input details<br/>Set option defaults
    
    BrowserAgentServer->>CDP: DOMSnapshot.captureSnapshot
    activate CDP
    CDP-->>BrowserAgentServer: snapshot response
    deactivate CDP
    
    Note over BrowserAgentServer: Validate response structure<br/>(documents, strings present)
    Note over BrowserAgentServer: Log snapshot details<br/>(node/doc/string counts)
    
    BrowserAgentServer-->>APIServer: {tabId, snapshot}
    deactivate BrowserAgentServer
    
    Note over APIServer: Construct response<br/>format: 'dom-snapshot'<br/>Add timestamp
    
    APIServer-->>Client: {clientId, tabId, snapshot,<br/>format, timestamp}
    deactivate APIServer
    
    Note over CDP: Error: Domain unavailable<br/>→ Chrome 74+ required

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Validation logic: Verify all required and optional parameters are correctly validated and defaulted in getDOMSnapshot
CDP integration: Confirm captureDOMSnapshot properly constructs and sends the DOMSnapshot.captureSnapshot command with correct parameters
Error handling: Review specialized error detection for DOMSnapshot domain unavailability and ensure appropriate error messages are thrown
Response structure: Validate that both methods return correctly structured objects with expected fields (clientId, tabId, snapshot, format, timestamp)

Suggested reviewers

tysonthomas9

Poem

🐰 A snapshot of the DOM so fine,
Through Chrome's own eyes, the trees align,
With rects and styles, a feature new,
The page laid bare for all to view!
Hop along, the browser's truth!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Added getDomSnapshot feature' accurately reflects the main changes: two new methods (getDOMSnapshot and captureDOMSnapshot) were added to enable DOM snapshot capture functionality.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feat/get-dom-snapshot

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 21edf39 and a421585.

📒 Files selected for processing (2)

agent-server/nodejs/src/api-server.js (2 hunks)
agent-server/nodejs/src/lib/BrowserAgentServer.js (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

agent-server/nodejs/src/api-server.js

📄 CodeRabbit inference engine (agent-server/nodejs/CLAUDE.md)

agent-server/nodejs/src/api-server.js: Expose REST endpoint POST /v1/responses that accepts task input, URL, timeout, and model configuration, and returns OpenAI-compatible response with metadata
Use formatResponse() method to convert agent responses to OpenAI-compatible format and include metadata with clientId and tabId for screenshot capture
Model configuration must use canonical nested format with main_model, mini_model, and nano_model tiers, each containing provider, model, and api_key fields
POST /page/screenshot endpoint must accept clientId and tabId, use CDP Page.captureScreenshot, and return base64-encoded PNG with metadata and timestamp
POST /page/content endpoint must accept clientId, tabId, format (html or text), and includeIframes parameters; recursively capture iframe content when includeIframes is true
POST /page/execute endpoint must accept clientId, tabId, expression, returnByValue, and awaitPromise; use CDP Runtime.evaluate and return result with type and value
Accept POST /v1/responses input as either string format (simple message) or conversation array format with role and content fields; enforce at least one user message and max 100 messages/10,000 characters per message

Files:

agent-server/nodejs/src/api-server.js

🧠 Learnings (7)

📓 Common learnings

Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/api-server.js : POST /page/screenshot endpoint must accept clientId and tabId, use CDP Page.captureScreenshot, and return base64-encoded PNG with metadata and timestamp

Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget

Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/api-server.js : POST /page/content endpoint must accept clientId, tabId, format (html or text), and includeIframes parameters; recursively capture iframe content when includeIframes is true