fix(core): harden composition and animation hooks#264
Merged
RtlZeroMemory merged 5 commits intomainfrom Mar 6, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR adds fragment-based container support, makes WidgetContext.useViewport required, replaces per-hook timers with a shared frame-driven animation scheduler, introduces/clarifies animation utility hooks and docs, clamps streaming reconnect delays, and adds tests and harness updates for layout and animation behavior. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Hook as Hook (useTransition / useParallel / useStagger)
end
rect rgba(200,255,200,0.5)
participant Scheduler as AnimationLoopScheduler
end
rect rgba(255,230,200,0.5)
participant Frame as Frame Source (rAF / tick)
end
Hook->>Scheduler: subscribe(callback, handle)
Scheduler->>Frame: requestFrame() when first subscriber
Frame-->>Scheduler: frameTick(timestamp)
Scheduler->>Hook: invoke callback(step, timestamp)
alt animation complete
Hook->>Scheduler: unsubscribe(callback)
Hook->>Hook: safeInvoke(onComplete)
end
Scheduler->>Frame: requestFrame() while subscribers exist
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🧹 Nitpick comments (2)
CHANGELOG.md (1)
11-11: Break the dense sentence into bullet points for readability and consistency.The Bug Fixes entry condenses 6+ distinct changes into a single 160+ word sentence. Previous releases (e.g., lines 42-44, 71-74) use bullet points to list multiple fixes, making them easier to scan and reference.
♻️ Proposed refactor for improved readability
- - **core/composition + hooks**: Composite widgets now use a layout-transparent default wrapper, animation hooks share a frame driver, transition/orchestration hooks stop relying on stringified config signatures, `useAnimatedValue` transition playback preserves progress across pause/resume, `useStagger` restarts on same-length item replacement, and streaming hook reconnect delays clamp away tight-loop reconnects. + - **core/composition**: Composite widgets now use a layout-transparent default wrapper (fragment) instead of implicit column layout. + - **hooks/animation**: Animation hooks now share a unified frame driver for frame-based scheduling, replacing prior timer mechanisms. + - **hooks/animation**: Transition and orchestration hooks no longer rely on stringified config signatures for invalidation. + - **hooks/animation**: `useAnimatedValue` with `mode: "transition"` now preserves playback progress across pause/resume. + - **hooks/animation**: `useParallel` and `useChain` now read latest callbacks to prevent stale closures. + - **hooks/animation**: `useStagger` now restarts when item identity changes for the same-length array. + - **hooks/streaming**: `useEventSource` and `useWebSocket` now clamp reconnect delays to prevent tight-loop reconnections.Note: This refactor also adds the missing
useParallel/useChaincallback behavior mentioned in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 11, Split the long Bug Fixes sentence under "core/composition + hooks" into separate bullet points so each distinct change is its own line: create bullets for "Composite widgets now use a layout-transparent default wrapper", "animation hooks share a frame driver", "transition/orchestration hooks stop relying on stringified config signatures", "`useAnimatedValue` transition playback preserves progress across pause/resume", "`useStagger` restarts on same-length item replacement", "streaming hook reconnect delays clamp away tight-loop reconnects", and add the missing bullet about `useParallel`/`useChain` callback behavior; preserve the original wording for each item and follow the existing bullet style used elsewhere in the changelog.packages/core/src/forms/__tests__/form.fieldArray.test.ts (1)
25-78: Consider using the sharedcreateFormHarnessfromharness.ts.This file defines its own
createTestContextfunction which largely duplicatescreateFormHarnessinharness.ts. Both now provide identicaluseViewportimplementations. Using the shared harness would reduce duplication and ensure consistency when the test harness evolves.♻️ Suggested refactor
-import { - createCompositeInstanceRegistry, - createHookContext, - runPendingEffects, -} from "../../runtime/instances.js"; -import type { WidgetContext } from "../../widgets/composition.js"; import type { UseFormOptions, UseFormReturn } from "../types.js"; -import { useForm } from "../useForm.js"; +import { createFormHarness } from "./harness.js";Then replace
createTestContext()calls withcreateFormHarness().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/forms/__tests__/form.fieldArray.test.ts` around lines 25 - 78, The test file defines a duplicate createTestContext function that mirrors the shared createFormHarness in harness.ts (including the same useViewport behavior); replace the local createTestContext with the shared createFormHarness to remove duplication: remove or stop exporting the local createTestContext, import and call createFormHarness instead of createTestContext in this test, and ensure the same generic typing and return shape are preserved so existing usages of createTestContext (and calls to methods like render, unmount, getInvalidateCount) continue to work unchanged; verify useViewport behavior matches by using the harness implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-11: Add a user-visible change note to the changelog documenting
that WidgetContext.useViewport is now required and createWidgetContext(...) will
always supply it; update the "Bug Fixes" entry or add a "Breaking Changes"
section that explicitly lists "Require WidgetContext.useViewport" and describes
that callers creating custom widget contexts must now provide a useViewport
implementation (mention createWidgetContext as the helper that will supply it
going forward). Ensure the language is concise and flagged as a breaking change
so users know to update existing code that constructs widget contexts.
In
`@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts`:
- Around line 165-174: The test file formatting is out of sync for the
"useParallel playback pause and resume preserves elapsed progress" test: run the
project formatter (e.g., biome format --write or your repo's configured
formatter) on this file to reformat the block that declares running, paused, and
the render call (variables running, paused, and render and functions
createHarness and useParallel) so the CI no longer flags lines 166-170; commit
the formatted changes.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 11: Split the long Bug Fixes sentence under "core/composition + hooks"
into separate bullet points so each distinct change is its own line: create
bullets for "Composite widgets now use a layout-transparent default wrapper",
"animation hooks share a frame driver", "transition/orchestration hooks stop
relying on stringified config signatures", "`useAnimatedValue` transition
playback preserves progress across pause/resume", "`useStagger` restarts on
same-length item replacement", "streaming hook reconnect delays clamp away
tight-loop reconnects", and add the missing bullet about
`useParallel`/`useChain` callback behavior; preserve the original wording for
each item and follow the existing bullet style used elsewhere in the changelog.
In `@packages/core/src/forms/__tests__/form.fieldArray.test.ts`:
- Around line 25-78: The test file defines a duplicate createTestContext
function that mirrors the shared createFormHarness in harness.ts (including the
same useViewport behavior); replace the local createTestContext with the shared
createFormHarness to remove duplication: remove or stop exporting the local
createTestContext, import and call createFormHarness instead of
createTestContext in this test, and ensure the same generic typing and return
shape are preserved so existing usages of createTestContext (and calls to
methods like render, unmount, getInvalidateCount) continue to work unchanged;
verify useViewport behavior matches by using the harness implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ba29812-d3d2-4b36-b512-8e4c457dda96
📒 Files selected for processing (19)
CHANGELOG.mdCLAUDE.mddocs/guide/animation.mddocs/guide/composition.mddocs/guide/hooks-reference.mdpackages/core/src/forms/__tests__/form.disabled.test.tspackages/core/src/forms/__tests__/form.fieldArray.test.tspackages/core/src/forms/__tests__/form.wizard.test.tspackages/core/src/forms/__tests__/harness.tspackages/core/src/forms/__tests__/useForm.test.tspackages/core/src/runtime/__tests__/reconcile.composite.test.tspackages/core/src/widgets/__tests__/composition.animationHooks.test.tspackages/core/src/widgets/__tests__/composition.animationOrchestration.test.tspackages/core/src/widgets/__tests__/composition.animationPlayback.test.tspackages/core/src/widgets/__tests__/composition.test.tspackages/core/src/widgets/__tests__/composition.useAnimatedValue.test.tspackages/core/src/widgets/composition.tspackages/core/src/widgets/hooks/animation.tspackages/core/src/widgets/hooks/data.ts
packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts
Outdated
Show resolved
Hide resolved
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b85abaf1f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts (1)
355-397:⚠️ Potential issue | 🟡 MinorFix formatting issues flagged by CI.
The pipeline reports that Biome formatter would have printed different content for lines 356-377. Run the formatter before merging.
Suggested fix
Run the project formatter to resolve the formatting discrepancy:
npx biome format --write packages/core/src/widgets/__tests__/composition.animationOrchestration.test.tsOr use the project's lint script if configured.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts` around lines 355 - 397, The test "useChain playback changes do not reset the active step" has formatting differences flagged by CI; run the project formatter (e.g. npx biome format --write) or the repo's lint/format script to reformat packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts so the block containing the running/paused arrays and the test body matches the project's style, then re-add the formatted file to the PR and push the changes.
🧹 Nitpick comments (2)
packages/core/src/widgets/hooks/animation.ts (1)
574-597: Deduplicate transition state sync logic to avoid drift.The transition reset/sync block is duplicated in
useTransitionanduseAnimatedValue(transition mode) even thoughsyncTransitionRunState(...)exists. Reusing the helper in both places will keep behavior aligned long-term.♻️ Refactor sketch
- const state = transitionStateRef.current; - const shouldReset = - !state.initialized || - !Object.is(state.to, value) || - state.durationMs !== durationMs || - state.delayMs !== delayMs || - state.easing !== easing; - - if (shouldReset) { - state.initialized = true; - state.from = currentRef.current; - state.to = value; - state.durationMs = durationMs; - state.delayMs = delayMs; - state.easing = easing; - state.elapsedMs = playback.reversed ? durationMs : 0; - state.pendingDelayMs = delayMs; - } else { - state.durationMs = durationMs; - state.delayMs = delayMs; - state.easing = easing; - if (state.elapsedMs < 0) state.elapsedMs = 0; - if (state.elapsedMs > state.durationMs) state.elapsedMs = state.durationMs; - } + const state = transitionStateRef.current; + syncTransitionRunState( + state, + currentRef.current, + value, + durationMs, + easing, + delayMs, + playback.reversed, + );Also applies to: 1068-1091
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/hooks/animation.ts` around lines 574 - 597, Replace the duplicated transition reset/sync logic in useTransition and useAnimatedValue (transition mode) by calling the existing helper syncTransitionRunState instead of manually mutating transitionStateRef.current; pass the current transition state and the relevant inputs (new target value, durationMs, delayMs, easing, currentRef/current value, and playback/reversed) so syncTransitionRunState performs the initialization/update and clamping of elapsed/pendingDelay consistently across both hooks and avoids drift.CHANGELOG.md (1)
16-21: Optional: compact repeated animation bullets for readability.These lines repeat the same prefix many times; grouping can make the section easier to scan.
📝 Optional wording cleanup
-- **hooks/animation**: Animation hooks now share a frame driver. -- **hooks/animation**: Transition/orchestration hooks stop relying on stringified config signatures. -- **hooks/animation**: `useAnimatedValue` transition playback preserves progress across pause/resume. -- **hooks/animation**: `useParallel` and `useChain` now read the latest callbacks without stale-closure behavior. -- **hooks/animation**: `useStagger` restarts on same-length item replacement. +- **hooks/animation**: + - Animation hooks now share a frame driver. + - Transition/orchestration hooks stop relying on stringified config signatures. + - `useAnimatedValue` transition playback preserves progress across pause/resume. + - `useParallel` and `useChain` now read the latest callbacks without stale-closure behavior. + - `useStagger` restarts on same-length item replacement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 16 - 21, The changelog repeats the "hooks/animation" prefix across multiple lines; consolidate these into a single "hooks/animation" entry followed by comma-separated or semicolon-separated sub-items for the specific changes (e.g., "shared frame driver", "no stringified config signatures", "`useAnimatedValue` preserves progress on pause/resume", "`useParallel`/`useChain` avoid stale closures", "`useStagger` restarts on same-length replacement") and keep the existing "hooks/streaming" line separate; update the text so the unique symbols `useAnimatedValue`, `useParallel`, `useChain`, and `useStagger` are still mentioned for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts`:
- Around line 355-397: The test "useChain playback changes do not reset the
active step" has formatting differences flagged by CI; run the project formatter
(e.g. npx biome format --write) or the repo's lint/format script to reformat
packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts
so the block containing the running/paused arrays and the test body matches the
project's style, then re-add the formatted file to the PR and push the changes.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 16-21: The changelog repeats the "hooks/animation" prefix across
multiple lines; consolidate these into a single "hooks/animation" entry followed
by comma-separated or semicolon-separated sub-items for the specific changes
(e.g., "shared frame driver", "no stringified config signatures",
"`useAnimatedValue` preserves progress on pause/resume",
"`useParallel`/`useChain` avoid stale closures", "`useStagger` restarts on
same-length replacement") and keep the existing "hooks/streaming" line separate;
update the text so the unique symbols `useAnimatedValue`, `useParallel`,
`useChain`, and `useStagger` are still mentioned for clarity.
In `@packages/core/src/widgets/hooks/animation.ts`:
- Around line 574-597: Replace the duplicated transition reset/sync logic in
useTransition and useAnimatedValue (transition mode) by calling the existing
helper syncTransitionRunState instead of manually mutating
transitionStateRef.current; pass the current transition state and the relevant
inputs (new target value, durationMs, delayMs, easing, currentRef/current value,
and playback/reversed) so syncTransitionRunState performs the
initialization/update and clamping of elapsed/pendingDelay consistently across
both hooks and avoids drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 984f1952-3846-47b3-b04f-3a7254aa2ff4
📒 Files selected for processing (16)
CHANGELOG.mdpackages/core/src/forms/__tests__/form.disabled.test.tspackages/core/src/forms/__tests__/form.fieldArray.test.tspackages/core/src/forms/__tests__/form.wizard.test.tspackages/core/src/forms/__tests__/harness.tspackages/core/src/forms/__tests__/useForm.test.tspackages/core/src/layout/engine/layoutEngine.tspackages/core/src/renderer/renderToDrawlist/overflowCulling.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/runtime/commit.tspackages/core/src/widgets/__tests__/composition.animationOrchestration.test.tspackages/core/src/widgets/__tests__/composition.useAnimatedValue.test.tspackages/core/src/widgets/hooks/animation.tspackages/core/src/widgets/protocol.tspackages/core/src/widgets/tests/protocol.test.tspackages/core/src/widgets/types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/forms/tests/form.wizard.test.ts
- packages/core/src/widgets/tests/composition.useAnimatedValue.test.ts
- packages/core/src/forms/tests/harness.ts
- packages/core/src/forms/tests/useForm.test.ts
- packages/core/src/forms/tests/form.disabled.test.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.
Summary
fragmentwrapper and align the widget context contract arounduseViewportuseAnimatedValueWhat changed
defineWidget(...)now defaults to a layout-transparentfragmentwrapper instead of silently inserting acolumnWidgetContext.useViewportis required andcreateWidgetContext(...)now supplies it consistentlyuseParallelanduseChainread latest callbacks without stale-closure behavioruseAnimatedValue(..., { mode: "transition" })preserves pause/resume progressuseStagger(...)restarts on same-length item identity changes instead of only count changesuseEventSource(...)anduseWebSocket(...)clamp reconnect delays away from tight-loop reconnectsValidation
npx tsx --test packages/core/src/widgets/__tests__/composition.test.ts packages/core/src/runtime/__tests__/reconcile.composite.test.ts packages/core/src/widgets/__tests__/composition.useAnimatedValue.test.ts packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts packages/core/src/widgets/__tests__/composition.animationHooks.test.ts packages/core/src/widgets/__tests__/composition.animationPlayback.test.ts packages/core/src/widgets/__tests__/composition.streamingHooks.test.tsnpx tsx --test packages/core/src/forms/__tests__/useForm.test.ts packages/core/src/forms/__tests__/form.disabled.test.ts packages/core/src/forms/__tests__/form.fieldArray.test.ts packages/core/src/forms/__tests__/form.wizard.test.tsnode scripts/run-tests.mjs4866pass /54fail300x68bridge->engineering), theme cycle, and quitbackend_submitted=877,worker_completed=877,hash_mismatch_backend_vs_worker=0Notes
Summary by CodeRabbit
Breaking Changes
New Features
Bug Fixes
Documentation