-
Notifications
You must be signed in to change notification settings - Fork 578
FEAT add Playwright-based Copilot target #1117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
" await ConsoleAttackResultPrinter().print_conversation_async(result=result, include_auxiliary_scores=True) # type: ignore\n", | ||
"\n", | ||
"\n", | ||
"# Uncomment to run (after starting browser with remote debugging)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: line 169 isn't commented so shouldn't it be # comment to pause execution or something like that or that line 169 should be commented ?
{ | ||
"cells": [ | ||
{ | ||
"cell_type": "code", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update the _toc.yml
""" | ||
self._validate_request(prompt_request=prompt_request) | ||
|
||
if not self._page: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't this be caught in the init
# Try to extract content to see if it's ready | ||
try: | ||
test_content = await self._extract_multimodal_content_async(selectors, initial_group_count) | ||
content_ready = False | ||
|
||
# Check for placeholder text | ||
placeholder_texts = ["generating response", "generating", "thinking"] | ||
|
||
if isinstance(test_content, str): | ||
text_lower = test_content.strip().lower() | ||
# Content is ready if it's not empty and not a placeholder | ||
content_ready = text_lower != "" and text_lower not in placeholder_texts | ||
elif isinstance(test_content, list): | ||
content_ready = len(test_content) > 0 | ||
|
||
if content_ready: | ||
logger.debug("Content is ready!") | ||
return test_content | ||
else: | ||
logger.debug("Message exists but content not ready yet, continuing to wait...") | ||
except Exception as e: | ||
# Continue waiting if extraction fails | ||
logger.debug(f"Error checking content readiness: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be made into another function like extract_content_if_ready
Returns: | ||
Filtered list without placeholder text | ||
""" | ||
placeholder_texts = ["generating response", "generating", "thinking"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're using it more than once, i think this should be a constant
pass | ||
|
||
# Check direct images | ||
imgs = await msg_group.query_selector_all('button[aria-label*="Thumbnail"] img') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering whether these strings should be made constants
logger.debug(f"Found {len(imgs)} img elements in message group {idx+1}") | ||
image_elements.extend(imgs) | ||
else: | ||
logger.debug(f"No imgs with button selector in message group {idx+1}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: No images with...
|
||
return image_pieces | ||
|
||
async def _extract_multimodal_content_async( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this function be broken up a bit ?
selectors: The selectors for the Copilot interface | ||
initial_group_count: Number of message groups before this response (to filter out old groups) | ||
""" | ||
# Get only NEW message groups from this response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is new in all caps ? is it an acronym ?
Description
This PR adds a copilot target for M365 and consumer copilots based on Playwright. There are also minor changes to the existing PlaywrightTarget.
Tests and Documentation
Added unit tests for the new target, and updated the existing playwright target tests.