lebahoang

Le Ba Hoang

@lebahoang
GitHub Profile
collaborative and methodical
Collaborative and methodical reviewer who focuses on code organization and workflow optimization. Frequently engages in detailed discussions with team members, especially @paveldudka, and provides constructive suggestions for improving code structure and build processes.
140
Comments
32
PRs
8
Repos
133
Avg Chars
3
Harshness

Personality

Collaborative and team-oriented Detail-oriented with build processes Methodical in explaining reasoning Proactive in suggesting improvements Patient in back-and-forth discussions Process-focused and systematic Helpful with actionable suggestions Technical problem-solver

Greatest Hits

"pls review again @paveldudka"
"I think you should try to use"
"makes our code sort of more united"
"I would suggest to remove this line of code"
"turned out that I only need to"
"Cheers"

Focus Areas

Common Phrases

"I think" "pls review again" "here will make" "I would suggest" "this flag will be" "may be better to" "also we want to" "I have added" "makes our code" "disk space usage" "build step" "review again" "noted it" "turned out that" "cheers"

Sentiment Breakdown

neutral
112
questioning
10
positive
8
constructive
5

Review Outcomes

APPROVED
8

Most Reviewed Authors

lebahoang
110
npkhang99
15
paveldudka
9
maxluong2496
5
ayc1
1

AI Persona Prompt

You are @lebahoang, a collaborative and methodical code reviewer who focuses heavily on code organization, build processes, and workflow optimization. You frequently engage in detailed technical discussions, especially around CI/CD workflows and Docker build processes. Your communication style is patient and explanatory - you often provide context for your suggestions and engage in back-and-forth discussions to reach the best solution. You have a particular expertise in GitHub workflows, disk space optimization, and event-driven architecture patterns. You commonly start suggestions with 'I think' or 'I would suggest' and often ask colleagues to 'pls review again' after making changes. You're especially attentive to code unity and consistency, often suggesting ways to make code 'more united' or follow established patterns. You frequently mention specific technical details like disk usage, build artifacts, and Docker cache management. You tend to provide actionable hints and explain your reasoning thoroughly, sometimes including screenshots or commit references to support your points. Your tone is always constructive and collaborative, ending discussions on positive notes like 'Cheers' when solutions are found. You pay close attention to method organization, suggesting cleaner implementations like extending EventEmitter classes for event-driven patterns, and you're proactive about preventing future issues by suggesting better code structure upfront.

Recent Comments (135 total)

unikraft-cdp/#251 add TF BROWSER launch options · tetra/src/tetra.ts [view]
@paveldudka actually this flag didn't exist before, you can double-check in main branch, indeed I and Max added it in @maxluong2496 branch feat/implement-tf-browser (this commit https://github.com/tinyfish-io/unikraft-cdp/commit/bc46a6480e477ee8fc953a1ab6680ab79f5d1240#diff-c65302cf1a84cb12bb2b5735c85f4cdc26ce86038c3ca9efefbf9e24b6e40845) this flag will be set for TF browser only,
unikraft-cdp/#251 add TF BROWSER launch options · tetra/src/tetra.ts [view]
@paveldudka I followed the pattern of how we are using BROWSER_MODES, my understanding here is if I do the browser type comparison and see that if it is TF Browser then I will set these launch options, btw could you elaborate more on this ? ``` if DO_SOMETHING: Do something ````
unikraft-cdp/#251 add TF BROWSER launch options [view]
@coderabbitai review
unikraft-cdp/#251 add TF BROWSER launch options [view]
@coderabbitai review
unikraft-cdp/#249 Add TF Browser image build to Github workflow [view]
@coderabbitai review
unikraft-cdp/#249 Add TF Browser image build to Github workflow [view]
@coderabbitai review
unikraft-cdp/#235 PF-778: make tetra emit heartbeat events · tetra/src/telemetry/telemetry-heartbeat-emitter.ts [view]
just opinion, extends EventEmitter class here will make the implementation cleaner since event driven pattern is pretty fit here: for every x seconds let emit an event then implement the handler for this event
unikraft-cdp/#235 PF-778: make tetra emit heartbeat events · tetra/src/http-proxy.ts [view]
I think you should try to use TelemetryHeartbeatEmitter.emitHeartbeat method here, may be introduce a new event ? because usageTracker.getSnapshot() is also called inside emitHeartbeat method. It makes our code sort of more united
unikraft-cdp/#235 PF-778: make tetra emit heartbeat events · tetra/src/telemetry/telemetry-heartbeat-emitter.ts [view]
Pls make sure this payload is consistent with DB schema. Also we want to use this payload to update the telemetry record via the condition heart_beat_time > updated_at hence timestamp field here should be in UTC format same as in DB
unikraft-cdp/#235 PF-778: make tetra emit heartbeat events · tetra/src/http-proxy.ts [view]
Yes I know that we should not call WADL endpoint in connectionClose event, so I mentioned about introducing the new event type: how about add a new event type such as "connection close event" (naming is on you), and move the logic into the handler for this event, right now the handler is "printing to stdout" and I think you already have this handler.
unikraft-cdp/#235 PF-778: make tetra emit heartbeat events · tetra/src/telemetry/telemetry-api-handler.ts [view]
yes, it will point to an endpoint in WADL
unikraft-cdp/#235 PF-778: make tetra emit heartbeat events · tetra/src/tetra.ts [view]
I don't quietly understand here, you created telemetryHeartbeatEmitter when creating new browser (at line 1181), why don't just start listening there too or in handleBrowserWebsocketConnectionRequest (at line 832) ?
unikraft-cdp/#231 Add session duration tracking to UsageTracker · tetra/src/tetra.ts [view]
I think it may be better to put the startSession function above the line 819, before function this.browserWebsocketConnectionLock.take, because the start times of concurrent connections to chromium browser don't really matter in measuring browser session duration.
unikraft-cdp/#231 Add session duration tracking to UsageTracker · tetra/src/usage-tracker.ts [view]
here the implementing of UsageTracker+UsageSnapshot when we want to add the new metric is fine now, but in future if we have to add many more metrics, each metric has different input/logic to calculate I think we should think again about the way we implement them ...
unikraft-cdp/#213 Add proxy usage tracking with per-session and cumulative metrics · tetra/docker-compose-anchor.yml [view]
I think this is the valid comment, we should add the docker compose version requirement