Skip to content

Commit dce5f09

Browse files
committed
chore: address coderabbit feedback
1 parent adc490d commit dce5f09

File tree

13 files changed

+156
-158
lines changed

13 files changed

+156
-158
lines changed

docs/README.md

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ Welcome to the MiniReact design docs. These documents dive deep into how the fib
66

77
MiniReact is a simplified React clone built with a modern fiber architecture. It's not trying to be a production framework - it's an educational project that demonstrates how React works under the hood.
88

9-
The codebase is fully functional with 484 passing tests covering all major features. It's a great way to understand React's internals without getting lost in production complexity.
9+
The codebase is fully functional with 539 passing tests covering all major features. It's a great way to understand React's internals without getting lost in production complexity.
1010

1111
## Documentation Structure
1212

@@ -19,6 +19,7 @@ The docs are organized to build understanding progressively:
1919
Start here. This covers the fundamental concepts: what fibers are, why we have two trees, how the render and commit phases work, and how we traverse the tree without recursion.
2020

2121
Key topics:
22+
2223
- Fiber data structure
2324
- Double-buffer pattern
2425
- Render vs commit phases
@@ -30,6 +31,7 @@ Key topics:
3031
The work loop is the engine that drives everything. It schedules updates, builds the work-in-progress tree, and coordinates between phases.
3132

3233
Key topics:
34+
3335
- Processing units of work
3436
- Depth-first traversal
3537
- Effect collection
@@ -41,6 +43,7 @@ Key topics:
4143
Reconciliation is the diffing algorithm. It figures out what changed between renders and computes the minimal DOM operations needed.
4244

4345
Key topics:
46+
4447
- Key-based reconciliation
4548
- Movement detection
4649
- Handling mixed keyed/unkeyed children
@@ -54,6 +57,7 @@ Key topics:
5457
Events in MiniReact use delegation for efficiency. One listener at the root handles everything and routes events through the React tree.
5558

5659
Key topics:
60+
5761
- Event delegation
5862
- Synthetic events
5963
- Portal event bubbling
@@ -65,6 +69,7 @@ Key topics:
6569
Hooks let functional components have state and side effects. They're implemented as an array on each fiber with order maintained by a cursor.
6670

6771
Key topics:
72+
6873
- Hook storage and context
6974
- useState and useReducer
7075
- useEffect and cleanup
@@ -193,7 +198,7 @@ Current bottlenecks and future work:
193198

194199
## Testing
195200

196-
The test suite has 484 passing tests covering:
201+
The test suite has 539 passing tests covering:
197202

198203
- Core rendering and reconciliation
199204
- All hooks (useState, useEffect, useRef, etc.)
@@ -259,6 +264,6 @@ MiniReact implements:
259264
- Fragment support
260265
- Component memoization
261266

262-
All in about 3000 lines of readable TypeScript with 100% test coverage.
267+
All in about 3000 lines of readable TypeScript with comprehensive test coverage.
263268

264269
The goal is understanding, not production use. Fork it, break it, learn from it.

example/src/server.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { Elysia } from "elysia";
33
const PORT = Number(process.env.PORT) || 3000;
44

55
const app = new Elysia()
6-
.get("/app.js", async () => {
6+
.get("/app.js", () => {
77
const file = Bun.file("./public/app.js");
88
return new Response(file, {
99
headers: {
@@ -12,13 +12,18 @@ const app = new Elysia()
1212
},
1313
});
1414
})
15-
.get("/", async () => {
15+
.get("/", () => {
1616
const file = Bun.file("./src/templates/index.html");
17-
return new Response(file, {
18-
headers: {
19-
"Content-Type": "text/html",
20-
},
21-
});
17+
const headers: Record<string, string> = {
18+
"Content-Type": "text/html",
19+
};
20+
21+
// Add caching in production
22+
if (process.env.NODE_ENV === "production") {
23+
headers["Cache-Control"] = "public, max-age=3600";
24+
}
25+
26+
return new Response(file, { headers });
2227
})
2328
.listen(PORT);
2429

src/fiber/beginWork.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import type { AnyMiniReactElement, FunctionalComponent } from "../core/types";
1818
import { FRAGMENT, PORTAL, TEXT_ELEMENT } from "../core/types";
1919
import { cloneChildFibers } from "./fiberCreation";
20+
import { Deletion } from "./fiberFlags";
2021
import { setCurrentRenderingFiber } from "./fiberHooks";
2122
import { reconcileChildren } from "./reconcileChildren";
2223
import type { Fiber } from "./types";
@@ -245,6 +246,12 @@ function updatePortal(
245246
workInProgress.deletions = workInProgress.deletions || [];
246247
let child: Fiber | null = current.child;
247248
while (child !== null) {
249+
// Mark for deletion and clear effect pointers
250+
child.effectTag = Deletion;
251+
child.nextEffect = null;
252+
child.firstEffect = null;
253+
child.lastEffect = null;
254+
248255
workInProgress.deletions.push(child);
249256
child = child.sibling;
250257
}

src/fiber/fiberCreation.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import type { AnyMiniReactElement, ElementType } from "../core/types";
1010
import { FRAGMENT, PORTAL, TEXT_ELEMENT } from "../core/types";
1111
import { NoEffect, NoLanes } from "./fiberFlags";
12-
import type { Fiber, Props } from "./types";
12+
import { FIBER_BRAND, type Fiber, type Props } from "./types";
1313

1414
/**
1515
* Create a new Fiber instance
@@ -28,6 +28,9 @@ export function createFiber(
2828
key: string | null,
2929
): Fiber {
3030
const fiber: Fiber = {
31+
// Brand for runtime type checking
32+
[FIBER_BRAND]: true,
33+
3134
// Identity
3235
type,
3336
key,

src/fiber/fiberFlags.ts

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,29 +36,49 @@ export const Update = UpdateEffect;
3636
export const Deletion: EffectTag = 0b0100;
3737

3838
// ===== LANES (Priority System) =====
39+
// Using bitwise flags for fine-grained priority control
40+
// Lower bit position = higher priority
3941

4042
/**
4143
* No lanes - no work scheduled
4244
*/
43-
export const NoLanes: Lanes = 0;
45+
export const NoLanes: Lanes = 0b00000000;
4446

4547
/**
4648
* Sync lane - highest priority, must complete immediately
47-
* Used for user interactions (clicks, typing, etc)
49+
* Used for controlled inputs, urgent user interactions
4850
*/
49-
export const SyncLane: Lanes = 1;
51+
export const SyncLane: Lanes = 0b00000001;
5052

5153
/**
52-
* Default lane - normal priority
53-
* Used for data fetches, most updates
54+
* Input lane - high priority for discrete user input
55+
* Used for clicks, key presses, focus changes
5456
*/
55-
export const DefaultLane: Lanes = 2;
57+
export const InputLane: Lanes = 0b00000010;
5658

5759
/**
58-
* Idle lane - lowest priority
59-
* Used for non-critical updates that can be delayed
60+
* Default lane - normal priority for most updates
61+
* Used for data fetches, network responses, regular state updates
6062
*/
61-
export const IdleLane: Lanes = 4;
63+
export const DefaultLane: Lanes = 0b00000100;
64+
65+
/**
66+
* Transition lane - lower priority for transitions
67+
* Used for startTransition(), loading states, animations
68+
*/
69+
export const TransitionLane: Lanes = 0b00001000;
70+
71+
/**
72+
* Retry lane - for retrying failed updates
73+
* Used internally for suspense and error boundaries
74+
*/
75+
export const RetryLane: Lanes = 0b00010000;
76+
77+
/**
78+
* Idle lane - lowest priority for non-critical updates
79+
* Used for analytics, prefetching, offscreen rendering
80+
*/
81+
export const IdleLane: Lanes = 0b00100000;
6282

6383
// ===== HELPER FUNCTIONS =====
6484

@@ -106,9 +126,20 @@ export function isSubsetOfLanes(set: Lanes, subset: Lanes): boolean {
106126

107127
/**
108128
* Get the highest priority lane from a set
129+
*
130+
* Uses two's complement negation to isolate the lowest-order (rightmost) set bit.
131+
* In two's complement, -x = ~x + 1. When we AND x with -x, all bits cancel except
132+
* the rightmost set bit.
133+
*
134+
* Example: lanes = 0b0110 (InputLane | DefaultLane)
135+
* lanes: 0b0110
136+
* -lanes: 0b1010 (two's complement)
137+
* lanes & -lanes: 0b0010 (isolates rightmost set bit = InputLane)
138+
*
139+
* Since lower bit positions represent higher priority (SyncLane = 0b00000001 is highest),
140+
* the rightmost set bit is the highest priority lane.
109141
*/
110142
export function getHighestPriorityLane(lanes: Lanes): Lanes {
111-
// Return the rightmost (highest priority) bit
112143
return lanes & -lanes;
113144
}
114145

src/fiber/fiberRoot.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
*/
77

88
import { NoLanes } from "./fiberFlags";
9-
import type { Fiber, FiberRoot } from "./types";
9+
import { FIBER_BRAND, type Fiber, type FiberRoot } from "./types";
1010

1111
/**
1212
* WeakMap to track FiberRoots by their container elements
@@ -67,6 +67,9 @@ export function createFiberRoot(containerInfo: HTMLElement): FiberRoot {
6767
*/
6868
function createHostRootFiber(): Fiber {
6969
return {
70+
// Brand for runtime type checking
71+
[FIBER_BRAND]: true,
72+
7073
// Identity
7174
type: null, // Root has no type
7275
key: null,

src/fiber/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,13 @@ export {
4242
Deletion,
4343
NoLanes,
4444
SyncLane,
45+
InputLane,
4546
DefaultLane,
47+
TransitionLane,
48+
RetryLane,
4649
IdleLane,
4750
isEffectTagMutation,
51+
hasEffectTag,
4852
includesSomeLane,
4953
mergeLanes,
5054
removeLanes,

src/fiber/reconcileChildren.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,13 @@ function deleteChild(returnFiber: Fiber, childToDelete: Fiber): void {
501501
// Mark fiber for deletion
502502
childToDelete.effectTag = Deletion;
503503

504+
// Clear effect pointers to prevent stale chains
505+
// This ensures the deleted fiber doesn't keep pointing to old neighbors
506+
// which could resurrect stale effect chains or cause memory leaks
507+
childToDelete.nextEffect = null;
508+
childToDelete.firstEffect = null;
509+
childToDelete.lastEffect = null;
510+
504511
// Add to parent's deletion list
505512
if (!returnFiber.deletions) {
506513
returnFiber.deletions = [];

src/fiber/types.ts

Lines changed: 51 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,16 @@
77
*/
88

99
import type { AnyMiniReactElement, ElementType } from "../core/types";
10+
import { FRAGMENT, PORTAL, TEXT_ELEMENT } from "../core/types";
1011
import type { StateOrEffectHook } from "../hooks/types";
1112

13+
/**
14+
* Unique symbol used to brand Fiber objects for stronger runtime type checking.
15+
* This prevents false positives in isFiber() by ensuring the object was actually
16+
* created as a Fiber, not just any object with similar properties.
17+
*/
18+
export const FIBER_BRAND = Symbol.for("mini.react.fiber");
19+
1220
/**
1321
* Ref types for fiber
1422
*/
@@ -17,15 +25,29 @@ export type RefCallback<T> = (instance: T | null) => void;
1725
export type Ref = RefObject<unknown> | RefCallback<unknown> | null;
1826

1927
/**
20-
* Props type for fiber
21-
* Represents any props object that can be passed to a component
28+
* Base props that every component/element can have
2229
*/
23-
export type Props = {
30+
export interface BaseProps {
2431
children?: AnyMiniReactElement[];
2532
key?: string | number | null;
2633
ref?: Ref;
27-
[key: string]: unknown;
28-
};
34+
}
35+
36+
/**
37+
* Props type for fiber
38+
*
39+
* Combines base React props (children, key, ref) with arbitrary attributes.
40+
* The Record<string, unknown> allows:
41+
* - DOM attributes (id, className, style, etc.)
42+
* - Event handlers (onClick, onChange, etc.)
43+
* - Data attributes (data-*, aria-*)
44+
* - Custom component props
45+
*
46+
* Note: While this allows any string key, values should be serializable
47+
* or functions (for event handlers). Avoid passing complex objects or
48+
* circular references that can't be compared for reconciliation.
49+
*/
50+
export type Props = BaseProps & Record<string, unknown>;
2951

3052
/**
3153
* Effect tags indicate what kind of DOM operation needs to be performed
@@ -141,6 +163,12 @@ export interface UpdateQueue<State> {
141163
*/
142164
export interface Fiber {
143165
// ===== IDENTITY =====
166+
/**
167+
* Brand symbol for runtime type checking
168+
* This ensures the object was created as a Fiber, not just duck-typed
169+
*/
170+
readonly [FIBER_BRAND]: true;
171+
144172
/**
145173
* The type of this fiber's element
146174
* - string: host component (div, span, etc)
@@ -402,13 +430,25 @@ export interface FiberRoot {
402430

403431
/**
404432
* Type guard to check if a value is a Fiber
433+
*
434+
* Checks for the FIBER_BRAND first for strong typing, then falls back to
435+
* structural property checks for compatibility with older code or testing.
405436
*/
406437
export function isFiber(value: unknown): value is Fiber {
438+
// Quick null/primitive check
439+
if (value == null || typeof value !== "object") {
440+
return false;
441+
}
442+
443+
// Primary check: verify the brand symbol (strongest guarantee)
444+
if (FIBER_BRAND in value && value[FIBER_BRAND] === true) {
445+
return true;
446+
}
447+
448+
// Fallback: structural duck-typing for compatibility
449+
// This handles fibers created before the brand was added, or in tests
407450
return (
408-
value !== null &&
409-
typeof value === "object" &&
410451
"type" in value &&
411-
"props" in value &&
412452
"return" in value &&
413453
"child" in value &&
414454
"sibling" in value
@@ -433,29 +473,21 @@ export function isFiberFunctionComponent(fiber: Fiber): boolean {
433473
* Type guard to check if a fiber represents text content
434474
*/
435475
export function isFiberText(fiber: Fiber): boolean {
436-
return fiber.type === "TEXT_ELEMENT";
476+
return fiber.type === TEXT_ELEMENT;
437477
}
438478

439479
/**
440480
* Type guard to check if a fiber represents a fragment
441481
*/
442482
export function isFiberFragment(fiber: Fiber): boolean {
443-
// Import FRAGMENT symbol when needed
444-
return (
445-
typeof fiber.type === "symbol" &&
446-
fiber.type.toString() === "Symbol(react.fragment)"
447-
);
483+
return fiber.type === FRAGMENT;
448484
}
449485

450486
/**
451487
* Type guard to check if a fiber represents a portal
452488
*/
453489
export function isFiberPortal(fiber: Fiber): boolean {
454-
// Import PORTAL symbol when needed
455-
return (
456-
typeof fiber.type === "symbol" &&
457-
fiber.type.toString() === "Symbol(react.portal)"
458-
);
490+
return fiber.type === PORTAL;
459491
}
460492

461493
/**

0 commit comments

Comments
 (0)