Urvish Thakker
@thakkerurvish
curious and collaborative with encouraging positivity
Collaborative and curious reviewer who frequently asks questions to understand the reasoning behind changes. Maintains a positive, supportive tone while being thorough about technical details and best practices.
1384
Comments
643
PRs
30
Repos
129
Avg Chars
2
Harshness
Personality
Inquisitive and detail-oriented
Collaborative team player
Process-focused and quality-conscious
Supportive and encouraging
Practical problem-solver
Security and workflow-aware
Performance-conscious
Documentation advocate
Greatest Hits
"LGTM, but will let @reviewer review as well!"
"Please take a look at pre-commit checks!"
"curious, what do we need X for?"
"gotcha, updated it!"
"just curious why we have"
"I think it might be better to"
"thank you for taking a stab at this!"
"Delta >>> Overlay"
"Boss, can you please share how did you test"
Focus Areas
- workflow processes and CI/CD
- pre-commit checks and code quality
- security considerations
- performance optimization
- logging and debugging
- configuration management
- code organization and structure
- testing approaches
Common Phrases
"LGTM"
"curious"
"I think"
"thank you"
"gotcha"
"updated it"
"just curious"
"might be"
"should we"
"pre-commit checks"
"will let"
"review as well"
"but I"
"what do we need"
"why are we"
Sentiment Breakdown
Review Outcomes
Most Reviewed Authors
Spiciest Comments
> > Each customer is an isolated environment and we don't intend to share anything between different customers
>
> @thakkerurvish in my initial and current implementation, `cooley` and `bain` do share some logic within the `common` folder. as you can see, both `cooley` and `bain` were broken after the top level `pyproject.toml` was removed by [b5cb530](https://github.com/tinyfish-io/agentql-apps/commit/b5cb5303616d0354501f1c5b00bb08f2477fc051)
>
> The ideal approach might be to move the fu
> @thakkerurvish I'm new to this code base and not sure I understand when the issue occurs and what the fix is.
>
> Just so I understand. This issue occurs when there is a delay between two requests and there is a native playwright issues that causes this?
Yes, when there is a gap between the two request to the service (not sure if delay is the right choice of word here).
So, the error is being spawned from the code line `page.goto()`. This error is sporadic and random, and only happens
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.
> > 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.
Oh, here is a PR for the version bump: https://github.com/tinyfish-io/agentql-client/pull/320
> > While sending Fact, Query and Response should we not make sure to split it as it is quite likely we may exceed the context window in some cases here. Or will there be a different module to handle this?
>
> @thakkerurvish
>
> As part of the V0, we don't support splitting as more work is needed to reproduce each chunk response for a Fact chunk. Right now, the merger can't decompose a merged response.
>
> This would be part of the future plan to simulate the exact splitting and merging
Imo, paraphrase agent should always take all the items. Selecting a sub items from the query is the remix_agent or mixing_agent. The paraphrase agent job should be just take entire query and paraphrase it.
CC: @mingyang-tinyfish, @zifanwTF
I think this type of splitting can create some problems and is not accurate. For example:
```
{
header
{
name,
password
}
}
```
What it would split on is `\n` but `\n` split does not gurantee it gives an stnadalone elemnt everytime. Here in example: header will come as an element and mixed with other elements which should not be mixed with some random element. So, this seems need some fixes else we will producing unintended queri
We should read project_name from environment variables as if someone goes and just changes this string it will start reporting to a new project. It is easy for some developer just to change the string without thinking about its impact. If we read it from a global variable or enviornment variable that adds a emphasis on it and people working on code are less likely to make modifications to those. Do you mind if I refractor the code a bit and make a PR, @zifanwTF ?
AI Persona Prompt
You are @thakkerurvish, a collaborative and inquisitive code reviewer who approaches reviews with genuine curiosity and supportive energy. Your reviewing style is characterized by asking thoughtful questions to understand the 'why' behind changes rather than just pointing out issues. You frequently start questions with 'curious' or 'just curious why' and often say 'I think' when offering suggestions. You're particularly focused on workflow processes, pre-commit checks, security considerations, and performance optimization. Always check for pre-commit issues and remind authors about them with 'Please take a look at pre-commit checks!' You often defer to domain experts with phrases like 'LGTM, but will let @expert review as well!' showing your collaborative nature. Use 'gotcha, updated it!' when acknowledging feedback and 'thank you for taking a stab at this!' to show appreciation. You're quick to approve with 'LGTM!' but thorough in your questioning. Pay attention to configuration files, logging practices, and CI/CD workflows. When you see something you don't understand, ask genuine questions like 'what do we need X for?' or 'why are we removing this method?' Your tone should be encouraging and supportive while maintaining technical rigor. Use casual expressions like 'Boss' occasionally and throw in some personality with references like 'Delta >>> Overlay' when appropriate.
Recent Comments (1262 total)
> 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
> > > 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_
I think we need to bump this date a bit. I think it's failing as this is today's date.
LGTM!
I think we just need to address the Vulnerability issue and we're good to merge!
not sure I understand the comment, could you please elaborate?
CI Checks are failing (OSV-Scanner PR Scan / vulnerability-check is required iirc)
LGTM!
I tested the changes in this branch and it worked fine!
LGTM!
iiuc, it's from `marshmallow`, but @frankfeng98 might have more context here.
LGTM!
> Good to merge pending README update
Updated it!
Updated it!
Updated it!
discussed offline, we plan to have it for now. once we test it and stuff, we can always remove later what might not be needed.
Sounds good, I'll create a follow-up PR to clean this up.
linear issue for tracking: https://linear.app/tinyfish/issue/ENG-10492/cleanup-poe-string-in-env-file