feat(jsx): add useAsync hook for managing asynchronous state and retries - #3743
Open
karthikj5453 wants to merge 4 commits into
Open
feat(jsx): add useAsync hook for managing asynchronous state and retries#3743karthikj5453 wants to merge 4 commits into
karthikj5453 wants to merge 4 commits into
Conversation
github-actions
Bot
added
type:feature
📝 WalkthroughWalkthroughThe PR adds a configurable ChangesAsync hook
Rating display width
Contributor records
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (1)
packages/jsx/src/hooks/useAsync.test.ts (1)
39-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSettle the promise started by
useAsync.These tests call
asyncFn,successFn, orfailFndirectly after the hook starts its own request. Each direct call creates a separate operation. The tests can miss duplicate execution and do not control the request that updates hook state.Use a deferred promise from the first mock invocation. Resolve or reject that promise, flush microtasks, and assert the mock call count before rerendering.
Also applies to: 58-65, 82-90, 101-114, 120-126
🤖 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/jsx/src/hooks/useAsync.test.ts` around lines 39 - 41, Update the affected useAsync tests around their initial render and rerender flows to control the request created by the hook: make the first asyncFn, successFn, or failFn mock invocation return a deferred promise, settle it by resolving or rejecting the deferred, flush microtasks, and assert the mock call count before rerendering. Remove direct callback invocations that start separate operations, preserving each test’s existing success and failure assertions.
🤖 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/jsx/src/hooks/useAsync.ts`:
- Around line 92-107: Update the async execution flow in useAsync to call
setData(null) when loading begins and again when the request fails, ensuring
loading and error states do not retain successful data. Add a regression test
covering a successful execution followed by a failed refetch and verify data is
null after the failure.
- Line 35: Update useAsync and UseAsyncResult to accept a TArgs extends
unknown[] generic, replacing any[] in asyncFn and execute signatures with TArgs
so argument types are preserved and invalid execute calls are rejected. Replace
the referenced dependency-array any[] usages with unknown[], and thread TArgs
through all related declarations and returned functions.
In `@packages/ui/src/Rating.ts`:
- Around line 246-251: Update the glyph-rendering loop in Rating’s rendering
method to compute glyphWidth before writing, stop when glyphWidth is
non-positive or would exceed maxX, and advance currentX by glyphWidth after
rendering. Add regression tests covering a width-2 glyph at the final column and
a width-0 glyph followed by another glyph.
---
Nitpick comments:
In `@packages/jsx/src/hooks/useAsync.test.ts`:
- Around line 39-41: Update the affected useAsync tests around their initial
render and rerender flows to control the request created by the hook: make the
first asyncFn, successFn, or failFn mock invocation return a deferred promise,
settle it by resolving or rejecting the deferred, flush microtasks, and assert
the mock call count before rerendering. Remove direct callback invocations that
start separate operations, preserving each test’s existing success and failure
assertions.
🪄 Autofix
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: b463875d-860a-4a12-876b-4a041cc7f767
📒 Files selected for processing (6)
CONTRIBUTORS.mdpackages/jsx/src/hooks.tspackages/jsx/src/hooks/useAsync.test.tspackages/jsx/src/hooks/useAsync.tspackages/jsx/src/index.tspackages/ui/src/Rating.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/jsx/src/hooks/useAsync.test.ts (2)
157-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCall
refetch()in the failed-refetch test.If this test covers the public
refetchpath, callres.refetch()on Line 175. The current call tores.execute()only tests direct execution. A regression in therefetchwrapper would not be detected here.Proposed test adjustment
- const failPromise = res.execute(); + const failPromise = res.refetch();Also applies to: 175-176
🤖 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/jsx/src/hooks/useAsync.test.ts` at line 157, Update the failed-refetch test “clears stale data during loading and error states on failed refetch” to invoke the public res.refetch() method instead of res.execute() at the existing retry call, ensuring the test covers the refetch wrapper path while preserving its loading and error assertions.
182-190: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an unmount-before-settlement test.
The test calls
destroyFiber(fiber)only afterfailPromisesettles on Line 182. This does not verify the hook's unmount guard. Add a deferred request, destroy the fiber before it settles, and assert that stale success or error callbacks do not run.🤖 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/jsx/src/hooks/useAsync.test.ts` around lines 182 - 190, Add an unmount-before-settlement test near the existing useAsync failure coverage, using a deferred request to keep the promise pending while the rendered hook is active. Call destroyFiber(fiber) before resolving or rejecting the deferred request, then settle it and assert that no stale success or error state callbacks run after unmount.
🤖 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.
Nitpick comments:
In `@packages/jsx/src/hooks/useAsync.test.ts`:
- Line 157: Update the failed-refetch test “clears stale data during loading and
error states on failed refetch” to invoke the public res.refetch() method
instead of res.execute() at the existing retry call, ensuring the test covers
the refetch wrapper path while preserving its loading and error assertions.
- Around line 182-190: Add an unmount-before-settlement test near the existing
useAsync failure coverage, using a deferred request to keep the promise pending
while the rendered hook is active. Call destroyFiber(fiber) before resolving or
rejecting the deferred request, then settle it and assert that no stale success
or error state callbacks run after unmount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b8bc3a8-c584-4062-9239-fe583036a5b8
📒 Files selected for processing (2)
packages/jsx/src/hooks/useAsync.test.tspackages/jsx/src/hooks/useAsync.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jsx/src/hooks/useAsync.ts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Adds a dedicated
useAsynchook to@termuijs/jsxto streamline asynchronous operations (data fetching, process execution, file I/O). The hook managesidle,loading,success, anderrorstates, supports immediate execution, manual triggers (execute), state reset (reset), and includes unmount safety guards to prevent stale updates.Which package(s)?
@termuijs/jsxType of Change
type:feature)Checklist
bun vitest run packages/jsxbun run buildbun run typecheckCONTRIBUTING.md.type: short description.anytypes without an inline comment.GSSoC 2026 Participation
Your GSSoC profile: https://gssoc.girlscript.org/profile/karthikj5453
Summary by CodeRabbit
New Features
useAsynchook with loading, success, error, idle, reset, refetch, and manual execution support.Bug Fixes
Tests