fix: keep relative timestamps on just now for the first minute - #3364
Conversation
c3b80cc to
6bcf3da
Compare
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head b2a51c66b208b2da10a690d51c641deaf62ff71f against current main@d62857a8357e9160926726a2a13096bc2dc2b91d.
The defect remains on main: sub-minute labels count seconds and RelativeTime schedules a render every second. This revision puts the first-minute boundary in the shared pure formatter, reuses it for compact and wide labels, and schedules exactly one timer at the boundary instead of adding a UI-local special case. Boundary coverage at 0/59,999/60,000ms would fail under main. I found no actionable P0-P2 defect; required test is green.
Required conclusions:
- Optimal for the actual problem: yes; one shared
JUST_NOW_MSauthority drives both formatting and refresh cadence. - Production code to delete: none beyond the superseded second-level branch/comments already removed.
- Tests to delete/replace: none identified; the two focused tests are behavioral and protect the original defect.
- Deeper refactor: no.
- Ready to merge: code-wise yes, but the PR is still Draft and this automated review is not approval.
- Residual risks/gaps: no rendered React timer test, though the pure delay contract covers the load-bearing behavior; independent human review is still required for this user-visible behavior change.
jackwener
left a comment
There was a problem hiding this comment.
Independent review of exact head b2a51c66b208b2da10a690d51c641deaf62ff71f.
No P0–P3. First-minute just-now is a single JUST_NOW_MS authority for both labels and ticker delay. Boundaries 0 / 59,999 / 60,000 ms and future/clock-skew (diffMs < JUST_NOW_MS) hold.
Authorization: Raft #PR-Review-Kabi:8bb877df via dm:@Opus-Qronos-AstroHan:c1beef44 (AstroHan-delegated). Approve only; not merging.
Dismissing: Kabi GitHub-action authorization was withdrawn. Independent Raft GO on b2a51c6 still stands; this APPROVE was submitted under a revoked auth chain.
jackwener
left a comment
There was a problem hiding this comment.
Independent review of b2a51c66b.
-
Problem. Sub-minute labels counted seconds, so sidebar rows flickered every second.
-
Solution. One just-now label (
刚刚/just now) for the first 60s, then minute buckets. Ticker waits until that window ends (JUST_NOW_MS - diffMs) instead of ticking every second. Compact path uses the same formatter. -
Occam. This is the right cut. Tests pin the first minute and the delay.
Merge caveat after #3397: packages/core/src/__tests__/relative-time.test.ts is a new file with no ASF header. The listed test SUCCESS is from 2026-08-21, before that gate. Rebase onto current main and run npm run write:asf-headers before merge, or check:asf-headers will fail.
Not requesting changes on the behavior.
|
Correction on the header caveat: only the new test file
|
Sidebar and other RelativeTime rows were counting seconds under one minute, which made the suffix flicker. Hold a single just-now label until the minute boundary, and only refresh then. Generated-by: Maka
…ions Generated-by: Maka
Generated-by: Maka
b2a51c6 to
88b860f
Compare
The new test file is covered by check:asf-headers after #3397. Generated-by: Grok
jackwener
left a comment
There was a problem hiding this comment.
Re-review of e65809320 after the header fix.
Behavior still holds: first minute is one just-now label; ticker waits for that boundary. The new test file now has the ASF header. Required test on this head started 18:42Z, after #3397's header gate, and is SUCCESS.
Approve. Merge stays with the author.
Withdrawing: nextRelativeRefreshDelay() treats future timestamps (diffMs < 0) as just-now remaining time, so delay = JUST_NOW_MS - diffMs can exceed Chromium 32-bit setTimeout. Overflow fires immediately; RelativeTime effect reschedules every render. Hold until a new head clamps age to >= 0.
jackwener
left a comment
There was a problem hiding this comment.
Withdrawing the previous APPROVE on this head.
P1: nextRelativeRefreshDelay computes JUST_NOW_MS - diffMs for every diffMs < JUST_NOW_MS. That formula is only valid for 0 <= diffMs < 60s. Future timestamps (diffMs < 0) — already documented as clock skew → just-now — produce an unbounded delay. Around 25 days ahead it exceeds INT32_MAX; Chromium/Node setTimeout overflow-fires in ~1ms. <RelativeTime>'s effect has no deps and reschedules every render, so that becomes a tight loop.
This is a regression from main, which used a constant 1000 in the same branch (1Hz tick, no overflow).
Do not clamp at setTimeout. Age should never be negative:
const diffMs = Math.max(0, now - ts);Future records stay on the just-now label and refresh on the existing 60s cadence until the clock catches up. That also survives clock correction; a 24-day timer would not.
Please add tests for ts > now and ts = now + 30d asserting the delay stays in the cadence table, not JUST_NOW_MS - diffMs.
| const diffMs = now - ts; | ||
| if (diffMs > RELATIVE_HORIZON_MS) return null; | ||
| if (diffMs < 60_000) return 1_000; | ||
| if (diffMs < JUST_NOW_MS) return JUST_NOW_MS - diffMs; |
There was a problem hiding this comment.
JUST_NOW_MS - diffMs assumes diffMs >= 0. Future ts makes this unbounded and can overflow Chromium's 32-bit timer.
Minimal/clean fix is the same line: const diffMs = Math.max(0, now - ts) so age is a non-negative duration. Then this branch returns JUST_NOW_MS for future timestamps, which matches the documented just-now display and cannot overflow.
jackwener
left a comment
There was a problem hiding this comment.
Independent re-review of a59cbb0.
The future-timestamp timer overflow is closed: one private relativeAgeMs() = Math.max(0, now - ts) feeds all three consumers. Delay for +30d is 60s, not an unbounded remaining window. Required test on this head is SUCCESS. Non-blocking: the new test name still says "finite" while the sample includes NaN/±Infinity.
Approve. Merge stays with the author.
Summary
Relative-time suffixes under one minute were counting seconds (
1秒钟前,2秒钟前, …), which made sidebar task rows and otherRelativeTimesurfaces flicker. Keep a single just-now label (刚刚/just now) for the whole first minute, then switch to minute buckets, and delay the ticker until that boundary.Verification
packages/corebuild +node --test dist/__tests__/relative-time.test.js(pass)biome format/biome linton the touched files (pass)AI use
Select exactly one:
Tool(s) and scope:
Maka implemented the formatter/ticker change, tests, and this PR.
Checklist
Does this PR entail a change in behavior?