feat: add createExternalStore utility and ClockProvider#195
Conversation
gcutrini
force-pushed
the
feature/external-store-clock-context
branch
from
8f3375c to
cd28bef
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a generic createExternalStore (Provider, useValue, useSelector), a ClockProvider that forwards Clock ticks into that store, tests for both modules, barrel exports and webpack entries, and a README/dependency note about the use-sync-external-store shim for React 16/17. ChangesExternal Store and Clock Context Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/__tests__/clock-context.test.js`:
- Around line 58-62: Two tests directly reassign console.error around rendering
ClockReader which can leak mocks if an exception occurs; replace those direct
assignments with jest.spyOn(console, 'error').mockImplementation(...) and ensure
you call mockRestore() in a finally block so the spy is always restored,
updating both the block that wraps render(<ClockReader />) (previously lines
58-62) and the similar block around lines 80-84; reference the ClockReader
render and use mockRestore() to guarantee cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 322fb309-5b6c-4966-b31b-9e06d7494ddd
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.jsonreadme.mdsrc/components/__tests__/clock-context.test.jssrc/components/clock-context.jssrc/components/index.jssrc/utils/__tests__/external-store.test.jssrc/utils/external-store.jswebpack.common.js
Backport of #195 to v4.x. Adds a generic external store factory (createExternalStore) using useSyncExternalStore that lets components subscribe to frequently-updating data sources without unnecessary re-renders, plus a pre-built clock store (ClockProvider, useClock, useClockSelector) wrapping the existing Clock component. Tests use @testing-library/react@11 for React 16 compatibility.
| const newResult = compute(value); | ||
| lastValueRef.current = value; | ||
|
|
||
| if (lastResultRef.current !== null && isEqual(lastResultRef.current, newResult)) { |
There was a problem hiding this comment.
null is used both as the "not yet initialized" sentinel for lastResultRef (line 169) and as a possible legitimate return value from compute. Once compute returns null, subsequent null → null transitions skip the isEqual call entirely — lastResultRef.current !== null is false, so the custom equality function is never consulted for those transitions.
In practice, useSyncExternalStore's own Object.is deduplication prevents spurious re-renders in all current cases, but the contract of the hook is broken: a consumer who passes a custom isEqual will find it silently ignored whenever the computed result is null.
Prefer a Symbol sentinel to cleanly separate the two states:
const UNSET = Symbol();
const lastResultRef = useRef(UNSET);
// …
if (lastResultRef.current !== UNSET && isEqual(lastResultRef.current, newResult)) {|
|
||
| lastResultRef.current = newResult; | ||
| return newResult; | ||
| }, [getSnapshot, compute, isEqual]); |
There was a problem hiding this comment.
isEqual is listed as a useCallback dependency, so when consumers pass an inline equality function the closure is recreated on every parent render. React's useSyncExternalStore responds to a changed getSnapshot identity by calling it immediately to check for tearing — correctness is preserved (the fast-path value === lastValueRef.current returns the cached result), but the extra call happens on every parent render rather than only on emits.
compute already receives ref-based stabilization (lines 174–177). The same pattern applied to isEqual eliminates the overhead and is consistent with how the rest of the hook is written:
const lastIsEqualRef = useRef(isEqual);
lastIsEqualRef.current = isEqual; // always up-to-date, no cache invalidation needed
const getComputedValue = useCallback(() => {
// …
if (lastResultRef.current !== null && lastIsEqualRef.current(lastResultRef.current, newResult)) {
// …
}, [getSnapshot, compute]); // isEqual removed from depsAdd a generic external store factory (createExternalStore) using useSyncExternalStore that allows components to subscribe to frequently-updating data sources without causing unnecessary re-renders. Includes a pre-built clock store (ClockProvider, useClock, useClockSelector) that wraps the existing Clock component, enabling projects to move clock state out of Redux and eliminate per-second re-renders of all connected components.
…seSyncExternalStoreWithSelector Delegates selector + equality handling to the react-recommended primitive from use-sync-external-store. Removes the manual ref bookkeeping, the null-sentinel ambiguity for cached results, and the useCallback dependency on isEqual that recreated the snapshot closure on every parent render.
gcutrini
force-pushed
the
feature/external-store-clock-context
branch
from
37adf17 to
1e96e65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/external-store.js`:
- Around line 39-40: The module docblock incorrectly states selector updates are
handled via useMemo; update the comment to reference
useSyncExternalStoreWithSelector instead. Edit the top-level comment in
src/utils/external-store.js (the module docblock) to replace any mention of
useMemo as the mechanism for selector updates with a clear note saying
useSyncExternalStoreWithSelector is used to subscribe and derive selector values
(and mention isEqual behavior if present), so readers know the actual
implementation used by functions like useExternalStore and
useSyncExternalStoreWithSelector.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7045c6ce-e833-445e-a51e-74233b372750
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.jsonreadme.mdsrc/components/__tests__/clock-context.test.jssrc/components/clock-context.jssrc/components/index.jssrc/utils/__tests__/external-store.test.jssrc/utils/external-store.jswebpack.common.js
🚧 Files skipped from review as they are similar to previous changes (6)
- package.json
- webpack.common.js
- src/components/tests/clock-context.test.js
- src/components/clock-context.js
- src/components/index.js
- src/utils/tests/external-store.test.js
| * 4. useMemo adds a layer on top: it runs a compute function on the raw value | ||
| * and only re-renders if the computed result changed (checked via isEqual). |
There was a problem hiding this comment.
Fix stale implementation note in the module docblock.
The comment says selector updates are handled via useMemo, but this implementation uses useSyncExternalStoreWithSelector. Keeping this accurate avoids confusion during maintenance.
Suggested doc fix
- * 4. useMemo adds a layer on top: it runs a compute function on the raw value
- * and only re-renders if the computed result changed (checked via isEqual).
+ * 4. useSyncExternalStoreWithSelector runs compute on the raw value and only
+ * re-renders when the computed result changes (checked via isEqual).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * 4. useMemo adds a layer on top: it runs a compute function on the raw value | |
| * and only re-renders if the computed result changed (checked via isEqual). | |
| * 4. useSyncExternalStoreWithSelector runs compute on the raw value and only | |
| * re-renders when the computed result changes (checked via isEqual). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/external-store.js` around lines 39 - 40, The module docblock
incorrectly states selector updates are handled via useMemo; update the comment
to reference useSyncExternalStoreWithSelector instead. Edit the top-level
comment in src/utils/external-store.js (the module docblock) to replace any
mention of useMemo as the mechanism for selector updates with a clear note
saying useSyncExternalStoreWithSelector is used to subscribe and derive selector
values (and mention isEqual behavior if present), so readers know the actual
implementation used by functions like useExternalStore and
useSyncExternalStoreWithSelector.
Provider now accepts initialValue so getSnapshot returns a real value before the first emit. ClockProvider forwards its now prop as initialValue so consumers reading useClock or useClockSelector during the initial render see a valid timestamp instead of null.
…#247) * feat: add createExternalStore utility and ClockProvider Backport of #195 to v4.x. Adds a generic external store factory (createExternalStore) using useSyncExternalStore that lets components subscribe to frequently-updating data sources without unnecessary re-renders, plus a pre-built clock store (ClockProvider, useClock, useClockSelector) wrapping the existing Clock component. Tests use @testing-library/react@11 for React 16 compatibility. * refactor(external-store): replace hand-rolled selector caching with useSyncExternalStoreWithSelector Delegates selector + equality handling to the react-recommended primitive from use-sync-external-store. Removes the manual ref bookkeeping, the null-sentinel ambiguity for cached results, and the useCallback dependency on isEqual that recreated the snapshot closure on every parent render. * feat(external-store): allow Provider to seed initial value Provider now accepts initialValue so getSnapshot returns a real value before the first emit. ClockProvider forwards its now prop as initialValue so consumers reading useClock or useClockSelector during the initial render see a valid timestamp instead of null.
2 participants
ref: https://app.clickup.com/t/86b8dm4da
Add a generic external store factory (
createExternalStore) usinguseSyncExternalStorethat lets components subscribe to frequently-updating data sources without unnecessary re-renders.Includes a pre-built clock store (
ClockProvider,useClock,useClockSelector) wrapping the existing Clock component. This enables projects to move clock state out of Redux, eliminating per-second re-renders of all connected components.API
createExternalStore(name)→{ Provider, useValue, useSelector }useValue()— re-renders on every emituseSelector(compute, isEqual)— re-renders only when the computed result changesClock usage
ref: https://app.clickup.com/t/86b8dm4da
Summary by CodeRabbit
New Features
Documentation
Tests
Chores