Conversation
|
pkg.pr.new packages benchmark commit |
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.83, 1.77, 4.06, 6.14, 6.55, 10.63, 20.54, 20.29]
line [0.91, 1.78, 3.75, 5.39, 6.61, 10.57, 21.23, 21.79]
line [0.85, 1.70, 3.56, 5.67, 6.60, 10.37, 20.47, 21.04]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.26, 0.46, 0.61, 0.74, 0.99, 1.11, 1.26, 1.40]
line [0.29, 0.46, 0.61, 0.72, 1.01, 1.05, 1.26, 1.38]
line [0.30, 0.45, 0.58, 0.72, 0.98, 1.01, 1.30, 1.38]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.78, 1.87, 3.28, 5.93, 10.52, 23.49, 49.09, 100.02]
line [0.81, 1.99, 3.36, 5.97, 10.70, 22.88, 49.03, 96.98]
line [0.87, 1.97, 3.25, 6.17, 11.05, 22.69, 46.93, 96.30]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased | ❔ Unknown |
|---|---|---|---|
| 0 | 325 | 0 | 0 |
No notable changes.
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
87cdbd8 to
749ab17
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two critical issues remain in root-context.tsx involving shared suspension state and incompatible Promise.resolve type inference.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates @typegpu/react root hooks to avoid unnecessary suspension after root fulfillment and adds lifecycle coverage.
Changes:
- Adjusts root promise and suspension handling.
- Adds tests for fulfillment, suspension, rejection, and hook ordering.
File summaries
| File | Description |
|---|---|
packages/typegpu-react/tests/use-root.test.tsx |
Adds coverage for root hook behavior. |
packages/typegpu-react/src/core/root-context.tsx |
Updates root promise handling and suspension behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | RootContextRejectedResult; | ||
|
|
||
| interface RootContext { | ||
| wasSuspended: boolean; |
| status: 'fulfilled', | ||
| value: root, | ||
| promise: Promise.resolve(root), | ||
| settledPromise: Promise.resolve({ status: 'fulfilled', value: root }), |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one stale-comment nit inline.
Reviewed changes
- Fulfilled-root discrimination — adds
wasSuspendedtoRootContext,OwnRootContextandExistingRootContext, and makespromise/settledPromiserequired onRootContextFulfilledResult. useRoot/useRootOrError— early-return the fulfilled value when the context was never suspended on, otherwiseuse(...)the promise and reset the flag on success.- Tests — new
use-root.test.tsx(11 cases): existing-root no-suspend, owned-root suspend→return, late mount after settle, hook-order stability, no-suspend when initialized byuseRootWithStatus, and the error/rejection paths.
I verified the change locally: the new suite passes (11/11, 40/40 across the package), test:types is clean, and reverting the two hook bodies to the old promise ? use(...) : value logic makes the new tests fail — so the tests genuinely exercise the fix. The flag logic is safe: the early return only skips use for an already-fulfilled result, and a stale wasSuspended = true can at worst force a redundant use on a fulfilled promise, never a wrong value or a skipped required suspension.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
749ab17 to
7dd37f9
Compare
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the per-hook
useRefrefactor is a clean narrowing of scope. Approval is withheld only because the earlier stale-comment thread above remains open, not because of any new finding.
Reviewed changes
- Relocated suspension state — removed
wasSuspendedfrom theRootContextinterface,OwnRootContext, andExistingRootContext, replacing the shared field with auseRef(false)local to each ofuseRootanduseRootOrError. No other code referenced the field. - Kept the fulfilled-result contract —
RootContextFulfilledResult.promise/.settledPromisestay required, andExistingRootContextstill populates both with resolved promises; the early-return condition now reads!wasSuspended.current.
Placing the flag on a per-call ref is strictly better than the previous shared field: a suspension by one consumer can no longer force another consumer, or a caller on the module-level globalRootContextValue, onto the use(...) path. It remains safe because the early return only fires for status === 'fulfilled'. I verified the package suite (40/40 across 6 files) and pnpm --filter @typegpu/react test:types are green at c58fc5c6.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
c58fc5c to
b7cd082
Compare
b7cd082 to
4408c89
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Reworded the suspension comment in
useRootanduseRootOrErrorto describe thewasSuspendeddiscriminator instead of the old "no promise to suspend on before" rationale. This addresses prior review feedback; the two hook bodies are otherwise unchanged. - Test-only cleanups (carried in the force-push):
CatcherrendersError.messageforErrorinstances, plus two Biome whitespace fixes.
The comment now matches the condition and the earlier open thread is resolved. I verified the package suite (40/40 across 6 files) and pnpm --filter @typegpu/react test:types are green at 4408c895.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
No description provided.