Fix PR review issues: selector fragility, error handling, deprecated APIs#99
Conversation
…ods, and unused imports Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const textarea = await this.page!.$('textarea[placeholder*="prompt"]'); | ||
| if (textarea) { | ||
| await this.page!.fill('textarea[placeholder*="prompt"]', params.prompt); | ||
| } else { | ||
| // For contenteditable elements, use type() or evaluate() | ||
| const contentEditable = await this.page!.$('[contenteditable="true"]'); | ||
| if (contentEditable) { | ||
| await this.page!.evaluate((el, text) => { | ||
| (el as HTMLElement).textContent = text; | ||
| }, contentEditable, params.prompt); |
There was a problem hiding this comment.
Trigger input events when filling contenteditable prompt
When no <textarea> exists, the prompt is inserted into the contenteditable element by running page.evaluate() and assigning textContent. This bypasses Playwright’s synthetic typing and does not fire input/change events. UIs that rely on those events (React, Angular, etc.) will keep their internal state empty, so pressing Run may submit a blank prompt even though the DOM shows the text. Consider using contentEditable.type() or dispatching an input event after the assignment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses 17 automated code review findings focused on improving selector robustness in Playwright automation, error handling transparency, API modernization, and code cleanup.
Key changes:
- Replaced ambiguous comma-separated Playwright selectors with explicit try-catch fallback chains to ensure predictable element selection order
- Enhanced error observability by including endpoint URLs in error messages and propagating token counting failures instead of silently returning zero
- Modernized deprecated
substr()tosubstring()and removed hardcoded user agent strings
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/browser/services/google-labs.ts | Replaced comma-separated selectors with explicit try-catch chains and added special character escaping |
| src/browser/services/ai-studio-ultra.ts | Fixed contenteditable element handling using evaluate() instead of fill() |
| src/browser/playwright-service-base.ts | Removed hardcoded Chrome 120 user agent to use Playwright defaults |
| packages/services/quantum/index.ts | Changed Bell state simulation from deterministic split to binomial sampling |
| packages/services/quantum/circuit-builder.ts | Removed unused QuantumGate import |
| packages/services/google-ai/gemini-service.ts | Changed countTokens() to throw errors instead of returning 0 |
| packages/core/protocols/mcp/index.ts | Removed unused MCPServerConfig import |
| packages/core/protocols/ap2/transaction-manager.ts | Removed unused PaymentAmount import |
| packages/core/protocols/ap2/mandate-manager.ts | Removed unused loop variable in cleanup method |
| packages/core/protocols/a2a/protocol-manager.ts | Replaced deprecated substr() with substring() |
| packages/core/protocols/a2a/communication.ts | Enhanced error logging with endpoint URL and removed unused loop variable |
| packages/core/protocols/a2a/agent-card.ts | Removed unused AgentEndpoint import |
| README.md | Clarified version output format in example |
| PROTOCOLS.md | Fixed protocol field to match URL scheme (http) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| const taskId = `task-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; | ||
| const taskId = `task-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; |
There was a problem hiding this comment.
The substring bounds are incorrect. The original substr(2, 9) extracts 9 characters starting at index 2, but substring(2, 11) extracts 9 characters (indices 2-10 inclusive). However, Math.random().toString(36) typically returns strings of varying lengths (around 11-13 characters including '0.' prefix). After removing '0.' (2 chars), the remaining string may be shorter than 9 characters, making substring(2, 11) potentially return fewer characters than intended. The correct replacement should be substring(2, 2+9) or simply substring(2) if variable length is acceptable.
| const taskId = `task-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; | |
| const taskId = `task-${Date.now()}-${Math.random().toString(36).substring(2, 2 + 9)}`; |
| // Select action type | ||
| await this.page!.click(`text="${step.action}"`); | ||
| // Select action type - escape special characters in selector | ||
| const escapedAction = step.action.replace(/["\\]/g, '\\$&'); |
There was a problem hiding this comment.
The regex escaping only handles double quotes and backslashes, but Playwright text selectors can be broken by other special characters. Consider using a more comprehensive escaping approach or use a different selector strategy (e.g., role-based selectors with accessible names) that's less fragile to special characters.
| // Try textarea first | ||
| const textarea = await this.page!.$('textarea[placeholder*="prompt"]'); | ||
| if (textarea) { | ||
| await this.page!.fill('textarea[placeholder*="prompt"]', params.prompt); |
There was a problem hiding this comment.
The selector textarea[placeholder*="prompt"] is executed twice: once in the $() call to check existence (line 146) and again in the fill() call (line 148). Store the element handle and use it directly, or use a single try-catch with fill() to avoid the redundant query.
| await this.page!.fill('textarea[placeholder*="prompt"]', params.prompt); | |
| await textarea.fill(params.prompt); |
| // Bell state results - simulate quantum noise with binomial distribution | ||
| let count00 = 0; | ||
| for (let i = 0; i < shots; i++) { | ||
| // Each shot has a 50% chance of being '00' or '11' | ||
| if (Math.random() < 0.5) { | ||
| count00++; | ||
| } | ||
| } |
There was a problem hiding this comment.
The loop-based binomial sampling is inefficient for large shot counts. For better performance, use a binomial distribution approximation or library function. For example, with a large number of shots, you could approximate using a normal distribution, or use a more efficient algorithm. The current O(n) approach could be slow for shot counts in the thousands.
| await this.page!.click(`[role="option"]:has-text("${escapedAction}")`, { timeout: 5000 }).catch(() => { | ||
| // Fallback to less specific selector | ||
| return this.page!.click(`text="${escapedAction}"`); | ||
| }); |
There was a problem hiding this comment.
The catch block silently swallows all errors from the first click attempt and tries a fallback selector. If the fallback also fails, the error will propagate, but it won't be clear which selector failed or why. Consider logging which selector is being tried or re-throwing with context about both attempts.
| // Try textarea first | ||
| const textarea = await this.page!.$('textarea[placeholder*="prompt"]'); | ||
| if (textarea) { | ||
| await this.page!.fill('textarea[placeholder*="prompt"]', params.prompt); | ||
| } else { | ||
| // For contenteditable elements, use type() or evaluate() | ||
| const contentEditable = await this.page!.$('[contenteditable="true"]'); | ||
| if (contentEditable) { | ||
| await this.page!.evaluate((el, text) => { | ||
| (el as HTMLElement).textContent = text; | ||
| }, contentEditable, params.prompt); | ||
| } | ||
| } |
There was a problem hiding this comment.
If neither textarea nor contenteditable element is found, the code silently does nothing. This could lead to test failures that are hard to debug. Consider throwing an error or logging a warning when no input element is found.
Addresses 17 code review findings from automated PR analysis covering selector specificity, error transparency, deprecated method usage, and unused imports.
Playwright Selector Improvements
google-labs.ts: Replaced comma-separated fallback selectors with explicit try-catch chains to prevent ambiguous element matching:
Added special character escaping for text-based selectors and role-based selection for action dropdowns.
ai-studio-ultra.ts: Fixed contenteditable handling by using
page.evaluate()to set textContent instead offill(), which doesn't work on non-input elements.playwright-service-base.ts: Removed hardcoded Chrome 120 user agent string to use Playwright's auto-updated defaults.
Error Handling & Observability
countTokens()to throw instead of returning 0, making API failures explicit to callersAPI Modernization
substr()withsubstring()for task ID generationCode Cleanup
Removed 7 unused imports across A2A, AP2, MCP, and quantum modules. Removed unused loop variables in mandate-manager.ts and communication.ts cleanup methods.
Documentation
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.