feat: inject snapshot bundle in browser context #672
Conversation
|
""" WalkthroughThe changes remove all network resource caching and monitoring logic from the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🔇 Additional comments (10)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
server/src/browser-management/classes/RemoteBrowser.ts (3)
278-293: Remove references to deleted networkResourceCacheThe
processCSSmethod still referencesthis.networkResourceCachewhich has been removed according to the changes. This will cause runtime errors.Since network resource caching has been removed, this code block should be updated to either:
- Remove the caching logic entirely
- Use an alternative approach for handling font resources
Consider removing or refactoring this entire block that depends on the removed cache.
495-609: Remove dead code methodsThe
shouldCacheResourceandcleanupResourceCachemethods are no longer used after the removal of network resource caching functionality.Remove these unused methods to keep the codebase clean:
- /** - * Check if a resource should be cached based on its MIME type and URL - * @private - */ - private shouldCacheResource(mimeType: string, url: string): boolean { - // ... entire method body - } - - /** - * Clean up old cached resources to prevent memory leaks - * @private - */ - private cleanupResourceCache(): void { - // ... entire method body - }
958-971: Clean up network monitoring initialization logicThe conditional blocks still check for
this.clientand have comments about initializing network monitoring, but theinitializeNetworkMonitoringmethod has been removed.Remove the dead code paths:
if (this.client) { - await this.initializeNetworkMonitoring(); - logger.info("Network monitoring initialized successfully"); + // CDP client initialized successfully }
🧹 Nitpick comments (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
772-784: Improve documentation for commented codeThe comment "DO NOT REMOVE THIS CODE - MIGHT BE NEEDED LATER" is too vague. Consider adding more context about when and why this code might be re-enabled.
Replace the comment with a more descriptive TODO:
- // DO NOT REMOVE THIS CODE - MIGHT BE NEEDED LATER + // TODO: Re-enable response-based DOM snapshot triggers when implementing + // dynamic content monitoring for SPAs. This was disabled to reduce overhead + // after removing network resource caching. + // Related issue: [add issue number if applicable]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/browser-management/classes/RemoteBrowser.ts(5 hunks)
🔇 Additional comments (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
1862-1865: LGTM! Good use of rrweb's built-in optionsThe change to use rrweb's built-in
inlineImagesandcollectFontsoptions is a good simplification that replaces the custom resource caching logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
230-275: Update processing stats to reflect simplified resource handling.The method now returns empty resource arrays but still calculates processing stats as if resources were processed. This is misleading for debugging and monitoring.
return { snapshot, resources, baseUrl, viewport, timestamp: Date.now(), processingStats: { discoveredResources: { - images: resources.images.length, - stylesheets: resources.stylesheets.length, - scripts: resources.scripts.length, - fonts: resources.fonts.length, - media: resources.media.length, + images: 0, + stylesheets: 0, + scripts: 0, + fonts: 0, + media: 0, }, cachedResources: { - stylesheets: resources.stylesheets.length, - images: resources.images.length, - fonts: resources.fonts.length, - scripts: resources.scripts.length, - media: resources.media.length, + stylesheets: 0, + images: 0, + fonts: 0, + scripts: 0, + media: 0, }, }, };
♻️ Duplicate comments (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
602-604: Critical initialization order bug still exists.This is the same bug identified in previous reviews. The code calls
addInitScriptonthis.currentPagebefore it's assigned, which will cause a TypeError.Apply the fix from the previous review:
-await this.currentPage.addInitScript({ path: './server/src/browser-management/classes/rrweb-bundle.js' }); - this.currentPage = await this.context.newPage(); + +await this.currentPage.addInitScript({ path: './server/src/browser-management/classes/rrweb-bundle.js' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/browser-management/classes/RemoteBrowser.ts(6 hunks)
🔇 Additional comments (3)
server/src/browser-management/classes/RemoteBrowser.ts (3)
438-450: LGTM! Response event listener appropriately disabled.The commenting out of the response event listener aligns with the removal of network monitoring functionality. The note to keep the code for potential future use is appropriate.
528-531: Excellent use of rrweb's built-in resource handling.The addition of
inlineImages: trueandcollectFonts: trueoptions correctly replaces the previous network resource caching approach with rrweb's native capabilities. This simplifies the codebase while maintaining functionality.
625-637: Network monitoring removal is consistent with the refactoring.The removal of
initializeNetworkMonitoring()calls is appropriate and consistent with the overall goal of simplifying the codebase by removing network resource caching functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
230-275: Implement or Remove Resource Processing for processRRWebSnapshotThe PR has stripped out all resource extraction, but client code still consumes
ProcessedSnapshot.resources. As a result, snapshots will never include stylesheets, fonts, images, scripts or media, breaking DOM rendering and styling. You need to either restore the resource‐processing logic inprocessRRWebSnapshotor update the UI to stop expecting these fields.Files requiring your attention:
- server/src/browser-management/classes/RemoteBrowser.ts
– theprocessRRWebSnapshotmethod andProcessedSnapshotinterface- src/components/recorder/DOMBrowserRenderer.tsx
– references tosnapshotData.resources.stylesheetsandresources.fonts- src/components/browser/BrowserWindow.tsx
– references tocurrentSnapshot?.resourcesPlease choose one of the following:
- Reintroduce resource collection (CSS, fonts, images, etc.) in
RemoteBrowser.processRRWebSnapshot, so downstream components continue working unchanged.- Remove all resource‐dependent code paths in the renderer components and adjust
ProcessedSnapshotaccordingly to only carry the raw snapshot.
♻️ Duplicate comments (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
201-203: Remove unused network request timeout fieldsThese fields were previously identified as unused and marked as addressed in commit eecfa43, but they still appear in the current code. Since the network monitoring logic has been removed, these fields serve no purpose.
- private networkRequestTimeout: NodeJS.Timeout | null = null; - private pendingNetworkRequests: string[] = []; - private readonly NETWORK_QUIET_PERIOD = 8000;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/browser-management/classes/RemoteBrowser.ts(6 hunks)
🔇 Additional comments (2)
server/src/browser-management/classes/RemoteBrowser.ts (2)
537-540: Approve the rrweb snapshot configurationThe addition of
inlineImages: trueandcollectFonts: trueoptions is appropriate for the new approach of relying on rrweb's built-in resource handling instead of custom network caching.
619-623: Confirm the initialization order fixThe order has been corrected to create the page first, then add the init script, which addresses the critical initialization bug identified in previous reviews.
| this.pendingNetworkRequests.push(url); | ||
|
|
||
| if (this.networkRequestTimeout) { | ||
| clearTimeout(this.networkRequestTimeout); | ||
| this.networkRequestTimeout = null; | ||
| } | ||
|
|
||
| logger.debug( | ||
| `Network request received: ${url}. Total pending: ${this.pendingNetworkRequests.length}` | ||
| ); | ||
|
|
||
| this.networkRequestTimeout = setTimeout(async () => { | ||
| logger.info( | ||
| `Network quiet period reached. Processing ${this.pendingNetworkRequests.length} requests` | ||
| ); | ||
|
|
||
| this.pendingNetworkRequests = []; | ||
| this.networkRequestTimeout = null; | ||
|
|
||
| await this.makeAndEmitDOMSnapshot(); | ||
| }, 800); | ||
| }, this.NETWORK_QUIET_PERIOD); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the new network quiet period implementation
The replacement of complex network monitoring with a simpler timeout-based approach is reasonable, but there are potential issues with the current implementation:
- The
pendingNetworkRequestsarray grows but is only cleared when the timeout fires - No bounds checking on the array size could lead to memory leaks
- The timeout resets on every request, which could delay snapshots indefinitely on busy sites
Consider adding safeguards:
this.pendingNetworkRequests.push(url);
+
+ // Prevent memory leaks by limiting queue size
+ if (this.pendingNetworkRequests.length > 100) {
+ this.pendingNetworkRequests = this.pendingNetworkRequests.slice(-50);
+ }
if (this.networkRequestTimeout) {
clearTimeout(this.networkRequestTimeout);
this.networkRequestTimeout = null;
}
logger.debug(
`Network request received: ${url}. Total pending: ${this.pendingNetworkRequests.length}`
);
+ // Force snapshot after maximum wait time regardless of ongoing requests
+ const maxWaitTime = Math.min(this.NETWORK_QUIET_PERIOD * 2, 16000);
this.networkRequestTimeout = setTimeout(async () => {
logger.info(
`Network quiet period reached. Processing ${this.pendingNetworkRequests.length} requests`
);
this.pendingNetworkRequests = [];
this.networkRequestTimeout = null;
await this.makeAndEmitDOMSnapshot();
- }, this.NETWORK_QUIET_PERIOD);
+ }, this.NETWORK_QUIET_PERIOD);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/src/browser-management/classes/RemoteBrowser.ts around lines 445 to
466, the current network quiet period implementation pushes URLs to
pendingNetworkRequests without bounds checking, which risks memory leaks, and
resets the timeout on every request, potentially delaying snapshots
indefinitely. To fix this, add a maximum size limit to pendingNetworkRequests
and discard or handle excess entries to prevent unbounded growth. Also, modify
the timeout logic so it does not reset indefinitely on continuous requests, for
example by allowing the timeout to run once or by implementing a maximum wait
time before forcing a snapshot.
closes: #671 #656
Summary by CodeRabbit
Summary by CodeRabbit