fix: message timestamps not being accurate #521
Conversation
This comment has been minimized.
This comment has been minimized.
- Added logic to set a timestamp for AGUI events in `agui_proxy.go` and `agui.go` if not already present, ensuring accurate message tracking. - Updated `compaction.go` to preserve the timestamp from events when creating messages. - Modified frontend components to utilize the timestamp from backend messages, providing a fallback for legacy messages. This change improves the reliability of message timestamp tracking across the system.
andybraren
force-pushed
the
message-timestamps
branch
from
40ba53b to
cd0df12
Compare
Claude Code ReviewSummaryThis PR adds per-message timestamps to the AGUI event stream, fixing an issue where all message timestamps would change to the time of the most recently received message. The changes ensure that timestamps are set at event creation time and preserved throughout the event processing pipeline (backend → compaction → frontend). Overall Assessment: The implementation is sound and addresses the issue effectively, but there are a few areas that need attention before merging. Issues by Severity🔴 Critical Issues1. Inconsistent timestamp format documentation
// In types/agui.go
const AGUITimestampFormat = time.RFC3339Nano // "2006-01-02T15:04:05.999999999Z07:00"2. Race condition with nil check pattern
// SAFER pattern
timestamp, ok := event["timestamp"].(string)
if \!ok || timestamp == "" {
event["timestamp"] = time.Now().UTC().Format(time.RFC3339Nano)
}🟡 Major Issues3. Missing timestamp validation in frontend
4. Potential timestamp loss in compaction
5. Makefile changes unrelated to timestamps
🔵 Minor Issues6. Inconsistent comment style
7. Missing error handling for time parsing in frontend
function normalizeTimestamp(timestamp: string | undefined): string {
if (\!timestamp) return new Date().toISOString()
try {
// Validate by parsing
const date = new Date(timestamp)
return isNaN(date.getTime()) ? new Date().toISOString() : timestamp
} catch {
return new Date().toISOString()
}
}8. No test coverage for timestamp preservation
Positive Highlights✅ Comprehensive coverage: Timestamps added at all entry points (WebSocket, proxy, compaction) RecommendationsPriority 1 (Must fix before merge)
Priority 2 (Should fix before merge)
Priority 3 (Nice to have)
Architecture Compliance✅ Security: No security issues - timestamps don't contain sensitive data Pre-Merge ChecklistBefore merging, verify:
Testing NotesBased on PR description, you mentioned "Still need to test this with an updated backend". Before merging:
Overall: This is a solid fix for the timestamp issue. The main concerns are the type-unsafe nil checks and the unrelated Makefile changes. Once those are addressed, this should be good to merge. Let me know if you need clarification on any of these points! 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
…mponents - Captured timestamps from AGUI events in the useAGUIStream hook for improved message tracking. This change improves the accuracy and reliability of message timestamps across the application.
Claude Code ReviewSummaryThis PR enhances timestamp handling in the AGUI event stream by ensuring that every event receives a timestamp at the point of creation. The changes address a critical bug where all message timestamps in a session would incorrectly update to the time of the most recently received message. Overall Assessment: Good implementation with proper timestamp propagation, but requires attention to error handling patterns and testing before merge. Issues by SeverityCritical Issues1. Inconsistent Error Handling Pattern (Backend) File: components/backend/websocket/compaction.go:235 The code uses underscore to ignore type assertion failures which violates error handling standards. Type assertion failures could silently produce empty timestamps, defeating the purpose of this PR. Files affected:
Major Issues2. Missing Tests for Timestamp Propagation No test coverage for the core functionality - timestamp preservation through the event pipeline. Expected tests:
Impact: No way to verify timestamps are correctly propagated without manual testing. Regression risk is high. 3. Inconsistent Timestamp Format Documentation No documentation or comments explaining the timestamp format (time.RFC3339Nano). Add comments at injection points explaining the format choice. 4. Fallback Logic Could Mask Backend Issues (Frontend) File: components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx:762 Silent fallback to current time makes it impossible to detect when backend fails to provide timestamps. Consider adding console.warn for debugging. Minor Issues5. Unrelated Makefile Fix The Makefile change (fixing port forwarding) is unrelated to timestamp handling. This should be in a separate PR or mentioned in the PR description. 6. TypeScript Could Be More Defensive File: components/frontend/src/hooks/use-agui-stream.ts:173 Consider adding immediate fallback at the point of capture rather than requiring downstream checks. Positive Highlights✅ Excellent timestamp propagation design - Timestamps are added at the source and preserved through the entire pipeline. ✅ Backward compatibility - All frontend code includes fallbacks for legacy messages without timestamps. ✅ Consistent implementation - Timestamp handling is applied uniformly across all event types. ✅ Proper type definitions - TypeScript types correctly updated with optional timestamp fields. ✅ RFC3339Nano format - Using high-precision timestamps ensures correct message ordering. ✅ Strategic timestamp placement - Adding timestamps in RouteAGUIEvent ensures all events get timestamps regardless of source. RecommendationsPriority 1 (Must Fix Before Merge)
Priority 2 (Should Fix Before Merge)
Code Quality ChecksBackend Standards
Frontend Standards
Security Review✅ No security issues identified Performance Considerations✅ Performance impact is negligible Final VerdictStatus: This is a solid implementation of timestamp tracking that addresses a real UX issue. However, the error handling violations and lack of test coverage need to be addressed before merging to maintain code quality standards. Estimated effort: 2-3 hours (add error handling + basic tests) Merge recommendation: Approve after addressing Priority 1 items. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
|
Timestamps for each message are now passing through the eventstream and appearing in the UI correctly, even after refreshing: 
|
- Introduced constants for AG-UI timestamp formats in `agui.go` to ensure uniformity in timestamp handling. - Updated various components (`agui_proxy.go`, `agui.go`, `legacy_translator.go`) to utilize the new timestamp constants, improving accuracy and reliability in event timestamping. This change enhances the overall consistency of timestamp formatting across the application.
Claude Code ReviewSummaryThis PR adds proper timestamp handling to AGUI (AG-UI) message streaming to fix the issue where all message timestamps update to the most recent message time. The implementation adds timestamp constants, ensures timestamps are captured from events at the backend, and propagates them through to the frontend display layer. Overall Assessment: Good implementation with consistent patterns, but has several issues that should be addressed before merge. Issues by Severity🚫 Blocker IssuesNone - No critical blocking issues that prevent merge. 🔴 Critical Issues1. Missing Tests for Critical Timestamp LogicLocation: All backend files (agui.go, agui_proxy.go, compaction.go) The timestamp logic is critical for correctness but has zero test coverage. This is particularly concerning because:
Recommendation: Add unit tests covering:
Example test structure: func TestRouteAGUIEvent_PreservesTimestamp(t *testing.T) {
expectedTime := "2024-01-15T10:30:45.123456789Z"
event := map[string]interface{}{
"type": types.EventTypeTextMessageStart,
"timestamp": expectedTime,
}
// ... test that timestamp is preserved
}
func TestRouteAGUIEvent_AddsTimestampWhenMissing(t *testing.T) {
event := map[string]interface{}{
"type": types.EventTypeTextMessageStart,
}
// ... test that timestamp is added in RFC3339Nano format
}2. Code Duplication - Repeated Timestamp LogicLocation:
The same 3-line timestamp check/set pattern is repeated verbatim in multiple locations: if event["timestamp"] == nil || event["timestamp"] == "" {
event["timestamp"] = time.Now().UTC().Format(types.AGUITimestampFormat)
}Recommendation: Extract to a helper function following DRY principle: // ensureEventTimestamp adds a timestamp to the event if not present
func ensureEventTimestamp(event map[string]interface{}) {
if event["timestamp"] == nil || event["timestamp"] == "" {
event["timestamp"] = time.Now().UTC().Format(types.AGUITimestampFormat)
}
}This improves maintainability and ensures consistency across all timestamp handling. 🟡 Major Issues3. Inconsistent Optional Field HandlingLocation: The code extracts timestamp with blank identifier: timestamp, _ := event["timestamp"].(string)This silently ignores type assertion failures. While this may be intentional for optional fields, it's inconsistent with the project's type-safety standards documented in CLAUDE.md. Recommendation: Check the boolean return and log unexpected type mismatches: timestamp, ok := event["timestamp"].(string)
if \!ok && event["timestamp"] \!= nil {
log.Printf("Warning: timestamp field has unexpected type: %T", event["timestamp"])
}4. Frontend: Potential Race Condition in Timestamp AssignmentLocation: The code uses
While this is noted in comments as intentional for backward compatibility, it could cause subtle ordering issues if comparing timestamps across new/legacy messages. Recommendation: Add a comment explaining the risk and consider normalizing all timestamps to UTC in a preprocessing step. 5. Unused AGUIMetadataTimestampFormat ConstantLocation: The PR adds Recommendation: Either:
🔵 Minor Issues6. Missing Error Handling DocumentationLocation: The code updates if c.currentMessage.Timestamp == "" && timestamp \!= "" {
c.currentMessage.Timestamp = timestamp
}However, there's no comment explaining WHY we preserve existing timestamps. This could be important for understanding message lifecycle. Recommendation: Add a comment: // Preserve the earliest timestamp (from TEXT_MESSAGE_START) rather than
// overwriting with tool call timestamps
if c.currentMessage.Timestamp == "" && timestamp \!= "" {
c.currentMessage.Timestamp = timestamp
}7. Indentation Inconsistency in FrontendLocation: The indentation was corrected from an incorrect level, but the diff shows the content block now has inconsistent alignment. This appears to be a pre-existing formatting issue that got exposed during the edit. Recommendation: Run 8. Type Safety: Timestamp Field is OptionalLocation: The Recommendation: Consider making it required in the type definition since the backend guarantees it: timestamp: string // Always present after backend fixOr add a comment if it needs to remain optional for backward compatibility: timestamp?: string // Optional for backward compatibility with legacy messagesPositive Highlights✅ Excellent Documentation: The new timestamp format constants have comprehensive JSDoc-style comments explaining when to use each format and why nanosecond precision matters. ✅ Consistent Format Usage: Good use of named constants ( ✅ Comprehensive Frontend Updates: All relevant frontend locations were updated to use the new timestamp field (8 locations in ✅ Backward Compatibility: Thoughtful fallbacks ( ✅ Code Comments: Good inline comments explaining the purpose of timestamp captures (e.g., "Capture timestamp from event"). ✅ Minimal Scope: Changes are focused on the specific problem (timestamp handling) without introducing unrelated refactoring. RecommendationsPriority 1 (Should Fix Before Merge)
Priority 2 (Nice to Have)
Testing StrategySince the PR notes "Still need to test this with an updated backend", recommend:
Code Quality Assessment
Overall: This PR solves a real user-facing bug with a solid approach. The main concerns are lack of test coverage and code duplication, both of which should be addressed before merge to maintain codebase quality standards. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
4 participants
Still need to test this with an updated backend, but this should add per-message timestamps to the eventstream from AGUI, enabling us to show accurate per-message timestamps. Right now all message timestamps in a session change to the time of the most recently received message.