Skip to content

Conversation

@yashlahase
Copy link

Fix incorrect moment.js usage in time formatting functions

Summary

This PR fixes a bug in the time formatting functions where moment().millisecond() was incorrectly used instead of moment().add() to format durations. The millisecond() method only sets the millisecond component (0-999), not total milliseconds, which could cause incorrect time formatting for durations over 1 second.

Problem

Both main/utils/format-time.ts and renderer/utils/format-time.js use moment().startOf('day').millisecond(time * 1000) to format time durations. This is semantically incorrect because:

  • millisecond() sets only the millisecond component (0-999), not total milliseconds
  • For example, if time = 65 seconds, time * 1000 = 65000 milliseconds
  • millisecond(65000) would set milliseconds to 65000 % 1000 = 0, losing the actual duration

Solution

Replace moment().startOf('day').millisecond(time * 1000) with moment().startOf('day').add(time, 'seconds') in both files. This correctly adds the duration as seconds to a moment at the start of the day, which can then be formatted properly.

Changes

  • ✅ Fixed main/utils/format-time.ts (line 19)
  • ✅ Fixed renderer/utils/format-time.js (line 19)
  • ✅ Added comprehensive test coverage in test/format-time.ts

Testing

Added tests covering:

  • Seconds formatting (0, 5, 30, 65, 125 seconds)
  • Minutes and seconds (60, 90, 3665 seconds)
  • Hours formatting (3600, 3661, 7325 seconds)
  • Milliseconds display
  • Extra time parameter handling

Why This Is Safe

  1. Semantic Fix: Corrects incorrect API usage to use proper moment.js API
  2. No Breaking Changes: Function signature and return format unchanged
  3. Minimal Surface Area: Only 2 lines changed across 2 files
  4. Well-Tested: Comprehensive test coverage added
  5. Type-Safe: No type changes required

The formatTime function incorrectly used moment().millisecond() which
only sets the millisecond component (0-999), not total milliseconds.
This could cause incorrect time formatting for durations over 1 second.

Replace millisecond(time * 1000) with add(time, 'seconds') to properly
add the duration to a moment at the start of the day.

Fixes incorrect behavior in:
- main/utils/format-time.ts
- renderer/utils/format-time.js

Adds comprehensive test coverage in test/format-time.ts
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.

1 participant