Repository navigation
Conversation
Notifications were stamped with 'YYYY-MM-DD hh:mm:ss.ms', which Moment renders as 2026-08-19 01:34:56.3456 for 13:34:56.789 UTC. '.ms' is not milliseconds. 'm' and 's' are both format tokens, so the pattern renders the minute and second a second time. 'hh' is a 12-hour clock with no meridiem token beside it, so 13:34 and 01:34 produce the same string. 'HH:mm:ss.SSS' gives a 24-hour clock and real milliseconds. This changes the text of notification timestamps. The timestamp property is documented as being a string in ISO 8601 format, which it still is not. That would be a further change to the format and is left alone here. Addresses nasa#8411
danfry1
force-pushed
the
fix-notification-timestamp-format
branch
from
August 21, 2026 15:42
c3b9799 to
7ad307a
Compare
unlikelyzero
left a comment
Contributor
There was a problem hiding this comment.
Some things I observed:
- The JSDoc on the
timestampproperty in NotificationAPI.js (around line 413) is inaccurate on two points. It claims ISO 8601 format, but the value produced never has a T separator or Z/offset, so it wasn't ISO 8601 before this fix and still isn't after it. Separately, the[timestamp]bracket notation in the doc implies the property is optional and caller-settable, but the code assigns it unconditionally, overwriting any value the caller passes in, unlike theseverityline right above it, which does honor a caller-supplied value via||. Since this PR is what makes the format claim demonstrably wrong, it seems worth fixing the comment here rather than leaving it stale, given it's a one-line doc change with no behavior or test risk. - There isn't a regression test asserting on the timestamp's format. It might be worth adding a shape assertion (e.g. matching
YYYY-MM-DD HH:mm:ss.SSS) instead of relying on an exact hardcoded string, since a future move to ISO 8601 would otherwise require touching a brittle exact-match test.
Describe notification timestamps as API-generated UTC values in the format produced by the implementation. Add deterministic regression coverage for 24-hour formatting and millisecond precision.
Author
|
Thanks for catching these, @unlikelyzero — both points are addressed in |
With the clock frozen, an exact match is as deterministic as the shape assertion it replaces, and stronger: the mocked instant has pairwise-distinct date and time components, so any transposed format token changes the output. The shape match guarded only the hour and fractional digits, leaving the other positions open to the same wrong-token class of bug this branch fixes. Addresses nasa#8411
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8411
Describe your changes:
Notifications are stamped with
'YYYY-MM-DD hh:mm:ss.ms', which Moment renders like this:.msisn't milliseconds.mandsare both format tokens, so it just repeats the minute and second — the3456is34and56again. Separately,hhis a 12-hour clock and there's no meridiem token next to it, so 13:34 and 01:34 render identically.'HH:mm:ss.SSS'gives a 24-hour clock and actual milliseconds:This does change the text of notification timestamps, so flagging that rather than burying it.
The
timestampJSDoc now describes the property as an API-generated UTC value in the format the implementation actually produces, rather than an optional ISO 8601 value.NotificationAPISpecalso includes deterministic regression coverage: it freezes time after noon and asserts the 24-hour timestamp shape with exactly three fractional digits, without relying on a complete hardcoded timestamp.Found this while reading through the time code for #7393. It doesn't depend on that work and still uses Moment, so it should be fine on its own.
Testing instructions are in #8411. In short: trigger any notification, open the notification list, and compare the timestamp against the wall clock — easiest to see between 13:00 and 23:59 UTC, where the hour is wrong as well as the fractional part.
Verification
NotificationAPISpec: 11/11 passing in Chrome Headless.NotificationAPI.jsandNotificationAPISpec.js.git diff --checkpasses.All Submissions:
Author Checklist
NotificationAPISpecpasses (11/11), including deterministic timestamp-format regression coverage.type:label? — I don't have permission to set labels;type:bugwould be right.