Skip to content

fix(id-web): needsRefresh treats non-numeric/future stored timestamps as stale (#847) - #857

Merged
dezren39 merged 4 commits into
mainfrom
sweep/847-needs-refresh
Oct 25, 2026
Merged

dezren39 merged 4 commits into
mainfrom
sweep/847-needs-refresh

Conversation

@dezren39

@dezren39 dezren39 commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

Fixes #847

needsRefresh() in pkgs/id/web/src/main.ts did Date.now() - Number(refreshedAt) >= interval. For a non-numeric stored id_identity_refreshed_at value that is NaN >= interval = false forever, so the SPA-navigation keep-alive never refreshed the 30-day identity token. A far-future timestamp had the same shape (negative elapsed).

Change:

  • Extract the decision into a testable helper identityNeedsRefresh(raw, now, intervalMs) in new pkgs/id/web/src/identity-refresh.ts (same extraction pattern as world-seed.ts): missing, non-numeric, or non-finite stored values count as needs-refresh, as does any timestamp in the future. Only a finite past timestamp is compared against the interval.
  • needsRefresh() in main.ts delegates to the helper; normal behaviour unchanged.

Tests: new identity-refresh.test.ts (missing/empty, garbage/NaN/Infinity, future, recent, exact-interval, long-elapsed). Verified locally on this branch: vitest run src/identity-refresh.test.ts — 5/5 PASS; tsc --noEmit clean.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

Summary by CodeRabbit

  • Bug Fixes
    • Identity refresh decisions now handle missing, invalid, and future timestamps consistently. A refresh occurs when the saved timestamp is at least 24 hours old, while recent timestamps do not trigger a refresh.

@dezren39
dezren39 requested a review from a team as a code owner October 25, 2026 08:07
@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 169f7b75-c025-4033-b605-22324628dc9b
Commits

Reviewing files that changed from the base of the PR and between 74b849b and 2dc9aa7.

Files selected for processing: 3
  • pkgs/id/web/src/identity-refresh.test.ts
  • pkgs/id/web/src/identity-refresh.ts
  • pkgs/id/web/src/main.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The identity refresh check now uses an exported helper to evaluate stored timestamps. Missing, invalid, and future timestamps require refresh. Tests cover those cases and the 24-hour interval boundary.

Changes: Identity refresh

Layer / File(s) Summary
Timestamp validation and tests
pkgs/id/web/src/identity-refresh.ts, pkgs/id/web/src/identity-refresh.test.ts
identityNeedsRefresh treats missing, empty, non-finite, and future timestamps as requiring refresh. Tests also cover current, recent, and interval-old timestamps.
Main refresh integration
pkgs/id/web/src/main.ts
needsRefresh delegates its decision to identityNeedsRefresh, passing the stored timestamp, current time, and refresh interval.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: Minimal · up to 2dc9a

The timestamp validation and main integration show no actionable merge blocker in the supplied evidence.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the change: needsRefresh now treats non-numeric and future stored timestamps as stale, which matches the helper extraction in identity-refresh.ts and the linked issue #847. It is spe…
Linked Issues check Passed Issue #847 requires invalid or non-finite stored timestamps and future timestamps to return true, while preserving missing, recent, and interval-elapsed behavior. identityNeedsRefresh implements the…
Out of Scope Changes check Passed The changes are limited to pkgs/id/web/src/main.ts and the directly related helper and test files. The helper extraction, delegation, and tests support issue #847. No unrelated behavior or files are…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files.
Full details: Title check

Explanation

The title names the change: needsRefresh now treats non-numeric and future stored timestamps as stale, which matches the helper extraction in identity-refresh.ts and the linked issue #847. It is specific, one sentence, and free of file lists or vague wording. The title uses the fix(scope) convention and cites the issue number.

Full details: Linked Issues check

Explanation

Issue #847 requires invalid or non-finite stored timestamps and future timestamps to return true, while preserving missing, recent, and interval-elapsed behavior. identityNeedsRefresh implements these cases with Number.isFinite, a future-time check, and elapsed >= intervalMs. main.ts delegates to the helper with the existing storage key and interval. identity-refresh.test.ts covers all required cases, including the exact interval.

Full details: Out of Scope Changes check

Explanation

The changes are limited to pkgs/id/web/src/main.ts and the directly related helper and test files. The helper extraction, delegation, and tests support issue #847. No unrelated behavior or files are changed.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 Biome (2.5.15)
pkgs/id/web/src/identity-refresh.test.ts

Biome could not lint this file: nested root configuration. Check the repository's Biome configuration and plugins.

pkgs/id/web/src/identity-refresh.ts

Biome could not lint this file: nested root configuration. Check the repository's Biome configuration and plugins.

pkgs/id/web/src/main.ts

Biome could not lint this file: nested root configuration. Check the repository's Biome configuration and plugins.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-25T12:02:02.827149Z 2dc9aa7 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@dezren39

Copy link
Copy Markdown
Member Author

Sweep fix-up: the nix flake check failure (run 40175418824) was a real but formatting-only failure — biome web-fmt-check wanted the identityNeedsRefresh signature in web/src/identity-refresh.ts on one line. Fixed in 18edf73 (add-commit, no rewrite). Verified locally: biome format src/identity-refresh.ts src/identity-refresh.test.ts clean, vitest identity-refresh 5/5 PASS. Awaiting fresh CI.

@dezren39

Copy link
Copy Markdown
Member Author

Sweep update: merged current main 0b9ea76 (new head b117750). This also pulls in the #866 fix (PR #867) — the treefmt FAILURE on the previous head was that unrelated main breakage (proxy-port guard vs literal 18747/18748 in opencode.jsonc), not this PR. Fresh CI now running on a clean main base; the identity-refresh biome fix (18edf73) is unchanged.

@dezren39

Copy link
Copy Markdown
Member Author

Sweep 2026-10-25: merged current main 74b849b8 into this branch (merge commit 2dc9aa7d, clean, ls-remote verified after push). The previous head's nix flake check FAILURE was a single test-shard leg; main's id-check is green at b975516b/4c05b000 and now runs the sharded 30m-cap matrix from #874, so this fresh run is the merge gate.

@dezren39

Copy link
Copy Markdown
Member Author

Sweep 2026-10-25: fresh rollup on head 2dc9aa7d is CLEAN with every check SUCCESS (sharded id-check from #874, ~15m instead of the old 51-60m serial leg). Merging per standing directive.

@dezren39
dezren39 merged commit 2d6d1c1 into main Oct 25, 2026
28 checks passed
@dezren39
dezren39 deleted the sweep/847-needs-refresh branch October 25, 2026 12:20
@dezren39 dezren39 added task/pr-shepherd Queue packet kind: shepherd an open PR or PR set to merge source/sweep Filed or worked by an issue-sweep job labels Oct 25, 2026
@dezren39
dezren39 restored the sweep/847-needs-refresh branch October 27, 2026 09:47
@dezren39
dezren39 deleted the sweep/847-needs-refresh branch November 1, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

source/sweep Filed or worked by an issue-sweep job task/pr-shepherd Queue packet kind: shepherd an open PR or PR set to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

id(web): needsRefresh suppresses identity token refresh forever on a non-numeric stored timestamp (NaN comparison)

1 participant