Skip to content

KM-XXXX: Update event aggregated identifier rotation to a day rather of 24 hours - #369

Merged
kp-juan-docal merged 1 commit into
masterfrom
juan.docal/KM-XXXX-update-kpi-rotation-to-day-rather-than-24-hours
Jul 22, 2026
Merged

KM-XXXX: Update event aggregated identifier rotation to a day rather of 24 hours#369
kp-juan-docal merged 1 commit into
masterfrom
juan.docal/KM-XXXX-update-kpi-rotation-to-day-rather-than-24-hours

Conversation

@kp-juan-docal

Copy link
Copy Markdown
Contributor

Summary

As per the title. It updates rotation to rely on the UTC day, rather than a fixed 24 hours.

@kp-juan-docal kp-juan-docal self-assigned this Jul 22, 2026
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude finished @kp-juan-docal's task in 1m 3s —— View job


Code Review: KPI event rotation by UTC day

  • Read changed file & PR diff
  • Analyze correctness, threading, and edge cases
  • Check test coverage
  • Post review

Nice, focused change. Switching from a rolling elapsedDays > 1.0 window to Calendar.isDate(_:inSameDayAs:) correctly aligns aggregated-id rotation to the UTC calendar day, and reusing UTC for both the calendar and the ISO8601 formatter keeps the comparison consistent. The change reads cleanly and the intent matches the title.

A few observations:

1. Intended behavior shift on the first day (worth confirming) — KPIEventUtils.swift:35

This is the substantive change and I believe it's intended, just flagging it explicitly:

  • Before: an id created at 23:00 UTC rotated exactly 24h later (23:00 the next day).
  • After: the same id rotates at the next 00:00 UTC — i.e. only ~1 hour later.

So the very first aggregation window for an id can now be arbitrarily short (down to seconds if created just before midnight). That's the expected consequence of moving to calendar-day semantics (and likely matches the other platforms), but it does mean the "first day" is not a full day. Assuming this matches the KPI spec, no action needed.

As a nice side effect, this also fixes the old negative-elapsedDays blind spot: if the clock ever moved backwards relative to createdAt, the old > 1.0 check would never rotate, whereas the day comparison now rotates on any different-day mismatch.

2. No test coverage for the rotation logic — KPIEventUtils.swift:30

The clock: () -> Date injection point (added earlier) makes this trivially unit-testable, but there don't appear to be any tests in the PIAKPI package for KPIEventUtils. Given this PR changes the rotation boundary, it'd be a good opportunity to add tests covering:

  • same UTC day → id preserved, clearAll() not called
  • different UTC day → new id generated + clearAll() called
  • boundary case: createdAt at 23:59:59.999 UTC, now just past 00:00 UTC → rotates

Add rotation tests →

3. Minor / nit — KPIEventUtils.swift:61

TimeZone(identifier: "UTC")! force-unwraps, while the formatter on line 55 assigns the optional directly. Both are safe ("UTC" always resolves), so this is purely a style consistency nit — no change required.

Threading / memory

No concerns. KPIEventUtils is a value type used inside the KPI actor, so access is serialized; the static let utcCalendar is a Sendable value type computed once. No retain cycles introduced.

Impacted flow to test: KPI event aggregation/rotation across a UTC midnight boundary — verify a new aggregatedId is issued and persisted events are cleared once, and that events within the same UTC day continue to share one id.

Overall LGTM — the only real ask is confirming the first-window behavior (#1) is intended and, ideally, adding the tests in #2.

@kp-juan-docal kp-juan-docal changed the title KM-XXXX: Update event rotation to day instead of 24 hours KM-XXXX: Update event aggregated identifier rotation to a day rather of 24 hours Jul 22, 2026
@kp-juan-docal
kp-juan-docal merged commit c7635b2 into master Jul 22, 2026
3 checks passed
@kp-juan-docal
kp-juan-docal deleted the juan.docal/KM-XXXX-update-kpi-rotation-to-day-rather-than-24-hours branch July 22, 2026 13:57
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.

2 participants