feat: improve react sync hook - #13
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
✅ Deploy Preview for hyperdb canceled.
|
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReworks the ChangesAsync selector snapshot store rework
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant useAsyncSelector
participant valueSnapshotStore
participant hybridCacheDB
Component->>useAsyncSelector: render
useAsyncSelector->>valueSnapshotStore: takeInitialRun()
valueSnapshotStore-->>useAsyncSelector: sync/async/error status
alt initial run is async
useAsyncSelector-->>Component: pending/loading state
useAsyncSelector->>hybridCacheDB: runCachedSelectorMaybeAsync
hybridCacheDB-->>useAsyncSelector: resolved value
useAsyncSelector->>valueSnapshotStore: publish(value)
useAsyncSelector-->>Component: success state with value
else initial run is sync
useAsyncSelector-->>Component: success state immediately
else initial run is error
useAsyncSelector->>valueSnapshotStore: getSnapshot()
useAsyncSelector-->>Component: error state
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/hyperdb/src/react/hooks.test.ts (1)
279-555: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the docs for the changed
useAsyncSelectorbehavior. The hook now skips the synchronous cache snapshot path and stays pending until the cached/async result resolves, sopackages/hyperdb-docand the rootREADME.mdshould be updated to match.🤖 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 `@packages/hyperdb/src/react/hooks.test.ts` around lines 279 - 555, The changed useAsyncSelector behavior now bypasses the synchronous cache snapshot path and remains pending until the cached or async result resolves, so update the documentation to reflect that flow. In the docs for useAsyncSelector, plus the root README.md and packages/hyperdb-doc, revise any descriptions that imply an immediate synchronous cache snapshot or sync success path; reference the hook name useAsyncSelector and the HybridDB cache behavior so the docs match the new pending-until-resolved semantics.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/hyperdb/src/react/hooks.test.ts (1)
437-479: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider asserting the middle call's args too.
Only
calls[0]andcalls[2]are checked againstproject-1; the intermediatecalls[1](presumablyproject-2, given the "across selector args" test intent) isn't asserted, leaving a gap in verifying the args-crossing behavior end-to-end.🤖 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 `@packages/hyperdb/src/react/hooks.test.ts` around lines 437 - 479, The test for useAsyncSelector does not fully verify the selector-args transition because it only checks the first and third runCachedSelectorMaybeAsync calls. Update this test to also assert the middle call’s args for the project-2 invocation, using the existing mocks.runCachedSelectorMaybeAsync.mock.calls checks, so the across-args behavior is verified end-to-end.
🤖 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 `@packages/hyperdb/src/react/hooks.ts`:
- Around line 495-500: The async initial selector path in the hooks logic can
still surface an unhandled rejection if the render is abandoned before the
effect attaches handlers. Update the code in the
`runInitial`/`isPromiseLike(value)` branch so the stored promise includes an
early no-op rejection handler (for example by chaining a catch that swallows the
rejection) before returning the `{ status: "async", promise, cmds }` result.
Keep the existing `status: "sync"` and `status: "error"` behavior unchanged.
- Line 589: The reset effect in hooks.ts is depending on input.selector, which
can change on every render when the selector is created inline and cause
repeated state resets. Update the effect around the setQueryState logic to
depend only on argsKey, and use genRef.current to read the latest selector
inside the effect instead of capturing input.selector from the dependency list.
---
Outside diff comments:
In `@packages/hyperdb/src/react/hooks.test.ts`:
- Around line 279-555: The changed useAsyncSelector behavior now bypasses the
synchronous cache snapshot path and remains pending until the cached or async
result resolves, so update the documentation to reflect that flow. In the docs
for useAsyncSelector, plus the root README.md and packages/hyperdb-doc, revise
any descriptions that imply an immediate synchronous cache snapshot or sync
success path; reference the hook name useAsyncSelector and the HybridDB cache
behavior so the docs match the new pending-until-resolved semantics.
---
Nitpick comments:
In `@packages/hyperdb/src/react/hooks.test.ts`:
- Around line 437-479: The test for useAsyncSelector does not fully verify the
selector-args transition because it only checks the first and third
runCachedSelectorMaybeAsync calls. Update this test to also assert the middle
call’s args for the project-2 invocation, using the existing
mocks.runCachedSelectorMaybeAsync.mock.calls checks, so the across-args behavior
is verified end-to-end.
🪄 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 Plus
Run ID: b29e1911-8885-49ce-bdc3-3b1c8420bbaa
📒 Files selected for processing (2)
packages/hyperdb/src/react/hooks.test.tspackages/hyperdb/src/react/hooks.ts
1 participant
Summary by CodeRabbit