fix(api): serialise stored timestamps as UTC so clients render the right time - #135
Conversation
…ght time
Every DateTime this app persists is UTC, but SQL Server's datetime2 carries no
zone, so EF Core returns it as DateTimeKind.Unspecified and the "O" specifier then
omits the trailing Z. A browser reads an ISO-8601 date-time with no zone as local
time, so every stored timestamp — article publish dates, comment and invite dates,
the admin user/report lists — rendered shifted by the viewer's UTC offset.
The same article proved it: pushed over the update stream its DateTime is still
Kind=Utc and kept its Z, so it rendered correctly, then jumped by the offset once
a reload served the identical instant from the database.
Route the mappers through Timestamps.ToIsoUtc, which pins the kind before
formatting (a no-op on a value already marked UTC) instead of each call site
picking "o"/"O" on a value whose kind depends on where it came from.
Verified that AdminMapper.UserInfoProjection still translates: it is the top-level
projection of its query, which EF Core evaluates client-side, same as the
ToString("O") it replaces.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
Adds a focused unit test for the fix in ef8bfbe: proves ToIsoUtc emits a trailing Z for both a DateTimeKind.Unspecified value (what EF Core hands back for a stored datetime2) and a DateTimeKind.Utc value, and that the two produce the identical wire format for the same instant — that disagreement was the actual bug, since a browser reads a zone-less ISO-8601 string as local time. Also covers the nullable overload's "" for null. No database needed; runs standalone via `dotnet test`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
Independent architect review found the existing tests only asserted
EndsWith("Z") and Utc/Unspecified equality, which can't distinguish the
correct SpecifyKind-based fix from a plausible-but-wrong alternative
(calling ToUniversalTime() first) that shifts an Unspecified-kind value by
the server's local offset before formatting — verified empirically: that
wrong implementation still passes every existing assertion under TZ=UTC,
which is what CI runs. Pinning the literal expected string closes the gap
without depending on the test runner's timezone.
Also: the CHANGELOG entry named article/comment/invite/admin-panel
timestamps but missed SubscriptionMapper's lastFetchedAt, which is
user-visible on the Subscriptions page.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
# Conflicts: # CHANGELOG.md
Code Review ✅ ApprovedFixes timestamp serialization by routing all mappers through OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
2 participants
Summary
DateTimethis app persists is UTC, but SQL Server'sdatetime2carries no zone, so EF Core hands stored values back asDateTimeKind.Unspecifiedand the"O"/"o"format specifier then omits the trailingZ— a browser reads an ISO-8601 date-time with no zone as local time, so every stored timestamp (article publish dates, comment and invite dates, admin user/report lists) rendered shifted by the viewer's UTC offsetDateTimeis stillKind=Utcand kept itsZso it rendered correctly, then jumped by the viewer's offset once a reload served the identical instant from the databaseTimestamps.ToIsoUtc(pins the kind to UTC before formatting — a no-op on an already-UTC value) and routes every mapper (AdminMapper,FeedItemMapper,FeedServiceImpl.Comments.cs,SubscriptionMapper) through it instead of each call site picking"o"/"O"on a value whose kind depends on where it came fromRelated issue
Found by
architectduring this cycle's/maintainrefactor analysis.Type of change
Checklist
dotnet build --configuration Releasepasses with 0 errors, 0 warningscd src/pressmark-web && npm run buildpasses with 0 TypeScript errors (unaffected by this branch)TimestampsTestsunit-testsToIsoUtcdirectly: trailingZfor bothDateTimeKind.UnspecifiedandDateTimeKind.Utc, that both produce the same wire format for the same instant (the actual bug), and""for the nullable overload'snullcase; 123/123 backend tests passt('ns:key')— no strings touchedNotes for reviewers
The riskiest part of this change is
AdminMapper.UserInfoProjection, anExpression<Func<...>>consumed inside a live EF query (AdminServiceImpl.Users.cs'sListUsers). I independently re-verified (not just re-read thearchitectagent's own claim) that it still translates: built a throwaway test usingIQueryable.ToQueryString()against aUseSqlServercontext with a fake connection string (translation doesn't require a live connection) and confirmed the generated SQL selects the rawcreated_atcolumn —ToIsoUtc()is client-evaluated post-materialization on the top-level projection, exactly as claimed, same as theToString("O")it replaces. That throwaway test was deleted before this PR — it's not part of the diff.🤖 Generated with Claude Code
https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
Generated by Claude Code