Log tab displays no messages: refactor logging to use JSON formatter instead of parsing formatted text - #254
Conversation
…tted text - Configure JSON formatter in logger Initialize() and Activate() functions - Replace text parsing with JSON parsing in loggingMultiWriter.Write() - Add mapStringToLogLevel() helper to map string levels to constants - Maintain structured data throughout logging pipeline - Handle JSON parsing errors gracefully without failing writes - Log files now use JSON format for machine-readable structured logs Fixes log tab display issue where logs were not appearing due to flawed text parsing architecture. The system now uses structured JSON throughout the pipeline eliminating parsing errors. Closes #252 Signed-off-by: Joseph Brinkman <joe.brinkman@improving.com>
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe logging pipeline now emits JSON from both logger initialization paths. The TUI writer parses JSON payloads, maps levels, preserves metadata, and adds structured entries to the ring buffer while continuing to write file output. ChangesJSON logging pipeline
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/tui.go (1)
1321-1361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused struct and simplify field extraction.
The
entrystruct and its JSON tags are unused because the code manually extracts fields from therawEntrymap. Furthermore, theTimefield is extracted but never used sinceringBuffer.Addgenerates its own timestamp. You can simplify this by extractinglevelandmessagedirectly into local string variables.♻️ Proposed refactor to remove the unused struct
- // Parse JSON structured log entry - var entry struct { - Time string `json:"time"` - Level string `json:"level"` - Message string `json:"message"` - Fields map[string]interface{} `json:"-"` // Capture remaining fields - } - - // Unmarshal into a map first to capture all fields + // Unmarshal into a map to capture all fields var rawEntry map[string]interface{} if err := json.Unmarshal(p, &rawEntry); err != nil { // JSON parse failed, but file write succeeded - don't fail the write // This could happen during transition or with malformed input return n, nil } - // Extract known fields - if t, ok := rawEntry["time"].(string); ok { - entry.Time = t - } - if l, ok := rawEntry["level"].(string); ok { - entry.Level = l - } - if m, ok := rawEntry["message"].(string); ok { - entry.Message = m - } + var levelStr, message string + if l, ok := rawEntry["level"].(string); ok { + levelStr = l + } + if m, ok := rawEntry["message"].(string); ok { + message = m + } // Map string level to log.Level constant - level := mapStringToLogLevel(entry.Level) + level := mapStringToLogLevel(levelStr) // Convert remaining fields to key-value pairs (exclude time, level, message) var keyvals []interface{} for k, v := range rawEntry { if k != "time" && k != "level" && k != "message" { keyvals = append(keyvals, k, v) } } // Add structured entry to ring buffer - lmw.ringBuffer.Add(level, entry.Message, keyvals...) + lmw.ringBuffer.Add(level, message, keyvals...)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/tui.go` around lines 1321 - 1361, Remove the unused entry struct and its JSON tags from the log parsing flow. In the code around rawEntry and lmw.ringBuffer.Add, extract only the level and message values into local string variables, omit time extraction entirely, and pass those locals through the existing mapStringToLogLevel and ringBuffer.Add calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/tui.go`:
- Around line 1321-1361: Remove the unused entry struct and its JSON tags from
the log parsing flow. In the code around rawEntry and lmw.ringBuffer.Add,
extract only the level and message values into local string variables, omit time
extraction entirely, and pass those locals through the existing
mapStringToLogLevel and ringBuffer.Add calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9acaa910-dbeb-475e-8b94-74eca363c4cc
📒 Files selected for processing (3)
.kiro-krew/specs/issue-252-logging-json-formatter.mdinternal/logging/logger.gointernal/tui/tui.go
There was a problem hiding this comment.
Pull request overview
This PR fixes the “log tab displays no messages” issue by switching the logging pipeline to JSON output (via charmbracelet/log’s JSON formatter) and updating the TUI’s loggingMultiWriter to parse structured JSON instead of attempting to re-parse formatted text.
Changes:
- Configure the global logger to emit JSON (
log.JSONFormatter) during initialization and activation. - Refactor
loggingMultiWriter.Write()tojson.Unmarshallog entries and forward structured level/message/metadata into the ring buffer. - Add a design/spec document capturing the rationale and intended architecture for issue #252.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/tui/tui.go | Replace fragile text parsing with JSON parsing and map JSON log level/message/fields into the ring buffer. |
| internal/logging/logger.go | Configure the global logger to use log.JSONFormatter so downstream sinks can parse reliably. |
| .kiro-krew/specs/issue-252-logging-json-formatter.md | Document the design, acceptance criteria, and validation strategy for the JSON logging refactor. |
Comments suppressed due to low confidence (1)
internal/logging/logger.go:114
Activate()only setsJSONFormatterwhenglobalLoggeris nil. If the logger already exists (e.g., created elsewhere with a different formatter), activating handlers could reintroduce non-JSON output and break the TUI’s JSON parsing. Setting the formatter in the existing-logger branch makes activation robust and keeps the log pipeline consistently structured.
} else {
// Add new handlers to existing logger
globalLogger.SetOutput(output)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var entry struct { | ||
| Time string `json:"time"` | ||
| Level string `json:"level"` | ||
| Message string `json:"message"` | ||
| Fields map[string]interface{} `json:"-"` // Capture remaining fields | ||
| } | ||
|
|
||
| // Unmarshal into a map first to capture all fields | ||
| var rawEntry map[string]interface{} | ||
| if err := json.Unmarshal(p, &rawEntry); err != nil { | ||
| // JSON parse failed, but file write succeeded - don't fail the write | ||
| // This could happen during transition or with malformed input | ||
| return n, nil | ||
| } | ||
|
|
||
| // Extract known fields | ||
| if t, ok := rawEntry["time"].(string); ok { | ||
| entry.Time = t | ||
| } | ||
| if l, ok := rawEntry["level"].(string); ok { | ||
| entry.Level = l | ||
| } | ||
| if m, ok := rawEntry["message"].(string); ok { | ||
| entry.Message = m | ||
| } |
| var keyvals []interface{} | ||
| for k, v := range rawEntry { | ||
| if k != "time" && k != "level" && k != "message" { | ||
| keyvals = append(keyvals, k, v) | ||
| } | ||
| } |
- Remove unused entry struct and its Time/Fields members - Extract only level and message into local variables - Normalize float64 values that are integers back to int64 Addresses Copilot and CodeRabbit review feedback on PR #254: - Eliminates dead code (unused struct fields) - Preserves original metadata types for integer values - Simplifies the parsing flow Signed-off-by: Joseph Brinkman <joe.brinkman@improving.com>
Summary
This PR refactors the logging architecture to fix the issue where logs were not appearing in the log tab display. The root cause was a flawed architecture where the logger formatted logs as text, then
loggingMultiWriterattempted to parse that formatted text back into structured data with incorrect field indexing.Changes Made
Core Architecture Fix
log.SetFormatter(log.JSONFormatter)in bothInitialize()andActivate()functionsloggingMultiWriter.Write()to parse JSON instead of formatted textencoding/jsonimport andmapStringToLogLevel()helper functionKey Files Modified
internal/logging/logger.go: Added JSON formatter configuration (2 lines)internal/tui/tui.go: Replaced text parsing with JSON parsing (~70 lines changed)Architecture Before (Flawed)
Architecture After (Fixed)
Quality Assurance
All automated QA checks pass:
task fmt:check(go fmt)task sync:check(template synchronization)task lint(go vet static analysis)task test(22/22 logging tests, 52/52 tui tests)task build(binary compilation)Breaking Changes
Log File Format: Log files now use JSON format instead of text format. This provides structured, machine-readable logs that can be easily parsed by tools like
jqor log aggregators.Example log file output:
{"time":"2026-07-15T09:30:00-04:00","level":"info","message":"Watcher started","repo":"owner/name"} {"time":"2026-07-15T09:30:01-04:00","level":"debug","message":"Polling GitHub","interval":"5m"}For human-readable viewing, users can pipe through
jq:Validation
Automated Validation: All technical criteria validated successfully
Manual Validation Required: TUI display testing (log tab appearance, color-coded levels, metadata display)
Testing
To verify the fix:
task build && ./dist/kiro-krewwatch startcat .kiro-krew/kiro-krew.logCloses #252
Summary by CodeRabbit
New Features
Bug Fixes