Frank Feng
@frankfeng98
collaborative and inquisitive
Frankfurt is a thorough and methodical reviewer who focuses on practical considerations and long-term maintainability. They ask clarifying questions to ensure they understand context and implications, often seeking to understand the 'why' behind changes before diving into implementation details.
1191
Comments
643
PRs
24
Repos
139
Avg Chars
2
Harshness
Personality
Context-seeking - frequently asks for background information
Detail-oriented - catches subtle bugs and inconsistencies
Collaborative - brings in other reviewers when needed
Process-conscious - cares about versioning, releases, and documentation
Pragmatic - balances ideal solutions with practical constraints
Supportive - uses encouraging language and thanks contributors
Safety-minded - concerned about backward compatibility and user impact
Communication-focused - emphasizes clear documentation and error messages
Greatest Hits
"I don't think I have enough context to review this PR"
"Just want to make sure that it will work properly. Have you tested it?"
"Thank you for pointing this out!"
"For better code readability, can we..."
"We probably want to specify..."
"Could you include a brief description about..."
"This is a publicly facing API, can we add sufficient doc string to it?"
Focus Areas
- release management and versioning
- documentation completeness
- backward compatibility
- code readability and maintainability
- error handling and validation
- API design and public interfaces
- testing and verification
- security considerations
Common Phrases
"I think"
"Could you"
"Just want to make sure"
"Thank you for"
"LGTM!"
"It seems that"
"We probably want to"
"That makes sense!"
"Could you share"
"Have you tested"
"We may want to"
"I don't think I have enough context"
"For better code readability"
"Just a nit"
"Thank you for pointing this out!"
Sentiment Breakdown
Review Outcomes
Most Reviewed Authors
Spiciest Comments
> Not sure I understand the scope/purpose of this ticket. Could you please explain what the new jsonl format changes are?
Yes! So here is the format of the entry in current test assets:
```
{"trace_id": "81c6d7a4-3cb3-4c89-8ad7-1106e91ab5b0", "click_run_id": "be8bd53e-58f5-4961-80ef-b91dcafa0768", "element_description": "The button with the name 'Close (Press escape to close)' in the dialog window.", "state_id": "request-1053", "langsmith_run_url": "https://smith.langchain.com/o/15e007eb-
> Lots of great work here – this is a decent solution for first round of evaluating weblog. However, I have some doubts about the long-term accuracy of a visual comparison approach. There may be things that result in subtle changes in the image but are breaking, for example if a button is currently clickable or not. Or, there may be major visual differences that are unimportant for successful replay, such as if an image loads correctly or if the content of a canvas element displays or not.
>
Relative URL won't work, prepend this with website base url
> Currently Client-SDK is broken for users, as the fix for async and multiple sessions is not published yet.
>
> We should land the hotfix for that in version `0.4.1` and than merge this PR.
Yes! I will publish a new version today. And I will probably wait till Mark @lozzle merge the trail logger into the main before merging this PR to avoid file conflicts on his end.
Thank you for pointing this out! However, I am wondering whether it is possible to still include this information in the comment while emphasizing that this function invokes a callback function. So something like below:
```
async def close(self):
"""Close the popup by invoking the callback function passed into the Popup object when it is initialized. By default, the callback function will query AgentQL server and click the close button in popup with the following query:
{
> @frankfeng98 , wait, if aforementioned code is inside JS code - Chrome Extension is broken now then?
Yes, and sorry for the confusion that's caused by this code. Kate and I are working on a fix rn in wadl-inspector. It should be a pretty simple fix, we will make a PR once it's ready. I will be more mindful about potential impact of modifying this kind of code in the future!
AI Persona Prompt
You are Frankfurt (@frankfeng98), a collaborative and thorough code reviewer who prioritizes understanding context before diving into implementation details. Your review style is inquisitive and supportive - you frequently start comments with 'I think', 'Could you', and 'Just want to make sure' to maintain a collaborative tone. You have a strong focus on release management, documentation, and backward compatibility, often asking about versioning implications and user impact. You're detail-oriented and catch subtle bugs, but you phrase findings as questions rather than demands: 'Have you tested this?' or 'Could you verify the concern with...?' When you lack context, you're honest about it and bring in other reviewers. You care deeply about code readability and often suggest structural improvements with phrases like 'For better code readability, can we...' You're process-conscious, frequently asking for release notes, proper versioning, and comprehensive documentation for public APIs. You use encouraging language like 'LGTM!', 'That makes sense!', and 'Thank you for pointing this out!' even when requesting changes. Your approval threshold is low - you're willing to approve to unblock progress while still providing valuable feedback. You balance ideal solutions with practical constraints, often acknowledging when refactoring might be too complex for the current scope. Always maintain Frankfurt's collaborative, context-seeking approach while being thorough about potential issues.
Recent Comments (901 total)
/korbit-review
LGTM!
I'm wondering do we need to update the image name used in request payload as well. When I test create tetra session with weblog (in prod) within Postman, the following error was thrown:
```
{
"error_info": "Input should be 'weblog' or 'weblog-test': body > experimental_custom_tetra_tag",
"status_code": 422,
"metadata": {
"request_id": "f703b930-f0d6-4144-bdf4-dfdacc1888
> > I'm wondering do we need to update the image name used in request payload as well. When I test create tetra session with weblog (in prod) within Postman, the following error was thrown:
> > ```
> > {
> > "error_info": "Input should be 'weblog' or 'weblog-test': body > experimental_custom_tetra_tag",
> > "status_code": 422,
> > "metadata": {
> > "request_id": "f703b930
> > > > I'm wondering do we need to update the image name used in request payload as well. When I test create tetra session with weblog (in prod) within Postman, the following error was thrown:
> > > > ```
> > > > {
> > > > "error_info": "Input should be 'weblog' or 'weblog-test': body > experimental_custom_tetra_tag",
> > > > "status_code": 422,
> > > > "metadata": {
> > > >
Thank you for pointing this out! I have updated the package version, since `express`, which is used by `@modelcontextprotocol/sdk` pins `qs` to `6.13.0`, I manually override the package version to `6.14.1`. Since this folder is not being actively developed and there is no breaking changes introduced, the impact should be minimal.
Oh yes, I was referring to what we talked about -- once we have the new examples set up, we can maybe remove the old test assets and this condition check. I will update the it to make it more clear and address CI issue.
> Not sure I understand the scope/purpose of this ticket. Could you please explain what the new jsonl format changes are?
Yes! So here is the format of the entry in current test assets:
```
{"trace_id": "81c6d7a4-3cb3-4c89-8ad7-1106e91ab5b0", "click_run_id": "be8bd53e-58f5-4961-80ef-b91dcafa0768", "element_description": "The button with the name 'Close (Press escape to close)' in the dialog
LGTM
I've addressed these security concerns in my PR so you could merge in the latest main and they should go away.
This is a good point! It's still a manual script as we're moving into Databricks I will make sure to update that script as well.
/korbit-review
Every worker will require s3 access.
The current algorithm relies on pixel comparison and is pretty sensitive to different colors, etc., on the page. For instance, on one of the page I've seen, if one page has popup showing up in the bottom of the page for cookie permission, and the other does not, the similarity drops to roughly 80%. The reason we set it at 95% is somewhat arbitrary, but I want to make sure that the image should be
state_id is equivalent to action id -- state_id was how we used to call it with Weblog v1, I will update the naming to something more representative.