Pasha Dudka
@paveldudka · Head of Engineering @ TinyFish
direct and uncompromising but educational
Extremely thorough and detail-oriented reviewer who questions everything and demands high engineering standards. Uses direct, sometimes blunt communication while providing constructive feedback with clear explanations of why changes are needed.
5899
Comments
2803
PRs
68
Repos
109
Avg Chars
7
Harshness
Personality
Highly inquisitive and questioning
Standards-driven perfectionist
Direct communicator who doesn't sugarcoat
Architecturally minded with strong opinions
Patient teacher who explains reasoning
Zero tolerance for production code quality issues
Pragmatic problem solver
Protective of codebase integrity
Greatest Hits
"curious, why there is a..."
"this is absolutely not acceptable"
"potential NPE"
"looks good overall. Just address"
"what's this?"
"absolutely redundant"
"please don't put obscenities in the production code"
"Im a bit puzzled by what we are trying to do here"
"doesn't sound like something I would want as a user"
Focus Areas
- code architecture and structure
- async/await correctness
- dependency management
- naming conventions
- production code quality
- error handling patterns
- Docker and deployment configs
- type safety and null pointer exceptions
- REST API standards
- project organization
Common Phrases
"curious, why"
"I don't see"
"looks like"
"what's this?"
"why do you need"
"shouldn't it be"
"this is absolutely not acceptable"
"potential NPE"
"looks good overall"
"address comments before landing"
"what is this?"
"Im a bit puzzled by"
"doesn't sound like something I would want"
"please don't"
"absolutely redundant"
Sentiment Breakdown
Review Outcomes
Most Reviewed Authors
Spiciest Comments
@jinxz01 , @lozzle
this is absolutely not acceptable to have here.
During pytest we shouldn't be talking to AWS Secret store at all and secrets functionality should be mocked out.
what is this name?
Is there a `BrokenNavigateWithAuthTool`?
what's fixed about this tool? I don't see anything in the code that would explain such odd naming.
this is absolutely not acceptable.
Util file making some assumptions about its position in relation to the root of the project?
Super fragile and insecure.
Tool should accept some kind of configuration object which would point to a writable location on the filesystem. I.e. I want to create a login tool and tell it where it can safely store any artifacts it wants to.
Tool itself should not make any assumptions about where it is located or what path it is allowed to write to
Im a little puzzled why would you split the logic of waiting.
I.e. here you want to wait for a popup event, right?
If so, why not just wait here rather than relying on a on_popup callback?
```
await (await self.get_current_page()).wait_for_event("popup")`
```
its so much easier to follow this way.
Am I missing something?
is all this vibe coded?
emmm.. why would you want to talk to CDP websocket directly and handle all the communication manually? why not Playwright?
can you elaborate?
700 lines of code just to take a screenshot?
vibe coded PR that wasnt read by the author
@mollimoll , indeed, `leading-[1.1]` works. I like it better.
Default leading doesn't work. Really need 1.1
please don't contaminate core backend code
all apps related code should be in its own folder, so its as isolated as possible
Don't use `core`, `models`, `services` modules
Each app should be in its own module:
```
backend/
└── app/
└── mini_apps/
├── config.py
├── router.py
└── brand_sentiment/
├── __init__.py
├── service.py
├── utils.py
└── models.py
```
Initially I did that, but then realized that typechecking is all messed up for models that inherit from Entity.
Just look at this
<img width="1314" height="792" alt="Screenshot 2025-07-26 at 10 47 23 PM" src="https://github.com/user-attachments/assets/d17b439d-ed28-478a-a7f4-c73a07188708" />
Also `Entity` uses a deprecated `json_encoders` attribute in `model_config` which adds even more warnings
Looking at the `Entity` class it uses custom timestamp type to mainly achieve 2 goals:
* be
AI Persona Prompt
You are Pavel Dudka, a senior engineer who takes code quality extremely seriously. You review code with intense scrutiny and ask probing questions about every decision. Start many comments with 'curious, why' or 'what's this?' when you see something questionable. You have zero tolerance for sloppy code, especially async/await misuse, poor error handling, or architectural shortcuts. You frequently point out 'potential NPE' issues and call out 'absolutely redundant' code. When you see serious problems, use phrases like 'this is absolutely not acceptable' or 'absolutely not.' You're particularly pedantic about naming conventions, dependency management, Docker configs, and project structure. You often provide detailed technical explanations and alternative approaches. End reviews with 'looks good overall. Just address [issues]' when mostly satisfied, or 'please address comments before landing' when more work is needed. You're direct and sometimes blunt, but always educational - you explain WHY things are wrong, not just THAT they're wrong. You catch subtle issues others miss and have strong opinions about REST API standards, async patterns, and production code quality. Use casual contractions like 'don't', 'can't', 'Im' and ask follow-up questions to understand the developer's thought process.
Recent Comments (3456 total)
@londondavila , @ayc1 , @lozzle
remember we talked about sth like this.
Few comments here:
* we want sth like this to be reusable across projects
* having to specify secret explicitly is quite inconvenient - it should just try to figure out secrets automatically. I.e we need to support variable expansion
* env var names don't always match secret name in secret manager
`env.example`:
``
what's the reason for ignoring it?
Looks like patch is already published and readily available.
Just run npm update?
> @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?
if I understand the code correctly, in this case its fine since main.py - is an entry point to the application
I don't think this is a right place for this.
These checks will be executed during class resolution stage (at the time of import)
Why not do this within `initialize_secrets`? which is an explicit point of initializing secrets
legit warning from Korbit.
business logic should not be placed before imports
The fact that you have to do this (to make `cory.langgraph_app.main` happy) highlights exactly the problem we are trying to address - `cory.langgraph_app.main` should not be loading env vars during module import. Module import is not the right place for any business logic
same here
@jinxz01 , @lozzle
this is absolutely not acceptable to have here.
During pytest we shouldn't be talking to AWS Secret store at all and secrets functionality should be mocked out.
let's make sure simply silencing security warnings is not the recommended approach everywhere.
Otherwise what's the point of these checks if we simply ignore them?
according to the method signature, this check is useless as browser is never None
useless comment
is it intentional that stream url is a property of a browser even though stream url is page-specific?
I.e. second page that is open will have a different stream url
looks reasonable.
Multiple projects in the same repo - is quite uncharted territory for tiny fish (especially around IDE setup), but considering main project here is still "pyt", I guess it may work
why so restrictive? (pinning exact version) This can be a ">1.11.2,<1.12"
only application entry point code should read env vars.
In all other cases environment variables should already be set. Looking at this class, it doesn't look like its an entry point to the application, so it shouldn't be reading `.env.local`
And also it should not be configuring logger too since this way it overwrites logger configuration for the entire application
> @paveldudka I'm not sure whether the correct user-agent for browser is used here. I see that by default, we're using Linux as the default user agent for browser service. Also would like to get your opinion on the use of residential proxy here.
I don't understand the question.