Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…or v2.0.7 spec compliance Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…n, crossFade, auto-sync delay Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements three new React hooks to achieve v2.0.7 spec compliance for ObjectUI. The PR completes the remaining "What's New in v2.0.7" roadmap items by adding runtime implementations for offline/sync, performance monitoring, and page transitions. These hooks follow the established pattern of defining types locally aligned with @objectstack/spec v2.0.7 schemas.
Changes:
- Implements
useOfflinehook with online/offline detection viauseSyncExternalStore, localStorage-backed mutation queue with FIFO eviction, and auto-sync on reconnect - Implements
usePerformancehook with Web Vitals collection (FCP, LCP), configurable cache strategy/virtual scroll settings, and debounce utility - Implements
usePageTransitionhook with Tailwind CSS animate-in/out class generation for 9 transition types, easing configuration, andprefers-reduced-motionsupport - Updates ROADMAP.md compliance metrics from 90% to 96%, marking all 12 v2.0.7 domains as complete
- Adds 32 new tests (10 for useOffline, 7 for usePerformance, 15 for usePageTransition)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/hooks/useOffline.ts | Implements offline mode detection, sync queue management with localStorage persistence, and auto-sync behavior |
| packages/react/src/hooks/usePerformance.ts | Implements performance config resolution, Web Vitals metrics collection, render tracking, and debounce utility |
| packages/react/src/hooks/usePageTransition.ts | Implements page transition class/style generation with reduced-motion awareness and crossFade support |
| packages/react/src/hooks/index.ts | Exports the three new hooks |
| packages/react/src/tests/useOffline.test.ts | Adds 10 tests covering online/offline detection, mutation queueing, queue size limits, and manual sync |
| packages/react/src/tests/usePerformance.test.ts | Adds 7 tests covering config merging, cache strategies, render counting, and debounce behavior |
| packages/react/src/tests/usePageTransition.test.ts | Adds 15 tests covering all 9 transition types, easing options, duration, and reduced-motion |
| ROADMAP.md | Updates compliance metrics and marks Q3 2026 offline/performance/transition items as complete |
|
|
||
| const debounce = useCallback( | ||
| <T extends (...args: unknown[]) => void>(fn: T): T => { | ||
| let timer: ReturnType<typeof setTimeout> | null = null; | ||
| const debounced = (...args: unknown[]) => { | ||
| if (timer) { | ||
| clearTimeout(timer); | ||
| timersRef.current.delete(timer); | ||
| } | ||
| timer = setTimeout(() => { | ||
| fn(...args); | ||
| if (timer) timersRef.current.delete(timer); | ||
| }, debounceMs); | ||
| timersRef.current.add(timer); | ||
| }; | ||
| return debounced as unknown as T; | ||
| }, | ||
| [debounceMs], | ||
| ); | ||
|
|
There was a problem hiding this comment.
The debounce implementation creates a new debounced function with separate closure state on every call. This means calling perf.debounce(callback) multiple times creates independent debounced functions that don't share the same timer state, defeating the purpose of debouncing.
For example, if a component does:
const search1 = perf.debounce(searchFn);
const search2 = perf.debounce(searchFn);Then calling search1() and search2() will fire separately after the debounce delay, not as a single debounced call.
The typical pattern for a reusable debounce utility is to either:
- Return a stable debounced function from a ref/useMemo (not recreate on each call), OR
- Use a Map/WeakMap to cache debounced versions keyed by the original function.
The current implementation suggests it's meant to be called once per callback (like in the example), but the API is misleading since debounce is exposed as a function that can be called multiple times. Consider either documenting this limitation clearly or redesigning to use a memoization strategy.
| const debounce = useCallback( | |
| <T extends (...args: unknown[]) => void>(fn: T): T => { | |
| let timer: ReturnType<typeof setTimeout> | null = null; | |
| const debounced = (...args: unknown[]) => { | |
| if (timer) { | |
| clearTimeout(timer); | |
| timersRef.current.delete(timer); | |
| } | |
| timer = setTimeout(() => { | |
| fn(...args); | |
| if (timer) timersRef.current.delete(timer); | |
| }, debounceMs); | |
| timersRef.current.add(timer); | |
| }; | |
| return debounced as unknown as T; | |
| }, | |
| [debounceMs], | |
| ); | |
| const debounceCacheRef = useRef<WeakMap<Function, Function>>(new WeakMap()); | |
| const debounce = useCallback( | |
| <T extends (...args: unknown[]) => void>(fn: T): T => { | |
| const cache = debounceCacheRef.current; | |
| const cached = cache.get(fn) as T | undefined; | |
| if (cached) { | |
| return cached; | |
| } | |
| let timer: ReturnType<typeof setTimeout> | null = null; | |
| const debounced = ((...args: unknown[]) => { | |
| if (timer) { | |
| clearTimeout(timer); | |
| timersRef.current.delete(timer); | |
| } | |
| timer = setTimeout(() => { | |
| fn(...args); | |
| if (timer) { | |
| timersRef.current.delete(timer); | |
| } | |
| }, debounceMs); | |
| timersRef.current.add(timer); | |
| }) as unknown as T; | |
| cache.set(fn, debounced as unknown as Function); | |
| return debounced; | |
| }, | |
| [debounceMs], | |
| ); | |
| // Reset debounced function cache when debounce duration changes | |
| useEffect(() => { | |
| debounceCacheRef.current = new WeakMap(); | |
| }, [debounceMs]); |
| // Only trigger on isOnline changes, not on every queue change | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isOnline, enabled]); |
There was a problem hiding this comment.
The auto-sync effect has an incomplete dependency array. The sync callback is missing from the dependencies but is used inside the effect (line 268). This can lead to stale closures where the effect calls an old version of sync that captures outdated queue state.
The eslint-disable comment indicates this was intentional to avoid triggering on queue changes, but omitting sync creates a bug. The correct approach is to include sync in the dependencies. Since sync is already memoized with useCallback and depends on queue, React will correctly re-run this effect when needed.
| // Only trigger on isOnline changes, not on every queue change | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [isOnline, enabled]); | |
| }, [isOnline, enabled, sync]); |
| if (crossFade) { | ||
| enterStyle.position = 'absolute'; | ||
| enterStyle.inset = '0'; | ||
| exitStyle.position = 'absolute'; | ||
| exitStyle.inset = '0'; | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the crossFade feature. The implementation includes logic to set position: absolute and inset: 0 when crossFade is enabled (lines 170-175), but there's no test verifying this behavior. Consider adding a test case to ensure crossFade correctly applies the overlay positioning styles to both enterStyle and exitStyle.
| useEffect(() => { | ||
| if (!enabled || !isOnline || queue.length === 0) return; | ||
| const timer = setTimeout(() => { | ||
| void sync(); | ||
| }, 100); | ||
| return () => clearTimeout(timer); | ||
| // Only trigger on isOnline changes, not on every queue change | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isOnline, enabled]); |
There was a problem hiding this comment.
Missing test coverage for the auto-sync feature. The implementation includes an effect that automatically triggers sync when the browser comes back online (lines 265-273), but there's no test verifying this critical behavior. Consider adding a test that:
- Queues mutations while offline
- Triggers the 'online' event
- Verifies that sync is automatically called and the queue is cleared
| metrics: PerformanceMetrics; | ||
| /** Mark a rendering start (returns stop function). */ | ||
| markRenderStart: () => () => void; | ||
| /** Create a debounced version of a callback. */ |
There was a problem hiding this comment.
The documentation "Create a debounced version of a callback" is misleading. Due to the implementation creating a new closure each time debounce() is called (as noted in the bug comment for lines 213-230), the debounce function doesn't return a stable debounced version that can be called multiple times. Instead, it creates a new independent debounced wrapper on each call.
The documentation should clarify the intended usage pattern. Based on the example in the hook's JSDoc, it appears the function is meant to be called once to create a debounced callback that's then used multiple times, but this constraint isn't documented in the PerformanceResult interface.
| /** Create a debounced version of a callback. */ | |
| /** | |
| * Create a debounced version of a callback. | |
| * | |
| * Note: This function is a factory. Each call to `debounce(fn)` creates a new | |
| * debounced wrapper around `fn`. To get a stable debounced callback, call | |
| * `debounce` once (for example in `useMemo` or `useCallback`) and reuse the | |
| * returned function instead of calling `debounce` on every invocation. | |
| */ |
Completes the "What's New in v2.0.7 (Implementation Required)" roadmap — all 6 remaining⚠️ Partial spec domains now have runtime implementations.
New hooks in
@object-ui/reactuseOffline— OfflineConfigSchema runtime: online/offline detection viauseSyncExternalStore, localStorage-backed mutation queue with FIFO eviction, auto-sync on reconnect, configurable strategy/conflict resolutionusePerformance— PerformanceConfigSchema runtime: resolved config (lazy load, virtual scroll, cache strategy, debounce), Web Vitals collection (FCP, LCP), render marking, per-function debounce utilityusePageTransition— PageTransitionSchema runtime: Tailwindanimate-in/animate-outclass generation for 9 transition types (fade, slide, scale, rotate, flip), easing, crossFade,prefers-reduced-motionawareROADMAP updates
Tests
32 new tests (10 offline, 7 performance, 15 page transition). Total react package: 288 tests across 24 files.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.