Andrew
@ayc1
casual and pragmatic
Pragmatic and direct reviewer who focuses on practical functionality over perfection. Balances thoroughness with efficiency, often skimming large PRs but providing detailed feedback on specific issues. Uses casual language and asks probing questions to understand the bigger picture.
2137
Comments
1055
PRs
28
Repos
104
Avg Chars
3
Harshness
Personality
Pragmatic and solution-oriented
Direct but supportive communicator
Focuses on real-world functionality
Appreciative of good work
Questioning and curious about implementation details
Balances perfectionism with practicality
Collaborative and team-oriented
Experience-driven decision maker
Greatest Hits
"too large for a thorough review so I just skimmed it. stamping to unblock"
"mostly skimmed, but seems good to me"
"node doesn't trust publishers but i trust you"
"yeeeeeee thanks frank"
"this is hard to read :["
"that's crazy"
"nit:"
"looks good to me but will let others have a look"
"can we just remove this all together?"
"understood. for the sake of this project, I think you've done enough"
Focus Areas
- Practical functionality
- Code simplification
- Error handling
- Test coverage
- Implementation efficiency
- User experience
- System architecture decisions
- Edge cases and validation
Common Phrases
"this should"
"just"
"I think"
"dont"
"what"
"nit:"
"looks good to me"
"can we"
"thanks for"
"mostly skimmed"
"seems fine"
"would be nice"
"good to me"
"make sense"
"wdyt"
Sentiment Breakdown
Review Outcomes
Most Reviewed Authors
Spiciest Comments
doesn't work
this is wrong. it should be using the same browser as the one being used in the script.
does this even work?
please don't do this. we don't want LicenseType tool to be here
just fyi, sometimes it doesn't say "subtree", but I don't know the full scope of what these error messages look like
can you share what the traces look like with the exact same setup but without id_stabilizer? and can you check what the consistency looks like over ~10 traces? I also experience times when two traces have identical outputs so I want to know what impact this is actually having
I actually can't find the example I was testing with a while back. the commit history seems to be lost.
@jinyangTF I feel like you're right in that clicking didn't work for all enabled buttons either, but I'm pretty confident that force clicking a disabled button is more wrong. I can imagine that a fix for clicking on enabled buttons can be to click using coordinates as a fallback if normal clicking doesn't work. in which case, we'll still want to have this change. wdyt?
this doesn't work all the time actually. it fails to extract the 72,600 yen option. https://smith.langchain.com/o/15e007eb-2337-4ddb-97f3-d01467854f36/projects/p/3d9f0773-4461-4799-ae2a-392ba661b6fc/r/35a452f9-3140-4890-8828-193291911b98?trace_id=24e463e8-92e3-4ae1-8837-32835cf25abb&start_time=2025-03-05T02:43:26.803614
this specific flow goes runs agentql on the deep link page to extract the expected output. does doggy hotel have the prices listed directly on the deeplink page? if not, this won't work and I'd suggest creating a separate integration test for this use case
@KateZhang98
code looks fine. it'd be good to show what the before/after looks like in the pr summary
im a little confused, invoke_tool looks exactly the same as what the reasoner used to be. what's the difference?
AI Persona Prompt
You are ayc1, a pragmatic senior developer who reviews code with a focus on practical functionality over theoretical perfection. Your communication style is casual, direct, and solution-oriented. You often use phrases like 'nit:', 'this should', 'can we', 'looks good to me', and 'I think' in your reviews. When reviewing large PRs, you'll often say something like 'too large for a thorough review so I just skimmed it. stamping to unblock' or 'mostly skimmed, but seems good to me'. You appreciate good work with phrases like 'thanks for adding the thorough test cases!' or casual expressions like 'yeeeeeee thanks frank'. You're not afraid to be direct when something is wrong, asking probing questions like 'does this even work?' or 'can we just remove this all together?'. You focus on practical concerns: simplifying code, proper error handling, test coverage, and real-world functionality. You often suggest centralizing constants, adding test cases for edge cases, and improving code readability. When you see hard-to-read code, you'll say 'this is hard to read :[' and suggest improvements. You balance being thorough with being efficient, often trusting teammates ('i trust you') while still catching important issues. You're collaborative, frequently tagging others with '@username' and asking 'wdyt?' (what do you think). Your reviews show you have experience with the codebase and care about maintainability and user experience.
Recent Comments (1512 total)
agreed with pasha. this is a good idea to start but what we want is something that can scale across all repos and even easier to auto populate
nice.
for the issue with clicking, can you see if using `page.get_by_prompt(query, timeout=300, mode="fast", experimental_query_elements_enabled=True)` fixes it?
what problem does this solve? adding a brand new prompt like "search for elon musk, trump, and steve jobs on google in 3 separate tabs." doesn't mean it helps with any of our existing use cases. there is already a test case in the linear ticket regarding ebay - does this solve a problem in ebay? if not, I'd suggest not pursuing this for now
the folder name `browser_use` had conflicts with the browser_use library. had to add the ugly _custom suffix
current approach:
friday
llm_extract_tool
```
{
products[] {
name
price
}
}
```
append the html to the artifact of the llm_extract tool message
then, in Learner
1. process the llm_extract tool messages to replace the html artifact with data selector codegen artifacts
2. process the entire AgentOutput to codegen the script
==============
llm_extract
no longer needed yea?
sorry, should have mentioned this earlier but we should be using @TinyGambe 's shared browser library instead. and then adding surfsky support to the shared browser library so that it can be accessible by all.
so three things
1) migrate friday to use the shared browser
2) delete tf_browser_service within friday
3) add surfsky to shared browser
can we gate all of these optimizations within a config option?
this doesn't make the args strict. how about generating the argparse logic from the `params_dict` value instead?
this pr is fine, but wanted to have more clarity on the one before this, particularly on the maintenance of this approach
@paveldudka I think this is something that you might have opinions about.
the code looks fine but does BrowserService really need to be reading env vars during import?
cory.langgraph_app.main imports secrets.py, so we need to set our env vars before this
cory.langgraph_app.main imports secrets.py, so we need to set our env vars before this
see the summary for why this is an issue and that this is outside the scope of this pr
because the error occurs in the init of the superclass SecretsConf. https://github.com/tinyfish-io/tf-common/blob/725db59ce6a86124f56085adce738a104f36c1b9/tf-common-secrets/src/tinyfish/common/secrets/secrets.py#L28-L31
this error message explains what variables need to be set. otherwise SecretConf will try to initialize and throw an unhelpful error message "AWS secret name cannot be empty"
because the error occurs in the init of the superclass SecretsConf. https://github.com/tinyfish-io/tf-common/blob/725db59ce6a86124f56085adce738a104f36c1b9/tf-common-secrets/src/tinyfish/common/secrets/secrets.py#L28-L31
this error message explains what variables need to be set. otherwise SecretConf will try to initialize and throw an unhelpful error message "AWS secret name cannot be empty"