fix(core): serial onLoadError block/advance for failed loadFn#27
Conversation
Default block keeps error occupying the serial lane (no leapfrog); advance removes the failed layer and drains the queue.
Align serial/error/scope/glossary/core-api copy with guide headers, occupancy wording, and AutoTypeTable for SerialOnLoadError.
Pre-1.0 bumps stay patch until v1.
Call onLayerDismiss before removing a failed serial advance layer; expand coverage; align apps/docs and maintainer docs with occupancy semantics; document leapfrog fix in the patch changeset.
Split the serial-queue lead, slim the glossary pointer, and align lifecycle mermaid with occupant leave.
SutuSebastian
added
the
docs
🦋 Changeset detectedLatest commit: b35fce9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 32 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 Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSerial stacks now support configurable handling for rejected ChangesSerial load error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant loadFn
participant LayerStack
participant QueuedLayer
loadFn-->>LayerStack: reject during layer loading
alt onLoadError is "block"
LayerStack->>LayerStack: retain error layer as occupant
else onLoadError is "advance"
LayerStack->>LayerStack: dismiss and remove failed layer
LayerStack->>QueuedLayer: activate next queued layer
end
🚥 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.
🧹 Nitpick comments (1)
packages/core/src/layerStack.ts (1)
230-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate state updates and align
onLayerDismisstiming.Currently, the
"advance"flow first setsphase: "error", then immediately overrides it withphase: "dismissed". This double update emits an unnecessary fleeting error state to layer subscribers. Additionally,this.onLayerDismiss?.(layer)is called before the layer's phase is updated to"dismissed", which slightly deviates from#commitDismisswhere the layer is marked"dismissed"before the teardown hook fires.You can bypass the intermediate error phase entirely and strictly mirror
#commitDismissby isolating the"advance"branch.♻️ Proposed refactor
- layer.setPartial({ error: error as E, phase: "error" }); - layer.reject(error as E); - if ( - this.#serial && - (this.options.scope?.onLoadError ?? "block") === "advance" - ) { - // Drop the failed occupant and drain — no lasting error UI on this stack. - // Mirror `#commitDismiss`: notify before remove so child stacks tear down. - this.onLayerDismiss?.(layer); - layer.setPartial({ - phase: "dismissed", - transition: "settled", - ended: true, - }); - this.#remove(layer); - return; - } + const isAdvance = + this.#serial && + (this.options.scope?.onLoadError ?? "block") === "advance"; + + if (isAdvance) { + // Drop the failed occupant and drain — no lasting error UI on this stack. + layer.reject(error as E); + layer.setPartial({ + error: error as E, + phase: "dismissed", + transition: "settled", + ended: true, + }); + // Mirror `#commitDismiss`: notify before remove so child stacks tear down. + this.onLayerDismiss?.(layer); + this.#remove(layer); + return; + } + + layer.setPartial({ error: error as E, phase: "error" }); + layer.reject(error as E);🤖 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/core/src/layerStack.ts` around lines 230 - 246, Isolate the serial `"advance"` branch before the initial error-state update in the surrounding load-error handling, so it does not emit `phase: "error"` before dismissal. In that branch, update the layer to its settled `"dismissed"` state first, then invoke `onLayerDismiss`, remove the layer, and return, matching the ordering used by `#commitDismiss`; keep the existing error state and rejection flow for other cases.
🤖 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/core/src/layerStack.ts`:
- Around line 230-246: Isolate the serial `"advance"` branch before the initial
error-state update in the surrounding load-error handling, so it does not emit
`phase: "error"` before dismissal. In that branch, update the layer to its
settled `"dismissed"` state first, then invoke `onLayerDismiss`, remove the
layer, and return, matching the ordering used by `#commitDismiss`; keep the
existing error state and rejection flow for other cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa032112-1b49-4723-950b-8aaf4a4f1790
📒 Files selected for processing (13)
.changeset/serial-onload-error.mdapps/docs/content/concepts/glossary.mdxapps/docs/content/concepts/lifecycle.mdxapps/docs/content/concepts/stacks-scope-gc.mdxapps/docs/content/guides/error-handling.mdxapps/docs/content/guides/serial-queues.mdxapps/docs/content/reference/core-api.mdxdocs/architecture.mddocs/glossary.mdpackages/core/skills/layers/SKILL.mdpackages/core/src/layerStack.test.tspackages/core/src/layerStack.tspackages/core/src/types.ts
Skip intermediate phase error on onLoadError advance; mark dismissed before onLayerDismiss, matching #commitDismiss.
Fact-check: advance path ordering nitVerdict:
Thanks — ordering now mirrors |
1 participant
Summary
loadFnas occupying the lane by default (onLoadError: "block"), fixing leapfrog where a later open could mount while an error layer stayed up.onLoadError: "advance"to remove the failed layer, fireonLayerDismiss(child teardown), and drain the next queued open.Test plan
bun test packages/core/src/layerStack.test.ts(block, advance, empty queue, onLayerDismiss, dismiss-after-advance, parallel ignores)bun run docs:check -- --isolated+docs:buildSummary by CodeRabbit
New Features
onLoadError: "advance"option to remove failed layers and continue with queued layers.Documentation