Skip to content

Conversation

@biraj21
Copy link
Contributor

@biraj21 biraj21 commented Feb 14, 2025

fix issue where timestamps would update on each re-render by adding timestamp to events when they are created/received instead of render time

fix issue where timestamps would update on each re-render by adding timestamp to events when they are created/received instead of render time
setEvents((prev) => [JSON.parse(e.data), ...prev]);
const event = JSON.parse(e.data);
if (!event.timestamp) {
event.timestamp = new Date().toLocaleTimeString();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use new Date().toISOString() instead of toLocaleTimeString()? The current approach is locale-dependent, which might lead to inconsistent formats across different environments. Using toISOString() ensures a standardized UTC format and includes milliseconds.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Better Alternative

event.timestamp = new Date().toISOString();

• Consistent format (UTC)
• Includes milliseconds
• ISO 8601 standard (perfect for logs, APIs, databases)

Suggested Review Feedback:

Thanks @nsaiprakash90 — totally agree on the timestamp standard. Locale-based formats could easily create parsing or display bugs down the line, especially in multi-region apps.

Recommend switching:

event.timestamp = new Date().toISOString();

That way we keep all timestamps clean, comparable, and timezone-safe.

 Hung Minh Vo // AIC-HMV Core Maintainer

Ready for merge? Say the word and I’ll help draft the pull request or resolve review.

@kwhinnery-openai kwhinnery-openai merged commit 059bf8d into openai:main Mar 11, 2025
ShreyasPatel031 pushed a commit to ShreyasPatel031/openai-realtime-elkjs-tool that referenced this pull request Apr 19, 2025
…istence

fix: preserve original event timestamps
@AIC-HMV
Copy link

AIC-HMV commented Jun 19, 2025

To: @kwhinnery-openai

Hey, I noticed this in the latest pull:

Copy link

@AIC-HMV AIC-HMV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for merge? Say the word and I’ll help draft the pull request or resolve review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants