-
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 2 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,46 @@ | ||
| import { describe, it, expect } from 'vitest' | ||
| import { useNow, shouldScheduleNow } from './useNow' | ||
|
|
||
| // We don't have @testing-library/react / jsdom in this package's | ||
| // vitest setup, so we can't invoke the hook through a renderer here. | ||
| // Instead we: | ||
| // 1. Import `useNow` so the test fails if the module fails to load | ||
| // or the export shape changes (catches accidental signature | ||
| // breaks in CI). | ||
| // 2. Pin the scheduling predicate `shouldScheduleNow` — extracted | ||
| // from the hook precisely so the branch logic that decides | ||
| // "tick or don't tick" is unit-testable without a renderer. | ||
| // | ||
| // (Cursor Bugbot pointed out that a previous iteration of this file | ||
| // inlined the branch logic without ever importing `useNow`, giving | ||
| // false coverage. This rewrite makes the test reflect what's actually | ||
| // shipped.) | ||
|
|
||
| describe('useNow', () => { | ||
| it('exports a callable hook with the documented signature', () => { | ||
| expect(typeof useNow).toBe('function') | ||
| // useNow(intervalMs?) — single optional parameter. | ||
| 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) | ||
| }) | ||
| }) | ||
|
cursor[bot] marked this conversation as resolved.
|
||
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,50 @@ | ||
| 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 | ||
| } | ||
|
|
||
| /** | ||
| * 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. SKY-820 captured users perceiving 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(() => { | ||
| if (!shouldScheduleNow(intervalMs)) return | ||
| // Use the global setInterval rather than `window.setInterval` so the | ||
| // hook works under SSR / Node-based tests too. In browsers they're | ||
| // the same function; in Node, `window` is undefined. | ||
| const id = setInterval(() => setNow(Date.now()), intervalMs as number) | ||
| return () => { | ||
| clearInterval(id) | ||
| } | ||
| }, [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.