fix(codex): use real activity timestamps and local calendar days#222
Conversation
b03099e to
f87924c
Compare
Code Review — PR #222@embogomolov solid fix for a real bug. The UTC→local transition is the correct approach. A few findings below. HIGH1. After this PR, the codex branch sets if (!ts || ts < 1000000000000) ts = s.first_ts;does not catch Fix: ts = parseTimestampMs(entry.timestamp || entry.ts);
if (!Number.isFinite(ts)) ts = s.first_ts;2. The
MEDIUM3. Timezone data duplicated in leaderboard sync payload (
const payload = {
timezone: stats.timezone || '', // here
utcOffsetMinutes: stats.utcOffsetMinutes, // and here
stats: {
timezone: stats.timezone || '', // duplicate
utcOffsetMinutes: stats.utcOffsetMinutes, // duplicate
},
};One of the two should be removed — the leaderboard server sees both, wastes bandwidth, and may cause confusion about which is authoritative. Top-level is the more conventional location for metadata. 4. const d = parseLocalDayStart(s.date);If LOW5. Setting 6. Both are now part of 7. The initial value was changed from Summary
The NaN propagation in HIGH-1 is the one to fix before merge — it will silently corrupt daily hours for any Codex entry that lacks a timestamp field. The rest are robustness improvements. |
vakovalskii
left a comment
There was a problem hiding this comment.
LGTM ✅ Welcome @embogomolov — focused fix that addresses a real analytics bug.
Verified locally:
node --test test/codex-activity-timezone.test.js→ 4/4 pass- Full suite: 131/0/2 pass
Spot-checks:
normalizeTimestampMsthreshold (1e12) correctly distinguishes seconds-since-epoch from milliseconds-since-epoch — values < year 2001 in ms get*1000. Codex stores seconds, others ms, so this rationalises the inputparseCodexSessionFilewas usingstat.mtimeMsas the initialfirstTs/lastTs— wrong because mtime is when the file was last touched, not when the conversation started. Walking JSONL entries first and falling back to mtime only when no embedded timestamps exist is the right inversion- Replacing
new Date(s.date).toISOString().slice(0,10)withfmtLocalDay()in week-key computation fixes the UTC day-boundary skew that was hiding active days for late-evening users - Adding
timezone+utcOffsetMinutesto leaderboard sync payload lets the leaderboard render local days correctly
Squash-merging.
|
Approved and ready — but #221 (Pi support) just merged and now this conflicts on Could you rebase onto git fetch origin && git rebase origin/main
# resolve in scanCodexSessions / syncLeaderboard (keep both — Pi paths and timezone metadata are orthogonal)
git push --force-with-leaseI'm about to cut 7.5.0 so this one will likely land in 7.5.1. Won't take long after rebase. |
f87924c to
7d425df
Compare
|
@embogomolov None of the 7 review findings (posted 25 May) have been addressed — no new commits since the review. Merge blocker — HIGH-1 (NaN propagation in Also waiting on rebase onto |
|
@NovakPAai Thanks. I think this is looking at the old PR head. The branch was rebased and force-pushed after the #221 conflict. Current head is
HIGH-1 is fixed in the current code. Codex entry timestamps now go through The other review notes are also addressed: safer epoch cutoff, top-level timezone metadata only, malformed date handling in cost analytics, TZ test comment, timezone/offset assertions, and explicit timestamp fallback logic. Verified locally:
GitHub currently shows the PR as mergeable, not dirty. |
|
@NovakPAai Any updates? The status of the each fix has been explicitly described above Thank you |
Summary
Fixes Codex activity dates, daily hours, and local-day streak calculation.
Codex detailed sessions were using session-level timestamps for user messages instead of each JSONL entry timestamp. This could collapse daily Codex hours to
0and hide active days in long-running sessions.Changes
mtimeis only a fallback when no timestamps exist.today, current streak, week keys, and burn-rate day from local calendar dates instead of UTC date slicing.codedash-*tocodbash-*so stale daily/analytics caches from older timestamp logic do not mask the fix after upgrade.Test plan
node --test test/codex-activity-timezone.test.jsnode --test test/pi-session.test.jsnode --test test/*.test.js