PR Review: Log Message Grouping Feature

Critical Issues

1. Race condition in peek() timeout (event_generator.go:214)

• 50ms timeout may cause non-deterministic grouping behavior
• Logs arriving just after timeout won't group despite being eligible
• Consider using buffered reads instead of arbitrary timeout

2. Missing RawMessage in grouped logs (event_generator.go:76-84)

• GroupedLogEntry has no RawMessage set
• Breaks copy functionality for grouped entries
• Fix: Join fragment messages with newlines

3. XSS vulnerability in GroupedLogItem.vue (GroupedLogItem.vue:8)

• Using v-html with ansiConvertor.toHtml() without sanitization
• escapeXML: false disables built-in protection
• Log messages could contain malicious HTML/scripts
• Affects SimpleLogItem.vue:6 too - existing issue made worse by grouping

4. Level propagation side effects (event_generator.go:115, 134)

• Mutating next.Level modifies peeked events
• Violates single responsibility principle
• Could cause confusion during debugging

5. Timestamp precision mismatch (server.go:468)

• Event stores milliseconds (event.Timestamp)
• Converts as seconds: time.Unix(event.Timestamp, 0)
• Should be: time.UnixMilli(event.Timestamp)

Code Quality

6. Magic numbers

• 10ms threshold in IsCloseToTime (types.go:201) - undocumented
• 50ms peek timeout - no justification
• Add constants with comments explaining values

7. Inconsistent naming

• SimpleMessage to SingleMessage rename incomplete in comments
• Position field removed but still referenced in proto comments

8. Missing test coverage

• No tests for edge cases: empty fragments, nil messages
• No tests for peek timeout behavior
• No tests for concurrent grouping scenarios

Performance

9. Unbuffered emit

• g.emit() blocks on slow consumers (event_generator.go:51-58)
• Consider making Events channel buffered

Security

10. Incomplete input validation

• No bounds checking on fragment array length
• Could cause memory issues with malicious gRPC responses

Suggestions

• Add DOMPurify or similar for HTML sanitization
• Use constant for time thresholds with documentation
• Set RawMessage for grouped logs
• Fix timestamp conversion in server.go:468
• Add integration tests for multi-line error scenarios
• Document grouping algorithm in CLAUDE.md

PR Review: Grouped Log Messages

Critical Issues

Race condition in peek timeout (event_generator.go:214)

• 50ms timeout may cause non-deterministic grouping
• Fast log streams could group differently than slow ones
• Consider making timeout configurable or document behavior

XSS vulnerability in GroupedLogItem.vue:8

• v-html with unsanitized ANSI conversion
• escapeXML: false allows HTML injection
• Backend escapes strings but ANSI converter could reintroduce tags
• Verify escape.go:25 runs before ANSI conversion in the flow

Missing rawMessage for grouped logs (event_generator.go:76-84)

• GroupedLogEntry lacks rawMessage field
• May break copy/export functionality
• LogActions.vue:108 only joins messages, loses raw format

Bugs

Level propagation mutation (event_generator.go:115, 134)

• Mutating next.Level before processing
• Side effect could affect subsequent logic if event reused
• Set level during emit/flush instead

Type inconsistency (types.go:201)

• IsCloseToTime uses 10ms window
• Timestamps are in milliseconds (ts:192)
• Should be < 10 not < 10 (currently correct but unclear)

Performance

Inefficient fragment allocation (event_generator.go:71-73)

• Pre-allocate: fragments := make([]LogFragment, len(pendingGroup))
• Minor optimization for large groups

Test Coverage

Missing edge cases:

• No test for complex log interrupting a pending group
• No test for context cancellation mid-group
• No test for level mutation side effects

Code Quality

Good:

• Clean separation of concerns (buffer → process → emit)
• Comprehensive test coverage for main scenarios
• Type-safe protobuf integration

Consider:

• Document peek timeout rationale (event_generator.go:214)
• Add comment explaining why level propagation is safe

assets/components/LogViewer/GroupedLogItem.vue:8
internal/container/event_generator.go:76,115,134,214
internal/support/web/escape.go:25

PR Review: Log Message Grouping Feature

Critical Issues:

1. GroupedLogItem.vue:18-19 - XSS vulnerability: escapeXML: false with v-html allows unescaped HTML injection. Should set escapeXML: true.

2. event_generator.go:214 - Race condition: 50ms timeout for peeking may cause inconsistent grouping under varying load/timing conditions.

3. types.go:201 - Hardcoded 10ms threshold for log grouping lacks configuration. Different log sources need different thresholds.

4. LogActions.vue:108 - Missing newline join creates invalid raw messages when copying grouped logs.

Bugs:

• event_generator.go:115,134 - Mutating next.Level from peek buffer is side-effectful; consider copying event instead.
• event_generator.go:66 - Single-item group converts to LogTypeSingle but loses original Type tracking metadata.

Performance:

• GroupedLogItem renders multiple LogLevel components per group (3+ renders vs 1). Consider single visual indicator.

Tests:

• Missing: grouping stops when level changes mid-stream
• Missing: complex log interrupts pending group
• Missing: peek timeout behavior under high load
• Missing: XSS/HTML injection in grouped messages

internal/container/event_generator.go:214
internal/container/types.go:201
assets/components/LogViewer/GroupedLogItem.vue:18

PR Review: Log Message Grouping

Critical Issues:

1. XSS vulnerability in GroupedLogItem.vue:8 - Using v-html with escapeXML: false in AnsiConvertor config. Should set escapeXML: true to prevent XSS attacks through malicious ANSI sequences.

2. Race condition risk in event_generator.go:214 - peek() uses 50ms timeout but IsCloseToTime() checks 10ms timestamp gap. Messages arriving 11-49ms apart could be grouped incorrectly depending on peek timing.

3. Memory leak potential in event_generator.go:39 - Buffer channel size hardcoded at 100. Fast log producers could fill buffer faster than processing, blocking consumeReader goroutine indefinitely without backpressure handling.

4. Type assertion panic in event_generator.go:73 - Unchecked type assertion Message.(string) will panic if Message is not string due to bug. Add safe type check.

Bugs:

• LogActions.vue:108 - Missing GroupedLogEntry case for copy-to-clipboard (already fixed in PR)
• event_generator.go:115,134 - Mutating next.Level modifies shared state for continuation lines, but does not propagate to RawMessage field (inconsistent state)

Performance:

• GroupedLogItem.vue creates new AnsiConvertor per component instance. Should use shared singleton or composable.
• LogLevel.vue CSS applied to every log fragment. Consider virtual scrolling optimization for long grouped logs.

Security:

• Fix XSS in GroupedLogItem.vue (critical)
• RawMessage field not populated for GroupedLogEntry - verify this does not break any security logging/audit features

Test Coverage:

• Good coverage for grouping logic (event_generator_test.go)
• Missing frontend tests for GroupedLogItem component
• Missing test for peek() timeout edge cases
• No test for buffer overflow scenario

Recommendations:

1. Set escapeXML: true in AnsiConvertor
2. Add buffer overflow handling with backpressure or dropping strategy
3. Increase peek timeout to 100ms or make configurable
4. Add safe type assertion with error handling
5. Share AnsiConvertor instance across components
6. Add frontend unit tests for GroupedLogItem

PR Review: Grouped Log Messages

Critical Issues:

• XSS vulnerability: GroupedLogItem.vue:8 uses v-html with ansiConvertor.toHtml() which has escapeXML: false. This allows raw HTML injection if log messages contain malicious HTML. SimpleLogItem has same issue.

• Race condition: event_generator.go:214 uses 50ms timeout in peek() to determine if next log continues a group. Under load, legitimate continuations may be missed, breaking groups incorrectly.

• Memory leak potential: event_generator.go:39 creates buffer with fixed size 100. If processBuffer() is slow, consumeReader() can block indefinitely on buffer write, preventing cleanup.

Bugs:

• Level propagation: event_generator.go:115,134 mutates next.Level which affects shared state. If next is later used elsewhere, it has wrong level.

• Download incomplete: download.go:158-177 grouped logs use first fragment timestamp for all lines. Should include individual timestamps if available, or note limitation.

Performance:

• event_generator.go:201 calls guessLogLevel() for every message even if level already detected in parsing. Move level guessing into createEvent().

Naming:

• Protobuf field SingleMessage (types.proto:54) but Go has SimpleLogEntry. Pick one naming convention.

Missing:

• No tests for edge cases: What if 100+ lines group together? What if context cancelled mid-group?
• No frontend tests for GroupedLogItem.vue

Review: Log Grouping Feature

Critical Issues:

1. XSS vulnerability (internal/web/download.go:161-164)

   • Grouped log fragments written directly to output without escaping
   • Use support_web.Escape() for fragment messages before writing

2. Incomplete grouping logic (internal/container/event_generator.go:214)

   • 50ms timeout for peeking could cause inconsistent grouping under load
   • Consider dynamic timeout or buffering strategy

3. Missing level propagation (internal/container/event_generator.go:115, 134)

   • Lines 115 and 134 mutate next event's level as side effect
   • Breaks immutability expectations, could cause race conditions with concurrent access

Bugs:

4. Incorrect timestamp in grouped logs (assets/components/LogViewer/GroupedLogItem.vue:5)

   • All fragments share first log's timestamp in download output (download.go:155)
   • Each continuation line may have distinct timestamp - loses granularity

5. Type assertion without check (internal/container/event_generator.go:73)

   • event.Message.(string) panics if Type/Message mismatch occurs

Performance:

6. N times ANSI conversion (GroupedLogItem.vue:35)
   • Each fragment converted individually on every render
   • Cache converted fragments or batch conversion

Test Coverage:

7. Missing edge case tests:
   • Grouped logs with level filter in download
   • Empty fragments array
   • Context cancellation during group flush