luomchl

Michael Luo

@luomchl
GitHub Profile
thoughtful and collaborative
Pragmatic and forward-thinking reviewer who balances immediate concerns with long-term considerations. Asks thoughtful questions about abuse prevention, performance implications, and production readiness while maintaining a collaborative tone.
71
Comments
32
PRs
10
Repos
67
Avg Chars
2
Harshness

Personality

future-oriented security-conscious pragmatic collaborative detail-oriented cost-aware production-focused flexible

Greatest Hits

"in the future if users want more control"
"how about enforcing this by throwing 400 err"
"is there a max timeout to avoid abuse?"
"over-engineering, keeping it simple for now"
"what's the RPS like? i think batching will be critical"
"can we ideally pass in temp credentials instead of static keys?"

Focus Areas

Common Phrases

"in the future" "how about" "are there other options?" "what's the lifecycle" "is there a max timeout" "can we ideally" "feels like" "i wonder if" "alternatively we can" "nit:" "over-engineering, keeping it simple" "confirmed working" "addressed" "done"

Sentiment Breakdown

neutral
36
constructive
10
questioning
2

Review Outcomes

APPROVED
25
CHANGES_REQUESTED
1

Most Reviewed Authors

luomchl
25
EricMulhernTinyfish
17
paveldudka
13
TinyGambe
5
ayc1
4
npkhang99
3
colriot
3
thakkerurvish
1

AI Persona Prompt

You are luomchl, a thoughtful and pragmatic code reviewer who thinks ahead. You're particularly focused on production readiness, security implications, and future maintainability. You often start suggestions with 'how about' or 'in the future' and frequently ask questions like 'is there a max timeout to avoid abuse?' or 'what's the lifecycle of...'. You're cost-conscious and performance-aware, often asking about RPS, batching, and rate limiting. You care about fingerprinting and detection avoidance for browser automation. When you see over-engineering, you'll say 'over-engineering, keeping it simple for now'. You prefer temp credentials over static keys and inline scripts over CDN calls. You're collaborative, using phrases like 'alternatively we can' and 'how about'. You notice small details (using 'nit:' for minor issues) but also see the big picture. You're concerned about production noise (questioning console.logs), prefer environment variables for configuration flexibility, and always consider abuse prevention. You're not harsh - you approve most PRs and phrase feedback as suggestions or questions rather than demands. When addressing feedback, you simply say 'done' or 'addressed'. You balance being thorough with being practical, and you're always thinking about what users might need in the future.

Recent Comments (48 total)

unikraft-cdp/#110 Randomize Xvfb screen size and browser window size [view]
in the future if users want more control over screen size we can make it a configurable feature
unikraft-cdp/#85 Support providing env to Docker [view]
alternatively we can add these inline to each docker-compose.yml
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/RRWEB_INTEGRATION.md [view]
nit: a few other repos have this naming convention `cp .env.example .env.local`
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/.env.example [view]
is .env needed for all of these? running `make start` should pass in the env vars defined in `docker-compose.yml`
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/.env.example [view]
if this would only ever be used for rrweb, prepend `RRWEB` to name?
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/src/init-scripts/rrweb.js [view]
are all these console.logs necessary for production code?
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/src/init-scripts/browser_recording.js [view]
how about injecting inline scripts instead of injecting from external CDN in case some websites don't like that and might block it? also 1 less network call for anchor browser i wonder if the rrweb script will be a fingerprint that increases detection 🤔
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/src/proxy.ts [view]
console log seems noisy to use as our transport, are there other options?
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/src/services/s3-service.ts [view]
what's the RPS like? i think batching will be critical since it feels like there could be 10s of thousands of events how large are the files? what is the lifecycle of the s3 objects? if this is high traffic, how about a simple local rate limit to keep costs bounded
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/src/main.ts [view]
i think dotenv isn't needed since we're passing in env vars from docker-compose.yml locally and in prod we're passing it in via Unikraft API call https://github.com/tinyfish-io/wadl/blob/main/agentqlserver/browser/session_manager.py#L150 EDIT: but then you also have the AWS creds that have to be passed into the container somehow
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/package.json [view]
is it better to modify the makefiles commands since it starts the server + container with all the right setup already?
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/.env.example [view]
can we ideally pass in temp credentials instead of static keys? in production we will be calling unikraft api from wadl service's environment and passing in creds there
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/browsers/anchor/docker-compose.yml [view]
this var not used in the code
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/.example.env [view]
unused in proxy
unikraft-cdp/#70 Record browser sessions rrweb · cdp-proxy/.env.example [view]
fingerprint handling scripts do not need to be injected in order for rrweb scripts to be injected right? though the code is currently structured that way for example when Anchor browser is used, we avoid changing the fingerprints/headers since the browser binary has built-in handling for that and we don't want to mess with it. so to support Anchor rrweb, we wouldn't want to enable this feature