-
Notifications
You must be signed in to change notification settings - Fork 61
fix(resources): stop 'Updated Xs' timer resetting on no-op refetches #572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
eliran-ops
wants to merge
5
commits into
main
Choose a base branch
from
feat/fix-list-refresh-age-shift-and-filter-open-refetch
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
f0cfc09
fix(resources): stop "Updated Xs" timer resetting on no-op refetches
eliran-mic 031411d
test(useNow): import the hook and pin its scheduling predicate
eliran-mic b8b25ed
perf(resources): isolate the 1Hz Updated-Xs tick to its own component
eliran-mic d67dd82
fix(useNow): test the hook end-to-end via pure scheduler driver
eliran-mic ba0a48d
fix(useNow.test): explicit vi.fn signatures for TSC strict tuple infer
eliran-mic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| import { describe, it, expect, vi } from 'vitest' | ||
| import { useNow, shouldScheduleNow, scheduleNowTicks } from './useNow' | ||
|
|
||
| // This package's vitest config has no @testing-library/react and no | ||
| // jsdom — we can't render hooks here. Instead the hook's effect | ||
| // body is hoisted into the pure `scheduleNowTicks` driver, and we | ||
| // exercise THAT directly: the same opt-out rules, the same | ||
| // setInterval call, the same cleanup contract that the hook | ||
| // installs at runtime. The hook itself is also imported so the | ||
| // suite fails if its export shape regresses. | ||
|
|
||
| describe('useNow', () => { | ||
| it('exports a callable hook with the documented signature', () => { | ||
| expect(typeof useNow).toBe('function') | ||
| expect(useNow.length).toBeLessThanOrEqual(1) | ||
| }) | ||
| }) | ||
|
|
||
| describe('shouldScheduleNow', () => { | ||
| it('schedules when intervalMs is a positive number', () => { | ||
| expect(shouldScheduleNow(1)).toBe(true) | ||
| expect(shouldScheduleNow(1000)).toBe(true) | ||
| expect(shouldScheduleNow(60_000)).toBe(true) | ||
| }) | ||
|
|
||
| it('opts out when intervalMs is null', () => { | ||
| expect(shouldScheduleNow(null)).toBe(false) | ||
| }) | ||
|
|
||
| it('opts out when intervalMs is zero', () => { | ||
| expect(shouldScheduleNow(0)).toBe(false) | ||
| }) | ||
|
|
||
| it('opts out when intervalMs is negative', () => { | ||
| expect(shouldScheduleNow(-1)).toBe(false) | ||
| expect(shouldScheduleNow(-1000)).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('scheduleNowTicks (the pure body of useNow)', () => { | ||
| function fakeTimers() { | ||
| // Type the spies explicitly so TS knows .mock.calls has the | ||
| // (cb, ms) shape — vi.fn() inference defaults to a 0-arity | ||
| // signature otherwise. | ||
| const setInterval = vi.fn<(cb: () => void, ms: number) => unknown>(() => 'TIMER_ID' as unknown) | ||
| const clearInterval = vi.fn<(id: unknown) => void>() | ||
| const now = vi.fn<() => number>(() => 1700000000000) | ||
| return { | ||
| timers: { setInterval, clearInterval, now }, | ||
| setInterval, | ||
| clearInterval, | ||
| now, | ||
| } | ||
| } | ||
|
|
||
| it('installs no timer and returns a no-op cleanup when interval is null', () => { | ||
| const setNow = vi.fn() | ||
| const { timers, setInterval, clearInterval } = fakeTimers() | ||
| const cleanup = scheduleNowTicks(null, setNow, timers) | ||
| expect(setInterval).not.toHaveBeenCalled() | ||
| cleanup() | ||
| expect(clearInterval).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('installs no timer when interval is zero or negative', () => { | ||
| const setNow = vi.fn() | ||
| for (const bad of [0, -1, -1000]) { | ||
| const { timers, setInterval } = fakeTimers() | ||
| scheduleNowTicks(bad, setNow, timers) | ||
| expect(setInterval, `interval=${bad}`).not.toHaveBeenCalled() | ||
| } | ||
| }) | ||
|
|
||
| it('installs an interval that calls setNow with the current time', () => { | ||
| const setNow = vi.fn() | ||
| const { timers, setInterval, now } = fakeTimers() | ||
| scheduleNowTicks(1000, setNow, timers) | ||
| expect(setInterval).toHaveBeenCalledTimes(1) | ||
| expect(setInterval.mock.calls[0][1]).toBe(1000) | ||
|
|
||
| // Fire the registered tick — the hook must update state with | ||
| // the latest wall-clock value, NOT the value captured at | ||
| // mount. | ||
| const tick = setInterval.mock.calls[0][0] as () => void | ||
| now.mockReturnValue(1700000005000) | ||
| tick() | ||
| expect(setNow).toHaveBeenCalledWith(1700000005000) | ||
| now.mockReturnValue(1700000010000) | ||
| tick() | ||
| expect(setNow).toHaveBeenLastCalledWith(1700000010000) | ||
| }) | ||
|
|
||
| it('returns a cleanup that clears the registered timer id', () => { | ||
| const setNow = vi.fn() | ||
| const { timers, setInterval, clearInterval } = fakeTimers() | ||
| setInterval.mockReturnValue(42) | ||
| const cleanup = scheduleNowTicks(1000, setNow, timers) | ||
| cleanup() | ||
| expect(clearInterval).toHaveBeenCalledWith(42) | ||
| }) | ||
|
|
||
| it('passes the interval through unchanged (1Hz vs 60Hz callers)', () => { | ||
| const setNow = vi.fn() | ||
| const { timers, setInterval } = fakeTimers() | ||
| scheduleNowTicks(60_000, setNow, timers) | ||
| expect(setInterval.mock.calls[0][1]).toBe(60_000) | ||
| }) | ||
| }) | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| import { useEffect, useState } from 'react' | ||
|
|
||
| /** | ||
| * Pure predicate for whether `useNow` should schedule a tick for the | ||
| * given interval. Extracted so the scheduling rule (null and | ||
| * non-positive intervals opt out; anything > 0 ticks) can be | ||
| * unit-tested without a React renderer. | ||
| */ | ||
| export function shouldScheduleNow(intervalMs: number | null): boolean { | ||
| if (intervalMs === null) return false | ||
| if (intervalMs <= 0) return false | ||
| return true | ||
| } | ||
|
|
||
| /** | ||
| * Pure scheduler used by `useNow`. Given the timer primitives, the | ||
| * interval, and a setter, it installs an interval that calls | ||
| * `setNow(nowFn())` every `intervalMs` and returns a cleanup | ||
| * function — or installs nothing and returns a no-op for opt-out | ||
| * intervals. | ||
| * | ||
| * Hoisted so the hook's full effect body (the part that's actually | ||
| * worth testing — opt-out vs schedule, the cleanup contract, the | ||
| * value passed to setNow) is unit-testable end-to-end without | ||
| * needing a React renderer (this package's vitest config has | ||
| * neither @testing-library/react nor jsdom). | ||
| */ | ||
| export function scheduleNowTicks( | ||
| intervalMs: number | null, | ||
| setNow: (n: number) => void, | ||
| timers: { | ||
| setInterval: (cb: () => void, ms: number) => unknown | ||
| clearInterval: (id: unknown) => void | ||
| now: () => number | ||
| }, | ||
| ): () => void { | ||
| if (!shouldScheduleNow(intervalMs)) { | ||
| return () => {} | ||
| } | ||
| const id = timers.setInterval(() => setNow(timers.now()), intervalMs as number) | ||
| return () => timers.clearInterval(id) | ||
| } | ||
|
|
||
| /** | ||
| * Returns the current wall-clock time, refreshed every `intervalMs`. | ||
| * | ||
| * Use this when you have UI that derives a relative time string | ||
| * (e.g. "Updated 8s", "24d") from a fixed timestamp. Without it, | ||
| * the relative label only changes when the parent re-renders for | ||
| * an unrelated reason — which makes the label feel frozen, and | ||
| * worse: the next unrelated re-render makes it jump forward by | ||
| * however many seconds passed in silence. Users perceived that as | ||
| * a "data re-fetch was triggered" because the displayed age | ||
| * suddenly updated. | ||
| * | ||
| * The interval is opt-in per call site so cells in tight tables | ||
| * don't pay the cost of a 1Hz tick when they only need a 60Hz | ||
| * update for "minutes" granularity. | ||
| * | ||
| * @param intervalMs how often to advance the clock. Defaults to | ||
| * 1000ms. Pass `null` to disable ticking | ||
| * (returns the time at mount and never updates). | ||
| * @returns the current `Date.now()` value. | ||
| */ | ||
| export function useNow(intervalMs: number | null = 1000): number { | ||
| const [now, setNow] = useState(() => Date.now()) | ||
|
|
||
| useEffect(() => { | ||
| // Use globalThis for the timer primitives so the hook works | ||
| // under SSR / Node-based tests too. In browsers these are the | ||
| // same as window.setInterval; in Node, `window` is undefined. | ||
| return scheduleNowTicks(intervalMs, setNow, { | ||
| setInterval: globalThis.setInterval as (cb: () => void, ms: number) => unknown, | ||
| clearInterval: globalThis.clearInterval as (id: unknown) => void, | ||
| now: Date.now, | ||
| }) | ||
| }, [intervalMs]) | ||
|
|
||
| return now | ||
| } |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.