feat: add lane-based priority system#35
Open
MarcelOlsen wants to merge 2 commits intofiber/type-guardsfrom
Open
feat: add lane-based priority system#35MarcelOlsen wants to merge 2 commits intofiber/type-guardsfrom
MarcelOlsen wants to merge 2 commits intofiber/type-guardsfrom
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This was referenced Feb 13, 2026
Owner
Author
This was referenced Feb 13, 2026
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
6fd8dfb to
8839bd3
Compare
MarcelOlsen
force-pushed
the
fiber/lanes
branch
2 times, most recently
from
7a67796 to
537be0d
Compare
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
8839bd3 to
40c8468
Compare
MarcelOlsen
force-pushed
the
fiber/lanes
branch
from
537be0d to
d999bbd
Compare
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
922519f to
ad74e21
Compare
MarcelOlsen
force-pushed
the
fiber/lanes
branch
2 times, most recently
from
4367aa1 to
8bf02c1
Compare
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
ad74e21 to
712f744
Compare
MarcelOlsen
force-pushed
the
fiber/lanes
branch
from
8bf02c1 to
af1b12e
Compare
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
712f744 to
20e7d76
Compare
MarcelOlsen
force-pushed
the
fiber/lanes
branch
from
af1b12e to
de99471
Compare
MarcelOlsen
force-pushed
the
fiber/type-guards
branch
from
20e7d76 to
e0d8218
Compare
MarcelOlsen
force-pushed
the
fiber/lanes
branch
7 times, most recently
from
638b997 to
3497a64
Compare
Add Lane type operations for priority scheduling: mergeLanes, removeLanes, includesSomeLane, and lane management utilities.
MarcelOlsen
force-pushed
the
fiber/lanes
branch
from
3497a64 to
ef8320c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/fiber/lanes.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🧬 Code graph analysis (1)
src/fiber/lanes.ts (1)
src/fiber/types.ts (6)
Lanes(54-54)createLanes(65-65)Lane(47-47)createLane(64-64)FiberRoot(390-443)Fiber(225-334)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fiber/lanes.ts`:
- Around line 500-502: The function formatLanes currently uses a hard-coded
padStart(31); update formatLanes to derive the width from a single source of
truth: either introduce a constant (e.g., MAX_LANE_BITS or LANE_BIT_COUNT) or
compute the bit width from the highest defined lane value (e.g., determine the
highest set bit of the Lanes type/enum) and use that value in padStart. Modify
the implementation in formatLanes (and any related lane-definition file) so the
padding value is not magic but comes from the new constant or computed bit
length, ensuring formatLanes stays correct when lanes change.
- Around line 344-353: In scheduleUpdateOnFiber, you only update fiber.lanes and
parent.childLanes, which misses marking the alternate tree; mirror the lane
updates to fiber.alternate (if present) by setting alternate.lanes =
mergeLanes(alternate.lanes, lane), and for each parent also update
parent.alternate.childLanes = mergeLanes(parent.alternate.childLanes, lane) when
parent.alternate exists; use the same mergeLanes utility and ensure you check
for null/undefined alternates while walking the return chain so both current and
alternate trees retain the new lane markings.
- Around line 221-289: getNextLanes ignores root.expiredLanes so lanes marked by
markRootExpired never get chosen; fix by reading root.expiredLanes at the start
of getNextLanes and prioritizing any expired pending lanes: compute
expiredPendingLanes = intersectLanes(pendingLanes, root.expiredLanes) and if
expiredPendingLanes !== NoLanes immediately return
getHighestPriorityLanes(expiredPendingLanes) (place this check before the
suspended/non-idle/idle branching) so expired lanes always preempt normal
scheduling.
- Around line 294-296: markRootUpdated currently only sets root.pendingLanes via
mergeLanes(updateLane) but fails to clear any suspended/pinged state for that
lane, which can cause the new update to remain blocked; update markRootUpdated
to also remove the updateLane bit from the root's suspended/pinged sets (e.g.,
root.suspendedLanes and root.pingedLanes) by ANDing them with the inverse of
updateLane after merging into root.pendingLanes so the newly arriving work is
unsuspended and eligible for getNextLanes.
- Around line 45-50: NonIdleLanes currently uses a hard-coded binary literal;
replace that with a derived mask so it auto-updates when lane constants change.
Compute NonIdleLanes by combining the existing lane constants (e.g., OR together
each non-idle lane constant such as SyncLane, InputContinuousLane, ...), or
derive it from AllLanes minus the idle lane (e.g., createLanes(AllLanes &
~IdleLane)), and use that expression in place of the literal in the NonIdleLanes
declaration so the value is maintained by the lane constants and createLanes
call.
- Around line 436-448: claimNextTransitionLane currently generates lanes by
shifting 1 by currentTransitionLane and then clamping, which cycles through
irrelevant bit positions; instead rotate only through the real transition lane
values by computing the lane as (TransitionLane1 shifted left by
currentTransitionLane) or by indexing into the sequence of transition lanes
between TransitionLane1 and TransitionLane2, and update currentTransitionLane
modulo the number of transition lanes; replace the existing
bit-1<<currentTransitionLane logic with this computed transition lane and return
createLane(lane) (use symbols claimNextTransitionLane, currentTransitionLane,
TransitionLane1, TransitionLane2, createLane).
Comment on lines
+45
to
+50
| /** | ||
| * All non-idle lanes. | ||
| */ | ||
| export const NonIdleLanes: Lanes = | ||
| createLanes(0b0001111111111111111111111111111); | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hard-coded NonIdleLanes mask is a future footgun.
Line 48 bakes in a binary mask that will drift the second lane layout changes. Derive it from lane constants so it self-updates.
♻️ Safer, self-documenting mask
-export const NonIdleLanes: Lanes =
- createLanes(0b0001111111111111111111111111111);
+export const NonIdleLanes: Lanes = createLanes(
+ (SyncLane as number) |
+ (InputContinuousLane as number) |
+ (DefaultLane as number) |
+ (TransitionLane1 as number) |
+ (TransitionLane2 as number) |
+ (OffscreenLane as number),
+);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fiber/lanes.ts` around lines 45 - 50, NonIdleLanes currently uses a
hard-coded binary literal; replace that with a derived mask so it auto-updates
when lane constants change. Compute NonIdleLanes by combining the existing lane
constants (e.g., OR together each non-idle lane constant such as SyncLane,
InputContinuousLane, ...), or derive it from AllLanes minus the idle lane (e.g.,
createLanes(AllLanes & ~IdleLane)), and use that expression in place of the
literal in the NonIdleLanes declaration so the value is maintained by the lane
constants and createLanes call.
Comment on lines
+221
to
+289
| export function getNextLanes( | ||
| root: FiberRoot, | ||
| wipLanes: Lanes = NoLanes, | ||
| ): Lanes { | ||
| const pendingLanes = root.pendingLanes; | ||
|
|
||
| if (pendingLanes === NoLanes) { | ||
| return NoLanes; | ||
| } | ||
|
|
||
| let nextLanes = NoLanes; | ||
|
|
||
| // Check for suspended lanes | ||
| const suspendedLanes = root.suspendedLanes; | ||
| const pingedLanes = root.pingedLanes; | ||
|
|
||
| // Non-suspended lanes | ||
| const nonIdlePendingLanes = intersectLanes(pendingLanes, NonIdleLanes); | ||
|
|
||
| if (nonIdlePendingLanes !== NoLanes) { | ||
| // Work on non-idle lanes first | ||
| const nonSuspendedLanes = removeLanes(nonIdlePendingLanes, suspendedLanes); | ||
|
|
||
| if (nonSuspendedLanes !== NoLanes) { | ||
| nextLanes = getHighestPriorityLanes(nonSuspendedLanes); | ||
| } else { | ||
| // All non-idle lanes are suspended, check pinged | ||
| const nonIdlePingedLanes = intersectLanes( | ||
| nonIdlePendingLanes, | ||
| pingedLanes, | ||
| ); | ||
| if (nonIdlePingedLanes !== NoLanes) { | ||
| nextLanes = getHighestPriorityLanes(nonIdlePingedLanes); | ||
| } | ||
| } | ||
| } else { | ||
| // Only idle lanes remain | ||
| const unblockedLanes = removeLanes(pendingLanes, suspendedLanes); | ||
| if (unblockedLanes !== NoLanes) { | ||
| nextLanes = getHighestPriorityLanes(unblockedLanes); | ||
| } else { | ||
| const idlePingedLanes = intersectLanes(pendingLanes, pingedLanes); | ||
| if (idlePingedLanes !== NoLanes) { | ||
| nextLanes = getHighestPriorityLanes(idlePingedLanes); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (nextLanes === NoLanes) { | ||
| return NoLanes; | ||
| } | ||
|
|
||
| // Check if currently rendering lanes should be prioritized | ||
| if ( | ||
| wipLanes !== NoLanes && | ||
| wipLanes !== nextLanes && | ||
| !includesAnyLanes(nextLanes, suspendedLanes) | ||
| ) { | ||
| const wipHighestLane = getHighestPriorityLane(wipLanes); | ||
| const nextHighestLane = getHighestPriorityLane(nextLanes); | ||
|
|
||
| if ((wipHighestLane as number) <= (nextHighestLane as number)) { | ||
| // WIP lanes are higher or equal priority | ||
| return wipLanes; | ||
| } | ||
| } | ||
|
|
||
| return nextLanes; | ||
| } |
Contributor
There was a problem hiding this comment.
Expired lanes are getting completely ghosted.
Line 225+ never checks root.expiredLanes, so markRootExpired can set lanes that never get picked, which defeats “must render sync.” That’s a scheduling correctness bug.
🛠️ Minimal fix to prioritize expired lanes
export function getNextLanes(
root: FiberRoot,
wipLanes: Lanes = NoLanes,
): Lanes {
const pendingLanes = root.pendingLanes;
if (pendingLanes === NoLanes) {
return NoLanes;
}
+
+ const expiredLanes = root.expiredLanes;
+ if (expiredLanes !== NoLanes) {
+ return getHighestPriorityLanes(expiredLanes);
+ }
let nextLanes = NoLanes;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fiber/lanes.ts` around lines 221 - 289, getNextLanes ignores
root.expiredLanes so lanes marked by markRootExpired never get chosen; fix by
reading root.expiredLanes at the start of getNextLanes and prioritizing any
expired pending lanes: compute expiredPendingLanes =
intersectLanes(pendingLanes, root.expiredLanes) and if expiredPendingLanes !==
NoLanes immediately return getHighestPriorityLanes(expiredPendingLanes) (place
this check before the suspended/non-idle/idle branching) so expired lanes always
preempt normal scheduling.
Comment on lines
+294
to
+296
| export function markRootUpdated(root: FiberRoot, updateLane: Lane): void { | ||
| root.pendingLanes = mergeLanes(root.pendingLanes, updateLane); | ||
| } |
Contributor
There was a problem hiding this comment.
New updates should unsuspend their lane, or you can stall forever.
Line 295 only adds to pendingLanes. If that lane was previously suspended/pinged, it stays blocked and can be skipped by getNextLanes. Clear it when new work arrives.
🛠️ Fix: clear suspended/pinged for the updated lane
export function markRootUpdated(root: FiberRoot, updateLane: Lane): void {
root.pendingLanes = mergeLanes(root.pendingLanes, updateLane);
+ root.suspendedLanes = removeLanes(root.suspendedLanes, updateLane);
+ root.pingedLanes = removeLanes(root.pingedLanes, updateLane);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function markRootUpdated(root: FiberRoot, updateLane: Lane): void { | |
| root.pendingLanes = mergeLanes(root.pendingLanes, updateLane); | |
| } | |
| export function markRootUpdated(root: FiberRoot, updateLane: Lane): void { | |
| root.pendingLanes = mergeLanes(root.pendingLanes, updateLane); | |
| root.suspendedLanes = removeLanes(root.suspendedLanes, updateLane); | |
| root.pingedLanes = removeLanes(root.pingedLanes, updateLane); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fiber/lanes.ts` around lines 294 - 296, markRootUpdated currently only
sets root.pendingLanes via mergeLanes(updateLane) but fails to clear any
suspended/pinged state for that lane, which can cause the new update to remain
blocked; update markRootUpdated to also remove the updateLane bit from the
root's suspended/pinged sets (e.g., root.suspendedLanes and root.pingedLanes) by
ANDing them with the inverse of updateLane after merging into root.pendingLanes
so the newly arriving work is unsuspended and eligible for getNextLanes.
Comment on lines
+344
to
+353
| export function scheduleUpdateOnFiber(fiber: Fiber, lane: Lane): void { | ||
| // Mark the fiber with the update lane | ||
| fiber.lanes = mergeLanes(fiber.lanes, lane); | ||
|
|
||
| // Bubble up to parents | ||
| let parent = fiber.return; | ||
| while (parent !== null) { | ||
| parent.childLanes = mergeLanes(parent.childLanes, lane); | ||
| parent = parent.return; | ||
| } |
Contributor
There was a problem hiding this comment.
You’re only scheduling one side of the fiber tree. That’s a correctness trap.
Line 346+ updates fiber but ignores fiber.alternate and parent alternates. When the current/work-in-progress swap happens, you can lose the lane markings and skip work.
🛠️ Fix: mirror lane updates to alternates
export function scheduleUpdateOnFiber(fiber: Fiber, lane: Lane): void {
// Mark the fiber with the update lane
fiber.lanes = mergeLanes(fiber.lanes, lane);
+ if (fiber.alternate !== null) {
+ fiber.alternate.lanes = mergeLanes(fiber.alternate.lanes, lane);
+ }
// Bubble up to parents
let parent = fiber.return;
while (parent !== null) {
parent.childLanes = mergeLanes(parent.childLanes, lane);
+ if (parent.alternate !== null) {
+ parent.alternate.childLanes = mergeLanes(parent.alternate.childLanes, lane);
+ }
parent = parent.return;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function scheduleUpdateOnFiber(fiber: Fiber, lane: Lane): void { | |
| // Mark the fiber with the update lane | |
| fiber.lanes = mergeLanes(fiber.lanes, lane); | |
| // Bubble up to parents | |
| let parent = fiber.return; | |
| while (parent !== null) { | |
| parent.childLanes = mergeLanes(parent.childLanes, lane); | |
| parent = parent.return; | |
| } | |
| export function scheduleUpdateOnFiber(fiber: Fiber, lane: Lane): void { | |
| // Mark the fiber with the update lane | |
| fiber.lanes = mergeLanes(fiber.lanes, lane); | |
| if (fiber.alternate !== null) { | |
| fiber.alternate.lanes = mergeLanes(fiber.alternate.lanes, lane); | |
| } | |
| // Bubble up to parents | |
| let parent = fiber.return; | |
| while (parent !== null) { | |
| parent.childLanes = mergeLanes(parent.childLanes, lane); | |
| if (parent.alternate !== null) { | |
| parent.alternate.childLanes = mergeLanes(parent.alternate.childLanes, lane); | |
| } | |
| parent = parent.return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fiber/lanes.ts` around lines 344 - 353, In scheduleUpdateOnFiber, you
only update fiber.lanes and parent.childLanes, which misses marking the
alternate tree; mirror the lane updates to fiber.alternate (if present) by
setting alternate.lanes = mergeLanes(alternate.lanes, lane), and for each parent
also update parent.alternate.childLanes =
mergeLanes(parent.alternate.childLanes, lane) when parent.alternate exists; use
the same mergeLanes utility and ensure you check for null/undefined alternates
while walking the return chain so both current and alternate trees retain the
new lane markings.
Comment on lines
+436
to
+448
| export function claimNextTransitionLane(): Lane { | ||
| const lane = 1 << currentTransitionLane; | ||
| currentTransitionLane = (currentTransitionLane + 1) % 31; | ||
|
|
||
| // Ensure we're in the transition lane range | ||
| if (lane < (TransitionLane1 as number)) { | ||
| return TransitionLane1; | ||
| } | ||
| if (lane > (TransitionLane2 as number)) { | ||
| return TransitionLane2; | ||
| } | ||
|
|
||
| return createLane(lane); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Transition lane rotation ignores the actual lane range.
Line 437 starts at bit 0 and then clamps to TransitionLane1/2, so you rotate through a bunch of useless values and this breaks the second you add a new transition lane. Just rotate through the actual constants.
♻️ Rotate through real transition lanes
-let currentTransitionLane = 0;
+const transitionLaneList: Lane[] = [TransitionLane1, TransitionLane2];
+let currentTransitionLaneIndex = 0;
export function claimNextTransitionLane(): Lane {
- const lane = 1 << currentTransitionLane;
- currentTransitionLane = (currentTransitionLane + 1) % 31;
-
- // Ensure we're in the transition lane range
- if (lane < (TransitionLane1 as number)) {
- return TransitionLane1;
- }
- if (lane > (TransitionLane2 as number)) {
- return TransitionLane2;
- }
-
- return createLane(lane);
+ const lane = transitionLaneList[currentTransitionLaneIndex];
+ currentTransitionLaneIndex =
+ (currentTransitionLaneIndex + 1) % transitionLaneList.length;
+ return lane;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fiber/lanes.ts` around lines 436 - 448, claimNextTransitionLane currently
generates lanes by shifting 1 by currentTransitionLane and then clamping, which
cycles through irrelevant bit positions; instead rotate only through the real
transition lane values by computing the lane as (TransitionLane1 shifted left by
currentTransitionLane) or by indexing into the sequence of transition lanes
between TransitionLane1 and TransitionLane2, and update currentTransitionLane
modulo the number of transition lanes; replace the existing
bit-1<<currentTransitionLane logic with this computed transition lane and return
createLane(lane) (use symbols claimNextTransitionLane, currentTransitionLane,
TransitionLane1, TransitionLane2, createLane).
Comment on lines
+500
to
+502
| export function formatLanes(lanes: Lanes): string { | ||
| return (lanes as number).toString(2).padStart(31, "0"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
padStart(31) is a magic number.
Line 501 hard-codes 31. That’s fine until someone adds/removes a lane. Please pull this into a constant (or derive from the highest lane bit) so format stays consistent with the actual lane layout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fiber/lanes.ts` around lines 500 - 502, The function formatLanes
currently uses a hard-coded padStart(31); update formatLanes to derive the width
from a single source of truth: either introduce a constant (e.g., MAX_LANE_BITS
or LANE_BIT_COUNT) or compute the bit width from the highest defined lane value
(e.g., determine the highest set bit of the Lanes type/enum) and use that value
in padStart. Modify the implementation in formatLanes (and any related
lane-definition file) so the padding value is not magic but comes from the new
constant or computed bit length, ensuring formatLanes stays correct when lanes
change.
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.
Add Lane type operations for priority scheduling: mergeLanes,
removeLanes, includesSomeLane, and lane management utilities.
Summary by CodeRabbit