Devmode: Improved logging output and sourcemap support #321
Conversation
WalkthroughThe changes introduce severity-aware logging and output stream separation in the TUI logger system. Log messages are now styled and routed based on severity (info, warning, error, etc.) and output type (stdout, stderr). Logging interfaces, constructors, and command creation functions are updated to support these changes, enabling clearer distinction and display of standard and error logs. Additionally, source map support is enhanced for Node.js and Bun runtimes with a new shim and conditional package installation. The GitHub Actions workflow runner was also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DevCmd
participant TuiLogger (stdout)
participant TuiLogger (stderr)
participant DevModeUI
User->>DevCmd: Run dev command
DevCmd->>TuiLogger (stdout): Log info/debug messages (stdout)
DevCmd->>TuiLogger (stderr): Log error messages (stderr)
TuiLogger (stdout)->>DevModeUI: AddLog(INFO/DEBUG, message)
TuiLogger (stderr)->>DevModeUI: AddErrorLog(ERROR, message)
DevModeUI->>User: Display styled logs (info, warning, error)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)internal/bundler/bundler.go (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
internal/dev/pending_logger.go (1)
157-159: Consider updating Fatal method for consistency.The
Fatalmethod still uses a plain string with a[FATAL]prefix rather than the structured approach used by other methods. Consider updating it to use the same structured approach for consistency, though it may not be necessary since it exits immediately.- val := tui.Bold("[FATAL] " + fmt.Sprintf(msg, args...)) - fmt.Println(val) + // Create structured log entry for consistency + val := pendingLog{ + level: logger.LevelFatal, + msg: fmt.Sprintf(msg, args...), + } + // Still print with bold for immediate visibility + fmt.Println(tui.Bold("[FATAL] " + val.msg))internal/dev/tui.go (1)
523-535: Errors still go to stdout when TUI is disabledWhen the UI is disabled we fall back to
fmt.Println, which writes to stdout even forlogger.LevelError.
That defeats the purpose of theStdout/Stderrsplit you introduced in the logger.-if !d.enabled { - fmt.Println(fmt.Sprintf(log, args...)) +if !d.enabled { + dst := os.Stdout + if severity >= logger.LevelError { + dst = os.Stderr + } + fmt.Fprintln(dst, fmt.Sprintf(log, args...)) return }This keeps normal logs on stdout while allowing CI pipelines or users to redirect errors by reading stderr.
internal/dev/tui_logger.go (1)
155-159: Preserve original severity when reading from stderrAll stderr writes are pushed through
AddErrorLog, losing the parsed severity (e.g.,[WARN]emitted on stderr becomes an “error” in the UI).
If you want severity-aware colouring even for stderr, callAddLog(severity, ...)and let the UI decide whether to style it as an error.- l.ui.AddErrorLog("%s", log) + l.ui.AddLog(severity, "%s", log)Alternatively, reserve stderr exclusively for true errors and document the expectation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
.github/workflows/release.yml(2 hunks)cmd/dev.go(3 hunks)internal/dev/dev.go(2 hunks)internal/dev/pending_logger.go(8 hunks)internal/dev/server.go(1 hunks)internal/dev/tui.go(4 hunks)internal/dev/tui_logger.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
cmd/dev.go (2)
internal/dev/tui_logger.go (3)
NewTUILogger(30-32)Stdout(20-20)StdErr(19-19)internal/dev/dev.go (1)
CreateRunProjectCmd(107-138)
internal/dev/server.go (2)
internal/dev/tui.go (1)
DevModeUI(414-429)internal/dev/pending_logger.go (1)
PendingLogger(18-23)
internal/dev/tui_logger.go (1)
internal/dev/tui.go (1)
DevModeUI(414-429)
🪛 actionlint (1.7.7)
.github/workflows/release.yml
16-16: label "blacksmith-4vcpu-ubuntu-2204" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
41-41: input "subject-checksums-type" is not defined in action "actions/attest-build-provenance@v2". available inputs are "github-token", "push-to-registry", "show-summary", "subject-digest", "subject-name", "subject-path"
(action)
🔇 Additional comments (15)
internal/dev/server.go (1)
550-554: Improved error logging handling.The
Connectmethod now accepts a separate error logger (tuiLoggerErr) and properly drains any pending error logs to the UI before standard logs. This separation will improve visibility of errors in the TUI.internal/dev/dev.go (2)
107-107: Function signature updated to support separate stdout and stderr streams.The
CreateRunProjectCmdfunction now properly separates standard output and error output by accepting distinct writers for each stream, which aligns with the improved logging architecture.
131-132: Output streams properly wired to separate writers.The command now correctly directs its standard output and error output to the appropriate writers, enabling proper separation and styling of logs by severity.
cmd/dev.go (4)
127-129: Separate loggers created for stdout and stderr.Two distinct TUI logger instances are now created for standard output and error output using the appropriate I/O types. This enhances log clarity by separating standard logs from error logs.
130-130: Server connection now utilizes both loggers.The server's
Connectmethod is properly called with both standard and error loggers, enabling the separation of output streams during the connection process.
143-143: Project command properly configured with separate output streams.The call to
CreateRunProjectCmdnow correctly passes both the standard and error loggers as writers, ensuring consistent separation of output streams.
245-245: Consistent use of separate loggers during project restart.The project restart logic maintains the separation of standard and error output streams, ensuring consistent logging behavior throughout the application lifecycle.
internal/dev/pending_logger.go (8)
13-16: New structure for better log level tracking.The new
pendingLogstruct properly encapsulates both the log level and message, enabling more structured log handling and severity-based presentation.
19-19: Updated pending logs storage to include log levels.The
pendingfield now stores structured log entries with explicit severity levels rather than plain strings, enabling more sophisticated log handling.
38-39: Updated log draining to pass severity information.The
drainmethod now correctly passes both the log level and message to the UI, enabling severity-aware rendering of logs.
70-74: Structured log capturing for trace level.The
Tracemethod now properly creates a structured log entry with explicit severity level information, consistent with the new logging architecture.
88-92: Structured log capturing for debug level.The
Debugmethod now properly creates a structured log entry with explicit severity level information, consistent with the new logging architecture.
106-110: Structured log capturing for info level.The
Infomethod now properly creates a structured log entry with explicit severity level information, consistent with the new logging architecture.
124-128: Structured log capturing for warning level.The
Warnmethod now properly creates a structured log entry with explicit severity level information, consistent with the new logging architecture.
142-146: Structured log capturing for error level.The
Errormethod now properly creates a structured log entry with explicit severity level information, consistent with the new logging architecture.
| jobs: | ||
| release: | ||
| runs-on: windows-latest # required to use wix | ||
| runs-on: blacksmith-4vcpu-ubuntu-2204 |
There was a problem hiding this comment.
Verify runner label validity for runs-on.
Actionlint flags blacksmith-4vcpu-ubuntu-2204 as unknown. If this is intended to target a custom self-hosted runner, ensure the runner is configured with that label and include the self-hosted scope (e.g., runs-on: [self-hosted, blacksmith-4vcpu-ubuntu-2204]). Otherwise, switch to an officially supported label such as ubuntu-22.04 or ubuntu-latest.
🧰 Tools
🪛 actionlint (1.7.7)
16-16: label "blacksmith-4vcpu-ubuntu-2204" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
In .github/workflows/release.yml at line 16, the runner label
'blacksmith-4vcpu-ubuntu-2204' is flagged as unknown by Actionlint. To fix this,
if this is a custom self-hosted runner, update the runs-on field to include
'self-hosted' along with the label as an array (e.g., runs-on: [self-hosted,
blacksmith-4vcpu-ubuntu-2204]). Otherwise, replace the label with a standard
supported label like 'ubuntu-22.04' or 'ubuntu-latest'.
| // Add an error log message to the log list | ||
| func (d *DevModeUI) AddErrorLog(log string, args ...any) { | ||
| if !d.enabled { | ||
| fmt.Println(errorStyle.Render(fmt.Sprintf(log, args...))) | ||
| return | ||
| } | ||
| raw := fmt.Sprintf(log, args...) | ||
| d.program.Send(addLogMsg{ | ||
| timestamp: time.Now(), | ||
| severity: logger.LevelError, | ||
| raw: raw, | ||
| message: errorStyle.Render(strings.ReplaceAll(ansiColorStripper.ReplaceAllString(raw, ""), "\n", " ")), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate styling & print to os.Stderr
message:already receiveserrorStyle.Render(...)andTitle()styles it again – see previous comment.- Similar to
AddLog, the fallback path should targetos.Stderr, otherwise colourised errors mingle with normal output.
- fmt.Println(errorStyle.Render(fmt.Sprintf(log, args...)))
+ fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf(log, args...)))Long-running scripts that grep stdout will appreciate the separation.
🤖 Prompt for AI Agents
In internal/dev/tui.go around lines 538 to 551, the AddErrorLog method applies
errorStyle.Render twice due to previous styling in Title(), causing duplicate
styling; remove the redundant errorStyle.Render call on the message field. Also,
change the fallback print path to write the formatted error message to os.Stderr
instead of stdout to separate error output from normal output, improving
usability in long-running scripts.
| func (l *TuiLogger) Write(p []byte) (n int, err error) { | ||
| trimmed := bytes.Split(p, eol) | ||
| l.pending.Write(p) | ||
|
|
||
| if !bytes.HasSuffix(l.pending.Bytes(), eol) { | ||
| return len(p), nil | ||
| } | ||
|
|
||
| trimmed := bytes.Split(l.pending.Bytes(), eol) | ||
| l.pending.Reset() | ||
|
|
||
| for _, line := range trimmed { | ||
| if len(line) == 0 { |
There was a problem hiding this comment.
Buffered write logic can lose data when the last chunk lacks \n
bytes.Split(l.pending.Bytes(), eol) splits the entire buffer, then pending.Reset() clears it, so the partial line after the final newline is lost.
Example flow:
- Write “abc\n”, buffer processed, fine.
- Write “def” (no newline) →
HasSuffixfalse → return. - Write “\nghi\n” → buffer now “def\nghi\n”,
HasSuffixtrue →Splitproduces["def", "ghi", ""]→ both lines processed! but"def"was never terminated by\nin the stream.
Consider using bufio.Scanner-like logic: process only the complete lines and keep the last unterminated chunk in pending.
lines := bytes.Split(buffer, eol)
if !bytes.HasSuffix(buffer, eol) {
pending.Write(lines[len(lines)-1])
lines = lines[:len(lines)-1]
}This preserves data integrity.
🤖 Prompt for AI Agents
In internal/dev/tui_logger.go around lines 107 to 118, the current Write method
clears the entire pending buffer after splitting by newline, causing loss of the
last partial line if it lacks a trailing newline. To fix this, modify the logic
to only process complete lines by splitting the buffer on newline, then if the
buffer does not end with a newline, keep the last partial line in pending and
process only the fully terminated lines. This ensures no data is lost between
writes.
jhaynie
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/bundler/shim.go (1)
70-76: Consider a more robust stack-frame regex
const frameRegex = /(.+)\((.+):(\d+):(\d+)\)$/assumes:
- every frame ends with
(...)- Windows paths never appear (
C:\foo\bar.js) – the colon inside the drive letter would break the capture groups- no anonymous frames / “at file:line:col” format
If any of these occur, the line will silently pass through unmapped.
Recommend tightening the pattern with a look-behind foratand allowing an optional drive letter:const frameRegex = /at (?:.+? )?\(?([A-Z]:)?([^):]+):(\d+):(\d+)\)?$/;Not urgent, but will improve resilience across platforms.
internal/bundler/bundler.go (1)
154-159: Missing newline between concatenated banners may break “use strict” pragmas
jsheader,jsshim, andpostShimare concatenated without explicit newlines:"js": jsheader + jsshim + postShim,If any of these strings forget their trailing
\n, the last line of one shim can run into the first line of the next, producing invalid JS (especially for immediately-invoked IIFEs).
Safer to join with\n:- "js": jsheader + jsshim + postShim, + "js": strings.Join([]string{jsheader, jsshim, postShim}, "\n"),(or ensure each constant ends with
\n).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
internal/bundler/bundler.go(4 hunks)internal/bundler/shim.go(2 hunks)internal/dev/dev.go(2 hunks)internal/dev/tui_logger.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/dev/dev.go
- internal/dev/tui_logger.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test (windows-latest)
| const sm = new __agentuity_SourceMapConsumer(__agentuity_readFileSync(filename).toString()); | ||
| __cachedSourceMap[filename] = sm; | ||
| return sm; | ||
| } |
There was a problem hiding this comment.
SourceMapConsumer is used synchronously – this will throw at runtime
source-map-js (same as source-map@0.x) returns a Promise when instantiated with new SourceMapConsumer(...).
Because await is not used (and getSourceMap is not declared async), sm will be a pending promise, so the subsequent call to sm.originalPositionFor will fail with TypeError: sm.originalPositionFor is not a function.
A minimal fix is to await the promise and make getSourceMap async:
-function getSourceMap(filename) {
+async function getSourceMap(filename) {
@@
- const sm = new __agentuity_SourceMapConsumer(
- __agentuity_readFileSync(filename).toString()
- );
+ const sm = await new __agentuity_SourceMapConsumer(
+ __agentuity_readFileSync(filename).toString()
+ );You will then need to:
- Mark every caller (
getSourceMap,Error.prepareStackTrace, etc.)asyncor switch to the synchronous helperSourceMapConsumer.with(...)to keep the stack-trace hook synchronous. - Change the
Error.prepareStackTraceoverride accordingly (e.g. wrap the mapping loop inawait Promise.all(...)).
Without this change, stack-trace rewriting will break on the first error thrown in Bun.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sm = new __agentuity_SourceMapConsumer(__agentuity_readFileSync(filename).toString()); | |
| __cachedSourceMap[filename] = sm; | |
| return sm; | |
| } | |
| async function getSourceMap(filename) { | |
| const sm = await new __agentuity_SourceMapConsumer( | |
| __agentuity_readFileSync(filename).toString() | |
| ); | |
| __cachedSourceMap[filename] = sm; | |
| return sm; | |
| } |
🤖 Prompt for AI Agents
In internal/bundler/shim.go around lines 66 to 69, the instantiation of
SourceMapConsumer is asynchronous and returns a Promise, but the code treats it
synchronously, causing runtime errors. To fix this, make getSourceMap an async
function and await the creation of SourceMapConsumer. Then, update all callers
of getSourceMap and related functions like Error.prepareStackTrace to be async
or refactor to use SourceMapConsumer.with(...) for synchronous handling. Adjust
the stack-trace processing to handle async operations properly, such as using
await Promise.all for mapping loops.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/dev/tui_logger.go (1)
109-120:⚠️ Potential issuePotential data loss when processing partial lines
The current implementation still doesn't properly handle partial lines that lack a trailing newline. When new data is written, any unterminated line from the previous write will be lost because
l.pending.Reset()is called before preserving the last chunk.Apply this fix to preserve partial lines:
trimmed := bytes.Split(l.pending.Bytes(), eol) l.pending.Reset() + + // If the buffer doesn't end with newline, preserve the last chunk + if !bytes.HasSuffix(p, eol) { + if len(trimmed) > 0 { + l.pending.Write(trimmed[len(trimmed)-1]) + trimmed = trimmed[:len(trimmed)-1] + } + }
🧹 Nitpick comments (1)
internal/dev/tui_logger.go (1)
137-162: Consider using a map for log level parsingThe lengthy chain of if-else statements could be replaced with a more maintainable map-based approach.
- if strings.HasPrefix(prefix, "[TRACE]") { - severity = logger.LevelTrace - if logger.LevelTrace < l.logLevel { - continue - } - } else if strings.HasPrefix(prefix, "[DEBUG]") { - severity = logger.LevelDebug - if logger.LevelDebug < l.logLevel { - continue - } - } else if strings.HasPrefix(prefix, "[INFO]") { - severity = logger.LevelInfo - if logger.LevelInfo < l.logLevel { - continue - } - } else if strings.HasPrefix(prefix, "[WARN]") { - severity = logger.LevelWarn - if logger.LevelWarn < l.logLevel { - continue - } - } else if strings.HasPrefix(prefix, "[ERROR]") { - severity = logger.LevelError - if logger.LevelError < l.logLevel { - continue - } + // Map prefix to severity level + prefixToLevel := map[string]logger.LogLevel{ + "[TRACE]": logger.LevelTrace, + "[DEBUG]": logger.LevelDebug, + "[INFO]": logger.LevelInfo, + "[WARN]": logger.LevelWarn, + "[ERROR]": logger.LevelError, + } + + // Find matching prefix + for p, level := range prefixToLevel { + if strings.HasPrefix(prefix, p) { + severity = level + if level < l.logLevel { + continue + } + break + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
.github/workflows/release.yml(1 hunks)internal/bundler/bundler.go(4 hunks)internal/dev/tui.go(4 hunks)internal/dev/tui_logger.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/bundler/bundler.go
- internal/dev/tui.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/dev/tui_logger.go (1)
internal/dev/tui.go (1)
DevModeUI(414-429)
🪛 actionlint (1.7.7)
.github/workflows/release.yml
16-16: label "blacksmith-4vcpu-ubuntu-2204" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
🔇 Additional comments (5)
internal/dev/tui_logger.go (4)
17-22: Nice addition of output type separationThe new
ioTypeenum provides a clear way to differentiate between stderr and stdout streams, which enables better handling of different log types.
28-29: Good synchronization implementationAdding the mutex and pending buffer addresses the thread safety issues mentioned in previous reviews. The
TuiLoggeris now properly synchronized for concurrent usage.
129-136: Robust handling of log prefixesGood job implementing the check for
bracket == -1which addresses the previous index safety issue. This makes the logger more resilient to arbitrary log formats without prefix patterns.
164-168: Great differentiation between stdout and stderr outputsThe differentiated handling of standard output and error streams allows for better visual distinction in the UI, improving the developer experience.
.github/workflows/release.yml (1)
16-16: Verify custom runner label specificationThe
blacksmith-4vcpu-ubuntu-2204runner label is flagged as unknown by the static analysis tool. If this is a custom self-hosted runner, the label should include theself-hostedscope.If this is a self-hosted runner, update the configuration to:
- runs-on: blacksmith-4vcpu-ubuntu-2204 + runs-on: [self-hosted, blacksmith-4vcpu-ubuntu-2204]Otherwise, if this is an internal GitHub-hosted runner specific to your organization, you can safely ignore this comment.
🧰 Tools
🪛 actionlint (1.7.7)
16-16: label "blacksmith-4vcpu-ubuntu-2204" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/dev/tui_logger.go (3)
24-30: Goroutine safety improvements to TuiLogger struct.The addition of a mutex and proper buffer management addresses the thread safety concerns raised in previous reviews.
135-157: Improved log parsing with index safety.The implementation now properly handles the case where a log line doesn't contain a
]delimiter, addressing the previous index safety issue.
128-130:⚠️ Potential issuePotential data loss for partial lines.
The current implementation still resets the entire buffer after splitting, which means if the last chunk doesn't end with a newline, that partial line will be lost.
Apply this fix to preserve partial lines:
- trimmed := bytes.Split(l.pending.Bytes(), eol) - l.pending.Reset() + buffer := l.pending.Bytes() + trimmed := bytes.Split(buffer, eol) + l.pending.Reset() + if !bytes.HasSuffix(buffer, eol) { + // Keep the last partial line in pending + l.pending.Write(trimmed[len(trimmed)-1]) + trimmed = trimmed[:len(trimmed)-1] + }
🧹 Nitpick comments (2)
internal/dev/tui_logger.go (2)
109-117: Well-structured prefix-to-severity mapping.This map provides a clean way to determine log severity from prefixes. Consider adding a default severity level for unrecognized prefixes to avoid potential nil map lookups.
148-156: Inefficient log level filtering logic.The current implementation still processes each log line even if it's below the configured log level, and only skips sending it to the UI. This could be optimized.
Consider moving the level check before processing:
- for p, level := range prefixToLevel { - if strings.HasPrefix(prefix, p) { - severity = level - if level < l.logLevel { - continue - } - break - } - } + for p, level := range prefixToLevel { + if strings.HasPrefix(prefix, p) { + severity = level + break + } + } + if severity < l.logLevel { + continue + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
internal/dev/tui_logger.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/dev/tui_logger.go (1)
internal/dev/tui.go (1)
DevModeUI(414-429)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/dev/tui_logger.go (6)
11-11: Added mutex import for thread safety.Good addition of the sync package to address goroutine safety in the TuiLogger implementation.
17-22: Clean implementation of IO type enum.Introducing this enum to differentiate between stdout and stderr streams is a good design choice, making the code's intent clear.
32-34: Updated constructor includes IO type parameter.The constructor changes properly initialize the logger with the new IO type parameter, maintaining clean dependency injection.
59-59: Consistent severity-aware logging implementation.The logging methods now properly tag messages with their severity level when adding to the UI, which enables better visual distinction in the TUI.
Also applies to: 67-67, 75-75, 83-83, 91-91, 97-97
119-126: Proper buffered handling with mutex protection.The Write method now correctly uses mutex locking and proper buffer handling to ensure thread safety and avoid data corruption when used concurrently.
158-162: Differentiated handling for stdout and stderr.Good implementation of different handling for standard output and error output streams, routing error messages through a dedicated UI method.
Summary by CodeRabbit
New Features
Refactor
Chores