fix: browser pool circular dependency #536
Conversation
WalkthroughThis change removes the copying of the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Controller
participant RB as RemoteBrowser
participant WG as WorkflowGenerator
participant S as Socket
C->>RB: new RemoteBrowser(socket, userId, poolId)
RB->>WG: new WorkflowGenerator(socket, poolId)
WG->>WG: Store poolId & listen for "decision" events
WG->>S: Process decision using poolId
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/workflow-management/classes/Generator.ts (1)
153-170: Changed decision event handler to use poolIdThe event handler now checks for
this.poolIdrather than attempting to access a browser instance through a global pool reference. The previous code that caused the circular dependency is still present but commented out, which maintains the history and context of the change.Consider removing the commented-out code once you've verified the changes work as expected. While it's helpful during development and debugging, it may create confusion for future maintainers.
if (this.poolId) { - // const activeBrowser = browserPool.getRemoteBrowser(id); - // const currentPage = activeBrowser?.getCurrentPage(); if (!decision) { switch (actionType) { case 'customAction': // pair.where.selectors = [this.generatedData.lastUsedSelector]; pair.where.selectors = pair.where.selectors.filter( (selector: string) => selector !== this.generatedData.lastUsedSelector ); break; default: break; } } - // if (currentPage) { - // await this.addPairToWorkflowAndNotifyClient(pair, currentPage); - // } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile(0 hunks)server/Dockerfile(0 hunks)server/src/browser-management/classes/RemoteBrowser.ts(1 hunks)server/src/browser-management/controller.ts(2 hunks)server/src/workflow-management/classes/Generator.ts(2 hunks)
💤 Files with no reviewable changes (2)
- Dockerfile
- server/Dockerfile
🧰 Additional context used
🧬 Code Graph Analysis (2)
server/src/browser-management/classes/RemoteBrowser.ts (2)
server/src/workflow-management/classes/Interpreter.ts (1)
WorkflowInterpreter(59-357)server/src/workflow-management/classes/Generator.ts (1)
WorkflowGenerator(54-1210)
server/src/browser-management/controller.ts (1)
server/src/browser-management/classes/RemoteBrowser.ts (1)
RemoteBrowser(51-904)
🔇 Additional comments (5)
server/src/browser-management/classes/RemoteBrowser.ts (1)
121-125: Constructor updated to include poolId parameterThe constructor now accepts an additional
poolIdparameter which is properly passed to theWorkflowGenerator. This change resolves the circular dependency issue mentioned in the PR objectives by providing the pool identifier directly to the generator instead of having the generator access it through a global reference.server/src/workflow-management/classes/Generator.ts (2)
72-72: Added poolId property to WorkflowGeneratorA new private property has been added to store the pool ID. This is appropriate for maintaining the state passed from the constructor.
80-82: Updated constructor to accept poolId parameterThe constructor now accepts and stores the
poolIdparameter, which is consistent with the changes made in theRemoteBrowserclass. This is the right approach for breaking the circular dependency.server/src/browser-management/controller.ts (2)
35-35: Updated RemoteBrowser instantiation to include pool IDThe
RemoteBrowserinstantiation now includes theidparameter, which corresponds to thepoolIdparameter in theRemoteBrowserconstructor. This change is consistent with the updated constructor signature.
65-65: Updated RemoteBrowser instantiation in createRemoteBrowserForRunThe
RemoteBrowserinstantiation in thecreateRemoteBrowserForRunfunction now includes theidparameter, maintaining consistency with the constructor changes. This ensures that all browser instances are properly initialized with their pool ID.
amhsirak
added
Type: Bug
closes #533
Summary by CodeRabbit
Summary by CodeRabbit