Read commit timestamps in UTC via linq2db and remove NormalizeTimestamp#2391
Draft
myieye wants to merge 7 commits into
Draft
Read commit timestamps in UTC via linq2db and remove NormalizeTimestamp#2391myieye wants to merge 7 commits into
myieye wants to merge 7 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
the
💻 FW Lite
Merged
myieye
changed the title
hahn-kev
reviewed
Jul 1, 2026
hahn-kev
requested changes
Jul 1, 2026
hahn-kev
left a comment
hahn-kev
left a comment
Collaborator
There was a problem hiding this comment.
nice catch. I have some suggestions.
hahn-kev
reviewed
Jul 2, 2026
hahn-kev
approved these changes
Jul 2, 2026
Locks in that every persisted DateTimeOffset round-trips to the same UTC instant through both EF Core and linq2db (#2092). linq2db skips the converter for the Harmony commit timestamp so HistoryService normalizes it by hand; comment dates use the global EF converter, which linq2db also applies. These errors equal the local UTC offset, so they vanish on UTC — hence the FwLite test job now runs under Eastern time (non-UTC, DST, negative offset). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lexbox's second UseLinqToDB created a fresh MappingSchema that shadowed Harmony's, dropping Harmony's Commit.HybridDateTime.DateTime UTC converter. linq2db then read commit timestamps in local time, so HistoryService normalized them by hand (#2092). Extend Harmony's existing schema instead of replacing it: the converter now applies, the redundant Commit column re-declaration is gone, and HistoryService.NormalizeTimestamp is deleted. The parity test now also covers UnreadComment.MarkedUnreadAt and fails loudly instead of a silent no-op when run on UTC. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collapse the repetitive per-column tuples into a generic AssertColumnsAreUtc<T> helper, and add a DeletedAt test that reads it from the deleted snapshot (EntityIsDeleted column) since it is never non-null in a projected column. Tightened comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
All plain DateTimeOffset columns share one global converter, so testing many proves nothing extra. Drop the generic helper and MarkedUnreadAt; keep one comment timestamp, the commit timestamp, and DeletedAt — the three distinct mechanisms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Explain the conditional AddMappingSchema (UseLinqToDbCrdt already registered the schema); drop the duplicated DeletedAt note from the test header and state it once at the call site, including why that test isn't gated on a non-UTC zone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Move RequireNonUtc() into InitializeAsync so it guards every test in the class (per hahn-kev); DeletedAt is now gated too, fine since it still round-trips correctly under a non-UTC zone. - Add an explicit linq2db read of the commit timestamp so the linq2db path stays covered even if HistoryService ever moves to EF. - Set the comment thread CreatedAt to a known non-UTC-offset instant and assert it exactly, testing that the write is recorded (not just the round-tripped return). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Falling back to a fresh MappingSchema silently dropped Harmony's Commit UTC conversion, reintroducing local-time timestamps (#2092). Fail loudly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
myieye
force-pushed
the
claude/date-parity-comments
branch
from
2cb4964 to
1d8ffe9
Compare
Collaborator
Author
|
Rebased and resolved merge conflicts |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
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
2 participants
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.
Makes every
DateTimeOffsetwe persist round-trip to the same UTC instant through both EF Core and linq2db, and removes the hand-rolledHistoryService.NormalizeTimestampworkaround for #2092.Root cause: lexbox's second
UseLinqToDBcreated a freshMappingSchemathat shadowed Harmony's, dropping Harmony'sCommit.HybridDateTime.DateTimeUTC converter — so linq2db read commit timestamps in local time andHistoryServicereinterpreted the ticks by hand. The fix is to extend Harmony's existing schema (its own get-or-create pattern) instead of replacing it; the converter then applies, the redundant Commit column re-declaration is deleted, andNormalizeTimestampis gone.DateTimeOffsetOrmParityTestslocks this in: the commit timestamp (EF + bothHistoryServicelinq2db paths) and the plain columns (commentCreatedAt/UpdatedAt,UnreadComment.MarkedUnreadAt) must all report the same UTC instant at offset zero.Set-TimeZonestep; the test fails loudly rather than passing vacuously if ever run on UTC.Draft because it's based on #2382 (
comment-model) for the non-Harmony comment-date coverage — it can't merge until that does, and CI won't run on this base.Note for review:
LcmCrdt.Testshas 4–5 pre-existing failures oncomment-modelthat are not from this change (regression baselines don't yet include the new comment types; one perf test is timing-flaky). Those belong to #2382. With this change the suite is otherwise green, and the kernel fix added no new failures.🤖 Generated with Claude Code