Conversation
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/main/utils/logger.ts:82-106
**Chromium log file grows unboundedly**
`chromium.log` has no size limit or rotation, unlike `main.log` which is capped at 10 MB with 3 archives. With `--log-level=0` (VERBOSE), Chromium is extremely chatty during normal operation and can produce several megabytes per session. Over time this will silently consume disk space with no cleanup mechanism. The same log directory already implements rotation for the main log — the same approach (or at least a `maxSize` guard that truncates the file at startup) should be applied here.
### Issue 2 of 3
apps/code/src/main/utils/logger.ts:93-94
**`readSync` return value not used to bound `toString`**
`readSync` returns the number of bytes actually read, which may be less than `length` (e.g. if the file was truncated between the `fstatSync` and `readSync` calls). Since `Buffer.alloc` zero-fills, any unread bytes at the end become null bytes in the UTF-8 string. Using `bytesRead` to slice the buffer is safer:
```suggestion
const bytesRead = readSync(fd, buf, 0, length, size - length);
return buf.subarray(0, bytesRead).toString("utf8");
```
### Issue 3 of 3
apps/code/src/main/bootstrap.ts:47-51
**Log directory path duplicated (OnceAndOnlyOnce)**
The same expression — `join(os.homedir(), ".posthog-code", isDev ? "logs-dev" : "logs")` — already appears verbatim in `logger.ts` as `LOG_DIR`. If the log directory ever changes, both sites need updating in sync. Consider extracting a shared helper (e.g. a small `utils/paths.ts` exporting `getLogDir(isDev)`) that both `bootstrap.ts` and `logger.ts` can import without creating a circular dependency.
Reviews (1): Last reviewed commit: "chore(code): add chromium logs" | Re-trigger Greptile |
| export function readChromiumLogTail(): string | undefined { | ||
| const path = getChromiumLogFilePath(); | ||
| if (!path) return undefined; | ||
|
|
||
| let fd: number | undefined; | ||
| try { | ||
| fd = openSync(path, "r"); | ||
| const { size } = fstatSync(fd); | ||
| if (size === 0) return undefined; | ||
| const length = Math.min(size, CHROMIUM_LOG_TAIL_BYTES); | ||
| const buf = Buffer.alloc(length); | ||
| readSync(fd, buf, 0, length, size - length); | ||
| return buf.toString("utf8"); | ||
| } catch { | ||
| return undefined; | ||
| } finally { | ||
| if (fd !== undefined) { | ||
| try { | ||
| closeSync(fd); | ||
| } catch { | ||
| // Best-effort close | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Chromium log file grows unboundedly
chromium.log has no size limit or rotation, unlike main.log which is capped at 10 MB with 3 archives. With --log-level=0 (VERBOSE), Chromium is extremely chatty during normal operation and can produce several megabytes per session. Over time this will silently consume disk space with no cleanup mechanism. The same log directory already implements rotation for the main log — the same approach (or at least a maxSize guard that truncates the file at startup) should be applied here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/utils/logger.ts
Line: 82-106
Comment:
**Chromium log file grows unboundedly**
`chromium.log` has no size limit or rotation, unlike `main.log` which is capped at 10 MB with 3 archives. With `--log-level=0` (VERBOSE), Chromium is extremely chatty during normal operation and can produce several megabytes per session. Over time this will silently consume disk space with no cleanup mechanism. The same log directory already implements rotation for the main log — the same approach (or at least a `maxSize` guard that truncates the file at startup) should be applied here.
How can I resolve this? If you propose a fix, please make it concise.| readSync(fd, buf, 0, length, size - length); | ||
| return buf.toString("utf8"); |
There was a problem hiding this comment.
readSync return value not used to bound toString
readSync returns the number of bytes actually read, which may be less than length (e.g. if the file was truncated between the fstatSync and readSync calls). Since Buffer.alloc zero-fills, any unread bytes at the end become null bytes in the UTF-8 string. Using bytesRead to slice the buffer is safer:
| readSync(fd, buf, 0, length, size - length); | |
| return buf.toString("utf8"); | |
| const bytesRead = readSync(fd, buf, 0, length, size - length); | |
| return buf.subarray(0, bytesRead).toString("utf8"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/utils/logger.ts
Line: 93-94
Comment:
**`readSync` return value not used to bound `toString`**
`readSync` returns the number of bytes actually read, which may be less than `length` (e.g. if the file was truncated between the `fstatSync` and `readSync` calls). Since `Buffer.alloc` zero-fills, any unread bytes at the end become null bytes in the UTF-8 string. Using `bytesRead` to slice the buffer is safer:
```suggestion
const bytesRead = readSync(fd, buf, 0, length, size - length);
return buf.subarray(0, bytesRead).toString("utf8");
```
How can I resolve this? If you propose a fix, please make it concise.| const chromiumLogDir = path.join( | ||
| os.homedir(), | ||
| ".posthog-code", | ||
| isDev ? "logs-dev" : "logs", | ||
| ); |
There was a problem hiding this comment.
Log directory path duplicated (OnceAndOnlyOnce)
The same expression — join(os.homedir(), ".posthog-code", isDev ? "logs-dev" : "logs") — already appears verbatim in logger.ts as LOG_DIR. If the log directory ever changes, both sites need updating in sync. Consider extracting a shared helper (e.g. a small utils/paths.ts exporting getLogDir(isDev)) that both bootstrap.ts and logger.ts can import without creating a circular dependency.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/bootstrap.ts
Line: 47-51
Comment:
**Log directory path duplicated (OnceAndOnlyOnce)**
The same expression — `join(os.homedir(), ".posthog-code", isDev ? "logs-dev" : "logs")` — already appears verbatim in `logger.ts` as `LOG_DIR`. If the log directory ever changes, both sites need updating in sync. Consider extracting a shared helper (e.g. a small `utils/paths.ts` exporting `getLogDir(isDev)`) that both `bootstrap.ts` and `logger.ts` can import without creating a circular dependency.
How can I resolve this? If you propose a fix, please make it concise.
Problem
https://posthog.slack.com/archives/C09G8Q32R6F/p1777640468457519
Changes
adds chromium logs to
~/.posthog-code/logs{-dev}/chromium.logand pushes a tail to posthog sometimesHow did you test this?
manually
Publish to changelog?
no